test: E2E verification for layout persistence flat payload pipeline#752
Conversation
commit: |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
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):
|
NathanDrake2406
force-pushed
the
feat/layout-persistence-pr-3-verification
branch
from
efebde9 to
490a5b2
Compare
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.
NathanDrake2406
force-pushed
the
feat/layout-persistence-pr-3-verification
branch
from
ceca050 to
7a73a1d
Compare
|
/bigbonk review |
There was a problem hiding this comment.
Review
This is a well-structured test PR. The 8 E2E tests cover genuinely user-observable behaviors (layout persistence, template remount semantics, error recovery, history navigation, parallel slot behavior), and the fixture additions are minimal and purposeful. All CI checks pass.
Correction on the prior review: The previous review claimed this PR modifies 11 production source files — that is incorrect. This PR changes exactly 8 files, all under tests/. The production code comments in that review (about app-browser-state.ts, app-page-route-wiring.tsx, app-render-dependency.tsx, etc.) are about files that do not appear in this diff. Those may be from a related PR (#726 / PR 2c) but are not part of this one.
A few observations below — nothing blocking.
| expect(after).toBeGreaterThan(before); | ||
| }).toPass({ timeout: 10_000 }); | ||
|
|
||
| return readCounterValue(page, controls); |
There was a problem hiding this comment.
This is a good hydration-gating pattern — polling click → read → assert inside .toPass() elegantly handles the timing gap between SSR render and React hydration.
One thing to be aware of: each .toPass() retry clicks the button, so if hydration completes mid-retry-loop, the counter could have been incremented multiple times. The tests handle this correctly by capturing the return value and using relative offsets (initialCount + 1, initialCount + 2) rather than assuming initialCount === 0. Nice.
| await expect(page.locator('[data-testid="team-slot"]')).not.toBeVisible(); | ||
| await expect(page.locator('[data-testid="analytics-slot"]')).not.toBeVisible(); |
There was a problem hiding this comment.
Minor inconsistency: in the soft-nav test (lines 274-275), you use not.toBeAttached() to assert the default slots are absent from the DOM. Here you use not.toBeVisible() to assert the page-specific slot content is absent after a hard load.
On a hard load to /dashboard/settings, the server renders default.tsx for these slots — the page-specific slot content (team-slot, analytics-slot) shouldn't be in the DOM at all, not just hidden. not.toBeAttached() would be the stronger assertion and consistent with the pattern used elsewhere in this file.
| await expect(page.locator('[data-testid="team-slot"]')).not.toBeVisible(); | |
| await expect(page.locator('[data-testid="analytics-slot"]')).not.toBeVisible(); | |
| await expect(page.locator('[data-testid="team-slot"]')).not.toBeAttached(); | |
| await expect(page.locator('[data-testid="analytics-slot"]')).not.toBeAttached(); |
| await expect(page.locator("#error-boundary")).toBeVisible({ timeout: 10_000 }); | ||
|
|
||
| // Client-navigate away to home via the link in the error boundary | ||
| await page.click('[data-testid="error-go-home"]'); |
There was a problem hiding this comment.
This test nicely complements the existing error-interactive.spec.ts "navigating away from error page works" test (line 95), which uses page.goto() (hard nav) to leave the error. Here you test the more interesting case: client-side nav away and back, proving the error boundary clears. Good coverage gap to fill.
| // --------------------------------------------------------------------------- | ||
|
|
||
| test.describe("Error recovery across navigation", () => { | ||
| test("navigating away from error and back clears the error", async ({ page }) => { |
There was a problem hiding this comment.
Consider whether this test should also suppress the Vite dev error overlay, similar to how error-interactive.spec.ts does with disableViteErrorOverlay(). When an error is thrown, Vite's dev overlay can appear and intercept pointer events, which could make the error-go-home link click flaky. The test passes in CI today, but the overlay behavior can be timing-dependent.
If you'd rather not add the helper dependency, at minimum you could add a brief comment explaining why it's not needed here (e.g., the Link click happens inside the error boundary which renders above the overlay, or the overlay doesn't appear for caught errors).
| Settings | ||
| </Link> | ||
| </nav> | ||
| <LayoutCounter /> |
There was a problem hiding this comment.
Good placement — putting LayoutCounter directly in the dashboard layout (not in a page) is exactly right for testing layout persistence. If it were in a page component, it would remount on every navigation regardless.
| import { test, expect } from "@playwright/test"; | ||
| import { waitForAppRouterHydration } from "../helpers"; | ||
|
|
||
| const BASE = "http://localhost:4174"; |
There was a problem hiding this comment.
Nit: other E2E specs in this directory also hardcode http://localhost:4174, so this is consistent. But since the Playwright config already sets baseURL for the app-router project, you could use relative URLs (just /dashboard instead of ${BASE}/dashboard) and Playwright will resolve them against the project's baseURL. That would make these tests resilient to port changes without code edits.
Not blocking since the existing convention is to use the hardcoded base.
|
Review posted successfully. Here's a summary of what I submitted: Overall assessment: This is a solid test-only PR. The 8 E2E tests are well-structured, testing user-observable behaviors rather than implementation details. All CI checks pass. The fixture additions are minimal and well-scoped. Key correction: The previous review by ask-bonk was hallucinated — it claimed 11 production source files were modified and posted 10 inline comments about files ( Inline comments posted (6):
|
|
/bigbonk re-review for issues. any blockers? |
There was a problem hiding this comment.
Review
This is a clean, well-structured test PR. The 8 E2E tests cover genuinely user-observable behaviors and the fixture additions are minimal and well-scoped. The PR description accurately reflects the scope — this is test-only code.
Two actionable items (one potential flakiness issue, one assertion inconsistency), plus a couple of minor observations.
| await page.getByTestId("trigger-error").click(); | ||
|
|
||
| // Error boundary should be visible | ||
| await expect(page.locator("#error-boundary")).toBeVisible({ timeout: 10_000 }); |
There was a problem hiding this comment.
This test doesn't suppress the Vite dev error overlay, unlike error-interactive.spec.ts which uses disableViteErrorOverlay() before triggering errors. When the error is thrown, Vite's dev overlay can appear and intercept pointer events on the error-go-home link at line 197.
The test passes in CI today, but this is a known source of flakiness in this repo — error-interactive.spec.ts added that helper specifically to solve flaky click failures. I'd recommend either:
- Importing and calling
disableViteErrorOverlay(page)before triggering the error, or - Adding a comment explaining why it's not needed here (e.g., the error boundary's Link renders above the overlay, or
.toPass()retries handle it)
Since the error-go-home click at line 197 is a bare page.click() with no retry wrapper, this seems like the more fragile path compared to error-interactive.spec.ts.
| await expect(page.locator('[data-testid="team-slot"]')).not.toBeVisible(); | ||
| await expect(page.locator('[data-testid="analytics-slot"]')).not.toBeVisible(); |
There was a problem hiding this comment.
Inconsistent assertion: the soft-nav test at lines 274-275 uses not.toBeAttached() (element not in DOM at all), but the hard-nav test here uses not.toBeVisible() (element may be in DOM but hidden).
On a hard load to /dashboard/settings, the server renders default.tsx for these slots — the page-specific team-slot / analytics-slot content shouldn't be in the DOM at all, making not.toBeAttached() the stronger and more correct assertion. It would also be consistent with the pattern 16 lines above.
| await expect(page.locator('[data-testid="team-slot"]')).not.toBeVisible(); | |
| await expect(page.locator('[data-testid="analytics-slot"]')).not.toBeVisible(); | |
| await expect(page.locator('[data-testid="team-slot"]')).not.toBeAttached(); | |
| await expect(page.locator('[data-testid="analytics-slot"]')).not.toBeAttached(); |
| await expect(async () => { | ||
| const before = await readCounterValue(page, controls); | ||
| await incrementButton.click(); | ||
| const after = await readCounterValue(page, controls); | ||
| expect(after).toBeGreaterThan(before); | ||
| }).toPass({ timeout: 10_000 }); | ||
|
|
||
| return readCounterValue(page, controls); |
There was a problem hiding this comment.
This is a well-designed hydration gate. Polling click → read → assert inside .toPass() handles the timing gap between SSR render and React hydration without hardcoded waits.
Worth noting: each retry iteration clicks the button, so if hydration completes mid-loop the counter will have been incremented multiple times. The callers handle this correctly by capturing the return value and using relative offsets (initialCount + 1, initialCount + 2) rather than assuming initialCount === 0. Good pattern.
| await expect(page.locator("#error-boundary")).toBeVisible({ timeout: 10_000 }); | ||
|
|
||
| // Client-navigate away to home via the link in the error boundary | ||
| await page.click('[data-testid="error-go-home"]'); |
There was a problem hiding this comment.
Nice complement to the existing error-interactive.spec.ts "navigating away from error page works" test (line 95 in that file), which uses page.goto() (hard nav) to leave the error page. This tests the more interesting case: client-side nav away and back, proving the error boundary resets. Good coverage gap to fill.
|
Review posted. Here's a summary of what I submitted: Overall: Clean, well-structured test-only PR. 8 E2E tests covering user-observable behaviors with minimal fixture additions. The PR description is accurate. Two actionable items:
Positive observations:
|
- Add disableViteErrorOverlay() before triggering error in recovery test to prevent Vite dev overlay intercepting pointer events on error-go-home link - Change not.toBeVisible() to not.toBeAttached() for hard-nav slot assertions (stronger: slot content is absent from DOM entirely, not just hidden)
|
/bigbonk re-review for issues. any blockers? |
There was a problem hiding this comment.
Clean, well-scoped test PR. The 8 E2E tests cover meaningful user-observable behaviors (layout persistence, template remount semantics, error recovery via client-nav, history navigation, parallel slot behavior). Fixture additions are minimal and purposeful. One minor suggestion below, otherwise LGTM.
| .catch(() => { | ||
| // best effort | ||
| }); | ||
| } |
There was a problem hiding this comment.
Nit: disableViteErrorOverlay is now copy-pasted in 3 spec files (error-interactive.spec.ts, form.spec.ts, and this one) with identical implementations. Consider extracting it into tests/e2e/helpers.ts alongside waitForAppRouterHydration. Not blocking — it's a pre-existing pattern — but this PR is a good opportunity to consolidate since it already imports from ../helpers.
| await expect(page.locator('[data-testid="analytics-default"]')).not.toBeAttached(); | ||
| }); |
There was a problem hiding this comment.
Agree with the prior review feedback here. On a hard load to /dashboard/settings, the server renders default.tsx for these slots — the page-specific slot content (team-slot, analytics-slot) should not be in the DOM at all. not.toBeAttached() is the stronger assertion and consistent with lines 274-275 above.
| await expect(page.locator('[data-testid="analytics-default"]')).not.toBeAttached(); | |
| }); | |
| await expect(page.locator('[data-testid="team-slot"]')).not.toBeAttached(); | |
| await expect(page.locator('[data-testid="analytics-slot"]')).not.toBeAttached(); |
|
Review submitted. Here's a summary: Approved with two minor comments:
The tests are well-structured, the fixture changes are minimal and correct (counter in layout not page, template at root level), and the |
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