Reject layer IDs as targets of page.rename, delete and setBackground

Fixes #414

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

crates/engine/src/cmd/page.rs

@@ -23,6 +23,15 @@ fn settle(s: &mut Session, cmd: &str) -> crate::Result<()> {
     Ok(())
 }
 
+/// The page a command targets: the explicit `id` (which must be a page) or the open page.
+fn page_target(s: &Session, cmd: &str, p: &Value) -> crate::Result<NodeId> {
+    let Some(target) = id(p, "id") else { return s.page() };
+    if !s.doc()?.pages.contains(&target) {
+        return Err(bad(cmd, format!("{target} is not a page")));
+    }
+    Ok(target)
+}
+
 pub fn specs() -> Vec<CommandSpec> {
     vec![
         spec!("page.add", "Add page", "{name?} (a name starting with a hyphen adds a divider)", true, |s, p| {
@@ -71,13 +80,13 @@ pub fn specs() -> Vec<CommandSpec> {
         }),
         spec!("page.rename", "Rename page", "{id?, name} (an empty page named with a leading hyphen becomes a divider)", true, |s, p| {
             let name = req_string("page.rename", p, "name")?;
-            let id = id(p, "id").unwrap_or(s.page()?);
+            let id = page_target(s, "page.rename", p)?;
             s.doc_mut()?.get_mut(id)?.name = name;
             settle(s, "page.rename")?;
             ok()
         }),
         spec!("page.delete", "Delete page", "{id?}", true, |s, p| {
-            let id = id(p, "id").unwrap_or(s.page()?);
+            let id = page_target(s, "page.delete", p)?;
             s.doc_mut()?.remove(id)?;
             settle(s, "page.delete")?;
             ok()
@@ -114,7 +123,7 @@ pub fn specs() -> Vec<CommandSpec> {
         }),
         spec!("page.setBackground", "Page background", "{id?, color}", true, |s, p| {
             let c = color(p, "color").ok_or_else(|| bad("page.setBackground", "`color` required"))?;
-            let id = id(p, "id").unwrap_or(s.page()?);
+            let id = page_target(s, "page.setBackground", p)?;
             if let NodeKind::Page { background, .. } = &mut s.doc_mut()?.get_mut(id)?.kind {
                 *background = c;
             }

crates/engine/src/lib.rs

@@ -1528,6 +1528,8 @@ mod open_issues_tests;
 #[cfg(test)]
 mod outline_options_tests;
 #[cfg(test)]
+mod page_target_tests;
+#[cfg(test)]
 mod paint_reorder_tests;
 #[cfg(test)]
 mod paint_tests;

crates/engine/src/page_target_tests.rs

@@ -0,0 +1,63 @@
+//! Page commands act on pages: a layer id (a rectangle, a frame) is refused before anything about
+//! the layer changes, as `page.select` already does.
+
+use figcraft_doc::{NodeId, NodeKind};
+use serde_json::{Value, json};
+
+use crate::Session;
+
+fn ex(s: &mut Session, cmd: &str, p: Value) -> Value {
+    s.execute(cmd, &p).unwrap_or_else(|e| panic!("{cmd}: {e}"))
+}
+
+fn id_of(v: &Value) -> NodeId {
+    NodeId(v["id"].as_u64().unwrap())
+}
+
+fn name(s: &Session, id: NodeId) -> String {
+    s.doc().unwrap().get(id).unwrap().name.clone()
+}
+
+#[test]
+fn page_commands_reject_a_rectangle() {
+    let mut s = Session::new();
+    let page = s.page().unwrap();
+    let r = id_of(&ex(&mut s, "shape.rectangle", json!({"x": 0, "y": 0, "width": 10, "height": 10})));
+    let before = name(&s, r);
+    assert!(s.execute("page.select", &json!({"id": r})).is_err());
+    assert!(s.execute("page.rename", &json!({"id": r, "name": "Not a page"})).is_err(), "rename refuses a layer");
+    assert_eq!(name(&s, r), before, "the layer keeps its name");
+    assert!(s.execute("page.setBackground", &json!({"id": r, "color": "#123456"})).is_err(), "background refuses a layer");
+    assert!(s.execute("page.delete", &json!({"id": r})).is_err(), "delete refuses a layer");
+    assert!(s.doc().unwrap().contains(r), "the layer is still there");
+    assert!(s.doc().unwrap().contains(page));
+}
+
+#[test]
+fn page_delete_keeps_a_frame_and_its_child() {
+    let mut s = Session::new();
+    let f = id_of(&ex(&mut s, "frame.create", json!({"x": 0, "y": 0, "width": 100, "height": 100})));
+    let c = id_of(&ex(&mut s, "shape.rectangle", json!({"x": 5, "y": 5, "width": 10, "height": 10, "parent": f})));
+    assert_eq!(s.doc().unwrap().get(c).unwrap().parent, Some(f));
+    assert!(s.execute("page.delete", &json!({"id": f})).is_err());
+    assert!(s.doc().unwrap().contains(f) && s.doc().unwrap().contains(c), "the frame and its child remain");
+    assert!(s.execute("page.rename", &json!({"id": f, "name": "Not a page"})).is_err());
+    assert_ne!(name(&s, f), "Not a page");
+}
+
+#[test]
+fn page_commands_still_work_on_pages() {
+    let mut s = Session::new();
+    let first = s.page().unwrap();
+    let second = id_of(&ex(&mut s, "page.add", json!({})));
+    ex(&mut s, "page.rename", json!({"id": second, "name": "Flows"}));
+    assert_eq!(name(&s, second), "Flows");
+    ex(&mut s, "page.setBackground", json!({"id": second, "color": "#123456"}));
+    match s.doc().unwrap().get(second).unwrap().kind {
+        NodeKind::Page { background, .. } => assert_eq!(background.to_css_hex().to_uppercase(), "#123456"),
+        _ => panic!("not a page"),
+    }
+    ex(&mut s, "page.delete", json!({"id": second}));
+    assert!(!s.doc().unwrap().contains(second));
+    assert!(s.execute("page.delete", &json!({"id": first})).is_err(), "the last ordinary page stays");
+}