fix(app-router): keep isPending true across RSC-level redirects#870
Conversation
router.push() to a page that calls redirect() in its server component produced isPending flashing false mid-redirect. This test exposes that by installing a MutationObserver before click, recording every text value of #pending-state, and asserting no idle entry appears between the first pending transition and the destination commit. Ported from Next.js: test/e2e/app-dir/navigation/navigation.test.ts#L482 New fixtures: - /nextjs-compat/router-push-pending-redirect -- server component that calls redirect() - /nextjs-compat/router-push-pending-destination -- final destination with #redirect-destination - Push-redirect button added to existing PendingClient
When router.push() targeted a page whose server component called redirect(), isPending flashed false mid-redirect. The fetch returned a 307, navigateRsc detected the response URL differing from the requested URL, settled the deferred pendingRouterState to prevent promise orphaning, then recursed with programmaticTransition=false. The settle fired before the recursive call so React resolved the transition and isPending dropped to false during the hop. Root cause: the recursive re-entry model required settling the deferred promise before handing off, because the recursive call had no way to inherit the existing promise. This was explicitly documented as a tradeoff in the bandage comment at the settle site. Convert the recursive redirect follow into an inline while loop. The loop variables (currentHref, currentHistoryMode, currentPrevNextUrl, redirectCount) are updated on each redirect hop and the loop continues without re-entering navigateRsc. A single pendingRouterState is created before the loop and lives until the finally block, so isPending stays true across all redirect hops and drops exactly once when the final destination commits. The scattered settlePendingBrowserRouterState() calls inside the try body (early-return stale-id guards) are removed. The finally owns the single settlement site, and the function is already inside React.startTransition from the programmatic push, so the correct transition priority is maintained throughout. Closes cloudflare#869.
commit: |
There was a problem hiding this comment.
Pull request overview
Fixes App Router programmatic navigation so useTransition().isPending stays continuously true across RSC-level redirects (e.g. router.push('/a') where /a server component calls redirect('/b')), aligning behavior with the intent from #868 and addressing #869.
Changes:
- Refactors
navigateRscredirect-following from recursive re-entry into an inlinewhile(true)loop so a singlePendingBrowserRouterStatespans all redirect hops. - Consolidates pending-router-state settlement to a single
finallysite (idempotent via thesettledflag), removing scattered settle calls in the main body. - Adds a new Next.js-compat E2E case + fixtures to verify no mid-redirect
"idle"flash in the pending indicator.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/fixtures/app-basic/app/nextjs-compat/router-push-pending/pending-client.tsx | Adds a new button to trigger a programmatic push to a server-redirecting route under startTransition. |
| tests/fixtures/app-basic/app/nextjs-compat/router-push-pending-redirect/page.tsx | New server component route that immediately calls redirect() to drive the RSC redirect path. |
| tests/fixtures/app-basic/app/nextjs-compat/router-push-pending-destination/page.tsx | New destination page used as the “committed” sentinel for the redirect test. |
| tests/e2e/app-router/nextjs-compat/router-push-pending.spec.ts | Adds an E2E test using a MutationObserver to assert isPending never flashes "idle" mid-redirect. |
| packages/vinext/src/server/app-browser-entry.ts | Core fix: inline redirect loop inside navigateRsc, keeping a single pending promise alive across hops and settling it once in finally. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Detection strategy: install a MutationObserver before click, record every | ||
| * text value #pending-state takes. After the destination commits, assert | ||
| * that no "idle" value appeared before the final "idle" (i.e. no mid-redirect | ||
| * flash). Because the destination page does not render #pending-state, we | ||
| * track when the element is removed as the end-of-navigation signal. |
There was a problem hiding this comment.
Comment says we assert no "idle" appeared before the final "idle", but the destination page unmounts #pending-state and the actual sentinel is "removed". Update the comment to match the implemented assertion (no "idle" between first "pending" and removal) to avoid confusion when maintaining this test.
| * Detection strategy: install a MutationObserver before click, record every | |
| * text value #pending-state takes. After the destination commits, assert | |
| * that no "idle" value appeared before the final "idle" (i.e. no mid-redirect | |
| * flash). Because the destination page does not render #pending-state, we | |
| * track when the element is removed as the end-of-navigation signal. | |
| * Detection strategy: install a MutationObserver before click and record | |
| * every text value #pending-state takes. Because the destination page does | |
| * not render #pending-state, the observer records "__removed__" when the | |
| * element is unmounted and uses that as the end-of-navigation signal. The | |
| * assertion then checks that no "idle" appears between the first "pending" | |
| * value and "__removed__" (i.e. no mid-redirect flash). |
| // Clear pendingPathname on all exit paths. On the success path this fires | ||
| // before the RAF commit effect, but commitClientNavigationState() in the | ||
| // commit effect clears it again — that double-clear is idempotent. Skipped |
There was a problem hiding this comment.
The comment says this finally-block clear runs "before the RAF commit effect", but navigateRsc awaits renderNavigationPayload's committed promise (resolved via RAF) before reaching finally. Please reword to accurately describe when/why pendingPathname is cleared, so future changes don't rely on incorrect timing assumptions.
| // Clear pendingPathname on all exit paths. On the success path this fires | |
| // before the RAF commit effect, but commitClientNavigationState() in the | |
| // commit effect clears it again — that double-clear is idempotent. Skipped | |
| // Clear pendingPathname on all exit paths as a final cleanup step. On | |
| // successful navigations it may already have been cleared as part of the | |
| // navigation commit, so this clear is intentionally idempotent. Skipped |
|
/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: fix(app-router): keep isPending true across RSC-level redirects
Clean fix. The recursion-to-loop refactor is the right call — it makes the pending-state lifetime obvious (one pendingRouterState created before the loop, one finally settlement after) and eliminates the structural reason the bug existed (settling before recursing).
The E2E test using MutationObserver to record the full state transition log is solid — it captures the exact sequence and fails deterministically on the old code.
A few items below, mostly minor.
| // Clear pendingPathname on all exit paths. On the success path this fires | ||
| // before the RAF commit effect, but commitClientNavigationState() in the | ||
| // commit effect clears it again — that double-clear is idempotent. Skipped |
There was a problem hiding this comment.
Copilot flagged this too and I agree — this comment is inaccurate. On the success path, await renderNavigationPayload(...) awaits the committed promise (resolved by the RAF commit effect), so the finally block runs after the commit effect, not before it.
The comment matters because it's explaining when clearPendingPathname fires relative to commit — getting that wrong could mislead future maintainers into adding order-dependent logic.
| // Clear pendingPathname on all exit paths. On the success path this fires | |
| // before the RAF commit effect, but commitClientNavigationState() in the | |
| // commit effect clears it again — that double-clear is idempotent. Skipped | |
| // Clear pendingPathname on all exit paths. On the success path, | |
| // renderNavigationPayload's committed promise already resolved (via the | |
| // RAF commit effect), so this is a no-op double-clear. On early-return | |
| // and error paths it is the primary cleanup. Skipped |
| let currentHref = href; | ||
| let currentHistoryMode = historyUpdateMode; | ||
| let currentPrevNextUrl = previousNextUrlOverride; | ||
| let redirectCount = redirectDepth; |
There was a problem hiding this comment.
PR description notes redirectDepth is now vestigial — no caller passes non-zero since recursion is gone. Consider adding a brief // TODO: remove — vestigial after inline-loop refactor so it doesn't get cargo-culted. Not blocking.
| setPendingPathname(url.pathname, navId); | ||
|
|
||
| const elementsAtNavStart = getBrowserRouterState().elements; |
There was a problem hiding this comment.
Minor observation: on redirect hops (second+ iteration), setPendingPathname is called again with the redirect destination. This is correct — it updates the pending pathname to the destination URL so isSameRoute and usePathname() see the right value. Just want to confirm this is intentional rather than an artifact of the loop running the full preamble on every hop.
Also: elementsAtNavStart and mountedSlotsHeader are recomputed on every hop. On redirect hops the browser tree hasn't changed (no render happened), so these will be identical to the first iteration's values. Not a correctness issue, but worth noting if this path gets hot.
| "", | ||
| destinationPath, | ||
| ); | ||
| if (finalUrl.pathname !== requestedUrl.pathname) { |
There was a problem hiding this comment.
The old code had a guard: if (!window.__VINEXT_RSC_NAVIGATE__) { ... window.location.href = destinationPath; return; } before recursing. Since we're now inside a loop within navigateRsc (which is __VINEXT_RSC_NAVIGATE__), that guard was unreachable dead code and removing it is correct.
| * Detection strategy: install a MutationObserver before click, record every | ||
| * text value #pending-state takes. After the destination commits, assert | ||
| * that no "idle" value appeared before the final "idle" (i.e. no mid-redirect | ||
| * flash). Because the destination page does not render #pending-state, we | ||
| * track when the element is removed as the end-of-navigation signal. |
There was a problem hiding this comment.
The comment says:
assert that no "idle" value appeared before the final "idle"
but the destination page doesn't render #pending-state, so there is no "final idle" — the signal is __removed__. The assertion logic at lines 124-134 correctly checks for no "idle" between first "pending" and __removed__, but this comment describes a different contract. Copilot's suggestion to reword is accurate.
| * Detection strategy: install a MutationObserver before click, record every | |
| * text value #pending-state takes. After the destination commits, assert | |
| * that no "idle" value appeared before the final "idle" (i.e. no mid-redirect | |
| * flash). Because the destination page does not render #pending-state, we | |
| * track when the element is removed as the end-of-navigation signal. | |
| * Detection strategy: install a MutationObserver before click, record every | |
| * text value #pending-state takes. Because the destination page does not | |
| * render #pending-state, the observer records "__removed__" when the element | |
| * is unmounted and uses that as the end-of-navigation signal. The assertion | |
| * then checks that no "idle" appears between the first "pending" and | |
| * "__removed__" (i.e. no mid-redirect flash). |
| obs.disconnect(); | ||
| } | ||
| }); | ||
| obs.observe(document.body, { childList: true, subtree: true, characterData: true }); |
There was a problem hiding this comment.
The observer watches { childList: true, subtree: true, characterData: true } but does not set characterData: true on the #pending-state element's text node specifically — it watches document.body. This means if any text node in the document changes (e.g., a loading spinner animates, or dev-mode HMR injects content), the callback fires and re-queries #pending-state. This is fine for correctness (the callback always re-reads the actual text), but it could produce duplicate log entries. The test handles duplicates gracefully since it only checks for the presence of "idle" in the window, so this is not a problem — just noting.
|
Review submitted. Here's a summary of the feedback: Approved — this is a clean, well-scoped fix. The recursion-to-loop refactor directly addresses the root cause (settling the pending promise before recursing) and the single 6 comments posted:
|
What this changes
router.push('/a')where/a's server component callsredirect('/b')no longer causesuseTransition().isPendingto flashfalsemid-redirect. The pending indicator staystruecontinuously from the push through the final destination commit.Why
PR #868 latched
isPendingduring programmatic navigations via a deferred promise in browser router state. But when the RSC response redirected (307),navigateRscdetected the response URL differing from the requested URL, settled the deferred promise to avoid orphaning it, then recursed withprogrammaticTransition=false. The settle fired before the recursive call, so React resolved the transition andisPendingdropped tofalseduring the hop.The bug was explicitly documented in the bandage comment before the recursive call:
Issue #869 asked to remove that tradeoff.
Approach
Convert the recursive redirect follow into an inline
while(true)loop insidenavigateRsc. Loop variables (currentHref,currentHistoryMode,currentPrevNextUrl,redirectCount) are updated on each redirect hop; the loopcontinues without re-enteringnavigateRsc. A singlependingRouterStateis created before the loop and lives until thefinallyblock, soisPendingstaystrueacross all hops.The scattered
settlePendingBrowserRouterState(pendingRouterState)calls inside the try body (early-return stale-id guards) are removed. Thefinallyowns the single settlement site and is idempotent via thesettledflag onPendingBrowserRouterState.Scope: this is the minimal fix described in issue #869. The
NavigationTask-based refactor innathan/navigation-architecture.mdis explicitly out of scope.Validation
New E2E test (
router-push-pending.spec.ts) installs aMutationObserverbefore click, records every text value of#pending-state, and asserts no"idle"entry appears between the first"pending"and the destination commit.Test log on unmodified code:
["idle","pending","idle","__removed__"]-- the mid-redirect flash at index 2 fails the assertion.Test log after fix:
["idle","pending","__removed__"]-- no idle flash.vp check: cleanPorted test from Next.js:
https://github.com/vercel/next.js/blob/canary/test/e2e/app-dir/navigation/navigation.test.ts#L482
Closes #869.
Risks / follow-ups
redirectDepthparameter onnavigateRscis now vestigial (no caller passes non-zero since recursion is gone). Left as-is to avoid an unnecessary signature churn; can be cleaned up in a separate pass.isPendingcorrectness only.