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
|
📝 WalkthroughWalkthroughThis PR introduces multi-thumb slider support, search-based filtering for combobox, scrollbar visibility/overflow detection, dialog focus management refactoring via Floater wrapper, dynamic height synchronization in collapsible, alertdialog controlled state via onOpenChange prop, and comprehensive styling updates across multiple components. Tests and jsdom polyfills are added. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant SliderRoot
participant SliderThumb
participant Viewport
User->>SliderRoot: onPointerDown (multi-thumb)
SliderRoot->>SliderRoot: Detect thumb index, set activeThumbIndexRef
User->>SliderRoot: onPointerMove
SliderRoot->>SliderRoot: setFromPosition(clientX) with activeThumbIndexRef
SliderRoot->>SliderRoot: Update value array[activeThumbIndex]
SliderRoot->>SliderThumb: Re-render with updated currentValue
SliderThumb->>Viewport: Update viewport.scrollLeft/scrollTop
User->>SliderRoot: onPointerUp
SliderRoot->>SliderRoot: Reset activeThumbIndexRef, dragging state
sequenceDiagram
actor User
participant ComboboxRoot
participant ComboboxPrimitive
participant ItemList
User->>ComboboxRoot: Type search query
ComboboxRoot->>ComboboxRoot: setSearch(query)
ComboboxRoot->>ComboboxPrimitive: Compute hiddenIndices via labelsRef
ComboboxPrimitive->>ComboboxPrimitive: Merge disabledIndices + hiddenIndices = totalDisabledIndices
ComboboxPrimitive->>ItemList: Filter visible items from totalDisabledIndices
User->>ComboboxPrimitive: Navigate/select
ComboboxPrimitive->>ComboboxPrimitive: Use totalDisabledIndices for typeahead & selection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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
🧪 Generate unit tests (beta)
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: 2
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/fragments/ScrollAreaScrollbar.tsx (1)
154-178:⚠️ Potential issue | 🟡 MinorEffect cleanup may not run when expected due to early return.
The effect has an early return when
!isScrollingRef.current, but it still returns a cleanup function. This means listeners are only added whenisScrollingRef.currentisfalse, which appears inverted from the intent. If the goal is to add global listeners when scrolling starts, the logic should be reconsidered.Additionally, the
setTimeouton line 121 that starts continuous scrolling has no cleanup if the component unmounts during the 300ms delay.🔧 Consider restructuring the effect
React.useEffect(() => { - if (!isScrollingRef.current) { - return () => { - removeListenersRef.current?.(); - stopContinuousScroll(); - }; - } + if (!isScrollingState) return; const handleMouseUp = () => stopContinuousScroll(); const handleMouseLeave = () => stopContinuousScroll(); document.addEventListener('mouseup', handleMouseUp); document.addEventListener('mouseleave', handleMouseLeave); removeListenersRef.current = () => { document.removeEventListener('mouseup', handleMouseUp); document.removeEventListener('mouseleave', handleMouseLeave); }; return () => { removeListenersRef.current?.(); removeListenersRef.current = null; stopContinuousScroll(); }; }, [isScrollingState, stopContinuousScroll]);src/components/ui/ScrollArea/stories/ScrollArea.stories.tsx (2)
18-25:⚠️ Potential issue | 🟡 MinorMissing
keyprop on fragments in map iteration.When using fragments in a
.map()iteration, React requires akeyprop. The shorthand<>syntax doesn't support keys - use<React.Fragment key={...}>instead.🔧 Proposed 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... </Text> - </> + </React.Fragment> ))}
56-61:⚠️ Potential issue | 🟡 MinorSame missing
keyissue in LayoutTemplate.This has the same fragment key issue as the main template.
🔧 Proposed 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/core/primitives/Combobox/fragments/ComboboxPrimitiveSearch.tsx (1)
56-64:⚠️ Potential issue | 🟡 MinorUser-provided
onKeyDownwill override internal Enter key handling.Spreading
{...props}after the internalonKeyDownmeans any user-providedonKeyDowncompletely replaces the Enter key selection behavior. Consider merging handlers:🛠️ Proposed fix to merge onKeyDown handlers
+ onKeyDown={(event: React.KeyboardEvent<HTMLInputElement>) => { + if (activeIndex !== null) { + if (event.key === 'Enter') { + event.preventDefault(); + handleSelect(activeIndex); + } + } + // Call user-provided handler if exists + props.onKeyDown?.(event); + }} {...props} + // Remove onKeyDown from spread to avoid overrideOr extract
onKeyDownfrom props before spreading:->(({ className, ...props }, forwardedRef) => { +>(({ className, onKeyDown: userOnKeyDown, ...props }, forwardedRef) => { ... onKeyDown={(event: React.KeyboardEvent<HTMLInputElement>) => { if (activeIndex !== null && event.key === 'Enter') { event.preventDefault(); handleSelect(activeIndex); } + userOnKeyDown?.(event); }}src/core/primitives/Combobox/fragments/ComboboxPrimitiveItem.tsx (1)
23-26:⚠️ Potential issue | 🟠 MajorHooks called after early return violates React's rules of hooks.
The early return at Lines 23-26 causes all subsequent hooks (Lines 45-110) to be called conditionally. React requires hooks to be called in the same order on every render. This can cause runtime errors or unpredictable behavior.
Proposed fix: Move hooks before the early return
const ComboboxPrimitiveItem = React.forwardRef< React.ElementRef<typeof Primitive.div>, ComboboxPrimitiveItemProps & React.ComponentPropsWithoutRef<typeof Primitive.div> >(({ children, value, disabled, className, ...props }, forwardedRef) => { const context = useContext(ComboboxPrimitiveContext); + const groupContext = useComboboxGroupContext(); + const itemRef = React.useRef<HTMLButtonElement>(null); + const { ref, index } = Floater.useListItem({ label: value }); if (!context) { console.error('ComboboxPrimitiveItem must be used within a ComboboxPrimitive'); return null; } const { handleSelect, isTypingRef, // ... rest of destructuring } = context; - const itemRef = React.useRef<HTMLButtonElement>(null); - const { ref, index } = Floater.useListItem({ label: value }); - const isHidden = hiddenIndices.includes(index); // ... rest of componentAlternatively, consider throwing an error or using a wrapper that guarantees context availability.
🤖 Fix all issues with AI agents
In `@src/components/ui/Slider/fragments/SliderRoot.tsx`:
- Around line 50-54: The fallback for useControllableState in SliderRoot.tsx is
using valueProp as a default which can blur controlled vs default semantics and
produce number/array type mismatches; change the second argument to choose a
safe shape instead of valueProp (e.g., defaultValue ?? (Array.isArray(valueProp)
? [0, 0] : 0)) so the initial state is a number for single-value sliders and an
array for range sliders, and keep using the existing
useControllableState/valueProp/onValueChange identifiers to preserve controlled
behavior.
In `@src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveContent.tsx`:
- Around line 47-65: The ResizeObserver usage in the useEffect can crash on
browsers without support; inside the useEffect (before creating new
ResizeObserver) add a guard that checks for runtime availability (e.g. ensure
typeof window !== "undefined" and 'ResizeObserver' in window or typeof
ResizeObserver !== "undefined") and simply return if it's not available; keep
the rest of the logic using ref, heightRef, setHeight, open and
transitionDuration unchanged so tests/mocks still work.
🧹 Nitpick comments (31)
src/components/ui/Tooltip/fragments/TooltipContent.tsx (1)
41-44: Good structural improvement for separating positioning from visual styling.The inner wrapper cleanly separates Floating UI's positioning (outer element) from the tooltip's visual styles and animations (inner element).
Minor nit: The
clsx('rad-ui-arrow')call is redundant when passing a single static string—className="rad-ui-arrow"would suffice.,
Optional simplification
- {showArrow && <FloatingArrow className={clsx('rad-ui-arrow')} ref={arrowRef} context={context} />} + {showArrow && <FloatingArrow className="rad-ui-arrow" ref={arrowRef} context={context} />}styles/themes/components/tooltip.scss (1)
18-18: Misleading comment—the arrow is a sibling element.The comment states "No arrow is sibling," but in
TooltipContent.tsx, theFloatingArrowis rendered as a sibling to{children}inside this inner wrapper. Consider updating or removing this comment to avoid confusion.Suggested fix
- position: relative; // Ensure z-index works if needed, and arrow positioning context? No arrow is sibling. + position: relative; // Ensure z-index works and provides positioning context for the arrowstyles/themes/components/combobox.scss (2)
1-5: Consider using a CSS variable for the fixed width.The
240pxwidth is hardcoded in both.rad-ui-combobox-root(line 4) and.rad-ui-combobox-content(line 55). Extracting this to a CSS custom property would improve maintainability and allow easier customization.♻️ Suggested refactor
.rad-ui-combobox-root { position: relative; display: inline-block; - width: 240px; + --rad-ui-combobox-width: 240px; + width: var(--rad-ui-combobox-width); }.rad-ui-combobox-content { z-index: 50; margin-top: 4px; - width: 240px; + width: var(--rad-ui-combobox-width);Also applies to: 52-56
56-56: Inconsistent CSS variable naming convention.The variable
--rad-combobox-content-heightdoesn't follow the--rad-ui-*prefix pattern used elsewhere in this file (e.g.,--rad-ui-color-gray-300). Consider renaming for consistency.♻️ Suggested fix
- max-height: var(--rad-combobox-content-height, 300px); + max-height: var(--rad-ui-combobox-content-height, 300px);styles/themes/components/menubar.scss (3)
1-1: Unused import:buttonmodule is not referenced in this file.The
@use 'button'import does not appear to be utilized anywhere in this stylesheet. If it's not needed, consider removing it to avoid potential unnecessary CSS inclusion.🧹 Proposed fix
-@use 'button'; - .rad-ui-menubar-root {
25-25: Consider removing no-optransition: all 0s;.A
0stransition duration effectively disables transitions. If this is intentional to override inherited transitions, consider adding a comment to clarify the intent. Otherwise, the property can be removed.
44-45: No-opbackdrop-filter: blur(0px)can be removed.A
blur(0px)backdrop filter has no visual effect. Combined withbackground-color: transparent, this appears to be a placeholder. If backdrop blur isn't needed, consider removing the property to reduce CSS overhead.🧹 Proposed fix
min-width: 200px; - background-color: transparent; - backdrop-filter: blur(0px); + background-color: var(--rad-ui-color-gray-50); border-radius: 8px;Alternatively, if transparency is intentional, just remove the no-op backdrop-filter:
background-color: transparent; - backdrop-filter: blur(0px); border-radius: 8px;src/components/ui/AlertDialog/tests/AlertDialog.a11y.test.tsx (2)
112-114: Track the flaky focus assertion explicitly.
Instead of commenting out expectations, consider moving the focus-restoration check into atest.todo/it.skipso it doesn’t silently lose coverage.
191-193: Avoid leaving focus-trap assertions commented.
These assertions are important for a11y behavior; consider extracting them into a dedicatedtest.todo/it.skipwith a clear reason to keep the gap visible.Also applies to: 196-197, 201-203
src/components/ui/ScrollArea/context/ScrollAreaContext.tsx (1)
11-11: Replaceanywithnumberfor type safety.Using
anyforclientXandclientYbypasses TypeScript's type checking. These should be typed asnumbersince they represent pixel coordinates.🔧 Proposed 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/ScrollAreaRoot.tsx (3)
38-40: Refs are stable and don't need to be in the dependency array.React refs created with
useRefare stable across renders. Including them in the dependency array has no effect since their identity never changes. This can be misleading to future maintainers.🔧 Suggested fix
useEffect(() => { initializeThumbSizes(); - }, [scrollYThumbRef, scrollXThumbRef, scrollAreaViewportRef]); + }, []);
52-54: Address static analysis warning about forEach callback.The static analysis tool flags line 54 because
resizeObserver.observe()returnsvoid, making the callback technically return a value. While this is harmless, you can silence the linter by using a block body or a simplefor...ofloop.🔧 Proposed fix using block body
const resizeObserver = new ResizeObserver(() => handleResize()); resizeObserver.observe(viewport); - Array.from(viewport.children).forEach(child => resizeObserver.observe(child)); + Array.from(viewport.children).forEach(child => { + resizeObserver.observe(child); + });
32-36: Consider using a ref callback utility to avoidas anycasts.The merged ref logic works correctly, but the
as anycasts reduce type safety. A common pattern is to use amergeRefsutility function.🔧 Alternative implementation
+ // Utility function (could be extracted to a shared module) + const mergeRefs = <T,>(...refs: (React.Ref<T> | undefined)[]) => (node: T | null) => { + refs.forEach(ref => { + if (typeof ref === 'function') ref(node); + else if (ref) (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);src/components/ui/ScrollArea/fragments/ScrollAreaScrollbar.tsx (1)
87-90: Address static analysis warning about forEach callback.Same issue as in ScrollAreaRoot.tsx - the
observe()call implicitly returns, triggering the linter warning.🔧 Proposed fix
const ro = new ResizeObserver(checkOverflow); ro.observe(viewport); - Array.from(viewport.children).forEach(c => ro.observe(c)); + Array.from(viewport.children).forEach(c => { + ro.observe(c); + }); return () => ro.disconnect();src/components/ui/ScrollArea/fragments/ScrollAreaThumb.tsx (1)
67-79: Global mouse listeners are always attached, not just during drag.The
mousemoveandmouseuplisteners are added on mount and persist throughout the component lifecycle. This is inefficient since these handlers only need to be active during drag operations.Consider attaching listeners in
startDragand removing them instopDragto avoid unnecessary event processing.🔧 Proposed optimization
+ const startDrag = useCallback((e: React.MouseEvent) => { + if (!scrollAreaViewportRef?.current) return; + + e.preventDefault(); + e.stopPropagation(); + + isDraggingRef.current = true; + dragStartRef.current = { + x: e.clientX, + y: e.clientY, + scrollTop: scrollAreaViewportRef.current.scrollTop, + scrollLeft: scrollAreaViewportRef.current.scrollLeft + }; + + document.body.style.userSelect = 'none'; + document.body.style.cursor = 'grabbing'; + + document.addEventListener('mousemove', handleDrag); + document.addEventListener('mouseup', stopDrag); + }, [scrollAreaViewportRef, handleDrag, stopDrag]); + const stopDrag = useCallback(() => { + isDraggingRef.current = false; + document.body.style.cursor = ''; + document.body.style.userSelect = ''; + document.removeEventListener('mousemove', handleDrag); + document.removeEventListener('mouseup', stopDrag); + }, [handleDrag]); - 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); - stopDrag(); - }; - }, [handleDrag, stopDrag]); + // Cleanup on unmount + React.useEffect(() => { + return () => stopDrag(); + }, [stopDrag]);Note: This refactor requires adjusting the dependency order of
startDrag,stopDrag, andhandleDragto avoid circular dependencies. Alternatively, keep the current approach if simplicity is preferred, as the performance impact is minimal for typical use cases.src/components/ui/Steps/fragments/StepRoot.tsx (1)
2-2: Remove unuseduseStateimport.The
useStateimport is no longer needed after refactoring to useuseControllableState.🧹 Proposed fix
-import React, { useState } from 'react'; +import React from 'react';src/components/ui/Accordion/fragments/AccordionHeader.tsx (1)
6-13: Good semantic improvement using heading element.Using
h3for the accordion header improves accessibility by establishing proper document structure. This aligns with WAI-ARIA accordion patterns.Consider making the heading level configurable via an
asprop (e.g.,as="h2") for contexts where the accordion appears at different levels in the document hierarchy. This is a nice-to-have for future flexibility.styles/themes/components/steps.scss (1)
50-55: Consider using a CSS variable instead of hardcodedwhite.For theme consistency, replace the hardcoded
whitewith a CSS variable likevar(--rad-ui-color-white)orvar(--rad-ui-color-gray-0)if available in your design system.🎨 Proposed fix
&[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; + color: var(--rad-ui-color-white, `#ffffff`); }src/components/ui/Dialog/tests/Dialog.portal.test.tsx (1)
103-109: Consider using the sharedaxehelper from test-utils for consistency.The test imports
axe-coredirectly and configures rule disabling inline. However,src/test-utils/index.tsexports a sharedaxehelper with pre-configured tag filtering. Using it (with a custom override mechanism) would improve consistency across the test suite.If per-test rule disabling is needed frequently, consider extending the shared helper to accept rule overrides:
♻️ Example approach
// In test-utils/index.ts export const axe = (element?: HTMLElement, options?: { disabledRules?: string[] }) => axeCore.run(element ?? document.body, { runOnly: { type: 'tag', values: ACCESSIBILITY_TEST_TAGS }, rules: options?.disabledRules?.reduce((acc, rule) => ({ ...acc, [rule]: { enabled: false } }), {}) });src/components/ui/Combobox/tests/Combobox.filtering.test.tsx (1)
7-44: Good test coverage for filtering functionality.The test validates core filtering and group visibility behavior. A few optional enhancements to consider:
- Test clearing the search restores all items
- Consider using
userEventinstead offireEvent.changefor more realistic input simulation- Optionally verify ARIA attributes on filtered items (e.g.,
aria-hidden)♻️ Optional: Add search clearing test
+ // Clear search + fireEvent.change(search, { target: { value: '' } }); + + expect(screen.getByText('Apple')).toBeVisible(); + expect(screen.getByText('Banana')).toBeVisible(); + expect(screen.getByText('Carrot')).toBeVisible();src/components/ui/Slider/tests/Slider.range.test.tsx (1)
6-8: Remove duplicate PointerEvent polyfill.This polyfill is now defined globally in
src/setupTests.ts(lines 64-66), so this local definition is redundant and can be removed.♻️ Suggested removal
-// Polyfill PointerEvent for jsdom -// `@ts-ignore` -if (typeof window !== 'undefined' && !window.PointerEvent) window.PointerEvent = MouseEvent; - describe('Slider Range Support', () => {src/components/ui/Slider/Slider.tsx (1)
63-69: Default implementation renders only a single thumb.When
defaultValueis passed as an array (for range/multi-thumb scenarios), this default implementation still renders only one<Slider.Thumb>. This is likely intentional since users needing multi-thumb functionality would use the compound components directly (e.g.,Slider.Rootwith multipleSlider.Thumbchildren orSlider.RangeSlider), but it may be worth documenting this behavior.src/components/ui/Steps/tests/Steps.test.tsx (1)
33-40: Consider addingdefaultValuefor uncontrolled mode consistency.This test doesn't provide
valueoronValueChange, which is inconsistent with the first test's controlled pattern. If the Steps component supports uncontrolled mode with adefaultValue, consider explicitly testing that:it('supports customRootClass', () => { render( - <Steps.Root customRootClass="custom-steps" data-testid="steps-root"> + <Steps.Root customRootClass="custom-steps" defaultValue={0} data-testid="steps-root"> <Steps.Item value={0}>Step 0</Steps.Item> </Steps.Root> );src/core/primitives/Combobox/fragments/ComboboxPrimitiveGroup.tsx (1)
51-51: Inlinedisplay: nonemay conflict with CSS specificity.Using inline styles for hiding works but can be overridden unexpectedly or conflict with CSS frameworks. Consider using a data attribute or class-based approach for more consistent styling control.
♻️ Optional alternative using data attribute
- <div - className={className} - ref={forwardedRef} - {...props} - style={{ display: shouldHide ? 'none' : undefined, ...props.style }} - > + <div + className={className} + ref={forwardedRef} + {...props} + data-hidden={shouldHide || undefined} + style={props.style} + >Then in CSS:
[data-hidden] { display: none; }src/core/primitives/Combobox/contexts/ComboboxGroupContext.tsx (1)
9-9: Consider adding null guard in the custom hook.The hook returns
ComboboxGroupContextType | null, requiring consumers to handle the null case. If items should always be within a group, consider throwing an error for better DX:♻️ Optional guard pattern
-export const useComboboxGroupContext = () => useContext(ComboboxGroupContext); +export const useComboboxGroupContext = () => { + const context = useContext(ComboboxGroupContext); + // Return null for optional group usage, or throw if required: + // if (!context) throw new Error('useComboboxGroupContext must be used within ComboboxGroup'); + return context; +};However, if items can exist outside groups (making the group optional), returning null is the correct approach.
src/core/primitives/Combobox/fragments/ComboboxPrimitiveSearch.tsx (1)
12-25: Several destructured context values appear unused.
labelsRef,valuesRef, andelementsRefare destructured but not used in the component. Consider removing them to reduce noise and prevent confusion:♻️ Suggested cleanup
const { refs, handleSelect, - labelsRef, - valuesRef, activeIndex, - elementsRef, virtualItemRef, getReferenceProps, setHasSearch, search, setSearch, - setActiveIndex + setActiveIndex, + valuesRef } = context;Note:
valuesRefis used on line 52 foraria-activedescendant, so keep that one.src/core/primitives/Combobox/fragments/ComboboxPrimitiveRoot.tsx (1)
136-146: Ref value in dependency array will not trigger re-renders.
selectedItemRef.currentin the dependency array (Line 146) won't cause the effect to re-run when the ref changes because React doesn't track ref mutations. The effect currently works becauseshiftprop changes or component remounts trigger it.If the intent is to run this effect when the selected item changes, consider using a state variable or a callback ref pattern instead.
Suggested fix
- useLayoutEffect(() => { - if (!shift) return; - if (!selectedItemRef) return; - if (refs.floating.current && selectedItemRef.current) { - const rectA = refs.floating.current.getBoundingClientRect(); - const rectB = selectedItemRef.current.getBoundingClientRect(); - - const relativeTop = rectA.top - rectB.bottom; - setOffsetPositionValue(relativeTop); - } - }, [selectedItemRef.current, shift]); + useLayoutEffect(() => { + if (!shift) return; + if (!selectedItemRef) return; + if (refs.floating.current && selectedItemRef.current) { + const rectA = refs.floating.current.getBoundingClientRect(); + const rectB = selectedItemRef.current.getBoundingClientRect(); + + const relativeTop = rectA.top - rectB.bottom; + setOffsetPositionValue(relativeTop); + } + }, [shift, refs.floating]);src/core/primitives/Combobox/fragments/ComboboxPrimitiveItem.tsx (1)
118-118: Style spread order allowsprops.styleto override hidden state.The current order
{ display: isHidden ? 'none' : undefined, ...props.style }allows consumers to accidentally override thedisplay: nonefor hidden items. If hiding should be enforced, reverse the order.If hidden state should be enforced
- style={{ display: isHidden ? 'none' : undefined, ...props.style }} + style={{ ...props.style, display: isHidden ? 'none' : undefined }}styles/themes/components/slider.scss (1)
172-177: Focus styles are duplicated from base thumb rules.The focus outline styles (Lines 172-176) are identical to the base thumb focus styles (Lines 76-80). Since outlines don't conflict with the transform overrides, these could be removed to reduce duplication, but keeping them explicit is also acceptable for maintainability.
src/components/ui/Slider/fragments/SliderRoot.tsx (1)
58-64: Consider using existinguseMergeRefsutility.The manual ref merging works but is verbose and uses type casts. The codebase already uses
Floater.useMergeRefsin other components (e.g., ComboboxPrimitiveItem, DialogPrimitiveContent).Suggested simplification
+import Floater from '~/core/primitives/Floater'; + // ... - const mergedRef = React.useMemo(() => { - return (node: HTMLDivElement | null) => { - (internalRef as any).current = node; - if (typeof ref === 'function') ref(node); - else if (ref) (ref as any).current = node; - }; - }, [ref]); + const mergedRef = Floater.useMergeRefs([internalRef, ref]);src/core/hooks/useControllableState/useControllableState.test.tsx (1)
88-100: Ensure console.warn spies restore even if assertions fail.
All three warnSpy tests (lines 88–100, 103–120, 138–161) callmockRestore()unconditionally at the end. If anyexpect()throws, the spy won't be restored, causing cross-test pollution. Wrap test logic intry/finally, or addafterEach(() => jest.restoreAllMocks())tosrc/setupTests.tsfor global cleanup (recommended, sincejest.config.tshasclearMocksbut notrestoreMocks).♻️ Safer cleanup with try/finally (per-test fix)
it('should fall back to defaultValue when controlledValue becomes undefined', () => { const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}); - const initialProps: { value: string | undefined } = { value: 'controlled' }; - const { result, rerender } = renderHook( - ({ value }) => useControllableState<string>(value, 'default', jest.fn()), - { initialProps } - ); - - expect(result.current[0]).toBe('controlled'); - - rerender({ value: undefined }); - expect(result.current[0]).toBe('default'); - warnSpy.mockRestore(); + try { + const initialProps: { value: string | undefined } = { value: 'controlled' }; + const { result, rerender } = renderHook( + ({ value }) => useControllableState<string>(value, 'default', jest.fn()), + { initialProps } + ); + + expect(result.current[0]).toBe('controlled'); + + rerender({ value: undefined }); + expect(result.current[0]).toBe('default'); + } finally { + warnSpy.mockRestore(); + } });
| const [value, setValue] = useControllableState<number | number[]>( | ||
| valueProp, | ||
| defaultValue ?? (Array.isArray(valueProp) ? valueProp : 0), | ||
| onValueChange | ||
| ); |
There was a problem hiding this comment.
defaultValue fallback logic may cause type inconsistencies.
The fallback defaultValue ?? (Array.isArray(valueProp) ? valueProp : 0) has issues:
- If
valuePropis an array anddefaultValueis undefined, it usesvaluePropas the default (butvaluePropmight be the controlled value, not a default) - If both are undefined, it defaults to
0(number), but ifvalueProplater becomes an array, there's a type mismatch
Suggested fix
const [value, setValue] = useControllableState<number | number[]>(
valueProp,
- defaultValue ?? (Array.isArray(valueProp) ? valueProp : 0),
+ defaultValue ?? (Array.isArray(valueProp) ? [min, max] : min),
onValueChange
);This provides sensible defaults based on the expected value type.
🤖 Prompt for AI Agents
In `@src/components/ui/Slider/fragments/SliderRoot.tsx` around lines 50 - 54, The
fallback for useControllableState in SliderRoot.tsx is using valueProp as a
default which can blur controlled vs default semantics and produce number/array
type mismatches; change the second argument to choose a safe shape instead of
valueProp (e.g., defaultValue ?? (Array.isArray(valueProp) ? [0, 0] : 0)) so the
initial state is a number for single-value sliders and an array for range
sliders, and keep using the existing
useControllableState/valueProp/onValueChange identifiers to preserve controlled
behavior.
| // Use ResizeObserver to handle dynamic content changes | ||
| useEffect(() => { | ||
| if (!open || !ref.current || transitionDuration === 0) return; | ||
|
|
||
| const resizeObserver = new ResizeObserver(() => { | ||
| if (ref.current && heightRef.current !== undefined) { | ||
| const newHeight = ref.current.scrollHeight; | ||
| if (newHeight !== heightRef.current) { | ||
| setHeight(newHeight); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| // Observe the first child if possible for more accurate content measurement, | ||
| // or the ref itself if it's not currently animating height: 0 | ||
| resizeObserver.observe(ref.current); | ||
|
|
||
| return () => resizeObserver.disconnect(); | ||
| }, [open, transitionDuration]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, explore the project structure
ls -laRepository: rad-ui/ui
Length of output: 2891
🏁 Script executed:
# Check the file in question
head -100 src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveContent.tsxRepository: rad-ui/ui
Length of output: 3619
🏁 Script executed:
# Check project configuration
cat package.json | head -50Repository: rad-ui/ui
Length of output: 1723
🏁 Script executed:
# Search for ResizeObserver usage in the codebase
rg "ResizeObserver" --type tsx --type ts -B 2 -A 2Repository: rad-ui/ui
Length of output: 80
🏁 Script executed:
# Search for ResizeObserver usage with correct syntax
rg "ResizeObserver" -A 2 -B 2Repository: rad-ui/ui
Length of output: 4344
🏁 Script executed:
# Check full package.json for dependencies related to polyfills
cat package.jsonRepository: rad-ui/ui
Length of output: 10507
🏁 Script executed:
# Check tsconfig for browser target
cat tsconfig.jsonRepository: rad-ui/ui
Length of output: 792
🏁 Script executed:
# Check jest configuration
cat jest.config.tsRepository: rad-ui/ui
Length of output: 1456
Add ResizeObserver availability guard to prevent runtime crashes in older browsers.
The project targets ES5 and lacks a production-ready ResizeObserver polyfill. The test setup includes a mock, but this doesn't prevent crashes when the component runs in production on browsers that don't support ResizeObserver. Add the feature check:
useEffect(() => {
if (!open || !ref.current || transitionDuration === 0) return;
+ if (typeof ResizeObserver === 'undefined') return;
const resizeObserver = new ResizeObserver(() => {📝 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 ResizeObserver to handle dynamic content changes | |
| useEffect(() => { | |
| if (!open || !ref.current || transitionDuration === 0) return; | |
| const resizeObserver = new ResizeObserver(() => { | |
| if (ref.current && heightRef.current !== undefined) { | |
| const newHeight = ref.current.scrollHeight; | |
| if (newHeight !== heightRef.current) { | |
| setHeight(newHeight); | |
| } | |
| } | |
| }); | |
| // Observe the first child if possible for more accurate content measurement, | |
| // or the ref itself if it's not currently animating height: 0 | |
| resizeObserver.observe(ref.current); | |
| return () => resizeObserver.disconnect(); | |
| }, [open, transitionDuration]); | |
| // Use ResizeObserver to handle dynamic content changes | |
| useEffect(() => { | |
| if (!open || !ref.current || transitionDuration === 0) return; | |
| if (typeof ResizeObserver === 'undefined') return; | |
| const resizeObserver = new ResizeObserver(() => { | |
| if (ref.current && heightRef.current !== undefined) { | |
| const newHeight = ref.current.scrollHeight; | |
| if (newHeight !== heightRef.current) { | |
| setHeight(newHeight); | |
| } | |
| } | |
| }); | |
| // Observe the first child if possible for more accurate content measurement, | |
| // or the ref itself if it's not currently animating height: 0 | |
| resizeObserver.observe(ref.current); | |
| return () => resizeObserver.disconnect(); | |
| }, [open, transitionDuration]); |
🤖 Prompt for AI Agents
In `@src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveContent.tsx`
around lines 47 - 65, The ResizeObserver usage in the useEffect can crash on
browsers without support; inside the useEffect (before creating new
ResizeObserver) add a guard that checks for runtime availability (e.g. ensure
typeof window !== "undefined" and 'ResizeObserver' in window or typeof
ResizeObserver !== "undefined") and simply return if it's not available; keep
the rest of the logic using ref, heightRef, setHeight, open and
transitionDuration unchanged so tests/mocks still work.
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/ui/ScrollArea/fragments/ScrollAreaRoot.tsx (1)
78-113:⚠️ Potential issue | 🟡 MinorPrevent overflow from being true when viewport size is non‑measurable.
If
viewportHeight/viewportWidthis<= 0but content is larger,hasV/hasHbecomestrue, which can surface scrollbars while layout is still settling. Consider guarding overflow on a positive viewport size.🔧 Suggested fix
- const hasV = contentHeight > viewportHeight; + const hasV = viewportHeight > 0 && contentHeight > viewportHeight; @@ - const hasH = contentWidth > viewportWidth; + const hasH = viewportWidth > 0 && contentWidth > viewportWidth;
🤖 Fix all issues with AI agents
In `@src/core/primitives/Combobox/fragments/ComboboxPrimitiveItem.tsx`:
- Around line 70-75: The early return in the ComboboxPrimitiveItem component
breaks the Rules of Hooks because it prevents subsequent hooks (useRef,
Floater.useListItem, useComboboxGroupContext, and the useEffect calls that call
bumpLabelsVersion) from being invoked on every render; replace that pattern by
creating a required-context helper (e.g., useRequiredComboboxGroupContext) that
throws if the context is missing and call it unconditionally at the top of
ComboboxPrimitiveItem, then remove the conditional early return so that
bumpLabelsVersion, useRef, Floater.useListItem, useComboboxGroupContext (via the
required hook), and all useEffect hooks are always executed in the same order;
ensure the helper references the same context used by useComboboxGroupContext so
existing logic continues to work.
🧹 Nitpick comments (3)
src/components/ui/Slider/fragments/SliderThumb.tsx (1)
113-116: Hard-coded 12px offset assumes fixed thumb dimensions.The positioning calculation uses a fixed
12pxoffset for centering. This works for a default thumb size but may misalign if consumers customize thumb dimensions via CSS.Consider using CSS custom properties or CSS
transform: translateX(-50%)/translateY(-50%)for more flexible centering that adapts to any thumb size.package.json (1)
285-285: Remove unusedts-nodedependency.No usage of
ts-nodewas found in the codebase. The project already usestsxfor direct TypeScript file execution (e.g.,generate-tokensscript) andts-jestfor Jest test execution. Addingts-nodeintroduces a redundant dev dependency without a clear purpose. Additionally, thetsconfig.jsonuses"module": "ESNext"and"moduleResolution": "node"rather than the TypeScript-recommended"NodeNext"/"Node16"settings for proper ESM tooling compatibility.src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveContent.tsx (1)
68-85: Avoid duplicate ResizeObservers—consolidate to one effect.
There are now two near‑identical ResizeObserver effects (this one and the earlier block). That registers two observers on the same element and duplicates callbacks/state updates. Recommend removing the earlier effect and keeping this heightRef‑based version.
| React.useEffect(() => { | ||
| bumpLabelsVersion(); | ||
| return () => { | ||
| bumpLabelsVersion(); | ||
| }; | ||
| }, [index, value, bumpLabelsVersion]); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
React rules of hooks early return before hooks violation
💡 Result:
An early return before some Hooks run is a Rules of Hooks violation because React must see the same Hooks called in the same order on every render. If one render returns early, React may see “fewer hooks than expected” and can’t reliably match Hook state to the right call site. The official lint rule explicitly lists “Hooks after early returns” as a common violation. [1][2]
The violating pattern
function MyComp({ user }) {
if (!user) return null; // ❌ early return
const [x, setX] = useState(0); // ❌ hook after early return
useEffect(() => {}, []);
return <div />;
}Fix patterns
1) Call Hooks first; return after
function MyComp({ user }) {
const [x, setX] = useState(0);
useEffect(() => {}, []);
if (!user) return null; // ✅ OK (after hooks)
return <div />;
}2) Split into a child component
function MyComp({ user }) {
if (!user) return null; // ✅ no hooks here
return <Authed user={user} />;
}
function Authed({ user }) {
const [x, setX] = useState(0); // ✅ always runs when Authed renders
return <div />;
}3) Keep the Hook call unconditional; make its inputs/behavior conditional
function MyComp({ user }) {
const enabled = !!user;
useEffect(() => {
if (!enabled) return;
// do the effect
}, [enabled]);
if (!user) return null;
return <div />;
}Notes:
- This applies to built-in Hooks and custom Hooks (anything that’s a Hook must obey the same rule). [2]
- React’s special
useAPI is an exception (it can be conditional), but normal Hooks likeuseState/useEffectcannot. [1]
Sources: React “Rules of Hooks” and the rules-of-hooks ESLint rule docs. [1][2]
🏁 Script executed:
find . -name "ComboboxPrimitiveItem.tsx" -type fRepository: rad-ui/ui
Length of output: 119
🏁 Script executed:
cat -n ./src/core/primitives/Combobox/fragments/ComboboxPrimitiveItem.tsx | head -100Repository: rad-ui/ui
Length of output: 3946
Fix hook ordering: early return before hooks violates Rules of Hooks.
The conditional early return at line 23 prevents all subsequent hooks (useRef at line 44, Floater.useListItem at line 45, useComboboxGroupContext at line 54, and multiple useEffect hooks) from being called on every render, breaking the Rules of Hooks. Create a required-context hook to ensure all hooks are called unconditionally.
Suggested fix
+const useRequiredComboboxContext = () => {
+ const context = useContext(ComboboxPrimitiveContext);
+ if (!context) {
+ throw new Error('ComboboxPrimitiveItem must be used within a ComboboxPrimitive');
+ }
+ return context;
+};
+
const ComboboxPrimitiveItem = React.forwardRef<
React.ElementRef<typeof Primitive.div>,
ComboboxPrimitiveItemProps & React.ComponentPropsWithoutRef<typeof Primitive.div>
>(({ children, value, disabled, className, ...props }, forwardedRef) => {
- const context = useContext(ComboboxPrimitiveContext);
-
- if (!context) {
- console.error('ComboboxPrimitiveItem must be used within a ComboboxPrimitive');
- return null;
- }
+ const context = useRequiredComboboxContext();🧰 Tools
🪛 Biome (2.3.13)
[error] 70-70: 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)
🤖 Prompt for AI Agents
In `@src/core/primitives/Combobox/fragments/ComboboxPrimitiveItem.tsx` around
lines 70 - 75, The early return in the ComboboxPrimitiveItem component breaks
the Rules of Hooks because it prevents subsequent hooks (useRef,
Floater.useListItem, useComboboxGroupContext, and the useEffect calls that call
bumpLabelsVersion) from being invoked on every render; replace that pattern by
creating a required-context helper (e.g., useRequiredComboboxGroupContext) that
throws if the context is missing and call it unconditionally at the top of
ComboboxPrimitiveItem, then remove the conditional early return so that
bumpLabelsVersion, useRef, Floater.useListItem, useComboboxGroupContext (via the
required hook), and all useEffect hooks are always executed in the same order;
ensure the helper references the same context used by useComboboxGroupContext so
existing logic continues to work.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Style
Chores
✏️ Tip: You can customize this high-level summary in your review settings.