Skip to content

[toast] Remove memoized selector#4751

Merged
atomiks merged 2 commits into
mui:masterfrom
atomiks:codex/remove-toast-reselect
May 11, 2026
Merged

[toast] Remove memoized selector#4751
atomiks merged 2 commits into
mui:masterfrom
atomiks:codex/remove-toast-reselect

Conversation

@atomiks
Copy link
Copy Markdown
Contributor

@atomiks atomiks commented May 6, 2026

Removes Toast's memoized selector use while keeping the ID-based toast metadata lookup fast.

Changes

  • Store toast metadata directly when the toast list changes.
  • Keep toast metadata synchronized through close transitions.
  • Leave createSelectorMemoized and reselect in utils for other consumers.

Performance

Local Node microbenchmark comparing upstream/master to this PR with 50 toasts:

Operation upstream/master PR
Metadata map rebuild after toast list change 1.864 us/op 1.100 us/op
Cached selector read by id 0.017 us/op 0.007 us/op

The rebuild timing moved: upstream rebuilds lazily on the first selector read after a toast list change, while this PR rebuilds when the toast list is written. Toast lists are normally small in practice, so this is not a meaningful cost.

@atomiks atomiks added dependencies Update of dependencies. performance component: toast Changes related to the toast component. package: utils Specific to the utils package. labels May 6, 2026
@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented May 6, 2026

commit: 2bdaf4d

@code-infra-dashboard
Copy link
Copy Markdown

code-infra-dashboard Bot commented May 6, 2026

Bundle size

Bundle Parsed size Gzip size
@base-ui/react ▼-4.46KB(-0.95%) ▼-1.7KB(-1.14%)

Details of bundle changes

Performance

Total duration: 1,288.11 ms 🔺+67.25 ms(+5.5%) | Renders: 50 (+0) | Paint: 1,953.97 ms 🔺+95.19 ms(+5.1%)

Test Duration Renders
Slider mount (300 instances) 191.23 ms 🔺+19.70 ms(+11.5%) 3 (+0)
Dialog mount (300 instances) 65.51 ms 🔺+13.65 ms(+26.3%) 1 (+0)
Select mount (200 instances) 160.54 ms 🔺+13.03 ms(+8.8%) 3 (+0)
Scroll Area mount (300 instances) 97.31 ms 🔺+9.98 ms(+11.4%) 3 (+0)
Tabs mount (200 instances) 224.32 ms 🔺+8.40 ms(+3.9%) 4 (+0)

…and 7 more — details


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

@netlify
Copy link
Copy Markdown

netlify Bot commented May 6, 2026

Deploy Preview for base-ui ready!

Name Link
🔨 Latest commit 2bdaf4d
🔍 Latest deploy log https://app.netlify.com/projects/base-ui/deploys/6a01846ffe7dfa00081f9c34
😎 Deploy Preview https://deploy-preview-4751--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.

@atomiks atomiks force-pushed the codex/remove-toast-reselect branch 4 times, most recently from 5889cae to 0f0046a Compare May 6, 2026 09:42
@atomiks atomiks marked this pull request as ready for review May 6, 2026 09:49
@atomiks atomiks requested a review from Copilot May 6, 2026 09:51
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR removes Toast’s dependency on a memoized selector (createSelectorMemoized / reselect) by precomputing and storing ID-indexed toast metadata in the ToastStore state whenever the toast list changes, keeping lookups fast without selector-level memoization.

Changes:

  • Removed createSelectorMemoized (and the reselect dependency) from @base-ui/utils/store.
  • Added toastMetadata (a Map) to ToastStore state and recompute it whenever toasts changes.
  • Added a Vitest test to ensure toast metadata stays synchronized across common toast mutations.

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
pnpm-lock.yaml Removes reselect from the utils importer section (but needs full lock regen).
packages/utils/src/store/index.ts Stops exporting createSelectorMemoized.
packages/utils/src/store/createSelectorMemoized.ts Deletes the memoized selector implementation that depended on reselect.
packages/utils/src/store/createSelector.ts Drops reselect type import and replaces it with a local selector type.
packages/utils/package.json Removes reselect from dependencies.
packages/react/src/toast/store.ts Stores toastMetadata in state and updates it whenever toasts mutate; updates selectors to read from metadata.
packages/react/src/toast/store.test.ts Adds coverage for metadata synchronization after toast mutations.
Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pnpm-lock.yaml
Comment on lines 541 to 546
'@floating-ui/utils':
specifier: ^0.2.11
version: 0.2.11
reselect:
specifier: ^5.1.1
version: 5.1.1
use-sync-external-store:
specifier: ^1.6.0
version: 1.6.0(react@19.2.5)
Copy link
Copy Markdown
Member

@flaviendelangle flaviendelangle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are using createSelectorMemoized heavily in advanced components (MUI X and Base), what is the proposed alternative here?

@atomiks
Copy link
Copy Markdown
Contributor Author

atomiks commented May 6, 2026

We are using createSelectorMemoized heavily in advanced components

If it's still needed externally, we can leave it in the package. But it wasn't necessary for Toast or any other component and adds needless bundle size, so it feels like a waste to have the reselect dep in our package.json.

@flaviendelangle
Copy link
Copy Markdown
Member

flaviendelangle commented May 6, 2026

I think we should leave it in the package, because it's the only centralized place we have to easily import it from everything without duplicating, even if it means keeping reselect as a dep.

