fix(app-router): apply slot override params to parallel slot segment map#790
Conversation
commit: |
There was a problem hiding this comment.
Pull request overview
This PR fixes a mismatch in the App Router’s parallel slot segment-map resolution by ensuring slot segment maps are resolved using the same “effective params” as slot rendering (slot override params when present; otherwise matched route params). It also adds regression tests to cover both override and non-override behaviors.
Changes:
- Use slot override params (when provided) to resolve parallel slot
segmentMapentries. - Preserve existing behavior by falling back to
matchedParamswhen no slot override exists. - Add unit regression coverage to validate slot segment resolution with and without overrides.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
packages/vinext/src/server/app-page-route-wiring.tsx |
Updates slot segment-map resolution to use override-effective params, aligning segment map behavior with slot rendering. |
tests/app-page-route-wiring.test.ts |
Adds tests asserting correct useSelectedLayoutSegments("sidebar") output for both override and non-override cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| continue; | ||
| } | ||
| const slotOverride = options.slotOverrides?.[slotName]; | ||
| const slotParams = slotOverride?.params ?? options.matchedParams; | ||
| if (slot.routeSegments) { |
There was a problem hiding this comment.
The slot override/params resolution logic is now duplicated (once when building slotElement props and again when building the segmentMap). Consider extracting a small helper (e.g. getEffectiveSlotParams(slotName)) or precomputing effective params per slot for the current layout index, so these two paths can’t drift over time.
|
/bigbonk review |
|
UnknownError: The operation timed out. |
|
@james-elicx Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean fix. The bug was that resolveAppPageChildSegments for parallel slots always used options.matchedParams (the parent route's params) instead of the slot override's params. This meant useSelectedLayoutSegments("slotName") would return unresolved dynamic segments (e.g. [id] instead of 42) when a slot override provided different params.
The fix correctly extracts a getEffectiveSlotParams helper — addressing the Copilot review comment about deduplication — and uses it in both the slot element rendering loop and the segment map construction loop. Tests cover both the override and fallback paths.
All 12 tests pass locally.
| @@ -181,6 +183,90 @@ describe("app page route wiring helpers", () => { | |||
| expect(html).toContain('data-segments="blog|post"'); | |||
| }); | |||
There was a problem hiding this comment.
Minor: now that RootLayout renders data-sidebar-segments, the existing test here could assert that attribute too (it would be empty since the sidebar slot has no routeSegments). Not blocking — just a completeness note.
| }); | |
| expect(html).toContain('data-segments="blog|post"'); | |
| expect(html).toContain('data-sidebar-segments=""'); | |
| }); |
|
Review submitted. Here's a summary of my assessment: Approved. The fix is correct and well-scoped:
|
Summary
slotOverride.paramswhen present, otherwise matched route params)app-page-route-wiringfor both override and non-override pathsTesting
vp check tests/app-page-route-wiring.test.ts packages/vinext/src/server/app-page-route-wiring.tsxvp test run tests/app-page-route-wiring.test.tsNotes
vp test run tests/app-router.test.ts -t "useSelectedLayoutSegments"is currently blocked in this worktree by an existing environment dependency error:Cannot find package 'magic-string'frompackages/vinext/src/plugins/instrumentation-client.ts.