fix(pages): render masked basePath error routes by NathanDrake2406 · Pull Request #1441 · cloudflare/vinext

NathanDrake2406 · 2026-05-22T11:21:35Z

Overview

Field	Details
Goal	Match Next.js Pages Router behavior for basePath error pages and masked error-route client navigation.
Core change	Render `pages/404` for production route misses, preserve masked `as` URLs, and fetch `/404` as the component route for `router.push('/404', as)` and `router.push('/_error', as)`.
Primary files	`packages/vinext/src/entries/pages-server-entry.ts`, `packages/vinext/src/server/pages-page-response.ts`, `packages/vinext/src/shims/router.ts`
Expected impact	Apps using `basePath` get custom 404 HTML and Pages Router client transitions hydrate the error component without hard navigating to the masked URL.

Why

For Pages Router navigation, the browser-visible URL and the component route are separate values. Next.js keeps as in history but resolves /404 and /_error as error component routes. Vinext collapsed those values too early, so masked error navigations fetched the friendly URL and production route misses under basePath fell back to generic HTML.

Area	Principle / invariant	What this PR changes
Production route miss	A Pages app with `pages/404` should render that route with status 404.	Route misses try the explicit `/404` page after request normalization and pass the original visible path as `asPath`.
Client navigation	`href` selects the page component, while `as` selects the visible browser URL.	Pages error-route `href`s fetch `/404`, preserve the masked history URL, and allow the 404 response to hydrate.
Hydration metadata	Client navigation needs module URLs in `__NEXT_DATA__` to import the fetched page.	Production Pages HTML includes the resolved page and `_app` module URLs.

What changed

Scenario	Before	After
`/docs/docs/other-page` with `basePath: '/docs'` and `pages/404`	Generic vinext 404 HTML.	Custom `pages/404` HTML, status 404, `__NEXT_DATA__.page === '/404'`.
`next.router.push('/404', '/slug-2')` under `/docs`	Fetched or hard-navigated to the masked URL.	Fetches `/docs/404`, keeps location `/docs/slug-2`, renders the error component.
`next.router.push('/_error', '/slug-2')` under `/docs`	Same collapse of component route and masked URL.	Maps the component fetch to `/docs/404` while preserving the masked URL.

Validation

vp test run tests/shims.test.ts -t "Pages Router (fetches the error route|maps masked /_error)"
vp test run tests/pages-router.test.ts -t "(renders pages/404|falls back to pages/_error)"
vp test run tests/prerender.test.ts -t "prerenderPages — default mode"
vp env exec --node 24 ./scripts/run-nextjs-deploy-suite.sh /Users/nathan/Projects/vinext/.refs/nextjs-v16.2.6 --retries 0 -c 1 --debug test/e2e/basepath/error-pages.test.ts
vp check

Maintainer review path

tests/pages-router.test.ts for the production basePath 404 and _error fallback regressions.
tests/shims.test.ts for the ported masked /404 and /_error client navigation contract.
tests/prerender.test.ts for the custom /404 prerender ownership regression.
packages/vinext/src/shims/router.ts for the narrow Pages error-route exception in client navigation.
packages/vinext/src/entries/pages-server-entry.ts and packages/vinext/src/server/pages-page-response.ts for custom 404 rendering, status propagation, asPath, and module metadata.

Risk / compatibility

Public API: no new public API.
Runtime: the client-side exception is limited to local Pages Router hrefs whose normalized path is /404 or /_error.
Status behavior: explicit /404 page renders with status 404, matching the error route contract.
Existing app risk: route-miss fallback only invokes an actual /404 page route, not a dynamic route that happens to match /404.

References

Reference	Why it matters
Next.js basePath error-pages e2e test	Source test for the masked `/404` and `/_error` client navigation behavior.
Next.js Pages Router client router	Shows the client router preserving `href` and `as` separately and resolving error routes as page components.
Next.js base server error rendering	Shows server-side preference for `pages/404` before falling back to `_error`.
Next.js router server miss handling	Shows route misses invoking the 404 render path with status 404.

