fix(router): restore pages scroll traversal state - #1999
Conversation
commit: |
Pages Router HTML fallback could import the module URL from fetched __NEXT_DATA__ even when the generated page loader already existed. In Vite chunk graphs that can evaluate a duplicate page or router module, same-route param navigations remounted the page and lost listener state before routeChangeComplete. Store the router wrapper component types on the shared browser runtime singleton, prefer registered page loaders on the HTML fallback path, and resolve navigation at the root commit boundary that Next.js awaits instead of waiting for passive effects. Non-DOM document mocks still resolve immediately so unit harnesses do not masquerade as real browser commits. Validated with the upstream reload-scroll-backforward-restoration deploy suite and the local router-events and scroll-restoration regressions.
Move the pagesRouterReady bit into PagesRouterRuntimeState so duplicated next/router module instances (client entry + page chunks) observe the same readiness value. The initial value is still computed from shouldDeferInitialPagesRouterReady() once per document. Also tighten the scroll-restoration e2e fixture so routeChangeComplete assertions cannot pass from stale page state, and add a regression test that simulates two evaluated router instances on a deferred-ready page.
|
Verified the |
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: restore pages scroll traversal state
Reviewed the full diff, ran the targeted tests (tests/shims.test.ts runtime-state + HTML-fallback + popstate-scroll cases, tests/entry-templates.test.ts hydration-tree case) and vp check on router.ts — all green.
The core change is solid and well-reasoned:
- Moving per-document router state onto a
window-scoped runtime object (keyed bySymbol.for) correctly deduplicates state across independently-bundlednext/routerinstances while keeping SSR module instances isolated. The comments explaining thetypeof window === "undefined"carve-out and the real-browser-vs-jsdom distinction are excellent. - The deprecated-event-bridge install guard (
deprecatedEventBridgeInstalled) plus readingrouterRuntimeState.publicRouterat fire-time (rather than capturing a module-localRouter) is the right way to avoid double-installs and stale targets when a second chunk re-evaluates. - Preferring the registered route loader on the HTML-fallback path to keep component identity stable is a good fix and is directly covered by the new test.
- The DOM-text-based
expectPageShowsRouteChangeCompletee2e assertion is more robust than the prior event-listener race and matches the browser-observable contract.
A few non-blocking observations below. None block merge; the behavior is intentional and tested, but worth a comment or follow-up note.
|
|
||
| scrollToPagesRouterPosition(scroll); | ||
| if (isAtScrollPosition(scroll)) return; | ||
| } |
There was a problem hiding this comment.
Non-blocking: when a saved scroll target can't be reached (e.g. the new page is shorter than the saved y after a back/forward), isAtScrollPosition never converges and this burns all SCROLL_RESTORE_MAX_FRAMES (~60 frames ≈ 1s at 60fps) before resolving. Because routeChangeComplete is gated behind this loop, that case delays the completion event by ~1s.
The scroll-restoration fixture has tall content so it converges quickly, but real apps with variable-height pages could hit this. Worth a short comment noting the cap is intentionally a hard upper bound on completion-event delay, or consider breaking early once window.scrollY stops changing across frames (content can't grow further).
There was a problem hiding this comment.
Good catch. Implemented an early-exit: the loop now tracks previousScrollY and breaks when window.scrollY stops changing across consecutive frames (meaning the content can't grow further). The 60-frame SCROLL_RESTORE_MAX_FRAMES cap remains as a safety net for edge cases like lazy-loaded content that takes multiple frames to settle. Pushed in b6546a2.
|
|
||
| return router; | ||
| if (typeof window !== "undefined" && window.__VINEXT_PAGE_LOADERS__ !== undefined) { | ||
| return Router; |
There was a problem hiding this comment.
Non-blocking behavioral note: this fallback returns the module-local Router singleton, which derives pathname/query from window.__NEXT_DATA__ and is not subscribed to vinext:navigate. A useRouter() consumer that hits this fallback (i.e. renders outside PagesRouterProvider) won't re-render on client navigation, unlike one that gets the reactive provider value.
In practice wrapWithRouterContext always provides the context so this should only trigger in genuine edge cases, but it's a silent behavior shift from the previous "throw with a docs link" path. A brief comment documenting that the fallback is intentionally non-reactive (and only meant for the split-chunk compat case) would help future readers.
There was a problem hiding this comment.
Added an inline comment documenting the fallback intent — that it returns the module-local, non-reactive Router singleton meant only for the split-chunk compat case where useRouter() is evaluated outside PagesRouterProvider. Pushed in b6546a2.
| }; | ||
| routerRuntimeState.components = components; | ||
| return components; | ||
| } |
There was a problem hiding this comment.
Worth confirming the intent: getPagesRouterRuntimeComponents() caches the first evaluating instance's PagesRouterProvider/PagesRouterCommitBoundary on the shared runtime state, so a later-evaluated chunk's wrapWithRouterContext will mount instance-A's provider/boundary. That's correct for React element identity (the goal), and instance-A's provider closes over shared routerRuntimeState, so it stays consistent — the new runtime-state-sharing test exercises exactly this. Just flagging that the cross-instance closure capture is load-by-load deterministic only because all the captured helpers read shared state; if any future field on those components becomes module-local-instance-specific, this caching would silently pick instance A. No change needed now.
There was a problem hiding this comment.
Confirmed — the caching is safe today because PagesRouterProvider only reads from shared routerRuntimeState. If future additions introduce module-local state on those components, the caching would need revisiting. No change needed now.
|
Review submitted successfully. Summary of what I reviewed and posted on PR #1999: Verdict: Approve-with-notes (no blocking issues)I reviewed the full diff, ran the targeted tests (runtime-state-sharing, HTML-fallback-loader, popstate-scroll in The core change is solid:
Three non-blocking observations posted as inline comments:
No code changes were made (review-only mode). |
…ocument useRouter fallback Addresses ask-bonk review feedback on PR cloudflare#1999: - restorePagesRouterScrollPosition now breaks early when window.scrollY stops changing across frames, avoiding the full 60-frame budget when the saved scroll position exceeds the rendered page height. - useRouter() fallback path now has an explanatory comment documenting the non-reactive singleton return is intentional for split-chunk compat.
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: restore pages scroll traversal state
Reviewed the full diff and ran the targeted suites locally with vp:
vp test run tests/shims.test.ts -t "runtime state sharing|HTML fallback prefers the registered route loader|falls back to the singleton|popstate restores history-state scroll|Pages Router concurrent navigation"— all green (41 concurrent-nav + 4 new cases)vp test run tests/entry-templates.test.ts -t "keeps the Pages Router client tree shape stable after hydration"— greenvp check packages/vinext/src/shims/router.ts— no format/lint/type errors
The core design is solid and matches Next.js closely:
scrollToPagesRouterPositionis a faithful port of Next.js'sdisableSmoothScrollDuringRouteTransition(v16.2.6shared/lib/router/utils/disable-smooth-scroll.ts): thedataset.scrollBehavior === "smooth"gate, thegetClientRects()forced reflow beforescrollTo, and the restore of the priorstyle.scrollBehaviorall line up exactly.- Moving per-document state onto a
Symbol.for-keyedwindowobject correctly dedupes router state across splitnext/routerchunks while keeping SSR module instances isolated, and the install guards (deprecatedEventBridgeInstalled, components cache, readingpublicRouterat fire-time) avoid double-installs/stale targets. - Preferring the registered route loader on the HTML-fallback path is a good change — it also sidesteps the
import(/* @vite-ignore */ pageModuleUrl)branch that needsisValidModulePathvalidation, since the loader map is build-controlled. Well covered by the new test.
The prior /bigbonk review already covered the scroll-loop frame budget, the useRouter non-reactive fallback, and the component-cache cross-instance capture (all addressed). Two additional non-blocking observations below.
| scrollToPagesRouterPosition(scroll); | ||
| if (isAtScrollPosition(scroll)) return; | ||
|
|
||
| if (window.scrollY === previousScrollY) { |
There was a problem hiding this comment.
Non-blocking: the early-break unreachability check only tracks window.scrollY (previousScrollY), but the target has both x and y and isAtScrollPosition checks both axes. If a restored page is still growing horizontally (e.g. async wide content) while scrollY has already settled at an unreachable value, the loop breaks on the first stalled scrollY frame even though x could still become reachable. The common case (vertical scroll) is handled correctly, so this is just an edge-case note — consider tracking both axes (scrollX + scrollY) for symmetry, or add a comment that the break is intentionally vertical-only since that's the dominant restoration axis.
| }); | ||
| }); | ||
| async function expectPageShowsRouteChangeComplete(page: Page, expectedPath: string): Promise<void> { | ||
| await expect(page.locator("html")).toContainText(`routeChangeComplete:${expectedPath}`); |
There was a problem hiding this comment.
Non-blocking robustness note: toContainText("routeChangeComplete:/1") is a substring match, so it would also pass against routeChangeComplete:/12. The fixture only ever produces single-digit ids (/0, /1, /2), so this is safe today, but if the fixture ever grows to two-digit ids this assertion could match the wrong page. An exact-match locator (e.g. page.locator("p", { hasText: ... }) with toHaveText) would be more robust against that.
|
Review submitted successfully to PR #1999. SummaryVerdict: Approve-with-notes (no blocking issues) I reviewed the full diff, verified Next.js parity against the v16.2.6 source, and ran the targeted suites locally with
The core change is solid:
Two new non-blocking observations posted as inline comments:
The prior |
|
/bigbonk review for issues |
2 similar comments
|
/bigbonk review for issues |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: restore pages scroll traversal state
Reviewed the full diff and ran the targeted suites locally with vp — all green:
vp test run tests/shims.test.ts -t "runtime state sharing|HTML fallback prefers the registered route loader|falls back to the singleton|popstate restores history-state scroll"(4 passed)vp test run tests/entry-templates.test.ts -t "keeps the Pages Router client tree shape stable after hydration"(passed)vp check packages/vinext/src/shims/router.ts(clean — no format/lint/type errors)
Verdict: Approve-with-notes — no blocking issues.
The core change is solid and the design is well-reasoned:
- Moving per-document router state onto a
Symbol.for-keyedwindowobject correctly dedupes state across splitnext/routerchunks while keeping SSR module instances isolated. Thetypeof window === "undefined"carve-out and the jsdom-vs-real-browser comments are clear. - The
deprecatedEventBridgeInstalledguard plus readingrouterRuntimeState.publicRouterat fire-time correctly avoids double-installs and stale targets when a second chunk re-evaluates. - Preferring the registered route loader on the HTML-fallback path keeps React component identity stable and sidesteps the
@vite-ignoredynamic-import branch. Well covered by the new test. scrollToPagesRouterPositionfaithfully ports Next.js's smooth-scroll-disable behavior, and the early-break now correctly tracks both x and y, addressing the prior review note.- The e2e assertion uses
getByText(..., { exact: true }), which fully resolves the earlier substring-match (/1vs/12) concern.
The prior /bigbonk observations (scroll-loop budget, non-reactive useRouter fallback, component-cache cross-instance capture) were all addressed in b6546a2. A few additional non-blocking notes below.
| return true; | ||
| } | ||
|
|
||
| function markPagesRouterHydrated(): void { |
There was a problem hiding this comment.
Non-blocking behavioral note: this PR moves the __NEXT_HYDRATED / __VINEXT_HYDRATED_AT write out of the synchronous post-hydrateRoot() path (removed from pages-client-entry.ts and dev-server.ts) into this passive useEffect-driven marker. The markers are now set strictly later — after the first commit's passive effects flush rather than right after hydrateRoot() returns.
That's the intended fix (so page-level routeChangeComplete subscriptions are installed before readiness is observed), and waitForHydration / the Pages Router instrumentation-client timing assertion (instrumentation < hydration) still hold because the marker only moves later. Just flagging it because a wide set of Pages Router e2e specs gate on __VINEXT_HYDRATED_AT, so any future regression in the provider mount path would now also stall hydration detection. Worth a one-line note in the PR description that this is a deliberate ordering shift for the many specs that depend on the marker.
| // `useRouter().isReady` still uses the serialized readiness snapshot to | ||
| // avoid server/client markup drift during hydration. | ||
| return ( | ||
| isPagesRouterReady() && (typeof window === "undefined" || window.__NEXT_HYDRATED === true) |
There was a problem hiding this comment.
Non-blocking: window.next.router.isReady now additionally gates on window.__NEXT_HYDRATED === true, which is only ever flipped by PagesRouterHydrationMarker inside PagesRouterProvider. If a Pages Router document somehow renders without the provider ever committing (e.g. an early hydration error that the commit boundary swallows on the initial tree), the singleton isReady would stay false indefinitely while the provider-backed useRouter().isReady could still report true via the serialized snapshot. In normal flows wrapWithRouterContext always mounts the provider so this can't happen, but it's a new coupling between two readiness signals that previously were independent. No change needed; documenting the invariant (singleton readiness depends on the provider committing at least once) would help future debugging.
| return () => { | ||
| router.events.off("routeChangeComplete", handler); | ||
| }; | ||
| }, [router]); |
There was a problem hiding this comment.
Non-blocking (fixture-only): the effect deps are [router], and with this PR the provider's router object identity changes on every vinext:navigate snapshot. So on each navigation the effect tears down and re-subscribes the routeChangeComplete handler. It works here because subscribe/unsubscribe are symmetric and the reset-on-id-change runs in the same effect, but it means the handler is briefly absent across the re-run. Since routeChangeComplete for the incoming navigation fires after the new tree commits (and thus after this effect re-runs), the ordering happens to be safe. If this fixture pattern is ever copied as a reference, a router.events-only dependency (or an empty dep array reading idRef) would be less subtle.
|
Review submitted successfully to PR #1999. SummaryVerdict: Approve-with-notes — no blocking issues. I reviewed the full diff, traced the cross-module-instance state-sharing design, and ran the targeted suites locally with
The core change is solid:
Three new non-blocking observations posted as inline comments:
No code changes were made (review-only mode). |
2 participants
Summary
windowso production bundles with multiplenext/routermodule instances agree on history keys, events, cancellation, and popstate state.routeChangeCompletesubscriptions can observe the event.Root Cause
The deploy-suite fixture exposed two coupled production-only problems. First, the entry chunk and a page chunk could evaluate separate
next/routermodules, leaving the popstate listener andwindow.next.routerwith separate history-key counters/current-key state. After reload plus back/forward traversal, the stale popstate-side key could snapshot the outgoing scroll into the wrong__next_scroll_<key>entry and overwrite the position that should be restored later.Second, vinext emitted
routeChangeCompleteimmediately afterroot.render()resolved, before the newly rendered page's passive effects had subscribed to router events. The upstream fixture rendersrouteChangeCompleteonly from a page-leveluseEffectlistener, so the event ordering needed to match the browser-observable Next.js contract more closely.References
Verification
vp test run tests/shims.test.tsvp test run tests/entry-templates.test.ts -t "keeps the Pages Router client tree shape stable after hydration"vp test run tests/pages-router.test.ts -t "client entry statically imports next/router|installs the vinext dev error overlay"vp checkCI=1 PLAYWRIGHT_PROJECT=pages-scroll-restoration pnpm run test:e2e tests/e2e/pages-scroll-restoration/reload-scroll-backforward-restoration.spec.ts --reporter=linevp env exec --node 24 ./scripts/run-nextjs-deploy-suite.sh /Users/nathan/Projects/vinext/.refs/nextjs-v16.2.6 --retries 0 -c 1 --debug test/e2e/reload-scroll-backforward-restoration/index.test.ts