test(app-router): lock navigation lifecycle settlement and root-layout hard navigation behaviour#1053
Conversation
…t hard navigation behaviour Adds 13 unit tests covering two Wave 0 baseline areas from issue cloudflare#726: BASE-02 — Lock current lifecycle settlement behaviour (9 tests): - Three concurrent navigations with reverse-order payload resolution - Rapid sequential navigation: stale deferred payload does not overwrite - Stale cross-root navigation is skipped instead of hard-navigating - resolvePendingNavigationCommitDisposition exhausts skip/dispatch/hard-navigate - resolveAndClassifyNavigationCommit classifies skip when IDs diverge - Failed RSC payload settles pending router state cleanly BASE-03 — Lock root-layout hard navigation behaviour (4 tests): - renderNavigationPayload calls window.location.assign on root change - Hard-navigate settles the pending router state before navigating away - shouldHardNavigate negative/null path exhaustiveness - shouldHardNavigate positive path with distinct roots These tests close the App Router concurrency gap: the Pages Router has 10 race-condition tests in shims.test.ts but the App Router (which uses a completely different lifecycle mechanism) had zero equivalent tests for out-of-order payload resolution and stale commit rejection.
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.
Thorough, well-structured test suite. All 58 tests pass (including the 13 new ones). The tests lock real concurrency semantics that had zero unit-level coverage on the App Router side, and the stale-cross-root-skip test in particular covers a subtle priority inversion (skip must win over hard-navigate for superseded navigations) that would be very hard to catch in E2E alone.
A few minor observations below — none blocking.
| }); | ||
| }); | ||
|
|
||
| // ─── BASE-02: Lock current lifecycle settlement behaviour ───────────────────── |
There was a problem hiding this comment.
Nit: The action-queue.ts link is a useful directional pointer, but the lifecycle settlement logic being tested here (resolvePendingNavigationCommitDisposition, the activeNavigationId counter pattern) is vinext's own mechanism, not a port of Next.js's action-queue.ts. Next.js uses a linked-list action queue with a different staleness model. Might be worth adjusting the comment to say "Inspired by" or "Analogous to" rather than "Ported from" to avoid implying the implementations match 1:1.
Same applies to the BASE-03 header at line 1000 referencing compare-states.ts — shouldHardNavigate is vinext's own function with its own null-safety semantics.
| }), | ||
| ); | ||
|
|
||
| const effectsRun: string[] = []; |
There was a problem hiding this comment.
Nice touch using the effectsRun array to verify that only C's createNavigationCommitEffect was invoked. This cleanly distinguishes "payload resolved but was classified as skip" from "payload resolved and commit effect was queued" — which is the exact boundary that matters for the Layer 1 migration.
| expect(result.pending.routeId).toBe("route:/dashboard"); | ||
| }); | ||
|
|
||
| it("failed payload cleanly settles the pending router state without leaving it hanging", async () => { |
There was a problem hiding this comment.
Minor: this test spies on clearPendingPathname but the assertion is only on pendingRouterState.settled and the rejection. The spy is necessary to prevent the real clearPendingPathname from throwing in a non-browser environment, but a brief comment explaining that (like the existing tests at lines 373, 395 do implicitly) would help future readers understand it's a guard, not an assertion target.
| } | ||
| }); | ||
|
|
||
| it("shouldHardNavigate returns false when either root-layout identity is null", () => { |
There was a problem hiding this comment.
Worth noting for future work: this tests shouldHardNavigate in isolation, but there's an asymmetry worth documenting — resolvePendingNavigationCommitDisposition delegates to shouldHardNavigate only when startedNavigationId === activeNavigationId. So the null-safety valve is only reachable for current navigations. For stale navigations, the function returns "skip" before ever checking roots. The BASE-02 "stale cross-root" integration test at line 840 already covers this priority, but a brief note in this test's comment about the layering would connect the two.
|
Review submitted. Here's a summary of what I found: Verdict: Approved All 58 tests pass (13 new + 45 existing). The new tests fill a real coverage gap — the App Router had zero unit-level concurrency tests despite the Pages Router having 10 proven race-condition tests in Highlights:
Minor observations (non-blocking):
|
|
|
||
| // ─── BASE-02: Lock current lifecycle settlement behaviour ───────────────────── | ||
| // Ported from Next.js navigation lifecycle semantics: | ||
| // https://github.com/vercel/next.js/blob/canary/packages/next/src/shared/lib/router/action-queue.ts |
There was a problem hiding this comment.
this doesn't exist?
are these tests not covered by existing e2es and fixtures?
There was a problem hiding this comment.
It probably hallucinated due to the instruction I put in for bug fixings. But the tests are good! The current e2e tests the happy path but don't test these controller contracts. I'll delete those comments
|
@codex Please review this PR critically and let me know if these new tests are necessary or redundant |
![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: b7a516eb60
ℹ️ 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".
| expect(shouldHardNavigate(null, null)).toBe(false); | ||
| expect(shouldHardNavigate(null, "/")).toBe(false); | ||
| expect(shouldHardNavigate("/", null)).toBe(false); |
There was a problem hiding this comment.
This test case repeats the exact shouldHardNavigate(null, …) expectations that are already covered by treats null root-layout identities as soft-navigation compatible later in the same file, so it doesn't add behavioral coverage and increases maintenance noise when the helper contract changes. Keeping one canonical test for this null-root safety valve is enough.
Useful? React with 👍 / 👎.
Summary
Adds 6 unit tests locking two Wave 0 baseline areas from the #726 router kernel architecture — in preparation for the Layer 1 lifecycle spine migration.
Lifecycle settlement (4 tests)
The App Router uses a completely different lifecycle mechanism from the Pages Router (
activeNavigationIdcounter + promise-based commit gate vsAbortController-based cancellation). The Pages Router has 10 proven race-condition tests inshims.test.ts. The App Router had zero equivalent unit tests for these controller-level concurrency semantics.window.location.assignresolveAndClassifyNavigationCommitskip classificationRoot-layout hard navigation (2 tests)
renderNavigationPayloadcallswindow.location.assignon cross-rootwindow.location.assignfires (asserted viaassign.mockImplementation)Why this matters for #726
The architecture's Implementation Plan says Layer 0 is "freeze today's flat-wire behavior with compatibility tests." These tests are the lifecycle-settlement and root-layout halves of that layer. Without them, the "delete the old writer" steps in Layers 4–5 have no regression safety net for the concurrency semantics that are hardest to verify by code review alone.
Design decisions
setTimeout, novi.advanceTimersByTime. Zero flake surface.renderNavigationPayloadreturns. The returned promise resolves only whenNavigationCommitSignalmounts in a React tree, which never happens in unit tests. Tests verify state throughstateRef.currentafter yielding microticks — same pattern as the existing "uses render ids independent from navigation ids" test.stateRef.current.routeId(user-visible state),window.location.assigncalls (browser effect), andcreateNavigationCommitEffectinvocation (side-effect gating). They don't assert on internal Map sizes or reducer shape.createControllerHarness,createState,createResolvedElements,stubWindowhelpers. Samevi.spyOn/mockRestore/detach()cleanup pattern. Sits in the same describe/file as the existing controller tests.