Closes #1333

pkg-pr-new · 2026-05-22T11:22:22Z

Open in StackBlitz

npm i https://pkg.pr.new/vinext@1441

commit: f25613e

james-elicx · 2026-05-22T12:04:12Z

/bigbonk review

ask-bonk

Review Summary

Clean, well-scoped fix for basePath error page rendering. The three-pronged approach (server 404 rendering, client error-route fetch URL, prerender dedup) is correct and each piece is independently testable. The tests are well-structured and ported from the right Next.js source.

A couple of observations — one about a missing _error fallback on the server side, and a few minor nits.

Key observations

Server-side: no _error fallback for route misses. The new route-miss block only tries /404. If a Pages app has _error.tsx but no 404.tsx, matchRoute("/404", pageRoutes) returns null and falls through to the generic HTML. Next.js falls back to _error in that case (the base-server.ts reference in the PR description confirms this). The prerender code handles this correctly (hasCustom404 || hasErrorPage), but the server entry does not.
Client-side: resolvePagesErrorHtmlFetchUrl operates on the raw url (href) param, not on as. This is correct — it's the href that selects the component route. Good.
allowNotFoundResponse is a clean extension to navigateClient that avoids changing the existing non-error behavior.
Prerender route.pattern === "/404" skip prevents the double-render issue cleanly.

Overall this is solid work. The _error fallback gap is the main thing worth discussing.

ask-bonk · 2026-05-22T12:06:41Z

Review posted. Here's a summary of the feedback:

Overall: Solid, well-scoped PR. The three-part fix (server rendering, client fetch URL, prerender dedup) is correct and well-tested.

Main discussion point: The server-side route-miss handler only looks for an explicit pages/404 route. If an app has pages/_error but no pages/404, the matchRoute("/404") call returns null and falls through to generic HTML. The prerender code in this same PR handles both cases (hasCustom404 || hasErrorPage), but the server entry doesn't. Worth discussing whether this is intentional scoping or should be addressed here.

Minor nits:

The recursive _renderPage call spreads all original options, which forwards isDataReq — safe today but a subtle invariant. Suggested constructing options explicitly.
navigateOptions ternary uses === null where truthy check would be more idiomatic.
Noted the resolveSsrManifest + getManifestFilesForModule refactor is a nice cleanup.
statusCode correctly takes precedence over gsspRes.statusCode in applyGsspHeaders, matching Next.js behavior.

github run

james-elicx · 2026-05-22T13:18:41Z

/bigbonk review

ask-bonk

Review Summary

Clean, well-structured fix. The three-part approach — server 404/_error rendering, client error-route fetch URL resolution, prerender dedup — is correct and each piece is independently testable. The tests are well-ported from the Next.js reference and the PR description is thorough.

I see the previous review's main concern (missing _error fallback on the server side) has been addressed — the generated entry now falls through from /404 to _errorPageRoute at the else if branch. Good.

A few observations below, mostly minor. The one substantive point is about what happens when both the /404 and _error renders themselves throw — currently the outer catch returns a generic 500, which is fine, but worth being aware of.

LGTM overall.

ask-bonk · 2026-05-22T13:21:47Z

Review posted. Here's a summary:

Approved with minor suggestions. The PR is well-structured and correct.

Key findings:

