Skip to content

[internal] Update the hook value when store/arguments change - #4866

Merged
romgrk merged 2 commits into
mui:masterfrom
chuganzy:store-fix
May 20, 2026
Merged

[internal] Update the hook value when store/arguments change#4866
romgrk merged 2 commits into
mui:masterfrom
chuganzy:store-fix

Conversation

@chuganzy

@chuganzy chuganzy commented May 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

This PR fixes an issue where useStoreFast returns a stale value when selector arguments changes during render.
This was not an issue in all test cases and dev mode because of the StrictMode, but was causing issues in production build.

ex: MenuTrigger in the Menu hero demo does not update aria-expanded.
https://base-ui.com/react/components/menu

@pkg-pr-new

pkg-pr-new Bot commented May 20, 2026 •
edited
Loading

Copy link
Copy Markdown

commit: 7e6ccaa

@code-infra-dashboard

code-infra-dashboard Bot commented May 20, 2026 •
edited
Loading

Copy link
Copy Markdown

Bundle size

Bundle Parsed size Gzip size
@base-ui/react ▼-26B(-0.01%) ▼-13B(-0.01%)

Details of bundle changes

Performance

Total duration: 1,194.71 ms +54.22 ms(+4.8%) | Renders: 50 (+0) | Paint: 1,821.51 ms +75.65 ms(+4.3%)

No significant changes — details

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

@netlify

netlify Bot commented May 20, 2026 •
edited
Loading

Copy link
Copy Markdown

✅ Deploy Preview for base-ui ready!

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

@chuganzy
chuganzy force-pushed the store-fix branch 3 times, most recently from 21ba704 to 0f81b48 Compare May 20, 2026 07:46
@chuganzy
chuganzy marked this pull request as ready for review May 20, 2026 07:51
@chuganzy
chuganzy requested a review from romgrk as a code owner May 20, 2026 07:51
@chuganzy chuganzy changed the title [internal][fix] update the hook value when store/arguments change [internal] update the hook value when store/arguments change May 20, 2026
@chuganzy chuganzy changed the title [internal] update the hook value when store/arguments change [internal] Update the hook value when store/arguments change May 20, 2026
@chuganzy
chuganzy force-pushed the store-fix branch 2 times, most recently from 42ab8c8 to 3eb568a Compare May 20, 2026 09:47
@atomiks atomiks added component: menu Changes related to the menu component. type: bug It doesn't behave as expected. labels May 20, 2026
@chuganzy

chuganzy commented May 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@atomiks
Thanks for labeling! Unfortunately, this might affect all places that utilize fastComponent, not only menu. Although I have not confirmed any active issues in them, we might want to also add the "tooltip" and "preview-card" labels just in case.

@romgrk romgrk added package: utils Specific to the utils package. and removed component: menu Changes related to the menu component. labels May 20, 2026
romgrk
romgrk reviewed May 20, 2026
View reviewed changes
Comment thread packages/utils/src/store/useStore.ts
hook.a2 = a2;
hook.a3 = a3;
hook.didChange = true;
hook.value = selector(store.getSnapshot(), a1, a2, a3);

@romgrk romgrk May 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This was an optimization because we have complex memoized selectors (in the MUI X repository), so avoiding running the selectors is desirable. It's indeed a bug to avoid it here, and the behavior was relying on react updating in response to our getSnapshot() (instance.syncTick) changing.

I will investigate avoiding the double selector call (here and in getSnapshot()) by keeping a copy of the snapshot on the hook, but in a different PR. The gains may not be worth the bookkeeping cost for some/all components.

@romgrk
romgrk merged commit ea720e8 into mui:master May 20, 2026
23 checks passed
@romgrk

romgrk commented May 20, 2026

Copy link
Copy Markdown
Contributor

Thanks for the PR!

@chuganzy
chuganzy deleted the store-fix branch May 20, 2026 14:43
mbrookes pushed a commit to mbrookes/base-ui that referenced this pull request May 25, 2026
@chuganzy @romgrk @mbrookes
[internal] Update the hook value when store/arguments change (mui#4866)
4c54e48 
Co-authored-by: Rom Grk <romgrk@users.noreply.github.com>
This was referenced Jun 1, 2026
Merged
Closed
@aduth aduth mentioned this pull request Jun 2, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@romgrk romgrk romgrk approved these changes

Assignees

No one assigned

Labels

package: utils Specific to the utils package. type: bug It doesn't behave as expected.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@chuganzy @romgrk @atomiks