fix(app-router): hash RSC navigation queries#2355
Conversation
|
/bigbonk review for issues |
|
Temporarily closing to retrigger the pull_request reopened event because GitHub did not dispatch checks on replacement creation. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
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_HEADERto the cache-busting input ensures_rscis 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 oftest/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/computePreviousLegacyRscCacheBustingSearchParamexclude the new state header (and render mode), andresolveInvalidRscCacheBustingRequestadds those toacceptedHashesonly on navigation render mode. An old client that omitsX-Vinext-Rsc-Statestill 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_HEADERis added toVINEXT_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-Treereserved. vinext never sets that header on requests (only reads it into the hash if present), so introducing a vinext-namespacedX-Vinext-Rsc-Stateinstead of populating the reserved Next.js header is the right separation. - Prefetch/navigation cache-key parity. Both
link.tsx/navigation.tsprefetch (viagetRscStateFingerprint()→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. createAppRscStateFingerprintis type-correct and serialization-safe.AppElementsSlotBindingis a plain serializable record, and the fingerprint value is computed once on the client and transmitted, so there's no cross-environmentJSON.stringifykey-ordering dependency. The optional-chainedgetRscStateFingerprint?.()plus runtime type guard keep it backward-compatible with older runtime bootstraps.
Verification performed
vitest runontests/app-rsc-state-fingerprint.test.ts+tests/app-rsc-cache-busting.test.ts→ 36 passedtsc --noEmiton the package → no errors in any touched file- Confirmed the e2e import path (
../../helpersfromtests/e2e/app-router/nextjs-compat/) and port4174match sibling specs - Confirmed the upstream Next.js test at v16.2.6 matches the ported structure
Minor (non-blocking) observations
- The e2e
requestlisteners are attached afterwaitForAppRouterHydrationand before the click, which is correct ordering. Both specs assert positional indices (requests[0]/[1]); sinceprefetch={false}is set on the fixture Links, there should be no stray prefetch_rscrequests racing ahead of the click-driven ones. Worth keeping an eye on E2E flakiness here, but the design is sound. app-rsc-state-fingerprint.tslives inserver/but is consumed by the browser entry/shims; that matches the existing convention forapp-rsc-cache-busting.ts, so no change needed.
LGTM.
|
Review posted to PR #2355. SummaryI reviewed PR #2355 ( What I verified:
Local checks run:
The only minor (non-blocking) note was potential E2E positional-index sensitivity, mitigated by the fixtures' |
|
Refreshing the PR head after GitHub failed to associate remediation commit 88d25e6. |
commit: |
|
|
/bigbonk review for issues |
|
Refreshing the PR head after the bounded-fingerprint remediation. |
There was a problem hiding this comment.
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.6test/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-Stateslot intocreateCacheBustingInputchanges the current hash for every request, including old clients that don't send the header (normalizeHeaderValue(null)→"0", so an extra,0lands mid-string). This is correctly absorbed by the rolling-upgrade path:computePreviousRscCacheBustingSearchParam/computePreviousLegacyRscCacheBustingSearchParamspread the state slot as[](notnull), 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 inapp-rsc-cache-busting.test.tsstill pass, confirming the old hash is reproduced byte-for-byte.resolveInvalidRscCacheBustingRequestonly 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 viarouterStateAtNavStart(app-browser-entry.ts:1569/1609) and prefetch/Link/Form/router.prefetch viaapplyNavigationRuntimeRscState→getRscStateFingerprint()→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
getRscStateFingerprintis registered, while navigation sets it unconditionally. In the App Router these can't diverge: Link/Form prefetch is gated onhasAppNavigationRuntime(), and the runtime function is registered in the sameregisterNavigationRuntimeFunctionscall that exposesnavigate, so by the time a prefetch can fire the fingerprint is available. -
Serialization is deterministic.
createAppRscStateFingerprintuses a fixed-key object literal;layoutIds/slotBindingsare 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_HEADERis added toVINEXT_RSC_VARY_HEADERand the exported-Vary assertion was updated to match. -
Next-Router-State-Treestays reserved. Using a vinext-namespacedX-Vinext-Rsc-Stateinstead 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.tsis fine —server/headers.tsis 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/interceptionContextare already captured at prefetch time. No correctness impact. -
E2E positional-index reliance. Both specs assert
requests[0]/[1]. The fixtures setprefetch={false}, so no stray prefetch_rscrequests 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.
|
Review posted to PR #2355. SummaryI independently reviewed PR #2355 ( Key verification:
Local checks: 38 unit tests passed, Non-blocking notes: prefetch-time vs nav-time fingerprint drift (graceful cache-miss degradation), and E2E positional-index reliance (mitigated by |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
Summary
_rscURLs are non-empty and state-aware_rscquery through config redirects and rewritesNext-Router-State-Treesemantically reserved by using a vinext-specific varying headerImpact
Fixes both failures from
test/e2e/app-dir/rsc-query-routing/rsc-query-routing.test.tsin run 28143992598.Validation
vp checkon touched filesReplaces #2354, whose GitHub PR object became pinned to an obsolete branch SHA.