-
Notifications
You must be signed in to change notification settings - Fork 482
Stop showing the component stack in case of an error, and show a more… #4526
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,9 +46,6 @@ | |
| } | ||
|
|
||
| .photon-message-bar-inner-text { | ||
| display: flex; | ||
| align-items: center; | ||
|
|
||
|
Comment on lines
-49
to
-51
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I looked closely at our example page, and I think that this wasn't useful after all, and this was in the way when I tried to add more than 1 child inside this element. So I removed them. But we will need to take care at regressions in some of our notification bars. |
||
| /* This padding is carefully computed so that multi-line message bars have the | ||
| * same top padding than single-line message bars. Having this property | ||
| * shouldn't change anything for single-line message bars. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,39 +13,19 @@ | |
| .appErrorBoundaryContents { | ||
| display: flex; | ||
| width: 100%; | ||
| max-width: 600px; | ||
| max-height: calc(100%); | ||
| max-width: 800px; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I gave some more space to the notice. |
||
| max-height: 100%; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and remove the |
||
| box-sizing: border-box; | ||
| flex-direction: column; | ||
| } | ||
|
|
||
| .appErrorBoundaryMessage { | ||
| flex: none; | ||
| } | ||
|
|
||
| .appErrorBoundaryDetails { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I forgot to remove those in the first version of the patch. |
||
| padding: 25px; | ||
| border-radius: 4px; | ||
| margin: 12px 0; | ||
| background: #fff; | ||
| box-shadow: 0 0 10px rgb(0 0 0 / 0.1); | ||
| color: var(--grey-70); | ||
| font-family: monospace; | ||
| line-height: 1.3; | ||
| opacity: 1; | ||
| overflow-y: auto; | ||
|
|
||
| /* Transition opacity, but do not delay changing the visibility. */ | ||
| transition: opacity 150ms, visibility 0s; | ||
| visibility: visible; | ||
| white-space: pre-wrap; | ||
| .appErrorBoundaryInnerText { | ||
| display: flex; | ||
| flex-direction: column; | ||
| gap: 4px; | ||
| } | ||
|
|
||
| .appErrorBoundaryDetails.hide { | ||
| opacity: 0; | ||
|
|
||
| /* Transition opacity, and create a delay on hiding the visibility to allow | ||
| time for the fade out. */ | ||
| transition: opacity 150ms, visibility 0s 150ms; | ||
| visibility: hidden; | ||
| .appErrorBoundaryInnerText > p { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wanted to use
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, it would be good to have a normalize.css. |
||
| padding: 0; | ||
| margin: 0; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,31 +4,35 @@ | |
| // @flow | ||
|
|
||
| import * as React from 'react'; | ||
| import { Localized } from '@fluent/react'; | ||
| import { reportError } from 'firefox-profiler/utils/analytics'; | ||
| import './ErrorBoundary.css'; | ||
|
|
||
| type State = {| | ||
| hasError: boolean, | ||
| showDetails: boolean, | ||
| errorString: string | null, | ||
| componentStack?: string, | ||
| |}; | ||
|
|
||
| type Props = {| | ||
| type ExternalProps = {| | ||
| +children: React.Node, | ||
| +message: string, | ||
| |}; | ||
|
|
||
| type InternalProps = {| | ||
| ...ExternalProps, | ||
| buttonContent: React.Node, | ||
| reportExplanationMessage: React.Node, | ||
| |}; | ||
|
|
||
| /** | ||
| * This component will catch errors in components, and display a more friendly error | ||
| * message. This stops the entire app from unmounting and displaying an empty screen. | ||
| * | ||
| * See: https://reactjs.org/docs/error-boundaries.html | ||
| */ | ||
| export class ErrorBoundary extends React.Component<Props, State> { | ||
| class ErrorBoundaryInternal extends React.Component<InternalProps, State> { | ||
| state = { | ||
| hasError: false, | ||
| showDetails: false, | ||
| errorString: null, | ||
| }; | ||
|
|
||
|
|
@@ -52,7 +56,10 @@ export class ErrorBoundary extends React.Component<Props, State> { | |
| errorString = result; | ||
| } | ||
| } | ||
| this.setState({ hasError: true, errorString, componentStack }); | ||
| this.setState({ | ||
| hasError: true, | ||
| errorString, | ||
| }); | ||
| reportError({ | ||
| exDescription: errorString | ||
| ? errorString + '\n' + componentStack | ||
|
|
@@ -61,35 +68,26 @@ export class ErrorBoundary extends React.Component<Props, State> { | |
| }); | ||
| } | ||
|
|
||
| _toggleErrorDetails = () => { | ||
| this.setState((state) => ({ showDetails: !state.showDetails })); | ||
| }; | ||
|
|
||
| render() { | ||
| if (this.state.hasError) { | ||
| const { errorString, componentStack, showDetails } = this.state; | ||
| const { errorString } = this.state; | ||
| return ( | ||
| <div className="appErrorBoundary"> | ||
| <div className="appErrorBoundaryContents"> | ||
| <div className="photon-message-bar photon-message-bar-error photon-message-bar-inner-content appErrorBoundaryMessage"> | ||
| <div className="photon-message-bar-inner-text"> | ||
| {this.props.message} | ||
| <div className="photon-message-bar photon-message-bar-error photon-message-bar-inner-content"> | ||
| <div className="photon-message-bar-inner-text appErrorBoundaryInnerText"> | ||
| <p>{this.props.message}</p> | ||
| {errorString ? <p>{errorString}.</p> : null} | ||
| <p>{this.props.reportExplanationMessage}</p> | ||
| </div> | ||
| <button | ||
| <a | ||
| className="photon-button photon-button-micro photon-message-bar-action-button" | ||
| type="button" | ||
| onClick={this._toggleErrorDetails} | ||
| aria-expanded={showDetails ? 'true' : 'false'} | ||
| href="https://github.com/firefox-devtools/profiler/issues/new?title=An%20error%20occurred%20in%20Firefox%20Profiler" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can even populate the content of the bug to include the error message etc. But we can do that later. |
||
| target="_blank" | ||
| rel="noreferrer" | ||
| > | ||
| {showDetails ? 'Hide error details' : 'View full error details'} | ||
| </button> | ||
| </div> | ||
| <div | ||
| data-testid="error-technical-details" | ||
| className={`appErrorBoundaryDetails ${showDetails ? '' : 'hide'}`} | ||
| > | ||
| {errorString ? <div>{errorString}</div> : null} | ||
| {componentStack ? <div>{componentStack}</div> : null} | ||
| {this.props.buttonContent} | ||
| </a> | ||
| </div> | ||
| </div> | ||
| </div> | ||
|
|
@@ -99,3 +97,38 @@ export class ErrorBoundary extends React.Component<Props, State> { | |
| return this.props.children; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Use this error boundary when outside of the AppLocalizationProvider hierarchy | ||
| */ | ||
| export function NonLocalizedErrorBoundary(props: ExternalProps) { | ||
| return ( | ||
| <ErrorBoundaryInternal | ||
| {...props} | ||
| buttonContent="Report the error on GitHub" | ||
| reportExplanationMessage="Please report this error to the developers, including the full error as displayed in the Developer Tools’ Web Console." | ||
| /> | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Use this error boundary when inside of the AppLocalizationProvider hierarchy | ||
| */ | ||
| export function LocalizedErrorBoundary(props: ExternalProps) { | ||
| return ( | ||
| <ErrorBoundaryInternal | ||
| {...props} | ||
| buttonContent={ | ||
| <Localized id="ErrorBoundary--report-error-on-github"> | ||
| Report the error on GitHub | ||
| </Localized> | ||
| } | ||
| reportExplanationMessage={ | ||
| <Localized id="ErrorBoundary--report-error-to-developers-description"> | ||
| Please report this issue to the developers, including the full error | ||
| as displayed in the Developer Tools’ Web Console. | ||
| </Localized> | ||
| } | ||
| /> | ||
| ); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some more comments here as well.