diff --git a/apps/website/content/docs/grid/clipboard.mdx b/apps/website/content/docs/grid/clipboard.mdx index bd05adec..f16d099e 100644 --- a/apps/website/content/docs/grid/clipboard.mdx +++ b/apps/website/content/docs/grid/clipboard.mdx @@ -41,10 +41,32 @@ keeps its channel-specific default coercion: - `string` / `number` / `boolean` / `bigint` → `String(value)` - plain object → `JSON.stringify(value)` (best-effort fallback) -A range that spans [group rows](/docs/grid/grouping) serializes them too: the -group's label in the derived group column, each aggregate column's -`formatAggregate` output or inherited native `numberFormat`, and an empty cell -everywhere else. +### Group rows + +A range that spans [group rows](/docs/grid/grouping) serializes them too. The +group's **label goes in the leftmost column of the copied range** — of the +range, not of the grid, so a range starting at the third column puts it +there — with each aggregate column's `formatAggregate` output or inherited +native `numberFormat` in its own column, and an empty cell everywhere else: + +{/* prettier-ignore */} +```text +Technology1240000 +``` + +This is the shape Excel's Subtotal and Google Sheets' pivot tables produce, so +a pasted block reads as native rather than as a grid with an extra column. It +carries one accepted cost: when that leftmost column is numeric, a text label +lands in it. It is a header row, spreadsheets tolerate it, and it is what the +incumbents do. + +The **derived group column is never serialized** — not here, not in +[CSV](/docs/grid/export), and it is never a [paste](/docs/grid/paste) target. +The clipboard is a spreadsheet interchange format: Excel and Sheets hand over +exactly as many values as you have real columns, so a synthetic column holding +a slot would put the first pasted value somewhere unwritable and shift every +other value one column right. Copy and paste span the same column space in +opposite directions, which is what makes a round trip land where it started. ### Escaping diff --git a/apps/website/content/docs/grid/export.mdx b/apps/website/content/docs/grid/export.mdx index 014ed3cc..dd2110d8 100644 --- a/apps/website/content/docs/grid/export.mdx +++ b/apps/website/content/docs/grid/export.mdx @@ -176,16 +176,16 @@ export, or narrow the predicate as above. ## Options -| Option | Type | Required | Description | -| ---------------------- | ------------------------------------------- | -------- | -------------------------------------------------------------------------------------------------------- | -| `delimiter` | `string` | no | Field separator, default `","`. Excel follows the OS list separator, which is `;` across much of Europe. | -| `bom` | `boolean` | no | Prepend a UTF-8 BOM, default `true`. Excel does not detect UTF-8 without one. | -| `includeHeaders` | `boolean` | no | Emit the header row, default `true`. | -| `escapeFormulas` | `boolean \| PretableFormulaEscapePredicate` | no | Default `true`, value-vouched as above. | -| `includeGroupRows` | `boolean` | no | Emit group header rows, default `true`. | -| `includeAggregateRows` | `boolean` | no | Emit group aggregate values, default `true`. | -| `columnIds` | `readonly string[]` | no | Column subset **and** order. An unknown id throws rather than narrowing the file in silence. | -| `rowIds` | `ReadonlySet` | no | Restrict to these data rows — how selection-only export is expressed. | +| Option | Type | Required | Description | +| ---------------------- | ------------------------------------------- | -------- | ------------------------------------------------------------------------------------------------------------------------------ | +| `delimiter` | `string` | no | Field separator, default `","`. Excel follows the OS list separator, which is `;` across much of Europe. | +| `bom` | `boolean` | no | Prepend a UTF-8 BOM, default `true`. Excel does not detect UTF-8 without one. | +| `includeHeaders` | `boolean` | no | Emit the header row, default `true`. | +| `escapeFormulas` | `boolean \| PretableFormulaEscapePredicate` | no | Default `true`, value-vouched as above. | +| `includeGroupRows` | `boolean` | no | Emit group header rows, default `true`. The label goes in the file's first column; the derived group column is never exported. | +| `includeAggregateRows` | `boolean` | no | Emit group aggregate values, default `true`. | +| `columnIds` | `readonly string[]` | no | Column subset **and** order. An unknown id throws rather than narrowing the file in silence. | +| `rowIds` | `ReadonlySet` | no | Restrict to these data rows — how selection-only export is expressed. | `TRowId` is your grid's row-id type, not the `string | number` union. That is deliberate: typed against the union, a `Set` on a string-id grid diff --git a/apps/website/content/docs/grid/grouping.mdx b/apps/website/content/docs/grid/grouping.mdx index 6fab4804..0bb73dd4 100644 --- a/apps/website/content/docs/grid/grouping.mdx +++ b/apps/website/content/docs/grid/grouping.mdx @@ -112,7 +112,7 @@ const rowModel = createLocalRowModel({ `snapshot.rowAt(index)` and `snapshot.range(start, end)` return a discriminated `"data" | "group"` union. Group entries expose the typed group path, `childCount`, depth, and finalized aggregates. A collapsed group simply omits its descendants from the index; the snapshot never materializes a nested tree or a complete derived-row array. -While grouping is active, the surface derives one tree column for the group label, twisty, and child count. Configure it with `groupColumn`; it is presentation state, not a member of the model's typed column tuple. +While grouping is active, the surface derives one tree column for the group label, twisty, and child count. Configure it with `groupColumn`; it is presentation state, not a member of the model's typed column tuple — so it is drawn, but never [copied, exported, or pasted into](/docs/grid/clipboard#group-rows). A copied group row carries its label in the leftmost column of the range instead. ## Aggregates and number formatting diff --git a/apps/website/content/docs/grid/paste.mdx b/apps/website/content/docs/grid/paste.mdx index 175d6496..2bb2df15 100644 --- a/apps/website/content/docs/grid/paste.mdx +++ b/apps/website/content/docs/grid/paste.mdx @@ -75,6 +75,7 @@ The **anchor** is the top-left of the target area: - With a selection, the anchor is the top-left of the range that contains the focused cell; if the focus is outside every range, the **first** range wins. - With no selection at all, the anchor is the focused cell and the selection counts as 1 × 1. - A range bound on the synthetic row-select column (a full-row selection) expands to the **full data-column span**, mirroring how [copy](/docs/grid/clipboard) translates that bound. +- A range bound or anchor on the derived group column that [grouping](/docs/grid/grouping) adds resolves to the **first data column**. That column is presentation, never a target, and it occupies no slot in the column space a block tiles across — so a block pasted from a spreadsheet into a grouped grid lands one value per real column, exactly as it does ungrouped. **Multi-range selections are not replayed.** ag-grid writes the same matrix @@ -91,7 +92,7 @@ The **anchor** is the top-left of the target area: the row checkboxes. -Rows are addressed in the grid's **current filtered and sorted order**, not your source array's order. A row filtered out is not a target; the block walks the rows the user is actually looking at, in the order they see them. The synthetic row-select column is never a target. +Rows are addressed in the grid's **current filtered and sorted order**, not your source array's order. A row filtered out is not a target; the block walks the rows the user is actually looking at, in the order they see them. Neither synthetic column — the row-select checkbox nor the derived group column — is ever a target, and neither takes up a column in the target area. ### Shape mismatch diff --git a/apps/website/e2e/grouping.spec.ts b/apps/website/e2e/grouping.spec.ts index 45056dff..d2288c17 100644 --- a/apps/website/e2e/grouping.spec.ts +++ b/apps/website/e2e/grouping.spec.ts @@ -778,13 +778,13 @@ test("grouped Cmd/Ctrl+A copy stays rectangular across labels, leaves, and aggre expect(representative).toHaveLength(7); expect(new Set(representative.map((fields) => fields.length))).toEqual( - new Set([4]), + new Set([3]), ); - expect(representative[0]).toEqual(["", "West", "Holding 01-1-1", "111"]); - expect(representative[1]).toEqual(["", "West", "Holding 01-1-2", "112"]); - expect(representative[5]).toEqual(["Industry 01-2", "", "", "Σ 615"]); - expect(representative[6]).toEqual(["", "West", "Holding 01-2-1", "121"]); - expect(lines).toContain("Sector 02\t\t\tΣ 4560"); + expect(representative[0]).toEqual(["West", "Holding 01-1-1", "111"]); + expect(representative[1]).toEqual(["West", "Holding 01-1-2", "112"]); + expect(representative[5]).toEqual(["Industry 01-2", "", "Σ 615"]); + expect(representative[6]).toEqual(["West", "Holding 01-2-1", "121"]); + expect(lines).toContain("Sector 02\t\tΣ 4560"); }); test("grouped row checkboxes copy every drawn data column without the selector", async ({ @@ -799,7 +799,6 @@ test("grouped row checkboxes copy every drawn data column without the selector", await expect(copyOutput(page)).not.toHaveText(""); expect(((await copyOutput(page).textContent()) ?? "").split("\t")).toEqual([ - "", "West", "Holding 01-1-1", "111", @@ -820,10 +819,10 @@ test("grouped row checkboxes copy every drawn data column without the selector", ).map((line) => line.split("\t")); expect(selectedRows).toHaveLength(ROWS_PER_FIXTURE); expect(new Set(selectedRows.map((fields) => fields.length))).toEqual( - new Set([4]), + new Set([3]), ); - expect(selectedRows[0]).toEqual(["", "West", "Holding 01-1-1", "111"]); - expect(selectedRows.at(-1)).toEqual(["", "East", "Holding 10-4-5", "1045"]); + expect(selectedRows[0]).toEqual(["West", "Holding 01-1-1", "111"]); + expect(selectedRows.at(-1)).toEqual(["East", "Holding 10-4-5", "1045"]); }); /* ------------------------------------------------------------------------- diff --git a/docs/superpowers/specs/2026-08-26-clipboard-spreadsheet-shape-design.md b/docs/superpowers/specs/2026-08-26-clipboard-spreadsheet-shape-design.md new file mode 100644 index 00000000..4421e8eb --- /dev/null +++ b/docs/superpowers/specs/2026-08-26-clipboard-spreadsheet-shape-design.md @@ -0,0 +1,104 @@ +# The clipboard is a spreadsheet interchange format + +Date: 2026-08-26 +Status: approved + +## The product constraint that decides this + +**Most customers use the clipboard to move data in and out of Excel and Google +Sheets. That is the compliance target.** The clipboard is therefore a +_spreadsheet interchange_ format, not an internal round-trip format. Where the +two conflict, the spreadsheet wins. + +## The defect + +When grouping is active the surface prepends a synthetic column +(`pretable-surface.tsx:1862-1873`): + +```js +{ id: GROUP_COLUMN_ID, header: "Group", value: () => "", ... } +``` + +Group rows render the label there; data rows render `""`. Both clipboard sides +treat it as a real field: + +- **copy** (`copy.ts:326`) emits the group label on a group row, and an empty + leading field on a data row. Documented at `clipboard.mdx:44-47`. +- **paste** (`paste.ts:316`) filters only `ROW_SELECT_COLUMN_ID`, so the group + column occupies a target slot. + +Those two are consistent with each other and inconsistent with every +spreadsheet. Excel hands us N values for the N columns a user can see; we tile +them across N+1 slots. **The first value lands in `__pretable_group__`, is +rejected as not-editable, and the rest shift.** Pasting from Excel into a +grouped grid loses the user's first column. + +Measured (#485): removing paste's slot _alone_ inverts the damage — a +copy-then-paste of a grouped row silently blanks column `a` and shifts right. +That is why this is a both-sides change, not a one-liner. + +## Decision + +**The clipboard carries only real data columns.** The synthetic group column is +presentation and does not appear in copy, CSV, or paste. + +**A group row's label moves to the leftmost column of the copied range**, with +aggregates in their own columns: + +``` +Technology1240000 +``` + +This is what Excel's Subtotal produces ("Technology Total" in column A) and what +Sheets pivot tables do, so a pasted block reads as native. Note the label lands +in the leftmost column _of the selected range_, not of the grid — a range that +starts at column C puts the label in C. + +Accepted cost: if that column is numeric, a text label lands in it. It is a +header row and spreadsheets tolerate this; it is what the incumbents do. + +## Scope + +- `copy.ts` — stop emitting a field for `GROUP_COLUMN_ID`; put the group label + in the leftmost column of the range for group rows. +- `csv.ts` — the same change; it has its own emit path (`csv.ts:523`). +- `paste.ts` — drop the `GROUP_COLUMN_ID` slot from `dataColumns`, alongside the + existing `ROW_SELECT_COLUMN_ID` exclusion, and re-anchor an anchor that lands + on it the way `:330` already does for row-select. +- `clipboard.mdx:44-47` — update; the documented behaviour changes. + +## The guard that already exists + +`paste-map.test.ts:495` round-trips a grouped row through +`serializeRanges` → `parseTsv` → `mapPasteToTargets`. Its comment states the +invariant: _"Change both sides together or neither: this test passes under +either arrangement and fails only when they disagree."_ + +So it supports this change and will catch a half-done one. It has one hardcoded +expectation to update — `"\tr0a\tr0b\tr0c\tr0d"` becomes +`"r0a\tr0b\tr0c\tr0d"`; that leading tab **is** the empty group field. Its range +also starts at `GROUP_COLUMN_ID`, which no longer makes sense as a copy bound. + +**Do not weaken this test to make the change pass.** If it fails in a way the +spec did not anticipate, that is a finding. + +## Acceptance + +The bug this exists to fix is _external_ paste, so prove that specifically: + +1. **Excel-shaped paste into a grouped grid.** N values for N visible columns, + pasted at the first data column — every value lands in its own column, and no + `rejected` entry names a synthetic id. +2. **Copy a grouped selection → the text is spreadsheet-shaped.** Rectangular, + N fields per row, group label in the leftmost column of the range. +3. **Round-trip still works** — copy from the grid, paste back, values land + where they came from. This is the side being made to yield; it must still be + correct, just no longer privileged. +4. **Ungrouped behaviour is byte-identical.** Nothing here may touch the + ungrouped path. + +## Out of scope + +Rehoming the label anywhere other than the leftmost column of the range; +changing what group rows render on screen; the HTML clipboard flavour beyond +keeping it consistent with the TSV shape. diff --git a/packages/react/src/__tests__/copy.test.ts b/packages/react/src/__tests__/copy.test.ts index ae04e483..46e63794 100644 --- a/packages/react/src/__tests__/copy.test.ts +++ b/packages/react/src/__tests__/copy.test.ts @@ -334,9 +334,12 @@ describe("serializeRanges escaping", () => { expect(out?.text).toBe("A\tB\n\na1\tb1"); }); - // Sub-project 2 decides what a copied group header emits. Until then it is - // omitted, which keeps the block rectangular over the data rows it spans. - it("omits group header rows spanned by a range", () => { + // Excel's Subtotal shape: a group header keeps its slot in the block, with + // the label in the leftmost column OF THE RANGE and aggregates in their own. + // Dropping the row instead — what this did before the clipboard was made a + // spreadsheet interchange format — silently deleted the only on-screen copy + // of the grouped-by value, since grouping HIDES the column it came from. + it("emits a group header row with its label in the leftmost column", () => { const rowModelSnapshot = createLocalRowModel({ rows, columns: modelColumns, @@ -353,11 +356,15 @@ describe("serializeRanges escaping", () => { columns: baseColumns, copyWithHeaders: false, }); - expect(out?.text).toBe("a1\tb1\na2\tb2\na3\tb3"); - // The HTML flavor walks the same loop, so the group row is skipped there - // too — three , not four. - expect(out?.html?.match(//g)).toHaveLength(3); - expect(out?.html).not.toContain("a2b2a2"); + // r1, then the a2 header, r2, the a3 header, r3 — every line two fields + // wide, because a receiving spreadsheet cannot read a ragged block. + expect(out?.text).toBe("a1\tb1\na2\t\na2\tb2\na3\t\na3\tb3"); + for (const line of out!.text.split("\n")) { + expect(line.split("\t")).toHaveLength(2); + } + // The HTML flavor walks the same loop, so the headers are rows there too. + expect(out?.html?.match(//g)).toHaveLength(5); + expect(out?.html).toContain("a2"); }); }); diff --git a/packages/react/src/__tests__/csv.test.ts b/packages/react/src/__tests__/csv.test.ts index c7e97ab0..c22c0330 100644 --- a/packages/react/src/__tests__/csv.test.ts +++ b/packages/react/src/__tests__/csv.test.ts @@ -385,6 +385,27 @@ describe("serializeCsv group and aggregate rows", () => { expect(file!.rowCount).toBeGreaterThan(rows.length); }); + it("does not export the derived group column, and puts the label in the first", async () => { + // The surface hands over the DRAWN columns, group column included. A file + // one column wider than the grid is a file no spreadsheet can read back — + // and it is the same off-by-one that loses column A on the way in. + const file = serializeCsv({ + rowModelSnapshot: await groupedSnapshot(), + columns: [ + { id: GROUP_COLUMN_ID, header: "Group" }, + { id: "b", header: "B", type: "text" }, + { id: "n", header: "N", type: "number" }, + ], + scope: "all", + options: { bom: false }, + }); + const lines = file!.text.split("\r\n"); + expect(lines[0]).toBe("B,N"); + expect(lines).toEqual(["B,N", "a1,", "b1,1", "a2,", "b2,2"]); + // Rectangular: two fields on every line, group headers included. + for (const line of lines) expect(line.split(",")).toHaveLength(2); + }); + it("omits group rows when includeGroupRows is false", async () => { const file = serializeCsv({ rowModelSnapshot: await groupedSnapshot(), @@ -592,8 +613,12 @@ describe("serializeCsv aggregate rows", () => { }).finished; return serializeCsv({ rowModelSnapshot: model.getState().snapshot, + // A real column ahead of the aggregate one: the group label takes the + // FIRST exported column, so an aggregate sitting there would be + // overwritten by it and this test could not see the option at all. columns: [ { id: GROUP_COLUMN_ID, header: "Group" }, + { id: "a", header: "A", type: "text" }, { id: "n", header: "N", type: "number" }, ], scope: "all", @@ -718,6 +743,7 @@ describe("serializeCsv rowIds — how selection-only export is expressed", () => rowModelSnapshot: model.getState().snapshot, columns: [ { id: GROUP_COLUMN_ID, header: "Group" }, + { id: "b", header: "B", type: "text" }, { id: "n", header: "N", type: "number" }, ], scope: "all", @@ -725,11 +751,12 @@ describe("serializeCsv rowIds — how selection-only export is expressed", () => }); const lines = file!.text.split("\r\n"); - // The selected row survives under its group header. + // The selected row survives under its group header, whose label takes the + // first exported column — the derived group column is not exported. expect(lines).toContain("a1,"); - expect(lines).toContain(",1"); + expect(lines).toContain("b1,1"); // Its sibling's DATA row is gone. - expect(lines).not.toContain(",2"); + expect(lines).not.toContain("b2,2"); // KNOWN BEHAVIOUR, asserted rather than assumed: the sibling's GROUP header // survives with nothing under it. Suppressing it needs lookahead — a group // row is written before its children are known — and AG Grid keeps group diff --git a/packages/react/src/__tests__/paste-map.test.ts b/packages/react/src/__tests__/paste-map.test.ts index faa837bb..9d106ee0 100644 --- a/packages/react/src/__tests__/paste-map.test.ts +++ b/packages/react/src/__tests__/paste-map.test.ts @@ -483,20 +483,22 @@ describe("mapPasteToTargets — the derived group column", () => { { id: "d" }, ]; - // The derived group column is a paste TARGET (it lands in `rejected` as - // `not-editable`, since nothing can be written to it) purely so that the - // column space paste counts across stays the same one copy counts across. - // Copy emits a field for the group column — the group label on a group row, - // an empty field on a data row — so dropping the column from one side alone - // shifts every value by one column on the way back in, silently overwriting - // a real column with copy's empty group field. Change both sides together - // or neither: this test passes under either arrangement and fails only when - // they disagree. + // The clipboard is a SPREADSHEET INTERCHANGE format, so it carries only real + // data columns: the derived group column appears in neither copy's output nor + // paste's target space. The two sides span the same column space in opposite + // directions, so dropping the column from one alone shifts every value by one + // column on the way back in — measured in #485, where removing paste's slot + // by itself blanked column `a` and shifted the rest right. Change both sides + // together or neither: this test passes under either arrangement and fails + // only when they disagree. it("round-trips a whole grouped row back into the columns it came from", () => { const copied = serializeRanges({ ranges: [ + // Bounded on the checkbox: "the whole row", the gesture that copies + // every column. It is the bound that spans the synthetic ones, so it + // is the one that catches a filter applied to only one side. { - start: { rowId: "r0", columnId: GROUP_COLUMN_ID }, + start: { rowId: "r0", columnId: ROW_SELECT_COLUMN_ID }, end: { rowId: "r0", columnId: "d" }, }, ], @@ -505,8 +507,8 @@ describe("mapPasteToTargets — the derived group column", () => { copyWithHeaders: false, locale: "en-US", }); - // A data row's group cell copies as an empty leading field. - expect(copied?.text).toBe("\tr0a\tr0b\tr0c\tr0d"); + // Four fields for the four data columns — no leading empty group field. + expect(copied?.text).toBe("r0a\tr0b\tr0c\tr0d"); const result = mapPasteToTargets({ matrix: parseTsv(copied!.text), @@ -524,4 +526,117 @@ describe("mapPasteToTargets — the derived group column", () => { ).toEqual(["r2:a=r0a", "r2:b=r0b", "r2:c=r0c", "r2:d=r0d"]); expect(result.clipped).toEqual({ rows: 0, columns: 0 }); }); + + // The defect this whole change exists to fix. Excel knows nothing about + // `__pretable_group__`; it hands over exactly as many values as the user can + // see columns. With the synthetic column holding a slot, the first value + // landed in it — rejected as not-editable — and every other value shifted one + // column right, so pasting from Excel into a grouped grid LOST column `a`. + it("lands an Excel-shaped block one value per visible column", () => { + const result = mapPasteToTargets({ + matrix: [["A", "B", "C", "D"]], + // Anchored at the START of the row — a row selection, or a click on the + // tree cell. That is where the slot mattered: the block began at column + // index 0, which was the synthetic one. + anchor: { + ref: { kind: "data", rowId: "r2" }, + columnId: ROW_SELECT_COLUMN_ID, + }, + selectionSize: { rows: 1, columns: 1 }, + rowModelSnapshot: groupedSnapshot, + columns: groupedColumns, + }); + + expect(shape(result)).toEqual(["r2:a=A", "r2:b=B", "r2:c=C", "r2:d=D"]); + // Nothing was aimed at a column the user cannot write to, so nothing will + // come back rejected naming a synthetic id. + expect( + result.cells.filter( + (cell) => + cell.columnId === GROUP_COLUMN_ID || + cell.columnId === ROW_SELECT_COLUMN_ID, + ), + ).toEqual([]); + expect(result.clipped).toEqual({ rows: 0, columns: 0 }); + }); + + it("re-anchors a block anchored ON the group column to the first data column", () => { + // Clicking a group cell and pasting is a real gesture; the group column is + // focusable. It means "start of the row", exactly as the checkbox does. + const result = mapPasteToTargets({ + matrix: [["A", "B"]], + anchor: { ref: { kind: "data", rowId: "r2" }, columnId: GROUP_COLUMN_ID }, + selectionSize: { rows: 1, columns: 1 }, + rowModelSnapshot: groupedSnapshot, + columns: groupedColumns, + }); + expect(shape(result)).toEqual(["r2:a=A", "r2:b=B"]); + }); + + it("copies a GROUP row with its label in the leftmost column of the range", () => { + // Excel's Subtotal shape: the label in the leftmost column, aggregates in + // their own, and the block rectangular — four fields on every row. + // Visible order while grouped: [group x, r0, r1, group y, r2, r3, r4]. A + // range bounded by two data rows spans the group header between them. + expect(groupedSnapshot.rowAt(3)?.kind).toBe("group"); + const copied = serializeRanges({ + ranges: [ + { + start: { rowId: "r0", columnId: "a" }, + end: { rowId: "r2", columnId: "d" }, + }, + ], + rowModelSnapshot: groupedSnapshot, + columns: groupedColumns, + copyWithHeaders: false, + locale: "en-US", + }); + const lines = copied!.text.split("\n"); + expect(lines).toEqual([ + "r0a\tr0b\tr0c\tr0d", + "r1a\tr1b\tr1c\tr1d", + "y\t\t\t", + "r2a\tr2b\tr2c\tr2d", + ]); + // Rectangular: every row carries exactly one field per data column. + for (const line of lines) expect(line.split("\t")).toHaveLength(4); + }); + + it("puts the label in the leftmost column OF THE RANGE, not of the grid", () => { + // A range starting at column `c` puts the label in `c`. Anchoring it to the + // grid's first column instead would drop it out of the copied block. + const copied = serializeRanges({ + ranges: [ + { + start: { rowId: "r1", columnId: "c" }, + end: { rowId: "r2", columnId: "d" }, + }, + ], + rowModelSnapshot: groupedSnapshot, + columns: groupedColumns, + copyWithHeaders: false, + locale: "en-US", + }); + // r1, then the group-y header, then r2 — the label sits in `c`. + expect(copied!.text.split("\n")).toEqual(["r1c\tr1d", "y\t", "r2c\tr2d"]); + }); + + it("treats a range bounded on the group column as starting at the first data column", () => { + // The drawn order is [checkbox, group, a, b, c, d], so dragging from the + // group cell to `b` selects `a` and `b`. Resolving that bound to nothing + // would collapse the range onto `b` alone and lose a column from the copy. + const copied = serializeRanges({ + ranges: [ + { + start: { rowId: "r0", columnId: GROUP_COLUMN_ID }, + end: { rowId: "r0", columnId: "b" }, + }, + ], + rowModelSnapshot: groupedSnapshot, + columns: groupedColumns, + copyWithHeaders: false, + locale: "en-US", + }); + expect(copied?.text).toBe("r0a\tr0b"); + }); }); diff --git a/packages/react/src/__tests__/paste-surface.test.tsx b/packages/react/src/__tests__/paste-surface.test.tsx index 59f23fba..19e79e96 100644 --- a/packages/react/src/__tests__/paste-surface.test.tsx +++ b/packages/react/src/__tests__/paste-surface.test.tsx @@ -10,6 +10,8 @@ import { } from "@testing-library/react"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { GROUP_COLUMN_ID } from "@pretable/core"; + import type { PastePayload } from "../paste"; import { PretableSurface, @@ -1079,3 +1081,120 @@ describe("PretableSurface paste announcements", () => { expect(liveRegion(view)).toHaveTextContent("NOPE"); }); }); + +// The defect this suite's grouped case exists to pin. Grouping prepends a +// SYNTHETIC column the surface derives (`__pretable_group__`) — presentation, +// never editable. Excel knows nothing about it: it hands over exactly as many +// values as the user has real columns. While that column held a paste slot, a +// block anchored at the start of the row put its first value there — rejected +// as not-editable — and shifted every other value one column right, so pasting +// from a spreadsheet into a grouped grid LOST the user's first column. +describe("PretableSurface paste while grouped", () => { + interface GroupedRow extends Record { + id: string; + sector: string; + name: string; + note: string; + qty: number; + } + + const GROUPED_ROWS: GroupedRow[] = [ + { id: "r1", sector: "Tech", name: "Ada", note: "n1", qty: 1 }, + { id: "r2", sector: "Tech", name: "Linus", note: "n2", qty: 2 }, + ]; + + // Grouping by `sector` hides it, so the drawn set is + // [__pretable_group__, name, note, qty] — three real columns. + const GROUPED_COLUMNS: PretableColumn[] = [ + { id: "sector", header: "Sector", type: "text" }, + { id: "name", header: "Name", editable: true }, + { id: "note", header: "Note", editable: true }, + { id: "qty", header: "Qty", type: "number", editable: true }, + ]; + + async function renderGroupedPasteGrid(opts: { + state: PretableSurfaceState; + onPaste: (payload: PastePayload) => void; + }) { + const view = render( + + ariaLabel="grouped-paste-grid" + columns={GROUPED_COLUMNS} + getRowId={(row) => row.id} + initialExpansion={{ kind: "expanded" }} + onPaste={opts.onPaste} + onQueryChange={() => {}} + overscan={0} + query={{ + filters: [], + sort: [], + rowGroups: [{ columnId: "sector" }], + }} + rows={GROUPED_ROWS} + state={opts.state} + viewportHeight={300} + />, + ); + await expect + .poll( + () => + view.container.querySelectorAll("[data-pretable-group-row]").length, + ) + .toBeGreaterThan(0); + return view; + } + + it("lands an Excel-shaped block one value per real column", async () => { + const onPaste = vi.fn(); + // Focus on the group column of a data row: the leftmost cell of the row, + // which is where Home lands and where a click on the tree column lands. + const view = await renderGroupedPasteGrid({ + state: cellSelection("r1", GROUP_COLUMN_ID), + onPaste, + }); + + // Three values for the three columns the user can actually write. + firePaste(view.getByRole("treegrid"), "Ada2\tn9\t7"); + await flush(); + + const payload = onPaste.mock.calls[0]![0] as PastePayload; + expect(payload.cells.map((c) => [c.columnId, c.value])).toEqual([ + ["name", "Ada2"], + ["note", "n9"], + ["qty", 7], + ]); + // Nothing was aimed at a column nobody can write to. + expect(payload.rejected).toEqual([]); + expect( + [...payload.cells, ...payload.rejected].map((c) => c.columnId), + ).not.toContain(GROUP_COLUMN_ID); + expect(payload.clipped).toEqual({ rows: 0, columns: 0 }); + }); + + it("does not report a phantom clipped column for a whole-row selection", async () => { + const onPaste = vi.fn(); + // Dragged from the tree cell to the last column: the whole row, as the + // user sees it. One value tiles across every column of it. + const view = await renderGroupedPasteGrid({ + state: rangeSelection("r1", "r1", GROUP_COLUMN_ID, "qty"), + onPaste, + }); + + // A value every column accepts: `qty` is a number column, so a text value + // would come back rejected and hide the geometry this test is about. + firePaste(view.getByRole("treegrid"), "9"); + await flush(); + + const payload = onPaste.mock.calls[0]![0] as PastePayload; + expect(payload.cells.map((c) => c.columnId)).toEqual([ + "name", + "note", + "qty", + ]); + // The selection is three columns wide, not four. Measuring it against a + // column space that still counts the synthetic one makes the target area + // one wider than anything can be written to, and the paste reports — + // and announces — a column "clipped to fit" that never existed. + expect(payload.clipped).toEqual({ rows: 0, columns: 0 }); + }); +}); diff --git a/packages/react/src/constants.ts b/packages/react/src/constants.ts index 3ad4a9d4..0568ba02 100644 --- a/packages/react/src/constants.ts +++ b/packages/react/src/constants.ts @@ -1,3 +1,5 @@ +import { GROUP_COLUMN_ID } from "@pretable/core"; + /** * Reserved column id for the synthetic row-select checkbox column. * Internal — surface authors shouldn't reference this directly. @@ -15,3 +17,23 @@ export const ROW_SELECT_COLUMN_ID = "__pretable_row_select__"; * @internal */ export const GROUP_PANEL_HEIGHT = 36; + +/** + * True for a column the grid draws but the clipboard must not carry: the + * synthetic row-select checkbox and the derived group column. + * + * The clipboard is a *spreadsheet interchange* format. Excel and Google Sheets + * hand us N values for the N data columns a user can see; a synthetic column + * occupying a paste slot tiles those N values across N+1 targets, so the first + * value lands in a column nothing can be written to and every other value + * shifts one column right. + * + * Copy, CSV and paste must agree on this predicate exactly. They span the same + * column space in opposite directions, so a column dropped from one side alone + * shifts every value by one on the way back through the other. + * + * @internal + */ +export function isSyntheticColumnId(id: string): boolean { + return id === ROW_SELECT_COLUMN_ID || id === GROUP_COLUMN_ID; +} diff --git a/packages/react/src/copy.ts b/packages/react/src/copy.ts index 07c37654..00ff09d4 100644 --- a/packages/react/src/copy.ts +++ b/packages/react/src/copy.ts @@ -5,7 +5,7 @@ import type { PretableRowModelSnapshot, } from "@pretable/core"; -import { ROW_SELECT_COLUMN_ID } from "./constants"; +import { isSyntheticColumnId, ROW_SELECT_COLUMN_ID } from "./constants"; import type { PretableExportScope } from "./csv"; import type { PretableColumn } from "./types"; import { @@ -16,7 +16,6 @@ import { } from "./value-formatting"; import { formatCellValue } from "./rendering"; import { groupLabel } from "./group-model"; -import { GROUP_COLUMN_ID } from "@pretable/core"; // The Blob written by defaultCopyToClipboard carries `type: "text/html"` with // no charset parameter, so state it in the payload itself. @@ -174,10 +173,15 @@ interface RangeBounds { * Resolve one range's id-based bounds to inclusive row/column indices, or * `null` when the range addresses nothing emittable. * - * The synthetic row-select column is positioned BEFORE all data columns in - * `effectiveColumns`. When it appears as a range bound it logically means + * Both synthetic columns — the row-select checkbox and the derived group + * column — are positioned BEFORE all data columns in `effectiveColumns` and are + * absent from `colIndex`. When one appears as a range bound it logically means * "start of the visible row", so it translates to the first data column. A - * range whose *both* ends are the synthetic column has no data to emit. + * range whose *both* ends are the checkbox has no data to emit. + * + * The ungrouped path is byte-identical to the pre-group-column arrangement: + * with no group column drawn, `isSyntheticColumnId` reduces to the checkbox + * test this always made. */ function resolveRangeBounds< TRow extends PretableRow, @@ -205,15 +209,27 @@ function resolveRangeBounds< const rowLo = Math.min(startRow, endRow); const rowHi = Math.max(startRow, endRow); - const startIsSynth = range.start.columnId === ROW_SELECT_COLUMN_ID; - const endIsSynth = range.end.columnId === ROW_SELECT_COLUMN_ID; + // A bound on either synthetic column means "start of the visible row": + // neither is in `colIndex` and both are drawn before every data column. + const startIsSynth = isSyntheticColumnId(range.start.columnId); + const endIsSynth = isSyntheticColumnId(range.end.columnId); const startCol = colIndex.get(range.start.columnId); const endCol = colIndex.get(range.end.columnId); let colLo: number; let colHi: number; if (startIsSynth && endIsSynth) { - return null; + // Two checkboxes address no data cell at all, and emit nothing. Anything + // involving the group column does still cover a real column — dragging + // across the tree cell selects the row's first data column — so it + // collapses onto that one rather than dropping the range. + if ( + range.start.columnId === ROW_SELECT_COLUMN_ID && + range.end.columnId === ROW_SELECT_COLUMN_ID + ) { + return null; + } + colLo = colHi = 0; } else if (startIsSynth && endCol !== undefined) { colLo = 0; colHi = endCol; @@ -279,7 +295,12 @@ export function serializeRangesWithNumberFormatters< args: SerializeRangesArgs, numberFormatters: NumberFormatterRegistry, ): CopyPayload | null { - const dataColumns = args.columns.filter((c) => c.id !== ROW_SELECT_COLUMN_ID); + // Real data columns only. The derived group column is presentation: emitting + // a field for it makes every copied row one column wider than the grid the + // user sees, which is a shape no spreadsheet can interpret. See + // `isSyntheticColumnId` — paste filters the same predicate, and the two must + // change together or values shift by one column on the way back in. + const dataColumns = args.columns.filter((c) => !isSyntheticColumnId(c.id)); if (dataColumns.length === 0) return null; const colIndex = new Map(); @@ -324,7 +345,16 @@ export function serializeRangesWithNumberFormatters< const col = dataColumns[c]!; let text: string; if (row.kind === "group") { - if (col.id === GROUP_COLUMN_ID) { + // The label goes in the leftmost column OF THE RANGE, not of the + // grid: a range starting at column C puts it in C. This is the shape + // Excel's Subtotal and Sheets' pivot tables produce, so a pasted + // block reads as native rather than as an extra column. + // + // It wins over an aggregate on that same column, deliberately: a + // group row with no label is unreadable, and this is the one cell + // whose position is fixed. Accepted cost — a text label lands in a + // numeric column. It is a header row and spreadsheets tolerate that. + if (c === colLo) { text = groupLabel(row.value); } else if ( Object.prototype.hasOwnProperty.call(row.aggregates, col.id) @@ -354,9 +384,11 @@ export function serializeRangesWithNumberFormatters< cells.push(escapeTsvField(text)); rowHtml += `${escapeHtmlText(text)}`; } - if (row.kind === "group" && cells.every((cell) => cell === "")) { - continue; - } + // A group row used to be dropped when every field came out empty — which + // was every group row a range covered without its group column. It now + // always carries its label in the leftmost field (`groupLabel` falls back + // to "(Blanks)" and never returns ""), so the block is rectangular over + // every row it spans and there is nothing left to drop. lines.push(cells.join("\t")); bodyHtml += `${rowHtml}`; } diff --git a/packages/react/src/csv.ts b/packages/react/src/csv.ts index 128d0c27..f87ba7a1 100644 --- a/packages/react/src/csv.ts +++ b/packages/react/src/csv.ts @@ -12,7 +12,6 @@ * formats differently from the clipboard would be a second answer to the same * question. */ -import { GROUP_COLUMN_ID } from "@pretable/core"; import type { ColumnType, PretableExpansionState, @@ -21,7 +20,7 @@ import type { PretableRowModelSnapshot, } from "@pretable/core"; -import { ROW_SELECT_COLUMN_ID } from "./constants"; +import { isSyntheticColumnId } from "./constants"; import { defaultCoerceForCopy } from "./copy"; import { groupLabel } from "./group-model"; import type { PretableColumn } from "./types"; @@ -426,7 +425,10 @@ export function serializeCsvWithNumberFormatters< ? defaultShouldEscapeFormula : options.escapeFormulas; - const drawn = args.columns.filter((c) => c.id !== ROW_SELECT_COLUMN_ID); + // Real data columns only — the derived group column is presentation, and a + // file one column wider than the grid is a file no spreadsheet can read back. + // `copy.ts` filters the same predicate; see `isSyntheticColumnId`. + const drawn = args.columns.filter((c) => !isSyntheticColumnId(c.id)); // `columnIds` selects AND orders. Reading it in the caller's order rather // than filtering the drawn list is the difference between "these columns" // and "these columns, like this" — both grids treat it as the latter. @@ -511,7 +513,7 @@ export function serializeCsvWithNumberFormatters< const cells: string[] = []; - for (const col of dataColumns) { + for (const [columnIndex, col] of dataColumns.entries()) { let text: string; // The value the formatted string came from, when it can vouch for it. // `undefined` means "cannot vouch": a synthesized cell, or a column whose @@ -520,7 +522,12 @@ export function serializeCsvWithNumberFormatters< let vouchRaw: unknown; if (row.kind === "group") { - if (col.id === GROUP_COLUMN_ID) { + // The label goes in the FIRST exported column — the same rule copy.ts + // applies to the leftmost column of a range, and what Excel's Subtotal + // and Sheets' pivot output look like. It wins over an aggregate on that + // column: a group row with no label is unreadable, and this is the one + // cell whose position is fixed. + if (columnIndex === 0) { text = groupLabel(row.value); } else if ( options.includeAggregateRows && @@ -565,11 +572,10 @@ export function serializeCsvWithNumberFormatters< ); } - // Matches copy.ts: a group row that produced nothing is noise, not data. - if (row.kind === "group" && cells.every((cell) => cell === "")) { - rowsSkipped += 1; - continue; - } + // A group row used to be dropped when every field came out empty. It now + // always carries its label in the first column (`groupLabel` falls back to + // "(Blanks)" and never returns ""), so the file is rectangular and there is + // nothing left to drop. Matches copy.ts. lines.push(cells.join(delimiter)); rowCount += 1; diff --git a/packages/react/src/paste.ts b/packages/react/src/paste.ts index d3227d74..683b2eae 100644 --- a/packages/react/src/paste.ts +++ b/packages/react/src/paste.ts @@ -10,7 +10,7 @@ import type { PretableVisibleRowRef, } from "@pretable/core"; -import { ROW_SELECT_COLUMN_ID } from "./constants"; +import { isSyntheticColumnId } from "./constants"; import type { PretableColumn } from "./types"; /** @@ -271,9 +271,15 @@ export interface PasteTargetMap { * counted into `clipped` — target, so a tiled block can clip more rows than it has. * No rows are invented — the data model is controlled. * - * The synthetic row-select column is never a target; when it *is* the anchor (a row - * selection) the block anchors on the first data column instead, mirroring how - * `serializeRanges` translates that bound on copy. + * Neither synthetic column — the row-select checkbox, nor the derived group + * column grouping adds — is ever a target, and neither occupies a slot in the + * column space a block tiles across. The clipboard is a spreadsheet interchange + * format: Excel hands over N values for the N data columns a user can see, and + * a synthetic slot would put the first of them somewhere unwritable and shift + * the rest. When one *is* the anchor (a row selection, or a click on a group + * cell) the block anchors on the first data column instead, mirroring how + * `serializeRanges` translates those bounds on copy. Copy and paste must span + * the same column space or a round trip shifts by one column. * * Group rows are never targets either — they hold aggregates, not editable cells. * They are **removed from the row space** rather than skipped in place, so the @@ -313,7 +319,11 @@ export function mapPasteToTargets< for (const row of args.matrix) blockCols = Math.max(blockCols, row.length); if (blockCols === 0) return empty; - const dataColumns = args.columns.filter((c) => c.id !== ROW_SELECT_COLUMN_ID); + // Real data columns only. Excel hands us N values for the N columns a user + // can see; a synthetic column occupying a slot would tile them across N+1 + // targets and shift every value one column right. `copy.ts` filters the same + // predicate — see `isSyntheticColumnId` for why they must agree exactly. + const dataColumns = args.columns.filter((c) => !isSyntheticColumnId(c.id)); if (dataColumns.length === 0) return empty; const snapshot = args.rowModelSnapshot; @@ -325,11 +335,12 @@ export function mapPasteToTargets< targetRowId = snapshot.nextDataRow(args.anchor.ref)?.rowId; if (targetRowId === undefined) return empty; } - // A row-select anchor means "start of the row" — translate to the first data column. - const anchorCol = - args.anchor.columnId === ROW_SELECT_COLUMN_ID - ? 0 - : dataColumns.findIndex((c) => c.id === args.anchor.columnId); + // An anchor on either synthetic column means "start of the row" — translate + // to the first data column. Both are drawn before every data column and + // neither is a target, so a block anchored on one starts at column 0. + const anchorCol = isSyntheticColumnId(args.anchor.columnId) + ? 0 + : dataColumns.findIndex((c) => c.id === args.anchor.columnId); if (anchorCol < 0) return empty; const extent = (selection: number, block: number): number => diff --git a/packages/react/src/pretable-surface.tsx b/packages/react/src/pretable-surface.tsx index 52b93be4..4dd7eb49 100644 --- a/packages/react/src/pretable-surface.tsx +++ b/packages/react/src/pretable-surface.tsx @@ -134,7 +134,11 @@ import { } from "./group-panel/group-panel-model"; export { ROW_SELECT_COLUMN_ID } from "./constants"; -import { GROUP_PANEL_HEIGHT, ROW_SELECT_COLUMN_ID } from "./constants"; +import { + GROUP_PANEL_HEIGHT, + isSyntheticColumnId, + ROW_SELECT_COLUMN_ID, +} from "./constants"; import { useCellEditController } from "./use-cell-edit-controller"; import { CellEditor } from "./cell-editor"; import { BooleanCellControl } from "./editors/BooleanCellControl"; @@ -7287,7 +7291,11 @@ function resolvePasteAnchor< }; selectionSize: { rows: number; columns: number }; } | null { - const dataColumns = columns.filter((c) => c.id !== ROW_SELECT_COLUMN_ID); + // The same column space `mapPasteToTargets` tiles across: real data columns + // only. Counting a synthetic column here would make `selectionSize.columns` + // one wider than the space the block lands in, so a full-row selection would + // report a phantom clipped column. + const dataColumns = columns.filter((c) => !isSyntheticColumnId(c.id)); if (dataColumns.length === 0 || rowModelSnapshot.visibleRowCount === 0) { return null; } @@ -7330,23 +7338,31 @@ function resolvePasteAnchor< rowId: range.end.rowId, }); if (r1 < 0 || r2 < 0) return null; - const startSynth = range.start.columnId === ROW_SELECT_COLUMN_ID; - const endSynth = range.end.columnId === ROW_SELECT_COLUMN_ID; + // The row-select column MEANS the whole row, so a range bounded on it + // expands to the full data-column span. The derived group column does not + // mean that — it is one drawn column that simply holds no data — so a bound + // on it resolves to the first data column, exactly as `mapPasteToTargets` + // re-anchors it. Expanding it to the whole row instead would turn a click + // on a group cell plus Cmd+V into a value tiled across every column. + const startSelect = range.start.columnId === ROW_SELECT_COLUMN_ID; + const endSelect = range.end.columnId === ROW_SELECT_COLUMN_ID; + const resolveBound = (columnId: string): number | undefined => + isSyntheticColumnId(columnId) ? 0 : colOrder.get(columnId); let colLo: number; let colHi: number; - if (startSynth && endSynth) { + if (startSelect && endSelect) { colLo = 0; colHi = dataColumns.length - 1; - } else if (startSynth || endSynth) { - const other = colOrder.get( - startSynth ? range.end.columnId : range.start.columnId, + } else if (startSelect || endSelect) { + const other = resolveBound( + startSelect ? range.end.columnId : range.start.columnId, ); if (other === undefined) return null; colLo = 0; colHi = other; } else { - const c1 = colOrder.get(range.start.columnId); - const c2 = colOrder.get(range.end.columnId); + const c1 = resolveBound(range.start.columnId); + const c2 = resolveBound(range.end.columnId); if (c1 === undefined || c2 === undefined) return null; colLo = Math.min(c1, c2); colHi = Math.max(c1, c2);