diff --git a/context/code/clients.md b/context/code/clients.md index c9daea61..cccf01c1 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 fd39ad65..879431d4 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -4177,3 +4177,40 @@ 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). diff --git a/crates/improvise-worker-server/src/worker.rs b/crates/improvise-worker-server/src/worker.rs index 19c38555..ca882eb9 100644 --- a/crates/improvise-worker-server/src/worker.rs +++ b/crates/improvise-worker-server/src/worker.rs @@ -315,7 +315,8 @@ mod tests { 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 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(); @@ -351,7 +352,10 @@ mod tests { None, "nothing was applied, so no persist ran and none failed" ); - assert!(!worker.app().model_state.dirty, "the edit reached the document"); + assert!( + !worker.app().model_state.dirty, + "the edit reached the document" + ); } /// The control: read-only is about the DOCUMENT. Moving the cursor writes @@ -360,7 +364,10 @@ mod tests { 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 })); + 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:?}" 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 ({