Skip to content

fix(pages-router): cancel in-flight nav on gSSP/gSP data redirect (#1465)#1691

Open
james-elicx wants to merge 2 commits into
mainfrom
fix/issue-1465-pages-nav-cancellation
Open

fix(pages-router): cancel in-flight nav on gSSP/gSP data redirect (#1465)#1691
james-elicx wants to merge 2 commits into
mainfrom
fix/issue-1465-pages-nav-cancellation

Conversation

@james-elicx
Copy link
Copy Markdown
Member

Summary

Fixes #1465 — Pages Router navigation cancellation: when a slow navigation is superseded by a redirect, vinext ended up on the wrong page (e.g. Redirect Page instead of a normal page).

The root cause was the getServerSideProps/getStaticProps { redirect } path for client-side _next/data navigations. vinext emitted a real HTTP redirect for data requests. The client's fetch() transparently follows it to the destination's HTML (not a valid { pageProps } envelope), which forced a hard reload (with console errors) and left navigation on the wrong page.

This mirrors how Next.js handles it: for a data request, the server replies 200 with __N_REDIRECT / __N_REDIRECT_STATUS in pageProps, and the client router re-dispatches a fresh client navigation to the destination. That fresh navigation increments the navigation id and supersedes (cancels) the in-flight one via the existing _navigationId / assertStillCurrent() / AbortController machinery — so the intermediate page never commits.

Changes

  • Server (prod): buildPagesRedirectResponse() in pages-page-data.ts — for isDataReq, emit { pageProps: { __N_REDIRECT, __N_REDIRECT_STATUS } } (200 JSON); otherwise a real HTTP redirect.
  • Server (dev): writeGsspRedirect() in dev-server.ts — same isDataReq branch, applied to both the gSSP and gSP redirect blocks (dev/prod parity per AGENTS.md).
  • Client: navigateClientData() in shims/router.ts detects pageProps.__N_REDIRECT and calls handleDataRedirect(), which re-enters performNavigation() for the destination (preserving push/replace mode, locale: false) then throws NavigationCancelledError so the superseded navigation unwinds cleanly. Internal-vs-external routing mirrors Next.js (this.change vs handleHardNavigation).

HTML page requests are unchanged (still a real HTTP redirect).

Tests

  • tests/pages-router.test.ts (vitest): gSSP redirect emits an HTTP 307 on HTML requests, and a 200 __N_REDIRECT JSON envelope (no Location) on _next/data requests.
  • tests/e2e/pages-router/gssp-redirect-cancellation.spec.ts (Playwright): clicking a Link / router.push to a gSSP-redirect page lands on the destination (a normal page), never commits the intermediate Redirect Page, does not full-reload, and produces no console errors (the consoleErrors fixture). Ported from Next.js test/e2e/getserversideprops.
  • New fixture pages: gssp-redirect.tsx, gssp-redirect-target.tsx, gssp-redirect-test.tsx.

Verification

Ran locally (targeted):

  • vp test run tests/pages-router.test.ts -t "Pages Router integration" → 116 passed
  • vp test run tests/pages-data-route.test.ts tests/pages-data-url.test.ts → 39 passed
  • PLAYWRIGHT_PROJECT=pages-router playwright test --grep "gSSP redirect cancellation" → 2 passed

…ts (#1465)

When getServerSideProps/getStaticProps returns `{ redirect }` on a
client-side `_next/data` navigation, vinext emitted a real HTTP redirect.
The client's fetch transparently followed it to the destination's HTML
(not a valid data envelope), forcing a hard reload with console errors and
leaving navigation on the wrong page.

Mirror Next.js: for data requests, reply 200 with `__N_REDIRECT` /
`__N_REDIRECT_STATUS` in pageProps. The client router now detects these
markers and re-dispatches a fresh client navigation to the destination,
which increments the navigation id and supersedes (cancels) the in-flight
navigation so the intermediate page never commits. HTML requests still emit
a real HTTP redirect.

Fixed in both server paths to keep dev/prod parity (see AGENTS.md):
- prod: buildPagesRedirectResponse() in pages-page-data.ts branches on isDataReq
- dev: writeGsspRedirect() in dev-server.ts branches on isDataReq
- client: navigateClientData() handles pageProps.__N_REDIRECT via
  handleDataRedirect(), preserving push/replace mode
- tests: data-endpoint redirect shape (vitest) + client cancellation (e2e)
@james-elicx
Copy link
Copy Markdown
Member Author

/bigbonk review

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented May 29, 2026

Open in StackBlitz

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

commit: 32ea84d

@github-actions
Copy link
Copy Markdown

Example Preview Production Original
app-router-cloudflare preview production
pages-router-cloudflare preview production
app-router-playground preview production original
realworld-api-rest preview production
nextra-docs-template preview production
benchmarks preview production
hackernews preview production original
web preview production

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: fix(pages-router): cancel in-flight nav on gSSP/gSP data redirect (#1465)

Overall this is a well-structured fix. The root cause analysis is correct — HTTP redirects on _next/data requests cause fetch() to transparently follow to HTML, which breaks the JSON envelope contract. The __N_REDIRECT / __N_REDIRECT_STATUS approach correctly mirrors Next.js. Dev/prod parity is maintained: writeGsspRedirect() in dev-server.ts mirrors buildPagesRedirectResponse() in pages-page-data.ts, and the prod server delegates through pages-page-data.ts via renderPage() — no bypass. Tests are well-ported from the Next.js test suite and cover both Link clicks and router.push.

One bug found, plus minor observations:

Bug: handleDataRedirect passes wrong URL for external hard navigation

router.ts:816scheduleHardNavigationAndThrow(fallbackUrl, ...) passes the original page URL (the page whose gSSP returned the redirect) instead of destination (the redirect target). For an external redirect like __N_REDIRECT: "https://example.com/oauth/callback", this sends the browser back to the page that issued the redirect, creating an infinite loop. Next.js passes the redirect destination to handleHardNavigation, not the original page.

-    scheduleHardNavigationAndThrow(fallbackUrl, "Navigation redirected externally");
+    scheduleHardNavigationAndThrow(destination, "Navigation redirected externally");

This also affects the redirectBasePath === false case (a valid same-origin absolute path that opted out of basePath) — the browser would navigate to the wrong URL.

Minor observations (non-blocking)

  1. Fire-and-forget void performNavigation(...) is safe — the async function runs to completion via the microtask queue, runNavigateClient catches all exceptions (no unhandled rejections), and the event ordering (routeChangeStart for new nav → routeChangeError for old nav) matches Next.js's this.change(method, ...) re-dispatch pattern. The NavigationCancelledError throw after the call is belt-and-suspenders since the new navigation already incremented _navigationId — good defense-in-depth.

  2. routeChangeError is correctly emitted for cancelled navigations with err.cancelled === true, matching Next.js's contract for listeners that distinguish cancellations from real errors.

  3. dev-server.ts uses JSON.stringify directly while pages-page-data.ts uses safeJsonStringify**. The dev server's writeGsspRedirect (line 109) calls JSON.stringify({ pageProps: { __N_REDIRECT: dest, __N_REDIRECT_STATUS: status } }), while the prod path in buildPagesRedirectResponse uses options.safeJsonStringify via buildNextDataJsonResponse. For the specific shape here (string + number, no user-controlled data), this is safe — safeJsonStringify exists for XSS protection on user-supplied pageProps. But it's worth noting the asymmetry.

}

// Re-dispatch as a fresh navigation. `locale: false` matches Next.js, which
// does not re-apply the locale prefix to a redirect destination.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: this passes fallbackUrl (the browser URL of the page whose gSSP returned the redirect) instead of destination (the redirect target URL).

For an external redirect like __N_REDIRECT: "https://example.com/oauth/callback", the browser navigates back to the page that issued the redirect → gSSP returns the same redirect → infinite loop.

Next.js passes the redirect destination to handleHardNavigation, not the originating page URL. Fix:

Suggested change
// does not re-apply the locale prefix to a redirect destination.
scheduleHardNavigationAndThrow(destination, "Navigation redirected externally");

Same issue for the redirectBasePath === false case — a valid same-origin path that opted out of basePath would navigate to the wrong URL.

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented May 29, 2026

Review posted successfully. Here's a summary of what was reviewed and found:


Review posted on PR #1691 with a request for changes.

Bug found: In handleDataRedirect() at packages/vinext/src/shims/router.ts:816, the external redirect fallback passes fallbackUrl (the original page URL) to scheduleHardNavigationAndThrow instead of destination (the redirect target). This would cause an infinite redirect loop for external gSSP redirects like __N_REDIRECT: "https://example.com/oauth/callback" — the browser would navigate back to the page that issued the redirect rather than to the target. Fix: scheduleHardNavigationAndThrow(destination, ...).

Everything else looks good:

  • Server-side __N_REDIRECT / __N_REDIRECT_STATUS envelope is correct for both dev (writeGsspRedirect) and prod (buildPagesRedirectResponse)
  • Dev/prod parity is maintained — prod delegates through pages-page-data.ts via renderPage(), no bypass
  • Fire-and-forget void performNavigation(...) is safe — no unhandled rejections, correct event ordering
  • NavigationCancelledError catch chain properly unwinds (emits routeChangeError with err.cancelled === true, cleans up abort controllers)
  • Tests are well-ported from the Next.js suite

github run

…#1465)

Address review: handleDataRedirect() passed the originating page URL to the
hard-navigation fallback for external/non-absolute __N_REDIRECT destinations,
which would loop back to the redirecting page (e.g. an OAuth callback at
https://example.com/...). Hard-navigate to the destination instead, matching
Next.js's handleHardNavigation({ url: destination }). Drop the now-unused
fallback parameter and add a server test asserting an external destination is
preserved verbatim in the data envelope.
@james-elicx
Copy link
Copy Markdown
Member Author

Good catch — fixed in 32ea84d. handleDataRedirect() now hard-navigates to destination (not the originating page URL) for external/non-absolute __N_REDIRECT targets, matching Next.js's handleHardNavigation({ url: destination }). Removed the unused fallback parameter and added a server test asserting an external destination is preserved verbatim in the data envelope.

/bigbonk review

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review: fix commit 32ea84d

The bug flagged in the first review is properly fixed:

  1. handleDataRedirect() now hard-navigates to destinationscheduleHardNavigationAndThrow(destination, ...) at router.ts:815 correctly mirrors Next.js's handleHardNavigation({ url: destination }). External gSSP redirects (e.g. https://example.com/oauth/callback) and basePath: false redirects now navigate to the intended target, not the originating page.

  2. Unused fallbackUrl parameter removed — clean API, no dangling footgun.

  3. Server test added"preserves an external destination in __N_REDIRECT on _next/data requests" in pages-router.test.ts asserts the full URL https://example.com/landing is carried verbatim in the JSON envelope. Good coverage for the round-trip: server emits correct envelope → client reads it → handleDataRedirect navigates to it.

  4. Call site updatednavigateClientData at router.ts:910 passes three args (destination, basePath flag, mode) matching the new signature. The NavigationCancelledError throw on line 911 is now truly unreachable (since handleDataRedirect always throws), but it's harmless belt-and-suspenders.

No further issues found. Ship it.

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented May 29, 2026

Review posted. The fix in 32ea84d correctly addresses the bug — handleDataRedirect() now passes destination (not the originating page URL) to scheduleHardNavigationAndThrow, matching Next.js's handleHardNavigation({ url: destination }). The unused fallbackUrl parameter is removed, and a new server test verifies external redirect destinations are preserved verbatim in the __N_REDIRECT envelope. Approved.

github run

@james-elicx james-elicx marked this pull request as ready for review May 29, 2026 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Pages Router: navigation cancellation/concurrent navigation handling broken

1 participant