feat: better comps rulebook and some QoL improvements#1905
Open
feat: better comps rulebook and some QoL improvements#1905
Conversation
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughStandardizes data-attributes and design tokens, migrates focus-ring tokens, introduces collision/boundary positioning for floating components, updates many SCSS/selectors to token-driven variables, adjusts class-name generation, expands tests, and adds Clarity audit/spec docs and an agent workflow. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ComponentRoot
participant Floater
participant PortalContainer
participant ThemeContext
User->>ComponentRoot: trigger open / interaction
ComponentRoot->>Floater: compute placement (side/align, sideOffset)
Floater->>Floater: apply middleware (flip/shift/size) with collisionBoundary & padding
Floater-->>ComponentRoot: placement, middlewareData, placedPlacement
ComponentRoot->>ThemeContext: resolve portalRootRef (if no explicit container)
ComponentRoot->>PortalContainer: render content into chosen container
PortalContainer-->>User: visible content positioned within collision boundary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
kotAPI
changed the title
![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: 7
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/TabNav/tab-nav.clarity.scss (1)
4-18: 🛠️ Refactor suggestion | 🟠 MajorUpdate the comment to reflect the new token mapping.
Line 4's comment states "step 8 (700) — muted/supporting text," but line 18 now uses
--rad-ui-text-secondaryinstead of the previous--rad-ui-color-gray-700. Iftext-secondarymaps to a different step (as suggested by the Quote component comments indicating step 11), this comment should be updated to avoid confusion.📝 Suggested comment update
// TabNav — Clarity design system aligned // Container bg: step 2 (100) — subtle surface // Container border: step 6 (500) — light border -// Link text default: step 8 (700) — muted/supporting text +// Link text default: text-secondary — muted/supporting text // Link hover bg: step 1 (50) — canvas bg (lifted feel) // Link hover text: step 11 (1000) — primary text // Link active/selected: handled by consumer via aria-current🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/TabNav/tab-nav.clarity.scss` around lines 4 - 18, Update the top comment to reflect the current token mapping: instead of referencing "step 8 (700) — muted/supporting text" (which implied --rad-ui-color-gray-700), state that the link text uses the text token --rad-ui-text-secondary and note its corresponding step/value (e.g., "text-secondary — step X (Y)") so readers know the actual token mapping; update the comment near .rad-ui-tab-nav and .rad-ui-tab-nav-link to mention --rad-ui-text-secondary explicitly and its new step/value to avoid confusion with the old gray-700 reference.src/components/ui/NumberField/fragments/NumberFieldDecrement.tsx (1)
18-23:⚠️ Potential issue | 🟠 Major
...propsorder currently allows overriding core decrement behavior.Because
...propsis applied after Line 18/20/21, callers can replaceonClick,disabled, ortype, which can break stepping and safety constraints.🔧 Suggested fix (preserve handler composition, enforce core props)
-const NumberFieldDecrement = forwardRef<NumberFieldDecrementElement, NumberFieldDecrementProps>(({ children, className, onMouseDown, ...props }, ref) => { +const NumberFieldDecrement = forwardRef<NumberFieldDecrementElement, NumberFieldDecrementProps>(({ children, className, onMouseDown, onClick, disabled: disabledProp, ...props }, ref) => { @@ <button ref={ref} - onClick={() => handleStep({ direction: 'decrement', type: 'small' })} + {...props} className={clsx(rootClass && `${rootClass}-decrement`, className)} - disabled={disabled || readOnly} + disabled={disabled || readOnly || disabledProp} type="button" - {...props} + onClick={(event) => { + onClick?.(event); + if (!event.defaultPrevented) { + handleStep({ direction: 'decrement', type: 'small' }); + } + }} tabIndex={-1} onMouseDown={(event) => { onMouseDown?.(event);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/NumberField/fragments/NumberFieldDecrement.tsx` around lines 18 - 23, The prop spread currently placed after core attributes allows callers to override critical behavior; update NumberFieldDecrement so that {...props} is applied before the component's core props or ensure core props are explicitly set after the spread: keep handleStep as the definitive onClick, compute disabled as (disabled || readOnly) and set type="button" and tabIndex={-1} after spreading user props so callers cannot replace onClick, disabled, type, or tabIndex; reference the handleStep handler and the rootClass/className usage when applying this change.src/components/ui/Splitter/stories/Splitter.stories.tsx (1)
600-629:⚠️ Potential issue | 🟠 MajorKeep panel text legible on the new dark/accent backgrounds.
SamplePanelonly changesbackgroundColor, so these new values make the heading/body text inherit the default dark foreground and several examples become unreadable. Pass an inverse foreground token with these darker panels, or switch these demos to lighter surface tokens.Also applies to: 664-720
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Splitter/stories/Splitter.stories.tsx` around lines 600 - 629, The dark background colors supplied to SamplePanel make text unreadable because SamplePanel only sets backgroundColor; update usages of SamplePanel (e.g., the instances inside Splitter.Panel and nested Splitter.Root) to either pass an explicit foreground/inverse text token prop (e.g., foreground="var(--rad-ui-foreground-inverse)" or similar) or swap the color prop to a lighter surface token so headings/body inherit readable contrast; apply the same change to the other Story instances noted (the blocks around lines 664–720) to ensure all dark/accent panels get an inverse foreground or lighter surface token.
🧹 Nitpick comments (9)
src/core/primitives/Dialog/stories/DialogPrimitive.stories.tsx (1)
30-35: Consider removing redundant background from child divs.Both the parent container (line 30) and child divs (line 32) apply the same background color. Since the parent already provides the background, the child divs don't need to redeclare it unless they require a different background.
♻️ Simplify by removing redundant background
<div className=" bg-[var(--rad-ui-surface-muted)]"> {Array.from({ length: 100 }).map((_, index) => ( - <div key={index} className="bg-[var(--rad-ui-surface-muted)]"> + <div key={index}> Lorem ipsum dolor sit amet consectetur adipisicing elit. Quisquam, quos. </div> ))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/primitives/Dialog/stories/DialogPrimitive.stories.tsx` around lines 30 - 35, Remove the redundant background class from the child divs inside the Dialog story: the parent <div className=" bg-[var(--rad-ui-surface-muted)]"> already sets the background, so delete the identical bg-[var(--rad-ui-surface-muted)] from the child <div key={index} className="bg-[var(--rad-ui-surface-muted)]"> (or replace it with only classes those children actually need) so styling is not duplicated.src/components/ui/Strong/stories/Strong.stories.tsx (1)
12-12: Consider removing the remaining hardcoded text color onStrong.Line 12 still uses
text-gray-1000on<Strong>, which partially offsets the token-migration consistency in this story.Proposed change
- <Text className="text-[var(--rad-ui-text-secondary)]">This is a very <Strong className='text-gray-1000'>Strong</Strong> word</Text> + <Text className="text-[var(--rad-ui-text-secondary)]">This is a very <Strong className="text-[var(--rad-ui-text-primary)]">Strong</Strong> word</Text>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Strong/stories/Strong.stories.tsx` at line 12, The Strong story still applies a hardcoded Tailwind color class on the <Strong> component ("text-gray-1000"); remove that literal class (or replace it with the design token/class used elsewhere, e.g. the token variable for strong text) so the <Strong> usage inherits tokenized color consistency from <Text> and the component itself (locate the usage in Strong.stories.tsx where <Text> wraps <Strong> and remove/replace className='text-gray-1000').src/components/ui/Code/stories/Code.stories.tsx (1)
60-60: Consider migrating remaining hardcoded text colors to design tokens.Lines 60 and 85 still use
text-gray-950, while other text colors in this file were updated to use CSS variables. For consistency, consider updating these totext-[var(--rad-ui-text-primary)]or the appropriate semantic token.♻️ Suggested token migration
- <p className='text-gray-950'>Code Sizes</p> + <p className='text-[var(--rad-ui-text-primary)]'>Code Sizes</p>- <p className='text-gray-950'>Code Variants</p> + <p className='text-[var(--rad-ui-text-primary)]'>Code Variants</p>Also applies to: 85-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Code/stories/Code.stories.tsx` at line 60, Replace the hardcoded Tailwind color class "text-gray-950" with the semantic design token class "text-[var(--rad-ui-text-primary)]" in the JSX elements that render the "Code Sizes" label and the other similar heading in the same file (search for the "Code Sizes" string and the second occurrence using text-gray-950). Update the className on those <p> elements (in src/components/ui/Code/stories/Code.stories.tsx) so both instances use the CSS variable token for primary text color to match the other migrated text colors.src/core/primitives/Checkbox/stories/CheckboxPrimitive.stories.tsx (1)
17-17: Consider finishing tokenization in these same class strings.
bg-[var(--rad-ui-surface-muted)]was updated, butborder-blue-800remains hardcoded in both updated roots. Replacing that with a--rad-ui-*border token would keep the story fully design-system aligned.Also applies to: 43-43
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/primitives/Checkbox/stories/CheckboxPrimitive.stories.tsx` at line 17, Replace the hardcoded "border-blue-800" in the CheckboxPrimitive.Root story class strings with a design-system border token (e.g. use the CSS variable form via var(--rad-ui-...)) so the story is fully tokenized; update both instances of CheckboxPrimitive.Root (the root at the top and the second occurrence) to use border-[var(--rad-ui-border-...)] (choose the appropriate token name like --rad-ui-border-strong or --rad-ui-border-muted) so the story aligns with the other bg token usage.src/components/ui/Button/tests/Button.test.tsx (1)
35-40: Make the stylesheet assertion less brittle.Current
lastIndexOf/exact-string checks can fail on formatting-only changes or pass due to unrelated selector occurrences. Consider scoping and regex-based checks.Proposed test hardening
- test('focus-visible styles are declared after variants so focus shadow wins', () => { - const stylesheet = fs.readFileSync(path.resolve(__dirname, '../button.clarity.scss'), 'utf8'); - expect(stylesheet.lastIndexOf('&:focus-visible')).toBeGreaterThan(stylesheet.lastIndexOf('&[data-button-variant="ghost"]')); - expect(stylesheet.lastIndexOf('&:focus-visible')).toBeGreaterThan(stylesheet.lastIndexOf('&[data-button-variant="outline"]')); - expect(stylesheet).toContain('box-shadow: var(--rad-ui-focus-ring-shadow-offset-panel), var(--rad-ui-shadow-sm);'); - }); + test('focus-visible styles are declared after variants so focus shadow wins', () => { + const stylesheet = fs.readFileSync(path.resolve(__dirname, '../button.clarity.scss'), 'utf8'); + const focusPos = stylesheet.indexOf('&:focus-visible'); + const ghostPos = stylesheet.indexOf('&[data-button-variant="ghost"]'); + const outlinePos = stylesheet.indexOf('&[data-button-variant="outline"]'); + + expect(focusPos).toBeGreaterThan(-1); + expect(ghostPos).toBeGreaterThan(-1); + expect(outlinePos).toBeGreaterThan(-1); + expect(focusPos).toBeGreaterThan(ghostPos); + expect(focusPos).toBeGreaterThan(outlinePos); + expect(stylesheet).toMatch( + /box-shadow:\s*var\(--rad-ui-focus-ring-shadow-offset-panel\),\s*var\(--rad-ui-shadow-sm\);/ + ); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Button/tests/Button.test.tsx` around lines 35 - 40, The test "focus-visible styles are declared after variants so focus shadow wins" is brittle because it relies on exact lastIndexOf and exact strings; update the assertion to scope and match selectors with regex or a CSS parser: read the stylesheet into the existing stylesheet variable, use a regex (or PostCSS/CSSOM) to locate the specific selector blocks for '&:focus-visible', '&[data-button-variant="ghost"]', and '&[data-button-variant="outline"]' and assert the focus-visible rule block appears after both variant blocks and contains the expected box-shadow value; replace the direct lastIndexOf checks with these scoped matches so formatting or unrelated selector occurrences won't break the test.src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx (1)
36-37: Consider extracting repeated radio class strings into constants.This is purely maintainability: same utility strings appear twice and can drift over time.
♻️ Optional cleanup
+const RADIO_ITEM_CLASS = 'w-4 h-4 rounded-full bg-[var(--rad-ui-surface-inverse)]'; +const RADIO_INDICATOR_CLASS = 'w-4 h-4 rounded-full flex items-center justify-center bg-[var(--rad-ui-color-accent-800)]'; ... -<RadioGroupPrimitive.Item value={option.value} className='w-4 h-4 rounded-full bg-[var(--rad-ui-surface-inverse)]'> - <RadioGroupPrimitive.Indicator className='w-4 h-4 rounded-full flex items-center justify-center bg-[var(--rad-ui-color-accent-800)]' /> +<RadioGroupPrimitive.Item value={option.value} className={RADIO_ITEM_CLASS}> + <RadioGroupPrimitive.Indicator className={RADIO_INDICATOR_CLASS} /> ... -<RadioGroupPrimitive.Item value={option.value} className='w-4 h-4 rounded-full bg-[var(--rad-ui-surface-inverse)]'> - <RadioGroupPrimitive.Indicator className='w-4 h-4 rounded-full flex items-center justify-center bg-[var(--rad-ui-color-accent-800)]' /> +<RadioGroupPrimitive.Item value={option.value} className={RADIO_ITEM_CLASS}> + <RadioGroupPrimitive.Indicator className={RADIO_INDICATOR_CLASS} />Also applies to: 85-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx` around lines 36 - 37, The repeated Tailwind class strings used on RadioGroupPrimitive.Item and RadioGroupPrimitive.Indicator should be extracted into shared constants to avoid drift; create two constants (e.g., ITEM_RADIO_CLASS and INDICATOR_RADIO_CLASS) near the top of RadioGroupPrimitive.stories.tsx, replace the inline className values on RadioGroupPrimitive.Item and RadioGroupPrimitive.Indicator (and the other occurrence at lines ~85-86) with those constants, and export or reuse them within the file so both places use the same identifier.src/components/ui/Tabs/stories/Tabs.stories.tsx (1)
157-170: Use design tokens for active selector button states too.The active branch still hardcodes blue/gray utility colors; swapping to token-based classes keeps this story consistent with the rest of the file.
♻️ Suggested update
-className={`mr-2 px-4 py-2 rounded ${value === 'tab1' ? 'bg-blue-500 text-gray-50' : 'bg-[var(--rad-ui-surface-muted)]'}`} +className={`mr-2 px-4 py-2 rounded ${value === 'tab1' ? 'bg-[var(--rad-ui-color-accent-700)] text-[var(--rad-ui-color-gray-50)]' : 'bg-[var(--rad-ui-surface-muted)]'}`} -className={`mr-2 px-4 py-2 rounded ${value === 'tab2' ? 'bg-blue-500 text-gray-50' : 'bg-[var(--rad-ui-surface-muted)]'}`} +className={`mr-2 px-4 py-2 rounded ${value === 'tab2' ? 'bg-[var(--rad-ui-color-accent-700)] text-[var(--rad-ui-color-gray-50)]' : 'bg-[var(--rad-ui-surface-muted)]'}`} -className={`px-4 py-2 rounded ${value === 'tab3' ? 'bg-blue-500 text-gray-50' : 'bg-[var(--rad-ui-surface-muted)]'}`} +className={`px-4 py-2 rounded ${value === 'tab3' ? 'bg-[var(--rad-ui-color-accent-700)] text-[var(--rad-ui-color-gray-50)]' : 'bg-[var(--rad-ui-surface-muted)]'}`}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Tabs/stories/Tabs.stories.tsx` around lines 157 - 170, The story uses hardcoded utility colors for the active button state; update the three button className expressions (the button with onClick={() => setValue('tab1')}, the button with onClick={() => setValue('tab2')}, and the button with onClick={() => setValue('tab3')}) to use the design token classes instead of 'bg-blue-500 text-gray-50' (for example replace with the project tokens like the primary/background and on-primary tokens such as bg-[var(--rad-ui-primary)] and text-[var(--rad-ui-on-primary)]), and ensure the inactive branch continues to use the surface muted token (bg-[var(--rad-ui-surface-muted)]); keep the conditional structure around value === 'tabX' but swap the hardcoded color utilities for the corresponding token-based classes.src/components/ui/ScrollArea/stories/ScrollArea.stories.tsx (1)
198-205: Finish token migration inHorizontalOnlyitem cards.Line 205 still uses hardcoded blue utility classes, which breaks the tokenized styling pattern used elsewhere in this file.
♻️ Suggested update
-<div key={i} className="min-w-[200px] h-16 bg-blue-50 border border-blue-200 flex items-center justify-center"> +<div key={i} className="min-w-[200px] h-16 bg-[var(--rad-ui-surface-subtle)] border border-[var(--rad-ui-border-soft)] flex items-center justify-center">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/ScrollArea/stories/ScrollArea.stories.tsx` around lines 198 - 205, The item cards in the HorizontalOnly story still use hardcoded blue utilities ("bg-blue-50", "border-blue-200") which breaks the tokenized styling; update the mapped card element inside ScrollArea.Viewport (the div with className "min-w-[200px] h-16 bg-blue-50 border border-blue-200 flex items-center justify-center") to use the project's design tokens consistent with other stories (replace bg-blue-50 and border-blue-200 with the corresponding token classes used elsewhere in this file, e.g., the tokenized background and border variables).src/components/ui/Button/button.clarity.scss (1)
2-4: Route the solid variant through the new theme-level solid tokens.These defaults bypass the
--rad-ui-solid-background/--rad-ui-solid-hovertokens added in this PR, so theme overrides won't propagate cleanly to Button's solid variant.Possible refactor
- --rad-ui-button-solid-background: var(--rad-ui-color-accent-950); - --rad-ui-button-solid-hover: var(--rad-ui-color-accent-1000); - --rad-ui-button-solid-foreground: var(--rad-ui-color-accent-50); + --rad-ui-button-solid-background: var(--rad-ui-solid-background); + --rad-ui-button-solid-hover: var(--rad-ui-solid-hover); + --rad-ui-button-solid-foreground: var(--rad-ui-text-inverse);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Button/button.clarity.scss` around lines 2 - 4, Replace the hard-coded accent token references used by the Button solid variant with the new theme-level solid tokens so theme overrides propagate: update the CSS variables --rad-ui-button-solid-background and --rad-ui-button-solid-hover to use --rad-ui-solid-background and --rad-ui-solid-hover (with optional fallbacks to the existing --rad-ui-color-accent-950 / --rad-ui-color-accent-1000), and ensure the foreground (--rad-ui-button-solid-foreground) reads from the theme-level solid foreground token (e.g. --rad-ui-solid-foreground) or falls back to --rad-ui-color-accent-50; modify these variables in button.clarity.scss where --rad-ui-button-solid-* are defined.
🤖 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/NavigationMenu/navigation-menu.clarity.scss`:
- Around line 84-85: The rule currently sets width: max-content and white-space:
nowrap which prevents long labels from honoring max-width; update the rule that
contains "width: max-content" and the later block with "white-space: nowrap" so
labels can wrap and respect the cap: remove or change width: max-content to
width: auto (or width: 100% constrained by the existing max-width), set
white-space: normal, and add overflow-wrap: anywhere (or word-break: break-word)
so long localized labels wrap inside the max-width; apply the same fixes to the
other occurrence referenced (lines 97-103).
In `@src/components/ui/NumberField/number-field.clarity.scss`:
- Around line 21-25: The stylesheet currently applies the same muted styling to
both &:has(.rad-ui-number-field-input:disabled) and
&:has(.rad-ui-number-field-input:read-only); split these into separate rules so
disabled keeps the muted background/border/box-shadow and read-only does NOT get
disabled styling (preserve normal background, cursor, focus outlines and text
color so it remains focusable/copiable); update the selectors
.rad-ui-number-field-input:read-only (and the matching occurrence at lines
~50-54) to only adjust properties appropriate for read-only (e.g., remove
pointer-events: none and cursor: not-allowed, keep readable text color) while
leaving disabled styling on .rad-ui-number-field-input:disabled as-is.
In `@src/components/ui/RadioGroup/radio-group.baremetal.scss`:
- Line 5: The selector :where(> [role="group"]) is invalid; change both
occurrences to move the child combinator outside the :where() so the
pseudo-class only wraps the simple selector. For example, replace patterns like
"S :where(> [role='group'])" with "S > :where([role='group'])" (i.e., use
:where([role="group"]) and apply the ">" between the left-hand selector and
:where), updating the instances of :where(> [role="group"]) found in the file.
In `@src/components/ui/Theme/stories/Theme.stories.tsx`:
- Around line 9-10: Replace the hardcoded "text-gray-950" class in the JSX
return (the <h1> element that currently reads 'Hello World, This is in
{appearance} mode') with the theme token CSS variable used by the project (e.g.
use a class that references a CSS var like
text-[var(--rad-ui-foreground-on-surface)] or whichever token your design system
uses), so the heading color follows appearance toggles and avoids dark-mode
contrast regressions.
In `@src/core/primitives/Menu/fragments/MenuPrimitiveRoot.tsx`:
- Around line 67-74: The Floater.shift middleware is currently always applied
even when avoidCollision is false; update the middleware list in
MenuPrimitiveRoot so collision-driven middlewares are gated by avoidCollision
(like Floater.flip is) — e.g., only include Floater.shift(...) when
avoidCollision is true (respecting isNested for crossAxis as before), ensuring
both Floater.flip and Floater.shift are conditionally added based on the
avoidCollision flag.
In `@src/core/primitives/Menu/stories/MenuPrimitive.stories.tsx`:
- Line 28: The MenuPrimitive.Item in the story has mismatched label and rendered
text: the element MenuPrimitive.Item uses label="item 3" but its children render
"item 1"; update the children text to match the label (or vice versa) so label
and visible content are consistent (e.g., change the inner text to "item 3") in
the MenuPrimitive.Item entry in the Menu story.
In `@src/design-systems/clarity/default.scss`:
- Around line 74-75: The two variables --rad-ui-text-secondary and
--rad-ui-text-muted currently resolve to the same gray
(--rad-ui-color-gray-950), collapsing emphasis; change --rad-ui-text-muted to a
lighter/less-contrast gray step than --rad-ui-text-secondary (e.g., point
--rad-ui-text-muted at a lighter token such as --rad-ui-color-gray-800 or
similar) so placeholders/disabled/helper copy render visually below supporting
content.
---
Outside diff comments:
In `@src/components/ui/NumberField/fragments/NumberFieldDecrement.tsx`:
- Around line 18-23: The prop spread currently placed after core attributes
allows callers to override critical behavior; update NumberFieldDecrement so
that {...props} is applied before the component's core props or ensure core
props are explicitly set after the spread: keep handleStep as the definitive
onClick, compute disabled as (disabled || readOnly) and set type="button" and
tabIndex={-1} after spreading user props so callers cannot replace onClick,
disabled, type, or tabIndex; reference the handleStep handler and the
rootClass/className usage when applying this change.
In `@src/components/ui/Splitter/stories/Splitter.stories.tsx`:
- Around line 600-629: The dark background colors supplied to SamplePanel make
text unreadable because SamplePanel only sets backgroundColor; update usages of
SamplePanel (e.g., the instances inside Splitter.Panel and nested Splitter.Root)
to either pass an explicit foreground/inverse text token prop (e.g.,
foreground="var(--rad-ui-foreground-inverse)" or similar) or swap the color prop
to a lighter surface token so headings/body inherit readable contrast; apply the
same change to the other Story instances noted (the blocks around lines 664–720)
to ensure all dark/accent panels get an inverse foreground or lighter surface
token.
In `@src/components/ui/TabNav/tab-nav.clarity.scss`:
- Around line 4-18: Update the top comment to reflect the current token mapping:
instead of referencing "step 8 (700) — muted/supporting text" (which implied
--rad-ui-color-gray-700), state that the link text uses the text token
--rad-ui-text-secondary and note its corresponding step/value (e.g.,
"text-secondary — step X (Y)") so readers know the actual token mapping; update
the comment near .rad-ui-tab-nav and .rad-ui-tab-nav-link to mention
--rad-ui-text-secondary explicitly and its new step/value to avoid confusion
with the old gray-700 reference.
---
Nitpick comments:
In `@src/components/ui/Button/button.clarity.scss`:
- Around line 2-4: Replace the hard-coded accent token references used by the
Button solid variant with the new theme-level solid tokens so theme overrides
propagate: update the CSS variables --rad-ui-button-solid-background and
--rad-ui-button-solid-hover to use --rad-ui-solid-background and
--rad-ui-solid-hover (with optional fallbacks to the existing
--rad-ui-color-accent-950 / --rad-ui-color-accent-1000), and ensure the
foreground (--rad-ui-button-solid-foreground) reads from the theme-level solid
foreground token (e.g. --rad-ui-solid-foreground) or falls back to
--rad-ui-color-accent-50; modify these variables in button.clarity.scss where
--rad-ui-button-solid-* are defined.
In `@src/components/ui/Button/tests/Button.test.tsx`:
- Around line 35-40: The test "focus-visible styles are declared after variants
so focus shadow wins" is brittle because it relies on exact lastIndexOf and
exact strings; update the assertion to scope and match selectors with regex or a
CSS parser: read the stylesheet into the existing stylesheet variable, use a
regex (or PostCSS/CSSOM) to locate the specific selector blocks for
'&:focus-visible', '&[data-button-variant="ghost"]', and
'&[data-button-variant="outline"]' and assert the focus-visible rule block
appears after both variant blocks and contains the expected box-shadow value;
replace the direct lastIndexOf checks with these scoped matches so formatting or
unrelated selector occurrences won't break the test.
In `@src/components/ui/Code/stories/Code.stories.tsx`:
- Line 60: Replace the hardcoded Tailwind color class "text-gray-950" with the
semantic design token class "text-[var(--rad-ui-text-primary)]" in the JSX
elements that render the "Code Sizes" label and the other similar heading in the
same file (search for the "Code Sizes" string and the second occurrence using
text-gray-950). Update the className on those <p> elements (in
src/components/ui/Code/stories/Code.stories.tsx) so both instances use the CSS
variable token for primary text color to match the other migrated text colors.
In `@src/components/ui/ScrollArea/stories/ScrollArea.stories.tsx`:
- Around line 198-205: The item cards in the HorizontalOnly story still use
hardcoded blue utilities ("bg-blue-50", "border-blue-200") which breaks the
tokenized styling; update the mapped card element inside ScrollArea.Viewport
(the div with className "min-w-[200px] h-16 bg-blue-50 border border-blue-200
flex items-center justify-center") to use the project's design tokens consistent
with other stories (replace bg-blue-50 and border-blue-200 with the
corresponding token classes used elsewhere in this file, e.g., the tokenized
background and border variables).
In `@src/components/ui/Strong/stories/Strong.stories.tsx`:
- Line 12: The Strong story still applies a hardcoded Tailwind color class on
the <Strong> component ("text-gray-1000"); remove that literal class (or replace
it with the design token/class used elsewhere, e.g. the token variable for
strong text) so the <Strong> usage inherits tokenized color consistency from
<Text> and the component itself (locate the usage in Strong.stories.tsx where
<Text> wraps <Strong> and remove/replace className='text-gray-1000').
In `@src/components/ui/Tabs/stories/Tabs.stories.tsx`:
- Around line 157-170: The story uses hardcoded utility colors for the active
button state; update the three button className expressions (the button with
onClick={() => setValue('tab1')}, the button with onClick={() =>
setValue('tab2')}, and the button with onClick={() => setValue('tab3')}) to use
the design token classes instead of 'bg-blue-500 text-gray-50' (for example
replace with the project tokens like the primary/background and on-primary
tokens such as bg-[var(--rad-ui-primary)] and text-[var(--rad-ui-on-primary)]),
and ensure the inactive branch continues to use the surface muted token
(bg-[var(--rad-ui-surface-muted)]); keep the conditional structure around value
=== 'tabX' but swap the hardcoded color utilities for the corresponding
token-based classes.
In `@src/core/primitives/Checkbox/stories/CheckboxPrimitive.stories.tsx`:
- Line 17: Replace the hardcoded "border-blue-800" in the CheckboxPrimitive.Root
story class strings with a design-system border token (e.g. use the CSS variable
form via var(--rad-ui-...)) so the story is fully tokenized; update both
instances of CheckboxPrimitive.Root (the root at the top and the second
occurrence) to use border-[var(--rad-ui-border-...)] (choose the appropriate
token name like --rad-ui-border-strong or --rad-ui-border-muted) so the story
aligns with the other bg token usage.
In `@src/core/primitives/Dialog/stories/DialogPrimitive.stories.tsx`:
- Around line 30-35: Remove the redundant background class from the child divs
inside the Dialog story: the parent <div className="
bg-[var(--rad-ui-surface-muted)]"> already sets the background, so delete the
identical bg-[var(--rad-ui-surface-muted)] from the child <div key={index}
className="bg-[var(--rad-ui-surface-muted)]"> (or replace it with only classes
those children actually need) so styling is not duplicated.
In `@src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx`:
- Around line 36-37: The repeated Tailwind class strings used on
RadioGroupPrimitive.Item and RadioGroupPrimitive.Indicator should be extracted
into shared constants to avoid drift; create two constants (e.g.,
ITEM_RADIO_CLASS and INDICATOR_RADIO_CLASS) near the top of
RadioGroupPrimitive.stories.tsx, replace the inline className values on
RadioGroupPrimitive.Item and RadioGroupPrimitive.Indicator (and the other
occurrence at lines ~85-86) with those constants, and export or reuse them
within the file so both places use the same identifier.
🪄 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: 369dfe93-5013-4cfb-8f1e-870cd2ec8124
📒 Files selected for processing (89)
agents/development/sync-verify-clarity-usage.mddocs/app/docs/components/hover-card/docs/example_1.tsxdocs/app/docs/components/hover-card/examples/HoverCardSizes.tsxdocs/app/docs/components/radio-group/docs/component_api/root.tsxknowledge/design_system/clarity_design_system.mdsrc/components/tools/SandboxEditor/SandboxEditor.tsxsrc/components/ui/Accordion/accordion.clarity.scsssrc/components/ui/Avatar/stories/Avatar.stories.tsxsrc/components/ui/Badge/stories/Badge.stories.tsxsrc/components/ui/BlockQuote/stories/BlockQuote.stories.tsxsrc/components/ui/Button/button.clarity.scsssrc/components/ui/Button/stories/Button.stories.tsxsrc/components/ui/Button/tests/Button.test.tsxsrc/components/ui/Callout/stories/Callout.stories.tsxsrc/components/ui/Checkbox/checkbox.clarity.scsssrc/components/ui/Checkbox/stories/Checkbox.stories.tsxsrc/components/ui/CheckboxCards/checkbox-cards.clarity.scsssrc/components/ui/CheckboxGroup/checkbox-group.clarity.scsssrc/components/ui/Code/stories/Code.stories.tsxsrc/components/ui/Collapsible/collapsible.clarity.scsssrc/components/ui/Combobox/combobox.clarity.scsssrc/components/ui/Command/command.clarity.scsssrc/components/ui/ContextMenu/context-menu.clarity.scsssrc/components/ui/DataList/stories/DataList.stories.tsxsrc/components/ui/Dialog/dialog.clarity.scsssrc/components/ui/Disclosure/disclosure.clarity.scsssrc/components/ui/Drawer/drawer.clarity.scsssrc/components/ui/DropdownMenu/dropdown-menu.clarity.scsssrc/components/ui/HoverCard/fragments/HoverCardPortal.tsxsrc/components/ui/HoverCard/fragments/HoverCardRoot.tsxsrc/components/ui/HoverCard/hover-card.clarity.scsssrc/components/ui/HoverCard/stories/HoverCard.stories.tsxsrc/components/ui/HoverCard/tests/HoverCard.test.tsxsrc/components/ui/Link/link.clarity.scsssrc/components/ui/Menubar/menubar.clarity.scsssrc/components/ui/Minimap/minimap.clarity.scsssrc/components/ui/NavigationMenu/fragments/NavigationMenuRoot.tsxsrc/components/ui/NavigationMenu/fragments/NavigationMenuTrigger.tsxsrc/components/ui/NavigationMenu/navigation-menu.clarity.scsssrc/components/ui/NavigationMenu/tests/NavigationMenu.test.tsxsrc/components/ui/NumberField/fragments/NumberFieldDecrement.tsxsrc/components/ui/NumberField/fragments/NumberFieldIncrement.tsxsrc/components/ui/NumberField/number-field.clarity.scsssrc/components/ui/NumberField/tests/NumberField.test.tsxsrc/components/ui/Quote/quote.clarity.scsssrc/components/ui/Quote/stories/Quote.stories.tsxsrc/components/ui/Radio/radio.clarity.scsssrc/components/ui/RadioCards/radiocards.clarity.scsssrc/components/ui/RadioGroup/radio-group.baremetal.scsssrc/components/ui/RadioGroup/radio-group.clarity.scsssrc/components/ui/RadioGroup/tests/RadioGroup.behavior.test.tsxsrc/components/ui/ScrollArea/scroll-area.clarity.scsssrc/components/ui/ScrollArea/stories/ScrollArea.stories.tsxsrc/components/ui/Select/select.clarity.scsssrc/components/ui/Separator/stories/Separator.stories.tsxsrc/components/ui/Skeleton/stories/Skeleton.stories.tsxsrc/components/ui/Slider/slider.clarity.scsssrc/components/ui/Slider/stories/Slider.stories.tsxsrc/components/ui/Splitter/demo.tsxsrc/components/ui/Splitter/splitter.clarity.scsssrc/components/ui/Splitter/stories/Splitter.stories.tsxsrc/components/ui/Steps/steps.clarity.scsssrc/components/ui/Strong/stories/Strong.stories.tsxsrc/components/ui/Switch/switch.clarity.scsssrc/components/ui/TabNav/tab-nav.clarity.scsssrc/components/ui/Tabs/stories/Tabs.stories.tsxsrc/components/ui/Tabs/tabs.clarity.scsssrc/components/ui/TextArea/textarea.clarity.scsssrc/components/ui/Theme/stories/Theme.stories.tsxsrc/components/ui/Toggle/toggle.clarity.scsssrc/components/ui/ToggleGroup/toggle-group.clarity.scsssrc/components/ui/Toolbar/stories/Toolbar.stories.tsxsrc/components/ui/Toolbar/toolbar.clarity.scsssrc/components/ui/Tree/stories/Tree.stories.tsxsrc/components/ui/Tree/tree.clarity.scsssrc/core/primitives/Checkbox/stories/CheckboxPrimitive.stories.tsxsrc/core/primitives/CheckboxGroup/stories/CheckboxGroupPrimitive.stories.tsxsrc/core/primitives/Collapsible/stories/Collapsible.stories.tsxsrc/core/primitives/Combobox/stories/Combobox.stories.tsxsrc/core/primitives/Dialog/stories/DialogPrimitive.stories.tsxsrc/core/primitives/Menu/fragments/MenuPrimitiveContent.tsxsrc/core/primitives/Menu/fragments/MenuPrimitiveRoot.tsxsrc/core/primitives/Menu/stories/MenuPrimitive.stories.tsxsrc/core/primitives/Menu/tests/MenuPrimitive.test.tsxsrc/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsxsrc/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveRoot.tsxsrc/core/utils/RovingFocusGroup/stories/RovingFocusGroup.stories.tsxsrc/design-systems/clarity/default.scsssrc/examples/Colors/Colors.stories.js
Comment on lines
+84
to
+85
| width: max-content; | ||
| max-width: min(28rem, calc(100vw - 2rem)); |
Contributor
There was a problem hiding this comment.
Allow long menu labels to respect the new width cap.
max-width only helps if the label can actually break. With width: max-content here and white-space: nowrap below, long localized labels will overflow the popup instead of fitting within the viewport constraint.
Possible fix
.rad-ui-navigation-menu-content .rad-ui-navigation-menu-link {
width: 100%;
box-sizing: border-box;
justify-content: flex-start;
min-height: 2.25rem;
border-radius: var(--rad-ui-radius-md);
padding: 0.65rem 0.8rem;
color: var(--rad-ui-text-secondary);
- white-space: nowrap;
+ white-space: normal;
+ overflow-wrap: anywhere;
}Also applies to: 97-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/NavigationMenu/navigation-menu.clarity.scss` around lines
84 - 85, The rule currently sets width: max-content and white-space: nowrap
which prevents long labels from honoring max-width; update the rule that
contains "width: max-content" and the later block with "white-space: nowrap" so
labels can wrap and respect the cap: remove or change width: max-content to
width: auto (or width: 100% constrained by the existing max-width), set
white-space: normal, and add overflow-wrap: anywhere (or word-break: break-word)
so long localized labels wrap inside the max-width; apply the same fixes to the
other occurrence referenced (lines 97-103).
Comment on lines
+21
to
+25
| &:has(.rad-ui-number-field-input:disabled), | ||
| &:has(.rad-ui-number-field-input:read-only) { | ||
| background-color: var(--rad-ui-surface-subtle); | ||
| border-color: var(--rad-ui-border-soft); | ||
| box-shadow: none; |
Contributor
There was a problem hiding this comment.
Don't style read-only the same as disabled.
A read-only number field can still be focused and copied, but these selectors mute it and show cursor: not-allowed exactly like a disabled control. That misrepresents the state and makes read-only values look unusable.
Possible fix
- &:has(.rad-ui-number-field-input:disabled),
- &:has(.rad-ui-number-field-input:read-only) {
+ &:has(.rad-ui-number-field-input:disabled) {
background-color: var(--rad-ui-surface-subtle);
border-color: var(--rad-ui-border-soft);
box-shadow: none;
}
+
+ &:has(.rad-ui-number-field-input:read-only) {
+ background-color: var(--rad-ui-surface-subtle);
+ border-color: var(--rad-ui-border-soft);
+ }
@@
- &:disabled,
- &:read-only {
+ &:disabled {
color: var(--rad-ui-text-muted);
cursor: not-allowed;
}
+
+ &:read-only {
+ color: var(--rad-ui-text-secondary);
+ cursor: text;
+ }Also applies to: 50-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/NumberField/number-field.clarity.scss` around lines 21 -
25, The stylesheet currently applies the same muted styling to both
&:has(.rad-ui-number-field-input:disabled) and
&:has(.rad-ui-number-field-input:read-only); split these into separate rules so
disabled keeps the muted background/border/box-shadow and read-only does NOT get
disabled styling (preserve normal background, cursor, focus outlines and text
color so it remains focusable/copiable); update the selectors
.rad-ui-number-field-input:read-only (and the matching occurrence at lines
~50-54) to only adjust properties appropriate for read-only (e.g., remove
pointer-events: none and cursor: not-allowed, keep readable text color) while
leaving disabled styling on .rad-ui-number-field-input:disabled as-is.
| @use "src/design-systems/baremetal/scope" as baremetal; | ||
|
|
||
| #{baremetal.$theme-root} :where(.rad-ui-radio-group-root) { | ||
| :where(> [role="group"]) { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In CSS Selectors Level 4, is :where(> [role="group"]) valid syntax? If not, what is the correct way to target direct children while keeping low specificity?
💡 Result:
No. The selector :where(> [role="group"]) is invalid syntax. :where takes a selector list as its argument (i.e., one or more selectors separated by commas). Inside that list you can use combinators only as part of “complex selectors” (anchored relative to the element the overall selector is evaluated on). A leading combinator like > by itself is not a complete selector to be used as an argument item. Correct way to target direct children with low specificity: 1) Use the child combinator outside :where: .container:where(> [role="group"]) { ... } But if you want to keep the full match low-specificity while still using >, put the element(s) that should contribute specificity outside :where and keep the rest inside. 2) Typical pattern (zero specificity for the filter part): .container > :where([role="group"]) { ... } Explanation: the child combinator > matches only direct children. The :where pseudo-class contributes zero specificity, so the [role="group"] filter won’t add specificity, keeping the rule easy to override. Sources: Selectors Level 4 defines :where as a specificity-adjustment pseudo-class and describes its selector-list argument; MDN documents that :where takes a selector list and always has 0 specificity. The child combinator > matches direct children only.
Citations:
- 1: https://www.w3.org/TR/selectors-4/
- 2: https://developer.mozilla.org/en-US/docs/Web/CSS/:where
- 3: https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Selectors/Child_combinator
- 4: https://www.w3.org/TR/selectors4/
Fix invalid CSS selector syntax: :where(> [role="group"]) is invalid and will be dropped.
At Lines 5 and 11, :where(> [role="group"]) is not valid per CSS Selectors Level 4. The :where() pseudo-class accepts a selector list but does not allow a leading combinator like > by itself as an argument. This rule will be silently dropped by the CSS parser, and the layout behavior will not apply.
Move the child combinator outside :where():
Fix
#{baremetal.$theme-root} :where(.rad-ui-radio-group-root) {
- :where(> [role="group"]) {
+ > [role="group"] {
display: flex;
flex-flow: row wrap;
gap: 0.875rem 2rem;
}
- &:where([aria-orientation="vertical"]) :where(> [role="group"]) {
+ &:where([aria-orientation="vertical"]) > [role="group"] {
flex-flow: column nowrap;
align-items: flex-start;
gap: 0.875rem;
}
}Also applies to: Line 11
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/RadioGroup/radio-group.baremetal.scss` at line 5, The
selector :where(> [role="group"]) is invalid; change both occurrences to move
the child combinator outside the :where() so the pseudo-class only wraps the
simple selector. For example, replace patterns like "S :where(> [role='group'])"
with "S > :where([role='group'])" (i.e., use :where([role="group"]) and apply
the ">" between the left-hand selector and :where), updating the instances of
:where(> [role="group"]) found in the file.
Comment on lines
+9
to
10
| return <div className="bg-[var(--rad-ui-surface-canvas)] p-4 rounded-lg"> | ||
| <h1 className="text-gray-950">Hello World, This is in {appearance} mode</h1> |
Contributor
There was a problem hiding this comment.
Tokenize heading text color too to avoid dark-mode contrast regressions.
Line 10 still hardcodes text-gray-950; in a theme-toggle story this can produce poor contrast in dark appearance. Please switch it to a theme token.
💡 Proposed fix
- <h1 className="text-gray-950">Hello World, This is in {appearance} mode</h1>
+ <h1 className="text-[var(--rad-ui-text-primary)]">Hello World, This is in {appearance} mode</h1>📝 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
| return <div className="bg-[var(--rad-ui-surface-canvas)] p-4 rounded-lg"> | |
| <h1 className="text-gray-950">Hello World, This is in {appearance} mode</h1> | |
| return <div className="bg-[var(--rad-ui-surface-canvas)] p-4 rounded-lg"> | |
| <h1 className="text-[var(--rad-ui-text-primary)]">Hello World, This is in {appearance} mode</h1> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/Theme/stories/Theme.stories.tsx` around lines 9 - 10,
Replace the hardcoded "text-gray-950" class in the JSX return (the <h1> element
that currently reads 'Hello World, This is in {appearance} mode') with the theme
token CSS variable used by the project (e.g. use a class that references a CSS
var like text-[var(--rad-ui-foreground-on-surface)] or whichever token your
design system uses), so the heading color follows appearance toggles and avoids
dark-mode contrast regressions.
Comment on lines
67
to
74
| Floater.flip({ | ||
| mainAxis: avoidCollision, | ||
| crossAxis: avoidCollision | ||
| crossAxis: avoidCollision && !isNested | ||
| }), | ||
| Floater.shift({ | ||
| padding: 4, | ||
| crossAxis: !isNested | ||
| }), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how avoidCollision is wired with floating middlewares in this repo.
rg -n -C3 '\bavoidCollision\b|Floater\.flip\(|Floater\.shift\(' src/core src/componentsRepository: rad-ui/ui
Length of output: 3881
🏁 Script executed:
cat -n src/core/primitives/Menu/tests/MenuPrimitive.test.tsx | sed -n '75,110p'Repository: rad-ui/ui
Length of output: 1469
🏁 Script executed:
cat -n src/core/primitives/Menu/fragments/MenuPrimitiveRoot.tsx | sed -n '50,85p'Repository: rad-ui/ui
Length of output: 1530
🏁 Script executed:
rg -n "avoidCollision.*false" src/core/primitives/Menu/tests/ -A 5 -B 2Repository: rad-ui/ui
Length of output: 738
🏁 Script executed:
grep -rn "shift" src/core/primitives/Menu/ --include="*.tsx" --include="*.ts" | head -20Repository: rad-ui/ui
Length of output: 461
🏁 Script executed:
cat -n src/core/primitives/Menu/tests/MenuPrimitive.test.tsx | sed -n '110,150p'Repository: rad-ui/ui
Length of output: 1957
avoidCollision is only partially respected—shift still runs when avoidCollision={false}.
At lines 67–74, the Floater.flip() middleware respects the avoidCollision flag, but Floater.shift() is always applied with padding: 4, allowing collision-driven repositioning even when avoidCollision={false}. This violates the contract that avoidCollision={false} should prevent deterministic placement changes. Test coverage (line 78–93) only verifies flip behavior, not shift behavior with avoidCollision={false}.
Suggested fix (gate collision middlewares together)
- Floater.flip({
- mainAxis: avoidCollision,
- crossAxis: avoidCollision && !isNested
- }),
- Floater.shift({
- padding: 4,
- crossAxis: !isNested
- }),
+ ...(avoidCollision
+ ? [
+ Floater.flip({
+ mainAxis: true,
+ crossAxis: !isNested
+ }),
+ Floater.shift({
+ padding: 4,
+ crossAxis: !isNested
+ })
+ ]
+ : []),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/primitives/Menu/fragments/MenuPrimitiveRoot.tsx` around lines 67 -
74, The Floater.shift middleware is currently always applied even when
avoidCollision is false; update the middleware list in MenuPrimitiveRoot so
collision-driven middlewares are gated by avoidCollision (like Floater.flip is)
— e.g., only include Floater.shift(...) when avoidCollision is true (respecting
isNested for crossAxis as before), ensuring both Floater.flip and Floater.shift
are conditionally added based on the avoidCollision flag.
| <MenuPrimitive.Content className="flex flex-col mt-2 bg-[var(--rad-ui-surface-inverse)] border border-[var(--rad-ui-border-soft)] rounded shadow min-w-[160px]"> | ||
| <MenuPrimitive.Item className="px-4 py-2 hover:bg-[var(--rad-ui-surface-subtle)] cursor-pointer rounded" label="Nested item 1">Nested item 1</MenuPrimitive.Item> | ||
| <MenuPrimitive.Item className="px-4 py-2 hover:bg-[var(--rad-ui-surface-subtle)] cursor-pointer rounded" label="Nested item 2">Nested item 2</MenuPrimitive.Item> | ||
| <MenuPrimitive.Item className="px-4 py-2 hover:bg-[var(--rad-ui-surface-subtle)] cursor-pointer rounded" label="item 3">item 1</MenuPrimitive.Item> |
Contributor
There was a problem hiding this comment.
Fix inconsistent item label text in nested menu story.
At Line [28], label="item 3" but rendered content is item 1. This makes the story harder to reason about.
Suggested fix
- <MenuPrimitive.Item className="px-4 py-2 hover:bg-[var(--rad-ui-surface-subtle)] cursor-pointer rounded" label="item 3">item 1</MenuPrimitive.Item>
+ <MenuPrimitive.Item className="px-4 py-2 hover:bg-[var(--rad-ui-surface-subtle)] cursor-pointer rounded" label="item 3">item 3</MenuPrimitive.Item>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <MenuPrimitive.Item className="px-4 py-2 hover:bg-[var(--rad-ui-surface-subtle)] cursor-pointer rounded" label="item 3">item 1</MenuPrimitive.Item> | |
| <MenuPrimitive.Item className="px-4 py-2 hover:bg-[var(--rad-ui-surface-subtle)] cursor-pointer rounded" label="item 3">item 3</MenuPrimitive.Item> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/primitives/Menu/stories/MenuPrimitive.stories.tsx` at line 28, The
MenuPrimitive.Item in the story has mismatched label and rendered text: the
element MenuPrimitive.Item uses label="item 3" but its children render "item 1";
update the children text to match the label (or vice versa) so label and visible
content are consistent (e.g., change the inner text to "item 3") in the
MenuPrimitive.Item entry in the Menu story.
Comment on lines
+74
to
+75
| --rad-ui-text-secondary: var(--rad-ui-color-gray-950); | ||
| --rad-ui-text-muted: var(--rad-ui-color-gray-950); |
Contributor
There was a problem hiding this comment.
Keep text-muted visually below text-secondary.
--rad-ui-text-secondary and --rad-ui-text-muted now resolve to the same gray step, which collapses the emphasis scale. Placeholders, disabled text, and helper copy will read almost as strongly as normal supporting content.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/design-systems/clarity/default.scss` around lines 74 - 75, The two
variables --rad-ui-text-secondary and --rad-ui-text-muted currently resolve to
the same gray (--rad-ui-color-gray-950), collapsing emphasis; change
--rad-ui-text-muted to a lighter/less-contrast gray step than
--rad-ui-text-secondary (e.g., point --rad-ui-text-muted at a lighter token such
as --rad-ui-color-gray-800 or similar) so placeholders/disabled/helper copy
render visually below supporting content.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/primitives/Combobox/fragments/ComboboxPrimitiveRoot.tsx (1)
226-247:⚠️ Potential issue | 🟠 MajorDon't eagerly mutate selection state ahead of a controlled
value.
handleSelectnow updatesselectedIndex/selectedLabelbefore the parent has accepted the newvalue. If the parent keepsvalueunchanged, the trigger/listbox can still move to the clicked item, so the UI drifts away from the actual controlled state. Let the[selectedValue]effect remain the single source of truth for index/label sync.Suggested fix
(index: number | null) => { if (index === null || totalDisabledIndices.includes(index)) return; - setSelectedIndex(index); setIsOpen(false); (refs.reference.current as HTMLElement | null)?.focus(); if (index !== null) { const item = elementsRef.current[index]; const value = item?.getAttribute('data-value') || valuesRef.current[index]; const label = item?.getAttribute('data-label') || displayLabelsRef.current[index] || labelsRef.current[index]; if (value) { setSelectedValue(value); if (label) { displayLabelsByValueRef.current[value] = label; } } - if (label) { - setSelectedLabel(label); - } } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/primitives/Combobox/fragments/ComboboxPrimitiveRoot.tsx` around lines 226 - 247, handleSelect is eagerly mutating selectedIndex/selectedLabel which can drift when the parent rejects a controlled value; instead only set the candidate value and close/focus, and let the existing effect that watches selectedValue perform index/label sync. Concretely, inside handleSelect remove calls to setSelectedIndex(...) and setSelectedLabel(...), and avoid writing displayLabelsByValueRef.current[...] there; keep setSelectedValue(value), setIsOpen(false) and (refs.reference.current as HTMLElement | null)?.focus(), and rely on the effect that reads selectedValue to update selectedIndex/selectedLabel from elementsRef/valuesRef/displayLabelsRef.
♻️ Duplicate comments (1)
src/core/primitives/Menu/fragments/MenuPrimitiveRoot.tsx (1)
93-94:⚠️ Potential issue | 🟠 MajorGate
shift()behindavoidCollisionas well.Line 94 still installs
Floater.shift()unconditionally, soavoidCollision={false}can keep moving the menu to avoid overflow even when flipping is disabled. That leaves the original behavior contract partially broken.Suggested fix
- Floater.flip(flipOptions), - Floater.shift(shiftOptions), + ...(avoidCollision + ? [Floater.flip(flipOptions), Floater.shift(shiftOptions)] + : []),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/primitives/Menu/fragments/MenuPrimitiveRoot.tsx` around lines 93 - 94, The Floater.shift(shiftOptions) call is applied unconditionally; update the installation logic in MenuPrimitiveRoot so that Floater.shift(shiftOptions) is only included when avoidCollision is truthy (same gating as Floater.flip(flipOptions)), i.e. conditionally push/apply Floater.shift based on the avoidCollision prop/variable to prevent shifting when collision avoidance is disabled.
🧹 Nitpick comments (9)
src/components/ui/Accordion/tests/Accordion.test.tsx (1)
104-106: Make stylesheet assertions less brittle.These exact string checks can fail on harmless whitespace/formatting changes. Prefer regex-based assertions for token presence/absence.
♻️ Suggested update
- expect(stylesheet).toContain('box-shadow: var(--rad-ui-focus-ring-shadow-offset);'); - expect(stylesheet).not.toContain('outline: var(--rad-ui-focus-ring-width'); - expect(stylesheet).not.toContain('outline-offset: var(--rad-ui-focus-ring-offset'); + expect(stylesheet).toMatch(/box-shadow:\s*var\(--rad-ui-focus-ring-shadow-offset\)\s*;/); + expect(stylesheet).not.toMatch(/outline:\s*var\(--rad-ui-focus-ring-width/); + expect(stylesheet).not.toMatch(/outline-offset:\s*var\(--rad-ui-focus-ring-offset/);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Accordion/tests/Accordion.test.tsx` around lines 104 - 106, The current assertions in Accordion.test.tsx use exact string containment checks on the stylesheet variable which are brittle; update the three expects that target 'box-shadow: var(--rad-ui-focus-ring-shadow-offset);', 'outline: var(--rad-ui-focus-ring-width', and 'outline-offset: var(--rad-ui-focus-ring-offset' to use regex-based assertions instead (e.g., expect(stylesheet).toMatch(/box-shadow\s*:\s*var\(--rad-ui-focus-ring-shadow-offset\)/) and expect(...).not.toMatch(/outline\s*:\s*var\(--rad-ui-focus-ring-width\)/), expect(...).not.toMatch(/outline-offset\s*:\s*var\(--rad-ui-focus-ring-offset\)/)) so they tolerate harmless whitespace/formatting changes while still verifying token presence/absence; update the three expect lines that reference the stylesheet variable accordingly.src/core/hooks/createDataAttribute/createDataAttribute.test.tsx (1)
15-21: Good migration coverage; consider one extra kebab-case edge test.Optional: add a case like
URLValueto pin expected output (data-url-value) and prevent regressions intoKebabCase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/hooks/createDataAttribute/createDataAttribute.test.tsx` around lines 15 - 21, Add a test case to cover an all-caps/initialism kebab-case edge: call composeAttributes (which uses toKebabCase) with a key like "URLValue" and assert the output contains "data-url-value": 'some' or specific value to pin behavior; this ensures toKebabCase handles initialisms correctly and prevents regressions—update createDataAttribute.test.tsx to include a new expect(composeAttributes(...)) assertion for the "URLValue" -> "data-url-value" mapping.src/core/hooks/createDataAttribute/index.ts (1)
1-3:_prefixis now dead but still part of the public API.Keeping an ignored parameter is confusing for callers and future maintenance. Consider removing it (or formally deprecating it with a compatibility shim) so the function contract matches runtime behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/hooks/createDataAttribute/index.ts` around lines 1 - 3, The exported function createDataAttributes currently accepts a useless _prefix parameter; remove _prefix from the public signature and implementation so the function contract matches runtime behavior (update the exported function signature and any internal references in createDataAttributes), and update all callers to stop passing the unused argument; alternatively, if backward compatibility is required, keep _prefix but add a clear deprecation shim inside createDataAttributes that logs a deprecation warning and ignores the value, then mark the parameter deprecated in the public API.knowledge/design_system/clarity_design_system.md (1)
71-71: Consider deduplicating the repeated token-rule statement.
The same “do not introduce1through12public tokens” rule appears in two places; keeping one canonical mention (and cross-referencing it) reduces future drift.Also applies to: 113-113
🤖 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 71, The document repeats the rule "Do not introduce `1` through `12` public tokens" in two places; keep one canonical occurrence (e.g., under the section that defines public token ranges such as "Token naming rules" or "Public token guidelines") and remove the duplicate, replacing the second instance with a short cross-reference or link back to the canonical mention; ensure any in-text references or examples that relied on the removed duplicate still point to the canonical rule and update the section heading or anchor if needed so cross-references remain valid.src/components/tools/SandboxEditor/SandboxEditor.tsx (1)
181-203: Reduce duplicated selected-accent UI markup.The same JSX is defined twice (in
renderValueand as trigger children), which is easy to drift over time. Extract it once and reuse.♻️ Suggested refactor
+ const renderSelectedAccent = () => ( + <span className='flex items-center gap-2'> + <ColorSelect + color={colors[colorName]} + isDarkMode={isDarkMode} + isSelected={true} + /> + <span className='capitalize'>{colorName}</span> + </span> + ); + return <div data-rad-ui-design-system={designSystem}> @@ - <Select.Root value={colorName} onValueChange={updateAccentColor}> + <Select.Root value={colorName} onValueChange={updateAccentColor}> <Select.Trigger aria-label='Accent color' - renderValue={() => ( - <span className='flex items-center gap-2'> - <ColorSelect - color={colors[colorName]} - isDarkMode={isDarkMode} - isSelected={true} - /> - <span className='capitalize'>{colorName}</span> - </span> - )} + renderValue={renderSelectedAccent} > - <span className='flex items-center gap-2'> - <ColorSelect - color={colors[colorName]} - isDarkMode={isDarkMode} - isSelected={true} - /> - <span className='capitalize'>{colorName}</span> - </span> + {renderSelectedAccent()} </Select.Trigger>🤖 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 181 - 203, The JSX for the selected-accent display is duplicated between Select.Trigger children and the renderValue prop; extract that markup into a single reusable constant or small component (e.g., SelectedAccentDisplay) and use it in both places. Locate the Select.Root/Select.Trigger usage that references renderValue and ColorSelect with props color={colors[colorName]}, isDarkMode, isSelected and replace both instances with the shared SelectedAccentDisplay (passing colorName/colors/isDarkMode as props or closing over them) so updateAccentColor remains the onValueChange handler and behavior is unchanged.src/components/ui/Select/tests/Select.full.test.tsx (1)
145-188: Consider usingafterEachortry/finallyfor DOM cleanup.The
portalRoot.remove()call at line 187 won't execute if an assertion fails earlier, potentially leaving stale DOM elements that could affect subsequent tests.♻️ Suggested cleanup pattern
test('custom trigger value rendering still opens controlled select', async() => { const portalRoot = document.createElement('div'); portalRoot.setAttribute('data-rad-ui-portal-root', ''); document.body.appendChild(portalRoot); + try { function Controlled() { // ... component code } render(<Controlled />); const trigger = screen.getByRole('combobox', { name: 'Accent color' }); expect(screen.getByTestId('custom-value')).toHaveTextContent('swatch amber'); await userEvent.click(trigger); expect(screen.getByRole('listbox')).toBeInTheDocument(); expect(portalRoot.querySelector('[role="listbox"]')).toBeTruthy(); await userEvent.click(screen.getByText('Blue')); expect(screen.getByTestId('custom-value')).toHaveTextContent('swatch blue'); + } finally { portalRoot.remove(); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Select/tests/Select.full.test.tsx` around lines 145 - 188, The test creates a portalRoot element and calls portalRoot.remove() at the end, but that removal won't run if an assertion throws; update the test to guarantee cleanup by either moving portalRoot creation to a beforeEach and removing it in an afterEach cleanup, or wrap the render/click/assert sequence in a try/finally and call portalRoot.remove() in finally. Target the portalRoot variable and the Controlled component/Select usage in the 'custom trigger value rendering still opens controlled select' test so the portal DOM is always removed even on failures.src/core/primitives/Combobox/contexts/ComboboxPrimitiveContext.tsx (1)
15-15: Consider typingmiddlewareDatawithMiddlewareDatafrom@floating-ui/react.Using
anyloses type safety. The@floating-ui/reactpackage exports aMiddlewareDatatype that provides proper typing for middleware results.+import type { MiddlewareData } from '@floating-ui/react'; + export type ComboboxPrimitiveContextType = { // ... - middlewareData: any; + middlewareData: MiddlewareData; // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/primitives/Combobox/contexts/ComboboxPrimitiveContext.tsx` at line 15, Replace the untyped middlewareData: any in the Combobox primitive context with the proper MiddlewareData type from `@floating-ui/react`: import MiddlewareData and use it for the middlewareData property (e.g., change middlewareData: any to middlewareData: MiddlewareData in the ComboboxPrimitiveContext / ComboboxPrimitiveContextValue type or interface) so middleware results are strongly typed; add the import for MiddlewareData from '@floating-ui/react' and adjust any related usages to satisfy the new type.src/core/primitives/Combobox/fragments/ComboboxPrimitiveTrigger.tsx (1)
29-32: Edge case:selectedValuecould be empty string whileselectedLabelexists.When
selectedValueis an empty string butselectedLabelhas a value,hasSelectedValueevaluates totrue(correct), butrenderValuereceives an empty string as the first argument. Consumers might expect the value to be truthy whenhasSelectedValueis true.Consider whether this is intentional, or if the condition should also check that
selectedValueis truthy before invokingrenderValue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/primitives/Combobox/fragments/ComboboxPrimitiveTrigger.tsx` around lines 29 - 32, The current logic sets hasSelectedValue based on selectedLabel || selectedValue but still passes selectedValue as the first argument to renderValue, which can be an empty string; update the render invocation in the ComboboxPrimitiveTrigger fragment so the first argument is a truthy display value when a label exists (e.g., use selectedValue unless it is an empty string, otherwise use selectedLabel), keeping the second arg semantics (selectedLabel || selectedValue) intact; adjust the renderedValue computation and any related hasSelectedValue check to use this explicit empty-string guard to avoid passing empty-string as the primary value to renderValue.src/components/ui/Select/select.clarity.scss (1)
17-24: Move the shared size tokens to.rad-ui-select-root.Defining
--rad-ui-select-control-*on each child shadows any value provided by the root/theme, so consumers can't override the Select's height/padding/radius once for the whole component. Put these defaults on.rad-ui-select-rootand let the trigger/content/item/label only consume them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Select/select.clarity.scss` around lines 17 - 24, Move the shared control size CSS custom properties (--rad-ui-select-control-height, --rad-ui-select-control-padding-x, --rad-ui-select-control-radius) from the child selectors (.rad-ui-select-trigger, .rad-ui-select-content, .rad-ui-select-item, .rad-ui-select-label) to the root selector .rad-ui-select-root so the root provides the defaults and children simply consume them; remove the duplicate declarations from the child selectors so consumers can override those tokens at the root/theme level and the trigger/content/item/label only reference the 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 `@src/components/ui/Button/tests/Button.test.tsx`:
- Around line 29-33: The test "renders button with the given variant" currently
only checks the base root class and can pass if the variant prop is ignored;
update the test that renders <Button customRootClass="rad-ui"
variant='outline'>button</Button> to also assert the rendered element's variant
is applied by checking the data attribute (use the same retrieved element via
screen.getByText('button') and add an assertion like
expect(button).toHaveAttribute('data-variant', 'outline')), referencing the
Button component and the test name to locate the change.
In `@src/core/hooks/createDataAttribute/index.ts`:
- Around line 14-16: The toKebabCase function currently only splits
lower-to-upper boundaries and fails on acronym-to-word transitions (e.g.,
URLValue -> url-value); update the implementation of toKebabCase to handle
uppercase acronym boundaries by adding a second replacement that inserts a
hyphen between an uppercase sequence and a following capital+lowercase start
(for example add a replace like .replace(/([A-Z])([A-Z][a-z])/g, '$1-$2') before
the final .toLowerCase()); modify the function named toKebabCase accordingly so
keys like URLValue normalize to url-value.
In `@src/core/primitives/Combobox/fragments/ComboboxPrimitiveRoot.tsx`:
- Around line 149-160: constrainSize middleware (Floater.size with
detectOverflowOptions -> apply) sets floatingStyle.width when
matchReferenceWidth is true but never clears it if matchReferenceWidth becomes
false; update the apply callback in ComboboxPrimitiveRoot.tsx to remove or reset
the inline width when matchReferenceWidth is falsy (e.g., call
floatingStyle.removeProperty('width') or set floatingStyle.width = '' in the
else branch) so the popup can resume natural sizing when the prop flips, keeping
the other property updates intact.
In `@src/core/primitives/Menu/fragments/MenuPrimitiveRoot.tsx`:
- Around line 65-69: flipOptions currently only adds padding when
collisionPadding !== 4, causing flip() to use zero padding by default while
shift() and size() use 4; update the flipOptions construction in
MenuPrimitiveRoot so flip() always receives the collisionPadding value (i.e.,
include padding: collisionPadding unconditionally or ensure collisionPadding
defaults to 4 before building flipOptions) so flip(), shift(), and size() behave
consistently.
---
Outside diff comments:
In `@src/core/primitives/Combobox/fragments/ComboboxPrimitiveRoot.tsx`:
- Around line 226-247: handleSelect is eagerly mutating
selectedIndex/selectedLabel which can drift when the parent rejects a controlled
value; instead only set the candidate value and close/focus, and let the
existing effect that watches selectedValue perform index/label sync. Concretely,
inside handleSelect remove calls to setSelectedIndex(...) and
setSelectedLabel(...), and avoid writing displayLabelsByValueRef.current[...]
there; keep setSelectedValue(value), setIsOpen(false) and
(refs.reference.current as HTMLElement | null)?.focus(), and rely on the effect
that reads selectedValue to update selectedIndex/selectedLabel from
elementsRef/valuesRef/displayLabelsRef.
---
Duplicate comments:
In `@src/core/primitives/Menu/fragments/MenuPrimitiveRoot.tsx`:
- Around line 93-94: The Floater.shift(shiftOptions) call is applied
unconditionally; update the installation logic in MenuPrimitiveRoot so that
Floater.shift(shiftOptions) is only included when avoidCollision is truthy (same
gating as Floater.flip(flipOptions)), i.e. conditionally push/apply
Floater.shift based on the avoidCollision prop/variable to prevent shifting when
collision avoidance is disabled.
---
Nitpick comments:
In `@knowledge/design_system/clarity_design_system.md`:
- Line 71: The document repeats the rule "Do not introduce `1` through `12`
public tokens" in two places; keep one canonical occurrence (e.g., under the
section that defines public token ranges such as "Token naming rules" or "Public
token guidelines") and remove the duplicate, replacing the second instance with
a short cross-reference or link back to the canonical mention; ensure any
in-text references or examples that relied on the removed duplicate still point
to the canonical rule and update the section heading or anchor if needed so
cross-references remain valid.
In `@src/components/tools/SandboxEditor/SandboxEditor.tsx`:
- Around line 181-203: The JSX for the selected-accent display is duplicated
between Select.Trigger children and the renderValue prop; extract that markup
into a single reusable constant or small component (e.g., SelectedAccentDisplay)
and use it in both places. Locate the Select.Root/Select.Trigger usage that
references renderValue and ColorSelect with props color={colors[colorName]},
isDarkMode, isSelected and replace both instances with the shared
SelectedAccentDisplay (passing colorName/colors/isDarkMode as props or closing
over them) so updateAccentColor remains the onValueChange handler and behavior
is unchanged.
In `@src/components/ui/Accordion/tests/Accordion.test.tsx`:
- Around line 104-106: The current assertions in Accordion.test.tsx use exact
string containment checks on the stylesheet variable which are brittle; update
the three expects that target 'box-shadow:
var(--rad-ui-focus-ring-shadow-offset);', 'outline:
var(--rad-ui-focus-ring-width', and 'outline-offset:
var(--rad-ui-focus-ring-offset' to use regex-based assertions instead (e.g.,
expect(stylesheet).toMatch(/box-shadow\s*:\s*var\(--rad-ui-focus-ring-shadow-offset\)/)
and expect(...).not.toMatch(/outline\s*:\s*var\(--rad-ui-focus-ring-width\)/),
expect(...).not.toMatch(/outline-offset\s*:\s*var\(--rad-ui-focus-ring-offset\)/))
so they tolerate harmless whitespace/formatting changes while still verifying
token presence/absence; update the three expect lines that reference the
stylesheet variable accordingly.
In `@src/components/ui/Select/select.clarity.scss`:
- Around line 17-24: Move the shared control size CSS custom properties
(--rad-ui-select-control-height, --rad-ui-select-control-padding-x,
--rad-ui-select-control-radius) from the child selectors
(.rad-ui-select-trigger, .rad-ui-select-content, .rad-ui-select-item,
.rad-ui-select-label) to the root selector .rad-ui-select-root so the root
provides the defaults and children simply consume them; remove the duplicate
declarations from the child selectors so consumers can override those tokens at
the root/theme level and the trigger/content/item/label only reference the
variables.
In `@src/components/ui/Select/tests/Select.full.test.tsx`:
- Around line 145-188: The test creates a portalRoot element and calls
portalRoot.remove() at the end, but that removal won't run if an assertion
throws; update the test to guarantee cleanup by either moving portalRoot
creation to a beforeEach and removing it in an afterEach cleanup, or wrap the
render/click/assert sequence in a try/finally and call portalRoot.remove() in
finally. Target the portalRoot variable and the Controlled component/Select
usage in the 'custom trigger value rendering still opens controlled select' test
so the portal DOM is always removed even on failures.
In `@src/core/hooks/createDataAttribute/createDataAttribute.test.tsx`:
- Around line 15-21: Add a test case to cover an all-caps/initialism kebab-case
edge: call composeAttributes (which uses toKebabCase) with a key like "URLValue"
and assert the output contains "data-url-value": 'some' or specific value to pin
behavior; this ensures toKebabCase handles initialisms correctly and prevents
regressions—update createDataAttribute.test.tsx to include a new
expect(composeAttributes(...)) assertion for the "URLValue" -> "data-url-value"
mapping.
In `@src/core/hooks/createDataAttribute/index.ts`:
- Around line 1-3: The exported function createDataAttributes currently accepts
a useless _prefix parameter; remove _prefix from the public signature and
implementation so the function contract matches runtime behavior (update the
exported function signature and any internal references in
createDataAttributes), and update all callers to stop passing the unused
argument; alternatively, if backward compatibility is required, keep _prefix but
add a clear deprecation shim inside createDataAttributes that logs a deprecation
warning and ignores the value, then mark the parameter deprecated in the public
API.
In `@src/core/primitives/Combobox/contexts/ComboboxPrimitiveContext.tsx`:
- Line 15: Replace the untyped middlewareData: any in the Combobox primitive
context with the proper MiddlewareData type from `@floating-ui/react`: import
MiddlewareData and use it for the middlewareData property (e.g., change
middlewareData: any to middlewareData: MiddlewareData in the
ComboboxPrimitiveContext / ComboboxPrimitiveContextValue type or interface) so
middleware results are strongly typed; add the import for MiddlewareData from
'@floating-ui/react' and adjust any related usages to satisfy the new type.
In `@src/core/primitives/Combobox/fragments/ComboboxPrimitiveTrigger.tsx`:
- Around line 29-32: The current logic sets hasSelectedValue based on
selectedLabel || selectedValue but still passes selectedValue as the first
argument to renderValue, which can be an empty string; update the render
invocation in the ComboboxPrimitiveTrigger fragment so the first argument is a
truthy display value when a label exists (e.g., use selectedValue unless it is
an empty string, otherwise use selectedLabel), keeping the second arg semantics
(selectedLabel || selectedValue) intact; adjust the renderedValue computation
and any related hasSelectedValue check to use this explicit empty-string guard
to avoid passing empty-string as the primary value to renderValue.
🪄 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: 5184926e-66a3-4719-8ba7-f474bd153328
⛔ Files ignored due to path filters (1)
src/components/ui/TextArea/__snapshots__/TextArea.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (113)
docs/app/docs/components/badge/docs/codeUsage.jsdocs/app/docs/components/button/docs/codeUsage.jsdocs/app/docs/components/card/docs/codeUsage.jsdocs/app/docs/components/code/docs/codeUsage.jsdocs/app/docs/components/em/docs/codeUsage.jsdocs/app/docs/components/heading/docs/codeUsage.jsdocs/app/docs/components/kbd/docs/codeUsage.jsdocs/app/docs/components/separator/docs/codeUsage.jsdocs/app/docs/components/strong/docs/codeUsage.jsdocs/app/docs/components/text/docs/codeUsage.jsdocs/app/docs/components/toggle/docs/codeUsage.jsknowledge/MASTER_LIST.mdknowledge/design_system/clarity_audit_spec.mdknowledge/design_system/clarity_design_system.mdknowledge/features/data-attributes.mdknowledge/features/styling-namespaces.mdpackage.jsonsrc/components/tools/SandboxEditor/SandboxEditor.tsxsrc/components/ui/Accordion/accordion.clarity.scsssrc/components/ui/Accordion/fragments/AccordionRoot.tsxsrc/components/ui/Accordion/tests/Accordion.test.tsxsrc/components/ui/AlertDialog/alert-dialog.clarity.scsssrc/components/ui/Avatar/_avatar-base.clarity.scsssrc/components/ui/Avatar/tests/Avatar.test.tsxsrc/components/ui/AvatarGroup/avatar-group.clarity.scsssrc/components/ui/AvatarGroup/tests/AvatarGroup.test.jssrc/components/ui/Badge/Badge.tsxsrc/components/ui/Badge/badge.baremetal.scsssrc/components/ui/Badge/badge.clarity.scsssrc/components/ui/Badge/tests/Badge.test.tsxsrc/components/ui/BlockQuote/BlockQuote.tsxsrc/components/ui/BlockQuote/blockquote.clarity.scsssrc/components/ui/BlockQuote/tests/BlockQuote.test.tsxsrc/components/ui/Button/Button.tsxsrc/components/ui/Button/button.baremetal.scsssrc/components/ui/Button/button.clarity.scsssrc/components/ui/Button/tests/Button.asChild.test.tsxsrc/components/ui/Button/tests/Button.test.tsxsrc/components/ui/Callout/callout.clarity.scsssrc/components/ui/Callout/tests/Callout.test.tsxsrc/components/ui/Card/card.baremetal.scsssrc/components/ui/Card/card.clarity.scsssrc/components/ui/Card/fragments/CardRoot.tsxsrc/components/ui/Card/tests/Card.test.tsxsrc/components/ui/Code/code.clarity.scsssrc/components/ui/Code/stories/Code.stories.tsxsrc/components/ui/Code/tests/Code.test.tsxsrc/components/ui/Combobox/combobox.clarity.scsssrc/components/ui/Combobox/fragments/ComboboxRoot.tsxsrc/components/ui/Combobox/stories/Combobox.stories.tsxsrc/components/ui/Command/command.clarity.scsssrc/components/ui/ContextMenu/context-menu.clarity.scsssrc/components/ui/ContextMenu/stories/ContextMenu.stories.tsxsrc/components/ui/DataList/data-list.clarity.scsssrc/components/ui/Dialog/dialog.clarity.scsssrc/components/ui/Drawer/drawer.clarity.scsssrc/components/ui/DropdownMenu/stories/DropdownMenu.stories.tsxsrc/components/ui/Em/em.clarity.scsssrc/components/ui/HoverCard/fragments/HoverCardRoot.tsxsrc/components/ui/HoverCard/hover-card.clarity.scsssrc/components/ui/HoverCard/stories/HoverCard.stories.tsxsrc/components/ui/Kbd/kbd.clarity.scsssrc/components/ui/Link/link.clarity.scsssrc/components/ui/Link/tests/Link.test.jssrc/components/ui/Menubar/stories/Menubar.stories.tsxsrc/components/ui/NumberField/number-field.clarity.scsssrc/components/ui/Progress/tests/Progress.test.tsxsrc/components/ui/Radio/tests/Radio.test.tsxsrc/components/ui/RadioGroup/tests/RadioGroup.test.tsxsrc/components/ui/ScrollArea/fragments/ScrollAreaCorner.tsxsrc/components/ui/ScrollArea/fragments/ScrollAreaScrollbar.tsxsrc/components/ui/ScrollArea/fragments/ScrollAreaThumb.tsxsrc/components/ui/ScrollArea/fragments/ScrollAreaViewport.tsxsrc/components/ui/ScrollArea/tests/ScrollArea.test.tsxsrc/components/ui/Select/fragments/SelectItem.tsxsrc/components/ui/Select/fragments/SelectPortal.tsxsrc/components/ui/Select/fragments/SelectRoot.tsxsrc/components/ui/Select/select.clarity.scsssrc/components/ui/Select/stories/Select.stories.tsxsrc/components/ui/Select/tests/Select.full.test.tsxsrc/components/ui/Separator/Separator.tsxsrc/components/ui/Separator/tests/Separator.test.tsxsrc/components/ui/Slider/slider.clarity.scsssrc/components/ui/Spinner/spinner.clarity.scsssrc/components/ui/Tabs/fragments/TabRoot.tsxsrc/components/ui/Tabs/tabs.clarity.scsssrc/components/ui/TextArea/textarea.clarity.scsssrc/components/ui/TextField/text-field.clarity.scsssrc/components/ui/Theme/useComponentClass.tssrc/components/ui/Toggle/Toggle.tsxsrc/components/ui/Toggle/tests/Toggle.test.jssrc/components/ui/ToggleGroup/fragments/ToggleGroupRoot.tsxsrc/components/ui/Toolbar/fragments/ToolbarToggleGroup.tsxsrc/components/ui/Toolbar/fragments/ToolbarToggleItem.tsxsrc/components/ui/Toolbar/tests/Toolbar.behavior.test.tsxsrc/components/ui/Toolbar/toolbar.clarity.scsssrc/components/ui/Tooltip/fragments/TooltipContent.tsxsrc/components/ui/Tooltip/fragments/TooltipRoot.tsxsrc/components/ui/Tooltip/stories/Tooltip.stories.tsxsrc/core/hooks/createDataAttribute/createDataAttribute.test.tsxsrc/core/hooks/createDataAttribute/index.tssrc/core/primitives/Combobox/contexts/ComboboxPrimitiveContext.tsxsrc/core/primitives/Combobox/fragments/ComboboxPrimitiveContent.tsxsrc/core/primitives/Combobox/fragments/ComboboxPrimitiveItem.tsxsrc/core/primitives/Combobox/fragments/ComboboxPrimitiveRoot.tsxsrc/core/primitives/Combobox/fragments/ComboboxPrimitiveTrigger.tsxsrc/core/primitives/Combobox/stories/Combobox.stories.tsxsrc/core/primitives/Floater/index.tsxsrc/core/primitives/Menu/fragments/MenuPrimitiveContent.tsxsrc/core/primitives/Menu/fragments/MenuPrimitiveRoot.tsxsrc/design-systems/clarity/default.scsssrc/tokenGen/transformers/generateAccentTokens.jsstyles/cssTokens/base.tokens.css
💤 Files with no reviewable changes (1)
- src/core/primitives/Menu/fragments/MenuPrimitiveContent.tsx
✅ Files skipped from review due to trivial changes (19)
- src/components/ui/Em/em.clarity.scss
- src/components/ui/Card/tests/Card.test.tsx
- src/components/ui/AvatarGroup/tests/AvatarGroup.test.js
- src/components/ui/Progress/tests/Progress.test.tsx
- src/components/ui/Link/tests/Link.test.js
- knowledge/features/styling-namespaces.md
- knowledge/features/data-attributes.md
- src/components/ui/Button/button.baremetal.scss
- src/components/ui/Toolbar/fragments/ToolbarToggleItem.tsx
- src/components/ui/Badge/badge.baremetal.scss
- src/core/primitives/Floater/index.tsx
- src/components/ui/Button/tests/Button.asChild.test.tsx
- src/components/ui/Accordion/accordion.clarity.scss
- src/components/ui/Radio/tests/Radio.test.tsx
- src/components/ui/Toggle/tests/Toggle.test.js
- src/components/ui/RadioGroup/tests/RadioGroup.test.tsx
- src/components/ui/Separator/tests/Separator.test.tsx
- src/components/ui/TextField/text-field.clarity.scss
- src/components/ui/NumberField/number-field.clarity.scss
🚧 Files skipped from review as they are similar to previous changes (13)
- src/components/ui/HoverCard/hover-card.clarity.scss
- src/components/ui/Toolbar/toolbar.clarity.scss
- src/components/ui/HoverCard/stories/HoverCard.stories.tsx
- src/components/ui/Link/link.clarity.scss
- src/components/ui/HoverCard/fragments/HoverCardRoot.tsx
- src/core/primitives/Combobox/stories/Combobox.stories.tsx
- src/components/ui/Tabs/tabs.clarity.scss
- src/components/ui/Command/command.clarity.scss
- src/components/ui/Combobox/combobox.clarity.scss
- src/components/ui/Button/button.clarity.scss
- src/components/ui/ContextMenu/context-menu.clarity.scss
- src/components/ui/TextArea/textarea.clarity.scss
- src/components/ui/Slider/slider.clarity.scss
Comment on lines
29
to
+33
| test('renders button with the given variant', () => { | ||
| render(<Button customRootClass="rad-ui" variant='outline'>button</Button>); | ||
| const button = screen.getByText('button'); | ||
| expect(button).toHaveClass('rad-ui-button'); | ||
| expect(button).toHaveClass('rad-ui-button-root'); | ||
| }); |
Contributor
There was a problem hiding this comment.
Variant test no longer validates the variant prop.
At Line 32, the assertion only checks the base root class, which can pass even if variant='outline' is ignored. Add an assertion for the variant data attribute.
Suggested fix
test('renders button with the given variant', () => {
render(<Button customRootClass="rad-ui" variant='outline'>button</Button>);
const button = screen.getByText('button');
expect(button).toHaveClass('rad-ui-button-root');
+ expect(button).toHaveAttribute('data-variant', 'outline');
});📝 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
| test('renders button with the given variant', () => { | |
| render(<Button customRootClass="rad-ui" variant='outline'>button</Button>); | |
| const button = screen.getByText('button'); | |
| expect(button).toHaveClass('rad-ui-button'); | |
| expect(button).toHaveClass('rad-ui-button-root'); | |
| }); | |
| test('renders button with the given variant', () => { | |
| render(<Button customRootClass="rad-ui" variant='outline'>button</Button>); | |
| const button = screen.getByText('button'); | |
| expect(button).toHaveClass('rad-ui-button-root'); | |
| expect(button).toHaveAttribute('data-variant', 'outline'); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/Button/tests/Button.test.tsx` around lines 29 - 33, The
test "renders button with the given variant" currently only checks the base root
class and can pass if the variant prop is ignored; update the test that renders
<Button customRootClass="rad-ui" variant='outline'>button</Button> to also
assert the rendered element's variant is applied by checking the data attribute
(use the same retrieved element via screen.getByText('button') and add an
assertion like expect(button).toHaveAttribute('data-variant', 'outline')),
referencing the Button component and the test name to locate the change.
Comment on lines
+14
to
+16
| const toKebabCase = (value: string) => { | ||
| return value.replace(/([a-z0-9])([A-Z])/g, '$1-$2').toLowerCase(); | ||
| }; |
Contributor
There was a problem hiding this comment.
toKebabCase misses acronym transitions (edge case).
Keys like URLValue currently normalize to urlvalue instead of url-value, which can break expected selector names.
Proposed fix
const toKebabCase = (value: string) => {
- return value.replace(/([a-z0-9])([A-Z])/g, '$1-$2').toLowerCase();
+ return value
+ .replace(/([A-Z]+)([A-Z][a-z])/g, '$1-$2')
+ .replace(/([a-z0-9])([A-Z])/g, '$1-$2')
+ .toLowerCase();
};📝 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
| const toKebabCase = (value: string) => { | |
| return value.replace(/([a-z0-9])([A-Z])/g, '$1-$2').toLowerCase(); | |
| }; | |
| const toKebabCase = (value: string) => { | |
| return value | |
| .replace(/([A-Z]+)([A-Z][a-z])/g, '$1-$2') | |
| .replace(/([a-z0-9])([A-Z])/g, '$1-$2') | |
| .toLowerCase(); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/hooks/createDataAttribute/index.ts` around lines 14 - 16, The
toKebabCase function currently only splits lower-to-upper boundaries and fails
on acronym-to-word transitions (e.g., URLValue -> url-value); update the
implementation of toKebabCase to handle uppercase acronym boundaries by adding a
second replacement that inserts a hyphen between an uppercase sequence and a
following capital+lowercase start (for example add a replace like
.replace(/([A-Z])([A-Z][a-z])/g, '$1-$2') before the final .toLowerCase());
modify the function named toKebabCase accordingly so keys like URLValue
normalize to url-value.
Comment on lines
+149
to
+160
| constrainSize && Floater.size({ | ||
| ...detectOverflowOptions, | ||
| apply: ({ elements, rects, availableWidth, availableHeight }) => { | ||
| const floatingStyle = elements.floating.style; | ||
| floatingStyle.setProperty('--rad-ui-floating-available-width', `${Math.max(0, availableWidth)}px`); | ||
| floatingStyle.setProperty('--rad-ui-floating-available-height', `${Math.max(0, availableHeight)}px`); | ||
| floatingStyle.setProperty('--rad-ui-floating-reference-width', `${rects.reference.width}px`); | ||
| floatingStyle.setProperty('--rad-ui-floating-reference-height', `${rects.reference.height}px`); | ||
|
|
||
| if (matchReferenceWidth) { | ||
| floatingStyle.width = `${rects.reference.width}px`; | ||
| } |
Contributor
There was a problem hiding this comment.
Clear the forced width when matchReferenceWidth is turned off.
This middleware only sets floatingStyle.width in the truthy branch. If the prop flips from true to false, the old inline width sticks around and the popup stays pinned to the reference width.
Suggested fix
if (matchReferenceWidth) {
floatingStyle.width = `${rects.reference.width}px`;
+ } else {
+ floatingStyle.width = '';
}📝 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
| constrainSize && Floater.size({ | |
| ...detectOverflowOptions, | |
| apply: ({ elements, rects, availableWidth, availableHeight }) => { | |
| const floatingStyle = elements.floating.style; | |
| floatingStyle.setProperty('--rad-ui-floating-available-width', `${Math.max(0, availableWidth)}px`); | |
| floatingStyle.setProperty('--rad-ui-floating-available-height', `${Math.max(0, availableHeight)}px`); | |
| floatingStyle.setProperty('--rad-ui-floating-reference-width', `${rects.reference.width}px`); | |
| floatingStyle.setProperty('--rad-ui-floating-reference-height', `${rects.reference.height}px`); | |
| if (matchReferenceWidth) { | |
| floatingStyle.width = `${rects.reference.width}px`; | |
| } | |
| constrainSize && Floater.size({ | |
| ...detectOverflowOptions, | |
| apply: ({ elements, rects, availableWidth, availableHeight }) => { | |
| const floatingStyle = elements.floating.style; | |
| floatingStyle.setProperty('--rad-ui-floating-available-width', `${Math.max(0, availableWidth)}px`); | |
| floatingStyle.setProperty('--rad-ui-floating-available-height', `${Math.max(0, availableHeight)}px`); | |
| floatingStyle.setProperty('--rad-ui-floating-reference-width', `${rects.reference.width}px`); | |
| floatingStyle.setProperty('--rad-ui-floating-reference-height', `${rects.reference.height}px`); | |
| if (matchReferenceWidth) { | |
| floatingStyle.width = `${rects.reference.width}px`; | |
| } else { | |
| floatingStyle.width = ''; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/primitives/Combobox/fragments/ComboboxPrimitiveRoot.tsx` around
lines 149 - 160, constrainSize middleware (Floater.size with
detectOverflowOptions -> apply) sets floatingStyle.width when
matchReferenceWidth is true but never clears it if matchReferenceWidth becomes
false; update the apply callback in ComboboxPrimitiveRoot.tsx to remove or reset
the inline width when matchReferenceWidth is falsy (e.g., call
floatingStyle.removeProperty('width') or set floatingStyle.width = '' in the
else branch) so the popup can resume natural sizing when the prop flips, keeping
the other property updates intact.
Comment on lines
+65
to
+69
| const flipOptions = { | ||
| mainAxis: avoidCollision, | ||
| crossAxis: avoidCollision && !isNested, | ||
| ...(collisionPadding !== 4 ? { padding: collisionPadding } : {}), | ||
| ...sharedDetectOverflowOptions |
Contributor
There was a problem hiding this comment.
Pass the default collisionPadding to flip() too.
Line 68 only adds padding when it differs from 4, so the default path still flips with zero padding while shift() and size() use 4. That makes the exposed prop inconsistent in the most common case.
Suggested fix
const flipOptions = {
mainAxis: avoidCollision,
crossAxis: avoidCollision && !isNested,
- ...(collisionPadding !== 4 ? { padding: collisionPadding } : {}),
+ padding: collisionPadding,
...sharedDetectOverflowOptions
};📝 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
| const flipOptions = { | |
| mainAxis: avoidCollision, | |
| crossAxis: avoidCollision && !isNested, | |
| ...(collisionPadding !== 4 ? { padding: collisionPadding } : {}), | |
| ...sharedDetectOverflowOptions | |
| const flipOptions = { | |
| mainAxis: avoidCollision, | |
| crossAxis: avoidCollision && !isNested, | |
| padding: collisionPadding, | |
| ...sharedDetectOverflowOptions | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/primitives/Menu/fragments/MenuPrimitiveRoot.tsx` around lines 65 -
69, flipOptions currently only adds padding when collisionPadding !== 4, causing
flip() to use zero padding by default while shift() and size() use 4; update the
flipOptions construction in MenuPrimitiveRoot so flip() always receives the
collisionPadding value (i.e., include padding: collisionPadding unconditionally
or ensure collisionPadding defaults to 4 before building flipOptions) so flip(),
shift(), and size() behave consistently.
…r-comps-rulebook-apr-21 # Conflicts: # src/components/ui/Drawer/drawer.clarity.scss
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
Documentation
Tests