Merge pull request #177 from fiddlerwoaroof/wire2/j4qt

A read-only tab refuses edits instead of painting what it cannot save (j4qt)

context/code/clients.md

@@ -124,6 +124,47 @@ So this is the browser's own answer, and `ClientFrame::Save` is the other one
 for a socket client editing a TUI-hosted document, where there *is* a file
 somebody owns.
 
+### A read-only session refuses edits; it does not accept them and fail to save (improvise-j4qt)
+
+A second tab on the same document cannot take the exclusive OPFS handle, so it
+boots **read-only** (improvise-i0v9). It reads the bytes, paints the grid, and
+navigates. Until improvise-j4qt, "read-only" described only the *storage*. The
+session was writable: an edit was applied to the tab's own in-memory `App` and
+projected, so the grid **painted a value the document did not have**
+(`18,400.00` became `184,004,242.00`). Only then did the automatic persist
+fail, with `NOT SAVED` replacing the read-only reason in the status. The
+first tab never saw the edit.
+
+The fix sets the session's access at boot: `Worker::read_only` versus
+`Worker::new`, as a private `Access`. A read-only worker answers any
+`ClientFrame::Commands` that contains a `Command::Model` with
+`Rejected(Rejection::ReadOnly)` **before the host sees it**. So nothing is
+applied, nothing is projected, and nothing is persisted. Three things make
+this the right place:
+
+- **It is decided up front, not observed afterwards.** The persist failure is
+  an observation, and it arrives *after* the paint. By the time a failed save
+  can be seen, the user has already been shown the number.
+- **The client's rollback is already correct for it** (improvise-nxah).
+  `Rejection::ReadOnly` answers `refused_an_edit() == true`, so the client
+  pops that frame's entry and replays `ViewState` from its settled slice. No
+  cache needs rolling back: `Command::Model` is a no-op in `reduce_view`, so
+  the optimistic fold never painted the value. The paint came from the host's
+  projection, and that projection is exactly what the refusal prevents.
+- **Only the document is refused.** A frame of view commands, such as a cursor
+  move, a panel or a help page, is answered as before. Read-only is a claim
+  about the document, not about this client's own slice.
+
+It is a property of the *boot*, not the backend. The backend in both cases is
+a `vfs::ReadOnly`, and `worker.rs`'s `a_persist_that_fails_raises_a_notice…`
+still uses one as a disk-full injection over a *writable* session. Reading
+access off the storage would have made those two indistinguishable.
+`ClientFrame::Save` is unchanged: a read-only tab that asks to save is still
+answered `SaveFailed` by the backend.
+
+What this does **not** do is i0v9's gap 2. There is still no upgrade path when
+the first tab closes, and the tab stays read-only until it is reloaded.
+
 ### Ordering
 
 `Worker::on_frame` returns the frames for one inbound frame, in order, and a

context/test-evidence/clients.md

@@ -4231,6 +4231,43 @@ and converged.
 **Fault injected:** the same `Binding::Prefix` → `Upstream` reversion. Fails on
 the second press.
 
+### `a_read_only_session_refuses_an_edit_before_anything_is_painted`
+
+improvise-j4qt. The finder measured this in Chromium and Firefox: two tabs, one
+document. The second tab boots read-only. Typing `4242` into a cell applied it
+to that tab's in-memory `App` and projected it, so the grid showed
+`184,004,242.00`. Only then did the persist fail and `NOT SAVED` replace the
+read-only reason. The first tab never saw the edit.
+
+The test builds the read-only boot the way `bindings::read_only` does: the
+document's bytes over a `vfs::ReadOnly` backend. It sends a caught-up `SetCell`
+and asserts four things:
+- the whole reply is `[Rejected(ReadOnly)]`;
+- the client's folded grid still shows the old text;
+- no notice was raised (no persist ran);
+- the document is not dirty.
+
+**Red first**: the same test over `Worker::new`, which was the boot's
+constructor before this change, failed. Its reply was a `Commands` frame of
+projections carrying the new value.
+
+The control is `a_read_only_session_still_answers_a_view_command`: a
+`SetSelected` frame is not refused. Without it, a worker that refused *every*
+frame would pass the test above.
+
+End to end: `harness/tests/shell.spec.js`, "a read-only tab refuses an edit and
+never paints it". It opens two real tabs on one document in Chromium, edits in
+the second, and asserts four things:
+- the toast says `read-only`;
+- the cell keeps its text;
+- the grid contains no `4,242`;
+- `#status` still says `read-only` and not `NOT SAVED`.
+
+Fault injected: `bindings.rs` reverted to `Worker::new(app)` for the read-only
+boot, then `harness/build.sh` rebuilt. The spec then failed on the toast, which
+received `""` because no refusal arrived. Reverted and rebuilt, and the whole
+of `shell.spec.js` passed (18).
+
 ### `an_open_empty_panel_is_not_a_closed_one`
 
 improvise-df2w. `views` and `formulas` said "closed" by being empty, so an open

