fix(routing): correct (.) interception target for nested slot subdirectories#1751
Conversation
…ctories When the (.) interception marker lives inside a subdirectory of the @slot dir (e.g. app/@modal/sub/(.)target/[id]/page.tsx), baseParts was computed from routeDir (the slot owner) instead of the directory containing the marker. This caused visible subdirectory segments to be dropped, producing /target/:id instead of /sub/target/:id. Fix: derive baseParts from path.dirname(interceptRoot) relative to appDir, stripping invisible segments via convertSegmentsToRouteParts — the same pattern used by computeInterceptSourceMatchPattern and the (..) / (...) convention branches.
commit: |
|
/bigbonk review |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review |
There was a problem hiding this comment.
Review: (.) interception target for nested slot subdirectories
The core fix is correct for the URL pattern and matches Next.js' extractInterceptionRouteInformation logic: for (.), the interceptedRoute is normalizeAppPath(<path before marker>) + '/' + <intercepted>. So app/@modal/sub/(.)target/[id] → /sub/target/:id is exactly right. Nice catch on the root cause (using routeDir-relative segments instead of the marker's parent dir).
I did find one latent bug and a test gap.
Bug: dynamic ancestor params are dropped (double-conversion)
The new case "." sets baseParts to already-converted URL segments (convertedParent.urlSegments, e.g. :locale). Those segments are then re-fed into convertSegmentsToRouteParts(allSegments) at line 2240. On the second pass, :locale no longer matches the [param] regex, so it falls through to the static branch and the locale param is dropped from params.
Reproduction (logic-level): app/[locale]/@modal/(.)photos/[id] →
- pattern:
/:locale/photos/:id✅ (correct) - params:
['id']❌ (should be['locale', 'id'])
The (..)/(../..) branches don't have this problem because they slice raw filesystem segments (routeSegments.slice(...), still bracketed) and let the single final conversion extract params. The new (.) branch breaks that invariant.
The cleanest fix is to keep baseParts as the raw segments (dirname of the marker, relative to appDir) and let the existing final convertSegmentsToRouteParts(allSegments) do all the stripping/param extraction in one place — matching the (..) branch. I verified this yields correct results for all three cases: nested-subdir (/sub/target/:id, params ['id']), dynamic-ancestor (/:locale/photos/:id, params ['locale','id']), and root-level (/nested, params []). See the inline suggestion.
Test gap
collectIntercepts in the test doesn't expose params, so the new regression test can't catch the dropped-param case. Please add a case with a dynamic ancestor segment (e.g. app/[locale]/@modal/(.)photos/[id]) and assert on params to lock in ['locale','id'].
Overall: the direction is right and matches Next.js, but I'd hold merge until the params double-conversion is addressed and covered by a test.
| const convertedParent = convertSegmentsToRouteParts(interceptParentSegments); | ||
| baseParts = convertedParent | ||
| ? convertedParent.urlSegments | ||
| : interceptParentSegments.filter((s) => !isInvisibleSegment(s)); |
There was a problem hiding this comment.
Bug: baseParts here are already-converted URL segments (e.g. :locale). They get re-fed into convertSegmentsToRouteParts(allSegments) at line 2240, where :locale no longer matches the [param] regex and is treated as static — so dynamic ancestor params (like locale) are dropped from params.
Keep baseParts as raw segments and let the single final conversion at line 2240 extract params, matching the (..) branch:
| : interceptParentSegments.filter((s) => !isInvisibleSegment(s)); | |
| const interceptParentDir = path.dirname(interceptRoot); | |
| // Use raw filesystem segments here. Invisible segments (@slot, route | |
| // groups) and dynamic [param] syntax are resolved by the single | |
| // convertSegmentsToRouteParts call below; feeding already-converted | |
| // segments would drop dynamic ancestor params on the second pass. | |
| baseParts = path.relative(appDir, interceptParentDir).split(path.sep).filter(Boolean); |
Note: convertSegmentsToRouteParts already skips invisible segments, so @modal/route groups are still stripped correctly. Verified for nested-subdir (/sub/target/:id), dynamic-ancestor (/:locale/photos/:id, params ['locale','id']), and root-level (/nested).
|
Review posted successfully: #1751 (review) SummaryI reviewed PR #1751, which fixes What's correct:
Issue found (would block merge):
I posted this as a single batched review (one summary + one inline suggestion) rather than editing any files, per fork-review-mode restrictions. |
… preserve dynamic ancestor params
|
/bigbonk re-review for issues |
There was a problem hiding this comment.
Re-review: both prior concerns are resolved ✅
Thanks for the quick turnaround. I verified the latest state of the diff against the source.
1. Double-conversion param drop — fixed
case "." now derives baseParts from raw filesystem segments:
const interceptParentDir = path.dirname(interceptRoot);
baseParts = path.relative(appDir, interceptParentDir).split(path.sep).filter(Boolean);Since interceptRoot is the marker directory itself (interceptDir, set at app-route-graph.ts:2024), path.dirname(interceptRoot) correctly yields the directory containing the marker. The raw [param] segments flow into the single convertSegmentsToRouteParts(allSegments) call (line 2237), which both skips invisible segments (isInvisibleSegment, line 2305) and extracts dynamic params (line 2333). This matches the (..)/(../..) raw-slice invariant and mirrors computeInterceptSourceMatchPattern (line 2146) exactly. No more double conversion.
Traced for app/[locale]/@modal/(.)photos/[id]:
interceptParentDir=app/[locale]/@modalbaseParts=["[locale]", "@modal"](raw)- after conversion:
@modalstripped,[locale]→:locale→ pattern/:locale/photos/:id, params["locale", "id"]✅
2. Test gap — fixed
The new test "includes dynamic ancestor params for (.) slot with a dynamic ancestor segment" exposes params via collectIntercepts and asserts params: ["locale", "id"], locking in the regression. The nested-subdir test covers /sub/target/:id + sourceMatchPattern: "/sub".
Verification
vp test run tests/app-route-graph.test.ts→ 43 passed.- Behavior matches Next.js
extractInterceptionRouteInformation: for(.),interceptedRoute = normalizeAppPath(<path before marker>) + '/' + <intercepted>.
LGTM.
|
Posted an approving review on PR #1751. SummaryThis is a re-review (
I ran |
Description
When the
(.)interception marker lives inside a subdirectory of the@slotdir (e.g.app/@modal/sub/(.)target/[id]/page.tsx),computeInterceptTargetwas computingbasePartsfromrouteDir(the parent of the slot, always the route that owns@slot). That value omits any subdirectory segments between the slot root and the marker directory, producing/target/:idinstead of the correct/sub/target/:id.Root cause:
case "."usedrouteSegments = path.relative(appDir, routeDir), which is correct only when the marker is directly inside@slot. When the marker is nested deeper,routeDirstill points to the slot owner, dropping the intermediate visible segments.Fix: Derive
basePartsfrompath.dirname(interceptRoot)(the directory containing the marker) relative toappDir, stripping invisible segments viaconvertSegmentsToRouteParts— the same pattern used bycomputeInterceptSourceMatchPatternand the(..)/(...)branches.Fixes issue #1364 Part A.
Related Issue
Part of #1364
Potential Risk & Impact
case "."branch ofcomputeInterceptTarget. The(..),(../..), and(...)branches are unaffected.app/@modal/(.)photo) is unchanged:path.dirname(interceptRoot)==@modaldir, which strips the invisible@modalsegment, yielding emptybaseParts— same as before.app-route-graphtests pass.How Has This Been Tested?
tests/app-route-graph.test.ts("computes target with subdirectory prefix for (.) slot nested in a slot subdirectory") that fails before the fix and passes after.tests/app-route-graph.test.tspass.vp checkreports only pre-existing lint errors (unchanged from base).