fix(router): replace stale optimistic layouts across dynamic params#2609
Conversation
A detached optimistic shell can commit stale dynamic-layout output before the authoritative payload resolves. Preparing the latter from live router state then makes the shell appear current, so stale server props and BFCache identity survive a cross-param navigation. All payloads in one navigation must derive reuse identity from the same initiation state. Capture that state once and pass it through commit preparation, while live router state remains the authority for cancellation and commit approval. Add composition coverage for cross-param replacement, same-param preservation, and layout-owned slots, plus a deterministic browser regression for the prefetched-shell handoff.
commit: |
Performance benchmarksCompared 0 improved · 1 regressed · 5 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
NathanDrake2406
left a comment
NathanDrake2406
left a comment
There was a problem hiding this comment.
navigationInitiationState is a correctness input for lifecycle-spanning navigations, but it remains optional and silently falls back to getBrowserRouterState(). That leaves future callers able to recreate the same temporal bug while still compiling.
Please strengthen the API so initiation-anchored payload preparation requires this state. At minimum, make it required in the private renderNavigationPayload wrapper used by navigateRsc(). If the lower-level controller still needs current-state preparation for isolated operations, expose that as an explicit separate path rather than an implicit fallback.
Navigation payload callers could omit the initiation state and silently prepare from live router state. A future caller could therefore compile while reintroducing the optimistic-to-authoritative identity bug. Require the state at both browser-entry and controller boundaries and remove the live-state fallback. Isolated controller tests now choose current-state preparation through an explicitly named test helper.
|
Addressed in |
Navigation payload regressions advanced a fixed number of microtasks before reading router state. That coupled the tests to the controller's current async scheduling depth.\n\nExpose a one-shot visible-commit dispatch waiter from the controller harness and await that explicit boundary before assertions.
…gationCommit createPendingNavigationCommit's currentState param has no note on what it should be. Navigation callers now pass the frozen navigation-initiation state (per the previous two commits), while the HMR caller still passes live state, and nothing marks that split as deliberate. A future navigation call site that passes live state instead of the initiation state would silently reintroduce the stale cross-param reuse bug this branch fixes, with no type error to catch it. Document the invariant on the field so the split reads as intentional.
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
👏 thanks for all these improvements, I've been loving vinext |
Thanks bro! But it's all this gigachad -> @james-elicx |
…ream merge Upstream now owns both behaviors this branch previously patched: - createPendingNavigationCommit baselines on the navigation initiation state (cloudflare#2609), which is what lets an authoritative payload replace a stale cross-param optimistic layout. The payload-ready live re-read re-introduced exactly that staleness, so the currentStateTiming / getCurrentStateAfterElementsReady plumbing is removed. - Loading-shell prefetch responses intentionally stream generated metadata so rewrites can reuse it without a second request (cloudflare#2455, cloudflare#2318). Stripping resolvedMetadata / streamingMetadata from shell renders broke that protocol; the element-builder now matches upstream. Keeping shell payloads out of navigation consumption remains handled client-side via isPrefetchCacheEntryConsumableForNavigation. Tests asserting the old behaviors are removed or aligned with the upstream contract (authoritative commits declare navigationCommitKind).
3 participants
Overview
Closes #2597.
Why
A single navigation can render a detached optimistic shell before its authoritative RSC payload arrives. Both payloads must derive layout reuse from the same pre-navigation state. Previously, commit preparation sampled whichever router state was visible at that moment. Once the detached shell committed, the authoritative response could treat that shell as its reuse base and preserve stale dynamic-layout output from another param.
navigateRsc()captures the initiation state once, before redirects, prefetch consumption, or detached commitsThis follows the navigation architecture in #726: deterministic preparation receives explicit inputs, while the visible commit gate owns lifecycle authority. The trace work in #2598 helped isolate this boundary, but this fix does not depend on diagnostics.
What changed
The browser regression holds the B RSC response long enough to inspect the detached shell, then releases it and verifies that server props, pathname, params, client state, and BFCache identity all agree on B.
Validation
vp checkon all touched source, test, and fixture filesvp test run tests/app-browser-entry.test.ts(227 tests)PLAYWRIGHT_PROJECT=app-router vp exec playwright test tests/e2e/app-router/nextjs-compat/use-router-bfcache-id.spec.ts --workers=1(13 tests)vp run vinext#buildgit diff --checkRisk and compatibility
Non-goals
NavigationPlannerdecisions or scan caches during commit preparation.reuseCurrentBfcacheIdsfallback.References