feat: flat keyed payload for App Router layout persistence#750
feat: flat keyed payload for App Router layout persistence#750NathanDrake2406 wants to merge 22 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 👍 / 👎.
When a layout entry exists but has no default export, the element is never written to the flat map. The unconditional Slot wrapping would return null (id not in elements), silently dropping the entire route subtree below that layout level. Guard the Slot wrapping with a check for the layout component. When absent, pass layoutChildren through directly — preserving the LayoutSegmentProvider and error/not-found boundaries while skipping the Slot indirection.
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