Merge pull request #1481 from StrajnarFilip/claude/fix-1085-normalize-guard

M8: normalising after a command runs under the panic guard (part of #1085)

crates/engine/src/canvas/textpreview.rs

@@ -39,7 +39,14 @@ pub fn show(s: &mut Session, command: &str, params: &Value) -> Result<()> {
     ds.text_edit = text_edit;
     match result {
         Ok(_) if !Arc::ptr_eq(&ds.doc, &base) => {
-            normalize(ds.doc_mut(), &base);
+            // A panic while normalising shows no preview (#1085).
+            if let Err(e) = guard::run(command, || {
+                normalize(ds.doc_mut(), &base);
+                Ok(())
+            }) {
+                ds.doc = base;
+                return Err(e);
+            }
             ds.text_preview = Some(base);
             ds.revision += 1;
             Ok(())

crates/engine/src/derived.rs

@@ -10,9 +10,28 @@ pub fn changed_nodes(doc: &Document, before: &Document) -> Vec<NodeId> {
         .collect()
 }
 
+#[cfg(test)]
+thread_local! {
+    /// Tests: normalising panics, standing for a bug in it (a command's edit is rolled back).
+    pub(crate) static PANIC: std::cell::Cell<bool> = const { std::cell::Cell::new(false) };
+}
+
+/// Tests: panic when [`PANIC`] is set, as a bug in normalising would.
+#[cfg(test)]
+fn test_panic() {
+    if PANIC.with(std::cell::Cell::get) {
+        panic!("normalize: test panic");
+    }
+}
+
+/// Shipped builds: nothing (the test hook compiles away).
+#[cfg(not(test))]
+fn test_panic() {}
+
 /// Keep derived state consistent after edits: text boxes resize to their content, groups wrap
 /// their children, instances follow their main components, auto layout re-flows.
 pub fn normalize(doc: &mut Document, before: &Document) {
+    test_panic();
     // Only what was touched since the last normalisation (edited, added or removed).
     let mut touched: BTreeSet<NodeId> = doc.take_dirty();
     // A variable or style edit changes no layer directly, but bound and linked layers must follow it.

crates/engine/src/lib.rs

@@ -346,6 +346,8 @@ mod nested_override_tests;
 #[cfg(test)]
 mod node_set_refusals_tests;
 #[cfg(test)]
+mod normalize_guard_tests;
+#[cfg(test)]
 mod online_tests;
 #[cfg(test)]
 mod open_issues_tests;

crates/engine/src/normalize_guard_tests.rs

@@ -0,0 +1,72 @@
+//! A panic while normalising what a command changed is caught like one in the command itself
+//! (#1085): the command fails with `EngineError::Internal`, the document is as it was, no undo step
+//! is pushed, and the session keeps working.
+
+use serde_json::json;
+
+use crate::{EngineError, Session, derived};
+
+/// Run `f` with normalisation panicking.
+fn panicking<T>(f: impl FnOnce() -> T) -> T {
+    derived::PANIC.with(|p| p.set(true));
+    let out = f();
+    derived::PANIC.with(|p| p.set(false));
+    out
+}
+
+/// The layer id a creating command returned.
+fn id(v: &serde_json::Value) -> u64 {
+    v["id"].as_u64().unwrap()
+}
+
+/// A command whose edit panics while normalising fails with `EngineError::Internal`, leaves the
+/// document and the undo history as they were, and the same command then runs as usual.
+#[test]
+fn a_panic_while_normalising_rolls_the_command_back() {
+    let mut s = Session::new();
+    let r = id(&s.execute("shape.rectangle", &json!({"x": 0, "y": 0, "width": 10, "height": 10})).unwrap());
+    let before = s.doc().unwrap().clone();
+    let undo = s.undo_depth();
+    let result = panicking(|| s.execute("node.set", &json!({"ids": [r], "props": {"name": "Renamed"}})));
+    assert!(matches!(result, Err(EngineError::Internal { .. })), "{result:?}");
+    assert_eq!(s.doc().unwrap(), &before, "the edit is rolled back");
+    assert_eq!(s.undo_depth(), undo, "no undo step for the failed command");
+    // The next command runs as usual.
+    s.execute("node.set", &json!({"ids": [r], "props": {"name": "Renamed"}})).unwrap();
+    assert_eq!(s.doc().unwrap().node(figcraft_doc::NodeId(r)).unwrap().name, "Renamed");
+    assert_eq!(s.undo_depth().0, undo.0 + 1);
+}
+
+/// The step fails like any other failed step: the batch's group restores what it set aside for
+/// its steps (the history limit, the group depth), which a panic unwinding through it skipped.
+#[test]
+fn a_panic_while_normalising_a_batch_step_rolls_the_batch_back() {
+    let mut s = Session::new();
+    let before = s.doc().unwrap().clone();
+    let undo = s.undo_depth();
+    let limit = s.ds().unwrap().history.limit;
+    let result = panicking(|| {
+        s.execute("command.batch", &json!({"commands": [{"command": "shape.rectangle", "params": {"x": 0, "y": 0, "width": 10, "height": 10}}]}))
+    });
+    assert!(result.is_err(), "{result:?}");
+    assert_eq!(s.doc().unwrap(), &before);
+    assert_eq!(s.undo_depth(), undo);
+    assert_eq!(s.ds().unwrap().history.limit, limit, "the undo limit is back");
+    assert_eq!(s.group_depth, 0, "the group ended");
+    s.execute("shape.rectangle", &json!({"x": 0, "y": 0, "width": 10, "height": 10})).unwrap();
+}
+
+/// A live text preview whose normalising panics fails and shows nothing: the document is as it
+/// was, and the same preview then shows.
+#[test]
+fn a_panic_while_normalising_a_text_preview_shows_none() {
+    let mut s = Session::new();
+    let t = id(&s.execute("text.create", &json!({"x": 0, "y": 0, "text": "Preview"})).unwrap());
+    let before = s.doc().unwrap().clone();
+    let result = panicking(|| s.execute("text.preview", &json!({"command": "text.style", "params": {"ids": [t], "fontSize": 32}})));
+    assert!(matches!(result, Err(EngineError::Internal { .. })), "{result:?}");
+    assert_eq!(s.doc().unwrap(), &before);
+    assert!(!crate::textpreview::active(&s), "no preview shows");
+    s.execute("text.preview", &json!({"command": "text.style", "params": {"ids": [t], "fontSize": 32}})).unwrap();
+    assert!(crate::textpreview::active(&s));
+}

crates/engine/src/session/exec.rs

@@ -51,7 +51,14 @@ impl Session {
             .and_then(|v| self.check_instance_structure(id, &before).map(|()| v))
             .and_then(|v| self.check_preview(id, &before).map(|()| v))
             .and_then(|v| self.check_live_access(id, &before).map(|()| v))
-            .and_then(|v| self.check_finite_geometry(id, &before).map(|()| v));
+            .and_then(|v| self.check_finite_geometry(id, &before).map(|()| v))
+            // Normalising is part of the command: a panic in it rolls the edit back too (#1085).
+            .and_then(|v| {
+                guard::run(id, || {
+                    self.normalize_edit(spec, &before);
+                    Ok(v)
+                })
+            });
         match &result {
             Ok(_) => {
                 self.after_command(spec, before);
@@ -141,7 +148,19 @@ impl Session {
             .map_err(|m| EngineError::Disabled(id.to_string(), m))
     }
 
-    /// Bookkeeping after a successful command: normalise what changed, push undo, bump revision.
+    /// Normalise what a command changed in the document it ran on (text boxes, groups, instances,
+    /// auto layout…).
+    fn normalize_edit(&mut self, spec: &CommandSpec, before: &Option<(Arc<Document>, Vec<NodeId>, NodeId, u64)>) {
+        let Some((bdoc, _, _, uid)) = before else { return };
+        let Some(ds) = self.docs.iter_mut().find(|d| d.uid == *uid) else { return };
+        // Saving may adopt the chosen filename as the document title. The encoded snapshot is
+        // already final; normalising it again would detach it from the saved baseline.
+        if !Arc::ptr_eq(&ds.doc, bdoc) && !matches!(spec.id, "file.save" | "file.saveAs") {
+            normalize(ds.doc_mut(), bdoc);
+        }
+    }
+
+    /// Bookkeeping after a successful command (normalised already): push undo, bump revision.
     fn after_command(&mut self, spec: &CommandSpec, before: Option<(Arc<Document>, Vec<NodeId>, NodeId, u64)>) {
         let Some((bdoc, bsel, bpage, uid)) = before else {
             if let Ok(ds) = self.ds_mut() {
@@ -150,13 +169,7 @@ impl Session {
             return;
         };
         let Some(ds) = self.docs.iter_mut().find(|d| d.uid == uid) else { return };
-        let changed = !Arc::ptr_eq(&ds.doc, &bdoc);
-        // Saving may adopt the chosen filename as the document title. The encoded snapshot is
-        // already final; normalising it again would detach it from the saved baseline.
-        if changed && !matches!(spec.id, "file.save" | "file.saveAs") {
-            normalize(ds.doc_mut(), &bdoc);
-        }
-        let edit = changed && ds.interaction.is_none();
+        let edit = !Arc::ptr_eq(&ds.doc, &bdoc) && ds.interaction.is_none();
         ds.clean_selection();
         ds.revision += 1;
         if edit {