Kotapi/refactor unstyled first behaviiour#1895
Merged
Conversation
|
Contributor
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request refactors component class generation by introducing a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The PR is large in file count (~150+ files modified) but exhibits high homogeneity with a consistent refactoring pattern applied repeatedly: replacing Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
…led-first-behaviiour # Conflicts: # src/components/tools/SandboxEditor/SandboxEditor.tsx
![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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/components/ui/Slider/fragments/SliderRange.tsx (1)
11-12:⚠️ Potential issue | 🟠 Major
classNamecan override and remove the base range class.At Line 42 you compute the internal class, but spreading
{...props}at Line 53 allowsprops.classNameto overwrite it. This can strip${rootClass}-rangeand break styling hooks. Please merge consumer and internal classes explicitly.💡 Proposed fix
-const SliderRange = forwardRef<SliderRangeElement, SliderRangeProps>(({ children, ...props }, ref) => { +const SliderRange = forwardRef<SliderRangeElement, SliderRangeProps>(({ children, className, ...props }, ref) => { @@ - className={rootClass ? `${rootClass}-range` : undefined} + className={[rootClass && `${rootClass}-range`, className].filter(Boolean).join(' ') || undefined} @@ - {...props} + {...props}Also applies to: 42-42, 53-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Slider/fragments/SliderRange.tsx` around lines 11 - 12, The computed internal class for the range element is being overwritten by consumer props because SliderRange spreads {...props} after computing classes; update SliderRange to merge its internal class (which includes `${rootClass}-range`) with any incoming props.className (e.g., compute a mergedClass like `${internalClass} ${props.className || ''}`) and pass that mergedClass as the className prop to the rendered element instead of allowing props.className to overwrite it; ensure you reference SliderRange, SliderContext (rootClass), and props.className when making the change.src/components/ui/Combobox/fragments/ComboboxTrigger.tsx (1)
18-23:⚠️ Potential issue | 🟡 Minor
classNameoverride can drop the namespaced trigger class.Because
{...props}is spread afterclassName, a consumer-providedclassNamereplaces${rootClass}-triggerinstead of merging with it.Suggested fix
+'use client'; import React, { useContext } from 'react'; +import clsx from 'clsx'; import ComboboxPrimitive from '~/core/primitives/Combobox/ComboboxPrimitive'; @@ -const ComboboxTrigger = React.forwardRef<ComboboxTriggerElement, ComboboxTriggerProps>(({ customRootClass, children, disabled, placeholder, ...props }, forwardedRef) => { +const ComboboxTrigger = React.forwardRef<ComboboxTriggerElement, ComboboxTriggerProps>(({ customRootClass, children, disabled, placeholder, className, ...props }, forwardedRef) => { @@ - className={rootClass ? `${rootClass}-trigger` : undefined} + className={clsx(rootClass && `${rootClass}-trigger`, className)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Combobox/fragments/ComboboxTrigger.tsx` around lines 18 - 23, The component ComboboxTrigger currently spreads {...props} after setting className, so a consumer className overwrites `${rootClass}-trigger`; fix by computing a merged className (merge `${rootClass}-trigger` with props.className or the incoming className prop) and pass that single computed value to the className prop instead of relying on spread order—update the JSX in ComboboxTrigger.tsx to derive combinedClassName (or use a utility like clsx) and remove/avoid letting props.className override the namespaced `${rootClass}-trigger`.src/components/ui/Slider/fragments/SliderTrack.tsx (1)
11-26:⚠️ Potential issue | 🟠 Major
classNamecan be unintentionally overridden by prop spread.Line 17 sets the internal track class, but line 25 spreads
...propsafterward, so any passedclassNamereplaces it instead of merging. Similarly, thestyleprop at lines 18-23 can be overridden bystylein the spread, losing the orientation-based positioning. This breaks component styling and functionality.Destructure
classNameandstyleseparately fromprops, then useclsxto merge the className. This pattern is already used correctly inSliderRoot.tsxin the same directory.Suggested fix
import React, { forwardRef, ElementRef, ComponentPropsWithoutRef } from 'react'; +import clsx from 'clsx'; import { SliderContext } from '../context/SliderContext'; -const SliderTrack = forwardRef<SliderTrackElement, SliderTrackProps>(({ children, ...props }, ref) => { +const SliderTrack = forwardRef<SliderTrackElement, SliderTrackProps>(({ children, className, style, ...props }, ref) => { const { rootClass, orientation } = React.useContext(SliderContext); return ( <div ref={ref} - className={rootClass ? `${rootClass}-track` : undefined} - style={ orientation === 'vertical' ? { + className={clsx(rootClass && `${rootClass}-track`, className)} + style={{ ...(orientation === 'vertical' ? { rotate: '180deg', position: 'relative' } : { position: 'relative' - }} + }), ...style }} {...props} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Slider/fragments/SliderTrack.tsx` around lines 11 - 26, The SliderTrack component currently spreads props last which allows incoming className and style to overwrite internal values; fix this in the forwardRef SliderTrack function by destructuring className and style from props (e.g., ({ children, className, style, ...props }, ref)), compute the internal class using clsx like clsx(rootClass ? `${rootClass}-track` : undefined, className) and compute the orientation style, then merge styles so the orientation-driven style wins (finalStyle = { ...style, ...orientationStyle }) and pass finalStyle and the merged className into the div before spreading the remaining ...props; follow the pattern used in SliderRoot.tsx for merging className/style.src/components/ui/Spinner/Spinner.tsx (1)
16-27:⚠️ Potential issue | 🟠 MajorForwarded ref is not attached to the rendered span element.
React.forwardRefreceivesrefand declaresSpinnerElement = ElementRef<'span'>, but the<span>element at line 22 does not useref={ref}. Consumers will getnullwhen accessing the ref. Attach the ref to the rendered span element.🔧 Proposed fix
<span + ref={ref} className={clsx(rootClass, className)} {...props}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Spinner/Spinner.tsx` around lines 16 - 27, The forwarded ref passed into React.forwardRef in the Spinner component isn't attached to the rendered DOM node, so consumers of Spinner get null; update the Spinner implementation (the forwardRef callback that declares SpinnerElement/SpinnerProps and the span JSX) to pass the received ref to the span (i.e., add ref={ref} on the <span>), ensuring the ref variable from forwardRef is forwarded to the rendered span element.
🧹 Nitpick comments (6)
src/components/ui/Combobox/fragments/ComboboxSearch.tsx (1)
12-14: Avoid losing internal class via prop spread order.Line 12 can be overridden by
props.classNamefrom Line 14. If this component should keep its root namespace class, mergeclassNameexplicitly.Proposed refactor
import React, { useContext } from 'react'; import ComboboxPrimitive from '~/core/primitives/Combobox/ComboboxPrimitive'; import { ComboboxRootContext } from '../contexts/ComboboxRootContext'; +import clsx from 'clsx'; @@ -const ComboboxSearch = React.forwardRef<ComboboxSearchElement, ComboboxSearchProps>((props, forwardedRef) => { +const ComboboxSearch = React.forwardRef<ComboboxSearchElement, ComboboxSearchProps>(({ className, ...props }, forwardedRef) => { const { rootClass } = useContext(ComboboxRootContext); return ( <ComboboxPrimitive.Search - className={rootClass ? `${rootClass}-search` : undefined} + className={clsx(rootClass && `${rootClass}-search`, className)} ref={forwardedRef} {...props} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Combobox/fragments/ComboboxSearch.tsx` around lines 12 - 14, The component currently sets className using rootClass (className={rootClass ? `${rootClass}-search` : undefined}) but then spreads {...props}, allowing props.className to overwrite the internal class; update ComboboxSearch to merge class names instead of letting props override: compute a mergedClassName that includes the internal `${rootClass}-search` (when rootClass exists) and any incoming props.className (using a safe join or a classnames utility) and pass that mergedClassName to the element while still spreading the other props and keeping forwardedRef.src/components/ui/Select/fragments/SelectGroup.tsx (1)
16-18: Preserve namespace class when consumer passesclassName.On Line 16,
classNameis set before{...props}(Lines 17-18), soprops.classNamecan override it. If additive styling is intended, merge both and removeclassNamefrom spread.Proposed refactor
import React, { useContext } from 'react'; import ComboboxPrimitive from '~/core/primitives/Combobox/ComboboxPrimitive'; import { SelectRootContext } from '../contexts/SelectRootContext'; +import clsx from 'clsx'; @@ -const SelectGroup = React.forwardRef<SelectGroupElement, SelectGroupComponentProps>(({ children, ...props }, forwardedRef) => { +const SelectGroup = React.forwardRef<SelectGroupElement, SelectGroupComponentProps>(({ children, className, ...props }, forwardedRef) => { const { rootClass } = useContext(SelectRootContext); return ( <ComboboxPrimitive.Group - className={rootClass ? `${rootClass}-group` : undefined} + className={clsx(rootClass && `${rootClass}-group`, className)} ref={forwardedRef} {...props} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Select/fragments/SelectGroup.tsx` around lines 16 - 18, The component sets className={rootClass ? `${rootClass}-group` : undefined} before spreading {...props}, allowing props.className to overwrite it; modify the SelectGroup render so you merge the consumer class (props.className) with the namespace class (rootClass ? `${rootClass}-group` : undefined) into a single className string (e.g., combined and deduped), remove className from the props spread, and pass the merged className along with forwardedRef and the remaining props; locate this change around the SelectGroup JSX where rootClass, forwardedRef and props are used.src/components/ui/Combobox/fragments/ComboboxIndicator.tsx (1)
12-12: Mergeprops.classNameexplicitly to keep indicator namespace class.On Line 12,
classNamemay be replaced byprops.classNamedue to subsequent spread. If internal class should always be retained, merge both class values.Proposed refactor
'use client'; import React, { useContext } from 'react'; import { ComboboxRootContext } from '../contexts/ComboboxRootContext'; import { Check } from 'lucide-react'; +import clsx from 'clsx'; @@ -const ComboboxIndicator = React.forwardRef<ComboboxIndicatorElement, ComboboxIndicatorProps>((props, forwardedRef) => { +const ComboboxIndicator = React.forwardRef<ComboboxIndicatorElement, ComboboxIndicatorProps>(({ className, ...props }, forwardedRef) => { const { rootClass } = useContext(ComboboxRootContext); return ( - <div className={rootClass ? `${rootClass}-item-indicator` : undefined} ref={forwardedRef} {...props}> + <div className={clsx(rootClass && `${rootClass}-item-indicator`, className)} ref={forwardedRef} {...props}> <Check width={16} height={16} /> </div> ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Combobox/fragments/ComboboxIndicator.tsx` at line 12, The component currently spreads {...props} after setting className so props.className can override the internal namespace; update ComboboxIndicator to merge the classes instead: build a combined class string from rootClass ? `${rootClass}-item-indicator` : '' and props.className (filtering falsy values and joining with a space), then spread props first and pass className={combinedClass} (or remove className from props before spreading) so the internal indicator class (rootClass/`${rootClass}-item-indicator`) is always preserved; reference symbols: rootClass, forwardedRef, props.className, ComboboxIndicator.knowledge/good_practices/index.md (3)
31-31: Avoid duplicated runtime-style-injection guidance.The same policy is stated at Line 31 and again in the full section at Lines 212-224. Keeping both increases doc drift risk; keep one canonical section and cross-reference it.
Possible consolidation
-Do not rely on runtime CSS injection as the default styling mechanism. Runtime injection complicates SSR, streaming, CSP, style ordering, and first paint. Prefer static CSS imports or framework-level stylesheet links. +See “Runtime Style Injection” below. Default styling should avoid runtime CSS injection and prefer static CSS imports or framework-level stylesheet links.Also applies to: 212-224
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge/good_practices/index.md` at line 31, Remove the duplicated guidance sentence "Do not rely on runtime CSS injection as the default styling mechanism..." found at the top (Line 31) and keep the full canonical policy in the later section (Lines 212-224); replace the removed sentence with a brief cross-reference pointing readers to the canonical section (or add an anchor link to that section) so there is a single source of truth and no drift between "runtime CSS injection" guidance instances.
134-138: Reduce repetitive “Document …” bullets for scannability.Lines 134-138 repeat the same opening word five times. This is readable but a small polish opportunity for flow.
Example rewrite
-- Document public slots or parts for every stylable component. -- Document generated class names using the namespace pattern. -- Document public data attributes. -- Document public CSS variables, if any exist. -- Document DOM structure guarantees that consumers can safely rely on. +- Public slots/parts for every stylable component. +- Generated class names using the namespace pattern. +- Public data attributes. +- Public CSS variables, if any exist. +- DOM structure guarantees consumers can safely rely on.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge/good_practices/index.md` around lines 134 - 138, The five list items all start with the repeated verb "Document" (e.g., "Document public slots or parts for every stylable component", "Document generated class names using the namespace pattern", "Document public data attributes", "Document public CSS variables, if any exist", "Document DOM structure guarantees that consumers can safely rely on"), so replace the repetitive bullets with a short lead sentence like "Document the following for each component:" and convert the five lines into concise noun-phrased bullets (slots/parts, generated class names, public data attributes, public CSS variables, DOM structure guarantees) or otherwise reword them to avoid repeating "Document" at the start of each bullet while preserving the original semantics and order.
210-210: Clarify wording in the portal warning sentence.Line 210 reads awkwardly (“carefully anywhere ... is expected to flow”). Consider tightening it to improve readability and avoid ambiguity.
Proposed wording
-Portals can break styling assumptions when content escapes the original subtree. Audit dialog, popover, hover card, context menu, and similar components carefully anywhere theme context, CSS variables, or namespace-based styling is expected to flow across the portal boundary. +Portals can break styling assumptions when content escapes the original subtree. Audit dialog, popover, hover card, context menu, and similar components carefully wherever theme context, CSS variables, or namespace-based styling is expected to flow across the portal boundary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge/good_practices/index.md` at line 210, The sentence starting "Portals can break styling assumptions when content escapes the original subtree..." is awkward; replace it with a clearer phrasing such as: "Portals can break styling assumptions when content renders outside the original subtree; audit dialog, popover, hover card, context menu, and similar components wherever theme context, CSS variables, or namespace-based styling must flow across the portal boundary." Update that sentence in the markdown to improve readability and remove the ambiguous "carefully anywhere ... is expected to flow" phrasing.
🤖 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/Accordion/tests/Accordion.test.tsx`:
- Around line 68-69: Save the original window.matchMedia before you overwrite it
in Accordion.test.tsx (e.g., const originalMatchMedia = window.matchMedia), then
restore it in an afterEach/afterAll hook (window.matchMedia =
originalMatchMedia) so the direct reassignment at the test (the mock created
with jest.fn() that returns a MediaQueryList-like object) does not leak to other
tests; alternatively replace the direct assignment with jest.spyOn(window,
'matchMedia').mockImplementation(...) and call mockRestore() in the teardown to
ensure global state is restored.
In `@src/components/ui/Avatar/fragments/AvatarImage.tsx`:
- Around line 10-12: The AvatarImage component currently spreads {...props}
after setting className which lets a consumer-supplied className override the
generated rootClass-based class; update AvatarImage (the forwardRef wrapper
around AvatarPrimitiveImage) to merge class names instead of allowing overwrite
by combining clsx(rootClass && `${rootClass}-image`, props.className,
props.className && props.className) or equivalent so the generated
`${rootClass}-image` always remains while still honoring consumer classes;
reference AvatarImage, AvatarPrimitiveImage, AvatarContext, rootClass and
props.className when making the change.
In `@src/components/ui/CheckboxCards/fragments/CheckboxCardsContent.tsx`:
- Line 12: The generated content class `${rootClass}-content` can be overridden
by a consumer `className` because `{...props}` comes after the explicit
className; update the JSX for CheckboxGroupPrimitive.Content so it merges the
generated namespace class with any consumer class instead of letting props
replace it: compute a combined className from `rootClass ?
`${rootClass}-content` : undefined` plus any `className`/`class` found in
`props` (filtering falsy and joining with a space), then pass that combined
value as `className` to `CheckboxGroupPrimitive.Content` while spreading the
rest of `props` (without reintroducing the original class fields). Ensure you
reference `CheckboxGroupPrimitive.Content`, `rootClass`, `props`, and
`className` when implementing the fix.
In `@src/components/ui/Skeleton/Skeleton.tsx`:
- Around line 31-33: The call to the custom hook useComponentClass must run
unconditionally to respect React Hooks rules: move the invocation of
useComponentClass(customRootClass, COMPONENT_NAME) to before the early return in
the Skeleton component so rootClass is computed on every render (even when
loading is false) and then keep the existing early return if (!loading) return
<>{children}</>; this ensures hooks are called in the same order every render.
In `@src/core/index.ts`:
- Line 3: The core entry now only exports composeRefs and mergeProps but removed
the customClassSwitcher re-export, which can break downstream consumers; restore
customClassSwitcher to the public surface (add it back to the export list
alongside composeRefs and mergeProps) or, if intentional, add an explicit
migration path: reintroduce a deprecated re-export named customClassSwitcher
that proxies to the new location and add a CHANGELOG/compat note and prepare a
major-version bump; reference the export statement containing composeRefs and
mergeProps and the symbol customClassSwitcher when making the change.
---
Outside diff comments:
In `@src/components/ui/Combobox/fragments/ComboboxTrigger.tsx`:
- Around line 18-23: The component ComboboxTrigger currently spreads {...props}
after setting className, so a consumer className overwrites
`${rootClass}-trigger`; fix by computing a merged className (merge
`${rootClass}-trigger` with props.className or the incoming className prop) and
pass that single computed value to the className prop instead of relying on
spread order—update the JSX in ComboboxTrigger.tsx to derive combinedClassName
(or use a utility like clsx) and remove/avoid letting props.className override
the namespaced `${rootClass}-trigger`.
In `@src/components/ui/Slider/fragments/SliderRange.tsx`:
- Around line 11-12: The computed internal class for the range element is being
overwritten by consumer props because SliderRange spreads {...props} after
computing classes; update SliderRange to merge its internal class (which
includes `${rootClass}-range`) with any incoming props.className (e.g., compute
a mergedClass like `${internalClass} ${props.className || ''}`) and pass that
mergedClass as the className prop to the rendered element instead of allowing
props.className to overwrite it; ensure you reference SliderRange, SliderContext
(rootClass), and props.className when making the change.
In `@src/components/ui/Slider/fragments/SliderTrack.tsx`:
- Around line 11-26: The SliderTrack component currently spreads props last
which allows incoming className and style to overwrite internal values; fix this
in the forwardRef SliderTrack function by destructuring className and style from
props (e.g., ({ children, className, style, ...props }, ref)), compute the
internal class using clsx like clsx(rootClass ? `${rootClass}-track` :
undefined, className) and compute the orientation style, then merge styles so
the orientation-driven style wins (finalStyle = { ...style, ...orientationStyle
}) and pass finalStyle and the merged className into the div before spreading
the remaining ...props; follow the pattern used in SliderRoot.tsx for merging
className/style.
In `@src/components/ui/Spinner/Spinner.tsx`:
- Around line 16-27: The forwarded ref passed into React.forwardRef in the
Spinner component isn't attached to the rendered DOM node, so consumers of
Spinner get null; update the Spinner implementation (the forwardRef callback
that declares SpinnerElement/SpinnerProps and the span JSX) to pass the received
ref to the span (i.e., add ref={ref} on the <span>), ensuring the ref variable
from forwardRef is forwarded to the rendered span element.
---
Nitpick comments:
In `@knowledge/good_practices/index.md`:
- Line 31: Remove the duplicated guidance sentence "Do not rely on runtime CSS
injection as the default styling mechanism..." found at the top (Line 31) and
keep the full canonical policy in the later section (Lines 212-224); replace the
removed sentence with a brief cross-reference pointing readers to the canonical
section (or add an anchor link to that section) so there is a single source of
truth and no drift between "runtime CSS injection" guidance instances.
- Around line 134-138: The five list items all start with the repeated verb
"Document" (e.g., "Document public slots or parts for every stylable component",
"Document generated class names using the namespace pattern", "Document public
data attributes", "Document public CSS variables, if any exist", "Document DOM
structure guarantees that consumers can safely rely on"), so replace the
repetitive bullets with a short lead sentence like "Document the following for
each component:" and convert the five lines into concise noun-phrased bullets
(slots/parts, generated class names, public data attributes, public CSS
variables, DOM structure guarantees) or otherwise reword them to avoid repeating
"Document" at the start of each bullet while preserving the original semantics
and order.
- Line 210: The sentence starting "Portals can break styling assumptions when
content escapes the original subtree..." is awkward; replace it with a clearer
phrasing such as: "Portals can break styling assumptions when content renders
outside the original subtree; audit dialog, popover, hover card, context menu,
and similar components wherever theme context, CSS variables, or namespace-based
styling must flow across the portal boundary." Update that sentence in the
markdown to improve readability and remove the ambiguous "carefully anywhere ...
is expected to flow" phrasing.
In `@src/components/ui/Combobox/fragments/ComboboxIndicator.tsx`:
- Line 12: The component currently spreads {...props} after setting className so
props.className can override the internal namespace; update ComboboxIndicator to
merge the classes instead: build a combined class string from rootClass ?
`${rootClass}-item-indicator` : '' and props.className (filtering falsy values
and joining with a space), then spread props first and pass
className={combinedClass} (or remove className from props before spreading) so
the internal indicator class (rootClass/`${rootClass}-item-indicator`) is always
preserved; reference symbols: rootClass, forwardedRef, props.className,
ComboboxIndicator.
In `@src/components/ui/Combobox/fragments/ComboboxSearch.tsx`:
- Around line 12-14: The component currently sets className using rootClass
(className={rootClass ? `${rootClass}-search` : undefined}) but then spreads
{...props}, allowing props.className to overwrite the internal class; update
ComboboxSearch to merge class names instead of letting props override: compute a
mergedClassName that includes the internal `${rootClass}-search` (when rootClass
exists) and any incoming props.className (using a safe join or a classnames
utility) and pass that mergedClassName to the element while still spreading the
other props and keeping forwardedRef.
In `@src/components/ui/Select/fragments/SelectGroup.tsx`:
- Around line 16-18: The component sets className={rootClass ?
`${rootClass}-group` : undefined} before spreading {...props}, allowing
props.className to overwrite it; modify the SelectGroup render so you merge the
consumer class (props.className) with the namespace class (rootClass ?
`${rootClass}-group` : undefined) into a single className string (e.g., combined
and deduped), remove className from the props spread, and pass the merged
className along with forwardedRef and the remaining props; locate this change
around the SelectGroup JSX where rootClass, forwardedRef and props are used.
🪄 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: 1cbbabdb-c44e-4b8b-9bd5-df17c635853a
⛔ Files ignored due to path filters (2)
src/components/ui/Slider/tests/__snapshots__/Slider.test.tsx.snapis excluded by!**/*.snapsrc/components/ui/TextArea/__snapshots__/TextArea.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (187)
knowledge/features/data-attributes.mdknowledge/features/styling-namespaces.mdknowledge/good_practices/index.mdknowledge/what-makes-a-library-truly-headless.mdsrc/components/tools/SandboxEditor/SandboxEditor.tsxsrc/components/ui/Accordion/fragments/AccordionContent.tsxsrc/components/ui/Accordion/fragments/AccordionHeader.tsxsrc/components/ui/Accordion/fragments/AccordionItem.tsxsrc/components/ui/Accordion/fragments/AccordionRoot.tsxsrc/components/ui/Accordion/fragments/AccordionTrigger.tsxsrc/components/ui/Accordion/tests/Accordion.test.tsxsrc/components/ui/AlertDialog/fragments/AlertDialogAction.tsxsrc/components/ui/AlertDialog/fragments/AlertDialogCancel.tsxsrc/components/ui/AlertDialog/fragments/AlertDialogContent.tsxsrc/components/ui/AlertDialog/fragments/AlertDialogDescription.tsxsrc/components/ui/AlertDialog/fragments/AlertDialogFooter.tsxsrc/components/ui/AlertDialog/fragments/AlertDialogOverlay.tsxsrc/components/ui/AlertDialog/fragments/AlertDialogRoot.tsxsrc/components/ui/AlertDialog/fragments/AlertDialogTitle.tsxsrc/components/ui/AlertDialog/fragments/AlertDialogTrigger.tsxsrc/components/ui/AspectRatio/AspectRatio.tsxsrc/components/ui/Avatar/fragments/AvatarFallback.tsxsrc/components/ui/Avatar/fragments/AvatarImage.tsxsrc/components/ui/Avatar/fragments/AvatarRoot.tsxsrc/components/ui/AvatarGroup/fragments/AvatarGroupFallback.tsxsrc/components/ui/AvatarGroup/fragments/AvatarGroupItem.tsxsrc/components/ui/AvatarGroup/fragments/AvatarGroupRoot.tsxsrc/components/ui/Badge/Badge.tsxsrc/components/ui/BlockQuote/BlockQuote.tsxsrc/components/ui/Button/Button.tsxsrc/components/ui/Button/tests/Button.asChild.test.tsxsrc/components/ui/Button/tests/Button.test.tsxsrc/components/ui/Callout/fragments/CalloutIcon.tsxsrc/components/ui/Callout/fragments/CalloutRoot.tsxsrc/components/ui/Callout/fragments/CalloutText.tsxsrc/components/ui/Card/fragments/CardAction.tsxsrc/components/ui/Card/fragments/CardContent.tsxsrc/components/ui/Card/fragments/CardDescription.tsxsrc/components/ui/Card/fragments/CardFooter.tsxsrc/components/ui/Card/fragments/CardHeader.tsxsrc/components/ui/Card/fragments/CardRoot.tsxsrc/components/ui/Card/fragments/CardTitle.tsxsrc/components/ui/Card/tests/Card.test.tsxsrc/components/ui/Checkbox/fragments/CheckboxIndicator.tsxsrc/components/ui/Checkbox/fragments/CheckboxRoot.tsxsrc/components/ui/CheckboxCards/fragments/CheckboxCardsContent.tsxsrc/components/ui/CheckboxCards/fragments/CheckboxCardsIndicator.tsxsrc/components/ui/CheckboxCards/fragments/CheckboxCardsItem.tsxsrc/components/ui/CheckboxCards/fragments/CheckboxCardsRoot.tsxsrc/components/ui/CheckboxGroup/fragments/CheckboxGroupIndicator.tsxsrc/components/ui/CheckboxGroup/fragments/CheckboxGroupLabel.tsxsrc/components/ui/CheckboxGroup/fragments/CheckboxGroupRoot.tsxsrc/components/ui/CheckboxGroup/fragments/CheckboxGroupTrigger.tsxsrc/components/ui/Code/Code.tsxsrc/components/ui/Collapsible/fragments/CollapsibleRoot.tsxsrc/components/ui/Combobox/fragments/ComboboxContent.tsxsrc/components/ui/Combobox/fragments/ComboboxGroup.tsxsrc/components/ui/Combobox/fragments/ComboboxIndicator.tsxsrc/components/ui/Combobox/fragments/ComboboxItem.tsxsrc/components/ui/Combobox/fragments/ComboboxRoot.tsxsrc/components/ui/Combobox/fragments/ComboboxSearch.tsxsrc/components/ui/Combobox/fragments/ComboboxTrigger.tsxsrc/components/ui/ContextMenu/fragments/ContextMenuContent.tsxsrc/components/ui/ContextMenu/fragments/ContextMenuItem.tsxsrc/components/ui/ContextMenu/fragments/ContextMenuRoot.tsxsrc/components/ui/ContextMenu/fragments/ContextMenuSub.tsxsrc/components/ui/ContextMenu/fragments/ContextMenuSubTrigger.tsxsrc/components/ui/ContextMenu/fragments/ContextMenuTrigger.tsxsrc/components/ui/DataList/fragments/DataListItem.tsxsrc/components/ui/DataList/fragments/DataListLabel.tsxsrc/components/ui/DataList/fragments/DataListRoot.tsxsrc/components/ui/DataList/fragments/DataListValue.tsxsrc/components/ui/DataList/tests/DataList.test.tsxsrc/components/ui/Dialog/fragments/DialogClose.tsxsrc/components/ui/Dialog/fragments/DialogContent.tsxsrc/components/ui/Dialog/fragments/DialogDescription.tsxsrc/components/ui/Dialog/fragments/DialogFooter.tsxsrc/components/ui/Dialog/fragments/DialogOverlay.tsxsrc/components/ui/Dialog/fragments/DialogRoot.tsxsrc/components/ui/Dialog/fragments/DialogTitle.tsxsrc/components/ui/Dialog/fragments/DialogTrigger.tsxsrc/components/ui/Disclosure/fragments/DisclosureContent.tsxsrc/components/ui/Disclosure/fragments/DisclosureItem.tsxsrc/components/ui/Disclosure/fragments/DisclosureRoot.tsxsrc/components/ui/Disclosure/fragments/DisclosureTrigger.tsxsrc/components/ui/DropdownMenu/fragments/DropdownMenuContent.tsxsrc/components/ui/DropdownMenu/fragments/DropdownMenuItem.tsxsrc/components/ui/DropdownMenu/fragments/DropdownMenuRoot.tsxsrc/components/ui/DropdownMenu/fragments/DropdownMenuSub.tsxsrc/components/ui/DropdownMenu/fragments/DropdownMenuSubTrigger.tsxsrc/components/ui/DropdownMenu/fragments/DropdownMenuTrigger.tsxsrc/components/ui/Em/Em.tsxsrc/components/ui/Heading/Heading.tsxsrc/components/ui/HoverCard/fragments/HoverCardArrow.tsxsrc/components/ui/HoverCard/fragments/HoverCardRoot.tsxsrc/components/ui/Kbd/Kbd.tsxsrc/components/ui/Link/Link.tsxsrc/components/ui/Menubar/fragments/MenubarContent.tsxsrc/components/ui/Menubar/fragments/MenubarItem.tsxsrc/components/ui/Menubar/fragments/MenubarMenu.tsxsrc/components/ui/Menubar/fragments/MenubarRoot.tsxsrc/components/ui/Menubar/fragments/MenubarSub.tsxsrc/components/ui/Menubar/fragments/MenubarSubTrigger.tsxsrc/components/ui/Menubar/fragments/MenubarTrigger.tsxsrc/components/ui/Minimap/fragments/MinimapBubble.tsxsrc/components/ui/Minimap/fragments/MinimapContent.tsxsrc/components/ui/Minimap/fragments/MinimapItem.tsxsrc/components/ui/Minimap/fragments/MinimapLine.tsxsrc/components/ui/Minimap/fragments/MinimapRoot.tsxsrc/components/ui/Minimap/fragments/MinimapTrack.tsxsrc/components/ui/NavigationMenu/fragments/NavigationMenuContent.tsxsrc/components/ui/NavigationMenu/fragments/NavigationMenuItem.tsxsrc/components/ui/NavigationMenu/fragments/NavigationMenuLink.tsxsrc/components/ui/NavigationMenu/fragments/NavigationMenuRoot.tsxsrc/components/ui/NavigationMenu/fragments/NavigationMenuTrigger.tsxsrc/components/ui/NumberField/fragments/NumberFieldDecrement.tsxsrc/components/ui/NumberField/fragments/NumberFieldIncrement.tsxsrc/components/ui/NumberField/fragments/NumberFieldInput.tsxsrc/components/ui/NumberField/fragments/NumberFieldRoot.tsxsrc/components/ui/Progress/fragments/ProgressIndicator.tsxsrc/components/ui/Progress/fragments/ProgressRoot.tsxsrc/components/ui/Quote/Quote.tsxsrc/components/ui/Radio/Radio.tsxsrc/components/ui/RadioCards/fragments/RadioCardsItem.tsxsrc/components/ui/RadioCards/fragments/RadioCardsRoot.tsxsrc/components/ui/RadioGroup/fragments/RadioGroupIndicator.tsxsrc/components/ui/RadioGroup/fragments/RadioGroupItem.tsxsrc/components/ui/RadioGroup/fragments/RadioGroupLabel.tsxsrc/components/ui/RadioGroup/fragments/RadioGroupRoot.tsxsrc/components/ui/ScrollArea/fragments/ScrollAreaRoot.tsxsrc/components/ui/Select/fragments/SelectContent.tsxsrc/components/ui/Select/fragments/SelectGroup.tsxsrc/components/ui/Select/fragments/SelectIndicator.tsxsrc/components/ui/Select/fragments/SelectItem.tsxsrc/components/ui/Select/fragments/SelectRoot.tsxsrc/components/ui/Select/fragments/SelectTrigger.tsxsrc/components/ui/Separator/Separator.tsxsrc/components/ui/Skeleton/Skeleton.tsxsrc/components/ui/Skeleton/tests/Skeleton.test.tsxsrc/components/ui/Slider/fragments/SliderMarks.tsxsrc/components/ui/Slider/fragments/SliderRange.tsxsrc/components/ui/Slider/fragments/SliderRangeSlider.tsxsrc/components/ui/Slider/fragments/SliderRoot.tsxsrc/components/ui/Slider/fragments/SliderThumb.tsxsrc/components/ui/Slider/fragments/SliderTrack.tsxsrc/components/ui/Spinner/Spinner.tsxsrc/components/ui/Splitter/fragments/SplitterHandle.tsxsrc/components/ui/Splitter/fragments/SplitterPanel.tsxsrc/components/ui/Splitter/fragments/SplitterRoot.tsxsrc/components/ui/Steps/fragments/StepBubble.tsxsrc/components/ui/Steps/fragments/StepContent.tsxsrc/components/ui/Steps/fragments/StepDescription.tsxsrc/components/ui/Steps/fragments/StepItem.tsxsrc/components/ui/Steps/fragments/StepLine.tsxsrc/components/ui/Steps/fragments/StepRoot.tsxsrc/components/ui/Steps/fragments/StepTitle.tsxsrc/components/ui/Steps/fragments/StepTrack.tsxsrc/components/ui/Strong/Strong.tsxsrc/components/ui/Switch/fragments/SwitchRoot.tsxsrc/components/ui/Switch/fragments/SwitchThumb.tsxsrc/components/ui/Switch/tests/Switch.test.tsxsrc/components/ui/TabNav/fragments/TabNavLink.tsxsrc/components/ui/TabNav/fragments/TabNavRoot.tsxsrc/components/ui/Table/fragments/TableRoot.tsxsrc/components/ui/Tabs/fragments/TabContent.tsxsrc/components/ui/Tabs/fragments/TabList.tsxsrc/components/ui/Tabs/fragments/TabRoot.tsxsrc/components/ui/Tabs/fragments/TabTrigger.tsxsrc/components/ui/Text/Text.tsxsrc/components/ui/TextArea/fragments/TextAreaRoot.tsxsrc/components/ui/Theme/Theme.tsxsrc/components/ui/Theme/ThemeContext.tsxsrc/components/ui/Theme/tests/Theme.test.tsxsrc/components/ui/Theme/useComponentClass.tssrc/components/ui/Toggle/Toggle.tsxsrc/components/ui/ToggleGroup/fragments/ToggleGroupRoot.tsxsrc/components/ui/ToggleGroup/fragments/ToggleItem.tsxsrc/components/ui/Toolbar/fragments/ToolbarButton.tsxsrc/components/ui/Toolbar/fragments/ToolbarLink.tsxsrc/components/ui/Toolbar/fragments/ToolbarRoot.tsxsrc/components/ui/Toolbar/fragments/ToolbarSeparator.tsxsrc/components/ui/Tree/fragments/TreeItem.tsxsrc/components/ui/Tree/fragments/TreeRoot.tsxsrc/components/ui/VisuallyHidden/VisuallyHidden.tsxsrc/core/customClassSwitcher/customClassSwitcher.test.tsxsrc/core/customClassSwitcher/index.tssrc/core/index.ts
💤 Files with no reviewable changes (2)
- src/core/customClassSwitcher/index.ts
- src/core/customClassSwitcher/customClassSwitcher.test.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
knowledge/good_practices/index.md (1)
237-237: Small wording polish opportunity.At Line 237, consider replacing “by accident” with “inadvertently” for tighter technical tone.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knowledge/good_practices/index.md` at line 237, Replace the phrase "Do not leak internal implementation attributes as part of the public styling API by accident." with "Do not leak internal implementation attributes as part of the public styling API inadvertently." — locate the exact sentence in the knowledge/good_practices/index.md content (the line containing the phrase "public styling API by accident") and update the wording to use "inadvertently" for a tighter technical tone.src/components/ui/Accordion/tests/Accordion.test.tsx (1)
63-70: Strengthen “classless” assertions to catch emptyclass=""regressions.Using
.className === ''passes both when theclassattribute is absent and whenclass=""is rendered. If the contract is “no empty class attributes,” assert against the attribute directly.Suggested test update
- expect(screen.getByTestId('accordion-root').className).toBe(''); - expect(screen.getByTestId('accordion-item').className).toBe(''); - expect(screen.getByTestId('accordion-header').className).toBe(''); - expect(screen.getByTestId('accordion-trigger').className).toBe(''); - expect(content.className).toBe(''); - expect(content.firstElementChild?.className).toBe(''); + expect(screen.getByTestId('accordion-root')).not.toHaveAttribute('class'); + expect(screen.getByTestId('accordion-item')).not.toHaveAttribute('class'); + expect(screen.getByTestId('accordion-header')).not.toHaveAttribute('class'); + expect(screen.getByTestId('accordion-trigger')).not.toHaveAttribute('class'); + expect(content).not.toHaveAttribute('class'); + expect(content.firstElementChild).not.toHaveAttribute('class');🤖 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 63 - 70, The tests currently assert empty className (e.g., screen.getByTestId('accordion-root').className === ''), which doesn't distinguish between missing class attribute and an explicit class=""; update each assertion for the tested elements (accordion-root, accordion-item, accordion-header, accordion-trigger, accordion-content, and content.firstElementChild) to assert the class attribute is absent — e.g., use element.getAttribute('class') toBeNull() or expect(element.hasAttribute('class')).toBe(false) — so the test fails if an empty class="" is rendered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@knowledge/good_practices/index.md`:
- Around line 157-170: The examples reference `root` but never define it; before
the assertions, capture the rendered element and assign it to `root` (e.g. call
render(...) and then const { container } = render(...); const root =
container.firstElementChild as HTMLElement; or const root =
container.querySelector('.accordion-root') as HTMLElement) — do this in both
snippets that render <Accordion.Root /> (and the themed variant with <Theme
classNamespace="acme">) so the subsequent expect(...) assertions have a defined
`root`.
---
Nitpick comments:
In `@knowledge/good_practices/index.md`:
- Line 237: Replace the phrase "Do not leak internal implementation attributes
as part of the public styling API by accident." with "Do not leak internal
implementation attributes as part of the public styling API inadvertently." —
locate the exact sentence in the knowledge/good_practices/index.md content (the
line containing the phrase "public styling API by accident") and update the
wording to use "inadvertently" for a tighter technical tone.
In `@src/components/ui/Accordion/tests/Accordion.test.tsx`:
- Around line 63-70: The tests currently assert empty className (e.g.,
screen.getByTestId('accordion-root').className === ''), which doesn't
distinguish between missing class attribute and an explicit class=""; update
each assertion for the tested elements (accordion-root, accordion-item,
accordion-header, accordion-trigger, accordion-content, and
content.firstElementChild) to assert the class attribute is absent — e.g., use
element.getAttribute('class') toBeNull() or
expect(element.hasAttribute('class')).toBe(false) — so the test fails if an
empty class="" is rendered.
🪄 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: 63cbc0c8-e328-431f-b514-6a490e13a424
📒 Files selected for processing (14)
knowledge/good_practices/index.mdsrc/components/tools/SandboxEditor/SandboxEditor.tsxsrc/components/ui/Accordion/tests/Accordion.test.tsxsrc/components/ui/Avatar/fragments/AvatarImage.tsxsrc/components/ui/CheckboxCards/fragments/CheckboxCardsContent.tsxsrc/components/ui/Combobox/fragments/ComboboxIndicator.tsxsrc/components/ui/Combobox/fragments/ComboboxSearch.tsxsrc/components/ui/Combobox/fragments/ComboboxTrigger.tsxsrc/components/ui/Select/fragments/SelectGroup.tsxsrc/components/ui/Skeleton/Skeleton.tsxsrc/components/ui/Slider/fragments/SliderRange.tsxsrc/components/ui/Slider/fragments/SliderTrack.tsxsrc/components/ui/Spinner/Spinner.tsxsrc/core/customClassSwitcher/index.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- src/components/tools/SandboxEditor/SandboxEditor.tsx
- src/components/ui/Combobox/fragments/ComboboxIndicator.tsx
- src/components/ui/CheckboxCards/fragments/CheckboxCardsContent.tsx
- src/components/ui/Avatar/fragments/AvatarImage.tsx
- src/components/ui/Select/fragments/SelectGroup.tsx
- src/components/ui/Slider/fragments/SliderTrack.tsx
- src/components/ui/Skeleton/Skeleton.tsx
- src/components/ui/Combobox/fragments/ComboboxTrigger.tsx
- src/core/customClassSwitcher/index.ts
- src/components/ui/Spinner/Spinner.tsx
Contributor
CoverageThis report compares the PR with the base branch. "Δ" shows how the PR affects each metric.
Coverage decreased for at least one metric. Please add or update tests to improve coverage. Run |
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
Release Notes
Documentation
New Features
classNamespaceprop for namespaced CSS class generation.Refactor
Tests