fix(router): honor hybrid pages route priority#1997
Conversation
commit: |
NathanDrake2406
force-pushed
the
nathan/use-params-parity
branch
from
71354f6 to
7be280e
Compare
PR cloudflare#1997 fixed the server-side route ownership for direct document loads, but the same invariant broke for client-side soft navigations and the matching prefetch path: - navigateClientSide() always delegated to the App runtime's RSC fetch, even when the Pages route had higher priority. renderPagesFallback() short-circuits RSC requests with null, so the App catch-all won. - prefetchUrl() prefetched an RSC stream for any URL that matched an App route, again ignoring Pages ownership. - The Pages entry was loaded on every hybrid request, even when a static App route had already matched (Pages can never win in that case). Expose the Pages route manifest on the client via a new __VINEXT_PAGES_LINK_PREFETCH_ROUTES__ window global emitted by both the App and Pages browser entries. The link shim consults a shared resolveHybridClientRouteOwner helper that mirrors the server-side pagesRouteHasPriorityOverAppRoute comparison. When Pages owns the URL, the click handler issues a window.location navigation and the prefetch path returns early. Gate the renderPagesFallback call behind a static-App-route check in handleAppRscRequest: when a static App route matches, the bridge cannot win, so skip the eager Pages entry load. Centralise the comparison in a new resolveHybridRouteOwner helper so server and client reach the same answer for the same (URL, route pair). Adds the missing client-navigation coverage to the use-params e2e fixture (Link from /app/ to /pages-dir/foobar) and unit tests for the shared owner decision.
PR cloudflare#1997 fixed the server-side route ownership for direct document loads, but the same invariant broke for client-side soft navigations and the matching prefetch path: - navigateClientSide() always delegated to the App runtime's RSC fetch, even when the Pages route had higher priority. renderPagesFallback() short-circuits RSC requests with null, so the App catch-all won. - prefetchUrl() prefetched an RSC stream for any URL that matched an App route, again ignoring Pages ownership. - The Pages entry was loaded on every hybrid request, even when a static App route had already matched (Pages can never win in that case). Expose the Pages route manifest on the client via a new __VINEXT_PAGES_LINK_PREFETCH_ROUTES__ window global emitted by both the App and Pages browser entries. The link shim consults a shared resolveHybridClientRouteOwner helper that mirrors the server-side pagesRouteHasPriorityOverAppRoute comparison. When Pages owns the URL, the click handler issues a window.location navigation and the prefetch path returns early. Gate the renderPagesFallback call behind a static-App-route check in handleAppRscRequest: when a static App route matches, the bridge cannot win, so skip the eager Pages entry load. Centralise the comparison in a new resolveHybridRouteOwner helper so server and client reach the same answer for the same (URL, route pair). Adds the missing client-navigation coverage to the use-params e2e fixture (Link from /app/ to /pages-dir/foobar) and unit tests for the shared owner decision.
PR review flagged two split-brain bugs in the previous hybrid
ownership fix: a hand-copied client comparator and a Link-only
ownership gate that left programmatic App Router navigations on
the wrong path.
The hand-copied routePrecedence in hybrid-client-route-owner.ts
omitted the static-prefix reduction that lives in
routing/utils.ts#routePrecedence, and used a strict-less-than
comparison that returned App for identical dynamic patterns. The
server returns Pages for both cases (Pages providers sort ahead
of App providers, and routePrecedence subtracts 50 per static
prefix segment). The split produced a real ownership disagreement
on overlapping patterns like /_sites/:slug* (Pages) vs /:slug*
(App).
Add a shared compareHybridRoutePatterns to routing/utils.ts as
the single source of truth: the static/dynamic short-circuits plus
a sortRoutes call (which carries the static-prefix reduction and
the Pages-first equal-pattern tiebreak). The server
pagesRouteHasPriorityOverAppRoute and the client
resolveHybridClientRouteOwner both delegate to it, so they
cannot diverge. Drop the hand-copied routePrecedence entirely.
Wire the ownership check at the App navigation runtime boundary
so useRouter().push, useRouter().replace, gesturePush, and form
submits all get the same hard-nav contract as the Link click
handler. Specifically:
- navigateClientSide: after same-origin normalization, if Pages
owns the URL, hard-navigate via window.location and return
(matching the existing external-URL branch).
- _appRouter.prefetch: short-circuit RSC URL construction for
Pages-owned targets so we do not warm an unusable cache entry.
Centralise the existing two inline hard-nav branches in
navigateClientSide into a hardNavigateTo helper for clarity.
Tests:
- Direct unit tests for compareHybridRoutePatterns covering
identical-dynamic tiebreak, static-prefix dynamic overlap,
static-prefix catch-all overlap, and infix-static bonus.
- Direct unit tests for resolveHybridClientRouteOwner mirroring
the server assertions plus a basePath-stripping test.
- e2e: useRouter().push('/pages-dir/foobar') from an App page
resolves to the Pages document.
- e2e: useRouter().prefetch('/pages-dir/foobar') issues zero
RSC requests for the target URL.
The hand-copied comparator note quoted the dynamic-segment scores (51 / 1000) for the optional-catch-all example (/_sites/:slug* vs /:slug*). The current optional-catch-all scoring actually produces 1951 vs 2000. The test assertion was correct; only the comment was stale.
NathanDrake2406
force-pushed
the
nathan/use-params-parity
branch
from
9559f37 to
6e45654
Compare
|
Addressed the review feedback:
Rebased onto latest |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
|
/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.
Reviewed the hybrid route-priority changes. The core feature (Pages dynamic route beating an App root catch-all) is well-covered by tests and the request-pipeline refactor keeps App and Pages rewrite handling in sync. A few things worth addressing before merge — mostly correctness-of-documentation and dead code, plus one comparator behavior question that affects parity claims.
Main concerns:
-
compareHybridRoutePatternsis documented as delegating tosortRoutes/routePrecedence, but it is actually a separate hand-rolled segment-rank comparator. The two disagree on a large number of dynamic-route pairs (I found ~95 divergences across a small pattern sample, e.g./:a/xvs/x/:a,/_sites/:a*vs/:a). The segment-rank approach may well be closer to Next.js's realgetSortedRoutes, but the comments/test comments asserting it "delegates to sortRoutes" are misleading and will mislead future maintainers about which algorithm is authoritative. Please either (a) actually route the comparison throughsortRoutes, or (b) fix the comments to state it intentionally implements Next.js's segment-tree ordering, which differs from vinext'sroutePrecedenceheuristic, and explain why that divergence is safe (it only arbitrates between two already-matched routes, so per-router trie ordering is unaffected). -
resolveHybridRouteOwneris exported and tested but has no production caller — the client path usesresolveHybridClientRouteOwnerand the server usespagesRouteHasPriorityOverAppRoutedirectly. Either wire it in or drop it to avoid dead surface area. -
The pages/app rewrite loops changed from "first matching rule, applied once" to "every matching rule applied sequentially (chained)" for
beforeFiles/afterFiles/fallback. App and Pages are now consistent with each other, which is good, but please confirm this chaining matches Next.js'sresolve-routesbehavior — particularly thatbeforeFilesis meant to chain rather than stop after the first match. This is a behavior change beyond the stated scope of the PR.
Nothing here is a hard blocker on the feature itself; the doc/dead-code items are quick fixes and the chaining question just needs a parity confirmation.
| * compareHybridRoutePatterns("/:path+", true, "/dashboard", false) // → "app" | ||
| * compareHybridRoutePatterns("/", false, "/", false) // → "app" | ||
| */ | ||
| export function compareHybridRoutePatterns( |
There was a problem hiding this comment.
This comment block and the test comments in tests/hybrid-route-priority.test.ts ("the shared comparator which delegates to sortRoutes") claim this delegates to sortRoutes, but the implementation below is an independent segment-rank comparator. It diverges from routePrecedence/sortRoutes on many dynamic-route pairs (e.g. /:a/x vs /x/:a). Please update the doc to state it intentionally mirrors Next.js's segment-tree ordering (distinct from vinext's routePrecedence heuristic) and note why arbitrating only between two already-matched routes keeps per-router trie ordering unaffected — otherwise a future reader will assume the two stay in lockstep.
| * server uses so client navigations, prefetch detection, and direct document | ||
| * loads all reach the same answer for the same route pair. | ||
| */ | ||
| export function resolveHybridRouteOwner<R extends HybridRoutePriorityRoute>( |
There was a problem hiding this comment.
resolveHybridRouteOwner is exported and unit-tested but has no production caller (the client uses resolveHybridClientRouteOwner, the server uses pagesRouteHasPriorityOverAppRoute). Consider wiring it into the client/server decision points it was designed for, or removing it to avoid dead code that has to be kept in sync with the real comparators.
| nextConfig?.pageExtensions, | ||
| fileMatcher, | ||
| ); | ||
| const appMatch = matchAppRoute(pipelineResult.resolvedUrl, appRoutes); |
There was a problem hiding this comment.
Minor inconsistency: line 3927 strips the query with .split("?")[0] before matching Pages, but this passes the full resolvedUrl (with query) to matchAppRoute. It happens to be harmless because matchRouteWithTrie strips the query internally (route-matching.ts:50), but mirroring the .split("?")[0] here would make the two matches visibly consistent.
| @@ -407,11 +412,11 @@ export async function runPagesRequest( | |||
|
|
|||
There was a problem hiding this comment.
This changes beforeFiles from "first matching rule, applied once" (the old single matchRewrite(..., configRewrites.beforeFiles, ...) call returns on first match) to "apply every matching rule in sequence" (no break). The App handler now does the same, so they're consistent — but please confirm chained beforeFiles rewrites match Next.js resolve-routes semantics. If Next.js stops after the first beforeFiles match, this would over-rewrite.
|
Review submitted successfully. Here's a summary of what I found in PR #1997. Review summaryThe core feature works and is well-tested. I posted one batch review with 4 inline comments. Findings: 1. Misleading documentation on 2. Dead code: 3. Rewrite chaining behavior change (pages-request-pipeline.ts:412) 4. Minor query-string inconsistency (index.ts:3937) No hard blockers — items 1, 2, and 4 are quick fixes, and item 3 just needs a parity confirmation against the AGENTS.md-mandated Next.js reference check. |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Re-reviewed after 9c5dc46 chore(router): clarify hybrid priority semantics. The three main concerns from the prior review look resolved:
- Misleading docs —
routing/utils.ts:100-119and the test comments inhybrid-route-priority.test.tsno longer claim the comparator "delegates to sortRoutes". They now state it intentionally implements Next.js's segment-tree ordering, distinct from vinext'sroutePrecedenceheuristic, and explain why arbitrating only between two already-matched routes keeps per-router trie ordering unaffected. - Dead code —
resolveHybridRouteOwnerwas removed; the remaining exports (pagesRouteHasPriorityOverAppRoute,validateHybridRouteConflicts,compareHybridRoutePatterns) all have production callers. - Query-string inconsistency (
index.tsdev path) — bothmatchRouteandmatchAppRoutenow consume the same query-strippedresolvedPathname. - Rewrite chaining — the sequential
beforeFilesbehavior is now documented (citing Next.jsserver-utils.ts) and covered by a new test (app-rsc-handler.test.ts:1193"applies sequential beforeFiles rewrites with accumulated query conditions").
Verified the segment-rank comparator matches Next.js getSortedRoutes on the divergent cases (/:a/x vs /x/:a → app; /_sites/:slug* vs /:slug* → pages). Ran hybrid-route-priority, hybrid-client-route-owner, pages-request-pipeline, and the rewrite-focused app-rsc-handler tests locally — all green (41 + 42 + 21 passing).
One new minor finding (not a blocker), same dead-surface-area category as the previously-removed resolveHybridRouteOwner:
matchesAppRoute (packages/vinext/src/shims/internal/app-route-detection.ts:109) now has no production callers. Both former callers switched to resolveHybridClientRouteOwner in this PR — markAppRouteDetectedOnPrefetch in the same file (line 131: if (resolveHybridClientRouteOwner(href, basePath) !== "app") return;) and performNavigation in router.ts:2249. It's still exported and exercised by tests/pages-router-app-prefetch-detection.test.ts, so it survives knip, but it's now exported-and-tested-but-unused production code — a second route-ownership predicate that has to be kept in sync with the comparator. Consider inlining it as a private helper of the trie match it wraps, or dropping it and folding its assertions into the resolveHybridClientRouteOwner suite.
|
Review submitted successfully. Here's a summary of what I found in PR #1997. Review summaryThe PR is in good shape. All three of the prior reviewer's main concerns were addressed in commit
I verified the segment-rank comparator matches Next.js One new minor finding (not a blocker): No hard blockers. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Reviewed the hybrid route-priority changes after the latest commits. The implementation is solid and the prior-review concerns are addressed:
resolveHybridRouteOwner— removed; all remaining exports (pagesRouteHasPriorityOverAppRoute,validateHybridRouteConflicts,compareHybridRoutePatterns) have production callers.matchesAppRoute— removed entirely fromapp-route-detection.ts;markAppRouteDetectedOnPrefetchnow callsresolveHybridClientRouteOwner.- Query-string consistency (
index.tsdev path) — bothmatchRouteandmatchAppRouteconsume the same query/hash-strippedresolvedPathname(3927-3946). - Rewrite chaining — App (
app-rsc-handler.ts:598-620) and Pages (pages-request-pipeline.ts:417-433)beforeFilesloops now both apply every matching rule with nobreak, documented and tested. They are consistent with each other. - Doc clarity in
routing/utils.ts—compareHybridRoutePatternsnow correctly documented as an intentional segment-tree implementation distinct fromroutePrecedence/sortRoutes.
I ran tests/hybrid-route-priority.test.ts and tests/hybrid-client-route-owner.test.ts locally — all 41 green.
One leftover doc issue (not a blocker): the patternFromParts JSDoc in hybrid-client-route-owner.ts still claims the comparator uses sortRoutes. This is the same misleading-documentation category that was fixed in routing/utils.ts but was missed in this file. Quick fix inline.
| * `sortRoutes` algorithm can score both Pages and App patterns. The | ||
| * `patternParts` array never includes an empty string for the static `/` | ||
| * route (the App catch-all handles the bare path), so the simple join is | ||
| * safe for everything the route trie actually matches. |
There was a problem hiding this comment.
Stale doc reference: this comment claims patternFromParts exists "so the same sortRoutes algorithm can score both Pages and App patterns," but resolveHybridClientRouteOwner (line 206) delegates to compareHybridRoutePatterns, which is the hand-rolled segment-rank comparator — not sortRoutes/routePrecedence. This is the same misleading-documentation issue that was fixed in routing/utils.ts (where the sortRoutes claim was replaced with the segment-tree wording), but it was missed here. A future maintainer reading this will assume sortRoutes is authoritative for hybrid ownership and may try to keep the two in lockstep.
| * safe for everything the route trie actually matches. | |
| * route-graph shape (`{ pattern: string }`) so the same | |
| * `compareHybridRoutePatterns` segment-rank comparator can score both Pages | |
| * and App patterns. The |
|
Review submitted successfully. Here's a summary of what I found in PR #1997. Review summaryThe hybrid route-priority feature is well-implemented and well-tested. I verified that all prior-review concerns have been addressed in the latest commits:
I traced the comparator logic against several edge cases (differing lengths, static-prefix dynamics, infix statics) and ran the two new test files locally — all 41 tests pass. I also confirmed both browser entries emit One leftover finding (not a blocker): the No hard blockers. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Reviewed the hybrid route-priority changes end-to-end. The implementation is solid and I confirmed every prior-review concern is resolved in the current branch state:
compareHybridRoutePatternsdocs (routing/utils.ts:100-119,hybrid-client-route-owner.ts:90-100) now correctly describe the hand-rolled segment-rank comparator as an intentional Next.js segment-tree implementation, distinct from theroutePrecedence/sortRoutesheuristic. The stalesortRoutesclaim inpatternFromPartsJSDoc that the last review flagged has been fixed.- Dead code removed —
resolveHybridRouteOwnerandmatchesAppRouteare both gone; all remaining exports (pagesRouteHasPriorityOverAppRoute,validateHybridRouteConflicts,compareHybridRoutePatterns,resolveHybridClientRouteOwner) have production callers. - Query-string consistency — the dev path in
index.ts:3927-3946strips query/hash intoresolvedPathnameand feeds it to bothmatchRouteandmatchAppRoute. - Rewrite chaining — App (
app-rsc-handler.ts:598-620) and Pages (pages-request-pipeline.ts:417-433)beforeFilesloops now both apply every matching rule with nobreak, and are consistent with each other and documented citing Next.jsserver-utils.ts.
I traced the comparator against the critical case (pages=/pages-dir/:dynamic vs app=/:path+ → pages owns) and the prior divergence cases (/:a/x vs /x/:a → app; /_sites/:slug* vs /:slug* → pages). Server pattern strings and client patternFromParts output use the same :param/:param+/:param* convention, so the client and server reach the same ownership decision.
No blockers. One minor non-blocking observation below.
| } | ||
|
|
||
| // Step 9: beforeFiles rewrites | ||
| // Next.js server-utils.ts applies every beforeFiles rule in sequence and |
There was a problem hiding this comment.
Non-blocking parity note: the comment cites Next.js server-utils.ts for the sequential beforeFiles behavior, but it's worth a quick double-check that Next.js's resolve-routes re-evaluates each beforeFiles rule's has/missing conditions against the post-previous-rewrite URL (which this loop does, since rewriteRequestContext() is re-read each iteration via the updated resolvedUrl/resolvedPathname), rather than against the original incoming request. The App handler at app-rsc-handler.ts:606-610 mirrors this by passing requestContextForResolvedUrl(..., resolvedUrl, ...) each iteration, so the two are consistent — just flagging it so the documented parity claim stays accurate if Next.js's behavior here is ever revisited.
|
Review submitted successfully. Here's a summary of what I found in PR #1997. Review summaryThe hybrid route-priority feature is well-implemented and well-tested. I reviewed the full implementation and confirmed that every concern from the prior review rounds has been resolved in the current branch state:
I traced the comparator against the critical PR case ( No blockers. I posted one non-blocking parity note on |
2 participants
Summary
/pages-dir/foobarmust renderpages/pages-dir/[dynamic]instead ofapp/[...path]./.Root Cause
Vinext kept App and Pages route ownership separate. In production, the App RSC handler only delegated to Pages when no App route matched, so
app/[...path]captured/pages-dir/foobarbefore the Pages Router route could render. Next.js registers Pages route matcher providers before App providers and sorts dynamic route pathnames together, sopages/pages-dir/[dynamic]has priority over an App root catch-all for this request.The implementation now uses the same sorted dynamic-route comparison only when dynamic route ownership is ambiguous. Static App/Page duplicates remain App-owned in Vinext, matching the existing Cloudflare Worker dev contract for the app-router-cloudflare fixture.
References
Verification
vp test run tests/hybrid-route-priority.test.ts tests/app-router-next-config-codegen.test.ts tests/app-pages-bridge.test.tsCI=1 PLAYWRIGHT_PROJECT=use-params-app-pages vp run test:e2ePLAYWRIGHT_PROJECT=cloudflare-dev vp run test:e2e tests/e2e/cloudflare-dev/pages-router.spec.ts tests/e2e/cloudflare-dev/middleware.spec.tsPLAYWRIGHT_PROJECT=app-router vp run test:e2e tests/e2e/app-router/pages-router-use-params.spec.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/app-dir/use-params/use-params.test.tsvp checkon the changed runtime, fixture, and test files