Skip to content

[composite] Support controlled item indexes#5148

Open
jjenzz wants to merge 2 commits into
mui:masterfrom
jjenzz:codex/controlled-composite-indexes
Open

[composite] Support controlled item indexes#5148
jjenzz wants to merge 2 commits into
mui:masterfrom
jjenzz:codex/controlled-composite-indexes

Conversation

@jjenzz

@jjenzz jjenzz commented Jun 30, 2026

Copy link
Copy Markdown
Member

Fixes #4657

This updates useCompositeListItem so passing an explicit index makes the item index controlled without opting the item out of CompositeList registration.

Previously, items with an explicit index skipped registration entirely. In non-virtualized Combobox/Autocomplete lists, useListNavigation still relies on listRef to resolve pointer hover targets, so listRef.current.indexOf(event.currentTarget) could not find hovered items and data-highlighted stayed on the previous item.

The new behavior:

  • explicit index items still register with CompositeList
  • controlled-index lists skip DOM reorder observation
  • mixed controlled/uncontrolled indexes in one CompositeList report a development error

Added test coverage for:

  • mixed controlled/uncontrolled index warnings
  • Combobox hover highlighting with manually indexed items

@atomiks it looks like you've touched this file the most, so would really appreciate your eyes on this one 🙏 lmk if this could have been achieved more simply elsewhere.

BEFORE AFTER
CleanShot.2026-06-30.at.16.50.53.mp4
CleanShot.2026-06-30.at.16.53.00.mp4

@pkg-pr-new

pkg-pr-new Bot commented Jun 30, 2026

Copy link
Copy Markdown

commit: d5f6916

@code-infra-dashboard

code-infra-dashboard Bot commented Jun 30, 2026

Copy link
Copy Markdown

Bundle size

Bundle Parsed size Gzip size
@base-ui/react 🔺+645B(+0.14%) 🔺+334B(+0.22%)

Details of bundle changes

Performance

Total duration: 1,147.81 ms ▼-320.41 ms(-21.8%) | Renders: 76 (▼-2)

Test Duration Renders
Tabs mount (200 instances) 171.71 ms ▼-110.65 ms(-39.2%) 3 (▼-1)
Slider mount (300 instances) 98.46 ms ▼-106.23 ms(-51.9%) 2 (▼-1)
Checkbox mount (500 instances) 60.39 ms ▼-25.19 ms(-29.4%) 1 (+0)
Popover mount (300 instances) 63.40 ms ▼-17.43 ms(-21.6%) 1 (+0)
Select open (500 options) 47.36 ms ▼-13.75 ms(-22.5%) 14 (+0)

…and 2 more (+7 within noise) — details


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

@netlify

netlify Bot commented Jun 30, 2026

Copy link
Copy Markdown

Deploy Preview for base-ui ready!

Built without sensitive environment variables

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

@jjenzz jjenzz force-pushed the codex/controlled-composite-indexes branch 5 times, most recently from 62d636f to 26171d8 Compare June 30, 2026 17:05
Comment on lines +4 to +6
export type CompositeMetadata<CustomMetadata> = {
index?: number | null | undefined;
} & CustomMetadata;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

moved this here to avoid circular imports

@jjenzz jjenzz marked this pull request as draft June 30, 2026 17:11
@jjenzz jjenzz force-pushed the codex/controlled-composite-indexes branch 4 times, most recently from 195aff9 to 32f4977 Compare June 30, 2026 18:19
Comment thread packages/react/src/internals/composite/list/CompositeList.tsx Outdated
@github-actions github-actions Bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Jul 1, 2026
@jjenzz jjenzz force-pushed the codex/controlled-composite-indexes branch 2 times, most recently from c0ebe9f to b6f1165 Compare July 1, 2026 10:36
@github-actions github-actions Bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Jul 1, 2026
@jjenzz jjenzz force-pushed the codex/controlled-composite-indexes branch 5 times, most recently from 8bb53ad to 5b2fdbf Compare July 1, 2026 13:35
@jjenzz jjenzz force-pushed the codex/controlled-composite-indexes branch from 5b2fdbf to 6936144 Compare July 1, 2026 13:47
listeners.add(fn);
// Delayed items need the latest snapshot when they subscribe, otherwise
// they can stay on a guessed index until another list change happens.
fn(sortedMapRef.current);

