fix(app-router): keep isPending true across router.back() and router.forward()#876
fix(app-router): keep isPending true across router.back() and router.forward()#876NathanDrake2406 wants to merge 19 commits intocloudflare:mainfrom
Conversation
commit: |
There was a problem hiding this comment.
Pull request overview
This PR aligns router.back() / router.forward() with existing router.push() / router.replace() behavior by ensuring useTransition().isPending stays true from the synchronous call until the RSC navigation commits, preventing mid-navigation “idle” flashes.
Changes:
- Add a pre-arming hook (
window.__VINEXT_BEGIN_PENDING_TRAVERSAL__) to publish a deferred router-state promise inside the caller’s transition beforehistory.back()/history.forward(). - Introduce a generation-counter mechanism (
__VINEXT_PROGRAMMATIC_TRAVERSAL__) and wire it through popstate →__VINEXT_RSC_NAVIGATE__to mark traverse navigations as “programmatic transitions”. - Add nextjs-compat E2E coverage + fixtures for back/forward pending-state behavior.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/fixtures/app-basic/app/nextjs-compat/router-back-forward-pending/page.tsx | Adds Page A fixture with a slow server component to create a pending window. |
| tests/fixtures/app-basic/app/nextjs-compat/router-back-forward-pending/nav-client.tsx | Adds client controls to push() and forward() inside startTransition. |
| tests/fixtures/app-basic/app/nextjs-compat/router-back-forward-pending/destination/page.tsx | Adds Page B fixture with a slow server component. |
| tests/fixtures/app-basic/app/nextjs-compat/router-back-forward-pending/destination/back-client.tsx | Adds client control to back() inside startTransition. |
| tests/e2e/app-router/nextjs-compat/router-back-forward-pending.spec.ts | Adds E2E tests asserting no pending→idle flash for back/forward. |
| packages/vinext/src/shims/navigation.ts | Updates App Router back() / forward() to pre-arm pending and manage traversal bookkeeping + safety net. |
| packages/vinext/src/server/app-browser-entry.ts | Adopts pre-armed pending state in navigateRsc and passes programmaticTransition on popstate traversals. |
| packages/vinext/src/global.d.ts | Adds typings/docs for __VINEXT_PROGRAMMATIC_TRAVERSAL__ and __VINEXT_BEGIN_PENDING_TRAVERSAL__. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const myGen = window.__VINEXT_PROGRAMMATIC_TRAVERSAL__!; | ||
| setTimeout(() => { | ||
| if (window.__VINEXT_PROGRAMMATIC_TRAVERSAL__ === myGen) { | ||
| window.__VINEXT_PROGRAMMATIC_TRAVERSAL__ = myGen > 1 ? myGen - 1 : undefined; | ||
| settlePending?.(); |
There was a problem hiding this comment.
The setTimeout(0) safety-net check window.__VINEXT_PROGRAMMATIC_TRAVERSAL__ === myGen can misfire when there are rapid consecutive traversals. Example: back() called twice quickly sets the counter to 2; the first popstate decrements it to 1, which equals the first call’s myGen (1), so the first timer will run and clear the counter. That can cause the second popstate (or a coalesced traversal) to be treated as non-programmatic, breaking the pending-state adoption and potentially making isPending flash false (or creating a pending state outside a user transition). Consider tracking traversals with a monotonic id + separate “consumed” counter (or a queue/set of pending traversal ids) so each timer can reliably detect whether its popstate occurred, independent of later increments/decrements.
| window.addEventListener("popstate", (event) => { | ||
| // Consume one generation of the programmatic-traversal counter set by | ||
| // router.back()/forward(). Decrement rather than clear so that a rapid second | ||
| // traversal retains its own generation for its safety-net setTimeout. | ||
| const gen = window.__VINEXT_PROGRAMMATIC_TRAVERSAL__; | ||
| const isProgrammaticTraversal = gen !== undefined && gen > 0; | ||
| window.__VINEXT_PROGRAMMATIC_TRAVERSAL__ = gen !== undefined && gen > 1 ? gen - 1 : undefined; |
There was a problem hiding this comment.
The programmatic-traversal bookkeeping here relies on decrementing __VINEXT_PROGRAMMATIC_TRAVERSAL__ and having each back()/forward() safety-net timer look for an exact counter match. With rapid consecutive traversals, the counter can legitimately return to an earlier generation value after a popstate decrement (e.g., 2 → 1), which can make an earlier timer incorrectly conclude “no popstate happened” and clear the counter. That can cause subsequent popstate events to be misclassified as non-programmatic, breaking the pending-state behavior. Consider switching to a monotonic traversal id plus a separate “handled/consumed” counter (or an explicit queue of outstanding traversal ids) so popstate consumption and timer settlement are unambiguous.
| window.addEventListener("popstate", (event) => { | |
| // Consume one generation of the programmatic-traversal counter set by | |
| // router.back()/forward(). Decrement rather than clear so that a rapid second | |
| // traversal retains its own generation for its safety-net setTimeout. | |
| const gen = window.__VINEXT_PROGRAMMATIC_TRAVERSAL__; | |
| const isProgrammaticTraversal = gen !== undefined && gen > 0; | |
| window.__VINEXT_PROGRAMMATIC_TRAVERSAL__ = gen !== undefined && gen > 1 ? gen - 1 : undefined; | |
| let programmaticTraversalBacklog = 0; | |
| window.addEventListener("popstate", (event) => { | |
| // Merge any newly announced programmatic traversals into a local backlog and | |
| // clear the shared window field immediately. This prevents older back()/forward() | |
| // safety-net timers from matching a recycled counter value after popstate | |
| // consumption, while still allowing later popstate events to consume any | |
| // remaining programmatic traversals. | |
| const gen = window.__VINEXT_PROGRAMMATIC_TRAVERSAL__; | |
| const pendingProgrammaticTraversals = | |
| typeof gen === "number" && gen > 0 ? Math.trunc(gen) : 0; | |
| if (pendingProgrammaticTraversals > 0) { | |
| programmaticTraversalBacklog += pendingProgrammaticTraversals; | |
| window.__VINEXT_PROGRAMMATIC_TRAVERSAL__ = undefined; | |
| } | |
| const isProgrammaticTraversal = programmaticTraversalBacklog > 0; | |
| if (isProgrammaticTraversal) { | |
| programmaticTraversalBacklog -= 1; | |
| } |
| * navigation, record every text value of #pending-state, and assert that | ||
| * at least one "pending" entry appears and no "idle" entry appears between | ||
| * the first "pending" and the element being removed from the DOM. |
There was a problem hiding this comment.
The header comment describes observing #pending-state, but the test actually records #back-pending-state and #forward-pending-state. Updating the comment to match the actual selectors will make the contract clearer and avoid confusion when maintaining the test.
| * navigation, record every text value of #pending-state, and assert that | |
| * at least one "pending" entry appears and no "idle" entry appears between | |
| * the first "pending" and the element being removed from the DOM. | |
| * navigation, record every text value of #back-pending-state or | |
| * #forward-pending-state (depending on the case), and assert that at least | |
| * one "pending" entry appears and no "idle" entry appears between the first | |
| * "pending" and the element being removed from the DOM. |
|
Addressed the review. Three changes in 7233137:
|
|
Review findings addressed in two commits on this branch. B1 — Rapid back(); back(); flashing isPending falseRoot cause: Fix:
Trace for
Files: B2 — Tests for rapid concurrent traversalsTwo new E2E tests added to
Fixture buttons added to N1 — Overlapping popstates drop reducer outputWith B1 fixed, each traversal holds its own N2 — Module-scoped id under Vite multi-instancePreviously File: N3 — User-initiated popstate races programmatic traversalDocumented as a known limitation in the popstate handler comment (app-browser-entry.ts ~line 1308). The race requires a user-initiated back/forward to arrive in the brief window between N4 — Silent no-op pre-hydration
File: |
The pending-promise handoff between navigateRsc and React useState was defined inline in app-browser-entry.ts, tightly coupled to the module's setBrowserRouterState, activePendingBrowserRouterState, and routerReducer. That coupling prevented direct unit tests of the invariants the per-traversal ownership fix relies on: concurrent begins must produce independent promises, and a safety-net settle must not overwrite an earlier resolve with a stale fallback. Extract the pure primitive into app-browser-pending-state.ts. The new module exports createPendingBrowserRouterState, settlePendingBrowserRouterState (takes a fallback state), and resolvePendingBrowserRouterState (takes the next state). app-browser-entry keeps ownership of the active-pending bookkeeping and the setter/state-getter plumbing, delegating the pure work to the primitive. Adds tests/app-browser-pending-state.test.ts covering three invariants: concurrent pendings resolve independently to their own target states; resolve wins over a subsequent safety-net settle; and settle completes with the fallback when no prior resolve ran. These are the regression canary for PR cloudflare#876's rapid back(); back(); scenario without depending on Playwright timing.
|
/bigbonk review |
|
@james-elicx Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review |
1 similar comment
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nice refactor overall — the per-traversal pending-state registry is a clean fix for the back/forward isPending flash, and the unique-id registry addresses the counter-recycling race Copilot flagged. The extraction of app-browser-pending-state.ts is also a good move toward keeping the generated entry thin.
One correctness concern that I think is a regression from #868, plus a couple of smaller comments. Details inline.
Summary
- Potential regression: removing the auto-settle in
beginPendingBrowserRouterStatecombined with the new "reuse the shared slot" branch innavigateRscappears to break rapid overlappingrouter.push()/router.replace()/router.refresh()calls. The second call reuses the first call's pending, the first call'sfinallysettles that shared pending with stale state, and React's router state latches on the stale resolution — the second destination never commits. See inline comment. - Minor: pages-router fallthrough via optional chaining is fine, but the DEV warning in
runProgrammaticTraversalcan fire on App Router pre-hydration where the caller has no realistic way to delay the call (e.g., arouter.back()in a click handler that somehow races hydration). Consider softening the wording or gating on an "App Router detected" signal if one exists. - Minor: the test comment fix for
#back-pending-state/#forward-pending-stateis good. The newback(); then forward();round-trip test is a reasonable substitute for the synchronousback();forward();case given Chromium's task-queueing behavior, but the comment could briefly note that the registry's FIFO invariant is actually exercised by the rapidback();back();test, which is the more important N1 coverage.
| activePendingBrowserRouterState && !activePendingBrowserRouterState.settled | ||
| ? activePendingBrowserRouterState | ||
| : beginPendingBrowserRouterState(); | ||
| } |
There was a problem hiding this comment.
I think this reuse-the-shared-slot path introduces a regression for rapid overlapping router.push() / router.replace() / router.refresh() calls. Trace for router.push(A); router.push(B); inside one startTransition (or two consecutive clicks):
- push A enters
navigateRsc,navId1 = ++activeNavigationId(= 1),activePendingis null sobeginPendingBrowserRouterState()createspending1, setsactivePending = pending1, and callssetter(pending1.promise). React state slot =pending1.promise. Awaitsfetch. - push B enters
navigateRsc,navId2 = ++activeNavigationId(= 2).activePending = pending1and is not settled, so it reuses pending1 without callingsetteragain. Awaits its ownfetch. - push A's fetch resolves. The stale-id check (
navId !== activeNavigationId, line 1100) fails and A returns. - A's
finallyrunssettlePendingBrowserRouterState(pendingRouterState)→pending1resolves withgetBrowserRouterState()(the pre-push committed state).activePending = null. - push B's fetch resolves, proceeds to
renderNavigationPayload→dispatchBrowserTree→applyAction. BecausependingRouterState(=pending1) is still truthy,applyActioncallsresolvePendingBrowserRouterState(pending1, action), which no-ops (already settled) and never falls through tosetter(routerReducer(...)). - React's state slot is still
pending1.promise, which has already resolved with the pre-push state. Final committed state = pre-push. push B's destination never renders.
Prior to this PR, beginPendingBrowserRouterState auto-settled the prior unsettled pending and created a fresh pending2, calling setter(pending2.promise). That made the push A finally's settle(pending1) a no-op (already settled) and left React with pending2.promise, which correctly resolved to push B's destination.
The comment says "push/replace/refresh: single in-flight navigation, so the shared slot is safe" — but navigateClientSide is void-called from inside startTransition (shims/navigation.ts ~1254) and neither awaits the prior call nor guards against re-entry, so overlapping programmatic nav is structurally possible any time a user double-clicks a button, or when a component fires multiple router.push calls in response to the same event / effect.
Two options:
- Drop the reuse branch and always call
beginPendingBrowserRouterState()whenprogrammaticTransitionis true. Then restore the auto-settle-of-prior behavior, but scoped to this one call site (so the traversal path's invariant — each traversal owns its own pending — is preserved). This matches the pre-PR behavior for push/replace/refresh. - Keep the reuse branch but have the first call's
finallyonly settle itspendingRouterStateifactiveNavigationId === navIdoractivePendingBrowserRouterState === pendingRouterState, so a superseded older call doesn't tear down the shared pending that a newer call is still relying on.
A regression test for push(); push(); (or push(); replace();) landing on the second target would also be worth adding alongside the existing router-push-pending coverage.
| "has finished hydrating. useTransition().isPending will not be kept " + | ||
| "true for this navigation. If this is called from a useEffect or " + | ||
| "useLayoutEffect, ensure it only runs after the app has hydrated.", | ||
| ); |
There was a problem hiding this comment.
Minor: this DEV warning will fire when router.back() / router.forward() is called before the App Router browser entry has attached __VINEXT_BEGIN_PENDING_TRAVERSAL__. In practice, users often call these from useEffect on mount or from Server Actions that race hydration, and there isn't always a clean way for them to delay the call. The warning body is useful but the "ensure it only runs after the app has hydrated" guidance is hard to act on without vinext exposing a readiness signal.
Not blocking — just something to revisit. Two concrete ideas:
- Downgrade to
console.debug, or throttle to one warning per session, so it isn't noisy in dev. - Expose a tiny
isAppRouterReady()helper (or aPromiseresolved on hydration) so users have a first-class way to delay. Even documenting the trade-off in the warning body ("this will be a no-op isPending-wise, the navigation itself still works") would help.
| pendingToAdopt = traversalPendingStates.get(traversalId); | ||
| traversalPendingStates.delete(traversalId); | ||
| } | ||
| } |
There was a problem hiding this comment.
A user-initiated browser back/forward (physical keyboard / gesture / address-bar) that fires a popstate while __VINEXT_PROGRAMMATIC_TRAVERSALS__ has an outstanding entry for a not-yet-delivered programmatic traversal will consume the wrong entry here (oldest wins regardless of origin). The known-limitation comment above notes this, which is fair — but the user-visible failure mode is worth spelling out inline:
- The programmatic pending is adopted by the user's popstate and resolves on that navigation's commit, which is likely the wrong route tree (the user went "back" while the programmatic call wanted to go "forward", or vice versa).
useTransition().isPendingwon't flash but the state that resolves is wrong. - The programmatic traversal's own popstate (if it arrives) then finds an empty registry and goes through the non-programmatic path, producing the idle flash this PR was written to prevent.
Given how rare this race is in practice (user has to generate a popstate inside the ~macrotask window between invokeHistoryApi() and the programmatic popstate being delivered), deferring the fix is reasonable. A short TODO with a link back to the N3 comment in the PR description would make the intent discoverable for whoever picks this up later.
| registry.delete(myId); | ||
| settleFn(); | ||
| } | ||
| }, 0); |
There was a problem hiding this comment.
The setTimeout(0) safety net assumes the popstate task is delivered before a same-tick setTimeout(0) task fires. That ordering is stable in all current browsers for history.back() / history.forward() but, as the PR description notes, is not formally guaranteed by the HTML event-loop spec.
One practical hardening worth considering: instead of a bare setTimeout(0), use a short-lived queueMicrotask + setTimeout pair, or bump the timeout to a small non-zero value (e.g., 50ms) to give the popstate event an unambiguously larger window. 50ms is well under any user-perceptible delay for a no-op back/forward but gives you substantial margin against scheduler skew on slow devices or during heavy main-thread work. Not blocking — the current behaviour is fine on every real browser today.
| await expect(page.locator("#page-b2-marker")).toBeVisible({ timeout: 10_000 }); | ||
| expect(page.url()).toBe(`${BASE}/nextjs-compat/router-back-forward-pending/destination/step2`); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The back(); forward(); test comment explains why you avoid a single synchronous back();forward(); assertion, which is good. One suggestion: add a short note that the rapid back();back(); test above is what actually exercises N1 (two concurrent entries in the registry), and that this test is specifically covering the cross-traversal-type round-trip (pending settlement on the correct target after a prior traversal committed). Readers scanning this file will otherwise wonder what the two tests uniquely cover.
|
Review posted successfully at #876 (review). Summary of findings: Main concern (likely regression from #868): In
Prior to the PR, the old auto-settle branch in Minor comments on the DEV warning wording, the known-limitation comment for user-initiated popstate races, the |
|
/bigbonk review |
|
/bigbonk re-review for issues |
There was a problem hiding this comment.
Nice iteration — the per-traversal registry with unique ids, the FIFO popstate consumption, and extracting app-browser-pending-state.ts to a small typed helper all look good. The rapid-double-back E2E and the new push alpha then beta regression test in router-push-pending.spec.ts are appropriate coverage.
On the earlier bonk review's main concern (claimed push/push regression)
I do not reproduce that regression in this code. bonk's trace asserted that push B "reuses pending1 (no new setter call)", but reading beginPendingBrowserRouterState at packages/vinext/src/server/app-browser-entry.ts:171-176, the function unconditionally calls createPendingBrowserRouterState() and setter(pending.promise) on every invocation — there is no reuse branch. The pendingToAdopt parameter in navigateRsc is only populated by the popstate handler (traverse path) from the programmatic-traversal registry, not by push/replace/refresh.
Trace for router.push(A); router.push(B); inside one event handler:
- push A →
navigateRsc→beginPendingBrowserRouterState()creates pending1,setter(pending1.promise). React slot = pending1.promise. - push B →
navigateRsc→beginPendingBrowserRouterState()creates pending2,setter(pending2.promise). React slot = pending2.promise (latest setter wins). - push A's fetch completes, stale-id check returns early;
finallycallssettlePendingBrowserRouterState(pending1)which resolves pending1 with current state. Nothing reads pending1 — React already swapped to pending2.promise. - push B commits via
renderNavigationPayload→applyAction→resolvePendingBrowserRouterState(pending2, action); pending2 resolves with push B's destination, React latches to it.
The pre-PR code used an activePendingBrowserRouterState module slot + auto-settle to produce the same end result. The new design instead relies on React's useState always latching onto the most recent setter(promise) call, so superseded callers' pendings are harmless orphans. The new push regression test exercises exactly this path.
If this flow is reproducing a bug in practice, I would want to see a failing test or trace — but on a static read of the code I think the reasoning in bonk's comment is based on a reuse branch that doesn't exist in the diff.
Remaining smaller comments inline.
| registry.delete(myId); | ||
| settleFn(); | ||
| } | ||
| }, 50); |
There was a problem hiding this comment.
The 50ms safety-net timeout is a reasonable hardening over setTimeout(0), but the asymmetry is worth noting: a no-op history.back() now keeps isPending=true for 50ms before the pending is settled, even though it is a synchronous no-op semantically. In the common case this is invisible, but in a fast test harness or a UI that lights up a spinner immediately on pending-true it could produce a brief spurious pending flash on routes where history.back() is a no-op (e.g. the very first entry in a deep link).
Two options to consider:
- Before arming the registry entry, synchronously check
window.history.length/history.stateto detect obvious no-op cases and skip the registration + timer entirely. - Document that the 50ms is the worst-case ceiling for the pending flicker on no-op traversals, not just a timer for ordering guarantees. Readers will otherwise read the comment as purely about event-loop ordering and not realize there is a user-visible consequence.
| // called, so the oldest registered traversal matches this event. | ||
| // Known limitation: a user-initiated popstate arriving while a programmatic | ||
| // traversal is pending can consume the wrong entry, producing a brief | ||
| // isPending flash. Fixing it requires synchronous history-entry stamping. |
There was a problem hiding this comment.
Minor: this known-limitation comment is accurate but buried. The user-visible failure modes (programmatic pending adopted by the wrong popstate → commits to the wrong route tree; programmatic popstate then finds empty registry → idle flash) are worth spelling out for the next maintainer.
| // isPending flash. Fixing it requires synchronous history-entry stamping. | |
| // FIFO: browsers deliver popstate in the order history.back/forward was | |
| // called, so the oldest registered traversal matches this event. | |
| // | |
| // Known limitation: a user-initiated popstate (keyboard/gesture/address | |
| // bar) arriving while a programmatic traversal entry is still in the | |
| // registry will consume that entry (oldest wins, regardless of origin). | |
| // Symptoms: | |
| // * the programmatic pending is adopted by the user's popstate and | |
| // resolves with that navigation's committed state, which may be the | |
| // wrong route tree. | |
| // * the programmatic popstate then finds an empty registry and goes | |
| // through the non-programmatic path, producing the isPending idle | |
| // flash this PR was written to prevent. | |
| // A clean fix requires synchronously stamping history state before the | |
| // history API call so popstate events can be classified by origin. |
| "has finished hydrating. useTransition().isPending will not be kept " + | ||
| "true for this navigation. If this is called from a useEffect or " + | ||
| "useLayoutEffect, ensure it only runs after the app has hydrated.", | ||
| ); |
There was a problem hiding this comment.
This warning fires whenever router.back() / router.forward() is called before the App Router browser entry finishes hydrating. In practice, users call these from useEffect / useLayoutEffect on mount, from server-action callbacks that race hydration, or from synchronous effects during SSR stream flush — none of which have a clean way to gate on "app is hydrated".
The call still works (the navigation itself fires), only isPending tracking is missing. Two mitigations worth considering:
- Downgrade to
console.debugor throttle to once-per-session — a persistent warning on every such call gets noisy. - Mention in the body that the navigation still succeeds and only
isPendingis affected, so users don't panic.
| ); | |
| console.debug( | |
| "[vinext] router.back()/forward() called before the RSC browser entry " + | |
| "has finished hydrating. The navigation will still occur, but " + | |
| "useTransition().isPending will not be kept true for this traversal. " + | |
| "If this was called from useEffect/useLayoutEffect on mount, expect " + | |
| "this on first render only.", | |
| ); |
| */ | ||
| test("router.back() then router.forward() round-trips via separate clicks", async ({ page }) => { | ||
| await page.goto(FIXTURE); | ||
| await waitForAppRouterHydration(page); |
There was a problem hiding this comment.
Small readability nit: this test's docstring explains why a synchronous back(); forward(); assertion was avoided, but does not state what this test uniquely covers beyond the rapid back(); back(); test above. A one-line summary (e.g. "covers programmatic forward settling on the correct target after a prior back has committed") would help readers scanning the file.
|
Review posted successfully at #876 (review). Summary of my review: Overall: The per-traversal pending-state registry with unique ids and FIFO popstate consumption is a clean solution. The extraction of Pushed back on bonk's earlier "main concern" about a push/push regression. bonk's trace claimed push B "reuses pending1 (no new setter call)", but there is no reuse branch in Minor comments inline:
|
|
/bigbonk re-review for issues |
There was a problem hiding this comment.
Nice iteration — the per-traversal pending-state registry with FIFO consumption and the extracted app-browser-pending-state.ts helper read well, and the new push-alpha-then-beta regression test convincingly rebuts the earlier claim that removing the auto-settle would break rapid overlapping pushes. React's useState slot latching onto the most recent setter(pending.promise) call is a cleaner invariant than the old activePendingBrowserRouterState module slot.
Main concern: rapid router.back(); router.back(); commits a stale tree when the second back is a no-op
This PR introduces a regression relative to main for a narrow but real case: a user calls router.back() twice in the same handler, but only one history entry is actually available (e.g., full-page deep link → one client push → user mashes back twice).
Trace:
- back1 →
beginPendingBrowserRouterState()creates pending1,setter(pending1.promise), registers id=0, callshistory.back()(valid). - back2 → creates pending2,
setter(pending2.promise), registers id=1, callshistory.back()(no-op: no earlier entry). - React batches the two setter calls; state slot =
pending2.promise. React suspends onuse(pending2.promise). - Single popstate fires (for back1). Handler consumes id=0, calls
navigateRsc(..., pendingToAdopt=pending1). navId=1. - navigateRsc1 fetches →
dispatchBrowserTree→applyAction: becausependingRouterStateis set, it only callsresolvePendingBrowserRouterState(pending1, action)and returns early (app-browser-entry.ts:648-655). Nosetter(routerReducer(...))call. - pending1 resolves with the correct state for URL A — but React's state slot is pending2, so React ignores pending1's resolution entirely.
- 50ms later, the safety-net timer fires for back2's pending2 →
settle()→settlePendingBrowserRouterState(pending2)→ resolves pending2 withgetBrowserRouterState(). Because no setter ever ran,browserRouterStateRef.currentis still the pre-back state (B2 in the repro). pending2 resolves with stateB2. - React unsuspends and commits tree = B2. URL has been popstate'd to A.
User-visible outcome: URL shows A, rendered tree is still B2. isPending flips to idle before the correct tree commits. The existing rapid back(); back(); E2E test doesn't catch this because it explicitly builds a three-deep history ([blank, A, B, B2]) so both backs are valid and both popstates fire — hitting the success path where navigateRsc2 resolves pending2.
Pre-PR, router.back() called navigateRsc with programmaticTransition=false / pendingToAdopt=undefined, so applyAction fell through to setter(routerReducer(...)) on app-browser-entry.ts:657 and React committed the new state directly, regardless of history-traversal count.
Suggested fixes (either works):
- When a pending is orphaned (i.e. its promise is no longer the current React state slot), still call
setter(routerReducer(...))so React's committed state catches up. This is hard to detect directly, butdispatchBrowserTreecould do both: resolve the pending AND callsetter(action)unconditionally, then rely on React dedupe. In Next.js's reducer-based model, the setter is always called; the pending-promise pattern is a vinext-specific optimization. - In the popstate handler, after
navigateRscreturns (or in a finally), sweep any remaining registry entries for ids smaller than the consumed one and settle their pendings by callingsetter(currentState)via a new primitive — i.e., treat a consumed-but-lower-id scenario as 'the others will never fire popstate, settle them proactively'. This is clunkier. - Track which pending is currently the React state slot (a module-level
currentPendingSlotreference updated every timesetter(pending.promise)is called), and inapplyActionfall back tosetter(routerReducer(...))whenpendingRouterState !== currentPendingSlot. This preserves the orphan-is-harmless invariant for the happy path but heals the no-op case.
Regression test to add: from the fixture page (one entry deep into the app, no push-to-step2), fire router.back(); router.back(); inside a single startTransition and assert that after navigation #page-a-marker is visible and #page-b-marker is not. With the current diff, the URL will land on page A but the B tree will still be mounted.
I don't think the other review comments block merge, but this one does — it's a plausible user interaction and the failure mode is silent (no error, just a stuck UI).
Minor comments inline
- 50ms safety-net timeout has a user-visible side effect (pending flicker on no-op traversal) that the current comment only hints at.
- DEV warning for pre-hydration calls is hard to act on;
console.debugor throttling would be friendlier. - Known-limitation comment for user-initiated popstate races could spell out the user-visible symptoms for future maintainers.
| // newer navigation's commit wins even when the older call's finally | ||
| // runs first. | ||
| pendingRouterState = beginPendingBrowserRouterState(); | ||
| } |
There was a problem hiding this comment.
Likely regression for rapid router.back(); router.back(); when the second back is a no-op. Full trace in the review summary.
Short version: when popstate only fires once (because the second back had no history entry), navigateRsc1 runs and calls applyAction with pendingRouterState=pending1. applyAction at app-browser-entry.ts:648-655 short-circuits to resolvePendingBrowserRouterState(pending1, action) and does not call setter(routerReducer(...)). But React's state slot was rebound to pending2.promise by back2, so pending1's resolution is ignored. 50ms later the safety net settles pending2 with the stale getBrowserRouterState(), and React commits the stale tree even though the URL has changed.
The existing rapid-double-back E2E doesn't catch this because it builds a three-deep history. Consider adding a variant that starts one entry deep so the second back is genuinely a no-op.
A minimal fix: track the most-recent pending that was handed to setter and, in applyAction, fall back to setter(routerReducer(...)) when pendingRouterState is not the current slot. That keeps the orphan-is-harmless invariant for the happy push(alpha); push(beta); path while healing the no-op traversal case. Alternatively, always call both resolvePending(...) and setter(routerReducer(...)) in dispatchBrowserTree — the setter call is idempotent in a reducer sense and mirrors what Next.js itself does.
| registry.delete(myId); | ||
| settleFn(); | ||
| } | ||
| }, 50); |
There was a problem hiding this comment.
Two minor notes on the 50ms timer:
-
The comment captures the two purposes well, but (2) is not just a no-op-at-history-boundary concern: whenever any traversal fails to produce a popstate (no history entry, concurrent navigation races, pagehide, etc.),
isPendingwill stay true for the full 50ms before the safety net fires. On a fast UI that spins up a loading indicator the instantisPendinggoes true, that's a visible flicker. Not blocking — just worth making explicit so a future maintainer doesn't shorten the timer without realizing it's a latency ceiling for a user-visible state. -
The ordering argument in (1) relies on the popstate task being dispatched before the
setTimeout(50)task. This is robust in current browsers but, as the PR description notes, is not formally guaranteed by the HTML spec. 50ms is generous enough that it's almost certainly fine in practice, but if you see the safety net firing in tests on slow CI runners with main-thread contention, consider bumping to 100ms.
A small hardening would be to synchronously check for obvious no-op cases before registering at all — e.g., window.history.length <= 1 for back() from the first entry — so the registry + timer don't get armed for a traversal that can't fire popstate. That eliminates both the flicker and the ordering assumption for the common deep-link-first-page case.
| "has finished hydrating. useTransition().isPending will not be kept " + | ||
| "true for this navigation. If this is called from a useEffect or " + | ||
| "useLayoutEffect, ensure it only runs after the app has hydrated.", | ||
| ); |
There was a problem hiding this comment.
The DEV warning fires whenever router.back() / router.forward() is called before __VINEXT_BEGIN_PENDING_TRAVERSAL__ is attached. In practice users often call these from useEffect / useLayoutEffect on mount, from server-action callbacks that race hydration, or from synchronous effect cleanups during SSR stream flush — none of which have a clean way to gate on "app is hydrated".
The navigation itself still fires correctly (the ?. fallback preserves that), only isPending tracking is missing. Two mitigations worth considering:
- Downgrade to
console.debugor throttle to once-per-session so it isn't noisy. - Clarify in the body that navigation still works and only
isPendingis affected.
| ); | |
| console.debug( | |
| "[vinext] router.back()/forward() called before the RSC browser entry " + | |
| "has finished hydrating. The navigation will still occur, but " + | |
| "useTransition().isPending will not be kept true for this traversal. " + | |
| "If this was called from useEffect/useLayoutEffect on mount, expect " + | |
| "this on first render only.", | |
| ); |
| // called, so the oldest registered traversal matches this event. | ||
| // Known limitation: a user-initiated popstate arriving while a programmatic | ||
| // traversal is pending can consume the wrong entry, producing a brief | ||
| // isPending flash. Fixing it requires synchronous history-entry stamping. |
There was a problem hiding this comment.
Minor: this known-limitation comment is accurate but underspecified. Spelling out the user-visible failure modes will make it easier for the next maintainer to judge urgency:
- The programmatic pending is adopted by the user's popstate and resolves with that navigation's committed state, which may be the wrong route tree (user went back while programmatic call wanted to go forward, or vice versa).
- The programmatic traversal's own popstate (if it still arrives) then finds an empty registry and goes through the non-programmatic path, producing the
isPendingidle flash this PR was written to prevent.
| // isPending flash. Fixing it requires synchronous history-entry stamping. | |
| // FIFO: browsers deliver popstate in the order history.back/forward was | |
| // called, so the oldest registered traversal matches this event. | |
| // | |
| // Known limitation: a user-initiated popstate (keyboard / gesture / | |
| // address bar) arriving while a programmatic traversal entry is still in | |
| // the registry will consume that entry (oldest wins, regardless of | |
| // origin). Symptoms: | |
| // * the programmatic pending is adopted by the user's popstate and | |
| // resolves with that navigation's committed state, which may be the | |
| // wrong route tree. | |
| // * the programmatic popstate then finds an empty registry and falls | |
| // through to the non-programmatic path, producing the isPending idle | |
| // flash this PR was written to prevent. | |
| // A clean fix requires synchronously stamping history state before the | |
| // history API call so popstate events can be classified by origin. |
|
Review posted successfully at #876 (review). SummaryMain finding (blocking): A regression in the rapid Trace: back1 creates pending1 and is valid → popstate1 adopts pending1 → Pre-PR this worked because The existing "rapid back(); back();" E2E test doesn't catch this because it intentionally builds a 3-deep history so both backs fire popstates. Suggested fix directions include tracking the "current pending slot" in a module ref and falling back to Minor notes:
Also pushed back (in agreement with the PR author and prior bonk re-review) on the earlier "push/push regression" claim — the |
|
/bigbonk re-review for issues |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Safety net: if history.back/forward was a no-op (no matching entry), | ||
| // no popstate fires; this settles the orphaned pending state. 50ms | ||
| // serves two purposes: (1) the popstate macrotask gets an unambiguous | ||
| // window ahead of the timer on slow devices or under main-thread | ||
| // pressure — the HTML spec does not guarantee popstate ordering | ||
| // relative to a 0ms timeout; (2) on a truly no-op traversal (e.g. the | ||
| // first entry of a deep link), 50ms is the worst-case isPending | ||
| // flicker before useTransition settles back to idle. | ||
| setTimeout(() => { | ||
| const settleFn = registry.get(myId); | ||
| if (settleFn !== undefined) { | ||
| registry.delete(myId); | ||
| settleFn(); | ||
| } | ||
| }, 50); |
There was a problem hiding this comment.
The safety-net timer for no-op traversals uses setTimeout(..., 50) here, but the PR description refers to a setTimeout(0) safety net. Please align the PR description with the implementation (or switch the implementation back to 0ms if that was the intended behavior), so future maintainers don't reason from stale documentation.
|
/bigbonk re-review for issues |
There was a problem hiding this comment.
Good iteration. The unconditional setter(routerReducer(...)) in applyAction is a clean fix for the rapid-back-with-noop regression I flagged last round — it ensures React's live state slot catches up even when the pending a traversal armed becomes orphaned. The new push-alpha-then-beta test is good regression coverage, and the unique-id-per-traversal registry plus FIFO popstate consumption read cleanly.
Tracing the two cases I was worried about:
Rapid back(); back(); with no-op second back (the regression I raised in the last review):
- back1 → setter(p1.promise); back2 → setter(p2.promise). Live slot = p2.promise.
- popstate1 → navigateRsc(pendingToAdopt=p1) → applyAction: resolvePending(p1, action) (orphan, ignored) AND setter(concreteStateA). Live slot = concreteStateA. ✓
- 50ms → settle(p2) → p2.resolve(getBrowserRouterState() = stateA). Orphan — ignored.
React commits stateA, URL is A, no idle flash. The new setter-always call is what heals this. The new "back(); back(); with no-op second back" E2E exercises exactly this path.
Overlapping push(A); push(B); (the concern the earlier bonk review raised against the auto-settle removal):
- pushA → setter(p1.promise); pushB → setter(p2.promise). Live slot = p2.promise.
- pushA's stale-id check bails before applyAction runs; finally → settle(p1). Orphan resolve, ignored.
- pushB applyAction: resolvePending(p2, action) AND setter(concreteStateB). Live slot = stateB. ✓
The push-alpha-then-beta test validates this and I could not reproduce the trace from the earlier bonk review claiming p1 was reused.
Remaining comments
All non-blocking. Details inline.
- The unconditional
setter(routerReducer(...))means the pending-promise handoff is now effectively redundant in the happy programmatic path — both signals resolve with the same state. Worth a brief comment so a future maintainer doesn't mistake one for dead code and remove it. - The new
back(); back(); no-op second backtest relies on abeforeunloaddialog-dismiss to cancel back2's cross-document unload. That setup is subtle enough to warrant a one-line comment pointing at what the guard is actually protecting against. - Minor comment-wording nits on the 50ms flicker ceiling and the DEV warning gating.
| } | ||
|
|
||
| setter(routerReducer(getBrowserRouterState(), action)); | ||
| }; |
There was a problem hiding this comment.
Consider adding a sentence explaining the relationship between the two calls here, because the pending.resolve + setter overlap is subtle:
- For a traversal with
pendingToAdopt: the resolve settles the promise that React is suspending on viause(pending.promise). Thesetter()below is redundant in the happy case (resolves to the same state) but heals the no-op rapid-traversal case where a sibling traversal rebound the live state slot to a different pending. - For push/replace/refresh where the pending was created by
beginPendingBrowserRouterState(): same as above — the setter is redundant in the happy case (the latestsetter(pending.promise)call innavigateRsclatched the live slot to this call's pending, soresolve()alone would suffice) but heals the case where a newer call has superseded this call's setter with its own pending.
Both branches converge to the same runtime behavior (React gets a final concrete state in its queue), so this is correctness-by-overdetermination. A future cleanup candidate is "if the unconditional setter is sufficient, the pending mechanism itself could be simplified" — noting that here will save the next reader from reaching the same conclusion and ripping out what looks like dead code.
| }; | |
| const applyAction = () => { | |
| // Unconditional setter heals the case where a sibling navigation rebound | |
| // React's live state slot to a different pending (rapid back();back(); | |
| // no-op second, or superseded push A while push B is still in flight). | |
| // In the happy case the resolve and the setter write the same state and | |
| // React dedupes; the resolve is still needed so `use(pending.promise)` | |
| // unsuspends in the same commit instead of waiting for the setter render. | |
| if (pendingRouterState) { | |
| resolvePendingBrowserRouterState(pendingRouterState, action); | |
| } | |
| setter(routerReducer(getBrowserRouterState(), action)); | |
| }; |
| * interactions must round-trip the URL cleanly: back lands on the | ||
| * previous entry, forward returns to the original. | ||
| * | ||
| * Uniquely covers: programmatic forward settles its pending state on the |
There was a problem hiding this comment.
The beforeunload guard is doing load-bearing work here (it cancels back2's cross-document unload so pending2 reaches the 50ms safety-net path), but the rationale is not obvious from the test body alone. Consider a brief comment so a future maintainer who tries to "simplify" this test doesn't accidentally remove it:
| * Uniquely covers: programmatic forward settles its pending state on the | |
| await page.evaluate(() => { | |
| // back2 would cross the document boundary into about:blank and unload | |
| // this page. beforeunload cancels that unload so back2 becomes a true | |
| // in-page no-op — no popstate fires, and pending2 reaches the 50ms | |
| // safety-net settle path. Without this, the test would exercise a | |
| // completely different code path (full-page navigation). | |
| window.addEventListener("beforeunload", (event) => { | |
| event.preventDefault(); | |
| event.returnValue = ""; | |
| }); | |
| }); |
(Depending on Playwright/Chromium behavior, the dialog.dismiss() handler at the top of the test may also need to stay documented in lockstep — modern Chromium does not show a dialog when returnValue is set programmatically without user interaction, but a future engine change could reintroduce one.)
| registry.delete(myId); | ||
| settleFn(); | ||
| } | ||
| }, 50); |
There was a problem hiding this comment.
Two small refinements:
- The
registry.get(myId) !== undefinedcheck followed byregistry.delete(myId)is slightly awkward;Map.deletereturns a boolean so you can collapse to one lookup:
| }, 50); | |
| setTimeout(() => { | |
| const settleFn = registry.get(myId); | |
| if (settleFn !== undefined && registry.delete(myId)) { | |
| settleFn(); | |
| } | |
| }, 50); |
- This 50ms timer fires for every programmatic traversal — not just no-ops. In the happy (popstate-fires) path the handler has already deleted
myIdbefore the timer runs, so the timer is a silent no-op, but each traversal still posts a timer task. Ifrouter.back()gets called hundreds of times per session (unlikely but plausible in automated-testing contexts) that's a pile of outstanding timers. Not blocking — just something to keep in mind if timer churn ever shows up in a profile. A lightweight fix would be to cancel the timer from the popstate handler by storing the timer id alongside the settle function in the registry, but it's not worth the added bookkeeping unless you actually see it.
| "useTransition().isPending will not be kept true for this " + | ||
| "traversal. If called from useEffect/useLayoutEffect on mount, " + | ||
| "expect this on first render only.", | ||
| ); |
There was a problem hiding this comment.
The console.debug downgrade is an improvement. One more small refinement worth considering: gate on window.__VINEXT_RSC_NAVIGATE__ being defined (i.e. "are we in an App Router app at all") to silence the warning cleanly for Pages Router usage, where calling router.back() is completely legitimate and always takes this null branch:
| ); | |
| if (import.meta.env.DEV && beginResult === null && typeof window.__VINEXT_RSC_NAVIGATE__ === "function") { | |
| // App Router detected (RSC nav handler attached) but pending gate not | |
| // armed: racy effect call during hydration. Only log in this narrow | |
| // case — Pages Router callers legitimately never arm the gate and | |
| // should not see the message. | |
| console.debug( |
Otherwise, a Pages Router codebase that doesn't even use useTransition will see this debug log every time router.back() is called, which is noise.
| pendingToAdopt = traversalPendingStates.get(traversalId); | ||
| traversalPendingStates.delete(traversalId); | ||
| } | ||
| } |
There was a problem hiding this comment.
Small suggestion for the FIFO semantics: reading the oldest key via registry.keys().next() is correct (Map iteration is insertion order) but a bit indirect. Consider extracting the intent into a named helper for readability, or at least a one-line reminder:
| } | |
| // Map iteration order is insertion order, so keys().next() returns the | |
| // oldest registered traversal id. FIFO: browsers deliver popstate in the | |
| // order history.back/forward was called, so the oldest registered | |
| // traversal matches this event. | |
| // | |
| // Known limitation: a user-initiated popstate (keyboard / gesture / |
Also worth calling out explicitly in the limitation block: the symptom list is accurate but leaves "what should a user do" implicit. If you anticipate shipping the synchronous-history-state-stamping fix, a TODO(#xxx) linking to a tracking issue would be valuable.
|
Review posted successfully at #876 (review). SummaryConvergence on the prior concern: The unconditional Pushed back (again) on the earlier claim that removing the auto-settle breaks overlapping Remaining comments (all non-blocking):
|
NathanDrake2406
force-pushed
the
fix/router-back-forward-ispending
branch
from
865c8fc to
abe7222
Compare
|
Review posted successfully at #876 (review). SummaryThe latest revision is in solid shape. The FIFO One non-blocking correctness gap worth fixing:
Two minor notes:
Other previously-flagged hazards (rapid same-direction back/back with both valid, one-entry-deep no-op second, StrictMode mount/cleanup, legacy browser degradation) are addressed in this revision. |
|
/bigbonk review |
There was a problem hiding this comment.
Latest revision looks good. The split between the FIFO traversalPendingQueue (back/forward) and activePendingBrowserRouterState (push/replace/refresh), plus the navigation.entries()-based optimistic budgeting, cleanly addresses the cross-kind adoption hazard and the same-tick no-op leak from earlier rounds. The unconditional setter(routerReducer(...)) in applyAction is the right heal mechanism for the orphan-pending case, and the new test fixtures (rapid double-back, one-entry-deep double-back, user-popstate-resets-budget) lock down the documented contract.
Traced the previously-flagged hazards against this revision and they're addressed:
- Rapid same-direction
back();back();(both valid): queue contains [p1, p2]. Two popstates fire, popstate1 dequeues p1 (FIFO matches browser delivery order), popstate2 dequeues p2. Each adoption is correct,applyActionresolves and writes setter — orphan from any stale-id finally is healed by unconditional setter. ✓ - Rapid same-direction
back();back();(one entry deep, second is no-op):canOptimisticallyTraverseSameDocumentreturnsfalsefor the second call (no entry atcurrentIndex + offset(-1) - 1),??short-circuits keepcanTraverse=false(boolean, not nullish), arm is skipped. Queue holds only p1, no leak. ✓ - Cross-kind
back()+push()interleave:beginPendingBrowserRouterState(push path) only auto-settlesactivePendingBrowserRouterState, not queue entries. The back's queue-armed pending is preserved and dequeued by its own popstate. React's slot rebinds last-setter-wins, and the unconditional setter inapplyActionconverges the final committed tree. ✓ - StrictMode dev mount→cleanup→mount: arm hook is (re)assigned in
BrowserRoot's mountuseLayoutEffect, so cleanup→mount restores it. ✓ - Legacy browsers (no Navigation API): arm is skipped, degrades to bare history call without leaking. JSDoc explicitly documents this is intentional. ✓
One genuine non-blocking observation worth a comment, plus a very small nit. None of these block merge.
| // resolves only the pending its own router.back/forward armed, | ||
| // independent of any push/replace/refresh that ran in between. | ||
| // User-initiated popstates find an empty queue and fall through. | ||
| pendingRouterState = dequeueTraversalPendingBrowserRouterState(); |
There was a problem hiding this comment.
Non-blocking: the FIFO queue isolates adoption ownership between programmatic kinds (push vs. traverse), but it cannot multiplex by origin within the traverse path. A user-initiated popstate (gesture / keyboard / address bar) that arrives while a queued programmatic pending exists will dequeue and adopt that pending — the user's traversal then resolves the programmatic transition's promise, and the programmatic traversal's own popstate (if it still fires) finds an empty queue and falls through to the plain path, producing a brief idle flash for that half. This race requires popstate2 to fire between runProgrammaticTraversal's history.back/forward() call and the queued popstate1 task running, so it's quite narrow in practice.
The unconditional setter in applyAction heals the committed tree state in both halves, but the user transition's isPending is bound to a navigation it didn't open. The known-limitation comment a few lines above only mentions "User-initiated popstates find an empty queue and fall through unchanged" — worth spelling out the interleave case too so the layering boundary is explicit for future maintainers (a clean fix would require synchronously stamping history.state with a programmatic marker before the history API call, deliberately out of scope per the PR description).
| useLayoutEffect(() => { | ||
| setBrowserRouterState = setTreeStateValue; | ||
| browserRouterStateRef = stateRef; | ||
| window.__VINEXT_APP_ROUTER_READY__ = true; |
There was a problem hiding this comment.
Tiny nit, non-blocking: __VINEXT_APP_ROUTER_READY__ = true here is fine, but the cleanup branch only resets it to false when browserRouterStateRef === stateRef (line 645-648). If two BrowserRoot instances ever overlapped (shouldn't normally happen, but StrictMode dev double-invokes effects), the older mount's cleanup wouldn't flip the flag back to false even though it's no longer the live owner. The __VINEXT_RSC_NAVIGATE__ and __VINEXT_ARM_TRAVERSAL_PENDING__ hooks also only get cleared in that conditional. The current setup works because the ready-flag check is paired with a setBrowserRouterState truthiness check inside the arm hook itself, but the asymmetric ownership (mount writes unconditionally, cleanup writes conditionally) is a small footgun. Not worth changing for this PR; just flagging.
| // App Router scroll restoration is handled in server/app-browser-entry.ts:697 | ||
| // with RSC navigation coordination (waits for pending navigation to settle). | ||
| window.addEventListener("popstate", (event) => { | ||
| resetOptimisticTraversalBudget(); |
There was a problem hiding this comment.
Worth a one-line comment here: this resetOptimisticTraversalBudget() call is what makes the same-tick offset model robust against user-initiated traversals. Without it, a user pressing the physical back button between two router.back() calls in different ticks could leave the offset stale and misclassify the second router.back() as a no-op (the failure mode in the second-to-last bonk review). Because the reset runs from the popstate handler unconditionally (App Router and Pages Router), the offset only ever tracks same-task arms, which matches the JSDoc claim on runProgrammaticTraversal.
A future refactor that splits the popstate handler by router kind could accidentally drop this reset; the comment would catch it.
|
Review posted successfully at #876 (review). SummaryThe PR has converged on a solid design. The split between the FIFO
Three non-blocking inline comments:
|
…ontinuity router.back() and router.forward() are bare window.history.back/forward() calls in the navigation shim. When wrapped in React.startTransition, the callback exits synchronously before popstate fires (popstate is a queued browser task), so no setState is tied to the transition and useTransition().isPending flashes idle before the traversal commits. Add a fixture tree that exercises the traversal paths with a ~1s Suspense boundary on each destination, and four Playwright specs covering: - router.back() keeps isPending true until the previous route commits - router.forward() keeps isPending true until the forward route commits - rapid router.back(); router.back(); stays pending through the second hop - router.back() then router.forward() across separate clicks round-trip The first spec reproduces the current bug with the log signature ["b:idle","b:pending","b:idle","__removed__"]. The remaining specs provide regression coverage for the forthcoming fix.
…forward() router.back() and router.forward() dispatched bare window.history.back() / forward() calls. When a user wrapped them in React.startTransition, the callback exited synchronously before the popstate event fired, because popstate is queued as a new browser task. No setState occurred inside the transition, so useTransition().isPending never latched and the UI flashed to idle mid-traversal. The PendingBrowserRouterState pattern introduced for programmatic push / replace / refresh published a deferred Promise into the router's useState slot; the transition stayed open until the new committed state resolved that Promise. Traversals were never wired into the same pattern. Arm a deferred pending from the shim, inside the user's startTransition, via a new window.__VINEXT_ARM_TRAVERSAL_PENDING__ hook registered by the App Router browser entry at hydration time. navigateRsc, when running on the popstate path with no programmatic flag, adopts the active pending and resolves it on commit. User-initiated popstates (keyboard / gesture / address bar) find no active pending and fall through unchanged. Use the Navigation API's synchronous canGoBack / canGoForward availability hints to avoid arming a pending when the traversal is a no-op (no popstate will fire). When the Navigation API is unavailable, skip arming and fall back to the bare traversal; isPending preservation degrades to the same behavior as Next.js on those browsers. No safety timer, no cleanup listeners, no registry.
CI's stock tsc (used by the pages-router-prod Playwright webServer via npx tsc -p packages/vinext/tsconfig.json) predates the Navigation API types in lib.dom.d.ts, so window.navigation is not known and the build fails with TS2551 before the server can start. Declare the minimal readonly surface we consume (canGoBack, canGoForward) on Window. The vp check lint uses a newer TS preview that already knows the type, so this was not caught locally.
__VINEXT_ARM_TRAVERSAL_PENDING__ is published immediately after hydrateRoot() returns, but setBrowserRouterState / browserRouterStateRef are only assigned inside BrowserRoot's useLayoutEffect. Child useLayoutEffects fire before BrowserRoot's (child-first ordering), so a router.back() / router.forward() called from a child's useLayoutEffect on mount can reach the arm hook before the setter is ready, throwing "Browser router state setter is not initialized". Check window.__VINEXT_APP_ROUTER_READY__ before arming. The readiness flag already flips in BrowserRoot's useLayoutEffect and reverts on unmount, so post-teardown calls are silently skipped too. The traversal itself still fires; only isPending tracking for this one call is missed — same trade-off as the pre-hydration path.
Rapid overlapping traversals (e.g. router.back(); router.back();) cause two navigateRsc invocations to adopt the same activePendingBrowserRouterState. The older invocation's stale-id early return runs settlePendingBrowserRouterState in its finally, resolving the shared pending with the pre-commit router state. The surviving invocation's applyAction then observed pending.settled and no-op'd, leaving React's state slot bound to the stale promise resolution — committing the wrong tree under the correct URL. The non-pending branch already wrote setter(routerReducer(...)). The pending branch returned early after resolvePendingBrowserRouterState, trusting that the resolution alone would drive the commit. That trust breaks when a sibling navigateRsc settles the shared pending stale before this branch runs. Drop the early return so applyAction always calls setter(routerReducer(...)) after resolvePending. The setter call rebinds React's slot to the concrete next state; in the common case it batches with the resolution into one transition commit, in the race case it overrides the stale resolution and re-commits the correct tree.
window.__VINEXT_ARM_TRAVERSAL_PENDING__ was assigned once in bootstrapHydration at module load, but BrowserRoot's useLayoutEffect cleanup cleared it on unmount. In React 19 StrictMode dev (mount → cleanup → mount), the cleanup permanently unset the hook for the rest of the session because bootstrapHydration never ran again — the second StrictMode mount published a new state setter and ref but no arm hook. Programmatic router.back()/forward() calls after the first dev StrictMode cycle silently fell through the shim's optional-chain fallback and isPending flashed idle. Assign the hook from inside BrowserRoot's mount effect alongside the state setter and ref. Mount and cleanup now own the hook's lifecycle as a single unit, so any remount cycle (StrictMode or otherwise) re-establishes it. Drop the duplicate assignment in bootstrapHydration; mount fires before the first user interaction, so there is no observable timing window the previous module-load assignment covered.
…ping Make the runProgrammaticTraversal JSDoc explicit about two design decisions that look like accidents: 1. Skipping the arm when the Navigation API is unavailable (pre-Safari 18.4 / pre-Firefox 136) is intentional. Without canGoBack/canGoForward we cannot detect no-op traversals; arming unconditionally would leave the pending unsettled forever on the first no-op (hanging isPending). Matching Next.js' degraded behaviour (isPending may flash idle mid-traversal) is preferable to a hang. Note: do not "fix" this branch by re-arming. 2. Wrapping the body in React.startTransition even when called outside a useTransition is a deliberate behaviour change vs. pre-PR. The wrap is the only way useTransition().isPending can latch across the popstate task boundary, and matches how push/replace already behave.
waitForAppRouterHydration coerced one half of the AND with Boolean(...) and strict-equaled the other half against `=== true`. The flag is only ever assigned true/false/undefined so both forms work, but the asymmetry reads as intentional and invites a future "consistency fix" that subtly changes the probe semantics. Use Boolean(...) on both halves. In the forward-test setup, replace the terse comment on the bare window.history.back() call with one that names the pollution it avoids: the forward observer log would otherwise capture the back-navigation's pending/idle entries instead of isolating the forward traversal under test.
`router.back()` / `router.forward()` and `router.push()` / `router.replace()` / `router.refresh()` shared one `activePendingBrowserRouterState` slot. An intervening push between a back's arm and its popstate would auto-settle the back's pending and overwrite the slot, so the back's popstate adopted the push's pending. The unconditional setter healed committed tree state, but each transition's `isPending` resolved on the wrong navigation's commit. Split traversal arming into its own FIFO `traversalPendingQueue`. The arm hook now uses `armTraversalPendingBrowserRouterState`, which never touches `activePendingBrowserRouterState`. The popstate path dequeues from this queue instead of reading the shared slot, so each traversal commits only the pending its own arm hook created. Push/replace/refresh continue to use `beginPendingBrowserRouterState` and the active slot unchanged. Settle and resolve helpers also splice the pending out of the queue so a stale-id finally cannot leave a dangling entry. Drop the redundant `__VINEXT_APP_ROUTER_READY__ = false` seed in `bootstrapHydration` since the flag is owned by `BrowserRoot`'s mount effect and the test helper coerces undefined to false. Trim the verbose comments around the arm-hook reassignment and the `applyAction` heal note.
`getBrowserNavigationTraversalHints` used `Reflect.get(window, "navigation")` plus two more `Reflect.get` calls to read `canGoBack` / `canGoForward`. Each returned `any`, so a typo in the property name would compile. Switch to a narrow typed cast so the compiler verifies the property names and the runtime guards (`typeof === "boolean"`) still gate against missing or non-boolean fields on legacy browsers.
Add two E2E specs and the supporting fixture buttons for the per-call traversal pending queue. `router.back(); router.forward();` inside one `startTransition` exercises two history-API calls in opposite directions. Each must adopt only the pending its own arm hook created so the final committed tree matches the final URL (page B2, the forward target). `router.back(); router.push();` inside one `startTransition` exercises the cross-kind overlap that motivated splitting the traversal queue from the programmatic active slot. Without per-call ownership, the back's popstate would hijack the push's pending, the push's transition would resolve on the back's commit, and the committed tree would diverge from the URL.
Both new specs ran a `startTransition(() => { router.back(); router.X(); })`
where X was either `router.forward()` or `router.push(...)`. The test
expectation was that the final committed tree matched the final URL.
In practice Chromium does not deliver popstates one-per-history-API-call
when two traversals (or a traversal plus a push) are issued from the same
task. The browser coalesces or reorders the dispatches, so the second
navigation often never reaches `navigateRsc` and the test lands on the
intermediate page instead of the expected target. Both specs flaked
deterministically in CI for that reason.
The per-call traversal pending queue still does what it was added for —
isolating each traversal's pending from push/replace/refresh — and the
existing four specs cover the documented contract. Drop these two specs
and the supporting buttons rather than leave them red.
The traversal hint probe needed the optional Navigation API fields on Window, but reading them by casting window violated this repo's no-casts rule for production TypeScript. Move the structural shape to a helper parameter and pass window through that boundary. TypeScript can now check the property names without adding a production cast.
NathanDrake2406
force-pushed
the
fix/router-back-forward-ispending
branch
from
fbda719 to
6158155
Compare
|
For Bonk: if LGTM pls formally approve |
|
Not happy w the architecture so I'm visiting this later |
What this changes
router.back()androuter.forward()now keepuseTransition().isPendingtrue from the synchronous call site until the RSC traversal commits. Matches the behavior shipped forrouter.push/replacein #868 and for RSC-level redirects in #870.Why
router.back()androuter.forward()callwindow.history.back/forward()synchronously. The browser queuespopstateas a new task, so by the time it fires the user'sReact.startTransitioncallback has already exited. No setState is tied to the transition,useTransition().isPendingnever latches, and any loading UI flashes to its idle state before the new route renders.Wrapping the popstate handler itself in
startTransitiondoes not help, because that creates a transition unrelated to the one the user opened in their component.Approach
Arm a deferred
PendingBrowserRouterState(a Promise published into the router'suseStateslot) inside the user's transition, then adopt it on the subsequent popstate'snavigateRsc.Three small production changes:
shims/navigation.ts- addrunProgrammaticTraversal(direction). It opens the user'sstartTransition, callswindow.__VINEXT_ARM_TRAVERSAL_PENDING__?.(), thenhistory.back/forward(). Gate the arm onwindow.navigation.canGoBack/canGoForwardso a no-op traversal does not leave an unsettled pending.server/app-browser-entry.ts- register__VINEXT_ARM_TRAVERSAL_PENDING__at hydration time; it invokesbeginPendingBrowserRouterState(), which auto-settles any stale pending, publishes a fresh promise via thesetBrowserRouterStatesetter, and records it asactivePendingBrowserRouterState.navigateRscadoption branch - whenprogrammaticTransitionis false and an active pending exists, adopt it so the commit resolves the promise the user's transition is suspended on. User-initiated popstates (keyboard, gesture, address bar) find no active pending and fall through unchanged.No registry, no id map, no safety timer. The Navigation API's synchronous availability check handles the no-op case; on older browsers without the Navigation API, arming is skipped and
isPendingpreservation degrades to current Next.js behavior (no hang).Non-goals: Pages Router is unaffected; the arm hook is defined only for App Router pages.
Validation
tests/e2e/app-router/nextjs-compat/router-back-forward-pending.spec.ts, four specs using the MutationObserver idle-flash pattern from fix(app-router): keep isPending true across RSC-level redirects #870:router.back()keepsisPendingtrue until page A commits (reproduces the current bug pre-fix with log["b:idle","b:pending","b:idle","__removed__"])router.forward()keepsisPendingtrue until page B commitsrouter.back(); router.back();stays pending through the second hoprouter.back()thenrouter.forward()across separate clicks round-trips cleanlyvp checkclean (format, lint, type-aware).Risks / follow-ups
isPendingpreservation is skipped for traversals. Same behavior as Next.js today on those browsers, no hang.history.statewith a programmatic marker would close this; deliberately out of scope.