feat(table): row/column deletion actions + readable selected headers - #207
Conversation
Input table only (output stays read-only): toolbar actions and keyboard Delete/Backspace remove selected rows or columns from the parsed table, serialize through the CSV serializer, and commit through the same guarded reconversion path as a cell edit — raw text stays the single source of truth. Deleting the last remaining column is blocked with an explanatory tooltip; deleting all rows falls through to the existing empty state; hand-edited raw routes through the guarded prompt like any cell edit. Selected column headers switch from the inverted-ink fill (white-on- white / black-on-black) to a wash treatment with contrasting text in both themes; idle header rendering is untouched. Changelog entry 10; no new undo stack — recovery is the existing guard/revert affordances.
![obvious-autobuild-staging[bot]](https://avatars.githubusercontent.com/u/31830338?s=60&v=4){kind=small}
| </button> | ||
| </div> | ||
| )} | ||
| {!isFiltered && view.length === 0 && ( |
There was a problem hiding this comment.
Medium · reliability — The new empty-rows overlay has no editable gate, and the output pane renders this same component with no onCellCommit (OutputPane.tsx:184), so a headers-only CSV output — exactly what deleting all input rows produces — shows "No rows. Paste or upload data to start over." in a strictly read-only pane. On that surface the guidance is wrong: the output accepts neither paste nor upload. Gate it on editable ({editable && !isFiltered && view.length === 0 && ...}) to match the delete buttons, and assert output-table-empty-rows stays absent for headers-only output in the output suite.
There was a problem hiding this comment.
Fixed in 9431833 — the empty-rows overlay is now gated on editable ({editable && !isFiltered && view.length === 0}), so the read-only output pane no longer shows input-pane guidance for headers-only output. Added the requested assertion: the output-style table (no onCellCommit) keeps csv-table-empty-rows absent while the editable input table still shows it when all rows are deleted.
Headers-only output — what deleting all input rows produces — rendered "No rows. Paste or upload data to start over." in the read-only output pane, which accepts neither paste nor upload. The overlay now renders only for the editable input table, matching the delete buttons' gate. Review finding: obvious-autobuild-staging[bot] Medium · reliability (PR #207). Adds the output-pane absence assertion it asked for.
|
Fixed the Medium finding in 9431833: the empty-rows overlay is gated on |
There was a problem hiding this comment.
Obvious Code Review — Pass 2 (9431833)
Verdict: COMMENT
Pass 1's Medium (empty-rows overlay rendered in the read-only output pane) is resolved — CsvTable.tsx:918 now gates on editable, and the rebuilt dist matches.
Summary
- Blocker: 0
- High: 0
- Medium: 1 (new)
Medium
app/src/components/CsvTable.test.tsx:539— The new regression test is vacuous: the harness fixture has 2 data rows, socsv-table-empty-rowsis absent under both the old and new code; the test does not pin theeditablegate it was added for.
| }); | ||
|
|
||
| it("keeps the empty-rows overlay out of the read-only output table", () => { | ||
| render(<DeletionHarness initial={"album\nDe Stijl\nElephant"} editable={false} />); |
There was a problem hiding this comment.
Medium · reliability — This test does not pin the gate it was added for: initial={"album\nDe Stijl\nElephant"} yields headers plus 2 data rows, so the overlay (view.length === 0) is absent under the old ungated code too — reverting the editable gate would still pass. Exercise the guarded path with a headers-only fixture: render(<DeletionHarness initial={"album"} editable={false} />) then assert queryByTestId("csv-table-empty-rows") is null. The editable=true + 0-rows presence case is already covered by "falls through to the empty state when every row is deleted".
There was a problem hiding this comment.
Fair catch — the 2-row fixture made the test vacuous. Fixed in a6845fe: the fixture is now headers-only ("album"), which makes the ungated code render the overlay (I verified by reverting the gate locally: the test fails without it, passes with it), and the output table keeps it absent only under the gate. Also asserts the headers still render for the headers-only shape.
The regression test used a 2-row fixture, so csv-table-empty-rows was absent under the ungated code too — reverting the editable gate would still pass. A headers-only fixture makes the ungated code render the overlay (verified: reverting the gate fails this test), and the output table keeps it absent only under the gate.
|
Fixed the vacuous-test finding in a6845fe: the fixture is now headers-only ("album"), so the ungated code would render the overlay there (verified locally — reverting the gate fails the test), and the read-only output table keeps it absent only under the editable gate. 439 tests green. |
There was a problem hiding this comment.
Obvious Code Review — Pass 3 (a6845fe)
Verdict: COMMENT
No findings — clean pass. Pass 2's Medium (the regression test did not pin the editable gate) is resolved: the fixture is now headers-only ("album", so view.length === 0), with a comment explaining the vacuity trap and a headers sanity assert so the fixture cannot silently degenerate again. Against ungated code this test would now fail.
Verification
Full test suite run at this head in a clean worktree with fresh install: 28 files / 439 tests, all passing.
Summary
- Blocker: 0
- High: 0
- Medium: 0
|
Thanks — Pass 3 acknowledged. Good catch on the fixture degeneration guard; the headers-only fixture plus sanity assert is the right pin. No further changes needed from this pass. |
1 participant
Why
Two defects from dogfooding the Glide table (PR #206): a selected column header renders as a solid inverted block — near-white fill with near-white text in dark mode, near-black on near-black in light — because
buildTheme()mappedaccentColorto the theme's inverted ink whiletextHeaderSelectedsits in the same value range. And the table grew selection, editing, and fill, but no way to remove rows or columns: fixing a bad row means hand-editing the raw CSV, which is exactly the low-trust path the table was built to replace.What
Selected-header contrast fix. The selected header now uses a wash treatment: a soft fill (
bgHeaderHovered) plus a 2pxaccentColorbottom border, with a genuinely contrastingtextHeaderSelectedin both themes. Idle headers are pixel-identical to merged master (muted uppercase 11px mono, hairline bottom border) — only the selected-state mapping changed. The customdrawHeadercallback (sort glyph, gutter#) keeps working and paints the wash for both data and gutter headers.Row/column deletion — input table only. The output table renders through the same
CsvTable, so every deletion affordance is gated on the editable instance.Delete N rows/Delete N columns, Trash2 icon, same dense chrome as Copy).onDelete(6.0.3 signature verified: returnsboolean | GridSelection; we returnfalseso Glide's default cell-clearing mutation never runs — our handler owns the mutation).serializeCsvTable→setInput→ the existing PR feat(output): editable JSON output with guarded regeneration #198 guarded reconversion — the identical path to a cell edit. Raw CSV text stays the single source of truth.No new undo stack this increment. Recovery is the existing guard/revert affordances (the guarded prompt keeps the previous raw text; reload restores it).
How to review:
app/src/lib/tableDeletion.ts(pure transforms),app/src/components/CsvTable.tsx(theme mapping, toolbar actions,onDelete),app/src/test/glideDataEditorMock.tsx(mock affordances). Intentionally excluded: output-table deletion (read-only by design), undo (deferred), GET endpoints (none added).Test Evidence
Selected-header contrast, both themes: BEFORE (master 85651b0, pixel-verified) — dark fills the selected header near-white rgb(250,250,250) under muted-gray text (~1.7:1); light fills it near-black rgb(9,9,11) with same-value text. AFTER (a6845fe) — bgHeaderHovered wash with contrasting text: dark rgb(39,39,42)+rgb(161,161,170) ~4.6:1, light rgb(244,244,245)+rgb(113,113,122) ~4.5:1. Before shots use a programmatically dispatched column selection on the master build; selection verified via the '1 column selected' chip
Hand-edit guard: after editing the raw text ('White Blood Cells (hand edit)'), deleting a selected row splices into the live hand-edited text — the hand edit survives (verified in-pane) while the selected row is removed; no silent clobber
Last-column guard: on a one-column CSV the selected header shows a disabled 'Delete 1 column' action with the tooltip 'At least one column must remain — the converter needs a column to detect'
Column deletion: selecting the year header shows 'Delete 1 column'; the action removes the header and every row's cell, and the reconverted output no longer contains the year field
Column deletion: selecting the year header shows 'Delete 1 column'; the action removes the header and every row's cell, and the reconverted output no longer contains the year field — recording
Row deletion: selecting a row via the gutter shows 'Delete 1 row'; the action removes the source row, raw CSV updates, and the output reconverts (6 rows → 5)
Row deletion: selecting a row via the gutter shows 'Delete 1 row'; the action removes the source row, raw CSV updates, and the output reconverts (6 rows → 5) — recording
title.🔗 Obvious Project · 🧵 Obvious Thread