context menu #1275
context menu #1275
Conversation
WalkthroughThis change introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ContextMenu.Trigger
participant ContextMenu.Context
participant ContextMenu.Root
participant ContextMenu.Content
participant ContextMenu.Item
User->>ContextMenu.Trigger: Right-click event
ContextMenu.Trigger->>ContextMenu.Context: setCoords(x, y), setIsOpen(true)
ContextMenu.Context->>ContextMenu.Root: Coordinates and open state updated
ContextMenu.Root->>ContextMenu.Content: Render at new position if open
ContextMenu.Content->>ContextMenu.Item: Render menu items
User->>ContextMenu.Item: Clicks item
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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: 3
🧹 Nitpick comments (12)
src/core/primitives/Menu/fragments/MenuPrimitiveItem.tsx (1)
5-9: Consider updating type definition for enhanced flexibility.The type definition is quite restrictive and doesn't account for the additional props that can now be forwarded to the button element.
Consider updating the type to be more flexible:
-export type MenuPrimitiveItemProps = { - children: React.ReactNode - className?: string - label?: string -} +export type MenuPrimitiveItemProps = { + children: React.ReactNode + className?: string + label?: string +} & React.ButtonHTMLAttributes<HTMLButtonElement>This would provide better TypeScript support for all the additional props that can now be passed through.
src/core/primitives/Menu/fragments/MenuPrimitiveContent.tsx (1)
6-9: Consider enhancing type definition for better TypeScript support.Similar to
MenuPrimitiveItem, the type definition could be more comprehensive to support the additional props being forwarded.Consider updating the type definition:
-export type MenuPrimitiveContentProps = { - children: React.ReactNode; - className?: string; -}; +export type MenuPrimitiveContentProps = { + children: React.ReactNode; + className?: string; +} & React.HTMLAttributes<HTMLDivElement>;src/core/primitives/Menu/fragments/MenuPrimitiveSub.tsx (1)
4-10: Consider enhancing type definition for comprehensive prop support.The type definition could be more inclusive to properly support the additional props being forwarded.
Consider updating the type definition:
-export type MenuPrimitiveSubProps = { - children: React.ReactNode - className?: string - open?: boolean - onOpenChange?: (open: boolean) => void - defaultOpen?: boolean -} +export type MenuPrimitiveSubProps = { + children: React.ReactNode + className?: string + open?: boolean + onOpenChange?: (open: boolean) => void + defaultOpen?: boolean +} & React.HTMLAttributes<HTMLElement>src/components/ui/ContextMenu/fragments/ContextMenuPortal.tsx (1)
15-15: Remove unused variablerootClass.The
rootClassvariable is extracted from context but never used in this component, as confirmed by static analysis.- const { rootClass } = context; + // Context validation only - no properties needed for portalsrc/components/ui/ContextMenu/stories/ContextMenu.stories.tsx (1)
13-13: Consider extracting the inline SVG component.The
ChevronRightcomponent is defined inline. For better reusability and maintainability, consider extracting it to a shared icons directory or using an existing icon from the design system.+import { ChevronRightIcon } from '~/components/icons'; // or appropriate path -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>src/components/ui/ContextMenu/fragments/ContextMenuSubTrigger.tsx (1)
14-14: Useconsole.warninstead ofconsole.logfor better visibility.For development warnings like context usage issues,
console.warnprovides better visibility in browser dev tools compared toconsole.log.- console.log('ContextMenuSubTrigger should be used in the ContextMenuRoot'); + console.warn('ContextMenuSubTrigger should be used in the ContextMenuRoot');src/components/ui/ContextMenu/fragments/ContextMenuItem.tsx (1)
15-15: Useconsole.warninstead ofconsole.logfor better visibility.For development warnings like context usage issues,
console.warnprovides better visibility in browser dev tools compared toconsole.log.- console.log('ContextMenuItem should be used in the ContextMenuRoot'); + console.warn('ContextMenuItem should be used in the ContextMenuRoot');src/components/ui/ContextMenu/fragments/ContextMenuTrigger.tsx (1)
15-15: Useconsole.warninstead ofconsole.logfor better visibility.For development warnings,
console.warnprovides better visibility in browser dev tools.- console.log('ContextMenuTrigger should be used in the ContextMenuRoot'); + console.warn('ContextMenuTrigger should be used in the ContextMenuRoot');styles/themes/components/context-menu.scss (3)
25-25: Fix formatting: missing space after colon.-width:max-content; +width: max-content;
91-95: Remove duplicate width property.The
width: max-contenton line 91 is overridden bywidth: 100%on line 95.outline: none; - width:max-content; + width: max-content; cursor: pointer; border-radius: 2px; display: flex; width: 100%;Actually, remove the first width declaration since it's immediately overridden:
outline: none; - width:max-content; cursor: pointer; border-radius: 2px; display: flex; width: 100%;
39-133: Consider refactoring shared styles to reduce duplication.The
.rad-ui-context-menu-sub-triggerand.rad-ui-context-menu-itemclasses share many identical styles (font-size, line-height, color, padding, user-select, outline, cursor, border-radius, display, box-sizing, text-align, gap, background, border, transition, and all state styles).Consider creating a shared base class or mixin to reduce code duplication:
%menu-item-base { font-size: medium; line-height: 1.4; color: var(--rad-ui-color-accent-900); padding: 8px 18px; user-select: none; outline: none; cursor: pointer; border-radius: 2px; display: flex; width: 100%; box-sizing: border-box; text-align: left; gap: 8px; background: none; border: none; transition: background 0.2s; &:hover { background-color: var(--rad-ui-color-accent-200); } &:focus-visible, &:focus { outline: none; background-color: var(--rad-ui-color-accent-300); box-shadow: 0 0 0 2px var(--rad-ui-color-accent-500) inset; } &[data-highlighted] { background-color: var(--rad-ui-color-accent-300); color: var(--rad-ui-color-accent-900); } &[data-disabled] { color: var(--rad-ui-color-accent-500); pointer-events: none; opacity: 0.5; } } .rad-ui-context-menu-sub-trigger { @extend %menu-item-base; padding-left: 18px; justify-content: space-between; align-items: center; } .rad-ui-context-menu-item { @extend %menu-item-base; &[aria-selected="true"] { background-color: var(--rad-ui-color-accent-400); color: var(--rad-ui-color-accent-900); font-weight: 500; &:hover { background-color: var(--rad-ui-color-accent-500); } } }src/components/ui/ContextMenu/fragments/ContextMenuRoot.tsx (1)
22-22: Consider initializing coordinates based on context menu trigger position.The coordinates are initialized to
{ x: 0, y: 0 }, which means the context menu will initially appear at the origin. Consider if there's a more appropriate default or if this should be documented as requiring proper coordinate setup via the context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/components/ui/ContextMenu/ContextMenu.tsx(1 hunks)src/components/ui/ContextMenu/contexts/ContextMenuContext.tsx(1 hunks)src/components/ui/ContextMenu/fragments/ContextMenuContent.tsx(1 hunks)src/components/ui/ContextMenu/fragments/ContextMenuItem.tsx(1 hunks)src/components/ui/ContextMenu/fragments/ContextMenuPortal.tsx(1 hunks)src/components/ui/ContextMenu/fragments/ContextMenuRoot.tsx(1 hunks)src/components/ui/ContextMenu/fragments/ContextMenuSub.tsx(1 hunks)src/components/ui/ContextMenu/fragments/ContextMenuSubTrigger.tsx(1 hunks)src/components/ui/ContextMenu/fragments/ContextMenuTrigger.tsx(1 hunks)src/components/ui/ContextMenu/stories/ContextMenu.stories.tsx(1 hunks)src/core/primitives/Menu/fragments/MenuPrimitiveContent.tsx(2 hunks)src/core/primitives/Menu/fragments/MenuPrimitiveItem.tsx(2 hunks)src/core/primitives/Menu/fragments/MenuPrimitiveRoot.tsx(4 hunks)src/core/primitives/Menu/fragments/MenuPrimitiveSub.tsx(1 hunks)src/core/primitives/Menu/fragments/MenuPrimitiveTrigger.tsx(1 hunks)styles/themes/components/context-menu.scss(1 hunks)styles/themes/components/dropdown-menu.scss(0 hunks)styles/themes/default.scss(1 hunks)
💤 Files with no reviewable changes (1)
- styles/themes/components/dropdown-menu.scss
🧰 Additional context used
🧠 Learnings (13)
📓 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`.
styles/themes/default.scss (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/fragments/MenuPrimitiveContent.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/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/fragments/MenuPrimitiveTrigger.tsx (4)
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: 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/MenuPrimitiveItem.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/ContextMenuRoot.tsx (2)
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.
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/ContextMenuSubTrigger.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: 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.
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.
src/core/primitives/Menu/fragments/MenuPrimitiveSub.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/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.
styles/themes/components/context-menu.scss (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/components/ui/ContextMenu/ContextMenu.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/components/ui/ContextMenu/fragments/ContextMenuTrigger.tsx (3)
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.
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.
🧬 Code Graph Analysis (5)
src/components/ui/ContextMenu/contexts/ContextMenuContext.tsx (3)
src/components/ui/Accordion/contexts/AccordionContext.tsx (1)
AccordionContextType(3-11)src/components/ui/Callout/contexts/CalloutContext.tsx (1)
CalloutContextType(3-5)src/components/ui/Select/contexts/SelectRootContext.tsx (1)
SelectRootContextType(3-5)
src/core/primitives/Menu/fragments/MenuPrimitiveContent.tsx (1)
src/components/ui/Select/fragments/SelectContent.tsx (1)
SelectContent(6-20)
src/components/ui/ContextMenu/fragments/ContextMenuItem.tsx (2)
src/components/ui/Select/fragments/SelectItem.tsx (1)
SelectItem(6-22)src/components/ui/Callout/fragments/CalloutIcon.tsx (1)
CalloutIcon(10-18)
src/core/primitives/Menu/fragments/MenuPrimitiveItem.tsx (2)
src/core/primitives/Select/fragments/SelectPrimitiveItem.tsx (1)
SelectPrimitiveItem(14-39)src/core/primitives/Select/fragments/SelectPrimitiveTrigger.tsx (1)
SelectPrimitiveTrigger(9-22)
src/components/ui/ContextMenu/fragments/ContextMenuSubTrigger.tsx (1)
src/components/ui/Select/fragments/SelectTrigger.tsx (1)
SelectTrigger(7-24)
🪛 GitHub Check: lint
src/components/ui/ContextMenu/fragments/ContextMenuPortal.tsx
[warning] 15-15:
'rootClass' is assigned a value but never used
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
🪛 Biome (2.1.2)
src/core/primitives/Menu/fragments/MenuPrimitiveTrigger.tsx
[error] 16-16: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 25-25: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
src/components/ui/ContextMenu/fragments/ContextMenuTrigger.tsx
[error] 19-19: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
⏰ 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 (23)
src/components/ui/ContextMenu/contexts/ContextMenuContext.tsx (3)
1-1: LGTM: Appropriate use of 'use client' directive.The 'use client' directive is correctly placed for a React context that will be used in client-side components.
5-9: LGTM: Clean and focused interface design.The
ContextMenuContextPropsinterface is well-designed with appropriate props for context menu functionality:
rootClassfor consistent styling (following the pattern seen in other contexts likeCalloutContextandSelectRootContext)setCoordsfor dynamic positioning based on user interactionsetIsOpenfor state management
11-13: LGTM: Standard React context pattern.The context implementation follows React best practices with proper typing and null default value, requiring consumers to handle null checks appropriately.
styles/themes/default.scss (1)
42-42: LGTM: Proper integration of context-menu styles.The import is correctly placed and follows the established pattern of other component style imports in the theme file.
src/core/primitives/Menu/fragments/MenuPrimitiveItem.tsx (1)
11-11: LGTM: Proper prop forwarding implementation.The addition of the rest parameter and prop spreading follows the established pattern seen in other primitive components and enables enhanced flexibility for the ContextMenu implementation.
Also applies to: 30-30
src/core/primitives/Menu/fragments/MenuPrimitiveContent.tsx (1)
11-11: LGTM: Consistent prop forwarding pattern.The prop forwarding implementation is consistent with other primitive components and properly positioned after the existing props, allowing for appropriate override behavior.
Also applies to: 30-30
src/core/primitives/Menu/fragments/MenuPrimitiveSub.tsx (1)
12-12: LGTM: Proper prop forwarding to MenuComponentRoot.The implementation correctly forwards additional props to the underlying
MenuComponentRootcomponent, maintaining consistency with the prop forwarding pattern across primitive components.Also applies to: 14-14
src/components/ui/ContextMenu/fragments/ContextMenuPortal.tsx (1)
9-21: LGTM! Well-implemented portal component.The implementation follows established patterns from other portal components in the codebase. Context validation with proper warning logging and clean prop forwarding to the underlying primitive are well executed.
src/components/ui/ContextMenu/stories/ContextMenu.stories.tsx (1)
15-50: Excellent story implementation with comprehensive menu structure.The story effectively demonstrates the hierarchical capabilities of the ContextMenu component with nested submenus and proper usage of all subcomponents. The SandboxEditor wrapper is consistent with other stories in the codebase.
src/components/ui/ContextMenu/fragments/ContextMenuSub.tsx (1)
11-23: LGTM! Clean submenu component implementation.The component properly handles context validation, combines class names using
clsx, and forwards props correctly to the underlying primitive. TherootClasswith "-sub" suffix provides consistent styling integration.src/components/ui/ContextMenu/ContextMenu.tsx (1)
9-22: LGTM! Consistent composite component pattern.The implementation follows the established pattern used by other composite components in the codebase (Tabs, Table, Collapsible). The warning message appropriately guides users to the correct usage pattern, and all subcomponents are properly exposed as static properties.
src/components/ui/ContextMenu/fragments/ContextMenuContent.tsx (1)
11-23: LGTM! Well-structured content component.The component follows the established pattern of other context menu fragments with proper context validation, consistent class name combination using
clsx, and correct prop forwarding to the underlying primitive.src/components/ui/ContextMenu/fragments/ContextMenuSubTrigger.tsx (1)
11-23: LGTM! Component follows established patterns.The implementation correctly follows the established pattern used across similar components in the codebase, including proper context usage, early return handling, and className composition.
src/core/primitives/Menu/fragments/MenuPrimitiveTrigger.tsx (1)
19-31: LGTM! Good refactoring to use Primitive.button.The changes to use
Primitive.buttoninstead of native button element, along with the addition ofasChildprop support and proper prop forwarding, align well with the established patterns in the codebase.src/components/ui/ContextMenu/fragments/ContextMenuItem.tsx (1)
12-24: LGTM! Component follows established patterns.The implementation correctly follows the established pattern, properly handling context usage, early returns, and prop forwarding to the underlying primitive component.
src/components/ui/ContextMenu/fragments/ContextMenuTrigger.tsx (1)
24-26: Verify the y-coordinate calculation logic.The y-coordinate calculation
rect.bottom - e.clientYseems unusual. This calculates the distance from the bottom of the element to the click point, which would give negative values for clicks above the bottom edge.Typically, you'd want either:
e.clientY - rect.topfor position relative to element tope.clientYfor absolute viewport positionCould you clarify the intended positioning behavior? If this calculation is correct for the context menu positioning logic, consider adding a comment explaining why
rect.bottom - e.clientYis used.src/components/ui/ContextMenu/fragments/ContextMenuRoot.tsx (3)
8-12: LGTM: Well-structured type definition.The type definition properly extends the primitive props while adding context menu-specific properties. The optional
customRootClassandclassNameprops provide good flexibility for styling customization.
16-21: LGTM: Proper controlled/uncontrolled state management.The implementation correctly uses
useControllableStateto handle both controlled and uncontrolled scenarios for the open state, with a sensible default offalse.
25-29: Verify offset calculation logic for positioning.The coordinate mapping to offsets seems inverted:
mainAxisOffset={-coords.y}andcrossAxisOffset={coords.x}. Please verify this is the intended behavior for context menu positioning.src/core/primitives/Menu/fragments/MenuPrimitiveRoot.tsx (4)
12-13: LGTM: Clean addition of positioning props.The new
crossAxisOffsetandmainAxisOffsetprops are well-defined as optional numbers, following the existing prop pattern in the interface.
16-16: LGTM: Proper default values and prop destructuring.The component correctly provides sensible defaults (0) for both offset props and properly destructures them from the props object.
43-46: LGTM: Correct integration with Floater offset middleware.The offset middleware is properly configured to use the new props, allowing for dynamic positioning of the floating menu based on the provided offsets.
116-116: LGTM: Consistent prop forwarding pattern.Both the
MenuComponentRootandMenuPrimitiveRootcomponents properly forward the new offset props and maintain the existing pattern of spreading additional props to the root element.Also applies to: 128-131
| const context = React.useContext(ContextMenuContext); | ||
| if (!context) { | ||
| console.log('ContextMenuTrigger should be used in the ContextMenuRoot'); | ||
| return null; | ||
| } | ||
| const { rootClass, setCoords, setIsOpen } = context; | ||
| const contextTriggerRef = React.useRef<HTMLSpanElement>(null); |
There was a problem hiding this comment.
Critical: Fix React hooks rule violation.
The useRef hook on line 19 is called after a potential early return on line 16, which violates the rules of hooks.
Move the useRef hook before the context check:
const ContextMenuTrigger = ({ children, className, ...props }:ContextMenuTriggerProps) => {
+ const contextTriggerRef = React.useRef<HTMLSpanElement>(null);
const context = React.useContext(ContextMenuContext);
if (!context) {
console.log('ContextMenuTrigger should be used in the ContextMenuRoot');
return null;
}
const { rootClass, setCoords, setIsOpen } = context;
- const contextTriggerRef = React.useRef<HTMLSpanElement>(null);📝 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.
| const context = React.useContext(ContextMenuContext); | |
| if (!context) { | |
| console.log('ContextMenuTrigger should be used in the ContextMenuRoot'); | |
| return null; | |
| } | |
| const { rootClass, setCoords, setIsOpen } = context; | |
| const contextTriggerRef = React.useRef<HTMLSpanElement>(null); | |
| const ContextMenuTrigger = ({ children, className, ...props }: ContextMenuTriggerProps) => { | |
| const contextTriggerRef = React.useRef<HTMLSpanElement>(null); | |
| const context = React.useContext(ContextMenuContext); | |
| if (!context) { | |
| console.log('ContextMenuTrigger should be used in the ContextMenuRoot'); | |
| return null; | |
| } | |
| const { rootClass, setCoords, setIsOpen } = context; | |
| // …rest of implementation | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 19-19: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🤖 Prompt for AI Agents
In src/components/ui/ContextMenu/fragments/ContextMenuTrigger.tsx around lines
13 to 19, the useRef hook is called after a conditional return, violating React
hooks rules. To fix this, move the declaration of the contextTriggerRef using
useRef to before the context null check so that all hooks are called
unconditionally and in the same order on every render.
| <span ref={contextTriggerRef} onContextMenu={handleContextMenu} onClick={(e) => {e.preventDefault();}}> | ||
| {children} | ||
| </span> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add keyboard accessibility support.
The span element with click/contextmenu handlers needs proper keyboard support and ARIA attributes for accessibility.
- <span ref={contextTriggerRef} onContextMenu={handleContextMenu} onClick={(e) => {e.preventDefault();}}>
+ <span
+ ref={contextTriggerRef}
+ onContextMenu={handleContextMenu}
+ onClick={(e) => {e.preventDefault();}}
+ onKeyDown={(e) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ e.preventDefault();
+ handleContextMenu(e as any);
+ }
+ }}
+ role="button"
+ tabIndex={0}
+ aria-haspopup="menu"
+ >
{children}
</span>📝 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} | |
| onContextMenu={handleContextMenu} | |
| onClick={(e) => { | |
| e.preventDefault(); | |
| }} | |
| onKeyDown={(e) => { | |
| if (e.key === 'Enter' || e.key === ' ') { | |
| e.preventDefault(); | |
| handleContextMenu(e as any); | |
| } | |
| }} | |
| role="button" | |
| tabIndex={0} | |
| aria-haspopup="menu" | |
| > | |
| {children} | |
| </span> |
🤖 Prompt for AI Agents
In src/components/ui/ContextMenu/fragments/ContextMenuTrigger.tsx around lines
34 to 36, the span element handling context menu and click events lacks keyboard
accessibility and ARIA attributes. To fix this, add keyboard event handlers such
as onKeyDown to handle keyboard triggers (e.g., Enter and Space keys) for
opening the context menu. Also, include appropriate ARIA attributes like
role="button" and tabIndex={0} on the span to make it focusable and announce its
interactive nature to assistive technologies.
| const MenuPrimitiveTrigger = ({ children, className, asChild, ...props }: MenuPrimitiveTriggerProps) => { | ||
| const context = useContext(MenuPrimitiveRootContext); | ||
| if (!context) return null; | ||
| const { isOpen, setIsOpen, activeIndex, refs, floatingStyles, getReferenceProps, isNested } = context; | ||
| const { ref, index } = Floater.useListItem(); |
There was a problem hiding this comment.
Critical: Fix React hooks rule violation.
Hooks are being called after an early return, which violates the rules of hooks. The useListItem() hook on line 16 and useMergeRefs() on line 25 are called after the potential early return on line 14.
Move the hooks before the context check:
const MenuPrimitiveTrigger = ({ children, className, asChild, ...props }: MenuPrimitiveTriggerProps) => {
+ const { ref, index } = Floater.useListItem();
const context = useContext(MenuPrimitiveRootContext);
- if (!context) return null;
+ if (!context) return null;
const { isOpen, setIsOpen, activeIndex, refs, floatingStyles, getReferenceProps, isNested } = context;
- const { ref, index } = Floater.useListItem();🧰 Tools
🪛 Biome (2.1.2)
[error] 16-16: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🤖 Prompt for AI Agents
In src/core/primitives/Menu/fragments/MenuPrimitiveTrigger.tsx around lines 12
to 16, the hooks useListItem() and useMergeRefs() are called after an early
return caused by a missing context, violating React hooks rules. To fix this,
move all hook calls, including useListItem() and useMergeRefs(), to before the
context null check on line 14, ensuring hooks are always called in the same
order regardless of conditional returns.
Screen.Recording.2025-07-28.225514.mp4
Summary by CodeRabbit
New Features
Enhancements
Style