[select] Fix autofill and selected state edge cases#4934
Conversation
commit: |
Bundle size
PerformanceTotal duration: 1,095.84 ms -53.61 ms(-4.7%) | Renders: 50 (+0) | Paint: 1,674.76 ms -67.03 ms(-3.8%) No significant changes — details Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| // `selectedIndex` is only updated after the items mount for the first time, | ||
| // the value check avoids a re-render for the initially selected item. | ||
| if (state.selectedIndex === index && state.selectedIndex !== null) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Select open renders are unchanged (14) according to the benchmark and running the benchmark showed ~no difference
What the old code was trying to do: use selectedIndex as a fast path after items mount, while falling back to the value comparison before selectedIndex is initialized so the initially selected item doesn’t need a second render. The problem is that selectedIndex is not kept in sync while open, so it can become stale and override the actual controlled value.
flaviendelangle
left a comment
There was a problem hiding this comment.
PR #4934 Review Summary — [select] Fix autofill and selected state edge cases
Four review agents (code, comments, tests, silent failures) ran in parallel against branch pr-4934. Tests, lint, and types were verified clean by the code reviewer.
The fixes themselves are solid — every behavior change is correctly implemented and aligned with peer components. The main risk is test coverage: several behavioral changes ship under-tested, and one existing test may not actually exercise the new guard it appears to verify.
Critical (should fix before merge)
commitSelectiondisabled/readOnly test may not isolate the new guard —SelectRoot.test.tsx:872-904usesuser.click(...), butuseButton({disabled})already swallows the click upstream. Verify the test fails when you removeSelectItem.tsx:134-138; if not, add a path that reachescommitSelection(keyboard Enter on a highlighted item, ordefaultOpenvariant).- No coverage for
SelectScrollArrowvisible → mountedchange — the entire fix (and the newtransitionStatusMapping) is untested. At minimum add a jsdom smoke test that the arrow remains in the DOM through the exit tick; ideally a chromium test assertingdata-ending-style/data-transition-status="ending"during exit. - Initial-mount regression scenario for the removed
isSelectedshort-circuit is untested — add<Select.Root defaultOpen value="b">(and an object-value +isItemEqualToValuevariant) assertingoption bhasdata-selectedon first paint, without any user interaction. The deleted short-circuit existed precisely for that scenario. - Multi-select stale-index not covered — the line 168 assertion is single-select. Mirror it for
value={['a','b']} → ['b']while open.
Important (should address)
- Autofill matcher precedence is untested — the rewrite changed the closure but the test only covers "rendered label matches". Add tests that pin (a) value wins over rendered text of a later item, (b) case-insensitive matching of the new rendered-text path, (c) tolerance of
nullrendered labels. - Autofill success path with
useValueChanged— only the cancel path is tested. Add a Field-wrapped test that asserts a successful autofill producesdata-dirty=""and triggers avalidatespy. Pins thatsetDirty+validation.changestill run after the refactor. - Rewrite comment at
SelectRoot.tsx:584-586— currently restates the three matching paths (the WHAT). Suggested condensed form:// Browsers autofill the rendered text (e.g. "United States"), which is neither the // serialized value nor the serialized label, so match it against `labelsRef` too.
Suggestions
- Delete redundant test comments at
SelectRoot.test.tsx:167and:806— both restate what theit()title + assertion already convey. (If keeping :806, expand it to describe thecancel → useValueChanged-doesn't-firechain.) hasSelectedValue: add a coverage case using a non-string value + customitemToStringValuereturning''(the present test happens to also pass on the old code).- UX follow-up (out of scope): items in a forced-open
disabled/readOnlySelect have no visible disabled state oraria-disabled, so the new no-op click is invisible to users. Worth a separate PR cascading the lock state to items.
Strengths
- Each fix correctly delivers what its description claims (verified by code reviewer and silent-failure hunter independently).
isSelectedchange makesvaluethe single source of truth and correctly fixes the simultaneous-data-selectedbug when controlled value changes while the popup is open (syncSelectedIndexdefers whileopen).hasSelectedValuelocal definition correctly mirrorsselectors.hasSelectedValue— Field's filled state now agrees with the trigger placeholder semantics.- Removing the explicit
setDirty/validation.changeand deferring touseValueChangedis the right refactor — it fixes the canceled-autofill-still-dirty bug on master. SelectScrollArrowchanges align it with peer components (SelectItemIndicator,SelectPopup,SelectArrow,SelectBackdrop) which all already mergetransitionStatusMapping.- Comments at
SelectItem.tsx:134,SelectRoot.tsx:197,600, andstore.ts:100are good examples of non-obvious WHY documentation. - All AGENTS.md rules observed: no
as any, noReact.useLayoutEffect, nowindow.setTimeout, Vitest-only matchers, etc.
Recommended action
- Investigate finding #1 first (test isolation) — if confirmed, expand to also cover keyboard activation.
- Add the missing tests for findings #2, #3, #4 — all are short additions.
- Apply finding #7 (comment rewrite) and #8 (comment deletions).
- Re-run
pnpm test:jsdom SelectRoot --no-watchandpnpm test:chromium SelectRoot --no-watch.
Fixes several Select edge cases where displayed selection state, autofill matching, and locked interactions could drift from the current value.
Changes
CAcan accept browser-filled text likeCanada.data-selectedtied to the current value when controlled value changes while the popup is open.