fix(pages): render masked basePath error routes#1441
Conversation
commit: |
|
/bigbonk review |
There was a problem hiding this comment.
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
_errorfallback for route misses. The new route-miss block only tries/404. If a Pages app has_error.tsxbut no404.tsx,matchRoute("/404", pageRoutes)returns null and falls through to the generic HTML. Next.js falls back to_errorin that case (thebase-server.tsreference in the PR description confirms this). The prerender code handles this correctly (hasCustom404 || hasErrorPage), but the server entry does not. -
Client-side:
resolvePagesErrorHtmlFetchUrloperates on the rawurl(href) param, not onas. This is correct — it's thehrefthat selects the component route. Good. -
allowNotFoundResponseis a clean extension tonavigateClientthat 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.
|
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 Minor nits:
|
|
/bigbonk review |
There was a problem hiding this comment.
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.
|
Review posted. Here's a summary: Approved with minor suggestions. The PR is well-structured and correct. Key findings:
|
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.
7260386 to
feb464a
Compare
…or-pages # Conflicts: # packages/vinext/src/entries/pages-server-entry.ts # packages/vinext/src/shims/router.ts
There was a problem hiding this comment.
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 topages/_error) with status 404, and pass the missed path through asasPath. - Client router shim: introduce a narrow exception so error-route navigations fetch
/404HTML (optionally allowing 404 responses) while keeping the maskedasURL in history. - Hydration metadata: embed resolved page and
_appmodule URLs into__NEXT_DATA__.__vinextso 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.
|
Addressed the ISR status review in e10bfcb. The Pages ISR cache now stores the rendered status from |
|
Addressed the stale ISR metadata review in 146343d. |
Overview
pages/404for production route misses, preserve maskedasURLs, and fetch/404as the component route forrouter.push('/404', as)androuter.push('/_error', as).packages/vinext/src/entries/pages-server-entry.ts,packages/vinext/src/server/pages-page-response.ts,packages/vinext/src/shims/router.tsbasePathget 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
asin history but resolves/404and/_erroras error component routes. Vinext collapsed those values too early, so masked error navigations fetched the friendly URL and production route misses underbasePathfell back to generic HTML.pages/404should render that route with status 404./404page after request normalization and pass the original visible path asasPath.hrefselects the page component, whileasselects the visible browser URL.hrefs fetch/404, preserve the masked history URL, and allow the 404 response to hydrate.__NEXT_DATA__to import the fetched page._appmodule URLs.What changed
/docs/docs/other-pagewithbasePath: '/docs'andpages/404pages/404HTML, status 404,__NEXT_DATA__.page === '/404'.next.router.push('/404', '/slug-2')under/docs/docs/404, keeps location/docs/slug-2, renders the error component.next.router.push('/_error', '/slug-2')under/docs/docs/404while 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.tsvp checkMaintainer review path
tests/pages-router.test.tsfor the production basePath 404 and_errorfallback regressions.tests/shims.test.tsfor the ported masked/404and/_errorclient navigation contract.tests/prerender.test.tsfor the custom/404prerender ownership regression.packages/vinext/src/shims/router.tsfor the narrow Pages error-route exception in client navigation.packages/vinext/src/entries/pages-server-entry.tsandpackages/vinext/src/server/pages-page-response.tsfor custom 404 rendering, status propagation,asPath, and module metadata.Risk / compatibility
hrefs whose normalized path is/404or/_error./404page renders with status 404, matching the error route contract./404page route, not a dynamic route that happens to match/404.References
/404and/_errorclient navigation behavior.hrefandasseparately and resolving error routes as page components.pages/404before falling back to_error.