diff --git a/context/code/clients.md b/context/code/clients.md index 995b99f4..edd147d8 100644 --- a/context/code/clients.md +++ b/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 diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index e8c8bc86..85766267 100644 --- a/context/test-evidence/clients.md +++ b/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 diff --git a/crates/improvise-protocol/src/basis.rs b/crates/improvise-protocol/src/basis.rs index 4a908268..58d0ce9e 100644 --- a/crates/improvise-protocol/src/basis.rs +++ b/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, } diff --git a/crates/improvise-wasm-client/src/socket.rs b/crates/improvise-wasm-client/src/socket.rs index 2d5a21b0..868f01b5 100644 --- a/crates/improvise-wasm-client/src/socket.rs +++ b/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 => { diff --git a/crates/improvise-worker-server/src/bindings.rs b/crates/improvise-worker-server/src/bindings.rs index 232e5d08..ca71fb7d 100644 --- a/crates/improvise-worker-server/src/bindings.rs +++ b/crates/improvise-worker-server/src/bindings.rs @@ -111,7 +111,7 @@ async fn read_only(storage: &OpfsFs, path: PathBuf, why: String) -> Result, + 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] diff --git a/harness/tests/shell.spec.js b/harness/tests/shell.spec.js index 9648ea92..9e393226 100644 --- a/harness/tests/shell.spec.js +++ b/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 ({