diff --git a/CHANGELOG.md b/CHANGELOG.md index 20af1ca..ca3d1cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,27 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `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. +- 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 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 + 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 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/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(' ') 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 } 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') + }) +})