test: add portal test utilities #1540
Merged
+117 −1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
WalkthroughAdds a new portal testing utility module and updates Dialog, DropdownMenu, and Tooltip tests to render via a shared portal root, assert focus-trap/return, and verify scroll lock behavior. Production code is untouched; changes are confined to tests and test utilities. Changes
Sequence Diagram(s)sequenceDiagram autonumber participant T as Test participant PU as Portal Utils participant R as RTL Renderer participant DOM as document.body participant C as Component (Dialog/Menu/Tooltip) T->>PU: renderWithPortal(<Component/>) PU->>DOM: create portalRoot (#rad-ui-theme-container) PU->>R: render(<Component/>) targeting portalRoot R-->>T: {container, portalRoot, cleanup} T->>C: open via Trigger (click/focus) T->>PU: assertScrollLock()/assertFocusTrap() alt Close via UI/Escape T->>C: close (click/Escape) T->>PU: assertFocusReturn(Trigger) T->>PU: assertScrollUnlock() end T->>PU: cleanup() PU->>DOM: unmount and remove portalRoot Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews: pre_merge_checks: custom_checks: - name: "Undocumented Breaking Changes" mode: "warning" instructions: | Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/test-utils/portal.ts (3)
4-17: Guard against duplicate portal root and add type importAvoid removing a pre-existing app/root container and ensure type safety.
+import type { ReactElement } from 'react'; -export function renderWithPortal(ui: React.ReactElement) { - const portalRoot = document.createElement('div'); - portalRoot.setAttribute('id', 'rad-ui-theme-container'); - document.body.appendChild(portalRoot); +export function renderWithPortal(ui: ReactElement, id = 'rad-ui-theme-container') { + let portalRoot = document.getElementById(id) as HTMLDivElement | null; + const created = !portalRoot; + if (!portalRoot) { + portalRoot = document.createElement('div'); + portalRoot.setAttribute('id', id); + document.body.appendChild(portalRoot); + } const result = render(ui); return { ...result, portalRoot, cleanup: () => { result.unmount(); - portalRoot.remove(); + if (created) portalRoot!.remove(); } }; }
12-16: Ensure cleanup runs even if assertions throwProvide a helper that guarantees cleanup with try/finally; makes tests harder to leak DOM.
You can add alongside existing exports:
export async function withPortal( ui: ReactElement, fn: (ctx: ReturnType<typeof renderWithPortal>) => Promise<void> | void ) { const ctx = renderWithPortal(ui); try { await fn(ctx); } finally { ctx.cleanup(); } }
19-39: Broaden focusable selector and skip disabled/hidden elementsPrevents false positives from disabled elements and covers contenteditable; also fail fast if container is detached.
export async function assertFocusTrap(container: HTMLElement) { const user = userEvent.setup(); + if (!container.isConnected) { + throw new Error('Container must be attached to document'); + } const focusable = Array.from( container.querySelectorAll<HTMLElement>( - 'button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])' + 'button:not([disabled]), [href], input:not([disabled]), select:not([disabled]), textarea:not([disabled]), [contenteditable]:not([contenteditable="false"]), [tabindex]:not([tabindex="-1"]):not([disabled])' ) );src/components/ui/Tooltip/tests/Tooltip.behavior.test.tsx (1)
100-115: Actually assert portal mounting and always cleanupStrengthen the test to verify content is rendered inside the portal root and ensure cleanup runs even on failure.
- test('portal renders tooltip content and focus is restored on escape', async () => { - const user = userEvent.setup(); - const { getByText, cleanup } = renderWithPortal( + test('portal renders tooltip content and focus is restored on escape', async () => { + const user = userEvent.setup(); + const { getByText, cleanup, portalRoot } = renderWithPortal( <Tooltip.Root> <Tooltip.Trigger>Tip</Tooltip.Trigger> <Tooltip.Content>content</Tooltip.Content> </Tooltip.Root> ); - - const trigger = getByText('Tip'); - trigger.focus(); - await screen.findByRole('tooltip'); - await user.keyboard('{Escape}'); - assertFocusReturn(trigger); - cleanup(); + try { + const trigger = getByText('Tip'); + trigger.focus(); + const tip = await screen.findByRole('tooltip'); + expect(portalRoot).toContainElement(tip); + await user.keyboard('{Escape}'); + assertFocusReturn(trigger); + } finally { + cleanup(); + } });src/components/ui/Dialog/tests/Dialog.test.tsx (1)
91-112: Narrow queries to the portal and guarantee cleanupTargeting elements within the portal avoids accidental matches; try/finally ensures no leaks.
- test('mounts in portal, traps focus, returns focus and locks scroll', async () => { + test('mounts in portal, traps focus, returns focus and locks scroll', async () => { const user = userEvent.setup(); - const { getByText, portalRoot, cleanup } = renderWithPortal( + const { getByText, portalRoot, cleanup } = renderWithPortal( <Dialog.Root> <Dialog.Trigger>Trigger</Dialog.Trigger> <Dialog.Portal> <Dialog.Overlay /> <Dialog.Content> <Dialog.Close>Close</Dialog.Close> </Dialog.Content> </Dialog.Portal> </Dialog.Root> ); - - await user.click(getByText('Trigger')); - await waitFor(() => assertScrollLock()); - await assertFocusTrap(portalRoot); - await user.click(getByText('Close')); - assertFocusReturn(getByText('Trigger')); - await waitFor(() => assertScrollUnlock()); - cleanup(); + try { + await user.click(getByText('Trigger')); + await waitFor(() => assertScrollLock()); + await assertFocusTrap(portalRoot); + await user.click(getByText('Close')); + assertFocusReturn(getByText('Trigger')); + await waitFor(() => assertScrollUnlock()); + } finally { + cleanup(); + } });src/components/ui/DropdownMenu/tests/DropdownMenu.test.tsx (1)
73-91: Add try/finally and assert portal containmentStrengthens the portal assertion and avoids leaking the portal root on failures.
- it('renders in portal and returns focus when closed', async () => { + it('renders in portal and returns focus when closed', async () => { const user = userEvent.setup(); - const { getByText, portalRoot, cleanup } = renderWithPortal( + const { getByText, portalRoot, cleanup } = renderWithPortal( <DropdownMenu.Root> <DropdownMenu.Trigger>Menu</DropdownMenu.Trigger> <DropdownMenu.Portal> <DropdownMenu.Content> <DropdownMenu.Item label="Profile">Profile</DropdownMenu.Item> </DropdownMenu.Content> </DropdownMenu.Portal> </DropdownMenu.Root> ); - - await user.click(getByText('Menu')); - expect(portalRoot).toContainElement(getByText('Profile')); - await user.keyboard('{Escape}'); - assertFocusReturn(getByText('Menu')); - cleanup(); + try { + await user.click(getByText('Menu')); + expect(portalRoot).toContainElement(getByText('Profile')); + await user.keyboard('{Escape}'); + assertFocusReturn(getByText('Menu')); + } finally { + cleanup(); + } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/ui/Dialog/tests/Dialog.test.tsx(2 hunks)src/components/ui/DropdownMenu/tests/DropdownMenu.test.tsx(2 hunks)src/components/ui/Tooltip/tests/Tooltip.behavior.test.tsx(2 hunks)src/test-utils/portal.ts(1 hunks)
🧰 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/DropdownMenu/tests/DropdownMenu.test.tsx
🧬 Code graph analysis (3)
src/components/ui/Tooltip/tests/Tooltip.behavior.test.tsx (1)
src/test-utils/portal.ts (2)
renderWithPortal(4-17)assertFocusReturn(41-43)
src/components/ui/DropdownMenu/tests/DropdownMenu.test.tsx (1)
src/test-utils/portal.ts (2)
renderWithPortal(4-17)assertFocusReturn(41-43)
src/components/ui/Dialog/tests/Dialog.test.tsx (1)
src/test-utils/portal.ts (5)
renderWithPortal(4-17)assertScrollLock(45-47)assertFocusTrap(19-39)assertFocusReturn(41-43)assertScrollUnlock(49-51)
⏰ 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). (2)
- GitHub Check: build
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
src/test-utils/portal.ts (1)
45-51: Scroll-lock checks look goodAssertions match the project convention (data-scroll-locked='1' on lock, removed on unlock).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
Summary
Testing
npm testSummary by CodeRabbit