Repository navigation
[popups] Avoid re-rendering inactive triggers on open and close - #5865
Conversation
commit: |
Bundle size
PerformanceTotal duration: 515.75 ms ▼-424.33 ms(-45.1%) | Renders: 76 (+0) | Paint: 915.04 ms ▼-718.56 ms(-44.0%)
…and 7 more (+3 within noise) — details Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
bb8bb4f to
cf4be11
Compare
PR reviewNothing blocks merge. On every component I traced, inactive handle-backed triggers stop re-rendering. The three new The one real regression is in Tooltip. Its interactions now stay mounted, and that leaves a stale cursor-point reference behind between open sessions. A smaller change to the Tooltip trigger would avoid both the bug and the retained state. I traced everything by reading the code; I did not run any tests. Bugs (1)1. 🟠 Handle-backed Tooltip keeps a stale
|
0e383cc to
9db1021
Compare
PR reviewNothing blocks merge. The trigger re-render fix checks out. The new Popover and Tooltip selectors behave exactly like the expressions they replace, and the deleted Bugs (2)1. 🟠 A closed dialog with a handle picks up nested-dialog counts and opens as "not topmost"Location: const shouldRenderInteractions = open || mounted || handle != null;
// …
store.useContextCallback('onNestedDialogOpen', (dialogCount, drawerCount) => {
setOwnNestedOpenDialogs(dialogCount);
setOwnNestedOpenDrawers(drawerCount);
});Before this PR, a closed parent Dialog had no Failure scenario: The parent is
The same thing happens when the parent closes while C stays open and the parent is reopened later. Fix:
2. ℹ️
|
9db1021 to
f5cdc95
Compare
PR reviewThe author fixed every finding from the last round except the optional How the earlier findings were handled:
Tests (2)1. 🟡 The reset of the nested count on close isn't testedLocation: useIsoLayoutEffect(() => {
if (!open && !mounted) {
setOwnNestedOpenDialogs(0);
setOwnNestedOpenDrawers(0);
}
}, [open, mounted]);Neither new test fails if this effect is deleted:
The effect only matters in this sequence:
Fix: Add that sequence as a test, and assert that the reopened P has no 2. ℹ️ CI's
|
Fixes #5895
With a shared handle, every inactive Popover, Tooltip, PreviewCard, and Dialog trigger re-rendered on each open and close. This PR mounts a handle-backed Popover, PreviewCard, or Dialog Root's interactions eagerly so trigger props stay stable, has inactive Tooltip triggers stop subscribing to root props they discard, scopes Popover trigger subscriptions per trigger, and resets the state the retained interactions keep between open sessions (including shared
useDismissstate).Performance
Main-thread time in ms with 1,000 triggers, open / close (production build, headless Chromium, M4 Pro; median of 3 interleaved page loads × 10 cycles):
Tooltip gets the contained-trigger win too, since its fix is in the trigger rather than the Root. The trade-off is the last column: a Popover, PreviewCard, or Dialog Root with a handle now mounts its interactions up front, about 10 µs per Root when every row gets its own handle (these rows vary by about ±10 ms between runs; Tooltip no longer mounts anything extra). A shared handle, the case this targets, mounts no slower.