fix(app-router): preserve history metadata for external state updates#1259
Conversation
External history.pushState and history.replaceState currently store caller data as-is. That drops vinext's App Router previousNextUrl marker when apps perform shallow URL updates inside an intercepted route, so later traversal can lose the interception source. The History API wrappers now copy the current App Router metadata onto caller-provided state before delegating to the browser history method. A focused shim regression covers object, null, and undefined caller state against the same contract Next.js preserves.
commit: |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean extraction and straightforward fix. The new app-history-state.ts module is a good separation, the wrapper semantics match the Next.js copyNextJsInternalHistoryState pattern, and the test covers the key state shapes (object, null, undefined). A few minor observations below, none blocking.
| return callerState; | ||
| } | ||
|
|
||
| return createHistoryStateWithPreviousNextUrl(callerState, previousNextUrl); |
There was a problem hiding this comment.
Nit: createHistoryStateWithPreviousNextUrl returns HistoryStateRecord | null, but createExternalHistoryStatePreservingMetadata is typed as returning unknown. When previousNextUrl is non-null and callerState is null, the returned value is { __vinext_previousNextUrl: "/feed" } — which is fine — but on the callerState passthrough path (line 40), the return type is unknown because the caller's original value is returned as-is.
This works correctly at runtime, but the return type could be narrowed. Not blocking.
| vi.resetModules(); | ||
| await import("../packages/vinext/src/shims/navigation.js"); | ||
|
|
||
| win.history.pushState({ myData: { foo: "bar" } }, "", "/photo/1?filter=active"); |
There was a problem hiding this comment.
Consider adding a test case where the current history state does not have __vinext_previousNextUrl (the no-metadata baseline). This would verify the passthrough path in createExternalHistoryStatePreservingMetadata (line 39-41 of app-history-state.ts) — i.e., that caller state is returned unchanged when there's no vinext metadata to preserve.
For example:
win.history.state = { unrelated: true };
win.history.pushState({ myData: 1 }, "", "/photo/2");
expect(win.history.state).toEqual({ myData: 1 }); // no metadata injectedNot blocking — the unit under test (createExternalHistoryStatePreservingMetadata) is simple enough that the passthrough is obvious from reading it.
| vi.resetModules(); | ||
| await import("../packages/vinext/src/shims/navigation.js"); | ||
|
|
||
| win.history.pushState({ myData: { foo: "bar" } }, "", "/photo/1?filter=active"); |
There was a problem hiding this comment.
Also worth considering: a case where the caller's own state object contains __vinext_previousNextUrl (e.g., a library naively serializing full history state). The current implementation correctly overwrites it with the current entry's value via createHistoryStateWithPreviousNextUrl. A test documenting that the internal key wins would make the intent explicit, but it's low priority given how unlikely the collision is.
|
Review submitted. Here's a summary of what I found: Verdict: Approved The PR is clean and well-scoped. It:
I left three non-blocking observations:
|
2 participants
Overview
window.history.pushStateorwindow.history.replaceStatedirectly.previousNextUrlhistory marker onto caller-provided history state before delegating to the browser History API.next/navigationshim.packages/vinext/src/shims/navigation.ts,packages/vinext/src/server/app-history-state.ts,tests/shims.test.tsWhy
App Router history entries need to carry enough internal state for later traversal to reconstruct the same router context. Vinext stores the intercepted-route source URL in
history.state.__vinext_previousNextUrl; if an external shallowpushStateorreplaceStatereplaces that state with caller data, later App Router work can lose the interception context.previousNextUrlis the client-side source for deriving interception context during traversal.null, andundefinedcaller state.pushStateandreplaceState.What Changed
history.pushState({ myData }, '', url)while current history state has__vinext_previousNextUrl{ myData }, dropping the vinext marker.__vinext_previousNextUrl.history.pushState(null, '', url)in the same statenull.history.replaceState(null/undefined, '', url)in the same statenullorundefined.Maintainer review path
packages/vinext/src/server/app-history-state.tsextracts the existing previousNextUrl history helpers and adds the external-copy helper.packages/vinext/src/shims/navigation.tsuses that helper in the patched publicpushStateandreplaceStatepaths.packages/vinext/src/server/app-browser-state.tsre-exports the extracted helpers so existing App Router imports keep the same boundary.tests/shims.test.tscovers the public wrapper behavior with object,null, andundefinedcaller state.Validation
vp test run tests/shims.test.ts -t "preserves App Router history metadata"vp test run tests/shims.test.tsvp test run tests/app-browser-entry.test.ts -t "previousNextUrl"vp test run tests/app-browser-entry.test.tsvp checkknip.Risk / compatibility
Non-goals
References
copyNextJsInternalHistoryStatepushStateandreplaceStatewrappersnull, andundefinedstate inputs.nextUrldimension matters for interception-aware routing and caching.