From 3ca12525715a7feee8e265152ac3cb4def8e6995 Mon Sep 17 00:00:00 2001
From: prxnvt <53838599+prxnvt@users.noreply.github.com>
Date: Fri, 23 Oct 2026 21:36:09 -0400
Subject: [PATCH] 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         | 18 +++++++++++++++
 crates/engine/src/tests/seams.rs | 39 ++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+)

diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs
index aaa8db4f6..09e39523b 100644
--- a/crates/engine/src/lib.rs
+++ b/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 {
diff --git a/crates/engine/src/tests/seams.rs b/crates/engine/src/tests/seams.rs
index 9c9aeb6ad..48105da25 100644
--- a/crates/engine/src/tests/seams.rs
+++ b/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> {