From ea1f2d313b7759fe47f4b0cc6c8fb25889b4c080 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:43:10 -0700 Subject: [PATCH 1/2] 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/2] 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() {