fix(table-core): guard Array.isArray before index access in range filter autoRemove#6394
Open
sanjibani wants to merge 1 commit into
Open
fix(table-core): guard Array.isArray before index access in range filter autoRemove#6394sanjibani wants to merge 1 commit into
sanjibani wants to merge 1 commit into
Conversation
…ter autoRemove `filterFn_between`, `filterFn_betweenInclusive`, and `filterFn_inNumberRange` shared an `autoRemove` that did `testFalsy(val[0]) && testFalsy(val[1])`. When a scalar value such as `99` was passed instead of a `[min, max]` tuple, `val[0]` and `val[1]` were both `undefined`, so the condition evaluated to `true && true` and the filter was silently auto-removed. Numeric columns default to `inNumberRange` via `column_getAutoFilterFn`, so calling `col.setColumnFilter(99)` on a numeric column discarded the filter before the row scan even ran. Wrap the index checks in `Array.isArray(val) &&`. The leading `testFalsy(val)` still handles `undefined`, `null`, `0`, and `''`. Behavior of the existing array-tuple cases is unchanged because `Array.isArray` is true and short-circuits to the same boolean. Closes TanStack#6353
Contributor
📝 WalkthroughWalkthroughThe autoRemove predicates for the between, betweenInclusive, and inNumberRange range filter functions now guard with Array.isArray(val) before indexing val[0] and val[1], preventing scalar values from being incorrectly auto-removed. Regression tests were added covering scalar inputs for all three filter functions. ChangesRange filter autoRemove fix
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested reviewers: 🚥 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 |
![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: 1
🤖 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/tests/unit/fns/filterFns.test.ts`:
- Around line 599-606: The scalar-number regression test in filterFns.test.ts is
incorrectly asserting that autoRemove('') should be false, but testFalsy('')
already makes autoRemove short-circuit to true. Update the test case around
autoRemove to keep only scalar number assertions (for example, the autoRemove
checks for 99 and 0) and remove the empty-string assertion so the test matches
the intended behavior and the PR’s handling of falsy strings.
🪄 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: 3ba58bf1-9358-4668-8e91-b0aa1143805e
📒 Files selected for processing (2)
packages/table-core/src/fns/filterFns.tspackages/table-core/tests/unit/fns/filterFns.test.ts
Comment on lines
+599
to
+606
| it('should NOT auto-remove a scalar number passed instead of a range tuple', () => { | ||
| // Regression test for #6353 - `val[0]` and `val[1]` were both | ||
| // undefined for a scalar, so the previous index checks produced | ||
| // true && true and the filter was discarded. | ||
| expect(autoRemove(99 as any)).toBe(false) | ||
| expect(autoRemove(0 as any)).toBe(false) | ||
| expect(autoRemove('' as any)).toBe(false) | ||
| }) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Test expects false for '' but autoRemove('') returns true — this test will fail.
testFalsy('') returns true (line 390: val === ''), so autoRemove('') short-circuits to true. The test at line 605 asserts .toBe(false), which will fail. This also contradicts the PR objectives, which state that '' should be auto-removed.
The '' case is also miscategorized in this test — it's titled "should NOT auto-remove a scalar number" but '' is an empty string, not a number. Remove the '' assertion from this test; '' is already correctly handled by testFalsy(val) and should remain auto-removed.
🐛 Proposed fix: remove the incorrect `''` assertion
it('should NOT auto-remove a scalar number passed instead of a range tuple', () => {
// Regression test for `#6353` - `val[0]` and `val[1]` were both
// undefined for a scalar, so the previous index checks produced
// true && true and the filter was discarded.
expect(autoRemove(99 as any)).toBe(false)
expect(autoRemove(0 as any)).toBe(false)
- expect(autoRemove('' as any)).toBe(false)
})📝 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.
Suggested change
| it('should NOT auto-remove a scalar number passed instead of a range tuple', () => { | |
| // Regression test for #6353 - `val[0]` and `val[1]` were both | |
| // undefined for a scalar, so the previous index checks produced | |
| // true && true and the filter was discarded. | |
| expect(autoRemove(99 as any)).toBe(false) | |
| expect(autoRemove(0 as any)).toBe(false) | |
| expect(autoRemove('' as any)).toBe(false) | |
| }) | |
| it('should NOT auto-remove a scalar number passed instead of a range tuple', () => { | |
| // Regression test for `#6353` - `val[0]` and `val[1]` were both | |
| // undefined for a scalar, so the previous index checks produced | |
| // true && true and the filter was discarded. | |
| expect(autoRemove(99 as any)).toBe(false) | |
| expect(autoRemove(0 as any)).toBe(false) | |
| }) |
🤖 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/fns/filterFns.test.ts` around lines 599 - 606,
The scalar-number regression test in filterFns.test.ts is incorrectly asserting
that autoRemove('') should be false, but testFalsy('') already makes autoRemove
short-circuit to true. Update the test case around autoRemove to keep only
scalar number assertions (for example, the autoRemove checks for 99 and 0) and
remove the empty-string assertion so the test matches the intended behavior and
the PR’s handling of falsy strings.
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.
Fixes #6353.
Bug
filterFn_between,filterFn_betweenInclusive, andfilterFn_inNumberRangeshare anautoRemove:When a scalar value such as
99is passed instead of a[min, max]tuple,val[0]andval[1]are bothundefined, so the condition evaluates totrue && trueand the filter is silently auto-removed.Numeric columns default to
inNumberRangeviacolumn_getAutoFilterFn, socol.setColumnFilter(99)on a numeric column discards the filter before the row scan even runs.Fix
Wrap the index checks in
Array.isArray(val) &&:The leading
testFalsy(val)still handlesundefined,null,0, and''. Behavior of the existing array-tuple cases is unchanged becauseArray.isArrayis true and short-circuits to the same boolean.Tests
filterFns.inNumberRange.autoRemovedescribe block (the function had no direct unit tests previously).between.autoRemoveandbetweenInclusive.autoRemovecoveringautoRemove(99).Local trace of all relevant cases:
valundefined[undefined, undefined][null, null]['', ''][5, 10][0, 10][undefined, 10]99''2 files changed, +49/-3.
Summary by CodeRabbit