Commit 9140963
authored
fix(playground-ui): make popup portal container resolution null-safe (#17560)
## Summary
Follow-up hardening to #17556. Base UI's `FloatingPortal` treats
`container={null}` as *"not ready — render nothing"*, while `undefined`
means *"portal to `document.body`"*. Our `PortalContainerContext`
defaulted to `null`, so popups opened outside a `SideDialog` (the model
& provider pickers live in the chat composer, which has no
`PortalContainerProvider` ancestor) resolved their portal container to
`null` and mounted to no DOM node — they opened in state but rendered
nowhere.
## Change
- `usePortalContainer(container?)` now resolves `explicit → provider →
undefined` and **never returns `null`**; the "no container" sentinel is
`undefined` to match Base UI's contract.
- Dropped the scattered `?? undefined` band-aids in `Combobox`,
`Select`, `Popover`, and `DropdownMenu` — they now just call
`usePortalContainer(container)`.
## Tests
- `portal-container.test.tsx` guards the resolver contract (never
`null`; normalizes a `null` provider; explicit wins).
- `combobox.test.tsx` asserts the popup portals into `document.body`.
- Verified these fail under the old `null`-leaking behavior (combobox
open/select tests go red) and pass with the fix.
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## ELI5
Imagine a magical doorway that decides where popup menus (like dropdowns
and options) should appear on the screen. This PR fixes a broken doorway
that prevented popups from showing up when opened outside certain
panels—now the doorway always works and shows popups in the right place
(the main document area) as a fallback.
## Problem
Base UI's FloatingPortal treats `container={null}` as "not ready —
render nothing" while `undefined` means "portal to document.body". The
PortalContainerContext was defaulting to `null`, which caused popups
opened outside a SideDialog (such as model and provider pickers in the
chat composer) to fail rendering because there was no valid DOM node to
attach to.
## Solution
The `usePortalContainer` hook was redesigned to:
- Accept an optional `container` parameter that can explicitly override
the default behavior
- Resolve the final portal target with precedence: explicit argument →
context provider value → `undefined`
- Never return `null`; always return `undefined` as the "no container"
sentinel to match Base UI's expectations
The PortalContainerContext now treats `undefined` as the default (fall
back to document.body) and normalizes any incoming `null` values to
`undefined`.
## Changes Made
- **portal-container.tsx**: Added `PortalContainer` type, updated
`usePortalContainer` to accept an optional container override and
exclude null from its return type, and normalized null values in the
provider context
- **Combobox, Select, Popover, and DropdownMenu**: Replaced manual
fallback logic (`container ?? portalContainer ?? undefined`) with direct
calls to `usePortalContainer(container)`
- **portal-container.test.tsx**: Added comprehensive tests asserting the
resolver contract (never returns null, normalizes null provider to
undefined, explicit values override provider values)
- **combobox.test.tsx**: Added test verifying popups mount into
`document.body` when no portal container provider is present
## Testing
Tests were added to verify the contract of the portal-container resolver
and confirm that popups now mount correctly when opened outside a
SideDialog. Tests were verified to fail under the previous null-leaking
behavior and pass with the fix.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->1 parent 5532141 commit 9140963
8 files changed
Lines changed: 108 additions & 32 deletions
File tree
- .changeset
- packages/playground-ui/src/ds
- components
- Combobox
- DropdownMenu
- Popover
- Select
- primitives
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
Lines changed: 14 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
45 | 45 | | |
46 | 46 | | |
47 | 47 | | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
48 | 62 | | |
49 | 63 | | |
50 | 64 | | |
| |||
Lines changed: 1 addition & 2 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
60 | 60 | | |
61 | 61 | | |
62 | 62 | | |
63 | | - | |
64 | | - | |
| 63 | + | |
65 | 64 | | |
66 | 65 | | |
67 | 66 | | |
| |||
Lines changed: 3 additions & 4 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
79 | 79 | | |
80 | 80 | | |
81 | 81 | | |
82 | | - | |
| 82 | + | |
83 | 83 | | |
84 | | - | |
| 84 | + | |
85 | 85 | | |
86 | 86 | | |
87 | 87 | | |
| |||
111 | 111 | | |
112 | 112 | | |
113 | 113 | | |
114 | | - | |
115 | | - | |
| 114 | + | |
116 | 115 | | |
117 | 116 | | |
118 | 117 | | |
| |||
Lines changed: 2 additions & 3 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
34 | 34 | | |
35 | 35 | | |
36 | 36 | | |
37 | | - | |
38 | | - | |
| 37 | + | |
39 | 38 | | |
40 | 39 | | |
41 | | - | |
| 40 | + | |
42 | 41 | | |
43 | 42 | | |
44 | 43 | | |
| |||
Lines changed: 2 additions & 3 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
158 | 158 | | |
159 | 159 | | |
160 | 160 | | |
161 | | - | |
162 | | - | |
| 161 | + | |
163 | 162 | | |
164 | | - | |
| 163 | + | |
165 | 164 | | |
166 | 165 | | |
167 | 166 | | |
| |||
Lines changed: 59 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
Lines changed: 22 additions & 20 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | 1 | | |
2 | 2 | | |
3 | 3 | | |
4 | | - | |
5 | | - | |
6 | | - | |
7 | | - | |
8 | | - | |
9 | | - | |
10 | | - | |
11 | | - | |
12 | | - | |
13 | | - | |
14 | | - | |
15 | | - | |
16 | | - | |
17 | | - | |
18 | | - | |
19 | | - | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
20 | 20 | | |
21 | 21 | | |
22 | 22 | | |
| |||
25 | 25 | | |
26 | 26 | | |
27 | 27 | | |
28 | | - | |
| 28 | + | |
| 29 | + | |
29 | 30 | | |
30 | 31 | | |
31 | | - | |
32 | | - | |
33 | | - | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
34 | 36 | | |
0 commit comments