diff --git a/context/code/clients.md b/context/code/clients.md index 7117ad5e..4e409168 100644 --- a/context/code/clients.md +++ b/context/code/clients.md @@ -3064,6 +3064,14 @@ held window is `None`, `settle_opened` simply tries again on the next fold, and the capture lands once the host projects the cell. #274 took #286 by merge rather than porting the reader; the PR body says why. +**A captured commit is not judged by the cursor's cell either (improvise-3h9w).** +#286's `refused_here` declines a commit key whose cursor cell `i` would decline, +and waits on one that is unprojected. It runs in `step()` before `commit()`, +so it must skip an editor that captured its target: that commit writes, or +basis-refuses, the captured cell, whatever now sits under the cursor. The guard +is kept for an editor that captured nothing, which is the case it was written +for. + **What the commit is follows the capture too.** Whether `Enter` writes a value or renames a coordinate is read off `opened.target()`, not off the cell under the cursor (ChatGPT Codex on PR #274): a relayed reframe can leave the cursor on diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index aabf11dd..3596d76b 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -5750,3 +5750,25 @@ is a `Step::Declined`, shared by the value and rename paths. - **Rival: `renamed_away()` is `Step::Local`** (the state before the fix). Fails only this test. It drives the value path; the rename path reaches the same function, so no rival can split them without also changing the code shape. + +### a_captured_edit_commits_when_the_cursor_now_sits_on_an_aggregate + +improvise-3h9w, a regression from merging #286 into #274, found by ChatGPT Codex +and the delta review at `8c28360f`. `step()` runs `refused_here` before +`commit()` consults the capture, and for the commit keys `refused_here` judged +the **cursor's** cell. A opens an editor on North/Cost and types `42`. B moves +Region to None, the relays reset A's cursor, and it now sits on an aggregate. +A's `Enter` was declined with "Cannot write to an aggregate" on every retry, no +frame was sent, and Esc lost the text, where `8d9de94e` wrote North/Cost. It +failed at the North/Cost assertion with `None`. + +The vanished-rows test could not see this, because `unprojected` exempts an +empty grid, so no guard fired there. This case needs the cell under the cursor +to change **kind** while it stays projected. + +- **Rival: `refused_here` without the `state.opened.is_some()` arm** (the state + before the fix). Fails only this test. +- **Not covered:** a captured editor whose cursor lands on a cell that is + inside the grid but outside the held window. The same arm exempts it, but no + test builds it, because every reframe here resets the cursor to the origin, + which is always inside the window. diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index c5e9a3b8..3da8df8b 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -344,11 +344,12 @@ fn committed(state: &ClientState, name: &str, args: &[String]) -> Option { } /// A commit onto a cell `i` declines is refused as `committable_cell_key` refuses -/// it, out loud with the binding (ik6l); a cell op on an unprojected cell waits (7mro). +/// it (ik6l); a cell op on an unprojected cell waits (7mro). Not a captured commit (3h9w). fn refused_here(state: &ClientState, name: &str) -> Option { let (row, col) = state.view.cursor().selected; let waiting = unprojected(state, row, col).then(|| NOT_PROJECTED.to_string()); match name { + "commit-cell-edit" | "commit-and-advance-right" if state.opened.is_some() => None, "commit-cell-edit" | "commit-and-advance-right" => declined_at(state, row, col).or(waiting), "clear-cell" | "yank" | "paste" => waiting, _ => None, diff --git a/crates/improvise-wasm-client/tests/browser_round_trip.rs b/crates/improvise-wasm-client/tests/browser_round_trip.rs index 638bb3c1..ee89e1fd 100644 --- a/crates/improvise-wasm-client/tests/browser_round_trip.rs +++ b/crates/improvise-wasm-client/tests/browser_round_trip.rs @@ -2008,3 +2008,76 @@ fn a_retry_whose_cell_is_gone_keeps_the_typed_text() { "the text the user typed must survive the decline" ); } + +/// `host()` with `Profit = Revenue - Cost`, so a grid with Region on None +/// shows aggregates and a derived column, and no stored cell. +fn host_with_profit() -> Host { + let mut wb = Workbook::new("Test"); + wb.add_category("Region").unwrap(); + wb.add_item("Region", "North").unwrap(); + wb.add_item("Region", "South").unwrap(); + wb.add_item("_Measure", "Revenue").unwrap(); + wb.add_item("_Measure", "Cost").unwrap(); + wb.model.add_formula( + improvise_command::formula::parse_formula("Profit = Revenue - Cost", "_Measure").unwrap(), + ); + wb.active_view_mut().set_axis("Region", Axis::Row); + wb.active_view_mut().set_axis("_Measure", Axis::Column); + wb.model.set_cell( + CellKey::new(vec![ + ("Region".into(), "North".into()), + ("_Measure".into(), "Revenue".into()), + ]), + CellValue::Number(1200.0), + ); + let mut app = App::new(wb, None); + app.rebuild_layout(); + Host::new(app) +} + +/// improvise-3h9w: B's reframe left A's cursor on an aggregate, and Enter was +/// refused for THAT cell instead of writing the captured one. Evidence file. +#[test] +fn a_captured_edit_commits_when_the_cursor_now_sits_on_an_aggregate() { + use improvise_command::ui::reduce::caught_up; + use improvise_protocol::ModelCommand; + + let mut host = host_with_profit(); + let (mut a, mut b) = (Client::new(), Client::new()); + let (a_id, a_snap) = host.connect(10, 10); + let (b_id, b_snap) = host.connect(10, 10); + a.set_viewport(10, 10); + b.set_viewport(10, 10); + deliver(&mut a, &[a_snap]); + deliver(&mut b, &[b_snap]); + for key in ["l", "i", "4", "2"] { + press(&mut host, a_id, &mut a, key); + } + let region_off = ModelCommand::SetAxis { + view: host.app().view_name().to_string(), + category: "Region".into(), + axis: Axis::None, + }; + let relayed = host.handle( + b_id, + ClientFrame::command(caught_up(host.app(), region_off)), + ); + route(&relayed, (a_id, &mut a), (b_id, &mut b)); + assert!(a.state().view.mode.is_editing(), "setup: still editing"); + let (row, col) = a.state().view.cursor().selected; + assert!( + matches!( + a.state().grid.cell(row, col).and_then(|c| c.id.clone()), + Some(improvise_protocol::CellId::Aggregate(_)) + ), + "setup: the cursor now sits on an aggregate" + ); + + press(&mut host, a_id, &mut a, "Enter"); + assert_eq!( + cell(&host, "North", "Cost"), + Some(CellValue::Number(42.0)), + "the captured cell must be written: status {:?}", + a.state().view.status_msg + ); +}