refactor(pages-router): unify triplicated request pipeline into runPagesRequest (#1782)#1860
Conversation
…ests Introduces server/pages-request-pipeline.ts which owns the canonical Next.js 9-step request ordering once. Returns a PagesPipelineResult discriminated union: type:response for prod/worker callers that supply render/api callbacks, type:render|api|next intents for dev callers that omit them. Mirrors createAppRscHandler's thin-closure injection pattern. Two latent drift bugs reconciled vs the worker copy: - middlewareStatus now uses result.status ?? result.rewriteStatus - applyMiddlewareRequestHeaders now passes preserveCredentialHeaders Adds 28 focused behavior tests covering all pipeline branches. Adds ./server/pages-request-pipeline export to package.json.
…sRequest Replaces the ~400-line inlined try block in startPagesRouterServer with a thin adapter that calls runPagesRequest(webRequest, deps). The adapter retains only Node-specific concerns: open-redirect guard, decode/400, prerender endpoint, static assets, image opt, basePath strip, _next/data normalization, and compression/streaming output. Extends renderPage callback signature with optional stagedHeaders param so CSP nonces and other pre-render header injection continue to work. Copies __vinextStreamedHtmlResponse marker through mergeHeaders to preserve stream-vs-buffer decision in the adapter. 276 pages-router tests, 112 routing tests, 28 pipeline tests all pass.
… to runPagesRequest Collapses the ~280-line inlined fetch handler into ~65 lines. Worker adapter keeps only: registerConfiguredCacheAdapters, open-redirect guard, static-asset 404, x-nextjs-data header capture, filterInternalHeaders+cloneRequestWithHeaders, basePath strip with hadBasePath, and image optimization. Delegates the rest to runPagesRequest via PagesPipelineDeps. Removes inlined imports: matchRedirect, matchRewrite, preserveRedirectDestinationQuery, requestContextFromRequest, applyMiddlewareRequestHeaders, isExternalUrl, proxyExternalRequest, sanitizeDestination, applyConfigHeadersToHeaderRecord, normalizeTrailingSlash, mergeHeaders, normalizeDefaultLocalePathname, stripI18nLocaleForApiRoute, mergeRewriteQuery. Updates deploy.test.ts assertions to verify delegation rather than inlined step logic. 245 deploy tests pass.
…te parallel helpers Replaces the inlined dev Pages Router pipeline with runPagesRequest. The adapter keeps Node-specific concerns: Vite skips, cross-origin guard, image 302 redirect, .html normalization, open-redirect guard, decode/400, basePath strip with hadBasePath, trailing-slash, _next/data normalization, rawHeaders snapshot, cloudflare-plugin delegation. Creates devRunMiddlewareAdapter closure that wraps the dev runMiddleware (runner, path, i18n, basePath, trailingSlash, isDataRequest args) and returns a MiddlewareResult-compatible object. Sets VINEXT_MW_CTX_HEADER for hybrid app+pages mode as a side effect before returning. Adds writeWebResponseToNodeRes() helper to stream Web Response to Node res preserving multi-value Set-Cookie. Adds requestHeaders field to render/api intents so dev adapter can flush post-middleware request headers to req.headers before calling createSSRHandler/handleApiRoute. Deletes: - applyRedirects (lines ~4829-4869) - proxyExternalRewriteNode (lines ~4867-4928) - applyRewrites (lines ~4929-4951) - applyHeaders (lines ~4951-5015) 276 pages-router tests, 112 routing tests, 245 deploy tests, 28 pipeline tests all pass. vp check clean.
commit: |
…y/headers; update stale after-deploy assertions Three regressions introduced by the dev pipeline refactor: 1. Hybrid app+pages dev mode: the 'render' intent branch in handlePagesMiddleware unconditionally called createSSRHandler, missing the original hasAppDir fallthrough gate. App Router page requests in the hybrid app-basic fixture returned 404 instead of being deferred to the RSC plugin. Restored by matching the route before calling the Pages SSR handler and returning next() when there is no pages match and hasAppDir is true. 2. External proxy body/headers: proxyExternalRewriteNode read the body from the Node IncomingMessage and the post-middleware headers from req. The refactored pipeline used a body-less webRequest, so POST bodies were empty and middleware-modified request headers (x-hello-from-middleware1 etc.) were not forwarded. Fixed by adding an optional proxyExternal dep to PagesPipelineDeps; the dev adapter supplies it, building a proper Request from the pipeline's current (post-middleware) headers and the Node req body stream. Prod and worker adapters fall through to proxyExternalRequest as before. 3. Stale after-deploy assertions: two tests checked old inline call-site strings that no longer appear in the generated worker entry after the delegation refactor. ctx is still forwarded (deploy.ts:654,657); updated assertions to match the new call sites.
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/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 refactor end-to-end against the three original copies (prod-server, worker template, dev middleware) and the new runPagesRequest owner. The consolidation is well-structured, the discriminated-union intent design is clean, and the new pipeline test suite is thorough. Ran tests/pages-request-pipeline.test.ts (28 ✓), tests/deploy.test.ts (245 ✓), tests/after-deploy.test.ts (9 ✓), and the pages-router content-type/octet-stream cases — all green.
The documented reconciliations (result.status ?? result.rewriteStatus and preserveCredentialHeaders on the worker path) and the body/headers preservation through applyMiddlewareRequestHeaders for external proxying both check out. Dev hybrid hasAppDir gating and afterFiles/fallback ordering are preserved.
One genuine behavioral divergence to flag (non-blocking), plus a couple of minor notes below.
| const responseHeaders = mergeResponseHeaders({}, mergedResponse); | ||
| const finalStatusText = mergedResponse.statusText || undefined; | ||
| const responseBody = Buffer.from(await response.arrayBuffer()); | ||
| const ct = response.headers.get("content-type") ?? "text/html"; |
There was a problem hiding this comment.
Behavioral regression: lost application/octet-stream default for API routes.
The pre-refactor prod-server had two distinct content-type fallbacks: API routes defaulted to application/octet-stream (old line 1945, with an explicit comment: "API routes may return arbitrary data (JSON, binary, etc.), so default to application/octet-stream rather than text/html when the handler doesn't set an explicit Content-Type"), while page routes defaulted to text/html (old line 2068).
This unified branch now handles both API and page responses with a single ?? "text/html" fallback. For an API response that reaches the adapter with no content-type header, the body is now served as text/html instead of application/octet-stream — the exact content-sniffing/XSS hazard the original code deliberately avoided.
In practice api-handler.ts sets sensible defaults (text/plain/octet-stream/json) for res.end(...)/res.send(...), so the existing tests/pages-router.test.ts:4547 (not.toContain("text/html")) still passes. But the safety net for a truly content-type-less API response is gone. Since the pipeline's {type:"response"} result doesn't distinguish API vs page, consider either tagging the API path in the result or restoring the octet-stream default for non-HTML responses. At minimum this divergence should be called out in the PR's "Behavioral decisions" section rather than dropped silently.
| requestHeaders: Headers; | ||
| middlewareStatus: number | undefined; | ||
| } | ||
| | { type: "next" }; // dev passthrough (no matching route, has appDir) |
There was a problem hiding this comment.
{ type: "next" } is declared in the result union but runPagesRequest never returns it — the dev next() fallthrough is decided in the adapter (index.ts:3598/3583) via hasAppDir after the render/api intent. The prod/worker adapters also have dead "render"|"api"|"next" guard branches.
This is fine as forward-compat surface, but worth a one-line comment here noting the variant is currently unused (reserved for a future dev passthrough) so a reader doesn't go hunting for where it's produced.
| } | ||
|
|
||
| // Step 8: External-URL proxy (post-mw rewrite target) | ||
| if (isExternalUrl(resolvedUrl)) { |
There was a problem hiding this comment.
Minor: the external-proxy merge asymmetry is preserved here (post-middleware rewrite path merges middleware headers via mergeHeaders(proxyResponse, middlewareHeaders, undefined), while the beforeFiles/afterFiles/fallback external paths at lines 337/389/435/478 return the raw proxyExternal(...) response without merging). The PR body documents this as intentional ("both prod and worker already agreed on this"), which matches what I see in the pre-refactor copies — just confirming it's deliberate and noting a short inline comment on the bare proxyExternal returns would help future maintainers not "fix" it into an accidental divergence.
|
Review posted successfully to PR #1860. SummaryI reviewed the refactor against all three original copies (prod-server, worker template, dev middleware) and the new Overall: Solid consolidation. The discriminated-union intent design, the documented reconciliations ( Findings posted (3 inline comments, none blocking):
|
…te dev api flushes
Three behavioral regressions from the pipeline extraction, plus the bonk
review findings:
- prod-server (Node): restore post-middleware public-directory static file
serving (original step 5b). The adapter now supplies a serveStaticFile dep
that the pipeline calls after middleware (so middleware can intercept) and
before rewrites. Without it, public/ files (favicon.ico, robots.txt, etc.)
fell through to the renderer and 404'd. Adds a {type:"handled"} result for
callbacks that write their own output.
- prod-server (Node): restore the application/octet-stream content-type default
for API responses. The unified response path collapsed API and page fallbacks
to text/html; the pipeline now tags API results with isApiResponse so the Node
adapter picks octet-stream (arbitrary data) over text/html (content-sniffing
hazard). [bonk finding 1]
- index.ts (dev): gate the staged-header/req-header flushes behind a pages-api
match, mirroring the original 'if (apiMatch)' guard. The unconditional
flushRequestHeaders() wiped req.headers (incl. VINEXT_MW_CTX_HEADER) on the
api-miss -> next() path, dropping middleware context for app/api/* routes in
hybrid app+pages dev mode.
- pipeline: note {type:"next"} is reserved/unused [bonk finding 2]; document
the intentional external-proxy merge asymmetry on the bare proxyExternal
returns [bonk finding 3].
Adds 5 pipeline tests (serveStaticFile/handled ordering, isApiResponse tag).
|
Addressed the bonk findings plus two further regressions found while reviewing the extraction against all three original copies. Pushed in 568dead. Bonk findings
Two additional regressions fixed Tests: +5 pipeline cases (serveStaticFile/handled ordering, isApiResponse tag); 33 pipeline, 276 pages-router, 245 deploy, 9 after-deploy, 112 routing all pass locally. |
|
/bigbonk review for issues |
…data-request defer Two divergences found in code review of the extraction: - deploy.ts (worker): the renderPage adapter dropped the pipeline's 4th stagedHeaders arg, passing undefined for the SSR renderer's middlewareHeaders param — so middleware-set CSP nonces were never applied to rendered HTML on the worker, unlike the prod path. Widen the wrapper to forward stagedHeaders. - pipeline: shouldDeferErrorPageOnMiss only gated on isDataReq, which the worker never sets (it doesn't normalize /_next/data paths). The pre-refactor worker gated on the x-nextjs-data header (isDataRequest), so a data-request miss now wrongly deferred + ran fallback rewrites + re-rendered. Gate on both signals; no-op for Node/dev where a data request already has isDataReq=true. Updates the after-deploy assertion to the new call site and adds a pipeline test for the data-request defer gate.
…ct query, dedup) - prod content-type: passthrough responses (middleware short-circuits, external proxies) no longer get a text/html default injected. Generalize the response tag to defaultContentType (render -> text/html, api -> octet-stream, passthrough -> unset); the Node adapter sends untagged responses verbatim via sendWebResponse, matching pre-refactor behavior. - redirect query: config-redirect Location now uses the raw req.url query (via a new rawSearch dep supplied by the Node prod + dev adapters) instead of new URL().search, which re-encoded chars and truncated at a literal '#'. The worker keeps url.search (it only ever had a Web Request). Restores the original prod/dev behavior. - efficiency: matchPageRoute no longer runs twice per request on the common no-afterFiles-rewrite path; reuse the Step 12 match unless afterFiles changed the pathname. - dedup: collapse prod-server's mergeWebResponse into the canonical mergeHeaders (worker-utils) via delegation, removing the duplicate implementation and the now-orphaned hasHeader/stripHeaders helpers. Existing mergeWebResponse tests validate the delegation. Updates pipeline tests for defaultContentType and adds a passthrough-verbatim assertion. 36 pipeline + 309 features + 276 pages-router + 245 deploy + 9 after-deploy + 112 routing pass; vp check clean.
|
Addressed the remaining code-review findings in dba28a6 (the two worker divergences were already fixed in 448764d):
3 review candidates were verified and dropped as refuted: dev Tests: 36 pipeline + 309 features + 276 pages-router + 245 deploy + 9 after-deploy + 112 routing pass; |
|
/bigbonk review for issues |
# Conflicts: # packages/vinext/src/index.ts
|
/bigbonk review for issues |
There was a problem hiding this comment.
Reviewed the full refactor against all three pre-refactor copies (prod-server Node, deploy.ts worker template, index.ts dev middleware) and the new runPagesRequest owner, plus the new pipeline test suite. Ran tests/pages-request-pipeline.test.ts (34), tests/deploy.test.ts (245), tests/after-deploy.test.ts (9), and tests/pages-router.test.ts (279) — all green.
This is a clean, well-structured consolidation. Verified specifically:
- Content-type regression fixes (all three) are correct and tested. Render →
text/html, API →application/octet-stream, passthrough (middleware short-circuit / external proxy / redirect) →undefined(sent verbatim). The prod adapter'sshouldStream || !response.body || defaultContentType === undefinedbranch matches the pre-refactor render (shouldStream || !body) and API (!body) verbatim-vs-buffer logic exactly. Tests at pipeline lines 181/379/387 cover all three. mergeWebResponse→mergeHeaderscollapse is behavior-identical. Header precedence (response wins for non-Set-Cookie; Set-Cookie = middleware cookies first, then response cookies), no-body status dropping, and streamed content-length stripping all match the originalmergeResponseHeaders+ inline logic.- Reconciled deltas verified against pre-refactor copies.
result.status ?? result.rewriteStatusandpreserveCredentialHeaders: isExternalUrl(resolvedUrl)are the correct supersets; the worker previously usedurl.searchfor redirect Locations (confirmed in HEAD~11deploy.ts:711), so omittingrawSearchon the worker path is correct. - Dev regression #5 fix is correct. The
if (apiMatch)gate aroundflushStagedHeaders()/flushRequestHeaders()mirrors the original exactly, preservingVINEXT_MW_CTX_HEADERon the app-passthroughnext()path. - App-passthrough refutations are sound.
app-middleware.ts:198reads the rewrite from the mw-ctxrfield (notreq.url), so not updatingreq.urlbefore the devnext()is correct.
One minor edge-case note below (non-blocking). Nice work.
| // callback owns the path guards (skip "/", "/api/", and the asset-prefix dir) and | ||
| // writes directly to Node `res`, so a `true` result means the response is already sent. | ||
| if (deps.serveStaticFile) { | ||
| const served = await deps.serveStaticFile(pathname, middlewareHeaders); |
There was a problem hiding this comment.
Minor edge-case divergence (non-blocking): serveStaticFile is called with pathname (derived from the inbound request.url at line 194), which for a _next/data request is the normalized page path (the prod adapter rewrites /_next/data/<buildId>/page.json → /page before building the webRequest). The pre-refactor prod-server computed staticLookupPath from the raw pathname (HEAD~11 prod-server.ts:1532, before the _next/data normalization at :1627), so it would attempt tryServeStatic("/_next/data/<buildId>/page.json") — effectively always a miss for data requests.
With the new ordering, a _next/data request whose normalized page path happens to collide with an extensionless public file could now serve that public file instead of falling through to the data render. Extremely unlikely in practice (public files normally have extensions; the guard already skips /, /api/, and the asset dir), but it is a behavioral change worth a one-line comment here noting that pathname is post-_next/data-normalization, so a future reader doesn't assume it's the raw inbound path.
|
Review posted successfully to PR #1860. SummaryI reviewed this refactor end-to-end against all three pre-refactor copies (prod-server Node, Overall: solid, well-structured consolidation. All three prior
One finding posted (minor, non-blocking):
No blocking issues. |
2 participants
Closes #1782.
The Pages Router request sequence was copy-pasted three times —
prod-server.ts(Node),deploy.ts(Cloudflare Worker template), andindex.ts(Vite dev middleware). This PR collapses all three into one owner that the adapters delegate to.What changed
New:
server/pages-request-pipeline.tsexportsrunPagesRequest(request, deps)which owns the canonical Next.js 9-step ordering once. Returns aPagesPipelineResultdiscriminated union:{type:"response"}for every early return + prod/worker render,{type:"render"|"api"}intents when render callbacks are absent (dev). MirrorscreateAppRscHandler's thin-closure injection pattern.Migrated:
prod-server.tsstartPagesRouterServer— keeps Node-specific concerns: decode/400,_next/datanormalization, static assets, image opt, compression sink. Delegates the 9-step ordering.deploy.tsworker template — keeps worker-specific concerns:registerConfiguredCacheAdapters, static-asset 404, image bindings. Delegates the 9-step ordering.index.tsdev middleware — keeps Vite-specific concerns: Vite skips, cross-origin guard, image 302, basePath strip,_next/datanormalization. Uses adevRunMiddlewareAdapterwrapper to bridge the dev runner API toMiddlewareResult.Deleted:
applyRedirects,applyRewrites,applyHeaders(parallel Node-reshelpers atindex.ts:4829-5015) — these were the exact three functions the issue called out.proxyExternalRewriteNode— external rewrites now return{type:"response"}from the pipeline.Behavioral decisions
Preserved as-is (out of scope): Config-headers-after-middleware timing gap; external-proxy merge asymmetry (merge only on post-middleware rewrite path, not on beforeFiles/afterFiles/fallback external paths — both prod and worker already agreed on this).
Reconciled (latent worker drift fixed): Two subtle differences between the prod and worker copies were unified to the superset:
middlewareStatus = result.status ?? result.rewriteStatus(worker previously droppedresult.status)preserveCredentialHeaders: isExternalUrl(resolvedUrl)passed toapplyMiddlewareRequestHeaders(worker previously omitted this)renderPagesignature extension: Added optionalstagedHeaders?: Headersparameter so the prod adapter can pass pre-staged middleware headers to the SSR renderer for CSP nonce injection — same information that was previously passed asmiddlewareResponseHeadersin the old inline code.Tests
tests/pages-request-pipeline.test.ts(new, 28 tests): drivesrunPagesRequestdirectly. Covers step order, redirect 307/308 + query preservation, middleware short-circuits + multi-value Set-Cookie, before/after/fallback rewrites + external proxy, out-of-basePath 404, trailing-slash, staged-header precedence, reconciled deltas, and render/api intent emission.Line counts (net)
prod-server.tsdeploy.tsindex.tspages-request-pipeline.tspages-request-pipeline.test.tsTotal: net ~−450 lines, with clear ownership and focused tests.