fix(app-router): honor basePath false rewrites#2370
Conversation
|
/bigbonk review for issues |
commit: |
|
Review:
|
|
Posted the review to PR #2370. Summary of findings: Review summaryBlocking issue — out-of-basePath leak (verified empirically): The change
I reproduced this against the real handler with basePath
In Next.js these are served only under the basePath, so unclaimed out-of-basePath requests must 404. Suggested fix: hoist the Non-blocking: The comment at Looks good: the server-action redirect hard-navigation, the |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/bigbonk review for issues |
|
/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: fix(app-router): honor basePath false rewrites (#2370)
I re-reviewed after the two follow-up commits (5a50abe, 91dab97) that address the earlier blocking feedback. I verified the changes by running the touched test files (189 passed) and tsc --noEmit (clean).
Previous blocking issue is resolved
The out-of-basePath leak is fixed correctly. outOfBasePathRequestClaimed is now computed at app-rsc-handler.ts:657 (after beforeFiles, so a basePath: false beforeFiles rewrite still legitimately claims the request) and gates all three handlers that previously leaked:
- image optimization (
app-rsc-handler.ts:660) - metadata routes (
app-rsc-handler.ts:675) - public files (
app-rsc-handler.ts:689)
plus route matching (:728), POST/action dispatch (:738), and renderPagesForMatchKind (:805). The new parameterized test at tests/app-rsc-handler.test.ts:2096 asserts 404 for unclaimed out-of-basePath /logo.svg, /_next/image, and /favicon.ico, mirroring the in-basePath tests. Good coverage.
Correctness checks that pass
shouldEvaluateRule(config-matchers.ts:521-523) is the right primitive: withhadBasePath: false, default rules are skipped and onlybasePath: falserules evaluate. Threading the realhadBasePaththroughbasePathState(replacing the hardcodedtrue) is consistent withprod-server.ts/pages-request-pipeline.ts.redirectDestinationWithBasePathnow correctly skips the prefix when!hadBasePath(:361).- Trailing-slash redirect is correctly suppressed for out-of-basePath requests (
:540), so unclaimed requests don't get a basePath-prefixed redirect. app-middleware.ts:240hadBasePath: options.hadBasePath ?? truepreserves backward compatibility; the new test confirms middleware seesnextUrl.basePath === ""/pathname === "/outside".resolveActionRedirectTargethard-navigates same-origin redirects that leave the basePath (app-browser-server-action-client.ts:96-99) — matches the Next.js v16.2.6 "redirect externally ... outside the basePath" test.normalizeRscRequest'sallowOutsideBasePathdefaults tofalse, so the only caller that opts in is the App handler; no other path changes behavior.
Non-blocking: second stale comment missed
The earlier review fixed the stale comment at the old line 518, but there's a second one that now contradicts the code directly below it. app-rsc-handler.ts:350-353:
// TODO(#1333): once App Router supports basePath: false rules (see
// normalizeRscRequest — it 404s out-of-basePath requests before they
// reach this code), pass hadBasePath here and skip the prefix when
// false, mirroring the same guard in prod-server.ts and deploy.ts.
This PR ships exactly that support, and redirectDestinationWithBasePath already takes basePathState and skips the prefix when hadBasePath is false (:361). Please update or remove this TODO so it doesn't mislead future readers.
Non-blocking: narrow afterFiles/fallback parity gap
outOfBasePathRequestClaimed is computed once before the afterFiles/fallback rewrite loops (:836, :878), and those loops are not gated by it. For an external basePath: false rewrite (the case the Next.js test covers, e.g. destination: 'https://example.vercel.sh/') applyRewrite returns a Response and short-circuits at :853, so it works. App-route internal destinations also work because they fall through to the main render at :958 (which isn't gated).
The remaining gap: an unclaimed out-of-basePath request whose internal basePath: false afterFiles/fallback rewrite targets a Pages-router route would 404, because renderPagesForMatchKind early-returns null while !outOfBasePathRequestClaimed (:805) even after the rewrite legitimately claims the request. This is an obscure combination not exercised by the ported Next.js v16.2.6 test, so it's fine to leave for follow-up — but worth a tracking note rather than a silent divergence.
Verdict
The blocking issue from the prior round is properly fixed and tested. Only the stale TODO at :350-353 is worth cleaning up in this PR; the afterFiles/Pages gap can be a follow-up.
|
Review posted to PR #2370. SummaryThe two follow-up commits ( Blocking issue resolved: The out-of-basePath leak is fixed. Correctness confirmed: Two non-blocking findings:
|
|
/bigbonk review for issues |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): honor basePath false rewrites (#2370)
I re-reviewed after the two new follow-up commits (03e72bd track basePath rewrite claims, e84df94 remove stale basePath note). I verified by running the touched test files plus the middleware suites (228 tests passed across app-rsc-handler, app-browser-server-action-client, app-rsc-request-normalization, entry-templates, app-post-middleware-context, app-router-middleware-next-request, middleware-runtime) and vp check on the four touched server files (clean).
Both prior non-blocking findings are resolved
-
Stale comments removed. Both stale
#1333comments are gone — the one at the old line 518 and the second one at the old:350-353.grepfordoes not yet support/#1333/404s out-of-basePathinapp-rsc-handler.tsnow returns nothing.redirectDestinationWithBasePathtakesbasePathStateand correctly skips the prefix when!hadBasePath(:351-363). -
afterFiles/fallback → Pages parity gap closed.
03e72bdmakesoutOfBasePathRequestClaimed = trueimmediately after a successful afterFiles (:851) and fallback (:894) rewrite, before the subsequentrenderPagesForMatchKindcalls. Since the gate at:801is re-checked per invocation (not once), an unclaimed out-of-basePath request whose internalbasePath: falseafterFiles/fallback rewrite targets a Pages route is now served instead of 404'd. The new parameterized testallows out-of-basePath %s rewrites to reach Pages routes(tests/app-rsc-handler.test.ts:184) exercises exactly this for both phases and asserts the/pageshandoff.
Correctness checks that pass
- The blocking leak fix from the first round holds: image optimization (
:660), metadata routes (:668), public files (:685), route matching (:724), POST/action dispatch (:734), andrenderPagesForMatchKind(:801) are all gated byoutOfBasePathRequestClaimed. The 404 regression tests at:2143cover/logo.svg,/_next/image, and/favicon.ico. resolveActionRedirectTargethard-navigates a same-origin redirect that leaves the basePath (app-browser-server-action-client.ts:96-99);hasBasePathmatches on segment boundaries so/docsfoowon't falsely match/docs.deps.basePathwas already part of the type.normalizeRscRequest'sallowOutsideBasePathdefaults tofalse, so only the App handler opts in;hadBasePathis threaded intobasePathState, the trailing-slash redirect (:536), and App middleware (hadBasePath: options.hadBasePath ?? true).
Non-blocking: one behavioral widening to flag
didMiddlewareRewrite changed from cleanPathname !== normalized.cleanPathname to middlewareResult.didRewrite (:615). The new flag is true for any middleware rewrite, including an identity rewrite (NextResponse.rewrite(req.nextUrl)) or a search-only rewrite, whereas the old pathname comparison treated those as false. This is load-bearing in two places: outOfBasePathRequestClaimed (correct — an explicit rewrite claims the request) and allowRscDocumentFallback: didMiddlewareRewrite (:806), which now permits the RSC→Pages-document fallback for identity/search-only middleware rewrites. This is arguably more correct (an explicit rewrite signals intent), no existing test regresses, and it isn't basePath-specific — but it's an incidental change to non-basePath behavior that isn't directly covered by a test. Worth a one-line note in the PR description or a focused test for the identity-rewrite case.
Verdict
Both items from the previous review are properly addressed and tested. No blocking issues. The only new observation is the non-blocking didMiddlewareRewrite widening above. LGTM.
|
Review posted to PR #2370. SummaryI re-reviewed PR #2370 after the two new commits since the last review round ( Both prior findings resolved:
Verified empirically: 228 tests pass across the touched test files plus middleware suites; One new non-blocking observation: Verdict: LGTM, no blocking issues. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): honor basePath false rewrites (#2370)
Re-reviewed after the new commit 1cd6c8b ("allow late rewrite route posts"), the only change since the last review round. Verified empirically: all touched test files pass (97 + 97 across app-rsc-handler, app-browser-server-action-client, app-rsc-request-normalization, entry-templates).
The new commit is correct
1cd6c8b makes two gating changes so an out-of-basePath POST claimed by a late (afterFiles/fallback) basePath: false rewrite can reach an App route handler:
-
app-rsc-handler.ts:734— the early out-of-basePath POST 404 is narrowed from!claimed && isPostto!claimed && isPost && actionId. Previously every unclaimed out-of-basePath POST 404'd at the top, before the afterFiles/fallback loops ran, so abasePath: falserewrite to a route handler could never be reached on POST. The added&& actionIdlets a non-action POST fall through to the rewrite loops. -
app-rsc-handler.ts:739-744— the progressive (multipart, no-actionId) action path now also requiresoutOfBasePathRequestClaimed, so an unclaimed out-of-basePath multipart POST is no longer treated as a progressive action.
I traced the unclaimed-no-actionId POST fall-through and confirmed it stays safe: every downstream handler (image opt :656, metadata :671, public files :685, preActionMatch :724, renderPagesForMatchKind :806) is still gated by outOfBasePathRequestClaimed, which remains false for a truly unclaimed request. The only new way such a POST produces a non-404 is a matching afterFiles/fallback basePath: false rewrite (:838/:881) — exactly the intended fix. A genuinely unclaimed POST still 404s, just via the !match not-found branch (:933) instead of the early notFoundResponse(), and clearRequestContext is still called (:955). The existing does not dispatch server actions directly outside basePath test (:258) still passes because it sends a next-action header.
The new parameterized test (tests/app-rsc-handler.test.ts:210) covers both the afterFiles and fallback phases for the route-handler handoff. Good coverage.
Intentional asymmetry worth noting (non-blocking)
An out-of-basePath server-action POST (with actionId) destined for a page via a late basePath: false rewrite still 404s at :734, because actionId is present and the request isn't yet claimed when that check runs. This is consistent with the existing design comment (:718-723: actions are processed against the cleanPathname they were posted to and do not re-run rewrites), and the ported Next.js v16.2.6 app-basepath suite passes 13/13. So the divergence (route handlers reachable via late rewrite, page actions not) appears intentional — but it's a subtle distinction that isn't directly asserted by a test. A one-line comment near :734 explaining why actionId gates the early 404 (i.e. actions don't re-run rewrites, route handlers do) would help future readers, and a negative test asserting the action-POST-to-page case still 404s would lock the boundary in.
Previously verified, still holds
- Out-of-basePath leak fix: image opt / metadata / public files / route match / POST dispatch /
renderPagesForMatchKindall gated byoutOfBasePathRequestClaimed; 404 regression tests at:2212cover/logo.svg,/_next/image,/favicon.ico. /__vinext/prerender endpoints are handled at:519-534, before any basePath gate, so thehadBasePath=falsethey now carry doesn't affect them.normalizeRscRequest'sallowOutsideBasePathdefaults tofalse; only the App handler opts in.resolveActionRedirectTargethard-navigates same-origin redirects leaving the basePath (app-browser-server-action-client.ts:96-99).- Stale
#1333comments removed;redirectDestinationWithBasePathskips the prefix when!hadBasePath.
Verdict
The new commit is correct and tested. No blocking issues. Only non-blocking suggestion: a comment + negative test documenting the intentional action-POST-vs-route-handler asymmetry at :734.
|
Review posted to PR #2370. SummaryI re-reviewed PR #2370 focused on the new commit The new commit is correct. It narrows the early out-of-basePath POST 404 ( Verified empirically: all touched test files pass (194 tests across the four files). I traced the unclaimed-no-actionId POST fall-through and confirmed it stays safe — every downstream handler is still gated by One non-blocking suggestion: there's an intentional asymmetry — an out-of-basePath server-action POST (with Verdict: no blocking issues. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): honor basePath false rewrites (#2370)
Re-reviewed after the new commit 8c4f4b0 ("short-circuit unclaimed basePath misses"), the only change since the last review round. Verified empirically: all touched test files pass (160 tests across app-rsc-handler, app-rsc-request-normalization, app-browser-server-action-client) and vp check on app-rsc-handler.ts is clean.
The core of the new commit is correct
8c4f4b0 adds an early-out at app-rsc-handler.ts:933-936: when a request reaches the bottom of the pipeline still !outOfBasePathRequestClaimed, return notFoundResponse() before invoking options.renderNotFound. This is a real optimization — previously an unclaimed out-of-basePath request fell to the if (!match) branch at :938 and compiled/rendered the app's not-found page even though Next.js never serves out-of-basePath requests from the app at all. The new test asserts renderNotFound is not called (tests/app-rsc-handler.test.ts:257).
I confirmed the placement is safe: match = preActionMatch and preActionMatch is null whenever !outOfBasePathRequestClaimed (:724). match only becomes truthy via the afterFiles/fallback rewrite loops, which always set outOfBasePathRequestClaimed = true first (:856, :899). So match truthy ⟹ claimed, and the short-circuit can never strand a legitimately-claimed late-rewrite match. The pagesDataRequest branch returns earlier (:917-931), so it's unaffected. In-basePath requests start with outOfBasePathRequestClaimed = hadBasePath = true (:595), so the short-circuit never fires for them.
Blocking-ish (parity regression): the short-circuit drops middleware response headers on the 404
The pre-existing not-found path at :960-963 merges middleware-contributed headers into the 404:
const headers = new Headers();
mergeMiddlewareResponseHeaders(headers, middlewareContext.headers);
return notFoundResponse({ headers });The new short-circuit at :933-936 returns a bare notFoundResponse() with no header merge. This silently diverges from both Next.js and vinext's own in-basePath behavior, because middleware runs for out-of-basePath requests too (this PR's own test at tests/app-rsc-handler.test.ts:276 confirms middleware sees /outside). A NextResponse.next({ headers }) sets middlewareContext.headers without setting didRewrite (app-middleware.ts:264-266 vs :288), so such a request stays unclaimed and now loses its headers.
I reproduced this against the real handler (basePath /docs, middleware does NextResponse.next() and sets x-mw-custom: from-mw):
GET /outside(out-of-basePath, unclaimed) → 404 withx-mw-custom= null (dropped)GET /docs/missing(in-basePath, unclaimed) → 404 withx-mw-custom= from-mw (preserved)
So identical middleware now yields different 404 headers depending solely on whether the path is inside the basePath. The docstring on notFoundResponse even calls out this exact contract — "The headers option lets call sites merge middleware response headers into the 404, matching the pattern used by app-rsc-handler after a route match fails but middleware has already contributed headers" (http-error-responses.ts:55-57) — and the new path bypasses it.
Suggested fix: merge middleware headers in the short-circuit, mirroring :960-963:
if (!outOfBasePathRequestClaimed) {
options.clearRequestContext();
const headers = new Headers();
mergeMiddlewareResponseHeaders(headers, middlewareContext.headers);
return notFoundResponse({ headers });
}Please also add a regression test asserting an out-of-basePath unclaimed 404 carries a header set by middleware via NextResponse.next({ headers }), mirroring the in-basePath expectation. The ported Next.js v16.2.6 suite passes 13/13 but doesn't cover middleware-header-on-404, so this gap is currently silent.
Previously verified, still holds
- Out-of-basePath leak fix: image opt / metadata / public files / route match / POST dispatch /
renderPagesForMatchKindall gated byoutOfBasePathRequestClaimed; the new short-circuit is consistent with that gating. - Late afterFiles/fallback rewrites correctly set the claim flag before
renderPagesForMatchKind, so route-handler and Pages handoffs still work. resolveActionRedirectTargethard-navigates same-origin redirects leaving the basePath (app-browser-server-action-client.ts:96-99).- Stale
#1333comments removed;redirectDestinationWithBasePathskips the prefix when!hadBasePath.
Verdict
The short-circuit itself is correct and is a genuine improvement. The one issue is the dropped middleware response headers on the out-of-basePath 404 — a small, verified parity regression introduced by this commit. Worth fixing here (one-line merge + a focused test) so out-of-basePath and in-basePath 404s stay consistent.
|
Review posted to PR #2370. SummaryI re-reviewed PR #2370 focused on the new commit The core change is correct. The early-out at One verified parity regression (worth fixing here): The new short-circuit returns a bare
So identical middleware yields inconsistent 404 headers based solely on basePath membership. This diverges from Next.js (middleware runs for out-of-basePath requests and its Verdict: The short-circuit is a genuine improvement; the only issue is the dropped middleware headers on the out-of-basePath 404. |
|
/bigbonk review for issues |
Review:
|
|
Review posted to PR #2370. SummaryI re-reviewed PR #2370 after the new commit Prior blocking issue resolved: The dropped-middleware-headers regression on the out-of-basePath 404 is fixed — One new non-blocking finding (verified empirically): This commit gates RSC cache-busting behind basePath, but the "claimed" cache-busting check ( Verdict: No blocking issues. The prior blocking issue is properly fixed and tested. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): honor basePath false rewrites (#2370)
Re-reviewed after the new commit 7040f3f ("validate late basePath rewrites"), the only change since the last review round. Verified empirically: all touched test files pass (200 tests across app-rsc-handler, app-rsc-request-normalization, app-browser-server-action-client, entry-templates) and vp check on app-rsc-handler.ts is clean.
The prior blocking + non-blocking items are resolved
The previously-flagged RSC cache-busting inconsistency is fixed. The "claimed" cache-busting check now also runs inside the afterFiles (app-rsc-handler.ts:918-924) and fallback (:968-974) loops, immediately after each late rewrite sets outOfBasePathRequestClaimed = true — mirroring the beforeFiles check at :655-661. The new parameterized test validates out-of-basePath RSC requests claimed by %s rewrites (tests/app-rsc-handler.test.ts:286) asserts the 307 + recomputed _rsc for both phases. I confirmed resolveInvalidRscCacheBustingRequest early-returns for non-GET/HEAD (app-rsc-cache-busting.ts:350-351), so the omission of this check from the new action loop is correct.
New behavior: out-of-basePath Server Actions now re-run late rewrites
7040f3f adds a dedicated pre-dispatch rewrite loop (:667-718) so an unclaimed out-of-basePath action POST (isPost && actionId) can be claimed by an afterFiles/fallback basePath: false rewrite and dispatched against the rewritten cleanPathname. The subsequent main rewrite loops are then skipped via the new resolvedLateRewritesForAction guard (:898, :948) so rewrites aren't applied twice. I traced this and it's correct:
- The early POST 404 at
:795no longer fires once the loop claims the request. - A genuinely-unclaimed action POST (no matching late rewrite) leaves
outOfBasePathRequestClaimedfalse and still 404s at:795. - An action POST whose late rewrite source matches but whose destination has no route (
actionMatchnull) is claimed, dispatch returns null,matchstays null, and it falls through torenderNotFoundat:1015— reasonable.
The new test (:265) covers both phases and asserts handleServerActionRequest is called with the rewritten cleanPathname: "/about".
Blocking-ish: stale design comment now contradicts the code
app-rsc-handler.ts:786-787 still says:
For action requests we intentionally do not re-run rewrites — actions are always processed against the cleanPathname they were posted to.
That was accurate as of 1cd6c8b (and was explicitly called out in the prior review as the intentional action-POST-vs-route-handler asymmetry), but 7040f3f reverses it: out-of-basePath action POSTs do now re-run afterFiles/fallback rewrites before dispatch (:667-718), and the new test asserts an action posted to /outside is dispatched against the rewritten /about. The comment now directly contradicts the loop a few lines above it. Please update it to describe the actual behavior (out-of-basePath actions re-run late rewrites to claim the request; in-basePath actions are processed against the posted cleanPathname), so future readers aren't misled. This is the same class of stale-comment issue the earlier rounds removed for the #1333 notes.
Non-blocking: action loop duplicates the main rewrite loops
The new :667-718 loop is a near-copy of the afterFiles/fallback loops at :898-990 (same applyRewrite args, mergeRewriteQuery, pathnameForResolvedUrl, matchRoute cadence), differing only in that it omits renderPagesForMatchKind and the cache-busting check (both correctly N/A for action POSTs). Two copies of the rewrite-claim sequence are easy to drift out of sync on a future fix. Per AGENTS.md's "generated entries should stay thin / move real behavior into typed helpers" guidance, consider extracting a small claimViaLateRewrites({ rewrites, ... }) helper that both call. Not required for this PR.
Non-blocking (pre-existing, not introduced here): action POST → metadata/public-file via late rewrite
Because the action loop runs before the image/metadata/public-file handlers (:720-760) and handleMetadataRouteRequest doesn't gate on method, an out-of-basePath action POST rewritten onto e.g. /sitemap.xml would serve the metadata route on a POST. This mirrors the existing in-basePath ordering (the metadata handler already runs before action dispatch for claimed requests), so it's not a regression from this PR — just worth a tracking note if method-gating metadata routes is ever desired.
Previously verified, still holds
- Out-of-basePath leak fix: image opt (
:720), metadata (:735), public files (:749), route match (:788), POST/action dispatch (:795), andrenderPagesForMatchKind(:867) are all gated byoutOfBasePathRequestClaimed; the short-circuit 404 at:1008-1012merges middleware headers. - Late
afterFiles/fallbackrewrites set the claim flag beforerenderPagesForMatchKind, so route-handler/Pages handoffs work. resolveActionRedirectTargethard-navigates same-origin redirects leaving the basePath;hasBasePathmatches on segment boundaries./__vinext/prerender endpoints handled at:519-534before any basePath gate, so the action POST loop doesn't intercept them.normalizeRscRequest'sallowOutsideBasePathdefaults tofalse; both stale#1333comments remain removed.
Verdict
The new commit correctly resolves the prior non-blocking RSC cache-busting finding (with tests for both phases) and adds correct, tested late-rewrite support for out-of-basePath action POSTs. The one thing worth fixing in this PR is the stale comment at :786-787, which now contradicts the new action-rewrite behavior. The loop duplication and the metadata-on-POST note are non-blocking.
|
Review posted to PR #2370. SummaryI re-reviewed PR #2370 focused on the new commit Prior non-blocking finding resolved: The RSC cache-busting inconsistency is fixed — the claimed cache-busting check now runs inside the New behavior verified correct: A dedicated pre-dispatch loop ( One thing worth fixing in this PR (blocking-ish): The design comment at Non-blocking: the new action loop is a near-duplicate of the main Verdict: Commit is correct and tested; only the stale comment at |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): honor basePath false rewrites (#2370)
Re-reviewed after the new commit 8c23bfe ("docs(app-router): clarify late action rewrites"), the only change since the last review round. Verified empirically: 166 tests pass across the three touched server test files (app-rsc-handler, app-rsc-request-normalization, app-browser-server-action-client) and vp check on app-rsc-handler.ts is clean.
Prior blocking item resolved
The stale design comment flagged last round (app-rsc-handler.ts:786-787) is fixed. It previously said:
For action requests we intentionally do not re-run rewrites — actions are always processed against the cleanPathname they were posted to.
…which contradicted the late-action-rewrite loop added in 7040f3f. The new comment (:782-786) now accurately describes the behavior:
The route is matched against the current cleanPathname here. Ordinary requests may still be rewritten by the afterFiles / fallback loops below, where the second
setRootParamscall replaces this value before rendering. Out-of-basePath Server Actions resolve those late rewrites above so this match already uses their claimed destination.
I grepped for the prior stale-comment patterns (does not yet support, #1333, intentionally do not re-run, 404s out-of-basePath) — none remain. The comment is correct: out-of-basePath action POSTs are claimed by the pre-dispatch loop at :667-718 (which sets resolvedLateRewritesForAction), so preActionMatch at :787 already sees the rewritten destination.
Full re-verification (all still holds)
- Out-of-basePath leak fix: image opt (
:720), metadata routes (:735), public files (:749), route match (:787), POST/action dispatch (:794), andrenderPagesForMatchKind(:866) are all gated byoutOfBasePathRequestClaimed. - Late rewrite claims:
beforeFiles(:649), the action pre-dispatch loop (:686/:710),afterFiles(:916), andfallback(:966) all flip the claim flag and (for non-basePath) run the claimed RSC cache-busting check (:655-661,:917-923,:967-973). - Action loop guard:
resolvedLateRewritesForAction(:717) correctly suppresses the laterafterFiles(:897) andfallback(:947) loops so rewrites aren't applied twice. - 404 semantics: both not-found paths (
:1007-1012short-circuit,:1037-1039post-render) merge middleware headers viamergeMiddlewareResponseHeaders, keeping out-of-basePath and in-basePath 404s consistent. - Trailing-slash redirect suppressed for out-of-basePath (
:536); redirect path usesredirectDestinationWithBasePathwithbasePathStateand skips the prefix when!hadBasePath. /__vinext/prerender endpoints handled at:519-534before any basePath gate.normalizeRscRequestallowOutsideBasePathis opt-in (defaults false; only the App handler opts in via thetruearg at:491).resolveActionRedirectTargethard-navigates same-origin redirects leaving the basePath; matches the ported Next.js v16.2.6 test.
Non-blocking (carried over, not introduced by this commit)
- Action-loop duplication: the
:667-718pre-dispatch loop is still a near-copy of theafterFiles/fallbackloops at:897-989. Per AGENTS.md's "move real behavior into typed helpers" guidance, extracting a sharedclaimViaLateRewrites(...)helper would reduce drift risk. Not required for this PR. - Metadata-on-POST ordering: an out-of-basePath action POST rewritten onto a metadata route (e.g.
/sitemap.xml) would serve it on POST, mirroring the existing in-basePath ordering — not a regression here.
Verdict
The prior blocking stale-comment issue is fully resolved and the comment now matches the implementation. No blocking issues. The two non-blocking items are pre-existing and reasonable to defer. LGTM.
|
Review posted to PR #2370. SummaryI re-reviewed PR #2370 after the new commit Prior blocking item resolved: The stale comment at Full re-verification: The out-of-basePath leak fix, late-rewrite claim flags, the Two non-blocking items (pre-existing, not introduced by this commit): the action-loop duplication (candidate for a shared helper) and metadata-on-POST ordering. Verdict: LGTM — no blocking issues. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): honor basePath false rewrites (#2370)
Re-reviewed after the new commit 6d9ecd5 ("resolve progressive action rewrites"), the only change since the last review round. Verified empirically: 202 tests pass across the four touched test files (app-rsc-handler 105, app-browser-server-action-client 2, app-rsc-request-normalization 61, entry-templates 34), and vp check on app-rsc-handler.ts is clean.
The new commit is correct
6d9ecd5 extends the early out-of-basePath late-rewrite loop so a progressive Server Action (multipart POST with no actionId) can be claimed by an afterFiles/fallback basePath: false rewrite, mirroring what 7040f3f already did for actionId-based actions. Two changes:
app-rsc-handler.ts:666-668—contentTypeis hoisted above the loop and a newisProgressiveActionRequest = isPostRequest && !actionId && contentType.startsWith("multipart/form-data")is computed. This signature exactly matches the existing progressive-action detection used by the dispatch block (:803) andapp-action-request.ts:15, so it's parity-consistent — a multipart POST without an action header is the canonical progressive-action shape in Next.js.app-rsc-handler.ts:670— the early claim loop guard widens fromisPostRequest && actionIdto(actionId || isProgressiveActionRequest), so a progressive POST now also resolves late rewrites before dispatch and setsresolvedLateRewritesForActionto suppress the duplicateafterFiles/fallbackloops below (:898,:948).
I traced the boundaries and they hold:
- In-basePath unaffected.
outOfBasePathRequestClaimedstarts ashadBasePath(:594), so for in-basePath requests it'strueand the early loop (gated by!outOfBasePathRequestClaimed) never runs. The change is strictly out-of-basePath scoped. - Genuinely-unclaimed progressive POST still 404s. A multipart POST to an out-of-basePath path with no matching late rewrite leaves
outOfBasePathRequestClaimedfalse: the early action 404 at:795doesn't fire (it'sactionId-gated andactionIdis null), the progressive dispatch block at:800requiresoutOfBasePathRequestClaimed(false) and is skipped, and the request falls through to the unclaimed short-circuit 404 at:1008-1012with middleware headers merged. Correct. contentTypeordering is safe — it's now declared once at:666and every read (:668,:803,:810,:849) is after the declaration; the old:792declaration was removed.
The new parameterized test allows out-of-basePath progressive Server Actions through %s rewrites (tests/app-rsc-handler.test.ts:286) covers both phases and asserts handleProgressiveActionRequest is called with the rewritten cleanPathname: "/about". Good coverage of the positive case.
Non-blocking: no negative test for the unclaimed progressive POST
The existing does not dispatch server actions directly outside basePath test (:396) only covers the actionId path. The widened early-loop guard now also admits multipart-no-actionId POSTs, but there's no test asserting that an unclaimed out-of-basePath progressive POST (multipart, no matching basePath: false rewrite) still 404s without invoking handleProgressiveActionRequest. I confirmed this behavior holds by tracing the code, but a one-line negative test would lock the boundary, mirroring the existing actionId negative test. Not required for this PR.
Non-blocking (carried over, not introduced here)
- Action-loop duplication: the
:670-721early loop remains a near-copy of theafterFiles/fallbackloops at:898-990. Per AGENTS.md's "move real behavior into typed helpers" guidance, a sharedclaimViaLateRewrites(...)helper would reduce drift risk — and this commit slightly increased the surface that depends on the two staying in sync. Still a follow-up, not a blocker. - Metadata-on-POST ordering: an out-of-basePath progressive POST rewritten onto a metadata route would serve it on POST, mirroring the existing in-basePath ordering — not a regression here.
Previously verified, still holds
- Out-of-basePath leak fix: image opt (
:723), metadata routes (:738), public files (:752), route match (:790), POST/action dispatch (:795), andrenderPagesForMatchKind(:867) are all gated byoutOfBasePathRequestClaimed. - Both not-found paths (
:1008-1012short-circuit,:1037-1040post-render) merge middleware headers, keeping out-of-basePath and in-basePath 404s consistent. - Claimed RSC cache-busting runs after
beforeFiles(:655-661),afterFiles(:918-924), andfallback(:968-974); correctly omitted from the action loop since actions are POST (non-GET/HEAD). resolvedLateRewritesForAction(:720) suppresses the laterafterFiles/fallbackloops so rewrites aren't applied twice.- Stale
#1333comments remain removed;redirectDestinationWithBasePathskips the prefix when!hadBasePath;/__vinext/prerender endpoints handled before any basePath gate;resolveActionRedirectTargethard-navigates same-origin redirects leaving the basePath.
Verdict
The new commit correctly and symmetrically extends late-rewrite claiming to progressive Server Actions, matching the prior actionId work, and is well-tested for the positive case. No blocking issues. The only suggestion is a negative test for the unclaimed progressive POST; the loop-duplication and metadata-on-POST items are pre-existing follow-ups. LGTM.
|
Review posted to PR #2370. SummaryI re-reviewed PR #2370 after the new commit The new commit is correct. It symmetrically extends the early out-of-basePath late-rewrite loop so a progressive Server Action (multipart POST without
No blocking issues. Non-blocking suggestions:
Verdict: LGTM. |
1 participant
Summary
basePath: falseto handle requests outside the configured mountNext.js parity
Ported from Next.js v16.2.6:
test/e2e/app-dir/app-basepath/index.test.tsFixes the run 28143992598 failure:
should redirect externally when encountering absolute URLs on the same host outside the basePathValidation
vp test run tests/app-rsc-request-normalization.test.ts tests/app-rsc-handler.test.ts tests/app-browser-server-action-client.test.ts --maxWorkers=1— 150 passedvp checkon all touched filesvp run vinext#buildtest/e2e/app-dir/app-basepath/index.test.ts— 13/13 passed