Micro-optimize table-core#6347
Conversation
- mostly turn forEach to c-style for
|
Important Review skippedDraft detected. 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:
📝 WalkthroughWalkthroughThe PR refactors several table-core helpers: column construction and defaults, sorting preparation, aggregation reducers, column flattening, and memoization logic. It also changes numeric string handling in sort functions and ChangesTable core internal updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
View your CI Pipeline Execution ↗ for commit 30aa208
☁️ Nx Cloud last updated this comment at |
|
I'm open to optimizations like this, but could we split them up by topic (sorting/aggregation/filtering/etc.), have some regression tests if not already there, and attempt to document improvements with some kind of measurement? |
|
@KevinVandy how do u want me to split these changes? I think they are small enough to fit in a single PR no? I will create a repo later to benchmark this branch vs |
![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 moved the @ts-expect-error comment in utils.ts to the line preceding the full ternary assignment, ensuring it suppresses type errors for both the fn(...deps) and fn() branches. Previously, the comment was placed inline inside the expression and only covered the immediately following line, leaving the fn() call exposed to TS2345. This single-line restructure fixes the type check failure without altering any runtime behaviour.
Tip
✅ We verified this fix by re-running @tanstack/table-core:test:types.
Warning
The suggested diff is too large to display here, but you can view it on Nx Cloud ↗
Because this branch comes from a fork, it is not possible for us to apply fixes directly, but you can apply the changes locally using the available options below.
Apply changes locally with:
npx nx-cloud apply-locally PvU7-XFGo
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/table-core/src/fns/aggregationFns.ts (1)
136-184: 🎯 Functional Correctness | 🟡 MinorAdd tests for the
mean/medianedge cases.
Cover empty input, single-row input, odd/even medians, numeric-string coercion inmean, and mixed numeric/non-numericmedianinputs.🤖 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/fns/aggregationFns.ts` around lines 136 - 184, Add tests around aggregationFn_mean and aggregationFn_median to cover the requested edge cases. In the aggregationFns test coverage, verify mean handles empty leafRows, a single row, and numeric-string coercion, and verify median handles empty input, single-row input, odd/even lengths, and returns undefined for mixed numeric/non-numeric values. Use the existing aggregationFn_mean and aggregationFn_median symbols so the tests stay tied to the current implementation.
🤖 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/core/columns/coreColumnsFeature.utils.ts`:
- Around line 106-116: The built-in defaults from DefaultColumnDef are only on
the prototype in constructColumn’s defaultColumn, so they are dropped when
constructColumn later spreads the object and loses header/cell. Fix this by
making the default header/cell values own properties on the object returned from
coreColumnsFeature.utils.ts, or by changing constructColumn to preserve
inherited defaults before merging columnDef. Use the constructColumn and
DefaultColumnDef symbols to locate the merge path and ensure defaultColumn
carries the built-in values as enumerable own properties.
In `@packages/table-core/src/features/row-sorting/createSortedRowModel.ts`:
- Around line 154-167: The sorted row model is appending nested descendants to
sortedFlatRows before their parent, so flatRows no longer matches the order of
the returned rows. Update createSortedRowModel/sortData so each cloned parent
row is pushed into sortedFlatRows before recursing into row.subRows, while still
preserving the prototype cloning behavior and recursive sorting of subRows.
- Around line 59-67: The sorting filter in createSortedRowModel is preserving
the wrong value: availableSorting is meant to contain ColumnSort entries, but
the loop currently pushes the column object and loses the desc flag. Update the
logic in createSortedRowModel so the retained item comes from the original
sorting array entry (sorting[i]) rather than the column, while still using
table.getColumn(id) and column_getCanSort(column) to decide whether to keep it.
This ensures sortData continues to receive the full ColumnSort information,
including descending state.
In `@packages/table-core/src/utils.ts`:
- Around line 165-177: The memoization logic in the utility that compares
dependencies is treating an initial empty array as already cached, so a
zero-dependency memo can return before its first computation. Update the
dependency tracking in the compare/memo helper in utils.ts so the first run is
represented by a sentinel state (for example, undefined instead of []), and only
apply the “unchanged” early return after the function has actually computed
once. Keep the existing comparison flow around newDeps, deps, depsChanged, and
onAfterCompare, but ensure [] still triggers the initial fn execution and then
caches the result for later calls.
---
Outside diff comments:
In `@packages/table-core/src/fns/aggregationFns.ts`:
- Around line 136-184: Add tests around aggregationFn_mean and
aggregationFn_median to cover the requested edge cases. In the aggregationFns
test coverage, verify mean handles empty leafRows, a single row, and
numeric-string coercion, and verify median handles empty input, single-row
input, odd/even lengths, and returns undefined for mixed numeric/non-numeric
values. Use the existing aggregationFn_mean and aggregationFn_median symbols so
the tests stay tied to the current implementation.
🪄 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: cf5e269f-ed1f-4995-af82-3b3abc9b72a1
📒 Files selected for processing (6)
packages/table-core/src/core/columns/constructColumn.tspackages/table-core/src/core/columns/coreColumnsFeature.utils.tspackages/table-core/src/features/row-sorting/createSortedRowModel.tspackages/table-core/src/fns/aggregationFns.tspackages/table-core/src/fns/sortFns.tspackages/table-core/src/utils.ts
|
I tried to run benchmarks on this PR to see what the improvements are, but there were too many bugs (regressions) in order to get useful measurements. The code rabbit feedback shows some of the breaking changes, here's some more from my local pr review
|
The reason I prefer smaller scoped PRs is because there can be a lot wrong with large PRs. Half of the code changes here are good, but the other half contain breaking changes. I can't accept the PR until it's all perfect. If it's broken up, at least by topic, like improving sorting performance, improving aggregation performance, and so on, then we could integrate the changes in smaller peices. |
|
@KevinVandy I will finish all my changes first, do review and fixes until its ready then I'm gonna create an issue with many small PRs attached to it and close this PR All my breaking changes are because I misread the original code |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@KevinVandy can I get your benchmark repo? |
2 participants
🎯 Changes
Mainly change array methods like
forEach/filterto c-style loops, reduce closures and make some code easier to understand.✅ Checklist
pnpm test:pr.Summary by CodeRabbit
Bug Fixes
Refactor