[composite] Fix CompositeList not updating item order on reordering #2675

chuganzy · 2025-09-07T07:01:31Z

This PR fixes a bug in CompositeList where the internal item order was not updated when the list items were reordered ~~in React versions earlier than 19~~ (this was also the case in React 19 production build) and as a result, arrow key navigation could behave incorrectly.

After doing some tests, it turned out React 19 remounts items accordingly and this is not an issue.

Sample project to reproduce that issue: https://codesandbox.io/p/sandbox/qfsgqw

This PR fixes the issue by making use of MutationObserver ~~if the React version is <19~~.

I have followed (at least) the PR section of the contributing guide.

pkg-pr-new · 2025-09-07T07:02:41Z

Open in StackBlitz

pnpm add https://pkg.pr.new/mui/base-ui/@base-ui-components/react@2675

pnpm add https://pkg.pr.new/mui/base-ui/@base-ui-components/utils@2675

commit: 9909a8d

netlify · 2025-09-07T07:04:15Z

✅ Deploy Preview for base-ui ready!

Name	Link
🔨 Latest commit	`9909a8d`
🔍 Latest deploy log	https://app.netlify.com/projects/base-ui/deploys/68c18c1e1ddd370008bb29d1
😎 Deploy Preview	https://deploy-preview-2675--base-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

chuganzy · 2025-09-07T15:05:43Z

packages/react/src/composite/root/CompositeRoot.test.tsx

+      act(() => item1.focus());
+      await user.keyboard('{ArrowDown}');
+      expect(item3).toHaveFocus();
+    });


This fails on the main branch when running the test after doing node scripts/useReactVersion.mjs ^18

chuganzy · 2025-09-07T16:39:39Z

packages/react/src/composite/list/CompositeList.tsx


  const map = useRefWithInit(createMap<Metadata>).current;
-  const [mapTick, setMapTick] = React.useState(0);
+  const [mapTick, setMapTick] = React.useState({});


Update to use just empty object instead because there is a (very low) risk of number overflow.

There's a static empty obj that could be used: https://github.com/mui/base-ui/blob/master/packages/react/src/utils/constants.ts#L4

This should be reverted, there is no realistic risk of number overflow. Number.MAX_SAFE_INTEGER is the upper limit for integers. Even with 1,000 updates per second, it would take 285 years before it causes problems.

Thanks for the feedback!
You're right that the risk of number overflow is practically negligible. I only meant to highlight that "negligible" isn't quite the same as "impossible", but my main motivation here was consistency with other parts of the codebase that use the object pattern, such as the one used in useForceRerendering.

That said, I don't feel strongly about it, and I'm happy to revert this change if the project prefers to keep the counter-based approach here when I have a chance.
Would you prefer I update this line back, or align the other occurrences instead? Thanks!

I would switch this line back to a number. And tbh number overflow is not possible with JS numbers, the issue would be hitting the double precision limit which would render the + 1 operation void. Number overflow would actually not be an issue, it would be a good thing. We could wrap on the int32 range by casting via >>> 0 to get number overflow.

Sorry, I was not very clear above. I'm aware of that behavior and understand that +1 can become ineffective (void) in JS. My point was that, this could cause Object.is in React to return true, preventing re-renders and memo updates.

That said, as you mentioned, this risk is extremely low, and I'm cool with reverting this change. When we do that, I'd like to leave a comment to prevent the same change from happening again since this change has passed the review, and I'll also need to make sure to mention it in the PR. To clarify, when explaining why the counter approach is used, is it sufficient to mention it for memory efficiency (GC)? Thanks again!

We can use setMapTick(t => (t + 1) >>> 0) to wrap around if we're really worried about hitting the precision limit, and yes we can mention memory efficiency.

chuganzy · 2025-09-07T16:40:29Z

packages/react/src/composite/list/CompositeList.tsx

      if (labelsRef && labelsRef.current.length !== sortedMap.size) {
        labelsRef.current.length = sortedMap.size;
      }
+      nextIndexRef.current = sortedMap.size;


Found nextIndexRef only keeps incrementing.

@chuganzy I found this line causes a bug in Autocomplete + autoHighlight where the scroll on the page jumps on the second+ open.

Did this actually cause a bug or can it be removed?

@atomiks My bad.. Yes, this line can be isolated and reverted! 🙏

Nevermind, it just uncovered a bug actually. #2815

mui-bot · 2025-09-08T02:40:18Z

Bundle size report

Bundle	Parsed size	Gzip size
@base-ui-components/react	🔺+381B^(+0.11%)	🔺+138B^(+0.13%)

Details of bundle changes

mj12albert · 2025-09-08T02:51:34Z

Tested the fix and it works: https://codesandbox.io/p/devbox/2675-forked-68z63n

atomiks · 2025-09-08T05:24:47Z

After doing some tests, it turned out React 19 remounts items accordingly and this is not an issue.

Curious if this only happens in React 18 dev mode? See: #2565

We also want to make sure performance doesn't degrade for composite list components if possible:

