fix(app-router): hard navigate streamed redirects#1742
Conversation
commit: |
NathanDrake2406
force-pushed
the
nathan/nav-redirect-loading-once
branch
from
729e8cf to
7b40ce7
Compare
|
/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.
Review: hard navigate streamed redirects
The implementation is faithful to Next.js. The new guard mirrors navigate-reducer.ts exactly: after the external-URL check and before the segment-cache (RSC) navigation, Next.js checks document.getElementById('__next-page-redirect') and falls back to completeHardNavigation. This PR places hasPendingAppRouterPageRedirect(document) at the equivalent point in navigateClientSide (after hash-only handling, before appNavigate), and routes through navigateExternal (which triggers the mpa-navigation tree state + scheduler) with a window.location fallback that matches the existing external-URL path at navigation.ts:1695-1701.
Verified locally:
vp test run tests/app-browser-mpa-navigation.test.ts -t "hasPendingAppRouterPageRedirect"— 4 passedvp test run tests/shims.test.ts -t "streamed App Router redirect meta"— 1 passedvp checkon both changed source files — format, lint, type all pass
Parity checks:
router.refresh()(navigation.ts:1889) correctly does NOT get this guard — Next.js'srefresh-reducerhas no__next-page-redirectcheck, onlynavigate-reducer. Correct.- The marker is App Router-specific and emitted into SSR'd HTML by
app-ssr-error-meta.ts:65, so guarding only the App RouternavigateClientSidepath is the right scope.
Two minor, non-blocking observations below.
| await page.waitForTimeout(400); | ||
| await expect(page.locator("#timestamp")).toHaveText(initialTimestamp ?? ""); | ||
| } | ||
| expect(requestedPathnames.filter((pathname) => pathname === "/redirect/result")).toHaveLength( |
There was a problem hiding this comment.
Minor/possible-flake note: page.on("request") captures all requests, and the filter keys only on pathname === "/redirect/result". If vinext ever issues an RSC fetch whose URL pathname is also /redirect/result (e.g. a prefetch or .rsc request that preserves the pathname), this toHaveLength(1) would fail even when the user-visible behavior is correct. Next.js's original test counts full document loads rather than any request to the pathname. Since the fix routes through the hard-navigation/MPA path this currently holds, but consider narrowing to document requests (e.g. request.resourceType() === "document") to make the assertion robust against future RSC request shapes.
| // Next.js treats a streamed redirect meta tag as an MPA-navigation marker. | ||
| // A soft RSC redirect would leave the source document alive long enough for | ||
| // the delayed meta refresh to fire and render the target a second time. | ||
| if (hasPendingAppRouterPageRedirect(typeof document === "undefined" ? undefined : document)) { |
There was a problem hiding this comment.
The typeof document === "undefined" ? undefined : document guard is reasonable defensiveness, but note this branch only runs in navigateClientSide, which is already a client-only path (the function dereferences window.location unconditionally just above at line 1706). hasPendingAppRouterPageRedirect itself also handles a missing/partial document. So the inline ternary is belt-and-suspenders — fine to keep, just flagging that document is expected to exist here whenever window does.
|
Review posted successfully. Here's a summary of my analysis: Review SummaryPR #1742 — fix(app-router): hard navigate streamed redirects is well-implemented and faithful to Next.js. I posted a COMMENT-level review (no blockers). What the PR doesAdds Verification I ran
Correctness/parity confirmed against Next.js
Minor observations raised (non-blocking)
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: hard navigate streamed redirects
The fix is correct and faithful to Next.js. hasPendingAppRouterPageRedirect detects the SSR-emitted <meta id="__next-page-redirect"> marker and routes the navigation through navigateExternal (the MPA/mpa-navigation tree-state path), with a window.location fallback that mirrors the existing external-URL branch at navigation.ts:1695-1701. Scope is correct: the marker is App-Router-only (emitted by app-ssr-error-meta.ts:65), the single chokepoint is navigateClientSide (both router.push/replace and Link clicks funnel through it), and router.refresh() correctly does not get the guard (Next.js's refresh-reducer has no such check).
Verified locally:
tests/app-browser-mpa-navigation.test.ts -t hasPendingAppRouterPageRedirect— 4 passedtests/shims.test.ts -t "streamed App Router redirect meta"— passedtests/app-ssr-error-meta.test.ts— 6 passedvp checkon both changed source files — format/lint/type clean
The new fixture routes (redirect/result, redirect/servercomponent, redirect-with-loading) are isolated and don't collide with the existing redirect-test/protected-loading fixtures referenced by app-router-dev-server.test.ts. The e2e test already narrows to request.resourceType() === 'document' (the prior review's suggestion was incorporated).
One parity note and a minor nit below — both non-blocking.
| // Next.js treats a streamed redirect meta tag as an MPA-navigation marker. | ||
| // A soft RSC redirect would leave the source document alive long enough for | ||
| // the delayed meta refresh to fire and render the target a second time. | ||
| if (hasPendingAppRouterPageRedirect(typeof document === "undefined" ? undefined : document)) { |
There was a problem hiding this comment.
Parity nit (non-blocking): in Next.js's navigate-reducer.ts the __next-page-redirect check is the very first thing in the reducer — it runs before hash-only handling and before the transition-start signal. Here it is placed after the hash-only short-circuit (line 1717) and after notifyAppRouterTransitionStart (line 1709). The practical impact is essentially nil: a server-side redirect() target that is a hash-only change of the redirecting document's own URL isn't a realistic scenario, so the hash-only branch can't plausibly swallow a real streamed redirect. Just flagging the ordering divergence in case you want exact navigate-reducer parity. If you keep the current placement, consider a one-line comment noting the deliberate ordering difference.
| // Next.js treats a streamed redirect meta tag as an MPA-navigation marker. | ||
| // A soft RSC redirect would leave the source document alive long enough for | ||
| // the delayed meta refresh to fire and render the target a second time. | ||
| if (hasPendingAppRouterPageRedirect(typeof document === "undefined" ? undefined : document)) { |
There was a problem hiding this comment.
Minor: typeof document === "undefined" ? undefined : document is belt-and-suspenders here — navigateClientSide is already client-only (it dereferences window.location unconditionally at line 1706, which would have thrown long before reaching this point if window/document were absent), and hasPendingAppRouterPageRedirect itself already null-guards. Fine to keep for defensiveness; just noting document is expected to exist here whenever window does.
|
Review posted successfully. Summary of my analysis: Review SummaryPR #1742 — fix(app-router): hard navigate streamed redirects is correct and faithful to Next.js. I posted a COMMENT-level review (no blockers). What it doesAdds Verification I ran
Correctness/parity confirmed
Non-blocking observations raised
|
2 participants
Summary
Root Cause
Vinext already emits the
#__next-page-redirectmeta marker for streamed App Router redirects. The client-side navigation shim still treated the redirect target as a normal App Router RSC navigation, so the source document stayed alive after rendering the destination. When the meta refresh fired, it hard-navigated to the same destination a second time and changed the rendered timestamp.Next.js avoids this by checking for the streamed redirect marker before handling client navigation and completing the navigation through the hard-navigation path instead.
Next.js References
navigation.test.tsredirect-once casesnavigate-reducer.tschecks__next-page-redirectmake-get-server-inserted-html.tsxemits redirect metaredirect-boundary.tsxValidation
vp test run tests/app-browser-mpa-navigation.test.ts tests/shims.test.ts -t "streamed App Router redirect meta|hasPendingAppRouterPageRedirect"vp test run tests/app-ssr-error-meta.test.ts tests/rsc-streaming.test.tsvp run vinext#buildPLAYWRIGHT_PROJECT=app-router pnpm run test:e2e tests/e2e/app-router/redirect-once.spec.tsvp checkPre-fix, the ported e2e failed for
/redirect/redirect-with-loadingbecause the timestamp changed after the delayed meta refresh. After the fix it passes and records exactly one/redirect/resultdocument request.