Skip to content

fix(app-router): hash RSC navigation queries#2355

Open
james-elicx wants to merge 7 commits into
mainfrom
codex/fix-rsc-query-routing-28143992598
Open

fix(app-router): hash RSC navigation queries#2355
james-elicx wants to merge 7 commits into
mainfrom
codex/fix-rsc-query-routing-28143992598

Conversation

@james-elicx

Copy link
Copy Markdown
Member

Summary

  • attach a stable visible-router-state variant to App Router RSC navigation requests
  • use the same variant for Link and router prefetches so _rsc URLs are non-empty and state-aware
  • preserve the hashed _rsc query through config redirects and rewrites
  • keep Next-Router-State-Tree semantically reserved by using a vinext-specific varying header
  • retain rolling-upgrade compatibility for prior RSC URL hashes

Impact

Fixes both failures from test/e2e/app-dir/rsc-query-routing/rsc-query-routing.test.ts in run 28143992598.

Validation

  • focused vp check on touched files
  • focused RSC identity unit tests — 36 passed
  • vinext browser parity fixture — 2 passed
  • targeted Next.js v16.2.6 deploy suite, one worker — 2 passed

Replaces #2354, whose GitHub PR object became pinned to an obsolete branch SHA.

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@james-elicx

Copy link
Copy Markdown
Member Author

Temporarily closing to retrigger the pull_request reopened event because GitHub did not dispatch checks on replacement creation.

@james-elicx james-elicx reopened this Jun 25, 2026
@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk ask-bonk Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: fix(app-router): hash RSC navigation queries

I reviewed the full diff, the rolling-upgrade compatibility path, the Vary-header wiring, dev/prod parity, and the faithfulness of the ported Next.js test. This looks solid and ready to merge. No blocking issues.

What works well

  • The core fix is correct. Adding VINEXT_RSC_STATE_HEADER to the cache-busting input ensures _rsc is non-empty and state-aware, so the URL survives config redirects/rewrites (createRscRedirectLocation / the rewrite path already forward a non-empty _rsc). The faithful port of test/e2e/app-dir/rsc-query-routing (v16.2.6) is actually stricter than upstream — it asserts ?_rsc= immediately follows the path rather than just .toContain('/source') — which directly pins the bug being fixed.
  • Rolling-upgrade compatibility is handled correctly. computePreviousRscCacheBustingSearchParam / computePreviousLegacyRscCacheBustingSearchParam exclude the new state header (and render mode), and resolveInvalidRscCacheBustingRequest adds those to acceptedHashes only on navigation render mode. An old client that omits X-Vinext-Rsc-State still validates against the previous hash, so in-flight requests across a deploy won't 307-loop. The legacy/SHA pair stays symmetric.
  • CDN correctness preserved. VINEXT_RSC_STATE_HEADER is added to VINEXT_RSC_VARY_HEADER, so all RSC responses advertise the new varying header — important on Cloudflare. The exported-Vary assertion test was updated to match.
  • Good call keeping Next-Router-State-Tree reserved. vinext never sets that header on requests (only reads it into the hash if present), so introducing a vinext-namespaced X-Vinext-Rsc-State instead of populating the reserved Next.js header is the right separation.
  • Prefetch/navigation cache-key parity. Both link.tsx/navigation.ts prefetch (via getRscStateFingerprint()getBrowserRouterState()) and the navigation path (routerStateAtNavStart = getBrowserRouterState()) fingerprint the current visible state, so a prefetched URL matches the later navigation URL. The shell-prefetch path also carries the same fingerprint.
  • createAppRscStateFingerprint is type-correct and serialization-safe. AppElementsSlotBinding is a plain serializable record, and the fingerprint value is computed once on the client and transmitted, so there's no cross-environment JSON.stringify key-ordering dependency. The optional-chained getRscStateFingerprint?.() plus runtime type guard keep it backward-compatible with older runtime bootstraps.

Verification performed

  • vitest run on tests/app-rsc-state-fingerprint.test.ts + tests/app-rsc-cache-busting.test.ts → 36 passed
  • tsc --noEmit on the package → no errors in any touched file
  • Confirmed the e2e import path (../../helpers from tests/e2e/app-router/nextjs-compat/) and port 4174 match sibling specs
  • Confirmed the upstream Next.js test at v16.2.6 matches the ported structure