chuganzy · 2025-09-08T07:45:07Z

@atomiks

Curious if this only happens in React 18 dev mode?

Actually, this was also the case in React 19..
React 19 dev mode (StrictMode) is the only one works fine, but production mode failed..

Added d04751a to remove the version check..

Regarding the performance, I will look into it deeper later!

chuganzy · 2025-09-08T15:29:26Z

Quick summary of performance check:

In the "Without manually specified index" cases of Select and Combobox, when elements change (on open / on filter), there is additional O(n) operation to start the MutationObserver. In dev, this takes less than 300μs even with 1,000 items, which is negligible.
With this change, even though the MutationObserver fires on open / close, it does not require the element order to be re-calculated in the above cases.

Given the above, in my opinion these results are acceptable, but what do you think?

atomiks

Thanks for the contribution! Performance seems unaffected, and the fix works correctly

Discussion: mui#2675 (comment)

chuganzy force-pushed the composite-reorder branch from 59d131c to 4696313 Compare September 7, 2025 14:35

chuganzy changed the title ~~[Composites] Improve CompositeList to update the sortedMap on reorder~~ [Composites] Fix CompositeList not updating item order on reordering in React <19 Sep 7, 2025

chuganzy force-pushed the composite-reorder branch from 3d2854a to 5e1d3ef Compare September 7, 2025 15:03

chuganzy commented Sep 7, 2025

View reviewed changes

chuganzy marked this pull request as ready for review September 7, 2025 15:06

chuganzy requested review from atomiks, colmtuite and michaldudak as code owners September 7, 2025 15:06

chuganzy marked this pull request as draft September 7, 2025 16:20

chuganzy added 2 commits September 8, 2025 01:36

[composites] Support dom reorder by making use of MutationObserver

21e3009

add a test case to verify the order update

52401e3

chuganzy force-pushed the composite-reorder branch from 5e1d3ef to 52401e3 Compare September 7, 2025 16:36

chuganzy commented Sep 7, 2025

View reviewed changes

chuganzy marked this pull request as ready for review September 7, 2025 16:40

mj12albert requested a review from romgrk September 8, 2025 02:49

Make use of EMPTY_OBJECT

ee29e43

Get rid of React version check

d04751a

chuganzy changed the title ~~[Composites] Fix CompositeList not updating item order on reordering in React <19~~ [Composites] Fix CompositeList not updating item order on reordering Sep 8, 2025

chuganzy added 2 commits September 8, 2025 22:42

Skip MutationObserver when the map is empty

8a7c628

Improve the performance

e6806da

chuganzy force-pushed the composite-reorder branch from 51257dd to e6806da Compare September 8, 2025 15:27

atomiks approved these changes Sep 10, 2025

View reviewed changes

zannager added the component: composite label Sep 10, 2025

Merge branch 'master' into composite-reorder

9909a8d

atomiks merged commit 3dc22fe into mui:master Sep 11, 2025
19 checks passed

chuganzy deleted the composite-reorder branch September 11, 2025 14:05

oliviertassinari changed the title ~~[Composites] Fix CompositeList not updating item order on reordering~~ [composite] Fix CompositeList not updating item order on reordering Sep 21, 2025

atomiks mentioned this pull request Sep 23, 2025

[useListNavigation] Fix forceSyncFocus ref #2815

Merged

brijeshb42 mentioned this pull request Sep 24, 2025

[code-infra] Fix changelog generation script #2820

Closed

1 task

chuganzy added a commit to chuganzy/base-ui that referenced this pull request Oct 1, 2025

Rollback to use counter-based update

5a7b17e

Discussion: mui#2675 (comment)

chuganzy mentioned this pull request Oct 1, 2025

[composite] Rollback CompositeList to use counter-based update #2880

Merged

1 task

Uh oh!

[composite] Fix CompositeList not updating item order on reordering #2675

[composite] Fix CompositeList not updating item order on reordering #2675

Uh oh!

Conversation

chuganzy commented Sep 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pkg-pr-new bot commented Sep 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Sep 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for base-ui ready!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chuganzy Sep 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chuganzy Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chuganzy Sep 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chuganzy Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mui-bot commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bundle size report

Uh oh!

mj12albert commented Sep 8, 2025

Uh oh!

atomiks commented Sep 8, 2025

Uh oh!

chuganzy commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chuganzy commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atomiks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

chuganzy commented Sep 7, 2025 •

edited

Loading

pkg-pr-new bot commented Sep 7, 2025 •

edited

Loading

netlify bot commented Sep 7, 2025 •

edited

Loading

chuganzy Sep 27, 2025 •

edited

Loading

chuganzy Sep 29, 2025 •

edited

Loading

chuganzy Sep 7, 2025 •

edited

Loading

chuganzy Sep 23, 2025 •

edited

Loading

mui-bot commented Sep 8, 2025 •

edited

Loading

chuganzy commented Sep 8, 2025 •

edited

Loading

chuganzy commented Sep 8, 2025 •

edited

Loading