fix: Close nested Menus when hovering over other items - #4090
Open
mannycarrera4 wants to merge 16 commits into
Open
fix: Close nested Menus when hovering over other items#4090mannycarrera4 wants to merge 16 commits into
mannycarrera4 wants to merge 16 commits into
Conversation
mannycarrera4
added
the
ai author
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe menu now closes stale sibling submenus, ignores disabled targets and items, restores focus after reopening, applies default list sizing and nested container styling, and adds nested-menu stories and tests. ChangesMenu interaction behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
mannycarrera4
force-pushed
the
mc-fix-nested-menu
branch
from
August 3, 2026 20:14
4bb117a to
34a699d
Compare
mannycarrera4
marked this pull request as ready for review
August 3, 2026 20:14
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/react/menu/lib/Submenu.tsx`:
- Around line 94-98: Update the target ID resolution in Submenu’s
sibling-closing logic to fall back to the ID captured from the target event when
model.state.targetRef.current is unavailable. Keep the existing parentCursorId
comparison and model.events.hide() behavior, and add coverage using a target
component that does not forward its ref.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d15f24a6-71bf-4f9b-b890-ad9aca92fc1a
📒 Files selected for processing (4)
cypress/component/Menu.spec.tsxmodules/react/menu/lib/Submenu.tsxmodules/react/menu/stories/Menu.stories.tsmodules/react/menu/stories/examples/NestedSiblings.tsx
Comment on lines
+94
to
+98
| const targetId = (model.state.targetRef.current as HTMLElement | null)?.getAttribute( | ||
| 'data-id' | ||
| ); | ||
| if (targetId && parentCursorId && parentCursorId !== targetId) { | ||
| model.events.hide(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not require a forwarded ref to close a sibling submenu.
Line 94 derives targetId only from targetRef. A target component without ref forwarding can open on hover, but its targetRef remains unset. When the parent cursor moves to a sibling, this condition does not call hide().
Use the ID captured from the target event as a fallback. Add coverage with a non-ref-forwarding target component.
Proposed fix
- const targetId = (model.state.targetRef.current as HTMLElement | null)?.getAttribute(
- 'data-id'
- );
+ const targetId =
+ currentTargetIdRef.current ||
+ (model.state.targetRef.current as HTMLElement | null)?.getAttribute('data-id');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const targetId = (model.state.targetRef.current as HTMLElement | null)?.getAttribute( | |
| 'data-id' | |
| ); | |
| if (targetId && parentCursorId && parentCursorId !== targetId) { | |
| model.events.hide(); | |
| const targetId = | |
| currentTargetIdRef.current || | |
| (model.state.targetRef.current as HTMLElement | null)?.getAttribute('data-id'); | |
| if (targetId && parentCursorId && parentCursorId !== targetId) { | |
| model.events.hide(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/react/menu/lib/Submenu.tsx` around lines 94 - 98, Update the target
ID resolution in Submenu’s sibling-closing logic to fall back to the ID captured
from the target event when model.state.targetRef.current is unavailable. Keep
the existing parentCursorId comparison and model.events.hide() behavior, and add
coverage using a target component that does not forward its ref.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/react/menu/spec/MenuDisabled.spec.tsx`:
- Around line 102-126: Extend the sibling submenu test around NestedSiblings to
cover opening the second submenu via hover, using the project’s established
hover event helper or event sequence instead of only fireEvent.click(thirdItem).
Assert that the previously opened Second submenu closes, Second Item becomes
aria-expanded="false", Third Item’s submenu appears, and the menu count remains
unchanged.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f89c24cc-b4db-42a6-921a-6b19f42c5dec
⛔ Files ignored due to path filters (1)
.cursor/debug-dea6ae.logis excluded by!**/*.log
📒 Files selected for processing (5)
modules/react/collection/lib/useListItemSelect.tsxmodules/react/menu/lib/MenuItem.tsxmodules/react/menu/lib/MenuList.tsxmodules/react/menu/lib/Submenu.tsxmodules/react/menu/spec/MenuDisabled.spec.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- modules/react/menu/lib/Submenu.tsx
Comment on lines
+102
to
+126
| it('should close sibling submenu when another is opened by click', async () => { | ||
| render(<NestedSiblings />); | ||
|
|
||
| fireEvent.click(screen.getByRole('button', {name: 'Open Menu'})); | ||
| await screen.findByRole('menu'); | ||
|
|
||
| const secondItem = screen.getByRole('menuitem', {name: 'Second Item'}); | ||
| const thirdItem = screen.getByRole('menuitem', {name: 'Third Item'}); | ||
|
|
||
| fireEvent.click(secondItem); | ||
| await waitFor(() => { | ||
| expect(screen.getByRole('menuitem', {name: 'Second: First Sub Item'})).toBeInTheDocument(); | ||
| }); | ||
| expect(secondItem).toHaveAttribute('aria-expanded', 'true'); | ||
|
|
||
| fireEvent.click(thirdItem); | ||
| await waitFor(() => { | ||
| expect( | ||
| screen.queryByRole('menuitem', {name: 'Second: First Sub Item'}) | ||
| ).not.toBeInTheDocument(); | ||
| }); | ||
| expect(secondItem).toHaveAttribute('aria-expanded', 'false'); | ||
| expect(screen.getByRole('menuitem', {name: 'Third: First Sub Item'})).toBeInTheDocument(); | ||
| expect(screen.getAllByRole('menu')).toHaveLength(2); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Cover the hover interaction.
This test opens the sibling submenu with fireEvent.click(thirdItem). The stated regression occurs when the user hovers over another submenu target. A click-only test can pass while the hover handler leaves the first submenu open.
Proposed test update
- it('should close sibling submenu when another is opened by click', async () => {
+ it('should close sibling submenu when another is opened by hover', async () => {
...
- fireEvent.click(thirdItem);
+ fireEvent.mouseEnter(thirdItem);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('should close sibling submenu when another is opened by click', async () => { | |
| render(<NestedSiblings />); | |
| fireEvent.click(screen.getByRole('button', {name: 'Open Menu'})); | |
| await screen.findByRole('menu'); | |
| const secondItem = screen.getByRole('menuitem', {name: 'Second Item'}); | |
| const thirdItem = screen.getByRole('menuitem', {name: 'Third Item'}); | |
| fireEvent.click(secondItem); | |
| await waitFor(() => { | |
| expect(screen.getByRole('menuitem', {name: 'Second: First Sub Item'})).toBeInTheDocument(); | |
| }); | |
| expect(secondItem).toHaveAttribute('aria-expanded', 'true'); | |
| fireEvent.click(thirdItem); | |
| await waitFor(() => { | |
| expect( | |
| screen.queryByRole('menuitem', {name: 'Second: First Sub Item'}) | |
| ).not.toBeInTheDocument(); | |
| }); | |
| expect(secondItem).toHaveAttribute('aria-expanded', 'false'); | |
| expect(screen.getByRole('menuitem', {name: 'Third: First Sub Item'})).toBeInTheDocument(); | |
| expect(screen.getAllByRole('menu')).toHaveLength(2); | |
| }); | |
| it('should close sibling submenu when another is opened by hover', async () => { | |
| render(<NestedSiblings />); | |
| fireEvent.click(screen.getByRole('button', {name: 'Open Menu'})); | |
| await screen.findByRole('menu'); | |
| const secondItem = screen.getByRole('menuitem', {name: 'Second Item'}); | |
| const thirdItem = screen.getByRole('menuitem', {name: 'Third Item'}); | |
| fireEvent.click(secondItem); | |
| await waitFor(() => { | |
| expect(screen.getByRole('menuitem', {name: 'Second: First Sub Item'})).toBeInTheDocument(); | |
| }); | |
| expect(secondItem).toHaveAttribute('aria-expanded', 'true'); | |
| fireEvent.mouseEnter(thirdItem); | |
| await waitFor(() => { | |
| expect( | |
| screen.queryByRole('menuitem', {name: 'Second: First Sub Item'}) | |
| ).not.toBeInTheDocument(); | |
| }); | |
| expect(secondItem).toHaveAttribute('aria-expanded', 'false'); | |
| expect(screen.getByRole('menuitem', {name: 'Third: First Sub Item'})).toBeInTheDocument(); | |
| expect(screen.getAllByRole('menu')).toHaveLength(2); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/react/menu/spec/MenuDisabled.spec.tsx` around lines 102 - 126, Extend
the sibling submenu test around NestedSiblings to cover opening the second
submenu via hover, using the project’s established hover event helper or event
sequence instead of only fireEvent.click(thirdItem). Assert that the previously
opened Second submenu closes, Second Item becomes aria-expanded="false", Third
Item’s submenu appears, and the menu count remains unchanged.
Workday/canvas-kit
 |
