From a26e1b0f48e5f078efa67f0e30c0dca7631c0229 Mon Sep 17 00:00:00 2001 From: Kayshen-X Date: Tue, 2 Jun 2026 09:42:01 +0800 Subject: [PATCH] fix(mcp): support ts move copy targets --- crates/op-cli/src/cli_node_tests.rs | 53 ++++++ crates/op-cli/src/main.rs | 16 +- crates/op-cli/src/usage.txt | 5 +- crates/op-editor-core/src/command.rs | 8 + crates/op-editor-core/src/command_apply.rs | 35 +++- crates/op-editor-core/src/command_node.rs | 79 +++++++-- .../src/command_reparent_tests.rs | 108 +++++++++++++ crates/op-editor-core/src/command_tests.rs | 8 + crates/op-editor-core/src/lib.rs | 2 + crates/op-host-desktop/src/mcp_serve.rs | 4 +- crates/op-mcp/src/batch_design_tests.rs | 26 +++ crates/op-mcp/src/batch_direct_ops.rs | 18 ++- crates/op-mcp/src/copy_node_tests.rs | 31 +++- crates/op-mcp/src/lib.rs | 9 +- crates/op-mcp/src/reparent_tools.rs | 153 ++++++++++++++++++ crates/op-mcp/src/write_tools.rs | 82 +--------- crates/op-mcp/src/write_tools_tests.rs | 34 ++++ .../src/dashboard_columns/height_reorder.rs | 2 + 18 files changed, 555 insertions(+), 118 deletions(-) create mode 100644 crates/op-editor-core/src/command_reparent_tests.rs create mode 100644 crates/op-mcp/src/reparent_tools.rs diff --git a/crates/op-cli/src/cli_node_tests.rs b/crates/op-cli/src/cli_node_tests.rs index d60254c4f..d86b1a7d6 100644 --- a/crates/op-cli/src/cli_node_tests.rs +++ b/crates/op-cli/src/cli_node_tests.rs @@ -79,3 +79,56 @@ fn parse_args_maps_delete_page_flag_to_ts_page_arg() { } ); } + +#[test] +fn parse_args_maps_move_page_and_index_flags_to_ts_args() { + let p = parse_args(&[ + "move".to_string(), + "n10".to_string(), + "--parent".to_string(), + "n20".to_string(), + "--index".to_string(), + "2".to_string(), + "--page".to_string(), + "page-2".to_string(), + ]) + .expect("parse move"); + + assert_eq!( + p.command, + Command::ToolCall { + tool: "move_node".to_string(), + args: vec![ + ("node_id".to_string(), "n10".to_string()), + ("target_parent_id".to_string(), "n20".to_string()), + ("index".to_string(), "2".to_string()), + ("pageId".to_string(), "page-2".to_string()), + ], + } + ); +} + +#[test] +fn parse_args_maps_copy_page_flag_to_ts_args() { + let p = parse_args(&[ + "copy".to_string(), + "n10".to_string(), + "--parent".to_string(), + "n20".to_string(), + "--page".to_string(), + "page-2".to_string(), + ]) + .expect("parse copy"); + + assert_eq!( + p.command, + Command::ToolCall { + tool: "copy_node".to_string(), + args: vec![ + ("node_id".to_string(), "n10".to_string()), + ("target_parent_id".to_string(), "n20".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 a94b77007..a9984c5d9 100644 --- a/crates/op-cli/src/main.rs +++ b/crates/op-cli/src/main.rs @@ -353,13 +353,19 @@ fn map_reparent(tool: &str, positionals: &[String], flags: &Flags) -> Result [--parent ]", + "Usage: op move/copy [--parent ] [--page PAGE]", )?; let parent = flag_value(flags, "parent").unwrap_or_default(); - tool_call( - tool, - vec![pair("node_id", id), pair("target_parent_id", parent)], - ) + let mut pairs = vec![pair("node_id", id), pair("target_parent_id", parent)]; + if tool == "move_node" { + if let Some(index) = flag_value(flags, "index") { + pairs.push(pair("index", index)); + } + } + if let Some(page) = flag_value(flags, "page") { + pairs.push(pair("pageId", page)); + } + tool_call(tool, pairs) } fn map_design_like(tool: &str, payload: Option<&String>) -> Result { diff --git a/crates/op-cli/src/usage.txt b/crates/op-cli/src/usage.txt index dcc118ad1..4c32707d5 100644 --- a/crates/op-cli/src/usage.txt +++ b/crates/op-cli/src/usage.txt @@ -20,8 +20,9 @@ COMMON COMMANDS: patch x/y/width/height/name/fill op delete [--page PAGE] delete a node op read-nodes [ids] [--depth N] [--vars] [--page P] [--file F] - op move [--parent P] reparent a node (empty parent = page root) - op copy [--parent P] deep-copy a node + op move [--parent P] [--index N] [--page PAGE] + reparent a node (empty parent = page root) + op copy [--parent P] [--page PAGE] deep-copy a node op replace replace with a leaf node op design call batch_design(nodes_json) op design:skeleton diff --git a/crates/op-editor-core/src/command.rs b/crates/op-editor-core/src/command.rs index 1048f3516..b78254fae 100644 --- a/crates/op-editor-core/src/command.rs +++ b/crates/op-editor-core/src/command.rs @@ -157,12 +157,20 @@ pub enum EditorCommand { MoveNode { node_id: NodeId, target_parent: NodeId, + /// `None` moves on the active page; `Some` targets a page id + /// or legacy page index. + page_id: Option, + /// Optional insertion index within the target parent/root. + index: Option, }, /// Deep-clone a node + subtree under a new parent (`NONE` = active /// page root). Fresh ids minted past the id space. CopyNode { node_id: NodeId, target_parent: NodeId, + /// `None` copies on the active page; `Some` targets a page id + /// or legacy page index. + page_id: Option, }, /// Replace an existing node with a freshly-built leaf at the same /// slot. `drop_children` is the destructive-swap guard: replacing a diff --git a/crates/op-editor-core/src/command_apply.rs b/crates/op-editor-core/src/command_apply.rs index 60b1d121c..c500384f9 100644 --- a/crates/op-editor-core/src/command_apply.rs +++ b/crates/op-editor-core/src/command_apply.rs @@ -133,7 +133,7 @@ fn apply_import_svg_on_active_page( else { return false; }; - imported_root.is_real() && state.cmd_move_node(&imported_root, target_parent) + imported_root.is_real() && state.cmd_move_node(&imported_root, target_parent, None) } else { true } @@ -220,11 +220,40 @@ impl EditorState { EditorCommand::MoveNode { node_id, target_parent, - } => self.cmd_move_node(&node_id, &target_parent), + page_id, + index, + } => { + 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 = self.cmd_move_node(&node_id, &target_parent, index); + if page_id.is_some() && target_page_index != original_page_index { + self.ui.active_page_index = original_page_index; + } + changed + } EditorCommand::CopyNode { node_id, target_parent, - } => self.cmd_copy_node(&node_id, &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 = self.cmd_copy_node(&node_id, &target_parent); + if page_id.is_some() && target_page_index != original_page_index { + self.ui.active_page_index = original_page_index; + } + changed + } EditorCommand::ReplaceNode { node_id, kind, diff --git a/crates/op-editor-core/src/command_node.rs b/crates/op-editor-core/src/command_node.rs index 1a22a0c4e..3257acddb 100644 --- a/crates/op-editor-core/src/command_node.rs +++ b/crates/op-editor-core/src/command_node.rs @@ -236,6 +236,50 @@ pub fn replace_node_in_children( false } +#[allow(clippy::result_large_err)] +fn insert_into_parent_or_root( + children: &mut Vec, + parent: &NodeId, + node: PenNode, + index: Option, +) -> Result<(), PenNode> { + if !parent.is_real() { + let idx = index.unwrap_or(children.len()).min(children.len()); + children.insert(idx, node); + return Ok(()); + } + insert_into_parent(children, parent, node, index) +} + +#[allow(clippy::result_large_err)] +fn insert_into_parent( + children: &mut [PenNode], + parent: &NodeId, + node: PenNode, + index: Option, +) -> Result<(), PenNode> { + if let Some(idx) = children.iter().position(|n| n.id_str() == parent.as_str()) { + match children[idx].children_mut() { + Some(grand) => { + let insert_idx = index.unwrap_or(grand.len()).min(grand.len()); + grand.insert(insert_idx, node); + return Ok(()); + } + None => return Err(node), + } + } + let mut carry = node; + for child in children.iter_mut() { + if let Some(grand) = child.children_mut() { + match insert_into_parent(grand, parent, carry, index) { + Ok(()) => return Ok(()), + Err(returned) => carry = returned, + } + } + } + Err(carry) +} + impl EditorState { /// Compute the numeric seed for the next editor-minted `n{N}` id — /// `max_node_id() + 1`. `None` on `u64` exhaustion. @@ -374,7 +418,12 @@ impl EditorState { /// `MoveNode` — reparent a node. A `NONE` target reparents to the /// active page root; a real target must resolve + must not create /// a cycle (target is a descendant of the moved node). - pub(crate) fn cmd_move_node(&mut self, node_id: &NodeId, target_parent: &NodeId) -> bool { + pub(crate) fn cmd_move_node( + &mut self, + node_id: &NodeId, + target_parent: &NodeId, + index: Option, + ) -> bool { if !node_id.is_real() || target_parent == node_id { return false; } @@ -389,7 +438,10 @@ impl EditorState { if walkers::descendant_contains(src, target_parent) { return false; } - if walkers::find_node(children, target_parent).is_none() { + let Some(target) = walkers::find_node(children, target_parent) else { + return false; + }; + if target.children().is_none() { return false; } } @@ -398,12 +450,7 @@ impl EditorState { let Some(detached) = walkers::extract_node(children, node_id) else { return false; }; - if target_parent.is_real() { - walkers::append_into(children, target_parent, detached).is_ok() - } else { - children.push(detached); - true - } + insert_into_parent_or_root(children, target_parent, detached, index).is_ok() } /// `CopyNode` — deep-clone a node + subtree under a new parent @@ -418,8 +465,13 @@ impl EditorState { if walkers::find_node(children, node_id).is_none() { return false; } - if target_parent.is_real() && walkers::find_node(children, target_parent).is_none() { - return false; + if target_parent.is_real() { + let Some(target) = walkers::find_node(children, target_parent) else { + return false; + }; + if target.children().is_none() { + return false; + } } } let Some(mut next_id) = self.next_node_id_seed() else { @@ -433,12 +485,7 @@ impl EditorState { walkers::deep_clone_with_new_ids(src, &mut next_id, &mut taken) }; let children = self.active_children_mut(); - if target_parent.is_real() { - walkers::append_into(children, target_parent, clone).is_ok() - } else { - children.push(clone); - true - } + insert_into_parent_or_root(children, target_parent, clone, None).is_ok() } /// `ReplaceNode` — swap an existing node for a freshly-built leaf at diff --git a/crates/op-editor-core/src/command_reparent_tests.rs b/crates/op-editor-core/src/command_reparent_tests.rs new file mode 100644 index 000000000..3f71533d2 --- /dev/null +++ b/crates/op-editor-core/src/command_reparent_tests.rs @@ -0,0 +1,108 @@ +//! `EditorCommand::MoveNode` / `CopyNode` 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, rect, state_with}; +use jian_ops_schema::page::PenPage; + +fn id(s: &str) -> NodeId { + NodeId::new(s) +} + +#[test] +fn move_node_can_target_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![rect("n1", "Current", 0.0, 0.0, 10.0, 10.0)], + state: None, + lifecycle: None, + }, + PenPage { + id: "page-2".into(), + name: "Page 2".into(), + children: vec![ + frame("n2", "Target", 0.0, 0.0, 100.0, 100.0, Vec::new()), + rect("n3", "Moved", 0.0, 0.0, 10.0, 10.0), + ], + state: None, + lifecycle: None, + }, + ]); + s.ui.active_page_index = 0; + + assert!(s.apply(EditorCommand::MoveNode { + node_id: id("n3"), + target_parent: id("n2"), + page_id: Some("page-2".into()), + index: None, + })); + + let pages = s.doc.pages.as_ref().expect("pages"); + let target_children = pages[1].children[0].children().expect("frame children"); + assert_eq!(target_children.len(), 1); + assert_eq!(target_children[0].id_str(), "n3"); + assert_eq!(s.ui.active_page_index, 0); +} + +#[test] +fn move_node_can_insert_at_requested_root_index() { + let mut s = state_with(vec![ + rect("n1", "A", 0.0, 0.0, 10.0, 10.0), + rect("n2", "B", 0.0, 0.0, 10.0, 10.0), + rect("n3", "C", 0.0, 0.0, 10.0, 10.0), + ]); + + assert!(s.apply(EditorCommand::MoveNode { + node_id: id("n3"), + target_parent: NodeId::NONE, + page_id: None, + index: Some(1), + })); + + let ids: Vec<&str> = s.active_children().iter().map(|n| n.id_str()).collect(); + assert_eq!(ids, vec!["n1", "n3", "n2"]); +} + +#[test] +fn copy_node_can_target_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![rect("n1", "Current", 0.0, 0.0, 10.0, 10.0)], + state: None, + lifecycle: None, + }, + PenPage { + id: "page-2".into(), + name: "Page 2".into(), + children: vec![ + frame("n2", "Target", 0.0, 0.0, 100.0, 100.0, Vec::new()), + rect("n3", "Source", 0.0, 0.0, 10.0, 10.0), + ], + state: None, + lifecycle: None, + }, + ]); + s.ui.active_page_index = 0; + + assert!(s.apply(EditorCommand::CopyNode { + node_id: id("n3"), + target_parent: id("n2"), + page_id: Some("page-2".into()), + })); + + let pages = s.doc.pages.as_ref().expect("pages"); + let target_children = pages[1].children[0].children().expect("frame children"); + assert_eq!(target_children.len(), 1); + assert_ne!(target_children[0].id_str(), "n3"); + assert_eq!(target_children[0].base().name.as_deref(), Some("Source")); + assert_eq!(s.ui.active_page_index, 0); +} diff --git a/crates/op-editor-core/src/command_tests.rs b/crates/op-editor-core/src/command_tests.rs index fc816a82e..a30631d22 100644 --- a/crates/op-editor-core/src/command_tests.rs +++ b/crates/op-editor-core/src/command_tests.rs @@ -29,6 +29,8 @@ fn move_node_reparents_into_container() { assert!(s.apply(EditorCommand::MoveNode { node_id: id("n11"), target_parent: id("n12"), + page_id: None, + index: None, })); let group = find_node(s.active_children(), &id("n12")).unwrap(); assert!(group @@ -45,6 +47,8 @@ fn move_node_rejects_cycle() { assert!(!s.apply(EditorCommand::MoveNode { node_id: id("n10"), target_parent: id("n12"), + page_id: None, + index: None, })); // n10 still at root. assert!(s.active_children().iter().any(|c| c.id_str() == "n10")); @@ -56,6 +60,8 @@ fn move_node_to_page_root_with_none_target() { assert!(s.apply(EditorCommand::MoveNode { node_id: id("n11"), target_parent: NodeId::NONE, + page_id: None, + index: None, })); // n11 now at the page root, alongside n10. assert!(s.active_children().iter().any(|c| c.id_str() == "n11")); @@ -70,6 +76,7 @@ fn copy_node_clones_subtree_with_fresh_ids() { assert!(s.apply(EditorCommand::CopyNode { node_id: id("n12"), target_parent: NodeId::NONE, + page_id: None, })); // A clone landed at the page root; its id is fresh. assert!(s.max_node_id() > pre_max); @@ -83,6 +90,7 @@ fn copy_node_rejects_unknown_source() { assert!(!s.apply(EditorCommand::CopyNode { node_id: id("ghost"), target_parent: NodeId::NONE, + page_id: None, })); } diff --git a/crates/op-editor-core/src/lib.rs b/crates/op-editor-core/src/lib.rs index e2dd78c13..5f890172e 100644 --- a/crates/op-editor-core/src/lib.rs +++ b/crates/op-editor-core/src/lib.rs @@ -63,6 +63,8 @@ mod command_delete_tests; #[cfg(test)] mod command_insert_tests; #[cfg(test)] +mod command_reparent_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 8a2984f39..d7ffc1227 100644 --- a/crates/op-host-desktop/src/mcp_serve.rs +++ b/crates/op-host-desktop/src/mcp_serve.rs @@ -782,8 +782,8 @@ const TOOL_SCHEMAS: &[&str] = &[ r#"{"name":"update_node","description":"Patch fields on an existing node. Accepts Rust flat fields or TS-style nodeId + data object.","inputSchema":{"type":"object","properties":{"node_id":{"type":"string"},"nodeId":{"type":"string"},"x":{"type":"string"},"y":{"type":"string"},"width":{"type":"string"},"height":{"type":"string"},"name":{"type":"string"},"fill_hex":{"type":"string"},"data":{"type":"object","description":"TS-style patch data: name/x/y/width/height/fill"},"pageId":{"type":"string","description":"optional target page id or legacy page index; omitted = active page"}}}}"#, 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. Accepts Rust node_id or TS nodeId, plus optional pageId.","inputSchema":{"type":"object","properties":{"node_id":{"type":"string"},"nodeId":{"type":"string"},"pageId":{"type":"string","description":"optional target page id or legacy page index; omitted = active page"}}}}"#, - r#"{"name":"move_node","description":"Reparent a node. target_parent_id=0 puts it at the active page root.","inputSchema":{"type":"object","properties":{"node_id":{"type":"string"},"target_parent_id":{"type":"string"}},"required":["node_id","target_parent_id"]}}"#, - r#"{"name":"copy_node","description":"Deep-clone a subtree with fresh ids under a new parent.","inputSchema":{"type":"object","properties":{"node_id":{"type":"string"},"target_parent_id":{"type":"string"}},"required":["node_id","target_parent_id"]}}"#, + r#"{"name":"move_node","description":"Reparent a node. Accepts Rust node_id/target_parent_id or TS nodeId/parent, plus optional index and pageId.","inputSchema":{"type":"object","properties":{"node_id":{"type":"string"},"nodeId":{"type":"string"},"target_parent_id":{"type":"string"},"parent":{"type":"string","description":"target parent node id; empty/0/null/root omitted = page root"},"index":{"type":"string","description":"optional insertion index within target parent/root"},"pageId":{"type":"string","description":"optional target page id or legacy page index; omitted = active page"}}}}"#, + r#"{"name":"copy_node","description":"Deep-clone a subtree with fresh ids under a new parent. Accepts Rust node_id or TS sourceId/nodeId, plus optional parent/pageId.","inputSchema":{"type":"object","properties":{"node_id":{"type":"string"},"sourceId":{"type":"string"},"nodeId":{"type":"string"},"target_parent_id":{"type":"string"},"parent":{"type":"string","description":"target parent node id; empty/0/null/root omitted = page root"},"pageId":{"type":"string","description":"optional target page id or legacy page index; omitted = active page"}}}}"#, r#"{"name":"replace_node","description":"Swap an existing node at the same parent slot with a freshly-built leaf. Set drop_children=true to discard a container's subtree.","inputSchema":{"type":"object","properties":{"node_id":{"type":"string"},"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"},"drop_children":{"type":"string","enum":["true","false"]}},"required":["node_id","kind","name","x","y","width","height"]}}"#, ]; diff --git a/crates/op-mcp/src/batch_design_tests.rs b/crates/op-mcp/src/batch_design_tests.rs index 8b3a9b5f6..bb5d8b651 100644 --- a/crates/op-mcp/src/batch_design_tests.rs +++ b/crates/op-mcp/src/batch_design_tests.rs @@ -177,11 +177,15 @@ fn batch_design_accepts_single_move_operation_without_index() { EditorCommand::MoveNode { node_id, target_parent, + page_id, + index, }, ) => { assert_eq!(result.get("count"), Some(&"1".to_string())); assert_eq!(node_id.as_str(), "n14"); assert!(!target_parent.is_real()); + assert!(page_id.is_none()); + assert!(index.is_none()); } other => panic!("expected MoveNode command, got {other:?}"), } @@ -238,6 +242,28 @@ fn batch_design_rejects_malformed_json() { } } +#[test] +fn batch_design_accepts_single_move_operation_with_index() { + let tool = batch_design_snapshot(); + let mut args = BTreeMap::new(); + args.insert("operations".into(), r##"M("n14", "n10", 2)"##.into()); + + match tool.call(&args) { + ToolOutcome::OkWithCommand( + _, + EditorCommand::MoveNode { + target_parent, + index, + .. + }, + ) => { + assert_eq!(target_parent.as_str(), "n10"); + assert_eq!(index, Some(2)); + } + other => panic!("expected indexed MoveNode command, got {other:?}"), + } +} + #[test] fn batch_insert_command_adds_all_nodes() { let mut s = sample(); diff --git a/crates/op-mcp/src/batch_direct_ops.rs b/crates/op-mcp/src/batch_direct_ops.rs index 18f8ede89..8baf29b3e 100644 --- a/crates/op-mcp/src/batch_direct_ops.rs +++ b/crates/op-mcp/src/batch_direct_ops.rs @@ -78,12 +78,22 @@ fn parse_move_operation(body: &str) -> Result { return Err("M() requires node id and parent id".into()); }; let rest = body[comma + 1..].trim(); - if find_top_level_char(rest, ',').is_some() { - return Err("M() index argument is not supported by the Rust MoveNode command".into()); - } + let (parent_raw, index) = if let Some(index_comma) = find_top_level_char(rest, ',') { + let raw_index = rest[index_comma + 1..] + .trim() + .trim_matches(|c| c == '"' || c == '\''); + let index = raw_index + .parse::() + .map_err(|_| format!("M() index must be a non-negative integer, got {raw_index:?}"))?; + (rest[..index_comma].trim(), Some(index)) + } else { + (rest, None) + }; Ok(EditorCommand::MoveNode { node_id: NodeId::new(&parse_ref_token(body[..comma].trim())?), - target_parent: parse_parent_node_id(rest)?, + target_parent: parse_parent_node_id(parent_raw)?, + page_id: None, + index, }) } diff --git a/crates/op-mcp/src/copy_node_tests.rs b/crates/op-mcp/src/copy_node_tests.rs index 275b906b7..41b3774bd 100644 --- a/crates/op-mcp/src/copy_node_tests.rs +++ b/crates/op-mcp/src/copy_node_tests.rs @@ -5,8 +5,8 @@ //! plus end-to-end `EditorState::apply` checks; the apply-path //! correctness is covered by `op-editor-core`'s `command_tests.rs`. +use super::reparent_tools::*; use super::test_fixtures::sample; -use super::write_tools::*; use super::{EditorCommand, McpTool, ToolErrorCode, ToolOutcome}; use op_editor_core::NodeId; use std::collections::BTreeMap; @@ -36,10 +36,12 @@ fn copy_node_validates_args() { EditorCommand::CopyNode { node_id, target_parent, + page_id, }, ) => { assert_eq!(node_id.as_str(), "n10"); assert_eq!(target_parent.as_str(), "n10"); + assert!(page_id.is_none()); } other => panic!("expected CopyNode, got {other:?}"), } @@ -59,6 +61,30 @@ fn copy_node_maps_zero_target_to_page_root() { } } +#[test] +fn copy_node_accepts_ts_source_parent_and_page_args() { + let tool = copy_node_snapshot(); + let mut args = BTreeMap::new(); + args.insert("sourceId".into(), "n12".into()); + args.insert("parent".into(), "n10".into()); + args.insert("pageId".into(), "page-2".into()); + match tool.call(&args) { + ToolOutcome::OkWithCommand( + _, + EditorCommand::CopyNode { + node_id, + target_parent, + page_id, + }, + ) => { + assert_eq!(node_id.as_str(), "n12"); + assert_eq!(target_parent.as_str(), "n10"); + assert_eq!(page_id.as_deref(), Some("page-2")); + } + other => panic!("expected CopyNode command with TS args, got {other:?}"), + } +} + #[test] fn copy_node_command_clones_to_page_root() { let mut s = sample(); @@ -66,6 +92,7 @@ fn copy_node_command_clones_to_page_root() { assert!(s.apply(EditorCommand::CopyNode { node_id: NodeId::new("n12"), target_parent: NodeId::NONE, + page_id: None, })); assert_eq!(s.active_children().len(), pre_root_len + 1); } @@ -76,9 +103,11 @@ fn copy_node_command_rejects_unknown_source_or_target() { assert!(!s.apply(EditorCommand::CopyNode { node_id: NodeId::new("n99999"), target_parent: NodeId::NONE, + page_id: None, })); assert!(!s.apply(EditorCommand::CopyNode { node_id: NodeId::new("n11"), target_parent: NodeId::new("n99999"), + page_id: None, })); } diff --git a/crates/op-mcp/src/lib.rs b/crates/op-mcp/src/lib.rs index 25206d34c..09090ca45 100644 --- a/crates/op-mcp/src/lib.rs +++ b/crates/op-mcp/src/lib.rs @@ -63,6 +63,7 @@ pub mod read_nodes; #[cfg(test)] mod read_nodes_tests; pub mod read_tools; +pub mod reparent_tools; #[cfg(test)] mod replace_node_tests; pub mod scalar_vars; @@ -157,6 +158,7 @@ pub use page_tools::{ }; pub use parser::parse_tool_call; pub use read_nodes::{read_nodes_snapshot, ReadNodes}; +pub use reparent_tools::{copy_node_snapshot, move_node_snapshot, CopyNode, MoveNode}; pub use scalar_vars::{ create_variable_snapshot, delete_variable_snapshot, rename_variable_snapshot, set_variable_boolean_snapshot, set_variable_number_snapshot, set_variable_string_snapshot, @@ -194,10 +196,9 @@ pub use tools::{ ListVariables, NodeRecord, SnapshotLayout, VariableRecord, }; pub use write_tools::{ - copy_node_snapshot, delete_node_snapshot, import_svg_snapshot, insert_node_snapshot, - move_node_snapshot, replace_node_snapshot, set_active_axis_value_snapshot, - set_variable_color_snapshot, update_node_snapshot, CopyNode, DeleteNode, ImportSvg, InsertNode, - MoveNode, ReplaceNode, SetActiveAxisValue, SetVariableColor, UpdateNode, + delete_node_snapshot, import_svg_snapshot, insert_node_snapshot, replace_node_snapshot, + set_active_axis_value_snapshot, set_variable_color_snapshot, update_node_snapshot, DeleteNode, + ImportSvg, InsertNode, ReplaceNode, SetActiveAxisValue, SetVariableColor, UpdateNode, }; /// JSON-RPC-style request id. Strings + integers both supported by diff --git a/crates/op-mcp/src/reparent_tools.rs b/crates/op-mcp/src/reparent_tools.rs new file mode 100644 index 000000000..cdf7d7ba2 --- /dev/null +++ b/crates/op-mcp/src/reparent_tools.rs @@ -0,0 +1,153 @@ +//! MCP move/copy tools. + +use std::collections::BTreeMap; + +use op_editor_core::NodeId; + +use crate::write_tools::root_or_node_id; +use crate::{EditorCommand, McpTool, ToolErrorCode, ToolOutcome}; + +/// First-party `move_node` tool — reparent a node. An empty +/// `target_parent_id` reparents to the active page root. +pub struct MoveNode; + +impl McpTool for MoveNode { + fn name(&self) -> &str { + "move_node" + } + + fn call(&self, args: &BTreeMap) -> ToolOutcome { + let node_id = match parse_node_id_any(args, &["node_id", "nodeId"], "node_id") { + Ok(v) => v, + Err(e) => return e, + }; + let target_parent = match parse_target_parent_arg(args) { + Ok(v) => v, + Err(e) => return e, + }; + if target_parent.is_real() && target_parent == node_id { + return ToolOutcome::Err( + ToolErrorCode::InvalidArgument, + "node_id and target_parent_id must differ".into(), + ); + } + let index = match parse_optional_usize_arg(args, "index") { + Ok(v) => v, + Err(e) => return e, + }; + let page_id = optional_page_id(args); + let mut out = BTreeMap::new(); + out.insert("wrote".into(), "true".into()); + ToolOutcome::OkWithCommand( + out, + EditorCommand::MoveNode { + node_id, + target_parent, + page_id, + index, + }, + ) + } +} + +pub fn move_node_snapshot() -> MoveNode { + MoveNode +} + +/// First-party `copy_node` tool — deep-clone a node + subtree under a +/// new parent. +pub struct CopyNode; + +impl McpTool for CopyNode { + fn name(&self) -> &str { + "copy_node" + } + + fn call(&self, args: &BTreeMap) -> ToolOutcome { + let node_id = match parse_node_id_any(args, &["sourceId", "node_id", "nodeId"], "node_id") { + Ok(v) => v, + Err(e) => return e, + }; + let target_parent = match parse_target_parent_arg(args) { + Ok(v) => v, + Err(e) => return e, + }; + let page_id = optional_page_id(args); + let mut out = BTreeMap::new(); + out.insert("wrote".into(), "true".into()); + ToolOutcome::OkWithCommand( + out, + EditorCommand::CopyNode { + node_id, + target_parent, + page_id, + }, + ) + } +} + +pub fn copy_node_snapshot() -> CopyNode { + CopyNode +} + +#[allow(clippy::result_large_err)] +fn parse_node_id_any( + args: &BTreeMap, + keys: &[&str], + message_key: &str, +) -> Result { + for key in keys { + if let Some(raw) = args.get(*key) { + return NodeId::new_opt(raw.as_str()).ok_or_else(|| { + ToolOutcome::Err( + ToolErrorCode::InvalidArgument, + format!("{key} must be a non-empty node id"), + ) + }); + } + } + Err(ToolOutcome::Err( + ToolErrorCode::MissingArgument, + format!("{message_key} is required"), + )) +} + +#[allow(clippy::result_large_err)] +fn parse_target_parent_arg(args: &BTreeMap) -> Result { + let Some(raw_target) = args + .get("parent") + .or_else(|| args.get("parent_id")) + .or_else(|| args.get("target_parent_id")) + else { + return Err(ToolOutcome::Err( + ToolErrorCode::MissingArgument, + "target_parent_id is required (\"\" or \"0\" = page root)".into(), + )); + }; + Ok(root_or_node_id(raw_target)) +} + +#[allow(clippy::result_large_err)] +fn parse_optional_usize_arg( + args: &BTreeMap, + key: &str, +) -> Result, ToolOutcome> { + let Some(raw) = args.get(key) else { + return Ok(None); + }; + raw.parse::().map(Some).map_err(|_| { + ToolOutcome::Err( + ToolErrorCode::InvalidArgument, + format!("{key} must be a non-negative integer, got {raw:?}"), + ) + }) +} + +fn optional_page_id(args: &BTreeMap) -> Option { + 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) +} diff --git a/crates/op-mcp/src/write_tools.rs b/crates/op-mcp/src/write_tools.rs index 4d5f87e1b..bc344c1d4 100644 --- a/crates/op-mcp/src/write_tools.rs +++ b/crates/op-mcp/src/write_tools.rs @@ -459,56 +459,12 @@ pub fn delete_node_snapshot() -> DeleteNode { DeleteNode } -/// First-party `move_node` tool — reparent a node. An empty -/// `target_parent_id` reparents to the active page root. -pub struct MoveNode; - -impl McpTool for MoveNode { - fn name(&self) -> &str { - "move_node" - } - fn call(&self, args: &BTreeMap) -> ToolOutcome { - let node_id = match parse_node_id(args, "node_id") { - Ok(v) => v, - Err(e) => return e, - }; - // `target_parent_id` is required; an empty string ("" or "0") - // means "the active page root" (the NONE sentinel). - let Some(raw_target) = args.get("target_parent_id") else { - return ToolOutcome::Err( - ToolErrorCode::MissingArgument, - "target_parent_id is required (\"\" or \"0\" = page root)".into(), - ); - }; - let target_parent = root_or_node_id(raw_target); - if target_parent.is_real() && target_parent == node_id { - return ToolOutcome::Err( - ToolErrorCode::InvalidArgument, - "node_id and target_parent_id must differ".into(), - ); - } - let mut out = BTreeMap::new(); - out.insert("wrote".into(), "true".into()); - ToolOutcome::OkWithCommand( - out, - EditorCommand::MoveNode { - node_id, - target_parent, - }, - ) - } -} - -pub fn move_node_snapshot() -> MoveNode { - MoveNode -} - /// Resolve a `target_parent_id`-style arg. The legacy wire used `"0"` /// for "page root"; the canonical model uses the empty `NodeId::NONE` /// sentinel. Both `""` and `"0"` map to `NONE` so older clients keep /// working. `"root"` is also accepted because the generated tool /// schema uses that wording for page-root inserts. -fn root_or_node_id(raw: &str) -> NodeId { +pub(super) fn root_or_node_id(raw: &str) -> NodeId { let trimmed = raw.trim(); if trimmed.is_empty() || trimmed == "0" @@ -521,42 +477,6 @@ fn root_or_node_id(raw: &str) -> NodeId { } } -/// First-party `copy_node` tool — deep-clone a node + subtree under a -/// new parent. -pub struct CopyNode; - -impl McpTool for CopyNode { - fn name(&self) -> &str { - "copy_node" - } - fn call(&self, args: &BTreeMap) -> ToolOutcome { - let node_id = match parse_node_id(args, "node_id") { - Ok(v) => v, - Err(e) => return e, - }; - let Some(raw_target) = args.get("target_parent_id") else { - return ToolOutcome::Err( - ToolErrorCode::MissingArgument, - "target_parent_id is required (\"\" or \"0\" = page root)".into(), - ); - }; - let target_parent = root_or_node_id(raw_target); - let mut out = BTreeMap::new(); - out.insert("wrote".into(), "true".into()); - ToolOutcome::OkWithCommand( - out, - EditorCommand::CopyNode { - node_id, - target_parent, - }, - ) - } -} - -pub fn copy_node_snapshot() -> CopyNode { - CopyNode -} - /// First-party `replace_node` tool — swap an existing node for a /// freshly-built one at the same parent slot. pub struct ReplaceNode; diff --git a/crates/op-mcp/src/write_tools_tests.rs b/crates/op-mcp/src/write_tools_tests.rs index a44fa5953..c8f1f7f9a 100644 --- a/crates/op-mcp/src/write_tools_tests.rs +++ b/crates/op-mcp/src/write_tools_tests.rs @@ -7,6 +7,7 @@ //! apply-path correctness itself is exhaustively covered by //! `op-editor-core`'s own `command_tests.rs`. +use super::reparent_tools::move_node_snapshot; use super::test_fixtures::{add_theme_axis, add_variable, sample, state_with}; use super::write_tools::*; use super::{EditorCommand, McpTool, ToolErrorCode, ToolOutcome}; @@ -615,15 +616,46 @@ fn move_node_returns_command_with_empty_target_for_page_root() { EditorCommand::MoveNode { node_id, target_parent, + page_id, + index, }, ) => { assert_eq!(node_id.as_str(), "n11"); assert!(!target_parent.is_real(), "0 maps to the page-root NONE"); + assert!(page_id.is_none()); + assert!(index.is_none()); } other => panic!("expected MoveNode, got {other:?}"), } } +#[test] +fn move_node_accepts_ts_node_parent_page_and_index_args() { + let tool = move_node_snapshot(); + let mut args = BTreeMap::new(); + args.insert("nodeId".into(), "n11".into()); + args.insert("parent".into(), "n10".into()); + args.insert("pageId".into(), "page-2".into()); + args.insert("index".into(), "2".into()); + match tool.call(&args) { + ToolOutcome::OkWithCommand( + _, + EditorCommand::MoveNode { + node_id, + target_parent, + page_id, + index, + }, + ) => { + assert_eq!(node_id.as_str(), "n11"); + assert_eq!(target_parent.as_str(), "n10"); + assert_eq!(page_id.as_deref(), Some("page-2")); + assert_eq!(index, Some(2)); + } + other => panic!("expected MoveNode command with TS args, got {other:?}"), + } +} + #[test] fn move_node_command_applies_through_editor_state() { let mut s = sample(); @@ -631,6 +663,8 @@ fn move_node_command_applies_through_editor_state() { assert!(s.apply(EditorCommand::MoveNode { node_id: op_editor_core::NodeId::new("n11"), target_parent: op_editor_core::NodeId::NONE, + page_id: None, + index: None, })); assert!(s .active_children() diff --git a/crates/op-orchestrator/src/dashboard_columns/height_reorder.rs b/crates/op-orchestrator/src/dashboard_columns/height_reorder.rs index 5cc974e6f..b2b70de62 100644 --- a/crates/op-orchestrator/src/dashboard_columns/height_reorder.rs +++ b/crates/op-orchestrator/src/dashboard_columns/height_reorder.rs @@ -167,6 +167,8 @@ pub(crate) fn reorder_dashboard_main_children( .map(|id| EditorCommand::MoveNode { node_id: NodeId::new(id), target_parent: main_node_id.clone(), + page_id: None, + index: None, }) .collect() }