Repository navigation
wp-build dashboards: throttle i18n catalog downloads and load widget catalogs on demand - #50904
Conversation
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Code Coverage SummaryCoverage changed in 1 file.
1 file is newly checked for coverage.
Full summary · PHP report · JS report If appropriate, add one of these labels to override the failing coverage check:
Covered by non-unit tests
|
chihsuan
left a comment
There was a problem hiding this comment.
Thanks for the fix! @dhasilva I tested this locally and the dashboard renders fine. Confirmed catalog requests dropped from 100 to 57, peak concurrency never exceeded 6, and the queue always drained.
There's still some delay before the grid fills in, and a batch of [jetpack-i18n] "still pending" warnings, but both look expected rather than regressions to me.
I think the residual cost really wants an upstream fix rather than more work in this PR. And useWidgetTypes looks worth improving too. cc @retrofox
So nothing blocking except one small thing I'd like before merge: the missing .catch() on the metadata gate, whose failure mode is a dashboard that never finishes loading. The rest are docs and wording. Thanks!
| if ( ! cancelled ) { | ||
| setReadyRecords( records ); | ||
| } | ||
| } ); |
There was a problem hiding this comment.
Could we add a .catch() here to fall back to English if this ever rejects?
There was a problem hiding this comment.
Added in b3ab026 — .catch( () => undefined ) before the .then(), so the gate opens either way.
For the record it should never fire: every rejection path inside preloadWidgetModuleCatalogs is already sealed (the manifest promise and downloadOnce both carry their own .catch, and each load is bounded). But the failure mode if that ever changes is a grid stuck on placeholders with nothing in the console, so the guard is worth the line.
Also covered it with a test that rejects the catalog load and asserts the records still reach useWidgetTypes.
Generated by Claude.
There was a problem hiding this comment.
Small one: docblock still says widget catalogs are "downloaded in the background and never awaited"
There was a problem hiding this comment.
Fixed in b3ab026. It now reads:
Widget catalogs (
<build>/widgets/**) are not requested here at all; they load throughloadBundleI18nCatalog()when a widget's module is imported.
Generated by Claude.
| import { useWidgetTypes, type WidgetModuleRecord } from '@wordpress/widget-primitives'; | ||
| import { type WidgetModuleRecord } from '@wordpress/widget-primitives'; | ||
| import { useDashboardGridSettings } from '../dashboard/hooks/use-dashboard-grid-settings'; | ||
| import { resolveWidgetModuleWithI18n, useWidgetTypesWithI18n } from '../widget-module-i18n'; |
There was a problem hiding this comment.
nit: could we move these imports under Internal dependencies?
There was a problem hiding this comment.
Moved in b3ab026. This was a regression from this PR — the new import landed above the Internal dependencies header and dragged useDashboardGridSettings up with it, orphaning the comment that explains why video-detail reuses the dashboard's hook. Both are back under the header, with the comment sitting directly above the import it describes.
Generated by Claude.
There was a problem hiding this comment.
Boot half looks right, widget bundles genuinely aren't requested now, not just un-awaited.
Small stuff first, mostly one-liners:
preloadWidgetModuleCatalogs( records ).then( ... )has no.catchand no bound, so only success opens the gate. One rejected catalog strands the grid on placeholders for the page.path.includes( '/widgets/' )should bepath.startsWith( 'build/widgets/' ).build/routes/widgets/foo.jsgets skipped at boot and the anchored on-demand regex can't recover it.throttled()could usePromise.resolve().then( task ).finally( () => releaseSlot( state ) )and lose both release branches.loadI18nCatalogs()'s JSDoc still promises background widget downloads.- A repeat
loadI18nCatalogs()publishes its manifest promise before resolving, so a failed refetch swaps a good manifest for an empty set and later widgets skip their catalogs quietly. - The gate has no real coverage. I set
gatedRecords = recordsto open it unconditionally and all 9 tests still passed. UpstreamuseWidgetTypesstarts resolving on its own, so that assertion holds either way. Mock it and assertnull, then the exact array.
Worth flagging: CATALOG_TIMEOUT_MS = 5000 and the Promise.race predate this PR, and the ticket measured 48 seconds with them active. So the gain appears to be the concurrency cap, not first paint.
On the local numbers above, 100 to 57 is the part worth pulling on. The ticket's split was 84 widget bundles and 16 route/module/script bundles, so a load with widget catalogs genuinely deferred should land near 16. 57 means roughly 41 came back anyway, because preloadWidgetModuleCatalogs gates the whole registry rather than the widgets actually on screen. That's very likely the same delay reported before the grid fills, and it's new: before this PR useWidgetTypes got its records straight away. Agreed the clean fix is upstream in useWidgetTypes so it isn't blocking here, but it's worth a follow-up issue rather than losing it.
Two things worth checking while the setup is still around:
- Were the "still pending" warnings from the boot path or the per-bundle one? Per-bundle means those widgets rendered untranslated, which is a bit more than log noise.
- Was the local site the MAMP and Jurassic Tube one from the ticket? On a fast origin the burst drains either way, so "queue always drained" wouldn't tell us much about the case in the report.
Follow-up to the review on #50904. - Open the widget-registry gate even if the metadata preload rejects. Every failure path inside it already falls back to English, so this should not fire, but if one ever escapes the grid would sit on placeholders for the rest of the page. - Match deferred widget bundles with an anchored `<build>/widgets/` test instead of a bare `/widgets/` substring. A route or script named `widgets` was skipped at boot and could not be recovered on demand, since the widget resolvers build `build/widgets/<name>/<file>.js` paths of their own. - Fetch the i18n manifest at most once per domain. A repeat boot whose manifest request failed replaced a good bundle set with an empty one, after which every widget skipped its catalog silently. - Free the concurrency slot from a single `finally` in `throttled()`. - Cover the gate for real: the previous assertion read upstream `useWidgetTypes`'s own initial resolving state, so it held whether or not the gate existed. Mock the hook and assert the records it is handed. - Correct the stale `loadI18nCatalogs()` docblock and restore the `useDashboardGridSettings` import to the internal-dependencies group, alongside the comment that explains it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@LiamSarsfield — all six addressed in b3ab026.
Stale JSDoc. Fixed. Manifest clobber. The fetch is now deduped per domain: Gate coverage. You were right, and the reason is worse than it looks: upstream Rewrote it to assert the contract directly — what argument the gate hands On the residual 57 requests and the Generated by Claude. |
…catalogs on demand Boot now downloads only route/module/script catalogs through a 6-slot concurrency queue; widget bundles (two per widget) are no longer requested at boot. Premium Analytics loads each widget bundle's catalog as part of importing that bundle: render modules via WidgetDashboard's resolveWidgetModule, metadata modules by gating useWidgetTypes records on their catalogs, since widget bundles evaluate __() at module scope. Fixes the 100-request unthrottled burst that stalled non-English dashboard loads on slow origins (WOOA7S-1822). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The upstream resolver contract returns Promise<WidgetModule>, not Promise<unknown>; derive the module type from ResolveWidgetModule since widget-primitives does not export it directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Follow-up to the review on #50904. - Open the widget-registry gate even if the metadata preload rejects. Every failure path inside it already falls back to English, so this should not fire, but if one ever escapes the grid would sit on placeholders for the rest of the page. - Match deferred widget bundles with an anchored `<build>/widgets/` test instead of a bare `/widgets/` substring. A route or script named `widgets` was skipped at boot and could not be recovered on demand, since the widget resolvers build `build/widgets/<name>/<file>.js` paths of their own. - Fetch the i18n manifest at most once per domain. A repeat boot whose manifest request failed replaced a good bundle set with an empty one, after which every widget skipped its catalog silently. - Free the concurrency slot from a single `finally` in `throttled()`. - Cover the gate for real: the previous assertion read upstream `useWidgetTypes`'s own initial resolving state, so it held whether or not the gate existed. Mock the hook and assert the records it is handed. - Correct the stale `loadI18nCatalogs()` docblock and restore the `useDashboardGridSettings` import to the internal-dependencies group, alongside the comment that explains it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
b3ab026 to
133daad
Compare
LiamSarsfield
left a comment
There was a problem hiding this comment.
Thanks, b3ab026 covers everything I raised. Two small things left and then I'm happy.
resolveWidgetModuleWithI18n didn't get the .catch() that useWidgetTypesWithI18n did.
The comment above it says it never rejects, but if loadBundleI18nCatalog ever does, the .then() never runs and the widget doesn't import at all, which is worse than rendering it in English.
sharedState() also adopts whatever is already on window.__jetpackWpBuildI18nCatalogs without checking the shape. Two separately-inlined copies from different builds on the same page is enough to break that assumption, and then state.manifests.get() throws, loadI18nCatalogs() rejects, and runInitModules has no catch around it. That's a blank dashboard rather than an untranslated one. A shape check, or a try/catch around the body, would keep the "always falls back to English" promise true. slots for the rest of the page. downloadI18n has no AbortSignal so it isn't really fixable from here, but it'd be worth folding into the same follow-up as the registry gate.
`resolveWidgetModuleWithI18n` chained the widget's `import()` off `loadBundleI18nCatalog()` without a `.catch()`, so a rejection there left the module unimported rather than untranslated. Guard it the same way the metadata gate is guarded. `sharedState()` adopted whatever sat on `window.__jetpackWpBuildI18nCatalogs` without checking its shape. That name is one global shared by every copy of the module on the page, including copies from other builds; a foreign shape threw out of `manifests.get()`, and the init modules that call `loadI18nCatalogs()` are awaited by boot with no catch — a blank dashboard instead of an English one. Validate the parked value and fall back to state private to this copy when it isn't recognized, rather than clobbering state another copy is still using. Both exported helpers now also catch around their own body, so the "always resolves, worst case English" contract holds structurally instead of depending on every internal path staying sealed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@LiamSarsfield — both fixed in 3c4e906.
It now validates the parked value and, when it isn't recognized, falls back to state private to this copy rather than replacing what's there. Replacing looked simpler but is the wrong move: the copy that parked it is still reading the same global on every call, so the two would overwrite each other's state indefinitely and neither would end up with a usable dedupe map. Unshared state costs a few repeat downloads and splits the concurrency budget — both cheaper than the alternatives, and it warns once so the situation is visible. I also took the second half of your suggestion rather than choosing between them: both exported helpers now catch around their own body. The "always resolves, worst case English" contract was true but argued — it held only as long as every internal path stayed individually sealed, which is precisely why this round found two more. It's structural now. On coverage, given last round: both new tests fail without their fix ( Follow-up: JETPACK-2095, covering both open items — the registry gate (100 → 57 measured, ~16 if only on-screen widgets loaded, so ~41 still arriving through the metadata gate, plus the grid delay that didn't exist before this PR) and the slot leak. One pushback on the slot leak, for the record: I don't think it's unfixable from here. Generated by Claude. |
Fixes # N/A (reported in WOOA7S-1822, follow-up to #50762 — see this comment)
Proposed changes
On any non-English locale,
loadI18nCatalogs()issued one translation-catalog request per bundle in the build's i18n manifest — 100 requests for the current Premium Analytics build — all in one unthrottled loop at boot. On HTTP/2 origins nothing caps that burst, and a slow origin serves nothing else until it drains (measured at ~48s to first render in WOOA7S-1822). Widget catalogs (84 of the 100) were "non-blocking" only in the sense of not being awaited; they were still all requested for widgets that may never render.wp-build-polyfills:loadI18nCatalogs()now downloads only route/module/script catalogs at boot, through a 6-slot concurrency queue, so the burst can never starve the origin regardless of protocol. Widget bundles are no longer requested at boot. A newloadBundleI18nCatalog( domain, bundlePath, timeoutMs? )export downloads a single bundle's catalog on demand — it checks manifest membership (bundles without gettext calls are skipped without a request), dedupes repeat requests, and shares the queue. Shared state lives onwindow, since each wp-build bundle inlines its own copy of the helper.premium-analytics: each widget bundle's catalog now loads as part of importing that bundle. Render modules go through a customresolveWidgetModuleonWidgetDashboard; metadata (widget.js) records are gated in auseWidgetTypeswrapper until their catalogs are installed. Both are awaited (bounded) before the import because widget bundles evaluate__()at module scope — a catalog that arrives after evaluation leaves those strings untranslated for the session.The other five packages sharing
loadI18nCatalogs(videopress, forms, activity-log, newsletter, publicize, backup) have no widget bundles; they pick up the boot throttle with no behavior change.Measured (local Docker site via a tunnel,
pt_BR, one full dashboard load each)Note: over HTTP/1.1 the browser's 6-connections-per-host cap already serializes the old burst, which hides the pathology locally. The unthrottled burst manifests on HTTP/2 (as in WOOA7S-1822), where the browser multiplexes all 100 requests at once; the new queue bounds concurrency in both cases.
Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
en_USlocale, e.g. Español.languages, and loadwp-admin/admin.php?page=jetpack-premium-analytics-wp-admin.[jetpack-i18n]warnings.en_USno catalog requests should fire at all.JS tests:
jp test js packages/wp-build-polyfillsandjp test js packages/premium-analytics.