fix(table-core): filterFn_between/betweenInclusive autoRemove never removes array filter values#6332
Merged
KevinVandy merged 1 commit intoJun 17, 2026
Conversation
…er removing array filter values filterFn_between and filterFn_betweenInclusive had `autoRemove: (val: any) => !val`. Since any array is truthy in JavaScript, `!val` is always `false` for the `[min, max]` tuple these filters receive, meaning the filter is NEVER auto-removed — even when both endpoints are empty strings, null, or undefined. A user who clears both range inputs ends up with an active filter that passes all rows but can never be auto-removed from column filter state. Fix: align with the same pattern used by filterFn_inNumberRange: autoRemove: (val) => testFalsy(val) || (testFalsy(val[0]) && testFalsy(val[1])) This correctly removes the filter when both endpoints are falsy (undefined, null, or empty string) while preserving 0 as a valid endpoint value. Fixes: filterFns.between and filterFns.betweenInclusive auto-remove logic
Contributor
📝 WalkthroughWalkthroughThe ChangesRange Filter autoRemove Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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/filterFns.ts (1)
213-223:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign blank-endpoint checks with the new
autoRemovesemantics.
autoRemovenow treatsnullas empty, but the row predicates still only treat''/undefinedas open-ended. That makes[null, 10]and[5, null]behave as bounded ranges due toNumber(null) === 0, which is inconsistent and can filter rows incorrectly.Suggested fix
const filterFn_between = Object.assign( @@ - ((['', undefined] as Array<any>).includes(filterValues[0]) || + (testFalsy(filterValues[0]) || filterFn_greaterThan(row, columnId, filterValues[0])) && @@ - (['', undefined] as Array<any>).includes(filterValues[1]) || + testFalsy(filterValues[1]) || filterFn_lessThan(row, columnId, filterValues[1])), @@ const filterFn_betweenInclusive = Object.assign( @@ - ((['', undefined] as Array<any>).includes(filterValues[0]) || + (testFalsy(filterValues[0]) || filterFn_greaterThanOrEqualTo(row, columnId, filterValues[0])) && @@ - (['', undefined] as Array<any>).includes(filterValues[1]) || + testFalsy(filterValues[1]) || filterFn_lessThanOrEqualTo(row, columnId, filterValues[1])),Also applies to: 237-247
🤖 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/filterFns.ts` around lines 213 - 223, The blank-endpoint checks in the row predicates are not aligned with the autoRemove semantics. Currently, the checks only treat empty strings and undefined as open-ended endpoints by using (['', undefined] as Array<any>).includes(), but autoRemove treats null as empty too via testFalsy(). This inconsistency causes null values to be converted to Number(null) === 0, resulting in incorrect filtering behavior. Update the blank-endpoint checks in both the filterFn_greaterThan condition (for filterValues[0]) and the filterFn_lessThan condition (for filterValues[1]) to include null alongside '' and undefined in the array check. Also apply the same fix to the similar pattern mentioned at lines 237-247.
🤖 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.
Outside diff comments:
In `@packages/table-core/src/fns/filterFns.ts`:
- Around line 213-223: The blank-endpoint checks in the row predicates are not
aligned with the autoRemove semantics. Currently, the checks only treat empty
strings and undefined as open-ended endpoints by using (['', undefined] as
Array<any>).includes(), but autoRemove treats null as empty too via testFalsy().
This inconsistency causes null values to be converted to Number(null) === 0,
resulting in incorrect filtering behavior. Update the blank-endpoint checks in
both the filterFn_greaterThan condition (for filterValues[0]) and the
filterFn_lessThan condition (for filterValues[1]) to include null alongside ''
and undefined in the array check. Also apply the same fix to the similar pattern
mentioned at lines 237-247.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d461f38c-ce47-42c9-b637-9568ee86da11
📒 Files selected for processing (2)
packages/table-core/src/fns/filterFns.tspackages/table-core/tests/unit/fns/filterFns.test.ts
|
View your CI Pipeline Execution ↗ for commit b4949ea
☁️ Nx Cloud last updated this comment at |
KevinVandy
approved these changes
Jun 17, 2026
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
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.
Bug
filterFn_betweenandfilterFn_betweenInclusivehave:Because the filter value for these functions is always a
[min, max]tuple (an array), and arrays are always truthy in JavaScript,!valis alwaysfalse. This means the filter is never auto-removed — even when both endpoints are cleared toundefined,null, or''.Effect: A user who empties both range inputs ends up with an active filter that passes all rows but is never cleaned up from column filter state.
Fix
Align the
autoRemovepredicate with the pattern already used byfilterFn_inNumberRange:testFalsychecksundefined | null | '', so0is correctly preserved as a valid endpoint.Verification
Before fix: 6 new tests fail (autoRemove returns false for empty-endpoint arrays).
After fix: 63/63 tests pass (51 pre-existing + 12 new).
Summary by CodeRabbit
Bug Fixes
Tests