Skip to content

refactor: share selection logic between Select and Multiselect - #5179

Open
amanabiy wants to merge 5 commits into
mainfrom
refactor/add-listbox-selection-models
Open

amanabiy wants to merge 5 commits into
mainfrom
refactor/add-listbox-selection-models

Conversation

@amanabiy

@amanabiy amanabiy commented Oct 22, 2026 •

Copy link
Copy Markdown
Member

Description

Stacked on #5178 (merged).

Why: We're adding the OptionList component. It needs the same list behaviour as Select and Multiselect, but that code lives in Select's private folders. Instead of OptionList depending on Select's internals, the three components will share one internal list engine in src/internal/components/listbox/. We're getting there in small steps that are easy to review and revert:

  1. refactor: Move list rendering and helpers into internal listbox #5178 Move the shared list helpers out of src/select (no behaviour change)
  2. refactor: share selection logic between Select and Multiselect #5179 Add shared selection models and use them in Select and Multiselect ← this PR
  3. feat: Add OptionList component #5180 Add OptionList on the new engine (Select and Multiselect unchanged)
  4. Later: move Select, then Multiselect, onto the engine, and remove the old Select-only code

What: One place for the selection rules, instead of copies in each component.

  • New: singleSelection and multiSelection in src/internal/components/listbox/selection/. Given the current selection, they return the next selection when the user picks an option, and the checkbox state of group headers and select-all.
  • Multiselect: option toggle, group toggle and select-all now come from multiSelection (removes about 40 lines of its own toggle logic).
  • Select and Multiselect: group header and select-all checkbox state in useSelect now comes from the model, replacing the copy of the same formula.

No behaviour change.

Related links, issue #, if available: n/a

How has this been tested?

  • Existing Select, Multiselect and PropertyFilter unit and integration tests pass unchanged.
  • New tests for the models, the order of options in onChange, and group and select-all checkbox state in Select and Multiselect. The new Multiselect order test passes on the old code too.
Review checklist

The following items are to be evaluated by the author(s) and the reviewer(s).

Correctness

  • Changes include appropriate documentation updates.
  • Changes are backward-compatible if not indicated, see CONTRIBUTING.md.
  • Changes do not include unsupported browser features, see CONTRIBUTING.md.
  • Changes were manually tested for accessibility, see accessibility guidelines.

Security

Testing

  • Changes are covered with new/existing unit tests?
  • Changes are covered with new/existing integration tests?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@amanabiy
amanabiy requested a review from a team as a code owner October 22, 2026 11:56
@amanabiy
amanabiy requested review from pan-kot and removed request for a team October 22, 2026 11:56
@amanabiy
amanabiy marked this pull request as draft October 22, 2026 11:58
@amanabiy
amanabiy requested a balanced review from Copilot October 22, 2026 15:16
@amanabiy
amanabiy added this pull request to stack #5183 October 22, 2026 15:17

Copilot AI left a comment

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.

🟢 Approval recommended

The isolated models have focused unit coverage, introduce no production callers, and have no identified blocking issues.

No open issues 🎉

What changed in this PR

Adds shared internal listbox selection models for the upcoming OptionList component and later Select/Multiselect migration. This builds on #5178 without changing existing component callers or runtime behavior.

Changes:

  • Defines a shared selection-model interface.
  • Adds single- and multi-selection logic, including group and select-all state.
  • Adds focused unit tests for toggling, filtering, disabled options, and selection edge cases.
File Description
src/​internal/​components/​listbox/​selection/​single-selection.ts Implements single-selection behavior and group state.
src/​internal/​components/​listbox/​selection/​multi-selection.ts Implements option, group, and select-all toggling.
src/​internal/​components/​listbox/​selection/​interfaces.ts Defines the shared selection contract.
src/​internal/​components/​listbox/​selection/​__tests__/​single-selection.test.ts Tests single-selection behavior and edge cases.
src/​internal/​components/​listbox/​selection/​__tests__/​multi-selection.test.ts Tests multi-selection, filtering, and disabled-option handling.

