fix(app-router): retain shared layouts across loading shells - #2940
Conversation
Add a `/layout-identity` fixture to the app-router-cloudflare example and e2e coverage showing that a shared `[slug]` layout is discarded during a soft navigation that only changes segments below it. The fixture pairs the shared layout with a `loading.tsx` boundary above it, a parallel slot beside `children`, and a nested dynamic segment with its own layout, matching the shape a real app has. Removing the `loading.tsx` boundary makes every assertion pass, which isolates it as the trigger. The five layout-identity assertions fail on main; the soft-navigation control passes, so the teardown is the router discarding a layout it should have retained rather than a full document load.
commit: |
|
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: retain shared layouts across loading shells
I reviewed the two commits (retain shared layouts + preserve omitted slot segment identity), traced the new identity derivations against the authoritative mint path in app-page-segment-plan.ts, ran the targeted suites, and confirmed vp check is clean.
Verdict
Looks correct. The core design decision — re-deriving each omitted segment's identity for the target params in canCommitOptimisticRouteTemplate rather than trusting the stale identity baked into the learned shell — is the right call and is well tested (does not commit a slot loading shell over a retained nested slot segment, lines 807-834 exercise the beta-learned/alpha-target case). Same-param retention returns false (wait for authoritative payload) and changed-param transitions return true (commit shell), matching the Next.js parity target.
Things I verified
- Identity derivation parity.
resolveTargetBfcacheSegmentIdentityusesbinding.slotId/binding.ownerLayoutIdand the layout check useslayout.id. I confirmed these equal the graph-mode minted values:graphIds.slots[slotKey] === slot.id === binding.slotId(app-route-graph.ts:1867, :739), andrequireGraphSequenceId(graphIds.layouts, i) === route.ids.layouts[i] === layout.id(app-route-graph.ts:540, :1875). So the derived identities match whatderiveSlotIdentitymints for both slot and layout kinds, including thebindOwnerState/ownerStateKeycomposition and the empty-branch level-0 fallback. - Refactor equivalence.
extractRawParamsForMatchedRoute-> sharedextractRawRoutePatternParams, andcanonicalizeAppPageParamsmoved toapp-page-segment-state.ts, are behavior-preserving (Object.create(null), same catch-all/decode semantics). No circular import introduced (app-page-segment-state.tsonly imports a type + utils). Types are structurally identical (Record<string, string | string[]>). - Tests.
app-optimistic-routing(16),app-page-segment-state(8),app-page-route-wiring(79),app-browser-entry(234) all pass locally;vp checkclean on the changed source + test files.
Minor notes (non-blocking)
-
Fallback param canonicalization asymmetry. In
canCommitOptimisticRouteTemplate, the no-identity fallback (app-optimistic-routing.ts:553-558) comparesresolveAppPagePatternStateKey(layout.patternParts, currentParams)againsttargetRouteParams.targetRouteParamsis canonicalized (canonicalizeAppPageParams) butcurrentParams(navigationSnapshot.params) is whatever the prior navigation stored. For normal param values this is a no-op, but a pre-encoded current value could produce a spurious "changed" verdict and commit the shell over a retained layout. This only affects the pre-bfcache-identity path (first render), so impact is narrow — but canonicalizingcurrentParamsbefore the fallback comparison would make it airtight. -
omittedLayoutIdsfallback pattern-vs-prefix equivalence. The fallback relies onresolveAppPagePatternStateKey(layout.patternParts, ...)producing the same key sequence as the mint'sresolveAppPageRouteStateKey(routeSegments.slice(0, treePosition), ...). This holds because route groups are excluded from both, but it's an implicit invariant worth a one-line comment so a future change topatternPartsderivation doesn't silently break the fallback.
Neither note blocks merge; the primary (derived-identity) path is robust and the fallback is only hit before identities exist.
|
Review posted to PR #2940. SummaryI reviewed PR #2940 ( Verdict: Looks correct. No blocking issues found. Key things verified
Minor non-blocking notes I raised
|
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: retain shared layouts across loading shells
Traced the new identity-derivation logic against the authoritative mint path in app-page-segment-plan.ts / app-rsc-route-matching.ts, ran the targeted suites (app-optimistic-routing 16, app-page-segment-state 8 — all green), and confirmed the refactor is behavior-preserving.
The core design — re-deriving each omitted segment's identity for the target params rather than trusting the stale identity baked into the learned shell — is correct and well tested for the primary (bfcache-identity) path. But I found one concrete correctness gap in the fallback path.
Finding: fallback path resets shared layout state for encoded params (bug, narrow)
canCommitOptimisticRouteTemplate at packages/vinext/src/server/app-optimistic-routing.ts:553-558:
if (
resolveAppPagePatternStateKey(layout.patternParts, options.currentParams) ===
resolveAppPagePatternStateKey(layout.patternParts, options.targetRouteParams)
) {
return false;
}targetRouteParams is canonicalized (resolveOptimisticNavigationParams calls canonicalizeAppPageParams, matching the authoritative mint which canonicalizes at app-rsc-route-matching.ts:185). But currentParams is navigationInitiationState.navigationSnapshot.params (app-browser-entry.ts:2063), which is the decoded param map from the prior RSC payload (applyClientParams stores decoded values; decodeMatchedParams runs on match). The two sides are derived from param maps in different encodings, so their state keys diverge for any param whose decoded and canonical forms differ.
Concrete repro — navigating within a shared /projects/[projectId] layout where projectId contains an encoded slash (/projects/a%2Fb/activity):
current key: ["projects","projectId|a/b|d"] // decoded currentParams
target key: ["projects","projectId|a%2Fb|d"] // canonicalized targetRouteParams
equal? false => canCommit returns true => commits shell, resets shared layout state
This is the exact parity bug the PR fixes, surviving in the pre-bfcache-identity fallback (first navigation after a full page load, before identities are minted onto the mounted elements). The same asymmetry would apply to any encoded value (UTF-8, %, double-encoding), i.e. the very cases the summary calls out as covered.
Why the tests don't catch it: the encoded-param assertions in tests/app-optimistic-routing.test.ts:628-656 pass currentParams: { projectId } where projectId is the already-encoded string ("a%2Fb"), so both sides canonicalize identically and the fallback returns false. In production currentParams would hold the decoded "a/b". The wiring tests use currentParams: {}. So the asymmetry is never exercised with a realistic decoded snapshot.
Fix: canonicalize a copy of currentParams before the fallback comparison (mirroring how the identity path already relies on canonicalized target params). A one-line const currentParamsCanonical = { ...options.currentParams }; canonicalizeAppPageParams(currentParamsCanonical); used on the fallback's left-hand side makes both sides encoding-consistent, and a regression test should pass a decoded currentParams with an encoded target to lock it in.
Notes (non-blocking)
-
Implicit invariant,
omittedLayoutIdsfallback + identity path. Both rely onresolveAppPagePatternStateKey(layout.patternParts, ...)producing the same key sequence as the mint'sresolveAppPageRouteStateKey(routeSegments.slice(0, treePosition), ...). This holds only because route groups are excluded from both (patternPartsnever contains them;resolveAppPageRouteStateKeyskips them). Worth a one-line comment so a future change topatternPartsderivation doesn't silently desync the retention check from minted identities. -
Refactor is clean.
extractRawParamsForMatchedRoute-> sharedextractRawRoutePatternParamsand thecanonicalizeAppPageParamsrelocation preserve semantics (Object.create(null), identical catch-all/decode handling, no circular import).
Verdict: primary path is solid; the fallback param-canonicalization gap is a real (if narrow) state-reset regression under encoded params and should be closed with a decoded-currentParams regression test.
|
Review posted to PR #2940. SummaryI reviewed PR #2940 ( Verdict: The primary (bfcache-identity) path is correct and well tested. I found one concrete, narrow correctness gap in the fallback path. Finding (bug, narrow)In I verified with a concrete repro: The added encoded-param tests mask this by hardcoding the encoded value into Non-blocking
|
|
Investigated this finding against the full runtime data flow; no code change is warranted because the premise that
The suggested decode/re-encode of a supposedly decoded current value is not generally recoverable for double encoding or raw percent values: after decoding, |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: retain shared layouts across loading shells
I independently traced the new identity-derivation logic against the authoritative mint path, resolved the disputed encoded-param "fallback bug" against the real runtime data flow, and ran the targeted suites (app-optimistic-routing 16, app-page-segment-state 8, app-page-route-wiring 79, route-pattern 25 — all green; vp check clean on all changed source).
Verdict: looks correct. No blocking issues.
The earlier bonk finding (fallback path resets shared-layout state for encoded params) does not reproduce, and the author's rebuttal is right. I verified the full param encoding lifecycle rather than taking either side at face value:
currentParams (navigationSnapshot.params) is canonical for App Pages, not decoded.
- App Page route matching canonicalizes at
app-rsc-route-matching.ts:185(normalizeMatchedParamsForRoute→canonicalizeAppPageParams; only Route Handlers take thedecodeMatchedParamsbranch). navigationParamsis{ ...routeParams }seeded from those canonical matched params (app-page-element-builder.ts:932, fed by the matcher atapp-page-dispatch.ts:1066/options.params).buildAppPageRscResponseserializes them atapp-page-response.ts:309asencodeURIComponent(JSON.stringify(params))— theencodeURIComponentis a transport wrapper around the JSON string, not a per-value re-encode. The values inside stay canonical (a%2Fb).- The browser reverses only that wrapper:
parseEncodedJsonHeaderdoesJSON.parse(decodeURIComponent(value))(app-browser-entry.ts:325), andapplyClientParams/createClientNavigationRenderSnapshotstore the parsed values verbatim (no per-value decode). - The initial-hydration path (
__VINEXT_RSC_PARAMS__) is injected from the same canonicalnavigationParams.
So both sides of the fallback comparison (currentParams and canonicalized targetRouteParams) are canonical. /projects/a%2Fb/activity yields projectId=a%2Fb on both sides → equal → correctly returns false (retain). The suggested decode/re-encode would actually be wrong: after decoding, %2F can't distinguish a single-encoded value from a decoded %252F, so the canonical-on-both-sides invariant this PR implements is the correct one. The encoded-param tests hardcoding the encoded value into currentParams are therefore realistic, not masking.
Identity parity (highest-risk area) — verified sound
resolveTargetBfcacheSegmentIdentity re-derives each omitted segment's identity for the target params rather than trusting the stale identity baked into the learned shell. I confirmed the derived identities equal the authoritative graph-mode mint in app-page-segment-plan.ts:
- Graph-id equivalence holds.
graphIds.slots[slotKey] === slot.id === binding.slotId(createAppRouteSemanticIdssetsslots[slot.key] = slot.idat app-route-graph.ts:1867;createRouteManifestSlotBindingsetsslotId: slot.idat :739).binding.ownerLayoutIdis already a graph layout id (findRouteManifestOwnerLayoutId→route.ids.layouts[i]), andresolveOwnerLayoutGraphIdmaps a graph layout id back to itself. So passingbinding.slotId/binding.ownerLayoutId/layout.iddirectly equals thegraphIds-resolved values the mint uses. If these ever diverged, the fix would silently no-op (never retain) rather than crash — but they don't. - Slot-param encoding parity holds. The mint's slot-override params come from
resolveSlotParamOverrides→matchRoutePattern(decoded); the optimistic path'sneedsSlotParamOverridebranch also usesmatchRoutePattern(targetUrlParts, slotPatternParts)(decoded). The non-override branch uses canonicaltargetRouteParamson both sides. Both branches are encoding-consistent with the mint. bindOwnerState/ownerStateKeycomposition matches (ownerStateKey ? JSON.stringify([ownerStateKey, identityKey]) : identityKey), and the level-0 empty-branch fallback matches the mint's[{ identityKey: "", ... treePosition: 0 }]candidate. Slot descriptor fields (activeRouteGraphId: null,interceptionTargetRouteGraphId: null,state) matchderiveSlotIdentitycalled withincludeInterceptionTarget=falseon a non-intercepted binding.
The layout path's identity branch (currentIdentity present) is the primary path and is airtight; the no-identity fallback is only reachable before identities are minted onto mounted elements (first navigation after a full page load).
Non-blocking notes
getOmittedBfcacheSegmentIdsdepth cap (depth > 100) is safe. A rendered segment below depth 100 would be misclassified as omitted, but the retention check then re-derives its target identity: if it matches current, it retains (safe — just waits for the authoritative payload); if not, it commits. A false "omitted" is conservative, not a correctness bug.- Implicit invariant worth a one-line comment. The fallback and identity paths both rely on
resolveAppPagePatternStateKey(layout.patternParts, ...)producing the same key sequence as the mint'sresolveAppPageRouteStateKey(routeSegments.slice(0, treePosition), ...). This holds only because route groups are excluded from both. A comment would guard against a futurepatternParts-derivation change silently desyncing the retention check from minted identities.
Refactor
extractRawParamsForMatchedRoute → shared extractRawRoutePatternParams and the canonicalizeAppPageParams relocation to app-page-segment-state.ts are behavior-preserving (Object.create(null), identical catch-all/decode handling, no circular import).
|
Review posted to PR #2940. SummaryI reviewed PR #2940 ( Verdict: looks correct, no blocking issues. The key dispute — resolvedThe earlier bonk review claimed a narrow bug: the no-identity fallback compares a decoded I traced the full param encoding lifecycle end-to-end and the author is correct — it is not a bug:
So both sides of the fallback comparison are canonical → Other things verified
I raised one genuinely useful non-blocking note: add a one-line comment documenting the implicit |
1 participant
Summary
This PR includes the production Cloudflare Worker E2E reproduction originally added in #2939, so the regression and fix land together.
Root cause and Next.js parity
A loading-shell prefetch carries the route's layout identities, but its rendered element stops at the first loading boundary and can omit mounted layouts below that boundary. Vinext committed the shell before the authoritative RSC payload, disconnecting those layouts and resetting their client state even when the destination reused the same layout segment.
Next.js keys each layout-router branch by segment identity. Navigating below an unchanged layout preserves that branch; changing its dynamic segment or parallel-slot branch replaces it. The direct behavioral oracle is
test/e2e/app-dir/app-prefetch-false-loading/app-prefetch-false-loading.test.ts, backed bylayout-router.tsx.The fix records omitted named-slot segments from existing BFCache identities, then reifies those identities from the target route rather than copying learned source identities. Before committing a loading shell, vinext compares the mounted and target identities: unchanged omitted layouts keep rendering until the authoritative payload arrives, while changed params or slot branches retain normal loading/remount behavior. Optimistic and authoritative matching now share canonical raw-param extraction so encoded dynamic values cannot disagree about identity.
Validation
app-router-cloudflarebuild served with Wrangler pluslayout-identity.spec.ts— 6 passedvp check, Knip,vp run vinext#build, andgit diff --check