@jjenzz jjenzz Jul 1, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The OTPField highlighted an edge case where an item could keep its guessed index because items subscribe to map changes after registering themselves, and this PR now produces the sorted map during registering (before the subscription).

AFAIUI, this wasn't a problem before because map listeners were notified after the mapTick state update, from within a layout effect tied to that state change, so index correction happened later in the render/effect cycle. The old callback ref writes were covering up that correction delay.

Now that the sorted map and refs are produced during registration,elementsRef/labelsRef are ready without waiting for that state/effect cycle. That means listener notification also happens earlier, so if a child subscribes after registering it wouldn't get the correction snapshot.

Long story short, we probably want to make sure we subscribe before we register, but this fix seems more correct/robust.

@jjenzz jjenzz force-pushed the codex/controlled-composite-indexes branch 2 times, most recently from 734dbf7 to 2d8d998 Compare July 1, 2026 14:27
@jjenzz jjenzz marked this pull request as ready for review July 1, 2026 14:36
@jjenzz jjenzz force-pushed the codex/controlled-composite-indexes branch from 2d8d998 to 20d103b Compare July 1, 2026 14:56
@atomiks

This comment was marked as outdated.

@jjenzz jjenzz force-pushed the codex/controlled-composite-indexes branch 3 times, most recently from 60e1af5 to 36f6a40 Compare July 2, 2026 12:24
Comment thread packages/react/src/internals/composite/list/useCompositeListItem.ts Outdated
Comment thread packages/react/src/internals/composite/list/CompositeList.tsx Outdated
Comment thread packages/react/src/internals/composite/list/CompositeList.tsx Outdated
Comment thread packages/react/src/internals/composite/list/CompositeList.tsx Outdated
Comment thread packages/react/src/internals/composite/list/CompositeList.tsx Outdated
@jjenzz jjenzz force-pushed the codex/controlled-composite-indexes branch 5 times, most recently from 275c005 to f5a1dd1 Compare July 2, 2026 17:59
@atomiks

atomiks commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Performance: measured, one real inefficiency found

Benchmarked CompositeList directly in headless Chromium (1000 items, flushSync mount/update, median of 12 rounds), on this PR's head vs master:

Scenario (1000 items) master PR #5148 PR + guard
Mount, uncontrolled 26.0 ms 26.7 ms 6.4 ms
Mount, controlled (index prop) 4.8 ms † 23.6 ms 4.1 ms
Replace half the items, uncontrolled 47.0 ms 47.6 ms 26.6 ms
Replace half the items, controlled 4.6 ms † 47.6 ms 5.6 ms
Filter 1000→500, controlled (all indexes reassigned) 2.4 ms † 36.6 ms 3.3 ms
Filter 1000→500, uncontrolled 23.7 ms 24.5 ms 12.3 ms

† master's controlled numbers are the skip-registration path — fast because it did nothing, which is exactly the bug being fixed.

Reading the middle column: the PR is perf-neutral for uncontrolled lists (the common case), but large controlled lists pay real new cost — a filter keystroke on a 1000-item indexed Combobox costs ~34 ms more than master, per keystroke.

The cost is not the sort — it's the state-update enqueue. Both register and unregister call setMapTick once per item, so a 1000-item commit enqueues 1000 updates that React coalesces into one render anyway. Guarding the enqueue on the dirty-flag transition eliminates it:

map.set(node, metadata);
if (!isDirtyRef.current) {
  isDirtyRef.current = true;
  setMapTick((tick) => tick + 1);
}

