fix(app-router): supersede restored history traversals#2368
fix(app-router): supersede restored history traversals#2368james-elicx wants to merge 5 commits into
Conversation
|
/bigbonk review for issues |
|
Review: fix(app-router): supersede restored history traversalsI traced the full lifecycle (popstate → The fix is soundThe supersede now has two independent guards working together, which is good defense-in-depth:
Both guards independently neutralize the live response, so the restored tree wins. The existing comment at
|
|
Review posted to PR #2368. SummaryI reviewed PR #2368 ( Verdict: the fix is correct and well-layered. One non-blocking documentation issue. Key findings
Verification: |
commit: |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): supersede restored history traversals
I traced the popstate restore lifecycle (popstate → handlePopstate/navigate → restoreHistoryStateSnapshot → beginNavigation + dispatchSynchronousVisibleCommit) and ran the affected unit suites. The fix is correct. Verification: tests/app-browser-entry.test.ts (206 passed), tests/navigation-planner-rsc-fetch-result.test.ts (16 passed), vp check on both changed source files (clean). The stale-comment that an earlier /bigbonk pass flagged at app-browser-navigation-controller.ts:562-567 is already fixed by the second commit (5b22513), so the controller comment now matches the code.
My one substantive finding is a documentation gap at the new beginNavigation() call site — the comment that explains the supersede mechanism predates this PR's primary change and no longer describes how the supersede actually works.
Why beginNavigation() is the actual fix (and why the existing comment is now stale)
The big comment at app-browser-entry.ts:2082-2100 explains the supersede as relying solely on the visibleCommitVersion bump: the async traverse captures startedVisibleCommitVersion = N, the synchronous restore bumps it to N+1, and when the traverse resolves verifyOperationTokenForCommit rejects it as a staleOperation.
But this PR adds a second, earlier, and now-primary supersede: restoreHistoryStateSnapshot now calls browserNavigationController.beginNavigation() (app-browser-entry.ts:358), bumping activeNavigationId. Tracing the ordering shows this guard fires before the version guard is ever reached:
handlePopstate(event)(:2081) callsnavigate(), which runsbeginNavigation()synchronously (app-browser-navigation-controller.ts-fed path atapp-browser-entry.ts:1491), capturingnavId = X.- For a traverse,
navigate()hits its firstawaitatapp-browser-entry.ts:1539(waitForBrowserRouterStateReady) before it ever callsrenderNavigationPayload/capturesstartedState. So it yields here. - Control returns to the popstate handler;
restoreHistoryStateSnapshot(event.state)runs synchronously, commits the cached snapshot (versionN→N+1), then callsbeginNavigation()again →activeNavigationIdbecomesX+1. - The traverse microtask resumes and checks
isCurrentNavigation(X)atapp-browser-entry.ts:1540→ false → it returns early, before capturingstartedStateor rendering any payload.
So with this fix the in-flight traverse typically bails at step 4 (the navId guard) and never reaches the startedVisibleCommitVersion !== currentState.visibleCommitVersion check that the :2082-2100 comment describes. The version bump is now the backstop (defense-in-depth for a traverse that slipped past the navId check), not the mechanism the comment presents it as.
Request: update the comment at app-browser-entry.ts:2082-2100 so it documents the beginNavigation() supersede as the primary mechanism (the navId bump invalidating the in-flight traversal at its next isCurrentNavigation(navId) boundary), with the visibleCommitVersion bump as the secondary guard. As written, a future reader debugging this path would look only for a version mismatch and be confused why the traverse already returned at :1540. Given the PR title is literally "supersede restored history traversals," the mechanism that does the superseding deserves a comment at its call site (:358).
Other things I checked (all OK)
flushSyncon all threedispatchSynchronousVisibleCommitcallers. HMR (:506), restore (:656), and server-action (:852) all invoke it from async continuations (afterawait createPendingNavigationCommit/await resolveAndClassifyNavigationCommit), not during React render/commit, soflushSyncwon't trip React 19's "flushSync from inside a lifecycle method" warning. The nested case the e2e test exercises (outerflushSyncatapp-browser-entry.ts:343→ inner at controller:580) is also outside render. Note this is a minor behavioral change to the dev-only HMR path (previously a plain batched setter, now a forced flush) — benign for a single tree replacement, but it's worth being aware the old "React flushes the plain setter itself" rationale no longer holds for HMR either.- Scroll-restore interplay. Bumping
activeNavigationIdin the restore means the deferred scroll restore insidecreatePopstateRestoreHandler(gated onisCurrentNavigation(popstateNavId == X),app-browser-popstate.ts:85) is now skipped, and the synchronous restore atapp-browser-entry.ts:2102instead handles it under the new navIdX+1. No double-restore or lost restore; theshouldSkipScrollRestorededup (:2064) becomes partially redundant for this path but stays harmless. - Interception route shape.
app/foo/bar/(..)(..)hoge/page.tsxcorrectly climbs two visible segments (foo,bar) to root and matches/hoge, consistent with the(..)(..)convention inrouting/app-route-graph.ts:2207.
Test quality
The new isolated-fixture spec follows the established *.browser.spec.ts pattern (linkFixtureNodeModules/closeServer/createBuilder().buildApp() → runPrerender → startProdServer), is correctly ported with a source link back to the Next.js v16.2.6 fixture, and its intercept → back → forward → re-intercept sequence directly reproduces the forward-restore race. Assertions use Playwright auto-retrying toHaveText, so the cross-navigation timing is handled without manual waits.
|
Review posted to PR #2368. SummaryI reviewed PR #2368 ( Verdict: the fix is correct. One substantive (non-blocking) documentation gap. Key findings
The earlier Verification: |
Performance benchmarksCompared 2 improved · 0 regressed · 4 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): supersede restored history traversals
I traced the full popstate restore lifecycle and verified the supersede ordering, the flushSync change (including the nested case), and ran the affected unit suites. The fix is correct, and the prior /bigbonk stale-comment findings are now resolved by commits 5b22513 (controller :562-565) and 96591bb (entry :2086-2095). I found no outstanding issues.
Supersede ordering is correct (verified by tracing)
The primary fix — adding beginNavigation() to restoreHistoryStateSnapshot (app-browser-entry.ts:358) — works as intended, and the ordering is sound:
handlePopstate(:2081) →navigate()callsbeginNavigation()synchronously (app-browser-navigation-controller.ts:1491path), capturingnavId = X, then yields at its firstawait waitForBrowserRouterStateReady()(:1539) for a traverse — before capturingstartedStateor rendering.- Control returns to the popstate handler;
restoreHistoryStateSnapshot(event.state)runs synchronously. The visible restore commits inside the outerflushSync(:343) via theapproveVisibleRestorecallback →restoreHistorySnapshotVisibleState→dispatchSynchronousVisibleCommit(:656). Crucially, the restore's own approval at:642-649usesstartedNavigationId: navId (X)and is checked againstactiveNavigationIdbefore the bump, so it is correctly approved. beginNavigation()(:358) then bumpsactiveNavigationId→X+1.- The traverse microtask resumes and
isCurrentNavigation(X)(:1540) is now false → returns early before it can dispatch the fetched payload.
The visibleCommitVersion bump remains as a documented backstop. The updated comment at :2086-2095 now correctly presents the navId supersede as primary and the version bump as the secondary guard.
flushSync change is correct for all three callers + the nested case
dispatchSynchronousVisibleCommit now uses flushSync (:580). Verified all three callers run in async continuations (after an await), not during React render/commit:
- HMR (
:506, afterawait createPendingNavigationCommit) - restore (
:656, reached synchronously from the popstate handler outside render) - server-action (
:852)
I confirmed against React 19.2.7 source (react-dom-profiling.development.js:28150-28168) that flushSync is a plain wrapper that sets transition/priority and runs fn(); the "flushSync was called from inside a lifecycle method" warning fires only in the finally block when ReactDOMSharedInternals.d.f() returns true while React is rendering. The nested case the e2e test exercises (outer flushSync at :343 → inner at controller :580) is benign: work is already flushed when the inner call returns, and neither runs during render. The dev-only HMR path now force-flushes instead of batching — benign for a single tree replacement.
Test quality
The new interception-two-levels.browser.spec.ts follows the established isolated-fixture pattern (linkFixtureNodeModules / createBuilder().buildApp() → runPrerender → startProdServer / closeServer), is correctly ported from the Next.js v16.2.6 fixture with a source link, and its intercept → back → forward → re-intercept sequence faithfully reproduces the forward-restore race. Assertions use Playwright auto-retrying toHaveText.
Verification
tests/app-browser-entry.test.ts— 206 passedtests/navigation-planner-rsc-fetch-result.test.ts— 16 passedvp checkon both changed source files — clean (format + lint + types)
|
Review posted to PR #2368. SummaryI reviewed PR #2368 ( Verdict: the fix is correct, with no outstanding issues. The stale-comment findings from earlier Key findings
Verification
|
Summary
Next.js parity
Fixes the remaining failure from run 28143992598 in:
test/e2e/app-dir/interception-segments-two-levels-above/interception-segments-two-levels-above.test.tsThis is stacked on #2363 because it relies on that PR's restored-history interception proof. #2363 handles planner approval; this PR fixes the later lifecycle race where the network traversal could still win after the snapshot was visibly restored.
Reference: https://github.com/vercel/next.js/blob/v16.2.6/test/e2e/app-dir/interception-segments-two-levels-above/interception-segments-two-levels-above.test.ts
Validation
vp check— passedExcludes cacheComponents, PPR, resume, fallback-shell, and external endpoint failures.