fix(app-router): render parallel slots through route groups - #2022
Conversation
commit: |
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk review — no actionable findings
Reviewed the parallel-slot-through-route-groups fix. The change is correct, minimal, and well-targeted.
What the change does
packages/vinext/src/routing/app-route-graph.ts:1813 replaces the strict isOwnDir (dir === routeDir) check with:
const isActiveUrlLevel = dir === routeDir || segmentsBelow.every(isInvisibleSegment);This makes a slot use its own page.tsx (active branch) not only at the route's literal directory, but also when the only thing standing between the slot owner and the route's page directory is URL-transparent segments (route groups). That's exactly the upstream Next.js behavior: a slot owner at app/x/ should stay active when the route's page lives at app/x/(children)/page.tsx.
Correctness review
- Map keying is safe. Slots are keyed by
slot.key, which includes the slot dir's relative path (app-route-graph.ts:2083), so slots at different depths never clash; later levels can only override the same slot at the same depth. - Empty
segmentsBelowadds no new behavior.[].every(...)istrue, butsegmentsBelowis only empty at the finaldirsToCheckentry wheresegmentIndex === segments.length, and that entry'sdiris alwaysrouteDir. So the empty case is already covered by thedir === routeDirarm. @/.invisibility is harmless here.segmentsBelowis derived from the route's page directory, which doesn't contain parallel-slot (@) segments, so the route-group ((...)) case is the only one that can flipisActiveUrlLevel, matching the comment's intent.- Inherited branch is unaffected for routes with real visible segments below the slot owner (e.g.
/dashboard/settingswith a@teamslot atapp/dashboard) — those still fall through to the mirror/default.tsxpath.
Verification
tests/app-route-graph.test.ts— 56 passed (incl. new route-graph regression)tests/app-router-dev-server.test.ts— 166 passed (incl. new integration test)- scoped
vp checkon the modified file — clean (format, lint, types)
Minor (non-blocking) nits
- There's an existing helper
hasRemainingVisibleSegments(segments, startIndex)(app-route-graph.ts:2683) that is the logical inverse ofsegmentsBelow.every(isInvisibleSegment). Reusing!hasRemainingVisibleSegments(segments, segmentIndex)would keep the "what counts as visible below this level" logic in one place. Purely a consistency nit — current inline form is correct.
LGTM.
|
Review posted successfully on PR #2022. SummaryI reviewed PR #2022 ( The fix at const isActiveUrlLevel = dir === routeDir || segmentsBelow.every(isInvisibleSegment);Key correctness checks I verified:
One non-blocking nit: an existing helper LGTM. |
Summary
Next.js parity
Ported from:
test/e2e/app-dir/parallel-routes-group-depth/parallel-routes-group-depth.test.tsBefore the fix, the slot rendered an empty value instead of
Slot Page. The exact pinned Next.js v16.2.6 suite now passes.Validation
vp check