Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughThis pull request comprehensively refactors the Dialog component system by extracting DialogContent into a dedicated module, consolidating overlay-related props into a single object, and replacing asChild patterns with render props. Nested dialog animation support and updated component organization are introduced. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
rohanchkrabrty
requested review from
paanSinghCoder and
rsbh
and removed request for
rsbh
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
apps/www/src/components/playground/dialog-examples.tsx (1)
11-25:⚠️ Potential issue | 🟡 MinorRemove duplicate close button from the example.
Dialog.Contentautomatically renders a styled close button (CloseButtonwith icon and aria-label) by default sinceshowCloseButtonis not overridden. Line 24's<Dialog.Close />adds a second, unstyled close button, creating duplicate close affordances. Remove it.Suggested fix
- <Dialog.Close />
🤖 Fix all issues with AI agents
In `@apps/www/src/components/playground/dialog-examples.tsx`:
- Around line 28-30: The Dialog.Trigger currently uses a render prop that
injects a Button (<Dialog.Trigger render={<Button variant='outline' />}>), but
also nests a <Button>Open Dialog</Button> as children which creates a
nested-button DOM; remove the nested Button child and replace it with plain text
(e.g., "Open Dialog") so the render prop provides the actual Button element and
the children become the trigger label; ensure only one Button instance is
present and keep references to Dialog.Trigger and Button when editing.
In `@apps/www/src/content/docs/components/dialog/demo.ts`:
- Around line 3-4: The getCode function can emit "undefined" when title or
description are missing; update the destructuring in getCode (or the template
interpolation) to default title and description to empty strings (e.g., const {
title = '', description = '' } = props or use fallback expressions when building
the template) so the generated snippet never contains the literal "undefined".
- Around line 145-179: When the parent dialog (parentOpen) closes we should
reset the nested dialog state (nestedOpen) to false so reopening the parent
doesn't auto-open the nested dialog; add a handler or useEffect in the component
that watches parentOpen (or inside setParentOpen wrapper) and calls
setNestedOpen(false) whenever parentOpen becomes false, referencing the existing
state setters setParentOpen and setNestedOpen used with the Dialog components.
In `@packages/raystack/components/dialog/dialog-content.tsx`:
- Around line 13-17: The JSDoc for the showNestedAnimation prop is backwards: it
currently says "Disables nested dialog animation" while the prop name and
`@default` true indicate it enables animation. Update the comment for
showNestedAnimation in dialog-content.tsx to correctly describe that it enables
nested dialog animations (scaling and translation) and keep `@default` true (or
adjust the default if intended otherwise); ensure the text references the
showNestedAnimation prop so consumers understand that true turns the nested
animation on.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/raystack/components/avatar/avatar.tsx (1)
196-200:⚠️ Potential issue | 🟡 MinorPotential runtime error with empty children array.
If
childrenis an empty array,avatars[0]will beundefined, andgetAvatarProps(undefined)will throw when accessingelement.props. Consider adding a guard.🛡️ Proposed defensive check
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]); + const firstAvatarProps = avatars[0] ? getAvatarProps(avatars[0]) : {};apps/www/src/content/docs/components/dialog/props.ts (1)
69-83:⚠️ Potential issue | 🔴 CriticalUpdate
DialogTriggerPropsandDialogCloseButtonPropsto reflect Base UI's actual API.Since
Dialog.TriggerandDialog.Closeare direct exports from@base-ui/react, they use Base UI'srenderprop pattern for composition, notasChild. The props.ts documentation currently listsasChild?: booleanfor bothDialogTriggerProps(line 71) andDialogCloseButtonProps(line 79), which is inconsistent with the actual Base UI API.Update these interfaces to include the
renderprop and removeasChild, matching Base UI's documented composition pattern.
🤖 Fix all issues with AI agents
In `@apps/www/src/content/docs/components/dialog/index.mdx`:
- Around line 82-85: Update the "Nested Dialogs" paragraph to state that the
parent dialog scaling and transparency only happen when the showNestedAnimation
prop is enabled (it defaults to true); mention that the nested dialog backdrop
is not rendered in this mode so the parent remains visible, and reference
showNestedAnimation explicitly so readers know this behavior is gated by that
prop.
In `@apps/www/src/content/docs/components/scroll-area/props.ts`:
- Around line 34-36: The render prop type for the ScrollArea component is
missing the second `state` parameter; update the `render?` union so the function
signature accepts `(props: React.HTMLAttributes<HTMLElement>, state:
ScrollArea.Root.State)` instead of only `props`, ensuring the type matches the
actual Base UI implementation (reference the `render` prop and
`ScrollArea.Root.State` types in the declaration).
In `@apps/www/src/content/docs/components/sidebar/props.ts`:
- Around line 15-18: The JSDoc for the collapsible prop is backwards: update the
comment for the collapsible?: boolean property so it correctly states that when
true the Sidebar can be collapsed/expanded (renders the collapse button) and
when false it disables that behavior; keep the `@default` true but change the
description from "Disable the click to collapse/expand the Sidebar" to something
like "Enable the click to collapse/expand the Sidebar" and include the behavior
note referencing the collapse button.
In `@apps/www/src/content/docs/components/switch/props.ts`:
- Around line 38-48: The render state type for the switch includes readOnly but
SwitchProps lacks a corresponding prop; update the type definition to be
consistent by either adding readOnly?: boolean to the SwitchProps interface (so
components can control it) or removing readOnly from the state union in the
render? declaration if the switch does not support a read-only mode; adjust
related places that reference SwitchProps or the render callback signature (look
for SwitchProps and the render? type in
apps/www/src/content/docs/components/switch/props.ts) to ensure the prop and
state stay in sync.
In `@packages/raystack/components/dialog/dialog-content.tsx`:
- Around line 39-46: The overlay object is being spread onto
DialogPrimitive.Backdrop which will pass the custom blur prop to the DOM;
destructure blur out of overlay first (e.g., const { blur, ...overlayProps } =
overlay || {}) and then spread overlayProps into DialogPrimitive.Backdrop while
using blur only for the conditional className (styles.overlayBlur) in the cx
call; update occurrences of {...overlay} to {...overlayProps} and keep
overlay?.className handling as-is to avoid leaking the blur attribute to the
DOM.
🧹 Nitpick comments (9)
packages/raystack/components/sidebar/__tests__/sidebar.test.tsx (1)
99-106: Consider adding a symmetric test for the open state.The test verifies the collapsed state attributes but doesn't explicitly verify that
data-openis present when the sidebar is expanded. Consider adding a complementary test for symmetry and completeness.🧪 Suggested test addition
it('has data-open attribute when expanded', () => { render(<BasicSidebar open={true} />); const nav = screen.getByRole('navigation'); expect(nav).toHaveAttribute('data-open'); expect(nav).not.toHaveAttribute('data-closed'); });packages/raystack/components/switch/__tests__/switch.test.tsx (1)
177-196: Consider updating controlled component test to match the new callback signature.The
onCheckedChangecallback now receives(checked: boolean, event: Event), but this test passessetCheckeddirectly. While this works (extra arguments are ignored), it doesn't demonstrate the actual API usage pattern. Consider using a wrapper function for consistency with other tests.🔧 Suggested improvement
it('works as controlled component', () => { const Component = () => { const [checked, setChecked] = React.useState(false); return ( <Switch checked={checked} - onCheckedChange={setChecked} + onCheckedChange={(newChecked) => setChecked(newChecked)} data-testid='controlled' /> ); };apps/www/src/content/docs/components/checkbox/demo.ts (1)
5-8: Optional: remove legacy string coercion in getCode.
With checkbox controls now boolean, these conversions appear unnecessary.♻️ Suggested cleanup
export const getCode = (props: any) => { - if (props.checked === 'false') props.checked = false; - else if (props.checked === 'true') props.checked = true; - return `<Checkbox${getPropsString(props)}/>`; };packages/raystack/components/separator/__tests__/separator.test.tsx (1)
88-92: Test duplicates assertion from "Orientation" test suite.The assertion
expect(separator).toHaveAttribute('aria-orientation', 'horizontal')is already verified in the "defaults to horizontal orientation" test at lines 29-34. Consider either:
- Removing this test to avoid redundancy, or
- Renaming and modifying it to test something distinct (e.g., verifying
aria-orientationis always present regardless of theorientationprop value)apps/www/src/content/docs/components/radio/props.ts (2)
3-9: Prefer specific types overanyfor value props.Using
anyfordefaultValue,value, and theonValueChangeparameter removes type safety and provides no guidance to consumers. Radio values are typically strings.Consider using
stringor a generic type parameter for better type safety in the documentation.♻️ Suggested type improvement
- /** The value of the radio item that should be checked by default. */ - defaultValue?: any; + /** The value of the radio item that should be checked by default. */ + defaultValue?: string; /** The controlled value of the radio item that is checked. */ - value?: any; + value?: string; /** Event handler called when the value changes. */ - onValueChange?: (value: any, event: Event) => void; + onValueChange?: (value: string, event: Event) => void;
31-33: Same type safety concern for RadioProps value.The
valueprop should usestring(or match the group's type) instead ofanyfor consistency and type safety.♻️ Suggested fix
export interface RadioProps { /** The unique value of the radio item. */ - value: any; + value: string;packages/raystack/components/dialog/dialog.module.css (2)
23-34: Consider narrowing the transition property.Using
transition: all 150mson Line 26 will animate any property change (includingwidth,height,background-color, etc.), which may cause unintended visual effects. Consider specifying only the properties that should animate.♻️ Suggested refinement
-.dialogContent { - transition: all 150ms; +.dialogContent { + transition: opacity 150ms, transform 150ms, translate 150ms;
30-34: Mixingtransformandtranslateproperties.The
.showNestedAnimationclass uses both thetransformproperty (withscale()) and the separatetranslateproperty. While this is valid modern CSS, it can be confusing since both affect element positioning. Thetranslateproperty won't be included in thetransformcalculation and operates independently.This works correctly but consider documenting this intentional separation or consolidating into a single
transformfor clarity.packages/raystack/components/dialog/dialog-misc.tsx (1)
55-71: Consider consolidatingCloseButtonimplementations.There's a similar
CloseButtoninpackages/raystack/components/sheet/sheet.tsx(Lines 98-112) that also wrapsDialogPrimitive.Close. Both serve the same purpose but have slightly different implementations. Consider extracting a sharedCloseButtoncomponent to reduce duplication.
| /** Disable the click to collapse/expand the Sidebar. | ||
| * @default undefined | ||
| * @default true | ||
| */ | ||
| collapsible?: boolean; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n -C5 'collapsible' --type=ts --type=tsx -g '*sidebar*' -g '!**/props.ts'Repository: raystack/apsara
Length of output: 86
🏁 Script executed:
rg -n -C5 'collapsible' --type=ts -g '*sidebar*' -g '!**/props.ts'Repository: raystack/apsara
Length of output: 10190
Update the sidebar collapsible prop documentation to match its actual behavior.
The prop name collapsible and the default value of true indicate that when enabled, the sidebar can be collapsed/expanded. However, the current description ("Disable the click to collapse/expand the Sidebar") is backwards. The implementation confirms that collapsible: true enables the collapse/expand functionality (renders the button), while collapsible: false disables it (hides the button).
📝 Suggested documentation fix
- /** Disable the click to collapse/expand the Sidebar.
+ /** Enable the sidebar to be collapsed/expanded via click.
* `@default` true
*/
collapsible?: boolean;📝 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.
| /** Disable the click to collapse/expand the Sidebar. | |
| * @default undefined | |
| * @default true | |
| */ | |
| collapsible?: boolean; | |
| /** Enable the sidebar to be collapsed/expanded via click. | |
| * `@default` true | |
| */ | |
| collapsible?: boolean; |
🤖 Prompt for AI Agents
In `@apps/www/src/content/docs/components/sidebar/props.ts` around lines 15 - 18,
The JSDoc for the collapsible prop is backwards: update the comment for the
collapsible?: boolean property so it correctly states that when true the Sidebar
can be collapsed/expanded (renders the collapse button) and when false it
disables that behavior; keep the `@default` true but change the description from
"Disable the click to collapse/expand the Sidebar" to something like "Enable the
click to collapse/expand the Sidebar" and include the behavior note referencing
the collapse button.
| <DialogPrimitive.Backdrop | ||
| {...overlay} | ||
| className={cx( | ||
| styles.dialogOverlay, | ||
| overlay?.blur && styles.overlayBlur, | ||
| overlay?.className | ||
| )} | ||
| /> |
There was a problem hiding this comment.
Potential prop leak: blur passed to DOM element.
The overlay object is spread directly onto DialogPrimitive.Backdrop, but blur is a custom prop used only for conditional className. This will pass blur as an attribute to the underlying DOM element, causing a React warning for unknown props.
🛠️ Suggested fix: destructure blur before spreading
(
{
className,
children,
showCloseButton = true,
overlay,
width,
style,
showNestedAnimation = true,
...props
},
ref
) => {
+ const { blur, ...overlayProps } = overlay ?? {};
return (
<DialogPrimitive.Portal>
<DialogPrimitive.Backdrop
- {...overlay}
+ {...overlayProps}
className={cx(
styles.dialogOverlay,
- overlay?.blur && styles.overlayBlur,
+ blur && styles.overlayBlur,
overlay?.className
)}
/>🤖 Prompt for AI Agents
In `@packages/raystack/components/dialog/dialog-content.tsx` around lines 39 - 46,
The overlay object is being spread onto DialogPrimitive.Backdrop which will pass
the custom blur prop to the DOM; destructure blur out of overlay first (e.g.,
const { blur, ...overlayProps } = overlay || {}) and then spread overlayProps
into DialogPrimitive.Backdrop while using blur only for the conditional
className (styles.overlayBlur) in the cx call; update occurrences of
{...overlay} to {...overlayProps} and keep overlay?.className handling as-is to
avoid leaking the blur attribute to the DOM.
2 participants
Description
Changes
showNestedAnimationpropshowCloseButton={true} (default)Docs & Tests
Summary by CodeRabbit
Release Notes
New Features
showNestedAnimationcontrol for nested dialog transitions.Refactor
asChildpattern with explicitrenderprops for cleaner composition.overlayobject for simplified customization.closeprop toshowCloseButtonfor clarity.Documentation