crates/improvise-protocol/src/basis.rs

@@ -54,6 +54,9 @@ pub enum Rejection {
     /// A mutation in the frame refused to apply at all. The frame is undone;
     /// the refusal's own explanation is in the host's status line.
     MutationRefused,
+    /// This session cannot write the document, so it takes no edit at all
+    /// (improvise-j4qt). An edit it could not keep must never be painted.
+    ReadOnly,
 
     /// A [`ClientFrame::Save`](crate::ClientFrame::Save) reached a host with
     /// no path (improvise-368h). Decided before anything runs.
@@ -109,7 +112,8 @@ impl Rejection {
             | Self::RecordMoved(_)
             | Self::RecordUnstated(_)
             | Self::ChainBroken(_)
-            | Self::MutationRefused => true,
+            | Self::MutationRefused
+            | Self::ReadOnly => true,
             Self::NoDocumentPath | Self::SaveFailed => false,
         }
     }
@@ -136,7 +140,8 @@ impl Rejection {
             Self::RecordMoved(_) => Self::RecordUnstated(sample),
             Self::RecordUnstated(_) => Self::ChainBroken(sample),
             Self::ChainBroken(_) => Self::MutationRefused,
-            Self::MutationRefused => Self::NoDocumentPath,
+            Self::MutationRefused => Self::ReadOnly,
+            Self::ReadOnly => Self::NoDocumentPath,
             Self::NoDocumentPath => Self::SaveFailed,
             Self::SaveFailed => Self::DocumentReplaced,
         }

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

