fix(app-router): support segment-cache runtime prefetch layout shells#2475
fix(app-router): support segment-cache runtime prefetch layout shells#2475james-elicx wants to merge 14 commits into
Conversation
|
/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: segment-cache runtime prefetch layout shells
Reviewed the full diff, ran targeted unit suites (app-optimistic-routing, ppr-fallback-shell, app-page-route-wiring, prefetch-cache, entry-templates, shims — all green) and vp check on the touched files (clean). Overall the design is coherent: layout IDs are advertised via X-Vinext-Rsc-Layout-Ids, the client re-sends held IDs via X-Vinext-Retained-Prefetch-Layouts, the server skips them (SKIP_RETAINED_PREFETCH_LAYOUTS), and the client refills skipped layouts from its own prefetch cache. A few things worth attention below.
1. Build-time unstable_instant detection is regex-based and fragile — entries/app-browser-entry.ts:83
readFilePrefetchMode greps raw source with:
/export\s+const\s+unstable_instant\s*=\s*\{[\s\S]*?\bprefetch\s*:\s*(['"])runtime\1/This diverges from the server path (app-page-route-wiring.tsx:110), which reads the evaluated module.unstable_instant. The regex silently fails to detect valid Next.js declarations, e.g.:
export const unstable_instant = CONFIG(referencing an imported/aliased constant)export const unstable_instant = { prefetch: mode }(computed value)const unstable_instant = {...}; export { unstable_instant }- re-exports, or
prefetch:'runtime'spread from another object
When the regex misses, the client manifest omits hasRuntimePrefetch/prefetchMode, so the client never issues a runtime/static prefetch even though the server would honor it. This is a silent client/server parity gap. At minimum add a comment documenting the supported declaration shape, and consider whether the value can be sourced from the same module evaluation the router already does rather than a second regex pass. Note also there is no fixture exercising this path — the only coverage is entry-templates.test.ts with the exact canonical shape.
2. runWithPrerenderWorkUnit now wraps every App Router request — server/prerender-work-unit-setup.ts:34
The non-prerender branch changed from return fn() to workUnitAsyncStorage.run({ type: "request" }, fn). This is the right call for the DebugRenderKind fixture (and matches Next.js, which always has a work-unit store during render), but it is a broader behavioral change than the PR title implies and affects any code that branches on workUnitAsyncStorage.getStore():
shims/cache.ts:233io()— safe:requestand no-store both return_resolvedIOPromise.server/otel-tracer-extension.ts:120startActiveSpan— now activates for normal requests where it previously forwarded unchanged (theif (!workUnitStore)early-return no longer fires). The re-enter/exit wrapping looks safe and arguably more correct, but this path was previously dead for normal requests and is now live for every request when a tracer is present. Worth a deliberate confirmation that the span-context exit/re-enter is desired for standard SSR requests, and ideally a test.
3. Prefetch scheduling change is undocumented — shims/link.tsx:734
Low-priority App Router prefetches previously deferred through requestIdleCallback; now hasAppNavigationRuntime() forces immediate execution for all App Router prefetches. This is presumably required so retained-layout headers observe a deterministic, already-populated cache from earlier prefetches, but it changes the main-thread scheduling characteristics of link prefetching and isn't mentioned in the PR summary. Please call it out (and confirm it's intentional rather than a side effect).
4. Dead parameter — server/app-page-render.ts:516
createRetainedPrefetchLayoutSkipDisposition declares layoutFlags in its input type and callers pass it (app-page-render.ts:745), but the body never reads it. Drop it to avoid implying the skip decision considers static/dynamic flags (it doesn't — it's a pure client-claim + element-presence check).
5. Trust model note (not a blocker) — SKIP_RETAINED_PREFETCH_LAYOUTS
Unlike SKIP_STATIC_LAYOUT_VERIFIED, the server skips retained layouts purely on the client's header claim plus element presence — no server-side re-verification. The only safety guard is client-side (canRetainLayoutForTargetParams, navigation.ts:412), which refuses to retain a dynamic layout unless the cached snapshot's params equal the target params. That guard is correct and the failure mode (client reuses a layout it already holds) matches Next.js's segment-cache trust model, so this is fine — just flagging that the invariant lives entirely on the client and should not be weakened.
Correctness spot-checks that passed
getRetainedPrefetchLayoutIdsHeaderis server-safe (getPrefetchCache()returns an empty Map on server,resolveTargetParamsFromHrefguards onisServer).wrapRuntimePrefetchableSubtree/RuntimePrefetchableServerComponentdefer invocation via returned elements — no double-invoke; ALS bypass re-applies at each nesting level.wrapStaticPrefetchShellSubtreeleaves the<Children />page placeholder intact (function component, nochildrenprop), so page content is not suppressed by the static shell wrapper.- Header is added to
VINEXT_RSC_VARY_HEADERand cache-busting input, so retained-layout variants won't collide in the RSC cache.
Nothing here is a hard blocker; items 1–3 are the ones I'd want addressed or explicitly acknowledged before merge.
|
Review posted to PR #2475. Summary of findings: Review completeI reviewed the full 36-file diff, ran the targeted unit suites (all green) and Findings posted (none hard blockers):
Also verified several correctness aspects that hold up: server-safe header resolution, no component double-invocation in the subtree wrappers, page content not suppressed by static shells, and proper Vary/cache-busting inclusion of the new header. |
james-elicx
force-pushed
the
codex/fix-segment-cache-prefetch-layout-sharing-28478866791
branch
from
d2e37ab to
0f0b4d8
Compare
|
/bigbonk review for issues |
james-elicx
force-pushed
the
codex/fix-segment-cache-prefetch-layout-sharing-28478866791
branch
from
0f0b4d8 to
05066e7
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: segment-cache runtime prefetch layout shells (re-review)
Re-reviewed the full diff at 0f0b4d8. Ran the targeted unit suites (entry-templates, prerender-work-unit-setup, app-optimistic-routing, app-page-route-wiring, ppr-fallback-shell, prefetch-cache — 154 tests, all green) and traced the retained-layout flow end to end.
Notably, the two commits (50cc24d, 0f0b4d8) landed after the first /bigbonk pass and already resolve most of that review's findings. Summary below.
Previously-flagged items now resolved
-
unstable_instantdetection is no longer a single fragile regex.entries/app-browser-entry.ts:89now uses a proper multi-file resolver (readFileExportedPrefetchMode) that follows imported constants, local initializers, re-exports (export { x } from "…"), default imports, andas const. There's cross-file coverage inentry-templates.test.ts:236(import { runtimeInstant } from "./instant-config"; export const unstable_instant = runtimeInstant;→hasRuntimePrefetch:true). This closes the main client/server parity concern from the prior review. -
Dead
layoutFlagsparameter removed fromcreateRetainedPrefetchLayoutSkipDisposition(app-page-render.ts:513). The skip decision is now a clean client-claim + element-presence check. -
runWithPrerenderWorkUnitwrapping every request is now covered bytests/prerender-work-unit-setup.test.ts(asserts{ type: "request" }for normal requests and prerender abort semantics). -
Immediate App Router prefetch scheduling is now documented in code —
link.tsx:433comment ("App Router and high-priority prefetches start immediately"). Theotel-tracer-extension.ts:120startActiveSpanexit/re-enter path that becomes live under the always-present request store is symmetric and correct (exit to mint a fresh span id, re-enter the same store inside the callback).
Correctness spot-checks that hold up
- TOCTOU on retained layouts is well-defended in depth. The client only advertises layout IDs it currently holds (
getRetainedPrefetchLayoutIdsHeaderrecomputed at request time,navigation.ts:495); the server skips only claimed + present layouts; the client refills viafillSkippedLayoutsFromPrefetchCache(app-browser-entry.ts:992). If a prefetch entry is evicted after the header is sent (e.g. the memory-pressure eviction from #2466),mergeSkippedLayoutPreservation(app-browser-state.ts:552) still preserves the layout from the current committed state, gated oncurrentLayoutIds.has(id) && targetLayoutIds.has(id) && Object.hasOwn(currentState.elements, id). Two independent fallbacks, so a skipped layout is not silently dropped. - Skipped-list metadata stays honest.
fillSkippedLayoutsFromElementSources(app-optimistic-routing.ts:390) rewritesskippedLayoutIdsto only the ids it couldn't fill, so the reducer's current-state preservation targets exactly the residual set. - Dynamic-layout retention guard is correct —
canRetainLayoutForTargetParams(navigation.ts:418) refuses to retain a dynamic layout unless the cached snapshot's params equal the resolved target params; server-side resolution is guarded byisServer. - Header input is bounded/validated —
parseRetainedPrefetchLayoutIdscaps at 64 ids / 512 chars each, dedups, and requireskind === "layout". Added toVINEXT_RSC_VARY_HEADER+ cache-busting input so retained variants don't collide in the RSC cache. - Static shell wrapping leaves page content intact (verified
wrapStaticPrefetchShellSubtreekeeps the<Children />placeholder).
Residual note (non-blocking)
- The build-time
unstable_instantobject regex (readObjectExpressionPrefetchMode,app-browser-entry.ts:174) and the initializer scanner (readInitializerExpression) do not strip comments. A pathological declaration like{ /* prefetch:"runtime" */ prefetch:"static" }sits outside what the regex matches (the comment breaks the required[{,]-adjacency), so it degrades to the correct static value here — but this is a heuristic, not a parser. Since the server path evaluates the real module and this only affects the client-side prefetch optimization (never the authoritative render), the failure mode is a missed prefetch, not incorrect output. Acceptable as-is; worth a one-line comment noting the supported declaration shapes if this grows.
Verdict: no blockers. The design is coherent, the prior review's actionable items are addressed, and the retained-layout invariant has redundant client-side safety nets. LGTM once CI is green.
|
Review posted to PR #2475. Review completeI re-reviewed the full diff at commit Key finding: The two commits on this branch (
Correctness verified:
Residual (non-blocking): The build-time object-config regex doesn't strip comments, but since the server path evaluates the real module and this only affects the client prefetch optimization (not the authoritative render), the worst case is a missed prefetch — acceptable. Verdict: no blockers. LGTM once CI is green. |
commit: |
|
|
/bigbonk review for issues |
james-elicx
force-pushed
the
codex/fix-segment-cache-prefetch-layout-sharing-28478866791
branch
from
4629e22 to
7c04e12
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: segment-cache runtime prefetch layout shells (focused re-review)
Re-reviewed at 4629e22. Ran the touched unit suites (app-rsc-cache-busting 34, app-optimistic-routing 13, app-page-route-wiring 44 — all green) and vp check on app-rsc-cache-busting.ts + app-page-render.ts (clean). This pass concentrated on the one commit that landed after the two prior /bigbonk reviews (4629e22 "accept retained-layout rsc cache keys") and re-traced the skip-disposition merge, since those are the least-covered areas so far.
New commit 4629e22 — correct, but the new acceptance path is untested
computePreviousRscCacheBustingSearchParams / ...Legacy... now each return an array that adds a retained-header-excluded variant (includeRetainedPrefetchLayoutsHeader: false). I traced the intent and it's right: adding X-Vinext-Retained-Prefetch-Layouts to createCacheBustingInput (as a ,0 slot when absent, or its value when present) changes the canonical hash for every App Router RSC URL. Without this, a URL minted by the previous deploy (6-slot input p,s,st,url,ic,slots) would no longer match the new canonical hash (7-slot input with the retained slot), triggering a 307 canonicalization redirect on the first navigation after rollout. The excluded-variant reproduces exactly the old 6-slot input, so the redirect is suppressed. Gating on navigation render mode (normalizeRenderModeHeaderValue(...) === null) matches the existing preserve-UI exclusions. This does not weaken cache correctness — the header is in VINEXT_RSC_VARY_HEADER, so distinct retained values still key distinct cache entries; the accept-set only affects the redirect decision, and the render always proceeds with the request's actual headers.
Gap: the only test change for this commit is the VINEXT_RSC_VARY_HEADER string assertion (app-rsc-cache-busting.test.ts:325). There's no test exercising the new branch — i.e. a request whose _rsc was computed without the retained header but that now carries one (or vice versa) should resolve to null (no redirect). The existing "accepts previous SHA cache-busting params after adding a varying header" test (line 259) covers the mounted-slots rollout, not the retained-layouts rollout. Please add a case: request with X-Vinext-Retained-Prefetch-Layouts set + a _rsc matching the retained-excluded hash → resolveInvalidRscCacheBustingRequest returns null. This is the whole point of the commit and it's currently unverified.
mergeSkipDispositions relabels the merged code/mode — cosmetic today, latent trap
app-page-render.ts:546 merges clientReuseSkipDisposition (which can be SKIP_STATIC_LAYOUT_VERIFIED / skipStaticLayout) with createRetainedPrefetchLayoutSkipDisposition (SKIP_RETAINED_PREFETCH_LAYOUTS / skipRetainedPrefetchLayouts). When both are enabled, the result unconditionally hardcodes code: "SKIP_RETAINED_PREFETCH_LAYOUTS" / mode: "skipRetainedPrefetchLayouts" while keeping the union of skippedEntryIds. So static-layout-verified ids get relabeled as retained-prefetch ids.
I confirmed this is not a functional bug today: the wire encoder (app-elements-wire.ts:661, createSkippedLayoutIds) only reads skippedEntryIds and emits a flat __skippedLayoutIds list — the mode/code are never serialized. The only other consumer is isSkipTransportEnabled, which reads .enabled. The client refill (fillSkippedLayoutsFromElementSources) is source-agnostic and refills the whole set from prefetch cache + current committed state, so the merged list is handled uniformly regardless of skip reason.
That said, it's a trap: the merged disposition is now a type-level lie (a skipStaticLayout entry reported as skipRetainedPrefetchLayouts). If any future server-side code branches on skipDisposition.mode/.code after the merge, it silently misclassifies. Two low-cost options: (a) if both modes truly are wire-equivalent, add a short comment on mergeSkipDispositions stating the code/mode is intentionally collapsed because only skippedEntryIds+enabled are observable downstream; or (b) introduce a neutral merged mode so the label isn't a specific-but-wrong one. Not a blocker.
Confirmed sound (independent of prior reviews)
- Dead
layoutFlagsparam flagged in the first review is gone fromcreateRetainedPrefetchLayoutSkipDisposition; remaininglayoutFlagsusages are legitimate. - Retained-layout skip is safe under eviction — refill falls back from prefetch cache →
mergeSkippedLayoutPreservation(current committed state, guarded oncurrentLayoutIds.has(id) && targetLayoutIds.has(id) && Object.hasOwn(...)), andskippedLayoutIdsmetadata is rewritten to the residual unfilled set, so nothing is silently dropped. - Retained header is in Vary + cache-busting input, so retained variants don't collide in the RSC cache.
- Rollout accept-set doesn't over-accept for preserve-UI render modes (still returns 307 to canonical).
Verdict: no blockers. One concrete ask (add the missing 4629e22 acceptance test) and one optional cleanup (mergeSkipDispositions label). LGTM once CI is green.
|
Review posted to PR #2475. Review completeI focused this pass on the commit that landed after the two prior Findings (no blockers):
Verified sound: dead Overall the design holds up and the prior reviews' actionable items are addressed. LGTM once CI is green. |
james-elicx
force-pushed
the
codex/fix-segment-cache-prefetch-layout-sharing-28478866791
branch
from
7c04e12 to
b949649
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: segment-cache runtime prefetch layout shells (fourth pass)
Re-reviewed the full 38-file diff at b949649. Ran the touched unit suites (app-optimistic-routing 13, app-page-route-wiring 44, app-rsc-cache-busting 38, prerender-work-unit-setup 2, ppr-fallback-shell 16 — 113 tests, all green) and vp check on the six primary source files (clean). This pass focused on verifying that the three prior /bigbonk reviews' actionable items actually landed, and on the retention/advertisement invariant chain that no prior pass traced end to end.
Prior review items — all confirmed resolved
unstable_instantdetection (entries/app-browser-entry.ts) is now a proper multi-file resolver (readFileExportedPrefetchMode→readPrefetchModeExpression) following direct exports, default exports,export { x } from "…"re-exports, local const initializers, named/default imports, andas const.readInitializerExpressioncorrectly balances quotes/braces/brackets/parens.resolveRelativeImportPathguards on.-relative specifiers and tries the standard extension set. Cross-file coverage exists inentry-templates.test.ts:280(importedruntimeInstant+as const). Closes the client/server parity concern.- Dead
layoutFlagsparam removed fromcreateRetainedPrefetchLayoutSkipDisposition(now a clean client-claim + element-presence check,app-page-render.ts:513). runWithPrerenderWorkUnitwrapping every request now has dedicated coverage (tests/prerender-work-unit-setup.test.ts). Theotel-tracer-extension.ts:119startActiveSpanpath that becomes live under the always-present request store is symmetric and correct (exit to mint a fresh span id, re-enter the same store inside the callback).mergeSkipDispositionscode/mode collapse is now documented in-code (app-page-render.ts:561).- Immediate App Router prefetch scheduling is documented (
link.tsx:433). 4629e22retained-layouts accept-set now has a test (app-rsc-cache-busting.test.ts:272, "accepts previous SHA cache-busting params after adding retained prefetch layouts").
Newly verified correctness
- Advertise/retain invariant is self-consistent and fail-safe. The server advertises
X-Vinext-Rsc-Layout-IdsfromcollectResponseLayoutIds, which filterslayoutFlagsagainst the encoded payload (app-page-render.ts:763). Skipped layouts are stripped from that payload (app-elements-wire.ts:675), so they are not re-advertised. The client storessnapshot.layoutIdsfrom that header (navigation.ts:921). Consequence: a layout that was skipped in prefetch B (refilled from A) is not in B's ownlayoutIds, so if A is later evicted the client won't claim to hold that layout via B for a future prefetch C. That is the correct fail-safe — worst case is a redundant re-render, never a dropped/incorrect layout. - Dynamic-layout retention guard is href-scoped.
getRetainedPrefetchLayoutIdsHeader({ targetHref })resolves target params from the prefetch target href (link.tsx:550), andcanRetainLayoutForTargetParamsrefuses to retain a dynamic layout unless the cached snapshot params equal the target params (navigation.ts:418). A prefetch for route B cannot claim a dynamic layout whose params belong to route A. - Static-shell wrapping is semantically correct.
wrapStaticPrefetchShellSubtreereplacesSuspensechildren with the (recursively-wrapped) fallback and otherwise recurses viacloneElement. Because the page lives in a separateelements[pageElementId]entry referenced through the<Children />context marker (shims/slot.tsx:470) rather than being physically nested, a static sub-layout shell shows its own content + Suspense fallbacks without dropping page content.tests/app-page-route-wiring.test.ts:949asserts exactly this (static shell shown, layout runtime content suppressed to fallback, page content preserved). - Server-side header handling is bounded/validated.
parseRetainedPrefetchLayoutIdscaps at 64 ids / 512 chars each, dedups, and requireskind === "layout"(app-rsc-request-normalization.ts:56). Truncation is safe: an un-parsed claimed layout is simply rendered fresh. The header is inVINEXT_RSC_VARY_HEADER+ cache-busting input, so retained variants don't collide in the RSC cache. - Rollout accept-set doesn't over-accept.
computePreviousRscCacheBustingSearchParamsadds only the retained-header-excluded variant, gated on navigation render mode (normalizeRenderModeHeaderValue(...) === null); preserve-UI modes still 307 to canonical. The header remains in Vary, so distinct retained values still key distinct cache entries — the accept-set only suppresses the rollout canonicalization redirect.
Residual (non-blocking, unchanged from prior pass)
readObjectExpressionPrefetchMode/readInitializerExpressionare heuristic string scanners, not a parser (no comment stripping). Since the server path evaluates the real module (app-page-route-wiring.tsx:109), this only affects the client-side prefetch optimization; the worst case is a missed prefetch, never incorrect output. Acceptable.
Verdict: no blockers. All three prior reviews' actionable items landed, the retention/advertisement invariant is self-consistent and fail-safe, and the new behavior is covered by unit + E2E tests. LGTM once CI is green.
|
Review posted to PR #2475. Review completeThis is the fourth Key conclusion: All actionable items from the three prior reviews have actually landed:
Newly verified this pass (not deeply examined before):
Verdict: no blockers. LGTM once CI is green. The one residual (heuristic object-config scanner without comment stripping) remains acceptable because the server path evaluates the real module — worst case is a missed prefetch optimization, not incorrect output. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: segment-cache runtime prefetch layout shells (fifth pass)
Re-reviewed at 72e69e9 (adds only the link.test.ts shape alignment + moving the prefetch-layout-sharing.spec.ts E2E from the app-router dev project to the prod-build app-router-client-cache project). Ran the touched unit suites (link 121, app-page-route-wiring 44, entry-templates 41 — 206 tests, all green). All prior reviews' actionable items remain resolved.
This pass I traced the retained-layout skip mechanics down to the wire encoder and cross-checked against Next.js segment-cache semantics, which surfaced one comment-accuracy issue and confirms a design point worth stating explicitly.
1. Stale comment in mergeSkippedLayoutPreservation — app-browser-state.ts:574
The soundness comment reads:
the server only emits a skip for a layout it proved byte-identical via the static-layout cache proof, so preserving the retained-and-identical layout … is sound
That was true before this PR, when only SKIP_STATIC_LAYOUT_VERIFIED fed pending.skippedLayoutIds. This PR adds a second source — SKIP_RETAINED_PREFETCH_LAYOUTS (app-page-render.ts:513) — which skips purely on client claim + element presence (retainedPrefetchLayoutIds ∩ Object.hasOwn(element, layoutId)), with no byte-identity proof. mergeSkipDispositions (app-page-render.ts:546) then merges both into a single skippedEntryIds list, so this preservation path now admits retained-prefetch ids whose justification is not the static-layout proof.
The behavior is still sound, but for a different reason than the comment states: retained layouts are reused under Next.js's segment-cache identity model (the client reuses a layout it already holds, keyed by segment identity), not because the server proved byte-identity. Please update the comment so a future reader doesn't rely on a guarantee that no longer holds for half the entries in this set. This is the same latent-trap concern raised for mergeSkipDispositions in the fourth review, now manifesting one layer up.
2. Retained-layout skip drops freshly rendered layout output — confirm this is the intended reuse contract
Tracing AppElementsWire.encodeOutgoingPayload → createSkippedLayoutIds (app-elements-wire.ts:661): the skip happens at encode time. The layout is fully rendered (its value is present in input.element), then omitted from the wire payload and its render dependency released (app-elements-wire.ts:674-678). So if a retained layout's content legitimately changed on the server (dynamic data behind Suspense, cookies/headers, time-based, non-param dynamic), the fresh output is discarded and the client refills with its previously held copy.
I confirmed this matches Next.js's documented "prefetch layout sharing" behavior — the shared-layout/layout.tsx fixture puts its dynamic content behind <Suspense> and shared layouts are reused across sibling navigations rather than refetched, which is exactly Next.js's intent. And the client guard canRetainLayoutForTargetParams (navigation.ts:418) correctly refuses to retain a dynamic-segment layout unless params match.
The point worth stating in the PR: this is a deliberate content-reuse optimization, not just a transport dedup — retained-layout skip can drop fresh non-param-dynamic layout content in favor of the client's held copy. That is correct for segment-cache parity, but it is a stronger semantic than "skip re-sending an identical layout." Worth a one-line note in the PR description / code so it isn't later mistaken for a bug and "fixed" by refetching.
3. Retained header + skip now applies to full/refresh RSC requests, not just runtime/static prefetch
getRetainedPrefetchLayoutIdsHeader is sent on real navigations and refresh (app-browser-entry.ts:1781, refresh mode at 1784-1785) and on full-after-shell prefetches (link.tsx:577), while createRetainedPrefetchLayoutSkipDisposition fires whenever isRscRequest && retainedPrefetchLayoutIds.length > 0 regardless of render mode. So the skip path is broader than the "runtime prefetch" framing in the PR title. I verified the refill fallbacks cover these paths (fillSkippedLayoutsFromPrefetchCache at app-browser-entry.ts:1891 for cached nav; mergeSkippedLayoutPreservation for the committed-state fallback), so nothing is dropped — but for a refresh, reusing a held layout is a semantic choice (refresh preserves UI, so this is consistent). Just flagging that the mechanism's reach exceeds the title; no change required.
Confirmed sound (this pass)
- Static-shell / runtime-page wrapping is correct.
wrapRuntimePrefetchableSubtreewraps a function-component node's children once, then wraps the component's render output once for a distinct returned tree — no double-invocation.wrapStaticPrefetchShellSubtreeswapsSuspensechildren for the (recursively wrapped) fallback.tests/app-page-route-wiring.test.ts:949verifies page content preserved, layout runtime content suppressed to fallback. Green. - Header parsing bounded/validated (
parseRetainedPrefetchLayoutIds, 64 ids / 512 chars, dedup,kind === "layout"); inVINEXT_RSC_VARY_HEADER+ cache-busting input. - E2E project move is right — segment-cache prefetch only exercises meaningfully against a prod build; grouping it with
client-cache/segment-cache-client-paramson port 4191 matches the existing pattern. unstable_instantmulti-file resolver, deadlayoutFlagsremoval,runWithPrerenderWorkUnit-per-request coverage,4629e22accept-set test, and themergeSkipDispositionsin-code note all remain in place.
Verdict: no blockers. One concrete ask (fix the now-inaccurate byte-identity comment at app-browser-state.ts:574) and two documentation clarifications (items 2 & 3). Design holds up and matches Next.js segment-cache semantics. LGTM once CI is green.
|
Review posted to PR #2475. Review completeFifth Findings (no blockers):
Verified sound: static-shell/runtime-page wrapping (no double-invoke, test green), bounded header parsing, and the E2E project move to the prod-build Verdict: no blockers. LGTM once CI is green. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: segment-cache runtime prefetch layout shells (sixth pass)
Focused this pass on the one commit that landed after the five prior /bigbonk reviews — d103675 "validate installed dev client references" — since the earlier passes never saw it. Ran the touched unit suites (app-page-route-wiring 44, entry-templates 41, link 121 — all green) and traced the new dev-only reference-validation plugin against @vitejs/plugin-rsc's internals. All prior reviews' items remain resolved.
1. New vinext:rsc-dev-reference-validation plugin has zero test coverage — index.ts:988
The plugin does three non-trivial things and nothing exercises them:
- Bypasses
rsc:reference-validation'sthis.error("invalid client reference")for three reference classes (installed vinext shims, the plugin'sremove-duplicate-server-csshelper, andnext/*package proxies) by returningexport {}from the pre-orderedload. - Provides a real
export * from "next/…"proxy body for\0virtual:vite-rsc/client-package-proxy/next/*, shadowing the RSC plugin's own handler (plugin-BK29Va7z.js:1438), which would otherwise crash withCannot read properties of undefined (reading 'exportNames')whenclientReferenceMetaMaphas no entry for that source.
The classifiers carry real logic — URLSearchParams parsing, decodeURIComponent, and a fairly specific node_modules/(.pnpm/…/)?vinext/dist/shims/*.js regex against normalizePathSeparators(...). That regex is the whole correctness surface for the installed-mode fix, and a silent miss (e.g. a future pnpm layout, a scoped path, or a shim served without the .js id shape) would regress to the pre-fix crash with no signal. Please add unit coverage that (a) a canonical installed-shim reference id classifies true, (b) a non-vinext node_modules client reference classifies false (so we don't blanket-suppress real invalid-reference errors), and (c) the client-package-proxy/next/* branch returns the proxy body.
2. effectiveGlobalErrorModule still injects the default reference the element-builder change was meant to drop — app-fallback-renderer.ts:179
The point of the app-page-element-builder.ts change is stated in the commit: pass globalErrorModule ?? null so GlobalErrorBoundary owns the built-in default internally, "threading that fallback as a normal RSC prop would serialize an otherwise unused client reference into every cold page payload."
But app-fallback-renderer.ts:179 still does the old thing:
const effectiveGlobalErrorModule =
globalErrorModule ?? (DEFAULT_GLOBAL_ERROR_MODULE as unknown as TModule);and feeds it into renderAppPageHttpAccessFallback → buildPageElements → buildAppPageElements (app-page-boundary-render.ts:394/426/513). On the fallback/HTTP-access-error paths with no user global-error.tsx, getErrorBoundaryExport is now truthy, so it takes the <ErrorBoundary fallback={default}> branch and re-serializes exactly the client reference the happy path just stopped serializing.
This is not a correctness bug — the wrapped UI is functionally identical, and these are the rare error/not-found paths, not cold happy-path pages — but it's an inconsistency worth an explicit decision: either align the fallback renderer to pass globalErrorModule ?? null and rely on GlobalErrorBoundary's default too, or add a one-line note that the fallback path intentionally still injects it (payload cost acceptable on error paths). As written a future reader will assume the two paths behave the same.
3. Minor: dead condition in the next/* classifier — index.ts:984 and index.ts:996
Both the load proxy branch and isNextClientPackageProxyReference guard with source === "next/image" || source.startsWith("next/"). The "next/image" disjunct is fully subsumed by startsWith("next/") (it's already next/image), so it's dead. Harmless, but drop it or comment why next/image is called out specifically.
Confirmed sound in d103675
- Plugin ordering is correct. vinext's plugin is
enforce: "pre", so itsloadruns before the RSC plugin's normal-orderedrsc:reference-validation/rsc:virtual-client-packagehandlers regardless of theplugins.pusharray position (index.ts:6293). The bypass and thenext/*proxy shadow both land first. GlobalErrorBoundarydefault relocation preserves behavior.fallback = DEFAULT_GLOBAL_ERROR_COMPONENTis the sameDefaultGlobalErroridentity, soisImplicitRootErrorBoundary={fallback === DEFAULT_GLOBAL_ERROR_COMPONENT}is equivalent to the old=== DefaultGlobalErrorcheck.getErrorBoundaryExport(null)→null→ the boundary falls through to its internal default. Tests updated accordingly (app-page-route-wiring.test.ts:2087/2122/2151) and green.apply: "serve"correctly scopes this to dev; the production build path (createBuilder/buildApp) is unaffected, so no risk to the deployed reference-validation semantics.
Residual (unchanged from prior passes, non-blocking)
- The build-time
unstable_instantobject/initializer scanners are heuristic string matchers (no comment stripping). Since the server path evaluates the real module, worst case is a missed prefetch optimization, never incorrect output. Acceptable.
Verdict: no blockers. The dev reference-validation fix is sound and correctly scoped to serve/installed mode. Two asks before merge: (1) add unit coverage for the new classifier plugin — it's currently the only untested new logic and its regex is the fix's entire correctness surface; (2) resolve or document the effectiveGlobalErrorModule inconsistency so the two render paths don't silently diverge on default-reference serialization. LGTM once CI is green.
|
Review posted to PR #2475. Review completeThis is the sixth Findings (no blockers):
Verified sound: plugin Verdict: no blockers. Two asks before merge (test the new plugin; resolve/document the fallback-path serialization divergence). LGTM once CI is green. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: segment-cache runtime prefetch layout shells (seventh pass)
Focused this pass on the one commit that landed after the six prior /bigbonk reviews — c62c606 "test(app-router): cover dev rsc reference validation" — since no earlier pass saw it. This commit directly addresses the two asks from the sixth review. Ran the touched unit suites (rsc-dev-reference-validation 5, app-page-route-wiring 44, entry-templates 41 — 90 tests, all green) and vp check on the four changed files (clean).
Sixth review's asks — both resolved
-
New reference-validation plugin now has unit coverage. The plugin was extracted from
index.tsintopackages/vinext/src/plugins/rsc-dev-reference-validation.tswithisInstalledVinextShimClientReference/isNextClientPackageProxyReference/createVinextRscDevReferenceValidationPluginexported for test.tests/rsc-dev-reference-validation.test.tscovers exactly the three cases I asked for:- canonical installed shim (both plain and
.pnpm/vinext@x.y.z/node_modules/...layouts, incl.?clientquery) →true(lines 31–45) - non-vinext node_modules (
react-dom/client.js) and the scoped@scope/vinext/...false-positive guard →false(lines 47–59), so real invalid-reference errors are not blanket-suppressed - the
client-package-proxy/next/*loadbranch returns theexport *proxy body, andreactreturnsnull(lines 92–105)
Plus a combinedbypasses validation ...case assertingexport {};\nfor shims + next proxies andnullforreact-dom. Good coverage of the entire correctness surface (the pnpm regex + the proxy shadow).
- canonical installed shim (both plain and
-
effectiveGlobalErrorModuledivergence is now documented (app-fallback-renderer.ts:169). The comment now explicitly states the fallback/error-boundary renderer must provide a concrete default global-error module because these paths render the fallback UI directly rather than relying on the outerGlobalErrorBoundary's implicit client fallback — passingnullwould leaverenderErrorBoundarywithout a response body. This turns the silent inconsistency into a deliberate, documented decision, which is the right resolution. -
Dead
next/imagecondition removed (item 3 from the sixth review). Both theloadproxy branch andisNextClientPackageProxyReferencenow guard onsource.startsWith("next/")alone. The extraction is otherwise byte-for-byte identical to the prior inline implementation.
Confirmed sound in c62c606
- Plugin registration/ordering intact — pushed at
index.ts:6217right after the RSC plugin promise, and it'senforce: "pre"+apply: "serve", so the bypass/next/*proxy still land beforersc:reference-validationand stay dev-scoped (productioncreateBuilder/buildApppath unaffected). Verified by the sixth review; extraction did not change it. - No dangling imports —
normalizePathSeparatorsremains used in ~25 otherindex.tssites;vp checkconfirms clean (no unused imports/type errors). - Test harness is honest —
loadPlugininvokesplugin.loadwith an emptythis, exercising the realURLSearchParams/decodeURIComponentclassifier logic rather than mocking it.
Residual (unchanged from prior passes, non-blocking)
- The build-time
unstable_instantobject/initializer scanners remain heuristic string matchers (no comment stripping). Since the server path evaluates the real module, worst case is a missed prefetch optimization, never incorrect output. Acceptable.
Verdict: no blockers. Both of the sixth review's asks landed cleanly with proper test coverage and documentation, and no new issues were introduced. All seven passes' actionable items are now resolved. LGTM once CI is green.
|
Review posted to PR #2475. Review completeSeventh Both sixth-review asks resolved:
Verified sound: plugin registration/ordering intact ( Verdict: no blockers. All seven passes' actionable items are now resolved. LGTM once CI is green. |
|
/bigbonk review for issues |
|
Review posted to PR #2475. Review completeNinth Eighth-review blocker resolved — reproduced both states:
Verdict: no blockers. The reproduced eviction regression is fixed with a real regression test, and all prior passes' items remain resolved. LGTM once CI is green. |
|
Deploy-suite run 28478866791 / report job 84413308650 triage: this PR appears to cover the non-excluded App Router layout-sharing/runtime-prefetch rows from Exact rows:
I am not counting excluded cacheComponents/use-cache rows here. |
james-elicx
force-pushed
the
codex/fix-segment-cache-prefetch-layout-sharing-28478866791
branch
from
5c6d50e to
f502a29
Compare
|
/bigbonk review for issues |
Performance benchmarksCompared 1 improved · 0 regressed · 5 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: retained-layout prefetch layout shells (tenth pass)
Reviewed the full diff at head f502a29. Ran the touched unit suites (prefetch-cache 41 — green) and vp check on the primary source files (app-browser-state.ts, app-page-render.ts, navigation.ts — clean). This pass re-audited the accumulated findings across the prior nine reviews and re-traced the retained-layout skip → client-refill → committed-state-preservation chain.
One unresolved item from the fifth review — stale soundness comment at app-browser-state.ts:574
The ninth review concluded "all prior passes' actionable items remain resolved," but the fifth review's one concrete ask — the now-inaccurate byte-identity comment in mergeSkippedLayoutPreservation — was never actually fixed. It still reads (app-browser-state.ts:574-577):
the server only emits a skip for a layout it proved byte-identical via the static-layout cache proof, so preserving the retained-and-identical layout … is sound regardless of ancestor-chain position.
That justification is no longer true for half the entries this path admits. Before this PR, pending.skippedLayoutIds was fed only by SKIP_STATIC_LAYOUT_VERIFIED (a byte-identity proof). This PR adds a second source, SKIP_RETAINED_PREFETCH_LAYOUTS (app-page-render.ts:513-544), which skips a layout purely on client claim + element presence (retainedPrefetchLayoutIds ∩ Object.hasOwn(element, layoutId)) with no byte-identity proof, and mergeSkipDispositions (app-page-render.ts:546-569) merges both into one skippedEntryIds set that flows into pending.skippedLayoutIds. So mergeSkippedLayoutPreservation now preserves layouts from current committed state whose skip was justified by the segment-cache identity model, not the static-layout proof the comment cites.
The behavior is still sound, but for a different reason than the comment states, and I verified the alternate soundness argument holds end to end:
- A retained-prefetch skip only happens for a layout the client advertised in
X-Vinext-Retained-Prefetch-Layouts, and the client only advertises a dynamic layout aftercanRetainLayoutForTargetParams(navigation.ts:420-429) confirms the cached snapshot params equal the resolved target params. So themergeSkippedLayoutPreservationfallback — which would otherwise preserve a same-layout-id-but-different-params dynamic layout from current committed state (currentLayoutIds.has(id) && targetLayoutIds.has(id) && Object.hasOwn(...), lines 578-579) — is only ever reached for layouts the client already validated as param-matching. The dynamic-param safety therefore lives entirely in the client-side advertisement guard, not in a server byte-identity proof.
Please update the comment so a future reader doesn't rely on a guarantee that no longer holds for retained-prefetch skips, and so the true invariant (dynamic-param safety is enforced by the client advertisement guard canRetainLayoutForTargetParams, not a server-side proof) is written down where the preservation happens. This is a comment/documentation fix, not a code change — but it's a real latent trap: anyone hardening this path later will assume every entry here carries a byte-identity proof and could weaken the client-side guard without realizing it is now load-bearing.
Confirmed resolved / sound this pass
- Eighth-review eviction blocker is genuinely fixed (
navigation.ts:762-766): the LRU loop re-readsgetPrefetchCacheByteSize(cache)at the loop head and dropped the stale localtotalSizedecrement, so cascade-freed dependent bytes are reflected before the next iteration. TheinspectedEntries < cache.sizeguard still bounds the loop.tests/prefetch-cache.test.tscovers the cascade-under-target case; green. - Same-target retention exclusion (
navigation.ts:529-534,5c6d50e) correctly stops a cached copy of target T from contributing layout ids to T's own retained header. The two normalizers use different bases (http://vinext.localvswindow.location.href) but both strip_rsc/.rscand comparepathname+search, so same-origin relative hrefs normalize consistently; the exclusion is a pure optimization (under-exclusion reverts to server-skip+refill, over-exclusion costs a redundant re-render). - Removed
isCurrentNavigation(navId)publish guard (app-browser-entry.ts:2174) is compensated by the five generation checks (2186/2189/2218/2239/2250); the snapshot is keyed by the navigation's ownnavResponseUrl, so it can't pollute a different in-flight target. - Cascade recursion in
deletePrefetchCacheEntry/deleteUnsatisfiedPrefetchLayoutDependents(navigation.ts:816-853) terminates — stale dependents are snapshotted before deletion and re-checked withcache.get(dependentKey) !== dependentEntry. - Multi-file
unstable_instantresolver, deadlayoutFlagsremoval,runWithPrerenderWorkUnit-per-request coverage,mergeSkipDispositionsin-code note,4629e22accept-set test, and the dev reference-validation plugin + tests all remain in place.
Residual (non-blocking, unchanged)
- The heuristic build-time
unstable_instantscanner (no comment stripping) remains acceptable since the server evaluates the real module — worst case is a missed prefetch optimization, never incorrect output.
Verdict: no hard blockers. One concrete ask carried over from the fifth review that was mislabeled as resolved: fix the stale byte-identity comment at app-browser-state.ts:574 so the true (client-side-guard) invariant is documented. The code is correct; the comment is a latent trap. LGTM once CI is green.
|
Review posted to PR #2475. Review completeTenth Key finding — one item mislabeled as resolved: The ninth review claimed "all prior passes' actionable items remain resolved," but the fifth review's one concrete ask was never actually fixed: the stale byte-identity soundness comment in I verified the behavior is still sound via a different invariant: the dynamic-param safety now lives entirely in the client-side Verified sound this pass: the eighth-review eviction blocker fix (byte-size re-read at loop head), the same-target retention exclusion, the removed publish guard (compensated by generation checks), and cascade-recursion termination. Verdict: no hard blockers. One carried-over comment fix to land before merge. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: retained-layout prefetch layout shells (eleventh pass)
Reviewed at head 40c7ac2. Ran the touched unit suites (prefetch-cache 41, app-page-route-wiring 44, app-optimistic-routing 13, link 121, entry-templates 41 — 260 tests, all green) and vp check on app-browser-state.ts (clean). This pass focused on the one commit that landed after the tenth review — 40c7ac2 "document retained layout skip invariant" — which directly addresses the tenth review's carried-over ask.
Tenth-review carried-over ask — now resolved
The tenth review flagged that the fifth review's concrete ask (the stale byte-identity soundness comment in mergeSkippedLayoutPreservation) had been mislabeled as resolved and was still wrong. Commit 40c7ac2 fixes it (app-browser-state.ts:574-579). The comment no longer claims the server proves byte-identity for every skipped layout; it now states the true invariant:
skipped layouts can come from either the server's static-layout byte-identity proof or from a retained-prefetch client claim. For retained-prefetch skips, dynamic-param safety is enforced by the client advertisement guard before the layout id is sent…
I verified the referenced invariant holds in code: canRetainLayoutForTargetParams (navigation.ts:420-429) is the client-side guard, and for a dynamic layout it returns false unless targetParams !== null and areRouteParamsEqual(snapshotParams, targetParams). getRetainedPrefetchLayoutIdsHeader gates every advertised id through that guard (navigation.ts:538) before sending X-Vinext-Retained-Prefetch-Layouts. So the mergeSkippedLayoutPreservation current-state fallback (app-browser-state.ts:580-581) is only ever reached for layouts the client already validated as param-matching. The comment now documents the load-bearing guard where it matters, closing the latent trap. Correct resolution — comment-only change, no behavior shift.
Independently re-verified this pass
- Eighth-review eviction blocker fix is present and correct (
navigation.ts:762-766): the LRU loop re-readsgetPrefetchCacheByteSize(cache)at the loop head; the stale localtotalSizedecrement is gone (totalSizenow only feeds the two cheap early-exit checks at 748/760). After a cascade deletion frees a large dependent, the loop head sees the accurate size and stops instead of over-evicting survivors. The targeted regression test (prefetch-cache.test.ts:793"stops byte LRU cleanup after retained-layout cascade deletion frees enough memory", 1 MiB source + 40 MiB dependent + 6×2 MiB survivors) covers exactly the reproduced failure. Green. - Same-target retention exclusion normalizer bases (
navigation.ts:547/557): the two normalizers use different bases (http://vinext.localvswindow.location.href) but both strip the_rsccache-busting param +.rscsuffix and compare onlypathname+search, which is base-independent for the same-origin relative/absolute hrefs in play. The exclusion is a pure optimization — under-exclusion reverts to server-skip+refill, over-exclusion costs a redundant re-render. No correctness dependency on the base mismatch. unstable_instantclient/server split remains safe: the server path evaluates the real module (app-page-route-wiring.tsx:108,module.unstable_instant), while the client build-time scanner (app-browser-entry.ts) is heuristic. A scanner miss affects only the client prefetch optimization, never the authoritative render — worst case a missed prefetch.
Residual (non-blocking, unchanged)
- The heuristic build-time
unstable_instantobject/initializer scanner still doesn't strip comments. Acceptable for the reason above (server evaluates the real module).
Verdict: no blockers. The one carried-over item from the fifth/tenth reviews (the stale byte-identity comment) is now genuinely fixed and the documented invariant matches the code. The eighth-review eviction regression fix and its regression test are in place. All prior passes' actionable items are resolved. LGTM once CI is green.
|
Review posted to PR #2475. Review completeEleventh Tenth-review carried-over ask — now resolved: Independently re-verified:
Verdict: no blockers. The one carried-over item is now truly fixed and the documented invariant matches the code. LGTM once CI is green. |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: retained-layout prefetch layout shells (twelfth pass)
Twelfth /bigbonk pass at head b828968. Focused on the two commits that landed after the eleventh review (40c7ac2) — 5718c10 "detect typed instant prefetch exports" and b828968 "parse typed runtime prefetch config" — since no earlier pass saw them. Ran the touched unit suites (entry-templates 41, link 121, prefetch-cache 41, app-page-route-wiring 44 — 247 tests, all green) and vp check on the two changed files (clean).
These two commits directly harden the long-standing "heuristic scanner" residual
Every prior pass (second through eleventh) flagged the build-time unstable_instant client scanner as an acceptable-but-heuristic residual. These commits shrink that gap for the most common real-world shape it previously missed: a TypeScript type annotation on the const declaration.
5718c10widens thefindConstInitializerregex to tolerateconst x: Type =(via(?:\s*:[^=;]+)?), with atyped-runtime-prefetchablefixture (export const unstable_instant: Instant = { prefetch: "runtime" }).b828968replaces that regex-based annotation skip with a proper brace/bracket/paren/angle-depth-aware scanner (readConstInitializerStart) that walks past the type annotation to the real initializer=, plus aninline-typed-runtime-prefetchablefixture (export const unstable_instant: { prefetch: "runtime" | "static"; } = ...) that the simpler[^=;]+regex would have failed (the inner;and|break the character class approach).
Verified readConstInitializerStart handles the realistic edge cases
I reproduced the scanner standalone and confirmed it correctly resolves the initializer for:
- plain
const x = {…}and annotatedconst x: Instant = {…} - inline object type with nested
;and union —const x: { prefetch: "runtime" | "static"; } = {…}(the;is guarded bybraceDepth > 0) - generics incl. arrow-in-generic and generic defaults —
Record<string, () => void>,Array<Map<string, number>>,Foo<T = string>(the=inside<…>is guarded byangleDepth > 0) - arrow-function value —
const x: () => void = () => {}correctly returns the() => {}initializer (arrow=>is skipped viasource[index + 1] !== ">") - conditional types —
T extends U ? A : B
The \b word-boundary change to the const-name regex (5718c10) correctly rejects longer identifiers: searching unstable_instant does not false-match export const unstable_instant_foo = …, and still finds the real declaration when a similarly-named one precedes it.
The only input that returns null is a malformed unclosed generic (const x: Partial<Foo = {…}), which isn't valid TypeScript and wouldn't compile — so it degrades to a missed prefetch, never incorrect output. That is exactly the safe-fail contract every prior pass relied on: this is a client-side prefetch optimization and the server path evaluates the real module (app-page-route-wiring.tsx, module.unstable_instant), so the authoritative render is unaffected.
Prior passes
All eleven prior reviews' actionable items remain resolved (multi-file unstable_instant resolver, dead layoutFlags removal, runWithPrerenderWorkUnit-per-request coverage, mergeSkipDispositions documentation, 4629e22 accept-set test, dev reference-validation plugin + tests, the eighth-pass eviction blocker fix + regression test, and the tenth/eleventh-pass byte-identity comment fix). Nothing regressed.
Residual (non-blocking, now smaller)
readObjectExpressionPrefetchMode/readInitializerExpressionstill don't strip comments, so a pathological{ /* prefetch:"runtime" */ prefetch:"static" }is handled by adjacency luck rather than parsing. Unchanged from prior passes and still acceptable for the same reason (client optimization only; server evaluates the real module).
Verdict: no blockers. These two commits are a net improvement — they close the most common annotated-const miss in the client scanner with a robust depth-aware walker and targeted fixtures, without touching the authoritative server path. LGTM once CI is green.
|
Review posted to PR #2475. Review completeTwelfth What these commits do: They directly harden the long-standing "heuristic
Verified independently by reproducing No blockers. Net improvement to the client scanner without touching the authoritative server path (which evaluates the real module). All eleven prior passes' items remain resolved. LGTM once CI is green. |
…he-prefetch-layout-sharing-28478866791 # Conflicts: # packages/vinext/src/shims/link.tsx # packages/vinext/src/shims/navigation.ts # packages/vinext/src/shims/next-shims.d.ts
1 participant
Summary
prefetch-layout-sharingactive failures from GitHub Actions run 28478866791 / job 84413308650unstable_instantconstants and covers normal request work-unit activationOriginal active failure rows
full prefetches should omit layouts that were already prefetched with a full prefetchnavigations should omit layouts that were already prefetched with a full prefetchsegment-level prefetch config > does not unnecessarily use a runtime prefetch for sub-pages of runtime-prefetchable layoutssegment-level prefetch config > statically prefetches a fully-static page segment if all its runtime-prefetchable parents are availablesegment-level prefetch config > uses a runtime prefetch for sub-pages of runtime-prefetchable layouts if requestedNo new skip rows were added. The five skipped rows in the target upstream file remain the upstream
it.skip(...)runtime-prefetch cases; that upstream file documents runtime prefetch asunstable_instantsegment config available only incacheComponentsmode, including cached IO /use cache: privatecoverage.Validation
vp check packages/vinext/src/client/vinext-next-data.ts packages/vinext/src/entries/app-browser-entry.ts packages/vinext/src/server/app-browser-entry.ts packages/vinext/src/server/app-optimistic-routing.ts packages/vinext/src/server/app-page-dispatch.ts packages/vinext/src/server/app-page-render.ts packages/vinext/src/server/app-page-route-wiring.tsx packages/vinext/src/server/app-rsc-render-mode.ts packages/vinext/src/server/prerender-work-unit-setup.ts packages/vinext/src/shims/link.tsx packages/vinext/src/shims/navigation.ts packages/vinext/src/shims/ppr-fallback-shell.ts packages/vinext/src/shims/server.ts tests/app-optimistic-routing.test.ts tests/app-page-route-wiring.test.ts tests/entry-templates.test.ts tests/ppr-fallback-shell.test.ts tests/prefetch-cache.test.ts tests/prerender-work-unit-setup.test.tsvp test run tests/app-page-route-wiring.test.ts tests/app-optimistic-routing.test.ts tests/ppr-fallback-shell.test.ts tests/prefetch-cache.test.ts tests/entry-templates.test.ts tests/prerender-work-unit-setup.test.ts -t "static layout fallbacks|loading-shell prefetches|fills skipped retained layouts|request API bypass|retained|runtime-prefetch|runWithPrerenderWorkUnit"REPO="$(pwd)" NEXTJS_DIR="/Users/jamesanderson/Developer/vinext/.nextjs-ref" NEXT_TEST_CONCURRENCY=1 ./scripts/run-targeted-nextjs-e2e.sh test/e2e/app-dir/segment-cache/prefetch-layout-sharing/prefetch-layout-sharing.test.ts