Add site editor initial redirect error handling #38655
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
talldan
added
[Type] Bug
talldan
commented
Feb 9, 2022
| @@ -48,7 +55,7 @@ async function getHomepageParams( siteUrl ) { | |||
| .then( ( { data } ) => data ); | |||
There was a problem hiding this comment.
I can probably add some better error handling to these lines to improve the user's error message.
There was a problem hiding this comment.
apiFetch will throw if there's a 4xx or 5xx error, right? Would be good to access any error message that comes back from that, in case it's useful for debugging.
There was a problem hiding this comment.
The window.fetch won't throw an error on 4xx, and we can't use apiFetch since it adds a REST API prefix.
There was a problem hiding this comment.
I've added some handling for status codes, and rest error messages.
|
Size Change: +363 B (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
I think this option is fine 👍 Divergences between the branches will confuse us in the medium term. |
Mamaduka
reviewed
Feb 10, 2022
| @@ -48,7 +55,7 @@ async function getHomepageParams( siteUrl ) { | |||
| .then( ( { data } ) => data ); | |||
|
|
|||
| if ( ! template?.id ) { | |||
| return; | |||
| throw new Error( '`getHomepageParams`: unable to find home template.' ); | |||
There was a problem hiding this comment.
suggestion: Maybe we should make this message more user-friendly and display it instead of a generic error message?
The editor is unable to find a block template for the home page.
What do you think?
There was a problem hiding this comment.
So I've kept these error messages the same so that we can locate them in the code when the user uses 'Copy Error', but the visible message is now what you suggested.
|
I think this is a good start, but a few issues remain.
What if we use a different route for similar errors and then display the error component inside gutenberg/packages/edit-site/src/components/app/index.js Lines 27 to 31 in 0b35edf |
Is the goal to render the navigation sidebar? If so, this unfortunately won't do it. We'd need to render an interface skeleton layout with the NavigationSidebar slot for that. I also feel like this will be making it a little complicated. The more we do after an error is encountered, the more other errors may be triggered. I think it's best to short circuit as quickly as possible in this kind of situation.
This is a good point. There would probably be the same issue with the normal Error Boundary. Seems like a side-effect of being fullscreen. I can add a link button to the error panel.
Again, we could add links for this, but we're probably getting to the point where this needs some design input. |
|
Good point regarding NavigationSidebar. Let's keep things simple for now.
We could probably replace the reboot button; not sure how helpful it will be in this case.
We don't have to introduce this right away. Just wishful thinking :) |
That's true, the user can always click the dashboard link and then the editor link again to reboot. I'll do that 👍
It's a good point, other templates probably work ok. Maybe it'd be best to try to find a fallback template before bailing. |
|
Thanks again for working on this, @talldan. I think this is something we could ship with the minor release.
We could iterate on better fallbacks and error design for 6.0. P.S. I think we need a better template resolution for WP. I like what was suggested here - #31652 (comment). |
Mamaduka
approved these changes
Feb 10, 2022
8 tasks
kevin940726
approved these changes
Feb 10, 2022
talldan
added
the
Backport to WP Minor Release
7 tasks
noisysocks
reviewed
Feb 15, 2022
|
|
||
| if ( error ) { | ||
| actions.push( | ||
| <CopyButton key="copy-error" text={ error.stack }> |
There was a problem hiding this comment.
Maybe we need to add a fallback here. When I add an empty front-page.php to Twenty Twenty-two and open the Site Editor I see an error ("The editor is unable to find a block template for the homepage.") but pressing Copy Error does nothing.
In React DevTools I can see that there is no text.
There was a problem hiding this comment.
So somewhere in the code isn't passing a proper error object? I can do some debugging, but if you have any clues let me know.
There was a problem hiding this comment.
No clues, sorry. It was pretty easy to reproduce, though. Add an empty front-page.php file to the top level of twentytwentytwo.
Mamaduka
removed
the
Backport to WP Minor Release
Mamaduka
pushed a commit
that referenced
this pull request
Feb 16, 2022
* Try adding error handling to `redirectToHomepage` * Return early to avoid unmounting the error warning on the next line * Handle http status errors * Handle REST errors * Add dashboard link and better labelling * Remove reboot button
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Partly addresses #37236.
Adds some error handling to the site editors loading process. If an error happens when trying to fetch the home template, the editor will now show the usual error boundary warning
The caveat here is that the code in
trunkis different to that inwp/5.9. The loading process was refactored in #37248.If this improvement needs to land in 5.9.x the options are:
a) backport both #37248 and this
b) develop a separate fix for the code on
wp/5.9.Testing Instructions
This is tricky to test. As suggested by @Mamaduka I modified the API response - #37236 (comment).
But I had to use a development environment for both wordpress-develop and gutenberg and comment out the corresponding line in wordpress-develop.
Expected - an error should be shown
Screenshots
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).