feat: X-Vinext-Router-Skip header optimization for static layouts#768
feat: X-Vinext-Router-Skip header optimization for static layouts#768NathanDrake2406 wants to merge 4 commits intocloudflare:mainfrom
Conversation
![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: d1fe67be4e
ℹ️ 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".
| if (options.isRscRequest && options.requestedSkipLayoutIds) { | ||
| const elementsRecord = options.element as Record<string, unknown>; | ||
| for (const id of options.requestedSkipLayoutIds) { | ||
| if (id in elementsRecord && layoutFlags[id] === "s") { | ||
| delete elementsRecord[id]; | ||
| } |
There was a problem hiding this comment.
When isRscRequest and requestedSkipLayoutIds are set, this branch deletes static layout:* entries directly from options.element before rendering; that same rendered stream is then used by scheduleAppPageRscCacheWrite() and stored under the path-only ISR key. In production ISR routes (revalidate > 0), a request from a client that already has layouts cached can therefore write a partial RSC payload to shared cache, and later clients that do not have those layouts will read a response missing required layout entries (rendering incomplete UI). The cache write path needs to use an unfiltered payload (or skip caching when skip headers are applied).
Useful? React with 👍 / 👎.
NathanDrake2406
force-pushed
the
feat/layout-persistence-pr-7-skip-header
branch
from
d1fe67b to
cd655cd
Compare
commit: |
NathanDrake2406
force-pushed
the
feat/layout-persistence-pr-7-skip-header
branch
2 times, most recently
from
745781e to
0655ee7
Compare
NathanDrake2406
force-pushed
the
feat/layout-persistence-pr-7-skip-header
branch
from
0655ee7 to
ecc25d6
Compare
|
haha jk I'll split this one out, but could you please ask bonk to review this so I can just fix it before attemping to split? |
There was a problem hiding this comment.
Pull request overview
Adds client/server support for the X-Vinext-Router-Skip optimization by propagating per-layout static/dynamic flags (__layoutFlags), sending cached static layout IDs from the client on RSC navigations, and filtering the outgoing RSC wire stream (and cached RSC bytes) to omit layouts the server agrees are static. Also introduces build-time classification manifest wiring + debug reason sidecar to reduce runtime probing overhead.
Changes:
- Introduce skip-header utilities (
parseSkipHeader,buildSkipHeaderValue, header constant) and server-side skip decision logic (computeSkipDecision), plus__layoutFlagsinjection into the outgoing payload. - Implement RSC wire-format byte filtering (
createSkipFilterTransform/wrapRscBytesForResponse) and apply it to both live RSC responses and cached RSC reads. - Wire build-time layout classifications into the generated RSC entry via a manifest +
generateBundlepatching of__VINEXT_CLASS/__VINEXT_CLASS_REASONS, with optional runtime debug logging.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
packages/vinext/src/server/app-elements.ts |
Adds skip header helpers, layout flags attachment utilities, and authoritative skip decision computation. |
packages/vinext/src/server/app-browser-entry.ts |
Sends skip header on navigation RSC fetches; updates request header builder. |
packages/vinext/src/server/app-page-render.ts |
Injects __layoutFlags into outgoing payload and applies skip filtering on the response stream only. |
packages/vinext/src/server/app-page-skip-filter.ts |
New RSC wire-format transform for omitting skipped slots and dropping orphaned rows. |
packages/vinext/src/server/app-page-cache.ts |
Applies skip filtering when serving cached canonical RSC bytes. |
packages/vinext/src/server/app-page-execution.ts |
Threads classification reasons/debug emission through the layout probe loop. |
packages/vinext/src/index.ts |
Collects build-time manifest and patches the generated RSC chunk with dispatch tables in generateBundle. |
packages/vinext/src/entries/app-rsc-entry.ts |
Generates routeIdx + build-time dispatch stubs and threads skip/mounted-slots values from handler scope. |
packages/vinext/src/build/* |
Adds manifest + shared classification types; enhances module-graph result shape and report classification output. |
packages/vinext/src/shims/error-boundary.tsx |
Uses local navigation shim import for reliability. |
tests/* + snapshots |
Adds/updates unit + integration coverage for manifest wiring, reasons, skip filter behavior, cache behavior, and __layoutFlags. |
Comments suppressed due to low confidence (1)
packages/vinext/src/server/app-browser-entry.ts:993
- At the navigation fetch call site,
mountedSlotsHeaderis computed earlier in the function but is no longer applied torequestHeaders. This makes the request inconsistent with the mounted-slots-based caching and can increase payload size because the server can't omit already-mounted slots.
Suggestion: apply mountedSlotsHeader to the outgoing request headers (e.g. X-Vinext-Mounted-Slots) when present.
const requestHeaders = createRscRequestHeaders(
requestInterceptionContext,
getBrowserRouterState().layoutFlags,
);
navResponse = await fetch(rscUrl, {
headers: requestHeaders,
credentials: "include",
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function createRscRequestHeaders( | ||
| interceptionContext: string | null, | ||
| layoutFlags: LayoutFlags, | ||
| ): Headers { | ||
| const headers = new Headers({ Accept: "text/x-component" }); | ||
| if (interceptionContext !== null) { | ||
| headers.set("X-Vinext-Interception-Context", interceptionContext); | ||
| } | ||
| const skipValue = buildSkipHeaderValue(layoutFlags); | ||
| if (skipValue !== null) { | ||
| headers.set(X_VINEXT_ROUTER_SKIP_HEADER, skipValue); | ||
| } | ||
| return headers; | ||
| } |
|
/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: X-Vinext-Router-Skip header optimization
Overall this is well-architected. The key design decisions are sound:
- Canonical-first cache strategy: The tee in
renderAppPageLifecyclehappens BEFORE the skip filter, soisrRscDataPromisealways captures full canonical bytes. The Codex P1 about ISR cache poisoning is a false positive — the architecture is correct. The skip filter is only applied torscCapture.responseStream(the client-facing branch), while the capture branch gets unfiltered canonical bytes. - Defense-in-depth:
computeSkipDecisionre-validates client-requested skips against server-side classification. The skip header is a hint, not a command. - Byte-layer filtering: Keeping the skip filter in the RSC wire format (
app-page-skip-filter.ts) rather than mutating the React element tree is the right call — it preserves the canonical payload for caching and avoids coupling to React internals. app-page-skip-filter.tsstream state machine: Clean architecture — buffer until row 0 arrives, compute live set, flush pending rows in stream order, then forward-pass subsequent rows. The fallback to passthrough on parse failure and the flush of pending rows if row 0 never arrives are good defensive choices.
Bug
- Mounted-slots header dropped from navigation fetches (confirmed Copilot finding): The refactor of
createRscRequestHeadersto acceptlayoutFlagsremoved themountedSlotsHeaderfrom the outgoing fetch. The header is still computed (line 919) and used for prefetch/visited-response cache lookups, but the actualfetch()no longer sends it to the server. This breaks server-side slot-aware payload tailoring.
Suggestions (non-blocking)
-
REFERENCE_PATTERNcould use a source link: The regex[LBFQWK@]is correct for React 19.2.x but will silently miss new tags React adds. A comment linking to the React Flight source would help future maintainers audit coverage. -
buildPageElementssignature is now 9 positional parameters: Consider bundling the trailing params into an options object in a follow-up. It's easy to swapmountedSlotsHeaderandskipLayoutIdsat a call site without the type system catching it (generated code is untyped). -
__VINEXT_CLASS_REASONSis always patched: The reasons dispatch table is always written into the bundle viagenerateBundlebut only consulted whenVINEXT_DEBUG_CLASSIFICATIONis set. For apps with many routes this is dead code in production. Consider documenting the tradeoff or gating on a build-time flag.
| const requestHeaders = createRscRequestHeaders( | ||
| requestInterceptionContext, | ||
| getBrowserRouterState().layoutFlags, | ||
| ); |
There was a problem hiding this comment.
Bug: mountedSlotsHeader is no longer sent on the navigation fetch. The refactor added layoutFlags to createRscRequestHeaders but dropped the mounted-slots header that was previously set here. mountedSlotsHeader is still computed and used for prefetch/visited-response cache matching, but the server never receives it. This breaks server-side slot-aware payload tailoring and visited-response cache hits on subsequent navigations.
Should be something like:
| ); | |
| const requestHeaders = createRscRequestHeaders( | |
| requestInterceptionContext, | |
| getBrowserRouterState().layoutFlags, | |
| ); | |
| if (mountedSlotsHeader) { | |
| requestHeaders.set("X-Vinext-Mounted-Slots", mountedSlotsHeader); | |
| } |
| * streams subsequent rows through a forward-pass live set. | ||
| */ | ||
|
|
||
| const REFERENCE_PATTERN = /^\$(?:[LBFQWK@])?([0-9a-fA-F]+)$/; |
There was a problem hiding this comment.
Consider adding a comment linking to the React Flight source where these tag prefixes are defined (e.g., ReactFlightClientConfig.js or equivalent). The character class [LBFQWK@] is correct for React 19.2.x but will silently miss any new tags React adds. Since a missed tag that IS a row reference would cause the filter to drop live rows (a silent correctness bug), a source-link comment would make this easier to audit on React upgrades.
| return; | ||
| } | ||
| const { rewritten, liveIds } = filterRow0(parsed, skipIds); | ||
| const newRow0Raw = `0:${JSON.stringify(rewritten)}`; |
There was a problem hiding this comment.
Nit: The re-serialization assumes row 0 is always plain JSON (0:${JSON.stringify(rewritten)}). For App Router payloads this is true (row 0 is the elements record), but if this filter is ever reused for other RSC use cases, tagged formats like 0:I{...} would be broken. A brief comment noting the App Router row-0 invariant would be helpful.
| @@ -935,7 +968,7 @@ function findIntercept(pathname) { | |||
| return null; | |||
There was a problem hiding this comment.
The buildPageElements signature is now 9 positional parameters (route, params, routePath, opts, searchParams, isRscRequest, request, mountedSlotsHeader, skipLayoutIds). Since this is generated code (untyped), it's easy to silently swap mountedSlotsHeader (string|null) and skipLayoutIds (Set) at a call site. Consider bundling the trailing params into an options object in a follow-up to get at least shape-level safety.
|
Review posted successfully. Here's a summary of my findings: Bug found (1):
False positive dismissed (1):
Non-blocking suggestions (3):
|
Summary
Implements the skip header optimization that lets the client tell the server "I already have these static layouts cached," and the server omits them from the RSC payload. This reduces bandwidth and parse time for navigation between routes that share static layouts.
parseSkipHeader(filters tolayout:*entries only),buildSkipHeaderValue(emits only static IDs),X_VINEXT_ROUTER_SKIP_HEADERconstantLayoutClassificationOptionsthroughrenderAppPageLifecycle→ probe →__layoutFlagsinjection into the RSC elements payloadcreateRscRequestHeadersincludesX-Vinext-Router-Skipwith static layout IDs from the current router state. Only sent on navigation RSC fetches (not SSR, HMR, server actions, or prefetches)app-rsc-entry.tsparses the skip header, wires classification withgetLayoutId+runWithIsolatedDynamicScope, passes skip set to render lifecycleSafety guarantees
__route,__rootLayout,__interceptionContext,__layoutFlagsare never skippable"d"__layoutFlags= empty flagsTest plan
parseSkipHeader: null, empty, single, comma-separated, whitespace, non-layout filteredbuildSkipHeaderValue: empty → null, all dynamic → null, mix → only static, all static__layoutFlagsinjection: static, dynamic, no classification (backward compat), multiple layoutsvp checkclean: format, lint, types all pass