fix(app-router): preserve front redirects through fallback route handlers - #2000
Conversation
commit: |
5a94057 to
18b96a8
Compare
14ca1db to
8446db4
Compare
|
/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.
Review: front-redirect-issue fix
Reviewed all 17 changed files plus surrounding context in app-rsc-handler.ts, app-rsc-request-normalization.ts, app-browser-entry.ts, and app-browser-stream.ts. Ran the unit suites:
vp test run tests/app-rsc-handler.test.ts -t "hides internal RSC cache-busting params"→ 3 passedvitest run tests/app-browser-stream.test.ts→ 11 passed
(app-rsc-handler.test.ts only fails under a bare npx vitest because the repo's vp test runner is required; it passes under vp test run.)
What's correct
1. request: userlandRequest (app-rsc-handler.ts:773) — Correct and consistent. The route-handler dispatch was the only userland-facing dispatch still passing the raw request (with _rsc in request.url), while middleware, before/after-files rewrites already used userlandRequest. Note searchParams was already clean here because stripRscCacheBustingSearchParam(url) runs at line 598, so this change specifically aligns request.url with the already-stripped searchParams. The page dispatch correctly keeps the raw request (it needs the real request for RSC rendering). The fix flows through entries/app-rsc-entry.ts:785 to __dispatchAppRouteHandler, and since the App Router prod server delegates to the built RSC entry, dev/prod parity holds — no separate Pages-Router-style fix needed.
2. Bootstrap ownership guard — claimInitialAppRouterBootstrap() correctly prevents a second module instance (loaded under a different ESM URL via deployment-id cache busting) from double-consuming the inline RSC and double-hydrating. The null-stream abort path leaving state at "starting" is acceptable because that branch triggers a full reload.
3. Progressive stream done-marker / cancel handling — The accessor defineProperty on done is configurable: true, so resetBrowserGlobals() deletion and stream re-creation both work. Microtask scheduling of closeOnce after a synchronous final-chunk push preserves ordering (verified by the existing replay test, which still passes).
Minor observations (non-blocking)
See inline comments.
| } | ||
|
|
||
| async function main(): Promise<void> { | ||
| if (!claimInitialAppRouterBootstrap()) return; |
There was a problem hiding this comment.
Minor: when readInitialRscStream() returns null (line 1582), __VINEXT_RSC_BOOTSTRAP_STATE__ stays "starting" permanently for this module instance. This is fine in practice because the null branch triggers a full reload (fresh module graph resets the marker), but it does mean a same-instance retry can't re-enter main(). Worth a one-line comment noting the reload-resets-the-marker invariant so a future refactor doesn't accidentally rely on re-entry.
| } | ||
| }, | ||
| cancel() { | ||
| cancelStream?.(); |
There was a problem hiding this comment.
After cancel(), the monkey-patched arr.push and the done accessor are left installed on the runtime/global object — cancelStream only flips closed. This is harmless (the if (closed) return guard and the no-op closeOnce neutralize late writes, and a subsequent createProgressiveRscStream() re-installs both since the accessor is configurable). Flagging only so it's a conscious choice rather than an oversight; no change required for the bootstrap-once scenario.
| path: string; | ||
| }>; | ||
| }, | ||
| ): Promise<Response> { |
There was a problem hiding this comment.
The handler re-fetches request.nextUrl after rewriting the pathname to /app-future/en/${path}. Confirmed this can't recurse through the fallback rewrite: the rewritten path matches the [lang]/(dashboard)/[teamSlug] page directly, so the fallback (which only fires when nothing else matches) doesn't re-trigger. Good — matches the upstream fixture intent.
|
Review posted successfully. Here's a summary of my findings: Review Summary for PR #2000The PR is well-implemented, correct, and well-tested. I verified the core mechanics by reading the full handler flow and running the affected unit suites (14 tests passing under the proper Key findingsThe fix is correct:
Non-blocking observations (3 inline comments)
One note: the PR description's verification commands reference files ( |
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: front-redirect-issue fix
Reviewed all 17 files plus surrounding context in app-rsc-handler.ts, app-browser-entry.ts, app-browser-stream.ts, app-ssr-entry.ts, and base-path.ts. Ran the affected suites and checks locally:
vp test run tests/app-browser-stream.test.ts tests/app-rsc-handler.test.ts→ 57 passed (11 + 46)vp checkon the 5 changed source/test files → format, lint, type checks cleannpx knip→ exit 0 (validates theknip.tschange)
The fix is correct
1. request: userlandRequest (app-rsc-handler.ts:773) — This was the last userland-facing dispatch still passing the raw request (with _rsc in request.url). Middleware (:513), before/after-files (:545/:668), and fallback (:688) rewrites already use userlandRequest. searchParams was already clean here (stripped at :598), so this aligns request.url with the already-clean searchParams. The page dispatch correctly keeps the raw request. The fix flows through entries/app-rsc-entry.ts:785 → __dispatchAppRouteHandler, and since App Router prod delegates to the built RSC entry, dev/prod parity holds. The new unit test (tests/app-rsc-handler.test.ts:780) correctly exercises the route-handler branch and would fail without the fix.
2. Bootstrap ownership guard — claimInitialAppRouterBootstrap() correctly gates main() so a second module instance (loaded under a deployment-id-busted ESM URL) can't double-consume the inline RSC or double-hydrate. Checking both __VINEXT_RSC_ROOT__ and __VINEXT_RSC_BOOTSTRAP_STATE__ covers the in-flight and committed cases.
3. latestHydrationPathAndSearch — Verified safe for basePath apps. The value comes from navContext.pathname (= canonicalPathname = basePath-stripped cleanPathname), and createClientNavigationRenderSnapshot re-applies stripBasePath, which is idempotent (utils/base-path.ts:22). So /docs/foo (old: window.location.href → strip → /foo) and /foo (new: already stripped → no-op) yield the same pathname. The meaningful change is the searchParams now reflect the server-rendered URL rather than the address bar — exactly what avoids the post-front-redirect hydration mismatch. It's only read once at bootstrap, so later navigations re-writing it is harmless.
4. Progressive stream done/cancel handling — The configurable: true accessor on done lets resetBrowserGlobals() deletion and stream re-creation work, and the microtask scheduling of closeOnce preserves the final-chunk-before-close ordering (the done marker is always a separate <script> from chunk pushes per build/prerender.ts:207). The cancel() path correctly flips closed and cancels the pending completion check.
Non-blocking observations
See inline comments. Nothing here blocks merge.
| } | ||
| }, | ||
| cancel() { | ||
| cancelStream?.(); |
There was a problem hiding this comment.
Non-blocking: after cancel() runs, the monkey-patched arr.push and the done accessor stay installed on the runtime/global — cancelStream only flips closed. This is harmless (the if (closed) return guard in push, and closeOnce/scheduleCloseOnce short-circuiting on closed, neutralize any late writes; and a subsequent createProgressiveRscStream() re-installs both since the accessor is configurable). Flagging only so the leak is a conscious choice rather than an oversight — no change required for the bootstrap-once production path.
| // global assignment, even if a future refactor interposes async work here. | ||
| // The recovery path reloads the document, which resets the "starting" claim; | ||
| // this module instance is intentionally not eligible to retry bootstrap. | ||
| if (rscStream === null) return; |
There was a problem hiding this comment.
Non-blocking: on the rscStream === null path, __VINEXT_RSC_BOOTSTRAP_STATE__ stays "starting" for this module instance, so a same-instance retry can't re-enter main(). The added comment (The recovery path reloads the document, which resets the "starting" claim) correctly documents why this is fine in practice. Good — this is the kind of invariant note that keeps a future refactor from accidentally relying on re-entry.
| use: { baseURL: "http://localhost:4186" }, | ||
| server: { | ||
| command: | ||
| "test -e node_modules || ln -s ../../../fixtures/app-basic/node_modules node_modules && npx vp run vinext#build && NEXT_DEPLOYMENT_ID=vinext-front-redirect-e2e node ../../../../packages/vinext/dist/cli.js build && NEXT_DEPLOYMENT_ID=vinext-front-redirect-e2e node ../../../../packages/vinext/dist/cli.js start --port 4186", |
There was a problem hiding this comment.
Non-blocking: test -e node_modules || ln -s ... — test -e follows symlinks, so if a dangling symlink is left over from a prior local run (target deleted), test -e returns false and the subsequent ln -s fails with File exists. This won't affect CI (fresh checkout, reuseExistingServer: false), but for local reuse test -e || test -L (or ln -sf) would be more robust. Minor.
|
Review posted successfully. Here's my summary: Review Summary for PR #2000The PR is correct, well-scoped, and well-tested. I verified the full handler flow, ran the affected suites (57 tests passing), Key findings — the fix is correct
Non-blocking observations (3 inline comments)
The |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: front-redirect-issue fix
Reviewed the full net diff against main (not just individual commits), read the request lifecycle in app-rsc-handler.ts, the bootstrap path in app-browser-entry.ts, and the stream lifecycle in app-browser-stream.ts. Ran the affected suites and checks locally:
vp test run tests/app-browser-stream.test.ts tests/app-rsc-handler.test.ts→ 58 passed (11 + 47)vp checkon the 4 changed source files → format, lint, type checks cleannpx knip→ exit 0
The three real fixes are correct and well-tested
-
request: userlandRequest+ always-strip_rsc(app-rsc-handler.ts:495,:595,:769) — Dropping theisRscRequestgate is safe:requestWithoutRscCacheBustingSearchParamearly-returns the original request when no_rscpair is present (:378), so ordinary requests are untouched (same object, no rebuild). The only behavioral change is that a request carrying a literal_rscquery now has it stripped for userland route handlers, which matches Next.js'stripInternalSearchParams. The route-handler dispatch was the last userland-facing dispatch still passing the rawrequest; this alignsrequest.urlwith the already-strippedsearchParams. The page dispatch correctly keeps the rawrequest. New unit tests exercise both the fallback-rewritten and non-RSC branches. -
Bootstrap ownership guard —
claimInitialAppRouterBootstrap()gatesmain()on both__VINEXT_RSC_ROOT__and__VINEXT_RSC_BOOTSTRAP_STATE__, correctly preventing a second deployment-id-busted module instance from double-consuming the inline RSC or double-hydrating. -
Progressive stream done/cancel handling — The
configurable: truedoneaccessor closes the stream when the done marker arrives without a following chunk, and the microtask scheduling preserves the final-chunk-before-close ordering (verified by the still-passing replay test, where settingdone=truethenpush("final")keeps"final"readable because the synchronous push enqueues before the scheduledcloseOnceruns).
Main finding: the PR description is stale and overstates the diff
The PR was reworked in-place. Two of the described changes were added and then reverted within the same PR, so the final net diff no longer matches the description:
- "preserve the server-rendered hydration path/search when bootstrapping the browser router after an internal front redirect" — the
latestHydrationPathAndSearchmechanism this referred to was removed in4f6fefa(bootstrap is back towindow.location.href). What survives in the net diff is a no-op local-variable extraction inrestoreHydrationNavigationContext(see inline comment). The?tab=latestquery-preservation assertion in the e2e was also reverted (e25a2a0), so query preservation across a front redirect is no longer covered by this PR at all. - "avoid appending the deployment-id query to the native ESM bootstrap module URL" — no
built-asset-url.ts/deployment-idsource change is in this diff; the e2edplassertion relies on pre-existing behavior. ThestripDeploymentIdQueryexport was added in191a9f1and removed in7823379, netting to zero.
None of this blocks merge — the surviving fixes are sound — but the description should be trimmed to the three changes that actually landed, and it's worth a maintainer's call whether the now-vestigial query-preservation work should be re-added (with coverage) or dropped cleanly.
Non-blocking observations
See inline comments. The knip.ts "agent" cleanup is benign (knip passes); it's unrelated to this PR's scope but harmless.
| searchParams: SearchParamInput, | ||
| params: Record<string, string | string[]>, | ||
| ): void { | ||
| const normalizedSearchParams = new URLSearchParams(searchParams); |
There was a problem hiding this comment.
This normalizedSearchParams extraction is now a pure no-op refactor against main. It was introduced to feed latestHydrationPathAndSearch, but that mechanism was reverted in 4f6fefa (bootstrapHydration is back to window.location.href). The only remaining use is the immediate setNavigationContext call below, so the intermediate adds nothing over the previous inline new URLSearchParams(searchParams). Consider reverting to keep the diff minimal:
| const normalizedSearchParams = new URLSearchParams(searchParams); | |
| setNavigationContext({ | |
| pathname, | |
| searchParams: new URLSearchParams(searchParams), | |
| params, | |
| }); |
|
|
||
| await expect(page.locator("#home-page h1")).toHaveText("Hello!", { timeout: 10_000 }); | ||
| expect(page.url()).toBe(`${BASE}/vercel-user`); | ||
| await expect(page.locator("#visible-url")).toHaveText("/vercel-user"); |
There was a problem hiding this comment.
With the ?tab=latest case reverted (e25a2a0), this only asserts the canonical pathname survives the front redirect — which window.location.href already provided before this PR. That's still a meaningful regression guard (it confirms the visible URL stays /vercel-user rather than the rewrite target /app-future/en/vercel-user), but it no longer exercises query-string preservation. If preserving the search across a front redirect is a goal, it'd be worth re-adding a query-param assertion here so the behavior is actually pinned; otherwise the VisibleUrl fixture's useSearchParams usage is unexercised.
| } | ||
| }, | ||
| cancel() { | ||
| cancelStream?.(); |
There was a problem hiding this comment.
Non-blocking (matches the prior reviewer's note): after cancel() runs, the monkey-patched arr.push and the done accessor stay installed on the runtime/global — cancelStream only flips closed. Harmless because the if (closed) return guard in push and closeOnce/scheduleCloseOnce short-circuit on closed, and a later createProgressiveRscStream() re-installs both (the accessor is configurable). Flagging only so the leak is a conscious choice for the bootstrap-once production path.
|
Review posted successfully. Here's a summary of my findings. Review Summary for PR #2000I reviewed the full net diff against The three real fixes are correct and well-tested
Main finding: the PR description is stale and overstates the diffThe PR went back-and-forth in-place. Two described changes were added then reverted within the same PR:
This doesn't block merge — the surviving fixes are sound — but the description should be trimmed, and a maintainer should decide whether the vestigial query-preservation work should be re-added with coverage or dropped cleanly. I posted 3 inline comments (no-op extraction suggestion, the now-narrowed e2e assertion, and the cancel()-leaves-patches-installed note). |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: front-redirect-issue fix
Reviewed the full net diff against main and the surrounding request lifecycle in app-rsc-handler.ts, the bootstrap path in app-browser-entry.ts, and the stream lifecycle in app-browser-stream.ts. Verified locally:
vp test run tests/app-rsc-handler.test.ts tests/app-browser-stream.test.ts-> 58 passed (47 + 11)vp checkon the 4 changed source files -> format / lint / type cleannpx knip-> exit 0
The three real fixes are correct and well-tested
-
request: userlandRequest+ unconditional_rscstrip (app-rsc-handler.ts:495,:595,:769) — Dropping theisRscRequestgate is safe:requestWithoutRscCacheBustingSearchParamearly-returns the original request object when no_rscpair is present (:378), so ordinary requests are untouched. The route-handler dispatch was the last userland-facing dispatch still passing the rawrequest(middleware:511, before/after/fallback rewrites:543/:684already useduserlandRequest), sorequest.urlis now aligned with the already-strippedsearchParams, matching Next.js'stripInternalSearchParams. The page dispatch correctly keeps the rawrequestbecause page components read the strippedurl.searchParams, notrequest.url. Both branches are covered by new unit tests. -
Bootstrap ownership guard —
claimInitialAppRouterBootstrap()gatesmain()on both__VINEXT_RSC_ROOT__and__VINEXT_RSC_BOOTSTRAP_STATE__, correctly preventing a second deployment-id-busted module instance from double-consuming the inline RSC or double-hydrating. HMR is unaffected (it dispatches viarsc:update, never re-invokesmain()). -
Progressive stream done/cancel — The
configurabledoneaccessor closes the stream when the done marker arrives without a following chunk, and microtask scheduling preserves final-chunk-before-close ordering. The inline<expr>.done=truemarker (app-ssr-stream.ts:57,build/prerender.ts:213) is exactly what the setter intercepts. Thecancel()path correctly flipsclosedand removes theDOMContentLoadedlistener.
Note on prior review comments
The earlier automated review comments referencing a normalizedSearchParams no-op extraction and a latestHydrationPathAndSearch mechanism describe code that is not present in this final diff — app-browser-entry.ts is purely additive (the bootstrap guard) and bootstrapHydration still uses window.location.href unchanged. Likewise visible-url.tsx uses usePathname, not useSearchParams. Those earlier comments are themselves stale; nothing to action there. The PR description is still broader than the landed diff, but the surviving changes are sound.
No blocking issues. Two minor observations inline.
| use: { baseURL: "http://localhost:4186" }, | ||
| server: { | ||
| command: | ||
| "test -e node_modules || test -L node_modules || ln -s ../../../fixtures/app-basic/node_modules node_modules && npx vp run vinext#build && NEXT_DEPLOYMENT_ID=vinext-front-redirect-e2e node ../../../../packages/vinext/dist/cli.js build && NEXT_DEPLOYMENT_ID=vinext-front-redirect-e2e node ../../../../packages/vinext/dist/cli.js start --port 4186", |
There was a problem hiding this comment.
Minor (non-blocking): this command relies on &&/|| being left-associative and equal-precedence, so it happens to work in both branches (test -e || test -L || ln -s always evaluates truthy before the && chain). But it reads as if the ln -s is conditionally chained to the build, which is confusing. Wrapping the symlink setup in a group would make the intent explicit and avoid a future edit accidentally breaking precedence:
| "test -e node_modules || test -L node_modules || ln -s ../../../fixtures/app-basic/node_modules node_modules && npx vp run vinext#build && NEXT_DEPLOYMENT_ID=vinext-front-redirect-e2e node ../../../../packages/vinext/dist/cli.js build && NEXT_DEPLOYMENT_ID=vinext-front-redirect-e2e node ../../../../packages/vinext/dist/cli.js start --port 4186", | |
| "{ test -e node_modules || test -L node_modules || ln -s ../../../fixtures/app-basic/node_modules node_modules; } && npx vp run vinext#build && NEXT_DEPLOYMENT_ID=vinext-front-redirect-e2e node ../../../../packages/vinext/dist/cli.js build && NEXT_DEPLOYMENT_ID=vinext-front-redirect-e2e node ../../../../packages/vinext/dist/cli.js start --port 4186", |
(Also note test -e follows symlinks, so a dangling symlink left from a prior local run makes both test -e and test -L... actually test -L catches the dangling case, so this is fine for local reuse. CI is unaffected with a fresh checkout.)
| } | ||
| }, | ||
| cancel() { | ||
| cancelStream?.(); |
There was a problem hiding this comment.
Non-blocking: after cancel() runs, the monkey-patched arr.push and the redefined done accessor remain installed on the runtime/global — cancelStream only flips closed. This is harmless (the if (closed) return guard in push and closeOnce/scheduleCloseOnce short-circuit on closed, and a later createProgressiveRscStream() re-installs both since the accessor is configurable), and the new cancel test confirms late writes don't throw. Flagging only so the leftover patches are a conscious choice for the bootstrap-once production path rather than an oversight.
|
Review posted successfully. Here's my summary: Review Summary for PR #2000I reviewed the full net diff against The three real fixes are correct and well-tested
Key correction to prior automated reviewsI verified that the earlier bonk comments about a Non-blocking observations (2 inline)
No blocking issues — the PR is sound and ready to merge once CI is green. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: front-redirect-issue fix
Reviewed the full net diff against main: the request lifecycle in app-rsc-handler.ts, the bootstrap claim in app-browser-entry.ts, the stream done/cancel logic in app-browser-stream.ts, plus the e2e fixture and playwright/CI wiring.
The three runtime fixes are correct
-
request: userlandRequest+ unconditional_rscstrip (app-rsc-handler.ts:495,:595,:769) — Dropping theisRscRequestgate is safe:requestWithoutRscCacheBustingSearchParamearly-returns the original request object when no_rscpair is present (:378), so ordinary requests are untouched (same object, no rebuild). The route-handler dispatch was the last userland-facing dispatch still passing the rawrequest; this alignsrequest.urlwith the already-strippedsearchParams. Page dispatch correctly keeps the rawrequest. Both branches are covered by the new unit tests, and dev/prod parity holds since App Router prod delegates to the built RSC entry. -
Bootstrap ownership guard —
claimInitialAppRouterBootstrap()gatesmain()on both__VINEXT_RSC_ROOT__and__VINEXT_RSC_BOOTSTRAP_STATE__, correctly preventing a second deployment-id-busted module instance from double-consuming the inline RSC or double-hydrating.main()is only invoked once at module eval (:2302); HMR dispatches viarsc:updateand never re-invokes it, so HMR is unaffected. This also closes the only path that could re-runcreateProgressiveRscStream()and orphan the first stream'sdoneaccessor — good. -
Progressive stream done/cancel — The
configurabledoneaccessor closes the stream when the done marker arrives without a following chunk. Because the done marker is always a separate<script>from chunk pushes (build/prerender.ts:207/:213), and thepushpatch enqueues synchronously whilecloseOnceis deferred via microtask, the final-chunk-before-close ordering is preserved. Thecancel()path flipsclosedand cancels the pending completion check; laterdone=true/pushare correctly neutralized by theif (closed) return/if (!closed)guards (verified by the new cancel test).
Note: the unconditional _rsc strip is a deliberate behavioral change
isRscCacheBustingSearchPair (app-rsc-cache-busting.ts:220) matches any param named _rsc regardless of value — not just one matching the computed cache-busting hash. So after this change, a non-RSC request carrying a literal ?_rsc=anything will have it stripped before userland route handlers see it. This matches Next.js, which treats _rsc as a reserved internal param and strips it unconditionally in stripInternalSearchParams, so it's correct parity. The new hides internal RSC cache-busting params from non-RSC route handler requests test pins exactly this. Worth keeping in mind that a user route handler can no longer observe a literal _rsc query param — that's intended.
Non-blocking observations
The prior automated reviews already flagged the cancel()-leaves-patches-installed note, the playwright &&/|| precedence, and the "starting" claim not resetting on the null-abort path. I agree those are all harmless. Two small additions inline.
No blocking issues — the PR is sound.
| expect(page.url()).toBe(`${BASE}/vercel-user`); | ||
| await expect(page.locator("#visible-url")).toHaveText("/vercel-user"); | ||
|
|
||
| const bootstrapSrc = await page.locator('script[type="module"][src]').getAttribute("src"); |
There was a problem hiding this comment.
page.locator('script[type="module"][src]') matches the first module script with a src. If the bootstrap output ever emits more than one such script (e.g. a vendor/preamble chunk before the entry), .getAttribute("src") silently resolves against whichever happens to be first in the DOM, which may not be the bootstrap entry. Since the assertion that follows (bootstrapRequests has length 1) is the real regression guard for single-hydration, this is low risk — but if multiple module scripts are emitted, consider .first() explicitly or filtering to the known bootstrap entry name so the test fails loudly rather than asserting against the wrong script.
|
|
||
| export default async function TeamDashboardPage(props: { params: Promise<{ teamSlug: string }> }) { | ||
| const params = await props.params; | ||
| await new Promise((resolve) => setTimeout(resolve, 1000)); |
There was a problem hiding this comment.
The fixed setTimeout(resolve, 1000) (ported from upstream to simulate slow data) adds a hard 1s to every run of this project. It's within the 10s locator timeout in the spec, so it's fine, but it does make this the slowest single-test e2e project. If CI time matters, a shorter delay (e.g. 200ms) would still reproduce the streamed-redirect timing window the regression depends on. Non-blocking.
|
Review posted successfully. Here's my summary: Review Summary for PR #2000I reviewed the full net diff against The three runtime fixes are correct
One behavioral note I made explicitThe Non-blocking inline comments (2)
No blocking issues — the PR is sound and well-tested. The prior reviews' non-blocking notes (cancel() leaves patches, playwright precedence, the "starting" null-abort path) are all accurate and harmless. |
|
/bigbonk review for issues |
2 similar comments
|
/bigbonk review for issues |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: front-redirect-issue fix
Reviewed the full net diff against main (15 commits — the PR was reworked in-place, so individual commits don't reflect the landed state). Verified locally: vp test run tests/app-rsc-handler.test.ts tests/app-browser-stream.test.ts → 59 passed (48 + 11); vp check on the 3 changed source files → clean.
The key thing I want to flag for human reviewers: the route-handler dispatch logic in the final two commits (0251e10, c99f2df) is materially different from what every prior automated review described. The earlier reviews kept saying "request: userlandRequest — strips _rsc for all route handlers." That is no longer what the code does. The landed code at app-rsc-handler.ts:766 is:
const routeHandlerRequest =
isRscRequest || isEdgeRouteHandler(route.routeHandler) ? userlandRequest : request;So there are now three cases:
- RSC request → stripped URL (
userlandRequest) - Edge route handler (
export const runtime = "edge") → stripped URL - Ordinary (non-RSC, node) route handler → raw
requestwith_rscpreserved inrequest.url
I independently checked this against Next.js source (since the prior reviews predate this design):
- Edge handlers go through
web/adapter.ts, which builds theNextRequestfromstripInternalSearchParams(normalizeURL)— so edge route handlers see_rscstripped from the URL. ✅ matchesisEdgeRouteHandler→userlandRequest. - Node handlers:
base-server.ts/next-server.tsonlydelete query[NEXT_RSC_UNION_QUERY]on the parsed query object, not onrequest.url(which is rebuilt frominitURLand keeps_rsc). So node route handlers see_rscpreserved inrequest.url. ✅ matches the: requestfallback.
This is a genuinely subtle parity detail and the implementation gets it right. isEdgeRouteHandler reading handler.runtime is also consistent with how vinext resolves runtime elsewhere (app-segment-config.ts:126 reads segment.runtime off the module exports). The three new unit tests pin all three branches.
Net residue confirmed clean: latestHydrationPathAndSearch, normalizedSearchParams, and stripDeploymentIdQuery (flagged by earlier bot runs) do not exist in the final diff — bootstrapHydration still uses window.location.href and app-browser-entry.ts is purely additive. Those earlier comments are stale.
The bootstrap ownership guard and the progressive-stream done/cancel handling are both correct and well-tested.
One real concern
The PR description's "Verification" section and several described changes (deployment-id bootstrap URL, hydration path/search preservation) describe work that is not in this diff. Combined with the fact that the route-handler design changed twice after the description was written, the description materially overstates and misdescribes the landed change. A maintainer should trim it to the changes that actually shipped, and confirm the edge/node split was an intentional design decision (it's correct, but it's worth recording why node preserves _rsc while edge strips it, since it looks backwards at first glance).
No blocking issues — the surviving fixes are sound. Two minor non-blocking notes inline.
| buildPageCacheTags(cleanPathname, [], [...route.routeSegments], "route"), | ||
| ); | ||
| const routeHandlerRequest = | ||
| isRscRequest || isEdgeRouteHandler(route.routeHandler) ? userlandRequest : request; |
There was a problem hiding this comment.
The edge-vs-node _rsc split here is correct Next.js parity but extremely non-obvious — at a glance it reads backwards (why would edge strip and node preserve?). The reason: edge route handlers run through web/adapter.ts, which builds the request from stripInternalSearchParams(normalizeURL) (URL stripped), whereas node handlers in base-server.ts/next-server.ts only delete query[NEXT_RSC_UNION_QUERY] on the parsed query object and rebuild request.url from initURL (which keeps _rsc).
Worth a comment so a future refactor doesn't "simplify" this back to unconditional stripping (which is exactly what every prior review of this PR assumed the code did):
| isRscRequest || isEdgeRouteHandler(route.routeHandler) ? userlandRequest : request; | |
| // Next.js parity: edge route handlers go through web/adapter.ts which | |
| // strips internal search params from the request URL | |
| // (stripInternalSearchParams), so they see `_rsc` removed. Node route | |
| // handlers only strip `_rsc` from the parsed query object — request.url is | |
| // rebuilt from initURL and keeps `_rsc`. RSC requests always use the | |
| // stripped userland URL. Do not collapse this to unconditional stripping. | |
| const routeHandlerRequest = | |
| isRscRequest || isEdgeRouteHandler(route.routeHandler) ? userlandRequest : request; |
|
|
||
| const bootstrapScripts = page.locator('script[type="module"][src]'); | ||
| await expect(bootstrapScripts).toHaveCount(1); | ||
| const bootstrapSrc = await bootstrapScripts.getAttribute("src"); |
There was a problem hiding this comment.
Non-blocking: bootstrapScripts.getAttribute("src") will throw a Playwright strict-mode violation if the locator ever resolves to more than one element. The toHaveCount(1) assertion on the line above guards the happy path (so this is safe today), but if a vendor/preamble module script is ever emitted before the entry, line 24 fails first with a count mismatch rather than this line silently reading the wrong src. Consider .first() or filtering to the known bootstrap entry name to make the intent explicit and the failure mode loud.
|
Review posted successfully. Here's my summary: Review Summary for PR #2000I reviewed the full net diff against Key new finding the prior automated reviews all missedEvery prior bot review described the route-handler fix as " const routeHandlerRequest =
isRscRequest || isEdgeRouteHandler(route.routeHandler) ? userlandRequest : request;
I independently verified this against Next.js source (which the prior reviews predated):
So the split is correct Next.js parity, even though it reads backwards at first glance. Verified cleanThe One real concern (non-blocking)The PR description overstates/misdescribes the landed change (references deployment-id and hydration-path work not in this diff). A maintainer should trim it and record why the edge/node I posted 2 inline notes: a suggested explanatory comment on the edge/node split, and a Playwright strict-mode |
|
/bigbonk review for issues |
2 similar comments
|
/bigbonk review for issues |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: front-redirect-issue fix
Reviewed the full net diff against main (the PR was reworked across 19 commits, so the landed code differs materially from what earlier automated reviews described — those reviews predate the final requestWithoutRscSuffix / external-rewrite rework). I traced the request lifecycle in app-rsc-handler.ts, the bootstrap claim in app-browser-entry.ts, and the stream done/cancel logic in app-browser-stream.ts, and verified the edge/node _rsc semantics against the Next.js canary source. Verified locally: vp test run tests/app-rsc-handler.test.ts tests/app-browser-stream.test.ts -> 60 passed; vp check on the 4 changed source files -> clean; npx knip -> exit 0.
The fixes are correct and well-tested
1. normalizedUserlandRequest vs userlandRequest split (app-rsc-handler.ts:510-511) — userlandRequest now unconditionally strips _rsc (the isRscRequest gate was dropped). requestWithoutRscCacheBustingSearchParam early-returns the original object when no _rsc pair is present (:383), so ordinary requests are untouched. This matches Next.js' stripInternalSearchParams (internal-utils.ts), which strips _rsc unconditionally. Middleware and metadata correctly continue to use the stripped userlandRequest.
2. External rewrite _rsc forwarding is a deliberate behavior reversal — and it's correct parity. The before/after/fallback rewrites changed from request: userlandRequest (stripped) to request: normalizedUserlandRequest (preserves _rsc). Since applyRewrite only uses request for external rewrites (proxyExternalRequest, :333), this only affects full-URL proxy destinations. I verified against Next.js: resolve-routes.ts never strips _rsc before the external proxy path, and proxy-request.ts forwards parsedUrl.query verbatim — so Next.js does forward _rsc to external rewrite destinations. The prior vinext behavior (stripping it) was the divergence. The test flip from .toBe(false) to .toBe(true) is the right call. This is the most consequential change in the PR and is barely mentioned in the description — worth calling out explicitly so it isn't mistaken for an accidental regression of the prior PR's tested behavior.
3. Edge vs Node route-handler split (:779) — Verified against Next.js canary: edge handlers go through web/adapter.ts:185 which builds the request from stripInternalSearchParams(normalizeURL) (URL stripped), while node handlers only delete query[NEXT_RSC_UNION_QUERY] on the parsed query and rebuild request.url from initURL (preserved). The isEdgeRouteHandler ? userlandRequest : normalizedUserlandRequest split is exactly correct, and the inline comment documents the non-obvious reasoning. All three branches are pinned by new unit tests.
4. Bootstrap ownership guard — claimInitialAppRouterBootstrap() correctly prevents a second deployment-id-busted module instance from double-consuming the inline RSC or double-hydrating. The null-abort-leaves-"starting" path is documented as reload-resets.
5. Progressive stream done/cancel — The configurable done accessor + microtask-deferred closeOnce preserves final-chunk-before-close ordering; cancel() correctly neutralizes late writes.
Non-blocking observations
- The PR description references work (deployment-id bootstrap URL, hydration path/search preservation) that is not in this diff —
built-asset-url.ts/deployment-id.tsare pre-existing and unchanged, andbootstrapHydrationstill useswindow.location.href. The description should be trimmed to the changes that actually landed, and should foreground the external-rewrite_rscreversal (#2 above). - The
knip.tsremoval of the"agent"ignore is unrelated to this PR's scope but benign — knip still passes (the"agent"vitest reporter invite.config.ts:109is a reporter-name string, not a resolved dependency).
No blocking issues — the surviving fixes are sound, well-tested, and correctly match Next.js behavior.
| @@ -492,9 +507,8 @@ async function handleAppRscRequest<TRoute extends AppRscHandlerRoute>( | |||
| // Keep cache-busting validation on the real request above, then hide the | |||
| // internal `_rsc` transport query from userland middleware and post-middleware | |||
| // has/missing matching. This mirrors Next.js' navigation middleware fixture. | |||
There was a problem hiding this comment.
Minor: this comment now only describes the userlandRequest (middleware/has-missing) purpose, but the line below it introduces a second derived request (normalizedUserlandRequest) with the opposite _rsc policy that feeds the external rewrites and node route handlers. Since the external-rewrite _rsc forwarding (a reversal of the prior tested behavior at :557/:675/:694) is the most surprising change in this PR, a one-line note here distinguishing the two would help a future reader:
| // has/missing matching. This mirrors Next.js' navigation middleware fixture. | |
| // Keep cache-busting validation on the real request above, then derive two | |
| // userland-facing request views: | |
| // - `normalizedUserlandRequest`: `.rsc` suffix stripped, `_rsc` PRESERVED. | |
| // Used for external rewrite proxies + Node route handlers (Next.js | |
| // forwards `_rsc` to external rewrites and rebuilds Node request.url | |
| // from initURL, keeping `_rsc`). | |
| // - `userlandRequest`: also strips `_rsc`. Used for middleware, | |
| // post-middleware has/missing matching, and edge route handlers | |
| // (which run through web/adapter.ts -> stripInternalSearchParams). | |
| // This mirrors Next.js' navigation middleware fixture. | |
| const normalizedUserlandRequest = requestWithoutRscSuffix(request); | |
| const userlandRequest = requestWithoutRscCacheBustingSearchParam(normalizedUserlandRequest); |
|
Review posted successfully. Here's a summary of my findings. Review Summary for PR #2000I reviewed the full net diff against The fixes are correct and well-tested
Non-blocking notes
No blocking issues — the fixes are sound and match Next.js behavior. |
2 participants
Summary
Fixes the App Router fallback rewrite + route-handler front redirect path covered by the upstream Next.js regression.
Changes:
_rsc: stripped for RSC and Edge App Route requests, preserved in ordinary Node App Routerequest.url, and always removed from parsed userland search paramsfront-redirect-issuefixture with production E2E coverage for visible pathname and single bootstrap loadingUpstream references
Verification
vp test run tests/app-browser-stream.test.ts tests/app-rsc-handler.test.tsPLAYWRIGHT_PROJECT=app-front-redirect-issue vp run test:e2evp checkE2E (app-front-redirect-issue)