Convert GutenbergKitSettingsBuilder to injectable class#22836
Conversation
Generated by 🚫 Danger |
|
|
|
|
…undles Move locale resolution into the library so Android consumers stop having to mirror the `SUPPORTED_LOCALES` table to avoid silently falling back to English. WP-Android historically passes `Locale.getLanguage()` (ISO 639-1 only), which dropped Brazilian users to the language-only `pt` bundle and Chinese users to English regardless of region — see wordpress-mobile/WordPress-Android#22836. The Vite build now emits `dist/supported-locales.json` from the `src/translations/` directory, and `LocaleResolver.fromAssets(context)` reads that manifest from the library's `assets/` to apply the chain full-tag → language-only → `en`. `EditorConfiguration.Builder.setLocale(context: Context, locale: Locale)` is the new type-safe entry point; `setLocale(String?)` stays as the existing power-user / test escape hatch and the wire format is unchanged. A Context is required because the manifest lives in the library's assets — consumers calling this from an editor activity already have one in hand. The shared JS pieces (manifest emission, JS-side resolver) also land in the iOS PR; merging the second of the two will be a no-op for those files. Refs #490
…undles Move locale resolution into the library so Android consumers stop having to mirror the `SUPPORTED_LOCALES` table to avoid silently falling back to English. WP-Android historically passes `Locale.getLanguage()` (ISO 639-1 only), which dropped Brazilian users to the language-only `pt` bundle and Chinese users to English regardless of region — see wordpress-mobile/WordPress-Android#22836. The Vite build now emits `dist/supported-locales.json` from the `src/translations/` directory, and `LocaleResolver.fromAssets(context)` reads that manifest from the library's `assets/` to apply the chain full-tag → language-only → `en`. `EditorConfiguration.Builder.setLocale(context: Context, locale: Locale)` is the new type-safe entry point; `setLocale(String?)` stays as the existing power-user / test escape hatch and the wire format is unchanged. A Context is required because the manifest lives in the library's assets — consumers calling this from an editor activity already have one in hand. The shared JS pieces (manifest emission, JS-side resolver) also land in the iOS PR; merging the second of the two will be a no-op for those files. Refs #490
34887ae to
d5911f3
Compare
|
@jkmassel This has some CI failures and conflicts that need to be addressed. I had Claude create a PDF review which also uncovered some issues. |
b984d31 to
3538ea9
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #22836 +/- ##
==========================================
+ Coverage 37.24% 37.27% +0.02%
==========================================
Files 2319 2318 -1
Lines 124604 124520 -84
Branches 16927 16914 -13
==========================================
- Hits 46413 46411 -2
+ Misses 74441 74355 -86
- Partials 3750 3754 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
d5911f3 to
15e2fdb
Compare
6687be2 to
0e21611
Compare
Promotes `GutenbergKitSettingsBuilder` from a Kotlin `object` with `buildSettings` returning a `Map<String, Any>` to a `@Singleton` class that returns `EditorConfiguration` directly via `buildPostConfiguration(site, post, accessToken)`. The injectable form is needed so the builder can constructor-inject `EditorCapabilityResolver` and consult per-site theme-styles and third-party-blocks state itself, instead of relying on each call site to assemble a `FeatureConfig`. `EditorConfigurationBuilder` (the intermediate Map → `EditorConfiguration` adapter) is now redundant and removed. `GutenbergKitActivity` adopts the injected builder via a small `buildEditorConfiguration` helper that overlays the per-call locale, cookies, and account fields the builder cannot know about, and drops its now-unused `GutenbergKitPluginsFeature` injection. Settings-builder tests are rewritten against the new ctor and API; capability-gating coverage already lives in `EditorCapabilityResolverTest`.
Three independent fixes surfaced while validating the converted builder; each is small but distinct. ## Handle schemeless URLs in `extractHost` `URI(url).host` returns null for hosts without a scheme (e.g. `example.wordpress.com`), but the OLD `UrlUtils.removeScheme` path that this PR replaced was lenient. `SiteModel.url` can be schemeless (FluxC's `SiteStoreUnitTest.java:472,494` writes `shieldeyesfromlight.wordpress.com` directly), which regressed `siteApiNamespace` (missing the `sites/$host/` alias) and `cachedAssetHosts` (missing the site host). Normalize by prepending `https://` when no scheme is present, then let `URI` parse. Pure JDK, no Android dependency. Added regression tests covering schemeless hosts, ports, userinfo, and integration through `buildSiteApiNamespace` and `buildCachedHosts` (the latter uses the exact value `shieldeyesfromlight.wordpress.com` to lock in the regression). ## Require per-call values explicitly in `buildPostConfiguration` The builder previously hardcoded `locale="en"`, `cookies=emptyMap()`, and `enableNetworkLogging=false`, relying on `GutenbergKitActivity.buildEditorConfiguration` to overlay them via `toBuilder()`. The stacked preloader (#22579) calls `buildPostConfiguration` directly with no overlay, baking the placeholder values into the preloaded `EditorDependencies`. Promoted `locale`, `cookies`, and `isNetworkLoggingEnabled` to required non-default parameters. The activity now passes them at the call site; the `.toBuilder()` overlay is gone. The preloader will fail to compile on rebase until it supplies the values — exactly the failure mode we want. ## Gate `editorAssetsEndpoint` on capability and correct the route check Two related bugs: - `EditorSettingsRepository.fetchRouteSupport` was populating `SiteSupportsEditorAssets` from `/wp-block-editor/v1/assets` — a copy-paste from the line above. The URL we actually hit is `wpcom/v2/editor-assets`, which a vanilla self-hosted WP 6.7+ install doesn't expose. So `getSupportsEditorAssetsForSite` could be true on sites where the URL 404s. - This PR set `editorAssetsEndpoint` unconditionally; the safety net was GutenbergKit's `plugins == false` short-circuit, which fails to protect when `plugins == true` on a site that lacks `wpcom/v2/editor-assets`. Fixed both: the route check now queries `/wpcom/v2, editor-assets`, and the URL is only set when `resolveThirdPartyBlocks(site).isAvailable`. Both `plugins` and `editorAssetsEndpoint` flow from the same resolver call and cannot drift. (Considered using `resolver.resolve(...)` to canonicalise the URL, but the concrete uniffi resolver classes load JNA on instantiation and the unit-test classpath doesn't include the native lib. The string-concat output is byte-identical to the resolver output for both WP.com and self-hosted shapes, so the resolver was purely cosmetic here.)
bf8eba5 to
e7e5d52
Compare
e7e5d52 to
d851897
Compare
|
@nbradbury, this is rebased now that #22812 landed. It's ready for review now! |


