Batch layer.moveTo test: make the target group before B moves into G

layer.new.group inserts above the active layer, which was B inside G, so
Target was created inside G and moving G into it was rightly refused.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01UHgFEHEsefoQoBParH47JA

crates/engine/src/commands.rs

@@ -719,45 +719,15 @@ fn build() -> Vec<CommandSpec> {
             })?;
             Ok(Value::Null)
         }),
-        cmd!("layer.moveTo", "Reorder Layer", [], None, r##"{"layer":id?,"target":id,"position":"above|below|into"="above"}"##, has_layer, |s, p| {
-            let id = layer_param(s, p)?;
-            let target = LayerId(p.get("target").and_then(Value::as_u64).ok_or_else(|| bad("layer.moveTo", "missing `target`"))?);
-            let pos = p.get("position").and_then(Value::as_str).unwrap_or("above").to_string();
-            if id == target {
-                return Ok(Value::Null);
-            }
-            s.edit("Reorder Layers", |doc, active| {
-                // Refuse to move a group into itself or its descendants.
-                if doc.layer(id).is_some_and(|l| contains_layer(l, target)) {
-                    return Err(EngineError::Other("can't move a group into itself".into()));
-                }
-                let layer = doc.remove(id).ok_or(EngineError::NoLayer(id))?;
-                let path = doc.path_of(target).ok_or(EngineError::NoLayer(target))?;
-                match pos.as_str() {
-                    "into" => {
-                        let g = doc.layer_at_mut(&path).and_then(|t| t.children_mut()).ok_or_else(|| EngineError::Other("target is not a group".into()))?;
-                        g.push(layer);
-                    }
-                    other => {
-                        let (&last, parent) = path.split_last().ok_or(EngineError::NoLayer(target))?;
-                        let sib = if parent.is_empty() {
-                            &mut doc.layers
-                        } else {
-                            doc.layer_at_mut(parent)
-                                .and_then(|t| t.children_mut())
-                                .ok_or_else(|| EngineError::Other("target's parent is not a group".into()))?
-                        };
-                        let at = if other == "below" { last } else { last + 1 };
-                        sib.insert(at.min(sib.len()), layer);
-                    }
-                }
-                // Moving a group into (or beside) a deeply nested layer can pass the nesting cap.
-                crate::layer_multi_cmds::check_group_depth(doc, "Reorder Layer")?;
-                *active = Some(id);
-                Ok(())
-            })?;
-            Ok(Value::Null)
-        }),
+        cmd!(
+            "layer.moveTo",
+            "Reorder Layer",
+            [],
+            None,
+            r##"{"layer":id?| "layers":[id,…]?, "target":id,"position":"above|below|into"="above"} (layers: one undoable move, document order preserved)"##,
+            has_layer,
+            crate::layer_multi_cmds::move_to
+        ),
         cmd!(
             "layer.translate",
             "Move Layer",
@@ -1208,8 +1178,3 @@ pub(crate) fn translate_layer(doc: &Document, l: &mut Layer, dx: i32, dy: i32) {
         _ => {}
     }
 }
-
-/// Does `l` (or any descendant) have id `target`?
-fn contains_layer(l: &Layer, target: LayerId) -> bool {
-    l.id == target || l.children().is_some_and(|c| c.iter().any(|c| contains_layer(c, target)))
-}

crates/engine/src/layer_multi_cmds.rs

@@ -523,6 +523,99 @@ fn distribute(s: &mut Session, kind: &str) -> Result<Value> {
     Ok(json!({"moved": moved}))
 }
 
