feat: Enable GutenbergKit asset caching#22035
Conversation
Using `isWPComAtomic` suffices for the need of identifying sites managed by WPCOM.
The current preloading strategy results in the editor loading twice. The duplicative load results in hidden editor UI for the second load, as the loading sequence is not idempotent currently. We should refactor to ensure the editor UI is only displayed once after the editor fully loads. Currently, The first occurs without blog configuration and always loads the bundled editor. We should ensure preload loads with the proper configuration and the correct editor (bundled/remote).
The editor relies upon a subscription to editor settings within the FluxC store. FluxC can dispatch change events multiple times, leading to unexpected, unnecessary invocations of GutenbergKit's `start` method. This lead to odd outcomes. First, during editor setup, we dispatch an event to request the latest editor settings. FluxC broadcasts two change events: first with the cached settings, and second when the fetched settings resolve. This caused two `start` invocations when opening the editor. Second, the My Site fragment requests the latest settings as a performance optimization. When closing the editor and returning to My Site, the request resulted in a broadcast of updated settings, which attempt to start the editor while it was closing.
dcalhoun
added
[Type] Enhancement
Gutenberg
Generated by 🚫 Danger |
Project dependencies changeslist! Upgraded Dependencies
org.wordpress.gutenbergkit:android:v0.5.0, (changed from v0.4.1)tree +--- project :libs:editor
-| \--- org.wordpress.gutenbergkit:android:v0.4.1
-| +--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.9.0 -> 2.0.21 (*)
-| +--- androidx.core:core-ktx:1.13.1 -> 1.16.0 (*)
-| +--- androidx.appcompat:appcompat:1.7.0 -> 1.7.1 (*)
-| +--- com.google.android.material:material:1.12.0 (*)
-| +--- androidx.webkit:webkit:1.11.0 -> 1.14.0 (*)
-| +--- com.google.code.gson:gson:2.8.9 -> 2.13.1
-| | \--- com.google.errorprone:error_prone_annotations:2.38.0
-| \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.9.0 -> 1.9.10 (*)
+| \--- org.wordpress.gutenbergkit:android:v0.5.0
+| +--- org.jetbrains.kotlin:kotlin-parcelize-runtime:2.0.21 (*)
+| +--- androidx.core:core-ktx:1.13.1 -> 1.16.0 (*)
+| +--- androidx.appcompat:appcompat:1.7.0 -> 1.7.1 (*)
+| +--- com.google.android.material:material:1.12.0 (*)
+| +--- androidx.webkit:webkit:1.11.0 -> 1.14.0 (*)
+| +--- com.google.code.gson:gson:2.8.9 -> 2.13.1
+| | \--- com.google.errorprone:error_prone_annotations:2.38.0
+| +--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.10.2 (*)
+| \--- org.jetbrains.kotlin:kotlin-stdlib:2.0.21 -> 2.1.21 (*)
-\--- org.wordpress.gutenbergkit:android:v0.4.1 (*)
+\--- org.wordpress.gutenbergkit:android:v0.5.0 (*) |
|
| App Name |  WordPress |
|
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr22035-3ef5c35 | |
| Commit | 3ef5c35 | |
| Direct Download | wordpress-prototype-build-pr22035-3ef5c35.apk |
|
| App Name |  Jetpack |
|
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr22035-3ef5c35 | |
| Commit | 3ef5c35 | |
| Direct Download | jetpack-prototype-build-pr22035-3ef5c35.apk |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## trunk #22035 +/- ##
==========================================
+ Coverage 39.02% 39.51% +0.48%
==========================================
Files 2153 1551 -602
Lines 101494 72498 -28996
Branches 15585 11838 -3747
==========================================
- Hits 39613 28650 -10963
+ Misses 58384 41372 -17012
+ Partials 3497 2476 -1021 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if (isGutenbergKitEditor) { | ||
| GutenbergWebViewPool.getPreloadedWebView(this) | ||
| } |
There was a problem hiding this comment.
This performance optimization leads to duplicate "editor mounted" events, which results in invisible editor UI. We should reinstate this after refactoring the WebView initialization to avoid this issue.
Additional details capture in f013498.
| onXpostsSettingsCapability(isXpostsCapable) | ||
| } | ||
|
|
||
| val isWpCom = site.isWPCom || siteModel.isPrivateWPComAtomic || siteModel.isWPComAtomic |
There was a problem hiding this comment.
isPrivateWPComAtomic is duplicative of isWPComAtomic in this context of identifying non-self-hosted sites.
| // TODO: Use the application password for self-hosted sites | ||
| val authHeader = if (isWpCom) "Bearer ${accountStore.accessToken}" else "Basic " |
There was a problem hiding this comment.
To my knowledge, we do not have a method for accessing the application password at this time.
|
|
||
| public void startWithEditorSettings(@NonNull String editorSettings) { | ||
| if (mGutenbergView == null) { | ||
| if (mGutenbergView == null || mEditorStarted) { |
There was a problem hiding this comment.
Due to the multiple FluxC subscription updates, the existing logic attempts to start the editor twice on open and once on close. This new state guards against unnecessary editor starts.
Additional details in dd36e2c.
There was a problem hiding this comment.
This change has caused the regression mentioned here.
oguzkocer
left a comment
oguzkocer
left a comment
There was a problem hiding this comment.
Approving pending testing.
There are bits that I don't fully understand due to my lack of domain knowledge and bits that I find confusing as outlined as a line comment. However, this shouldn't be a blocker as I asked @dcalhoun to handle this part of the work specifically because I lacked this knowledge, so I think testing it is more important than having a line-by-line understanding at the moment. Having said that, I am slowly acquiring the domain knowledge and hope to contribute more in the reviews in the future.
| val siteApiRoot = if (isWpCom) "https://public-api.wordpress.com/" | ||
| else siteModel.wpApiRestUrl ?: "$siteURL/wp-json/" |
There was a problem hiding this comment.
As a future improvement, I'd love for us to be a bit more clear on when the editor is fully supported and we know the logic is correct and when it's not or we are making a guess.
- Moving
siteApiRooturl calculation to a central place with some unit tests would be great - Having a
isGutenbergKitsupported function in a central place would also help, where the function would take a configuration type (with api root, authentication header etc) and decides whether it can move forward or not. We can add comments about each scenario. - We can add a way to convert a
SiteModelinto the configuration type mentioned above. - The configuration type then can be passed to relevant places, rather than doing to calculation here or in other fragments
My main feedback - and it's not about this PR - is that I am having a hard time following the current code, mostly because it's only partially implemented. If it was fully implemented and I just needed to process it, than it'd be easy, but that's impossible right now without at least comments for each situation.
There was a problem hiding this comment.
I agree. Thank you for the note.
Centralizing and documenting common calculations would definitely help communicate intent. A utility for converting SiteModel to editor configuration would also be helpful, and is necessary for implementing the warmup functionality without further duplication.
|
|
@dcalhoun Tested as instructed and it worked great. Caching does seem to make a huge difference as the difference is very obvious when I switch sites and the assets have to be downloaded again. I think we can merge it 👍 |
Trailing slashes are now expected to be providing in the configuration. This expectation was put in place to simplify configuration logic here and in host apps. This change prevents double trailing slashes, which began occurring in WP-Android in the following. This resulted in failed API requests, like image uploads. wordpress-mobile/WordPress-Android#22035
…#158) Trailing slashes are now expected to be providing in the configuration. This expectation was put in place to simplify configuration logic here and in host apps. This change prevents double trailing slashes, which began occurring in WP-Android in the following. This resulted in failed API requests, like image uploads. wordpress-mobile/WordPress-Android#22035
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
|
Sentry's suspected issue report seems unlikely. The referenced |
|
@dcalhoun I looked into the reported issue yesterday and thought the same thing, but forgot to record it here. So, thanks for doing that! 🙇♂️ |
4 participants
Description
Integrate asset caching from wordpress-mobile/GutenbergKit#153.
Close CMM-226. Fix CMM-512.
Testing instructions
1: Smoke test WPCOM Simple site bundled editor
Experimental block editorexperimental featuregutenberg_kit_pluginsdebug flag2: Smoke test WPCOM Simple site remote editor
Experimental block editorexperimental featuregutenberg_kit_pluginsdebug flag3: Smoke test WPCOM Atomic site bundled editor
Experimental block editorexperimental featuregutenberg_kit_pluginsdebug flag