Skip to content

Commit 46ce320

Browse files
bloveclaude
andcommitted
fix(website): resolve the hero's selection summary against the drawn grid
#298 moved the cockpit's selection summary onto the engine's drawn order and explained at length why nothing else can be correct. #321 migrated the hero to the `model` prop, dropped `onGridReady` with it, and put the prop-derived version back. The invariant is now live-broken again, and the guard #298 added does not catch it. Filter the hero to Consumer, drag across two adjacent rows and three columns, and the sidebar reads `9 × 3 selected`. The columns are right; the rows are the distance between those two holdings in the UNFILTERED book, because the row order came from this component's locally sorted array while the drawn set is the filtered one. Rows the user cannot see were counted inside a rectangle they can. Both orders come off the engine again, via the post-#321 API: `columnLayout` for the drawn columns (the pre-#321 `getColumns()`/`getSnapshot().visibleRows` pair is gone) and `range(0, visibleRowCount)` for the drawn rows, mapping group headers by `groupId` since they sit inside the copied rectangle too. Why the existing guard stayed green: grouping.spec.ts groups the hero, hits ⌘A and checks the totals — but grouping swaps the grouped column for the derived one, so the column total lands on the right number read against either order, and ⌘A spans everything, so the row total does too. Counts that coincide are not a test of which order was used. A filter makes the drawn set a strict subset, which no coincidence covers, so that is what the new smoke test pins. The local sort path existed only to feed this call site, so it goes: `userSort`, its model subscription, `sortedRows` and `sortedRowsRef`. `heroGrid/sort.ts` is left on disk, now unreferenced by the app, pending the separate ranking decision below. Verified: website e2e at --workers=1 in both engines against a production build, 99 passed. The one failure, webkit `showcase: scale grid virtualizes`, is pre-existing — reproduced on this branch's base with these two files reverted. Not fixed here, found while verifying: #321 also dropped the hero's default weight-desc ranking. The grid draws arrival order and never re-ranks — measured live, the visible weights read 16.4, 9.7, 8.2, 5, 4.3, 7, 4.5 (four inversions). Restoring it means giving the engine the sort, which makes rows re-rank on every tick, and how much the homepage should churn is a product call, not a drive-by. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 7536e83 commit 46ce320

2 files changed

Lines changed: 78 additions & 37 deletions

File tree

‎apps/website/app/components/HeroGrid.tsx‎

Lines changed: 38 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
import {
44
PretableSurface,
55
type PastePayload,
6-
type PretableSortEntry,
6+
type PretableSurfaceGrid,
77
} from "@pretable/react";
88
import { createLocalRowModel } from "@pretable/core";
99
import { createBatcher } from "@pretable/stream-adapter";
@@ -30,28 +30,32 @@ import { PORTFOLIO_RECORDING } from "./heroGrid/recordings/portfolio";
3030
import { createPortfolioReplay } from "./heroGrid/replay-engine";
3131
import { PortfolioSummary } from "./heroGrid/PortfolioSummary";
3232
import { startingPositions } from "./heroGrid/roster";
33-
import { applySort } from "./heroGrid/sort";
3433
import type { PositionRow } from "./heroGrid/types";
3534
import styles from "./heroGrid/heroGrid.module.css";
3635

36+
type HeroSurfaceGrid = PretableSurfaceGrid<
37+
PositionRow,
38+
string,
39+
ReturnType<typeof makePositionColumns>
40+
>;
41+
3742
const FALLBACK_VIEWPORT_HEIGHT = 520;
3843
/** How long the paste summary stays up before clearing itself. */
3944
const PASTE_SUMMARY_MS = 5000;
4045

4146
export function HeroGrid() {
4247
const { ratePerSec, isPlaying } = useControlState();
4348
const [rows, setRows] = useState<PositionRow[]>([]);
44-
const [userSort, setUserSort] = useState<PretableSortEntry[]>([]);
4549
const replayRef = useRef<ReturnType<typeof createPortfolioReplay> | null>(
4650
null,
4751
);
52+
const gridRef = useRef<HeroSurfaceGrid | null>(null);
4853

4954
// Live rows ref — lets columns factory read current rows without being in its deps
5055
const rowsRef = useRef<PositionRow[]>([]);
5156
useEffect(() => {
5257
rowsRef.current = rows;
5358
}, [rows]);
54-
const sortedRowsRef = useRef<PositionRow[]>([]);
5559

5660
// Stable columns — created once so the grid instance is never recreated under streaming.
5761
// The getRows closure captures the ref *object* (not .current) so it always reads the
@@ -71,30 +75,6 @@ export function HeroGrid() {
7175
}),
7276
);
7377

74-
const sortedRows = useMemo(() => applySort(rows, userSort), [rows, userSort]);
75-
useEffect(
76-
() =>
77-
rowModel.subscribe(() => {
78-
const nextSort = [
79-
...rowModel.getState().snapshot.query.sort,
80-
] as PretableSortEntry[];
81-
setUserSort((currentSort) =>
82-
currentSort.length === nextSort.length &&
83-
currentSort.every(
84-
(entry, index) =>
85-
entry.columnId === nextSort[index]?.columnId &&
86-
entry.direction === nextSort[index]?.direction,
87-
)
88-
? currentSort
89-
: nextSort,
90-
);
91-
}),
92-
[rowModel],
93-
);
94-
useEffect(() => {
95-
sortedRowsRef.current = sortedRows;
96-
}, [sortedRows]);
97-
9878
// Selection / copy state (filtering is uncontrolled — the built-in header
9979
// funnel menus own it)
10080
const [selection, setSelection] = useState<SelectionSummary | null>(null);
@@ -286,15 +266,33 @@ export function HeroGrid() {
286266
[],
287267
);
288268

