fix(table-core): correct number-range, filter-depth, autoRemove, and custom faceting semantics - #6503
Merged
Merged
Conversation
…custom faceting semantics Four filtering-semantics bugs from the beta triage. filterFn_inNumberRange matched non-numeric values: JavaScript's loose relational coercion let null, '', and booleans slip into a numeric range (null >= 0 && null <= 20 is true), so the auto-selected number filter leaked empty rows into a [0, max] range on nullable numeric columns. The filter now only matches real numbers. Numeric strings also stop matching; `between`/`betweenInclusive` remain the hybrid string/number range filters. When maxLeafRowFilterDepth stopped the root-down filter recursion, the kept rows' unfiltered descendants stayed visible through row.subRows but never entered flatRows or rowsById, so facet counts and other flat-representation consumers under-counted after filtering. Truncated subtrees now join both. Scoped to the root-down path; the leaf-up path drops truncated subRows entirely (pre-existing behavior) and is left as a possible follow-up. shouldAutoRemoveFilter ORed a hardcoded empty-string check over a custom autoRemove, so a filter function that wanted to keep '' as a filter value could not. A provided autoRemove is now authoritative for defined values; undefined always clears, since it is the universal setFilterValue(undefined) sentinel. Built-ins are unaffected: they all test falsy values themselves. Custom facetedUniqueValues/facetedMinMaxValues/facetedRowModel factories were frozen by a redundant memoization layer in columnFacetingFeature. The stock factories already memoize internally with the same dependencies, so the API-layer memoDeps only served to cache custom implementations against inputs they do not depend on. The API layer is now a plain pass-through like every other row model: stock results stay referentially stable and compute once per invalidation (pinned by Map-identity tests), the no-factory fallback keeps a stable empty Map, and custom factories own their memoization (tableMemo is exported for this). The faceting guides now document the contract and their server-side examples read live data through table.options.meta instead of stale closures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe change updates table-core filtering and faceting behavior. It adds tests for numeric ranges, custom ChangesCluster 5 behavior updates
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 eed3da9
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@beta-window-triage.md`:
- Around line 114-117: Split the step-4 entry so PR `#6443` remains associated
only with the `#5801` expansion-reset work, and move the Cluster 5 filtering
issues (`#6007`, `#5987`, `#6101`, and `#6081`) into a separate step. If a consolidated
Cluster 5 PR is known, reference it there; otherwise state that its vehicle is
pending.
In `@docs/framework/alpine/guide/column-faceting.md`:
- Around line 286-287: Update the faceting example’s factory functions so they
no longer close over the one-time serverFacets value. Expose current server
facet data through a meta getter, then read table.options.meta?.serverFacets
inside both returned faceting functions so subsequent server responses update
both APIs.
In `@docs/framework/ember/guide/column-faceting.md`:
- Around line 307-308: Update the custom faceting examples so each returned
resolver reads current facet data inside its function rather than capturing a
one-time fetch result in the factory closure. Apply this to
docs/framework/ember/guide/column-faceting.md lines 307-308,
docs/framework/lit/guide/column-faceting.md lines 290-291, and
docs/framework/octane/guide/column-faceting.md lines 268-269: populate
uniqueValueMap and return ranges from table.options.meta, a reactive store, or
another live source, preserving immediate updates to server facets.
🪄 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: 7dce26c9-9091-4edf-971f-220b888df3e1
📒 Files selected for processing (31)
beta-window-triage.mddocs/framework/alpine/guide/column-faceting.mddocs/framework/alpine/guide/column-filtering.mddocs/framework/angular/guide/column-faceting.mddocs/framework/angular/guide/column-filtering.mddocs/framework/ember/guide/column-faceting.mddocs/framework/ember/guide/column-filtering.mddocs/framework/lit/guide/column-faceting.mddocs/framework/lit/guide/column-filtering.mddocs/framework/octane/guide/column-faceting.mddocs/framework/octane/guide/column-filtering.mddocs/framework/preact/guide/column-faceting.mddocs/framework/preact/guide/column-filtering.mddocs/framework/react/guide/column-faceting.mddocs/framework/react/guide/column-filtering.mddocs/framework/solid/guide/column-faceting.mddocs/framework/solid/guide/column-filtering.mddocs/framework/svelte/guide/column-faceting.mddocs/framework/svelte/guide/column-filtering.mddocs/framework/vue/guide/column-faceting.mddocs/framework/vue/guide/column-filtering.mdpackages/table-core/src/features/column-faceting/columnFacetingFeature.tspackages/table-core/src/features/column-faceting/columnFacetingFeature.utils.tspackages/table-core/src/features/column-filtering/columnFilteringFeature.types.tspackages/table-core/src/features/column-filtering/columnFilteringFeature.utils.tspackages/table-core/src/features/column-filtering/filterFns.tspackages/table-core/src/features/column-filtering/filterRowsUtils.tspackages/table-core/tests/implementation/features/column-faceting/createFacetedRowModels.test.tspackages/table-core/tests/implementation/features/column-filtering/createFilteredRowModel.test.tspackages/table-core/tests/unit/features/column-filtering/columnFilteringFeature.utils.test.tspackages/table-core/tests/unit/fns/filterFns.test.ts
Comment on lines
+114
to
+117
| 4. **[#6443](https://github.com/TanStack/table/pull/6443)** rebase + merge ([#5801](https://github.com/TanStack/table/issues/5801)). (Cluster 5 — [#6007](https://github.com/TanStack/table/issues/6007), [#5987](https://github.com/TanStack/table/issues/5987), [#6101](https://github.com/TanStack/table/issues/6101), [#6081](https://github.com/TanStack/table/issues/6081) — implemented 2026-08-02, pending PR; close [#6313](https://github.com/TanStack/table/pull/6313)/[#6361](https://github.com/TanStack/table/pull/6361) as superseded when it lands.) | ||
| 5. `_valuesCache`/`defaultColumn` invalidation pair ([#5363](https://github.com/TanStack/table/issues/5363)/[#4485](https://github.com/TanStack/table/issues/4485) + [#5275](https://github.com/TanStack/table/issues/5275)). | ||
| 6. Sorting defaults ([#4946](https://github.com/TanStack/table/issues/4946) one-liner; [#5147](https://github.com/TanStack/table/issues/5147)/[#5832](https://github.com/TanStack/table/issues/5832) auto-dir sampling). | ||
| 7. Remaining semantics decisions: [#5909](https://github.com/TanStack/table/issues/5909) (undefined = uncontrolled), [#5778](https://github.com/TanStack/table/issues/5778), [#6101](https://github.com/TanStack/table/issues/6101), [#6081](https://github.com/TanStack/table/issues/6081). (Cluster 3 — [#6115](https://github.com/TanStack/table/issues/6115), [#5833](https://github.com/TanStack/table/issues/5833), [#4939](https://github.com/TanStack/table/issues/4939), [#6136](https://github.com/TanStack/table/issues/6136) — implemented 2026-08-02, pending PR.) | ||
| 7. Remaining semantics decisions: [#5909](https://github.com/TanStack/table/issues/5909) (undefined = uncontrolled), [#5778](https://github.com/TanStack/table/issues/5778). (Cluster 3 — [#6115](https://github.com/TanStack/table/issues/6115), [#5833](https://github.com/TanStack/table/issues/5833), [#4939](https://github.com/TanStack/table/issues/4939), [#6136](https://github.com/TanStack/table/issues/6136) — implemented 2026-08-02, pending PR.) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file info =="
wc -l beta-window-triage.md
sed -n '1,140p' beta-window-triage.md | nl -ba
echo
echo "== occurrences of `#6443`, `#5801`, Cluster 5, step 4 =="
rg -n "(`#6443`|`#5801`|Cluster 5|step 4|filtering|defaultColumn|_valuesCache|sorting|Cluster 3)" beta-window-triage.mdRepository: TanStack/table
Length of output: 234
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file info =="
wc -l beta-window-triage.md
echo
echo "== file excerpt =="
python3 - <<'PY'
from pathlib import Path
p = Path("beta-window-triage.md")
for i, line in enumerate(p.read_text(encoding="utf-8").splitlines(), 1):
if 1 <= i <= 140:
print(f"{i:4d}: {line}")
PY
echo
echo "== occurrences of key references =="
rg -n "(`#6443`|`#5801`|Cluster 5|step 4|filtering|defaultColumn|_valuesCache|sorting|Cluster 3)" beta-window-triage.md || true
echo
echo "== parse references by line =="
python3 - <<'PY'
from pathlib import Path
import re
text = Path("beta-window-triage.md").read_text(encoding="utf-8")
for match in re.finditer(r"#(\d{3,4})", text):
line = text.count("\n", 0, match.start()) + 1
line_text = text.splitlines()[line-1]
print(f"line {line}: #{match.group(1)}: {line_text[:120]}")
PYRepository: TanStack/table
Length of output: 50370
Split or relabel the step-4 line.
Step 4 currently merges PR #6443 (#5801 expansion reset) and also describes Cluster 5 filtering fixes (#6007, #5987, #6101, #6081). The filtering fixes do not have a consolidated PR listed, so maintainers may treat the wrong PR as the vehicle for Cluster 5. Split this into separate steps, or replace the parenthetical with the actual Cluster 5 PR when available.
🤖 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 `@beta-window-triage.md` around lines 114 - 117, Split the step-4 entry so PR
`#6443` remains associated only with the `#5801` expansion-reset work, and move the
Cluster 5 filtering issues (`#6007`, `#5987`, `#6101`, and `#6081`) into a separate
step. If a consolidated Cluster 5 PR is known, reference it there; otherwise
state that its vehicle is pending.
…ng examples The alpine, ember, lit, and octane guides captured a one-time `await fetch` result in the factory closure, so later server responses could never reach either faceting API. They now read through `table.options.meta` inside the returned functions like the other framework guides, using each framework's own reactive idiom. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
Four filtering-semantics fixes from the beta-window triage (cluster 5), with new unit/implementation tests and guide updates across all 10 framework docs. No changeset (handled at release assembly).
Changes
filterFn_inNumberRangeonly matches real numbers. JS loose relational coercion letnull,'', and booleans slip into a numeric range (null >= 0 && null <= 20istrue), so the auto-selected number filter leaked empty rows into[0, max]ranges on nullable numeric columns. Numeric strings also stop matching;between/betweenInclusiveremain the hybrid string/number range filters.maxLeafRowFilterDepthkeepsflatRows/rowsByIdcomplete. When the root-down recursion stops at the max depth, the kept rows' unfiltered descendants stay visible throughrow.subRowsbut never entered the flat arrays, under-counting facets after filtering. Truncated subtrees now join both. Scoped to the root-down path like the original PR; the leaf-up path drops truncated subRows entirely (pre-existing behavior) and is left as a possible follow-up.autoRemoveis authoritative.shouldAutoRemoveFilterno longer ORs the hardcoded empty-string check over a customautoRemove, so custom filter functions can keep''as a filter value.undefinedalways clears (the universalsetFilterValue(undefined)sentinel). Built-ins are unaffected: they all test falsy values themselves.memoDepsincolumnFacetingFeatureduplicated the stock factories' internaltableMemo(same dependencies), so for customfacetedUniqueValues/facetedMinMaxValues/facetedRowModelimplementations it only cached them against inputs they do not depend on. The API layer is now a plain pass-through like every other row model: stock results stay referentially stable and compute once per invalidation (pinned by Map-identity tests), the no-factory fallback keeps a stable empty Map, and custom factories own their memoization (tableMemois exported). The faceting guides document the contract, and the server-side examples now read live data throughtable.options.metainstead of stale closures.Closes
nullvalues)getFacetedRowModelinconsistently respectsmaxLeafRowFilterDepth: 0)filterFn.autoRemoveignored for empty strings #6101 (filterFn.autoRemoveignored for empty strings)getFacetedUniqueValuesdoes not update on rerenders #6081 (customgetFacetedUniqueValuesdoes not update on rerenders)Supersedes
constructFilterFnstructure, plus the PR's test matrix ported.maxLeafRowFilterDepth): fix and helper ported to the currentfilterRowsUtils.ts, with tests rewritten against the existing nested fixture.Verification
tsc+ eslint clean.pnpm test:lib(18 projects) green.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
undefinedalways clears the filter.Documentation
Tests