feat(table): search, sort, and selection on the CSV data table - #204
Conversation
![obvious-autobuild-staging[bot]](https://avatars.githubusercontent.com/u/31830338?s=60&v=4){kind=small}
There was a problem hiding this comment.
Obvious Code Review
Verdict: COMMENT
Summary
- Blocker: 0
- High: 1
- Medium: 2
High
app/src/components/CsvTable.tsx:166— "N selected" chip vs Copy-as-CSV payload diverge when the filter hides selected rows
Medium
app/src/components/CsvTable.tsx:250— aria-rowcount without aria-rowindex under virtualizationapp/src/lib/tableView.ts:66— numeric sort loses precision above Number.MAX_SAFE_INTEGER
Gates re-verified at head 449e237: eslint clean, tsc clean, 396/396 vitest, production build + prerender, committed dist byte-identical (parity check).
| }; | ||
|
|
||
| const copySelection = () => { | ||
| const rowsInView = view.filter((index) => selected.has(index)); |
There was a problem hiding this comment.
High · reliability — The "N selected" chip renders selected.size (all selected source rows) while copySelection copies only rows still present in the current filtered view (view.filter((i) => selected.has(i))). With a filter active that hides selected rows, the chip overstates the clipboard payload: select 10 rows, filter to 3, chip still reads "10 selected", paste yields 3 data rows — silent divergence, no error, no recovery cue (the hidden rows are invisible under the filter). Make chip and clipboard agree: either render view∩selected count in the chip, or copy the full selected set (view order for visible rows, source order for hidden ones). Note the shipped copy test filters with "200", which matches every fixture row, so this narrowing path is untested — add a test where the filter hides a selected row and assert both chip text and clipboard payload.
There was a problem hiding this comment.
Fixed in 92be714. The chip now counts selectedInView — exactly the rows Copy as CSV emits — and the selection toolbar gates on that count, so chip and clipboard can no longer diverge. Selection itself stays keyed by source index, so a selection hidden by the filter comes back when the filter clears. Added the missing narrowing-path DOM test: select two rows, filter so one hides, assert chip reads "1 selected" and the clipboard payload is the header plus that one row only.
| ref={scrollRef} | ||
| data-testid={testId} | ||
| role="grid" | ||
| aria-rowcount={view.length + 1} |
There was a problem hiding this comment.
Medium · reliability — aria-rowcount={view.length + 1} advertises the full view length on the role="grid" container, but under virtualization only the rendered window exists in the DOM and no row carries aria-rowindex. Assistive technology honoring aria-rowcount derives positions from DOM order, announcing e.g. "row 1 of 12,409" for whichever row sits at the top of the window after scrolling — the announced count and the row positions contradict each other. Complete the ARIA grid contract on the rendered window (aria-rowindex={virtualRow.index + 2} per data row with header = 1, aria-colindex on cells), or drop aria-rowcount until row indices are wired.
There was a problem hiding this comment.
Fixed in 92be714. Completed the grid contract on the rendered window: aria-rowindex={1} on the header row and aria-rowindex={virtualRow.index + 2} on data rows, plus aria-colindex on the gutter (1), header buttons, and every data cell. aria-rowcount stays on the role=grid container and now agrees with the DOM positions. DOM test added asserting the header colindex and a data row's rowindex.
| if (aNan && bNan) return 0; | ||
| return aNan ? 1 : -1; | ||
| } | ||
| return factor * (na - nb); |
There was a problem hiding this comment.
Medium · reliability — The numeric comparator sorts via Number() subtraction (factor * (na - nb)) with no magnitude guard, and numericColumns classifies any all-digit column as numeric. 64-bit id columns (Snowflake/Dynamo/Slack exports — common CSV inputs for this product) lose precision above Number.MAX_SAFE_INTEGER: distinct ids collapse to the same double and mis-sort silently. Compare integer-shaped cells longer than 15 significant digits via BigInt (keep Number for decimals/exponents), and pin it with a unit test on 9007199254740993 vs 9007199254740994.
There was a problem hiding this comment.
Fixed in 92be714. sortRowIndices now compares integer-shaped cells of 16+ digits via BigInt (/^[+-]?\d{16,}$/ on both sides); decimals and exponents keep the Number path. The unit test sorts 9007199254740993 vs 9007199254740994 with a reversed input array — under the old Number comparator both round to the same double, so the stable sort would silently keep the input order, which is exactly the failure this catches.
|
All three findings addressed in 92be714 (chip now counts view∩selected so it always matches the clipboard payload; aria-rowindex/aria-colindex wired across the virtualized window; BigInt comparator for 16+ digit integer cells). Suite at 399 passing, dist rebuilt, gates green. The narrowed-selection path now has the DOM test you asked for. |
There was a problem hiding this comment.
Obvious Code Review — pass 3 (incremental)
Verdict: COMMENT
Pass-2 findings — all resolved
- High (chip vs Copy divergence):
selectedInViewnow feeds both the count chip andcopySelection; pinned by a test that hides a selected row and asserts chip text and clipboard payload. - Medium (aria-rowcount without aria-rowindex): rows now carry
aria-rowindex/aria-colindex; pinned by test. - Medium (numeric precision above 2^53): 16+ digit integer-shaped cells compare via
BigInt; pinned by tests in both directions.
Gates verified at head 92be714: eslint clean, tsc clean, 399/399 vitest, production build + prerender, committed dist byte-identical.
New findings — 0 Blocker, 0 High, 2 Medium
- Medium · reliability — inline on
app/src/components/CsvTable.tsx:355: grid rows still lack cell roles, so the newaria-rowindex/aria-colindexare inert for assistive-tech cell traversal. - Medium · hygiene — body-only (binary files take no line anchor): four debug PNGs are committed at the repo root —
.probe.png,.probe2.png(byte-identical blobs),.tc1.png,.tc2.png. None are referenced by any source, none are gitignored, and they ship to master on the auto-deploy merge. Delete them (and consider adding*.pngroot-level entries or a broader ignore for probe artifacts).
Readiness: CLEAN — no Blocker or High findings open.
| return ( | ||
| <div | ||
| key={c} | ||
| aria-colindex={c + 2} |
There was a problem hiding this comment.
Medium · reliability — The grid contract is still incomplete at the cell level: the children of each role="row" are plain divs (data cells) and a bare button (the gutter), and aria-colindex only conforms on elements with a cell role. Without role="gridcell" (or "cell") on these children, assistive technology cannot traverse the grid cell-by-cell, and the aria-rowindex/aria-colindex values added in this fix never materialize in the accessibility tree. Give each data-cell div role="gridcell" (moving aria-colindex is fine where it is), and wrap the gutter button in a div role="gridcell" aria-colindex={1} so the button keeps its own semantics inside the cell. The row-number header cell has the same gap.
There was a problem hiding this comment.
Fixed in 13294b5. Every child of a role=row is now a real cell role: data-cell divs carry role="gridcell" (aria-colindex c+2), the gutter button is wrapped in a role="gridcell" aria-colindex={1} div so the button keeps its own semantics inside the cell, and the header gutter cell got role="gridcell" aria-colindex={1}. The DOM test now asserts the full cell contract — 3 gridcells per row, colindex 1/2/2, button semantics preserved inside the first cell.
|
Both pass-3 findings addressed in 13294b5: gridcell roles across header gutter, row gutter wrapper, and data cells (pinned by the updated ARIA test), and the four debug PNGs deleted with a root-level /*.png gitignore rule so probe captures cannot be swept into a commit again. Gates green locally at this head: tsc, eslint, 399/399 vitest, build + prerender + SEO, dist rebuilt and committed. |
There was a problem hiding this comment.
Obvious Code Review — pass 4 (incremental)
Verdict: COMMENT
Pass-3 findings — all resolved
- Medium (grid cell roles): the gutter button is now wrapped in a
role="gridcell"container (the button keeps its own semantics inside), data cells carryrole="gridcell"alongsidearia-colindex, and the row-number header cell got the same treatment. The updated test asserts three real cells per row with the gutter button's accessible name intact. - Medium (committed debug PNGs): all four root-level captures deleted, and
.gitignoregains a commented root-level/*.pngrule so the debris class can't return.
No re-introductions: chip/copy agreement, aria-rowindex/aria-colindex, and the BigInt comparator are all unchanged.
Gates verified at head 13294b5: eslint clean, tsc clean, 399/399 vitest, production build + prerender, committed dist byte-identical.
No findings — clean pass.
|
Clean pass noted — no action needed on my side. PR is at head 13294b5 with all three evidence items (desktop, 375px, flow WebM) recorded against it, CI green, and zero open findings. Ready for merge. |
1 participant
Why
The CSV table renders thousands of rows but offers no way to navigate them: finding a row means scrolling, understanding a column means eyeballing, and extracting a subset means re-filtering the source text manually. This ships the first stage of the table superpowers spec (search, sort, select) on both table instances —
input-tableandoutput-tableshare the same component, so both get the toolbar for free.What
Search (criterion 1). A dense toolbar strip above the sticky header: search input, live count chip ("340 of 12,408"), clear via × button and Esc. Debounced ~150ms, case-insensitive substring across all cells; only matching rows render, and matches are highlighted in-cell with
<mark>plus amarkfallback that survives with or without color (never color-alone).Sort (criterion 2). Header cells are now buttons cycling asc → desc → natural. Numeric columns (existing
numericColumnsdetection) compare numerically with non-numeric values forced last; text useslocaleCompare. Single-column only. Headers exposearia-sortand a direction glyph.Selection + copy (criterion 3). Row-number gutter click selects, Shift-click selects a range, Cmd/Ctrl-click toggles. Selection is a
Setof source row indices, so it survives filtering and sorting. The toolbar shows "N selected", Copy-as-CSV (view order, proper RFC-4180 serialization via the sharedserializeCsvRow), and Clear. Rows exposearia-selected.Row-number fidelity (criterion 4). The gutter always shows the source row number, even when the view is filtered or sorted.
Empty filter state (criterion 5). A "No rows matching …" message with a Show-all-rows action — never a blank grid.
Architecture (locked by the spec):
app/src/lib/tableView.ts—filterRowIndices,sortRowIndices,splitHighlight,serializeCsvRow— fully unit-tested (23 tests). No new dependencies; TanStack Table was considered and rejected in the spec.view.lengthand each rendered row maps back throughsourceIndex = view[virtualRow.index]. Fixed 24px row height and absolute positioning untouched; transforms memoized on(table, query, sort)— no per-keystroke re-parse of source text.role="grid"+aria-rowcount(view length + header), labeled search input,aria-sortheaders,aria-selectedrows, visible focus rings, highlight/selection never color-alone.Both table instances get identical behavior — the toolbar, sorting, and selection live entirely in
CsvTable.How to Review
app/src/lib/tableView.ts— the pure transforms; read with its test file first, the component just wires them.app/src/components/CsvTable.tsx— toolbar + header buttons + view mapping. The virtualizer contract (count = view length, source-index mapping) is the load-bearing invariant.app/src/components/CsvTable.test.tsx— 16 new DOM tests; the large-grid virtualization test is unchanged and still green (396 tests total, 357 baseline + 39 new).app/src/changelog/entries.ts— entry id 8 announcing the feature.Test Evidence
WebM of the full flow: filter 'west' (5 of 20 with highlights), sort Units ascending, click + meta-click selection (2 selected), Copy as CSV emits header plus the two selected rows in sorted view order
WebM of the full flow: filter 'west' (5 of 20 with highlights), sort Units ascending, click + meta-click selection (2 selected), Copy as CSV emits header plus the two selected rows in sorted view order — recording
Toolbar remains usable at 375px — search, count chip, and selection controls visible on mobile viewport
Desktop table shows search toolbar, tri-state sort glyph (Units ascending), and '2 selected' state with source-row gutter fidelity
Local gates:
tsc, ESLint, full Vitest suite (396 passing), production build withapp/distrebuilt and committed,verify-seo.shpassing. The shim verification (verify-shim.sh) requires PHP 8.4 which is not installed in the local sandbox — it runs unchanged in the PHP CI job; no PHP files are touched by this PR. Browser evidence (desktop, 375px, and a filter→sort→select→copy recording) is attached below against the tested head SHA.🔗 Obvious Project · 🧵 Obvious Thread