From 6c05d0a7a86619b2cf6c7df7432a72e9e8ba5dd1 Mon Sep 17 00:00:00 2001 From: Fini Date: Sun, 9 Aug 2026 01:18:02 +0800 Subject: [PATCH] fix(mcp): stop later batches consuming named screen shells MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The replaceable-empty-root snapshot reset per program, so screens a previous batch inserted (still empty, but named) read as starters and were swallowed by the next insert. Only an unnamed frame — or the factory starter's literal name — is replaceable now. Claude-Session: https://claude.ai/code/session_01FqKQqNj8exYwopGDpYUU7x --- crates/op-mcp/src/batch_program.rs | 45 ++++++++++++++------ crates/op-mcp/src/batch_program_tests.rs | 53 +++++++++++++++++++++--- 2 files changed, 80 insertions(+), 18 deletions(-) diff --git a/crates/op-mcp/src/batch_program.rs b/crates/op-mcp/src/batch_program.rs index c3c59bcfd..c76904a78 100644 --- a/crates/op-mcp/src/batch_program.rs +++ b/crates/op-mcp/src/batch_program.rs @@ -132,13 +132,7 @@ pub(crate) fn run_batch_design_program( .sim .active_children() .iter() - .filter(|node| { - matches!(node, PenNode::Frame(_)) - && node - .children() - .map(|children| children.is_empty()) - .unwrap_or(true) - }) + .filter(|node| is_replaceable_starter_root(node)) .map(|node| node.id_str().to_string()) .collect(); // Live-doc node count BEFORE any line runs — the honest `nodeCount` @@ -198,6 +192,36 @@ pub(crate) fn run_batch_design_program( } } +/// The name the shipped blank starter frame carries +/// (`op_editor_core::blank_starter`). Treated as "unnamed" below: it is the +/// placeholder the auto-replace exists for, not an authored screen. +const STARTER_FRAME_NAME: &str = "Frame"; + +/// Whether an existing root may be consumed by a root-level `I(null, frame)`. +/// +/// Emptiness alone is not enough. The pre-program snapshot stops a batch from +/// eating its OWN earlier inserts, but the snapshot is retaken every program, +/// so a shell inserted by batch N is a "pre-existing empty root" to batch N+1 — +/// measured: batch 1 inserts A and B, batch 2 inserts C and A silently +/// disappears. An authored NAME is what separates the two: a screen shell the +/// model is about to fill is named, the placeholder it is replacing is not. +/// This also matches the policy `op_editor_core::blank_starter` states for the +/// document-level twin of this question — an empty frame the user drew and has +/// not filled in yet must not be treated as disposable. +fn is_replaceable_starter_root(node: &PenNode) -> bool { + matches!(node, PenNode::Frame(_)) + && node + .children() + .map(|children| children.is_empty()) + .unwrap_or(true) + && node + .base() + .name + .as_deref() + .map(str::trim) + .is_none_or(|name| name.is_empty() || name == STARTER_FRAME_NAME) +} + pub(crate) struct ProgramCtx { pub(crate) sim: EditorState, pub(crate) page_id: Option, @@ -462,12 +486,7 @@ fn execute_insert(binding: &str, args: &str, ctx: &mut ProgramCtx) -> Result<()> .active_children() .iter() .find(|candidate| { - candidate.id_str() == id - && matches!(candidate, PenNode::Frame(_)) - && candidate - .children() - .map(|children| children.is_empty()) - .unwrap_or(true) + candidate.id_str() == id && is_replaceable_starter_root(candidate) }) .map(|empty| (id.clone(), empty.base().x, empty.base().y)) }); diff --git a/crates/op-mcp/src/batch_program_tests.rs b/crates/op-mcp/src/batch_program_tests.rs index 6d39f100f..7fb201f81 100644 --- a/crates/op-mcp/src/batch_program_tests.rs +++ b/crates/op-mcp/src/batch_program_tests.rs @@ -5,7 +5,7 @@ use std::collections::BTreeMap; -use op_editor_core::{EditorCommand, NodeId, PenNodeExt}; +use op_editor_core::{EditorCommand, EditorState, NodeId, PenNodeExt}; use serde_json::Value; use super::batch_design_snapshot; @@ -13,7 +13,7 @@ use super::batch_program_test_support::{ binding_id, call_operations, call_operations_best_effort, contains_merge_app_state, subtree_contains_text, }; -use super::test_fixtures::{frame, sample, state_with}; +use super::test_fixtures::{frame, sample, state_with, text}; use super::{McpTool, ToolOutcome}; #[test] @@ -433,8 +433,9 @@ fn delete_of_unknown_id_is_a_silent_noop_like_ts_remove_node_from_tree() { fn root_frame_insert_replaces_the_first_empty_frame_and_inherits_position() { // TS auto-replace: `I(null, frame)` swaps in for an empty root // frame instead of creating a sibling. Lenient JSON keeps this - // program on the executor path. - let mut state = state_with(vec![frame("f1", "Blank", 30.0, 40.0, 100.0, 100.0, vec![])]); + // program on the executor path. The placeholder carries the shipped + // starter's name, which is what makes it replaceable. + let mut state = state_with(vec![frame("f1", "Frame", 30.0, 40.0, 100.0, 100.0, vec![])]); let program = "page=I(null, {type:'frame', name:'Page', width:320, height:240})"; let (envelope, cmd) = call_operations(&state, program); assert!(envelope.get("errors").is_none(), "{envelope}"); @@ -454,7 +455,7 @@ fn root_frame_insert_replaces_the_first_empty_frame_and_inherits_position() { #[test] fn root_frame_batch_replaces_only_preexisting_empty_starters() { - let mut state = state_with(vec![frame("f1", "Blank", 30.0, 40.0, 100.0, 100.0, vec![])]); + let mut state = state_with(vec![frame("f1", "Frame", 30.0, 40.0, 100.0, 100.0, vec![])]); let program = "a=I(null, {type:'frame', name:'A'})\nb=I(null, {type:'frame', name:'B'})\nc=I(null, {type:'frame', name:'C'})"; let (envelope, cmd) = call_operations(&state, program); assert!(envelope.get("errors").is_none(), "{envelope}"); @@ -475,6 +476,48 @@ fn root_frame_batch_replaces_only_preexisting_empty_starters() { ); } +/// The pre-program snapshot is retaken every `batch_design` call, so shells a +/// previous batch inserted looked like fresh starter placeholders to the next +/// one: batch 1 built A and B, batch 2 built C, and A was silently deleted. +/// An authored name is what tells the two apart. +#[test] +fn a_later_batch_does_not_consume_the_previous_batchs_empty_screen_shells() { + let mut state = state_with(vec![frame( + "home", + "Home", + 0.0, + 0.0, + 390.0, + 844.0, + vec![text("home-title", "Title", 0.0, 0.0, 100.0, 20.0, "Home")], + )]); + + let program = "a=I(null, {type:'frame', name:'A'})\nb=I(null, {type:'frame', name:'B'})"; + let (envelope, cmd) = call_operations(&state, program); + assert!(envelope.get("errors").is_none(), "{envelope}"); + assert!(state.apply(cmd.expect("first batch"))); + assert_eq!(root_names(&state), ["Home", "A", "B"]); + + // A and B are still empty — the model fills them in later batches. + let program = "c=I(null, {type:'frame', name:'C'})\nd=I(null, {type:'frame', name:'D'})"; + let (envelope, cmd) = call_operations(&state, program); + assert!(envelope.get("errors").is_none(), "{envelope}"); + assert!(state.apply(cmd.expect("second batch"))); + assert_eq!( + root_names(&state), + ["Home", "A", "B", "C", "D"], + "an earlier batch's empty shells must survive the next batch" + ); +} + +fn root_names(state: &EditorState) -> Vec<&str> { + state + .active_children() + .iter() + .filter_map(|root| root.base().name.as_deref()) + .collect() +} + #[test] fn bound_move_records_the_binding_and_honors_the_index() { let mut state = sample();