feat: better faceting performance#6369
Merged
Merged
Conversation
|
View your CI Pipeline Execution ↗ for commit 09543d4
☁️ Nx Cloud last updated this comment at |
Contributor
📝 WalkthroughWalkthroughRefactors table-core faceted row model caching to use per-table memoized factory functions keyed by column ID instead of re-resolving factories per call, adds an early-return optimization for empty filterable columns, simplifies unique-value counting, adds unit tests, and updates perf tracking docs and build target dependencies. ChangesFaceted row model performance changes
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Column
participant TableRowModels as table._rowModels
participant Factory as Faceted Factory
Column->>TableRowModels: column_getFacetedRowModel(columnId)
alt cache miss
TableRowModels->>Factory: resolve factory for columnId
Factory-->>TableRowModels: store factory
end
TableRowModels-->>Column: return cached factory result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/table-core/tests/unit/features/column-faceting/columnFacetingFeature.test.ts (1)
104-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider strengthening the caching test's assertions.
facetedMinMaxValuesandfacetedUniqueValuesmocks are only checked for call count (Lines 149-150), unlikefacetedRowModelwhich also verifies call arguments viatoHaveBeenNthCalledWith(Lines 147-148). Also, no test in this suite exercises the realcreateFacetedMinMaxValues()implementation end-to-end (the row-model and unique-values wrappers each get a real integration test, but min/max only appears here as a mock used solely to validate caching).♻️ Suggested additions
expect(facetedMinMaxValues).toHaveBeenCalledTimes(2) + expect(facetedMinMaxValues).toHaveBeenNthCalledWith(1, table, 'status') + expect(facetedMinMaxValues).toHaveBeenNthCalledWith(2, table, '__global__') expect(facetedUniqueValues).toHaveBeenCalledTimes(2) + expect(facetedUniqueValues).toHaveBeenNthCalledWith(1, table, 'status') + expect(facetedUniqueValues).toHaveBeenNthCalledWith(2, table, '__global__')🤖 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/tests/unit/features/column-faceting/columnFacetingFeature.test.ts` around lines 104 - 151, Strengthen the caching test in columnFacetingFeature.test by asserting the actual arguments passed to facetedMinMaxValues and facetedUniqueValues, similar to the existing toHaveBeenNthCalledWith checks on facetedRowModel, so the cache key behavior is fully verified. Also add a real end-to-end test for createFacetedMinMaxValues like the existing integration coverage for the row-model and unique-values faceting wrappers, using the relevant column faceting helpers to ensure the implementation works beyond the mock-only caching case.packages/table-core/src/features/column-faceting/columnFacetingFeature.utils.ts (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSix near-identical caching blocks — extract a shared helper.
The per-column (Record-keyed) and global (single-field) caching logic is duplicated three times each with only the feature-fn name/fallback differing. Worth consolidating into one or two small generic helpers to reduce duplication and future drift risk (e.g. if the caching strategy needs to change later, it must be updated in 6 places).
♻️ Example consolidation
+function getOrCreateFacetFactory<TFn>( + cache: Record<string, TFn>, + key: string, + resolve: () => TFn | undefined, + fallback: TFn, +): TFn { + let fn = cache[key] + if (!fn) { + fn = cache[key] = resolve() ?? fallback + } + return fn +} + export function column_getFacetedMinMaxValues<...>(...): [number, number] | undefined { const facetedMinMaxValues = (table._rowModels.facetedMinMaxValues ??= makeObjectMap()) - let facetedMinMaxValuesFn = facetedMinMaxValues[column.id] - - if (!facetedMinMaxValuesFn) { - facetedMinMaxValuesFn = facetedMinMaxValues[column.id] = - table.options.features.facetedMinMaxValues?.(table, column.id) ?? - (() => undefined) - } - - return facetedMinMaxValuesFn() + return getOrCreateFacetFactory( + facetedMinMaxValues, + column.id, + () => table.options.features.facetedMinMaxValues?.(table, column.id), + () => undefined, + )() }A similar single-slot variant can wrap the three global getters.
Also applies to: 27-36, 60-70, 93-102, 121-127, 147-153, 172-178
🤖 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/column-faceting/columnFacetingFeature.utils.ts` at line 1, The column faceting utilities contain repeated caching logic across the per-column and global getters, so extract a shared helper to centralize the memoization behavior. Update the functions in columnFacetingFeature.utils.ts that currently repeat the same cache-check/store pattern, using makeObjectMap and the existing feature getter names/fallbacks as inputs to a generic helper. Keep the helper flexible enough to cover both the Record-keyed column cache and the single-slot global cache, then wire the existing getters to it so the feature-fn-specific differences remain the only per-call variation.
🤖 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
`@packages/table-core/src/features/column-faceting/columnFacetingFeature.utils.ts`:
- Line 1: The column faceting utilities contain repeated caching logic across
the per-column and global getters, so extract a shared helper to centralize the
memoization behavior. Update the functions in columnFacetingFeature.utils.ts
that currently repeat the same cache-check/store pattern, using makeObjectMap
and the existing feature getter names/fallbacks as inputs to a generic helper.
Keep the helper flexible enough to cover both the Record-keyed column cache and
the single-slot global cache, then wire the existing getters to it so the
feature-fn-specific differences remain the only per-call variation.
In
`@packages/table-core/tests/unit/features/column-faceting/columnFacetingFeature.test.ts`:
- Around line 104-151: Strengthen the caching test in columnFacetingFeature.test
by asserting the actual arguments passed to facetedMinMaxValues and
facetedUniqueValues, similar to the existing toHaveBeenNthCalledWith checks on
facetedRowModel, so the cache key behavior is fully verified. Also add a real
end-to-end test for createFacetedMinMaxValues like the existing integration
coverage for the row-model and unique-values faceting wrappers, using the
relevant column faceting helpers to ensure the implementation works beyond the
mock-only caching case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 809e35be-fdf1-4048-96be-b35a6b524cb7
📒 Files selected for processing (9)
nx.jsonpackage.jsonpackages/table-core/src/features/column-faceting/columnFacetingFeature.types.tspackages/table-core/src/features/column-faceting/columnFacetingFeature.utils.tspackages/table-core/src/features/column-faceting/createFacetedRowModel.tspackages/table-core/src/features/column-faceting/createFacetedUniqueValues.tspackages/table-core/tests/unit/features/column-faceting/columnFacetingFeature.test.tsperf-done.mdperf-todo.md
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
1 participant
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.
🎯 Changes
✅ Checklist
pnpm test:pr.Summary by CodeRabbit
New Features
Bug Fixes
Chores