menu primitive #1252
menu primitive #1252
Conversation
WalkthroughThis change introduces a new floating menu primitive system. It adds a set of React components, supporting contexts, and utility exports to enable nested, accessible, floating menus. The update includes a new story for demonstration, expanded floating UI exports, and comprehensive context management for menu state and interactions. Changes
Sequence Diagram(s)sequenceDiagram participant User participant MenuPrimitiveTrigger participant MenuPrimitiveRoot participant MenuPrimitiveContent participant MenuPrimitiveItem participant MenuPrimitivePortal User->>MenuPrimitiveTrigger: Clicks trigger MenuPrimitiveTrigger->>MenuPrimitiveRoot: setIsOpen(true) MenuPrimitiveRoot->>MenuPrimitiveContent: Renders menu content MenuPrimitiveContent->>MenuPrimitiveItem: Renders menu items MenuPrimitiveContent->>MenuPrimitivePortal: Renders portal if open MenuPrimitiveItem->>MenuPrimitiveRoot: Handles item events (click, hover, etc.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 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: 12
🧹 Nitpick comments (4)
src/core/primitives/Menu/fragments/MenuPrimitiveTrigger.tsx (1)
1-1: Remove unused import.The
useStateimport is not used in this component.-import React, { useState, useContext } from 'react'; +import React, { useContext } from 'react';src/core/primitives/Menu/fragments/MenuPrimitiveContent.tsx (2)
1-1: Remove unused import.The
useEffectimport is not used in this component.-import React, { useEffect, useContext } from 'react'; +import React, { useContext } from 'react';
9-9: Remove unused destructured variables.Some variables are destructured from context but not used in the component.
- const { isOpen, refs, floatingStyles, getFloatingProps, elementsRef, labelsRef, nodeId, isNested } = context; + const { refs, floatingStyles, getFloatingProps, elementsRef, labelsRef } = context;src/core/primitives/Menu/contexts/MenuPrimitiveRootContext.tsx (1)
17-26: Consider more specific types where possible.Several properties use
anytype which reduces type safety. While this might be necessary due to floating UI's complex typing, consider if any of these can be more specifically typed.If floating UI provides more specific types for these properties, consider using them:
- getReferenceProps: (userProps?: any) => any; - getFloatingProps: (userProps?: any) => any; - getItemProps: (userProps?: any) => any; + getReferenceProps: (userProps?: React.HTMLProps<Element>) => React.HTMLProps<Element>; + getFloatingProps: (userProps?: React.HTMLProps<HTMLElement>) => React.HTMLProps<HTMLElement>; + getItemProps: (userProps?: React.HTMLProps<HTMLElement>) => React.HTMLProps<HTMLElement>;Note: Verify that floating UI supports these more specific types before applying this change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/core/primitives/Floater/index.tsx(1 hunks)src/core/primitives/Menu/MenuPrimitive.tsx(1 hunks)src/core/primitives/Menu/contexts/MenuPrimitiveRootContext.tsx(1 hunks)src/core/primitives/Menu/fragments/MenuPrimitiveContent.tsx(1 hunks)src/core/primitives/Menu/fragments/MenuPrimitiveGroup.tsx(1 hunks)src/core/primitives/Menu/fragments/MenuPrimitiveItem.tsx(1 hunks)src/core/primitives/Menu/fragments/MenuPrimitiveRoot.tsx(1 hunks)src/core/primitives/Menu/fragments/MenuPrimitiveTrigger.tsx(1 hunks)src/core/primitives/Menu/stories/MenuPrimitive.stories.tsx(1 hunks)
🧠 Learnings (9)
📓 Common learnings
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`. 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/core/primitives/Menu/fragments/MenuPrimitiveItem.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: GoldGroove06
PR: #1215
File: src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveTrigger.tsx:25-27
Timestamp: 2025-07-18T16:43:26.143Z
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/MenuPrimitiveGroup.tsx (2)
Learnt from: GoldGroove06
PR: #1215
File: src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveTrigger.tsx:25-27
Timestamp: 2025-07-18T16:43:26.143Z
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: 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 (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: GoldGroove06
PR: #1215
File: src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveTrigger.tsx:25-27
Timestamp: 2025-07-18T16:43:26.143Z
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/MenuPrimitive.tsx (4)
Learnt from: GoldGroove06
PR: #1215
File: src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveTrigger.tsx:25-27
Timestamp: 2025-07-18T16:43:26.143Z
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: 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: 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/stories/MenuPrimitive.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/contexts/MenuPrimitiveRootContext.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: GoldGroove06
PR: #1215
File: src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveTrigger.tsx:25-27
Timestamp: 2025-07-18T16:43:26.143Z
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/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: GoldGroove06
PR: #1215
File: src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveTrigger.tsx:25-27
Timestamp: 2025-07-18T16:43:26.143Z
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.
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.
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
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`. 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/core/primitives/Menu/fragments/MenuPrimitiveItem.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: GoldGroove06
PR: #1215
File: src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveTrigger.tsx:25-27
Timestamp: 2025-07-18T16:43:26.143Z
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/MenuPrimitiveGroup.tsx (2)
Learnt from: GoldGroove06
PR: #1215
File: src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveTrigger.tsx:25-27
Timestamp: 2025-07-18T16:43:26.143Z
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: 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 (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: GoldGroove06
PR: #1215
File: src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveTrigger.tsx:25-27
Timestamp: 2025-07-18T16:43:26.143Z
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/MenuPrimitive.tsx (4)
Learnt from: GoldGroove06
PR: #1215
File: src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveTrigger.tsx:25-27
Timestamp: 2025-07-18T16:43:26.143Z
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: 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: 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/stories/MenuPrimitive.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/contexts/MenuPrimitiveRootContext.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: GoldGroove06
PR: #1215
File: src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveTrigger.tsx:25-27
Timestamp: 2025-07-18T16:43:26.143Z
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/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: GoldGroove06
PR: #1215
File: src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveTrigger.tsx:25-27
Timestamp: 2025-07-18T16:43:26.143Z
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.
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.
⏰ 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 (2)
src/core/primitives/Floater/index.tsx (1)
1-31: LGTM: Clean expansion of floating UI utilities.The expansion of imports and exports from
@floating-ui/reactis well-structured and provides the necessary utilities for the menu primitive system. The object structure maintains consistency with the existing pattern.src/core/primitives/Menu/fragments/MenuPrimitiveRoot.tsx (1)
64-70: LGTM! Proper floating UI integration.The FloatingTree and FloatingNode usage correctly enables nested menu support and follows floating UI best practices.
| <div | ||
| ref={refs.setFloating} | ||
| style={floatingStyles} | ||
| {...getFloatingProps()} | ||
| className={className} | ||
| > | ||
| <Floater.FloatingList elementsRef={elementsRef} labelsRef={labelsRef}> | ||
| | ||
| {children} | ||
| | ||
| </Floater.FloatingList> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add ARIA role for menu content.
Menu content should have proper ARIA role for accessibility.
<div ref={refs.setFloating} style={floatingStyles} {...getFloatingProps()} className={className} + role="menu" >🤖 Prompt for AI Agents
In src/core/primitives/Menu/fragments/MenuPrimitiveContent.tsx between lines 12 and 23, the menu content div lacks an ARIA role attribute. Add the appropriate ARIA role, such as role="menu", to the div containing the menu content to improve accessibility and ensure assistive technologies correctly identify it as a menu. | @@ -0,0 +1,10 @@ | |||
| import React from 'react'; | |||
| | |||
| const MenuPrimitiveGroup = ({ children, className }:any) => { | |||
There was a problem hiding this comment.
Fix TypeScript typing and apply className prop.
The component has several issues:
- Uses
anytype instead of proper TypeScript interface - Declares
classNameprop but doesn't apply it to the div element
-const MenuPrimitiveGroup = ({ children, className }:any) => { +interface MenuPrimitiveGroupProps { + children: React.ReactNode; + className?: string; +} + +const MenuPrimitiveGroup = ({ children, className }: MenuPrimitiveGroupProps) => {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/core/primitives/Menu/fragments/MenuPrimitiveGroup.tsx at line 3, replace the use of 'any' with a proper TypeScript interface defining the props, including children as React.ReactNode and className as an optional string. Then, ensure the className prop is applied to the div element rendered by the component to correctly pass styling classes. | <div> | ||
| {children} | ||
| </div> |
There was a problem hiding this comment.
Apply className prop and add ARIA role for semantic grouping.
The className prop is declared but not applied to the div element. Additionally, menu groups should have proper ARIA attributes for accessibility.
- <div> + <div className={className} role="group"> {children} </div>📝 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.
| <div> | |
| {children} | |
| </div> | |
| <div className={className} role="group"> | |
| {children} | |
| </div> |
🤖 Prompt for AI Agents
In src/core/primitives/Menu/fragments/MenuPrimitiveGroup.tsx around lines 5 to 7, the div element does not apply the className prop and lacks ARIA role attributes. Fix this by adding the className prop to the div and include an appropriate ARIA role such as role="group" to improve semantic grouping and accessibility. | virtualItemRef, | ||
| nodeId, | ||
| isNested, | ||
| floatingContext |
There was a problem hiding this comment.
Add missing floatingContext property to context interface.
The context value includes floatingContext but this property is not defined in the MenuPrimitiveRootPrimitiveContextProps interface, which will cause TypeScript errors.
Add the missing property to the context interface in MenuPrimitiveRootContext.tsx:
export interface MenuPrimitiveRootPrimitiveContextProps { // ... existing properties + floatingContext: any; // or more specific floating UI context type }🤖 Prompt for AI Agents
In src/core/primitives/Menu/fragments/MenuPrimitiveRoot.tsx at line 59, the context value includes the property floatingContext, but this property is missing from the MenuPrimitiveRootPrimitiveContextProps interface, causing TypeScript errors. To fix this, add the floatingContext property with the correct type to the MenuPrimitiveRootPrimitiveContextProps interface in the MenuPrimitiveRootContext.tsx file to ensure type consistency. | <button | ||
| className={className} | ||
| onClick={() => setIsOpen(!isOpen)} | ||
| tabIndex={isNested ? activeIndex === index ? 0 : -1 : 0} | ||
| ref={Floater.useMergeRefs([refs.setReference, ref])} | ||
| {...getReferenceProps()} | ||
| > | ||
| {children} | ||
| </button> | ||
| ); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add ARIA attributes for menu trigger accessibility.
Based on the retrieved learning about verifying existing ARIA attributes, I need to check if the getReferenceProps() already includes ARIA attributes. However, menu triggers should have aria-haspopup and aria-expanded attributes.
Let me verify what props are already included in getReferenceProps():
🏁 Script executed:
#!/bin/bash # Search for getReferenceProps implementation and usage to understand what ARIA attributes might already be included ast-grep --pattern 'getReferenceProps'Length of output: 662
🏁 Script executed:
#!/bin/bash # Display the first 50 lines of MenuPrimitiveTrigger.tsx to inspect imports and getReferenceProps origin sed -n '1,50p' src/core/primitives/Menu/fragments/MenuPrimitiveTrigger.tsxLength of output: 944
🏁 Script executed:
#!/bin/bash # Find and display the context definition to locate getReferenceProps rg -n "MenuPrimitiveRootContext" -g "src/core/primitives/Menu/**" # Once we find the file, print its contents FILE=$(rg -l "createContext" -g "src/core/primitives/Menu/**MenuPrimitiveRootContext*") if [ -n "$FILE" ]; then echo "=== $FILE ===" sed -n '1,200p' "$FILE" else echo "MenuPrimitiveRootContext file not found" fiLength of output: 1562
🏁 Script executed:
#!/bin/bash echo "Floater directory contents:" find src/core/primitives/Floater -maxdepth 1 -type f echo -e "\nFiles exporting Floater:" rg -l "export default Floater" -g "src/core/primitives/Floater/**" echo -e "\nOccurrences of getReferenceProps in Floater:" rg -n "getReferenceProps" -g "src/core/primitives/Floater/**" echo -e "\nListing all hooks in Floater files:" rg -n "function use" -g "src/core/primitives/Floater/**"Length of output: 642
🏁 Script executed:
#!/bin/bash # Display the contents of Floater index to inspect getReferenceProps definition and included ARIA attributes sed -n '1,200p' src/core/primitives/Floater/index.tsxLength of output: 1002
🏁 Script executed:
#!/bin/bash # Display first 200 lines of MenuPrimitiveRoot.tsx to inspect getReferenceProps creation and included ARIA attributes sed -n '1,200p' src/core/primitives/Menu/fragments/MenuPrimitiveRoot.tsxLength of output: 2256
Add missing ARIA attributes to MenuPrimitiveTrigger
getReferenceProps() from useInteractions only wires up event handlers—it doesn’t inject menu-specific ARIA attributes. To meet accessibility best practices, add aria-haspopup="menu" and aria-expanded={isOpen} on the trigger button:
• File: src/core/primitives/Menu/fragments/MenuPrimitiveTrigger.tsx
Adjust the <button> props as follows:
- <button - className={className} - onClick={() => setIsOpen(!isOpen)} - tabIndex={isNested ? activeIndex === index ? 0 : -1 : 0} - ref={Floater.useMergeRefs([refs.setReference, ref])} - {...getReferenceProps()} - > + <button + className={className} + aria-haspopup="menu" + aria-expanded={isOpen} + onClick={() => setIsOpen(!isOpen)} + tabIndex={isNested ? activeIndex === index ? 0 : -1 : 0} + ref={Floater.useMergeRefs([refs.setReference, ref])} + {...getReferenceProps()} + >This ensures screen readers announce the presence of a menu and its open/closed state.
📝 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.
| <button | |
| className={className} | |
| onClick={() => setIsOpen(!isOpen)} | |
| tabIndex={isNested ? activeIndex === index ? 0 : -1 : 0} | |
| ref={Floater.useMergeRefs([refs.setReference, ref])} | |
| {...getReferenceProps()} | |
| > | |
| {children} | |
| </button> | |
| ); | |
| <button | |
| className={className} | |
| aria-haspopup="menu" | |
| aria-expanded={isOpen} | |
| onClick={() => setIsOpen(!isOpen)} | |
| tabIndex={isNested ? (activeIndex === index ? 0 : -1) : 0} | |
| ref={Floater.useMergeRefs([refs.setReference, ref])} | |
| {...getReferenceProps()} | |
| > | |
| {children} | |
| </button> |
🤖 Prompt for AI Agents
In src/core/primitives/Menu/fragments/MenuPrimitiveTrigger.tsx around lines 12 to 21, the button element lacks necessary ARIA attributes for accessibility. Add aria-haspopup="menu" and aria-expanded={isOpen} to the button props to indicate that it controls a menu and to reflect its open or closed state. This will improve screen reader support and meet accessibility best practices. | <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">item 1</MenuPrimitive.Item> | ||
| <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">item 1</MenuPrimitive.Item> | ||
| <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">item 1</MenuPrimitive.Item> | ||
| <MenuPrimitive.Root> | ||
| <MenuPrimitive.Trigger className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">Trigger</MenuPrimitive.Trigger> | ||
| <MenuPrimitive.Content className="flex flex-col mt-2 bg-gray-50 border border-gray-200 rounded shadow min-w-[160px]"> | ||
| <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">item 1</MenuPrimitive.Item> | ||
| <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">item 1</MenuPrimitive.Item> | ||
| <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">item 1</MenuPrimitive.Item> | ||
| <MenuPrimitive.Root> | ||
| <MenuPrimitive.Trigger className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">Trigger</MenuPrimitive.Trigger> | ||
| <MenuPrimitive.Content className="flex flex-col mt-2 bg-gray-100 border border-gray-200 rounded shadow min-w-[140px]"> | ||
| <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-200 cursor-pointer rounded">item 1</MenuPrimitive.Item> | ||
| <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-200 cursor-pointer rounded">item 1</MenuPrimitive.Item> | ||
| <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-200 cursor-pointer rounded">item 1</MenuPrimitive.Item> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use descriptive and unique labels for menu items.
All menu items currently have the same label "item 1", which makes it difficult to distinguish between items and provides a poor demonstration experience. Consider using more descriptive labels that reflect the menu hierarchy and functionality.
Apply this diff to improve the menu item labels:
- <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">item 1</MenuPrimitive.Item> - <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">item 1</MenuPrimitive.Item> - <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">item 1</MenuPrimitive.Item> + <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">New Document</MenuPrimitive.Item> + <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">Open File</MenuPrimitive.Item> + <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">Save</MenuPrimitive.Item> <MenuPrimitive.Root> - <MenuPrimitive.Trigger className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">Trigger</MenuPrimitive.Trigger> + <MenuPrimitive.Trigger className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">Export ›</MenuPrimitive.Trigger> <MenuPrimitive.Content className="flex flex-col mt-2 bg-gray-50 border border-gray-200 rounded shadow min-w-[160px]"> - <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">item 1</MenuPrimitive.Item> - <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">item 1</MenuPrimitive.Item> - <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">item 1</MenuPrimitive.Item> + <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">PDF</MenuPrimitive.Item> + <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">Excel</MenuPrimitive.Item> + <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">Word</MenuPrimitive.Item> <MenuPrimitive.Root> - <MenuPrimitive.Trigger className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">Trigger</MenuPrimitive.Trigger> + <MenuPrimitive.Trigger className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">Advanced ›</MenuPrimitive.Trigger> <MenuPrimitive.Content className="flex flex-col mt-2 bg-gray-100 border border-gray-200 rounded shadow min-w-[140px]"> - <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-200 cursor-pointer rounded">item 1</MenuPrimitive.Item> - <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-200 cursor-pointer rounded">item 1</MenuPrimitive.Item> - <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-200 cursor-pointer rounded">item 1</MenuPrimitive.Item> + <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-200 cursor-pointer rounded">With Images</MenuPrimitive.Item> + <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-200 cursor-pointer rounded">Text Only</MenuPrimitive.Item> + <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-200 cursor-pointer rounded">Custom Format</MenuPrimitive.Item>📝 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.
| <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">item 1</MenuPrimitive.Item> | |
| <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">item 1</MenuPrimitive.Item> | |
| <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">item 1</MenuPrimitive.Item> | |
| <MenuPrimitive.Root> | |
| <MenuPrimitive.Trigger className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">Trigger</MenuPrimitive.Trigger> | |
| <MenuPrimitive.Content className="flex flex-col mt-2 bg-gray-50 border border-gray-200 rounded shadow min-w-[160px]"> | |
| <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">item 1</MenuPrimitive.Item> | |
| <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">item 1</MenuPrimitive.Item> | |
| <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">item 1</MenuPrimitive.Item> | |
| <MenuPrimitive.Root> | |
| <MenuPrimitive.Trigger className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">Trigger</MenuPrimitive.Trigger> | |
| <MenuPrimitive.Content className="flex flex-col mt-2 bg-gray-100 border border-gray-200 rounded shadow min-w-[140px]"> | |
| <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-200 cursor-pointer rounded">item 1</MenuPrimitive.Item> | |
| <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-200 cursor-pointer rounded">item 1</MenuPrimitive.Item> | |
| <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-200 cursor-pointer rounded">item 1</MenuPrimitive.Item> | |
| <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">New Document</MenuPrimitive.Item> | |
| <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">Open File</MenuPrimitive.Item> | |
| <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">Save</MenuPrimitive.Item> | |
| <MenuPrimitive.Root> | |
| <MenuPrimitive.Trigger className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">Export ›</MenuPrimitive.Trigger> | |
| <MenuPrimitive.Content className="flex flex-col mt-2 bg-gray-50 border border-gray-200 rounded shadow min-w-[160px]"> | |
| <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">PDF</MenuPrimitive.Item> | |
| <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">Excel</MenuPrimitive.Item> | |
| <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">Word</MenuPrimitive.Item> | |
| <MenuPrimitive.Root> | |
| <MenuPrimitive.Trigger className="px-4 py-2 hover:bg-gray-100 cursor-pointer rounded">Advanced ›</MenuPrimitive.Trigger> | |
| <MenuPrimitive.Content className="flex flex-col mt-2 bg-gray-100 border border-gray-200 rounded shadow min-w-[140px]"> | |
| <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-200 cursor-pointer rounded">With Images</MenuPrimitive.Item> | |
| <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-200 cursor-pointer rounded">Text Only</MenuPrimitive.Item> | |
| <MenuPrimitive.Item className="px-4 py-2 hover:bg-gray-200 cursor-pointer rounded">Custom Format</MenuPrimitive.Item> |
🤖 Prompt for AI Agents
In src/core/primitives/Menu/stories/MenuPrimitive.stories.tsx between lines 20 and 34, all MenuPrimitive.Item components have the identical label "item 1", which reduces clarity and usability. Update each MenuPrimitive.Item label to be unique and descriptive, reflecting their position or purpose in the menu hierarchy to improve readability and demonstration quality. There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/core/primitives/Menu/fragments/MenuPrimitiveTrigger.tsx (1)
17-27: Add missing ARIA attributes for menu accessibility.Based on the past review comments and accessibility best practices, the menu trigger is missing essential ARIA attributes. While
getReferenceProps()provides event handlers, it doesn't include menu-specific ARIA attributes.Add
aria-haspopup="menu"andaria-expanded={isOpen}to meet accessibility standards:<button className={className} + aria-haspopup="menu" + aria-expanded={isOpen} onClick={() => setIsOpen(!isOpen)} tabIndex={ !isNested ? undefined : activeIndex === item.index ? 0 : -1 } ref={Floater.useMergeRefs([refs.setReference, ref])} {...getReferenceProps()} >This ensures screen readers can properly announce the menu's presence and state.
🧹 Nitpick comments (1)
src/core/primitives/Menu/fragments/MenuPrimitiveTrigger.tsx (1)
20-22: Simplify the tabIndex logic for better readability.The current ternary expression for
tabIndexcan be simplified and made more readable.tabIndex={ - !isNested ? undefined : activeIndex === item.index ? 0 : -1 + isNested ? (activeIndex === item.index ? 0 : -1) : undefined }This maintains the same logic but follows a more conventional pattern where the condition being checked (
isNested) comes first.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/core/primitives/Floater/index.tsx(1 hunks)src/core/primitives/Menu/fragments/MenuPrimitiveItem.tsx(1 hunks)src/core/primitives/Menu/fragments/MenuPrimitiveRoot.tsx(1 hunks)src/core/primitives/Menu/fragments/MenuPrimitiveTrigger.tsx(1 hunks)src/core/primitives/Menu/stories/MenuPrimitive.stories.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/core/primitives/Floater/index.tsx
- src/core/primitives/Menu/stories/MenuPrimitive.stories.tsx
- src/core/primitives/Menu/fragments/MenuPrimitiveItem.tsx
- src/core/primitives/Menu/fragments/MenuPrimitiveRoot.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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`. src/core/primitives/Menu/fragments/MenuPrimitiveTrigger.tsx (5)
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.
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: 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.
🧬 Code Graph Analysis (1)
src/core/primitives/Menu/fragments/MenuPrimitiveTrigger.tsx (1)
src/core/primitives/Select/fragments/SelectPrimitiveTrigger.tsx (1)
SelectPrimitiveTrigger(9-22)
🪛 Biome (2.1.2)
src/core/primitives/Menu/fragments/MenuPrimitiveTrigger.tsx
[error] 14-14: 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] 23-23: 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 (1)
src/core/primitives/Menu/fragments/MenuPrimitiveTrigger.tsx (1)
1-31: Component follows established patterns correctly.The
MenuPrimitiveTriggercomponent follows the same architectural patterns as other primitive triggers in the codebase (SelectPrimitiveTrigger, DialogPrimitiveTrigger, etc.):
- Uses context to access shared state and handlers
- Toggles open state on click
- Merges refs appropriately
- Handles conditional rendering for missing context
The TypeScript typing has been properly addressed based on the past review comments, and the component integrates well with the Floater utilities.
| const context = useContext(MenuPrimitiveRootContext); | ||
| if (!context) return null; | ||
| const { isOpen, setIsOpen, activeIndex, refs, floatingStyles, getReferenceProps, isNested, item } = context; | ||
| const { ref, index } = Floater.useListItem(); |
There was a problem hiding this comment.
Fix React hooks rule violation by moving hooks before conditional return.
The static analysis correctly identifies that useListItem() and useMergeRefs() are called after a conditional early return, violating the Rules of Hooks. All hooks must be called unconditionally and in the same order on every render.
Move the hook calls before the context check:
const MenuPrimitiveTrigger = ({ children, className }: MenuPrimitiveTriggerProps) => { + const { ref, index } = Floater.useListItem(); const context = useContext(MenuPrimitiveRootContext); if (!context) return null; const { isOpen, setIsOpen, activeIndex, refs, floatingStyles, getReferenceProps, isNested, item } = context; - const { ref, index } = Floater.useListItem(); ref={Floater.useMergeRefs([refs.setReference, ref])}This ensures hooks are called unconditionally while maintaining the same functionality.
🧰 Tools
🪛 Biome (2.1.2)
[error] 14-14: 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 11 to 14, the hooks useListItem() and useMergeRefs() are called after a conditional return, violating React's Rules of Hooks. To fix this, move all hook calls, including useListItem() and useMergeRefs(), before the conditional check for context. This ensures hooks are called unconditionally and in the same order on every render, preventing the violation. There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/core/primitives/Menu/fragments/MenuPrimitiveTrigger.tsx (2)
10-14: Fix React hooks rule violation by moving hooks before conditional return.The static analysis correctly identifies that
useListItem()is called after a conditional early return, violating the Rules of Hooks. All hooks must be called unconditionally and in the same order on every render.Move the hook calls before the context check:
const MenuPrimitiveTrigger = ({ children, className }: MenuPrimitiveTriggerProps) => { + const { ref, index } = Floater.useListItem(); const context = useContext(MenuPrimitiveRootContext); if (!context) return null; const { isOpen, setIsOpen, activeIndex, refs, floatingStyles, getReferenceProps, isNested } = context; - const { ref, index } = Floater.useListItem();
17-28: Add missing ARIA attributes for accessibility.Comparing with similar trigger components in the codebase (like
SelectPrimitiveTriggerandCollapsiblePrimitiveTrigger), this menu trigger is missing essential accessibility attributes. Menu triggers should havearia-haspopupandaria-expandedattributes.Add the missing ARIA attributes:
<button className={className} + aria-haspopup="menu" + aria-expanded={isOpen} onClick={() => setIsOpen(!isOpen)} tabIndex={ !isNested ? undefined : activeIndex === index ? 0 : -1 } ref={Floater.useMergeRefs([refs.setReference, ref])} {...getReferenceProps()} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/core/primitives/Menu/MenuPrimitive.tsx(1 hunks)src/core/primitives/Menu/contexts/MenuPrimitiveRootContext.tsx(1 hunks)src/core/primitives/Menu/fragments/MenuPrimitiveContent.tsx(1 hunks)src/core/primitives/Menu/fragments/MenuPrimitiveItem.tsx(1 hunks)src/core/primitives/Menu/fragments/MenuPrimitivePortal.tsx(1 hunks)src/core/primitives/Menu/fragments/MenuPrimitiveRoot.tsx(1 hunks)src/core/primitives/Menu/fragments/MenuPrimitiveSub.tsx(1 hunks)src/core/primitives/Menu/fragments/MenuPrimitiveTrigger.tsx(1 hunks)src/core/primitives/Menu/stories/MenuPrimitive.stories.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/core/primitives/Menu/fragments/MenuPrimitiveSub.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- src/core/primitives/Menu/fragments/MenuPrimitiveContent.tsx
- src/core/primitives/Menu/stories/MenuPrimitive.stories.tsx
- src/core/primitives/Menu/contexts/MenuPrimitiveRootContext.tsx
- src/core/primitives/Menu/MenuPrimitive.tsx
🧰 Additional context used
🧠 Learnings (4)
src/core/primitives/Menu/fragments/MenuPrimitivePortal.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/MenuPrimitiveItem.tsx (3)
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.
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/MenuPrimitiveRoot.tsx (5)
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.
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: 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.
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/MenuPrimitiveTrigger.tsx (5)
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.
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: 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.
🧬 Code Graph Analysis (2)
src/core/primitives/Menu/fragments/MenuPrimitivePortal.tsx (1)
src/core/primitives/Select/fragments/SelectPrimitivePortal.tsx (1)
SelectPrimitivePortal(6-24)
src/core/primitives/Menu/fragments/MenuPrimitiveTrigger.tsx (1)
src/core/primitives/Select/fragments/SelectPrimitiveTrigger.tsx (1)
SelectPrimitiveTrigger(9-22)
🪛 Biome (2.1.2)
src/core/primitives/Menu/fragments/MenuPrimitivePortal.tsx
[error] 10-10: 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] 13-13: 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/core/primitives/Menu/fragments/MenuPrimitiveItem.tsx
[error] 15-15: 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] 18-18: 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/core/primitives/Menu/fragments/MenuPrimitiveTrigger.tsx
[error] 14-14: 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] 24-24: 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 (8)
src/core/primitives/Menu/fragments/MenuPrimitiveTrigger.tsx (2)
1-8: LGTM! Type definitions are properly implemented.The imports and TypeScript interface are well-defined, addressing the previous typing concerns.
31-31: LGTM! Export statement is correct.Standard default export implementation.
src/core/primitives/Menu/fragments/MenuPrimitiveRoot.tsx (3)
1-12: LGTM! Clean imports and well-defined types.The imports are minimal and appropriate, and the
MenuPrimitiveRootPropsinterface properly defines all necessary props with correct TypeScript types, including support for the controllable state pattern.
14-115: Excellent implementation of menu primitive root logic.The component demonstrates solid architecture with:
- Proper controllable state management using
useControllableState- Comprehensive floating UI integration with appropriate hooks for navigation, interactions, and positioning
- Clean event handling with proper cleanup in the
useEffect- Well-structured context value providing all necessary state and methods to descendants
The nested menu detection and conditional placement logic is particularly well implemented.
117-126: Clean wrapper component with proper separation of concerns.The wrapper pattern effectively separates the floating tree context setup from the individual menu logic, enabling proper nesting support while keeping the implementation clean and focused.
src/core/primitives/Menu/fragments/MenuPrimitiveItem.tsx (2)
22-32: LGTM! Proper accessibility and event handling implementation.The button implementation correctly includes:
- Dynamic
tabIndexfor keyboard navigation management- Proper event handling through
getItemProps- Tree event emission for menu interaction coordination
The accessibility attributes and click handler requested in previous reviews have been properly implemented.
5-9: Well-defined TypeScript interface.The
MenuPrimitiveItemPropsinterface properly addresses the previous typing concerns by replacinganywith specific types forchildren,className, and the optionallabelprop.src/core/primitives/Menu/fragments/MenuPrimitivePortal.tsx (1)
19-24: LGTM! Portal implementation follows established patterns.The conditional rendering logic and
Floater.Portalusage is consistent with other portal components in the codebase likeSelectPrimitivePortalandDialogPrimitivePortal.
| const MenuPrimitiveItem = ({ children, className, label }: MenuPrimitiveItemProps) => { | ||
| const context = React.useContext(MenuPrimitiveRootContext); | ||
| if (!context) return null; | ||
| const { activeIndex, getItemProps } = context; | ||
| const { ref, index } = Floater.useListItem({ | ||
| label | ||
| }); | ||
| const tree = Floater.useFloatingTree(); | ||
| const isActive = activeIndex === index; |
There was a problem hiding this comment.
Fix React hooks ordering violation.
The early return on line 13 violates the Rules of Hooks, as subsequent hooks (useListItem and useFloatingTree) are called conditionally. All hooks must be called in the same order on every render.
Move the hooks before the conditional check:
const MenuPrimitiveItem = ({ children, className, label }: MenuPrimitiveItemProps) => { const context = React.useContext(MenuPrimitiveRootContext); - if (!context) return null; - const { activeIndex, getItemProps } = context; const { ref, index } = Floater.useListItem({ label }); const tree = Floater.useFloatingTree(); + + if (!context) return null; + const { activeIndex, getItemProps } = context; const isActive = activeIndex === index;📝 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 MenuPrimitiveItem = ({ children, className, label }: MenuPrimitiveItemProps) => { | |
| const context = React.useContext(MenuPrimitiveRootContext); | |
| if (!context) return null; | |
| const { activeIndex, getItemProps } = context; | |
| const { ref, index } = Floater.useListItem({ | |
| label | |
| }); | |
| const tree = Floater.useFloatingTree(); | |
| const isActive = activeIndex === index; | |
| const MenuPrimitiveItem = ({ children, className, label }: MenuPrimitiveItemProps) => { | |
| const context = React.useContext(MenuPrimitiveRootContext); | |
| const { ref, index } = Floater.useListItem({ | |
| label | |
| }); | |
| const tree = Floater.useFloatingTree(); | |
| if (!context) return null; | |
| const { activeIndex, getItemProps } = context; | |
| const isActive = activeIndex === index; | |
| // …rest of implementation |
🧰 Tools
🪛 Biome (2.1.2)
[error] 15-15: 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] 18-18: 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/MenuPrimitiveItem.tsx around lines 11 to 19, the React hooks useListItem and useFloatingTree are called conditionally after an early return, violating the Rules of Hooks. To fix this, move the calls to these hooks before the conditional check on context so that all hooks are invoked unconditionally and in the same order on every render. | function MenuPrimitivePortal({ children }: { children: React.ReactNode }) { | ||
| const context = useContext(MenuPrimitiveRootContext); | ||
| if (!context) return null; | ||
| const { isOpen } = context; | ||
| const [rootElementFound, setRootElementFound] = useState(false); | ||
| const rootElement = document.querySelector('#rad-ui-theme-container') || document.body as HTMLElement | null; | ||
| | ||
| useEffect(() => { | ||
| if (rootElement) { | ||
| setRootElementFound(true); | ||
| } | ||
| }, [rootElement]); |
There was a problem hiding this comment.
Fix hooks rule violation by moving hooks before conditional returns.
The component violates React's rules of hooks by calling useState and useEffect after a conditional early return. All hooks must be called in the same order on every render.
Apply this diff to fix the hook ordering:
function MenuPrimitivePortal({ children }: { children: React.ReactNode }) { + const [rootElementFound, setRootElementFound] = useState(false); + const rootElement = document.querySelector('#rad-ui-theme-container') || document.body as HTMLElement | null; + + useEffect(() => { + if (rootElement) { + setRootElementFound(true); + } + }, [rootElement]); + const context = useContext(MenuPrimitiveRootContext); if (!context) return null; const { isOpen } = context; - const [rootElementFound, setRootElementFound] = useState(false); - const rootElement = document.querySelector('#rad-ui-theme-container') || document.body as HTMLElement | null; - - useEffect(() => { - if (rootElement) { - setRootElementFound(true); - } - }, [rootElement]);🧰 Tools
🪛 Biome (2.1.2)
[error] 10-10: 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] 13-13: 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/MenuPrimitivePortal.tsx lines 6 to 17, hooks like useState and useEffect are called after an early return caused by a missing context, violating React's rules of hooks. To fix this, move all hook calls (useContext, useState, useEffect) to the top of the component before any conditional returns, ensuring hooks are always called in the same order on every render. 
kotAPI added the automerge 
kodiakhq bot removed the automerge 
| This PR currently has a merge conflict. Please resolve this and then re-add the |
| already merged with dropdown menu |
Summary by CodeRabbit
New Features
Enhancements