feat(skip): enable proven static layout transport skips#1437
feat(skip): enable proven static layout transport skips#1437NathanDrake2406 wants to merge 23 commits into
Conversation
Authorize skip transport only after a parsed client reuse manifest entry cross-checks against a public static layout cache proof. Add a skip transport planner that preserves rejection traces, falls back to render/send for absent, rejected, oversized, or over-budget manifests, and uses canonical parsed entry ids rather than verifier-provided ids. Wire enabled static-layout skip disposition into the RSC payload encoder so only proven layout records are omitted. HTML responses and non-layout entries still render and send the full tree.
commit: |
Canonical RSC requests could parse X-Vinext-Client-Reuse-Manifest but still send full AppElements payloads. The parsed manifest stopped at the App RSC handler, so dispatch and render never had the proof input needed to plan static layout omission. Thread the parsed manifest through generated App RSC dispatch into dispatchAppPage and renderAppPageLifecycle, then build the skip disposition from current static layout classifications and layout-scoped proof metadata before encoding the RSC payload. HTML renders still ignore skip dispositions. Tests cover header parsing into dispatch, generated entry wiring, static-only dispatch omission, dynamic fallback, and HTML full-payload behaviour.
Verified skip transport can omit layout records for a specific client. Caching that partial RSC stream under the normal route key is unsafe because later clients may not have the omitted layout locally. Bypass RSC capture and ISR cache scheduling when skip transport is enabled, force the client response to no-store, and emit a debug trace for the skipped cache write. Tests cover the render lifecycle cache boundary and the manifest-driven dispatch path.
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: feat(skip): enable proven static layout transport skips
The architecture is sound. The planner stays untrusted-at-parse / verified-at-runtime, the cache bypass is correctly wired end-to-end, and the test coverage is thorough. The inline comments below are refinements, not blockers.
What's good
- Trust boundary is clean. Parser disposition stays
renderAndSend; only the runtime planner can upgrade toSKIP_STATIC_LAYOUT_VERIFIEDafter server-side verification. The hardening from #726 is preserved. - Cache bypass is correct.
shouldCaptureRscForCacheMetadatais gated off,capturedRscDataPromiseis null,scheduleAppPageRscCacheWritereturns false,finalizeAppPageRscCacheResponsereturns the response unchanged, andno-store, must-revalidateis set. No shared cache poisoning path. - Layout-only omission guard is correct.
createSkippedLayoutIdsre-parses each entry ID throughparseAppElementsWireElementKeyand only admitskind: "layout", so a malicious manifest that smuggles a page/template/route entry ID intoskippedEntryIdscannot omit non-layout content. - Budget enforcement — the
maxEntriesToVerifycheck rejects over-budget manifests before the verifier loop runs. The test for this is explicit. - Entry ID hardening —
skippedEntryIdsis populated fromentry.id(the canonical parsed entry), not from verifier-returned IDs. The "uses the verified manifest entry id instead of trusting verifier-provided skipped ids" test proves this. - HTML path is untouched — skip disposition is only passed to
encodeOutgoingPayloadfor RSC requests, and the dispatch test proves HTML responses still contain the full tree.
Nits and suggestions below.
|
Review submitted. Here's a summary of what I found: Overall: The PR is solid. The trust boundary design from #726 is correctly preserved — parse-time disposition stays 8 inline comments posted, all non-blocking refinements:
|
Browser navigation could only benefit from skip transport when an external caller manually supplied X-Vinext-Client-Reuse-Manifest. That left normal App Router soft navigations rendering full layout payloads even when the current visible state retained a proven public static layout. The missing boundary was the browser request path. Build a bounded manifest from visible AppElements metadata, reuse the same static-layout compatibility and payload-hash proof contract as server verification, and attach it through createRscRequestHeaders only for navigation fetches. Also folds in Bonk review refinements around skip planner policy ownership, invalid verification budgets, synthetic static-layout observations, no-store cache semantics, and dispatch-level waitUntil coverage.
|
To use Codex here, create an environment for this repo. |
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR enables verified static-layout skip transport for App Router RSC navigations by introducing an untrusted client reuse manifest header, parsing it at the canonical RSC boundary, and upgrading to an enabled skip disposition only after server-side proof cross-checks at the render lifecycle boundary. It also ensures skipped (client-specific) RSC payloads bypass shared caching and are returned with Cache-Control: no-store, must-revalidate.
Changes:
- Add a browser-side producer for a bounded
X-Vinext-Client-Reuse-Manifestderived from visible retained static layout entries, and attach it to real RSC navigation requests. - Add server-side skip transport planning/proofing to upgrade from parsed (disabled) disposition to
SKIP_STATIC_LAYOUT_VERIFIED, and omit only proven layout records from RSC payload encoding. - Add extensive wiring + boundary tests (planner behavior, request normalization/dispatch plumbing, cache bypass/no-store behavior, and e2e header emission).
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/skip-cache-proof.test.ts | Expands unit coverage to assert verified entries produce enabled skip plans and that budgets/oversize/malicious cases fall back safely. |
| tests/e2e/app-router/build-id-navigation.spec.ts | Adds Playwright coverage proving RSC soft navigations send a bounded client reuse manifest header. |
| tests/app-rsc-handler.test.ts | Verifies parsed client reuse manifest hints are threaded from canonical RSC requests into page dispatch. |
| tests/app-rsc-cache-busting.test.ts | Ensures manifest header is attached without becoming a shared cache-variant input. |
| tests/app-router.test.ts | Asserts generated dispatchMatchedPage wiring includes clientReuseManifest threading. |
| tests/app-page-render.test.ts | Adds render lifecycle coverage for skip omission, no-store cache policy, and HTML non-omission behavior. |
| tests/app-page-dispatch.test.ts | Adds dispatch-boundary integration coverage for verified static-layout omission (RSC only) and cache bypass. |
| tests/app-elements.test.ts | Validates outgoing payload builder omits only layout keys when skip is enabled and preserves metadata/layout flags. |
| tests/app-browser-client-reuse-manifest.test.ts | New tests for browser manifest construction (static-only, retained-only) and byte-budget trimming. |
| packages/vinext/src/server/static-layout-client-reuse-proof.ts | Introduces shared proof helpers for static-layout compatibility shaping + payload hash computation. |
| packages/vinext/src/server/skip-cache-proof.ts | Adds skip transport planner (createClientReuseSkipTransportPlan) and upgrades verified cross-checks to enabled skip disposition. |
| packages/vinext/src/server/client-reuse-manifest.ts | Extends skip disposition type to include SKIP_STATIC_LAYOUT_VERIFIED (enabled) alongside disabled mode. |
| packages/vinext/src/server/app-rsc-handler.ts | Threads clientReuseManifest through handleAppRscRequest to dispatch boundary. |
| packages/vinext/src/server/app-rsc-cache-busting.ts | Allows createRscRequestHeaders() to attach the manifest header when provided. |
| packages/vinext/src/server/app-page-render.ts | Builds runtime skip disposition at render lifecycle boundary; omits proven layouts from RSC payload and bypasses RSC cache writes (no-store). |
| packages/vinext/src/server/app-page-dispatch.ts | Threads clientReuseManifest through dispatch into render lifecycle. |
| packages/vinext/src/server/app-elements-wire.ts | Applies skip disposition during outgoing payload encoding by filtering proven layout keys only. |
| packages/vinext/src/server/app-browser-entry.ts | Attaches the client reuse manifest header to real navigation RSC fetches (prefetch unchanged). |
| packages/vinext/src/server/app-browser-client-reuse-manifest.ts | New browser helper to construct a bounded manifest from visible AppElements metadata. |
| packages/vinext/src/entries/app-rsc-entry.ts | Updates generated entry wiring to pass clientReuseManifest into app page dispatch/render. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58dfd28128
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Skip transport sends partial RSC payloads only for clients that already hold retained static layouts. Treating those payloads as restorable visited-cache entries, sending manifests on refresh, or skipping layouts marked dynamic by runtime segment config breaks that invariant. The previous wiring relied on generic payload metadata and build-time classifications, while hard-navigation recovery had drifted around the navigation controller guard. Record skipped layout ids in AppElements metadata, deny visited-cache restoration for skip-pruned payloads, preserve skipped layouts from current state on approved commits, restrict browser manifests to navigate requests, honor runtime dynamic segment config, and route hard navigations through the controller.
Compatibility-map acceptance lets a retained layout hint verify across compatible canary and rollback deployments, but it does not prove byte-for-byte transport identity. Treating that accepted compatibility as permission to omit layout bytes breaks the cache proof boundary when deployments only share a declared compatibility set. Keep the cross-check verified in that case, but return the disabled skip disposition unless the manifest entry compatibility exactly matches the current artifact compatibility. Add regression coverage for shared and stale canary/rollback compatibility maps.
…05-static-layout-skip-transport # Conflicts: # tests/skip-cache-proof.test.ts
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b04d9b7273
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
/bigbonk review |
There was a problem hiding this comment.
Review: feat(skip): enable proven static layout transport skips
I read the full diff, the #726 architectural spec, and the prior reviews from Bonk, Copilot, and Codex. The architecture is well-designed and the implementation is disciplined. I'm focusing on areas the other reviews didn't cover or where I disagree with their findings.
Correctness assessment
The end-to-end trust boundary is sound:
- Parse-time disposition stays disabled.
parseClientReuseManifestHeaderalways returnsenabled: false. OnlycreateClientReuseSkipTransportPlanat render time can upgrade toSKIP_STATIC_LAYOUT_VERIFIED. - Cache bypass is complete.
shouldCaptureRscForCacheMetadatais false,capturedRscDataPromiseis null,waitUntilis never called, andno-store, must-revalidateis set. - Visited-cache poisoning is prevented.
isCacheRestorableAppPayloadMetadatareturns false whenskippedLayoutIds.length > 0(line 157 ofapp-browser-state.ts), so skip-pruned payloads are never stored. The Codex P1 comment about visited-route cache keying is a false positive — the store path is gated off entirely. - Layout-only omission guard is structurally correct.
createSkippedLayoutIdsre-parses throughparseAppElementsWireElementKeyand only admitskind: "layout". - Render dependency release on skip is correct.
releaseAppElementRenderDependencyis called for skipped entries so downstreamrenderAfterAppDependenciesbarriers don't hang. - CDN cache coherence is correct. The manifest header is intentionally excluded from
createCacheBustingInputandVINEXT_RSC_VARY_HEADER, so CDN caches are not manifest-dimensioned. A CDN-cached full response is harmlessly served to manifest-bearing clients.
Param scope design is the strongest part
The structural param scope tracking (via resolveAppPageSegmentParamScopeKeys) is the right conservative approach. It blocks skip for any layout whose ancestor segments include dynamic, catch-all, or optional catch-all params — regardless of whether the layout component actually reads them. This is correct because:
- A layout under
[slug]could receiveparamsand pass them to children without reading them itself - The probe-time observation only catches direct reads, not pass-through
- Optional catch-all segments whose runtime value is
[]/{}still have non-empty structural scope
The three-gate rejection order in rejectParamDependentStaticLayout (incomplete observation → non-empty scope → observed reads) is clean and each gate has a distinct rejection code for traceability.
On the Codex P2 concern about synthetic observations
The second Codex comment about cacheTags: [] and dynamicFetches: [] in synthetic observations raises a real concern worth tracking. A layout that does await fetch(url, { next: { tags: ['config'] } }) without calling request APIs would be classified "s" at probe time, but revalidateTag('config') would change its output. The synthetic observation with cacheTags: [] means the skip proof doesn't know about the config tag. This is a known scope limitation for this PR (no tag-based invalidation tracking for skip proof), but it should be tracked as a future correctness gap.
Items below — all non-blocking refinements
See inline comments.
Static-layout skip could over-share or under-observe probe dependency state. Fetch tags, cacheable fetch URLs, dynamic fetch URLs, and cacheLife were read from request-wide collections, so dependencies from one layout probe could be attributed to later probes. The violated invariant was that skip eligibility must be proven from the layout subtree being considered, not from neighbouring probes or from wrapper shapes the probe cannot see. Isolate each layout probe's dependency observation in a child request scope, render through React.lazy wrappers, and count array and iterable branches against the existing subtree depth and node budgets. Tests cover per-layout dependency isolation, lazy cache-tagged layout children preserving the RSC layout record, and array depth and node limit enforcement.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6de623645a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Browser soft navigation could emit more retained layout entries than the server skip planner will verify. When a valid manifest exceeded that verification budget, the server rejected the whole hint with SKIP_ENTRY_COUNT_EXCEEDED and skip transport stayed disabled for routes with many retained static layouts. The mistaken invariant was treating the parser's hostile-input entry limit as the producer budget. Keep the parser envelope larger, share the server verification budget through the manifest protocol module, and cap browser-produced manifests to that budget before byte-size trimming. The regression test builds a route with more retained static layouts than the verification budget and asserts the browser manifest only contains the verifiable prefix.
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Static-layout skip could still authorize omission after a layout probe observed request APIs. Build-time or segment-config static classification can skip isolated dynamic detection, so a retained layout that reads headers(), cookies(), draftMode(), connection(), params, or search params needed an independent runtime negative proof before bytes could be omitted. The violated invariant was that skip proof must be based on evidence owned by the per-layout observation tracker, not a fabricated empty request API observation. Record request API usage inside each isolated layout probe, reject observed request APIs before static-layout proof construction, and feed the completed per-layout request API observation into the synthetic cache proof. Tests cover isolated request API tracking across layout probes and a dispatch regression where a static-classified layout probe observes headers usage and the RSC payload still includes the layout.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1863f07ab5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Layout subtree probing consumed arbitrary iterable children before the real RSC render. If an app returned a single-use iterator or shared generator from a layout subtree, the probe could advance it and leave the real render with missing children. The mistaken invariant was treating all iterables like arrays. Arrays are stable values to traverse, but arbitrary iterables can be stateful, so the probe now rejects non-array iterables without advancing them. That keeps the real render authoritative and makes skip fall back instead of mutating output. The regression verifies a shared generator is still unconsumed after the subtree probe rejects it.
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Layout probes could record unsafe static-reuse observations while still publishing an "s" layout flag. That was safe for server skip authorization, but it let the browser keep proposing manifest entries for layouts that could never be accepted. The violated invariant was that client-facing layout flags should be derived from the same per-layout evidence used by skip authorization. Treat incomplete observations, structural params, observed params, request APIs, finite revalidate, cacheLife, cache tags, cacheable fetches, and dynamic fetches as dynamic for layout metadata. Tests cover both build-time-static false positives and runtime-probed request API layouts publishing "d" while still rendering and sending the layout.
|
@codex review |
Layout skip probes rendered layouts with children set to null. That could hide layout chrome dependencies behind children truthiness checks, producing a clean static observation for a layout whose real render would observe request or data dependencies. The violated invariant was that per-layout skip evidence must be gathered under a child shape equivalent to the real App Router render. Probe layouts now receive a terminal non-null React child, while the subtree probe treats it as inert instead of descending into page content. This also restores build-time layout flags before special-error probes so short-circuit responses keep the same metadata contract covered by CI. Tests cover children-dependent chrome observations and the special-error flag regression.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Static layout skip could treat layouts that read unstable_cache() data as clean. That was unsafe because cached data can be tagged, revalidated, and invalidated without passing through the fetch, cacheLife, request API, or params observation channels. The violated invariant was that skip proof must observe every layout data dependency source before synthesizing a static-layout proof. unstable_cache() now records a bounded observation for every invocation, including cache hits, using hashed keys and tag metadata rather than raw keys or values. Layout observation carries those dependencies into skip rejection and client-visible layout flags. Tests cover cache-miss and cache-hit observation plus dispatch fallback for a retained static layout whose probe reads unstable_cache() data.
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
/bigbonk review |
There was a problem hiding this comment.
Review: feat(skip): enable proven static layout transport skips
I read the full diff (4154 additions, 43 files), the #726 architectural context, and all prior review threads from Bonk, Copilot, and Codex. The overall architecture is strong — the trust boundary is clean, the cache bypass is complete, and the observation isolation design is well-thought-out. The test coverage is thorough (7500+ lines of new/modified tests across 15 files).
Correctness summary
Trust boundary — Parse-time disposition stays renderAndSend (disabled). Only createClientReuseSkipTransportPlan at render time can upgrade to SKIP_STATIC_LAYOUT_VERIFIED. The skippedEntryIds array is populated from entry.id (canonical parsed manifest entry), not from verifier-returned IDs, preventing injection. The "uses the verified manifest entry id instead of trusting verifier-provided skipped ids" test in skip-cache-proof explicitly covers this.
Cache bypass — Complete end-to-end. When skip is enabled: shouldCaptureRscForCacheMetadata is false, capturedRscDataPromise is null, waitUntil is never called, no-store, must-revalidate is set, and the absence of cacheState in rscResponsePolicy correctly causes preserveClientResponseHeaders: true in finalizeAppPageRscCacheResponse, preventing the no-store header from being overwritten.
Visited cache — Skip-pruned payloads carry skippedLayoutIds in metadata. isCacheRestorableAppPayloadMetadata returns false when skippedLayoutIds.length > 0 (line 157 of app-browser-state.ts), gating off the visited-response store path entirely. Codex P1 about visited cache keying was a false positive, as the author and Bonk both confirmed.
Observation isolation — runWithUnifiedStateMutation creates fresh cacheableFetchUrls, currentRequestTags, dynamicFetchUrls, dynamicUsageDetected, renderRequestApiUsage, requestScopedCacheLife, and unstableCacheObservations containers per layout probe. Dependencies are recorded via recordProbeDependencies inside the isolated scope before it exits. Codex P2 and Copilot correctly identified the pre-isolation accumulation bug; the fix in 6de6236 is sound.
Render dependency release — releaseAppElementRenderDependency is correctly called in buildOutgoingAppPayload for skipped entries, preventing downstream page/layout render barriers from deadlocking. The WeakMap-based registration in app-page-route-wiring.tsx and release in app-elements-wire.ts form a clean lifecycle.
Subtree probe — Handles function components, React.memo, React.forwardRef, and React.lazy (via thrown-promise resolution). Array branches correctly count against depth/node limits. Non-array iterables are rejected without advancing them. The wrapperDepth counter prevents infinite loops through nested wrapper chains.
Items below — all non-blocking
See inline comments for specific code-level observations.
| }, | ||
| (error: unknown) => { | ||
| recordProbeDependencies(layoutId); | ||
| throw error; |
There was a problem hiding this comment.
The error path records dependencies (recordProbeDependencies) but does not call markProbeComplete, leaving the observation at completeness: "unknown". This is the correct conservative outcome (blocks skip via SKIP_LAYOUT_PARAMS_OBSERVATION_INCOMPLETE), as discussed in the prior Bonk review thread.
However, there's a subtlety worth noting: recordProbeDependencies on the error path means the observation will contain whatever fetch tags/cache reads/etc the layout accumulated before it threw. These partial dependency snapshots are never used for skip authorization (since completeness blocks it), but they are surfaced in the rejection's fields object via rejectStaticLayoutObservation. This is fine for diagnostics — just noting it's not dead data in the trace output.
| if (value == null || typeof value === "boolean" || typeof value === "number") return; | ||
| if (typeof value === "string" || typeof value === "bigint") return; | ||
| if (isPromiseLike(value)) { | ||
| await visit(await value, depth); |
There was a problem hiding this comment.
Awaited promises don't increment depth — await visit(await value, depth) re-visits at the same depth as the promise itself. This means a chain of nested promises (Promise.resolve(Promise.resolve(Promise.resolve(<Component />)))) would visit N+1 nodes without increasing depth, potentially allowing deeper-than-maxDepth effective nesting through promise chains.
In practice, React doesn't produce arbitrarily nested promise chains in server component output, and the maxNodes counter still bounds the total work. But if you want the depth invariant to be airtight, this could use depth + 1.
Non-blocking — the node limit catches any real abuse.
| pathTags: [input.cleanPathname], | ||
| requestApis: buildRenderRequestApiObservations({ | ||
| completeness: "complete", | ||
| observed: input.layoutParamAccess?.getLayoutObservation(entry.id).requestApis ?? [], |
There was a problem hiding this comment.
The requestApis field in the synthetic observation reads from layoutParamAccess?.getLayoutObservation(entry.id).requestApis ?? []. But by this point, rejectUnsafeStaticLayoutObservation (line 447) has already verified that observation.requestApis.length === 0 — otherwise the function would have returned an early rejection at line 342. So this will always be [] when it reaches here.
Not a bug, but this creates a subtle coupling: the correctness of the synthetic observation depends on the ordering of rejection checks in rejectUnsafeStaticLayoutObservation. If someone adds a new rejection check between the request API check and the synthetic observation, and that new check somehow passes despite request APIs being present, the synthetic observation would carry stale data.
Consider either:
- Hardcoding
requestApis: buildRenderRequestApiObservations({ completeness: "complete", observed: [] })with a comment explaining the guarantee - Or adding an assertion:
assert(observation.requestApis.length === 0)
Non-blocking.
| treePosition, | ||
| options.matchedParams, | ||
| ); | ||
| return probeReactServerSubtree( |
There was a problem hiding this comment.
The probe renders with probeReactServerSubtree(<LayoutComponent ... >{APP_PAGE_LAYOUT_PROBE_CHILD}</LayoutComponent>) where APP_PAGE_LAYOUT_PROBE_CHILD = <Fragment />. This is a solid approach — it exercises the children-truthy branch in layouts that conditionally render chrome based on whether children are present.
One nuance: if a layout destructures children and checks its type (e.g., if (typeof children.type === 'string')), the Fragment child would behave differently than a real page component child. This is an unlikely pattern in real layouts, and the probe would still correctly observe any data dependencies triggered by the truthy-children branch, so skip safety is maintained. Just noting the semantic gap.
| ): ClientReuseManifestLimits { | ||
| return { | ||
| ...limits, | ||
| maxEntryCount: Math.min( |
There was a problem hiding this comment.
Good design: capClientReuseManifestProducerLimits ensures the browser never produces more entries than the server's verification budget (CLIENT_REUSE_MANIFEST_SKIP_VERIFICATION_ENTRY_BUDGET = 8). This prevents the guaranteed SKIP_ENTRY_COUNT_EXCEEDED rejection that Codex P2 identified.
The limit is applied as Math.min(limits.maxEntryCount, budget) — this means if the default limit (64) is larger than the budget (8), the budget wins. If someone configures a custom limit below 8, that custom limit wins. Both directions are correct.
| return { | ||
| kind: "dynamic", | ||
| reason: { layer: "segment-config", key: "dynamic", value: "force-dynamic" }, | ||
| }; |
There was a problem hiding this comment.
Good addition: classifyLayoutSegmentConfigFromModule reads dynamic and revalidate exports from layout modules at runtime. This closes the gap where a layout with export const dynamic = 'force-dynamic' at the module level wouldn't be reflected in build-time classifications.
Note that createEffectiveLayoutClassifications merges these runtime classifications by overriding build-time ones (line 285: classifications.set(index, classification.kind)). This means a runtime force-static will override a build-time dynamic classification. I verified this is the correct precedence — segment config exports are the developer's explicit intent and should win over build-time heuristic analysis.
| releaseAppElementRenderDependency(input.element, key); | ||
| continue; | ||
| } | ||
| payload[key] = value === UNMATCHED_SLOT ? APP_UNMATCHED_SLOT_WIRE_VALUE : value; |
There was a problem hiding this comment.
The iteration-based approach (replacing the previous spread) is cleaner and enables skip filtering in a single pass. The UNMATCHED_SLOT → APP_UNMATCHED_SLOT_WIRE_VALUE conversion that previously happened somewhere else is now handled inline here.
One observation: the ordering of keys in the output payload is now:
- Non-skipped element entries (in source iteration order)
__layoutFlags__skippedLayoutIds(if non-empty)__artifactCompatibility__cacheEntryReuseProof(if present)__renderObservation(if present)
Previously, the spread put all entries first with __layoutFlags and __artifactCompatibility interleaved. The new order places metadata keys at the end. This shouldn't affect RSC serialization (Flight doesn't depend on key order), but it's a subtle change in wire format that downstream snapshot tests might notice. The existing tests passing confirms this is fine.
| ); | ||
| } | ||
| const errorResponse = await probeLayoutForErrors(options, layoutIndex); | ||
| if (errorResponse) return errorResponse; | ||
| const observationDynamic = cls.isLayoutObservationDynamic?.(layoutId) === true; |
There was a problem hiding this comment.
Important correctness detail: for build-time classified layouts, the observation check happens after the special error probe (line 421: const errorResponse = await probeLayoutForErrors(...)). This means:
- Layout is probed for redirects/not-found errors
- If no special error, observation dynamic check runs
- Layout flag is set to
"d"if build-time said dynamic OR observation said dynamic
The flag assignment at line 411 (layoutFlags[layoutId] = buildTimeResult === "static" ? "s" : "d") happens before the error probe, and then gets overwritten at line 423. The intermediate value at line 411 is never read by anything between lines 411 and 423 because the error probe doesn't consult layout flags.
This two-step assignment is slightly confusing but correct. Consider combining into a single assignment after the observation check if you want to clean this up later.
| const value = Reflect.get(target, prop, receiver); | ||
| if (typeof value !== "function") return value; | ||
| return (...args: unknown[]) => { | ||
| observeAllParamKeys(observer, plain); |
There was a problem hiding this comment.
When await params triggers the then handler, observeAllParamKeys reports ALL param keys — not just the ones the layout destructures after awaiting. This is the correct conservative choice: const { slug } = await params desugars to calling .then() and then destructuring the resolved value, so the observer only sees the .then() call, not which properties are later read from the resolved object.
Worth noting: this means await params always reports the full param set as observed, which will trigger SKIP_LAYOUT_PARAMS_OBSERVED rejection in rejectUnsafeStaticLayoutObservation even if the layout only reads one param. Combined with the structural param scope gate (SKIP_LAYOUT_PARAMS_PRESENT), any layout under a dynamic segment that touches params at all — even via await — will correctly never be skip-eligible.
|
Review submitted successfully with 9 inline comments. Here's a summary: Overall: The PR is solid. The architecture correctly preserves the #726 trust boundary design end-to-end. All 9 inline comments are non-blocking refinements. Key correctness findings:
Inline comments covered: partial dependency snapshots in error-path observations, promise-chain depth counting in subtree probe, redundant |
|
Bonk: pls approve the PR if there's no more issues |
The static export test times out at the default 30s in CI due to the additional probeReactServerSubtree walk introduced by static layout transport skip verification. The probe walks the component tree to detect dynamic data dependencies, adding overhead that pushes the test past 30s on CI's slower hardware. Increase to 60s.
|
/bigbonk review |
There was a problem hiding this comment.
Review: feat(skip): enable proven static layout transport skips
I read the full diff (4155 additions across 43 files), the #726 architectural context, all prior review threads, and the source code of every module touched by this PR. The implementation is correct and well-designed. Approving with a few non-blocking observations below.
Architecture assessment
The trust boundary design is the strongest aspect of this PR. The layering is disciplined:
- Parse time —
parseClientReuseManifestHeaderalways returnsenabled: false. No parser-level trust of client input. - Render time —
createRenderLifecycleSkipDispositionis the sole upgrade path toSKIP_STATIC_LAYOUT_VERIFIED, gated behind a 10-check observation pipeline (rejectUnsafeStaticLayoutObservation). - Encoding time —
buildOutgoingAppPayloadre-validates layout-kind viaparseAppElementsWireElementKeybefore admitting any ID to the skip set. Defense in depth. - Client time —
isCacheRestorableAppPayloadMetadatagates off visited-cache storage whenskippedLayoutIds.length > 0, andmergeSkippedLayoutPreservationcan only add topreserveElementIds, never remove.
Cache bypass is complete
shouldCaptureRscForCacheMetadatais false when skip is active (line 717)capturedRscDataPromiseis null (passed as null at line 793)no-store, must-revalidateis set viaNO_STORE_CACHE_CONTROL(line 746)- Missing
cacheStateinrscResponsePolicycorrectly causespreserveClientResponseHeaders: trueat line 821, preventing finalization from overwriting the no-store header - ISR write is skipped (the
waitUntilpath is never reached) - ISR debug log is emitted (line 758)
Observation isolation is sound
runWithIsolatedProbeDependencies creates a child unified request context via runWithUnifiedStateMutation, replacing all 7 accumulator fields with fresh containers. The recordProbeDependencies call at the end of each probe reads from the child scope's containers (within the ALS continuation), so parent/sibling layout observations are not contaminated. The fix in 6de6236 correctly resolved the cross-layout accumulation bug that Codex and Copilot identified.
Render dependency lifecycle is correct
When a layout is skipped in buildOutgoingAppPayload, releaseAppElementRenderDependency is called (line 634 of app-elements-wire.ts), which resolves the promise that downstream renderAfterAppDependencies barriers await. Without this, skipped layouts would cause downstream page/layout renders to deadlock. The WeakMap-based registration ensures the release is scoped to the element record's lifetime.
Non-blocking observations
See inline comments.
| if (value == null || typeof value === "boolean" || typeof value === "number") return; | ||
| if (typeof value === "string" || typeof value === "bigint") return; | ||
| if (isPromiseLike(value)) { | ||
| await visit(await value, depth); |
There was a problem hiding this comment.
Promise-chain depth: await visit(await value, depth) revisits at the same depth rather than depth + 1. This means a chain of nested promises doesn't increment depth, so Promise.resolve(Promise.resolve(Promise.resolve(<Component />))) traverses N+1 nodes at the same depth level.
This is fine in practice — React server component output doesn't produce arbitrarily nested promise chains, and the maxNodes counter still bounds total work. But if you ever want the depth invariant to be airtight against synthetic inputs, this could use depth + 1 for the outer visit.
Non-blocking — noted for completeness, maxNodes catches real abuse.
| return cacheLife === null ? null : JSON.stringify(cacheLife); | ||
| }; | ||
|
|
||
| const runWithIsolatedProbeDependencies = (probe: () => unknown): unknown => { |
There was a problem hiding this comment.
The isInsideUnifiedScope() guard at line 123 means that when layout probes run outside a unified request context (e.g., in certain test harnesses or edge cases), the probe executes without isolation — dependencies accumulate on the request-wide scope. This is the correct fallback (no isolation = conservative accumulation = more rejections = safe), but worth noting that tests exercising probe isolation must run inside a unified scope to get meaningful coverage.
The existing test suite does this correctly via the dispatch helper setup.
| pathTags: [input.cleanPathname], | ||
| requestApis: buildRenderRequestApiObservations({ | ||
| completeness: "complete", | ||
| observed: input.layoutParamAccess?.getLayoutObservation(entry.id).requestApis ?? [], |
There was a problem hiding this comment.
This reads layoutParamAccess?.getLayoutObservation(entry.id).requestApis ?? [] for the synthetic observation, but by this point rejectUnsafeStaticLayoutObservation (line 342) has already verified requestApis.length === 0 — otherwise we'd have returned at line 343. So this will always be [].
The code is correct, but the dependency on rejection check ordering is implicit. If someone later reorders the rejection checks in rejectUnsafeStaticLayoutObservation and a new check passes despite request APIs being present, this synthetic observation would carry unexpected data.
Consider either hardcoding observed: [] with a comment explaining the invariant, or adding a debug assertion. Non-blocking — the ordering dependency is stable and well-tested.
| limits: ClientReuseManifestLimits; | ||
| visibleCommitVersion: number; | ||
| }): string | null { | ||
| const entries = input.entries.slice(0, input.limits.maxEntryCount); |
There was a problem hiding this comment.
The binary search for bounded serialization is clean. One edge case worth noting: if entries.length === 0 after the slice(0, maxEntryCount), the while (low <= high) loop never executes (low=1 > high=0) and best stays null. This is correct — the caller checks entries.length === 0 before calling this function. But if this function were ever called directly with an empty entries array, it would return null rather than an empty manifest. The current call site at line 165 is correctly guarded.
Non-blocking — just documenting the contract.
| const layoutModule = options.route.layouts[options.layoutIndex]; | ||
| const LayoutComponent = getDefaultExport(layoutModule); | ||
| if (!LayoutComponent) return null; | ||
| recordLayoutSkipObservationScope({ |
There was a problem hiding this comment.
Good: recordLayoutSkipObservationScope runs inside the probe closure, which means it executes within the runLayoutProbe isolation scope. This ensures the param scope and finite revalidate recording happen after the isolated context is established but before the layout component is invoked.
One subtlety: recordLayoutSkipObservationScope only reads static module exports (revalidate) and route segment structure — it doesn't invoke any async code or trigger data fetches. So it wouldn't actually be affected by the isolation scope. But placing it inside the probe closure is still the right choice for maintainability — it keeps all per-layout observation recording in one place.
| } | ||
| const errorResponse = await probeLayoutForErrors(options, layoutIndex); | ||
| if (errorResponse) return errorResponse; | ||
| const observationDynamic = cls.isLayoutObservationDynamic?.(layoutId) === true; |
There was a problem hiding this comment.
The two-step layout flag assignment (line 411: set from build-time, line 427: potentially override from observation) works correctly because nothing between lines 411 and 427 reads layoutFlags[layoutId]. The error probe at line 421 doesn't consult layout flags.
But if a future change to probeLayoutForErrors ever needed to read layout flags (e.g., to condition error handling on static/dynamic classification), the intermediate "s" value from line 411 could be incorrect for a layout whose observation says dynamic. Consider collapsing into a single assignment after the observation check:
| const observationDynamic = cls.isLayoutObservationDynamic?.(layoutId) === true; | |
| const errorResponse = await probeLayoutForErrors(options, layoutIndex); | |
| if (errorResponse) return errorResponse; | |
| const observationDynamic = cls.isLayoutObservationDynamic?.(layoutId) === true; | |
| layoutFlags[layoutId] = buildTimeResult === "dynamic" || observationDynamic ? "d" : "s"; |
Non-blocking — the current code is correct for the current call graph.
| releaseAppElementRenderDependency(input.element, key); | ||
| continue; | ||
| } | ||
| payload[key] = value === UNMATCHED_SLOT ? APP_UNMATCHED_SLOT_WIRE_VALUE : value; |
There was a problem hiding this comment.
The UNMATCHED_SLOT → APP_UNMATCHED_SLOT_WIRE_VALUE conversion is now handled inline during the iteration loop. Previously this conversion was done by the spread operator implicitly (since the element values were already converted before being stored). Verify that UNMATCHED_SLOT symbols can actually appear as values in input.element at this point — if they're already converted upstream, this check is dead code.
Looking at buildAppPageElements in app-page-route-wiring.tsx, UNMATCHED_SLOT symbols are indeed stored as element values for unmatched parallel route slots. So this conversion is necessary and correct.
Non-blocking observation — the inline conversion is cleaner than the previous implicit approach.
|
Review submitted successfully. Here's a summary: Approved the PR with 7 non-blocking inline observations. Key findings:The implementation is correct. The trust boundary design is disciplined across all four layers (parse → render → encode → client). The cache bypass is complete end-to-end. The observation isolation via Inline comments covered:
|
…05-static-layout-skip-transport # Conflicts: # packages/vinext/src/server/app-page-element-builder.ts # tests/app-page-dispatch.test.ts
Overview
Implements
#726-SKIP-04/05from #726.Bonk: please read issue #726 for the architectural big picture before reviewing this slice.
client-reuse-manifest,skip-cache-proof,app-page-render,app-page-dispatch,app-layout-param-observation,app-page-probe,app-page-route-wiring,app-page-params,app-browser-client-reuse-manifest, generatedapp-rsc-entrywiring.Why
Skip transport is only correct when the retained client layout is indistinguishable from the target server layout for this request. That means the client manifest cannot authorize omission by itself, partial RSC responses cannot become shared cache artifacts, and a layout whose output can depend on params, data invalidation, or finite revalidation cannot be reused without stronger proof. This PR keeps those responsibilities at the server render boundary and treats uncertain layouts conservatively.
params.unstable_cache()usage in an isolated child request scope before skip verification. Any observed dependency blocks skip for that layout without leaking into sibling or parent probe observations.revalidatesegment config, and skip rejects those layouts withSKIP_LAYOUT_REVALIDATE_PRESENT.childrenis present. Probes must not mutate single-use children.paramsobservation records sync reads,await params, destructuring/enumeration, andinchecks, while skip authorization now requires both complete observation and empty structural param scope.Cache-Control: no-store, must-revalidate, and avoids visited-response replay.What changed
navigateRsc()attachesX-Vinext-Client-Reuse-Manifestfor retained public static layouts, capped to the skip planner entry budget before byte-size trimming.paramsto children and be incorrectly retained across sibling values. Empty optional catch-all targets could look param-free because the runtime params object was{}.unstable_cache()usage, and publishes those unsafe observations as dynamic layout metadata.revalidatesegment config."s"layout flags even after per-layout observation proved skip would be rejected.unstable_cache()usage, and incomplete probes publish"d".childrencould hide chrome dependencies because probes rendered withchildren={null}.memo(),forwardRef(), andReact.lazyreturned below a layout could hide data dependencies from the probe, while arbitrary iterable children could be single-use.Maintainer review path
packages/vinext/src/server/client-reuse-manifest.tspackages/vinext/src/server/skip-cache-proof.tspackages/vinext/src/server/app-page-render.tspackages/vinext/src/server/app-page-dispatch.tspackages/vinext/src/server/app-layout-param-observation.tspackages/vinext/src/server/app-page-probe.tspackages/vinext/src/server/app-page-route-wiring.tsxpackages/vinext/src/server/app-page-params.tspackages/vinext/src/entries/app-rsc-entry.tspackages/vinext/src/shims/fetch-cache.tspackages/vinext/src/shims/thenable-params.tsawait params, destructuring, enumeration, and promise continuations.packages/vinext/src/server/app-browser-client-reuse-manifest.tsand browser entry testsValidation
vp checkvp test run tests/app-page-probe.test.ts tests/app-layout-param-observation.test.ts tests/app-page-dispatch.test.tsvp test run tests/app-page-probe.test.ts tests/app-page-dispatch.test.ts tests/app-page-render.test.ts tests/app-layout-param-observation.test.tsvp test run tests/app-page-dispatch.test.ts tests/app-layout-param-observation.test.ts tests/app-page-render.test.ts tests/skip-cache-proof.test.tsvp test run tests/app-layout-param-observation.test.ts tests/app-page-dispatch.test.ts -t "unstable_cache"unstable_cache()dependencies on cache miss and hit, and that verified static-layout skip falls back to render/send for retained layouts backed byunstable_cache()data. Passed: 2 tests.vp test run tests/app-layout-param-observation.test.ts tests/app-page-dispatch.test.ts tests/app-page-render.test.ts tests/shims.test.ts tests/unified-request-context.test.ts tests/skip-cache-proof.test.tsunstable_cache()observation channel. Passed: 6 files, 1094 tests.vp test run tests/app-browser-client-reuse-manifest.test.ts tests/client-reuse-manifest.test.ts tests/skip-cache-proof.test.tsvp test run tests/app-page-render.test.ts tests/app-router.test.tsvp test run tests/app-page-route-wiring.test.ts tests/app-page-execution.test.ts tests/app-page-probe.test.ts tests/app-page-dispatch.test.ts tests/app-page-render.test.ts tests/app-layout-param-observation.test.tsvp test run tests/app-router.test.ts tests/entry-templates.test.ts tests/app-page-route-wiring.test.tsvp test run tests/app-page-dispatch.test.ts tests/app-page-route-wiring.test.ts tests/app-page-probe.test.ts tests/app-page-render.test.ts tests/app-router.test.tsvp test run tests/app-page-probe.test.ts tests/app-page-dispatch.test.ts tests/app-page-render.test.ts tests/app-browser-client-reuse-manifest.test.ts tests/client-reuse-manifest.test.ts tests/skip-cache-proof.test.tsvp test run tests/app-router.test.ts tests/entry-templates.test.ts tests/app-page-request.test.ts tests/app-elements.test.ts tests/app-page-route-wiring.test.ts tests/thenable-params.test.ts tests/app-page-params.test.tsvp run vinext#buildprivate-next-instrumentation-client,virtual:vinext-rsc-entry, andvirtual:vite-rsc/client-references.PLAYWRIGHT_PROJECT=app-router pnpm exec playwright test tests/e2e/app-router/layout-persistence.spec.ts --project=app-router -g "dynamic segment layout counter survives param navigation while content updates"vp test run tests/app-page-execution.test.ts -t "build-time classified layout throws"git diff --checkknip. Passed.Previously on this PR, the navigation manifest and visited-cache e2e slice also passed:
Risk / compatibility
unstable_cache()usage observed during isolated layout probing block skip rather than relying on synthetic clean proof. Wrapped memo()/forwardRef()/React.lazy children are probed too.unstable_cache()trace metadata uses hashed keys and tag metadata, not raw keys or values.revalidateblocks skip because the retained layout has a revalidation policy, not a permanent artifact proof.Non-goals
References
#726-SKIP-04/05tests/app-page-dispatch.test.tsunstable_cache()rejection, optional catch-all empty-target rejection, data-dependent layout rejection including wrapped children and React.lazy children, finite-revalidate rejection, and hostile param-dependent layouts.tests/app-browser-client-reuse-manifest.test.tstests/app-page-route-wiring.test.tstests/app-layout-param-observation.test.tsunstable_cache()dependency observation isolation across child, parent, and sibling layouts.tests/app-page-probe.test.tstests/app-page-params.test.tstests/thenable-params.test.tsCloses #726