Previous review's _error fallback concern is resolved — the generated entry now correctly falls through from /404 match to _errorPageRoute in the else if branch at line 532.
The matchRoute("/404") + pattern === "/404" guard is important — without the pattern check, a dynamic catch-all route could match /404. Suggested adding an inline comment explaining why.
The /_error → /404 client fetch mapping chain is clever — when a server only has _error, the client fetches /404 which triggers the server's own route-miss → _errorPageRoute fallback. The end-to-end flow works correctly.
resolveSsrManifest + getManifestFilesForModule refactor is a nice cleanup that deduplicates the manifest lookup logic.
statusCode prop injection correctly scoped to /_error only, matching Next.js behavior.
Prerender status === 404 guard prevents misconfigured 404 pages from being written as 404.html.
Test coverage is solid — both server-side (basePath 404, _error fallback) and client-side (masked /404 and /_error navigations) are covered, with proper Next.js test provenance comments.

github run

Pages Router route misses under basePath returned generic HTML and masked client navigations to /404 or /_error could fetch the visible as-path instead of the error route. This diverged from Next.js when applications used next/router to preserve a friendly URL while rendering the error component. The implementation assumed the browser URL and the component route could always collapse to one fetch target. Preserve the masked history URL, fetch the Pages error route as the HTML target, allow its 404 response to hydrate, and render pages/404 for production route misses with the correct status and client module metadata.

Vitest integration CI does not install Playwright browsers, so launching Chromium from tests/pages-router.test.ts made the PR fail deterministically even though the upstream deploy harness passed. Keep the browser-level verification in the upstream deploy-suite run and cover vinext locally at the lower boundaries: production 404 rendering through HTTP and Pages Router masked error-route navigation through the router shim. The generated Pages entry now also shares one SSR manifest module lookup helper between asset tag collection and client module URL resolution.

The pattern check prevents a dynamic catch-all route (e.g. [...slug]) from being used as the 404 fallback. Without it, matchRoute can return a catch-all match and silently hijack the error rendering path.

…or-pages # Conflicts: # packages/vinext/src/entries/pages-server-entry.ts # packages/vinext/src/shims/router.ts

Copilot

Pull request overview

This PR aligns Vinext’s Pages Router error-route behavior with Next.js when basePath and masked client navigations are involved, ensuring custom pages/404 renders for production misses and that client-side router.push('/404', as) / router.push('/_error', as) fetch the correct component route while preserving the masked URL.

Changes:

Server: on production route misses, prefer rendering an explicit pages/404 (or fall back to pages/_error) with status 404, and pass the missed path through as asPath.
Client router shim: introduce a narrow exception so error-route navigations fetch /404 HTML (optionally allowing 404 responses) while keeping the masked as URL in history.
Hydration metadata: embed resolved page and _app module URLs into __NEXT_DATA__.__vinext so error-route HTML navigations can hydrate without hard navigation.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tests/shims.test.ts	Adds coverage for masked `/404` and `/_error` client navigation under `basePath` (including locale-prefixed paths).
tests/prerender.test.ts	Ensures only one `/404` prerender result is emitted.
tests/pages-router.test.ts	Adds prod-server regression tests for `basePath` route-miss 404 rendering and fallback rewrite ordering.
packages/vinext/src/shims/router.ts	Routes masked error navigations to fetch `/404` HTML while preserving `as`, and allows 404 HTML responses for this path.
packages/vinext/src/server/prod-server.ts	Defers custom error rendering on initial miss to allow fallback rewrites, then renders error page after rewrites if still missing.
packages/vinext/src/server/pages-page-response.ts	Adds status override support and embeds `__vinext` module URL metadata into `__NEXT_DATA__`.
packages/vinext/src/entries/pages-server-entry.ts	Adds `_error` route awareness, improves miss handling to render `/404` (or `_error`), propagates status/asPath, and emits module URL metadata.
packages/vinext/src/deploy.ts	Mirrors prod-server miss handling logic in the Worker entry (defer error page until after fallback rewrites).
packages/vinext/src/build/prerender.ts	Skips `/404` in the generic prerender loop and renders it via the dedicated 404 block expecting a 404 status.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

NathanDrake2406 · 2026-05-23T09:13:19Z