+/// Reorder one or multiple layers to a single destination in one undoable edit.
+///
+/// Explicit `layers` are ordered according to the source document, not caller ordering.
+/// Selected descendants of a moved group remain inside their parent; only top-level
+/// selected nodes are detached, and every reference is validated before editing.
+pub fn move_to(s: &mut Session, p: &Value) -> Result<Value> {
+    const CMD: &str = "layer.moveTo";
+    let target = LayerId(p.get("target").and_then(Value::as_u64).ok_or_else(|| bad(CMD, "missing `target`"))?);
+    let pos = p.get("position").and_then(Value::as_str).unwrap_or("above");
+    if !matches!(pos, "above" | "below" | "into") {
+        return Err(bad(CMD, "position must be above, below, or into"));
+    }
+
+    let batch = p.get("layers");
+    if batch.is_some() && p.get("layer").is_some() {
+        return Err(bad(CMD, "give either `layer` or `layers`, not both"));
+    }
+    let requested = if let Some(raw) = batch {
+        let arr = raw.as_array().filter(|a| !a.is_empty()).ok_or_else(|| bad(CMD, "`layers` must be a nonempty array of ids"))?;
+        let mut ids = Vec::with_capacity(arr.len());
+        for item in arr {
+            let id = LayerId(item.as_u64().ok_or_else(|| bad(CMD, "every layer id must be an unsigned integer"))?);
+            if ids.contains(&id) {
+                return Err(bad(CMD, "duplicate layer id"));
+            }
+            ids.push(id);
+        }
+        ids
+    } else {
+        vec![crate::commands::layer_param(s, p)?]
+    };
+    let st = s.active().ok_or(EngineError::NoDocument)?;
+    let doc = &st.doc;
+    let target_path = doc.path_of(target).ok_or(EngineError::NoLayer(target))?;
+    for &id in &requested {
+        if doc.layer(id).is_none() {
+            return Err(EngineError::NoLayer(id));
+        }
+    }
+    let moved_ids = top_level(doc, &requested);
+    if moved_ids.len() == 1 && moved_ids[0] == target && batch.is_none() {
+        // Preserve single-layer drag-to-self semantics.
+        return Ok(Value::Null);
+    }
+    // Also rejects dragging a selected group onto any of its descendants.
+    for id in &moved_ids {
+        let path = doc.path_of(*id).ok_or(EngineError::NoLayer(*id))?;
+        if target_path.starts_with(&path) {
+            return Err(bad(CMD, "target cannot be one of the moved layers or its descendant"));
+        }
+    }
+    if pos == "into" && doc.layer(target).is_none_or(|l| !l.is_group()) {
+        return Err(bad(CMD, "target is not a group"));
+    }
+
+    let original_active = st.active_layer;
+    let count = s.edit("Reorder Layers", |doc, active| {
+        let mut moved = Vec::with_capacity(moved_ids.len());
+        for &id in &moved_ids {
+            moved.push(doc.remove(id).ok_or(EngineError::NoLayer(id))?);
+        }
+        // Look up the destination AFTER removing every source: its indices may have shifted.
+        let path = doc.path_of(target).ok_or(EngineError::NoLayer(target))?;
+        if pos == "into" {
+            let children = doc.layer_at_mut(&path).and_then(Layer::children_mut).ok_or_else(|| bad(CMD, "target is not a group"))?;
+            children.extend(moved);
+        } else {
+            let (&idx, parent) = path.split_last().ok_or(EngineError::NoLayer(target))?;
+            let siblings = if parent.is_empty() {
+                &mut doc.layers
+            } else {
+                doc.layer_at_mut(parent).and_then(Layer::children_mut).ok_or_else(|| bad(CMD, "target's parent is not a group"))?
+            };
+            let at = if pos == "below" { idx } else { idx + 1 };
+            siblings.splice(at..at, moved);
+        }
+        check_group_depth(doc, "Reorder Layer")?;
+        *active = if batch.is_some() {
+            original_active.filter(|id| requested.contains(id)).or_else(|| requested.last().copied())
+        } else {
+            moved_ids.first().copied()
+        };
+        Ok(moved_ids.len())
+    })?;
+    if batch.is_some() {
+        let active = original_active.filter(|id| requested.contains(id)).or_else(|| requested.last().copied());
+        reselect(s, requested, active);
+        Ok(json!({"moved": count}))
+    } else {
+        Ok(Value::Null)
+    }
+}
+
 // ---------- structural commands ----------
 
 fn link_layers(s: &mut Session) -> Result<Value> {

crates/engine/src/tests.rs

@@ -634,6 +634,90 @@ fn move_to_reorders_and_nests() {
     assert_eq!(names(&s), ["Background", "B", "A", "G"]);
 }
 
+#[test]
+fn move_selected_layers_into_existing_group_preserves_order_selection_and_one_undo() {
+    let mut s = session_with_doc();
+    let a = s.execute("layer.new.layer", json!({"name": "A"})).unwrap()["layer"].as_u64().unwrap();
+    let b = s.execute("layer.new.layer", json!({"name": "B"})).unwrap()["layer"].as_u64().unwrap();
+    let c = s.execute("layer.new.layer", json!({"name": "C"})).unwrap()["layer"].as_u64().unwrap();
+    let g = s.execute("layer.new.group", json!({"name": "Existing Group"})).unwrap()["layer"].as_u64().unwrap();
+    s.execute("layer.select", json!({"layer": a})).unwrap();
+    s.execute("layer.select", json!({"layer": c, "mode": "add"})).unwrap();
+
+    let past = s.active().unwrap().history.past_len();
+    // Pass reversed IDs deliberately. The document order (A below C) must win.
+    let r = s.execute("layer.moveTo", json!({"layers": [c, a], "target": g, "position": "into"})).unwrap();
+    assert_eq!(r["moved"], 2);
+    let st = s.active().unwrap();
+    let root: Vec<_> = st.doc.layers.iter().map(|l| l.name.as_str()).collect();
+    assert_eq!(root, ["Background", "B", "Existing Group"]);
+    let names: Vec<_> = st.doc.layer(LayerId(g)).unwrap().children().unwrap().iter().map(|l| l.name.as_str()).collect();
+    assert_eq!(names, ["A", "C"]);
+    assert_eq!(st.history.past_len(), past + 1, "one drag = one undo step");
+    assert_eq!(st.active_layer, Some(LayerId(c)), "preserve the active member");
+    assert!(st.selected_layers().contains(&LayerId(a)));
+    assert!(st.selected_layers().contains(&LayerId(c)));
+
+    s.execute("edit.undo", json!({})).unwrap();
+    let root: Vec<_> = s.active().unwrap().doc.layers.iter().map(|l| l.name.as_str()).collect();
+    assert_eq!(root, ["Background", "A", "B", "C", "Existing Group"]);
+    assert!(s.active().unwrap().doc.layer(LayerId(g)).unwrap().children().unwrap().is_empty());
+    s.execute("edit.redo", json!({})).unwrap();
+    assert_eq!(s.active().unwrap().doc.layer(LayerId(g)).unwrap().children().unwrap().len(), 2);
+    assert_eq!(b, s.active().unwrap().doc.layers[1].id.0);
+}
+
+#[test]
+fn move_selected_layers_keeps_selected_group_children_and_supports_above_below() {
+    let mut s = session_with_doc();
+    let a = s.execute("layer.new.layer", json!({"name": "A"})).unwrap()["layer"].as_u64().unwrap();
+    let b = s.execute("layer.new.layer", json!({"name": "B"})).unwrap()["layer"].as_u64().unwrap();
+    let c = s.execute("layer.new.layer", json!({"name": "C"})).unwrap()["layer"].as_u64().unwrap();
+    let group = s.execute("layer.new.group", json!({"name": "G"})).unwrap()["layer"].as_u64().unwrap();
+    // Made before B moves into G: a new group goes above the active layer, which would then be inside G.
+    let target = s.execute("layer.new.group", json!({"name": "Target"})).unwrap()["layer"].as_u64().unwrap();
+    s.execute("layer.moveTo", json!({"layer": b, "target": group, "position": "into"})).unwrap();
+
+    let r = s.execute("layer.moveTo", json!({"layers": [b, group, a], "target": target, "position": "into"})).unwrap();
+    assert_eq!(r["moved"], 2, "moving a group already carries its selected child");
+    let target_children = s.active().unwrap().doc.layer(LayerId(target)).unwrap().children().unwrap();
+    assert_eq!(target_children.iter().map(|l| l.name.as_str()).collect::<Vec<_>>(), ["A", "G"]);
+    assert_eq!(target_children[1].children().unwrap()[0].id.0, b);
+    assert!(s.active().unwrap().selected_layers().contains(&LayerId(b)), "nested selected child is still selected");
+
+    s.execute("layer.moveTo", json!({"layers": [group, a], "target": c, "position": "above"})).unwrap();
+    assert_eq!(s.active().unwrap().doc.layers.iter().map(|l| l.name.as_str()).collect::<Vec<_>>(), ["Background", "C", "A", "G", "Target"]);
+    s.execute("layer.moveTo", json!({"layers": [group, a], "target": c, "position": "below"})).unwrap();
+    assert_eq!(s.active().unwrap().doc.layers.iter().map(|l| l.name.as_str()).collect::<Vec<_>>(), ["Background", "A", "G", "C", "Target"]);
+}
+
+#[test]
+fn move_selected_layers_invalid_targets_leave_document_and_history_unchanged() {
+    let mut s = session_with_doc();
+    let a = s.execute("layer.new.layer", json!({"name": "A"})).unwrap()["layer"].as_u64().unwrap();
+    let g = s.execute("layer.new.group", json!({"name": "G"})).unwrap()["layer"].as_u64().unwrap();
+    s.execute("layer.moveTo", json!({"layer": a, "target": g, "position": "into"})).unwrap();
+    let outside = s.execute("layer.new.layer", json!({"name": "Outside"})).unwrap()["layer"].as_u64().unwrap();
+    let root = s.active().unwrap().doc.layers.clone();
+    let past = s.active().unwrap().history.past_len();
+    for params in [
+        json!({"layers": [g, a], "target": a, "position": "into"}),
+        json!({"layers": [g], "target": g, "position": "above"}),
+        json!({"layers": [g, outside], "target": g, "position": "into"}),
+        json!({"layers": [g, outside], "target": a, "position": "below"}),
+        json!({"layers": [a], "target": outside, "position": "into"}),
+        json!({"layers": [a, a], "target": outside}),
+        json!({"layers": [a, "bogus"], "target": outside}),
+        json!({"layers": [], "target": outside}),
+        json!({"layers": [u64::MAX], "target": outside}),
+        json!({"layers": [a], "target": outside, "position": "sideways"}),
+    ] {
+        assert!(s.execute("layer.moveTo", params.clone()).is_err(), "{params}");
+        assert_eq!(s.active().unwrap().history.past_len(), past, "{params}: no history step");
+        assert_eq!(s.active().unwrap().doc.layers, root, "{params}: layers unchanged");
+    }
+}
+
 #[test]
 fn selecting_a_layer_does_not_dirty_the_document() {
     let mut s = session_with_doc();

crates/ui-egui/src/panels.rs

@@ -1817,7 +1817,7 @@ fn layer_row(
     // Photoshop's default (medium) thumbnails: 32 pt rows.
     let row_h = if t.pro { 32.0 } else { 46.0 };
     let (rect, resp) = ui.allocate_exact_size(vec2(ui.available_width(), row_h), Sense::click_and_drag());
-    layer_drag_and_drop(ctx, ui, l, rect, &resp, actions);
+    layer_drag_and_drop(app, ctx, ui, l, rect, &resp, actions);
     if resp.drag_started() {
         crate::layer_transfer::begin_from_panel(app, ctx, l.id);
     }
@@ -2676,9 +2676,27 @@ fn brush_preset_chip(ui: &mut egui::Ui, b: &photocraft_engine::BrushSettings, st
     }
 }
 
-/// Drag a layer row to reorder: drop on the upper/lower half to place above/below, or on the middle
-/// of a group to move into it. One `layer.moveTo` command (one undo step).
-fn layer_drag_and_drop(ctx: &egui::Context, ui: &egui::Ui, l: &Layer, rect: Rect, resp: &egui::Response, actions: &mut Vec<(String, Value)>) {
+/// A drag from within a multi-selection moves the whole selection, not only the grabbed
+/// row. Dragging an unselected row keeps the existing single-layer behavior.
+fn layer_drop_payload(dragged: u64, target: LayerId, position: &str, selected: &[LayerId]) -> Value {
+    if selected.len() > 1 && selected.contains(&LayerId(dragged)) {
+        json!({"layers": selected.iter().map(|id| id.0).collect::<Vec<_>>(), "target": target.0, "position": position})
+    } else {
+        json!({"layer": dragged, "target": target.0, "position": position})
+    }
+}
+
+/// Drag a layer row to reorder: drop above, below, or inside an existing group.
+/// Multi-layer moves are atomic (one undo step), using the engine's stable document order.
+fn layer_drag_and_drop(
+    app: &PhotocraftApp,
+    ctx: &egui::Context,
+    ui: &egui::Ui,
+    l: &Layer,
+    rect: Rect,
+    resp: &egui::Response,
+    actions: &mut Vec<(String, Value)>,
+) {
     let t = Tokens::get(ctx);
     let key = egui::Id::new("layer-drag");
     if resp.drag_started() {
@@ -2719,7 +2737,8 @@ fn layer_drag_and_drop(ctx: &egui::Context, ui: &egui::Ui, l: &Layer, rect: Rect
         }
     }
     if released {
-        actions.push(("layer.moveTo".into(), json!({"layer": dragged, "target": l.id.0, "position": position})));
+        let selected = app.session.active().map(|st| st.selected_layers()).unwrap_or_default();
+        actions.push(("layer.moveTo".into(), layer_drop_payload(dragged, l.id, position, &selected)));
     }
 }
 
@@ -3333,3 +3352,32 @@ mod layer_drag_edge_scroll_tests {
         assert_eq!(layer_drag_edge_scroll(Some(pos2(10.0, 20.0)), viewport, true, 0.016), 0.0);
     }
 }
+
+#[cfg(test)]
+mod group_drag_selection_tests {
+    use super::*;
+
+    #[test]
+    fn dragging_a_selected_layer_moves_the_complete_selection_into_a_group() {
+        let a = LayerId(10);
+        let b = LayerId(11);
+        let group = LayerId(20);
+        let payload = layer_drop_payload(a.0, group, "into", &[a, b]);
+        assert_eq!(payload, json!({"layers": [10, 11], "target": 20, "position": "into"}));
+        assert_eq!(payload.get("layer"), None, "batch drops must not also send a single layer");
+
+        // Above/below use the same batch route; engine preserves the document stack order.
+        assert_eq!(layer_drop_payload(b.0, group, "above", &[a, b])["position"], "above");
+        assert_eq!(layer_drop_payload(b.0, group, "below", &[a, b])["position"], "below");
+    }
+
+    #[test]
+    fn dragging_unselected_or_singular_row_remains_a_single_layer_move() {
+        let a = LayerId(10);
+        let b = LayerId(11);
+        let group = LayerId(20);
+        assert_eq!(layer_drop_payload(9, group, "into", &[a, b]), json!({"layer": 9, "target": 20, "position": "into"}));
+        assert_eq!(layer_drop_payload(a.0, group, "above", &[a]), json!({"layer": 10, "target": 20, "position": "above"}));
+        assert_eq!(layer_drop_payload(a.0, group, "below", &[]), json!({"layer": 10, "target": 20, "position": "below"}));
+    }
+}