Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions packages/vinext/src/entries/app-rsc-entry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,8 @@ import {
import {
APP_INTERCEPTION_CONTEXT_KEY as __APP_INTERCEPTION_CONTEXT_KEY,
createAppPayloadRouteId as __createAppPayloadRouteId,
parseSkipHeader as __parseSkipHeader,
X_VINEXT_ROUTER_SKIP_HEADER as __X_VINEXT_ROUTER_SKIP_HEADER,
} from ${JSON.stringify(appElementsPath)};
import {
buildAppPageElements as __buildAppPageElements,
Expand Down Expand Up @@ -1420,6 +1422,10 @@ async function _handleRequest(request, __reqCtx, _mwCtx) {
}

const isRscRequest = pathname.endsWith(".rsc") || request.headers.get("accept")?.includes("text/x-component");
const __EMPTY_SKIP_LAYOUT_IDS = new Set();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous review flagged that this allocates a fresh new Set() on every non-RSC request. The author responded that this was fixed to use a shared empty sentinel, but the code still shows const __EMPTY_SKIP_LAYOUT_IDS = new Set() inside the request handler body — meaning it's allocated per-request, not shared.

To actually share it, this needs to be hoisted outside _handleRequest to module scope in the generated entry (similar to how EMPTY_SKIP_SET is a module-level constant in app-page-render.ts and app-page-cache.ts). The current placement inside the function body means a new Set instance is created on every request, including non-RSC ones where __skipLayoutIds immediately points to it.

Not a correctness issue (empty Set is cheap), but the intent from the review response doesn't match what landed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. The empty skip set is now hoisted to module scope in the generated RSC entry template, so non-RSC requests really reuse a shared sentinel instead of allocating inside _handleRequest. Updated the entry snapshots with the hoist.

const __skipLayoutIds = isRscRequest
? __parseSkipHeader(request.headers.get(__X_VINEXT_ROUTER_SKIP_HEADER))
: __EMPTY_SKIP_LAYOUT_IDS;
// Read mounted-slots header once at the handler scope and thread it through
// every buildPageElements call site. Previously both the handler and
// buildPageElements read and normalized it independently, which invited
Expand Down Expand Up @@ -2206,6 +2212,7 @@ async function _handleRequest(request, __reqCtx, _mwCtx) {
isrSet: __isrSet,
mountedSlotsHeader: __mountedSlotsHeader,
revalidateSeconds,
skipIds: __skipLayoutIds,
renderFreshPageForCache: async function() {
// Re-render the page to produce fresh HTML + RSC data for the cache
// Use an empty headers context for background regeneration — not the original
Expand Down Expand Up @@ -2420,6 +2427,7 @@ async function _handleRequest(request, __reqCtx, _mwCtx) {
isForceStatic,
isProduction: process.env.NODE_ENV === "production",
isRscRequest,
supportsFilteredRscStream: false,
isrDebug: __isrDebug,
isrHtmlKey: __isrHtmlKey,
isrRscKey: __isrRscKey,
Expand Down Expand Up @@ -2468,6 +2476,7 @@ async function _handleRequest(request, __reqCtx, _mwCtx) {
},
revalidateSeconds,
mountedSlotsHeader: __mountedSlotsHeader,
requestedSkipLayoutIds: __skipLayoutIds,
renderErrorBoundaryResponse(renderErr) {
return renderErrorBoundaryPage(route, renderErr, isRscRequest, request, params, _scriptNonce);
},
Expand Down
46 changes: 46 additions & 0 deletions packages/vinext/src/server/app-elements.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,52 @@ export function resolveVisitedResponseInterceptionContext(
return payloadInterceptionContext ?? requestInterceptionContext;
}

export const X_VINEXT_ROUTER_SKIP_HEADER = "X-Vinext-Router-Skip";
export const X_VINEXT_MOUNTED_SLOTS_HEADER = "X-Vinext-Mounted-Slots";
const MAX_SKIP_LAYOUT_IDS = 50;
const EMPTY_SKIP_HEADER_IDS: ReadonlySet<string> = new Set<string>();

export function parseSkipHeader(header: string | null): ReadonlySet<string> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The header parsing accepts any layout:* prefixed value without length or character validation. A malicious client could send a very long header with many entries to force the server to allocate a large Set. Since this runs on every RSC request (when the header is present), consider adding a cap — e.g., bail out or truncate after N entries (say 50, since deeply nested layout trees beyond that would be unusual).

This is low severity since Cloudflare Workers already enforce request header size limits, but defense-in-depth at the application layer doesn't hurt.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. parseSkipHeader now uses a bounded parser and stops after 50 unique layout:* ids, which keeps the allocation surface capped even if a client sends a pathological skip header. Added a test for the cap.

if (!header) return EMPTY_SKIP_HEADER_IDS;
const ids = new Set<string>();
for (const part of header.split(",")) {
const trimmed = part.trim();
if (trimmed.startsWith("layout:")) {
ids.add(trimmed);
if (ids.size >= MAX_SKIP_LAYOUT_IDS) {
break;
}
}
}
return ids.size > 0 ? ids : EMPTY_SKIP_HEADER_IDS;
}

const EMPTY_SKIP_DECISION: ReadonlySet<string> = new Set<string>();

/**
* Pure: computes the authoritative set of layout ids that should be omitted
* from the outgoing payload. Defense-in-depth — an id is only included if the
* server independently classified it as `"s"` (static). Empty or missing
* `requested` yields a shared empty set so the hot path does not allocate.
*
* See `LayoutFlags` type docblock in this file for lifecycle.
*/
export function computeSkipDecision(
layoutFlags: LayoutFlags,
requested: ReadonlySet<string> | undefined,
): ReadonlySet<string> {
if (!requested || requested.size === 0) {
return EMPTY_SKIP_DECISION;
}
const decision = new Set<string>();
for (const id of requested) {
if (layoutFlags[id] === "s") {
decision.add(id);
}
}
return decision;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

computeSkipDecision allocates a new Set<string>() even when no requested ids match any "s" flags (i.e., the client asked to skip layouts but the server classified all of them as dynamic). The function returns this empty Set instead of EMPTY_SKIP_DECISION.

This means skipIds.size > 0 downstream is still false so it's not a correctness bug, but it defeats the purpose of the EMPTY_SKIP_DECISION sentinel since the caller (renderAppPageLifecycle) checks skipIds.size > 0 to decide whether to create a transform stream. Consider:

Suggested change
return decision;
return decision.size > 0 ? decision : EMPTY_SKIP_DECISION;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. computeSkipDecision now returns the shared empty sentinel when every requested id is rejected, and there’s a focused test that checks the all-dynamic case reuses the same empty reference instead of leaking a fresh Set.

}

export function normalizeAppElements(elements: AppWireElements): AppElements {
let needsNormalization = false;
for (const [key, value] of Object.entries(elements)) {
Expand Down
11 changes: 10 additions & 1 deletion packages/vinext/src/server/app-page-cache.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
import type { CachedAppPageValue } from "../shims/cache.js";
import { buildAppPageCacheValue, type ISRCacheEntry } from "./isr-cache.js";
import { wrapRscBytesForResponse } from "./app-page-skip-filter.js";

const EMPTY_SKIP_SET: ReadonlySet<string> = new Set<string>();

type AppPageDebugLogger = (event: string, detail: string) => void;
type AppPageCacheGetter = (key: string) => Promise<ISRCacheEntry | null>;
Expand All @@ -22,6 +25,7 @@ export type BuildAppPageCachedResponseOptions = {
isRscRequest: boolean;
mountedSlotsHeader?: string | null;
revalidateSeconds: number;
skipIds?: ReadonlySet<string>;
};

export type ReadAppPageCacheResponseOptions = {
Expand All @@ -37,6 +41,7 @@ export type ReadAppPageCacheResponseOptions = {
revalidateSeconds: number;
renderFreshPageForCache: () => Promise<AppPageCacheRenderResult>;
scheduleBackgroundRegeneration: AppPageBackgroundRegenerator;
skipIds?: ReadonlySet<string>;
};

export type FinalizeAppPageHtmlCacheResponseOptions = {
Expand Down Expand Up @@ -106,7 +111,9 @@ export function buildAppPageCachedResponse(
rscHeaders["X-Vinext-Mounted-Slots"] = options.mountedSlotsHeader;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good use of EMPTY_SKIP_SET as the fallback — wrapRscBytesForResponse short-circuits on empty set and returns the raw ArrayBuffer without creating a ReadableStream. The cache-hit hot path stays allocation-free when no skip header is present.

return new Response(cachedValue.rscData, {
const body = wrapRscBytesForResponse(cachedValue.rscData, options.skipIds ?? EMPTY_SKIP_SET);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cache-read path applies skipIds to the cached canonical bytes here. This is correct for the response sent to the current client, but I want to confirm: the skipIds used here come from the current request's X-Vinext-Router-Skip header (threaded from the generated entry → readAppPageCacheResponsebuildAppPageCachedResponse), not from the request that originally populated the cache. This is the right behavior — each client gets a response filtered to its own skip set — just want to make sure this is documented as intentional since it could be confusing during debugging (same cache entry, different response bytes for different clients).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documented. Added an inline comment on the cache-read path clarifying that the cached bytes stay canonical and the current request’s skipIds only shape the egress response for this client. That matches the intended canonical-cache / per-client-response split.


return new Response(body, {
status,
headers: rscHeaders,
});
Expand Down Expand Up @@ -142,6 +149,7 @@ export async function readAppPageCacheResponse(
isRscRequest: options.isRscRequest,
mountedSlotsHeader: options.mountedSlotsHeader,
revalidateSeconds: options.revalidateSeconds,
skipIds: options.skipIds,
});

if (hitResponse) {
Expand Down Expand Up @@ -198,6 +206,7 @@ export async function readAppPageCacheResponse(
isRscRequest: options.isRscRequest,
mountedSlotsHeader: options.mountedSlotsHeader,
revalidateSeconds: options.revalidateSeconds,
skipIds: options.skipIds,
});

if (staleResponse) {
Expand Down
30 changes: 25 additions & 5 deletions packages/vinext/src/server/app-page-render.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
import type { ReactNode } from "react";
import type { CachedAppPageValue } from "../shims/cache.js";
import { buildOutgoingAppPayload, type AppOutgoingElements } from "./app-elements.js";
import {
buildOutgoingAppPayload,
computeSkipDecision,
type AppOutgoingElements,
} from "./app-elements.js";
import {
finalizeAppPageHtmlCacheResponse,
scheduleAppPageRscCacheWrite,
} from "./app-page-cache.js";
import { createSkipFilterTransform } from "./app-page-skip-filter.js";
import {
buildAppPageFontLinkHeader,
resolveAppPageSpecialError,
Expand Down Expand Up @@ -32,6 +37,8 @@ import {
type AppPageSsrHandler,
} from "./app-page-stream.js";

const EMPTY_SKIP_SET: ReadonlySet<string> = new Set<string>();

type AppPageBoundaryOnError = (
error: unknown,
requestInfo: unknown,
Expand Down Expand Up @@ -68,6 +75,7 @@ export type RenderAppPageLifecycleOptions = {
isForceStatic: boolean;
isProduction: boolean;
isRscRequest: boolean;
supportsFilteredRscStream?: boolean;
isrDebug?: AppPageDebugLogger;
isrHtmlKey: (pathname: string) => string;
isrRscKey: (pathname: string, mountedSlotsHeader?: string | null) => string;
Expand Down Expand Up @@ -97,6 +105,7 @@ export type RenderAppPageLifecycleOptions = {
waitUntil?: (promise: Promise<void>) => void;
element: ReactNode | Readonly<Record<string, ReactNode>>;
classification?: LayoutClassificationOptions | null;
requestedSkipLayoutIds?: ReadonlySet<string>;
};

function buildResponseTiming(
Expand Down Expand Up @@ -148,9 +157,9 @@ export async function renderAppPageLifecycle(

const layoutFlags = preRenderResult.layoutFlags;

// Render the CANONICAL element. The outgoing payload carries per-layout
// static/dynamic flags under `__layoutFlags` so the client can later tell
// which layouts are safe to skip on subsequent navigations.
// Always render the CANONICAL element. Skip semantics are applied on the
// egress branch only so the cache branch receives full bytes regardless of
// the client's skip header. See `app-page-skip-filter.ts`.
const outgoingElement = buildOutgoingAppPayload({
element: options.element,
layoutFlags,
Expand All @@ -172,9 +181,17 @@ export async function renderAppPageLifecycle(
revalidateSeconds !== Infinity &&
!options.isForceDynamic,
);
const rscForResponse = rscCapture.responseStream;
const isrRscDataPromise = rscCapture.capturedRscDataPromise;

const skipIds =
options.isRscRequest && (options.supportsFilteredRscStream ?? true)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ?? true default for supportsFilteredRscStream means the filter is opt-out rather than opt-in. Since the generated entry currently always passes false, this is safe today. But if a future caller forgets to set the flag, the filter will activate by default.

Given that this is explicitly "dormant until canonical-stream is validated" per the PR description, an opt-in default (?? false) would be more conservative and match the stated intent. When PR 5/6 or 6/6 is ready to activate the filter, it would explicitly set true.

Suggested change
options.isRscRequest && (options.supportsFilteredRscStream ?? true)
options.isRscRequest && (options.supportsFilteredRscStream ?? false)

This way, omitting the flag keeps the filter off rather than on.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied. The dormant filter is now opt-in: supportsFilteredRscStream defaults to false in renderAppPageLifecycle, and the render tests now prove that omitting the flag keeps the response canonical until a later PR explicitly enables it.

? computeSkipDecision(layoutFlags, options.requestedSkipLayoutIds)
: EMPTY_SKIP_SET;
const rscForResponse =
skipIds.size > 0
? rscCapture.responseStream.pipeThrough(createSkipFilterTransform(skipIds))
: rscCapture.responseStream;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth noting for future readers: rscForResponse is also used for the HTML SSR path (line 261 in this file), but skipIds will always be empty when !isRscRequest, so the HTML path always receives the unfiltered stream. The code flow is correct; a brief inline comment at the HTML usage site could save someone from a double-take.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added an inline comment at the HTML SSR call site clarifying that rscForResponse is still canonical there because skipIds only becomes non-empty for RSC requests.

Comment on lines +190 to +193

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So from what I gather, it sounds like we render the entire tree, then skip returning parts of that tree to client? Wouldn't we want to skip rendering those before-hand to reduce server work? I believe both Next.js and Waku would skip before the rendering.


if (options.isRscRequest) {
const dynamicUsedDuringBuild = options.consumeDynamicUsage();
const rscResponsePolicy = resolveAppPageRscResponsePolicy({
Expand Down Expand Up @@ -241,6 +258,9 @@ export async function renderAppPageLifecycle(
return renderAppPageHtmlStream({
fontData,
navigationContext: options.getNavigationContext(),
// HTML SSR always consumes the canonical stream: skipIds is only
// non-empty for RSC requests, so the HTML path never sees filtered
// bytes even though it reuses the same `rscForResponse` handle.
rscStream: rscForResponse,
scriptNonce: options.scriptNonce,
ssrHandler,
Expand Down
Loading
Loading