Addressed the ISR status review in e10bfcb. The Pages ISR cache now stores the rendered status from renderPagesPageResponse, cached HIT/STALE responses reuse the cached status with a 200 fallback for legacy entries, and background regeneration preserves the render status as well. Added regression coverage for a revalidating custom pages/404 route miss: first MISS and second HIT both return 404 with the custom 404 HTML.

NathanDrake2406 · 2026-05-23T09:31:50Z

Addressed the stale ISR metadata review in 146343d. resolvePagesPageData now receives the same __vinext module metadata as normal HTML rendering, and renderPagesIsrHtml() passes it into the regenerated __NEXT_DATA__ script. The generated Pages entry computes pageModuleUrl / appModuleUrl before page-data resolution so stale background regeneration can preserve them. Added a focused STALE regression that runs regeneration and asserts the rewritten cached HTML still includes __vinext.pageModuleUrl and appModuleUrl.

james-elicx · 2026-05-26T10:29:01Z

/bigbonk review

ask-bonk

Review Summary

Well-executed fix. The three-layer approach — server route-miss rendering, client error-route fetch URL resolution, and prerender dedup — is correct and each piece is independently testable. The iteration across prior reviews (ISR status preservation, stale metadata propagation, fallback-rewrite gating on route-miss vs status code, data-request exclusion) has addressed all the substantive concerns raised.

The test coverage is thorough: production basePath 404, _error fallback, ISR cache status round-trip, fallback-rewrite ordering, and four client navigation variants (with/without locale, /404 vs /_error). The Next.js test provenance comments are appreciated.

Two minor observations below — neither blocking.

ask-bonk · 2026-05-26T10:31:22Z

+        const initialRenderOptions = shouldDeferErrorPageOnMiss
+          ? { renderErrorPageOnMiss: false }
+          : undefined;
+        response = await renderPage(request, resolvedUrl, null, ctx, undefined, initialRenderOptions);


Nit: In the initial render call when shouldDeferErrorPageOnMiss is true, initialRenderOptions is { renderErrorPageOnMiss: false } — but this doesn't include isDataReq: true for data requests. That's fine because shouldDeferErrorPageOnMiss requires !isDataRequest, so the two are mutually exclusive. But if a future change relaxes that guard, this would silently drop the isDataReq flag.

Consider making the mutual exclusivity explicit with a comment:

Suggested change

response = await renderPage(request, resolvedUrl, null, ctx, undefined, initialRenderOptions);

const initialRenderOptions = shouldDeferErrorPageOnMiss

? { renderErrorPageOnMiss: false }

: undefined; // shouldDeferErrorPageOnMiss requires !isDataRequest, so no isDataReq flag needed

Not blocking — the current code is correct.

