Skip to content

Commit 56ab473

Browse files
authored
fix(table-core): guard Array.isArray before index access in range filter autoRemove (TanStack#6394)
* fix(table-core): guard Array.isArray before index access in range filter 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 * test(filterFns): assert empty-string scalar IS auto-removed The previous regression test for TanStack#6353 bundled the non-empty scalar case (autoRemove(99) and autoRemove(0)) with an empty-string expectation (autoRemove('')). testFalsy treats '' as falsy, so the leading testFalsy(val) branch correctly returns true for '' - the Array.isArray guard added in 0041efb is intentionally additive and not meant to override that behavior. Split the assertion: the non-empty scalar test now stands alone with just autoRemove(99) and autoRemove(0); the empty-string behavior is covered by a separate explicit assertion (expect(autoRemove(''))).toBe(true)) Vitest run: Test Files 1 passed (1) Tests 73 passed (73) Signed-off-by: sanjibani <18418553+sanjibani@users.noreply.github.com> --------- Signed-off-by: sanjibani <18418553+sanjibani@users.noreply.github.com> Co-authored-by: sanjibani <18418553+sanjibani@users.noreply.github.com> Co-authored-by: JSap0914
1 parent efdeeb9 commit 56ab473

2 files changed

Lines changed: 55 additions & 3 deletions

File tree

packages/table-core/src/fns/filterFns.ts

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -246,7 +246,8 @@ const filterFn_between = Object.assign(
246246
},
247247
{
248248
autoRemove: (val: any) =>
249-
testFalsy(val) || (testFalsy(val[0]) && testFalsy(val[1])),
249+
testFalsy(val) ||
250+
(Array.isArray(val) && testFalsy(val[0]) && testFalsy(val[1])),
250251
},
251252
)
252253

@@ -285,7 +286,8 @@ const filterFn_betweenInclusive = Object.assign(
285286
},
286287
{
287288
autoRemove: (val: any) =>
288-
testFalsy(val) || (testFalsy(val[0]) && testFalsy(val[1])),
289+
testFalsy(val) ||
290+
(Array.isArray(val) && testFalsy(val[0]) && testFalsy(val[1])),
289291
},
290292
)
291293

@@ -329,7 +331,8 @@ export const filterFn_inNumberRange = Object.assign(
329331
return [min, max] as const
330332
},
331333
autoRemove: (val: any) =>
332-
testFalsy(val) || (testFalsy(val[0]) && testFalsy(val[1])),
334+
testFalsy(val) ||
335+
(Array.isArray(val) && testFalsy(val[0]) && testFalsy(val[1])),
333336
},
334337
)
335338

packages/table-core/tests/unit/fns/filterFns.test.ts

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -593,6 +593,12 @@ describe('Filter Functions', () => {
593593
expect(autoRemove([undefined, 10])).toBe(false)
594594
expect(autoRemove([5, undefined])).toBe(false)
595595
})
596+
it('should NOT auto-remove a scalar value passed instead of a range tuple', () => {
597+
// Regression test for #6353 - without the Array.isArray guard,
598+
// `val[0]` and `val[1]` are undefined for a scalar and the index
599+
// checks incorrectly auto-remove the filter.
600+
expect(autoRemove(99 as any)).toBe(false)
601+
})
596602
})
597603

598604
describe('filterFns.betweenInclusive.autoRemove', () => {
@@ -617,6 +623,49 @@ describe('Filter Functions', () => {
617623
expect(autoRemove([undefined, 10])).toBe(false)
618624
expect(autoRemove([5, undefined])).toBe(false)
619625
})
626+
it('should NOT auto-remove a scalar value passed instead of a range tuple', () => {
627+
// Regression test for #6353 - same Array.isArray guard needed
628+
// here as in `between` and `inNumberRange`.
629+
expect(autoRemove(99 as any)).toBe(false)
630+
})
631+
})
632+
633+
describe('filterFns.inNumberRange.autoRemove', () => {
634+
const autoRemove = filterFns.inNumberRange.autoRemove!
635+
636+
it('should auto-remove when both endpoints are undefined', () => {
637+
expect(autoRemove([undefined, undefined])).toBe(true)
638+
})
639+
it('should auto-remove when both endpoints are null', () => {
640+
expect(autoRemove([null, null])).toBe(true)
641+
})
642+
it('should auto-remove when both endpoints are empty strings', () => {
643+
expect(autoRemove(['', ''])).toBe(true)
644+
})
645+
it('should NOT auto-remove when both endpoints are valid numbers', () => {
646+
expect(autoRemove([5, 10])).toBe(false)
647+
})
648+
it('should NOT auto-remove when lower bound is 0 (falsy number)', () => {
649+
expect(autoRemove([0, 10])).toBe(false)
650+
})
651+
it('should NOT auto-remove when only one endpoint is provided', () => {
652+
expect(autoRemove([undefined, 10])).toBe(false)
653+
expect(autoRemove([5, undefined])).toBe(false)
654+
})
655+
it('should NOT auto-remove a non-empty scalar number passed instead of a range tuple', () => {
656+
// Regression test for #6353 - `val[0]` and `val[1]` were both
657+
// undefined for a non-array scalar, so the previous index checks
658+
// produced true && true and the filter was discarded.
659+
expect(autoRemove(99 as any)).toBe(false)
660+
expect(autoRemove(0 as any)).toBe(false)
661+
})
662+
it('should auto-remove an empty-string scalar (matches existing testFalsy behavior)', () => {
663+
// `testFalsy` treats '' as falsy, and the leading `testFalsy(val)`
664+
// branch short-circuits to true for an empty string. The new
665+
// Array.isArray guard is intentionally additive and only kicks in
666+
// when the value is not already caught by `testFalsy`.
667+
expect(autoRemove('' as any)).toBe(true)
668+
})
620669
})
621670
})
622671

0 commit comments

Comments
 (0)