fix: App Router route-group and slot collision by JaredStowell · Pull Request #447 · cloudflare/vinext

JaredStowell · 2026-03-11T04:30:40Z

Fix App Router collision handling when multiple filesystem entries resolve to the same visible URL.

This changes route discovery and validation to:

reject duplicate normalized route patterns after route-group elision
treat /foo and /foo/ as the same route during validation
reject grouped slot sub-pages that collapse to the same URL within the same slot
reject slot sub-pages that collide with route.ts handlers
preserve shadowing semantics for same-named slots at different layout depths
merge matching slot sub-pages from different slots onto the same synthesized route

Details

In app-router.ts:

track slot ownership with internal ownerDir metadata
key synthetic slot merges by (slot name, owner dir) instead of slot name alone
normalize visible slot subpaths before duplicate detection
throw on page-vs-route-handler collisions
stop generating slash-suffixed synthetic patterns for empty/group-only subpaths

In route-validation.ts:

canonicalize trailing slashes before duplicate checks and dynamic-route validation

Tests

Added regressions for:

duplicate normalized patterns
slash-equivalent patterns
route-group duplicates like (a)/about vs (b)/about
grouped slot sub-page duplicates within one slot
grouped slot sub-pages from different slots merging correctly
slot sub-pages colliding with route.ts
parent slot sub-pages not overwriting a shadowing child slot of the same name

Verification

Passed:

pnpm test tests/route-sorting.test.ts
pnpm test tests/routing.test.ts
pnpm test tests/app-router.test.ts
pnpm run fmt
pnpm run typecheck

pkg-pr-new · 2026-03-11T04:31:19Z

Open in StackBlitz

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

commit: dd04025

james-elicx · 2026-03-11T12:40:42Z

/bigbonk review

ask-bonk

Good fix — the collision handling for route groups and parallel slot sub-pages is a real gap that needed closing. The test coverage is thorough and the error messages match Next.js conventions. A few items worth addressing before merge:

routeSegments behavioral change (bug): Synthetic routes now strip route groups from routeSegments, contradicting the field's contract. This affects useSelectedLayoutSegments() at runtime.
Redundant convertSegmentsToRouteParts call: The stored routeSegments are already visible-only segments, so the second conversion call is doing work that was already done.
Missing dedup check for syntheticRoutes: The old code checked syntheticRoutes.some(...) to skip/merge duplicates. The new code relies on validateRoutePatterns to catch this, which means it will throw instead of merging — a different (arguably better) behavior, but worth being intentional about.
ownerDir could be required: It's always set in discoverParallelSlots, so the type should reflect that.

ask-bonk · 2026-03-11T12:43:57Z

