From 993c660ad523139d2d29af0cccd4d65ea5ce9141 Mon Sep 17 00:00:00 2001 From: Fini Date: Mon, 20 Jul 2026 21:33:16 +0800 Subject: [PATCH] fix(mcp): preserve sizing keywords in update operations --- crates/op-mcp/src/batch_direct_ops.rs | 2 +- crates/op-mcp/src/batch_direct_ops_tests.rs | 55 ++++++++++++++ .../op-mcp/src/batch_program_update_tests.rs | 43 +++++++++++ crates/op-mcp/src/lib.rs | 4 ++ crates/op-mcp/src/update_node_data.rs | 71 +++++++++++++++++-- crates/op-mcp/src/update_node_data_tests.rs | 21 ++++++ 6 files changed, 188 insertions(+), 8 deletions(-) create mode 100644 crates/op-mcp/src/batch_direct_ops_tests.rs create mode 100644 crates/op-mcp/src/batch_program_update_tests.rs diff --git a/crates/op-mcp/src/batch_direct_ops.rs b/crates/op-mcp/src/batch_direct_ops.rs index 9dc0459a3..fcee88925 100644 --- a/crates/op-mcp/src/batch_direct_ops.rs +++ b/crates/op-mcp/src/batch_direct_ops.rs @@ -84,7 +84,7 @@ pub(crate) fn update_command_from_value( let Some(obj) = value.as_object() else { return Err("U() update JSON must be an object".into()); }; - if let Some(patch_json) = ts_update_patch_json_value(value) { + if let Some(patch_json) = ts_update_patch_json_value(value)? { return Ok(EditorCommand::PatchNodeData { node_id, patch_json, diff --git a/crates/op-mcp/src/batch_direct_ops_tests.rs b/crates/op-mcp/src/batch_direct_ops_tests.rs new file mode 100644 index 000000000..0a6f1f93a --- /dev/null +++ b/crates/op-mcp/src/batch_direct_ops_tests.rs @@ -0,0 +1,55 @@ +//! Regression tests for direct `U()` operation command selection. + +use op_editor_core::{EditorCommand, NodeId}; + +use super::batch_direct_ops::parse_single_direct_operation; +use super::test_fixtures::sample; + +#[test] +fn direct_update_routes_sizing_keywords_to_patch_and_applies() { + let mut state = sample(); + let command = parse_single_direct_operation( + r##"U("n10", {"width":"fill_container","height":"fit_content","fill_hex":"#112233"})"##, + ) + .expect("valid direct update") + .expect("update command"); + + let EditorCommand::PatchNodeData { + node_id, + patch_json, + page_id, + } = &command + else { + panic!("sizing keywords must use PatchNodeData, got {command:?}"); + }; + assert_eq!(node_id.as_str(), "n10"); + assert_eq!(page_id, &None); + let patch: serde_json::Value = serde_json::from_str(patch_json).expect("patch json"); + assert_eq!(patch["width"], "fill_container"); + assert_eq!(patch["height"], "fit_content"); + assert_eq!(patch["fill"][0]["color"], "#112233"); + assert!(patch.get("fill_hex").is_none()); + + assert!(state.apply(command), "keyword patch must apply"); + let node = op_editor_core::walkers::find_node(state.active_children(), &NodeId::new("n10")) + .expect("updated frame"); + let value = serde_json::to_value(node).expect("updated node json"); + assert_eq!(value["width"], "fill_container"); + assert_eq!(value["height"], "fit_content"); + assert_eq!(value["fill"][0]["color"], "#112233"); +} + +#[test] +fn direct_update_keeps_numeric_sizes_on_flat_command() { + let command = parse_single_direct_operation(r#"U("n10", {"width":320,"height":480})"#) + .expect("valid direct update") + .expect("update command"); + + match command { + EditorCommand::UpdateNode { width, height, .. } => { + assert_eq!(width, Some(320)); + assert_eq!(height, Some(480)); + } + other => panic!("numeric sizes must stay on UpdateNode, got {other:?}"), + } +} diff --git a/crates/op-mcp/src/batch_program_update_tests.rs b/crates/op-mcp/src/batch_program_update_tests.rs new file mode 100644 index 000000000..dba5e6a19 --- /dev/null +++ b/crates/op-mcp/src/batch_program_update_tests.rs @@ -0,0 +1,43 @@ +//! Transaction regression tests for `U()` sizing updates in mixed programs. + +use std::collections::BTreeMap; + +use op_editor_core::{EditorCommand, NodeId}; + +use super::batch_design_snapshot; +use super::test_fixtures::sample; +use super::{McpTool, ToolOutcome}; + +#[test] +fn batch_program_transaction_applies_sizing_keywords() { + let mut state = sample(); + let tool = batch_design_snapshot(&state); + let mut args = BTreeMap::new(); + args.insert( + "operations".into(), + concat!( + "U(\"n10\", {\"x\":48})\n", + "U(\"n10\", {\"width\":\"fill_container\",\"height\":\"fit_content\"})" + ) + .into(), + ); + + let ToolOutcome::OkJsonWithCommand(json, command) = tool.call(&args) else { + panic!("successful transaction must emit a command"); + }; + let envelope: serde_json::Value = serde_json::from_str(&json).expect("result envelope"); + assert!(envelope.get("errors").is_none(), "{envelope}"); + let EditorCommand::Batch { commands } = &command else { + panic!("two updates must remain one atomic batch, got {command:?}"); + }; + assert!(matches!(commands[0], EditorCommand::UpdateNode { .. })); + assert!(matches!(commands[1], EditorCommand::PatchNodeData { .. })); + + assert!(state.apply(command), "complete transaction must apply"); + let node = op_editor_core::walkers::find_node(state.active_children(), &NodeId::new("n10")) + .expect("updated frame"); + let value = serde_json::to_value(node).expect("updated node json"); + assert_eq!(value["x"].as_f64(), Some(48.0)); + assert_eq!(value["width"], "fill_container"); + assert_eq!(value["height"], "fit_content"); +} diff --git a/crates/op-mcp/src/lib.rs b/crates/op-mcp/src/lib.rs index 186cead6e..6645772b6 100644 --- a/crates/op-mcp/src/lib.rs +++ b/crates/op-mcp/src/lib.rs @@ -27,6 +27,8 @@ pub mod batch_design_result; #[cfg(test)] mod batch_design_tests; mod batch_direct_ops; +#[cfg(test)] +mod batch_direct_ops_tests; pub mod batch_get; #[cfg(test)] mod batch_get_tests; @@ -38,6 +40,8 @@ mod batch_program; pub mod batch_program_objects; #[cfg(test)] mod batch_program_tests; +#[cfg(test)] +mod batch_program_update_tests; pub mod bulk_vars; #[cfg(test)] mod bulk_vars_tests; diff --git a/crates/op-mcp/src/update_node_data.rs b/crates/op-mcp/src/update_node_data.rs index c56242d89..5d3c52528 100644 --- a/crates/op-mcp/src/update_node_data.rs +++ b/crates/op-mcp/src/update_node_data.rs @@ -25,16 +25,73 @@ pub(super) fn ts_update_patch_json( "data must be a JSON object".into(), )); }; - Ok(ts_update_patch_json_value(&value)) + ts_update_patch_json_value(&value) + .map_err(|message| ToolOutcome::Err(ToolErrorCode::InvalidArgument, message)) } -pub(super) fn ts_update_patch_json_value(value: &Value) -> Option { - let obj = value.as_object()?; - if obj.is_empty() || obj.keys().any(|key| !is_flat_update_data_key(key)) { - Some(value.to_string()) - } else { - None +pub(super) fn ts_update_patch_json_value(value: &Value) -> Result, String> { + let Some(obj) = value.as_object() else { + return Ok(None); + }; + let is_rich = obj.is_empty() + || obj.keys().any(|key| !is_flat_update_data_key(key)) + || uses_sizing_keyword(obj); + if !is_rich { + return Ok(None); } + + let mut patch = value.clone(); + canonicalize_fill_hex_shortcut(&mut patch)?; + Ok(Some(patch.to_string())) +} + +/// `UpdateNode` stores width and height as integers, while canonical node data +/// also accepts content/fill sizing keywords. Keep literal numeric geometry on +/// the lightweight command, but route keyword sizing through the rich shallow +/// patch so serde can preserve its `SizingBehavior` variant. +fn uses_sizing_keyword(obj: &serde_json::Map) -> bool { + ["width", "height"].into_iter().any(|key| { + matches!( + obj.get(key), + Some(Value::String(value)) + if matches!(value.as_str(), "fit_content" | "fill_container") + ) + }) +} + +/// `fill_hex` / `fillHex` belong to the lightweight update tool, not the +/// canonical PenNode schema. Once any rich field moves the update onto +/// `PatchNodeData`, rewrite the shortcut to a canonical solid fill so it does +/// not disappear during PenNode deserialization. +fn canonicalize_fill_hex_shortcut(value: &mut Value) -> Result<(), String> { + let Some(obj) = value.as_object_mut() else { + return Ok(()); + }; + let shortcut = obj.remove("fill_hex"); + let camel_shortcut = obj.remove("fillHex"); + let Some(shortcut) = shortcut.or(camel_shortcut) else { + return Ok(()); + }; + let Some(hex) = shortcut.as_str() else { + return Err("fill_hex must be a string".into()); + }; + if !validate_hex(hex) { + return Err(format!( + "fill_hex must be #rgb/#rrggbb/#rrggbbaa, got {hex:?}" + )); + } + obj.insert( + "fill".into(), + serde_json::json!([{ "type": "solid", "color": hex }]), + ); + Ok(()) +} + +fn validate_hex(value: &str) -> bool { + let Some(hex) = value.trim().strip_prefix('#') else { + return false; + }; + matches!(hex.len(), 3 | 6 | 8) && hex.chars().all(|ch| ch.is_ascii_hexdigit()) } fn is_flat_update_data_key(key: &str) -> bool { diff --git a/crates/op-mcp/src/update_node_data_tests.rs b/crates/op-mcp/src/update_node_data_tests.rs index e46e8d8d9..2a7dd6641 100644 --- a/crates/op-mcp/src/update_node_data_tests.rs +++ b/crates/op-mcp/src/update_node_data_tests.rs @@ -1,7 +1,9 @@ //! TS-style `update_node(data)` parity tests. +use super::test_fixtures::sample; use super::write_tools::update_node_snapshot; use super::{EditorCommand, McpTool, ToolOutcome}; +use op_editor_core::NodeId; use serde_json::Value; use std::collections::BTreeMap; @@ -32,3 +34,22 @@ fn update_node_data_with_ts_text_fields_emits_patch_command() { assert_eq!(patch["fontSize"], 24); assert_eq!(page_id.as_deref(), Some("page-2")); } + +#[test] +fn update_node_data_with_sizing_keyword_emits_patch_command_and_applies() { + let mut state = sample(); + let mut args = BTreeMap::new(); + args.insert("nodeId".into(), "n10".into()); + args.insert("data".into(), r##"{"height":"fit_content"}"##.into()); + + let ToolOutcome::OkWithCommand(_, command @ EditorCommand::PatchNodeData { .. }) = + update_node_snapshot().call(&args) + else { + panic!("expected PatchNodeData command for sizing keyword"); + }; + assert!(state.apply(command), "sizing keyword patch must apply"); + let node = op_editor_core::walkers::find_node(state.active_children(), &NodeId::new("n10")) + .expect("updated frame"); + let value = serde_json::to_value(node).expect("updated node json"); + assert_eq!(value["height"], "fit_content"); +}