fix(mcp): preserve sizing keywords in update operations
This commit is contained in:
parent
dc5f946592
commit
993c660ad5
|
|
@ -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,
|
||||
|
|
|
|||
55
crates/op-mcp/src/batch_direct_ops_tests.rs
Normal file
55
crates/op-mcp/src/batch_direct_ops_tests.rs
Normal file
|
|
@ -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:?}"),
|
||||
}
|
||||
}
|
||||
43
crates/op-mcp/src/batch_program_update_tests.rs
Normal file
43
crates/op-mcp/src/batch_program_update_tests.rs
Normal file
|
|
@ -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");
|
||||
}
|
||||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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<String> {
|
||||
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<Option<String>, 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<String, Value>) -> 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 {
|
||||
|
|
|
|||
|
|
@ -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");
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue