fix(pages-router): restore scroll across reload history traversal - #1905
Conversation
commit: |
|
/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.
Reviewed the scroll-restoration changes. The history-key tracking, sessionStorage snapshots, and runtime-install ordering all look sound and closely mirror the upstream Next.js behavior, with good test coverage including the ported regression test.
One correctness concern worth fixing before merge: the new commit boundary uses a raw useLayoutEffect, but wrapWithRouterContext is shared with the server-side SSR render path, so this will emit React's "useLayoutEffect does nothing on the server" warning on every Pages Router SSR render. The codebase already established the SSR-safe pattern for exactly this situation (shims/image.tsx). A couple of smaller observations inline as well.
| }): ReactElement { | ||
| useLayoutEffect(() => { | ||
| onCommit(); | ||
| }, [onCommit]); |
There was a problem hiding this comment.
wrapWithRouterContext is not client-only — it is invoked on the server during Pages Router SSR (e.g. server/pages-page-handler.ts:511, entries/pages-server-entry.ts:378, and several call sites in server/dev-server.ts). Since PagesRouterCommitBoundaryHelper always calls useLayoutEffect, React will log
Warning: useLayoutEffect does nothing on the server, because its effect cannot be observed during rendering.
for every Pages Router SSR render. On the server onCommit is the noopCommit default anyway, so the layout effect is pure overhead there.
This repo already established the SSR-safe pattern for exactly this case in shims/image.tsx:125:
// A version of useLayoutEffect that doesn't warn during SSR.
const useNonWarningLayoutEffect = typeof window === "undefined" ? useEffect : useLayoutEffect;Next.js itself only runs the layout-effect callback on the client (packages/next/src/client/index.tsx), not in the shared render path. Suggest adopting the same guard (useEffect is already imported):
| }, [onCommit]); | |
| useNonWarningLayoutEffect(() => { | |
| onCommit(); | |
| }, [onCommit]); | |
| return createElement(Fragment, null, children); | |
| } | |
| const useNonWarningLayoutEffect = typeof window === "undefined" ? useEffect : useLayoutEffect; |
(Define useNonWarningLayoutEffect at module scope rather than inline; placement is illustrative.)
| onCommit: () => void; | ||
| onError: (error: Error) => void; | ||
| }> { | ||
| componentDidCatch(error: Error) { |
There was a problem hiding this comment.
PagesRouterCommitBoundary implements only componentDidCatch and never updates state via getDerivedStateFromError, so on a caught error React unmounts the whole subtree. For the navigation path this is intended (the rejection drives the hard-nav fallback in runNavigateClient). But this same boundary now also wraps the initial hydration tree (pages-client-entry.ts:185 / dev-server.ts:1387) with the default onError = noopCommit, so a hydration-time render error is now caught and routed to a no-op instead of surfacing as before. React still console.errors the caught error, so it is not fully silent, but it is a behavior change vs. the pre-PR tree that had no boundary. Worth confirming this is intentional — a brief comment on wrapWithRouterContext noting that the default-noop boundary also governs hydration would help future readers.
| if (doScroll) { | ||
| if (hash) scrollToHashTarget(hash); | ||
| else window.scrollTo(0, 0); | ||
| } |
There was a problem hiding this comment.
Minor ordering change for the shallow path: previously the post-routeChangeComplete block scrolled (hash or (0,0)) for both shallow and non-shallow when doScroll was set. Now shallow navigations scroll in this else branch before routeChangeComplete, while the non-shallow no-hash case relies on the (0,0) reset inside the render-commit callback. The reordering for shallow is observable to routeChangeComplete listeners (scroll now applied before the event rather than after). Probably fine since shallow routing rarely changes scroll, but it is an untested ordering shift — consider adding a shallow-routing assertion or a note that the divergence is intentional.
|
Review submitted successfully. Summary of what I found in PR #1905: Review postedOverall: The core implementation (history-key tracking, sessionStorage scroll snapshots, runtime-install ordering, commit-boundary lifecycle) is sound and closely mirrors upstream Next.js, with strong test coverage including a ported regression test. Main issue (correctness):
Smaller observations:
No blocking concerns beyond the SSR |
…allow scroll ordering Address bonk review on cloudflare#1905: - swap the commit boundary's raw useLayoutEffect for the SSR-safe useNonWarningLayoutEffect pattern from shims/image.tsx so Pages Router SSR renders no longer log React's useLayoutEffect warning - document that the default-noop commit boundary intentionally also wraps SSR and initial hydration - document the intentional shallow-scroll-before-routeChangeComplete ordering and add a unit test pinning it
|
/bigbonk review for issues |
There was a problem hiding this comment.
Reviewed the scroll-restoration changes. The opt-in path (history-key tracking, sessionStorage snapshots, runtime-install ordering, commit boundary) is sound and mirrors upstream Next.js closely, with good coverage from the ported regression test. The prior useLayoutEffect-on-SSR concern has already been addressed via useNonWarningLayoutEffect.
One concern I'd want resolved before merge: this PR appears to silently regress back/forward scroll restoration for Pages Router apps that do not opt into experimental.scrollRestoration. Details inline.
| void (async () => { | ||
| const scrollTarget = manualScrollRestoration | ||
| ? (forcedScroll ?? readScrollPosition(state) ?? { x: 0, y: 0 }) | ||
| : null; |
There was a problem hiding this comment.
Regression for apps without experimental.scrollRestoration.
On main, the popstate handler unconditionally called restoreScrollPosition(e.state) after routeChangeComplete, restoring the __vinext_scrollX/Y saved in history state by saveScrollPosition() on every back/forward — for all Pages Router apps, regardless of the flag.
Now scrollTarget is null whenever manualScrollRestoration is false, so renderPagesRouterElement does no scrolling, and there is no other scroll-restore on the popstate path. For an app without the experimental flag, back/forward scroll position is no longer restored.
This matters because vinext's Pages Router popstate is a soft navigation (it re-fetches /_next/data and calls root.render()). The browser's native "auto" scroll restoration cannot restore the correct position for content that is re-rendered after the popstate fires — which is exactly why the old code compensated with restoreScrollPosition. Dropping it leaves these apps scrolled to the wrong position on back/forward.
Note that saveScrollPosition() still writes __vinext_scrollX/Y into history state on push (line 537), and readScrollPosition(state) can already read that shape — so the fix is likely just to also honor it when the flag is off. Suggestion:
| : null; | |
| // When manual scroll restoration is enabled we drive the position from the | |
| // sessionStorage snapshot keyed by history key. When it is disabled we | |
| // still restore the per-entry scroll saved in history state on push, since | |
| // a soft popstate re-renders content the browser's native restoration | |
| // can't position correctly. | |
| const scrollTarget = manualScrollRestoration | |
| ? (forcedScroll ?? readScrollPosition(state) ?? { x: 0, y: 0 }) | |
| : readScrollPosition(state); |
If this drop is intentional (e.g. you verified Next.js itself does not restore here for non-flag apps), please add a test pinning the new behavior and a comment explaining the divergence — the new fixture only covers the flag-enabled case, so nothing currently guards this.
| if (manualScrollRestoration) { | ||
| const currentKey = _currentHistoryKey; | ||
| if (currentKey !== undefined && currentKey !== targetKey) { | ||
| saveScrollPositionToSessionStorage(currentKey, getWindowScrollPosition()); |
There was a problem hiding this comment.
Minor: this saves getWindowScrollPosition() under currentKey at popstate time. This is correct only because history.scrollRestoration = "manual" is set when manualScrollRestoration is true, so the browser hasn't already moved window.scrollX/Y to the target entry's position by the time popstate fires. Worth a one-line comment noting the dependency on manual mode, since it would silently capture the wrong position if the manual guard ever drifted from this block's guard.
| if (hash) scrollToHashTarget(hash); | ||
| else window.scrollTo(0, 0); | ||
| } | ||
| } |
There was a problem hiding this comment.
The shallow path now scrolls before routeChangeComplete, whereas on main the scroll ran after the event for both shallow and non-shallow. The new tests/shims.test.ts test pins ["scrollTo:0,0", "routeChangeComplete"], so the ordering change is at least covered — good. Just flagging that this is an observable behavior change for any shallow-routing routeChangeComplete listener that reads scroll position; if that's intentional (matching the non-shallow commit-callback ordering, as the comment says), no action needed.
|
Review submitted successfully. Review postedOverall: The opt-in scroll-restoration path is well implemented — history-key tracking, sessionStorage snapshots, runtime-install ordering, and the commit boundary closely mirror upstream Next.js, with solid coverage from the ported regression test. The earlier Main concern (potential regression):
Smaller notes:
No blocking issues beyond confirming/fixing the non-flag back/forward scroll behavior. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Reviewed the Pages Router scroll-restoration changes. The implementation closely mirrors upstream Next.js: history-key tracking, sessionStorage snapshots keyed __next_scroll_<key>, the history.scrollRestoration = "manual" opt-in guard, runtime-install ordering before window.next.router is exposed, and the commit-boundary lifecycle so routeChangeComplete represents a committed route. Test coverage is strong (ported regression test + focused unit tests), and the prior-round concerns (SSR useLayoutEffect warning, non-flag back/forward regression) have both been addressed.
No blocking issues. One latent correctness concern (a render-commit canceller race) and two smaller observations below.
| if (typeof document === "undefined") { | ||
| _cancelPendingRenderCommit = null; | ||
| resolve(); | ||
| } |
There was a problem hiding this comment.
Latent race: the render-commit callbacks unconditionally null the module-level _cancelPendingRenderCommit, even after a newer render has replaced it.
Flow that triggers it:
- Navigation A calls
renderPagesRouterElement→ installscancellerA(line 141). - Navigation B starts;
navigateClient(line 1664) +cancelPreviousRenderCommitreject A's promise and set_cancelPendingRenderCommit = null, then B'srenderPagesRouterElementinstallscancellerB. - A's React tree commits late (concurrent React can still run the old boundary's layout effect before unmount). A's
onCommitruns_cancelPendingRenderCommit = null(line 156) — nullingcancellerB. - Navigation C's
cancelPreviousRenderCommit()is now a no-op, so B's render-commit promise can hang if C supersedes B before B commits.
The onError callback (line 165) and the document === undefined branch (line 171) have the same unconditional-null. Capturing the canceller identity and only clearing when it's still current closes the window:
| } | |
| return new Promise<void>((resolve, reject) => { | |
| const cancel = () => { | |
| if (_cancelPendingRenderCommit === cancel) _cancelPendingRenderCommit = null; | |
| reject(new NavigationCancelledError("superseded")); | |
| }; | |
| _cancelPendingRenderCommit = cancel; | |
| const clearIfCurrent = () => { | |
| if (_cancelPendingRenderCommit === cancel) _cancelPendingRenderCommit = null; | |
| }; | |
| const scrollHandler = () => { | |
| if (scroll) { | |
| window.scrollTo(scroll.x, scroll.y); | |
| } | |
| }; | |
| root.render( | |
| wrapWithRouterContext( | |
| element, | |
| () => { | |
| clearIfCurrent(); | |
| try { | |
| scrollHandler(); | |
| resolve(); | |
| } catch (err) { | |
| reject(err); | |
| } | |
| }, | |
| (error) => { | |
| clearIfCurrent(); | |
| reject(error); | |
| }, | |
| ), | |
| ); | |
| if (typeof document === "undefined") { | |
| clearIfCurrent(); | |
| resolve(); | |
| } | |
| }); |
Low probability in practice (the superseded tree usually unmounts before its layout effect fires), so non-blocking — but worth hardening since the symptom (a hung navigation promise) is exactly what the _cancelPendingRenderCommit machinery exists to prevent.
| // Match Next.js's Router constructor: stamp the initial history entry and | ||
| // attach the popstate listener while next/router itself is evaluating, | ||
| // before window.next.router is exposed to userland. | ||
| installPagesRouterRuntime(); |
There was a problem hiding this comment.
Moving installPagesRouterRuntime() to the next/router module top-level (instead of the explicit call in the Pages client entries) means the popstate handler + replaceState history stamp now run whenever next/router is evaluated as a value import — including in an App Router app where userland imports next/router directly. next/compat/router is type-only so it's unaffected, and Next.js's own Router constructor runs at module eval too, so this is defensible parity. Just flagging it as an observable behavior change vs. main (where install was Pages-entry-gated) in case an App Router fixture imports next/router for compat. Worth a one-line note in the comment that this install is intentionally unconditional at module eval.
| * nothing on the server" warning on every render. Same pattern as | ||
| * `shims/image.tsx`; Next.js only runs the commit callback on the client. | ||
| */ | ||
| const useNonWarningLayoutEffect = typeof window === "undefined" ? useEffect : useLayoutEffect; |
There was a problem hiding this comment.
Good catch addressing the SSR useLayoutEffect warning here — matches the established shims/image.tsx pattern. Note that PagesRouterCommitBoundaryHelper's effect depends on [onCommit], and onCommit is a fresh closure on every renderPagesRouterElement call, so the layout effect re-fires on each commit. That's correct for the single-render-per-navigation case and harmless for hydration (default noopCommit), so no action needed — just confirming the dependency array is intentional rather than a stale-closure guard.
|
Review submitted successfully. Review postedOverall: The opt-in scroll-restoration path is well implemented and closely mirrors upstream Next.js — history-key tracking, sessionStorage snapshots keyed Main finding (non-blocking, latent correctness):
Smaller observations:
No blocking issues found. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Reviewed the Pages Router scroll-restoration changes. The implementation is sound and closely mirrors upstream Next.js: the experimental.scrollRestoration config -> client define plumbing, history-key tracking, __next_scroll_<key> sessionStorage snapshots, the history.scrollRestoration = "manual" opt-in guard, runtime-install ordering before window.next.router is exposed, the commit-boundary lifecycle, and the SSR-safe useNonWarningLayoutEffect. The three substantive concerns from prior review rounds (SSR useLayoutEffect warning, non-flag back/forward regression, render-commit canceller race) all appear resolved in this version, and the unit + ported e2e coverage is strong. I ran the new shims.test.ts and next-config.test.ts cases locally and they pass.
One minor non-blocking observation below; no blocking issues.
| } | ||
|
|
||
| if (targetKey !== undefined) { | ||
| _currentHistoryKey = targetKey; |
There was a problem hiding this comment.
Minor (non-blocking): _currentHistoryKey is updated to targetKey here, before the beforePopState callback runs below (line 2348). If beforePopState returns false and cancels the navigation, the app stays on the previous entry but _currentHistoryKey now points at the target key. This is inconsistent with how _lastPathnameAndSearch is deliberately deferred until after beforePopState confirms the navigation proceeds (line 2357-2360, with the explanatory comment). A later saveScrollPosition() / popstate would then key off the wrong entry.
The scroll snapshot/read above it is fine to compute eagerly (it's local), but the commit of _currentHistoryKey ideally moves below the beforePopState early-return to match the _lastPathnameAndSearch handling. Low impact since beforePopState cancellation is rare, and the new fixture doesn't exercise it.
|
Review submitted successfully. SummaryOverall: This is a well-implemented opt-in scroll-restoration path that closely mirrors upstream Next.js v16.2.6. The config→client-define plumbing, history-key tracking, Prior-round concerns — all resolved in this version:
One new minor observation (non-blocking):
No blocking issues found. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Reviewed the Pages Router scroll-restoration changes end to end. The implementation is sound and mirrors upstream Next.js v16.2.6 closely: experimental.scrollRestoration → process.env.__NEXT_SCROLL_RESTORATION client define, history-key tracking, __next_scroll_<key> sessionStorage snapshots, the history.scrollRestoration = "manual" opt-in guard (only flipped when the flag is on, history supports it, and sessionStorage is usable), runtime-install ordering before window.next.router is exposed, and the commit-boundary lifecycle so routeChangeComplete represents a committed route.
I verified the substantive concerns from the four prior review rounds are all resolved in this version:
- SSR
useLayoutEffectwarning →useNonWarningLayoutEffect(router.ts:95), matchingshims/image.tsx. - Non-flag back/forward regression → popstate still honors
__vinext_scrollX/Yfrom history state when the flag is off (router.ts:2394-2396), covered by a new unit test. - Render-commit canceller race →
clearIfCurrentidentity check (router.ts:150-152, 164, 173). _currentHistoryKey-before-beforePopState→ the commit at router.ts:2359-2361 is now correctly placed after theif (!shouldContinue) return;early-return (router.ts:2350), consistent with_lastPathnameAndSearch.
Ran vp check on the touched files plus the targeted unit suites (next-config.test.ts -t scrollRestoration, shims.test.ts -t "Pages Router concurrent navigation") locally — all green. No blocking issues. Two minor, non-blocking observations below.
| // Reading window scroll here is only correct because manualScrollRestoration | ||
| // implies history.scrollRestoration = "manual", so the browser hasn't yet | ||
| // moved the viewport to the target entry's position when popstate fires. | ||
| saveScrollPositionToSessionStorage(currentKey, getWindowScrollPosition()); |
There was a problem hiding this comment.
Minor (non-blocking): the outgoing-entry sessionStorage snapshot is written here before the beforePopState early-return at line 2350. If beforePopState cancels the traversal, the app stays on the current entry but its scroll has already been re-snapshotted under currentKey. That's harmless (it's the entry the app remains on, and the value is the live scroll), but it's a subtle asymmetry with the _currentHistoryKey commit which you deliberately deferred until after the cancel check. Worth a one-line note that the eager snapshot is intentional since currentKey is the entry we stay on either way.
| /> | ||
| <p>{ready ? "routeChangeComplete" : "loading"}</p> | ||
| <Link | ||
| href={`/${id + 1}`} |
There was a problem hiding this comment.
id arrives from context.query as a string, so `/${id + 1}` does string concatenation (/0 → /01, not /1). The spec navigates via pushWithPagesRouter(page, "/1") rather than clicking this Link, so the assertions aren't affected, and the upstream fixture has the same shape — but if anyone later switches the test to click #link the href won't match the expected next route. Non-blocking; flagging so it isn't mistaken for a working increment.
|
Review submitted successfully. Review postedOverall: The opt-in Pages Router scroll-restoration path is well implemented and closely mirrors upstream Next.js v16.2.6 — the config→client-define plumbing, history-key tracking, Prior-round concerns — verified resolved in this version:
Two minor, non-blocking observations:
No blocking issues found. |
|
/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.
Reviewed the Pages Router scroll-restoration changes independently. The implementation is sound and closely mirrors upstream Next.js v16.2.6: the experimental.scrollRestoration -> process.env.__NEXT_SCROLL_RESTORATION client define, history-key tracking via _currentHistoryKey, __next_scroll_<key> sessionStorage snapshots, the history.scrollRestoration = "manual" opt-in guard (flag + history support + usable sessionStorage), the runtime-install-before-window.next.router ordering, the commit-boundary lifecycle so routeChangeComplete represents a committed route, and the SSR-safe useNonWarningLayoutEffect.
I verified the substantive concerns from the prior six review rounds are all resolved in this version:
- SSR
useLayoutEffectwarning ->useNonWarningLayoutEffect(router.ts:95) - Non-flag back/forward regression -> popstate still honors
__vinext_scrollX/Yfrom history state when the flag is off (router.ts:2397-2399), with a new unit test - Render-commit canceller race ->
clearIfCurrentidentity check (router.ts:150-152, 164, 173) _currentHistoryKey-before-beforePopState-> commit now correctly after the cancel early-return (router.ts:2362-2364)
No blocking issues. Two genuinely-unaddressed minor observations below, both narrow edge cases outside the ported fixture's scope.
| } | ||
| _lastPathnameAndSearch = browserUrl; | ||
|
|
||
| if (isHashOnly) { |
There was a problem hiding this comment.
Non-blocking edge case: when manualScrollRestoration is enabled, a hash-only back/forward (isHashOnly) returns here after only scrollToHashTarget(...). The per-entry __next_scroll_<targetKey> snapshot computed above as forcedScroll (line 2342) is discarded, so the target entry's saved x/y is never restored — only the hash anchor is honored. With manual restoration, the browser's native "auto" restoration is disabled, so a hash-only entry that was scrolled to a non-anchor position won't restore. The PR explicitly lists broader hash-scroll coverage as a non-goal, so flagging rather than blocking — but the fixture doesn't exercise hash-only popstate, so nothing currently guards this path.
| // attach the popstate listener while next/router itself is evaluating, | ||
| // before window.next.router is exposed to userland. This install is | ||
| // intentionally unconditional at module eval — any value import of | ||
| // next/router (even from an App Router app) triggers it, mirroring |
There was a problem hiding this comment.
Non-blocking: combining this unconditional installPagesRouterRuntime() at module eval with the now-global process.env.__NEXT_SCROLL_RESTORATION define means an App Router app that (a) sets experimental.scrollRestoration: true and (b) imports next/router as a value for compat would flip history.scrollRestoration = "manual" via stampInitialHistoryState -> installManualScrollRestoration, even though App Router owns its own scroll behavior. Prior review flagged the unconditional install generally; this is the specific interaction with the new flag. Narrow (requires both conditions), but worth a one-line note that the manual-restoration opt-in is intended to be Pages-Router-only, or gating installManualScrollRestoration behind a router-mode check if App Router parity matters.
|
Review posted successfully. SummaryIndependently reviewed PR #1905 (Pages Router scroll restoration). The implementation is sound and closely mirrors upstream Next.js v16.2.6. Verified resolved (from prior 6 review rounds):
Two new minor, non-blocking observations posted inline:
No blocking issues found. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Reviewed the Pages Router scroll-restoration changes. The runtime logic (history-key tracking, __next_scroll_<key> sessionStorage snapshots, the history.scrollRestoration = "manual" opt-in guard, runtime-install ordering before window.next.router is exposed, the render-commit boundary, and the SSR-safe useNonWarningLayoutEffect) is sound and closely mirrors upstream Next.js v16.2.6. The substantive concerns from the prior review rounds all appear resolved in this version, and the unit coverage in shims.test.ts / next-config.test.ts is thorough.
One blocking finding the prior rounds missed: the new pages-scroll-restoration Playwright project — the ported regression test that is the headline validation of this PR — is defined in playwright.config.ts but never wired into CI, so it does not run on PRs or on main. Details inline. This should be fixed before merge, otherwise the regression this PR fixes is unguarded going forward.
No other blocking issues.
| timeout: 60_000, | ||
| }, | ||
| }, | ||
| "pages-scroll-restoration": { |
There was a problem hiding this comment.
Blocking: this new E2E project is never run in CI.
Every other key in projectServers has a matching entry in the E2E job matrix in .github/workflows/ci.yml (pages-router, pages-router-prod, app-router-bfcache, cloudflare-workers, static-export, etc.). pages-scroll-restoration is the only project added here without a corresponding matrix entry, so vp exec playwright test with PLAYWRIGHT_PROJECT=pages-scroll-restoration is never invoked in CI.
I confirmed this against the live PR checks — there is no E2E (pages-scroll-restoration) job in the status rollup. The result is that the ported Next.js regression test (reload-scroll-backforward-restoration.spec.ts), which is the central validation cited throughout the PR description, only runs when a maintainer manually exports PLAYWRIGHT_PROJECT=pages-scroll-restoration locally. After merge nothing guards against a regression in this exact behavior.
Please add the project to the matrix in .github/workflows/ci.yml (in the E2E job's matrix.include), e.g.:
- project: pages-scroll-restoration
label: pages-scroll-restoration
shardIndex: 1
shardTotal: 1(Place it alongside the other single-shard Pages Router entries.)
| // still restore the per-entry scroll saved in history state on push, since | ||
| // a soft popstate re-renders content the browser's native restoration | ||
| // can't position correctly. | ||
| const scrollTarget = manualScrollRestoration |
There was a problem hiding this comment.
Non-blocking: when manualScrollRestoration is off, a soft popstate to an entry that has no __vinext_scrollX/Y in state resolves scrollTarget to null (readScrollPosition(state) returns null), so renderPagesRouterElement does not scroll at all on that branch. On main the old restoreScrollPosition(e.state) was also a no-op for entries lacking the saved shape, so this is parity-preserving — but worth confirming the intent is "only restore entries we explicitly stamped on push," since unstamped entries (e.g. a back to the very first server-rendered entry whose state was never written by saveScrollPosition) now neither restore from history-state nor fall through to native auto restoration (native is still enabled when the flag is off, so the browser handles it). Just flagging that the two code paths (manualScrollRestoration ? ... : readScrollPosition(state)) have different fallbacks ({x:0,y:0} vs null) and the asymmetry is intentional.
|
Review posted successfully. SummaryReviewed PR #1905 (Pages Router scroll restoration). The runtime logic has already been hardened across 7 prior review rounds and is sound, but I found one material issue that those rounds (which focused on Blocking finding — new E2E test never runs in CI:
Non-blocking observation:
Verified resolved (from prior rounds): SSR |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Reviewed the current HEAD (5bda29d). The opt-in Pages Router scroll-restoration path is well implemented and closely mirrors upstream Next.js v16.2.6: the experimental.scrollRestoration → process.env.__NEXT_SCROLL_RESTORATION define plumbing, history-key tracking via _currentHistoryKey, __next_scroll_<key> sessionStorage snapshots, the history.scrollRestoration = "manual" opt-in guard, runtime-install ordering before window.next.router is exposed, the commit-boundary lifecycle, and the SSR-safe useNonWarningLayoutEffect.
Verified resolved — every substantive concern from the 8 prior review rounds is addressed in this version:
- CI matrix:
pages-scroll-restorationis now wired into thee2ejob matrix (.github/workflows/ci.yml:305-308) — the round-8 blocker is fixed by the HEAD commit. - SSR
useLayoutEffectwarning →useNonWarningLayoutEffect(router.ts:95). - Non-flag back/forward regression → popstate honors
__vinext_scrollX/Ywhen the flag is off (router.ts:2425), with a unit test. - Render-commit canceller race →
clearIfCurrentidentity check (router.ts:150-152). _currentHistoryKey-before-beforePopStateordering → committed after the cancel early-return (router.ts:2374).- App Router manual-restoration gate →
installManualScrollRestorationcheckswindow.next?.appDir(router.ts:216), with a unit test. - Fixture link increment →
Number(id) + 1.
No blocking issues. One minor, non-blocking nuance below.
| } | ||
| } | ||
|
|
||
| const manualScrollRestoration = |
There was a problem hiding this comment.
Non-blocking nuance: the module-level manualScrollRestoration constant does not include the window.next?.appDir check that installManualScrollRestoration (line 216) uses to keep manual restoration Pages-Router-only. The save/read sessionStorage branches in the popstate handler (lines 2341, 2350, 2353-2355) gate on this constant, not on the App Router check.
In practice this is harmless because the Pages Router popstate handler early-returns on any object state lacking __N: true (line 2299), and an App Router document's history entries are not stamped with the Pages Router state shape — so the sessionStorage writes never actually fire there. But the asymmetry means manualScrollRestoration being true on an App Router doc is relying on a second guard (the __N filter) rather than the same appDir gate used for the history.scrollRestoration flip. If a future change ever made the popstate path reachable for App Router state, the sessionStorage writes would leak. Worth a one-line comment noting that the __N filter is what keeps these branches Pages-only, or folding the appDir check into the constant for symmetry.
|
Review submitted successfully. SummaryReviewed PR #1905 (Pages Router scroll restoration) at HEAD commit Verified resolved:
No blocking issues. I posted one minor, non-blocking observation: the module-level The config→define plumbing, runtime-install ordering, commit-boundary lifecycle, and test coverage (unit + ported e2e regression test) are all sound and closely mirror upstream Next.js v16.2.6. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Independent review at HEAD 1077acf (newer than the commits earlier rounds referenced). I read the current tree rather than the diff alone, traced the reload / back / forward / push / hash code paths through router.ts, verified the config→process.env.__NEXT_SCROLL_RESTORATION define plumbing (index.ts:1331, next-config.ts:1524), confirmed the CI matrix wiring (ci.yml:305-308), and verified the reload-recovery path (stampInitialHistoryState recovers _currentHistoryKey from the persisted entry at router.ts:531).
The runtime implementation is sound and closely mirrors upstream Next.js v16.2.6: _currentHistoryKey tracking, __next_scroll_<key> sessionStorage snapshots, the history.scrollRestoration = "manual" opt-in guard (flag + appDir gate + history support + usable sessionStorage at router.ts:214), runtime-install ordering before window.next.router is exposed, the commit-boundary lifecycle, and the SSR-safe useNonWarningLayoutEffect. Every substantive correctness concern from the prior review rounds (SSR useLayoutEffect warning, non-flag back/forward regression, render-commit canceller race via clearIfCurrent, _currentHistoryKey/beforePopState ordering, App Router appDir gate, CI matrix wiring, hash-only push snapshot) is resolved at this HEAD.
One blocking, mechanical finding (still unresolved): the PR is in a conflicting merge state with no CI running. The GitHub API reports mergeable: CONFLICTING / mergeStateStatus: DIRTY and an empty statusCheckRollup at HEAD 1077acf — the required checks (Check, Vitest, Playwright E2E) are not running on this branch. main has advanced materially since the branch point — notably fix(router): support experimental gesture push (#1909) and perf(server): eliminate redundant per-request parsing in the request pipeline (#1929), the former of which edits router.ts (the file with the largest change here, +373/-51) — so the conflict is real, not stale metadata. This needs a rebase onto main so conflicts resolve and the newly-wired pages-scroll-restoration E2E project (this PR's headline validation) actually runs before merge. The runtime logic being correct is moot until CI can verify it on a mergeable tree.
No new code-level blocking issues found.
|
Review submitted successfully. SummaryI performed an independent review of PR #1905 (Pages Router scroll restoration) at HEAD The runtime implementation is sound and closely mirrors upstream Next.js v16.2.6:
Verified resolved — every substantive concern from the prior 17 review rounds, including the round-8 CI-wiring blocker ( One blocking, mechanical finding: the PR is in a conflicting merge state. The GitHub API reports No new code-level blocking issues found. |
afd7886 to
1077acf
Compare
Pages Router pages with experimental.scrollRestoration currently expose window.next.router before the popstate runtime is installed and only keep Vinext scroll coordinates in the in-memory history state. After a reload, a back or forward traversal can run before the runtime is ready and has no durable scroll snapshot for the target history entry. The violated invariant is that the Pages Router owns browser scroll restoration whenever the Next.js scroll restoration flag is enabled, with one stable history key per entry and routeChangeComplete emitted after the new page commits. Resolve the Next.js config flag into process.env.__NEXT_SCROLL_RESTORATION, install the Pages Router runtime during next/router evaluation, persist Next-style sessionStorage scroll snapshots per history key, and render navigations through a stable commit boundary before emitting routeChangeComplete. Regression coverage ports Next.js reload-scroll-backforward-restoration and verifies manual restoration plus exact scroll positions across reload, back, and forward traversal.
…ion and settle on render failures
Move hash scrolling out of renderPagesRouterElement's commit callback and into the post-routeChangeComplete path, matching Next.js ordering: x/y restoration happens during render commit, hash scrolling happens after the completion event. Also fix E2E test expectRouteChangeComplete to use one-shot event listeners instead of checking for stale text, and add listener cleanup to the fixture to prevent accumulation.
…2E helper expectRouteChangeComplete() now throws if window.next.router.events is absent instead of silently resolving. Remove the redundant expectRouteChangeComplete call after pushWithPagesRouter in the third test — the awaited router.push already resolves after routeChangeComplete, so registering a listener after it would deadlock.
…in E2E scroll tests
…te and cancel pending render on supersede
- Pass {x:0, y:0} through commit boundary for all scrollable
navigations, not just hash-less ones (hash scroll deferred to after
routeChangeComplete as before).
- Cancel pending render-commit Promise when a newer navigation starts,
preventing hang when the first root.render() commit is dropped by
React re-reconciliation.
- Add regression tests for both edge cases.
…allow scroll ordering Address bonk review on cloudflare#1905: - swap the commit boundary's raw useLayoutEffect for the SSR-safe useNonWarningLayoutEffect pattern from shims/image.tsx so Pages Router SSR renders no longer log React's useLayoutEffect warning - document that the default-noop commit boundary intentionally also wraps SSR and initial hydration - document the intentional shallow-scroll-before-routeChangeComplete ordering and add a unit test pinning it
…lRestoration flag is off Without experimental.scrollRestoration, popstate must still honour the __vinext_scrollX/Y saved into history state on push, since a soft popstate re-renders content the browser's native restoration can't position. Also document why the popstate-time sessionStorage snapshot depends on history.scrollRestoration = "manual".
…lows navigation When beforePopState cancels a popstate the app stays on the previous history entry, but _currentHistoryKey had already been updated to the target key, skewing subsequent scroll-position bookkeeping. Move the key commit below the beforePopState early-return, matching the deliberate deferral of _lastPathnameAndSearch.
…nk increment - Document that the outgoing-entry scroll snapshot is deliberately written before the beforePopState gate: on cancellation the app stays on currentKey, so the saved value remains that entry's live scroll. - Coerce id to a number in the pages-scroll-restoration fixture Link so the href increments numerically instead of string-concatenating.
Gate installManualScrollRestoration on the document not being an App Router one (window.next.appDir === true): with the now-global __NEXT_SCROLL_RESTORATION define, an App Router app that value-imports next/router for compat would otherwise have its native browser scroll restoration disabled at module eval. Upstream only flips history.scrollRestoration inside the Pages Router constructor, which never runs on App Router documents. Also document the hash-only popstate path discarding the per-entry __next_scroll_<key> snapshot as intentional upstream parity (change()'s onlyAHashChange branch passes null to this.set and only scrollToHash runs), and add unit coverage for both behaviors.
Add the missing E2E matrix entry so the ported Next.js regression spec
(reload-scroll-backforward-restoration.spec.ts) actually executes in CI,
matching the shape of the sibling single-shard Pages Router entries.
Also document why the soft-popstate scroll fallback differs between
manual restoration on ({ x: 0, y: 0 }) and off (null -> native auto
restoration handles unstamped entries), per review.
…onstant The popstate handler's sessionStorage save/read branches gated on the module-level constant, which lacked the window.next?.appDir check that installManualScrollRestoration used, leaving those branches relying solely on the __N state filter to stay inert on App Router documents. Fold the appDir check into the constant so every consumer is Pages-Router-only by the same gate; the App Router bootstrap stamps window.next.appDir before any client module evaluates, so the module-eval-time check is reliable.
…ng "0" Route params always arrive as strings, so the numeric 0 fallback was inconsistent with the string values the component relies on (Number(id) + 1 href, id === "error" check). Default to "0" for type consistency.
…with pages-basic Import vinext via the workspace package name instead of reaching into dist by relative path, and declare the same react/react-dom/vinext dependencies as the other fixtures so the fixture is self-describing and the pnpm-lock.yaml importer entry is populated.
Upstream Next.js snapshots the outgoing entry's scroll into
__next_scroll_<key> inside Router.push() itself, before change()'s
onlyAHashChange short-circuit, so hash-only pushes still capture the
departed entry's position. Our hash-only push branch returned before
saveScrollPosition() ran, so a later back-popstate to that entry
restored {x: 0, y: 0} instead of the position the user had reached.
Save the outgoing scroll before updateHistory mints the new key,
matching the full-navigation push path and upstream ordering.
1077acf to
302e29d
Compare
2 participants
Overview
experimental.scrollRestoration, persist per-entry scroll snapshots, and install the Pages Router runtime beforewindow.next.routeris exposed.packages/vinext/src/shims/router.ts,packages/vinext/src/config/next-config.ts,packages/vinext/src/index.ts,tests/e2e/pages-scroll-restoration/reload-scroll-backforward-restoration.spec.ts.Why
When
experimental.scrollRestorationis enabled, the Pages Router owns browser scroll restoration. That requireshistory.scrollRestoration = "manual", a stable key for each history entry, durable scroll snapshots that survive reload, androuteChangeCompleteonly after the new page has committed.experimental.scrollRestorationand definesprocess.env.__NEXT_SCROLL_RESTORATION.sessionStorageentries named__next_scroll_<key>.window.next.router.isReadybefore the popstate runtime exists.next/routerevaluates, beforewindow.next.routeris exposed.routeChangeCompleteshould represent a committed route.What Changed
experimental.scrollRestorationhistory.scrollRestoration = "auto".Maintainer review path
packages/vinext/src/shims/router.ts: runtime ordering, history key tracking, sessionStorage scroll snapshots, and the commit boundary around client renders.packages/vinext/src/config/next-config.tsandpackages/vinext/src/index.ts: config resolution and client define parity with Next.js.tests/e2e/pages-scroll-restoration/reload-scroll-backforward-restoration.spec.tsplustests/fixtures/pages-scroll-restoration: the ported upstream regression test and fixture.tests/shims.test.tsandtests/next-config.test.ts: focused lower-boundary coverage for runtime install ordering and config parsing.Validation
vp checkvp test run tests/next-config.test.ts -t scrollRestorationvp test run tests/shims.test.ts -t "Pages Router concurrent navigation"vp test run tests/pages-router.test.ts -t "client entry statically imports next/router"CI=1 PLAYWRIGHT_PROJECT=pages-scroll-restoration vp run test:e2e tests/e2e/pages-scroll-restoration/reload-scroll-backforward-restoration.spec.tsvp 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.tsThe pre-commit hook also passed entry template snapshots,
vp check --fix, full check, staged tests, and knip.Risk / Compatibility
sessionStorageis usable, matching the Next.js guard.experimental.scrollRestorationkeep browser-managed scroll restoration.next-webdriver, but preserves the same route structure, config flag, navigation sequence, readiness wait,routeChangeCompletewait, and exact scroll assertions.Non-goals
References
experimental.scrollRestoration.routeChangeCompleteobservable used by the test.process.env.__NEXT_SCROLL_RESTORATION.__next_scroll_<key>.routeChangeCompleteas the Pages Router completion signal observed by the fixture.