From e29ea4229cc2b06ecd0ae96e204f71931ce80bb1 Mon Sep 17 00:00:00 2001 From: Fini Date: Thu, 30 Jul 2026 08:59:39 +0800 Subject: [PATCH] fix(agent): backfill generated screen interactions --- .../op-host-native/src/preview/auto_wire.rs | 6 +- crates/op-orchestrator/src/cleanup.rs | 20 +- .../src/cleanup_repair_summary_tests.rs | 86 +++ .../src/geometry_bottom_gap.rs | 6 +- crates/op-orchestrator/src/geometry_echo.rs | 5 +- .../src/geometry_interaction_backfill.rs | 480 +++++++++++++++ .../geometry_interaction_backfill_tests.rs | 567 ++++++++++++++++++ .../src/geometry_validation.rs | 7 + .../src/prompt_components_tests.rs | 1 + .../src/prompt_planning_tests.rs | 2 + crates/op-orchestrator/src/prompt_subagent.rs | 56 ++ .../src/prompt_subagent_content_tests.rs | 92 +++ .../op-orchestrator/src/run_screen_groups.rs | 6 + .../src/run_tests_screen_groups.rs | 35 ++ .../src/screen_route_inventory.rs | 182 ++++++ .../src/spawn_concurrent_tests.rs | 46 ++ crates/op-orchestrator/src/subagent.rs | 16 +- .../src/unify_shared_nav_active_tab_tests.rs | 2 +- .../src/wire_screen_navigation.rs | 158 ++--- .../src/wire_screen_navigation_tests.rs | 225 +++++-- crates/op-smoke/src/audit_rubric.rs | 24 +- 21 files changed, 1855 insertions(+), 167 deletions(-) create mode 100644 crates/op-orchestrator/src/geometry_interaction_backfill.rs create mode 100644 crates/op-orchestrator/src/geometry_interaction_backfill_tests.rs create mode 100644 crates/op-orchestrator/src/screen_route_inventory.rs diff --git a/crates/op-host-native/src/preview/auto_wire.rs b/crates/op-host-native/src/preview/auto_wire.rs index 0e7f13e4f..d32cddca2 100644 --- a/crates/op-host-native/src/preview/auto_wire.rs +++ b/crates/op-host-native/src/preview/auto_wire.rs @@ -31,12 +31,14 @@ //! accumulates screens incrementally and each turn should link the new //! ones in. At PREVIEW-ENTRY time it is deliberately stricter: an author //! who has started marking screens by hand is mid-way through an -//! intentional App Mode setup, and silently binding additional nav/back -//! taps behind their back on every preview open would be a confusing, +//! intentional App Mode setup, and silently binding additional nav taps +//! behind their back on every preview open would be a confusing, //! preview-only side effect they never asked for and can't see in the //! editor. So this module's gate is document-wide ("does ANY authored //! marker exist"), not per-node — user/model intent wins outright rather //! than being merged with. +//! Generation-only back/card completion is deliberately not part of this +//! cloned-state fallback; those interactions are persisted by cleanup. //! //! ## Why op-host-native can depend on op-orchestrator //! diff --git a/crates/op-orchestrator/src/cleanup.rs b/crates/op-orchestrator/src/cleanup.rs index 517d4c5ac..1bc04d742 100644 --- a/crates/op-orchestrator/src/cleanup.rs +++ b/crates/op-orchestrator/src/cleanup.rs @@ -610,14 +610,18 @@ pub fn run_cleanup_passes_with_summary( // point, so both the classic and loop-finalize paths pick it up. crate::unify_shared_status_bar::unify_shared_status_bar(sink); - // Track A of the interactive-preview plan: mark screen-shaped top-level - // frames + wire their nav tabs / back buttons, so a multi-screen document - // enters App Mode preview with zero model cooperation. Runs LAST — after - // bottom-nav anchoring/dedup/distribution above have settled the final - // nav shape, so tab-item discovery sees the real tree, not an - // in-progress one. Whole-doc (scans `sink.state()`, not `root_ids`) so - // it also links PRE-EXISTING screens from earlier turns, matching - // `avatar_repair` above. + // Establish final screen routes first. The cleanup-only semantic pass can + // then persist only fact-proven back/card interactions against those real + // routes, before the label-matching nav fallback. Keeping the semantic pass + // outside public `wire_screen_navigation` prevents Cmd+P's cloned-state + // fallback from creating preview-only interactions that never reach the + // saved document. + crate::wire_screen_navigation::ensure_screen_routes(sink); + crate::geometry_validation::wire_interaction_backfill(sink); + + // Track A fallback: wire bottom-nav/sidebar tabs after final chrome shape + // and semantic interactions are settled. Whole-doc (scans `sink.state()`, + // not `root_ids`) so it also links pre-existing screens from earlier turns. crate::wire_screen_navigation::wire_screen_navigation(sink); counter.checkpoint(summary, CheckCategory::Structure); } diff --git a/crates/op-orchestrator/src/cleanup_repair_summary_tests.rs b/crates/op-orchestrator/src/cleanup_repair_summary_tests.rs index 9fd5f305c..c87387d40 100644 --- a/crates/op-orchestrator/src/cleanup_repair_summary_tests.rs +++ b/crates/op-orchestrator/src/cleanup_repair_summary_tests.rs @@ -142,6 +142,92 @@ fn an_over_bold_screen_counts_its_repairs_under_hierarchy() { ); } +#[test] +fn interaction_backfill_edits_are_counted_as_structure_repairs() { + let nodes: Vec = serde_json::from_value(json!([ + { + "type": "frame", "id": "entry", "name": "Discover", "screen": "/", + "x": 0, "y": 0, "width": 390, "height": 844, "layout": "none", + "children": [{ + "type": "frame", "id": "row", "x": 20, "y": 180, + "width": 350, "height": 170, "layout": "horizontal", + "children": [ + { + "type": "frame", "id": "card-a", "width": 100, "height": 150, + "layout": "vertical", "children": [ + {"type":"image","id":"image-a","width":100,"height":90, + "src":"https://example.invalid/a.png"}, + {"type":"text","id":"title-a","content":"A","width":100,"height":20} + ] + }, + { + "type": "frame", "id": "card-b", "width": 100, "height": 150, + "layout": "vertical", "children": [ + {"type":"image","id":"image-b","width":100,"height":90, + "src":"https://example.invalid/b.png"}, + {"type":"text","id":"title-b","content":"B","width":100,"height":20} + ] + } + ] + }] + }, + { + "type": "frame", "id": "detail", "name": "Movie Detail", + "screen": "/detail", "x": 450, "y": 0, "width": 390, "height": 844, + "layout": "none", "children": [ + { + "type": "frame", "id": "back", "x": 24, "y": 80, + "width": 44, "height": 44, "layout": "none", "children": [{ + "type":"icon_font","id":"back-icon","x":12,"y":12, + "width":20,"height":20,"iconFontName":"arrow-left" + }] + }, + {"type":"frame","id":"detail-content","x":0,"y":160, + "width":390,"height":600,"children":[]} + ] + } + ])) + .expect("fixture nodes"); + let mut sink = VecDocSink::new(); + sink.state.apply(EditorCommand::InsertSubtree { + nodes, + parent_id: NodeId::NONE, + page_id: None, + }); + let root_ids = sink + .state + .active_children() + .iter() + .map(|node| node.id_str().to_string()) + .collect::>(); + let root_refs = root_ids.iter().map(String::as_str).collect::>(); + sink.applied.clear(); + let mut summary = RepairSummary::default(); + + run_cleanup_passes_with_summary(&mut sink, &plan(), &root_refs, &mut summary); + + let interaction_patches = sink + .applied + .iter() + .filter(|command| { + matches!( + command, + EditorCommand::PatchNodeData { patch_json, .. } + if patch_json.contains(r#""pop":null"#) + || patch_json.contains(r#""push":"\"/detail\"""#) + ) + }) + .count(); + assert_eq!( + interaction_patches, 3, + "one back frame and two cards must be persisted" + ); + assert!( + summary.repairs_for(CheckCategory::Structure) >= interaction_patches, + "the quality credential must count every interaction patch: {summary:?}" + ); +} + #[test] fn the_tally_never_exceeds_the_edits_the_sink_actually_took() { // The credential's number must be defensible against the document: it diff --git a/crates/op-orchestrator/src/geometry_bottom_gap.rs b/crates/op-orchestrator/src/geometry_bottom_gap.rs index 3573aef9c..1ad7d96e0 100644 --- a/crates/op-orchestrator/src/geometry_bottom_gap.rs +++ b/crates/op-orchestrator/src/geometry_bottom_gap.rs @@ -101,7 +101,11 @@ pub(super) fn push_mobile_bottom_gap_diagnostic( /// the authored `role` semantic the corpus mandates, or the resolved shape a /// tab bar always has — a full-width band in the nav height range laying out /// at least three evenly-sized tap targets in a row. -fn is_bottom_nav_shape(child: &Value, root_rect: &Rect, rects: &HashMap) -> bool { +pub(super) fn is_bottom_nav_shape( + child: &Value, + root_rect: &Rect, + rects: &HashMap, +) -> bool { if child.get("role").and_then(Value::as_str) == Some("bottom-tab-bar") { return true; } diff --git a/crates/op-orchestrator/src/geometry_echo.rs b/crates/op-orchestrator/src/geometry_echo.rs index 22230f58b..641c8da26 100644 --- a/crates/op-orchestrator/src/geometry_echo.rs +++ b/crates/op-orchestrator/src/geometry_echo.rs @@ -14,7 +14,9 @@ use op_editor_core::{EditorState, NodeId}; use super::text_collision::push_text_collision_diagnostics; -use super::{collect_diagnostics, resolved_rects, MAX_DIAGNOSTICS}; +use super::{ + collect_diagnostics, push_interaction_backfill_diagnostics, resolved_rects, MAX_DIAGNOSTICS, +}; /// Geometry diagnostics for exactly the subtrees rooted at `root_ids` — /// the `inserted_root_ids` one subtask's `InsertSubtree` produced. Runs the @@ -30,6 +32,7 @@ pub(crate) fn geometry_diagnostics_for_roots( ) -> Vec { let rects = resolved_rects(state); let mut out = Vec::new(); + push_interaction_backfill_diagnostics(state, Some(root_ids), &mut out); for root_id in root_ids { if out.len() >= MAX_DIAGNOSTICS { break; diff --git a/crates/op-orchestrator/src/geometry_interaction_backfill.rs b/crates/op-orchestrator/src/geometry_interaction_backfill.rs new file mode 100644 index 000000000..d348f7616 --- /dev/null +++ b/crates/op-orchestrator/src/geometry_interaction_backfill.rs @@ -0,0 +1,480 @@ +//! Shared facts for deterministic interaction backfill and its geometry echo. +//! +//! Back controls are recognized only from resolved geometry and dedicated icon +//! data (`iconFontName` / `iconId`). Card routing requires one unambiguous +//! detail-shaped screen and repeated image+text subtree shapes. Node names, +//! ids, and inferred intent never participate. + +use std::collections::{BTreeMap, BTreeSet, HashSet}; + +use jian_ops_schema::node::PenNode; + +use super::geometry_bottom_gap::is_bottom_nav_shape; +use super::*; + +const HEADER_STRIP_MAX_Y: f64 = 120.0; +const LEFT_STRIP_MAX_X: f64 = 120.0; +const MAX_BACK_CONTROL_SIZE: f64 = 56.0; +const SQUARE_EPS: f64 = 1.0; +const BOUNDS_EPS: f64 = 1.0; + +#[derive(Clone, Debug, PartialEq, Eq)] +struct InteractionTarget { + node_id: String, +} + +#[derive(Clone, Debug, Default, PartialEq, Eq)] +struct InteractionBackfillFacts { + back_targets: Vec, + card_targets: Vec, + detail_route: Option, +} + +struct ScreenFact { + id: String, + route: String, + value: Value, + rect: Rect, +} + +#[derive(Clone, Debug)] +struct BackShapeFact { + node_id: String, + can_wire: bool, + has_pop: bool, +} + +/// Persist every interaction that the shared fact scan proves unambiguous. +/// This is cleanup-only; preview's cloned-state fallback never calls it. +pub(crate) fn wire_interaction_backfill(sink: &mut dyn DocSink) { + let facts = interaction_backfill_facts(sink.state()); + for target in facts.back_targets { + sink.apply(EditorCommand::PatchNodeData { + node_id: NodeId::new(target.node_id), + patch_json: r#"{"events":{"onTap":[{"pop":null}]}}"#.to_string(), + page_id: None, + }); + } + if let Some(route) = facts.detail_route { + for target in facts.card_targets { + sink.apply(EditorCommand::PatchNodeData { + node_id: NodeId::new(target.node_id), + patch_json: crate::wire_screen_navigation::navigate_patch("push", &route), + page_id: None, + }); + } + } +} + +/// Shape-only strict back-control fact for compatibility consumers that must +/// recognize an already-wired detail screen without repeating M1's detector. +pub(crate) fn screen_has_back_control_shape(state: &EditorState, screen_id: &str) -> bool { + let rects = resolved_rects(state); + let Some(root) = op_editor_core::walkers::find_node( + state.active_children(), + &NodeId::new(screen_id.to_string()), + ) else { + return false; + }; + let Some(root_rect) = rects.get(screen_id).copied() else { + return false; + }; + let Ok(value) = serde_json::to_value(root) else { + return false; + }; + let mut shapes = Vec::new(); + collect_back_shapes(&value, &root_rect, &rects, false, &mut shapes); + !shapes.is_empty() +} + +/// Whole-document interaction-gap echo. The repair and echo consume the exact +/// same [`interaction_backfill_facts`] result, so their eligibility cannot drift. +pub(super) fn push_interaction_backfill_diagnostics( + state: &EditorState, + root_ids: Option<&[String]>, + out: &mut Vec, +) { + if out.len() >= MAX_DIAGNOSTICS { + return; + } + let facts = interaction_backfill_facts(state); + let allowed = root_ids.map(|ids| descendant_ids_for_roots(state, ids)); + let included = |target: &InteractionTarget| { + allowed + .as_ref() + .is_none_or(|ids| ids.contains(&target.node_id)) + }; + let back_count = facts + .back_targets + .iter() + .filter(|target| included(target)) + .count(); + if back_count > 0 { + out.push(format!( + "interaction-unwired-back: {back_count} non-entry top-left back control(s) have no \ + onTap; bind events.onTap to pop." + )); + } + if out.len() >= MAX_DIAGNOSTICS { + return; + } + let card_count = facts + .card_targets + .iter() + .filter(|target| included(target)) + .count(); + if card_count > 0 { + if let Some(route) = facts.detail_route { + out.push(format!( + "interaction-unwired-cards: {card_count} repeated image+text card(s) have no \ + onTap, and the document has one detail route {route:?}; bind their onTap to \ + push that exact route." + )); + } + } +} + +fn interaction_backfill_facts(state: &EditorState) -> InteractionBackfillFacts { + let rects = resolved_rects(state); + let screens = collect_screen_facts(state, &rects); + if screens.is_empty() { + return InteractionBackfillFacts::default(); + } + + let mut existing_push_routes = BTreeSet::new(); + for root in state.active_children() { + if let Ok(value) = serde_json::to_value(root) { + collect_push_routes(&value, &mut existing_push_routes); + } + } + + let mut back_targets = Vec::new(); + let mut detail_indices = Vec::new(); + for (index, screen) in screens.iter().enumerate() { + let mut shapes = Vec::new(); + collect_back_shapes(&screen.value, &screen.rect, &rects, false, &mut shapes); + if screen.route != "/" { + let unreachable_single = + screens.len() == 1 && !existing_push_routes.contains(screen.route.as_str()); + if !unreachable_single { + back_targets.extend(shapes.iter().filter(|fact| fact.can_wire).map(|fact| { + InteractionTarget { + node_id: fact.node_id.clone(), + } + })); + } + let has_trailing_bottom_nav = node_children(&screen.value) + .last() + .is_some_and(|last| is_bottom_nav_shape(last, &screen.rect, &rects)); + let has_return_control = shapes.iter().any(|shape| shape.can_wire || shape.has_pop); + if !has_trailing_bottom_nav && has_return_control { + detail_indices.push(index); + } + } + } + + if detail_indices.len() != 1 { + return InteractionBackfillFacts { + back_targets, + card_targets: Vec::new(), + detail_route: None, + }; + } + + let detail = &screens[detail_indices[0]]; + let mut card_targets = Vec::new(); + for screen in &screens { + if screen.id == detail.id { + continue; + } + collect_card_targets(&screen.value, false, &mut card_targets); + } + InteractionBackfillFacts { + back_targets, + card_targets, + detail_route: Some(detail.route.clone()), + } +} + +fn collect_screen_facts(state: &EditorState, rects: &HashMap) -> Vec { + state + .active_children() + .iter() + .filter_map(|node| { + let PenNode::Frame(frame) = node else { + return None; + }; + let route = frame + .screen + .clone() + .filter(|route| route.starts_with('/'))?; + let rect = rects.get(&frame.base.id).copied()?; + let value = serde_json::to_value(node).ok()?; + Some(ScreenFact { + id: frame.base.id.clone(), + route, + value, + rect, + }) + }) + .collect() +} + +fn collect_back_shapes( + node: &Value, + root_rect: &Rect, + rects: &HashMap, + ancestor_has_on_tap: bool, + out: &mut Vec, +) { + let node_has_on_tap = has_on_tap(node); + if is_back_control_shape(node, root_rect, rects) { + if let Some(node_id) = node.get("id").and_then(Value::as_str) { + out.push(BackShapeFact { + node_id: node_id.to_string(), + can_wire: !has_events(node) && !ancestor_has_on_tap && !subtree_has_on_tap(node), + has_pop: !ancestor_has_on_tap + && !descendants_have_on_tap(node) + && has_exact_pop_action(node), + }); + } + return; + } + let descendant_ancestor_has_on_tap = ancestor_has_on_tap || node_has_on_tap; + for child in node_children(node) { + collect_back_shapes(child, root_rect, rects, descendant_ancestor_has_on_tap, out); + } +} + +fn is_back_control_shape(node: &Value, root_rect: &Rect, rects: &HashMap) -> bool { + if node.get("type").and_then(Value::as_str) != Some("frame") { + return false; + } + let children = node_children(node); + let [icon] = children.as_slice() else { + return false; + }; + if !is_back_icon_data(icon) { + return false; + } + let Some(rect) = node + .get("id") + .and_then(Value::as_str) + .and_then(|id| rects.get(id)) + .copied() + else { + return false; + }; + if rect.w <= 0.0 + || rect.h <= 0.0 + || rect.w > MAX_BACK_CONTROL_SIZE + || rect.h > MAX_BACK_CONTROL_SIZE + || (rect.w - rect.h).abs() > SQUARE_EPS + { + return false; + } + let local_x = rect.x - root_rect.x; + let local_y = rect.y - root_rect.y; + local_x >= -BOUNDS_EPS + && local_x + rect.w <= LEFT_STRIP_MAX_X + BOUNDS_EPS + && (-BOUNDS_EPS..HEADER_STRIP_MAX_Y).contains(&local_y) +} + +fn is_back_icon_data(icon: &Value) -> bool { + let raw = match icon.get("type").and_then(Value::as_str) { + Some("icon_font") => icon.get("iconFontName").and_then(Value::as_str), + Some("path") => icon.get("iconId").and_then(Value::as_str), + _ => None, + }; + raw.and_then(|name| name.trim().rsplit(':').next()) + .map(str::to_ascii_lowercase) + .is_some_and(|name| matches!(name.as_str(), "chevron-left" | "arrow-left")) +} + +fn collect_card_targets( + parent: &Value, + ancestor_has_on_tap: bool, + out: &mut Vec, +) { + let parent_chain_has_on_tap = ancestor_has_on_tap || has_on_tap(parent); + let mut groups: BTreeMap> = BTreeMap::new(); + if !parent_chain_has_on_tap { + for child in node_children(parent) { + if is_card_container(child) + && !has_events(child) + && !subtree_has_on_tap(child) + && subtree_has_text(child) + && subtree_has_image(child) + { + groups.entry(subtree_shape(child)).or_default().push(child); + } + } + } + + let mut selected = HashSet::new(); + for group in groups.values().filter(|group| group.len() >= 2) { + for card in group { + if let Some(id) = card.get("id").and_then(Value::as_str) { + selected.insert(id.to_string()); + out.push(InteractionTarget { + node_id: id.to_string(), + }); + } + } + } + + for child in node_children(parent) { + let selected_here = child + .get("id") + .and_then(Value::as_str) + .is_some_and(|id| selected.contains(id)); + if !selected_here { + collect_card_targets(child, parent_chain_has_on_tap, out); + } + } +} + +fn is_card_container(value: &Value) -> bool { + matches!( + value.get("type").and_then(Value::as_str), + Some("frame" | "group" | "rectangle") + ) && !node_children(value).is_empty() +} + +fn subtree_shape(value: &Value) -> String { + let kind = value + .get("type") + .and_then(Value::as_str) + .unwrap_or("unknown"); + let child_shapes = node_children(value) + .iter() + .map(|child| subtree_shape(child)) + .collect::>() + .join(","); + format!("{kind}[{child_shapes}]") +} + +fn subtree_has_text(value: &Value) -> bool { + value.get("type").and_then(Value::as_str) == Some("text") + || node_children(value) + .iter() + .any(|child| subtree_has_text(child)) +} + +fn subtree_has_image(value: &Value) -> bool { + let has_image_fill = value + .get("fill") + .and_then(Value::as_array) + .is_some_and(|fills| { + fills + .iter() + .any(|fill| fill.get("type").and_then(Value::as_str) == Some("image")) + }); + value.get("type").and_then(Value::as_str) == Some("image") + || value + .get("imagePrompt") + .and_then(Value::as_str) + .is_some_and(|prompt| !prompt.trim().is_empty()) + || has_image_fill + || node_children(value) + .iter() + .any(|child| subtree_has_image(child)) +} + +fn has_events(value: &Value) -> bool { + value.get("events").is_some() +} + +fn has_on_tap(value: &Value) -> bool { + value + .get("events") + .and_then(|events| events.get("onTap")) + .and_then(Value::as_array) + .is_some_and(|actions| !actions.is_empty()) +} + +fn has_exact_pop_action(value: &Value) -> bool { + let Some(actions) = value + .get("events") + .and_then(|events| events.get("onTap")) + .and_then(Value::as_array) + else { + return false; + }; + let [action] = actions.as_slice() else { + return false; + }; + action + .as_object() + .is_some_and(|action| action.len() == 1 && action.get("pop").is_some_and(Value::is_null)) +} + +fn descendants_have_on_tap(value: &Value) -> bool { + node_children(value) + .iter() + .any(|child| subtree_has_on_tap(child)) +} + +fn subtree_has_on_tap(value: &Value) -> bool { + has_on_tap(value) + || node_children(value) + .iter() + .any(|child| subtree_has_on_tap(child)) +} + +fn collect_push_routes(value: &Value, out: &mut BTreeSet) { + if let Some(actions) = value + .get("events") + .and_then(|events| events.get("onTap")) + .and_then(Value::as_array) + { + for raw in actions + .iter() + .filter_map(|action| action.get("push")) + .filter_map(Value::as_str) + { + let decoded = serde_json::from_str::(raw).unwrap_or_else(|_| raw.to_string()); + out.insert(decoded); + } + } + for child in node_children(value) { + collect_push_routes(child, out); + } +} + +fn descendant_ids_for_roots(state: &EditorState, root_ids: &[String]) -> HashSet { + let mut ids = HashSet::new(); + for root_id in root_ids { + let Some(root) = op_editor_core::walkers::find_node( + state.active_children(), + &NodeId::new(root_id.clone()), + ) else { + continue; + }; + if let Ok(value) = serde_json::to_value(root) { + collect_ids(&value, &mut ids); + } + } + ids +} + +fn collect_ids(value: &Value, out: &mut HashSet) { + if let Some(id) = value.get("id").and_then(Value::as_str) { + out.insert(id.to_string()); + } + for child in node_children(value) { + collect_ids(child, out); + } +} + +fn node_children(value: &Value) -> Vec<&Value> { + value + .get("children") + .and_then(Value::as_array) + .map(|children| children.iter().collect()) + .unwrap_or_default() +} + +#[cfg(test)] +#[path = "geometry_interaction_backfill_tests.rs"] +mod tests; diff --git a/crates/op-orchestrator/src/geometry_interaction_backfill_tests.rs b/crates/op-orchestrator/src/geometry_interaction_backfill_tests.rs new file mode 100644 index 000000000..55ae07cea --- /dev/null +++ b/crates/op-orchestrator/src/geometry_interaction_backfill_tests.rs @@ -0,0 +1,567 @@ +use super::*; + +use crate::test_support::VecDocSink; +use jian_ops_schema::PenDocument; +use op_editor_core::EditorState; +use serde_json::{json, Value}; + +fn sink_from_value(value: Value) -> VecDocSink { + let document: PenDocument = serde_json::from_value(value).expect("valid document"); + VecDocSink { + state: EditorState::from_document(document), + applied: Vec::new(), + batch_depth: 0, + } +} + +fn icon_back(id: &str, x: f64, y: f64) -> Value { + json!({ + "type": "frame", + "id": id, + "x": x, + "y": y, + "width": 44, + "height": 44, + "layout": "none", + "children": [{ + "type": "icon_font", + "id": format!("{id}-icon"), + "x": 12, + "y": 12, + "width": 20, + "height": 20, + "iconFontName": "chevron-left" + }] + }) +} + +fn path_back(id: &str, icon_id: Option<&str>, name: Option<&str>) -> Value { + let mut path = json!({ + "type": "path", + "id": format!("{id}-path"), + "x": 12, + "y": 12, + "width": 20, + "height": 20, + "d": "M15 18l-6-6 6-6" + }); + if let Some(icon_id) = icon_id { + path["iconId"] = json!(icon_id); + } + if let Some(name) = name { + path["name"] = json!(name); + } + json!({ + "type": "frame", + "id": id, + "x": 24, + "y": 80, + "width": 44, + "height": 44, + "layout": "none", + "children": [path] + }) +} + +fn card(id: &str) -> Value { + json!({ + "type": "frame", + "id": id, + "width": 100, + "height": 150, + "layout": "vertical", + "children": [ + { + "type": "image", + "id": format!("{id}-image"), + "width": 100, + "height": 90, + "src": "https://example.invalid/poster.png" + }, + { + "type": "text", + "id": format!("{id}-title"), + "width": 100, + "height": 20, + "content": "Movie" + } + ] + }) +} + +fn entry_screen(cards: Vec) -> Value { + json!({ + "type": "frame", + "id": "entry", + "name": "Discover", + "screen": "/", + "x": 0, + "y": 0, + "width": 390, + "height": 844, + "layout": "none", + "children": [{ + "type": "frame", + "id": "card-row", + "x": 20, + "y": 180, + "width": 350, + "height": 170, + "layout": "horizontal", + "gap": 12, + "children": cards + }] + }) +} + +fn detail_screen(id: &str, route: &str, back: Value) -> Value { + json!({ + "type": "frame", + "id": id, + "name": "Movie Detail", + "screen": route, + "x": 450, + "y": 0, + "width": 390, + "height": 844, + "layout": "none", + "children": [ + back, + { + "type": "frame", + "id": format!("{id}-content"), + "x": 0, + "y": 160, + "width": 390, + "height": 600, + "children": [] + } + ] + }) +} + +fn document(children: Vec) -> Value { + json!({ "version": "1.0", "children": children }) +} + +fn node_value(state: &EditorState, id: &str) -> Value { + let node = + op_editor_core::walkers::find_node(state.active_children(), &NodeId::new(id.to_string())) + .unwrap_or_else(|| panic!("missing node {id}")); + serde_json::to_value(node).expect("serialize node") +} + +fn on_tap(state: &EditorState, id: &str) -> Option { + node_value(state, id) + .get("events") + .and_then(|events| events.get("onTap")) + .cloned() +} + +#[test] +fn strict_back_and_isomorphic_cards_are_wired_and_echo_clears() { + let mut sink = sink_from_value(document(vec![ + entry_screen(vec![card("movie-a"), card("movie-b")]), + detail_screen("detail", "/detail", icon_back("back", 24.0, 80.0)), + ])); + + let before = crate::geometry_validation::geometry_diagnostics(&sink.state); + assert!(before + .iter() + .any(|line| line.starts_with("interaction-unwired-back: 1"))); + assert!(before + .iter() + .any(|line| line.starts_with("interaction-unwired-cards: 2"))); + + wire_interaction_backfill(&mut sink); + + assert_eq!(on_tap(&sink.state, "back"), Some(json!([{"pop": null}]))); + assert!( + on_tap(&sink.state, "back-icon").is_none(), + "the square frame, not its icon child, owns the tap target" + ); + for card_id in ["movie-a", "movie-b"] { + assert_eq!( + on_tap(&sink.state, card_id), + Some(json!([{"push": "\"/detail\""}])), + "push must carry a Tier-1 quoted string literal" + ); + } + let after = crate::geometry_validation::geometry_diagnostics(&sink.state); + assert!( + after + .iter() + .all(|line| !line.starts_with("interaction-unwired-")), + "{after:?}" + ); +} + +#[test] +fn root_scoped_echo_reports_only_targets_inside_the_inserted_subtree() { + let sink = sink_from_value(document(vec![ + entry_screen(vec![card("movie-a"), card("movie-b")]), + detail_screen("detail", "/detail", icon_back("back", 24.0, 80.0)), + ])); + + let mut cards_only = Vec::new(); + push_interaction_backfill_diagnostics( + &sink.state, + Some(&["card-row".to_string()]), + &mut cards_only, + ); + assert_eq!(cards_only.len(), 1); + assert!(cards_only[0].starts_with("interaction-unwired-cards: 2")); + + let mut detail_only = Vec::new(); + push_interaction_backfill_diagnostics( + &sink.state, + Some(&["detail".to_string()]), + &mut detail_only, + ); + assert_eq!(detail_only.len(), 1); + assert!(detail_only[0].starts_with("interaction-unwired-back: 1")); +} + +#[test] +fn entry_screen_back_shape_is_never_wired() { + let mut entry = entry_screen(vec![card("movie-a"), card("movie-b")]); + entry["children"] + .as_array_mut() + .unwrap() + .insert(0, icon_back("entry-menu", 24.0, 80.0)); + let mut sink = sink_from_value(document(vec![ + entry, + detail_screen("detail", "/detail", icon_back("detail-back", 24.0, 80.0)), + ])); + + wire_interaction_backfill(&mut sink); + + assert!(on_tap(&sink.state, "entry-menu").is_none()); + assert_eq!( + on_tap(&sink.state, "detail-back"), + Some(json!([{"pop": null}])) + ); +} + +#[test] +fn two_detail_candidates_abandon_all_card_pushes_without_guessing() { + let mut second = detail_screen( + "detail-b", + "/detail-b", + icon_back("detail-b-back", 24.0, 80.0), + ); + second["x"] = json!(900); + let mut sink = sink_from_value(document(vec![ + entry_screen(vec![card("movie-a"), card("movie-b")]), + detail_screen( + "detail-a", + "/detail-a", + icon_back("detail-a-back", 24.0, 80.0), + ), + second, + ])); + + wire_interaction_backfill(&mut sink); + + assert!(on_tap(&sink.state, "movie-a").is_none()); + assert!(on_tap(&sink.state, "movie-b").is_none()); + assert_eq!( + on_tap(&sink.state, "detail-a-back"), + Some(json!([{"pop": null}])) + ); + assert_eq!( + on_tap(&sink.state, "detail-b-back"), + Some(json!([{"pop": null}])) + ); +} + +#[test] +fn existing_events_are_byte_stable_and_clean_siblings_still_qualify() { + let mut authored_card = card("movie-authored"); + authored_card["events"] = json!({ + "onTap": [{"custom_action": null}], + "onHover": [{"set": {"selected": true}}] + }); + let mut authored_back = icon_back("back", 24.0, 80.0); + authored_back["events"] = json!({"onTap": [{"pop": null}]}); + let mut sink = sink_from_value(document(vec![ + entry_screen(vec![authored_card, card("movie-a"), card("movie-b")]), + detail_screen("detail", "/detail", authored_back), + ])); + let card_events_before = node_value(&sink.state, "movie-authored")["events"].clone(); + let back_events_before = node_value(&sink.state, "back")["events"].clone(); + + wire_interaction_backfill(&mut sink); + + assert_eq!( + node_value(&sink.state, "movie-authored")["events"], + card_events_before + ); + assert_eq!( + node_value(&sink.state, "back")["events"], + back_events_before + ); + assert_eq!( + on_tap(&sink.state, "movie-a"), + Some(json!([{"push": "\"/detail\""}])) + ); + assert_eq!( + on_tap(&sink.state, "movie-b"), + Some(json!([{"push": "\"/detail\""}])) + ); +} + +#[test] +fn conflicting_back_action_is_not_a_detail_fact_and_never_routes_cards() { + let mut conflicting_back = icon_back("back", 24.0, 80.0); + conflicting_back["events"] = json!({ + "onTap": [{"pop": null}, {"custom_action": null}] + }); + let mut sink = sink_from_value(document(vec![ + entry_screen(vec![card("movie-a"), card("movie-b")]), + detail_screen("detail", "/detail", conflicting_back), + ])); + let before = node_value(&sink.state, "back")["events"].clone(); + + wire_interaction_backfill(&mut sink); + + assert_eq!(node_value(&sink.state, "back")["events"], before); + assert!(on_tap(&sink.state, "movie-a").is_none()); + assert!(on_tap(&sink.state, "movie-b").is_none()); +} + +#[test] +fn exact_pop_detail_stays_navless_before_backfill_and_routes_cards() { + let mut entry = entry_screen(vec![card("movie-a"), card("movie-b")]); + let entry_tabs = ["Home", "Search", "Library"] + .into_iter() + .enumerate() + .map(|(index, label)| { + json!({ + "type": "frame", + "id": format!("tab-{index}"), + "width": 130, + "height": 64, + "children": [{ + "type": "text", + "id": format!("tab-{index}-label"), + "content": label, + "fontSize": 12 + }] + }) + }) + .collect::>(); + entry["children"].as_array_mut().unwrap().push(json!({ + "type": "frame", + "id": "entry-nav", + "role": "bottom-tab-bar", + "x": 0, + "y": 780, + "width": 390, + "height": 64, + "layout": "horizontal", + "children": entry_tabs + })); + let mut back = icon_back("back", 24.0, 80.0); + back["events"] = json!({"onTap": [{"pop": null}]}); + let mut detail = detail_screen("detail", "/detail", back); + detail["name"] = json!("Library"); + let mut sink = sink_from_value(document(vec![entry, detail])); + + crate::unify_shared_nav::unify_shared_nav(&mut sink); + wire_interaction_backfill(&mut sink); + + assert_eq!( + node_value(&sink.state, "detail")["children"] + .as_array() + .unwrap() + .len(), + 2 + ); + assert_eq!( + on_tap(&sink.state, "movie-a"), + Some(json!([{"push": "\"/detail\""}])) + ); + assert_eq!( + on_tap(&sink.state, "movie-b"), + Some(json!([{"push": "\"/detail\""}])) + ); +} + +#[test] +fn invalid_detail_route_is_not_a_screen_fact() { + let mut detail = detail_screen("detail", "movie-detail", icon_back("back", 24.0, 80.0)); + detail["x"] = json!(450); + let mut sink = sink_from_value(document(vec![ + entry_screen(vec![card("movie-a"), card("movie-b")]), + detail, + ])); + + wire_interaction_backfill(&mut sink); + + assert!(on_tap(&sink.state, "back").is_none()); + assert!(on_tap(&sink.state, "movie-a").is_none()); + assert!(on_tap(&sink.state, "movie-b").is_none()); +} + +#[test] +fn ancestor_on_tap_blocks_card_backfill_for_the_whole_chain() { + let mut entry = entry_screen(vec![card("movie-a"), card("movie-b")]); + entry["children"][0]["events"] = json!({"onTap": [{"custom_action": null}]}); + let mut sink = sink_from_value(document(vec![ + entry, + detail_screen("detail", "/detail", icon_back("back", 24.0, 80.0)), + ])); + + wire_interaction_backfill(&mut sink); + + assert!(on_tap(&sink.state, "movie-a").is_none()); + assert!(on_tap(&sink.state, "movie-b").is_none()); +} + +#[test] +fn path_uses_icon_id_only_and_never_falls_back_to_node_name() { + let mut detail = detail_screen( + "detail", + "/detail", + path_back("real-back", Some("lucide:arrow-left"), Some("Anything")), + ); + detail["children"] + .as_array_mut() + .unwrap() + .insert(1, path_back("name-only", None, Some("arrow-left"))); + let mut sink = sink_from_value(document(vec![ + entry_screen(vec![card("movie-a"), card("movie-b")]), + detail, + ])); + + wire_interaction_backfill(&mut sink); + + assert_eq!( + on_tap(&sink.state, "real-back"), + Some(json!([{"pop": null}])) + ); + assert!(on_tap(&sink.state, "name-only").is_none()); +} + +#[test] +fn geometry_rejects_late_large_non_square_and_multi_child_controls() { + let mut detail = detail_screen("detail", "/detail", icon_back("valid-back", 24.0, 80.0)); + let late = icon_back("late", 24.0, 140.0); + let mut large = icon_back("large", 24.0, 80.0); + large["width"] = json!(64); + large["height"] = json!(64); + let mut non_square = icon_back("non-square", 24.0, 80.0); + non_square["width"] = json!(48); + non_square["height"] = json!(36); + let mut multi = icon_back("multi", 24.0, 80.0); + multi["children"] + .as_array_mut() + .unwrap() + .push(json!({"type":"text","id":"extra","content":"x","width":10,"height":10})); + detail["children"] + .as_array_mut() + .unwrap() + .splice(1..1, [late, large, non_square, multi]); + let mut sink = sink_from_value(document(vec![ + entry_screen(vec![card("movie-a"), card("movie-b")]), + detail, + ])); + + wire_interaction_backfill(&mut sink); + + assert_eq!( + on_tap(&sink.state, "valid-back"), + Some(json!([{"pop": null}])) + ); + for id in ["late", "large", "non-square", "multi"] { + assert!( + on_tap(&sink.state, id).is_none(), + "{id} must stay untouched" + ); + } +} + +#[test] +fn trailing_bottom_nav_uses_geometry_bottom_gap_predicate_for_detail_gate() { + let nav_tabs = [0, 1, 2] + .into_iter() + .map(|index| { + json!({ + "type": "frame", + "id": format!("unnamed-tab-{index}"), + "width": 130, + "height": 64, + "children": [] + }) + }) + .collect::>(); + let mut tab_screen = detail_screen( + "tab-screen", + "/tab-screen", + icon_back("tab-back", 24.0, 80.0), + ); + tab_screen["children"].as_array_mut().unwrap().push(json!({ + "type": "frame", + "id": "unnamed-nav", + "x": 0, + "y": 780, + "width": 390, + "height": 64, + "layout": "horizontal", + "children": nav_tabs + })); + let mut actual_detail = detail_screen( + "actual-detail", + "/actual-detail", + icon_back("actual-back", 24.0, 80.0), + ); + actual_detail["x"] = json!(900); + let mut sink = sink_from_value(document(vec![ + entry_screen(vec![card("movie-a"), card("movie-b")]), + tab_screen, + actual_detail, + ])); + + wire_interaction_backfill(&mut sink); + + assert_eq!( + on_tap(&sink.state, "movie-a"), + Some(json!([{"push": "\"/actual-detail\""}])) + ); + assert_eq!( + on_tap(&sink.state, "movie-b"), + Some(json!([{"push": "\"/actual-detail\""}])) + ); +} + +#[test] +fn single_page_without_screen_is_a_byte_exact_noop_and_pass_is_idempotent() { + let mut single = sink_from_value(document(vec![json!({ + "type": "frame", + "id": "poster", + "name": "Single-page poster", + "width": 390, + "height": 844, + "children": [icon_back("decorative-arrow", 24.0, 80.0)] + })])); + let before = serde_json::to_value(single.state.active_children()).unwrap(); + wire_interaction_backfill(&mut single); + assert_eq!( + serde_json::to_value(single.state.active_children()).unwrap(), + before + ); + assert!(single.applied.is_empty()); + + let mut multi = sink_from_value(document(vec![ + entry_screen(vec![card("movie-a"), card("movie-b")]), + detail_screen("detail", "/detail", icon_back("back", 24.0, 80.0)), + ])); + wire_interaction_backfill(&mut multi); + let once = serde_json::to_value(multi.state.active_children()).unwrap(); + wire_interaction_backfill(&mut multi); + assert_eq!( + serde_json::to_value(multi.state.active_children()).unwrap(), + once + ); +} diff --git a/crates/op-orchestrator/src/geometry_validation.rs b/crates/op-orchestrator/src/geometry_validation.rs index 98c562c56..ece89fc25 100644 --- a/crates/op-orchestrator/src/geometry_validation.rs +++ b/crates/op-orchestrator/src/geometry_validation.rs @@ -46,6 +46,12 @@ use text_collision::push_text_collision_diagnostics; #[path = "geometry_bottom_gap.rs"] mod geometry_bottom_gap; use geometry_bottom_gap::push_mobile_bottom_gap_diagnostic; +#[path = "geometry_interaction_backfill.rs"] +mod geometry_interaction_backfill; +use geometry_interaction_backfill::push_interaction_backfill_diagnostics; +pub(crate) use geometry_interaction_backfill::{ + screen_has_back_control_shape, wire_interaction_backfill, +}; #[path = "geometry_card_rail_fixes.rs"] mod geometry_card_rail_fixes; #[path = "geometry_diagnostics_collect.rs"] @@ -222,6 +228,7 @@ const MAX_DIAGNOSTICS: usize = 8; pub fn geometry_diagnostics(state: &EditorState) -> Vec { let rects = resolved_rects(state); let mut out = Vec::new(); + push_interaction_backfill_diagnostics(state, None, &mut out); for root in state.active_children() { if out.len() >= MAX_DIAGNOSTICS { break; diff --git a/crates/op-orchestrator/src/prompt_components_tests.rs b/crates/op-orchestrator/src/prompt_components_tests.rs index 733070360..c4c53b203 100644 --- a/crates/op-orchestrator/src/prompt_components_tests.rs +++ b/crates/op-orchestrator/src/prompt_components_tests.rs @@ -399,6 +399,7 @@ fn tight_budget_dashboard_keeps_component_composition() { false, false, &lib, + &[], ); let sys = &cr.system_prompt; diff --git a/crates/op-orchestrator/src/prompt_planning_tests.rs b/crates/op-orchestrator/src/prompt_planning_tests.rs index d58be2c95..dc9e9ee45 100644 --- a/crates/op-orchestrator/src/prompt_planning_tests.rs +++ b/crates/op-orchestrator/src/prompt_planning_tests.rs @@ -316,6 +316,7 @@ fn subagent_prompt_reduced_complexity_full_tier_skill_filtering_is_noop() { false, true, &ComponentLibrary::default(), + &[], ); let (reduced_cr, _) = build_subagent_prompt_core( &st, @@ -326,6 +327,7 @@ fn subagent_prompt_reduced_complexity_full_tier_skill_filtering_is_noop() { false, true, &ComponentLibrary::default(), + &[], ); assert_eq!( full_cr.system_prompt, reduced_cr.system_prompt, diff --git a/crates/op-orchestrator/src/prompt_subagent.rs b/crates/op-orchestrator/src/prompt_subagent.rs index f487ee4d4..e8f83d48c 100644 --- a/crates/op-orchestrator/src/prompt_subagent.rs +++ b/crates/op-orchestrator/src/prompt_subagent.rs @@ -24,6 +24,36 @@ pub fn build_subagent_prompt( reduced_complexity: bool, minimal_skills: bool, components: &ComponentLibrary, +) -> (CallRequest, SkillLoadReport) { + build_subagent_prompt_with_screen_routes( + subtask, + plan, + req, + abort, + reduced_complexity, + minimal_skills, + components, + &[], + ) +} + +/// Production prompt builder with the document-wide screen route inventory. +/// +/// The public compatibility wrapper above passes an empty inventory so direct +/// callers that do not own a document snapshot keep byte-identical prompts. +/// Generation paths call this variant after resolving normalized plan groups +/// (or loop continuation's live screens) through navigation's shared route +/// allocator. +#[allow(clippy::too_many_arguments)] +pub(crate) fn build_subagent_prompt_with_screen_routes( + subtask: &Subtask, + plan: &OrchestratorPlan, + req: &DesignRequest, + abort: AbortFlag, + reduced_complexity: bool, + minimal_skills: bool, + components: &ComponentLibrary, + screen_routes: &[(String, String)], ) -> (CallRequest, SkillLoadReport) { // Script-gen is THE subagent protocol on every rung. Retry flags narrow // the skill set only; they never switch the output protocol. @@ -37,6 +67,7 @@ pub fn build_subagent_prompt( minimal_skills, script_on, components, + screen_routes, ) } @@ -53,6 +84,7 @@ pub(super) fn build_subagent_prompt_core( minimal_skills: bool, script_on: bool, components: &ComponentLibrary, + screen_routes: &[(String, String)], ) -> (CallRequest, SkillLoadReport) { // Resolve the full generation skill set, then apply tier-gated filtering. let model_id = req.model.as_deref().unwrap_or(""); @@ -293,6 +325,7 @@ pub(super) fn build_subagent_prompt_core( .as_ref() .map(|instruction| format!("{instruction}\n\n")) .unwrap_or_default(); + let screen_route_block = screen_route_prompt_block(screen_routes); let spacing_rule = if is_mobile_layout { "SPACING CONSISTENCY — MOBILE CONTENT RAIL: The root page may keep 0 horizontal padding for full-width status/navigation/full-bleed media. This ordinary transparent root-direct section owns padding:[0,24] exactly once; do not duplicate it on an inner wrapper. If this section is a clipped horizontal scroller, keep its section full width, inset its header 24px on both sides, and give the clipped viewport a 24px leading inset with a flush 0px trailing edge." } else { @@ -320,6 +353,7 @@ pub(super) fn build_subagent_prompt_core( Generate ONLY \"{}\" (~{:.0}px of content).{}\n\ Overall design: {}\n\n\ {}\ +{}\ CRITICAL LAYOUT CONSTRAINTS:\n\ - {}\n\ - Target content amount: ~{:.0}px tall. Generate enough elements to fill this area.\n\ @@ -347,6 +381,7 @@ CRITICAL LAYOUT CONSTRAINTS:\n\ subtask.region.height, my_elements, req.prompt, + screen_route_block, explicit_user_token_block, root_rule, subtask.region.height, @@ -469,3 +504,24 @@ CRITICAL LAYOUT CONSTRAINTS:\n\ report, ) } + +fn screen_route_prompt_block(screen_routes: &[(String, String)]) -> String { + if screen_routes.is_empty() { + return String::new(); + } + + let rows = screen_routes + .iter() + .map(|(name, route)| { + let name = serde_json::to_string(name).expect("serializing a string cannot fail"); + let route = serde_json::to_string(route).expect("serializing a string cannot fail"); + format!("- {name} -> {route}") + }) + .collect::>() + .join("\n"); + + format!( + "DOCUMENT SCREEN ROUTES (use these exact route values in schema-encoded navigation \ + actions; never invent another route):\n{rows}\n\n" + ) +} diff --git a/crates/op-orchestrator/src/prompt_subagent_content_tests.rs b/crates/op-orchestrator/src/prompt_subagent_content_tests.rs index 12739ddc2..efacabb39 100644 --- a/crates/op-orchestrator/src/prompt_subagent_content_tests.rs +++ b/crates/op-orchestrator/src/prompt_subagent_content_tests.rs @@ -3,6 +3,98 @@ use super::*; +#[test] +fn subagent_prompt_injects_exact_json_quoted_screen_route_inventory() { + let subtask = subtask(); + let plan = plan(); + let req = req(); + let routes = vec![ + ("Home".to_string(), "/".to_string()), + ( + "Movie \"Night\"\nDetail".to_string(), + "/movie-detail".to_string(), + ), + ]; + + let (call, with_routes_report) = build_subagent_prompt_with_screen_routes( + &subtask, + &plan, + &req, + AbortFlag::new(), + false, + false, + &ComponentLibrary::default(), + &routes, + ); + let expected = r#"DOCUMENT SCREEN ROUTES (use these exact route values in schema-encoded navigation actions; never invent another route): +- "Home" -> "/" +- "Movie \"Night\"\nDetail" -> "/movie-detail" + +CRITICAL LAYOUT CONSTRAINTS:"#; + assert!( + call.user_prompt.contains(expected), + "route inventory must be exact and JSON-quoted:\n{}", + call.user_prompt + ); + + let (_, empty_report) = build_subagent_prompt_with_screen_routes( + &subtask, + &plan, + &req, + AbortFlag::new(), + false, + false, + &ComponentLibrary::default(), + &[], + ); + assert_eq!( + with_routes_report, empty_report, + "user-prompt route context must not perturb the skill budget" + ); +} + +#[test] +fn empty_screen_route_inventory_matches_public_builder_byte_for_byte() { + let subtask = subtask(); + let plan = plan(); + let req = req(); + let components = ComponentLibrary::default(); + + let (compat_call, compat_report) = build_subagent_prompt( + &subtask, + &plan, + &req, + AbortFlag::new(), + false, + false, + &components, + ); + let (empty_call, empty_report) = build_subagent_prompt_with_screen_routes( + &subtask, + &plan, + &req, + AbortFlag::new(), + false, + false, + &components, + &[], + ); + + assert_eq!(compat_call.system_prompt, empty_call.system_prompt); + assert_eq!(compat_call.user_prompt, empty_call.user_prompt); + assert_eq!(compat_call.timeout, empty_call.timeout); + assert_eq!(compat_call.no_text_timeout, empty_call.no_text_timeout); + assert_eq!( + compat_call.first_text_timeout, + empty_call.first_text_timeout + ); + assert_eq!(compat_report, empty_report); + assert!( + !empty_call.user_prompt.contains("DOCUMENT SCREEN ROUTES"), + "empty inventory must not grow a route header" + ); +} + #[test] fn subagent_prompt_honors_explicit_radius_and_spacing_numbers() { let mobile_req = DesignRequest { diff --git a/crates/op-orchestrator/src/run_screen_groups.rs b/crates/op-orchestrator/src/run_screen_groups.rs index 583bd69d4..be7350978 100644 --- a/crates/op-orchestrator/src/run_screen_groups.rs +++ b/crates/op-orchestrator/src/run_screen_groups.rs @@ -121,6 +121,12 @@ pub(crate) fn insert_screen_group_roots( } } + // Freeze the normalized planning screen list into real route markers + // before any fan-out prompt is built. The prompt inventory reads the same + // merged candidates and allocator, so generated cross-screen actions can + // never reference a virtual route that cleanup later assigns differently. + crate::wire_screen_navigation::ensure_planned_screen_routes(sink, plan); + // Only the genuinely-concurrent shape (`identities.len() == // new_root_ids.len()`) tags anything — an empty slice (the sequential // path) deliberately leaves every root untagged; see this function's doc. diff --git a/crates/op-orchestrator/src/run_tests_screen_groups.rs b/crates/op-orchestrator/src/run_tests_screen_groups.rs index 799bf44e8..003a3817d 100644 --- a/crates/op-orchestrator/src/run_tests_screen_groups.rs +++ b/crates/op-orchestrator/src/run_tests_screen_groups.rs @@ -159,6 +159,41 @@ fn multi_screen_plan_gets_one_root_per_screen_group() { assert_eq!(summary.subtasks.len(), 2); } +#[test] +fn every_fanout_subtask_prompt_receives_the_full_screen_route_inventory() { + let llm = ScriptedLlm::new(vec![ + ScriptResponse::Text(MULTI_SCREEN_PLAN_JSON.into()), + ScriptResponse::Text(node_json("home")), + ScriptResponse::Text(node_json("profile")), + ]); + let mut sink = VecDocSink::new(); + + futures::executor::block_on(Orchestrator::new().run( + req(), + &mut sink, + &llm, + &mut |_| {}, + &AbortFlag::new(), + &stub_providers(), + )) + .expect("multi-screen run ok"); + + let route_prompts = llm + .user_prompts() + .into_iter() + .filter(|prompt| prompt.contains("DOCUMENT SCREEN ROUTES")) + .collect::>(); + assert_eq!( + route_prompts.len(), + 2, + "both Home and Profile workers need the same document-wide inventory" + ); + for prompt in route_prompts { + assert!(prompt.contains(r#"- "Home" -> "/""#), "{prompt}"); + assert!(prompt.contains(r#"- "Profile" -> "/profile""#), "{prompt}"); + } +} + /// Co-op point with Track A (`wire_screen_navigation`, run as part of /// `finalize_design`'s cleanup tail): once N screen-shaped roots exist, /// EVERY one of them — not just the first — gets a `screen` route marker, diff --git a/crates/op-orchestrator/src/screen_route_inventory.rs b/crates/op-orchestrator/src/screen_route_inventory.rs new file mode 100644 index 000000000..0cfd85148 --- /dev/null +++ b/crates/op-orchestrator/src/screen_route_inventory.rs @@ -0,0 +1,182 @@ +//! Planning/live screen-route context shared by fan-out prompts and wiring. + +use super::*; + +/// Name/route context for loop continuation prompts. Authored top-level +/// screens are runtime truth regardless of width; unmarked roots still use +/// navigation's conservative screen-shape bands. +pub(crate) fn screen_route_inventory(state: &EditorState) -> Vec<(String, String)> { + let screens = collect_prompt_live_candidates(state); + if screens.len() < 2 { + return Vec::new(); + } + route_inventory_for_candidates(&screens) +} + +/// Classic fan-out uses normalized planning groups, merged with existing +/// screens. Synthetic loop plans have no distinct screen roots and fall back +/// to the live inventory above. +pub(crate) fn prompt_screen_route_inventory( + plan: &crate::plan::OrchestratorPlan, + state: &EditorState, +) -> Vec<(String, String)> { + plan_route_candidates(plan, state) + .map(|candidates| route_inventory_for_candidates(&candidates)) + .unwrap_or_else(|| screen_route_inventory(state)) +} + +/// Persist the same merged assignments that fan-out prompts see. Called once +/// after per-screen scaffold roots exist and before their workers run. +pub(crate) fn ensure_planned_screen_routes( + sink: &mut dyn DocSink, + plan: &crate::plan::OrchestratorPlan, +) { + let Some(candidates) = plan_route_candidates(plan, sink.state()) else { + return; + }; + for (node_id, path) in assign_screen_paths(&candidates) { + sink.apply(EditorCommand::PatchNodeData { + node_id: NodeId::new(node_id), + patch_json: format!(r#"{{"screen":"{path}"}}"#), + page_id: None, + }); + } +} + +fn route_inventory_for_candidates(screens: &[ScreenCandidate]) -> Vec<(String, String)> { + let assignments = assign_screen_paths(screens); + let assigned: HashMap<&str, &str> = assignments + .iter() + .map(|(id, path)| (id.as_str(), path.as_str())) + .collect(); + screens + .iter() + .filter_map(|screen| { + screen + .existing_path + .clone() + .or_else(|| { + assigned + .get(screen.id.as_str()) + .map(|path| path.to_string()) + }) + .map(|path| (screen.name.clone(), path)) + }) + .collect() +} + +fn plan_route_candidates( + plan: &crate::plan::OrchestratorPlan, + state: &EditorState, +) -> Option> { + let groups = crate::screen_groups::group_subtasks_by_screen(&plan.subtasks); + if groups.len() < 2 { + return None; + } + let planned = groups + .iter() + .enumerate() + .map(|(index, group)| { + let root_id = group + .indices + .iter() + .filter_map(|subtask_index| plan.subtasks.get(*subtask_index)) + .find_map(|subtask| subtask.parent_frame_id.clone()) + .unwrap_or_else(|| format!("planned-screen-{index}")); + (root_id, group.screen.clone()) + }) + .collect::>(); + let distinct_ids = planned + .iter() + .map(|(id, _)| id.as_str()) + .collect::>(); + if distinct_ids.len() < 2 || distinct_ids.len() != planned.len() { + return None; + } + + let planned_names = planned.iter().cloned().collect::>(); + let shaped = collect_screen_candidates(state) + .into_iter() + .map(|candidate| (candidate.id.clone(), candidate)) + .collect::>(); + let mut seen = BTreeSet::new(); + let mut candidates = Vec::new(); + for node in state.active_children() { + let PenNode::Frame(frame) = node else { + continue; + }; + let id = frame.base.id.clone(); + if let Some(name) = planned_names.get(&id) { + seen.insert(id.clone()); + candidates.push(ScreenCandidate { + id, + name: name.clone(), + existing_path: frame.screen.clone(), + }); + } else if let Some(candidate) = shaped.get(&id) { + seen.insert(id); + candidates.push(ScreenCandidate { + id: candidate.id.clone(), + name: candidate.name.clone(), + existing_path: candidate.existing_path.clone(), + }); + } else if frame.screen.is_some() { + seen.insert(id.clone()); + candidates.push(ScreenCandidate { + id, + name: frame + .base + .name + .clone() + .unwrap_or_else(|| frame.base.id.clone()), + existing_path: frame.screen.clone(), + }); + } + } + for (id, name) in planned { + if seen.insert(id.clone()) { + candidates.push(ScreenCandidate { + id, + name, + existing_path: None, + }); + } + } + Some(candidates) +} + +pub(super) fn collect_prompt_live_candidates(state: &EditorState) -> Vec { + let shaped = collect_screen_candidates(state) + .into_iter() + .map(|candidate| (candidate.id.clone(), candidate)) + .collect::>(); + state + .active_children() + .iter() + .filter_map(|node| { + let PenNode::Frame(frame) = node else { + return None; + }; + shaped.get(&frame.base.id).map_or_else( + || { + frame.screen.as_ref().map(|path| ScreenCandidate { + id: frame.base.id.clone(), + name: frame + .base + .name + .clone() + .unwrap_or_else(|| frame.base.id.clone()), + existing_path: Some(path.clone()), + }) + }, + |candidate| { + Some(ScreenCandidate { + id: candidate.id.clone(), + name: candidate.name.clone(), + existing_path: candidate.existing_path.clone(), + }) + }, + ) + }) + .collect() +} diff --git a/crates/op-orchestrator/src/spawn_concurrent_tests.rs b/crates/op-orchestrator/src/spawn_concurrent_tests.rs index fd8c543a4..69981bb6e 100644 --- a/crates/op-orchestrator/src/spawn_concurrent_tests.rs +++ b/crates/op-orchestrator/src/spawn_concurrent_tests.rs @@ -96,6 +96,52 @@ fn run_spawned_agents_invokes_real_runner_and_inserts_n_subtrees() { ); } +#[test] +fn loop_spawned_agent_prompts_receive_live_document_screen_routes() { + let mut sink = VecDocSink::new(); + let screens: Vec = serde_json::from_str( + r#"[ + {"type":"frame","id":"home","name":"Home","screen":"/", + "x":0,"y":0,"width":390,"height":844,"children":[]}, + {"type":"frame","id":"detail","name":"Movie Detail","screen":"/movie-detail", + "x":470,"y":0,"width":390,"height":844,"children":[]} + ]"#, + ) + .unwrap(); + sink.apply(EditorCommand::InsertSubtree { + nodes: screens, + parent_id: op_editor_core::NodeId::NONE, + page_id: None, + }); + sink.applied.clear(); + + let llm = ScriptedLlm::new(vec![ + ScriptResponse::Text(node_json("a")), + ScriptResponse::Text(node_json("b")), + ]); + let specs = vec![spec("a"), spec("b")]; + let results = block_on(run_spawned_agents_concurrent( + &specs, + &make_req(), + &llm, + &mut sink, + &AbortFlag::new(), + 2, + None, + )); + + assert!(results.iter().all(|result| result.error.is_none())); + let prompts = llm.user_prompts(); + assert_eq!(prompts.len(), 2); + for prompt in prompts { + assert!(prompt.contains(r#"- "Home" -> "/""#), "{prompt}"); + assert!( + prompt.contains(r#"- "Movie Detail" -> "/movie-detail""#), + "{prompt}" + ); + } +} + /// Concurrency is GENUINE: with a cap ≥ N, `CountingLlm` observes more than /// one in-flight call — the workers' LLM calls overlap (vs. strictly /// sequential, which would cap at 1). diff --git a/crates/op-orchestrator/src/subagent.rs b/crates/op-orchestrator/src/subagent.rs index e4165b68e..fd0d94ed9 100644 --- a/crates/op-orchestrator/src/subagent.rs +++ b/crates/op-orchestrator/src/subagent.rs @@ -8,7 +8,7 @@ //! - `node_count > 0`(`error` 可带软错误)—— 部分产出,继续后续。 use crate::plan::{OrchestratorPlan, Subtask}; -use crate::prompt::build_subagent_prompt; +use crate::prompt::build_subagent_prompt_with_screen_routes; use crate::types::{AbortFlag, DesignRequest, DocSink, LlmChunk, LlmClient, SubtaskOutcome}; use futures::StreamExt; use jian_ops_schema::node::PenNode; @@ -108,15 +108,16 @@ pub(crate) async fn run_subtask_with_reveal_at( subtask: Some(subtask.clone()), }; - // Snapshot the document's reusable-component registry before the prompt - // build so the AVAILABLE COMPONENTS manifest reflects whatever masters were - // merged into the doc (e.g. a loaded `.lib.op`). Cloned to release the - // shared `sink` borrow before the later mutable inserts. Empty registry ⇒ - // `build_subagent_prompt` leaves the prompt unchanged. + // Snapshot document-wide prompt context before building the prompt. + // Classic fan-out derives routes from normalized planning groups; loop + // continuation (whose synthetic plan has no screen labels) falls back to + // live screen markers. Both paths share navigation's route allocator. + let screen_routes = + crate::wire_screen_navigation::prompt_screen_route_inventory(plan, sink.state()); let components = sink.state().components.clone(); // 收集 LLM 文本输出。 - let (call_req, skill_report) = build_subagent_prompt( + let (call_req, skill_report) = build_subagent_prompt_with_screen_routes( subtask, plan, req, @@ -124,6 +125,7 @@ pub(crate) async fn run_subtask_with_reveal_at( reduced_complexity, minimal_skills, &components, + &screen_routes, ); // Surface the per-subtask skill-load report to the chat UI immediately // after the prompt is built (spec Component 4). diff --git a/crates/op-orchestrator/src/unify_shared_nav_active_tab_tests.rs b/crates/op-orchestrator/src/unify_shared_nav_active_tab_tests.rs index ff25c16cd..ad9a71d8e 100644 --- a/crates/op-orchestrator/src/unify_shared_nav_active_tab_tests.rs +++ b/crates/op-orchestrator/src/unify_shared_nav_active_tab_tests.rs @@ -554,7 +554,7 @@ fn active_retarget_preserves_descendant_routes_by_tab_position() { /// A nav-less screen with a back-shaped header control (chevron-left / /// arrow-left icon as the first child — resolves within the header region -/// exactly like `wire_screen_navigation_tests.rs::header_back_icon_gets_pop`). +/// exactly like the legacy shared-nav detail exemption). fn screen_json_no_nav_with_back_header(id: &str, name: &str) -> serde_json::Value { serde_json::json!({ "type": "frame", "id": id, "name": name, "width": 390, "height": 844, diff --git a/crates/op-orchestrator/src/wire_screen_navigation.rs b/crates/op-orchestrator/src/wire_screen_navigation.rs index 3eb4c0e6d..8fb229e4d 100644 --- a/crates/op-orchestrator/src/wire_screen_navigation.rs +++ b/crates/op-orchestrator/src/wire_screen_navigation.rs @@ -3,10 +3,10 @@ //! The preview engine (PreviewSession + jian-core `ScreenRouter` App Mode) //! already understands multi-screen documents: it looks for top-level //! `FrameNode.screen` markers and `events.onTap` navigation actions. What is -//! missing is generation-side wiring — AI-produced documents never carry -//! these fields, so `project_screens` finds nothing and preview degrades to -//! a single scrolling page. This pass fills that gap deterministically, with -//! zero model cooperation required (see +//! missing is generation-side wiring — AI-produced documents may omit these +//! fields, so `project_screens` finds nothing and preview degrades to a single +//! scrolling page. This module fills the screen/nav gap deterministically, +//! while cleanup-only interaction backfill owns strict back/card actions (see //! `openpencil-docs/openpencil/generation/preview-interactive-app-mode-0712.md`, //! "Track A contract v2"). //! @@ -24,11 +24,9 @@ //! 3. `screen` only ever marks a top-level (page-root-level) frame — the //! projection pass (`jian_ops_schema::screen_projection`) only scans //! top-level children per page (or per-document when pageless). -//! 4. Idempotent + additive-only: a node that already carries `screen` or -//! `events` is never touched (an authored marker may be a future -//! breakpoint-variant of the same path — see the jian -//! `feat/responsive-m1a` compatibility notes in the plan doc). Running -//! the pass twice must be a no-op the second time. +//! 4. Idempotent + additive-only: an authored `screen` marker or `events` +//! collection is never overwritten. Running the pass twice must be a no-op +//! the second time. //! 5. Zero new schema fields — only the existing `screen` / `events` fields //! are ever written. //! @@ -36,8 +34,7 @@ //! //! 1. `crate::cleanup::run_cleanup_passes` — the in-crate generation-pipeline //! caller (orchestrator per-subtask cleanup + the agentic loop's whole-doc -//! finalize), which is why the pass writes through the crate's own -//! `DocSink` rather than a concrete document type. +//! finalize), which invokes this after cleanup-only interaction backfill. //! 2. `op_host_native::preview::auto_wire` (Track C-1) — enter-preview //! auto-wiring. When a document carries no authored `screen` marker at //! all, the preview host runs this SAME pass over a JSON-cloned @@ -60,11 +57,9 @@ use crate::types::DocSink; const MOBILE_SCREEN_WIDTH: std::ops::RangeInclusive = 320.0..=480.0; const DESKTOP_SCREEN_MIN_WIDTH: f64 = 1024.0; -/// A back control must resolve within this many px of its screen's top edge -/// to count as "header region" — generous enough to cover a padded header -/// band (56-96px measured) plus a nested icon's own inset, tight enough that -/// a mid-page control is never mistaken for a nav-bar back arrow. -const HEADER_REGION_MAX_Y: f64 = 140.0; +/// Compatibility band for shared-nav's shipped detail-page exemption. +/// Interaction backfill uses its own stricter geometry fact. +const LEGACY_HEADER_REGION_MAX_Y: f64 = 140.0; const ENTRY_NAME_HINTS: [&str; 4] = ["home", "main", "dashboard", "index"]; @@ -98,47 +93,53 @@ pub(crate) struct NavParts<'a> { } /// Entry point: mark screen-shaped top-level frames with a `screen` route -/// path and wire each screen's bottom-nav / sidebar-nav tabs + header back -/// buttons to `events.onTap` navigation actions. No-ops when the document +/// path and wire each screen's bottom-nav / sidebar-nav tabs to `events.onTap` +/// navigation actions. No-ops when the document /// has fewer than two screen-shaped top-level frames (single-screen docs /// keep today's scrolling-page preview — zero regression surface). pub fn wire_screen_navigation(sink: &mut dyn DocSink) { - let screens = collect_screen_candidates(sink.state()); + let screens = ensure_screen_routes(sink); if screens.len() < 2 { return; } - let assignments = assign_screen_paths(&screens); - for (node_id, path) in &assignments { - sink.apply(EditorCommand::PatchNodeData { - node_id: NodeId::new(node_id.clone()), - patch_json: format!(r#"{{"screen":"{path}"}}"#), - page_id: None, - }); - } - - // Full id -> (path, name) table: authored markers keep their path, fresh - // assignments add theirs. Every candidate has exactly one entry. - let assigned: HashMap<&str, &str> = assignments - .iter() - .map(|(id, path)| (id.as_str(), path.as_str())) - .collect(); let screen_paths: Vec<(String, String)> = screens .iter() - .map(|s| { - let path = s + .filter_map(|screen| { + screen .existing_path - .clone() - .or_else(|| assigned.get(s.id.as_str()).map(|p| p.to_string())) - .unwrap_or_default(); - (s.name.clone(), path) + .as_ref() + .map(|path| (screen.name.clone(), path.clone())) }) .collect(); wire_nav_tabs(sink, &screens, &screen_paths); - wire_back_buttons(sink, &screens); } +/// Persist routes without wiring interactions. Generation cleanup uses this +/// before its document-writing interaction backfill; preview fallback keeps +/// calling [`wire_screen_navigation`] on a clone and only gets route/nav wiring. +pub(crate) fn ensure_screen_routes(sink: &mut dyn DocSink) -> Vec { + let screens = screen_route_inventory::collect_prompt_live_candidates(sink.state()); + if screens.len() < 2 { + return screens; + } + for (node_id, path) in assign_screen_paths(&screens) { + sink.apply(EditorCommand::PatchNodeData { + node_id: NodeId::new(node_id), + patch_json: format!(r#"{{"screen":"{path}"}}"#), + page_id: None, + }); + } + screen_route_inventory::collect_prompt_live_candidates(sink.state()) +} + +#[path = "screen_route_inventory.rs"] +mod screen_route_inventory; +pub(crate) use screen_route_inventory::{ + ensure_planned_screen_routes, prompt_screen_route_inventory, +}; + /// Scan the active page's top-level `Frame` children for screen-shaped /// candidates (numeric width AND height, width in a mobile or desktop band). /// `pub(crate)` for `unify_shared_nav`'s reuse — see [`ScreenCandidate`]. @@ -562,44 +563,20 @@ pub(crate) fn labels_match(a: &str, b: &str) -> bool { ta.iter().any(|t| tb.contains(t)) } -// ── Back-button wiring ────────────────────────────────────────────────── +// ── Shared-nav legacy detail signal ────────────────────────────────────── -/// Bind a header-region back control (name/icon containing back / -/// arrow-left / chevron-left) to `pop`. There is no system-level back -/// affordance in v1 (design §7) — only an authored UI node can carry it. -fn wire_back_buttons(sink: &mut dyn DocSink, screens: &[ScreenCandidate]) { - let y_offsets = resolved_y_offsets(sink.state()); - let mut patches: Vec = Vec::new(); - for screen in screens { - let Some(root) = op_editor_core::walkers::find_node( - sink.state().active_children(), - &NodeId::new(screen.id.clone()), - ) else { - continue; - }; - let screen_top = y_offsets.get(&screen.id).copied().unwrap_or(0.0); - collect_back_controls(root, screen_top, &y_offsets, &mut patches); - } - for node_id in patches { - sink.apply(EditorCommand::PatchNodeData { - node_id: NodeId::new(node_id), - patch_json: r#"{"events":{"onTap":[{"pop":null}]}}"#.to_string(), - page_id: None, - }); - } -} - -/// Whether `screen` has a back-shaped control (role `back`/`back-button`/ -/// `nav-back`, or a name/icon containing "back" / "arrow-left" / -/// "chevron-left") within its header region — reuses -/// [`collect_back_controls`]'s exact detection (same `HEADER_REGION_MAX_Y` -/// band `wire_back_buttons` binds `pop` against). Shared detail-page signal -/// for `unify_shared_nav`'s Inject-exemption gate: a screen with a header -/// back control reads as a push-in detail screen, not a tab destination. +/// Preserve shared-nav's shipped detail-page exemption. This intentionally +/// remains broader than M1 interaction backfill: it may recognize legacy +/// direct icons, roles, and authored names, but it never writes an event. +/// The strict cleanup repair and diagnostic predicate lives in +/// `geometry_interaction_backfill`. pub(crate) fn screen_has_back_control_in_header( sink: &dyn DocSink, screen: &ScreenCandidate, ) -> bool { + if crate::geometry_validation::screen_has_back_control_shape(sink.state(), &screen.id) { + return true; + } let Some(root) = op_editor_core::walkers::find_node( sink.state().active_children(), &NodeId::new(screen.id.clone()), @@ -609,37 +586,34 @@ pub(crate) fn screen_has_back_control_in_header( let y_offsets = resolved_y_offsets(sink.state()); let screen_top = y_offsets.get(&screen.id).copied().unwrap_or(0.0); let mut hits = Vec::new(); - collect_back_controls(root, screen_top, &y_offsets, &mut hits); + collect_legacy_back_controls(root, screen_top, &y_offsets, &mut hits); !hits.is_empty() } -fn collect_back_controls( +fn collect_legacy_back_controls( node: &PenNode, screen_top: f64, y_offsets: &HashMap, out: &mut Vec, ) { if node_has_events(node) { - // An authored interactive control owns its whole subtree — wiring a - // descendant underneath (e.g. the arrow icon inside an already-bound - // back button) would double-dispatch the same tap. return; } - if is_back_control(node) { + if is_legacy_back_control(node) { let within_header = y_offsets .get(node.id_str()) - .is_some_and(|y| (y - screen_top) <= HEADER_REGION_MAX_Y); + .is_some_and(|y| (y - screen_top) <= LEGACY_HEADER_REGION_MAX_Y); if within_header { out.push(node.id_str().to_string()); - return; // don't also bind a nested icon inside the matched control + return; } } for child in node.children().into_iter().flatten() { - collect_back_controls(child, screen_top, y_offsets, out); + collect_legacy_back_controls(child, screen_top, y_offsets, out); } } -fn is_back_control(node: &PenNode) -> bool { +fn is_legacy_back_control(node: &PenNode) -> bool { if matches!( node.base().role.as_deref(), Some("back" | "back-button" | "nav-back") @@ -656,10 +630,11 @@ fn is_back_control(node: &PenNode) -> bool { hay.contains("back") || hay.contains("arrowleft") || hay.contains("chevronleft") } -fn compact_lower(s: &str) -> String { - s.chars() - .filter(|c| c.is_ascii_alphanumeric()) - .flat_map(|c| c.to_lowercase()) +fn compact_lower(value: &str) -> String { + value + .chars() + .filter(|character| character.is_ascii_alphanumeric()) + .flat_map(|character| character.to_lowercase()) .collect() } @@ -694,17 +669,12 @@ pub fn subtree_has_events(node: &PenNode) -> bool { /// `/`-rooted route path; the JSON string VALUE is the literal /// `""` (quotes included) so it compiles as a Tier-1 string-literal /// expression — see the module doc, contract point 1. -fn navigate_patch(verb: &str, path: &str) -> String { +pub(crate) fn navigate_patch(verb: &str, path: &str) -> String { let body = serde_json::to_string(path).unwrap_or_default(); // -> "\"/path\"" let escaped_body = serde_json::to_string(&body).unwrap_or_default(); // -> "\"\\\"/path\\\"\"" format!(r#"{{"events":{{"onTap":[{{"{verb}":{escaped_body}}}]}}}}"#) } -/// Node id -> resolved absolute-doc-space Y offset, via the same jian layout -/// pass `snapshot_layout` / `geometry_validation` use. Only Y is needed here -/// (header-region gating); a fuller Rect lives in `geometry_validation` -/// scoped to its own module, mirroring the existing per-module -/// `resolved_widths` precedent in `sidebar_archetype`. fn resolved_y_offsets(state: &EditorState) -> HashMap { let scene = op_pen_loader::editor_state_to_active_page_layout_scene(state); let mut out = HashMap::new(); diff --git a/crates/op-orchestrator/src/wire_screen_navigation_tests.rs b/crates/op-orchestrator/src/wire_screen_navigation_tests.rs index cc015e01c..b86eec3a7 100644 --- a/crates/op-orchestrator/src/wire_screen_navigation_tests.rs +++ b/crates/op-orchestrator/src/wire_screen_navigation_tests.rs @@ -194,6 +194,172 @@ fn duplicate_slugs_get_numeric_suffix() { assert_eq!(frame_screen(b), Some("/settings-2")); } +#[test] +fn prompt_inventory_matches_the_routes_the_pass_persists() { + let json = r#"{"version":"1.0","children":[ + {"type":"frame","id":"authored","name":"Checkout","width":390,"height":844, + "layout":"vertical","children":[],"screen":"/buy-now"}, + {"type":"frame","id":"home","name":"Home","width":390,"height":844, + "layout":"vertical","children":[]}, + {"type":"frame","id":"settings-a","name":"Settings","width":390,"height":844, + "layout":"vertical","children":[]}, + {"type":"frame","id":"settings-b","name":"Settings","width":390,"height":844, + "layout":"vertical","children":[]} + ]}"#; + let mut state = state_from_json(json); + let inventory = screen_route_inventory::screen_route_inventory(&state); + run_pass(&mut state); + let persisted = state + .active_children() + .iter() + .filter_map(|node| { + let PenNode::Frame(frame) = node else { + return None; + }; + Some(( + frame.base.name.clone().unwrap_or_default(), + frame.screen.clone()?, + )) + }) + .collect::>(); + assert_eq!(inventory, persisted); + assert_eq!( + inventory, + vec![ + ("Checkout".into(), "/buy-now".into()), + ("Home".into(), "/".into()), + ("Settings".into(), "/settings".into()), + ("Settings".into(), "/settings-2".into()), + ] + ); +} + +#[test] +fn planned_inventory_merges_existing_entry_and_persists_midwidth_routes() { + let json = r#"{"version":"1.0","children":[ + {"type":"frame","id":"home","name":"Home","screen":"/", + "width":390,"height":844,"layout":"vertical","children":[]}, + {"type":"frame","id":"search","name":"Search", + "width":768,"height":1024,"layout":"vertical","children":[]}, + {"type":"frame","id":"profile","name":"Profile", + "width":768,"height":1024,"layout":"vertical","children":[]} + ]}"#; + let mut state = state_from_json(json); + let plan: crate::plan::OrchestratorPlan = serde_json::from_value(serde_json::json!({ + "rootFrame": { + "id": "root", + "name": "App", + "width": 768, + "height": 1024 + }, + "subtasks": [ + { + "id": "search-task", + "label": "Search", + "screen": "Search", + "parentFrameId": "search", + "region": {"width": 768, "height": 400} + }, + { + "id": "profile-task", + "label": "Profile", + "screen": "Profile", + "parentFrameId": "profile", + "region": {"width": 768, "height": 400} + } + ] + })) + .unwrap(); + + let planned = prompt_screen_route_inventory(&plan, &state); + assert_eq!( + planned, + vec![ + ("Home".into(), "/".into()), + ("Search".into(), "/search".into()), + ("Profile".into(), "/profile".into()), + ] + ); + + { + let mut sink = crate::loop_finalize::StateDocSink { state: &mut state }; + ensure_planned_screen_routes(&mut sink, &plan); + } + assert_eq!(prompt_screen_route_inventory(&plan, &state), planned); + assert_eq!( + ["home", "search", "profile"] + .into_iter() + .map(|id| frame_screen(find_by_id(state.active_children(), id).unwrap()).unwrap()) + .collect::>(), + vec!["/", "/search", "/profile"] + ); +} + +#[test] +fn authored_midwidth_entry_reserves_root_for_new_shaped_screen() { + let json = r#"{"version":"1.0","children":[ + {"type":"frame","id":"tablet-home","name":"Home","screen":"/", + "width":768,"height":1024,"layout":"vertical","children":[]}, + {"type":"frame","id":"detail","name":"Detail", + "width":390,"height":844,"layout":"vertical","children":[]} + ]}"#; + let mut state = state_from_json(json); + assert_eq!( + screen_route_inventory::screen_route_inventory(&state), + vec![ + ("Home".into(), "/".into()), + ("Detail".into(), "/detail".into()) + ] + ); + + run_pass(&mut state); + + assert_eq!( + frame_screen(find_by_id(state.active_children(), "detail").unwrap()), + Some("/detail") + ); +} + +#[test] +fn collapsed_plan_groups_fall_back_to_the_live_route_inventory() { + let state = state_from_json( + r#"{"version":"1.0","children":[ + {"type":"frame","id":"home","name":"Home","screen":"/", + "width":390,"height":844,"children":[]}, + {"type":"frame","id":"detail","name":"Detail","screen":"/detail", + "width":390,"height":844,"children":[]} + ]}"#, + ); + let plan: crate::plan::OrchestratorPlan = serde_json::from_value(serde_json::json!({ + "rootFrame": {"id": "root", "name": "App", "width": 390, "height": 844}, + "subtasks": [ + { + "id": "search-task", + "label": "Search", + "screen": "Search", + "parentFrameId": "home", + "region": {"width": 390, "height": 300} + }, + { + "id": "profile-task", + "label": "Profile", + "screen": "Profile", + "parentFrameId": "home", + "region": {"width": 390, "height": 300} + } + ] + })) + .unwrap(); + + assert_eq!( + prompt_screen_route_inventory(&plan, &state), + vec![ + ("Home".into(), "/".into()), + ("Detail".into(), "/detail".into()) + ] + ); +} + #[test] fn authored_screen_marker_is_never_overwritten_and_not_reused_as_entry() { // "Checkout" already carries an authored (non-"/") marker. Since the @@ -557,36 +723,36 @@ fn existing_tab_events_are_left_alone() { ); } -// ── Back-button wiring ────────────────────────────────────────────────── +// ── Cleanup-only interaction boundary ────────────────────────────────── #[test] -fn header_back_icon_gets_pop() { +fn preview_fallback_does_not_create_a_temporary_back_binding() { let json = r#"{"version":"1.0","children":[ {"type":"frame","id":"profile","name":"Profile","width":390,"height":844, - "layout":"vertical","children":[ - {"type":"icon_font","id":"back-icon","name":"Back","iconFontName":"arrow-left", - "width":24,"height":24} + "layout":"none","children":[ + {"type":"frame","id":"back","x":24,"y":80,"width":44,"height":44, + "layout":"none","children":[ + {"type":"icon_font","id":"back-icon","x":12,"y":12, + "iconFontName":"arrow-left","width":20,"height":20} + ]} ]}, {"type":"frame","id":"home","name":"Home","width":390,"height":844, "layout":"vertical","children":[]} ]}"#; let mut state = state_from_json(json); run_pass(&mut state); - let back = find_by_id(state.active_children(), "back-icon").unwrap(); - assert_eq!( - node_events_json(back).unwrap(), - serde_json::json!({"onTap": [{"pop": null}]}) + assert!( + node_events_json(find_by_id(state.active_children(), "back").unwrap()).is_none(), + "public preview fallback may assign routes/nav, but cleanup-only backfill must write \ + interactions to the real document" ); } -/// An authored interactive control owns its whole subtree: the arrow icon -/// INSIDE an already-bound back button must not receive a second `pop` -/// (both handlers firing on one tap would double-pop the route stack). #[test] -fn icon_inside_authored_back_button_is_not_double_bound() { +fn authored_back_binding_is_preserved_by_preview_fallback() { let json = r#"{"version":"1.0","children":[ {"type":"frame","id":"profile","name":"Profile","width":390,"height":844, - "layout":"vertical","children":[ + "layout":"none","children":[ {"type":"frame","id":"back-btn","name":"Back Button","width":44,"height":44, "events":{"onTap":[{"pop":null}]}, "children":[ @@ -598,32 +764,9 @@ fn icon_inside_authored_back_button_is_not_double_bound() { "layout":"vertical","children":[]} ]}"#; let mut state = state_from_json(json); + let before = node_events_json(find_by_id(state.active_children(), "back-btn").unwrap()); run_pass(&mut state); - let icon = find_by_id(state.active_children(), "inner-icon").unwrap(); - assert!( - node_events_json(icon).is_none(), - "a descendant of an authored interactive control must never be wired" - ); -} - -#[test] -fn back_icon_outside_header_region_is_not_bound() { - let json = r#"{"version":"1.0","children":[ - {"type":"frame","id":"profile","name":"Profile","width":390,"height":844, - "layout":"vertical","children":[ - {"type":"frame","id":"filler","name":"Filler","width":"fill_container","height":700, - "children":[]}, - {"type":"icon_font","id":"chevron-icon","name":"chevron-left","iconFontName":"chevron-left", - "width":24,"height":24} - ]}, - {"type":"frame","id":"home","name":"Home","width":390,"height":844, - "layout":"vertical","children":[]} - ]}"#; - let mut state = state_from_json(json); - run_pass(&mut state); - let chevron = find_by_id(state.active_children(), "chevron-icon").unwrap(); - assert!( - node_events_json(chevron).is_none(), - "a back-shaped icon well below the header band must not be bound" - ); + let after = node_events_json(find_by_id(state.active_children(), "back-btn").unwrap()); + assert_eq!(after, before); + assert!(node_events_json(find_by_id(state.active_children(), "inner-icon").unwrap()).is_none()); } diff --git a/crates/op-smoke/src/audit_rubric.rs b/crates/op-smoke/src/audit_rubric.rs index 0424f8631..ba574af23 100644 --- a/crates/op-smoke/src/audit_rubric.rs +++ b/crates/op-smoke/src/audit_rubric.rs @@ -9,13 +9,14 @@ //! `hasEntryScreen` / `navBoundTabs` / `navTotalTabs` / `popBound` / //! `appModeReady` quantify whether a generated document actually enters //! PreviewSession's routed multi-screen App Mode — `wire_screen_navigation` -//! (Track A) now auto-wires `screen` markers + nav-tab / back-button -//! `events.onTap` bindings at the end of every generation turn, so this is -//! the deterministic check that it actually landed, not just that the pass -//! exists. The nav-container / has-events predicates are reused directly -//! from `op_orchestrator::wire_screen_navigation` (bumped `pub` for this; +//! (Track A) now auto-wires `screen` markers + nav-tab bindings, while the +//! cleanup-only interaction backfill persists fact-proven back/card actions +//! at the end of every generation turn. This is the deterministic check that +//! the bindings actually landed, not just that the passes exist. The +//! nav-container / has-events predicates are reused directly from +//! `op_orchestrator::wire_screen_navigation` (bumped `pub` for this; //! op-smoke already depends on `op-orchestrator`) rather than reimplemented, -//! so the rubric can never silently disagree with what the pass wired. +//! so the rubric can never silently disagree with what the nav pass wired. //! //! M3 addition (content-completeness follow-up): `completeness` closes a //! blind spot the geometry + chrome + interactivity metrics above all share @@ -208,12 +209,11 @@ fn count_pop_bound_value(value: &Value) -> usize { } /// Whether a node's (already-serialized) `events.onTap` includes a bound -/// `pop` navigation action. Track A's own back-button wiring only ever -/// writes `{"pop": null}` (`wire_screen_navigation::wire_back_buttons`); -/// checked generically (rather than reusing a Track A "is this shaped like a -/// back button" name/icon heuristic) so a hand-authored pop binding on any -/// node counts too — this metric is "did a pop binding land", not "did the -/// back-button heuristic fire". +/// `pop` navigation action. The cleanup interaction backfill only ever writes +/// `{"pop": null}`; checked generically (rather than repeating its strict +/// geometry/icon-data predicate) so a hand-authored pop binding on any node +/// counts too — this metric is "did a pop binding land", not "did the strict +/// back-control fact match". fn has_pop_action(value: &Value) -> bool { value .get("events")