From 384e04acbcb2e4f8154e519b324ae627d4b0df0f Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:37:56 -0700 Subject: [PATCH] The inline editor commits by record identity, not viewport position (improvise-2wu3) Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/clients.md | 30 +++++++++++++-- context/test-evidence/clients.md | 36 ++++++++++++++++++ .../tests/browser_round_trip.rs | 38 +++++++++++++++---- web/app.js | 10 +++-- 4 files changed, 100 insertions(+), 14 deletions(-) diff --git a/context/code/clients.md b/context/code/clients.md index 995b99f4..be8aae39 100644 --- a/context/code/clients.md +++ b/context/code/clients.md @@ -909,7 +909,8 @@ gotcha 47 asks for — closing e24v has to come to that line and delete it. aggregate, which is true in both front ends because the browser's refusal names drill. Delivering drill is `improvise-obtu`'s, not this bullet's. - **The commit's write is not spelled again here.** It is - `Client::typed_cell`'s, the same one click-to-edit uses, so a browser keyboard + `typed_cell_command`'s, the same one click-to-edit reaches through + `typed_record_frame`, so a browser keyboard commit and a browser mouse commit cannot disagree about what `""` or `"42"` means. - **`cell_at_cursor` returns `None` when the selection is scrolled out of the @@ -1374,8 +1375,30 @@ withholds rather than invents). from the host, which is the side that formats it; guessing it locally is how the two ends drift apart. +**The page commits by identity, never by position (improvise-2wu3).** While an +inline editor is open, `repaint` returns early, so the screen is frozen. But +`on_frame` goes on folding, so the cache moves while the screen does not. The +page used to keep the editor's viewport `(row, col)` and resolve it against the +**current** cache at commit. If another client had hidden a row above, that +position now held a different record. `basis.claim` was satisfied too, because +the fresh `Projection::Basis` had been folded along with the rows. So the write +landed on a row the user never looked at, and it was reported accepted. The +basis was current *for the client*; the stale thing was the screen, and +nothing measures that. + +So the JS surface is now two calls. `edit_target(row, col)` returns the +`RecordCell` as an opaque string, and the page takes it when the editor opens. +`typed_cell(target, text)` composes the write against that surrogate +(`typed_record_frame`). The position-taking composer is gone from the page's +surface, so the confusion cannot be written in the page. `typed_cell_frame(row, +col, …)` survives as Rust-only sugar over `record_cell_at` and +`typed_record_frame`, for tests that compose at the moment they read. The +surrogate cannot go stale in the way a position can: if the record is written +by someone else meanwhile, the basis tier refuses it (`RecordMoved`); if it is +gone, `RecordGone`. Either way the user is told. + `typed_cell_command` is the one composition shared by the mouse path -(`typed_cell_frame`) and the keyboard path (`keys::commit`). Writing it twice is +(`typed_record_frame`) and the keyboard path (`keys::commit`). Writing it twice is how a browser commit by Enter and a browser commit by click would come to disagree about what an empty string means. @@ -1524,7 +1547,8 @@ whether a typed string is a number. Five calls are the whole harness: | `client.set_viewport(rows, cols)` | the handshake frame, and every resize; also records the window so keys resolve here | | `client.on_frame(json)` | fold one frame, in arrival order | | `client.render_model()` | everything to paint, already decided | -| `client.typed_cell(r, c, s)` | the frame for "the user typed s here" | +| `client.edit_target(r, c)` | which stored cell an editor opened here edits: an opaque string the page holds, or `null` | +| `client.typed_cell(target, s)` | the frame for "the user typed s into that cell", by identity (improvise-2wu3) | | `client.on_key_event(k,c,a,s,m)` | the frame for a keypress, folded locally first where this client may answer it | **The two rules this file must not break are ordering and framing**: frames are diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index e8c8bc86..20bb1aae 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -4353,3 +4353,39 @@ Records mode is deliberately not covered. There the TUI yanks `None` and its `p` writes a junk `_Dim`/`_Index` cell, because it resolves the synthetic key (improvise-6p8q). The browser is right and the TUI is wrong, so a lockstep case there would assert the bug. + +### an_edit_writes_the_cell_the_user_clicked_after_the_rows_renumber + +improvise-2wu3. `web/app.js` freezes the screen while an inline editor is open +(`repaint` returns early), but `on_frame` goes on folding. The editor kept its +viewport `(row, col)`, and `typed_cell` resolved it against the *current* +cache at commit. + +The test follows the ticket's acceptance criteria against a real `Host`: +1. Client A reads the North/Revenue record at `(0, 0)`. +2. Client B's `HideItem("North")` is applied. +3. Only the frames addressed to A are folded into A, with no repaint, just as + the page does it. +4. A precondition asserts the rows really renumbered. +5. The write A composes must name the record A clicked. + +**Red first**, at HEAD after improvise-ktlk (PR #169), which moved the editor's +seed but not its commit. Written against the position API as it stood +(`typed_cell(0, 0, "42")`), it failed. The write named `RecordHash(161215…)`, +which is South's record now sitting at row 0, where North's `RecordHash(246296…)` +was expected. So the ticket still reproduced. The frame would have passed the +basis check, because A had folded the fresh `Projection::Basis` along with the +rows. + +**The fix makes the fault unwritable in the page.** The page's surface no +longer takes a position at commit. `edit_target(row, col)` is read when the +editor opens and returns the `RecordCell` as an opaque string, and +`typed_cell(target, text)` composes against it. The same test, rewritten for +that surface, passes. There is no position left at commit for a fault to +re-introduce. Rust-side `typed_cell_frame(row, col, …)` remains for tests +that compose at read time. + +The page path is exercised end to end by `harness/tests/shell.spec.js`'s +"committing an edit sends one frame and shows the host's value", which goes +through `edit_target` and the new `typed_cell`. The whole harness passes +locally (69 passed, 4 skipped). diff --git a/crates/improvise-wasm-client/tests/browser_round_trip.rs b/crates/improvise-wasm-client/tests/browser_round_trip.rs index b0d674ea..9c1a4f08 100644 --- a/crates/improvise-wasm-client/tests/browser_round_trip.rs +++ b/crates/improvise-wasm-client/tests/browser_round_trip.rs @@ -135,8 +135,12 @@ fn a_browser_client_seeds_types_a_number_and_sees_it_come_back() { this fixture has no group headers, so the two row coordinates \ coincide here — see a_grouped_row_axis_does_not_shift_the_write" ); + let target = client + .edit_target(at.data_row, at.col) + .expect("serialization must not fail") + .expect("an editable cell has an edit target"); let frame = client - .typed_cell(at.data_row, at.col, "7500") + .typed_cell(&target, "7500") .expect("serialization must not fail") .expect("an editable cell must compose a write"); let frames = send(&mut host, id, &frame); @@ -218,8 +222,12 @@ fn a_grouped_row_axis_does_not_shift_the_write() { coordinates coincide and nothing below is being tested" ); + let target = client + .edit_target(at.data_row, at.col) + .expect("serialization must not fail") + .expect("an editable cell has an edit target"); let frame = client - .typed_cell(at.data_row, at.col, "4200") + .typed_cell(&target, "4200") .expect("serialization must not fail") .expect("an editable cell must compose a write"); send(&mut host, id, &frame); @@ -287,9 +295,9 @@ fn an_aggregated_cell_reaches_the_page_as_not_editable() { "every cell here aggregates over Product, so none may be written" ); assert_eq!( - client.typed_cell(0, 0, "42").unwrap(), + client.edit_target(0, 0).unwrap(), None, - "and typing into one composes nothing, because the id is of the wrong kind" + "and there is no cell to type into, because the id is of the wrong kind" ); } @@ -649,7 +657,13 @@ fn an_edit_writes_the_cell_the_user_clicked_after_the_rows_renumber() { let (b, _) = host.connect(10, 10); // A opens an editor on the North/Revenue cell it is looking at. - let clicked = editor.record_cell_at(0, 0).expect("North/Revenue is stored"); + let clicked = editor + .record_cell_at(0, 0) + .expect("North/Revenue is stored"); + let target = editor + .edit_target(0, 0) + .expect("serialization must not fail") + .expect("a stored cell has an edit target"); // B hides North; A folds the result without repainting, as the page does. let view = host.app().view_name().to_string(); @@ -671,15 +685,23 @@ fn an_edit_writes_the_cell_the_user_clicked_after_the_rows_renumber() { ); let json = editor - .typed_cell(0, 0, "42") + .typed_cell(&target, "42") .expect("serialization must not fail") .expect("a stored cell composes a write"); let ClientFrame::Commands(cmds) = serde_json::from_str(&json).expect("a client frame") else { panic!("a write is a Commands frame"); }; let written = match cmds.as_slice() { - [Command::Model { cmd: ModelCommand::SetCell { cell, .. }, .. }] => cell.clone(), + [ + Command::Model { + cmd: ModelCommand::SetCell { cell, .. }, + .. + }, + ] => cell.clone(), other => panic!("expected one SetCell, got {other:?}"), }; - assert_eq!(written, clicked, "the write landed on a row the user never looked at"); + assert_eq!( + written, clicked, + "the write landed on a row the user never looked at" + ); } diff --git a/web/app.js b/web/app.js index b3c0149c..22c487ce 100644 --- a/web/app.js +++ b/web/app.js @@ -541,6 +541,10 @@ function gridTable(model) { * back is the server's, never the string typed here. */ function edit(td, row, col, seed) { if (editing) return; + // WHICH cell, captured now: rows renumber while the editor is open, so the + // position is not an address by commit time (improvise-2wu3). + const target = client.edit_target(row, col); + if (!target) return; // `seed` is `RenderCell::edit_text` -- the STORED value, not the drawn one: // `td.textContent` has been through the number format. const original = seed; @@ -563,9 +567,9 @@ function edit(td, row, col, seed) { closed = true; editing = null; if (commit) { - const frame = client.typed_cell(row, col, input.value); - // null only if the grid moved under the editor; the repaint that - // follows restores whatever the server now says is there. + const frame = client.typed_cell(target, input.value); + // null only before any basis arrived; the repaint that follows + // restores whatever the server now says is there. if (frame) transport.send(frame); } td.replaceChildren(drawn);