fix: add back sub-rows onto aggregate context#6421
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (83)
📝 WalkthroughWalkthroughAggregation now supports depth-limited row selection, immediate sub-row contexts, renamed merge payloads, depth-aware cached values, and a new maximum sub-row depth table API. Grouped aggregation, built-in functions, tests, and examples are updated for these contracts. ChangesDepth-aware aggregation contracts and execution
Maximum row-depth API
Grouped aggregation and built-in merges
Validation and example adoption
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Table
participant column_getAggregationValue
participant aggregateColumnValue
participant AggregationFnDef
Table->>column_getAggregationValue: request rows and maxDepth
column_getAggregationValue->>aggregateColumnValue: pass resolved depth
aggregateColumnValue->>AggregationFnDef: pass subRows and subRowResults
AggregationFnDef-->>aggregateColumnValue: return aggregated value
aggregateColumnValue-->>column_getAggregationValue: update depth-aware cache
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
View your CI Pipeline Execution ↗ for commit 394a1af
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/table-core/src/features/aggregation/aggregationFeature.utils.ts (1)
284-291: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHelp TypeScript narrow
subRows.TypeScript's control flow analysis does not narrow the type of
subRowsacross closures or via thecanMergeboolean variable assignment. Depending on the TypeScript compiler version and settings,subRows.mapmight trigger anObject is possibly 'undefined'error.Consider explicitly including
subRowsin theifcondition to appease the compiler.♻️ Proposed refactor
- if (canMerge && definition.merge) { + if (canMerge && subRows && definition.merge) { return definition.merge({ ...context, subRowResults: subRows.map((row) =>🤖 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/table-core/src/features/aggregation/aggregationFeature.utils.ts` around lines 284 - 291, Update the merge branch around the canMerge check to explicitly require subRows before calling subRows.map. Preserve the existing definition.merge guard and merge arguments, ensuring TypeScript can narrow subRows as defined within the branch.
🤖 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 `@packages/table-core/src/features/aggregation/aggregationFns.ts`:
- Around line 169-174: Update the loop handling subRowResults in the aggregation
function to validate that extent is a defined array before reading extent[0] or
extent[1]. Skip invalid or missing extents, while preserving the existing
valueKind and undefined min/max filtering for valid ranges.
---
Nitpick comments:
In `@packages/table-core/src/features/aggregation/aggregationFeature.utils.ts`:
- Around line 284-291: Update the merge branch around the canMerge check to
explicitly require subRows before calling subRows.map. Preserve the existing
definition.merge guard and merge arguments, ensuring TypeScript can narrow
subRows as defined within the branch.
🪄 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
Run ID: 91c08fdc-fc43-4a86-a5ac-6e920363e953
📒 Files selected for processing (6)
packages/table-core/src/features/aggregation/aggregationFeature.types.tspackages/table-core/src/features/aggregation/aggregationFeature.utils.tspackages/table-core/src/features/aggregation/aggregationFns.tspackages/table-core/src/features/column-grouping/createGroupedRowModel.tspackages/table-core/tests/implementation/features/aggregation/aggregationFeature.test.tspackages/table-core/tests/unit/fns/aggregationFns.test.ts
| for (let i = 0; i < subRowResults.length; i++) { | ||
| const extent = subRowResults[i]! | ||
| const min = extent[0] | ||
| const max = extent[1] | ||
| const valueKind = getRangeKind(min) | ||
| if (!valueKind || min === undefined || max === undefined) continue |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Missing null-guard on extent elements.
If subRowResults[i] evaluates to undefined (which can happen if a sub-row lacks the aggregation value or getSubRowResult returns undefined), destructuring extent[0] will cause a runtime crash: TypeError: Cannot read properties of undefined (reading '0').
Check if extent is a valid array before accessing its elements.
🛡️ Proposed fix to prevent runtime crash
for (let i = 0; i < subRowResults.length; i++) {
- const extent = subRowResults[i]!
- const min = extent[0]
- const max = extent[1]
+ const extent = subRowResults[i]
+ if (!Array.isArray(extent)) continue
+ const min = extent[0]
+ const max = extent[1]
const valueKind = getRangeKind(min)
if (!valueKind || min === undefined || max === undefined) continue📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (let i = 0; i < subRowResults.length; i++) { | |
| const extent = subRowResults[i]! | |
| const min = extent[0] | |
| const max = extent[1] | |
| const valueKind = getRangeKind(min) | |
| if (!valueKind || min === undefined || max === undefined) continue | |
| for (let i = 0; i < subRowResults.length; i++) { | |
| const extent = subRowResults[i] | |
| if (!Array.isArray(extent)) continue | |
| const min = extent[0] | |
| const max = extent[1] | |
| const valueKind = getRangeKind(min) | |
| if (!valueKind || min === undefined || max === undefined) continue |
🤖 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/table-core/src/features/aggregation/aggregationFns.ts` around lines
169 - 174, Update the loop handling subRowResults in the aggregation function to
validate that extent is a defined array before reading extent[0] or extent[1].
Skip invalid or missing extents, while preserving the existing valueKind and
undefined min/max filtering for valid ranges.
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
At least one additional CI pipeline execution has run since the conclusion below was written and it may no longer be applicable.
Nx Cloud is proposing a fix for your failed CI:
We updated all 10 aggregation example files to align with the PR's API change, which replaced the getAggregationValue(rows?) signature with getAggregationValue(options?) where rows are now passed as { rows: [...] }. Without this fix, every framework's aggregation example failed type-checking with TS2345 because a raw Row[] array is no longer assignable to AggregationValueOptions. These changes restore type correctness across all affected examples.
Tip
✅ We verified this fix by re-running tanstack-angular-table-example-aggregation:build, tanstack-alpine-table-example-aggregation:test:types.
Warning
The suggested diff is too large to display here, but you can view it on Nx Cloud ↗
Or Apply changes locally with:
npx nx-cloud apply-locally DS7h-ftW2
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
1 participant
🎯 Changes
✅ Checklist
pnpm test:pr.Summary by CodeRabbit
Improvements
{ rows, maxDepth }) and added column-levelmaxAggregationDepth(defaulting to0).Examples
Tests