fix(app-router): preserve rewritten route identity#2358
Conversation
|
/bigbonk review for issues |
commit: |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(app-router): preserve rewritten route identity
I traced the change end-to-end and it is correct, minimal, and well-tested. Approving with one minor (non-blocking) suggestion.
What the change does
createAppPageRenderIdentity previously derived the RSC routeId/pageId from displayPathname (the browser-visible URL). After a middleware rewrite, displayPathname is the canonical URL the user sees (route A) while routePath is the post-rewrite matched route (route B). Encoding the keys from the visible URL produced a payload whose params/segment data were for B but whose __route/page element keys were for A — tripping the client RSC error boundary and forcing a hard reload.
The fix threads the matched route through a new optional matchedRoutePathname input and uses it (falling back to displayPathname) for routeId/pageId. app-page-element-builder.ts:229 wires it to routePath.
Correctness checks I verified
- Architecture alignment.
app-rsc-handler.ts:506capturescanonicalPathname = cleanPathnamebefore rewrites mutatecleanPathname, anddisplayPathname: canonicalPathnameis passed atapp-rsc-handler.ts:1018.routePathis the rewritten match. SodisplayPathname(visible) vsroutePath(matched) is exactly the right pair to split here. - Interception precedence preserved.
matchedRoutePathname = sourceMatchedPathname ?? requestedMatchedRoutePathnamekeeps interception-source identity winning over the rewrite path, matching prior behavior. The interception unit tests still pass unchanged. targetMatchedPathnamestill derives fromdisplayPathname, so interceptiontargetMatchedUrl/targetRouteIdare unaffected; in the non-interception rewrite caseinterceptionis null so it's harmless.- Backward compatible. The new input is optional and defaults to
displayPathname; the single call site is the only consumer. - Header value is real. The fixture middleware gates on
x-vinext-rsc-render-mode !== "prefetch-loading-shell", which matchesVINEXT_RSC_RENDER_MODE_HEADER(X-Vinext-Rsc-Render-Mode, headers.ts:62) and theprefetch-loading-shellmode (app-rsc-render-mode.ts:9). Case-insensitive header lookup is fine. - Fixture deps exist.
connection()is exported from thenext/servershim (shims/server.ts:1049).
Test quality
- The e2e spec is a strong regression: it sets a
windowmarker, asserts the loading boundary from A renders, content from B renders, the URL stays at/dynamic-page/a(visible) while serving B (rewritten),documentRequestsis empty, and the marker survives — proving a soft SPA transition rather than a hard reload. Good fidelity to the upstream Next.js test it ports. vp checkclean on both changed source files;vitestonapp-page-render-identity.test.ts+app-page-element-builder.test.ts→ 43 passed.
Minor suggestion (non-blocking)
The new identity unit test covers the function directly, and the e2e covers the full integration, but there is no app-page-element-builder.test.ts case asserting that when displayPathname !== routePath the returned element map is keyed on the routePath-derived routeId (not the displayPathname one). That's the one seam between the builder wiring (matchedRoutePathname: routePath) and the identity function. A small builder-level assertion would close the loop and guard against a future regression where the wiring is dropped while the identity function stays correct. Optional.
Overall: correct, narrowly scoped, good parity comments and test coverage. LGTM.
|
Review posted to PR #2358. SummaryPR #2358 ( The fix: What I verified:
Minor suggestion: add a builder-level test asserting the returned element map is keyed on the |
|
/bigbonk review for issues |
|
Updated the regression to an isolated production-only browser fixture. This removes shared app-basic middleware changes, avoids dev mode where Link prefetch is intentionally disabled, and resolves the unrelated middleware invocation flake. Local exact browser spec passes at |
|
/bigbonk review for issues |
|
UnknownError: "Overloaded" |
|
@james-elicx Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): preserve rewritten route identity
The core fix is correct, minimal, and well-tested. I traced it end-to-end and verified the unit tests (44 pass). I found one leftover-dead-code issue that should be cleaned up before merge.
Core fix — correct
createAppPageRenderIdentity now threads a new optional matchedRoutePathname (wired to routePath at app-page-element-builder.ts:229) and derives routeId/pageId from it instead of from displayPathname. After a middleware rewrite, displayPathname is the visible URL (route A) and routePath is the post-rewrite matched route (B), so encoding the keys from the visible URL produced a payload whose params/segment data were for B but whose __route/page element keys were for A — tripping the client RSC error boundary.
Things I verified:
- Aligns with the existing fallback.
app-page-route-wiring.tsx:546-550already encodesrouteId/pageIdfromoptions.routePathwhenrenderIdentityis absent. Before this PR, therenderIdentity-present path useddisplayPathname, so the two paths disagreed. The fix makes them consistent, and consistent with the rest of the element map (sourcePage, slot bindings, params all derive from route B). - Interception precedence preserved.
matchedRoutePathname = sourceMatchedPathname ?? requestedMatchedRoutePathnamekeeps interception-source identity winning;targetMatchedPathnamestill derives fromdisplayPathname, sotargetMatchedUrl/targetRouteIdare unaffected. The interception unit tests pass unchanged. - Backward compatible. The new input is optional and defaults to
displayPathname; single call site. - The builder-level test (
tests/app-page-element-builder.test.ts) asserts the element map is keyed on theroutePath-derived id whendisplayPathname !== routePath— this closes the wiring-seam gap noted in the earlier review.
Issue: orphaned app-basic/middleware.ts changes (should be reverted)
tests/fixtures/app-basic/middleware.ts still contains the rewrite branch and matcher entry from the first commit (ec5a77c):
- the
/nextjs-compat/mismatching-prefetch/dynamic-page/rewrite branch (lines 89-97) - the
/nextjs-compat/mismatching-prefetch/:path*matcher entry (line 374)
But commit f1b7ce6 ("isolate rewrite prefetch regression") deleted the corresponding app-basic fixture pages (app/nextjs-compat/mismatching-prefetch/page.tsx, dynamic-page/[param]/page.tsx, dynamic-page/[param]/loading.tsx) and the old spec, replacing them with the self-contained mismatching-prefetch.browser.spec.ts (which writes its own temp fixture + middleware and uses a different route path, /mismatching-prefetch/..., without the /nextjs-compat prefix).
The result is that these app-basic middleware additions are dead code:
- No fixture page exists under
tests/fixtures/app-basic/app/nextjs-compat/mismatching-prefetch/. - No test references
/nextjs-compat/mismatching-prefetch— the only two matches in the tree are the orphaned middleware lines themselves. - The matcher entry needlessly runs middleware on a non-existent route in the shared fixture.
This also contradicts the PR comment: "This removes shared app-basic middleware changes." They were not actually removed when the fixture was isolated in f1b7ce6.
Suggested fix: revert the two additions in tests/fixtures/app-basic/middleware.ts (the lines 89-97 branch and the line 374 matcher entry) so the shared fixture is untouched, matching the stated intent.
Verification
vp check/vitest run tests/app-page-render-identity.test.ts tests/app-page-element-builder.test.ts→ 44 passed.- Removing the orphaned middleware lines is safe: nothing in the suite exercises that route.
Overall: the actual fix is good and should ship. Please drop the orphaned app-basic/middleware.ts additions first.
|
Review posted to PR #2358. SummaryThe core fix in PR #2358 is correct, minimal, and well-tested. It threads a new optional
One issue found (non-fatal, should fix before merge): orphaned dead code in
There are no fixture pages and no tests referencing that route — the only matches in the tree are the orphaned lines themselves. This contradicts the PR comment claiming the shared app-basic middleware changes were removed. I recommended reverting those two additions. |
Performance benchmarksCompared 1 improved · 0 regressed · 5 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): preserve rewritten route identity
Re-reviewed at 8e7d88c (HEAD). The core fix is correct, minimal, well-tested, and the prior review's blocker has been resolved. Recommend merge.
Core fix — correct (re-verified)
createAppPageRenderIdentity now threads an optional matchedRoutePathname (wired to routePath at app-page-element-builder.ts:229) and derives routeId/pageId from it instead of displayPathname. After a middleware rewrite, displayPathname is the visible URL (route A) while routePath is the post-rewrite matched route (B); encoding the keys from the visible URL produced a payload with B's params/segments but A's __route/page keys, tripping the client RSC error boundary.
I traced and confirmed:
- Pre/post-rewrite split is the right pair.
app-rsc-handler.ts:506capturescanonicalPathname = cleanPathnamebefore rewrites mutatecleanPathname, and passes it asdisplayPathnameatapp-rsc-handler.ts:1018.routePathis the rewritten match. Splitting visible vs matched here is exactly correct. - Consistency with existing fallback.
app-page-route-wiring.tsx:546-550already encodesrouteId/pageIdfromoptions.routePathwhenrenderIdentityis absent. Before this PR therenderIdentity-present path disagreed (useddisplayPathname); the fix makes both paths consistent, and consistent with the rest of the element map (params/segments derive from route B). routePathis always absolute (basePath-stripped request pathname), so the new call intonormalizeAppPageRenderMatchedPathnamewill not spuriously throw its absolute-path assertion.- Interception precedence preserved.
matchedRoutePathname = sourceMatchedPathname ?? requestedMatchedRoutePathnamekeeps interception-source identity winning;targetMatchedPathnamestill derives fromdisplayPathname, sotargetMatchedUrl/targetRouteIdare unaffected. Interception unit tests pass unchanged. - Backward compatible. New input is optional, defaults to
displayPathname; single call site.
Prior blocker resolved
The orphaned app-basic/middleware.ts additions flagged in the previous review were removed in commit 8e7d88c ("remove orphaned rewrite fixture", -17 lines). A repo-wide search for nextjs-compat/mismatching-prefetch now returns zero matches — the shared fixture is clean and the PR comment ("removes shared app-basic middleware changes") is now accurate.
Tests
- 44 unit tests pass (
app-page-render-identity.test.ts+app-page-element-builder.test.ts). - The new builder-level test keys the element map on the
routePath-derived id whendisplayPathname !== routePathand asserts the/visiblekeys are absent — closes the wiring-seam gap from the first review. - The browser spec follows the established self-contained build-and-serve pattern (
linkFixtureNodeModules/buildApp/runPrerender/startProdServer/closeServer) used byssr-error-shell-recovery.browser.spec.tsandhash-rsc-requests.browser.spec.ts. It is a strong regression: content B renders while the URL stays at A,documentRequestsis empty, and the window marker survives — proving a soft SPA transition rather than a hard reload. vp checkclean on both changed source files.
Minor (non-blocking) — test does not assert the loading shell
The spec creates loading.tsx (#dynamic-page-loading-a) and the PR description states the test "asserts the loading boundary from A renders," but the spec never asserts #dynamic-page-loading-a is visible. The loading.tsx is still functionally necessary (it creates the Suspense boundary that drives the prefetch-loading-shell request the test waits on), so this doesn't weaken the core assertion — but adding await expect(page.locator("#dynamic-page-loading-a")).toBeVisible() between the click and the content-B assertion would match the stated intent and tighten the regression. Optional.
Overall: correct, narrowly scoped, the previous blocker is fixed. LGTM.
|
Review posted to PR #2358. SummaryI re-reviewed PR #2358 at HEAD ( Core fix — verified correct:
Prior blocker resolved: The orphaned Verification I ran:
One minor non-blocking note: the spec creates |
…-route-identity # Conflicts: # packages/vinext/src/server/app-page-element-builder.ts # packages/vinext/src/server/app-page-render-identity.ts
1 participant
Summary
Next.js parity
Targets
test/e2e/app-dir/concurrent-navigations/mismatching-prefetch.test.tsfrom Next.js v16.2.6.The baseline deploy suite stops earlier in that test while waiting for Next.js's Suspense-derived partial prefetch request. Vinext's equivalent explicit loading-boundary fixture reproduces the latent next-stage failure: the response had params/segment data for B but
__routeand page element keys for A, causing the client RSC error boundary. This PR fixes that independently reproduced route-identity mismatch. The exact upstream suite still requires the separate Suspense shell-classification parity gap before it can reach this assertion.Validation
vp checkon all changed filesvp test run tests/app-page-render-identity.test.ts tests/app-page-element-builder.test.ts— 43 passedvp run vinext#buildExcludes cacheComponents, PPR, resume, fallback-shell, and external endpoint failures.