chore: add portalling and make ui updates#1726
Merged
Conversation
🦋 Changeset detectedLatest commit: 13c1e2a 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 |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ThemeContext-based portal resolution and many UI token/styling refactors; introduces Card/Dialog/AlertDialog footer subcomponents and multiple Card fragments; refactors SandboxEditor color picker; updates numerous Storybook stories; and makes accessibility/focus and portal-related behavior changes across primitives and components. Changes
Sequence Diagram(s)mermaid Component->>Theme: read ThemeContext (portalRootRef/containerRef) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/components/ui/Badge/stories/Badge.stories.tsx (1)
89-100:⚠️ Potential issue | 🟡 MinorDon't advertise
coloron the destructive variant.
badge.scsshard-codesdestructiveto red, so these examples either ignorecoloror render identical badges with different labels. Keepdestructivein the variant demo, but filter it out of the color showcase.Suggested story adjustment
- {Variants.map((variant, vIndex) => ( + {Variants.filter((variant) => variant !== 'destructive').map((variant, vIndex) => ( <div key={vIndex} className='flex flex-col gap-2'> <p className='text-xs text-gray-600'>{variant}</p> <div className='flex flex-wrap gap-2'> {Colors.slice(0, 3).map((color, cIndex) => ( <Badge key={cIndex} color={color} variant={variant}> {color} </Badge> ))} </div> </div> ))} @@ - <Badge color="blue" variant="destructive">Destructive</Badge> + <Badge variant="destructive">Destructive</Badge>Also applies to: 140-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Badge/stories/Badge.stories.tsx` around lines 89 - 100, The story currently advertises different colors for every variant, but the destructive variant is hard-coded to red in badge.scss so color props are ignored; in the Badge.stories.tsx change the inner render logic inside the Variants.map: if variant === 'destructive' render a single Badge (e.g., <Badge variant="destructive">destructive</Badge>) instead of mapping Colors, otherwise continue mapping Colors.slice(0,3). Update the block that renders Colors (referencing Variants, Colors and Badge) to conditionally skip the color showcase for the 'destructive' variant.src/components/tools/SandboxEditor/SandboxEditor.tsx (1)
45-47:⚠️ Potential issue | 🟡 MinorAvoid rendering a literal
undefinedclass.When
classNameis omitted, the wrapper on Line 130 becomespt-3 undefined. Coalesce the prop before interpolating so bare<SandboxEditor>usages don't leak a bogus class.💡 Minimal fix
-type SandboxProps = {className?: string | ''} & PropsWithChildren +type SandboxProps = {className?: string} & PropsWithChildren ... - <div className={`pt-3 ${className}`} > + <div className={className ? `pt-3 ${className}` : 'pt-3'}>Also applies to: 130-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/tools/SandboxEditor/SandboxEditor.tsx` around lines 45 - 47, The SandboxEditor currently types className as "string | ''" and interpolates it directly causing a literal "undefined" class when omitted; change SandboxProps to className?: string and update the SandboxEditor to coalesce the prop (e.g., default param or use className ?? '' or conditional concat) when building the wrapper class so that bare <SandboxEditor> does not inject "undefined" into the class list; locate the SandboxProps type and the SandboxEditor function to apply these changes.src/core/primitives/Dialog/fragments/DialogPrimitivePortal.tsx (1)
46-76: 🛠️ Refactor suggestion | 🟠 MajorRedundant conditional branches render identical JSX.
The
forceMount,keepMounted, and default branches (lines 46-76) all render the exact same<Floater.Portal>markup. This makes the conditions meaningless and adds unnecessary complexity.Either differentiate the behavior between these branches or consolidate to a single return:
- // If forceMount is true, always render - if (forceMount) { - return ( - <Floater.Portal - root={rootElementRef.current} - {...props} - > - {children} - </Floater.Portal> - ); - } - - // If keepMounted is true, keep the portal container but children follow their own mounting logic - if (keepMounted) { - return ( - <Floater.Portal - root={rootElementRef.current} - {...props} - > - {children} - </Floater.Portal> - ); - } - return ( <Floater.Portal root={rootElementRef.current} {...props} > {children} </Floater.Portal> );If
forceMountandkeepMountedare meant to control rendering differently, implement the actual differentiation logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/primitives/Dialog/fragments/DialogPrimitivePortal.tsx` around lines 46 - 76, The three branches checking forceMount and keepMounted in DialogPrimitivePortal that all return the same Floater.Portal are redundant; consolidate them by removing the conditional branches and returning a single Floater.Portal using rootElementRef.current, props, and children, or if forceMount/keepMounted should alter behavior, implement the intended differences inside the component (e.g., conditionally render children, change portal props, or apply mounting logic) within the DialogPrimitivePortal function so the symbols forceMount, keepMounted, Floater.Portal, rootElementRef, children, and props are used meaningfully.
🟡 Minor comments (19)
styles/themes/components/steps.scss-50-55 (1)
50-55:⚠️ Potential issue | 🟡 MinorReplace hardcoded
color: whitewith the semantic token--rad-ui-text-inverse.Line 54 uses a hardcoded
whitevalue, which is inconsistent with the semantic token approach used throughout the codebase. The design system provides--rad-ui-text-inverse(defined asvar(--rad-ui-color-gray-50)) for text on dark/accent backgrounds, as used inbutton.scssandtooltip.scss. Update to maintain consistency with the design system.♻️ Suggested refactor
&[data-state="completed"] { .rad-ui-steps-bubble { border-color: var(--rad-ui-color-accent-900); background-color: var(--rad-ui-color-accent-900); - color: white; + color: var(--rad-ui-text-inverse); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/steps.scss` around lines 50 - 55, In the &[data-state="completed"] rule for .rad-ui-steps-bubble replace the hardcoded "color: white" with the semantic token --rad-ui-text-inverse; update the declaration to use var(--rad-ui-text-inverse) so text on accent backgrounds uses the design system token (match the pattern used in button.scss and tooltip.scss).src/components/ui/Progress/stories/Progress.stories.tsx-40-47 (1)
40-47:⚠️ Potential issue | 🟡 MinorAdd an accessible name to the range control.
Lines 40-47 render a range input without an accessible name. Add
aria-label(or a<label htmlFor>pair) so the control is discoverable by assistive tech.Based on learnings: 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`.💡 Suggested fix
<input type="range" + aria-label="Progress value" min="0" max="100" value={value}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Progress/stories/Progress.stories.tsx` around lines 40 - 47, The range input rendered (the <input> with type="range" using value, setValue, onChange and className="w-full max-w-[25.25rem]") needs an accessible name; add either an aria-label (e.g., aria-label="Progress" or similar descriptive text) to the input or wire it to a visible <label htmlFor="..."> by giving the input an id and matching label text so assistive tech can discover the control; update the input element where value and onChange are defined (and ensure the label text describes the control's purpose).styles/themes/components/table.scss-2-2 (1)
2-2:⚠️ Potential issue | 🟡 MinorUse an existing border token key on the table wrapper.
Line 2 references
--rad-ui-border, but the semantic border tokens in this PR are--rad-ui-border-soft/default/strong. If--rad-ui-borderis undefined, the border declaration is dropped.💡 Suggested fix
-.rad-ui-table-wrapper{ - border: 1px solid var(--rad-ui-border); +.rad-ui-table-wrapper{ + border: 1px solid var(--rad-ui-border-default);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/table.scss` at line 2, The table wrapper's border uses a nonexistent token (--rad-ui-border); replace it with an existing semantic border token such as --rad-ui-border-soft (or more specific variant --rad-ui-border-soft-default or --rad-ui-border-soft-strong as appropriate for visual weight) in the rule that sets "border: 1px solid var(--rad-ui-border)" so the declaration never drops when the token is undefined and matches the project's semantic token naming.styles/themes/components/textarea.scss-21-24 (1)
21-24:⚠️ Potential issue | 🟡 MinorLet the placeholder inherit the current size variant.
Hard-coding
::placeholderto1removerrides thesmallandlargefont-size variants, so the placeholder and entered text no longer match. Useinheritor remove the explicit size.Suggested fix
&::placeholder { color: var(--rad-ui-text-muted); - font-size: 1rem; + font-size: inherit; }Also applies to: 62-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/textarea.scss` around lines 21 - 24, The ::placeholder rule in the textarea component currently hard-codes font-size: 1rem which breaks the small/large size variants; update the &::placeholder selector in styles/themes/components/textarea.scss to use font-size: inherit (or remove the font-size declaration) so the placeholder inherits the component's current size variant, and apply the same change to the duplicate placeholder block around lines 62-76 to keep behavior consistent.src/components/ui/Checkbox/stories/Checkbox.stories.tsx-85-92 (1)
85-92:⚠️ Potential issue | 🟡 MinorThe new copy mapping assigns the wrong labels to two rows.
notificationsnow renders the same label/description asterms, and thedisabledrow falls through to the notification label. That leaves Storybook showing duplicated content instead of three distinct checkbox states.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Checkbox/stories/Checkbox.stories.tsx` around lines 85 - 92, The label ternary and the conditional description in Checkbox.stories.tsx are mis-mapped: the expression rendering the label text (inside the <label> for variable key) currently gives 'Accept terms and conditions' for both 'terms' and 'notifications', and the description block beneath only targets 'notifications' but uses the terms copy. Fix the ternary so key === 'terms' => 'Accept terms and conditions', key === 'notifications' => 'Enable notifications' (and for 'disabled' use an appropriate fallback like 'Disabled' or the intended label), and update the conditional paragraph rendering so that 'terms' shows "By clicking this checkbox, you agree to the terms." and 'notifications' shows a notifications-specific line (e.g., "Enable notifications to receive updates."), leaving 'disabled' without the description if intended; adjust the label rendering and the conditional (key === 'terms' / key === 'notifications') to match these mappings.styles/themes/components/select.scss-130-154 (1)
130-154:⚠️ Potential issue | 🟡 MinorSelected options lose hover/highlight feedback.
[aria-selected="true"]is declared after[data-highlighted]and[data-active="true"], so the selected option keepssurface-mutedeven while hovered or keyboard-highlighted. That makes the current focus target harder to track.Suggested fix
- &[data-highlighted] { - background-color: var(--rad-ui-surface-subtle); - color: var(--rad-ui-text-primary); - } + &[data-highlighted] { + background-color: var(--rad-ui-surface-subtle); + color: var(--rad-ui-text-primary); + } @@ - &[data-active="true"] { + &[data-active="true"] { outline: none; background-color: var(--rad-ui-surface-subtle); } @@ - &[aria-selected="true"] { + &[aria-selected="true"]:not([data-highlighted]):not([data-active="true"]) { background-color: var(--rad-ui-surface-muted); color: var(--rad-ui-text-primary); font-weight: 500; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/select.scss` around lines 130 - 154, The selected option's styles ([aria-selected="true"]) are overriding hover/keyboard feedback ([data-highlighted], [data-active="true"]); update the stylesheet so highlight/active states take precedence by adding more specific selectors or reordering: create combined selectors like &[aria-selected="true"][data-highlighted] and &[aria-selected="true"][data-active="true"] (and also adjust .rad-ui-select-item-indicator if needed) to set background-color: var(--rad-ui-surface-subtle), color: var(--rad-ui-text-primary) and opacity for the indicator, ensuring hovered/active selected items show the highlight instead of surface-muted.src/components/ui/Table/stories/Table.stories.tsx-74-75 (1)
74-75:⚠️ Potential issue | 🟡 MinorLabel the actions column, not just the icon.
The last header cell is empty and each row action is icon-only, so screen readers get no useful name for that column/action.
💡 Minimal fix
- <Table.ColumnCellHeader style={{ width: '3rem' }} /> + <Table.ColumnCellHeader style={{ width: '3rem' }}> + <span className="sr-only">Actions</span> + </Table.ColumnCellHeader> ... - <button className="inline-flex h-8 w-8 items-center justify-center rounded-md text-gray-700" type="button"> + <button + aria-label={`Open actions for ${row.email}`} + className="inline-flex h-8 w-8 items-center justify-center rounded-md text-gray-700" + type="button" + >Also applies to: 89-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Table/stories/Table.stories.tsx` around lines 74 - 75, The actions column header is empty so screen readers can't identify the icon-only buttons; update the two Table.ColumnCellHeader instances (the one currently with style width: '3rem' and the other occurrence around rows 89-92) to include an accessible label such as the visible text "Actions" or a visually-hidden span "Actions" inside the Table.ColumnCellHeader, and ensure each row action icon/button (the icon-only controls referenced in the row rendering) includes an appropriate aria-label or title attribute describing the action.styles/themes/components/splitter.scss-59-70 (1)
59-70:⚠️ Potential issue | 🟡 MinorReduced-motion still animates the handle indicator.
The visible motion now lives on
::after, but the media query only disables transitions on the handle itself. Users withprefers-reduced-motionwill still get the fade/background animation.💡 Minimal fix
`@media` (prefers-reduced-motion: reduce) { .rad-ui-splitter-panel { transition: none; } .rad-ui-splitter-handle { transition: none; } + + .rad-ui-splitter-handle::after { + transition: none; + } }Also applies to: 111-123, 144-151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/splitter.scss` around lines 59 - 70, The ::after pseudo-element still has transitions so users with prefers-reduced-motion still see the fade; update the prefers-reduced-motion media query that currently targets the handle to also target the handle::after (and any other pseudo-elements used, e.g., the visible indicator) and disable its animations/transitions (set transition: none or remove opacity/background-color transitions) for the selectors that include ::after (references: the rule containing &::after and the other blocks around lines 111-123 and 144-151).src/components/ui/Table/stories/Table.stories.tsx-47-52 (1)
47-52:⚠️ Potential issue | 🟡 MinorGive the filter field an accessible name.
The placeholder disappears on input and doesn’t reliably serve as a label, so this control is currently unnamed to assistive tech.
💡 Minimal fix
<input + aria-label="Filter emails" className="h-10 w-full max-w-[29rem] rounded-xl border border-gray-300 bg-gray-50 px-3 text-sm text-gray-950 outline-none" placeholder="Filter emails..." type="text" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Table/stories/Table.stories.tsx` around lines 47 - 52, The input in the Table story is missing an accessible name; add one by giving the input an id and a visible or visually-hidden label or by adding an aria-label (e.g., aria-label="Filter emails") so assistive tech can identify it when the placeholder disappears; update the input element in Table.stories.tsx (the <input ... placeholder="Filter emails..." ... />) to include either a matching <label htmlFor="filter-emails">Filter emails</label> or aria-label="Filter emails".styles/themes/components/toolbar.scss-100-107 (1)
100-107:⚠️ Potential issue | 🟡 MinorPreserve the pressed state on hover.
[data-state='on']and:hoverhave the same specificity here, so the later hover rule replaces the accent background and makes selected items look unselected under the pointer.💡 Minimal fix
&:hover { background-color: var(--rad-ui-surface-subtle); } + + &[data-state='on']:hover { + background-color: var(--rad-ui-color-accent-100); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/toolbar.scss` around lines 100 - 107, The hover rule currently overrides the selected rule because both selectors (&[data-state='on'] and &:hover) have equal specificity; update the CSS so hover styling does not apply when the item is selected — e.g. change the hover selector to only target non-selected items (use &:hover:not([data-state='on']) or add a specific rule for &[data-state='on']:hover to preserve its background) so the &[data-state='on'] accent background and color remain visible while hovering.src/components/ui/Collapsible/stories/Collapsible.stories.tsx-27-33 (1)
27-33:⚠️ Potential issue | 🟡 MinorHide the chevron from assistive tech.
This icon is purely decorative in both triggers. Without
aria-hidden, some screen readers will announce an extra graphic inside the button.💡 Suggested fix
- <svg width="16" height="16" viewBox="0 0 16 16" fill="none" xmlns="http://www.w3.org/2000/svg" {...props}> + <svg width="16" height="16" viewBox="0 0 16 16" fill="none" xmlns="http://www.w3.org/2000/svg" {...props} aria-hidden="true" focusable="false">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Collapsible/stories/Collapsible.stories.tsx` around lines 27 - 33, The DoubleChevronIcon SVG is decorative and being announced by screen readers; update the DoubleChevronIcon component to hide it from assistive tech by adding accessibility attributes to the <svg> element (e.g., aria-hidden="true" and focusable="false") so the graphic is removed from the accessibility tree and not focusable; keep spreading {...props} but ensure these attributes are present on the svg in the DoubleChevronIcon function.styles/themes/components/slider.scss-133-136 (1)
133-136:⚠️ Potential issue | 🟡 MinorFix the vertical mark label layout.
In the vertical slider variant (lines 133–136), the
.rad-ui-slider-mark-labelhas nopositionproperty, sotopandrightdo nothing. However,transform: translateY(-50%)still applies and shifts the label upward instead of keeping it flex-centered. Replace with margin-based spacing for the flex row layout.💡 Suggested fix
.rad-ui-slider-mark-label { - right: 1rem; - top: 50%; - transform: translateY(-50%); + margin-left: 0.5rem; + transform: none; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/slider.scss` around lines 133 - 136, The .rad-ui-slider-mark-label in the vertical slider variant lacks a position, so its top/right rules are ineffective while transform: translateY(-50%) incorrectly shifts it; update the .rad-ui-slider-mark-label styling used by the vertical slider to remove top/right/transform and instead rely on the flex row layout by using margin (e.g., margin-right or margin-left as appropriate) and align-items/justify-content to vertically center the label; adjust any parent .rad-ui-slider (or vertical variant selector) to ensure the label is placed in the flex row and uses margin-based spacing rather than absolute positioning.styles/themes/components/switch.scss-1-12 (1)
1-12:⚠️ Potential issue | 🟡 MinorAdd
box-sizing: border-boxafterall: unset.The
all: unsetdeclaration resetsbox-sizingtocontent-box, which expands the root's total width to 42px (40px content + 2px borders). This misaligns the checked thumb position—attranslateX(1.125rem)(18px), it falls 2px short of the right edge when border-box sizing is intended.Suggested fix
.rad-ui-switch { all: unset; + box-sizing: border-box; position: relative;Please verify the thumb alignment in the
Checkedstory after applying this fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/switch.scss` around lines 1 - 12, The .rad-ui-switch rule uses all: unset which resets box-sizing to content-box and causes the component width to grow by borders; add box-sizing: border-box immediately after all: unset in the .rad-ui-switch selector to ensure borders are included in the width so the checked thumb (the switch's thumb translateX value used in the Checked story) aligns correctly with the right edge; after adding it, verify the Checked story thumb position and adjust translateX only if needed.styles/themes/components/context-menu.scss-91-94 (1)
91-94:⚠️ Potential issue | 🟡 MinorKeep destructive items red when they’re highlighted.
[data-highlighted]and[aria-selected="true"]set the text color back to--rad-ui-text-primary, so.rad-ui-context-menu-dangeronly works at rest. Add matching state selectors for the danger variant.🎯 Minimal state-preserving variant
-.rad-ui-context-menu-danger { - color: var(--rad-ui-color-red-900); -} +.rad-ui-context-menu-danger, +.rad-ui-context-menu-sub-trigger.rad-ui-context-menu-danger[data-highlighted], +.rad-ui-context-menu-item.rad-ui-context-menu-danger[data-highlighted], +.rad-ui-context-menu-item.rad-ui-context-menu-danger[aria-selected="true"] { + color: var(--rad-ui-color-red-900); +}Also applies to: 147-150, 182-184
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/context-menu.scss` around lines 91 - 94, The danger variant loses its red text when highlighted/selected because the generic highlighted selectors override it; update the `.rad-ui-context-menu-danger` rules to include matching state selectors so the danger color is preserved (add `[data-highlighted]` and `[aria-selected="true"]` variants for `.rad-ui-context-menu-danger` and the inner elements that currently set color in the `[data-highlighted]`/`[aria-selected="true"]` blocks) — target the `.rad-ui-context-menu-danger` class and the existing `[data-highlighted]` and `[aria-selected="true"]` selectors referenced in the diff so highlighted/selected danger items keep the danger color.styles/themes/components/alert-dialog.scss-94-95 (1)
94-95:⚠️ Potential issue | 🟡 MinorMake the mobile and desktop breakpoints mutually exclusive.
At exactly 640px, both
(width >= 640px)and(width <= 640px)match, causing both the desktop footer layout and mobile padding/typography overrides to apply together. Change one comparison to exclude the boundary (e.g.,(width > 640px)or(width < 640px)).Also applies to: 127-146
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/alert-dialog.scss` around lines 94 - 95, The media query at `@media` (width >= 640px) currently overlaps the mobile rule `@media` (width <= 640px) at exactly 640px; update the desktop breakpoint to be exclusive (e.g., change `@media` (width >= 640px) to `@media` (width > 640px)) so the mobile and desktop rules do not both apply; apply the same boundary fix to the other overlapping queries in the file (the block around lines 127-146) so each breakpoint is mutually exclusive.src/core/primitives/Dialog/fragments/DialogPrimitivePortal.tsx-29-32 (1)
29-32:⚠️ Potential issue | 🟡 MinorMissing null in type cast for querySelector.
For consistency with
HoverCardPortal.tsx(line 16) and type safety, the cast should includenull:- const themeContainer = themeContext?.portalRootRef.current - || document.querySelector('[data-rad-ui-portal-root]') as HTMLElement + const themeContainer = themeContext?.portalRootRef.current + || (document.querySelector('[data-rad-ui-portal-root]') as HTMLElement | null)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/primitives/Dialog/fragments/DialogPrimitivePortal.tsx` around lines 29 - 32, The querySelector cast for themeContainer in DialogPrimitivePortal (the themeContainer assignment using themeContext?.portalRootRef.current || document.querySelector('[data-rad-ui-portal-root]') as HTMLElement || ...) should include null for type-safety and to match HoverCardPortal.tsx; change the HTMLElement cast to HTMLElement | null (e.g., cast the document.querySelector('[data-rad-ui-portal-root]') to HTMLElement | null) so themeContainer's union resolution is correct and TypeScript won't assume a non-null HTMLElement.src/components/ui/HoverCard/stories/HoverCard.stories.tsx-63-65 (1)
63-65:⚠️ Potential issue | 🟡 MinorUse a semantic button for the interactive trigger in the controlled example.
Line 64 uses a
<span onClick={() => setOpen(true)}>, which lacks keyboard accessibility. Replace it with a<button type="button">to ensure the controlled example demonstrates accessible patterns.Suggested fix
<HoverCard.Trigger> - <span onClick={() => setOpen(true)}>{open ? 'Hover Here' : 'Show Card'}</span> + <button type="button" onClick={() => setOpen(true)}> + {open ? 'Hover Here' : 'Show Card'} + </button> </HoverCard.Trigger>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/HoverCard/stories/HoverCard.stories.tsx` around lines 63 - 65, The trigger uses a non-semantic <span> which breaks keyboard accessibility; replace the <span onClick={() => setOpen(true)}> inside HoverCard.Trigger with a <button type="button"> that calls setOpen(true) (and preserves any className/aria props) so the controlled HoverCard example demonstrates an accessible, keyboard-focusable trigger; ensure any existing styling or props are forwarded to the button and keep the open ? 'Hover Here' : 'Show Card' label logic.styles/themes/components/radiocards.scss-80-85 (1)
80-85:⚠️ Potential issue | 🟡 Minor
cursor: not-allowedis ineffective whenpointer-events: noneis set.When
pointer-events: noneis applied, the element doesn't receive pointer events, so the cursor style won't be visible. Consider removingpointer-events: noneif you want the not-allowed cursor to show, or removecursor: not-allowedsince it has no effect.🔧 Suggested fix (option: show cursor)
&[data-disabled], &:disabled { opacity: 0.5; cursor: not-allowed; - pointer-events: none; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/radiocards.scss` around lines 80 - 85, The disabled state block for the radio card selectors (&[data-disabled], &:disabled) mixes cursor: not-allowed with pointer-events: none which makes the cursor ineffective; update the rule to remove pointer-events: none (leave cursor: not-allowed and opacity) so the not-allowed cursor is visible for disabled items, and if needed move pointer-events: none to an inner interactive element or toggle it only when you truly want to block pointer interactions.styles/themes/components/radio-group.scss-32-35 (1)
32-35:⚠️ Potential issue | 🟡 MinorBorder-width change on checked state may cause minor layout shift.
Increasing
border-widthfrom 1px to 2px shifts the inner content by 1px. Consider usingbox-shadowfor the thicker border effect to avoid layout shift, or adjust the checked state sizing to compensate.🔧 Alternative using inset box-shadow
&[data-state="checked"] { - border-width: 2px; - border-color: var(--rad-ui-text-primary); + border-color: var(--rad-ui-text-primary); + box-shadow: inset 0 0 0 1px var(--rad-ui-text-primary); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/radio-group.scss` around lines 32 - 35, The checked-state rule for the radio-group (&[data-state="checked"]) increases border-width from 1px to 2px causing a 1px layout shift; instead, keep the border-width constant and emulate a thicker border using an inset box-shadow or outline so inner content doesn't move. Update the &[data-state="checked"] rule in radio-group.scss to remove the border-width change and add an inset box-shadow (or outline) using var(--rad-ui-text-primary), or alternatively adjust padding/margin to compensate while keeping box-sizing consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@styles/themes/components/blockquote.scss`:
- Around line 5-11: The base blockquote rule currently defines neutral defaults
(border-inline-start: 4px solid var(--rad-ui-border-default); color:
var(--rad-ui-text-secondary);) but then immediately overrides them with accent
values (border-inline-start-color: var(--rad-ui-color-accent-600); color:
var(--rad-ui-color-accent-1000);), making the neutral variant unreachable;
remove the accent-specific declarations from this base rule and instead place
them in a dedicated variant selector (e.g., .blockquote--accent or a
.variant-accent rule) so the base styles (border-inline-start, color, font-size,
line-height) remain the default and the accent colors are applied only when the
accent variant class is present.
In `@styles/themes/components/context-menu.scss`:
- Around line 14-16: The shared context-menu theme currently hardcodes
demo-sized dimensions (padding: 6rem 1.5rem; width: 24rem; min-height: 13.5rem)
which forces every .rad-ui-context-menu-trigger to be oversized; remove the
fixed width and min-height from the theme (leave sensible padding or use
relative sizes like auto/max-content) and instead apply the 24rem×13.5rem sizing
in the story/docs via a demo-specific modifier class (e.g.,
.rad-ui-context-menu--demo) or a CSS variable so normal inline/custom triggers
remain unmodified; update any references to .rad-ui-context-menu-trigger in
styles/themes/components/context-menu.scss and the story CSS to use the demo
modifier.
---
Outside diff comments:
In `@src/components/tools/SandboxEditor/SandboxEditor.tsx`:
- Around line 45-47: The SandboxEditor currently types className as "string |
''" and interpolates it directly causing a literal "undefined" class when
omitted; change SandboxProps to className?: string and update the SandboxEditor
to coalesce the prop (e.g., default param or use className ?? '' or conditional
concat) when building the wrapper class so that bare <SandboxEditor> does not
inject "undefined" into the class list; locate the SandboxProps type and the
SandboxEditor function to apply these changes.
In `@src/components/ui/Badge/stories/Badge.stories.tsx`:
- Around line 89-100: The story currently advertises different colors for every
variant, but the destructive variant is hard-coded to red in badge.scss so color
props are ignored; in the Badge.stories.tsx change the inner render logic inside
the Variants.map: if variant === 'destructive' render a single Badge (e.g.,
<Badge variant="destructive">destructive</Badge>) instead of mapping Colors,
otherwise continue mapping Colors.slice(0,3). Update the block that renders
Colors (referencing Variants, Colors and Badge) to conditionally skip the color
showcase for the 'destructive' variant.
In `@src/core/primitives/Dialog/fragments/DialogPrimitivePortal.tsx`:
- Around line 46-76: The three branches checking forceMount and keepMounted in
DialogPrimitivePortal that all return the same Floater.Portal are redundant;
consolidate them by removing the conditional branches and returning a single
Floater.Portal using rootElementRef.current, props, and children, or if
forceMount/keepMounted should alter behavior, implement the intended differences
inside the component (e.g., conditionally render children, change portal props,
or apply mounting logic) within the DialogPrimitivePortal function so the
symbols forceMount, keepMounted, Floater.Portal, rootElementRef, children, and
props are used meaningfully.
---
Minor comments:
In `@src/components/ui/Checkbox/stories/Checkbox.stories.tsx`:
- Around line 85-92: The label ternary and the conditional description in
Checkbox.stories.tsx are mis-mapped: the expression rendering the label text
(inside the <label> for variable key) currently gives 'Accept terms and
conditions' for both 'terms' and 'notifications', and the description block
beneath only targets 'notifications' but uses the terms copy. Fix the ternary so
key === 'terms' => 'Accept terms and conditions', key === 'notifications' =>
'Enable notifications' (and for 'disabled' use an appropriate fallback like
'Disabled' or the intended label), and update the conditional paragraph
rendering so that 'terms' shows "By clicking this checkbox, you agree to the
terms." and 'notifications' shows a notifications-specific line (e.g., "Enable
notifications to receive updates."), leaving 'disabled' without the description
if intended; adjust the label rendering and the conditional (key === 'terms' /
key === 'notifications') to match these mappings.
In `@src/components/ui/Collapsible/stories/Collapsible.stories.tsx`:
- Around line 27-33: The DoubleChevronIcon SVG is decorative and being announced
by screen readers; update the DoubleChevronIcon component to hide it from
assistive tech by adding accessibility attributes to the <svg> element (e.g.,
aria-hidden="true" and focusable="false") so the graphic is removed from the
accessibility tree and not focusable; keep spreading {...props} but ensure these
attributes are present on the svg in the DoubleChevronIcon function.
In `@src/components/ui/HoverCard/stories/HoverCard.stories.tsx`:
- Around line 63-65: The trigger uses a non-semantic <span> which breaks
keyboard accessibility; replace the <span onClick={() => setOpen(true)}> inside
HoverCard.Trigger with a <button type="button"> that calls setOpen(true) (and
preserves any className/aria props) so the controlled HoverCard example
demonstrates an accessible, keyboard-focusable trigger; ensure any existing
styling or props are forwarded to the button and keep the open ? 'Hover Here' :
'Show Card' label logic.
In `@src/components/ui/Progress/stories/Progress.stories.tsx`:
- Around line 40-47: The range input rendered (the <input> with type="range"
using value, setValue, onChange and className="w-full max-w-[25.25rem]") needs
an accessible name; add either an aria-label (e.g., aria-label="Progress" or
similar descriptive text) to the input or wire it to a visible <label
htmlFor="..."> by giving the input an id and matching label text so assistive
tech can discover the control; update the input element where value and onChange
are defined (and ensure the label text describes the control's purpose).
In `@src/components/ui/Table/stories/Table.stories.tsx`:
- Around line 74-75: The actions column header is empty so screen readers can't
identify the icon-only buttons; update the two Table.ColumnCellHeader instances
(the one currently with style width: '3rem' and the other occurrence around rows
89-92) to include an accessible label such as the visible text "Actions" or a
visually-hidden span "Actions" inside the Table.ColumnCellHeader, and ensure
each row action icon/button (the icon-only controls referenced in the row
rendering) includes an appropriate aria-label or title attribute describing the
action.
- Around line 47-52: The input in the Table story is missing an accessible name;
add one by giving the input an id and a visible or visually-hidden label or by
adding an aria-label (e.g., aria-label="Filter emails") so assistive tech can
identify it when the placeholder disappears; update the input element in
Table.stories.tsx (the <input ... placeholder="Filter emails..." ... />) to
include either a matching <label htmlFor="filter-emails">Filter emails</label>
or aria-label="Filter emails".
In `@src/core/primitives/Dialog/fragments/DialogPrimitivePortal.tsx`:
- Around line 29-32: The querySelector cast for themeContainer in
DialogPrimitivePortal (the themeContainer assignment using
themeContext?.portalRootRef.current ||
document.querySelector('[data-rad-ui-portal-root]') as HTMLElement || ...)
should include null for type-safety and to match HoverCardPortal.tsx; change the
HTMLElement cast to HTMLElement | null (e.g., cast the
document.querySelector('[data-rad-ui-portal-root]') to HTMLElement | null) so
themeContainer's union resolution is correct and TypeScript won't assume a
non-null HTMLElement.
In `@styles/themes/components/alert-dialog.scss`:
- Around line 94-95: The media query at `@media` (width >= 640px) currently
overlaps the mobile rule `@media` (width <= 640px) at exactly 640px; update the
desktop breakpoint to be exclusive (e.g., change `@media` (width >= 640px) to
`@media` (width > 640px)) so the mobile and desktop rules do not both apply; apply
the same boundary fix to the other overlapping queries in the file (the block
around lines 127-146) so each breakpoint is mutually exclusive.
In `@styles/themes/components/context-menu.scss`:
- Around line 91-94: The danger variant loses its red text when
highlighted/selected because the generic highlighted selectors override it;
update the `.rad-ui-context-menu-danger` rules to include matching state
selectors so the danger color is preserved (add `[data-highlighted]` and
`[aria-selected="true"]` variants for `.rad-ui-context-menu-danger` and the
inner elements that currently set color in the
`[data-highlighted]`/`[aria-selected="true"]` blocks) — target the
`.rad-ui-context-menu-danger` class and the existing `[data-highlighted]` and
`[aria-selected="true"]` selectors referenced in the diff so
highlighted/selected danger items keep the danger color.
In `@styles/themes/components/radio-group.scss`:
- Around line 32-35: The checked-state rule for the radio-group
(&[data-state="checked"]) increases border-width from 1px to 2px causing a 1px
layout shift; instead, keep the border-width constant and emulate a thicker
border using an inset box-shadow or outline so inner content doesn't move.
Update the &[data-state="checked"] rule in radio-group.scss to remove the
border-width change and add an inset box-shadow (or outline) using
var(--rad-ui-text-primary), or alternatively adjust padding/margin to compensate
while keeping box-sizing consistent.
In `@styles/themes/components/radiocards.scss`:
- Around line 80-85: The disabled state block for the radio card selectors
(&[data-disabled], &:disabled) mixes cursor: not-allowed with pointer-events:
none which makes the cursor ineffective; update the rule to remove
pointer-events: none (leave cursor: not-allowed and opacity) so the not-allowed
cursor is visible for disabled items, and if needed move pointer-events: none to
an inner interactive element or toggle it only when you truly want to block
pointer interactions.
In `@styles/themes/components/select.scss`:
- Around line 130-154: The selected option's styles ([aria-selected="true"]) are
overriding hover/keyboard feedback ([data-highlighted], [data-active="true"]);
update the stylesheet so highlight/active states take precedence by adding more
specific selectors or reordering: create combined selectors like
&[aria-selected="true"][data-highlighted] and
&[aria-selected="true"][data-active="true"] (and also adjust
.rad-ui-select-item-indicator if needed) to set background-color:
var(--rad-ui-surface-subtle), color: var(--rad-ui-text-primary) and opacity for
the indicator, ensuring hovered/active selected items show the highlight instead
of surface-muted.
In `@styles/themes/components/slider.scss`:
- Around line 133-136: The .rad-ui-slider-mark-label in the vertical slider
variant lacks a position, so its top/right rules are ineffective while
transform: translateY(-50%) incorrectly shifts it; update the
.rad-ui-slider-mark-label styling used by the vertical slider to remove
top/right/transform and instead rely on the flex row layout by using margin
(e.g., margin-right or margin-left as appropriate) and
align-items/justify-content to vertically center the label; adjust any parent
.rad-ui-slider (or vertical variant selector) to ensure the label is placed in
the flex row and uses margin-based spacing rather than absolute positioning.
In `@styles/themes/components/splitter.scss`:
- Around line 59-70: The ::after pseudo-element still has transitions so users
with prefers-reduced-motion still see the fade; update the
prefers-reduced-motion media query that currently targets the handle to also
target the handle::after (and any other pseudo-elements used, e.g., the visible
indicator) and disable its animations/transitions (set transition: none or
remove opacity/background-color transitions) for the selectors that include
::after (references: the rule containing &::after and the other blocks around
lines 111-123 and 144-151).
In `@styles/themes/components/steps.scss`:
- Around line 50-55: In the &[data-state="completed"] rule for
.rad-ui-steps-bubble replace the hardcoded "color: white" with the semantic
token --rad-ui-text-inverse; update the declaration to use
var(--rad-ui-text-inverse) so text on accent backgrounds uses the design system
token (match the pattern used in button.scss and tooltip.scss).
In `@styles/themes/components/switch.scss`:
- Around line 1-12: The .rad-ui-switch rule uses all: unset which resets
box-sizing to content-box and causes the component width to grow by borders; add
box-sizing: border-box immediately after all: unset in the .rad-ui-switch
selector to ensure borders are included in the width so the checked thumb (the
switch's thumb translateX value used in the Checked story) aligns correctly with
the right edge; after adding it, verify the Checked story thumb position and
adjust translateX only if needed.
In `@styles/themes/components/table.scss`:
- Line 2: The table wrapper's border uses a nonexistent token (--rad-ui-border);
replace it with an existing semantic border token such as --rad-ui-border-soft
(or more specific variant --rad-ui-border-soft-default or
--rad-ui-border-soft-strong as appropriate for visual weight) in the rule that
sets "border: 1px solid var(--rad-ui-border)" so the declaration never drops
when the token is undefined and matches the project's semantic token naming.
In `@styles/themes/components/textarea.scss`:
- Around line 21-24: The ::placeholder rule in the textarea component currently
hard-codes font-size: 1rem which breaks the small/large size variants; update
the &::placeholder selector in styles/themes/components/textarea.scss to use
font-size: inherit (or remove the font-size declaration) so the placeholder
inherits the component's current size variant, and apply the same change to the
duplicate placeholder block around lines 62-76 to keep behavior consistent.
In `@styles/themes/components/toolbar.scss`:
- Around line 100-107: The hover rule currently overrides the selected rule
because both selectors (&[data-state='on'] and &:hover) have equal specificity;
update the CSS so hover styling does not apply when the item is selected — e.g.
change the hover selector to only target non-selected items (use
&:hover:not([data-state='on']) or add a specific rule for
&[data-state='on']:hover to preserve its background) so the &[data-state='on']
accent background and color remain visible while hovering.
---
Nitpick comments:
In `@src/components/ui/Card/fragments/CardRoot.tsx`:
- Around line 17-23: The CardContext.Provider is being passed an inline object
value {{ rootClass }} which creates a new reference each render and can cause
unnecessary re-renders in consumers; fix this in CardRoot (the component
rendering CardContext.Provider) by memoizing the context value using
React.useMemo (e.g., const value = useMemo(() => ({ rootClass }), [rootClass]))
and pass that memoized value to CardContext.Provider instead of the inline
object; ensure you import useMemo from React if not already.
In `@src/components/ui/Card/stories/Card.stories.tsx`:
- Around line 29-47: Replace hardcoded Tailwind color classes in the
Card.stories form inputs (e.g., text-gray-950, border-gray-300, bg-gray-50 used
on the input and label elements) with the project’s semantic design tokens or
the shared Input component so colors adapt to theme changes; either swap the
class names for token-based classes (e.g., token-text, token-border, token-bg
equivalents) or refactor the repeated input markup into the existing Input
component and use that in Card.stories.tsx to ensure consistent theming.
In `@src/components/ui/Slider/fragments/SliderRangeSlider.tsx`:
- Around line 204-206: Replace the hard-coded "8px" thumb offset in
SliderRangeSlider.tsx with a single shared value: define a constant or CSS
custom property (e.g., THUMB_HALF_SIZE or --thumb-half-size) used wherever the
thumb is centered, and update the two places that compute positions (the style
objects using percent1 and percent2 with orientation checks) to use that shared
value instead of 8px so future thumb-size changes only require one edit.
In `@src/components/ui/Slider/stories/Slider.stories.tsx`:
- Around line 10-15: Add a new story named VerticalWithMarks to this file that
renders the Slider in the vertical+marks branch so regressions are caught;
locate the stories block (where Basic.render and SandboxEditor are used) and
create VerticalWithMarks that returns a SandboxEditor wrapping a container with
a fixed height (so vertical layout is visible) and a Slider configured for
vertical orientation and marks (e.g., set the Slider's vertical/orientation prop
and a marks array and a defaultValue) so the story exercises the vertical and
marks code paths.
In `@src/components/ui/TextArea/stories/TextArea.stories.tsx`:
- Around line 12-24: The story currently uses aria-label on <TextArea> while the
visible "Message" label and helper text aren't programmatically attached; update
the story to demonstrate a real label/description pair by rendering the visible
label text as the form label (use the TextArea's label prop or wrap with a
<label> tied to the textarea id) and give the helper copy an id (e.g.,
message-desc) then pass that id into TextArea via aria-describedby so the label
and helper are correctly associated with the textarea; update the
placeholders/props accordingly to remove the redundant aria-label.
In `@src/components/ui/Theme/Theme.tsx`:
- Around line 40-43: The current useCallback for setRefs includes the
parent-provided ref in its dependency array which can recreate the callback when
a new ref object/function is passed; fix this by storing the latest ref in a
stable ref and using that inside a deps-free callback: create a ref holder
(e.g., refRef = useRef(ref)), update refRef.current via useEffect whenever prop
ref changes, then define setRefs with useCallback((node) => {
containerRef.current = node; assignRef(refRef.current, node); }, []) so setRefs
identity stays stable while still using the latest parent ref; reference
symbols: setRefs, containerRef, assignRef, ref, refRef.
In `@src/components/ui/Tooltip/fragments/TooltipContent.tsx`:
- Line 35: TooltipContent uses a simpler portal root fallback (FloatingPortal
root={themeContext?.portalRootRef.current || themeContext?.containerRef.current
|| undefined}) that is inconsistent with HoverCardPortal and
DialogPrimitivePortal; update TooltipContent to use the same fallback lookup
those components use (check for portalRootRef, containerRef, then query the DOM
for [data-rad-ui-portal-root] and any other selectors used by
HoverCardPortal/DialogPrimitivePortal) or extract and call the shared utility
resolver (e.g., getPortalRoot or resolvePortalRoot) so FloatingPortal receives
the identical resolved root as other portal components.
In `@styles/themes/components/alert-dialog.scss`:
- Around line 100-123: Extract the duplicated styles for
.rad-ui-alert-dialog-action and .rad-ui-alert-dialog-cancel into a single shared
placeholder/utility and have both classes extend it: create a placeholder (e.g.,
%rad-ui-alert-dialog-button-skin) that contains the common base rules and the
shared &:hover rules, then replace the duplicated rule sets by `@extend-ing` that
placeholder in both .rad-ui-alert-dialog-action and .rad-ui-alert-dialog-cancel
(apply same change to the other duplicated pair mentioned). Preserve any unique
properties by keeping them in the specific classes and only move truly identical
declarations into the shared placeholder so future token changes stay in one
place.
In `@styles/themes/components/avatar-group.scss`:
- Around line 74-79: The duplicated box-shadow on the more specific selector
&[data-slot="avatar"] .rad-ui-avatar-group-item appears redundant because
.rad-ui-avatar-group-item already sets the same box-shadow; either remove the
entire &[data-slot="avatar"] block or, if it exists to re-apply the shadow after
other overrides, leave it and add a concise comment above the block explaining
that it intentionally restores the 0 0 0 2px var(--rad-ui-surface-panel)
box-shadow for grouped avatar items (reference selector
.rad-ui-avatar-group-item and attribute selector [data-slot="avatar"]).
In `@styles/themes/components/card.scss`:
- Around line 12-16: The outline variant selector &[data-card-variant="outline"]
repeats background-color and border-color that are already defined on the base
.rad-ui-card; remove the redundant background-color and border-color
declarations from the &[data-card-variant="outline"] block and keep only
box-shadow: none so the variant inherits the base .rad-ui-card colors while
overriding the shadow.
In `@styles/themes/components/code.scss`:
- Around line 18-22: The soft variant block &[data-code-variant="soft"] in
styles/themes/components/code.scss is redundant because it duplicates the base
.rad-ui-code styles; either remove the entire &[data-code-variant="soft"] rule
so consumers inherit the base styles, or modify it to be visually distinct (for
example change background-color to var(--rad-ui-color-accent-50) and/or
remove/alter border like no border or a lighter border color) — update the
selector block for &[data-code-variant="soft"] accordingly to implement one of
these two options.
In `@styles/themes/components/dialog.scss`:
- Line 44: Remove the redundant "gap: 0" declaration in
styles/themes/components/dialog.scss (the "gap: 0" property shown in the dialog
styles) since flex containers default to gap: 0; only keep it if you are
intentionally overriding an earlier non-zero value—otherwise delete the line to
simplify the stylesheet.
In `@styles/themes/components/navigation-menu.scss`:
- Around line 32-44: The open-state box-shadow is snapping because the
transition declaration on navigation-menu.scss (currently on the selector with
&:hover) omits box-shadow; update the transition to include box-shadow (e.g.,
add "box-shadow 0.2s" to the transition property) so the &[data-state="open"]
change animates smoothly alongside background and color.
- Around line 71-83: Replace the hardcoded border-radius value in the
.rad-ui-navigation-menu-link rule with the theme radius token; locate the
.rad-ui-navigation-menu-link selector and change border-radius: 4px to use the
project radius CSS variable (e.g. var(--rad-ui-radius-1) or the established
radius token name) so the component follows the theming tokens.
- Around line 90-93: The rule targeting &[aria-current="page"],
&[data-active="true"] uses the accent token --rad-ui-color-accent-100; replace
that token with the semantic token --rad-ui-surface-subtle to match select.scss
and the design system. Update the background value in the navigation-menu.scss
selector (the &[aria-current="page"], &[data-active="true"] block) to use
var(--rad-ui-surface-subtle) so active/selected styling is consistent across
themes.
In `@styles/themes/components/number-field.scss`:
- Line 63: The .rad-ui-number-field-decrement nested selector is missing the
leading space used by other nested selectors, making the nesting unclear; update
the indentation of the .rad-ui-number-field-decrement block to match the nesting
level of .rad-ui-number-field-input and .rad-ui-number-field-increment so it
aligns with the other nested selectors and maintains consistent SCSS structure.
- Around line 36-87: Both .rad-ui-number-field-increment and
.rad-ui-number-field-decrement contain identical rules; consolidate them by
replacing the two duplicate blocks with a single combined selector
".rad-ui-number-field-increment, .rad-ui-number-field-decrement { ... }" that
contains the shared properties and nested &:hover and &:focus-visible states,
then remove the redundant separate block so maintenance is easier and
duplication is eliminated.
In `@styles/themes/components/radio-group.scss`:
- Around line 26-30: The rule currently applies both &:focus and &:focus-visible
so the focus ring appears on mouse clicks as well as keyboard navigation; change
the selector to only use &:focus-visible in the radio-group styles block
(replace the combined &:focus, &:focus-visible rule with a single
&:focus-visible rule that keeps outline: none and box-shadow:
var(--rad-ui-ring-shadow-sm)) so focus rings are shown only for keyboard users.
In `@styles/themes/components/radiocards.scss`:
- Around line 1-6: The .rad-ui-radio-cards selector currently hardcodes
max-width: 29rem which limits reuse; make this configurable by replacing the
hardcoded value with a CSS custom property (e.g., use
var(--rad-ui-radio-cards-max-width, 29rem)) so callers can override it, or
alternatively add documentation/comments near .rad-ui-radio-cards describing the
intentional constraint and default value; update the SCSS for the rule and any
relevant component docs/readme to reflect the new CSS variable name and default.
In `@styles/themes/components/tabs.scss`:
- Around line 44-48: Replace the hardcoded focus-visible box-shadow rgba values
in the tabs component with the theme tokens used elsewhere so the ring adapts to
theme/mode changes: locate the box-shadow definition in
styles/themes/components/tabs.scss (the block setting box-shadow for focus/
focus-visible) and swap the rgba entries for the corresponding design tokens
(e.g., use var(--rad-ui-ring-shadow-sm) and/or var(--rad-ui-ring-shadow) in
place of the rgba(...) layers) so it matches radio-group.scss / radiocards.scss
token usage and inherits theme colors.
- Around line 60-66: The active+focus-visible rule (&.active:focus-visible)
currently uses a hardcoded box-shadow; replace it with the same token-based
focus-ring values used elsewhere in the theme (use the shared focus/ring tokens
used for regular :focus-visible rules) so the ring color, offset and blur come
from theme variables rather than literals, updating the &.active:focus-visible
declaration in styles/themes/components/tabs.scss to reference those tokens for
consistency.
In `@styles/themes/components/toggle.scss`:
- Around line 18-28: The hover selector (&:hover) and the pressed selectors
(&[data-state="on"], &[aria-pressed="true"]) currently use the same
background-color and border-color, so update the pressed state styling in
components/toggle.scss to use a distinct token (e.g., a stronger surface or
border token) or adjust color intensity so pressed has a clearer visual
difference from hover; locate the selectors "&:hover" and the group
"&[data-state=\"on\"], &[aria-pressed=\"true\"]" and change the background-color
and/or border-color values for the pressed group to a stronger/different CSS
variable (and ensure color contrast via color: var(--rad-ui-text-primary)
remains appropriate).
In `@styles/themes/default.scss`:
- Around line 91-95: The portal root CSS hardcodes z-index: 1000 on the
[data-rad-ui-portal-root] selector; replace this hardcoded value with a
theme/token variable so host apps can override overlay stacking. Update the
styles to reference your design token (e.g., $z-index-portal or
var(--z-index-portal)) and ensure the token exists in the theme tokens file and
is exported where themes are composed; adjust any places that import or use
[data-rad-ui-portal-root] to rely on the token rather than the numeric literal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
styles/themes/components/minimap.scss (1)
56-66:⚠️ Potential issue | 🟡 MinorDuplicate
padding-bottomdeclaration.Line 61 sets
padding-bottom: 0px;but line 65 setspadding-bottom: 20px;, making line 61 dead code.🧹 Suggested fix to remove redundant property
padding-left: 10px; display: flex; flex-direction: column; gap: 4px; - padding-bottom: 0px; color: var(--rad-ui-text-secondary); font-size: 0.875rem; padding-top: 5px; padding-bottom: 20px;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/minimap.scss` around lines 56 - 66, The .rad-ui-minimap-content block contains a duplicated padding-bottom (0px at the earlier line and 20px later); remove the redundant padding-bottom: 0px declaration so only the intended padding-bottom: 20px remains in the .rad-ui-minimap-content rule to avoid dead code and ensure consistent styling.
♻️ Duplicate comments (1)
styles/themes/components/context-menu.scss (1)
14-16:⚠️ Potential issue | 🟠 MajorMove demo-sized trigger dimensions out of shared theme styles.
Line 14–16 still force every
.rad-ui-context-menu-triggerinto a large canvas, which breaks normal inline/custom trigger composition.Suggested fix
.rad-ui-context-menu-trigger { @@ - padding: 6rem 1.5rem; - width: 24rem; - min-height: 13.5rem; + padding: 1rem 1.25rem;Use a story/docs-only modifier class (or CSS variable override) for the 24rem × 13.5rem demo sizing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/context-menu.scss` around lines 14 - 16, The shared theme file currently forces demo dimensions on the component by applying padding/width/min-height in context-menu.scss to the default selector (affecting .rad-ui-context-menu-trigger); remove those demo sizes from the shared rule and instead add a story/docs-only modifier or CSS-variable override (e.g., .rad-ui-context-menu-trigger--demo or --rad-context-menu-demo-width/height) and update the demo/story styles to apply that modifier/variables so inline/custom triggers keep normal sizing while the demo uses 24rem × 13.5rem.
🧹 Nitpick comments (15)
styles/themes/components/tabs.scss (1)
38-65: Hardcoded focus ring colors won't adapt to theme changes.The
rgba(17, 24, 39, ...)values are hardcoded dark gray (#111827) which will look inconsistent in different themes, particularly light vs dark mode. Consider using a CSS custom property for the focus ring color to maintain theming consistency with the rest of the file.♻️ Suggested refactor using theme tokens
&:focus-visible{ z-index: 2; color: var(--rad-ui-text-primary); background-color: var(--rad-ui-surface-panel); outline: none; box-shadow: - 0 0 0 3px rgba(17, 24, 39, 0.3), - 0 0 0 4px rgba(17, 24, 39, 0.1), + 0 0 0 3px var(--rad-ui-focus-ring, rgba(17, 24, 39, 0.3)), + 0 0 0 4px var(--rad-ui-focus-ring-outer, rgba(17, 24, 39, 0.1)), 0 1px 2px rgba(0, 0, 0, 0.05), 0 2px 6px rgba(0, 0, 0, 0.04); }Apply similar changes to
&.active:focus-visibleblock (lines 59-65).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/tabs.scss` around lines 38 - 65, The focus ring color is hardcoded as rgba(17,24,39,...) in the &:focus-visible, &.active and &.active:focus-visible blocks which breaks theming; replace those rgba(...) color literals with a CSS custom property (e.g. var(--rad-ui-focus-ring-color, rgba(17,24,39,0.3)) and similar fallbacks for the other alpha stops) so the ring adapts to theme tokens, and ensure a sensible default is declared in your theme tokens file or a root selector so existing visuals remain unchanged if the token is missing.knowledge/design_system/clarity_design_system.md (2)
25-38: Fix markdown table formatting to satisfy lint and avoid rendering issues.The table has missing trailing pipes on some rows and should be separated by blank lines. This will keep markdownlint clean and improve consistency. (e.g., Lines 34, 35, and spacing around the table near Line 37).
Proposed doc-format fix
### 2. Intent-Based Design (VERY IMPORTANT) - | Step | Token | Category | Usage Description | |------|------|---------------------------|------------------| | 1 | 50 | Backgrounds | App/page background, lowest visual weight | | 2 | 100 | Backgrounds | Subtle surfaces, section backgrounds | | 3 | 200 | Interactive components | Hover backgrounds, subtle interaction states | | 4 | 300 | Interactive components | Active/pressed states, stronger surface contrast | | 5 | 400 | Interactive components | Selected states, emphasized surfaces | | 6 | 500 | Borders & separators | Light borders, dividers | | 7 | 600 | Borders & separators | Default borders, stronger separators | -| 8 | 700 | Solid colors | Disabled text/icons, muted UI elements -| 9 | 800 | Solid colors | supporting content +| 8 | 700 | Solid colors | Disabled text/icons, muted UI elements | +| 9 | 800 | Solid colors | Supporting content | | 10 | 900 | Accessible text | Secondary text, high readability | | 11 | 1000 | Accessible text | Headings,Primary text, max contrast text (near black) | + ---🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge/design_system/clarity_design_system.md` around lines 25 - 38, The markdown table in clarity_design_system.md has malformed rows (missing trailing "|" on some rows) and lacks blank lines before and after the table which breaks markdownlint; fix it by adding the missing trailing pipe characters to every table row (ensure each row, e.g., the rows for steps 8, 9, 10, 11, etc., ends with "|"), verify the header separator row is intact, and add a blank line both immediately before the table and immediately after the trailing "---" separator so the table renders cleanly and passes linting.
63-68: Make accessibility guidance testable with explicit contrast criteria.“Must meet WCAG AA” is good, but this section should specify concrete ratio targets for intended token pairs to prevent inconsistent usage.
Proposed clarity addition
### 5. Accessibility First - Text (900–1000) must meet **WCAG AA** - Borders (400–500) must be visible but not distracting -- Avoid low contrast adjacent steps +- Avoid low-contrast adjacent steps +- Define and validate target pairs (example): + - Body text: 900 on 50/100 ≥ 4.5:1 + - Large text/headings: 1000 on 50/100 ≥ 3:1 + - Borders: 500 on 50/100 should remain visually distinct in both light and dark themes🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge/design_system/clarity_design_system.md` around lines 63 - 68, The "Accessibility First" section (### 5. Accessibility First) needs explicit, testable contrast criteria instead of just “must meet WCAG AA”: add concrete ratio targets for the referenced tokens (Text (900–1000), Borders (400–500)) and common token pairs — e.g., require text (normal) to be >=4.5:1 on its intended background, large text >=3:1, UI components and primary controls >=3:1, and borders (400–500) to be >=3:1 against adjacent surfaces; also call out minimum contrast deltas for adjacent steps (suggest >=3:1) and state that all ratios must be measurable with standard tools (e.g., WCAG contrast checkers) so designers can validate token usage.styles/themes/components/minimap.scss (1)
30-38: Incomplete transition properties for in-view state.The transition animates
colorandbox-shadow, but the[data-in-view="true"]state also changesborder-colorandbackground-color. These will snap instantly while other properties animate smoothly, creating a jarring visual effect.♻️ Suggested fix to add missing transition properties
transition: color 0.3s ease-in-out, + background-color 0.3s ease-in-out, + border-color 0.3s ease-in-out, box-shadow 0.3s ease-in-out;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/minimap.scss` around lines 30 - 38, The transition currently only lists color and box-shadow, but the &[data-in-view="true"] rule also changes border-color and background-color; update the transition declaration (the block containing "transition: color 0.3s ease-in-out, box-shadow 0.3s ease-in-out;") to include border-color and background-color so those properties animate as well when the selector &[data-in-view="true"] toggles.src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveContent.tsx (1)
134-134: IncludingshouldRenderin the dependency array may cause redundant effect executions.When the closing animation completes,
setShouldRender(false)at line 121 triggers a re-run of this effect. At that point,openis stillfalse, so the closing branch (lines 107-124) executes again on content that's already collapsed. While the cleanup on lines 126-132 prevents the old timeout from firing, the effect still performs unnecessary DOM reads (scrollHeight) and schedules new animation frames.Consider removing
shouldRenderfrom the dependency array or adding an early return guard:Option 1: Remove shouldRender from dependencies
- }, [open, transitionDuration, forceMount, shouldRender]); + }, [open, transitionDuration, forceMount]);Option 2: Add early return guard
if (!ref.current) return; + + // Skip if already unmounted (closing animation completed) + if (!open && !shouldRender && !forceMount) return; if (transitionDuration === 0) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveContent.tsx` at line 134, The effect is re-running unnecessarily because shouldRender is included in the dependency array; either remove shouldRender from the array or add an early-return guard at the top of the useEffect so it bails when both open is false and shouldRender is false. Specifically, in the effect that references open, transitionDuration, forceMount, shouldRender (around the block that calls setShouldRender(false) and uses scrollHeight/animation frames), remove shouldRender from the dependency list or add: if (!open && !shouldRender) return; to avoid extra DOM reads/RAF scheduling; keep other dependencies (open, transitionDuration, forceMount) intact and ensure any timers/RAFs still get cleaned up in the existing cleanup block.styles/themes/components/toggle.scss (1)
17-24: Consider transitioning the properties you mutate on hover.Hover changes
background-colorandborder-color, but transition currently excludes both, so state change is abrupt.Optional tweak
transition: color 150ms cubic-bezier(0.4, 0, 0.2, 1), - box-shadow 150ms cubic-bezier(0.4, 0, 0.2, 1); + background-color 150ms cubic-bezier(0.4, 0, 0.2, 1), + border-color 150ms cubic-bezier(0.4, 0, 0.2, 1), + box-shadow 150ms cubic-bezier(0.4, 0, 0.2, 1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/toggle.scss` around lines 17 - 24, The transition declaration currently only lists color and box-shadow but the :hover block mutates background-color and border-color; update the transition on the relevant selector (the rule surrounding transition and &:hover) to include background-color and border-color (e.g., add "background-color 150ms ..." and "border-color 150ms ...") so hover state changes animate smoothly rather than abruptly.styles/themes/components/slider.scss (3)
78-79: Redundant state selectors with no visual differentiation.Both
[data-state="dragging"]and[data-state="active"]apply the sametransform: translateY(-50%)that's already set on the base selector (line 63). If these are placeholders for future styling, consider adding a comment. Otherwise, they can be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/slider.scss` around lines 78 - 79, The two redundant rules [&data-state="dragging"] and [&data-state="active"] in slider.scss duplicate the base selector's transform (translateY(-50%)); either remove these duplicate selectors to clean up the CSS or, if kept as placeholders for future state-specific styling, replace their bodies with a brief comment noting they are intentional placeholders (e.g., "TODO: state-specific styles") so reviewers know they are deliberate—locate the rules by the selectors [data-state="dragging"] and [data-state="active"] and update accordingly.
50-52: Consider using attribute selector instead of listing class variants.The current approach lists all thumb class variants explicitly. If new variants are added, this selector needs updating. Using an attribute selector or a more flexible pattern would be more maintainable.
♻️ Alternative: Use attribute selector
- .rad-ui-slider-thumb, - .rad-ui-slider-thumb-lower, - .rad-ui-slider-thumb-upper { + [class^="rad-ui-slider-thumb"], + [class*=" rad-ui-slider-thumb"] {Or keep the explicit listing if you want strict control over which elements receive these styles.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/slider.scss` around lines 50 - 52, Replace the explicit multi-class selector (.rad-ui-slider-thumb, .rad-ui-slider-thumb-lower, .rad-ui-slider-thumb-upper) with a single, more maintainable selector that matches all thumb variants (e.g., an attribute or prefix-based selector such as [class^="rad-ui-slider-thumb"] or a custom data-* attribute like [data-rad-ui-thumb]) so new variants inherit the styles without editing this rule; update any markup to include the chosen attribute if using data-* and ensure the rule still targets only slider thumbs (adjust specificity if needed).
161-175: Duplicate thumb selectors in vertical orientation.Lines 161-175 repeat the thumb selector list and include the same redundant state selectors (lines 168-169) as the horizontal variant. Consider extracting shared thumb properties to reduce duplication, or use a mixin if the SCSS setup supports it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/slider.scss` around lines 161 - 175, The vertical thumb block duplicates the same selectors and state rules as the horizontal one (.rad-ui-slider-thumb, .rad-ui-slider-thumb-lower, .rad-ui-slider-thumb-upper with &[data-state="dragging"], &[data-state="active"], and &:focus-visible); extract the shared properties into a reusable SCSS construct (a mixin or placeholder selector) and include it from both the horizontal and vertical orientation blocks, leaving only orientation-specific rules (e.g., left/top/transform differences) in each block and removing the redundant state selectors from the vertical section.styles/themes/components/table.scss (1)
36-42: Hover state has no visual transition.The row hover changes
background-colorbut lacks atransitionproperty, causing an abrupt color change. Header cells at line 30 also use--rad-ui-surface-panel, which means both header and row share the same background—the hover won't visually distinguish rows from headers.🎨 Add transition for smoother hover effect
.row{ vertical-align: inherit; border-bottom: 1px solid var(--rad-ui-border-soft); background-color: var(--rad-ui-surface-panel); + transition: background-color 150ms ease; &:hover{ background-color: var(--rad-ui-surface-subtle); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/table.scss` around lines 36 - 42, Add a background-color transition to the row hover rule to smooth the change (e.g., transition: background-color 150ms ease) by updating the block containing &:hover; also ensure header cells do not share the same background variable as rows—either change the header cell background from --rad-ui-surface-panel to a distinct token (e.g., --rad-ui-surface-header) or pick a hover color variable different from the header so the &:hover background-color: var(--rad-ui-surface-subtle) is visually distinct.styles/cssTokens/base.tokens.css (2)
813-825: Mixed color format between light and dark mode gray tokens.Light mode gray tokens now use hex values (lines 3-14), while dark mode continues to use
oklch()(lines 814-825). This creates a maintenance inconsistency—if you need to adjust a gray shade, you'll need to work with different color spaces.Consider whether both modes should use the same format for consistency, or document the rationale for the mixed approach.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/cssTokens/base.tokens.css` around lines 813 - 825, The dark theme gray tokens (--rad-ui-color-gray-50 through --rad-ui-color-gray-1000) use oklch() while the light tokens use hex, causing inconsistent color formats; pick a single color format (either hex or oklch) and convert the dark-theme token values to that format (or convert light tokens to oklch) so all --rad-ui-color-gray-* variables use the same notation, then update any documentation or a short inline comment explaining the chosen convention; locate and edit the block containing [data-rad-ui-theme="dark"] and the specific variables --rad-ui-color-gray-50 ... --rad-ui-color-gray-1000 to apply the converted values.
3-14: Inconsistent indentation in gray token declarations.Line 3 has proper indentation inside the
:rootblock, but lines 4-14 are left-aligned. This breaks the formatting consistency with the rest of the file.🔧 Proposed fix for indentation
:root { --rad-ui-color-gray-50: `#fafafa`; ---rad-ui-color-gray-100: `#f4f4f5`; ---rad-ui-color-gray-200: `#e4e4e7`; ---rad-ui-color-gray-300: `#d4d4d8`; ---rad-ui-color-gray-400: `#a1a1aa`; ---rad-ui-color-gray-500: `#71717a`; ---rad-ui-color-gray-600: `#52525b`; ---rad-ui-color-gray-700: `#3f3f46`; ---rad-ui-color-gray-800: `#27272a`; ---rad-ui-color-gray-900: `#18181b`; ---rad-ui-color-gray-950: `#141416`; ---rad-ui-color-gray-1000: `#0f0f11`; + --rad-ui-color-gray-100: `#f4f4f5`; + --rad-ui-color-gray-200: `#e4e4e7`; + --rad-ui-color-gray-300: `#d4d4d8`; + --rad-ui-color-gray-400: `#a1a1aa`; + --rad-ui-color-gray-500: `#71717a`; + --rad-ui-color-gray-600: `#52525b`; + --rad-ui-color-gray-700: `#3f3f46`; + --rad-ui-color-gray-800: `#27272a`; + --rad-ui-color-gray-900: `#18181b`; + --rad-ui-color-gray-950: `#141416`; + --rad-ui-color-gray-1000: `#0f0f11`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/cssTokens/base.tokens.css` around lines 3 - 14, The gray color CSS custom properties inside the :root block are misaligned; please align the declarations (--rad-ui-color-gray-100 through --rad-ui-color-gray-1000) to match the indentation of the first token (--rad-ui-color-gray-50) so all lines share the same indentation level and spacing style, e.g., indent each line to the same column within the :root block and keep one property per line.styles/themes/components/alert-dialog.scss (2)
100-124: Consider consolidating duplicate action/cancel button styles.The
.rad-ui-alert-dialog-actionand.rad-ui-alert-dialog-cancelstyles are duplicated between the nested context (lines 100-124) and standalone context (lines 149-173). This duplication increases maintenance burden.You could use SCSS placeholder selectors or mixins to DRY this up:
♻️ Example using placeholder selectors
// Define once at top of file %alert-dialog-action-base { `@extend` .rad-ui-button; background-color: var(--rad-ui-text-primary); color: var(--rad-ui-surface-panel); border: 1px solid var(--rad-ui-text-primary); &:hover { background-color: color-mix(in oklab, var(--rad-ui-text-primary) 92%, var(--rad-ui-color-gray-1000)); border-color: color-mix(in oklab, var(--rad-ui-text-primary) 92%, var(--rad-ui-color-gray-1000)); } } %alert-dialog-cancel-base { `@extend` .rad-ui-button; background-color: var(--rad-ui-surface-panel); color: var(--rad-ui-text-primary); border: 1px solid var(--rad-ui-border-soft); box-shadow: var(--rad-ui-shadow-xs); &:hover { background-color: var(--rad-ui-surface-subtle); border-color: var(--rad-ui-border); color: var(--rad-ui-text-primary); } } // Then extend in both locations .rad-ui-alert-dialog-footer .rad-ui-alert-dialog-action { `@extend` %alert-dialog-action-base; } .rad-ui-alert-dialog-action { `@extend` %alert-dialog-action-base; }Also applies to: 149-173
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/alert-dialog.scss` around lines 100 - 124, The .rad-ui-alert-dialog-action and .rad-ui-alert-dialog-cancel rules are duplicated in both the nested and standalone contexts; define shared SCSS placeholders or mixins (e.g., %alert-dialog-action-base and %alert-dialog-cancel-base) that contain the common properties (including &:hover blocks) and replace the duplicated blocks by extending/including these placeholders wherever .rad-ui-alert-dialog-action and .rad-ui-alert-dialog-cancel appear (both the nested footer variant and the top-level selectors) so the styles are centralized and maintained in one place.
106-109: Be aware thatcolor-mix()lacks support in older browsers.
color-mix(in oklab, ...)is a CSS Color Level 5 feature supported in Chrome/Edge 111+ (March 2023), Firefox 113+ (May 2023), and Safari 16.2+ (October 2022). As of 2026, this covers over 93% of global browser usage. Older browsers will skip this declaration, resulting in no hover color change on the action button.If your project targets older browsers, consider adding a fallback color or using a CSS custom property instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/alert-dialog.scss` around lines 106 - 109, The hover rule using color-mix(in oklab, ...) may be ignored in older browsers; update the &:hover block in styles/themes/components/alert-dialog.scss (the selector using &:hover for the action button) to provide a compatible fallback before the color-mix declaration — e.g., set a static background-color and border-color (or use an existing CSS custom property like --rad-ui-color-gray-1000 or a derived var(--rad-ui-hover)) and then keep the color-mix rule after it so modern browsers use the advanced mix while legacy browsers pick the fallback.styles/themes/components/radio-group.scss (1)
10-13: Reconsiderwidth: max-contenton the label.Line 13 forces the label to its unwrapped width, so longer or localized option text will overflow instead of wrapping, and the hit area shrinks to the content width. If this row should stay responsive, leaving the width auto is safer.
Please verify this with a long label in a narrow container or Storybook viewport.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/radio-group.scss` around lines 10 - 13, The rule in radio-group.scss that sets width: max-content forces labels to their unwrapped width causing overflow and small hit areas; locate the CSS block containing display: inline-flex; align-items: center; gap: 0.75rem; width: max-content and replace/remove width: max-content (use width: auto or remove it) and optionally add max-width: 100% to allow wrapping and keep responsiveness; then verify the change by testing a long radio label in a narrow container or Storybook viewport to ensure it wraps and the hit area expands correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@knowledge/design_system/clarity_design_system.md`:
- Around line 49-57: The phrase "slightly tinted white" conflicts with the "###
4. Neutral Only" rule (which requires true grayscale); update the wording so
both places are consistent—either remove "slightly tinted white" and replace it
with "neutral white (no tint)" or change the "Neutral Only" bullets to allow the
specific tint; locate the string "slightly tinted white" and the "### 4. Neutral
Only" section and make them use the same explicit wording (e.g., "neutral white
— no hue tint") to eliminate the semantic conflict.
In `@styles/themes/components/alert-dialog.scss`:
- Around line 94-98: The media queries for `(width >= 640px)` and `(width <=
640px)` overlap at exactly 640px; update them to create exclusive ranges—e.g.,
change the mobile query `(width <= 640px)` to `(width < 640px)` (or change the
desktop query to `(width >= 641px)`) so the rules in the alert-dialog styles no
longer conflict at 640px; adjust both occurrences (the block starting with
`@media (width >= 640px)` and the block starting with `@media (width <= 640px)`)
accordingly.
In `@styles/themes/components/card.scss`:
- Around line 77-115: Nested cards are picking up outer size rules because
selectors like &[data-card-size="small"] .rad-ui-card-content and
&[data-card-size="x-large"] .rad-ui-card-footer use descendant combinators;
change them to target only the current card’s direct children (use the child
combinator, e.g. &> .rad-ui-card-content and &> .rad-ui-card-footer) for all
size overrides (small, large, x-large) so outer card sizes cannot bleed into
nested cards.
In `@styles/themes/components/fix_tokens.cjs`:
- Line 4: The file hardcodes a machine-local path in the const dir variable and
immediately uses it; replace the hardcoded string for dir with a resolved path
built from __dirname or process.cwd() (using path.resolve/path.join) and allow
an override via an environment variable (e.g., process.env.COMPONENTS_DIR) so
the script is portable, and stop executing side-effects at module load — instead
compute or export a getter (e.g., getComponentsDir) or defer using dir until a
function is called so consumers can set overrides before use; update references
to dir accordingly.
- Around line 10-11: The current raw replacements (content.replace(/-950/g,
'-1000') and the similar 'gray-200' replacement) are too broad and mutate
unintended text; update these to use more specific regexes with
lookbehind/lookahead that target CSS variable declarations like the existing
patterns in this file (e.g., the surrounding variable-matching logic between
lines 14–23) so they only match values inside --variable: ... or token keys, not
arbitrary occurrences (use the same style of (?<=--[\w-]+:\s*)... or lookarounds
that assert variable context); keep the replacements applied to the same content
variable and preserve global flag semantics where appropriate.
- Around line 14-18: The two color-rewrite replace calls that currently use
(?<=color:\s*var\(--rad-ui-color-...) should be tightened so they only match the
standalone "color:" property and not compound properties like
"background-color:" or "outline-color:"; update the lookbehind in the two
replace patterns (the content.replace calls matching 600→700 and 800→900) to
require a boundary/anchor before "color:" (e.g. ensure "color:" is preceded by a
non-word/non-dash or a word boundary) so those replaces only run on actual
"color:" declarations and the later background-color replace (500|600|900→800)
can apply as intended.
In `@styles/themes/components/radio-group.scss`:
- Around line 47-49: The hover rule currently applies to disabled radio items;
update the selector that currently uses &:hover to exclude disabled items by
changing it to &:not([data-disabled]):hover (so the border-color change only
applies to interactive radios). Locate the rule using &:hover in
radio-group.scss and replace it with the :not([data-disabled]) guard around the
hover selector.
- Around line 51-60: The checked-state rule &[data-state="checked"] currently
comes after :focus-visible and hides the keyboard focus ring; add a combined
selector &[data-state="checked"]:focus-visible that merges the checked
box-shadow (the inset 1px color-mix shadow and subtle drop shadow) with the
focus ring (var(--rad-ui-ring-shadow-sm)) so both shadows are preserved (place
the combined selector after the existing rules or include it alongside the
checked rule) — reference the selectors &:focus-visible,
&[data-state="checked"], and add &[data-state="checked"]:focus-visible to apply
both shadows together.
In `@styles/themes/components/radiocards.scss`:
- Around line 22-24: The hardcoded rgba(255, 255, 255, 0.52) used in the
box-shadow in radiocards.scss causes light-only highlights that break dark mode;
replace each hardcoded white value (the box-shadow occurrences around the shown
block and the other instances) with a theme-aware token or dynamic color
expression such as var(--rad-ui-highlight-inset) or a semi-transparent
surface-based color (e.g., using color-mix() or rgba(var(--rad-ui-surface-rgb),
0.52)) so the inset highlight adapts to both light and dark themes; update all
occurrences in the radiocards.scss file to use the chosen token/expression
instead of the literal rgba(255,255,255,0.52).
In `@styles/themes/components/table.scss`:
- Around line 40-42: The hover rule under the table row selector (&:hover) uses
--rad-ui-surface-subtle which resolves to the same color as
--rad-ui-surface-panel, making the hover invisible; update the rule to use
--rad-ui-surface-hover instead (replace the background-color in the &:hover
block to var(--rad-ui-surface-hover)) so the row hover becomes visually
distinct.
In `@styles/themes/components/textarea.scss`:
- Around line 20-22: The placeholder rule in textarea.scss currently forces
font-size: 1rem causing size jumps for small/large textarea variants; change the
::placeholder selector to inherit the textarea's font-size (remove the fixed
1rem and use font-size: inherit or no font-size so it inherits) so placeholder
text matches the component variants (affects the &::placeholder block and the
equivalent rules at lines referenced for variants).
- Around line 25-41: The shared state selectors (&:disabled, &:read-only,
&:focus) are being overridden by same-specificity per-variant rules (.soft,
.outline, .ghost), so either move the shared state blocks to appear after the
variant selectors in textarea.scss so they win by source order, or lower the
variants' specificity (e.g. wrap variant selectors in :where(...) or otherwise
avoid repeating the base selector) so the shared state rules keep the
focus/disabled styling; update the &:disabled / &:read-only / &:focus blocks
(and the duplicate block at 116-142) accordingly to ensure shared state wins
while keeping only intentional per-variant overrides.
In `@styles/themes/components/toggle-group.scss`:
- Around line 10-12: The vertical orientation rule lives inside the component
scope (using &), so it never matches the default wrapper that gets
data-orientation (RovingFocusGroup.Group); update the CSS so the
[data-orientation="vertical"] selector targets the wrapper element used at
runtime (e.g., the RovingFocusGroup.Group wrapper) instead of being nested,
ensuring the flex-direction: column rule applies for vertical groups by either
moving the rule out of the nested scope or adding an explicit selector that
matches RovingFocusGroup.Group[data-orientation="vertical"] (and keep the same
flex-direction change).
---
Outside diff comments:
In `@styles/themes/components/minimap.scss`:
- Around line 56-66: The .rad-ui-minimap-content block contains a duplicated
padding-bottom (0px at the earlier line and 20px later); remove the redundant
padding-bottom: 0px declaration so only the intended padding-bottom: 20px
remains in the .rad-ui-minimap-content rule to avoid dead code and ensure
consistent styling.
---
Duplicate comments:
In `@styles/themes/components/context-menu.scss`:
- Around line 14-16: The shared theme file currently forces demo dimensions on
the component by applying padding/width/min-height in context-menu.scss to the
default selector (affecting .rad-ui-context-menu-trigger); remove those demo
sizes from the shared rule and instead add a story/docs-only modifier or
CSS-variable override (e.g., .rad-ui-context-menu-trigger--demo or
--rad-context-menu-demo-width/height) and update the demo/story styles to apply
that modifier/variables so inline/custom triggers keep normal sizing while the
demo uses 24rem × 13.5rem.
---
Nitpick comments:
In `@knowledge/design_system/clarity_design_system.md`:
- Around line 25-38: The markdown table in clarity_design_system.md has
malformed rows (missing trailing "|" on some rows) and lacks blank lines before
and after the table which breaks markdownlint; fix it by adding the missing
trailing pipe characters to every table row (ensure each row, e.g., the rows for
steps 8, 9, 10, 11, etc., ends with "|"), verify the header separator row is
intact, and add a blank line both immediately before the table and immediately
after the trailing "---" separator so the table renders cleanly and passes
linting.
- Around line 63-68: The "Accessibility First" section (### 5. Accessibility
First) needs explicit, testable contrast criteria instead of just “must meet
WCAG AA”: add concrete ratio targets for the referenced tokens (Text (900–1000),
Borders (400–500)) and common token pairs — e.g., require text (normal) to be
>=4.5:1 on its intended background, large text >=3:1, UI components and primary
controls >=3:1, and borders (400–500) to be >=3:1 against adjacent surfaces;
also call out minimum contrast deltas for adjacent steps (suggest >=3:1) and
state that all ratios must be measurable with standard tools (e.g., WCAG
contrast checkers) so designers can validate token usage.
In `@src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveContent.tsx`:
- Line 134: The effect is re-running unnecessarily because shouldRender is
included in the dependency array; either remove shouldRender from the array or
add an early-return guard at the top of the useEffect so it bails when both open
is false and shouldRender is false. Specifically, in the effect that references
open, transitionDuration, forceMount, shouldRender (around the block that calls
setShouldRender(false) and uses scrollHeight/animation frames), remove
shouldRender from the dependency list or add: if (!open && !shouldRender)
return; to avoid extra DOM reads/RAF scheduling; keep other dependencies (open,
transitionDuration, forceMount) intact and ensure any timers/RAFs still get
cleaned up in the existing cleanup block.
In `@styles/cssTokens/base.tokens.css`:
- Around line 813-825: The dark theme gray tokens (--rad-ui-color-gray-50
through --rad-ui-color-gray-1000) use oklch() while the light tokens use hex,
causing inconsistent color formats; pick a single color format (either hex or
oklch) and convert the dark-theme token values to that format (or convert light
tokens to oklch) so all --rad-ui-color-gray-* variables use the same notation,
then update any documentation or a short inline comment explaining the chosen
convention; locate and edit the block containing [data-rad-ui-theme="dark"] and
the specific variables --rad-ui-color-gray-50 ... --rad-ui-color-gray-1000 to
apply the converted values.
- Around line 3-14: The gray color CSS custom properties inside the :root block
are misaligned; please align the declarations (--rad-ui-color-gray-100 through
--rad-ui-color-gray-1000) to match the indentation of the first token
(--rad-ui-color-gray-50) so all lines share the same indentation level and
spacing style, e.g., indent each line to the same column within the :root block
and keep one property per line.
In `@styles/themes/components/alert-dialog.scss`:
- Around line 100-124: The .rad-ui-alert-dialog-action and
.rad-ui-alert-dialog-cancel rules are duplicated in both the nested and
standalone contexts; define shared SCSS placeholders or mixins (e.g.,
%alert-dialog-action-base and %alert-dialog-cancel-base) that contain the common
properties (including &:hover blocks) and replace the duplicated blocks by
extending/including these placeholders wherever .rad-ui-alert-dialog-action and
.rad-ui-alert-dialog-cancel appear (both the nested footer variant and the
top-level selectors) so the styles are centralized and maintained in one place.
- Around line 106-109: The hover rule using color-mix(in oklab, ...) may be
ignored in older browsers; update the &:hover block in
styles/themes/components/alert-dialog.scss (the selector using &:hover for the
action button) to provide a compatible fallback before the color-mix declaration
— e.g., set a static background-color and border-color (or use an existing CSS
custom property like --rad-ui-color-gray-1000 or a derived var(--rad-ui-hover))
and then keep the color-mix rule after it so modern browsers use the advanced
mix while legacy browsers pick the fallback.
In `@styles/themes/components/minimap.scss`:
- Around line 30-38: The transition currently only lists color and box-shadow,
but the &[data-in-view="true"] rule also changes border-color and
background-color; update the transition declaration (the block containing
"transition: color 0.3s ease-in-out, box-shadow 0.3s ease-in-out;") to include
border-color and background-color so those properties animate as well when the
selector &[data-in-view="true"] toggles.
In `@styles/themes/components/radio-group.scss`:
- Around line 10-13: The rule in radio-group.scss that sets width: max-content
forces labels to their unwrapped width causing overflow and small hit areas;
locate the CSS block containing display: inline-flex; align-items: center; gap:
0.75rem; width: max-content and replace/remove width: max-content (use width:
auto or remove it) and optionally add max-width: 100% to allow wrapping and keep
responsiveness; then verify the change by testing a long radio label in a narrow
container or Storybook viewport to ensure it wraps and the hit area expands
correctly.
In `@styles/themes/components/slider.scss`:
- Around line 78-79: The two redundant rules [&data-state="dragging"] and
[&data-state="active"] in slider.scss duplicate the base selector's transform
(translateY(-50%)); either remove these duplicate selectors to clean up the CSS
or, if kept as placeholders for future state-specific styling, replace their
bodies with a brief comment noting they are intentional placeholders (e.g.,
"TODO: state-specific styles") so reviewers know they are deliberate—locate the
rules by the selectors [data-state="dragging"] and [data-state="active"] and
update accordingly.
- Around line 50-52: Replace the explicit multi-class selector
(.rad-ui-slider-thumb, .rad-ui-slider-thumb-lower, .rad-ui-slider-thumb-upper)
with a single, more maintainable selector that matches all thumb variants (e.g.,
an attribute or prefix-based selector such as [class^="rad-ui-slider-thumb"] or
a custom data-* attribute like [data-rad-ui-thumb]) so new variants inherit the
styles without editing this rule; update any markup to include the chosen
attribute if using data-* and ensure the rule still targets only slider thumbs
(adjust specificity if needed).
- Around line 161-175: The vertical thumb block duplicates the same selectors
and state rules as the horizontal one (.rad-ui-slider-thumb,
.rad-ui-slider-thumb-lower, .rad-ui-slider-thumb-upper with
&[data-state="dragging"], &[data-state="active"], and &:focus-visible); extract
the shared properties into a reusable SCSS construct (a mixin or placeholder
selector) and include it from both the horizontal and vertical orientation
blocks, leaving only orientation-specific rules (e.g., left/top/transform
differences) in each block and removing the redundant state selectors from the
vertical section.
In `@styles/themes/components/table.scss`:
- Around line 36-42: Add a background-color transition to the row hover rule to
smooth the change (e.g., transition: background-color 150ms ease) by updating
the block containing &:hover; also ensure header cells do not share the same
background variable as rows—either change the header cell background from
--rad-ui-surface-panel to a distinct token (e.g., --rad-ui-surface-header) or
pick a hover color variable different from the header so the &:hover
background-color: var(--rad-ui-surface-subtle) is visually distinct.
In `@styles/themes/components/tabs.scss`:
- Around line 38-65: The focus ring color is hardcoded as rgba(17,24,39,...) in
the &:focus-visible, &.active and &.active:focus-visible blocks which breaks
theming; replace those rgba(...) color literals with a CSS custom property (e.g.
var(--rad-ui-focus-ring-color, rgba(17,24,39,0.3)) and similar fallbacks for the
other alpha stops) so the ring adapts to theme tokens, and ensure a sensible
default is declared in your theme tokens file or a root selector so existing
visuals remain unchanged if the token is missing.
In `@styles/themes/components/toggle.scss`:
- Around line 17-24: The transition declaration currently only lists color and
box-shadow but the :hover block mutates background-color and border-color;
update the transition on the relevant selector (the rule surrounding transition
and &:hover) to include background-color and border-color (e.g., add
"background-color 150ms ..." and "border-color 150ms ...") so hover state
changes animate smoothly rather than abruptly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fb3dad09-d835-4160-8ce8-18252d8bf721
📒 Files selected for processing (108)
.changeset/green-ducks-rest.mddocs/app/docs/components/blockquote/examples/BlockQuoteColor.tsxdocs/app/docs/components/blockquote/examples/BlockQuoteExample.tsxdocs/app/docs/components/blockquote/examples/BlockQuoteSizes.tsxdocs/app/docs/components/button/docs/codeUsage.jsdocs/app/docs/components/card/content.mdxdocs/app/docs/components/em/docs/EmExample.tsxdocs/app/docs/components/heading/content.mdxdocs/app/docs/components/separator/content.mdxdocs/app/docs/components/strong/content.mdxdocs/app/docs/components/strong/docs/codeUsage.jsdocs/app/docs/components/text/content.mdxdocs/app/docs/components/tooltip/docs/examples/tooltip_example1.tsxdocs/app/docs/layout.tsxdocs/app/landingComponents/HeroSection.jsdocs/app/landingComponents/MusicAppPlayerDemo.jsdocs/app/landingComponents/ToolbarDemo.jsdocs/app/landingComponents/TrafficAnalyticsDemo.jsdocs/app/landingComponents/YourTeamDemo.jsdocs/app/layout.jsdocs/app/not-found.tsxdocs/app/og/route.tsxdocs/app/page.tsxdocs/app/playground/components/BlockquotePlayground.jsdocs/app/playground/components/EmPlayground.jsdocs/app/playground/components/HeadingPlayground.jsdocs/app/playground/components/QuotePlayground.jsdocs/app/playground/components/SeparatorPlayground.jsdocs/app/playground/components/TextPlayground.jsdocs/app/playground/helpers/ColorLooper.jsdocs/app/showcase/layout.jsdocs/app/showcase/music-app/helpers/MusicPlayer.tsxdocs/app/showcase/music-app/helpers/MusicSidebar.jsdocs/app/showcase/music-app/helpers/sections/PlaylistHero.tsxdocs/app/sponsors/components/BecomeSponsorCTA.tsxdocs/app/sponsors/components/SponsorCard.tsxdocs/app/sponsors/components/SponsorsFooter.tsxdocs/app/sponsors/components/SponsorsHeader.tsxdocs/app/testing/full_page_scroll/page.tsxdocs/components/Main/Main.jsdocs/components/Main/NavBar/helpers/NavRoot/index.tsxdocs/components/layout/Documentation/Documentation.jsdocs/components/layout/Documentation/helpers/ComponentFeatures/ComponentFeatures.jsdocs/components/layout/Documentation/helpers/ComponentHero/ComponentHero.jsdocs/components/layout/Documentation/helpers/DocsTable.jsdocs/components/navigation/Category.tsxdocs/components/navigation/NavItem.jsdocs/icons/logos/SoundWaveSampleLogo.jsdocs/mdx-components.tsxdocs/utils/seo/generateSeoMetadata.tsknowledge/design_system/clarity_design_system.mdsrc/components/tools/SandboxEditor/SandboxEditor.tsxsrc/components/ui/Accordion/fragments/AccordionRoot.tsxsrc/components/ui/Button/stories/Button.stories.tsxsrc/components/ui/Collapsible/tests/Collapsible.control.test.tsxsrc/components/ui/Splitter/demo.tsxsrc/components/ui/Tabs/stories/Tabs.stories.tsxsrc/components/ui/VisuallyHidden/stories/VisuallyHidden.stories.tsxsrc/core/primitives/Collapsible/fragments/CollapsiblePrimitiveContent.tsxsrc/core/primitives/Combobox/stories/Combobox.stories.tsxsrc/core/primitives/Menu/stories/MenuPrimitive.stories.tsxsrc/core/primitives/Menu/tests/MenuPrimitive.test.tsxsrc/design-systems/clarity/tokens/colors/gray.tssrc/examples/Colors/Colors.stories.jssrc/examples/FloatingUI/useInteractions/UseInteractionsExample.jsstyles/cssTokens/base.tokens.cssstyles/themes/components/_avatar-base.scssstyles/themes/components/alert-dialog.scssstyles/themes/components/badge.scssstyles/themes/components/blockquote.scssstyles/themes/components/button.scssstyles/themes/components/callout.scssstyles/themes/components/card.scssstyles/themes/components/checkbox-cards.scssstyles/themes/components/checkbox-group.scssstyles/themes/components/checkbox.scssstyles/themes/components/code.scssstyles/themes/components/combobox.scssstyles/themes/components/context-menu.scssstyles/themes/components/dialog.scssstyles/themes/components/dropdown-menu.scssstyles/themes/components/fix_tokens.cjsstyles/themes/components/heading.scssstyles/themes/components/kbd.scssstyles/themes/components/menubar.scssstyles/themes/components/minimap.scssstyles/themes/components/navigation-menu.scssstyles/themes/components/number-field.scssstyles/themes/components/progress.scssstyles/themes/components/radio-group.scssstyles/themes/components/radio.scssstyles/themes/components/radiocards.scssstyles/themes/components/select.scssstyles/themes/components/separator.scssstyles/themes/components/slider.scssstyles/themes/components/spinner.scssstyles/themes/components/splitter.scssstyles/themes/components/steps.scssstyles/themes/components/switch.scssstyles/themes/components/tab-nav.scssstyles/themes/components/table.scssstyles/themes/components/tabs.scssstyles/themes/components/textarea.scssstyles/themes/components/toggle-group.scssstyles/themes/components/toggle.scssstyles/themes/components/toolbar.scssstyles/themes/components/tree.scssstyles/themes/default.scss
✅ Files skipped from review due to trivial changes (66)
- .changeset/green-ducks-rest.md
- src/core/primitives/Combobox/stories/Combobox.stories.tsx
- docs/app/landingComponents/TrafficAnalyticsDemo.js
- src/examples/Colors/Colors.stories.js
- src/core/primitives/Menu/stories/MenuPrimitive.stories.tsx
- docs/app/docs/components/heading/content.mdx
- docs/app/docs/components/card/content.mdx
- docs/app/showcase/layout.js
- docs/app/og/route.tsx
- docs/app/sponsors/components/SponsorsFooter.tsx
- docs/components/layout/Documentation/helpers/ComponentFeatures/ComponentFeatures.js
- src/components/ui/VisuallyHidden/stories/VisuallyHidden.stories.tsx
- src/examples/FloatingUI/useInteractions/UseInteractionsExample.js
- docs/app/testing/full_page_scroll/page.tsx
- docs/app/playground/components/TextPlayground.js
- docs/app/docs/components/separator/content.mdx
- docs/app/playground/components/QuotePlayground.js
- docs/components/layout/Documentation/helpers/ComponentHero/ComponentHero.js
- docs/icons/logos/SoundWaveSampleLogo.js
- docs/app/sponsors/components/SponsorsHeader.tsx
- docs/app/docs/components/strong/content.mdx
- docs/app/docs/components/blockquote/examples/BlockQuoteColor.tsx
- docs/app/sponsors/components/BecomeSponsorCTA.tsx
- docs/components/Main/Main.js
- src/components/ui/Splitter/demo.tsx
- docs/components/navigation/Category.tsx
- docs/app/sponsors/components/SponsorCard.tsx
- docs/utils/seo/generateSeoMetadata.ts
- docs/app/playground/components/HeadingPlayground.js
- src/components/ui/Button/stories/Button.stories.tsx
- docs/app/docs/components/blockquote/examples/BlockQuoteSizes.tsx
- docs/app/docs/components/tooltip/docs/examples/tooltip_example1.tsx
- docs/app/not-found.tsx
- docs/app/docs/components/text/content.mdx
- docs/app/landingComponents/ToolbarDemo.js
- src/core/primitives/Menu/tests/MenuPrimitive.test.tsx
- docs/app/showcase/music-app/helpers/sections/PlaylistHero.tsx
- src/components/ui/Tabs/stories/Tabs.stories.tsx
- docs/app/docs/layout.tsx
- docs/app/playground/helpers/ColorLooper.js
- docs/app/docs/components/strong/docs/codeUsage.js
- docs/app/showcase/music-app/helpers/MusicSidebar.js
- docs/app/docs/components/em/docs/EmExample.tsx
- docs/app/playground/components/EmPlayground.js
- docs/components/navigation/NavItem.js
- docs/app/landingComponents/YourTeamDemo.js
- docs/components/layout/Documentation/helpers/DocsTable.js
- docs/app/showcase/music-app/helpers/MusicPlayer.tsx
- docs/app/docs/components/blockquote/examples/BlockQuoteExample.tsx
- styles/themes/components/spinner.scss
- docs/app/page.tsx
- docs/mdx-components.tsx
- docs/components/Main/NavBar/helpers/NavRoot/index.tsx
- docs/app/landingComponents/HeroSection.js
- styles/themes/components/progress.scss
- styles/themes/components/heading.scss
- styles/themes/components/tab-nav.scss
- docs/app/docs/components/button/docs/codeUsage.js
- docs/app/layout.js
- styles/themes/components/steps.scss
- styles/themes/components/tree.scss
- docs/app/playground/components/SeparatorPlayground.js
- docs/components/layout/Documentation/Documentation.js
- docs/app/playground/components/BlockquotePlayground.js
- styles/themes/components/select.scss
- docs/app/landingComponents/MusicAppPlayerDemo.js
🚧 Files skipped from review as they are similar to previous changes (22)
- styles/themes/components/kbd.scss
- styles/themes/components/toolbar.scss
- styles/themes/components/blockquote.scss
- styles/themes/components/code.scss
- styles/themes/components/checkbox-group.scss
- styles/themes/components/_avatar-base.scss
- src/components/tools/SandboxEditor/SandboxEditor.tsx
- styles/themes/components/radio.scss
- styles/themes/components/callout.scss
- styles/themes/components/dialog.scss
- styles/themes/components/separator.scss
- styles/themes/components/checkbox-cards.scss
- styles/themes/components/checkbox.scss
- styles/themes/components/combobox.scss
- styles/themes/components/splitter.scss
- styles/themes/components/switch.scss
- styles/themes/components/badge.scss
- styles/themes/components/navigation-menu.scss
- styles/themes/components/button.scss
- styles/themes/components/dropdown-menu.scss
- styles/themes/components/number-field.scss
- styles/themes/default.scss
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
styles/themes/components/alert-dialog.scss (1)
27-44:⚠️ Potential issue | 🟠 MajorCap the dialog height and allow internal scrolling.
This panel is fixed and vertically centered, but it never gets a
max-heightor overflow rule. Longer content will clip off-screen on short viewports, and the footer can become unreachable.🔧 Suggested fix
.rad-ui-alert-dialog-content { position: fixed; left: 50%; top: 50%; z-index: 60; width: 100%; max-width: 36rem; transform: translate(-50%, -50%); + max-height: calc(100vh - 2rem); + overflow-y: auto; padding: 1.2rem 1.2rem 0; border-radius: 1.25rem; background-color: var(--rad-ui-surface-panel); color: var(--rad-ui-text-primary);Also applies to: 125-144
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/alert-dialog.scss` around lines 27 - 44, The dialog panel (.rad-ui-alert-dialog-content) lacks a max-height and overflow, causing content/footer to be clipped on short viewports; update .rad-ui-alert-dialog-content to set a sensible max-height (e.g., relative to viewport like max-height: 80vh) and enable internal scrolling via overflow: auto (or overflow-y: auto) so long content can scroll while the panel remains centered; apply the same change to the other dialog content selector defined later (the second dialog content block around lines 125-144) to keep consistent behavior across dialogs.
♻️ Duplicate comments (5)
styles/themes/components/textarea.scss (2)
18-21:⚠️ Potential issue | 🟡 MinorLet placeholder typography follow the active size variant.
Line 20 hard-codes
font-size: 1rem, sosmall/largevariants still show placeholder/content size mismatch. Use inheritance instead.Suggested fix
&::placeholder { color: var(--rad-ui-text-muted); - font-size: 1rem; + font-size: inherit; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/textarea.scss` around lines 18 - 21, The placeholder rule in textarea.scss currently hard-codes font-size: 1rem causing mismatch with size variants; update the &::placeholder block (for the textarea component's placeholder styling) to stop forcing 1rem and instead inherit the font size from the host (e.g., use font-size: inherit or remove the font-size line) so the placeholder follows the active small/large size variant while keeping the color var(--rad-ui-text-muted).
114-140:⚠️ Potential issue | 🟠 MajorVariant base rules are still overriding shared focus/disabled/read-only states.
Because these selectors come later with matching specificity, variant declarations can cancel shared state styling (focus ring/border, muted disabled/read-only colors). Scope variant base rules to non-state cases (or move shared state rules after variants).
Suggested fix (state-safe selector scoping)
- &[data-textarea-variant="soft"] textarea { + &[data-textarea-variant="soft"] textarea:not(:disabled):not(:read-only):not(:focus) { border-color: transparent; background-color: var(--rad-ui-surface-subtle); color: var(--rad-ui-text-primary); box-shadow: inset 0 0 0 1px color-mix(in oklab, var(--rad-ui-border-soft) 55%, transparent); } - &[data-textarea-variant="outline"] textarea { + &[data-textarea-variant="outline"] textarea:not(:disabled):not(:read-only):not(:focus) { background-color: transparent; border: 1px solid var(--rad-ui-border-default); box-shadow: none; } - &[data-textarea-variant="solid"] textarea { + &[data-textarea-variant="solid"] textarea:not(:disabled):not(:read-only):not(:focus) { background-color: var(--rad-ui-surface-panel); border: 1px solid var(--rad-ui-border-soft); } - &[data-textarea-variant="ghost"] textarea { + &[data-textarea-variant="ghost"] textarea:not(:disabled):not(:read-only):not(:focus) { background-color: transparent; border: 1px solid transparent; box-shadow: none;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/textarea.scss` around lines 114 - 140, The variant selectors (e.g. &[data-textarea-variant="soft"] textarea, &[data-textarea-variant="outline"] textarea, &[data-textarea-variant="solid"] textarea, &[data-textarea-variant="ghost"] textarea) are declared after the shared focus/disabled/read-only rules and thus override those state styles; fix by scoping these variant base rules to only apply when no state pseudo-classes or state attributes are present (e.g. restrict to non-:focus, non-[data-disabled], non[readonly] contexts) or alternatively move the shared state rules so they appear after the variant blocks so focus/disabled/read-only styling wins; update the selectors for the variant blocks (and the ghost &:focus rule) accordingly so they no longer cancel the shared state styles.styles/themes/components/toggle-group.scss (1)
10-12:⚠️ Potential issue | 🟠 MajorMake the vertical selector match the default render path.
When
rovingFocusis enabled,data-orientationlives onRovingFocusGroup.Group, not on.rad-ui-toggle-group, so this selector still misses the common case and vertical groups stay horizontal.Proposed fix
- &[data-orientation="vertical"] { + &[data-orientation="vertical"], + [data-orientation="vertical"] & { flex-direction: column; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/toggle-group.scss` around lines 10 - 12, The vertical selector only targets .rad-ui-toggle-group[data-orientation="vertical"] via the current nested &[data-orientation="vertical"], but when rovingFocus is enabled the attribute is placed on RovingFocusGroup.Group instead; update the stylesheet so the same rule (flex-direction: column) also applies to RovingFocusGroup.Group[data-orientation="vertical"]—i.e., add an additional selector for RovingFocusGroup.Group[data-orientation="vertical"] alongside the existing &[data-orientation="vertical"] to ensure vertical groups render correctly in both cases.styles/themes/components/context-menu.scss (1)
14-16:⚠️ Potential issue | 🟠 MajorKeep the demo-sized canvas out of the shared trigger.
These fixed values make every
.rad-ui-context-menu-triggerrender like the story/demo surface. Normal inline or custom triggers should size from their content or an opt-in demo modifier instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/context-menu.scss` around lines 14 - 16, The shared .rad-ui-context-menu-trigger currently hardcodes padding, width and min-height (padding: 6rem 1.5rem; width: 24rem; min-height: 13.5rem;) which forces every trigger to render like the demo; remove those fixed properties from the .rad-ui-context-menu-trigger rule in styles/themes/components/context-menu.scss and instead place them behind an opt-in demo modifier class (e.g. .rad-ui-context-menu-trigger--demo or a story wrapper) so normal inline/custom triggers size from their content while the demo surface retains the fixed demo dimensions.styles/themes/components/alert-dialog.scss (1)
92-96:⚠️ Potential issue | 🟡 MinorMake the 640px breakpoint exclusive.
At exactly 640px, both media queries match, so the later mobile block overrides part of the desktop layout. Pick one side to be exclusive.
🔧 Suggested fix
- `@media` (width <= 640px) { + `@media` (width < 640px) {Also applies to: 125-144
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/alert-dialog.scss` around lines 92 - 96, The breakpoint at 640px is inclusive on both sides causing overlapping rules; make the desktop media query (`@media` (width >= 640px)) or the mobile counterpart exclusive so they don't both match at exactly 640px—for example change one side to use a strict upper bound (e.g., max-width just below 640px) or change the desktop to min-width:640px and the mobile to max-width:639.98px; update the `@media` block(s) around the alert-dialog styles (the `@media` (width >= 640px) block and its mobile counterpart) accordingly so only one applies at 640px.
🧹 Nitpick comments (7)
styles/themes/components/radiocards.scss (1)
89-94: Minor:pointer-events: nonemay block tooltip interactions.While the disabled styling is comprehensive,
pointer-events: noneprevents any mouse interaction, including hover tooltips that might explain why the item is disabled. If tooltips on disabled items are needed, consider removing this line or wrapping the item in a container that can receive pointer events.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/radiocards.scss` around lines 89 - 94, The disabled rule in radiocards.scss (selectors &[data-disabled] and &:disabled) uses pointer-events: none which blocks hover/tooltips; to fix, remove the pointer-events: none declaration from that rule (or alternatively move pointer-events: none to a specific interactive child selector inside the card so the outer element still receives hover events), keeping opacity: 0.5 and cursor: not-allowed so the visual and accessibility cues remain while allowing tooltip interactions.styles/themes/components/splitter.scss (2)
147-153: Remove empty reduced-motion media query.The
@media (prefers-reduced-motion: reduce)block contains empty rule sets that have no effect. Since the base styles no longer include transitions, this media query is dead code.🧹 Suggested removal
} - `@media` (prefers-reduced-motion: reduce) { - .rad-ui-splitter-panel { - } - - .rad-ui-splitter-handle { - } - } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/splitter.scss` around lines 147 - 153, Remove the dead `@media` (prefers-reduced-motion: reduce) block that contains empty rule sets for .rad-ui-splitter-panel and .rad-ui-splitter-handle; delete the entire media query since base styles no longer use transitions and these selectors have no rules inside, leaving no behavioral change.
93-96: Remove dead selector—handle is not a child of panel.The
.rad-ui-splitter-panel &selector targets the handle when it's nested inside a panel, but structurally the handle is a sibling to panels, not a child. This selector will never match. Additionally,background: transparentis already set on line 33, making this redundant even if it could match.🧹 Suggested removal
&.active::after, &:active::after { background: var(--rad-ui-border-strong); } - - // expand hit target without changing visuals - .rad-ui-splitter-panel & { - background: transparent; - } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/splitter.scss` around lines 93 - 96, Remove the dead selector `.rad-ui-splitter-panel & { background: transparent; }` from the splitter SCSS: this rule never matches because the handle is a sibling of `.rad-ui-splitter-panel` (not a child) and the same `background: transparent` is already declared earlier (line with the handle's base rule). Delete the entire selector block referencing `.rad-ui-splitter-panel &` to avoid redundancy and dead CSS.styles/themes/components/slider.scss (2)
74-75: Redundant transform rules provide no visual feedback.The
data-state="dragging"anddata-state="active"transforms are identical to the base transform (translateY(-50%)). If these states should provide visual feedback (e.g., slight scale), consider adding distinct transforms. Otherwise, these rules can be removed.💡 Option: Add visual feedback or remove redundant rules
- &[data-state="dragging"] { transform: translateY(-50%); } - &[data-state="active"] { transform: translateY(-50%); } + &[data-state="dragging"] { transform: translateY(-50%) scale(1.1); } + &[data-state="active"] { transform: translateY(-50%) scale(1.05); }Or simply remove if no feedback is intended:
- &[data-state="dragging"] { transform: translateY(-50%); } - &[data-state="active"] { transform: translateY(-50%); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/slider.scss` around lines 74 - 75, The two rules for &[data-state="dragging"] and &[data-state="active"] duplicate the base transform translateY(-50%) and thus provide no feedback; either remove these redundant selectors from slider.scss or change them to a distinct transform (for example add a subtle scale/translate difference) so the .slider thumb shows visual feedback during active/dragging states; update the selectors &[data-state="dragging"] and &[data-state="active"] accordingly.
162-163: Same redundant transform issue for vertical orientation.As with the horizontal thumbs, these state transforms are identical to the base and provide no visual feedback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/slider.scss` around lines 162 - 163, The state rules &[data-state="dragging"] and &[data-state="active"] in slider.scss are redundant for the vertical thumb because they repeat the base transform; remove these duplicate rules or, if you intended vertical positioning feedback, change them to use translateY(-50%) under the vertical orientation selector (e.g. [data-orientation="vertical"] &[data-state="dragging"] and [data-orientation="vertical"] &[data-state="active"]) so only vertical thumbs get the correct transform.styles/themes/components/badge.scss (1)
22-25: Consider consolidating the shared neutral fill rules forsecondaryandsoft.Lines 23-25 and Lines 51-53 are identical now. Pulling them into a shared selector would make the next token tweak less likely to drift between the two variants.
Also applies to: 50-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/badge.scss` around lines 22 - 25, The SCSS contains duplicated neutral-fill rules for the badge variants [data-badge-variant="secondary"] and [data-badge-variant="soft"]; consolidate them by creating a combined selector (e.g., &[data-badge-variant="secondary"], &[data-badge-variant="soft"]) and move the shared properties (background-color: var(--rad-ui-surface-subtle); border-color: transparent; color: var(--rad-ui-text-primary);) into that combined rule so both variants reuse the same declarations, leaving any unique overrides in their individual selectors.styles/themes/components/tabs.scss (1)
34-60: Use the theme ring tokens here instead of hard-coded grays.These focus states are the outlier in this PR: they won't track accent color or theme changes, so tabs end up with a different focus treatment than the rest of the system.
Proposed refactor
&:focus-visible{ z-index: 2; color: var(--rad-ui-text-primary); background-color: var(--rad-ui-surface-panel); outline: none; box-shadow: - 0 0 0 3px rgba(17, 24, 39, 0.3), - 0 0 0 4px rgba(17, 24, 39, 0.1), + var(--rad-ui-ring-shadow), 0 1px 2px rgba(0, 0, 0, 0.05), 0 2px 6px rgba(0, 0, 0, 0.04); } @@ &.active:focus-visible{ box-shadow: - 0 0 0 3px rgba(17, 24, 39, 0.3), - 0 0 0 4px rgba(17, 24, 39, 0.1), + var(--rad-ui-ring-shadow), 0 1px 2px rgba(0, 0, 0, 0.05), 0 2px 6px rgba(0, 0, 0, 0.04); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/tabs.scss` around lines 34 - 60, The focus box-shadow in styles/themes/components/tabs.scss uses hard-coded gray RGBA values in the &:focus-visible and &.active:focus-visible selectors; replace those literal colors with the project's theme ring tokens (the ring/accent token variables used elsewhere for focus rings) so the tab focus treatment tracks accent color and theme changes, keeping the existing box-shadow structure but substituting the rgba(17,24,39,...) and rgba(17,24,39,0.1) values with the corresponding token variables.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@styles/themes/components/checkbox-cards.scss`:
- Around line 29-34: The checked-state CSS currently hardcodes semantic tokens
and ignores per-instance accent color from the public color prop (composed into
data-rad-ui-accent-color in CheckboxCardsRoot.tsx); update the checked-state
rules in checkbox-cards.scss (and the other matching blocks at lines 95-113) to
use the accent override variable (e.g., var(--rad-ui-accent-color) with a
fallback to var(--rad-ui-text-primary)) for border-color, inset border and any
relevant background or shadow so per-instance color customization works, or
alternatively remove/deprecate the color prop in CheckboxCardsRoot.tsx if you
intend to drop instance-level customization.
- Around line 29-39: The current SCSS uses [data-state="checked"/"unchecked"]
but the checkbox primitive exposes state via aria-checked, so update the
selectors in styles/themes/components/checkbox-cards.scss (including the block
around .rad-ui-checkbox-cards-content and the repeated block at lines ~95-113)
to target aria-checked values instead (e.g., use &[aria-checked="true"] and
&[aria-checked="false"] or the equivalent on the trigger/card element referenced
by CheckboxGroupPrimitiveTrigger.tsx) so the checked/unchecked visual styles
actually apply when the component toggles.
In `@styles/themes/components/slider.scss`:
- Around line 106-116: Update the .rad-ui-slider-mark-label rule to use an
accessible font size: replace the hardcoded 10px with a token or a rem-based
value (suggest 0.75rem/12px) so labels are more readable across devices; modify
the font-size declaration in .rad-ui-slider-mark-label to reference an existing
design token (e.g., --rad-ui-font-size-sm) or set font-size: 0.75rem and keep
the rest of the rule unchanged.
- Around line 178-185: The `.rad-ui-slider-mark-label` positioning properties
are ineffective because the label is in normal document flow; change its
positioning to be absolute (and ensure the mark container has `position:
relative`) so `right`, `top`, and `transform` take effect, and remove the
redundant duplicate rule for `.rad-ui-slider-mark-line` under
`&.rad-ui-slider-mark-custom` since it repeats the same width/height as the base
`.rad-ui-slider-mark-line`; update the styles around
`.rad-ui-slider-mark-label`, `.rad-ui-slider-mark-line`, and
`&.rad-ui-slider-mark-custom .rad-ui-slider-mark-line` accordingly.
- Around line 50-65: The slider thumb is misaligned because .rad-ui-slider-thumb
in styles sets width: 1.25rem (20px) but SliderThumb.tsx positions thumbs using
calc(${percent}% - 8px) (half of 16px), causing a 2px offset; fix by either
changing the JS offset in SliderThumb.tsx from 8px to 10px or change
.rad-ui-slider-thumb width to 1rem (16px) so the existing 8px offset is correct.
Also remove or update the duplicate transform: translateY(-50%) rules under
[data-state="dragging"] and [data-state="active"] in slider.scss—either delete
them if no additional feedback is needed or add distinct active/dragging styles
(e.g., scale, box-shadow, or color change) to provide visible interactivity
feedback.
In `@styles/themes/components/splitter.scss`:
- Around line 15-29: .rad-ui-splitter-panel has overflow: hidden in SCSS but
SplitterPanel.tsx sets overflow: 'auto' inline, creating a fragile dependency;
either remove the overflow property from .rad-ui-splitter-panel so the component
(SplitterPanel.tsx) is the single source of truth for scrolling, or add a clear
comment in the SCSS above .rad-ui-splitter-panel stating that overflow is
intentionally omitted/overridden by SplitterPanel.tsx and must remain in the
component; update whichever file you choose (.scss or SplitterPanel.tsx) so the
intent is explicit and add a brief note referencing SplitterPanel.tsx if you
keep the SCSS entry.
In `@styles/themes/components/tabs.scss`:
- Around line 7-18: The .rad-ui-tabs-list CSS never changes flex-direction based
on TabList's aria-orientation, so vertical tabs still render horizontally;
update the styling for .rad-ui-tabs-list to switch flex-direction to column when
the container/tablist indicates vertical orientation (i.e., when TabList sets
aria-orientation="vertical") so the layout matches the semantic orientation and
stacked/column layout is applied for vertical tabs.
In `@styles/themes/components/toggle-group.scss`:
- Around line 16-17: The fixed width/height on the ToggleGroup item forces
icon-only squares and will clip labeled children; update the CSS rule targeting
the toggle item (e.g., the ToggleGroup.Item selector) to remove the hard
width/height and instead use min-width/min-height and appropriate padding (or
rely on line-height/inline-flex sizing) so labeled content can expand naturally;
keep any square styling for purely-icon variants by applying a specific
modifier/class for icon-only buttons rather than the base ToggleGroup.Item.
- Around line 29-39: The pressed state selector &[aria-pressed="true"] overrides
the :focus-visible box-shadow, removing the focus ring; update the CSS so the
focus-visible ring is preserved for pressed items by adding a more specific rule
(e.g. &:focus-visible[aria-pressed="true"] or splitting the selectors) that
merges the focus ring with the pressed state's shadows, or by composing the
box-shadow in the &[aria-pressed="true"] rule to include
var(--rad-ui-ring-shadow-inset) in addition to the pressed shadows; modify the
rules around :focus-visible and &[aria-pressed="true"] to ensure the focus ring
is retained during roving-focus navigation.
---
Outside diff comments:
In `@styles/themes/components/alert-dialog.scss`:
- Around line 27-44: The dialog panel (.rad-ui-alert-dialog-content) lacks a
max-height and overflow, causing content/footer to be clipped on short
viewports; update .rad-ui-alert-dialog-content to set a sensible max-height
(e.g., relative to viewport like max-height: 80vh) and enable internal scrolling
via overflow: auto (or overflow-y: auto) so long content can scroll while the
panel remains centered; apply the same change to the other dialog content
selector defined later (the second dialog content block around lines 125-144) to
keep consistent behavior across dialogs.
---
Duplicate comments:
In `@styles/themes/components/alert-dialog.scss`:
- Around line 92-96: The breakpoint at 640px is inclusive on both sides causing
overlapping rules; make the desktop media query (`@media` (width >= 640px)) or the
mobile counterpart exclusive so they don't both match at exactly 640px—for
example change one side to use a strict upper bound (e.g., max-width just below
640px) or change the desktop to min-width:640px and the mobile to
max-width:639.98px; update the `@media` block(s) around the alert-dialog styles
(the `@media` (width >= 640px) block and its mobile counterpart) accordingly so
only one applies at 640px.
In `@styles/themes/components/context-menu.scss`:
- Around line 14-16: The shared .rad-ui-context-menu-trigger currently hardcodes
padding, width and min-height (padding: 6rem 1.5rem; width: 24rem; min-height:
13.5rem;) which forces every trigger to render like the demo; remove those fixed
properties from the .rad-ui-context-menu-trigger rule in
styles/themes/components/context-menu.scss and instead place them behind an
opt-in demo modifier class (e.g. .rad-ui-context-menu-trigger--demo or a story
wrapper) so normal inline/custom triggers size from their content while the demo
surface retains the fixed demo dimensions.
In `@styles/themes/components/textarea.scss`:
- Around line 18-21: The placeholder rule in textarea.scss currently hard-codes
font-size: 1rem causing mismatch with size variants; update the &::placeholder
block (for the textarea component's placeholder styling) to stop forcing 1rem
and instead inherit the font size from the host (e.g., use font-size: inherit or
remove the font-size line) so the placeholder follows the active small/large
size variant while keeping the color var(--rad-ui-text-muted).
- Around line 114-140: The variant selectors (e.g.
&[data-textarea-variant="soft"] textarea, &[data-textarea-variant="outline"]
textarea, &[data-textarea-variant="solid"] textarea,
&[data-textarea-variant="ghost"] textarea) are declared after the shared
focus/disabled/read-only rules and thus override those state styles; fix by
scoping these variant base rules to only apply when no state pseudo-classes or
state attributes are present (e.g. restrict to non-:focus, non-[data-disabled],
non[readonly] contexts) or alternatively move the shared state rules so they
appear after the variant blocks so focus/disabled/read-only styling wins; update
the selectors for the variant blocks (and the ghost &:focus rule) accordingly so
they no longer cancel the shared state styles.
In `@styles/themes/components/toggle-group.scss`:
- Around line 10-12: The vertical selector only targets
.rad-ui-toggle-group[data-orientation="vertical"] via the current nested
&[data-orientation="vertical"], but when rovingFocus is enabled the attribute is
placed on RovingFocusGroup.Group instead; update the stylesheet so the same rule
(flex-direction: column) also applies to
RovingFocusGroup.Group[data-orientation="vertical"]—i.e., add an additional
selector for RovingFocusGroup.Group[data-orientation="vertical"] alongside the
existing &[data-orientation="vertical"] to ensure vertical groups render
correctly in both cases.
---
Nitpick comments:
In `@styles/themes/components/badge.scss`:
- Around line 22-25: The SCSS contains duplicated neutral-fill rules for the
badge variants [data-badge-variant="secondary"] and [data-badge-variant="soft"];
consolidate them by creating a combined selector (e.g.,
&[data-badge-variant="secondary"], &[data-badge-variant="soft"]) and move the
shared properties (background-color: var(--rad-ui-surface-subtle); border-color:
transparent; color: var(--rad-ui-text-primary);) into that combined rule so both
variants reuse the same declarations, leaving any unique overrides in their
individual selectors.
In `@styles/themes/components/radiocards.scss`:
- Around line 89-94: The disabled rule in radiocards.scss (selectors
&[data-disabled] and &:disabled) uses pointer-events: none which blocks
hover/tooltips; to fix, remove the pointer-events: none declaration from that
rule (or alternatively move pointer-events: none to a specific interactive child
selector inside the card so the outer element still receives hover events),
keeping opacity: 0.5 and cursor: not-allowed so the visual and accessibility
cues remain while allowing tooltip interactions.
In `@styles/themes/components/slider.scss`:
- Around line 74-75: The two rules for &[data-state="dragging"] and
&[data-state="active"] duplicate the base transform translateY(-50%) and thus
provide no feedback; either remove these redundant selectors from slider.scss or
change them to a distinct transform (for example add a subtle scale/translate
difference) so the .slider thumb shows visual feedback during active/dragging
states; update the selectors &[data-state="dragging"] and &[data-state="active"]
accordingly.
- Around line 162-163: The state rules &[data-state="dragging"] and
&[data-state="active"] in slider.scss are redundant for the vertical thumb
because they repeat the base transform; remove these duplicate rules or, if you
intended vertical positioning feedback, change them to use translateY(-50%)
under the vertical orientation selector (e.g. [data-orientation="vertical"]
&[data-state="dragging"] and [data-orientation="vertical"]
&[data-state="active"]) so only vertical thumbs get the correct transform.
In `@styles/themes/components/splitter.scss`:
- Around line 147-153: Remove the dead `@media` (prefers-reduced-motion: reduce)
block that contains empty rule sets for .rad-ui-splitter-panel and
.rad-ui-splitter-handle; delete the entire media query since base styles no
longer use transitions and these selectors have no rules inside, leaving no
behavioral change.
- Around line 93-96: Remove the dead selector `.rad-ui-splitter-panel & {
background: transparent; }` from the splitter SCSS: this rule never matches
because the handle is a sibling of `.rad-ui-splitter-panel` (not a child) and
the same `background: transparent` is already declared earlier (line with the
handle's base rule). Delete the entire selector block referencing
`.rad-ui-splitter-panel &` to avoid redundancy and dead CSS.
In `@styles/themes/components/tabs.scss`:
- Around line 34-60: The focus box-shadow in styles/themes/components/tabs.scss
uses hard-coded gray RGBA values in the &:focus-visible and
&.active:focus-visible selectors; replace those literal colors with the
project's theme ring tokens (the ring/accent token variables used elsewhere for
focus rings) so the tab focus treatment tracks accent color and theme changes,
keeping the existing box-shadow structure but substituting the
rgba(17,24,39,...) and rgba(17,24,39,0.1) values with the corresponding token
variables.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dfcd1cc3-2e7b-414a-994a-a4167bcaf939
📒 Files selected for processing (36)
docs/next.config.jsstyles/themes/components/accordion.scssstyles/themes/components/alert-dialog.scssstyles/themes/components/badge.scssstyles/themes/components/button.scssstyles/themes/components/checkbox-cards.scssstyles/themes/components/checkbox-group.scssstyles/themes/components/checkbox.scssstyles/themes/components/collapsible.scssstyles/themes/components/combobox.scssstyles/themes/components/context-menu.scssstyles/themes/components/dialog.scssstyles/themes/components/dropdown-menu.scssstyles/themes/components/kbd.scssstyles/themes/components/link.scssstyles/themes/components/menubar.scssstyles/themes/components/minimap.scssstyles/themes/components/navigation-menu.scssstyles/themes/components/number-field.scssstyles/themes/components/progress.scssstyles/themes/components/radio-group.scssstyles/themes/components/radio.scssstyles/themes/components/radiocards.scssstyles/themes/components/scroll-area.scssstyles/themes/components/select.scssstyles/themes/components/slider.scssstyles/themes/components/splitter.scssstyles/themes/components/steps.scssstyles/themes/components/switch.scssstyles/themes/components/tab-nav.scssstyles/themes/components/tabs.scssstyles/themes/components/textarea.scssstyles/themes/components/toggle-group.scssstyles/themes/components/toggle.scssstyles/themes/components/toolbar.scssstyles/themes/components/tree.scss
✅ Files skipped from review due to trivial changes (2)
- styles/themes/components/combobox.scss
- styles/themes/components/menubar.scss
🚧 Files skipped from review as they are similar to previous changes (19)
- styles/themes/components/scroll-area.scss
- styles/themes/components/kbd.scss
- styles/themes/components/link.scss
- styles/themes/components/tree.scss
- styles/themes/components/minimap.scss
- styles/themes/components/progress.scss
- styles/themes/components/tab-nav.scss
- styles/themes/components/steps.scss
- styles/themes/components/switch.scss
- styles/themes/components/radio-group.scss
- styles/themes/components/dialog.scss
- styles/themes/components/toggle.scss
- styles/themes/components/collapsible.scss
- styles/themes/components/toolbar.scss
- styles/themes/components/accordion.scss
- styles/themes/components/navigation-menu.scss
- styles/themes/components/number-field.scss
- styles/themes/components/button.scss
- styles/themes/components/radio.scss
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/ui/Tree/fragments/TreeItem.tsx (1)
141-141:⚠️ Potential issue | 🟡 MinorUsing
labelas React key may cause issues with duplicate labels.If two sibling items share the same label, React will produce duplicate key warnings and may incorrectly reuse/unmount components. Consider using a unique identifier from the item data.
- key={subItem.label} + key={subItem.id ?? subItem.label}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Tree/fragments/TreeItem.tsx` at line 141, The current JSX uses key={subItem.label} which can create duplicate React keys; update the key for the mapped child in TreeItem (where subItem is rendered) to use a unique identifier from the data (e.g., subItem.id) or a stable composite (e.g., `${subItem.id}-${subItem.label}` or include the index as a fallback) instead of subItem.label so sibling items always have unique keys.
♻️ Duplicate comments (2)
styles/themes/components/tabs.scss (1)
7-17:⚠️ Potential issue | 🟠 MajorVertical orientation is still only semantic here.
TabListalready exposesaria-orientation, but.rad-ui-tabs-listnever switches direction, so vertical tabs still render as a horizontal strip.Proposed fix
.rad-ui-tabs-list{ display: inline-flex; align-items: center; gap: 0.16rem; width: fit-content; padding: 0.14rem; border-radius: 0.95rem; background-color: var(--rad-ui-surface-muted); box-shadow: inset 0 0 0 1px rgba(0, 0, 0, 0.06), 0 2px 8px rgba(0, 0, 0, 0.025); + + &[aria-orientation="vertical"] { + flex-direction: column; + align-items: stretch; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/tabs.scss` around lines 7 - 17, The .rad-ui-tabs-list class currently always lays out tabs horizontally; update the stylesheet so it responds to TabList's aria-orientation by adding a rule targeting .rad-ui-tabs-list[aria-orientation="vertical"] to switch layout to a vertical column (set flex-direction: column and adjust alignment/gap/width as needed) so vertical tabs render stacked instead of in a horizontal strip; ensure the rule references the existing .rad-ui-tabs-list selector so it overrides the default display properties.styles/themes/components/toggle-group.scss (1)
10-12:⚠️ Potential issue | 🟠 MajorVertical groups can still render horizontally.
This selector only matches when
data-orientationis on.rad-ui-toggle-groupitself. If the primitive still places that attribute on the roving-focus wrapper, the vertical variant never flips to a column layout.💡 Proposed fix
- &[data-orientation="vertical"] { + &[data-orientation="vertical"], + [data-orientation="vertical"] & { flex-direction: column; }Run this to confirm where
data-orientationis applied at runtime. If it lands on a wrapper rather than the.rad-ui-toggle-groupelement, the current selector is still too narrow.#!/bin/bash rg -n -C3 'data-orientation|orientation' src --iglob '*ToggleGroup*' rg -n -C3 'RovingFocusGroup' src --iglob '*ToggleGroup*' rg -n -C3 'rad-ui-toggle-group' src --iglob '*ToggleGroup*'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/toggle-group.scss` around lines 10 - 12, The CSS only flips layout when data-orientation="vertical" is on .rad-ui-toggle-group itself, so when the primitive places data-orientation on the roving-focus wrapper (RovingFocusGroup) the vertical layout never applies; update the selector in toggle-group.scss to also match the case where the attribute lives on the wrapper/ancestor (e.g. target .rad-ui-toggle-group when a parent or wrapper has [data-orientation="vertical"] or when the attribute is placed on the roving-focus element), then verify at runtime that data-orientation is applied to which element (search for data-orientation, RovingFocusGroup, and rad-ui-toggle-group) and adjust the selector accordingly so vertical groups render in a column.
🧹 Nitpick comments (8)
styles/themes/components/blockquote.scss (1)
52-55: Consider de-duplicating shared accent color declarations.
softandaccentboth set the same border/text accent colors. You could group shared declarations to reduce drift risk.♻️ Optional DRY refactor
+ &[data-block-quote-variant="soft"], + &[data-block-quote-variant="accent"] { + border-inline-start-color: var(--rad-ui-color-accent-600); + color: var(--rad-ui-color-accent-1000); + } + &[data-block-quote-variant="soft"] { background-color: var(--rad-ui-color-accent-50); border-radius: 0.375rem; // rounded-md - border-inline-start-color: var(--rad-ui-color-accent-600); - color: var(--rad-ui-color-accent-1000);Also applies to: 76-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/blockquote.scss` around lines 52 - 55, The soft and accent blockquote variants duplicate the same border/text accent declarations; factor those shared declarations into a single reusable SCSS unit (e.g., a placeholder like %blockquote-accent or a mixin named blockquote-accent) that contains border-inline-start-color and color, then `@extend` or include that placeholder/mixin inside both the soft and accent variant blocks while keeping their variant-specific background-color lines in place; apply the same refactor for the other duplicated block (the one currently at lines 76-79).styles/themes/components/navigation-menu.scss (2)
84-84: Consider using a z-index token for dropdown layering.The hardcoded
z-index: 100could lead to layering conflicts with other components. If the design system has a z-index scale token (e.g.,--rad-ui-z-dropdown), using it here would improve consistency across components.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/navigation-menu.scss` at line 84, Replace the hardcoded z-index: 100 in the navigation menu styles with the design-system z-index token to avoid layering conflicts; locate the rule in styles/themes/components/navigation-menu.scss where z-index is set and change it to use the shared token (e.g., var(--rad-ui-z-dropdown) or the project’s equivalent SCSS variable like $z-index-dropdown) so dropdown stacking follows the global z-index scale.
8-8: Consider using design tokens consistently forborder-radius.The file mixes hardcoded
border-radiusvalues with token usage:
- Line 8:
border-radius: 1rem;(hardcoded)- Line 27:
border-radius: 0.8rem;(hardcoded)- Line 81:
border-radius: 1rem;(hardcoded)- Line 90:
border-radius: var(--rad-ui-radius-md);(token)Given the PR's objective of introducing semantic design tokens, consider using radius tokens (e.g.,
--rad-ui-radius-lg,--rad-ui-radius-md) consistently for maintainability.Also applies to: 27-27, 81-81, 90-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/navigation-menu.scss` at line 8, Replace hardcoded border-radius values with the design tokens used elsewhere: find the rules that currently set border-radius: 1rem; (two occurrences) and border-radius: 0.8rem; and change them to the appropriate semantic tokens (e.g., --rad-ui-radius-lg for 1rem and --rad-ui-radius-md for 0.8rem) so all radius values use the --rad-ui-radius-* tokens (note there is already a line using --rad-ui-radius-md to mirror). Update the three hardcoded occurrences to use the token names instead of literal rem values.src/components/ui/Checkbox/stories/Checkbox.stories.tsx (2)
20-20: Prefer semantic text tokens over hardcoded gray utilities in stories.These updated story lines still use raw grayscale utilities (
text-gray-*). If semantic tokens are available in this repo, use those classes/variables so stories stay aligned with the new design-token direction.Also applies to: 35-37, 49-50, 55-56, 87-87, 96-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Checkbox/stories/Checkbox.stories.tsx` at line 20, Replace hardcoded grayscale utility classes on story labels with the project's semantic text token classes/variables; locate the label elements in Checkbox.stories.tsx (e.g., the label with htmlFor="accept-terms" and the other label instances referenced in the comment) and swap className values like "text-gray-950" / "text-gray-700" / "text-gray-500" for the corresponding semantic token class names or CSS variable (e.g., text-primary, text-muted, text-subtle or --text-strong/--text-muted) used across the codebase so stories use design tokens consistently.
89-100: Replace nested conditional label/description rendering with a config map.The chained ternary + duplicated conditional blocks are harder to extend. A keyed config object will simplify this story and reduce branching noise.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Checkbox/stories/Checkbox.stories.tsx` around lines 89 - 100, Replace the chained ternary and duplicated conditional blocks that render the label and description for keys 'terms' and 'notifications' with a single keyed config map (e.g., an object mapping each key to { label, description }). In the component/story where the nested conditional currently produces the label and the two separate {key === 'terms'} / {key === 'notifications'} paragraphs, look up the entry for the current key (e.g., config[key]) and render its label and description if present (use optional chaining/fallbacks), removing the chained ternary and the two separate conditional <p> blocks; this centralizes text for 'terms' and 'notifications' and makes it easy to add more keys later.src/components/ui/Tree/fragments/TreeItem.tsx (1)
108-108: Use explicit boolean value foraria-hidden.While JSX treats
aria-hidden(without a value) as truthy, ARIA spec expects"true"or"false". Usearia-hidden="true"for explicit semantics.- <span className={`${rootClass}-item-chevron`} aria-hidden> + <span className={`${rootClass}-item-chevron`} aria-hidden="true">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Tree/fragments/TreeItem.tsx` at line 108, The span element in TreeItem.tsx using class `${rootClass}-item-chevron` sets aria-hidden without a value; update that attribute to use an explicit boolean string (aria-hidden="true") to comply with ARIA semantics so assistive tech reads it correctly.styles/themes/components/context-menu.scss (1)
170-182: Keep the danger color semantic here too.This selector goes back to a palette-specific
--rad-ui-color-red-900while the rest of the file resolves through semantic theme tokens. That makes the danger state harder to retheme and easier to drift in dark/high-contrast variants. Please route it through a semantic danger foreground token and reuse that token for the shortcut/sub-icon overrides.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/context-menu.scss` around lines 170 - 182, The selectors under .rad-ui-context-menu-danger are using the palette token --rad-ui-color-red-900; change all occurrences in that block (including the nested .rad-ui-context-menu-shortcut and .rad-ui-context-menu-sub-icon overrides) to use the semantic danger foreground token used elsewhere in the theme (e.g. the project’s --*-danger-foreground or equivalent semantic token) so the danger state is themed consistently and reused for the shortcut/sub-icon styles; update .rad-ui-context-menu-danger, &[data-highlighted], &[aria-selected="true"], and the nested shortcut/sub-icon rules to reference that single semantic token.styles/themes/components/select.scss (1)
152-168: Collapse the overlapping selected-item state rules.
&[aria-selected="true"][data-active="true"]is declared in both blocks, so the later selector immediately overrides the earlierbackground-color. Merging these into a single rule would make the final active-selected state explicit and easier to maintain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/select.scss` around lines 152 - 168, The two overlapping rules for &[aria-selected="true"][data-active="true"] and &[aria-selected="true"][data-highlighted] should be collapsed into a single explicit rule so the active-selected state isn't unintentionally overridden; combine the styles (background-color, color, box-shadow) and the nested .rad-ui-select-item-indicator { opacity: 1 } into one selector group that includes both &[aria-selected="true"][data-active="true"] and &[aria-selected="true"][data-highlighted] (or a combined selector like &[aria-selected="true"][data-active="true"], &[aria-selected="true"][data-highlighted]) and remove the earlier duplicate block so the final active-selected styling is explicit and maintainable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/ui/Badge/stories/Badge.stories.tsx`:
- Line 146: The story shows Badge with both color="blue" and
variant="destructive", which conflicts with the WithColors pattern that avoids
using color with destructive; update this Badge instance (component Badge in the
stories file) to follow the same pattern as WithColors by removing the color
prop when variant="destructive" (i.e., use <Badge
variant="destructive">Destructive</Badge>) so examples are consistent for
consumers.
In `@src/components/ui/Checkbox/stories/Checkbox.stories.tsx`:
- Line 17: The onCheckedChange handlers on Checkbox.Root (and the other
instances around lines 78–80) currently use unsafe casts like "val as boolean";
replace those with explicit boolean normalization (e.g., use val === true) and
update state via the functional setter to avoid closure issues—call
setChecked(prev => val === true) (or equivalent) inside the onCheckedChange for
Checkbox.Root to safely handle boolean | 'indeterminate' | null without type
assertions.
In `@src/components/ui/RadioCards/fragments/RadioCardsRoot.tsx`:
- Around line 45-51: RadioCardsRoot spreads {...props} after the explicit
orientation prop, allowing props.orientation to override the explicit value and
making behavior inconsistent with CheckboxCardsRoot; fix this in the
RadioGroupPrimitive.Root element inside RadioCardsRoot by moving the {...props}
spread to come before the explicit orientation prop (mirroring
CheckboxCardsRoot) so orientation={orientation} takes precedence; update the JSX
in RadioCardsRoot (RadioGroupPrimitive.Root) accordingly.
In `@src/components/ui/ToggleGroup/stories/ToggleGroup.stories.tsx`:
- Around line 17-25: ToggleGroup.Root is missing an accessible name, so add an
aria-label (for example "Text formatting") to the ToggleGroup.Root element to
give the group/toolbar context for screen readers; update the ToggleGroup.Root
declaration (the component named ToggleGroup.Root in this story) to include
aria-label="Text formatting".
In `@styles/themes/components/_avatar-base.scss`:
- Around line 64-70: The accent border is being applied to the avatar wrapper so
the default soft inset shadow from the fallback still shows; update the selector
&[data-rad-ui-accent-color]:not([data-rad-ui-has-image]) to either move the
accent box-shadow onto .rad-ui-avatar-fallback or explicitly unset the inherited
shadow on .rad-ui-avatar-fallback before applying the accent shadow. Concretely,
locate the block with &[data-rad-ui-accent-color]:not([data-rad-ui-has-image])
and adjust .rad-ui-avatar-fallback to remove any existing box-shadow (e.g.,
box-shadow: none) and then apply the accent box-shadow there so the fallback
fully replaces the default inset border.
In `@styles/themes/components/context-menu.scss`:
- Around line 35-36: The rule setting width: max-content (paired with min-width:
14.5rem) allows unbounded growth; change it to include a viewport-aware
max-width (e.g. max-width: min(90vw, <some-desired-absolute>)) on the same
selector and add truncation rules on the menu row text cell (overflow: hidden;
text-overflow: ellipsis; white-space: nowrap) so long labels/shortcuts don’t
expand the panel; apply the same max-width/truncation changes to the other
occurrences noted (the blocks at the lines referenced: 69 and 112) and ensure
the min-width stays for small menus.
- Around line 61-66: The rule using justify-content: space-between is causing
labels to separate from their leading icons when items gain extra children;
change the layout to keep items left-packed (use justify-content: flex-start or
remove space-between) and instead push only trailing affordances by adding
margin-inline-start: auto to .rad-ui-context-menu-shortcut and
.rad-ui-context-menu-sub-icon (apply the same change to the other blocks
mentioned around the other occurrences). Locate the blocks where
justify-content: space-between is set (e.g., the rule containing gap: 0.75rem)
and replace the justification, then add margin-inline-start: auto to the two
selector rules so trailing icons/shortcuts are right-aligned without
distributing free space across the whole row.
In `@styles/themes/components/dialog.scss`:
- Around line 79-94: The CSS uses .rad-ui-dialog-footer with flex-direction:
column-reverse which flips visual order but not DOM/tab order; change it to
maintain DOM order by replacing column-reverse with column (i.e., use
flex-direction: column for the mobile/small-screen rule) and keep the existing
media query that sets flex-direction: row for larger screens so visual and
keyboard/tab order remain aligned; ensure no other rules rely on the reverse
ordering and adjust spacing/gap if needed.
In `@styles/themes/components/tabs.scss`:
- Around line 8-11: The tab strip is shrink-wrapped (width: fit-content in the
tab strip block) so toggling the active label weight (500→600 at the active
label rule) changes trigger and strip width causing layout jump; fix by
preventing width reflow—either remove/replace width: fit-content on the tab
strip with a non-shrinking layout (e.g., width: auto or full-width), or ensure
each .tab-trigger has a fixed/minimum width (or reserve space by using the
heavier font-weight on all triggers and use opacity/color for active emphasis)
so the font-weight change on the active label (the active label rule) no longer
alters element width.
In `@styles/themes/components/toggle-group.scss`:
- Around line 32-35: Hover styles for the toggle group are being applied even
when items are disabled because the &:hover block isn't restricted; update the
hover selectors in the toggle-group.scss hover rule(s) (the existing &:hover
blocks) to only apply when the element is not disabled by using a negation such
as &:not([data-disabled]):not(:disabled):hover (and update the second hover
block similarly), so disabled items (marked with [data-disabled] or :disabled)
do not receive the hover background or text color.
In `@styles/themes/components/toggle.scss`:
- Around line 18-21: The hover styles currently apply even when the toggle is
disabled; update the hover selector so it only matches enabled toggles by gating
&:hover with enabled state checks (e.g., use :not([data-disabled]) and
:not(:disabled) combined before :hover) for the rule that contains
background-color and border-color, and make the same change for the other hover
block referenced (the block around lines with similar hover styles). Ensure you
modify the selectors that contain &:hover so disabled controls no longer receive
hover visuals.
---
Outside diff comments:
In `@src/components/ui/Tree/fragments/TreeItem.tsx`:
- Line 141: The current JSX uses key={subItem.label} which can create duplicate
React keys; update the key for the mapped child in TreeItem (where subItem is
rendered) to use a unique identifier from the data (e.g., subItem.id) or a
stable composite (e.g., `${subItem.id}-${subItem.label}` or include the index as
a fallback) instead of subItem.label so sibling items always have unique keys.
---
Duplicate comments:
In `@styles/themes/components/tabs.scss`:
- Around line 7-17: The .rad-ui-tabs-list class currently always lays out tabs
horizontally; update the stylesheet so it responds to TabList's aria-orientation
by adding a rule targeting .rad-ui-tabs-list[aria-orientation="vertical"] to
switch layout to a vertical column (set flex-direction: column and adjust
alignment/gap/width as needed) so vertical tabs render stacked instead of in a
horizontal strip; ensure the rule references the existing .rad-ui-tabs-list
selector so it overrides the default display properties.
In `@styles/themes/components/toggle-group.scss`:
- Around line 10-12: The CSS only flips layout when data-orientation="vertical"
is on .rad-ui-toggle-group itself, so when the primitive places data-orientation
on the roving-focus wrapper (RovingFocusGroup) the vertical layout never
applies; update the selector in toggle-group.scss to also match the case where
the attribute lives on the wrapper/ancestor (e.g. target .rad-ui-toggle-group
when a parent or wrapper has [data-orientation="vertical"] or when the attribute
is placed on the roving-focus element), then verify at runtime that
data-orientation is applied to which element (search for data-orientation,
RovingFocusGroup, and rad-ui-toggle-group) and adjust the selector accordingly
so vertical groups render in a column.
---
Nitpick comments:
In `@src/components/ui/Checkbox/stories/Checkbox.stories.tsx`:
- Line 20: Replace hardcoded grayscale utility classes on story labels with the
project's semantic text token classes/variables; locate the label elements in
Checkbox.stories.tsx (e.g., the label with htmlFor="accept-terms" and the other
label instances referenced in the comment) and swap className values like
"text-gray-950" / "text-gray-700" / "text-gray-500" for the corresponding
semantic token class names or CSS variable (e.g., text-primary, text-muted,
text-subtle or --text-strong/--text-muted) used across the codebase so stories
use design tokens consistently.
- Around line 89-100: Replace the chained ternary and duplicated conditional
blocks that render the label and description for keys 'terms' and
'notifications' with a single keyed config map (e.g., an object mapping each key
to { label, description }). In the component/story where the nested conditional
currently produces the label and the two separate {key === 'terms'} / {key ===
'notifications'} paragraphs, look up the entry for the current key (e.g.,
config[key]) and render its label and description if present (use optional
chaining/fallbacks), removing the chained ternary and the two separate
conditional <p> blocks; this centralizes text for 'terms' and 'notifications'
and makes it easy to add more keys later.
In `@src/components/ui/Tree/fragments/TreeItem.tsx`:
- Line 108: The span element in TreeItem.tsx using class
`${rootClass}-item-chevron` sets aria-hidden without a value; update that
attribute to use an explicit boolean string (aria-hidden="true") to comply with
ARIA semantics so assistive tech reads it correctly.
In `@styles/themes/components/blockquote.scss`:
- Around line 52-55: The soft and accent blockquote variants duplicate the same
border/text accent declarations; factor those shared declarations into a single
reusable SCSS unit (e.g., a placeholder like %blockquote-accent or a mixin named
blockquote-accent) that contains border-inline-start-color and color, then
`@extend` or include that placeholder/mixin inside both the soft and accent
variant blocks while keeping their variant-specific background-color lines in
place; apply the same refactor for the other duplicated block (the one currently
at lines 76-79).
In `@styles/themes/components/context-menu.scss`:
- Around line 170-182: The selectors under .rad-ui-context-menu-danger are using
the palette token --rad-ui-color-red-900; change all occurrences in that block
(including the nested .rad-ui-context-menu-shortcut and
.rad-ui-context-menu-sub-icon overrides) to use the semantic danger foreground
token used elsewhere in the theme (e.g. the project’s --*-danger-foreground or
equivalent semantic token) so the danger state is themed consistently and reused
for the shortcut/sub-icon styles; update .rad-ui-context-menu-danger,
&[data-highlighted], &[aria-selected="true"], and the nested shortcut/sub-icon
rules to reference that single semantic token.
In `@styles/themes/components/navigation-menu.scss`:
- Line 84: Replace the hardcoded z-index: 100 in the navigation menu styles with
the design-system z-index token to avoid layering conflicts; locate the rule in
styles/themes/components/navigation-menu.scss where z-index is set and change it
to use the shared token (e.g., var(--rad-ui-z-dropdown) or the project’s
equivalent SCSS variable like $z-index-dropdown) so dropdown stacking follows
the global z-index scale.
- Line 8: Replace hardcoded border-radius values with the design tokens used
elsewhere: find the rules that currently set border-radius: 1rem; (two
occurrences) and border-radius: 0.8rem; and change them to the appropriate
semantic tokens (e.g., --rad-ui-radius-lg for 1rem and --rad-ui-radius-md for
0.8rem) so all radius values use the --rad-ui-radius-* tokens (note there is
already a line using --rad-ui-radius-md to mirror). Update the three hardcoded
occurrences to use the token names instead of literal rem values.
In `@styles/themes/components/select.scss`:
- Around line 152-168: The two overlapping rules for
&[aria-selected="true"][data-active="true"] and
&[aria-selected="true"][data-highlighted] should be collapsed into a single
explicit rule so the active-selected state isn't unintentionally overridden;
combine the styles (background-color, color, box-shadow) and the nested
.rad-ui-select-item-indicator { opacity: 1 } into one selector group that
includes both &[aria-selected="true"][data-active="true"] and
&[aria-selected="true"][data-highlighted] (or a combined selector like
&[aria-selected="true"][data-active="true"],
&[aria-selected="true"][data-highlighted]) and remove the earlier duplicate
block so the final active-selected styling is explicit and maintainable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ff5ec4e-1ed8-4066-9b44-30ff1d0ab545
📒 Files selected for processing (52)
package.jsonsrc/components/tools/SandboxEditor/SandboxEditor.tsxsrc/components/ui/Badge/stories/Badge.stories.tsxsrc/components/ui/Card/fragments/CardRoot.tsxsrc/components/ui/Card/stories/Card.stories.tsxsrc/components/ui/Checkbox/stories/Checkbox.stories.tsxsrc/components/ui/CheckboxCards/fragments/CheckboxCardsRoot.tsxsrc/components/ui/CheckboxCards/stories/CheckboxCards.stories.tsxsrc/components/ui/Collapsible/stories/Collapsible.stories.tsxsrc/components/ui/ContextMenu/stories/ContextMenu.stories.tsxsrc/components/ui/HoverCard/stories/HoverCard.stories.tsxsrc/components/ui/Progress/stories/Progress.stories.tsxsrc/components/ui/RadioCards/RadioCards.stories.tsxsrc/components/ui/RadioCards/fragments/RadioCardsRoot.tsxsrc/components/ui/Slider/fragments/SliderRangeSlider.tsxsrc/components/ui/Slider/stories/Slider.stories.tsxsrc/components/ui/Switch/stories/Switch.stories.tsxsrc/components/ui/TextArea/stories/TextArea.stories.tsxsrc/components/ui/Theme/Theme.tsxsrc/components/ui/ToggleGroup/stories/ToggleGroup.stories.tsxsrc/components/ui/Tooltip/fragments/TooltipContent.tsxsrc/components/ui/Tree/fragments/TreeItem.tsxsrc/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveTrigger.tsxsrc/core/primitives/Dialog/fragments/DialogPrimitivePortal.tsxsrc/core/primitives/Menu/fragments/MenuPrimitiveRoot.tsxsrc/core/primitives/Menu/tests/MenuPrimitive.test.tsxsrc/core/utils/RovingFocusGroup/fragments/RovingFocusGroup.tsxstyles/themes/components/_avatar-base.scssstyles/themes/components/alert-dialog.scssstyles/themes/components/avatar-group.scssstyles/themes/components/blockquote.scssstyles/themes/components/card.scssstyles/themes/components/code.scssstyles/themes/components/context-menu.scssstyles/themes/components/dialog.scssstyles/themes/components/menubar.scssstyles/themes/components/minimap.scssstyles/themes/components/navigation-menu.scssstyles/themes/components/number-field.scssstyles/themes/components/radiocards.scssstyles/themes/components/select.scssstyles/themes/components/slider.scssstyles/themes/components/splitter.scssstyles/themes/components/switch.scssstyles/themes/components/table.scssstyles/themes/components/tabs.scssstyles/themes/components/textarea.scssstyles/themes/components/toggle-group.scssstyles/themes/components/toggle.scssstyles/themes/components/toolbar.scssstyles/themes/components/tree.scssstyles/themes/default.scss
💤 Files with no reviewable changes (1)
- styles/themes/components/table.scss
✅ Files skipped from review due to trivial changes (8)
- package.json
- src/components/ui/Card/fragments/CardRoot.tsx
- src/components/ui/Slider/fragments/SliderRangeSlider.tsx
- styles/themes/components/toolbar.scss
- styles/themes/components/alert-dialog.scss
- styles/themes/components/textarea.scss
- src/components/ui/Card/stories/Card.stories.tsx
- src/components/ui/ContextMenu/stories/ContextMenu.stories.tsx
🚧 Files skipped from review as they are similar to previous changes (19)
- src/core/primitives/Menu/tests/MenuPrimitive.test.tsx
- src/components/ui/RadioCards/RadioCards.stories.tsx
- src/components/ui/Tooltip/fragments/TooltipContent.tsx
- src/components/ui/Theme/Theme.tsx
- src/components/ui/HoverCard/stories/HoverCard.stories.tsx
- styles/themes/components/code.scss
- src/core/primitives/Dialog/fragments/DialogPrimitivePortal.tsx
- styles/themes/components/switch.scss
- styles/themes/components/number-field.scss
- styles/themes/components/avatar-group.scss
- src/components/ui/Collapsible/stories/Collapsible.stories.tsx
- styles/themes/components/minimap.scss
- src/components/ui/TextArea/stories/TextArea.stories.tsx
- src/components/ui/CheckboxCards/stories/CheckboxCards.stories.tsx
- src/components/ui/Switch/stories/Switch.stories.tsx
- styles/themes/default.scss
- styles/themes/components/slider.scss
- styles/themes/components/menubar.scss
- styles/themes/components/splitter.scss
Contributor
CoverageThis report compares the PR with the base branch. "Δ" shows how the PR affects each metric.
Coverage decreased for at least one metric. Please add or update tests to improve coverage. Run |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
styles/themes/components/toggle-group.scss (2)
42-45:⚠️ Potential issue | 🟡 MinorDisabled items still receive hover styles.
The
:hoverrule applies unconditionally, so disabled items will show the hover background and text color despite being visually marked as disabled.Proposed fix
- &:hover { + &:not([data-disabled]):not(:disabled):hover { background-color: color-mix(in oklab, var(--rad-ui-surface-panel) 88%, transparent); color: var(--rad-ui-text-primary); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/toggle-group.scss` around lines 42 - 45, The hover rule currently applies to all items via the &:hover selector, causing disabled items to receive hover styles; update the selector to exclude disabled elements (e.g., use &:not([aria-disabled="true"]):hover and/or &:not(:disabled):hover) so the background-color and color rules only apply to interactive items and not to elements marked disabled.
10-12:⚠️ Potential issue | 🟠 MajorVertical orientation selector won't match at runtime.
Per
ToggleGroupRoot.tsx,data-orientationis spread ontoRovingFocusGroup.Group, which wraps thediv.rad-ui-toggle-group. The current&[data-orientation="vertical"]selector expects the attribute on the.rad-ui-toggle-groupelement itself.Proposed fix
Either move the
data-orientationattribute to the inner div inToggleGroupRoot.tsx, or update the selector:- &[data-orientation="vertical"] { + [data-orientation="vertical"] & { flex-direction: column; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/toggle-group.scss` around lines 10 - 12, The SCSS rule in styles/themes/components/toggle-group.scss currently expects data-orientation on .rad-ui-toggle-group but ToggleGroupRoot.tsx spreads data-orientation onto the outer RovingFocusGroup.Group wrapper, so the vertical selector never matches; either move the data-orientation attribute from the wrapper to the inner div.rad-ui-toggle-group in ToggleGroupRoot.tsx (so the existing &[data-orientation="vertical"] selector works), or update the SCSS selector to target the inner .rad-ui-toggle-group when the attribute is present on its parent wrapper (i.e., match the wrapper's data-orientation to apply the same flex-direction: column to .rad-ui-toggle-group).src/components/ui/ToggleGroup/stories/ToggleGroup.stories.tsx (1)
17-27:⚠️ Potential issue | 🟡 MinorAdd an accessible name to
ToggleGroup.Root.While the individual items have proper
aria-labelattributes, the group itself lacks an accessible name. Screen reader users won't have context about what this toolbar controls.Proposed fix
- <ToggleGroup.Root type="multiple" defaultValue={['bold']}> + <ToggleGroup.Root type="multiple" defaultValue={['bold']} aria-label="Text formatting">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/ToggleGroup/stories/ToggleGroup.stories.tsx` around lines 17 - 27, The ToggleGroup.Root is missing an accessible name; add an accessible name (e.g., aria-label="Text formatting toolbar" or aria-labelledby referencing a visible label) and ensure the root has an appropriate role such as role="toolbar" if needed; update the ToggleGroup.Root element (the ToggleGroup.Root component that wraps ToggleGroup.Item elements) to include the chosen aria-label or aria-labelledby so screen readers have context for the bold/italic/underline controls.styles/themes/components/tabs.scss (1)
8-12:⚠️ Potential issue | 🟡 MinorActive-state weight still causes tab-strip reflow.
Line 12 keeps the list shrink-wrapped, so the
500 → 600jump on Line 55 can still resize both the active trigger and the overall control when selection moves.Minimal fix
&.active{ z-index: 1; color: var(--rad-ui-text-primary); - font-weight: 600; background-color: var(--rad-ui-surface-canvas); box-shadow:If you want to keep the heavier active label, reserve width per trigger instead.
Also applies to: 52-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/tabs.scss` around lines 8 - 12, The tab container uses width: fit-content which shrink-wraps and lets the active-state font-weight change (500→600) reflow the control; remove or replace width: fit-content on the container in styles/themes/components/tabs.scss with width: auto (or an explicit inline-size) and instead reserve space per trigger by adding a min-width to the trigger selector (e.g., .tab-trigger) or introduce a CSS variable like --tab-reserve-width applied as min-width on .tab-trigger so the heavier active label won’t resize the trigger or the overall tab-strip when selection changes.
🧹 Nitpick comments (2)
knowledge/design_system/clarity_design_system.md (1)
67-67: Hyphenate compound adjective for clarity.Line 67 should use “low-contrast adjacent steps”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge/design_system/clarity_design_system.md` at line 67, The phrase "Avoid low contrast adjacent steps" should hyphenate the compound adjective; update the copy so the sentence reads "Avoid low-contrast adjacent steps" (replace the existing "Avoid low contrast adjacent steps" text to include the hyphen between "low" and "contrast").styles/themes/components/_avatar-base.scss (1)
54-59: Remove duplicated fallback declarations in image-state block.Line 56-Line 58 repeat the base fallback styles from Line 30-Line 32, so this block can be dropped without behavior change and with less maintenance overhead.
♻️ Proposed no-op cleanup
- &[data-rad-ui-has-image] { - .rad-ui-avatar-fallback { - color: var(--rad-ui-text-primary); - background-color: var(--rad-ui-surface-muted); - box-shadow: inset 0 0 0 1px var(--rad-ui-border-soft); - } - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/themes/components/_avatar-base.scss` around lines 54 - 59, The &[data-rad-ui-has-image] block duplicates the .rad-ui-avatar-fallback styles already declared earlier, so remove the entire &[data-rad-ui-has-image] { .rad-ui-avatar-fallback { ... } } block to eliminate duplicate declarations; locate the selector &[data-rad-ui-has-image] and delete the nested .rad-ui-avatar-fallback rules (color, background-color, box-shadow) so the component relies on the single base .rad-ui-avatar-fallback definition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@knowledge/design_system/clarity_design_system.md`:
- Around line 34-35: The two markdown table rows starting with "| 8 | 700 |
Solid colors | Disabled text/icons, muted UI elements" and "| 9
| 800 | Solid colors | supporting content" are missing the
trailing pipe; update these rows in clarity_design_system.md by appending a
closing "|" to each line so the table cells are properly terminated (i.e.,
ensure both rows end with " |").
- Around line 37-38: The Markdown table ending with the row "| 11 | 1000 |
Accessible text | Headings,Primary text, max contrast text (near
black) |" needs a blank line added immediately after it to satisfy Markdown
table boundary rules; edit the document to insert one empty line following that
table block so the subsequent content (line 38) is separated from the table.
In `@styles/themes/components/card.scss`:
- Around line 75-114: The footer divider and content top spacing should only
apply when those sections follow another section; update the selectors so
.rad-ui-card-footer’s border/margin/padding apply via an adjacent/sibling rule
(e.g. use .rad-ui-card-content + .rad-ui-card-footer or .rad-ui-card-header +
.rad-ui-card-footer or a combined :is(...) + .rad-ui-card-footer) instead of
unconditionally on .rad-ui-card-footer, and likewise only add
.rad-ui-card-content’s extra padding when it is not the first child (e.g. use
:is(.rad-ui-card-header, .rad-ui-card-actions) + .rad-ui-card-content or
.rad-ui-card-content:not(:first-child)); mirror these conditional selector
changes in the small/large/x-large overrides ([data-card-size="small"] >
.rad-ui-card-footer, [data-card-size="large"] > .rad-ui-card-footer,
[data-card-size="x-large"] > .rad-ui-card-footer and the corresponding content
rules) so separators and inset adjustments only appear when a preceding section
exists.
---
Duplicate comments:
In `@src/components/ui/ToggleGroup/stories/ToggleGroup.stories.tsx`:
- Around line 17-27: The ToggleGroup.Root is missing an accessible name; add an
accessible name (e.g., aria-label="Text formatting toolbar" or aria-labelledby
referencing a visible label) and ensure the root has an appropriate role such as
role="toolbar" if needed; update the ToggleGroup.Root element (the
ToggleGroup.Root component that wraps ToggleGroup.Item elements) to include the
chosen aria-label or aria-labelledby so screen readers have context for the
bold/italic/underline controls.
In `@styles/themes/components/tabs.scss`:
- Around line 8-12: The tab container uses width: fit-content which shrink-wraps
and lets the active-state font-weight change (500→600) reflow the control;
remove or replace width: fit-content on the container in
styles/themes/components/tabs.scss with width: auto (or an explicit inline-size)
and instead reserve space per trigger by adding a min-width to the trigger
selector (e.g., .tab-trigger) or introduce a CSS variable like
--tab-reserve-width applied as min-width on .tab-trigger so the heavier active
label won’t resize the trigger or the overall tab-strip when selection changes.
In `@styles/themes/components/toggle-group.scss`:
- Around line 42-45: The hover rule currently applies to all items via the
&:hover selector, causing disabled items to receive hover styles; update the
selector to exclude disabled elements (e.g., use
&:not([aria-disabled="true"]):hover and/or &:not(:disabled):hover) so the
background-color and color rules only apply to interactive items and not to
elements marked disabled.
- Around line 10-12: The SCSS rule in styles/themes/components/toggle-group.scss
currently expects data-orientation on .rad-ui-toggle-group but
ToggleGroupRoot.tsx spreads data-orientation onto the outer
RovingFocusGroup.Group wrapper, so the vertical selector never matches; either
move the data-orientation attribute from the wrapper to the inner
div.rad-ui-toggle-group in ToggleGroupRoot.tsx (so the existing
&[data-orientation="vertical"] selector works), or update the SCSS selector to
target the inner .rad-ui-toggle-group when the attribute is present on its
parent wrapper (i.e., match the wrapper's data-orientation to apply the same
flex-direction: column to .rad-ui-toggle-group).
---
Nitpick comments:
In `@knowledge/design_system/clarity_design_system.md`:
- Line 67: The phrase "Avoid low contrast adjacent steps" should hyphenate the
compound adjective; update the copy so the sentence reads "Avoid low-contrast
adjacent steps" (replace the existing "Avoid low contrast adjacent steps" text
to include the hyphen between "low" and "contrast").
In `@styles/themes/components/_avatar-base.scss`:
- Around line 54-59: The &[data-rad-ui-has-image] block duplicates the
.rad-ui-avatar-fallback styles already declared earlier, so remove the entire
&[data-rad-ui-has-image] { .rad-ui-avatar-fallback { ... } } block to eliminate
duplicate declarations; locate the selector &[data-rad-ui-has-image] and delete
the nested .rad-ui-avatar-fallback rules (color, background-color, box-shadow)
so the component relies on the single base .rad-ui-avatar-fallback definition.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 70b2d809-6226-4a8c-ae53-0036e237aaf6
⛔ Files ignored due to path filters (1)
src/components/ui/Slider/tests/__snapshots__/Slider.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (20)
docs/app/docs/components/toggle-group/docs/component_api/item.tsxdocs/app/docs/components/toggle-group/docs/examples/ToggleGroupExample.tsxknowledge/design_system/clarity_design_system.mdsrc/components/ui/Badge/stories/Badge.stories.tsxsrc/components/ui/RadioCards/fragments/RadioCardsRoot.tsxsrc/components/ui/Slider/fragments/SliderThumb.tsxsrc/components/ui/ToggleGroup/fragments/ToggleItem.tsxsrc/components/ui/ToggleGroup/stories/ToggleGroup.stories.tsxstyles/themes/components/_avatar-base.scssstyles/themes/components/badge.scssstyles/themes/components/card.scssstyles/themes/components/checkbox-cards.scssstyles/themes/components/context-menu.scssstyles/themes/components/fix_tokens.cjsstyles/themes/components/radio-group.scssstyles/themes/components/slider.scssstyles/themes/components/splitter.scssstyles/themes/components/tabs.scssstyles/themes/components/textarea.scssstyles/themes/components/toggle-group.scss
✅ Files skipped from review due to trivial changes (4)
- docs/app/docs/components/toggle-group/docs/examples/ToggleGroupExample.tsx
- docs/app/docs/components/toggle-group/docs/component_api/item.tsx
- src/components/ui/Slider/fragments/SliderThumb.tsx
- styles/themes/components/splitter.scss
🚧 Files skipped from review as they are similar to previous changes (5)
- styles/themes/components/badge.scss
- styles/themes/components/fix_tokens.cjs
- styles/themes/components/textarea.scss
- styles/themes/components/context-menu.scss
- styles/themes/components/slider.scss
Comment on lines
+34
to
+35
| | 8 | 700 | Solid colors | Disabled text/icons, muted UI elements | ||
| | 9 | 800 | Solid colors | supporting content |
Contributor
There was a problem hiding this comment.
Fix malformed table rows (missing trailing pipe).
Line 34 and Line 35 are missing the closing |, so markdown table rendering/linting can break.
Proposed fix
-| 8 | 700 | Solid colors | Disabled text/icons, muted UI elements
-| 9 | 800 | Solid colors | supporting content
+| 8 | 700 | Solid colors | Disabled text/icons, muted UI elements |
+| 9 | 800 | Solid colors | Supporting content |📝 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.
Suggested change
| | 8 | 700 | Solid colors | Disabled text/icons, muted UI elements | |
| | 9 | 800 | Solid colors | supporting content | |
| | 8 | 700 | Solid colors | Disabled text/icons, muted UI elements | | |
| | 9 | 800 | Solid colors | Supporting content | |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 34-34: Table pipe style
Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
(MD055, table-pipe-style)
[warning] 35-35: Table pipe style
Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
(MD055, table-pipe-style)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@knowledge/design_system/clarity_design_system.md` around lines 34 - 35, The
two markdown table rows starting with "| 8 | 700 | Solid colors
| Disabled text/icons, muted UI elements" and "| 9 | 800 | Solid colors
| supporting content" are missing the trailing pipe; update these rows in
clarity_design_system.md by appending a closing "|" to each line so the table
cells are properly terminated (i.e., ensure both rows end with " |").
Comment on lines
+37
to
+38
| | 11 | 1000 | Accessible text | Headings,Primary text, max contrast text (near black) | | ||
| --- |
Contributor
There was a problem hiding this comment.
Add a blank line after the table block.
Line 38 should be separated from the table by a blank line to satisfy markdown table boundary rules.
Proposed fix
-| 11 | 1000 | Accessible text | Headings,Primary text, max contrast text (near black) |
----
+| 11 | 1000 | Accessible text | Headings, Primary text, max contrast text (near black) |
+
+---📝 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.
Suggested change
| | 11 | 1000 | Accessible text | Headings,Primary text, max contrast text (near black) | | |
| --- | |
| | 11 | 1000 | Accessible text | Headings, Primary text, max contrast text (near black) | | |
| --- |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 37-37: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@knowledge/design_system/clarity_design_system.md` around lines 37 - 38, The
Markdown table ending with the row "| 11 | 1000 | Accessible text |
Headings,Primary text, max contrast text (near black) |" needs a blank line
added immediately after it to satisfy Markdown table boundary rules; edit the
document to insert one empty line following that table block so the subsequent
content (line 38) is separated from the table.
Comment on lines
+75
to
+114
| .rad-ui-card-content { | ||
| display: flex; | ||
| flex-direction: column; | ||
| gap: 1rem; | ||
| padding-top: 1.5rem; | ||
| } | ||
|
|
||
| .rad-ui-card-footer { | ||
| display: flex; | ||
| flex-direction: column; | ||
| gap: 0.75rem; | ||
| margin: 1.5rem -1.5rem -1.5rem; | ||
| padding: 1.25rem 1.5rem 1.5rem; | ||
| border-top: 1px solid var(--rad-ui-border); | ||
| background-color: color-mix(in oklab, var(--rad-ui-surface) 94%, var(--rad-ui-surface-subtle)); | ||
| } | ||
|
|
||
| &[data-card-size="small"] > .rad-ui-card-content { | ||
| padding-top: 1rem; | ||
| } | ||
|
|
||
| &[data-card-size="small"] > .rad-ui-card-footer { | ||
| margin: 1rem -1rem -1rem; | ||
| padding: 1rem; | ||
| } | ||
|
|
||
| &[data-card-size="large"] > .rad-ui-card-content, | ||
| &[data-card-size="x-large"] > .rad-ui-card-content { | ||
| padding-top: 1.75rem; | ||
| } | ||
|
|
||
| &[data-card-size="large"] > .rad-ui-card-footer { | ||
| margin: 1.75rem -1.75rem -1.75rem; | ||
| padding: 1.25rem 1.75rem 1.75rem; | ||
| } | ||
|
|
||
| &[data-card-size="x-large"] > .rad-ui-card-footer { | ||
| margin: 2rem -2rem -2rem; | ||
| padding: 1.5rem 2rem 2rem; | ||
| } |
Contributor
There was a problem hiding this comment.
Only add content/footer separators when they follow another section.
These rules assume Card.Content and Card.Footer are never the first child. A content-only card gets double top inset from the root padding plus padding-top, and a first footer renders with an orphan divider/gap. Make that spacing conditional, and mirror the same rule in the small/large/x-large overrides.
💡 Possible fix
.rad-ui-card-content {
display: flex;
flex-direction: column;
gap: 1rem;
- padding-top: 1.5rem;
+ padding-top: 0;
}
.rad-ui-card-footer {
display: flex;
flex-direction: column;
gap: 0.75rem;
- margin: 1.5rem -1.5rem -1.5rem;
+ margin: 0 -1.5rem -1.5rem;
padding: 1.25rem 1.5rem 1.5rem;
- border-top: 1px solid var(--rad-ui-border);
+ border-top: 0;
background-color: color-mix(in oklab, var(--rad-ui-surface) 94%, var(--rad-ui-surface-subtle));
}
+ > .rad-ui-card-content:not(:first-child) {
+ padding-top: 1.5rem;
+ }
+
+ > .rad-ui-card-footer:not(:first-child) {
+ margin-top: 1.5rem;
+ border-top: 1px solid var(--rad-ui-border);
+ }
+
- &[data-card-size="small"] > .rad-ui-card-content {
+ &[data-card-size="small"] > .rad-ui-card-content:not(:first-child) {
padding-top: 1rem;
}
&[data-card-size="small"] > .rad-ui-card-footer {
- margin: 1rem -1rem -1rem;
+ margin: 0 -1rem -1rem;
padding: 1rem;
}
+
+ &[data-card-size="small"] > .rad-ui-card-footer:not(:first-child) {
+ margin-top: 1rem;
+ }
- &[data-card-size="large"] > .rad-ui-card-content,
- &[data-card-size="x-large"] > .rad-ui-card-content {
+ &[data-card-size="large"] > .rad-ui-card-content:not(:first-child),
+ &[data-card-size="x-large"] > .rad-ui-card-content:not(:first-child) {
padding-top: 1.75rem;
}
&[data-card-size="large"] > .rad-ui-card-footer {
- margin: 1.75rem -1.75rem -1.75rem;
+ margin: 0 -1.75rem -1.75rem;
padding: 1.25rem 1.75rem 1.75rem;
}
+
+ &[data-card-size="large"] > .rad-ui-card-footer:not(:first-child) {
+ margin-top: 1.75rem;
+ }
&[data-card-size="x-large"] > .rad-ui-card-footer {
- margin: 2rem -2rem -2rem;
+ margin: 0 -2rem -2rem;
padding: 1.5rem 2rem 2rem;
}
+
+ &[data-card-size="x-large"] > .rad-ui-card-footer:not(:first-child) {
+ margin-top: 2rem;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@styles/themes/components/card.scss` around lines 75 - 114, The footer divider
and content top spacing should only apply when those sections follow another
section; update the selectors so .rad-ui-card-footer’s border/margin/padding
apply via an adjacent/sibling rule (e.g. use .rad-ui-card-content +
.rad-ui-card-footer or .rad-ui-card-header + .rad-ui-card-footer or a combined
:is(...) + .rad-ui-card-footer) instead of unconditionally on
.rad-ui-card-footer, and likewise only add .rad-ui-card-content’s extra padding
when it is not the first child (e.g. use :is(.rad-ui-card-header,
.rad-ui-card-actions) + .rad-ui-card-content or
.rad-ui-card-content:not(:first-child)); mirror these conditional selector
changes in the small/large/x-large overrides ([data-card-size="small"] >
.rad-ui-card-footer, [data-card-size="large"] > .rad-ui-card-footer,
[data-card-size="x-large"] > .rad-ui-card-footer and the corresponding content
rules) so separators and inset adjustments only appear when a preceding section
exists.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Style Updates
Documentation