Skip to content

fix: use closest ancestor parallel slot instead of farthest#545

Merged
james-elicx merged 1 commit intocloudflare:mainfrom
NathanDrake2406:fix/parallel-slot-priority
Mar 15, 2026
Merged

fix: use closest ancestor parallel slot instead of farthest#545
james-elicx merged 1 commit intocloudflare:mainfrom
NathanDrake2406:fix/parallel-slot-priority

Conversation

@NathanDrake2406
Copy link
Copy Markdown
Contributor

Summary

  • discoverInheritedParallelSlots iterated root-to-leaf but used if (!slotMap.has(slot.name)) which made the farthest ancestor's slot win instead of the closest
  • For example, with @sidebar at both app/ and app/dashboard/, the route /dashboard/settings would incorrectly get the root-level sidebar
  • Removed the has() guard so slotMap.set() is called unconditionally for inherited slots — since iteration goes root-to-leaf, later (closer) ancestors naturally overwrite earlier (farther) ones
  • One-line fix, consistent with the isOwnDir branch which already used unconditional set()

Test plan

  • New test with fixture: same @sidebar at root and dashboard levels, verifies /dashboard/settings gets the dashboard-level slot
  • All 128 routing tests pass (91 routing + 37 route-sorting)
  • Typecheck and lint clean

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented Mar 15, 2026

Open in StackBlitz

npm i https://pkg.pr.new/vinext@545

commit: 22d3c03

discoverInheritedParallelSlots iterates from root to leaf, but used
a `has()` guard that let the first (farthest) ancestor win. Remove
the guard so later (closer) ancestors overwrite earlier ones.
@NathanDrake2406 NathanDrake2406 force-pushed the fix/parallel-slot-priority branch from d37b09c to 22d3c03 Compare March 15, 2026 06:21
@james-elicx
Copy link
Copy Markdown
Collaborator

/bigbonk review

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean, correct fix with a good test. The bug analysis is spot-on: the old if (!slotMap.has(slot.name)) guard caused the first (root-level) slot to win since iteration goes root-to-leaf, when the desired behavior is for the closest ancestor to win. Removing the guard lets later iterations naturally overwrite earlier ones, which is consistent with how the isOwnDir branch already works.

The test creates exactly the right scenario (same @sidebar at root and dashboard levels) and asserts the correct outcome for a leaf route under dashboard.

Comment thread tests/routing.test.ts
expect(sidebarSlot).toBeDefined();

// The dashboard-level @sidebar (closest ancestor) should win
expect(sidebarSlot!.defaultPath).not.toBeNull();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: You could also assert that sidebarSlot!.layoutIndex matches the dashboard layout (index 1) rather than the root layout (index 0). That would make the test even more precise about which slot was selected, since the fix also ensures layoutIndex is correct. Not blocking — the defaultPath assertion is sufficient to prove the right slot won.

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented Mar 15, 2026

Approved the PR. Here's a summary of the review:

Verdict: Approved

The fix is correct and minimal. The old if (!slotMap.has(slot.name)) guard was inverted in effect — since discoverInheritedParallelSlots iterates root-to-leaf, the guard caused the farthest ancestor's slot to stick (first write wins), when the intended behavior is for the closest ancestor to win. Removing the guard lets later (closer) iterations overwrite earlier ones via slotMap.set(), which is also consistent with the isOwnDir branch that already used unconditional set().

The test is well-constructed: it sets up the exact scenario (same @sidebar at both root and dashboard/ levels) and verifies that /dashboard/settings gets the dashboard-level slot. I left one minor suggestion about additionally asserting layoutIndex for extra precision, but it's non-blocking.

github run

@james-elicx james-elicx merged commit 6723edb into cloudflare:main Mar 15, 2026
19 checks passed
@NathanDrake2406 NathanDrake2406 deleted the fix/parallel-slot-priority branch March 18, 2026 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants