fix(app-router): share prefetched layouts across routes - #2862
fix(app-router): share prefetched layouts across routes#2862james-elicx wants to merge 14 commits into
Conversation
…he-prefetch-layout-sharing-current # Conflicts: # packages/vinext/src/shims/navigation.ts
|
/bigbonk review for issues |
commit: |
|
Performance benchmarksCompared 0 improved · 4 regressed · 2 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(app-router): share prefetched layouts across routes
Reviewed the full diff (52 files, +4472/-360) and ran the relevant Vitest suites. The layout-sharing architecture — retained-layout claims with exact provider tracking, dependency-cascade invalidation, param-compatibility gating, and the static/runtime instant-shell segment staging — is coherent and well tested. Most changed suites pass locally (app-route-graph, app-instant-prefetch-segments, app-optimistic-routing, build-report, prefetch-cache, app-page-render, app-page-route-wiring, app-page-element-builder, shims, fetch-cache, cache, link, thenable-params, entry-templates, pages-router, app-rsc-*).
Blocking: one deterministic test failure
tests/link-navigation.test.ts > Link prefetch scheduling > downgrades explicit full prefetches to runtime instant shells fails deterministically (3/3 in isolation, and in the full-file run: 1 failed | 75 passed).
AssertionError: expected { cacheForNavigation: true, … } to match object { cacheForNavigation: false, … }
- Expected "cacheForNavigation": false,
+ Received "cacheForNavigation": true,
tests/link-navigation.test.ts:2255
Root cause: the test (added earlier in this stack, commit 6ab36a8) asserts a runtime instant-shell prefetch entry stays cacheForNavigation: false. The main commit here (ca5f865) introduced the promoteCompleteInstantShell path in prefetchRscResponse (packages/vinext/src/shims/navigation.ts:1665-1668): a complete (non-partial) runtime instant shell is promoted to cacheForNavigation: true + retainedLayoutProvider: true. The test's fetch mock returns new Response("") with no X-Vinext-Rsc-Partial-Shell header — i.e. a complete shell — so it now promotes.
The promotion is the intended behavior and is explicitly covered by the new prefetch-cache.test.ts case ("promotes a complete runtime instant shell despite dynamic stale time zero"). The link-navigation.test.ts assertion was simply not updated when the promotion landed. This directly contradicts the PR body's "361/361 passed" claim.
Fix: update the assertion at tests/link-navigation.test.ts:2255-2259 to reflect the promoted state for a complete runtime shell (cacheForNavigation: true, retainedLayoutProvider: true), or point the test's fetch mock at a partial shell (set X-Vinext-Rsc-Partial-Shell: 1) if the intent was to exercise the learning-only branch. Given the test name ("runtime instant shells") and the parity direction, the assertion should be updated. Please re-run the full suite after fixing — the "361/361" number needs to be re-established.
Non-blocking observations
-
Cross-route claim over-offering (bounded, safe).
getRetainedPrefetchLayoutClaimsoffers static (param-less) layouts as claims unconditionally (canRetainLayoutForTargetParamsreturnstruewhenparamNames.length === 0), including layouts from unrelated route sections. These are never consumed because the server only lists a route's own layouts inskippedLayoutIds, andgetLiveRetainedPrefetchLayoutClaims/fillSkippedLayoutsFromElementSourcesfilter byrequiredLayoutIds/missing. TheX-Vinext-Retained-Prefetch-Layoutsheader can carry irrelevant IDs, but it's capped server-side (MAX_RETAINED_PREFETCH_LAYOUT_IDS = 64). Worth a comment noting the intentional over-offering, since it's non-obvious. -
resolvePrefetchedRscResponseExpiresAtreorder is a behavior change for all prefetches. Moving thehonorDynamicStaleTime && dynamicSecondscheck ahead of the cacheLife/server checks changes expiry for automatic prefetches (dynamic bound now wins over a longer nested cacheLife claim). This is intentional and validated by the reworkedsegment-cache-stale-timeport (now + 30_000vs priornow + 240_000), but it's a broad change beyond instant shells — flagging for reviewer awareness. -
Parity note. Could not verify the new error strings (client-component
unstable_instant,unstable_dynamicStaleTime+unstable_instantconflict, invalid-config messages) against upstream because no.nextjs-refclone is present. Please confirm they match Next.js v16.2.6 wording exactly, per AGENTS.md.
Everything else — the recursive dependent-eviction in deletePrefetchCacheEntry, the partial-shell no-store locking in finalizeAppRscResponse, the connection()/params suspension wiring, the per-plugin instantConfigDependencies HMR tracking, and dev/prod parity for the retained-layout header threading through app-rsc-handler → dispatch → element builder — looks correct.
|
Review complete and posted to PR #2862. SummaryI reviewed all 52 changed files and ran the relevant Vitest suites. The core finding: Blocking issue — one deterministic test failure: Non-blocking observations:
Everything else (recursive dependent eviction, partial-shell |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Re-review: fix(app-router): share prefetched layouts across routes
Re-reviewed the full diff (52 files, +4475/-360) after the follow-up commit 0b29870, and re-ran the relevant Vitest suites. The blocking issue from the prior review is resolved.
Prior blocker — resolved ✅
0b29870 test(app-router): align complete instant shell promotion renamed the test to "promotes complete runtime instant shells from explicit full prefetches", awaits entry.pending, and now asserts the promoted state (cacheForNavigation: true, retainedLayoutProvider: true). Verified green in isolation and in the full-file run:
✓ tests/link-navigation.test.ts (76 tests | 75 skipped)
✓ promotes complete runtime instant shells from explicit full prefetches
Re-ran the surrounding suites, all green:
link-navigation(76),prefetch-cache(80),thenable-params(22),app-instant-prefetch-segments(3),app-route-graph(70),app-optimistic-routing(14),app-page-render(68),app-page-element-builder(65),app-page-route-wiring(82),fetch-cache(145),app-rsc-request-normalization(76),shims(1285),link(129).
Security boundary — verified safe
The retained-layout-skip mechanism is the highest-risk part of this PR (a client-supplied header controls which layouts the server omits). I traced the full path and it is sound:
- Server input is bounded/validated.
parseRetainedPrefetchLayoutIds(app-rsc-request-normalization.ts:155) caps atMAX_RETAINED_PREFETCH_LAYOUT_IDS = 64, bounds each token to 512 chars, dedupes, and rejects any token whoseparseElementKey(...).kind !== "layout". - Skips are gated on the route's own tree. In prefetch-shell mode the server derives candidate layout IDs from
route.layoutsand only skips those intersecting the client set (app-page-element-builder.ts:257-265). For navigation responses,createRetainedPrefetchLayoutSkipDispositiononly skips a layout whenObject.hasOwn(input.element, layoutId)(app-page-render.ts:580) — a client cannot induce skipping of a layout that isn't already in its own route's rendered tree. - Client reconstruction fails closed.
fillSkippedLayoutsFromPrefetchCache(app-browser-entry.ts:1049) re-validates provider liveness viagetLiveRetainedPrefetchLayoutClaimsand throws"Retained prefetch layout provider is no longer available"if any required provider is gone, forcing a fresh RSC request rather than committing an incomplete tree. - Param compatibility gate.
canRetainLayoutForTargetParams(navigation.ts:515) allows param-less (static) layouts unconditionally but requires exact param equality for parameterized layouts — retained layouts cannot leak across incompatible dynamic routes.
The cross-route claim over-offering the prior review flagged remains bounded and safe (offered claims are filtered by requiredLayoutIds/missing at consume time and capped server-side). Non-blocking; a short comment on getRetainedPrefetchLayoutClaims noting the intentional over-offering would help future readers.
Next.js v16.2.6 parity — verified verbatim
Cloned vercel/next.js@v16.2.6 and diffed the new error strings against source (packages/next/src/build/analysis/get-page-static-info.ts and .../segment-config/app/app-segment-config.ts). These match verbatim:
Page "..." cannot export "unstable_instant" from a Client Component module. ...Page "..." cannot use `export const unstable_instant = ...` without enabling `cacheComponents`.Page "..." cannot use both `export const unstable_dynamicStaleTime` and `export const unstable_instant`.Invalid unstable_instant value ... on "...", must be an object with `prefetch: "static"` or `prefetch: "runtime"`, or `false`. Read more at https://nextjs.org/docs/messages/invalid-instant-configuration
The "...the exported configuration must be statically analyzable..." string (app-route-graph.ts:461) has no verbatim upstream twin — Next.js surfaces a lower-level extractExportedConstValue failure. This is an acceptable, deliberate divergence (vinext uses a different static-analysis path) and it links to the same docs anchor.
Non-blocking: one parity gap
Next.js v16.2.6 validates that unstable_dynamicStaleTime cannot be exported from a layout file (get-page-static-info.ts:711):
"${page}" cannot use `export const unstable_dynamicStaleTime`. This config is only supported in page files, not layouts.
vinext implements the unstable_dynamicStaleTime + unstable_instant conflict check (app-route-graph.ts:541) but not the layout-restriction check. A user who exports unstable_dynamicStaleTime from a layout file gets a hard build error in Next.js but silent no-op behavior in vinext. Given this PR's stated goal of exact fixture parity, worth a follow-up (or a ported test + the check). Not blocking — it's a validation-only divergence outside the layout-sharing feature this PR targets.
Other paths reviewed, look correct
resolvePrefetchedRscResponseExpiresAtreorder (navigation.ts:608) — the dynamic-bound-wins-over-cacheLife change is intentional and covered by the reworked stale-time port. ThepromoteCompleteInstantShell && !isPartialInstantShellbranch correctly opts complete runtime shells onto the static floor (navigation.ts:1693-1701).suspendStaticInstantPrefetchRequestData(instant-prefetch-shell.ts:173) — static shells fail request-data reads (params/headers/cookies/connection/private-cache) into hanging promises React records as holes; theisInsideStaticRequestDataCacheScopeguard correctly avoids the abort-vs-cache-task deadlock. The thenable-paramsthen-proxy path forwards to the hanging promise's continuation rather than the resolved value — correct.- Recursive dependent eviction, partial-shell
no-storelocking, and dev/prod parity (retained-layout header only flows through the shared App Router RSC handler; Pages Router unaffected) — all consistent.
Verdict
The prior blocker is fixed and the PR now behaves as its body claims. No new blocking issues. Recommend addressing the unstable_dynamicStaleTime-in-layouts parity gap as a follow-up and adding a one-line comment on the intentional claim over-offering.
|
Review posted to PR #2862. SummaryThe prior review's blocking issue is resolved. A follow-up commit ( Key findings:
No new blocking issues. The PR now behaves as its body claims. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Re-review: fix(app-router): share prefetched layouts across routes
Re-reviewed after the follow-up commit 0ef0d6b fix(app-router): preserve skipped layout provenance, which landed after the previous re-review. Re-ran the affected suites and type-checked the changed source. No blocking issues. This commit is a genuine correctness/security improvement and closes a real gap from the prior state.
What 0ef0d6b fixes (and why it matters)
Before this commit, fillSkippedLayoutsFromPrefetchCache (client) filtered on the entire __skippedLayoutIds set. That set can contain two very different kinds of omission:
- Standard verified reuse (
SKIP_STATIC_LAYOUT_VERIFIED) — the server proved a layout byte-identical to the client's own current in-memory tree and expects it to be preserved from current state (mergeSkippedLayoutPreservation, gated oncurrentLayoutIds.has(id)). - Retained-prefetch layouts (
SKIP_RETAINED_PREFETCH_LAYOUTS) — skipped because of the client-suppliedX-Vinext-Retained-Prefetch-Layoutsheader, to be filled from prefetch-cache providers.
Filling standard-reuse omissions from retained prefetch providers conflated the two channels: a layout the server skipped because it matched the current tree could get resolved from an unrelated prefetch snapshot. The commit introduces a distinct __retainedOnlySkippedLayoutIds wire key so only channel (2) is filled from providers, and channel (1) is left for the existing current-state preservation path. That is the correct separation.
Verified correct
- Merge de-classification is sound.
mergeSkipDispositions(app-page-render.ts:610-625) collectsstandardReuseIdsfrom anySKIP_STATIC_LAYOUT_VERIFIEDdisposition and excludes those IDs fromretainedOnlySkippedEntryIds. So a layout authorized by both verified static reuse and the retained header is treated as standard-reuse (filled from current state), never from a client-supplied provider. Covered by the newapp-page-rendertest "does not require a retained provider when standard reuse also authorizes the omission" and theapp-optimistic-routingtest "does not fill standard-reuse omissions from a retained-layout provider". - Wire subset invariant enforced.
readAppElementsMetadata(app-elements-wire.ts:914-919) throws if__retainedOnlySkippedLayoutIdsis not a subset of__skippedLayoutIds, so a malformed/adversarial payload cannot smuggle a retained-only ID outside the declared skip set. Tested ("rejects retained-only skip metadata outside the complete skip set"). - Fail-closed reconstruction preserved.
fillRetainedPrefetchLayoutsFromElementSourcesstill throws"Retained prefetch layout provider is no longer available"when a retained-only layout has no live provider (forcing a fresh RSC request), and returnselementsunchanged when nothing is retained-only-missing.getMissingRetainedPrefetchLayoutIdscorrectly excludes already-present layouts viaObject.hasOwn. Both branches tested. - Rename + visibility cleanup is complete.
fillSkippedLayoutsFromElementSources→fillRetainedPrefetchLayoutsFromElementSourceshas no stale references;NavigationRuntimeRetainedLayoutClaimwas correctly demoted fromexport(no external importers).
Test / check results (local)
app-elements(88),app-optimistic-routing(18),app-page-render(69) — all greenapp-page-route-wiring(82),app-page-element-builder(65),app-rsc-request-normalization(76),prefetch-cache(80),link-navigation(76) — all greenvp checkon the six changed source files and the changed test files — format/lint/types clean
Non-blocking (carried over from prior reviews, still open)
unstable_dynamicStaleTime-in-layouts parity gap. Next.js v16.2.6 hard-errors whenunstable_dynamicStaleTimeis exported from alayoutfile; vinext silently no-ops. Not touched by this commit. Worth a follow-up given the exact-parity goal.- Cross-route claim over-offering. Still bounded (server caps at 64 IDs, filters by the route's own tree) and safe. A one-line comment on
getRetainedPrefetchLayoutClaimsnoting the intentional over-offering would help future readers. - Bundle-size regression. The perf comment reports ~+2–2.7% across client/server/RSC bundle sizes. Expected for a feature of this size, but worth a glance to confirm nothing feature-specific leaked onto the common startup path.
Verdict
The provenance-tracking commit is correct and strengthens the security boundary by preventing client-supplied prefetch providers from filling server-verified static-reuse omissions. Combined with the previously-verified header bounding, param-compatibility gating, and fail-closed reconstruction, the retained-layout mechanism is sound. Recommend merging once the non-blocking parity gap is tracked as a follow-up.
|
Review posted to PR #2862. SummaryI re-reviewed the PR after the new follow-up commit What the new commit fixes: Previously the client's
This meant a layout the server skipped because it matched the current tree could get resolved from an unrelated prefetch snapshot. The commit adds a separate Verified correct:
Tests/checks (local): 8 affected suites all green (554 tests); Non-blocking (carried over): the |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Re-review: fix(app-router): share prefetched layouts across routes
Re-reviewed after the new follow-up commit 1d10cb2 fix(link): refresh expired runtime instant prefetches, which landed after the previous re-review. Re-ran the affected suites and type-checked the changed file. No blocking issues. This commit is a correct, well-targeted fix.
What 1d10cb2 fixes (and why it matters)
Runtime instant-shell prefetches start life as learning-only entries (cacheForNavigation: false), and a complete response is later promoted to cacheForNavigation: true for navigation reuse. But the <Link> prefetch scheduler gates re-prefetching on the learning-only prefetched marker set:
if (prefetched.has(cacheKey)) {
if (!autoPrefetch.cacheForNavigation) {
return; // old behavior: always skip
}
}For a runtime-instant <Link>, autoPrefetch.cacheForNavigation is false (learning-only config), so this prefetched.has(cacheKey) block was the only gate — the later hasNavigationCacheEntry checks (link.tsx:650-658) and the discardLearningOnlyPrefetchCacheEntry call (link.tsx:550) are all gated on autoPrefetch.cacheForNavigation and never run for this config. That meant once a shell was fetched and its prefetched marker set, an expired promoted shell stayed pinned forever and was never refreshed.
The fix applies the same freshness-aware gate a regular reusable prefetch uses:
const entry = getPrefetchCache().get(cacheKey);
if (
entry?.cacheForNavigation !== true ||
hasPrefetchCacheEntryForNavigation(rscUrl, interceptionContext, mountedSlotsHeader)
) {
return;
}Verified correct
- Short-circuit ordering is right. If the entry was never promoted (
cacheForNavigation !== true) the code still skips (learning-only shells are intentionally not re-driven by visibility). Only a promoted shell reaches the freshness check; if it is fresh,hasPrefetchCacheEntryForNavigationreturnstrue→ skip; if it is stale, it returnsfalseand deletes the stale entry (from both the cache andprefetchedUrlsviadeletePrefetchCacheEntry), so execution falls through to re-prefetch. - Re-prefetch path is clean. After fall-through, because
autoPrefetch.cacheForNavigationisfalse, the intermediatehasNavigationCacheEntrygates (link.tsx:650-676) are skipped,prefetched.add(cacheKey)re-adds the marker (link.tsx:677), andprefetchRscResponseis called with the correct instant-shell options (instantShell: true,promoteCompleteInstantShell: "runtime"→true) at link.tsx:783-812. The refreshed shell is promoted again on the next complete response. No double-add or orphaned marker. - Exact-vs-normalized lookup alignment. The exact
getPrefetchCache().get(cacheKey)uses the sameAppElementsWire.encodeCacheKey(rscUrl, interceptionContext)key the helper's exact-lookup branch uses, so the two lookups agree for the target URL. If a valid-but-slot-mismatched entry exists, the helper returnsfalsewithout deleting it (match isnull) and the code re-prefetches for the current mounted-slots context — correct. - Static floor preserved. The new test asserts
expiresAt: 1_120_000fromNext-Router-Stale-Time: 120(not theX-Vinext-Dynamic-Stale-Time: 30), confirming the promoted complete runtime shell still opts onto the static stale floor rather than the dynamic bound — consistent with the previously-reviewedpromoteCompleteInstantShell && !isPartialInstantShellbranch.
Test / check results (local)
link-navigation(77) — green, including the new "re-prefetches a promoted runtime instant shell after its cacheLife expires" (exercises pre-expiry no-op att=1_119_999and post-expiry refetch att=1_120_001)prefetch-cache(80),link(129) — greenvp check packages/vinext/src/shims/link.tsx tests/link-navigation.test.ts— format/lint/types clean
Non-blocking (carried over from prior reviews, still open)
unstable_dynamicStaleTime-in-layouts parity gap. Next.js v16.2.6 hard-errors whenunstable_dynamicStaleTimeis exported from alayoutfile; vinext silently no-ops. Not touched by this commit. Worth a follow-up given the exact-parity goal.- Cross-route claim over-offering. Still bounded (server caps at 64 IDs, filters by the route's own tree) and safe. A one-line comment on
getRetainedPrefetchLayoutClaimsnoting the intentional over-offering would help future readers. - Bundle-size regression. The perf comment reports ~+2–2.9% across client/server/RSC bundle sizes. Expected for a feature of this size, but worth a glance to confirm nothing feature-specific leaked onto the common startup path.
Verdict
The freshness-refresh commit is correct and closes a real staleness bug (promoted runtime instant shells were never refreshed after cacheLife expiry). Combined with the previously-verified header bounding, param-compatibility gating, fail-closed reconstruction, and skipped-layout provenance separation, the retained-layout mechanism is sound. No new blocking issues. Recommend merging once the non-blocking parity gap is tracked as a follow-up.
|
Review posted to PR #2862. SummaryI re-reviewed after the new follow-up commit What Verified correct:
Tests/checks (local): Non-blocking (carried over): the Recommend merging once the non-blocking parity gap is tracked as a follow-up. |
…he-prefetch-layout-sharing-current # Conflicts: # packages/vinext/src/shims/internal/app-route-prefetch-policy.ts # packages/vinext/src/shims/link.tsx # tests/link.test.ts
# Conflicts: # packages/vinext/src/shims/cache-runtime.ts # packages/vinext/src/shims/internal/app-route-prefetch-policy.ts # packages/vinext/src/shims/link.tsx # packages/vinext/src/shims/navigation.ts # tests/prefetch-cache.test.ts
1 participant
Summary
Next.js parity
This targets the unskipped assertions in
prefetch-layout-sharing.test.ts.Target command:
Validation
main: 564/564 passedvp run checkgit diff --check