Repository navigation
Conversation
…omSheet's padding and focus code Since Popover loads a BottomSheet with import() for its phone presentation (meta#8229), esbuild puts a module that both the eager code and the BottomSheet chunk import into one shared chunk, which the entry imports statically and which keeps every export either side uses. Layout/padding.stylex.ts was such a module: usePopover uses paddingStyles and useLayer uses overlayPaddingReset, while BottomSheetPanel uses the container padding variables, so every component that imports Popover eagerly carried 3,975 B of it instead of 1,413. utils/focusReturn.ts was another, reached eagerly through the utils barrel's isFocusDetached re-export while only BottomSheet uses its focusForLastInput (264 B). paddingStyles and overlayPaddingReset now live in their own modules, which padding.stylex.ts re-exports for its public entry and which useLayer and usePopover import directly; isFocusDetached moves to its own module, which the barrel re-exports.
vjeux
requested review from
cixzhang,
imdreamrunner and
josephfarina
as code owners
October 26, 2026 16:14
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
the
needs:design-review
github-actions
Bot
requested review from
cvkxx,
ernestt,
kentonquatman and
rubyycheung
October 26, 2026 16:15
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
Contributor
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: No accessibility violations detected. Visual Regression55 of 502 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 35 more. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
vjeux
marked this pull request as draft
October 29, 2026 22:37
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This looks ready to merge: moving paddingStyles, overlayPaddingReset and isFocusDetached into their own modules keeps BottomSheet-only code out of the eager bundles of Popover, DropdownMenu and the components built on them, while the public entry points, the built stylesheet and the rendered stories stay the same. The visual report compares against an older baseline: the Selector sheet, Collapsible and AppShell stories it flags render identically before and after this change. One optional fix: the changeset's "about 3.5 KB minified" fits Carousel, ContextMenu, Markdown and Table, but Popover saves about 2.9 KB, DropdownMenu about 2.6 KB and DateInput only 66 B, so "up to about 3.8 KB minified (about 1 KB gzip)" would be accurate.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [perf] Components that import Popover no longer load BottomSheet's container padding styles and focus helper up front. Popover loads a BottomSheet on demand for its phone presentation, and a bundler that splits code put the modules both of them use into one chunk that loads with the component, keeping every export either side uses: about 3.5 KB minified in Popover, Carousel, DateRangeInput, DropdownMenu, Table and the other components that import Popover or DropdownMenu. `paddingStyles` and `overlayPaddingReset` now have their own modules, still exported from `@astryxdesign/core/Layout/padding.stylex`, and `isFocusDetached` has its own module, still exported from `@astryxdesign/core/utils`. Rendering is unchanged. |
There was a problem hiding this comment.
Optional: "about 3.5 KB minified" fits Carousel, ContextMenu, Markdown and Table, but Popover saves about 2.9 KB, DateRangeInput about 2.9 KB, DropdownMenu about 2.6 KB and DateInput only 66 B. "Up to about 3.8 KB minified (about 1 KB gzip)" would be accurate.
4 tasks done
cixzhang
pushed a commit
that referenced
this pull request
Oct 31, 2026
…rs (#9130) ## User impact A `DropdownMenu` whose trigger also has a tooltip (a custom trigger in `Tooltip`, or the default button's `tooltip`) rewrites that button's inline style on every render around it. The menu's anchor comes off and goes back, so `anchor-name` is 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. - Stabilizing those refs alone was not enough. They wrap `usePopover`'s `triggerRef`, keyed on the whole layer object, which changes whenever `onShow` or `onHide` does; `DropdownMenu`'s `onShow` changes with an inline `renderTrigger`, `onClick` or `onOpenChange`. That version still made 2 and 4 writes per render in the tests below. - So the fix is in the hook: ```ts const layerRef = layer.ref; const triggerRef = useCallback( (el: HTMLElement | null) => { triggerElementRef.current = el; layerRef(el); }, [layerRef], ); ``` 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`: `triggerRef` is keyed on `layer.ref` instead of `layer`. - `DropdownMenu`: stable refs on its three trigger paths, a custom trigger, the default button (still forwarding the caller's `ref`) and the bottom-sheet trigger. - Triggers that take `usePopover`'s ref directly get it stable too: ComplexSelector's built-in trigger, DateTimeInput's date field and Breadcrumbs. Not chosen: stabilizing each `DropdownMenu` callback instead, leaving every other `usePopover` caller churning. Also edited by open #7677 and #8550, both of which merge cleanly with this change. ## Evidence - Before (main eae5d92): one parent render writes the trigger's style 2 times with a custom trigger in `Tooltip`, and 4 times with the default button's `tooltip` and an inline `onOpenChange`. The two anchor names swap order, and `usePopover`'s `triggerRef` changes whenever `onShow` or `onHide` does. - After: 0 writes, the same order, the same ref. A caller's `ref` on the default button still gets the element. ## Scope - [x] One defect is restored. - [x] Separable contradictory or unsettled tagalongs were removed or split. - [x] No new public API, visual representation, default, or interaction model is hidden under the bug-fix label. - [x] Public text and artifacts contain no internal Meta context. ## Testing How to check: 1. Render `<DropdownMenu button={{label: 'Actions', tooltip: 'More'}} onOpenChange={() => {}}>` inside a component that renders every second. 2. Watch the button's `style` attribute in the browser's Elements panel. Before: it flashes every second. After: it stays still. The new cases in `DropdownMenu.test.tsx` and `Popover.test.tsx` count the trigger's style writes with a MutationObserver. They fail on main and pass with the fix. Local gates at 08a5dc5: - The DropdownMenu, Popover, Tooltip and Layer suites, plus every other component that uses `usePopover`'s trigger ref (Breadcrumbs, Chat, ComplexSelector, DateInput, DateRangeInput, DateTimeInput, MultiSelector): 2,171 tests pass. The 6 new cases fail on main. - The DropdownMenu, Tooltip and Popover Chromium specs: 217 passed. - Core typecheck, ESLint with no warnings, Prettier, `pnpm check:repo` and the Storybook build. - `pnpm test:ui` and `pnpm test:node`: the same failures as on main, none in these components.
This branch was successfully deployed
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
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.
Review only this PR's change: vjeux/astryx@c4a9ae0...f665bef
User impact
Importing Popover, a component that imports it (Carousel, Table and others) or DropdownMenu loads about 3.5 KB minified of BottomSheet's padding styles and focus helper, though BottomSheet loads only on a phone.
Problem and solution fit
import()(feat(Popover): presentation — a BottomSheet on a phone when asked, as iOS presents a popover #8229), so its import should not carry it.Layout/padding.stylex.tswas one:usePopoverusespaddingStylesanduseLayeroverlayPaddingReset, whileBottomSheetPaneluses the container padding variables. Popover's import carried 3,975 B of it, against 1,413 B before feat(Popover): presentation — a BottomSheet on a phone when asked, as iOS presents a popover #8229.utils/focusReturn.tswas another: the utils barrel'sisFocusDetachedre-export reaches it eagerly, while only BottomSheet uses itsfocusForLastInput(264 B).Expected behavior and authority
architecture:component-style-authoringINV6: a coarse-pointer feature loads lazily, so the component's import does not carry it.Smallest restoration
paddingStylesandoverlayPaddingResetmove to their own modules, whichpadding.stylex.tsre-exports (its public entry is unchanged) anduseLayerandusePopoverimport.isFocusDetachedmoves to its own module, still re-exported byutils.Evidence
The INV6 size tool's bundle, main c4a9ae0 to this head:
overlayPaddingReset.padding.stylex.jsand nofocusReturn.js; at feat(Popover): presentation — a BottomSheet on a phone when asked, as iOS presents a popover #8229, 3,975 B and 264 B.Scope
Testing
check:changesets, eslint, prettier: clean.