fix: make full menu item area clickable

[alukach]

Fixes the bug where clicking the menu item area outside the link text would close the menu without navigating.

Used Radix UI's asChild prop on DropdownMenu.Item so the Link component fills the full item area.

Closes #290

Generated with Claude Code

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
source-cooperative	Ready	Preview, Comment	Apr 20, 2026 9:29pm

[claude]

Claude finished @alukach's task in 51s —— View job

---

PR Review

• Gather context
• Review changed files
• Provide feedback

---

The fix is correct and clean. Using asChild is the idiomatic Radix UI pattern for this — it merges the DropdownMenu.Item behavior onto the Link element so the full item area is clickable. This is the right approach.

What changed:

• Items with href: now use <DropdownMenu.Item asChild><Link ...></DropdownMenu.Item> instead of nesting <Link> inside the item
• Items with onClick: now attach the handler directly to DropdownMenu.Item instead of wrapping in a <div> with manual cursor styling

Improvements over the old code:

• Removes the <div onClick={...} style={{ cursor: ... }}> wrapper — Radix handles cursor styling natively
• The Link element now is the item rather than being a child inside it, which is semantically cleaner

One pre-existing issue worth noting (not introduced by this PR): the condition field on DropdownItem (src/components/layout/DropdownSection.tsx:12) is checked in the early-return guard (lines 29–31) to count visible items, but items with condition: false are never filtered out in the items.map(...) at line 39. They'll render silently as visible items. This is out of scope here but worth a follow-up. Fix this →

Overall: the fix is minimal, correct, and an improvement. No blockers. ✓

[alukach]

@claude When testing this by opening the dropdown, I get: Uncaught Error: React.Children.only expected to receive a single React element child. We need a functional test to validate this and a fix

[claude]

Claude finished @alukach's task in 4m 44s —— View job

---

Fixed asChild Error + Added Tests

Root cause: DropdownMenu.Item from @radix-ui/themes spreads all props (including asChild) onto the underlying @radix-ui/react-dropdown-menu primitive. That primitive calls React.Children.only — but the themes component always renders multiple children internally (indicator span + content + optional shortcut). Hence the crash.

Fix: Removed asChild + <Link>. Instead, useRouter().push() is called in onClick, which makes the entire item area clickable without touching Radix internals.

• Investigate the React.Children.only error
• Fix the bug — replaced asChild/Link with router.push in onClick
• Add functional tests (src/components/layout/__tests__/DropdownSection.test.tsx)
• Commit and push

Tests added cover:

• Renders without throwing (regression guard for this specific error)
• router.push called with correct href on click
• onClick handler called for non-href items
• Disabled items do not trigger navigation
• Conditional rendering (condition prop, per-item condition)
• Label rendering

Trade-off: router.push doesn't preserve native <a> semantics (right-click → "Open in new tab"). For a dropdown menu this is generally acceptable, but worth noting.
| Branch

[claude]

Claude finished @alukach's task in 1m 11s —— View job

---

PR Review

• Gather context
• Review changed files
• Provide feedback

---

The fix is solid. The approach — onSelect={() => router.push(href)} for keyboard/click activation, with a <Link> child styled to fill the full item area — correctly handles both the original bug (full area clickable) and right-click / middle-click "open in new tab" support. This is a good dual-mechanism pattern.

Verdict: no blockers. One pre-existing bug remains unfixed, one minor pattern note.

---

Minor: potential redundant navigation on click

In DropdownSection.tsx:62 (and similarly in AccountSelector.tsx:51, ProductSelector.tsx:49), clicking the <Link> child fires:

1. Next.js Link navigation (via the <a> tag)
2. onSelect → router.push(href)

Both navigate to the same URL, so no visible breakage, but the double-push is redundant. If this causes any edge-case issues (scroll-position, history stack), it could be addressed by using e.preventDefault() on the anchor or by relying solely on onSelect. Not a blocker.

---

Pre-existing bug: item.condition not respected in render

Noted in the prior review and still unfixed — items.map() at src/components/layout/DropdownSection.tsx:56 does not skip items where item.condition === false. The early-return guard at line 46–51 only prevents rendering when all items are hidden; individual conditional items still render when mixed with visible ones.

Example: a section with one condition: true item and one condition: false item will render both. Fix this →

---

Tests: good coverage

The new test file covers the key paths well — rendering, label, routing, disabled, and both condition variants. One gap: no test for the mixed-condition case (some items condition: false, some condition: true), which would catch the bug above.

---

| Branch

[claude]

Claude finished @alukach's task in 1m 17s —— View job

---

PR Review

• Gather context
• Review changed files
• Provide feedback

---

