Repository navigation
fix(usePopover): its trigger ref stays the same function across renders - #9130
Conversation
usePopover built its trigger ref from the whole layer object, which changes whenever the popover's onShow or onHide does. A caller that passes inline callbacks got a new trigger ref on every render, so React detached and reattached the trigger each time. The layer's anchor came off the trigger and went back, two writes to its inline style per render, and with a tooltip anchored to the same trigger the anchor names changed order. The trigger ref is now keyed on the layer's own ref, which is stable, as useHoverCard's refs were in meta#6606. DropdownMenu also hands its trigger stable refs on all three trigger paths: a custom trigger, the default button (still forwarding the caller's ref), and the bottom-sheet trigger. Its own inline refs churned the same way.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
PR Analysis Report
📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsPopover (@astryxdesign/core) · View in Storybook
Component Size
Sizes are gzip. Import counts the JavaScript the import loads eagerly; CSS counts the package stylesheet rules referenced by the modules it can load from its own package, lazily loaded ones included. Bundle Size SummaryGzip figures measure each package's entry file, not the code an import pulls in.
Accessibility AuditStatus: The accessibility audit did not run on this pull request. It runs after merges to Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
cixzhang
added
the
ci:run
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
The usePopover and DropdownMenu fix looks right: the new tests fail without it and pass with it, and the related component suites pass. One change is needed before this lands: the changeset says DateInput, DateRangeInput, MultiSelector and ChatComposerInput's trigger menu are fixed too, but they are not.
- DateInput, DateRangeInput and MultiSelector wrap the trigger ref in their own inline arrow, so React still detaches and reattaches their triggers on every render (so does ComplexSelector's
renderTriggerpath). ChatComposerInput's trigger menu callspopover.triggerRefdirectly instead of passing it as a ref, so this change does not affect it.
Please list only the triggers that take usePopover's ref directly, or stabilize those wrapper refs in this pull request.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] `usePopover`'s trigger ref now stays the same function when the popover's `onShow` or `onHide` changes, and `DropdownMenu`'s own trigger refs are stable too. A trigger that takes it (DropdownMenu, Breadcrumbs, ComplexSelector, DateInput, DateRangeInput, DateTimeInput, MultiSelector and the trigger menu of `ChatComposerInput`) is no longer detached and reattached on renders that pass new callbacks, so its style is not rewritten each time and a tooltip anchored to the same trigger keeps its place. |
There was a problem hiding this comment.
This list goes further than the change does. DateInput (DateInput.tsx:838), DateRangeInput (DateRangeInput.tsx:697) and MultiSelector (MultiSelector.tsx:2580) wrap popover.triggerRef in an inline arrow, so React still detaches and reattaches their triggers on every render; ComplexSelector's renderTrigger path (ComplexSelector.tsx:781) does the same. ChatComposerInput's trigger menu calls the ref directly and never passes it to React. Suggest naming only DropdownMenu, ComplexSelector's built-in trigger, DateTimeInput's date field and Breadcrumbs, or stabilizing those wrappers here.
There was a problem hiding this comment.
Narrowed in 08a5dc5: the changeset and description now list only the triggers that take the ref directly.
… reaches DateInput, DateRangeInput, MultiSelector and ComplexSelector's custom trigger wrap the trigger ref in an inline function of their own, and the composer's trigger menu calls it directly instead of passing it as a ref, so this change does not reach them. The changeset now names DropdownMenu and the triggers that take usePopover's ref directly.
There was a problem hiding this comment.
The usePopover and DropdownMenu trigger-ref fix is ready to land: the new tests fail without it and pass with it, and the related component suites pass. The narrowed changeset now matches what the change reaches, so nothing more is needed here.
[Automated review]
User impact
A
DropdownMenuwhose trigger also has a tooltip (a custom trigger inTooltip, or the default button'stooltip) rewrites that button's inline style on every render around it. The menu's anchor comes off and goes back, soanchor-nameis rewritten and its two names swap order. A page that renders often never settles. We found it moving one app onto Astryx.Problem and solution fit
DropdownMenu's trigger ref was a new function each render, so React called it again on every commit (null, then the element), moving the anchor each time.usePopover'striggerRef, keyed on the whole layer object, which changes wheneveronShoworonHidedoes;DropdownMenu'sonShowchanges with an inlinerenderTrigger,onClickoronOpenChange. That version still made 2 and 4 writes per render in the tests below.The callback reads nothing else from the layer, and
layer.ref(useLayer's anchor callback, keyed on the anchor name and anchor-clearance option) reads only refs, so nothing goes stale.Expected behavior and authority
Tooltip (#5951) and
useHoverCard(#6606) got the same fix: a trigger ref keeps its identity when nothing about the trigger changes. No API or record changes.Smallest restoration
usePopover:triggerRefis keyed onlayer.refinstead oflayer.DropdownMenu: stable refs on its three trigger paths, a custom trigger, the default button (still forwarding the caller'sref) and the bottom-sheet trigger.usePopover's ref directly get it stable too: ComplexSelector's built-in trigger, DateTimeInput's date field and Breadcrumbs.Not chosen: stabilizing each
DropdownMenucallback instead, leaving every otherusePopovercaller churning.Also edited by open #7677 and #8550, both of which merge cleanly with this change.
Evidence
Tooltip, and 4 times with the default button'stooltipand an inlineonOpenChange. The two anchor names swap order, andusePopover'striggerRefchanges wheneveronShoworonHidedoes.refon the default button still gets the element.Scope
Testing
How to check:
<DropdownMenu button={{label: 'Actions', tooltip: 'More'}} onOpenChange={() => {}}>inside a component that renders every second.styleattribute in the browser's Elements panel. Before: it flashes every second. After: it stays still.The new cases in
DropdownMenu.test.tsxandPopover.test.tsxcount the trigger's style writes with a MutationObserver. They fail on main and pass with the fix.Local gates at 08a5dc5:
usePopover's trigger ref (Breadcrumbs, Chat, ComplexSelector, DateInput, DateRangeInput, DateTimeInput, MultiSelector): 2,171 tests pass. The 6 new cases fail on main.pnpm check:repoand the Storybook build.pnpm test:uiandpnpm test:node: the same failures as on main, none in these components.