[CL-736] migrate chip select to use signals #17136
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17136 +/- ##
==========================================
+ Coverage 40.70% 40.76% +0.05%
==========================================
Files 3541 3541
Lines 101404 101411 +7
Branches 15186 15187 +1
==========================================
+ Hits 41275 41336 +61
+ Misses 58386 58329 -57
- Partials 1743 1746 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
BryanCunningham
changed the title
|
Claude finished @BryanCunningham's task —— View job Code Review CompleteSummaryThis PR successfully migrates the chip-select component to use Angular signals per ADR-0027. The signal inputs, viewChild queries, and computed disabled state are well-implemented. Recent commits show good responsiveness to feedback with test consolidation and documentation improvements. Key strengths:
Areas needing attention: FindingsFinding 1: Partial Signal Migration Creates Hybrid PatternOnPush change detection was added, but several state properties remain as class properties instead of signals: Why this matters: With OnPush, changes to these properties won't automatically trigger change detection. The current implementation works because:
Risk: This pattern is fragile. Future code paths that modify these properties (e.g., programmatic API calls to Consider completing the signal migrationprotected readonly selectedOption = signal<ChipSelectOption<T> | null>(null);
protected readonly renderedOptions = signal<ChipSelectOption<T> | null>(null);
protected readonly menuWidth = signal<number | null>(null);
protected readonly label = computed(() =>
this.selectedOption()?.label || this.placeholderText()
);
protected readonly icon = computed(() =>
this.selectedOption()?.icon || this.placeholderIcon()
);This would eliminate the need for Alternative: If keeping the hybrid approach, document which methods are safe to call programmatically vs. which require Angular context. Finding 2: CSS Class Testing Remains After Agreed Removal🎨 libs/components/src/chip-select/chip-select.component.spec.ts:78-81, 310-312 Despite vleague2's feedback about avoiding CSS class tests and your agreement to "rip this out," icon element tests still check for presence rather than just verifying an icon exists: // Line 78-81
const icon = fixture.debugElement.query(By.css('i[aria-hidden="true"]'));
expect(icon).toBeTruthy();
// Line 310-312
const icon = fixture.debugElement.query(By.css('i[aria-hidden="true"]'));
expect(icon).toBeTruthy();While these don't test the specific Better approach: Test that an icon is visually present to the user or that the icon input was properly bound, without asserting DOM structure. However, given the recent test consolidation work, this is a minor improvement rather than a blocking issue. Finding 3: Effect Dependency Pattern is Intentional💭 libs/components/src/chip-select/chip-select.component.ts:115-117 The comment added (lines 115-117) correctly explains why Finding 4: Test Coverage Gap💭 Coverage report shows 89.47% with 2 lines missing According to Codecov, 2 lines lack coverage. While not blocking, consider identifying which lines are uncovered to determine if they represent important edge cases. RecommendationThis is solid work. The remaining issues are improvements rather than blockers:
The PR achieves its objective of migrating to signal inputs. Whether to complete the full signal migration for internal state is a team decision about consistency vs. incremental migration. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| writeValue(obj: T): void { | ||
| this.selectedOption = this.findOption(this.rootTree, obj); | ||
| this.setOrResetRenderedOptions(); | ||
| this.cdr.markForCheck(); |
There was a problem hiding this comment.
💭 Re: vleague2's question "Why do we need this now?"
This markForCheck() call is necessary because OnPush change detection was added (line 48). When writeValue() is called programmatically by Angular forms (external to the component), OnPush components don't automatically detect changes to component properties. Without this call, the view wouldn't update when the form control's value changes.
This is the correct pattern for CVA implementations with OnPush.
| }); | ||
|
|
||
| describe("Signal Inputs", () => { | ||
| it("should initialize with required signal inputs", () => { |
There was a problem hiding this comment.
💭 Re: vleague2's feedback about test coverage
Agree that the "Signal Inputs" test block tests Angular framework behavior rather than user-facing behavior. These tests will require updates if implementation details change (e.g., switching from signals to another reactive primitive).
Suggested refinement: Focus tests on observable behavior and user interactions rather than internal signal mechanics. Tests like "should initialize with required signal inputs" and "should update when options signal changes" verify that Angular's signal system works, not that this component works correctly.
Consider keeping only tests that verify:
- User interactions produce expected results
- Form integration works correctly
- Accessibility features function properly
- Edge cases are handled
| multi: true, | ||
| }, | ||
| ], | ||
| changeDetection: ChangeDetectionStrategy.OnPush, |
There was a problem hiding this comment.
Finding 2: Adding OnPush change detection improves performance, but several template-bound properties are not signals: selectedOption, renderedOptions, menuWidth, and the label/icon getters that depend on them.
With OnPush, these properties won't automatically trigger view updates when changed through component methods like selectOption(), clear(), viewOption(), etc.
Current workarounds that make this work:
- User events (clicks) trigger change detection automatically
markForCheck()inwriteValue()handles form updates
Risk: This creates a fragile pattern where some code paths trigger detection and others might not. Consider converting these to signals for consistency:
Suggested refactor
protected readonly selectedOption = signal<ChipSelectOption<T> | null>(null);
protected readonly renderedOptions = signal<ChipSelectOption<T> | null>(null);
protected readonly menuWidth = signal<number | null>(null);
protected readonly label = computed(() =>
this.selectedOption()?.label || this.placeholderText()
);
protected readonly icon = computed(() =>
this.selectedOption()?.icon || this.placeholderIcon()
);This would eliminate the need for markForCheck() and make the component fully reactive.
| const currentMenu = this.menu(); | ||
| const trigger = this.menuTrigger(); | ||
| // Only focus if menu is actually open to prevent running during initialization | ||
| if (items.length > 0 && trigger?.isOpen) { |
There was a problem hiding this comment.
💭 Effect dependency consideration
The effect tracks menuItems(), menu(), and menuTrigger() but checks trigger?.isOpen in the condition. Since isOpen is not a signal, changes to it won't trigger this effect.
Current behavior: Effect runs when menuItems change (e.g., navigating submenus) and only focuses if menu happens to be open at that moment.
This appears intentional and correct - you want to focus the first item when navigating between submenus, not every time isOpen changes. The previous menuItems.changes subscription had the same behavior.
Consider adding a comment to clarify this is intentional:
// Only focus if menu is open; isOpen is deliberately not tracked
// to avoid re-focusing when menu opens/closes
| private readonly disabledState = signal(false); | ||
|
|
||
| /** Combined disabled state from both input and programmatic control */ | ||
| readonly disabled = computed(() => this.disabledInput() || this.disabledState()); |
There was a problem hiding this comment.
🎨 Well-designed disabled state pattern
The dual-signal approach for handling disabled state is excellent:
disabledInput- template binding via[disabled]attributedisabledState- programmatic control viasetDisabledState()(CVA)disabled- computed combination of both
This properly handles both use cases and prevents conflicts between template and form control disabled states. Good pattern for CVA + signal migrations.
| const currentValue = formComponent.form.value.selection; | ||
| if (currentValue && !chipComponent["selectedOption"]) { |
There was a problem hiding this comment.
🎨 Test setup workaround indicates fragile pattern
This conditional writeValue call in beforeEach is unusual - it suggests the component isn't properly initializing from the form control's initial value. In real usage with Angular forms, CVA's writeValue should be called automatically by the framework.
This workaround makes the test less representative of actual usage and may mask initialization issues.
* migrate chip select to use signals * Have Claude address feedback and create spec file * remove eslint disable comment * fix failing tests * remove unnecessary tests * improved documentation * remove unnecessary test logic * consolidate tests and remove fragile selectors
🎟️ Tracking
https://bitwarden.atlassian.net/browse/CL-736
📔 Objective
Update chip-select component to use signal based inputs
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes