From 0a4e70fb972ab127ce918c37bd3c14b8a40478b4 Mon Sep 17 00:00:00 2001 From: NetterSilver <133535268+Silvertree2010@users.noreply.github.com> Date: Sun, 25 Oct 2026 18:23:37 +0000 Subject: [PATCH 1/2] Name the failing step in batch command errors Fixes #1331 Co-Authored-By: Claude Haiku 5.5 Co-Authored-By: Claude Sonnet 5.5 --- crates/engine/src/batch_step_ref_tests.rs | 23 +++++++++++++++++++++++ crates/engine/src/lib.rs | 10 +++++++++- 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/crates/engine/src/batch_step_ref_tests.rs b/crates/engine/src/batch_step_ref_tests.rs index ac69118e..e84985c6 100644 --- a/crates/engine/src/batch_step_ref_tests.rs +++ b/crates/engine/src/batch_step_ref_tests.rs @@ -110,3 +110,26 @@ fn a_nested_batch_refers_to_its_own_steps() { let rect = s.doc().unwrap().node(id(&inner[1])).unwrap().clone(); assert_eq!(rect.parent, Some(id(&inner[0])), "in the nested batch's own first frame, not the outer one"); } + +#[test] +fn a_failing_command_names_its_step_and_rolls_back() { + let mut s = Session::new(); + let frame = id(&s.execute("node.create", &json!({"type": "frame", "width": 50, "height": 50})).unwrap()); + let text = id(&s.execute("node.create", &json!({"type": "text", "text": "orig"})).unwrap()); + let undo = s.ds().unwrap().history.undo.len(); + let name = s.doc().unwrap().node(s.page().unwrap()).unwrap().name.clone(); + let r = s.execute( + "command.batch", + &json!({"commands": [ + {"command": "text.set", "params": {"ids": [text.0], "text": "novo"}}, + {"command": "text.set", "params": {"ids": [frame.0], "text": "erro"}}, + {"command": "page.rename", "params": {"name": "Depois do erro"}} + ]}), + ); + let msg = r.unwrap_err().to_string(); + assert!(msg.contains("step 1: invalid parameters for"), "{msg}"); + assert!(msg.contains("is not text"), "{msg}"); + assert!(matches!(&s.doc().unwrap().node(text).unwrap().kind, figcraft_doc::NodeKind::Text(t) if t.characters == "orig")); + assert_eq!(s.doc().unwrap().node(s.page().unwrap()).unwrap().name, name); + assert_eq!(s.ds().unwrap().history.undo.len(), undo); +} diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 99b0f6f2..2d53f109 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -1280,7 +1280,15 @@ impl Session { /// resolve `$step` the same way, inside its own [`Session::grouped`]. pub fn batch_step(&mut self, results: &[Value], step: usize, cmd: &str, params: &Value) -> Result { let p = if cmd == "command.batch" { params.clone() } else { resolve_step_refs(params, results, step, 0)? }; - self.execute(cmd, &p) + self.execute(cmd, &p).map_err(|e| match e { + // Say which step failed; a nested batch's error already does. + EngineError::UnknownCommand(_) | EngineError::BadParams { .. } | EngineError::NoNode(_) | EngineError::Other(_) + if !e.to_string().starts_with("step ") => + { + EngineError::Other(format!("step {step}: {e}")) + } + e => e, + }) } /// Run `f` as one undo step called `label`: the commands it executes push no steps of their From 6f1999c24836ab13aa188a139adbec274fee497c Mon Sep 17 00:00:00 2001 From: bflatastic Date: Mon, 26 Oct 2026 09:14:25 +0700 Subject: [PATCH 2/2] #1340: a failed step keeps its kind of error; only its message names the step --- crates/engine/src/batch_step_ref_tests.rs | 13 ++++++------ crates/engine/src/lib.rs | 24 +++++++++++++---------- 2 files changed, 20 insertions(+), 17 deletions(-) diff --git a/crates/engine/src/batch_step_ref_tests.rs b/crates/engine/src/batch_step_ref_tests.rs index 64467255..47d6e876 100644 --- a/crates/engine/src/batch_step_ref_tests.rs +++ b/crates/engine/src/batch_step_ref_tests.rs @@ -266,17 +266,16 @@ fn a_failing_command_names_its_step_and_rolls_back() { let text = id(&s.execute("node.create", &json!({"type": "text", "text": "orig"})).unwrap()); let undo = s.ds().unwrap().history.undo.len(); let name = s.doc().unwrap().node(s.page().unwrap()).unwrap().name.clone(); - let r = s.execute( - "command.batch", - &json!({"commands": [ + let msg = batch_error( + &mut s, + json!([ {"command": "text.set", "params": {"ids": [text.0], "text": "novo"}}, {"command": "text.set", "params": {"ids": [frame.0], "text": "erro"}}, {"command": "page.rename", "params": {"name": "Depois do erro"}} - ]}), + ]), ); - let msg = r.unwrap_err().to_string(); - assert!(msg.contains("step 1: invalid parameters for"), "{msg}"); - assert!(msg.contains("is not text"), "{msg}"); + // Still the command's own kind of error, its message saying which step. + assert!(msg.starts_with("step 1: ") && msg.contains("is not text"), "{msg}"); assert!(matches!(&s.doc().unwrap().node(text).unwrap().kind, figcraft_doc::NodeKind::Text(t) if t.characters == "orig")); assert_eq!(s.doc().unwrap().node(s.page().unwrap()).unwrap().name, name); assert_eq!(s.ds().unwrap().history.undo.len(), undo); diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 710dd779..f47e2ea2 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -117,6 +117,18 @@ impl From for EngineError { } } +impl EngineError { + /// This error from step `step` of a batch, saying so in its message; the kind stays (errors + /// without a message stay as they are). + fn at_step(self, step: usize) -> Self { + match self { + EngineError::BadParams { cmd, msg } => EngineError::BadParams { cmd, msg: format!("step {step}: {msg}") }, + EngineError::Other(msg) => EngineError::Other(format!("step {step}: {msg}")), + e => e, + } + } +} + pub type Result = std::result::Result; pub fn bad(cmd: &str, msg: impl Into) -> EngineError { @@ -1258,16 +1270,8 @@ impl Session { /// so its steps resolve references the same way, inside its own [`Session::grouped`]. pub fn batch_step(&mut self, scope: &mut BatchScope, cmd: &str, params: &Value) -> Result<()> { let p = if cmd == "command.batch" { params.clone() } else { scope.resolve(params)? }; - let step = scope.next_step(); - let v = self.execute(cmd, &p).map_err(|e| match e { - // Say which step failed; a nested batch's error already does. - EngineError::UnknownCommand(_) | EngineError::BadParams { .. } | EngineError::NoNode(_) | EngineError::Other(_) - if !e.to_string().starts_with("step ") => - { - EngineError::Other(format!("step {step}: {e}")) - } - e => e, - })?; + // Say which step failed; a nested batch names its own steps. + let v = self.execute(cmd, &p).map_err(|e| if cmd == "command.batch" { e } else { e.at_step(scope.next_step()) })?; scope.push(v); Ok(()) }