Skip to content

[popups] Improve trigger mount performance - #5426

Merged
atomiks merged 6 commits into
mui:masterfrom
atomiks:codex/improve-popup-trigger-mount-performance
Aug 7, 2026
Merged

[popups] Improve trigger mount performance#5426
atomiks merged 6 commits into
mui:masterfrom
atomiks:codex/improve-popup-trigger-mount-performance

Conversation

@atomiks

@atomiks atomiks commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

This PR reduces the work needed to mount closed popup roots and triggers.

Changes

  • Initializes each popup's real floating context directly instead of creating and replacing an unused context.
  • Seeds Menu's initial inactive-trigger props before triggers subscribe, avoiding a second render after mount.
  • Batches store subscriptions in Dialog and Popover roots and triggers.
  • Keeps Menu dismissal and typeahead interactions mounted normally, so the mount improvement does not move their initialization cost to first open.

Performance

Scenario Fixture Base UI 1.7.0 This PR Improvement
Menu closed mount 300 roots, 1 trigger each 22.4 ms 16.4 ms 26.9%
Menu closed mount 100 roots, 20 triggers each 72.5 ms 42.6 ms 41.2%
Menu first open 1 trigger, 10 items 2.0 ms 1.7 ms 14.4%
Menu subsequent open 1 trigger, 10 items 2.2 ms 1.5 ms 30.6%
Popover closed mount 500 triggers 21.3 ms 16.5 ms 22.4%
Dialog closed mount 500 triggers 15.6 ms 13.2 ms 15.4%

Menu results are React render totals from the repository's production benchmark harness. Base UI 1.7.0 is averaged across two runs and this PR across three runs. Popover and Dialog results use the standalone production harness and average two reverse-order fresh-browser runs per version. Every run uses 20 measured iterations with outlier filtering.

@atomiks atomiks added type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature. component: menu Changes related to the menu component. component: popover Changes related to the popover component. component: dialog Changes related to the dialog component. component: alert dialog Changes related to the alert dialog component. component: drawer Changes related to the drawer component. component: tooltip Changes related to the tooltip component. component: preview card Changes related to the preview card component. labels Aug 6, 2026
@code-infra-dashboard

code-infra-dashboard Bot commented Aug 6, 2026

Copy link
Copy Markdown

Bundle size

Bundle Parsed size Gzip size
@base-ui/react 🔺+83B(+0.02%) 🔺+146B(+0.10%)

Details of bundle changes

Performance

Total duration: 1,140.30 ms -85.31 ms(-7.0%) | Renders: 76 (▼-2) | Paint: 1,804.27 ms -143.42 ms(-7.4%)

Test Duration Renders
Menu mount (300 instances) 87.43 ms ▼-47.71 ms(-35.3%) 1 (▼-1)
Menu open (500 items) 69.01 ms -9.78 ms(-12.4%) 11 (▼-1)

13 tests within noise — details


Check out the code infra dashboard for more information about this PR.

@netlify

netlify Bot commented Aug 6, 2026

Copy link
Copy Markdown

Deploy Preview for base-ui ready!

Name Link
🔨 Latest commit 3436cb6
🔍 Latest deploy log https://app.netlify.com/projects/base-ui/deploys/6a756705af3f1600088f450b
😎 Deploy Preview https://deploy-preview-5426--base-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@pkg-pr-new

pkg-pr-new Bot commented Aug 6, 2026

Copy link
Copy Markdown

commit: 3436cb6

@atomiks atomiks added scope: all components Widespread work has an impact on almost all components. and removed component: menu Changes related to the menu component. component: alert dialog Changes related to the alert dialog component. component: drawer Changes related to the drawer component. component: tooltip Changes related to the tooltip component. component: dialog Changes related to the dialog component. component: popover Changes related to the popover component. component: preview card Changes related to the preview card component. labels Aug 7, 2026
@atomiks
atomiks force-pushed the codex/improve-popup-trigger-mount-performance branch from 702110d to a3fb892 Compare August 7, 2026 02:20
@atomiks

atomiks commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

@claude review low

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown

PR review

