Separator for dropdown , context and menubar menus #1588
Conversation
🦋 Changeset detectedLatest commit: 1262db5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughIntroduces Separator subcomponents for ContextMenu, DropdownMenu, and Menubar. Adds corresponding fragments, wires them into the public APIs as static members, updates stories to render separators, and adds stylesheet rules for each menu’s separator variants. Includes a changeset entry for a patch release. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Consumer App
participant CM as Menu Component (Context/Dropdown/Menubar)
participant Sep as *.Separator fragment
participant Ctx as *MenuContext
participant Base as Separator (base UI)
participant DOM as DOM
Dev->>CM: Render Menu structure
Dev->>Sep: Use <Menu.Separator {...props} />
Sep->>Ctx: useContext()
alt Context available
Sep->>Base: render with { customRootClass: ctx.rootClass, ...props }
Base->>DOM: render separator element
else No context
Sep->>Sep: console.warn(...)
Sep-->>Dev: return null
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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. 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: 0
🧹 Nitpick comments (5)
src/components/ui/Menubar/Menubar.tsx (1)
9-9: Expose type forMenubar.Separatorto keep TS IntelliSense consistent with DropdownMenu.
Menubarcurrently augments static members dynamically; consider declaring aMenubarComponenttype (includingSeparator) and casting the export, mirroring theDropdownMenupattern, so consumers get types forMenubar.Separator.Example (outside these lines):
import React, { ComponentPropsWithoutRef, ElementRef, forwardRef } from 'react'; type MenubarComponent = React.FC & { Root: typeof MenubarRoot; Menu: typeof MenubarMenu; Trigger: typeof MenubarTrigger; Content: typeof MenubarContent; Portal: typeof MenubarPortal; Item: typeof MenubarItem; Sub: typeof MenubarSub; SubTrigger: typeof MenubarSubTrigger; Separator: typeof MenubarSeparator; }; // keep existing warn-only impl, just cast: const Menubar = ((..._args: any[]) => { console.warn('Direct usage of Menubar is not supported. Please use Menubar.Root, Menubar.Item instead.'); return null; }) as MenubarComponent;Also applies to: 24-24
src/components/ui/ContextMenu/fragments/ContextMenuSeparator.tsx (1)
4-8: Pass through props and set a displayName for better flexibility/devtools.Forward props to allow styling/variants when used in menus, and set a displayName.
Apply this diff:
-import React from 'react'; +import React, { ComponentPropsWithoutRef } from 'react'; import Separator from '../../Separator/Separator'; -function ContextMenuSeparator() { - return ( - <Separator/> - ); -} +function ContextMenuSeparator(props: ComponentPropsWithoutRef<typeof Separator>) { + return <Separator {...props} />; +} +ContextMenuSeparator.displayName = 'ContextMenu.Separator';Also confirm the shared
Separatorsetsrole="separator"and correct orientation for menu contexts.src/components/ui/DropdownMenu/fragments/DropdownMenuSeparator.tsx (1)
4-8: Mirror: allow prop passthrough and add displayName.Same rationale as ContextMenu separator for customization and DX.
Apply this diff:
-import React from 'react'; +import React, { ComponentPropsWithoutRef } from 'react'; import Separator from '../../Separator/Separator'; -function DropdownMenuSeparator() { - return ( - <Separator/> - ); -} +function DropdownMenuSeparator(props: ComponentPropsWithoutRef<typeof Separator>) { + return <Separator {...props} />; +} +DropdownMenuSeparator.displayName = 'DropdownMenu.Separator';src/components/ui/Menubar/fragments/MenubarSeparator.tsx (1)
4-8: Ditto: forward props + displayName.Keep the API symmetric across all menus.
Apply this diff:
-import React from 'react'; +import React, { ComponentPropsWithoutRef } from 'react'; import Separator from '../../Separator/Separator'; -function MenubarSeparator() { - return ( - <Separator/> - ); -} +function MenubarSeparator(props: ComponentPropsWithoutRef<typeof Separator>) { + return <Separator {...props} />; +} +MenubarSeparator.displayName = 'Menubar.Separator';Additionally, if all three wrappers stay identical, consider re-exporting the base
Separator(alias) to avoid duplicate components.src/components/ui/ContextMenu/ContextMenu.tsx (1)
32-40: Wiring works; consider consolidating statics via Object.assign for type safety and cohesion.Optional, but this avoids piecemeal assignments and keeps the runtime shape in one place.
-const ContextMenu = forwardRef<ContextMenuElement, ContextMenuProps>((_props, _ref) => { - console.warn('Direct usage of ContextMenu is not supported. Please use ContextMenu.Root, ContextMenu.Item instead.'); - return null; -}) as ContextMenuComponent; - -ContextMenu.displayName = 'ContextMenu'; - -ContextMenu.Root = ContextMenuRoot; -ContextMenu.Trigger = ContextMenuTrigger; -ContextMenu.Content = ContextMenuContent; -ContextMenu.Portal = ContextMenuPortal; -ContextMenu.Item = ContextMenuItem; -ContextMenu.Sub = ContextMenuSub; -ContextMenu.SubTrigger = ContextMenuSubTrigger; -ContextMenu.Separator = ContextMenuSeparator; +const ContextMenu = Object.assign( + forwardRef<ContextMenuElement, ContextMenuProps>((_props, _ref) => { + console.warn('Direct usage of ContextMenu is not supported. Please use ContextMenu.Root, ContextMenu.Item instead.'); + return null; + }), + { + Root: ContextMenuRoot, + Trigger: ContextMenuTrigger, + Content: ContextMenuContent, + Portal: ContextMenuPortal, + Item: ContextMenuItem, + Sub: ContextMenuSub, + SubTrigger: ContextMenuSubTrigger, + Separator: ContextMenuSeparator, + } +) as ContextMenuComponent; + +ContextMenu.displayName = 'ContextMenu';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/components/ui/ContextMenu/ContextMenu.tsx(3 hunks)src/components/ui/ContextMenu/fragments/ContextMenuSeparator.tsx(1 hunks)src/components/ui/ContextMenu/stories/ContextMenu.stories.tsx(1 hunks)src/components/ui/DropdownMenu/DropdownMenu.tsx(3 hunks)src/components/ui/DropdownMenu/fragments/DropdownMenuSeparator.tsx(1 hunks)src/components/ui/DropdownMenu/stories/DropdownMenu.stories.tsx(1 hunks)src/components/ui/Menubar/Menubar.tsx(2 hunks)src/components/ui/Menubar/fragments/MenubarSeparator.tsx(1 hunks)src/components/ui/Menubar/stories/Menubar.stories.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-12-12T08:22:59.375Z
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`.
Applied to files:
src/components/ui/DropdownMenu/DropdownMenu.tsx
📚 Learning: 2024-12-12T08:34:33.079Z
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`.
Applied to files:
src/components/ui/DropdownMenu/stories/DropdownMenu.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: coverage
🔇 Additional comments (6)
src/components/ui/DropdownMenu/stories/DropdownMenu.stories.tsx (1)
26-26: Separator usage looks good; please confirm a11y semantics.Ensure the underlying
Separatorrendersrole="separator"and is not focusable inside the menu. Also confirm arrow-key navigation skips the separator.src/components/ui/Menubar/stories/Menubar.stories.tsx (1)
37-37: Nice addition; verify keyboard/nav behavior.Confirm the separator is excluded from the roving focus set and uses proper ARIA for menu separators.
src/components/ui/ContextMenu/stories/ContextMenu.stories.tsx (1)
25-25: Good placement; check a11y.Please verify
role="separator"is applied and the item is non-interactive in the context menu.src/components/ui/DropdownMenu/DropdownMenu.tsx (1)
9-9: API wiring forDropdownMenu.Separatorlooks correct.Import, type surface, and static assignment are consistent with existing patterns.
Please ensure any barrel exports (e.g.,
src/components/ui/index.ts) don’t need updates for the new subcomponent in docs/typing.Also applies to: 22-23, 39-39
src/components/ui/ContextMenu/ContextMenu.tsx (2)
9-9: Import OK — default export present; confirm accessible separator semantics.
- Found export default in src/components/ui/ContextMenu/fragments/ContextMenuSeparator.tsx (export default at line 10).
- No explicit role="separator" found in that file; ensure the fragment renders a semantic, non-focusable separator (add role="separator" or verify the shared applies it).
22-23: Public API extended with Separator — LGTM; confirm docs/stories & barrel exportsType wiring looks correct. Repo search returned "No files were searched", so references couldn't be confirmed; ensure stories/docs and any barrel/index exports expose ContextMenu.Separator.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (13)
styles/themes/components/menubar.scss (2)
173-176: Use a token fallback for separator color.Future-proof the color by allowing an overridable CSS var.
-.rad-ui-menubar-separator { - background-color: var(--rad-ui-color-gray-600); +.rad-ui-menubar-separator { + background-color: var(--rad-ui-color-separator, var(--rad-ui-color-gray-600)); align-self: stretch; }
182-190: Prevent 1px separators from collapsing in flex layouts.Add flex-shrink: 0 to keep the 1px thickness under tight space.
.rad-ui-menubar-separator-vertical{ margin: 0 8px; width: 1px; + flex-shrink: 0; } .rad-ui-menubar-separator-horizontal{ margin: 8px 0; height: 1px; + flex-shrink: 0; }styles/themes/components/dropdown-menu.scss (2)
159-166: Use a token fallback for separator color.Match menubar/context with an overridable token.
-.rad-ui-dropdown-menu-separator { - background-color: var(--rad-ui-color-gray-600); +.rad-ui-dropdown-menu-separator { + background-color: var(--rad-ui-color-separator, var(--rad-ui-color-gray-600)); align-self: stretch; }
168-176: Add flex-shrink: 0 to preserve thickness in flex containers..rad-ui-dropdown-menu-separator-vertical{ margin: 0 8px; width: 1px; + flex-shrink: 0; } .rad-ui-dropdown-menu-separator-horizontal{ margin: 8px 0; height: 1px; + flex-shrink: 0; }src/components/ui/DropdownMenu/fragments/DropdownMenuSeparator.tsx (2)
14-16: Gate console.warn to dev builds.Avoid noisy logs in production.
- if (!context) { - console.warn('DropdownMenuSeparator should be used within DropdownMenuRoot'); - return null; - } + if (!context) { + if (process.env.NODE_ENV !== 'production') { + console.warn('DropdownMenuSeparator should be used within DropdownMenuRoot'); + } + return null; + }
19-20: Remove redundant template literal in clsx.No need to interpolate rootClass.
- <Separator ref={ref} customRootClass={clsx(`${rootClass}`, className)} {...props}/> + <Separator ref={ref} customRootClass={clsx(rootClass, className)} {...props}/>src/components/ui/ContextMenu/fragments/ContextMenuSeparator.tsx (2)
13-15: Gate console.warn to dev builds.- if (!context) { - console.warn('ContextMenuSeparator should be used within ContextMenuRoot'); - return null; - } + if (!context) { + if (process.env.NODE_ENV !== 'production') { + console.warn('ContextMenuSeparator should be used within ContextMenuRoot'); + } + return null; + }
18-18: Remove redundant template literal in clsx.- <Separator ref={ref} customRootClass={clsx(`${rootClass}`, className)} {...props}/> + <Separator ref={ref} customRootClass={clsx(rootClass, className)} {...props}/>styles/themes/components/context-menu.scss (2)
135-142: Use a token fallback for separator color.-.rad-ui-context-menu-separator { - background-color: var(--rad-ui-color-gray-600); +.rad-ui-context-menu-separator { + background-color: var(--rad-ui-color-separator, var(--rad-ui-color-gray-600)); align-self: stretch; }
144-152: Add flex-shrink: 0 to preserve thickness in flex containers..rad-ui-context-menu-separator-vertical{ margin: 0 8px; width: 1px; + flex-shrink: 0; } .rad-ui-context-menu-separator-horizontal{ margin: 8px 0; height: 1px; + flex-shrink: 0; }src/components/ui/Menubar/fragments/MenubarSeparator.tsx (3)
14-15: Gate console warning to dev builds.Prevent noisy server/prod logs.
- console.warn('MenubarSeparator should be used within MenubarRoot'); + if (process.env.NODE_ENV !== 'production') { + // eslint-disable-next-line no-console + console.warn('MenubarSeparator should be used within MenubarRoot'); + }
19-19: Tiny cleanup: drop unnecessary template literal.
clsx(rootClass, className)is sufficient.- <Separator ref={ref} customRootClass={clsx(`${rootClass}`, className)} {...props}/> + <Separator ref={ref} customRootClass={clsx(rootClass, className)} {...props}/>
1-1: Minor import style nit.Optional: import useContext and drop default React usage for consistency with named imports.
-import React, { forwardRef, ElementRef, ComponentPropsWithoutRef } from 'react'; +import { forwardRef, ElementRef, ComponentPropsWithoutRef, useContext } from 'react'; ... - const context = React.useContext(MenubarContext); + const context = useContext(MenubarContext);Also applies to: 12-12
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/components/ui/ContextMenu/fragments/ContextMenuSeparator.tsx(1 hunks)src/components/ui/DropdownMenu/fragments/DropdownMenuSeparator.tsx(1 hunks)src/components/ui/Menubar/fragments/MenubarSeparator.tsx(1 hunks)styles/themes/components/context-menu.scss(1 hunks)styles/themes/components/dropdown-menu.scss(1 hunks)styles/themes/components/menubar.scss(1 hunks)
⏰ 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). (2)
- GitHub Check: Build
- GitHub Check: coverage
🔇 Additional comments (4)
styles/themes/components/menubar.scss (1)
173-190: Resolved — Separator appends -vertical / -horizontal classes.
Separator computes orientationClass = orientation === 'vertical' ?${rootClass}-vertical:${rootClass}-horizontaland includes it in the element's className, so the SCSS selectors will match.src/components/ui/Menubar/fragments/MenubarSeparator.tsx (3)
13-16: Good defensive guard when used outside provider.Warning + null render avoids broken markup when the component is misused.
23-25: Display name and default export look good.Helps with DevTools and aligns with other fragments.
19-20: A11y: orientation validated — no change required. Separator defaults to orientation='horizontal' and MenubarSeparator forwards props, so no accessibility change needed.
src/components/ui/ContextMenu/fragments/ContextMenuSeparator.tsx
Outdated
Show resolved
Hide resolved
src/components/ui/DropdownMenu/fragments/DropdownMenuSeparator.tsx
Outdated
Show resolved
Hide resolved
| export type MenubarSeparatorProps = { | ||
| className?: string | ||
| } & ComponentPropsWithoutRef<typeof Separator>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Confirm Separator’s styling prop; avoid prop duplication.
- If Separator expects className (common), pass className and drop customRootClass.
- If Separator indeed uses customRootClass, keep as-is, but ensure Separator does not also expose className to avoid type duplication/confusion.
Proposed change (if Separator expects className):
-export type MenubarSeparatorProps = {
- className?: string
-} & ComponentPropsWithoutRef<typeof Separator>;
+export type MenubarSeparatorProps = ComponentPropsWithoutRef<typeof Separator>;
...
- <Separator ref={ref} customRootClass={clsx(`${rootClass}`, className)} {...props}/>
+ <Separator ref={ref} className={clsx(rootClass, className)} {...props}/>Run to verify Separator’s API:
Also applies to: 19-20
🏁 Script executed:
#!/bin/bash
# Locate Separator implementation(s)
fd -t f -a --glob "src/**/Separator*.{ts,tsx}"
# Inspect props for styling prop names
rg -n -C2 -P '\b(customRootClass|className)\b' src | sed -n '1,200p'
# Confirm default export form to match import style
rg -n -C2 -P 'export\s+default\s+(function\s+Separator|Separator)\b' srcLength of output: 17230
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- Separator.tsx ----"
nl -ba src/components/ui/Separator/Separator.tsx | sed -n '1,240p' || true
echo
echo "---- MenubarSeparator.tsx ----"
nl -ba src/components/ui/Menubar/fragments/MenubarSeparator.tsx | sed -n '1,240p' || trueLength of output: 137
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- Separator directory ----"
ls -la src/components/ui/Separator || true
echo
echo "---- Separator.tsx (first 240 lines) ----"
sed -n '1,240p' src/components/ui/Separator/Separator.tsx || true
echo
echo "---- MenubarSeparator.tsx (first 240 lines) ----"
sed -n '1,240p' src/components/ui/Menubar/fragments/MenubarSeparator.tsx || true
echo
echo "---- Search for styling props in Separator files ----"
rg -n --hidden --no-heading -S '\b(customRootClass|className)\b' src/components/ui/Separator || trueLength of output: 11543
Pass Menubar root via customRootClass and forward consumer className separately
- Separator exposes both customRootClass and className (see src/components/ui/Separator/Separator.tsx) — MenubarSeparator currently merges consumer className into customRootClass: customRootClass={clsx(
${rootClass}, className)}. That pushes consumer classes into the root-prefix/orientation logic instead of appending them as additional classes. - Change (src/components/ui/Menubar/fragments/MenubarSeparator.tsx): remove the explicit className?: string from MenubarSeparatorProps (it’s already in ComponentPropsWithoutRef) and render as:
<Separator ref={ref} customRootClass={rootClass} className={className} {...props} /> - Rationale: keep root prefix routing via customRootClass (used by customClassSwitcher/orientation) and ensure consumer className is appended predictably.
🤖 Prompt for AI Agents
In src/components/ui/Menubar/fragments/MenubarSeparator.tsx around lines 7 to 9,
MenubarSeparator currently defines className?: string in its props and merges
the consumer className into customRootClass
(customRootClass={clsx(`${rootClass}`, className)}), which incorrectly injects
consumer classes into the root-prefix/orientation logic; remove the explicit
className?: string from MenubarSeparatorProps (it already comes from
ComponentPropsWithoutRef<typeof Separator>) and change the render to pass
customRootClass={rootClass} and className={className} (forward ref unchanged),
i.e., stop merging className into customRootClass and forward consumer className
separately along with {...props}.
There was a problem hiding this comment.
explain it more with some example
There was a problem hiding this comment.
@GoldGroove06, let me explain the issue with a detailed example showing why separating customRootClass and className is important:
Current Problematic Implementation
// Current MenubarSeparator.tsx
export type MenubarSeparatorProps = {
className?: string // ❌ Explicitly defining className (redundant)
} & ComponentPropsWithoutRef<typeof Separator>;
// In the render:
<Separator
ref={ref}
customRootClass={clsx(`${rootClass}`, className)} // ❌ Merging everything into customRootClass
{...props}
/>What Happens Currently
Let's say:
rootClassfrom context ="rad-ui-menubar-separator"- User passes
className="my-custom-spacing"
Current result:
<Separator customRootClass="rad-ui-menubar-separator my-custom-spacing" />The Problem
The Separator component uses customRootClass for internal theming logic (like orientation detection, theme switching). When you merge user classes into it:
- Theme conflicts: User classes might interfere with internal theme logic
- Orientation issues: If Separator checks
customRootClassfor orientation keywords, user classes could break this - Specificity problems: User styles mixed with system styles in the same prop
Recommended Implementation
// Fixed MenubarSeparator.tsx
export type MenubarSeparatorProps =
ComponentPropsWithoutRef<typeof Separator>; // ✅ No explicit className (already included)
// In the render:
<Separator
ref={ref}
customRootClass={rootClass} // ✅ System/theme classes only
className={className} // ✅ User classes separate
{...props}
/>What Should Happen
Same inputs:
rootClass="rad-ui-menubar-separator"- User passes
className="my-custom-spacing"
Fixed result:
<Separator
customRootClass="rad-ui-menubar-separator" // System theming
className="my-custom-spacing" // User customization
/>Why This Separation Matters
// Example: User wants custom spacing
<Menubar.Separator className="mx-4 my-2" />
// Current (problematic): Everything mixed together
// customRootClass="rad-ui-menubar-separator mx-4 my-2"
// Fixed: Clean separation
// customRootClass="rad-ui-menubar-separator"
// className="mx-4 my-2"The Separator component can then:
- Use
customRootClassfor internal theme/orientation logic - Apply
classNameas additional user styles - Maintain proper CSS specificity and avoid conflicts
This same pattern should be applied to DropdownMenuSeparator and ContextMenuSeparator as well for consistency.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/coverage.yml (1)
11-13: Concurrency group will collide across PRsWith pull_request_target, github.ref resolves to the base branch (e.g., main), so runs from different PRs cancel each other. Scope the group to the PR number.
Apply:
concurrency: - group: ${{ github.workflow }}-${{ github.ref }} + group: ${{ github.workflow }}-pr-${{ github.event.pull_request.number }} cancel-in-progress: true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/calm-times-switch.md(1 hunks).github/workflows/coverage.yml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/calm-times-switch.md
⏰ 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
.github/workflows/coverage.yml
Outdated
| pull_request_target: | ||
| branches: [ main ] |
There was a problem hiding this comment.
Security: pull_request_target + executing PR code = token exfiltration risk
This workflow now runs arbitrary PR code (npm ci/coverage) under pull_request_target with a write-scoped GITHUB_TOKEN, which is unsafe for forked PRs. A malicious PR can steal the token or push commits. Revert to pull_request (safe token) and, if you need to comment on forked PRs, use a two‑workflow “bridge” (run coverage on pull_request, upload artifact; have a minimal pull_request_target job only download the artifact and post the comment). Also prevent git from storing the token in the repo URL.
Apply:
-on:
- pull_request_target:
+on:
+ pull_request:
branches: [ main ]And harden checkout:
- uses: actions/checkout@v4
with:
fetch-depth: 0
+ persist-credentials: falseIf you need the bridge pattern, I can provide a minimal pair of workflows (one runs coverage and uploads coverage-summary.json; the other, triggered via pull_request_target, downloads the artifact and posts the comment) without ever executing PR code in a privileged context.
Also applies to: 32-36, 41-51
🤖 Prompt for AI Agents
In .github/workflows/coverage.yml around lines 4-5 (and similarly address lines
32-36 and 41-51), the workflow is using pull_request_target to run untrusted PR
code with a write-scoped GITHUB_TOKEN; change the event back to pull_request so
the token is least-privileged, or implement the recommended bridge pattern: have
one workflow triggered on pull_request that runs tests/coverage and uploads the
coverage artifact, and a minimal pull_request_target workflow that only
downloads that artifact and posts the comment (never executes PR code in
privileged context). Also harden checkout by setting persist-credentials: false
and a minimal fetch-depth, and remove any use of GITHUB_TOKEN embedded in repo
URLs (use actions/checkout with token input only when necessary and avoid
exposing the token in git remotes). Ensure any jobs that need to post comments
explicitly use a separate, scoped token or the minimal pull_request_target step
that only uses safe operations.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/ui/Menubar/fragments/MenubarSeparator.tsx (1)
7-10: Avoid prop duplication; hide theming override from public APIRedefining
className(already on Separator props) is redundant, and exposingcustomRootClassinvites theme breakage by letting consumers override the menubar’s root class. Derive types fromSeparatorand omitcustomRootClassfrom the public surface.Apply:
-export type MenubarSeparatorProps = { - className?: string - customRootClass?: string -} & ComponentPropsWithoutRef<typeof Separator>; +export type MenubarSeparatorProps = Omit< + ComponentPropsWithoutRef<typeof Separator>, + 'customRootClass' +>; -const MenubarSeparator = forwardRef<MenubarSeparatorElement, MenubarSeparatorProps>(({ className, customRootClass, ...props }: MenubarSeparatorProps, ref) => { +const MenubarSeparator = forwardRef<MenubarSeparatorElement, MenubarSeparatorProps>( + ({ className, ...props }, ref, +) => { @@ - const rootClass = customRootClass || context.rootClass; + const rootClass = context.rootClass;Run to confirm
Separatoralready defines bothclassNameandcustomRootClass:#!/bin/bash set -euo pipefail rg -n -C2 -P '\b(customRootClass|className)\b' src/components/ui/Separator/Separator.tsx rg -n -C2 -P 'type\s+.*SeparatorProps|interface\s+.*SeparatorProps' src/components/ui/Separator/Separator.tsxAlso applies to: 12-12, 18-18
🧹 Nitpick comments (2)
src/components/ui/Menubar/fragments/MenubarSeparator.tsx (2)
4-4: Remove unused import
clsxis imported but not used.Apply:
-import clsx from 'clsx';
14-16: Guard console warnings to dev-onlyAvoid noisy logs in production bundles.
Apply:
- if (!context) { - console.warn('MenubarSeparator should be used within MenubarRoot'); - return null; - } + if (!context) { + if (process.env.NODE_ENV !== 'production') { + console.warn('MenubarSeparator should be used within MenubarRoot'); + } + return null; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/ui/ContextMenu/fragments/ContextMenuSeparator.tsx(1 hunks)src/components/ui/DropdownMenu/fragments/DropdownMenuSeparator.tsx(1 hunks)src/components/ui/Menubar/fragments/MenubarSeparator.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/ui/DropdownMenu/fragments/DropdownMenuSeparator.tsx
- src/components/ui/ContextMenu/fragments/ContextMenuSeparator.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). (2)
- GitHub Check: Build
- GitHub Check: coverage
🔇 Additional comments (1)
src/components/ui/Menubar/fragments/MenubarSeparator.tsx (1)
20-23: LGTM: separator wiring and ref forwarding are correct
customRootClassreceives the menubar root,classNameis forwarded for consumer styles, and the ref is properly passed through.
| console.warn('ContextMenuSeparator should be used within ContextMenuRoot'); | ||
| return null; | ||
| } | ||
| const rootClass = customRootClass || context.rootClass; |
There was a problem hiding this comment.
if context is invalid we anyway exit right? this check is redundant
There was a problem hiding this comment.
we are here checking if the customRootClass exists , if not fall back to context for the rootClass
| console.warn('DropdownMenuSeparator should be used within DropdownMenuRoot'); | ||
| return null; | ||
| } | ||
| const rootClass = customRootClass || context.rootClass; |
| console.warn('MenubarSeparator should be used within MenubarRoot'); | ||
| return null; | ||
| } | ||
| const rootClass = customRootClass || context.rootClass; |
closes #1586
Summary by CodeRabbit
New Features
Style
Documentation
Chores