ask-bonk · 2026-05-26T10:31:22Z

  try {
    const dataTarget = resolvePagesDataNavigationTarget(url, __basePath);
-    if (dataTarget) {
+    if (dataTarget && options.allowNotFoundResponse !== true) {


The allowNotFoundResponse !== true guard here forces error-route navigations to skip the /_next/data JSON path and go through HTML fetch. This is correct — the error page doesn't have a data endpoint — but the coupling between allowNotFoundResponse and "skip data navigation" is a bit indirect. The option name suggests it controls response acceptance, not the fetch strategy.

This is fine for now since the only consumer is the error-route path, but if this option grows more callers, it might be worth renaming or splitting the concern. Just a design note, not blocking.

ask-bonk · 2026-05-26T10:31:42Z

Review posted. Here's a summary:

Approved with two minor, non-blocking observations.

Overall assessment: The PR is well-executed. The three-layer fix (server route-miss rendering, client error-route fetch URL, prerender dedup) is correct and independently testable. All prior review concerns — ISR status preservation, stale metadata propagation, fallback-rewrite gating on route-miss signal, data-request exclusion — have been addressed across the 11 commits.

Key things verified:

Server-side /404 → _error fallback chain works correctly (line 606-619 in pages-server-entry.ts)
The matchRoute("/404") + pattern === "/404" guard prevents catch-all routes from hijacking the 404 path
renderErrorPageOnMiss: false / deferred re-render pattern in both prod-server.ts and deploy.ts correctly gates fallback rewrites on route-miss rather than response status
shouldDeferErrorPageOnMiss excludes data requests in both paths
Client resolvePagesErrorHtmlFetchUrl correctly maps /_error → /404 fetch, preserves as in history, and handles locale prefixes
allowNotFoundResponse bypasses both the data navigation path and the hard-navigation-on-error path
ISR cache correctly stores and retrieves the 404 status code
Prerender dedup correctly skips /404 in the generic loop and renders it via the dedicated 404 block with status === 404 guard
Test coverage is thorough with proper Next.js test provenance

Two minor notes:

deploy.ts line 893: The mutual exclusivity between shouldDeferErrorPageOnMiss and data requests is correct but implicit — a comment would help future readers.
router.ts line 999: allowNotFoundResponse controls both response acceptance AND fetch strategy (skipping /_next/data). The dual role is fine for now since only the error-route path uses it, but worth noting as a design consideration if the option grows more callers.

github run

…-pages # Conflicts: # packages/vinext/src/entries/pages-server-entry.ts # packages/vinext/src/server/pages-page-data.ts # packages/vinext/src/server/pages-page-response.ts # packages/vinext/src/shims/router.ts

NathanDrake2406 · 2026-05-26T15:21:52Z

Merged latest main (f25613e3) and resolved the conflicts. Summary of how each was resolved, plus a note on the remaining non-blocking review nits.

Conflict resolution

Four files conflicted, all from the Pages Router error-route changes here overlapping with recent main work:

shims/router.ts (navigateClient): main added client middleware-redirect probing in the no-data-target path, while this branch added the allowNotFoundResponse error-route exception. Error-route navigations now short-circuit to navigateClientHtml and skip both data navigation and middleware-redirect probing, since the masked browser URL is fictional and the error page has no data endpoint. Normal navigations keep main's new middleware-redirect behavior. The redirect-follow block (res.redirected) and the allowNotFoundResponse check in navigateClientHtml were also combined.
server/pages-page-response.ts: kept this branch's earlier responseHeaders/finalStatus declarations (so finalStatus can feed the ISR cache write) and retained main's new userSetCacheControl capture. applyGsspHeaders is the only Cache-Control writer before that point, so the captured value matches main's original capture site.
server/pages-page-data.ts: both sides added the vinext field and the VinextNextData import; deduped to one of each and kept this branch's statusCode.
entries/pages-server-entry.ts: kept the /404 -> _error route-miss fallback chain but switched its default-404 returns to main's new buildDefaultPagesNotFoundResponse() helper, and merged main's hasMiddleware into the same __vinext object that already carries pageModuleUrl/appModuleUrl.

Validation after the merge: vp check (full repo, 0 errors) plus tests/shims.test.ts -t "Pages Router", tests/pages-router.test.ts, tests/prerender.test.ts, tests/deploy.test.ts, and tests/pages-page-data.test.ts all green (563 passing).

Remaining nits (no code change)

resolvePagesErrorHtmlFetchUrl try/catch around new URL(window.location.href): leaving as-is. This only runs in the browser where window.location.href is well-formed, so a guard would be defensive code masking a state that cannot occur. Failing fast is preferable to swallowing.
deploy.ts initialRenderOptions dropping isDataReq: no change needed. The generated entry's _renderPage re-derives isDataReq from the request URL (__dataNorm.isDataReq) when the option is absent, so even if the !isDataRequest guard were relaxed later the flag would not actually be lost.
allowNotFoundResponse dual role / ternary idiom: noted, non-blocking, no behavioral change.

NathanDrake2406 marked this pull request as ready for review May 22, 2026 11:32

ask-bonk Bot reviewed May 22, 2026

View reviewed changes

ask-bonk Bot approved these changes May 22, 2026

View reviewed changes

NathanDrake2406 added 5 commits May 22, 2026 23:26

fix(prerender): keep custom 404 out of generic page loop

eba8710

fix(pages): fall back to _error on route misses

74125cc

fix(pages): document the catch-all guard on matchRoute("/404")

feb464a

The pattern check prevents a dynamic catch-all route (e.g. [...slug]) from being used as the 404 fallback. Without it, matchRoute can return a catch-all match and silently hijack the error rendering path.

NathanDrake2406 force-pushed the nathan/basepath-error-pages branch from 7260386 to feb464a Compare May 22, 2026 13:27

NathanDrake2406 marked this pull request as draft May 22, 2026 13:42

fix(pages): preserve error navigation rewrite order

ee81428

NathanDrake2406 marked this pull request as ready for review May 22, 2026 13:54

Merge remote-tracking branch 'upstream/main' into nathan/basepath-err…

8864372

…or-pages # Conflicts: # packages/vinext/src/entries/pages-server-entry.ts # packages/vinext/src/shims/router.ts

Copilot AI review requested due to automatic review settings May 22, 2026 17:08

Copilot started reviewing on behalf of NathanDrake2406 May 22, 2026 17:08 View session

Copilot AI reviewed May 22, 2026

View reviewed changes

Comment thread packages/vinext/src/server/prod-server.ts

Comment thread packages/vinext/src/deploy.ts

fix(pages): limit fallback rewrites to route misses

a0fd6b3

NathanDrake2406 requested a review from Copilot May 23, 2026 06:41

Copilot started reviewing on behalf of NathanDrake2406 May 23, 2026 06:41 View session

Copilot AI reviewed May 23, 2026

View reviewed changes

Comment thread packages/vinext/src/deploy.ts

NathanDrake2406 added 2 commits May 23, 2026 16:58

fix(deploy): skip deferred error rendering for data requests

66b0ecc

fix(pages): preserve ISR response status

e10bfcb

fix(pages): keep module metadata in ISR regeneration

146343d

ask-bonk Bot approved these changes May 26, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into nathan/basepath-error…

f25613e

…-pages # Conflicts: # packages/vinext/src/entries/pages-server-entry.ts # packages/vinext/src/server/pages-page-data.ts # packages/vinext/src/server/pages-page-response.ts # packages/vinext/src/shims/router.ts

james-elicx merged commit 5b633b0 into cloudflare:main May 27, 2026
28 checks passed

NathanDrake2406 deleted the nathan/basepath-error-pages branch June 13, 2026 13:25

Uh oh!

Conversation

NathanDrake2406 commented May 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Why

What changed

References

Uh oh!

pkg-pr-new Bot commented May 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

james-elicx commented May 22, 2026

Uh oh!

ask-bonk Bot left a comment

Choose a reason for hiding this comment

Review Summary

Key observations

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ask-bonk Bot commented May 22, 2026

Uh oh!

james-elicx commented May 22, 2026

Uh oh!

ask-bonk Bot left a comment

Choose a reason for hiding this comment

Review Summary

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ask-bonk Bot commented May 22, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

NathanDrake2406 commented May 23, 2026

Uh oh!

NathanDrake2406 commented May 23, 2026

Uh oh!

james-elicx commented May 26, 2026

Uh oh!

ask-bonk Bot left a comment

Choose a reason for hiding this comment

Review Summary

Uh oh!

ask-bonk Bot May 26, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot May 26, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot commented May 26, 2026

Uh oh!

NathanDrake2406 commented May 26, 2026

Conflict resolution

Remaining nits (no code change)

Uh oh!

Uh oh!

Reviewers

Assignees

NathanDrake2406 commented May 22, 2026 •

edited

Loading

pkg-pr-new Bot commented May 22, 2026 •

edited

Loading