Merge pull request #4798 from marshiyar/fix/3599-guard-history

Engine guard: a recovered panic restores every document's undo history and the active document (#3599)

crates/engine/src/session/execute.rs

@@ -199,27 +199,30 @@ impl Session {
         r
     }
 
-    /// A top-level command: a panic (a bug) becomes [`EngineError::Internal`] and the active
-    /// document goes back to how it was before the command, instead of crashing the frontend.
+    /// A top-level command: a panic (a bug) becomes [`EngineError::Internal`] and every open
+    /// document goes back to how it was before the command (its art, selection, interaction and
+    /// undo history), and the same document stays active, instead of crashing the frontend.
     pub(crate) fn run_guarded(&mut self, id: &str, f: impl FnOnce(&mut Self) -> Result<Value>) -> Result<Value> {
-        let snapshot = self.active().map(|d| (d.uid, d.doc.clone(), d.selection.clone(), d.interaction.clone()));
-        let active = self.active;
+        let snapshot: Vec<_> =
+            self.docs.iter().map(|d| (d.uid, d.doc.clone(), d.selection.clone(), d.interaction.clone(), d.history.clone())).collect();
+        let active = self.active().map(|d| d.uid);
         let r = match guard::catch_panic(|| self.run_nested(f)) {
             Ok(r) => r,
             Err(msg) => {
                 self.depth = 0;
                 self.journal_note.clear();
-                if let Some((uid, doc, selection, interaction)) = snapshot
-                    && let Some(st) = self.docs.iter_mut().find(|d| d.uid == uid)
-                {
+                for (uid, doc, selection, interaction, history) in snapshot {
+                    // A document the command closed stays closed.
+                    let Some(st) = self.docs.iter_mut().find(|d| d.uid == uid) else { continue };
                     st.doc = doc;
                     st.selection = selection;
                     st.interaction = interaction;
+                    st.history = history;
                     cmd::globaledit::follow_history(st);
                     st.revision += 1;
                 }
-                if active.is_some_and(|i| i < self.docs.len()) {
-                    self.active = active;
+                if let Some(i) = active.and_then(|uid| self.docs.iter().position(|d| d.uid == uid)) {
+                    self.active = Some(i);
                 }
                 Err(EngineError::Internal { cmd: id.to_string(), msg })
             }

crates/engine/src/tests.rs

@@ -3521,6 +3521,46 @@ fn a_panicking_command_rolls_back_instead_of_crashing() {
     assert_eq!(s.journal.last().map(|j| j.0.as_str()), Some("shape.rectangle"));
 }
 
+/// A command that recorded an undo step and then panicked leaves Undo and Redo as they were.
+#[test]
+fn a_panic_after_an_edit_leaves_undo_and_redo_as_they_were() {
+    let mut s = session();
+    let a = rect(&mut s, 10.0, 10.0, 100.0, 50.0);
+    rect(&mut s, 200.0, 10.0, 50.0, 50.0);
+    s.execute("edit.undo", &json!({})).unwrap();
+    let (undo, redo) = (s.doc().unwrap().history.undo.len(), s.doc().unwrap().history.redo.len());
+    assert_eq!((undo, redo), (1, 1));
+    let r = s.run_guarded("test.panic", |s| {
+        s.execute("edit.clear", &json!({"ids": [a.0]}))?;
+        s.run_nested(|_| panic!("boom"))
+    });
+    assert!(matches!(r, Err(EngineError::Internal { .. })));
+    let st = s.doc().unwrap();
+    assert_eq!((st.history.undo.len(), st.history.redo.len()), (undo, redo));
+    assert!(st.doc.node(a).is_some());
+}
+
+/// A panic rolls back every document the command touched, and the active one stays the same.
+#[test]
+fn a_panic_rolls_back_other_documents_and_keeps_the_active_one() {
+    let mut s = session();
+    s.execute("file.new", &json!({"width": 400, "height": 400})).unwrap();
+    let second = rect(&mut s, 10.0, 10.0, 20.0, 20.0);
+    s.execute("document.activate", &json!({"index": 0})).unwrap();
+    let first_uid = s.doc().unwrap().uid;
+    let second_before = s.docs[1].doc.clone();
+    let steps = s.docs[1].history.undo.len();
+    let r = s.run_guarded("test.panic", |s| {
+        s.docs[1].history.undo.clear();
+        Arc::make_mut(&mut s.docs[1].doc).remove(second)?;
+        s.run_nested(|_| panic!("boom"))
+    });
+    assert!(matches!(r, Err(EngineError::Internal { .. })));
+    assert!(Arc::ptr_eq(&s.docs[1].doc, &second_before));
+    assert_eq!(s.docs[1].history.undo.len(), steps);
+    assert_eq!(s.doc().unwrap().uid, first_uid);
+}
+
 /// New layers and sublayers share one "Layer N" numbering, whatever art the document holds, and
 /// never repeat a name.
 #[test]