fix: sidebar collapse/expand toggle not working after first interaction#72
Merged
fix: sidebar collapse/expand toggle not working after first interaction#72
Conversation
…nteraction
Replace Base-UI Collapsible with state-driven conditional rendering in
LayoutMenu. The Collapsible.Trigger render prop chain through
SidebarMenuButton's Tooltip wrapper broke event handling and animation
state tracking after the first expand cycle.
- Use handleClick + setOpen for direct toggle control
- Conditionally render SidebarMenuSub with {open && ...}
- Preserve accessibility via aria-expanded and aria-controls with useId
- Remove @base-ui/react/collapsible dependency from layout-menu
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughReplaced Collapsible primitives with local React Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/wordpress/layout-menu.tsx`:
- Around line 446-459: The parent menu items with children currently drop
item.href and item.onClick making expandable rows disclosure-only; update the
JSX where SidebarMenuButton is rendered (the instance that sets tooltip,
isActive, onClick={handleClick}, aria-expanded={open},
aria-controls={submenuId}) to preserve and prefer item.href and item.onClick
when present (e.g., pass href={item.href} and onClick={item.onClick ??
handleClick} or call item.onClick inside handleClick), so parent
LayoutMenuItemData with children still performs its navigation/callback; apply
the same change to the other identical block referenced (the block around lines
582-594) to keep item-level actions consistent.
- Around line 582-594: The nested disabled parent is still interactive because
the rendered button in SidebarMenuSubButton ignores item.disabled; update the
render/button and click handling so disabled items cannot be focused or
expanded: pass disabled and aria-disabled attributes into the rendered <button>
(e.g. disabled={item.disabled} aria-disabled={item.disabled}), set
tabIndex={item.disabled ? -1 : undefined}, and add a guard in the onClick
handler (handleClick) to immediately return when item.disabled is true; also
ensure any classNames (menuItemClassName/activeItemClassName/item.className)
reflect a disabled style when item.disabled so pointer interactions are blocked
visually/semantically.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b3b5fc1d-ab8d-417c-91da-90eaf7ada25f
📒 Files selected for processing (1)
src/components/wordpress/layout-menu.tsx
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/wordpress/layout-menu.tsx (1)
549-576:⚠️ Potential issue | 🟡 MinorLeaf sub-items don't respect
item.disabled.The button element lacks
disabled={item.disabled}, so disabled leaf sub-items remain interactive. This is inconsistent with the nested parent sub-item (line 585) which does setdisabled.🛡️ Proposed fix
<SidebarMenuSubButton render={ comp === "a" ? <a href={item.href} /> - : <button type="button" /> + : <button type="button" disabled={item.disabled} /> } isActive={isActive} - onClick={handleClick} + onClick={item.disabled ? undefined : handleClick} className={cn( menuItemClassName, isActive && activeItemClassName, item.className, )} data-active={isActive || undefined} + aria-disabled={item.disabled || undefined} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/wordpress/layout-menu.tsx` around lines 549 - 576, Leaf sub-items aren't using item.disabled so they remain interactive; update the leaf branch in the render where SidebarMenuSubButton is returned to pass the disabled prop from item (i.e., add disabled={item.disabled}) and ensure click handling respects it (e.g., avoid invoking handleClick when item.disabled is true) for the element created via comp (anchor or button) inside SidebarMenuSubButton; reference SidebarMenuSubButton, SidebarMenuSubItem, comp, item.disabled, and handleClick to locate and fix the code.
♻️ Duplicate comments (1)
src/components/wordpress/layout-menu.tsx (1)
581-598:⚠️ Potential issue | 🟡 MinorDisabled handling is incomplete for nested parent sub-items.
The button case correctly sets
disabled={item.disabled}, but two issues remain:
- Anchor case: When
item.hrefexists, anchors don't supportdisablednatively—clicks will still navigate.- onClick handler:
handleClickfires regardless ofitem.disabled, so even if the button is disabled, callbacks may still execute on some browsers/assistive tech.🛡️ Proposed fix
<SidebarMenuSubButton render={ item.href - ? <a href={item.href} /> + ? <a href={item.disabled ? undefined : item.href} aria-disabled={item.disabled || undefined} /> : <button type="button" disabled={item.disabled} /> } isActive={isActive} - onClick={handleClick} + onClick={item.disabled ? undefined : handleClick} className={cn( menuItemClassName, isActive && activeItemClassName, item.className, + item.disabled && "pointer-events-none opacity-50", )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/wordpress/layout-menu.tsx` around lines 581 - 598, The nested parent sub-item handling must prevent interaction when item.disabled is true: update the render passed to SidebarMenuSubButton so the anchor branch also respects disabled by adding aria-disabled="true", tabIndex={-1} (or equivalent), role="button", and an onClick that calls event.preventDefault() when item.disabled; additionally wrap or guard the handleClick passed to onClick so it returns early if item.disabled is true (or pass a noop) to ensure clicks never run the callback for disabled items; reference SidebarMenuSubButton, item.href, item.disabled, and handleClick when making the changes.
🧹 Nitpick comments (1)
src/components/wordpress/layout-menu.tsx (1)
533-539: Consider guarding callbacks against disabled state.For consistency and defense-in-depth, consider adding an early return when
item.disabledis true, even though the button/anchor should prevent the click.♻️ Optional defensive fix
const handleClick = useCallback(() => { + if (item.disabled) return; if (hasChildren) { setOpen((o) => !o); } item.onClick?.(); onItemClick?.(item); - }, [hasChildren, item, onItemClick]); + }, [hasChildren, item, item.disabled, onItemClick]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/wordpress/layout-menu.tsx` around lines 533 - 539, Add a defensive early return in the handleClick callback to no-op when item.disabled is true: at the top of handleClick (the useCallback that toggles setOpen for hasChildren and calls item.onClick and onItemClick), check if (item.disabled) return; and update the useCallback dependency list to include item.disabled (or item.disabled via item) so the closure stays current; this prevents any accidental activation when the item is disabled while preserving the existing behavior for hasChildren, setOpen, item.onClick, and onItemClick.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/components/wordpress/layout-menu.tsx`:
- Around line 549-576: Leaf sub-items aren't using item.disabled so they remain
interactive; update the leaf branch in the render where SidebarMenuSubButton is
returned to pass the disabled prop from item (i.e., add
disabled={item.disabled}) and ensure click handling respects it (e.g., avoid
invoking handleClick when item.disabled is true) for the element created via
comp (anchor or button) inside SidebarMenuSubButton; reference
SidebarMenuSubButton, SidebarMenuSubItem, comp, item.disabled, and handleClick
to locate and fix the code.
---
Duplicate comments:
In `@src/components/wordpress/layout-menu.tsx`:
- Around line 581-598: The nested parent sub-item handling must prevent
interaction when item.disabled is true: update the render passed to
SidebarMenuSubButton so the anchor branch also respects disabled by adding
aria-disabled="true", tabIndex={-1} (or equivalent), role="button", and an
onClick that calls event.preventDefault() when item.disabled; additionally wrap
or guard the handleClick passed to onClick so it returns early if item.disabled
is true (or pass a noop) to ensure clicks never run the callback for disabled
items; reference SidebarMenuSubButton, item.href, item.disabled, and handleClick
when making the changes.
---
Nitpick comments:
In `@src/components/wordpress/layout-menu.tsx`:
- Around line 533-539: Add a defensive early return in the handleClick callback
to no-op when item.disabled is true: at the top of handleClick (the useCallback
that toggles setOpen for hasChildren and calls item.onClick and onItemClick),
check if (item.disabled) return; and update the useCallback dependency list to
include item.disabled (or item.disabled via item) so the closure stays current;
this prevents any accidental activation when the item is disabled while
preserving the existing behavior for hasChildren, setOpen, item.onClick, and
onItemClick.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e768b5dd-ee48-4f31-a4ea-c126f1e94059
📒 Files selected for processing (1)
src/components/wordpress/layout-menu.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Collapsiblecomponent with React state-driven conditional rendering inLayoutMenuCollapsible.Triggerrender prop chain throughSidebarMenuButton's Tooltip wrapper broke event handling and animation state tracking after the first expand cyclearia-expandedandaria-controlsattributes usinguseId()Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Accessibility Improvements
New Features
Bug Fixes
UI Improvements