Chores and improvements - 13 Aug 2025 #1334
Conversation
WalkthroughMultiple UI component updates: formatting fixes in README and SVG markup, behavior and lifecycle improvements for HoverCard and ScrollAreaScrollbar, controlled/uncontrolled handling fixes in Tab/TabNav, Theme default and system-color handling with tests, a Link props type change, test refactors, and minor cleanup in Splitter. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Trigger as HoverCard.Trigger
participant HoverRoot as HoverCard.Root
participant Context as HoverCardContext
User->>Trigger: mouseenter / click
Trigger->>HoverRoot: notify open request
HoverRoot->>Context: call openWithDelay()
Context->>HoverRoot: schedule timeout (openTimeoutRef)
HoverRoot-->>Context: setOpen(true) via useControllableState (onOpenChange handled by hook)
User->>Trigger: mouseleave
HoverRoot->>Context: call closeWithDelay()
Context->>HoverRoot: schedule/clear closeTimeoutRef
HoverRoot-->>Context: setOpen(false)
sequenceDiagram
participant User
participant Thumb as ScrollArea.Thumb
participant Scrollbar as ScrollAreaScrollbar
User->>Thumb: mousedown & hold (mouseY)
Thumb->>Scrollbar: startContinuousScroll(e)
Scrollbar->>Scrollbar: immediate scroll step (mousePositionRef)
Scrollbar->>Scrollbar: after 300ms start interval (50ms)
loop while shouldContinueScrolling(mouseY)
Scrollbar->>Scrollbar: repeated scroll steps
end
User->>Scrollbar: mouseup / mouseleave
Scrollbar->>Scrollbar: stopContinuousScroll() -> clear interval, cleanup listeners
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
15-15: Nit: Smooth the link phrasing and casing.Consider lowercasing the link text and adding “the” to read more naturally.
Apply this diff:
-For full documentation, visit [Documentation](https://www.rad-ui.com/docs/first-steps/introduction). +For full documentation, visit the [documentation](https://www.rad-ui.com/docs/first-steps/introduction).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(1 hunks)
🔇 Additional comments (1)
README.md (1)
15-15: LGTM: Whitespace fix is correct.Removing the extra space improves readability and keeps the docs tidy.
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (4)
src/components/ui/Link/Link.tsx (2)
22-26: Rename rest param and drop ts-ignore; ensure native attrs pass throughAfter retyping LinkProps, forward native anchor attributes via a clearly named rest param and remove the ts-ignore.
Apply this diff:
-const Link = ({ children, href = '#', customRootClass, className, size = '', ...props }: LinkProps) => { +const Link = ({ children, href = '#', customRootClass, className, size = '', ...rest }: LinkProps) => { const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME); const dataAttributes = useCreateDataAttribute('link', { size }); - // @ts-ignore - return <Primitive.a href={href} className={clsx(rootClass, className)} {...dataAttributes()} {...props}>{children}</Primitive.a>; + return ( + <Primitive.a + href={href} + className={clsx(rootClass, className)} + {...dataAttributes()} + {...rest} + > + {children} + </Primitive.a> + ); };If TS still complains about spreading
dataAttributes(), consider giving the hook a precise return type or locally asserting:{...(dataAttributes() as React.HTMLAttributes<HTMLAnchorElement>)}
10-17: Fix LinkProps to Extend Anchor Attributes and Rename Rest ParameterTo eliminate the
propscollision and properly type all native<a>attributes:
- Remove the custom
props?: Record<string, any>field fromLinkProps.- Extend
React.ComponentPropsWithoutRef<'a'>so consumers can pass any valid anchor props (e.g.target,rel, etc.).- Rename the destructured rest parameter from
...propsto...rest.- Update the JSX spread to
{...rest}.- No usages of
<Link props={…}>were found in the codebase, so no migration of call sites is required.Apply this diff in
src/components/ui/Link/Link.tsx:-export type LinkProps = { - children: React.ReactNode; - href: string; - customRootClass?: string; - size?: string; - className?: string; - props?: Record<string, any>; -} +export type LinkProps = React.ComponentPropsWithoutRef<'a'> & { + customRootClass?: string; + size?: string; +}; -const Link = ({ children, href = '#', customRootClass, className, size = '', ...props }: LinkProps) => { +const Link = ({ + children, + href = '#', + className, + customRootClass, + size = '', + ...rest +}: LinkProps) => { const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME); const dataAttributes = useCreateDataAttribute('link', { size }); - // @ts-ignore - return <Primitive.a href={href} className={clsx(rootClass, className)} {...dataAttributes()} {...props}>{children}</Primitive.a>; + return ( + // @ts-ignore + <Primitive.a + href={href} + className={clsx(rootClass, className)} + {...dataAttributes()} + {...rest} + > + {children} + </Primitive.a> + ); };src/components/ui/ScrollArea/fragments/ScrollAreaScrollbar.tsx (2)
41-66: Untracked startup delay timer; potential leak and late firing after stop/unmount
setTimeoutis used to defer the start of continuous scrolling but its ID isn’t stored/cleared. If scrolling is stopped or the component unmounts before the delay elapses, the callback may run later than intended.+ // delay before the continuous scroll starts + const delayTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null); @@ - // Start continuous scrolling after a brief delay - setTimeout(() => { + // Start continuous scrolling after a brief delay + delayTimeoutRef.current = window.setTimeout(() => { if (isScrollingRef.current) { intervalRef.current = setInterval(() => { if (isScrollingRef.current && shouldContinueScrolling(mousePositionRef.current)) { handleScrollbarClick({ clientY: mousePositionRef.current }); } else { // Stop scrolling if thumb reached mouse position stopContinuousScroll(); } }, 50); // Scroll every 50ms } - }, 300); // 300ms delay before continuous scrolling starts + }, 300); // 300ms delay before continuous scrolling startsAnd clear it when stopping:
const stopContinuousScroll = useCallback(() => { isScrollingRef.current = false; removeListenersRef.current?.(); removeListenersRef.current = null; + if (delayTimeoutRef.current) { + clearTimeout(delayTimeoutRef.current); + delayTimeoutRef.current = null; + } if (intervalRef.current) { clearInterval(intervalRef.current); intervalRef.current = null; } setIsScrolling(false); }, []);
24-38: Ensure Orientation-aware Scroll LogicThe
ScrollAreaScrollbarcurrently ignores theorientationprop and always uses a singlescrollXThumbRefwithclientY/top–bottomchecks—there’s no support for horizontal scrolling (clientX/left–right), and the context doesn’t expose ascrollYThumbRef. We should either remove the unusedorientationprop (and renamescrollXThumbReftoscrollThumbRefif only vertical mode is needed) or fully implement orientation support by:
- Adding
scrollYThumbRef?: RefObject<HTMLDivElement>toScrollAreaContextType- Initializing and providing
scrollYThumbRefalongsidescrollXThumbRefinScrollAreaRoot.tsx- Updating
ScrollAreaScrollbar.tsxto destructurescrollYThumbRefand switch axis/ref based onorientationLocations to update:
- src/components/ui/ScrollArea/context/ScrollAreaContext.tsx
- src/components/ui/ScrollArea/fragments/ScrollAreaRoot.tsx
- src/components/ui/ScrollArea/fragments/ScrollAreaScrollbar.tsx (lines 12–38)
Proposed diff for
shouldContinueScrollingin ScrollAreaScrollbar.tsx:- const { rootClass, handleScrollbarClick, scrollXThumbRef } = useContext(ScrollAreaContext); + const { rootClass, handleScrollbarClick, scrollXThumbRef, scrollYThumbRef } = useContext(ScrollAreaContext); - const shouldContinueScrolling = useCallback((mouseY: number): boolean => { - if (!scrollXThumbRef?.current) return false; - - const thumbRect = scrollXThumbRef.current.getBoundingClientRect(); - const thumbStart = thumbRect.top; - const thumbEnd = thumbRect.bottom; - - // Stop if mouse is within thumb bounds - if (mouseY >= thumbStart && mouseY <= thumbEnd) { - return false; - } - - return true; - }, [scrollXThumbRef]); + const shouldContinueScrolling = useCallback((mousePos: number): boolean => { + // Choose the correct thumb and axis per orientation + const thumbRef = orientation === 'horizontal' ? scrollXThumbRef : scrollYThumbRef; + if (!thumbRef?.current) return false; + + const rect = thumbRef.current.getBoundingClientRect(); + const [start, end] = + orientation === 'horizontal' ? [rect.left, rect.right] : [rect.top, rect.bottom]; + + // Stop when pointer enters the thumb bounds + return !(mousePos >= start && mousePos <= end); + }, [orientation, scrollXThumbRef, scrollYThumbRef]);
🧹 Nitpick comments (7)
src/components/ui/Menubar/stories/Menubar.stories.tsx (1)
14-20: Mark decorative icon as aria-hidden to avoid SR noiseChevronRight is purely decorative in SubTrigger. Hide it from assistive tech and prevent accidental focus.
Apply this diff:
<svg width="15" height="15" viewBox="0 0 15 15" fill="none" xmlns="http://www.w3.org/2000/svg" + aria-hidden="true" + focusable="false" >src/components/ui/VisuallyHidden/stories/VisuallyHidden.stories.tsx (1)
8-22: SVG camelCase conversion looks good; optionally allow prop overrides on icons
- The JSX-friendly attributes and multi-line formatting are correct. Icons are wrapped with aria-hidden at usage sites, which is ideal.
- Optional: expose standard SVG props to make these icons reusable (e.g., size/className overrides in stories).
Apply this diff for both icons:
-const EmailIcon = () => { +const EmailIcon = (props: React.SVGProps<SVGSVGElement>) => { return ( <svg width="15" height="15" viewBox="0 0 15 15" fill="none" xmlns="http://www.w3.org/2000/svg" + {...props} >-const SearchIcon = () => { +const SearchIcon = (props: React.SVGProps<SVGSVGElement>) => { return ( <svg width="15" height="15" viewBox="0 0 15 15" fill="none" xmlns="http://www.w3.org/2000/svg" + {...props} >Also applies to: 26-41
src/components/ui/Checkbox/fragments/CheckboxIndicator.tsx (1)
14-21: Hide indicator SVG from assistive techThe checkbox state should be conveyed by the control semantics; the indicator SVG is decorative. Mark it aria-hidden and not focusable to avoid duplicate announcements.
Apply this diff:
<svg width="15" height="15" viewBox="0 0 15 15" className={clsx(`${rootClass}-indicator`, className)} fill="none" xmlns="http://www.w3.org/2000/svg" + aria-hidden="true" + focusable="false" >src/components/ui/Link/Link.tsx (1)
19-21: Remove stale comment about 'alt' on anchorsThis comment refers to previous code and adds noise. Safe to remove.
Apply this diff:
-// TODO: in the previous return value -// return <a href={href} alt={alt} className={clsx(rootClass, className)} {...props}>{children}</a>; -// 'alt' prop does not exist on an anchor elementsrc/components/ui/Theme/stories/Theme.stories.tsx (1)
25-27: Story toggle no longer affects Theme; consider aligning UI/wording or exposing a controlWith
appearance="system", the “Toggle Theme” button now only changes the CardComponent text, not the Theme. This can confuse readers of the story.
- Option A: Wire the toggle back to Theme appearance for a light/dark demo.
- Option B: Keep system mode and adjust the button text or add a Storybook control (args) for appearance.
Minor nit: prefer
appearance="system"overappearance={'system'}.- <Theme appearance={'system'}> + <Theme appearance="system">src/components/ui/Theme/tests/Theme.test.tsx (1)
13-47: Solid coverage of system color-scheme lifecycle (init, update, cleanup)The test accurately validates initial light mode, listener registration, dark-mode transition, and cleanup via
removeEventListener. Nicely scoped and readable.Two minor type/robustness improvements:
- Avoid
as unknown as MediaQueryListby providing the minimal shape forMediaQueryListin the stub.- Optionally include
addListener/removeListenerno-ops for older APIs if your component supports them.- const mediaQueryList = { - matches: false, - addEventListener: jest.fn((_event, listener) => { - listeners.push(listener); - }), - removeEventListener: jest.fn((_event, listener) => { - const index = listeners.indexOf(listener); - if (index !== -1) { - listeners.splice(index, 1); - } - }) - } as unknown as MediaQueryList; + const mediaQueryList: MediaQueryList = { + media: '(prefers-color-scheme: dark)', + matches: false, + onchange: null, + addEventListener: jest.fn((_event, listener: EventListenerOrEventListenerObject) => { + listeners.push(listener as (e: MediaQueryListEvent) => void); + }), + removeEventListener: jest.fn((_event, listener: EventListenerOrEventListenerObject) => { + const index = listeners.indexOf(listener as (e: MediaQueryListEvent) => void); + if (index !== -1) listeners.splice(index, 1); + }), + // Legacy APIs (optional) + addListener: jest.fn(), + removeListener: jest.fn(), + dispatchEvent: jest.fn(() => true), + };src/components/ui/ScrollArea/fragments/ScrollAreaScrollbar.tsx (1)
41-66: Optional: track live mouse position for smoother UXComments suggest tracking the “latest mouse Y position”, but the code only captures the initial down position. If live tracking is desired, add a
mousemovelistener while scrolling to updatemousePositionRef.current.document.addEventListener('mouseup', handleMouseUp); document.addEventListener('mouseleave', handleMouseLeave); + const handleMouseMove = (ev: MouseEvent) => { + mousePositionRef.current = orientation === 'vertical' ? ev.clientY : ev.clientX; + }; + document.addEventListener('mousemove', handleMouseMove); @@ removeListenersRef.current = () => { document.removeEventListener('mouseup', handleMouseUp); document.removeEventListener('mouseleave', handleMouseLeave); + document.removeEventListener('mousemove', handleMouseMove); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/components/ui/Checkbox/fragments/CheckboxIndicator.tsx(1 hunks)src/components/ui/HoverCard/fragments/HoverCardRoot.tsx(3 hunks)src/components/ui/HoverCard/stories/HoverCard.stories.tsx(1 hunks)src/components/ui/Link/Link.tsx(1 hunks)src/components/ui/Menubar/stories/Menubar.stories.tsx(1 hunks)src/components/ui/ScrollArea/fragments/ScrollAreaScrollbar.tsx(3 hunks)src/components/ui/Splitter/fragments/SplitterRoot.tsx(2 hunks)src/components/ui/TabNav/fragments/TabNavRoot.tsx(1 hunks)src/components/ui/Table/tests/Table.test.tsx(3 hunks)src/components/ui/Tabs/fragments/TabRoot.tsx(1 hunks)src/components/ui/Theme/Theme.tsx(1 hunks)src/components/ui/Theme/stories/Theme.stories.tsx(1 hunks)src/components/ui/Theme/tests/Theme.test.tsx(1 hunks)src/components/ui/VisuallyHidden/stories/VisuallyHidden.stories.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-04-07T04:38:34.864Z
Learnt from: kotAPI
PR: rad-ui/ui#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/TabNav/fragments/TabNavRoot.tsxsrc/components/ui/Tabs/fragments/TabRoot.tsx
📚 Learning: 2025-07-18T16:43:26.175Z
Learnt from: GoldGroove06
PR: rad-ui/ui#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/TabNav/fragments/TabNavRoot.tsxsrc/components/ui/Splitter/fragments/SplitterRoot.tsxsrc/components/ui/Tabs/fragments/TabRoot.tsx
🧬 Code Graph Analysis (1)
src/components/ui/HoverCard/fragments/HoverCardRoot.tsx (1)
src/core/hooks/useControllableState/index.tsx (1)
useControllableState(15-78)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (14)
src/components/ui/Menubar/stories/Menubar.stories.tsx (1)
24-25: SVG attribute camelCase is correctGood conversion from fill-rule/clip-rule to fillRule/clipRule for React JSX compatibility.
src/components/ui/Checkbox/fragments/CheckboxIndicator.tsx (1)
25-26: CamelCase attributes are correctfillRule/clipRule usage is correct for JSX; no issues spotted.
src/components/ui/Splitter/fragments/SplitterRoot.tsx (2)
2-2: LGTM! Proper useEffect import addition.The useEffect import is correctly added to support the cleanup functionality being introduced.
55-62: Excellent memory leak prevention with proper timeout cleanup.The cleanup useEffect correctly clears the debounced timeout on component unmount, preventing potential memory leaks and callbacks executing after the component is destroyed. The implementation properly checks for the timeout existence before clearing it and sets it to null afterwards.
src/components/ui/TabNav/fragments/TabNavRoot.tsx (1)
39-47: Excellent controlled/uncontrolled state handling improvement.The changes correctly implement the controlled vs uncontrolled component pattern by:
- Only setting the default value when
value === undefined(uncontrolled mode)- Including
valuein the dependency array to handle mode transitions- Adding clear comments explaining the behavior
This aligns with React best practices and the retrieved learning about controlled component implementation.
src/components/ui/Tabs/fragments/TabRoot.tsx (1)
53-63: Perfect controlled/uncontrolled pattern implementation with clear documentation.The implementation correctly follows the controlled component pattern established in the learning from AccordionRoot:
- Only applies
defaultValuein uncontrolled mode (value === undefined)- Includes
valuein dependencies to handle control state transitions- Provides comprehensive comments explaining the behavior
This change maintains consistency with the TabNavRoot implementation and aligns with React best practices.
src/components/ui/Table/tests/Table.test.tsx (4)
2-2: Good addition of thewithinutility.The
withinimport enables more semantic and accessible testing patterns by scoping queries to specific DOM subtrees.
51-54: Improved accessibility-focused testing approach.The change from DOM querying to
getAllByRole('table')provides better semantic testing that aligns with how screen readers and users interact with the component.
57-68: Excellent semantic testing with proper DOM structure validation.The new approach using
within()and role-based queries:
- Properly validates the table structure (rowgroup → row → columnheader hierarchy)
- Uses semantic ARIA roles instead of generic DOM queries
- Maintains the same test coverage with better accessibility validation
This is a significant improvement in test quality.
71-88: Robust data validation with semantic queries.The updated test approach:
- Validates proper table structure with 2 rowgroups (header + body)
- Uses semantic role queries throughout
- Maintains comprehensive data validation
- Follows accessibility-first testing patterns
This provides better confidence in the component's accessibility and structure.
src/components/ui/Theme/Theme.tsx (2)
13-13: Excellent default change to system-aware theming.Changing the default appearance to 'system' provides a better user experience by respecting the user's OS-level dark/light mode preference by default.
20-35: Perfect system color-scheme handling with proper cleanup.The implementation correctly:
- Creates a MediaQueryList to detect system color scheme preference
- Only updates theme when in 'system' mode to avoid overriding explicit user choices
- Sets initial theme based on current media query state
- Adds event listener for dynamic color scheme changes
- Properly cleans up the event listener to prevent memory leaks
The logic handles both initial state and ongoing changes while respecting controlled behavior.
src/components/ui/HoverCard/stories/HoverCard.stories.tsx (1)
67-69: Click-to-open UX looks goodSwitching the trigger to an explicit click aligns with the “Controlled” story intent and helps disambiguate hover vs. click behaviors.
src/components/ui/HoverCard/fragments/HoverCardRoot.tsx (1)
41-41: Good adoption of useControllableStateRouting all open state changes through
useControllableStatesimplifies the controlled/uncontrolled logic and centralizes theonOpenChangecallback.
| const openTimeoutRef = useRef<number | null>(null); | ||
| const closeTimeoutRef = useRef<number | null>(null); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Timer ref typing/casts: use ReturnType and drop double casts
Avoid as unknown as number by typing refs to ReturnType<typeof setTimeout> | null. This is portable across DOM/Node typings and removes unsafe casts.
- const openTimeoutRef = useRef<number | null>(null);
- const closeTimeoutRef = useRef<number | null>(null);
+ const openTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);
+ const closeTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);
@@
- openTimeoutRef.current = setTimeout(() => {
+ openTimeoutRef.current = setTimeout(() => {
handleOpenChange(true);
- }, openDelay) as unknown as number;
+ }, openDelay);
@@
- closeTimeoutRef.current = setTimeout(() => {
+ closeTimeoutRef.current = setTimeout(() => {
setMouseIsExiting(prevState => {
if (prevState) {
handleOpenChange(false);
}
return prevState;
});
- }, closeDelay) as unknown as number;
+ }, closeDelay);
@@
useEffect(() => {
return () => {
if (openTimeoutRef.current) {
clearTimeout(openTimeoutRef.current);
}
if (closeTimeoutRef.current) {
clearTimeout(closeTimeoutRef.current);
}
};
}, []);Also applies to: 79-82, 86-97, 103-113
🤖 Prompt for AI Agents
In src/components/ui/HoverCard/fragments/HoverCardRoot.tsx around lines 46-47
(and also update usages at 79-82, 86-97, 103-113), the timer refs are currently
typed/used with unsafe number casts; change their types to use
useRef<ReturnType<typeof setTimeout> | null> (initialize to null) for
openTimeoutRef and closeTimeoutRef and any other timeout refs, remove the "as
unknown as number" casts, and adjust setTimeout/clearTimeout calls to accept
these typed refs (check for null before clearTimeout and assign setTimeout
result directly to the ref). Ensure all references and null checks are updated
accordingly.
| onOpenChange={() => {}} | ||
| openDelay={100} | ||
| closeDelay={200} | ||
| customRootClass="" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Controlled pattern: propagate onOpenChange to keep external state in sync
Using open={open} with onOpenChange={() => {}} prevents internal interactions (hover, dismiss) from updating your controlled state, which can lead to confusing demos. For a controlled example, pass setOpen to onOpenChange so both internal requests and your buttons remain in sync.
- <HoverCard.Root
- open={open}
- onOpenChange={() => {}}
+ <HoverCard.Root
+ open={open}
+ onOpenChange={setOpen}
openDelay={100}
closeDelay={200}
customRootClass=""
>📝 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.
| onOpenChange={() => {}} | |
| openDelay={100} | |
| closeDelay={200} | |
| customRootClass="" | |
| <HoverCard.Root | |
| open={open} | |
| onOpenChange={setOpen} | |
| openDelay={100} | |
| closeDelay={200} | |
| customRootClass="" | |
| > |
🤖 Prompt for AI Agents
In src/components/ui/HoverCard/stories/HoverCard.stories.tsx around lines 61 to
64, the story uses a controlled open={open} but supplies a no-op onOpenChange
handler, which prevents internal interactions from updating the external state;
replace the empty handler with the state setter (e.g., onOpenChange={setOpen})
so internal hover/dismiss events propagate to the story's open state and keep
the UI and buttons in sync.
| const intervalRef = useRef<NodeJS.Timeout | null>(null); | ||
| // read/write ref to know if continuous scrolling is currently active | ||
| const isScrollingRef = useRef(false); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Timer type mismatch in the browser
This file runs on the client; using NodeJS.Timeout can be incorrect and cause TS friction in DOM builds. Prefer number | null or ReturnType<typeof setInterval>.
- const intervalRef = useRef<NodeJS.Timeout | null>(null);
+ const intervalRef = useRef<ReturnType<typeof setInterval> | null>(null);📝 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 intervalRef = useRef<NodeJS.Timeout | null>(null); | |
| // read/write ref to know if continuous scrolling is currently active | |
| const isScrollingRef = useRef(false); | |
| const intervalRef = useRef<ReturnType<typeof setInterval> | null>(null); | |
| // read/write ref to know if continuous scrolling is currently active | |
| const isScrollingRef = useRef(false); |
🤖 Prompt for AI Agents
In src/components/ui/ScrollArea/fragments/ScrollAreaScrollbar.tsx around lines
14 to 16, the intervalRef is typed as NodeJS.Timeout which is wrong for browser
builds; change its type to number | null (or ReturnType<typeof setInterval>) and
update places that call setInterval/clearInterval to treat the ref value as a
number (use window.setInterval if needed) and pass it to clearInterval without
Node-specific typing so the code compiles cleanly in DOM builds.
| // Attach global mouse listeners only while actively scrolling | ||
| React.useEffect(() => { | ||
| if (!isScrollingRef.current) { | ||
| return () => { | ||
| removeListenersRef.current?.(); | ||
| stopContinuousScroll(); | ||
| }; | ||
| } | ||
|
|
||
| const handleMouseUp = () => stopContinuousScroll(); | ||
| const handleMouseLeave = () => stopContinuousScroll(); | ||
|
|
||
| document.addEventListener('mouseup', handleMouseUp); | ||
| document.addEventListener('mouseleave', handleMouseLeave); | ||
|
|
||
| return () => { | ||
| removeListenersRef.current = () => { | ||
| document.removeEventListener('mouseup', handleMouseUp); | ||
| document.removeEventListener('mouseleave', handleMouseLeave); | ||
| }; | ||
|
|
||
| return () => { | ||
| removeListenersRef.current?.(); | ||
| removeListenersRef.current = null; | ||
| stopContinuousScroll(); // Cleanup on unmount | ||
| }; | ||
| }, [stopContinuousScroll]); | ||
| }, [isScrolling, stopContinuousScroll]); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Effect cleanup calls setState via stopContinuousScroll when not scrolling; risks state thrash
The effect returns a cleanup that invokes stopContinuousScroll() even when no listeners were attached. This can trigger state flips during re-renders and complicate the lifecycle.
Refactor to a standard “attach when active” pattern: only attach when isScrolling is true, and cleanup just removes those listeners without calling stopContinuousScroll.
- React.useEffect(() => {
- if (!isScrollingRef.current) {
- return () => {
- removeListenersRef.current?.();
- stopContinuousScroll();
- };
- }
+ React.useEffect(() => {
+ if (!isScrolling) 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(); // Cleanup on unmount
- };
- }, [isScrolling, stopContinuousScroll]);
+ return () => {
+ removeListenersRef.current?.();
+ removeListenersRef.current = null;
+ };
+ }, [isScrolling, stopContinuousScroll]);📝 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.
| // Attach global mouse listeners only while actively scrolling | |
| React.useEffect(() => { | |
| if (!isScrollingRef.current) { | |
| return () => { | |
| removeListenersRef.current?.(); | |
| stopContinuousScroll(); | |
| }; | |
| } | |
| const handleMouseUp = () => stopContinuousScroll(); | |
| const handleMouseLeave = () => stopContinuousScroll(); | |
| document.addEventListener('mouseup', handleMouseUp); | |
| document.addEventListener('mouseleave', handleMouseLeave); | |
| return () => { | |
| removeListenersRef.current = () => { | |
| document.removeEventListener('mouseup', handleMouseUp); | |
| document.removeEventListener('mouseleave', handleMouseLeave); | |
| }; | |
| return () => { | |
| removeListenersRef.current?.(); | |
| removeListenersRef.current = null; | |
| stopContinuousScroll(); // Cleanup on unmount | |
| }; | |
| }, [stopContinuousScroll]); | |
| }, [isScrolling, stopContinuousScroll]); | |
| // Attach global mouse listeners only while actively scrolling | |
| React.useEffect(() => { | |
| if (!isScrolling) 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; | |
| }; | |
| }, [isScrolling, stopContinuousScroll]); |
🤖 Prompt for AI Agents
In src/components/ui/ScrollArea/fragments/ScrollAreaScrollbar.tsx around lines
80 to 106, the effect currently calls stopContinuousScroll() in its cleanup even
when no listeners were attached, which can cause unnecessary state updates;
change the effect to only run and attach document listeners when isScrolling (or
isScrollingRef.current) is true, register mouseup/mouseleave handlers, store a
removal function, and in the cleanup simply remove those listeners and clear the
removal ref without calling stopContinuousScroll(); ensure stopContinuousScroll
is invoked only from the actual handlers (or where scrolling truly ends), and
adjust the effect dependencies accordingly.
Summary by CodeRabbit
New Features
Behavior Changes
Documentation
Tests