feat: static/dynamic layout detection for skip-header optimization#767
Conversation
…tion Reads `export const dynamic` and `export const revalidate` from layout source files to classify them as static or dynamic. Unlike page classification, positive revalidate values return null (ISR is a page concept), deferring to module graph analysis for layout skip decisions.
NathanDrake2406
force-pushed
the
feat/layout-persistence-pr-6-static-dynamic
branch
from
a212cf5 to
8bd1d01
Compare
commit: |
NathanDrake2406
force-pushed
the
feat/layout-persistence-pr-6-static-dynamic
branch
from
8bd1d01 to
41dd434
Compare
|
/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: Static/dynamic layout detection for skip-header optimization
Clean, well-structured PR. The three-layer classification design is sound, the test coverage is thorough (31 new tests across 7 files), and backward compatibility is properly maintained. A few observations below, none blocking.
Architecture
The separation of concerns is good: Layer 1 (segment config) and Layer 2 (module graph) are build-time and live in build/, while Layer 3 (runtime probe) stays in server/. The LayoutClassificationOptions grouping enforces the all-or-nothing invariant cleanly.
The type gap between classifyAllRouteLayouts returning "needs-probe" and buildTimeClassifications accepting only "static" | "dynamic" is intentional and correct — the wiring layer in PR 768 will filter "needs-probe" entries. Worth noting for anyone reading this in isolation.
Wire format
The "s" / "d" compact flags are fine for a payload that goes over the wire on every RSC navigation. parseLayoutFlags correctly validates at the boundary and defaults gracefully.
Minor items below
| const queue: string[] = [layoutModuleId]; | ||
|
|
||
| while (queue.length > 0) { | ||
| const currentId = queue.shift()!; |
There was a problem hiding this comment.
Nit: queue.shift() is O(n) on JS arrays. For typical layout dependency graphs this won't matter, but if you ever want to make this more robust against pathological module graphs (e.g., barrel files that re-export hundreds of modules), you could swap to a pointer-based approach:
| const currentId = queue.shift()!; | |
| let head = 0; |
…and then const currentId = queue[head++] instead of queue.shift(). Not blocking — just noting since this runs once per layout at build time and the visited set bounds the work.
| return response; | ||
| const buildTimeResult = cls?.buildTimeClassifications?.get(layoutIndex); | ||
|
|
||
| if (cls && buildTimeResult) { |
There was a problem hiding this comment.
Subtle: cls && buildTimeResult means buildTimeResult === undefined falls through to the Layer 3 probe (correct), but it also means a layout that was explicitly not in the buildTimeClassifications map falls through to isolated-scope probing. This is the right behavior, but the condition reads as "if cls is non-null AND buildTimeResult is truthy" — since the classification values are "static" | "dynamic" (both truthy strings), this works. Worth a brief comment that undefined (not in map) intentionally falls through.
Also: if buildTimeClassifications is an empty Map or null, every layout falls through to the Layer 3 cls branch, which is correct and tested.
| if (cls) { | ||
| // Layer 3: probe with isolated dynamic scope to detect per-layout | ||
| // dynamic API usage (headers(), cookies(), connection(), etc.) | ||
| try { |
There was a problem hiding this comment.
In the Layer 3 catch path (line 146), the error is passed to options.onLayoutError which resolves special errors (redirect/not-found). But the error originates from inside runWithIsolatedDynamicScope, which wraps the probe. If runWithIsolatedDynamicScope itself throws (implementation bug, not the layout function), the error still gets routed through onLayoutError and the layout is flagged dynamic. That seems like the right conservative behavior, but it does mean implementation bugs in the scope-isolation machinery would be silently swallowed as "dynamic layout" rather than surfacing as a crash. Probably fine for production, but worth a debug log in the future.
| } | ||
|
|
||
| export function readAppElementsMetadata(elements: AppElements): AppElementsMetadata { | ||
| function isLayoutFlagsRecord(value: unknown): value is LayoutFlags { |
There was a problem hiding this comment.
Minor: isLayoutFlagsRecord returns true for {} (empty object), which is intentional — an empty flags record is valid. Good.
One edge case: if the RSC payload somehow contains a __layoutFlags value that is a non-plain object (e.g., a Date, RegExp, or Map), the typeof value !== "object" check passes, and Object.values() would enumerate own properties. In practice this can't happen since the RSC wire format only produces plain objects, but the guard is correct regardless.
| return { | ||
| elements: mergeElements(state.elements, action.elements, action.type === "traverse"), | ||
| interceptionContext: action.interceptionContext, | ||
| layoutFlags: { ...state.layoutFlags, ...action.layoutFlags }, |
There was a problem hiding this comment.
The merge semantics are clean: navigate/traverse spread { ...state.layoutFlags, ...action.layoutFlags } (cumulative), replace uses only action.layoutFlags. This means layout flags accumulate across soft navigations, which is what you want for skip-header — once a layout is known static, that knowledge persists until a hard navigation.
One thing to consider for the wiring PR: a layout that was "s" on one route might be "d" on another (e.g., a shared root layout that conditionally calls headers() based on a child import). The merge here would update "s" → "d", which is correct (latest probe wins). But it would never go back from "d" to "s" on a subsequent navigate to the original route, unless the server re-sends "s". As long as the server always sends flags for all layouts in the current tree, this is fine.
| options: RenderAppPageLifecycleOptions, | ||
| ): Promise<Response> { | ||
| const preRenderResponse = await probeAppPageBeforeRender({ | ||
| const preRenderResult = await probeAppPageBeforeRender({ |
There was a problem hiding this comment.
Good that classification is not passed here yet — the backward-compatible path (no classification, empty layoutFlags) runs until PR 768 wires it in. The preRenderResult.layoutFlags is computed but unused, which is expected for this intermediate PR.
| layoutCount: 2, | ||
| onLayoutError() { | ||
| return Promise.resolve(null); | ||
| }, |
There was a problem hiding this comment.
Nice test for the error → conservative dynamic fallback. The runWithIsolatedDynamicScope here does Promise.resolve(fn()) which means if fn() throws synchronously (as probeLayoutAt does for layoutIndex === 1), the Promise.resolve() catches it and creates a rejected promise that propagates to the outer try/catch. This correctly exercises the catch path in probeAppPageLayouts.
One scenario not explicitly tested: what happens when the layout probe for a build-time classified layout (buildTimeResult is set) throws a special error? The probeLayoutForErrors path handles it, but there's no test with both buildTimeClassifications and a throwing probeLayoutAt for a classified layout. The "still handles special errors with classification enabled" test in app-page-probe.test.ts covers this at the probeAppPageBeforeRender level (no buildTimeClassifications in that test, so it goes through Layer 3), but not the build-time + error combo. Minor gap.
|
Review submitted successfully. Here's a summary of what I covered: Overall assessment: Clean, well-structured PR with sound three-layer classification design and thorough test coverage (31 new tests across 7 files). No blocking issues. Key observations posted:
|
BFS traversal of each layout's dependency tree via Vite's module graph. If no transitive dynamic shim import (headers, cache, server) is found, the layout is provably static. Otherwise it needs a runtime probe. classifyAllRouteLayouts combines Layer 1 (segment config, from prior commit) with Layer 2 (module graph), deduplicating shared layouts.
Extends probeAppPageLayouts to return per-layout flags ("s"/"d")
alongside the existing Response. Three paths per layout:
- Build-time classified: pass flag through, still probe for errors
- Needs probe: run with isolated dynamic scope, detect usage
- No classification: original behavior (backward compat)
probeAppPageBeforeRender propagates layoutFlags through the result.
renderAppPageLifecycle updated to destructure the new return type.
Adds APP_LAYOUT_FLAGS_KEY to the RSC payload metadata, carrying
per-layout static/dynamic flags ("s"/"d"). readAppElementsMetadata
now parses layoutFlags with a type predicate guard.
AppRouterState and AppRouterAction carry layoutFlags. Navigate merges
flags (preserving previously-seen layouts), replace replaces them.
All dispatchBrowserTree call sites updated to pass layoutFlags.
…onOptions type The three optional fields (buildTimeClassifications, getLayoutId, runWithIsolatedDynamicScope) had an all-or-nothing invariant enforced only at runtime. Grouping them into a single optional `classification` object makes the constraint type-safe — you either provide the full classification context or nothing. Also deduplicates the LayoutFlags type: canonical definition lives in app-elements.ts, re-exported from app-page-execution.ts.
When runWithIsolatedDynamicScope throws and the error is non-special (onLayoutError returns null), the layout was silently omitted from layoutFlags. Now conservatively defaults to "d" — if probing failed, the layout cannot be proven static.
NathanDrake2406
force-pushed
the
feat/layout-persistence-pr-6-static-dynamic
branch
from
41dd434 to
4392f14
Compare
|
Thanks for the thorough pass. Walking through each item: Addressed#1 ( #7 ( Skipped intentionally#2 ( #3 ( Acknowledged, no action needed#4 ( #5 ( #6 ( |
|
/bigbonk review |
There was a problem hiding this comment.
Review: Static/dynamic layout detection
Clean, well-designed PR. The three-layer classification is well-separated (build-time in build/, runtime in server/), the backward compatibility path is solid, and the test coverage is thorough. The previous review's feedback has been addressed. A few minor observations below — none blocking.
What looks good
- Type design:
LayoutClassificationOptionsas a single optional object enforces the all-or-nothing invariant cleanly. The gap betweenclassifyAllRouteLayoutsreturning"needs-probe"andbuildTimeClassificationsaccepting only"static" | "dynamic"is correctly intentional — PR 768 will filter. - Backward compat: The
classification?: ... | nulloptional field means the entire probe path is a no-op addition when classification isn't wired. TheparseLayoutFlags→{}default handles missing wire data gracefully. - Wire validation:
isLayoutFlagsRecordis strict enough for the wire format while handling edge cases (empty objects, non-plain objects). - Merge vs replace semantics: Navigate/traverse accumulate flags, replace resets. This is the correct behavior for the skip-header optimization.
- BFS pointer fix: The
headpointer approach from the previous review is clean.
Minor observations (all non-blocking)
|
|
||
| // Layer 2: module graph | ||
| const graphResult = classifyLayoutByModuleGraph( | ||
| route.layouts[i], |
There was a problem hiding this comment.
Nit: route.layouts[i] is typed as string[], so the index access returns string | undefined under noUncheckedIndexedAccess. The loop bounds (i < route.layouts.length) guarantee it's in range, but the non-null assertion is missing (unlike line 86 where layoutTreePositions[i] uses ?? 0). This is fine since the project likely doesn't enable that tsconfig flag, but worth noting for consistency.
Not blocking — just flagging the implicit assumption that layouts and layoutTreePositions arrays are always the same length. The type doesn't enforce this invariant.
| // Layer 3: probe with isolated dynamic scope to detect per-layout | ||
| // dynamic API usage (headers(), cookies(), connection(), etc.) | ||
| try { | ||
| const { dynamicDetected } = await cls.runWithIsolatedDynamicScope(() => |
There was a problem hiding this comment.
The Layer 3 catch path (line 146) conflates "the layout function threw" with "runWithIsolatedDynamicScope itself threw". Both get flagged as dynamic and routed through onLayoutError. This is the right conservative behavior, but it means an implementation bug in the scope-isolation machinery would be silently classified as dynamic rather than surfacing. The author addressed this in the previous review (no actionable signal without a known failure mode) — just reaffirming this is a reasonable tradeoff for now.
| options: RenderAppPageLifecycleOptions, | ||
| ): Promise<Response> { | ||
| const preRenderResponse = await probeAppPageBeforeRender({ | ||
| const preRenderResult = await probeAppPageBeforeRender({ |
There was a problem hiding this comment.
Good: preRenderResult.layoutFlags is computed but not passed to probeAppPageBeforeRender's classification option, so this runs the backward-compatible path (no classification, empty layoutFlags). The wiring lands in PR 768. No issue here — just confirming the intermediate state is correct.
| return { | ||
| elements: mergeElements(state.elements, action.elements, action.type === "traverse"), | ||
| interceptionContext: action.interceptionContext, | ||
| layoutFlags: { ...state.layoutFlags, ...action.layoutFlags }, |
There was a problem hiding this comment.
Minor thought: the spread merge { ...state.layoutFlags, ...action.layoutFlags } means stale flags for layouts no longer in the current tree persist indefinitely (until a replace/hard navigation). This is harmless for the skip-header optimization (the consumer in PR 768 will only read flags for layouts actually in the current tree), but the state object grows monotonically across soft navigations. Not a practical concern for typical apps, but worth keeping in mind if layout flags ever carry more data.
|
Review submitted with approval. Here's a summary of what I covered: Verdict: Approve — Clean, well-designed PR with no blocking issues. Key observations posted as inline comments:
|
- Introduce LayoutEntry (moduleId, treePosition, segmentConfig) in layout-classification.ts, collapsing the three parallel arrays (layouts/layoutTreePositions/layoutSegmentConfigs) in RouteForClassification into a single struct. Switches the inner loop to for...of, removing the ?? 0 fallback that masked index mismatches. - Update all five classifyAllRouteLayouts test fixtures to the struct shape. - Add a JSDoc on LayoutFlags clarifying that "s" = static, "d" = dynamic. - Tighten the buildTimeClassifications comment to say "keyed by layout index". https://claude.ai/code/session_01MDJzCNao3dX9tmmVtSqsw5
This reverts commit 8e09f70.
This reverts commit 1faf2cd.
| // Probe failed — conservatively treat as dynamic. | ||
| layoutFlags[cls.getLayoutId(layoutIndex)] = "d"; |
There was a problem hiding this comment.
i wonder if it's worth having a separate flag for failures that we interpret as dynamic but still track as being errored
|
sorry about the delay |
3 participants
Summary
Part of #726 (Phase 3, PR 1). Adds a three-layer classification system that determines whether each layout in an App Router route tree is static or dynamic. This metadata is the foundation for PR 7's skip-header optimization, which avoids re-rendering static layouts on RSC navigations. Wiring is done in PR 768.
Three-layer classification
Layer 1 — Segment config (
classifyLayoutSegmentConfiginreport.ts). Readsexport const dynamic/export const revalidatefrom layout source files. Unlike page classification, positiverevalidatevalues return null (ISR is a page concept) — only the extremes (0→ dynamic,Infinity→ static) are decisive.Layer 2 — Module graph (
classifyLayoutByModuleGraphinlayout-classification.ts). BFS traversal of each layout's dependency tree via aModuleInfoProviderabstraction over Vite'sthis.getModuleInfo. If no transitive dynamic shim import (headers, cache, server) is found, the layout is provably static without any runtime cost.Layer 3 — Runtime probe (extended
probeAppPageLayoutsinapp-page-execution.ts). For layouts that import dynamic shims but may not call them on every request, the probe runs the layout function with isolated dynamic scope tracking. TherunWithIsolatedDynamicScopecallback is injected from the RSC entry, keeping the execution module decoupled from ALS internals.Wire format
__layoutFlagsmetadata key in the RSC payload:Record<string, "s" | "d">mapping layout IDs to compact flags. Follows the established pattern of__route,__rootLayout,__interceptionContext.On client-side navigation, the router merges new flags with existing ones (navigate) or replaces them entirely (replace), building up cumulative knowledge of which layouts are safe to skip.
Type design
LayoutClassificationOptionsgroups the three classification fields (getLayoutId,runWithIsolatedDynamicScope,buildTimeClassifications) into a single optional object. This makes the all-or-nothing invariant type-safe — you either provide the full classification context or nothing.LayoutFlagsis canonically defined inapp-elements.tsand re-exported fromapp-page-execution.ts.readAppElementsMetadataacceptsRecord<string, unknown>since the RSC payload carries heterogeneous values.parseLayoutFlagsvalidates at the wire boundary using a type predicate guard.New files
packages/vinext/src/build/layout-classification.tstests/layout-classification.test.tsTest plan
tests/build-report.test.ts— 7 tests forclassifyLayoutSegmentConfigtests/layout-classification.test.ts— 13 tests for module graph traversal + combined classificationtests/app-page-execution.test.ts— 5 tests for per-layout probe classification (dynamic detection, build-time skip, backward compat, error default)tests/app-page-probe.test.ts— 2 tests for layout flags propagation + special error handling with classificationtests/app-elements.test.ts— 2 tests for__layoutFlagsmetadata parsing + backward compattests/app-browser-entry.test.ts— 2 tests for layoutFlags merge (navigate) and replace semanticstests/app-router.test.ts— 276 integration tests pass with zero regressionsvp check— format, lint, type checks all cleanRefs #726
Closes #756