fix: intercept routes (..) climb visible route segments, not filesystem dirs#499
Conversation
commit: |
|
/bigbonk review |
There was a problem hiding this comment.
Review: Fix intercept route segment climbing
This is a well-executed fix for a real correctness bug. The core insight is correct: Next.js intercept conventions (..) and (..)(..) climb by visible route segments, not filesystem directories. The old path.dirname() approach counted route groups like (marketing) as levels, producing wrong targets.
What I verified
-
The climbing logic is correct. Walking backward through
routeSegments, skipping invisible segments (route groups,@slots), and only counting visible ones matches Next.js behavior. Traced the full call chain fromdiscoverInterceptingRoutes→computeInterceptTargetto confirmrouteDiris always the page-owning directory (never the@slotor intercept dir). -
basePartscan still contain invisible segments after climbing (e.g.,["a", "(group)"]after climbing 1 level from["a", "(group)", "b"]). This is correctly handled becauseconvertSegmentsToRoutePartsindependently filters them out viaisInvisibleSegmenton line 976. -
isInvisibleSegmentconsolidation is sound. The three call sites (computeInterceptTarget,convertSegmentsToRouteParts,hasRemainingVisibleSegments) now use a single source of truth. The behavioral change inhasRemainingVisibleSegments— now treating"."as invisible, which it didn't before — makes it consistent withconvertSegmentsToRouteParts(which already skipped"."). In practice"."never appears in these segments because callers usefilter(Boolean)or guard withdir === "." ? [], so it's a correctness-for-consistency fix, not a behavior change. -
Edge case:
(.),(..)as segment names — these would matchisInvisibleSegment'sstartsWith("(")/endsWith(")")check. However, they cannot appear inrouteSegmentsbecause the page scanner excludes@-prefixed directories, and intercept convention directories only live inside@slotdirs. Not a real concern. -
Edge case: climbing more levels than visible segments exist — the
while (cutIndex > 0 && climbed < levelsToClimb)loop handles this gracefully by stopping at index 0, producingbaseParts = [](same as root). This matches Next.js behavior where over-climbing resolves to root. -
All 90 routing tests pass, including the 3 new test cases.
Tests
The three new tests provide good coverage:
(..)(..)with a single route group between segments(..)with a single route group(..)(..)with multiple consecutive route groups
Clean PR — LGTM.
| * Compute the target URL pattern for an intercepting route. | ||
| * | ||
| * Interception conventions (..), (..)(..)" climb by *visible route segments* | ||
| * (not filesystem directories). Route groups like (marketing) and parallel |
There was a problem hiding this comment.
Nit: stray " in the doc comment — (..)(..)".
| * (not filesystem directories). Route groups like (marketing) and parallel | |
| * Interception conventions (..), (..)(..) climb by *visible route segments* |
| case "../..": | ||
| baseDir = path.dirname(path.dirname(routeDir)); | ||
| case "../..": { | ||
| const levelsToClimb = convention === ".." ? 1 : 2; |
There was a problem hiding this comment.
Worth noting: if levelsToClimb exceeds the available visible segments, the loop exits with cutIndex = 0 and climbed < levelsToClimb. This silently resolves to root, which matches Next.js behavior. If you ever want to emit a warning for "(..)(..) used but only 1 visible segment above", this is where it would go — but no action needed now.
|
Review submitted. Here's a summary of my findings: Approved -- this is a correct fix for a real bug where Key points from the review:
I left two minor inline comments: a stray |
…em dirs computeInterceptTarget() used path.dirname() to climb parent directories, but Next.js defines interception levels in terms of route segments. Route groups like (marketing) are filesystem directories but invisible in the URL, so path.dirname() would waste a climb on them. Now walks backward through segments, counting only visible ones (skipping route groups, parallel slots, and "."). Also consolidates three copies of invisible-segment detection into a single isInvisibleSegment() helper.
a3c4e94 to
1dd9ac9
Compare
Summary
computeInterceptTarget()to climb by visible route segments instead of filesystem directories. Previously it usedpath.dirname()which counted route groups like(marketing)as levels, causing(..)and(..)(..)to resolve to the wrong target when route groups sit between visible segments.isInvisibleSegment()helper, replacing three duplicated inline checks acrosscomputeInterceptTarget,convertSegmentsToRouteParts, andhasRemainingVisibleSegments.Example of the bug
Visible route path:
/a/b.(..)(..)should climb 2 visible segments → root → intercept/target.Before:
path.dirname(path.dirname("app/a/(group)/b"))="app/a"→ produced/a/target(wrong — climbedbthen(group), wasting a level on the invisible route group).After: Walks backward through
["a", "(group)", "b"], counting only visible segments (b→a), correctly resolving to/target.Test plan
(..)and(..)(..)with route groups between segments