From 177eeb8d92c3322f728c1eff0ec70a8238caf6c6 Mon Sep 17 00:00:00 2001 From: nguyenlongdang0412 Date: Fri, 28 Aug 2026 15:03:15 +0700 Subject: [PATCH 1/7] fix(filtering): stop trusting the snapshot a filter model came back in `hydrate` cast the state slice straight to `FilterModel`, which is a promise to the compiler and not a check. A condition whose operator belonged to another kind, a `set` whose `values` was not a list, or a condition missing the value its operator needs all reached the predicate builders as a shape they never tested, and threw while the pipeline's `$derived` was reading them. That read happens inside the body's `{#each grid.nodes}`, so the throw took the render pass rather than one column. Six of the seven shapes measured against 1.3.0 brought the grid down, and every path that reaches `hydrate` is untrusted: share links, `localStorage`, and anything handed back to `setState`. `sanitizeFilterModel` now rebuilds the model from the part that can be read and drops the rest. A column left with no readable condition stops filtering, which shows more rows rather than none. That is the deliberate call: for a column behind a value gate it is not strictly failing safe, and a grid that will not render is worse. The predicates carry a second layer for a condition arriving some other way, `applyFilterModel` included: an unknown operator, a missing value, a `set` whose values are not a list and a kind nothing knows now pass every row instead of throwing. Closes #41 --- CHANGELOG.md | 19 ++ .../features/filtering/filter-predicates.ts | 50 +++- .../filtering/filter-sanitize.test.ts | 224 ++++++++++++++++++ src/lib/features/filtering/filter-sanitize.ts | 159 +++++++++++++ .../features/filtering/filtering.svelte.ts | 6 +- src/lib/features/filtering/index.ts | 1 + 6 files changed, 447 insertions(+), 12 deletions(-) create mode 100644 src/lib/features/filtering/filter-sanitize.test.ts create mode 100644 src/lib/features/filtering/filter-sanitize.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index a3a23ac..20af1ca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,25 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- A malformed filter snapshot no longer brings the grid down. `filtering`'s + `hydrate` cast the slice straight to `FilterModel`, so an operator that + belonged to another kind, a `set` whose `values` was not a list, or a + condition missing the value its operator needs all threw while the pipeline + was reading them - a throw inside a `$derived` costs the render pass, not one + column. Six of the seven shapes measured against 1.3.0 threw. The slice is + now sanitized at the boundary: what cannot be read is dropped, and a column + left with nothing stops filtering, which shows more rows rather than none. +- The filter predicates no longer assume the condition handed to them is well + formed. An unknown operator, a missing value, a `set` whose values are not a + list and a kind nothing knows now pass every row instead of throwing. This is + the layer that covers `applyFilterModel`, which an app can call with a model + it read back from its own storage. +- `sorting`'s `hydrate` checked `Array.isArray` and then cast, so a null entry + in the array threw on `columnId`. Entries that do not name a column and a + direction are now dropped. + ## [1.3.0] - 2026-08-24 ### Added diff --git a/src/lib/features/filtering/filter-predicates.ts b/src/lib/features/filtering/filter-predicates.ts index 1923082..49d3a56 100644 --- a/src/lib/features/filtering/filter-predicates.ts +++ b/src/lib/features/filtering/filter-predicates.ts @@ -13,6 +13,14 @@ import { getCellValue, isBlank } from '../../core/utils/index.js' import { setKeyOf } from './distinct-values.js' import { normalizeFilterEntry } from './filter-model.js' +/** + * What an unreadable condition does: nothing. `sanitizeFilterModel` is the + * boundary that should have dropped it, and this is the layer that keeps a + * condition arriving some other way out of the pipeline's `$derived`, where a + * throw costs the whole render pass rather than one column. + */ +const PASSES = (): boolean => true + export function filterTypeOf(def: ColumnDef): FilterType | null { if (def.filter === false || def.filter === undefined) return null return typeof def.filter === 'string' ? def.filter : def.filter.type @@ -24,18 +32,27 @@ function customPredicateOf( return typeof def.filter === 'object' ? def.filter.predicate : undefined } +/** Folding once here keeps `toLowerCase` out of the per-row loop. */ +function foldedQuery(filter: Extract): { + query: string + read: (value: unknown) => string +} { + const fold = filter.caseSensitive + ? (text: string) => text + : (text: string) => text.toLowerCase() + return { + query: fold(String(filter.value ?? '').trim()), + read: (value: unknown) => fold(String(value)) + } +} + function textPredicate( filter: Extract ): (value: unknown) => boolean { if (filter.op === 'blank') return (value) => isBlank(value) if (filter.op === 'notBlank') return (value) => !isBlank(value) - // Folding once here keeps `toLowerCase` out of the per-row loop. - const fold = filter.caseSensitive - ? (text: string) => text - : (text: string) => text.toLowerCase() - const query = fold(filter.value.trim()) - const read = (value: unknown) => fold(String(value)) + const { query, read } = foldedQuery(filter) switch (filter.op) { case 'equals': @@ -52,12 +69,16 @@ function textPredicate( return (value) => !isBlank(value) && read(value).includes(query) case 'notContains': return (value) => isBlank(value) || !read(value).includes(query) + default: + return PASSES } } +type NumberComparator = (value: number, target: number) => boolean + const numberComparators: Record< Exclude, - (value: number, target: number) => boolean + NumberComparator > = { eq: (value, target) => value === target, neq: (value, target) => value !== target, @@ -83,7 +104,12 @@ function numberPredicate( return numeric >= target && numeric <= to } } - const compare = numberComparators[filter.op] + // Widened on purpose: the key is exhaustive by type, and an operator that + // reached here from outside the type system is exactly what this catches. + const compare = (numberComparators as Partial>)[ + filter.op + ] + if (!compare) return PASSES return (value) => !isBlank(value) && compare(Number(value), target) } @@ -134,10 +160,13 @@ function datePredicate( const day = toEpochDay(value) return day >= target && day <= to } + default: + return PASSES } } function setPredicate(filter: Extract): (value: unknown) => boolean { + if (!Array.isArray(filter.values)) return PASSES // Keyed on both sides by the same function the value list is built with, or // the cell holding a Date is never the entry the user picked, and a filter // that came back through a snapshot is never the one that went in. @@ -163,6 +192,8 @@ export function valuePredicateFor(filter: ColumnFilter): (value: unknown) => boo return setPredicate(filter) case 'boolean': return booleanPredicate(filter) + default: + return PASSES } } @@ -174,7 +205,8 @@ function entryPredicate( const { join, conditions } = normalizeFilterEntry(entry) const custom = customPredicateOf(def) - const tests = conditions.map((condition) => { + const listed = Array.isArray(conditions) ? conditions : [] + const tests = listed.map((condition) => { if (custom) return (value: unknown, row: TRow) => custom(value, row, condition) const predicate = valuePredicateFor(condition) return (value: unknown) => predicate(value) diff --git a/src/lib/features/filtering/filter-sanitize.test.ts b/src/lib/features/filtering/filter-sanitize.test.ts new file mode 100644 index 0000000..2e51889 --- /dev/null +++ b/src/lib/features/filtering/filter-sanitize.test.ts @@ -0,0 +1,224 @@ +import { describe, expect, it } from 'vitest' +import { createDataGrid } from '../../core/grid/index.js' +import type { ColumnDef, ColumnFilter } from '../../core/types/index.js' +import { filtering, getFiltering, sanitizeFilterModel } from './index.js' + +interface Row { + id: number + total: number + name: string + when: string + ok: boolean +} + +const columns: ColumnDef[] = [ + { id: 'total', header: 'Total', filter: 'number' }, + { id: 'name', header: 'Name', filter: 'text' }, + { id: 'when', header: 'When', filter: 'date' }, + { id: 'ok', header: 'Ok', filter: 'boolean' } +] + +const data: Row[] = [ + { id: 1, total: 10, name: 'alpha', when: '2026-01-02', ok: true }, + { id: 2, total: 20, name: 'beta', when: '2026-02-02', ok: false } +] + +function gridWith(columnId: string, filter: unknown) { + const grid = createDataGrid({ + columns, + data, + getRowId: (row) => String(row.id), + features: [filtering()] + }) + grid.setState({ + version: 1, + features: { filtering: { quick: '', columns: { [columnId]: filter } } } + } as never) + return grid +} + +describe('sanitizeFilterModel', () => { + it('keeps a model the editor could have built', () => { + const model = sanitizeFilterModel({ + quick: 'ph', + columns: { + total: { kind: 'number', op: 'gt', value: 15 }, + name: { kind: 'text', op: 'contains', value: 'al', caseSensitive: true } + } + }) + + expect(model).toEqual({ + quick: 'ph', + columns: { + total: { kind: 'number', op: 'gt', value: 15 }, + name: { kind: 'text', op: 'contains', value: 'al', caseSensitive: true } + } + }) + }) + + it('reads a group, dropping only the conditions that are broken', () => { + const model = sanitizeFilterModel({ + quick: '', + columns: { + name: { + kind: 'group', + join: 'or', + conditions: [ + { kind: 'text', op: 'contains', value: 'al' }, + { kind: 'text', op: 'eq', value: 'beta' } + ] + } + } + }) + + expect(model?.columns.name).toEqual({ + kind: 'group', + join: 'or', + conditions: [{ kind: 'text', op: 'contains', value: 'al' }] + }) + }) + + it('drops a group whose conditions are not a list, and one left with none', () => { + const model = sanitizeFilterModel({ + columns: { + name: { kind: 'group', join: 'and', conditions: 'nope' }, + when: { kind: 'group', join: 'and', conditions: [{ kind: 'date', op: 'gt' }] } + } + }) + + expect(model).toEqual({ quick: '', columns: {} }) + }) + + it('takes a presence operator with no value, in either spelling', () => { + const model = sanitizeFilterModel({ + columns: { + name: { kind: 'text', op: 'blank' }, + total: { kind: 'number', op: 'notBlank' } + } + }) + + expect(model?.columns).toEqual({ + name: { kind: 'text', op: 'blank', value: '' }, + total: { kind: 'number', op: 'notBlank' } + }) + }) + + it('rejects a slice that is not an object', () => { + expect(sanitizeFilterModel(null)).toBeNull() + expect(sanitizeFilterModel('filtered')).toBeNull() + expect(sanitizeFilterModel([])).toBeNull() + }) + + it('falls back to an empty quick filter rather than carrying junk', () => { + expect(sanitizeFilterModel({ quick: 42, columns: 'nope' })).toEqual({ + quick: '', + columns: {} + }) + }) +}) + +describe('hydrating a malformed filter snapshot', () => { + // One case per row of the table measured on 1.3.0, where six of the seven + // threw while the pipeline was reading them. + const cases: [string, string, unknown][] = [ + [ + 'a number operator spelled as the text one', + 'total', + { + kind: 'number', + op: 'equals', + value: 10 + } + ], + [ + 'a text operator spelled as the number one', + 'name', + { + kind: 'text', + op: 'eq', + value: 'alpha' + } + ], + [ + 'a date operator spelled as the number one', + 'when', + { + kind: 'date', + op: 'gt', + value: '2026-01-01' + } + ], + ['a set whose values are not a list', 'name', { kind: 'set', values: {} }], + ['a kind nothing knows', 'name', { kind: 'nonsense', op: 'eq' }], + ['a text condition with no value', 'name', { kind: 'text', op: 'contains' }], + ['a boolean value that is not a boolean', 'ok', { kind: 'boolean', value: 'yes' }] + ] + + for (const [label, columnId, filter] of cases) { + it(`reads every row and does not throw: ${label}`, () => { + const grid = gridWith(columnId, filter) + + expect(() => grid.nodes).not.toThrow() + expect(grid.nodes).toHaveLength(2) + expect(getFiltering(grid)?.columnFilters).toEqual({}) + }) + } + + it('keeps the readable column when another is dropped', () => { + const grid = createDataGrid({ + columns, + data, + getRowId: (row) => String(row.id), + features: [filtering()] + }) + grid.setState({ + version: 1, + features: { + filtering: { + quick: '', + columns: { + total: { kind: 'number', op: 'gt', value: 15 }, + name: { kind: 'text', op: 'eq', value: 'alpha' } + } + } + } + } as never) + + expect(grid.nodes).toHaveLength(1) + expect(grid.nodes[0]?.row.name).toBe('beta') + }) +}) + +describe('a broken condition set through the public API', () => { + // `applyFilterModel` does not sanitize: it is typed, and an app calling it + // has said what the model is. The predicate layer is what keeps a model + // that lied from reaching the pipeline as a throw. + const conditions: [string, string, ColumnFilter][] = [ + ['an operator from another kind', 'total', { kind: 'number', op: 'equals' } as never], + ['a text operator from another kind', 'name', { kind: 'text', op: 'eq' } as never], + ['a date operator from another kind', 'when', { kind: 'date', op: 'gt' } as never], + ['a set whose values are not a list', 'name', { kind: 'set', values: {} } as never], + ['a kind nothing knows', 'name', { kind: 'nonsense' } as never], + ['a text condition with no value', 'name', { kind: 'text', op: 'contains' } as never], + [ + 'a group whose conditions are not a list', + 'name', + { kind: 'group', join: 'and', conditions: 'nope' } as never + ] + ] + + for (const [label, columnId, condition] of conditions) { + it(`passes every row rather than throwing: ${label}`, () => { + const grid = createDataGrid({ + columns, + data, + getRowId: (row) => String(row.id), + features: [filtering()] + }) + getFiltering(grid)?.applyFilterModel({ quick: '', columns: { [columnId]: condition } }) + + expect(() => grid.nodes).not.toThrow() + expect(grid.nodes).toHaveLength(2) + }) + } +}) diff --git a/src/lib/features/filtering/filter-sanitize.ts b/src/lib/features/filtering/filter-sanitize.ts new file mode 100644 index 0000000..97d3226 --- /dev/null +++ b/src/lib/features/filtering/filter-sanitize.ts @@ -0,0 +1,159 @@ +import type { + ColumnFilter, + ColumnFilterEntry, + DateFilterOp, + FilterModel, + NumberFilterOp, + SetFilterValue, + TextFilterOp +} from '../../core/types/index.js' + +/** + * A snapshot is not a `FilterModel` just because it was cast to one. Share + * links, `localStorage` and anything handed back to `setState` have all been + * outside the grid, and a filter whose operator or value does not match its + * kind reaches the predicate builders as a shape they never check. That threw + * inside the pipeline's `$derived`, which takes down the render pass rather + * than one column. + * + * So the boundary drops what it cannot read. A condition that fails to + * sanitize is left out; a column left with nothing is left out; a model with + * no readable column filters nothing, which is what an empty model already + * does. That shows more rows than the snapshot asked for, deliberately: it is + * the honest reading of a filter nobody can reconstruct, and the alternative + * measured here was a grid that would not render at all. + */ + +const TEXT_OPS = new Set([ + 'contains', + 'notContains', + 'equals', + 'notEqual', + 'startsWith', + 'endsWith', + 'blank', + 'notBlank' +]) +const NUMBER_OPS = new Set([ + 'eq', + 'neq', + 'gt', + 'gte', + 'lt', + 'lte', + 'between', + 'blank', + 'notBlank' +]) +const DATE_OPS = new Set(['equals', 'before', 'after', 'between', 'blank', 'notBlank']) +const PRESENCE_OPS = new Set(['blank', 'notBlank']) + +function isRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value) +} + +function opOf(raw: Record, allowed: Set): string | null { + const { op } = raw + return typeof op === 'string' && allowed.has(op) ? op : null +} + +function isSetValue(value: unknown): value is SetFilterValue { + return ( + value === null || + typeof value === 'string' || + typeof value === 'number' || + typeof value === 'boolean' + ) +} + +function sanitizeText(raw: Record): ColumnFilter | null { + const op = opOf(raw, TEXT_OPS) + if (op === null) return null + // The presence operators carry no value, and the editor writes them with + // an empty one, so both spellings have to hydrate. + if (PRESENCE_OPS.has(op)) return { kind: 'text', op: op as TextFilterOp, value: '' } + if (typeof raw.value !== 'string') return null + const filter: Extract = { + kind: 'text', + op: op as TextFilterOp, + value: raw.value + } + if (raw.caseSensitive === true) filter.caseSensitive = true + return filter +} + +function sanitizeNumber(raw: Record): ColumnFilter | null { + const op = opOf(raw, NUMBER_OPS) + if (op === null) return null + if (PRESENCE_OPS.has(op)) return { kind: 'number', op: op as NumberFilterOp } + if (typeof raw.value !== 'number' || !Number.isFinite(raw.value)) return null + if (op === 'between') { + if (typeof raw.to !== 'number' || !Number.isFinite(raw.to)) return null + return { kind: 'number', op: 'between', value: raw.value, to: raw.to } + } + return { kind: 'number', op: op as NumberFilterOp, value: raw.value } +} + +function sanitizeDate(raw: Record): ColumnFilter | null { + const op = opOf(raw, DATE_OPS) + if (op === null) return null + if (PRESENCE_OPS.has(op)) return { kind: 'date', op: op as DateFilterOp } + if (typeof raw.value !== 'string' || raw.value === '') return null + if (op === 'between') { + if (typeof raw.to !== 'string' || raw.to === '') return null + return { kind: 'date', op: 'between', value: raw.value, to: raw.to } + } + return { kind: 'date', op: op as DateFilterOp, value: raw.value } +} + +function sanitizeSet(raw: Record): ColumnFilter | null { + if (!Array.isArray(raw.values)) return null + const values = raw.values.filter(isSetValue) + // The editor never builds an empty selection, so an empty one here is the + // remains of a broken list rather than a request to match nothing. + return values.length > 0 ? { kind: 'set', values } : null +} + +function sanitizeCondition(raw: unknown): ColumnFilter | null { + if (!isRecord(raw)) return null + switch (raw.kind) { + case 'text': + return sanitizeText(raw) + case 'number': + return sanitizeNumber(raw) + case 'date': + return sanitizeDate(raw) + case 'set': + return sanitizeSet(raw) + case 'boolean': + return typeof raw.value === 'boolean' ? { kind: 'boolean', value: raw.value } : null + default: + return null + } +} + +function sanitizeEntry(raw: unknown): ColumnFilterEntry | null { + if (!isRecord(raw)) return null + if (raw.kind !== 'group') return sanitizeCondition(raw) + if (!Array.isArray(raw.conditions)) return null + const conditions = raw.conditions + .map((condition) => sanitizeCondition(condition)) + .filter((condition): condition is ColumnFilter => condition !== null) + if (conditions.length === 0) return null + return { kind: 'group', join: raw.join === 'or' ? 'or' : 'and', conditions } +} + +/** A model built only from what the snapshot got right; null if it got nothing right. */ +export function sanitizeFilterModel(slice: unknown): FilterModel | null { + if (!isRecord(slice)) return null + + const columns: Record = {} + if (isRecord(slice.columns)) { + for (const [columnId, entry] of Object.entries(slice.columns)) { + const clean = sanitizeEntry(entry) + if (clean !== null) columns[columnId] = clean + } + } + + return { quick: typeof slice.quick === 'string' ? slice.quick : '', columns } +} diff --git a/src/lib/features/filtering/filtering.svelte.ts b/src/lib/features/filtering/filtering.svelte.ts index ce8a1a9..fcd5e1e 100644 --- a/src/lib/features/filtering/filtering.svelte.ts +++ b/src/lib/features/filtering/filtering.svelte.ts @@ -3,6 +3,7 @@ import type { ColumnFilterEntry, FilterModel, GridFeature } from '../../core/typ import { mutator } from '../../core/utils/index.js' import { distinctValuesCached } from './distinct-values.js' import { compileColumnFilters } from './filter-predicates.js' +import { sanitizeFilterModel } from './filter-sanitize.js' import { quickFilterNodes } from './quick-filter.js' export const FILTERING = 'filtering' @@ -112,9 +113,8 @@ export function filtering(options: FilteringOptions = {}): GridFeature { - if (slice && typeof slice === 'object') { - getFiltering(grid)?.applyFilterModel(slice as FilterModel) - } + const model = sanitizeFilterModel(slice) + if (model !== null) getFiltering(grid)?.applyFilterModel(model) }, pipelineStage: { order: PIPELINE_ORDER.filter, diff --git a/src/lib/features/filtering/index.ts b/src/lib/features/filtering/index.ts index 3846786..fe5cbf0 100644 --- a/src/lib/features/filtering/index.ts +++ b/src/lib/features/filtering/index.ts @@ -15,6 +15,7 @@ export { normalizeFilterEntry, toFilterRequest } from './filter-model.js' +export { sanitizeFilterModel } from './filter-sanitize.js' export { filterUnitScaleOf, toDisplayUnit, toModelUnit } from './filter-units.js' export { floatingCellOf, type FloatingCell } from './floating-filter.js' export { From e422a6d027591c0487519c0088240b5e5575f986 Mon Sep 17 00:00:00 2001 From: nguyenlongdang0412 Date: Fri, 28 Aug 2026 15:03:22 +0700 Subject: [PATCH 2/7] fix(sorting): drop the sort entries a snapshot could not describe `hydrate` checked `Array.isArray` and then cast, which reads as a check and is not one: a null entry in that array threw on `columnId` while the pipeline was sorting. The same untrusted path as the filter model, one layer thinner. `sanitizeSortState` keeps only the entries naming a column and a direction. An unknown direction, a missing `columnId` and a plain string entry already degraded quietly; they are now dropped rather than carried. --- src/lib/features/sorting/cycle.test.ts | 13 +++++++ src/lib/features/sorting/index.ts | 2 +- src/lib/features/sorting/sort-model.test.ts | 39 ++++++++++++++++++++- src/lib/features/sorting/sort-model.ts | 17 +++++++++ src/lib/features/sorting/sorting.svelte.ts | 4 ++- 5 files changed, 72 insertions(+), 3 deletions(-) diff --git a/src/lib/features/sorting/cycle.test.ts b/src/lib/features/sorting/cycle.test.ts index c39e678..6785509 100644 --- a/src/lib/features/sorting/cycle.test.ts +++ b/src/lib/features/sorting/cycle.test.ts @@ -68,3 +68,16 @@ describe('sort cycle', () => { expect(sort.sort).toEqual([{ columnId: 'name', direction: 'asc' }]) }) }) + +describe('hydrating a malformed sort snapshot', () => { + it('reads every row and keeps only the entry it could rebuild', () => { + const target = grid() + target.setState({ + version: 1, + features: { sorting: [null, { columnId: 'name', direction: 'asc' }] } + } as never) + + expect(() => target.nodes).not.toThrow() + expect(getSorting(target)?.sort).toEqual([{ columnId: 'name', direction: 'asc' }]) + }) +}) diff --git a/src/lib/features/sorting/index.ts b/src/lib/features/sorting/index.ts index ed37c3d..e339e2f 100644 --- a/src/lib/features/sorting/index.ts +++ b/src/lib/features/sorting/index.ts @@ -1,5 +1,5 @@ export { sortNodes, type SortNulls } from './sort.js' -export { toSortRequest } from './sort-model.js' +export { sanitizeSortState, toSortRequest } from './sort-model.js' export { getSorting, Sorting, diff --git a/src/lib/features/sorting/sort-model.test.ts b/src/lib/features/sorting/sort-model.test.ts index 153e168..71520ad 100644 --- a/src/lib/features/sorting/sort-model.test.ts +++ b/src/lib/features/sorting/sort-model.test.ts @@ -1,7 +1,7 @@ import { describe, expect, it } from 'vitest' import { buildRowNodes } from '../../core/grid/index.js' import type { ColumnDef } from '../../core/types/index.js' -import { toSortRequest } from './sort-model.js' +import { sanitizeSortState, toSortRequest } from './sort-model.js' import { sortNodes } from './sort.js' interface Person { @@ -113,3 +113,40 @@ describe('toSortRequest', () => { expect(toSortRequest([], columns)).toEqual([]) }) }) + +describe('sanitizeSortState', () => { + it('keeps the entries that name a column and a direction', () => { + expect( + sanitizeSortState([ + { columnId: 'lastName', direction: 'asc' }, + { columnId: 'age', direction: 'desc' } + ]) + ).toEqual([ + { columnId: 'lastName', direction: 'asc' }, + { columnId: 'age', direction: 'desc' } + ]) + }) + + it('drops a null entry rather than reading through it', () => { + expect(sanitizeSortState([null, { columnId: 'age', direction: 'asc' }])).toEqual([ + { columnId: 'age', direction: 'asc' } + ]) + }) + + it('drops what a sort cannot be built from', () => { + expect( + sanitizeSortState([ + { columnId: 'age', direction: 'sideways' }, + { direction: 'asc' }, + { columnId: '', direction: 'asc' }, + 'age', + 42 + ]) + ).toEqual([]) + }) + + it('rejects a slice that is not a list', () => { + expect(sanitizeSortState({ columnId: 'age', direction: 'asc' })).toBeNull() + expect(sanitizeSortState(null)).toBeNull() + }) +}) diff --git a/src/lib/features/sorting/sort-model.ts b/src/lib/features/sorting/sort-model.ts index 1f946a8..10caf27 100644 --- a/src/lib/features/sorting/sort-model.ts +++ b/src/lib/features/sorting/sort-model.ts @@ -37,3 +37,20 @@ export function toSortRequest( function flip(nulls: SortNulls): SortNulls { return nulls === 'first' ? 'last' : 'first' } + +/** + * A sort read back from a snapshot, keeping only the entries that name a + * column and a direction. The same reasoning as the filter model: what came + * through storage is not a `SortState[]` because it was cast to one, and a + * null entry in that array threw while the pipeline was reading it. + */ +export function sanitizeSortState(slice: unknown): SortState[] | null { + if (!Array.isArray(slice)) return null + return slice.flatMap((entry) => { + if (typeof entry !== 'object' || entry === null) return [] + const { columnId, direction } = entry as Record + if (typeof columnId !== 'string' || columnId === '') return [] + if (direction !== 'asc' && direction !== 'desc') return [] + return [{ columnId, direction }] + }) +} diff --git a/src/lib/features/sorting/sorting.svelte.ts b/src/lib/features/sorting/sorting.svelte.ts index 7eb600f..27e5dec 100644 --- a/src/lib/features/sorting/sorting.svelte.ts +++ b/src/lib/features/sorting/sorting.svelte.ts @@ -3,6 +3,7 @@ import { type GridState, PIPELINE_ORDER } from '../../core/grid/index.js' import type { GridFeature, Keybinding, SortDirection, SortState } from '../../core/types/index.js' import { mutator } from '../../core/utils/index.js' import { sortNodes, type SortNulls } from './sort.js' +import { sanitizeSortState } from './sort-model.js' export const SORTING = 'sorting' @@ -116,7 +117,8 @@ export function sorting(options: SortingOptions = {}): GridFeature { return sort.length > 0 ? sort : undefined }, hydrate: (slice, grid) => { - if (Array.isArray(slice)) getSorting(grid)?.setSort(slice as SortState[]) + const sort = sanitizeSortState(slice) + if (sort !== null) getSorting(grid)?.setSort(sort) }, pipelineStage: { order: PIPELINE_ORDER.sort, From 8f8d16545ce688f4b518fa0d293f62e9b5d4541b Mon Sep 17 00:00:00 2001 From: nguyenlongdang0412 Date: Wed, 2 Sep 2026 14:15:11 +0700 Subject: [PATCH 3/7] fix(core): refuse a column width the layout cannot draw A `NaN` or `Infinity` width reached the CSS custom property as `NaNpx`. A custom property accepts that, but `grid-template-columns: var(...)` then resolves to an invalid value, the declaration is dropped at computed-value time, and every column folds into a single track with the cells stacked down the page. Nothing threw and nothing was logged: the grid simply looked broken, and `widthOverrides` still read as a plausible record while the layout was already gone. Two ways in, both closed. The snapshot boundary now reads values as carefully as it already read keys, and `setWidth`/`setWidths` refuse a width that is not finite, where `clamp` and `Math.round` had been carrying `NaN` straight through. An app computing a width from an empty input field reached the second without going near a snapshot. `setState` also stopped throwing on a corrupt `columns` slice: an `order` that was not an array reached `.filter` inside the caller's own call. Only string ids order the columns now, only real booleans hide one or fold a group, and a slice that is not an object is read as nothing at all. The fields are typed as unknown while they are read, because naming the type there would be the same promise that let the broken snapshot in. The regression test asserts the computed `grid-template-columns` in the browser, not the override record. Backing the fix out turns it red with one track where there should be two. Closes #43 --- src/lib/core/columns/column-model.svelte.ts | 8 +- src/lib/core/columns/column-model.test.ts | 30 +++++++ src/lib/core/grid/snapshot.test.ts | 61 +++++++++++++ src/lib/core/grid/snapshot.ts | 76 ++++++++++++---- .../column-width-snapshot.svelte.test.ts | 88 +++++++++++++++++++ 5 files changed, 246 insertions(+), 17 deletions(-) create mode 100644 src/tests/column-width-snapshot.svelte.test.ts diff --git a/src/lib/core/columns/column-model.svelte.ts b/src/lib/core/columns/column-model.svelte.ts index 31404ca..1e946ac 100644 --- a/src/lib/core/columns/column-model.svelte.ts +++ b/src/lib/core/columns/column-model.svelte.ts @@ -241,6 +241,11 @@ export class ColumnModel { setWidth(id: string, width: number): number { const column = this.get(id) if (!column) return 0 + // `clamp` and `Math.round` both carry `NaN` through, and a width the + // layout cannot draw takes `grid-template-columns` down with it rather + // than making one column wrong. Refuse it and say what the width still + // is. + if (!Number.isFinite(width)) return this.widthOf(id) ?? 0 const clamped = Math.round(clamp(width, column.minWidth, column.maxWidth)) this.widthOverrides = { ...this.widthOverrides, [id]: clamped } return clamped @@ -250,7 +255,8 @@ export class ColumnModel { const next = { ...this.widthOverrides } for (const [id, width] of Object.entries(widths)) { const column = this.get(id) - if (column) next[id] = Math.round(clamp(width, column.minWidth, column.maxWidth)) + if (!column || !Number.isFinite(width)) continue + next[id] = Math.round(clamp(width, column.minWidth, column.maxWidth)) } this.widthOverrides = next } diff --git a/src/lib/core/columns/column-model.test.ts b/src/lib/core/columns/column-model.test.ts index 8bac05b..03c0b18 100644 --- a/src/lib/core/columns/column-model.test.ts +++ b/src/lib/core/columns/column-model.test.ts @@ -85,3 +85,33 @@ describe('ColumnModel runtime state', () => { expect(model.visible.map((column) => column.id)).toEqual(['x', 'y', 'z']) }) }) + +describe('a width the layout could not draw', () => { + it('refuses it and reports the width the column still has', () => { + const model = createModel() + expect(model.setWidth('a', Number.NaN)).toBe(100) + expect(model.setWidth('a', Number.POSITIVE_INFINITY)).toBe(100) + expect(model.widthOverrides).toEqual({}) + expect(model.widthOf('a')).toBe(100) + }) + + it('reports zero for a column that is not there, as before', () => { + expect(createModel().setWidth('missing', Number.NaN)).toBe(0) + }) + + it('skips it in a batch and keeps the rest', () => { + const model = createModel() + model.setWidths({ a: Number.NaN, c: 150, d: Number.NEGATIVE_INFINITY }) + + expect(model.widthOverrides).toEqual({ c: 150 }) + expect(model.widthOf('a')).toBe(100) + expect(model.widthOf('d')).toBe(90) + }) + + it('leaves a width already set alone', () => { + const model = createModel() + model.setWidth('a', 250) + expect(model.setWidth('a', Number.NaN)).toBe(250) + expect(model.widthOf('a')).toBe(250) + }) +}) diff --git a/src/lib/core/grid/snapshot.test.ts b/src/lib/core/grid/snapshot.test.ts index ea654ea..172a4fb 100644 --- a/src/lib/core/grid/snapshot.test.ts +++ b/src/lib/core/grid/snapshot.test.ts @@ -146,3 +146,64 @@ describe('folded groups in a snapshot', () => { ).toEqual({}) }) }) + +describe('a column snapshot that has been outside the grid', () => { + const knownIds = ['a', 'b'] + + it('ignores an order that is not a list', () => { + expect(resolveColumnSnapshot({ order: 'a' }, knownIds).orderIds).toEqual([]) + expect(resolveColumnSnapshot({ order: 42 }, knownIds).orderIds).toEqual([]) + }) + + it('keeps only the string ids in an order', () => { + expect(resolveColumnSnapshot({ order: [null, 7, 'b'] }, knownIds).orderIds).toEqual([ + 'b', + 'a' + ]) + }) + + it('drops a width the layout could not draw', () => { + const widths = { + a: Number.NaN, + b: Number.POSITIVE_INFINITY + } + expect(resolveColumnSnapshot({ widths }, knownIds).widthOverrides).toEqual({}) + }) + + it('drops a width that is not a number at all', () => { + expect( + resolveColumnSnapshot({ widths: { a: '120', b: null } }, knownIds).widthOverrides + ).toEqual({}) + }) + + it('keeps a width it can draw, negative included, since the model clamps it', () => { + expect( + resolveColumnSnapshot({ widths: { a: 120, b: -5 } }, knownIds).widthOverrides + ).toEqual({ a: 120, b: -5 }) + }) + + it('keeps only real booleans for hidden and collapsed', () => { + const stored = { hidden: { a: 'yes', b: true }, collapsed: { g: 1 } } + const resolved = resolveColumnSnapshot(stored, knownIds, ['g']) + expect(resolved.hiddenOverrides).toEqual({ b: true }) + expect(resolved.collapsedGroups).toEqual({}) + }) + + it('reads an unpinnable side as unpinned rather than dropping the entry', () => { + expect( + resolveColumnSnapshot({ pinned: { a: 'middle' } }, knownIds).pinnedOverrides + ).toEqual({ a: null }) + }) + + it('takes a columns slice that is not an object as nothing at all', () => { + for (const stored of ['nope', 42, ['a'], null, undefined]) { + expect(resolveColumnSnapshot(stored, knownIds)).toEqual({ + orderIds: [], + widthOverrides: {}, + hiddenOverrides: {}, + pinnedOverrides: {}, + collapsedGroups: {} + }) + } + }) +}) diff --git a/src/lib/core/grid/snapshot.ts b/src/lib/core/grid/snapshot.ts index f4e3b50..0a66146 100644 --- a/src/lib/core/grid/snapshot.ts +++ b/src/lib/core/grid/snapshot.ts @@ -16,8 +16,39 @@ export interface ColumnSnapshotSource { const DENSITIES: Density[] = ['compact', 'standard', 'comfortable'] -function pruneRecord(record: Record, known: Set): Record { - return Object.fromEntries(Object.entries(record).filter(([id]) => known.has(id))) +/** + * Keys pruned against the columns that exist, values against what the model + * can actually hold. A snapshot has been outside the grid - a share link, + * `localStorage`, anything handed back to `setState` - so a key surviving is + * no evidence its value did. + */ +function pruneRecord( + record: unknown, + known: Set, + keep: (value: unknown) => value is T +): Record { + if (typeof record !== 'object' || record === null) return {} + + const kept: Record = {} + for (const [id, value] of Object.entries(record)) { + if (known.has(id) && keep(value)) kept[id] = value + } + return kept +} + +function isBoolean(value: unknown): value is boolean { + return typeof value === 'boolean' +} + +/** + * A width the layout can draw. `NaN` and `Infinity` are the ones that matter: + * they reach the CSS custom property as `NaNpx`, which makes + * `grid-template-columns` invalid at computed-value time and collapses every + * column into one track. Nothing throws on the way there, so an unusable width + * has to be refused rather than reported. + */ +function isDrawableWidth(value: unknown): value is number { + return typeof value === 'number' && Number.isFinite(value) } function isEmpty(value: object): boolean { @@ -41,8 +72,9 @@ export function buildColumnSnapshot( * Groups are named apart from columns, since a folded group is keyed by the * group's own id and no column carries it. */ -function resolveOrder(stored: string[] | undefined, known: Set, knownIds: string[]) { - const order = (stored ?? []).filter((id) => known.has(id)) +function resolveOrder(stored: unknown, known: Set, knownIds: string[]) { + if (!Array.isArray(stored)) return [] + const order = stored.filter((id): id is string => typeof id === 'string' && known.has(id)) if (order.length === 0) return [] // A column that appeared since the snapshot was written goes last rather // than disappearing for want of a place in the order. @@ -50,29 +82,41 @@ function resolveOrder(stored: string[] | undefined, known: Set, knownIds } export function resolveColumnSnapshot( - stored: GridSnapshot['columns'], + stored: unknown, knownIds: string[], knownGroupIds: string[] = [] ): ColumnSnapshotSource { const known = new Set(knownIds) - const columns = stored ?? {} + // Read as unknown fields rather than as `GridSnapshot['columns']`. Naming + // the type here would be the same promise that let a broken snapshot in: + // every field below is checked where it is used, so none of them may claim + // a shape on the way past. + const columns: Record = + typeof stored === 'object' && stored !== null && !Array.isArray(stored) + ? (stored as Record) + : {} return { orderIds: resolveOrder(columns.order, known, knownIds), - widthOverrides: pruneRecord(columns.widths ?? {}, known), - hiddenOverrides: pruneRecord(columns.hidden ?? {}, known), - pinnedOverrides: prunePinned(columns.pinned ?? {}, known), - collapsedGroups: pruneRecord(columns.collapsed ?? {}, new Set(knownGroupIds)) + widthOverrides: pruneRecord(columns.widths, known, isDrawableWidth), + hiddenOverrides: pruneRecord(columns.hidden, known, isBoolean), + pinnedOverrides: prunePinned(columns.pinned, known), + collapsedGroups: pruneRecord(columns.collapsed, new Set(knownGroupIds), isBoolean) } } -/** Values too, not just keys: a corrupt entry must not reach the model. */ -function prunePinned( - stored: Record, - known: Set -): Record { +/** + * Values too, not just keys: a corrupt entry must not reach the model. A side + * that cannot be read is kept as an explicit `null` rather than dropped, so a + * column the user unpinned stays unpinned instead of springing back to the + * side its definition names. + */ +function prunePinned(stored: unknown, known: Set): Record { + if (typeof stored !== 'object' || stored === null) return {} + const pinned: Record = {} - for (const [id, side] of Object.entries(pruneRecord(stored, known))) { + for (const [id, side] of Object.entries(stored)) { + if (!known.has(id)) continue pinned[id] = side === 'left' || side === 'right' ? side : null } return pinned diff --git a/src/tests/column-width-snapshot.svelte.test.ts b/src/tests/column-width-snapshot.svelte.test.ts new file mode 100644 index 0000000..379af10 --- /dev/null +++ b/src/tests/column-width-snapshot.svelte.test.ts @@ -0,0 +1,88 @@ +import type { Component } from 'svelte' +import { describe, expect, it } from 'vitest' +import { render } from 'vitest-browser-svelte' +import { + createDataGrid, + DataGrid, + type ColumnDef, + type DataGridProps, + type GridState +} from '$lib/index.js' + +interface Row { + id: number + a: string + b: string +} + +const TypedDataGrid = DataGrid as unknown as Component> + +const rows: Row[] = [{ id: 1, a: 'x', b: 'y' }] +const columns: ColumnDef[] = [ + { id: 'a', header: 'A', width: 200 }, + { id: 'b', header: 'B', width: 200 } +] + +function makeGrid(): GridState { + return createDataGrid({ columns, data: rows, getRowId: (row) => String(row.id) }) +} + +async function mount(grid: GridState) { + const screen = await render(TypedDataGrid, { grid }) + await expect.element(screen.getByRole('grid')).toBeVisible() + return screen +} + +function cellAt(container: Element, row: number, col: number): HTMLElement { + const cell = container.querySelector(`[data-dg-cell="${row}:${col}"]`) + if (!cell) throw new Error(`no cell at ${row}:${col}`) + return cell +} + +/** + * Asserted on the computed style rather than on `widthOverrides`, because the + * override record looked perfectly reasonable while the layout was already + * gone: a width of `NaN` reached the custom property as `NaNpx`, which makes + * `grid-template-columns` invalid at computed-value time. The browser drops + * the whole declaration, every column folds into one track, and the cells + * stack down the page. Nothing throws on the way. + */ +function tracksOf(container: Element, row: number): string[] { + const line = cellAt(container, row, 0).parentElement + if (!line) throw new Error('no row element') + return getComputedStyle(line).gridTemplateColumns.split(' ') +} + +describe('a column width the layout cannot draw', () => { + it('keeps the track list when a snapshot carries NaN', async () => { + const grid = makeGrid() + const screen = await mount(grid) + expect(tracksOf(screen.container, 0)).toHaveLength(2) + + grid.setState({ version: 1, columns: { widths: { a: Number.NaN } } } as never) + await expect.poll(() => tracksOf(screen.container, 0).length).toBe(2) + + const [cellA, cellB] = [cellAt(screen.container, 0, 0), cellAt(screen.container, 0, 1)] + expect(cellA.getBoundingClientRect().top).toBe(cellB.getBoundingClientRect().top) + expect(cellA.getBoundingClientRect().right).toBeLessThanOrEqual( + Math.ceil(cellB.getBoundingClientRect().left) + ) + }) + + it('keeps the track list when the width setter is handed NaN', async () => { + const grid = makeGrid() + const screen = await mount(grid) + + grid.columns.setWidth('a', Number.NaN) + await expect.poll(() => tracksOf(screen.container, 0).length).toBe(2) + expect(grid.columns.style).not.toContain('NaN') + }) + + it('still applies a width it can draw', async () => { + const grid = makeGrid() + const screen = await mount(grid) + + grid.setState({ version: 1, columns: { widths: { a: 320 } } } as never) + await expect.poll(() => tracksOf(screen.container, 0)[0]).toBe('320px') + }) +}) From f8a037ec2f5cb558f385738c33992a40dc393760 Mon Sep 17 00:00:00 2001 From: nguyenlongdang0412 Date: Wed, 2 Sep 2026 14:15:23 +0700 Subject: [PATCH 4/7] fix(core): say so when two rows share an id The row index keeps the last row for a repeated id, so an edit addressed to the row the user opened was written to a different one, and a selection stood for two rows at once. Nothing reported it. The id belongs to the app and the grid cannot mend it, but failing at it silently, in the data, is the worst of the available behaviours. A development build now names the ids that collided. Production pays one integer comparison for the whole data set, `index.size` against `nodes.length`; working out which ids repeated costs a second pass and only a build that will print it pays for that. This is the library's only `console` statement, and the lint rule that forbids them is disabled on exactly that line. An error would take an app down over data the grid can still draw, and there is no logger to route it to. Closes #44 --- CHANGELOG.md | 21 ++++++++ src/lib/core/grid/row-node.test.ts | 78 ++++++++++++++++++++++++++++++ src/lib/core/grid/row-node.ts | 40 +++++++++++++++ 3 files changed, 139 insertions(+) create mode 100644 src/lib/core/grid/row-node.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index a3a23ac..7d4338d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,27 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- A column width that the layout cannot draw no longer destroys the grid. A + `NaN` or `Infinity` width reached the CSS custom property as `NaNpx`, which + makes `grid-template-columns` invalid at computed-value time: the browser + dropped the declaration, every column folded into one track and the cells + stacked down the page, with nothing thrown and nothing logged. Such a width + is now refused both at the snapshot boundary and in `setWidth`/`setWidths`, + where `clamp` and `Math.round` had been carrying `NaN` straight through. +- Two rows sharing an id no longer fail silently. The row index keeps the last + row for a repeated id, so an edit addressed to the row the user opened was + written to the other one and nothing said so. A development build now names + the ids that collided. Production pays one integer comparison for the check + and nothing more; working out which ids repeated happens only in a build that + will print it. +- `setState` no longer throws on a corrupt `columns` slice. An `order` that was + not an array reached `.filter` and threw inside the caller's own call. Values + are now read as carefully as keys already were: only string ids order the + columns, only real booleans hide a column or fold a group, and a `columns` + slice that is not an object is taken as nothing at all. + ## [1.3.0] - 2026-08-24 ### Added diff --git a/src/lib/core/grid/row-node.test.ts b/src/lib/core/grid/row-node.test.ts new file mode 100644 index 0000000..2fd9174 --- /dev/null +++ b/src/lib/core/grid/row-node.test.ts @@ -0,0 +1,78 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { buildRowNodes, nodesById } from './row-node.js' + +interface Row { + id: string + name: string +} + +const build = (rows: Row[]) => buildRowNodes(rows, (row) => row.id) + +afterEach(() => { + vi.restoreAllMocks() +}) + +describe('nodesById', () => { + it('keys every row by its id', () => { + const index = nodesById( + build([ + { id: 'a', name: 'first' }, + { id: 'b', name: 'second' } + ]) + ) + + expect(index.size).toBe(2) + expect(index.get('a')?.row.name).toBe('first') + }) + + it('says nothing when the ids are unique', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + nodesById( + build([ + { id: 'a', name: 'first' }, + { id: 'b', name: 'second' } + ]) + ) + + expect(warn).not.toHaveBeenCalled() + }) + + it('names the ids that collided', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + nodesById( + build([ + { id: 'a', name: 'first' }, + { id: 'a', name: 'second' }, + { id: 'b', name: 'third' } + ]) + ) + + expect(warn).toHaveBeenCalledTimes(1) + const message = warn.mock.calls[0]?.[0] as string + expect(message).toContain('getRowId') + expect(message).toContain('a') + expect(message).toContain('3 rows share 2 ids') + }) + + it('caps the list rather than printing every id of a broken set', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const rows = Array.from({ length: 20 }, (_, i) => ({ id: `dup-${i % 10}`, name: `r${i}` })) + nodesById(build(rows)) + + const message = warn.mock.calls[0]?.[0] as string + expect(message).toContain('and 5 more') + }) + + it('keeps the last row for a repeated id, which is why it warns', () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + const index = nodesById( + build([ + { id: 'a', name: 'first' }, + { id: 'a', name: 'second' } + ]) + ) + + expect(index.size).toBe(1) + expect(index.get('a')?.row.name).toBe('second') + }) +}) diff --git a/src/lib/core/grid/row-node.ts b/src/lib/core/grid/row-node.ts index 67e3619..e1b990a 100644 --- a/src/lib/core/grid/row-node.ts +++ b/src/lib/core/grid/row-node.ts @@ -11,6 +11,42 @@ export function buildRowNodes( return nodes } +const DUPLICATES_SHOWN = 5 + +/** + * Two rows sharing an id is the app's bug, but the grid fails at it silently + * and in the worst possible way: the map keeps the last row for an id, and an + * edit addressed to the row the user opened is written to the other one. Said + * out loud in development, where it is cheap, rather than left to be found in + * the data later. + * + * Repeated on every rebuild on purpose. Nothing here remembers what it already + * reported, so no state outlives a grid, and the data stays wrong until it is + * fixed. + */ +function warnDuplicateIds(nodes: RowNode[], unique: number): void { + const seen = new Set() + const repeated = new Set() + for (const node of nodes) { + if (seen.has(node.id)) repeated.add(node.id) + else seen.add(node.id) + } + + const shown = [...repeated].slice(0, DUPLICATES_SHOWN).join(', ') + const rest = repeated.size - DUPLICATES_SHOWN + // The one console statement in the library. A silent wrong write is worse + // than a line in a development console, and there is no other channel: the + // grid has no logger, and an error would take down an app over data it can + // still draw. + // eslint-disable-next-line no-console + console.warn( + `[sv5ui-datagrid] getRowId returned the same id for more than one row: ${shown}` + + (rest > 0 ? ` and ${rest} more` : '') + + `. ${nodes.length} rows share ${unique} ids, so an edit or a selection meant for ` + + 'one of them will land on another. Give every row an id of its own.' + ) +} + /** * Rebuilt whole by the derived that owns it, so a plain Map suffices. Filled * by hand rather than from `map`, which would allocate a second array of pairs @@ -19,6 +55,10 @@ export function buildRowNodes( export function nodesById(nodes: RowNode[]): ReadonlyMap> { const index = new Map>() for (const node of nodes) index.set(node.id, node) + // The whole check, in production: one integer against another. Working out + // which ids collided costs a second pass, and only a build that will print + // it pays for that. + if (index.size !== nodes.length && import.meta.env?.DEV) warnDuplicateIds(nodes, index.size) return index } From c39848478bbd9f30f1a24fa8de470a82b2bb3868 Mon Sep 17 00:00:00 2001 From: nguyenlongdang0412 Date: Wed, 2 Sep 2026 14:24:43 +0700 Subject: [PATCH 5/7] fix(core): refuse a non-finite track where it becomes CSS Closing the width setters and the snapshot boundary closed the routes the issue named, not the failure itself. Three more reach the same line without passing either: a container measured as `NaN`, which makes every flex column `NaNpx` at once, and a column definition written with `width: NaN` or `flex: NaN`. They all converge on `buildColumnCssVars`, so the check belongs there. A value that is not finite falls back to the column's minimum, and an unusable minimum or flex weight falls back to a width that can be drawn. A track of the wrong size is a much smaller failure than a grid that will not lay out at all. The earlier guards stay. They keep unusable values out of the model rather than papering over them at the end, and they let `setWidth` return an honest answer about the width a column still has. --- CHANGELOG.md | 7 ++- src/lib/core/columns/column-sizing.test.ts | 56 ++++++++++++++++++++++ src/lib/core/columns/column-sizing.ts | 30 ++++++++++-- 3 files changed, 87 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7d4338d..29f8456 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,8 +14,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 makes `grid-template-columns` invalid at computed-value time: the browser dropped the declaration, every column folded into one track and the cells stacked down the page, with nothing thrown and nothing logged. Such a width - is now refused both at the snapshot boundary and in `setWidth`/`setWidths`, - where `clamp` and `Math.round` had been carrying `NaN` straight through. + is now refused where it becomes CSS, so no route reaches the property: a + container measured as `NaN`, a definition written with `width: NaN` or + `flex: NaN`, a snapshot carrying one, and `setWidth`/`setWidths`, where + `clamp` and `Math.round` had been passing `NaN` straight through. A track + that cannot be drawn falls back to the column's minimum. - Two rows sharing an id no longer fail silently. The row index keeps the last row for a repeated id, so an edit addressed to the row the user opened was written to the other one and nothing said so. A development build now names diff --git a/src/lib/core/columns/column-sizing.test.ts b/src/lib/core/columns/column-sizing.test.ts index 0ad1185..184225d 100644 --- a/src/lib/core/columns/column-sizing.test.ts +++ b/src/lib/core/columns/column-sizing.test.ts @@ -108,3 +108,59 @@ describe('toStyleString', () => { expect(toStyleString({ '--a': '1px', '--b': '2px' })).toBe('--a: 1px; --b: 2px') }) }) + +describe('a track the browser could not parse', () => { + // Every one of these produced `NaNpx`, which makes grid-template-columns + // invalid at computed-value time: the declaration is dropped, the columns + // fold into one track and the cells stack down the page. + it('never writes a non-finite width, whichever way it arrived', () => { + const fromOverride = buildColumnCssVars([createColumnState({ id: 'a' })], null, null, { + a: Number.NaN + }) + const fromDefinition = buildColumnCssVars([ + createColumnState({ id: 'a', width: Number.NaN }) + ]) + const fromResolved = buildColumnCssVars([createColumnState({ id: 'a' })], [Number.NaN]) + + for (const vars of [fromOverride, fromDefinition, fromResolved]) { + expect(vars['--dg-col-a-w']).not.toContain('NaN') + } + }) + + it('falls back to the column minimum rather than to nothing', () => { + const vars = buildColumnCssVars( + [createColumnState({ id: 'a', minWidth: 64 })], + [Number.NaN] + ) + expect(vars['--dg-col-a-w']).toBe('64px') + }) + + it('reads an unusable flex weight as one', () => { + expect(columnTrackSize(createColumnState({ id: 'a', flex: Number.NaN }))).toBe( + 'minmax(40px, 1fr)' + ) + }) + + it('reads an unusable minimum as a width it can draw', () => { + expect(columnTrackSize(createColumnState({ id: 'a', minWidth: Number.NaN }))).toBe( + 'minmax(100px, 1fr)' + ) + }) + + it('writes a pin offset it can draw', () => { + const vars = buildColumnCssVars([createColumnState({ id: 'a', pinned: 'left' })], [100], { + a: Number.NaN + }) + expect(vars['--dg-col-a-pin']).toBe('0px') + }) + + it('leaves a healthy track exactly as it was', () => { + expect(columnTrackSize(createColumnState({ id: 'a', width: 120 }))).toBe('120px') + expect(columnTrackSize(createColumnState({ id: 'a', flex: 2, minWidth: 80 }))).toBe( + 'minmax(80px, 2fr)' + ) + expect(buildColumnCssVars([createColumnState({ id: 'a' })], [220])['--dg-col-a-w']).toBe( + '220px' + ) + }) +}) diff --git a/src/lib/core/columns/column-sizing.ts b/src/lib/core/columns/column-sizing.ts index d5eba54..249ce12 100644 --- a/src/lib/core/columns/column-sizing.ts +++ b/src/lib/core/columns/column-sizing.ts @@ -42,6 +42,27 @@ export function createColumnState( export type WidthOverrides = Record +/** What a track falls back to when nothing usable is left to fall back on. */ +const LAST_RESORT_WIDTH = 100 + +/** + * The last gate before a number becomes CSS. A non-finite one reaches the + * custom property as `NaNpx`, and `grid-template-columns: var(...)` is then + * invalid at computed-value time: the browser drops the whole declaration, + * every column folds into a single track and the cells stack down the page, + * with nothing thrown and nothing logged. + * + * The width setters and the snapshot boundary each refuse such a value on the + * way in. This is here because they are not the only way in - a container + * measured as `NaN` and a column definition written with `width: NaN` reach + * the same line without passing either - and because a track that is merely + * the wrong size is a far smaller failure than a grid that will not lay out. + */ +function px(value: number | undefined, fallback: number): string { + if (typeof value === 'number' && Number.isFinite(value)) return `${value}px` + return `${Number.isFinite(fallback) ? fallback : LAST_RESORT_WIDTH}px` +} + function effectiveWidth( column: ColumnState, overrides: WidthOverrides @@ -55,9 +76,10 @@ export function columnTrackSize( ): string { const width = effectiveWidth(column, overrides) if (width !== undefined) { - return `${clamp(width, column.minWidth, column.maxWidth)}px` + return px(clamp(width, column.minWidth, column.maxWidth), column.minWidth) } - return `minmax(${column.minWidth}px, ${column.flex ?? 1}fr)` + const flex = typeof column.flex === 'number' && Number.isFinite(column.flex) ? column.flex : 1 + return `minmax(${px(column.minWidth, LAST_RESORT_WIDTH)}, ${flex}fr)` } export function trackWidthEstimates( @@ -106,10 +128,10 @@ export function buildColumnCssVars( const vars: Record = {} visible.forEach((column, index) => { vars[column.cssVar] = resolvedWidths - ? `${resolvedWidths[index]}px` + ? px(resolvedWidths[index], column.minWidth) : columnTrackSize(column, overrides) if (column.pinned && pins) { - vars[column.pinVar] = `${pins[column.id] ?? 0}px` + vars[column.pinVar] = px(pins[column.id], 0) } }) vars['--dg-grid-template'] = visible.map((column) => `var(${column.cssVar})`).join(' ') From 4a4ac67da991f47d31bf1fae7d33183e62dfc168 Mon Sep 17 00:00:00 2001 From: nguyenlongdang0412 Date: Thu, 3 Sep 2026 14:47:32 +0700 Subject: [PATCH 6/7] test(filtering): count debounced writes against the time the typing took The two tests that assert a typed value reaches the filter model once counted writes with no reference to how long the typing took, and the count only means something against that. `GridFilterCell` debounces at 200ms, while the browser driver round-trips once per key, so on a loaded machine the debounce fires between keystrokes and one write per key is the correct behaviour, not a regression. Three release runs on this machine measured 2, 3 and 3 writes for `365` while the same file passed alone in 5s. The bound is now what a working debounce can produce: once per window it is left alone for, plus once at the end. Where the typing fits in one window, on CI and any unloaded machine, that is exactly the 1 the tests asserted before, so nothing is given up where the assertion was meaningful. Where it does not, the test no longer fails the machine instead of the code. The assertion that the model ends holding the typed value is untouched. That is the half which catches the bug these tests were written for, the stale value winning, and it holds at any speed. --- src/tests/filter-row.svelte.test.ts | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/src/tests/filter-row.svelte.test.ts b/src/tests/filter-row.svelte.test.ts index 18e287f..d877ae5 100644 --- a/src/tests/filter-row.svelte.test.ts +++ b/src/tests/filter-row.svelte.test.ts @@ -76,6 +76,27 @@ async function renderGrid(grid: GridState) { return screen } +/** What `GridFilterCell` waits before a typed value reaches the model. */ +const FILTER_DEBOUNCE_MS = 200 + +/** + * Types through the driver and reports the most writes a working debounce could + * have produced. + * + * Counting writes only means something against the time the typing took. The + * browser driver round-trips once per key, and on a loaded machine that round + * trip outlasts the window, so the debounce fires between keystrokes and a + * write per key is correct rather than a regression. A debounce can write once + * per window it is left alone for, plus once at the end, which is the bound + * this returns: exactly 1 when the typing fitted in one window, which is what + * an unloaded machine and CI do. + */ +async function typeWithin(keys: string) { + const started = performance.now() + await userEvent.keyboard(keys) + return Math.floor((performance.now() - started) / FILTER_DEBOUNCE_MS) + 1 +} + describe('the filter row', () => { it('draws one cell per column, and a field only where a filter is declared', async () => { const screen = await renderGrid(makeGrid()) @@ -596,12 +617,12 @@ describe('the data ops demo', () => { const cell = filterCell(screen.container, 0) cell.querySelector('[role="spinbutton"]')!.focus() - await userEvent.keyboard('01052026') + const windows = await typeWithin('01052026') await expect .poll(() => getFiltering(grid)!.columnFilters['joined'], { timeout: 2000 }) .toEqual({ kind: 'date', op: 'equals', value: '2026-01-05' }) - expect(writes).toBe(1) + expect(writes).toBeLessThanOrEqual(windows) await expect.poll(() => bodyRows(screen.container)).toBe(1) }) @@ -615,12 +636,12 @@ describe('the data ops demo', () => { 'input[role="spinbutton"]' )! input.focus() - await userEvent.keyboard('365') + const windows = await typeWithin('365') await expect .poll(() => getFiltering(grid)!.columnFilters['age'], { timeout: 2000 }) .toEqual({ kind: 'number', op: 'eq', value: 365 }) - expect(writes).toBe(1) + expect(writes).toBeLessThanOrEqual(windows) }) it('clears a picked date at once rather than after the wait', async () => { From 417301833c124a64666172d3220330421abf1f32 Mon Sep 17 00:00:00 2001 From: nguyenlongdang0412 Date: Thu, 3 Sep 2026 14:50:23 +0700 Subject: [PATCH 7/7] chore(release): 1.3.1 --- CHANGELOG.md | 3 +++ package.json | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ca3d1cb..4d3e453 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [1.3.1] - 2026-09-03 + ### Fixed - A malformed filter snapshot no longer brings the grid down. `filtering`'s @@ -912,6 +914,7 @@ full table. - Performance budgets in CI as coarse regression ceilings, measured best-of-3 so a loaded machine does not fail a build. +[1.3.1]: https://github.com/ndlabdev/sv5ui-datagrid/releases/tag/v1.3.1 [1.3.0]: https://github.com/ndlabdev/sv5ui-datagrid/releases/tag/v1.3.0 [1.2.0]: https://github.com/ndlabdev/sv5ui-datagrid/releases/tag/v1.2.0 [1.1.0]: https://github.com/ndlabdev/sv5ui-datagrid/releases/tag/v1.1.0 diff --git a/package.json b/package.json index 759a7fa..9ae37c8 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@sv5ui/datagrid", - "version": "1.3.0", + "version": "1.3.1", "description": "A high-performance data grid for Svelte 5, built on sv5ui", "author": "ndlabdev", "license": "MIT",