feat(skip): plumb client reuse manifests through the app request path#1717
Conversation
The browser never sends a client reuse manifest and the server has no path to receive one, so the static-layout skip planner (cloudflare#1674) and observation foundations (cloudflare#1672) have nothing to act on. Skip transport stays fully off, but the transport wiring those slices depend on is missing. Add a bounded browser producer that derives a client reuse manifest from the visible AppElements state and attaches it as X-Vinext-Client-Reuse-Manifest only on soft navigations. Parse the header into a render/send-by-default disposition and thread the parsed result through the RSC handler, the generated app entry, page dispatch, and the render lifecycle. Wire the per-layout observation tracker and segment-config classification into dispatch so the enable-transport slice can read them. No RSC payload is omitted and no response cache-control changes: the parsed manifest is carried but never authorizes a skip yet. Unproven and absent- manifest requests render and send exactly as before. Slice 3 of 4 from cloudflare#1437; enable-transport follows. Refs cloudflare#726
commit: |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the transport plumbing for the client reuse manifest. The threading from browser -> RSC handler -> dispatch -> render is consistent and correctly wired, the byte-bounded producer is sound (binary-search prefix fit, returns null rather than emitting oversized headers, capped to the skip verification budget), and the mounted-slots header refactor is behavior-preserving (getMountedSlotIdsHeader never returns "", so moving from if (mountedSlotsHeader) to the !== null && !== undefined guard in createRscRequestHeaders is equivalent). Per the PR description I did not block on missing tests.
A few things worth a closer look, mostly around the "request behavior is unchanged" claim:
-
Layout classification is now recomputed at request time, which changes
layoutFlagsoutput.getEffectiveLayoutClassificationsis called unconditionally and mergesclassifyLayoutSegmentConfigFromModuleresults on top of the build-time map, andprobeAppPageLayoutsnow also folds in an observation-based dynamic override.layoutFlagsis serialized to the wire under__layoutFlagsand stored in client router state (app-browser-state.ts/app-browser-visible-commit.ts), so the values a client receives can differ from before this PR. That is a real (likely intended, more-Next-accurate) behavioral change, not pure plumbing — worth calling out explicitly in the description and ideally covered by a parity assertion before the enable slice lands. -
New per-request render cost.
probeAppPageLayoutWithTrackingnow renders each layout's subtree (with a Fragment child) for observation on every App Router request, whereprobeLayoutAtpreviously only did error/dynamic probing. This runs unconditionally now thatlayoutParamAccessis always constructed. Bounded byprobeReactServerSubtree's depth/node caps, but it's added work on the hot path with no consumer yet — flagging for awareness. -
Debug classification reason can disagree with the emitted flag in the build-time path (diagnostics-only). See inline comment.
-
Write-only WeakMap.
appElementRenderDependenciesis registered but never read in this PR (acknowledged as a non-goal). Harmless (WeakMap GCs with the elements object), just noting it is dead until the enable slice.
Nothing here looks blocking for a plumbing-only slice; the classification-output change (#1) is the item most worth confirming is deliberate.
| const errorResponse = await probeLayoutForErrors(options, layoutIndex); | ||
| if (errorResponse) return errorResponse; | ||
| const observationDynamic = cls.isLayoutObservationDynamic?.(layoutId) === true; | ||
| layoutFlags[layoutId] = buildTimeResult === "dynamic" || observationDynamic ? "d" : "s"; |
There was a problem hiding this comment.
In the build-time-classified branch, debugClassification is reported earlier (line 420-423) with the build-time reason, but the final flag here can be flipped to "d" by observationDynamic even when buildTimeResult === "static". After that flip, the emitted layoutFlags[layoutId] disagrees with the debug reason that was already recorded (e.g. reason says segment-config: dynamic=force-static but the flag is d). The Layer-3 branch below avoids this by computing layoutDynamic before calling debugClassification. Diagnostics-only, but if any debug-variant invariant test asserts reason/flag agreement this will trip it. Consider deferring the debug call until after the observation override, or emitting a runtime-observation reason when the override flips the flag.
| const reasons = includeReasons ? new Map(route.__buildTimeReasons ?? []) : null; | ||
|
|
||
| for (let index = 0; index < route.layouts.length; index++) { | ||
| const classification = classifyLayoutSegmentConfigFromModule(route.layouts[index]); |
There was a problem hiding this comment.
classifyLayoutSegmentConfigFromModule duplicates the build-time classifyLayoutSegmentConfig (build/report.ts:472) but is applied at request time and merged on top of __buildTimeClassifications. Two notes: (a) this request-time pass adds revalidate === false handling that the build-time classifier lacks (it reads a numeric literal), so the two can diverge — intentional, but worth a shared helper or a comment cross-referencing the build-time version to keep them in sync; (b) because this now mutates the effective classification for every request, layouts whose segment config was not captured at build time (e.g. dev mode) will now be classified where they previously were not, which is the source of the layoutFlags output change noted in the summary.
| ReadonlyMap<string, AppRenderDependency> | ||
| >(); | ||
|
|
||
| export function registerAppElementRenderDependencies( |
There was a problem hiding this comment.
registerAppElementRenderDependencies writes into a module-level WeakMap that has no reader in this PR (releaseAppElementRenderDependency consumer lands with enable, per the description). It's harmless since the WeakMap is keyed by the elements object and GCs with it, but a one-line comment marking this as write-only-until-enable would help reviewers of the next slice avoid wondering whether the read path was dropped.
|
Review posted successfully. Summary of what I reviewed and found: Review of PR #1717 — client reuse manifest plumbingWhat's solid:
Findings posted (4, none blocking for a plumbing slice):
The most important item to confirm is #1 — the classification-output change is real and not pure plumbing. |
- app-page-execution: defer layout debug classification until after the observation override so the emitted reason agrees with the final flag; emit a runtime-probe reason when an observation flips a build-time static layout to dynamic. - app-page-dispatch: document classifyLayoutSegmentConfigFromModule as the request-time counterpart to the build-time classifier, noting the intentional revalidate===false divergence and the dev-mode classification. - app-render-dependency: mark registerAppElementRenderDependencies as write-only until the enable slice's consumer lands.
|
/bigbonk review |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review |
There was a problem hiding this comment.
Review — client reuse manifest plumbing (slice 3 of 4)
Reviewed the transport wiring end to end. The threading (browser producer -> RSC handler -> normalization -> dispatch -> render lifecycle) is consistent and correctly typed across every hop, including the generated app-rsc-entry. The byte-bounded producer is sound (binary-search prefix fit, returns null instead of an oversized header, capped to the skip-verification budget of 8). Parsing remains byte-bounded in parseClientReuseManifestHeader, so the wider parse path is not a new DoS surface. Per the PR description I did not block on missing tests.
Good news: the debug-reason/flag disagreement that an earlier review flagged in the build-time branch has been fixed in this revision — layoutFlags[layoutId] is now recomputed before debugClassification is called, and a runtime-probe reason is emitted when an observation flips a build-time static to dynamic. That concern is resolved.
The central issue is that "request behavior is unchanged" is not accurate for this slice. Two observable changes ride along with the plumbing:
-
The error-probe now renders the full layout subtree, not just the layout function with
children: null.probeLayoutAtwas rewritten to callprobeAppPageLayoutWithTracking->probeReactServerSubtree, which (a) passes<Fragment />as children instead ofnull, and (b) recursively invokes child server components. BecauseprobeLayoutForErrorsruns on the build-time-classified path too, a layout whose chrome only renders whenchildrenis truthy (e.g.if (!children) return null) — or a child component deep in the subtree — can now thrownotFound()/redirect()(or runfetch/side effects) during the probe where it previously did not. That can change the emitted HTTP response, on every App Router request, with no consumer reading the manifest yet. The full suite passing shows no current fixture trips it, but it is a latent behavioral/parity change, not pure plumbing. -
__layoutFlagsoutput can change.isLayoutObservationDynamicis wired into classification and flips a build-timestaticlayout todwheneverisAppLayoutObservationUnsafeForStaticReuseis true (finite revalidate, cache tags, request APIs, param-scope, etc.).__layoutFlagsis serialized to the wire and stored in client router state, and the new producer readslayoutFlags[layoutId] === "s", so the values a client receives — and which entries enter the manifest — differ from before this PR.
Neither is necessarily wrong (both look more Next-accurate), but they should be called out explicitly as intentional behavioral changes and ideally pinned by a parity assertion before the enable slice lands, rather than described as a no-op plumbing slice. Inline notes below, plus two smaller items (write-only WeakMap, merge-precedence in createEffectiveLayoutClassifications).
Nothing here is blocking for a stacked plumbing slice given the stated non-goals.
| params, | ||
| )), | ||
| children: null, | ||
| probeLayoutAt(li, layoutParamAccess) { |
There was a problem hiding this comment.
This rewrite changes the error-probe semantics, not just observation. The old probeLayoutAt called the layout once with children: null and did not recurse. probeAppPageLayoutWithTracking now renders via probeReactServerSubtree, which (1) passes <Fragment /> (truthy) as children and (2) recursively invokes child server components.
Since probeLayoutForErrors calls this on the build-time-classified path too (app-page-execution.ts:416), a layout that conditionally renders children-dependent chrome (if (!children) return null) — or any child component in the subtree — can now throw notFound()/redirect() or run side effects during the probe where it previously could not. That can change the HTTP response on every App Router request. The full suite passing means no current fixture trips it, but this is a real behavioral change that contradicts the "request behavior unchanged" claim. Worth an explicit callout in the description and a parity fixture (a layout that throws only when children is present).
There was a problem hiding this comment.
Confirmed and intentional. The probe now renders the full layout subtree (Fragment children + recursive child server components) so observation tracking sees data dependencies created below a layout's immediate body, matching Next's render order. We've stopped describing this slice as a no-op: the PR description now has a "Behavioral changes" section calling this out explicitly, and the change is pinned by tests/app-page-route-wiring.test.ts → "probes layout branches that render only when children are present" (a layout with if (!children) return null whose children-dependent chrome now runs during the probe and contributes its cache tags). No current fixture trips a new throw; CI is green.
| buildTimeClassifications: layoutClassifications.buildTimeClassifications, | ||
| buildTimeReasons: layoutClassifications.buildTimeReasons, | ||
| debugClassification: options.debugClassification, | ||
| isLayoutObservationDynamic(layoutId) { |
There was a problem hiding this comment.
isLayoutObservationDynamic is now live in classification, so a build-time static layout flips to d whenever isAppLayoutObservationUnsafeForStaticReuse returns true (finite revalidate, cache tags, request APIs, param scope, etc.). __layoutFlags is serialized to the wire (app-elements-wire.ts:611) and stored in client router state (app-browser-state.ts:488), and the new producer keys off layoutFlags[layoutId] === "s". So the flags a client receives can differ from before this PR. Likely intended (more Next-accurate), but it is an observable output change, not plumbing — please document it and add a parity assertion before the enable slice consumes it.
There was a problem hiding this comment.
Documented and pinned. The PR description "Behavioral changes" section now states explicitly that __layoutFlags output can differ: isLayoutObservationDynamic flips a build-time static layout to d when the observation is unsafe for static reuse, and request-time classifyLayoutSegmentConfigFromModule is merged on top of the build-time map. Added a parity assertion in tests/app-page-execution.test.ts → "flips a build-time static layout to dynamic when a runtime observation is unsafe for static reuse", which verifies a build-time static layout downgrades to d with a runtime-probe reason while a safe observation stays s.
| * normalizes `false` to `Infinity` upstream. The runtime override is merged on | ||
| * top of the build-time classification map in | ||
| * `createEffectiveLayoutClassifications`, so layouts not captured at build time | ||
| * (e.g. dev mode) are classified here where they previously were not. |
There was a problem hiding this comment.
The "intentional divergence" note is slightly misleading: the build-time classifier already normalizes revalidate = false to Infinity upstream (build/report.ts:238), so both versions treat false as static — they agree on that case, they don't diverge. The genuine difference is the one in the next sentence (request-time classifies layouts that were never captured at build time, e.g. dev mode). Consider trimming the false/Infinity framing so a future reader doesn't go looking for a divergence that isn't there.
There was a problem hiding this comment.
Fixed. The docblock on classifyLayoutSegmentConfigFromModule no longer frames revalidate === false/Infinity as a divergence — it now notes both the build-time and request-time classifiers treat false as static (the build-time version normalizes false → Infinity upstream), and that the genuine difference is scope: the request-time pass classifies layouts never captured at build time (e.g. dev mode).
|
|
||
| for (let index = 0; index < route.layouts.length; index++) { | ||
| const classification = classifyLayoutSegmentConfigFromModule(route.layouts[index]); | ||
| if (classification === null) continue; |
There was a problem hiding this comment.
Minor precedence note: this unconditionally overrides the build-time map for any layout whose module config classifies (classifications.set(index, ...)). If a Layer 1/2 build-time classifier marked a layout dynamic for a non-segment-config reason and the module's segment config says force-static, request-time will now win and flip it to static. That is probably the desired "module config is authoritative" behavior, but it is a deliberate precedence decision worth a one-line comment so the next slice doesn't treat the build-time map as the source of truth.
There was a problem hiding this comment.
Clarified. The precedence is intentional ("module config is authoritative") and is now documented inline in createEffectiveLayoutClassifications: when a layout's module segment config classifies, it overrides the build-time map for that layout even if a Layer 1/2 classifier marked it differently for a non-segment-config reason, and downstream consumers should treat the merged result — not __buildTimeClassifications — as the source of truth.
| // (`releaseAppElementRenderDependency`) that lands with enable-transport. It is | ||
| // keyed by the elements object and GCs with it, so it is harmless while unread. | ||
| export function registerAppElementRenderDependencies( | ||
| elements: Readonly<Record<string, unknown>>, |
There was a problem hiding this comment.
The write-only-until-enable comment is clear and the WeakMap GCs with the elements object, so this is harmless. Just confirming for the next reviewer: there is no reader of appElementRenderDependencies in this PR, and releaseAppElementRenderDependency is the planned consumer in the enable slice. No action needed.
There was a problem hiding this comment.
Acknowledged, no behavior change needed. registerAppElementRenderDependencies is marked write-only-until-enable with an inline comment; releaseAppElementRenderDependency is the planned reader in the enable slice, and the WeakMap GCs with the elements object so there's no leak in the interim.
|
Review posted successfully. Summary of what I reviewed and found: Review of PR #1717 — client reuse manifest plumbing (slice 3 of 4)What's solid:
Findings posted (5, none blocking for a stacked plumbing slice):
|
- app-page-dispatch: correct the classifyLayoutSegmentConfigFromModule docblock — the build-time classifier already normalizes revalidate=false to Infinity, so both agree on that case; the real difference is scope (request-time classifies dev-mode layouts not captured at build time). - app-page-dispatch: document the merge precedence in createEffectiveLayoutClassifications — module segment config is authoritative over the build-time map.
|
/bigbonk review |
…t to dynamic Adds a parity assertion for the request-time behavioral change the review flagged: isLayoutObservationDynamic downgrades a build-time `static` layout to `d` (and emits a runtime-probe reason) when the observed render is unsafe for static reuse, while a safe observation keeps it static. Complements the existing children-subtree probe assertion in app-page-route-wiring.test.ts.
|
/bigbonk review |
There was a problem hiding this comment.
Reviewed the manifest plumbing end to end. I agree with the prior /bigbonk rounds: the browser→handler→normalization→dispatch→render threading is consistent and correctly typed, the byte-bounded producer (serializeBoundedClientReuseManifest) is sound (binary-search prefix fit on a monotonic JSON array, returns null rather than an oversized header, capped to the skip-verification budget), the mounted-slots header refactor is behavior-preserving, and the two ride-along behavioral changes (full-subtree error probe + __layoutFlags flips) are now documented in the description and pinned by parity tests. No need to re-litigate those.
I have one new finding the earlier rounds did not surface: the rewrite of probeLayoutAt to run inside runLayoutProbe's isolated dependency scope can mask the Layer-3 dynamicDetected signal, and the compensating observationDynamic tracks a slightly narrower set of signals than consumeDynamicUsage(). Detail inline. Not blocking for a plumbing slice with the planner not yet consuming the flag, but worth confirming before the enable slice keys skip decisions off __layoutFlags.
| // dynamic API usage (headers(), cookies(), connection(), etc.) | ||
| try { | ||
| const { dynamicDetected } = await cls.runWithIsolatedDynamicScope(() => | ||
| options.probeLayoutAt(layoutIndex), |
There was a problem hiding this comment.
New (not flagged in prior rounds): this PR can mask Layer-3 dynamic detection.
Before this PR, options.probeLayoutAt(layoutIndex) ran the layout body directly inside runWithIsolatedDynamicScope, so any markDynamicUsage() during the probe was caught by the surrounding consumeDynamicUsage() and surfaced as dynamicDetected.
Now probeLayoutAt is wired (generated entry app-rsc-entry.ts:656) to probeAppPageLayoutWithTracking → runLayoutProbe → runWithIsolatedProbeDependencies, which opens a child unified scope and resets ctx.dynamicUsageDetected = false (app-layout-param-observation.ts:126). markDynamicUsage() calls inside the probe now write to the child scope and are discarded when it exits, so the parent's consumeDynamicUsage() at line 454 returns false.
The new observationDynamic (isLayoutObservationDynamic) is the intended compensator, but it only fires on the signals captured by recordProbeDependencies (request APIs, cache tags, finite revalidate, fetch observations, param scope). It does not cover markDynamicUsage() paths that don't also record a RenderRequestApiKind or other observed dependency — e.g. "use cache: private" during prerender (cache-runtime.ts:471) calls markDynamicUsage() with no paired request-API record. A layout hitting such a path would previously classify "d" via dynamicDetected and now classifies "s".
For a static-reuse skip feature a false-static is a correctness risk (it controls which entries enter the manifest), not just diagnostics. The full suite passing means no current fixture trips it, but please confirm the observationDynamic signal set is a strict superset of every markDynamicUsage() trigger before the enable slice authorizes skips off __layoutFlags — or fold the probe's child-scope dynamicUsageDetected back into the observation so the two signals can't diverge.
There was a problem hiding this comment.
Good catch — fixed rather than deferred, since it's a direct consequence of this PR's probe wiring. recordProbeDependencies now peeks the probe's child-scope dynamicUsageDetected (via a new non-resetting peekDynamicUsage()) before the isolated scope is discarded, and folds it into the per-layout observation as dynamicUsageObserved. isAppLayoutObservationUnsafeForStaticReuse treats that flag as unsafe, so the observation signal is now a strict superset of every markDynamicUsage() trigger the old Layer-3 dynamicDetected path caught (including markDynamicUsage() paths with no paired request-API/observed-dependency record, e.g. "use cache: private"). The two signals can no longer diverge into a false-static. Pinned by tests/app-layout-param-observation.test.ts → "folds a probe-scoped markDynamicUsage() into the observation as unsafe for static reuse".
| if (entries.length >= limits.maxEntryCount) break; | ||
| if (layoutId.length > limits.maxEntryIdLength) continue; | ||
| if (metadata.layoutFlags[layoutId] !== "s") continue; | ||
| if (!hasRetainedElement(state.elements, layoutId)) continue; |
There was a problem hiding this comment.
Minor: the per-entry maxEntryIdLength and maxEntryCount guards run against metadata.layoutIds, but the actual byte budget (maxManifestBytes) is only enforced afterward in serializeBoundedClientReuseManifest. That's fine functionally — just noting that an entry id passing maxEntryIdLength here can still be dropped later by the byte trim, so the two limits aren't independently observable from a caller's perspective. No change needed; flagging so the enable slice doesn't assume layoutFlags[id] === "s" && id in elements implies the entry made it into the emitted header.
There was a problem hiding this comment.
Acknowledged, no change needed — agreed the byte-budget trim in serializeBoundedClientReuseManifest can drop an entry that passed the per-entry maxEntryIdLength/maxEntryCount guards, so layoutFlags[id] === "s" && id in elements does not imply the entry made it into the emitted header. Noted in the PR description's risks so the enable slice doesn't assume independence between the two limits.
|
Review posted successfully. SummaryI reviewed PR #1717 (client reuse manifest plumbing, slice 3 of 4). The prior New finding (the value-add of this pass): The rewrite of
For a static-reuse skip feature a false- I also left a minor note on the producer about the per-entry vs. byte-budget limits not being independently observable. |
The probe rewrite runs layout probes inside runWithIsolatedProbeDependencies, which opens a child unified scope and resets dynamicUsageDetected. A markDynamicUsage() during the probe that leaves no other observable trace (e.g. "use cache: private") was therefore discarded on scope exit, so the parent's consumeDynamicUsage() in the Layer-3 path returned false and the layout could classify static where the pre-PR direct probe classified dynamic. recordProbeDependencies now peeks the child scope's dynamic-usage flag before it is reset and folds it into the per-layout observation (dynamicUsageObserved), which isAppLayoutObservationUnsafeForStaticReuse treats as unsafe. This keeps the observation signal a superset of the markDynamicUsage() triggers the Layer-3 dynamicDetected path covered, so the two signals can no longer diverge into a false-static. Adds peekDynamicUsage() (non-resetting read) and a regression test.
|
/bigbonk review |
There was a problem hiding this comment.
Review — client reuse manifest plumbing (slice 3 of 4)
Reviewed the full transport wiring end to end. The prior /bigbonk rounds were thorough and the author addressed everything I'd otherwise have raised — so this pass confirms the resolutions and adds one new performance note.
Confirmed resolved from earlier rounds:
- Threading (browser producer →
normalizeRscRequest→app-rsc-handler→dispatchMatchedPage→renderAppPageLifecycle) is consistent and correctly typed across every hop, including the generatedapp-rsc-entry. - Dynamic-detection masking is genuinely fixed, not deferred.
recordProbeDependenciesnowpeekDynamicUsage()s the probe's child scope before it's discarded and folds it intodynamicUsageObserved(app-layout-param-observation.ts:150), andisAppLayoutObservationUnsafeForStaticReusetreats it as unsafe (:55). So the observation signal is now a strict superset of the old Layer-3dynamicDetectedpath — amarkDynamicUsage()with no paired record (e.g."use cache: private") can no longer produce a false-static. Pinned bytests/app-layout-param-observation.test.ts. - Debug reason/flag agreement is fixed: the flag is recomputed before
debugClassificationand aruntime-probereason is emitted on a flip (app-page-execution.ts:418-435). Pinned bytests/app-page-execution.test.ts. revalidate=falsecomment corrected; merge precedence documented increateEffectiveLayoutClassifications; write-only WeakMap clearly annotated.- Manifest is correctly NOT a cache variant:
VINEXT_CLIENT_REUSE_MANIFEST_HEADERis absent fromVINEXT_RSC_VARY_HEADER(app-rsc-cache-busting.ts:32-42), pinned by the cache-busting test.
Confirmed behavioral changes (documented + pinned): the full-subtree error probe (probeReactServerSubtree recurses child server components, passing a truthy <Fragment/> as children) and the __layoutFlags output change. Both are now in the description's "Behavioral changes" section with parity assertions. I verified probeLayoutForErrors routes any child notFound()/redirect() through onLayoutError → special-error response, and non-special throws are swallowed (app-page-execution.ts:494-503), and probe-phase fetch() is deduped against the real render because runWithIsolatedProbeDependencies does not reset currentFetchDedupeEntries. Sound.
One new finding (non-blocking): the manifest producer runs on the visited-response cache hit path, wasting work. Detail inline.
Nothing blocking for a stacked plumbing slice given the stated non-goals.
| const clientReuseManifestHeader = | ||
| navigationKind === "navigate" | ||
| ? createClientReuseManifestHeaderFromVisibleAppState(routerStateAtNavStart) | ||
| : null; |
There was a problem hiding this comment.
Minor (perf, not correctness): the manifest is produced before the getVisitedResponse cache check below (line 1675), so createClientReuseManifestHeaderFromVisibleAppState runs on every navigate — including navigations that resolve entirely from the visited-response cache and never send a request. The producer is not free: it iterates metadata.layoutIds, builds a cache-variant proof per static layout (buildCacheVariantWithRouteBudget), and then binary-searches the byte budget in serializeBoundedClientReuseManifest.
It also doesn't affect createRscRequestUrl — the manifest header is excluded from VINEXT_RSC_VARY_HEADER, so it never changes the cache-busting hash. So on the cache-hit path the work is pure waste.
Not blocking (it's bounded by the skip-verification entry budget), but since this is on the soft-nav hot path, consider deferring production until after the getVisitedResponse miss is confirmed, or memoizing per routerStateAtNavStart.
There was a problem hiding this comment.
Fixed. Manifest production is now deferred to just before the network fetch (gated on the same navigationKind === "navigate" condition, computed from the captured nav-start router state), so soft navigations that resolve from the visited-response cache or a prefetch no longer run createClientReuseManifestHeaderFromVisibleAppState / buildCacheVariantWithRouteBudget / the byte-budget binary search. Since the header is excluded from VINEXT_RSC_VARY_HEADER, the cache-busting URL and the emitted request header are both unchanged on the request path. All browser-entry navigation tests still pass.
| // observable trace (e.g. `"use cache: private"`) would otherwise be lost | ||
| // when the child scope resets `dynamicUsageDetected`, masking the Layer-3 | ||
| // `dynamicDetected` signal this probe wiring replaced. | ||
| if (peekDynamicUsage()) { |
There was a problem hiding this comment.
Good fix — folding the child-scope dynamicUsageDetected in here (via the new non-resetting peekDynamicUsage()) closes the masking gap cleanly, and isAppLayoutObservationUnsafeForStaticReuse now treats the whole observation as a strict superset of the old Layer-3 dynamicDetected signal. Worth confirming once more before the enable slice keys skips off __layoutFlags: this peek captures the flag at recordProbeDependencies time, which for the async-probe path runs in the .then/.catch after the layout promise settles (line 242/252). Any markDynamicUsage() that fires strictly after the probe's awaited result resolves (e.g. in a not-yet-awaited child microtask the probe didn't surface) would be outside the isolated scope by then and wouldn't be peeked. The subtree probe's recursive await visit(...) should drain those, but it's the one residual edge to keep in mind for the enable slice's safety hardening.
There was a problem hiding this comment.
Thanks — agreed on the residual edge. For this plumbing slice the fold-in closes the divergence the rewrite introduced (the two signals now agree on everything the awaited probe surfaces). The remaining case you flag — a markDynamicUsage() in a microtask that fires strictly after the awaited result resolves and that the subtree probe's recursive await visit(...) didn't drain — is a pre-existing observation-completeness concern rather than something this PR regresses, and it lands on the safe side anyway: an incompletely-drained probe leaves completeness: "unknown", which isAppLayoutObservationUnsafeForStaticReuse already treats as unsafe (render-and-send). I've left a note for the enable slice's safety-hardening pass to confirm the drain is exhaustive before skips key off __layoutFlags.
|
Review posted successfully. SummaryI reviewed PR #1717 (client reuse manifest plumbing, slice 3 of 4) end to end. The prior 5 Confirmed resolved:
New finding (non-blocking): The manifest producer runs before the visited-response cache check in I also left a forward-looking note on the async-probe peek timing for the enable slice's safety hardening. Nothing blocking for a stacked plumbing slice. |
…path The manifest header is excluded from VINEXT_RSC_VARY_HEADER, so it never affects the cache-busting URL or the visited-response cache lookup. Producing it up front ran createClientReuseManifestHeaderFromVisibleAppState (iterates visible layout ids, builds a cache-variant proof per static layout, then binary-searches the byte budget) on every soft navigate — including ones that resolve entirely from the visited-response cache or a prefetch and never send a request, where the work is pure waste. Move production to just before the network fetch, gated on the same `navigationKind === "navigate"` condition and computed from the captured nav-start router state, so the emitted header is unchanged on the request path while the cache-hit/prefetch-hit hot paths skip it entirely.
|
/bigbonk review |
There was a problem hiding this comment.
Review — client reuse manifest plumbing (slice 3 of 4)
Reviewed end to end. The six prior /bigbonk rounds were thorough and every material finding was addressed, so I independently re-verified the resolutions rather than re-litigating them. They hold up:
Verified correct:
- Threading (browser producer →
normalizeRscRequest→app-rsc-handler→dispatchMatchedPage→renderAppPageLifecycle) is consistent and correctly typed across every hop, including the generatedapp-rsc-entry. - Dynamic-detection masking is genuinely fixed.
runWithUnifiedStateMutationshallow-clonesdynamicUsageDetected(a primitive) into the child probe scope, so probe-scopedmarkDynamicUsage()cannot pollute the real render scope; andrecordProbeDependenciesnowpeekDynamicUsage()s the child scope before it is discarded and folds it intodynamicUsageObserved, whichisAppLayoutObservationUnsafeForStaticReusetreats as unsafe. The observation is a strict superset of the old Layer-3dynamicDetectedsignal. This fix also covers the build-time-classified branch (which probes viaprobeLayoutForErrorswithout an isolated dynamic scope), sinceisLayoutObservationDynamicis checked after the probe completes. revalidate=falsedocblock is now accurate. I confirmed the build-timeextractNumberFromConstInitializernormalizesfalse→Infinity(build/report.ts:238), soclassifyLayoutSegmentConfigand the newclassifyLayoutSegmentConfigFromModuleagree onfalse→static. The genuine difference is scope (request-time reads the resolved module value, so it also classifies computed/dev-mode layouts the static AST extractor can't).- Mounted-slots refactor is behavior-preserving.
getMountedSlotIdsHeaderreturnsnull(never"") vianormalizeMountedSlotsHeader, so moving tocreateRscRequestHeaders's!== null && !== undefinedguard is equivalent. - Manifest is correctly NOT a cache variant.
VINEXT_CLIENT_REUSE_MANIFEST_HEADERis absent fromVINEXT_RSC_VARY_HEADER, pinned by the cache-busting test, so it never changes the cache-busting hash. - Producer is robust to missing wire fields.
readAppElementsMetadatadefaults missing__layoutFlagsto{}, solayoutFlags[id] !== "s"safely skips. - The deferred manifest production (now gated behind the prefetch/optimistic/cache miss) correctly avoids wasted work on the soft-nav hot path.
Behavioral changes (full-subtree error probe + __layoutFlags flips) are documented in the description and pinned by parity tests — agreed they are more Next-accurate.
Nothing blocking for a stacked plumbing slice. Two non-blocking notes inline for the enable slice.
| options.clientReuseManifestHeader !== undefined && | ||
| options.clientReuseManifestHeader !== null | ||
| ) { | ||
| headers.set(VINEXT_CLIENT_REUSE_MANIFEST_HEADER, options.clientReuseManifestHeader); |
There was a problem hiding this comment.
Forward-looking note for the enable slice (not this PR): the manifest header is deliberately excluded from VINEXT_RSC_VARY_HEADER, so it does not split the cache key. That is correct here because the manifest is carried but never authorizes a skip. Once the enable slice uses the manifest to omit RSC payload, a skip-shortened response must not be storable in any shared cache under the same key as a full response for that URL — otherwise a client that sent a manifest could poison the entry served to a client that didn't. Worth an explicit Cache-Control: private/no-store (or a cache-key dimension) decision in the enable PR, since the vary-header exclusion means shared caches won't distinguish the two responses on their own.
There was a problem hiding this comment.
Agreed and noted for the enable slice. This PR keeps the manifest header out of VINEXT_RSC_VARY_HEADER because it is carried but never authorizes a skip (no payload omission, no cache-control change). When the enable slice uses the manifest to shorten the RSC payload, a skip-shortened response must not be storable in any shared cache under the same key as a full response for that URL — so that PR will need an explicit Cache-Control: private/no-store or a cache-key dimension decision. Captured as a follow-up for the enable/SKIP-05 hardening work.
| } | ||
|
|
||
| if (entries.length === 0) { | ||
| return null; |
There was a problem hiding this comment.
Minor robustness note (non-blocking): serializeBoundedClientReuseManifest is only called with entries.length > 0, and its binary search starts at low = 1. If a single entry already exceeds maxManifestBytes, best stays null and the function returns null — correct. Just confirming this is intentional: an oversized single entry yields no header at all rather than a one-entry header that blows the budget. Fine as-is; flagging so the enable slice doesn't assume a non-null entries input guarantees a non-null serialized result.
There was a problem hiding this comment.
Confirmed intentional. An oversized single entry returning null (no header) rather than a one-entry header that blows maxManifestBytes is the desired behavior — the server treats an absent manifest as render-and-send, which is the safe default. Noted that a non-null entries input does not guarantee a non-null serialized result, so the enable slice won't assume otherwise. No change needed here.
|
Review posted successfully. SummaryI reviewed PR #1717 (client reuse manifest plumbing, slice 3 of 4) end to end. The six prior Confirmed correct (verified against source, not just the diff):
Two non-blocking notes for the enable slice:
No blocking issues for a stacked plumbing slice. |
2 participants
What this changes
Adds the transport wiring that carries a client reuse manifest from the browser to the server. The browser now derives a bounded manifest from the visible AppElements state and attaches it on soft navigations; the server parses it and threads the parsed result through the RSC handler, the generated app entry, page dispatch, and the render lifecycle. Nothing reads the manifest to authorize a skip yet — no skip decision, no RSC payload omission, no cache-control change.
Note: this is not a pure no-op plumbing slice. Two observable, intentional (more Next-accurate) behavioral changes ride along with the wiring so the enable slice has correct inputs to consume; both are called out under "Behavioral changes" below and pinned by parity assertions.
Bonk: Pls read #1437 to see the context. Tests will come in the next PR. Please do not block on missing tests since churning is time wasting. Focus on the logic/implementation please.
Why
The static-layout skip planner (#1674) and observation foundations (#1672) have no input to act on until a manifest actually reaches the server. Landing the plumbing on its own keeps it reviewable in isolation and lets the enable-transport slice focus purely on the skip decision and payload omission.
Approach
app-browser-client-reuse-manifest: bounded producer that builds the manifest from visible AppElements state and caps entries to the skip planner's verification budget. Attached asX-Vinext-Client-Reuse-Manifestonly for normalnavigatetransitions (not refresh).client-reuse-manifest: parse the header into a render/send-by-default disposition.app-rsc-handler-> generatedapp-rsc-entry->app-page-dispatch->app-page-render.app-layout-param-observation) and segment-config classification into dispatch, and register per-element render dependencies viaapp-page-route-wiring, so the enable slice can read them.releaseAppElementRenderDependencyconsumer (that lands with enable).Behavioral changes (intentional, not no-op)
children: null.probeLayoutAtwas rewritten to go throughprobeAppPageLayoutWithTracking->probeReactServerSubtree, which (a) passes<Fragment />(truthy) aschildreninstead ofnulland (b) recursively invokes child server components. BecauseprobeLayoutForErrorsruns on the build-time-classified path too, a layout whose chrome only renders whenchildrenis truthy (if (!children) return null), or a child component deep in the subtree, can now thrownotFound()/redirect()(or runfetch/side effects) during the probe where it previously did not — which can change the emitted HTTP response on an App Router request. This matches Next's render order (layouts render with their children present) and is required so observation tracking sees the dependencies created below a layout's immediate body. Pinned bytests/app-page-route-wiring.test.ts("probes layout branches that render only when children are present").__layoutFlagsoutput can change.isLayoutObservationDynamicis now live in classification and flips a build-timestaticlayout todwheneverisAppLayoutObservationUnsafeForStaticReuseis true (finite revalidate, cache tags, request APIs, param scope, etc.). Additionally,classifyLayoutSegmentConfigFromModuleruns at request time and is merged on top of the build-time map (module segment config is authoritative), so layouts not captured at build time (e.g. dev mode) are now classified.__layoutFlagsis serialized to the wire (app-elements-wire.ts) and stored in client router state (app-browser-state.ts), so the flags a client receives — and which entries enter the manifest — can differ from before this PR. Pinned bytests/app-page-execution.test.ts("flips a build-time static layout to dynamic when a runtime observation is unsafe for static reuse") plus the existing classification suite.Validation
vp check: format, lint, type check passed.vp test run(full suite): 243 files, 6827 tests passed.tests/app-rsc-handler.test.ts("passes parsed ClientReuseManifest hints..."),tests/app-browser-client-reuse-manifest.test.ts, andtests/app-rsc-cache-busting.test.ts(manifest attached without becoming a shared cache variant).Risks / follow-ups
maxEntryIdLength,maxEntryCount) and the byte budget (maxManifestBytes, enforced inserializeBoundedClientReuseManifest) are not independently observable: an entry can pass the per-entry guards yet still be trimmed by the byte budget. The enable slice must not assumelayoutFlags[id] === "s" && id in elementsimplies the entry made it into the emitted header.Refs #726