Engine: a group a panicking command ended keeps its step

A group open before a command and ended or rolled back inside it lost its
undo step when the command then panicked: the restored history held no entry
for the group's changes, so Undo skipped the state before the group. The
panic path now pushes the outermost such group's step from its start to the
restored workbook. The group stays ended, because its token went with the
command. Regression test for a group ended and one rolled back.

crates/engine/src/lib.rs

@@ -536,6 +536,13 @@ impl Session {
         // made or undid, and the redo history they cleared, come back. A length is not enough,
         // because a nested `edit.undo` removes entries.
         let history_before = before.as_ref().and_then(|(uid, ..)| self.docs.iter().find(|d| d.uid == *uid).map(|d| (d.undo.clone(), d.redo.clone())));
+        // The groups open on the document before the command (usually none), for the same reason.
+        let groups_before: Vec<(u64, String, Arc<Workbook>, Selection)> = match &before {
+            Some((uid, ..)) => {
+                self.groups.iter().filter(|g| g.doc == *uid).map(|g| (g.id, g.label.clone(), g.wb.clone(), g.selection.clone())).collect()
+            }
+            None => Vec::new(),
+        };
         // 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()),
@@ -558,6 +565,7 @@ impl Session {
                 // Restore the workbook and its history as they were, and drop the undo groups the
                 // command opened.
                 self.groups.retain(|g| g.id < first_group);
+                let group_open = before.as_ref().is_some_and(|(uid, ..)| self.groups.iter().any(|g| g.doc == *uid));
                 if let Some((uid, wb, sel)) = &before
                     && let Some(d) = self.docs.iter_mut().find(|d| d.uid == *uid)
                 {
@@ -567,6 +575,16 @@ impl Session {
                         d.undo = undo;
                         d.redo = redo;
                     }
+                    // A group open before the command and ended or rolled back inside it stays
+                    // ended (its token went with the command), and keeps a step from its start to
+                    // the restored workbook, so Undo still returns to the state before the group.
+                    if !group_open
+                        && let Some((_, label, wb, selection)) = groups_before.into_iter().min_by_key(|g| g.0)
+                        && !Arc::ptr_eq(&wb, &d.wb)
+                    {
+                        push_entry(&mut d.undo, label, wb, &d.wb, selection);
+                        d.redo.clear();
+                    }
                     d.calc.rebuild(&d.wb);
                 }
                 if self.depth == 0 {

crates/engine/src/tests/seams.rs

@@ -579,6 +579,45 @@ fn a_command_that_panics_leaves_the_history_as_it_found_it() {
     assert_eq!(v(&s, "A1").display(), "1");
 }
 
+#[test]
+fn a_group_ended_by_a_command_that_then_panics_keeps_its_step() {
+    // The command takes a token the test left for it, ends or rolls back that group, and panics.
+    // The group's token went with the command, so the group stays ended; its step runs from the
+    // group's start to the restored workbook, so Undo still returns to the state before the group.
+    static TOKEN: std::sync::Mutex<Option<crate::GroupToken>> = std::sync::Mutex::new(None);
+    crate::cmd::register_undoable_test_command("test.endGroupThenPanic", |s, _| {
+        let token = TOKEN.lock().map_err(|_| EngineError::Other("lock".into()))?.take();
+        if let Some(token) = token {
+            s.group_end(token)?;
+        }
+        panic!("after ending a group");
+    });
+    crate::cmd::register_undoable_test_command("test.rollBackGroupThenPanic", |s, _| {
+        let token = TOKEN.lock().map_err(|_| EngineError::Other("lock".into()))?.take();
+        if let Some(token) = token {
+            s.group_rollback(token)?;
+        }
+        panic!("after rolling back a group");
+    });
+    for cmd in ["test.endGroupThenPanic", "test.rollBackGroupThenPanic"] {
+        let mut s = s();
+        set(&mut s, "A1", "before");
+        let token = s.group_begin("Outer").unwrap();
+        set(&mut s, "A1", "inside");
+        set(&mut s, "A2", "inside too");
+        *TOKEN.lock().unwrap() = Some(token);
+        assert!(s.execute(cmd, json!({})).is_err());
+        let uid = s.doc().unwrap().uid;
+        assert!(!s.in_group(uid), "{cmd}: the group stays ended");
+        assert_eq!(undo_labels(&s), ["Enter Cell", "Outer"], "{cmd}");
+        assert_eq!(v(&s, "A1").display(), "inside", "{cmd}: the workbook is as the command found it");
+        s.execute("edit.undo", json!({})).unwrap();
+        assert_eq!((v(&s, "A1").display(), v(&s, "A2").display()), ("before".into(), String::new()), "{cmd}");
+        s.execute("edit.redo", json!({})).unwrap();
+        assert_eq!(v(&s, "A2").display(), "inside too", "{cmd}");
+    }
+}
+
 // ---------------------------------------------------------------- recording buffer
 
 fn recorded_ids(s: &Session) -> Vec<String> {