From ae72084a9ce02ef4fc5b90c27a04bbdc6efa1fcc Mon Sep 17 00:00:00 2001
From: Vikram Bhaskaran <vikram.bhaskaran@outlook.com>
Date: Tue, 13 Oct 2026 10:19:23 +1100
Subject: [PATCH] Resolve every pattern name before a batch delete removes any
 of them. (#4132)

* Resolve every pattern name before a batch delete removes any of them.

A missing name used to delete the earlier presets and then return an error. The pattern library is not on the document undo stack, so that partial delete could not be undone.

Fixes #4103

* Pattern batch delete: reject non-string entries and resolve against the library only

A document-only pattern resolved through pattern_cmds::resolve, so the batch
deleted earlier entries before pattern.delete refused it. Non-string array
members were silently dropped (ported from #4127).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Brandon Thomas <echelon@gmail.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
---
 crates/engine/src/presets/patterns.rs | 27 ++++++++++++++++---
 crates/engine/src/presets/tests.rs    | 37 +++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 4 deletions(-)

diff --git a/crates/engine/src/presets/patterns.rs b/crates/engine/src/presets/patterns.rs
index f4ecb15c3c..2425dd0900 100644
--- a/crates/engine/src/presets/patterns.rs
+++ b/crates/engine/src/presets/patterns.rs
@@ -125,13 +125,32 @@ fn edit(s: &mut Session, p: &Value) -> Result<Value> {
         "rename" => super::call(s, "pattern.rename", json!({"pattern": req_str(p, "preset", CMD)?, "name": req_str(p, "name", CMD)?}))?,
         "delete" => {
             let keys: Vec<String> = match p.get("preset") {
-                Some(Value::Array(a)) => a.iter().filter_map(Value::as_str).map(str::to_string).collect(),
+                Some(Value::Array(a)) => a
+                    .iter()
+                    .map(|v| v.as_str().map(str::to_string).ok_or_else(|| bad(CMD, "`preset` must contain only pattern ids or names")))
+                    .collect::<Result<_>>()?,
                 _ => vec![req_str(p, "preset", CMD)?.to_string()],
             };
-            for k in &keys {
-                super::call(s, "pattern.delete", json!({"pattern": k}))?;
+            if keys.is_empty() {
+                return Err(bad(CMD, "missing `preset`"));
             }
-            json!({"deleted": keys.len()})
+            // Resolve every reference first. Deleting as we go left the library partly
+            // removed when a later name was missing, and Undo cannot put a preset back (#4103).
+            let mut ids = Vec::new();
+            for k in &keys {
+                // Only library patterns can be deleted here; a document-only pattern must
+                // fail the whole batch up front rather than midway through it.
+                let id = photocraft_doc::pattern::find(&s.patterns.items, k, k)
+                    .map(|q| q.id.clone())
+                    .ok_or_else(|| bad(CMD, format!("no library pattern \"{k}\"")))?;
+                if !ids.contains(&id) {
+                    ids.push(id);
+                }
+            }
+            for id in &ids {
+                super::call(s, "pattern.delete", json!({"pattern": id}))?;
+            }
+            json!({"deleted": ids.len()})
         }
         "deleteGroup" => {
             // Deleting a folder deletes its patterns, as in Photoshop.
diff --git a/crates/engine/src/presets/tests.rs b/crates/engine/src/presets/tests.rs
index 7ebf2145b1..dcfc4e94d8 100644
--- a/crates/engine/src/presets/tests.rs
+++ b/crates/engine/src/presets/tests.rs
@@ -385,6 +385,43 @@ fn pattern_groups_select_apply_and_sync() {
     assert!(crate::pattern_cmds::resolve(&s, "Weave").is_none());
 }
 
+#[test]
+fn pattern_batch_delete_keeps_the_library_when_one_name_is_missing() {
+    let mut s = session(8);
+    let before = s.execute("pattern.presets.list", json!({})).unwrap();
+    let err = s.execute("pattern.presets.edit", json!({"action": "delete", "preset": ["Bricks", "missing"]})).unwrap_err();
+    assert!(err.to_string().contains("missing"), "{err}");
+    assert_eq!(s.execute("pattern.presets.list", json!({})).unwrap(), before);
+    assert!(crate::pattern_cmds::resolve(&s, "Bricks").is_some());
+}
+
+#[test]
+fn pattern_batch_delete_rejects_non_string_members_without_deleting() {
+    for invalid in [json!(null), json!(7), json!({})] {
+        let mut s = session(8);
+        let before = s.execute("pattern.presets.list", json!({})).unwrap();
+        assert!(s.execute("pattern.presets.edit", json!({"action": "delete", "preset": ["Bricks", invalid]})).is_err());
+        assert_eq!(s.execute("pattern.presets.list", json!({})).unwrap(), before);
+    }
+}
+
+#[test]
+fn pattern_batch_delete_refuses_a_document_only_pattern_up_front() {
+    let mut s = session(8);
+    let added = s.execute("edit.definePattern", json!({"name": "Document Only"})).unwrap();
+    let id = added["pattern"].as_str().unwrap().to_string();
+    let pattern = s.patterns.items.iter().find(|p| p.id == id).unwrap().clone();
+    s.edit("Embed pattern fixture", |doc, _| {
+        doc.patterns.push(pattern.clone());
+        Ok(())
+    })
+    .unwrap();
+    s.execute("pattern.delete", json!({"pattern": id})).unwrap();
+    let before = s.patterns.items.clone();
+    assert!(s.execute("pattern.presets.edit", json!({"action": "delete", "preset": ["Bricks", id]})).is_err());
+    assert_eq!(s.patterns.items, before, "nothing is deleted when one entry is not in the library");
+}
+
 // ------------------------------------------------------------------ styles
 
 #[test]