@@ -29,6 +29,10 @@ fn rejection_message(rejection: &Rejection) -> String {
         Rejection::MutationRefused => {
             "part of that batch was refused — the batch was not applied".into()
         }
+        Rejection::ReadOnly => {
+            "this tab is read-only: another tab has the document open — your edit was not applied"
+                .into()
+        }
         // Not staleness, so neither offers a retry: they are worded as facts
         // about the host, because that is what they are (improvise-368h).
         Rejection::NoDocumentPath => {

crates/improvise-worker-server/src/bindings.rs

@@ -111,7 +111,7 @@ async fn read_only(storage: &OpfsFs, path: PathBuf, why: String) -> Result<Worke
     let mut app = App::with_storage(opened.workbook, Some(path.clone()), Box::new(fs));
     app.rebuild_layout();
     Ok(WorkerServer {
-        worker: Worker::new(app),
+        worker: Worker::read_only(app),
         storage: None,
         notice: Some(Notice::new(
             NoticeKind::ReadOnly,

crates/improvise-worker-server/src/worker.rs

@@ -5,7 +5,7 @@ use improvise_command::ui::app::App;
 use improvise_command::ui::effect;
 use improvise_command::ui::host::{Addressed, Host};
 use improvise_command::ui::project::SessionId;
-use improvise_protocol::{ClientFrame, ServerFrame};
+use improvise_protocol::{ClientFrame, Command, Rejection, ServerFrame};
 
 use crate::boot::{Notice, NoticeKind};
 
@@ -18,16 +18,50 @@ pub struct Worker {
     /// Raised by a persist nobody asked for and drained by the shim; see
     /// [`Worker::take_notice`] and `context/code/clients.md`.
     notice: Option<Notice>,
+    access: Access,
+}
+
+/// Whether this session may change the document. A read-only boot still
+/// answers every view command; `context/code/clients.md` says why.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+enum Access {
+    Writable,
+    /// Another context holds the handle, so no edit could ever be kept.
+    ReadOnly,
+}
+
+impl Access {
+    /// Decided before the host sees the frame, so nothing is applied and
+    /// nothing projected: an edit that cannot be saved is never painted.
+    fn refuses(self, frame: &ClientFrame) -> bool {
+        match (self, frame) {
+            (Self::ReadOnly, ClientFrame::Commands(cmds)) => {
+                cmds.iter().any(|c| matches!(c, Command::Model { .. }))
+            }
+            (Self::ReadOnly, _) | (Self::Writable, _) => false,
+        }
+    }
 }
 
 impl Worker {
     /// Take an `App` already pointed at its document and storage backend;
     /// building that is [`crate::vfs`]'s job and the shim's, not this loop's.
     pub fn new(app: App) -> Self {
+        Self::with_access(app, Access::Writable)
+    }
+
+    /// A session over a document another context holds (improvise-i0v9). It
+    /// paints and navigates, and refuses every edit (improvise-j4qt).
+    pub fn read_only(app: App) -> Self {
+        Self::with_access(app, Access::ReadOnly)
+    }
+
+    fn with_access(app: App, access: Access) -> Self {
         Self {
             host: Host::new(app),
             client: None,
             notice: None,
+            access,
         }
     }
 
@@ -48,6 +82,9 @@ impl Worker {
             // Anything else before the handshake: composed against a document
             // the client was never sent, so nothing to apply and nothing to say.
             (None, _) => Vec::new(),
+            (Some(_), frame) if self.access.refuses(&frame) => {
+                vec![ServerFrame::Rejected(Rejection::ReadOnly)]
+            }
             (Some(id), frame) => {
                 // A save the client asked for is already answered; retrying it
                 // here double-writes. See `context/code/clients.md`.
@@ -272,6 +309,71 @@ mod tests {
         );
     }
 
+    /// The read-only boot `bindings::read_only` performs: the document's bytes
+    /// over a backend that refuses every write, because another tab holds it.
+    fn read_only_worker() -> Worker {
+        let fs = MemoryFs::new();
+        let path = PathBuf::from("demo.improv");
+        fs.write(&path, &demo_bytes()).unwrap();
+        let locked =
+            crate::vfs::ReadOnly::new(fs, "another tab or window is holding this document");
+        let wb = vfs::open(&locked, &Opening::named(&path)).unwrap().workbook;
+        let mut app = App::with_storage(wb, Some(path), Box::new(locked));
+        app.rebuild_layout();
+        Worker::read_only(app)
+    }
+
+    /// improvise-j4qt: a read-only tab applied an edit and PAINTED a value the
+    /// document never had. Evidence: `context/test-evidence/clients.md`.
+    #[test]
+    fn a_read_only_session_refuses_an_edit_before_anything_is_painted() {
+        let mut worker = read_only_worker();
+        let mut grid = connect(&mut worker);
+        let was = grid.cells[0][0].text.clone();
+        let cmd = reduce::caught_up(
+            worker.app(),
+            ModelCommand::SetCell {
+                cell: addressed(&grid, 0, 0),
+                value: CellValue::Number(4242.0),
+            },
+        );
+
+        let out = worker.on_frame(ClientFrame::command(cmd));
+        let _ = folded(&mut grid, &out);
+
+        assert_eq!(
+            out,
+            vec![ServerFrame::Rejected(Rejection::ReadOnly)],
+            "a session that cannot save must refuse the edit, and say only that"
+        );
+        assert_eq!(grid.cells[0][0].text, was, "the refused value was painted");
+        assert_eq!(
+            worker.take_notice(),
+            None,
+            "nothing was applied, so no persist ran and none failed"
+        );
+        assert!(
+            !worker.app().model_state.dirty,
+            "the edit reached the document"
+        );
+    }
+
+    /// The control: read-only is about the DOCUMENT. Moving the cursor writes
+    /// only this client's slice, so it must still be answered.
+    #[test]
+    fn a_read_only_session_still_answers_a_view_command() {
+        let mut worker = read_only_worker();
+        let _ = connect(&mut worker);
+        let out = worker.on_frame(ClientFrame::command(Command::SetSelected {
+            row: 1,
+            col: 0,
+        }));
+        assert!(
+            !out.iter().any(|f| matches!(f, ServerFrame::Rejected(_))),
+            "a read-only tab refused a cursor move: {out:?}"
+        );
+    }
+
     /// improvise-zre1: the failure stayed in a `status_msg` the worker has no
     /// UI for, so the client could not tell "save failed" from "save pending".
     #[test]

harness/tests/shell.spec.js

@@ -257,6 +257,40 @@ test.describe("the standalone deployment", () => {
     await first.close();
   });
 
+  // improvise-j4qt: the read-only tab used to APPLY an edit to its own copy
+  // and paint it, a number the document never had. It must refuse the edit.
+  test("a read-only tab refuses an edit and never paints it", async ({ context }) => {
+    const doc = `read-only-edit-${Date.now()}.improv`;
+    const first = await context.newPage();
+    await first.goto(`/app/index.html?doc=${doc}`);
+    await expect(first.locator("#grid table")).toBeVisible({ timeout: 20_000 });
+
+    const second = await context.newPage();
+    await second.goto(`/app/index.html?doc=${doc}`);
+    await expect(second.locator("#grid table")).toBeVisible({ timeout: 20_000 });
+    await expect(second.locator("#status")).toContainText("read-only");
+
+    const cell = second.locator("#grid td.editable").first();
+    const before = await cell.textContent();
+    await cell.click();
+    const input = cell.locator("input");
+    await expect(input).toBeFocused();
+    await input.fill("4242");
+    await input.press("Enter");
+
+    // The refusal is what arrives, and a document message is a toast. Waiting
+    // on it first means the grid below is asserted AFTER the answer landed.
+    await expect(second.locator("#toast")).toContainText("read-only", { timeout: 10_000 });
+    await expect(cell).toHaveText(before);
+    await expect(second.locator("#grid")).not.toContainText("4,242");
+    // The boot's reason stands; no failed save replaced it (the old symptom).
+    await expect(second.locator("#status")).toContainText("read-only");
+    await expect(second.locator("#status")).not.toContainText("NOT SAVED");
+
+    await second.close();
+    await first.close();
+  });
+
   // What lands on screen is the HOST's value, not the string typed — the
   // round trip through a real worker, which no unit test can stand in for.
   test("committing an edit sends one frame and shows the host's value", async ({