From 3d7086073b65482d8cdf585c4cd79b63156e9fa0 Mon Sep 17 00:00:00 2001 From: Julien Wajsberg Date: Mon, 13 Mar 2023 14:30:07 +0100 Subject: [PATCH 1/2] Stop showing the component stack in case of an error, and show a more helpful message --- locales/en-US/app.ftl | 19 ++++- res/css/photon/button.css | 6 ++ src/components/app/Details.js | 10 +-- src/components/app/ErrorBoundary.js | 81 +++++++++++++------ src/components/app/Root.js | 35 +++++--- src/test/components/ErrorBoundary.test.js | 69 ++++++++-------- .../__snapshots__/ErrorBoundary.test.js.snap | 68 +++++++++++----- 7 files changed, 190 insertions(+), 98 deletions(-) diff --git a/locales/en-US/app.ftl b/locales/en-US/app.ftl index d6b97ecd6c..32167f9743 100644 --- a/locales/en-US/app.ftl +++ b/locales/en-US/app.ftl @@ -222,8 +222,18 @@ Details--open-sidebar-button = .title = Open the sidebar Details--close-sidebar-button = .title = Close the sidebar -Details--error-boundary-message = - .message = Uh oh, some unknown error happened in this panel. +Details--error-boundary-message2 = + .message = Uh oh, some unknown error happened in this panel + +## ErrorBoundary +## This component is shown when an unexpected error is encountered in the application. +## Note that the localization won't be always applied in this component. + +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. + +ErrorBoundary--report-error-on-github = Report the error on GitHub ## Footer Links @@ -668,6 +678,11 @@ ProfileLoaderAnimation--loading-view-not-found = View not found ProfileRootMessage--title = { -profiler-brand-name } ProfileRootMessage--additional = Back to home +## Root + +Root--error-boundary-message = + .message = Uh oh, some error happened in profiler.firefox.com + ## ServiceWorkerManager ## This is the component responsible for handling the service worker installation ## and update. It appears at the top of the UI. diff --git a/res/css/photon/button.css b/res/css/photon/button.css index 9a37ddf7d2..e335b97b9b 100644 --- a/res/css/photon/button.css +++ b/res/css/photon/button.css @@ -4,6 +4,9 @@ /* See https://design.firefox.com/photon/components/buttons.html for the spec */ .photon-button { + /* These two flex options aren't necessary when a real - -
- {errorString ?
{errorString}
: null} - {componentStack ?
{componentStack}
: null} + {this.props.buttonContent} +
@@ -99,3 +97,38 @@ export class ErrorBoundary extends React.Component { return this.props.children; } } + +/** + * Use this error boundary when outside of the AppLocalizationProvider hierarchy + */ +export function NonLocalizedErrorBoundary(props: ExternalProps) { + return ( + + ); +} + +/** + * Use this error boundary when inside of the AppLocalizationProvider hierarchy + */ +export function LocalizedErrorBoundary(props: ExternalProps) { + return ( + + Report the error on GitHub + + } + reportExplanationMessage={ + + Please report this issue to the developers, including the full error + as displayed in the Developer Tools’ Web Console. + + } + /> + ); +} diff --git a/src/components/app/Root.js b/src/components/app/Root.js index ec9c820875..35b995b3fe 100644 --- a/src/components/app/Root.js +++ b/src/components/app/Root.js @@ -4,10 +4,14 @@ // @flow import React, { PureComponent } from 'react'; +import { Localized } from '@fluent/react'; import { Provider } from 'react-redux'; import { UrlManager } from './UrlManager'; import { FooterLinks } from './FooterLinks'; -import { ErrorBoundary } from './ErrorBoundary'; +import { + NonLocalizedErrorBoundary, + LocalizedErrorBoundary, +} from './ErrorBoundary'; import { AppViewRouter } from './AppViewRouter'; import { ProfileLoader } from './ProfileLoader'; import { ServiceWorkerManager } from './ServiceWorkerManager'; @@ -28,21 +32,28 @@ export class Root extends PureComponent { render() { const { store } = this.props; return ( - + - - - - - - - - - + + + + + + + + + + + + + - + ); } } diff --git a/src/test/components/ErrorBoundary.test.js b/src/test/components/ErrorBoundary.test.js index 10962907fc..4f86fde978 100644 --- a/src/test/components/ErrorBoundary.test.js +++ b/src/test/components/ErrorBoundary.test.js @@ -4,15 +4,16 @@ // @flow import * as React from 'react'; -import { screen } from '@testing-library/react'; import { stripIndent } from 'common-tags'; import { render } from 'firefox-profiler/test/fixtures/testing-library'; -import { ErrorBoundary } from '../../components/app/ErrorBoundary'; +import { + NonLocalizedErrorBoundary, + LocalizedErrorBoundary, +} from '../../components/app/ErrorBoundary'; import { withAnalyticsMock } from '../fixtures/mocks/analytics'; -import { fireFullClick } from '../fixtures/utils'; -describe('app/ErrorBoundary', function () { +describe('app/NonLocalizedErrorBoundary', function () { const childComponentText = 'This is a child component'; const friendlyErrorMessage = 'Oops, there was an error'; const technicalErrorMessage = 'This is an error.'; @@ -23,28 +24,15 @@ describe('app/ErrorBoundary', function () { function setupComponent(childComponent) { const spy = jest.spyOn(console, 'error').mockImplementation(() => {}); const results = render( - + {childComponent} - + ); return { spy, ...results }; } it('matches the snapshot', () => { const { container } = setupComponent(); - - // We need to change the textContent of the stack, so that path information - // isn't tied to a specific environment. - const stack = screen.getByText(/at ThrowingComponent/); - stack.textContent = stack.textContent - .replace(/\\/g, '/') // normalizes Windows paths - .replace(/\(.*\/src/g, '(REDACTED)/src') // Removes the home directory - // Removes the home directory and line / column numbers of vendored files. - .replace( - /\(.*\/node_modules\/([^:]*):.+/g, - '(REDACTED)/node_modules/$1:(REDACTED)' - ); - expect(container.firstChild).toMatchSnapshot(); }); @@ -55,7 +43,7 @@ describe('app/ErrorBoundary', function () { it('shows the error message children when the component throws error', () => { const { getByText } = setupComponent(); - expect(getByText(friendlyErrorMessage)).toBeInTheDocument(); + expect(getByText(new RegExp(friendlyErrorMessage))).toBeInTheDocument(); }); it('surfaces the error via console.error', () => { @@ -63,19 +51,6 @@ describe('app/ErrorBoundary', function () { expect(console.error).toHaveBeenCalled(); }); - it('can click the component to view the full details', () => { - const { getByText, getByTestId } = setupComponent(); - - // The technical error isn't visible yet. - expect(getByTestId('error-technical-details')).toHaveClass('hide'); - - // Click the button to expand the details. - fireFullClick(getByText('View full error details')); - - // The technical error now exists. - expect(getByTestId('error-technical-details')).not.toHaveClass('hide'); - }); - it('reports errors to the analytics', () => { withAnalyticsMock(() => { setupComponent(); @@ -87,8 +62,9 @@ describe('app/ErrorBoundary', function () { new RegExp(stripIndent` Error: This is an error\\. - at ThrowingComponent \\(.*[/\\\\]ErrorBoundary.test.js:20:11\\) - at ErrorBoundary \\(.*[/\\\\]ErrorBoundary.js:28:66\\) + at ThrowingComponent \\(.*[/\\\\]ErrorBoundary.test.js:.*\\) + at ErrorBoundaryInternal \\(.*[/\\\\]ErrorBoundary.js:.*\\) + at NonLocalizedErrorBoundary at LocalizationProvider \\(.*[/\\\\]@fluent[/\\\\]react[/\\\\]index.js:.*\\) `) ), @@ -97,3 +73,26 @@ describe('app/ErrorBoundary', function () { }); }); }); + +describe('app/LocalizedErrorBoundary', function () { + const friendlyErrorMessage = 'Oops, there was an error'; + const technicalErrorMessage = 'This is an error.'; + const ThrowingComponent = () => { + throw new Error(technicalErrorMessage); + }; + + function setupComponent(childComponent) { + const spy = jest.spyOn(console, 'error').mockImplementation(() => {}); + const results = render( + + {childComponent} + + ); + return { spy, ...results }; + } + + it('matches the snapshot', () => { + setupComponent(); + expect(document.body).toMatchSnapshot(); + }); +}); diff --git a/src/test/components/__snapshots__/ErrorBoundary.test.js.snap b/src/test/components/__snapshots__/ErrorBoundary.test.js.snap index 7d227db6c4..f622898dc1 100644 --- a/src/test/components/__snapshots__/ErrorBoundary.test.js.snap +++ b/src/test/components/__snapshots__/ErrorBoundary.test.js.snap @@ -1,6 +1,43 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`app/ErrorBoundary matches the snapshot 1`] = ` +exports[`app/LocalizedErrorBoundary matches the snapshot 1`] = ` + +
+
+
+
+
+ Oops, there was an error + (Error: This is an error.) + . + + Please report this issue to the developers, including the full +error as displayed in the Developer Tools’ Web Console. +
+ + Report the error on GitHub + +
+
+
+
+ +`; + +exports[`app/NonLocalizedErrorBoundary matches the snapshot 1`] = `
@@ -14,28 +51,19 @@ exports[`app/ErrorBoundary matches the snapshot 1`] = ` class="photon-message-bar-inner-text" > Oops, there was an error + (Error: This is an error.) + . + + Please report this error to the developers, including the full error as displayed in the Developer Tools’ Web Console.
- - -
-
- Error: This is an error. -
-
- - at ThrowingComponent (REDACTED)/src/test/components/ErrorBoundary.test.js:20:11) - at ErrorBoundary (REDACTED)/src/components/app/ErrorBoundary.js:28:66) - at LocalizationProvider (REDACTED)/node_modules/@fluent/react/index.js:(REDACTED) -
+ Report the error on GitHub +
From 91259a423f423066de6baa86c1e66c0a68e3c302 Mon Sep 17 00:00:00 2001 From: Julien Wajsberg Date: Wed, 15 Mar 2023 14:33:56 +0100 Subject: [PATCH 2/2] Fix review comments --- locales/en-US/app.ftl | 8 ++-- res/css/photon/message-bar.css | 3 -- src/components/app/Details.js | 2 +- src/components/app/ErrorBoundary.css | 38 +++++-------------- src/components/app/ErrorBoundary.js | 10 ++--- src/components/app/Root.js | 4 +- .../__snapshots__/ErrorBoundary.test.js.snap | 38 ++++++++++++------- 7 files changed, 46 insertions(+), 57 deletions(-) diff --git a/locales/en-US/app.ftl b/locales/en-US/app.ftl index 32167f9743..18b1b1b99a 100644 --- a/locales/en-US/app.ftl +++ b/locales/en-US/app.ftl @@ -222,17 +222,19 @@ Details--open-sidebar-button = .title = Open the sidebar Details--close-sidebar-button = .title = Close the sidebar -Details--error-boundary-message2 = - .message = Uh oh, some unknown error happened in this panel +Details--error-boundary-message = + .message = Uh oh, some unknown error happened in this panel. ## ErrorBoundary ## This component is shown when an unexpected error is encountered in the application. ## Note that the localization won't be always applied in this component. +# This message will always be displayed after another context-specific message. 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. +# This is used in a call to action button, displayed inside the error box. ErrorBoundary--report-error-on-github = Report the error on GitHub ## Footer Links @@ -681,7 +683,7 @@ ProfileRootMessage--additional = Back to home ## Root Root--error-boundary-message = - .message = Uh oh, some error happened in profiler.firefox.com + .message = Uh oh, some unknown error happened in profiler.firefox.com. ## ServiceWorkerManager ## This is the component responsible for handling the service worker installation diff --git a/res/css/photon/message-bar.css b/res/css/photon/message-bar.css index 181a4f8b9d..b119857f3b 100644 --- a/res/css/photon/message-bar.css +++ b/res/css/photon/message-bar.css @@ -46,9 +46,6 @@ } .photon-message-bar-inner-text { - display: flex; - align-items: center; - /* 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. diff --git a/src/components/app/Details.js b/src/components/app/Details.js index be68635815..193a194d02 100644 --- a/src/components/app/Details.js +++ b/src/components/app/Details.js @@ -103,7 +103,7 @@ class ProfileViewerImpl extends PureComponent { ) : null} p { + padding: 0; + margin: 0; } diff --git a/src/components/app/ErrorBoundary.js b/src/components/app/ErrorBoundary.js index 1e1bf52750..54ce4270f7 100644 --- a/src/components/app/ErrorBoundary.js +++ b/src/components/app/ErrorBoundary.js @@ -74,11 +74,11 @@ class ErrorBoundaryInternal extends React.Component { return (
-
-
- {this.props.message} - {errorString ? ` (${errorString})` : null}.{' '} - {this.props.reportExplanationMessage} +
+
+

{this.props.message}

+ {errorString ?

{errorString}.

: null} +

{this.props.reportExplanationMessage}

{ render() { const { store } = this.props; return ( - + - + diff --git a/src/test/components/__snapshots__/ErrorBoundary.test.js.snap b/src/test/components/__snapshots__/ErrorBoundary.test.js.snap index f622898dc1..cdd9bfdbbf 100644 --- a/src/test/components/__snapshots__/ErrorBoundary.test.js.snap +++ b/src/test/components/__snapshots__/ErrorBoundary.test.js.snap @@ -10,17 +10,22 @@ exports[`app/LocalizedErrorBoundary matches the snapshot 1`] = ` class="appErrorBoundaryContents" >
- Oops, there was an error - (Error: This is an error.) - . - - Please report this issue to the developers, including the full +

+ Oops, there was an error +

+

+ Error: This is an error. + . +

+

+ Please report this issue to the developers, including the full error as displayed in the Developer Tools’ Web Console. +

- Oops, there was an error - (Error: This is an error.) - . - - Please report this error to the developers, including the full error as displayed in the Developer Tools’ Web Console. +

+ Oops, there was an error +

+

+ Error: This is an error. + . +

+

+ Please report this error to the developers, including the full error as displayed in the Developer Tools’ Web Console. +