(and the same in unregister). This is safe by construction: every false→true dirty transition enqueues exactly one tick, and the only thing that clears the flag is flush(), which is guaranteed to run afterwards — either in the same commit's parent layout effect or in the tick-triggered render. With this guard, all 2679 tests across the eleven affected suites pass, and every scenario becomes faster than master — including the previously "free" controlled paths (4.1 vs 4.8 ms mount) and a 4× improvement on uncontrolled mounts, since master paid the same per-register enqueue tax.

Minimality assessment

The architecture is close to minimal for what it has to preserve — I looked for structural cuts and found none that survive scrutiny: the mapTick state is the only mechanism that gets CompositeList to re-run its flush effect after child registrations (a dep-less effect wouldn't fire without a re-render); sortedMapRef is needed so late subscribers and the MutationObserver path get a current snapshot without a render; isDirtyRef is what lets one flush serve n registrations. The head commit already removed the redundant tick from flush. Remaining trims are cosmetic:

  • sortMap's controlled comparator (a[1]?.index ?? 0) — metadata can no longer be null/undefined in the map (the type changed in this PR), so the optional chaining and fallback are dead defensiveness.
  • The ComboboxItem virtualized manual listRef effect now overlaps with registration-driven syncRefs for items that pass index (both write the same node into the same slot; the registration flush runs after the manual write in the same commit) — consolidating to a single writer is the one real simplification available.

The single actionable outcome of this pass is the setMapTick guard: a two-site, behavior-preserving change that turns the PR's only measurable regression into an across-the-board win over master.


🤖 Review generated with Claude Code

Comment thread packages/react/src/internals/composite/list/CompositeList.tsx Outdated
Comment thread packages/react/src/select/root/SelectRoot.test.tsx Outdated
@atomiks

atomiks commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Second pass: virtualized regressions

Follow-up hunt on the current head — the earlier findings (deep-subtree registration, per-register tick cost, label: null, vacuous warning tests) are all resolved now. This pass focused on the newly-changed virtualized path, since removing the if (virtualized) return element early-return means virtualized lists mount a CompositeList for the first time. Found one merge-blocking regression there plus a few smaller things.

1. 🔴 Virtualized: listRef.current.length is truncated to the rendered window, breaking keyboard bounds

On master, virtualized mode renders no CompositeList; items write store.state.listRef.current[index] = el directly (and delete on cleanup never shrinks length), while AriaCombobox maintains the invariant listRef.current.length = flatFilteredItems.length in a layout effect keyed on [items, flatFilteredItems].

With this PR, every flush runs syncRefs, which does elementsRef.current = [] and sparse-fills only the currently rendered window — the array is replaced with one whose length is maxRenderedIndex + 1, not the filtered total. The AriaCombobox fixup effect doesn't re-run (its deps haven't changed), and even when it does run in the same commit, the next window shift (scroll or key-repeat arrowing → items re-register with a new index → flush in the tick-triggered render) clobbers it again with no counter-fixup.

