feat(app-router): support useRouter bfcacheId semantics#1588
Conversation
commit: |
|
@codex review |
![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: e66ec5c91c
ℹ️ 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".
| const segmentCount = getVisibleTreePathSegments(parsed.treePath).length; | ||
| return `${id}@${getPathPrefix(pathname, segmentCount)}`; |
There was a problem hiding this comment.
createBfcacheSegmentIdentity() derives layout/slot/template identity from a fixed path-prefix length (getVisibleTreePathSegments(...).length), which treats each dynamic segment as consuming exactly one URL segment. For catch-all routes like [...slug]/[[...slug]], the segment value spans the remaining path, so navigating /docs/a/b → /docs/a/c keeps the same computed prefix (/docs/a) and incorrectly reuses the previous bfcache id. That preserves client state for a segment whose params actually changed, so useRouter().bfcacheId semantics diverge for catch-all layouts/templates/slots.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I’ve updated it accordingly.
|
Important PR bro, I think it’ll get merged when maintainer is online. That said, there are a few improvements to be made. Blocking1.
|
|
Blocking issue:
That loses a persisted bfcache map when hydrating after a hard reload of a non-initial session-history entry. Failure shape:
I think initial browser state should seed from Test I’d add before merge:
I’d also port Next’s original browser-back form-state assertion, not just the restored-id assertion. The id text proves the map changed; the form-state assertion proves the user-visible semantics are correct. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61bf1e9d29
ℹ️ 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".
| const restoredBfcacheIds = | ||
| navigationKind === "traverse" ? readHistoryStateBfcacheIds(window.history.state) : null; |
There was a problem hiding this comment.
When a traverse navigation follows one or more server redirects, this captures restoredBfcacheIds once from the original history entry and reuses it for the final redirected payload. Because createNextBfcacheIdMap prioritizes restored IDs over current/minted IDs, a back/forward navigation that redirects can incorrectly apply stale segment IDs from the source entry to the redirected target, leading to wrong state preservation/reset behavior in router.bfcacheId-keyed components. The restored map should be recomputed (or cleared) when currentHref changes due to redirect hops.
Useful? React with 👍 / 👎.
|
I think slots/intercepted routes still have an identity bug.
Can we make slot identity include the active slot route/page identity, and add an E2E for fresh intercepted-slot navigation resetting state, while browser back restores it? |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Implement useRouter bfcacheId semantics
Solid implementation overall. The architecture is well-layered: history persistence (app-history-state), id lifecycle (app-browser-state), browser integration (app-browser-entry), and rendering (slot + navigation shims) are cleanly separated. The version-gated restoration mechanism for hard reload safety, consistent redirect clearing across all three redirect paths, and the preserveBfcacheIdsForMergedElements post-merge reconciliation are all well-designed.
The E2E and unit test coverage is thorough — 12 unit tests for the id map algorithm, updated integration tests, and 10 Playwright E2E tests covering traversal, redirect, hard reload, hash-only, intercepted routes, server actions, and programmatic push/replace.
I have a few inline observations and one thing I'd like resolved before merge. The rest are non-blocking notes.
Blocking
formatPublicBfcacheId has an unreachable branch. The INITIAL_BFCACHE_ID is "0" (truthy), but formatPublicBfcacheId uses !value as the first check and then value === _appRouter.bfcacheId as a separate condition. The "0" sentinel is truthy, so the !value check won't catch it — but value === _appRouter.bfcacheId will. However, the intent would be clearer (and less fragile if the internal sentinel ever changes) with an explicit check. See the inline comment.
Non-blocking observations
-
useRouter()referential stability change. This PR changesuseRouter()from returning a stable singleton to auseMemo-ed spread. Components in user code that depend onObject.is(router1, router2)across renders will break. This is the correct behavior for Next.js 15.3+ parity, and the docstring is updated accordingly, but it's worth calling out in release notes. -
cloneHistoryStateis a shallow clone. The bfcache ids value read viareadHistoryStateBfcacheIds()goes throughcloneHistoryStatewhich doesObject.entries()at the top level only. The inner__vinext_bfcacheIdsobject is a direct reference from the original state. SincereadHistoryStateBfcacheIdsbuilds a freshidsobject from the entries, this is safe — just noting it for future maintainers. -
Module-level
nextBfcacheIdcounter. The counter is global to the browser entry module, which is correct since bfcache ids are only used in a single Vite environment. TheMath.maxinrememberBfcacheIdensures monotonic advancement even under concurrent navigation starts. -
isBfcacheSegmentIdcallsparseElementKeyon every invocation. This runs for every key incollectBfcacheSegmentIdsand for every key duringreadHistoryStateBfcacheIdsvalidation. For typical route trees (5-20 segments) this is negligible, but it's worth noting as a potential optimization target if route trees grow large.
| }; | ||
|
|
||
| function formatPublicBfcacheId(value: string | null | undefined): string { | ||
| if (!value || value === _appRouter.bfcacheId) return "_b_0_"; |
There was a problem hiding this comment.
The logic here is subtly correct but easy to misread.
!value catches null, undefined, and "" (all falsy). value === _appRouter.bfcacheId catches "0" (the internal sentinel, which is truthy). The two conditions handle different cases.
However, if INITIAL_BFCACHE_ID ever changes from "0" to something else, or _appRouter.bfcacheId drifts, this breaks silently. I'd prefer making the sentinel comparison explicit:
| if (!value || value === _appRouter.bfcacheId) return "_b_0_"; | |
| if (!value || value === "0") return "_b_0_"; |
This makes it obvious that the internal "0" sentinel is mapped to the public "_b_0_" format, and doesn't depend on _appRouter.bfcacheId remaining "0". Alternatively, import INITIAL_BFCACHE_ID from app-browser-state and compare against that.
| | DispatchPendingNavigationCommitDispositionDecision | ||
| | NonDispatchPendingNavigationCommitDispositionDecision; | ||
|
|
||
| let nextBfcacheId = 0; |
There was a problem hiding this comment.
Module-level mutable state — the nextBfcacheId counter resets to 0 on every full page load. That's fine for the browser environment, but worth a brief comment noting that this is intentionally scoped to a single browser document lifetime and resets are safe because the version-gating mechanism in app-browser-entry.ts prevents cross-document id collisions.
| const currentValue = currentIdentity === nextIdentity ? options.current[id] : undefined; | ||
| // History traversals restore persisted ids first, matching segments keep | ||
| // their current id, and newly-created segments mint a fresh opaque id. | ||
| const value = options.restored?.[id] ?? currentValue ?? mintBfcacheId(); |
There was a problem hiding this comment.
The priority chain restored > currentValue > mint is the core invariant and it's correct. One thing I want to confirm: when restored contains an id for a segment whose identity has changed (e.g., [group] layout at /x now at /y), the restored id still wins because options.restored?.[id] is checked before currentValue. Is this intentional for traversals?
I believe the answer is yes — traversal should restore the exact ids from the target history entry regardless of identity matching, because those ids were correct when the entry was created. The identity-matching logic only applies to currentValue (the "carry forward" case). If that's the intent, a brief comment would help future readers.
| } | ||
|
|
||
| function BfcacheSlotBoundary({ content, id }: { content: React.ReactNode; id: string }) { | ||
| const SegmentContext = BfcacheSegmentIdContext!; |
There was a problem hiding this comment.
Non-null assertion is safe here because BfcacheSlotBoundary is only rendered when BfcacheSegmentIdContext is truthy (line 230). But if someone moves this component or calls it independently, it'll crash. Consider a defensive guard:
| const SegmentContext = BfcacheSegmentIdContext!; | |
| const SegmentContext = BfcacheSegmentIdContext; | |
| if (!SegmentContext) return <>{content}</>; | |
| return <SegmentContext.Provider value={id}>{content}</SegmentContext.Provider>; |
| readHistoryStateTraversalIndex(window.history.state) ?? 0; | ||
| let nextHistoryTraversalIndex: number = currentHistoryTraversalIndex; | ||
|
|
||
| function isCurrentBfcacheVersion(state: unknown): boolean { |
There was a problem hiding this comment.
When readHistoryStateBfcacheVersion(state) returns null (no version in history state) and currentBfcacheVersion is 0, this returns true. That means on a completely fresh page load (no prior bfcache version in history), any history entry with bfcache ids but no version field would be treated as current-version.
Is that the intended behavior? I think it is — the only way to have ids without a version is if they were written before version-gating was introduced, and treating them as version 0 (the fresh-load default) is reasonable. Just want to confirm this edge case was considered.
| } | ||
| } | ||
|
|
||
| if (metadata.bfcacheIds !== undefined) { |
There was a problem hiding this comment.
When bfcacheIds is non-null but empty ({}), both the ids key and the version key are deleted. This is clean — empty maps are equivalent to no bfcache state. But note that bfcacheVersion handling on lines 75-81 runs independently, so if someone passes bfcacheIds: {} with bfcacheVersion: 5, the version is first deleted (line 69) then re-written (line 77). The final state is correct (version is present but ids are absent, then version is written back), but the interaction is a bit surprising.
Considering the empty-ids branch already deletes the version key, the version block at line 75 should probably be guarded to skip when ids were explicitly cleared. Not a bug, just a code clarity nit.
| } | ||
| } | ||
|
|
||
| function createActiveSlotIdentity(id: string, metadata: AppElementsMetadata | null): string | null { |
There was a problem hiding this comment.
Good: createActiveSlotIdentity addresses the intercepted-route identity concern from the earlier review by checking activeRouteId from slot bindings first, falling back to the interception metadata. This means /photo/42 → /photo/43 intercepted into the same @modal slot correctly produces different identities.
One edge case: if activeSlotBinding?.activeRouteId is an empty string (falsy), it falls through to the interception check. Empty string seems unlikely from encodeRouteId, but a strict !== null && !== undefined check would be more robust than truthiness.
| @@ -0,0 +1,256 @@ | |||
| /** | |||
There was a problem hiding this comment.
Excellent test coverage. The attribution comment linking to the original Next.js test file is appreciated per the AGENTS.md guidelines.
One gap I noticed: there's no test for "hard reload on a non-initial history entry preserves the reloaded entry's fresh ids while ensuring back navigation gets fresh (not stale pre-reload) ids." The "uses the hydration bfcacheId after hard reload" test (line 43) covers this partially — it reloads on /x/2, verifies it gets _b_0_, then goes back to /x/1 and verifies a fresh minted id — but it doesn't verify that the reloaded /x/2 entry's id survives if you navigate forward to it again.
This was flagged in the PR comments by NathanDrake2406. The version-bumping mechanism should handle it correctly, but an explicit test would close the loop.
| const [treeStateValue, setTreeStateValue] = useState<AppRouterState | Promise<AppRouterState>>( | ||
| () => ({ | ||
| activeOperation: null, | ||
| bfcacheIds: createInitialBfcacheIdMap(resolvedElements), |
There was a problem hiding this comment.
Good change: switching from a direct object to a lazy initializer () => ({...}) for useState. This avoids re-creating the initial state object on every render (only matters if BrowserRoot re-renders before mounting, but it's a correct React pattern).
| const mergedElements = mergeElements(state.elements, action.elements, { | ||
| clearAbsentSlots: action.type === "traverse", | ||
| preserveAbsentSlots: commit.decision.preserveAbsentSlots, | ||
| preserveElementIds: commit.decision.preserveElementIds, |
There was a problem hiding this comment.
Clean separation: compute mergedElements first, then reconcile bfcache ids against the merged element set. This correctly handles the case where element merging preserves segments from the previous state that aren't in the new action's bfcache id map.
|
Review posted on PR #1588 with 10 inline comments. Here's a summary of the key findings: Architecture: Well-layered and clean. History persistence, id lifecycle, browser integration, and rendering are properly separated across One item to resolve before merge:
Non-blocking observations:
|
manNomi
force-pushed
the
fix/use-router-bfcache-id-semantics-v2
branch
from
9f8d07e to
0c373bf
Compare
|
@james-elicx Would it be okay to trigger another /bigbonk review ? |
|
/bigbonk review |
There was a problem hiding this comment.
Review: useRouter bfcacheId semantics (v2)
This is a well-structured implementation. The layering is clean — history persistence (app-history-state), id lifecycle (app-browser-state), browser integration (app-browser-entry), rendering (slot + navigation shims) — and the separation of concerns makes each piece independently testable. The test coverage is strong: 12 unit tests for the id map algorithm, integration tests for reducer-level preservation, and 10 E2E Playwright tests.
The version-gating mechanism for hard reload safety is sound. The redirect-clearing across all three redirect paths (cached, streamed, server-action) is consistent. The preserveBfcacheIdsForMergedElements post-merge reconciliation correctly bounds the map to the merged element set.
I have a few items below, roughly in order of importance.
Items to address
-
Hydration on non-initial history entry discards persisted ids (inline at
app-browser-entry.ts:887). This was raised in the PR thread. The current code always callscreateInitialBfcacheIdMap(resolvedElements)on hydration, so a hard reload on a non-initial entry resets to"0"and persists that map, potentially colliding with the initial entry's_b_0_. The version bump at line 244 mitigates cross-document collisions for traversals, but the hydration entry itself still gets a stale-looking"0"instead of preserving its pre-reload minted id. The E2E test at spec line 43 asserts this reset to_b_0_is expected — is this intentional parity with Next.js, or a known gap? If intentional, a brief comment at line 887 explaining why would help. -
createActiveSlotIdentitytruthiness check (inline atapp-browser-state.ts:207). Empty string fromactiveRouteIdwould fall through to the interception check. Strict nullish check would be more robust. -
BfcacheSlotBoundarynon-null assertion (inline atslot.tsx:125). Safe in context but fragile if the component is ever extracted.
Non-blocking observations
-
useRouter()referential stability change. Switching from singleton return touseMemospread is correct for Next.js 15.3+ parity, but components relying onObject.is(router1, router2)across renders will break. Worth noting in release notes. -
Module-level
nextBfcacheIdcounter resets to 0 on every full page load. TheMath.maxinrememberBfcacheIdensures monotonic advancement even under concurrent navigation starts. The version-gating mechanism prevents cross-document collisions. This is correct. -
isBfcacheSegmentIdcallsparseElementKeyper invocation. For typical route trees (5-20 segments) this is negligible, but worth noting as an optimization target if trees grow large. -
Forward-after-reload E2E gap. The hard reload test (spec line 43) verifies reload on
/x/2→ back to/x/1gets a fresh id, but doesn't verify that forward back to/x/2preserves the reloaded entry's id. The version-bumping mechanism should handle it, but an explicit test would strengthen coverage.
| const [treeStateValue, setTreeStateValue] = useState<AppRouterState | Promise<AppRouterState>>( | ||
| () => ({ | ||
| activeOperation: null, | ||
| bfcacheIds: createInitialBfcacheIdMap(resolvedElements), |
There was a problem hiding this comment.
This always seeds bfcache ids from createInitialBfcacheIdMap (all "0"), even when hydrating a non-initial history entry that has persisted bfcache ids from a prior soft navigation.
The version bump at line 244 prevents traversal restoration of stale ids, which is correct. But the hydration entry itself gets "0" and then persists that map to history state in the layout effect at line 949. This means the reloaded entry loses its pre-reload identity.
Is this intentional Next.js parity (i.e., Next.js also resets to _b_0_ on hard reload of any entry)? If so, a brief comment here would help future readers understand the design decision:
| bfcacheIds: createInitialBfcacheIdMap(resolvedElements), | |
| bfcacheIds: createInitialBfcacheIdMap(resolvedElements), // Intentional: hard reload resets to hydration sentinel, matching Next.js behavior. Version-gating prevents cross-document traversal collisions. |
If it's not intentional parity, the fix would be to seed from readCurrentBfcacheVersionHistoryIds(window.history.state) when present (filtered to the current element set), falling back to createInitialBfcacheIdMap. But the version bump means persisted ids will always be version-gated out here, so that would require special-casing the initial version check. Either way, documenting the choice is the ask.
|
|
||
| function createActiveSlotIdentity(id: string, metadata: AppElementsMetadata | null): string | null { | ||
| const activeSlotBinding = metadata?.slotBindings.find((binding) => binding.slotId === id); | ||
| if (activeSlotBinding?.activeRouteId) { |
There was a problem hiding this comment.
This uses truthiness to check activeSlotBinding?.activeRouteId. If encodeRouteId ever returns an empty string (falsy), this falls through to the interception check instead of using the binding's route id. Empty string seems unlikely from encodeRouteId, but a strict nullish check would be more defensive:
| if (activeSlotBinding?.activeRouteId) { | |
| if (activeSlotBinding?.activeRouteId != null && activeSlotBinding.activeRouteId !== '') { |
Or simply != null if empty string is truly impossible from the wire format.
| } | ||
|
|
||
| function BfcacheSlotBoundary({ content, id }: { content: React.ReactNode; id: string }) { | ||
| const SegmentContext = BfcacheSegmentIdContext!; |
There was a problem hiding this comment.
Non-null assertion is safe here because BfcacheSlotBoundary is only rendered when BfcacheSegmentIdContext is truthy (line 230). But if this component is ever moved or used independently, it will crash with a confusing error. A defensive guard costs nothing:
| const SegmentContext = BfcacheSegmentIdContext!; | |
| const SegmentContext = BfcacheSegmentIdContext; | |
| if (!SegmentContext) return <>{content}</>; | |
| return <SegmentContext.Provider value={id}>{content}</SegmentContext.Provider>; |
| const currentValue = currentIdentity === nextIdentity ? options.current[id] : undefined; | ||
| // History traversals restore persisted ids first, matching segments keep | ||
| // their current id, and newly-created segments mint a fresh opaque id. | ||
| const value = options.restored?.[id] ?? currentValue ?? mintBfcacheId(); |
There was a problem hiding this comment.
The priority chain restored > currentValue > mint is the core invariant. For traversals, restored wins even if the segment identity has changed (e.g., [group] layout at /x restored to /y's entry). This is correct — traversal should restore exact ids from the target history entry regardless of identity matching, because those ids were correct when the entry was created.
A brief comment documenting this would help future readers:
| const value = options.restored?.[id] ?? currentValue ?? mintBfcacheId(); | |
| // History traversals restore persisted ids first, matching segments keep | |
| // their current id, and newly-created segments mint a fresh opaque id. | |
| // Restored ids intentionally win over identity-matching: the target entry's | |
| // ids were authoritative when that entry was created, and traversal must | |
| // faithfully restore them even if the segment's identity has since changed. | |
| const value = options.restored?.[id] ?? currentValue ?? mintBfcacheId(); |
| } | ||
| } | ||
|
|
||
| if (metadata.bfcacheIds !== undefined) { |
There was a problem hiding this comment.
Nit: when bfcacheIds is non-null but empty ({}), lines 68-69 delete both the ids key and the version key. Then the version block at line 75 runs independently — if bfcacheVersion was also passed (e.g., bfcacheIds: {}, bfcacheVersion: 5), it would re-write the version key at line 77 after it was just deleted.
The final state is correct (version is present without ids, which is harmless), but the interaction is surprising. Consider guarding the version write against the empty-ids case, or adding a comment noting this is intentional.
| let nextHistoryTraversalIndex: number = currentHistoryTraversalIndex; | ||
|
|
||
| function isCurrentBfcacheVersion(state: unknown): boolean { | ||
| return (readHistoryStateBfcacheVersion(state) ?? 0) === currentBfcacheVersion; |
There was a problem hiding this comment.
When readHistoryStateBfcacheVersion(state) returns null (no version field) and currentBfcacheVersion is 0, this returns true. That means on a completely fresh page load, any history entry with bfcache ids but no version field is treated as current-version.
This is a narrow edge case (only happens if ids were written before version-gating was introduced), and treating them as version 0 is reasonable. Just want to confirm this was considered — if old pre-version-gating entries exist in user session history, they'll be restored as current, which may or may not be desirable.
| | DispatchPendingNavigationCommitDispositionDecision | ||
| | NonDispatchPendingNavigationCommitDispositionDecision; | ||
|
|
||
| let nextBfcacheId = 0; |
There was a problem hiding this comment.
The module-level nextBfcacheId counter resets to 0 on every full page load. This is correct for the browser environment (single document lifetime), and the version-gating mechanism in app-browser-entry.ts prevents cross-document id collisions. A brief comment noting this scoping would help:
| let nextBfcacheId = 0; | |
| // Monotonic counter scoped to the current browser document lifetime. Resets on | |
| // full page load, which is safe because version-gating in app-browser-entry.ts | |
| // prevents cross-document bfcache id collisions. | |
| let nextBfcacheId = 0; |
| @@ -0,0 +1,256 @@ | |||
| /** | |||
There was a problem hiding this comment.
The test coverage here is thorough — traversal, redirect, hard reload, hash-only, intercepted routes, server actions, and programmatic push/replace are all covered.
One gap: no test for "forward after reload preserves the reloaded entry's id." The hard reload test (line 43) verifies reload → back gives a fresh id, but doesn't verify that going forward back to the reloaded entry restores the post-reload _b_0_ id (rather than minting yet another fresh one). The version-bumping mechanism should handle this correctly, but an explicit assertion would close the loop.
Also appreciate the Next.js test attribution comment at line 3 per AGENTS.md guidelines.
| const override = resolveSlotOverride(slotKey, slot.name); | ||
| const slotId = resolveAppPageSlotId(slot, layoutEntry?.treePath ?? "/"); | ||
| const state = resolveAppPageSlotBindingState(slot, override); | ||
| const activeRouteId = |
There was a problem hiding this comment.
Clean implementation: activeRouteId is only populated for state === "active" slots, distinguishing interception targets from regular route ids via encodeRouteId. This correctly addresses the intercepted-route identity concern from the earlier review, where /photo/42 → /photo/43 in the same @modal slot would incorrectly share a bfcache id.
| const mergedElements = mergeElements(state.elements, action.elements, { | ||
| clearAbsentSlots: action.type === "traverse", | ||
| preserveAbsentSlots: commit.decision.preserveAbsentSlots, | ||
| preserveElementIds: commit.decision.preserveElementIds, | ||
| preservePreviousSlotIds: commit.decision.preservePreviousSlotIds, | ||
| }); | ||
| return commitVisibleRouterState( | ||
| state, | ||
| { | ||
| elements: mergeElements(state.elements, action.elements, { | ||
| clearAbsentSlots: action.type === "traverse", | ||
| preserveAbsentSlots: commit.decision.preserveAbsentSlots, | ||
| preserveElementIds: commit.decision.preserveElementIds, | ||
| preservePreviousSlotIds: commit.decision.preservePreviousSlotIds, | ||
| bfcacheIds: preserveBfcacheIdsForMergedElements({ |
There was a problem hiding this comment.
Good refactor: computing mergedElements first, then reconciling bfcache ids against the merged set. This correctly handles the case where element merging preserves segments from the previous state that aren't in the new action's bfcache id map. The separation is clean and makes the bfcache id scope obvious — the final map is always bounded by the merged element keys.
|
Review posted on PR #1588 with 10 inline comments. Here's a summary: Architecture: Well-layered and clean. History persistence, id lifecycle, browser integration, and rendering are properly separated. The version-gating mechanism for hard reload safety, redirect-clearing across all three redirect paths, and Items to address (3):
Non-blocking observations (4):
|
manNomi
force-pushed
the
fix/use-router-bfcache-id-semantics-v2
branch
from
0c373bf to
63129fe
Compare
manNomi
changed the title
manNomi
changed the title
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@james-elicx Would it be okay to trigger one final /bigbonk review? |
|
/bigbonk review |
There was a problem hiding this comment.
Review: useRouter bfcacheId semantics (v2)
The architecture is well-designed and the layering is clean. History persistence (app-history-state), id lifecycle (app-browser-state), browser integration (app-browser-entry), and rendering (slot + navigation shims) have good separation of concerns. The version-gating mechanism for hard reload safety is sound. The redirect-clearing across all three redirect paths is consistent. The preserveBfcacheIdsForMergedElements post-merge reconciliation correctly bounds the map to the merged element set.
Test coverage is thorough: 12 unit tests for the id map algorithm, integration tests for reducer-level preservation, and 10 Playwright E2E tests covering traversal, redirect, hard reload, hash-only, intercepted routes, server actions, and programmatic push/replace. The Next.js test attribution at the top of the E2E spec follows AGENTS.md guidelines. The catch-all segment handling (from the earlier Codex review) is addressed.
I have a few items below, ordered by importance.
Items to address
-
formatPublicBfcacheIdsentinel check couples to the singleton (inline atnavigation.ts:1610). The"0"string literal is already correct there, butappRouterInstance.bfcacheIdis also"0"— making it ambiguous whether the check is against the sentinel constant or the singleton's property. ImportINITIAL_BFCACHE_IDfromapp-browser-state(it's already"0") or add a constant locally to make intent clear. -
createActiveSlotIdentitytruthiness check (inline atapp-browser-state.ts:210). IfencodeRouteIdever produces empty string, the fallthrough would silently use the interception check instead of the binding's route id. Strict nullish coalescing would be more robust. -
BfcacheSlotBoundarynon-null assertion (inline atslot.tsx:125). The current code is safe because the caller gates onBfcacheSegmentIdContextbeing truthy, but if the component is extracted or reused, it will crash. A defensive guard costs nothing. -
Hydration on non-initial history entry always resets to
"0"(inline atapp-browser-entry.ts:891). This was raised by NathanDrake2406 and addressed by ask-bonk. The version bump prevents cross-document traversal collisions, but the hydration entry itself loses its pre-reload identity. The E2E test at spec line 61 asserts_b_0_after reload, confirming this is the intended behavior. A brief inline comment documenting this as intentional parity would prevent future churn.
Non-blocking observations
-
Module-level
nextBfcacheIdcounter scoping. Already has a good comment block (lines 147-149), but noting for completeness — theMath.maxinrememberBfcacheId+ version-gating make this safe. -
areBfcacheIdMapsEqualis O(n) on everysyncCurrentHistoryStatePreviousNextUrlcall. For typical route trees (5-20 segments) this is negligible, but documenting the expectation would help if someone uses this for a deep tree. -
Forward-after-reload E2E gap. The hard reload test (spec line 43) was updated to cover forward-after-reload (line 73-76), which closes the loop raised in the thread. Good.
-
useRouter()referential stability change. Switching from singleton return touseMemospread is correct for Next.js 15.3+ parity, but components relying onObject.is(router1, router2)will see new references. Worth noting in release notes.
| }; | ||
|
|
||
| function formatPublicBfcacheId(value: string | null | undefined): string { | ||
| if (!value || value === "0") return "_b_0_"; |
There was a problem hiding this comment.
The "0" literal is correct and already decouples from _appRouter.bfcacheId, which addresses the concern from the earlier ask-bonk review. But the relationship between this "0" and the INITIAL_BFCACHE_ID constant in app-browser-state.ts is implicit — if one changes, the other silently breaks.
Consider importing the constant to make the coupling explicit:
| if (!value || value === "0") return "_b_0_"; | |
| function formatPublicBfcacheId(value: string | null | undefined): string { | |
| if (!value || value === INITIAL_BFCACHE_ID) return "_b_0_"; | |
| return value; | |
| } |
(with import { INITIAL_BFCACHE_ID } from "../server/app-browser-state.js" — or just export it from app-history-state.ts where the type already lives.)
Alternatively, a local constant const INITIAL_BFCACHE_ID = "0" would also work. The goal is to avoid silent drift.
There was a problem hiding this comment.
Addressed in 55135824 by moving the internal zero sentinel into shared bfcache id constants and using INITIAL_BFCACHE_ID / PUBLIC_INITIAL_BFCACHE_ID in the navigation shim, so the public formatting can no longer drift from the runtime sentinel.
|
|
||
| function createActiveSlotIdentity(id: string, metadata: AppElementsMetadata | null): string | null { | ||
| const activeSlotBinding = metadata?.slotBindings.find((binding) => binding.slotId === id); | ||
| if (activeSlotBinding?.activeRouteId != null && activeSlotBinding.activeRouteId !== "") { |
There was a problem hiding this comment.
The != null && !== "" guard was added per earlier review feedback — good. One nit: the != null check already covers undefined (via loose equality), but the empty-string check uses strict equality. This is fine logically but reads a bit mixed. If you want consistency, either:
if (activeSlotBinding?.activeRouteId !== undefined && activeSlotBinding.activeRouteId !== null && activeSlotBinding.activeRouteId !== '')or keep the current form (which is correct). Not blocking.
There was a problem hiding this comment.
Already addressed: createActiveSlotIdentity now uses a strict guard for activeRouteId (!= null plus an empty-string check) before falling back to interception metadata.
| } | ||
|
|
||
| function BfcacheSlotBoundary({ content, id }: { content: React.ReactNode; id: string }) { | ||
| const SegmentContext = BfcacheSegmentIdContext; |
There was a problem hiding this comment.
The non-null assertion BfcacheSegmentIdContext! is safe here because the caller at line 231 checks BfcacheSegmentIdContext is truthy before rendering this component. But if BfcacheSlotBoundary is ever extracted or called from a different site, it'll crash with a confusing error.
A defensive guard costs nothing and prevents future surprises:
| const SegmentContext = BfcacheSegmentIdContext; | |
| function BfcacheSlotBoundary({ content, id }: { content: React.ReactNode; id: string }) { | |
| const SegmentContext = BfcacheSegmentIdContext; | |
| if (!SegmentContext) return <>{content}</>; | |
| return <SegmentContext.Provider value={id}>{content}</SegmentContext.Provider>; | |
| } |
There was a problem hiding this comment.
Already addressed: BfcacheSlotBoundary now reads the context into a local SegmentContext and returns the content unchanged when the context is unavailable, avoiding the non-null assertion.
| // in-memory CacheNode/Activity tree. Hydrate the new document on the | ||
| // zero sentinel and rely on the document-scoped bfcache version gate to | ||
| // reject stale ids persisted by previous documents. | ||
| bfcacheIds: createInitialBfcacheIdMap(resolvedElements), |
There was a problem hiding this comment.
This always seeds from createInitialBfcacheIdMap (all "0"), even when hydrating a non-initial history entry. The version bump at line 244 ensures traversals into this entry won't collide, and the E2E test at spec line 61 asserts _b_0_ is the expected post-reload value.
The existing comment on lines 887-890 is good but doesn't explicitly state that this is intentional Next.js parity. Adding a brief note would prevent future engineers from "fixing" this:
| bfcacheIds: createInitialBfcacheIdMap(resolvedElements), | |
| bfcacheIds: createInitialBfcacheIdMap(resolvedElements), // Intentional: hard reload resets to hydration sentinel, matching Next.js behavior. |
There was a problem hiding this comment.
Addressed in 55135824: the hydration comment now explicitly calls this intentional Next.js parity and explains that hard reload starts a new browser document on the zero sentinel while the document-scoped bfcache version gate rejects stale ids from previous documents.
| // Restored ids intentionally win over identity-matching: the target entry's | ||
| // ids were authoritative when that entry was created, and traversal must | ||
| // faithfully restore them even if the segment's identity has since changed. | ||
| const value = options.restored?.[id] ?? currentValue ?? mintBfcacheId(); |
There was a problem hiding this comment.
The comment here is clear and correct. The restored > currentValue > mint priority chain is the core invariant and the comment documents why restored wins over identity-matching for traversals. Well done — this was one of the trickier design decisions and the explanation helps future readers.
| } | ||
| } | ||
|
|
||
| if (metadata.bfcacheVersion !== undefined) { |
There was a problem hiding this comment.
Nit: when bfcacheIds is non-null but empty ({}), lines 70-72 delete both the ids key and the version key. Then this bfcacheVersion block runs independently — if bfcacheVersion was also passed (e.g., bfcacheIds: {}, bfcacheVersion: 5), line 82 would re-write the version key after it was just deleted at line 72.
The final state is correct (version ends up present without ids, which is harmless — and the unit test at app-browser-entry.test.ts:4374 covers this exact case, asserting both are cleaned up). But the intermediate write-then-overwrite is slightly surprising.
Consider guarding:
if (metadata.bfcacheVersion !== undefined && !bfcacheIdsWereCleared) {Not blocking since the test already covers the correct final state.
There was a problem hiding this comment.
Already addressed: createHistoryStateWithNavigationMetadata now tracks bfcacheIdsWereCleared and skips/re-clears the version field when ids are explicitly cleared. The regression is covered by the empty bfcacheIds unit test.
| let nextHistoryTraversalIndex: number = currentHistoryTraversalIndex; | ||
|
|
||
| function isCurrentBfcacheVersion(state: unknown): boolean { | ||
| return (readHistoryStateBfcacheVersion(state) ?? 0) === currentBfcacheVersion; |
There was a problem hiding this comment.
When readHistoryStateBfcacheVersion(state) returns null and currentBfcacheVersion is 0, this returns true. This means pre-version-gating history entries (which lack the version field) are treated as current-version on a fresh page load.
This is a narrow edge case — only possible if ids were written before version-gating was introduced — and treating them as version 0 is reasonable. The behavior is sound; just flagging that this was considered.
| const override = resolveSlotOverride(slotKey, slot.name); | ||
| const slotId = resolveAppPageSlotId(slot, layoutEntry?.treePath ?? "/"); | ||
| const state = resolveAppPageSlotBindingState(slot, override); | ||
| const activeRouteId = |
There was a problem hiding this comment.
Clean implementation: activeRouteId is only populated for state === "active" slots, and the interception target distinction via encodeRouteId correctly handles the intercepted-route identity concern from the earlier review (where /photo/42 → /photo/43 in the same @modal would incorrectly share a bfcache id).
| expect(postReloadBackBfcacheId).not.toBe(x1BfcacheId); | ||
| await expect(visibleTestId(page, "leaf-input")).not.toHaveValue("x2-state-after-reload"); | ||
|
|
||
| await page.goForward(); |
There was a problem hiding this comment.
Good: this forward-after-reload assertion at lines 73-76 closes the coverage gap that was flagged in the PR thread. It verifies that navigating forward back to the reloaded entry preserves the post-reload _b_0_ id rather than minting yet another fresh one.
| // the mounted router instance as the source of method behavior and only layer | ||
| // contextual bfcacheId on top. | ||
| // Ported from: https://github.com/vercel/next.js/blob/canary/packages/next/src/client/components/navigation.ts | ||
| it("useRouter() preserves methods from the mounted AppRouterContext router", async () => { |
There was a problem hiding this comment.
Excellent test — this verifies that useRouter() preserves custom methods from the mounted AppRouterContext router (not just the singleton's methods) while layering bfcacheId on top. This directly addresses the concern from NathanDrake2406's earlier review about context ownership.
|
Review posted successfully on PR #1588 with 10 inline comments. Here's a summary: Architecture: Well-layered and clean. History persistence, id lifecycle, browser integration, and rendering are properly separated. The version-gating mechanism, redirect-clearing across all three redirect paths, and Items to address (4):
Non-blocking observations: module-level counter scoping is correct, |
|
/bigbonk review |
There was a problem hiding this comment.
Review: useRouter bfcacheId semantics
This is a well-designed feature with clean layering and thorough test coverage. The architecture separates concerns effectively:
app-bfcache-id.ts— shared constants (the simplest possible shared module)app-history-state.ts— serialization/deserialization of bfcache state in history entriesapp-browser-state.ts— identity derivation, minting, and map lifecycleapp-browser-entry.ts— version-gating, browser integration, and state threadingslot.tsx+navigation.ts— React context plumbing and public API surface
The version-gating mechanism for hard reload safety is sound. The three-way redirect clearing (cached, streamed, server-action) is consistent. The preserveBfcacheIdsForMergedElements post-merge reconciliation correctly bounds the map to the merged element set. The E2E suite ports the Next.js test scenarios well and adds coverage for programmatic navigation and intercepted routes.
I have a few observations and one substantive concern. The rest are minor or non-blocking.
Substantive
readBfcacheIdFromContextswallows all exceptions (navigation.ts:1627). Thetry/catcharounduseContextcalls catches everything and returns the fallback sentinel. This means if the context providers are present but a bug causesuseContextto throw for a reason other than "called outside render" (e.g., a React internal invariant violation), the error is silently swallowed and the component renders with_b_0_instead of surfacing the bug. Consider narrowing the catch or at least logging in dev.
Non-blocking
-
SSR entry doesn't provide
BfcacheSegmentIdContext(app-ssr-entry.ts:306-312). The SSR entry providesBfcacheIdMapContextbut not the per-segmentBfcacheSegmentIdContext. This meansreadBfcacheIdFromContext()during SSR will always fall through to the null path and return_b_0_. This is likely intentional (SSR doesn't have segment-level bfcache ids), but a brief comment inapp-ssr-entry.tswould make that explicit. -
createHashOnlyHistoryStatePreservingNavigationMetadataomitstraversalIndex(app-history-state.ts:152-166). The hash-only preservation function reads and carries forwardpreviousNextUrl,bfcacheIds, andbfcacheVersion, but drops thetraversalIndex. Compare withcommitHashOnlyNavigationinapp-browser-entry.ts:298which handles traversal index separately viaallocateNavigationHistoryTraversalIndex. This is fine because the two code paths serve different purposes (one is for the browser entry, the other for the navigation shim's direct hash commits), but the asymmetry is easy to miss. -
collectBfcacheSegmentIdssilently catches metadata errors (app-browser-state.ts:252-258). The empty catch when reading metadata means partial element maps (from tests) don't blow up, but it also means corrupted metadata in production would silently produce incomplete bfcache id maps. The comment says "Some low-level tests pass partial element maps" — this is fine for now but consider making the catch dev-only or logging if you add more segment sources later. -
areBfcacheIdMapsEqualiteratesabut notb(app-browser-entry.ts:504-510). The function checksaEntries.length === bEntries.lengththen only iteratesato verify values match inb. Since the lengths are equal, this is correct — every key inamust exist inbwith the same value, and sincebhas the same number of entries, there can't be extra keys. But at first glance it looks like it only checks one direction. A brief comment noting the length check makes this sufficient would help.
Good work overall. The feedback from prior reviews has been addressed well — useRouter() correctly reads from AppRouterContext, the public format uses shared constants, and the forward-after-reload E2E gap is closed.
| } catch { | ||
| // Low-level tests and direct module calls can hit this outside render. | ||
| return formatPublicBfcacheId(null); | ||
| } |
There was a problem hiding this comment.
This catch-all silently swallows any exception from useContext, not just the "called outside render" case. If the contexts are wired up but React throws an internal invariant (e.g., a hook ordering violation in a caller that happens to use readBfcacheIdFromContext), this hides the real error and quietly returns _b_0_.
Consider at minimum logging in dev:
| } | |
| } catch (error) { | |
| // Low-level tests and direct module calls can hit this outside render. | |
| if (process.env.NODE_ENV !== "production") { | |
| console.warn("[vinext] readBfcacheIdFromContext failed:", error); | |
| } | |
| return formatPublicBfcacheId(null); | |
| } |
| { value: createInitialBfcacheIdMap(elements) }, | ||
| routeTree, | ||
| ) | ||
| : routeTree; |
There was a problem hiding this comment.
The SSR entry provides BfcacheIdMapContext but not BfcacheSegmentIdContext. This means readBfcacheIdFromContext() during SSR will always return _b_0_ (the null fallback), which is correct since SSR doesn't have segment-level bfcache ids. A brief comment would make this intentional omission explicit and prevent someone from "fixing" it later.
Something like:
// BfcacheSegmentIdContext is intentionally omitted during SSR: per-segment
// bfcache ids are a browser-only concern. useRouter().bfcacheId returns the
// hydration sentinel during SSR, matching Next.js behavior.| for (const layoutId of AppElementsWire.readMetadata(elements).layoutIds) { | ||
| ids.add(layoutId); | ||
| } | ||
| } catch { |
There was a problem hiding this comment.
Nit: the empty catch here means corrupted metadata in production would silently produce an incomplete bfcache id map (only the direct element keys, not the layoutIds). The comment explains the test motivation, which is fine. But if you later add more metadata-derived segment sources, this catch could mask real issues.
Not blocking — just flagging for future awareness.
| return null; | ||
| } | ||
|
|
||
| return createHistoryStateWithNavigationMetadata(null, { |
There was a problem hiding this comment.
Minor: this function preserves previousNextUrl, bfcacheIds, and bfcacheVersion from the current history state but intentionally omits traversalIndex. The browser entry's commitHashOnlyNavigation handles the traversal index separately via allocateNavigationHistoryTraversalIndex, while the navigation shim's commitHashOnlyHistoryState calls this function directly.
The asymmetry is correct (two different code paths for two different callers), but it's subtle. Consider adding a brief comment noting that traversal index is handled by the caller in the browser entry path.
| if (aEntries.length !== bEntries.length) return false; | ||
| return aEntries.every(([key, value]) => b[key] === value); | ||
| } | ||
|
|
There was a problem hiding this comment.
Nit: this iterates only a's entries and checks them against b, relying on the length equality check to ensure completeness. This is correct — if a and b have the same number of entries, and every key-value in a exists in b, then a === b as maps. But on first reading it looks like a one-directional subset check.
A one-line comment would help:
| return aEntries.every(([key, value]) => b[key] === value); // length check above guarantees bidirectional equality |
|
Review posted successfully on PR #1588 with 5 inline comments. Here's a summary: Architecture: Clean and well-layered. The separation across Substantive concern:
Non-blocking observations:
|
manNomi
force-pushed
the
fix/use-router-bfcache-id-semantics-v2
branch
from
c87b723 to
f0ff09d
Compare
|
@james-elicx could you run /bigbonk review once more when you have a chance? |
|
/bigbonk review |
|
Addressed the functional concern around the
Targeted local verification passed: pnpm exec vp check packages/vinext/src/shims/navigation.ts tests/shims.test.ts
pnpm exec vp test run --project unit tests/shims.test.ts -t "useRouter" |
manNomi
force-pushed
the
fix/use-router-bfcache-id-semantics-v2
branch
from
2bcf94f to
4b309da
Compare
|
/bigbonk review |
There was a problem hiding this comment.
Review: useRouter bfcacheId semantics
This is a well-designed feature implementation that has clearly benefited from multiple rounds of thoughtful review feedback. The architecture is clean and the layering is well-considered:
app-bfcache-id.ts— shared constants (minimal, the right abstraction)app-history-state.ts— serialization/deserialization with validation and version-gatingapp-browser-state.ts— identity derivation, minting, map lifecycleapp-browser-entry.ts— version-gating integration, browser state threadingslot.tsx+navigation.ts— React context plumbing and public API surface
Strengths
- The
restored > currentValue > mintpriority chain increateNextBfcacheIdMapis the right design for back/forward navigation semantics, and it's well-documented. - Version-gating for hard reload safety (
currentBfcacheVersion = initialVersion + 1) is sound and avoids cross-document id collisions without requiring complex bookkeeping. - Redirect clearing is consistent across all three paths (cached, streamed, server-action).
preserveBfcacheIdsForMergedElementscorrectly scopes the bfcache id map to the merged element set, preventing stale slot ids from leaking across reducer-level element merges.- The
isHistoryStateNavigationMetadataInSyncextraction is clean and eliminates the sync-predicate duplication that was flagged. - Test coverage is thorough: unit tests for identity matching (catch-all, intercepted slots, reducer-level preservation, monotonic counter), history serialization round-trips, and 10 E2E Playwright tests covering the full behavioral surface.
- The E2E fixture design is clever —
LinkAccordionavoids prefetch-triggered navigations, and the redirect test simulates a stale history entry withpushStateto test traverse redirect behavior.
Minor observations (non-blocking)
A few inline comments below — nothing blocking, just notes for future maintainers. The prior review feedback has been comprehensively addressed.
Good work overall. This is ready to merge.
| // Some low-level tests pass partial element maps without metadata. | ||
| } | ||
|
|
||
| return Array.from(ids).filter(isBfcacheSegmentId); |
There was a problem hiding this comment.
Nit: collectBfcacheSegmentIds iterates Object.keys(elements) and then filters through isBfcacheSegmentId, which calls AppElementsWire.parseElementKey per key. For a typical route tree this is negligible, but this function is called from both createInitialBfcacheIdMap and createNextBfcacheIdMap (the latter calls it twice — once for current, once for next elements, via the collectBfcacheSegmentIds in createBfcacheSegmentIdentity). If someone later adds deeply nested parallel routes, the O(n × parse) cost could add up.
Not worth changing now, but worth noting for future optimization if route trees grow large — a cached Set<string> per element map or a fast prefix check could replace the parseElementKey calls.
| let nextHistoryTraversalIndex: number = currentHistoryTraversalIndex; | ||
|
|
||
| function isCurrentBfcacheVersion(state: unknown): boolean { | ||
| return (readHistoryStateBfcacheVersion(state) ?? 0) === currentBfcacheVersion; |
There was a problem hiding this comment.
Minor: when readHistoryStateBfcacheVersion(state) returns null and currentBfcacheVersion is 0 (a completely fresh page load with no prior bfcache history), this returns true — meaning pre-version-gating history entries (with ids but no version field) are treated as current-version.
This is the correct behavior for the bootstrap edge case (there's no way to have ids without a version except from before version-gating existed), and readCurrentBfcacheVersionHistoryIds correctly returns null when ids themselves are absent, so it's safe in practice. A one-line comment noting this edge case was considered would help future readers, but not blocking.
| // Restored ids intentionally win over identity-matching: the target entry's | ||
| // ids were authoritative when that entry was created, and traversal must | ||
| // faithfully restore them even if the segment's identity has since changed. | ||
| const value = options.restored?.[id] ?? currentValue ?? mintBfcacheId(); |
There was a problem hiding this comment.
The comment here is excellent. The restored > currentValue > mint priority chain is the trickiest invariant in this implementation, and the explanation makes the design decision immediately clear to future readers. Well done.
| try { | ||
| const segmentId = React.useContext(segmentContext); | ||
| const idMap = React.useContext(idMapContext); | ||
| return formatPublicBfcacheId(segmentId !== null ? idMap?.[segmentId] : null); |
There was a problem hiding this comment.
Good: this now uses segmentId !== null (strict nullish) rather than truthiness, correctly handling the empty-string segment id case. The unit test at shims.test.ts:158 covers this explicitly.
| preserveAbsentSlots: commit.decision.preserveAbsentSlots, | ||
| preserveElementIds: commit.decision.preserveElementIds, | ||
| preservePreviousSlotIds: commit.decision.preservePreviousSlotIds, | ||
| bfcacheIds: preserveBfcacheIdsForMergedElements({ |
There was a problem hiding this comment.
Clean refactor: computing mergedElements before preserveBfcacheIdsForMergedElements makes the scope obvious — the final bfcache id map is always bounded by the merged element keys, so stale slot ids that were cleared during element merge are also dropped. The variable extraction makes this two-phase process readable.
|
Review posted successfully on PR #1588 with an APPROVE and 5 inline comments. Here's the summary: Architecture: Clean five-layer separation — shared constants, serialization, identity/minting, browser integration, React context/API. The design decisions are sound and well-documented after multiple review iterations. No blocking items. All prior review feedback has been comprehensively addressed:
Inline observations (all non-blocking):
|
|
/bigbonk re-review for issues and next.js parity |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk re-review for issues and next.js parity |
1 similar comment
|
/bigbonk re-review for issues and next.js parity |
There was a problem hiding this comment.
Re-review: Next.js parity and correctness
I've compared this implementation against the Next.js source at .nextjs-ref (packages/next/src/client/components/navigation.ts, ppr-navigations.ts, segment-cache/bfcache.ts, app-router-context.shared-runtime.ts) and the reference test suite at test/e2e/app-dir/use-router-bfcache-id/.
Architecture parity assessment
The vinext implementation uses a fundamentally different mechanism than Next.js but achieves equivalent user-visible semantics:
| Concern | Next.js | vinext (this PR) |
|---|---|---|
| Storage | bfcacheId: number on each CacheNode in the router state tree |
BfcacheIdMap (Record<string, string>) keyed by element wire keys |
| Generation | ++nextBFCacheId (monotonic counter, 0 for hydration) |
mintBfcacheId() returning _b_N_ strings (monotonic counter, "0" sentinel for hydration) |
| Context scoping | LayoutRouterContext.parentCacheNode.bfcacheId (per-CacheNode) |
BfcacheSegmentIdContext (per-slot boundary) + BfcacheIdMapContext (tree-wide map) |
| Public format | '_b_' + number + '_' computed in useRouter() |
formatPublicBfcacheId() — same format |
| History persistence | Stored in segment-cache BFCache data store (not history state) | Stored in window.history.state.__vinext_bfcacheIds |
| Identity matching | compareSegments() with SegmentMatchKind enum |
createBfcacheSegmentIdentity() with pathname prefix matching |
The architectural difference in history persistence is notable: Next.js stores bfcache data in an in-memory segment cache (bfcache.ts) and reads it during traversals. Vinext stores the id map directly in history state, which is correct for the Cloudflare Workers deployment target where there's no persistent in-memory cache across navigations. The version-gating mechanism compensates for the cross-document risks this introduces.
Behavioral parity
The PR correctly implements the core Next.js semantics:
- Fresh push/replace → new bfcacheId ✓
- Back/forward → restored bfcacheId ✓
router.refresh()→ same bfcacheId ✓- Search-param-only change → same bfcacheId ✓
- Hash-only change → same bfcacheId ✓
- Shared layout across sibling leaves → same layout bfcacheId ✓
- Different dynamic group → new layout bfcacheId ✓
- Hard reload → reset to
_b_0_(matches Next.js hydration sentinel) ✓ - Server action refresh → same bfcacheId ✓
Items found
I have two parity concerns to flag, a question about non-bfcache changes bundled in this PR, and a test coverage suggestion. See inline comments.
Minor note on non-bfcache changes
The cached redirect lifecycle hop handling (around line 1538 of app-browser-entry.ts) and the server-action redirect currentHistoryMode/currentPrevNextUrl updates (around line 1800) appear to be new navigation behavior that isn't specific to bfcacheId. If these are fixing pre-existing bugs, they should be called out in the PR description to make the scope clear.
| ...router, | ||
| bfcacheId, | ||
| }), | ||
| [router, bfcacheId], |
There was a problem hiding this comment.
Next.js parity note: useRouter() spreads the entire context router, but Next.js enumerates specific method names.
Next.js's useRouter() at packages/next/src/client/components/navigation.ts:193-204 explicitly lists the method names it returns:
return useMemo(() => ({
back: router.back,
forward: router.forward,
refresh: router.refresh,
hmrRefresh: router.hmrRefresh,
push: router.push,
replace: router.replace,
prefetch: router.prefetch,
experimental_gesturePush: router.experimental_gesturePush,
bfcacheId: '_b_' + bfcacheIdNumber + '_',
}), [router, bfcacheIdNumber])Vinext uses { ...router, bfcacheId } which includes any extra properties on the context router value. The unit test at shims.test.ts:62 actually tests this "preserves custom methods" behavior as a feature — but it's a divergence from Next.js, which does not preserve extra properties.
This is a minor divergence that's arguably more permissive than Next.js. Not blocking, but worth noting in a code comment or compatibility note that the spread approach leaks internal properties to userland (e.g., any future private properties added to appRouterInstance).
| metadata: nextMetadata, | ||
| pathname: options.nextPathname, | ||
| }); | ||
| const currentValue = currentIdentity === nextIdentity ? options.current[id] : undefined; |
There was a problem hiding this comment.
Next.js parity note: identity matching uses pathname prefix vs. Next.js's direct segment comparison.
Next.js determines whether a segment changed via compareSegments(newSegment, oldSegment) which compares the segment values directly from the flight response tree. Vinext's createBfcacheSegmentIdentity derives identity from getTreePathIdentityPrefix(pathname, treePath) — which computes how many pathname segments the tree path consumes, then takes that prefix of the actual pathname.
This works correctly for the tested cases, and the existing comment at line 220-223 ("Legacy bridge...") correctly acknowledges this is a temporary approach. No action needed now, just confirming the design boundary is understood and documented.
One thing worth testing in the future: route groups with multiple dynamic segments at different tree depths where the identity prefix length matters (e.g., /(group)/[a]/[b]/layout.tsx navigating between /x/1 and /x/2 — the [b] change should mint a new id for this layout).
| performHardNavigationForScrollIntent(compatibilityDecision.hardNavigationTarget); | ||
| return; | ||
| } | ||
| const cachedRedirectDecision = resolveRscRedirectLifecycleHop({ |
There was a problem hiding this comment.
Question: is this cached redirect lifecycle hop handling new non-bfcache behavior?
This block (lines 1538-1564) adds redirect handling for cached (visited-response) routes, including redirect resolution, max-redirect guard, and currentHref/currentHistoryMode/currentPrevNextUrl updates. The restoredBfcacheIds = null clearing at line 1557 is correct for bfcache purposes.
However, the redirect lifecycle hop resolution itself appears to be new navigation behavior. Was this cached redirect path previously missing entirely, or was it handled differently before? If this is fixing a pre-existing gap (cached routes that redirect were not followed), that's a separate concern from bfcacheId and should be noted in the PR description for reviewer clarity.
| @@ -0,0 +1,261 @@ | |||
| /** | |||
| * Next.js Compat E2E: useRouter().bfcacheId | |||
| * Ported from: https://github.com/vercel/next.js/blob/56d95137fd6d84f4bc1e5ef2bb31e0136d5fad9c/test/e2e/app-dir/use-router-bfcache-id/use-router-bfcache-id.test.ts | |||
There was a problem hiding this comment.
Next.js test parity gap: missing form-state preservation assertions.
The Next.js reference test at the linked URL has a test preserves leaf form state on browser back navigation that types into the leaf input, navigates away, goes back, and asserts the input retains its typed value. This relies on React Activity (cacheComponents: true) preserving actual DOM state.
Vinext doesn't have Activity yet, so back navigation remounts the form (same key → same identity, but no DOM preservation). The vinext test at line 23 correctly tests bfcacheId restoration but not form-state preservation, which is the right call for now.
Two suggestions:
- Add a comment noting that Next.js's form-state-preservation tests are omitted because vinext doesn't implement Activity yet.
- The search-param navigation test at line 121 should also test form-state preservation (type into input, navigate to
?q=2, assert input retains value) — this should work even without Activity, since the bfcacheId doesn't change and React won't remount the<form>.
| </ParallelSlotsContext.Provider> | ||
| ); | ||
|
|
||
| return BfcacheIdMapContext && BfcacheSegmentIdContext ? ( |
There was a problem hiding this comment.
Two-level context indirection is clean.
The Slot component wraps every element in a BfcacheSlotBoundary that sets BfcacheSegmentIdContext to the element's wire key. Then readBfcacheIdFromContext looks up that key in BfcacheIdMapContext (the tree-wide map). This is a clean design — the context tree naturally gives each segment the nearest key.
The defensive guard in BfcacheSlotBoundary (line 126) is appreciated.
| for (const value of Object.values(options.current)) { | ||
| rememberBfcacheId(value); | ||
| } | ||
| for (const value of Object.values(options.restored ?? {})) { |
There was a problem hiding this comment.
Subtle but correct: rememberBfcacheId is called before the minting loop to advance the counter past all known ids.
This ensures mintBfcacheId() on line 304 produces ids strictly greater than anything in current or restored. The second rememberBfcacheId(value) at line 306 handles the case where a restored/carried-forward value is used directly (not minted).
Correct, but the intent could be clearer. Consider a brief comment on these lines, e.g.:
| for (const value of Object.values(options.restored ?? {})) { | |
| // Advance the monotonic counter past all known ids before minting. | |
| // This ensures freshly minted ids never collide with existing or | |
| // restored values. | |
| for (const value of Object.values(options.current)) { | |
| rememberBfcacheId(value); | |
| } |
| const state = resolveAppPageSlotBindingState(slot, override); | ||
| const activeRouteId = | ||
| state === "active" | ||
| ? options.interception?.slotId === slotId |
There was a problem hiding this comment.
Good: interception target is distinguished from regular route via targetRouteId vs encodeRouteId.
When the slot is the interception target, activeRouteId uses the interception's targetRouteId (which includes the interception context suffix). Otherwise, it uses encodeRouteId(routePath, null). This ensures /photo/42 and /photo/43 intercepted into the same @modal slot produce distinct identities in createActiveSlotIdentity.
|
Review posted successfully on PR #1588 with 7 inline comments. Here's the summary: Overall assessment: The implementation is architecturally sound and achieves behavioral parity with Next.js's Key findings:
|
3 participants
Summary
useRouter().bfcacheIdsemantics for the App Router shim.bfcacheIdonto the mountedAppRouterContextrouter.Feedback addressed
This is a fresh follow-up based on the review feedback from the earlier bfcacheId PR. In particular, this version:
AppRouterContextas the authority for router methods and only adds the contextualbfcacheIdinuseRouter()_b_0_while sharing the internal zero sentinel constant across the shim/runtimerouter.push()/router.replace()and forward-after-reload coverage in the E2E fixturebfcacheIdstays stableCompatibility note
useRouter()now returns a memoized wrapper around the mounted context router instead of the old module-level singleton. This is intentional Next.js App Router parity so custom/instrumented context router methods are preserved while the nearest segment'sbfcacheIdis added. This is slightly more permissive than Next.js's explicit method list, but it avoids dropping vinext-provided or user-instrumented router methods.The redirect-related updates in this PR are included to keep bfcache metadata coherent across navigation hops. Cached redirect traversal handling and server-action redirect history metadata updates clear or realign stale restored ids,
currentHistoryMode, andcurrentPrevNextUrlso a redirected navigation cannot accidentally reuse history state from the pre-redirect entry.Tests
pnpm exec vp check packages/vinext/src/server/app-bfcache-id.ts packages/vinext/src/server/app-browser-entry.ts packages/vinext/src/server/app-browser-state.ts packages/vinext/src/shims/navigation.ts packages/vinext/src/shims/slot.tsx tests/shims.test.ts tests/app-browser-entry.test.ts tests/e2e/app-router/nextjs-compat/use-router-bfcache-id.spec.tspnpm exec vp test run --project unit tests/shims.test.ts -t "useRouter|hash-only"pnpm exec vp test run --project unit tests/app-browser-entry.test.ts -t "bfcache|history state|version|intercepted slot|redirected traverse"pnpm exec vp check tests/e2e/app-router/nextjs-compat/use-router-bfcache-id.spec.tsPLAYWRIGHT_PROJECT=app-router pnpm run test:e2e tests/e2e/app-router/nextjs-compat/use-router-bfcache-id.spec.ts