fix(app-router): keep usePathname in sync across back/forward around shallow routing - #2835
fix(app-router): keep usePathname in sync across back/forward around shallow routing#2835sensei-woo wants to merge 2 commits into
Conversation
…shallow routing Two defects conspired to freeze usePathname()/useSearchParams() on a stale value after Back/Forward when the app uses shallow routing (native history.pushState/replaceState), cloudflare#1541: 1. The patched history.pushState/replaceState copied the current entry's __vinext_historyIndex onto the externally written entry. Two distinct entries then shared one restorable-snapshot slot, so a later traversal could restore the OTHER entry's router state. Externally written entries now get their own traversal index, allocated and committed through a new allocateExternalHistoryTraversalIndex runtime function wired to AppBrowserHistoryController. Without the App Router runtime (Pages Router, pre-hydration) the previous copy behavior is unchanged. replaceState also allocates: its URL rewrite makes the snapshot remembered under the old index unrepresentative of the entry. 2. The same-route popstate fast path (committed router tree already matches the target URL) returned before resyncing the navigation shim's cached URL state, which an interleaved shallow pushState had moved elsewhere. It now runs commitClientNavigationState() so the hooks resync from the restored location. The e2e regression test drives the aliasing sequence (push, push, back, push, back, forward) and fails on both defects without the fix; the /shallow-test/sub fixture route now exists as a real route so the traverse fallback RSC navigation resolves instead of 404ing. Fixes cloudflare#1541
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6dfe3a7c5f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| createExternalHistoryStatePreservingMetadata( | ||
| data, | ||
| window.history.state, | ||
| allocateExternalHistoryTraversalIndex(), |
There was a problem hiding this comment.
When an app calls history.pushState or replaceState with a cross-origin URL or an uncloneable state value, the native method throws without changing the browser entry, but this callback has already committed the newly allocated index. The history controller then disagrees with the browser, so a later Back/Forward traversal or router replace can infer direction and resolve snapshots using the wrong current index. Allocate the candidate before the native call if needed, but commit it only after originalPushState/originalReplaceState returns successfully.
Useful? React with 👍 / 👎.
| const index = historyController.allocateNavigationHistoryTraversalIndex("push"); | ||
| historyController.commitHistoryTraversalIndex(index); |
There was a problem hiding this comment.
When an app uses history.replaceState(data, "") without changing the URL, this callback still allocates as a push and assigns the existing entry a fresh traversal index. Its valid router snapshot remains stored under the old index, and because the router tree did not change, no layout effect records a replacement snapshot under the new one; navigating away and then back therefore misses client restoration and falls back to an RSC navigation, potentially resetting client state unnecessarily. Keep the current index when replaceState leaves the effective route unchanged, and allocate a fresh identity only for URL-changing replacements.
Useful? React with 👍 / 👎.
| --- | ||
| "vinext": patch |
There was a problem hiding this comment.
This commit adds a regular named changeset even though this repository generates changesets automatically from Conventional Commit subjects and reserves manually committed changesets for SHA-named retroactive reclassification. Keeping this file can duplicate or conflict with the CI-generated release entry, so delete it and rely on the existing fix(app-router): ... commit subject.
AGENTS.md reference: AGENTS.md:L348-L350
Useful? React with 👍 / 👎.
Address Codex review on cloudflare#2835: - Commit the allocated traversal index only after the native pushState/replaceState succeeds. Allocation is now a pure peek through the runtime seam and a separate commitExternalHistoryTraversalIndex runs after the write, so a throwing call (cross-origin URL, uncloneable state) leaves traversal bookkeeping untouched. - Keep the entry's index on state-only and hash-only replaceState. Only a pathname/search change makes the remembered snapshot unrepresentative; App Router state does not include hashes. - Drop the manually authored changeset; this repo generates changesets from Conventional Commit subjects.
|
Addressed all three Codex findings in b1198f3:
|
1 participant
Fixes #1541.
Problem
usePathname()/useSearchParams()freeze on a stale value after Back/Forward when the app uses shallow routing (nativehistory.pushState/replaceState, per the Next.js SPA guide). Minimal repro and a vinext-vs-next devstep table are in this comment on #1541: the desync needs a pushState performed after a traversal, and then it's the second Back that loses sync — the URL moves while the hooks stay put.Root cause — two defects
1. Externally written entries inherited the current entry's traversal index. The patched
history.pushState/replaceStatebuilt the new entry's state withcreateExternalHistoryStatePreservingMetadata, which copied__vinext_historyIndexfrom the entry being pushed from. Two distinct history entries then shared one slot in the index-keyed restorable-snapshot cache (HistoryStateSnapshotCache), so a later traversal could resolve a "restore" against the other entry's remembered router state — committing a router tree whosenavigationSnapshotcarries the wrong URL, and short-circuiting the full traverse navigation that would have synced fromlocation.2. The same-route popstate fast path skipped the URL-state resync. With distinct indices in place, a second defect surfaced: when a traversal target's pathname+search already matches the committed router tree (
isSameAppRoutePopstateTarget), the popstate handler returned after committing the traversal index — but an interleaved shallow pushState had moved the navigation shim's cached URL state elsewhere, and nothing brought it back. The tree matched; the hooks didn't.Fix
shims/navigation.ts: the patchedpushState/replaceStateallocate a fresh traversal index for the externally written entry through a newallocateExternalHistoryTraversalIndexruntime function (skipped for vinext-internal suppressed writes, and absent-runtime callers — Pages Router, pre-hydration — keep the previous copy behavior).replaceStateallocates too: its URL rewrite makes the snapshot remembered under the old index unrepresentative of the entry it now describes.client/navigation-runtime.ts: the new optional runtime function, registered by the App Router browser entry and wired toAppBrowserHistoryController(allocateNavigationHistoryTraversalIndex("push")+commitHistoryTraversalIndex).server/app-browser-entry.ts: the same-route popstate fast path now runscommitClientNavigationState()sousePathname()/useSearchParams()resync from the restored location.server/app-history-state.ts:createExternalHistoryStatePreservingMetadataaccepts the allocated index;undefinedpreserves the legacy copy.Tests
tests/e2e/app-router/advanced.spec.ts): drives the aliasing sequence — push/shallow-test/sub, push?filter=active, Back, push again, Back, Forward — assertingusePathname/useSearchParamsmatch the URL at every settle point. Fails onmain(frozen pathname at the second Back), passes with the fix. The fixture gains a real/shallow-test/subroute so the traverse-fallback RSC navigation resolves instead of 404ing (that page's pathname was already being shallow-pushed by the existing fixture buttons).tests/app-browser-history-controller.test.ts): allocated index is stamped (including onto metadata-less entries), legacy copy preserved without allocation, and the aliasing scenario at the controller level — an externally pushed entry gets its own snapshot slot, so restoring the original entry yields the original state.pnpm test,PLAYWRIGHT_PROJECT=app-router pnpm run test:e2e -- tests/e2e/app-router/advanced.spec.ts(38/38), andpnpm run checkare green.Found while root-causing a glyph-soup animation bug in a production app whose account tabs shallow-route with native pushState; the frozen
usePathnamecompounds with cacheComponents' hidden-Activity keep-alive (stale state keeps morphing offscreen). Happy to iterate on review feedback.https://claude.ai/code/session_01Y4w3KwfeSpsDQWFeLnZMXd