feat(ag-grid-angular-theme): added KbqAgGridRowGroup directive (#DS-4970) - #190
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds client-side Angular row grouping with custom renderers, persistent selection, expansion and sorting APIs, package exports, theme styling, development routing, comprehensive tests, workflow gating, and grid test synchronization. ChangesAngular row grouping
Workflow and test stability
Estimated code review effort: 5 (Critical) | ~90+ minutes Sequence Diagram(s)sequenceDiagram
participant User
participant DevRowGroup
participant KbqAgGridRowGroup
participant AGGrid
User->>DevRowGroup: change grouping, expansion, sorting, or selection
DevRowGroup->>KbqAgGridRowGroup: invoke public API
KbqAgGridRowGroup->>AGGrid: rebuild rowData and update grid state
AGGrid->>KbqAgGridRowGroup: report loaded-node selection changes
KbqAgGridRowGroup->>DevRowGroup: emit selected rows
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Visit the preview URL for this PR (updated for commit 795d636): https://data-grid-next--data-grid-pr-190-kxadmoy7.web.app (expires Mon, 20 Jul 2026 08:19:20 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: b9d49913f5b5988e9af8690a8b37f16143707448 |
🚨 E2E tests failedReview the report for details. 💡 Comment |
artembelik
changed the title
KbqAgGridRowGroup directive (#DS-4970)
|
@rmnturov путь до страницы с примером: /e2e/row-group |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new client-side row grouping solution for AG Grid Community within @koobiq/ag-grid-angular-theme, including custom group/selection renderers, selection persistence across collapse/expand, and sorting of group headers. It also adds comprehensive Jest and Playwright coverage plus theme styles and a dev/demo route for manual verification.
Changes:
- Added
KbqAgGridRowGroupdirective implementing grouping, collapse/expand, selection rollups, and group-column sorting. - Added styling for the new group cell renderer in the theme SCSS.
- Added Jest unit tests and Playwright e2e tests + a dev test page/route for the new functionality.
Reviewed changes
Copilot reviewed 12 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ag-grid-angular-theme/src/row-group.ng.ts | Adds the new row grouping directive plus custom cell/header renderers and selection/sort behavior. |
| packages/ag-grid-angular-theme/src/theme.scss | Introduces styles for the group cell renderer. |
| packages/ag-grid-angular-theme/tests/row-group.ng.spec.ts | Adds unit tests covering grouping, collapse/expand, sorting, and selection persistence/outputs. |
| packages/ag-grid-angular-theme/src/module.ng.ts | Registers the new directive in the theme module’s component list. |
| packages/ag-grid-angular-theme/index.ts | Exports the new directive/public API. |
| dev/ag-grid-angular/src/tests/row-group.ts | Adds a dev/demo page for row grouping (manual + e2e target). |
| dev/ag-grid-angular/src/tests/row-group.playwright-spec.ts | Adds Playwright coverage for grouping UX, selection behavior, and sorting. |
| dev/ag-grid-angular/src/main.ts | Adds a dev route to load the new row-group page. |
| dev/ag-grid-angular/src/tests/skeleton-cell-renderer.playwright-spec.ts | Adjusts a skeleton-cell test to avoid false positives by scoping to the first block. |
| dev/ag-grid-angular/src/tests/column-state.playwright-spec.ts | Stabilizes column width persistence test by waiting for URL state before reload. |
| .github/workflows/e2e-approve-snapshots.yml | Clarifies workflow purpose and adds inline permission rationale comments. |
| .github/workflows/deploy-preview.yml | Removes manual dispatch trigger and an explicit firebase tools version pin. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/ag-grid-angular-theme/src/row-group.ng.ts (2)
1202-1225: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
computeSubGroupPathssplits onPATH_SEPARATOR, which is fragile if a key contains::.Paths are built by joining
toKeyvalues with::, but here the path is split back by::and each segment is assumed to be exactly one grouping level. A group field value containing::breakspathPartsalignment, so the "reveal children start collapsed" invariant can silently misbehave. NotetoggleCollapse/setExpandeddon't split and are unaffected — only this split-based reconstruction is at risk. Low likelihood; flagging for awareness.🤖 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 `@packages/ag-grid-angular-theme/src/row-group.ng.ts` around lines 1202 - 1225, Update computeSubGroupPaths to avoid splitting expandedPath on PATH_SEPARATOR, since encoded grouping keys may contain that separator. Reconstruct and compare the expanded grouping path using the existing key/path representation or another separator-safe approach, while preserving the current child-path generation and collapsed-reveal behavior.
597-621: 📐 Maintainability & Code Quality | 🔵 TrivialSide effects inside the
rowIdMapcomputed().
console.warnand mutation ofwarnedMissingRowId/warnedDuplicateRowIdsare side effects performed inside acomputed(), which is expected to be pure. It works today because the flags make it idempotent, but a purer approach is to keeprowIdMapside-effect-free and emit these dev warnings from a dedicatedeffect(). Deferable.🤖 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 `@packages/ag-grid-angular-theme/src/row-group.ng.ts` around lines 597 - 621, Move the missing-row-ID and duplicate-row-ID warning logic out of the pure rowIdMap computed property and into a dedicated effect, while preserving the existing once-per-warning behavior through warnedMissingRowId and warnedDuplicateRowIds. Keep rowIdMap focused solely on resolving and returning IDs, and have the effect react to the same row ID, group column, data, and duplicate ID state needed for the warnings.
🤖 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 @.github/workflows/e2e-approve-snapshots.yml:
- Around line 8-10: Restrict the issue-comment workflow by adding an
author_association gate to its triggering condition or job condition, allowing
only OWNER, MEMBER, or COLLABORATOR values before checkout and snapshot updates
run. Preserve the existing write permissions and workflow behavior for
authorized users.
In `@dev/ag-grid-angular/src/tests/row-group.playwright-spec.ts`:
- Around line 76-85: Update the row-group test around getGroupCheckbox to first
reset the grid to an ungrouped state, removing the default Country and Sport
groupings before clicking the Country checkbox. Then retain the existing
waitForGroupsVisible and group-header assertion so the test verifies grouping is
added by Country rather than merely preserved through Sport.
In `@packages/ag-grid-angular-theme/src/row-group.ng.ts`:
- Around line 125-126: Update toKey to preserve distinct group-value identities:
include the primitive type in keys so values such as 1 and "1" cannot collide,
and handle null, undefined, objects, and arrays without mapping them all to an
empty key. Either encode supported non-primitive values uniquely or reject them
before group paths and collapse identities are generated.
---
Nitpick comments:
In `@packages/ag-grid-angular-theme/src/row-group.ng.ts`:
- Around line 1202-1225: Update computeSubGroupPaths to avoid splitting
expandedPath on PATH_SEPARATOR, since encoded grouping keys may contain that
separator. Reconstruct and compare the expanded grouping path using the existing
key/path representation or another separator-safe approach, while preserving the
current child-path generation and collapsed-reveal behavior.
- Around line 597-621: Move the missing-row-ID and duplicate-row-ID warning
logic out of the pure rowIdMap computed property and into a dedicated effect,
while preserving the existing once-per-warning behavior through
warnedMissingRowId and warnedDuplicateRowIds. Keep rowIdMap focused solely on
resolving and returning IDs, and have the effect react to the same row ID, group
column, data, and duplicate ID state needed for the warnings.
🪄 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: Pro Plus
Run ID: b8e1b972-1b08-4f7b-91c2-2ac85659c14b
⛔ Files ignored due to path filters (2)
dev/ag-grid-angular/src/tests/__screenshots__/row-group-dark.pngis excluded by!**/*.pngdev/ag-grid-angular/src/tests/__screenshots__/row-group-light.pngis excluded by!**/*.png
📒 Files selected for processing (12)
.github/workflows/deploy-preview.yml.github/workflows/e2e-approve-snapshots.ymldev/ag-grid-angular/src/main.tsdev/ag-grid-angular/src/tests/column-state.playwright-spec.tsdev/ag-grid-angular/src/tests/row-group.playwright-spec.tsdev/ag-grid-angular/src/tests/row-group.tsdev/ag-grid-angular/src/tests/skeleton-cell-renderer.playwright-spec.tspackages/ag-grid-angular-theme/index.tspackages/ag-grid-angular-theme/src/module.ng.tspackages/ag-grid-angular-theme/src/row-group.ng.tspackages/ag-grid-angular-theme/src/theme.scsspackages/ag-grid-angular-theme/tests/row-group.ng.spec.ts
💤 Files with no reviewable changes (1)
- .github/workflows/deploy-preview.yml
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
🚨 E2E tests failedReview the report for details. 💡 Comment |
Review finding: native sorting on data columns breaks the grouped layout 🔴Confirmed / reproduced. While grouping is active, the data columns are still natively sortable (AG Grid defaults The Fix direction (keeping sorting functional): the directive should own data-column sorting the same way it already owns the group column — make AG's native sort a no-op on data columns ( Also worth a look:
|
|
Сгруппированные колонки не должны ли пропадать? мне кажется, да А вот |
|
Дизайн надо точнее реализовать Колонка с чекбоксами широкая, иконка не та, отсупы проверить, ширину табуляции-сдвига |
|
Давай в дев-пример добавим возможность отключать счетчики |
поправил |
баг, поправил |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@dev/ag-grid-angular/src/tests/row-group.playwright-spec.ts`:
- Around line 545-570: The sorting E2E assertions are too incidental to verify
the complete row arrangement. In the test beginning “sorting a data column
reorders leaf rows within a group,” assert the ordered group/leaf structure and
that each group header remains attached to its children after sorting; in
dev/ag-grid-angular/src/tests/row-group.playwright-spec.ts lines 545-570, update
the existing checks accordingly. In the sibling sorting test at lines 590-611,
assert that every Gold value is ascending after the ascending sort and
descending after the descending sort.
In `@packages/ag-grid-angular-theme/tests/row-group.ng.spec.ts`:
- Around line 161-169: Document the public test-stub API by adding purpose
comments to the newColumnsLoaded property and the emitGridReady() and
emitNewColumnsLoaded() methods, describing the event or behavior each exposes or
triggers.
In `@packages/ag-grid-angular-theme/tests/theme.ng.spec.ts`:
- Around line 31-42: Add changeDetection: ChangeDetectionStrategy.OnPush to the
`@Component` decorator for TestGridCustomSelectionWidth, and add a comment
describing the purpose of its public grid test handle; preserve the existing
template and selectionColumnDef behavior.
🪄 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: Pro Plus
Run ID: 93d68d87-bfbc-402d-93e4-c241e7321378
⛔ Files ignored due to path filters (7)
dev/ag-grid-angular/src/tests/__screenshots__/column-menu-opened-dark.pngis excluded by!**/*.pngdev/ag-grid-angular/src/tests/__screenshots__/column-menu-opened-light.pngis excluded by!**/*.pngdev/ag-grid-angular/src/tests/__screenshots__/infinite-selection-dark.pngis excluded by!**/*.pngdev/ag-grid-angular/src/tests/__screenshots__/infinite-selection-light.pngis excluded by!**/*.pngdev/ag-grid-angular/src/tests/__screenshots__/row-group-dark.pngis excluded by!**/*.pngdev/ag-grid-angular/src/tests/__screenshots__/row-group-light.pngis excluded by!**/*.pngdev/ag-grid-angular/src/tests/__screenshots__/status-bar-light.pngis excluded by!**/*.png
📒 Files selected for processing (9)
.github/workflows/deploy-preview.yml.github/workflows/e2e-approve-snapshots.ymldev/ag-grid-angular/src/overview.ng.tsdev/ag-grid-angular/src/tests/row-group.playwright-spec.tsdev/ag-grid-angular/src/tests/theme.ng.tspackages/ag-grid-angular-theme/src/row-group.ng.tspackages/ag-grid-angular-theme/src/theme.ng.tspackages/ag-grid-angular-theme/tests/row-group.ng.spec.tspackages/ag-grid-angular-theme/tests/theme.ng.spec.ts
💤 Files with no reviewable changes (1)
- dev/ag-grid-angular/src/overview.ng.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ag-grid-angular-theme/src/row-group.ng.ts
поправил
сделал по умолчанию только заголовок (как в фигме) добавил возможность настройки пользовательского заголовка (компонента) 
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/ag-grid-angular-theme/src/row-group.ng.ts (1)
170-172: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the public
groupproperty.Add property-level JSDoc describing the group information supplied by the renderer. As per coding guidelines, “All public methods and properties must have a comment describing their purpose.”
🤖 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 `@packages/ag-grid-angular-theme/src/row-group.ng.ts` around lines 170 - 172, Document the public group property in KbqAgGridRowGroupCellContent with property-level JSDoc describing the group information supplied by the renderer.Source: Coding guidelines
dev/ag-grid-angular/src/tests/row-group.ts (1)
104-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider using a signal-driven checkbox instead of
ngModel, for consistency.
[(ngModel)]="useCustomCellContent"bound to amodel()signal is a supported Angular pattern (the forms module special-cases signal targets internally), so this works, but it pulls inFormsModulefor a single control and diverges from the[checked]/(change)pattern already used for every other checkbox in this same template (e.g. the group-by checkboxes viaonToggle). As per coding guidelines, "Prefer Reactive Forms over template-driven forms," and keeping one consistent idiom for all checkboxes in this file would be simpler to maintain.♻️ Suggested refactor
- <label> - <input type="checkbox" data-testid="useCustomCellContentCheckbox" [(ngModel)]="useCustomCellContent" /> - Use custom group cell content - </label> + <label> + <input + type="checkbox" + data-testid="useCustomCellContentCheckbox" + [checked]="useCustomCellContent()" + (change)="useCustomCellContent.set($event.target.checked)" + /> + Use custom group cell content + </label>This also lets you drop the now-unused
FormsModuleimport/importsentry.Also applies to: 139-142
🤖 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 `@dev/ag-grid-angular/src/tests/row-group.ts` around lines 104 - 107, Replace the [(ngModel)] binding on the useCustomCellContent checkbox and the corresponding checkbox at the additional occurrence with the existing [checked]/(change) signal-driven pattern used by onToggle. Update the change handlers to write the useCustomCellContent model signal, then remove the now-unused FormsModule import and component imports entry.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@dev/ag-grid-angular/src/tests/row-group.ts`:
- Around line 104-107: Replace the [(ngModel)] binding on the
useCustomCellContent checkbox and the corresponding checkbox at the additional
occurrence with the existing [checked]/(change) signal-driven pattern used by
onToggle. Update the change handlers to write the useCustomCellContent model
signal, then remove the now-unused FormsModule import and component imports
entry.
In `@packages/ag-grid-angular-theme/src/row-group.ng.ts`:
- Around line 170-172: Document the public group property in
KbqAgGridRowGroupCellContent with property-level JSDoc describing the group
information supplied by the renderer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 18748e47-55f4-4dad-a3f8-0448dd5a91d8
⛔ Files ignored due to path filters (2)
dev/ag-grid-angular/src/tests/__screenshots__/row-group-dark.pngis excluded by!**/*.pngdev/ag-grid-angular/src/tests/__screenshots__/row-group-light.pngis excluded by!**/*.png
📒 Files selected for processing (5)
dev/ag-grid-angular/src/tests/row-group.playwright-spec.tsdev/ag-grid-angular/src/tests/row-group.tspackages/ag-grid-angular-theme/src/row-group.ng.tspackages/ag-grid-angular-theme/src/theme.scsspackages/ag-grid-angular-theme/tests/row-group.ng.spec.ts
💤 Files with no reviewable changes (1)
- packages/ag-grid-angular-theme/src/theme.scss
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ag-grid-angular-theme/tests/row-group.ng.spec.ts
Summary by CodeRabbit