From 2b33fcf881e40ef4ad1effaea270e2762f7b008d Mon Sep 17 00:00:00 2001 From: prxnvt <53838599+prxnvt@users.noreply.github.com> Date: Sat, 24 Oct 2026 00:49:43 -0400 Subject: [PATCH] Engine: a failed command rolls back the undo groups it opened A command that opened an undo group and then returned an error (a `?` between `group_begin` and `group_end`) left the group open for good: Undo and Redo refused with "finish or stop the running action first", later edits made no undo steps, and nothing could stop the group. `execute` now rolls back every group a failed command opened, as `group_rollback` does, restoring the workbook, selection, undo and redo history to where each group began. Only a command that succeeds keeps a group it opened, which is what a debugger's start needs. `Session::abort_groups(doc)` and the menu-less command `automate.stopAction` roll back the groups left open on a document, for a debugger the person abandons or a caller that dropped its token. The command is not undoable and works while a group is open, and the Undo refusal now names it. --- crates/engine/src/cmd/extra.rs | 16 ++++++ crates/engine/src/cmd/mod.rs | 2 +- crates/engine/src/lib.rs | 29 +++++++++++ crates/engine/src/tests/seams.rs | 89 ++++++++++++++++++++++++++++++++ docs/architecture.md | 12 +++-- 5 files changed, 143 insertions(+), 5 deletions(-) diff --git a/crates/engine/src/cmd/extra.rs b/crates/engine/src/cmd/extra.rs index a566f7f12..6197dd5e2 100644 --- a/crates/engine/src/cmd/extra.rs +++ b/crates/engine/src/cmd/extra.rs @@ -212,6 +212,15 @@ pub fn specs() -> Vec { trust_scripts ), cmd!("automate.deleteScript", "Delete Script", [], None, "{name}", has_doc, delete_script), + cmd!( + noundo "automate.stopAction", + "Stop Action", + [], + None, + "{} → {stopped: bool}: stops an action left running on the active workbook (an open undo group, such as a debugger's) and puts back the workbook and its undo history as they were when it began", + has_doc, + stop_action + ), // Cell controls, themes, borders, styles cmd!( "insert.checkbox", @@ -1088,6 +1097,13 @@ fn run_script(s: &mut Session, p: &Json) -> Result { Ok(json!({"ran": n})) } +/// Automate › Stop Action: rolls back the undo groups open on the active workbook, so Undo and +/// Redo work there again. +fn stop_action(s: &mut Session, _: &Json) -> Result { + let uid = s.doc()?.uid; + Ok(json!({"stopped": s.abort_groups(uid)})) +} + fn delete_script(s: &mut Session, p: &Json) -> Result { let name = str_param(p, "name").ok_or_else(|| bad("automate.deleteScript", "missing `name`"))?.to_string(); edit(s, |cx| { diff --git a/crates/engine/src/cmd/mod.rs b/crates/engine/src/cmd/mod.rs index 766a41005..8a0bb1052 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/crates/engine/src/cmd/mod.rs @@ -108,7 +108,7 @@ pub fn can_redo(s: &Session) -> std::result::Result<(), String> { /// Undo and Redo wait while an undo group is open on the active document (an action that spans /// several calls is still running). fn not_grouping(s: &Session) -> std::result::Result<(), String> { - if s.active().is_some_and(|d| s.in_group(d.uid)) { Err("finish or stop the running action first".into()) } else { Ok(()) } + if s.active().is_some_and(|d| s.in_group(d.uid)) { Err("finish or stop the running action first (automate.stopAction)".into()) } else { Ok(()) } } pub fn has_clipboard(s: &Session) -> std::result::Result<(), String> { has_doc(s)?; diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 8a8d8bcfa..3b40d7401 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -594,6 +594,12 @@ impl Session { return Err(EngineError::Internal(id.to_string(), msg)); } }; + // A command that fails leaves none of its own undo groups open (a `?` between + // `group_begin` and `group_end`): each is rolled back as `group_rollback` does. Only a + // command that succeeds keeps a group it opened (a debugger's start). + if result.is_err() { + self.rollback_groups_from(first_group); + } if spec.journal && result.is_ok() { // A command that started recording is not part of the recording; one that stopped it // finds no recorder. @@ -751,6 +757,29 @@ impl Session { Ok(changed) } + /// Stops the running action on the document with uid `doc`: every undo group open on it is + /// rolled back as [`Session::group_rollback`] does, putting back the workbook, selection, undo + /// and redo history as they were when the outermost one began. Their tokens are then spent + /// (ending or rolling one back reports that it has already ended). Returns whether a group + /// was open. `automate.stopAction` runs it on the active document. + pub fn abort_groups(&mut self, doc: u64) -> bool { + let Some(id) = self.groups.iter().filter(|g| g.doc == doc).map(|g| g.id).min() else { return false }; + // Rolling back the outermost group ends the ones opened inside it. + self.group_rollback(GroupToken { id, doc }).is_ok() + } + + /// Rolls back every open undo group numbered `first` or later (the groups a failed command + /// opened), outermost first on each document. + fn rollback_groups_from(&mut self, first: u64) { + let opened: Vec<(u64, u64)> = self.groups.iter().filter(|g| g.id >= first).map(|g| (g.id, g.doc)).collect(); + for (id, doc) in opened { + // A group inside one rolled back before it is already gone. + if self.groups.iter().any(|g| g.id == id) { + let _ = self.group_rollback(GroupToken { id, doc }); + } + } + } + /// Bounds the undo steps pushed while a command runs (a script of many steps, a subtotal per /// group): at most [`UNDO_LIMIT`] of them within [`UNDO_BYTES_LIMIT`], dropping the oldest. /// Only steps after the latest mark go: a running command's start, an open group's start and diff --git a/crates/engine/src/tests/seams.rs b/crates/engine/src/tests/seams.rs index 0f1bc3072..9d32c66d1 100644 --- a/crates/engine/src/tests/seams.rs +++ b/crates/engine/src/tests/seams.rs @@ -643,6 +643,95 @@ fn a_group_ended_by_a_command_that_then_panics_keeps_its_step() { } } +#[test] +fn a_command_that_fails_rolls_back_the_groups_it_opened() { + // A `?` between `group_begin` and `group_end` used to leave the group open for good: Undo + // and Redo refused, and later edits made no steps. + crate::cmd::register_undoable_test_command("test.groupThenFail", |s, _| { + let _token = s.group_begin("Half")?; + put(s, "A1", "half done")?; + Err(EngineError::Other("stopped halfway".into())) + }); + crate::cmd::register_undoable_test_command("test.editGroupThenFail", |s, _| { + put(s, "B1", "outside")?; + let _token = s.group_begin("Half")?; + put(s, "B2", "inside")?; + Err(EngineError::Other("stopped halfway".into())) + }); + let mut s = s(); + set(&mut s, "A1", "before"); + set(&mut s, "A2", "undone"); + s.execute("edit.undo", json!({})).unwrap(); + s.execute("selection.set", json!({"range": "C3"})).unwrap(); + let uid = s.doc().unwrap().uid; + let err = s.execute("test.groupThenFail", json!({})).unwrap_err().to_string(); + assert!(err.contains("stopped halfway"), "{err}"); + assert!(!s.in_group(uid), "no group is left open"); + assert_eq!(v(&s, "A1").display(), "before"); + assert_eq!(s.doc().unwrap().selection.a1(), "C3"); + assert_eq!(undo_labels(&s), ["Enter Cell"]); + assert_eq!(s.doc().unwrap().redo.len(), 1, "the redo history comes back"); + assert_eq!(s.execute("history.list", json!({})).unwrap()["grouping"], json!(false)); + set(&mut s, "A3", "next"); + assert_eq!(undo_labels(&s), ["Enter Cell", "Enter Cell"], "later edits make steps again"); + s.execute("edit.undo", json!({})).unwrap(); + assert!(v(&s, "A3").is_empty()); + + // What the command changed before opening the group stays, as for any failed command, and is + // a step of its own; what it changed inside the group goes. + assert!(s.execute("test.editGroupThenFail", json!({})).is_err()); + assert!(!s.in_group(uid)); + assert_eq!((v(&s, "B1").display(), v(&s, "B2").display()), ("outside".into(), String::new())); + assert_eq!(undo_labels(&s), ["Enter Cell", "test.editGroupThenFail"]); + s.execute("edit.undo", json!({})).unwrap(); + assert!(v(&s, "B1").is_empty()); + + // A group open before the command stays open: only the command's own group goes. + let token = s.group_begin("Outer").unwrap(); + set(&mut s, "D1", "outer"); + assert!(s.execute("test.groupThenFail", json!({})).is_err()); + assert!(s.in_group(uid), "the outer group is still running"); + assert_eq!((v(&s, "A1").display(), v(&s, "D1").display()), ("before".into(), "outer".into())); + assert!(s.group_end(token).unwrap()); + assert_eq!(undo_labels(&s), ["Enter Cell", "Outer"]); + s.execute("edit.undo", json!({})).unwrap(); + assert!(v(&s, "D1").is_empty()); +} + +#[test] +fn stop_action_rolls_back_a_group_left_open() { + // A host that dropped its token, or a debugger the person wants to abandon: the message on + // Undo names the command that stops it, and the command puts the start back. + let mut s = s(); + set(&mut s, "A1", "start"); + set(&mut s, "A2", "undone"); + s.execute("edit.undo", json!({})).unwrap(); + let uid = s.doc().unwrap().uid; + let token = s.group_begin("Debug").unwrap(); + set(&mut s, "A1", "step 1"); + drop(s.group_begin("Inner").unwrap()); + set(&mut s, "A3", "step 2"); + let err = s.execute("edit.undo", json!({})).unwrap_err().to_string(); + assert!(err.contains("automate.stopAction"), "{err}"); + assert_eq!(s.execute("automate.stopAction", json!({})).unwrap(), json!({"stopped": true})); + assert!(!s.in_group(uid)); + assert_eq!(v(&s, "A1").display(), "start"); + assert!(v(&s, "A3").is_empty()); + assert_eq!(undo_labels(&s), ["Enter Cell"], "stopping is not a step of its own"); + assert_eq!(s.doc().unwrap().redo.len(), 1, "the redo history comes back"); + assert!(s.group_end(token).is_err(), "the token is spent"); + assert_eq!(s.execute("automate.stopAction", json!({})).unwrap(), json!({"stopped": false})); + s.execute("edit.redo", json!({})).unwrap(); + assert_eq!(v(&s, "A2").display(), "undone"); + // Other documents keep their groups. + let other = s.new_workbook(); + let other_uid = s.documents()[other].uid; + let _kept = s.group_begin("Theirs").unwrap(); + s.set_active(0); + assert_eq!(s.execute("automate.stopAction", json!({})).unwrap(), json!({"stopped": false})); + assert!(s.in_group(other_uid)); +} + // ---------------------------------------------------------------- recording buffer fn recorded_ids(s: &Session) -> Vec { diff --git a/docs/architecture.md b/docs/architecture.md index f51f3a61b..63119fe16 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -1,6 +1,6 @@ # Architecture -> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-23 · **Change:** minor (engine building blocks for multi-command actions) · **Target:** Microsoft Excel (Microsoft 365) +> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-24 · **Change:** minor (engine building blocks for multi-command actions) · **Target:** Microsoft Excel (Microsoft 365) How GridCraft is built today. Rules for contributors are in [`AGENTS.md`](../AGENTS.md). @@ -59,8 +59,12 @@ Actions made of many commands (macros, a debugger that runs one step per call) u closes its own groups still makes one undo step for itself, covering its edits inside and outside the groups. A command that ends a group open before it leaves the group its step and makes one of its own for what it changes after the end; a group a command opens and leaves open starts - where the command did. The history limit is applied when the outermost command finishes; until - then the steps made inside it are bounded on their own. + where the command did, but only when the command succeeds: a command that fails rolls back + every group it opened, as `group_rollback` does. `Session::abort_groups(doc)`, and the command + `automate.stopAction` on the active workbook, roll back the groups left open on a document (a + debugger the person abandons, a caller that dropped its token), so Undo works there again. The + history limit is applied when the outermost command finishes; until then the steps made inside + it are bounded on their own. - **Recording buffer** (`record.rs`). `Session::start_recording` collects each successful top-level command with the document, sheet and selection it ran in, without secret parameters. Commands run by other commands are not recorded, and the buffer is independent of the journal @@ -103,6 +107,6 @@ rpm, tarball), FreeBSD, and web (WASM; WebGPU or WebGL2). Release workflows in | Date | Change | Summary | |---|---|---| -| 2026-10-23 | minor | Engine building blocks for multi-command actions: undo groups, recording buffer, document trust, chunked hidden-name storage | +| 2026-10-24 | minor | Engine building blocks for multi-command actions: undo groups, recording buffer, document trust, chunked hidden-name storage | | 2026-10-14 | minor | Document Undo's retained-cell-storage budget and latest-step guarantee | | 2026-10-10 | major | Created from the code on main: crates, layers, data flow, agent control, known gaps |