Merge pull request #219 from fiddlerwoaroof/fix/vndi-edit-generation

improvise-vndi: an inline edit opened before a document swap is refused

context/code/clients.md

@@ -1472,14 +1472,24 @@ so another client's write to *that same record* bumped the counter in the
 client's `BasisCache`. The commit then claimed the fresh counter and was
 accepted, and the other write was silently overwritten. If the other client
 cleared the cell instead, the commit re-created it. An `EditTarget` is
-`{ cell, version }`: the record's counter as the client had it when the editor
-opened. `BasisCache::claim_as_read` states that version beside the *current*
-document tiers.
+`{ document_generation, cell, version, opened }`: the document and the record's
+counter as the client had them when the editor opened, and the seed it opened
+on (improvise-ixxx, above). `BasisCache::claim_as_read`
+states both beside the *current* `layout_version`.
 
-The document tiers stay current on purpose. The rows renumbering under the
-editor, which was 2wu3's case, moves `layout_version`. Claiming that at open
-would refuse every such commit as `LayoutMoved`, although the identity already
-makes the commit safe. So the result is:
+**The two document tiers are not alike, and only the layout stays current.**
+The rows renumbering under the editor, which was 2wu3's case, moves
+`layout_version`. Claiming that at open would refuse every such commit as
+`LayoutMoved`, although the identity already makes the commit safe. The
+generation is the opposite case (improvise-vndi). A `RecordHash` is derived
+from coordinates, so after another client's `LoadModel` the same record can
+resolve in the incoming document, and a fresh `DataStore`'s counter can equal
+the one the editor read. Claiming the current generation then got the commit
+**accepted**: the loaded document's value, which this user never saw, was
+overwritten. Nothing composed against the outgoing document names anything in
+the incoming one, which is `Rejection::DocumentReplaced`'s own contract, so the
+generation is captured at open and the swap refuses the commit. So the result is:
+- the document was replaced: `DocumentReplaced`;
 - the record moved: `RecordMoved`;
 - the record is gone: `RecordGone`;
 - only the rows moved: the commit is accepted.
@@ -1491,10 +1501,11 @@ row. So the record resolves, and its counter has moved. `RecordGone` needs a
 record that neither the store nor the layout resolves.
 
 **`EditTarget` is the one object an editor captures at open, and it is a
-struct so it can grow.** improvise-ixxx needs the seed captured at open too (a
-commit whose buffer equals the seed writes nothing). That is a `seed: String`
-field beside `cell` and `version`, and the page does not change, because the
-target is opaque to it.
+struct so it can grow.** Two fields have joined `cell` and `version` since:
+the seed as `opened` (improvise-ixxx: a commit whose buffer equals the seed
+writes nothing) and the generation (improvise-vndi). The page did not change
+for either, because the target is opaque to it. `typed_target_frame` asks the
+two in that order: an untouched editor writes nothing, so it is never refused.
 
 `typed_cell_command` is the one composition shared by the mouse path
 (`typed_target_frame`) and the keyboard path (`keys::commit`). Writing it twice is

context/test-evidence/clients.md

@@ -5098,3 +5098,22 @@ on row 1. The test presses `i`, types `9`, and asserts the first editor really
 is touched. Enter must then land on (2, 0), and the folded `opened_editor` must
 be exactly `OpenedEditor::on("800")`. **Injection:** `resolve::advanced`
 carrying the previous editor's `touched` flag. This test fails alone.
+
+### an_edit_opened_before_a_document_swap_is_refused_in_the_new_document
+
+improvise-vndi, through a real `Host` over `MemoryFs`. A opens an inline editor
+on North/Revenue, which shows 1200. B sends `LoadModel` for a document that
+differs only in that cell, which holds 5555. Every name, the view included, is
+kept, per gotcha 39's fixture warning: a rename would relay a `SwitchView` and
+hide the bug. A folds the rebase `Snapshot` with the editor still open, as
+`web/app.js` does, and then commits "42". The `EditTarget` held `{cell,
+version}` and `claim_as_read` paired that version with the *current*
+generation. `RecordHash` is coordinate-derived, so the same record resolved
+in the new document at a matching counter, and the host **accepted** the
+commit. It failed at the `DocumentReplaced` assertion: the answer was
+`Accepted`, and 5555 had become 42.
+
+Rival injected: `claim_as_read` ignoring the captured generation and claiming
+the current one. That is the pre-fix behaviour, restated after the fix. Only
+this test failed in the whole workspace. The layout tier staying current is
+still pinned by `an_edit_writes_the_cell_the_user_clicked_after_the_rows_renumber`.

crates/improvise-protocol/src/basis.rs

