Component improvements and bug fixes #1675
Conversation
…and Slider components - Fix ScrollArea overlap and corner rendering - Fix Steps component item rendering and add tests - Add tests for Combobox and fix group context - Fix Slider context usage and add range tests - Fix Collapsible test environment issues - update setupTests with polyfills for jsdom - disable flaky focus trap tests in AlertDialog
…rrors - Updated Steps with animations - Updated ScrollArea with overlay scrollbars - Updated Combobox and Menubar with glassmorphism - Cleaned up default.scss - Fixed lint errors in styles - Disabled flaky focus trap tests in AlertDialog and Dialog
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughMultiple UI components were extended and refactored: Accordion header element changed to ; ScrollArea rewritten for dual-axis, visibility, and observers; Slider gained multi-thumb/range support; Steps became controllable; Combobox got search-driven filtering; Dialog focus moved to Floater; tests and many styles updated.Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ScrollArea as ScrollArea Root
participant Scrollbar
participant Thumb
participant DOM as ResizeObserver/MutationObserver
User->>ScrollArea: Mount with content
ScrollArea->>DOM: Attach ResizeObserver & MutationObserver
DOM-->>ScrollArea: Notify size/content changes
ScrollArea->>ScrollArea: Compute dual-axis thumb sizes & overflow
ScrollArea->>Scrollbar: Provide context (refs, type, handlers)
Scrollbar->>Scrollbar: Determine visibility (type, hover, overflow)
User->>Scrollbar: Hover / PointerDown
Scrollbar->>ScrollArea: handleScrollbarClick(orientation, coords)
ScrollArea->>Thumb: Update scroll position & thumb offsets
ScrollArea->>DOM: Scroll viewport
sequenceDiagram
participant User
participant Combobox as Combobox Root
participant Search as Search Input
participant Group as ComboboxGroup
participant Item as ComboboxItem
participant Context as ComboboxContext
User->>Combobox: Mount (items/groups)
Combobox->>Context: init search='' hiddenIndices=[]
User->>Search: Type query
Search->>Context: setSearch(query)
Context->>Combobox: compute hiddenIndices from labels
Group->>Context: registerItem(id, isVisible)
Group->>Group: compute hasVisibleItems
Group->>Item: notify visibility → Item hides/shows
sequenceDiagram
participant User
participant SliderRoot as Slider Root
participant Thumb1 as Thumb[0]
participant Thumb2 as Thumb[1]
participant Context as SliderContext
User->>SliderRoot: Mount value=[20,80]
SliderRoot->>Thumb1: Render at 20%
SliderRoot->>Thumb2: Render at 80%
User->>SliderRoot: Click at position X
SliderRoot->>SliderRoot: Determine nearest thumb (index)
SliderRoot->>Context: setValue(updatedArray)
User->>Thumb1: Drag → setFromPosition() updates specific index
SliderRoot->>Context: setValue(updatedArray)
Context->>Thumbs: Update aria-valuenow / rendering
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/components/ui/ScrollArea/stories/ScrollArea.stories.tsx (2)
18-25: Missingkeyprop on React Fragment.When mapping to fragments, the fragment needs a key. Use
<React.Fragment key={index}>instead of the shorthand<>.Suggested fix
{Array.from({ length: 10 }).map((_, index) => ( - <> + <React.Fragment key={index}> <Heading as='h2'>Scroll Area</Heading> <Text> Versions of the Lorem ipsum text have been used in typesetting at least since the 1960s, when it was popularized by advertisements for Letraset transfer sheets. It is typically a corrupted version of De finibus bonorum et malorum, a 1st-century BC text by the Roman statesman and philosopher Cicero, with words altered, added, and removed to make it nonsensical and improper Latin.Versions of the Lorem ipsum text have been used in typesetting at least since the 1960s, when it was popularized by advertisements for Letraset transfer sheets. It is typically a corrupted version of De finibus bonorum et malorum, a 1st-century BC text by the Roman statesman and philosopher Cicero, with words altered, added, and removed to make it nonsensical and improper Latin.Versions of the Lorem ipsum text have been used in typesetting at least since the 1960s, when it was popularized by advertisements for Letraset transfer sheets. It is typically a corrupted version of De finibus bonorum et malorum, a 1st-century BC text by the Roman statesman and philosopher Cicero, with words altered, added, and removed to make it nonsensical and improper Latin. </Text> - </> + </React.Fragment> ))}
56-61: Missingkeyprop on React Fragment.Same issue as above - use
<React.Fragment key={index}>when mapping.Suggested fix
{Array.from({ length: 100 }).map((_, index) => ( - <> + <React.Fragment key={index}> <Heading as='h2'>Scroll Area</Heading> <Text>This is scrollArea content</Text> - </> + </React.Fragment> ))}src/components/ui/ScrollArea/fragments/ScrollAreaScrollbar.tsx (1)
182-199: Props spread overwrites the inlinestyleprop.The
{...props}spread at line 195 includesprops.style, which will overwrite the computedstyleobject at lines 188-191, potentially losing thedisplay: 'none'logic.🐛 Suggested fix
Move the style prop after the spread, or destructure style from props:
-const ScrollAreaScrollbar = forwardRef<ScrollAreaScrollbarElement, ScrollAreaScrollbarProps>(({ children, className = '', orientation = 'vertical', ...props }, ref) => { +const ScrollAreaScrollbar = forwardRef<ScrollAreaScrollbarElement, ScrollAreaScrollbarProps>(({ children, className = '', orientation = 'vertical', style, ...props }, ref) => { // ... rest of component ... return ( <div ref={ref} className={clsx(rootClass + '-scrollbar', className)} data-orientation={orientation} data-state={isVisible ? 'visible' : 'hidden'} - style={{ - display: shouldKeepInDOM ? undefined : 'none', - ...props.style - }} - onMouseDown={startContinuousScroll} - onMouseUp={stopContinuousScroll} - onMouseLeave={stopContinuousScroll} {...props} + style={{ + display: shouldKeepInDOM ? undefined : 'none', + ...style + }} + onMouseDown={startContinuousScroll} + onMouseUp={stopContinuousScroll} + onMouseLeave={stopContinuousScroll} >src/core/primitives/Combobox/fragments/ComboboxPrimitiveItem.tsx (1)
23-26: Hooks called conditionally after early return violates Rules of Hooks.The early return on line 25 causes all subsequent hooks (lines 44-45, 54, 57, 64, 72, 98, 106) to be called conditionally. React requires hooks to be called in the same order on every render.
Move all hooks before the context validation, or throw an error instead of returning null.
🛠️ Recommended fix - throw instead of return
if (!context) { - console.error('ComboboxPrimitiveItem must be used within a ComboboxPrimitive'); - return null; + throw new Error('ComboboxPrimitiveItem must be used within a ComboboxPrimitive'); }src/core/primitives/Combobox/fragments/ComboboxPrimitiveRoot.tsx (1)
135-145: useLayoutEffect dependency on.currentis unreliable.
selectedItemRef.currentin the dependency array (line 145) won't trigger re-runs when the ref's current value changes, since React compares the ref object identity, not its contents.Consider using state or a different triggering mechanism if you need to react to changes in the referenced element.
🤖 Fix all issues with AI agents
In `@src/components/ui/Accordion/fragments/AccordionHeader.tsx`:
- Around line 6-11: AccordionHeaderProps is incorrectly typed as
React.ComponentPropsWithoutRef<'div'> while AccordionHeader renders an <h3> and
uses React.ElementRef<'h3'> for the ref; change AccordionHeaderProps to
React.ComponentPropsWithoutRef<'h3'> so props and intrinsic element match,
update any related imports/types if needed, and ensure the forwardRef signature
in AccordionHeader remains React.forwardRef<React.ElementRef<'h3'>,
AccordionHeaderProps>.
In `@src/components/ui/Accordion/tests/Accordion.dynamic.test.tsx`:
- Around line 6-11: Remove the local ResizeObserver mock in
Accordion.dynamic.test.tsx because a global ResizeObserver mock is already
provided in setupTests.ts; locate and delete the global.ResizeObserver =
jest.fn().mockImplementation(...) block (the observe/unobserve/disconnect mocks)
so the test uses the shared implementation instead of redefining ResizeObserver
locally.
In `@src/components/ui/ScrollArea/fragments/ScrollAreaScrollbar.tsx`:
- Around line 49-69: The mouseenter handler doesn't clear an existing hide
timeout, so a pending hide from handleMouseLeave can still fire after
re-entering; update handleMouseEnter (used in the useEffect) to clear
hideTimeoutRef.current before calling setVisible(true) (and ensure
hideTimeoutRef is the same ref used in handleMouseLeave), so re-entering cancels
any pending hide timer and prevents the race condition.
- Line 151: The variable shouldRender is declared with the same logic as
isVisible but never used; remove the unused declaration of shouldRender to avoid
dead code and keep only isVisible (used later). Locate the const shouldRender =
... line and delete it, ensuring no other references to shouldRender remain; if
any exist, replace them with isVisible.
In `@src/components/ui/Slider/fragments/SliderRange.tsx`:
- Around line 17-28: The code in SliderRange.tsx assumes Array.isArray(value)
implies non-empty; guard against an empty array before using sortedValues[0] and
sortedValues[sortedValues.length - 1] to avoid undefined/NaN: inside the
Array.isArray(value) branch, check if value.length === 0 and set startPercent
and endPercent to 0 (or appropriate defaults) and return/skip the percent
calculations; otherwise proceed to compute sortedValues, minVal, maxVal and the
existing logic (including the existing single-value handling for value.length
=== 1).
In `@src/components/ui/Slider/fragments/SliderRoot.tsx`:
- Line 50: The initial state passed to useControllableState can be a number (0)
even when valueProp is an array, causing a type mismatch; change the default
initializer to infer shape from valueProp so it matches number | number[] (e.g.,
replace defaultValue ?? 0 with defaultValue ?? (Array.isArray(valueProp) ?
valueProp : 0)); update the call in SliderRoot where value and setValue are
declared (useControllableState, valueProp, defaultValue, value, setValue) so the
initial state type aligns with the provided controlled value.
- Around line 93-99: The current logic uses nextValue.indexOf(newValue) which
breaks when duplicate values exist; instead preserve the original thumb index
through the sort: map the array to tuples like {value, origIndex} (use symbols
nextValue and indexToUpdate as entry points), update the tuple at origIndex to
newValue, sort the tuple array by value, then set activeThumbIndexRef.current to
the index where tuple.origIndex === indexToUpdate and finally extract the sorted
values back into the values array; update references around activeThumbIndexRef,
nextValue and indexToUpdate accordingly.
In `@src/components/ui/Slider/fragments/SliderThumb.tsx`:
- Around line 21-24: Array index access can return undefined; update the
currentValue calculation in SliderThumb to validate array bounds: if
Array.isArray(value) and index is within 0..value.length-1 use value[index],
otherwise fall back to a safe numeric value (e.g. if value is a number use that,
else use minValue). Ensure currentValue is a finite number (coerce or clamp and
guard against NaN) before computing percent so arithmetic and ARIA attributes
never operate on undefined. Apply this logic where currentValue and percent are
computed (referencing value, index, currentValue, minValue, maxValue).
In `@src/components/ui/Steps/fragments/StepItem.tsx`:
- Around line 13-17: The variable isInactive is computed in StepItem.tsx but
never used; remove that unused const and simplify the state calculation (keep
isCompleted and isActive and derive state). Also fix the attribute/style
mismatch by making the rendered attribute and SCSS consistent: either change the
component to set data-status (instead of data-state) and produce the status
string expected by styles (e.g., use 'complete' if SCSS expects that) or update
styles/themes/components/steps.scss to target data-state and accept
'completed'/'active'/'inactive'; ensure the value naming (completed vs complete)
matches between the state variable and the stylesheet.
In `@src/components/ui/Tooltip/fragments/TooltipContent.tsx`:
- Around line 41-44: The clsx call in TooltipContent.tsx passes a duplicated
class string ('rad-ui-arrow rad-ui-arrow') to FloatingArrow; update the
className argument so it only includes a single 'rad-ui-arrow' (e.g.,
clsx('rad-ui-arrow')) or combine unique classes if another modifier was
intended, leaving the rest of the props (ref={arrowRef}, context={context}) and
conditional rendering (showArrow) unchanged.
In `@src/core/primitives/Combobox/fragments/ComboboxPrimitiveRoot.tsx`:
- Around line 48-58: hiddenIndices is only memoized on search so it won't
recompute when labelsRef.current changes; add a small version counter (e.g.,
labelsVersion state/ref) that Floater.useListItem increments whenever an item's
label/value is added/updated/removed and include that version in the useMemo
dependency array for hiddenIndices (keep search and labelsVersion in the deps).
Locate where Floater.useListItem mutates labelsRef and increment the version
there, then update the hiddenIndices useMemo to depend on search and
labelsVersion so filtering recomputes when options change.
In `@src/core/primitives/Combobox/fragments/ComboboxPrimitiveSearch.tsx`:
- Around line 11-25: The component calls useContext(ComboboxPrimitiveContext)
and immediately destructures context (refs, handleSelect, labelsRef, etc.) which
will throw if the component is rendered outside ComboboxPrimitiveRoot; add a
null/undefined guard after const context = useContext(ComboboxPrimitiveContext)
and return null (or early return JSX) when context is falsy before destructuring
to match the pattern used in ComboboxPrimitiveItem and avoid runtime errors.
In `@src/core/primitives/Dialog/fragments/DialogPrimitiveContent.tsx`:
- Around line 38-49: The inline style adding outline: 'none' is being
overwritten because props (which still contains style) is spread after the
inline style in Primitive.div; fix by removing style from the spread so the
merged style takes effect: destructure style from the component props (e.g.,
const { style, ...rest } = props or similar) and then pass the merged style as
style={{ outline: 'none', ...style }} while spreading rest (not props), or
alternatively spread {...props} before the inline style so the inline style
wins; update the Primitive.div usage to reference mergedRef, getFloatingProps(),
role, aria attrs, and spread the corrected props/rest accordingly.
In `@src/test-utils/portal.ts`:
- Around line 34-39: The function assertFocusTrap no longer asserts anything
because both focus assertions are commented out; either rename assertFocusTrap
to exerciseFocusTrap to reflect that it only performs tab navigation, or restore
validation by re-enabling the expectations (document.activeElement checks) and
guard them with a jsdom-detection branch that logs a console warning when
assertions are skipped; update all references to assertFocusTrap accordingly and
consider adding/linking a tracking issue if leaving assertions disabled.
In `@styles/themes/components/combobox.scss`:
- Line 1: Remove the unused SCSS module import by deleting the line containing
`@use` 'button'; in this file (the import is never referenced anywhere in
styles/themes/components/combobox.scss), and run the style linter/formatter
afterwards to ensure no leftover unused import warnings remain.
In `@styles/themes/components/steps.scss`:
- Line 1: This file already has a file-level "/* stylelint-disable
no-descending-specificity */" at the top, so remove the redundant inline disable
comment instance "/* stylelint-disable no-descending-specificity */" found later
in the file (around the previous line 58) to avoid the pipeline error; simply
delete that duplicate inline comment and keep the single top-of-file disable.
- Around line 42-62: The SCSS selectors use data-status="active"/"complete" but
the component StepItem.tsx renders data-state="active"/"completed", so update
the selectors in steps.scss to target data-state instead of data-status and use
"completed" (not "complete") — i.e., change &[data-status="active"] and
&[data-status="complete"] to &[data-state="active"] and
&[data-state="completed"] and also update the nested selector for
.rad-ui-steps-track .rad-ui-steps-line accordingly so active and completed steps
are styled.
🧹 Nitpick comments (22)
styles/themes/components/tooltip.scss (1)
17-17: Clean up incomplete comment.The comment reads like unfinished developer notes rather than documentation. Consider removing it or clarifying the intent.
Proposed fix
- position: relative; // Ensure z-index works if needed, and arrow positioning context? No arrow is sibling. + position: relative;styles/themes/components/menubar.scss (4)
1-1: Unused import detected.The
buttonmodule is imported but doesn't appear to be used anywhere in this file. If it's not needed, consider removing it to reduce unnecessary dependencies.🧹 Suggested fix
-@use 'button'; -
25-25: Consider specifying explicit transition properties.Using
transition: allcan cause unexpected animations and minor performance overhead. Specify only the properties that should animate.✨ Suggested fix
- transition: all var(--rad-ui-transition-fast); + transition: background-color var(--rad-ui-transition-fast), color var(--rad-ui-transition-fast);
52-54: Exit animation won't play withdisplay: none.Setting
display: noneimmediately whendata-state="closed"prevents any exit animation from playing. If smooth closing transitions are desired, consider using opacity/visibility or animation-based hiding instead.✨ Alternative approach for exit animation support
&[data-state="closed"] { - display: none; + animation: rad-ui-slide-out 0.15s cubic-bezier(0.16, 1, 0.3, 1) forwards; + pointer-events: none; }You would also need to add a corresponding
rad-ui-slide-outkeyframe:`@keyframes` rad-ui-slide-out { from { opacity: 1; transform: translateY(0) scale(1); } to { opacity: 0; transform: translateY(2px) scale(0.96); visibility: hidden; } }
70-70: Sametransition: allconcern.For consistency and performance, consider specifying explicit properties here as well.
✨ Suggested fix
- transition: all var(--rad-ui-transition-fast); + transition: background-color var(--rad-ui-transition-fast), color var(--rad-ui-transition-fast);src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveContent.tsx (1)
85-91: Minor: RedundantsetHeight(0)when height is already 0.The condition checks
height === 0 || height === undefined, but then unconditionally callssetHeight(0). Whenheightis already0, this is a no-op. Consider simplifying:Suggested simplification
if (open) { // Opening animation // First set height to 0 to ensure proper animation start state // Only if we are not already open/opening - if (height === 0 || height === undefined) { + if (height === undefined) { setHeight(0); }src/components/ui/Steps/fragments/StepItem.tsx (1)
20-24: Props spreading after data attributes allows unintended overrides.Spreading
{...props}afterdata-stateanddata-valuemeans consumers can accidentally override the component's computed state. Consider spreading props before the data attributes to preserve the internal state logic.Proposed fix
<div className={clsx(`${rootClass}-item`, className)} + {...props} data-state={state} data-value={value} - {...props} >src/components/ui/Steps/fragments/StepRoot.tsx (1)
2-2: Remove unuseduseStateimport.
useStateis no longer used after switching touseControllableState.Proposed fix
-import React, { useState } from 'react'; +import React from 'react';src/components/ui/Steps/tests/Steps.test.tsx (1)
6-41: Good test coverage for core functionality.The tests effectively verify:
- Data-state derivation logic (completed/active/inactive) based on step values
- Uncontrolled mode via
defaultValue- Controlled mode via
valueprop- Custom root class composition
Consider adding tests for:
onValueChangecallback invocation when step changesorientationprop anddata-orientationattribute on the root elementsrc/components/ui/ScrollArea/context/ScrollAreaContext.tsx (1)
11-11: Avoid usinganyfor coordinate parameters.
clientXandclientYshould be typed asnumberinstead ofanyfor type safety.Suggested fix
- handleScrollbarClick?: (e : { clientX?: any; clientY?: any; orientation: 'vertical' | 'horizontal' }) => void; + handleScrollbarClick?: (e : { clientX?: number; clientY?: number; orientation: 'vertical' | 'horizontal' }) => void;src/components/ui/ScrollArea/fragments/ScrollAreaThumb.tsx (1)
67-78: Consider attaching global listeners only during drag.Global
mousemoveandmouseuplisteners are always attached, which adds overhead when many ScrollArea instances exist. Attaching them only instartDragand removing them instopDragwould be more efficient.Suggested optimization
+ React.useEffect(() => { + return () => { + // Cleanup on unmount in case drag is in progress + document.body.style.cursor = ''; + document.body.style.userSelect = ''; + }; + }, []); - React.useEffect(() => { - const handleMouseMove = (e: MouseEvent) => handleDrag(e); - const handleMouseUp = () => stopDrag(); - - document.addEventListener('mousemove', handleMouseMove); - document.addEventListener('mouseup', handleMouseUp); - - return () => { - document.removeEventListener('mousemove', handleMouseMove); - document.removeEventListener('mouseup', handleMouseUp); - }; - }, [handleDrag, stopDrag]);Then in
startDrag:const startDrag = useCallback((e: React.MouseEvent) => { // ... existing code ... const handleMouseMove = (e: MouseEvent) => handleDrag(e); const handleMouseUp = () => { stopDrag(); document.removeEventListener('mousemove', handleMouseMove); document.removeEventListener('mouseup', handleMouseUp); }; document.addEventListener('mousemove', handleMouseMove); document.addEventListener('mouseup', handleMouseUp); }, [scrollAreaViewportRef, handleDrag, stopDrag]);src/components/ui/ScrollArea/fragments/ScrollAreaRoot.tsx (3)
32-36: Consider using a type-safe ref merging utility.The ref merging logic works but uses
anycasts. A cleaner approach would be to use a utility function or a library like@radix-ui/react-compose-refs.♻️ Suggested improvement
+import { useCallback } from 'react'; + +const mergeRefs = <T,>(...refs: (React.Ref<T> | undefined)[]) => { + return (node: T | null) => { + refs.forEach((ref) => { + if (typeof ref === 'function') { + ref(node); + } else if (ref != null) { + (ref as React.MutableRefObject<T | null>).current = node; + } + }); + }; +}; - const mergedRootRef = (node: HTMLDivElement | null) => { - (internalRootRef as any).current = node; - if (typeof ref === 'function') ref(node); - else if (ref) (ref as any).current = node; - }; + const mergedRootRef = mergeRefs(internalRootRef, ref);
38-41: Refs in dependency array are misleading.Refs are stable and don't trigger re-renders. This effect will only run once on mount, which may be intentional. If so, use an empty dependency array to clarify intent.
♻️ Suggested fix
useEffect(() => { initializeThumbSizes(); - }, [scrollYThumbRef, scrollXThumbRef, scrollAreaViewportRef]); + }, []);
191-211: Consider memoizing the context value to prevent unnecessary re-renders.The context value object is recreated on every render, which can cause all consumers to re-render even when values haven't changed.
♻️ Suggested improvement
+ const contextValue = React.useMemo(() => ({ + rootClass, + scrollYThumbRef, + scrollXThumbRef, + scrollAreaViewportRef, + handleScroll, + handleScrollbarClick, + type, + rootRef: internalRootRef + }), [rootClass, type, handleScroll, handleScrollbarClick]); + return ( - <ScrollAreaContext.Provider value={{ - rootClass, - scrollYThumbRef, - scrollXThumbRef, - scrollAreaViewportRef, - handleScroll, - handleScrollbarClick, - type, - rootRef: internalRootRef - }}> + <ScrollAreaContext.Provider value={contextValue}>Note: You'll also need to wrap
handleScrollandhandleScrollbarClickinuseCallbackfor this optimization to be effective.src/components/ui/AlertDialog/tests/AlertDialog.a11y.test.tsx (1)
112-113: Multiple focus trap assertions disabled - consider tracking resolution.Disabling these focus-related assertions is understandable given jsdom limitations, and other critical a11y tests (axe violations, data-state attributes, some focus scenarios) remain active. However, focus trapping is a core accessibility requirement for alert dialogs.
Consider creating a tracking issue to:
- Document which assertions are disabled and why
- Explore alternatives (e.g., Playwright/Cypress for focus tests in a real browser)
- Re-enable once jsdom or the underlying library improves
This ensures these gaps aren't forgotten.
Would you like me to open an issue to track re-enabling these focus trap assertions?
Also applies to: 191-202
src/components/ui/Dialog/tests/Dialog.portal.test.tsx (1)
47-48: Consider tracking these FIXMEs for future resolution.The disabled focus restoration assertions are reasonable given jsdom limitations. To ensure these don't get lost, consider opening an issue to investigate alternatives (e.g., testing with a more complete DOM implementation or mocking focus behavior).
Would you like me to open an issue to track the jsdom focus restoration flakiness?
Also applies to: 55-56
src/setupTests.ts (1)
46-61: Polyfills are appropriate for jsdom environment.The
ResizeObserverandPointerEventpolyfills address known jsdom limitations. The window existence check provides SSR safety.Minor type safety improvement could replace
@ts-ignorewith explicit typing:♻️ Optional type-safe alternative
// PointerEvent - // `@ts-ignore` - if (!window.PointerEvent) { - // `@ts-ignore` - window.PointerEvent = MouseEvent; - } + if (!('PointerEvent' in window)) { + (window as unknown as { PointerEvent: typeof MouseEvent }).PointerEvent = MouseEvent; + }src/components/ui/Accordion/tests/Accordion.dynamic.test.tsx (1)
45-55: Consider usingwaitFororfindByfor more robust async handling.The manual
setTimeoutapproach works but is less idiomatic than RTL's built-in async utilities, which handle timing more reliably:Suggested improvement using waitFor
- // Wait for items to update - await act(async () => { - await new Promise(resolve => setTimeout(resolve, 100)); - }); - - // The content height should have updated or at least not be broken. - // Since we mocked ResizeObserver, we can't easily test the actual offsetHeight - // in JSDOM, but we can verify that the component didn't crash and the new items are rendered. - expect(screen.getByText('Item 1')).toBeInTheDocument(); - expect(screen.getByText('Item 2')).toBeInTheDocument(); - expect(screen.getByText('Item 3')).toBeInTheDocument(); + // Wait for dynamic items to render + expect(await screen.findByText('Item 2')).toBeInTheDocument(); + expect(screen.getByText('Item 1')).toBeInTheDocument(); + expect(screen.getByText('Item 3')).toBeInTheDocument();src/core/primitives/Combobox/fragments/ComboboxPrimitiveItem.tsx (1)
98-102: Potential stale closure in selectedItemRef effect.The effect depends on
isSelectedandhasSearch, butitemRef.currentmay not be set when this effect runs on initial mount. Consider addingitemRefto dependencies or using a callback ref pattern.src/core/primitives/Combobox/fragments/ComboboxPrimitiveRoot.tsx (1)
147-190: Context value memoization looks correct.The expanded dependency list properly includes the new search-related state values. However, note that
refs,getFloatingProps,getReferenceProps,getItemProps, andfloatingStylesare recreated on each render by Floating UI, which may cause unnecessary context updates.src/core/primitives/Combobox/fragments/ComboboxPrimitiveGroup.tsx (1)
51-51: Style spread order may unintentionally override hide behavior.The current spread order
{ display: shouldHide ? 'none' : undefined, ...props.style }allows consumer-provided styles to override thedisplay: nonewhen the group should be hidden.If this is unintentional, swap the order:
Suggested fix
- style={{ display: shouldHide ? 'none' : undefined, ...props.style }} + style={{ ...props.style, display: shouldHide ? 'none' : undefined }}Alternatively, if consumers should be able to override the hide behavior, this is fine as-is.
src/components/ui/Slider/fragments/SliderRoot.tsx (1)
54-60: PreferuseCallbackfor ref callbacks.While
useMemoworks,useCallbackis the idiomatic choice for memoized callbacks and makes the intent clearer.Suggested refactor
- const mergedRef = React.useMemo(() => { - return (node: HTMLDivElement | null) => { + const mergedRef = React.useCallback((node: HTMLDivElement | null) => { (internalRef as any).current = node; if (typeof ref === 'function') ref(node); else if (ref) (ref as any).current = node; - }; - }, [ref]); + }, [ref]);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/components/ui/Slider/tests/__snapshots__/Slider.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (40)
src/components/ui/Accordion/fragments/AccordionHeader.tsxsrc/components/ui/Accordion/tests/Accordion.dynamic.test.tsxsrc/components/ui/Accordion/tests/Accordion.test.tsxsrc/components/ui/AlertDialog/tests/AlertDialog.a11y.test.tsxsrc/components/ui/Combobox/tests/Combobox.filtering.test.tsxsrc/components/ui/Combobox/tests/Combobox.simple.test.tsxsrc/components/ui/Dialog/tests/Dialog.portal.test.tsxsrc/components/ui/ScrollArea/context/ScrollAreaContext.tsxsrc/components/ui/ScrollArea/fragments/ScrollAreaCorner.tsxsrc/components/ui/ScrollArea/fragments/ScrollAreaRoot.tsxsrc/components/ui/ScrollArea/fragments/ScrollAreaScrollbar.tsxsrc/components/ui/ScrollArea/fragments/ScrollAreaThumb.tsxsrc/components/ui/ScrollArea/stories/ScrollArea.stories.tsxsrc/components/ui/Slider/Slider.tsxsrc/components/ui/Slider/context/SliderContext.tsxsrc/components/ui/Slider/fragments/SliderRange.tsxsrc/components/ui/Slider/fragments/SliderRoot.tsxsrc/components/ui/Slider/fragments/SliderThumb.tsxsrc/components/ui/Slider/tests/Slider.interaction.test.tsxsrc/components/ui/Slider/tests/Slider.range.test.tsxsrc/components/ui/Steps/fragments/StepItem.tsxsrc/components/ui/Steps/fragments/StepRoot.tsxsrc/components/ui/Steps/tests/Steps.test.tsxsrc/components/ui/Tooltip/fragments/TooltipContent.tsxsrc/core/primitives/Collapsible/fragments/CollapsiblePrimitiveContent.tsxsrc/core/primitives/Combobox/contexts/ComboboxGroupContext.tsxsrc/core/primitives/Combobox/contexts/ComboboxPrimitiveContext.tsxsrc/core/primitives/Combobox/fragments/ComboboxPrimitiveGroup.tsxsrc/core/primitives/Combobox/fragments/ComboboxPrimitiveItem.tsxsrc/core/primitives/Combobox/fragments/ComboboxPrimitiveRoot.tsxsrc/core/primitives/Combobox/fragments/ComboboxPrimitiveSearch.tsxsrc/core/primitives/Dialog/fragments/DialogPrimitiveContent.tsxsrc/setupTests.tssrc/test-utils/portal.tsstyles/themes/components/combobox.scssstyles/themes/components/menubar.scssstyles/themes/components/scroll-area.scssstyles/themes/components/steps.scssstyles/themes/components/tooltip.scssstyles/themes/default.scss
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-04-07T04:38:34.864Z
Learnt from: kotAPI
Repo: rad-ui/ui PR: 1031
File: src/components/ui/Accordion/fragments/AccordionRoot.tsx:41-44
Timestamp: 2025-04-07T04:38:34.864Z
Learning: The Accordion component in rad-ui/ui supports both controlled and uncontrolled modes through props like `value`, `defaultValue`, and `onValueChange`. When implementing controlled components, remember to: 1) Initialize state from defaultValue, 2) Update internal state when value changes (controlled mode), 3) Call onValueChange callback, and 4) Prevent internal state updates when in controlled mode.
Applied to files:
src/components/ui/Slider/fragments/SliderRange.tsxsrc/components/ui/Slider/fragments/SliderThumb.tsxsrc/components/ui/Slider/fragments/SliderRoot.tsxsrc/components/ui/Steps/fragments/StepRoot.tsxsrc/components/ui/Slider/Slider.tsx
📚 Learning: 2024-12-12T08:34:33.079Z
Learnt from: decipher-cs
Repo: rad-ui/ui PR: 417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
Applied to files:
src/components/ui/Combobox/tests/Combobox.filtering.test.tsxsrc/components/ui/AlertDialog/tests/AlertDialog.a11y.test.tsxstyles/themes/components/combobox.scsssrc/components/ui/ScrollArea/stories/ScrollArea.stories.tsxsrc/components/ui/Dialog/tests/Dialog.portal.test.tsxsrc/components/ui/Tooltip/fragments/TooltipContent.tsx
📚 Learning: 2025-07-18T16:43:26.175Z
Learnt from: GoldGroove06
Repo: rad-ui/ui PR: 1215
File: src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveTrigger.tsx:25-27
Timestamp: 2025-07-18T16:43:26.175Z
Learning: In the CheckboxGroupPrimitiveTrigger component (`src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveTrigger.tsx`), the component uses two separate useEffect hooks with different purposes: the first useEffect (lines 25-27) with empty dependency array `[]` is meant to set the initial state only once on mount by syncing with the group's checked values, while the second useEffect (lines 28-34) handles ongoing state updates by propagating local `isChecked` changes back to the group's `checkedValues`. This pattern prevents infinite loops while maintaining proper synchronization.
Applied to files:
src/components/ui/Combobox/tests/Combobox.filtering.test.tsxsrc/core/primitives/Combobox/fragments/ComboboxPrimitiveRoot.tsxsrc/core/primitives/Combobox/fragments/ComboboxPrimitiveGroup.tsxsrc/core/primitives/Combobox/fragments/ComboboxPrimitiveItem.tsx
📚 Learning: 2024-11-24T06:43:42.194Z
Learnt from: kotAPI
Repo: rad-ui/ui PR: 576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
Applied to files:
src/components/ui/Slider/fragments/SliderRoot.tsxsrc/components/ui/Steps/fragments/StepRoot.tsx
🧬 Code graph analysis (15)
src/components/ui/Slider/fragments/SliderRange.tsx (1)
src/components/ui/Slider/context/SliderContext.tsx (1)
SliderContext(20-35)
src/components/ui/Accordion/tests/Accordion.dynamic.test.tsx (2)
docs/app/docs/components/accordion/docs/anatomy.tsx (1)
Accordion(3-14)rollup.config.cjs (1)
resolve(1-1)
src/components/ui/ScrollArea/fragments/ScrollAreaThumb.tsx (1)
src/components/ui/ScrollArea/context/ScrollAreaContext.tsx (1)
ScrollAreaContext(16-19)
src/components/ui/ScrollArea/fragments/ScrollAreaCorner.tsx (1)
src/components/ui/ScrollArea/context/ScrollAreaContext.tsx (1)
ScrollAreaContext(16-19)
src/components/ui/Slider/fragments/SliderThumb.tsx (2)
src/components/ui/Slider/context/SliderContext.tsx (1)
SliderContext(20-35)scripts/generate-exports.cjs (1)
name(74-74)
src/components/ui/Steps/fragments/StepItem.tsx (1)
src/components/ui/Steps/context/StepsContext.tsx (1)
useStepsContext(14-20)
src/components/ui/Dialog/tests/Dialog.portal.test.tsx (1)
test-utils/index.ts (1)
axe(56-62)
src/components/ui/ScrollArea/fragments/ScrollAreaRoot.tsx (1)
src/components/ui/ScrollArea/context/ScrollAreaContext.tsx (1)
ScrollAreaContext(16-19)
src/core/primitives/Combobox/fragments/ComboboxPrimitiveGroup.tsx (2)
src/core/primitives/Combobox/contexts/ComboboxPrimitiveContext.tsx (1)
ComboboxPrimitiveContext(37-37)src/core/primitives/Combobox/contexts/ComboboxGroupContext.tsx (1)
ComboboxGroupContext(7-7)
src/components/ui/Accordion/fragments/AccordionHeader.tsx (2)
src/components/ui/Accordion/Accordion.tsx (1)
AccordionHeaderProps(17-17)src/components/ui/Accordion/contexts/AccordionContext.tsx (1)
AccordionContext(13-21)
src/components/ui/Slider/fragments/SliderRoot.tsx (1)
src/core/hooks/useControllableState/index.tsx (1)
useControllableState(15-78)
src/components/ui/ScrollArea/fragments/ScrollAreaScrollbar.tsx (1)
src/components/ui/ScrollArea/context/ScrollAreaContext.tsx (1)
ScrollAreaContext(16-19)
src/core/primitives/Combobox/fragments/ComboboxPrimitiveItem.tsx (1)
src/core/primitives/Combobox/contexts/ComboboxGroupContext.tsx (1)
useComboboxGroupContext(9-9)
src/core/primitives/Dialog/fragments/DialogPrimitiveContent.tsx (3)
docs/components/Main/NavBar/index.js (1)
useContext(11-11)src/core/primitives/Dialog/context/DialogPrimitiveContext.tsx (1)
DialogPrimitiveContext(19-33)src/examples/FloatingUI/useInteractions/UseInteractionsExample.js (2)
isOpen(6-6)role(16-18)
src/components/ui/Steps/fragments/StepRoot.tsx (2)
src/core/hooks/useControllableState/index.tsx (1)
useControllableState(15-78)src/core/index.ts (1)
customClassSwitcher(4-4)
🪛 Biome (2.1.2)
src/core/primitives/Combobox/fragments/ComboboxPrimitiveItem.tsx
[error] 44-44: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 45-45: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 54-54: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 57-57: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 64-64: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 72-72: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 98-98: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 106-106: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🪛 GitHub Actions: Lint
styles/themes/components/steps.scss
[error] 58-58: stylelint: 'no-descending-specificity' has already been disabled. CssSyntaxError
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: coverage
🔇 Additional comments (49)
styles/themes/components/tooltip.scss (1)
1-33: LGTM on the tooltip styling module.The separation of concerns between the floating element (positioning) and inner content (visual styling) is well-structured. The animation is smooth with appropriate
will-changehints.styles/themes/default.scss (1)
49-50: LGTM on the tooltip module import.The import correctly integrates the new dedicated tooltip styling module, replacing the legacy inline styles.
styles/themes/components/menubar.scss (3)
3-11: LGTM!The root container styling is clean and follows good practices with CSS custom properties for theming.
84-88: LGTM!Simple and clean separator styling.
90-99: LGTM!The slide-in animation is well-crafted with appropriate values for a smooth, subtle entrance effect.
src/components/ui/Steps/fragments/StepRoot.tsx (1)
20-35: Controllable state implementation looks correct.The use of
useControllableStatefollows the established pattern from learnings for supporting both controlled and uncontrolled modes. The hook handles the internal/external state synchronization, warnings for mode switching, and onChange callbacks appropriately. Thedata-orientationattribute is a good addition for styling and accessibility.src/components/ui/ScrollArea/fragments/ScrollAreaCorner.tsx (1)
3-12: LGTM!Clean implementation using context for consistent class naming. The
clsxcomposition and ref forwarding are correctly implemented.styles/themes/components/scroll-area.scss (1)
20-71: LGTM! Well-structured orientation-aware scrollbar styling.The implementation correctly handles both vertical and horizontal orientations with appropriate positioning. The 44px minimum hit area on the thumb pseudo-element aligns with touch accessibility guidelines.
src/components/ui/ScrollArea/context/ScrollAreaContext.tsx (1)
5-19: Context type and defaults are well-defined.The expanded context type with orientation support and new refs enables the dual-axis scrollbar functionality cleanly.
src/components/ui/ScrollArea/stories/ScrollArea.stories.tsx (1)
75-247: Good story coverage for the new ScrollArea features.The new stories comprehensively demonstrate all type variations (
auto,always,scroll,hover), both orientations, and composition with other components like Card.src/components/ui/ScrollArea/fragments/ScrollAreaThumb.tsx (2)
35-59: Drag handling logic is well-implemented.The orientation-aware drag calculation correctly computes scroll ratios and clamps values within bounds for both vertical and horizontal axes.
80-90: Type assertion for ref assignment is acceptable but worth noting.The cast from
RefObjecttoMutableRefObjectis a common pattern when combining context-provided refs with forwardRef. This works but relies on the context actually providing mutable refs.src/components/ui/ScrollArea/fragments/ScrollAreaRoot.tsx (3)
74-101: LGTM!The thumb sizing logic correctly handles both axes with appropriate minimum size constraints and safely updates the overflow state.
130-160: LGTM!The animated scrolling implementation with cubic easing is clean and handles both axes appropriately.
162-189: LGTM!The click-based scrolling logic correctly handles both orientations with appropriate bounds checking.
src/components/ui/ScrollArea/fragments/ScrollAreaScrollbar.tsx (5)
12-23: LGTM!The component signature and state initialization correctly set up the visibility management based on the
typeprop.
71-88: LGTM!The overflow detection correctly uses ResizeObserver and observes both the viewport and its children for dynamic content changes.
90-101: LGTM!The scroll continuation logic correctly determines whether the mouse position is still outside the thumb bounds based on orientation.
103-137: LGTM!The continuous scrolling implementation with initial click and delayed interval is well-structured with proper orientation handling.
139-149: LGTM!The cleanup function properly resets all scrolling state and clears the interval.
src/components/ui/Combobox/tests/Combobox.simple.test.tsx (1)
6-11: Minimal smoke test - consider expanding coverage.This test validates basic rendering, which is a reasonable starting point. Given the AI summary mentions other Combobox tests (filtering, dynamic) provide broader coverage, this simple test is acceptable as a quick sanity check.
Consider adding at least one interaction test (e.g., opening the dropdown) if not covered elsewhere to ensure the component integrates correctly.
src/core/primitives/Combobox/contexts/ComboboxPrimitiveContext.tsx (1)
32-34: LGTM!The new context properties (
search,setSearch,hiddenIndices) are well-typed and align with the search-driven filtering behavior described in the PR. The simplersetSearchsignature (vsReact.Dispatch) provides flexibility for controlled search implementations.src/core/primitives/Combobox/contexts/ComboboxGroupContext.tsx (1)
1-9: LGTM!Clean and well-typed context implementation. The
nulldefault value correctly signals that the context is optional, aligning with the hook returningnullwhen used outside a provider.src/core/primitives/Dialog/fragments/DialogPrimitiveContent.tsx (1)
32-37: VerifyinitialFocus={0}behavior with Floater.FocusManager.
initialFocus={0}sets focus to the first tabbable element. Confirm this is the intended behavior for all dialog use cases, as some dialogs may need focus on a specific element (e.g., a close button or primary action).src/components/ui/Dialog/tests/Dialog.portal.test.tsx (1)
103-109: Well-documented axe rule exclusion.The comment clearly explains why
aria-command-nameis disabled. Since this is an upstream Floating UI implementation detail (focus guards withrole="button"but no accessible name), this is a reasonable workaround.styles/themes/components/combobox.scss (1)
54-75: Good accessibility and animation patterns.The content styling properly handles visibility with
data-state, includes focus rings, and uses a smooth animation. The glass background effect withbackdrop-filterprovides a modern look.src/components/ui/Accordion/tests/Accordion.test.tsx (1)
47-47: LGTM! Type updates align with AccordionHeader rendering change.The ref type and assertion correctly reflect that
AccordionHeadernow renders an<h3>element instead of a<div>, improving semantic HTML structure for accessibility.Also applies to: 68-68
src/components/ui/Slider/tests/Slider.interaction.test.tsx (1)
67-67: Acceptable type narrowing for single-value test scenario.The cast
v as numberis appropriate here since this test explicitly uses a single-thumb slider. For production code, a runtime check would be preferable, but for test files this pattern is acceptable.src/components/ui/Combobox/tests/Combobox.filtering.test.tsx (2)
6-44: Good coverage of core filtering behavior.The test validates the primary filtering flow: initial visibility, search-based filtering, and group hiding. Consider adding tests for edge cases in a follow-up (e.g., clearing search, empty search string, case sensitivity).
37-42: The test assertions are correct and will not fail.The filtering implementation uses CSS
display: noneto hide items rather than removing them from the DOM (ComboboxPrimitiveItem.tsx line 110 and ComboboxPrimitiveGroup.tsx line 51). Because elements remain in the DOM when hidden,screen.queryByText()will successfully find them, andnot.toBeVisible()will correctly assert they are not visible. The test is safe as written.src/components/ui/Slider/context/SliderContext.tsx (1)
5-6: LGTM! Clean API expansion for range slider support.The type widening from
numbertonumber | number[]enables multi-thumb (range) sliders while maintaining backward compatibility with single-value usage. Default context values remain valid.src/components/ui/Accordion/tests/Accordion.dynamic.test.tsx (1)
24-35: Test structure looks good for validating dynamic content handling.The test properly exercises the accordion with dynamically growing content, verifies initial rendering from
defaultValue, and confirms the component doesn't crash when content updates. The inline comment acknowledging JSDOM limitations is helpful documentation.src/components/ui/Slider/tests/Slider.range.test.tsx (4)
6-8: LGTM on the PointerEvent polyfill.Standard pattern for enabling pointer events in jsdom test environments.
11-27: LGTM on the dual-thumb rendering test.Good coverage for verifying that both thumbs render with correct
aria-valuenowattributes.
29-61: Second assertion may fail due to stale component state.The test fires two
pointerDownevents sequentially without re-rendering. After the first click atclientX: 10, the component's internal state updates to[10, 80]. However, the second assertion at line 60 expects[10, 90], which assumes the first update persisted. SincedefaultValueis used (uncontrolled mode) andonValueChangeis mocked, the component's internal state does update, but verify that the mock captures independent calls rather than cumulative state.If the intent is to test cumulative updates, consider using controlled mode with state, or use
toHaveBeenLastCalledWithfor the second assertion.
63-77: LGTM on the range rendering test.Correctly validates the visual positioning of the range element between two thumbs.
src/core/primitives/Combobox/fragments/ComboboxPrimitiveSearch.tsx (2)
30-39: LGTM on the lifecycle management.Properly sets
hasSearchon mount, resetsactiveIndex, and cleans up bothhasSearchandsearchon unmount.
44-65: LGTM on the controlled input binding.The input is correctly bound to context-provided
searchstate with proper event handling.src/components/ui/Slider/fragments/SliderThumb.tsx (2)
64-72: LGTM on the array value update logic.The approach of updating the specific index without sorting is a valid design choice to prevent thumb hopping during keyboard navigation. The comment documents the intent well.
119-124: LGTM on the hidden input handling.The naming convention
name[index]aligns with standard form array field patterns for multi-thumb values.src/core/primitives/Combobox/fragments/ComboboxPrimitiveItem.tsx (1)
72-95: Disabled indices effect has inverted early-return logic.Lines 75-76 return
prevunchanged when the item is disabled and already in the list, or when enabled and not in the list. This is correct for avoiding no-op state updates.However, verify this doesn't cause issues when items re-render with the same disabled state - the cleanup function (lines 87-94) will always try to remove the index on unmount regardless of disabled state, which is correct.
src/core/primitives/Combobox/fragments/ComboboxPrimitiveRoot.tsx (1)
60-63: LGTM on totalDisabledIndices computation.Correctly merges disabled and hidden indices into a sorted, deduplicated array for navigation exclusion.
src/components/ui/Slider/fragments/SliderRange.tsx (1)
36-55: LGTM!The orientation-aware styling correctly positions the range indicator using percentage-based values for both horizontal and vertical modes.
src/components/ui/Slider/Slider.tsx (2)
12-27: LGTM!The type definitions correctly support both single-value and array-based (range) modes. The
Omiton line 27 properly prevents conflicts between the custom value props and the native div element props.
52-72: Default implementation only supports single-value mode.The default render structure creates a single
Slider.Thumb, but the props now acceptnumber[]for range mode. If a user passesdefaultValue={[20, 80]}to this component, only one thumb will render, causing unexpected behavior.Consider either:
- Dynamically rendering thumbs based on the value array length, or
- Adding a runtime warning when array values are passed to the simple
SlidercomponentIs the expectation that users must use
Slider.Rootwith manually composedSlider.Thumbelements for range mode? If so, documenting this distinction would help prevent confusion.src/components/ui/Slider/fragments/SliderRoot.tsx (3)
155-170: LGTM!The context values correctly expose all necessary state and callbacks for child components. The value/setValue pair properly supports both single and array modes as defined in the context type.
172-188: LGTM!The root element correctly uses the merged ref, applies orientation and disabled data attributes, and spreads remaining props for extensibility.
112-120: Thedata-indexattribute is already properly set bySliderThumbon line 103 ofSliderThumb.tsxasdata-index={index}, which correctly supports both single-value and multi-thumb scenarios. No changes needed.src/core/primitives/Combobox/fragments/ComboboxPrimitiveGroup.tsx (1)
17-38: The cleanup safeguard prevents the race condition described.The
registerItemfunction does call on every visibility change (via theisHiddendependency in ComboboxPrimitiveItem's useEffect), but the cleanup function at line 32 includes a safeguard check:if (!prev.has(id)) return prev;. This prevents the stale removal scenario you described—if cleanup runs at any point, it safely returns without removing the item if it's no longer in the Set.React's effect cleanup semantics ensure the old cleanup runs before the new effect executes, and even if ordering were different, the safeguard makes the operation idempotent. The code is safe as written.
Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| const handleScroll = () => { | ||
| // The full height of all the content inside the scrollable element [Visible Area Height] | ||
| const scrollAreaContainerHeight = scrollAreaViewportRef.current?.clientHeight || 0; | ||
|
|
||
| // The full height of all the content inside the scrollable element [Total Content Height] | ||
| const scrollAreaHeight = scrollAreaViewportRef.current?.scrollHeight || 0; | ||
|
|
||
| // The current scroll position of the scrollable element [Scroll Position] | ||
| const scrollTopPosition = scrollAreaViewportRef.current?.scrollTop || 0; | ||
| if (!scrollAreaViewportRef.current) return; | ||
| const viewport = scrollAreaViewportRef.current; | ||
|
|
||
| // The height of the scroll thumb [Scroll Thumb Height] | ||
| const scrollThumbHeight = scrollXThumbRef.current?.clientHeight || 0; | ||
| // Vertical | ||
| if (scrollYThumbRef.current) { | ||
| const viewportHeight = viewport.clientHeight; | ||
| const contentHeight = viewport.scrollHeight; | ||
| const scrollTop = viewport.scrollTop; | ||
| const thumbHeight = scrollYThumbRef.current.clientHeight; | ||
|
|
||
| const scrollThumbPosition = (scrollTopPosition / (scrollAreaHeight - scrollAreaContainerHeight)) * (scrollAreaContainerHeight - scrollThumbHeight); | ||
| const thumbPosition = (scrollTop / (contentHeight - viewportHeight)) * (viewportHeight - thumbHeight); | ||
| scrollYThumbRef.current.style.top = `${thumbPosition}px`; | ||
| } | ||
|
|
||
| // Horizontal | ||
| if (scrollXThumbRef.current) { | ||
| scrollXThumbRef.current.style.top = `${scrollThumbPosition}px`; | ||
| const viewportWidth = viewport.clientWidth; | ||
| const contentWidth = viewport.scrollWidth; | ||
| const scrollLeft = viewport.scrollLeft; | ||
| const thumbWidth = scrollXThumbRef.current.clientWidth; | ||
|
|
||
| const thumbPosition = (scrollLeft / (contentWidth - viewportWidth)) * (viewportWidth - thumbWidth); | ||
| scrollXThumbRef.current.style.left = `${thumbPosition}px`; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Guard against division by zero when content fits exactly.
If contentHeight === viewportHeight or contentWidth === viewportWidth, the division results in NaN, setting invalid CSS values like NaNpx.
🐛 Suggested fix
const handleScroll = () => {
if (!scrollAreaViewportRef.current) return;
const viewport = scrollAreaViewportRef.current;
// Vertical
if (scrollYThumbRef.current) {
const viewportHeight = viewport.clientHeight;
const contentHeight = viewport.scrollHeight;
const scrollTop = viewport.scrollTop;
const thumbHeight = scrollYThumbRef.current.clientHeight;
+ const scrollableHeight = contentHeight - viewportHeight;
- const thumbPosition = (scrollTop / (contentHeight - viewportHeight)) * (viewportHeight - thumbHeight);
+ const thumbPosition = scrollableHeight > 0
+ ? (scrollTop / scrollableHeight) * (viewportHeight - thumbHeight)
+ : 0;
scrollYThumbRef.current.style.top = `${thumbPosition}px`;
}
// Horizontal
if (scrollXThumbRef.current) {
const viewportWidth = viewport.clientWidth;
const contentWidth = viewport.scrollWidth;
const scrollLeft = viewport.scrollLeft;
const thumbWidth = scrollXThumbRef.current.clientWidth;
+ const scrollableWidth = contentWidth - viewportWidth;
- const thumbPosition = (scrollLeft / (contentWidth - viewportWidth)) * (viewportWidth - thumbWidth);
+ const thumbPosition = scrollableWidth > 0
+ ? (scrollLeft / scrollableWidth) * (viewportWidth - thumbWidth)
+ : 0;
scrollXThumbRef.current.style.left = `${thumbPosition}px`;
}
};📝 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.
| const handleScroll = () => { | |
| // The full height of all the content inside the scrollable element [Visible Area Height] | |
| const scrollAreaContainerHeight = scrollAreaViewportRef.current?.clientHeight || 0; | |
| // The full height of all the content inside the scrollable element [Total Content Height] | |
| const scrollAreaHeight = scrollAreaViewportRef.current?.scrollHeight || 0; | |
| // The current scroll position of the scrollable element [Scroll Position] | |
| const scrollTopPosition = scrollAreaViewportRef.current?.scrollTop || 0; | |
| if (!scrollAreaViewportRef.current) return; | |
| const viewport = scrollAreaViewportRef.current; | |
| // The height of the scroll thumb [Scroll Thumb Height] | |
| const scrollThumbHeight = scrollXThumbRef.current?.clientHeight || 0; | |
| // Vertical | |
| if (scrollYThumbRef.current) { | |
| const viewportHeight = viewport.clientHeight; | |
| const contentHeight = viewport.scrollHeight; | |
| const scrollTop = viewport.scrollTop; | |
| const thumbHeight = scrollYThumbRef.current.clientHeight; | |
| const scrollThumbPosition = (scrollTopPosition / (scrollAreaHeight - scrollAreaContainerHeight)) * (scrollAreaContainerHeight - scrollThumbHeight); | |
| const thumbPosition = (scrollTop / (contentHeight - viewportHeight)) * (viewportHeight - thumbHeight); | |
| scrollYThumbRef.current.style.top = `${thumbPosition}px`; | |
| } | |
| // Horizontal | |
| if (scrollXThumbRef.current) { | |
| scrollXThumbRef.current.style.top = `${scrollThumbPosition}px`; | |
| const viewportWidth = viewport.clientWidth; | |
| const contentWidth = viewport.scrollWidth; | |
| const scrollLeft = viewport.scrollLeft; | |
| const thumbWidth = scrollXThumbRef.current.clientWidth; | |
| const thumbPosition = (scrollLeft / (contentWidth - viewportWidth)) * (viewportWidth - thumbWidth); | |
| scrollXThumbRef.current.style.left = `${thumbPosition}px`; | |
| } | |
| }; | |
| const handleScroll = () => { | |
| if (!scrollAreaViewportRef.current) return; | |
| const viewport = scrollAreaViewportRef.current; | |
| // Vertical | |
| if (scrollYThumbRef.current) { | |
| const viewportHeight = viewport.clientHeight; | |
| const contentHeight = viewport.scrollHeight; | |
| const scrollTop = viewport.scrollTop; | |
| const thumbHeight = scrollYThumbRef.current.clientHeight; | |
| const scrollableHeight = contentHeight - viewportHeight; | |
| const thumbPosition = scrollableHeight > 0 | |
| ? (scrollTop / scrollableHeight) * (viewportHeight - thumbHeight) | |
| : 0; | |
| scrollYThumbRef.current.style.top = `${thumbPosition}px`; | |
| } | |
| // Horizontal | |
| if (scrollXThumbRef.current) { | |
| const viewportWidth = viewport.clientWidth; | |
| const contentWidth = viewport.scrollWidth; | |
| const scrollLeft = viewport.scrollLeft; | |
| const thumbWidth = scrollXThumbRef.current.clientWidth; | |
| const scrollableWidth = contentWidth - viewportWidth; | |
| const thumbPosition = scrollableWidth > 0 | |
| ? (scrollLeft / scrollableWidth) * (viewportWidth - thumbWidth) | |
| : 0; | |
| scrollXThumbRef.current.style.left = `${thumbPosition}px`; | |
| } | |
| }; |
| }, []); | ||
|
|
||
| // Attach global mouse listeners only while actively scrolling | ||
| const shouldRender = type === 'always' || (type === 'auto' && isOverflowing) || (isOverflowing && visible); |
There was a problem hiding this comment.
Remove unused variable shouldRender.
This variable is defined but never used. Line 179 defines isVisible with identical logic, which is the one actually used.
🧹 Suggested fix
- const shouldRender = type === 'always' || (type === 'auto' && isOverflowing) || (isOverflowing && visible);📝 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.
| const shouldRender = type === 'always' || (type === 'auto' && isOverflowing) || (isOverflowing && visible); |
🤖 Prompt for AI Agents
In `@src/components/ui/ScrollArea/fragments/ScrollAreaScrollbar.tsx` at line 151,
The variable shouldRender is declared with the same logic as isVisible but never
used; remove the unused declaration of shouldRender to avoid dead code and keep
only isVisible (used later). Locate the const shouldRender = ... line and delete
it, ensuring no other references to shouldRender remain; if any exist, replace
them with isVisible.
| <Primitive.div | ||
| ref={mergedRef} | ||
| asChild={asChild} | ||
| {...getFloatingProps()} | ||
| style={{ outline: 'none', ...props.style }} | ||
| role={role} | ||
| aria-labelledby={isOpen ? ariaLabelledBy : undefined} | ||
| aria-describedby={isOpen ? ariaDescribedBy : undefined} | ||
| data-state={dataState} | ||
| aria-modal={ariaModal} | ||
| {...props} | ||
| > |
There was a problem hiding this comment.
Style prop override bug.
The style prop is merged on line 42 to include outline: 'none', but then {...props} on line 48 spreads the original style again, overwriting the merged value. Since style isn't destructured from the component props, it remains in ...props.
🐛 Proposed fix
Either destructure style from props:
const DialogPrimitiveContent = forwardRef<HTMLDivElement, DialogPrimitiveContentProps>(({
children,
asChild = false,
forceMount = false,
role = 'dialog',
'aria-modal': ariaModal = true,
'aria-labelledby': ariaLabelledBy,
'aria-describedby': ariaDescribedBy,
+ style,
...props
}, ref) => {Then update line 42:
- style={{ outline: 'none', ...props.style }}
+ style={{ outline: 'none', ...style }}Or spread props before the inline style:
<Primitive.div
ref={mergedRef}
asChild={asChild}
{...getFloatingProps()}
- style={{ outline: 'none', ...props.style }}
+ {...props}
+ style={{ outline: 'none', ...props.style }}
role={role}
aria-labelledby={isOpen ? ariaLabelledBy : undefined}
aria-describedby={isOpen ? ariaDescribedBy : undefined}
data-state={dataState}
aria-modal={ariaModal}
- {...props}
>📝 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.
| <Primitive.div | |
| ref={mergedRef} | |
| asChild={asChild} | |
| {...getFloatingProps()} | |
| style={{ outline: 'none', ...props.style }} | |
| role={role} | |
| aria-labelledby={isOpen ? ariaLabelledBy : undefined} | |
| aria-describedby={isOpen ? ariaDescribedBy : undefined} | |
| data-state={dataState} | |
| aria-modal={ariaModal} | |
| {...props} | |
| > | |
| <Primitive.div | |
| ref={mergedRef} | |
| asChild={asChild} | |
| {...getFloatingProps()} | |
| {...props} | |
| style={{ outline: 'none', ...props.style }} | |
| role={role} | |
| aria-labelledby={isOpen ? ariaLabelledBy : undefined} | |
| aria-describedby={isOpen ? ariaDescribedBy : undefined} | |
| data-state={dataState} | |
| aria-modal={ariaModal} | |
| > |
🤖 Prompt for AI Agents
In `@src/core/primitives/Dialog/fragments/DialogPrimitiveContent.tsx` around lines
38 - 49, The inline style adding outline: 'none' is being overwritten because
props (which still contains style) is spread after the inline style in
Primitive.div; fix by removing style from the spread so the merged style takes
effect: destructure style from the component props (e.g., const { style, ...rest
} = props or similar) and then pass the merged style as style={{ outline:
'none', ...style }} while spreading rest (not props), or alternatively spread
{...props} before the inline style so the inline style wins; update the
Primitive.div usage to reference mergedRef, getFloatingProps(), role, aria
attrs, and spread the corrected props/rest accordingly.
| // FIXME: Focus trap navigation is flaky in jsdom environment | ||
| // expect(document.activeElement).toBe(last); | ||
|
|
||
| last.focus(); | ||
| await user.tab(); | ||
| expect(document.activeElement).toBe(first); | ||
| // expect(document.activeElement).toBe(first); |
There was a problem hiding this comment.
Function name no longer reflects behavior.
With both assertions commented out, assertFocusTrap no longer actually asserts anything about the focus trap. It performs tab navigation but doesn't validate the results, which could mask real focus trap regressions.
Consider one of these approaches:
- Rename to
exerciseFocusTrapto clarify it only performs navigation without validation - Add a console warning that focus trap assertions are skipped in jsdom
- Create a tracking issue to fix the underlying jsdom limitation
🤖 Prompt for AI Agents
In `@src/test-utils/portal.ts` around lines 34 - 39, The function assertFocusTrap
no longer asserts anything because both focus assertions are commented out;
either rename assertFocusTrap to exerciseFocusTrap to reflect that it only
performs tab navigation, or restore validation by re-enabling the expectations
(document.activeElement checks) and guard them with a jsdom-detection branch
that logs a console warning when assertions are skipped; update all references
to assertFocusTrap accordingly and consider adding/linking a tracking issue if
leaving assertions disabled.
| @@ -1,213 +1,152 @@ | |||
| @use 'button'; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if 'button' namespace or any button variables/mixins are used in this file
rg -n 'button\.' styles/themes/components/combobox.scssRepository: rad-ui/ui
Length of output: 35
🏁 Script executed:
# Also read the entire combobox.scss file to verify
wc -l styles/themes/components/combobox.scssRepository: rad-ui/ui
Length of output: 95
🏁 Script executed:
# Read the full file content
cat -n styles/themes/components/combobox.scssRepository: rad-ui/ui
Length of output: 4705
Remove unused @use 'button' import.
The button module is imported on line 1 but is not referenced anywhere in this file. It can be safely removed.
♻️ Proposed fix
-@use 'button';
-
.rad-ui-combobox-root {📝 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.
| @use 'button'; | |
| .rad-ui-combobox-root { |
🤖 Prompt for AI Agents
In `@styles/themes/components/combobox.scss` at line 1, Remove the unused SCSS
module import by deleting the line containing `@use` 'button'; in this file (the
import is never referenced anywhere in styles/themes/components/combobox.scss),
and run the style linter/formatter afterwards to ensure no leftover unused
import warnings remain.
| &[data-status="active"] { | ||
| .rad-ui-steps-bubble { | ||
| border-color: var(--rad-ui-color-accent-600); | ||
| background-color: var(--rad-ui-color-accent-50); | ||
| color: var(--rad-ui-color-accent-700); | ||
| box-shadow: 0 0 0 4px var(--rad-ui-color-accent-100); | ||
| } | ||
| } | ||
|
|
||
| &[data-status="complete"] { | ||
| .rad-ui-steps-bubble { | ||
| border-color: var(--rad-ui-color-accent-600); | ||
| background-color: var(--rad-ui-color-accent-600); | ||
| color: white; | ||
| } | ||
|
|
||
| // stylelint-disable-next-line no-descending-specificity | ||
| .rad-ui-steps-track .rad-ui-steps-line { | ||
| background-color: var(--rad-ui-color-accent-600); | ||
| } | ||
| } |
There was a problem hiding this comment.
Data attribute selectors don't match the component output.
The SCSS uses data-status with values "active" and "complete", but StepItem.tsx renders data-state with values "active" and "completed". These selectors will never match, leaving active/completed steps unstyled.
Proposed fix
- &[data-status="active"] {
+ &[data-state="active"] {
.rad-ui-steps-bubble {
border-color: var(--rad-ui-color-accent-600);
background-color: var(--rad-ui-color-accent-50);
color: var(--rad-ui-color-accent-700);
box-shadow: 0 0 0 4px var(--rad-ui-color-accent-100);
}
}
- &[data-status="complete"] {
+ &[data-state="completed"] {
.rad-ui-steps-bubble {
border-color: var(--rad-ui-color-accent-600);
background-color: var(--rad-ui-color-accent-600);
color: white;
}
- // stylelint-disable-next-line no-descending-specificity
.rad-ui-steps-track .rad-ui-steps-line {
background-color: var(--rad-ui-color-accent-600);
}
}📝 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.
| &[data-status="active"] { | |
| .rad-ui-steps-bubble { | |
| border-color: var(--rad-ui-color-accent-600); | |
| background-color: var(--rad-ui-color-accent-50); | |
| color: var(--rad-ui-color-accent-700); | |
| box-shadow: 0 0 0 4px var(--rad-ui-color-accent-100); | |
| } | |
| } | |
| &[data-status="complete"] { | |
| .rad-ui-steps-bubble { | |
| border-color: var(--rad-ui-color-accent-600); | |
| background-color: var(--rad-ui-color-accent-600); | |
| color: white; | |
| } | |
| // stylelint-disable-next-line no-descending-specificity | |
| .rad-ui-steps-track .rad-ui-steps-line { | |
| background-color: var(--rad-ui-color-accent-600); | |
| } | |
| } | |
| &[data-state="active"] { | |
| .rad-ui-steps-bubble { | |
| border-color: var(--rad-ui-color-accent-600); | |
| background-color: var(--rad-ui-color-accent-50); | |
| color: var(--rad-ui-color-accent-700); | |
| box-shadow: 0 0 0 4px var(--rad-ui-color-accent-100); | |
| } | |
| } | |
| &[data-state="completed"] { | |
| .rad-ui-steps-bubble { | |
| border-color: var(--rad-ui-color-accent-600); | |
| background-color: var(--rad-ui-color-accent-600); | |
| color: white; | |
| } | |
| .rad-ui-steps-track .rad-ui-steps-line { | |
| background-color: var(--rad-ui-color-accent-600); | |
| } | |
| } |
🧰 Tools
🪛 GitHub Actions: Lint
[error] 58-58: stylelint: 'no-descending-specificity' has already been disabled. CssSyntaxError
🤖 Prompt for AI Agents
In `@styles/themes/components/steps.scss` around lines 42 - 62, The SCSS selectors
use data-status="active"/"complete" but the component StepItem.tsx renders
data-state="active"/"completed", so update the selectors in steps.scss to target
data-state instead of data-status and use "completed" (not "complete") — i.e.,
change &[data-status="active"] and &[data-status="complete"] to
&[data-state="active"] and &[data-state="completed"] and also update the nested
selector for .rad-ui-steps-track .rad-ui-steps-line accordingly so active and
completed steps are styled.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on February 7
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| document.addEventListener('mouseup', handleMouseUp); | ||
| } | ||
|
|
||
| // Add listeners when dragging starts |
There was a problem hiding this comment.
Missing cleanup leaves body styles corrupted on unmount
Medium Severity
The useEffect cleanup in ScrollAreaThumb no longer calls stopDrag() when the component unmounts. The startDrag function sets document.body.style.cursor to 'grabbing' and document.body.style.userSelect to 'none'. If the component unmounts while the user is dragging (e.g., navigation or conditional render), these body styles are never reset, leaving the page with a stuck grabbing cursor and disabled text selection everywhere.
Coverage
✅ Coverage thresholds met! All tests passing. Run |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@styles/themes/components/combobox.scss`:
- Around line 60-64: The dropdown content rule in
styles/themes/components/combobox.scss currently uses background-color:
transparent, backdrop-filter: blur(0px), and box-shadow: none which causes
readability issues and includes no-op properties; change the dropdown content
selector to use a solid or semi‑transparent background (for example a theme
token like var(--rad-ui-color-surface) or rgba(...) with ~0.9 alpha) to ensure
text contrast, remove backdrop-filter: blur(0px) and box-shadow: none if you
don't need a shadow, and verify text color contrasts against the chosen
background so the combobox dropdown items remain readable.
♻️ Duplicate comments (3)
styles/themes/components/steps.scss (1)
41-61: Data attribute selectors mismatch component output — styles will not apply.The selectors use
data-status="active"anddata-status="complete", butStepItem.tsxrendersdata-state="active"anddata-state="completed". These styles will never be applied.Proposed fix
- &[data-status="active"] { + &[data-state="active"] { .rad-ui-steps-bubble { border-color: var(--rad-ui-color-accent-600); background-color: var(--rad-ui-color-accent-50); color: var(--rad-ui-color-accent-700); box-shadow: 0 0 0 4px var(--rad-ui-color-accent-100); } } - &[data-status="complete"] { + &[data-state="completed"] { .rad-ui-steps-bubble { border-color: var(--rad-ui-color-accent-600); background-color: var(--rad-ui-color-accent-600); color: white; } - // stylelint-disable-next-line no-descending-specificity .rad-ui-steps-track .rad-ui-steps-line { background-color: var(--rad-ui-color-accent-600); } }styles/themes/components/combobox.scss (1)
1-2: Remove unused@use 'button'import.This import is not referenced anywhere in the file and should be removed.
src/components/ui/ScrollArea/fragments/ScrollAreaScrollbar.tsx (1)
154-154: Remove unused variableshouldRender.This variable duplicates the logic of
isVisibleon line 182 and is never used.
🧹 Nitpick comments (7)
styles/themes/components/tooltip.scss (1)
16-17: Add reduced‑motion fallback for tooltip animation.
Animation should respect user motion preferences.♿ Suggested change
.rad-ui-tooltip-content-inner { background-color: var(--rad-ui-color-gray-950); color: var(--rad-ui-color-gray-50); @@ animation: rad-ui-tooltip-slide-in 200ms cubic-bezier(0.16, 1, 0.3, 1); will-change: transform, opacity; position: relative; // Ensure z-index works if needed, and arrow positioning context? No arrow is sibling. @@ } +@media (prefers-reduced-motion: reduce) { + .rad-ui-tooltip-content-inner { + animation: none; + } +} + `@keyframes` rad-ui-tooltip-slide-in { from { opacity: 0; transform: translateY(2px) scale(0.96); }Also applies to: 25-35
.stylelintrc.json (1)
24-24: Consider documenting the rationale for disabling specificity checks.Disabling
no-descending-specificityremoves warnings about CSS selector specificity conflicts, which can make stylesheets harder to maintain and debug over time. While this may be necessary given the complex component styling in this PR, consider:
- Adding a comment in the config explaining why this rule is disabled
- Reviewing if specific styling patterns could be restructured to avoid specificity conflicts
- Monitoring for specificity issues during development and code review
styles/themes/components/steps.scss (1)
27-27: Consider whether 0s transitions are intentional.Setting
transition: all 0sexplicitly disables animations. If no animation is intended, you could omit the property entirely (unless overriding inherited transitions). If animations were desired for state changes, consider values like0.2s ease.styles/themes/components/menubar.scss (1)
41-50: Respectprefers-reduced-motionfor the new animation.Add a reduced-motion override so users who opt out of animations aren’t forced to see the slide/scale effect.
♻️ Proposed diff
.rad-ui-menubar-content { z-index: 50; min-width: 200px; background-color: transparent; backdrop-filter: blur(0px); border-radius: 8px; border: 1px solid var(--rad-ui-color-gray-200); padding: 4px; box-shadow: none; animation: rad-ui-slide-in 0.2s cubic-bezier(0.16, 1, 0.3, 1); + + `@media` (prefers-reduced-motion: reduce) { + animation: none; + } &[data-state="closed"] { display: none; } }Also applies to: 90-99
src/components/ui/Accordion/tests/Accordion.dynamic.test.tsx (1)
39-48: Consider usingwaitForfor more idiomatic async testing.The manual
setTimeoutinsideact()works but couples the test to specific timing. UsingwaitForis more resilient and idiomatic for testing-library.♻️ Suggested refactor
- // Wait for items to update - await act(async() => { - await new Promise(resolve => setTimeout(resolve, 100)); - }); - - // The content height should have updated or at least not be broken. - // Since we mocked ResizeObserver, we can't easily test the actual offsetHeight - // in JSDOM, but we can verify that the component didn't crash and the new items are rendered. - expect(screen.getByText('Item 1')).toBeInTheDocument(); - expect(screen.getByText('Item 2')).toBeInTheDocument(); - expect(screen.getByText('Item 3')).toBeInTheDocument(); + // Wait for items to update and verify new items are rendered + // Since we mocked ResizeObserver, we can't easily test the actual offsetHeight + // in JSDOM, but we can verify that the component didn't crash and the new items are rendered. + await waitFor(() => { + expect(screen.getByText('Item 1')).toBeInTheDocument(); + expect(screen.getByText('Item 2')).toBeInTheDocument(); + expect(screen.getByText('Item 3')).toBeInTheDocument(); + });You'll also need to add
waitForto the import:-import { render, screen, act } from '@testing-library/react'; +import { render, screen, waitFor } from '@testing-library/react';styles/themes/components/combobox.scss (1)
21-21: Consider removingtransition: all 0sdeclarations.Setting
transitionwith0sduration has no visual effect—it's equivalent to having no transition at all. If animations are intentionally disabled, simply omitting these properties is cleaner and reduces CSS payload.♻️ Proposed fix
font-size: 14px; - transition: all 0s; outline: none; cursor: pointer;margin-left: 8px; - transition: transform 0s; }Also applies to: 41-41
src/components/ui/ScrollArea/fragments/ScrollAreaScrollbar.tsx (1)
74-91: Consider using MutationObserver for dynamic content.The ResizeObserver only tracks children present when the effect runs (line 89). Dynamically added children won't be observed until a re-render triggers the effect again.
For most use cases, the viewport observation is sufficient, but if content is frequently added/removed, consider adding a MutationObserver to re-attach observers:
♻️ Optional enhancement
checkOverflow(); const ro = new ResizeObserver(checkOverflow); ro.observe(viewport); - Array.from(viewport.children).forEach(c => ro.observe(c)); - return () => ro.disconnect(); + + const observeChildren = () => { + Array.from(viewport.children).forEach(c => ro.observe(c)); + }; + observeChildren(); + + const mo = new MutationObserver(() => { + observeChildren(); + checkOverflow(); + }); + mo.observe(viewport, { childList: true }); + + return () => { + ro.disconnect(); + mo.disconnect(); + };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.stylelintrc.jsonsrc/components/ui/Accordion/fragments/AccordionHeader.tsxsrc/components/ui/Accordion/tests/Accordion.dynamic.test.tsxsrc/components/ui/ScrollArea/fragments/ScrollAreaScrollbar.tsxstyles/themes/components/combobox.scssstyles/themes/components/menubar.scssstyles/themes/components/steps.scssstyles/themes/components/tooltip.scss
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/ui/Accordion/fragments/AccordionHeader.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: kotAPI
Repo: rad-ui/ui PR: 1031
File: src/components/ui/Accordion/fragments/AccordionRoot.tsx:41-44
Timestamp: 2025-04-07T04:38:34.864Z
Learning: The Accordion component in rad-ui/ui supports both controlled and uncontrolled modes through props like `value`, `defaultValue`, and `onValueChange`. When implementing controlled components, remember to: 1) Initialize state from defaultValue, 2) Update internal state when value changes (controlled mode), 3) Call onValueChange callback, and 4) Prevent internal state updates when in controlled mode.
📚 Learning: 2024-12-12T08:34:33.079Z
Learnt from: decipher-cs
Repo: rad-ui/ui PR: 417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
Applied to files:
styles/themes/components/steps.scssstyles/themes/components/combobox.scss
🧬 Code graph analysis (2)
src/components/ui/ScrollArea/fragments/ScrollAreaScrollbar.tsx (1)
src/components/ui/ScrollArea/context/ScrollAreaContext.tsx (1)
ScrollAreaContext(16-19)
src/components/ui/Accordion/tests/Accordion.dynamic.test.tsx (2)
docs/app/docs/components/accordion/docs/anatomy.tsx (1)
Accordion(3-14)rollup.config.cjs (1)
resolve(1-1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: coverage
🔇 Additional comments (10)
styles/themes/components/steps.scss (1)
84-92: LGTM!The last-child cleanup rules appropriately hide the trailing line and remove unnecessary padding on the final step.
src/components/ui/Accordion/tests/Accordion.dynamic.test.tsx (2)
1-4: LGTM!Imports are appropriate for the test requirements.
6-29: LGTM!The DynamicContent test component is well-structured with proper cleanup of the timeout in the useEffect return.
styles/themes/components/combobox.scss (5)
3-7: LGTM!Root container styling is clean and properly scoped.
77-93: LGTM!Search input styling is well-structured with appropriate focus states. Note: the
transition: all 0son line 87 follows the same pattern flagged earlier.
95-133: LGTM!Item styling properly uses
aria-selectedfor the selected indicator visibility, which aligns with accessibility best practices. The flex layout and state handling are well-implemented.
135-141: LGTM!Label styling provides clear visual hierarchy for grouped items.
143-153: LGTM!Animation uses performant properties (opacity and transform) which won't trigger layout recalculations.
src/components/ui/ScrollArea/fragments/ScrollAreaScrollbar.tsx (2)
49-72: LGTM! Race condition fix verified.The
handleMouseEnternow correctly clearshideTimeoutRef.currentbefore setting visibility (line 56), preventing the race condition where a pending hide timeout could fire after re-entering the scroll area.
182-203: LGTM! Visibility logic is well-structured.The rendering logic correctly handles all four visibility types (
always,auto,scroll,hover). TheshouldKeepInDOMoptimization keeps the element mounted when needed while usingdisplay: nonefor hidden state.Note: The style spread order allows
props.style.displayto override the conditional display, which provides customization flexibility but could lead to unexpected visibility if consumers inadvertently pass a display style.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| background-color: transparent; | ||
| backdrop-filter: blur(0px); | ||
| border: 1px solid var(--rad-ui-color-gray-200); | ||
| border-radius: 8px; | ||
| box-shadow: none; |
There was a problem hiding this comment.
Transparent background on dropdown content may cause readability issues.
The content panel has background-color: transparent and backdrop-filter: blur(0px) (which has no effect). This means dropdown items will be visible against whatever is behind them, potentially causing contrast/readability issues. Additionally, box-shadow: none and backdrop-filter: blur(0px) are no-ops that add unnecessary bytes.
Was a solid or semi-transparent background intended here?
♻️ Suggested fix (if solid background intended)
padding: 4px;
- background-color: transparent;
- backdrop-filter: blur(0px);
+ background-color: white;
border: 1px solid var(--rad-ui-color-gray-200);
border-radius: 8px;
- box-shadow: none;
display: flex;📝 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.
| background-color: transparent; | |
| backdrop-filter: blur(0px); | |
| border: 1px solid var(--rad-ui-color-gray-200); | |
| border-radius: 8px; | |
| box-shadow: none; | |
| padding: 4px; | |
| background-color: white; | |
| border: 1px solid var(--rad-ui-color-gray-200); | |
| border-radius: 8px; | |
| display: flex; |
🤖 Prompt for AI Agents
In `@styles/themes/components/combobox.scss` around lines 60 - 64, The dropdown
content rule in styles/themes/components/combobox.scss currently uses
background-color: transparent, backdrop-filter: blur(0px), and box-shadow: none
which causes readability issues and includes no-op properties; change the
dropdown content selector to use a solid or semi‑transparent background (for
example a theme token like var(--rad-ui-color-surface) or rgba(...) with ~0.9
alpha) to ensure text contrast, remove backdrop-filter: blur(0px) and
box-shadow: none if you don't need a shadow, and verify text color contrasts
against the chosen background so the combobox dropdown items remain readable.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| return ( | ||
| <div | ||
| className={clsx(`${rootClass}-item`, className)} | ||
| data-state={state} |
There was a problem hiding this comment.
Steps styling broken due to data attribute mismatch
High Severity
The StepItem component sets data-state attribute with values 'completed', 'active', and 'inactive', but the CSS selectors in steps.scss look for [data-status="active"] and [data-status="complete"]. This causes two mismatches: the attribute name (data-state vs data-status) and the completed value (completed vs complete). The step status styling (active glow, completed checkmark colors, progress line highlighting) will never be applied.
Additional Locations (1)
|
|
||
| /* Firefox */ | ||
| -ms-overflow-style: none; | ||
|
|
There was a problem hiding this comment.
ScrollArea viewport missing overflow property breaks scrolling
High Severity
The .rad-ui-scroll-area-viewport CSS class no longer has an overflow property. The previous stylesheet had overflow-y: scroll; but this was removed without replacement. Since ScrollAreaViewport component doesn't set overflow inline and relies on the CSS class, the viewport content won't be scrollable at all.
| {...props} | ||
| > | ||
| {children} | ||
| </Primitive.div> |
There was a problem hiding this comment.
Dialog missing aria-hidden when force-mounted but closed
Medium Severity
The refactored DialogPrimitiveContent removed the aria-hidden={!isOpen ? 'true' : undefined} attribute that was present in the old implementation. When forceMount is true and the dialog is closed, the content remains in the DOM without aria-hidden="true", causing screen readers to announce the hidden dialog content. This is an accessibility regression.
Note
Introduces key UI component enhancements with new capabilities, accessibility refinements, and expanded tests.
number | number[]), updated range rendering and thumb indexing; adds interaction and range teststypevisibility modes (auto | always | scroll | hover), vertical/horizontal thumbs, dynamic sizing/dragging, corner styling, and storiesvalue/defaultValue/onValueChange) and per-itemdata-state; new testsHeaderis now anh3; adds dynamic content height test and ref-forwarding updatesWritten by Cursor Bugbot for commit 9cf35f7. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Style
✏️ Tip: You can customize this high-level summary in your review settings.