Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughA large-scale refactoring removes 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/raystack/components/data-table/components/search.tsx (1)
7-13: Narrow public props to exclude internally controlledSearchfields.
DataTableSearchPropscurrently includesonChange,value, andonClear, but those are always overridden (Line 56, Line 57, Line 58). This makes the API misleading for consumers.♻️ Proposed refactor
-export interface DataTableSearchProps extends ComponentProps<typeof Search> { +export interface DataTableSearchProps + extends Omit<ComponentProps<typeof Search>, 'onChange' | 'value' | 'onClear'> { /** * Automatically disable search in zero state (when no data and no filters/search applied). * `@defaultValue` true */ autoDisableInZeroState?: boolean; }Also applies to: 53-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/data-table/components/search.tsx` around lines 7 - 13, DataTableSearchProps should not expose props that are internally controlled; replace the current extension of ComponentProps<typeof Search> with an omitted type that removes 'onChange', 'value', and 'onClear' (e.g., Omit<ComponentProps<typeof Search>, 'onChange'|'value'|'onClear'>) and then add the autoDisableInZeroState? boolean back in; update any references to DataTableSearchProps and the Search usage to rely on the component's internal handlers rather than consumer-supplied onChange/value/onClear.
🤖 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/data-table/data-table.tsx`:
- Line 218: The return uses React 19 context shorthand (<TableContext
value={contextValue}>) which breaks React 18; revert to the React 18-compatible
API by returning <TableContext.Provider
value={contextValue}>{children}</TableContext.Provider> in the component that
references TableContext (use the same contextValue and children identifiers), or
if you intentionally want React 19-only, update package.json to remove ^18 from
the supported engines; pick one approach and make the code and package.json
consistent (do not leave mixed syntax like TableContext here while ThemeContext
uses .Provider).
In `@packages/raystack/components/dialog/dialog-content.tsx`:
- Around line 19-37: DialogContent is spreading the overlay prop directly into
DialogPrimitive.Backdrop which forwards overlay.blur (a wrapper-only prop) and
wraps overlay.className in cx, converting a potential state callback into a
string; update DialogContent so it does NOT spread overlay blindly into
DialogPrimitive.Backdrop (remove overlay.blur before forwarding) and pass
className to Backdrop by merging styles while preserving a function callback:
compute backdropClassName by combining styles.dialogOverlay and
overlay?.blur-derived classes, then if overlay.className is a function call
Backdrop with that function (or pass it through unchanged) so state-aware
className callbacks from overlay.className are preserved when rendering
DialogPrimitive.Backdrop.
In `@packages/raystack/components/empty-state/empty-state.tsx`:
- Around line 24-40: EmptyState currently spreads incoming props (including
className) onto the root Flex causing caller className to replace
styles.emptyStatePage / styles.emptyState; extract className from the props (or
the component args) and merge it with the root style using a classnames utility
(e.g., clsx or classnames) when rendering the root Flex in EmptyState, then
spread the remaining props (without className). Do this for the variant ===
'empty2' root Flex (use styles.emptyStatePage) and the other root Flex used for
the non-empty2 branch (use styles.emptyState), and also include any classNames
prop in the merge if intended (EmptyStateProps / classNames / icon/heading etc.
are the identifiers to locate the component).
In `@packages/raystack/components/headline/headline.tsx`:
- Around line 49-64: The Headline function was converted to a plain component
and removed forwardRef, breaking ref forwarding expected by tests and React 18
consumers; restore ref forwarding by wrapping Headline with React.forwardRef
(keep HeadlineProps), accept a ref parameter and pass it through to the rendered
"Component" element, and export the forwarded component so existing tests
(headline.test.tsx) and consumers relying on refs continue to work without
changing peerDependencies.
---
Nitpick comments:
In `@packages/raystack/components/data-table/components/search.tsx`:
- Around line 7-13: DataTableSearchProps should not expose props that are
internally controlled; replace the current extension of ComponentProps<typeof
Search> with an omitted type that removes 'onChange', 'value', and 'onClear'
(e.g., Omit<ComponentProps<typeof Search>, 'onChange'|'value'|'onClear'>) and
then add the autoDisableInZeroState? boolean back in; update any references to
DataTableSearchProps and the Search usage to rely on the component's internal
handlers rather than consumer-supplied onChange/value/onClear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e3bf4fd2-63c6-488f-a988-ae39d656a206
📒 Files selected for processing (19)
packages/raystack/components/copy-button/copy-button.tsxpackages/raystack/components/data-table/components/content.tsxpackages/raystack/components/data-table/components/display-settings.tsxpackages/raystack/components/data-table/components/filters.tsxpackages/raystack/components/data-table/components/search.tsxpackages/raystack/components/data-table/components/toolbar.tsxpackages/raystack/components/data-table/components/virtualized-content.tsxpackages/raystack/components/data-table/data-table.tsxpackages/raystack/components/dialog/dialog-content.tsxpackages/raystack/components/dialog/dialog-misc.tsxpackages/raystack/components/drawer/drawer-content.tsxpackages/raystack/components/drawer/drawer-misc.tsxpackages/raystack/components/drawer/drawer-root.tsxpackages/raystack/components/empty-state/empty-state.tsxpackages/raystack/components/flex/flex.tsxpackages/raystack/components/grid/grid-item.tsxpackages/raystack/components/grid/grid.tsxpackages/raystack/components/headline/headline.tsxpackages/raystack/components/icon-button/icon-button.tsx
| {children} | ||
| </TableContext.Provider> | ||
| ); | ||
| return <TableContext value={contextValue}>{children}</TableContext>; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the minimum React version required in package.json
fd -t f 'package.json' packages/raystack --exec cat {} | jq -r '.peerDependencies.react // .dependencies.react // "not found"' 2>/dev/null | head -5
# Also check if there are any other context usages in the codebase that still use .Provider
rg -n '\.Provider' --type ts --type tsx -g '*.tsx' -g '*.ts' packages/raystack/Repository: raystack/apsara
Length of output: 114
🏁 Script executed:
# Search for .Provider usages in the entire packages/raystack directory
rg -n 'Provider' packages/raystack --type ts --type js -A 2 | head -100
# Also check the specific file for any other context-related changes
cat -n packages/raystack/components/data-table/data-table.tsx | sed -n '210,230p'Repository: raystack/apsara
Length of output: 8661
Remove React 19-only context syntax or drop React 18 support.
The change on line 218 uses <TableContext value={contextValue}> (React 19 syntax) instead of <TableContext.Provider value={...}> (React 18 syntax). Since package.json declares support for ^18 || ^19, this breaks React 18 compatibility. Other contexts in the codebase (e.g., ThemeContext in theme-provider/theme.tsx line 201) still use the .Provider syntax, indicating this is an inconsistent partial migration. Either revert to .Provider syntax for React 18 compatibility or explicitly drop React 18 support from package.json.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/data-table/data-table.tsx` at line 218, The
return uses React 19 context shorthand (<TableContext value={contextValue}>)
which breaks React 18; revert to the React 18-compatible API by returning
<TableContext.Provider value={contextValue}>{children}</TableContext.Provider>
in the component that references TableContext (use the same contextValue and
children identifiers), or if you intentionally want React 19-only, update
package.json to remove ^18 from the supported engines; pick one approach and
make the code and package.json consistent (do not leave mixed syntax like
TableContext here while ThemeContext uses .Provider).
| export function DialogContent({ | ||
| className, | ||
| children, | ||
| showCloseButton = true, | ||
| overlay, | ||
| width, | ||
| style, | ||
| showNestedAnimation = true, | ||
| ...props | ||
| }: DialogContentProps) { | ||
| return ( | ||
| <DialogPrimitive.Portal> | ||
| <DialogPrimitive.Backdrop | ||
| {...overlay} | ||
| className={cx( | ||
| styles.dialogOverlay, | ||
| overlay?.blur && styles.overlayBlur, | ||
| overlay?.className | ||
| )} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*dialog*" | head -20Repository: raystack/apsara
Length of output: 730
🏁 Script executed:
cat -n packages/raystack/components/dialog/dialog-content.tsx | head -50Repository: raystack/apsara
Length of output: 1784
🏁 Script executed:
rg "DialogPrimitive" packages/raystack/components/dialog/ -A 2 -B 2Repository: raystack/apsara
Length of output: 6169
🏁 Script executed:
rg "DialogContentProps" packages/raystack/components/dialog/ -A 10 -B 2Repository: raystack/apsara
Length of output: 2174
🌐 Web query:
@base-ui/react Dialog Backdrop className API callback state
💡 Result:
The @base-ui/react Dialog Backdrop component supports a className prop that accepts either a static string or a callback function receiving the component's state. Key details from official Base UI documentation: - className: string | ((state: Dialog.Backdrop.State) => string | undefined) - Description: CSS class applied to the element, or a function that returns a class based on the component’s state. Similar support exists for style: React.CSSProperties | ((state: Dialog.Backdrop.State) => CSSProperties | undefined) The exact shape of Dialog.Backdrop.State is not detailed in the public docs but typically includes properties like open and transitionStatus based on component patterns (e.g., open, mounted, transitioning). Example usage: <Dialog.Backdrop className={(state) => state.open ? 'backdrop-open' : 'backdrop-closed'} /> Data attributes like data-open are also present on the rendered div for additional styling. This API enables dynamic styling of the backdrop based on its open/transition state. Root-level state control uses open and onOpenChange props.
Citations:
- 1: https://base-ui.com/react/components/dialog
- 2: https://v1.reui.io/docs/base-dialog
- 3: https://base-ui.com/react/components/alert-dialog
- 4: https://forums.basehub.com/mui/base-ui/1
🏁 Script executed:
# Check package.json to confirm `@base-ui/react` version
rg "@base-ui/react" packages/raystack/components/ -B 2 -A 2Repository: raystack/apsara
Length of output: 31037
🏁 Script executed:
# Check if there are any tests or usage examples showing overlay className
rg "overlay.*className" packages/raystack/components/dialog/ -A 3 -B 3Repository: raystack/apsara
Length of output: 655
🏁 Script executed:
# Look for any test files to understand expected behavior
cat -n packages/raystack/components/dialog/__tests__/dialog.test.tsx | head -100Repository: raystack/apsara
Length of output: 3919
Fix overlay.className callback handling and prevent blur leak.
DialogPrimitive.Backdrop supports className as a string or a state callback function. Wrapping overlay?.className in cx(...) converts any function to a stringified value, breaking state-aware styling. Additionally, blur is a wrapper-only property and should not be forwarded to the primitive via {...overlay}.
Suggested fix
export function DialogContent({
className,
children,
showCloseButton = true,
overlay,
width,
style,
showNestedAnimation = true,
...props
}: DialogContentProps) {
+ const {
+ blur,
+ className: overlayClassName,
+ ...overlayProps
+ } = overlay ?? {};
+
return (
<DialogPrimitive.Portal>
<DialogPrimitive.Backdrop
- {...overlay}
- className={cx(
- styles.dialogOverlay,
- overlay?.blur && styles.overlayBlur,
- overlay?.className
- )}
+ {...overlayProps}
+ className={
+ typeof overlayClassName === 'function'
+ ? state =>
+ cx(
+ styles.dialogOverlay,
+ blur && styles.overlayBlur,
+ overlayClassName(state)
+ )
+ : cx(styles.dialogOverlay, blur && styles.overlayBlur, overlayClassName)
+ }
/>📝 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.
| export function DialogContent({ | |
| className, | |
| children, | |
| showCloseButton = true, | |
| overlay, | |
| width, | |
| style, | |
| showNestedAnimation = true, | |
| ...props | |
| }: DialogContentProps) { | |
| return ( | |
| <DialogPrimitive.Portal> | |
| <DialogPrimitive.Backdrop | |
| {...overlay} | |
| className={cx( | |
| styles.dialogOverlay, | |
| overlay?.blur && styles.overlayBlur, | |
| overlay?.className | |
| )} | |
| export function DialogContent({ | |
| className, | |
| children, | |
| showCloseButton = true, | |
| overlay, | |
| width, | |
| style, | |
| showNestedAnimation = true, | |
| ...props | |
| }: DialogContentProps) { | |
| const { | |
| blur, | |
| className: overlayClassName, | |
| ...overlayProps | |
| } = overlay ?? {}; | |
| return ( | |
| <DialogPrimitive.Portal> | |
| <DialogPrimitive.Backdrop | |
| {...overlayProps} | |
| className={ | |
| typeof overlayClassName === 'function' | |
| ? state => | |
| cx( | |
| styles.dialogOverlay, | |
| blur && styles.overlayBlur, | |
| overlayClassName(state) | |
| ) | |
| : cx(styles.dialogOverlay, blur && styles.overlayBlur, overlayClassName) | |
| } | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/dialog/dialog-content.tsx` around lines 19 - 37,
DialogContent is spreading the overlay prop directly into
DialogPrimitive.Backdrop which forwards overlay.blur (a wrapper-only prop) and
wraps overlay.className in cx, converting a potential state callback into a
string; update DialogContent so it does NOT spread overlay blindly into
DialogPrimitive.Backdrop (remove overlay.blur before forwarding) and pass
className to Backdrop by merging styles while preserving a function callback:
compute backdropClassName by combining styles.dialogOverlay and
overlay?.blur-derived classes, then if overlay.className is a function call
Backdrop with that function (or pass it through unchanged) so state-aware
className callbacks from overlay.className are preserved when rendering
DialogPrimitive.Backdrop.
| export function EmptyState({ | ||
| icon, | ||
| heading, | ||
| subHeading, | ||
| primaryAction, | ||
| secondaryAction, | ||
| classNames, | ||
| variant = 'empty1' | ||
| }: EmptyStateProps) => { | ||
| variant = 'empty1', | ||
| ...props | ||
| }: EmptyStateProps) { | ||
| if (variant === 'empty2') { | ||
| return ( | ||
| <Flex align='center' justify='center' className={styles.emptyStatePage}> | ||
| <Flex | ||
| align='center' | ||
| justify='center' | ||
| className={styles.emptyStatePage} | ||
| {...props} |
There was a problem hiding this comment.
Merge the outer className instead of letting it replace the root styles.
EmptyStateProps now accepts Flex props, but className stays inside ...props and both root Flex elements spread ...props after setting their own className. Any caller-supplied className therefore replaces styles.emptyStatePage / styles.emptyState instead of augmenting them, which drops the component’s base layout styling.
💡 Suggested fix
export function EmptyState({
icon,
heading,
subHeading,
primaryAction,
secondaryAction,
classNames,
+ className,
variant = 'empty1',
...props
}: EmptyStateProps) {
if (variant === 'empty2') {
return (
<Flex
align='center'
justify='center'
- className={styles.emptyStatePage}
+ className={cx(styles.emptyStatePage, className)}
{...props}
>
@@
return (
<Flex
direction='column'
align='center'
gap='medium'
- className={cx(styles.emptyState, classNames?.container)}
+ className={cx(styles.emptyState, classNames?.container, className)}
{...props}
>Also applies to: 86-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/empty-state/empty-state.tsx` around lines 24 -
40, EmptyState currently spreads incoming props (including className) onto the
root Flex causing caller className to replace styles.emptyStatePage /
styles.emptyState; extract className from the props (or the component args) and
merge it with the root style using a classnames utility (e.g., clsx or
classnames) when rendering the root Flex in EmptyState, then spread the
remaining props (without className). Do this for the variant === 'empty2' root
Flex (use styles.emptyStatePage) and the other root Flex used for the non-empty2
branch (use styles.emptyState), and also include any classNames prop in the
merge if intended (EmptyStateProps / classNames / icon/heading etc. are the
identifiers to locate the component).
| export function Headline({ | ||
| className, | ||
| size, | ||
| weight, | ||
| align, | ||
| truncate, | ||
| as: Component = 'h2', | ||
| ...props | ||
| }: HeadlineProps) { | ||
| return ( | ||
| <Component | ||
| className={headline({ size, weight, align, truncate, className })} | ||
| {...props} | ||
| /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether React 18 is still declared in peerDependencies and whether tests still require ref forwarding.
set -euo pipefail
echo "== package.json files mentioning peerDependencies/react =="
fd '^package\.json$' | xargs -I{} sh -c '
if rg -n "\"peerDependencies\"|\"react\"|\"react-dom\"" "{}" >/dev/null; then
echo "--- {}"
rg -n "\"peerDependencies\"|\"react\"|\"react-dom\"" "{}"
fi
'
echo
echo "== Headline ref-forwarding expectation in tests =="
rg -n -C2 "forwards ref correctly|<Headline ref=|toHaveBeenCalled\\(" packages/raystack/components/headline/__tests__/headline.test.tsx
echo
echo "Expected:"
echo "1) If peerDependencies include React 18, forwardRef removal is a compatibility break."
echo "2) If peerDependencies are React 19+ only, update release notes/versioning and keep tests aligned with intended behavior."Repository: raystack/apsara
Length of output: 944
Ref forwarding becomes a breaking change for React 18 consumers
The change at line 49 removed forwardRef and converted Headline to a plain function component. This breaks ref forwarding in React 18, even though packages/raystack/package.json declares React 18 support in peerDependencies ("react": "^18 || ^19"). The test at packages/raystack/components/headline/__tests__/headline.test.tsx lines 13–17 explicitly expects refs to be forwarded and will fail in React 18 with the current implementation.
Either restore forwardRef to maintain React 18 compatibility, or drop React 18 from peerDependencies and mark this as a breaking release.
Patch to restore React 18 + 19 compatibility
-import { ComponentProps } from 'react';
+import { ComponentProps, forwardRef } from 'react';
@@
-export function Headline({
- className,
- size,
- weight,
- align,
- truncate,
- as: Component = 'h2',
- ...props
-}: HeadlineProps) {
- return (
- <Component
- className={headline({ size, weight, align, truncate, className })}
- {...props}
- />
- );
-}
+export const Headline = forwardRef<HTMLHeadingElement, HeadlineProps>(
+ function Headline(
+ { className, size, weight, align, truncate, as: Component = 'h2', ...props },
+ ref
+ ) {
+ return (
+ <Component
+ ref={ref}
+ className={headline({ size, weight, align, truncate, className })}
+ {...props}
+ />
+ );
+ }
+);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/headline/headline.tsx` around lines 49 - 64, The
Headline function was converted to a plain component and removed forwardRef,
breaking ref forwarding expected by tests and React 18 consumers; restore ref
forwarding by wrapping Headline with React.forwardRef (keep HeadlineProps),
accept a ref parameter and pass it through to the rendered "Component" element,
and export the forwarded component so existing tests (headline.test.tsx) and
consumers relying on refs continue to work without changing peerDependencies.
Summary
React.forwardRef()wrappers from 9 components (copy-button through icon-button), using ref-as-prop (React 19 Case A pattern)ComponentPropsWithoutRef/HTMLAttributes→ComponentPropsto include ref in typesdisplayNameon all exported components, matching their public export namesComponents migrated:
Summary by CodeRabbit
Release Notes
Refactor
Improvements
EmptyStatecomponent to support additionalFlexproperties for greater customization flexibility.Bug Fixes