Minor (non-blocking) observations

  • The e2e request listeners are attached after waitForAppRouterHydration and before the click, which is correct ordering. Both specs assert positional indices (requests[0]/[1]); since prefetch={false} is set on the fixture Links, there should be no stray prefetch _rsc requests racing ahead of the click-driven ones. Worth keeping an eye on E2E flakiness here, but the design is sound.
  • app-rsc-state-fingerprint.ts lives in server/ but is consumed by the browser entry/shims; that matches the existing convention for app-rsc-cache-busting.ts, so no change needed.

LGTM.

@ask-bonk

ask-bonk Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Review posted to PR #2355.

Summary

I reviewed PR #2355 (fix(app-router): hash RSC navigation queries) thoroughly and found no blocking issues — it's solid and ready to merge.

What I verified:

  • Core fix correctness — adding X-Vinext-Rsc-State to the cache-busting input makes _rsc non-empty and state-aware, so it survives config redirects/rewrites.
  • Rolling-upgrade compatibility — the "previous" hash functions correctly exclude the new state header, and resolveInvalidRscCacheBustingRequest accepts them on navigation render mode, preventing 307 loops during a deploy.
  • CDN correctnessVINEXT_RSC_STATE_HEADER was added to VINEXT_RSC_VARY_HEADER, with the exported-Vary test updated.
  • Prefetch/navigation parity — both paths fingerprint the current getBrowserRouterState(), so prefetch and navigation cache keys match.
  • Test port fidelity — confirmed against the upstream Next.js v16.2.6 test; the vinext port is actually stricter (asserts ?_rsc= directly follows the path).
  • Type/serialization safetycreateAppRscStateFingerprint is type-correct, AppElementsSlotBinding is plain-serializable, and the optional runtime function keeps backward compatibility.

Local checks run:

  • 36 unit tests passed (fingerprint + cache-busting)
  • tsc --noEmit clean on all touched files
  • Verified e2e import paths and port match sibling specs

The only minor (non-blocking) note was potential E2E positional-index sensitivity, mitigated by the fixtures' prefetch={false}.

github run

@james-elicx

Copy link
Copy Markdown
Member Author

Refreshing the PR head after GitHub failed to associate remediation commit 88d25e6.

@james-elicx james-elicx reopened this Jun 25, 2026
@pkg-pr-new

pkg-pr-new Bot commented Jun 25, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@2355
npm i https://pkg.pr.new/vinext@2355

commit: 88d25e6

@github-actions

Copy link
Copy Markdown
Contributor
Example Preview Production Original
app-router-cloudflare preview production
pages-router-cloudflare preview production
app-router-playground preview production original
realworld-api-rest preview production
nextra-docs-template preview production
benchmarks preview production
hackernews preview production original
workers-cache preview production
web preview production

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@james-elicx

Copy link
Copy Markdown
Member Author

Refreshing the PR head after the bounded-fingerprint remediation.

@james-elicx james-elicx reopened this Jun 25, 2026

@ask-bonk ask-bonk Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: fix(app-router): hash RSC navigation queries

I independently re-reviewed the full diff focusing on cache-key correctness, rolling-upgrade safety, prefetch/navigation parity, test-port fidelity, and dev/prod parity. No blocking issues — this is correct and ready to merge. Notes below for the record.

Verification performed

  • 38 unit tests passed (tests/app-rsc-cache-busting.test.ts, tests/app-rsc-state-fingerprint.test.ts, tests/navigation-runtime-rsc-state.test.ts).
  • tsc --noEmit -p packages/vinext/tsconfig.json → clean.
  • Confirmed the e2e port against upstream vercel/next.js@v16.2.6 test/e2e/app-dir/rsc-query-routing/rsc-query-routing.test.ts — the vinext port is faithful and slightly stricter (asserts ?_rsc= directly follows the path).