289-
// onSelectionChange → summarize into row/col counts
290-
const handleSelectionChange = useCallback(
291-
(next: PretableSelectionState) => {
292-
const colOrder = columns.map((column) => column.id);
293-
const rowOrder = sortedRowsRef.current.map((row) => row.id);
294-
setSelection(summarizeSelection(next, colOrder, rowOrder));
295-
},
296-
[columns],
297-
);
269+
// onSelectionChange → summarize into row/col counts.
270+
//
271+
// Both orders come off the engine, never off this component's `columns` or
272+
// its rows: a range is a pair of boundary ids with everything between them
273+
// implied, so it resolves only against the model the grid is DRAWING, and the
274+
// two diverge the moment the grid draws something the props do not carry or
275+
// stops drawing something they do. The synthetic row-select column is drawn
276+
// and is in no prop; grouping adds the derived group column and removes the
277+
// grouped one; a header funnel filters rows out of the drawn set entirely.
278+
//
279+
// `columnLayout` is that drawn column list, and `range(0, visibleRowCount)`
280+
// the drawn row list — group headers included, because they are inside the
281+
// rectangle ⌘C copies and the label speaks for that rectangle.
282+
const handleSelectionChange = useCallback((next: PretableSelectionState) => {
283+
const grid = gridRef.current;
284+
if (grid === null) return;
285+
const { snapshot } = grid.rowModel.getState();
286+
setSelection(
287+
summarizeSelection(
288+
next,
289+
grid.getState().columnLayout.map((column) => column.id),
290+
snapshot
291+
.range(0, snapshot.visibleRowCount)
292+
.map((row) => (row.kind === "data" ? row.rowId : row.groupId)),
293+
),
294+
);
295+
}, []);
298296

299297
// Copy feedback — transient "Copied ✓" toast when ⌘/Ctrl+C fires with a selection
300298
useEffect(() => {
@@ -339,6 +337,9 @@ export function HeroGrid() {
339337
groupPanel={{ enabled: true }}
340338
groupColumn={{ header: "Group" }}
341339
model={rowModel}
340+
onGridReady={(grid) => {
341+
gridRef.current = grid;
342+
}}
342343
onPaste={handlePaste}
343344
onSelectionChange={handleSelectionChange}
344345
rowSelectionColumn={{ enabled: true, headerCheckbox: true }}

‎apps/website/e2e/smoke.spec.ts‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -326,6 +326,46 @@ test("cockpit: filter, edit (guardrail + success), and select+copy under streami
326326
await expect(page.getByText(/selected · ⌘C to copy/i)).toBeVisible();
327327
});
328328

329+
test("cockpit: the selection summary counts the rows the user can see", async ({
330+
page,
331+
}) => {
332+
// The sidebar's "N × M selected" claims to describe the rectangle ⌘C copies.
333+
// A selection range is a pair of boundary ids with everything between them
334+
// implied, so it only means anything against the order the grid is DRAWING.
335+
//
336+
// A filter is the cheapest way to make the drawn order diverge from any
337+
// locally-held one — the drawn set is a subset — and it is a gesture the hero
338+
// invites, so it is the case worth pinning. The grouped case is pinned in
339+
// grouping.spec.ts, but only by counts that happen to coincide there: the
340+
// derived group column replaces the grouped one, so a column total taken
341+
// against the wrong order still lands on the right number.
342+
await page.goto("/", { waitUntil: "domcontentloaded" });
343+
await waitForGridReady(page);
344+
345+
const sectorDialog = await openFilterMenu(page, "Sector");
346+
await sectorDialog
347+
.locator("[data-pretable-filter-set]")
348+
.getByRole("checkbox", { name: "Consumer" })
349+
.check();
350+
await expect(page.locator("[data-pretable-row]")).toHaveCount(6);
351+
await page.keyboard.press("Escape");
352+
353+
// Two adjacent rows ON SCREEN, three columns wide (symbol → sector → qty).
354+
// Those two holdings are far apart in the unfiltered book, so a summary read
355+
// against the whole roster reports the gap between them instead of the two
356+
// rows the user dragged across — it read "9 × 3" for this exact selection.
357+
const rows = page.locator("[data-pretable-row]");
358+
await rows.nth(0).locator('[data-pretable-column-id="symbol"]').click();
359+
await rows
360+
.nth(1)
361+
.locator('[data-pretable-column-id="qty"]')
362+
.click({ modifiers: ["Shift"] });
363+
364+
await expect(page.getByRole("region", { name: "Selection" })).toContainText(
365+
"2 × 3 selected",
366+
);
367+
});
368+
329369
test("cockpit: paste a TSV block into Qty (real clipboard on Chromium)", async ({
330370
page,
331371
context,

0 commit comments

Comments
 (0)