diff --git a/.changeset/tidy-jars-repeat.md b/.changeset/tidy-jars-repeat.md new file mode 100644 index 0000000000..422f6b6966 --- /dev/null +++ b/.changeset/tidy-jars-repeat.md @@ -0,0 +1,9 @@ +--- +'@object-ui/plugin-dashboard': patch +--- + +Fix dataset pivot cells showing another row's numbers when a dimension value contains a space. + +The cross-tab cell key joined the row bucket id and the column bucket id with a plain space, so two rows whose ids met at a different point of the same string produced ONE key — `"New"` × `"York Q1"` and `"New York"` × `"Q1"` both spelled `New York Q1`. The later row silently overwrote the earlier one: the cell showed a different row's measure, the overwritten row's value was unreachable, and drill-through followed the same wrong index into the wrong records. Row and cell ids are now encoded with `JSON.stringify`, which needs no assumption about characters the data will not contain. + +The row-subtotal lookup builds the same row bucket id and now shares that single encoder. It previously rolled its own join, which agreed with the row headers only when a pivot had exactly one row dimension, so the Total column rendered blank for any pivot with three or more dimensions. diff --git a/packages/plugin-dashboard/src/DatasetWidget.tsx b/packages/plugin-dashboard/src/DatasetWidget.tsx index e1bdbdf2a0..2cf7055b72 100644 --- a/packages/plugin-dashboard/src/DatasetWidget.tsx +++ b/packages/plugin-dashboard/src/DatasetWidget.tsx @@ -63,13 +63,45 @@ interface DatasetCapableSource { */ export const buildDrillFilter = buildDatasetDrillFilter; +/** + * Encode a pivot ROW bucket id from its dimension values. + * + * `JSON.stringify` of the value array — not a delimiter character — carries the + * boundary between two values, so the id is unambiguous for ANY value a + * dimension can hold. The previous encodings both relied on a character the + * values were assumed never to contain, which is exactly how two different + * rows collapsed into one bucket (objectstack#5473; same treatment as the + * include key in objectui#3388 and the warning-dedupe key in objectstack#5450). + * + * Every consumer of a row id — the cell index below AND the row-total lookup in + * the cross-tab renderer — must build its key with this function; a second, + * hand-rolled encoding of the same id is what made the old bug invisible. + * + * Known residual, tracked separately in objectstack#5666: a null/undefined + * dimension value is encoded as the placeholder character below, so it still + * collides with a value that literally equals that placeholder. + */ +export const pivotRowId = (dimensionValues: string[]): string => JSON.stringify(dimensionValues); + +/** + * Encode the `cellIndex` key for a (row bucket, column bucket) pair. + * + * Was `${rowId} ${colId}` — a plain space, while dimension values contain + * spaces all the time ("New York", "In Progress"). Two rows whose ids met at a + * different point of the same string ("New" + "York Q1" vs "New York" + "Q1") + * produced ONE key: the later row silently overwrote the earlier one, the cell + * showed another row's measure, and drill-through followed the same wrong + * index. `JSON.stringify` of the pair has no such boundary (objectstack#5473). + */ +export const pivotCellKey = (rowId: string, colId: string): string => JSON.stringify([rowId, colId]); + /** * Pivot flat dataset rows into a cross-tab: `rowDims` go DOWN, `colDim` spreads * ACROSS. Returns ordered row/column headers (display labels from the rows) and - * a map from a `${rowId} ${colId}` cell key to the FLAT row index holding - * that combination's measure values. No re-aggregation — the dataset already - * grouped by every dimension, so each cell maps to exactly one row (the index - * is also what drill-through uses to read `drillRawRows`). + * a map from a `pivotCellKey(rowId, colId)` cell key to the FLAT row index + * holding that combination's measure values. No re-aggregation — the dataset + * already grouped by every dimension, so each cell maps to exactly one row (the + * index is also what drill-through uses to read `drillRawRows`). */ export function buildPivot( rows: Array>, @@ -86,17 +118,17 @@ export function buildPivot( const colSeen = new Set(); const cellIndex = new Map(); rows.forEach((row, index) => { - // The row-dimension separator below is the escaped U+0001 spelling, not the - // byte itself (objectstack#5450). U+0001 does not blind grep the way U+0000 - // does, but written raw it is invisible in every editor and every diff, so no - // reviewer could tell what this separator actually was. The runtime value is - // unchanged: a character no dimension value can carry, which is what keeps - // two dimension values from merging into one ambiguous row id. - const rid = rowDims.map((d) => String(row[d] ?? '∅')).join('\u0001'); + // Both ids are opaque lookup keys, never displayed — the visible text comes + // from `labels`/`label` via formatDimensionValue. The column id stays the + // bare value because a single value needs no boundary; only the row id joins + // several values, and pivotRowId encodes that join unambiguously. (It used to + // join them with a control character no dimension value was ASSUMED to carry; + // pivotRowId needs no such assumption.) + const rid = pivotRowId(rowDims.map((d) => String(row[d] ?? '∅'))); const cid = String(row[colDim] ?? '∅'); if (!rowSeen.has(rid)) { rowSeen.add(rid); rowHeaders.push({ id: rid, labels: rowDims.map((d) => formatDimensionValue(row[d])) }); } if (!colSeen.has(cid)) { colSeen.add(cid); colHeaders.push({ id: cid, label: formatDimensionValue(row[colDim]) }); } - cellIndex.set(`${rid} ${cid}`, index); + cellIndex.set(pivotCellKey(rid, cid), index); }); return { rowHeaders, colHeaders, cellIndex }; } @@ -460,7 +492,7 @@ export function DatasetWidget({ widget, dataSource }: { widget: any; dataSource: const findTotals = (dims: string[]) => state.totals?.find((t) => Array.isArray(t.dimensions) && t.dimensions.join(',') === dims.join(','))?.rows; const rowTotalById = new Map(); - for (const r of findTotals(rowDims) ?? []) rowTotalById.set(rowDims.map((d) => String(r[d] ?? '∅')).join(''), r); + for (const r of findTotals(rowDims) ?? []) rowTotalById.set(pivotRowId(rowDims.map((d) => String(r[d] ?? '∅'))), r); const colTotalById = new Map(); for (const r of findTotals([colDim]) ?? []) colTotalById.set(String(r[colDim] ?? '∅'), r); const grandTotal = findTotals([])?.[0]; @@ -492,7 +524,7 @@ export function DatasetWidget({ widget, dataSource }: { widget: any; dataSource: {lbl} ))} {cellCols.map((cc) => { - const index = pivot.cellIndex.get(`${rh.id} ${cc.col.id}`); + const index = pivot.cellIndex.get(pivotCellKey(rh.id, cc.col.id)); const fr = index != null ? state.rows[index] : undefined; const clickable = canDrill && index != null; const title = [...rh.labels, cc.col.label].filter(Boolean).join(' / '); diff --git a/packages/plugin-dashboard/src/__tests__/DatasetWidget.test.tsx b/packages/plugin-dashboard/src/__tests__/DatasetWidget.test.tsx index 5630055655..8739a00ed5 100644 --- a/packages/plugin-dashboard/src/__tests__/DatasetWidget.test.tsx +++ b/packages/plugin-dashboard/src/__tests__/DatasetWidget.test.tsx @@ -2,10 +2,36 @@ import { describe, it, expect, vi, afterEach } from 'vitest'; import { render, screen, cleanup, waitFor, within, fireEvent } from '@testing-library/react'; -import { DatasetWidget, buildDrillFilter, buildPivot, toCsv } from '../DatasetWidget'; +import { DatasetWidget, buildDrillFilter, buildPivot, pivotCellKey, toCsv } from '../DatasetWidget'; +// The drill-through case below mounts DrillDownDrawer's record list, which is +// rendered through the component registry. Imported at module scope, not in a +// hook: the cold transform must not be billed to a bounded test/hook budget +// (AGENTS.md 测试纪律, objectui#3010). +import '@object-ui/components'; afterEach(cleanup); +/** + * Resolve a cross-tab cell the way the renderer does: find the row/column + * bucket by its DISPLAY labels, then look the pair up with the module's own key + * builder. Tests state which (row, column) combination must resolve to which + * flat row index — never how the key is spelled — so the encoding stays free to + * change while the guarantee (distinct combinations never share a cell) does + * not. Spelling keys literally (`cellIndex.get('Open High')`) is what let a key + * that merged two rows on a space read as correct for so long (objectstack#5473). + */ +const cellAt = ( + p: ReturnType, + rowLabels: string[], + colLabel: string, +): number | undefined => { + const rh = p.rowHeaders.find( + (r) => r.labels.length === rowLabels.length && r.labels.every((l, i) => l === rowLabels[i]), + ); + const ch = p.colHeaders.find((c) => c.label === colLabel); + return rh && ch ? p.cellIndex.get(pivotCellKey(rh.id, ch.id)) : undefined; +}; + const makeSource = (impl: (d: string, s: any) => Promise<{ rows: any[] }>) => ({ queryDataset: vi.fn(impl) }); describe('DatasetWidget', () => { @@ -313,26 +339,60 @@ describe('DatasetWidget', () => { const p = buildPivot(rows, ['status'], 'priority'); expect(p.rowHeaders.map((r) => r.labels[0])).toEqual(['Open', 'Done']); expect(p.colHeaders.map((c) => c.label)).toEqual(['High', 'Low']); - expect(p.cellIndex.get('Open High')).toBe(0); - expect(p.cellIndex.get('Open Low')).toBe(1); - expect(p.cellIndex.get('Done High')).toBe(2); - expect(p.cellIndex.get('Done Low')).toBeUndefined(); // sparse combo absent - }); - - // objectstack#5450. Every case above pivots on ONE row dimension, so the - // separator that joins several of them was never exercised — it could have - // been anything, including the empty string, and the suite stayed green. It - // was a raw U+0001 in the source (invisible in every editor and diff) and is - // now the same character written as an escape; these pin what that character - // is responsible for. - // - // Byte discipline: the expected id is built from a NUMBER via fromCharCode. - // No control character is written into this file, as a literal or an escape. - it('buildPivot keeps two row dimensions apart that would merge without a separator', () => { + expect(cellAt(p, ['Open'], 'High')).toBe(0); + expect(cellAt(p, ['Open'], 'Low')).toBe(1); + expect(cellAt(p, ['Done'], 'High')).toBe(2); + expect(cellAt(p, ['Done'], 'Low')).toBeUndefined(); // sparse combo absent + }); + + // objectstack#5473 — the cell key joined the row id and the column id with a + // PLAIN SPACE, and dimension values contain spaces constantly ("New York", + // "In Progress"). Two rows whose ids met at a different point of the same + // string produced one key, the later row overwrote the earlier one, and the + // cell showed a different row's number with no error anywhere. + it('buildPivot keeps two rows apart whose ids meet at a different point of the same string', () => { + const rows = [ + { region: 'New', quarter: 'York Q1', amount: 111 }, + { region: 'New York', quarter: 'Q1', amount: 222 }, + ]; + const p = buildPivot(rows, ['region'], 'quarter'); + // Two rows in, two cells out — one key for both is the defect. + expect(p.cellIndex.size).toBe(2); + expect(cellAt(p, ['New'], 'York Q1')).toBe(0); + expect(cellAt(p, ['New York'], 'Q1')).toBe(1); + // The combinations no row covers stay absent — a merged key made one of + // them borrow the other row's index instead. + expect(cellAt(p, ['New'], 'Q1')).toBeUndefined(); + expect(cellAt(p, ['New York'], 'York Q1')).toBeUndefined(); + }); + + it('buildPivot keeps multi-dimension rows apart when the dimension values contain spaces', () => { + // The same collision one dimension deeper, and the shape a single-row- + // dimension fixture cannot reach: under the old space join, "New York" · + // "SMB Retail" × "Q1" and "New York" · "SMB" × "Retail Q1" spelled one key. const rows = [ - // "x" + "yz" and "xy" + "z" concatenate to the same string, so an empty - // (or absent) separator collapses these into ONE row header and the second - // row's cell overwrites the first. + { region: 'New York', segment: 'SMB Retail', quarter: 'Q1', amount: 10 }, + { region: 'New York', segment: 'SMB', quarter: 'Retail Q1', amount: 20 }, + ]; + const p = buildPivot(rows, ['region', 'segment'], 'quarter'); + expect(p.cellIndex.size).toBe(2); + expect(cellAt(p, ['New York', 'SMB Retail'], 'Q1')).toBe(0); + expect(cellAt(p, ['New York', 'SMB'], 'Retail Q1')).toBe(1); + }); + + // objectstack#5450 added this case: before it, every pivot fixture had ONE row + // dimension, so the boundary that joins several of them was never exercised — + // it could have been anything, including the empty string, and the suite + // stayed green. That version pinned the ids' literal shape (a control + // character built from a number via fromCharCode); objectstack#5473 replaced + // that encoding, so what is pinned here now is what the boundary is FOR. No + // control character is referenced by this file any more — not as a literal, + // not as an escape, not via fromCharCode. + it('buildPivot keeps two row dimensions apart that would merge without a boundary', () => { + const rows = [ + // "x" + "yz" and "xy" + "z" concatenate to the same string, so an absent + // boundary collapses these into ONE row header and the second row's cell + // overwrites the first. { region: 'x', segment: 'yz', priority: 'High', total: 1 }, { region: 'xy', segment: 'z', priority: 'High', total: 2 }, ]; @@ -344,10 +404,9 @@ describe('DatasetWidget', () => { ['xy', 'z'], ]); - const SEP = String.fromCharCode(0x01); - expect(p.rowHeaders.map((r) => r.id)).toEqual([`x${SEP}yz`, `xy${SEP}z`]); - expect(p.cellIndex.get(`x${SEP}yz High`)).toBe(0); - expect(p.cellIndex.get(`xy${SEP}z High`)).toBe(1); + expect(p.rowHeaders[0].id).not.toBe(p.rowHeaders[1].id); + expect(cellAt(p, ['x', 'yz'], 'High')).toBe(0); + expect(cellAt(p, ['xy', 'z'], 'High')).toBe(1); }); it('renders a pivot (≥2 dims) as a true cross-tab, not a flat table', async () => { @@ -384,6 +443,48 @@ describe('DatasetWidget', () => { expect(screen.queryByTestId('dataset-matrix')).not.toBeInTheDocument(); }); + it('gives each colliding row its OWN cell and drills that cell to ITS record set (objectstack#5473)', async () => { + // End-to-end shape of the defect: two rows whose region/quarter values meet + // at a different point of the same string. The cell key merged them, so the + // table showed 222 twice, 111 was unreachable anywhere, and the "New" cell + // drilled into the OTHER row's records — all with no error. + const src = { + queryDataset: vi.fn(async () => ({ + rows: [ + { region: 'New', quarter: 'York Q1', amount: 111 }, + { region: 'New York', quarter: 'Q1', amount: 222 }, + ], + fields: [ + { name: 'region', type: 'string', label: 'Region' }, + { name: 'quarter', type: 'string', label: 'Quarter' }, + { name: 'amount', type: 'number', label: 'Amount' }, + ], + object: 'showcase_deal', + dimensionFields: { region: 'region', quarter: 'quarter' }, + drillRawRows: [ + { region: 'new', quarter: 'york_q1' }, + { region: 'new_york', quarter: 'q1' }, + ], + })), + find: vi.fn(async () => ({ data: [] })), + getObjectSchema: vi.fn(async () => ({ fields: { region: { type: 'text', label: 'Region' } } })), + }; + render(); + const m = await screen.findByTestId('dataset-matrix'); + + // Column order is first-seen: "York Q1", then "Q1". + const rowNew = within(m).getByText('New').closest('tr') as HTMLElement; + expect([...rowNew.querySelectorAll('td')].map((td) => td.textContent)).toEqual(['New', '111', '—']); + const rowNewYork = within(m).getByText('New York').closest('tr') as HTMLElement; + expect([...rowNewYork.querySelectorAll('td')].map((td) => td.textContent)).toEqual(['New York', '—', '222']); + + // Drill-through reads `drillRawRows` by the same index the cell resolved, + // so a merged cell drilled into the wrong records too. + fireEvent.click(within(rowNew).getByTestId('dataset-drill-cell')); + await waitFor(() => expect(src.find).toHaveBeenCalled()); + expect((src.find.mock.calls[0][1] as any).$filter).toEqual({ region: 'new', quarter: 'york_q1' }); + }); + it('makes matrix cells drillable when the server returns drill metadata', async () => { const src = { queryDataset: vi.fn(async () => ({ rows: [{ status: 'Open', priority: 'High', task_count: 2 }], @@ -439,6 +540,40 @@ describe('DatasetWidget', () => { expect(within(m).getByTestId('matrix-grand-total').textContent).toBe('6'); }); + it('matches server row subtotals to a MULTI-dimension row bucket', async () => { + // The row-total lookup keys the same row bucket ids as the row headers, so + // it has to build them with the same encoder. It used to roll its own join, + // which agreed with the headers only while a pivot had exactly ONE row + // dimension — the shape every fixture in this file had. With two row + // dimensions (a 3-dimension pivot) every lookup missed and the Total column + // rendered blank. Unifying the encoder (objectstack#5473) is what makes + // this pass; the assertion is on the totals, not on the id spelling. + const src = { queryDataset: vi.fn(async () => ({ + rows: [ + { region: 'North', segment: 'SMB', quarter: 'Q1', amount: 1 }, + { region: 'North', segment: 'Enterprise', quarter: 'Q1', amount: 2 }, + ], + fields: [ + { name: 'region', type: 'string', label: 'Region' }, + { name: 'segment', type: 'string', label: 'Segment' }, + { name: 'quarter', type: 'string', label: 'Quarter' }, + { name: 'amount', type: 'number', label: 'Amount' }, + ], + totals: [ + { dimensions: ['region', 'segment'], rows: [ + { region: 'North', segment: 'SMB', amount: 10 }, + { region: 'North', segment: 'Enterprise', amount: 20 }, + ] }, + { dimensions: ['quarter'], rows: [{ quarter: 'Q1', amount: 30 }] }, + { dimensions: [], rows: [{ amount: 30 }] }, + ], + })) }; + render(); + const m = await screen.findByTestId('dataset-matrix'); + expect(within(m).getAllByTestId('matrix-row-total').map((e) => e.textContent)).toEqual(['10', '20']); + expect(within(m).getByTestId('matrix-grand-total').textContent).toBe('30'); + }); + it('renders no totals UI when the server omits totals (older server)', async () => { const src = { queryDataset: vi.fn(async () => ({ rows: [{ status: 'Open', priority: 'High', task_count: 2 }],