Merge #251 (3f5f90a5, improvise-20ek/milr rename races) into the editor integration branch

Resolutions:
- client.rs imports: EditorSession (main) and ModelCommand (#251).
- client.rs refused(): #251 keeps the popped frame to learn which record it
  renamed; main (#279/3kfc) calls settle_sent() after the pop. Both kept, in
  the order pop, settle_sent, replay, then #251's Opened::Retry.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze

crates/improvise-command/src/ui/reduce.rs

@@ -282,16 +282,43 @@ pub fn stale_basis(app: &App, basis: &Basis, cmd: &ModelCommand) -> Option<Rejec
     let layout_moved =
         (basis.layout_version != app.layout_version()).then_some(Rejection::LayoutMoved);
     document_moved.or(layout_moved).or_else(|| {
-        records_named(cmd)
+        records_at_stake(app, cmd)
             .into_iter()
             .find_map(|h| stale_record(app, basis, h))
     })
 }
 
+/// [`records_named`], plus what only the document can name: a rename's
+/// destination, when it is a stored record (improvise-milr, `command.md`).
+pub fn records_at_stake(app: &App, cmd: &ModelCommand) -> Vec<RecordHash> {
+    let mut records = records_named(cmd);
+    if let ModelCommand::RenameRecordCoordinate {
+        record,
+        category,
+        item,
+    } = cmd
+    {
+        let destination = app
+            .resolve_record(*record)
+            .map(|source| RecordHash::of(&source.with(category, item)))
+            .filter(|dest| dest != record)
+            .filter(|dest| {
+                app.model_state
+                    .workbook
+                    .model
+                    .data
+                    .resolve_record(*dest)
+                    .is_some()
+            });
+        records.extend(destination);
+    }
+    records
+}
+
 /// The record tier for a command inside a frame whose document tiers are
 /// already answered, skipping the records the frame is about to create.
 fn stale_records_in_a_frame(app: &App, basis: &Basis, cmd: &ModelCommand) -> Option<Rejection> {
-    records_named(cmd)
+    records_at_stake(app, cmd)
         .into_iter()
         .filter(|hash| !will_be_created_by_the_frame(app, basis, *hash))
         .find_map(|hash| stale_record(app, basis, hash))
@@ -320,7 +347,7 @@ pub fn caught_up(app: &App, cmd: ModelCommand) -> Command {
         basis: Basis {
             document_generation: app.document_generation(),
             layout_version: app.layout_version(),
-            records: records_named(&cmd)
+            records: records_at_stake(app, &cmd)
                 .into_iter()
                 .map(|h| (h, app.record_version_of(h)))
                 .collect(),

crates/improvise-protocol/src/client.rs

@@ -3,8 +3,8 @@
 
 use crate::{
     BasisCache, CategoryTreeCache, Command, DocumentCache, EditorSession, FormulaCache, GridCache,
-    Projection, Rejection, Snapshot, TileBarCache, ViewListCache, ViewState, WidgetCache,
-    reduce_view, settled_mode,
+    ModelCommand, Projection, Rejection, Snapshot, TileBarCache, ViewListCache, ViewState,
+    WidgetCache, reduce_view, settled_mode,
 };
 use improvise_core::model::record::RecordHash;
 use serde::Serialize;
@@ -53,16 +53,26 @@ pub struct ClientState {
     /// The coordinate an open editor renames, captured as the user saw it.
     /// When it is taken, and when not yet: `context/code/clients.md`.
     #[serde(skip)]
-    pub opened: Option<OpenedCoordinate>,
+    pub opened: Option<Opened>,
 }
 
-/// A coordinate editor's target: the record, the category, and the record's
-/// write counter when the editor opened (improvise-fdk1, improvise-0j0f).
+/// What an open coordinate editor renames. After a refusal the retry may name
+/// only the record it named (`Retry`) or none (`Gone`): improvise-20ek, -9ww8.
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub enum Opened {
+    Target(OpenedCoordinate),
+    Retry(RecordHash),
+    Gone(RecordHash),
+}
+
+/// A coordinate editor's target, and every record counter this client held when
+/// it opened: the destination is among them if shown (improvise-fdk1, -milr).
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub struct OpenedCoordinate {
     pub record: RecordHash,
     pub category: String,
     pub version: u64,
+    pub seen: Vec<(RecordHash, u64)>,
     /// The document the editor opened on, so a swap refuses it (improvise-vndi).
     pub document_generation: u64,
 }
@@ -203,9 +213,10 @@ impl ClientState {
         // A refused *save* answers no `Commands` frame, so it must not consume
         // one's entry — `Rejection::refused_an_edit` is that question.
         if rejection.refused_an_edit() {
-            self.in_flight.pop_front();
+            let dropped = self.in_flight.pop_front();
             self.settle_sent();
             self.replay();
+            self.opened = renamed(dropped.as_ref()).map(Opened::Retry);
         }
         self.last_rejection = Some(rejection);
         self.settle_mode();
@@ -260,22 +271,29 @@ impl ClientState {
         let answered = !self.in_flight.iter().any(|sent| sent.awaited);
         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)) => Some(opened),
-            (true, None) if answered => {
-                crate::resolve::coordinate_at_cursor(self).and_then(|(record, category)| {
-                    let basis = self.basis.current()?;
-                    Some(OpenedCoordinate {
-                        record,
-                        category,
-                        version: basis.record(record)?,
-                        document_generation: basis.document_generation,
-                    })
-                })
-            }
+            (true, None) if answered => self.captured().map(Opened::Target),
             (true, None) => None,
         };
     }
 
+    /// The coordinate under the cursor, with every counter this client holds.
+    fn captured(&self) -> Option<OpenedCoordinate> {
+        let (record, category) = crate::resolve::coordinate_at_cursor(self)?;
+        let basis = self.basis.current()?;
+        Some(OpenedCoordinate {
+            record,
+            category,
+            version: basis.record(record)?,
+            seen: basis.records.clone(),
+            document_generation: basis.document_generation,
+        })
+    }
+
     /// An editor that arrived without its buffer is seeded here, so what is
     /// drawn and what Enter commits read one buffer (improvise-ktlk).
     fn settle_editor(&mut self) {
@@ -354,6 +372,17 @@ impl ClientState {
     }
 }
 
+/// The record a refused frame renamed, if it renamed one (improvise-9ww8).
+fn renamed(sent: Option<&Sent>) -> Option<RecordHash> {
+    sent?.cmds.iter().find_map(|cmd| match cmd {
+        Command::Model {
+            cmd: ModelCommand::RenameRecordCoordinate { record, .. },
+            ..
+        } => Some(*record),
+        _ => None,
+    })
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

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, Opened, OpenedCoordinate};
 use crate::frame::ClientFrame;
 use crate::keymap::{Binding, Keymaps, ModeKey};
 use crate::viewmodel::{CatTreeEntry, axis_change_status, flatten_cat_tree};
@@ -1052,6 +1052,10 @@ fn commit(state: &ClientState, edit_mode: AppMode, advance: Advance) -> Option<V
     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).
+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,
 /// at the version it was shown — `context/code/clients.md` (improvise-fdk1).
 fn rename_coordinate(
@@ -1072,16 +1076,18 @@ fn rename_coordinate(
         (false, true, _) => vec![Command::SetStatus(
             RECORD_COORDS_CANNOT_BE_EMPTY.to_string(),
         )],
-        (false, false, None) => {
+        (false, false, None | Some(Opened::Retry(_))) => {
             return Some(vec![Command::SetStatus(
                 "still waiting for the host to answer your last edit".to_string(),
             )]);
         }
-        (false, false, Some(opened)) => vec![Command::Model {
-            basis: state.basis.claim_as_read(
-                opened.document_generation,
-                [(opened.record, opened.version)],
-            )?,
+        (false, false, Some(Opened::Gone(_))) => {
+            return Some(vec![Command::SetStatus(RECORD_RENAMED_AWAY.to_string())]);
+        }
+        (false, false, Some(Opened::Target(opened))) => vec![Command::Model {
+            basis: state
+                .basis
+                .claim_as_read(opened.document_generation, claimed(opened))?,
             cmd: ModelCommand::RenameRecordCoordinate {
                 record: opened.record,
                 category: opened.category.clone(),
@@ -1096,6 +1102,14 @@ fn rename_coordinate(
     Some(commands)
 }
 
+/// The rename's claim: its own record, then every record shown when the editor
+/// opened, so the destination is claimed if seen (improvise-milr).
+fn claimed(opened: &OpenedCoordinate) -> impl Iterator<Item = (crate::RecordHash, u64)> + '_ {
+    let others = opened.seen.iter().copied();
+    std::iter::once((opened.record, opened.version))
+        .chain(others.filter(|(h, _)| *h != opened.record))
+}
+
 /// The commit's own clear of `edit_mode`'s buffer, at its head: the keymap
 /// welds none to these keys, so a re-seed survives (improvise-fy9o).
 fn cleared(edit_mode: &AppMode) -> Option<Command> {

crates/improvise-wasm-client/tests/browser_round_trip.rs

@@ -1353,3 +1353,312 @@ fn a_second_rename_after_a_reorder_renames_the_row_the_host_shows() {
         "West untouched"
     );
 }
+
+// ── races on a coordinate rename (improvise-20ek, improvise-milr) ───────
+
+type Sid = improvise_command::ui::project::SessionId;
+
+/// A client driven into records mode through the tile bar, its cursor on the
+/// coordinate column of row 0. Returns the session, the client and the column.
+fn records_client_at_coordinate(host: &mut Host) -> (Sid, Client, usize) {
+    let mut a = Client::new();
+    a.set_viewport(10, 10);
+    let (id, snapshot) = host.connect(10, 10);
+    deliver(&mut a, &[snapshot]);
+    press(host, id, &mut a, "T");
+    tile_to(host, id, &mut a, "_Index", "r");
+    tile_to(host, id, &mut a, "_Dim", "c");
+    press(host, id, &mut a, "Escape");
+    assert!(a.state().grid.records, "setup: records mode");
+    let coordinate = (0..a.state().grid.extent.cols)
+        .find(|&c| {
+            format!("{:?}", a.state().grid.cell(0, c).and_then(|x| x.id.clone()))
+                .starts_with("Some(Coordinate")
+        })
+        .expect("a coordinate column");
+    while a.state().view.cursor().selected.1 < coordinate {
+        press(host, id, &mut a, "l");
+    }
+    (id, a, coordinate)
+}
+
+fn retype(host: &mut Host, id: Sid, a: &mut Client, text: &str) {
+    for _ in 0..8 {
+        press(host, id, a, "Backspace");
+    }
+    for ch in text.chars() {
+        press(host, id, a, &ch.to_string());
+    }
+}
+
+/// Another session's frame, applied, with what it owes `a` delivered to `a`.
+fn from_another_client(host: &mut Host, a_id: Sid, a: &mut Client, cmd: ModelCommandArg) {
+    let (b_id, _) = host.connect(10, 10);
+    let frame = improvise_command::ui::reduce::caught_up(host.app(), cmd);
+    let relayed = host.handle(b_id, ClientFrame::command(frame));
+    assert!(
+        relayed
+            .iter()
+            .any(|f| f.to == b_id && matches!(f.msg, improvise_protocol::ServerFrame::Accepted)),
+        "the other client's write must land: {relayed:?}"
+    );
+    let for_a: Vec<Addressed> = relayed.into_iter().filter(|f| f.to == a_id).collect();
+    deliver(a, &for_a);
+}
+
+type ModelCommandArg = improvise_protocol::ModelCommand;
+
+/// improvise-20ek: the record an editor opened on is gone, and a retry must not
+/// rename whatever moved under the cursor. Evidence: `context/test-evidence/clients.md`.
+#[test]
+fn a_retry_after_record_gone_renames_no_other_record() {
+    use improvise_protocol::{CellId, ModelCommand, Rejection, ServerFrame};
+    let mut host = three_records_host();
+    let (a_id, mut a, coordinate) = records_client_at_coordinate(&mut host);
+    assert_eq!(a.state().grid.cell(0, coordinate).unwrap().text, "North");
+    let Some(CellId::Coordinate { record: north, .. }) =
+        a.state().grid.cell(0, coordinate).unwrap().id.clone()
+    else {
+        panic!("setup: a coordinate id")
+    };
+    press(&mut host, a_id, &mut a, "i");
+    retype(&mut host, a_id, &mut a, "Zed");
+
+    from_another_client(
+        &mut host,
+        a_id,
+        &mut a,
+        ModelCommand::RenameRecordCoordinate {
+            record: north,
+            category: "Region".into(),
+            item: "Yak".into(),
+        },
+    );
+
+    let frame = a.key(&BrowserKey::new("Enter")).expect("Enter composes");
+    let answer = send(&mut host, a_id, &serde_json::to_string(&frame).unwrap());
+    deliver(&mut a, &answer);
+    assert!(
+        answer
+            .iter()
+            .any(|f| matches!(f.msg, ServerFrame::Rejected(Rejection::RecordGone(_)))),
+        "the first Enter is RecordGone: {answer:?}"
+    );
+    let row = a.state().view.cursor().selected.0;
+    assert!(
+        a.state().view.mode.is_editing()
+            && a.state().grid.cell(row, coordinate).unwrap().text != "Yak",
+        "setup: the editor is back, over a record other than the one renamed away"
+    );
+
+    press(&mut host, a_id, &mut a, "Enter");
+    assert_eq!(
+        region(&host, "Zed"),
+        None,
+        "the retry renamed a record never opened"
+    );
+    assert_eq!(region(&host, "South"), Some(CellValue::Number(2.0)));
+    assert_eq!(region(&host, "West"), Some(CellValue::Number(3.0)));
+    assert_eq!(region(&host, "Yak"), Some(CellValue::Number(1.0)));
+}
+
+/// improvise-milr: the rename's destination is a record too, and a write to it
+/// this client never saw must refuse the rename. Evidence file.
+#[test]
+fn a_rename_onto_a_record_another_client_just_wrote_is_refused() {
+    use improvise_protocol::{ModelCommand, Rejection, ServerFrame};
+    let mut host = three_records_host();
+    let (a_id, mut a, coordinate) = records_client_at_coordinate(&mut host);
+    let revenue = (0..a.state().grid.extent.cols)
+        .find(|&c| {
+            a.record_cell_at(1, c)
+                .is_some_and(|x| x.measure.as_deref() == Some("Revenue"))
+        })
+        .expect("South's revenue");
+    let south = a.record_cell_at(1, revenue).unwrap();
+    assert_eq!(a.state().grid.cell(1, coordinate).unwrap().text, "South");
+    press(&mut host, a_id, &mut a, "i");
+    retype(&mut host, a_id, &mut a, "South");
+
+    from_another_client(
+        &mut host,
+        a_id,
+        &mut a,
+        ModelCommand::SetCell {
+            cell: south.clone(),
+            value: CellValue::Number(999.0),
+        },
+    );
+
+    let frame = a.key(&BrowserKey::new("Enter")).expect("Enter composes");
+    let answer = send(&mut host, a_id, &serde_json::to_string(&frame).unwrap());
+    assert!(
+        answer.iter().any(|f| matches!(
+            f.msg,
+            ServerFrame::Rejected(Rejection::RecordMoved(h)) if h == south.record
+        )),
+        "the destination moved under this client: {answer:?}"
+    );
+    assert_eq!(region(&host, "South"), Some(CellValue::Number(999.0)));
+    assert_eq!(region(&host, "North"), Some(CellValue::Number(1.0)));
+}
+
+/// The control for the test above: a destination this client HAS seen at its
+/// current version is overwritten, which is improvise-kndq's policy, unchanged.
+#[test]
+fn a_rename_onto_a_record_this_client_has_seen_still_overwrites_it() {
+    let mut host = three_records_host();
+    let (a_id, mut a, _) = records_client_at_coordinate(&mut host);
+    press(&mut host, a_id, &mut a, "i");
+    retype(&mut host, a_id, &mut a, "South");
+    let frame = a.key(&BrowserKey::new("Enter")).expect("Enter composes");
+    let answer = send(&mut host, a_id, &serde_json::to_string(&frame).unwrap());
+    assert!(
+        answer
+            .iter()
+            .any(|f| matches!(f.msg, improvise_protocol::ServerFrame::Accepted)),
+        "{answer:?}"
+    );
+    assert_eq!(
+        region(&host, "South"),
+        Some(CellValue::Number(1.0)),
+        "North's value"
+    );
+    assert_eq!(region(&host, "North"), None);
+}
+
+/// Another session's frame, applied, with what it owes `a` HELD rather than
+/// delivered: `a` has not heard it yet (improvise-9ww8).
+fn held_from_another_client(host: &mut Host, a_id: Sid, cmd: ModelCommandArg) -> Vec<Addressed> {
+    let (b_id, _) = host.connect(10, 10);
+    let frame = improvise_command::ui::reduce::caught_up(host.app(), cmd);
+    let relayed = host.handle(b_id, ClientFrame::command(frame));
+    assert!(
+        relayed
+            .iter()
+            .any(|f| f.to == b_id && matches!(f.msg, improvise_protocol::ServerFrame::Accepted)),
+        "the other client's write must land: {relayed:?}"
+    );
+    relayed.into_iter().filter(|f| f.to == a_id).collect()
+}
+
+/// A's rename crosses B's rename of North in flight: A composes Enter before
+/// hearing B, then hears B's relays and its own refusal, in that order.
+fn crossing_rename(row: usize) -> Host {
+    use improvise_protocol::{CellId, ModelCommand, Rejection, ServerFrame};
+    let mut host = three_records_host();
+    let (a_id, mut a, coordinate) = records_client_at_coordinate(&mut host);
+    let Some(CellId::Coordinate { record: north, .. }) =
+        a.state().grid.cell(0, coordinate).unwrap().id.clone()
+    else {
+        panic!("setup: North's coordinate id")
+    };
+    for _ in 0..row {
+        press(&mut host, a_id, &mut a, "j");
+    }
+    press(&mut host, a_id, &mut a, "i");
+    retype(&mut host, a_id, &mut a, "Zed");
+
+    let held = held_from_another_client(
+        &mut host,
+        a_id,
+        ModelCommand::RenameRecordCoordinate {
+            record: north,
+            category: "Region".into(),
+            item: "Yak".into(),
+        },
+    );
+    let frame = a.key(&BrowserKey::new("Enter")).expect("Enter composes");
+    let answer = send(&mut host, a_id, &serde_json::to_string(&frame).unwrap());
+    deliver(&mut a, &held);
+    deliver(&mut a, &answer);
+    assert!(
+        answer
+            .iter()
+            .any(|f| matches!(f.msg, ServerFrame::Rejected(Rejection::LayoutMoved))),
+        "setup: the crossing is refused on the layout tier, before any record: {answer:?}"
+    );
+    assert!(
+        a.state().view.mode.is_editing(),
+        "setup: the editor is back"
+    );
+
+    press(&mut host, a_id, &mut a, "Enter");
+    host
+}
+
+/// improvise-9ww8, the source gone: B renamed the very record A opened on.
+/// Evidence: `context/test-evidence/clients.md`.
+#[test]
+fn a_retry_after_a_crossing_rename_of_its_record_renames_no_other_record() {
+    let host = crossing_rename(0);
+    assert_eq!(
+        region(&host, "Zed"),
+        None,
+        "the retry renamed a record never opened"
+    );
+    assert_eq!(region(&host, "Yak"), Some(CellValue::Number(1.0)));
+    assert_eq!(region(&host, "South"), Some(CellValue::Number(2.0)));
+    assert_eq!(region(&host, "West"), Some(CellValue::Number(3.0)));
+}
+
+/// improvise-9ww8, the source alive: B's rename only moves A's record off the
+/// row under the cursor. Evidence: `context/test-evidence/clients.md`.
+#[test]
+fn a_retry_after_a_crossing_reorder_renames_no_other_record() {
+    let host = crossing_rename(1);
+    assert_eq!(
+        region(&host, "West"),
+        Some(CellValue::Number(3.0)),
+        "the retry renamed West, the record that moved under the cursor"
+    );
+    assert_ne!(region(&host, "Zed"), Some(CellValue::Number(3.0)));
+    assert_eq!(region(&host, "Yak"), Some(CellValue::Number(1.0)));
+}
+
+/// The control for the two crossings above: `RecordMoved` on the SAME record is
+/// still a one-keystroke force. Evidence: `context/test-evidence/clients.md`.
+#[test]
+fn a_retry_after_a_crossing_write_to_its_own_record_is_a_force() {
+    use improvise_protocol::{ModelCommand, Rejection, ServerFrame};
+    let mut host = three_records_host();
+    let (a_id, mut a, coordinate) = records_client_at_coordinate(&mut host);
+    assert_eq!(a.state().grid.cell(0, coordinate).unwrap().text, "North");
+    let revenue = (0..a.state().grid.extent.cols)
+        .find(|&c| {
+            a.record_cell_at(0, c)
+                .is_some_and(|x| x.measure.as_deref() == Some("Revenue"))
+        })
+        .expect("North's revenue");
+    let north = a.record_cell_at(0, revenue).unwrap();
+    press(&mut host, a_id, &mut a, "i");
+    retype(&mut host, a_id, &mut a, "Zed");
+
+    let held = held_from_another_client(
+        &mut host,
+        a_id,
+        ModelCommand::SetCell {
+            cell: north.clone(),
+            value: CellValue::Number(9.0),
+        },
+    );
+    let frame = a.key(&BrowserKey::new("Enter")).expect("Enter composes");
+    let answer = send(&mut host, a_id, &serde_json::to_string(&frame).unwrap());
+    deliver(&mut a, &held);
+    deliver(&mut a, &answer);
+    assert!(
+        answer.iter().any(|f| matches!(
+            f.msg,
+            ServerFrame::Rejected(Rejection::RecordMoved(h)) if h == north.record
+        )),
+        "setup: the crossing write moves the record A is renaming: {answer:?}"
+    );
+
+    press(&mut host, a_id, &mut a, "Enter");
+    assert_eq!(
+        region(&host, "Zed"),
+        Some(CellValue::Number(9.0)),
+        "the retry must rename North, carrying B's write"
+    );
+    assert_eq!(region(&host, "North"), None);
+}