From ea1f2d313b7759fe47f4b0cc6c8fb25889b4c080 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:43:10 -0700 Subject: [PATCH 1/7] improvise-lrrx/8nd9: view history does not survive a document swap install_workbook clears view_back_stack and view_forward_stack beside cursors, so `<` after LoadModel or AcceptWizardModel no longer walks the outgoing document's views. ViewBack and ViewForward now observe the switch as SwitchView does (improvise-swzk): a frame the document refuses is consumed, leaves no frame behind, and restores no mode. Each fix holds without the other. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/command.md | 22 ++- context/test-evidence/command.md | 39 +++++ crates/improvise-command/src/ui/app.rs | 3 + crates/improvise-command/src/ui/effect.rs | 179 ++++++++++++++++++++-- 4 files changed, 221 insertions(+), 22 deletions(-) diff --git a/context/code/command.md b/context/code/command.md index 987df348..fc111c79 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -721,12 +721,22 @@ is gone rather than joined by a second one. A guard spelled here as copy of `switch_to_view`'s own condition, free to disagree with it the day that condition grows a clause. -Not fixed, and reachable only by construction: `ViewBack` and `ViewForward` call -`switch_to_view` just as unguardedly, pushing the current view onto the opposite -stack before the switch they cannot be sure of. They are safe **because -`ViewState::forget_view` purges both stacks when a view is deleted**, so a popped -frame always names a live view — a guard living one level away from the hazard, -which is the shape that keeps returning here. +`ViewBack` and `ViewForward` have the same shape now (improvise-8nd9). They used to +push the current view onto the opposite stack **before** a switch they could not be +sure of, and were safe only because `ViewState::forget_view` purges both stacks when +a view is deleted — a guard living one level away from the hazard. improvise-lrrx +proved the guard incomplete: `App::install_workbook` (`LoadModel`, +`AcceptWizardModel`) cleared `cursors` and not the history, so `<` after a load +walked the **outgoing** document's frames, landing on a view of the same name in the +incoming one that this client had never visited, or on a refused switch that still +left a phantom frame behind. Both halves are fixed, so neither relies on the other: +`install_workbook` clears both stacks beside `cursors`, for the same reason, and the +two steps go through `stepped_to`, which switches, observes whether `active_view` +moved, and only then restores the frame's mode and hands back the frame just left. +A frame the document refuses is consumed and leaves nothing. `reduce_view`'s +`stepped` is unchanged: the client holds no `Workbook` to observe against, and its +stacks cannot hold a dead name — `ForgetView` purges them and a load is a fresh +`Snapshot`. ### `ReturnToNormalUnlessMoved` — observation, not prediction (improvise-toc) diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 511a8461..bbf5ac27 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5021,3 +5021,42 @@ bracket-matched `#[…]` before reading the name. `an_attributed_or_wrapped_effect_is_still_an_effect` pins that on a fixture: a doc comment, two attributes (one with nested brackets), and a wrapped call. With the whitespace skip removed it fails with `[]`. + +### loading_a_document_forgets_the_outgoing_documents_view_history + +improvise-lrrx: `App::install_workbook` cleared `cursors` but not +`view_back_stack`/`view_forward_stack`, so `<` after `LoadModel` walked the +outgoing document's views inside the incoming one. The history is two deep +(`Default`, `X`, then on `Z`), and the incoming document **has** a view named `X` +but opens on `Y`. Without that, a stale frame is a refused switch and looks the +same as "nothing happened" (the ticket's own fixture warning). Against the unfixed +code, `<` landed on `X`, a view this client never visited in the new document. +Rival injected: the two `clear()` lines removed from `install_workbook`, with the +improvise-8nd9 fix still in place. This test and its wizard twin failed at the +`view-back walked the outgoing document's history` assertion. + +### accepting_an_import_forgets_the_outgoing_documents_view_history + +improvise-lrrx, through the second door into `install_workbook`. A +wizard-built workbook has only `Default`, so the test creates an `X` in it after +accepting. Without that, the stale `X` frame would be refused and could not be +told apart from a cleared history. Same rival and same failure as the load test. + +### a_step_back_the_document_refuses_leaves_no_frame_behind + +improvise-8nd9: `ViewBack` pushed the current view onto the forward stack before +the guarded `switch_to_view`, so a frame naming a view the document lacks left a +phantom behind, and `,` would then "go forward" to where it already was. The +dead frame is pushed by hand, standing for any path that forgets +`forget_view`, with a live frame under it and the frame's mode set to `Help` so +that a restored mode is observable. Two rivals. First, the original code, which +pushes first and restores the mode unconditionally: it fails at `nothing moved, +so no mode`. Second, a push-first version that restores the mode only on a move, +so the mode assertion cannot hide the stack assertion: it fails at `a refused +step left X on the forward stack`. The last two assertions check that the live +frame beneath still works, so a fix that drops the whole stack cannot pass. + +### a_step_forward_the_document_refuses_leaves_no_frame_behind + +The `ViewForward` half of the test above, with the stacks' roles swapped. It fails +against the unfixed code at `nothing moved, so no mode`. diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index 56f6fb8c..bf5151de 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -439,6 +439,9 @@ impl App { pub fn install_workbook(&mut self, workbook: Workbook) { self.view_state.active_view = workbook.active_view.clone(); self.view_state.cursors.clear(); + // The history names the outgoing document's views too (improvise-lrrx). + self.view_state.view_back_stack.clear(); + self.view_state.view_forward_stack.clear(); self.model_state.workbook = workbook; // Every client's mirror is scoped to the outgoing document too, and an // effect cannot reach the session list. The generation is how it hears. diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index 62a4f6a0..494fedc1 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -354,19 +354,31 @@ impl Effect for SwitchView { } } +/// Switch to `frame`'s view and restore its mode, returning the frame just left +/// — or `None`, touching nothing, if the view did not move. +fn stepped_to(app: &mut App, frame: ViewFrame) -> Option { + let left = ViewFrame { + view_name: app.view_state.active_view.clone(), + mode: app.view_state.mode.clone(), + }; + app.switch_to_view(&frame.view_name); + (app.view_state.active_view != left.view_name).then(|| { + app.view_state.mode = frame.mode; + left + }) +} + /// Go back in view history (pop back stack, push current to forward stack). #[derive(Debug)] pub struct ViewBack; impl Effect for ViewBack { fn apply(&self, app: &mut App) { - if let Some(frame) = app.view_state.view_back_stack.pop() { - let current = app.view_state.active_view.clone(); - app.view_state.view_forward_stack.push(ViewFrame { - view_name: current, - mode: app.view_state.mode.clone(), - }); - app.switch_to_view(&frame.view_name); - app.view_state.mode = frame.mode; + // Observed, as `SwitchView` is: a frame the document refuses is dropped + // and leaves nothing behind (improvise-8nd9, `context/code/command.md`). + if let Some(frame) = app.view_state.view_back_stack.pop() + && let Some(left) = stepped_to(app, frame) + { + app.view_state.view_forward_stack.push(left); } } fn kind(&self) -> EffectKind { @@ -379,14 +391,11 @@ impl Effect for ViewBack { pub struct ViewForward; impl Effect for ViewForward { fn apply(&self, app: &mut App) { - if let Some(frame) = app.view_state.view_forward_stack.pop() { - let current = app.view_state.active_view.clone(); - app.view_state.view_back_stack.push(ViewFrame { - view_name: current, - mode: app.view_state.mode.clone(), - }); - app.switch_to_view(&frame.view_name); - app.view_state.mode = frame.mode; + // The mirror of `ViewBack`, observed the same way (improvise-8nd9). + if let Some(frame) = app.view_state.view_forward_stack.pop() + && let Some(left) = stepped_to(app, frame) + { + app.view_state.view_back_stack.push(left); } } fn kind(&self) -> EffectKind { @@ -1878,6 +1887,144 @@ mod tests { assert_eq!(app.view_state.active_view, before); } + /// A history two deep through `X` and `Z`: back is `[Default, X]`. + fn app_with_history(storage: Box) -> App { + let mut wb = Workbook::new("Outgoing"); + wb.create_view("X"); + wb.create_view("Z"); + let mut app = App::with_storage(wb, None, storage); + for name in ["X", "Z"] { + SwitchView(name.to_string()).apply(&mut app); + } + assert_eq!(app.view_state.view_back_stack.len(), 2, "setup: two deep"); + app + } + + /// Asserts `<` and `,` leave `view` put and the history empty. + fn assert_history_went_with_the_document(app: &mut App, view: &str) { + assert_eq!(app.view_state.active_view, view, "setup: opened on {view}"); + for (name, step) in [ + ("view-back", Box::new(ViewBack) as Box), + ("view-forward", Box::new(ViewForward)), + ] { + step.apply(app); + assert_eq!( + app.view_state.active_view, view, + "{name} walked the outgoing document's history in the incoming one" + ); + } + assert!(app.view_state.view_back_stack.is_empty()); + assert!(app.view_state.view_forward_stack.is_empty()); + } + + /// improvise-lrrx. Evidence: `context/test-evidence/command.md`. + #[test] + fn loading_a_document_forgets_the_outgoing_documents_view_history() { + use crate::storage::{MemoryFs, Storage}; + let fs = MemoryFs::new(); + // The incoming document HAS a view named `X`, and opens somewhere else. + let mut incoming = Workbook::new("Incoming"); + incoming.create_view("X"); + incoming.create_view("Y"); + incoming.active_view = "Y".to_string(); + let text = crate::persistence::format_md(&incoming); + fs.write(std::path::Path::new("b.improv"), text.as_bytes()) + .unwrap(); + let mut app = app_with_history(Box::new(fs)); + + LoadModel("b.improv".into()).apply(&mut app); + assert_eq!( + app.model_state.workbook.model.name, "Incoming", + "setup: loaded" + ); + assert_history_went_with_the_document(&mut app, "Y"); + } + + /// improvise-lrrx: the wizard is the other door into `install_workbook`. + #[test] + fn accepting_an_import_forgets_the_outgoing_documents_view_history() { + let mut app = app_with_history(Box::new(crate::storage::MemoryFs::new())); + let mut wizard = crate::import::wizard::ImportWizard::new(serde_json::json!([ + {"region": "East", "revenue": 100.0}, + {"region": "West", "revenue": 200.0}, + ])); + wizard.advance(); + wizard.advance(); + wizard.build(); + assert!(wizard.built_summary().is_some(), "setup: a built workbook"); + app.wizard = Some(wizard); + + AcceptWizardModel.apply(&mut app); + // The new document gains an `X` of its own, so a stale frame could land. + app.model_state.workbook.create_view("X"); + let opened = app.view_state.active_view.clone(); + assert_ne!(opened, "X", "setup: the import must not open on X"); + assert_history_went_with_the_document(&mut app, &opened); + } + + /// improvise-8nd9: a frame naming a view the document lacks is refused, and + /// a refused step leaves no frame behind. See `context/test-evidence/command.md`. + #[test] + fn a_step_back_the_document_refuses_leaves_no_frame_behind() { + let mut app = test_app(); + app.model_state.workbook.create_view("X"); + SwitchView("X".to_string()).apply(&mut app); + // A path that forgot `forget_view`: the history names a dead view. + app.view_state.view_back_stack.push(ViewFrame { + view_name: "Gone".to_string(), + mode: AppMode::Help, + }); + + ViewBack.apply(&mut app); + assert_eq!( + app.view_state.active_view, "X", + "setup: the step is refused" + ); + assert_eq!( + app.view_state.mode, + AppMode::Normal, + "nothing moved, so no mode" + ); + assert!( + app.view_state.view_forward_stack.is_empty(), + "a refused step left X on the forward stack: `,` would go nowhere" + ); + ViewBack.apply(&mut app); + assert_eq!( + app.view_state.active_view, "Default", + "the live frame survives" + ); + assert_eq!(app.view_state.view_forward_stack.len(), 1); + } + + /// improvise-8nd9: the forward half of the step above. + #[test] + fn a_step_forward_the_document_refuses_leaves_no_frame_behind() { + let mut app = test_app(); + app.model_state.workbook.create_view("X"); + SwitchView("X".to_string()).apply(&mut app); + ViewBack.apply(&mut app); + app.view_state.view_forward_stack.push(ViewFrame { + view_name: "Gone".to_string(), + mode: AppMode::Help, + }); + + ViewForward.apply(&mut app); + assert_eq!(app.view_state.active_view, "Default", "setup: refused"); + assert_eq!( + app.view_state.mode, + AppMode::Normal, + "nothing moved, so no mode" + ); + assert!( + app.view_state.view_back_stack.is_empty(), + "a refused step left Default on the back stack: `<` would go nowhere" + ); + ViewForward.apply(&mut app); + assert_eq!(app.view_state.active_view, "X", "the live frame survives"); + assert_eq!(app.view_state.view_back_stack.len(), 1); + } + #[test] fn create_and_delete_view() { let mut app = test_app(); From 99046f7c630853432f2455112ed1198208c5b546 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:45:36 -0700 Subject: [PATCH 2/7] improvise-8nd9: a history step observes acceptance, not movement A frame naming the view you are on is still a place (view + mode), and the client's reduce_view::stepped steps onto it unconditionally. Observing "active_view changed" dropped that frame on the host and so split host from client; colon_view_back_respects_whether_history_exists caught it. App::switch_to_view now says whether the document accepted the view, and stepped_to asks that rather than re-deriving the condition. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/command.md | 23 ++++++++++++----- context/test-evidence/command.md | 12 +++++++++ crates/improvise-command/src/ui/app.rs | 10 +++++--- crates/improvise-command/src/ui/effect.rs | 30 ++++++++++++++++++++--- 4 files changed, 62 insertions(+), 13 deletions(-) diff --git a/context/code/command.md b/context/code/command.md index fc111c79..847aac2a 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -731,12 +731,23 @@ walked the **outgoing** document's frames, landing on a view of the same name in incoming one that this client had never visited, or on a refused switch that still left a phantom frame behind. Both halves are fixed, so neither relies on the other: `install_workbook` clears both stacks beside `cursors`, for the same reason, and the -two steps go through `stepped_to`, which switches, observes whether `active_view` -moved, and only then restores the frame's mode and hands back the frame just left. -A frame the document refuses is consumed and leaves nothing. `reduce_view`'s -`stepped` is unchanged: the client holds no `Workbook` to observe against, and its -stacks cannot hold a dead name — `ForgetView` purges them and a load is a fresh -`Snapshot`. +two steps go through `stepped_to`, which switches, observes whether the document +**accepted** the view (`App::switch_to_view` now returns that), and only then +restores the frame's mode and hands back the frame just left. A frame the document +refuses is consumed and leaves nothing. + +The observation is *accepted*, not *moved*, and that is the one place the sibling +shapes differ. `SwitchView` asks "did `active_view` change" because asking for the +view you are on is not a move. A history frame is a **place**, meaning a view and a +mode, so a live frame naming the view you are on still restores its mode. Such +frames exist: `forget_view` collapses `[A, B, A]` to `[A, A]`. The client's +`reduce_view::stepped` also steps onto them unconditionally. A "moved" test +would therefore drop a frame the client keeps, and the host and the client +would disagree. The first version of this fix made exactly that mistake. +`colon_view_back_respects_whether_history_exists` caught it, because its +fixture is such a frame. `reduce_view`'s `stepped` is unchanged. The client holds +no `Workbook` to observe against, and its stacks cannot hold a dead name: +`ForgetView` purges them, and a load arrives as a fresh `Snapshot`. ### `ReturnToNormalUnlessMoved` — observation, not prediction (improvise-toc) diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index bbf5ac27..63bfa926 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5056,6 +5056,18 @@ so the mode assertion cannot hide the stack assertion: it fails at `a refused step left X on the forward stack`. The last two assertions check that the live frame beneath still works, so a fix that drops the whole stack cannot pass. +### a_step_back_to_the_view_you_are_on_is_the_step_the_client_folds + +improvise-8nd9. This test holds the fix to the client's `reduce_view::stepped`. +A live frame naming the view you are on (a `forget_view` collapse leaves one) +is still a step. It restores its mode and leaves a forward frame, exactly as +the client folds it. Rival injected: `stepped_to` observing whether +`active_view` *changed* rather than whether the document *accepted* the view. +That was the first version of this fix. This test failed at the +mode assertion, as did +`command::cmd::text_buffer::tests::colon_view_back_respects_whether_history_exists`. +That test's fixture is such a frame, and it is what caught the first version. + ### a_step_forward_the_document_refuses_leaves_no_frame_behind The `ViewForward` half of the test above, with the stacks' roles swapped. It fails diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index bf5151de..faaa5277 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -564,12 +564,14 @@ impl App { .flat_map(move |r| (0..layout.col_count()).filter_map(move |c| layout.cell_key(r, c))) } - /// Point **this client's** slice at `name`, if the document has such a view. - /// The document's own default stays put (improvise-jytm, gotcha 29). - pub fn switch_to_view(&mut self, name: &str) { - if self.model_state.workbook.views.contains_key(name) { + /// Point **this client's** slice at `name`, if the document has such a view, + /// and say whether it did. The document's default stays put (gotcha 29). + pub fn switch_to_view(&mut self, name: &str) -> bool { + let accepted = self.model_state.workbook.views.contains_key(name); + if accepted { self.view_state.active_view = name.to_string(); } + accepted } /// What `name` names in this document, falling back to the default for a name diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index 494fedc1..ec16e636 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -355,14 +355,13 @@ impl Effect for SwitchView { } /// Switch to `frame`'s view and restore its mode, returning the frame just left -/// — or `None`, touching nothing, if the view did not move. +/// — or `None`, touching nothing, if the document refused the view. fn stepped_to(app: &mut App, frame: ViewFrame) -> Option { let left = ViewFrame { view_name: app.view_state.active_view.clone(), mode: app.view_state.mode.clone(), }; - app.switch_to_view(&frame.view_name); - (app.view_state.active_view != left.view_name).then(|| { + app.switch_to_view(&frame.view_name).then(|| { app.view_state.mode = frame.mode; left }) @@ -1997,6 +1996,31 @@ mod tests { assert_eq!(app.view_state.view_forward_stack.len(), 1); } + /// improvise-8nd9: a live frame naming the view you are on is still a step, as + /// it is in the client's `reduce_view`. See `context/test-evidence/command.md`. + #[test] + fn a_step_back_to_the_view_you_are_on_is_the_step_the_client_folds() { + let mut app = test_app(); + app.model_state.workbook.create_view("X"); + SwitchView("X".to_string()).apply(&mut app); + app.view_state.view_back_stack.push(ViewFrame { + view_name: "X".to_string(), + mode: AppMode::Help, + }); + let mut mirror = app.view_state.clone(); + + ViewBack.apply(&mut app); + improvise_protocol::reduce_view(&mut mirror, &improvise_protocol::Command::ViewBack); + assert_eq!( + app.view_state.mode, + AppMode::Help, + "the frame's mode is restored" + ); + assert_eq!(app.view_state.view_back_stack, mirror.view_back_stack); + assert_eq!(app.view_state.view_forward_stack, mirror.view_forward_stack); + assert_eq!(app.view_state.mode, mirror.mode); + } + /// improvise-8nd9: the forward half of the step above. #[test] fn a_step_forward_the_document_refuses_leaves_no_frame_behind() { From ffd3ec80be469866b4b758a4eaebd8aec3878edc Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:46:55 -0700 Subject: [PATCH 3/7] jgzw: keep _Index/_Dim tileable, and pin the offered set by name The tile bar offering _Index and _Dim is a decision the user already made (improvise-kos), and it is the only way out of a records layout that was saved into a real view: R there emits ViewBack on an empty back stack and claims Pivot mode (filed as improvise-gj5p). A new test names the whole offered list with _Measure on a column, so a filter dropping _Measure, or one dropping _Index/_Dim, fails it. Reasoning: context/code/command.md, cmd/tile.rs. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/command.md | 37 +++++++++++++++++++ context/test-evidence/command.md | 24 ++++++++++++ .../improvise-command/src/command/cmd/tile.rs | 20 ++++++++++ 3 files changed, 81 insertions(+) diff --git a/context/code/command.md b/context/code/command.md index 987df348..e656bb9a 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -2770,6 +2770,43 @@ flag: tidy-up goes where an abort cannot reach it. stays in `TileSelect` mode so the user can adjust several tiles. `axis: None` cycles; `axis: Some(a)` sets. +**The tile bar offers `_Index` and `_Dim`, and that is deliberate (improvise-jgzw).** +Every tile call site passes `Model::category_names()`, virtual categories included, +so tile cursor 0 is `_Index`. The ticket asked whether that was a leak. It is not, +for three reasons, each checked rather than argued: + +- **It is a decision the user already made.** improvise-kos: "`_Dim` and `_Index` + should work as pivot axes outside Records/Drill view". A category that works as a + pivot axis is one the axis picker offers. +- **The tile bar is the only way out of a records layout that was saved.** + improvise-jgzw's "check what it does" found that hand-assigning `_Index` → Row and + `_Dim` → Column in a real view (tile bar or `:set-axis`) is **written** to the + `.improv`, because a real view persists every axis, and it reopens as records mode. + (`VIRTUAL_VIEW_NAMES`' "never-persisted `_Index`/`_Dim` on axes" is true only of + `_Records`/`_Drill`.) In that document, `R` (`toggle-records-mode`) emits + `ViewBack` on an empty back stack, so it leaves the layout as it was and sets the + status to "Pivot mode". That is filed as improvise-gj5p. Moving the two axes back through + the tile bar is what gets out. In the browser, which declines + `toggle-records-mode`, the tile bar is also the only way in, and improvise-bd9o + already made that route safe instead of closing it + (`the_tile_bar_route_into_records_mode_does_not_strand_the_user`). +- **What filtering would cost was measured.** With `View::tileable_cats` filtering + on `CategoryKind::is_index_or_dim`, that bd9o test and the TUI's + `renders_each_tile_with_its_axis_label` fail, as well as the two characterization + tests here. + +What the default cursor does cost, stated: one cycle key on tile 0 puts `_Index` +on Row alone, and in a pivot view that empties the grid (0 rows on +`web/demo.improv`). The status line says `_Index → Row`, and three more presses +undo it. If that turns out to matter, the fix is the order the tile bar lists +categories in, not taking entries out of it. + +**`_Measure` must stay tileable.** It is on an axis in every shipped document, so +`regular_category_names()`, the advice for *prompts*, is the wrong filter here. +`the_tile_bar_offers_every_category_by_name_measure_and_records_axes_included` +names the whole offered list with `_Measure` on a column, and fails if either +decision changes. + ### `cmd/effect_cmds.rs` The parseable model-mutation commands are thin `Cmd` wrappers around effects, diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 511a8461..8c1dce99 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5021,3 +5021,27 @@ bracket-matched `#[…]` before reading the name. `an_attributed_or_wrapped_effect_is_still_an_effect` pins that on a fixture: a doc comment, two attributes (one with nested brackets), and a wrapped call. With the whitespace skip removed it fails with `[]`. + +## `command/cmd/tile.rs` — the tile bar's offered set, by name (improvise-jgzw) + +### the_tile_bar_offers_every_category_by_name_measure_and_records_axes_included + +This test pins a decision, not a bug. The argument is in `context/code/command.md` +under `cmd/tile.rs`. Before this test, no tile assertion named the whole offered +set. The two older ones named only the category at cursor 0, so a filter that +changed what the bar offers past the cursor was invisible. The fixture puts +`_Measure` on **Column**, as every shipped document does. The test then walks the +cursor past the end and asserts the exact list +`[_Index, _Dim, _Measure, Type, Month]`. + +Rivals, injected, full workspace run: + +- **`regular_category_names()` in `TileAxisOp`**, the obvious "no virtual + categories in pickers" fix, which drops `_Measure` too. It fails this test + (`["Type", "Month"]`), the two characterization tests above it, and + `lockstep.rs::a_locally_resolved_tile_axis_change_agrees_with_the_host`. +- **`View::tileable_cats` filtering `is_index_or_dim`**, the ticket's option (b). + With the list asserted this test fails by construction, so the point is what + *else* fails: `renders_each_tile_with_its_axis_label` (TUI) and + `the_tile_bar_route_into_records_mode_does_not_strand_the_user`. The second one + is improvise-bd9o's own test of the route this decision keeps. diff --git a/crates/improvise-command/src/command/cmd/tile.rs b/crates/improvise-command/src/command/cmd/tile.rs index 1ff05418..c4a71cf9 100644 --- a/crates/improvise-command/src/command/cmd/tile.rs +++ b/crates/improvise-command/src/command/cmd/tile.rs @@ -59,6 +59,26 @@ mod tests { assert!(effects.is_empty()); } + /// improvise-jgzw: `_Index`/`_Dim` stay tileable on purpose, and `_Measure` on + /// a column must too — `context/test-evidence/command.md`. + #[test] + fn the_tile_bar_offers_every_category_by_name_measure_and_records_axes_included() { + let mut m = two_cat_model(); + m.active_view_mut().set_axis("_Measure", Axis::Column); + let layout = make_layout(&m); + let reg = make_registry(); + let mut ctx = make_ctx(&m, &layout, ®); + let mut offered = Vec::new(); + for idx in 0..8 { + ctx.tile_cat_idx = idx; + let dbg = effects_debug(&TileAxisOp { axis: None }.execute(&ctx)); + if let Some(rest) = dbg.split("CycleAxis(Active, \"").nth(1) { + offered.push(rest.split('"').next().unwrap_or_default().to_string()); + } + } + assert_eq!(offered, ["_Index", "_Dim", "_Measure", "Type", "Month"]); + } + #[test] fn move_tile_cursor_right() { let m = two_cat_model(); From bb1a05674c152b7e25b94fffe040c5c6f804136c Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:54:11 -0700 Subject: [PATCH 4/7] improvise-exdg: a records coordinate cell projects CellId::Coordinate A drawn coordinate cell projected id: None, which ADR 002 and gotcha 42 define as "no projected cell". Absence was encoding a permission (the host's). It is now its own kind, carrying no operand because no client composes a coordinate rename. render.rs::editable is an exhaustive match so the next variant must be classified. Paste, clear and edit are unchanged: every write path still matches Record alone. The test that pinned the bug (a_coordinate_column_is_projected_with_no_identity) is replaced by one that enumerates the whole records grid. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/adr/002-naming-a-cell.md | 6 +++ context/code/protocol.md | 33 +++++++++++--- context/test-evidence/command.md | 36 ++++++++++----- crates/improvise-command/src/ui/project.rs | 52 +++++++++++++--------- crates/improvise-protocol/src/render.rs | 3 ++ crates/improvise-protocol/src/resolve.rs | 9 +++- crates/improvise-wasm-client/src/lib.rs | 5 ++- crates/improvise-wasm-client/src/render.rs | 19 +++++++- 8 files changed, 123 insertions(+), 40 deletions(-) diff --git a/context/adr/002-naming-a-cell.md b/context/adr/002-naming-a-cell.md index f777136c..80626411 100644 --- a/context/adr/002-naming-a-cell.md +++ b/context/adr/002-naming-a-cell.md @@ -226,3 +226,9 @@ learns no coordinate space, and an invalid operation is unconstructible rather than rejected. What the extension adds is that the enum is now **four** answers to "what may be done here", so `None` finally means only what this ADR always said it meant — no projected cell at all. + +**A fifth kind (2026-09-22, improvise-exdg).** The records-mode coordinate cell +was the last `None` that named a visible cell. It is now `CellId::Coordinate`. +The variant carries no operand, because no client composes a coordinate rename. +The previous paragraph's "four answers" is therefore five. `None` now means +only a padded or blanked slot. Reasoning: `context/code/protocol.md` §3. diff --git a/context/code/protocol.md b/context/code/protocol.md index 85f47b8a..5259f84d 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -592,14 +592,35 @@ broader than its reason — improvise-gcvb, which also records the two things that make it more than a one-liner: an inserted `+cat` column means the TUI does **not** advance, and `AddCategory` can abort. +### `CellId::Coordinate` — a drawn coordinate is not "no cell" (improvise-exdg) + +A records-mode coordinate cell (the `Region` column holding `North`) used to +project `None`. That was a **third** meaning of `None`, next to the padded slot +and the blanked position: a visible cell that holds text and whose write +belongs to the host. So absence was encoding a permission, the same conflation +`Create` removed for the creating columns (improvise-poia). The pinning test, +`a_coordinate_column_is_projected_with_no_identity`, asserted the bug and is +gone. + +`Coordinate` carries **nothing**. Editing a coordinate renames it across every +measure cell in the row (gotcha 1), and no client composes that operation: it +needs `record_col`, and the browser declines every records-mode edit before +reaching a cell. Giving the variant a `RecordHash` would hand a client a +surrogate with nothing to spend it on. The operand belongs with the command +that consumes it, whenever that command exists. Nothing a client can do has +changed. `render.rs::editable` is now an exhaustive `match` rather than a +`matches!`, so the next variant has to be classified. `typed_cell_command`, +`record_at_cursor` and `record_cell_at_cursor` still answer `None` for it. +Paste onto a coordinate therefore stays silent in the browser, exactly as the +TUI's `cursor_key` refusal leaves it (PR #184's review, design call 2). + ### `CellDisplay::id` — `None` is not "aggregate" That is `Some(CellId::Aggregate(..))`, and the distinction is the whole point of the enum. `None` means the slot holds **no projected cell**: a `GridCache` -position padded by `fit_to_labels`, blanked by a reframing, or a -records-mode column that is not a cell to write into at all (a category column -renames a coordinate across the whole row, gotcha 1, which is a different -intent from `SetCell`). Conflating the two either hides a cell or offers a +position padded by `fit_to_labels`, or one blanked by a reframing. Every +records-mode column is some kind of cell (`Record`, `Derived`, `Create` or +`Coordinate`). Conflating the two either hides a cell or offers a write the type system exists to make unconstructible. ### `CellDisplay::edit_text` — the difference corrupts data @@ -615,8 +636,8 @@ and an editor's seed is exactly a feedback path. It is the projection of `App::display_value_at_cursor`, which is what `EditOrDrill` seeds the TUI's buffer from, so both front ends start from one string rather than each deriving one. `None` where there is nothing to edit: -the same set of positions `id` is `None` for, plus cells whose value is -absent. An error cell carries none, because its text is a diagnostic and not +positions whose `id` is `None`, plus pivot cells with no key. A records-mode +cell always carries one, even an empty one. An error cell carries none, because its text is a diagnostic and not a value. A browser found this: `tests/lockstep.rs` drove `i` through the host and the diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 511a8461..ba6ddbe1 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -1978,12 +1978,27 @@ Two measure columns of one row are two cells of one record, and the next row is different record — the same shape the pivot case has, checked here because records mode reaches it by a different path. -### a_coordinate_column_is_projected_with_no_identity +### a_coordinate_cell_is_named_rather_than_left_as_no_cell -A coordinate column is not a cell to write into: editing it renames that coordinate -across *every* measure cell in the row (gotcha 1), which is a different intent from -`SetCell`. So it is projected with no id at all, and a client cannot compose a -write against it. +improvise-exdg. This entry replaces `a_coordinate_column_is_projected_with_no_identity`, +a test that **pinned the bug**. It asserted that a records-mode coordinate cell +projects `id: None`. ADR 002 and gotcha 42 define `None` as "no projected cell", +yet the Region column is drawn and holds text. So a client could not tell it +from a padded slot. Absence was being used to encode a permission ("the host's"), +which is the conflation ADR 002's rejected `Option` draft was about. +`improvise-poia` removed the same conflation for the creating columns. + +The cell now projects `CellId::Coordinate`. That keeps the old test's real +property: it is still not a `SetCell` target, because `typed_cell_command`, +`record_cell_at_cursor` and `render.rs::editable` all match `Record` alone. +The test seats the cursor on the **second** row, so the first-row and +first-item cases cannot coincide. It then enumerates every cell of the records +grid and requires each to carry an id, which is the ticket's claim with no +case picked by hand. Before the fix it failed at `a drawn coordinate projected +as no cell`. Rival injected afterwards: `RecordCol::Category(_) => return None` +restored. This test failed at the `Some(CellId::Coordinate)` assertion, and +`a_creating_column_is_named_rather_than_left_as_no_cell` (whose `None`-count +now expects zero) failed too. ### a_write_moves_the_version_under_the_clients_own_surrogate @@ -3295,11 +3310,12 @@ aggregates, arriving a third time. The test walks **every** column of a records grid and counts, rather than asserting the two creating columns alone. That matters in the direction a -weaker test fails: a producer that answered `Create` for everything, or that -made the `Region` coordinate column nameable, would pass a two-position check -and fails this one. The fault injected — returning `None` for both creating -columns, which is the pre-fix code — reports three nameless columns where one -is allowed. +weaker test fails: a producer that answered `Create` for everything would pass +a two-position check and fails this one. The fault injected — returning `None` +for both creating columns, which is the pre-fix code — reported three nameless +columns where one was allowed. Since improvise-exdg the coordinate column is +named too (`CellId::Coordinate`), so the count of nameless columns is now zero. +See `a_coordinate_cell_is_named_rather_than_left_as_no_cell`. ### a_records_coordinate_is_text_however_it_is_spelled diff --git a/crates/improvise-command/src/ui/project.rs b/crates/improvise-command/src/ui/project.rs index 1fe8526e..b4fcb98d 100644 --- a/crates/improvise-command/src/ui/project.rs +++ b/crates/improvise-command/src/ui/project.rs @@ -324,7 +324,7 @@ fn cell_id(app: &App, layout: &GridLayout, row: usize, col: usize) -> Option return Some(CellId::Create(CreateKind::Category)), // Renaming a coordinate moves every measure cell in the row, which is a // different intent from `SetCell` and still the host's (gotcha 1). - RecordCol::Category(_) => return None, + RecordCol::Category(_) => return Some(CellId::Coordinate), }; return Some(CellId::Record(RecordCell { record: RecordHash::of(record), @@ -493,7 +493,10 @@ pub fn published_basis(app: &App, grid: &GridCache) -> Basis { .flatten() .filter_map(|cell| match cell.id.as_ref()? { CellId::Record(rc) => Some((rc.record, app.record_version_of(rc.record))), - CellId::Aggregate(_) | CellId::Derived { .. } | CellId::Create(_) => None, + CellId::Aggregate(_) + | CellId::Derived { .. } + | CellId::Create(_) + | CellId::Coordinate => None, }) .collect(); basis::published(app.document_generation(), app.layout_version(), records) @@ -4788,20 +4791,6 @@ mod cell_identity { ); } - /// A coordinate column is not a cell to write into (gotcha 1), so it is projected - /// with no id and a client cannot compose a write against it. - #[test] - fn a_coordinate_column_is_projected_with_no_identity() { - let app = records_app(); - let region = column_named(&app, "Region"); - assert_eq!( - grid_of(&app, WHOLE) - .cell(0, region) - .and_then(|c| c.id.clone()), - None - ); - } - /// A document where `Profit` is BOTH an explicit `_Measure` item and a /// formula target — which the `+` column lets a user build. fn shadowed_measure_records_app() -> App { @@ -5073,10 +5062,9 @@ mod cell_identity { let ids: Vec> = (0..app.layout.col_count()) .map(|c| grid.cell(0, c).and_then(|cell| cell.id.clone())) .collect(); - assert_eq!( - ids.iter().filter(|id| id.is_none()).count(), - 1, - "only the one Region coordinate column may be nameless: {ids:?}" + assert!( + ids.iter().all(Option::is_some), + "no records column is nameless since improvise-exdg: {ids:?}" ); assert_eq!( ids.iter() @@ -5097,6 +5085,30 @@ mod cell_identity { ); } + /// improvise-exdg: a coordinate cell is drawn and holds text, so it is not + /// "no cell". `context/test-evidence/command.md`. + #[test] + fn a_coordinate_cell_is_named_rather_than_left_as_no_cell() { + let app = records_app(); + let grid = grid_of(&app, WHOLE); + let region = column_named(&app, "Region"); + let north = grid.cell(1, region).expect("the second row's coordinate"); + assert_eq!(north.text, "North", "setup: a visible coordinate cell"); + assert_eq!( + north.id, + Some(CellId::Coordinate), + "a drawn coordinate is named" + ); + let nameless: Vec<(usize, usize)> = (0..app.layout.row_count()) + .flat_map(|r| (0..app.layout.col_count()).map(move |c| (r, c))) + .filter(|&(r, c)| grid.cell(r, c).is_some_and(|cell| cell.id.is_none())) + .collect(); + assert!( + nameless.is_empty(), + "every records cell is something: {nameless:?}" + ); + } + /// The version counter is keyed by the surrogate the client holds, so one /// identity answers ADR 002's question and ADR 001's. #[test] diff --git a/crates/improvise-protocol/src/render.rs b/crates/improvise-protocol/src/render.rs index 57f0d0c5..eb304136 100644 --- a/crates/improvise-protocol/src/render.rs +++ b/crates/improvise-protocol/src/render.rs @@ -55,6 +55,9 @@ pub enum CellId { /// A records-mode creating column: typing a **name** into it adds that /// measure or category and writes no cell at all (gotcha 1, improvise-poia). Create(CreateKind), + /// A records-mode coordinate. Renaming it moves every measure cell in the + /// row (gotcha 1), which no client composes: `protocol.md` §3 (improvise-exdg). + Coordinate, } /// What a creating column creates. The name typed into it is the whole diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index 3a86f346..0e5d0c6e 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -756,6 +756,7 @@ fn edit_or_drill(state: &ClientState, mode: AppMode) -> Option> { // Both creating columns are records-only, so the guard above takes // them; declining rather than `None` keeps that a door, not a hole. CellId::Create(_) => Some(vec![Command::SetStatus(not_yet("creating a column"))]), + CellId::Coordinate => Some(vec![Command::SetStatus(not_yet("renaming a coordinate"))]), } } @@ -949,7 +950,9 @@ fn advanced(state: &ClientState, edit_mode: AppMode, advance: Advance) -> Option fn record_at_cursor(state: &ClientState) -> Option { match cell_at_cursor(state)?.id.as_ref()? { CellId::Record(cell) => Some(cell.record), - CellId::Aggregate(_) | CellId::Derived { .. } | CellId::Create(_) => None, + CellId::Aggregate(_) | CellId::Derived { .. } | CellId::Create(_) | CellId::Coordinate => { + None + } } } @@ -1016,7 +1019,9 @@ fn model(state: &ClientState, cmd: ModelCommand) -> Option { fn record_cell_at_cursor(state: &ClientState) -> Option { match cell_at_cursor(state)?.id.as_ref()? { CellId::Record(cell) => Some(cell.clone()), - CellId::Aggregate(_) | CellId::Derived { .. } | CellId::Create(_) => None, + CellId::Aggregate(_) | CellId::Derived { .. } | CellId::Create(_) | CellId::Coordinate => { + None + } } } diff --git a/crates/improvise-wasm-client/src/lib.rs b/crates/improvise-wasm-client/src/lib.rs index ebf73c08..e0873e6f 100644 --- a/crates/improvise-wasm-client/src/lib.rs +++ b/crates/improvise-wasm-client/src/lib.rs @@ -172,7 +172,10 @@ impl Client { pub fn record_cell_at(&self, row: usize, col: usize) -> Option { match self.state.grid.cell(row, col)?.id.clone()? { CellId::Record(cell) => Some(cell), - CellId::Aggregate(_) | CellId::Derived { .. } | CellId::Create(_) => None, + CellId::Aggregate(_) + | CellId::Derived { .. } + | CellId::Create(_) + | CellId::Coordinate => None, } } diff --git a/crates/improvise-wasm-client/src/render.rs b/crates/improvise-wasm-client/src/render.rs index fa48a6bd..3f97b979 100644 --- a/crates/improvise-wasm-client/src/render.rs +++ b/crates/improvise-wasm-client/src/render.rs @@ -144,7 +144,16 @@ impl RenderCell { text: cell.text.clone(), numeric: cell.numeric, error: cell.error, - editable: matches!(cell.id, Some(CellId::Record(_))), + editable: match cell.id { + Some(CellId::Record(_)) => true, + Some( + CellId::Aggregate(_) + | CellId::Derived { .. } + | CellId::Create(_) + | CellId::Coordinate, + ) + | None => false, + }, selected, edit_text: cell.edit_text.clone().unwrap_or_default(), } @@ -628,6 +637,10 @@ mod tests { }), ), ((1, 0), CellDisplay::text("pending")), + ( + (1, 2), + CellDisplay::text("North").with_id(CellId::Coordinate), + ), ])), ], }); @@ -659,6 +672,10 @@ mod tests { !model.rows[1].cells()[1].editable, "and neither is a slot fit_to_labels padded" ); + assert!( + !model.rows[1].cells()[2].editable, + "renaming a coordinate moves the whole row, which is not a cell write" + ); } /// The numeric/error flags are the server's answers, echoed — so the test From 25a06d52fff34fc1fa7170706939c5236b53753c Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:56:53 -0700 Subject: [PATCH 5/7] improvise-7xyx: delete FrameGroup::cat and the echo it promised The field's doc comment said a client echoes it back in ToggleGroup, but resolve.rs has no toggle-group arm and neither renderer read it. Deleting it is the minimal honest answer: composing `z` in a browser needs its own design (the TUI places the cursor on the layout the toggle will produce), and the operand should come back with that consumer. The h0iv test now asserts whole-header equality, which is stronger than the (cat, name) pair it compared. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/protocol.md | 21 ++++++++++++++------- context/test-evidence/protocol.md | 12 +++++++----- crates/improvise-command/src/ui/project.rs | 1 - crates/improvise-protocol/src/render.rs | 18 +++++------------- crates/improvise-wasm-client/src/render.rs | 9 ++++----- 5 files changed, 30 insertions(+), 31 deletions(-) diff --git a/context/code/protocol.md b/context/code/protocol.md index 5259f84d..1d1555f0 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -708,9 +708,16 @@ one**. `AxisFrame::flat` leaves `cats` empty on purpose, so it is not a way to smuggle a multi-level axis in flattened. `FrameGroup::collapsed` is on the wire rather than left to the client because -it is a fact about the *view*, which only the server holds. `FrameGroup::cat` -travels so a client can echo it back in `ToggleGroup` without ever working out -which category a label came from — which slash-joined labels made impossible. +it is a fact about the *view*, which only the server holds. There is no +`FrameGroup::cat` any more (improvise-7xyx). Its doc comment promised that a +client echoes it back in `ToggleGroup`, but no client ever did. `resolve.rs` +has no `toggle-group-under-cursor` arm (`z` is on the still-needs-the-host +list), and neither renderer read the field. A promise about an interaction no +code performs is the stale-sibling defect, so the field went with it. It is +not a loss for `z`. The TUI's `ToggleGroupAtCursor` also places the cursor on +the layout the toggle *will produce*, which a thin client cannot compute. So +composing `z` in a browser needs its own design, and the operand should come +back with that design rather than wait for it. ### `AxisFrame`: four accessors that are **not for a draw loop** @@ -1217,13 +1224,13 @@ frozen behaviour reads as decided afterwards and nothing says it was only frozen for the duration of the move. **The ticket's own proposed fix does not work, and that is worth more than the -fix.** h0iv suggested comparing the `(cat, name)` pair, `FrameGroup` carrying -`cat` precisely so a client need not work out which category a label came from. +fix.** h0iv suggested comparing the `(cat, name)` pair; `FrameGroup` carried +`cat` then (improvise-7xyx has since removed it). But its own reachability case is *one* category: `Month` grouped into `Q1`, nested under `Region`, giving `North/Q1` and `South/Q1` adjacent — the two headers are `(Month, Q1)` and `(Month, Q1)`, **equal as values**, so the pair -separates nothing. `the_two_headers_are_equal_as_values_so_comparing_cat_and_name_cannot_split_them` -pins that, so the next reader does not re-propose it. What separates them is +separates nothing. `the_two_headers_are_equal_as_values_so_comparing_them_cannot_split_them` +pins that as whole-value equality, so the next reader does not re-propose it. What separates them is that they are two different `FrameEntry::Group` entries, and `groups()` already borrows each from `self.entries` — so the comparison is `std::ptr::eq` on the header, which is the literal statement of "the same header opened both". diff --git a/context/test-evidence/protocol.md b/context/test-evidence/protocol.md index b00b88db..7653d83b 100644 --- a/context/test-evidence/protocol.md +++ b/context/test-evidence/protocol.md @@ -557,7 +557,7 @@ the evidence that nothing was relying on the permissive parse. ### render::tests::two_runs_under_two_headers_of_one_name_are_two_runs -### render::tests::the_two_headers_are_equal_as_values_so_comparing_cat_and_name_cannot_split_them +### render::tests::the_two_headers_are_equal_as_values_so_comparing_them_cannot_split_them improvise-h0iv. `group_runs` coalesced adjacent positions by group **name**, so two runs under two different headers sharing a name read as one and both front @@ -571,10 +571,12 @@ and nothing ungrouped between the runs. Before the fix it returns at the precondition rather than pass with a single run. The second test exists because the ticket proposed the wrong fix and a reader -will propose it again. h0iv says `FrameGroup` carries `cat` and "comparing the -pair would fix it" — but its own reachability case is a single category nested -under an outer one, so both headers are `(Month, Q1)`, equal as values. The test -asserts that equality directly. What actually separates them is that they are +will propose it again. h0iv said `FrameGroup` carried `cat` and that "comparing +the pair would fix it". But its own reachability case is a single category +nested under an outer one, so both headers are `(Month, Q1)`, equal as values. +The test asserts that equality directly. Since improvise-7xyx removed +`FrameGroup::cat`, it asserts equality of the whole header, which is stronger: +no field-wise comparison can separate two equal values. What actually separates them is that they are two distinct entries in `self.entries`, which `std::ptr::eq` on the borrowed header states exactly. diff --git a/crates/improvise-command/src/ui/project.rs b/crates/improvise-command/src/ui/project.rs index b4fcb98d..d22ce402 100644 --- a/crates/improvise-command/src/ui/project.rs +++ b/crates/improvise-command/src/ui/project.rs @@ -222,7 +222,6 @@ fn axis_frame( pending.clear(); } pending.push(FrameEntry::Group(FrameGroup { - cat: cat_name.clone(), name: group_name.clone(), collapsed: view.is_group_collapsed(cat_name, group_name), })); diff --git a/crates/improvise-protocol/src/render.rs b/crates/improvise-protocol/src/render.rs index eb304136..e41ee37b 100644 --- a/crates/improvise-protocol/src/render.rs +++ b/crates/improvise-protocol/src/render.rs @@ -160,9 +160,6 @@ pub const MAX_ROW_HEADER_W: u16 = 24; /// because it is a fact about the *view*, which only the server holds. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct FrameGroup { - /// The category the group belongs to, echoed back in `ToggleGroup` so a - /// client never works out which category a label came from. - pub cat: String, pub name: String, pub collapsed: bool, } @@ -1064,7 +1061,6 @@ mod tests { FrameEntry::Item(vec!["A".into(), "x".into()]), FrameEntry::Item(vec!["A".into(), "y".into()]), FrameEntry::Group(FrameGroup { - cat: "Type".into(), name: "Late".into(), collapsed: true, }), @@ -1077,7 +1073,6 @@ mod tests { /// ungrouped between them — improvise-h0iv's reachable shape. fn repeated_group_frame() -> AxisFrame { let q1 = || FrameGroup { - cat: "Month".into(), name: "Q1".into(), collapsed: false, }; @@ -1103,7 +1098,6 @@ mod tests { cats: vec!["Month".into()], entries: vec![ FrameEntry::Group(FrameGroup { - cat: "Month".into(), name: "Q1".into(), collapsed: false, }), @@ -1141,7 +1135,6 @@ mod tests { cats: vec!["Month".into()], entries: vec![ FrameEntry::Group(FrameGroup { - cat: "Month".into(), name: "Q1".into(), collapsed: true, }), @@ -1193,7 +1186,7 @@ mod tests { /// The pair `FrameGroup` carries cannot separate them — h0iv's own proposed /// fix. See `context/test-evidence/protocol.md`. #[test] - fn the_two_headers_are_equal_as_values_so_comparing_cat_and_name_cannot_split_them() { + fn the_two_headers_are_equal_as_values_so_comparing_them_cannot_split_them() { let frame = repeated_group_frame(); let headers: Vec<&FrameGroup> = frame .entries @@ -1204,10 +1197,9 @@ mod tests { }) .collect(); assert_eq!( - (&headers[0].cat, &headers[0].name), - (&headers[1].cat, &headers[1].name), - "both headers name the same category AND the same group, so the \ - ticket's (cat, name) comparison would still merge them" + headers[0], headers[1], + "both headers are the same group in the same state, so no \ + comparison of their fields can split them" ); } @@ -1271,7 +1263,7 @@ mod tests { assert_eq!(frame.item(2), Some(&vec!["B".to_string(), "z".to_string()])); assert!(frame.group_of(0).is_none(), "[A,x] is in no group"); let group = frame.group_of(2).expect("[B,z] sits under Late"); - assert_eq!((group.cat.as_str(), group.name.as_str()), ("Type", "Late")); + assert_eq!(group.name, "Late"); assert!(group.collapsed, "the collapse state travels with the group"); } diff --git a/crates/improvise-wasm-client/src/render.rs b/crates/improvise-wasm-client/src/render.rs index 3f97b979..8aa220b4 100644 --- a/crates/improvise-wasm-client/src/render.rs +++ b/crates/improvise-wasm-client/src/render.rs @@ -572,9 +572,8 @@ mod tests { /// A 2x3 grid whose cells cover the four id shapes plus a padded slot, so /// one fixture answers every editability question. - fn group(cat: &str, name: &str, collapsed: bool) -> FrameEntry { + fn group(name: &str, collapsed: bool) -> FrameEntry { FrameEntry::Group(improvise_protocol::FrameGroup { - cat: cat.into(), name: name.into(), collapsed, }) @@ -592,11 +591,11 @@ mod tests { first: 0, cats: vec!["Month".into()], entries: vec![ - group("Month", "Q1", false), + group("Q1", false), item("Jan"), - group("Fiscal", "Q1", true), + group("Q1", true), item("Feb"), - group("Month", "Q2", false), + group("Q2", false), item("Apr"), ], }; From 6b3dd8ca3ec5cc471ec1f6b29ab0e305545e4daa Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:58:43 -0700 Subject: [PATCH 6/7] improvise-exdg: correct the paste claim against #184, and name improvise-kkcy Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/protocol.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/context/code/protocol.md b/context/code/protocol.md index 1d1555f0..a676e28f 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -611,8 +611,10 @@ that consumes it, whenever that command exists. Nothing a client can do has changed. `render.rs::editable` is now an exhaustive `match` rather than a `matches!`, so the next variant has to be classified. `typed_cell_command`, `record_at_cursor` and `record_cell_at_cursor` still answer `None` for it. -Paste onto a coordinate therefore stays silent in the browser, exactly as the -TUI's `cursor_key` refusal leaves it (PR #184's review, design call 2). +So paste onto a coordinate is still a silent `Upstream` in the browser. The +TUI has declined it out loud since PR #184. That divergence is +improvise-kkcy, and a named kind is what lets the browser decline deliberately +now. ### `CellDisplay::id` — `None` is not "aggregate" From c960713f8cf4b2f920591e1fbbf8cb9161d66dfe Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 22:10:29 -0700 Subject: [PATCH 7/7] The worker's status claims success only on its first frame (improvise-i0v9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The worker transport wrote "in-page worker · " when it spawned the worker, before anything had answered, so a tab whose worker could not start showed a success-looking status over a blank page. It now writes "starting the worker · …", and createShell's arm() takes an `answer` that sawFrame writes on the first frame, but only while the watchdog is still armed. A ReadOnly or SeedMissing notice lands BEFORE the Snapshot, and fail() has already disarmed, so the claim cannot overwrite the precise reason. The two-tab spec now logs every #status and denies both the premature claim and "did not answer" before the page clock is walked. A new spec pins the exact status sequence on the writable path. Reasoning: context/code/clients.md, "The watchdog". 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 | 49 ++++++++++++++++++++++++++++++++ harness/tests/shell.spec.js | 29 +++++++++++++++++++ web/app.js | 4 ++- web/shell.js | 20 ++++++++----- web/tests/shell.test.js | 37 ++++++++++++++++++++++++ 6 files changed, 156 insertions(+), 8 deletions(-) diff --git a/context/code/clients.md b/context/code/clients.md index edd147d8..e88653f3 100644 --- a/context/code/clients.md +++ b/context/code/clients.md @@ -1700,6 +1700,31 @@ watchdog must be DISARMED and not merely pre-empted**: it fires later and would overwrite the better message with "did not answer" (PR #13 review). The watchdog is for silence only, and `setShellFailure` clears the timer. +**Success is claimed by the first frame, and only if nothing failed first** +(improvise-i0v9). The worker transport used to write `in-page worker · ` +the moment it spawned the worker — 13ms after navigation, before anything had +answered — so a worker that could not start showed a success-looking status +over a blank page. It now writes `starting the worker · …`, and +`createShell().arm(message, { answer })` holds the claim until `sawFrame`. The +rule the claim obeys is **the watchdog's own state**: it is written only if the +watchdog is still armed when the first frame lands. That is not a convenience. +`worker.js` posts a `ReadOnly` (or `SeedMissing`) notice straight after +`start()` and **before** the Snapshot that answers the handshake, so an +unconditional "first frame ⇒ say we are up" would overwrite the precise reason +the notice just wrote. `fail` already disarms the watchdog, so "armed" answers +"has anything been said since arming" without a second flag to keep in step. +`fail` disarms **directly** rather than through `sawFrame`, or it would +briefly flash the success text on its way to the failure. The starting text +deliberately does not contain `in-page worker`, so the older specs asserting +that substring now need the first frame to pass. The socket path passes no +`answer`: `open` is already a real fact, and its status was never premature. + +One consequence changes what the watchdog guards. In the read-only case a +Snapshot always follows the notice, and `sawFrame` disarms the watchdog either +way, so a notice that forgot to disarm would **not** be overwritten eight +seconds later. It is overwritten **at once**, by the claim. The eight-second +overwrite survives only where no frame follows: `BootFailed`. + **Did this socket ever open?** The page has to record that itself, because nothing on the close event carries it: `readyState` is already CLOSED by the time a close listener runs, whichever happened, and a refused connection and a diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 3ce396f8..1a557e9c 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -3054,6 +3054,23 @@ The third is the gotcha 48 property, and it needs `timesArmed() > 0` first or it denies an unreachable event: the precise reason must still be on screen after the 8s window, not replaced by the watchdog. `page.clock` walks past it. +**Strengthened by improvise-i0v9's second pass.** Every assertion above read the +*final* `#status`, so two failures could still pass it. A watchdog that fired +first and was then replaced by a late notice passed. So did a success claim +shown before the notice. The test now records every status with `logStatus` and +asserts, **before** the page clock is walked, that the log ends in `read-only` +and never contained `did not answer` or the exact `in-page worker · `. A +reason that arrived after the watchdog would sit in that log behind "did not +answer"; that is the "well before 8s" property, stated without a wall-clock +bound. Measured failing against `gh/main` at `da2e5ab`, whose log was +`["in-page worker · two-tab-….improv", "read-only — …"]`: the premature claim +the ticket measured at 13ms is still there once 1wft's notice exists. The +brief's rival, `onNotice` calling `setShell` instead of `fail` (the watchdog not +disarmed), fails it at the first `toContainText("read-only")`. The Snapshot's +claim replaced the reason at once, which is why the eight-second form of this +rival is now exercised by the invalid-`?doc=` spec instead (see +`context/code/clients.md`, "The watchdog"). + It runs on Chromium and Firefox — the two engines with OPFS — rather than Chromium alone, because the exclusivity semantics are the engine's and the two word the refusal completely differently. @@ -4405,3 +4422,35 @@ assertion. The tail yanks off the `Region` and `+` columns and requires the two registers to agree; a TUI-only blank filter (`Text("")` to `None`) fails it on `+`, which is why that filter was removed rather than kept. The evidence entries for the host-side tests are in `command.md`. + +### the worker is not called up until its first frame has arrived + +improvise-i0v9. The worker transport wrote `in-page worker · ` when it +spawned the worker, before anything had answered. A worker that then failed +therefore showed a success-looking status over a blank page. The spec records +every `#status` with `logStatus` and requires the whole log to be exactly +`["starting the worker · …", "in-page worker · "]`. It pins the order +as well as the final text, since a final read cannot see a claim made too early. +Measured failing against `gh/main` at `da2e5ab`. The rival, restoring the +immediate `setShell(\`in-page worker · ${DOC}\`)` in `workerTransport`, fails it +on the log's first entry. The final read is `toHaveText`, not `toContainText`. +The older specs' `toContainText("in-page worker")` could not tell the starting +text from the claim if the one contained the other, and the starting text +deliberately does not. + +### only the first frame claims the peer answered, and not before it + +improvise-i0v9, the unit half of the spec above. `arm(message, { answer })` +must write nothing until `sawFrame`, then write `answer` once, however many +frames follow. Fails against `gh/main`'s `createShell`, which has no `answer` +and so never writes the claim at all. + +### a reason given before the first frame is not replaced by the frame's claim + +The order `worker.js` really produces on a locked document: the `ReadOnly` +notice is posted after `start()` and **before** the Snapshot. So `fail` then +`sawFrame` must leave the notice's text as the only thing ever shown. The rival +is the obvious fix, `sawFrame` writing `answer` unconditionally. It fails both +this test and the one above: the first on its exact log, which is then written +twice. This test passes against `gh/main` by construction, since no claim exists +there. It is not evidence of the bug; it guards against the naive fix. diff --git a/harness/tests/shell.spec.js b/harness/tests/shell.spec.js index 9e393226..73feb294 100644 --- a/harness/tests/shell.spec.js +++ b/harness/tests/shell.spec.js @@ -187,6 +187,21 @@ test.describe("the standalone deployment", () => { await expect(page.locator("#grid td.editable").first()).toBeVisible(); }); + // improvise-i0v9: "in-page worker · " was written at 13ms, before the + // worker had answered anything, and read as success over a blank page. + test("the worker is not called up until its first frame has arrived", async ({ page }) => { + const doc = `first-frame-${Date.now()}.improv`; + await page.addInitScript(logStatus); + await page.goto(`/app/index.html?doc=${doc}`); + await expect(page.locator("#grid table")).toBeVisible({ timeout: 20_000 }); + await expect(status(page)).toHaveText(`in-page worker · ${doc}`); + const log = await page.evaluate(() => window.__statusLog); + expect(log, `#status said: ${json(log)}`).toEqual([ + `starting the worker · ${doc}…`, + `in-page worker · ${doc}`, + ]); + }); + // improvise-9cma: a `?doc=` this backend cannot hold used to reach the // generic watchdog 8s later. The REASON now crosses the port (improvise-1wft). test("an invalid ?doc= is named on screen, and not blamed on the browser", async ({ @@ -235,6 +250,7 @@ test.describe("the standalone deployment", () => { const second = await context.newPage(); await second.clock.install(); + await second.addInitScript(logStatus); await second.goto(`/app/index.html?doc=${doc}`); // It PAINTS. That is the whole point: a locked document is read-only, not @@ -242,6 +258,15 @@ test.describe("the standalone deployment", () => { await expect(second.locator("#grid table")).toBeVisible({ timeout: 20_000 }); await expect(second.locator("#status")).toContainText("read-only"); await expect(second.locator("#status")).toContainText("another tab"); + // The page's clock has not been walked, so a reason that arrived AFTER the + // watchdog would have been logged behind "did not answer". Nor was success + // ever claimed: the Snapshot that follows the notice must not overwrite it. + const early = await second.evaluate(() => window.__statusLog); + expect(early.at(-1), `#status said: ${json(early)}`).toContain("read-only"); + expect( + early.filter((t) => t.includes("did not answer") || t === `in-page worker · ${doc}`), + `#status said: ${json(early)}`, + ).toEqual([]); // And the reason STANDS: it is not replaced 8s later by a watchdog // blaming the browser for something the code knew precisely. @@ -252,6 +277,10 @@ test.describe("the standalone deployment", () => { await second.clock.runFor(HANDSHAKE_TIMEOUT_MS + 2_000); await expect(second.locator("#status")).toContainText("read-only"); await expect(second.locator("#status")).not.toContainText("did not answer"); + const late = await second.evaluate(() => window.__statusLog); + expect(late.some((t) => t.includes("did not answer")), `#status said: ${json(late)}`).toBe( + false, + ); await second.close(); await first.close(); diff --git a/web/app.js b/web/app.js index b3c0149c..3d0cad82 100644 --- a/web/app.js +++ b/web/app.js @@ -181,7 +181,8 @@ function workerTransport() { shell.fail(`worker failed to start: ${event.message ?? "see console"}`); console.error("improvise: worker error", event); }); - shell.setShell(`in-page worker · ${DOC}`); + // Not yet a claim that it works: the first frame makes that (improvise-i0v9). + shell.setShell(`starting the worker · ${DOC}…`); // Frames posted before the worker has finished `start()` are held by // worker.js in arrival order, so the handshake can go out immediately. return { send: (text) => worker.postMessage(text), ready: Promise.resolve() }; @@ -797,6 +798,7 @@ shell.arm( WS_URL ? `${WS_URL} never answered — is improvise-ws-server running, and did it send a document?` : "the in-page worker did not answer (see console)", + { answer: WS_URL ? null : `in-page worker · ${DOC}` }, ); await transport.ready; sendViewport(); // the handshake: the snapshot is built at this extent diff --git a/web/shell.js b/web/shell.js index c2393afc..169373a7 100644 --- a/web/shell.js +++ b/web/shell.js @@ -339,7 +339,12 @@ export function createShell({ clearTimer = clearTimeout, }) { let watchdog = null; + let answered = null; let arms = 0; + const disarm = () => { + clearTimer(watchdog); + watchdog = null; + }; return { setShell(text) { @@ -350,24 +355,25 @@ export function createShell({ }, /** A peer that never answers the handshake. Deliberately about the *first * frame*, not either peer's own failure modes — clients.md. */ - arm(message) { + arm(message, { answer = null } = {}) { arms += 1; - clearTimer(watchdog); + answered = answer; + disarm(); watchdog = setTimer(() => { watchdog = null; this.setShell(message); }, timeoutMs); }, - /** A frame arrived, so the peer answered: the watchdog has no question - * left to ask. */ + /** A frame arrived, so the peer answered. Only the FIRST, and only if no + * failure spoke first, may say so (improvise-i0v9, clients.md). */ sawFrame() { - clearTimer(watchdog); - watchdog = null; + if (watchdog !== null && answered !== null) this.setShell(answered); + disarm(); }, /** A peer that failed *loudly* has already said something more precise, so * the watchdog must be DISARMED and not merely pre-empted (PR #13). */ fail(text) { - this.sawFrame(); + disarm(); this.setShell(text); }, armed: () => watchdog !== null, diff --git a/web/tests/shell.test.js b/web/tests/shell.test.js index b2d02999..e8a9570d 100644 --- a/web/tests/shell.test.js +++ b/web/tests/shell.test.js @@ -351,6 +351,43 @@ test("a notice DISARMS the watchdog rather than being replaced by it", () => { ); }); +// improvise-i0v9: "in-page worker · " was written before the worker had +// answered anything, and read as success over a blank page for eight seconds. +test("only the first frame claims the peer answered, and not before it", () => { + const clock = fakeClock(); + const seen = []; + const shell = createShell({ + onShell: (t) => seen.push(t), + timeoutMs: 8000, + setTimer: clock.setTimer, + clearTimer: clock.clearTimer, + }); + shell.arm("did not answer", { answer: "in-page worker · a.improv" }); + assert.deepEqual(seen, [], "the claim was made before any frame arrived"); + shell.sawFrame(); + shell.sawFrame(); + assert.deepEqual(seen, ["in-page worker · a.improv"]); +}); + +// The order worker.js really produces on a locked document: the ReadOnly +// notice is posted after `start()` and BEFORE the Snapshot answers the handshake. +test("a reason given before the first frame is not replaced by the frame's claim", () => { + const clock = fakeClock(); + const seen = []; + const shell = createShell({ + onShell: (t) => seen.push(t), + timeoutMs: 8000, + setTimer: clock.setTimer, + clearTimer: clock.clearTimer, + }); + shell.arm("did not answer", { answer: "in-page worker · a.improv" }); + shell.fail(noticeText({ kind: "ReadOnly", document: "a.improv", detail: "" })); + shell.sawFrame(); + clock.fire(); + assert.equal(seen.length, 1, `#status said: ${JSON.stringify(seen)}`); + assert.match(seen[0], /read-only/); +}); + // The notice's remembered state. The value crosses `localStorage`, which is // allowed to forget and allowed to throw, so the interesting inputs are the // ones that are NOT either spelling.