AlertDialog Test coverage improvement #1508
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds controllable open state, ID propagation, asChild/forceMount/keepMounted/disabled props, ARIA/data-state attributes, portal container control, focus/keyboard handling, context expansions, stories and styles updates, and comprehensive API + accessibility tests across AlertDialog and Dialog primitives. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Trigger as AlertDialog.Trigger
participant Root as AlertDialog.Root
participant Ctx as AlertDialogContext
participant Portal as DialogPrimitive.Portal
participant Overlay as DialogPrimitive.Overlay
participant Content as DialogPrimitive.Content
participant Title as AlertDialog.Title
participant Desc as AlertDialog.Description
Note over Root,Ctx: Root provides isOpen/defaultOpen, setters and ID setters
User->>Trigger: click
Trigger->>Ctx: read isOpen → data-state
alt trigger disabled
Trigger--xRoot: click blocked
else
Trigger->>Root: request open
Root->>Ctx: setIsOpen(true)
end
Ctx-->>Portal: container/forceMount/keepMounted
Portal->>Overlay: mount when isOpen || forceMount (data-state)
Overlay->>Content: render (role + aria + data-state)
par ID propagation
Title->>Ctx: setTitleId(id)
Desc->>Ctx: setDescriptionId(id)
end
User->>Content: dismiss (Escape/Action)
Content->>Root: request close
Root->>Ctx: setIsOpen(false)
Root->>Trigger: return focus
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/ui/AlertDialog/fragments/AlertDialogTrigger.tsx (1)
3-3: Incorrect clsx import — will be undefined at runtime.
clsxhas a default export; destructuring import breaks it.-import { clsx } from 'clsx'; +import clsx from 'clsx';src/components/ui/AlertDialog/fragments/AlertDialogContent.tsx (1)
4-4: Fix broken clsx import.
clsxhas a default export. The named import will be undefined and cause runtime/compile failures.-import { clsx } from 'clsx'; +import clsx from 'clsx';
🧹 Nitpick comments (26)
src/core/primitives/Dialog/fragments/DialogPrimitiveRoot.tsx (1)
45-46: Memoize context value to reduce avoidable re-renders.
contextPropsis recreated every render; consumers will re-render even when values are referentially equal.Apply:
-import React, { forwardRef, useState, useEffect } from 'react'; +import React, { forwardRef, useState, useEffect, useMemo } from 'react'; @@ - const contextProps = { isOpen, handleOpenChange, floaterContext, handleOverlayClick, getReferenceProps, getFloatingProps, getItemProps, refs, floatingStyles }; + const contextProps = useMemo( + () => ({ isOpen, handleOpenChange, floaterContext, handleOverlayClick, getReferenceProps, getFloatingProps, getItemProps, refs, floatingStyles }), + [isOpen, floaterContext, getReferenceProps, getFloatingProps, getItemProps, refs, floatingStyles] + );styles/themes/components/alert-dialog.scss (2)
11-19: Prevent hidden overlay from intercepting clicks and avoid FOUC.When closed, the overlay should not capture pointer events; also set a safe default hidden state to avoid flash before
data-statelands..rad-ui-alert-dialog-overlay { position: fixed; inset: 0; z-index: 50; background-color: rgba(0, 0, 0, 0.8); overflow: hidden; + opacity: 0; + visibility: hidden; + pointer-events: none; transition: opacity 0.2s ease-in-out, visibility 0.2s ease-in-out; &[data-state="open"] { opacity: 1; visibility: visible; + pointer-events: auto; } &[data-state="closed"] { opacity: 0; visibility: hidden; + pointer-events: none; } }
46-60: Set base hidden state for content and add reduced-motion fallback.Avoid layout/visibility jump on first paint; respect users who prefer reduced motion.
.rad-ui-alert-dialog-content { @@ - border: 1px solid var(--rad-ui-color-gray-600); - box-shadow: 0 0 10px 0 rgba(0, 0, 0, 0.4); - transition: opacity 0.2s ease-in-out, visibility 0.2s ease-in-out, transform 0.2s ease-in-out; + border: 1px solid var(--rad-ui-color-gray-600); + box-shadow: 0 0 10px 0 rgba(0, 0, 0, 0.4); + opacity: 0; + visibility: hidden; + transform: translate(-50%, -50%) scale(0.95); + transition: opacity 0.2s ease-in-out, visibility 0.2s ease-in-out, transform 0.2s ease-in-out; @@ &[data-state="closed"] { opacity: 0; visibility: hidden; transform: translate(-50%, -50%) scale(0.95); } + + @media (prefers-reduced-motion: reduce) { + transition: none; + } }src/components/ui/AlertDialog/AlertDialog.tsx (1)
27-30: Console warn on direct usage — consider gating to dev only.Avoid noisy logs in production bundles.
-const AlertDialog = () => { - console.warn('Direct usage of AlertDialog is not supported. Please use AlertDialog.Root, AlertDialog.Content, etc. instead.'); +const AlertDialog = () => { + if (process.env.NODE_ENV !== 'production') { + // eslint-disable-next-line no-console + console.warn('Direct usage of AlertDialog is not supported. Please use AlertDialog.Root, AlertDialog.Content, etc. instead.'); + } return null; };src/components/ui/AlertDialog/fragments/AlertDialogTrigger.tsx (1)
16-37: Exposearia-disabledfor non-buttonasChildcases.When
asChildrenders non-form controls,disabledwon’t be valid; addaria-disabledto keep it accessible.<DialogPrimitive.Trigger ref={ref} className={clsx(`${rootClass}-trigger`, className)} asChild={asChild} disabled={disabled} + aria-disabled={disabled || undefined} data-state={dataState} data-disabled={dataDisabled} {...props} >src/core/primitives/Dialog/fragments/DialogPrimitiveTrigger.tsx (1)
11-11: Disabled prop: verify non-button/asChild semantics.If
asChildrenders a non-button element,disabledwon’t be a valid attribute and keyboard activation (Enter/Space) may still fire. ConfirmButtonPrimitivemapsdisabledtoaria-disabled, removes focusability, and blocks keyboard/pointer events for asChild.If not already handled downstream, consider:
<ButtonPrimitive ... - disabled={disabled} + disabled={disabled} + aria-disabled={disabled || undefined} + data-disabled={disabled ? '' : undefined} + tabIndex={disabled ? -1 : undefined}src/components/ui/AlertDialog/fragments/AlertDialogTitle.tsx (1)
36-37: Guard className prefix when rootClass is empty.Default context sets
rootClass: '', which yields a-titleclass. Join conditionally.- className={`${rootClass}-title ${className}`} + className={`${rootClass ? `${rootClass}-title ` : ''}${className}`.trim()}src/components/ui/AlertDialog/fragments/AlertDialogPortal.tsx (1)
8-12: Avoid duplicate prop declarations; rely on the primitive’s prop type.
container,forceMount, andkeepMountedalready come fromDialogPrimitive.Portal’s props. Redefining them here risks drift and confusion.-export type AlertDialogPortalProps = DialogPrimitivePortalProps & { - container?: Element | null; - forceMount?: boolean; - keepMounted?: boolean; -}; +export type AlertDialogPortalProps = DialogPrimitivePortalProps;src/components/ui/AlertDialog/fragments/AlertDialogDescription.tsx (1)
12-15: Don’t re-declare props already in Primitive.p (optional).If
Primitive.palready exposesclassName/asChild, extending with them is redundant.src/components/ui/AlertDialog/fragments/AlertDialogContent.tsx (1)
10-14: Optional: avoid repeating primitive props in the public type.
asChild/forceMount/classNameshould already flow through fromDialogPrimitive.Content.-export type AlertDialogContentProps = DialogPrimitiveContentProps & { - className?: string; - asChild?: boolean; - forceMount?: boolean; -}; +export type AlertDialogContentProps = DialogPrimitiveContentProps;src/components/ui/AlertDialog/fragments/AlertDialogOverlay.tsx (1)
13-16: Constrain children usage to asChild=true (or warn in dev).Overlay typically shouldn’t host arbitrary children unless it’s operating in slot/asChild mode. Today nothing prevents content with asChild=false.
Minimal dev warning:
}, ref) => { const { rootClass } = useContext(AlertDialogContext); + if (process.env.NODE_ENV !== 'production' && !asChild && children) { + // Children are only meaningful when composing via `asChild` + console.warn('AlertDialogOverlay: `children` are ignored unless `asChild` is true.'); + } return (Type-level alternative (stricter):
-type AlertDialogOverlayProps = DialogPrimitiveOverlayProps & { - className?: string; - asChild?: boolean; - forceMount?: boolean; - children?: React.ReactNode; -}; +type Base = DialogPrimitiveOverlayProps & { className?: string; forceMount?: boolean; }; +type WithChild = Base & { asChild: true; children: React.ReactElement }; +type WithoutChild = Base & { asChild?: false; children?: never }; +type AlertDialogOverlayProps = WithChild | WithoutChild;Also applies to: 21-35
src/components/ui/AlertDialog/fragments/AlertDialogRoot.tsx (2)
49-55: Avoid extra wrapper; forward ref to the actual Root element.The inner div duplicates classes and shifts the ref away from the true root. Prefer attaching ref/className to DialogPrimitive.Root and remove the wrapper.
- <DialogPrimitive.Root open={isOpen} onOpenChange={setIsOpen} className={clsx(rootClass, className)} {...props}> - <AlertDialogContext.Provider value={contextProps}> - <div ref={ref} className={clsx(rootClass, className)}> - {children} - </div> - </AlertDialogContext.Provider> - </DialogPrimitive.Root> + <DialogPrimitive.Root + ref={ref} + open={isOpen} + onOpenChange={setIsOpen} + className={clsx(rootClass, className)} + {...props} + > + <AlertDialogContext.Provider value={contextProps}> + {children} + </AlertDialogContext.Provider> + </DialogPrimitive.Root>
38-46: Memoize context value to reduce avoidable re-renders.Provider gets a fresh object each render; memoizing on its deps is cheap and improves consumer stability.
-import React, { forwardRef, useState } from 'react'; +import React, { forwardRef, useMemo, useState } from 'react'; @@ - const contextProps = { - rootClass, - isOpen, - setIsOpen, - titleId, - descriptionId, - setTitleId, - setDescriptionId - }; + const contextProps = useMemo(() => ({ + rootClass, + isOpen, + setIsOpen, + titleId, + descriptionId, + setTitleId, + setDescriptionId + }), [rootClass, isOpen, titleId, descriptionId]);src/components/ui/AlertDialog/tests/AlertDialog.test.tsx (6)
120-136: Prefer userEvent and assert dialog doesn’t openUse user-level events and ensure UI state stays closed.
- const trigger = screen.getByText('Disabled Trigger'); - fireEvent.click(trigger); + const trigger = screen.getByText('Disabled Trigger'); + await userEvent.click(trigger); // Should not call onOpenChange when disabled expect(onOpenChange).not.toHaveBeenCalled(); + expect(screen.queryByRole('alertdialog')).not.toBeInTheDocument();
255-268: Content forceMount: ensure portal also renders when closedIf Portal gates rendering, force-mount both to avoid false negatives.
- it('should support forceMount prop', () => { + it('should support forceMount prop', () => { render( <AlertDialog.Root> <AlertDialog.Portal> - <AlertDialog.Content forceMount> + <AlertDialog.Content forceMount> <AlertDialog.Title>Title</AlertDialog.Title> </AlertDialog.Content> </AlertDialog.Portal> </AlertDialog.Root> ); expect(screen.getByText('Title')).toBeInTheDocument(); });If Portal guards by open/forceMount, consider:
- <AlertDialog.Portal> + <AlertDialog.Portal forceMount>
287-302: Title: also assert generated id existsMinor robustness.
const title = screen.getByText('Test Title'); expect(title).toBeInTheDocument(); expect(title.tagName).toBe('H2'); + expect(title).toHaveAttribute('id');
322-337: Description: also assert generated id existsMirror the Title check.
const description = screen.getByText('Test Description'); expect(description).toBeInTheDocument(); expect(description.tagName).toBe('P'); + expect(description).toHaveAttribute('id');
389-426: Add keyboard/overlay dismissal coverageConsider Escape key and overlay click to verify modal semantics.
I can supply diffs if you confirm the intended close behaviors (Esc/overlay).
537-555: “Invalid props” test provides little signalPassing undefined to disabled rarely guards real-world misuse. Prefer validating graceful handling of unexpected types on asChild/forceMount/keepMounted.
src/core/primitives/Dialog/fragments/DialogPrimitiveContent.tsx (3)
9-14: Type role narrowly and document ARIA propsConstrain role to known dialog roles and add brief JSDoc for ARIA props.
-export type DialogPrimitiveContentProps = { +export type DialogPrimitiveContentProps = { children: React.ReactNode; className?: string; - asChild?: boolean; + asChild?: boolean; forceMount?: boolean; - role?: string; + role?: 'dialog' | 'alertdialog'; 'aria-modal'?: boolean; 'aria-labelledby'?: string; 'aria-describedby'?: string; }
17-26: asChild prop is unused hereEither implement Slot-based polymorphism or drop the prop at this layer to avoid confusion.
I can draft a Slot-based implementation if you prefer keeping asChild at the primitive.
36-46: Focus management: ensure container is focusableSet tabIndex when role is a dialog to guarantee focus capture on open.
- <div + <div ref={mergedRef} {...getFloatingProps()} role={role} aria-modal={ariaModal} aria-labelledby={ariaLabelledBy} aria-describedby={ariaDescribedBy} data-state={dataState} - {...props} + tabIndex={role ? -1 : undefined} + {...props} >src/components/ui/AlertDialog/stories/AlertDialog.stories.tsx (4)
28-35: Action/Cancel container: consistent spacingUse the same gap utilities as other stories for consistency.
- <div className="flex justify-end"> + <div className="flex justify-end gap-2 mt-4">
78-115: Controlled story: consider exposing args to toggle openSmall UX improvement for Storybook controls.
194-221: ForceMount story: demonstrate closed state stylingConsider showing closed state (data-state="closed") and toggling to illustrate persistence.
223-256: AsChild: show destructive styling via ButtonDemonstrate polymorphism + visual intent.
- <AlertDialog.Trigger asChild> - <Button variant="destructive"> + <AlertDialog.Trigger asChild> + <Button variant="destructive"> Delete Account </Button> </AlertDialog.Trigger>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/components/ui/AlertDialog/AlertDialog.tsx(1 hunks)src/components/ui/AlertDialog/contexts/AlertDialogContext.tsx(1 hunks)src/components/ui/AlertDialog/fragments/AlertDialogContent.tsx(1 hunks)src/components/ui/AlertDialog/fragments/AlertDialogDescription.tsx(1 hunks)src/components/ui/AlertDialog/fragments/AlertDialogOverlay.tsx(1 hunks)src/components/ui/AlertDialog/fragments/AlertDialogPortal.tsx(1 hunks)src/components/ui/AlertDialog/fragments/AlertDialogRoot.tsx(1 hunks)src/components/ui/AlertDialog/fragments/AlertDialogTitle.tsx(2 hunks)src/components/ui/AlertDialog/fragments/AlertDialogTrigger.tsx(1 hunks)src/components/ui/AlertDialog/stories/AlertDialog.stories.tsx(2 hunks)src/components/ui/AlertDialog/tests/AlertDialog.test.tsx(1 hunks)src/core/primitives/Dialog/fragments/DialogPrimitiveContent.tsx(1 hunks)src/core/primitives/Dialog/fragments/DialogPrimitiveOverlay.tsx(1 hunks)src/core/primitives/Dialog/fragments/DialogPrimitivePortal.tsx(1 hunks)src/core/primitives/Dialog/fragments/DialogPrimitiveRoot.tsx(2 hunks)src/core/primitives/Dialog/fragments/DialogPrimitiveTrigger.tsx(2 hunks)styles/themes/components/alert-dialog.scss(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
📚 Learning: 2025-07-18T16:43:26.175Z
Learnt from: GoldGroove06
PR: rad-ui/ui#1215
File: src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveTrigger.tsx:25-27
Timestamp: 2025-07-18T16:43:26.175Z
Learning: In the CheckboxGroupPrimitiveTrigger component (`src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveTrigger.tsx`), the component uses two separate useEffect hooks with different purposes: the first useEffect (lines 25-27) with empty dependency array `[]` is meant to set the initial state only once on mount by syncing with the group's checked values, while the second useEffect (lines 28-34) handles ongoing state updates by propagating local `isChecked` changes back to the group's `checkedValues`. This pattern prevents infinite loops while maintaining proper synchronization.
Applied to files:
src/core/primitives/Dialog/fragments/DialogPrimitiveRoot.tsx
📚 Learning: 2024-11-24T06:43:42.194Z
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
Applied to files:
src/core/primitives/Dialog/fragments/DialogPrimitiveTrigger.tsx
📚 Learning: 2024-12-12T08:34:33.079Z
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
Applied to files:
src/components/ui/AlertDialog/fragments/AlertDialogTrigger.tsx
🧬 Code graph analysis (11)
src/components/ui/AlertDialog/tests/AlertDialog.test.tsx (1)
docs/app/docs/components/alert-dialog/docs/anatomy.tsx (1)
AlertDialog(3-18)
src/components/ui/AlertDialog/fragments/AlertDialogPortal.tsx (2)
src/components/ui/AlertDialog/AlertDialog.tsx (1)
AlertDialogPortalProps(17-17)src/core/primitives/Dialog/fragments/DialogPrimitivePortal.tsx (1)
DialogPrimitivePortalProps(5-10)
src/components/ui/AlertDialog/fragments/AlertDialogTitle.tsx (1)
src/components/ui/AlertDialog/contexts/AlertDialogContext.tsx (1)
AlertDialogContext(14-22)
src/components/ui/AlertDialog/fragments/AlertDialogContent.tsx (1)
src/components/ui/AlertDialog/contexts/AlertDialogContext.tsx (1)
AlertDialogContext(14-22)
src/core/primitives/Dialog/fragments/DialogPrimitiveOverlay.tsx (1)
src/core/primitives/Dialog/context/DialogPrimitiveContext.tsx (1)
DialogPrimitiveContext(19-33)
src/components/ui/AlertDialog/fragments/AlertDialogOverlay.tsx (1)
src/components/ui/AlertDialog/contexts/AlertDialogContext.tsx (1)
AlertDialogContext(14-22)
src/core/primitives/Dialog/fragments/DialogPrimitiveContent.tsx (1)
src/core/primitives/Dialog/context/DialogPrimitiveContext.tsx (1)
DialogPrimitiveContext(19-33)
src/components/ui/AlertDialog/stories/AlertDialog.stories.tsx (1)
docs/app/docs/components/alert-dialog/docs/anatomy.tsx (1)
AlertDialog(3-18)
src/components/ui/AlertDialog/fragments/AlertDialogRoot.tsx (2)
src/core/primitives/Dialog/fragments/DialogPrimitiveRoot.tsx (1)
DialogPrimitiveRootProps(6-12)src/core/hooks/useControllableState/index.tsx (1)
useControllableState(15-78)
src/components/ui/AlertDialog/fragments/AlertDialogTrigger.tsx (1)
src/components/ui/AlertDialog/contexts/AlertDialogContext.tsx (1)
AlertDialogContext(14-22)
src/components/ui/AlertDialog/fragments/AlertDialogDescription.tsx (1)
src/components/ui/AlertDialog/contexts/AlertDialogContext.tsx (1)
AlertDialogContext(14-22)
🪛 GitHub Actions: Build RAD UI
src/components/ui/AlertDialog/AlertDialog.tsx
[error] 25-25: TS2307: Cannot find module './types' or its corresponding type declarations.
🔇 Additional comments (12)
src/core/primitives/Dialog/fragments/DialogPrimitiveRoot.tsx (1)
19-22: Prop/state sync looks good.The effect correctly mirrors external
openinto internalisOpen, preventing drift when the controlled prop changes.src/components/ui/AlertDialog/AlertDialog.tsx (1)
13-25: Consolidate AlertDialog type exports into atypes.tsbarrel
Notypes.tsinsrc/components/ui/AlertDialog. Create one that re-exports all ten types from their source files, then keepexport type { AlertDialogRootProps, AlertDialogTriggerProps, AlertDialogPortalProps, AlertDialogOverlayProps, AlertDialogContentProps, AlertDialogTitleProps, AlertDialogDescriptionProps, AlertDialogContextType, AlertDialogComponentTree, OpenState } from './types';– fragments/AlertDialog*.tsx → Props types
– contexts/AlertDialogContext.tsx → ContextType, ComponentTree, OpenStatesrc/core/primitives/Dialog/fragments/DialogPrimitiveTrigger.tsx (1)
14-20: LGTM: props shape and defaults.Adding
disabledwith a sane default is consistent with the API.src/core/primitives/Dialog/fragments/DialogPrimitiveOverlay.tsx (1)
33-37: LGTM: data-state and children passthrough.
data-stateenables styling andchildrensupport is appropriate.src/components/ui/AlertDialog/fragments/AlertDialogTitle.tsx (1)
3-6: LGTM: ID generation and context wiring approach.Using a stable id and propagating to context aligns with the ARIA pattern.
src/components/ui/AlertDialog/fragments/AlertDialogContent.tsx (1)
25-34: ARIA wiring looks correct.
role="alertdialog",aria-modal, and ID-based labelling/description from context are applied appropriately.src/components/ui/AlertDialog/fragments/AlertDialogOverlay.tsx (1)
27-35: Props passthrough and ref forwarding look solid.Forwarding ref, passing forceMount/asChild through, and class merging are correct post-fix.
src/core/primitives/Dialog/fragments/DialogPrimitivePortal.tsx (1)
35-39: SSR gating and root resolution look good.Mounted check avoids document access during SSR; root selection with theme container fallback is fine.
Also applies to: 62-69
src/components/ui/AlertDialog/fragments/AlertDialogRoot.tsx (1)
34-36: Controllable state wiring LGTM.useControllableState integrates cleanly with open/defaultOpen/onOpenChange.
src/components/ui/AlertDialog/tests/AlertDialog.test.tsx (1)
557-575: ClassName assertions: LGTMGood coverage for polymorphic className pass-through.
src/core/primitives/Dialog/fragments/DialogPrimitiveContent.tsx (1)
27-27: No action needed:floaterContextis declared onDialogPrimitiveContextTypeand passed inDialogPrimitiveRoot’s provider, so it’s already exposed.src/components/ui/AlertDialog/stories/AlertDialog.stories.tsx (1)
46-76: Basic story: looks goodClear anatomy and sensible layout.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/core/primitives/Dialog/fragments/DialogPrimitiveContent.tsx (1)
31-33: Force-mounted but closed content must be hidden from AT and interactionWhen
forceMountis true andisOpenis false, the node stays mounted and currently remains perceivable/interactive. Add anisHiddenflag and reflect it witharia-hidden, inert (if available), and no pointer events to keep it out of the a11y tree and focus order while preservingdata-state="closed".Apply:
- const dataState = isOpen ? 'open' : 'closed'; + const dataState = isOpen ? 'open' : 'closed'; + const isHidden = !isOpen && forceMount;- {...getFloatingProps()} - tabIndex={-1} - style={{ outline: 'none' }} + {...getFloatingProps()} + tabIndex={isHidden ? -1 : -1} + // Hide but keep mounted for transitions/measurement + aria-hidden={isHidden || undefined} + // If you polyfill inert, this will fully remove it from tab/focus nav + {...(isHidden ? ({ inert: '' } as any) : {})} + style={isHidden + ? { visibility: 'hidden', pointerEvents: 'none', outline: 'none' } + : { outline: 'none' }}Also applies to: 84-95, 137-139
🧹 Nitpick comments (15)
src/components/ui/AlertDialog/types.ts (2)
10-10: Consider an ergonomic alias for the context valueMany codebases use “ContextValue” to denote the runtime shape. You can add a non-breaking alias:
export type { AlertDialogContextType } from './contexts/AlertDialogContext'; +// Alias for ergonomics; keep original for backward-compat +export type { AlertDialogContextType as AlertDialogContextValue } from './contexts/AlertDialogContext';
11-11: Avoid a broadly named alias that may collide; rename or drop“OpenState = boolean” is generic, easy to duplicate across primitives, and adds little signal. Prefer a scoped alias or inline boolean.
Option A — scoped alias:
-export type OpenState = boolean; +// Prefer a component-scoped alias to avoid cross-module name collisions +export type AlertDialogOpenState = boolean;Option B — remove the alias and inline boolean where used:
-export type OpenState = boolean; +// Use `boolean` directly at call sites to reduce indirection.src/core/primitives/Dialog/fragments/DialogPrimitiveContent.tsx (3)
72-79: Make focus restore more robustThe previously focused element might be detached by the time you restore. Guard on
isConnectedand clear the ref.- if (!isOpen && previousActiveElement.current) { - const timer = setTimeout(() => { - previousActiveElement.current?.focus(); - }, 0); - return () => clearTimeout(timer); - } + if (!isOpen && previousActiveElement.current) { + const el = previousActiveElement.current; + previousActiveElement.current = null; + const timer = setTimeout(() => { + if ((el as any)?.isConnected) el.focus(); + }, 0); + return () => clearTimeout(timer); + }
9-14: asChild prop is accepted but unused
asChildis declared but the content always renders a div. Either implement the slot/clone pattern or drop the prop to avoid API confusion.If you want
asChild, a minimal approach without external deps:- {shouldRender && ( - <div - ref={mergedRef} + {shouldRender && (asChild && React.isValidElement(children) + ? React.cloneElement(children as React.ReactElement, { + ref: mergedRef, {...getFloatingProps({ ...props, onKeyDown: /* composed as above */ })} - ... - > - {children} - </div> - )} + role, 'aria-modal': ariaModal, 'aria-labelledby': ariaLabelledBy, + 'aria-describedby': ariaDescribedBy, 'data-state': dataState, + }) + : ( + <div + ref={mergedRef} + {...getFloatingProps({ ...props, onKeyDown: /* composed as above */ })} + ... + > + {children} + </div> + ) + )}Happy to wire this fully if you confirm the preferred pattern (custom Slot vs cloneElement).
Also applies to: 19-26, 83-141
48-55: Deduplicate the focusable selectorYou build the same selector twice. Extract a module-level constant to avoid drift and reallocations.
Example:
const FOCUSABLE_SELECTOR = 'button:not([disabled]),[href],input:not([disabled]),select:not([disabled]),textarea:not([disabled]),[tabindex]:not([tabindex="-1"])';Also applies to: 104-111
src/components/ui/AlertDialog/tests/AlertDialog.a11y.test.tsx (10)
38-38: Fix async test function declaration spacing.There's a missing space between
asyncand the parentheses in the test function declaration.- test('Trigger opens dialog; first focusable receives focus; data-state="open" on overlay/content', async() => { + test('Trigger opens dialog; first focusable receives focus; data-state="open" on overlay/content', async () => {
74-74: Fix async test function declaration spacing.Missing space between
asyncand parentheses.- test('Esc and overlay click close dialog; focus returns to trigger; data-state="closed"', async() => { + test('Esc and overlay click close dialog; focus returns to trigger; data-state="closed"', async () => {
125-125: Fix async test function declaration spacing.Missing space between
asyncand parentheses.- test('AXE: no violations when open or closed', async() => { + test('AXE: no violations when open or closed', async () => {
151-151: Fix async test function declaration spacing.Missing space between
asyncand parentheses.- test('Tab/Shift+Tab loop within content; Enter/Space activates focused close button', async() => { + test('Tab/Shift+Tab loop within content; Enter/Space activates focused close button', async () => {
194-194: Fix async test function declaration spacing.Missing space between
asyncand parentheses.- test('asChild on Trigger and Cancel preserves semantics and refs', async() => { + test('asChild on Trigger and Cancel preserves semantics and refs', async () => {
221-221: Fix async test function declaration spacing.Missing space between
asyncand parentheses.- test('controlled vs uncontrolled: open prop syncs and defaultOpen works', async() => { + test('controlled vs uncontrolled: open prop syncs and defaultOpen works', async () => {
264-264: Fix async test function declaration spacing.Missing space between
asyncand parentheses.- test('container prop portals correctly and returns focus', async() => { + test('container prop portals correctly and returns focus', async () => {
8-35: Consider extracting the canvas mock to a test utilities file.The canvas context mock is quite verbose and could be reused across other test files that need similar mocking. Consider moving it to a shared test utilities file.
Create a shared test utilities file:
// src/test-utils/canvas-mock.ts export const setupCanvasMock = () => { Object.defineProperty(HTMLCanvasElement.prototype, 'getContext', { value: jest.fn(() => ({ fillRect: jest.fn(), clearRect: jest.fn(), // ... rest of the mock })) }); };Then use it in the test:
+import { setupCanvasMock } from '@/test-utils/canvas-mock'; + +// Mock canvas for jsdom +setupCanvasMock(); -// Mock canvas for jsdom -Object.defineProperty(HTMLCanvasElement.prototype, 'getContext', { - value: jest.fn(() => ({ - fillRect: jest.fn(), - // ... rest of the mock - })) -});
60-60: Improve null-checking for overlay elements.The current approach of checking
if (overlay)could be improved. Consider asserting that overlay exists when expected or handling the null case more explicitly.- const overlay = document.querySelector('[data-floating-ui-portal] [data-state="open"]') as HTMLElement | null; + const overlay = document.querySelector('[data-floating-ui-portal] [data-state="open"]'); + expect(overlay).toBeTruthy(); // Assert overlay should exist when dialog is open // first focusable should have focus (First button) - wait for focus to change await waitFor(() => { expect(screen.getByText('First')).toHaveFocus(); }); // data-state on overlay/content expect(content).toHaveAttribute('data-state', 'open'); - if (overlay) { - expect(overlay).toHaveAttribute('data-state', 'open'); - } + expect(overlay).toHaveAttribute('data-state', 'open');Also applies to: 69-71, 92-92, 101-104, 120-122
189-191: Consider testing Space key activation as mentioned in test description.The test description mentions "Enter/Space activates focused close button" but only tests Enter key. Consider adding Space key test for completeness.
// Activate Close via keyboard await user.keyboard('{Enter}'); expect(screen.getByText('Open')).toHaveFocus(); + + // Re-open and test Space key + await user.click(screen.getByText('Open')); + await waitFor(() => expect(screen.getByText('First')).toHaveFocus()); + await user.tab(); + await user.tab(); + expect(screen.getByText('Close')).toHaveFocus(); + await user.keyboard(' '); // Space key + expect(screen.getByText('Open')).toHaveFocus();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/components/ui/AlertDialog/AlertDialog.tsx(1 hunks)src/components/ui/AlertDialog/contexts/AlertDialogContext.tsx(1 hunks)src/components/ui/AlertDialog/fragments/AlertDialogOverlay.tsx(1 hunks)src/components/ui/AlertDialog/tests/AlertDialog.a11y.test.tsx(1 hunks)src/components/ui/AlertDialog/types.ts(1 hunks)src/core/primitives/Dialog/fragments/DialogPrimitiveContent.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/ui/AlertDialog/AlertDialog.tsx
- src/components/ui/AlertDialog/fragments/AlertDialogOverlay.tsx
- src/components/ui/AlertDialog/contexts/AlertDialogContext.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-12-12T08:34:33.079Z
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
Applied to files:
src/components/ui/AlertDialog/tests/AlertDialog.a11y.test.tsxsrc/core/primitives/Dialog/fragments/DialogPrimitiveContent.tsx
🔇 Additional comments (4)
src/components/ui/AlertDialog/types.ts (2)
1-9: Solid public type surface via type-only re-exportsClear, explicit, and tree-shakeable. Good centralization of the AlertDialog API.
1-11: Double-check for type cycles and duplicate OpenState aliases across componentsIf any fragment imports OpenState from this file while this file re-exports types from fragments, you could introduce a type-only cycle. Also check if other primitives export an
OpenStatetoo.Run:
#!/bin/bash set -euo pipefail echo "1) Ensure the re-exported types exist at their paths:" for t in AlertDialogRootProps AlertDialogTriggerProps AlertDialogPortalProps AlertDialogOverlayProps AlertDialogContentProps AlertDialogTitleProps AlertDialogDescriptionProps AlertDialogCancelProps AlertDialogActionProps AlertDialogContextType; do rg -nP "(export\s+(type|interface)\s+$t\b|export\s+{\s*$t\s+as\s+)" -C2 --type=ts src/components/ui/AlertDialog || echo "Missing: $t" done echo echo "2) Check for fragments importing from types.ts (potential cycle if they also get re-exported here):" rg -nP "from\s+['\"]\.\./types['\"]|from\s+['\"]\./types['\"]" -C1 --type=ts src/components/ui/AlertDialog/fragments || true echo echo "3) Look for other exported OpenState aliases across the repo:" rg -nP "export\s+type\s+OpenState\s*=\s*boolean" --type=tssrc/core/primitives/Dialog/fragments/DialogPrimitiveContent.tsx (1)
21-24: Default role vs. AlertDialog usageDefaulting to
role="dialog"is fine, but ensure AlertDialog setsrole="alertdialog"at the composition layer for proper semantics.Would you like me to scan the repo for AlertDialog usages to confirm role propagation?
src/components/ui/AlertDialog/tests/AlertDialog.a11y.test.tsx (1)
37-304: Comprehensive test coverage - excellent work!The test suite provides thorough coverage of AlertDialog functionality including:
- Accessibility testing with axe-core
- Focus management and keyboard navigation
- Controlled/uncontrolled state management
- Portal behavior and custom containers
- SSR safety
- asChild prop behavior
- Data attributes for state tracking
The tests are well-structured and cover both happy paths and edge cases.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
src/core/primitives/Dialog/fragments/DialogPrimitivePortal.tsx (3)
40-62: forceMount/keepMounted are no-ops here; simplify and leave a TODO.Both branches return identical markup. Collapse to the single return and annotate the intended future behavior (tracked in #1509).
- // If forceMount is true, always render - if (forceMount) { - return ( - <Floater.Portal - root={rootElementRef.current} - {...props} - > - {children} - </Floater.Portal> - ); - } - - // If keepMounted is true, keep the portal container but children follow their own mounting logic - if (keepMounted) { - return ( - <Floater.Portal - root={rootElementRef.current} - {...props} - > - {children} - </Floater.Portal> - ); - } + // TODO(kotAPI): forceMount/keepMounted have no effect at this level; tracked in #1509.
5-10: Narrow container type to HTMLElement; avoid unsafe casts.Use HTMLElement throughout to match actual Portal roots and eliminate the need for casts.
export type DialogPrimitivePortalProps = { children: React.ReactNode; - container?: Element | null; + container?: HTMLElement | null; forceMount?: boolean; keepMounted?: boolean; };
22-33: Type-safe root resolution (no casts) and stricter checks.Guard by instanceof and type querySelector to HTMLElement.
- if (container) { - rootElementRef.current = container as HTMLElement; - } else { - const themeContainer = document.querySelector('#rad-ui-theme-container') as HTMLElement; - const fallback = document.body; - const selectedRoot = themeContainer || fallback; - rootElementRef.current = selectedRoot; - } + if (container instanceof HTMLElement) { + rootElementRef.current = container; + } else { + const themeContainer = document.querySelector<HTMLElement>('#rad-ui-theme-container'); + rootElementRef.current = themeContainer ?? document.body; + }src/components/ui/AlertDialog/fragments/AlertDialogPortal.tsx (2)
13-16: Forward the ref and align its type to the underlying Portal.Restores ref support and keeps types consistent with the primitive.
-const AlertDialogPortal = ({ - children, - ...props -}: AlertDialogPortalProps) => { +const AlertDialogPortal = React.forwardRef<React.ElementRef<typeof DialogPrimitive.Portal>, AlertDialogPortalProps>(( + { children, ...props }, ref +) => { @@ -}; +});To check if any callers depend on a ref:
#!/bin/bash rg -nP -C2 '\bAlertDialogPortal\b' --type tsx rg -nP -C2 '<AlertDialogPortal[^>]*\bref=' --type tsxAlso applies to: 24-24
18-20: Pass the forwarded ref to the primitive.- <DialogPrimitive.Portal - {...props} - > + <DialogPrimitive.Portal ref={ref} {...props}>src/components/ui/AlertDialog/fragments/AlertDialogTitle.tsx (1)
26-41: Simplify effect and unmount cleanup; current logic can clobber othersIncluding
currentTitleIdin deps makes the cleanup run on context changes (not just unmount), which can clear another instance’s ID. Drop the ref/compare and clear only on unmount.Apply:
- const titleIdRef = useRef(titleId); - - useEffect(() => { - titleIdRef.current = titleId; - if (titleId) { - setTitleId(titleId); - } - - // Cleanup: clear the titleId when this component unmounts - // Only clear if the stored id still matches to avoid clobbering other instances - return () => { - if (currentTitleId === titleIdRef.current) { - setTitleId(undefined); - } - }; - }, [titleId, setTitleId, currentTitleId]); + useEffect(() => { + if (titleId) setTitleId(titleId); + return () => setTitleId(undefined); + }, [titleId, setTitleId]);
🧹 Nitpick comments (3)
src/components/ui/AlertDialog/fragments/AlertDialogPortal.tsx (1)
7-11: Avoid re-declaring props already provided by the primitive.Rely on the primitive’s props to prevent drift and duplication.
-export type AlertDialogPortalProps = DialogPrimitivePortalProps & { - container?: Element | null; - forceMount?: boolean; - keepMounted?: boolean; -}; +export type AlertDialogPortalProps = DialogPrimitivePortalProps;src/components/ui/AlertDialog/fragments/AlertDialogTitle.tsx (2)
3-3: Remove unused import after ref cleanupIf you apply the effect simplification,
useRefbecomes unused.-import React, { forwardRef, useContext, useEffect, useRef } from 'react'; +import React, { forwardRef, useContext, useEffect } from 'react';
47-47: Avoid producing “-title” when rootClass is emptyGuard the prefix or use an array join to prevent a dangling hyphen.
- className={`${rootClass}-title ${className}`} + className={[rootClass && `${rootClass}-title`, className].filter(Boolean).join(' ')}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/components/ui/AlertDialog/contexts/AlertDialogContext.tsx(1 hunks)src/components/ui/AlertDialog/fragments/AlertDialogDescription.tsx(1 hunks)src/components/ui/AlertDialog/fragments/AlertDialogPortal.tsx(1 hunks)src/components/ui/AlertDialog/fragments/AlertDialogTitle.tsx(2 hunks)src/components/ui/AlertDialog/tests/AlertDialog.a11y.test.tsx(1 hunks)src/components/ui/AlertDialog/tests/AlertDialog.test.tsx(1 hunks)src/core/primitives/Dialog/fragments/DialogPrimitiveContent.tsx(1 hunks)src/core/primitives/Dialog/fragments/DialogPrimitivePortal.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/core/primitives/Dialog/fragments/DialogPrimitiveContent.tsx
- src/components/ui/AlertDialog/tests/AlertDialog.test.tsx
- src/components/ui/AlertDialog/tests/AlertDialog.a11y.test.tsx
- src/components/ui/AlertDialog/contexts/AlertDialogContext.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
src/components/ui/AlertDialog/fragments/AlertDialogDescription.tsx (1)
src/components/ui/AlertDialog/contexts/AlertDialogContext.tsx (1)
AlertDialogContext(14-22)
src/components/ui/AlertDialog/fragments/AlertDialogPortal.tsx (1)
src/core/primitives/Dialog/fragments/DialogPrimitivePortal.tsx (1)
DialogPrimitivePortalProps(5-10)
src/components/ui/AlertDialog/fragments/AlertDialogTitle.tsx (1)
src/components/ui/AlertDialog/contexts/AlertDialogContext.tsx (1)
AlertDialogContext(14-22)
🪛 Biome (2.1.2)
src/components/ui/AlertDialog/fragments/AlertDialogDescription.tsx
[error] 25-25: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
src/components/ui/AlertDialog/fragments/AlertDialogTitle.tsx
[error] 25-25: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🔇 Additional comments (5)
src/core/primitives/Dialog/fragments/DialogPrimitivePortal.tsx (2)
12-18: Prop forwarding looks good.Destructure + spread preserves future Portal props; keep it.
Also applies to: 67-67
33-33: Effect deps are correct.Recomputing root on container change is appropriate.
src/components/ui/AlertDialog/fragments/AlertDialogPortal.tsx (1)
2-2: No issues here.src/components/ui/AlertDialog/fragments/AlertDialogDescription.tsx (1)
12-15: Props/render surface looks solid.asChild passthrough, id override support, and DOM id wiring into Primitive.p read cleanly.
Also applies to: 43-53
src/components/ui/AlertDialog/fragments/AlertDialogTitle.tsx (1)
21-22: Good: consumeridoverrides generated idHonors consumer control and SSR/anchor compatibility. Keep this behavior.
Summary by CodeRabbit
New Features
Accessibility
Style
Documentation
Tests