diff --git a/CHANGELOG.md b/CHANGELOG.md index 0b50eea..f2f90ff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,32 @@ deliberate decision, never an accident. ## [Unreleased] +### Fixed: Data Grid edits landing on the wrong row or field +Four ways an edit could be written to the wrong place, all without an error: +- With a pinned column that isn't first in `config.columns` (or after + dragging a column in front of one), editing or pasting into a cell edited + the column next to it. (#127) +- An editor left open while the binding refreshed committed to whichever row + had moved into its position. The editor now stays with its row, keeping + the keyboard focus, and closes if the row or column leaves the view or + another row takes the same id. (#128) +- In cell mode, `onCellEdit`'s `row` held only that one new value, so + persisting it undid an earlier edit of the same row that hadn't been + rebound yet (a two-column paste, or two quick edits). It now carries every + pending edit of the row. (#129) +- A pending edit only cleared when the rebound value matched it, so a + write-back that rejected or normalized the value left the old value shown + and sent again with the row's next edit. It now also clears when the cell's + bound value changes to anything else: the bound data wins. In batch mode + this means someone else's change to a cell drops your unsaved edit of that + cell, instead of Save overwriting it. +- Rows without a `config.idField` value, or sharing one, all answered to the + same id: clicking one selected all of them and Delete removed all of them. + Such rows are now read-only, a Shift-range selection skips them, and a grid + that can select, edit or delete logs a warning. Row ids containing `::` no + longer confuse the pending-edit bookkeeping. (#130) +- CSV export now lists pinned columns first, as the grid shows them. + ## [0.8.0] - 2026-10-06 ### Added: toasts from any script with `system.mustry.toast()` diff --git a/README.md b/README.md index bf2bfef..d982253 100644 --- a/README.md +++ b/README.md @@ -242,10 +242,10 @@ An editable virtualized data grid. Component id `mustrysolutions.perspective.inp ### Features - **Virtualized rendering** — fixed `config.rowHeight` makes row windowing exact; the client-side model is intended for up to ~50k rows (see `/grid-stress` in the verify project). Sticky header, one scroll container, **frozen (pinned) columns**. -- **Columns** (`config.columns`, rendered in array order) — each reads `field` from every row: `header`, `type` (`text`/`number`/`boolean`/`date`/`datetime`), `align`, `width`, `pin`, typed `format`, dropdown `options`, per-column `editable`, declarative validation (`required`/`min`/`max`/`pattern`) and **conditional styling** (`cellStyles` rules matched on the value). +- **Columns** (`config.columns`, rendered in array order with pinned columns first) — each reads `field` from every row: `header`, `type` (`text`/`number`/`boolean`/`date`/`datetime`), `align`, `width`, `pin`, typed `format`, dropdown `options`, per-column `editable`, declarative validation (`required`/`min`/`max`/`pattern`) and **conditional styling** (`cellStyles` rules matched on the value). - **Column gestures, two-way** — drag a header to reorder, drag its edge handle to resize, hide/show via the toolbar column chooser; the user's adjustments persist in `state.columnLayout`. -- **Read interactions, two-way** — `state.sort` (header click cycles asc → desc → off), `state.quickFilter` (case-insensitive contains across configured columns; instant local echo while the prop write round-trips), `state.selection` (`config.rowSelect`: `none` / `single` / `multi`, with Ctrl/Cmd-toggle and Shift-range over the *visible* view). CSV export of the current view (filtered + sorted, configured columns). -- **Controlled editing** (`config.editable` + per-column `editable`) — double-click/Enter/F2/typing opens a **typed editor** (text, decimal, date, datetime-local, dropdown-in-cell; booleans render as live checkboxes). Validation runs before commit; Escape reverts. Number cells are read with `config.locale`'s separators (`1,234.5` in `en`, `1.234,5` in `de`); input whose grouping is malformed or ambiguous — `4,5` in `en`, `1.234` in `fr` — is rejected rather than guessed. The grid **never mutates `data.rows`**: a commit fires `onCellEdit` (old/new value + full row) and overlays the value as *pending* until your write-back rebinds the rows. +- **Read interactions, two-way** — `state.sort` (header click cycles asc → desc → off), `state.quickFilter` (case-insensitive contains across configured columns; instant local echo while the prop write round-trips), `state.selection` (`config.rowSelect`: `none` / `single` / `multi`, with Ctrl/Cmd-toggle and Shift-range over the *visible* view). CSV export of the current view (filtered + sorted, visible columns in display order, pinned first). +- **Controlled editing** (`config.editable` + per-column `editable`) — double-click/Enter/F2/typing opens a **typed editor** (text, decimal, date, datetime-local, dropdown-in-cell; booleans render as live checkboxes). Validation runs before commit; Escape reverts. Number cells are read with `config.locale`'s separators (`1,234.5` in `en`, `1.234,5` in `de`); input whose grouping is malformed or ambiguous — `4,5` in `en`, `1.234` in `fr` — is rejected rather than guessed. The grid **never mutates `data.rows`**: a commit fires `onCellEdit` (old/new value + the full row with every not-yet-rebound edit of that row applied) and overlays the value as *pending* until your write-back rebinds the rows. - **Batch mode** (`config.editMode: 'batch'`) — edits accumulate as dirty cells (italic + dot + "{n} unsaved" badge, `output.dirtyCount`); **Save fires one `onBatchSave`** with every dirty cell and each changed row, Discard reverts them all. - **Excel range paste** — paste a TSV range from a spreadsheet onto the focused cell; each landing cell validates individually. - **Aggregate footers** — per-column `aggregate` summarised over the current view. @@ -255,7 +255,9 @@ An editable virtualized data grid. Component id `mustrysolutions.perspective.inp ### How editing works -Same philosophy as the calendar/timeline: **controlled, read-from-data**. Committed edits overlay the display (`pending`) and clear as soon as any `data.rows` change arrives — your `onCellEdit`/`onBatchSave` script persists and rebinds; the demo at `/grid` in the verify project ships a complete write-back script for both modes. +Same philosophy as the calendar/timeline: **controlled, read-from-data**. Committed edits overlay the display (`pending`) until a `data.rows` update shows that cell with the new value (your write-back landed) or with any other new value (you stored something else, so the bound data wins). Your `onCellEdit`/`onBatchSave` script persists and rebinds; the demo at `/grid` in the verify project ships a complete write-back script for both modes. + +Edits, selection and deletes address rows by `config.idField`, so every row needs its own value there. A row without one, or sharing one with another row, stays visible but read-only: it can't be selected, edited or deleted, and when the grid can select, edit or delete it logs a console warning naming the problem (usually an `idField` whose case doesn't match the query's column). ### Theming diff --git a/common/src/main/resources/datagrid.oncelledit.event.json b/common/src/main/resources/datagrid.oncelledit.event.json index 7cc5933..2a7725a 100644 --- a/common/src/main/resources/datagrid.oncelledit.event.json +++ b/common/src/main/resources/datagrid.oncelledit.event.json @@ -5,6 +5,6 @@ "field": { "type": "string", "description": "The edited column's field." }, "oldValue": { "description": "The value before the edit (null when the cell was empty)." }, "newValue": { "description": "The validated new value, typed per the column (number columns commit numbers)." }, - "row": { "type": "object", "description": "The full row with newValue applied — persist it (e.g. a named-query UPDATE keyed on rowId) and rebind data.rows; the grid shows the new value optimistically until the rebind arrives." } + "row": { "type": "object", "description": "The full row with newValue and any other not-yet-rebound edits of that row applied — persist it (e.g. a named-query UPDATE keyed on rowId) and rebind data.rows; the grid shows the new value optimistically until the rebind arrives." } } } diff --git a/docs/user_manual.md b/docs/user_manual.md index 5441424..cb3cba9 100644 --- a/docs/user_manual.md +++ b/docs/user_manual.md @@ -132,6 +132,9 @@ carries the row, column and new value; your script persists. Typed and pasted numbers follow `config.locale` (empty = the browser's): `1,234.5` in `en`, `1.234,5` in `de`. Input that is malformed or ambiguous for that locale, such as `4,5` in `en`, shows an error instead of being guessed. +Every row needs its own `config.idField` value: a row without one, or +sharing one with another row, is shown read-only and can't be selected or +deleted. ## 8. Pan & Zoom View diff --git a/e2e/tests/grid.spec.ts b/e2e/tests/grid.spec.ts index 913cf5c..45ef9a0 100644 --- a/e2e/tests/grid.spec.ts +++ b/e2e/tests/grid.spec.ts @@ -1,5 +1,27 @@ +import { Page } from '@playwright/test'; import { test, expect, openRoute } from './helpers'; +type Row = Record; + +// A binding refresh, simulated: read or write data.rows through the grid's own +// property store, where a binding update lands. The /grid demo never refreshes +// on its own, and Perspective has no public client API for this, so it walks +// React's internal fiber from the grid's root element to the component. +const GRID_STORE = `(() => { + const el = document.querySelector('.mustry-datagrid'); + let f = el[Object.keys(el).find((k) => k.startsWith('__reactFiber$') || k.startsWith('__reactInternalInstance$'))]; + while (f && !(f.stateNode && f.stateNode.props && f.stateNode.props.store)) f = f.return; + return f.stateNode.props; +})()`; + +const gridRows = (page: Page): Promise => + page.evaluate(`JSON.parse(JSON.stringify(${GRID_STORE}.props.rows))`); + +const rebindRows = (page: Page, rows: Row[]): Promise => + page.evaluate(`${GRID_STORE}.store.props.write('data.rows', ${JSON.stringify(rows)})`); + +const gridRow = (page: Page, wo: string) => page.locator('.mustry-dg-row', { hasText: wo }); + test('grid: renders rows, headers and the aggregate footer', async ({ page }) => { await openRoute(page, '/grid', '.mustry-datagrid'); await expect(page.getByText('WO-10000', { exact: true })).toBeVisible(); @@ -25,3 +47,85 @@ test('grid stress: 50k rows virtualize', async ({ page }) => { await expect(page.getByText('WO-100000', { exact: true })).toBeVisible({ timeout: 60_000 }); await expect(page.locator('.mustry-dg-foot')).toBeVisible(); }); + +// #127: dragging Qty onto the pinned Order column makes the configured order +// [qty, wo, ...] while the grid still draws Order first. Editing the drawn Qty +// cell must edit qty, not the (read-only) order number next to it. +test('grid: editing a cell after a reorder past a pinned column edits that column', async ({ page }) => { + await openRoute(page, '/grid', '.mustry-datagrid'); + const qtyHead = page.getByRole('button', { name: 'Qty', exact: true }); + const orderHead = page.getByRole('button', { name: 'Order', exact: true }); + const from = await qtyHead.boundingBox(); + const to = await orderHead.boundingBox(); + if (!from || !to) throw new Error('grid headers not laid out'); + await page.mouse.move(from.x + 10, from.y + from.height / 2); + await page.mouse.down(); + await page.mouse.move(from.x - 20, from.y + from.height / 2, { steps: 4 }); + await page.mouse.move(to.x + 20, to.y + to.height / 2, { steps: 8 }); + await page.mouse.up(); + const heads = page.locator('.mustry-dg-head-cell'); + await expect(heads.nth(0)).toHaveText('Order'); + await expect(heads.nth(1)).toHaveText('Qty'); + + const firstRow = page.locator('.mustry-dg-row').first(); + const qtyCell = firstRow.locator('.mustry-dg-cell').nth(1); + const shown = (await qtyCell.innerText()).trim(); + await qtyCell.dblclick(); + const editor = qtyCell.locator('.mustry-dg-editor'); + await expect(editor).toBeVisible(); + await expect(editor).toHaveValue(shown); + await editor.fill('123'); + await editor.press('Enter'); + await expect(qtyCell).toHaveText('123'); + await expect(qtyCell).toHaveClass(/mustry-dg-cell--pending/); + await expect(firstRow.locator('.mustry-dg-cell').nth(0)).toHaveText('WO-10000'); +}); + +// #128: rows were keyed by index, so a rebind that shifted the edited row +// remounted the editor's input and dropped the focus: typing and Enter went +// nowhere. Keyed by id, the input moves with its row. +test('grid: an open editor keeps its record and the focus when a rebind inserts a row above', async ({ page }) => { + await openRoute(page, '/grid', '.mustry-datagrid'); + const qty = gridRow(page, 'WO-10003').locator('.mustry-dg-cell').nth(3); + await qty.dblclick(); + const editor = qty.locator('.mustry-dg-editor'); + await expect(editor).toBeFocused(); + + const rows = await gridRows(page); + await rebindRows(page, [{ ...rows[0], wo: 'WO-09999' }, ...rows]); + await expect(page.locator('.mustry-dg-row').first().locator('.mustry-dg-cell').first()).toHaveText('WO-09999'); + await expect(editor).toBeFocused(); + + await page.keyboard.press('ControlOrMeta+a'); + await page.keyboard.type('123'); + await page.keyboard.press('Enter'); + await expect(qty).toHaveText('123'); + await expect(gridRow(page, 'WO-10002').locator('.mustry-dg-cell').nth(3)).not.toHaveText('123'); +}); + +// #130: once another row shares the edited row's id, the id no longer says which +// record the edit is for. The editor closes instead of committing to the first. +test('grid: an open editor closes when a rebind makes its row id a duplicate', async ({ page }) => { + await openRoute(page, '/grid', '.mustry-datagrid'); + await gridRow(page, 'WO-10005').locator('.mustry-dg-cell').nth(3).dblclick(); + await expect(page.locator('.mustry-dg-editor')).toHaveCount(1); + + const rows = await gridRows(page); + await rebindRows(page, rows.map((r) => (r.wo === 'WO-10001' ? { ...r, wo: 'WO-10005' } : r))); + await expect(page.locator('.mustry-dg-editor')).toHaveCount(0); + await expect(page.locator('.mustry-dg-cell--pending')).toHaveCount(0); +}); + +// #130: a Shift-range over a duplicate id must not put that id in the selection. +test('grid: a shift-range selection leaves out rows with a duplicate id', async ({ page }) => { + await openRoute(page, '/grid', '.mustry-datagrid'); + await expect(gridRow(page, 'WO-10002')).toBeVisible(); // the binding has delivered the rows + const rows = await gridRows(page); + await rebindRows(page, rows.map((r) => (r.wo === 'WO-10002' ? { ...r, wo: 'WO-10001' } : r))); + await expect(gridRow(page, 'WO-10001')).toHaveCount(2); + + await gridRow(page, 'WO-10000').locator('.mustry-dg-cell').nth(1).click(); + await gridRow(page, 'WO-10004').locator('.mustry-dg-cell').nth(1).click({ modifiers: ['Shift'] }); + await expect(page.locator('.mustry-dg-selected-badge')).toHaveText('3 selected'); + await expect(page.locator('.mustry-dg-row--selected')).toHaveCount(3); +}); diff --git a/web/typescript/components/__tests__/gridLogic.test.ts b/web/typescript/components/__tests__/gridLogic.test.ts index df9c3b2..0be703c 100644 --- a/web/typescript/components/__tests__/gridLogic.test.ts +++ b/web/typescript/components/__tests__/gridLogic.test.ts @@ -237,6 +237,17 @@ describe('effectiveColumns (two-way layout over config)', () => { expect(out[1].width).toBe(40); // MIN_COL_PX expect(effectiveColumns(cols, LAYOUT0)).toEqual(cols); }); + + // #127: cell positions are display indexes, so this order must match the render. + it('puts pinned columns first, in display order', () => { + const cfg = [col('name'), col('id', { pinned: true })]; + expect(effectiveColumns(cfg, LAYOUT0).map((c) => c.field)).toEqual(['id', 'name']); + const grid = [col('wo', { pinned: true }), col('product'), col('qty')]; + const out = effectiveColumns(grid, { widths: {}, order: ['qty', 'wo', 'product'], hidden: [] }); + expect(out.map((c) => c.field)).toEqual(['wo', 'qty', 'product']); + const laid = columnLayout(out); + expect([...laid.pinned, ...laid.scrolling].map((lc) => lc.col.field)).toEqual(out.map((c) => c.field)); + }); }); describe('reorderFields', () => { @@ -418,13 +429,17 @@ describe('nextCell (keyboard grid navigation)', () => { }); }); -import { aggregateValue, batchPayload, parsePasteMatrix, pastePlan } from '../grid/gridLogic'; +import { + aggregateValue, batchPayload, locateCell, parsePasteMatrix, pastePlan, pendingKey, parsePendingKey, + rowWithPending, settlePending, unusableIdWarning, unusableRowIds +} from '../grid/gridLogic'; describe('batchPayload (Save in batch mode)', () => { const rows = [{ id: 'r1', a: 1, b: 'x' }, { id: 'r2', a: 2, b: 'y' }] as Array>; it('builds per-cell edits and fully-patched changed rows', () => { - const { edits, rows: changed } = batchPayload({ 'r1::a': 9, 'r1::b': 'z', 'r2::a': 5 }, rows, 'id'); + const { edits, rows: changed } = batchPayload( + { [pendingKey('r1', 'a')]: 9, [pendingKey('r1', 'b')]: 'z', [pendingKey('r2', 'a')]: 5 }, rows, 'id'); expect(edits).toHaveLength(3); expect(edits.find((e) => e.rowId === 'r1' && e.field === 'a')).toEqual( { rowId: 'r1', field: 'a', oldValue: 1, newValue: 9 }); @@ -434,7 +449,92 @@ describe('batchPayload (Save in batch mode)', () => { }); it('drops pendings whose row left the dataset', () => { - expect(batchPayload({ 'gone::a': 1 }, rows, 'id').edits).toHaveLength(0); + expect(batchPayload({ [pendingKey('gone', 'a')]: 1 }, rows, 'id').edits).toHaveLength(0); + }); + + it('keeps ids and fields that contain the old :: separator apart (#130)', () => { + const odd = [{ id: 'Line1::Filler', a: 1 }] as Array>; + const { edits } = batchPayload({ [pendingKey('Line1::Filler', 'a')]: 2 }, odd, 'id'); + expect(edits).toEqual([{ rowId: 'Line1::Filler', field: 'a', oldValue: 1, newValue: 2 }]); + expect(parsePendingKey(pendingKey('a::b', 'c::d'))).toEqual({ rowId: 'a::b', field: 'c::d' }); + }); +}); + +describe('rowWithPending (#129: the onCellEdit row carries every pending edit)', () => { + it('applies all pending values of that row and none of the others', () => { + const pending = { [pendingKey('r1', 'a')]: 9, [pendingKey('r1', 'b')]: 'z', [pendingKey('r2', 'a')]: 5 }; + expect(rowWithPending({ id: 'r1', a: 1, b: 'x', c: true }, 'r1', pending)) + .toEqual({ id: 'r1', a: 9, b: 'z', c: true }); + expect(rowWithPending({ id: 'r3', a: 1 }, 'r3', pending)).toEqual({ id: 'r3', a: 1 }); + }); +}); + +describe('unusableRowIds (#130: rows the grid cannot address safely)', () => { + it('flags a missing id and every duplicated id', () => { + const rows = [{ id: 'a' }, { ID: 'b' }, { id: 'c' }, { id: 'c' }, { id: null }, { id: 7 }]; + expect(Array.from(unusableRowIds(rows, 'id')).sort()).toEqual(['', 'c']); + }); + + it('is empty when every row has its own id', () => { + expect(unusableRowIds([{ id: 1 }, { id: 2 }], 'id').size).toBe(0); + }); + + it('flags every row when idField matches no key (case matters)', () => { + const ids = unusableRowIds([{ ID: 1 }, { ID: 2 }], 'id'); + expect(ids.has('')).toBe(true); + }); +}); + +describe('locateCell (#128: an open editor follows its row, not its index)', () => { + const cols = [col('wo'), col('qty')]; + const none = new Set(); + + it('finds the cell at its current position', () => { + expect(locateCell(['n', 'a', 'b'], cols, 'b', 'qty', none)).toEqual({ row: 2, col: 1 }); + }); + + it('returns null when the row or the column is gone', () => { + expect(locateCell(['a'], cols, 'b', 'qty', none)).toBeNull(); + expect(locateCell(['b'], cols, 'b', 'notes', none)).toBeNull(); + }); + + it('returns null once a rebind gives another row the same id', () => { + const ids = ['a', 'b', 'a']; + expect(locateCell(ids, cols, 'a', 'qty', unusableRowIds(ids.map((id) => ({ id })), 'id'))).toBeNull(); + }); +}); + +describe('unusableIdWarning', () => { + it('names missing and duplicate ids', () => { + const msg = unusableIdWarning(new Set(['', 'c']), 'id', true); + expect(msg).toContain('rows without a "id" value'); + expect(msg).toContain('rows with duplicate ids (c)'); + }); + + it('stays quiet when there is nothing wrong or the grid never acts on ids', () => { + expect(unusableIdWarning(new Set(), 'id', true)).toBe(''); + expect(unusableIdWarning(new Set(['']), 'id', false)).toBe(''); + }); +}); + +describe('settlePending (which pending edits a data.rows update clears)', () => { + const k = pendingKey('r1', 'a'); + + it('clears an edit once the bound value matches it', () => { + expect(settlePending({ [k]: 9 }, { [k]: '1' }, [{ id: 'r1', a: 9 }], 'id')).toEqual({}); + }); + + it('keeps an edit while the bound value is unchanged or the row is gone', () => { + expect(settlePending({ [k]: 9 }, { [k]: '1' }, [{ id: 'r1', a: 1 }], 'id')).toBeNull(); + expect(settlePending({ [k]: 9 }, { [k]: '1' }, [{ id: 'r2', a: 1 }], 'id')).toBeNull(); + }); + + // A rejected or normalized write-back would otherwise leave the stale value + // pending, shown, and re-sent in the next onCellEdit row. + it('clears an edit when the author stored something else', () => { + const next = settlePending({ [k]: 9, [pendingKey('r1', 'b')]: 'z' }, { [k]: '1', [pendingKey('r1', 'b')]: 'x' }, + [{ id: 'r1', a: 3, b: 'x' }], 'id'); + expect(next).toEqual({ [pendingKey('r1', 'b')]: 'z' }); }); }); diff --git a/web/typescript/components/grid/DataGrid.tsx b/web/typescript/components/grid/DataGrid.tsx index 64c9131..d8f3296 100644 --- a/web/typescript/components/grid/DataGrid.tsx +++ b/web/typescript/components/grid/DataGrid.tsx @@ -13,9 +13,9 @@ import { DocDismiss } from '../../shared/dismiss'; import { CellPos, ColumnLayout, EditError, GridColumn, GridSort, LaidColumn, MIN_COL_PX, RowRange, aggregateValue, batchPayload, cellText, columnLayout, editDraft, effectiveColumns, - formatCell, gridIsEmpty, gridToCsv, matchStyle, nextCell, nextSelection, nextSort, - parsePasteMatrix, pastePlan, quickFilterRows, reorderFields, sortRows, - validateCell, visibleRowRange + formatCell, gridIsEmpty, gridToCsv, locateCell, matchStyle, nextCell, nextSelection, nextSort, + parsePasteMatrix, pastePlan, pendingKey, quickFilterRows, reorderFields, rowWithPending, + settlePending, sortRows, unusableIdWarning, unusableRowIds, validateCell, visibleRowRange } from './gridLogic'; import { GridProps, mapGridProps } from './gridProps'; import { GridCell, GridHeadCell } from './GridCells'; @@ -24,8 +24,10 @@ import { GridToolbar } from './GridToolbar'; // Must match DataGrid.COMPONENT_ID on the Java side. export const COMPONENT_TYPE = 'mustrysolutions.perspective.input.datagrid'; +// The editor is tied to a row id + field, never to a position: a rebind or a +// layout change while it is open would otherwise retarget it (#128). interface EditState { - pos: CellPos; + rowId: string; field: string; draft: string; error: EditError; @@ -46,9 +48,9 @@ interface DataGridState { // Editing (M2): the roving focused cell and the open editor. focus: CellPos | null; editing: EditState | null; - // Committed-but-not-yet-rebound values, keyed `rowId::field` — the grid never - // mutates data.rows; these overlay the display until the author's write-back - // rebinds the rows (any data.rows change clears them, calendar semantics). + // Committed-but-not-yet-rebound values, keyed pendingKey(rowId, field) — the grid never + // mutates data.rows; these overlay the display until a data.rows update settles + // them (settlePending: the write-back landed, or the bound value changed). pending: Record; } @@ -85,6 +87,7 @@ export class DataGrid extends Component, DataGridState componentDidMount(): void { this.measure(); + this.warnUnusableIds(); if (typeof ResizeObserver !== 'undefined' && this.scrollRef.current) { this.resizeObs = new ResizeObserver(() => this.measure()); this.resizeObs.observe(this.scrollRef.current); @@ -95,7 +98,12 @@ export class DataGrid extends Component, DataGridState if (this.state.filterDraft !== null && this.state.filterDraft === this.props.props.quickFilter) { this.setState({ filterDraft: null }); // the write echoed back; the prop leads again } + const ed = this.state.editing; + if (ed && !this.editTarget(ed)) { + this.setState({ editing: null }); + } this.reconcilePending(); + this.warnUnusableIds(); } componentWillUnmount(): void { @@ -165,14 +173,70 @@ export class DataGrid extends Component, DataGridState return cellText(row[this.props.props.idField]); } + private idsMemo: { deps: unknown[]; bad: Set } | null = null; + private warnedIds = ''; + + /** Ids shared by several rows, or missing: rows the grid must not act on. */ + private unusableIds(): Set { + const p = this.props.props; + const deps = [p.rows, p.idField]; + if (!this.idsMemo || !this.idsMemo.deps.every((d, i) => d === deps[i])) { + this.idsMemo = { deps, bad: unusableRowIds(p.rows, p.idField) }; + } + return this.idsMemo.bad; + } + + private rowUsable(row: Row): boolean { + return !this.unusableIds().has(this.rowId(row)); + } + + private warnUnusableIds(): void { + const p = this.props.props; + const interactive = p.rowSelect !== 'none' || p.editable || p.allowDelete; + const msg = unusableIdWarning(this.unusableIds(), p.idField, interactive); + if (msg !== this.warnedIds) { + this.warnedIds = msg; + if (msg) { + console.warn(msg); + } + } + } + + private viewIdsMemo: { rows: Row[]; ids: string[] } | null = null; + + private viewIds(): string[] { + const view = this.viewRows(); + if (!this.viewIdsMemo || this.viewIdsMemo.rows !== view) { + this.viewIdsMemo = { rows: view, ids: view.map((r) => this.rowId(r)) }; + } + return this.viewIdsMemo.ids; + } + + private locate(rowId: string, field: string): CellPos | null { + return locateCell(this.viewIds(), this.effCols(), rowId, field, this.unusableIds()); + } + + /** The open editor's cell as it is now, or null when it can't be edited any + * more: its row or column left the view, its id became shared, or editing + * was turned off. */ + private editTarget(ed: EditState): { pos: CellPos; row: Row; col: GridColumn } | null { + const pos = this.locate(ed.rowId, ed.field); + if (!pos) { + return null; + } + const row = this.viewRows()[pos.row]; + const col = this.effCols()[pos.col]; + return this.cellEditable(row, col) ? { pos, row, col } : null; + } + /** The cell's current value: a committed-but-unbound edit wins over the prop. */ private cellValue(row: Row, field: string): unknown { - const k = `${this.rowId(row)}::${field}`; + const k = pendingKey(this.rowId(row), field); return k in this.state.pending ? this.state.pending[k] : row[field]; } - private colEditable(col: GridColumn): boolean { - return this.props.props.editable && col.editable; + private cellEditable(row: Row, col: GridColumn): boolean { + return this.props.props.editable && col.editable && this.rowUsable(row); } // --- two-way state writes ------------------------------------------------- @@ -201,11 +265,13 @@ export class DataGrid extends Component, DataGridState private clickRow(row: Row, e: React.MouseEvent): void { const p = this.props.props; - if (p.rowSelect === 'none') { + if (p.rowSelect === 'none' || !this.rowUsable(row)) { return; } const id = this.rowId(row); - const orderedIds = this.viewRows().map((r) => this.rowId(r)); + const bad = this.unusableIds(); + // a Shift-range must not sweep in ids that name no single row + const orderedIds = bad.size ? this.viewIds().filter((v) => !bad.has(v)) : this.viewIds(); const next = nextSelection( p.selection, id, p.rowSelect, { toggle: e.ctrlKey || e.metaKey, range: e.shiftKey }, @@ -238,37 +304,39 @@ export class DataGrid extends Component, DataGridState private startEdit(pos: CellPos, initial?: string): void { const col = this.effCols()[pos.col]; const row = this.viewRows()[pos.row]; - if (!col || !row || !this.colEditable(col)) { + if (!col || !row || !this.cellEditable(row, col)) { return; } const draft = initial !== undefined ? initial : editDraft(this.cellValue(row, col.field), col, this.props.props.locale); - this.setState({ focus: pos, editing: { pos, field: col.field, draft, error: null } }, + this.setState({ focus: pos, editing: { rowId: this.rowId(row), field: col.field, draft, error: null } }, () => this.editorRef.current?.focus()); } - /** Validate + fire + close. Returns false (and stays open) on a validation error. */ - private commitEdit(): boolean { + /** Validate + fire + close. Returns the edited cell's current position once + * closed, or null while it stays open on a validation error. */ + private commitEdit(): CellPos | null { + const fallback = this.state.focus || { row: 0, col: 0 }; const ed = this.state.editing; if (!ed) { - return true; + return fallback; } - const col = this.effCols()[ed.pos.col]; - const row = this.viewRows()[ed.pos.row]; - if (!col || !row) { - this.setState({ editing: null }); - return true; + const target = this.editTarget(ed); + if (!target) { + this.setState({ editing: null }); // never write to whatever is there now + return fallback; } + const { pos, row, col } = target; const { value, error } = validateCell(ed.draft, col, this.props.props.locale); if (error) { this.setState({ editing: { ...ed, error } }); - return false; + return null; } const oldValue = this.cellValue(row, col.field); if (cellText(value) !== cellText(oldValue)) { this.fireCellEdit(row, col.field, oldValue, value); } this.setState({ editing: null }); - return true; + return pos; } private cancelEdit(): void { @@ -276,8 +344,18 @@ export class DataGrid extends Component, DataGridState } private lastDirtyCount = -1; + // Each pending edit's bound value when it was made (cellText), so a rebind + // that stores something else can be told apart from one that hasn't landed. + private pendingBase: Record = {}; private setPending(pending: Record): void { + const base: Record = {}; + Object.keys(pending).forEach((k) => { + if (k in this.pendingBase) { + base[k] = this.pendingBase[k]; + } + }); + this.pendingBase = base; this.setState({ pending }); const n = Object.keys(pending).length; if (n !== this.lastDirtyCount) { @@ -286,31 +364,11 @@ export class DataGrid extends Component, DataGridState } } - /** Drop pending entries whose value now matches the bound data — the author's - * write-back landed. (Identity checks are useless here: the props reducer - * rebuilds the rows array on EVERY prop write, including our own dirtyCount - * write — comparing values is the only reliable signal.) */ + /** Drop the pending entries a data.rows update has settled (settlePending). */ private reconcilePending(): void { - const pending = this.state.pending; - const keys = Object.keys(pending); - if (!keys.length) { - return; - } const p = this.props.props; - const byId = new Map(); - p.rows.forEach((r) => byId.set(this.rowId(r), r)); - const next: Record = {}; - let dropped = false; - for (const k of keys) { - const sep = k.indexOf('::'); - const row = byId.get(k.slice(0, sep)); - if (row && cellText(row[k.slice(sep + 2)]) === cellText(pending[k])) { - dropped = true; // the data caught up with the edit - } else { - next[k] = pending[k]; - } - } - if (dropped) { + const next = settlePending(this.state.pending, this.pendingBase, p.rows, p.idField); + if (next) { this.setPending(next); } } @@ -320,7 +378,9 @@ export class DataGrid extends Component, DataGridState * `acc` is given (multi-cell paste), the overlay entry goes there instead — * setState is async, so per-cell setPending calls would clobber each other. */ private fireCellEdit(row: Row, field: string, oldValue: unknown, newValue: unknown, acc?: Record): void { - const k = `${this.rowId(row)}::${field}`; + const rowId = this.rowId(row); + const k = pendingKey(rowId, field); + this.pendingBase[k] = cellText(row[field]); if (acc) { acc[k] = newValue; } else { @@ -330,11 +390,11 @@ export class DataGrid extends Component, DataGridState return; } this.fireEvent('onCellEdit', { - rowId: this.rowId(row), + rowId, field, oldValue: oldValue === undefined ? null : oldValue, newValue, - row: { ...row, [field]: newValue } + row: rowWithPending(row, rowId, { ...this.state.pending, ...acc, [k]: newValue }) }); } @@ -353,6 +413,9 @@ export class DataGrid extends Component, DataGridState /** A boolean cell's checkbox toggled — commits immediately (no editor). */ private toggleBoolean(row: Row, col: GridColumn): void { + if (!this.cellEditable(row, col)) { + return; + } const cur = this.cellValue(row, col.field); this.fireCellEdit(row, col.field, cur, !(cur === true || cur === 'true')); } @@ -371,10 +434,10 @@ export class DataGrid extends Component, DataGridState } // --- keyboard model ----------------------------------------------------------- - private moveFocus(key: string, shift: boolean): void { + private moveFocus(key: string, shift: boolean, from?: CellPos): void { const cols = this.effCols(); const rows = this.viewRows(); - const cur = this.state.focus || { row: 0, col: 0 }; + const cur = from || this.state.focus || { row: 0, col: 0 }; const pos = nextCell(cur, key === 'Tab' && shift ? 'ShiftTab' : key, rows.length, cols.length); this.setState({ focus: pos }, () => this.scrollCellIntoView(pos)); } @@ -458,6 +521,9 @@ export class DataGrid extends Component, DataGridState for (const t of plan) { const col = cols[t.col]; const row = view[t.row]; + if (!this.rowUsable(row)) { + continue; + } const { value, error } = validateCell(t.draft, col, this.props.props.locale); if (!error) { const oldValue = this.cellValue(row, col.field); @@ -481,8 +547,9 @@ export class DataGrid extends Component, DataGridState e.preventDefault(); const key = e.key; const shift = e.shiftKey; - if (this.commitEdit()) { - this.moveFocus(key, shift); + const pos = this.commitEdit(); + if (pos) { + this.moveFocus(key, shift, pos); this.scrollRef.current?.focus(); } } @@ -592,11 +659,11 @@ export class DataGrid extends Component, DataGridState const { col } = lc; const p = this.props.props; const ed = this.state.editing; - const isEditing = !!(ed && ed.pos.row === pos.row && ed.pos.col === pos.col); + const editable = this.cellEditable(row, col); + const isEditing = !!(editable && ed && ed.rowId === this.rowId(row) && ed.field === col.field); const isFocused = !!(!isEditing && this.state.focus && this.state.focus.row === pos.row && this.state.focus.col === pos.col); const value = this.cellValue(row, col.field); - const editable = this.colEditable(col); const boolEditable = editable && col.type === 'boolean'; return ( , DataGridState styleRule={col.cellStyles.length ? matchStyle(value, col.cellStyles) : null} editable={editable} isFocused={isFocused} - isPending={`${this.rowId(row)}::${col.field}` in this.state.pending} + isPending={pendingKey(this.rowId(row), col.field) in this.state.pending} editor={isEditing ? this.renderEditor(col, ed as EditState, rowHeight) : null} boolChecked={!isEditing && boolEditable ? (value === true || value === 'true') : null} onFocus={() => this.setState({ focus: pos })} @@ -661,7 +728,7 @@ export class DataGrid extends Component, DataGridState private deleteSelected = (): void => { const p = this.props.props; const selected = new Set(p.selection); - const rows = p.rows.filter((r) => selected.has(this.rowId(r))); + const rows = p.rows.filter((r) => selected.has(this.rowId(r)) && this.rowUsable(r)); if (rows.length) { this.fireEvent('onRowsDelete', { rowIds: rows.map((r) => this.rowId(r)), rows }); this.props.store.props.write('state.selection', []); @@ -710,10 +777,13 @@ export class DataGrid extends Component, DataGridState const visible: React.ReactNode[] = []; for (let i = range.first; i <= range.last; i++) { const row = view[i]; - const isSel = p.rowSelect !== 'none' && selected.has(this.rowId(row)); + const id = this.rowId(row); + const isSel = p.rowSelect !== 'none' && selected.has(id) && this.rowUsable(row); visible.push( + // Keyed by id so a rebind that shifts rows moves the DOM with + // them: an open editor keeps its input, and with it the focus.
this.clickRow(row, e)} diff --git a/web/typescript/components/grid/gridLogic.ts b/web/typescript/components/grid/gridLogic.ts index 46ea34e..b6aea15 100644 --- a/web/typescript/components/grid/gridLogic.ts +++ b/web/typescript/components/grid/gridLogic.ts @@ -240,7 +240,9 @@ export interface ColumnLayoutState { /** config.columns with the user's layout applied: hidden filtered out, order * permuted (fields in `order` first, in that order; the rest keep config - * position), widths overridden (clamped). config stays the authoring truth. */ + * position), widths overridden (clamped). config stays the authoring truth. + * Pinned columns come first, as they are drawn: cell positions index this + * list, so it must match the display order (#127). */ export function effectiveColumns(columns: GridColumn[], layout: ColumnLayoutState): GridColumn[] { const hidden = new Set(layout.hidden); const visible = columns.filter((c) => !hidden.has(c.field)); @@ -250,7 +252,7 @@ export function effectiveColumns(columns: GridColumn[], layout: ColumnLayoutStat }; return visible .slice() - .sort((a, b) => pos(a) - pos(b)) + .sort((a, b) => (a.pinned === b.pinned ? pos(a) - pos(b) : a.pinned ? -1 : 1)) .map((c) => { const w = layout.widths[c.field]; return Number.isFinite(w) ? { ...c, width: Math.max(MIN_COL_PX, w) } : c; @@ -502,6 +504,55 @@ export interface CellPos { col: number; // effective-columns index } +/** Where a row/field pair is now drawn, or null when either has left the view + * or the id no longer names one row (`unusable`, see unusableRowIds). + * `viewIds` are the row ids in view order. */ +export function locateCell( + viewIds: string[], columns: GridColumn[], rowId: string, field: string, unusable: Set +): CellPos | null { + if (unusable.has(rowId)) { + return null; + } + const row = viewIds.indexOf(rowId); + const col = columns.findIndex((c) => c.field === field); + return row >= 0 && col >= 0 ? { row, col } : null; +} + +/** Row ids the grid can't act on safely: '' (the row has no idField value) and + * any id shared by two rows. Selecting, editing or deleting by such an id + * would hit other rows too, so those rows are read-only (#130). */ +export function unusableRowIds(rows: Array>, idField: string): Set { + const seen = new Set(); + const bad = new Set(); + for (const r of rows) { + const id = cellText(r[idField]); + if (!id || seen.has(id)) { + bad.add(id); + } + seen.add(id); + } + return bad; +} + +/** The console warning for unusable row ids, or '' when there is nothing to + * say. A grid that can't select, edit or delete never addresses rows by id, + * so missing ids are harmless there and stay quiet. */ +export function unusableIdWarning(bad: Set, idField: string, interactive: boolean): string { + if (!interactive || !bad.size) { + return ''; + } + const ids = Array.from(bad); + const dupes = ids.filter((id) => id); + const what: string[] = []; + if (bad.has('')) { + what.push(`rows without a "${idField}" value`); + } + if (dupes.length) { + what.push(`rows with duplicate ids (${dupes.slice(0, 5).join(', ')}${dupes.length > 5 ? ', ...' : ''})`); + } + return `Data Grid: ${what.join(' and ')} cannot be selected, edited or deleted. Check config.idField.`; +} + /** The next focused cell for an arrow/Tab/Enter step, clamped to the grid. */ export function nextCell(pos: CellPos, key: string, rowCount: number, colCount: number): CellPos { let { row, col } = pos; @@ -534,6 +585,62 @@ export interface PendingEdit { newValue: unknown; } +/** Key of a pending edit. Structured, because ids and fields may contain any + * separator a plain join would use (#130: `Line1::Filler`). */ +export function pendingKey(rowId: string, field: string): string { + return JSON.stringify([rowId, field]); +} + +export function parsePendingKey(key: string): { rowId: string; field: string } { + const [rowId, field] = JSON.parse(key) as [string, string]; + return { rowId, field }; +} + +/** The row with every pending value of that row applied: what the author + * should persist, so one edit's event doesn't undo another's (#129). */ +export function rowWithPending(row: Record, rowId: string, pending: Record): Record { + const out = { ...row }; + Object.keys(pending).forEach((k) => { + const key = parsePendingKey(k); + if (key.rowId === rowId) { + out[key.field] = pending[k]; + } + }); + return out; +} + +/** The pending edits that survive a data.rows update, or null when none clear. + * `base` holds each edit's bound value (as cellText) from when it was made. + * An edit clears once the bound value equals it (the write-back landed) or + * differs from its base (the author stored something else: a rejected, + * normalized or concurrent change, and the bound data wins). It stays while + * the bound value is unchanged or its row is gone. Values compare as text: + * the props reducer rebuilds the rows on every prop write, so identity + * means nothing here. */ +export function settlePending>( + pending: Record, base: Record, rows: T[], idField: string +): Record | null { + const keys = Object.keys(pending); + if (!keys.length) { + return null; + } + const byId = new Map(); + rows.forEach((r) => byId.set(cellText(r[idField]), r)); + const next: Record = {}; + let cleared = false; + for (const k of keys) { + const { rowId, field } = parsePendingKey(k); + const row = byId.get(rowId); + const bound = row ? cellText(row[field]) : ''; + if (row && (bound === cellText(pending[k]) || (k in base && bound !== base[k]))) { + cleared = true; + } else { + next[k] = pending[k]; + } + } + return cleared ? next : null; +} + /** The batch-save payload from the pending map: per-cell edits plus each * changed row with ALL its pending values applied. */ export function batchPayload>( @@ -544,9 +651,7 @@ export function batchPayload>( const edits: PendingEdit[] = []; const changed = new Map>(); Object.keys(pending).forEach((k) => { - const sep = k.indexOf('::'); - const rowId = k.slice(0, sep); - const field = k.slice(sep + 2); + const { rowId, field } = parsePendingKey(k); const row = byId.get(rowId); if (!row) { return; // the row left the dataset while the edit was pending