feat(select): added virtual scroll #552 - #553
Conversation
📝 WalkthroughWalkthroughChangesThe select component now supports virtualized option rendering for flat menu dropdowns through Select virtual scrolling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SelectComponent
participant CdkVirtualScrollViewport
User->>SelectComponent: Open virtualized menu
SelectComponent->>CdkVirtualScrollViewport: Render filtered virtual rows
User->>SelectComponent: Navigate or activate option
SelectComponent->>CdkVirtualScrollViewport: Scroll active row into view
SelectComponent-->>User: Update selection and aria-activedescendant
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Stylelint (17.14.0)tedi/components/form/select/select.component.scssConfigurationError: Could not find "stylelint-config-recess-order". Do you need to install the package or use the "configBasedir" option? Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
tedi/components/form/select/select.component.spec.ts (1)
2667-2673: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
setInput()and exercise automatic row measurement.Expose the test-host fields as signal inputs, update them through
fixture.componentRef.setInput(), and leavevirtualItemSizeunset in one test that verifies the measured-height path.As per coding guidelines, “Tests must use
fixture.componentRef.setInput('propName', value)to provide component inputs.” As per path instructions, tests should use a test host plusfixture.componentRef.setInput()for signal inputs.Also applies to: 2681-2685, 2751-2758, 2801-2802, 2824-2826, 2846-2847
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tedi/components/form/select/select.component.spec.ts` around lines 2667 - 2673, Update the select test host fields used by the affected virtual-scroll tests to signal inputs, and provide their values through fixture.componentRef.setInput() rather than direct host assignment. In the test covering automatic row measurement, omit virtualItemSize so the measured-height path is exercised; keep explicit sizing only in tests that require it, and update all referenced setup blocks consistently.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tedi/components/form/select/select.component.html`:
- Around line 290-294: Update the no-options element in the select template’s
empty-state branch to indicate that it is not selectable, preferably by adding
aria-disabled="true" while preserving its existing message and styling.
In `@tedi/components/form/select/select.component.ts`:
- Around line 1059-1066: Update onVirtualOptionClick to mark the clicked option
as active and synchronize the active index with that option’s value after
selection. Ensure the index is re-resolved against the rebuilt virtualRows after
clearing the search/filter, so subsequent Enter or Space targets the clicked
option rather than a stale row.
- Around line 1045-1049: Update toggleSelectAll and both virtual select-all
entry points, including activateRow, to use a Set-based membership path when the
default comparator is active, making bulk select and clear operations linear in
the row count. Preserve the existing comparator-based scan for custom
comparators and keep current selection behavior unchanged.
- Around line 627-639: Update the virtualRows computed property so the
select-all row is added only when filteredOptions() contains at least one option
and the row is actually rendered. Preserve the existing option-row ordering and
empty-state behavior, ensuring no select-all entry remains when the filtered
option list is empty.
- Around line 1027-1037: Update initVirtualActive so its selected-row lookup
only considers options that are both selected and navigable, excluding disabled
selected options from initial activation. Preserve the fallback to the first
navigable row via isRowNavigable when no eligible selected option exists.
---
Nitpick comments:
In `@tedi/components/form/select/select.component.spec.ts`:
- Around line 2667-2673: Update the select test host fields used by the affected
virtual-scroll tests to signal inputs, and provide their values through
fixture.componentRef.setInput() rather than direct host assignment. In the test
covering automatic row measurement, omit virtualItemSize so the measured-height
path is exercised; keep explicit sizing only in tests that require it, and
update all referenced setup blocks consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 572db352-0330-4713-b282-f94f451775bb
📒 Files selected for processing (6)
skills/tedi-angular/references/components.mdtedi/components/form/select/select.component.htmltedi/components/form/select/select.component.scsstedi/components/form/select/select.component.spec.tstedi/components/form/select/select.component.tstedi/components/form/select/select.stories.ts
| } @else { | ||
| <div class="tedi-dropdown-item tedi-select__no-options" role="option"> | ||
| {{ noOptionsMessage() || ("select.no-options" | tediTranslate) }} | ||
| </div> | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not announce the empty state as an enabled option.
This row is absent from the navigation model but has role="option", so assistive technology presents it as selectable. Mark it aria-disabled="true" or render a status message outside the listbox.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tedi/components/form/select/select.component.html` around lines 290 - 294,
Update the no-options element in the select template’s empty-state branch to
indicate that it is not selectable, preferably by adding aria-disabled="true"
while preserving its existing message and styling.
| /** Whether the pinned select-all row is shown above the virtual viewport. */ | ||
| readonly showSelectAllRow = computed( | ||
| () => this.allowMultiple() && this.showSelectAll() | ||
| ); | ||
|
|
||
| /** Ordered navigable rows for the virtual listbox (pinned select-all + options). */ | ||
| readonly virtualRows = computed<VirtualRow<T>[]>(() => { | ||
| const rows: VirtualRow<T>[] = []; | ||
| if (this.showSelectAllRow()) rows.push({ kind: "select-all" }); | ||
| for (const option of this.filteredOptions()) { | ||
| rows.push({ kind: "option", option }); | ||
| } | ||
| return rows; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not expose a select-all row that is not rendered.
With no filtered options, virtualRows() still contains select-all, while the template renders only the empty state. This creates a dangling aria-activedescendant, and Enter activates an invisible row.
Proposed fix
readonly showSelectAllRow = computed(
- () => this.allowMultiple() && this.showSelectAll()
+ () =>
+ this.allowMultiple() &&
+ this.showSelectAll() &&
+ this.filteredOptions().length > 0
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** Whether the pinned select-all row is shown above the virtual viewport. */ | |
| readonly showSelectAllRow = computed( | |
| () => this.allowMultiple() && this.showSelectAll() | |
| ); | |
| /** Ordered navigable rows for the virtual listbox (pinned select-all + options). */ | |
| readonly virtualRows = computed<VirtualRow<T>[]>(() => { | |
| const rows: VirtualRow<T>[] = []; | |
| if (this.showSelectAllRow()) rows.push({ kind: "select-all" }); | |
| for (const option of this.filteredOptions()) { | |
| rows.push({ kind: "option", option }); | |
| } | |
| return rows; | |
| /** Whether the pinned select-all row is shown above the virtual viewport. */ | |
| readonly showSelectAllRow = computed( | |
| () => | |
| this.allowMultiple() && | |
| this.showSelectAll() && | |
| this.filteredOptions().length > 0 | |
| ); | |
| /** Ordered navigable rows for the virtual listbox (pinned select-all + options). */ | |
| readonly virtualRows = computed<VirtualRow<T>[]>(() => { | |
| const rows: VirtualRow<T>[] = []; | |
| if (this.showSelectAllRow()) rows.push({ kind: "select-all" }); | |
| for (const option of this.filteredOptions()) { | |
| rows.push({ kind: "option", option }); | |
| } | |
| return rows |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tedi/components/form/select/select.component.ts` around lines 627 - 639,
Update the virtualRows computed property so the select-all row is added only
when filteredOptions() contains at least one option and the row is actually
rendered. Preserve the existing option-row ordering and empty-state behavior,
ensuring no select-all entry remains when the filtered option list is empty.
| /** Set the active row on open: the first selected option, else the first navigable row. */ | ||
| private initVirtualActive(): void { | ||
| const rows = this.virtualRows(); | ||
| const selectedIndex = rows.findIndex( | ||
| (row) => row.kind === "option" && this.isOptionSelected(row.option.value) | ||
| ); | ||
| this.activeIndex.set( | ||
| selectedIndex >= 0 | ||
| ? selectedIndex | ||
| : rows.findIndex((row) => this.isRowNavigable(row)) | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exclude disabled selected options from initial activation.
A preselected option that later becomes disabled is still made active on open, despite virtual navigation otherwise skipping disabled rows. Enter then silently does nothing.
Proposed fix
const selectedIndex = rows.findIndex(
- (row) => row.kind === "option" && this.isOptionSelected(row.option.value)
+ (row) =>
+ row.kind === "option" &&
+ this.isRowNavigable(row) &&
+ this.isOptionSelected(row.option.value)
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** Set the active row on open: the first selected option, else the first navigable row. */ | |
| private initVirtualActive(): void { | |
| const rows = this.virtualRows(); | |
| const selectedIndex = rows.findIndex( | |
| (row) => row.kind === "option" && this.isOptionSelected(row.option.value) | |
| ); | |
| this.activeIndex.set( | |
| selectedIndex >= 0 | |
| ? selectedIndex | |
| : rows.findIndex((row) => this.isRowNavigable(row)) | |
| ); | |
| /** Set the active row on open: the first selected option, else the first navigable row. */ | |
| private initVirtualActive(): void { | |
| const rows = this.virtualRows(); | |
| const selectedIndex = rows.findIndex( | |
| (row) => | |
| row.kind === "option" && | |
| this.isRowNavigable(row) && | |
| this.isOptionSelected(row.option.value) | |
| ); | |
| this.activeIndex.set( | |
| selectedIndex >= 0 | |
| ? selectedIndex | |
| : rows.findIndex((row) => this.isRowNavigable(row)) | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tedi/components/form/select/select.component.ts` around lines 1027 - 1037,
Update initVirtualActive so its selected-row lookup only considers options that
are both selected and navigable, excluding disabled selected options from
initial activation. Preserve the fallback to the first navigable row via
isRowNavigable when no eligible selected option exists.
| private activateRow(row: VirtualRow<T>): void { | ||
| if (row.kind === "select-all") { | ||
| this.toggleSelectAll(); | ||
| this.onTouched(); | ||
| return; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Keep virtual select-all linear for the default comparator.
Both virtual select-all entry points call toggleSelectAll(), whose nested membership scans are O(n²). Selecting or clearing thousands of rows can block the UI, undermining virtual scrolling. Add a Set-based default-comparator branch.
Also applies to: 1068-1071
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tedi/components/form/select/select.component.ts` around lines 1045 - 1049,
Update toggleSelectAll and both virtual select-all entry points, including
activateRow, to use a Set-based membership path when the default comparator is
active, making bulk select and clear operations linear in the row count.
Preserve the existing comparator-based scan for custom comparators and keep
current selection behavior unchanged.
| onVirtualOptionClick(option: SelectOption<T>): void { | ||
| if (this.disabled() || option.disabled) return; | ||
| if (this.allowMultiple()) { | ||
| this.toggleOptionValue(option.value); | ||
| } else { | ||
| this.selectSingleValue(option.value); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reconcile the active row after pointer selection and search clearing.
Pointer clicks leave the previous row active. Additionally, clearing the search rebuilds virtualRows() while retaining the numeric index, so subsequent Enter/Space can toggle a different option. Mark the clicked option active and re-resolve its index by value after clearing the filter.
Also applies to: 1074-1086
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tedi/components/form/select/select.component.ts` around lines 1059 - 1066,
Update onVirtualOptionClick to mark the clicked option as active and synchronize
the active index with that option’s value after selection. Ensure the index is
re-resolved against the rebuilt virtualRows after clearing the search/filter, so
subsequent Enter or Space targets the clicked option rather than a stale row.
1 participant
Summary by CodeRabbit
New Features
Documentation
Bug Fixes