For Toast specifically, if we can have the same performances without the bundle size cost, great 👍

@atomiks atomiks force-pushed the codex/remove-toast-reselect branch from 0f0046a to 60f4638 Compare May 6, 2026 10:05
@atomiks atomiks removed dependencies Update of dependencies. package: utils Specific to the utils package. labels May 6, 2026
@atomiks atomiks requested a review from flaviendelangle May 6, 2026 10:15
@romgrk
Copy link
Copy Markdown
Contributor

romgrk commented May 7, 2026

@atomiks Please keep in mind that @base-ui/utils, despite the name, is not utils just for BUI — it's utils that are used organization-wide in all our repositories. It's placed here because it needs to be in a repository, not because it's owned by BUI. For any planned changes to the Store (or to the utils), you can check in with me if you want to know if/how something is used elsewhere, I'm the code owner for that package because I have a good overview of how it's used by the rest of the org.

@atomiks
Copy link
Copy Markdown
Contributor Author

atomiks commented May 8, 2026

@romgrk I've kept the deps and memoized module after @flaviendelangle's review. I wasn't sure if it was being used elsewhere previously, but now I know

I'm the code owner for that package because I have a good overview of how it's used by the rest of the org.

Looks like you were marked as a code reviewer here automatically. Since there are no changes to the utils package anymore, there shouldn't be any issue there

@romgrk
Copy link
Copy Markdown
Contributor

romgrk commented May 8, 2026

👍

And if there are BUI-specific utils, they should actually go in @base-ui/react, not in @base-ui/utils which should be reserved for general-purpose utils.

@atomiks
Copy link
Copy Markdown
Contributor Author

atomiks commented May 11, 2026

Code Review (GPT-5.5 + Opus 4.7)

Approve ✅ because the full Toast store diff keeps metadata synchronized across every affected mutation path, preserves behavior, and the test/typecheck evidence plus local benchmarking support the eager metadata tradeoff.

1. Bugs / Issues (None)

Behavior Preservation Assessment

Checked constructor initialization, addToast, duplicate-id upsert, replacing an ending toast, updateToastInternal, single-toast close, close-all, removeToast, promise updates, limit recalculation, focus-management lookups, and the Toast.Root / Toast.Positioner selector consumers.

The new toastMetadata map carries the same value, domIndex, visibleIndex, and offsetY data as the previous memoized selector. The change is timing: metadata is rebuilt when the toast list is written instead of lazily on first selector read.

Performance Assessment

I reran a local Node microbenchmark against freshly fetched upstream/master (7a6cf63b1) and the PR head (2bdaf4d3e). Values below are median microseconds per operation from 9 rounds, with the store-construction baseline subtracted for non-construction scenarios. Because this refactor intentionally moves work from selector reads to toast-list writes, I measured both write-only and write-plus-first-read paths.

Scenario 5 toasts 100 toasts Takeaway
createStore with preloaded toasts master 1.49 µs / PR 1.61 µs master 1.52 µs / PR 4.61 µs Slower when constructed with existing toasts; mostly synthetic for public Toast.Provider, which initializes with toasts: [].
addToast without an id, write only master 0.33 µs / PR 0.64 µs master 0.96 µs / PR 4.38 µs Slower on the PR because metadata is rebuilt eagerly before render reads.
addToast without an id + first selector reads master 1.93 µs / PR 0.64 µs master 6.59 µs / PR 4.42 µs Faster once the render/positioning read path is included.
addToast with an id + first selector reads master 2.18 µs / PR 0.43 µs master 11.08 µs / PR 4.17 µs Faster for the ID lookup/upsert path.
updateToastInternal on a cold selector state master 1.45 µs / PR 0.32 µs master 5.71 µs / PR 3.76 µs Faster in the cold-selector path.
closeToast on a cold selector state master 1.48 µs / PR 0.31 µs master 6.67 µs / PR 4.70 µs Faster in the cold-selector path.
first 4 selector reads master 1.33 µs / PR 0.04 µs master 3.62 µs / PR 0.04 µs Much faster because selectors now read through direct Map.get() calls.

The code confirms the underlying tradeoff: toast-list writes now rebuild metadata eagerly, while render/positioning selectors read from a direct Map.get() path. The one real write-only regression above is expected from moving the work earlier; the common write-plus-read paths still look favorable for Toast usage.

Simplification and DRY Assessment

This is a clean simplification. createToastMetadata is a small pure function, and toast selectors now use direct Map.get() lookups without the createSelectorMemoized / reselect layer. createSelectorMemoized and reselect remain available in utils for other consumers, so this does not broaden the change beyond Toast.

Repo Conventions / Cleanliness Assessment

The new store.test.ts follows the local Vitest style and directly exercises the metadata invariant. I did not find a repo-convention issue in the touched files.

Test Coverage Assessment

Validated with pnpm typescript, pnpm test:jsdom toast --no-watch, pnpm test:chromium toast --no-watch, targeted eslint, and git diff --check. The added store test covers metadata sync through update, close/ending transition, remove, and add; existing Toast tests cover duplicate-id, limit, close-all, promise, viewport, root, and positioner paths.

@atomiks atomiks merged commit ea10b8f into mui:master May 11, 2026
23 checks passed
@atomiks atomiks deleted the codex/remove-toast-reselect branch May 11, 2026 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: toast Changes related to the toast component. performance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants