fix(grouping): aggregate grouped columns at shallower group levels#6282
fix(grouping): aggregate grouped columns at shallower group levels#6282ATOM00blue wants to merge 1 commit into
Conversation
When grouping by multiple columns, a grouped column was never aggregated on any group row because `getValue` skipped aggregation whenever the column appeared anywhere in the grouping state. As a result a column that is grouped at a deeper level showed the first leaf row's raw value at its ancestor group rows instead of the aggregated value. Only treat a column as a grouping value when it groups the row at the current depth or a shallower (ancestor) level. Columns grouped at a deeper level are aggregated as expected.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes grouped column aggregation so columns grouped at deeper levels are still aggregated for parent group rows (instead of showing the first leaf row’s raw value), addressing #6228 / #3232.
Changes:
- Update grouped-row
getValuelogic to only short-circuit aggregation for grouping columns at the current/ancestor depth. - Add a regression test covering multi-column grouping where a secondary grouped column must aggregate at the parent level.
- Add a changeset for
@tanstack/table-corepatch release.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/table-core/src/utils/getGroupedRowModel.ts | Fixes aggregation short-circuit logic by considering grouping depth. |
| packages/table-core/tests/getGroupedRowModel.test.ts | Adds regression test for aggregating a grouped column at shallower levels. |
| .changeset/fix-grouped-column-aggregation.md | Declares patch changeset describing the bugfix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Don't aggregate columns that group this row at the current | ||
| // level or an ancestor level - their value is constant across | ||
| // the group. Columns grouped at a deeper level still need to | ||
| // be aggregated here. | ||
| const groupingIndex = existingGrouping.indexOf(columnId) | ||
| if (groupingIndex > -1 && groupingIndex <= depth) { |
| function createPerson(firstName: string, age: number): Person { | ||
| return { | ||
| firstName, | ||
| lastName: 'Doe', | ||
| age, | ||
| visits: 0, | ||
| progress: 0, | ||
| status: 'single', | ||
| } | ||
| } |
Summary
Fixes #6228 (also the long-standing #3232).
When grouping by more than one column, a grouped column was never aggregated on any group row, so it showed an empty/incorrect value at parent group levels.
Bug
Group by
DepartmentthenAge, withaggregationFn: 'min'onAge. At the top-levelDepartmentgroup row theAgecell does not show the aggregated (min) age across the department — it shows the first leaf row's raw value (and renders blank in the demo because the grouped cell isn't treated as an aggregated cell).Root cause
In
getGroupedRowModel, the grouped row'sgetValueskips aggregation for any column that appears anywhere in the grouping state:A column's value is only constant across a group when that column groups the row at the current depth or a shallower (ancestor) level. A column grouped at a deeper level still varies within the current group and must be aggregated.
Fix
Compare the column's position in the grouping order against the row's depth, so only ancestor/current grouping columns short-circuit aggregation:
Single-column grouping is unaffected (index
0at depth0), so the common case behaves exactly as before.Testing
getGroupedRowModel.test.tsthat groups by two columns and asserts the secondary grouped column is aggregated at the parent group level, while still keeping its grouping value at its own level. The test fails onmainand passes with this change.pnpm --filter @tanstack/table-core test:lib— all greenpnpm --filter @tanstack/table-core test:types— greenpnpm --filter @tanstack/table-core build— green