Nothing here is merge-blocking. The refactor is mostly a re-shaping of when state that already existed gets its final value: each popup store now builds its real FloatingRootStore inside createInitialPopupStoreState instead of allocating a throwaway empty context and swapping it in a layout effect, and Menu seeds trigger props / controlled-ish values into the store before triggers subscribe. I traced every removed line (createPopupFloatingRootContext, getEmptyRootContext in store.ts, the post-spread state.floatingRootContext = … assignments) to its replacement and all five stores plus both test helpers were updated consistently; getEmptyRootContext still has legitimate remaining users in navigation-menu. The fastComponent/fastComponentRef wrapping of Dialog/Popover Root and Trigger matches what Menu/Tooltip/PreviewCard already ship, and I verified those four components have a fixed, unconditional useStore call count (the batching layer matches hooks by call index, so a varying count would crash) — including the hooks they call (useTriggerDataForwarding, useClick, useHoverReferenceInteraction, useTriggerFocusGuards), none of which read stores conditionally. Two informational notes below; this was a low-effort pass, so tests, docs, and simplifications were not systematically reviewed.

Bugs (2)

1. ℹ️ Menu's floating context now starts closed even when the root mounts open

Location: packages/react/src/utils/popups/store.ts:93

floatingRootContext: new FloatingRootStore({
  open: false,
  ...

Previously Menu did not pass floatingRootContext into useSyncedFloatingRootContext, so the hook lazily constructed the store on the first render seeded with open: popupStore.useState('open') — i.e. true for <Menu.Root defaultOpen> or an initially-open controlled root. Now the context is built in the MenuStore constructor with a hardcoded open: false, and only reaches true in the useIsoLayoutEffect inside useSyncedFloatingRootContext, which lives in MenuRoot and therefore runs after descendant layout effects.

This is almost certainly benign: Popover, Dialog, Tooltip and PreviewCard have always had exactly this shape (their createPopupFloatingRootContext also hardcoded open: false), the store update happens synchronously in the same commit so the corrected pass still lands before paint, and MenuRoot.test.tsx already exercises defaultOpen: true around focus behavior. Flagging it because it is the one genuine semantic delta in the refactor rather than a pure reordering.

Failure scenario: On the first commit of <Menu.Root defaultOpen>, FloatingFocusManager and useFloating inside the positioner read open === false from the floating root store and skip their initial-focus / position-setup work, relying on the parent's layout effect to flip the flag and force a second effect pass. Any descendant that latches on open without an open dependency would silently miss the transition.

Fix: No change required if the existing defaultOpen menu tests pass. If you want the old first-render value back, seed the store from the initial open state, e.g. pass initialState.open ?? false through createInitialPopupStoreState instead of the literal false.

2. ℹ️ Render-phase store.update() is safe only while nothing subscribes during the Root's first render

Location: packages/react/src/menu/root/MenuRoot.tsx:486

useRefWithInit(() => {
  store.update({ inactiveTriggerProps });
  return null;
});

The comment states the invariant correctly, and I confirmed it holds today: the store is created in this same render, PopupHandleAttachment attaches in a layout effect, and MenuRoot's own batched useSyncExternalStore subscribes after render — so update() notifies nobody and the render stays observably pure. It is worth knowing that the invariant is positional: moving this seed below anything that subscribes, or extending the same trick to a store that is already live, turns a render into a listener-notifying side effect.

The seeding is also asymmetric — activeTriggerProps and popupProps are left at EMPTY_OBJECT — so an initially-open menu's active trigger still renders twice on mount; only the (much more common) all-inactive case gets the win. That looks deliberate given the perf numbers, just not stated.

Failure scenario: A future change that renders any store subscriber above line 486 in MenuRoot — or reuses this pattern in a Root whose store can already be attached to a handle — would mutate a subscribed store during render, producing a tearing/useSyncExternalStore warning that is hard to trace back to this line.

Fix: Optional. Consider noting in the comment that the seed must stay above every subscriber and above PopupHandleAttachment, and why activeTriggerProps/popupProps are intentionally not seeded.

Verdict

Approve - no correctness regressions found; both notes are informational and the store/trigger-props reshaping is internally consistent across all five popup stores.


🤖 Review generated with Claude Code · low effort · 60 turns · 10m10s · $3.79 · run

@atomiks
atomiks marked this pull request as ready for review August 7, 2026 05:03
@atomiks
atomiks requested a review from colmtuite as a code owner August 7, 2026 05:03
@atomiks
atomiks merged commit 1aef3bf into mui:master Aug 7, 2026
23 checks passed
@atomiks
atomiks deleted the codex/improve-popup-trigger-mount-performance branch August 11, 2026 03:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope: all components Widespread work has an impact on almost all components. type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant