feat: flat keyed payload for App Router layout persistence#750
feat: flat keyed payload for App Router layout persistence#750NathanDrake2406 wants to merge 37 commits intocloudflare:mainfrom
Conversation
- Fix stale closure on readBrowserRouterState by using a useRef updated synchronously during render instead of a closure captured in useLayoutEffect. External callers (navigate, server actions, HMR) now always read the current router state. - Restore GlobalErrorBoundary wrapping that was dropped when switching from buildPageElement to buildAppPageElements. Apps with app/global-error.tsx now get their global error boundary back. - Add exhaustive default case to routerReducer so new action types produce a compile error and a runtime throw instead of silent undefined. - Remove dead code: createRouteNodeSnapshot, AppRouteNodeSnapshot, AppRouteNodeValue were defined but never imported. - Remove deprecated buildAppPageRouteElement and its test — no production callers remain after the flat payload cutover. - Short-circuit normalizeAppElements when no slot keys need rewriting to avoid unnecessary allocation on every payload. - Align test data in error boundary RSC payload test (matchedParams slug: "post" -> "missing" to match requestUrl /posts/missing).
commit: |
createFromReadableStream() returns a React thenable whose .then() returns undefined (not a Promise). Chaining .then(normalizeAppElements) broke SSR by assigning undefined to flightRoot. Fix: call use() on the raw thenable, then normalize synchronously after resolution. Also widen renderAppPageLifecycle element type to accept flat map payloads.
The SSR entry always expects a flat Record<string, ReactNode> with __route and __rootLayout metadata from the RSC stream. Three paths were still producing bare ReactNode payloads: 1. renderAppPageBoundaryElementResponse only created the flat map for isRscRequest=true, but HTML requests also flow through RSC→SSR 2. buildPageElements "no default export" early return 3. Server action "Page not found" fallback All three now produce the flat keyed element map, fixing 17 test failures across 404/not-found, forbidden/unauthorized, error boundary, production build, rewrite, and encoded-slash paths.
- Update renderElementToStream mock to extract the route element from the flat map before rendering to HTML (mirrors real SSR entry flow) - Update entry template snapshots for the buildPageElements changes
createFromReadableStream() returns a React Flight thenable whose .then() returns undefined instead of a new Promise. The browser entry's normalizeAppElementsPromise chained .then() on this raw thenable, producing undefined — which crashed use() during hydration with "An unsupported type was passed to use(): undefined". Wrapping in Promise.resolve() first converts the Flight thenable into a real Promise, making .then() chains work correctly. The same fix was already applied to the SSR entry in 5395efc but was missed in the browser entry.
React 19.2.4's use(Promise) during hydration triggers "async Client Component" because native Promises lack React's internal .status property (set only by Flight thenables). When use() encounters a Promise without .status, it suspends — which React interprets as the component being async, causing a fatal error. Fix: store resolved AppElements directly in ElementsContext and router state instead of Promise<AppElements>. The navigation async flow (createPendingNavigationCommit) awaits the Promise before dispatching, so React state never holds a Promise. - ElementsContext: Promise<AppElements> → AppElements - AppRouterState.elements: Promise<AppElements> → AppElements - mergeElementsPromise → mergeElements (sync object spread) - Slot: useContext only, no use(Promise) - SSR entry: pass resolved elements to context - dispatchBrowserTree: simplified, no async error handler Also fix flaky instrumentation E2E test that read the last error entry instead of finding by path.
- Remove Promise wrappers from ElementsContext test values - mergeElementsPromise → mergeElements (sync) - Replace Suspense streaming test with direct render test - Remove unused createDeferred helper and Suspense import - Update browser state test assertions (no longer async)
P1a: mergeElements preserves previous slot content when the new payload marks a parallel slot as unmatched. On soft navigation, unmatched slots keep their previous subtree instead of triggering notFound(). P1b: renderNavigationPayload now receives navId and checks for superseded navigations after its await. Stale payloads are discarded instead of being dispatched into the React tree. P2: The catch block in renderNavigationPayload only calls commitClientNavigationState() when activateNavigationSnapshot() was actually reached, preventing counter underflow. P3: The no-default-export fallback in buildPageElements now derives the root layout tree path from route.layoutTreePositions and route.routeSegments instead of hardcoding "/".
|
diff should look less scary after 2a and 2b are merged and this PR is rebased |
There was a problem hiding this comment.
Pull request overview
This PR switches vinext’s App Router RSC/SSR pipeline from a single monolithic ReactNode payload to a flat keyed Record<string, ReactNode> element map, enabling layout persistence across client navigations by merging element entries instead of replacing the whole tree.
Changes:
- Introduces the flat payload contract (
__route,__rootLayout) plus normalization helpers and unmatched-slot sentinel handling. - Adds server-side route wiring builder (
buildAppPageElements) and render-dependency barriers to ensure correct serialization ordering. - Updates SSR and browser router entrypoints to consume/merge the flat map and handle hard navigations on root layout switches.
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/slot.test.ts | New tests for Slot/Children/ParallelSlot behavior, unmatched-slot normalization, and merge semantics. |
| tests/error-boundary.test.ts | Expands tests to cover pathname-based reset behavior and digest classification. |
| tests/entry-templates.test.ts | Updates route fixtures to include templateTreePositions. |
| tests/e2e/app-router/instrumentation.spec.ts | Makes assertion resilient by locating the error entry by path. |
| tests/app-router.test.ts | Adds integration check that .rsc responses include flat payload metadata/keys. |
| tests/app-render-dependency.test.ts | New tests demonstrating and validating render dependency barrier behavior. |
| tests/app-page-route-wiring.test.ts | New unit tests for flat element-map building, tree-path IDs, and template/slot wiring. |
| tests/app-page-boundary-render.test.ts | Updates boundary rendering tests to validate flat payloads for RSC fallbacks/errors. |
| tests/app-elements.test.ts | New tests for payload normalization and __route/__rootLayout invariants. |
| tests/app-browser-entry.test.ts | New tests for client reducer merge/replace, hard-nav on root layout change, and commit deferral. |
| tests/snapshots/entry-templates.test.ts.snap | Snapshot updates reflecting extracted route wiring and flat payload generation. |
| packages/vinext/src/shims/slot.tsx | Adds client Slot primitives, merge semantics, and unmatched-slot handling. |
| packages/vinext/src/shims/error-boundary.tsx | Adds pathname-based reset via ErrorBoundaryInner wrapper to match Next.js behavior. |
| packages/vinext/src/server/app-ssr-entry.ts | SSR now deserializes flat payload, validates metadata, and renders via ElementsContext + Slot. |
| packages/vinext/src/server/app-render-dependency.tsx | Adds dependency primitives used to order serialization relative to async layouts/templates. |
| packages/vinext/src/server/app-page-route-wiring.tsx | Implements buildAppPageElements producing route/layout/template/page/slot entries + wiring. |
| packages/vinext/src/server/app-page-render.ts | Broadens render lifecycle types to allow flat element maps in renderToReadableStream. |
| packages/vinext/src/server/app-page-boundary-render.ts | Boundary RSC responses now emit minimal flat payloads with route/root-layout metadata. |
| packages/vinext/src/server/app-elements.ts | Defines payload keys, wire sentinel, normalization, and metadata reader. |
| packages/vinext/src/server/app-browser-state.ts | Introduces reducer/commit helpers for merge/replace and hard-nav detection. |
| packages/vinext/src/server/app-browser-entry.ts | Reworks client runtime to manage router state via reducer and merge flat payloads. |
| packages/vinext/src/routing/app-router.ts | Tracks templateTreePositions on routes for correct template tree-path IDs. |
| packages/vinext/src/entries/app-rsc-entry.ts | Generated app entry now delegates to buildAppPageElements and emits flat payloads. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (props.pathname !== state.previousPathname && state.error) { | ||
| return { error: null, previousPathname: props.pathname }; | ||
| } | ||
| return { error: state.error, previousPathname: props.pathname }; |
There was a problem hiding this comment.
getDerivedStateFromProps always returns a new state object, even when pathname hasn’t changed and there’s no error to reset. This causes an unnecessary state update on every render and can make updates harder to reason about. Consider returning null when no state change is needed (e.g., only update when pathname !== state.previousPathname, and clear error only when that change occurs while an error is set).
| if (props.pathname !== state.previousPathname && state.error) { | |
| return { error: null, previousPathname: props.pathname }; | |
| } | |
| return { error: state.error, previousPathname: props.pathname }; | |
| if (props.pathname !== state.previousPathname) { | |
| if (state.error) { | |
| return { error: null, previousPathname: props.pathname }; | |
| } | |
| return { error: state.error, previousPathname: props.pathname }; | |
| } | |
| return null; |
There was a problem hiding this comment.
Dismissing — this is a micro-optimization with no measurable impact. The fallback path returns an object with identical values to the current state. React's class component reconciliation compares the values, so no actual re-render is triggered. Error boundaries don't re-render frequently from their own state.
Also, NotFoundBoundaryInner at line 119-128 has the exact same pattern — flagging one but not the other suggests pattern-matching rather than analysis.
| export function mergeElements(prev: AppElements, next: AppElements): AppElements { | ||
| const merged: Record<string, AppElementValue> = { ...prev, ...next }; | ||
| // On soft navigation, unmatched parallel slots preserve their previous subtree | ||
| // instead of firing notFound(). Only hard navigation (full page load) should 404. | ||
| // This matches Next.js behavior for parallel route persistence. | ||
| for (const key of Object.keys(merged)) { | ||
| if (key.startsWith("slot:") && merged[key] === UNMATCHED_SLOT && key in prev) { | ||
| merged[key] = prev[key]; | ||
| } | ||
| } | ||
| return merged; |
There was a problem hiding this comment.
mergeElements spreads prev+next and then iterates over all merged keys to handle the unmatched-slot sentinel. As the elements map grows, this makes each navigation O(totalEntries) even if only a few slot keys changed. You can keep the same semantics by iterating only over next (or only over keys in next that are UNMATCHED_SLOT and start with slot:) and restoring from prev when appropriate.
There was a problem hiding this comment.
Dismissing — the elements map has ~10-50 entries per route (one per layout, template, page, slot). This is a single cache line. The suggestion to iterate only `next` keys is technically correct but the difference is nanoseconds on a map that runs once per navigation.
| // server actions, HMR) always read the current state. The ref is updated | ||
| // synchronously during render -- not in an effect -- so there is no stale | ||
| // window between React committing a new state and the effect firing. | ||
| const stateRef = useRef(treeState); | ||
| stateRef.current = treeState; | ||
| browserRouterStateRef = stateRef; | ||
|
|
There was a problem hiding this comment.
Assigning browserRouterStateRef = stateRef during render introduces a render-phase side effect. In concurrent rendering / Strict Mode, React may start rendering a new state and later abandon it, but external callers could still observe the uncommitted browserRouterStateRef.current. Consider only wiring the module-level reference in a commit-phase effect (e.g., set the ref object once in useLayoutEffect), and update .current in a commit-phase effect as well if you need to guarantee readers only see committed state.
| // server actions, HMR) always read the current state. The ref is updated | |
| // synchronously during render -- not in an effect -- so there is no stale | |
| // window between React committing a new state and the effect firing. | |
| const stateRef = useRef(treeState); | |
| stateRef.current = treeState; | |
| browserRouterStateRef = stateRef; | |
| // server actions, HMR) always read the current state. | |
| const stateRef = useRef(treeState); | |
| // Wire the module-level browserRouterStateRef and keep stateRef.current in | |
| // sync with the committed treeState during the commit phase, not during | |
| // render, to avoid exposing uncommitted state in concurrent/Strict Mode. | |
| useLayoutEffect(() => { | |
| stateRef.current = treeState; | |
| browserRouterStateRef = stateRef; | |
| }, [treeState]); |
There was a problem hiding this comment.
Rejecting — the existing code is deliberately correct and the suggestion would introduce the exact bug the code comment (lines 332-335) warns about.
`browserRouterStateRef` is updated during render so that external callers (`navigateRsc()`, server action callbacks, HMR handler) always read the committed state immediately, with no stale window between React's commit phase and effect execution. Moving this to `useLayoutEffect` creates a timing gap where `getBrowserRouterState()` reads stale state.
The concurrent-mode concern (abandoned render leaves stale `stateRef.current`) is theoretical here: external callers only run from event handlers and effects, never during render. The next committed render corrects the value. And `browserRouterStateRef = stateRef` is a no-op after mount (same ref object).
Next.js's own router uses this same render-phase ref update pattern for the same reason.
![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: f8e22769e2
ℹ️ 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".
| {children} | ||
| <ReleaseAppRenderDependency /> | ||
| </> |
There was a problem hiding this comment.
renderWithAppDependencyBarrier() only calls dependency.release() from a sibling component rendered after children. If a layout/template wrapped by this helper throws before that sibling renders, the dependency promise never resolves, and entries created with renderAfterAppDependencies() can remain suspended indefinitely (hanging or leaking an in-flight RSC response during error-boundary flows). The release path needs to run even when children errors.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Dismissing — this can't happen due to React's tree rendering model.
When a layout throws inside `renderWithAppDependencyBarrier`, React catches it at the nearest `ErrorBoundary`. The error boundary replaces the entire subtree below it with the fallback. All components that depend on this dependency (via `renderAfterAppDependencies`) are tree-descendants of the throwing layout — they never render because the error boundary already caught the error above them.
The dependency mechanism is safe precisely because the tree structure guarantees: every consumer is a descendant of the producer. An error at the producer level prevents the consumer from ever starting to render, so no component is suspended waiting for the unreleased promise.
| <Slot | ||
| id={layoutEntry.id} | ||
| parallelSlots={createAppPageParallelSlotEntries( |
There was a problem hiding this comment.
This Slot is always rendered for every layoutEntry, but earlier in buildAppPageElements() layouts with no default export are skipped and never written to elements[layoutEntry.id]. In that case Slot returns null, which drops layoutChildren and blanks the route subtree instead of preserving child rendering (the previous implementation simply skipped that layout wrapper). Guarding this Slot path for missing layout elements prevents silent content loss.
Useful? React with 👍 / 👎.
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
This is a well-architected change. The flat keyed payload design is clean and the separation of concerns is solid: app-elements.ts for wire format/normalization, app-browser-state.ts for the reducer/disposition logic, app-render-dependency.tsx for serialization ordering, and the updated app-page-route-wiring.tsx for the flat-map builder. The test coverage for the new primitives is thorough.
I agree with the second ask-bonk review that the prior automated reviews had several false positives (redirect header ordering, basePath rejection, public file signal, phantom applyAppRouterStateUpdate export). Those are non-issues — the redirect header ordering is preserved (middleware merge first, then framework .set() calls), and there's now an explicit test for it in app-router.test.ts.
Below: one duplication concern worth addressing now, one minor consistency observation, and a few smaller items.
| if (isServerActionResult(result)) { | ||
| updateBrowserTree( | ||
| result.root, | ||
| createClientNavigationRenderSnapshot(window.location.href, latestClientParams), | ||
| ++nextNavigationRenderId, | ||
| false, | ||
| const navigationSnapshot = createClientNavigationRenderSnapshot( | ||
| window.location.href, | ||
| latestClientParams, | ||
| ); | ||
| const currentState = getBrowserRouterState(); | ||
| const startedNavigationId = activeNavigationId; | ||
| const pending = await createPendingNavigationCommit({ | ||
| currentState, | ||
| nextElements: Promise.resolve(normalizeAppElements(result.root)), | ||
| navigationSnapshot, | ||
| renderId: ++nextNavigationRenderId, | ||
| type: "navigate", | ||
| }); | ||
| const disposition = resolvePendingNavigationCommitDisposition({ | ||
| activeNavigationId, | ||
| currentRootLayoutTreePath: currentState.rootLayoutTreePath, | ||
| nextRootLayoutTreePath: pending.rootLayoutTreePath, | ||
| startedNavigationId, | ||
| }); | ||
| // Known limitation: if a same-URL navigation fully commits while this | ||
| // server action is awaiting createPendingNavigationCommit(), the action | ||
| // can still dispatch its older payload afterward. The old pre-2c code had | ||
| // the same race, and Next.js has similar behavior. Tightening this would | ||
| // need a stronger commit-version gate than activeNavigationId alone. | ||
| if (disposition === "hard-navigate") { | ||
| window.location.assign(window.location.href); | ||
| return undefined; | ||
| } | ||
| if (disposition === "dispatch") { | ||
| dispatchBrowserTree( | ||
| pending.action.elements, | ||
| navigationSnapshot, | ||
| pending.action.renderId, | ||
| "navigate", | ||
| pending.routeId, | ||
| pending.rootLayoutTreePath, | ||
| false, | ||
| ); | ||
| } | ||
| if (result.returnValue) { | ||
| if (!result.returnValue.ok) throw result.returnValue.data; | ||
| return result.returnValue.data; | ||
| } | ||
| return undefined; |
There was a problem hiding this comment.
Duplication: server action commit logic repeated verbatim
The isServerActionResult(result) branch (lines 617-661) and the else branch (lines 663-698) contain ~30 lines of near-identical commit/dispatch logic. The only material differences are:
normalizeAppElements(result.root)vsnormalizeAppElements(result)- The
returnValuehandling after dispatch
This could be extracted into a helper:
async function commitServerActionPayload(
wireElements: AppWireElements,
): Promise<"dispatched" | "hard-navigated" | "skipped"> {
const navigationSnapshot = createClientNavigationRenderSnapshot(
window.location.href, latestClientParams,
);
const currentState = getBrowserRouterState();
const startedNavigationId = activeNavigationId;
const pending = await createPendingNavigationCommit({...});
const disposition = resolvePendingNavigationCommitDisposition({...});
if (disposition === "hard-navigate") {
window.location.assign(window.location.href);
return "hard-navigated";
}
if (disposition === "dispatch") {
dispatchBrowserTree(...);
return "dispatched";
}
return "skipped";
}Not blocking, but this is the kind of near-duplicate where a future fix to one branch (e.g., the Phase 2 commit-version gate from the TODO comment at line 637) could easily miss the other.
| // Keep the latest router state in a ref so external callers (navigate(), | ||
| // server actions, HMR) always read the current state. | ||
| const stateRef = useRef(treeState); | ||
| stateRef.current = treeState; |
There was a problem hiding this comment.
Nit: the comment block below (lines 337-341) explains the timing guarantee for when browserRouterStateRef becomes non-null, but doesn't explain why stateRef.current = treeState as a render-phase write is safe here. The author's earlier dismissal of Copilot's suggestion is correct (external readers only run from event handlers/effects), but a one-liner like // Safe: external readers run from effects/events, never during render would preempt future reviewers from raising the same concern.
| rscPayload, | ||
| createClientNavigationRenderSnapshot(window.location.href, latestClientParams), | ||
| ++nextNavigationRenderId, | ||
| const pending = await createPendingNavigationCommit({ |
There was a problem hiding this comment.
Minor: the HMR handler doesn't check disposition before dispatching — it always does a full replace. If the root layout tree path changes during HMR (unlikely but possible with route group restructuring), this would replace the tree with a mismatched root layout instead of triggering a hard navigation.
The navigation and server-action paths both check resolvePendingNavigationCommitDisposition(). The HMR path skips it because type: "replace" already replaces the entire tree, which is reasonable — but it means rootLayoutTreePath in state could silently become stale relative to the rendered tree. Not blocking since HMR is dev-only.
There was a problem hiding this comment.
Leaving this as-is for PR 2c. The production navigation and server-action paths already carry the disposition check; this branch is the HMR-only replace path, so I do not want to add more churn to a large stacked refactor for a dev-only edge case. If we decide HMR should enforce the same root-layout hard-nav parity, that is cleaner as a follow-up.
| // used inside the RSC render graph, where producing this leaf means the | ||
| // owning entry has reached the serialization point that downstream entries | ||
| // are allowed to observe. | ||
| dependency.release(); |
There was a problem hiding this comment.
I agree with the author's dismissal of the error-release concern. The tree structure guarantees consumers are unreachable descendants of a throwing producer.
Forward-looking note: if Phase 2+ adds a render timeout (AbortSignal), a stuck async layout would leave this dependency unresolved and downstream entries permanently suspended. A reject() path on the dependency (e.g., dependency.abort(reason)) triggered by AbortSignal would be the natural safety net. The released idempotency guard is good but only covers the happy path.
| element = ( | ||
| <NotFoundBoundary fallback={<LayoutNotFoundComponent />}>{element}</NotFoundBoundary> | ||
| ); | ||
| const rootLayoutTreePath = layoutEntries[0]?.treePath ?? null; |
There was a problem hiding this comment.
Nice. layoutEntries[0]?.treePath ?? null means rootLayoutTreePath correctly includes route group segments (e.g., /(marketing)). This is what makes shouldHardNavigate() in app-browser-state.ts work for route-group-scoped root layouts — navigating between /(marketing)/... and /(dashboard)/... triggers a hard nav because the tree paths differ, even though both have a root layout.
| // On soft navigation, unmatched parallel slots preserve their previous subtree | ||
| // instead of firing notFound(). Only hard navigation (full page load) should 404. | ||
| // This matches Next.js behavior for parallel route persistence. | ||
| for (const key of Object.keys(merged)) { |
There was a problem hiding this comment.
Noting for clarity: this loop iterates all keys of merged (union of prev and next). For keys that exist only in prev, the merged[key] === UNMATCHED_SLOT check is always false (previous elements are resolved values, never the sentinel), so the Object.hasOwn(prev, key) check is dead code for those entries. Functionally correct — just slightly over-iterating on a small map. Not worth changing.
| } | ||
|
|
||
| const rootLayoutTreePath = elements[APP_ROOT_LAYOUT_KEY]; | ||
| if (rootLayoutTreePath === undefined) { |
There was a problem hiding this comment.
Good — this now distinguishes "missing key" (line 50) from "invalid type" (line 53), which addresses the prior review feedback about the error message being misleading.
| } | ||
|
|
||
| void main(); | ||
| if (typeof document !== "undefined") { |
There was a problem hiding this comment.
Good defensive guard. Enables importing this module in test/SSR contexts without side effects.
|
The review was submitted successfully. Here's a summary of what I posted: Review summary for PR #750 (flat keyed payload for App Router layout persistence): The review confirms this is a well-architected change with solid separation of concerns and thorough test coverage. I cleared up that the prior automated review false positives (redirect header ordering regression, basePath rejection, public file signal) are indeed non-issues. Comments posted (8 inline):
|
|
/bigbonk review |
There was a problem hiding this comment.
Review: flat keyed payload for App Router layout persistence
This is a well-executed architectural shift. The core design — flat keyed Record<string, ReactNode> with merge semantics on soft navigation, replace on HMR — is clean and the separation into app-elements.ts (wire format), app-browser-state.ts (reducer/disposition), app-render-dependency.tsx (serialization DAG), and app-page-route-wiring.tsx (tree builder) is solid. The three-state slot semantics (absent → persisted, UNMATCHED_SLOT → notFound, present → render) correctly model Next.js parallel route persistence.
I verified the prior review concerns. The redirect header ordering is preserved (merge before .set()), and the new test at entry-templates.test.ts explicitly asserts this ordering. The basePath rejection and public file signal are intact. The getRoutePattern + matchSourceRouteParams → getRouteParamNames + pickRouteParams refactor in intercept handling is a nice simplification.
A few observations below — two substantive items and some minor notes.
| navigationSnapshot, | ||
| nextElements, | ||
| renderId: ++nextNavigationRenderId, | ||
| startedNavigationId, |
There was a problem hiding this comment.
The type: "navigate" hardcoding here means commitSameUrlPayload always uses merge semantics, even though the caller (server action handler) could conceptually want replace. This is correct for the current use case (server actions stay on the same URL and should merge), but the function name commitSameUrlPayload doesn't communicate that it's always-merge. If Phase 2+ adds a type parameter here, the function name would be fine — just noting the implicit contract.
| // server actions, HMR) always read the current state. Safe: those readers | ||
| // run from events/effects, never from React render itself. | ||
| const stateRef = useRef(treeState); | ||
| stateRef.current = treeState; |
There was a problem hiding this comment.
Agreeing with the author's dismissal of Copilot's suggestion here. The render-phase ref write is safe because the only external readers (navigateRsc, server action callbacks, HMR handler) run from event handlers and effects, never from React render. The useLayoutEffect on line 396 publishes browserRouterStateRef = stateRef once at mount — subsequent renders just update stateRef.current in place, which is a no-op from an identity perspective.
The one timing edge case (between hydrateRoot returning and the first useLayoutEffect firing) is safe because __VINEXT_RSC_NAVIGATE__ is assigned after hydrateRoot() returns, and the layout effect fires synchronously during the hydration commit phase. Worth a one-line comment noting this:
| stateRef.current = treeState; | |
| // Keep the latest router state in a ref so external callers (navigate(), | |
| // server actions, HMR) always read the current state. Safe: those readers | |
| // run from events/effects, never from React render itself. | |
| // Note: stateRef.current is written during render, not in an effect, to | |
| // avoid a stale-read window. This is safe per the above constraint. | |
| const stateRef = useRef(treeState); | |
| stateRef.current = treeState; |
| const slotErrorComponent = getErrorBoundaryExport(slot.error); | ||
| if (slotErrorComponent) { | ||
| slotElement = <ErrorBoundary fallback={slotErrorComponent}>{slotElement}</ErrorBoundary>; | ||
| for (let index = orderedTreePositions.length - 1; index >= 0; index--) { |
There was a problem hiding this comment.
Nice bottom-up construction. The orderedTreePositions reverse iteration correctly handles:
- Template-only positions (line 544-546: falls through to
routeChildren = segmentChildrenwhen nolayoutEntry) - Layout-only positions (no template wrapping)
- Positions with both (template wraps first, then layout slot)
This is cleaner than the old parallel null-padded arrays approach.
| routeChildren = segmentChildren; | ||
| continue; | ||
| } | ||
| const layoutHasElement = getDefaultExport(layoutEntry.layoutModule) !== null; |
There was a problem hiding this comment.
Minor: when a layout module has no default export (layoutHasElement === false), the code at line 569 skips the Slot wrapper entirely and just uses segmentChildren directly. This means the layout's error boundary and notFound boundary (from lines 510-526) are still applied, but the layout module's default component is not rendered — the boundary just wraps the raw children.
This is correct behavior (layouts without exports are passthrough wrappers for boundaries), but it's worth noting that if someone has a layout.tsx that only exports metadata (no default), their error boundary still applies. This matches Next.js behavior where error.tsx activates based on the directory level, not on whether a layout exists.
| rootLayoutTreePath: string | null; | ||
| }; | ||
|
|
||
| export function normalizeAppElements(elements: AppWireElements): AppElements { |
There was a problem hiding this comment.
Good optimization: the fast path (no normalization needed) returns the original object without allocation. For the common case where there are no unmatched slots, this avoids the overhead of copying the entire elements map on every payload.
| const _noExportRouteId = "route:" + routePath; | ||
| let _noExportRootLayout = null; | ||
| if (route.layouts?.length > 0) { | ||
| const _tp = route.layoutTreePositions?.[0] ?? 0; |
There was a problem hiding this comment.
The buildPageElements no-default-export path (line 906-921) builds a minimal flat payload with __route, __rootLayout, and a single element. The __rootLayout computation replicates createAppPageLayoutEntries logic inline — computing tree position from layoutTreePositions[0] and slicing routeSegments.
This is acceptable for now since it's the error path (no page export), but if the tree path computation logic ever changes in createAppPageTreePath, this inline version would drift. Consider adding a comment noting the dependency:
| const _tp = route.layoutTreePositions?.[0] ?? 0; | |
| async function buildPageElements(route, params, routePath, opts, searchParams) { | |
| const PageComponent = route.page?.default; | |
| if (!PageComponent) { | |
| const _noExportRouteId = "route:" + routePath; | |
| // Compute rootLayout tree path inline — mirrors createAppPageTreePath() logic. | |
| // If that helper's algorithm changes, update this path too. | |
| let _noExportRootLayout = null; |
| const elements = React.use(React.useContext(ElementsContext)); | ||
| const elements = React.useContext(ElementsContext); | ||
|
|
||
| if (!Object.hasOwn(elements, id)) { |
There was a problem hiding this comment.
Key design point: returning null when the key is absent (not present in elements) is what enables layout persistence. During soft navigation, the server only emits entries for the segments that changed. Layouts that weren't re-emitted keep their existing element from the merged map (via mergeElements), and Slot returns null for any key that was never set — which in practice means "this slot's parent already has the right content from a previous render."
This is the correct approach. The only edge case to be aware of: if a layout entry is accidentally omitted from the server payload (bug in buildAppPageElements), the client silently renders nothing instead of erroring. Phase 2 entry eviction would need to be careful about this — evicting a layout entry that's still being rendered would cause it to disappear.
| } | ||
|
|
||
| void main(); | ||
| if (typeof document !== "undefined") { |
There was a problem hiding this comment.
Good guard. This enables importing the module in test/SSR contexts without triggering main(). The previous version would crash in any non-browser context.
| function discoverTemplates( | ||
| segments: string[], | ||
| appDir: string, | ||
| matcher: ValidFileMatcher, |
There was a problem hiding this comment.
Clean improvement. The old discoverLayoutAlignedTemplates was coupled to layout discovery — it only checked for template.tsx at levels that had layout.tsx, producing null-padded arrays. The new discoverTemplates independently discovers all templates and uses templateTreePositions for alignment.
This correctly handles the edge case where a template exists without a layout at the same directory level — Next.js supports this (template.tsx re-mounts children on navigation regardless of layout presence). The old code would miss these templates entirely.
| @@ -2316,6 +2316,17 @@ async function _handleRequest(request, __reqCtx, _mwCtx) { | |||
| return LayoutComp({ params: _asyncLayoutParams, children: null }); | |||
There was a problem hiding this comment.
Good move. _probeSearchObj and _asyncSearchParams were being computed unconditionally at line 2260 (old code) even when they were only used inside probePage(). Moving them inside the closure is both cleaner and avoids unnecessary work when probePage() is never called (e.g., the page is served from ISR cache).
|
Review posted successfully. Here's a summary of what I reviewed and commented on: Review summary for PR #750 (flat keyed payload for App Router layout persistence): The PR is a well-executed architectural shift. I verified and cleared the prior automated review concerns (redirect header ordering is preserved, basePath/public file checks intact). The core design is sound: flat 11 inline comments posted:
|
|
/bigbonk review |
Review: flat keyed payload for App Router layout persistenceThis is a well-architected change. The flat I verified the concerns raised in prior automated reviews:
Findings1. Duplication in Both functions follow the same flow: Not blocking — but this is the kind of near-duplicate where a future fix to one path (e.g., the Phase 2 commit-version gate from the TODO at line 336) could miss the other. Worth extracting a shared 2. Render-phase ref write comment ( Minor nit. The 3. HMR path skips disposition check ( The HMR handler always dispatches 4. Render dependency barrier has no timeout ( If a layout suspends indefinitely (stuck fetch), 5. Good correctness fix. Params are now staged inside Positive observations
SummaryNo blocking issues. The architecture is sound, the three rendering paths (server/SSR/browser) are properly symmetric, and the test coverage is good. The items above are non-blocking observations for documentation and Phase 2 cleanup. |
| } | ||
|
|
||
| return undefined; | ||
| } |
There was a problem hiding this comment.
Duplication worth tracking for Phase 2.
commitSameUrlNavigatePayload and renderNavigationPayload both follow the same commit lifecycle: create pending commit → check disposition → hard-navigate/skip/dispatch. The key structural difference is that this function bundles disposition into createPendingSameUrlCommit while renderNavigationPayload (line 461) calls createPendingNavigationCommit and resolvePendingNavigationCommitDisposition separately.
The TODO at line 336 about a "stronger commit-version gate" would need to be applied in both places. Consider extracting a shared resolveAndClassifyCommit helper that both functions delegate to, keeping only path-specific logic (returnValue handling here, prePaintEffect+snapshot activation in renderNavigationPayload) in each caller.
There was a problem hiding this comment.
Not taking that extraction in PR 2c. The remaining overlap is intentional at this stage: renderNavigationPayload() owns pre-paint effects and snapshot activation, while commitSameUrlNavigatePayload() is the same-URL server-action path with no snapshot activation and return-value handling. If we add a stronger commit-version gate in Phase 2, that is the right time to factor the shared classification seam further rather than widening this refactor again now.
| ): () => void { | ||
| return () => { | ||
| const targetHref = new URL(href, window.location.origin).href; | ||
| stageClientParams(params); |
There was a problem hiding this comment.
Good correctness fix. Moving stageClientParams into the commit effect means params are only staged when the navigation actually commits during React's paint phase. This eliminates the previous race where params were staged before confirming the navigation was not superseded, and removes the ordering dance with _snapshotPending that the old code needed.
| // used inside the RSC render graph, where producing this leaf means the | ||
| // owning entry has reached the serialization point that downstream entries | ||
| // are allowed to observe. | ||
| dependency.release(); |
There was a problem hiding this comment.
Forward-looking note for Phase 2+: if a layout suspends indefinitely (e.g., stuck fetch), ReleaseAppRenderDependency never renders, and all downstream AwaitAppRenderDependencies components hang forever. The RSC stream stays open with no timeout.
This matches Next.js behavior (no server component render timeout). But when you add AbortSignal-based render timeouts, this barrier would need a reject() path — e.g., dependency.abort(reason) triggered by the signal, with AwaitAppRenderDependencies catching the rejection and propagating the error to its subtree.
| const elements = React.use(React.useContext(ElementsContext)); | ||
| const elements = React.useContext(ElementsContext); | ||
|
|
||
| if (!Object.hasOwn(elements, id)) { |
There was a problem hiding this comment.
Key design point for layout persistence. Returning null for absent keys means layouts not re-emitted by the server keep their element from the merged map via mergeElements.
Phase 2 entry eviction note: evicting a layout entry that an active Slot depends on would cause it to silently disappear (returns null instead of erroring). Consider adding a dev-mode warning when a key that should exist based on the route's expected entries is absent — this would distinguish "intentionally absent from soft nav payload" from "accidentally evicted."
|
All comments posted successfully. Here's a summary of the review: Review posted on PR #750Top-level comment: #750 (comment) 4 inline comments on key areas:
Overall assessment: No blocking issues. The architecture is sound, the three rendering paths (server/SSR/browser) are properly symmetric, and test coverage is thorough. |
|
/bigbonk review |
There was a problem hiding this comment.
Review: flat keyed payload for App Router layout persistence
This is a well-executed architectural change. The flat Record<string, ReactNode> payload design cleanly separates concerns: app-elements.ts for wire format/normalization, app-browser-state.ts for reducer/disposition logic, app-render-dependency.tsx for RSC serialization ordering, and the updated app-page-route-wiring.tsx for flat-map construction. The three-state slot semantics (absent → persisted, UNMATCHED_SLOT → notFound, present → render) are correct and match Next.js parallel route persistence behavior.
I verified the items flagged by prior automated reviews:
- Redirect header ordering: Confirmed safe.
__mergeMiddlewareResponseHeaders(line 1736) runs beforeredirectHeaders.set("x-action-redirect", ...)(lines 1737-1739). The framework's.set()calls are authoritative. - basePath rejection and public file signal: Both intact.
Overall the code is solid. I have a few observations — two items I think deserve attention for this PR, and several smaller notes.
Items worth addressing
- Inline tree-path computation in generated entry could drift (comment below on
app-rsc-entry.ts:916) commitSameUrlNavigatePayloadreturnsundefinedon skip — a navigation that was silently skipped while a server action was in flight returnsundefinedto the caller with no signal that the action's UI update was dropped (comment below)
Non-blocking observations
- The render-phase ref write pattern (
stateRef.current = treeState) is well-documented and correct. The timing guarantee betweenhydrateRootanduseLayoutEffectis sound. - The
normalizeAppElementsfast path (two-pass scan) is a good optimization for the common case. pickRouteParamsreplacingmatchSourceRouteParamsis a nice simplification — using already-matched params by name instead of re-parsing URL patterns.- The
typeof document !== "undefined"guard at the module tail enables clean test/SSR imports. - Test coverage for the new primitives is thorough. The deferred-resolution test and the async-template dependency barrier test are good additions.
Summary
No blocking issues. The architecture is sound, the server/SSR/browser rendering paths are symmetric, and the test coverage is solid for Phase 1.
| // mirrors the createAppPageTreePath() logic used by buildAppPageElements(). | ||
| const _tp = route.layoutTreePositions?.[0] ?? 0; | ||
| const _segs = route.routeSegments?.slice(0, _tp) ?? []; | ||
| _noExportRootLayout = _segs.length === 0 ? "/" : "/" + _segs.join("/"); |
There was a problem hiding this comment.
This inline tree-path computation duplicates createAppPageTreePath() logic. The comment acknowledges the dependency, but the duplication is in a generated template where it's easy to miss when the canonical helper changes.
The helper is already imported by the generated entry (via app-page-route-wiring.tsx). Could this branch call createAppPageTreePath() directly instead of reimplementing it? That would eliminate the drift risk entirely:
| _noExportRootLayout = _segs.length === 0 ? "/" : "/" + _segs.join("/"); | |
| // Compute the root layout tree path for this error payload using the | |
| // canonical helper to avoid drift with buildAppPageElements(). | |
| const _tp = route.layoutTreePositions?.[0] ?? 0; | |
| _noExportRootLayout = __createAppPageTreePath(route.routeSegments, _tp); |
This would require adding createAppPageTreePath to the import from app-page-route-wiring.tsx at the top of the generated entry. If there's a reason this needs to stay inline (e.g., avoiding import weight for this error path), the current approach is fine — just flagging the risk.
| return undefined; | ||
| } | ||
|
|
||
| if (disposition === "dispatch") { |
There was a problem hiding this comment.
When disposition === "skip", commitSameUrlNavigatePayload falls through to the returnValue handling below, which returns the server action's data as if the dispatch succeeded. This is arguably correct for the caller (they get their return value regardless), but it means the UI silently doesn't update even though the action completed successfully.
Is this intentional? The navigation path (renderNavigationPayload) returns early on "skip" without proceeding further. Here, the caller can't distinguish "dispatched" from "skipped" — if the action had side effects that the UI should reflect, the user won't see them until the next navigation.
Not blocking — the same gap existed before, and the returnValue semantics are correct. But a follow-up TODO noting that server actions should ideally retry the dispatch or signal the caller when skipped would be helpful.
| @@ -548,35 +672,21 @@ function registerServerActionCallback(): void { | |||
| // If server actions ever trigger URL changes via RSC payload (instead of hard | |||
| // redirects), this would need renderNavigationPayload() + snapshotActivated=true. | |||
| if (isServerActionResult(result)) { | |||
There was a problem hiding this comment.
Subtle: both branches (isServerActionResult(result) and else) wrap the result in Promise.resolve(normalizeAppElements(...)). The normalizeAppElements call is synchronous and doesn't throw for well-formed payloads, so Promise.resolve() just wraps an already-computed value.
But if the server returns a malformed payload (e.g., missing __route), readAppElementsMetadata inside createPendingNavigationCommit will throw from an awaited microtask, which means the error surfaces as an unhandled promise rejection rather than propagating to the caller. The try/catch in registerServerActionCallback's caller would need to handle this.
This is fine for now — malformed payloads are a server bug, not a user error. Just noting the error propagation path is slightly different from the old synchronous updateBrowserTree approach.
| const layoutIndex = layoutIndicesByTreePosition.get(treePosition); | ||
| if (layoutIndex !== undefined) { | ||
| const layoutEntry = layoutEntries[layoutIndex]; | ||
| layoutDependenciesBefore[layoutIndex] = [...pageDependencies]; |
There was a problem hiding this comment.
Good: layoutDependenciesBefore[layoutIndex] = [...pageDependencies] snapshots the accumulated dependencies at this point, and pageDependencies.push(layoutDependency) adds the current layout's dependency after the snapshot. This means a layout's own entry only waits for prior layouts/templates, not itself. Clean accumulator pattern.
|
|
||
| elements[APP_ROUTE_KEY] = routeId; | ||
| elements[APP_ROOT_LAYOUT_KEY] = rootLayoutTreePath; | ||
| elements[pageId] = renderAfterAppDependencies(options.element, pageDependencies); |
There was a problem hiding this comment.
The page entry's dependency list includes all layout and template dependencies accumulated in pageDependencies. This means if any layout or template is async (e.g., fetches data), the page waits for all of them before serializing.
This is correct for the common case where layouts set request-scoped state (e.g., locale) that pages need to read. But it's worth noting that this serializes the page after all layouts/templates, not just the ones in its direct ancestor chain. For routes with many parallel slots each having their own templates, this could add unnecessary waiting.
Not an issue for Phase 1 (the map is small), but Phase 2 could refine this by tracking per-slot dependency chains separately.
|
|
||
| if (!Object.hasOwn(elements, id)) { | ||
| if (process.env.NODE_ENV !== "production" && !id.startsWith("slot:")) { | ||
| if (!warnedMissingEntryIds.has(id)) { |
There was a problem hiding this comment.
The warnedMissingEntryIds Set is module-scoped, so it persists across navigations. This means if a missing entry triggers a warning once, subsequent navigations to the same route won't warn again even if the underlying bug persists.
For dev-mode debugging, this is acceptable (first occurrence is usually enough). Just noting that if someone clears and re-navigates expecting to see the warning again, they won't.
| ): boolean { | ||
| // `null` means the payload could not identify an enclosing root layout | ||
| // boundary. Treat that as soft-navigation compatible so fallback payloads | ||
| // do not force a hard reload purely because metadata is absent. |
There was a problem hiding this comment.
Good defensive design. Treating null root layout paths as soft-navigation compatible means boundary/error payloads (which may not have full layout metadata) don't force unnecessary hard reloads. This is the right default — hard navigation should only trigger when we have positive evidence of a layout boundary change, not when metadata is absent.
| flightRoot = createFromReadableStream<AppWireElements>(ssrStream); | ||
| } | ||
| return flightRoot as unknown as ReactNode; | ||
| const wireElements = use(flightRoot); |
There was a problem hiding this comment.
Nice: the SSR path follows the identical deserialization pipeline as the browser entry — createFromReadableStream<AppWireElements> → normalizeAppElements → readAppElementsMetadata → ElementsContext.Provider + Slot. This symmetry is important for hydration mismatch avoidance.
|
Review posted successfully. Here's a summary of what I submitted: Review for PR #750 (flat keyed payload for App Router layout persistence): Overall assessment: No blocking issues. The architecture is sound — clean separation of concerns across Verified prior review concerns: Redirect header ordering is safe (merge before 8 inline comments posted:
|
|
/bigbonk review |
There was a problem hiding this comment.
Review: flat keyed payload for App Router layout persistence
This is a well-designed architectural change. The flat Record<string, ReactNode> payload with merge/replace semantics is a clean foundation for layout persistence. The separation of concerns across the new modules (app-elements.ts, app-browser-state.ts, app-render-dependency.tsx) is solid, and the server/SSR/browser deserialization pipeline is symmetric — both paths go through normalizeAppElements → readAppElementsMetadata → ElementsContext.Provider + Slot.
I verified the prior automated review concerns:
- Redirect header ordering: Confirmed safe at lines 1736-1739 in
app-rsc-entry.ts—__mergeMiddlewareResponseHeadersruns beforeredirectHeaders.set(). - basePath rejection and public file signal: Both intact.
The test coverage is thorough for the new state primitives, normalization, dependency barriers, and flat-map construction. The integration test at app-router.test.ts:131 verifying the wire format contract is a good addition.
I have two items worth attention (one should be addressed in this PR, one is a Phase 2 note), plus a few minor observations.
Summary
No blocking issues. The architecture is sound, the three rendering paths are properly symmetric, and the test coverage is solid.
| false, | ||
| ); | ||
| return result; | ||
| return commitSameUrlNavigatePayload(Promise.resolve(normalizeAppElements(result))); |
There was a problem hiding this comment.
Stale comment block (lines 670-677) references removed symbols.
The comment mentions updateBrowserTree, snapshotActivated, and handleAsyncError — none of which exist in the new code. The function now delegates to commitSameUrlNavigatePayload, which handles disposition classification internally. The comment should be updated to describe the current flow.
| return commitSameUrlNavigatePayload(Promise.resolve(normalizeAppElements(result))); | |
| // Server actions stay on the same URL and use commitSameUrlNavigatePayload() | |
| // for merge-based dispatch. This path does NOT call activateNavigationSnapshot() | |
| // because there is no URL change to commit — hooks use useSyncExternalStore | |
| // values directly. If server actions ever trigger URL changes via RSC payload | |
| // (instead of hard redirects), this would need renderNavigationPayload(). |
| action: { | ||
| elements, | ||
| navigationSnapshot: options.navigationSnapshot, | ||
| renderId: options.renderId ?? options.currentState.renderId + 1, |
There was a problem hiding this comment.
Minor: when renderId is not provided, the fallback currentState.renderId + 1 could produce duplicate renderIds if called multiple times without updating state between calls (e.g., two rapid server actions both reading the same currentState). In practice this doesn't happen because commitSameUrlNavigatePayload increments nextNavigationRenderId and passes it explicitly. But the fallback suggests renderId is truly optional when it's effectively required for correctness.
Consider making renderId required in the type signature to make this invariant explicit — it's always passed by callers.
| return undefined; | ||
| } | ||
|
|
||
| if (disposition === "dispatch") { |
There was a problem hiding this comment.
When disposition === "skip", execution falls through to the returnValue handling below. This means the caller gets their return value even though the UI update was silently dropped. The navigation path (renderNavigationPayload at line 498) returns early on "skip" without proceeding.
This asymmetry is probably intentional — server action return values shouldn't be lost just because a navigation superseded the UI update. But it's worth a one-line comment here making it explicit that the fall-through is deliberate:
| if (disposition === "dispatch") { | |
| if (disposition === "dispatch") { | |
| dispatchBrowserTree( |
Actually the fall-through is fine as-is since the comment at line 358-361 already explains this. Disregard — the existing comment covers this well.
| "replace", | ||
| pending.routeId, | ||
| pending.rootLayoutTreePath, | ||
| false, |
There was a problem hiding this comment.
The HMR path uses createPendingNavigationCommit but doesn't call resolvePendingNavigationCommitDisposition — it always does a full replace dispatch. If a route group restructuring during HMR changes rootLayoutTreePath, this would update state without triggering a hard navigation. The next soft navigation could then compare against a stale rootLayoutTreePath.
Not blocking — dev-only, and the author acknowledged this in a prior review. Just confirming this is a known gap.
| if (cached) { | ||
| return cached; | ||
| export function mergeElements(prev: AppElements, next: AppElements): AppElements { | ||
| const merged: Record<string, AppElementValue> = { ...prev, ...next }; |
There was a problem hiding this comment.
Design note: mergeElements creates a new object on every soft navigation (spread + iteration). For the current map sizes (~10-50 entries) this is fine. If Phase 2+ entry eviction significantly grows the map, consider tracking whether any key actually changed and short-circuiting — but that's premature optimization at this point.
| // owning entry has reached the serialization point that downstream entries | ||
| // are allowed to observe. If Phase 2 adds AbortSignal-based render | ||
| // timeouts, this dependency will also need an abort/reject path so stuck | ||
| // async layouts do not suspend downstream entries forever. |
There was a problem hiding this comment.
Good comment about the Phase 2 AbortSignal-based timeout. One additional note: release() is idempotent (the released guard), but there's no reject() path. When the AbortSignal work lands, AwaitAppRenderDependencies will need to handle promise rejection from aborted dependencies — the current Promise.all(dependencies.map(d => d.promise)) would need to be wrapped in error handling or the dependency promise would need a rejection path that the AwaitAppRenderDependencies component can catch and propagate as an error boundary trigger.
|
|
||
| elements[APP_ROUTE_KEY] = routeId; | ||
| elements[APP_ROOT_LAYOUT_KEY] = rootLayoutTreePath; | ||
| elements[pageId] = renderAfterAppDependencies(options.element, pageDependencies); |
There was a problem hiding this comment.
The page entry's dependency list includes all accumulated dependencies from every layout and template, not just direct ancestors. For routes with parallel slots that each have their own templates (e.g., a dashboard with @analytics and @team slots, each with templates), the page would wait for all slot templates even though it doesn't depend on their state.
This is correct for Phase 1 where the map is small and the pattern matches Next.js behavior (layouts set request-scoped context that pages need). But it's worth noting as a Phase 2 optimization opportunity — per-slot dependency chains could reduce unnecessary waiting for deeply parallel route structures.
| } | ||
| } | ||
|
|
||
| return layoutModules.length > 0 ? "/" : null; |
There was a problem hiding this comment.
Good defensive fallback. When route.layouts is empty/absent (e.g., a global error boundary response), returning null for rootLayoutTreePath ensures the client treats it as soft-navigation compatible rather than forcing a hard reload. This aligns with shouldHardNavigate treating null as compatible.
| } | ||
|
|
||
| const rootLayoutTreePath = elements[APP_ROOT_LAYOUT_KEY]; | ||
| if (rootLayoutTreePath === undefined) { |
There was a problem hiding this comment.
Good: the three-way error reporting now distinguishes:
- Missing
__route(line 44-46) - Missing
__rootLayoutkey (line 49-51) - Invalid
__rootLayouttype (line 52-53)
This addresses the feedback from earlier reviews about the misleading error messages.
| rootNotFoundModule: null, | ||
| }); | ||
|
|
||
| const body = await renderHtml( |
There was a problem hiding this comment.
This test renders the elements as siblings in a Fragment rather than through the ElementsContext + Slot rendering path that the actual runtime uses. The test at line 259 (renderRouteEntry) does exercise the real Slot path.
For this specific test (template-only segments with async dependency barriers), the Fragment approach is fine because it directly verifies the serialization ordering guarantee — if page:de appears in the output, the dependency barrier worked regardless of rendering path. Just noting the distinction for future test authors.
|
Review posted successfully. Here's a summary of what I submitted: Review posted on PR #750Overall assessment: No blocking issues. The architecture is sound. 10 inline comments covering:
|
|
/bigbonk review |
There was a problem hiding this comment.
Review: flat keyed payload for App Router layout persistence
This is a well-designed architectural shift. I read through every changed source file and test. The core design — flat Record<string, ReactNode> with wire/runtime sentinel separation, merge semantics on soft navigation, replace on HMR, render dependency DAG for serialization ordering — is clean and correct.
Verified prior review concerns
- Redirect header ordering: Confirmed safe. Lines 1737-1740 in
app-rsc-entry.tsshow__mergeMiddlewareResponseHeadersruns first, thenredirectHeaders.set("x-action-redirect", ...)runs after. Framework values are authoritative. - basePath rejection: Intact at lines 1261-1269.
- Public file static signal: Intact.
Architecture observations
The good:
-
Clean module decomposition. The extraction into
app-elements.ts(wire format + normalization),app-browser-state.ts(reducer + disposition classification),app-render-dependency.tsx(serialization DAG), and the updatedapp-page-route-wiring.tsx(flat-map builder) is solid separation of concerns. Each module is independently testable. -
Server/SSR/Browser symmetry. All three paths follow the identical deserialization pipeline:
createFromReadableStream<AppWireElements>→normalizeAppElements→readAppElementsMetadata→ElementsContext.Provider+Slot. This eliminates hydration mismatch risk. -
pickRouteParamsreplacingmatchSourceRouteParams. Using already-matched params by name instead of re-parsing URL patterns is simpler and more correct. The old approach had edge cases with pattern matching against request pathnames. -
Template discovery decoupling.
discoverTemplatesindependently discovers all templates and usestemplateTreePositionsfor alignment, correctly handling templates without layouts at the same directory level. The olddiscoverLayoutAlignedTemplateswould miss these. -
normalizeAppElementsfast path. Two-pass scan avoids allocation when no normalization is needed — good hot-path optimization for the common case. -
Three-state slot semantics. Absent key → persisted from previous soft nav,
UNMATCHED_SLOT→ notFound(), present null → valid render from default.tsx. This correctly models Next.js parallel route persistence behavior. -
Test coverage. Thorough for all new primitives. The render dependency barrier test (
app-render-dependency.test.ts) demonstrating the race condition without barriers is particularly good pedagogy.
Items for consideration
I have no blocking issues. Below are observations that could improve clarity or inform Phase 2 work.
See inline comments for details.
| false, | ||
| ); | ||
| return result; | ||
| return commitSameUrlNavigatePayload(Promise.resolve(normalizeAppElements(result))); |
There was a problem hiding this comment.
The comment block (lines 670-677) describes the current server action flow accurately. One thing I want to flag: this is one of two places in this file that creates a commitSameUrlNavigatePayload call — the other is line 677. The isServerActionResult branch extracts result.root, while the else branch passes result directly. Both normalize and wrap in Promise.resolve().
This is correct, but the near-identical structure across both branches is an invitation for a future fix to miss one. The author and ask-bonk already discussed this — deferring extraction to Phase 2 is fine. Just confirming I agree with the decision.
| currentState: AppRouterState; | ||
| nextElements: Promise<AppElements>; | ||
| navigationSnapshot: ClientNavigationRenderSnapshot; | ||
| renderId: number; |
There was a problem hiding this comment.
Minor type design point: renderId in createPendingNavigationCommit's options is required (good — prevents accidental duplicate IDs), and the test at app-browser-entry.test.ts:44 explicitly verifies this with @ts-expect-error. The routerReducer action type also requires renderId. This is consistent.
However, resolveAndClassifyNavigationCommit at line 114 also takes renderId — and its value flows through to createPendingNavigationCommit. This creates two places where callers must supply renderIds. Currently only commitSameUrlNavigatePayload calls resolveAndClassifyNavigationCommit, so there's no risk of confusion. But if Phase 2 adds more callers, consider whether the renderId should be generated inside the helper rather than passed in.
| const routeThenableParams = options.makeThenableParams(options.matchedParams); | ||
| const getEffectiveSlotParams = (slotName: string): Record<string, string | string[]> => | ||
| options.slotOverrides?.[slotName]?.params ?? options.matchedParams; | ||
| const rootLayoutTreePath = layoutEntries[0]?.treePath ?? null; |
There was a problem hiding this comment.
Good design: rootLayoutTreePath correctly includes route group segments because createAppPageTreePath slices routeSegments (which include groups like (marketing)) up to the layout's tree position. This means navigating between /(marketing)/... and /(dashboard)/... produces different tree paths, triggering shouldHardNavigate() in the browser state. This is the correct behavior.
| const layoutIndex = layoutIndicesByTreePosition.get(treePosition); | ||
| if (layoutIndex !== undefined) { | ||
| const layoutEntry = layoutEntries[layoutIndex]; | ||
| layoutDependenciesBefore[layoutIndex] = [...pageDependencies]; |
There was a problem hiding this comment.
Clean accumulator pattern. layoutDependenciesBefore[layoutIndex] snapshots pageDependencies before adding the current layout's dependency, then pageDependencies.push(layoutDependency) adds it after. This ensures each layout waits for prior layouts/templates but not itself. The same approach at line 374 for templates is correct.
One thing to note for Phase 2: pageDependencies accumulates all layout and template dependencies linearly. For routes with deeply parallel slot structures, the page ends up waiting for slots' templates that it doesn't directly depend on. Per-slot dependency chains could reduce unnecessary serialization blocking.
| // Build the segment map for this layout level. The "children" key always | ||
| // contains the route segments below this layout. Named parallel slots at | ||
| // this layout level add their own keys with per-slot segment data. | ||
| if (!layoutEntry) { |
There was a problem hiding this comment.
Correct: when layoutEntry is absent but templateEntry is present (template-only segment), the code at line 559 wraps segmentChildren in the template Slot, then line 575 assigns routeChildren = segmentChildren and continues. The layout boundaries (NotFoundBoundary, ErrorBoundary at lines 540-556) are gated on if (layoutEntry) so they're correctly skipped for template-only positions.
This matches Next.js behavior where template.tsx without layout.tsx still re-mounts children on navigation.
| return children; | ||
| } | ||
|
|
||
| function normalizeAppElementsPromise(payload: Promise<AppWireElements>): Promise<AppElements> { |
There was a problem hiding this comment.
Good documentation on the React Flight thenable quirk. createFromReadableStream() returns a Flight thenable whose .then() returns undefined (not a new Promise), so chaining without Promise.resolve() wrapping would produce undefined → use() crashes.
This is subtle enough that I'd suggest keeping the comment even if it seems redundant — future developers who see Promise.resolve(payload).then(...) and think "the Promise.resolve is unnecessary" would remove it and introduce a crash.
| let _noExportRootLayout = null; | ||
| if (route.layouts?.length > 0) { | ||
| // Compute the root layout tree path for this error payload using the | ||
| // canonical helper so it stays aligned with buildAppPageElements(). |
There was a problem hiding this comment.
Good: the no-default-export path now calls __createAppPageTreePath(route.routeSegments, _tp) instead of reimplementing the tree-path computation inline. This addresses the earlier review concern about drift risk between the canonical helper and inline logic. The comment at lines 916-918 documents the intent clearly.
| } | ||
|
|
||
| void main(); | ||
| if (typeof document !== "undefined") { |
There was a problem hiding this comment.
Clean guard. typeof document !== 'undefined' prevents main() from executing in test/SSR contexts. This is the correct pattern for a module that's imported by the test suite but should only auto-execute in a real browser.
| } | ||
|
|
||
| describe("app render dependency helpers", () => { | ||
| it("documents that React can render a sync sibling before an async sibling completes", async () => { |
There was a problem hiding this comment.
Excellent test design. The first test ("documents that React can render a sync sibling before an async sibling completes") demonstrates the problem — React renders LocalePage (sync) before LocaleLayout (async) has set activeLocale = "de", so the page sees "en". The second test shows that renderWithAppDependencyBarrier + renderAfterAppDependencies fixes this by deferring the page until the layout has rendered.
This is great pedagogy — anyone reading these tests immediately understands why the render dependency mechanism exists.
|
|
||
| it("resolves slot segmentMap from matched params when no override exists", () => { | ||
| const element = buildAppPageRouteElement({ | ||
| it("renders same-named slot props independently at different layout levels", async () => { |
There was a problem hiding this comment.
Good test for the slot collision fixture. This verifies that same-named slots (@modal) at different layout levels (/parent/@modal and /parent/child/@modal) are keyed by their stable slot id (including the owner path), not just the prop name. The slotKey distinction (modal@parent/@modal vs modal@parent/child/@modal) is what makes this work.
The companion test at line 365 ("does not apply ambiguous name-only slot overrides") correctly verifies that intercept overrides keyed only by "modal" are rejected when multiple same-named slots exist.
|
Review posted successfully on PR #750. Here's a summary: Overall assessment: No blocking issues. The architecture is sound. The PR switches from a monolithic
I verified the prior automated review concerns (redirect header ordering, basePath rejection, public file signal) are all non-issues. Posted 14 inline comments covering architecture observations, correctness confirmations, and Phase 2 considerations on the key design points. |
Summary
Switches the App Router from a monolithic
ReactNodepayload to a flat keyedRecord<string, ReactNode>element map (#726, Phase 1 PR 2c). Layouts now persist across navigations — the server emits separate entries for each layout, template, page, and parallel slot, and the browser merges new entries into the existing map instead of replacing the entire tree.This is the core cutover. Includes PRs 2a (client primitives) and 2b (route wiring extraction) as base commits.
What changed
buildAppPageElements()replaces the inline monolithic tree builder. Produces flat entries keyed by tree path (includes route groups), plus__routeand__rootLayoutmetadata. All RSC render paths switched: normal routes, ISR, error/not-found/forbidden fallbacks, server actionsuseReducer-based router state with atomicelements+routeId+rootLayoutTreePath. Navigation/refresh/server actions/back-forward use merge semantics. HMR uses replace. Root layout switches trigger hard navigation (window.location.assign). URL state deferred until payload resolves (torn URL fix)VinextFlightRootreads flat payload, validates__route, renders throughElementsContext+Slot. GlobalErrorBoundary, ServerInsertedHTMLContext, font injection preserved at same positions"__VINEXT_UNMATCHED_SLOT__"normalized toUNMATCHED_SLOTSymbol after deserialization. Three states: absent key = persisted from soft nav, Symbol = unmatched (notFound), present null = valid render from default.tsxWhat this does NOT do
Skip-header optimization, entry eviction/LRU, interception-context encoding, incremental per-entry streaming. Those are Phases 2-4.
Test plan
tests/slot.test.ts— wire marker normalization, three-state distinctiontests/app-elements.test.ts— normalize,__routeinvariant,__rootLayoutvalidationtests/app-browser-entry.test.ts— reducer merge/replace, root-layout hard nav, torn URL, refresh/back-forward/server-action mergetests/app-page-route-wiring.test.ts— flat-map entries, tree-path IDs, per-slot segment providers, template keystests/error-boundary.test.ts— pathname reset preservedtests/entry-templates.test.ts— snapshots updated for flat payloadtests/app-router.test.ts— integration test for flat payload contractvp check— 0 lint/type errors