fix(app-router): refetch same-page search navigations - #1744
Conversation
Same-page App Router navigations with a changed search string could replay a prefetched RSC payload instead of requesting the target payload. That diverges from Next.js, which refetches page segments for same-page navigations because search params are page inputs. Bypass visited and prefetched RSC caches for navigate operations whose pathname is unchanged but search params differ. Add a prerendered production browser regression that ports the Next.js hash navigation request assertion.
commit: |
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the same-page search refetch + _rsc hiding changes. The core logic is sound and matches repo conventions well:
isSamePageSearchNavigationmirrors the existingisSameAppRoutePopstateTargetcomparison (stripBasePathpathname +searchParams.toString()), and gating onnavigationKind === "navigate"correctly covers bothrouter.pushandrouter.replacesince push/replace are distinguished byhistoryUpdateMode, notnavigationKind.requestWithoutRscCacheBustingSearchParamcorrectly runs afterresolveInvalidRscCacheBustingRequest, so cache-busting validation still sees the real request before the strip. PassinguserlandRequestto both middleware andbuildPostMwRequestContextis consistent — post-middleware rewrite has/missing matching readsurl.searchParams, which should not see_rsc.useSearchParams()parity was already handled by the existingstripRscCacheBustingSearchParam(url)at line 540, so this change only closes the middleware gap. Good targeting.
A few minor, non-blocking observations below. None of them block merge.
(2) Optimistic-shell path (app-browser-entry.ts ~line 1911): the optimistic-shell branch (!navResponse && navigationKind === "navigate") is not gated by shouldBypassNavigationCache, so a same-page search change can still render an optimistic shell from cached route templates before the real fetch commits. This is a detached commit superseded by the authoritative fetch (same as cross-route navs), so likely harmless, but worth confirming it doesn't briefly paint stale page-segment content for the changed-search case. A one-line comment noting the optimistic shell is intentionally allowed here would help future readers.
(3) Guard vs stripper matching (app-rsc-handler.ts line 346): the short-circuit uses url.searchParams.has(...) (decoded-key match), while stripRscCacheBustingSearchParam -> isRscCacheBustingSearchPair does its own decodeURIComponent/+-aware matching. For the normal _rsc=<hash> shape these agree and vinext always appends the param itself, so it's fine in practice — just flagging the two different matching strategies in case encoded-key edge cases ever diverge.
| path.resolve(process.cwd(), "node_modules"), | ||
| path.join(fixtureRoot, "node_modules"), | ||
| "junction", | ||
| ); |
There was a problem hiding this comment.
This symlinks the repo-root node_modules (process.cwd()/node_modules), whereas the sibling production-fixture specs (client-reference-runtime-map.browser.spec.ts, inline-css.browser.spec.ts) use a linkFixtureNodeModules helper that symlinks tests/fixtures/app-basic/node_modules per-entry. The fixture-scoped variant pins the same next/react resolution the rest of the suite uses; the root-level link can resolve a different copy if hoisting differs. Consider reusing the established helper pattern for consistency and to avoid resolution drift. Not a correctness blocker, just parity with existing specs.
|
|
||
| await checkLink("query-param", 2284); | ||
| await expect(page).toHaveURL( | ||
| `${app.baseUrl}/nextjs-compat/hash-rsc-requests?with-query-param#hash-160`, |
There was a problem hiding this comment.
The test verifies scroll offsets and the final URL but never asserts that the navigation didn't trip the fixture middleware (which returns HTTP 599 if _rsc leaks). A 599 on the RSC fetch would hard-navigate rather than soft-navigate, so the scroll assertions catch it only indirectly. Consider adding an explicit assertion that no response had status 599 (e.g., track page.on("response") for the with-query-param RSC request and assert response.ok()), so a regression in the _rsc-hiding logic fails with a clear message instead of a confusing scroll-offset mismatch.
|
Review posted successfully. Here's a summary of my review of PR #1744: Review SummaryThe PR is well-implemented and follows repo conventions. The core logic is sound: Verified correct:
Minor, non-blocking observations posted as inline comments:
No blockers found. The fix is well-targeted and matches Next.js behavior. |
- Reuse fixture-scoped node_modules linking in the hash RSC spec instead of symlinking repo-root node_modules, matching sibling production specs. - Add an explicit assertion that no request leaked `_rsc` to the fixture middleware (HTTP 599) for a clear failure mode. - Document that the optimistic shell is intentionally not gated by shouldBypassNavigationCache (detached commit, always superseded). - Detect the RSC cache-busting param via the encoding-aware stripper so the guard and stripper can never diverge on encoded-key edge cases.
|
/bigbonk re-review for issues |
There was a problem hiding this comment.
Re-review (/bigbonk). Verified the source changes against the repo's middleware/rewrite execution path and the App Router client navigation snapshot model. The fix is well-targeted and correct, and the earlier review concerns are either already addressed or based on a stale premise.
Verified correct:
-
requestWithoutRscCacheBustingSearchParam+userlandRequestthreading is complete._rscvalidation still runs on the realrequestatresolveInvalidRscCacheBustingRequest(line 451-455) before the strip. The strippeduserlandRequestis then passed to bothapplyAppMiddleware(line 478) andbuildPostMwRequestContext(line 497). I confirmed the post-middleware rewrite has/missing matching readsqueryexclusively from the requestContext (app-post-middleware-context.ts:22->url.searchParams), andmatchRewriteusesoptions.requestContext, notoptions.request(applyRewriteat line 292-297). So_rsccannot leak into middleware or config-rewrite condition matching. The only remaining use of the originalrequestin the rewrite path isproxyExternalRequestfor external rewrites, where preserving the on-the-wire URL is reasonable. -
isSamePageSearchNavigationis consistent withisSameAppRoutePopstateTarget. Both comparestripBasePath(pathname)+ normalizedsearchParams.toString(). The snapshot'spathnameis already basePath-stripped (createClientNavigationRenderSnapshotinnavigation.ts:1299) andsearchParamsis aReadonlyURLSearchParams, so.toString()normalization matches on both sides. Returningtrueonly when pathname matches AND search differs correctly leaves hash-only and identical-URL navigations on the cached path. -
The
request.body ? request.clone() : requestguard is correct for GET RSC navigations (no body) and POST action requests (clone to avoid consuming the stream).
On the prior review's open items:
- The
linkFixtureNodeModuleshelper in the new spec is byte-identical to the one inclient-reference-runtime-map.browser.spec.ts(lines 22-37), and both resolvetests/fixtures/app-basic/node_modules. There is no resolution drift versus the established pattern — this concern appears to have been based on an earlier revision. No action needed. - The explicit no-599/
_rsc-leak assertion is now present (middlewareLeakResponsesviapage.on("response"), assertedtoEqual([])). Addressed. - The optimistic-shell-not-gated behavior is now documented with an accurate comment; the shell is a detached commit superseded by the authoritative fetch, and the bypass path still forces a real fetch. No stale page-segment persistence.
Minor, non-blocking: requestWithoutRscCacheBustingSearchParam decides whether to rebuild by comparing url.search before/after stripRscCacheBustingSearchParam. Because the stripper reserializes the whole query (split/filter/join), a query with empty pairs (e.g. ?a=1&&b=2) would compare unequal even without _rsc and trigger a harmless rebuild that also normalizes the middleware-facing URL. This is a degenerate edge case and doesn't affect correctness, but the comment's claim that the guard and stripper "can never diverge" is about matching intent rather than byte-identical search strings — worth a one-word note if you revisit.
No blockers. LGTM.
|
Re-review posted. Summary of findings: Review Result: LGTM, no blockersThe PR is well-implemented and correct. I verified the two core changes against the repo's actual execution path: 1.
2. Same-page search refetch (
On the prior One minor non-blocking note posted: the before/after |
Replace the before/after url.search comparison in requestWithoutRscCacheBustingSearchParam with a dedicated hasRscCacheBustingSearchParam helper that reuses the stripper's isRscCacheBustingSearchPair matcher. This makes the guard and stripper share a single source of truth and avoids spuriously rebuilding/normalizing requests whose only difference is degenerate empty query pairs (e.g. ?a=1&&b=2).
|
/bigbonk re-review for issues |
1 similar comment
|
/bigbonk re-review for issues |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk re-review for issues |
There was a problem hiding this comment.
Re-review (/bigbonk). The core logic is correct and matches Next.js behavior — I re-verified both changes against the repo's execution path:
isSamePageSearchNavigationis consistent with the existingisSameAppRoutePopstateTarget(app-browser-entry.ts:1264): both comparestripBasePath(pathname)+ normalizedsearchParams.toString(), and the snapshot'spathnameis already basePath-stripped (navigation.ts:1299) withsearchParamsaReadonlyURLSearchParams. Returningtrueonly when pathname matches AND search differs correctly leaves hash-only / identical-URL navigations on the cached path._rschiding: validation still runs on the realrequestatresolveInvalidRscCacheBustingRequest(line 453) before the strip;userlandRequestis threaded toapplyAppMiddlewareandbuildPostMwRequestContext.matchRewritereads has/missing fromrequestContext(userland-derived), and the only remaining use of the originalrequestinapplyRewriteisproxyExternalRequest, so_rsccannot leak into middleware or config-rewrite matching.- Prior review concern about the spec's node_modules symlink is not valid —
linkFixtureNodeModuleshere is byte-identical toclient-reference-runtime-map.browser.spec.ts:22and resolves the same fixture-scoped path. No resolution drift.
Two new non-blocking findings on requestWithoutRscCacheBustingSearchParam, both edge cases that don't affect the normal GET-navigation flow this PR targets. Details inline.
|
|
||
| stripRscCacheBustingSearchParam(url); | ||
| const source = request.body ? request.clone() : request; | ||
| return new Request(url.toString(), source); |
There was a problem hiding this comment.
Drops the Workers cf property (and risks throwing on bodied requests). This is the only request-reconstruction site in the codebase that uses a bare new Request(url, source) instead of the established cloneRequestWithHeaders helper (request-pipeline.ts:651), which exists precisely because new Request() does not copy the Workers-specific cf metadata and re-attaches it explicitly.
Two concrete consequences:
cfis lost on RSC navigations. Since this runs beforeapplyAppMiddleware, theuserlandRequestpassed to middleware no longer carriescf. Vinext'sNextRequest.geo/ipread from headers, so geo/ip data survives — but user middleware that readsrequest.cfdirectly (a documented Workers escape hatch) would seeundefinedon every soft RSC navigation while still seeing it on the initial HTML load. That's a dev/prod parity gap between full loads and client navigations.- Missing
duplex: "half"for bodied requests. Every other request-reconstruction site setsduplex: "half"when a body is present (prod-server.ts:755,app-route-handler-runtime.ts:150,api-handler.ts:201,cloneRequestWithHeadersfallback atrequest-pipeline.ts:671). Therequest.body ? request.clone() : requestbranch here can producenew Request(string, bodiedRequest)withoutduplex, which throwsRequestInit: duplex option is required when sending a bodyon Node/undici. Vinext's own client never appends_rscto POST action requests (they usecreateServerActionRequestUrl, which omits_rsc), so this won't fire for normal flows — but a non-standard client POSTing to?_rsc=...with a body would hit it.
Reusing cloneRequestWithHeaders would address both — it preserves cf and handles the duplex/body case:
| return new Request(url.toString(), source); | |
| stripRscCacheBustingSearchParam(url); | |
| const source = request.body ? request.clone() : request; | |
| return cloneRequestWithHeaders(new Request(url.toString(), { method: source.method }), source.headers); |
(Note: the suggestion above is illustrative — cloneRequestWithHeaders overrides headers but keeps the original URL, so a clean fix likely needs a small variant that also overrides the URL. The key point is to route through the same cf-preserving / duplex-aware path rather than a bare new Request.)
|
Review posted successfully. Here's my re-review summary of PR #1744: Review Result: LGTM with two non-blocking edge-case findingsThe core logic is correct and matches Next.js behavior. I independently re-verified both changes against the actual execution path: Verified correct:
New findings (both non-blocking, on
Both are edge cases outside the GET-navigation flow this PR targets, so they don't block merge. Reusing |
requestWithoutRscCacheBustingSearchParam reconstructed the userland request with a bare new Request(url, source), which drops the Workers-specific cf property and omits duplex: "half" for streaming bodies (throwing on Node/undici). Add a cloneRequestWithUrl helper mirroring cloneRequestWithHeaders that overrides the URL while preserving headers/cf and setting duplex, and route the _rsc strip through it.
|
/bigbonk re-review for issues |
There was a problem hiding this comment.
Re-review (/bigbonk). The two core changes are correct and well-targeted, and the prior review's cf/duplex concerns are now fully addressed by the new cloneRequestWithUrl helper (it mirrors cloneRequestWithHeaders, re-attaches Workers cf, and sets duplex: "half" on the bodied fallback path, with direct unit coverage).
Verified correct:
requestWithoutRscCacheBustingSearchParamruns afterresolveInvalidRscCacheBustingRequest(handler line 458), so cache-busting validation still sees the real_rscbefore the strip.userlandRequestis threaded to bothapplyAppMiddleware(line 485) andbuildPostMwRequestContext(line 504).hasRscCacheBustingSearchParamandstripRscCacheBustingSearchParamnow shareisRscCacheBustingSearchPair, so the guard and the strip can never disagree (closes the prior "guard vs stripper matching" concern). Gating on the matcher instead of a before/afterurl.searchcomparison also avoids spurious rebuilds on degenerate empty query pairs.isSamePageSearchNavigationis consistent with the existingisSameAppRoutePopstateTarget(stripBasePathpathname + normalizedsearchParams.toString()); it bypasses cache only when pathname matches AND search differs, leaving hash-only / identical-URL navigations on the cached path.- The e2e spec uses the fixture-scoped
linkFixtureNodeModuleshelper (byte-identical to sibling specs) and adds an explicit 599-leak assertion. Both prior e2e concerns are resolved.
One new non-blocking finding (external rewrite _rsc leak): see inline comment. The has/missing matching is correctly userland-derived, but the original request (with _rsc) is still forwarded to proxyExternalRequest for external config rewrites, which leaks the internal transport query to the upstream service — inconsistent with this PR's stated invariant that RSC transport params stay framework-only.
No blockers.
|
|
||
| const scriptNonce = getScriptNonceFromHeaderSources(request.headers, middlewareContext.headers); | ||
| const postMiddlewareRequestContext = buildPostMwRequestContext(request); | ||
| const postMiddlewareRequestContext = buildPostMwRequestContext(userlandRequest); |
There was a problem hiding this comment.
Threading userlandRequest into middleware + buildPostMwRequestContext is correct, but the applyRewrite calls below still pass the original request — and _rsc can still leak to external rewrite proxies.
The has/missing matching is safe (matchRewrite reads options.requestContext, which derives from userlandRequest). But when a config rewrite resolves to an external URL, applyRewrite forwards the original request to proxyExternalRequest (line 304), which reconstructs the target from request.url and merges its query params (config-matchers.ts:1186-1196). So an RSC navigation matching a beforeFiles/afterFiles/fallback external rewrite (e.g. destination: "https://api.example.com/:path*") forwards _rsc=<hash> to the upstream service.
This is inconsistent with the PR's stated invariant that internal RSC transport params remain framework-only — the same reasoning that motivates hiding _rsc from middleware applies to external proxy destinations. The fix is to pass userlandRequest to all three applyRewrite calls (lines 515, 620, 638); it's already in scope, and for an external POST-action rewrite it's the body-clone, which is safer than re-consuming the original stream. Non-blocking and narrow (only external config rewrites on RSC navigations), but worth closing for consistency.
|
Review posted successfully. Summary of my re-review of PR #1744: Review Result: LGTM with one new non-blocking findingVerified correct (re-checked against the execution path):
New non-blocking finding: The three No blockers found. |
The beforeFiles/afterFiles/fallback applyRewrite calls forwarded the original request (with _rsc) to proxyExternalRequest, which reconstructs the upstream URL from request.url and merges its query params — leaking the internal RSC transport query to external rewrite destinations. Pass the _rsc-stripped userlandRequest to all three calls, consistent with the middleware invariant. Add a regression test that points an external rewrite at a loopback server and asserts the forwarded URL drops _rsc while preserving userland query params.
|
/bigbonk re-review for issues |
There was a problem hiding this comment.
Re-review (/bigbonk). All prior-round findings are now resolved in this revision, and I re-verified the core logic against the actual execution path. No blockers.
Verified correct:
-
_rscvalidation order +userlandRequestthreading. Cache-busting validation runs on the realrequestatresolveInvalidRscCacheBustingRequest(app-rsc-handler.ts:458) before the strip. The strippeduserlandRequestis then threaded toapplyAppMiddleware(485),buildPostMwRequestContext(504), and all threeapplyRewritecalls (517 / 627 / 646). This closes the prior external-rewrite leak finding:proxyExternalRequestreconstructs the target fromrequest.urland merges its query (config-matchers.ts:1186-1196), so passing the_rsc-stripped request prevents the internal transport query from reaching upstream services. -
cloneRequestWithUrlmirrorscloneRequestWithHeaders. It preserves the Workerscfproperty viaObject.definePropertyand setsduplex: "half"on the bodied fallback path (request-pipeline.ts:699-733), resolving the priorcf/duplexconcerns. Direct unit coverage added intests/request-pipeline.test.ts. -
Guard/stripper share one matcher.
hasRscCacheBustingSearchParam(app-rsc-cache-busting.ts:239) derivesrawQueryidentically togetSearchPairsWithoutRscCacheBustingand reusesisRscCacheBustingSearchPair, so the guard and strip can never disagree, and degenerate empty query pairs no longer trigger spurious rebuilds. -
Body-stream safety.
requestWithoutRscCacheBustingSearchParamclones when a body is present;applyAppMiddlewarealso clones before reading (app-middleware.ts:80), so the shareduserlandRequestbody remains readable for the externalapplyRewrite->proxyExternalRequestpath. Only one external rewrite resolves per request, so the body is consumed at most once. -
isSamePageSearchNavigationis consistent with the existingisSameAppRoutePopstateTarget(app-browser-entry.ts:1264): both comparestripBasePath(pathname)+ normalizedsearchParams.toString(). It bypasses the visited/prefetch caches only when pathname matches AND search differs, leaving hash-only and identical-URL navigations on the cached path. The optimistic-shell exemption is now documented. -
Tests.
tests/app-rsc-handler.test.tsandtests/request-pipeline.test.tspass locally (133 tests). The new handler test asserts both the middleware-leak guard and the external-rewrite leak guard; the e2e spec uses the fixture-scopedlinkFixtureNodeModuleshelper and adds an explicit no-599 assertion.
LGTM.
|
Re-review posted. SummaryPR #1744 has converged through several review rounds, and this revision resolves all prior findings. I independently verified the core logic against the execution path: No blockers found. Key verifications:
Unit tests pass locally (133 tests across the two affected files). |
The query-param link is in the viewport and gets prefetched on load. On slower runtimes (WebKit), that prefetch could land after rscRequestUrls.clear(), so it was misattributed to the hash-only navigations and tripped the "no with-query-param request" assertion. Wait for networkidle before clearing, mirroring upstream's waitForIdleNetwork() in the ported Next.js test.
2 participants
Overview
navigatekeeps the pathname but changes search params, and hide the internal_rsccache-busting query from middleware-facingrequest.nextUrl.packages/vinext/src/server/app-browser-entry.ts,packages/vinext/src/server/app-rsc-handler.ts,tests/e2e/app-router/nextjs-compat/hash-rsc-requests.browser.spec.ts,tests/app-rsc-handler.test.ts_rsc; cross-route prefetch reuse remains unchanged.Why
For App Router, search params are part of the page input. Next.js treats same-page navigations specially: hash-only changes are local, but same-page search changes refetch page segments rather than relying on a cached payload that may have been prefetched before the navigation.
The upstream navigation fixture also asserts that middleware must not observe Next's internal RSC union query. Vinext already uses
_rscto validate RSC cache-busting, but the same raw URL was passed into middleware. That made the query+hash RSC request fail before it could commit and scroll.with-query-paramRSC request occurs during hash-only clicks and now mirrors the upstream fixture's exact scroll offsets.navigateoperations with unchanged pathname and changed search params._rscfirst, then strips it from the request clone passed to middleware and post-middleware has/missing matching.What changed
/hash#non-existentto/hash?with-query-param#hash-160after visible Link prefetch_rscinrequest.nextUrl.searchParams, matching the remaining deploy-suite failure./hash#...Validation
vp test run tests/app-rsc-handler.test.tsvp run vinext#buildPLAYWRIGHT_PROJECT=app-router-chrome-browser-specific pnpm run test:e2e tests/e2e/app-router/nextjs-compat/hash-rsc-requests.browser.spec.tsvp test run tests/app-browser-entry.test.ts tests/prefetch-cache.test.tsPLAYWRIGHT_PROJECT=app-router pnpm run test:e2e tests/e2e/app-router/nextjs-compat/hash-popstate-scroll.spec.ts tests/e2e/app-router/nextjs-compat/router-autoscroll.spec.tsvp checkThe new handler regression was red before the source change: middleware received a
NextRequest.nextUrlwhosesearchParamsstill contained_rsc.References
createHrefFromUrlNon-goals
This PR does not replace vinext's route-level prefetch cache with Next.js's segment cache. It only closes the observed parity gap for same-page search navigations while preserving the existing cache model.