Skip to content

Commit 4438a5c

Browse files
bloveclaude
andauthored
fix(react): the group column is not a data column, and the seams that said it was (#497)
* chore: stop a failing package hiding the rest of the suite, and strike a done roadmap line pnpm -r bails on the first failing package by default, so one timeout in an early package silently skipped every package after it — a full run once never reached core, renderer-dom, react or the apps at all, while the failure read as a single known flake. --no-bail runs them all and STILL exits 1 (measured on pnpm 10.12.1; pnpm's own docs claim otherwise). Left on the build step, where a failure should stop everything. Residual: the three phases are still chained with &&, so a package-test failure keeps the apps from running. Fixing that needs exit-code collection, not a flag. Roadmap: "public API/documentation gaps + stable row identity" is done. All four .api.md reports are at zero ae-forgotten-export; getRowId is required at every entry point with its index parameter removed (#293); and four fail-closed guards cover API surface (#290), docs tables (#280), engine reconciliation (#266) and facade forwarding (#301). * fix(react): the group column is not a data column, and the seams that said it was Grouping's boundaries with pinning, editing and streaming had no tests at all, which is where all three prior grouping correctness rounds came from (#259, #264, #495). Probing those three seams found two live defects, both the #495 shape: a consumer-facing "data column" set that filters only the row-select checkbox instead of `isSyntheticColumnId`. Ungrouped the two predicates are the same test, so each site looked correct for as long as nothing was grouped. 1. Announced column counts described a grid one column wider than the artifact. Grouped, Cmd+A then Cmd+C put 2 columns on the clipboard and announced 3; `exportCsv` wrote a 2-column file and announced 3. `copy.ts`, `csv.ts` and `paste.ts` all drop both synthetics, so the announcement — a statement ABOUT that clipboard or that file — has to drop them too. `computeSelectionExtent` now resolves spans in DATA ordinals with a group-column bound collapsing to data column 0, byte-for-byte `resolveRangeBounds`. 2. `onSelectedRowIdChange` went silent while grouped. A range over every real data column is a full row, but `singleFullRowSelection` was handed the drawn list, whose first entry is the synthetic group column — so the range never matched and a consumer's "open the selected record" wiring stopped firing the moment the user grouped. Tests: grouping boundaries added to the three suites that had none — right-pin (drawn order, pinned insets, grouping BY a right-pinned column, `groupColumn: {pinned:"left"}`, group-row alignment, drawn `aria-colcount`), editing (commit inside a group, group rows refuse an editor, an edit cancels when grouping hides its column or collapses its row, a commit that re-paths into a collapsed group settles), and streaming (selection/focus survive an in-place update and a key re-path, expansion and aggregates track, a removed focused row clears focus, an in-flight editor survives a re-path). Every one runs a grouped leg against an ungrouped control. Editing and streaming were probed and found CORRECT — those tests pin behaviour rather than fix it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(website): the hero's selection summary counts what the clipboard carries summarizeSelection filtered only the row-select column, so while grouped it counted the derived group column too — the sidebar claimed one more column than Cmd+C actually copies. Same defect the library seams had, and it is directly in the Excel-interop story: 'N x 3 selected' next to a two-column paste. Two tests pinned the over-count and are corrected, not weakened: - selection.test.ts asserted cols: 3 and its comment claimed the group column 'is inside the rectangle Cmd+C copies'. Rows and columns are not symmetric: group ROWS carry labels and aggregates and do copy; the group COLUMN is presentation and was dropped from copy/CSV/paste in #495. - grouping.spec.ts derived its expectation as headerCells - 1, subtracting only the selector. It now counts headers carrying a data-pretable-column-id and excludes the group column, so it still fails if a hero column is added. Negative control: reverting the filter fails selection.test.ts. * test(react): poll the header assertion that races setQuery's settle external-filter-authority polled for the rows to settle, then asserted the header state bare. setQuery settles asynchronously across cooperative slices (#321), so the rows can be drawn a slice before the header reports — under load the bare assertion loses that race and reads ariaSort 'none' / filterActive 'false'. Not caused by this branch: three full react runs on it produced three different outcomes (eviction-population-change, then this, then 1457 green), which is the signature of a load race rather than a regression. Both suspects pass 3/3 in isolation. Surfaced now because --no-bail (this branch) stops a failing package hiding the ones after it, so more of the suite actually runs. * test(website): drive the stubbed clock inside the wait it gates HeroGrid's replay test stubs requestAnimationFrame, so the replay only advances when a frame is flushed by hand. It flushed exactly two, then waitFor'd for an aggregate to change. If those two did not move THAT sector's value, the retry loop span against a frozen clock until timeout — waiting cannot rescue a value nothing is ticking. Each attempt now flushes another frame, so the loop advances the thing it waits on. HONEST LIMIT: I could not reproduce the CI failure locally to prove this fixes it. The old version passes 6/6 at load ~14; the observed failures were at load 30-50. So this is reasoned, not demonstrated — it removes a real mechanism that can produce exactly this symptom, and is strictly more correct given rAF is stubbed, but it is not a verified fix for that run. NOT caused by this branch, proven structurally rather than statistically: the failing assertions read summary-nav/summary-pnl, which come from buildModel(rows) in PortfolioSummary and depend only on rows. This branch changed summarizeSelection, whose output feeds a sibling SelectionSection. There is no path from the change to those elements. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 48735be commit 4438a5c

12 files changed

Lines changed: 1308 additions & 62 deletions

ROADMAP.md

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,6 @@ See the current [core changelog](packages/core/CHANGELOG.md),
3737

3838
## Now — harden and measure
3939

40-
- Close remaining public API/documentation gaps and make stable row identity
41-
consistent across entry points.
4240
- Add deterministic PMS and financial-planning benchmark profiles without
4341
weakening existing 60 Hz, zero-gap, and interaction-continuity gates.
4442

apps/website/app/components/__tests__/HeroGrid.test.tsx

Lines changed: 21 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -256,21 +256,27 @@ describe("HeroGrid", () => {
256256
const sidebarBefore = screen.getByTestId("summary-pnl").textContent;
257257
expect(screen.getByTestId("summary-nav")).toHaveTextContent("$66.1M");
258258

259-
await act(async () => {
260-
flushAnimationFrame(1_000);
261-
flushAnimationFrame(1_016);
262-
});
263-
264-
await waitFor(() => {
265-
expect(
266-
groupRowNamed("Consumer")!.querySelector(
267-
'[data-pretable-column-id="dayPnl"]',
268-
),
269-
).not.toHaveTextContent(aggregateBefore ?? "");
270-
expect(screen.getByTestId("summary-pnl")).not.toHaveTextContent(
271-
sidebarBefore ?? "",
272-
);
273-
});
259+
// `requestAnimationFrame` is stubbed, so the replay only advances when a
260+
// frame is flushed by hand — `waitFor` alone cannot rescue a value that has
261+
// not been ticked. Two frames were not always enough to move THIS sector's
262+
// aggregate, and the retry loop then span against a frozen clock until it
263+
// timed out. Drive the clock inside the wait instead: each attempt flushes
264+
// another frame, so the loop advances the thing it is waiting on.
265+
let frame = 1_000;
266+
await waitFor(
267+
() => {
268+
flushAnimationFrame((frame += 16));
269+
expect(
270+
groupRowNamed("Consumer")!.querySelector(
271+
'[data-pretable-column-id="dayPnl"]',
272+
),
273+
).not.toHaveTextContent(aggregateBefore ?? "");
274+
expect(screen.getByTestId("summary-pnl")).not.toHaveTextContent(
275+
sidebarBefore ?? "",
276+
);
277+
},
278+
{ timeout: 5_000 },
279+
);
274280
expect(panel).toHaveTextContent("Sector");
275281
expect(
276282
screen.getByRole("treegrid", { name: /live portfolio positions/i }),

apps/website/app/components/heroGrid/__tests__/selection.test.ts

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -96,19 +96,22 @@ describe("summarizeSelection", () => {
9696
).toBeNull();
9797
});
9898

99-
it("counts the derived group column, and group rows, while grouped", () => {
99+
it("counts group rows but NOT the derived group column, while grouped", () => {
100100
// Grouped by c1: the drawn order loses c1 and gains the group column
101101
// after the pinned region, and `visibleRows` interleaves group headers
102-
// with the leaves. Both are inside the rectangle ⌘C copies, so both count
103-
// — a whole-row range here is 3 rows (two leaves and a header) × 3 data
104-
// columns (the group column and the two survivors).
102+
// with the leaves. They are not symmetric. Group ROWS reach the clipboard
103+
// — they carry the label and the aggregates — so they count. The group
104+
// COLUMN does not: it is presentation, dropped from copy/CSV/paste so a
105+
// block pasted into Excel is the shape Excel expects. Counting it would
106+
// make the sidebar claim one more column than ⌘C actually copies.
107+
// So: 3 rows (two leaves and a header) × 2 columns (the survivors).
105108
expect(
106109
summarizeSelection(
107110
sel([["g:A", "r2", ROW_SELECT, "c3"]]),
108111
[ROW_SELECT, GROUP_COLUMN_ID, "c2", "c3"],
109112
["g:A", "r1", "r2", "g:B", "r3"],
110113
),
111-
).toEqual({ rows: 3, cols: 3 });
114+
).toEqual({ rows: 3, cols: 2 });
112115
});
113116
});
114117
});

apps/website/app/components/heroGrid/selection.ts

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,11 @@
1+
import { GROUP_COLUMN_ID } from "@pretable/core";
12
import { ɵROW_SELECT_COLUMN_ID as ROW_SELECT_COLUMN_ID } from "@pretable/react";
3+
4+
// Both synthetic columns are presentation, and neither reaches the clipboard —
5+
// mirroring `isSyntheticColumnId` in @pretable/react. Counting either here makes
6+
// the sidebar claim a wider selection than ⌘C actually copies.
7+
const isSynthetic = (id: string) =>
8+
id === ROW_SELECT_COLUMN_ID || id === GROUP_COLUMN_ID;
29
import type { PretableSelectionState } from "@pretable/core";
310

411
export interface SelectionSummary {
@@ -35,7 +42,7 @@ export function summarizeSelection(
3542
rowOrder: readonly string[],
3643
): SelectionSummary | null {
3744
if (!selection.ranges.length) return null;
38-
const dataColumns = columnOrder.filter((id) => id !== ROW_SELECT_COLUMN_ID);
45+
const dataColumns = columnOrder.filter((id) => !isSynthetic(id));
3946
if (!dataColumns.length) return null;
4047
const rowIdx = new Map(rowOrder.map((id, i) => [id, i]));
4148
const colIdx = new Map(dataColumns.map((id, i) => [id, i]));
@@ -45,8 +52,8 @@ export function summarizeSelection(
4552
const r0 = rowIdx.get(r.startRowId),
4653
r1 = rowIdx.get(r.endRowId);
4754
if (r0 === undefined || r1 === undefined) continue;
48-
const startSynth = r.startColumnId === ROW_SELECT_COLUMN_ID;
49-
const endSynth = r.endColumnId === ROW_SELECT_COLUMN_ID;
55+
const startSynth = isSynthetic(r.startColumnId);
56+
const endSynth = isSynthetic(r.endColumnId);
5057
if (startSynth && endSynth) continue;
5158
const c0 = startSynth ? 0 : colIdx.get(r.startColumnId);
5259
const c1 = endSynth ? 0 : colIdx.get(r.endColumnId);

apps/website/e2e/grouping.spec.ts

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1227,10 +1227,16 @@ test("the hero arrives ungrouped and groups when a header is dragged onto the pa
12271227

12281228
const selection = page.getByRole("region", { name: "Selection" });
12291229
await expect(selection).toContainText(/selected · C to copy/);
1230-
// Every drawn column except the selector, derived from the header row rather
1231-
// than hard-coded so adding a hero column does not silently pass.
1232-
const drawnDataColumns =
1233-
(await page.locator("[data-pretable-header-cell]").count()) - 1;
1230+
// Every drawn column the clipboard actually carries: derived from the header
1231+
// row rather than hard-coded so adding a hero column does not silently pass,
1232+
// and excluding BOTH synthetic columns. The selector header carries no
1233+
// `data-pretable-column-id`; the group column carries its own. Counting
1234+
// either would pin the sidebar claiming a wider selection than ⌘C copies.
1235+
const drawnDataColumns = await page
1236+
.locator(
1237+
'[data-pretable-header-cell][data-pretable-column-id]:not([data-pretable-column-id="__pretable_group__"])',
1238+
)
1239+
.count();
12341240
expect(drawnDataColumns).toBeGreaterThan(1);
12351241
const [rows, cols] = (await selection.innerText())
12361242
.match(/(\d+) × (\d+) selected/)!

package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@
3232
"lint": "pnpm -r lint",
3333
"lint:packaging": "pnpm --filter @pretable/core lint:packaging && pnpm --filter @pretable/react lint:packaging && pnpm --filter @pretable/stream-adapter lint:packaging && pnpm --filter @pretable/ui lint:packaging",
3434
"publish:preflight": "node ./scripts/publish-preflight.mjs",
35-
"test": "node --test scripts/__tests__/analyze-cdp.test.mjs scripts/__tests__/bench-comparator-provenance.test.mjs scripts/__tests__/bench-e2e.test.mjs scripts/__tests__/bench-matrix.test.mjs scripts/__tests__/check-bench-budgets.test.mjs scripts/__tests__/check-prod-deploy.test.mjs scripts/__tests__/check-type-performance.test.mjs scripts/__tests__/public-api-forgotten-exports.test.mjs scripts/__tests__/public-api-symbol-brands.test.mjs scripts/__tests__/publish-preflight.test.mjs scripts/__tests__/publish-public-packages.test.mjs scripts/__tests__/workspace-scripts-own-one-package.test.mjs && pnpm -r --filter './packages/*' build && pnpm -r --filter './packages/*' test && pnpm -r --workspace-concurrency=1 --filter './apps/*' test",
35+
"test": "node --test scripts/__tests__/analyze-cdp.test.mjs scripts/__tests__/bench-comparator-provenance.test.mjs scripts/__tests__/bench-e2e.test.mjs scripts/__tests__/bench-matrix.test.mjs scripts/__tests__/check-bench-budgets.test.mjs scripts/__tests__/check-prod-deploy.test.mjs scripts/__tests__/check-type-performance.test.mjs scripts/__tests__/public-api-forgotten-exports.test.mjs scripts/__tests__/public-api-symbol-brands.test.mjs scripts/__tests__/publish-preflight.test.mjs scripts/__tests__/publish-public-packages.test.mjs scripts/__tests__/workspace-scripts-own-one-package.test.mjs && pnpm -r --filter './packages/*' build && pnpm -r --no-bail --filter './packages/*' test && pnpm -r --no-bail --workspace-concurrency=1 --filter './apps/*' test",
3636
"typecheck": "pnpm -r --filter './packages/*' build && pnpm -r --filter './packages/*' typecheck && pnpm -r --workspace-concurrency=1 --filter './apps/*' typecheck",
3737
"typecheck:performance": "pnpm --filter '@pretable/core...' build && node ./scripts/check-type-performance.mjs",
3838
"typecheck:public": "pnpm --filter '@pretable/react...' build && pnpm exec tsc -p type-tests/tsconfig.json --noEmit && pnpm exec tsc -p type-tests/tsconfig.react.json --noEmit"

0 commit comments

Comments
 (0)