Summary
GutenbergKitSettingsBuilderfrom a KotlinobjectreturningMap<String, Any>to a@Singletonclass returningEditorConfigurationdirectly viabuildPostConfiguration(site, post, accessToken).EditorCapabilityResolverand consult theme-styles / third-party-blocks state itself, instead of every call site assembling aFeatureConfig.EditorConfigurationBuilder(the intermediate Map →EditorConfigurationadapter) is now redundant and removed.GutenbergKitActivityadopts the injected builder via a smallbuildEditorConfigurationhelper that overlays the per-call locale, cookies, and network-logging pref the builder cannot know about, and drops its now-unusedGutenbergKitPluginsFeatureinjection.Capability-gating coverage already lives in
EditorCapabilityResolverTest(#22812); settings-builder tests are rewritten to verify the builder threads its inputs and the resolver's result through to theEditorConfiguration.Extracted from #22579 so the preloading PR can focus on the preloader itself.
Behavior changes
Not a pure refactor — two intentional changes worth flagging:
GutenbergKitPluginsFeaturewas on. The new resolver-driven path also requires the per-siteuseThirdPartyBlockstoggle to be set. Roll-out continues to be controlled by the existingGutenbergKitPluginsFeatureremote flag, so users only see the toggle once the flag reaches them.editorAssetsEndpointis now always populated. Previously it was unset for self-hosted sites (empty namespace). The endpoint is now set unconditionally — butGutenbergKit'sEditorAssetsLibrary.fetchManifestshort-circuits whenplugins == false, so the URL is never dereferenced on sites that don't support it. Filed GutenbergKit#494 to tighten that API shape upstream.Test plan
./gradlew :WordPress:testJetpackDebugUnitTest --tests "org.wordpress.android.ui.posts.GutenbergKitSettingsBuilderTest"passeswpcom/v2/editor-assetsrequest fires (regardless of toggle state, since the endpoint short-circuits)Related