Merge pull request #1340 from Silvertree2010/fix/1331-bug-report-figcraft-a-failed-bat

Name the failing step in batch command errors

crates/engine/src/batch_step_ref_tests.rs

@@ -258,3 +258,25 @@ fn a_nested_batch_has_its_own_names() {
     );
     assert!(msg.contains("`$ref` \"frame\" names no step"), "the outer names aren't visible inside: {msg}");
 }
+
+#[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 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"}}
+        ]),
+    );
+    // 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);
+}

crates/engine/src/lib.rs

@@ -117,6 +117,18 @@ impl From<figcraft_doc::DocError> 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<T> = std::result::Result<T, EngineError>;
 
 pub fn bad(cmd: &str, msg: impl Into<String>) -> EngineError {
@@ -1258,7 +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 v = self.execute(cmd, &p)?;
+        // 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(())
     }