Add error boundary to the Settings page #6320

delawski · 2021-05-31T12:53:53Z

Summary

This PR adds the error boundary and error screen components from the Onboarding Wizard to the AMP Settings page. The error screen is also extended. It now shows not only the error message but also the stack information. Some cosmetic updates to the look and feel of the error message have been done as well.

Onboarding Wizard	AMP Settings

Onboarding Wizard before	Onboarding Wizard after

Fixes #6230

Checklist

My code is tested and passes existing tests.
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

delawski · 2021-05-31T12:59:55Z

We should wrap this with a try/catch block to show an error message with the details, including a link to submit a support topic with the associated stack trace.

@westonruter In the current iteration I haven't included the "submit a support topic" button you asked for. Should this button create a ticker on wordpress.org? If yes, is it possible to create a ticket with a predefined topic/content? I briefly searched for something like this but couldn't find anything relevant.

github-actions · 2021-05-31T13:22:59Z

Plugin builds for 555324b are ready 🛎️!

Download development build
Download production build

pierlon · 2021-06-04T09:45:48Z

assets/src/onboarding-wizard/index.js

-			<ErrorBoundary exitLink={ FINISH_LINK } fullScreen={ true }>
-
+			<ErrorBoundary
+				exitLink={ {


The prop type for exitLink is a string, but you're passing an object here.

Good catch. In fact, after addressing your feedback on the finishLink prop, this becomes outdated.

pierlon · 2021-06-04T09:49:22Z

assets/src/components/error-screen/index.js

@@ -19,27 +19,38 @@ import './style.css';
 *
 * @param {Object} props Component props.
 * @param {Object} props.error Error object containing a message string.
- * @param {string} props.finishLink The link to return to the admin.
+ * @param {Object} props.finishLink The link to return to the admin.


Accepting an object for a prop named finishLink seems like a misnomer. Accepting the link and label as separate props seems more appropriate.

I agree. I've broken down the finishLink object into 2 separate props in 81e29d8.

pierlon · 2021-06-04T09:57:07Z

Maybe we could add a button to copy the error stack-trace as well, similar to the one in the block editor.

delawski · 2021-06-08T16:33:16Z

Maybe we could add a button to copy the error stack-trace as well, similar to the one in the block editor.

@pierlon I think it's a good idea. I've migrated the (now deprecated) ClipboardButton from @wordpress/components (it's based on the useCopyToClipboard() custom hook that is proposed as a replacement in the deprecation notice).

Here's how it looks:

westonruter · 2021-06-10T04:25:08Z

PHPUnit failures to be fixed by #6366.

westonruter · 2021-06-10T05:57:29Z

You can rebase now against develop to fix the failures.

pierlon · 2021-06-10T22:58:24Z

Great work @delawski. One thing left here is to catch and handle any errors that may occur outside of the error boundary, like in #6230 where an incorrect version of a dependency was loaded, causing the settings page components not to render. I think we can accomplish this by adding an error event listener to the page that would then render the error screen.

So maybe something like this:

diff --git a/assets/src/settings-page/index.js b/assets/src/settings-page/index.js
--- a/assets/src/settings-page/index.js	(revision 983e5e106759bb2aecd845e958494c299c850a31)
+++ b/assets/src/settings-page/index.js	(date 1623357057438)
@@ -217,6 +217,8 @@
 	const root = document.getElementById( 'amp-settings-root' );
 
 	if ( root ) {
+		global.addEventListener( 'error', ( error ) => wp.element.render( <ErrorScreen error={ error } /> ), root );
+
 		render( (
 			<Providers>
 				<Root appRoot={ root } />

I'll see if this actually works...

westonruter · 2021-06-10T23:34:55Z

I think we can accomplish this by adding an error event listener to the page that would then render the error screen.

Maybe this event listener should be removed once the app has successfully initialized. We wouldn't want errors caused by other random JS on the page to prevent the settings screen from rendering.

Or rather, there could be a 5 second timeout after which if the app does not initialize, then allow whatever error(s) were caught by the error handler to be rendered. Upon initialization, the error event handler should then be removed, leaving only the error boundary error catcher in place.

delawski · 2021-06-11T14:43:46Z

Those are some really good points. I'll dig into that 👍

delawski · 2021-06-15T09:11:43Z

As suggested by @pierlon, in dea9fd4 we handle errors that are thrown outside of the error boundary by listening to the global error event.

The event listener is set up before the app is rendered. In the render() function's callback, the event listener is removed.

We wouldn't want errors caused by other random JS on the page to prevent the settings screen from rendering.

@westonruter It seems that we can filter out "our own" errors by examining the event.filename string. It tells us where the error is coming from.

Or rather, there could be a 5 second timeout after which if the app does not initialize, then allow whatever error(s) were caught by the error handler to be rendered. Upon initialization, the error event handler should then be removed, leaving only the error boundary error catcher in place.

I think that checking the event.filename combined with the error handler removal in the render function callback should do the trick.

In order to build and test out a global error handler as suggested in the previous comments, I first had to simulate an error similar the one in #6230 (a white screen of death).

Check out my test steps

First, I've added a dummy function to the compiled package node_modules/@wordpress/i18n/build-types/index.d.ts:

export { sprintf } from "./sprintf";
export * from "./create-i18n";
+ export function Foo() {
+ 	return null;
+ };
export { default as defaultI18n, setLocaleData, resetLocaleData, getLocaleData, subscribe, __, _x, _n, _nx, isRTL, hasTranslation } from "./default-i18n";
//# sourceMappingURL=index.d.ts.map

Thanks to that, webpack had no issues with importing that dummy function to our entry points:

- import { __ } from '@wordpress/i18n';
+ import { __, Foo } from '@wordpress/i18n';

Then, I've added a call to that function before the render function (so before we have the error boundary set up):

domReady( () => {
	const root = document.getElementById( 'amp-settings-root' );

+	Foo();

	if ( root ) {
		render( (
			<Providers>
				<Root appRoot={ root } />
			</Providers>
		), root );
	}
} );

Since in the WP admin the Core version of the i18n package is used, Foo becomes a missing dependency which leads to a white screen of death.

pierlon · 2021-06-16T20:23:55Z

assets/src/onboarding-wizard/index.js

+
+	const errorHandler = ( error ) => {
+		// Handle only own errors.
+		if ( error?.filename?.match( /amp-onboarding-wizard(\.min)?\.js/ ) ) {


This would be more appropriate since we're not making use of the matches:

Suggested change

if ( error?.filename?.match( /amp-onboarding-wizard(\.min)?\.js/ ) ) {

if ( /amp-onboarding-wizard(\.min)?\.js/.test( error?.filename ) ) {

pierlon · 2021-06-16T20:25:33Z

assets/src/onboarding-wizard/index.js

 	}
+
+	const errorHandler = ( error ) => {


This is actually the ErrorEvent, not the error object, so we could use a more appropriate name:

Suggested change

const errorHandler = ( error ) => {

const errorHandler = ( event ) => {

pierlon · 2021-06-16T20:26:25Z

assets/src/onboarding-wizard/index.js

+	const errorHandler = ( error ) => {
+		// Handle only own errors.
+		if ( error?.filename?.match( /amp-onboarding-wizard(\.min)?\.js/ ) ) {
+			render( <ErrorScreen error={ error } />, root );


The error event is passed here, instead of the error object, so this should actually be:

Suggested change

render( <ErrorScreen error={ error } />, root );

render( <ErrorScreen error={ error.error } />, root );

pierlon · 2021-06-16T20:30:08Z

assets/src/onboarding-wizard/index.js

+			<SetupWizard closeLink={ CLOSE_LINK } finishLink={ FINISH_LINK } appRoot={ root } />
+		</Providers>,
+		root,
+		() => global.removeEventListener( 'error', errorHandler ),


According to the React docs the callback is executed after the component is rendered or updated, which means the error boundary will not be handling errors until all renders are complete. Maybe we could remove the listener in the Providers component?

pierlon · 2021-06-16T20:31:21Z

assets/src/settings-page/index.js

 	}
+
+	const errorHandler = ( error ) => {
+		// Handle only own errors.


Same comments above apply for this file as well.

pierlon

I've made my suggestions in fa13a7f, feel free to modify if needed. That said, I think this is ready to ship .

pierlon · 2021-06-16T20:45:25Z

Oops one more thing:

The error stacktrace is not being displayed correctly in Firefox. Given this change:

diff --git a/assets/src/onboarding-wizard/index.js b/assets/src/onboarding-wizard/index.js
--- a/assets/src/onboarding-wizard/index.js	(revision fa13a7f5ef7d2fe6dc93c6ca113357f4c1007868)
+++ b/assets/src/onboarding-wizard/index.js	(date 1623874797950)
@@ -112,6 +112,8 @@
 
 	global.addEventListener( 'error', errorHandler );
 
+	foo();
+
 	render(
 		<Providers>
 			<SetupWizard closeLink={ CLOSE_LINK } finishLink={ FINISH_LINK } appRoot={ root } />

Here's what the error screen looks like in Firefox vs Chrome:

Firefox	Chrome

Both browsers outputs error.stack differently, and in Firefox you can see that part of the stacktrace is incorrectly being rendered as an HTML comment.

pierlon · 2021-06-16T20:51:29Z

Result after 2a4b083:

delawski · 2021-06-17T08:22:49Z

@pierlon Thank you for the feedback and for addressing it right away. I've updated the ErrorScreen snapshot and pushed it so that the unit tests should now pass.

westonruter · 2021-06-17T22:02:13Z

I just tried this without the fix from #6396:

Chrome	Firefox

It seems like in Firefox the error message is not included in the details.

pierlon · 2021-06-17T22:06:06Z

Maybe the JS sourcemap isn't being detected properly in Firefox?

westonruter · 2021-06-17T22:11:32Z

I'm talking about the contents of the details. In Chrome the "TypeError: Cannot read property 'edit_link' of undefined" message is part of the details. In Firefox it is not. This means the error message is not copied to the clipboard in Firefox.

pierlon · 2021-06-17T22:19:01Z

What about something like:

diff --git a/assets/src/components/error-screen/index.js b/assets/src/components/error-screen/index.js
--- a/assets/src/components/error-screen/index.js	(revision 2a4b083eb11bdd802ede455a0fcda1a160ad3a5f)
+++ b/assets/src/components/error-screen/index.js	(date 1623968168232)
@@ -27,6 +27,7 @@
  */
 export function ErrorScreen( { error, finishLinkLabel, finishLinkUrl, title } ) {
 	const [ hasCopied, setHasCopied ] = useState( false );
+	const { message, stack } = error;
 
 	return (
 		<div className="error-screen-container">
@@ -37,21 +38,21 @@
 
 				{ /* dangerouslySetInnerHTML reason: WordPress sometimes sends back HTML in error messages. */ }
 				<p dangerouslySetInnerHTML={ {
-					__html: error.message || __( 'There was an error loading the page.', 'amp' ),
+					__html: message || __( 'There was an error loading the page.', 'amp' ),
 				} } />
 
-				{ error?.stack && (
+				{ stack && (
 					<details>
 						<summary>
 							{ __( 'Details', 'amp' ) }
 						</summary>
 						<pre>
-							{ error.stack }
+							{ stack }
 						</pre>
 						<ClipboardButton
 							isSmall={ true }
 							isSecondary={ true }
-							text={ error.stack }
+							text={ JSON.stringify( { message, stack }, null, 2 ) }
 							onCopy={ () => setHasCopied( true ) }
 							onFinishCopy={ () => setHasCopied( false ) }
 						>

Which would copy the error message and stack.

westonruter · 2021-06-17T22:40:20Z

Yeah, that looks good.

pierlon · 2021-06-17T22:45:16Z

Done in 94d183e.

westonruter · 2021-06-22T20:52:53Z

(Rebased against develop.)

westonruter · 2021-06-22T22:53:42Z

@delawski @pierlon I made a few more additional improvements:

I increased the timeout to show the uncaught error message from 5 to 30 seconds, as this accounts for a slow connections.
When JS is disabled the no-JS message is now shown immediately without having to wait for the animation timeout.
When JS is disabled, I also hid the spinners for the validation counts since they will never be populated.
I added a spinner.gif to show in the HTML before the Loading JS component is rendered. This ensures that there is a spinner showing on the screen to let them know to wait (for up to 30 seconds).
I scoped the box-sizing property in 3d636b3 because it was causing the validation error count loading animation in the admin menu to become mistyled.

Successful Load

Notice how there are two spinners: a pre-loading spinner and a loading spinner.

amp-settings-load-success.mov

Failure due to JS disabled

Failure with caught error

Here I tested by throwing an error at the top of api-fetch.js:

amp-settings-load-failure-error-caught.mov

Failure with uncaught error

Here I tested by throwing an error at the top of lodash.js (note this takes 30 seconds):

amp-settings-load-failure-error-uncaught.mov

westonruter

Great work!

@delawski @pierlon please review the additional changes and I'll merge tomorrow once all verified.

codecov · 2021-06-23T03:39:41Z

Codecov Report

Merging #6320 (8484586) into develop (a958b43) will increase coverage by 0.18%.
The diff coverage is 75.00%.

❗ Current head 8484586 differs from pull request most recent head 26c1206. Consider uploading reports for the commit 26c1206 to get more accurate results

@@              Coverage Diff              @@
##             develop    #6320      +/-   ##
=============================================
+ Coverage      75.26%   75.45%   +0.18%     
- Complexity      5899     5900       +1     
=============================================
  Files            187      237      +50     
  Lines          17076    17867     +791     
=============================================
+ Hits           12852    13481     +629     
- Misses          4224     4386     +162

Flag	Coverage Δ
javascript	`79.15% <50.00%> (?)`
php	`75.28% <100.00%> (+0.02%)`	⬆️
unit	`75.28% <100.00%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/AmpWpPlugin.php	`100.00% <ø> (ø)`
assets/src/components/clipboard-button/index.js	`50.00% <50.00%> (ø)`
assets/src/components/error-screen/index.js	`66.66% <50.00%> (ø)`
...s/validation/class-amp-validated-url-post-type.php	`66.24% <100.00%> (ø)`
...validation/class-amp-validation-error-taxonomy.php	`50.20% <100.00%> (ø)`
src/Admin/OnboardingWizardSubmenuPage.php	`84.04% <100.00%> (+0.34%)`	⬆️
src/Admin/OptionsMenu.php	`33.33% <100.00%> (+1.33%)`	⬆️
src/LoadingError.php	`100.00% <100.00%> (ø)`
assets/src/components/svg/check.js	`100.00% <0.00%> (ø)`
...idation/components/error/get-error-source-title.js	`93.75% <0.00%> (ø)`
... and 48 more

delawski · 2021-06-23T15:08:18Z

@westonruter Thank you for the improvements. They all look good to me.

The only thing that I changed in 26c1206 is the loading spinner. Right now it looks exactly the same as in the React app so that it's hard to notice when they get swapped:

Screen+Recording+2021-06-23+at+05.04.36+PM.mp4

src/Admin/OnboardingWizardSubmenuPage.php

pierlon · 2021-06-23T19:28:56Z

src/LoadingError.php

+
+			#amp-pre-loading-spinner {
+				visibility: visible;
+				animation: amp-wp-hide-element 30s steps(1, end) 0s 1 normal both; /* This could probably reuse amp-wp-show-element if reversed. */


I'm not sure how a slow connection would come into play here, as the page would have already loaded and the necessary JS would have either executed or not by then. A 30s wait time seems to be overkill here.

I was testing with Fast 3G and the JS files were taking over 30 seconds to load. Granted, I was testing with the development versions, so it was 8MB (😱). I'll test again with the production version instead and adjust the timeout down.

With Fast 3G, the settings page takes 10 seconds to load. With Slow 3G it takes 36 seconds to load. This is with the production build, where amp-settings.js is 875KB. So I think 30 seconds is not actually far off.

Ah I see. That makes sense.

What an ingenious solution!

Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>

westonruter · 2021-06-23T19:47:26Z

The only thing that I changed in 26c1206 is the loading spinner. Right now it looks exactly the same as in the React app so that it's hard to notice when they get swapped:

Nice improvement. I do notice the second spinner shifts a tiny bit when it loads, but otherwise it's better as you have it now since the loading.gif doesn't load unnecessarily.

pierlon

This looks ready to ship. Great work @delawski and @westonruter!

pierlon · 2021-06-23T20:43:40Z

src/LoadingError.php

+
+			#amp-pre-loading-spinner {
+				visibility: visible;
+				animation: amp-wp-hide-element 30s steps(1, end) 0s 1 normal both; /* This could probably reuse amp-wp-show-element if reversed. */


What an ingenious solution!

src/LoadingError.php

Co-authored-by: Piotr Delawski <piotr.delawski@gmail.com> Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>

delawski added Bug Something isn't working Enhancement New feature or improvement of an existing one WS:UX Work stream for UX/Front-end labels May 31, 2021

delawski requested review from westonruter, jwold and pierlon May 31, 2021 12:53

pierlon suggested changes Jun 4, 2021

View reviewed changes

delawski requested a review from pierlon June 8, 2021 16:44

delawski force-pushed the add/6230-settings-page-error-boundary branch from 677defb to 983e5e1 Compare June 10, 2021 15:18

pierlon suggested changes Jun 16, 2021

View reviewed changes

pierlon approved these changes Jun 16, 2021

View reviewed changes

Fix OnboardingWizardSubmenuPageTest::test_render

32f57dc

westonruter force-pushed the add/6230-settings-page-error-boundary branch from 6e5602e to 32f57dc Compare June 22, 2021 20:52

westonruter added 4 commits June 22, 2021 14:41

Show pre-loading spinner and show error immediately when JS disabled

0d50dce

Hide validation counts in admin menu when JS is disabled

f226e34

Scope box-sizing style to .error-screen-container

3d636b3

Increase timeout to show error screen for sake of slow connections

b0a8495

westonruter closed this Jun 22, 2021

westonruter reopened this Jun 22, 2021

Use service for dependency injection and add test

7922d33

westonruter requested a review from pierlon June 22, 2021 23:15

westonruter added 2 commits June 22, 2021 16:50

Fix translation function

4aad8b0

Add link to clipboard-button component with the specific commit

8484586

westonruter approved these changes Jun 23, 2021

View reviewed changes

Use the same spinner animation while pre-loading

26c1206

Update unit test

f159152

pierlon suggested changes Jun 23, 2021

View reviewed changes

Fix phpdoc comment

027cbb8

Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>

pierlon approved these changes Jun 23, 2021

View reviewed changes

westonruter reviewed Jun 23, 2021

View reviewed changes

src/LoadingError.php Outdated Show resolved Hide resolved

Remove extraneous margin from spinner

555324b

westonruter merged commit 2a19492 into develop Jun 23, 2021

westonruter deleted the add/6230-settings-page-error-boundary branch June 23, 2021 21:18

westonruter added a commit that referenced this pull request Jun 23, 2021

Add error boundary to the Settings page (#6320)

fb9b094

Co-authored-by: Piotr Delawski <piotr.delawski@gmail.com> Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>

This was referenced Jun 24, 2021

Update amp-toolbox-php to 0.6.0 (almost) and update error page to support fatal errors (in PHP 7+) #6421

Merged

Adds streamlined support request flow #6147

Merged

	if ( error?.filename?.match( /amp-onboarding-wizard(\.min)?\.js/ ) ) {
	if ( /amp-onboarding-wizard(\.min)?\.js/.test( error?.filename ) ) {

	const errorHandler = ( error ) => {
	const errorHandler = ( event ) => {

	render( <ErrorScreen error={ error } />, root );
	render( <ErrorScreen error={ error.error } />, root );

Add error boundary to the Settings page #6320

Add error boundary to the Settings page #6320

Conversation

delawski commented May 31, 2021

Summary

Checklist

delawski commented May 31, 2021

github-actions bot commented May 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierlon commented Jun 4, 2021

delawski commented Jun 8, 2021

westonruter commented Jun 10, 2021

westonruter commented Jun 10, 2021

pierlon commented Jun 10, 2021 • edited Loading

westonruter commented Jun 10, 2021

delawski commented Jun 11, 2021

delawski commented Jun 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierlon Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

pierlon left a comment

Choose a reason for hiding this comment

pierlon commented Jun 16, 2021

pierlon commented Jun 16, 2021

delawski commented Jun 17, 2021

westonruter commented Jun 17, 2021

pierlon commented Jun 17, 2021

westonruter commented Jun 17, 2021

pierlon commented Jun 17, 2021

westonruter commented Jun 17, 2021

pierlon commented Jun 17, 2021

westonruter commented Jun 22, 2021

westonruter commented Jun 22, 2021

Successful Load

Failure due to JS disabled

Failure with caught error

Failure with uncaught error

westonruter left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 23, 2021 • edited Loading

Codecov Report

delawski commented Jun 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Jun 23, 2021

pierlon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 31, 2021 •

edited

Loading

pierlon commented Jun 10, 2021 •

edited

Loading

pierlon Jun 16, 2021 •

edited

Loading

codecov bot commented Jun 23, 2021 •

edited

Loading