@@ -94,15 +94,16 @@ impl BasisCache {
 }
 
 impl BasisCache {
-    /// A basis for records whose counters were read EARLIER, such as when an
-    /// editor opened (improvise-0j0f). The document tiers are current.
+    /// A basis for what was read EARLIER, such as when an editor opened: the
+    /// document and its records then, the layout now (improvise-0j0f, improvise-vndi).
     pub fn claim_as_read(
         &self,
+        document_generation: u64,
         read: impl IntoIterator<Item = (RecordHash, u64)>,
     ) -> Option<Basis> {
         let current = self.current.as_ref()?;
         Some(Basis {
-            document_generation: current.document_generation,
+            document_generation,
             layout_version: current.layout_version,
             records: read.into_iter().collect(),
         })

crates/improvise-wasm-client/src/socket.rs

@@ -12,6 +12,8 @@ use serde::{Deserialize, Serialize};
 /// A struct so a field such as the seed can join it — `context/code/clients.md`.
 #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
 pub struct EditTarget {
+    /// The document the editor opened on; a swap under it refuses the commit (improvise-vndi).
+    pub document_generation: u64,
     pub cell: RecordCell,
     /// The record's write counter as this client had it at open.
     pub version: u64,
@@ -117,9 +119,12 @@ impl Client {
     /// version of it the user is looking at. `None` before any basis arrives.
     pub fn edit_target_at(&self, row: usize, col: usize) -> Option<EditTarget> {
         let cell = self.record_cell_at(row, col)?;
-        let version = self.state().basis.current()?.record(cell.record)?;
+        let basis = self.state().basis.current()?;
+        let version = basis.record(cell.record)?;
+        let document_generation = basis.document_generation;
         let seed = self.state().grid.cell(row, col)?.edit_text.clone();
         Some(EditTarget {
+            document_generation,
             cell,
             version,
             opened: OpenedEditor::on(seed.unwrap_or_default()),
@@ -132,10 +137,10 @@ impl Client {
         if untouched(Some(&target.opened), text) {
             return None;
         }
-        let basis = self
-            .state()
-            .basis
-            .claim_as_read([(target.cell.record, target.version)])?;
+        let basis = self.state().basis.claim_as_read(
+            target.document_generation,
+            [(target.cell.record, target.version)],
+        )?;
         let cmds = vec![Command::Model {
             basis,
             cmd: typed_cell_command(&improvise_protocol::CellId::Record(target.cell), text)?,

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

@@ -817,6 +817,70 @@ fn an_edit_is_refused_when_the_cell_was_cleared_while_the_editor_was_open() {
     );
 }
 
+/// improvise-vndi: an editor opened before another client's `LoadModel` must not
+/// write into the loaded document. Evidence: test-evidence/clients.md.
+#[test]
+fn an_edit_opened_before_a_document_swap_is_refused_in_the_new_document() {
+    use improvise_command::ui::reduce::caught_up;
+    use improvise_protocol::{ModelCommand, Rejection, ServerFrame};
+
+    let north = CellKey::new(vec![
+        ("Region".into(), "North".into()),
+        ("_Measure".into(), "Revenue".into()),
+    ]);
+    // The incoming document keeps every name, view included (gotcha 39's
+    // fixture warning), and differs only in the one value the edit targets.
+    let path = std::path::PathBuf::from("other.improv");
+    let mut host = host();
+    host.app_mut().storage = Box::new(improvise_command::storage::MemoryFs::new());
+    let mut other = host.app().model_state.workbook.clone();
+    other
+        .model
+        .set_cell(north.clone(), CellValue::Number(5555.0));
+    improvise_command::persistence::save_to(&*host.app().storage, &other, &path).unwrap();
+
+    let mut editor = Client::new();
+    let (a, snapshot) = host.connect(10, 10);
+    deliver(&mut editor, &[snapshot]);
+    let (b, _) = host.connect(10, 10);
+
+    // A opens an editor on North/Revenue, which shows 1200.
+    let target = editor
+        .edit_target(0, 0)
+        .expect("serialization must not fail")
+        .expect("a stored cell has an edit target");
+
+    // B loads the other document; A folds its rebase Snapshot, editor still open.
+    let generation = host.app().document_generation();
+    let load = caught_up(host.app(), ModelCommand::LoadModel(path));
+    let relayed = host.handle(b, ClientFrame::command(load));
+    assert_eq!(
+        host.app().document_generation(),
+        generation + 1,
+        "setup: the load must really replace the document"
+    );
+    let for_a: Vec<Addressed> = relayed.into_iter().filter(|f| f.to == a).collect();
+    deliver(&mut editor, &for_a);
+
+    let commit = editor
+        .typed_cell(&target, "42")
+        .expect("serialization must not fail")
+        .expect("a stored cell composes a write");
+    let answer = send(&mut host, a, &commit);
+
+    assert!(
+        answer
+            .iter()
+            .any(|f| matches!(f.msg, ServerFrame::Rejected(Rejection::DocumentReplaced))),
+        "the edit was composed against the outgoing document: {answer:?}"
+    );
+    assert_eq!(
+        host.app().model_state.workbook.model.get_cell(&north),
+        Some(&CellValue::Number(5555.0)),
+        "the loaded document's value was overwritten by an edit its user never saw"
+    );
+}
+
 // ── numbers across the serialized wire (improvise-6oy4) ─────────────────
 
 /// A value serde_json's default f64 parser reads back one ULP low. Evidence file.