From 7bf8debd12c7e3f346238bd0dce64b940e841873 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 17:49:43 -0700 Subject: [PATCH 1/4] The keyboard editor commits to the cell it opened on (improvise-8xq0, improvise-xz2z) resolve.rs::written read its operands at Enter: the record and measure from the cell under the cursor, and the basis from claim(), i.e. the version current at commit. So a HideItem from another client that renumbered the rows under an open editor sent the write to whichever cell the cursor then named (8xq0), and a write by another client to the record while the editor was open was silently overwritten instead of refused RecordMoved (xz2z, 0j0f's rule on the keyboard door). The fix reuses #221's capture rather than adding a second one. ClientState::opened becomes OpenedCell { record, target, version, document_generation }, where OpenedTarget is Value { measure } or Coordinate { category }. settle_opened captures a Record cell as a Value target, and written builds the RecordCell and the claim_as_read basis from it. With no capture while the host has yet to answer this client's own last write, the commit declines with the rename's "still waiting" status. Red first: both round-trip probes failed on gh/main 202e7eac and on #221's head 39821d5f. Unit-test fixtures that set Editing by assignment now open the editor through the fold, which is where the capture is taken. Stacked on #221 (fix/fdk1-coordinate-rename). Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/clients.md | 35 +++- context/code/protocol.md | 10 +- context/test-evidence/clients.md | 71 ++++++++- crates/improvise-protocol/src/client.rs | 37 +++-- crates/improvise-protocol/src/resolve.rs | 149 ++++++++++++------ .../tests/browser_round_trip.rs | 148 +++++++++++++++++ .../improvise-wasm-client/tests/lockstep.rs | 8 +- 7 files changed, 391 insertions(+), 67 deletions(-) diff --git a/context/code/clients.md b/context/code/clients.md index 12bd469d..d1ff7085 100644 --- a/context/code/clients.md +++ b/context/code/clients.md @@ -2851,9 +2851,11 @@ they answer different questions: text it opened on, and whether the user has changed it**. It is portable state. It is composed optimistically at the keypress and folded the same way on the host, where `CommitAndAdvance` reads it. -- `ClientState::opened` (`OpenedCoordinate { record, category, version, document_generation }`) - answers **which record the rename names, and at which version the basis - claims it**. It is session-local and never serialized. +- `ClientState::opened` (`OpenedCell { record, target, version, document_generation }`) + answers **which cell the commit writes, and at which version the basis + claims it**. `target` is `Coordinate { category }` for a rename and + `Value { measure }` for a stored value. It is session-local and never + serialized. Neither answers "is an editor open": the mode does, for both. `opened_editor` is not cleared when the editor closes, only replaced when the next one opens. @@ -2875,3 +2877,30 @@ editor reopens with the typed text. `settle_opened` then captures the record at its **current** version, so a second `Enter` succeeds. That is the one-keystroke force `RecordMoved` promises (gotcha 43). The user has just been told that someone else changed the row. + +**A value editor commits to the cell it opened on, too (improvise-8xq0).** The +keyboard commit over a plain grid cell used to read the cursor at `Enter`: the +record from `record_at_cursor`, the measure from the `CellId` under it, and the +basis from `claim`, which reads the version as it is *now*. Another client's +`HideItem` renumbers the rows under an open editor, the cursor then names a +different cell, and the write landed there with the text typed for the first. +The inline `EditTarget` fixed this for the mouse editor (improvise-2wu3), but it +lives in the page, so the keyboard path never saw it. The fix reuses the rename +capture rather than adding a second one: `settle_opened` captures a `Record` +cell as `OpenedTarget::Value`, and `written` builds the `RecordCell` and the +`claim_as_read` basis from it. Nothing about the write is read at `Enter` except +the text. Whether the commit is a rename, a creating column or a value is still +decided by the cell under the cursor, as it was. + +Two things come with it. **The version is the one read at open**, so a write by +another client while the editor is open is refused `RecordMoved` on this path +as well: improvise-0j0f's rule, which the keyboard path had not had +(improvise-xz2z). And **a +value commit can now wait**: the capture waits for the host's answer to this +client's own last write, so `Enter` pressed again inside one round trip has no +target yet. It declines with the rename's "still waiting" status and writes +nothing. With no write in flight and no capture, the cursor is on no cell an +editor writes, and the commit answers `None` as before. The waiting decline is +still followed by the `Enter` binding's welded `clear-buffer`, so the typed +text is lost on that path, for a rename as for a value (improvise-joed). + diff --git a/context/code/protocol.md b/context/code/protocol.md index 10d6af24..f2975dd2 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -622,8 +622,8 @@ it, and the address it needs is the name. What was missing was therefore never an address but a **recognition**. `Create` carries the kind and nothing else, because the kind plus the typed name is the whole operand. It is not a `SetCell` target, so `render.rs::editable` stays -`Record`-only and `record_at_cursor` still answers `None` — both by matching, -which the compiler now forces. +`Record`-only and `opened_at_cursor` captures no editor target for it — both +by matching, which the compiler now forces. **No client composes it yet**, and that is one gap rather than this one: `keys.rs::commit` returns early for every records-mode commit, because a @@ -665,9 +665,9 @@ by surrogate and the new coordinate by value. `records_named` names the record, because a rename moves it (gotcha 43). Creating the item is internal to the effect. `render.rs::editable` still answers `false` for a coordinate: the page's inline editor writes stored cells only, and the rename's producer is the -keyboard path (`context/code/clients.md`). `typed_cell_command`, -`record_at_cursor` and `record_cell_at_cursor` still answer `None` for it, so -paste onto a coordinate is still improvise-kkcy. +keyboard path (`context/code/clients.md`). `typed_cell_command` and +`record_cell_at_cursor` still answer `None` for it, so paste onto a coordinate +is still improvise-kkcy. ### `CellDisplay::id` — `None` is not "aggregate" diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 96102811..7183c150 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -5235,7 +5235,7 @@ still pinned by `an_edit_writes_the_cell_the_user_clicked_after_the_rows_renumbe Added when #221 merged #219 (improvise-vndi). vndi changed `claim_as_read` to take the document generation captured when an editor opened, so an inline edit opened before a `LoadModel` is refused `DocumentReplaced` rather than written -into the new document. The rename capture (`OpenedCoordinate`) now records +into the new document. The rename capture (`OpenedCoordinate`, now `OpenedCell`) records `document_generation` at open for the same reason, and the rename claims it. **No rival of that choice is observable, and this test says why.** B loads a @@ -5261,3 +5261,72 @@ sits on row 2, which in the answered grid is Zed, the record the first rename produced. The test's principle is unchanged: the second rename moves the row the client shows, and that row differs from the stale capture (West). Only its value table assumed Zed could not come up, so Zed was added to it. + +### a_keyboard_edit_writes_the_cell_it_opened_on_after_the_rows_renumber + +improvise-8xq0, the ticket's probe, through a real `Host` with two clients. A +presses `l`, `i` on North/Cost at (0,1), and types `42`. B hides North, and A +folds the relayed frames, so the rows renumber under the open editor. A then +presses `Enter`. `resolve.rs::commit` composed `SetCell` from the cell under +the cursor **at Enter**. The cursor had been reset to the origin, which now +held South/Revenue, so South/Revenue became 42 and the host accepted it. That +failed at the `South was written` assertion, on `gh/main` at `202e7eac` and on +#221's head `39821d5f` alike, so #221's coordinate capture did not reach this +path. + +The editor opens on North/**Cost**, not the first column, so the cell at Enter +differs from the opened one in both record and measure. A fix that kept the +record and read the measure at Enter would still write the wrong column. + +- **Rival: `written` takes the whole cell from the cursor at Enter** (the bug). + Fails only this test, at `South was written`. +- **Rival: the record from the capture, the measure from the cursor.** Fails + only this test, at `the edit must land on the cell the editor opened on`: + North/Cost stays empty. +- **Rival: the basis claimed at commit (`claim`) instead of as read.** Fails + this test too: after the hide, A's basis no longer names North's record, so + `claim` leaves it out and the host answers `RecordUnstated`. + +### a_keyboard_edit_is_refused_when_its_record_was_written_while_open + +improvise-xz2z: improvise-0j0f's rule on the keyboard path, which it had not +had. xz2z was filed separately (by agent-227-fixes, scoping improvise-3al1) and +has the same cause as 8xq0: `written` read its operands at `Enter`. Same setup, +but B writes 999 into North/Revenue while A's editor is open on North/Cost. A +record is the coordinate tuple minus `_Measure` (gotcha 40), so that write +moves the version of the record A is editing. The keyboard commit claimed the +version current at Enter, which A had already folded, so the host accepted it. +It failed at the `RecordMoved` assertion, on `gh/main` and on #221's head. + +- **Rival: the basis claimed at commit instead of as read.** Fails this test + (and the renumber test, above). + +### a_second_keyboard_commit_writes_the_cell_the_advance_landed_on + +A regression guard for the capture's lifetime, which the fix made +load-bearing for value cells. `i`, `5`, `Enter` on North/Revenue writes 12005, +advances to South/Revenue and reopens the editor there; `7`, `Enter` must write +South/Revenue. The capture is dropped when the commit's `ChangeMode(Normal)` +folds and retaken for the reopened editor once the host answers. + +- **Rival: `settle_opened` keeps a capture after the editor closes.** Fails + this test, the second commit writing North again. Before this test only #221's + two coordinate tests saw that rival + (`a_second_rename_after_a_reorder_renames_the_row_the_host_shows` and + `a_document_swap_closes_an_open_rename_editor`); nothing on a value cell did. + +### a_value_commit_before_the_host_answers_the_last_one_waits_out_loud + +With the value target captured only after the host answers this client's own +last write, `Enter` pressed again before that answer has no target. The test +commits once, folds the frame as sent and not yet answered, and presses +`Enter` again in the reopened editor. The fixture's second row is given a +`Record` identity, or `commit` stops at the cursor's cell before it reaches the +capture and the test would pass for the wrong reason. + +- **Rival: no waiting decline (`None`, as for a cell no editor writes).** Fails + only this test: the key goes upstream, which in a browser does nothing. + +The test asserts the status and that nothing is written, not the exact +command list: the `Enter` binding's welded `clear-buffer` still runs after the +decline and empties the buffer. That loss is improvise-joed. diff --git a/crates/improvise-protocol/src/client.rs b/crates/improvise-protocol/src/client.rs index df670a92..8a4b6502 100644 --- a/crates/improvise-protocol/src/client.rs +++ b/crates/improvise-protocol/src/client.rs @@ -44,23 +44,32 @@ pub struct ClientState { /// replay the rest. Why a queue: `context/code/clients.md`. #[serde(skip)] pub in_flight: std::collections::VecDeque, - /// The coordinate an open editor renames, captured as the user saw it. + /// The cell an open editor commits to, captured as the user saw it. /// When it is taken, and when not yet: `context/code/clients.md`. #[serde(skip)] - pub opened: Option, + pub opened: Option, } -/// A coordinate editor's target: the record, the category, and the record's -/// write counter when the editor opened (improvise-fdk1, improvise-0j0f). +/// A cell editor's target: the record, which cell of it, and the record's write +/// counter when the editor opened (improvise-fdk1, improvise-0j0f, improvise-8xq0). #[derive(Debug, Clone, PartialEq, Eq)] -pub struct OpenedCoordinate { +pub struct OpenedCell { pub record: RecordHash, - pub category: String, + pub target: OpenedTarget, pub version: u64, /// The document the editor opened on, so a swap refuses it (improvise-vndi). pub document_generation: u64, } +/// Which cell of the record an editor opened on, as the projection named it. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum OpenedTarget { + /// A stored value: `SetCell`/`ClearCell` names it (improvise-8xq0). + Value { measure: Option }, + /// A records-mode coordinate: `RenameRecordCoordinate` names it. + Coordinate { category: String }, +} + /// One frame this client has sent. `awaited` is true exactly when the host /// will answer it, which is exactly when it can be refused — `clients.md`. #[derive(Debug, Clone, PartialEq)] @@ -181,19 +190,25 @@ impl ClientState { self.view.mode = settled_mode(&self.view.mode, self.grid.records); } - /// Capture the coordinate an open editor renames, but only from a cache that + /// Whether a frame the host will answer is still in flight, so this + /// client's cache does not yet show the host's answer to its own writes. + pub(crate) fn awaiting_answer(&self) -> bool { + self.in_flight.iter().any(|sent| sent.awaited) + } + + /// Capture the cell an open editor commits to, but only from a cache that /// shows the host's answer to this client's own writes (improvise-fdk1). fn settle_opened(&mut self) { - let answered = !self.in_flight.iter().any(|sent| sent.awaited); + let answered = !self.awaiting_answer(); self.opened = match (self.view.mode.is_editing(), self.opened.take()) { (false, _) => None, (true, Some(opened)) => Some(opened), (true, None) if answered => { - crate::resolve::coordinate_at_cursor(self).and_then(|(record, category)| { + crate::resolve::opened_at_cursor(self).and_then(|(record, target)| { let basis = self.basis.current()?; - Some(OpenedCoordinate { + Some(OpenedCell { record, - category, + target, version: basis.record(record)?, document_generation: basis.document_generation, }) diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index cdb2c78c..cd9e833c 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -4,7 +4,7 @@ use improvise_core::view::Axis; use serde::{Deserialize, Serialize}; -use crate::client::ClientState; +use crate::client::{ClientState, OpenedTarget}; use crate::frame::ClientFrame; use crate::keymap::{Binding, Keymaps, ModeKey}; use crate::viewmodel::{CatTreeEntry, axis_change_status, flatten_cat_tree}; @@ -800,7 +800,7 @@ fn edit_or_drill(state: &ClientState, mode: AppMode) -> Option> { /// The record and category of the coordinate cell under the cursor, as the /// projection names them — never borrowed from a sibling cell (ADR 002). -pub(crate) fn coordinate_at_cursor(state: &ClientState) -> Option<(crate::RecordHash, String)> { +fn coordinate_at_cursor(state: &ClientState) -> Option<(crate::RecordHash, String)> { match cell_at_cursor(state)?.id.as_ref()? { CellId::Coordinate { record, category } => Some((*record, category.clone())), CellId::Record(_) | CellId::Aggregate(_) | CellId::Derived { .. } | CellId::Create(_) => { @@ -809,6 +809,26 @@ pub(crate) fn coordinate_at_cursor(state: &ClientState) -> Option<(crate::Record } } +/// The record and cell an editor opened on the cursor commits to, as the +/// projection names them (improvise-8xq0). `None` for a cell no editor writes. +pub(crate) fn opened_at_cursor(state: &ClientState) -> Option<(crate::RecordHash, OpenedTarget)> { + match cell_at_cursor(state)?.id.as_ref()? { + CellId::Record(cell) => Some(( + cell.record, + OpenedTarget::Value { + measure: cell.measure.clone(), + }, + )), + CellId::Coordinate { record, category } => Some(( + *record, + OpenedTarget::Coordinate { + category: category.clone(), + }, + )), + CellId::Aggregate(_) | CellId::Derived { .. } | CellId::Create(_) => None, + } +} + /// Open the cell editor on `seed`: the buffer, what it captured, then the mode. /// `EditOrDrill`'s and `EnterEditAtCursor`'s shared shape (improvise-ixxx). fn opened_on(seed: String, mode: AppMode) -> Vec { @@ -928,7 +948,9 @@ fn commit(state: &ClientState, edit_mode: AppMode, advance: Advance) -> Option vec![Command::SetStatus( RECORD_COORDS_CANNOT_BE_EMPTY.to_string(), )], - (false, false, None) => { - return Some(vec![Command::SetStatus( - "still waiting for the host to answer your last edit".to_string(), - )]); + (false, false, None) => return Some(still_waiting()), + (false, false, Some(opened)) => { + let OpenedTarget::Coordinate { category } = &opened.target else { + return Some(still_waiting()); + }; + vec![Command::Model { + basis: state.basis.claim_as_read( + opened.document_generation, + [(opened.record, opened.version)], + )?, + cmd: ModelCommand::RenameRecordCoordinate { + record: opened.record, + category: category.clone(), + item: text.to_string(), + }, + }] } - (false, false, Some(opened)) => vec![Command::Model { - basis: state.basis.claim_as_read( - opened.document_generation, - [(opened.record, opened.version)], - )?, - cmd: ModelCommand::RenameRecordCoordinate { - record: opened.record, - category: opened.category.clone(), - item: text.to_string(), - }, - }], }; commands.push(Command::ChangeMode(AppMode::Normal)); commands.extend(advanced(state, edit_mode, advance)?); Some(commands) } -/// The write a commit makes: none when the buffer is the seed the editor -/// captured as it opened, and only the write is skipped (improvise-ixxx). -fn written(state: &ClientState, id: &CellId, text: &str) -> Option> { - match untouched(state.view.opened_editor.as_ref(), text) { - true => Some(Vec::new()), - false => Some(vec![Command::Model { - basis: state.basis.claim([record_at_cursor(state)?])?, - cmd: typed_cell_command(id, text)?, - }]), +/// The write a commit makes: none when untouched (improvise-ixxx), else to the +/// cell the editor OPENED on, at the version it read then (improvise-8xq0). +fn written(state: &ClientState, text: &str) -> Option> { + if untouched(state.view.opened_editor.as_ref(), text) { + return Some(Vec::new()); } + let opened = state.opened.as_ref()?; + let OpenedTarget::Value { measure } = &opened.target else { + return None; + }; + let cell = RecordCell { + record: opened.record, + measure: measure.clone(), + }; + Some(vec![Command::Model { + basis: state.basis.claim_as_read( + opened.document_generation, + [(opened.record, opened.version)], + )?, + cmd: typed_cell_command(&CellId::Record(cell), text)?, + }]) +} + +/// A commit whose target is not captured yet, because the host has not answered +/// this client's own last write. The editor stays open (improvise-fdk1). +fn still_waiting() -> Vec { + vec![Command::SetStatus( + "still waiting for the host to answer your last edit".to_string(), + )] } /// `CommitAndAdvance` over a creating column: a schema change over the typed @@ -1054,18 +1096,6 @@ fn advanced(state: &ClientState, edit_mode: AppMode, advance: Advance) -> Option Some(commands) } -/// 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 { - match cell_at_cursor(state)?.id.as_ref()? { - CellId::Record(cell) => Some(cell.record), - CellId::Aggregate(_) - | CellId::Derived { .. } - | CellId::Create(_) - | CellId::Coordinate { .. } => None, - } -} - /// What an editor opened on the cursor starts with, or `None` with no cell /// there to seed from. Read by the fold, never a renderer (improvise-ktlk). pub(crate) fn edit_seed(state: &ClientState) -> Option { @@ -2265,7 +2295,7 @@ mod tests { // A records MODE over a non-records GRID is a state no host can // produce, so the fixture projects both (improvise-e5pf). crate::WidgetCache::apply(&mut state.grid, &GridUpdate::Records(mode.is_records())); - state.view.mode = mode; + state.fold(&Command::ChangeMode(mode)); state.grid.page_coords = vec![crate::PageAxis { cat: "Quarter".into(), selected: "Q2".into(), @@ -2978,7 +3008,7 @@ mod tests { // Half two: the predicate is about *finishing*, so something has to // press Enter and observe. let mut client = client_on(record_cell()); - client.view.mode = AppMode::editing(); + client.fold(&Command::ChangeMode(AppMode::editing())); client.view.buffers.insert("edit".into(), "42".into()); let KeyOutcome::Local(commands) = resolve(&client, ev(KeyCode::Enter)) else { panic!("Enter in Editing must resolve here — that is the arm"); @@ -3432,12 +3462,43 @@ mod tests { ); } + /// improvise-8xq0: a commit whose target is not captured yet, because the + /// last write is unanswered, declines out loud. Evidence file. + #[test] + fn a_value_commit_before_the_host_answers_the_last_one_waits_out_loud() { + let mut client = client_on(record_cell()); + // The row the advance lands on is a stored cell too, so the commit gets + // past the cursor's cell and reaches the capture. + let mut below = CellDisplay::number("900"); + below.id = Some(record_cell()); + client.fold(&Command::Project(Projection::Grid(GridUpdate::Cells( + vec![((1, 0), below)], + )))); + client.fold(&Command::ChangeMode(AppMode::editing())); + client.view.buffers.insert("edit".into(), "42".into()); + let KeyOutcome::Local(first) = resolve(&client, ev(KeyCode::Enter)) else { + panic!("Enter in Editing must resolve here"); + }; + client.fold_outgoing(&first); + assert!(client.view.mode.is_editing(), "setup: the advance reopened"); + assert_eq!(client.opened, None, "setup: nothing captured yet"); + client.view.buffers.insert("edit".into(), "43".into()); + let KeyOutcome::Local(second) = resolve(&client, ev(KeyCode::Enter)) else { + panic!("a commit with no target yet must answer, not go upstream"); + }; + assert_eq!(second[..1], still_waiting()[..], "it says so: {second:?}"); + assert!( + !second.iter().any(|c| matches!(c, Command::Model { .. })), + "and it must write nothing: {second:?}" + ); + } + /// Enter in `Editing` is `[commit-cell-edit editing, clear-buffer edit]` /// and every part shows up here — including the odd-looking trailing clear. #[test] fn enter_commits_the_cell_advances_and_re_enters_editing() { let mut client = client_on(record_cell()); - client.view.mode = AppMode::editing(); + client.fold(&Command::ChangeMode(AppMode::editing())); client.view.buffers.insert("edit".into(), "42".into()); let KeyOutcome::Local(commands) = resolve(&client, ev(KeyCode::Enter)) else { panic!("Enter in Editing must resolve here"); @@ -3733,7 +3794,7 @@ mod tests { #[test] fn committing_an_empty_buffer_clears_the_cell() { let mut client = client_on(record_cell()); - client.view.mode = AppMode::editing(); + client.fold(&Command::ChangeMode(AppMode::editing())); client.view.buffers.insert("edit".into(), String::new()); let KeyOutcome::Local(commands) = resolve(&client, ev(KeyCode::Enter)) else { panic!("Enter in Editing must resolve here"); @@ -3814,7 +3875,7 @@ mod tests { ], }); client.window = (1, 1); - client.view.mode = AppMode::editing(); + client.fold(&Command::ChangeMode(AppMode::editing())); client.view.buffers.insert("edit".into(), "7".into()); let KeyOutcome::Local(commands) = resolve(&client, ev(KeyCode::Enter)) else { diff --git a/crates/improvise-wasm-client/tests/browser_round_trip.rs b/crates/improvise-wasm-client/tests/browser_round_trip.rs index 279cb473..eb6f5127 100644 --- a/crates/improvise-wasm-client/tests/browser_round_trip.rs +++ b/crates/improvise-wasm-client/tests/browser_round_trip.rs @@ -1311,3 +1311,151 @@ fn a_second_rename_after_a_reorder_renames_the_row_the_host_shows() { "West untouched" ); } + +// ── the keyboard editor's target (improvise-8xq0) ──────────────────────── + +/// What client B does while A's keyboard editor is open on North/Cost. +#[derive(PartialEq)] +enum Meanwhile { + HideNorth, + WriteNorth, +} + +/// Client A presses `i` on North/Cost (0,1) and types `42`; B does `meanwhile` +/// before A presses Enter. Evidence file. +fn commit_north_cost(meanwhile: Meanwhile) -> (Host, Vec) { + use improvise_command::ui::reduce::caught_up; + use improvise_protocol::ModelCommand; + + let mut host = host(); + 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); + } + assert_eq!( + a.state().view.cursor().selected, + (0, 1), + "setup: on North/Cost" + ); + assert!( + a.state().view.mode.is_editing(), + "setup: the editor is open" + ); + + let view = host.app().view_name().to_string(); + let cmd = match meanwhile { + Meanwhile::HideNorth => ModelCommand::HideItem { + view, + category: "Region".into(), + item: "North".into(), + }, + Meanwhile::WriteNorth => ModelCommand::SetCell { + cell: a.record_cell_at(0, 0).expect("North/Revenue is stored"), + value: CellValue::Number(999.0), + }, + }; + let relayed = host.handle(b_id, ClientFrame::command(caught_up(host.app(), cmd))); + route(&relayed, (a_id, &mut a), (b_id, &mut b)); + assert!(a.state().view.mode.is_editing(), "setup: still editing"); + if meanwhile == Meanwhile::HideNorth { + assert_eq!( + a.state().grid.row_frame.leaf_labels().first().cloned(), + Some("South".to_string()), + "setup: the rows renumbered under the open editor" + ); + } + + let frame = a + .key(&BrowserKey::new("Enter")) + .expect("a commit composes a frame"); + let json = serde_json::to_string(&frame).expect("a client frame serializes"); + let answers = send(&mut host, a_id, &json); + (host, answers) +} + +fn cell(host: &Host, region: &str, measure: &str) -> Option { + host.app() + .model_state + .workbook + .model + .get_cell(&CellKey::new(vec![ + ("Region".into(), region.into()), + ("_Measure".into(), measure.into()), + ])) + .cloned() +} + +/// improvise-8xq0: B hid the row under A's open editor, and Enter wrote the +/// cell under the cursor at Enter. Evidence: `context/test-evidence/clients.md`. +#[test] +fn a_keyboard_edit_writes_the_cell_it_opened_on_after_the_rows_renumber() { + let (host, answers) = commit_north_cost(Meanwhile::HideNorth); + assert!( + answers + .iter() + .any(|f| matches!(f.msg, improvise_protocol::ServerFrame::Accepted)), + "the write must be accepted: {answers:?}" + ); + assert_eq!(cell(&host, "South", "Revenue"), None, "South was written"); + assert_eq!(cell(&host, "South", "Cost"), None, "South was written"); + assert_eq!( + cell(&host, "North", "Cost"), + Some(CellValue::Number(42.0)), + "the edit must land on the cell the editor opened on" + ); +} + +/// improvise-8xq0: the editor an advance reopens commits to the cell it landed +/// on, not the one the last commit captured. Evidence file. +#[test] +fn a_second_keyboard_commit_writes_the_cell_the_advance_landed_on() { + let mut host = host(); + let mut a = Client::new(); + a.set_viewport(10, 10); + let (id, snapshot) = host.connect(10, 10); + deliver(&mut a, &[snapshot]); + for key in ["i", "5", "Enter"] { + press(&mut host, id, &mut a, key); + } + assert_eq!(a.state().view.cursor().selected, (1, 0), "setup: advanced"); + assert!(a.state().view.mode.is_editing(), "setup: reopened"); + for key in ["7", "Enter"] { + press(&mut host, id, &mut a, key); + } + assert_eq!( + cell(&host, "North", "Revenue"), + Some(CellValue::Number(12005.0)) + ); + assert_eq!( + cell(&host, "South", "Revenue"), + Some(CellValue::Number(7.0)), + "the second commit must write the cell the advance landed on" + ); +} + +/// improvise-xz2z, 0j0f's rule on the keyboard path: the basis is the one read +/// when the editor opened. Evidence: `context/test-evidence/clients.md`. +#[test] +fn a_keyboard_edit_is_refused_when_its_record_was_written_while_open() { + let (host, answers) = commit_north_cost(Meanwhile::WriteNorth); + assert!( + answers.iter().any(|f| matches!( + f.msg, + improvise_protocol::ServerFrame::Rejected(improvise_protocol::Rejection::RecordMoved( + _ + )) + )), + "an edit composed against a record someone else wrote must be refused: {answers:?}" + ); + assert_eq!( + cell(&host, "North", "Revenue"), + Some(CellValue::Number(999.0)) + ); + assert_eq!(cell(&host, "North", "Cost"), None, "nothing was written"); +} diff --git a/crates/improvise-wasm-client/tests/lockstep.rs b/crates/improvise-wasm-client/tests/lockstep.rs index ee3bc6f2..d1a0e23e 100644 --- a/crates/improvise-wasm-client/tests/lockstep.rs +++ b/crates/improvise-wasm-client/tests/lockstep.rs @@ -1610,13 +1610,15 @@ fn a_records_commit_is_declined_off_the_grid_and_not_off_the_mode() { // The control: the same mode over a REGULAR grid must still commit, or the // decline above is "this client never writes" and proves nothing. let plain = furnished_host(); - let mut ordinary = paged_client(&plain); - ordinary.view.mode = AppMode::editing(); + // Seeded as `records_client` seeds, basis included, over a regular grid; + // opened through the fold, which captures the cell the commit writes. + let mut ordinary = records_client(&plain); + ordinary.view.cursor_mut().selected = stored_cell_at(&ordinary); + ordinary.fold(&improvise_protocol::Command::ChangeMode(AppMode::editing())); ordinary .view .buffers .insert("edit".into(), "42".to_string()); - ordinary.view.cursor_mut().selected = stored_cell_at(&ordinary); assert!(!ordinary.grid.records, "the control must be a regular grid"); assert!( composes_a_write(&ordinary, enter), From 8d9de94e0823ef061b3623bc0343961da678a970 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 18:29:33 -0700 Subject: [PATCH 2/4] A value retry writes only the cell it named; Enter follows the capture (improvise-8phw) 8phw: when A's Enter crossed B's HideItem, the LayoutMoved refusal replayed the editor over South and settle_opened recaptured South/Cost, so the retry wrote there. #251's Opened::Retry/Gone now covers values: written_to() recognises SetCell/ClearCell as well as a rename, and Retry and Gone carry the whole OpenedTarget, which the recapture must match. A Gone value editor declines with RECORD_RENAMED_AWAY. seen stays rename-only. Codex P2 on #274: commit picked value/rename/create from the cell under the cursor before it consulted the capture, so a reframe that left no cell there sent a captured editor's Enter upstream. commit now dispatches on opened.target(); only an uncaptured editor is dispatched by the cursor. A records grid's measure cell is not captured as a value, since the browser still declines it. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/clients.md | 46 +++-- context/test-evidence/clients.md | 53 +++++ crates/improvise-protocol/src/client.rs | 58 ++++-- crates/improvise-protocol/src/resolve.rs | 82 +++++--- .../tests/browser_round_trip.rs | 187 ++++++++++++++++-- 5 files changed, 353 insertions(+), 73 deletions(-) diff --git a/context/code/clients.md b/context/code/clients.md index bc679f46..ad45b5c7 100644 --- a/context/code/clients.md +++ b/context/code/clients.md @@ -2875,21 +2875,29 @@ the host's value would have no record in it. Gating the capture on `enter-mode` records no opening, yet the host still commits a rename from it, so the gate would strand that path on "still waiting". -**A retry after a refusal may rename only the record it named (improvise-20ek, -improvise-9ww8).** A refusal replays the view slice, so the editor reopens with -the typed text over whatever row the cursor now shows. The rule is about that -row: +**A retry after a refusal may write only the cell it named (improvise-20ek, +improvise-9ww8, improvise-8phw).** A refusal replays the view slice, so the +editor reopens with the typed text over whatever cell the cursor now shows. The +rule is about that cell, and it holds for a rename and for a value alike: -- `refused()` pins `ClientState::opened` to `Opened::Retry(record)`, where - `record` is the one the refused rename named. The refusal's kind plays no part. +- `refused()` pins `ClientState::opened` to `Opened::Retry(record, target)`, + naming the cell the refused frame wrote: a rename's record and category, or a + `SetCell`/`ClearCell`'s record and measure (`written_to`). The refusal's kind + plays no part. - Once the host has answered, `settle_opened` captures what is under the cursor. - If that is the same record, it becomes the target at its **current** version, - so a second `Enter` goes through. That is the one-keystroke force - `RecordMoved` promises (gotcha 43), and the user has just been told someone - else changed the row. -- If it is any other record, or none, `opened` becomes `Opened::Gone(record)`, - and `rename_coordinate` answers `Enter` with a status and no write. Esc - leaves the editor, and opening it again captures afresh. + If that is the same record **and** the same `OpenedTarget`, it becomes the + target at its **current** version, so a second `Enter` goes through. That is + the one-keystroke force `RecordMoved` promises (gotcha 43), and the user has + just been told someone else changed the row. +- If it is anything else, or nothing, `opened` becomes `Opened::Gone`, and + `Enter` answers with a status and no write. Esc leaves the editor, and opening + it again captures afresh. + +The whole target is compared, not only the record, because a record is a row +and a value editor names a cell in it: a crossing `HideItem` on a measure leaves +the **same** record under the cursor in another column, and a record-only +comparison forces the retry onto a measure the user never typed for. `seen` +stays rename-only, because only a rename has a destination to claim. **Why the rule names the record rather than a rejection.** The first version (20ek) pinned only on `RecordGone` of the renamed record. But `stale_basis` @@ -2935,8 +2943,16 @@ lives in the page, so the keyboard path never saw it. The fix reuses the rename capture rather than adding a second one: `settle_opened` captures a `Record` cell as `OpenedTarget::Value`, and `written` builds the `RecordCell` and the `claim_as_read` basis from it. Nothing about the write is read at `Enter` except -the text. Whether the commit is a rename, a creating column or a value is still -decided by the cell under the cursor, as it was. +the text. **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 +no cell, on a creating column, or on a records grid, and dispatching on that +sent a captured value editor's `Enter` upstream or down the wrong path. Only an +editor that captured nothing (`uncaptured`) is dispatched by the cursor, and +that is the creating columns plus the waiting cases. A records grid's measure +cell is deliberately not captured as a value, since the browser still declines +editing it (gotcha 1); `every_advertised_command_answers_in_its_own_mode` +fails if it is. Two things come with it. **The version is the one read at open**, so a write by another client while the editor is open is refused `RecordMoved` on this path diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 8a826838..f4bb6bad 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -5461,3 +5461,56 @@ subsumes it: after `RecordGone(r)` the capture can never be `r`, so it becomes `Gone(r)` anyway. A rival that dropped the explicit pin (`gone_as_retry`) failed no test, which is how the redundancy was found. The test's live rivals are now `no_pin_at_all` and `retry_any_capture`. + +### a_keyboard_edit_retried_after_a_crossing_hide_writes_no_other_cell + +improvise-8phw, the value twin of improvise-9ww8, found reviewing PR #274 at +`7bf8debd`. A presses `l i 4 2` on North/Cost. B hides North, and **B's relays +reach A only after A has sent `Enter`**, which is ordinary latency. The host +refuses A's frame `LayoutMoved`. The refusal replays the view, the editor +reopens at (0,1), and `settle_opened` recaptured what was now there, South/Cost. +The retry `Enter` was accepted and wrote 42 into South/Cost. It failed at +`South/Cost was written`. The 8xq0 test cannot see this: it delivers the hide +before `Enter`, so the capture from open time is still in use. + +The test also asserts the retry's status. Without that, a decline that goes +silently upstream would pass. + +- **Rival: `written_to` recognises renames only** (#251 as it was). Fails this + test and the column-hide test below. +- **Rival: a `Gone` value editor answers `None`**, which goes upstream. Fails + only this test, at the status assertion. + +### a_keyboard_edit_retried_after_a_crossing_column_hide_writes_no_other_measure + +improvise-8phw, the measure half. A opens on North/Revenue at (0,0) and types. +B's crossing `HideItem` hides the Revenue *measure*, so after the refusal the +cursor sits on North/Cost: the same **record**, another cell. This is the input +that separates "compare the whole `OpenedTarget`" from "compare the record", +which the row-hide test above cannot do, since there the record differs too. + +- **Rival: `Retry` compares the record only.** Fails only this test: the retry + writes the typed text into North/Cost. + +### a_keyboard_edit_retried_after_a_crossing_write_forces_the_same_cell + +improvise-8phw's control. B's crossing write to North/Revenue moves the +version of A's record, and the refusal is `RecordMoved`. The recapture is +North/Cost again, so the retry must go through: the one-keystroke force of +gotcha 43. A fix that declined every retry would fail here. It passed before +the fix too; it is a guard, not a reproduction. + +### a_keyboard_edit_whose_rows_all_vanished_writes_the_cell_it_opened_on + +ChatGPT Codex's P2 on PR #274 at `7bf8debd`. `commit` picked the operation from +the cell under the cursor before it consulted the capture. B hides both regions +while A's editor is open on North/Cost and A folds the relays, so there is no +cell under the cursor at all. `Enter` then answered `None`, which goes upstream +and does nothing in a browser. The capture was intact the whole time. It failed +at the `Accepted` assertion. + +- **Rival: dispatch on the cursor's cell** (the code before the fix). Fails only + this test. +- **Rival: capture a records grid's measure cell as a value.** Fails + `every_advertised_command_answers_in_its_own_mode`: `RECORDS INSERT` `Enter` + would start writing a cell the browser deliberately declines. diff --git a/crates/improvise-protocol/src/client.rs b/crates/improvise-protocol/src/client.rs index 0d889e26..2533a0f5 100644 --- a/crates/improvise-protocol/src/client.rs +++ b/crates/improvise-protocol/src/client.rs @@ -50,13 +50,23 @@ pub struct ClientState { pub opened: Option, } -/// What an open cell editor commits to. After a refusal the retry may name -/// only the record it named (`Retry`) or none (`Gone`): improvise-20ek, -9ww8. +/// What an open cell editor commits to. After a refusal the retry may name only +/// the cell it named (`Retry`) or none (`Gone`): improvise-20ek, -9ww8, -8phw. #[derive(Debug, Clone, PartialEq, Eq)] pub enum Opened { Target(OpenedCell), - Retry(RecordHash), - Gone(RecordHash), + Retry(RecordHash, OpenedTarget), + Gone(RecordHash, OpenedTarget), +} + +impl Opened { + /// Which kind of cell the editor commits to, whether or not it is captured. + pub fn target(&self) -> &OpenedTarget { + match self { + Opened::Target(cell) => &cell.target, + Opened::Retry(_, target) | Opened::Gone(_, target) => target, + } + } } /// A cell editor's target: the record, which cell of it, the record's write @@ -161,7 +171,7 @@ impl ClientState { if rejection.refused_an_edit() { let dropped = self.in_flight.pop_front(); self.replay(); - self.opened = renamed(dropped.as_ref()).map(Opened::Retry); + self.opened = written_to(dropped.as_ref()).map(|(r, t)| Opened::Retry(r, t)); } self.last_rejection = Some(rejection); self.settle_mode(); @@ -213,10 +223,14 @@ impl ClientState { let answered = !self.awaiting_answer(); self.opened = match (self.view.mode.is_editing(), self.opened.take()) { (false, _) => None, - (true, Some(Opened::Retry(record))) if answered => Some(match self.captured() { - Some(target) if target.record == record => Opened::Target(target), - _ => Opened::Gone(record), - }), + (true, Some(Opened::Retry(record, target))) if answered => { + Some(match self.captured() { + Some(cell) if cell.record == record && cell.target == target => { + Opened::Target(cell) + } + _ => Opened::Gone(record, target), + }) + } (true, Some(opened)) => Some(opened), (true, None) if answered => self.captured().map(Opened::Target), (true, None) => None, @@ -272,13 +286,27 @@ impl ClientState { } } -/// The record a refused frame renamed, if it renamed one (improvise-9ww8). -fn renamed(sent: Option<&Sent>) -> Option { +/// The cell a refused frame wrote, if it wrote one: a rename's coordinate or a +/// value's record and measure (improvise-9ww8, improvise-8phw). +fn written_to(sent: Option<&Sent>) -> Option<(RecordHash, OpenedTarget)> { sent?.cmds.iter().find_map(|cmd| match cmd { - Command::Model { - cmd: ModelCommand::RenameRecordCoordinate { record, .. }, - .. - } => Some(*record), + Command::Model { cmd, .. } => match cmd { + ModelCommand::RenameRecordCoordinate { + record, category, .. + } => Some(( + *record, + OpenedTarget::Coordinate { + category: category.clone(), + }, + )), + ModelCommand::SetCell { cell, .. } | ModelCommand::ClearCell(cell) => Some(( + cell.record, + OpenedTarget::Value { + measure: cell.measure.clone(), + }, + )), + _ => None, + }, _ => None, }) } diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index 5702a31f..7d65dec9 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -813,7 +813,7 @@ fn coordinate_at_cursor(state: &ClientState) -> Option<(crate::RecordHash, Strin /// projection names them (improvise-8xq0). `None` for a cell no editor writes. pub(crate) fn opened_at_cursor(state: &ClientState) -> Option<(crate::RecordHash, OpenedTarget)> { match cell_at_cursor(state)?.id.as_ref()? { - CellId::Record(cell) => Some(( + CellId::Record(cell) if !state.grid.records => Some(( cell.record, OpenedTarget::Value { measure: cell.measure.clone(), @@ -825,7 +825,9 @@ pub(crate) fn opened_at_cursor(state: &ClientState) -> Option<(crate::RecordHash category: category.clone(), }, )), - CellId::Aggregate(_) | CellId::Derived { .. } | CellId::Create(_) => None, + CellId::Record(_) | CellId::Aggregate(_) | CellId::Derived { .. } | CellId::Create(_) => { + None + } } } @@ -939,25 +941,59 @@ fn commit(state: &ClientState, edit_mode: AppMode, advance: Advance) -> Option commit_value(state, edit_mode, advance, &text), + Some(OpenedTarget::Coordinate { .. }) => { + rename_coordinate(state, edit_mode, advance, &text) + } + None => uncaptured(state, edit_mode, advance, &text), } - if state.grid.records { - return rename_coordinate(state, edit_mode, advance, id, &text); +} + +/// An editor that captured nothing: the cell under the cursor says what it is. +/// Records mode needs `record_col` for a measure cell (gotcha 1, improvise-gcvb). +fn uncaptured( + state: &ClientState, + edit_mode: AppMode, + advance: Advance, + text: &str, +) -> Option> { + match cell_at_cursor(state)?.id.as_ref()? { + CellId::Create(kind) => create_column(state, edit_mode, advance, kind, text), + CellId::Coordinate { .. } => rename_coordinate(state, edit_mode, advance, text), + CellId::Record(_) if !state.grid.records => commit_value(state, edit_mode, advance, text), + CellId::Record(_) | CellId::Aggregate(_) | CellId::Derived { .. } => None, } - let Some(mut commands) = written(state, &text) else { - return state.awaiting_answer().then(still_waiting); +} + +/// `CommitAndAdvance` over a stored value: write the cell the editor OPENED on, +/// or decline when a refusal left it no longer there (improvise-8phw). +fn commit_value( + state: &ClientState, + edit_mode: AppMode, + advance: Advance, + text: &str, +) -> Option> { + let unchanged = untouched(state.view.opened_editor.as_ref(), text); + let mut commands = match (unchanged, state.opened.as_ref()) { + (true, _) => Vec::new(), + (false, Some(Opened::Target(opened))) => written(state, opened, text)?, + (false, Some(Opened::Gone(..))) => { + return Some(vec![Command::SetStatus(RECORD_RENAMED_AWAY.to_string())]); + } + (false, None | Some(Opened::Retry(..))) => { + return state.awaiting_answer().then(still_waiting); + } }; commands.push(Command::ChangeMode(AppMode::Normal)); commands.extend(advanced(state, edit_mode, advance)?); Some(commands) } -/// Why a retry after a refusal sends nothing: the record it opened on is no longer -/// the one under the cursor, or is gone (improvise-20ek, improvise-9ww8). +/// Why a retry after a refusal sends nothing: the cell it opened on is no longer +/// the one under the cursor, or is gone (improvise-20ek, -9ww8, -8phw). const RECORD_RENAMED_AWAY: &str = "the row you were editing has changed; Esc to leave the editor"; /// `CommitAndAdvance` over a coordinate: rename the record the editor OPENED on, @@ -966,12 +1002,8 @@ fn rename_coordinate( state: &ClientState, edit_mode: AppMode, advance: Advance, - id: &CellId, text: &str, ) -> Option> { - let CellId::Coordinate { .. } = id else { - return None; - }; // Untouched writes nothing (improvise-ixxx); empty is refused as the host // refuses it, where composing the write would undo the whole frame. let unchanged = untouched(state.view.opened_editor.as_ref(), text); @@ -980,8 +1012,8 @@ fn rename_coordinate( (false, true, _) => vec![Command::SetStatus( RECORD_COORDS_CANNOT_BE_EMPTY.to_string(), )], - (false, false, None | Some(Opened::Retry(_))) => return Some(still_waiting()), - (false, false, Some(Opened::Gone(_))) => { + (false, false, None | Some(Opened::Retry(..))) => return Some(still_waiting()), + (false, false, Some(Opened::Gone(..))) => { return Some(vec![Command::SetStatus(RECORD_RENAMED_AWAY.to_string())]); } (false, false, Some(Opened::Target(opened))) => { @@ -1013,15 +1045,9 @@ fn claimed(opened: &OpenedCell) -> impl Iterator Option> { - if untouched(state.view.opened_editor.as_ref(), text) { - return Some(Vec::new()); - } - let Some(Opened::Target(opened)) = state.opened.as_ref() else { - return None; - }; +/// The write to the value cell the editor OPENED on, at the version it read +/// then (improvise-8xq0, improvise-xz2z). +fn written(state: &ClientState, opened: &OpenedCell, text: &str) -> Option> { let OpenedTarget::Value { measure } = &opened.target else { return None; }; diff --git a/crates/improvise-wasm-client/tests/browser_round_trip.rs b/crates/improvise-wasm-client/tests/browser_round_trip.rs index 73a6fe42..40a080ab 100644 --- a/crates/improvise-wasm-client/tests/browser_round_trip.rs +++ b/crates/improvise-wasm-client/tests/browser_round_trip.rs @@ -1319,13 +1319,41 @@ fn a_second_rename_after_a_reorder_renames_the_row_the_host_shows() { enum Meanwhile { HideNorth, WriteNorth, + HideBoth, + HideRevenue, +} + +/// The commands B sends for `meanwhile`, in order. +fn meanwhile_commands( + host: &Host, + a: &Client, + meanwhile: &Meanwhile, +) -> Vec { + use improvise_protocol::ModelCommand; + let hide = |item: &str| ModelCommand::HideItem { + view: host.app().view_name().to_string(), + category: "Region".into(), + item: item.into(), + }; + match meanwhile { + Meanwhile::HideNorth => vec![hide("North")], + Meanwhile::HideBoth => vec![hide("North"), hide("South")], + Meanwhile::HideRevenue => vec![ModelCommand::HideItem { + view: host.app().view_name().to_string(), + category: "_Measure".into(), + item: "Revenue".into(), + }], + Meanwhile::WriteNorth => vec![ModelCommand::SetCell { + cell: a.record_cell_at(0, 0).expect("North/Revenue is stored"), + value: CellValue::Number(999.0), + }], + } } /// Client A presses `i` on North/Cost (0,1) and types `42`; B does `meanwhile` /// before A presses Enter. Evidence file. fn commit_north_cost(meanwhile: Meanwhile) -> (Host, Vec) { use improvise_command::ui::reduce::caught_up; - use improvise_protocol::ModelCommand; let mut host = host(); let (mut a, mut b) = (Client::new(), Client::new()); @@ -1348,21 +1376,17 @@ fn commit_north_cost(meanwhile: Meanwhile) -> (Host, Vec) { "setup: the editor is open" ); - let view = host.app().view_name().to_string(); - let cmd = match meanwhile { - Meanwhile::HideNorth => ModelCommand::HideItem { - view, - category: "Region".into(), - item: "North".into(), - }, - Meanwhile::WriteNorth => ModelCommand::SetCell { - cell: a.record_cell_at(0, 0).expect("North/Revenue is stored"), - value: CellValue::Number(999.0), - }, - }; - let relayed = host.handle(b_id, ClientFrame::command(caught_up(host.app(), cmd))); - route(&relayed, (a_id, &mut a), (b_id, &mut b)); + for cmd in meanwhile_commands(&host, &a, &meanwhile) { + let relayed = host.handle(b_id, ClientFrame::command(caught_up(host.app(), cmd))); + route(&relayed, (a_id, &mut a), (b_id, &mut b)); + } assert!(a.state().view.mode.is_editing(), "setup: still editing"); + if meanwhile == Meanwhile::HideBoth { + assert!( + a.state().grid.cell(0, 0).is_none(), + "setup: no cell is left under the cursor" + ); + } if meanwhile == Meanwhile::HideNorth { assert_eq!( a.state().grid.row_frame.leaf_labels().first().cloned(), @@ -1460,6 +1484,139 @@ fn a_keyboard_edit_is_refused_when_its_record_was_written_while_open() { assert_eq!(cell(&host, "North", "Cost"), None, "nothing was written"); } +/// A's Enter on North/Cost CROSSES B's `meanwhile`: B's relays reach A only +/// after A has sent. Then A presses Enter again. Evidence file. +fn cross_north_cost(meanwhile: Meanwhile) -> (Host, Vec, Client) { + cross_north(&["l", "i", "4", "2"], meanwhile) +} + +/// `cross_north_cost`, with the keys that open the editor and type into it. +fn cross_north(keys: &[&str], meanwhile: Meanwhile) -> (Host, Vec, Client) { + use improvise_command::ui::reduce::caught_up; + + let mut host = host(); + 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 keys { + press(&mut host, a_id, &mut a, key); + } + let mut held = Vec::new(); + for cmd in meanwhile_commands(&host, &a, &meanwhile) { + held.extend(host.handle(b_id, ClientFrame::command(caught_up(host.app(), cmd)))); + } + let first = a + .key(&BrowserKey::new("Enter")) + .expect("a commit composes a frame"); + let json = serde_json::to_string(&first).expect("a client frame serializes"); + let answered = send(&mut host, a_id, &json); + assert!( + answered + .iter() + .any(|f| matches!(f.msg, improvise_protocol::ServerFrame::Rejected(_))), + "setup: the crossing Enter is refused: {answered:?}" + ); + route(&held, (a_id, &mut a), (b_id, &mut b)); + route(&answered, (a_id, &mut a), (b_id, &mut b)); + assert!( + a.state().view.mode.is_editing(), + "setup: the refusal reopens the editor" + ); + + let retry = a.key(&BrowserKey::new("Enter")); + let answers = match retry { + Some(frame) => { + let json = serde_json::to_string(&frame).expect("a client frame serializes"); + send(&mut host, a_id, &json) + } + None => Vec::new(), + }; + (host, answers, a) +} + +/// improvise-8phw: the crossing refusal reopened the editor over South, and the +/// retry wrote 42 into South/Cost. Evidence: `context/test-evidence/clients.md`. +#[test] +fn a_keyboard_edit_retried_after_a_crossing_hide_writes_no_other_cell() { + let (host, answers, a) = cross_north_cost(Meanwhile::HideNorth); + assert_eq!(cell(&host, "South", "Cost"), None, "South/Cost was written"); + assert_eq!( + cell(&host, "South", "Revenue"), + None, + "South/Revenue was written" + ); + assert!( + !answers + .iter() + .any(|f| matches!(f.msg, improvise_protocol::ServerFrame::Accepted)), + "the retry must send no write: {answers:?}" + ); + assert!( + a.state().view.mode.is_editing(), + "the retry declines and leaves the editor open" + ); + assert!( + a.state().view.status_msg.contains("has changed"), + "and says so, rather than going silently upstream: {:?}", + a.state().view.status_msg + ); +} + +/// improvise-8phw, the measure half: the crossing hide leaves the SAME record +/// under the cursor in another column. Evidence: `context/test-evidence/clients.md`. +#[test] +fn a_keyboard_edit_retried_after_a_crossing_column_hide_writes_no_other_measure() { + let (host, answers, _) = cross_north(&["i", "4", "2"], Meanwhile::HideRevenue); + assert_eq!(cell(&host, "North", "Cost"), None, "North/Cost was written"); + assert_eq!( + cell(&host, "North", "Revenue"), + Some(CellValue::Number(1200.0)), + "the opened cell is hidden, so nothing may land on it either" + ); + assert!( + !answers + .iter() + .any(|f| matches!(f.msg, improvise_protocol::ServerFrame::Accepted)), + "the retry must send no write: {answers:?}" + ); +} + +/// improvise-8phw's control: a crossing WRITE to the record is refused, and the +/// retry is the one-keystroke force onto the same cell (gotcha 43). +#[test] +fn a_keyboard_edit_retried_after_a_crossing_write_forces_the_same_cell() { + let (host, answers, _) = cross_north_cost(Meanwhile::WriteNorth); + assert!( + answers + .iter() + .any(|f| matches!(f.msg, improvise_protocol::ServerFrame::Accepted)), + "the retry is a force: {answers:?}" + ); + assert_eq!(cell(&host, "North", "Cost"), Some(CellValue::Number(42.0))); + assert_eq!( + cell(&host, "North", "Revenue"), + Some(CellValue::Number(999.0)) + ); +} + +/// Codex on PR #274: with no cell left under the cursor, Enter went upstream +/// instead of writing the cell the editor opened on. Evidence file. +#[test] +fn a_keyboard_edit_whose_rows_all_vanished_writes_the_cell_it_opened_on() { + let (host, answers) = commit_north_cost(Meanwhile::HideBoth); + assert!( + answers + .iter() + .any(|f| matches!(f.msg, improvise_protocol::ServerFrame::Accepted)), + "the write must be sent and accepted: {answers:?}" + ); + assert_eq!(cell(&host, "North", "Cost"), Some(CellValue::Number(42.0))); +} + // ── races on a coordinate rename (improvise-20ek, improvise-milr) ─────── type Sid = improvise_command::ui::project::SessionId; From 0458b435199a947a913cef885209663a411b3b58 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 20:04:29 -0700 Subject: [PATCH 3/4] improvise-3h9w: red test, a captured edit is refused for the cursor's aggregate Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- .../tests/browser_round_trip.rs | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/crates/improvise-wasm-client/tests/browser_round_trip.rs b/crates/improvise-wasm-client/tests/browser_round_trip.rs index 40a080ab..e8db507b 100644 --- a/crates/improvise-wasm-client/tests/browser_round_trip.rs +++ b/crates/improvise-wasm-client/tests/browser_round_trip.rs @@ -1925,3 +1925,76 @@ fn a_retry_after_a_crossing_write_to_its_own_record_is_a_force() { ); assert_eq!(region(&host, "North"), None); } + +/// `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 + ); +} From 3dad3b0954b3099934b6fa9c79f2e915854d05b2 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 20:10:36 -0700 Subject: [PATCH 4/4] improvise-3h9w: a captured commit is not refused for the cursor's cell refused_here (from #286) ran before commit() consulted the capture and, for the commit keys, judged the cell under the cursor. After a reframe left the cursor on an aggregate, a captured editor's Enter was declined on every retry. The commit-key guard now applies only to an editor that captured nothing. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/clients.md | 8 ++++++++ context/test-evidence/clients.md | 22 ++++++++++++++++++++++ crates/improvise-protocol/src/resolve.rs | 3 ++- 3 files changed, 32 insertions(+), 1 deletion(-) diff --git a/context/code/clients.md b/context/code/clients.md index 067333ac..7c6d971c 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 e4428803..03a21b6d 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -5708,3 +5708,25 @@ captured cell stick. On the merged tree the rival that reads the capture through the cursor's offsets (`opened_at_cursor` through `row_offset`/`col_offset` rather than `cell_at_cursor`) fails exactly that case, with `["Jan/Revenue"]` against `["Feb/Revenue"]`. + +### 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 3637bb35..6f5f9697 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -325,11 +325,12 @@ fn step(state: &ClientState, event: KeyEvent, name: &str, args: &[String]) -> St } /// 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,