[select] Remove redundant size check in SelectPositioner

[sai6855]

Remove redundant size check in SelectPositioner component as flush in CompositeList already handles this

[pkg-pr-new]

• base-ui-tanstack-start

• vite-css-base-ui-example

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

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

commit: 01ef857

[sai6855]

flush in CompositeList already handles this check and calls onMapChange when 2 maps change, so this check again in onMapChange is redundant and can introduce more bugs.

[sai6855]

@claude review

[github-actions]

PR review

The core change is sound: CompositeList.flush gates onMapChange on item identity/index/length, so dropping the map.size === prevSize short-circuit is what lets a same-count item replacement reconcile a stale value. Nothing here is merge-blocking, but the removal also widens a layout side effect that no test covers, the multiple branch it affects is untested, and the reorder test's waitFor is vacuous.

Bugs (2)

1. 🟡 The aligned-popup layout reset now fires on pure reorders, with nothing to recompute it

Location: packages/react/src/select/positioner/SelectPositioner.tsx:193

if (open && alignItemWithTriggerActive) {
  store.update({
    scrollUpArrowVisible: false,
    scrollDownArrowVisible: false,
  });

  const stylesToClear: React.CSSProperties = { height: '' };
  clearStyles(positionerElement, stylesToClear);
  clearStyles(popupRef.current, stylesToClear);
}

The removed guard protected all three blocks in onMapChange, not just value reconciliation. flush reports changed whenever an item's element or resolved index differs, so a keyed reorder (detected via the MutationObserver path in CompositeList.observe) now reaches this block too. alignItemWithTrigger defaults to true, and the height that gets cleared is set by the big layout effect in SelectPopup.tsx:343-349, whose deps (open, positionerElement, triggerElement, alignItemWithTriggerActive, isPositioned, listElement, …) do not change on a reorder — so nothing re-measures and the popup stays at its natural height with scroll arrows hidden until it closes.

Note this is browser-only: in jsdom all rects are 0, so fallbackToAlignPopupToTrigger flips alignItemWithTriggerActive off and the block never runs. That is why the new reorder test passes without noticing the change. I could not run the suite (PR head is data-only here), so this is reasoned from the code rather than observed.

Failure scenario: A default Select is open (aligned to the trigger, list scrolled with visible scroll arrows). The app reorders the options — e.g. a "most recently used" list re-sorts. The popup's computed height is cleared and both scroll arrows disappear, and neither is restored until the select is closed and reopened.

Fix: Keep a narrower guard for this block only — e.g. run the layout reset when the item set actually changed (map.size !== prevSize, or a set-membership comparison) rather than on every changed flush — or re-run the measurement after clearing. If reordering should reset the alignment, add a comment saying so, since the previous behaviour was deliberate.

2. ℹ️ Object values without isItemEqualToValue become easier to clear accidentally

Location: packages/react/src/select/positioner/SelectPositioner.tsx:162

const selectedValueIndex = findItemIndex(valuesRef.current, value, isItemEqualToValue);

With inline object values (<Select.Item value={{ id }}>), each render publishes a fresh identity into valuesRef while the controlled value still holds the object captured at selection time. Default comparison is reference equality, so findItemIndex returns -1 and the selection is reset. This footgun already existed for count-changing updates; removing the size check now also exposes it to reorders and same-count replacements. Not a defect in this diff, but worth knowing the blast radius grew — a doc/warning nudge toward isItemEqualToValue may be warranted.

Failure scenario: A select with inline object item values and a controlled value loses its selection when the option list is reordered while open.

Fix: No change required in this PR; optionally note the widened exposure in the PR description so it can be tracked separately.

Tests (3)

1. 🟠 The multiple reconciliation branch has no unchanged-count coverage

Location: packages/react/src/select/positioner/SelectPositioner.tsx:178

