Merged
Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR converts many components across the raystack package from React.forwardRef wrappers to plain function components, removes ref forwarding in many places, adjusts TypeScript prop types (often switching to native ComponentProps or including/excluding Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
rohanchkrabrty
changed the title
rohanchkrabrty
changed the title
rohanchkrabrty
changed the title
paanSinghCoder
approved these changes
Mar 23, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/raystack/components/sidebar/sidebar-misc.tsx (1)
45-68:⚠️ Potential issue | 🟡 MinorUse
sectionprops forSidebarNavigationGroup.Line 45 exposes
divprops/ref viaComponentProps<'div'>, but the component renders a<section>element. This creates a typing mismatch: consumers will receive anHTMLElement(from the section), but the type contract promisesHTMLDivElement. UseComponentProps<'section'>to match the rendered element.-export interface SidebarNavigationGroupProps extends ComponentProps<'div'> { +export interface SidebarNavigationGroupProps extends ComponentProps<'section'> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/sidebar/sidebar-misc.tsx` around lines 45 - 68, SidebarNavigationGroupProps currently extends ComponentProps<'div'> while SidebarNavigationGroup renders a <section>, causing an incorrect prop/ref type; update the props type to extend ComponentProps<'section'> (change ComponentProps<'div'> to ComponentProps<'section'>) so the SidebarNavigationGroupProps and SidebarNavigationGroup signatures correctly reflect the rendered element and its attributes.
🧹 Nitpick comments (2)
packages/raystack/components/progress/progress-misc.tsx (1)
7-17: Ref support removed from ProgressLabel and ProgressValue.These components no longer support ref forwarding. For label and value display components, direct DOM access is typically less critical. If consumers need ref access, consider adding
refas an optional prop similar to other components in this PR.Also applies to: 21-31
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/progress/progress-misc.tsx` around lines 7 - 17, ProgressLabel (and ProgressValue) currently lack ref forwarding; reintroduce optional ref support by converting the component to accept a ref prop and forward it to ProgressPrimitive.Label (and ProgressPrimitive.Value) using React.forwardRef or the same pattern used by other components in this PR; ensure the prop typing includes a ref (matching ProgressPrimitive.Label.Props / ProgressPrimitive.Value.Props) and pass that ref through to the underlying primitive component while preserving className and other props (reference symbols: ProgressLabel, ProgressValue, ProgressPrimitive.Label, ProgressPrimitive.Value).packages/raystack/components/radio/radio.tsx (1)
7-11: Ref support removed from RadioGroup and RadioItem.Unlike other components in this PR that convert
refto a regular prop, these components have completely removed ref support. If consumers need to access the underlying DOM elements, this is a breaking change.Consider adding
refas an optional prop if ref forwarding is needed:♻️ Optional: Add ref support if needed
-function RadioGroup({ className, ...props }: RadioGroupPrimitive.Props) { +function RadioGroup({ ref, className, ...props }: RadioGroupPrimitive.Props) { return ( - <RadioGroupPrimitive className={cx(styles.radio, className)} {...props} /> + <RadioGroupPrimitive ref={ref} className={cx(styles.radio, className)} {...props} /> ); }-function RadioItem({ className, ...props }: RadioPrimitive.Root.Props) { +function RadioItem({ ref, className, ...props }: RadioPrimitive.Root.Props) { return ( - <RadioPrimitive.Root {...props} className={cx(styles.radioitem, className)}> + <RadioPrimitive.Root ref={ref} {...props} className={cx(styles.radioitem, className)}> <RadioPrimitive.Indicator className={styles.indicator} /> </RadioPrimitive.Root> ); }Also applies to: 15-21
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/radio/radio.tsx` around lines 7 - 11, RadioGroup (and RadioItem) currently drop ref support, which is a breaking change; change the component to accept and forward refs by converting the function to use React.forwardRef and typing it appropriately (e.g., const RadioGroup = React.forwardRef<HTMLDivElement, RadioGroupPrimitive.Props>(( { className, ...props }, ref) => { return <RadioGroupPrimitive ref={ref} className={cx(styles.radio, className)} {...props} />; })); do the same pattern for RadioItem (forward the ref to the underlying primitive), ensure types use ComponentPropsWithRef/appropriate HTML element type and export the forwarded-ref component.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/raystack/components/progress/progress-root.tsx`:
- Around line 47-59: The computed percentage (const percentage) can become
Infinity/NaN when max <= min or fall outside 0-100 if value is out of range;
clamp and validate it before using in ProgressContext and inline style: compute
a safePercentage by first handling null value, ensuring denominator > 0,
bounding raw ((value-min)/(max-min))*100 to [0,100], and fallback to 0 on
invalid math, then pass safePercentage into ProgressContext value and the inline
CSS variable '--rs-progress-percentage' used in ProgressPrimitive.Root
(referencing percentage, value, min, max, ProgressContext, and
ProgressPrimitive.Root).
In `@packages/raystack/components/search/search.tsx`:
- Around line 16-29: The Search component is destructuring a ref but not wrapped
with React.forwardRef, and InputField likewise doesn't forward refs to its
underlying input; wrap both Search and InputField with React.forwardRef so they
receive a ref parameter (instead of destructuring ref from props) and pass that
ref down: in Search accept (props, ref) and forward the ref to the child
InputField (e.g., <InputField ref={ref} ...>), and in InputField accept (props,
ref) and attach the ref to the actual <input> element (e.g., <input ref={ref}
...>), updating their prop typings accordingly.
In `@packages/raystack/components/select/select-content.tsx`:
- Around line 12-28: The SelectContent component now accepts ref as a regular
prop (SelectContent) which only works in React 19; either update the package
peerDependencies to require React 19 (set peerDependencies "react" to "^19") in
packages/raystack/package.json, or restore the previous forwardRef wrapping for
SelectContent and the other functions where forwardRef was removed (the
components referenced at the other changed blocks) so refs continue to work with
React 18 consumers—pick one approach and apply it consistently across all
components that had forwardRef removed.
In `@packages/raystack/components/select/select-value.tsx`:
- Around line 71-73: When item is an array the ref isn't forwarded to the
rendered node, causing inconsistent behavior; wrap the multiple-selection render
in a simple element that accepts the ref and forward the ref to that wrapper.
Update the branch in select-value.tsx that returns <SelectMultipleValue
data={item} /> to instead render a wrapper (e.g., <span>) with the incoming ref
attached and render <SelectMultipleValue data={item} /> inside it so the ref is
consistently available for both single and multiple selection paths; reference
the existing item array check and the SelectMultipleValue component when making
the change.
---
Outside diff comments:
In `@packages/raystack/components/sidebar/sidebar-misc.tsx`:
- Around line 45-68: SidebarNavigationGroupProps currently extends
ComponentProps<'div'> while SidebarNavigationGroup renders a <section>, causing
an incorrect prop/ref type; update the props type to extend
ComponentProps<'section'> (change ComponentProps<'div'> to
ComponentProps<'section'>) so the SidebarNavigationGroupProps and
SidebarNavigationGroup signatures correctly reflect the rendered element and its
attributes.
---
Nitpick comments:
In `@packages/raystack/components/progress/progress-misc.tsx`:
- Around line 7-17: ProgressLabel (and ProgressValue) currently lack ref
forwarding; reintroduce optional ref support by converting the component to
accept a ref prop and forward it to ProgressPrimitive.Label (and
ProgressPrimitive.Value) using React.forwardRef or the same pattern used by
other components in this PR; ensure the prop typing includes a ref (matching
ProgressPrimitive.Label.Props / ProgressPrimitive.Value.Props) and pass that ref
through to the underlying primitive component while preserving className and
other props (reference symbols: ProgressLabel, ProgressValue,
ProgressPrimitive.Label, ProgressPrimitive.Value).
In `@packages/raystack/components/radio/radio.tsx`:
- Around line 7-11: RadioGroup (and RadioItem) currently drop ref support, which
is a breaking change; change the component to accept and forward refs by
converting the function to use React.forwardRef and typing it appropriately
(e.g., const RadioGroup = React.forwardRef<HTMLDivElement,
RadioGroupPrimitive.Props>(( { className, ...props }, ref) => { return
<RadioGroupPrimitive ref={ref} className={cx(styles.radio, className)}
{...props} />; })); do the same pattern for RadioItem (forward the ref to the
underlying primitive), ensure types use ComponentPropsWithRef/appropriate HTML
element type and export the forwarded-ref component.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0b48c3f7-cafe-4b8b-b969-834dee8c1f33
📒 Files selected for processing (20)
packages/raystack/components/popover/popover.tsxpackages/raystack/components/preview-card/preview-card.tsxpackages/raystack/components/progress/progress-misc.tsxpackages/raystack/components/progress/progress-root.tsxpackages/raystack/components/progress/progress-track.tsxpackages/raystack/components/radio/radio.tsxpackages/raystack/components/scroll-area/scroll-area-scrollbar.tsxpackages/raystack/components/scroll-area/scroll-area.tsxpackages/raystack/components/search/search.tsxpackages/raystack/components/select/select-content.tsxpackages/raystack/components/select/select-item.tsxpackages/raystack/components/select/select-misc.tsxpackages/raystack/components/select/select-root.tsxpackages/raystack/components/select/select-trigger.tsxpackages/raystack/components/select/select-value.tsxpackages/raystack/components/side-panel/side-panel.tsxpackages/raystack/components/sidebar/sidebar-item.tsxpackages/raystack/components/sidebar/sidebar-main.tsxpackages/raystack/components/sidebar/sidebar-misc.tsxpackages/raystack/components/sidebar/sidebar-root.tsx
Comment on lines
+47
to
+59
| const percentage = value === null ? 0 : ((value - min) * 100) / (max - min); | ||
|
|
||
| return ( | ||
| <ProgressContext.Provider | ||
| value={{ variant: variant ?? 'linear', value, percentage }} | ||
| return ( | ||
| <ProgressContext | ||
| value={{ variant: variant ?? 'linear', value, percentage }} | ||
| > | ||
| <ProgressPrimitive.Root | ||
| className={progress({ variant, className })} | ||
| style={ | ||
| { | ||
| ...style, | ||
| '--rs-progress-percentage': percentage | ||
| } as CSSProperties |
Contributor
There was a problem hiding this comment.
Guard the derived percentage against invalid ranges.
Line 47 can produce Infinity/NaN when max <= min, and out-of-range values can push --rs-progress-percentage past the expected 0-100 band. That leaks into both the context value and the inline style.
🩹 Proposed fix
- const percentage = value === null ? 0 : ((value - min) * 100) / (max - min);
+ const range = max - min;
+ const percentage =
+ value == null || range <= 0
+ ? 0
+ : Math.min(100, Math.max(0, ((value - min) * 100) / range));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/progress/progress-root.tsx` around lines 47 -
59, The computed percentage (const percentage) can become Infinity/NaN when max
<= min or fall outside 0-100 if value is out of range; clamp and validate it
before using in ProgressContext and inline style: compute a safePercentage by
first handling null value, ensuring denominator > 0, bounding raw
((value-min)/(max-min))*100 to [0,100], and fallback to 0 on invalid math, then
pass safePercentage into ProgressContext value and the inline CSS variable
'--rs-progress-percentage' used in ProgressPrimitive.Root (referencing
percentage, value, min, max, ProgressContext, and ProgressPrimitive.Root).
Comment on lines
+16
to
+29
| export function Search({ | ||
| ref, | ||
| className, | ||
| disabled, | ||
| placeholder = 'Search', | ||
| size, | ||
| showClearButton, | ||
| onClear, | ||
| value, | ||
| onChange, | ||
| width = '100%', | ||
| variant = 'default', | ||
| ...props | ||
| }: SearchProps) { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify InputField's ref handling
cat packages/raystack/components/input-field/input-field.tsx | head -100Repository: raystack/apsara
Length of output: 2633
🏁 Script executed:
# Read search.tsx to see the full context around lines 16-29 and 52-65
wc -l packages/raystack/components/search/search.tsxRepository: raystack/apsara
Length of output: 108
🏁 Script executed:
# Read the search.tsx file with context around the mentioned lines
sed -n '1,80p' packages/raystack/components/search/search.tsxRepository: raystack/apsara
Length of output: 1769
🏁 Script executed:
# Check the complete InputField to see if it's wrapped in forwardRef
cat packages/raystack/components/input-field/input-field.tsxRepository: raystack/apsara
Length of output: 4556
Search component does not properly forward ref to InputField due to missing forwardRef wrapper.
The Search component destructures ref but is not wrapped in forwardRef, so React filters out the ref before it reaches the function. Additionally, InputField is a plain function component and does not use forwardRef, so even if the ref were passed, it would be silently ignored at runtime.
To fix this, both components need to be wrapped with React.forwardRef(), and InputField should forward the ref to the underlying <input> element.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/search/search.tsx` around lines 16 - 29, The
Search component is destructuring a ref but not wrapped with React.forwardRef,
and InputField likewise doesn't forward refs to its underlying input; wrap both
Search and InputField with React.forwardRef so they receive a ref parameter
(instead of destructuring ref from props) and pass that ref down: in Search
accept (props, ref) and forward the ref to the child InputField (e.g.,
<InputField ref={ref} ...>), and in InputField accept (props, ref) and attach
the ref to the actual <input> element (e.g., <input ref={ref} ...>), updating
their prop typings accordingly.
Comment on lines
+71
to
+73
| if (Array.isArray(item)) { | ||
| return <SelectMultipleValue data={item} />; | ||
| } |
Contributor
There was a problem hiding this comment.
Ref is not forwarded when rendering SelectMultipleValue.
When item is an array (multiple selection with more than one item), the ref is not passed to SelectMultipleValue. This creates inconsistent behavior where the ref works for single selections but is lost for multiple selections.
Consider wrapping in a span to maintain ref consistency:
🔧 Proposed fix
if (Array.isArray(item)) {
- return <SelectMultipleValue data={item} />;
+ return (
+ <span ref={ref} className={className} {...props}>
+ <SelectMultipleValue data={item} />
+ </span>
+ );
}📝 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.
Suggested change
| if (Array.isArray(item)) { | |
| return <SelectMultipleValue data={item} />; | |
| } | |
| if (Array.isArray(item)) { | |
| return ( | |
| <span ref={ref} className={className} {...props}> | |
| <SelectMultipleValue data={item} /> | |
| </span> | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/select/select-value.tsx` around lines 71 - 73,
When item is an array the ref isn't forwarded to the rendered node, causing
inconsistent behavior; wrap the multiple-selection render in a simple element
that accepts the ref and forward the ref to that wrapper. Update the branch in
select-value.tsx that returns <SelectMultipleValue data={item} /> to instead
render a wrapper (e.g., <span>) with the incoming ref attached and render
<SelectMultipleValue data={item} /> inside it so the ref is consistently
available for both single and multiple selection paths; reference the existing
item array check and the SelectMultipleValue component when making the change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
forwardRefwrappers (Cases A & B) across popover, preview-card, progress, radio, scroll-area, search, select, and sidebar components<Context.Provider value={…}>to<Context value={…}>shorthand in progress, select, and sidebarComponentPropsWithoutRefwithComponentPropsand updated named imports across select and sidebarComponents migrated:
Summary by CodeRabbit