fix(app-router): apply trailingSlash to route handler request URL (#1827)#1835
Conversation
) App Router route handlers received a NextRequest whose `url` was normalized with the default `trailingSlash: false` policy, so a request to `/runtime/edge/` (with `trailingSlash: true`) surfaced `req.url` without the trailing slash even though `req.nextUrl.pathname` kept it. Thread the configured `trailingSlash` through the route-handler dispatch chain (dispatch -> execution/cache -> tracked request -> NextRequest) so `NextURL` applies the correct policy when computing `request.url`, matching Next.js (test/e2e/app-dir/app-routes-trailing-slash). Closes #1827
|
/bigbonk review for issues |
commit: |
|
There was a problem hiding this comment.
Review: fix(app-router): apply trailingSlash to route handler request URL
Verdict: LGTM. The fix is correct, minimal, and complete. Verified by tracing the full dispatch chain and running the new + adjacent suites.
What I verified
Threading is complete and consistent. trailingSlash is wired through every hop and both branches of the dispatch:
app-rsc-entry.ts:753passes__trailingSlash(already defined atapp-rsc-entry.ts:516, used elsewhere) intodispatchAppRouteHandler.dispatchAppRouteHandlerforwards it on both paths: the cache/revalidation read atapp-route-handler-dispatch.ts:202and the direct execution path at:260.- The cache revalidation path forwards it into
runAppRouteHandler(app-route-handler-cache.ts:116). runAppRouteHandlerpasses it tocreateTrackedAppRouteRequest(app-route-handler-execution.ts:132).buildNextConfigincludes it and now returns non-null when onlytrailingSlashis set (app-route-handler-runtime.ts:123).
The downstream consumption is sound. nextConfig.trailingSlash flows into new NextRequest(..., { nextConfig }) → NextURL config (shims/server.ts:130) → this._url = this._nextUrl.toString() (shims/server.ts:134-136), which applies the trailing-slash policy to req.url. This is exactly the field the bug report identified as missing.
Single dispatch path → dev/prod/CF parity holds. createTrackedAppRouteRequest has exactly one call site, and route-handler dispatch flows only through the shared codegen entry (dispatchMatchedRouteHandler → __dispatchAppRouteHandler). There is no separate prod-only or Pages-Router route-handler dispatch that needs the same fix, so the AGENTS.md dev/prod-parity concern does not apply here.
Common case unchanged. buildNextConfig still returns null when trailingSlash is false/unset and no basePath/i18n is configured, so existing behavior is preserved.
Checks run locally
vp test run tests/app-route-handler-trailing-slash.test.ts— 4 passed (both edge + node, true + false).vp test run tests/app-router.test.ts tests/routing.test.ts— 112 + routing passed (new fixtures don't regress route scanning).vp test run tests/app-router-next-config-codegen.test.ts tests/entry-templates.test.ts— 50 passed (codegen entry change is benign).vp checkon all 5 changed source files + the test — format/lint/types clean.
Minor (non-blocking)
- Coverage asymmetry: the
trailingSlash: truecase usesit.each(["edge", "node"]), but thetrailingSlash: falsecase only exercisesedge. Addingnodeto the false case would make the matrix symmetric. Not required — it mirrors the upstream test shape. - Type optionality:
trailingSlash?: booleanis threaded as optional throughout (matching the existingi18npattern), which is fine. No action needed.
No correctness, edge-case, or parity issues found.
|
Review posted to PR #1835. Summary of my findings: Verdict: LGTMThe fix is correct, minimal, and complete. I traced the full dispatch chain and ran the relevant suites. What's correct:
Checks run locally (all green):
Minor non-blocking notes:
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk review — fix(app-router): apply trailingSlash to route handler request URL
Verdict: LGTM. Independently re-traced the dispatch chain, ran the suites, and checked for parity/edge-case gaps. The fix is correct, minimal, and complete.
Threading is complete on every hop and both branches
app-rsc-entry.ts:753passes__trailingSlashinto__dispatchAppRouteHandlerviadispatchMatchedRouteHandler.__trailingSlashis the same value already defined atapp-rsc-entry.ts:516and used by the top-level handler config (:948), so no new state is introduced.dispatchAppRouteHandlerforwards it on both paths: the cache/revalidation read (app-route-handler-dispatch.ts:202) and direct execution (:260).- Cache path →
runAppRouteHandler(app-route-handler-cache.ts:114); direct path →executeAppRouteHandler, which spreads...optionsintorunAppRouteHandler. ConfirmedExecuteAppRouteHandlerOptions = {...} & RunAppRouteHandlerOptions(app-route-handler-execution.ts:109), sotrailingSlashrides the intersection — no silent drop. runAppRouteHandler→createTrackedAppRouteRequest(app-route-handler-execution.ts:132) →buildNextConfig.
Downstream consumption is the field the bug flagged
buildNextConfig now includes trailingSlash → new NextRequest(..., { nextConfig }) → NextURLConfig.nextConfig.trailingSlash (shims/server.ts:130) → this._url = this._nextUrl.toString() (shims/server.ts:134-136). That's exactly req.url, the value the report identified as missing.
Common case preserved
buildNextConfig still returns null when trailingSlash is falsy and no basePath/i18n is set (app-route-handler-runtime.ts:123), so the default path is unchanged.
Parity holds
createTrackedAppRouteRequest has one call site, and app-router route-handler dispatch flows only through the shared codegen entry — covering dev/prod/CF. Edge vs node route handlers share this dispatch (the runtime = "edge" segment config affects the x-edge-runtime header, not a separate dispatch fork), so one threading point covers both. No separate prod-only path needs the same fix.
No fixture regression
The new app/runtime/{edge,node}/route.ts are added to the shared app-basic fixture under a new runtime/ dir (no collision). No test asserts an exact total route count on app-basic; routing.test.ts re-ran green.
Checks run locally (all green)
tests/app-route-handler-trailing-slash.test.ts— 4 passed (edge + node × true + false)tests/{app-route-handler-runtime,dispatch,execution,cache}.test.ts+tests/trailing-slash.test.ts— 36 passedtests/routing.test.ts— 112 passedvp checkon all 5 source files + the test — format/lint/types clean
Note (resolved)
The earlier coverage-asymmetry note (false case only testing edge) was already addressed by the follow-up commit f1e4090 — both true and false now use it.each(["edge", "node"]). Matrix is symmetric.
No correctness, edge-case, or parity issues found.
|
Review posted to PR #1835. Verdict: LGTMI independently re-traced the full dispatch chain, ran the suites, and checked for parity and edge-case gaps. The fix is correct, minimal, and complete. What I verified:
Checks run (all green): new trailing-slash test (4), the 4 route-handler unit suites + One note (already resolved): the coverage-asymmetry the prior review flagged was fixed by follow-up commit No correctness, edge-case, or parity issues found. |
Problem
The
trailingSlashconfig was not applied to App Router route handlers (app/**/route.ts). A request to/runtime/edge/withtrailingSlash: truesurfacedreq.urlwithout the trailing slash, even thoughreq.nextUrl.pathnamecorrectly retained it.This caused the upstream suite
test/e2e/app-dir/app-routes-trailing-slash(edge + node runtime variants) to fail: the handler readsnew URL(req.url).pathnameand expects/runtime/edge/.Root cause
Route-handler dispatch builds the handler's
NextRequestviacreateTrackedAppRouteRequest->buildNextConfig.buildNextConfigthreadedbasePathandi18ninto theNextRequest'sNextURL, but omittedtrailingSlash. WithtrailingSlashunset,NextURLdefaulted tofalseandNextURL.toString()(used to computeNextRequest.url) stripped the trailing slash fromreq.url.Note: the 308 redirect itself already worked (via the central
normalizeTrailingSlash); only the served request'sreq.urlwas wrong.Fix
Thread the configured
trailingSlashthrough the route-handler dispatch chain soNextURLapplies the correct policy when computingrequest.url:app-rsc-entry(codegen) ->dispatchAppRouteHandler->executeAppRouteHandler/ cache revalidation ->runAppRouteHandler->createTrackedAppRouteRequest->buildNextConfig->NextRequest/NextURL.This mirrors the existing
i18nthreading exactly. WhentrailingSlashisfalse(default) and no basePath/i18n is set,buildNextConfigstill returnsnullas before, so the common case is unchanged.The codegen entry (
app-rsc-entry.ts) is shared by dev, prod, and Cloudflare Workers runtimes, so all three are covered by the single change.Test
Added
tests/app-route-handler-trailing-slash.test.ts(mirrors the upstream test) plus fixture route handlerstests/fixtures/app-basic/app/runtime/{edge,node}/route.ts. Covers:trailingSlash: true:/runtime/{edge,node}-> 308 ->/runtime/{edge,node}/, then 200 with the handler observing the slashedurlandnextUrl.trailingSlash: false:/runtime/edge/-> 308 ->/runtime/edge, then 200.Confirmed the test fails before the fix and passes after. Existing
request-pipeline,trailing-slash, and route-handler dispatch/execution/cache/runtime suites remain green.Closes #1827