Engine: a command that runs its own undo groups makes one undo step for itself

execute treated any change in the group epoch as 'the group owns the step'. A command that edited outside its own group, or whose group rolled back before it edited, left that edit with no undo step or only part of it. Now only a group that was open before the command (or is still open after) owns the step; a command that opened and closed its own groups replaces their steps with one entry for the command.

crates/engine/src/cmd/mod.rs

@@ -187,6 +187,23 @@ pub fn register_test_command(id: &'static str, run: Run) {
     test_commands().push(spec);
 }
 
+/// Test-only: an undoable command, to exercise how `execute` records its step.
+#[cfg(test)]
+pub(crate) fn register_undoable_test_command(id: &'static str, run: Run) {
+    let spec: &'static CommandSpec = Box::leak(Box::new(CommandSpec {
+        id,
+        label: id,
+        menu: &[],
+        shortcut: None,
+        params: "",
+        enabled: |_| Ok(()),
+        run,
+        journal: false,
+        undoable: true,
+    }));
+    test_commands().push(spec);
+}
+
 fn test_commands() -> std::sync::MutexGuard<'static, Vec<&'static CommandSpec>> {
     #[allow(clippy::unwrap_used)] // a poisoned test registry has no meaningful recovery
     match TEST_COMMANDS.lock() {

crates/engine/src/lib.rs

@@ -327,6 +327,8 @@ pub struct Session {
     next_group: u64,
     /// Counts group begins and ends, so `execute` can tell that a command opened or closed one.
     group_epoch: u64,
+    /// The step the last group made: document, its undo length after the step, and the epoch.
+    group_step: Option<(u64, usize, u64)>,
 }
 
 impl Session {
@@ -396,6 +398,8 @@ impl Session {
         let before = self.active().map(|d| (d.uid, d.wb.clone(), d.selection.clone()));
         let params = if params.is_null() { Json::Object(Default::default()) } else { params };
         let (first_group, group_epoch) = (self.next_group, self.group_epoch);
+        // Whether a group was open on the document before the command, and how long its history was.
+        let start = before.as_ref().and_then(|(uid, ..)| self.docs.iter().find(|d| d.uid == *uid).map(|d| (self.in_group(*uid), d.undo.len())));
         // The recorder keeps top-level calls with the place they ran in.
         let place = (self.depth == 0 && self.recorder.is_some()).then(|| match self.active() {
             Some(d) => (d.uid, d.wb.active_sheet, d.wb.active().map(|sh| sh.name.clone()).unwrap_or_default(), d.selection.clone()),
@@ -434,15 +438,31 @@ impl Session {
                 self.journal.drain(..5_000);
             }
         }
-        // Inside an undo group, or when the command opened or closed one, the group owns the step.
-        let grouped = before.as_ref().is_some_and(|(uid, ..)| self.group_epoch != group_epoch || self.in_group(*uid));
+        // Inside an undo group, or when the command ended one that was open before it, the group
+        // owns the step. A command that opened and closed its own groups makes one step of its
+        // own, which replaces the steps those groups made.
+        let epoch_changed = self.group_epoch != group_epoch;
+        let in_group_now = before.as_ref().is_some_and(|(uid, ..)| self.in_group(*uid));
+        let own_groups = epoch_changed && !in_group_now && start.is_some_and(|(was_open, _)| !was_open);
+        let grouped = !own_groups && (epoch_changed || in_group_now);
         if spec.undoable
             && let Some((uid, wb, sel)) = before
             && let Some(d) = self.docs.iter_mut().find(|d| d.uid == uid)
             && !Arc::ptr_eq(&d.wb, &wb)
         {
             if !grouped {
-                d.undo.push(HistoryEntry { label: spec.label.trim_end_matches('…').to_string(), wb, selection: sel });
+                let mut label = spec.label.trim_end_matches('…').to_string();
+                if own_groups && let Some((_, len)) = start {
+                    // A command whose only step is its group's keeps that name ("Run Macro: Totals").
+                    if d.undo.len() == len + 1
+                        && self.group_step.is_some_and(|(doc, n, epoch)| doc == uid && n == len + 1 && epoch != group_epoch)
+                        && let Some(last) = d.undo.last()
+                    {
+                        label = last.label.clone();
+                    }
+                    d.undo.truncate(len);
+                }
+                d.undo.push(HistoryEntry { label, wb, selection: sel });
                 if d.undo.len() > UNDO_LIMIT {
                     d.undo.remove(0);
                 }
@@ -509,6 +529,7 @@ impl Session {
             while d.undo.len() > UNDO_LIMIT {
                 d.undo.remove(0);
             }
+            self.group_step = Some((g.doc, d.undo.len(), self.group_epoch));
             d.redo.clear();
             d.revision += 1;
         }

crates/engine/src/tests/seams.rs

@@ -202,6 +202,50 @@ fn a_group_owns_the_steps_of_commands_that_run_commands() {
     assert_eq!(v(&s, "A1").display(), "kept");
 }
 
+fn put(s: &mut Session, cell: &str, text: &str) -> crate::Result<()> {
+    s.execute("cell.set", json!({"cell": cell, "input": text})).map(|_| ())
+}
+
+#[test]
+fn a_command_that_runs_its_own_group_makes_one_step() {
+    use crate::cmd::register_undoable_test_command as register;
+    // Edits before its group are part of the step.
+    register("test.editThenGroup", |s, _| {
+        put(s, "A1", "outside")?;
+        s.grouped("Inner", |s| put(s, "A2", "inside"))?;
+        Ok(json!({}))
+    });
+    // A group that fails and rolls back, then an edit outside it.
+    register("test.failThenEdit", |s, _| {
+        let failed: crate::Result<()> = s.grouped("Doomed", |s| {
+            put(s, "B1", "gone")?;
+            Err(EngineError::Other("no".into()))
+        });
+        assert!(failed.is_err());
+        put(s, "B2", "kept")?;
+        Ok(json!({}))
+    });
+    // A command that is one group keeps the group's name.
+    register("test.onlyGroup", |s, _| s.grouped("Named Step", |s| put(s, "D1", "x")).map(|_| json!({})));
+    let mut s = s();
+    s.execute("test.onlyGroup", json!({})).unwrap();
+    assert_eq!(undo_labels(&s), ["Named Step"]);
+    s.execute("edit.undo", json!({})).unwrap();
+    set(&mut s, "C1", "base");
+    s.execute("test.editThenGroup", json!({})).unwrap();
+    assert_eq!(undo_labels(&s), ["Enter Cell", "test.editThenGroup"]);
+    s.execute("edit.undo", json!({})).unwrap();
+    assert!(v(&s, "A1").is_empty() && v(&s, "A2").is_empty());
+    assert_eq!(v(&s, "C1").display(), "base");
+
+    s.execute("test.failThenEdit", json!({})).unwrap();
+    assert!(v(&s, "B1").is_empty());
+    assert_eq!(v(&s, "B2").display(), "kept");
+    assert_eq!(undo_labels(&s), ["Enter Cell", "test.failThenEdit"]);
+    s.execute("edit.undo", json!({})).unwrap();
+    assert!(v(&s, "B2").is_empty());
+}
+
 // ---------------------------------------------------------------- recording buffer
 
 fn recorded_ids(s: &Session) -> Vec<String> {

docs/architecture.md

@@ -51,7 +51,9 @@ Actions made of many commands (macros, a debugger that runs one step per call) u
 - **Undo groups.** `Session::grouped(label, f)` runs `f` as one undo step and rolls every change
   back if `f` fails; `group_begin`, `group_end` and `group_rollback` do the same across calls.
   While a group is open on a document, its commands add no undo steps of their own and Undo and
-  Redo are disabled there. Groups nest; the outermost one makes the step.
+  Redo are disabled there. Groups nest; the outermost one makes the step. A command that opens and
+  closes its own groups still makes one undo step for itself, covering its edits inside and outside
+  the groups.
 - **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