From 85c483bd24b54f0d5356e23cbecf936c2127119e Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 08:38:25 -0700 Subject: [PATCH] Tab at the rightmost column reopens on the value just written; pin the +cat clear (improvise-3tt4, improvise-5wai) Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/test-evidence/clients.md | 37 +++++++++ crates/improvise-protocol/src/resolve.rs | 26 ++++-- .../improvise-wasm-client/tests/lockstep.rs | 83 +++++++++++++++++++ 3 files changed, 140 insertions(+), 6 deletions(-) diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 826f8cd4..cb64a664 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -5035,3 +5035,40 @@ where nothing reopens. At the bottom-right corner, `i`, `9`, Enter must leave `edit` as `""` on both ends, with the mode no longer editing. **Rival:** the browser without `cleared` leaves `"49"` and fails here. The equivalent TUI rival fails `text_entry_modes_clear_the_buffer_they_type_into`. + +### `tab_at_the_rightmost_column_reopens_on_the_value_just_written` + +**improvise-3tt4**, found by the #227 review at `a1b23c05`. `resolve::advanced` +re-seeded the reopened editor from the client's cache, on the argument that the +advance always lands on a *different* cell. That is false for Tab at the +rightmost column: `Advance::Right` clamps back onto the cell the frame has just +written, and that cell's cache entry is the pre-write value. + +The test uses the lockstep 2×2 fixture and starts at (0, 1), which is North / +Revenue and holds 2. It presses `i 9 Tab 5 Enter` on a browser client and on a +key-driven host, and compares the buffers after every key. It failed at +`a1b23c05` on `Tab`: the browser buffer was `"2"` and the host's was `"29"`. +Pressing `5 Enter` would then have written 25 over the 29 the user had just +committed. Both ends must end at 295. + +**Fix:** `landed_seed` seeds the landing cell from the committed value's edit +text when the advance lands on the cursor's own cell and the commit wrote. In +every other case it uses the cache. No other advance lands on the written cell: +`Down` moves to the next row or the next column, or reopens nothing at the +corner. + +### `tab_at_the_rightmost_column_reopens_on_the_stored_spelling` + +This pins what "the committed value" means in the fix above: the stored value's +edit text, as `typed()` would store it, not the raw typed string. `i Backspace 0 +0 7 Tab` at (0, 1) must leave `"7"` in both buffers. **Rival:** seeding from the +raw text fails with `"007"` against the host's `"7"`. + +### `a_category_committed_at_plus_cat_leaves_no_buffer_behind_on_both_ends` + +**improvise-5wai**, found by the #227 review. `+cat` reopens nothing, so the +commit's own head clear (`resolve::cleared` in `create_column`) is the only +thing that empties the buffer. The test types `Season` at the `+cat` column of +the records pair, presses Enter on both ends, and requires the category to have +landed and both buffers to be `""`. **Rival:** `let _ = cleared(&edit_mode)` in +`create_column` fails only this test, with the browser keeping `"Season"`. diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index 330a37e9..2659b1f9 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -898,7 +898,8 @@ fn commit(state: &ClientState, edit_mode: AppMode, advance: Advance) -> Option Option> { +fn advanced( + state: &ClientState, + edit_mode: AppMode, + advance: Advance, + wrote: Option<&str>, +) -> Option> { let GridExtent { rows, cols } = state.grid.extent; let row_max = rows.saturating_sub(1); let col_max = cols.saturating_sub(1); @@ -983,12 +989,20 @@ fn advanced(state: &ClientState, edit_mode: AppMode, advance: Advance) -> Option return Some(Vec::new()); }; let mut commands = scrolled_to(state, nr, nc); - // `EnterEditAtCursor`: seed from where we landed, then change mode. The - // new cell is a different cell, so a stale cache is correct here. - commands.extend(opened_on(seed_at(state, nr, nc), edit_mode)); + // `EnterEditAtCursor`: seed from where we landed, then change mode. + commands.extend(opened_on(landed_seed(state, (nr, nc), wrote), edit_mode)); Some(commands) } +/// The seed at the cell an advance lands on. The cache is stale only where the +/// frame just wrote, which is the cursor's own cell (improvise-3tt4). +fn landed_seed(state: &ClientState, (row, col): (usize, usize), wrote: Option<&str>) -> String { + match wrote.filter(|_| state.view.cursor().selected == (row, col)) { + Some(text) => typed(text).map(|v| v.edit_text()).unwrap_or_default(), + None => seed_at(state, row, col), + } +} + /// The surrogate naming the record under the cursor, or `None` when the /// cursor is not on a writable cell. fn record_at_cursor(state: &ClientState) -> Option { diff --git a/crates/improvise-wasm-client/tests/lockstep.rs b/crates/improvise-wasm-client/tests/lockstep.rs index 89353591..ae224fe8 100644 --- a/crates/improvise-wasm-client/tests/lockstep.rs +++ b/crates/improvise-wasm-client/tests/lockstep.rs @@ -1680,6 +1680,30 @@ fn a_locally_committed_creating_column_agrees_with_the_host() { } } +/// improvise-5wai: `+cat` reopens nothing, so its commit's own clear is all +/// that empties the buffer, on both ends. Evidence file. +#[test] +fn a_category_committed_at_plus_cat_leaves_no_buffer_behind_on_both_ends() { + let (mut app, mut twin) = records_pair(); + let client = furnished_client(&app, (12, 12)); + let at = creating_cell(&client, improvise_protocol::CreateKind::Category); + let mut client = client; + typing_at(&mut app, &mut twin, &mut client, at, "Season"); + client = furnished_client(&app, (12, 12)); + client.view = app.view_state.clone(); + assert_eq!(read_edit(&client.view.buffers), "Season", "nothing was typed, so this proves nothing"); + + both_ends(&mut app, &mut twin, &mut client, "Enter"); + + assert!(document_text(&app).contains("Season"), "the commit did not land"); + assert_eq!(read_edit(&app.view_state.buffers), "", "the host left the name behind"); + assert_eq!(read_edit(&client.view.buffers), "", "the browser left the name behind"); +} + +fn read_edit(buffers: &std::collections::HashMap) -> &str { + buffers.get("edit").map(String::as_str).unwrap_or("") +} + /// The twin of the commit test above, for the door rather than the exit /// (improvise-bd9o). Evidence file. #[test] @@ -2228,6 +2252,65 @@ fn a_commit_at_the_corner_leaves_no_buffer_behind_on_both_ends() { } } +/// BUG (improvise-3tt4) — see `context/test-evidence/clients.md`. +#[test] +fn tab_at_the_rightmost_column_reopens_on_the_value_just_written() { + let mut client_host = precise_host(improvise_protocol::CellValue::Number(4.0)); + client_host.cursor_mut().selected = (0, 1); + let mut key_host = precise_host(improvise_protocol::CellValue::Number(4.0)); + key_host.cursor_mut().selected = (0, 1); + let mut client = client_ready_to_edit(&client_host, (2, 2)); + for key in ["i", "9", "Tab", "5", "Enter"] { + let event = BrowserKey::new(key).to_key_event().unwrap(); + let KeyOutcome::Local(cmds) = resolve(&client, event) else { + panic!("{key}: the client must answer this key locally"); + }; + for cmd in &cmds { + client.fold(cmd); + } + apply_on(&mut client_host, &cmds); + reduce_key(&mut key_host, event); + assert_eq!( + client.view.buffers, key_host.view_state.buffers, + "{key}: client and host disagree about the buffers" + ); + } + for (end, app) in [("browser", &client_host), ("host", &key_host)] { + assert_eq!( + app.model_state + .workbook + .model + .get_cell(®ion_measure("North", "Revenue")), + Some(&improvise_protocol::CellValue::Number(295.0)), + "{end}: the reopened editor did not extend the value Tab had just written" + ); + } +} + +/// The reopened seed is the STORED value's edit text, not the typed string: +/// `007` is stored as 7 on both ends (improvise-3tt4). Evidence file. +#[test] +fn tab_at_the_rightmost_column_reopens_on_the_stored_spelling() { + let mut client_host = precise_host(improvise_protocol::CellValue::Number(4.0)); + client_host.cursor_mut().selected = (0, 1); + let mut key_host = precise_host(improvise_protocol::CellValue::Number(4.0)); + key_host.cursor_mut().selected = (0, 1); + let mut client = client_ready_to_edit(&client_host, (2, 2)); + for key in ["i", "Backspace", "0", "0", "7", "Tab"] { + let event = BrowserKey::new(key).to_key_event().unwrap(); + let KeyOutcome::Local(cmds) = resolve(&client, event) else { + panic!("{key}: the client must answer this key locally"); + }; + for cmd in &cmds { + client.fold(cmd); + } + apply_on(&mut client_host, &cmds); + reduce_key(&mut key_host, event); + } + assert_eq!(key_host.view_state.buffers.get("edit").map(String::as_str), Some("7")); + assert_eq!(client.view.buffers, key_host.view_state.buffers); +} + /// BUG (improvise-fy9o) — see `context/test-evidence/clients.md`. #[test] fn typing_into_the_reopened_editor_extends_the_cell_on_both_ends() {