test: E2E verification for layout persistence flat payload pipeline#752
test: E2E verification for layout persistence flat payload pipeline#752NathanDrake2406 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).
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 "/".
Prove the flat keyed map architecture works end-to-end: - Layout state persists across sibling navigation (counter survives) - Template remounts on segment boundary change, persists within segment - Error boundary clears on navigate-away-and-back - Back/forward preserves layout state through history - Parallel slots persist on soft nav, show default.tsx on hard nav Zero production code changes — test fixtures and Playwright specs only.
commit: |
…ence-pr-3-verification # Conflicts: # packages/vinext/src/entries/app-rsc-entry.ts # packages/vinext/src/server/app-page-route-wiring.tsx # packages/vinext/src/shims/slot.tsx # tests/__snapshots__/entry-templates.test.ts.snap # tests/app-page-route-wiring.test.ts # tests/error-boundary.test.ts # tests/slot.test.ts
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR description is misleading — this is not "pure test code"
The title and summary claim "Pure test code — zero production changes" but this PR modifies 11 production source files (3 entirely new) across the entire App Router rendering pipeline:
app-rsc-entry.ts— rewrites the generated entry to produce flat element maps instead of nested ReactNode treesapp-browser-entry.ts— replacesuseStatewithuseReducer, adds hard-navigation detection, changes how all navigation dispatches workapp-ssr-entry.ts— rewritesVinextFlightRootto consumeAppWireElementsviaElementsContextapp-page-route-wiring.tsx— complete rewrite from nested element tree to flat keyed mapslot.tsx— removesReact.use()for Promises, switches to synchronous context, rewrites merge logicapp-router.ts— addstemplateTreePositionsto route scanningapp-page-boundary-render.ts— integrates elements map into error/access-fallback renderingapp-page-render.ts— widens type signaturesapp-browser-state.ts(NEW) — router reducer, hard-navigate detection, pending commit helpersapp-elements.ts(NEW) — elements map types, normalization, metadata extractionapp-render-dependency.tsx(NEW) — render ordering mechanism via promise-based dependency gates
This is a fundamental architectural change — the shift from a single nested ReactNode tree to a flat keyed AppElements map for the RSC payload. That's a legitimate and valuable change, but the PR description should accurately reflect it. As-is, reviewers scanning the title would skip the production code review entirely.
Please update the PR title and description to accurately reflect the scope of production changes. If this was originally meant to be split from another PR (the summary mentions "PR 2c"), the production changes should be in their own PR or the description should clearly enumerate them.
Code-level feedback
The architectural direction (flat element map, reducer-based state, hard-navigate detection) looks sound. The E2E tests are well-written. Below are specific issues I found in the production code.
| }; | ||
| } | ||
|
|
||
| export async function applyAppRouterStateUpdate(options: { |
There was a problem hiding this comment.
applyAppRouterStateUpdate is exported but never called from production code — it's only used in tests/app-browser-entry.test.ts. If this is intended as a public helper for future use, that's fine, but consider adding a comment indicating it's part of the planned API. If it's test-only scaffolding, it shouldn't live in the production module.
Alternatively, inline the test logic directly — the test already imports createPendingNavigationCommit and shouldHardNavigate separately, which is all the function composes.
| const stateRef = useRef(treeState); | ||
| stateRef.current = treeState; | ||
| browserRouterStateRef = stateRef; |
There was a problem hiding this comment.
Assigning to stateRef.current during render (line 337) is fine — it's a ref update, not a side effect. But assigning to the module-level browserRouterStateRef (line 338) during render is a side effect that runs on every render, not just mount. Under React Strict Mode or concurrent features, this could run multiple times for the same render.
The comment says "not in an effect — so there is no stale window" but this trades correctness for timing. In Strict Mode dev, React intentionally double-invokes render to catch impure renders — this assignment will run twice per render, though it happens to be idempotent (same ref object).
Consider moving the module-level assignment into the useLayoutEffect alongside the dispatch assignment, or document why the Strict Mode double-execution is acceptable.
| segment.startsWith("[[...") && | ||
| segment.endsWith("]]") && | ||
| segment.length >= "[[...x]]".length | ||
| segment.length > "[[...x]]".length - 1 |
There was a problem hiding this comment.
This change from >= "[[...x]]".length to > "[[...x]]".length - 1 is functionally equivalent (>= 8 vs > 7) but harder to reason about. The original was clearer: "the segment must be at least as long as the shortest valid optional catch-all". The new form inverts the comparison direction and subtracts one, which reads like a bug fix but isn't.
If this was changed to fix a linting rule or formatter preference, a comment explaining why would help. Otherwise, prefer the original form:
| segment.length > "[[...x]]".length - 1 | |
| segment.length >= "[[...x]]".length |
| children: ReactNode, | ||
| dependency: AppRenderDependency, | ||
| ): ReactNode { | ||
| function ReleaseAppRenderDependency() { |
There was a problem hiding this comment.
ReleaseAppRenderDependency calls dependency.release() during render (not in an effect). This works for server-side rendering (RSC) where components render once, top-to-bottom. But if this component is ever rendered in a client environment (or under React Strict Mode), React could invoke it multiple times for the same render pass. The released guard in createAppRenderDependency prevents double-resolution, so it's safe in practice, but calling side effects during render is unconventional.
Worth a brief comment explaining this is intentional and RSC-only.
| pending.rootLayoutTreePath, | ||
| false, | ||
| ); | ||
| return result; |
There was a problem hiding this comment.
When the server action result is NOT a ServerActionResult (the else branch), this returns result — which is the raw AppWireElements object. The caller of the server action callback likely expects either undefined or a return value, not the entire wire-format elements map.
The isServerActionResult branch returns result.returnValue.data or undefined, which is correct. But this fallback branch leaks internal transport data back to the caller. Was this intentional? The previous code had the same pattern, but now result is typed as AppWireElements rather than ReactNode, making the leak more obvious.
| } | ||
| } | ||
|
|
||
| export function shouldHardNavigate( |
There was a problem hiding this comment.
When either currentRootLayoutTreePath or nextRootLayoutTreePath is null, this returns false (no hard navigate). This means:
- First page load (
current = null) → always soft navigate (correct) - Route with no root layout (
next = null) → always soft navigate
But what about navigating FROM a route with a root layout TO a route without one (current = "/", next = null)? That would be a root layout removal, which arguably should hard-navigate. And the reverse (current = null, next = "/") would be a root layout addition.
Is this intentional? If so, a comment explaining the null semantics would help. If not, this could be a subtle bug where layout-less routes silently soft-navigate when they shouldn't.
| export type AppElementValue = ReactNode | typeof UNMATCHED_SLOT | string | null; | ||
| export type AppWireElementValue = ReactNode | string | null; | ||
|
|
||
| export type AppElements = Readonly<Record<string, AppElementValue>>; |
There was a problem hiding this comment.
The Readonly<Record<string, ...>> type is a good start, but it allows any string key. Consider whether a branded/tagged type would prevent accidental misuse — e.g., passing a random Record<string, ReactNode> where AppElements is expected without the required __route and __rootLayout keys. The runtime validation in readAppElementsMetadata catches this, but a compile-time guard would be stronger.
Not blocking, just a suggestion for future hardening.
| } | ||
|
|
||
| void main(); | ||
| if (typeof document !== "undefined") { |
There was a problem hiding this comment.
Good addition — guarding main() behind typeof document !== "undefined" prevents execution in non-browser environments (e.g., when the module is imported for type-checking or testing). This is a nice improvement over the previous unconditional void main().
| @@ -0,0 +1,228 @@ | |||
| import { test, expect } from "@playwright/test"; | |||
There was a problem hiding this comment.
The E2E tests are well-structured and test genuinely user-observable behavior. The table in the PR description mapping behaviors to test methods is helpful. A few observations:
- The
waitForHydrationhelper using.toPass()with retry is a good pattern for flaky-resistant hydration checks. - The error recovery test (line 120-123) wraps
click+expectin.toPass()which handles the timing gap between click and error boundary rendering nicely. - Consider adding a test for the hard-navigate detection path — e.g., if route groups with different root layouts exist in the fixture, navigating between them should trigger a full page load (verifiable by checking
window.__VINEXT_HYDRATED_ATchanges).
| export function buildAppPageElements< | ||
| TModule extends AppPageModule, | ||
| TErrorModule extends AppPageErrorModule, | ||
| >(options: BuildAppPageElementsOptions<TModule, TErrorModule>): AppElements { | ||
| const elements: Record<string, ReactNode | string | null> = {}; |
There was a problem hiding this comment.
This function is ~300 lines long and manages multiple parallel data structures (6+ Maps, multiple dependency arrays). While the logic is correct, the complexity is high. Consider extracting the dependency-graph construction (lines 306-343) into a separate function — it's a self-contained concern and would make the main function easier to follow.
Not blocking, but this is the kind of function that's easy to introduce bugs into during future maintenance.
|
Review posted successfully. Here's a summary of my review: Requested changes on PR #752 with 10 inline comments covering: Primary concern: Misleading PR descriptionThe PR claims "Pure test code — zero production changes" but actually modifies 11 production source files (3 entirely new), implementing a fundamental architectural shift from a single nested Code-level feedback (10 inline comments):
|
2 participants
Summary
Part of #726 (PR 3). Pure test code — zero production changes. Proves the flat keyed map payload pipeline from PR 2c actually works end-to-end in a real browser.
Every test answers: "would a real user notice if this broke?" Layout state surviving navigation, template state resetting on segment change, errors clearing on re-navigation — these are observable user behaviors, not implementation details.
What's tested
page.goto()resets counter vs Link click preserving it/→/about(segment changes)/dashboard→/dashboard/settings(same top-level segment)goBack()/goForward()through dashboard history@teamand@analyticsslot content survives/dashboard→/dashboard/settings/dashboard/settingsrendersdefault.tsxinstead of page slot contentWhat's NOT tested here (and why)
shouldHardNavigateintests/app-browser-entry.test.tsFixture changes
Minimal additions to
tests/fixtures/app-basic/to make behaviors observable:app/components/layout-counter.tsx—"use client"counter proving layout persistenceapp/components/template-counter.tsx—"use client"counter proving template remountapp/dashboard/layout.tsx— added LayoutCounter + nav links between dashboard pagesapp/template.tsx— added TemplateCounterapp/error-test/error.tsx— added "Go home" Link for error recovery navigationapp/page.tsx— added "Error Test" LinkAll existing E2E tests (navigation, error-handling, navigation-flows — 25 tests) still pass with these fixture changes.
Test plan
tests/e2e/app-router/layout-persistence.spec.ts— 8 E2E tests, all passingtests/app-router.test.ts -t "not-found|forbidden|unauthorized"— 5 SSR integration tests, all passingvp check— 0 lint/type errors