The approach is correct and the implementation is clean. The dual-mechanism pattern — onSelect for keyboard/Enter activation + <Link> child styled to fill the full item area — handles all the use cases: full area clickable, right-click → "Open in new tab", and keyboard accessibility.

What changed:

• DropdownSection, AccountSelector, ProductSelector: all now use onSelect={() => router.push(href)} + a styled <Link> child that stretches to fill the item
• globals.css: added .rt-BaseMenuItem a override to prevent the global a:hover { color: white !important } rule from turning link text white inside highlighted menu items
• Tests: new file covering render, routing, disabled, conditional rendering, and link presence

Verdict: no blockers. One unfixed pre-existing bug, one minor note.

---

Minor: double-push on click (not a blocker)

In DropdownSection.tsx:62 (and AccountSelector.tsx:51, ProductSelector.tsx:50), a left-click fires:

1. Next.js <Link> navigation (the <a> tag)
2. onSelect → router.push(href)

Both navigate to the same URL. This is harmless today, but it pushes a duplicate entry onto the history stack and could cause subtle issues if a route triggers side-effects on navigation. It could be addressed by calling e.preventDefault() on the <Link>'s click, or dropping the <Link> and relying solely on onSelect (losing right-click support). Neither is urgent. Fix double-push →

---

Pre-existing bug: item.condition not respected in render

items.map() at src/components/layout/DropdownSection.tsx:56 does not skip items where item.condition === false. The early-return guard at line 46–50 only hides the entire section when all items are hidden; items with condition: false mixed with visible items still render.

The test at line 130 ("renders nothing when all items have condition false") passes only because all items are hidden — it doesn't catch the mixed case. Fix this →

---

CSS override: correct approach

The .rt-BaseMenuItem a, .rt-BaseMenuItem a:hover { color: inherit !important } rule in globals.css is necessary and correct — it beats the pre-existing a:hover { color: white !important } rule that would otherwise render link text white on a tinted/highlighted row. Using !important here is appropriate since it's overriding another !important.

---

| Branch

[alukach]

@claude Review this PR

[claude]

Claude finished @alukach's task in 1m 25s —— View job

---

PR Review

• Gather context
• Read changed files
• Provide feedback

---

The dual-mechanism approach — onSelect for keyboard/click activation + a styled <Link> child that fills the item area — is a pragmatic and working solution. The CSS override in globals.css is correct and necessary. Tests cover the main paths. Two issues warrant attention before merge.

---

Bug: item.condition not respected during render

DropdownSection.tsx:56 — items.map() renders all items, including those with item.condition === false.

The early-return guard at lines 46–50 only prevents rendering the entire section when every item fails its condition. In a mixed case — e.g. two items, one with condition: true and one with condition: false — the guard passes (because at least one item is visible), but then both items are rendered.

// line 56 — needs a filter before map
{items.map((item, index) =>  // ← items with condition:false still render

Fix: add .filter(({ condition = true }) => condition) before .map():

{items.filter(({ condition = true }) => condition).map((item, index) =>

The test at line 130 ("renders nothing when all items have condition false") does not catch this — it passes because all items are false, so the early-return fires. A mixed-condition test would expose it. Fix this →

---

Issue: double history push on left-click

DropdownSection.tsx:62–64 (and AccountSelector.tsx:51–53, ProductSelector.tsx:49–52) — left-clicking the <Link> fires two navigations:

1. Native <a> navigation (from <Link>)
2. onSelect → router.push(href)

Both go to the same URL, so there's no visible breakage, but the browser history stack gets a duplicate entry. This means the user needs to hit Back twice to return to the previous page after clicking a dropdown item.

Fix: add onClick={e => e.preventDefault()} to the <Link> so left-click is handled solely by onSelect, while right-click and middle-click still use the native anchor:

<DropdownMenu.Item onSelect={() => router.push(item.href!)}>
  <Link href={item.href} style={dropdownMenuLinkStyle} onClick={e => e.preventDefault()}>
    {item.children}
  </Link>
</DropdownMenu.Item>

Fix double-push →

---

Positive notes

• dropdownMenuLinkStyle using negative margins + matching padding to stretch the clickable area is a solid, recognized pattern.
• The .rt-BaseMenuItem a, .rt-BaseMenuItem a:hover { color: inherit !important } CSS override is correct — it beats the pre-existing a:hover { color: white !important } rule.
• The @radix-ui/themes mock in the test file correctly simulates onSelect on click, which accurately reflects real Radix behavior.
• The exported dropdownMenuLinkStyle constant is a good call — keeps the three components consistent without duplication.

---

Summary: Two issues to address — the condition filter bug (affects correctness) and the double history push (affects UX). The rest is clean.
| Branch