What I verified holds up

  • Hash format change is backward-compatible. Inserting the X-Vinext-Rsc-State slot into createCacheBustingInput changes the current hash for every request, including old clients that don't send the header (normalizeHeaderValue(null)"0", so an extra ,0 lands mid-string). This is correctly absorbed by the rolling-upgrade path: computePreviousRscCacheBustingSearchParam / computePreviousLegacyRscCacheBustingSearchParam spread the state slot as [] (not null), so they reproduce the exact pre-PR field layout ([prefetch, segPrefetch, stateTree, nextUrl, interception, mountedSlots]). The pre-existing "0,0,0,0,0,slot:modal:/" assertions in app-rsc-cache-busting.test.ts still pass, confirming the old hash is reproduced byte-for-byte. resolveInvalidRscCacheBustingRequest only adds these previous hashes on navigation render mode, so an in-flight old-client request validates instead of 307-looping across a deploy.

  • Prefetch ↔ navigation cache-key parity. Both sides fingerprint getBrowserRouterState(): the navigation path via routerStateAtNavStart (app-browser-entry.ts:1569/1609) and prefetch/Link/Form/router.prefetch via applyNavigationRuntimeRscStategetRscStateFingerprint()createAppRscStateFingerprint(getBrowserRouterState()) (app-browser-entry.ts:2045). Same source, same serializer.

  • The conditional-vs-unconditional header set is safe. Prefetch paths set the header only if getRscStateFingerprint is registered, while navigation sets it unconditionally. In the App Router these can't diverge: Link/Form prefetch is gated on hasAppNavigationRuntime(), and the runtime function is registered in the same registerNavigationRuntimeFunctions call that exposes navigate, so by the time a prefetch can fire the fingerprint is available.

  • Serialization is deterministic. createAppRscStateFingerprint uses a fixed-key object literal; layoutIds/slotBindings are ordered arrays and the value is computed once per client and transmitted opaquely (the server never re-serializes it for hashing), so there's no cross-environment key-ordering hazard.

  • CDN/Vary correctness. VINEXT_RSC_STATE_HEADER is added to VINEXT_RSC_VARY_HEADER and the exported-Vary assertion was updated to match.

  • Next-Router-State-Tree stays reserved. Using a vinext-namespaced X-Vinext-Rsc-State instead of populating the Next.js header keeps the protocol header read-only, which is the right call.

  • Importing the header constant into client/navigation-runtime.ts is fineserver/headers.ts is pure constants with no heavy imports, so no server runtime is pulled into the client graph.

Minor (non-blocking) observations

  • Prefetch-time vs nav-time fingerprint drift. Prefetch fingerprints visible state when the Link/Form scrolls into view; navigation fingerprints it at click time. If the visible router state changes in between (slot update, search-param change without navigation), the prefetched payload simply misses the cache and refetches — graceful degradation, and consistent with how mountedSlotsHeader/interceptionContext are already captured at prefetch time. No correctness impact.

  • E2E positional-index reliance. Both specs assert requests[0]/[1]. The fixtures set prefetch={false}, so no stray prefetch _rsc requests should race ahead of the click-driven ones — this is what makes the positional assertions deterministic (and is a reasonable improvement over upstream's default-prefetch fixture). Worth keeping an eye on for E2E flakiness, but the design is sound.

LGTM.

@ask-bonk

ask-bonk Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Review posted to PR #2355.

Summary

I independently reviewed PR #2355 (fix(app-router): hash RSC navigation queries) and found no blocking issues — it's correct and ready to merge.

Key verification:

  • Backward-compatible hash change — The most important detail: adding the X-Vinext-Rsc-State slot changes the _rsc hash for every request (even old clients, since normalizeHeaderValue(null)"0" inserts an extra ,0 mid-string). This is correctly absorbed by the rolling-upgrade path, which spreads the state slot as [] (not null) to reproduce the exact pre-PR field layout. The pre-existing "0,0,0,0,0,slot:modal:/" test assertions still pass, proving byte-for-byte reproduction of the old hash.
  • Prefetch ↔ navigation parity — both fingerprint getBrowserRouterState() via the same serializer.
  • Conditional-vs-unconditional header set — safe because App Router prefetch is gated on hasAppNavigationRuntime(), registered in the same call that exposes the fingerprint function.
  • Test port fidelity — confirmed against upstream vercel/next.js@v16.2.6; the vinext port is faithful and slightly stricter.

Local checks: 38 unit tests passed, tsc --noEmit clean.

Non-blocking notes: prefetch-time vs nav-time fingerprint drift (graceful cache-miss degradation), and E2E positional-index reliance (mitigated by prefetch={false} fixtures).

github run

@github-actions

Copy link
Copy Markdown
Contributor

Performance benchmarks

Compared 2896bb7 against base de014d4 using alternating same-runner rounds. Next.js was unchanged and skipped.

0 improved · 0 regressed · 6 within ±1.5%

Scenario Framework Baseline Current Change
Client bundle size (gzip) vinext 117.1 KB 117.2 KB ⚫ +0.1%
Client entry size (gzip) vinext 112.5 KB 112.6 KB ⚫ +0.1%
Dev server cold start vinext 2.59 s 2.58 s ⚫ -0.5%
Production build time vinext 3.12 s 3.13 s ⚫ +0.3%
RSC entry closure size (gzip) vinext 93.0 KB 93.1 KB ⚫ +0.0%
Server bundle size (gzip) vinext 156.9 KB 157.0 KB ⚫ +0.1%

View detailed results and traces

🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head

@james-elicx james-elicx marked this pull request as ready for review June 25, 2026 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant