refactor(combo, drop-down, select): switch to OnPush with signal-backed state - #17622
viktorkombov wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved reactive dependencies can leave remote handling and OnPush item ARIA, selection, or text state stale.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR migrates Combo, Simple Combo, Drop Down, and Select to OnPush change detection with signal-backed state while preserving their public APIs.
Changes:
- Updates components, items, groups, queries, and host metadata for OnPush.
- Adds reactive selection tracking and modernizes overlay and grouping behavior.
- Adds zoneless regression tests and removes unnecessary change detection.
File summaries
| File | Summary |
|---|---|
projects/igniteui-angular/simple-combo/src/simple-combo/simple-combo.component.ts |
Adds OnPush state and signal queries. |
projects/igniteui-angular/simple-combo/src/simple-combo/simple-combo.component.spec.ts |
Adds zoneless state tests. |
projects/igniteui-angular/select/src/select/select.component.ts |
Adds OnPush and signal-backed properties. |
projects/igniteui-angular/select/src/select/select.component.spec.ts |
Adds zoneless Select tests. |
projects/igniteui-angular/select/src/select/select-item.component.ts |
Enables OnPush for select items. |
projects/igniteui-angular/select/src/select/select-group.component.ts |
Enables OnPush for select groups. |
projects/igniteui-angular/drop-down/src/drop-down/drop-down.component.ts |
Adds reactive state and selection revision tracking. |
projects/igniteui-angular/drop-down/src/drop-down/drop-down.component.spec.ts |
Adds zoneless Drop Down tests. |
projects/igniteui-angular/drop-down/src/drop-down/drop-down.base.ts |
Migrates host bindings and state to signals. |
projects/igniteui-angular/drop-down/src/drop-down/drop-down-navigation.directive.ts |
Migrates host bindings and listeners. |
projects/igniteui-angular/drop-down/src/drop-down/drop-down-item.component.ts |
Enables OnPush and updates focus handling. |
projects/igniteui-angular/drop-down/src/drop-down/drop-down-item.base.ts |
Migrates item host bindings and state. |
projects/igniteui-angular/drop-down/src/drop-down/drop-down-group.component.ts |
Adds OnPush and signal-backed group state. |
projects/igniteui-angular/drop-down/src/drop-down/autocomplete/autocomplete.directive.ts |
Removes unnecessary change detection. |
projects/igniteui-angular/combo/src/combo/combo.pipes.ts |
Adjusts filtered-data updates during rendering. |
projects/igniteui-angular/combo/src/combo/combo.component.ts |
Adds OnPush, signals, queries, and selection tracking. |
projects/igniteui-angular/combo/src/combo/combo.component.spec.ts |
Adds zoneless Combo tests. |
projects/igniteui-angular/combo/src/combo/combo.component.html |
Updates reactive template bindings. |
projects/igniteui-angular/combo/src/combo/combo.common.ts |
Adds signal-backed state, queries, and computed values. |
projects/igniteui-angular/combo/src/combo/combo.api.ts |
Makes transition state reactive. |
projects/igniteui-angular/combo/src/combo/combo-item.component.ts |
Enables OnPush and reactive selection. |
projects/igniteui-angular/combo/src/combo/combo-dropdown.component.ts |
Enables OnPush and reactive single-mode state. |
projects/igniteui-angular/combo/src/combo/combo-add-item.component.ts |
Enables OnPush and host metadata. |
Review details
Suppressed comments (4)
projects/igniteui-angular/drop-down/src/drop-down/drop-down-item.component.ts:14
- The indexed branch of
IgxDropDownItemComponent.selectedstill readsselection.first_item(...)directly, without consumingselectionRevision. After this item becomes OnPush,setSelectedItem()andclearSelection()can update the drop-down revision while the virtual row'saria-selectedand selected class remain unchanged. Make this getter read the reactive drop-down selection/revision, or do not switch indexed items to OnPush yet.
changeDetection: ChangeDetectionStrategy.OnPush,
projects/igniteui-angular/drop-down/src/drop-down/drop-down.component.ts:603
- For virtualized items,
IgxDropDownItemComponent.selectedreadsselection.first_item(...)directly and does not consumeselectionRevision. ThussetSelectedItem()/clearSelection()only update this signal while a recycled OnPush item can keep stalearia-selectedand selected-class state in zoneless or programmatic flows. Make the virtual item getter depend on the revision (or another reactive selection signal) and cover virtual selection updates.
this.selectionRevision.update(revision => revision + 1);
projects/igniteui-angular/select/src/select/select-item.component.ts:7
- The OnPush item also leaves its public
textvalue in the plain_textfield. When the selected item's text is changed programmatically, the select component's[value]="selectionValue"binding has no signal dependency on that setter, so the input can keep displaying the old text until some unrelated check occurs. Maketextsignal-backed (or otherwise notify the select) as part of this OnPush migration.
changeDetection: ChangeDetectionStrategy.OnPush,
projects/igniteui-angular/select/src/select/select.component.ts:698
- The new revision is consumed by
IgxSelectComponent.selectedItem, butIgxSelectItemComponent.selectedstill only reads the non-reactive selection service. With the item now OnPush,select.value = ...can update the input text while the item'saria-selectedattribute and selected class remain stale until an unrelated check. Make select items consume this revision (or update an item signal) and add a programmatic selection assertion.
this.selectionRevision.update(revision => revision + 1);
- Files reviewed: 23/23 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| protected _onChangeCallback: (_: any) => void = noop; | ||
| protected readonly selectionRevision = signal(0); | ||
| private readonly dataTypeState = computed(() => this.displayKey ? DataTypes.COMPLEX : DataTypes.PRIMITIVE); | ||
| private readonly remoteState = computed(() => !!(this.totalItemCount > 0 && this.valueKey && this.dataType === DataTypes.COMPLEX)); |
| standalone: true | ||
| host: { | ||
| '[attr.id]': 'id', | ||
| '[attr.aria-label]': 'ariaLabel', |
| selector: 'igx-select-item', | ||
| templateUrl: 'select-item.component.html', | ||
| changeDetection: ChangeDetectionStrategy.Eager, | ||
| changeDetection: ChangeDetectionStrategy.OnPush, |
Closes #17606
Description
Moves Combo, Simple Combo, Drop Down and Select, together with their items and groups, from
ChangeDetectionStrategy.EagertoOnPush. That covers 11 components. The state their templates and host bindings read now lives in signals behind the existing properties, so setting it from code repaints the component without a forceddetectChanges(), under both zone.js and zoneless change detection.The public API does not change.
combo.data = items,select.value = xanddropdown.role = 'menu'still work as property assignments. Inputs are not converted toinput()here, because that would be a breaking change and belongs in its own PR withng updatemigrations.Motivation / Context
These components are the last common form controls still checked on every change-detection pass, and several of them only updated because of that. Moving them to OnPush exposed exactly those cases:
role,labelledByandallowItemsFocusnever reached the view.value,type,disabled,placeholderand the header, footer and toggle-icon templates. In practiceformControl.disable()updatedselect.disabledbut not the rendered input.selectedItemread the selection service directly, and a view cannot observe that. The drop-down now keeps aselectionRevisionsignal, bumped wherever the selection is written, the same mechanism the combo already uses.Other changes along the way:
@HostBinding/@HostListenerare replaced byhostmetadata.viewChild/contentChild/contentChildren, and their property accessors are kept.dataTypeandisRemotearecomputed().ngAfterViewChecked, which is removed.filteredDataback into the combo while the template renders, which signal state rejects, so that write is wrapped inuntracked(). This keeps the existing behaviour. The existingTODOon that line is still the real fix, which is to build the item window in the component instead of in the template.cdr.detectChanges()afternavigateFirst()is dropped.Unobserved bookkeeping stays in plain fields. Only state a view actually reads is signal-backed.
Type of Change
Refactoring, with bug fixes for the stale-rendering cases above, plus tests.
Component(s) / Area(s) Affected
Combo, Simple Combo, Drop Down (including items, groups, navigation and autocomplete), Select (including items and groups).
Compatibility
No public properties, inputs, outputs or types were renamed or removed. The observable difference is OnPush itself.
Code that mutates these components from outside and relied on a parent's check to repaint them is now covered for the state listed above. There is one place it is not: drop-down items'
id,isHeaderandroleare still plain inputs. They work through template bindings but won't repaint if assigned from code.IgxSelectComponentnow overrideswidth,height,maxHeightandallowItemsFocusas accessors that delegate to the drop-down base. ItsmaxHeightdefault of256pxis set in the constructor, so a bound value still wins.How Has This Been Tested?
New zoneless regression tests set state from code and wait with
whenStable(), with nodetectChanges():setDisabledStatepath, and the placeholder.Both select tests failed before the fix. One existing drop-down assertion now reads the group label's real id instead of a hard-coded module counter, which shifts as other specs create groups.
Results against a clean
masterworktree, run with the same commands:masterThe non-grid difference is the 10 new tests. The two Tree Grid failures are the same on both sides:
findNext/findPrev should navigate search highlights with collapsed rows. Both pass when the spec runs alone.Three things on
masterare unrelated to this change but will get in the way of reviewers:--includerun crashes at load withClass extends value undefined. Each suite passes on its own.karma.pivot-grid.conf.jsrequireskarma-jasmine-html-reporter, which isn't installed, sotest:lib:pgridfails before building.IgxTreeGrid - Summaries › should render rows correctly after collapse and expanddepends on fixed sleeps. It failed once in eight runs.Test Configuration
Checklist
CHANGELOG.MDupdates for newly added functionality