Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThis PR removes React.forwardRef/ElementRef usage across many raystack components, converting them to plain function components, updates props to use ComponentProps for DOM elements, adjusts a few internal render flows, and adds/normalizes displayName assignments. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
📝 Coding Plan
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 |
![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: 10
🧹 Nitpick comments (2)
packages/raystack/components/amount/amount.tsx (1)
175-180: Edge case: short string values produce incorrect minor unit conversion.When
valueis a string with fewer digits thandecimals, the slice produces incorrect results. For example, with USD (2 decimals):
value = "9"(9 cents) →"".slice(0,-2) + "." + "9".slice(-2)→".9"(interpreted as $0.90, not $0.09)🔧 Proposed fix to pad short strings
const baseValue = valueInMinorUnits && decimals > 0 ? typeof value === 'string' - ? value.slice(0, -decimals) + '.' + value.slice(-decimals) + ? (() => { + const padded = value.padStart(decimals + 1, '0'); + return padded.slice(0, -decimals) + '.' + padded.slice(-decimals); + })() : value / Math.pow(10, decimals) : value;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/amount/amount.tsx` around lines 175 - 180, The current conversion for baseValue mishandles string minor-unit inputs shorter than decimals (e.g., value="9", decimals=2); update the branch that handles string values (when valueInMinorUnits && decimals > 0 && typeof value === 'string') to left-pad the string with zeros (use padStart) so its length is at least decimals+1 before slicing, then perform the slice for integer and fractional parts; keep the non-string numeric branch (value / Math.pow(10, decimals)) unchanged and ensure references to baseValue, valueInMinorUnits, decimals, and value are used to locate the change.packages/raystack/components/badge/badge.tsx (1)
2-2: Use type-only imports for React types for consistency.On Line 2,
ComponentPropsandReactNodeare type-only. The file already uses type-only imports elsewhere (Line 1:import { cva, type VariantProps }), so applying the same pattern here improves consistency and aligns with TypeScript best practices.Proposed fix
-import { ComponentProps, ReactNode } from 'react'; +import { type ComponentProps, type ReactNode } from 'react';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/badge/badge.tsx` at line 2, Replace the value import of React types with type-only imports: change the import that brings in ComponentProps and ReactNode so they are imported using the TypeScript `type` qualifier (e.g., `import type { ComponentProps, ReactNode } from 'react'`) to match the existing `type VariantProps` pattern; update any references in the Badge component and its props (e.g., where ComponentProps or ReactNode are used) to rely on these type-only imports.
🤖 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/accordion/accordion-root.tsx`:
- Around line 38-48: The computations for baseValue and baseDefaultValue
incorrectly use truthy checks which convert falsy-but-valid values (e.g. empty
string) to undefined; update the conditional expressions in the baseValue and
baseDefaultValue assignments to use explicit !== undefined checks (use value !==
undefined and defaultValue !== undefined) so that Array.isArray wrapping still
occurs for non-array values while preserving empty strings and other
falsy-but-valid values.
In `@packages/raystack/components/alert-dialog/alert-dialog-content.tsx`:
- Line 47: In AlertDialogContent where the inline style is built using { width,
...style }, change the merge order so the explicit width prop wins over
style.width by spreading style first and then applying width (i.e., use ...style
then width); update the JSX where style={{ width, ...style }} or style={ {
width, ...style } } is used so that style becomes style={{ ...style, width }}
and ensure this logic is applied inside the AlertDialogContent component
rendering path.
- Around line 32-38: The Backdrop is receiving wrapper-only flags because the
component spreads the entire overlay object into AlertDialogPrimitive.Backdrop;
update the render to destructure overlay (const { blur, className,
...backdropProps } = overlay) and pass only backdropProps to
AlertDialogPrimitive.Backdrop while using blur and className for composing
className (e.g., styles.dialogOverlay, overlayBlur, overlay.className) so blur
is not forwarded as a DOM/backdrop prop.
In `@packages/raystack/components/avatar/avatar.tsx`:
- Around line 198-203: The current logic treats max=0 as falsy and can
dereference avatars[0] when children is empty; update the computation to
explicitly check for max being a number (e.g., max !== undefined) and convert
children to an array (React.Children.toArray(children)) before slicing so
max={0} behaves correctly and slicing is safe; compute count using the same
numeric check (count = typeof max === 'number' && childrenArray.length > max ?
childrenArray.length - max : 0) and guard getAvatarProps by only calling
getAvatarProps(avatars[0]) when avatars[0] exists (or pass a safe default) to
avoid dereferencing undefined when children is empty.
- Around line 157-181: AvatarRoot currently drops React.forwardRef (losing ref
forwarding in React 18) because it relies on ref-in-props; reintroduce ref
forwarding by converting AvatarRoot into a React.forwardRef component (use
React.forwardRef<HTMLDivElement, AvatarProps>(function AvatarRoot(props, ref) {
... })) and pass the ref explicitly to AvatarPrimitive.Root (e.g.,
<AvatarPrimitive.Root ref={ref} ... />), ensuring other props still spread via
{...props}; alternatively, if you intend to drop React 18 support, update
peerDependencies to remove ^18 and bump the major version—prefer restoring
forwardRef to retain React 18 compatibility.
In `@packages/raystack/components/badge/badge.tsx`:
- Around line 6-25: The cva configuration for the badge constant places
defaultVariants inside the variants object, so defaults won't be applied; update
the cva call that defines badge so that defaultVariants is moved out of the
variants block to be a top-level sibling of variants (keep variant and size
under variants and ensure defaultVariants has variant: 'accent' and size:
'small'), preserving existing variant names (accent, warning, danger, success,
neutral, gradient) and sizes (micro, small, regular) and the styles references
used in badge.
In `@packages/raystack/components/box/box.tsx`:
- Around line 3-4: Box currently renders <div {...props}/> which drops the
component's default class; update the Box component (function Box) to merge the
default class (e.g., styles.box or the string "box") with props.className using
a class-merge utility (classnames/clsx) or manual concatenation, then spread the
remaining props so the returned <div> includes className={mergedClassName} and
{...rest}; ensure props typing (ComponentProps<'div'>) is preserved and that
custom className values are appended so callers can override styles.
In `@packages/raystack/components/breadcrumb/breadcrumb-item.tsx`:
- Around line 27-67: The component currently drops the ref when dropdownItems is
present; update BreadcrumbItem to accept and forward refs properly by converting
it to a React.forwardRef component (use forwardRef and a ref parameter) and pass
that forwarded ref into the Menu.Trigger element (and into the non-dropdown
rendered element path as before). Ensure the prop signature no longer
destructures a raw "ref" prop and instead uses the forwarded ref variable, and
forward the ref to Menu.Trigger (and to the anchor/renderedElement fallback) so
consumers receive their ref in both code paths.
In `@packages/raystack/components/calendar/date-picker.tsx`:
- Around line 202-204: The fallback trigger button inside Popover.Trigger
currently defaults to type="submit" and may submit surrounding forms; update the
fallback to explicitly set type="button" (i.e., when rendering fallback
<button>{trigger}</button>) so the Popover.Trigger render branch uses a
non-submitting button; locate the Popover.Trigger usage and change the fallback
button declaration accordingly (referencing Popover.Trigger and the trigger
prop/variable).
In `@packages/raystack/components/calendar/range-picker.tsx`:
- Around line 170-172: The fallback trigger renders an implicit submit button
which can cause unwanted form submissions; update the Popover.Trigger usage so
that when the fallback is used (i.e., render={isValidElement(trigger) ? trigger
: <button>{trigger}</button>}) the button includes an explicit type="button"
attribute; locate the Popover.Trigger line in range-picker.tsx and change the
fallback <button> to include type="button" to prevent form submissions when used
inside forms.
---
Nitpick comments:
In `@packages/raystack/components/amount/amount.tsx`:
- Around line 175-180: The current conversion for baseValue mishandles string
minor-unit inputs shorter than decimals (e.g., value="9", decimals=2); update
the branch that handles string values (when valueInMinorUnits && decimals > 0 &&
typeof value === 'string') to left-pad the string with zeros (use padStart) so
its length is at least decimals+1 before slicing, then perform the slice for
integer and fractional parts; keep the non-string numeric branch (value /
Math.pow(10, decimals)) unchanged and ensure references to baseValue,
valueInMinorUnits, decimals, and value are used to locate the change.
In `@packages/raystack/components/badge/badge.tsx`:
- Line 2: Replace the value import of React types with type-only imports: change
the import that brings in ComponentProps and ReactNode so they are imported
using the TypeScript `type` qualifier (e.g., `import type { ComponentProps,
ReactNode } from 'react'`) to match the existing `type VariantProps` pattern;
update any references in the Badge component and its props (e.g., where
ComponentProps or ReactNode are used) to rely on these type-only imports.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 13501e9a-7a01-4a97-a3aa-9c155b500f1a
📒 Files selected for processing (19)
packages/raystack/components/accordion/accordion-content.tsxpackages/raystack/components/accordion/accordion-item.tsxpackages/raystack/components/accordion/accordion-root.tsxpackages/raystack/components/accordion/accordion-trigger.tsxpackages/raystack/components/alert-dialog/alert-dialog-content.tsxpackages/raystack/components/alert-dialog/alert-dialog-misc.tsxpackages/raystack/components/amount/amount.tsxpackages/raystack/components/announcement-bar/announcement-bar.tsxpackages/raystack/components/avatar/avatar.tsxpackages/raystack/components/badge/badge.tsxpackages/raystack/components/box/box.module.csspackages/raystack/components/box/box.tsxpackages/raystack/components/breadcrumb/breadcrumb-item.tsxpackages/raystack/components/breadcrumb/breadcrumb-misc.tsxpackages/raystack/components/breadcrumb/breadcrumb-root.tsxpackages/raystack/components/button/button.tsxpackages/raystack/components/calendar/calendar.tsxpackages/raystack/components/calendar/date-picker.tsxpackages/raystack/components/calendar/range-picker.tsx
💤 Files with no reviewable changes (1)
- packages/raystack/components/box/box.module.css
Comment on lines
+38
to
+48
| const baseValue = value | ||
| ? Array.isArray(value) | ||
| ? value | ||
| : [value] | ||
| : undefined; | ||
|
|
||
| const baseDefaultValue = defaultValue | ||
| ? Array.isArray(defaultValue) | ||
| ? defaultValue | ||
| : [defaultValue] | ||
| : undefined; | ||
| const baseDefaultValue = defaultValue | ||
| ? Array.isArray(defaultValue) | ||
| ? defaultValue | ||
| : [defaultValue] | ||
| : undefined; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For @base-ui/react Accordion, is an empty string ("") a valid item value? Please link official docs/API reference.
💡 Result:
Yes. In @base-ui/react Accordion, Accordion.Item’s value prop is typed as any and is described as “a unique value that identifies this accordion item.” An empty string ("") is still a (string) value, so it’s valid as long as it’s unique among items. If you omit value entirely, Base UI will generate an ID instead. [1]
Official docs / API reference:
- Base UI Accordion docs (includes
Accordion.Item valueprop table): https://base-ui.com/react/components/accordion [1]
Related note (initially-open behavior requires an explicit value):
- Release notes v1.0.0-beta.4 (Accordion change): https://base-ui.com/react/overview/releases/v1-0-0-beta-4 [2]
Sources: [1] https://base-ui.com/react/components/accordion, [2] https://base-ui.com/react/overview/releases/v1-0-0-beta-4
🏁 Script executed:
find . -name "accordion-root.tsx" -type f | head -5Repository: raystack/apsara
Length of output: 118
🏁 Script executed:
cat -n ./packages/raystack/components/accordion/accordion-root.tsx | sed -n '30,60p'Repository: raystack/apsara
Length of output: 1032
🏁 Script executed:
cat -n ./packages/raystack/components/accordion/accordion-root.tsx | sed -n '1,20p'Repository: raystack/apsara
Length of output: 747
🏁 Script executed:
cat -n ./packages/raystack/components/accordion/accordion-root.tsx | sed -n '20,36p'Repository: raystack/apsara
Length of output: 524
Use explicit !== undefined checks to preserve falsy values like empty strings.
The truthy checks on lines 38 and 44 will incorrectly convert empty strings ("") to undefined. Since @base-ui/react Accordion accepts empty strings as valid item values, this causes silent data loss. Replace value ? with value !== undefined and defaultValue ? with defaultValue !== undefined.
Proposed patch
- const baseValue = value
+ const baseValue = value !== undefined
? Array.isArray(value)
? value
: [value]
: undefined;
- const baseDefaultValue = defaultValue
+ const baseDefaultValue = defaultValue !== undefined
? Array.isArray(defaultValue)
? defaultValue
: [defaultValue]
: undefined;📝 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
| const baseValue = value | |
| ? Array.isArray(value) | |
| ? value | |
| : [value] | |
| : undefined; | |
| const baseDefaultValue = defaultValue | |
| ? Array.isArray(defaultValue) | |
| ? defaultValue | |
| : [defaultValue] | |
| : undefined; | |
| const baseDefaultValue = defaultValue | |
| ? Array.isArray(defaultValue) | |
| ? defaultValue | |
| : [defaultValue] | |
| : undefined; | |
| const baseValue = value !== undefined | |
| ? Array.isArray(value) | |
| ? value | |
| : [value] | |
| : undefined; | |
| const baseDefaultValue = defaultValue !== undefined | |
| ? Array.isArray(defaultValue) | |
| ? defaultValue | |
| : [defaultValue] | |
| : undefined; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/accordion/accordion-root.tsx` around lines 38 -
48, The computations for baseValue and baseDefaultValue incorrectly use truthy
checks which convert falsy-but-valid values (e.g. empty string) to undefined;
update the conditional expressions in the baseValue and baseDefaultValue
assignments to use explicit !== undefined checks (use value !== undefined and
defaultValue !== undefined) so that Array.isArray wrapping still occurs for
non-array values while preserving empty strings and other falsy-but-valid
values.
Comment on lines
+32
to
+38
| <AlertDialogPrimitive.Backdrop | ||
| {...overlay} | ||
| className={cx( | ||
| styles.dialogOverlay, | ||
| overlay?.blur && styles.overlayBlur, | ||
| overlay?.className | ||
| )} |
Contributor
There was a problem hiding this comment.
Strip overlay.blur before forwarding backdrop props.
Line 33 spreads the full overlay object into AlertDialogPrimitive.Backdrop, so the wrapper-only blur flag is forwarded downstream as if it were a backdrop prop. Destructure blur (and className) first, then spread only the remaining backdrop props.
Proposed fix
export const AlertDialogContent = ({
className,
children,
showCloseButton = true,
overlay,
width,
style,
showNestedAnimation = true,
...props
}: AlertDialogContentProps) => {
+ const {
+ blur: overlayBlur,
+ className: overlayClassName,
+ ...overlayProps
+ } = overlay ?? {};
+
return (
<AlertDialogPrimitive.Portal>
<AlertDialogPrimitive.Backdrop
- {...overlay}
+ {...overlayProps}
className={cx(
styles.dialogOverlay,
- overlay?.blur && styles.overlayBlur,
- overlay?.className
+ overlayBlur && styles.overlayBlur,
+ overlayClassName
)}
/>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/alert-dialog/alert-dialog-content.tsx` around
lines 32 - 38, The Backdrop is receiving wrapper-only flags because the
component spreads the entire overlay object into AlertDialogPrimitive.Backdrop;
update the render to destructure overlay (const { blur, className,
...backdropProps } = overlay) and pass only backdropProps to
AlertDialogPrimitive.Backdrop while using blur and className for composing
className (e.g., styles.dialogOverlay, overlayBlur, overlay.className) so blur
is not forwarded as a DOM/backdrop prop.
| ); | ||
| } | ||
| ); | ||
| style={{ width, ...style }} |
Contributor
There was a problem hiding this comment.
Let the explicit width prop win over style.width.
Line 47 currently uses { width, ...style }, so style.width overrides the dedicated width prop whenever both are set. That makes the component-specific API easy to bypass accidentally.
Proposed fix
- style={{ width, ...style }}
+ style={width === undefined ? style : { ...style, width }}📝 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
| style={{ width, ...style }} | |
| style={width === undefined ? style : { ...style, width }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/alert-dialog/alert-dialog-content.tsx` at line
47, In AlertDialogContent where the inline style is built using { width,
...style }, change the merge order so the explicit width prop wins over
style.width by spreading style first and then applying width (i.e., use ...style
then width); update the JSX where style={{ width, ...style }} or style={ {
width, ...style } } is used so that style becomes style={{ ...style, width }}
and ensure this logic is applied inside the AlertDialogContent component
rendering path.
Comment on lines
+157
to
181
| const AvatarRoot = ({ | ||
| className, | ||
| alt, | ||
| src, | ||
| fallback, | ||
| size, | ||
| radius, | ||
| variant, | ||
| color, | ||
| ...props | ||
| }: AvatarProps) => ( | ||
| <AvatarPrimitive.Root | ||
| className={cx( | ||
| styles.imageWrapper, | ||
| avatar({ size, radius, variant, color }), | ||
| className | ||
| )} | ||
| {...props} | ||
| > | ||
| <AvatarPrimitive.Image className={image()} src={src} alt={alt} /> | ||
| <AvatarPrimitive.Fallback className={styles.fallback}> | ||
| {fallback} | ||
| </AvatarPrimitive.Fallback> | ||
| </AvatarPrimitive.Root> | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== React versions declared in package manifests =="
fd package.json -x sh -c '
file="$1"
react=$(jq -r ".dependencies.react // .peerDependencies.react // .devDependencies.react // empty" "$file")
react_dom=$(jq -r ".dependencies[\"react-dom\"] // .peerDependencies[\"react-dom\"] // .devDependencies[\"react-dom\"] // empty" "$file")
if [ -n "${react}${react_dom}" ]; then
printf "%s -> react=%s react-dom=%s\n" "$file" "${react:-<none>}" "${react_dom:-<none>}"
fi
' sh {}
echo
echo "== Existing Avatar ref expectations in tests =="
rg -n -C2 --type=ts --type=tsx 'forwards ref correctly|<Avatar ref=|<AvatarGroup ref='Repository: raystack/apsara
Length of output: 336
🏁 Script executed:
# Correct the tsx file type issue and search for Avatar ref patterns
rg -n -C2 'forwards ref correctly|ref=' --type ts -A 2 -B 2
# Also check the actual avatar.tsx file to confirm forwardRef was removed
fd avatar.tsx --type f -x head -n 200 {}Repository: raystack/apsara
Length of output: 50372
🏁 Script executed:
# Check the actual avatar.tsx file for forwardRef status
cat -n packages/raystack/components/avatar/avatar.tsx | head -220
# Search specifically for Avatar tests
fd avatar --type f | grep -i testRepository: raystack/apsara
Length of output: 7989
🏁 Script executed:
# Read the avatar test file to see if ref-forwarding is tested
head -100 packages/raystack/components/avatar/__tests__/avatar.test.tsx
# Check the package version to see if it was bumped
cat packages/raystack/package.json | jq '.version'Repository: raystack/apsara
Length of output: 3516
This removes forwardRef support but the package still claims React 18 compatibility—a breaking change.
Lines 157 and 192 remove forwardRef and rely on ref-as-prop forwarding via ...props. While the ref-forwarding test passes (indicating React 19 environment), the package's peerDependencies still declare react=^18 || ^19. In React 18, refs passed as props are silently ignored; this change breaks ref forwarding for any consumers on React 18.
Either bump to a major version and drop React 18 from peerDependencies, or keep forwardRef if React 18 support is required.
Also applies to: 192-224
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/avatar/avatar.tsx` around lines 157 - 181,
AvatarRoot currently drops React.forwardRef (losing ref forwarding in React 18)
because it relies on ref-in-props; reintroduce ref forwarding by converting
AvatarRoot into a React.forwardRef component (use
React.forwardRef<HTMLDivElement, AvatarProps>(function AvatarRoot(props, ref) {
... })) and pass the ref explicitly to AvatarPrimitive.Root (e.g.,
<AvatarPrimitive.Root ref={ref} ... />), ensuring other props still spread via
{...props}; alternatively, if you intend to drop React 18 support, update
peerDependencies to remove ^18 and bump the major version—prefer restoring
forwardRef to retain React 18 compatibility.
Comment on lines
+198
to
203
| const avatars = max ? children.slice(0, max) : children; | ||
| const count = max && children.length > max ? children.length - max : 0; | ||
|
|
||
| // Overflow avatar matches the first avatar styling | ||
| const firstAvatarProps = getAvatarProps(avatars[0]); | ||
| // Overflow avatar matches the first avatar styling | ||
| const firstAvatarProps = getAvatarProps(avatars[0]); | ||
|
|
Contributor
There was a problem hiding this comment.
Handle max={0} and empty children safely to avoid incorrect output/crash.
Line 198/Line 199 use truthy checks, so max={0} behaves like “no max”. Also Line 202 dereferences avatars[0] without guarding, which can throw when children is empty.
💡 Proposed fix
export const AvatarGroup = ({
children,
max,
className,
...props
}: AvatarGroupProps) => {
- const avatars = max ? children.slice(0, max) : children;
- const count = max && children.length > max ? children.length - max : 0;
+ const hasMax = max != null;
+ const normalizedMax = Math.max(0, max ?? 0);
+ const avatars = hasMax ? children.slice(0, normalizedMax) : children;
+ const count =
+ hasMax && children.length > normalizedMax ? children.length - normalizedMax : 0;
// Overflow avatar matches the first avatar styling
- const firstAvatarProps = getAvatarProps(avatars[0]);
+ const firstAvatarProps = avatars.length > 0 ? getAvatarProps(avatars[0]) : {};📝 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
| const avatars = max ? children.slice(0, max) : children; | |
| const count = max && children.length > max ? children.length - max : 0; | |
| // Overflow avatar matches the first avatar styling | |
| const firstAvatarProps = getAvatarProps(avatars[0]); | |
| // Overflow avatar matches the first avatar styling | |
| const firstAvatarProps = getAvatarProps(avatars[0]); | |
| const hasMax = max != null; | |
| const normalizedMax = Math.max(0, max ?? 0); | |
| const avatars = hasMax ? children.slice(0, normalizedMax) : children; | |
| const count = | |
| hasMax && children.length > normalizedMax ? children.length - normalizedMax : 0; | |
| // Overflow avatar matches the first avatar styling | |
| const firstAvatarProps = avatars.length > 0 ? getAvatarProps(avatars[0]) : {}; | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/avatar/avatar.tsx` around lines 198 - 203, The
current logic treats max=0 as falsy and can dereference avatars[0] when children
is empty; update the computation to explicitly check for max being a number
(e.g., max !== undefined) and convert children to an array
(React.Children.toArray(children)) before slicing so max={0} behaves correctly
and slicing is safe; compute count using the same numeric check (count = typeof
max === 'number' && childrenArray.length > max ? childrenArray.length - max : 0)
and guard getAvatarProps by only calling getAvatarProps(avatars[0]) when
avatars[0] exists (or pass a safe default) to avoid dereferencing undefined when
children is empty.
Comment on lines
6
to
+25
| const badge = cva(styles['badge'], { | ||
| variants: { | ||
| variant: { | ||
| accent: styles["badge-accent"], | ||
| warning: styles["badge-warning"], | ||
| danger: styles["badge-danger"], | ||
| success: styles["badge-success"], | ||
| neutral: styles["badge-neutral"], | ||
| gradient: styles["badge-gradient"] | ||
| accent: styles['badge-accent'], | ||
| warning: styles['badge-warning'], | ||
| danger: styles['badge-danger'], | ||
| success: styles['badge-success'], | ||
| neutral: styles['badge-neutral'], | ||
| gradient: styles['badge-gradient'] | ||
| }, | ||
| size: { | ||
| micro: styles["badge-micro"], | ||
| small: styles["badge-small"], | ||
| regular: styles["badge-regular"], | ||
| micro: styles['badge-micro'], | ||
| small: styles['badge-small'], | ||
| regular: styles['badge-regular'] | ||
| }, | ||
| defaultVariants: { | ||
| variant: "accent", | ||
| size: "small" | ||
| variant: 'accent', | ||
| size: 'small' | ||
| } | ||
| }, | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In class-variance-authority cva(), should defaultVariantsbe declared as a top-level option (sibling ofvariants) rather than nested inside variants?
💡 Result:
Yes. In class-variance-authority, defaultVariants is a top-level config option passed as the second argument to cva(), alongside (i.e., a sibling of) variants and compoundVariants—it is not meant to be nested inside variants. This is reflected both in the published TypeScript types and in the README examples. [1], [2]
Correct shape:
cva(base, {
variants: { /* ... */ },
defaultVariants: { /* ... */ },
compoundVariants: [ /* ... */ ],
});Sources:
[1] https://app.unpkg.com/class-variance-authority%400.2.3/files/dist/index.d.ts
[2] https://www.npmjs.com/package/class-variance-authority/v/0.2.4
defaultVariants is nested at the wrong level in cva config.
On Line 21, defaultVariants is inside variants. In cva, it should be a top-level sibling of variants; otherwise defaults are not applied via cva as intended, and VariantProps can be skewed.
Proposed fix
const badge = cva(styles['badge'], {
variants: {
variant: {
accent: styles['badge-accent'],
warning: styles['badge-warning'],
danger: styles['badge-danger'],
success: styles['badge-success'],
neutral: styles['badge-neutral'],
gradient: styles['badge-gradient']
},
size: {
micro: styles['badge-micro'],
small: styles['badge-small'],
regular: styles['badge-regular']
- },
- defaultVariants: {
- variant: 'accent',
- size: 'small'
}
+ },
+ defaultVariants: {
+ variant: 'accent',
+ size: 'small'
}
});📝 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
| const badge = cva(styles['badge'], { | |
| variants: { | |
| variant: { | |
| accent: styles["badge-accent"], | |
| warning: styles["badge-warning"], | |
| danger: styles["badge-danger"], | |
| success: styles["badge-success"], | |
| neutral: styles["badge-neutral"], | |
| gradient: styles["badge-gradient"] | |
| accent: styles['badge-accent'], | |
| warning: styles['badge-warning'], | |
| danger: styles['badge-danger'], | |
| success: styles['badge-success'], | |
| neutral: styles['badge-neutral'], | |
| gradient: styles['badge-gradient'] | |
| }, | |
| size: { | |
| micro: styles["badge-micro"], | |
| small: styles["badge-small"], | |
| regular: styles["badge-regular"], | |
| micro: styles['badge-micro'], | |
| small: styles['badge-small'], | |
| regular: styles['badge-regular'] | |
| }, | |
| defaultVariants: { | |
| variant: "accent", | |
| size: "small" | |
| variant: 'accent', | |
| size: 'small' | |
| } | |
| }, | |
| } | |
| const badge = cva(styles['badge'], { | |
| variants: { | |
| variant: { | |
| accent: styles['badge-accent'], | |
| warning: styles['badge-warning'], | |
| danger: styles['badge-danger'], | |
| success: styles['badge-success'], | |
| neutral: styles['badge-neutral'], | |
| gradient: styles['badge-gradient'] | |
| }, | |
| size: { | |
| micro: styles['badge-micro'], | |
| small: styles['badge-small'], | |
| regular: styles['badge-regular'] | |
| } | |
| }, | |
| defaultVariants: { | |
| variant: 'accent', | |
| size: 'small' | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/badge/badge.tsx` around lines 6 - 25, The cva
configuration for the badge constant places defaultVariants inside the variants
object, so defaults won't be applied; update the cva call that defines badge so
that defaultVariants is moved out of the variants block to be a top-level
sibling of variants (keep variant and size under variants and ensure
defaultVariants has variant: 'accent' and size: 'small'), preserving existing
variant names (accent, warning, danger, success, neutral, gradient) and sizes
(micro, small, regular) and the styles references used in badge.
Comment on lines
+3
to
+4
| export function Box(props: ComponentProps<'div'>) { | ||
| return <div {...props} />; |
Contributor
There was a problem hiding this comment.
Box drops its base class, causing a styling regression.
<div {...props} /> no longer guarantees the component’s default box styling (tests also expect styles.box). Since Box is part of the public surface, this is a high-impact behavioral change.
Suggested fix
import { ComponentProps } from 'react';
+import styles from './box.module.css';
-export function Box(props: ComponentProps<'div'>) {
- return <div {...props} />;
+export function Box({ className, ...props }: ComponentProps<'div'>) {
+ const mergedClassName = [styles.box, className].filter(Boolean).join(' ');
+ return <div {...props} className={mergedClassName} />;
}📝 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
| export function Box(props: ComponentProps<'div'>) { | |
| return <div {...props} />; | |
| import { ComponentProps } from 'react'; | |
| import styles from './box.module.css'; | |
| export function Box({ className, ...props }: ComponentProps<'div'>) { | |
| const mergedClassName = [styles.box, className].filter(Boolean).join(' '); | |
| return <div {...props} className={mergedClassName} />; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/box/box.tsx` around lines 3 - 4, Box currently
renders <div {...props}/> which drops the component's default class; update the
Box component (function Box) to merge the default class (e.g., styles.box or the
string "box") with props.className using a class-merge utility (classnames/clsx)
or manual concatenation, then spread the remaining props so the returned <div>
includes className={mergedClassName} and {...rest}; ensure props typing
(ComponentProps<'div'>) is preserved and that custom className values are
appended so callers can override styles.
Comment on lines
+27
to
67
| export const BreadcrumbItem = ({ | ||
| ref, | ||
| as, | ||
| children, | ||
| className, | ||
| leadingIcon, | ||
| current, | ||
| href, | ||
| dropdownItems, | ||
| ...props | ||
| }: BreadcrumbItemProps) => { | ||
| const renderedElement = as ?? <a ref={ref} />; | ||
| const label = ( | ||
| <> | ||
| {leadingIcon && ( | ||
| <span className={styles['breadcrumb-icon']}>{leadingIcon}</span> | ||
| )} | ||
| {children && <span>{children}</span>} | ||
| </> | ||
| ); | ||
|
|
||
| if (dropdownItems) { | ||
| return ( | ||
| <Menu> | ||
| <Menu.Trigger className={styles['breadcrumb-dropdown-trigger']}> | ||
| {label} | ||
| <ChevronDownIcon className={styles['breadcrumb-dropdown-icon']} /> | ||
| </Menu.Trigger> | ||
| <Menu.Content className={styles['breadcrumb-dropdown-content']}> | ||
| {dropdownItems.map((dropdownItem, dropdownIndex) => ( | ||
| <Menu.Item | ||
| key={dropdownItem.key ?? dropdownIndex} | ||
| className={styles['breadcrumb-dropdown-item']} | ||
| onClick={dropdownItem?.onClick} | ||
| > | ||
| {dropdownItem.label} | ||
| </Menu.Item> | ||
| ))} | ||
| </Menu.Content> | ||
| </Menu> | ||
| ); | ||
| } | ||
| if (dropdownItems) { | ||
| return ( | ||
| <li className={cx(styles['breadcrumb-item'], className)}> | ||
| {cloneElement( | ||
| renderedElement, | ||
| { | ||
| className: cx( | ||
| styles['breadcrumb-link'], | ||
| current && styles['breadcrumb-link-active'] | ||
| ), | ||
| href, | ||
| ...props, | ||
| ...renderedElement.props | ||
| }, | ||
| label | ||
| )} | ||
| </li> | ||
| <Menu> | ||
| <Menu.Trigger className={styles['breadcrumb-dropdown-trigger']}> | ||
| {label} | ||
| <ChevronDownIcon className={styles['breadcrumb-dropdown-icon']} /> | ||
| </Menu.Trigger> | ||
| <Menu.Content className={styles['breadcrumb-dropdown-content']}> | ||
| {dropdownItems.map((dropdownItem, dropdownIndex) => ( | ||
| <Menu.Item | ||
| key={dropdownItem.key ?? dropdownIndex} | ||
| className={styles['breadcrumb-dropdown-item']} | ||
| onClick={dropdownItem?.onClick} | ||
| > | ||
| {dropdownItem.label} | ||
| </Menu.Item> | ||
| ))} | ||
| </Menu.Content> | ||
| </Menu> | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if Menu.Trigger component supports ref forwarding
ast-grep --pattern $'export const MenuTrigger = $$$'
rg -n "MenuTrigger" --type ts --type tsx -A 5 | head -50Repository: raystack/apsara
Length of output: 2289
🏁 Script executed:
cat -n packages/raystack/components/breadcrumb/breadcrumb-item.tsxRepository: raystack/apsara
Length of output: 2810
Ref is silently ignored when dropdownItems is provided.
When dropdownItems is present, the component returns early (lines 48-67) without using the ref at all. Any ref passed by consumers will be silently dropped in this code path.
Consider forwarding ref to the Menu.Trigger element:
Suggested fix
if (dropdownItems) {
return (
<Menu>
- <Menu.Trigger className={styles['breadcrumb-dropdown-trigger']}>
+ <Menu.Trigger ref={ref} className={styles['breadcrumb-dropdown-trigger']}>
{label}
<ChevronDownIcon className={styles['breadcrumb-dropdown-icon']} />
</Menu.Trigger>📝 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
| export const BreadcrumbItem = ({ | |
| ref, | |
| as, | |
| children, | |
| className, | |
| leadingIcon, | |
| current, | |
| href, | |
| dropdownItems, | |
| ...props | |
| }: BreadcrumbItemProps) => { | |
| const renderedElement = as ?? <a ref={ref} />; | |
| const label = ( | |
| <> | |
| {leadingIcon && ( | |
| <span className={styles['breadcrumb-icon']}>{leadingIcon}</span> | |
| )} | |
| {children && <span>{children}</span>} | |
| </> | |
| ); | |
| if (dropdownItems) { | |
| return ( | |
| <Menu> | |
| <Menu.Trigger className={styles['breadcrumb-dropdown-trigger']}> | |
| {label} | |
| <ChevronDownIcon className={styles['breadcrumb-dropdown-icon']} /> | |
| </Menu.Trigger> | |
| <Menu.Content className={styles['breadcrumb-dropdown-content']}> | |
| {dropdownItems.map((dropdownItem, dropdownIndex) => ( | |
| <Menu.Item | |
| key={dropdownItem.key ?? dropdownIndex} | |
| className={styles['breadcrumb-dropdown-item']} | |
| onClick={dropdownItem?.onClick} | |
| > | |
| {dropdownItem.label} | |
| </Menu.Item> | |
| ))} | |
| </Menu.Content> | |
| </Menu> | |
| ); | |
| } | |
| if (dropdownItems) { | |
| return ( | |
| <li className={cx(styles['breadcrumb-item'], className)}> | |
| {cloneElement( | |
| renderedElement, | |
| { | |
| className: cx( | |
| styles['breadcrumb-link'], | |
| current && styles['breadcrumb-link-active'] | |
| ), | |
| href, | |
| ...props, | |
| ...renderedElement.props | |
| }, | |
| label | |
| )} | |
| </li> | |
| <Menu> | |
| <Menu.Trigger className={styles['breadcrumb-dropdown-trigger']}> | |
| {label} | |
| <ChevronDownIcon className={styles['breadcrumb-dropdown-icon']} /> | |
| </Menu.Trigger> | |
| <Menu.Content className={styles['breadcrumb-dropdown-content']}> | |
| {dropdownItems.map((dropdownItem, dropdownIndex) => ( | |
| <Menu.Item | |
| key={dropdownItem.key ?? dropdownIndex} | |
| className={styles['breadcrumb-dropdown-item']} | |
| onClick={dropdownItem?.onClick} | |
| > | |
| {dropdownItem.label} | |
| </Menu.Item> | |
| ))} | |
| </Menu.Content> | |
| </Menu> | |
| ); | |
| export const BreadcrumbItem = ({ | |
| ref, | |
| as, | |
| children, | |
| className, | |
| leadingIcon, | |
| current, | |
| href, | |
| dropdownItems, | |
| ...props | |
| }: BreadcrumbItemProps) => { | |
| const renderedElement = as ?? <a ref={ref} />; | |
| const label = ( | |
| <> | |
| {leadingIcon && ( | |
| <span className={styles['breadcrumb-icon']}>{leadingIcon}</span> | |
| )} | |
| {children && <span>{children}</span>} | |
| </> | |
| ); | |
| if (dropdownItems) { | |
| return ( | |
| <Menu> | |
| <Menu.Trigger ref={ref} className={styles['breadcrumb-dropdown-trigger']}> | |
| {label} | |
| <ChevronDownIcon className={styles['breadcrumb-dropdown-icon']} /> | |
| </Menu.Trigger> | |
| <Menu.Content className={styles['breadcrumb-dropdown-content']}> | |
| {dropdownItems.map((dropdownItem, dropdownIndex) => ( | |
| <Menu.Item | |
| key={dropdownItem.key ?? dropdownIndex} | |
| className={styles['breadcrumb-dropdown-item']} | |
| onClick={dropdownItem?.onClick} | |
| > | |
| {dropdownItem.label} | |
| </Menu.Item> | |
| ))} | |
| </Menu.Content> | |
| </Menu> | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/breadcrumb/breadcrumb-item.tsx` around lines 27
- 67, The component currently drops the ref when dropdownItems is present;
update BreadcrumbItem to accept and forward refs properly by converting it to a
React.forwardRef component (use forwardRef and a ref parameter) and pass that
forwarded ref into the Menu.Trigger element (and into the non-dropdown rendered
element path as before). Ensure the prop signature no longer destructures a raw
"ref" prop and instead uses the forwarded ref variable, and forward the ref to
Menu.Trigger (and to the anchor/renderedElement fallback) so consumers receive
their ref in both code paths.
Comment on lines
+202
to
+204
| <Popover.Trigger | ||
| render={isValidElement(trigger) ? trigger : <button>{trigger}</button>} | ||
| /> |
Contributor
There was a problem hiding this comment.
Set an explicit button type on the fallback trigger.
The fallback button defaults to type="submit", which can unintentionally submit surrounding forms.
Proposed fix
<Popover.Trigger
- render={isValidElement(trigger) ? trigger : <button>{trigger}</button>}
+ render={
+ isValidElement(trigger) ? trigger : <button type='button'>{trigger}</button>
+ }
/>📝 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
| <Popover.Trigger | |
| render={isValidElement(trigger) ? trigger : <button>{trigger}</button>} | |
| /> | |
| <Popover.Trigger | |
| render={ | |
| isValidElement(trigger) ? trigger : <button type='button'>{trigger}</button> | |
| } | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/calendar/date-picker.tsx` around lines 202 -
204, The fallback trigger button inside Popover.Trigger currently defaults to
type="submit" and may submit surrounding forms; update the fallback to
explicitly set type="button" (i.e., when rendering fallback
<button>{trigger}</button>) so the Popover.Trigger render branch uses a
non-submitting button; locate the Popover.Trigger usage and change the fallback
button declaration accordingly (referencing Popover.Trigger and the trigger
prop/variable).
Comment on lines
+170
to
+172
| <Popover.Trigger | ||
| render={isValidElement(trigger) ? trigger : <button>{trigger}</button>} | ||
| /> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the file
git ls-files packages/raystack/components/calendar/range-picker.tsxRepository: raystack/apsara
Length of output: 113
🏁 Script executed:
# Check if file exists and get its size
wc -l packages/raystack/components/calendar/range-picker.tsxRepository: raystack/apsara
Length of output: 117
🏁 Script executed:
# Read the relevant section around lines 170-172
sed -n '160,180p' packages/raystack/components/calendar/range-picker.tsxRepository: raystack/apsara
Length of output: 648
🏁 Script executed:
# Also check the imports at the beginning to see if isValidElement is indeed imported
head -20 packages/raystack/components/calendar/range-picker.tsxRepository: raystack/apsara
Length of output: 892
🏁 Script executed:
# Look for context around the Popover component and trigger prop definition
sed -n '1,50p' packages/raystack/components/calendar/range-picker.tsxRepository: raystack/apsara
Length of output: 1540
🏁 Script executed:
# Check if there are any parent form wrappers or form context usage
rg -A 5 -B 5 "Popover.Trigger" packages/raystack/components/calendar/range-picker.tsxRepository: raystack/apsara
Length of output: 445
🏁 Script executed:
# Search for similar button patterns in the codebase to check if type="button" is commonly used
rg '<button[^>]*>' packages/raystack/components/ -A 1 | head -50Repository: raystack/apsara
Length of output: 3826
Add explicit button type to fallback trigger
Line 171 renders a fallback <button> without a type attribute. While HTML defaults to type="submit", explicitly specifying type="button" prevents unintended form submission if this component is used inside a form and improves code clarity.
Suggested fix
<Popover.Trigger
render={isValidElement(trigger) ? trigger : <button type="button">{trigger}</button>}
/>📝 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
| <Popover.Trigger | |
| render={isValidElement(trigger) ? trigger : <button>{trigger}</button>} | |
| /> | |
| <Popover.Trigger | |
| render={isValidElement(trigger) ? trigger : <button type="button">{trigger}</button>} | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/calendar/range-picker.tsx` around lines 170 -
172, The fallback trigger renders an implicit submit button which can cause
unwanted form submissions; update the Popover.Trigger usage so that when the
fallback is used (i.e., render={isValidElement(trigger) ? trigger :
<button>{trigger}</button>}) the button includes an explicit type="button"
attribute; locate the Popover.Trigger line in range-picker.tsx and change the
fallback <button> to include type="button" to prevent form submissions when used
inside forms.
rohanchkrabrty
changed the title
rsbh
approved these changes
Mar 18, 2026
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
React.forwardRef()wrappers from 10 components (accordion through calendar), using ref-as-prop (React 19 Case A/B pattern)ComponentPropsWithoutRef/HTMLAttributes→ComponentPropsto include ref in typesdisplayNameon all exported components, matching their public export names (e.g.'Accordion','Breadcrumb.Item')Components migrated:
Summary by CodeRabbit