[PR #870] [MERGED] fix(app-router): keep isPending true across RSC-level redirects #910

New issue

Closed

opened 2026-05-06 13:10:47 +02:00 by BreizhHardware · 0 comments

BreizhHardware commented

2026-05-06 13:10:47 +02:00

Owner

📋 Pull Request Information

Original PR: https://github.com/cloudflare/vinext/pull/870
Author: @NathanDrake2406
Created: 4/22/2026
Status: ✅ Merged
Merged: 4/22/2026
Merged by: @james-elicx

Base: main ← Head: fix/app-router-redirect-inline-loop

📝 Commits (2)

6edaeb2 test(app-router): add E2E coverage for isPending across RSC redirect
01079cb fix(app-router): keep isPending true across RSC-level redirects

📊 Changes

5 files changed (+320 additions, -219 deletions)

View changed files

📝 packages/vinext/src/server/app-browser-entry.ts (+199 -219)
📝 tests/e2e/app-router/nextjs-compat/router-push-pending.spec.ts (+94 -0)
➕ tests/fixtures/app-basic/app/nextjs-compat/router-push-pending-destination/page.tsx (+9 -0)
➕ tests/fixtures/app-basic/app/nextjs-compat/router-push-pending-redirect/page.tsx (+8 -0)
📝 tests/fixtures/app-basic/app/nextjs-compat/router-push-pending/pending-client.tsx (+10 -0)

📄 Description

What this changes

router.push('/a') where /a's server component calls redirect('/b') no longer causes useTransition().isPending to flash false mid-redirect. The pending indicator stays true continuously from the push through the final destination commit.

Why

PR #868 latched isPending during programmatic navigations via a deferred promise in browser router state. But when the RSC response redirected (307), navigateRsc detected the response URL differing from the requested URL, settled the deferred promise to avoid orphaning it, 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.

The bug was explicitly documented in the bandage comment before the recursive call:

Settling here with the current committed state makes isPending flip false mid-redirect -- a deliberate tradeoff over leaning on React's internal transition-completion heuristic.

Issue #869 asked to remove that tradeoff.

Approach

Convert the recursive redirect follow into an inline while(true) loop inside navigateRsc. Loop variables (currentHref, currentHistoryMode, currentPrevNextUrl, redirectCount) are updated on each redirect hop; 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 hops.

The scattered settlePendingBrowserRouterState(pendingRouterState) calls inside the try body (early-return stale-id guards) are removed. The finally owns the single settlement site and is idempotent via the settled flag on PendingBrowserRouterState.

Scope: this is the minimal fix described in issue #869. The NavigationTask-based refactor in nathan/navigation-architecture.md is explicitly out of scope.

Validation

New E2E test (router-push-pending.spec.ts) installs a MutationObserver before 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.

New redirect E2E test: passes
Existing same-route pending E2E test: passes
Existing search-params-key E2E tests: pass
All 62 nextjs-compat E2E tests: pass
vp check: clean

Ported 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

The redirectDepth parameter on navigateRsc is 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.
Next.js history semantics for "push that redirects" (should it land as push or replace in history?) are unchanged; this PR is about isPending correctness only.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/cloudflare/vinext/pull/870 **Author:** [@NathanDrake2406](https://github.com/NathanDrake2406) **Created:** 4/22/2026 **Status:** ✅ Merged **Merged:** 4/22/2026 **Merged by:** [@james-elicx](https://github.com/james-elicx) **Base:** `main` ← **Head:** `fix/app-router-redirect-inline-loop` --- ### 📝 Commits (2) - [`6edaeb2`](https://github.com/cloudflare/vinext/commit/6edaeb2afcc5d68ad1b12b8c474c0c2a3c5abec6) test(app-router): add E2E coverage for isPending across RSC redirect - [`01079cb`](https://github.com/cloudflare/vinext/commit/01079cbc146a8d879e21276972a9b5db6fc91d47) fix(app-router): keep isPending true across RSC-level redirects ### 📊 Changes **5 files changed** (+320 additions, -219 deletions) <details> <summary>View changed files</summary> 📝 `packages/vinext/src/server/app-browser-entry.ts` (+199 -219) 📝 `tests/e2e/app-router/nextjs-compat/router-push-pending.spec.ts` (+94 -0) ➕ `tests/fixtures/app-basic/app/nextjs-compat/router-push-pending-destination/page.tsx` (+9 -0) ➕ `tests/fixtures/app-basic/app/nextjs-compat/router-push-pending-redirect/page.tsx` (+8 -0) 📝 `tests/fixtures/app-basic/app/nextjs-compat/router-push-pending/pending-client.tsx` (+10 -0) </details> ### 📄 Description ## What this changes `router.push('/a')` where `/a`'s server component calls `redirect('/b')` no longer causes `useTransition().isPending` to flash `false` mid-redirect. The pending indicator stays `true` continuously from the push through the final destination commit. ## Why PR #868 latched `isPending` during programmatic navigations via a deferred promise in browser router state. But when the RSC response redirected (307), `navigateRsc` detected the response URL differing from the requested URL, settled the deferred promise to avoid orphaning it, 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. The bug was explicitly documented in the bandage comment before the recursive call: > Settling here with the current committed state makes isPending flip false mid-redirect -- a deliberate tradeoff over leaning on React's internal transition-completion heuristic. Issue #869 asked to remove that tradeoff. ## Approach Convert the recursive redirect follow into an inline `while(true)` loop inside `navigateRsc`. Loop variables (`currentHref`, `currentHistoryMode`, `currentPrevNextUrl`, `redirectCount`) are updated on each redirect hop; the loop `continue`s without re-entering `navigateRsc`. A single `pendingRouterState` is created before the loop and lives until the `finally` block, so `isPending` stays `true` across all hops. The scattered `settlePendingBrowserRouterState(pendingRouterState)` calls inside the try body (early-return stale-id guards) are removed. The `finally` owns the single settlement site and is idempotent via the `settled` flag on `PendingBrowserRouterState`. Scope: this is the minimal fix described in issue #869. The `NavigationTask`-based refactor in `nathan/navigation-architecture.md` is explicitly out of scope. ## Validation New E2E test (`router-push-pending.spec.ts`) installs a `MutationObserver` before 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. - New redirect E2E test: passes - Existing same-route pending E2E test: passes - Existing search-params-key E2E tests: pass - All 62 nextjs-compat E2E tests: pass - `vp check`: clean Ported 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 - The `redirectDepth` parameter on `navigateRsc` is 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. - Next.js history semantics for "push that redirects" (should it land as push or replace in history?) are unchanged; this PR is about `isPending` correctness only. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>