menu primitive tests #1276
menu primitive tests #1276
Conversation
WalkthroughThis pull request introduces test coverage for the Changes
Sequence Diagram(s)sequenceDiagram
participant TestSuite as MenuPrimitive.test.tsx
participant MenuRoot as MenuPrimitive.Root
participant Trigger as MenuPrimitive.Trigger
participant Item as MenuPrimitive.Item
participant Content as MenuPrimitive.Content
participant Sub as MenuPrimitive.Sub
participant Portal as MenuPrimitive.Portal
TestSuite->>MenuRoot: Render with props/context
TestSuite->>Trigger: Render and simulate click
Trigger->>MenuRoot: Open/close state update
TestSuite->>Item: Render and check props
TestSuite->>Content: Render based on open state
TestSuite->>Sub: Render nested submenu
TestSuite->>Portal: Render portal content when open
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (2)
src/components/ui/ContextMenu/stories/ContextMenu.stories.tsx (1)
13-13: React SVG attribute casingInside JSX, use
fillRule/clipRuleinstead offill-rule/clip-ruleto avoid React console warnings.src/components/ui/DropdownMenu/stories/DropdownMenu.stories.tsx (1)
13-15: Camel-case SVG attributesSame note as above – change
fill-rule/clip-ruletofillRule/clipRulein the two inline SVGs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/components/ui/ContextMenu/fragments/ContextMenuTrigger.tsx(1 hunks)src/components/ui/ContextMenu/stories/ContextMenu.stories.tsx(2 hunks)src/components/ui/DropdownMenu/stories/DropdownMenu.stories.tsx(1 hunks)src/core/primitives/Menu/contexts/MenuPrimitiveRootContext.tsx(1 hunks)src/core/primitives/Menu/fragments/MenuPrimitivePortal.tsx(1 hunks)src/core/primitives/Menu/fragments/MenuPrimitiveRoot.tsx(2 hunks)src/core/primitives/Menu/tests/MenuPrimitive.test.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
src/components/ui/DropdownMenu/stories/DropdownMenu.stories.tsx (2)
Learnt from: decipher-cs
PR: #417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the Dropdown.Trigger component in src/components/ui/Dropdown/Dropdown.stories.tsx.
Learnt from: decipher-cs
PR: #417
File: src/components/ui/Dropdown/Dropdown.tsx:0-0
Timestamp: 2024-12-12T08:22:59.375Z
Learning: The Dropdown.Trigger component is customizable and needs to be used with Dropdown.Root.
src/core/primitives/Menu/fragments/MenuPrimitiveRoot.tsx (1)
Learnt from: kotAPI
PR: #576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the TogglePrimitive component (src/core/primitives/Toggle/index.tsx), when the component becomes controlled, it's acceptable to not sync the internal isPressed state with the external pressed prop.
src/components/ui/ContextMenu/fragments/ContextMenuTrigger.tsx (3)
Learnt from: decipher-cs
PR: #417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the Dropdown.Trigger component in src/components/ui/Dropdown/Dropdown.stories.tsx.
Learnt from: GoldGroove06
PR: #1215
File: src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveTrigger.tsx:25-27
Timestamp: 2025-07-18T16:43:26.175Z
Learning: In the CheckboxGroupPrimitiveTrigger component (src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveTrigger.tsx), the component uses two separate useEffect hooks with different purposes: the first useEffect (lines 25-27) with empty dependency array [] is meant to set the initial state only once on mount by syncing with the group's checked values, while the second useEffect (lines 28-34) handles ongoing state updates by propagating local isChecked changes back to the group's checkedValues. This pattern prevents infinite loops while maintaining proper synchronization.
Learnt from: decipher-cs
PR: #417
File: src/components/ui/Dropdown/Dropdown.tsx:0-0
Timestamp: 2024-12-12T08:22:59.375Z
Learning: The Dropdown.Trigger component is customizable and needs to be used with Dropdown.Root.
src/core/primitives/Menu/fragments/MenuPrimitivePortal.tsx (2)
Learnt from: kotAPI
PR: #576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the TogglePrimitive component (src/core/primitives/Toggle/index.tsx), when the component becomes controlled, it's acceptable to not sync the internal isPressed state with the external pressed prop.
Learnt from: krau5
PR: #609
File: src/components/ui/Text/Text.tsx:19-22
Timestamp: 2024-12-06T14:30:05.834Z
Learning: When the keys and values for the as prop are the same, we should define the type directly using string literals (e.g., 'div' | 'span' | 'p' | 'label') and eliminate extra mappings like tagMap to simplify the code.
src/components/ui/ContextMenu/stories/ContextMenu.stories.tsx (1)
Learnt from: decipher-cs
PR: #417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the Dropdown.Trigger component in src/components/ui/Dropdown/Dropdown.stories.tsx.
src/core/primitives/Menu/tests/MenuPrimitive.test.tsx (3)
Learnt from: decipher-cs
PR: #417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the Dropdown.Trigger component in src/components/ui/Dropdown/Dropdown.stories.tsx.
Learnt from: kotAPI
PR: #576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the TogglePrimitive component (src/core/primitives/Toggle/index.tsx), when the component becomes controlled, it's acceptable to not sync the internal isPressed state with the external pressed prop.
Learnt from: GoldGroove06
PR: #1215
File: src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveTrigger.tsx:25-27
Timestamp: 2025-07-18T16:43:26.175Z
Learning: In the CheckboxGroupPrimitiveTrigger component (src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveTrigger.tsx), the component uses two separate useEffect hooks with different purposes: the first useEffect (lines 25-27) with empty dependency array [] is meant to set the initial state only once on mount by syncing with the group's checked values, while the second useEffect (lines 28-34) handles ongoing state updates by propagating local isChecked changes back to the group's checkedValues. This pattern prevents infinite loops while maintaining proper synchronization.
🪛 GitHub Check: lint
src/components/ui/ContextMenu/fragments/ContextMenuTrigger.tsx
[warning] 33-33:
Avoid non-native interactive elements. If using native HTML is not possible, add an appropriate role and support for tabbing, mouse, keyboard, and touch inputs to an interactive content element
[warning] 33-33:
Visible, non-interactive elements with click handlers must have at least one keyboard listener
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (12)
src/core/primitives/Menu/fragments/MenuPrimitiveRoot.tsx (2)
16-16: Whitespace-only change looks good
Parameter list now has consistent spacing – no functional impact.
45-46: Minor style cleanup acknowledged
Trailing-space removal insideFloater.offsetkeeps formatting consistent.src/components/ui/ContextMenu/fragments/ContextMenuTrigger.tsx (1)
21-29: Coordinate math may flip Y axis
y = rect.bottom - e.clientYreturns a positive value only when the click is below the element. If the click is inside the element the value will be negative, pushing the menu upward unexpectedly. Confirm this is intended; otherwise usee.clientY - rect.top.src/components/ui/ContextMenu/stories/ContextMenu.stories.tsx (1)
22-45: Whitespace cleanup approved
Prop spacing is now uniform. No functional impact.src/components/ui/DropdownMenu/stories/DropdownMenu.stories.tsx (1)
23-46: Formatting improvements acknowledged
Label-prop spacing is standardised – looks good.src/core/primitives/Menu/fragments/MenuPrimitivePortal.tsx (1)
11-11: Type-safety tweak is correctParenthesising the selector expression before the type assertion removes a subtle precedence foot-gun – nice catch.
src/core/primitives/Menu/tests/MenuPrimitive.test.tsx (6)
1-18: LGTM!Excellent test setup with proper imports, console.warn mocking, and cleanup patterns. The beforeEach/afterEach structure ensures clean test isolation.
20-29: LGTM!This test correctly validates the guard behavior for direct MenuPrimitive usage, ensuring proper warnings and null return.
31-74: LGTM!Comprehensive testing of MenuPrimitive.Root with good coverage of both controlled and uncontrolled states, custom styling, and positioning props.
76-172: LGTM!Excellent test coverage for Trigger and Item components, including context dependency validation, asChild prop support, and proper styling application. The pattern of testing both in-context and out-of-context behavior is particularly good.
174-310: LGTM!Great testing approach for Content, Sub, and Portal components. The Portal tests demonstrate excellent DOM API mocking practices, and the conditional rendering tests ensure proper UI state management.
312-388: LGTM!Excellent integration tests that validate complete menu functionality with realistic component composition. The interaction testing with
onOpenChangecallbacks ensures the component API works correctly in practice.
| <span ref={contextTriggerRef} onContextMenu={handleContextMenu} onClick={(e) => { e.preventDefault(); }}> | ||
| {children} | ||
| </span> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
<span> is now an interactive element – add accessibility hooks
Static-analysis correctly flags the span: it handles clicks/context-menu but is not keyboard-reachable and lacks an ARIA role. Add role="button", tabIndex={0}, and a onKeyDown handler so keyboard users can open the context menu.
- <span ref={contextTriggerRef} onContextMenu={handleContextMenu} onClick={(e) => { e.preventDefault(); }}>
+ <span
+ ref={contextTriggerRef}
+ role="button"
+ tabIndex={0}
+ onContextMenu={handleContextMenu}
+ onClick={(e) => e.preventDefault()}
+ onKeyDown={(e) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ e.preventDefault();
+ // Simulate right-click open at center of element
+ const rect = (e.currentTarget as HTMLElement).getBoundingClientRect();
+ setCoords({ x: rect.width / 2, y: rect.height / 2 });
+ setIsOpen(true);
+ }
+ }}
+ >📝 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.
| <span ref={contextTriggerRef} onContextMenu={handleContextMenu} onClick={(e) => { e.preventDefault(); }}> | |
| {children} | |
| </span> | |
| <span | |
| ref={contextTriggerRef} | |
| role="button" | |
| tabIndex={0} | |
| onContextMenu={handleContextMenu} | |
| onClick={(e) => e.preventDefault()} | |
| onKeyDown={(e) => { | |
| if (e.key === 'Enter' || e.key === ' ') { | |
| e.preventDefault(); | |
| // Simulate right-click open at center of element | |
| const rect = (e.currentTarget as HTMLElement).getBoundingClientRect(); | |
| setCoords({ x: rect.width / 2, y: rect.height / 2 }); | |
| setIsOpen(true); | |
| } | |
| }} | |
| > | |
| {children} | |
| </span> |
🧰 Tools
🪛 GitHub Check: lint
[warning] 33-33:
Avoid non-native interactive elements. If using native HTML is not possible, add an appropriate role and support for tabbing, mouse, keyboard, and touch inputs to an interactive content element
[warning] 33-33:
Visible, non-interactive elements with click handlers must have at least one keyboard listener
🤖 Prompt for AI Agents
In src/components/ui/ContextMenu/fragments/ContextMenuTrigger.tsx around lines
33 to 35, the <span> element is interactive but lacks accessibility features.
Add role="button" and tabIndex={0} to make it keyboard-focusable and assign an
onKeyDown handler that triggers the context menu on keyboard events like Enter
or Space to ensure keyboard users can open the context menu.
There was a problem hiding this comment.
can we replace span with native button element?
There was a problem hiding this comment.
@mrkazmi333 Yes, absolutely! Replacing the <span> with a native <button> element is a much better approach. A button provides built-in accessibility features and is semantically correct for interactive elements.
Here's the improved implementation:
return (
<MenuPrimitive.Trigger className={clsx(`${rootClass}-trigger`, className)} asChild={true} {...props}>
<button
ref={contextTriggerRef}
onContextMenu={handleContextMenu}
onClick={(e) => { e.preventDefault(); }}
type="button"
>
{children}
</button>
</MenuPrimitive.Trigger>
);Benefits of using a native <button>:
- Built-in accessibility: Automatically focusable, keyboard navigable (Enter/Space), and has the correct semantic role
- Resolves linter warnings: No need for manual
role,tabIndex, oronKeyDownhandlers - Semantic correctness: Buttons are the standard HTML element for interactive triggers
- Better UX: Users expect interactive elements to behave like buttons
The type="button" attribute prevents the button from submitting forms if the ContextMenu is used within a form context.
🧠 Learnings used
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.tsx:0-0
Timestamp: 2024-12-12T08:22:59.375Z
Learning: The `Dropdown.Trigger` component is customizable and needs to be used with `Dropdown.Root`.
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
Learnt from: GoldGroove06
PR: rad-ui/ui#1215
File: src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveTrigger.tsx:25-27
Timestamp: 2025-07-18T16:43:26.175Z
Learning: In the CheckboxGroupPrimitiveTrigger component (`src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveTrigger.tsx`), the component uses two separate useEffect hooks with different purposes: the first useEffect (lines 25-27) with empty dependency array `[]` is meant to set the initial state only once on mount by syncing with the group's checked values, while the second useEffect (lines 28-34) handles ongoing state updates by propagating local `isChecked` changes back to the group's `checkedValues`. This pattern prevents infinite loops while maintaining proper synchronization.
| setIsOpen: React.Dispatch<React.SetStateAction<boolean>>; | ||
| refs: { | ||
| reference: React.MutableRefObject<Element | null>; | ||
| reference: React.MutableRefObject<any>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using union types instead of any for better type safety.
The change from specific DOM element types to any removes valuable compile-time type checking. Consider using a union type that includes the expected element types while maintaining flexibility:
refs: {
- reference: React.MutableRefObject<any>;
+ reference: React.MutableRefObject<Element | HTMLElement | null>;
floating: React.MutableRefObject<HTMLElement | null>;
domReference: React.MutableRefObject<Element | null>;
- setReference(node: any): void;
+ setReference(node: Element | HTMLElement | null): void;
setFloating(node: HTMLElement | null): void;
setPositionReference(node: Element): void;
};This maintains type safety while supporting the broader usage patterns indicated by the test suite.
Also applies to: 12-12
🤖 Prompt for AI Agents
In src/core/primitives/Menu/contexts/MenuPrimitiveRootContext.tsx at lines 9 and
12, the type of the React.MutableRefObject is currently set to any, which
reduces type safety. Replace any with a union of the expected DOM element types
that the ref might hold, such as HTMLDivElement | null or other relevant element
types, to maintain type safety while allowing flexibility. This change will
improve compile-time checks without restricting usage.
|
|
||
| const context = React.useContext(ContextMenuContext); | ||
| if (!context) { | ||
| console.log('ContextMenuTrigger should be used in the ContextMenuRoot'); |
There was a problem hiding this comment.
can we remove console.log here
| }; | ||
|
|
||
| const ChevronRight =() => <svg width="15" height="15" viewBox="0 0 15 15" fill="none" xmlns="http://www.w3.org/2000/svg"><path d="M6.1584 3.13508C6.35985 2.94621 6.67627 2.95642 6.86514 3.15788L10.6151 7.15788C10.7954 7.3502 10.7954 7.64949 10.6151 7.84182L6.86514 11.8418C6.67627 12.0433 6.35985 12.0535 6.1584 11.8646C5.95694 11.6757 5.94673 11.3593 6.1356 11.1579L9.565 7.49985L6.1356 3.84182C5.94673 3.64036 5.95694 3.32394 6.1584 3.13508Z" fill="currentColor" fill-rule="evenodd" clip-rule="evenodd"></path></svg> | ||
| const ChevronRight = () => <svg width="15" height="15" viewBox="0 0 15 15" fill="none" xmlns="http://www.w3.org/2000/svg"><path d="M6.1584 3.13508C6.35985 2.94621 6.67627 2.95642 6.86514 3.15788L10.6151 7.15788C10.7954 7.3502 10.7954 7.64949 10.6151 7.84182L6.86514 11.8418C6.67627 12.0433 6.35985 12.0535 6.1584 11.8646C5.95694 11.6757 5.94673 11.3593 6.1356 11.1579L9.565 7.49985L6.1356 3.84182C5.94673 3.64036 5.95694 3.32394 6.1584 3.13508Z" fill="currentColor" fill-rule="evenodd" clip-rule="evenodd"></path></svg>; |
There was a problem hiding this comment.
we should not have these svg defined here, they should be in some separate files/folder
| setIsOpen: React.Dispatch<React.SetStateAction<boolean>>; | ||
| refs: { | ||
| reference: React.MutableRefObject<Element | null>; | ||
| reference: React.MutableRefObject<any>; |
There was a problem hiding this comment.
avoid using any type, lets use the dedicated type here
GoldGroove06
added
the
automerge
Summary by CodeRabbit
Style
Tests
Refactor