||||||||||||||||||||||||||||||||||||||||
| Project |
Workday/canvas-kit
|
| Branch Review |
mc-fix-nested-menu
|
| Run status |
|
| Run duration | 02m 31s |
| Commit |
|
| Committer | Manuel Carrera |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
 |
0
|
 |
0
|
 |
17
|
 |
0
|
 |
824
|
| View all changes introduced in this branch ↗︎ | |
UI Coverage
19.34%
|
|
|---|---|
|
|
1570
|
|
|
374
|
Accessibility
99.57%
|
|
|---|---|
|
|
5 critical
5 serious
0 moderate
2 minor
|
|
|
66
|
…y/canvas-kit into mc-fix-nested-menu
A prior commit accidentally left in fetch() calls to a local debug ingest server on every menu click/render, plus a stray debug log file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…/canvas-kit into mc-fix-nested-menu
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)
cypress/component/Menu.spec.tsx (1)
235-240: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd accessibility checks for each mounted story.
Both new story contexts mount an example without
cy.checkA11y(). Add an accessibility test in each context.
cypress/component/Menu.spec.tsx#L235-L240: Addcy.checkA11y()forNestedSiblings.cypress/component/Menu.spec.tsx#L328-L333: Addcy.checkA11y()forNestedDynamic.As per coding guidelines, Cypress component tests must “include
cy.checkA11y()for every mounted example.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cypress/component/Menu.spec.tsx` around lines 235 - 240, Add cy.checkA11y() to both mounted story contexts: cypress/component/Menu.spec.tsx lines 235-240 for NestedSiblings and lines 328-333 for NestedDynamic, ensuring each context’s beforeEach accessibility-checks its mounted example.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cypress/component/Menu.spec.tsx`:
- Around line 235-240: Add cy.checkA11y() to both mounted story contexts:
cypress/component/Menu.spec.tsx lines 235-240 for NestedSiblings and lines
328-333 for NestedDynamic, ensuring each context’s beforeEach
accessibility-checks its mounted example.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7951fd41-250f-43f8-a3cf-df70151812bb
📒 Files selected for processing (6)
cypress/component/Menu.spec.tsxmodules/react/collection/lib/useListItemSelect.tsxmodules/react/menu/lib/MenuItem.tsxmodules/react/menu/lib/MenuList.tsxmodules/react/menu/lib/Submenu.tsxmodules/react/menu/spec/MenuDisabled.spec.tsx
💤 Files with no reviewable changes (3)
- modules/react/collection/lib/useListItemSelect.tsx
- modules/react/menu/lib/MenuList.tsx
- modules/react/menu/lib/Submenu.tsx
mannycarrera4
commented
Aug 10, 2026
| boxShadow: system.depth[3], | ||
| minWidth, | ||
| maxHeight, | ||
| maxHeight: cssVar(maxHeight, '60vh'), |
Contributor
Author
There was a problem hiding this comment.
add a default value here
RayRedGoose
reviewed
Aug 10, 2026
Comment on lines
+12
to
+18
| if (!element) { | ||
| return false; | ||
| } | ||
| return ( | ||
| element.getAttribute('aria-disabled') === 'true' || | ||
| (element as HTMLButtonElement | HTMLInputElement).disabled === true | ||
| ); |
Contributor
There was a problem hiding this comment.
Suggested change
| if (!element) { | |
| return false; | |
| } | |
| return ( | |
| element.getAttribute('aria-disabled') === 'true' || | |
| (element as HTMLButtonElement | HTMLInputElement).disabled === true | |
| ); | |
| return ( | |
| Boolean(element?.getAttribute('aria-disabled')) || | |
| (element as HTMLButtonElement | HTMLInputElement)?.disabled | |
| ); |
Contributor
There was a problem hiding this comment.
But honestly, I would change that function to be isElementActive with the next return:
return (
element &&
!Boolean(element?.getAttribute('aria-disabled')) &&
!(element as HTMLButtonElement | HTMLInputElement)?.disabled
);
it would be more precise checking and you will not need to have many returns in other functions
RayRedGoose
reviewed
Aug 10, 2026
Comment on lines
+27
to
+31
| if (isElementDisabled(event.currentTarget) || state.nonInteractiveIds.includes(name)) { | ||
| return null; | ||
| } | ||
| events.select({id: name}); | ||
| return undefined; |
Contributor
There was a problem hiding this comment.
Do you need anything to be returned here?
RayRedGoose
reviewed
Aug 10, 2026
Comment on lines
+119
to
123
| if (isElementDisabled(event.currentTarget)) { | ||
| return; | ||
| } | ||
| currentTargetIdRef.current = event.currentTarget.getAttribute('data-id')!; | ||
| mouseEnterTimer.start(); |
Contributor
There was a problem hiding this comment.
why do not check "If element is not disabled" and do not have return at all?
added 2 commits
August 10, 2026 13:40
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
4 participants
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
Fixes: #3333, #4119, #4118, #4117
Release Category
Components
Checklist
ready for reviewhas been added to PRFor the Reviewer
Where Should the Reviewer Start?
Areas for Feedback? (optional)
Testing Manually
Screenshots or GIFs (if applicable)
Thank You Gif (optional)
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Style
Tests