Repository navigation
Premium Analytics: load widget translation catalogs only for the widgets on screen, and free stalled download slots - #50950
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
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! |
Code Coverage SummaryCoverage changed in 3 files.
|
|
A download that never settles held its slot forever, so six stalled downloads deadlocked the shared queue for the rest of the page. Arm a watchdog when a task takes a slot and return the slot when it expires, letting the orphaned request finish in the background.
useWidgetTypes imports every record's metadata module eagerly, so gating on the full record set put one catalog request per registered widget on the boot critical path. Resolve only the widgets the active layout renders, widening to the full registry when edit mode opens. visibleNames is tri-state because "the layout is empty" and "the layout has not loaded yet" must not be treated alike: hooks cannot be skipped, so a caller rendering a spinner until its layout arrives still calls this hook meanwhile, and treating that as an empty layout preloads the whole registry before the layout can ever narrow it.
The test credited with this coverage settled every download normally, so `clearTimeout` — mocked alongside `setTimeout` by node:test — cancelled each watchdog and none ever fired. Deleting the `released` guard left the whole suite green. Cover the sequence the guard exists for instead: a stalled download whose watchdog frees its slot, which then settles and runs `finally` against a slot that is already back in the pool. Without the guard `active` reaches -5 and the next burst runs over the concurrency cap. Rename the settle-normally test for what it does check — that a completed download's watchdog was cancelled rather than merely silent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…osed Scoping closes the gate during render — a render-phase `setNeededNames` widens the scope, and the new `scopedRecords` array lands the same pass — but `useWidgetTypes` only raises `isResolvingWidgetTypes` from an effect, one commit later. A section switch therefore commits one frame holding the previous section's types and claiming to be resolved, and `widget-chrome` consults that flag only for widgets whose type it cannot find: every widget the incoming section adds renders as "Missing widget" for that frame. Derive the flag from the gate instead — records being withheld is itself proof that resolution has not finished. The suite could not see this: its `useWidgetTypes` stand-in reported resolving synchronously during render, which is exactly the timing that hides the bug. Add a second stand-in timed the way upstream times it, and assert on every committed frame rather than the settled result. Reported by @CGastrell in review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… is idle
`WidgetPicker` hardcodes `isLoading={ false }` and takes `totalItems` from the
types the dashboard hands it, so it has no way to say "still loading" — it
just renders a short gallery with a matching count. Widening the registry only
when edit mode opens therefore left a window, between Customize and the last
metadata catalog landing, where the gallery was silently incomplete. Boot
scoping did not remove those downloads so much as move them behind a click.
Move them behind first render instead: once the widgets on screen have
resolved, download the rest of the registry's catalogs on the next idle
callback. `includeAll` then adopts the promises already in the loader's
download map — it keys by bundle path — so the widening resolves in a
microtask and the picker never mounts against a partial registry.
The warm-up waits for the visible widgets deliberately. Scheduling it earlier
would queue the whole registry into the same six-slot download queue they are
draining, which is the inversion the scoping exists to prevent.
Reported by @CGastrell in review.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
c69f193 to
d7bdf59
Compare
|
All three reproduce. Fixed in d5f3ab0, 0150758 and 44e7997 (force-pushed — the branch had been rebased). 1. Section switch commits a "Missing widget" frame. Confirmed, and your frame sequence is exact. The gate closes during render (render-phase Worth calling out why the suite was blind to it: the 2. Widget picker gallery transiently incomplete. Confirmed — That does mean this PR defers those requests rather than eliminating them — after the page settles the download count is the same as #50904's, just after the grid is on screen instead of before it. I've rewritten the PR description and the changelog entry to say that rather than "only for the widgets on screen", and reworked the testing instructions so nothing depends on click speed. The warm-up deliberately waits for the visible widgets first; scheduling it earlier would queue the whole registry into the same six-slot queue they're draining. The real fix is still lazy per-widget metadata upstream. Filing the 3. Double-release guard untested. Confirmed — deleting There's now a test for the sequence the guard exists for: a stalled download whose watchdog frees its slot, which then settles and runs Generated by Claude. |
|
Tested on a fresh Jurassic Ninja site running this branch ( The PR's numbers reproduce. Boot: 39 total / 11 metadata / 12 render. Insights: +5 metadata, +9 render. Back to Traffic: +0 downloads, no tile drops back to loading. 2 (short gallery) — confirmed. The widening is triggered by Customize ( So step 6 is decided by reviewer tempo. Following it literally lands inside the window; pausing to look at edit mode first hides it. Worth rewording the step to open the gallery immediately, or to assert the count rather than eyeball it. 3 (untested guard) — confirmed. Removed the 1 (missing-widget frame) — I cannot reproduce the DOM symptom; my original wording was too strong. I patched New, same root cause as 2: the widening burst trips a caller bound, contradicting testing step 8 ("no Add widget queues ~48 catalogs into 6 slots; late entries exceed the 5s bound. Catalog 404s are expected on a test site. |
…y scope `scopedRecords` fell back to the full record set whenever the scoped subset came out empty, on the theory that an empty dashboard force-opens edit mode and needs the whole registry anyway. That makes the scoping collapse if a layout is empty for even a single render — every catalog back on the boot critical path, non-deterministically, which is also enough queue traffic to push a later per-widget catalog past its own 5s bound and warn. `[]` already means "nothing on screen" in this option's contract, so resolve nothing. The genuinely empty dashboard is covered by `includeAll` a render later, at no boot cost. This closes one path from a transiently empty layout to a full preload. The other runs through `WidgetDashboard`, which force-opens edit mode when the layout is empty and so latches `includeAll` — that one is visible as the dashboard opening in Customize mode, and is not addressed here. Reported by @CGastrell in review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Your retraction on 1 is correct and I've taken it into the PR description — my write-up had asserted the DOM symptom as fact, which it wasn't. Traced why it can't render: Keeping the guard anyway, framed as a guard rather than a fix: the cancellation rests on an internal 2 — fixed, and it takes the warning with it. Once the widgets on screen have resolved, the rest of the registry's catalogs now download on the next idle callback, so Residual risk worth naming: the warm-up is itself a ~33-catalog batch, so a section switch landing while it drains could requeue a render catalog behind it. Arithmetic says that's ~1.6s at your latencies, comfortably inside the 5s bound, but if you see the warning move there rather than disappear, the background batch needs to yield instead of taking all six slots — I'd rather have your measurement than build that on a guess. Testing steps 6 and 8 are rewritten: open the gallery inside ~100ms and assert the count rather than eyeball it, and step 8 now names the warning and the queue arithmetic behind it so it isn't just "should be clean". 3 — fixed as described in my previous comment. One more, from your point about boot scoping not holding reliably (4211b71): That closes one route from a transiently empty layout to a full preload. The other runs through Generated by Claude. |
…'s width The loader's download queue is shared and FIFO with six slots, and each catalog's bound counts its queue wait. So a background batch that submits the whole registry at once pushes anything requested after it to the back — including a widget's own render catalog, which blocks that widget's import. Past the bound it gives up and the widget renders in English, which is what the `still pending after 5000ms` warning reports. Warm in batches of three instead. Holding at most half the slots means a download with a caller blocked on it starts immediately rather than waiting out the registry, so the warning goes back to meaning what it says: the origin was slow. Nothing awaits the warm-up, so trading its wall-clock for that is free. The gated preload stays unbatched — the registry gate and the widget picker are both waiting on it, so it should take the queue at full width. Suggested by @CGastrell in review, as the alternative to moving where the bound starts. Moving it would make the message honest by dropping the cap on how long a widget's import can block, which is the guarantee that keeps a stalled origin from wedging first render. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Took the queue fix rather than moving the bound (55206c2). The warm-up now warms three catalogs at a time instead of submitting the registry in one go. Since The gated preload stays unbatched, since the registry gate and the picker are both waiting on it and should have the queue at full width. On moving the bound to dequeue: the reason I'd avoid it is that the 5s span is doing two jobs, and only one of them is the message. The other is capping how long Your option 2 is now cheap to add on top if you still want it — with background work out of the way the remaining conflation is only downloads queueing behind their own batch-mates, where nobody is blocked, so I'd rather see whether the warning shows up at all first than add a status field for a reading we may not get. Testing step 8 is rewritten to try to provoke it: switch sections repeatedly in the first few seconds after load, which is when the warm-up is draining and newly mounted widgets are requesting their own render catalogs. That's the collision this is meant to prevent, so if it warns there, the batch size is wrong. Generated by Claude. |
2 participants
Fixes # N/A (JETPACK-2095, follow-up to #50904 — both items raised by @LiamSarsfield in that review)
Proposed changes
Two independent items that were deliberately left out of #50904 because the clean fixes lived outside it.
premium-analytics: keep widget metadata resolution off the initial render. UpstreamuseWidgetTypes()imports every record'swidget.jsmetadata module eagerly, with no resolver seam, and those modules evaluate__()at module scope — so wp-build dashboards: throttle i18n catalog downloads and load widget catalogs on demand #50904 had to gate the whole registry on preloading every record's catalog. That put one catalog request per registered widget on the boot critical path (~41 of them by the ticket's estimate) and added a grid delay that did not exist before.useWidgetTypesWithI18n()now handsuseWidgetTypes()only the records the active section's layout renders, then downloads the rest of the registry's catalogs on the next idle callback once those widgets have resolved. Edit mode still widens the registry explicitly, but by then it is adopting downloads that already happened.wp-build-polyfills: a stalled download no longer holds its concurrency slot forever.throttled()released a slot only fromtask().finally(), anddownloadI18n()awaits a barefetchwith no timeout and noAbortSignal. The callers'Promise.racebounds unblock only the caller; the request keeps its slot. WithMAX_CONCURRENT_DOWNLOADS = 6, six genuinely stalled downloads deadlocked the queue for the rest of the page. Each slot now carries a watchdog that returns it after 30s and lets the orphaned request finish in the background.Notes for reviewers
This defers the registry's catalogs; it does not drop them. An earlier revision of this PR downloaded only the visible widgets' catalogs and widened the registry when edit mode opened.
WidgetPickerhardcodesisLoading={ false }and takestotalItemsfrom whatever types the dashboard hands it, so it has no way to render "still loading" — during the widening it just showed a short gallery with a matching count (@CGastrell measured 12 of 44, for 3.26s). The same burst is what tripped the 5s per-widget catalog bound and produced a[jetpack-i18n]warning. The idle warm-up closes both:includeAllnow adopts promises already in the loader's download map (keyed by bundle path), so widening resolves in a microtask and queues nothing. The saving is therefore on the critical path, not in total bytes; after the page settles the registry is as complete as it was in #50904.The warm-up waits for the widgets on screen, and stays under the queue's width. Scheduling it any earlier would queue the whole registry into the same six-slot download queue the visible widgets are draining, putting the grid behind the registry — the inversion the scoping exists to prevent. For the same reason it warms three catalogs at a time rather than all at once: the queue is FIFO and each catalog's bound counts its queue wait, so a background batch that fills all six slots pushes a widget's own
render.jscatalog — which blocks that widget's import — past its bound, and the widget renders in English. Holding at most half the slots means anything with a caller blocked on it starts immediately. Nothing awaits the warm-up, so trading its wall-clock for that costs nothing.The gated preload deliberately does not batch: the registry gate and the widget picker are both waiting on it, so it takes the queue at full width.
That is also why this PR does not move where the bound starts. Measuring only the fetch would make the
still pendingmessage literally true, but the bound's other job is capping how long a widget's import can block — start it at dequeue and that cap becomesqueue wait + 5s, unbounded by queue depth, which is the stalled-origin-wedges-first-render shape this whole area exists to avoid. Keeping background work out of the way fixes the same conflation from the other end, and leaves the cap intact.visibleNamesis tri-state, and that is load-bearing.undefinedmeans "no scoping",nullmeans "the caller's layout has not resolved yet",[]means "genuinely nothing on screen". The middle state is not decoration: hooks cannot be skipped, sostage.tsxcalls this hook on the renders where it returns a spinner, and on those renderslayoutis empty. An earlier revision treated that as an empty layout and fell back to the full record set — which made the scoping a no-op on every real page load while every unit test still passed, because tests hand the hook a settled layout. There is a regression test named for exactly this.Why the resolving flag is derived rather than passed through — and why that is a guard, not a bug fix. Scoping closes the gate during render — the render-phase
setNeededNameswidens the scope and the newscopedRecordsarray lands the same pass — butuseWidgetTypes()only raisesisResolvingWidgetTypesfrom an effect, one commit later. A section switch therefore commits one frame holding the previous section's types while reporting resolved, which is the tuplewidget-chrome.tsxturns into "Missing widget" for any widget it cannot find.Nothing renders it today, and @CGastrell was right to push back on the original wording here.
WidgetDashboardkeeps its ownstagingLayoutinuseState, synced from thelayoutprop by an effect, and renders that — so the incoming section's tiles mount one commit after the layout changes, by which time the flag has already flipped. Two unrelated one-commit lags cancel out; he measured 158 aria-attribute ops across a section switch with"Missing widget"set zero times.The flag is still derived —
resolving || gatedRecords === null— because the cancellation is accidental. It rests on an internaluseState+useEffectsync in an upstream package, and the invariant this hook owes its caller is "withholding records means resolution is not finished", independent of who happens to be mounted. The suite could not see the bad tuple at all originally: itsuseWidgetTypesstand-in reported resolving synchronously during render, the exact timing that hides it. There is now a second stand-in timed the way upstream times it, asserting on every committed frame.An empty layout no longer collapses the scope.
scopedRecordsused to fall back to the full record set whenever the scoped subset came out empty, on the theory that an empty dashboard force-opens edit mode and needs the registry anyway. That makes the scoping collapse if a layout is empty for even one render, putting every catalog back on the boot critical path.[]already means "nothing on screen" in this option's contract, so it now resolves nothing;includeAllcovers the genuinely empty dashboard one render later at no boot cost. Note this closes only one route from a transiently empty layout to a full preload — the other runs throughWidgetDashboardforce-opening edit mode, which latchesincludeAll, and shows up as the dashboard opening in Customize mode.Why not
AbortSignalinjetpack-assets. Adding it todownloadI18n()would also cancel the wasted bytes, but it cannot replace the watchdog — the autoloader can hand the page an olderjetpack-assetscopy that ignores the signal, sothrottled()needs its own guarantee regardless. That makes abort purely additive, at the cost of a second package and a version-skew story, so it is left out.Known limitation. The queue state is parked on
windowand shared with any other copy of the module on the page, including copies built before the watchdog existed. A stalled task started by such a copy still holds its slot. Nothing in this package can fix that.Still open upstream. The clean fix for the first item is lazy per-widget metadata resolution in
@wordpress/widget-primitives; that is the path to ~16 requests, and the only thing that turns this deferral into a real reduction. This change reduces the cost of waiting for it and is written to fold away when it lands.WidgetPicker's missing loading state is worth reporting upstream separately.Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
Measured on a local site through a tunnel,
pt_BR, one full dashboard load each. Counts are of unique translation-catalog downloads; note that the browser's resource-timing buffer fills before the catalogs are requested, so the DevTools Network panel undercounts — readwindow.__jetpackWpBuildI18nCatalogs.downloadsinstead.widget.js)render.js)The first four rows were measured on the revision that had no idle warm-up; the warm-up starts only after the visible widgets have resolved, so it does not move them. The last row is the point of the change: the same catalogs are fetched, after the grid is on screen instead of before it.
en_USlocale, e.g. Português do Brasil.wp-admin/admin.php?page=jetpack-premium-analytics-wp-admin. The widgets on the Traffic tab should render with translated titles, with no longer a wait than trunk.[jetpack-i18n]warnings, in particular nostill pending after 5000msfor arender.jscatalog. That warning is queue arithmetic, not a network fault: the 6-slot download queue serves everything, and a per-widget catalog carries a 5s bound that includes its wait, so a burst of ~48 catalogs ahead of it pushes the tail past the bound. Two things now keep that from happening — the widening at Customize has nothing left to queue, and the warm-up itself never takes more than three of the six slots. Try to provoke it: switch sections repeatedly during the first few seconds after load, which is when the warm-up is draining and new widgets are requesting their own render catalogs. Catalog requests returning 404 is expected on a test site; no language packs exist for these bundles yet, and the UI falls back to English.en_US, no catalog requests should fire at all.Automated tests:
The slot watchdog is covered by three
node:testtests. One asserts a queued download starts after six stalled downloads give up their slots. One asserts a normally-settling download's watchdog is cancelled rather than merely silent —clearTimeoutis mocked alongsidesetTimeout, so ticking past the timeout proves cancellation. The third covers thereleasedguard itself: a stalled download whose watchdog frees its slot and which then settles, runningfinallyagainst a slot already back in the pool. Without the guard that drivesactiveto -5 and lets the next burst exceed the concurrency cap; the first two tests pass with the guard deleted, this one does not.