Edit Site: Add a loading timeout #51049
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
tyxla
added
[Type] Enhancement
tyxla
requested review from
fullofcaffeine,
youknowriad,
mtias,
mirka,
jsnajdr,
jameskoster,
SaxonF,
ciampo,
jasmussen and
flootr
|
Size Change: +28 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
|
I haven't had a chance to test the PR, but speaking to the concept, I appreciate it. If I understand it correctly, in most cases you will have a good experience opening the site editor, seeing the spinner, then seeing the fully formed editor. And then in case of timeouts, it still gives access. Are you able to provide a quick GIF or video? Otherwise, generally thumbs up on the concept. |
Glad to hear, thanks for confirming!
Exactly 👍 And FWIW, in the general case, nothing should fail and the editor should load faster than 30 seconds.
Sure thing, I've updated the PR description, let me know what you think. |
|
Sounds good. If anything, I'd reduce that timeout quite a bit. Honestly anything that takes more than 5 or 6 seconds feels unusually long. Which is to say, I am aware that most of the time, things will load faster than that, but for the case shown, 30s feels like an eternity and I'm likely to have moved on. Can we set it to max 10 seconds? Longer term it could be interesting to explore a progressbar. We might even show a text message below the progressbar after, say, 5 seconds have passed, such as "This is taking longer than usual..." |
tyxla
force-pushed
the
add/site-editor-loading-timeout
branch
from
49a71ae to
00e62ba
Compare
Sure thing, I think your rationale makes sense! Updated to 10 seconds in 00e62ba. Noting that we can always tweak it as we go.
I think it could be a nice improvement. I know a progress bar has been discussed before - do we have any pre-existing design mockups for that? |
@jameskoster does, but I don't think it needs to be a 6.3 thing. But it could be interesting to explore fading in that little message ("This is taking an unusual amount of time...") after n seconds. But I think this one just needs a code check. |
My first thoughts about that (in light of changing the timeout time to 10 seconds) are that if we show it after, let's say, 6 seconds, and hide it after 10 seconds, it may be too brief a time to read the message. |
|
Yep, nothing to block this one, something to think about as an option. |
tyxla
force-pushed
the
add/site-editor-loading-timeout
branch
from
00e62ba to
e6cd944
Compare
|
Flaky tests detected in e6cd944. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5210940337
|
sethrubenstein
pushed a commit
to pewresearch/gutenberg
that referenced
this pull request
Jul 13, 2023
* Edit Site: Add a loading timeout * Set maximum loading time to 10s
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.
What?
This PR adds an arbitrary timeout to the site editor loading experience we introduced in #50222, so if a request hangs, it won't block the entire editor frame canvas forever.
Why?
While doing some production testing of the improved site editor loading experience, I noticed that if there's a failure during loading, it will block the editor loading and keep it in an infinite loading state.
The front-end of an app should not be blocked by a server-side error, therefore, after we consider we've timed out, we should re-enable the editor for interaction
How?
We're adding an arbitrary
3010-second timeout, which should be more than enough time to load most of the site editor instances. I'm happy to alter that value if there are better suggestions.cc @fullofcaffeine since we've been discussing this as one of the potential improvements we can make.
One additional improvement I think we can make is to add some error notices that appear when a request has failed. It could help the user understand what is going on. Maybe those notices could offer additional actions, like reloading the page or re-triggering the request.
Testing Instructions
sleep(60)to theget_items()method of one of the endpoints used in your theme - likeWP_REST_Block_Pattern_Categories_Controllerfor example.3010 seconds, the editor will be marked as loaded, even though the network request would still be pending.Testing Instructions for Keyboard
None
Screenshots or screencast
Here's a preview of when I added
sleep(60)to theget_items()method ofWP_REST_Block_Pattern_Categories_Controller:Screen.Recording.2023-05-30.at.14.55.41.mov
You can see that the editor appears after the timeout(
3010 seconds), even though the block patterns are still loading.Without such a long-loading request, the editor will continue loading as it did before.