fix(app-router): model RSC redirect and traversal lifecycle#1248
Conversation
commit: |
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ 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". |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: RSC redirect lifecycle and traversal intent model
Read #726 before reviewing. This is a well-structured slice — the redirect lifecycle extraction is clean, the traversal metadata model is deliberately minimal, and the PR maintains the discipline of not guessing when metadata is absent.
What works well
- Redirect lifecycle stays inside the initiating navigation. The previous pattern of writing history mid-redirect-chain before the final RSC payload was approved is the correct bug to fix. Deferring URL/history mutation to the approved commit effect is the right architectural choice.
resolveRscRedirectLifecycleHopis pure, testable, and correctly factored. Explicit decision types (no-redirect,follow,terminal-hard-navigation) with exhaustive handling in the caller. This matches the planner's decision-model spirit from #726.- Unknown traversal direction is kept explicit. Not synthesizing
back/forwardfrom stale index state is correct. The test atnavigation-planner.test.ts:668explicitly validates this. nextHistoryTraversalIndex/currentHistoryTraversalIndexsplit is sound.nextHistoryTraversalIndextracks the high-water mark so push always allocates forward, whilecurrentHistoryTraversalIndextracks what was last committed (including null for unknown traversal targets). The?? 0initializer correctly bootstraps from a metadata-less initial entry.- E2E coverage is good. The back-after-redirect test directly validates the core invariant this PR exists to enforce.
Issues
One concern with the redirect decision and two minor observations below.
| options.currentHref, | ||
| options.origin, | ||
| ); | ||
| if (redirectedHref === toVisibleAppHref(options.currentHref, options.origin)) { |
There was a problem hiding this comment.
Redirect detection compares visible hrefs, but resolveHardNavigationTargetFromRscResponse preserves the hash from currentHref (the request URL), not the response.
Looking at resolveHardNavigationTargetFromRscResponse (app-rsc-cache-busting.ts:124-126):
let hardNavigationTarget = pathname + parsed.search;
if (origUrl.hash) hardNavigationTarget += origUrl.hash;So redirectedHref will carry the original request's hash appended to the response's pathname+search. Then toVisibleAppHref(options.currentHref, ...) also carries the original hash.
This means the hash component cancels out in the comparison and doesn't affect redirect detection — which is probably fine for now since hash-only entries are out of scope (#1252). But it's worth being aware that if a server redirect changes the hash (e.g. response URL has a fragment), resolveHardNavigationTargetFromRscResponse ignores it and keeps the original hash. That's consistent with HTTP semantics (servers can't set fragments), but if the RSC response URL ever carries a hash hint, this comparison would miss it.
Not blocking — just flagging for the #1252 follow-up.
| }; | ||
| } | ||
|
|
||
| const redirectedHref = resolveHardNavigationTargetFromRscResponse( |
There was a problem hiding this comment.
Minor: resolveHardNavigationTargetFromRscResponse can return currentHref unchanged when responseUrl is null/undefined. That shouldn't happen here since the caller always passes a non-null responseUrl (from navResponseUrl ?? navResponse.url), but the types don't enforce it — options.responseUrl is typed as string, so we're fine, though the function itself accepts string | null | undefined. Just noting the implicit contract.
| 0, | ||
| "traverse", | ||
| undefined, | ||
| undefined, |
There was a problem hiding this comment.
The positional arguments to __VINEXT_RSC_NAVIGATE__ are getting long (7 args, several undefined placeholders). This is fine for now, but the next addition might be the time to switch to an options object for the internal call site.
Non-blocking observation.
| const targetHistoryIndex = readHistoryStateTraversalIndex(options.historyState); | ||
| let direction: TraverseDirection = "unknown"; | ||
|
|
||
| if (options.currentHistoryIndex !== null && targetHistoryIndex !== null) { |
There was a problem hiding this comment.
Good: when both indices are present and equal (e.g., currentHistoryIndex === targetHistoryIndex), this falls through to direction: "unknown". That's the correct choice — same-index traversal (replace on the current entry, or some exotic history API usage) shouldn't be classified as back or forward.
| href: "/target?tab=1", | ||
| historyUpdateMode: "push", | ||
| kind: "follow", | ||
| previousNextUrl: "/feed", |
There was a problem hiding this comment.
Nice: this test validates that same-path-different-search is treated as a redirect. This is the specific scenario the PR description calls out ("Redirect detection only compared pathnames" → now compares visible app hrefs).
| ); | ||
| const redirectDecision = resolveRscRedirectLifecycleHop({ | ||
| currentHref, | ||
| historyUpdateMode: currentHistoryMode ?? "replace", |
There was a problem hiding this comment.
The fallback currentHistoryMode ?? "replace" means that if the redirect decision helper receives historyUpdateMode: undefined (which happens after a previous redirect hop set currentHistoryMode = undefined), it gets coerced to "replace". But the follow decision then propagates this "replace" back as redirectDecision.historyUpdateMode, overwriting the loop variable.
On a multi-hop chain: first hop enters with the original mode (e.g. "push"), follow preserves "push". On the second hop, if the first hop's follow already set currentHistoryMode = "push", we're fine. But re-check: the first hop's follow sets currentHistoryMode = redirectDecision.historyUpdateMode which is the input options.historyUpdateMode ("push"). Second hop: currentHistoryMode is still "push", so "push" ?? "replace" = "push". Correct.
The ?? "replace" only fires if historyUpdateMode was previously set to undefined — which only happens in the old code path (now removed). With the new code, historyUpdateMode stays as the original value through all hops. The fallback is defensive. This is fine.
|
Review posted. Here's a summary of what I found: Overall assessment: Clean, well-structured PR. The redirect lifecycle extraction into Key feedback points:
|
NathanDrake2406
force-pushed
the
nathan/726-core-18-19-rsc-redirect-traverse
branch
3 times, most recently
from
da9e33a to
f99f4a3
Compare
|
/bigbonk review |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
NathanDrake2406
force-pushed
the
nathan/726-core-18-19-rsc-redirect-traverse
branch
from
f99f4a3 to
712737c
Compare
|
/bigbonk review |
There was a problem hiding this comment.
Review: RSC redirect lifecycle and traversal intent model
Read #726 before reviewing. This is a clean, well-scoped slice of the navigation lifecycle architecture.
What this gets right
Redirect lifecycle extraction is correct. resolveRscRedirectLifecycleHop is pure, exhaustive, and correctly factored as a decision helper. The three decision kinds (no-redirect, follow, terminal-hard-navigation) cover the complete space. The budget check only fires on actual redirects — a no-redirect at any depth correctly means the chain terminated naturally. External origin detection runs first, before any RSC-specific URL manipulation.
The deferred-commit model enforces the core invariant. The old code wrote history state mid-redirect-chain (replaceHistoryStateWithoutNotify inside the loop), which broke the "visible state only changes after approved commit" principle. The new code keeps all redirect hops inside currentHref/currentHistoryMode/currentPrevNextUrl loop variables and defers URL/history mutation to the eventual createNavigationCommitEffect. This preserves Back to the source entry after router.push() → server redirect.
Traversal intent resolution is deliberately conservative. resolveHistoryTraversalIntent correctly returns "unknown" when either index is null, and same-index traversal (equal indices) also falls through to unknown. This avoids synthesizing direction from stale or incomplete state.
Popstate timing is correct. The popstate handler calls navigateRsc(href, 0, "traverse") without a traversalIntent, so the fallback at lines 1081-1088 resolves intent from window.history.state. Since the browser updates history.state to the target entry before firing popstate, this reads the target's __vinext_historyIndex — which is the correct comparand against currentHistoryTraversalIndex (the index of the entry we're navigating from).
Test coverage validates the claimed invariants. The E2E test (back after router.push to a server-redirecting page) directly tests the core problem. The unit tests for redirect lifecycle, traversal intent, and planner trace fields are well-targeted.
Non-blocking observations
-
The
NavigateRsctype inapp-browser-popstate.tsstill has only 3 positional params, so the popstate handler can't passtraversalIntentdirectly. The fallback works correctly, but if a future slice needs explicit traversal intent passing (batched popstate events, synthetic traversals), that type will need updating. -
The positional args to
__VINEXT_RSC_NAVIGATE__(7 params, severalundefinedplaceholders) are getting long — echoing the previous review's observation that the next addition would be a good time to switch to an options object.
Inline comments below.
| ): number | null { | ||
| switch (historyUpdateMode) { | ||
| case "push": | ||
| return nextHistoryTraversalIndex + 1; |
There was a problem hiding this comment.
nextHistoryTraversalIndex is typed number and initialized from currentHistoryTraversalIndex ?? 0 (since line 196 applies ?? 0). commitHistoryTraversalIndex(null) sets currentHistoryTraversalIndex to null but only updates nextHistoryTraversalIndex when index !== null. So nextHistoryTraversalIndex stays a number through its entire lifecycle — meaning null + 1 is actually unreachable for the push case here.
Still, the coercion reads like a bug on first glance. A brief comment would help:
| return nextHistoryTraversalIndex + 1; | |
| // NB: nextHistoryTraversalIndex is always a number (initialized from | |
| // ?? 0 and only updated with non-null values), so this addition is | |
| // safe. If the invariant ever changes, null + 1 evaluates to 1 in JS, | |
| // intentionally resuming index allocation from unknown state. | |
| return nextHistoryTraversalIndex + 1; |
| options.currentHref, | ||
| options.origin, | ||
| ); | ||
| if (redirectedHref === toVisibleAppHref(options.currentHref, options.origin)) { |
There was a problem hiding this comment.
This comparison normalizes both sides to visible app hrefs. Since resolveHardNavigationTargetFromRscResponse carries the original request's hash (not the response's), the hash component cancels out in the comparison. That's correct for this slice since hash-only entries are scoped to #1252.
Flagging for the #1252 follow-up: if the RSC response URL ever carries a hash hint (unusual since HTTP servers can't set fragments, but possible via RSC-level signaling), this comparison would miss it because resolveHardNavigationTargetFromRscResponse always substitutes the original request's hash.
| ); | ||
| const redirectDecision = resolveRscRedirectLifecycleHop({ | ||
| currentHref, | ||
| historyUpdateMode: currentHistoryMode ?? "replace", |
There was a problem hiding this comment.
The ?? "replace" fallback only fires if currentHistoryMode is undefined. In the new code, currentHistoryMode starts as the caller's historyUpdateMode and gets set to redirectDecision.historyUpdateMode on each follow hop — which preserves the original mode. So undefined arriving here would require the initial caller to pass undefined for historyUpdateMode.
Traverse and refresh callers can do this (they don't set historyUpdateMode), so the fallback is reachable and correct — a traverse-initiated redirect should commit as "replace". Good defensive default.
| } | ||
|
|
||
| function isValidHistoryTraversalIndex(value: unknown): value is number { | ||
| return typeof value === "number" && Number.isSafeInteger(value) && value >= 0; |
There was a problem hiding this comment.
Good: isValidHistoryTraversalIndex rejects negative numbers, non-integers, and non-safe integers. This means corrupted or adversarial history state (e.g., NaN, Infinity, -1, 1.5) won't pollute the index tracking. The validation is tight.
| const targetHistoryIndex = readHistoryStateTraversalIndex(options.historyState); | ||
| let direction: TraverseDirection = "unknown"; | ||
|
|
||
| if (options.currentHistoryIndex !== null && targetHistoryIndex !== null) { |
There was a problem hiding this comment.
When both indices are present and equal, this correctly falls through to direction: "unknown". Same-index traversal (replace on the current entry, or exotic history API usage) should not be classified as back or forward — it's genuinely ambiguous.
| state: NavigationPlannerStateV0; | ||
| work: RequestedWork; | ||
| }): NavigationDecisionV0 { | ||
| const traverseFields = |
There was a problem hiding this comment.
Clean: traverseFields is only populated for traverseFlight work, so navigate/refresh/prefetch traces don't get a spurious traverseDirection: undefined field. The spread produces an empty object for non-traverse work, which is correct.
|
Review posted and approved. Here's a summary of what I found: Overall: Approved. This is a well-structured slice of the #726 navigation lifecycle architecture. Key findings:
|
RSC redirects were followed by mutating browser history before the redirected payload had passed the visible commit lifecycle. That could collapse the initiating history entry and leave back/forward traversal without an explicit intent model. Keep redirect hops inside the initiating navigation until an approved commit publishes history, add explicit terminal redirect decisions, and record per-entry traversal metadata so unknown browser history state stays unknown instead of being guessed. Add focused unit coverage for redirect decisions and traversal metadata plus a browser regression for router.push redirect back navigation.
NathanDrake2406
force-pushed
the
nathan/726-core-18-19-rsc-redirect-traverse
branch
from
712737c to
648f1ce
Compare
2 participants
Overview
#726-CORE-18/19: Model RSC redirect lifecycle and back/forward traversal intentunknownrather than guessed.app-browser-entry.ts,app-browser-rsc-redirect.ts,app-browser-state.ts,navigation-planner.tsBonk: please read issue #726 before reviewing this PR. The redirect and traversal changes are part of the broader navigation lifecycle architecture, not an isolated redirect patch.
Why
For App Router navigation to be correct, visible browser state must only change after the lifecycle controller approves a commit. The previous RSC redirect path followed server redirects by writing history before the final RSC payload was approved, which could collapse the source entry for
router.push()and leave back/forward traversal without a durable intent model.back,forward, andunknownare modeled explicitly, and unknown traversal targets keep the local index unknown.What changed
router.push()to an RSC server redirectback,forward, orunknown.Maintainer review path
packages/vinext/src/server/app-browser-rsc-redirect.ts: pure redirect lifecycle decision helper.packages/vinext/src/server/app-browser-entry.ts: inline redirect following, approved-commit history publishing, popstate traversal intent capture.packages/vinext/src/server/app-browser-state.ts: history metadata read/write helpers and traversal intent resolution.packages/vinext/src/server/navigation-planner.tsandnavigation-trace.ts: small planner surface for traversal direction and traces.tests/app-browser-entry.test.ts,tests/navigation-planner.test.ts, and the Playwright spec for coverage.Validation
vp test run tests/app-browser-entry.test.ts tests/navigation-planner.test.tsPLAYWRIGHT_PROJECT=app-router pnpm run test:e2e -- tests/e2e/app-router/nextjs-compat/router-push-pending.spec.tsvp checkknip.Risk and compatibility
window.__VINEXT_RSC_NAVIGATE__signature.#726-CORE-18/19.References
#726-CORE-18/19.nextjs-ref/packages/next/src/client/components/router-reducer/fetch-server-response.ts.nextjs-ref/packages/next/src/client/components/app-router.tsx