From 62c69ba7ab1bed1e6cbf941a6aeeb7734d0e4465 Mon Sep 17 00:00:00 2001 From: Filip Strajnar Date: Wed, 28 Oct 2026 21:59:47 +0100 Subject: [PATCH] M8: normalising after a command runs under the panic guard (part of #1085) Session::execute ran a command under guard::run but normalised what it changed afterwards, outside it: a panic in normalisation escaped execute instead of becoming EngineError::Internal with the edit rolled back. Inside a batch it unwound through Session::grouped, which then never restored the undo limit it had lifted or ended the group. Normalising is now the last step of the guarded result chain, so a panic there fails the command like one in the command itself (and a batch step fails like any other failed step); live text previews normalise under the guard too. The other half of #1085, restoring grouped()'s state when its closure panics, is left alone: #1419 rewrites that function. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01LvG6E2qDnjtMzfPVuEP7iH --- crates/engine/src/canvas/textpreview.rs | 9 ++- crates/engine/src/derived.rs | 19 ++++++ crates/engine/src/lib.rs | 2 + crates/engine/src/normalize_guard_tests.rs | 72 ++++++++++++++++++++++ crates/engine/src/session/exec.rs | 31 +++++++--- 5 files changed, 123 insertions(+), 10 deletions(-) create mode 100644 crates/engine/src/normalize_guard_tests.rs diff --git a/crates/engine/src/canvas/textpreview.rs b/crates/engine/src/canvas/textpreview.rs index da101d5e..07a09a15 100644 --- a/crates/engine/src/canvas/textpreview.rs +++ b/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(()) diff --git a/crates/engine/src/derived.rs b/crates/engine/src/derived.rs index b6a001b8..c618269f 100644 --- a/crates/engine/src/derived.rs +++ b/crates/engine/src/derived.rs @@ -10,9 +10,28 @@ pub fn changed_nodes(doc: &Document, before: &Document) -> Vec { .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 = 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 = doc.take_dirty(); // A variable or style edit changes no layer directly, but bound and linked layers must follow it. diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index ea135ca1..beaf989c 100644 --- a/crates/engine/src/lib.rs +++ b/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; diff --git a/crates/engine/src/normalize_guard_tests.rs b/crates/engine/src/normalize_guard_tests.rs new file mode 100644 index 00000000..c4ad7c08 --- /dev/null +++ b/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(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)); +} diff --git a/crates/engine/src/session/exec.rs b/crates/engine/src/session/exec.rs index 00798b2c..f5743d5d 100644 --- a/crates/engine/src/session/exec.rs +++ b/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, Vec, 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, Vec, 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 {