Review effort: Balanced

@codecov

codecov Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.73%. Comparing base (b3106e4) to head (228673d).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5179   +/-   ##
=======================================
  Coverage   97.73%   97.73%           
=======================================
  Files         996      998    +2     
  Lines       32222    32236   +14     
  Branches    11909    11909           
=======================================
+ Hits        31493    31507   +14     
  Misses        684      684           
  Partials       45       45           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Add single and multi selection models for the shared listbox under
src/internal/components/listbox/selection. Nothing calls them yet:
OptionList adopts them in PR 3, Select and Multiselect in PRs 4 and 5.
The models use DropdownOption; there is no type rename in this stack.

The models match today's use-select / use-multiselect behaviour for
re-select, group state (including the empty-value quirk), toggle order,
disabled options, group toggle, select-all state and toggle, and token
dismiss. Differences and caller contracts:

- The models do not gate input. Callers must keep the isInteractive /
  isGroupInteractive checks and read-only handling in front of them.
- singleSelection uses only the first selected option (Select passes at
  most one, so this is not observable).
- Group toggle and select-all read OptionDefinition.disabled, not the
  wrapper disabled set on children of a disabled group (same as today).
- allOptions defaults to options. Filtering callers must pass the full
  list, or select-all state and select-all on use only visible options.
- Row-level selected matching stays with the caller.
Multiselect's toggle, group toggle and select-all now go through multiSelection. Behaviour is unchanged, proven by the unchanged Multiselect unit and integration tests. The caller still gates disabled, read-only and non-interactive options before calling getNextSelection.
useSelect accepts an optional selection model. Multiselect passes it, so
group and select-all state come from the model. Select is unchanged and
keeps its own path until it moves to the engine.

Tidy-ups: replace the stale disabled-option comment in useMultiselect
with a pointer to the model, and make updateSelectedOption a plain
function (its useCallback never memoized because the model is rebuilt
every render). The isAllSelected / isSomeSelected props are removed; no
caller needs them now that the model provides select-all state.
Select now passes singleSelection(selectedOptions) to useSelect, so group
state for both Select and Multiselect comes from the selection models.

useSelect's own group-state formula (getOptionGroupState and its values
set) is removed. The selection prop stays optional and defaults to
singleSelection(selectedOptions), because use-select.test.ts calls the
hook without it. The default is the same model Select passes, so there
is one source of group state.

Behaviour is unchanged: existing tests pass unmodified, and the new
group-state tests pass on both the old and new code.
- Remove history/migration wording from selection model comments
- Inline the getGroupState wrapper in use-select and drop the unused GroupState import
- Remove a redundant undefined check in singleSelection.getGroupState
- Drop test comments that restate the test name
- Assert a matching render exists in the multiselect lastRenderedItem helper
- Pass an optional selection model in the select group-state test helper and rename the default-model test

No behaviour change.

Copilot AI left a comment

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.

🟢 Approval recommended

Static review supports behavior-preserving extraction with regression coverage and no blocking issues; tests were not executed during this review.

No open issues 🎉

Review effort: Balanced

@amanabiy amanabiy changed the title refactor: Add shared listbox selection models refactor: Share selection logic between Select and Multiselect Oct 26, 2026
@amanabiy amanabiy changed the title refactor: Share selection logic between Select and Multiselect refactor: share selection logic between Select and Multiselect Oct 26, 2026
@amanabiy
amanabiy enabled stacked auto-merge October 26, 2026 10:50

This branch was successfully deployed

3 active deployments
dev-pages-react16 — 228673d2 Deployed Oct 26, 2026 by amanabiy via deploy (React 16) / deploy #2961
dev-pages-react18 — 228673d2 Deployed Oct 26, 2026 by amanabiy via deploy (React 18) / deploy #2961
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants