fix(app-router): preserve recent segment state with Activity BFCache#1739
fix(app-router): preserve recent segment state with Activity BFCache#1739NathanDrake2406 wants to merge 26 commits into
Conversation
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.
Reviewed the BFCache segment-state work. The core design is sound: separating the render-tree state key (createBfcacheSegmentStateKeyMap / pathname-derived identity) from the userland useRouter().bfcacheId history identity is the right boundary, and it matches the upstream Next.js split between layout-router stateKey and bfcache-state-manager. The updateBfcacheSlotEntries LRU/eviction logic is clean and well unit-tested, and the e2e fixture coverage (browser back/forward, link return, 3-entry bound, nested layout+page) maps closely to the upstream back-forward-cache.test.ts.
I verified a few things that are NOT problems and want to record them so the reviewer doesn't have to re-derive:
- SSR/hydration convergence is safe. During SSR and the first client render, the
useStateinitializer already produces the correct single-entry order, sohaveSameBfcacheSlotEntryOrderreturns true and the render-phasesetEntriesdoes not fire. No SSR setState warning, no hydration drift. - Define parity across environments.
process.env.__NEXT_CACHE_COMPONENTSis added to the top-leveldefinemap (index.ts:1806), so it is inlined identically in client, SSR, and RSC bundles. There's no risk of one environment wrapping inReact.Activitywhile another doesn't. React.Activityis a stable export in the pinned React 19.2.7, so the bareReact.Activityusage is fine.- The
latestEntriesByStateKeyref does not leak — it is pruned to the current entry keys whenever the order changes, and otherwise only overwrites the active key in place.
Comments below are minor (a doc inconsistency and one behavioral note for reviewer awareness). Nothing blocking from my read.
| // See: https://github.com/vercel/next.js/pull/93997 | ||
| defines["process.env.__NEXT_APP_SHELLS"] = JSON.stringify(nextConfig.appShells); | ||
| // Cache Components — Next.js gates segment Activity BFCache retention | ||
| // on this build-time flag. The active segment is still keyed without |
There was a problem hiding this comment.
Minor doc inconsistency. This comment says "The active segment is still keyed without the flag", but the corresponding logic in slot.tsx (the !process.env.__NEXT_CACHE_COMPONENTS branch) deliberately renders the active entry unkeyed — the long comment there explains that keying it by the pathname-derived stateKey would remount shared layouts on every URL change. Suggest aligning the wording so the two comments don't contradict each other:
| // on this build-time flag. The active segment is still keyed without | |
| // Cache Components — Next.js gates segment Activity BFCache retention | |
| // on this build-time flag. Without the flag the active segment renders | |
| // in place (unkeyed); enabling it turns on the three-entry inactive | |
| // Activity tree cache. | |
| // See: packages/next/src/client/components/layout-router.tsx |
|
|
||
| if (!haveSameBfcacheSlotEntryOrder(entries, orderedEntries)) { | ||
| pruneLatestBfcacheSlotEntries(latestEntriesByStateKey.current, orderedEntries); | ||
| setEntries(orderedEntries); |
There was a problem hiding this comment.
Behavioral note for reviewers (not a defect): before this PR BfcacheSlotBoundary was a trivial provider. It now performs a render-phase setEntries on any navigation where activeStateKey changes — and this runs for every App Router app that has BfcacheIdMapContext, not just cacheComponents apps. The update converges (the next render produces the same key order, so no second setEntries), and the no-cacheComponents path still renders unkeyed so it shouldn't remount. Worth a quick sanity pass that the existing use-router-bfcache-id / app-router e2e projects stay green, since this widens the blast radius beyond the new fixture.
| segmentId: id, | ||
| stateKey: activeStateKey, | ||
| stateKeyMap, | ||
| }); |
There was a problem hiding this comment.
This ref write happens on every render before the setEntries/order check. For inactive (hidden) entries the boundary only re-renders with the active segment's content, so their captured content/elements stay frozen at last-visit values — which is the intended BFCache snapshot behavior. No change requested; just confirming the snapshot semantics are deliberate, since it's the crux of why returning to a cached segment restores prior client state.
|
Review submitted successfully. Here's a summary of my review of PR #1739. Review summaryThe PR implements App Router BFCache segment-state retention with Verdict: Sound, well-tested, nothing blocking. What's good
Things I verified are safe (recorded in the review)
Comments posted (all minor)
No |
|
/bigbonk review |
There was a problem hiding this comment.
Reviewed the BFCache segment-state retention work. The core design is sound and matches the upstream Next.js split: pathname-derived render-tree state keys (createBfcacheSegmentStateKeyMap) are kept separate from the userland useRouter().bfcacheId history identity, mirroring Next.js layout-router stateKey vs bfcache-state-manager. The updateBfcacheSlotEntryOrder LRU/eviction is clean and well unit-tested, and the e2e fixtures map closely to the upstream back-forward-cache.test.ts.
Things I verified are NOT problems:
- SSR/hydration convergence is safe. During SSR and the first client render the
useStateinitializer already yields the single-entry order, sohaveSameBfcacheSlotEntryOrderis true and the render-phasesetEntryOrdernever fires — no SSR setState warning, no hydration drift. Activity wraps the single entry invisiblemode, matching the client first render. renderEntries[0]is always defined in the non-cacheComponents path: the active key is set in the snapshot map every render and is always first inrenderOrder.React.Activityis a stable export in the pinned React 19.2.7 (typeof React.Activity === 'symbol').defineparity:__NEXT_CACHE_COMPONENTSis in the top-leveldefinemap, inlined identically across client/SSR/RSC, so no environment can disagree on the Activity path.
Nothing blocking. A couple of notes below for reviewer awareness, the main one being that this widens the per-render work for every app-router app, not just cacheComponents apps.
| // state that survives a normal navigation. Reset for genuinely fresh entries is | ||
| // driven by userland bfcacheId keying, not by remounting the slot subtree, so | ||
| // the active entry renders unkeyed here. | ||
| if (!process.env.__NEXT_CACHE_COMPONENTS) { |
There was a problem hiding this comment.
Behavioral-widening note (not a defect, worth a sanity check). Before this PR the non-cacheComponents path was just <SegmentContext.Provider value={id}>{content}</SegmentContext.Provider>. Now every app-router app that has BfcacheIdMapContext runs useBfcacheSlotEntries (with a render-phase setEntryOrder on each navigation where activeStateKey changes) and re-wraps content in three redundant providers (BfcacheStateKeyMapContext, ElementsContext, SegmentContext) whose fallback values equal the live context values.
The update converges (next render produces the same key order, so no second setEntryOrder) and the path renders unkeyed so it shouldn't remount, but this is extra per-render work and an extra setState on the hot common path for all app-router apps. Please confirm the existing app-router and use-router-bfcache-id e2e projects stay green — the new fixture only exercises the cacheComponents: true branch, so the non-flag regression surface is not directly covered by the added tests.
There was a problem hiding this comment.
Valid. I split the Activity-retention path into BfcacheActivitySlotBoundary, so non-cacheComponents apps now return the baseline <SegmentContext.Provider value={id}>{content}</SegmentContext.Provider> path and no longer run useBfcacheSlotEntries, render-phase setEntryOrder, or the extra provider stack. Verified with vp check, vp test run tests/shims.test.ts, PLAYWRIGHT_PROJECT=app-router pnpm exec playwright test tests/e2e/app-router/nextjs-compat/use-router-bfcache-id.spec.ts --retries=0 (11/11), and PLAYWRIGHT_PROJECT=app-router-bfcache pnpm exec playwright test tests/e2e/app-router-bfcache/back-forward-cache.spec.ts --retries=0 (6/6).
| // in place (unkeyed); enabling it turns on the three-entry inactive | ||
| // Activity tree cache. | ||
| // See: packages/next/src/client/components/layout-router.tsx | ||
| defines["process.env.__NEXT_CACHE_COMPONENTS"] = JSON.stringify(nextConfig.cacheComponents); |
There was a problem hiding this comment.
nextConfig.cacheComponents is optional, so when unset JSON.stringify(undefined) returns the JS value undefined (not a string), and defines["..."] is set to undefined. This matches the adjacent __NEXT_APP_SHELLS pattern so it's consistent, and Vite inlines the falsy/undefined replacement fine. Still, an explicit boolean reads more clearly and avoids relying on JSON.stringify(undefined) semantics:
| defines["process.env.__NEXT_CACHE_COMPONENTS"] = JSON.stringify(nextConfig.cacheComponents); | |
| defines["process.env.__NEXT_CACHE_COMPONENTS"] = JSON.stringify(nextConfig.cacheComponents ?? false); |
Non-blocking — only worth it if you want the define map to always carry a concrete value.
There was a problem hiding this comment.
Valid, with one extra guard needed. I applied the explicit ?? false define, but also changed the consumer to gate on the boolean define value via isCacheComponentsEnabled(). That avoids the unsafe truthiness shape where a concrete false define could accidentally behave as enabled, and keeps the non-flag path disabled while preserving the cacheComponents: true Activity path. Verified by rebuilding with vp run vinext#build and rerunning the app-router/app-router-bfcache e2e targets.
| const snapshotsByStateKey = React.useRef(new Map<string, BfcacheSlotEntry>()); | ||
| const [entryOrder, setEntryOrder] = React.useState<string[]>(() => [activeEntry.stateKey]); | ||
|
|
||
| snapshotsByStateKey.current.set(activeEntry.stateKey, activeEntry); |
There was a problem hiding this comment.
Confirming the snapshot semantics are deliberate (no change requested): the ref write captures the active entry's content/elements/stateKeyMap each render, and hidden (inactive) entries keep their last-visit snapshot frozen because the boundary only re-renders with the active segment's content. That frozen snapshot is exactly what makes returning to a cached segment restore prior client state, so this is the crux of the feature working. Pruning to nextOrder on order change keeps the ref bounded to the retained set.
|
Review submitted successfully. Here's a summary of my review of PR #1739. Review summaryThe PR implements App Router BFCache segment-state retention using Verdict: Sound design, well-tested, nothing blocking. What's good
Things I verified are safe (recorded in the review)
Comments posted (all minor / non-blocking)
No file edits or git write operations were performed; review-only pass on PR #1739. |
|
/bigbonk review |
There was a problem hiding this comment.
Reviewed the App Router BFCache segment-state retention work. The design is sound and faithful to Next.js: createBfcacheSegmentStateKeyMap derives a pathname-based render-tree state key (mirroring createRouterCacheKey(activeSegment, true) in layout-router.tsx), kept separate from the userland useRouter().bfcacheId history identity, and updateBfcacheSlotEntryOrder reimplements the most-recently-active LRU ordering from useRouterBFCache (bfcache-state-manager.ts) including the move-to-front-without-eviction fast path. The React.Activity visible/hidden wrapping matches upstream OuterLayoutRouter.
I verified locally:
vp test run tests/shims.test.ts -t "slot BFCache entry ordering"(5 pass) and the newapp-browser-entrystate-key test pass.vp checkis clean on all four changed source files (format, lint, types).React.Activityis a stable export in the pinned React 19.2.7.- The
useStateinitializer converges to a single-entry order, so the render-phasesetEntryOrdernever fires during SSR or first client render (no setState warning / no hydration drift), andisCacheComponentsEnabled()defaults safely tofalsewhen the define is absent.
Nothing blocking. A few non-blocking observations below for awareness.
| const snapshotsByStateKey = React.useRef(new Map<string, BfcacheSlotEntry>()); | ||
| const [entryOrder, setEntryOrder] = React.useState<string[]>(() => [activeEntry.stateKey]); | ||
|
|
||
| snapshotsByStateKey.current.set(activeEntry.stateKey, activeEntry); |
There was a problem hiding this comment.
Non-blocking, concurrency note. This mutates a ref Map during render (and pruneBfcacheSlotEntrySnapshots mutates it again on order change), which diverges from upstream useRouterBFCache, which is pure copy-on-write over React state and never touches a ref during render. Under StrictMode dev double-invocation or a concurrent render that React discards before commit, this set runs for a render that may be thrown away.
It looks self-healing in practice: the active-key snapshot is overwritten every render so the last committed render wins, and the map is re-pruned to the retained order on the next order change, so it stays bounded. Worth a one-line comment documenting that the ref mutation is intentionally tolerant of discarded/StrictMode renders, since a future reader may otherwise flag it as an impure-render violation.
| { | ||
| value: createBfcacheSegmentStateKeyMap({ | ||
| elements, | ||
| pathname: navContext?.pathname ?? "/", |
There was a problem hiding this comment.
Non-blocking, hydration-parity note. SSR derives the state-key map from navContext?.pathname ?? "/", while the client (app-browser-entry.ts) derives it from treeState.navigationSnapshot.pathname (ultimately window.location.href). Page state keys are ${id}@${pathname}, and they become the React.Activity key, so if these two pathname sources ever diverge for the same request (trailing-slash normalization, basePath stripping, encoded segments), the SSR'd Activity key would not match the hydration key.
The tested routes align so this is fine today, but it relies on the two pathname derivations staying byte-identical. Might be worth asserting/normalizing both through the same helper, or at least a comment noting the invariant.
| const SegmentContext = BfcacheSegmentIdContext; | ||
| const elements = React.useContext(ElementsContext); | ||
| const stateKeyMap = React.useContext(BfcacheStateKeyMapContext); | ||
| const activeStateKey = stateKeyMap[id]; |
There was a problem hiding this comment.
Non-blocking. activeStateKey is read as stateKeyMap[id] and used as a bare object index. When id is not present the value is undefined and the === undefined guard below handles it correctly, but consider documenting that the empty-map default (EMPTY_BFCACHE_STATE_KEYS) plus the undefined guard is what keeps apps that never provide the context on the original unkeyed path. It's correct, just implicit.
| </div> | ||
| <div> | ||
| <input id={`uncontrolled-input-${n}`} type="text" /> | ||
| </div> |
There was a problem hiding this comment.
The fixture ships search-param affordances (?param=true links in the layout, uncontrolled-input-*, has-search-param-*) ported from upstream, but the new back-forward-cache.spec.ts never exercises them — the PR lists the search-param BFCache case as a non-goal. That's fine, but the unused fixture surface may read as dead code to a future maintainer. Either add a short comment that these are reserved for the not-yet-ported search-param case, or trim them until that test lands.
|
Review submitted successfully. Here's a summary of my review of PR #1739. Review summaryThe PR adds App Router BFCache segment-state retention using Verdict: Sound, faithful to Next.js, well-tested — nothing blocking. What I verified
Comments posted (all non-blocking)
Review-only pass; no file edits or git write operations performed. |
|
/bigbonk review |
There was a problem hiding this comment.
Reviewed the BFCache segment-state retention work after the latest revision. The design is sound and faithful to Next.js, the prior review feedback has been addressed, and I verified the key behaviors locally.
What I verified
vp checkis clean on all four changed source files (format, lint, types).- Unit tests pass:
tests/shims.test.ts -t "slot BFCache entry ordering"(5 pass) and the newapp-browser-entrypage-segment-state-key test (1 pass). React.Activityis a stable export in the pinned React 19.2.7 (typeof React.Activity === "symbol").- Design parity:
createBfcacheSegmentStateKeyMapderives a pathname-based render-tree state key, andupdateBfcacheSlotEntryOrderreimplements the move-to-front LRU (incl. the no-eviction fast path) frombfcache-state-manager.React.Activityvisible/hidden wrapping matchesOuterLayoutRouter. - The identity split is correct and well-tested: the
BfcacheIdMapContext.Providersits aboveBfcacheActivitySlotBoundaryin bothBrowserRootandhandleSsr, so returning to a cached segment preservesuseState(via Activity) while the leaf still re-renders a freshbfcacheIdfrom the context above the Activity boundary. Thekeeps Activity state separate from fresh bfcacheId identitye2e covers exactly this. - Provider nesting matches between SSR and browser (BfcacheIdMap > StateKeyMap > routeTree), so hydration keys line up.
- Behavioral widening from the earlier review is resolved: splitting
BfcacheActivitySlotBoundaryout means non-cacheComponentsapps return the baseline<SegmentContext.Provider value={id}>{content}</SegmentContext.Provider>path with nouseBfcacheSlotEntries/ render-phasesetEntryOrder/ extra provider stack. The remaining per-navigation cost for all app-router apps is thecreateBfcacheSegmentStateKeyMapuseMemoinBrowserRoot, which is bounded. defineis consistent across environments (top-leveldefinemap at index.ts:1837) and the?? false+isCacheComponentsEnabled()string-compare guard is deliberate and safe.
Notes (all non-blocking)
Nothing blocking. A couple of minor observations inline.
| // Activity tree cache. | ||
| // See: packages/next/src/client/components/layout-router.tsx | ||
| defines["process.env.__NEXT_CACHE_COMPONENTS"] = JSON.stringify( | ||
| String(nextConfig.cacheComponents ?? false), |
There was a problem hiding this comment.
Minor: String(nextConfig.cacheComponents ?? false) is intentional (the consumer does a === "true" string compare via isCacheComponentsEnabled(), so a string-typed define is required), but it reads as a double-wrap next to the boolean JSON.stringify(nextConfig.appShells) pattern right above it. Consider a one-line comment noting that this define is deliberately a string ("true"/"false") so a future reader doesn't "simplify" it to JSON.stringify(nextConfig.cacheComponents ?? false) and silently break the runtime check (which would then compare the boolean true against the string "true").
| return ( | ||
| <> | ||
| {renderEntries.map((entry) => ( | ||
| <React.Activity |
There was a problem hiding this comment.
Behavioral note (not a defect): retained hidden entries keep their full DOM mounted, so duplicate element ids exist while an inactive entry is cached (e.g. #counter-display-1 present for both the visible and a hidden page). The e2e locators correctly :visible-scope around this, and it matches Next.js Activity behavior, but it does mean document.getElementById / global id-based queries in userland code can resolve to a hidden snapshot under cacheComponents. Worth being aware of when documenting the cacheComponents opt-in surface.
|
Review submitted successfully. Here's a summary of my review of PR #1739. Review summaryThe PR implements App Router BFCache segment-state retention using Verdict: Sound, faithful to Next.js, well-tested — nothing blocking. What I verified locally
Comments posted (both non-blocking)
Review-only pass; no file edits or git write operations performed. |
|
/bigbonk review |
There was a problem hiding this comment.
Reviewed the App Router BFCache segment-state retention work. The design is sound and faithful to Next.js: the pathname-derived render-tree state key (createBfcacheSegmentStateKeyMap, reusing the existing createBfcacheSegmentIdentity helper) is correctly kept separate from the userland useRouter().bfcacheId history identity, mirroring the upstream layout-router stateKey / bfcache-state-manager split. The updateBfcacheSlotEntryOrder LRU/eviction is clean and well unit-tested, and the new e2e fixtures map closely to upstream back-forward-cache.test.ts.
What I verified locally
vp checkis clean on all five changed source files (format, lint, types).- Unit tests pass:
tests/shims.test.ts -t "slot BFCache entry ordering"(5), the newapp-browser-entrypage-segment-state-key test (1), and the fulltests/app-page-render.test.tssuite (41) — which still passes despite a test helper usinggetNavigationContext: () => null(it mocksloadSsrHandler, so it never reaches the realhandleSsr). - The define is consistent:
__NEXT_CACHE_COMPONENTSlands in the top-leveldefinemap as the JSON-encoded string"true"/"false", andisCacheComponentsEnabled()does a=== "true"compare, so the inlined replacement matches across client/SSR/RSC.ResolvedNextConfig.cacheComponentsis already a guaranteed boolean (normalized inconfig/next-config.ts), so the?? falseis redundant-but-harmless. - The non-
cacheComponentspath is correctly preserved as the baseline unkeyed<SegmentContext.Provider>— the prior behavioral-widening concern is resolved by theBfcacheActivitySlotBoundarysplit. - The new
app-router-bfcachePlaywright project does not collide with theapp-routerproject glob (**/app-router/**/*.spec.tsdoes not match theapp-router-bfcache/directory), and the de-dupwebServermap picks up the new server.
The one change worth a second look
The handleSsr signature change from navContext?.pathname ?? "/" to a hard requireNavigationContext() throw is the only behavioral hardening that converts a previously-degraded-but-rendering path into a 500. I traced the production callers and it appears safe: app-rsc-handler.ts:534 sets navigation context (with a real searchParams) before every page/not-found render, and handleSsr is HTML-only (RSC requests return the flight stream directly). But it does narrow the contract, so the inline notes below are worth confirming.
Nothing blocking.
| pathname: navContext?.pathname ?? "/", | ||
| searchParams: navContext?.searchParams ? [...navContext.searchParams.entries()] : [], | ||
| pathname: navContext.pathname, | ||
| searchParams: [...navContext.searchParams.entries()], |
There was a problem hiding this comment.
This now reads navContext.searchParams.entries() without the previous navContext?.searchParams ? guard. It's safe given requireNavigationContext() guarantees navContext is non-null, but it also now assumes searchParams is always present on the context. Production callers do set it (app-rsc-handler.ts passes url.searchParams, dispatch passes new URLSearchParams()), but several test nav contexts are shaped as { pathname } only (e.g. app-page-boundary-render.test.ts returns { pathname: "/posts/missing" }). Those tests mock handleSsr so they don't hit this line today — just flagging that the NavigationContext type should make searchParams non-optional if this assumption is now load-bearing, otherwise a future test/caller passing a partial context would throw here rather than degrade.
|
|
||
| function requireNavigationContext(navContext: NavigationContext | null): NavigationContext { | ||
| if (!navContext) { | ||
| throw new Error("App SSR requires navigation context for BFCache state keys"); |
There was a problem hiding this comment.
Behavioral hardening note (not blocking): previously a null navContext degraded gracefully (navContext?.pathname ?? "/"); now it throws a hard error, surfacing as a 500. I traced the production paths and setNavigationContext is called in app-rsc-handler.ts (line ~534) before every render including the route-miss not-found branch, so this should be unreachable in prod. The throw is reasonable as a defensive invariant, but since it widens the failure mode, it would be worth a one-line comment here pointing at where the context is guaranteed to be set (the RSC handler), so a future change that adds a new handleSsr entry path knows the contract.
|
|
||
| return renderOrder | ||
| .map((stateKey) => snapshotsByStateKey.current.get(stateKey)) | ||
| .filter((entry): entry is BfcacheSlotEntry => entry !== undefined); |
There was a problem hiding this comment.
Minor robustness note: renderOrder keys whose snapshot was never captured are silently dropped by the .filter(... !== undefined). In normal operation every key in renderOrder has a snapshot (the active key is set this render; inactive keys retain their last-active snapshot and are pruned together with the order), so this can't drop the active entry. It's correct, but the filter quietly masks an invariant violation if it ever happened. Consider asserting (dev-only) that the active stateKey is always present, since dropping it would render nothing for the slot without any signal.
|
Review submitted successfully. Here's a summary of my review of PR #1739. Review summaryThe PR implements App Router BFCache segment-state retention using Verdict: Sound, faithful to Next.js, well-tested — nothing blocking. What I verified independently
Comments posted (all non-blocking)
Review-only pass; no file edits or git write operations performed. The earlier multi-round review by ask-bonk was thorough; my pass focused on independently verifying the |
|
/bigbonk review |
There was a problem hiding this comment.
Independent review of the App Router BFCache segment-state retention work.
Verdict: Sound, faithful to Next.js, well-tested — nothing blocking.
The design correctly separates the pathname-derived render-tree state key (createBfcacheSegmentStateKeyMap, reusing createBfcacheSegmentIdentity) from the userland useRouter().bfcacheId history identity, mirroring the upstream layout-router stateKey / bfcache-state-manager split. The BfcacheActivitySlotBoundary split correctly keeps non-cacheComponents apps on the baseline unkeyed path, resolving the behavioral-widening concern from earlier rounds.
What I verified independently
requireNavigationContext()hardening is safe on all production paths. I traced every caller that reacheshandleSsr:app-rsc-handler.ts:534sets a full context (pathname/searchParams/params) before the main render.app-page-dispatch.ts:422and:506set it before the ISR/revalidation render that callsrevalidatedSsrEntry.handleSsr(:595) — the revalidation path was the one I was least sure about, and it is covered.app-page-boundary-render.ts:275synthesizes a context viagetNavigationContext() ?? { pathname, searchParams, params }so the fallback boundary never passes null.
So the previous degrade-to-/path is genuinely unreachable in prod; the throw is a defensive invariant, not a new failure mode.
searchParamsis already non-optional onNavigationContext(navigation.ts:187), so the removal of thenavContext?.searchParams ?guard inbuildHeadInjectionHtmlis type-safe — the earlier suggestion to make it non-optional is already satisfied.- SSR/browser provider nesting matches (
BfcacheIdMap > BfcacheStateKeyMap > routeTreein bothhandleSsrandBrowserRoot), so hydration keys line up. - Unit tests pass:
slot BFCache entry ordering(5). - CI matrix correctly adds the
app-router-bfcacheproject, and the Playwrightapp-router-bfcache/dir does not collide with theapp-router/glob.
Two minor, non-blocking observations inline.
| elements, | ||
| // Must match the client navigation snapshot pathname byte-for-byte; | ||
| // Activity keys are derived from this value during hydration. | ||
| pathname: ssrNavigationContext.pathname, |
There was a problem hiding this comment.
Non-blocking hydration-parity note. The SSR Activity state keys are derived from ssrNavigationContext.pathname, while the client derives them from treeState.navigationSnapshot.pathname (app-browser-entry.ts:1174). Since these become the React.Activity key, the two pathname sources must stay byte-identical or the SSR'd hidden-entry key won't match the hydrated one (trailing-slash normalization, basePath stripping, encoded segments are the usual divergence points). The comment here documents the invariant, which is good — but consider routing both through the same canonicalization helper so the invariant can't silently drift. The currently-tested routes align, so this is a robustness note, not a defect.
There was a problem hiding this comment.
Normalized inside createBfcacheSegmentStateKeyMap (commit fa29e20) so both SSR and client callers produce byte-identical keys automatically — decodeURIComponent + trailing-slash stripping covers the common divergence cases.
| if (!navContext) { | ||
| // Real request rendering must pass the RSC handler's navigation context; | ||
| // fallback boundary renderers synthesize one when request scope is gone. | ||
| throw new Error("App SSR requires navigation context for BFCache state keys"); |
There was a problem hiding this comment.
Non-blocking: this requireNavigationContext() throw makes the handleSsr navigation context load-bearing (null now 500s instead of degrading to /). I traced all production callers and it is safe — app-rsc-handler.ts:534, app-page-dispatch.ts:422/:506, and the app-page-boundary-render.ts:275 synthesized fallback all seed a full context before any handleSsr call, including the ISR/revalidation render at app-page-dispatch.ts:595. The comment here is good; consider also referencing the RSC handler as the guaranteeing caller so a future handleSsr entry path knows the contract it must uphold.
There was a problem hiding this comment.
Updated the comment (commit fa29e20) to reference the specific guaranteeing callers: app-rsc-handler.ts (main render), app-page-dispatch.ts (ISR/revalidation), and app-page-boundary-render.ts (fallback synthesis).
|
Review submitted successfully. Review summaryI performed an independent review of PR #1739 (App Router BFCache segment-state retention with Verdict: Sound, faithful to Next.js, well-tested — nothing blocking. What I verified independently (beyond the 5 prior rounds)The main thing prior reviewers flagged but hadn't fully traced was the
So the throw is a safe defensive invariant, not a new failure mode. I also confirmed:
Comments posted (both non-blocking)
Review-only pass; no file edits or git write operations performed. |
|
/bigbonk review for issues |
u should try the keyword "adversarial review". I've been using it and it's peak |
There was a problem hiding this comment.
Reviewed the App Router BFCache segment-state retention work. The design is sound and faithful to Next.js — separating the pathname-derived render-tree state key (createBfcacheSegmentStateKeyMap) from the userland useRouter().bfcacheId history identity mirrors the upstream layout-router stateKey / bfcache-state-manager split, and the BfcacheActivitySlotBoundary split correctly keeps non-cacheComponents apps on the baseline unkeyed path.
I independently verified:
slot BFCache entry orderingunit tests (5) and the newapp-browser-entrypage-segment-state-key test pass viavp test run.vp lintis clean on the changed shim/server files.__NEXT_CACHE_COMPONENTSlands in the shared top-leveldefinemap (applies to dev + build, all three environments), andisCacheComponentsEnabled()does a matching=== "true"string compare. Dev parity holds since thedefineis set in theconfighook.requireNavigationContext()is safe on every productionhandleSsrcaller:app-rsc-handler.ts(main render),app-page-dispatch.ts:422(the ISR/revalidation path that reacheshandleSsrat :595), and theapp-page-boundary-render.ts:275synthesized fallback. The previous null→/degrade is genuinely unreachable in prod.- The Playwright
app-router-bfcacheproject is isolated:**/app-router/**/*.spec.tsdoes not match theapp-router-bfcache/segment, and thewebServerde-dup map picks up the new fixture server.
One genuine (edge-case) defect worth addressing inline; everything else is non-blocking.
| // Decode and strip trailing slash so SSR and client produce byte-identical | ||
| // Activity keys regardless of encoding or trailing-slash normalization | ||
| // differences between the two pathname sources. | ||
| const decoded = decodeURIComponent(pathname); |
There was a problem hiding this comment.
decodeURIComponent throws on malformed percent sequences (e.g. a lone % in a path like /foo%). This runs unguarded during both the SSR flight-root render (app-ssr-entry.ts) and the client useMemo (app-browser-entry.ts), so a throw here crashes rendering for the whole route, not just BFCache retention.
The rest of the codebase treats decodeURIComponent(pathname) as throwable and wraps it — see prod-server.ts:562-566 (try { decodeURIComponent(pathname) } catch { return false }). While URL.pathname normally re-encodes a lone % to %25, navigationSnapshot.pathname / navContext.pathname aren't guaranteed to be URL.pathname-derived, so this should fail soft and fall back to the raw pathname rather than throwing:
| const decoded = decodeURIComponent(pathname); | |
| function normalizeBfcachePathname(pathname: string): string { | |
| // Decode and strip trailing slash so SSR and client produce byte-identical | |
| // Activity keys regardless of encoding or trailing-slash normalization | |
| // differences between the two pathname sources. decodeURIComponent can throw | |
| // on malformed input (a lone `%`), so fall back to the raw pathname rather | |
| // than crashing the render — matching the defensive pattern in prod-server.ts. | |
| let decoded: string; | |
| try { | |
| decoded = decodeURIComponent(pathname); | |
| } catch { | |
| decoded = pathname; | |
| } | |
| return decoded.length > 1 ? decoded.replace(/\/$/, "") : decoded; | |
| } |
There was a problem hiding this comment.
Already guarded with try/catch in the previous commit — normalizeBfcachePathname at line 301-306 falls back to the raw pathname on decode failure.
|
|
||
| function useBfcacheSlotEntries(activeEntry: BfcacheSlotEntry): BfcacheSlotEntry[] { | ||
| const snapshotsByStateKey = React.useRef(new Map<string, BfcacheSlotEntry>()); | ||
| const [entryOrder, setEntryOrder] = React.useState<string[]>(() => [activeEntry.stateKey]); |
There was a problem hiding this comment.
Non-blocking: useBfcacheSlotEntries adjusts state during render (setEntryOrder(nextOrder) at line 253-255) and mutates a ref Map during render (line 238 / pruning). This is the documented "adjusting state when a prop changes" pattern and converges in two renders (the next render sees entryOrder === nextOrder, so orderChanged is false), but it diverges from upstream useRouterBFCache, which is pure copy-on-write over React state. Under StrictMode double-invoke or a discarded concurrent render the ref set runs for a render that may be thrown away. It's self-healing (active key overwritten every render, pruned to render order), so no change is required — but a one-line comment noting the ref mutation is intentionally tolerant of discarded/StrictMode renders would save a future reader from flagging it as an impure-render violation.
There was a problem hiding this comment.
Already present — the comment at line 236-237 documents this: "Render can be restarted or discarded; snapshots are render-tolerant because the active key is overwritten on every render and pruned to render order."
|
|
||
| return ( | ||
| <> | ||
| {renderEntries.map((entry) => ( |
There was a problem hiding this comment.
Behavioral note for the cacheComponents opt-in docs (not a defect): retained hidden Activity entries keep their full DOM mounted, so duplicate userland element ids exist while an inactive entry is cached (e.g. #counter-display-1 present for both a visible and a hidden page). The e2e locators correctly :visible-scope around this, but userland document.getElementById / global id queries can resolve to a hidden snapshot under cacheComponents. Worth calling out when documenting the opt-in surface.
There was a problem hiding this comment.
Already present — the comment in BfcacheActivitySlotBoundary above the React.Activity map documents this: "Hidden Activity entries keep their DOM mounted, so duplicate userland ids can exist... Consumers should query by visible scope when that distinction matters."
|
Review submitted successfully. Here's a summary of my review of PR #1739. Review summaryThe PR implements App Router BFCache segment-state retention with Verdict: Sound, faithful to Next.js, well-tested. One genuine edge-case defect, otherwise non-blocking. What I verified independently
The one finding worth addressing (
|
|
/bigbonk adversarial review |
|
UnknownError: "Overloaded" |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk adversarial review |
There was a problem hiding this comment.
Adversarial review of the App Router BFCache segment-state retention work. I independently re-derived the design against the upstream Next.js v16.2.6 layout-router.tsx + bfcache-state-manager.ts, traced the requireNavigationContext() contract across every production handleSsr caller (including the SSG build-time prerender path, which invokes the RSC handler with a synthetic Request and so goes through app-page-dispatch.ts → setNavigationContext), and ran the unit suite (slot BFCache entry ordering, 5 pass) plus vp check on the four changed source files (clean).
Verdict: Sound and faithful to Next.js where it claims to be. Nothing blocking. Prior rounds already covered the define parity, SSR/hydration convergence, the LRU ordering, and the requireNavigationContext hardening; I confirmed those and won't repeat them. Two findings below the prior rounds did not surface — one is a genuine behavioral-parity divergence worth documenting precisely, the other is a robustness note.
Verified against upstream
createRouterCacheKey(activeSegment, true)in upstream explicitly excludes search params from the state key ("search params do not cause state to be lost"). vinext's page state key${id}@${pathname}likewise omits search params — correct, and consistent with the search-param case being a stated non-goal.- The 3-entry bound under cacheComponents and the move-to-front-without-eviction fast path in
updateBfcacheSlotEntryOrdermirroruseRouterBFCache/MAX_BF_CACHE_ENTRIES. - SSG prerender does not bypass the navigation-context invariant.
| // state that survives a normal navigation. Reset for genuinely fresh entries is | ||
| // driven by userland bfcacheId keying, not by remounting the slot subtree, so | ||
| // the active entry renders unkeyed here. | ||
| if (!isCacheComponentsEnabled()) { |
There was a problem hiding this comment.
Documented behavioral divergence from Next.js — worth making explicit in the comment, not a blocker.
Upstream OuterLayoutRouter keys the active segment's child by stateKey unconditionally via <TemplateContext.Provider key={stateKey}> — the key is present whether or not __NEXT_CACHE_COMPONENTS is set; cacheComponents only adds the extra <Activity> wrapper and raises MAX_BF_CACHE_ENTRIES from 1 to 3. With cacheComponents off, upstream still resets default (non-keyed) client state when re-pushing a route because the prior subtree was never retained (MAX=1), so a fresh push mounts a brand-new instance.
vinext deliberately renders the active segment unkeyed here and instead relies on userland bfcacheId keying (see the use-router-bfcache-id fixture, which keys its leaf by bfcacheId) to drive that reset. The observable result diverges for the default, un-keyed case: a plain useState leaf that upstream resets on a fresh re-push ('resets leaf form state when re-entering a route via fresh push') will persist in vinext without cacheComponents.
The comment correctly explains why keying by the pathname-derived stateKey would over-remount shared layouts, but it presents the unkeyed choice as parity-preserving when it is actually a deliberate parity trade-off. Suggest tightening the wording so a future reader understands this is a known divergence (state reset is opt-in via userland bfcacheId keying here, vs. automatic stateKey remount upstream), e.g. add: // NOTE: This diverges from Next.js, which keys the active child by stateKey even without cacheComponents; vinext defers fresh-entry reset to userland bfcacheId keying. See use-router-bfcache-id fixture.
There was a problem hiding this comment.
Added the diveregence note (commit c1abe1e): "NOTE: This diverges from Next.js, which keys the active child by stateKey even without cacheComponents; vinext defers fresh-entry reset to userland bfcacheId keying. See use-router-bfcache-id fixture."
|
|
||
| function useBfcacheSlotEntries(activeEntry: BfcacheSlotEntry): BfcacheSlotEntry[] { | ||
| const snapshotsByStateKey = React.useRef(new Map<string, BfcacheSlotEntry>()); | ||
| const [entryOrder, setEntryOrder] = React.useState<string[]>(() => [activeEntry.stateKey]); |
There was a problem hiding this comment.
Robustness note (non-blocking). useState(() => [activeEntry.stateKey]) seeds the order from the first active key, but snapshotsByStateKey is a useRef(new Map()). If activeEntry.stateKey changes between mount and the next render (normal — that's the whole point), the render-phase setEntryOrder plus the ref write converge in two renders. The dev-only invariant throw at line 250 guards the active key, which is good.
The one thing not covered by the new fixture: the non-cacheComponents path never enters this hook at all (it returns early at the unkeyed provider), so this hook is only exercised under cacheComponents: true. That's fine, but it means the MAX_BFCACHE_SLOT_ENTRIES_WITHOUT_CACHE_COMPONENTS = 1 limit branch of getBfcacheSlotEntryLimit() is effectively dead in the React path — it's only reachable via the directly-unit-tested updateBfcacheSlotEntryOrder(..., 1). Worth a one-line comment that the WITHOUT_CACHE_COMPONENTS limit exists for the unit-tested helper contract and the future non-flag-keyed path, not the current render path, so a reader doesn't assume the limit-1 branch runs in BfcacheActivitySlotBoundary.
There was a problem hiding this comment.
Added a comment (commit c1abe1e) explaining that MAX_BFCACHE_SLOT_ENTRIES_WITHOUT_CACHE_COMPONENTS = 1 serves the directly-unit-tested helper contract and a future non-flag-keyed path, not the current render path which only reaches BfcacheActivitySlotBoundary under cacheComponents.
| @@ -322,6 +333,18 @@ export async function handleSsr( | |||
| { value: elements }, | |||
There was a problem hiding this comment.
Confirmation, no change requested. The SSR tree now nests BfcacheIdMapContext.Provider > BfcacheStateKeyMapContext.Provider(stateKeyTree) > routeTree, matching app-browser-entry.ts (BfcacheIdMapContext > stateKeyTree > routeTree). Under cacheComponents, BfcacheSlotBoundary will render React.Activity during SSR as well (the define is shared across environments), but with a single entry in visible mode, so the server markup is a single visible Activity that hydrates against the client's single-entry first render. Verified the provider order and the single-entry SSR convergence; the passing e2e (waitForAppRouterHydration) empirically confirms no hydration mismatch from the Activity wrapper. Flagging only so the SSR-renders-Activity-too fact is recorded — it's easy to assume Activity is client-only.
There was a problem hiding this comment.
Confirmed. The define parity across environments is verified — __NEXT_CACHE_COMPONENTS is in the top-level define map so both SSR and client bundles inline the same value, and isCacheComponentsEnabled() does a === "true" string compare. The single-entry SSR Activity hydrates against the client single-entry first render with no mismatch (confirmed by e2e waitForAppRouterHydration).
…oject The header noted that Activity-backed preservation is covered in back-forward-cache.spec.ts, but that spec moved to the dedicated app-bfcache fixture under the app-router-bfcache project. Name the project and path so the cross-reference stays accurate.
Fallback boundary rendering can outlive the original request-scoped navigation context. That made app SSR fail fast with a missing navigation context when loading-boundary digest routes rendered their HTTP fallback shell. The SSR boundary contract still requires a navigation context for real request renders, but boundary renderers now synthesize the same typed shape from the request URL and matched params when request scope has already been cleared.
…riant callers Moves hydration-pathname normalization into createBfcacheSegmentStateKeyMap so SSR and client always produce byte-identical Activity keys. Documents the requireNavigationContext guaranteeing callers per review feedback.
NathanDrake2406
force-pushed
the
nathan/issue-1366-bfcache-restore
branch
from
0fae761 to
cd5bb26
Compare
|
/bigbonk re-review for issues |
|
/bigbonk review this pr for issues |
…an/issue-1366-bfcache-restore # Conflicts: # .github/workflows/ci.yml
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
There was a problem hiding this comment.
Independent review of the App Router BFCache segment-state retention work. I re-derived the design from scratch rather than re-checking prior rounds, ran the unit suite, and type/lint-checked the changed source files.
Verdict: Sound, faithful to Next.js, well-tested. Nothing blocking — only minor notes below.
What I verified independently
- Unit tests pass:
slot BFCache entry ordering(5) viavp test run;vp checkis clean onslot.tsx,app-browser-state.ts,app-ssr-entry.ts,app-browser-entry.ts. React.Activityis asymbolexport in the pinned React 19.2.7.- Eviction parity: traced the e2e "three most recent" scenario (1→2→3→4→2→1) against
updateBfcacheSlotEntryOrder— order resolves to[1,2,4], so page 1 is correctly a fresh entry (Count:0) while page 2 is retained. The move-to-front-no-eviction fast path matches the unit tests. - SSR/hydration convergence: both SSR (
app-ssr-entry.ts:343) and client (app-browser-entry.ts:1188) feedcreateBfcacheSegmentStateKeyMapthrough the samenormalizeBfcachePathname, so Activity keys are byte-identical. SSR renders a single visible Activity entry; theuseStateinitializer seeds the same single-entry order on first client render — no setState-in-render firing, no hydration drift. - Hook ordering is safe:
isCacheComponentsEnabled()is a build-timedefineconstant, so theBfcacheSlotBoundarybranch intoBfcacheActivitySlotBoundaryis stable per build; the newuseMemoinBrowserRootsits after the existing conditionalthrow/useflow with no intervening early return. defineshape:JSON.stringify(String(cacheComponents ?? false))yields the JSON string"true"/"false", matching the=== "true"guard.ResolvedNextConfig.cacheComponentsis already normalized to a boolean (config/next-config.ts:1423), so the?? falseis redundant-but-harmless.- Playwright isolation: the
app-routerglob**/app-router/**/*.spec.tsdoes not match theapp-router-bfcache/segment, and the CI matrix adds the new project. requireNavigationContext()hardening:searchParamsis non-optional onNavigationContext(navigation.ts:187), so the dropped?.guards inbuildHeadInjectionHtmlare type-safe; the synthesized fallback inapp-page-boundary-render.ts:276keeps the throw unreachable on the boundary path.
Notes (all non-blocking)
- See inline comments on
slot.tsxandapp-browser-state.ts. - Stale author reply: the malformed-percent concern from a prior round is genuinely resolved —
normalizeBfcachePathnameroutes throughdecodeRouteSegment, which already try/catches per segment — but the author's reply citing a try/catch "at line 301-306" is inaccurate; the safety lives in the normalizer, not a local catch (see inline comment). - Pre-existing
innerTreequirk (not introduced here, just outside the changed hunk atapp-browser-entry.ts:1201-1203): theinnerTreeternary falls back tobfcacheTreein the no-AppRouterContextbranch while the truthy branch wrapsredirectedTree(withAppRouterRedirectBridge). WhenAppRouterContextis absent the redirect bridge is silently dropped.AppRouterContextis effectively always present so this is latent, but since this PR is already reshaping the surrounding provider stack, falling back toredirectedTreefor consistency would be a cheap correctness fix.
| !snapshotsByStateKey.current.has(activeEntry.stateKey) | ||
| ) { | ||
| throw new Error("BFCache Activity slot is missing the active entry snapshot"); | ||
| } |
There was a problem hiding this comment.
Minor: this dev-only invariant throw is effectively dead. activeEntry.stateKey is .set() into the map at line 248, and renderOrder always begins with activeStateKey (updateBfcacheSlotEntryOrder unconditionally puts it first), so pruneBfcacheSlotEntrySnapshots(map, renderOrder) can never delete the active key. The guard is harmless defensive code, but a reader may wonder how it could ever fire — a one-line comment noting it guards against a future renderOrder that drops the active key would help.
| // Use the route-match normalizer so decoded delimiters like %2F remain data | ||
| // inside their segment instead of becoming structural path separators. | ||
| const normalized = normalizePath(normalizePathnameForRouteMatch(pathname)); | ||
| return normalized.length > 1 ? normalized.replace(/\/$/, "") : normalized; |
There was a problem hiding this comment.
Confirming this is safe (no change requested): normalizeBfcachePathname no longer calls decodeURIComponent directly — it routes through normalizePathnameForRouteMatch → decodeRouteSegment, which try/catches per segment and falls back to the raw segment on malformed percent input. The falls back to raw pathname for malformed encoded state-key paths unit test (/page/% → ${id}@/page/%) covers this. Note for the record: the author's review reply citing a try/catch "at line 301-306" is stale — the actual safety lives in decodeRouteSegment, not in this function.
|
Review submitted successfully. Review summary — PR #1739I performed an independent review of the App Router BFCache segment-state retention work, re-deriving the design rather than re-checking the prior rounds. Verdict: Sound, faithful to Next.js, well-tested. Nothing blocking. What I verified independently
Findings posted (all non-blocking)
No file edits or git write operations were performed; this was a review-only pass. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ 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". |
3 participants
Overview
React.ActivitywhencacheComponentsis enabled.useRouter().bfcacheIdhistory-entry identity.packages/vinext/src/shims/slot.tsx,packages/vinext/src/server/app-browser-state.ts, browser and SSR app entries.Why
App Router correctness needs two different identities to stay separate: the render tree state key for a segment, and the history-entry
bfcacheIdexposed to userland. Next.js keys retained rendered entries by segment identity/pathname, then uses a separate BFCache state manager to bound inactive entries. Vinext already had the history identity, but not the segment state cache, so fresh navigations could reuse the wrong React tree./page/1and/page/2must have distinct render state.createBfcacheSegmentStateKeyMap()and provides it to slot boundaries.cacheComponentsis enabled.React.Activitywith visible/hidden modes and a bounded entry list.useRouter().bfcacheIdBfcacheIdMapContextplumbing and layers state keys separately.What changed
/page/1to/page/2cacheComponentsbfcacheIdbehaviorMaintainer Review Path
packages/vinext/src/server/app-browser-state.ts: verify state keys reuse the existing segment identity helper and pathname metadata without changing historybfcacheIdgeneration.packages/vinext/src/shims/slot.tsx: verify the entry ordering, bounded retention,React.Activityrendering, and the unkeyed non-cacheComponentsactive-segment path.packages/vinext/src/server/app-browser-entry.tsandpackages/vinext/src/server/app-ssr-entry.ts: verify provider placement keeps SSR and hydration aligned.packages/vinext/src/index.ts: verify the Next.jscacheComponentscompile-time flag is exposed consistently.tests/e2e/app-router/nextjs-compat/back-forward-cache.spec.ts: compare the ported regression scenarios against the upstream test.Validation
vp test run --project unit tests/shims.test.ts --testNamePattern "slot BFCache entry state"vp test run --project unit tests/app-browser-entry.test.ts --testNamePattern "app browser entry bfcacheId helpers"PLAYWRIGHT_PROJECT=app-router pnpm run test:e2e -- tests/e2e/app-router/nextjs-compat/back-forward-cache.spec.ts --retries=0PLAYWRIGHT_PROJECT=app-router pnpm run test:e2e -- tests/e2e/app-router/nextjs-compat/use-router-bfcache-id.spec.ts --retries=0vp checkvp env exec --node 24 ./scripts/run-nextjs-deploy-suite.sh /Users/nathan/Projects/vinext/.refs/nextjs-v16.2.6 --retries 0 -c 1 --debug test/e2e/app-dir/back-forward-cache/back-forward-cache.test.tsThe final upstream deploy-suite run passed 4 tests with the same 1 upstream skipped case.
Risk / Compatibility
cacheComponents: truenow retains hidden Activity trees, which intentionally creates duplicate hidden DOM for inactive entries. Existing e2e locators that target this area use visible filtering.useRouter().bfcacheIdseparate from segment state keys.cacheComponents; withoutcacheComponents, the active segment renders in place unkeyed and no inactive Activity entries are retained.Non-goals
useRouter().bfcacheIduserland contract beyond preserving it alongside Activity retention.References
stateKeycreation andReact.Activityvisible/hidden rendering.Closes #1366