Repository navigation
fix(app-router): bind layout-only children defaults - #2335
Conversation
|
/bigbonk review for issues |
commit: |
|
|
/bigbonk review for issues |
Review: regression in layout-only parallel-slot parentsI ran the routing test suites against this branch. This PR introduces a regression in an existing, already-ported Next.js parity test that is not addressed in the diff. Failing test
I confirmed this by checking out Root causeThe new gate in the layout-scan loop ( const hasOwnedSlotPage = route.parallelSlots.some(
(slot) => path.dirname(slot.ownerDir) === routeDir && slot.pagePath !== null,
);
if (!route.pagePath && !hasOwnedSlotPage) {
ghostParentRoutes.push(route);
continue;
}The two scenarios are nearly identical in shape:
The distinguishing factor is whether a competing real route already covers the pattern — which is exactly what the pre-existing Suggested directionOnly demote the default-less layout-only owner to a ghost when a competing route actually covers its pattern. Folding the Minor
|
|
Posted the review on PR #2335. SummaryI found one blocking regression in this PR. Regression: Root cause. The new gate at Suggested fix. Gate the demotion on an actual pattern collision (fold the condition into the existing I also noted that the PR's validation didn't run The targeted tests the PR aimed to fix ( |
|
Addressed the blocking layout-only parent regression in 5d1f599:
/bigbonk review for issues |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
Re-review: layout-only parent regression is fixedCommit What changedThe old gate ghosted any default-less layout-only owner: const hasOwnedSlotPage = route.parallelSlots.some(
(slot) => path.dirname(slot.ownerDir) === routeDir && slot.pagePath !== null,
);
if (!route.pagePath && !hasOwnedSlotPage) { ghostParentRoutes.push(route); continue; }The new gate only ghosts when an optional catch-all sibling structurally covers the same pattern ( const optionalCatchAllOwnsPattern = routes.some(
(candidate) =>
candidate.patternParts.length === route.patternParts.length + 1 &&
candidate.patternParts.at(-1)?.endsWith("*") &&
patternsStructurallyEquivalent(candidate.patternParts.slice(0, -1), route.patternParts),
);
if (!route.pagePath && optionalCatchAllOwnsPattern) { ghostParentRoutes.push(route); continue; }This is correct:
VerificationI confirmed locally on this branch:
The new Nits (non-blocking)
LGTM — the regression is resolved and well-covered. |
|
Posted the re-review on PR #2335. SummaryThe blocking regression I flagged earlier is fixed by commit The fix. The old gate unconditionally ghosted any default-less layout-only slot owner. The new gate (
Verification (all passing on this branch):
Non-blocking nits: the comment block at lines 945-949 still only documents the route-group collision case (not the new optional-catch-all branch), and LGTM. |
|
Addressed fresh independent review finding at
/bigbonk review for issues |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
Final-head CI is fully green and an independent review returned NO FINDINGS for /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.
Review: layout-only children defaults at 8085354 — LGTM, no blocking issues
The prior Bonk runs reviewed earlier commits. This review covers the new combined diff at 80853544d5ece1ee2ff823a569ed6decb0c7ad13, which differs from 5d1f599 in two coupled ways that I verified are mutually consistent and correct.
What changed since 5d1f599
1. directoryToAppRoute now binds default.tsx as the implicit children page (app-route-graph.ts:1523):
const effectivePagePath = pagePath ?? (routePath ? null : findFile(routeDir, "default", matcher));I confirmed this only affects the layout-only parallel-slot scan loop (the only caller passing pagePath=null, routePath=null, line 962). The other caller (fileToAppRoute, line 1467) always passes either a real page path (→ effectivePagePath = pagePath) or a route path (→ routePath truthy → null, so API routes never pick up default.tsx). Good.
2. The optional-catch-all gate dropped its !route.pagePath guard (app-route-graph.ts:964-973). This is the necessary companion to change 1: now that a layout-only owner with a default.tsx gets a non-null pagePath, the old !route.pagePath && optionalCatchAllOwnsPattern condition would have stopped ghosting it, leaving /:locale as a competing route. Removing !route.pagePath keeps the default-backed owner ghosted, which is the parity target (parallel-routes-catchall-slotted-non-catchalls).
Why this is safe
- The gate only operates on routes constructed inside the layout loop. Real
page.tsxroutes are scanned first (lines 918-928) and pushed toroutesbefore the loop runs; they are never re-evaluated by the gate, so no explicit page route can be incorrectly ghosted. - In the layout loop,
effectivePagePathcan only ever resolve adefault.tsx(neverpage.tsx), so making the gate topology-only (rather thanpagePath-gated) is correct — there is no explicit-page case to protect here. - The standalone layout-only owner (no sibling catch-all) is preserved:
optionalCatchAllOwnsPatternis false androutePatterns.hasis false, so it stays a real route. Covered bytests/app-route-graph.test.ts"keeps a standalone layout-only slot owner as a route" andtests/routing.test.ts"discovers nested parallel slot sub-routes from layout-only parent" (the previously-regressed test). patternsStructurallyEquivalentcomparessegmentTreeNodeTypeper segment — literals require exact value match (literal:${seg}), dynamics collapse to"dynamic". This matches Next.js's tree-node comparison and avoids false positives across unrelated siblings.
Verification (all green on this branch at 8085354)
tests/app-route-graph.test.ts— 60 passed (incl. the 3 new ported parity tests + standalone-owner regression)tests/routing.test.ts+tests/route-sorting.test.ts+tests/build-report.test.ts— 254 passed (incl. the previously-regressed nested-slot test)tests/app-router.test.ts+tests/features.test.ts— 338 passedvp check packages/vinext/src/routing/app-route-graph.ts tests/app-route-graph.test.ts— clean (format, lint, types)
The new e2e fixture (parallel-slot-default/nested/[foo]/[bar]) has no sibling catch-all, so it is intentionally not ghosted and renders its default.tsx as children — directly exercising change 1 end-to-end.
Non-blocking nits (carried over, still applicable)
- The comment block at
app-route-graph.ts:945-949still documents only the route-group collision flavor of ghosting. Now there are twocontinuebranches (optional-catch-all at 970, route-group at 974); extending the comment to mentionoptionalCatchAllOwnsPatternwould help the next reader. candidate.patternParts.at(-1)?.endsWith("*")(line 967) matches any segment ending in*. In practice only optional catch-alls produce that, butsegmentTreeNodeType(candidate.patternParts.at(-1)) === "optionalCatchAll"would be more explicit and consistent with the helper used on the very next line.
LGTM — the two coupled changes are correct, the earlier regression stays fixed, and coverage is solid.
|
Posted the review on PR #2335. SummaryI reviewed the combined diff at The latest commit introduced two coupled changes that I verified are correct and mutually consistent:
Why it's safe: the gate only operates on routes built inside the layout loop; real Verification (all green on this branch):
Two non-blocking nits (carried over): the comment block at lines 945-949 doesn't document the new optional-catch-all LGTM. |
Summary
default.tsxas the implicit children page for layout-only App Router routesNext.js parity
Targets failures from run 28143992598:
parallel-routes-catchall-default: 1 test now passingparallel-routes-catchall-dynamic-segment: 1 test now passingparallel-routes-catchall-slotted-non-catchalls: 1 test now passingThe related specificity, layout soft-navigation, and no-children-slot failures still fail and are separate follow-up tracks.
Validation
vp check packages/vinext/src/routing/app-route-graph.ts tests/app-route-graph.test.ts tests/e2e/app-router/parallel-slot-default.spec.tsvp test run tests/app-route-graph.test.ts— 59 passedPLAYWRIGHT_PROJECT=app-router vp exec playwright test tests/e2e/app-router/parallel-slot-default.spec.ts --workers=1— 1 passed