Code improvements - 13 jan #1674
Conversation
…nd fix Storybook mismatches
|
📝 WalkthroughWalkthroughThis pull request introduces new utility functions for composing refs and merging props, refactors Steps components to use a custom hook instead of direct context access, updates type assertions in story files, improves component props typing, and adds tests for asChild prop merging. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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: 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/Minimap/fragments/MinimapWaypoint.tsx (1)
15-23: Remove debug console.log statements.The
console.logstatements on lines 16 and 22 appear to be debug artifacts that should be removed before merging to production.🧹 Proposed fix
const handleEnter = React.useCallback(() => { - console.log('waypoint', value, 'in view'); handleInView(value); }, [value, handleInView]); const handleLeave = React.useCallback(() => { - console.log('waypoint', value, 'out of view'); handleOutView(value); }, [value, handleOutView]);
🤖 Fix all issues with AI agents
In @src/components/ui/Steps/fragments/StepItem.tsx:
- Around line 11-14: StepItem currently destructures value and { currentStep,
setCurrentStep } from useStepsContext() but never uses them; either remove the
unused destructuring and the value prop from StepItemProps, or implement step
behavior: use value and currentStep to compute state (e.g., active/completed
classes based on value === currentStep or value < currentStep) and call
setCurrentStep(value) on click to change steps; update the JSX in StepItem to
apply conditional classes (using rootClass from useStepsContext()) and an
onClick that calls setCurrentStep when value is provided, or if you intend to
leave them unused for now, add a TODO comment next to the destructuring in
StepItem to prevent lint failures.
🧹 Nitpick comments (5)
src/components/ui/Minimap/fragments/MinimapWaypoint.tsx (1)
32-42: Consider using the newcomposeRefsutility for cleaner ref composition.The PR introduces a
composeRefsutility insrc/core/utils/mergeProps.tsthat handles composing multiple refs robustly. SinceuseInViewreturns an object ref and the component needs to forward the DOM node to bothuseInViewand the provider'sregisterRef, this can be simplified usingcomposeRefs:const registerCallback = React.useCallback((node: HTMLDivElement | null) => { if (value) { registerRef(value, node); } }, [value, registerRef]); const combinedRef = composeRefs(ref, registerCallback);This removes the manual ref handling and aligns with how other components (e.g.,
Primitive) usecomposeRefs. Import the utility from~/corealongside the existing imports.src/components/ui/Checkbox/stories/Checkbox.stories.tsx (1)
17-17: Type assertion may mask indeterminate state handling.If the Checkbox component supports an indeterminate state,
valcould be'indeterminate'rather than a boolean. Usingval as booleanwould silently pass through the string value, causing unexpected behavior.Also, this approach is inconsistent with the
WithLabelstory (lines 75-76) which doesn't use a type assertion.Consider a safer approach:
Suggested fix
- <Checkbox.Root id="accept-terms" checked={checked} onCheckedChange={(val) => setChecked(val as boolean)}> + <Checkbox.Root id="accept-terms" checked={checked} onCheckedChange={(val) => setChecked(val === true)}>src/components/ui/Disclosure/stories/Disclosure.stories.tsx (1)
5-15: Consider using proper Storybook types instead ofas any.Casting to
anybypasses type checking entirely. Storybook provides proper type utilities that maintain type safety.Example using Storybook types
+import type { Meta } from '@storybook/react'; + -export default { +export default { title: 'WIP/Disclosure', component: Disclosure, render: (args: JSX.IntrinsicAttributes & DisclosureProps) => <SandboxEditor> <div> <Disclosure {...args} /> </div> </SandboxEditor> -} as any; +} satisfies Meta<typeof Disclosure>;src/core/utils/mergeProps.ts (1)
62-72: Consider preserving handler return values.The composed event handler doesn't return any value. While React DOM event handlers typically don't use return values, some edge cases (like
onSubmitreturning false to prevent submission in legacy patterns) might be affected.♻️ Optional: Return child handler result
mergedProps[propName] = (...args: unknown[]) => { // Run the child handler first so the child has priority. - childPropValue(...args); + const result = childPropValue(...args); // If child calls event.preventDefault(), treat it as "cancel parent behavior". const event = args[0] as { defaultPrevented?: boolean } | undefined; if (!event || event.defaultPrevented !== true) { slotPropValue(...args); } + return result; };src/core/primitives/Primitive/tests/Primitive.asChild.test.tsx (1)
41-59: Good test coverage for the new merging behavior.The test properly validates both className concatenation and event handler composition. Consider adding an additional test case to verify the
preventDefaultbehavior (that callinge.preventDefault()in the child handler prevents the parent handler from running).💡 Optional: Add test for preventDefault behavior
test('child preventDefault stops parent handler', async() => { const user = userEvent.setup(); const parentClick = jest.fn(); const childClick = jest.fn((e: React.MouseEvent) => e.preventDefault()); render( <Primitive.button asChild onClick={parentClick}> <button onClick={childClick}>Trigger</button> </Primitive.button> ); const button = screen.getByRole('button'); await user.click(button); expect(childClick).toHaveBeenCalledTimes(1); expect(parentClick).not.toHaveBeenCalled(); });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
src/components/ui/Checkbox/stories/Checkbox.stories.tsxsrc/components/ui/Combobox/stories/Combobox.stories.tsxsrc/components/ui/Dialog/stories/Dialog.stories.tsxsrc/components/ui/Disclosure/stories/Disclosure.stories.tsxsrc/components/ui/Minimap/fragments/MinimapTrack.tsxsrc/components/ui/Minimap/fragments/MinimapWaypoint.tsxsrc/components/ui/Minimap/stories/Minimap.stories.tsxsrc/components/ui/RadioCards/RadioCards.stories.tsxsrc/components/ui/Select/stories/Select.stories.tsxsrc/components/ui/Slider/stories/Slider.stories.tsxsrc/components/ui/Steps/fragments/StepBubble.tsxsrc/components/ui/Steps/fragments/StepContent.tsxsrc/components/ui/Steps/fragments/StepDescription.tsxsrc/components/ui/Steps/fragments/StepItem.tsxsrc/components/ui/Steps/fragments/StepLine.tsxsrc/components/ui/Steps/fragments/StepRoot.tsxsrc/components/ui/Steps/fragments/StepTitle.tsxsrc/components/ui/Steps/fragments/StepTrack.tsxsrc/core/index.tssrc/core/primitives/Primitive/index.tsxsrc/core/primitives/Primitive/tests/Primitive.asChild.test.tsxsrc/core/utils/mergeProps.ts
🧰 Additional context used
🧠 Learnings (4)
📚 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/Select/stories/Select.stories.tsxsrc/components/ui/Slider/stories/Slider.stories.tsxsrc/components/ui/Combobox/stories/Combobox.stories.tsxsrc/components/ui/Checkbox/stories/Checkbox.stories.tsxsrc/components/ui/Dialog/stories/Dialog.stories.tsx
📚 Learning: 2025-08-13T15:45:46.806Z
Learnt from: CR
Repo: rad-ui/ui PR: 0
File: docs/.cursor/rules/posthog-integration.mdc:0-0
Timestamp: 2025-08-13T15:45:46.806Z
Learning: Applies to docs/**/*.{ts,tsx,js,jsx} : If a custom person/event property is referenced in two or more places, define it in a central enum (TS) or const object (JS) and reuse that reference
Applied to files:
src/core/utils/mergeProps.ts
📚 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/core/primitives/Primitive/tests/Primitive.asChild.test.tsxsrc/core/primitives/Primitive/index.tsxsrc/components/ui/Checkbox/stories/Checkbox.stories.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/Checkbox/stories/Checkbox.stories.tsx
🧬 Code graph analysis (9)
src/components/ui/Steps/fragments/StepTitle.tsx (1)
src/components/ui/Steps/context/StepsContext.tsx (1)
useStepsContext(14-20)
src/core/utils/mergeProps.ts (1)
src/core/index.ts (2)
composeRefs(4-4)mergeProps(4-4)
src/components/ui/Steps/fragments/StepBubble.tsx (1)
src/components/ui/Steps/context/StepsContext.tsx (1)
useStepsContext(14-20)
src/components/ui/Steps/fragments/StepDescription.tsx (1)
src/components/ui/Steps/context/StepsContext.tsx (1)
useStepsContext(14-20)
src/components/ui/Steps/fragments/StepTrack.tsx (1)
src/components/ui/Steps/context/StepsContext.tsx (1)
useStepsContext(14-20)
src/core/primitives/Primitive/index.tsx (1)
src/core/utils/mergeProps.ts (2)
composeRefs(15-34)mergeProps(47-91)
src/components/ui/Steps/fragments/StepItem.tsx (1)
src/components/ui/Steps/context/StepsContext.tsx (1)
useStepsContext(14-20)
src/components/ui/Steps/fragments/StepLine.tsx (1)
src/components/ui/Steps/context/StepsContext.tsx (1)
useStepsContext(14-20)
src/components/ui/Steps/fragments/StepContent.tsx (1)
src/components/ui/Steps/context/StepsContext.tsx (1)
useStepsContext(14-20)
⏰ 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). (3)
- GitHub Check: Cursor Bugbot
- GitHub Check: Jest (1/2)
- GitHub Check: coverage
🔇 Additional comments (16)
src/components/ui/Dialog/stories/Dialog.stories.tsx (1)
47-47: Acceptable use ofas anyfor Storybook meta export.Using
as anyon Storybook's default export is a common workaround when the component's types don't perfectly align with Storybook'sMetatype expectations. This is consistent with the pattern applied across other story files in this PR.src/components/ui/Minimap/stories/Minimap.stories.tsx (1)
35-35: LGTM!The change from
value={index}tovalue={index.toString()}correctly aligns with the updatedMinimap.Itemprop type that now expects astringinstead of anumber.src/components/ui/Combobox/stories/Combobox.stories.tsx (1)
6-9: Consistent with PR-wide pattern.The
as anyassertion on the default export follows the same pattern applied to other story files in this PR for Storybook type compatibility.src/components/ui/Slider/stories/Slider.stories.tsx (1)
96-96: Remove the unnecessaryas anycast fromdefaultValue.The
RangeSlidercomponent's type definition already correctly accepts[number, number]fordefaultValue, so theas anycast is redundant and can be safely removed.Also applies to: 160-160, 238-238
Likely an incorrect or invalid review comment.
src/components/ui/Minimap/fragments/MinimapTrack.tsx (1)
6-10: LGTM!Good improvement to explicitly type the props. This ensures type safety for consumers while allowing all standard
HTMLDivElementattributes to be passed through.src/core/utils/mergeProps.ts (2)
15-34: Well-implemented ref composition utility.The implementation correctly handles both function and object refs, properly skips null/undefined refs, and returns undefined when no refs are provided to avoid attaching a no-op ref. The documentation is clear and explains the use case well.
1-7: Clean utility module setup.The type definitions and regex pattern are appropriate for the use case. The
AnyPropstype is intentionally loose to handle arbitrary React props shapes, which is the right approach for a generic merge utility.src/core/index.ts (1)
2-4: Clean public API extension.The new utilities are properly imported and re-exported alongside existing exports, maintaining a clean module boundary.
src/core/primitives/Primitive/index.tsx (1)
37-44: Good use of the new merge utilities.The refactor correctly centralizes ref composition and prop merging. The
(child as any).refcast is a common pattern since React'sReactElementtype doesn't exposerefdirectly, though it exists at runtime.src/components/ui/Steps/fragments/StepTrack.tsx (1)
4-9: Clean refactor to use the context hook.Using
useStepsContext()instead of direct context consumption provides better error handling (throws a descriptive error if used outside the provider) and improves code consistency across all Steps fragments.src/components/ui/Steps/fragments/StepLine.tsx (1)
4-9: Consistent hook-based context access.Same clean refactor pattern as other Steps fragments, using the
useStepsContexthook for better error handling and consistency.src/components/ui/Steps/fragments/StepBubble.tsx (1)
4-9: Consistent refactor with explicit prop typing.Good improvement adding the explicit
StepBubblePropstype alias alongside the hook migration. This matches the pattern established in other Steps fragments and improves code clarity.src/components/ui/Steps/fragments/StepDescription.tsx (1)
4-9: LGTM!Clean refactor to use the
useStepsContexthook. This approach provides better error handling when the component is used outside theStepsContext.Provider, as the hook throws a descriptive error.src/components/ui/Steps/fragments/StepTitle.tsx (1)
4-9: LGTM!Clean refactor to use
useStepsContext. The addedStepTitlePropstype alias improves readability and maintains consistency with other Step fragments.src/components/ui/Steps/fragments/StepContent.tsx (1)
4-9: LGTM!Consistent refactor using
useStepsContextwith proper type alias. Aligns well with the pattern established in other Step fragments.src/components/ui/Steps/fragments/StepRoot.tsx (1)
10-21: Good addition of orientation support.The implementation properly:
- Adds typed
orientationprop with sensible default ('horizontal')- Propagates orientation through context for child components
- Applies orientation-based CSS class for styling
The
StepsContexttype definition correctly includes theorientationproperty asStepsOrientation, matching the context provider usage in StepRoot.tsx. Type safety is maintained.
| const StepItem = ({ children, value = null, className = '', ...props }: StepItemProps) => { | ||
| const { rootClass, currentStep, setCurrentStep } = React.useContext(StepsContext); | ||
| const { rootClass, currentStep, setCurrentStep } = useStepsContext(); | ||
| return <div className={clsx(`${rootClass}-item`, className)} {...props}>{children}</div>; | ||
| }; |
There was a problem hiding this comment.
Unused variables: value, currentStep, and setCurrentStep.
The component destructures currentStep and setCurrentStep from context but never uses them. Additionally, the value prop is accepted but not utilized. This appears to be incomplete implementation - typically a StepItem would use these to determine active/completed states or handle click interactions.
Suggested options
Option 1: If these are needed for future implementation, add a TODO comment:
const StepItem = ({ children, value = null, className = '', ...props }: StepItemProps) => {
- const { rootClass, currentStep, setCurrentStep } = useStepsContext();
+ const { rootClass } = useStepsContext();
+ // TODO: Implement step selection using value, currentStep, and setCurrentStep
return <div className={clsx(`${rootClass}-item`, className)} {...props}>{children}</div>;
};Option 2: If these are intended to be used now, implement the step logic:
const StepItem = ({ children, value = null, className = '', ...props }: StepItemProps) => {
const { rootClass, currentStep, setCurrentStep } = useStepsContext();
- return <div className={clsx(`${rootClass}-item`, className)} {...props}>{children}</div>;
+ const isActive = value === currentStep;
+ return (
+ <div
+ className={clsx(`${rootClass}-item`, { [`${rootClass}-item-active`]: isActive }, className)}
+ onClick={() => value !== null && setCurrentStep(value)}
+ {...props}
+ >
+ {children}
+ </div>
+ );
};📝 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 StepItem = ({ children, value = null, className = '', ...props }: StepItemProps) => { | |
| const { rootClass, currentStep, setCurrentStep } = React.useContext(StepsContext); | |
| const { rootClass, currentStep, setCurrentStep } = useStepsContext(); | |
| return <div className={clsx(`${rootClass}-item`, className)} {...props}>{children}</div>; | |
| }; | |
| const StepItem = ({ children, value = null, className = '', ...props }: StepItemProps) => { | |
| const { rootClass } = useStepsContext(); | |
| // TODO: Implement step selection using value, currentStep, and setCurrentStep | |
| return <div className={clsx(`${rootClass}-item`, className)} {...props}>{children}</div>; | |
| }; |
| const StepItem = ({ children, value = null, className = '', ...props }: StepItemProps) => { | |
| const { rootClass, currentStep, setCurrentStep } = React.useContext(StepsContext); | |
| const { rootClass, currentStep, setCurrentStep } = useStepsContext(); | |
| return <div className={clsx(`${rootClass}-item`, className)} {...props}>{children}</div>; | |
| }; | |
| const StepItem = ({ children, value = null, className = '', ...props }: StepItemProps) => { | |
| const { rootClass, currentStep, setCurrentStep } = useStepsContext(); | |
| const isActive = value === currentStep; | |
| return ( | |
| <div | |
| className={clsx(`${rootClass}-item`, { [`${rootClass}-item-active`]: isActive }, className)} | |
| onClick={() => value !== null && setCurrentStep(value)} | |
| {...props} | |
| > | |
| {children} | |
| </div> | |
| ); | |
| }; |
🤖 Prompt for AI Agents
In @src/components/ui/Steps/fragments/StepItem.tsx around lines 11 - 14,
StepItem currently destructures value and { currentStep, setCurrentStep } from
useStepsContext() but never uses them; either remove the unused destructuring
and the value prop from StepItemProps, or implement step behavior: use value and
currentStep to compute state (e.g., active/completed classes based on value ===
currentStep or value < currentStep) and call setCurrentStep(value) on click to
change steps; update the JSX in StepItem to apply conditional classes (using
rootClass from useStepsContext()) and an onClick that calls setCurrentStep when
value is provided, or if you intend to leave them unused for now, add a TODO
comment next to the destructuring in StepItem to prevent lint failures.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
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.
| <Slider.Root defaultValue={500} min={0} max={1000} step={50}> | ||
| <Slider.Track> | ||
| <Slider.RangeSlider aria-label="Price range" defaultValue={[100, 500]} /> | ||
| <Slider.RangeSlider aria-label="Range slider" defaultValue={[25, 75] as any} /> |
There was a problem hiding this comment.
PriceRangeSlider story uses incorrect default values
Low Severity
The PriceRangeSlider story's defaultValue was changed from [100, 500] to [25, 75], and the aria-label from "Price range" to "Range slider". Since this slider has min={0} and max={1000} with price marks at $0 through $1000, the original values representing a $100-$500 range made sense. The new values [25, 75] appear to be mistakenly copied from the basic RangeSlider story and don't represent meaningful prices in this context.
Note
Core/Primitives
core/utils/mergeProps.tswithmergeProps(composes event handlers, merges className/style) andcomposeRefsutilities; exported viacore/index.ts.Primitivenow uses these utilities forasChild, properly composing refs/handlers and merging props; new tests cover className merge, handler composition, and ref forwarding.Steps
useStepsContextand explicit prop types;StepItem.valuenow acceptsstring | number.Steps.Rootaddsorientationprop and provides it via context; root element gets${rootClass}-${orientation}class.Minimap
MinimapTracktyped props;MinimapWaypointref handling simplified; stories passvalueasstring.Stories (misc)
Written by Cursor Bugbot for commit 6cc631a. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Breaking Changes
valueprop now accepts string instead of number; update calls to useindex.toString().valueprop now accepts string | number (previously string only).Improvements
✏️ Tip: You can customize this high-level summary in your review settings.