fix: handle Pages Router middleware redirects#1439
Conversation
commit: |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Pages Router middleware redirect parity
Solid work. The core approach is correct — probe /_next/data/<buildId>/*.json with x-nextjs-data: 1, consume x-nextjs-redirect, and handle same-origin vs external redirects. The __VINEXT_PAGE_LOADERS__ fallback for production builds where module URLs aren't in the HTML is a clean design. Tests cover the two primary paths well (internal client redirect, locale-prefixed API hard-nav).
A few things to look at below, plus one observation that doesn't fit an inline comment:
ISR parity gap
renderPagesIsrHtml in pages-page-data.ts (not in this diff) calls buildPagesNextDataScript without a vinext key. This means ISR-cached pages will produce __NEXT_DATA__ without hasMiddleware: true, so after ISR serves stale HTML, the client-side data probe will be skipped (hasVinextMiddleware() returns false). For apps using both ISR and middleware, middleware redirects could be missed on ISR-cached page navigations. This might be acceptable for now (ISR + middleware is an unusual combo), but worth noting — or threading vinext: { hasMiddleware } through the ISR render options to close the gap.
| }, | ||
| signal, | ||
| }); | ||
| return res.headers.get("x-nextjs-redirect"); |
There was a problem hiding this comment.
Resilience concern: data probe errors bubble up as navigation failures.
If the fetch throws (network timeout, DNS failure), the error propagates to navigateClient's catch block which only handles AbortError and re-throws everything else. This means a transient network failure on the probe (which is purely an optimization) prevents the regular page fetch from even being attempted, and runNavigateClient will hard-navigate as recovery.
Upstream Next.js's client router treats data fetch failures more gracefully — it falls through to a full page load. Consider catching non-abort errors inside resolveMiddlewareDataRedirect itself and returning null, so the navigation falls through to the normal SSR HTML fetch:
async function resolveMiddlewareDataRedirect(
browserUrl: string,
signal: AbortSignal,
): Promise<string | null> {
const dataUrl = getPagesDataFetchUrl(browserUrl);
if (!dataUrl) return null;
try {
const res = await fetch(dataUrl, {
headers: { Accept: "application/json", "x-nextjs-data": "1" },
signal,
});
return res.headers.get("x-nextjs-redirect");
} catch (err: unknown) {
if (err instanceof DOMException && err.name === "AbortError") throw err;
return null; // probe failed — fall through to normal navigation
}
}This would also simplify the catch block in navigateClient at line 762-767.
There was a problem hiding this comment.
Fixed in fbb0332. resolveMiddlewareDataRedirect() now catches non-abort probe failures and returns null, so navigation falls through to the normal HTML fetch. Abort still propagates back to navigateClient() and is converted to NavigationCancelledError, preserving superseded-navigation behavior. Added regression coverage in tests/shims.test.ts for a failing data probe followed by successful page navigation.
| const firstSegment = pathname.split("/")[1]; | ||
| if (!firstSegment || !locales.includes(firstSegment)) return appPath; | ||
|
|
||
| const withoutLocale = pathname.slice(firstSegment.length + 1) || "/"; |
There was a problem hiding this comment.
Nit: the pathname.slice(firstSegment.length + 1) calculation depends on pathname starting with / (so the locale segment starts at index 1). This is safe because pathname comes from new URL(...).pathname which always has a leading /. But it's a subtle invariant — a brief comment would help future readers:
| const withoutLocale = pathname.slice(firstSegment.length + 1) || "/"; | |
| const withoutLocale = pathname.slice(firstSegment.length + 1) || "/"; // +1 for leading `/` |
There was a problem hiding this comment.
Applied in fbb0332. Added the invariant comment on the slice offset.
| scheduleHardNavigationAndThrow(url, "Navigation failed: invalid page module path"); | ||
| if (pendingRedirectHistoryUrl) { | ||
| window.history.replaceState(window.history.state ?? {}, "", pendingRedirectHistoryUrl); | ||
| _lastPathnameAndSearch = window.location.pathname + window.location.search; |
There was a problem hiding this comment.
The replaceState happens before the page module import (loadPagesPageModule). If the module import fails (network error loading the chunk), the URL bar shows the redirect target but content is stale until hard-nav recovery kicks in. This matches upstream Next.js behavior (they also commit the URL early), so it's fine — just noting the transient URL/content mismatch window exists.
There was a problem hiding this comment.
Verified. This matches the intended upstream-compatible behavior, and no code change is needed for this thread. The hard-navigation recovery path still handles module load failures after the early URL commit.
| ${loaderEntries.join(",\n")} | ||
| }; | ||
| window.__VINEXT_PAGE_LOADERS__ = pageLoaders; |
There was a problem hiding this comment.
Good addition. The loaders are keyed by route pattern and loadPagesPageModule looks them up via nextData.page — these should be consistent since both come from the route matcher's routePattern. Worth a quick sanity check that dynamic routes (e.g. /blog/[slug]) use the same key in both the loader map and __NEXT_DATA__.page.
There was a problem hiding this comment.
Verified. Dynamic route keys use the same route-pattern source on both sides: pages-client-entry.ts stores loaders under pagesPatternToNextFormat(r.pattern), and pages-server-entry.ts serializes __NEXT_DATA__.page from patternToNextFormat(route.pattern). Those are the same formatter over the matched route pattern, so /blog/[slug] style routes line up. No code change needed.
| if (redirectLocation) { | ||
| const redirectedUrl = resolveLocalRedirectUrl(redirectLocation); | ||
| if (!redirectedUrl) { | ||
| scheduleHardNavigationAndThrow(redirectLocation, "Navigation redirected externally"); |
There was a problem hiding this comment.
When resolveLocalRedirectUrl returns null (external redirect), scheduleHardNavigationAndThrow is called with the raw redirectLocation from x-nextjs-redirect. Protocol-relative URLs like //evil.com/path would be correctly rejected by resolveLocalRedirectUrl (the startsWith("//") check), then passed to window.location.href as a hard-nav target. This matches upstream behavior since the middleware author controls the redirect target and NextResponse.redirect() validates the URL. Just flagging for awareness — if you ever want defense-in-depth here, normalize to an absolute URL before the hard-nav.
There was a problem hiding this comment.
Verified. This is intentional parity with the middleware-controlled redirect target: protocol-relative values are rejected as local redirects, then treated as external hard-navigation targets. NextResponse.redirect() already validates middleware-authored redirect URLs. No code change needed here.
|
Review posted on PR #1439. Here's the summary: Overall: The approach is correct and well-structured. The core data probe mechanism ( Key feedback points:
|
NathanDrake2406
force-pushed
the
nathan/middleware-redirect-parity
branch
from
3453cb5 to
fbb0332
Compare
|
Addressed the review in fbb0332.
Validation rerun:
|
|
/bigbonk review |
There was a problem hiding this comment.
Review: Pages Router middleware redirect parity
The previous review's feedback (data probe resilience, ISR parity gap, slice-offset comment) has all been addressed. The implementation is correct and matches upstream Next.js behavior. Approving.
What this does right:
-
The
hasMiddlewaregate via__NEXT_DATA__.__vinextis clean — apps without middleware pay zero cost (no probe fetch). The flag is threaded through all four render paths: dev normal, dev ISR regen, prodresolvePagesPageData, and prodrenderPagesPageResponse. -
resolveMiddlewareDataRedirectcorrectly swallows non-abort errors and returnsnull, so probe failures fall through to normal HTML navigation. Abort errors still propagate asNavigationCancelledErrorfor superseded-navigation handling. -
loadPagesPageModuleis a nice extraction — the three-tier fallback (pageModuleUrl→ HTML regex →__VINEXT_PAGE_LOADERS__) consolidates page-loading logic that was previously inline innavigateClient. -
The locale-prefix stripping for API redirects (
stripLocalePrefixForApiRedirect) correctly scopes to/apipaths only, matching the upstream client router's special-casing. -
Test coverage hits the three primary paths: internal client redirect, probe failure fallthrough, and locale-prefixed API hard-nav. The ISR assertions verify
hasMiddlewareround-trips through cache storage.
Two minor observations (not blocking):
| if (parsed.origin !== window.location.origin) return null; | ||
|
|
||
| const appPathname = stripBasePath(parsed.pathname, __basePath); | ||
| const normalizedPathname = appPathname === "/" ? "/index" : appPathname.replace(/\/$/, ""); |
There was a problem hiding this comment.
Nit: the /index special-casing and trailing-slash strip produce the correct data URL for Next.js's convention, but there's no corresponding handling for routes that end in /index literally (e.g., a page at pages/index/index.tsx). This is an edge case that Next.js itself barely supports, so not actionable — just noting it.
| if (!isValidModulePath(pageModuleUrl)) { | ||
| console.error("[vinext] Blocked import of invalid page module path:", pageModuleUrl); | ||
| scheduleHardNavigationAndThrow(url, "Navigation failed: invalid page module path"); | ||
| if (pendingRedirectHistoryUrl) { |
There was a problem hiding this comment.
The replaceState commit happens before loadPagesPageModule, which is the same ordering as upstream (URL commits before async module load). If you ever add retry/fallback logic for module load failures, keep this ordering in mind — the URL bar will already show the redirect target.
|
Review posted and approved. Here's a summary: Approved — all previous review feedback has been addressed. The implementation correctly adds Pages Router client-side middleware redirect support by:
Two non-blocking observations left as inline comments about the |
NathanDrake2406
force-pushed
the
nathan/middleware-redirect-parity
branch
from
fbb0332 to
f07f69a
Compare
NathanDrake2406
force-pushed
the
nathan/middleware-redirect-parity
branch
from
f07f69a to
42b65b0
Compare
|
Resolved the conflict by rebasing Follow-up from the review/conflict resolution:
Verification:
GitHub now reports |
There was a problem hiding this comment.
Pull request overview
This PR updates vinext’s Pages Router client navigation to match Next.js middleware redirect semantics by probing /_next/data/<buildId>/*.json for x-nextjs-redirect, enabling client-side handling of same-origin redirects, and carrying a hasMiddleware flag through __NEXT_DATA__ to avoid unnecessary probes when middleware isn’t configured.
Changes:
- Add middleware data-probe redirect handling to the Pages Router client (
shims/router.ts), including same-origin vs external redirect behavior and locale-prefixed API redirect normalization. - Expose production Pages Router page loaders on
windowand propagate__vinext.hasMiddlewareinto__NEXT_DATA__in dev + prod server paths. - Extend/adjust tests to cover middleware redirect scenarios and ensure
__vinextmetadata is present in rendered HTML/ISR cache.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/shims.test.ts | Adds unit coverage for middleware redirect probe behavior (internal redirects, basePath, probe failure fallback, API hard-nav). |
| tests/pages-page-data.test.ts | Verifies __vinext.hasMiddleware is included in HTML and ISR-cached payloads. |
| packages/vinext/src/shims/router.ts | Implements Pages Router middleware redirect probe + redirect URL normalization; adds module/component type guards and loader fallback. |
| packages/vinext/src/server/pages-page-response.ts | Threads optional vinext metadata into __NEXT_DATA__ script generation. |
| packages/vinext/src/server/pages-page-data.ts | Passes vinext metadata through ISR/Pages page data rendering pipeline. |
| packages/vinext/src/server/dev-server.ts | Adds hasMiddleware parameter and injects it into __NEXT_DATA__.__vinext in dev responses. |
| packages/vinext/src/index.ts | Passes middleware presence into the dev SSR handler. |
| packages/vinext/src/global.d.ts | Updates window global typings for Pages Router runtime globals (but currently introduces a duplicate conflicting declaration). |
| packages/vinext/src/entries/pages-server-entry.ts | Embeds middleware presence into server-rendered __NEXT_DATA__.__vinext. |
| packages/vinext/src/entries/pages-client-entry.ts | Exposes __VINEXT_PAGE_LOADERS__ on window for production navigation. |
| packages/vinext/src/client/vinext-next-data.ts | Extends VinextNextData.__vinext with hasMiddleware. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
NathanDrake2406
force-pushed
the
nathan/middleware-redirect-parity
branch
from
42b65b0 to
2437c32
Compare
|
Addressed the new review in Changes:
Verification:
The amend hook also reran entry-template tests, checks, knip, and rebuilt |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
packages/vinext/src/shims/router.ts:806
isPageComponentvalidates that the default export is a React component (function/forwardRef/memo). When this check fails the thrown message says "page module has no default export", which is misleading if a default export exists but is not a component. Consider updating the error text to reflect an invalid default export (or include the detected type) to make debugging easier.
This issue also appears on line 1001 of the same file.
const PageComponent = pageModule.default;
if (!isPageComponent(PageComponent)) {
scheduleHardNavigationAndThrow(
url,
"Data navigation failed: page module has no default export",
);
packages/vinext/src/shims/router.ts:1006
- Same issue as the data-navigation path:
isPageComponentcan fail even when a default export exists (e.g. default-exporting a non-component). The error message currently claims there is "no default export"; please adjust it to describe an invalid/non-component default export so the hard-navigation fallback is diagnosable.
const PageComponent = pageModule.default;
if (!isPageComponent(PageComponent)) {
scheduleHardNavigationAndThrow(
browserUrl,
"Navigation failed: page module has no default export",
);
| function getWindowOrigin(): string { | ||
| const { origin, href } = window.location; | ||
| if (origin) return origin; | ||
| return new URL(href).origin; | ||
| } | ||
|
|
||
| function resolveSameOriginRedirectedUrl(responseUrl: string): string | null { | ||
| const appPath = toSameOriginAppPath(responseUrl, __basePath); | ||
| if (appPath === null) return null; | ||
| return normalizePathTrailingSlash( | ||
| toBrowserNavigationHref(appPath, window.location.href, __basePath), | ||
| __trailingSlash, | ||
| ); | ||
| } |
NathanDrake2406
force-pushed
the
nathan/middleware-redirect-parity
branch
from
2437c32 to
8ee18dc
Compare
|
Addressed the new review in Assessment:
Changes:
Verification:
The amend hook also reran checks, knip, and rebuilt |
Summary
test/e2e/middleware-redirects/test/node-runtime.test.ts: internal client redirects and locale-prefixed API redirects, both with and without/fr./_next/data/<buildId>/*.json, consumesx-nextjs-redirect, and treats same-origin redirects as client redirects.next/router, because production SSR HTML intentionally does not include source module URLs.__vinext.hasMiddlewareinto Pages__NEXT_DATA__in dev and prod so apps without middleware avoid the extra data probe./nl/api/okto/api/ok, matching the upstream client router behavior for locale-prefixed middleware redirects to API routes.Root Cause
vinext already returned the right middleware redirect headers for
_next/datarequests, so the same-host and data-header candidate cases were already passing. The missing piece was the Pages Router client navigation path: it fetched the destination HTML directly and never performed the Next.js data-route redirect probe that upstreamnext/routeruses before committing a client navigation.That caused
/old-homelink clicks to miss the middleware redirect to/new-home, and caused locale-qualified/to?pathname=/api/oknavigations to remain stuck on the client route instead of falling through to the API response.Upstream References
/old-homeand/to: https://github.com/vercel/next.js/blob/canary/test/e2e/middleware-redirects/app/middleware.jsx-nextjs-redirectand distinguishes internal/external redirects: https://github.com/vercel/next.js/blob/canary/packages/next/src/shared/lib/router/router.tsNextResponse.redirect()source: https://github.com/vercel/next.js/blob/canary/packages/next/src/server/web/spec-extension/response.tsValidation
vp checkvp test run tests/shims.test.tsvp 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/middleware-redirects/test/node-runtime.test.ts