-
Notifications
You must be signed in to change notification settings - Fork 1.1k
refactor: Fetch and cache GutenbergKit editor settings #24417
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
Conversation
Generated by 🚫 Danger |
7cdf99a
to
9ceb20e
Compare
|
App Name | WordPress | |
Configuration | Release-Alpha | |
Build Number | 27516 | |
Version | PR #24417 | |
Bundle ID | org.wordpress.alpha | |
Commit | 487ede4 | |
Installation URL | 5qc7k318n5vjo |
|
App Name | Jetpack | |
Configuration | Release-Alpha | |
Build Number | 27516 | |
Version | PR #24417 | |
Bundle ID | com.jetpack.alpha | |
Commit | 487ede4 | |
Installation URL | 2klik7q580q4g |
9ceb20e
to
80f864d
Compare
WordPress/Classes/ViewRelated/NewGutenberg/NewGutenbergViewController.swift
Show resolved
Hide resolved
} | ||
|
||
@MainActor | ||
func fetchSettings() async throws -> [String: Any] { |
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.
What happens if we've never fetched the settings and the network connection fails?
In my testing it seems like it'll just show a spinner forever?
If we don't have locally cached settings and the network request fails we should probably:
- Try again some number of times (maybe 2?)
- Show an error message
I could see an argument for showing some kind of editor with no remote settings, but I don't know how feasible that is?
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.
Your experience is surprising. These changes were built to accomplish what you describe. When I block the network request (via Charles Proxy) for the editor settings, the editor continues to load with default editor settings after a moment.
How are you causing the network connection failure in your testing?
The screen recording below showcases both the default settings and site-specific settings.
Screen.Recording.2025-04-16.at.16.44.12.mov
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.
In fact, the situation you describe is a part of the testing instructions in the PR description. 😄 I'm perplexed as to why you may be experiencing a different outcome.
d26a943
to
c8aa2cf
Compare
So it's not forever, but it is almost 5 minutes:
I can consistently repro this by enabling the Network Link Conditioner (on a real device), setting it to "Very Bad Network", then running this PR on that device and following the testing instructions. I suspect we can resolve the issue by putting a timeout on how long we wait for the remote network settings (I'd suggest 5 seconds – IINM, the connection timing out should automatically cause the "whoops, we couldn't load editor settings" dialog. |
This reverts commit e4ec71b6c77e2439e9d98c0273f9bdcff64d098f.
This reverts commit 39ac9caf71089853e5f256697f4882cc55d2bffd.
Remove unnecessary completion handler structure.
Improve editor start performance.
Mitigate stale editor settings.
The cache was discarded each time the editor closed, which defeats the purpose of the cache. We must store it somewhere outside the editor view controller.
Avoid sending both foreground and background requests.
Revalidating the cache on each editor launch will ensure the latest settings are available.
Improve load speed of editor.
This conditional was overlooked when the local experimental feature toggle was augmented with a remote feature flag for roll out control. #24465
Avoid postponing the editor start for longer than three seconds. We instead present the editor without any site-specific settings.
f294279
to
e8a892c
Compare
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.
Having discussed this in Slack, it seems this is as good as it'll get for now. We can control the settings loading prior to the editor being displayed, but unfortunately there's no UI inside the editor to show the other network operations that are happening.
This looks solid, and should work well in the vast majority of cases – if someone's network is this bad they're probably having a hard time using the app anyway.
|
||
Task { @MainActor in | ||
// Start the editor with default settings after 3 seconds | ||
let timeoutTask = Task { |
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.
This is a clever way to address this – I think ideally we'd just use the timeout
property on URLRequest
, but we don't really own WordPressOrgRestApi
. I guess we'll come back to this when it's time to adopt the Rust-layer REST API, and we can switch it over then.
Long term, I believe we can improve the experience with WebView-driven UI so that it display a "skeleton" while additional requests complete. That said, it gets complicated for the remote/site-specific editor. That editor fetches its React library (used for composing UI)—do we await React or build skeleton UI without React? We likely do the latter, and we can also finish implementing cached bundles to further improve the loading experience. |
|
Fetch and cache editor settings before providing them to the editor, as it
allows improving offline support. The changes also communicate the
background activity with an activity indicator.
Ref CMM-200.
Related:
To test:
Important
Ensure the following experimental features are enabled:
1: Initial editor fallbacks to default settings
2: Editor fetches site-specific settings
3: Editor fallback to cached settings
Regression Notes
Regressions in Gutenberg Mobile or Aztec editors.
Manually smoke tested the editors.
Deemed unnecessary for the experimental editor.
PR submission checklist:
RELEASE-NOTES.txt
if necessary.Testing checklist: