From 5690903d605febcfe1c54266b9b536b2c035fdf3 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 07:48:15 -0700 Subject: [PATCH 01/15] improvise-vndi: an inline edit opened before a document swap is refused EditTarget captures document_generation when the editor opens, and BasisCache::claim_as_read claims it rather than the current one. The layout tier stays current, as clients.md argues for 2wu3. A red test through a real Host over MemoryFs: A opens North/Revenue, B loads a same-shaped document holding 5555, A commits "42" -> DocumentReplaced, 5555 survives. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/clients.md | 25 +++++--- context/test-evidence/clients.md | 20 ++++++ crates/improvise-protocol/src/basis.rs | 7 +- crates/improvise-wasm-client/src/socket.rs | 20 ++++-- .../tests/browser_round_trip.rs | 64 +++++++++++++++++++ 5 files changed, 119 insertions(+), 17 deletions(-) diff --git a/context/code/clients.md b/context/code/clients.md index 4af02584..cc1c6143 100644 --- a/context/code/clients.md +++ b/context/code/clients.md @@ -1442,14 +1442,23 @@ 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 }`: the document and the record's counter +as the client had them when the editor opened. `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. @@ -1461,7 +1470,7 @@ 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 +struct so it can grow.** The generation was the second field to join it. 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. diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 15ff6714..6c729efc 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -4749,3 +4749,23 @@ claims at commit. The commit was then *accepted*. With the refusal assertion disabled, the host held `Some(Number(42.0))`, so A's commit had re-created the cell B cleared. Under that rival, the 0j0f test above failed as well. Both files were restored. + +### 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`. + diff --git a/crates/improvise-protocol/src/basis.rs b/crates/improvise-protocol/src/basis.rs index 43108c66..4884e5be 100644 --- a/crates/improvise-protocol/src/basis.rs +++ b/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, ) -> Option { 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(), }) diff --git a/crates/improvise-wasm-client/src/socket.rs b/crates/improvise-wasm-client/src/socket.rs index c01e5f65..50dfaa63 100644 --- a/crates/improvise-wasm-client/src/socket.rs +++ b/crates/improvise-wasm-client/src/socket.rs @@ -11,6 +11,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, @@ -100,17 +102,23 @@ 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 { let cell = self.record_cell_at(row, col)?; - let version = self.state().basis.current()?.record(cell.record)?; - Some(EditTarget { cell, version }) + let basis = self.state().basis.current()?; + let version = basis.record(cell.record)?; + let document_generation = basis.document_generation; + Some(EditTarget { + document_generation, + cell, + version, + }) } /// The write an editor opened earlier commits: to the record it opened on, /// claimed at the version it read then (improvise-2wu3, improvise-0j0f). pub fn typed_target_frame(&mut self, target: EditTarget, text: &str) -> Option { - 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)?, diff --git a/crates/improvise-wasm-client/tests/browser_round_trip.rs b/crates/improvise-wasm-client/tests/browser_round_trip.rs index f92e9deb..1ab86c87 100644 --- a/crates/improvise-wasm-client/tests/browser_round_trip.rs +++ b/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 = 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. From 5eaafbae1e11bf0328d952c68baf45643f2ed2c4 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 08:18:55 -0700 Subject: [PATCH 02/15] =?UTF-8?q?improvise-b114:=20red=20=E2=80=94=20a=20w?= =?UTF-8?q?ire=20write=20naming=20a=20measure=20group=20is=20stored?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The record {Month=Feb} is stored, so the stored arm accepts it and Month=Feb, _Measure=Money = 7 lands: a non-item coordinate. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- crates/improvise-command/src/ui/host.rs | 33 +++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/crates/improvise-command/src/ui/host.rs b/crates/improvise-command/src/ui/host.rs index 9502dec0..1bc74bc9 100644 --- a/crates/improvise-command/src/ui/host.rs +++ b/crates/improvise-command/src/ui/host.rs @@ -1554,6 +1554,39 @@ mod tests { assert_eq!(model.get_cell(&target), Some(&CellValue::Number(7.0))); } + /// improvise-b114: a coordinate naming a group is no item, in any view — see + /// context/test-evidence/command.md. + #[test] + fn a_wire_write_naming_a_measure_group_is_rejected_in_any_view() { + for collapsed in [true, false] { + let mut wb = Workbook::new("T"); + wb.add_category("Month").unwrap(); + for month in ["Jan", "Feb"] { + wb.add_item("Month", month).unwrap(); + for measure in ["Revenue", "Cost"] { + wb.add_item_in_group("_Measure", measure, "Money").unwrap(); + let key = month_cell(month, measure); + wb.model.set_cell(key, CellValue::Number(10.0)); + } + } + wb.add_item("_Measure", "Units").unwrap(); + wb.active_view_mut().set_axis("Month", Axis::Row); + wb.active_view_mut().set_axis("_Measure", Axis::Column); + if collapsed { + wb.active_view_mut() + .toggle_group_collapse("_Measure", "Money"); + } + let mut app = App::new(wb, None); + app.rebuild_layout(); + let mut host = Host::new(app); + let before = document_text(&host); + let answer = wire_write(&mut host, &month_cell("Feb", "Money"), false); + assert!(refused(&answer), "collapsed={collapsed}: {answer:?}"); + let after = document_text(&host); + assert_eq!(after, before, "collapsed={collapsed}: a group was stored"); + } + } + /// A STORED record is writable whatever the host's view does to it — see /// context/test-evidence/command.md (improvise-sjdb). #[test] From 1c78e71322b58bd1d508470598b4279ef7fec65c Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 08:19:35 -0700 Subject: [PATCH 03/15] wip-b114 fix --- crates/improvise-command/src/ui/app.rs | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index 0113359c..95ec1299 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -583,7 +583,20 @@ impl App { f.target ) }); - derived.or_else(|| self.aggregate_refusal(cell.record, &key)) + let refusal = self.group_refusal(&key).or(derived); + refusal.or_else(|| self.aggregate_refusal(cell.record, &key)) + } + + /// A coordinate naming a group rather than an item is no cell in ANY view, so + /// this asks the document, never the layout (improvise-b114). + fn group_refusal(&self, key: &CellKey) -> Option { + let model = &self.model_state.workbook.model; + key.0.iter().find_map(|(cat, name)| { + let category = model.category(cat)?; + let group = !category.items.contains_key(name.as_str()) + && !category.items_in_group(name).is_empty(); + group.then(|| format!("{name} is a group of {cat}, so there is no one cell to write")) + }) } /// A record the store does not hold is known only by its layout position, and From d9ea594604a266ec81dc43d5ca3a6ad16c676a84 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 08:21:59 -0700 Subject: [PATCH 04/15] A TUI status message lasts until the next key, then the hint returns improvise-cq3d: nothing in production cleared status_msg, and draw_status prints it instead of the derived hint, so after the first message of a session the TUI never drew the key hint again, in any mode. run_tui now sends each key through draw.rs::press, which applies SetStatus("") and then dispatches: a message a key produces is drawn after that key and expires at the next one, vim-style, so an error is never wiped before it is drawn. The expiry lives in the TUI front end rather than App::dispatch_key, which the lockstep harness shares with the browser. The browser already shows messages transiently (a TOAST_MS toast) beside an always-drawn #hint. Reasoning: context/code/tui.md. The test drives press: a stale message must give way to TileSelect's hint, a message must survive its own key, and give way at the next. Red at base at the first step; clearing after dispatch fails the second. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/tui.md | 45 +++++++++++++++++++++++++++++++ context/test-evidence/tui.md | 23 ++++++++++++++++ src/draw.rs | 52 ++++++++++++++++++++++++++++++++++-- 3 files changed, 118 insertions(+), 2 deletions(-) diff --git a/context/code/tui.md b/context/code/tui.md index 68c830f5..309ee160 100644 --- a/context/code/tui.md +++ b/context/code/tui.md @@ -379,3 +379,48 @@ again. hint is width-constrained and the history lives on help page 5 under "View history" with both halves. That is the #149 lesson — a hint that does not fit is never drawn, so the test pinning the widget cannot see it. + +## A status message lasts until the next key (improvise-cq3d) + +`draw_status` draws `status_msg` *instead of* the mode's derived hint, and +nothing in production ever cleared `status_msg`: it was only replaced by the next +message. So after the first "Saved", "Loaded" or error of a session, the derived +key hint (the one key table on screen, improvise-rr8n) was never drawn again, in +any mode. + +**The rule: a message expires at the start of the next keypress.** `draw.rs::press` +applies `SetStatus("")` and then dispatches the key, so: + +- a message a key produces is drawn after that key, whatever it is, errors + included. It stays until the user presses something, which is the moment they + have acted on it. An error is never wiped before it has been drawn. +- a message that arrives outside a key, such as one from startup or an import, + also stands until the next key. +- the next key clears it *before* its own effects run. So a key that writes a + new message shows that message, and a key that writes none shows the hint. + +**Rejected alternatives.** + +- *Expire on mode change.* This fails the ticket's own case: a message written in + Normal would stand in Normal indefinitely. +- *Expire on the next successful command.* Effects carry no notion of success, + and a failed command would leave the stale message in place. That is exactly + the case where the user most needs the hint back. +- *Clear after dispatch.* Every message would be wiped before it is drawn. The + test's middle step exists to catch that. +- *Compose message and hint on one line.* A long message still eats the hint, and + the stale message still stands forever. + +**Why here and not in `App::dispatch_key`.** `dispatch_key` is shared with the +lockstep harness (`reduce_key`), which compares `status_msg` on the host and the +browser after every key. Expiring there without changing the browser's resolver +too would make the two ends disagree. The expiry is also a presentation decision +about the TUI's one-line status row, so it belongs to the TUI front end. + +**The browser already behaves this way.** Its `status_msg` is state that is never +cleared either, but `web/app.js` shows it in a toast that hides after +`TOAST_MS` (`web/shell.js::createToasts`), and `#hint` is a separate element that +is always drawn. So a browser user sees a message briefly and never loses the +hint. The TUI now does the same, on a keypress rather than a timer, because a +terminal status row has one line to share and a keypress is the event the TUI +already has. diff --git a/context/test-evidence/tui.md b/context/test-evidence/tui.md index 080a27b0..9a0d2197 100644 --- a/context/test-evidence/tui.md +++ b/context/test-evidence/tui.md @@ -870,3 +870,26 @@ exactly, and first asserts that hint is non-empty and pages. the protocol keymap). The test then fails: the copy still advertises `n`, which the keymap no longer binds. That rebinding case is exactly the one the ticket said the old footer would lie through. + +### a_status_message_gives_way_to_the_hint_on_the_next_key + +**What broke (improvise-cq3d).** Nothing in production ever cleared +`status_msg`, and `draw_status` prints it instead of the derived hint. After the +first message of a session, the TUI never drew the key hint again, in any mode. + +**The case, in three steps, each guarding one direction.** +1. A standing "Saved to x.improv", then `T`: in TileSelect, a mode other than + the one the message was written in, the row must hold TileSelect's hint and + not the stale message. This is the ticket's assertion. +2. `Space` cycles an axis and writes a message: that message must be on the row + after its own key. This guards against expiring too eagerly. +3. `Esc` back to Normal: the axis message is gone and Normal's hint is drawn. + +It drives keys through `draw.rs::press`, the function `run_tui` calls, rather +than `App::handle_key`, because the expiry is the front end's rule. + +**Red, and the rival.** +- With `press` delegating straight to `handle_key` (the old behaviour), it fails + at step 1: `" TILES Saved to x.improv …"`. +- With the expiry moved *after* dispatch, it fails at step 2: "the axis cycle + must say what it did", since every message is wiped before it can be drawn. diff --git a/src/draw.rs b/src/draw.rs index c287c748..11db0e70 100644 --- a/src/draw.rs +++ b/src/draw.rs @@ -79,7 +79,7 @@ pub fn run_tui( if event::poll(Duration::from_millis(100))? { match event::read()? { Event::Key(key) => { - app.handle_key(convert_key_event(key))?; + press(&mut app, convert_key_event(key))?; } Event::Resize(w, h) => { app.term_width = w; @@ -99,6 +99,13 @@ pub fn run_tui( Ok(()) } +/// One keypress in the TUI. A message lasts until the next key, so it is drawn +/// once and then gives way to the hint (improvise-cq3d, `context/code/tui.md`). +fn press(app: &mut App, key: key::KeyEvent) -> Result<()> { + app.apply_effects(vec![Box::new(crate::ui::effect::SetStatus(String::new()))]); + app.handle_key(key) +} + // ── Key event conversion (TUI boundary) ────────────────────────────────────── // The only place crossterm's key types and `command::key` may both appear. @@ -566,7 +573,8 @@ fn draw_welcome(f: &mut Frame, area: Rect) { #[cfg(test)] mod bottom_bar_tests { - use super::{draw_bottom_bar, minibuffer_line}; + use super::{draw_bottom_bar, minibuffer_line, press}; + use crate::command::key::{KeyCode, KeyEvent, KeyModifiers}; use crate::ui::app::{App, AppMode}; use crate::workbook::Workbook; use ratatui::{Terminal, backend::TestBackend, layout::Rect}; @@ -587,6 +595,46 @@ mod bottom_bar_tests { .collect() } + /// A message lasts until the next key, then the derived hint is back + /// (improvise-cq3d). Why each step: `context/test-evidence/tui.md`. + #[test] + fn a_status_message_gives_way_to_the_hint_on_the_next_key() { + let key = |code| KeyEvent::new(code, KeyModifiers::NONE); + let mut app = App::new(Workbook::new("T"), None); + app.view_state.status_msg = "Saved to x.improv".to_string(); + + press(&mut app, key(KeyCode::Char('T'))).unwrap(); + assert!(matches!(app.view_state.mode, AppMode::TileSelect)); + let row = bottom_row(&app, 240); + assert!( + !row.contains("Saved to"), + "a stale message outlived a key: {row:?}" + ); + assert!( + row.contains(&app.hint_text()), + "no hint in TileSelect: {row:?}" + ); + + press(&mut app, key(KeyCode::Char(' '))).unwrap(); + let said = app.view_state.status_msg.clone(); + assert!(!said.is_empty(), "the axis cycle must say what it did"); + assert!( + bottom_row(&app, 240).contains(&said), + "a message was wiped before it was drawn" + ); + + press(&mut app, key(KeyCode::Esc)).unwrap(); + let row = bottom_row(&app, 240); + assert!( + !row.contains(&said), + "the axis message outlived the next key: {row:?}" + ); + assert!( + row.contains(&app.hint_text()), + "no hint back in Normal: {row:?}" + ); + } + /// The wizard overlay draws correct per-step hints; the status bar stacked a /// second static table under it (improvise-rr8n) — `context/test-evidence/tui.md`. #[test] From 872f9bc07b4f3a6b67f7180c5e321f53e10572e5 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 08:29:28 -0700 Subject: [PATCH 05/15] improvise-b114: no coordinate of a wire write may name a group App::group_refusal asks the document, not the layout, whether any coordinate names a group rather than an item, and runs first in unwritable: a measure-half fold hides from both the stored arm and the projection. Rival (rule removed) fails the new test and nothing else. command.md and the ciw8 fold evidence now say the aggregate arm guards only the None-axis case. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/command.md | 10 ++++++++++ context/test-evidence/command.md | 22 ++++++++++++++++++++-- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/context/code/command.md b/context/code/command.md index 05f0cefc..6950d05f 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -3535,6 +3535,16 @@ the refusal marks the batch, `Host::mutating_frame` undoes the frame, the asker gets `Rejected(MutationRefused)`, and the reason is on the host's status line. The question it asks is the projection's own, never a second derivation of it: +- A coordinate that names a **group** rather than an item is refused, in any + view (improvise-b114). A group has no cell of its own, so writing one stores a + non-item coordinate. This rule asks the document, not the layout, and runs + first, because a fold in the measure half hides from both rules below. Take + `{Month=Feb, _Measure=Money}`, where `Money` groups Revenue and Cost. Its + record `{Month=Feb}` is stored, so the second rule below accepts it and never + asks the projection. And with `Money` expanded the host draws no such cell at + all, so the third rule has no position to ask. A fold on the row axis is + refused here too, so the `Aggregate` arm below now guards only the None-axis + aggregate. - A cell a formula owns is refused. That is `cell_id`'s own `Derived` rule, `formula_for(&key)`. It is asked directly because a stored record may sit on no position in the host's current view. diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index e44a90a2..17efb870 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5789,8 +5789,10 @@ client sends `SetCell` (then, separately, `ClearCell`) for `{Month=Q2, _Measure= with a caught-up basis. At `ea119bd9` both were answered `Accepted`, and the `SetCell` stored `Month=Q2, _Measure=Cost = 7`, the non-item coordinate improvise-gw18 exists to prevent. The test asserts `Rejected(MutationRefused)` -and an unchanged document for both. Rival: the aggregate refusal disabled. This -test and the None-axis test fail. The control is +and an unchanged document for both. Rival: the aggregate refusal disabled. At +`46e75c41` that failed this test and the None-axis test. Since improvise-b114 a +fold is refused first by the group rule, so this rival fails only the None-axis +test. Disabling both rules fails this test. The control is `a_wire_write_to_a_stored_cell_beside_a_fold_is_accepted`, so a host that refused every wire write cannot pass. @@ -5833,6 +5835,22 @@ that the target projects as `Record`, so a fixture where the first column is harmless cannot pass. The fix looks the cell up by its whole key. The rival is the old lookup by record, which is this test's red commit, `6e021699`. +### a_wire_write_naming_a_measure_group_is_rejected_in_any_view + +improvise-b114, found in the review of PR #217 at `bc060f67` (the reviewer's r3). +`_Measure` is on Column, Revenue and Cost are grouped as `Money`, and `Units` is +ungrouped. Every month is funded. A caught-up `SetCell` on +`{Month=Feb, _Measure=Money}` was answered `Accepted` and stored +`Month=Feb, _Measure=Money = 7`, a non-item coordinate. Its record `{Month=Feb}` +is stored, so the stored arm accepted it without asking the projection. The test +runs with `Money` collapsed, where the host draws that cell as the fold, and +expanded, where the host draws no such cell. It asserts `MutationRefused` and an +unchanged document in both, so a rule that asks the host's layout cannot pass +the expanded half. Red at `5eaafbae`. The fix is `App::group_refusal`, which asks +the document whether any coordinate names a group rather than an item. Rival: +the group rule removed. Injected into a full workspace run, it fails this test +and nothing else. + ### a_stored_record_the_hosts_view_aggregates_is_writable improvise-sjdb, from the #217 review. South/Revenue is stored, then `Product` is From c4877456a42ac81ddc8173c09d7356671bd0625e Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 08:26:36 -0700 Subject: [PATCH 06/15] WIP red: mouse editor stale seed and touched (improvise-ugj4, improvise-jibp) --- harness/tests/contract.spec.js | 83 ++++++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/harness/tests/contract.spec.js b/harness/tests/contract.spec.js index 906a0455..cb8d816b 100644 --- a/harness/tests/contract.spec.js +++ b/harness/tests/contract.spec.js @@ -9,6 +9,19 @@ import { test, expect } from "@playwright/test"; // the same with the handshake window). import { NOTICE_KEY } from "../../web/shell.js"; +/** Every string the page posts to its worker, kept on `window.__posted`. */ +function recordPosts() { + window.__posted = []; + const post = Worker.prototype.postMessage; + Worker.prototype.postMessage = function (message, ...rest) { + window.__posted.push(String(message)); + return post.call(this, message, ...rest); + }; +} + +const setCells = (page) => + page.evaluate(() => window.__posted.filter((m) => m.includes("SetCell")).length); + async function openApp(page) { await page.goto("/app/index.html"); await expect(page.locator("#grid table")).toBeVisible({ timeout: 20_000 }); @@ -320,6 +333,76 @@ test.describe("a pointer event reaches the client and its answer is painted", () await expect.poll(writes).toBe(1); }); + // improvise-jibp. The page reports its first edit, so retyping the seed over + // the selection is a write, not an untouched editor. + test("a mouse editor typed back to its own seed still writes", async ({ page }) => { + await page.addInitScript(recordPosts); + await openApp(page); + const cells = page.locator("#grid tbody td"); + await cells.nth(4).click(); + const input = cells.nth(4).locator("input"); + await expect(input).toHaveCount(1); + const seed = await input.inputValue(); + expect(seed, "the fixture cell has no seed to retype").not.toBe(""); + await page.keyboard.type(seed); + await expect(input).toHaveValue(seed); + await page.keyboard.press("Enter"); + await expect.poll(() => setCells(page)).toBe(1); + }); + + // improvise-ugj4. A frame that lands while an editor is open changes the + // cache but not the paint; the next editor must open on what it captured. + test("an editor opened after an unpainted write shows and commits what it captured", async ({ + page, + }) => { + await page.addInitScript(recordPosts); + await page.addInitScript(() => { + window.__frames = []; + const add = Worker.prototype.addEventListener; + Worker.prototype.addEventListener = function (type, listener, ...rest) { + if (type !== "message") return add.call(this, type, listener, ...rest); + window.__deliver = (data) => listener.call(this, new MessageEvent("message", { data })); + const recorded = (event) => { + if (typeof event.data === "string") window.__frames.push(event.data); + return listener.call(this, event); + }; + return add.call(this, type, recorded, ...rest); + }; + }); + await openApp(page); + const cells = page.locator("#grid tbody td"); + const target = cells.nth(4); + const where = await target.evaluate((td) => [Number(td.dataset.row), Number(td.dataset.col)]); + + await cells.nth(0).click(); + await expect(cells.nth(0).locator("input")).toHaveCount(1); + // Another client's write to cell 4, delivered while cell 0's editor is open. + await page.evaluate(([row, col]) => { + let latest = null; + const walk = (node) => { + if (Array.isArray(node)) node.forEach(walk); + else if (node && typeof node === "object") { + for (const [key, value] of Object.entries(node)) { + if (key === "Cells" && Array.isArray(value)) { + for (const [[r, c], cell] of value) if (r === row && c === col) latest = cell; + } else walk(value); + } + } + }; + window.__frames.forEach((f) => walk(JSON.parse(f))); + const moved = { ...latest, text: "777", edit_text: "777", value: { Number: 777 } }; + window.__deliver(JSON.stringify({ Commands: [{ Project: { Grid: { Cells: [[[row, col], moved]] } } }] })); + }, where); + + await target.click(); + const input = target.locator("input"); + await expect(input).toHaveCount(1); + await expect(input, "the editor opened on the stale paint").toHaveValue("777"); + await page.keyboard.press("Enter"); + await page.waitForTimeout(300); + expect(await setCells(page), "an untouched editor wrote the stale text").toBe(0); + }); + // improvise-b8ot moved these five surfaces onto one handler per stable root. // The three tests below are the properties a naive delegation loses, each of // which is silent: the page still paints, and only the pointer stops working. From 8747074e52cea2a868c561b3f12fa7008f51b8f6 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 08:33:40 -0700 Subject: [PATCH 07/15] Seed the mouse editor from its target, and mark the target on the first edit (improvise-ugj4, improvise-jibp) Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- crates/improvise-wasm-client/src/lib.rs | 60 ++++++++++++++++++++++ crates/improvise-wasm-client/src/render.rs | 8 --- crates/improvise-wasm-client/src/socket.rs | 32 ++++++++++++ web/app.js | 17 +++--- web/shell.js | 5 +- web/tests/dispatch.test.js | 5 +- web/tests/grid_plan.test.js | 9 ---- 7 files changed, 105 insertions(+), 31 deletions(-) diff --git a/crates/improvise-wasm-client/src/lib.rs b/crates/improvise-wasm-client/src/lib.rs index e02f7cf9..e1cafc33 100644 --- a/crates/improvise-wasm-client/src/lib.rs +++ b/crates/improvise-wasm-client/src/lib.rs @@ -109,6 +109,24 @@ impl Client { .map_err(|e| JsError::new(&e.to_string())) } + /// What the editor opened on `target` shows: the seed the target captured, + /// never the page's paint, which can be stale (improvise-ugj4). + #[wasm_bindgen] + pub fn edit_seed(&self, target: &str) -> Result { + let target: EditTarget = + serde_json::from_str(target).map_err(|e| JsError::new(&e.to_string()))?; + Ok(target.opened.seed) + } + + /// `target` after the user's first edit of its ``, still opaque to + /// the page (improvise-jibp). + #[wasm_bindgen] + pub fn edited_target(&self, target: &str) -> Result { + let target: EditTarget = + serde_json::from_str(target).map_err(|e| JsError::new(&e.to_string()))?; + serde_json::to_string(&target.edited()).map_err(|e| JsError::new(&e.to_string())) + } + /// The frame for "the user typed `text` into the cell `target` names". By /// identity, never position: rows renumber while an editor is open. #[wasm_bindgen] @@ -267,6 +285,48 @@ mod tests { assert_eq!(client.typed_cell_frame(0, 0, "42"), None); } + /// BUG (improvise-ugj4, improvise-jibp) — see `context/test-evidence/clients.md`. + #[test] + fn the_page_opens_its_editor_on_what_the_target_captured() { + let id = improvise_protocol::RecordCell { + record: surrogate("fe4c8e5e9cbd578e0d388f4b2b5a7879"), + measure: Some("Revenue".into()), + }; + let at = |text: &str, stored: &str| { + let mut cell = improvise_protocol::CellDisplay::number(text); + cell.edit_text = Some(stored.into()); + cell.id = Some(CellId::Record(id.clone())); + Projection::Grid(GridUpdate::Cells(vec![((0, 0), cell)])) + }; + let mut client = init(); + client.bootstrap(Snapshot { + view: ViewState::default(), + seed: vec![ + Projection::Grid(GridUpdate::RowFrame(AxisFrame::flat(["North"]))), + Projection::Grid(GridUpdate::ColFrame(AxisFrame::flat(["Revenue"]))), + at("1,200.50", "1200.5"), + Projection::Basis(improvise_protocol::basis::published( + 0, + 0, + [(id.record, 0)].into_iter().collect(), + )), + ], + }); + let first = client.edit_target(0, 0).unwrap().expect("a record cell"); + assert_eq!(client.edit_seed(&first).unwrap(), "1200.5", "not the drawn text"); + client.fold_frame(ServerFrame::Commands(vec![Command::Project(at("777", "777"))])); + let later = client.edit_target(0, 0).unwrap().expect("a record cell"); + assert_eq!(client.edit_seed(&later).unwrap(), "777", "not the cache at open"); + assert_eq!(client.edit_seed(&first).unwrap(), "1200.5", "a target is one capture"); + + assert!(client.typed_cell(&later, "777").unwrap().is_none()); + let edited = client.edited_target(&later).unwrap(); + assert!( + client.typed_cell(&edited, "777").unwrap().is_some(), + "an edited editor typed back to its seed composed nothing" + ); + } + /// The end-to-end shape a JS harness drives: bootstrap, key, message. #[test] fn a_client_can_be_driven_entirely_through_json() { diff --git a/crates/improvise-wasm-client/src/render.rs b/crates/improvise-wasm-client/src/render.rs index 24ca5e93..f71d120e 100644 --- a/crates/improvise-wasm-client/src/render.rs +++ b/crates/improvise-wasm-client/src/render.rs @@ -133,9 +133,6 @@ pub struct RenderCell { /// May this client compose a write for it? `CellId::Record` alone. pub editable: bool, pub selected: bool, - /// What an editor should start with: the stored value, not the formatted - /// one. Seeding from `text` turns `1200` into the string `"1,200"`. - pub edit_text: String, } impl RenderCell { @@ -155,7 +152,6 @@ impl RenderCell { | None => false, }, selected, - edit_text: cell.edit_text.clone().unwrap_or_default(), } } } @@ -1024,10 +1020,6 @@ mod tests { "a row says which of the two it is; the page does not guess" ); assert_eq!(value["rows"][0]["labels"][0], "North"); - assert_eq!( - value["rows"][0]["cells"][0]["edit_text"], "1200", - "an editor seeds from the stored value, never from the drawn one" - ); } fn page_axis(cat: &str, selected: &str, items: Option>) -> PageAxis { diff --git a/crates/improvise-wasm-client/src/socket.rs b/crates/improvise-wasm-client/src/socket.rs index 1bb05232..2da28cd7 100644 --- a/crates/improvise-wasm-client/src/socket.rs +++ b/crates/improvise-wasm-client/src/socket.rs @@ -19,6 +19,20 @@ pub struct EditTarget { pub opened: OpenedEditor, } +impl EditTarget { + /// The same target after the user's first edit: a commit of its seed is then + /// a write (improvise-jibp). The page's `` is not a buffer `resolve` sees. + pub fn edited(self) -> Self { + Self { + opened: OpenedEditor { + touched: true, + ..self.opened + }, + ..self + } + } +} + /// What a refused batch says to the person who composed it. Worded here rather /// than on `Rejection`, and why the wording matters: `context/code/clients.md`. fn rejection_message(rejection: &Rejection) -> String { @@ -304,6 +318,24 @@ mod tests { } } + /// BUG (improvise-jibp) — see `context/test-evidence/clients.md`. + #[test] + fn a_mouse_target_marked_edited_writes_its_own_seed() { + let mut client = client_showing(Some(CellId::Record(record("Revenue")))); + let target = client.edit_target_at(0, 0).expect("a record cell"); + assert_eq!( + client.typed_target_frame(target.clone(), "1200"), + None, + "control: an untouched editor committing its seed writes nothing" + ); + let edited = target.edited(); + assert_eq!(edited.opened.seed, "1200", "marking must not move the seed"); + assert!( + client.typed_target_frame(edited, "1200").is_some(), + "an edited editor typed back to its seed was dropped" + ); + } + /// BUG (improvise-ixxx) — see `context/test-evidence/clients.md`. #[test] fn a_mouse_commit_is_judged_against_the_seed_it_opened_with() { diff --git a/web/app.js b/web/app.js index 31289ce1..3277eb55 100644 --- a/web/app.js +++ b/web/app.js @@ -546,20 +546,19 @@ function gridTable(model) { /** Open an inline editor. Committing sends a frame; the value that comes * back is the server's, never the string typed here. */ -function edit(td, row, col, seed) { +function edit(td, row, col) { if (editing) return; // WHICH cell, captured now: rows renumber while the editor is open, so the // position is not an address by commit time (improvise-2wu3). - const target = client.edit_target(row, col); + let target = client.edit_target(row, col); if (!target) return; - // `seed` is `RenderCell::edit_text` -- the STORED value, not the drawn one: - // `td.textContent` has been through the number format. - const original = seed; // What was on the screen, kept so closing can put it back: `close` no longer - // always repaints, so restoring `seed` would leave `1200` where `1,200` was. + // always repaints, so restoring the seed would leave `1200` where `1,200` was. const drawn = td.textContent; const input = document.createElement("input"); - input.value = original; + // The seed the target captured, never the paint, which may be stale while + // another editor is open: one capture shown and compared (improvise-ugj4). + input.value = client.edit_seed(target); td.replaceChildren(input); input.focus(); input.select(); @@ -590,6 +589,8 @@ function edit(td, row, col, seed) { // These two stay DIRECT while the five surfaces above are delegated: one // transient node, one at a time, and `blur` does not bubble (improvise-b8ot). + // The first edit marks the target, so retyping the seed is still a write. + input.addEventListener("input", () => (target = client.edited_target(target)), { once: true }); input.addEventListener("keydown", (e) => { if (e.key === "Enter") dismiss(true); else if (e.key === "Escape") dismiss(false); @@ -745,7 +746,7 @@ el.grid.addEventListener("mousedown", (event) => { if (frame) transport.send(frame); // Every cell moves the selection; an editable one also opens its editor. // NOT repainted in that branch: a repaint would take the away. - if (action.editable) edit(td, action.row, action.col, action.editText); + if (action.editable) edit(td, action.row, action.col); else repaint(); }); diff --git a/web/shell.js b/web/shell.js index 169373a7..296fa1de 100644 --- a/web/shell.js +++ b/web/shell.js @@ -222,7 +222,6 @@ export function gridPlan(model) { row: dataRow, col, text: typing ? `${model.editing}▏` : cell.text, - editText: cell.edit_text, editable: cell.editable, classes: { numeric: cell.numeric && !typing, @@ -260,7 +259,7 @@ export function gridPlan(model) { /** What a data cell carries on its node, so one delegated handler can read * back the answers `gridPlan` already made (improvise-b8ot, clients.md). */ export function cellAttrs(cell) { - return { row: String(cell.row), col: String(cell.col), edit: cell.editText }; + return { row: String(cell.row), col: String(cell.col) }; } /** …and the inverse: what a delegated mousedown MEANS. `null` where the event @@ -271,7 +270,7 @@ export function cellAction(dataset, editable) { // A node that carries no coordinates is not a cell: every header and the // corner reach here, and none of them means anything. if (row === null || col === null) return null; - return { row, col, editable, editText: dataset.edit ?? "" }; + return { row, col, editable }; } /** The same, for the surfaces whose rows are named by one index: the panels, diff --git a/web/tests/dispatch.test.js b/web/tests/dispatch.test.js index 3246634e..045a6811 100644 --- a/web/tests/dispatch.test.js +++ b/web/tests/dispatch.test.js @@ -16,7 +16,6 @@ import { const cell = (text, extra = {}) => ({ text, - edit_text: text, numeric: true, error: false, selected: false, @@ -40,7 +39,7 @@ function grouped() { { kind: "data", labels: ["Cold Brew"], - cells: [cell("3", { editable: false }), cell("4", { edit_text: "4000" })], + cells: [cell("3", { editable: false }), cell("4")], }, ], }; @@ -50,7 +49,7 @@ test("a cell's attributes carry the plan's answers back to the handler", () => { const plan = gridPlan(grouped()); const last = plan.body.at(-1).cells[1]; const action = cellAction(cellAttrs(last), last.editable); - assert.deepEqual(action, { row: 1, col: 1, editable: true, editText: "4000" }); + assert.deepEqual(action, { row: 1, col: 1, editable: true }); }); // improvise-b8ot: the DATA row index is what a click sends, and `gridPlan` diff --git a/web/tests/grid_plan.test.js b/web/tests/grid_plan.test.js index c6f04420..61dbd6b1 100644 --- a/web/tests/grid_plan.test.js +++ b/web/tests/grid_plan.test.js @@ -8,7 +8,6 @@ import { gridPlan } from "../shell.js"; const cell = (text, extra = {}) => ({ text, - edit_text: text, numeric: true, error: false, selected: false, @@ -102,14 +101,6 @@ test("an empty edit buffer still means an editor is open", () => { assert.equal(plan.body[1].cells[0].text, "▏"); }); -test("the editor is seeded from the stored value, not the drawn one", () => { - const model = grouped(); - model.rows[1].cells[0] = cell("20,000", { edit_text: "20000" }); - const plan = gridPlan(model); - assert.equal(plan.body[1].cells[0].text, "20,000"); - assert.equal(plan.body[1].cells[0].editText, "20000"); -}); - test("an empty totals strip is no strip at all, not a row of blanks", () => { // Gotcha 33: an EMPTY strip means this grid has no totals — records mode, // or an empty grid — so there is nothing to draw and nothing to pad. From b060d4cc65de10612d495f297dc1826c82f9821b Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 08:35:57 -0700 Subject: [PATCH 08/15] improvise-xur5: pin the fold record written with an undrawn measure The reviewer's probe_fold_hidden_measure, over its three rows (hidden, bogus, no measure). The b114 group rule refuses all three; removing it fails this test and the b114 test and nothing else. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/command.md | 4 +++- context/test-evidence/command.md | 17 +++++++++++++++++ crates/improvise-command/src/ui/host.rs | 19 +++++++++++++++++++ 3 files changed, 39 insertions(+), 1 deletion(-) diff --git a/context/code/command.md b/context/code/command.md index 6950d05f..67a88f4a 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -3544,7 +3544,9 @@ The question it asks is the projection's own, never a second derivation of it: asks the projection. And with `Money` expanded the host draws no such cell at all, so the third rule has no position to ask. A fold on the row axis is refused here too, so the `Aggregate` arm below now guards only the None-axis - aggregate. + aggregate. It also closes improvise-xur5: a fold's record `{Month=Q2}` written + with a measure the host draws nowhere (hidden, unknown or absent) has no + position for the third rule to ask, and was accepted. - A cell a formula owns is refused. That is `cell_id`'s own `Derived` rule, `formula_for(&key)`. It is asked directly because a stored record may sit on no position in the host's current view. diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 17efb870..1fc04875 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5851,6 +5851,23 @@ the document whether any coordinate names a group rather than an item. Rival: the group rule removed. Injected into a full workspace run, it fails this test and nothing else. +### a_wire_write_to_a_fold_record_with_an_undrawn_measure_is_rejected + +improvise-xur5, found in the delta re-review of PR #217 at `6026768c`. It is the +reviewer's `review_217b::probe_fold_hidden_measure`, widened to all three rows +of the reviewer's table. Q2 is collapsed and every month is stored, so the +record `{Month=Q2}` is unstored and resolves only through the layout, at the +fold row. A caught-up write names that record with a measure the host draws +nowhere: `Cost` hidden in the host's view, `Bogus`, which is no measure at all, +or no measure. At `6026768c` all three were **accepted** and stored `Month=Q2`, +a group name, as a coordinate. The sjdb fix finds the position by the whole key, +and "not drawn" read as "not refused". Falling back to the by-record lookup for +an undrawn key would close this, but the reviewer showed it wrongly refuses +Feb × Revenue beside a collapsed `Money` group, so it is not the fix. The +improvise-b114 rule is: no coordinate names a group, asked of the document. +Rival: the group rule removed. Injected into a full workspace run, it fails this +test and the b114 test, and nothing else. + ### a_stored_record_the_hosts_view_aggregates_is_writable improvise-sjdb, from the #217 review. South/Revenue is stored, then `Product` is diff --git a/crates/improvise-command/src/ui/host.rs b/crates/improvise-command/src/ui/host.rs index 3275e69d..91c8d616 100644 --- a/crates/improvise-command/src/ui/host.rs +++ b/crates/improvise-command/src/ui/host.rs @@ -1609,6 +1609,25 @@ mod tests { } } + /// improvise-xur5 (the reviewer's `probe_fold_hidden_measure`): a fold's record + /// with a measure the host draws nowhere — see context/test-evidence/command.md. + #[test] + fn a_wire_write_to_a_fold_record_with_an_undrawn_measure_is_rejected() { + let hidden = month_cell("Q2", "Cost"); + let bogus = month_cell("Q2", "Bogus"); + let bare = CellKey::new(vec![("Month".to_string(), "Q2".to_string())]); + for key in [hidden, bogus, bare] { + let mut host = host_on_a_fold(); + let view = host.app.model_state.workbook.active_view_mut(); + view.hide_item("_Measure", "Cost"); + host.app.rebuild_layout(); + let before = document_text(&host); + let answer = wire_write(&mut host, &key, false); + assert!(refused(&answer), "{key:?} accepted: {answer:?}"); + assert_eq!(document_text(&host), before, "{key:?} stored a group"); + } + } + /// A STORED record is writable whatever the host's view does to it — see /// context/test-evidence/command.md (improvise-sjdb). #[test] From 6096c03caacad7594ec6189a58c7dbe43a7c3f8b Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 08:45:14 -0700 Subject: [PATCH 09/15] Make Model's Debug exhaustive and the Debug fingerprint canonical across instances (improvise-djcn) Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- crates/improvise-command/src/ui/reduce.rs | 40 +++++++++++++++++++++-- crates/improvise-core/src/model/cell.rs | 33 ++++++++++++++++++- crates/improvise-core/src/model/symbol.rs | 12 ++++++- crates/improvise-core/src/model/types.rs | 25 +++++++++----- crates/improvise-core/src/view/types.rs | 34 ++++++++++++++++++- 5 files changed, 130 insertions(+), 14 deletions(-) diff --git a/crates/improvise-command/src/ui/reduce.rs b/crates/improvise-command/src/ui/reduce.rs index dcc8f070..85d215c1 100644 --- a/crates/improvise-command/src/ui/reduce.rs +++ b/crates/improvise-command/src/ui/reduce.rs @@ -920,13 +920,47 @@ pub(crate) mod tests { fn a_model_command_lands_where_its_keypress_lands() { /// Two categories on different axes: a one-category fixture cannot tell /// a transpose from a cycle (`doctrine(check_the_checker)`). + /// Cells and multi-entry view maps too: two `App`s are compared, so every + /// `HashMap` the fingerprint walks must hold 2+ entries (improvise-djcn). fn pivoted() -> App { + use crate::model::cell::{CellKey, CellValue}; let mut app = app(); - app.model_state.workbook.add_category("Month").unwrap(); - app.model_state.workbook.add_item("Month", "Jan").unwrap(); - let view = app.model_state.workbook.active_view_mut(); + let wb = &mut app.model_state.workbook; + let items = [ + ("Region", ["South", "East", "West"]), + ("Month", ["Jan", "Feb", "Mar"]), + ("Channel", ["Web", "Store", "Phone"]), + ("Year", ["2024", "2025", "2026"]), + ]; + for (category, names) in items { + wb.add_category(category).ok(); + names.iter().for_each(|n| drop(wb.add_item(category, *n))); + } + for (n, (region, month)) in [("North", "Jan"), ("South", "Feb"), ("East", "Jan")] + .into_iter() + .enumerate() + { + let key = CellKey::new(vec![ + ("Region".into(), region.into()), + ("Month".into(), month.into()), + ("Channel".into(), "Web".into()), + ("Year".into(), "2024".into()), + ]); + wb.model.set_cell(key, CellValue::Number(n as f64 + 1.0)); + } + let view = wb.active_view_mut(); view.set_axis("Region", Axis::Row); view.set_axis("Month", Axis::Column); + view.set_axis("Channel", Axis::Page); + view.set_axis("Year", Axis::Page); + view.set_page_selection("Channel", "Web"); + view.set_page_selection("Year", "2024"); + view.hide_item("Region", "West"); + view.hide_item("Region", "South"); + view.hide_item("Month", "Mar"); + view.toggle_group_collapse("Region", "Coastal"); + view.toggle_group_collapse("Region", "Inland"); + view.toggle_group_collapse("Month", "Q1"); app.rebuild_layout(); app } diff --git a/crates/improvise-core/src/model/cell.rs b/crates/improvise-core/src/model/cell.rs index 7a10edf1..9b55a6c2 100644 --- a/crates/improvise-core/src/model/cell.rs +++ b/crates/improvise-core/src/model/cell.rs @@ -194,7 +194,7 @@ impl RecordEntry { } } -#[derive(Debug, Clone, Default)] +#[derive(Clone, Default)] pub struct DataStore { /// Primary storage: interned keys, insertion-ordered so records mode can /// show rows in entry order. That order is correctness — core.md §8. @@ -208,6 +208,37 @@ pub struct DataStore { records: HashMap, } +/// Resolved through the symbol table and sorted wherever the storage is a +/// `HashMap`, so two equal stores print alike (improvise-djcn). +impl std::fmt::Debug for DataStore { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let DataStore { + cells, + symbols, + index, + records, + } = self; + let cells: Vec<_> = cells + .iter() + .map(|(key, value)| (self.to_cell_key(key), value)) + .collect(); + let index: std::collections::BTreeMap<_, std::collections::BTreeSet<_>> = index + .iter() + .map(|((cat, item), keys)| { + let pair = (symbols.resolve(*cat), symbols.resolve(*item)); + (pair, keys.iter().map(|k| self.to_cell_key(k).0).collect()) + }) + .collect(); + let records: std::collections::BTreeMap<_, _> = records.iter().collect(); + f.debug_struct("DataStore") + .field("cells", &cells) + .field("symbols", symbols) + .field("index", &index) + .field("records", &records) + .finish() + } +} + impl DataStore { pub fn new() -> Self { Self::default() diff --git a/crates/improvise-core/src/model/symbol.rs b/crates/improvise-core/src/model/symbol.rs index 65d53857..a60ec9c9 100644 --- a/crates/improvise-core/src/model/symbol.rs +++ b/crates/improvise-core/src/model/symbol.rs @@ -5,12 +5,22 @@ use std::collections::HashMap; pub struct Symbol(u64); /// Bidirectional string ↔ Symbol mapping. -#[derive(Debug, Clone, Default)] +#[derive(Clone, Default)] pub struct SymbolTable { to_id: HashMap, to_str: Vec, } +/// The interned strings, sorted: which `Symbol` each got is interning order, +/// not content, so two equal stores print alike (improvise-djcn). +impl std::fmt::Debug for SymbolTable { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let SymbolTable { to_id, to_str: _ } = self; + let interned: std::collections::BTreeSet<&String> = to_id.keys().collect(); + f.debug_set().entries(interned).finish() + } +} + impl SymbolTable { #[allow(dead_code)] pub fn new() -> Self { diff --git a/crates/improvise-core/src/model/types.rs b/crates/improvise-core/src/model/types.rs index 8ae92d91..96c7057b 100644 --- a/crates/improvise-core/src/model/types.rs +++ b/crates/improvise-core/src/model/types.rs @@ -317,16 +317,25 @@ pub struct Model { /// cache in hash order, which is not a change (improvise-pc3b, gotcha 18). impl std::fmt::Debug for Model { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - let mut aggs: Vec<_> = self.measure_agg.iter().collect(); - aggs.sort_by(|a, b| a.0.cmp(b.0)); - let mut cache: Vec<_> = self.formula_cache.iter().collect(); + // Destructured, so a new field is a compile error here (improvise-djcn). + let Model { + name, + categories, + data, + formulas, + next_category_id, + measure_agg, + formula_cache, + } = self; + let aggs: std::collections::BTreeMap<_, _> = measure_agg.iter().collect(); + let mut cache: Vec<_> = formula_cache.iter().collect(); cache.sort_by(|a, b| a.0.0.cmp(&b.0.0)); f.debug_struct("Model") - .field("name", &self.name) - .field("categories", &self.categories) - .field("data", &self.data) - .field("formulas", &self.formulas) - .field("next_category_id", &self.next_category_id) + .field("name", name) + .field("categories", categories) + .field("data", data) + .field("formulas", formulas) + .field("next_category_id", next_category_id) .field("measure_agg", &aggs) .field("formula_cache", &cache) .finish() diff --git a/crates/improvise-core/src/view/types.rs b/crates/improvise-core/src/view/types.rs index 1967bfb8..e9b91961 100644 --- a/crates/improvise-core/src/view/types.rs +++ b/crates/improvise-core/src/view/types.rs @@ -27,7 +27,7 @@ pub fn unused_view_name<'a>(existing: impl IntoIterator) -> Stri .expect("a name is free below usize::MAX, there being fewer views") } -#[derive(Debug, Clone)] +#[derive(Clone)] pub struct View { pub name: String, /// Axis assignment for each category @@ -44,6 +44,38 @@ pub struct View { pub prune_empty: bool, } +/// Sorted wherever the storage is a `HashMap`, so two equal views print alike; +/// destructured, so a new field is a compile error here (improvise-djcn). +impl std::fmt::Debug for View { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + use std::collections::{BTreeMap, BTreeSet}; + let View { + name, + category_axes, + page_selections, + hidden_items, + collapsed_groups, + number_format, + prune_empty, + } = self; + let sorted = |m: &HashMap>| -> BTreeMap> { + m.iter() + .map(|(k, v)| (k.clone(), v.iter().cloned().collect())) + .collect() + }; + let pages: BTreeMap<_, _> = page_selections.iter().collect(); + f.debug_struct("View") + .field("name", name) + .field("category_axes", category_axes) + .field("page_selections", &pages) + .field("hidden_items", &sorted(hidden_items)) + .field("collapsed_groups", &sorted(collapsed_groups)) + .field("number_format", number_format) + .field("prune_empty", prune_empty) + .finish() + } +} + impl View { /// True for the on-demand scratch views, rebuilt rather than persisted. pub fn is_virtual(&self) -> bool { From 47bab86a50f7c04327cd875806feee73f30becff Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 08:47:21 -0700 Subject: [PATCH 10/15] Document the canonical Debug fingerprint and its evidence (improvise-djcn) Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/formula-and-io.md | 24 ++++++++++++++++++++++++ context/test-evidence/command.md | 22 ++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/context/code/formula-and-io.md b/context/code/formula-and-io.md index eacb06f1..8b6e52ee 100644 --- a/context/code/formula-and-io.md +++ b/context/code/formula-and-io.md @@ -512,6 +512,30 @@ change. `recomputing_formulas_leaves_the_model_fingerprint_alone` pins this. `view_only_effects_leave_shared_state_alone` now runs over a formula-bearing fixture, for the same reason. +**That fix was canonical within one instance, not across two (improvise-djcn).** +It fixed `Model`'s own maps but not the maps inside its fields. `DataStore` has +`index`, `records` and the symbol table, and `View` has `page_selections`, +`hidden_items` and `collapsed_groups`. All still printed through a derived +`Debug`, in hash order. Each `HashMap` gets its own `RandomState`, so two equal +documents built separately printed differently in 32 of 32 runs. +- The effect-classification tests compare one instance with itself before and + after, so they were unaffected. +- `a_model_command_lands_where_its_keypress_lands` compares **two** `App`s. It + passed only because its fixture had no cells and no multi-entry view maps. + +Now: +- `DataStore`, `SymbolTable` and `View` have hand-written `Debug` impls. Each + sorts wherever its storage is a hash map. +- `DataStore` resolves symbols to their strings. A `Symbol`'s number is interning + order, not content, so printing it would make two equal stores differ again. +- `SymbolTable` prints its interned strings, sorted. +- `cells` keeps its `IndexMap` order, because that order is part of the document + (gotcha 18). +- Every hand-written `Debug`, `Model`'s included, **destructures** `self`. A field + added later is then a compile error there, where a `debug_struct` that names + fields one by one would skip it silently. The pc3b version of `Model`'s impl + had exactly that hole. + `parse_number_cell` returns `CellValue::Error` naming the offending text rather than a fabricated `0.0`. No token the current grammar admits actually fails `f64::from_str` (overflow saturates to ±inf), but if the two ever drift the diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 4f0bae52..27aa9ffe 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -205,6 +205,28 @@ Row, swapping the axes and rotating Row→Column land on the same view, and an equivalence test that cannot distinguish the wrong effect from the right one proves nothing (`doctrine(check_the_checker)`). +**The fixture needs cells and multi-entry view maps too (improvise-djcn).** The +test compares `format!("{:?}", workbook)` across **two** `App`s. It passed only +because the fixture had no cells and every view map had at most one entry, and a +`HashMap` of one entry has only one order. The fixture now carries: +- three cells; +- two page selections; +- two categories with hidden items, one of them with two; +- two categories with collapsed groups. + +That fixture was red in 3 of 3 runs on the old `Debug` impls: the by-key and +by-command fingerprints differed only in hash order. It was green after the fix. + +Each half of the fix was checked by restoring the derived `Debug` on its own: +- `View`'s derived `Debug` alone: 3 of 3 runs red. +- `DataStore` and `SymbolTable`'s derived `Debug` alone: 3 of 3 runs red. + +So neither half is carried by the other. The exhaustive destructure in `Model`'s +`Debug` has no test of its own, because it is a compile-time guard. It was +checked by adding a `probe_field` to `Model` and to `Model::new`. The build then +failed with `E0027: pattern does not mention field probe_field` at the `Debug` +impl, and nowhere else. + ### a_stale_wire_model_command_cannot_be_applied A wire `Command::Model` cannot reach the document without its basis being checked From 3a4b0eac6901fdc3d9c7b137df804f3a3165161b Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 08:49:20 -0700 Subject: [PATCH 11/15] Keep the fixture's doc to two lines (improvise-djcn) Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- crates/improvise-command/src/ui/reduce.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/crates/improvise-command/src/ui/reduce.rs b/crates/improvise-command/src/ui/reduce.rs index 85d215c1..cdfa5770 100644 --- a/crates/improvise-command/src/ui/reduce.rs +++ b/crates/improvise-command/src/ui/reduce.rs @@ -918,10 +918,8 @@ pub(crate) mod tests { /// to the same mutation lands, `Dirty` included. #[test] fn a_model_command_lands_where_its_keypress_lands() { - /// Two categories on different axes: a one-category fixture cannot tell - /// a transpose from a cycle (`doctrine(check_the_checker)`). - /// Cells and multi-entry view maps too: two `App`s are compared, so every - /// `HashMap` the fingerprint walks must hold 2+ entries (improvise-djcn). + /// Two categories on two axes, plus cells and 2+-entry view maps: why each + /// is needed is in `context/test-evidence/command.md` (improvise-djcn). fn pivoted() -> App { use crate::model::cell::{CellKey, CellValue}; let mut app = app(); From 35013b6bda680179f6f26ff3c3dc35641c454184 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 08:52:26 -0700 Subject: [PATCH 12/15] Evidence, docs and the bundle surface for the mouse editor's seed and first edit (improvise-ugj4, improvise-jibp) Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/clients.md | 17 ++++++--- context/code/protocol.md | 22 ++++++----- context/test-evidence/clients.md | 49 ++++++++++++++++++++++--- crates/improvise-wasm-client/src/lib.rs | 22 +++++++++-- web/tests/bundle/wasm_surface.test.js | 4 ++ 5 files changed, 90 insertions(+), 24 deletions(-) diff --git a/context/code/clients.md b/context/code/clients.md index b4062cca..c7f06583 100644 --- a/context/code/clients.md +++ b/context/code/clients.md @@ -1573,8 +1573,10 @@ Four smaller decisions that would be re-derived wrongly: Since improvise-n6t8 it shares `#bottom-dock` with the toast, which stacks **above** it, so the prompt is neither covered nor displaced. -`RenderCell::edit_text` is the stored value, not the formatted one: seeding an -editor from `text` and committing turns `1200` into the string `"1,200"`. The +An editor is seeded from `edit_seed(target)`, the stored value the target +captured, never from `text`: seeding from the formatted text and committing +turns `1200` into the string `"1,200"`. `RenderCell` carries no `edit_text` +since improvise-ugj4, because a paint can be stale while an editor is open. The `numeric`/`error` flags are the server's answers, echoed — the client must not re-derive them from the text, which is how the two ends drift. @@ -1617,7 +1619,7 @@ The browser half of improvise-cr3, kept as thin as it can be made, now talking to either of two peers (improvise-djm). **The boundary is JSON strings in and out, so this file holds no protocol knowledge at all**: it never inspects a `Command`, never builds a `CellId`, never decides whether a cell is editable or -whether a typed string is a number. Five calls are the whole harness: +whether a typed string is a number. These calls are the whole harness: | call | what it is | |---|---| @@ -1625,6 +1627,8 @@ whether a typed string is a number. Five calls are the whole harness: | `client.on_frame(json)` | fold one frame, in arrival order | | `client.render_model()` | everything to paint, already decided | | `client.edit_target(r, c)` | what an editor opened here edits (the record, the version seen, and the seed it opened on): an opaque string the page holds, or `null` | +| `client.edit_seed(target)` | the text the target's editor opens on: the seed it captured, never the paint (improvise-ugj4) | +| `client.edited_target(target)` | the same target marked edited, so retyping the seed still writes (improvise-jibp) | | `client.typed_cell(target, s)` | the frame for "the user typed s into that cell", by identity (improvise-2wu3), or none if `s` is still the target's seed (improvise-ixxx) | | `client.on_key_event(k,c,a,s,m)` | the frame for a keypress, folded locally first where this client may answer it | @@ -1832,9 +1836,10 @@ Neither arm leaves one armed. - **`editable` is the server's answer relayed through Rust**: a stored cell. An aggregate is a drill target and drill is not on the wire yet, so it is simply not offered. -- The inline editor seeds from `RenderCell::edit_text` — the STORED value, not - the drawn one: `td.textContent` has been through the number format, so a stored - 1200 reads `"1,200"` and committing that stores the *string* `"1,200"`. Enter +- The inline editor seeds from `client.edit_seed(target)` — the STORED value the + target captured, not the drawn one: `td.textContent` has been through the number + format, so a stored 1200 reads `"1,200"` and committing that stores the *string* + `"1,200"`. Enter removes the focused ``, which fires `blur`, which calls `close` again: harmless, but only by accident. - **The title bar is three fields and one joining.** `doc_name`, `doc_dirty` and diff --git a/context/code/protocol.md b/context/code/protocol.md index 97cdab54..d00dcb36 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -699,7 +699,9 @@ for that, beside `typed()`. Every commit path reaches it: - **Browser mouse.** The seed is a field of #188's `EditTarget` (`opened`), captured by `edit_target_at` from the cache's `edit_text` beside the record and its version. `typed_target_frame` starts with the `untouched` check, and - the page passes no seed (`typed_cell(target, text)`). + the page passes no seed (`typed_cell(target, text)`). The `` shows + `edit_seed(target)`, so what the page shows and what it compares against are + one capture (improvise-ugj4). **The seed is captured at open, never recomputed at commit.** Where the editor opened is where `OpenedEditor` is written. For the TUI that is `EditOrDrill` @@ -742,14 +744,16 @@ deliberately do **not** mark it: The comparison stays exact: whitespace is content, and nothing trims. -**The mouse editor tracks no `touched`.** Its edits happen in a DOM `` -that Rust never sees, so on that path `untouched` compares content alone, and -the 2054 case survives there (improvise-jibp). The fix belongs with -`EditTarget`, for example an `input` listener that reports the first edit. A -second gap sits on the same path. The `` is still seeded from the -**painted** `editText`, while `EditTarget` captures the **cache**. So a paint -that went stale while another editor was open shows old text against a newer -seed and version (improvise-ugj4). +**The mouse editor reports its first edit.** Its edits happen in a DOM +`` that Rust never sees, so the page's `input` listener, registered +`{ once: true }`, swaps its target for `edited_target(target)`, which is +`EditTarget::edited`: the same capture with `touched` set (improvise-jibp). +Before that, the mouse path compared content alone, so the 2054 case survived +there. The `` used to be seeded from the **painted** `editText` while +`EditTarget` captured the **cache**. `repaint` returns early while an editor is +open, so a paint that went stale then showed old text against a newer seed and +version (improvise-ugj4). The page now seeds from `edit_seed(target)`, and +`RenderCell` no longer carries `edit_text`, so the paint cannot be asked. This does **not** change classification. `007` typed fresh is still `Number(7)`, and `a_touched_browser_commit_still_writes` pins that. diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 8d718569..d206d459 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -4933,11 +4933,50 @@ sends no write, and a touched one does". It wraps `Worker.prototype.postMessage` opens the editor on cell 4, and presses `Enter`. No `SetCell` may be posted. Then it types `1` and presses `Enter`, and exactly one must be. -**Known gap, filed as improvise-ugj4.** The `` is still seeded from the -painted `editText`, while `EditTarget` captures the cache. A paint made stale -while another editor was open (`repaint` returns early while `editing` is set) -therefore shows old text against a newer seed and version. -**improvise-jibp** is the other residue: the mouse editor has no `touched` flag. +Both gaps this entry used to list are closed: the stale-paint seed +(improvise-ugj4) and the missing `touched` flag (improvise-jibp). See the +entries that follow. + +### `a_mouse_target_marked_edited_writes_its_own_seed` + +**improvise-jibp**, the Rust half. A target on the seed `"1200"` commits +`"1200"` and composes nothing: that is the control. `edited()` must leave the +seed where it was, and the same text committed through the edited target must +compose a write. **Rival:** `edited()` leaving `touched` false fails this test +and `the_page_opens_its_editor_on_what_the_target_captured`, and nothing else. + +### `the_page_opens_its_editor_on_what_the_target_captured` + +**improvise-ugj4 and improvise-jibp**, through the wasm-bindgen surface the +page calls. The cell is drawn `"1,200.50"` and stored `"1200.5"`, and +`edit_seed` must return `"1200.5"`. A projected write then moves the cell to +`"777"`. A target taken afterwards seeds `"777"`, and the first target still +seeds `"1200.5"`, because a target is one capture. `typed_cell(later, "777")` +composes nothing, and the same text through `edited_target(later)` composes a +write. **Rival:** as above, `edited()` leaving `touched` false fails here. I +wrote no rival for an `edit_seed` that re-reads the cache. `EditTarget` names a +record, not a position, so that rival would need a lookup the client does not +have. + +### a mouse editor typed back to its own seed still writes — `harness/tests/contract.spec.js` + +**improvise-jibp**, the page half. The editor opens on cell 4, and the test +types the seed over the selection and presses Enter. Exactly one `SetCell` must +be posted. Before the fix the page sent `typed_cell(target, seed)` with an +untouched target, which composes nothing. **Rival:** delete the page's `input` +listener. This test fails, and the rest of `contract.spec.js` passes (16). + +### an editor opened after an unpainted write shows and commits what it captured — `harness/tests/contract.spec.js` + +**improvise-ugj4.** The test opens an editor on cell 0, so `repaint` stops. +While it is open, it delivers a synthetic frame that moves cell 4 to `777`. It +builds that frame from the last `Cells` entry the page actually received for +cell 4, changing only `text`, `edit_text` and `value`, so the record and the +version are real. Then it opens cell 4. The `` must show `777`, and +Enter with no edit must post no `SetCell`. **Rival:** seed the `` from the +paint (`drawn.replaceAll(",", "")`). This test fails on the value, and so does +"an untouched mouse editor sends no write, and a touched one does", because the +drawn text is not the seed. The other 15 pass. ### `a_browser_editor_typed_back_to_its_seed_still_writes` diff --git a/crates/improvise-wasm-client/src/lib.rs b/crates/improvise-wasm-client/src/lib.rs index e1cafc33..cd5dca15 100644 --- a/crates/improvise-wasm-client/src/lib.rs +++ b/crates/improvise-wasm-client/src/lib.rs @@ -313,11 +313,25 @@ mod tests { ], }); let first = client.edit_target(0, 0).unwrap().expect("a record cell"); - assert_eq!(client.edit_seed(&first).unwrap(), "1200.5", "not the drawn text"); - client.fold_frame(ServerFrame::Commands(vec![Command::Project(at("777", "777"))])); + assert_eq!( + client.edit_seed(&first).unwrap(), + "1200.5", + "not the drawn text" + ); + client.fold_frame(ServerFrame::Commands(vec![Command::Project(at( + "777", "777", + ))])); let later = client.edit_target(0, 0).unwrap().expect("a record cell"); - assert_eq!(client.edit_seed(&later).unwrap(), "777", "not the cache at open"); - assert_eq!(client.edit_seed(&first).unwrap(), "1200.5", "a target is one capture"); + assert_eq!( + client.edit_seed(&later).unwrap(), + "777", + "not the cache at open" + ); + assert_eq!( + client.edit_seed(&first).unwrap(), + "1200.5", + "a target is one capture" + ); assert!(client.typed_cell(&later, "777").unwrap().is_none()); let edited = client.edited_target(&later).unwrap(); diff --git a/web/tests/bundle/wasm_surface.test.js b/web/tests/bundle/wasm_surface.test.js index babcd2ca..a9f8550f 100644 --- a/web/tests/bundle/wasm_surface.test.js +++ b/web/tests/bundle/wasm_surface.test.js @@ -80,9 +80,13 @@ test("the page still reaches the client through a handful of calls", () => { "click_page_item", "click_tile", "click_view", + // improvise-ugj4: the editor shows the seed its target captured, not the paint. + "edit_seed", // improvise-2wu3/0j0f: what an editor edits, captured when it opens, so // its commit names that record at that version and not a position. "edit_target", + // improvise-jibp: the first edit of the marks the target touched. + "edited_target", "on_frame", "on_key_event", "render_model", From 4e52d06838c339dbd7621bb64aab586db1592293 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 08:54:15 -0700 Subject: [PATCH 13/15] WIP red: four ways of shipping a document the .gitignore walk misses (improvise-c0aj) --- tests/shipped_documents_load.rs | 79 +++++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/tests/shipped_documents_load.rs b/tests/shipped_documents_load.rs index deb725e4..e8db9d6c 100644 --- a/tests/shipped_documents_load.rs +++ b/tests/shipped_documents_load.rs @@ -30,6 +30,14 @@ fn documents_in(dir: &Path) -> Vec { found } +/// Every shipped document under `root`: the walk the test below loads. +fn shipped_documents(root: &Path) -> Vec { + shipped_directories(root) + .iter() + .flat_map(|dir| documents_in(dir)) + .collect() +} + #[test] fn every_shipped_document_loads() { let root = Path::new(env!("CARGO_MANIFEST_DIR")); @@ -60,3 +68,74 @@ fn every_shipped_document_loads() { } } } + +/// A scratch repository, so each way of shipping a document is set up for real. +fn scratch_repo() -> tempfile::TempDir { + let dir = tempfile::tempdir().expect("a temp dir"); + run_git(dir.path(), &["init", "-q"]); + dir +} + +fn run_git(root: &Path, args: &[&str]) { + let status = std::process::Command::new("git") + .arg("-C") + .arg(root) + .args(args) + .status() + .expect("git runs"); + assert!(status.success(), "git {args:?} failed"); +} + +fn write(root: &Path, rel: &str, text: &str) { + let path = root.join(rel); + std::fs::create_dir_all(path.parent().expect("a parent")).expect("dirs"); + std::fs::write(path, text).expect("writes"); +} + +fn walked(root: &Path, rel: &str) -> bool { + shipped_documents(root).contains(&root.join(rel)) +} + +/// improvise-c0aj: one test per way of shipping a document that reading the +/// root `.gitignore` missed. Evidence: `context/test-evidence/tui.md`. +#[test] +fn a_single_file_un_ignore_is_walked() { + let repo = scratch_repo(); + let root = repo.path(); + write(root, ".gitignore", "*.improv\n!web/demo.improv\n"); + write(root, "web/demo.improv", "x"); + run_git(root, &["add", ".gitignore", "web/demo.improv"]); + assert!(walked(root, "web/demo.improv")); +} + +#[test] +fn a_gzipped_document_is_walked() { + let repo = scratch_repo(); + let root = repo.path(); + write(root, ".gitignore", "*.improv\n"); + write(root, "docs/model.improv.gz", "x"); + run_git(root, &["add", ".gitignore", "docs/model.improv.gz"]); + assert!(walked(root, "docs/model.improv.gz")); +} + +#[test] +fn a_document_un_ignored_by_a_nested_gitignore_is_walked() { + let repo = scratch_repo(); + let root = repo.path(); + write(root, ".gitignore", "*.improv\n"); + write(root, "harness/.gitignore", "!keep.improv\n"); + write(root, "harness/keep.improv", "x"); + run_git(root, &["add", ".gitignore", "harness/.gitignore", "harness/keep.improv"]); + assert!(walked(root, "harness/keep.improv")); +} + +#[test] +fn a_force_added_document_is_walked() { + let repo = scratch_repo(); + let root = repo.path(); + write(root, ".gitignore", "*.improv\n"); + write(root, "secret/forced.improv", "x"); + run_git(root, &["add", ".gitignore"]); + run_git(root, &["add", "-f", "secret/forced.improv"]); + assert!(walked(root, "secret/forced.improv")); +} From 6dceaad1c726c1d06651e2a191d464bea0988aba Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 08:56:26 -0700 Subject: [PATCH 14/15] every_shipped_document_loads walks the index, not a .gitignore reading (improvise-c0aj) --- context/test-evidence/tui.md | 73 +++++++++++++++-------- tests/shipped_documents_load.rs | 102 ++++++++++++++++---------------- 2 files changed, 97 insertions(+), 78 deletions(-) diff --git a/context/test-evidence/tui.md b/context/test-evidence/tui.md index 48c7165e..4318bf1f 100644 --- a/context/test-evidence/tui.md +++ b/context/test-evidence/tui.md @@ -426,38 +426,59 @@ generator-driven proptests in `improvise-io`. What *those* cannot see is a checked-in document drifting out of the format, because they generate their inputs. This test is aimed at exactly that gap. -**It is deliberately a directory walk rather than a list of names**: a list is a +**It walks the files the repository tracks, not a list of names**: a list is a second place to remember, and the file that gets forgotten is the one nobody -added to it. The scope is the directories `.gitignore` un-ignores `*.improv` -in. Everything else matching `*.improv` is somebody's working file. +added to it. "Shipped" means "tracked", and `shipped_documents` reads it off +the index: every `*.improv` and `*.improv.gz` path that `ls-files` lists. It does +not include untracked files, so a working `.improv.gz` in someone's tree cannot +fail their run. A document not yet added is walked once it is, which is the +state CI tests. -**The directory list is read from `.gitignore`, not written in the test -(improvise-qlrh).** The test used to hard-code `["examples", "web"]`, a list of -exactly the kind the paragraph above warns against. When improvise-plcx -un-ignored `harness/fixtures/*.improv`, nothing added that directory to the -walk. Its two fixtures were then loaded only by the Playwright job, where a -missing seed degrades to an empty model and surfaces as a missing-label -failure, not a load error. `shipped_directories` now takes every -`!/*.improv` line, so un-ignoring a directory is what enlists it. +**How the walk got here: two steps, each closing the previous one's holes.** -The precondition is per directory: each derived directory must yield at least -one document. The old whole-walk `>= 2` could not see a directory silently lost. +- **improvise-qlrh.** The test hard-coded `["examples", "web"]`. When + improvise-plcx un-ignored `harness/fixtures/*.improv`, nothing added that + directory. Its two fixtures were then loaded only by the Playwright job, + where a missing seed degrades to an empty model and fails as a missing label, + not a load error. The fix read `!/*.improv` lines from the root + `.gitignore`, with a per-directory precondition. Red at `705c92d`: + `harness/fixtures/tall.improv` replaced with `{"legacy": true}` passed the old + test. +- **improvise-c0aj.** That `.gitignore` reading was a parser for one shape of + one file, so four other ways of shipping a document were missed silently: + - a single-file un-ignore (`!web/demo.improv`); + - a `.improv.gz` anywhere, since `*.improv` never ignores it and so no + un-ignore line names it; + - a `!` line in a nested `.gitignore`; + - a force-added (`add -f`) document. -**Red, measured at `705c92d`:** `harness/fixtures/tall.improv` was replaced with -`{"legacy": true}`, and the old test still passed. **Rivals, each run against -the new test:** + Each now has its own test, which builds the shape in a scratch repository + and asserts the walk lists the document. **All four were red** against the + `.gitignore` walk, each failing at its `walked` assertion with its setup + commands succeeding. Walking the index closes all four by construction and + deletes the parser, because git has already answered what the parser + approximated. -- **That same broken fixture.** Fails with "…/harness/fixtures/tall.improv does - not load". -- **A walk that drops every `harness` path.** Fails the per-directory - precondition, naming `harness/fixtures`. -- **A derivation that matches no `.gitignore` line.** Fails "…must un-ignore at - least one *.improv directory". +**The one new failure mode, answered: no index.** `ls-files` needs a checkout. +In CI (`actions/checkout`) and every worktree it has one. No nix build or +packaging step runs this test; the flake is a dev shell only. Where there is no +index, the walk must not answer "nothing", because an empty walk passes every +per-document assertion vacuously. So it asserts `ls-files` succeeded, and +`a_directory_that_is_not_a_checkout_fails_loudly` pins that. The whole-walk +precondition "at least one document" stays as the second line of defence. -**It matches on the full name, not on `Path::extension()`**: `.improv.gz` is the -same document gzipped and `extension()` calls it `gz`, so matching on the -extension alone would skip it and put back the very hole this test exists to -close. +**Rivals, each run against the new walk:** + +- **The `.improv.gz` pathspec removed.** Fails `a_gzipped_document_is_walked`, + and nothing else. +- **A failed `ls-files` answered with an empty list instead of a panic.** Fails + `a_directory_that_is_not_a_checkout_fails_loudly`. +- **`harness/fixtures/tall.improv` replaced with `{"legacy": true}`.** Fails + `every_shipped_document_loads`, naming the file. + +**It matches on the full name, not on `Path::extension()`.** `.improv.gz` is the +same document gzipped, and `extension()` calls it `gz`. The pathspec lists both +suffixes for the same reason. --- diff --git a/tests/shipped_documents_load.rs b/tests/shipped_documents_load.rs index e8db9d6c..777b1b1b 100644 --- a/tests/shipped_documents_load.rs +++ b/tests/shipped_documents_load.rs @@ -1,71 +1,52 @@ //! Every `.improv` document this repository ships must load (improvise-odbc). -//! Why a DIRECTORY WALK and not a list: `context/test-evidence/tui.md`. +//! "Shipped" is "tracked", read off git's index: `context/test-evidence/tui.md`. use std::path::{Path, PathBuf}; -/// The directories `.gitignore` un-ignores `*.improv` in: the definition of -/// "shipped", read from the one place that states it (improvise-qlrh). -fn shipped_directories(root: &Path) -> Vec { - let gitignore = std::fs::read_to_string(root.join(".gitignore")).expect("a .gitignore"); - gitignore - .lines() - .filter_map(|line| line.trim().strip_prefix('!')) - .filter_map(|pattern| pattern.strip_suffix("/*.improv")) - .map(|dir| root.join(dir)) - .collect() -} - -fn documents_in(dir: &Path) -> Vec { - let mut found: Vec = std::fs::read_dir(dir) - .expect("a shipped-document directory") - .map(|entry| entry.expect("a readable directory entry").path()) - // `extension()` of `x.improv.gz` is `gz`, so matching on the extension - // alone would skip it and put back the hole this test closes. - .filter(|p| { - p.to_str() - .is_some_and(|s| s.ends_with(".improv") || s.ends_with(".improv.gz")) - }) +/// Every tracked `.improv` and `.improv.gz` under `root`. Outside a checkout this +/// panics rather than answering empty (improvise-c0aj). +fn shipped_documents(root: &Path) -> Vec { + let out = std::process::Command::new("git") + .arg("-C") + .arg(root) + .args(["ls-files", "-z", "--", "*.improv", "*.improv.gz"]) + .output() + .expect("git runs: this test reads the index, so it needs a checkout"); + assert!( + out.status.success(), + "git ls-files failed in {}: {}", + root.display(), + String::from_utf8_lossy(&out.stderr) + ); + let mut found: Vec = String::from_utf8(out.stdout) + .expect("UTF-8 paths") + .split('\0') + .filter(|p| !p.is_empty()) + .map(|p| root.join(p)) .collect(); found.sort(); found } -/// Every shipped document under `root`: the walk the test below loads. -fn shipped_documents(root: &Path) -> Vec { - shipped_directories(root) - .iter() - .flat_map(|dir| documents_in(dir)) - .collect() -} - #[test] fn every_shipped_document_loads() { let root = Path::new(env!("CARGO_MANIFEST_DIR")); - let dirs = shipped_directories(root); + let documents = shipped_documents(root); assert!( - !dirs.is_empty(), - "precondition: .gitignore must un-ignore at least one *.improv directory" + !documents.is_empty(), + "precondition: the index must list at least one shipped document" ); - for dir in dirs { - let documents = documents_in(&dir); - // Per directory, so a walk that silently loses one cannot pass (qlrh). + for path in documents { + let workbook = improvise::persistence::load(&path) + .unwrap_or_else(|e| panic!("{} does not load: {e}", path.display())); assert!( - !documents.is_empty(), - "precondition: {} is un-ignored for *.improv but the walk found none there", - dir.display() + workbook.views.contains_key(&workbook.active_view), + "{} loads but its initial view {:?} is not one of {:?}", + path.display(), + workbook.active_view, + workbook.views.keys().collect::>() ); - for path in documents { - let workbook = improvise::persistence::load(&path) - .unwrap_or_else(|e| panic!("{} does not load: {e}", path.display())); - assert!( - workbook.views.contains_key(&workbook.active_view), - "{} loads but its initial view {:?} is not one of {:?}", - path.display(), - workbook.active_view, - workbook.views.keys().collect::>() - ); - } } } @@ -125,10 +106,27 @@ fn a_document_un_ignored_by_a_nested_gitignore_is_walked() { write(root, ".gitignore", "*.improv\n"); write(root, "harness/.gitignore", "!keep.improv\n"); write(root, "harness/keep.improv", "x"); - run_git(root, &["add", ".gitignore", "harness/.gitignore", "harness/keep.improv"]); + run_git( + root, + &[ + "add", + ".gitignore", + "harness/.gitignore", + "harness/keep.improv", + ], + ); assert!(walked(root, "harness/keep.improv")); } +/// An empty walk would pass every per-document assertion vacuously, so a +/// directory git cannot read must stop the test instead. +#[test] +#[should_panic(expected = "git ls-files failed")] +fn a_directory_that_is_not_a_checkout_fails_loudly() { + let not_a_repo = tempfile::tempdir().expect("a temp dir"); + shipped_documents(not_a_repo.path()); +} + #[test] fn a_force_added_document_is_walked() { let repo = scratch_repo(); From 228f130e612bf8eb75122ad716125641bba6fd4a Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 08:58:41 -0700 Subject: [PATCH 15/15] evidence: an entry for the shape test that cites it (improvise-c0aj) --- context/test-evidence/tui.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/context/test-evidence/tui.md b/context/test-evidence/tui.md index 4318bf1f..cfda95cd 100644 --- a/context/test-evidence/tui.md +++ b/context/test-evidence/tui.md @@ -480,6 +480,16 @@ precondition "at least one document" stays as the second line of defence. same document gzipped, and `extension()` calls it `gz`. The pathspec lists both suffixes for the same reason. +### a_single_file_un_ignore_is_walked + +improvise-c0aj. This test is the first of four shape tests. Its siblings are +`a_gzipped_document_is_walked`, +`a_document_un_ignored_by_a_nested_gitignore_is_walked` and +`a_force_added_document_is_walked`. Each builds one way of shipping a document +in a scratch repository and asserts the walk lists it. All four were red +against the `.gitignore` walk. The account, including the rivals, is under +`every_shipped_document_loads` above. + --- ## `tests/web_demo_model.rs`