fix(app-router): restore shallow pathname on history traversal - #2829
fix(app-router): restore shallow pathname on history traversal#2829GtechGovind wants to merge 4 commits into
Conversation
![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: ee5604e238
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 126007f9e4
ℹ️ 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".
| this.#pushHistoryState(historyState, options.href); | ||
| } | ||
| this.commitHistoryTraversalIndex(navigationHistoryIndex); | ||
| this.rememberHistoryStateSnapshot(options.snapshotState); |
There was a problem hiding this comment.
When a shallow entry uses a pathname that is not itself a valid route, storing its tree only in the bounded, invalidatable snapshot cache makes the entry stop working after router.refresh() or cache eviction. For example, push /shallow-test/sub, navigate to /about, refresh there, and then go Back: clearClientNavigationCaches() has removed this snapshot, so popstate falls through to an RSC request for nonexistent /shallow-test/sub instead of restoring the Shallow Routing Test tree. The shallow entry needs a restoration mechanism that survives general navigation-cache invalidation.
Useful? React with 👍 / 👎.
| createExternalHistoryStatePreservingMetadata( | ||
| data, | ||
| window.history.state, | ||
| new URL(url ?? window.location.href, window.location.href).href, |
There was a problem hiding this comment.
When no App Router state is available—normally on a Pages Router page or before hydration—passing shallowUrl prevents createExternalHistoryStatePreservingMetadata() from returning the caller's data unchanged. It instead clones the value into a plain record, so pushState(42, ...), arrays, dates, and other structured-cloneable values no longer round-trip through history.state; non-cloneable values such as functions can also be discarded rather than producing the native DataCloneError. Avoid adding the App Router shallow marker on this fallback path, or preserve arbitrary history data without coercing it to a record.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e4bdabd84
ℹ️ 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".
| // A normal navigation replacing the same traversal entry supersedes any | ||
| // shallow restoration state previously associated with that index. | ||
| this.#durableSnapshots.delete(options.historyIndex); |
There was a problem hiding this comment.
Fresh evidence after the prior cache-invalidation fix: when Back restores a durable shallow entry, BrowserRoot renders that stored tree and its useLayoutEffect calls rememberHistoryStateSnapshot() without durable, so this line immediately demotes the entry into the bounded cache. After navigating Forward, invalidating caches (for example with router.refresh()), and going Back again, the snapshot is gone and the router requests the potentially nonexistent shallow pathname. A shallow entry must retain its durable status when the ordinary render-snapshot effect observes the same history index.
Useful? React with 👍 / 👎.
| if (!browserNavigationController.hasBrowserRouterState()) { | ||
| return false; | ||
| } | ||
| const href = new URL(url ?? window.location.href, window.location.href).href; |
There was a problem hiding this comment.
On a Chromium document loaded with URL userinfo, such as http://user:pass@host/, window.location.href strips the userinfo while document.URL retains it (the existing #2614 hydration regression covers this). Resolving a caller's relative pushState/replaceState URL against location.href and forwarding this credential-free absolute href to the controller makes the native history operation throw SecurityError, whereas passing the caller's relative URL succeeds. Use the absolute URL for the navigation snapshot only, while preserving the original URL for the native history write.
Useful? React with 👍 / 👎.
| if (options.durable === true) { | ||
| this.#snapshots.delete(options.historyIndex); | ||
| this.#durableSnapshots.set(options.historyIndex, options.state); |
There was a problem hiding this comment.
Every hydrated external history.pushState() adds a complete AppRouterState to this map, but maxEntries and clear() apply only to #snapshots; durable entries are never evicted unless the same traversal index is later remembered normally. Repeated shallow pushes therefore retain entire rendered trees for the document lifetime, including entries made unreachable when the user goes Back and pushes a new history branch. Add explicit pruning or bounded ownership for durable snapshots while preserving entries that remain traversable.
AGENTS.md reference: AGENTS.md:L477-L483
Useful? React with 👍 / 👎.
1 participant
Summary
history.pushStateandhistory.replaceStateentriesusePathnameanduseSearchParamswhen popstate takes the same-route fast pathRoot cause
External History API calls updated Vinext's navigation-hook URL cache, but the App Router popstate fast path only restored history metadata and scroll position. It did not resynchronize the hooks when traversing back to the rendered route. The pushed entry also lacked a marker that would let forward traversal reuse the current tree instead of attempting an RSC navigation.
The new internal shallow URL metadata identifies those entries, allowing popstate to synchronize the URL-backed hooks while preserving the current App Router tree.
Fixes #1541.
Validation
./node_modules/.bin/vp check./node_modules/.bin/vp test run tests/shims.test.ts -t 'preserves App Router history metadata when external history calls provide caller state'PLAYWRIGHT_PROJECT=app-router ./node_modules/.bin/playwright test tests/e2e/app-router/advanced.spec.ts -g 'Shallow Routing'(4 passed, 1 existing fixme skipped)