feat(dx): standardize portal container/forceMount APIs across overlay primitives#1935
Open
feat(dx): standardize portal container/forceMount APIs across overlay primitives#1935
Conversation
🦋 Changeset detectedLatest commit: 3965973 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
📝 WalkthroughWalkthroughStandardizes overlay portal APIs across menu, combobox, and hover card primitives by adding ChangesPortal API Standardization
Sequence DiagramsequenceDiagram
actor User
participant Portal as Portal Component
participant Context as Portal Context<br/>(forceMount)
participant Content as Content Component
participant DOM
User->>Portal: Render with forceMount=true
Portal->>Portal: Resolve root element
Portal->>Context: Provider with { forceMount: true }
rect rgba(100, 200, 150, 0.5)
Note over Content: Menu closed (isOpen=false)
Content->>Context: Read forceMount
Content->>Content: shouldRender = isOpen || forceMount || portalForceMount
Content->>Content: shouldRender = true (forceMount=true)
end
rect rgba(150, 150, 200, 0.5)
Content->>DOM: Render with visibility:hidden<br/>pointerEvents:none<br/>data-state="closed"
end
rect rgba(100, 200, 150, 0.5)
Note over Content: Menu opens (isOpen=true)
Content->>Content: Render with visibility:visible<br/>pointerEvents:auto<br/>data-state="open"
end
User->>DOM: Content now interactive
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/ui/DropdownMenu/tests/DropdownMenu.test.tsx`:
- Around line 93-111: The test 'supports a custom portal container' creates a
div and appends it to document.body (variable container) but never removes it;
wrap the test's interaction/assertion block in a try/finally (or ensure cleanup)
and in the finally call document.body.removeChild(container) (or
container.remove()) so the portal container is removed after the test; update
the test around DropdownMenu.Root / DropdownMenu.Portal usage to ensure
container cleanup even if assertions fail.
In `@src/components/ui/HoverCard/fragments/HoverCardPortal.tsx`:
- Around line 47-50: HoverCardPortal is wrapped with forwardRef but never
forwards that ref to the rendered Floater.Portal; update the component so the
forwarded ref (usually named ref or forwardedRef) is attached to Floater.Portal
(e.g., pass ref={ref} or ref={forwardedRef} along with {...props}) so consumers
can access the portal DOM node; ensure the component signature uses
React.forwardRef((props, ref) => ...) and that the ref is passed through to
Floater.Portal.
- Around line 41-43: The early return in HoverCardPortal (the if (!isOpen &&
!forceMount) return null) prevents the portal context provider from mounting so
children like <HoverCard.Content forceMount /> can't read portalForceMount;
remove that short-circuit and always render the portal context provider in
HoverCardPortal (so the provider is mounted even when closed), but keep the
actual DOM/content conditional (use isOpen or forceMount to decide whether to
render the visible portal nodes) so you still avoid DOM output when fully
closed—ensure you reference/maintain the existing isOpen, forceMount and
portalForceMount variables/props and only move the null-return logic inside the
provider's rendered content.
In `@src/components/ui/HoverCard/tests/HoverCard.test.tsx`:
- Around line 106-121: The test creates and appends a DOM node (const container
= document.createElement('div'); document.body.appendChild(container)) for
HoverCard.Portal but never removes it, risking DOM leakage; update the test
around HoverCard.Root / HoverCard.Portal to remove the appended container after
the assertions (e.g., call container.remove() or
document.body.removeChild(container) in a finally block or use the test
framework's cleanup/afterEach) so the created container is cleaned up after the
test completes.
In `@src/core/primitives/Combobox/fragments/ComboboxPrimitiveContent.tsx`:
- Around line 93-107: The current conditional renders different wrapper trees
for isOpen (FocusManager -> FloatingList -> content) vs closed (FloatingList ->
content), causing remounts; change the render so the wrapper tree stays
identical and only the focus-management behavior toggles: always render
Floater.FocusManager as the parent of Floater.FloatingList (preserving
elementsRef, labelsRef and content order) and control focus behavior via a prop
or flag on Floater.FocusManager (or pass floatingContext/no-op context when
closed) so the subtree containing content is not replaced when isOpen changes.
In `@src/core/primitives/Combobox/fragments/ComboboxPrimitivePortal.tsx`:
- Around line 26-31: The fallback order for resolvedRoot can pick a global
document portal before the current theme's container; update the resolution so
themeContext?.containerRef.current is checked immediately after
themeContext?.portalRootRef.current (i.e., use container ?? root ??
themeContext?.portalRootRef.current ?? themeContext?.containerRef.current ??
document.querySelector('[data-rad-ui-portal-root]') ??
document.querySelector('#rad-ui-theme-container')), ensuring the combobox mounts
into the active theme container when themeContext exists but
portalRootRef.current is null.
In `@src/core/primitives/Menu/fragments/MenuPrimitiveContent.tsx`:
- Around line 54-71: The wrapper hierarchy must be identical whether open or
closed: always render Floater.FloatingList with elementsRef/labelsRef and always
render Floater.FocusManager inside it (use the existing floatingContext, modal,
initialFocus and returnFocus props), and move the isOpen conditional inside the
FocusManager to hide/disable the inner content (e.g. apply aria-hidden or a
hidden/style prop or pass a disabled flag) instead of swapping the FocusManager
in/out; update MenuPrimitiveContent to render the same Floater.FloatingList ->
Floater.FocusManager -> {content} tree for both branches so forceMount actually
keeps the subtree mounted.
In `@src/core/primitives/Menu/fragments/MenuPrimitivePortal.tsx`:
- Around line 25-30: The fallback order in MenuPrimitivePortal.tsx can attach
portals to a global selector before the current theme's container; update the
resolution of resolvedRoot so that it prefers container, then root, then
themeContext?.containerRef.current, then themeContext?.portalRootRef.current,
and only after those check document.querySelector('[data-rad-ui-portal-root]')
and document.querySelector('#rad-ui-theme-container'); specifically move
themeContext?.containerRef.current ahead of the global query to ensure menus
inside a theme attach to that theme's container.
In `@src/core/primitives/Menu/tests/MenuPrimitive.test.tsx`:
- Around line 488-500: The test appends a DOM container div (variable container)
to document.body but never removes it; modify the test (or add an afterEach) so
the appended container is removed after the test completes to maintain DOM
isolation — specifically, ensure the container created in MenuPrimitive.test
(the const container = document.createElement('div') used with
MenuPrimitive.Portal) is cleaned up by calling container.remove() (or
document.body.removeChild(container)) at the end of the test or in a teardown
hook so no residual nodes remain between tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d01e1c39-ce6a-4ef8-a542-5ce5e1d6a69e
📒 Files selected for processing (20)
.changeset/fresh-portals-align.mdsrc/components/ui/Combobox/fragments/ComboboxPortal.tsxsrc/components/ui/Combobox/tests/Combobox.full.test.tsxsrc/components/ui/ContextMenu/stories/ContextMenu.stories.tsxsrc/components/ui/DropdownMenu/stories/DropdownMenu.stories.tsxsrc/components/ui/DropdownMenu/tests/DropdownMenu.test.tsxsrc/components/ui/HoverCard/contexts/HoverCardPortalContext.tsxsrc/components/ui/HoverCard/fragments/HoverCardContent.tsxsrc/components/ui/HoverCard/fragments/HoverCardPortal.tsxsrc/components/ui/HoverCard/stories/HoverCard.stories.tsxsrc/components/ui/HoverCard/tests/HoverCard.test.tsxsrc/components/ui/Menubar/stories/Menubar.stories.tsxsrc/components/ui/Select/fragments/SelectPortal.tsxsrc/core/primitives/Combobox/contexts/ComboboxPrimitivePortalContext.tsxsrc/core/primitives/Combobox/fragments/ComboboxPrimitiveContent.tsxsrc/core/primitives/Combobox/fragments/ComboboxPrimitivePortal.tsxsrc/core/primitives/Menu/contexts/MenuPrimitivePortalContext.tsxsrc/core/primitives/Menu/fragments/MenuPrimitiveContent.tsxsrc/core/primitives/Menu/fragments/MenuPrimitivePortal.tsxsrc/core/primitives/Menu/tests/MenuPrimitive.test.tsx
Comment on lines
+93
to
+111
| it('supports a custom portal container', async() => { | ||
| const user = userEvent.setup(); | ||
| const container = document.createElement('div'); | ||
| document.body.appendChild(container); | ||
|
|
||
| render( | ||
| <DropdownMenu.Root> | ||
| <DropdownMenu.Trigger>Menu</DropdownMenu.Trigger> | ||
| <DropdownMenu.Portal container={container}> | ||
| <DropdownMenu.Content> | ||
| <DropdownMenu.Item label="Profile">Profile</DropdownMenu.Item> | ||
| </DropdownMenu.Content> | ||
| </DropdownMenu.Portal> | ||
| </DropdownMenu.Root> | ||
| ); | ||
|
|
||
| await user.click(screen.getByText('Menu')); | ||
| expect(container).toContainElement(screen.getByText('Profile')); | ||
| }); |
Contributor
There was a problem hiding this comment.
Remove the manually appended portal container after the test.
The div added to document.body on Line 96 is never removed, so this test leaves global DOM behind for later cases in the file. Wrapping the body of the test in try/finally is enough here.
Suggested cleanup
it('supports a custom portal container', async() => {
const user = userEvent.setup();
const container = document.createElement('div');
document.body.appendChild(container);
- render(
- <DropdownMenu.Root>
- <DropdownMenu.Trigger>Menu</DropdownMenu.Trigger>
- <DropdownMenu.Portal container={container}>
- <DropdownMenu.Content>
- <DropdownMenu.Item label="Profile">Profile</DropdownMenu.Item>
- </DropdownMenu.Content>
- </DropdownMenu.Portal>
- </DropdownMenu.Root>
- );
-
- await user.click(screen.getByText('Menu'));
- expect(container).toContainElement(screen.getByText('Profile'));
+ try {
+ render(
+ <DropdownMenu.Root>
+ <DropdownMenu.Trigger>Menu</DropdownMenu.Trigger>
+ <DropdownMenu.Portal container={container}>
+ <DropdownMenu.Content>
+ <DropdownMenu.Item label="Profile">Profile</DropdownMenu.Item>
+ </DropdownMenu.Content>
+ </DropdownMenu.Portal>
+ </DropdownMenu.Root>
+ );
+
+ await user.click(screen.getByText('Menu'));
+ expect(container).toContainElement(screen.getByText('Profile'));
+ } finally {
+ container.remove();
+ }
});📝 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.
Suggested change
| it('supports a custom portal container', async() => { | |
| const user = userEvent.setup(); | |
| const container = document.createElement('div'); | |
| document.body.appendChild(container); | |
| render( | |
| <DropdownMenu.Root> | |
| <DropdownMenu.Trigger>Menu</DropdownMenu.Trigger> | |
| <DropdownMenu.Portal container={container}> | |
| <DropdownMenu.Content> | |
| <DropdownMenu.Item label="Profile">Profile</DropdownMenu.Item> | |
| </DropdownMenu.Content> | |
| </DropdownMenu.Portal> | |
| </DropdownMenu.Root> | |
| ); | |
| await user.click(screen.getByText('Menu')); | |
| expect(container).toContainElement(screen.getByText('Profile')); | |
| }); | |
| it('supports a custom portal container', async() => { | |
| const user = userEvent.setup(); | |
| const container = document.createElement('div'); | |
| document.body.appendChild(container); | |
| try { | |
| render( | |
| <DropdownMenu.Root> | |
| <DropdownMenu.Trigger>Menu</DropdownMenu.Trigger> | |
| <DropdownMenu.Portal container={container}> | |
| <DropdownMenu.Content> | |
| <DropdownMenu.Item label="Profile">Profile</DropdownMenu.Item> | |
| </DropdownMenu.Content> | |
| </DropdownMenu.Portal> | |
| </DropdownMenu.Root> | |
| ); | |
| await user.click(screen.getByText('Menu')); | |
| expect(container).toContainElement(screen.getByText('Profile')); | |
| } finally { | |
| container.remove(); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/ui/DropdownMenu/tests/DropdownMenu.test.tsx` around lines 93 -
111, The test 'supports a custom portal container' creates a div and appends it
to document.body (variable container) but never removes it; wrap the test's
interaction/assertion block in a try/finally (or ensure cleanup) and in the
finally call document.body.removeChild(container) (or container.remove()) so the
portal container is removed after the test; update the test around
DropdownMenu.Root / DropdownMenu.Portal usage to ensure container cleanup even
if assertions fail.
Comment on lines
+41
to
+43
| if (!isOpen && !forceMount) { | ||
| return null; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect HoverCard portal/content forceMount flow"
rg -n -C3 'forceMount|portalForceMount|if \(!isOpen' src/components/ui/HoverCard
echo
echo "Inspect HoverCard forceMount-related tests"
rg -n -C2 'forceMount|container' src/components/ui/HoverCard/testsRepository: rad-ui/ui
Length of output: 9407
Portal short-circuits before children render, breaking HoverCard.Content forceMount when Portal.forceMount is absent.
When <HoverCard.Portal> lacks forceMount={true}, line 41's early return prevents the context provider (line 46) from executing. This means <HoverCard.Content forceMount /> cannot access the portal context and its own forceMount prop becomes ineffective. The Content's conditional check at line 39 verifies portalForceMount, expecting the portal to have propagated it—but that provider never runs.
Example broken scenario:
<HoverCard.Portal>
<HoverCard.Content forceMount />
</HoverCard.Portal>
When closed with Portal.forceMount absent, the portal returns null before Content can render.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/ui/HoverCard/fragments/HoverCardPortal.tsx` around lines 41 -
43, The early return in HoverCardPortal (the if (!isOpen && !forceMount) return
null) prevents the portal context provider from mounting so children like
<HoverCard.Content forceMount /> can't read portalForceMount; remove that
short-circuit and always render the portal context provider in HoverCardPortal
(so the provider is mounted even when closed), but keep the actual DOM/content
conditional (use isOpen or forceMount to decide whether to render the visible
portal nodes) so you still avoid DOM output when fully closed—ensure you
reference/maintain the existing isOpen, forceMount and portalForceMount
variables/props and only move the null-return logic inside the provider's
rendered content.
Comment on lines
+47
to
+50
| <Floater.Portal | ||
| root={rootElem} | ||
| {...props} | ||
| > |
Contributor
There was a problem hiding this comment.
Forward the ref to Floater.Portal.
HoverCardPortal is still wrapped in forwardRef, but the rendered Floater.Portal never receives ref, so consumers cannot observe the portal element through the public API.
Suggested fix
<HoverCardPortalContext.Provider value={{ forceMount }}>
<Floater.Portal
+ ref={ref}
root={rootElem}
{...props}
>📝 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.
Suggested change
| <Floater.Portal | |
| root={rootElem} | |
| {...props} | |
| > | |
| <HoverCardPortalContext.Provider value={{ forceMount }}> | |
| <Floater.Portal | |
| ref={ref} | |
| root={rootElem} | |
| {...props} | |
| > |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/ui/HoverCard/fragments/HoverCardPortal.tsx` around lines 47 -
50, HoverCardPortal is wrapped with forwardRef but never forwards that ref to
the rendered Floater.Portal; update the component so the forwarded ref (usually
named ref or forwardedRef) is attached to Floater.Portal (e.g., pass ref={ref}
or ref={forwardedRef} along with {...props}) so consumers can access the portal
DOM node; ensure the component signature uses React.forwardRef((props, ref) =>
...) and that the ref is passed through to Floater.Portal.
Comment on lines
+106
to
+121
| const container = document.createElement('div'); | ||
| document.body.appendChild(container); | ||
|
|
||
| render( | ||
| <HoverCard.Root open={false} onOpenChange={() => {}} customRootClass="rad-ui"> | ||
| <HoverCard.Trigger>Trigger</HoverCard.Trigger> | ||
| <HoverCard.Portal container={container} forceMount> | ||
| <HoverCard.Content>Content</HoverCard.Content> | ||
| </HoverCard.Portal> | ||
| </HoverCard.Root> | ||
| ); | ||
|
|
||
| const content = await screen.findByRole('dialog', { hidden: true }); | ||
| expect(container).toContainElement(content); | ||
| expect(content).toHaveAttribute('data-state', 'closed'); | ||
| }); |
Contributor
There was a problem hiding this comment.
Clean up the appended portal container in this test.
Line 107 appends a node to document.body, but it is never removed. That can leak DOM state between tests.
Proposed fix
test('supports a custom portal container and portal forceMount', async() => {
mockMatchMedia();
const container = document.createElement('div');
document.body.appendChild(container);
- render(
- <HoverCard.Root open={false} onOpenChange={() => {}} customRootClass="rad-ui">
- <HoverCard.Trigger>Trigger</HoverCard.Trigger>
- <HoverCard.Portal container={container} forceMount>
- <HoverCard.Content>Content</HoverCard.Content>
- </HoverCard.Portal>
- </HoverCard.Root>
- );
-
- const content = await screen.findByRole('dialog', { hidden: true });
- expect(container).toContainElement(content);
- expect(content).toHaveAttribute('data-state', 'closed');
+ try {
+ render(
+ <HoverCard.Root open={false} onOpenChange={() => {}} customRootClass="rad-ui">
+ <HoverCard.Trigger>Trigger</HoverCard.Trigger>
+ <HoverCard.Portal container={container} forceMount>
+ <HoverCard.Content>Content</HoverCard.Content>
+ </HoverCard.Portal>
+ </HoverCard.Root>
+ );
+
+ const content = await screen.findByRole('dialog', { hidden: true });
+ expect(container).toContainElement(content);
+ expect(content).toHaveAttribute('data-state', 'closed');
+ } finally {
+ container.remove();
+ }
});📝 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.
Suggested change
| const container = document.createElement('div'); | |
| document.body.appendChild(container); | |
| render( | |
| <HoverCard.Root open={false} onOpenChange={() => {}} customRootClass="rad-ui"> | |
| <HoverCard.Trigger>Trigger</HoverCard.Trigger> | |
| <HoverCard.Portal container={container} forceMount> | |
| <HoverCard.Content>Content</HoverCard.Content> | |
| </HoverCard.Portal> | |
| </HoverCard.Root> | |
| ); | |
| const content = await screen.findByRole('dialog', { hidden: true }); | |
| expect(container).toContainElement(content); | |
| expect(content).toHaveAttribute('data-state', 'closed'); | |
| }); | |
| test('supports a custom portal container and portal forceMount', async() => { | |
| mockMatchMedia(); | |
| const container = document.createElement('div'); | |
| document.body.appendChild(container); | |
| try { | |
| render( | |
| <HoverCard.Root open={false} onOpenChange={() => {}} customRootClass="rad-ui"> | |
| <HoverCard.Trigger>Trigger</HoverCard.Trigger> | |
| <HoverCard.Portal container={container} forceMount> | |
| <HoverCard.Content>Content</HoverCard.Content> | |
| </HoverCard.Portal> | |
| </HoverCard.Root> | |
| ); | |
| const content = await screen.findByRole('dialog', { hidden: true }); | |
| expect(container).toContainElement(content); | |
| expect(content).toHaveAttribute('data-state', 'closed'); | |
| } finally { | |
| container.remove(); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/ui/HoverCard/tests/HoverCard.test.tsx` around lines 106 - 121,
The test creates and appends a DOM node (const container =
document.createElement('div'); document.body.appendChild(container)) for
HoverCard.Portal but never removes it, risking DOM leakage; update the test
around HoverCard.Root / HoverCard.Portal to remove the appended container after
the assertions (e.g., call container.remove() or
document.body.removeChild(container) in a finally block or use the test
framework's cleanup/afterEach) so the created container is cleaned up after the
test completes.
Comment on lines
+93
to
107
| if (!isOpen) { | ||
| return ( | ||
| <Floater.FloatingList elementsRef={elementsRef} labelsRef={labelsRef}> | ||
| {content} | ||
| </Floater.FloatingList> | ||
| ); | ||
| } | ||
|
|
||
| return ( | ||
| <Floater.FocusManager context={floatingContext}> | ||
| <Floater.FloatingList elementsRef={elementsRef} labelsRef={labelsRef} > | ||
| {content} | ||
| </Floater.FloatingList> | ||
| </Floater.FocusManager> | ||
| ); |
Contributor
There was a problem hiding this comment.
forceMount still remounts the content subtree on open/close.
The closed path renders FloatingList -> content, while the open path renders FocusManager -> FloatingList -> content. That wrapper change causes React to replace the mounted content when isOpen flips, so internal state and enter/exit animations on the same node are lost. Keep the wrapper tree stable across both states and only toggle the focus-management behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/primitives/Combobox/fragments/ComboboxPrimitiveContent.tsx` around
lines 93 - 107, The current conditional renders different wrapper trees for
isOpen (FocusManager -> FloatingList -> content) vs closed (FloatingList ->
content), causing remounts; change the render so the wrapper tree stays
identical and only the focus-management behavior toggles: always render
Floater.FocusManager as the parent of Floater.FloatingList (preserving
elementsRef, labelsRef and content order) and control focus behavior via a prop
or flag on Floater.FocusManager (or pass floatingContext/no-op context when
closed) so the subtree containing content is not replaced when isOpen changes.
Comment on lines
+26
to
+31
| const resolvedRoot = container | ||
| ?? root | ||
| ?? themeContext?.portalRootRef.current | ||
| ?? document.querySelector('[data-rad-ui-portal-root]') as HTMLElement | null | ||
| ?? themeContext?.containerRef.current | ||
| ?? document.querySelector('#rad-ui-theme-container') as HTMLElement | null |
Contributor
There was a problem hiding this comment.
Prefer the current theme container before global document fallbacks.
If themeContext exists but portalRootRef.current is still null, this chain can pick the first [data-rad-ui-portal-root] anywhere in the document before falling back to the current theme’s containerRef. In multi-theme pages that mounts the combobox into the wrong container. Move themeContext?.containerRef.current ahead of the global selectors.
Suggested change
const resolvedRoot = container
?? root
?? themeContext?.portalRootRef.current
- ?? document.querySelector('[data-rad-ui-portal-root]') as HTMLElement | null
?? themeContext?.containerRef.current
+ ?? document.querySelector('[data-rad-ui-portal-root]') as HTMLElement | null
?? document.querySelector('#rad-ui-theme-container') as HTMLElement | null
?? document.body;📝 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.
Suggested change
| const resolvedRoot = container | |
| ?? root | |
| ?? themeContext?.portalRootRef.current | |
| ?? document.querySelector('[data-rad-ui-portal-root]') as HTMLElement | null | |
| ?? themeContext?.containerRef.current | |
| ?? document.querySelector('#rad-ui-theme-container') as HTMLElement | null | |
| const resolvedRoot = container | |
| ?? root | |
| ?? themeContext?.portalRootRef.current | |
| ?? themeContext?.containerRef.current | |
| ?? document.querySelector('[data-rad-ui-portal-root]') as HTMLElement | null | |
| ?? document.querySelector('#rad-ui-theme-container') as HTMLElement | null | |
| ?? document.body; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/primitives/Combobox/fragments/ComboboxPrimitivePortal.tsx` around
lines 26 - 31, The fallback order for resolvedRoot can pick a global document
portal before the current theme's container; update the resolution so
themeContext?.containerRef.current is checked immediately after
themeContext?.portalRootRef.current (i.e., use container ?? root ??
themeContext?.portalRootRef.current ?? themeContext?.containerRef.current ??
document.querySelector('[data-rad-ui-portal-root]') ??
document.querySelector('#rad-ui-theme-container')), ensuring the combobox mounts
into the active theme container when themeContext exists but
portalRootRef.current is null.
Comment on lines
+54
to
71
| if (!isOpen) { | ||
| return ( | ||
| <Floater.FloatingList elementsRef={elementsRef} labelsRef={labelsRef}> | ||
| {content} | ||
| </Floater.FloatingList> | ||
| ); | ||
| } | ||
|
|
||
| return ( | ||
| <> | ||
| <Floater.FloatingList elementsRef={elementsRef} labelsRef={labelsRef}> | ||
| <Floater.FocusManager | ||
| context={floatingContext} | ||
| modal={false} | ||
| initialFocus={isNested ? -1 : 0} | ||
| returnFocus={!isNested} | ||
| > | ||
| <div | ||
| ref={mergedRef} | ||
| style={floatingStyles} | ||
| {...getFloatingProps()} | ||
| className={className} | ||
| {...props} | ||
| > | ||
| <div style={{overflowY:"auto", overflowX:"hidden"}}> | ||
| {children} | ||
| </div> | ||
| </div> | ||
| > | ||
| {content} | ||
| </Floater.FocusManager> |
Contributor
There was a problem hiding this comment.
The menu content is not actually kept mounted across state changes.
With forceMount, the closed branch renders FloatingList -> content, but the open branch inserts FocusManager above the same content. React treats that as a different tree and remounts the subtree when the menu opens/closes, which defeats forceMount for state retention and transition hooks. Keep the wrapper hierarchy identical in both branches.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/primitives/Menu/fragments/MenuPrimitiveContent.tsx` around lines 54
- 71, The wrapper hierarchy must be identical whether open or closed: always
render Floater.FloatingList with elementsRef/labelsRef and always render
Floater.FocusManager inside it (use the existing floatingContext, modal,
initialFocus and returnFocus props), and move the isOpen conditional inside the
FocusManager to hide/disable the inner content (e.g. apply aria-hidden or a
hidden/style prop or pass a disabled flag) instead of swapping the FocusManager
in/out; update MenuPrimitiveContent to render the same Floater.FloatingList ->
Floater.FocusManager -> {content} tree for both branches so forceMount actually
keeps the subtree mounted.
Comment on lines
+25
to
+30
| const resolvedRoot = container | ||
| ?? root | ||
| ?? themeContext?.portalRootRef.current | ||
| ?? document.querySelector('[data-rad-ui-portal-root]') as HTMLElement | null | ||
| ?? themeContext?.containerRef.current | ||
| ?? document.querySelector('#rad-ui-theme-container') as HTMLElement | null |
Contributor
There was a problem hiding this comment.
This fallback order can portal menus into the wrong theme container.
The document-wide [data-rad-ui-portal-root] lookup runs before themeContext?.containerRef.current, so a menu inside one theme can attach to the first matching portal root from another theme instance. Use the current theme container before any global selector fallback.
Suggested change
const resolvedRoot = container
?? root
?? themeContext?.portalRootRef.current
- ?? document.querySelector('[data-rad-ui-portal-root]') as HTMLElement | null
?? themeContext?.containerRef.current
+ ?? document.querySelector('[data-rad-ui-portal-root]') as HTMLElement | null
?? document.querySelector('#rad-ui-theme-container') as HTMLElement | null
?? document.body;📝 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.
Suggested change
| const resolvedRoot = container | |
| ?? root | |
| ?? themeContext?.portalRootRef.current | |
| ?? document.querySelector('[data-rad-ui-portal-root]') as HTMLElement | null | |
| ?? themeContext?.containerRef.current | |
| ?? document.querySelector('#rad-ui-theme-container') as HTMLElement | null | |
| const resolvedRoot = container | |
| ?? root | |
| ?? themeContext?.portalRootRef.current | |
| ?? themeContext?.containerRef.current | |
| ?? document.querySelector('[data-rad-ui-portal-root]') as HTMLElement | null | |
| ?? document.querySelector('#rad-ui-theme-container') as HTMLElement | null | |
| ?? document.body; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/primitives/Menu/fragments/MenuPrimitivePortal.tsx` around lines 25 -
30, The fallback order in MenuPrimitivePortal.tsx can attach portals to a global
selector before the current theme's container; update the resolution of
resolvedRoot so that it prefers container, then root, then
themeContext?.containerRef.current, then themeContext?.portalRootRef.current,
and only after those check document.querySelector('[data-rad-ui-portal-root]')
and document.querySelector('#rad-ui-theme-container'); specifically move
themeContext?.containerRef.current ahead of the global query to ensure menus
inside a theme attach to that theme's container.
Comment on lines
+488
to
+500
| const container = document.createElement('div'); | ||
| document.body.appendChild(container); | ||
|
|
||
| render( | ||
| <MenuPrimitive.Root defaultOpen={true}> | ||
| <MenuPrimitive.Portal container={container}> | ||
| <div>Portal Content</div> | ||
| </MenuPrimitive.Portal> | ||
| </MenuPrimitive.Root> | ||
| ); | ||
|
|
||
| expect(container).toContainElement(screen.getByText('Portal Content')); | ||
| }); |
Contributor
There was a problem hiding this comment.
Remove custom container after the test to keep DOM isolation.
Line 489 appends a container to document.body without cleanup, which can leave residual nodes between tests.
Proposed fix
it('should render portal content into a custom container', () => {
const container = document.createElement('div');
document.body.appendChild(container);
- render(
- <MenuPrimitive.Root defaultOpen={true}>
- <MenuPrimitive.Portal container={container}>
- <div>Portal Content</div>
- </MenuPrimitive.Portal>
- </MenuPrimitive.Root>
- );
-
- expect(container).toContainElement(screen.getByText('Portal Content'));
+ try {
+ render(
+ <MenuPrimitive.Root defaultOpen={true}>
+ <MenuPrimitive.Portal container={container}>
+ <div>Portal Content</div>
+ </MenuPrimitive.Portal>
+ </MenuPrimitive.Root>
+ );
+
+ expect(container).toContainElement(screen.getByText('Portal Content'));
+ } finally {
+ container.remove();
+ }
});📝 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.
Suggested change
| const container = document.createElement('div'); | |
| document.body.appendChild(container); | |
| render( | |
| <MenuPrimitive.Root defaultOpen={true}> | |
| <MenuPrimitive.Portal container={container}> | |
| <div>Portal Content</div> | |
| </MenuPrimitive.Portal> | |
| </MenuPrimitive.Root> | |
| ); | |
| expect(container).toContainElement(screen.getByText('Portal Content')); | |
| }); | |
| const container = document.createElement('div'); | |
| document.body.appendChild(container); | |
| try { | |
| render( | |
| <MenuPrimitive.Root defaultOpen={true}> | |
| <MenuPrimitive.Portal container={container}> | |
| <div>Portal Content</div> | |
| </MenuPrimitive.Portal> | |
| </MenuPrimitive.Root> | |
| ); | |
| expect(container).toContainElement(screen.getByText('Portal Content')); | |
| } finally { | |
| container.remove(); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/primitives/Menu/tests/MenuPrimitive.test.tsx` around lines 488 -
500, The test appends a DOM container div (variable container) to document.body
but never removes it; modify the test (or add an afterEach) so the appended
container is removed after the test completes to maintain DOM isolation —
specifically, ensure the container created in MenuPrimitive.test (the const
container = document.createElement('div') used with MenuPrimitive.Portal) is
cleaned up by calling container.remove() (or
document.body.removeChild(container)) at the end of the test or in a teardown
hook so no residual nodes remain between tests.
kotAPI
changed the title
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Closes #1783.
What changed
containerandforceMountforceMountcan keep menu, combobox, and hover-card content mounted in the closed statecontainerinstead of legacyroot/rootElementWhy
container,root,rootElement) and unevenforceMountsupportTests / validation
npm test -- --runInBand src/core/primitives/Menu/tests/MenuPrimitive.test.tsx src/components/ui/Combobox/tests/Combobox.full.test.tsx src/components/ui/DropdownMenu/tests/DropdownMenu.test.tsx src/components/ui/HoverCard/tests/HoverCard.test.tsxnpm test -- --runInBand src/components/ui/Menubar/tests/Menubar.test.tsx src/components/ui/ContextMenu/tests/ContextMenu.test.tsxnpm run check:typesstill fails on unrelated pre-existing issues inDrawerSwipeZone,Popover.stories,ToastRoot, andClarity.storiesRisks / follow-ups
root/rootElementaliases are still accepted for compatibility, but stories now point developers tocontainerSummary by CodeRabbit
Release Notes
New Features
forceMountprop to menu, combobox, hover card, and select portal components, enabling overlay content to remain mounted in the DOM while closedcontainerprop to portal components for specifying custom DOM mount targetsTests
forceMountandcontainerportal functionality