+
+        if (!subPathEntry) {
+          subPathEntry = {
+            routeSegments: normalizeVisibleRouteSegments(subSegments),


Bug: normalizeVisibleRouteSegments strips route groups from subSegments, and this result is later spread into AppRoute.routeSegments at line 328:

routeSegments: [...parentRoute.routeSegments, ...subSegments],

But routeSegments is documented (line 101-104) as:

"Includes route groups and dynamic segments (as template strings like [id]). Used at render time to compute the child segments for useSelectedLayoutSegments()."

The old code used subPath.split(path.sep) directly, which preserved route groups in routeSegments. The new code strips them. This could cause useSelectedLayoutSegments() to return incorrect results for synthetic slot sub-routes.

You should store the original subSegments (from relativePath.split(path.sep)) separately for use in routeSegments, and only use the normalized visible segments for dedup keying and URL pattern generation.

ask-bonk · 2026-03-11T12:43:57Z

-      // Convert sub-path segments to URL pattern parts
-      const subSegments = subPath.split(path.sep);
+    for (const { routeSegments: subSegments, slotPages } of subPathMap.values()) {
      const convertedSubRoute = convertSegmentsToRouteParts(subSegments);


Minor: convertSegmentsToRouteParts(subSegments) is called here on segments that have already been filtered by normalizeVisibleRouteSegments (no route groups, no @ slots, no .). This means the route-group and slot-skip branches inside convertSegmentsToRouteParts will never trigger. Not a bug, but it's redundant work and slightly confusing — the reader has to trace back to realize these segments are already stripped.

Consider storing the convertedSubRoute result from the first call (line 250) alongside routeSegments in the map, so you don't need to re-derive it here.

ask-bonk · 2026-03-11T12:43:57Z

  /** Slot name (e.g. "team" from @team) */
  name: string;
+  /** Absolute path to the @slot directory that owns this slot. Internal routing metadata. */
+  ownerDir?: string;


ownerDir is always set when a ParallelSlot is created by discoverParallelSlots (line 697). The optional ? type means every consumer has to handle undefined, but it never actually is. Consider making it required (non-optional) and providing a meaningful default or assertion at the creation site. The slotKey helper already has a ?? "" fallback for the undefined case that would be dead code if the type were tightened.

ask-bonk · 2026-03-11T12:43:57Z

-      if (syntheticRoutes.some((r) => r.pattern === pattern)) continue;
+      const pattern = joinRoutePattern(parentRoute.pattern, subUrlPath);
+
+      const existingRoute = routes.find((route) => route.pattern === pattern);


Nit: routes.find(...) is O(n) per sub-path per parent route. The old code used existingPatterns = new Set(routes.map(r => r.pattern)) which was O(1) lookup. With many routes and many slots this could add up. Consider building a Map<string, AppRoute> keyed by pattern before the loop (similar to the old Set) for O(1) lookups:

Suggested change

const existingRoute = routes.find((route) => route.pattern === pattern);

const existingRoute = routesByPattern.get(pattern);

(Where routesByPattern is built once before the outer loop as new Map(routes.map(r => [r.pattern, r])))

ask-bonk · 2026-03-11T12:43:57Z

  return false;
 }

+function normalizeVisibleRouteSegments(segments: string[]): string[] {


This function is essentially a subset of convertSegmentsToRouteParts — both skip ., route groups, and @ slots. The only difference is that convertSegmentsToRouteParts also converts dynamic segments ([id] -> :id) while this one keeps them as-is.

Consider adding a comment noting the intentional difference, or refactoring to share the filtering logic. As-is, a future maintainer might not realize these two functions need to stay in sync if a new invisible segment type is added.

ask-bonk · 2026-03-11T12:43:57Z

    .replace(/:([\w-]+)/g, "[$1]");
 }

+function normalizeRoutePattern(pattern: string): string {


Minor: replace(/\/+$/g, "") — the g flag is unnecessary here since $ anchors the match to the end of the string, so there can only ever be one match. Not a bug, just noise:

Suggested change

function normalizeRoutePattern(pattern: string): string {

function normalizeRoutePattern(pattern: string): string {

if (pattern === "/") return "/";

const normalized = pattern.replace(/\/+$/, "");

return normalized === "" ? "/" : normalized;

}

ask-bonk · 2026-03-11T12:44:07Z

Review posted successfully. Here's a summary of the feedback:

Key finding (bug): Synthetic routes now strip route groups from routeSegments, which contradicts the field's documented contract ("Includes route groups and dynamic segments"). The old code preserved route groups in routeSegments for synthetic slot sub-routes; the new code uses normalizeVisibleRouteSegments which strips them. This could break useSelectedLayoutSegments() at runtime for these routes.

Other feedback:

Redundant double-call to convertSegmentsToRouteParts — the second call operates on already-filtered segments
routes.find(...) is O(n) per lookup where the old code used a Set for O(1) — suggest building a Map<string, AppRoute> by pattern
ownerDir is always set but typed as optional, adding unnecessary null-handling burden
normalizeVisibleRouteSegments duplicates filtering logic from convertSegmentsToRouteParts without a comment explaining why both exist
Minor regex nit on the g flag in normalizeRoutePattern

github run

…e-group-collisions

- Fix routeSegments bug: store raw filesystem segments (with route groups) instead of normalizeVisibleRouteSegments output, so useSelectedLayoutSegments() sees the correct segment list at runtime for synthetic slot sub-routes - Eliminate redundant convertSegmentsToRouteParts call: store the pre-computed result in subPathMap alongside rawSegments, reuse it in the merge loop instead of re-deriving from already-stripped segments - Make ownerDir required (non-optional) in ParallelSlot: it is always set by discoverParallelSlots; remove the dead ?? '' fallback in slotKey - Replace O(n) routes.find() with O(1) Map lookup: build routesByPattern before the outer loop and keep it updated as synthetic routes are added - Add comment to normalizeVisibleRouteSegments explaining the intentional difference from convertSegmentsToRouteParts (dynamic segments not converted) and that both functions must stay in sync - Remove spurious g flag from normalizeRoutePattern regex: $ anchors to end of string so there can only ever be one match

… to convertSegmentsToRouteParts

james-elicx · 2026-03-11T13:09:50Z

/bigbonk re-review

ask-bonk

Solid fix. The collision detection for route groups and parallel slot sub-pages is well-structured, the test coverage is thorough, and the error messages match Next.js conventions. All three relevant test suites pass and typecheck is clean.

The previous automated review flagged a routeSegments bug (route groups being stripped), but that was incorrect — the current code correctly stores rawSegments (preserving route groups) for routeSegments and only uses urlSegments (groups stripped) for dedup keying and pattern generation. The __resolveChildSegments function in app-rsc-entry.ts will see the correct segment list.

A few minor observations below, none blocking.

ask-bonk · 2026-03-11T13:13:36Z

+        let subPathEntry = subPathMap.get(normalizedSubPath);
+
+        if (!subPathEntry) {
+          subPathEntry = {


Minor: When two different slots contribute sub-pages to the same normalized path (e.g., @team/(a)/members and @analytics/(b)/members), rawSegments is set from whichever slot is processed first. This means routeSegments for the synthesized route will contain route-group names from an arbitrary slot.

In practice this is harmless — route groups are invisible in URLs and __resolveChildSegments passes them through as-is — but it's worth a brief comment noting the first-writer-wins behavior so a future reader doesn't think it's a bug.

ask-bonk · 2026-03-11T13:13:36Z


+function normalizeRoutePattern(pattern: string): string {
+  if (pattern === "/") return "/";
+  const normalized = pattern.replace(/\/+$/, "");


Ultra-minor: the g flag on this regex is unnecessary since $ anchors the match to the end of the string, so there can only ever be one match. Not a bug, just noise.

Suggested change

const normalized = pattern.replace(/\/+$/, "");

const normalized = pattern.replace(/\/+$/, "");

ask-bonk · 2026-03-11T13:13:36Z

+
+      const existingRoute = routesByPattern.get(pattern);
+      if (existingRoute) {
+        if (existingRoute.routePath && !existingRoute.pagePath) {


This collision check catches the case where a slot sub-page targets a pattern that already has a route.ts handler (no pagePath). Good.

Worth noting: the inverse case — a route.ts handler discovered after a synthetic slot sub-page — is also caught because the synthetic route is added to routesByPattern (line 341), so the next call to fileToAppRoute for the route handler won't produce a duplicate pattern (it goes through the regular validateRoutePatterns call at line 181). The coverage is complete.

ask-bonk · 2026-03-11T13:13:46Z

Review submitted. Here's a summary of my findings:

Approved. The PR correctly addresses App Router collision handling for route groups and parallel slot sub-pages. Key points from my review:

Previous review's routeSegments bug was a false positive — The earlier automated review claimed routeSegments was stripping route groups, but the current code correctly stores rawSegments (original filesystem segments with route groups preserved) for routeSegments, and only uses the normalized urlSegments for dedup keying and pattern generation. useSelectedLayoutSegments() will work correctly.
All tests pass — route-sorting.test.ts (37 tests), routing.test.ts (82 tests), and app-router.test.ts (245 tests) all green. Typecheck clean.
Minor observations (non-blocking):
- rawSegments uses first-writer-wins when multiple slots contribute to the same normalized path — harmless but worth a comment
- Unnecessary g flag on the trailing-slash regex in normalizeRoutePattern
- The collision coverage is complete for both directions (slot sub-page targeting existing route handler, and route handler targeting existing synthetic route)

github run

Fix app router route-group conflicts

4f222c6

Review app router slot discovery

c86efe8

ask-bonk Bot reviewed Mar 11, 2026

View reviewed changes

james-elicx added 3 commits March 11, 2026 12:59

Merge remote-tracking branch 'origin/main' into jstowell/fix-app-rout…

af29375

…e-group-collisions

fix: remove now-unused normalizeVisibleRouteSegments, add doc comment…

91be782

… to convertSegmentsToRouteParts

ask-bonk Bot approved these changes Mar 11, 2026

View reviewed changes

james-elicx merged commit 17dd075 into cloudflare:main Mar 11, 2026
19 checks passed

	const existingRoute = routes.find((route) => route.pattern === pattern);
	const existingRoute = routesByPattern.get(pattern);

	const normalized = pattern.replace(/\/+$/, "");
	const normalized = pattern.replace(/\/+$/, "");

Conversation

JaredStowell commented Mar 11, 2026

Details

Tests

Verification

Uh oh!

pkg-pr-new Bot commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

james-elicx commented Mar 11, 2026

Uh oh!

ask-bonk Bot left a comment

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot commented Mar 11, 2026

Uh oh!

james-elicx commented Mar 11, 2026

Uh oh!

ask-bonk Bot left a comment

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot commented Mar 11, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pkg-pr-new Bot commented Mar 11, 2026 •

edited

Loading