useListNavigation computes bounds from that length on every keydown (getMaxListIndexlength - 1; the combobox passes disabledIndices: EMPTY_ARRAY, so holes aren't skipped):

  • loopFocus (default true) + allowEscape (true whenever autoHighlight is off): at currentIndex >= maxIndex the highlight escapes to -1 in the middle of the list — reachable with key-repeat ArrowDown, which outruns the virtualizer's scroll-triggered re-window.
  • With autoHighlight (no escape): wraps to the top mid-list instead of advancing.
  • Trigger-based (non-typeable) virtualized combobox: End jumps to the last rendered item, not the last filtered item.

The new test registers items at their filtered indexes when the rendered window starts after index 0 actually asserts the broken invariant: snapshot '||three|four' is length 4 while flatFilteredItems.length is 5 — pre-PR that snapshot would be '||three|four|'. JSDOM tests can't see the fallout because they don't exercise virtualizer window shifts.

Fix direction: either syncRefs shouldn't shrink/replace the array for controlled lists (mutate in place, never reduce length, leaving total-length ownership with AriaCombobox), or ComboboxList should re-assert listRef.current.length = flatFilteredItems.length after each flush (it already has filteredItems in scope and could pass an onMapChange).

2. 🟡 Virtualized items missing from the filter register with controlled index -1

ComboboxItem.tsx:

const index = indexProp ?? (virtualized ? (indexFromFilter ?? -1) : undefined);

findItemIndex returns -1 (never undefined) when a rendered item isn't in flatFilteredItems — a normal transient state while a virtualizer's window lags a keystroke, so the ?? -1 arm is dead and -1 flows through as a controlled index. On master these items were excluded (hasRegistered gated the manual write). Now they land in the sorted map (sorted first), and syncRefs executes elementsRef.current[-1] = node / labelsRef.current[-1] = … — junk string-keyed properties holding node references. If a whole stale window misses the filter, that flush produces an essentially empty listRef (length 0) for the commit — which also feeds regression 1. A metadata.index >= 0 guard in syncRefs (or skipping registration of -1 at the call site) closes it.

3. 🟡 MutationObserver is never re-created after an observer-triggered flush (uncontrolled lists)

On master, the MO callback called setMapTick → re-render → new sortedMap → the observer effect re-ran and re-observed the current parents. Now the MO callback calls flush() directly, which updates sortedMapRef but never bumps mapTick, and the observer effect only re-runs on mapTick. After a reorder that moves a node to a different parent (DnD libraries, manual DOM re-parenting — exactly the no-React-commit scenario this observer exists for), the new parent is never observed, so subsequent reorders under it go undetected until an unrelated register/unregister. Edge-case, but a strict capability loss vs master's re-observe-every-flush.

4. ℹ️ Stale type on subscribeMapChange

CompositeListContextValue.subscribeMapChange is still typed (map: Map<Element, Metadata | null>) => void, but listeners now always receive Map<Element, CompositeMetadata<Metadata>> — values are never null after this PR, and index/labelRef are what the subscriber in useCompositeListItem actually reads. Cosmetic, but it hides type errors in listener code.

Checked and clear

  • Flush timing (refs filled in CompositeList's layout effect instead of item ref callbacks): every descendant reader checks out — SelectPopup's listRef.current[0] read waits on isPositioned (a post-flush commit), useCompositeRoot.onMapChange runs inside the flush, and event handlers run long after.
  • onMapChange consumers (Tabs, Toolbar, Slider, OTPField, SelectPositioner): all size-guarded or idempotent against the new call pattern (no more initial empty-map call; unregister-to-empty still flushes).
  • Mode transitions: controlledRef resets correctly when the map empties (cleanups run before creates within a commit), and StrictMode double-invoke is safe.
  • Label semantics: label={null} typeahead-exclusion survives, and label-change-without-reregister works via the labelRef indirection.
  • Registration churn: the register effect's deps (metadata, externalIndex) have the same identity requirements as master — no new infinite-render risk.

No. 1 is the one I'd block on — it likely decides what syncRefs's ownership contract over elementsRef should be.

@jjenzz jjenzz force-pushed the codex/controlled-composite-indexes branch 2 times, most recently from 27c41a1 to 76da89c Compare July 3, 2026 15:18
@jjenzz jjenzz force-pushed the codex/controlled-composite-indexes branch from 76da89c to d028a8d Compare July 3, 2026 17:38
@jjenzz jjenzz force-pushed the codex/controlled-composite-indexes branch from d028a8d to d5f6916 Compare July 3, 2026 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: composite type: bug It doesn't behave as expected.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[combobox][autocomplete] Combobox.Item/Autocomplete.Item index breaks hover state

2 participants