VideoPress: fix runaway render loop on admin library pagination#48411
Merged
Conversation
The admin-page useDashboardVideos hook had two anti-patterns that, on
WP 7.0-Beta sites, combined into a sustained ~12k commits/sec loop after
clicking pagination, starving the REST .then() handler of microtask slots.
- Extract useDashboardVideos to projects/packages/videopress/src/client/
admin/hooks/use-dashboard-videos/ to match project convention and to
decouple it from the heavy admin-page UI imports for unit testing.
- Drop tempPage.current from the URL/store sync useEffect deps array.
Refs do not participate in re-render scheduling; including .current
while mutating it inside the effect made it re-fire indefinitely.
- Replace the per-render uid() placeholder array with a useMemo of
deterministic placeholder-{i} ids so child keys stay stable across
renders while isFetching.
Adds 4 tests covering the two regressions and the surviving sync logic.
Fixes #48408
obenland
added
[Status] Needs Review
Contributor
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Contributor
|
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! Videopress plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a severe React render storm on the VideoPress admin library pagination path (notably on WordPress 7.0-beta), by stabilizing URL/store sync behavior and preventing placeholder list churn during fetches.
Changes:
- Extracts
useDashboardVideosinto a dedicated hook module to decouple it from the admin page component and enable isolated unit testing. - Fixes the URL/store pagination sync effect by removing a mutable ref (
tempPage.current) from the effect dependency array to prevent oscillation. - Stabilizes loading placeholders via
useMemoand deterministic IDs, and adds unit tests to cover the regressions and expected sync behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| projects/packages/videopress/src/client/admin/hooks/use-dashboard-videos/index.ts | New extracted hook implementing the pagination URL/store sync fix and stable placeholders. |
| projects/packages/videopress/src/client/admin/hooks/use-dashboard-videos/test/index.test.ts | Adds unit tests covering the prior render-loop triggers and expected URL/store sync behavior. |
| projects/packages/videopress/src/client/admin/components/admin-page/index.tsx | Switches the admin page to consume the extracted useDashboardVideos hook and removes inlined hook logic. |
| projects/packages/videopress/changelog/fix-videopress-admin-render-loop | Adds a changelog entry documenting the pagination render-loop fix. |
`parseInt(getParam('page', '1'))` returns NaN when the URL has a
non-numeric `?page=` value. The total>0 bounds check did not catch this
(every comparison with NaN is false), so the page-sync branch could
dispatch `setVideosQuery({ page: NaN })`. Pass an explicit radix and
fall back to 1 when the parsed value is NaN, and add a regression test
exercising `?page=foo`.
- Trim two verbose multi-line comments down to one line each. - Use `??` so the items spread isn't computed when placeholders are active.
Code Coverage SummaryCoverage changed in 1 file.
1 file is newly checked for coverage.
|
|
wow that was fast, thanks :) |
github-actions
Bot
removed
the
[Status] Needs Review
Copilot AI
pushed a commit
to dognose24/jetpack
that referenced
this pull request
May 4, 2026
…mattic#48411) Co-authored-by: dognose24 <6869813+dognose24@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #48408
Proposed changes
useDashboardVideosfromadmin-page/index.tsxtoprojects/packages/videopress/src/client/admin/hooks/use-dashboard-videos/, matching the project's convention for hooks and decoupling it from the heavy admin-page UI imports so it can be unit-tested in isolation.tempPage.currentfrom the URL/store syncuseEffect's dependency array. Refs do not participate in re-render scheduling; including.currentwhile the same effect mutates it caused the effect to alternate between its two branches indefinitely on every re-render.Array(n).fill({}).map(() => ({ id: uid() }))placeholder with auseMemokeyed on the inputs that determine count, using deterministicplaceholder-{i}IDs so children'suseEffect/useMemodeps stop seeing fresh references every render.total === 0, URL push after store-side page change).Together these stop the ~12k commits/sec render storm reported on WP 7.0-Beta admin pagination, which was starving the REST
.then()handler of microtask slots and leaving the library stuck on skeleton placeholders.Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
/wp-admin/admin.php?page=jetpack-videopressand wait for page 1 thumbnails to load fully.__REACT_DEVTOOLS_GLOBAL_HOOK__.onCommitFiberRootversion, or the MutationObserver fallback).?paged=5) still works.pnpm jest --testPathPatterns='use-dashboard-videos'fromprojects/packages/videopress/— 4 tests should pass.