if (prevSize !== 0 && store.state.multiple && Array.isArray(value)) {
  const nextValue = value.filter(/* … */);

The removed guard short-circuited this branch identically to the single-select one, so multi-select is equally affected by the change. The only existing multi-select reconciliation test, removes selections that no longer exist (SelectRoot.test.tsx:5211), removes items and therefore changes the count — it would pass with or without this diff.

Failure scenario: multiple with value={['a', 'c']} and items ['a', 'b', 'c'] swapped to ['a', 'b', 'd']. Nothing pins that the selection becomes ['a'], so a future re-introduction of a size guard would silently regress multi-select.

Fix: Add a multiple variant alongside the new single-select tests asserting the filtered array after a same-count replacement.

2. 🟡 The reorder test's waitFor is already satisfied before the reorder is processed

Location: packages/react/src/select/root/SelectRoot.test.tsx:4463

fireEvent.click(screen.getByTestId('reorder'));

await waitFor(() => {
  expect(screen.getByRole('option', { name: 'c' })).toHaveAttribute('data-selected', '');
});

// The item set is unchanged, so nothing should have been reconciled away.
expect(onValueChange).not.toHaveBeenCalled();

c is already data-selected before the click, so the callback passes on its first synchronous invocation and waitFor returns immediately. The reorder is reconciled asynchronously — the MutationObserver in CompositeList.observe schedules the map update in a microtask — so expect(onValueChange).not.toHaveBeenCalled() can run before the flush that the test is meant to exercise, making a pass vacuous.

Failure scenario: If a future change made reordering reset the value, this test could still pass because it never waits for the map flush.

Fix: Wait on something the reorder actually changes, then assert. For example await waitFor(() => expect(screen.getAllByRole('option').map((o) => o.textContent)).toEqual(['c', 'a', 'b'])) plus await flushMicrotasks() (already imported) before the negative assertion.

3. 🟡 New tests use the outer real-timer renderer inside a fake-timer describe

Location: packages/react/src/select/root/SelectRoot.test.tsx:4315

const { user } = await render(<Test />);

The enclosing describe('dynamic items') block installs clock.withFakeTimers() (SelectRoot.test.tsx:4157-4163) and every sibling test renders through renderFakeTimers. All three new tests use the outer render instead, so user-event runs against a renderer that is not clock-aware while sinon fake timers are globally installed. shouldAdvanceTime: true probably keeps this working, but it is an avoidable flake source and diverges from the block's convention.

Failure scenario: A later change to clockOptions (dropping shouldAdvanceTime) hangs these three tests on user.click, with no obvious cause.

Fix: Use renderFakeTimers like the neighbouring tests, or move the new tests to a describe that does not install fake timers.

Simplifications (2)

1. 🟡 The first two new tests exercise the same mechanism

Location: packages/react/src/select/root/SelectRoot.test.tsx:4352

resets the value when the selected item is replaced and the item count is unchanged and resets the value when every item is replaced and the item count is unchanged are byte-for-byte identical apart from ['a', 'b', 'd'] vs ['x', 'y', 'z'], and both hit the same path: same-count flush, selected value no longer in valuesRef, reset to null. The second one adds ~60 lines of duplicated component boilerplate for no extra coverage.

Failure scenario: Two near-identical tests to keep in sync; any change to the shared Test shape must be applied twice for no additional signal.

Fix: Drop the second test, or repoint it at coverage that is actually missing — e.g. multiple mode (see Tests 1), or a replacement that falls back to defaultValue rather than null.

2. ℹ️ prevMapSizeRef no longer needs to hold a size

Location: packages/react/src/select/positioner/SelectPositioner.tsx:148

const prevMapSizeRef = React.useRef(0);
// …
const prevSize = prevMapSizeRef.current;
prevMapSizeRef.current = map.size;

After this change the stored number is only ever compared against 0, in two places, to mean "the list has been populated at least once". A boolean ref (hasRegisteredItemsRef) would say that directly and stop the name from implying a size comparison that no longer exists.

Failure scenario: The prevSize name invites a future reader to reinstate the size comparison this PR intentionally removes.

Fix: Rename to a boolean ref and check it once, keeping the layout-reset block's own condition explicit.

Docs (1)

1. 🟡 "Redundant" mischaracterises the change, and no comment records the new invariant

Location: packages/react/src/select/positioner/SelectPositioner.tsx:156

The PR title and description state the size check is redundant because flush in CompositeList "already handles this". flush compares item length, element identity, resolved index, explicit index, and metadata identity (CompositeList.tsx:147-158) — it does not dedupe by map size. The check was not redundant; removing it is a deliberate behaviour fix, which the type: bug label and the new regression tests confirm. Since the PR title becomes the squashed commit message, the history would misrepresent the fix, and the code carries no note explaining why a size guard must not come back.

Failure scenario: Someone bisecting a "selection cleared unexpectedly" report skips this commit because it reads as a no-op refactor; or someone re-adds the size check as an optimisation and silently reverts the fix.

Fix: Retitle along the lines of [select] Reset a stale value when items are replaced without a count change, and add a short comment above prevSize noting that CompositeList only invokes onMapChange when the item snapshot actually changed, so a size comparison would wrongly skip same-count replacements.

Verdict

Approve after nits — the fix is correct and well-motivated; the open items are an unasserted layout side effect on reorder, a missing multiple test, and a vacuous assertion in the reorder test.

---

🤖 Review generated with Claude Code · Opus 5 (High) · medium review depth · 38 turns · 8m0s · $2.42 · run

[code-infra-dashboard]

Bundle size

Bundle	Parsed size	Gzip size
@base-ui/react	▼-21B(0.00%)	▼-6B(0.00%)

Details of bundle changes

Performance

Total duration: 1,206.93 ms -62.14 ms^(-4.9%) | Renders: 76 ⁽⁺⁰⁾ | Paint: 1,890.35 ms -94.40 ms^(-4.8%)

Test	Duration	Renders
Checkbox mount (500 instances)	103.87 ms 🔺+23.38 ms(+29.0%)	1 (+0)
Tooltip mount (300 contained roots)	46.82 ms ▼-12.47 ms(-21.0%)	1 (+0)

13 tests within noise — details

---

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

[netlify]

✅ Deploy Preview for base-ui ready!

Name	Link
🔨 Latest commit	01ef857
🔍 Latest deploy log	https://app.netlify.com/projects/base-ui/deploys/6a7c2708650a3800086acd72
😎 Deploy Preview	https://deploy-preview-5469--base-ui.netlify.app
📱 Preview on mobile	Toggle 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.

[Copilot]

Pull request overview

This PR updates Select’s internal reconciliation behavior so that changes to the item set are handled even when the total item count stays the same, and adds regression tests to cover these scenarios.

Changes:

• Remove an early-return in SelectPositioner so onMapChange runs for same-size item replacements/reorders.
• Add tests ensuring the value resets when the selected item is replaced (or all items are replaced) without changing the item count.
• Add a test ensuring reordering items does not reset the value.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
packages/react/src/select/positioner/SelectPositioner.tsx	Removes the map-size guard so list mutations with unchanged count still trigger reconciliation.
packages/react/src/select/root/SelectRoot.test.tsx	Adds regression tests for value reconciliation when items are replaced vs reordered with unchanged item count.

Suppressed comments (1)

packages/react/src/select/positioner/SelectPositioner.tsx:163

• Now that the map.size === prevSize early-return is removed, onMapChange will run for replacements/reorders where the item count stays the same. In the open && alignItemWithTriggerActive branch later in this callback, the store is updated to set scrollUpArrowVisible/scrollDownArrowVisible to false but nothing in this callback recomputes their correct visibility (it’s normally derived from scroll geometry). This can cause scroll arrows to disappear after swapping/reordering items while the popup is open, until the user scrolls and triggers a recalculation.

      const prevSize = prevMapSizeRef.current;
      prevMapSizeRef.current = map.size;

      const eventDetails = createChangeEventDetails(REASONS.none);

      if (prevSize !== 0 && !store.state.multiple && value !== null) {
        const selectedValueIndex = findItemIndex(valuesRef.current, value, isItemEqualToValue);
        if (selectedValueIndex === -1) {

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[atomiks]

Verified the same-count reconciliation change on the current head, added multiple-select regression coverage, merged current master, and ran focused jsdom/Chromium plus formatting, lint, and type checks locally.