Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughA new Menubar component is introduced that wraps multiple Menu instances, providing context-aware rendering for MenuTrigger elements. The component supports horizontal and vertical orientations, integrates with the existing Menu component system, and includes comprehensive documentation, examples, and test coverage across the monorepo. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
packages/raystack/components/menubar/__tests__/menubar.test.tsx (1)
49-63: Consider usingcreateReffor cleaner ref testing.The current type assertion pattern is unconventional. Using
React.createRef()provides cleaner test code:♻️ Proposed refactor
+ import { createRef } from 'react'; // ... it('forwards ref', () => { - const ref = { current: null } as unknown as React.RefObject<HTMLDivElement>; + const ref = createRef<HTMLDivElement>(); render( <Menubar ref={ref}> <Menu> <Menu.Trigger>File</Menu.Trigger> <Menu.Content> <Menu.Item>New</Menu.Item> </Menu.Content> </Menu> </Menubar> ); expect(ref.current).toBeInstanceOf(HTMLDivElement); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/menubar/__tests__/menubar.test.tsx` around lines 49 - 63, The test "forwards ref" uses an unconventional cast for the ref; replace the manual cast with React.createRef<HTMLDivElement>() to create a real RefObject and pass that to the Menubar component in the test (update the ref declaration used in the it('forwards ref') block). Ensure the test still renders the same Menu/Item tree and keeps the assertion expect(ref.current).toBeInstanceOf(HTMLDivElement); (update any imports if React.createRef is not currently available).packages/raystack/components/menu/menu-trigger.tsx (1)
14-19: Consider explicitly declaringrenderinMenuTriggerPropsfor clarity.The
renderprop is destructured on line 19 but not explicitly declared inMenuTriggerProps. It's inherited fromMenuPrimitive.Trigger.Props, which is correct, but adding it explicitly would improve discoverability and allow for custom JSDoc documentation specific to the menubar behavior.📝 Optional: Explicitly declare render prop
+import { ReactElement } from 'react'; + export interface MenuTriggerProps extends MenuPrimitive.Trigger.Props { stopPropagation?: boolean; + /** + * Custom render element. When provided, overrides the default menubar trigger styling. + */ + render?: ReactElement; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/menu/menu-trigger.tsx` around lines 14 - 19, MenuTrigger destructures a render prop but MenuTriggerProps does not explicitly declare it; add an explicit optional render property to MenuTriggerProps (matching the type from MenuPrimitive.Trigger.Props) so it's discoverable and can have custom JSDoc, then update MenuTriggerProps interface to include render?: /* appropriate type */ and ensure MenuTrigger consumer code still compiles; refer to MenuTriggerProps and MenuTrigger where render is currently destructured.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/www/src/content/docs/components/menubar/props.ts`:
- Around line 3-33: Update the MenubarProps interface to add a style prop and
widen className and render to accept stateful function overloads: add style?:
React.CSSProperties | ((state: Menubar.State) => React.CSSProperties |
undefined), change className?: string to className?: string | ((state:
Menubar.State) => string | undefined), and change render?: ReactElement to
render?: ReactElement | ((props: React.HTMLProps<HTMLElement>, state:
Menubar.State) => ReactElement); keep the existing optional semantics and
reference the MenubarProps interface and Menubar.State type when making these
edits.
In `@packages/raystack/components/menubar/__tests__/menubar.test.tsx`:
- Around line 81-95: The test in menubar.test.tsx incorrectly expects the class
'rs-menu-trigger' which doesn't exist; update the assertion in the "menu trigger
skips rs-menu-trigger className when render prop is present" test to assert
against the actual CSS module class name used by Menu.Trigger (e.g., replace
'rs-menu-trigger' with the real class pattern produced by the CSS module for
Menu.Trigger), by locating the test block that renders <Menubar><Menu.Trigger
render={...}>File</Menu.Trigger>...</Menubar> and changing the
expect(trigger).not.toHaveClass(...) call to use the correct class string for
Menu.Trigger.
- Around line 65-79: The test expects a static class "rs-menu-trigger" but the
component (Menu.Trigger in menu-trigger.tsx) uses CSS modules
(styles.menuBarTrigger) which produces hashed class names; update the test in
menubar.test.tsx to assert against the actual CSS-module class instead of
"rs-menu-trigger" (e.g., import the CSS module used by Menu.Trigger and expect
the trigger element to have styles.menuBarTrigger), or alternatively change the
assertion to check behavior/structure (e.g., that the element is inside
<Menubar> or has the expected role/attributes) rather than a hard-coded class
name.
In `@packages/raystack/components/menubar/menubar.tsx`:
- Around line 1-28: Update the import to pull Menubar from the menubar entry
(replace import from '@base-ui/react' to '@base-ui/react/menubar') and adjust
MenubarRoot's className handling so it supports both string and function forms:
if className is a function, pass a new function to MenubarPrimitive that calls
the user function with the Menubar state and returns cx(styles.menubar,
userResult) (or styles.menubar when userResult is undefined); if className is a
string or undefined, continue using cx(styles.menubar, className) as before;
keep the MenubarContext, MenubarRoot, and displayName unchanged.
---
Nitpick comments:
In `@packages/raystack/components/menu/menu-trigger.tsx`:
- Around line 14-19: MenuTrigger destructures a render prop but MenuTriggerProps
does not explicitly declare it; add an explicit optional render property to
MenuTriggerProps (matching the type from MenuPrimitive.Trigger.Props) so it's
discoverable and can have custom JSDoc, then update MenuTriggerProps interface
to include render?: /* appropriate type */ and ensure MenuTrigger consumer code
still compiles; refer to MenuTriggerProps and MenuTrigger where render is
currently destructured.
In `@packages/raystack/components/menubar/__tests__/menubar.test.tsx`:
- Around line 49-63: The test "forwards ref" uses an unconventional cast for the
ref; replace the manual cast with React.createRef<HTMLDivElement>() to create a
real RefObject and pass that to the Menubar component in the test (update the
ref declaration used in the it('forwards ref') block). Ensure the test still
renders the same Menu/Item tree and keeps the assertion
expect(ref.current).toBeInstanceOf(HTMLDivElement); (update any imports if
React.createRef is not currently available).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 63349035-e98b-49cd-a4c4-a8dfe111f3f2
📒 Files selected for processing (12)
apps/www/src/components/playground/index.tsapps/www/src/components/playground/menubar-examples.tsxapps/www/src/content/docs/components/menubar/demo.tsapps/www/src/content/docs/components/menubar/index.mdxapps/www/src/content/docs/components/menubar/props.tspackages/raystack/components/menu/menu-trigger.tsxpackages/raystack/components/menu/menu.module.csspackages/raystack/components/menubar/__tests__/menubar.test.tsxpackages/raystack/components/menubar/index.tspackages/raystack/components/menubar/menubar.module.csspackages/raystack/components/menubar/menubar.tsxpackages/raystack/index.tsx
| export interface MenubarProps { | ||
| /** | ||
| * Whether the menubar is modal. | ||
| * @defaultValue true | ||
| */ | ||
| modal?: boolean; | ||
|
|
||
| /** | ||
| * Whether the whole menubar is disabled. | ||
| * @defaultValue false | ||
| */ | ||
| disabled?: boolean; | ||
|
|
||
| /** | ||
| * The orientation of the menubar. | ||
| * @defaultValue "horizontal" | ||
| */ | ||
| orientation?: 'horizontal' | 'vertical'; | ||
|
|
||
| /** | ||
| * Whether to loop keyboard focus back to the first item when the end of the list is reached. | ||
| * @defaultValue true | ||
| */ | ||
| loopFocus?: boolean; | ||
|
|
||
| /** Additional CSS class names */ | ||
| className?: string; | ||
|
|
||
| /** Replaces the rendered element with a custom one via Base UI's render prop pattern */ | ||
| render?: ReactElement; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
@base-ui/react Menubar component props API
💡 Result:
@base-ui/react/menubar exports a single <Menubar /> root component. Its props API is: (base-ui.com)
loopFocus?: boolean(default:true) — Loop arrow-key focus from last item back to first.modal?: boolean(default:true) — Whether the menubar is modal.disabled?: boolean(default:false) — Disables the whole menubar.orientation?: 'horizontal' | 'vertical'(default:'horizontal') — Menubar orientation.className?: string | ((state: Menubar.State) => string | undefined)— Static or state-based class.style?: React.CSSProperties | ((state: Menubar.State) => React.CSSProperties | undefined)— Static or state-based inline styles.render?: ReactElement | ((props: HTMLProps, state: Menubar.State) => ReactElement)— Replace/compose the underlying element.
Citations:
Add missing style prop and complete type signatures for className and render.
The documented props are incomplete. @base-ui/react's Menubar supports:
classNameandrenderwith function overloads for state-based dynamic values:className?: string | ((state: Menubar.State) => string | undefined)andrender?: ReactElement | ((props: HTMLProps, state: Menubar.State) => ReactElement)styleprop (completely missing from documentation):style?: React.CSSProperties | ((state: Menubar.State) => React.CSSProperties | undefined)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/www/src/content/docs/components/menubar/props.ts` around lines 3 - 33,
Update the MenubarProps interface to add a style prop and widen className and
render to accept stateful function overloads: add style?: React.CSSProperties |
((state: Menubar.State) => React.CSSProperties | undefined), change className?:
string to className?: string | ((state: Menubar.State) => string | undefined),
and change render?: ReactElement to render?: ReactElement | ((props:
React.HTMLProps<HTMLElement>, state: Menubar.State) => ReactElement); keep the
existing optional semantics and reference the MenubarProps interface and
Menubar.State type when making these edits.
| it('menu trigger has rs-menu-trigger className when inside menubar', () => { | ||
| render( | ||
| <Menubar> | ||
| <Menu> | ||
| <Menu.Trigger>File</Menu.Trigger> | ||
| <Menu.Content> | ||
| <Menu.Item>New</Menu.Item> | ||
| </Menu.Content> | ||
| </Menu> | ||
| </Menubar> | ||
| ); | ||
|
|
||
| const trigger = screen.getByText('File'); | ||
| expect(trigger).toHaveClass('rs-menu-trigger'); | ||
| }); |
There was a problem hiding this comment.
Test assertion expects non-existent class name.
The test is failing because it expects rs-menu-trigger class, but the actual implementation uses CSS modules which generate hashed class names (e.g., _menuBarTrigger_5f8152).
Based on the context snippet from menu-trigger.tsx, the MenuTrigger uses styles.menuBarTrigger from CSS modules, not a static rs-menu-trigger class.
Either update the test to check for the correct behavior or add the expected class in the component implementation:
🐛 Option 1: Fix the test to match actual CSS module behavior
it('menu trigger has rs-menu-trigger className when inside menubar', () => {
render(
<Menubar>
<Menu>
<Menu.Trigger>File</Menu.Trigger>
<Menu.Content>
<Menu.Item>New</Menu.Item>
</Menu.Content>
</Menu>
</Menubar>
);
const trigger = screen.getByText('File');
- expect(trigger).toHaveClass('rs-menu-trigger');
+ // Check that the trigger renders as a Button with menubar-specific styling
+ expect(trigger).toHaveAttribute('type', 'button');
+ expect(trigger.className).toMatch(/menuBarTrigger/);
});🐛 Option 2: Rename the test to reflect the actual behavior being tested
- it('menu trigger has rs-menu-trigger className when inside menubar', () => {
+ it('menu trigger renders with menuBarTrigger class when inside menubar', () => {
render(
<Menubar>
<Menu>
<Menu.Trigger>File</Menu.Trigger>
<Menu.Content>
<Menu.Item>New</Menu.Item>
</Menu.Content>
</Menu>
</Menubar>
);
const trigger = screen.getByText('File');
- expect(trigger).toHaveClass('rs-menu-trigger');
+ expect(trigger.className).toMatch(/menuBarTrigger/);
});📝 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.
| it('menu trigger has rs-menu-trigger className when inside menubar', () => { | |
| render( | |
| <Menubar> | |
| <Menu> | |
| <Menu.Trigger>File</Menu.Trigger> | |
| <Menu.Content> | |
| <Menu.Item>New</Menu.Item> | |
| </Menu.Content> | |
| </Menu> | |
| </Menubar> | |
| ); | |
| const trigger = screen.getByText('File'); | |
| expect(trigger).toHaveClass('rs-menu-trigger'); | |
| }); | |
| it('menu trigger has rs-menu-trigger className when inside menubar', () => { | |
| render( | |
| <Menubar> | |
| <Menu> | |
| <Menu.Trigger>File</Menu.Trigger> | |
| <Menu.Content> | |
| <Menu.Item>New</Menu.Item> | |
| </Menu.Content> | |
| </Menu> | |
| </Menubar> | |
| ); | |
| const trigger = screen.getByText('File'); | |
| // Check that the trigger renders as a Button with menubar-specific styling | |
| expect(trigger).toHaveAttribute('type', 'button'); | |
| expect(trigger.className).toMatch(/menuBarTrigger/); | |
| }); |
| it('menu trigger has rs-menu-trigger className when inside menubar', () => { | |
| render( | |
| <Menubar> | |
| <Menu> | |
| <Menu.Trigger>File</Menu.Trigger> | |
| <Menu.Content> | |
| <Menu.Item>New</Menu.Item> | |
| </Menu.Content> | |
| </Menu> | |
| </Menubar> | |
| ); | |
| const trigger = screen.getByText('File'); | |
| expect(trigger).toHaveClass('rs-menu-trigger'); | |
| }); | |
| it('menu trigger renders with menuBarTrigger class when inside menubar', () => { | |
| render( | |
| <Menubar> | |
| <Menu> | |
| <Menu.Trigger>File</Menu.Trigger> | |
| <Menu.Content> | |
| <Menu.Item>New</Menu.Item> | |
| </Menu.Content> | |
| </Menu> | |
| </Menubar> | |
| ); | |
| const trigger = screen.getByText('File'); | |
| expect(trigger.className).toMatch(/menuBarTrigger/); | |
| }); |
🧰 Tools
🪛 GitHub Actions: Tests
[error] 78-79: Test failed: expected element to have class 'rs-menu-trigger' (found a different className).
🪛 GitHub Check: test-and-lint (22.x)
[failure] 78-78: components/menubar/tests/menubar.test.tsx > Menubar > menu trigger has rs-menu-trigger className when inside menubar
Error: expect(element).toHaveClass("rs-menu-trigger")
Expected the element to have class:
rs-menu-trigger
Received:
_button_663cf8 _button-text_663cf8 _button-small_663cf8 _button-color-neutral_663cf8 _button-text-neutral_663cf8 _menuBarTrigger_5f8152
❯ components/menubar/tests/menubar.test.tsx:78:21
🪛 GitHub Check: test-and-lint (24.x)
[failure] 78-78: components/menubar/tests/menubar.test.tsx > Menubar > menu trigger has rs-menu-trigger className when inside menubar
Error: expect(element).toHaveClass("rs-menu-trigger")
Expected the element to have class:
rs-menu-trigger
Received:
_button_663cf8 _button-text_663cf8 _button-small_663cf8 _button-color-neutral_663cf8 _button-text-neutral_663cf8 _menuBarTrigger_5f8152
❯ components/menubar/tests/menubar.test.tsx:78:21
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/menubar/__tests__/menubar.test.tsx` around lines
65 - 79, The test expects a static class "rs-menu-trigger" but the component
(Menu.Trigger in menu-trigger.tsx) uses CSS modules (styles.menuBarTrigger)
which produces hashed class names; update the test in menubar.test.tsx to assert
against the actual CSS-module class instead of "rs-menu-trigger" (e.g., import
the CSS module used by Menu.Trigger and expect the trigger element to have
styles.menuBarTrigger), or alternatively change the assertion to check
behavior/structure (e.g., that the element is inside <Menubar> or has the
expected role/attributes) rather than a hard-coded class name.
| it('menu trigger skips rs-menu-trigger className when render prop is present', () => { | ||
| render( | ||
| <Menubar> | ||
| <Menu> | ||
| <Menu.Trigger render={<div />}>File</Menu.Trigger> | ||
| <Menu.Content> | ||
| <Menu.Item>New</Menu.Item> | ||
| </Menu.Content> | ||
| </Menu> | ||
| </Menubar> | ||
| ); | ||
|
|
||
| const trigger = screen.getByText('File'); | ||
| expect(trigger).not.toHaveClass('rs-menu-trigger'); | ||
| }); |
There was a problem hiding this comment.
Same class name issue in render prop test.
This test also references rs-menu-trigger which doesn't exist. Update to match the actual CSS module class pattern:
🐛 Proposed fix
it('menu trigger skips rs-menu-trigger className when render prop is present', () => {
- it('menu trigger skips rs-menu-trigger className when render prop is present', () => {
+ it('menu trigger skips menuBarTrigger class when render prop is present', () => {
render(
<Menubar>
<Menu>
<Menu.Trigger render={<div />}>File</Menu.Trigger>
<Menu.Content>
<Menu.Item>New</Menu.Item>
</Menu.Content>
</Menu>
</Menubar>
);
const trigger = screen.getByText('File');
- expect(trigger).not.toHaveClass('rs-menu-trigger');
+ expect(trigger.className).not.toMatch(/menuBarTrigger/);
});📝 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.
| it('menu trigger skips rs-menu-trigger className when render prop is present', () => { | |
| render( | |
| <Menubar> | |
| <Menu> | |
| <Menu.Trigger render={<div />}>File</Menu.Trigger> | |
| <Menu.Content> | |
| <Menu.Item>New</Menu.Item> | |
| </Menu.Content> | |
| </Menu> | |
| </Menubar> | |
| ); | |
| const trigger = screen.getByText('File'); | |
| expect(trigger).not.toHaveClass('rs-menu-trigger'); | |
| }); | |
| it('menu trigger skips menuBarTrigger class when render prop is present', () => { | |
| render( | |
| <Menubar> | |
| <Menu> | |
| <Menu.Trigger render={<div />}>File</Menu.Trigger> | |
| <Menu.Content> | |
| <Menu.Item>New</Menu.Item> | |
| </Menu.Content> | |
| </Menu> | |
| </Menubar> | |
| ); | |
| const trigger = screen.getByText('File'); | |
| expect(trigger.className).not.toMatch(/menuBarTrigger/); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/menubar/__tests__/menubar.test.tsx` around lines
81 - 95, The test in menubar.test.tsx incorrectly expects the class
'rs-menu-trigger' which doesn't exist; update the assertion in the "menu trigger
skips rs-menu-trigger className when render prop is present" test to assert
against the actual CSS module class name used by Menu.Trigger (e.g., replace
'rs-menu-trigger' with the real class pattern produced by the CSS module for
Menu.Trigger), by locating the test block that renders <Menubar><Menu.Trigger
render={...}>File</Menu.Trigger>...</Menubar> and changing the
expect(trigger).not.toHaveClass(...) call to use the correct class string for
Menu.Trigger.
| 'use client'; | ||
|
|
||
| import { Menubar as MenubarPrimitive } from '@base-ui/react'; | ||
| import { cx } from 'class-variance-authority'; | ||
| import { createContext, ElementRef, forwardRef, useContext } from 'react'; | ||
| import styles from './menubar.module.css'; | ||
|
|
||
| const MenubarContext = createContext(false); | ||
|
|
||
| export function useMenubarContext() { | ||
| return useContext(MenubarContext); | ||
| } | ||
|
|
||
| const MenubarRoot = forwardRef< | ||
| ElementRef<typeof MenubarPrimitive>, | ||
| MenubarPrimitive.Props | ||
| >(({ className, ...props }, ref) => ( | ||
| <MenubarContext.Provider value={true}> | ||
| <MenubarPrimitive | ||
| ref={ref} | ||
| className={cx(styles.menubar, className)} | ||
| {...props} | ||
| /> | ||
| </MenubarContext.Provider> | ||
| )); | ||
| MenubarRoot.displayName = 'Menubar'; | ||
|
|
||
| export const Menubar = MenubarRoot; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
@base-ui/react Menubar component API 2025
💡 Result:
Base UI’s Menubar was introduced in v1.0.0-beta.0 (Apr 17, 2025). [2]
Import
import { Menubar } from '@base-ui/react/menubar';
import { Menu } from '@base-ui/react/menu';[1]
Composition (structure)
<Menubar> wraps one or more <Menu.Root> menus (each with its own <Menu.Trigger>, <Menu.Portal>, <Menu.Positioner>, <Menu.Popup>, items, etc.). [1]
<Menubar /> props (API)
loopFocus?: boolean(defaulttrue) — loop arrow-key focus at the ends. [1]modal?: boolean(defaulttrue) — whether the menubar is modal. [1]disabled?: boolean(defaultfalse) — disables the whole menubar. [1]orientation?: 'horizontal' | 'vertical'(default'horizontal'). [1]className?: string | ((state: Menubar.State) => string | undefined)[1]style?: React.CSSProperties | ((state: Menubar.State) => CSSProperties | undefined)[1]render?: ReactElement | ((props: HTMLProps, state: Menubar.State) => ReactElement)— replace/compose the underlying element. [1]
Sources: Base UI Menubar docs [1], Base UI releases (Menubar added) [2].
Fix className handling to support function-based style computation from Base UI.
The implementation has two significant issues:
-
Import path: Should import from
@base-ui/react/menubarper official Base UI documentation, not from@base-ui/react. -
className prop handling: Base UI's Menubar API accepts
classNameas either a string OR a function(state: Menubar.State) => string | undefined. The current implementation destructures className and passes it directly tocx():className={cx(styles.menubar, className)}
This breaks when consumers pass a function for className, as
cx()cannot process functions. The className prop must be handled conditionally to support both string and function forms, or passed through without merging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/menubar/menubar.tsx` around lines 1 - 28, Update
the import to pull Menubar from the menubar entry (replace import from
'@base-ui/react' to '@base-ui/react/menubar') and adjust MenubarRoot's className
handling so it supports both string and function forms: if className is a
function, pass a new function to MenubarPrimitive that calls the user function
with the Menubar state and returns cx(styles.menubar, userResult) (or
styles.menubar when userResult is undefined); if className is a string or
undefined, continue using cx(styles.menubar, className) as before; keep the
MenubarContext, MenubarRoot, and displayName unchanged.
1 participant
Description
Menubarcomponent wrapping multipleMenuinstances in a horizontal/vertical barMenubarContextletsMenu.Triggerauto-render as<Button variant="text" color="neutral" size="small">inside a menubarSummary by CodeRabbit
Release Notes
New Features
Documentation
Tests