fix: cross-route client navigation hangs in Firefox (#652)#690
fix: cross-route client navigation hangs in Firefox (#652)#690Divkix wants to merge 10 commits intocloudflare:mainfrom
Conversation
Replace flushSync-based RSC tree rendering with a two-phase navigation commit that uses startTransition for same-route navigations and synchronous updates for cross-route navigations. This fixes the Firefox hang where startTransition never commits when the entire component tree is replaced, and also resolves the Suspense double-flash from cloudflare#639. Key changes: - navigation.ts: ClientNavigationState on Symbol.for global for module instance safety, render snapshot context for hook consistency during transitions, RSC response snapshot/restore for visited cache, unified navigateClientSide() entry point, history suppression helpers - app-browser-entry.ts: BrowserRoot component with useState-managed tree, NavigationCommitSignal that defers URL commit to useLayoutEffect, visited response cache with LRU eviction, navigation ID counter for stale navigation bailout, manual scroll restoration - link.tsx: remove duplicated helpers, delegate to navigateClientSide - form.tsx: delegate App Router GET navigation to navigateClientSide Closes cloudflare#652
commit: |
There was a problem hiding this comment.
Pull request overview
Fixes a Firefox-specific App Router client-navigation hang by introducing a two-phase navigation commit strategy that uses startTransition only for same-route updates and synchronous updates for cross-route navigations, while deferring URL/history publication until after the new tree commits.
Changes:
- Reworked App Router client navigation flow (snapshot context for hooks, two-phase URL commit, visited response replay cache, and navigation staleness bailout).
- Unified
<Link>and<Form>App Router navigations throughnavigateClientSide()and updated prefetch behavior to snapshot RSC responses for replay. - Added/updated unit + Playwright regression coverage and new fixture routes to validate navigation behavior and hook consistency.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/form.test.ts | Updates window stubs and assertions for the new __VINEXT_RSC_NAVIGATE__ signature and navigation flow. |
| tests/fixtures/app-basic/app/page.tsx | Adds entry links to new navigation regression fixture routes. |
| tests/fixtures/app-basic/app/nav-flash/query-sync/page.tsx | New fixture page for same-route (search param) navigation + Suspense behavior. |
| tests/fixtures/app-basic/app/nav-flash/query-sync/FilterControls.tsx | Client controls validating usePathname/useSearchParams sync during transitions. |
| tests/fixtures/app-basic/app/nav-flash/provider/[id]/page.tsx | New dynamic-param fixture route for cross-route navigation coverage. |
| tests/fixtures/app-basic/app/nav-flash/param-sync/[filter]/page.tsx | New dynamic segment fixture for param-change navigation behavior. |
| tests/fixtures/app-basic/app/nav-flash/param-sync/[filter]/FilterControls.tsx | Client controls validating useParams/usePathname updates on param changes. |
| tests/fixtures/app-basic/app/nav-flash/list/page.tsx | New cross-route destination page used by regression tests. |
| tests/fixtures/app-basic/app/nav-flash/link-sync/page.tsx | New fixture page for link-driven same-route navigation with Suspense. |
| tests/fixtures/app-basic/app/nav-flash/link-sync/FilterLinks.tsx | Client links validating hook snapshot consistency across navigation. |
| tests/e2e/app-router/navigation-regressions.spec.ts | New Playwright regression suite for #652 and related hook/scroll/state behaviors. |
| packages/vinext/src/shims/navigation.ts | Adds navigation global state, render snapshot context, two-phase navigation entry, and prefetch snapshot/consume helpers. |
| packages/vinext/src/shims/link.tsx | Removes duplicated hash/scroll/nav logic and delegates App Router navigation + prefetch to shared helpers. |
| packages/vinext/src/shims/form.tsx | Delegates App Router GET form navigation to navigateClientSide() to align history publishing with commits. |
| packages/vinext/src/server/app-browser-entry.ts | Introduces BrowserRoot state tree management, commit signaling, visited response replay cache, and same-route detection for transition mode. |
| packages/vinext/src/global.d.ts | Extends global typings for updated navigation signature and new prefetch cache entry shape. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function prefetchRscResponse(rscUrl: string, fetchPromise: Promise<Response>): void { | ||
| const cache = getPrefetchCache(); | ||
| const prefetched = getPrefetchedUrls(); | ||
| const now = Date.now(); | ||
|
|
||
| const entry: PrefetchCacheEntry = { timestamp: now }; | ||
|
|
||
| entry.pending = fetchPromise | ||
| .then(async (response) => { | ||
| if (response.ok) { | ||
| entry.snapshot = await snapshotRscResponse(response); | ||
| } else { | ||
| prefetched.delete(rscUrl); | ||
| cache.delete(rscUrl); | ||
| } | ||
| }) | ||
| .catch(() => { | ||
| prefetched.delete(rscUrl); | ||
| cache.delete(rscUrl); | ||
| }) | ||
| .finally(() => { | ||
| entry.pending = undefined; | ||
| }); | ||
|
|
||
| cache.set(rscUrl, entry); | ||
| } |
There was a problem hiding this comment.
prefetchRscResponse() adds entries to the shared prefetch cache without enforcing MAX_PREFETCH_CACHE_SIZE or sweeping expired entries. On link-heavy pages this can cause unbounded growth of both __VINEXT_RSC_PREFETCH_CACHE__ and __VINEXT_RSC_PREFETCHED_URLS__ (successful prefetches are never evicted unless navigated to). Consider reusing the same eviction/TTL sweep logic as storePrefetchResponse() before cache.set() (and deleting from prefetched when evicting).
| export function consumePrefetchResponse(rscUrl: string): CachedRscResponse | null { | ||
| const cache = getPrefetchCache(); | ||
| const entry = cache.get(rscUrl); | ||
| if (!entry) return null; | ||
|
|
||
| // Don't consume pending entries — let the navigation fetch independently. | ||
| if (entry.pending) return null; | ||
|
|
||
| cache.delete(rscUrl); | ||
| getPrefetchedUrls().delete(rscUrl); | ||
|
|
||
| if (entry.snapshot) return entry.snapshot; | ||
|
|
||
| // Legacy: raw Response entries (from storePrefetchResponse) | ||
| // These can't be consumed synchronously as snapshots — skip them. | ||
| // The navigation code will re-fetch. | ||
| return null; | ||
| } |
There was a problem hiding this comment.
consumePrefetchResponse() does not check PREFETCH_CACHE_TTL before returning a cached snapshot. As written, a prefetched RSC payload can be consumed arbitrarily long after it was fetched, which can serve stale UI and defeats the TTL constant/comment above. Suggest: if Date.now() - entry.timestamp >= PREFETCH_CACHE_TTL, delete the entry + prefetched marker and return null.
| function evictVisitedResponseCacheIfNeeded(): void { | ||
| while (visitedResponseCache.size >= MAX_VISITED_RESPONSE_CACHE_SIZE) { | ||
| const oldest = visitedResponseCache.keys().next().value; | ||
| if (oldest === undefined) { | ||
| return; | ||
| } | ||
| visitedResponseCache.delete(oldest); | ||
| } | ||
| } |
There was a problem hiding this comment.
evictVisitedResponseCacheIfNeeded() evicts the oldest inserted entry (Map insertion order), but cache hits in getVisitedResponse() don't refresh recency. This makes the behavior FIFO rather than true LRU as described in the PR summary, and can evict an entry that was just used. If LRU is intended, update the entry's insertion order on read (delete+set) or maintain an explicit recency structure.
…ams eagerly createFromFetch() returns a Thenable (not instanceof Promise), so the async branch was never taken. The raw unresolved Thenable was set as React children, causing suspension without a Suspense boundary and empty content after cross-route navigations. - Duck-type the .then() check to handle both Promises and Thenables - Replace stageClientParams with applyClientParams so useSyncExternalStore subscribers see correct params immediately - Remove dead stageClientParams function and replaceClientParamsWithoutNotify import Fixes useParams E2E failures in CI (hooks.spec.ts:161, hooks.spec.ts:180).
The Flight Thenable from createFromFetch was passed unresolved to renderNavigationPayload/updateBrowserTree. The .then() callback fired unreliably across consecutive navigations, causing empty content. Await createFromFetch in both navigation paths (cached and fresh) so renderNavigationPayload always receives a resolved ReactNode. Add stale navigation checks after the new await points. The HMR handler and server action handler already awaited correctly.
Preserve the X-Vinext-Params header when snapshotting prefetched and visited RSC responses so cached navigations keep dynamic params intact. Stage client params until the URL commit lands so useParams, history state, and redirect handling stay aligned during App Router transitions.
|
/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: Firefox cross-route navigation hang fix (#652)
This is a substantial and well-thought-out rework of the client navigation system. The core insight — that Firefox's startTransition scheduler stalls when replacing the entire RSC tree — is sound, and the two-phase approach (synchronous for cross-route, startTransition for same-route) is a pragmatic solution. The deferred URL commit via useLayoutEffect, the snapshot context for hook consistency, and the navigation ID counter for stale-navigation bailout are all good architectural choices.
The test coverage is thorough: Playwright E2E tests covering cross-route, same-route, back/forward, rapid clicking, hook sync, and round-trip SPA state.
I have a few concerns, mostly around edge cases in the caching layers and one correctness issue with the navigation flow.
Major concerns
-
Double history push for App Router navigations: In
navigateClientSide()(navigation.ts:906-912), the URL is pushed viapushHistoryStateWithoutNotifybefore calling__VINEXT_RSC_NAVIGATE__. Then insidenavigateRsc(app-browser-entry.ts:514),createNavigationCommitEffectis created with the samehistoryUpdateMode, and the commit effect (line 142-156) also pushes/replaces history ifwindow.location.href !== targetHref. SincenavigateClientSidealready pushed, the location should match and the effect becomes a no-op — but this is fragile. If any middleware or redirect changes the URL between these two points, you could get a double push. Consider either removing the early push innavigateClientSidefor RSC navigations and letting the commit effect handle it exclusively, or documenting why both are needed. -
prefetchRscResponse()does not enforce cache size limits: The Copilot review already flagged this.prefetchRscResponse()callscache.set()without any eviction check. On link-heavy pages, this can cause unbounded growth. The oldstorePrefetchResponse()has proper eviction logic — consider reusing it or adding equivalent bounds toprefetchRscResponse(). -
consumePrefetchResponse()ignores TTL: Also flagged by Copilot. A prefetched snapshot can be consumed well past itsPREFETCH_CACHE_TTL. CheckDate.now() - entry.timestamp >= PREFETCH_CACHE_TTLbefore returning the snapshot.
Minor concerns
-
FIFO cache labeled LRU: The PR description says "visited response cache with LRU eviction" but
getVisitedResponse()does not refresh recency on hit. This is FIFO, which is fine for a 50-entry cache, but the PR description should be updated or the cache should be promoted on access (delete+set to move to end of Map insertion order). -
Traversal bypasses TTL on visited cache:
getVisitedResponse()returns cached entries unconditionally for"traverse"navigations (line 182-183). This is intentional for instant back/forward, but means a user could see arbitrarily stale content after going back. Consider whether there should be an upper bound (e.g., 30 minutes) even for traversals. -
navigationSnapshotActiveCountcan leak:activateNavigationSnapshot()increments the counter, but decrement only happens incommitClientNavigationState(). If a navigation throws before reaching commit (e.g., thecreateFromFetchpromise rejects after theactivateNavigationSnapshotcall inrenderNavigationPayload), the counter increments but never decrements. The catch block at line 604 callscommitClientNavigationState()which decrements, but only if the error is in the outernavigateRsctry block. Errors insiderenderNavigationPayload→updateBrowserTree(async promise chain) could leak. -
BrowserRootsets module-level state in render:setBrowserTreeState = setTreeStateat line 247 runs during every render ofBrowserRoot. This is a side effect during render, which React Strict Mode may call twice. It works in practice because the setter is stable, but it's worth noting this pattern is fragile and could break under concurrent features.
Nits
| entry.pending = undefined; | ||
| }); | ||
|
|
||
| cache.set(rscUrl, entry); |
There was a problem hiding this comment.
prefetchRscResponse() adds entries without enforcing MAX_PREFETCH_CACHE_SIZE. On link-heavy pages (hundreds of Link components), the prefetch cache can grow unbounded. The old storePrefetchResponse() has eviction logic — consider adding the same sweep+FIFO eviction here before cache.set().
Also, getPrefetchedUrls() entries for successfully-prefetched URLs are never evicted unless the URL is navigated to via consumePrefetchResponse(). If the user never visits those routes, they accumulate forever.
| cache.delete(rscUrl); | ||
| getPrefetchedUrls().delete(rscUrl); | ||
|
|
||
| if (entry.snapshot) return entry.snapshot; |
There was a problem hiding this comment.
This returns the snapshot without checking TTL. A prefetched entry that's 5 minutes old will still be consumed here, serving stale UI. Consider:
| if (entry.snapshot) return entry.snapshot; | |
| if (entry.snapshot) { | |
| if (Date.now() - entry.timestamp >= PREFETCH_CACHE_TTL) { | |
| // Expired — discard and let navigation re-fetch | |
| return null; | |
| } | |
| return entry.snapshot; | |
| } |
| } | ||
| visitedResponseCache.delete(oldest); | ||
| } | ||
| } |
There was a problem hiding this comment.
PR summary says "LRU eviction" but this is FIFO — getVisitedResponse() doesn't refresh insertion order on cache hits. For true LRU, add visitedResponseCache.delete(rscUrl) + visitedResponseCache.set(rscUrl, cached) in getVisitedResponse() when returning a hit. Or just update the PR description to say FIFO — it's perfectly fine for a 50-entry cache.
| if (typeof window.__VINEXT_RSC_NAVIGATE__ === "function") { | ||
| await window.__VINEXT_RSC_NAVIGATE__(fullHref); | ||
| if (mode === "replace") { |
There was a problem hiding this comment.
For App Router RSC navigations, this pushes the URL into history before calling __VINEXT_RSC_NAVIGATE__, and then createNavigationCommitEffect inside navigateRsc will also attempt to push/replace history (guarded by window.location.href !== targetHref). The guard makes the second push a no-op in the normal case, but the intent would be clearer if navigateClientSide skipped the early push for RSC navigations and let the commit effect own it exclusively. The current code works but is fragile — a redirect could cause the URLs to diverge between the two push sites.
| navigationSnapshot: initialNavigationSnapshot, | ||
| }); | ||
|
|
||
| setBrowserTreeState = setTreeState; |
There was a problem hiding this comment.
Setting module-level state (setBrowserTreeState) during render is a side effect that React Strict Mode or concurrent rendering could call multiple times. This works because setTreeState is referentially stable, but it would be more robust to use a useEffect or useRef pattern:
| setBrowserTreeState = setTreeState; | |
| const setterRef = useRef(setTreeState); | |
| setterRef.current = setTreeState; | |
| useEffect(() => { | |
| setBrowserTreeState = setterRef.current; | |
| }); |
Not blocking, but worth considering for future React versions.
| pendingNavigationCommits.set(renderId, resolve); | ||
| }); | ||
|
|
||
| activateNavigationSnapshot(); |
There was a problem hiding this comment.
If the promise chain in updateBrowserTree rejects (e.g., createFromFetch fails after the snapshot is activated), activateNavigationSnapshot() has already incremented the counter but commitClientNavigationState() may never be called for this navigation, causing navigationSnapshotActiveCount to leak. The hooks would then permanently prefer stale snapshot values over useSyncExternalStore.
Consider wrapping renderNavigationPayload with a .catch() that calls commitClientNavigationState() to ensure the counter is always balanced.
| return null; | ||
| } | ||
|
|
||
| if (navigationKind === "traverse") { |
There was a problem hiding this comment.
Traversal ("traverse") returns cached entries regardless of expiresAt. This means back/forward can show arbitrarily old content if the user waits long enough. This is probably the right tradeoff for instant back/forward UX, but consider whether a generous upper bound (e.g., 30 minutes) would be appropriate to avoid very stale pages.
| * mounts, so its pre-paint effect never fires. By draining all effects | ||
| * <= the committed renderId here, the winning transition cleans up after | ||
| * any superseded ones, keeping the counter balanced. | ||
| */ | ||
| function drainPrePaintEffects(upToRenderId: number): void { |
There was a problem hiding this comment.
Good design: draining all effects ≤ the committed renderId handles the superseded-transition cleanup. One subtlety: for superseded renderIds (id < upToRenderId), you call commitClientNavigationState() which decrements navigationSnapshotActiveCount. This is correct since each was activated by renderNavigationPayload. Worth a brief inline comment noting that the 1:1 activate/commit pairing is maintained here.
| */ | ||
| export function activateNavigationSnapshot(): void { | ||
| const state = getClientNavigationState(); | ||
| if (state) state.navigationSnapshotActiveCount++; |
There was a problem hiding this comment.
This only increments; decrement happens in commitClientNavigationState(). The 1:1 pairing relies on drainPrePaintEffects cleaning up superseded navigations, and the catch block in navigateRsc calling commitClientNavigationState(). If you add error handling in renderNavigationPayload per my other comment, this becomes more robust. Consider adding a doc comment noting the pairing invariant.
| } else { | ||
| pushHistoryStateWithoutNotify(null, "", fullHref); | ||
| } | ||
| await window.__VINEXT_RSC_NAVIGATE__(fullHref, 0, "navigate", mode); |
There was a problem hiding this comment.
Nit: the historyUpdateMode parameter is only meaningful for the commit effect inside navigateRsc. Since navigateClientSide already pushed/replaced the URL at lines 907-911 (before calling this), passing mode here is only needed so the commit effect knows to push again if the URL was changed by a redirect. This dual-purpose is confusing — a comment explaining why both the caller and callee touch history would help future readers.
|
The review has been submitted successfully. Here's a summary of the feedback I posted: Major concerns (3):
Minor concerns (4): Inline comments (10) with specific code suggestions on the affected lines. |
…ment - Remove double history push for RSC navigations: let the commit effect in navigateRsc own push/replace exclusively. Also fixes a hidden bug where the early push broke isSameRoute detection (always true), negating the Firefox cross-route hang fix. - Enforce prefetch cache size limits in prefetchRscResponse() by extracting shared eviction logic from storePrefetchResponse(). - Add TTL check in consumePrefetchResponse() so expired snapshots are discarded instead of served as stale UI. - Prevent navigationSnapshotActiveCount leak when async payloads reject in updateBrowserTree by adding error handlers that balance the activate/commit pairing. - Move BrowserRoot setBrowserTreeState assignment from render to useLayoutEffect to avoid side effects under React Strict Mode. - Make visited response cache true LRU (delete+set on hit) and add a 30-minute upper bound for traversal cache hits to prevent arbitrarily stale back/forward content.
|
This is the same PR as mine... #643 |
ah, didn't realize it. I'll close this one |
Resolved conflict in app-browser-entry.ts by integrating both branches: - Kept PR cloudflare#690's navigation caching system (BrowserRoot, visitedResponseCache, etc.) - Applied cloudflare#700's fix: removed await before createFromReadableStream for proper hydration timing - Used React 19 'use' hook to resolve the Thenable in BrowserRoot component - Fixed eslint warning by passing children as createElement third argument Conflicts resolved: - packages/vinext/src/server/app-browser-entry.ts Refs: cloudflare#690, cloudflare#652, cloudflare#700
Resolved conflict in app-browser-entry.ts by integrating both branches: - Kept PR cloudflare#690's navigation caching system (BrowserRoot, visitedResponseCache, etc.) - Applied cloudflare#700's fix: removed await before createFromReadableStream for proper hydration timing - Used React 19 'use' hook to resolve the Thenable in BrowserRoot component - Fixed eslint warning by passing children as createElement third argument Conflicts resolved: - packages/vinext/src/server/app-browser-entry.ts Refs: cloudflare#690, cloudflare#652, cloudflare#700 Co-authored-by: Nathan Nguyen <146415969+NathanDrake2406@users.noreply.github.com>
Divkix
force-pushed
the
fix/firefox-navigation-hang
branch
from
b73d850 to
c36caba
Compare
Resolved conflict in app-browser-entry.ts by integrating both branches: - Kept PR cloudflare#690's navigation caching system (BrowserRoot, visitedResponseCache, etc.) - Applied cloudflare#700's fix: removed await before createFromReadableStream for proper hydration timing - Used React 19 'use' hook to resolve the Thenable in BrowserRoot component - Fixed eslint warning by passing children as createElement third argument Conflicts resolved: - packages/vinext/src/server/app-browser-entry.ts Refs: cloudflare#690, cloudflare#652, cloudflare#700 Co-authored-by: Nathan Nguyen <146415969+NathanDrake2406@users.noreply.github.com>
Divkix
force-pushed
the
fix/firefox-navigation-hang
branch
from
c36caba to
0d5b843
Compare
|
/bigbonk review |
There was a problem hiding this comment.
Review: Firefox cross-route navigation hang fix
Note: This PR has been identified as a duplicate of #643 and the author has indicated they will close it. Providing a review anyway for the record, since the approach and code quality are worth discussing.
Architecture assessment
The core approach is sound — splitting navigation into same-route (startTransition) vs cross-route (synchronous) is a pragmatic workaround for the Firefox scheduler issue. The deferred URL commit via useLayoutEffect, the snapshot context for hook consistency during transitions, and the navigation ID counter for stale-navigation bailout are all well-designed.
The Symbol.for global for ClientNavigationState is the right call for surviving multiple Vite module instances.
The test coverage is thorough — the Playwright E2E suite covers the critical regression paths and the fixture pages are well-structured.
Issues
Beyond what the previous reviews flagged (which I largely agree with), I have several additional concerns:
1. Dual scroll restoration for popstate is confusing
There are now two popstate handlers that deal with scroll restoration:
navigation.ts:1255— callsrestoreScrollPosition(event.state)for Pages Router (guarded by__VINEXT_RSC_NAVIGATE__check)app-browser-entry.ts:655— callsrestorePopstateScrollPosition(event.state)for App Router
Both functions do the same thing (read __vinext_scrollX/__vinext_scrollY from state) but with different deferral strategies. The navigation.ts version uses a microtask chain to wait for __VINEXT_RSC_PENDING__, while the browser-entry version fires after the RSC navigation settles. This split is reasonable but the duplication is confusing — consider extracting a shared utility or at least cross-referencing with comments.
2. getPathnameSnapshot / getSearchParamsSnapshot lost their live-update behavior
The old snapshot functions actively read from window.location and updated the cached value. The new versions just return the cached value from ClientNavigationState. The cached values are only updated in syncCommittedUrlStateFromLocation() which is called by commitClientNavigationState(). This means if external code calls history.pushState while suppressUrlNotifyCount > 0 (shouldn't happen normally, but third-party libraries might), the hooks won't see the update until the next commit. This is probably fine in practice but is a subtle behavioral change worth noting.
3. updateBrowserTree for server actions doesn't activate/commit navigation snapshot
Server action results call updateBrowserTree directly (lines 488, 501) without going through renderNavigationPayload. This means activateNavigationSnapshot is never called, so the navigationSnapshotActiveCount stays at 0, and hooks will use useSyncExternalStore values. This is correct for the current server action flow (which stays on the same URL), but if server actions ever trigger URL changes, the hooks would show stale URLs during the transition.
4. resolveCommittedNavigations iterates during mutation
At app-browser-entry.ts:222, the function iterates over pendingNavigationCommits and deletes entries during iteration. This works in practice because Map iteration is defined to handle concurrent deletion, but it would be clearer to collect keys first or use the drainPrePaintEffects pattern (which also iterates-and-deletes but is at least commented).
5. isSameRoute comparison doesn't account for basePath
At app-browser-entry.ts:552, isSameRoute compares url.pathname === window.location.pathname. If basePath is set, both sides include the basePath prefix so this still works. But if navigating from a bare URL to a basePath-prefixed URL (edge case), the comparison could give wrong results. The stripBasePath utility is available but not used here.
6. Missing historyUpdateMode propagation for redirect chains
At app-browser-entry.ts:607, when a redirect is detected, the recursive call passes navigationKind but drops historyUpdateMode. This means redirect chains always get undefined for historyUpdateMode, which makes the commit effect a no-op for history (relying on the replaceHistoryStateWithoutNotify at line 599). This works because the redirect already replaced the URL, but if the redirect target itself redirects, the final URL update relies entirely on the replaceHistoryStateWithoutNotify before the recursive call, not on the commit effect. This is fragile — the intent would be clearer if historyUpdateMode were explicitly set to undefined (or "replace") in the recursive call.
Overall
This is a substantial and well-structured rework. The two-phase navigation model is a sound architectural decision. The main concerns are around edge cases in the caching layers (already flagged by previous reviewers) and some fragility in the history/scroll state management. Given that this is a duplicate of #643, the valuable parts of this implementation should be compared with that PR to see if either has better handling of the edge cases noted above.
| // so React keeps the old UI visible during the transition. For cross-route | ||
| // navigations (different pathname), use synchronous updates — React's | ||
| // startTransition hangs in Firefox when replacing the entire tree. | ||
| const isSameRoute = url.pathname === window.location.pathname; |
There was a problem hiding this comment.
isSameRoute compares raw url.pathname against window.location.pathname. This works when basePath is consistent, but consider using stripBasePath on both sides for robustness:
| const isSameRoute = url.pathname === window.location.pathname; | |
| const isSameRoute = stripBasePath(url.pathname, __basePath) === stripBasePath(window.location.pathname, __basePath); |
(Would need to import stripBasePath and __basePath — not blocking, but worth considering for basePath edge cases.)
| } | ||
|
|
||
| return navigate(destinationPath, redirectDepth + 1); | ||
| return navigate(destinationPath, redirectDepth + 1, navigationKind); |
There was a problem hiding this comment.
The recursive redirect call drops historyUpdateMode, relying on the replaceHistoryStateWithoutNotify at line 599 to have already set the URL. This is intentional (redirects always replace) but non-obvious. Consider adding a comment:
| return navigate(destinationPath, redirectDepth + 1, navigationKind); | |
| return navigate(destinationPath, redirectDepth + 1, navigationKind); | |
| // historyUpdateMode intentionally omitted — the replaceHistoryStateWithoutNotify | |
| // above already set the URL, and the commit effect will be a no-op. |
Or alternatively, explicitly pass "replace" so the commit effect handles it consistently.
|
|
||
| if (isServerActionResult(result)) { | ||
| getReactRoot().render(result.root); | ||
| updateBrowserTree( |
There was a problem hiding this comment.
Server action results call updateBrowserTree directly without activateNavigationSnapshot() / commitClientNavigationState() pairing. This means the navigation snapshot context won't be active during the server action render. Currently fine because server actions stay on the same URL, but if this ever changes (e.g., server action with redirect()), hooks would show stale values during the transition. Consider adding a comment noting the intentional omission.
| window.__VINEXT_RSC_NAVIGATE__?.(window.location.href, 0, "traverse") ?? Promise.resolve(); | ||
| window.__VINEXT_RSC_PENDING__ = pendingNavigation; | ||
| void pendingNavigation.finally(() => { | ||
| restorePopstateScrollPosition(event.state); |
There was a problem hiding this comment.
The scroll restoration here (restorePopstateScrollPosition) fires after the RSC navigation promise settles. But navigation.ts also has a restoreScrollPosition function in its popstate handler (guarded by !__VINEXT_RSC_NAVIGATE__ so they don't both fire). The two functions do the same thing with different names and different deferral strategies. Consider cross-referencing them with a comment or extracting a shared scrollToSavedPosition utility to reduce confusion.
| const state = getClientNavigationState(); | ||
| if (!state) return; | ||
|
|
||
| state.navigationSnapshotActiveCount = Math.max(0, state.navigationSnapshotActiveCount - 1); |
There was a problem hiding this comment.
Math.max(0, ...) is a good defensive floor, but it masks bugs — if the counter goes negative, something is wrong with the activate/commit pairing. Consider adding a dev-mode warning:
| state.navigationSnapshotActiveCount = Math.max(0, state.navigationSnapshotActiveCount - 1); | |
| state.navigationSnapshotActiveCount = Math.max(0, state.navigationSnapshotActiveCount - 1); | |
| if (import.meta.env?.DEV && state.navigationSnapshotActiveCount < 0) { | |
| console.warn('[vinext] navigationSnapshotActiveCount went negative — activate/commit pairing mismatch'); | |
| } |
(The warning would need to go before the Math.max clamp to be useful.)
| * <= the committed renderId here, the winning transition cleans up after | ||
| * any superseded ones, keeping the counter balanced. | ||
| */ | ||
| function drainPrePaintEffects(upToRenderId: number): void { |
There was a problem hiding this comment.
Good design: draining all effects ≤ the committed renderId handles the superseded-transition cleanup correctly. The 1:1 activate/commit pairing is maintained because each superseded navigation gets a commitClientNavigationState() call. A brief inline comment explaining why non-winning effects get commitClientNavigationState() instead of their original effect would help future readers understand the invariant.
| const state = getClientNavigationState(); | ||
| if (!state) return; | ||
| for (const fn of state.listeners) fn(); | ||
| } |
There was a problem hiding this comment.
The initialization of ClientNavigationState captures originalPushState and originalReplaceState as bound methods. If another library patches history.pushState before this module loads (e.g., analytics libraries, router libraries), the "original" captured here is actually the patched version, and calling it via state.originalPushState.call(...) would invoke the other library's wrapper. This is the same risk as the old code, but worth noting in a comment since the Symbol.for global makes the timing of initialization less obvious.
|
The review has been submitted successfully. Here's a summary of the feedback I posted: Review submitted on PR #690 with 7 inline comments and a detailed summary. Key points:
|
Fixes the following review issues from ask-bonk: Major fixes: - Fix prefetch cache eviction order to add entry first, then evict (prevents temporary overflow past MAX_PREFETCH_CACHE_SIZE) - Add error handling in renderNavigationPayload to ensure navigationSnapshotActiveCount is decremented on synchronous errors, preventing counter leaks Minor fixes: - Add cross-reference comments between Pages Router and App Router scroll restoration handlers (navigation.ts and app-browser-entry.ts) - Add JSDoc to getPathnameSnapshot/getSearchParamsSnapshot explaining cached behavior vs live window.location reads - Add comment documenting server action navigation flow (doesn't use activateNavigationSnapshot because it stays on same URL) - Add comment explaining Map iteration safety in resolveCommittedNavigations - Use stripBasePath in isSameRoute comparison for robust basePath handling - Explicitly pass undefined for historyUpdateMode in redirect chains with explanatory comment (prevents double URL push) All 1094 tests pass.
|
/bigbonk review |
There was a problem hiding this comment.
Review: Firefox cross-route navigation hang fix
Status: This PR has been identified as a duplicate of #643 and the author has indicated they will close it. Reviewing for the record since the implementation is substantive and the discussion is valuable.
Overall assessment
The two-phase navigation model (synchronous for cross-route, startTransition for same-route) is a sound fix for the Firefox scheduler issue. The architecture is well-designed: Symbol.for global state, navigation ID counter for stale bailout, deferred URL commit via useLayoutEffect, and render snapshot context for hook consistency during transitions.
The E2E test coverage is good — covers the critical regression paths including cross-route, same-route, back/forward, rapid clicking, hook sync, and round-trip SPA state.
Previous reviews from bonk raised valid concerns. I note that several of them have been addressed in the latest revision (TTL check in consumePrefetchResponse, LRU promotion in getVisitedResponse, MAX_TRAVERSAL_CACHE_TTL bound on traversals, handleAsyncError for the activate/commit counter leak, useLayoutEffect for setBrowserTreeState, prefetch eviction via shared evictPrefetchCacheIfNeeded). Good follow-through.
Remaining concerns
1. Stale navigation bailout leaves orphaned state on cancellation
When navId !== activeNavigationId causes an early return (lines 597, 624, 664, 671 in app-browser-entry.ts), the function returns without calling commitClientNavigationState(). But params were already staged via stageClientParams() (line 589 or 653). These pending params and the hasPendingNavigationUpdate flag remain set on ClientNavigationState and are never cleaned up — they'll be committed by the next navigation, potentially applying stale params from the cancelled navigation.
The visited response cache also stores a snapshot from the cancelled navigation (line 666) before the navId check at line 671, so a stale snapshot can end up in the cache.
2. evictPrefetchCacheIfNeeded is called after cache.set — newly-added entry could be immediately evicted
In prefetchRscResponse (navigation.ts:384-386), the entry is added to the cache first, then eviction runs. If the cache was already at capacity, evictPrefetchCacheIfNeeded evicts the oldest entry — which could be a different still-pending entry. But if the new entry happens to be the oldest (it was just inserted, so it's at the end of Map insertion order, so this can't happen)... actually this ordering is fine since evictPrefetchCacheIfNeeded takes FIFO from the front. Ignore this point.
3. isHashOnlyChange in navigateClientSide reads window.location.href which hasn't been updated yet
At navigation.ts:956, isHashOnlyChange(fullHref) compares against window.location.href. For RSC navigations, the URL hasn't been pushed to history yet at this point (the commit effect will do it later). This is correct because we want to compare against the current URL to determine if only the hash changed. However, during rapid navigations where the previous navigation's commit effect hasn't fired yet, window.location.href still reflects the URL from two navigations ago, not the previous navigation's URL. This could cause a hash-only navigation to be misidentified as a non-hash navigation.
4. NavigationCommitSignal cleanup function cancels requestAnimationFrame but doesn't call resolveCommittedNavigations
At app-browser-entry.ts:252-254, the cleanup function cancels the requestAnimationFrame but never resolves the pending commit promise. If React unmounts this component (e.g., during a rapid navigation that supersedes the current one), pendingNavigationCommits will hold unresolved promises. The navigateRsc function awaits renderNavigationPayload which returns this promise — so the awaiting navigation hangs forever.
The drainPrePaintEffects at line 246 handles superseded transitions for the winning render, but if the NavigationCommitSignal itself is unmounted before its useLayoutEffect fires (React decides not to commit this render), neither drainPrePaintEffects nor resolveCommittedNavigations runs for this renderId. The next successful navigation's drainPrePaintEffects would clean up the pre-paint effects, but the commit promise (in pendingNavigationCommits) for the cancelled renderId is never resolved.
Actually — looking more carefully, drainPrePaintEffects(upToRenderId) does iterate pendingNavigationPrePaintEffects and calls commitClientNavigationState() for superseded IDs, which handles the counter. And resolveCommittedNavigations(renderId) resolves all pending commits with id <= renderId. So the winning render does clean up after superseded ones. The concern is only if NO render ever commits (e.g., the component tree is unmounted entirely), in which case both promises and effects leak. This is an extreme edge case.
5. Scroll handling after RSC navigation has a subtle race
In navigateClientSide (navigation.ts:993-999), scroll-to-top happens after await __VINEXT_RSC_NAVIGATE__() returns. But __VINEXT_RSC_NAVIGATE__ resolves when resolveCommittedNavigations fires inside a requestAnimationFrame. The scroll at line 997 then runs synchronously after the rAF-resolved promise, which means it runs in the next microtask after the rAF callback — potentially before the browser has actually painted the new content. In practice this is probably fine since the DOM update from React's commit phase has already happened, but it's worth noting.
| const cachedPayload = await createFromFetch<ReactNode>( | ||
| Promise.resolve(restoreRscResponse(cachedRoute.response)), | ||
| ); | ||
| if (navId !== activeNavigationId) return; |
There was a problem hiding this comment.
When a stale navigation is detected here, the function returns without undoing the stageClientParams(cachedRoute.params) call at line 589. The pending params remain on ClientNavigationState and will be committed by the next navigation's commitClientNavigationState() call — potentially applying params from this cancelled navigation to a completely different route.
Consider resetting pending params on early return:
| if (navId !== activeNavigationId) return; | |
| if (navId !== activeNavigationId) { | |
| // Clean up staged params from this cancelled navigation | |
| replaceClientParamsWithoutNotify(latestClientParams); | |
| return; | |
| } |
(Same concern applies to the other navId checks at lines 664 and 671.)
| flushSync(() => { | ||
| getReactRoot().render(rscPayload as ReactNode); | ||
| }); | ||
| storeVisitedResponseSnapshot(rscUrl, responseSnapshot, navParams); |
There was a problem hiding this comment.
This stores the response snapshot in the visited cache before the stale-navigation check at line 671. If this navigation is stale, a potentially incorrect snapshot (fetched for a navigation that was superseded) ends up in the cache and could be served on a future back/forward traversal.
Consider moving the storeVisitedResponseSnapshot call to after the final navId check, or guarding it:
| storeVisitedResponseSnapshot(rscUrl, responseSnapshot, navParams); | |
| if (navId !== activeNavigationId) return; | |
| storeVisitedResponseSnapshot(rscUrl, responseSnapshot, navParams); | |
| const rscPayload = await createFromFetch<ReactNode>( |
| const frame = requestAnimationFrame(() => { | ||
| resolveCommittedNavigations(renderId); | ||
| }); | ||
|
|
||
| return () => { | ||
| cancelAnimationFrame(frame); | ||
| }; |
There was a problem hiding this comment.
If React unmounts this component without the useLayoutEffect having fired (or during cleanup), the cancelAnimationFrame prevents resolveCommittedNavigations from running, but the pending promise in pendingNavigationCommits for this renderId is never resolved. The caller (navigateRsc) awaits this promise via renderNavigationPayload, so it would hang.
In practice, the next successful navigation's resolveCommittedNavigations resolves all id <= upToRenderId, which covers this. But if the component tree is torn down entirely (e.g., full page navigation triggered by the catch block), the promise leaks.
Consider resolving the promise in the cleanup:
| const frame = requestAnimationFrame(() => { | |
| resolveCommittedNavigations(renderId); | |
| }); | |
| return () => { | |
| cancelAnimationFrame(frame); | |
| }; | |
| const frame = requestAnimationFrame(() => { | |
| resolveCommittedNavigations(renderId); | |
| }); | |
| return () => { | |
| cancelAnimationFrame(frame); | |
| // Resolve any pending commit promises to prevent hanging callers | |
| resolveCommittedNavigations(renderId); | |
| }; |
| const state = getClientNavigationState(); | ||
| if (!state) return; | ||
|
|
||
| state.navigationSnapshotActiveCount = Math.max(0, state.navigationSnapshotActiveCount - 1); |
There was a problem hiding this comment.
Previous review suggested a dev-mode warning before the Math.max clamp. I'd echo that — the clamp silently hides pairing bugs. A console.warn in dev mode before the clamp would make debugging much easier:
| state.navigationSnapshotActiveCount = Math.max(0, state.navigationSnapshotActiveCount - 1); | |
| if (import.meta.env?.DEV && state.navigationSnapshotActiveCount <= 0) { | |
| console.warn('[vinext] navigationSnapshotActiveCount already at 0 — possible activate/commit pairing mismatch'); | |
| } | |
| state.navigationSnapshotActiveCount = Math.max(0, state.navigationSnapshotActiveCount - 1); |
| @@ -585,11 +956,11 @@ async function navigateImpl( | |||
| if (isHashOnlyChange(fullHref)) { | |||
There was a problem hiding this comment.
Minor: isHashOnlyChange compares against window.location.href, which during rapid RSC navigations might still reflect the pre-previous URL (the preceding navigation's commit effect hasn't fired yet). This could cause a hash-only change to be misidentified as a full navigation, triggering an unnecessary RSC fetch. Low-severity since the fetch would still succeed and produce the correct result.
| clientParamsJson: "{}", | ||
| pendingClientParams: null, | ||
| pendingClientParamsJson: null, | ||
| originalPushState: window.history.pushState.bind(window.history), |
There was a problem hiding this comment.
Previous review noted this captures whatever history.pushState/replaceState is currently on the prototype, which could be a third-party library's patched version. The comment about Symbol.for making initialization timing less obvious is worth noting here:
| originalPushState: window.history.pushState.bind(window.history), | |
| // NB: If a third-party library (analytics, router) has already patched | |
| // history methods, these capture the patched versions, not the native ones. | |
| // This is the same behavior as the old code, but the Symbol.for global | |
| // makes the initialization timing less obvious — the first module instance | |
| // to load wins. | |
| originalPushState: window.history.pushState.bind(window.history), | |
| originalReplaceState: window.history.replaceState.bind(window.history), |
| // The URL has already been updated via replaceHistoryStateWithoutNotify above, | ||
| // so the recursive navigation should NOT push/replace again. Pass undefined | ||
| // for historyUpdateMode to make the commit effect a no-op for history updates. | ||
| return navigate(destinationPath, redirectDepth + 1, navigationKind, undefined); |
There was a problem hiding this comment.
Good improvement from the previous revision — the comment clearly explains why historyUpdateMode is undefined for the recursive redirect call. This was flagged as confusing in earlier reviews and the comment resolves the concern.
|
The review has been submitted successfully. Here's a summary of my feedback: Review posted on PR #690 with 7 inline comments and a detailed summary. Key findings:
|
Addresses review feedback on PR cloudflare#690: - Move stageClientParams() to after final navId check in both cached and fresh paths - Build navigationSnapshot from local params instead of latestClientParams - Move storeVisitedResponseSnapshot after final stale check - Add doc comment about history method patching behavior This fixes a race where cancelled navigations could leak their staged params into subsequent unrelated navigations when commitClientNavigationState() was called later without clearing the pending state. Test: Add E2E test for cancelled slow navigation param leak scenario
…support Integrate main branch changes (instrumentation-client imports, notifyAppRouterTransitionStart, __VINEXT_HYDRATED_AT) into the Firefox navigation fix branch while preserving BrowserRoot, NavigationCommitSignal, and navigateClientSide refactoring.
The refactor to delegate Link navigation to navigateClientSide() left a duplicate notifyAppRouterTransitionStart() call in link.tsx, causing the callback to fire twice per push navigation.
|
/bigbonk review |
There was a problem hiding this comment.
Review: Firefox cross-route navigation hang fix
Context: This PR is a duplicate of #643 and the author has indicated they will close it. Reviewing for the record since the implementation has substantive design decisions worth documenting.
What works well
The core architecture is sound. The two-phase navigation model — startTransition for same-route, synchronous for cross-route — is a clean workaround for the Firefox scheduler issue. Several design decisions are well-executed:
Symbol.forglobal state correctly survives multiple Vite module instances- Navigation ID counter for stale-navigation bailout is the right approach for rapid-click deduplication
- Deferred URL commit via
useLayoutEffectensures hooks see the pending URL during transitions drainPrePaintEffectscorrectly cleans up after supersededstartTransitionrenders, maintaining the 1:1 activate/commit pairing- Previous review feedback addressed: TTL checks in
consumePrefetchResponse, LRU promotion via delete+set,MAX_TRAVERSAL_CACHE_TTL,handleAsyncErrorfor counter leak,useLayoutEffectforsetBrowserTreeState, sharedevictPrefetchCacheIfNeeded - History ownership clarified: The comment at lines 986-989 of
navigation.tsclearly documents thatnavigateClientSidedoes NOT push history for RSC navigations — the commit effect owns it exclusively. This addresses the double-push concern from earlier reviews.
The E2E test coverage is thorough — the cancelled-navigation param leak test (navigation-regressions.spec.ts:189-226) is particularly valuable.
Remaining issues
1. NavigationCommitSignal cleanup leaves unresolved promises
When the cleanup at line 254 fires, cancelAnimationFrame prevents resolveCommittedNavigations from running, but the promise in pendingNavigationCommits is never resolved. The next winning render's resolveCommittedNavigations(upToRenderId) covers the normal case, but if the component tree is torn down entirely, the promise leaks. In practice the catch block does window.location.href = href which discards the JS context, so this is moot — but resolving in cleanup would be strictly more correct.
2. Stale visited cache entries from superseded navigations
storeVisitedResponseSnapshot at line 668 runs between two navId checks (lines 665 and 673). A snapshot from a superseded navigation can end up in the visited cache and be served on a future back/forward.
3. Prefetch eviction only removes one entry
evictPrefetchCacheIfNeeded only evicts one FIFO entry after the TTL sweep (navigation.ts:298-305). Compare with evictVisitedResponseCacheIfNeeded which uses a while loop. If router.prefetch() is called in a tight loop, the prefetch cache can temporarily exceed MAX_PREFETCH_CACHE_SIZE.
4. isHashOnlyChange race during rapid navigations
During rapid RSC navigations where the previous commit effect hasn't fired yet, window.location.href still reflects two navigations ago. A hash-only change could be misidentified as a full navigation, triggering an unnecessary RSC fetch. Low severity — the fetch succeeds and produces the correct result.
Summary
This is solid work that addresses a real Firefox regression with a well-designed architecture. The major concerns from previous reviews (double history push, prefetch cache bounds, TTL checks) have been addressed. The remaining issues are edge cases, not blockers. Since this is a duplicate of #643, the implementation details should be compared between the two PRs to pick the best handling of each edge case.
| }); | ||
|
|
||
| return () => { | ||
| cancelAnimationFrame(frame); |
There was a problem hiding this comment.
This cleanup cancels the requestAnimationFrame but doesn't resolve the pending promise in pendingNavigationCommits. If the component tree is torn down entirely (e.g., error boundary triggers full-page nav), the navigateRsc caller hangs on the unresolved promise. In practice, window.location.href = href in the catch block discards the JS context, making this moot. But for robustness:
| cancelAnimationFrame(frame); | |
| return () => { | |
| cancelAnimationFrame(frame); | |
| // Resolve pending commits to prevent callers from hanging if React | |
| // unmounts this component without committing (e.g., error boundary). | |
| resolveCommittedNavigations(renderId); | |
| }; |
| const state = getClientNavigationState(); | ||
| if (!state) return; | ||
|
|
||
| state.navigationSnapshotActiveCount = Math.max(0, state.navigationSnapshotActiveCount - 1); |
There was a problem hiding this comment.
The Math.max(0, ...) clamp silently hides activate/commit pairing bugs. Previous reviews have suggested a dev-mode warning before the clamp — I'd echo that. Silent counter bugs are notoriously hard to track down:
| state.navigationSnapshotActiveCount = Math.max(0, state.navigationSnapshotActiveCount - 1); | |
| if (import.meta.env?.DEV && state.navigationSnapshotActiveCount <= 0) { | |
| console.warn('[vinext] navigationSnapshotActiveCount already at 0 — possible activate/commit pairing mismatch'); | |
| } | |
| state.navigationSnapshotActiveCount = Math.max(0, state.navigationSnapshotActiveCount - 1); |
| } | ||
|
|
||
| // FIFO fallback if still at capacity after sweep | ||
| if (cache.size >= MAX_PREFETCH_CACHE_SIZE) { |
There was a problem hiding this comment.
After the TTL sweep, only one entry is evicted via FIFO. If prefetchRscResponse is called in rapid succession (e.g., router.prefetch() in a loop on a link-heavy page), the cache can temporarily exceed MAX_PREFETCH_CACHE_SIZE. Compare with evictVisitedResponseCacheIfNeeded in app-browser-entry.ts which uses a while loop. Consider matching the pattern:
| if (cache.size >= MAX_PREFETCH_CACHE_SIZE) { | |
| while (cache.size >= MAX_PREFETCH_CACHE_SIZE) { | |
| const oldest = cache.keys().next().value; | |
| if (oldest !== undefined) { | |
| cache.delete(oldest); | |
| prefetched.delete(oldest); | |
| } else { | |
| break; | |
| } | |
| } |
| getReactRoot().render(rscPayload as ReactNode); | ||
| }); | ||
| // Store visited response only for navigations that haven't been superseded | ||
| storeVisitedResponseSnapshot(rscUrl, responseSnapshot, navParams); |
There was a problem hiding this comment.
This stores the response snapshot in the visited cache between two navId checks (line 665 and line 673). If this navigation is stale at line 673, a snapshot from the superseded navigation ends up in the cache and could be served on a future back/forward traversal.
Consider moving storeVisitedResponseSnapshot below the final navId check:
| storeVisitedResponseSnapshot(rscUrl, responseSnapshot, navParams); | |
| stageClientParams(navParams); | |
| storeVisitedResponseSnapshot(rscUrl, responseSnapshot, navParams); |
(i.e., move the storeVisitedResponseSnapshot call from line 668 to after line 676, grouped with stageClientParams.)
| // to properly activate/commit the navigation snapshot. | ||
| if (isServerActionResult(result)) { | ||
| getReactRoot().render(result.root); | ||
| updateBrowserTree( |
There was a problem hiding this comment.
Good comment explaining the intentional omission of activateNavigationSnapshot / commitClientNavigationState pairing for server actions. The assumption (server actions stay on the same URL, redirects go through window.location.href) is correct today. The comment is well-placed for future maintainers.
| // It coordinates scroll restoration with the pending RSC navigation. | ||
| // Pages Router scroll restoration is handled in navigation.ts with | ||
| // microtask-based deferral for compatibility with non-RSC navigation. | ||
| window.addEventListener("popstate", (event) => { |
There was a problem hiding this comment.
This popstate handler and the one in navigation.ts:1289 both deal with scroll restoration but with different deferral strategies. They don't conflict because the navigation.ts one is guarded by typeof window.__VINEXT_RSC_NAVIGATE__ !== 'function'. The comment at lines 694-697 explains the split clearly.
Consider adding a reciprocal comment in navigation.ts:1289 pointing back here — future maintainers may find one without the other.
Also, note the asymmetry: the navigation.ts handler calls commitClientNavigationState() for Pages Router popstate events, while this handler relies on navigateRsc's commit effect to handle it. This is correct (RSC navigation owns the commit lifecycle) but worth a brief inline note.
| originalPushState: window.history.pushState.bind(window.history), | ||
| originalReplaceState: window.history.replaceState.bind(window.history), | ||
| patchInstalled: false, | ||
| hasPendingNavigationUpdate: false, |
There was a problem hiding this comment.
Good addition of the comment about capturing potentially-patched history methods. The Symbol.for global means the first module instance to load wins — this is the right tradeoff (consistent with the old code's behavior) and is now properly documented.
|
The review was submitted successfully with 7 inline comments and a detailed summary. Here's what the review covers: Acknowledged strengths:
Inline comments with suggestions:
|
Summary
Fixes #652 —
startTransitionnever commits in Firefox when the entire RSC component tree is replaced during cross-route navigation. The old page stays visible indefinitely in production builds on workerd.Root cause: vinext replaces the full RSC tree on every cross-route navigation inside
startTransition. The React transition scheduler cannot finalize this in Firefox. The previousflushSyncapproach also caused the Suspense double-flash from #639.Fix: Two-phase navigation commit with same-route detection:
startTransitionfor smooth incremental updatesuseLayoutEffectso hooks see the pending URL during transitionsKey changes
navigation.ts:ClientNavigationStateonSymbol.forglobal (survives multiple module instances), render snapshot context for hook consistency during transitions, RSC response snapshot/restore for visited cache, unifiednavigateClientSide()entry point, history suppression helpersapp-browser-entry.ts:BrowserRootcomponent withuseState-managed tree,NavigationCommitSignalthat defers URL commit touseLayoutEffect, visited response cache with LRU eviction (50 entries, 5min TTL), navigation ID counter for stale navigation bailout, manual scroll restorationlink.tsx: removed duplicatedisHashOnlyChange/scrollToHash, delegates tonavigateClientSideform.tsx: App Router GET forms delegate tonavigateClientSideglobal.d.ts: extended__VINEXT_RSC_NAVIGATE__signature withnavigationKindandhistoryUpdateModeEdge cases handled
location.assign)Symbol.forglobal state)Test plan
vp checkpasses (only pre-existing benchmark errors remain)tests/shims.test.ts— 742 tests passtests/form.test.ts— updated assertions for new navigate signature, all passtests/prefetch-cache.test.ts— compatible with newPrefetchCacheEntrytype, all passtests/link.test.ts— all passtests/routing.test.ts— all passtests/app-router.test.ts— all passtests/entry-templates.test.ts— all passtests/e2e/app-router/navigation-regressions.spec.tscovering:usePathname/useSearchParams/useParamshook sync