fix(components/ag-grid): add keyboard commands for row selection header checkbox#4403
Open
johnhwhite wants to merge 2 commits intomainfrom
Open
fix(components/ag-grid): add keyboard commands for row selection header checkbox#4403johnhwhite wants to merge 2 commits intomainfrom
johnhwhite wants to merge 2 commits intomainfrom
Conversation
johnhwhite
added
the
risk level (author): 1
johnhwhite
temporarily deployed
to
e2e-team-members
GitHub Actions
Inactive
Contributor
📝 WalkthroughWalkthroughSwitched the large data-manager component from BehaviorSubject/imperative updates to Angular Signals/computed gridOptions, introduced a viewId field, replaced HostBinding getters with computed host bindings, made gridSettings reactive via toSignal, and added pause-and-show logic for toggling render. Header-row-selector gained explicit Subscription management, renderer-based keyboard support (Enter/Space) to toggle selectAll/deselectAll, and tests covering keyboard behavior and listener deduplication. ChangesData Manager Large — Signals & reactive gridOptions
Header Row Selector — keyboard interaction and subscription handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
|
View your CI Pipeline Execution ↗ for commit c54a9c7
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
libs/components/ag-grid/src/lib/modules/ag-grid/header/header-row-selector/header-row-selector.component.spec.ts (1)
203-208: Avoid introducingas anyin test setup—use strict typing instead.Line 208 uses
as any, which masks parameter-shape regressions in strict mode. Replace it with a typedIHeaderParamsobject usingas unknown as IHeaderParamsto maintain type safety.Suggested patch
import { GridApi, + IHeaderParams, IRowNode, RowDataUpdatedEvent, SelectionChangedEvent, } from 'ag-grid-community'; @@ - component.agInit({ + const headerParams = { api, displayName: 'test-selected', eGridCell: fixture.nativeElement, eGridHeader: headerEl, - } as any); + } as unknown as IHeaderParams; + component.agInit(headerParams);Per coding guidelines: "Avoid the
anytype in TypeScript; useunknownwhen type is uncertain."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/components/ag-grid/src/lib/modules/ag-grid/header/header-row-selector/header-row-selector.component.spec.ts` around lines 203 - 208, The test is using an unsafe cast with "as any" when calling component.agInit; replace that with a properly typed header params object and cast through unknown to satisfy strict typing: construct the object containing api, displayName, eGridCell, eGridHeader matching the IHeaderParams shape and call component.agInit(obj as unknown as IHeaderParams) instead of using "as any" so the test keeps type safety while accommodating the agInit signature.apps/playground/src/app/components/ag-grid/data-manager-large/data-manager-large.component.ts (1)
165-165: ⚡ Quick winPrefer a signal for local active-state toggling instead of
BehaviorSubject.This state is local to the component and mutation-driven; a signal keeps it aligned with the Angular state-management guideline and simplifies updates at Lines 223 and 233.
As per coding guidelines: “Use signals for state management in Angular.”
Also applies to: 223-234
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/playground/src/app/components/ag-grid/data-manager-large/data-manager-large.component.ts` at line 165, Replace the local BehaviorSubject isActive$ with an Angular signal: import { signal } from '@angular/core' and declare e.g. public isActive = signal(false) (drop the $ suffix). Update all places that currently call isActive$.next(...) to use isActive.set(...) (or isActive.update(...) for toggles) and reads that used isActive$.value or subscriptions to use isActive() (or create a computed/readonly signal if you need an observable-like API). Also remove any subscriptions or teardown logic related to isActive$.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/playground/src/app/components/ag-grid/data-manager-large/data-manager-large.component.ts`:
- Line 165: The grid stays hidden because isActive$ (BehaviorSubject declared as
isActive$) is initialized false; update ngOnInit to call
this.isActive$.next(true) after the data manager is initialized (e.g., after any
initDataManager / data manager setup logic in ngOnInit or the method that
prepares the grid) so the template's `@if` (isActive$ | async) evaluates true on
load and the grid renders immediately.
In
`@libs/components/ag-grid/src/lib/modules/ag-grid/header/header-row-selector/header-row-selector.component.ts`:
- Around line 81-87: Replace the deprecated 'keypress' event with 'keydown' in
the fromEvent call and prevent default for Space to avoid page scrolling: change
fromEvent<KeyboardEvent>(el, 'keypress') to fromEvent<KeyboardEvent>(el,
'keydown'), keep the filter to accept Enter and Space (consider supporting both
' ' and legacy 'Spacebar'), and inside the subscription (where this.checked() is
called) call evt.preventDefault() when evt.key indicates Space before handling
selection; keep existing takeUntilDestroyed, filter, and subscribe flow.
---
Nitpick comments:
In
`@apps/playground/src/app/components/ag-grid/data-manager-large/data-manager-large.component.ts`:
- Line 165: Replace the local BehaviorSubject isActive$ with an Angular signal:
import { signal } from '@angular/core' and declare e.g. public isActive =
signal(false) (drop the $ suffix). Update all places that currently call
isActive$.next(...) to use isActive.set(...) (or isActive.update(...) for
toggles) and reads that used isActive$.value or subscriptions to use isActive()
(or create a computed/readonly signal if you need an observable-like API). Also
remove any subscriptions or teardown logic related to isActive$.
In
`@libs/components/ag-grid/src/lib/modules/ag-grid/header/header-row-selector/header-row-selector.component.spec.ts`:
- Around line 203-208: The test is using an unsafe cast with "as any" when
calling component.agInit; replace that with a properly typed header params
object and cast through unknown to satisfy strict typing: construct the object
containing api, displayName, eGridCell, eGridHeader matching the IHeaderParams
shape and call component.agInit(obj as unknown as IHeaderParams) instead of
using "as any" so the test keeps type safety while accommodating the agInit
signature.
🪄 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: defaults
Review profile: CHILL
Plan: Enterprise
Run ID: 94d05124-8a39-49d4-b7a8-1faef2be7f8f
📒 Files selected for processing (3)
apps/playground/src/app/components/ag-grid/data-manager-large/data-manager-large.component.tslibs/components/ag-grid/src/lib/modules/ag-grid/header/header-row-selector/header-row-selector.component.spec.tslibs/components/ag-grid/src/lib/modules/ag-grid/header/header-row-selector/header-row-selector.component.ts
Collaborator
|
Component Storybooks: Apps: |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/playground/src/app/components/ag-grid/data-manager-large/data-manager-large.component.ts (1)
189-205:⚠️ Potential issue | 🟠 MajorInitialize form values in ngOnInit() after Angular applies
@Inputs.The
gridSettingsform is built in the constructor using default property values, but@Inputproperties are assigned by Angular after the constructor completes. This means if a parent component passes bound inputs, the form (and derivedgridOptionssignal) start with incorrect initial values until form.valueChanges fires.Move form initialization to
ngOnInit()usingpatchValue()to ensure the form reflects actual input values:Suggested fix
public ngOnInit(): void { + this.gridSettings.patchValue( + { + enableTopScroll: this.enableTopScroll, + useColumnGroups: this.useColumnGroups, + showSelect: this.showSelect, + showDelete: this.showDelete, + domLayout: this.domLayout, + compact: this.compact, + wrapText: this.wrapText, + autoHeightColumns: this.autoHeightColumns, + }, + { emitEvent: false }, + ); + this.#dataManagerService.initDataManager({Also applies to lines 263-269.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/playground/src/app/components/ag-grid/data-manager-large/data-manager-large.component.ts` around lines 189 - 205, The form is being created in the constructor so `@Input` values set later are not reflected; move the FormBuilder/this.gridSettings creation and the toSignal setup (gridSettings and gridSettingsValue) into ngOnInit(), or keep creation minimal in the constructor and in ngOnInit() call this.gridSettings.patchValue(...) with the actual `@Input-backed` properties and then initialize/refresh the toSignal-derived gridSettingsValue (and any dependent gridOptions signal) so the initial form value matches Inputs; update the same pattern for the similar form block at the second occurrence (lines ~263-269) and ensure ngOnInit contains the initialization/patching logic referencing gridSettings, gridSettingsValue, and any dependent gridOptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@apps/playground/src/app/components/ag-grid/data-manager-large/data-manager-large.component.ts`:
- Around line 189-205: The form is being created in the constructor so `@Input`
values set later are not reflected; move the FormBuilder/this.gridSettings
creation and the toSignal setup (gridSettings and gridSettingsValue) into
ngOnInit(), or keep creation minimal in the constructor and in ngOnInit() call
this.gridSettings.patchValue(...) with the actual `@Input-backed` properties and
then initialize/refresh the toSignal-derived gridSettingsValue (and any
dependent gridOptions signal) so the initial form value matches Inputs; update
the same pattern for the similar form block at the second occurrence (lines
~263-269) and ensure ngOnInit contains the initialization/patching logic
referencing gridSettings, gridSettingsValue, and any dependent gridOptions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Enterprise
Run ID: 8eca4aee-2ceb-4799-9379-7b863aff6b50
📒 Files selected for processing (4)
apps/playground/src/app/components/ag-grid/data-manager-large/data-manager-large.component.htmlapps/playground/src/app/components/ag-grid/data-manager-large/data-manager-large.component.tslibs/components/ag-grid/src/lib/modules/ag-grid/header/header-row-selector/header-row-selector.component.spec.tslibs/components/ag-grid/src/lib/modules/ag-grid/header/header-row-selector/header-row-selector.component.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AB#3963510
Summary by CodeRabbit
New Features
Improvements
Tests