[popover] Fix detached trigger store migration - #5442
Conversation
atomiks
added
type: regression
commit: |
Bundle size
PerformanceTotal duration: 1,120.11 ms ▼-315.39 ms(-22.0%) | Renders: 76 (+0) | Paint: 1,779.79 ms ▼-489.73 ms(-21.6%)
…and 4 more (+6 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. |
Make the trigger registration callback stable and track the active registration as a `(store, id, element)` triple. A downstream ref merger that retains the callback it was first given now still reaches the trigger's current store, and unregistering always targets the store the element was registered in. Migration between stores moves to a layout effect keyed on `[store, id]`, since a stable ref no longer re-fires on those changes. Skipping a register call that would re-register the same element under the same store and id keeps map churn at its previous level.
`useTriggerRegistration` now returns a stable callback, so the ref no longer re-fires when the swipe area's id changes. Re-register from a layout effect keyed on `[store, id]`, matching `useTriggerDataForwarding`. Without it the swipe area never registers on React 17, where `useId` returns `undefined` on the first render and resolves in an effect, and a changed `id` prop strands the registration under the old id. Cover both cases, plus the ref-only caller shape in the `useTriggerRegistration` unit tests.
`useTriggerRegistration` returns `void`, so the captured `cleanup` was always `undefined`.
|
@claude review low |
PR reviewNothing here is merge-blocking. The core move — making the trigger-registration ref stable and tracking the live registration as a Bugs (1)1. ℹ️ Early return drops the old code's self-healing re-registrationLocation: if (registration !== null) {
if (
registration.element === element &&
registration.store === store &&
registration.id === id
) {
// Already registered where it belongs, so the caller's migration effect is free on mount.
return;
}The early return trusts The divergence is only observable when two concurrently mounted triggers share an id (a consumer passing the same The id-collision precondition is a consumer mistake either way, and the old behavior only recovered by accident, so I don't think this needs to change. Flagging it because the invariant Failure scenario: Two Fix: Optional. If you want the old resilience back, re-verify before the shortcut, e.g. only early-return when VerdictApprove — the stable-callback plus 🤖 Review generated with Claude Code · |
1 participant
Fixes #5416
(Cause: #5149)
Base UI 1.7 changed a detached trigger's registration ref when its handle moved between the fallback and Root stores. A ref-merging component that retains the callback it was first given never calls the new one, so the trigger stays registered on the fallback store. Hover handoff resolves the destination trigger through the attached store's trigger map, finds nothing, and closes the popup instead of switching to the new trigger.
Changes
(store, id, element)triple, so unregistering targets the store the element was registered in rather than whichever store the caller captured.[store, id], since a stable ref no longer re-fires on those changes.Notes
A stable ref also fixes a second failure with the same precondition. Once a merger has retained the callback from before the trigger migrated, replacing the rendered host node re-invokes that callback. It closes over the previous store, so the live node returns to the fallback store while the attached store keeps the disconnected one.
handle.open(triggerId)searches attached stores first, so the popup then anchors to a removed element. A component that merges refs correctly, or does not merge at all, was never affected.Consumers' own refs are no longer detached and reattached when a trigger's handle moves between stores.
Tests
closeDelayis what forces the handoff path: without it the popup closes and reopens, which passes even when the trigger is registered on the wrong store.handle.open()anchors to the live node.useTriggerRegistrationunit test covering callback stability and unregistering from the correct store.