fix(app-router): skip RSC navigation for hash-only history traversal#1270
Conversation
App Router popstate currently sends every history traversal through __VINEXT_RSC_NAVIGATE__, even when the target pathname and search already match the committed route and only the hash entry changed. That violates the hash-only traversal invariant and replays cached RSC payloads unnecessarily.\n\nCompare popstate targets against the committed App Router pathname and search before dispatching an RSC traversal. Same-route entries now restore scroll directly, while real route traversals keep the existing pending-navigation coordination.\n\nAdd an App Router E2E regression that wraps __VINEXT_RSC_NAVIGATE__ and verifies hash back/forward traversal does not call it.
commit: |
|
/bigbonk review |
There was a problem hiding this comment.
Clean, well-scoped fix. The implementation correctly identifies hash-only popstate traversals by comparing the target pathname/search against the committed App Router snapshot, and short-circuits RSC navigation while still restoring scroll. Two minor observations below — neither blocking.
| const target = new URL(href, window.location.origin); | ||
| const routerState = getBrowserRouterState(); | ||
| const targetPathname = stripBasePath(target.pathname, __basePath); | ||
| const targetSearch = new URLSearchParams(target.search).toString(); |
There was a problem hiding this comment.
Nit: new URLSearchParams(target.search).toString() round-trips the search string through URLSearchParams normalization (sorts nothing, but does strip a leading ? and re-encode). Since the committed snapshot's searchParams was also constructed via new ReadonlyURLSearchParams(url.search) (in createClientNavigationRenderSnapshot), both sides go through the same normalization, so this works correctly.
If you wanted to shave the allocation, target.search (which includes the ?) could be compared against a pre-stored raw search string, but the URLSearchParams approach is more robust against encoding differences, so this is fine as-is.
| window.addEventListener("popstate", (event) => { | ||
| notifyAppRouterTransitionStart(window.location.href, "traverse"); | ||
| const href = window.location.href; | ||
| notifyAppRouterTransitionStart(href, "traverse"); |
There was a problem hiding this comment.
Worth noting: notifyAppRouterTransitionStart still fires on the early-return path (before the isSameAppRoutePopstateTarget check). I verified there's no corresponding transitionEnd callback, so this is fine — it's fire-and-forget instrumentation. Just calling it out for future readers: if a transition-end hook is ever added, the early-return path will need to emit it too.
| await expect(page).toHaveURL(`${BASE}/nextjs-compat/hash-popstate-scroll#content`); | ||
| await expect(page.locator("#content")).toBeInViewport(); | ||
|
|
||
| await expect |
There was a problem hiding this comment.
Good use of expect.poll here — it handles the case where the assertion runs before the popstate handler has fully completed. The goBack/goForward awaits above ensure the URL has changed, but expect.poll makes the RSC-call-count assertion robust against any remaining async work.
|
Review submitted. Here's a summary of my findings: Approved. The PR is clean and well-scoped. What it does: Adds Key verification points:
|
Overview
popstatewhen the target pathname and search already match the committed router state, then restore scroll directly.packages/vinext/src/server/app-browser-entry.ts,tests/e2e/app-router/nextjs-compat/hash-popstate-scroll.spec.ts__VINEXT_RSC_NAVIGATE__or replays cached RSC payloads.Why
For App Router correctness, a history traversal should only enter the RSC navigation lifecycle when the route identity changes. Hash-only entries preserve the same pathname and search, so the browser should update location and scroll without asking React Flight to rebuild the route tree.
onlyHashChange.What changed
/page#section-ato/page#section-bvia browser buttonspopstatealways called__VINEXT_RSC_NAVIGATE__.Maintainer review path
packages/vinext/src/server/app-browser-entry.tsisSameAppRoutePopstateTarget()and thepopstatebranch. The key question is whether committed pathname/search is the right App Router boundary for skipping RSC work.tests/e2e/app-router/nextjs-compat/hash-popstate-scroll.spec.tswindow.__VINEXT_RSC_NAVIGATE__and verifies hash back/forward traversal keeps the call count at zero while preserving URL and scroll behavior.Validation
vp run vinext#buildPLAYWRIGHT_PROJECT=app-router pnpm run test:e2e -- tests/e2e/app-router/nextjs-compat/hash-popstate-scroll.spec.tsvp test run tests/app-browser-entry.test.tsvp checkThe new E2E failed before the implementation with 2 RSC navigation calls, one for back and one for forward.
Risk / compatibility
popstatework only when the target pathname/search matches the committed App Router route snapshot.Non-goals
popstatebehavior.pushState/replaceStateentries.References
onlyHashChangescroll={false}interaction.