Repository navigation
fix(app-router): discover nested leaf slot routes - #2350
Conversation
|
/bigbonk review for issues |
commit: |
|
|
/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: /bigbonk — found a blocking regression
The core fix is correct and matches Next.js behavior for the no-children leaf case (verified against the v16.2.6 fixture: /no-children/other renders the slot page with the parent default.tsx as children). But the change is too broad and breaks an existing test that encodes deliberate behavior.
Blocking: breaks tests/routing.test.ts:681
The new !hasNestedPages guard in discoverParallelSlots (packages/vinext/src/routing/app-route-graph.ts:2173) retains the slot unconditionally whenever it contains a nested page — regardless of whether the parent owns a children page. This regresses the pre-existing test:
appRouter - route discovery > does not discover nested @slot sub-routes when the slot root has no page or default(tests/routing.test.ts:681)
AssertionError: expected [ '/inbox', '/inbox/profile' ] to not include '/inbox/profile'
❯ tests/routing.test.ts:694:28
That fixture is:
inbox/page.tsx # parent HAS a children page
inbox/default.tsx # children default exists
inbox/@modal/profile/page.tsx # slot has only a nested page, no slot page/default
Before this PR, @modal was dropped by discoverParallelSlots (no page/default/intercept), so /inbox/profile was never minted. After this PR, the slot is retained, discoverSlotSubRoutes sees the parent inbox/default.tsx as childrenFallback, and /inbox/profile is now materialized — the exact opposite of what the test asserts.
Why this matters / Next.js parity
The PR's own comment says the intent is to materialize the leaf slot route "when the owner has no children page of its own" (app-route-graph.ts:2171), but the code never checks that. The distinction is real:
- PR's new fixture (
parallel-leaf-default/): parent has onlydefault.tsx(layout-only, nopage.tsx) → Next.js discovers/.../other. ✅ Correct. - Line-681 fixture (
inbox/): parent haspage.tsx+default.tsx→ vinext deliberately does not discover/inbox/profile. There is no Next.js test asserting this shape should be discovered, so flipping it silently diverges from a documented, intentional vinext decision.
The Next.js v16.2.6 no-children fixture (the one this PR ports) has the parent with only default.tsx, confirming the leaf behavior should be gated on the parent being layout-only — not applied whenever any nested page exists.
Suggested fix
Narrow the new guard so the slot is only retained for its nested pages when the owner has no children page (i.e. layout-only parent), matching the comment's stated intent. Then re-run tests/routing.test.ts (both line-663 and line-681 cases) in addition to tests/app-route-graph.test.ts, and add a unit test for the line-681 shape to lock in the distinction.
Process note
The PR validation list ran tests/app-route-graph.test.ts but not tests/routing.test.ts / tests/route-sorting.test.ts. Per AGENTS.md, routing changes should run tests/routing.test.ts and tests/route-sorting.test.ts — that would have caught this before CI.
The fixtures, the e2e regression, and the route-graph test additions are all good. Once the guard is scoped to layout-only parents (or the line-681 test is intentionally revised with documented Next.js justification — which I could not find), this is mergeable.
|
Posted the review on PR #2350. Summary of findings: Blocking regression foundThe PR's change in
Root causeThe PR's comment states the intent is to materialize the leaf route "when the owner has no children page of its own", but the code never checks that condition. The real distinction:
I verified the authoritative Next.js behavior: the v16.2.6 Recommended fixNarrow the guard to layout-only parents (matching the comment's stated intent), re-run The fixtures, e2e regression, and route-graph test additions themselves are all correct and well-referenced. |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: /bigbonk — LGTM, prior blocking regression is resolved
The earlier /bigbonk pass flagged a blocking regression: the original !hasNestedPages guard in discoverParallelSlots was unconditional and broke tests/routing.test.ts:681 ("does not discover nested @slot sub-routes when the slot root has no page or default"). Commit c544999 (fix(app-router): scope nested leaf slot discovery) fixes this correctly.
What the fix does
The guard is now gated behind a new opt-in includeNestedOnlySlots parameter (default false), threaded through directoryToAppRoute → discoverInheritedParallelSlots → discoverParallelSlots:
app-route-graph.ts:2182—hasNestedPagesonly counts whenincludeNestedOnlySlotsis set.app-route-graph.ts:1903— at the inherited-slot level it is further narrowed toincludeNestedOnlySlots && isActiveUrlLevel, so ancestor/inherited slots are never retained for nested pages alone.app-route-graph.ts:960,962— the flag is passedtrueonly from the ghost-parent loop, which scans**/layoutand therefore only fires for layout-bearing parents.
This is exactly the "layout-only parent" scoping the previous review asked for. The two fixtures are now correctly distinguished:
- Layout-only parent (
inbox/layout.tsx+default.tsx, nopage.tsx) →/inbox/profileis discovered (tests/routing.test.ts:702). Matches Next.js v16.2.6no-children. - Page-bearing parent (
inbox/page.tsx+default.tsx) →/inbox/profileis not discovered (tests/routing.test.ts:681). Theinboxroute is minted via the normal page path withincludeNestedOnlySlots=false, so@modalis dropped anddiscoverSlotSubRoutesskips it (no slots). The ghost loop never touches it (nolayout.tsx).
Verification (ran the AGENTS.md-required routing suites the original PR validation skipped)
vp test run tests/routing.test.ts— 113 passed (includes both line-681 and line-702 cases)vp test run tests/route-sorting.test.ts tests/app-route-graph.test.ts— 105 passedvp check packages/vinext/src/routing/app-route-graph.ts— format, lint, types clean
Notes
- Fixtures, the e2e regression (
advanced.spec.ts), and the route-graph unit tests are all correct and reference the authoritative Next.js v16.2.6 source. - The
includeNestedOnlySlots && isActiveUrlLevelnarrowing also correctly avoids retaining nested-only slots for inherited ancestor slots, which prevents over-discovery on deeper routes.
No blocking issues found. This is mergeable.
|
Posted the review on PR #2350. SummaryThe previous The fix gates the guard behind a new opt-in This achieves exactly the "layout-only parent" scoping the prior review requested:
I verified by running the routing suites the original PR validation skipped (per AGENTS.md):
No blocking issues found. Fixtures, e2e regression, and unit tests are all correct and reference the authoritative Next.js source. |
Summary
childrendefaultNext.js parity
Fixes the remaining failure from:
test/e2e/app-dir/parallel-routes-leaf-segments/parallel-routes-leaf-segments.no-build-error.test.tsThe original requested
parallel-routes-use-selected-layout-segmentfailures are already covered by #2246. PR #2347 covers the separateparallel-routes-layoutssuite.Reference: https://github.com/vercel/next.js/blob/v16.2.6/test/e2e/app-dir/parallel-routes-leaf-segments/parallel-routes-leaf-segments.no-build-error.test.ts
Validation
vp test run tests/app-route-graph.test.ts— 58 passedvp check— passedvp run vinext#build— passed