fix(app-router): restore scroll snapshots on back#1743
Conversation
commit: |
NathanDrake2406
force-pushed
the
nathan/nav-scroll-restoration
branch
2 times, most recently
from
b95701f to
1b406b4
Compare
|
/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: scroll snapshot restoration on back
Reviewed the diff against Next.js v16.2.6 source/tests. The core approach is sound and matches Next.js behavior in two important ways:
- Removing
history.scrollRestoration = "manual"is correct parity. Confirmed againstpackages/next/src/client/components/app-router.tsxat v16.2.6 — Next's App Router never setsscrollRestoration. It relies on browser-default restoration plus its ownfocusAndScrollRefmechanism. Good catch on the divergence. - Synchronous snapshot restore on
popstatebefore the async RSC traverse settles is a reasonable way to bridge the gap between vinext's router-state representation and Next's__PRIVATE_NEXTJS_INTERNALS_TREEtraversal. The bounded (50-entry), document-local snapshot cache keyed by traversal index is appropriate and won't leak across documents.
The positional-arg threading through navigate(...) in app-browser-popstate.ts matches the NavigationRuntimeNavigate signature, and the restoreVisibleState controller method correctly bumps visibleCommitVersion instead of rewinding it.
The main concerns are around scope creep and test coverage — this PR is titled/scoped as a scroll fix but also rewrites the server-action handler with a new cache-invalidation guard and threads a reuseCurrentBfcacheIds flag through the entire visible-commit merge path. Those are significant behavioral changes that deserve their own focused unit tests (per AGENTS.md guidance on testing extracted runtime helpers). Inline notes below.
No blocking correctness bugs found, but I'd want unit tests for the new guard/reuse logic before merge given how central these code paths are.
| { temporaryReferences }, | ||
| ); | ||
| if ( | ||
| revalidation === "none" && |
There was a problem hiding this comment.
Behavioral change beyond the PR title. The server-action handler was rewritten so cache invalidation now happens eagerly before createFromFetch when revalidation !== "none", and shouldClearClientNavigationCachesForServerActionResult(...) is only consulted when revalidation === "none".
The final union of "when do we clear caches" looks equivalent to the old code (since shouldClear returns true for revalidation !== "none" anyway), but the timing now differs: caches are cleared before the result is decoded. Combined with the new pendingServerActionCacheInvalidationGuards, this is a non-trivial change to server-action semantics that isn't really about scroll restoration.
This would be much easier to review (and safer to land) as a separate stacked PR, per the repo's "small stacked PRs" guidance. At minimum, please add a focused unit test asserting the clear-timing for both the revalidation !== "none" and return-value-only paths.
| pendingServerActionCacheInvalidationGuards - 1, | ||
| ); | ||
| }; | ||
| } |
There was a problem hiding this comment.
The guard counter (beginServerActionCacheInvalidationGuard / isClientNavigationCacheInvalidationGuarded) is module-level mutable state with no direct unit test. It's consumed in three places (readCurrentBfcacheVersionHistoryIds, restoreHistoryStateSnapshot, and the reuseCurrentBfcacheIds computation in bootstrapHydration). Per AGENTS.md ("add a focused unit test for the new helper module"), please add a small test covering nesting (multiple concurrent guards) and double-release idempotency — the released flag and Math.max(0, ...) clamp both deserve coverage.
| }); | ||
| commitClientNavigationState(); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
rememberHistoryStateSnapshot / restoreHistoryStateSnapshot carry the bulk of this PR's new logic (index keying, bfcache-version gating, LRU eviction at MAX_HISTORY_STATE_SNAPSHOTS, guard gating) but are only exercised via the new Playwright spec. A unit test for these (snapshot hit/miss, stale-version eviction, eviction at the 50-entry cap) would catch regressions far faster than the E2E and aligns with the repo guidance to unit-test extracted runtime helpers rather than relying on generated/E2E assertions.
| preserveAbsentSlots: action.reuseCurrentBfcacheIds && commit.decision.preserveAbsentSlots, | ||
| preserveElementIds, | ||
| preservePreviousSlotIds, | ||
| }); |
There was a problem hiding this comment.
The reuseCurrentBfcacheIds === false branch flips several merge behaviors at once: forces clearAbsentSlots, drops preserveAbsentSlots, empties preserveElementIds/preservePreviousSlotIds, and resets the previous bfcache map to {}. This is the right idea for a fresh traverse, but it's a meaningful change to the merge reducer with no dedicated unit test in tests/app-browser-entry.test.ts. Consider adding a reducer-level test asserting the merged output for both reuseCurrentBfcacheIds: true and false, since this path affects all navigate/traverse commits, not just back-with-stale-bfcache.
| return; | ||
| } | ||
| handlePopstate(event); | ||
| if (restoreHistoryStateSnapshot(event.state)) { |
There was a problem hiding this comment.
Minor: when isSameAppRoutePopstateTarget(href) is true (hash-only traversal) the handler returns early at the block above and never calls restoreHistoryStateSnapshot, so a snapshot for that index can persist until evicted by the 50-entry cap. That's harmless given the cap, but worth a one-line comment noting the hash-only path intentionally skips snapshot restore so future readers don't think it's an oversight.
| await expect(page.locator("#back-button")).toBeVisible(); | ||
| await page.locator("#back-button").click(); | ||
|
|
||
| const newScrollPosition = await page.evaluate(() => window.pageYOffset); |
There was a problem hiding this comment.
window.pageYOffset is read immediately after the #back-button click without first awaiting any post-back content assertion. This mirrors the upstream Next test (which also reads immediately), and the fix is synchronous, so it should be deterministic. But if this turns out flaky in CI, add an await expect(body).toContainText("Item 200") (or wait for the page URL) before reading the offset to ensure the back traverse has visibly committed.
|
Review submitted successfully. Here's a summary of my review of PR #1743: Review summaryVerdict: Approach is sound, no blocking correctness bugs, but flagged scope creep and missing unit tests. What's good (verified against Next.js v16.2.6)
Concerns raised (6 inline comments)
The key theme: solid fix verified against upstream, but the PR bundles two significant behavioral subsystems (server-action guard + bfcache-reuse merge flag) under a scroll-restoration title, and those need dedicated unit tests before merge. |
NathanDrake2406
force-pushed
the
nathan/nav-scroll-restoration
branch
from
1b406b4 to
dc0b18e
Compare
NathanDrake2406
force-pushed
the
nathan/nav-scroll-restoration
branch
from
d1623d5 to
de9c72f
Compare
|
Addressed the latest review on the rebased head
Also rebased the branch onto current Local verification:
|
NathanDrake2406
force-pushed
the
nathan/nav-scroll-restoration
branch
4 times, most recently
from
1bc7771 to
3effaf8
Compare
|
Clarifying the scope question from the latest review thread: the server-action/cache-invalidation pieces are intentional, not incidental cleanup. Once this PR adds a synchronous visible-state snapshot restore for popstate, that snapshot has to participate in the same invalidation boundary as history BFCache ids. Otherwise the scroll fix can resurrect stale client state after a server action revalidates or returns a replacement tree. Concretely:
So this does expand beyond the original scroll timing symptom, but it is the correctness envelope for making synchronous snapshot restore safe. The nonessential wrapper cleanup was removed; what remains is tied to the invalidation contract. |
NathanDrake2406
force-pushed
the
nathan/nav-scroll-restoration
branch
from
3effaf8 to
fbcd910
Compare
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
10 mins cap broke it at the end |
It died 2 minutes into its run and did nothing for the following 8 mins. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: synchronous scroll snapshot restore on back
I re-reviewed the rebased head against the App Router state/commit machinery. The core fix holds up and the new test coverage (reducer-level reuseCurrentBfcacheIds: false, the HistoryStateSnapshotCache/RestorableClientStateController unit tests, the synchronous popstate scroll guard, and the polled E2E) addresses the prior round's concerns well. Removing history.scrollRestoration = "manual" matches Next v16.2.6, and the bfcache-epoch coupling of the snapshot cache + in-flight server-action guard is a coherent correctness envelope, as described in the thread.
No blocking correctness bugs. My main remaining ask is documentation of one subtle, load-bearing invariant plus a couple of minor scope/clarity notes. Details inline.
Key invariant that should be commented
The whole fix depends on the synchronous snapshot commit superseding the in-flight async RSC traverse for the same navId. Concretely:
popstatecallsnavigate(...)first;renderNavigationPayloadsynchronously capturesstartedState = getBrowserRouterState()(version N) and thenawaitsnextElements, yielding at least one microtask.restoreHistoryStateSnapshot(event.state)then runs synchronously (no await) in the same task, commits the cached snapshot, and bumpsvisibleCommitVersionto N+1.- When the async traverse resolves,
resolvePendingNavigationCommitDispositionDecisionseesoperation.startedVisibleCommitVersion (N) !== currentState.visibleCommitVersion (N+1)and returnsstaleOperation->no-commit(app-browser-state.ts:512-522).
So the fresh server RSC payload is intentionally discarded on every snapshot-restored back/forward, and the user keeps the cached client snapshot (matching Next's in-memory bfcache, which also does not refetch). This ordering is deterministic only because restoreHistoryStateSnapshot is synchronous while the async traverse always yields. That is a fragile, non-obvious contract carrying the whole feature and currently has no comment at the call site. Please document it where restoreHistoryStateSnapshot is invoked (and ideally note the staleOperation interaction).
| return; | ||
| } | ||
| handlePopstate(event); | ||
| if (restoreHistoryStateSnapshot(event.state)) { |
There was a problem hiding this comment.
This is the load-bearing seam of the fix and deserves a comment. The synchronous restoreHistoryStateSnapshot commit bumps visibleCommitVersion, which causes the in-flight async RSC traverse kicked off by handlePopstate (same navId) to be skipped as a staleOperation in resolvePendingNavigationCommitDispositionDecision (app-browser-state.ts:512). Net effect: the fresh server RSC payload is intentionally dropped and the cached client snapshot wins (bfcache-style, gated by bfcacheVersion).
This only works because restoreHistoryStateSnapshot runs synchronously in the same task while the async traverse always yields a microtask at await nextElements. That ordering invariant is invisible to future readers — a comment here (and a note that the snapshot path supersedes the in-flight traverse) would prevent someone from accidentally making this async or reordering it.
Separately: the hash-only early-return branch above never sets synchronousPopstateScrollRestoreNavigationId and skips both the snapshot restore and the consumed-scroll guard. That's correct (hash-only traversals need no tree snapshot), but a one-line note on that branch would save a future reader from thinking the snapshot path was forgotten there.
| "sass", | ||
|
|
||
| // Vite+ reporter name used outside CI in vite.config.ts. | ||
| "agent", |
There was a problem hiding this comment.
Scope: the sass and agent knip-ignore entries (and the navigation-trace.ts export type -> type change) are unrelated to scroll restoration. They look like incidental vp check fixes. Not blocking, but per the repo's small-stacked-PR guidance these would be cleaner as a separate chore commit/PR so the scroll fix stays focused.
| window.scrollTo(x, y); | ||
| if (!shouldContinue() || Math.abs(window.scrollY - y) <= 1 || attempts >= 60) { | ||
| const reachedTarget = Math.abs(window.scrollY - y) <= 1; | ||
| if (!shouldContinue() || (reachedTarget && attempts >= minFrames) || attempts >= 60) { |
There was a problem hiding this comment.
Worth a brief comment on why minFrames exists: after a synchronous snapshot restore the document may still be short (the layout's client state hasn't grown the page back yet), so scrollTo(0, savedY) can momentarily satisfy reachedTarget at a clamped offset. minFrames: 1 forces at least one extra rAF so the retry survives until the restored content expands the scroll height. Without that context this condition reads like an arbitrary off-by-one.
|
Review submitted successfully. Here's a summary of my review of PR #1743: Review summaryVerdict: The fix is correct and well-tested. No blocking correctness bugs. Main ask is documenting one subtle invariant, plus minor scope/clarity notes. What I verified
Concerns raised (3 inline comments)
I also folded a note about the hash-only popstate branch (correctly skipping the snapshot path) into the main inline comment. |
:| the log is now gone I saw it was about to post the review then got cut off at 10m, weird |
https://github.com/cloudflare/vinext/actions/runs/27127062967 |
|
/bigbonk review for issues |
That one died after 5 mins |
There was a problem hiding this comment.
Review: synchronous scroll snapshot restore on back
Re-reviewed the current head. The fix is sound and the prior review rounds have been addressed well:
history.scrollRestoration = "manual"removal matches Next.js v16.2.6 App Router behavior (browser-default restoration +focusAndScrollRef).- The load-bearing staleOperation invariant is now documented at the popstate call site (
app-browser-entry.ts:388-407) and atresolvePendingNavigationCommitDispositionDecision(app-browser-state.ts:514-519). This was the main ask from the last round and it's now clear. - Test coverage is solid: reducer-level
reuseCurrentBfcacheIds: falsecases,HistoryStateSnapshotCache/RestorableClientStateControllerunit tests, the synchronous popstate scroll guard unit test, and the polled E2E (no longer readspageYOffsetimmediately after the back click). - The bfcache-epoch coupling (snapshot cache + in-flight server-action guard +
reuseCurrentBfcacheIdsmerge flag) is a coherent correctness envelope, and the thread justifies why it can't be split out.
No blocking correctness bugs. A few smaller observations inline — none are merge blockers.
| historyIndex: currentHistoryTraversalIndex, | ||
| state: treeState, | ||
| }); | ||
| }, [treeState]); |
There was a problem hiding this comment.
This effect keys the snapshot by the module-level currentHistoryTraversalIndex but only depends on [treeState]. The ordering works today because commitHistoryTraversalIndex(...) runs inside the navigation commit effect before commitClientNavigationState(navId) triggers setTreeStateValue, so the index is already updated when this layout effect fires for the new treeState. That's correct, but it's an implicit cross-module ordering contract that's invisible here. A one-line comment noting that currentHistoryTraversalIndex is committed before the treeState update would prevent a future change to the commit ordering from silently mis-keying snapshots (which would cause resolveRestore to read the wrong index on back).
| getActiveNavigationId: () => browserNavigationController.getActiveNavigationId(), | ||
| isCurrentNavigation: (navId) => browserNavigationController.isCurrentNavigation(navId), | ||
| markScrollRestoreConsumed: (navId) => { | ||
| synchronousPopstateScrollRestoreNavigationId = navId; |
There was a problem hiding this comment.
synchronousPopstateScrollRestoreNavigationId is written here but never reset. It works as a single-slot marker because activeNavigationId is strictly monotonic (beginNavigation only ever increments), so shouldSkipScrollRestore(navId) can only match the most-recent synchronously-restored navigation. That's fine, but worth a brief comment that this is intentionally a latch on the latest navId rather than a per-navigation set — otherwise a future reader may think the missing reset is a leak.
| (!restorableClientState.isCacheInvalidationGuarded() && | ||
| restorableClientState.isCurrentBfcacheVersion( | ||
| activeTraversalIntent?.historyState ?? window.history.state, | ||
| )); |
There was a problem hiding this comment.
reuseCurrentBfcacheIds is computed once at navigation start and captured in the navigateRsc closure, then used for all three render paths (cached/optimistic/fresh). The async traverse can resolve much later; if the bfcache epoch changes or a server-action guard is released in between, this captured value is stale. This mirrors restoredBfcacheIds above (same window, same staleness), so it's consistent with existing behavior and not a regression — but since the prior review round explicitly worried about epoch/guard timing, a short note that both restoredBfcacheIds and reuseCurrentBfcacheIds are intentionally snapshotted at navigation-start (not re-read at commit) would close that loop for reviewers.
| "sass", | ||
|
|
||
| // Vite+ reporter name used outside CI in vite.config.ts. | ||
| "agent", |
There was a problem hiding this comment.
Scope: the sass and agent knip-ignore entries (and the navigation-trace.ts export type -> type change) are unrelated vp check fixups. Not blocking, but per the repo's small-stacked-PR guidance these would read cleaner as a separate chore commit so the scroll fix stays focused. (Already flagged in a prior round; restating only because it's still present.)
|
Review submitted successfully. Here's a summary of my review of PR #1743: Review summaryVerdict: The fix is sound, prior review rounds have been addressed, and there are no blocking correctness bugs. I raised 4 small non-blocking observations. What I verified against the code
Inline comments (all non-blocking)
|
ah yeah you're right. that's even worse tho, hope cloudflare doesn't get billed for these failed runs |
Adds comments for three load-bearing invariants identified in review: - rememberHistoryStateSnapshot layout effect ordering - synchronousPopstateScrollRestoreNavigationId single-slot latch - restoredBfcacheIds/reuseCurrentBfcacheIds snapshot timing
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: synchronous scroll snapshot restore on back
Re-reviewed the current head (562fd40). This PR has been through several thorough rounds and the prior asks are now addressed — I verified the fix end-to-end rather than re-litigating settled points.
What I verified against the code
history.scrollRestoration = "manual"removal is correct parity — Next v16.2.6 App Router never sets it; relies on browser-default restoration +focusAndScrollRef.- The load-bearing staleOperation invariant is now documented at the popstate call site (
app-browser-entry.ts:407-426) and atresolvePendingNavigationCommitDispositionDecision(app-browser-state.ts:514-519). This was the main ask last round and it is clearly captured now. - The commit-ordering contract for the snapshot-remember effect checks out:
commitHistoryTraversalIndex(navId)runs beforecommitClientNavigationState(navId)(app-browser-entry.ts:697/702/711then:717), which triggerssetTreeStateValueand the[treeState]layout effect. SocurrentHistoryTraversalIndexis already current when the snapshot is keyed. The comment at:1183documents this. beforeCommitonly runs after approval succeeds (app-browser-navigation-controller.ts:600-605), sostageClientParams(...)is never applied for a rejected restore — no leaked staged params.- The popstate
navigate(...)positional call is correct: the 6th argfalsemaps toprogrammaticTransition(already the default), 7th totraversalIntent, matchingNavigationRuntimeNavigate.reuseCurrentBfcacheIdsis computed internally from the traversal intent, not passed positionally. RestorableClientStateController/HistoryStateSnapshotCacheare now unit-tested (LRU eviction, guard nesting/double-release, stale-bfcache eviction, fresh-document epoch), plus reducer-levelreuseCurrentBfcacheIds: falsecoverage and a polled E2E. Ranvp test run tests/app-browser-entry.test.tslocally — 180 tests pass.
No blocking correctness bugs. Two small non-blocking notes inline.
| "traverse", | ||
| undefined, | ||
| undefined, | ||
| false, |
There was a problem hiding this comment.
Minor clarity: the 6th positional arg false here is programmaticTransition (already its default), not reuseCurrentBfcacheIds. reuseCurrentBfcacheIds for the async traverse is computed internally in navigateRsc from the traversal intent (app-browser-entry.ts:1826). Passing the default explicitly via positionals is easy to misread as forcing fresh bfcache ids on the async path. A one-line comment, or using a small options-object overload, would prevent a future reader from misattributing this arg. Not blocking — the call is correct as-is.
| "sass", | ||
|
|
||
| // Vite+ reporter name used outside CI in vite.config.ts. | ||
| "agent", |
There was a problem hiding this comment.
Scope (already flagged in prior rounds, restating only because it's still here): the sass/agent knip-ignore entries and the navigation-trace.ts export type -> type change are unrelated vp check fixups. Per the repo's small-stacked-PR guidance these read cleaner as a separate chore commit so the scroll fix stays focused. Non-blocking.
|
Review submitted successfully. Here's a summary of my review of PR #1743: Review summaryVerdict: The fix is sound, prior review rounds are fully addressed, and there are no blocking correctness bugs. I verified the fix end-to-end against the code rather than re-litigating settled points. What I verified
Inline notes (both non-blocking)
|
2 participants
Summary
navigation.test.tsinto the app-router Playwright suite.history.scrollRestoration; Next's App Router does not do that by default.popstatebefore the async RSC traverse finishes.Root Cause
The failing upstream case navigates from
/scroll-restorationto/scroll-restoration/other, callsrouter.back(), and immediately readswindow.pageYOffset. Vinext already persisted scroll coordinates in history state, but the App Router back path waited for an async RSC traverse before the source route was visible again. At the immediate assertion point the document was still rendering the short/otherpage, so the browser reported0instead of the original scroll offset.Next.js stores App Router tree state in the browser history entry and restores from that state during traversal. Vinext has a different router-state representation, so this PR keeps a bounded document-local snapshot cache keyed by the existing traversal index and restores the visible snapshot synchronously while still letting the normal RSC traverse reconcile afterward.
Next.js References
popstatehandling: https://github.com/vercel/next.js/blob/v16.2.6/packages/next/src/client/components/app-router.tsx#L350-L371Verification
Red before fix:
PLAYWRIGHT_PROJECT=app-router pnpm run test:e2e tests/e2e/app-router/scroll-restoration.spec.tsfailed with expected saved scroll offset and received0.Green after fix:
vp test run tests/app-browser-entry.test.tsvp run vinext#buildPLAYWRIGHT_PROJECT=app-router pnpm run test:e2e tests/e2e/app-router/scroll-restoration.spec.ts tests/e2e/app-router/nextjs-compat/hash-popstate-scroll.spec.ts tests/e2e/app-router/nextjs-compat/router-autoscroll.spec.tsPLAYWRIGHT_PROJECT=app-router pnpm run test:e2e tests/e2e/app-router/scroll-restoration.spec.tsvp checkCloses #1367