test(dialog): cover portal container, focus, and hydration #1528
Conversation
WalkthroughAdds Dialog portal test suite; updates DialogPrimitiveContent to render via Primitive.div with asChild support; adds data-state attribute to DialogPrimitiveRoot reflecting isOpen. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Trigger as Dialog.Trigger
participant Root as DialogPrimitiveRoot
participant Portal as Dialog.Portal
participant Overlay as Overlay
participant Content as Content (Primitive.div)
User->>Trigger: Click
Trigger->>Root: set isOpen = true
Note over Root: data-state="open"
Root->>Portal: Render into container
Portal->>Overlay: Mount
Portal->>Content: Mount and focus first element
User->>Content: Press Escape / click Overlay
Content-->>Root: requestClose
Root->>Trigger: restore focus
Root->>Portal: Unmount children
Note over Root: data-state="closed"
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
✨ 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: 3
♻️ Duplicate comments (1)
src/components/ui/Dialog/tests/Dialog.portal.test.tsx (1)
145-147: Good fix: hydrateRoot is unmounted and container removed.This resolves the prior leak/scroll-lock issue flagged in earlier reviews. Thanks for closing the loop.
🧹 Nitpick comments (7)
src/core/primitives/Dialog/fragments/DialogPrimitiveRoot.tsx (1)
11-11: Minor TS style: add spacing after colon.Purely cosmetic for consistency with the rest of the codebase.
- className?:string + className?: stringsrc/core/primitives/Dialog/fragments/DialogPrimitiveContent.tsx (3)
99-99: Broaden event generic to support asChild.When
asChildrenders non-divs (e.g., form),React.KeyboardEvent<HTMLDivElement>is too narrow. UseHTMLElement.- ? (e: React.KeyboardEvent<HTMLDivElement>) => { + ? (e: React.KeyboardEvent<HTMLElement>) => {
30-31: Loosen ref element type for asChild.
forwardRef<HTMLDivElement>anduseRef<HTMLDivElement>conflict with polymorphicasChild. PreferHTMLElement.-const DialogPrimitiveContent = forwardRef<HTMLDivElement, DialogPrimitiveContentProps>(( +const DialogPrimitiveContent = forwardRef<HTMLElement, DialogPrimitiveContentProps>(( @@ - const contentRef = useRef<HTMLDivElement | null>(null); + const contentRef = useRef<HTMLElement | null>(null);Also applies to: 18-18
61-66: tabIndex removal via DOM mutation is brittle.React will re-apply
tabIndex={-1}on re-render, negatingremoveAttribute. Drive it from state and props instead.+ const [hasFocusableChild, setHasFocusableChild] = React.useState(false); @@ - const firstFocusable = root.querySelector<HTMLElement>(focusableSelector); - if (firstFocusable) { - firstFocusable.focus(); - // Remove tabIndex from content div when there are focusable children - root.removeAttribute('tabindex'); - } else { + const firstFocusable = root.querySelector<HTMLElement>(focusableSelector); + if (firstFocusable) { + setHasFocusableChild(true); + firstFocusable.focus(); + } else { + setHasFocusableChild(false); (root as HTMLElement).focus({ preventScroll: true }); } @@ - tabIndex={-1} + tabIndex={hasFocusableChild ? undefined : -1}Also applies to: 89-89
src/components/ui/Dialog/tests/Dialog.portal.test.tsx (3)
60-86: Clean up the custom portal container after the test.
portalRootis appended todocument.bodybut never removed. Remove it post-assertions to avoid cross-test leakage.test('portal container mounts elements and preserves layering', async () => { @@ expect(relation & Node.DOCUMENT_POSITION_FOLLOWING).toBeTruthy(); + portalRoot.remove(); });
152-171: Type casts in asChild refs hint at missing polymorphic typing.The casts work for tests, but consider making Trigger/Content polymorphic so ref types follow the rendered element, avoiding
unknown asin consumers and tests.Would you like a follow-up PR to add a polymorphic
asChildtype pattern (e.g., generic props + ElementType) to the primitives?
88-103: axe usage is fine; consider jest-axe for nicer DX.Not required, but
jest-axeprovides matchers liketoHaveNoViolations()and async helpers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/ui/Dialog/tests/Dialog.portal.test.tsx(1 hunks)src/core/primitives/Dialog/fragments/DialogPrimitiveContent.tsx(4 hunks)src/core/primitives/Dialog/fragments/DialogPrimitiveRoot.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/ui/Dialog/tests/Dialog.portal.test.tsx (1)
docs/app/docs/components/dialog/docs/anatomy.tsx (1)
Dialog(3-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
| <Primitive.div | ||
| ref={mergedRef} | ||
| asChild={asChild} | ||
| {...getFloatingProps()} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t let consumer onKeyDown override escape/Tab trap; compose handlers.
Currently {...props} after your onKeyDown lets consumers replace core logic. Compose user and internal handlers via getFloatingProps({ onKeyDown }), and avoid passing onKeyDown again in {...props}.
- return (
+ // Extract user onKeyDown to compose safely
+ const { onKeyDown: userOnKeyDown, ...restProps } = props as React.HTMLAttributes<HTMLElement>;
+ return (
<>
{shouldRender && (
<Primitive.div
ref={mergedRef}
asChild={asChild}
- {...getFloatingProps()}
+ {...getFloatingProps({
+ onKeyDown: isOpen
+ ? (e: React.KeyboardEvent<HTMLElement>) => {
+ userOnKeyDown?.(e);
+ if (e.defaultPrevented) return;
+ // escape + tab trap
+ if (e.key === 'Escape') {
+ e.preventDefault();
+ e.stopPropagation();
+ handleOpenChange(false);
+ return;
+ }
+ if (e.key === 'Tab') {
+ const focusableSelector = [
+ 'button:not([disabled])',
+ '[href]',
+ 'input:not([disabled])',
+ 'select:not([disabled])',
+ 'textarea:not([disabled])',
+ '[tabindex]:not([tabindex="-1"])'
+ ].join(',');
+ const focusableElements = Array.from(
+ contentRef.current?.querySelectorAll(focusableSelector) || []
+ ) as HTMLElement[];
+ if (focusableElements.length === 0) return;
+ const firstElement = focusableElements[0];
+ const lastElement = focusableElements[focusableElements.length - 1];
+ if (e.shiftKey) {
+ if (document.activeElement === firstElement) {
+ e.preventDefault();
+ lastElement.focus();
+ }
+ } else {
+ if (document.activeElement === lastElement) {
+ e.preventDefault();
+ firstElement.focus();
+ }
+ }
+ }
+ }
+ : undefined
+ })}
tabIndex={-1}
- style={{ outline: 'none' }}
role={role}
aria-modal={isOpen ? ariaModal : undefined}
aria-labelledby={isOpen ? ariaLabelledBy : undefined}
aria-describedby={isOpen ? ariaDescribedBy : undefined}
aria-hidden={!isOpen ? 'true' : undefined}
data-state={dataState}
- onKeyDown={
- isOpen
- ? (e: React.KeyboardEvent<HTMLDivElement>) => {
- // Handle escape key
- if (e.key === 'Escape') {
- e.preventDefault();
- e.stopPropagation();
- handleOpenChange(false);
- }
- // Handle tab trapping
- if (e.key === 'Tab') {
- const focusableSelector = [
- 'button:not([disabled])',
- '[href]',
- 'input:not([disabled])',
- 'select:not([disabled])',
- 'textarea:not([disabled])',
- '[tabindex]:not([tabindex="-1"])'
- ].join(',');
- const focusableElements = Array.from(
- contentRef.current?.querySelectorAll(focusableSelector) || []
- ) as HTMLElement[];
- if (focusableElements.length === 0) return;
- const firstElement = focusableElements[0];
- const lastElement = focusableElements[focusableElements.length - 1];
- if (e.shiftKey) {
- // Shift + Tab
- if (document.activeElement === firstElement) {
- e.preventDefault();
- lastElement.focus();
- }
- } else {
- // Tab
- if (document.activeElement === lastElement) {
- e.preventDefault();
- firstElement.focus();
- }
- }
- }
- }
- : undefined
- }
- {...props}
+ {...restProps}
>Also applies to: 97-145
🤖 Prompt for AI Agents
In src/core/primitives/Dialog/fragments/DialogPrimitiveContent.tsx around lines
85-88 (and likewise apply to 97-145), the current JSX spreads {...props} after
you set an internal onKeyDown which allows a consumer-supplied onKeyDown to
replace your escape/Tab trap; compose handlers instead. Create an internal
onKeyDown that implements the escape/Tab logic and then call getFloatingProps({
onKeyDown: internalOnKeyDown }) so getFloatingProps composes handlers; when
spreading props, exclude props.onKeyDown (or explicitly merge userOnKeyDown by
invoking it from internalOnKeyDown) so consumers cannot override the core
behavior. Ensure the same composition and exclusion of onKeyDown is applied to
the other block at lines 97-145.
| asChild={asChild} | ||
| {...getFloatingProps()} | ||
| tabIndex={-1} | ||
| style={{ outline: 'none' }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid removing focus outline.
style={{ outline: 'none' }} removes visible focus when the content itself is focused (no focusable children) — an a11y regression.
- style={{ outline: 'none' }}
+ // Keep default outline for accessibility; customize via CSS if neededCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/core/primitives/Dialog/fragments/DialogPrimitiveContent.tsx around line
90, an inline style of style={{ outline: 'none' }} was added which removes the
visible focus outline and causes an accessibility regression when the content
itself is focused; remove this inline style and instead ensure focus visuals are
preserved by using a CSS approach such as leaving the default outline, or
applying a rule that only hides outline for non-keyboard interactions (e.g. use
:focus:not(:focus-visible){outline:none} or a focus-visible utility class) so
keyboard users still see a focus ring while mouse users aren’t shown an unwanted
outline.
| <div | ||
| ref={ref} | ||
| data-state={isOpen ? 'open' : 'closed'} | ||
| {...props} | ||
| className={className} | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent consumers from overriding the computed data-state.
Because {...props} comes after data-state, a consumer can unintentionally shadow the controlled attribute. Move {...props} before data-state so your state wins.
<div
ref={ref}
- data-state={isOpen ? 'open' : 'closed'}
- {...props}
+ {...props}
+ data-state={isOpen ? 'open' : 'closed'}
className={className}
>📝 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.
| <div | |
| ref={ref} | |
| data-state={isOpen ? 'open' : 'closed'} | |
| {...props} | |
| className={className} | |
| > | |
| <div | |
| ref={ref} | |
| {...props} | |
| data-state={isOpen ? 'open' : 'closed'} | |
| className={className} | |
| > |
🤖 Prompt for AI Agents
In src/core/primitives/Dialog/fragments/DialogPrimitiveRoot.tsx around lines 48
to 53, the JSX spreads {...props} after setting data-state which allows consumer
props to overwrite the computed data-state; move the {...props} spread to before
the data-state attribute (e.g., place {...props} prior to data-state={isOpen ?
'open' : 'closed'}) so the component-controlled data-state cannot be shadowed by
user-provided props.
Summary
data-stateasChildon dialog contentTesting
npm testSummary by CodeRabbit
New Features
Refactor
Tests