feat(table): inline cell editing on the input table - #205
feat(table): inline cell editing on the input table#205obvious-autobuild-staging[bot] wants to merge 5 commits into
Conversation
serializeCsvTable(headers, rows, delimiter?) writes a parsed grid back to CSV text for the cell-edit commit path: any cell containing a quote, the delimiter, CR or LF is quoted with inner quotes doubled; the optional delimiter (default comma) keeps a TSV edit from rewriting the file as comma-CSV. Total function; parse(serialize(parse(text))) is stable.
…ntics Click (or Enter on a focused cell) opens a single-line input pre-filled with the raw cell text: Enter commits, Tab commits and advances in view order (wrapping rows, Shift+Tab reverses), Esc cancels silently, blur commits. Commits splice the value into the parsed grid and hand the re-serialized text to onCellCommit; an unchanged value skips the write. The editor closes when a commit filters or sorts its row out of view. The output table gets no onCellCommit and keeps zero editing affordances.
The input table's cell commits feed onInputChange — the same pipeline a raw-view keystroke uses — so edits pick up the debounce, analytics, and the PR #198 output guard: a hand-edited output stays frozen across a cell edit until Revert or Discard & reconvert. App-level tests pin the commit → raw-text → reconverted-output flow and the guarded interplay; changelog entry 9 announces editable cells. app/dist rebuilt + SEO verified.
![obvious-autobuild-staging[bot]](https://avatars.githubusercontent.com/u/31830338?s=60&v=4){kind=small}
| const grid = table.rows.map((cells, r) => | ||
| r === row ? [...cells.slice(0, col), value, ...cells.slice(col + 1)] : cells | ||
| ); | ||
| onCellCommit(serializeCsvTable(table.headers, grid, table.delimiter)); |
There was a problem hiding this comment.
🔴 Blocker — reliability: Cell commit silently deletes fields from untouched rows in ragged CSVs.
parseCsvTable pads short rows with "" and truncates over-long rows via row.slice(0, width) (app/src/lib/csvTable.ts:128-131) — correct for display, but this commit path splices the edit into that padded/truncated grid and serializeCsvTable writes the whole grid back over the raw text. Any row wider than the header row loses its extra fields from the user's data when they edit an unrelated cell.
Reproduced with this PR's own parser:
input: album,year
De Stijl,2000,extra,more
Elephant,2003
edit: cell (row 1, col 1) → "De Stijl II"
committed: album,year\nDe Stijl II,2000\nElephant,2003 ← ",extra,more" deleted
rowWidths ([4, 2] here) is already parsed and exists precisely to flag this shape; nothing gates the commit on it. The suite (420/420 green at head) passes while this loses data — no test covers a ragged grid. Short rows also gain trailing delimiters (a,b,c\n1 → a,b,c\n1,,), and the malformed-CSV warning keyed on rowWidths silently clears after any edit.
Suggestion: Gate editing on a rectangular grid — table.rowWidths.every(w => w === table.headers.length) — and disable the edit affordance (pointing at the existing malformed-CSV warning) when violated; or make the commit a surgical splice of only the edited row into the original text so untouched rows keep their exact bytes.
There was a problem hiding this comment.
Fixed in 671e18b — the commit path no longer serializes the whole grid. parseCsvTable now reports each row's exact byte span and parses records at their true width (parseCsvRecord); a commit splices only the edited row back into the source text via serializeCsvRow. Untouched rows keep their bytes — wide rows keep extra fields, short rows gain no padding. Regression tests: "commit rewrites only the edited row's bytes — ragged neighbors survive" and "editing a padded cell of a short row writes just that field".
| * the file as comma-CSV. When the delimiter is not comma, cells containing | ||
| * that delimiter are quoted too — the grid must re-parse identically. | ||
| */ | ||
| export function serializeCsvTable(headers: string[], rows: string[][], delimiter = ","): string { |
There was a problem hiding this comment.
🟡 Medium — reliability: One cell edit rewrites the whole file's byte form — CRLF → LF and the trailing newline is dropped.
serializeCsvTable joins with "\n" unconditionally. Reproduced: album,year\r\nDe Stijl,2000\r\nElephant,2003\r\n → commit → album,year\nDe Stijl II,2000\nElephant,2003. The parsed grid is identical, but the raw view visibly rewrites lines the user never touched, and a subsequent download serializes the normalized bytes. Excel-pasted CRLF data is the common case, and the component docstring's invariant ("the commit is exactly a raw-view keystroke of the serialized text") is byte-false here — a raw keystroke preserves the rest of the file.
Suggestion: Detect the dominant line terminator at parse time (plus whether the text ended with one) and join with it; or fold into the surgical row splice suggested on the commit call, which preserves untouched bytes by construction.
There was a problem hiding this comment.
Fixed in 671e18b — the row spans end before line terminators and the splice rewrites only the edited row inside the original text, so CRLF endings and the trailing newline on untouched rows survive verbatim. Regression test: "commit keeps the file's CRLF endings and trailing newline". Also fixed en route: a trailing delimiter now parses as the empty field RFC 4180 promises ("album," keeps its second, empty header) instead of being dropped.
| : adjacentCell(editing, dir) | ||
| ); | ||
| }} | ||
| onKeyDown={(event) => { |
There was a problem hiding this comment.
🟡 Medium — reliability: Focus drops to <body> on the two primary editor exits — Escape and Enter.
On Escape-cancel and Enter-commit the input unmounts and nothing refocuses the cell, so focus lands on the document body: the next Tab restarts from the top of the page and screen-reader context is lost. This grid is keyboard-first (cells are tabIndex=0; the Tab/Enter/Escape flows are designed and tested), and the ARIA grid editing pattern returns focus to the committed/cancelled cell. Tab-advance is currently the only exit that manages focus.
Suggestion: In the Enter and Escape branches, focus the parent cell (event.currentTarget.closest('[role="gridcell"]')?.focus()) before the state clears — the cell is already tabbable.
There was a problem hiding this comment.
Fixed in 671e18b — Enter and Escape now return focus to the edited cell per the ARIA grid editing pattern, with the refocus blur suppressed through an exit ref so it cannot double-commit the value. Regression tests: "returns focus to the cell on Enter commit" and "returns focus to the cell on Escape cancel without committing".
There was a problem hiding this comment.
Review summary — 1 Blocker, 2 Medium, 1 Suggestion
Solid feature work: the commit-rides-the-raw-input-pipeline design is right, the guarded-regeneration interplay is genuinely well tested, the uncontrolled editor keeps keystrokes out of the virtualized render path, and the suite is green (420/420 verified at head in a scratch worktree). The blocker is a data-fidelity gap in the new write path: the commit round-trips through a padded/truncated display grid, so editing one cell in a ragged CSV silently deletes fields from untouched rows — reproduced with this PR's own parser.
Findings
- 🔴 Blocker — reliability (
app/src/components/CsvTable.tsx:144): cell commit serializes the padded/truncated grid back over the raw text — a row wider than the header loses its extra fields on any unrelated cell edit (De Stijl,2000,extra,more→,extra,moredeleted).rowWidthsis already parsed; gate the affordance on a rectangular grid or splice only the edited row surgically. Untested gap — 420/420 pass while this loses data. - 🟡 Medium — reliability (
app/src/lib/csvTable.ts:151): one edit normalizes the whole file's byte form — CRLF → LF, trailing newline dropped, short rows gain trailing delimiters. Semantically neutral, byte-destructive on untouched lines; shares a root cause with the Blocker. - 🟡 Medium — reliability (
app/src/components/CsvTable.tsx:503): focus drops to<body>on Escape-cancel and Enter-commit — a keyboard-first grid should return focus to the cell (ARIA grid editing pattern; Tab-advance is the only exit that does). - 🔵 Suggestion — hygiene (
app/src/components/CsvTable.tsx:486): editor aria-label reads "Edit , row 1" when a header cell is empty (common with trailing delimiters) — fall back tocolumn ${c + 1}.
Verified: canonical merge-base diff reviewed; ragged/CRLF/short-row commit paths reproduced empirically with the PR's parser; vitest 420/420 green at cf191ff; InputPane wiring type-checks (onInputChange: (value: string) => void); changelog entry id 9 follows convention; app/dist rebuilt with hash parity (CI-gated).
Readiness: BLOCKED on the ragged-grid data loss. Readiness cannot be CLEAN until it resolves; the CRLF rewrite shares its root cause and is worth fixing together.
… line endings Review findings on PR #205: whole-grid serialization dropped wide rows past the header width, padded short rows with trailing delimiters, normalized CRLF endings and trailing newlines, and lost grid focus after Enter/Escape. - parseCsvTable now reports exact row spans and parses records at their true width (parseCsvRecord), including the empty field a trailing delimiter promises (RFC 4180) - commits splice serializeCsvRow(header-width) of the edited row into the original text — untouched rows keep their bytes, short/wide rows keep their shape, the file's line endings and trailing newline stay - Enter/Escape return focus to the cell (ARIA grid pattern) with the refocus blur suppressed so it cannot double-commit - editor labels fall back to "column N" when a header is the empty field tableView re-exports serializeCsvRow; copy-as-CSV behavior is unchanged.
|
All four findings are fixed in 671e18b — the splice approach you recommended, not a gated affordance:
New regression tests cover each: ragged-neighbor survival, padded-cell writes, CRLF/trailing-newline preservation, focus return on Enter and Escape, and the label fallback. Full suite 435/435, lint and tsc clean, production build + verify-seo.sh green locally at this head. |
There was a problem hiding this comment.
Incremental review (pass 2) — all pass-1 findings resolved. No new blocking findings.
The fix push replaces the whole-grid re-serialization with a surgical splice: parseCsvTable now records each data row's exact byte span (rowSpans), and the commit re-serializes only the edited row via parseCsvRecord + serializeCsvRow, leaving every other byte of the file untouched. Verified in a scratch worktree at 671e18b: vitest 435/435 pass, tsc -b --noEmit clean.
Resolution of pass-1 findings (each now regression-tested):
- 🔴 Blocker — reliability (ragged-grid data loss): resolved. A test pins the exact pass-1 scenario — editing row 1 of
album,year\nDe Stijl,2000,extra,more\nElephant,2003commits with,extra,moreintact. - 🟡 Medium — reliability (CRLF rewrite): resolved by the same splice; a test asserts the committed text byte-exactly preserves
\r\nendings and the trailing newline. - 🟡 Medium — reliability (focus loss): resolved — Enter/Escape refocus the cell via
closest('[role="gridcell"]')?.focus()with anexitRefstand-down so the refocus blur can't double-commit; focus-return is asserted for both exits. - 🔵 Suggestion (empty-header aria-label): resolved —
column ${c + 1}fallback, test-pinned.
Reviewed and accepted: the new expectField parser branch (a trailing delimiter now yields the RFC 4180 empty last field) — load-bearing for splice fidelity on delimiter-terminated rows, exercised by the serializeCsvRow round-trip assertion, and it makes the rowWidths-keyed malformed-CSV warning more correct. The serializeCsvRow consolidation from tableView.ts into csvTable.ts is a behavior-preserving re-export that unifies copy-as-CSV and the edit commit on one quoting primitive.
Remaining (non-blocking, 🔵 Suggestion — hygiene, author's discretion):
onCellCommitprop JSDoc still says the commit is "called with the full grid re-serialized" — the implementation now splices one row; the component-header docstring was updated but this one still contradicts it.serializeCsvTableincsvTable.tsis now unused outside its own tests after the splice rework (tree-shaken from dist, so no bundle cost) — drop it until a consumer exists or mark it a deliberate codec primitive.
Readiness: CLEAN — no Blocker or High findings.
…rk serializeCsvTable a codec primitive Review pass-2 suggestions: the prop doc still described whole-grid re-serialization, and the grid-level serializer now has no production consumer (the commit path splices per-row). The export stays — the spec names it a deliverable and round-trip tests exercise it.
|
Both suggestions taken in 3b2e2a8:
|
There was a problem hiding this comment.
Incremental review (pass 3) — no findings — clean pass.
The delta since 671e18b is a single doc-only commit (2 files, 8+/2−): the onCellCommit prop JSDoc now describes the splice path ("only the edited row's bytes are spliced … every other byte is preserved") instead of the obsolete full-grid re-serialization, and serializeCsvTable's JSDoc marks it a deliberate codec primitive kept for round-trip testing and future whole-grid operations. Both pass-2 suggestions are resolved.
Re-certified at head 3b2e2a8 in a scratch worktree: vitest 435/435 pass (27 files), tsc -b --noEmit clean. No runtime, wiring, security, or performance surface touched.
All findings across three passes are resolved — no open Blocker, High, Medium, or Suggestion.
|
Thanks — both pass-2 suggestions are addressed in 3b2e2a8 exactly as described. Final QA evidence (editing-state screenshot and the edit → re-convert → Raw view WebM) is being re-recorded against this head now, then this merges. |
|
Superseded by a direction change: the data table is moving to react-data-grid with spreadsheet-style editing (library-based grid per amended spec art_Z56fIBHg). The serializeCsvTable RFC 4180 writer, guarded-reconversion wiring, and tests from this PR carry forward to the replacement implementation — thanks for the clean data-path work here. |
…y-as-CSV, editing (#206) * feat(table): adopt glide-data-grid with RFC 4180 serializer (ported from #205) Swap the hand-rolled table internals for @glideapps/glide-data-grid 6.0.3 (canvas-rendered, first-class rectangular + multi-range selection). Port the RFC 4180 serializer and its round-trip suite from feat/table-editing @ cf191ff, and keep the BigInt-safe comparator / filtering helpers in tableView.ts — Glide does not sort or filter for us. New dependency-free tableSelection.ts serializes row / column / rectangular selections into CSV through the same writer. * feat(table): glide grid with selection, copy-as-CSV, search toolbar CsvTable runs on DataEditorCore: source-row identity for row selection, single-column numeric-aware sort state, 150ms debounced search with a count chip, header-click column selection, and a window copy interceptor that replaces Glide's native TSV with RFC 4180 CSV of the selection. jsdom cannot exercise canvas, so tests swap DataEditorCore for a mock that drives the component's real handlers; canvas gestures get browser evidence instead of faked unit tests. * feat(table): spreadsheet editing through guarded reconversion Input-pane edits commit through onCellCommit: each batched Glide edit splices its exact row span out of the raw CSV and feeds the shared input handler, so raw text and table view never disagree and PR #198's dirty/revert/discard semantics stay intact. Output table remains strictly read-only. Changelog entry 9 announces the rebuilt table; dist rebuilt and committed. * test: adapt app-level suites to the glide grid mock Permalink, pane-state, paste-routing, and output-editing suites render the real App, so they import the shared glide mock and assert on row data (canvas headers render uppercase) instead of raw header text. * fix(table): floor the grid wrapper so squeezed mobile panes still show rows The stacked mobile layout can hand the table pane less height than the grid's natural size (the split container shrinks; master's DOM table overflowed visibly in the same case). Floor the wrapper at the header plus up to 12 rows via minHeight — flex-1 still fills taller panes and Glide scrolls internally — so a fresh 375px load paints the table instead of a header sliver. * fix(table): mount the Glide editor — #portal host and explicit renderers Two real-grid-only editor defects the jsdom mock could not catch: 1. Glide mounts its cell-editor overlay through document.getElementById('portal'), which did not exist — every activation logged "Cannot open Data Grid overlay editor, because portal not found". Added <div id="portal"> as the last child of app/index.html. 2. DataEditorCore builds no cell-renderer map on its own, so the overlay shell mounted with no text editor inside. Pass renderers={AllCellRenderers}. Verified live on the dev server: two-click activation opens the portal textarea, typing + Enter commits through the guarded reconversion, and Raw view reflects the edit. Full suite 415 green, lint/tsc clean, dist rebuilt, verify-seo.sh passed. --------- Co-authored-by: Obvious <obvious@obvious.ai>
1 participant
Why
The data table (PR A) gave the input surface search, sort, and selection — but cells were read-only. Any value fix meant flipping to the Raw view and hand-editing CSV text, quoting included. This PR closes that loop: click a cell, type the fix, done.
What
Inline cell editor on the input table only. Click (or Enter on a focused cell) opens a single-line input pre-filled with the raw cell text:
onCellCommit. An unchanged value skips the write entirely.RFC 4180 serializer — new total export
serializeCsvTable(headers, rows, delimiter?)inapp/src/lib/csvTable.ts:Any cell containing the delimiter, quote, CR, or LF is quoted and escaped; everything else passes through. Round-trip property:
parse(serialize(parse(text)))is stable for valid input, and clean inputs re-serialize byte-identically (TSV/semicolon preserved via the delimiter arg).One input pipeline. The commit path feeds the same
onInputChangea raw-view keystroke would: debounce, analytics, and the PR #198 output guard all apply unchanged — a hand-edited output stays frozen across a cell edit until Revert or Discard & reconvert. Raw view and table view always render the same data; the text is the single source of truth.Deliberately excluded (locked): output-table editing, multi-line editors, per-cell undo, paste-into-grid, structural add/remove of rows or columns.
How to Review
app/src/lib/csvTable.ts—serializeCsvTable(the pure core; all quoting lives here).app/src/components/CsvTable.tsx— editor state machine + keyboard semantics; output-table gating by omitted callback.app/src/components/InputPane.tsx— the one-line wiring toonInputChange.app/src/App.test.tsx,app/src/outputEditing.test.tsx— commit-flow and guarded-regeneration interplay tests.Verification: 420/420 Vitest (19 new), lint, tsc, production build with committed
app/distparity,verify-seo.sh.🔗 Obvious Project · 🧵 Obvious Thread
Test Evidence
Cell edit commits and re-converts: editing Elephant to "Elephant II" updates the JSON output after the debounce, and the Raw view shows the same edit serialized back into the CSV text (consistency invariant)
Cell edit commits and re-converts: editing Elephant to "Elephant II" updates the JSON output after the debounce, and the Raw view shows the same edit serialized back into the CSV text (consistency invariant) — recording
Inline cell editor opens on the input table: click a cell and a single-line input appears pre-filled with the raw cell text (Elephant, row 4)
