diff --git a/crates/op-cli/src/cli_node_tests.rs b/crates/op-cli/src/cli_node_tests.rs new file mode 100644 index 000000000..4d7379181 --- /dev/null +++ b/crates/op-cli/src/cli_node_tests.rs @@ -0,0 +1,33 @@ +use super::*; + +#[test] +fn parse_args_maps_insert_parent_and_page_flags_to_ts_args() { + let p = parse_args(&[ + "insert".to_string(), + r##"{"kind":"rect","name":"Child","x":1,"y":2,"width":3,"height":4,"fill":"#fff"}"## + .to_string(), + "--parent".to_string(), + "n10".to_string(), + "--page".to_string(), + "page-2".to_string(), + ]) + .expect("parse insert"); + + assert_eq!( + p.command, + Command::ToolCall { + tool: "insert_node".to_string(), + args: vec![ + ("kind".to_string(), "rect".to_string()), + ("name".to_string(), "Child".to_string()), + ("x".to_string(), "1".to_string()), + ("y".to_string(), "2".to_string()), + ("width".to_string(), "3".to_string()), + ("height".to_string(), "4".to_string()), + ("fill_hex".to_string(), "#fff".to_string()), + ("parent".to_string(), "n10".to_string()), + ("pageId".to_string(), "page-2".to_string()), + ], + } + ); +} diff --git a/crates/op-cli/src/main.rs b/crates/op-cli/src/main.rs index 401fc896e..9c3caec89 100644 --- a/crates/op-cli/src/main.rs +++ b/crates/op-cli/src/main.rs @@ -232,7 +232,7 @@ fn command_from_positionals(positionals: &[String], flags: &Flags) -> Result map_get(flags), "selection" => tool_call("get_selection", vec![]), - "insert" => map_insert(positionals), + "insert" => map_insert(positionals, flags), "update" => map_update(positionals), "delete" => map_one_id("delete_node", positionals), "read-nodes" => map_read_nodes(positionals, flags), @@ -292,9 +292,16 @@ fn map_get(flags: &Flags) -> Result { tool_call("batch_get", pairs) } -fn map_insert(positionals: &[String]) -> Result { +fn map_insert(positionals: &[String], flags: &Flags) -> Result { let raw = resolve_arg(positionals.get(1).map(String::as_str))?; - tool_call("insert_node", insert_pairs(&raw)?) + let mut pairs = insert_pairs(&raw)?; + if let Some(parent) = flag_value(flags, "parent") { + pairs.push(pair("parent", parent)); + } + if let Some(page) = flag_value(flags, "page") { + pairs.push(pair("pageId", page)); + } + tool_call("insert_node", pairs) } fn map_update(positionals: &[String]) -> Result { @@ -723,4 +730,6 @@ fn version_json() -> String { #[cfg(test)] mod cli_import_tests; #[cfg(test)] +mod cli_node_tests; +#[cfg(test)] mod tests; diff --git a/crates/op-cli/src/usage.txt b/crates/op-cli/src/usage.txt index 7395e5a72..e7b8cfa01 100644 --- a/crates/op-cli/src/usage.txt +++ b/crates/op-cli/src/usage.txt @@ -14,7 +14,8 @@ COMMON COMMANDS: op status check whether MCP HTTP is reachable op get [--type T] [--name N] [--id ID] [--depth N] [--parent P] op selection get current selection - op insert insert a leaf node + op insert [--parent P] [--page PAGE] + insert a leaf node op update patch x/y/width/height/name/fill op delete delete a node op read-nodes [ids] [--depth N] [--vars] [--page P] [--file F] diff --git a/crates/op-editor-core/src/command.rs b/crates/op-editor-core/src/command.rs index 87532c976..e8b8a6e63 100644 --- a/crates/op-editor-core/src/command.rs +++ b/crates/op-editor-core/src/command.rs @@ -126,6 +126,11 @@ pub enum EditorCommand { width: i32, height: i32, fill_hex: Option, + /// `NodeId::NONE` inserts at the active/requested page root. + target_parent: NodeId, + /// `None` inserts on the active page; `Some` targets a page id + /// or legacy page index. + page_id: Option, }, /// Patch fields on an existing node — every field optional. UpdateNode { diff --git a/crates/op-editor-core/src/command_apply.rs b/crates/op-editor-core/src/command_apply.rs index dafb5dc14..f103132b2 100644 --- a/crates/op-editor-core/src/command_apply.rs +++ b/crates/op-editor-core/src/command_apply.rs @@ -70,7 +70,7 @@ fn parse_variable_kind(s: &str) -> Option { } } -fn import_svg_page_index(state: &EditorState, page_id: Option<&str>) -> Option { +fn command_page_index(state: &EditorState, page_id: Option<&str>) -> Option { let Some(raw) = page_id.map(str::trim).filter(|s| !s.is_empty()) else { return Some( state @@ -88,6 +88,21 @@ fn import_svg_page_index(state: &EditorState, page_id: Option<&str>) -> Option, + target_parent: &NodeId, +) -> bool { + state.cmd_insert_node(kind, name, x, y, width, height, fill_hex, target_parent) +} + fn apply_import_svg_on_active_page( state: &mut EditorState, svg: &str, @@ -139,7 +154,32 @@ impl EditorState { width, height, fill_hex, - } => self.cmd_insert_node(&kind, &name, x, y, width, height, &fill_hex), + target_parent, + page_id, + } => { + let Some(target_page_index) = command_page_index(self, page_id.as_deref()) else { + return false; + }; + let original_page_index = self.ui.active_page_index; + if page_id.is_some() { + self.ui.active_page_index = target_page_index; + } + let changed = apply_insert_node_on_active_page( + self, + &kind, + &name, + x, + y, + width, + height, + &fill_hex, + &target_parent, + ); + if page_id.is_some() && target_page_index != original_page_index { + self.ui.active_page_index = original_page_index; + } + changed + } EditorCommand::UpdateNode { node_id, x, @@ -453,8 +493,7 @@ impl EditorState { target_parent, page_id, } => { - let Some(target_page_index) = import_svg_page_index(self, page_id.as_deref()) - else { + let Some(target_page_index) = command_page_index(self, page_id.as_deref()) else { return false; }; let original_page_index = self.ui.active_page_index; diff --git a/crates/op-editor-core/src/command_insert_tests.rs b/crates/op-editor-core/src/command_insert_tests.rs new file mode 100644 index 000000000..f662512a3 --- /dev/null +++ b/crates/op-editor-core/src/command_insert_tests.rs @@ -0,0 +1,129 @@ +//! `EditorCommand::InsertNode` parent/page targeting tests. + +#![cfg(test)] + +use crate::command::EditorCommand; +use crate::node_id::NodeId; +use crate::pen_node_ext::PenNodeExt; +use crate::test_support::{frame, state_with}; +use jian_ops_schema::page::PenPage; + +#[test] +fn insert_node_appends_a_fresh_leaf() { + let mut s = state_with(vec![]); + assert!(s.apply(EditorCommand::InsertNode { + kind: "rect".into(), + name: "Box".into(), + x: 10, + y: 20, + width: 100, + height: 60, + fill_hex: Some("#ff0000".into()), + target_parent: NodeId::NONE, + page_id: None, + })); + assert_eq!(s.active_children().len(), 1); + let node = &s.active_children()[0]; + assert_eq!(node.base().name.as_deref(), Some("Box")); + assert_eq!(node.base().x, Some(10.0)); +} + +#[test] +fn insert_node_rejects_unknown_kind() { + let mut s = state_with(vec![]); + assert!(!s.apply(EditorCommand::InsertNode { + kind: "bogus".into(), + name: "X".into(), + x: 0, + y: 0, + width: 10, + height: 10, + fill_hex: None, + target_parent: NodeId::NONE, + page_id: None, + })); + assert!(s.active_children().is_empty()); +} + +#[test] +fn insert_node_atomic_on_bad_fill_hex() { + let mut s = state_with(vec![]); + assert!(!s.apply(EditorCommand::InsertNode { + kind: "rect".into(), + name: "X".into(), + x: 0, + y: 0, + width: 10, + height: 10, + fill_hex: Some("not-hex".into()), + target_parent: NodeId::NONE, + page_id: None, + })); + assert!(s.active_children().is_empty()); + assert_eq!(s.max_node_id(), 0); +} + +#[test] +fn insert_node_can_insert_under_parent_container() { + let mut s = state_with(vec![frame("n1", "Frame", 0.0, 0.0, 100.0, 100.0, vec![])]); + + assert!(s.apply(EditorCommand::InsertNode { + kind: "rect".into(), + name: "Child".into(), + x: 1, + y: 2, + width: 3, + height: 4, + fill_hex: None, + target_parent: NodeId::new("n1"), + page_id: None, + })); + + assert_eq!(s.active_children().len(), 1); + let children = s.active_children()[0].children().expect("parent children"); + assert_eq!(children.len(), 1); + assert_eq!(children[0].base().name.as_deref(), Some("Child")); +} + +#[test] +fn insert_node_can_insert_on_requested_page_without_switching_active_page() { + let mut s = state_with(vec![]); + s.doc.pages = Some(vec![ + PenPage { + id: "page-1".into(), + name: "Page 1".into(), + children: vec![frame("n1", "Current", 0.0, 0.0, 100.0, 100.0, vec![])], + state: None, + lifecycle: None, + }, + PenPage { + id: "page-2".into(), + name: "Page 2".into(), + children: Vec::new(), + state: None, + lifecycle: None, + }, + ]); + s.ui.active_page_index = 0; + + assert!(s.apply(EditorCommand::InsertNode { + kind: "rect".into(), + name: "Other Page".into(), + x: 1, + y: 2, + width: 3, + height: 4, + fill_hex: None, + target_parent: NodeId::NONE, + page_id: Some("page-2".into()), + })); + + let pages = s.doc.pages.as_ref().expect("pages"); + assert_eq!(pages[0].children.len(), 1); + assert_eq!(pages[1].children.len(), 1); + assert_eq!( + pages[1].children[0].base().name.as_deref(), + Some("Other Page") + ); + assert_eq!(s.ui.active_page_index, 0); +} diff --git a/crates/op-editor-core/src/command_node.rs b/crates/op-editor-core/src/command_node.rs index c869a0150..1a22a0c4e 100644 --- a/crates/op-editor-core/src/command_node.rs +++ b/crates/op-editor-core/src/command_node.rs @@ -264,6 +264,7 @@ impl EditorState { width: i32, height: i32, fill_hex: &Option, + target_parent: &NodeId, ) -> bool { if !kind_is_valid(kind) || width < 0 || height < 0 { return false; @@ -274,6 +275,12 @@ impl EditorState { return false; } } + if target_parent.is_real() { + match walkers::find_node(self.active_children(), target_parent) { + Some(parent) if parent.is_container() => {} + _ => return false, + } + } let Some(new_id) = self.next_node_id() else { return false; }; @@ -284,7 +291,18 @@ impl EditorState { if let Some(hex) = fill_hex { set_primary_fill_hex(&mut node, hex); } - self.active_children_mut().push(node); + if target_parent.is_real() { + let root = self.active_children_mut(); + let Some(parent) = walkers::find_node_mut(root, target_parent) else { + return false; + }; + let Some(children) = parent.children_mut() else { + return false; + }; + children.push(node); + } else { + self.active_children_mut().push(node); + } true } diff --git a/crates/op-editor-core/src/command_tests.rs b/crates/op-editor-core/src/command_tests.rs index b0ef24247..879414dbe 100644 --- a/crates/op-editor-core/src/command_tests.rs +++ b/crates/op-editor-core/src/command_tests.rs @@ -22,58 +22,6 @@ fn id(s: &str) -> NodeId { NodeId::new(s) } -// --- InsertNode ------------------------------------------------------ - -#[test] -fn insert_node_appends_a_fresh_leaf() { - let mut s = state_with(vec![]); - assert!(s.apply(EditorCommand::InsertNode { - kind: "rect".into(), - name: "Box".into(), - x: 10, - y: 20, - width: 100, - height: 60, - fill_hex: Some("#ff0000".into()), - })); - assert_eq!(s.active_children().len(), 1); - let node = &s.active_children()[0]; - assert_eq!(node.base().name.as_deref(), Some("Box")); - assert_eq!(node.base().x, Some(10.0)); -} - -#[test] -fn insert_node_rejects_unknown_kind() { - let mut s = state_with(vec![]); - assert!(!s.apply(EditorCommand::InsertNode { - kind: "bogus".into(), - name: "X".into(), - x: 0, - y: 0, - width: 10, - height: 10, - fill_hex: None, - })); - assert!(s.active_children().is_empty()); -} - -#[test] -fn insert_node_atomic_on_bad_fill_hex() { - let mut s = state_with(vec![]); - assert!(!s.apply(EditorCommand::InsertNode { - kind: "rect".into(), - name: "X".into(), - x: 0, - y: 0, - width: 10, - height: 10, - fill_hex: Some("not-hex".into()), - })); - // No id minted, no node appended. - assert!(s.active_children().is_empty()); - assert_eq!(s.max_node_id(), 0); -} - // --- UpdateNode ------------------------------------------------------ #[test] diff --git a/crates/op-editor-core/src/lib.rs b/crates/op-editor-core/src/lib.rs index 64941c25e..250433f80 100644 --- a/crates/op-editor-core/src/lib.rs +++ b/crates/op-editor-core/src/lib.rs @@ -57,6 +57,8 @@ pub mod walkers; #[cfg(test)] mod command_attr_tests; #[cfg(test)] +mod command_insert_tests; +#[cfg(test)] mod command_style_replace_tests; #[cfg(test)] mod command_subtree_tests; diff --git a/crates/op-host-desktop/src/mcp_serve.rs b/crates/op-host-desktop/src/mcp_serve.rs index c47a1c5f1..c0e11abbf 100644 --- a/crates/op-host-desktop/src/mcp_serve.rs +++ b/crates/op-host-desktop/src/mcp_serve.rs @@ -778,7 +778,7 @@ const TOOL_SCHEMAS: &[&str] = &[ r#"{"name":"duplicate_page","description":"Clone a page and switch the active page to the clone. Accepts TS-style pageId or legacy index; optional name overrides the clone name.","inputSchema":{"type":"object","properties":{"pageId":{"type":"string","description":"Page id returned by list_pages"},"index":{"type":"string","description":"0-based page index (legacy)"},"name":{"type":"string","description":"Optional clone name"}}}}"#, r#"{"name":"reorder_page","description":"Move a page. Accepts TS-style pageId + index or legacy from + to. Target is clamped to [0, page_count).","inputSchema":{"type":"object","properties":{"pageId":{"type":"string","description":"Page id returned by list_pages"},"index":{"type":"string","description":"0-based target page index"},"from":{"type":"string","description":"0-based source page index (legacy)"},"to":{"type":"string","description":"0-based target page index (legacy)"}}}}"#, r#"{"name":"set_active_axis_value","description":"Pin a theme axis to one of its allowed values.","inputSchema":{"type":"object","properties":{"axis":{"type":"string"},"value":{"type":"string"}},"required":["axis","value"]}}"#, - r#"{"name":"insert_node","description":"Create a new leaf node on the active page.","inputSchema":{"type":"object","properties":{"kind":{"type":"string","enum":["frame","group","rect","ellipse","polygon","line","text","path"]},"name":{"type":"string"},"x":{"type":"string"},"y":{"type":"string"},"width":{"type":"string"},"height":{"type":"string"},"fill_hex":{"type":"string"}},"required":["kind","name","x","y","width","height"]}}"#, + r#"{"name":"insert_node","description":"Create a new leaf node on the active or requested page.","inputSchema":{"type":"object","properties":{"kind":{"type":"string","enum":["frame","group","rect","ellipse","polygon","line","text","path"]},"name":{"type":"string"},"x":{"type":"string"},"y":{"type":"string"},"width":{"type":"string"},"height":{"type":"string"},"fill_hex":{"type":"string"},"parent":{"type":"string","description":"optional parent node id; empty/0/root omitted = page root"},"pageId":{"type":"string","description":"optional target page id or legacy page index; omitted = active page"}},"required":["kind","name","x","y","width","height"]}}"#, r#"{"name":"update_node","description":"Patch fields on an existing node. Pass any subset of x/y/width/height/name/fill_hex.","inputSchema":{"type":"object","properties":{"node_id":{"type":"string"},"x":{"type":"string"},"y":{"type":"string"},"width":{"type":"string"},"height":{"type":"string"},"name":{"type":"string"},"fill_hex":{"type":"string"}},"required":["node_id"]}}"#, r#"{"name":"import_svg","description":"Parse an SVG document and insert the resulting nodes on the active or requested page. Supports rect/circle/ellipse/line/polyline/polygon and path (M/L/H/V/C/S/Q/T/Z); /transforms/CSS are skipped. x/y (optional, default 0) offset the imported nodes in doc-px; parent inserts under a container node.","inputSchema":{"type":"object","properties":{"svg":{"type":"string","description":"SVG document text"},"x":{"type":"string","description":"i32 doc-px x offset (default 0)"},"y":{"type":"string","description":"i32 doc-px y offset (default 0)"},"parent":{"type":"string","description":"optional parent node id; empty/0/root omitted = page root"},"pageId":{"type":"string","description":"optional target page id or legacy page index; omitted = active page"}},"required":["svg"]}}"#, r#"{"name":"delete_node","description":"Remove a node + descendants from its parent.","inputSchema":{"type":"object","properties":{"node_id":{"type":"string"}},"required":["node_id"]}}"#, diff --git a/crates/op-host-desktop/src/mcp_serve/tests.rs b/crates/op-host-desktop/src/mcp_serve/tests.rs index 52ba56d04..a0f4f6ec4 100644 --- a/crates/op-host-desktop/src/mcp_serve/tests.rs +++ b/crates/op-host-desktop/src/mcp_serve/tests.rs @@ -225,6 +225,8 @@ fn find_empty_space_returns_padded_position_from_active_page_bounds() { width: 100, height: 50, fill_hex: None, + target_parent: op_editor_core::NodeId::NONE, + page_id: None, })); assert!(state.apply(EditorCommand::InsertNode { kind: "rect".into(), @@ -234,6 +236,8 @@ fn find_empty_space_returns_padded_position_from_active_page_bounds() { width: 50, height: 40, fill_hex: None, + target_parent: op_editor_core::NodeId::NONE, + page_id: None, })); let line = r#"{"jsonrpc":"2.0","id":9,"method":"tools/call","params":{"name":"find_empty_space","arguments":{"direction":"right","width":"320","height":"240"}}}"#; let response = process_message_with_applier(&mut state, line, |_, _| false) @@ -255,6 +259,8 @@ fn read_nodes_accepts_structured_ids_over_mcp() { width: 100, height: 50, fill_hex: None, + target_parent: op_editor_core::NodeId::NONE, + page_id: None, })); let node_id = state.active_children()[0].base().id.clone(); let line = format!( diff --git a/crates/op-mcp/src/write_tools.rs b/crates/op-mcp/src/write_tools.rs index 60c0e416c..00c1ef7bc 100644 --- a/crates/op-mcp/src/write_tools.rs +++ b/crates/op-mcp/src/write_tools.rs @@ -188,6 +188,19 @@ impl McpTool for InsertNode { } Some(s) => Some(s.clone()), }; + let target_parent = args + .get("parent") + .or_else(|| args.get("parent_id")) + .or_else(|| args.get("target_parent_id")) + .map(|s| root_or_node_id(s)) + .unwrap_or(NodeId::NONE); + let page_id = args + .get("pageId") + .or_else(|| args.get("page_id")) + .or_else(|| args.get("page")) + .map(|s| s.trim()) + .filter(|s| !s.is_empty()) + .map(str::to_string); let mut out = BTreeMap::new(); out.insert("wrote".into(), "true".into()); ToolOutcome::OkWithCommand( @@ -200,6 +213,8 @@ impl McpTool for InsertNode { width, height, fill_hex, + target_parent, + page_id, }, ) } diff --git a/crates/op-mcp/src/write_tools_tests.rs b/crates/op-mcp/src/write_tools_tests.rs index c28aab09f..13143a7a9 100644 --- a/crates/op-mcp/src/write_tools_tests.rs +++ b/crates/op-mcp/src/write_tools_tests.rs @@ -11,7 +11,7 @@ use super::test_fixtures::{add_theme_axis, add_variable, sample, state_with}; use super::write_tools::*; use super::{EditorCommand, McpTool, ToolErrorCode, ToolOutcome}; use jian_ops_schema::variable::{VariableKind, VariableScalar}; -use op_editor_core::EditorState; +use op_editor_core::{EditorState, NodeId}; use std::collections::BTreeMap; fn state_with_color_var(name: &str, hex: &str) -> EditorState { @@ -297,6 +297,8 @@ fn insert_node_returns_command_with_parsed_args() { width, height, fill_hex, + target_parent, + page_id, }, ) => { assert_eq!(kind, "rect"); @@ -306,11 +308,42 @@ fn insert_node_returns_command_with_parsed_args() { assert_eq!(width, 100); assert_eq!(height, 50); assert_eq!(fill_hex.as_deref(), Some("#ff0000")); + assert!(!target_parent.is_real()); + assert_eq!(page_id, None); } other => panic!("expected InsertNode command, got {other:?}"), } } +#[test] +fn insert_node_accepts_ts_parent_and_page_args() { + let tool = insert_node_snapshot(); + let mut args = BTreeMap::new(); + args.insert("kind".into(), "rect".into()); + args.insert("name".into(), "Nested".into()); + args.insert("x".into(), "1".into()); + args.insert("y".into(), "2".into()); + args.insert("width".into(), "3".into()); + args.insert("height".into(), "4".into()); + args.insert("parent".into(), "n10".into()); + args.insert("pageId".into(), "page-2".into()); + + match tool.call(&args) { + ToolOutcome::OkWithCommand( + _, + EditorCommand::InsertNode { + target_parent, + page_id, + .. + }, + ) => { + assert_eq!(target_parent.as_str(), "n10"); + assert_eq!(page_id.as_deref(), Some("page-2")); + } + other => panic!("expected InsertNode command with parent/page, got {other:?}"), + } +} + #[test] fn insert_node_command_applies_through_editor_state() { let mut s = state_with(vec![]); @@ -323,6 +356,8 @@ fn insert_node_command_applies_through_editor_state() { width: 200, height: 150, fill_hex: Some("#00ff00".into()), + target_parent: NodeId::NONE, + page_id: None, })); assert_eq!(s.active_children().len(), before + 1); }