From 72fd294514972012ef81abfae66af01dbce14a8c Mon Sep 17 00:00:00 2001 From: Fini Date: Fri, 31 Jul 2026 21:05:55 +0800 Subject: [PATCH] feat(agent): honor root dimensions requested in the prompt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A prompt that names its canvas size ("1200x800", "390 宽") had no path to the root frame: planning always applied the desktop/mobile defaults, and cleanup was free to grow the root past whatever was asked for. Parse the request once (`request_dimensions`), apply it during plan normalization, state it in the compact prompt so the model builds to the same number, and carry a `preserve_requested_root_height` flag into cleanup through an explicit `CleanupPolicy`. The policy defaults to the historical behavior — only the fresh-root orchestrator path opts in — so append and modify runs are untouched. Cleanup needs the RESOLVED height to honor that flag without collapsing real content, so `geometry_validation` grows `resolved_node_height`, measuring the laid-out subtree against the node's own top edge rather than trusting the declared value. Planning corpus follows: the desktop sizes are labelled "Desktop default", so an explicit request reads as an override rather than a contradiction. Claude-Session: https://claude.ai/code/session_01FqKQqNj8exYwopGDpYUU7x --- .../skills/phases/planning/decomposition.md | 10 +- .../skills/phases/planning/design-type.md | 8 +- crates/op-orchestrator/src/cleanup.rs | 48 +++- .../src/cleanup_root_and_nav.rs | 13 +- .../src/cleanup_root_height_tests.rs | 115 ++++++++ crates/op-orchestrator/src/compact_prompt.rs | 58 +++- .../src/geometry_resolved_extent_tests.rs | 78 ++++++ .../src/geometry_validation.rs | 61 +++++ crates/op-orchestrator/src/lib.rs | 1 + crates/op-orchestrator/src/plan_normalize.rs | 18 +- .../src/plan_normalize_dimensions.rs | 28 ++ .../src/plan_normalize_dimensions_tests.rs | 104 +++++++ .../op-orchestrator/src/request_dimensions.rs | 259 ++++++++++++++++++ crates/op-orchestrator/src/run.rs | 5 +- .../op-orchestrator/src/run_orchestrator.rs | 10 +- crates/op-orchestrator/src/run_tests_core.rs | 41 +++ 16 files changed, 828 insertions(+), 29 deletions(-) create mode 100644 crates/op-orchestrator/src/geometry_resolved_extent_tests.rs create mode 100644 crates/op-orchestrator/src/plan_normalize_dimensions.rs create mode 100644 crates/op-orchestrator/src/plan_normalize_dimensions_tests.rs create mode 100644 crates/op-orchestrator/src/request_dimensions.rs diff --git a/crates/op-ai-skills/skills/phases/planning/decomposition.md b/crates/op-ai-skills/skills/phases/planning/decomposition.md index 760176c70..e27b5cb7a 100644 --- a/crates/op-ai-skills/skills/phases/planning/decomposition.md +++ b/crates/op-ai-skills/skills/phases/planning/decomposition.md @@ -25,7 +25,7 @@ If Type 0: OTHERWISE classify by purpose: 1. Multi-section page — marketing, promotional, or informational content designed to be scrolled (e.g. product sites, portfolios, company pages): - - Desktop: width=1200, height=0 (scrollable), 6-10 subtasks + - Desktop default: width=1200, height=0 (scrollable), 6-10 subtasks - Structure: navigation - hero - content sections - CTA - footer 2. Single-task SCREEN — full functional screen for one user task (e.g. login screen, signup screen, settings page, profile page): @@ -34,7 +34,7 @@ OTHERWISE classify by purpose: - NOT a single card/badge/modal — those are Type 0 components 3. Data-rich workspace — overview screens with metrics, tables, or management panels (e.g. dashboards, admin consoles, analytics): - - Desktop: width=1200, height=0, 2-5 subtasks + - Desktop default: width=1200, height=0, 2-5 subtasks - Structure: sidebar or topbar + content panels - Sidebar subtasks: a sidebar is a VERTICAL rail (brand block, stacked nav items, footer profile) — NEVER a horizontal navbar, NEVER a hero headline or marketing copy. @@ -60,14 +60,14 @@ RULES: - CJK FONT RULE: If the user's request is in Chinese/Japanese/Korean or the product targets CJK audiences, the styleGuide fonts MUST use CJK-compatible fonts: heading="Noto Sans SC" (Chinese) / "Noto Sans JP" (Japanese) / "Noto Sans KR" (Korean), body="Inter". NEVER use "Space Grotesk" or "Manrope" as heading font for CJK content — they have no CJK character support. - Root frame fill must use the background color from the selected style guide. Each guide in the list shows its bg color (e.g. bg:#0A0F1C). Use that exact hex value for the rootFrame fill color. - Root frame gap: Landing pages with distinct section backgrounds - gap=0 (sections flush). Mobile screens and dashboards - gap=16-24 (breathing room between sections). Always include "gap" in rootFrame. -- Root frame height: Mobile (width=375) - set height=812 (fixed viewport). Desktop (width=1200) - set height=0 (auto-expands as sections are generated). +- Root frame height: Mobile default (width=375) - set height=812 (fixed viewport). Desktop default (width=1200) - set height=0 (auto-expands as sections are generated). Preserve an explicit user-requested root height. - Landing page height hints: nav 64-80px, hero 500-600px, feature sections 400-600px, testimonials 300-400px, CTA 200-300px, footer 200-300px. - App screen height hints: status bar is pre-inserted (62px, do NOT plan a "Status Bar" section). Header 56-64px, form fields 48-56px each, buttons 48px, spacing 16-24px. - If a section is about "App截图"/"XX截图"/"screenshot"/"mockup", plan it as a phone mockup placeholder block, not a detailed mini-app reconstruction. - For landing pages: navigation sections should preserve good horizontal balance, links evenly distributed in the center group. - Regions tile to fill rootFrame. vertical = top-to-bottom. -- Mobile: 375x812 (both width AND height are fixed). Desktop: 1200x0 (width fixed, height auto-expands). -- WIDTH SELECTION: Type 0 components - width=400, height=0. Type 2 single-task SCREENS (login screen, profile page, settings page) - width=375, height=812 (mobile). Multi-section pages and data-rich workspaces (types 1 & 3) - width=1200, height=0 (desktop). A "profile card" is Type 0 (width=400), NOT Type 2. This is mandatory. +- Mobile default: 375x812 (both width AND height are fixed). Desktop default: 1200x0 (width fixed, height auto-expands). +- WIDTH SELECTION: Type 0 components default to width=400, height=0. Type 2 single-task SCREENS (login screen, profile page, settings page) default to width=375, height=812 (mobile). Multi-section pages and data-rich workspaces (types 1 & 3) default to width=1200, height=0 (desktop). A "profile card" is Type 0 (width=400), NOT Type 2. An explicit user-requested root width or width×height pair overrides every default and must be copied exactly into rootFrame. - MULTI-SCREEN APPS: When the request involves multiple distinct screens/pages (e.g. "登录页+个人中心", "login and profile", "continue generating the remaining 3 pages"), add "screen":"" to EVERY subtask. Each DISTINCT "screen" value becomes its OWN top-level root frame, placed as a separate sibling screen on the canvas; subtasks sharing the same "screen" value land together in that one root. Tagging is mandatory to get separate screens — if you omit "screen" (or give every subtask the SAME value), all subtasks collapse into a single shared root frame, even when the request clearly asked for multiple distinct pages. This applies starting at just 2-3 distinct screens — do NOT wait for a larger count before tagging; a separate skill's guidance about when to hand a screen off to a sub-agent (parallel delegation) is a DIFFERENT decision with its own higher threshold and has no bearing on whether you tag "screen" here. Use a concise page name per screen (e.g. "登录", "Profile") — it becomes that root frame's name. Single-screen requests don't need "screen" at all. Example (2 screens, "Login" then "Profile"): [{"id":"brand","label":"Brand Area","screen":"Login","region":{...}},{"id":"form","label":"Login Form","screen":"Login","region":{...}},{"id":"card","label":"User Card","screen":"Profile","region":{...}}] - SHARED CHROME ACROSS SCREENS: only plan a FULL bottom-nav/sidebar subtask for the FIRST screen. For every screen after that, either omit the nav subtask entirely or plan it as a minimal placeholder (no need to invent its own icon/label set) — a deterministic pass copies the first screen's nav onto every screen whose name matches one of that nav's tabs (even a screen with NO nav content at all, e.g. its own nav subtask failed) and fixes up which tab is active, so re-planning a full nav per screen wastes subtasks on content that gets replaced anyway. A screen whose name matches none of the nav's tabs (a standalone detail view, say) is correctly left alone — give it its own nav subtask only if it genuinely needs different chrome. - PUSH-IN DETAIL SCREENS NEVER GET A TAB BAR: a screen reached by tapping a card/row on another screen (a destination/product/article detail, not one of the bottom nav's own top-level destinations) never gets a "Bottom Navigation Bar" subtask, even a placeholder one. Give it a header with a Back control instead — returning relies on that Back tap (`pop`), not a tab switch. Only plan a nav subtask for a screen that genuinely IS one of the shared nav's tabs. diff --git a/crates/op-ai-skills/skills/phases/planning/design-type.md b/crates/op-ai-skills/skills/phases/planning/design-type.md index a67fd36cf..b1ef19398 100644 --- a/crates/op-ai-skills/skills/phases/planning/design-type.md +++ b/crates/op-ai-skills/skills/phases/planning/design-type.md @@ -34,7 +34,7 @@ If Type 0: OTHERWISE classify by purpose: 1. Multi-section page — marketing, promotional, or informational content designed to be scrolled (e.g. product sites, portfolios, company pages): - - Desktop: width=1200, height=0 (scrollable), 6-10 subtasks + - Desktop default: width=1200, height=0 (scrollable), 6-10 subtasks - Structure: navigation - hero - content sections - CTA - footer 2. Single-task screen — full functional SCREEN focused on one user task (e.g. login screen, signup screen, settings page, profile page, full onboarding flow): @@ -43,15 +43,15 @@ OTHERWISE classify by purpose: - NOT a single card/badge/modal — those are Type 0 components 3. Data-rich workspace — overview screens with metrics, tables, or management panels (e.g. dashboards, admin consoles, analytics): - - Desktop: width=1200, height=0, 2-5 subtasks + - Desktop default: width=1200, height=0, 2-5 subtasks - Structure: sidebar or topbar + content panels WIDTH SELECTION RULES: - Type 0 components — width=400, height=0 - Type 2 single-task SCREEN (login screen, profile page) — width=375, height=812 -- Types 1 & 3 (multi-section / dashboard) — width=1200, height=0 -- This mapping is mandatory. +- Types 1 & 3 (multi-section / dashboard) — default width=1200, height=0 +- An explicit user-requested root width or width×height pair overrides these defaults and must be used exactly. MOBILE vs MOCKUP: diff --git a/crates/op-orchestrator/src/cleanup.rs b/crates/op-orchestrator/src/cleanup.rs index c61e34301..3d17ede28 100644 --- a/crates/op-orchestrator/src/cleanup.rs +++ b/crates/op-orchestrator/src/cleanup.rs @@ -249,6 +249,16 @@ fn find_root<'a>(state: &'a EditorState, root_id: &str) -> Option<&'a PenNode> { op_editor_core::walkers::find_node(state.active_children(), &NodeId::new(root_id.to_string())) } +/// Cleanup intent that exists outside the generated document tree. +/// +/// The default deliberately preserves the historical cleanup behavior. Only +/// the fresh-root orchestrator path may opt into the request-derived height +/// contract; append, section, and whole-document loop finalization stay false. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub(crate) struct CleanupPolicy { + pub(crate) preserve_requested_root_height: bool, +} + /// 阶段 4 清理 pass —— 在全部 subtask 插入完成后运行。 /// /// `root_ids` 是本轮产出的根 frame id(S3a 单屏只有一个;S3b 并发 @@ -291,6 +301,17 @@ pub fn finalize_design_with_summary( run_cleanup_passes_with_summary(sink, plan, root_ids, summary); } +/// Fresh-root orchestrator variant carrying request-derived cleanup intent. +pub(crate) fn finalize_design_with_summary_and_policy( + sink: &mut dyn DocSink, + plan: &OrchestratorPlan, + root_ids: &[&str], + summary: &mut RepairSummary, + policy: CleanupPolicy, +) { + run_cleanup_passes_with_summary_and_policy(sink, plan, root_ids, summary, policy); +} + /// Env-gated (`OPENPENCIL_DEBUG_CLEANUP=1`) probe: log the named child's /// current height under `root_id`, tagged with the pass that just ran. fn debug_probe_child_height(sink: &dyn DocSink, root_id: &str, tag: &str) { @@ -338,6 +359,22 @@ pub fn run_cleanup_passes_with_summary( plan: &OrchestratorPlan, root_ids: &[&str], summary: &mut RepairSummary, +) { + run_cleanup_passes_with_summary_and_policy( + sink, + plan, + root_ids, + summary, + CleanupPolicy::default(), + ); +} + +fn run_cleanup_passes_with_summary_and_policy( + sink: &mut dyn DocSink, + plan: &OrchestratorPlan, + root_ids: &[&str], + summary: &mut RepairSummary, + policy: CleanupPolicy, ) { let mut counter = RepairCounter::new(); let mut counting = counter.wrap(sink); @@ -548,11 +585,12 @@ pub fn run_cleanup_passes_with_summary( } } } - let preserve_root_height = find_root(sink.state(), rid).is_some_and(|root| { - root_has_explicit_fit_content_height(root) - || has_explicit_mobile_viewport_contract(root) - || crate::mobile_reflow::has_mobile_trailing_nav_reflow_contract(root) - }); + let preserve_root_height = policy.preserve_requested_root_height + || find_root(sink.state(), rid).is_some_and(|root| { + root_has_explicit_fit_content_height(root) + || has_explicit_mobile_viewport_contract(root) + || crate::mobile_reflow::has_mobile_trailing_nav_reflow_contract(root) + }); if preserve_root_height { let mut guarded = PreserveRootHeightSink { inner: sink, diff --git a/crates/op-orchestrator/src/cleanup_root_and_nav.rs b/crates/op-orchestrator/src/cleanup_root_and_nav.rs index cd6653db6..c84f6d042 100644 --- a/crates/op-orchestrator/src/cleanup_root_and_nav.rs +++ b/crates/op-orchestrator/src/cleanup_root_and_nav.rs @@ -63,7 +63,18 @@ pub(super) fn adjust_root_height_to_content( if root_has_explicit_fit_content_height(root) { return; } - (root_content_height(root), root.height_px()) + let estimated = root_content_height(root); + // The tree estimator intentionally has no font shaper, so wrapped + // fit-content text can make the real jian layout a little taller. + // Reconcile against that same resolved scene used by diagnostics. + let resolved = crate::geometry_validation::resolved_node_height(sink.state(), root_id) + .filter(|height| height.is_finite() && *height > 0.0) + .map(|height| height.ceil() as i32); + let required = match (estimated, resolved) { + (Some(estimated), Some(resolved)) => Some(estimated.max(resolved)), + (estimated, resolved) => estimated.or(resolved), + }; + (required, root.height_px()) }; // Non-mobile roots only GROW a too-short fixed height to fit overflowing diff --git a/crates/op-orchestrator/src/cleanup_root_height_tests.rs b/crates/op-orchestrator/src/cleanup_root_height_tests.rs index 880933e60..2f4c5eb4b 100644 --- a/crates/op-orchestrator/src/cleanup_root_height_tests.rs +++ b/crates/op-orchestrator/src/cleanup_root_height_tests.rs @@ -393,6 +393,121 @@ fn cleanup_expands_zero_height_desktop_root_from_fit_content_children() { assert_eq!(root.height_px(), Some(316.0)); } +#[test] +fn cleanup_reconciles_root_height_with_resolved_wrapped_text() { + let mut sink = VecDocSink::new(); + let tree: PenNode = serde_json::from_value(json!({ + "type": "frame", + "id": "root", + "name": "Long Landing Page", + "width": 320, + "height": 40, + "layout": "vertical", + "children": [{ + "type": "text", + "id": "copy", + "name": "Wrapped Copy", + "content": "A deliberately long fixed-width sentence that wraps across several lines in the real layout engine.", + "width": 88, + "textGrowth": "fixed-width", + "fontSize": 18, + "lineHeight": 1.5 + }] + })) + .expect("wrapped text root json"); + sink.state.apply(EditorCommand::InsertSubtree { + nodes: vec![tree], + parent_id: NodeId::NONE, + page_id: None, + }); + let root_id = sink.state.active_children()[0].id_str().to_string(); + sink.applied.clear(); + + run_cleanup_passes(&mut sink, &plan(), &[&root_id]); + + let declared = sink + .state + .active_children() + .iter() + .find(|node| node.id_str() == root_id) + .and_then(PenNodeExt::height_px) + .expect("numeric root height"); + let resolved = crate::geometry_validation::resolved_node_height(&sink.state, &root_id) + .expect("resolved root height"); + assert!( + declared + 0.5 >= resolved, + "declared {declared}px must contain resolved {resolved}px" + ); +} + +fn overfull_desktop_artboard() -> (VecDocSink, String) { + let mut sink = VecDocSink::new(); + let tree: PenNode = serde_json::from_value(json!({ + "type": "frame", + "id": "root", + "name": "Explicit Desktop Artboard", + "width": 1440, + "height": 900, + "layout": "vertical", + "gap": 24, + "children": [ + {"type":"frame", "id":"upper", "name":"Upper Section", + "width":"fill_container", "height":500, "children":[]}, + {"type":"frame", "id":"lower", "name":"Lower Section", + "width":"fill_container", "height":500, "children":[]} + ] + })) + .expect("desktop artboard json"); + sink.state.apply(EditorCommand::InsertSubtree { + nodes: vec![tree], + parent_id: NodeId::NONE, + page_id: None, + }); + let root_id = sink.state.active_children()[0].id_str().to_string(); + sink.applied.clear(); + (sink, root_id) +} + +#[test] +fn cleanup_policy_preserves_only_requested_fixed_root_height() { + let (mut growing_sink, growing_root_id) = overfull_desktop_artboard(); + run_cleanup_passes(&mut growing_sink, &plan(), &[&growing_root_id]); + let grown_height = growing_sink + .state + .active_children() + .iter() + .find(|node| node.base().name.as_deref() == Some("Explicit Desktop Artboard")) + .and_then(PenNodeExt::height_px) + .expect("grown root height"); + assert!( + grown_height > 900.0, + "default cleanup must keep growing ordinary overfull roots" + ); + + let (mut preserved_sink, preserved_root_id) = overfull_desktop_artboard(); + let mut summary = RepairSummary::default(); + run_cleanup_passes_with_summary_and_policy( + &mut preserved_sink, + &plan(), + &[&preserved_root_id], + &mut summary, + CleanupPolicy { + preserve_requested_root_height: true, + }, + ); + let preserved_height = preserved_sink + .state + .active_children() + .iter() + .find(|node| node.base().name.as_deref() == Some("Explicit Desktop Artboard")) + .and_then(PenNodeExt::height_px) + .expect("preserved root height"); + assert_eq!( + preserved_height, 900.0, + "request-derived cleanup policy must freeze the explicit 1440x900 root" + ); +} + #[test] fn cleanup_recolors_safe_dark_bottom_nav_on_light_mobile_root() { let mut sink = VecDocSink::new(); diff --git a/crates/op-orchestrator/src/compact_prompt.rs b/crates/op-orchestrator/src/compact_prompt.rs index caecbd430..638f48185 100644 --- a/crates/op-orchestrator/src/compact_prompt.rs +++ b/crates/op-orchestrator/src/compact_prompt.rs @@ -6,6 +6,7 @@ use crate::design_md_policy::{ build_design_md_style_policy, guess_neutral_background_from_theme, infer_design_md_background, }; use crate::design_type::{detect_design_type, DesignType}; +use crate::request_dimensions::requested_root_dimensions; use crate::style_guide_context::infer_tags_from_prompt; use jian_ops_schema::DesignMdSpec; use op_ai_skills::style_guide::{ @@ -92,19 +93,36 @@ pub fn build_compact_planning_prompt( DesignType::LandingPage => "Create 4-8 scrollable page sections in top-to-bottom order.", }; - let mobile_rules: Vec<&str> = match preset.type_ { + let size_rule = if let Some(dimensions) = requested_root_dimensions(prompt) { + format!( + "The user explicitly requested the root dimensions. Use width={} and height={} on the root frame exactly.", + dimensions.width, + dimensions.height.unwrap_or(0.0) + ) + } else { + match preset.type_ { + DesignType::MobileScreen => { + "Use width=375 and height=812 on the root frame.".to_string() + } + DesignType::Component => "Use width=400 and height=0 on the root frame.".to_string(), + _ => "Use width=1200 and height=0 on the root frame.".to_string(), + } + }; + + let mobile_rules: Vec = match preset.type_ { DesignType::MobileScreen => vec![ - "This is a direct mobile screen, not a phone mockup.", - "Do NOT create a status bar section. The status bar is inserted separately.", - "Use width=375 and height=812 on the root frame.", + "This is a direct mobile screen, not a phone mockup.".to_string(), + "Do NOT create a status bar section. The status bar is inserted separately." + .to_string(), + size_rule, ], DesignType::Component => vec![ - "This is a single component (Type 0), not a screen.", - "Do NOT create a status bar, navigation, or footer section.", - "Use width=400 and height=0 on the root frame.", - "Use exactly 1 subtask for the component itself.", + "This is a single component (Type 0), not a screen.".to_string(), + "Do NOT create a status bar, navigation, or footer section.".to_string(), + size_rule, + "Use exactly 1 subtask for the component itself.".to_string(), ], - _ => vec!["Use width=1200 and height=0 on the root frame."], + _ => vec![size_rule], }; let style_rule = if design_md.is_some() { @@ -137,8 +155,8 @@ pub fn build_compact_planning_prompt( .to_string(), style_rule, ]; - for r in &mobile_rules { - lines.push((*r).to_string()); + for rule in mobile_rules { + lines.push(rule); } lines.push(format!( "Always set rootFrame layout=\"vertical\" and gap={default_gap}." @@ -194,6 +212,24 @@ mod tests { assert!(!cp.selected_style_guide_name.is_empty()); } + #[test] + fn compact_prompt_honors_explicit_dimension_pair() { + let cp = + build_compact_planning_prompt("Design a 1440×900 desktop operations dashboard", None); + assert!(cp.system.contains("width=1440 and height=900")); + assert!(!cp.system.contains("width=1200 and height=0")); + } + + #[test] + fn compact_prompt_honors_explicit_wide_root() { + let cp = build_compact_planning_prompt( + "Design a desktop landing page. Make the root exactly 1440px wide.", + None, + ); + assert!(cp.system.contains("width=1440 and height=0")); + assert!(!cp.system.contains("width=1200 and height=0")); + } + #[test] fn compact_design_md_forces_custom_name() { let spec = jian_ops_schema::DesignMdSpec { diff --git a/crates/op-orchestrator/src/geometry_resolved_extent_tests.rs b/crates/op-orchestrator/src/geometry_resolved_extent_tests.rs new file mode 100644 index 000000000..ddf321579 --- /dev/null +++ b/crates/op-orchestrator/src/geometry_resolved_extent_tests.rs @@ -0,0 +1,78 @@ +use super::resolved_node_height; +use crate::cleanup::run_cleanup_passes; +use crate::plan::{OrchestratorPlan, RootFrameSpec}; +use crate::test_support::VecDocSink; +use jian_ops_schema::node::PenNode; +use op_editor_core::{EditorCommand, NodeId, PenNodeExt}; +use serde_json::json; + +fn plan() -> OrchestratorPlan { + OrchestratorPlan { + root_frame: RootFrameSpec { + id: "root".into(), + name: "Wrapped Copy".into(), + width: 320.0, + height: 40.0, + layout: Some("vertical".into()), + gap: None, + padding: None, + fill: None, + }, + subtasks: Vec::new(), + style_guide_name: None, + } +} + +#[test] +fn cleanup_grows_fixed_root_to_resolved_wrapped_text_bottom() { + let mut sink = VecDocSink::new(); + let tree: PenNode = serde_json::from_value(json!({ + "type": "frame", + "id": "root", + "name": "Long Landing Page", + "width": 320, + "height": 40, + "layout": "vertical", + "children": [{ + "type": "text", + "id": "copy", + "name": "Wrapped Copy", + "content": "A deliberately long fixed-width sentence that wraps across several lines in the real layout engine.", + "width": 88, + "textGrowth": "fixed-width", + "fontSize": 18, + "lineHeight": 1.5 + }] + })) + .expect("wrapped text root json"); + sink.state.apply(EditorCommand::InsertSubtree { + nodes: vec![tree], + parent_id: NodeId::NONE, + page_id: None, + }); + let root_id = sink.state.active_children()[0].id_str().to_string(); + + let overflowing_extent = + resolved_node_height(&sink.state, &root_id).expect("resolved content extent"); + assert!( + overflowing_extent > 40.0, + "wrapped text must resolve past the fixed root, got {overflowing_extent}px" + ); + + run_cleanup_passes(&mut sink, &plan(), &[&root_id]); + + let declared = sink + .state + .active_children() + .iter() + .find(|node| node.id_str() == root_id) + .and_then(PenNodeExt::height_px) + .expect("numeric root height"); + let resolved = + resolved_node_height(&sink.state, &root_id).expect("post-cleanup resolved content extent"); + assert!(declared > 40.0, "cleanup must grow the fixed 40px root"); + assert!( + declared + 0.5 >= resolved, + "declared {declared}px must contain descendant bottom at {resolved}px" + ); +} diff --git a/crates/op-orchestrator/src/geometry_validation.rs b/crates/op-orchestrator/src/geometry_validation.rs index 9a1591d97..851951024 100644 --- a/crates/op-orchestrator/src/geometry_validation.rs +++ b/crates/op-orchestrator/src/geometry_validation.rs @@ -103,6 +103,63 @@ fn resolved_rects(state: &EditorState) -> HashMap { map } +pub(crate) fn resolved_node_height(state: &EditorState, node_id: &str) -> Option { + let scene = op_pen_loader::editor_state_to_active_page_layout_scene(state); + let page = scene.active_page()?; + let root = find_scene_node(&page.children, node_id)?; + resolved_subtree_height(root) +} + +fn find_scene_node<'a>(nodes: &'a [SceneNode], node_id: &str) -> Option<&'a SceneNode> { + for node in nodes { + if node.id == node_id { + return Some(node); + } + if let Some(found) = find_scene_node(&node.children, node_id) { + return Some(found); + } + } + None +} + +/// Measure the resolved subtree against the node's own top edge. +/// +/// `SceneNode::aggregate_bounds()` intentionally returns a bounded node's own +/// rectangle, so it cannot reveal descendants that overflow a fixed-height +/// root. Raw scene bounds are absolute; walking every descendant preserves the +/// real layout bottom even when an ancestor clips or has an authored height. +fn resolved_subtree_height(root: &SceneNode) -> Option { + let root_top = f64::from(root.bounds.origin.y); + if !root_top.is_finite() { + return None; + } + let bottom = max_raw_bottom(root)?; + let height = bottom - root_top; + height.is_finite().then_some(height.max(0.0)) +} + +fn max_raw_bottom(node: &SceneNode) -> Option { + let y = f64::from(node.bounds.origin.y); + let height = f64::from(node.bounds.size.y); + let own_bottom = if y.is_finite() && height.is_finite() && height >= 0.0 { + let bottom = y + height; + bottom.is_finite().then_some(bottom) + } else { + None + }; + + node.children + .iter() + .filter_map(max_raw_bottom) + .fold(own_bottom, |max_bottom, child_bottom| { + Some( + max_bottom + .map(|current| current.max(child_bottom)) + .unwrap_or(child_bottom), + ) + }) +} + fn collect_rects(nodes: &[SceneNode], map: &mut HashMap) { for n in nodes { let b = n.aggregate_bounds(); @@ -332,3 +389,7 @@ mod rail_collapse_tests; #[cfg(test)] #[path = "geometry_compact_status_tests.rs"] mod compact_status_tests; + +#[cfg(test)] +#[path = "geometry_resolved_extent_tests.rs"] +mod resolved_extent_tests; diff --git a/crates/op-orchestrator/src/lib.rs b/crates/op-orchestrator/src/lib.rs index 1d1e387c2..91ea9ade4 100644 --- a/crates/op-orchestrator/src/lib.rs +++ b/crates/op-orchestrator/src/lib.rs @@ -29,6 +29,7 @@ pub mod plan; pub mod plan_normalize; pub mod plan_repair; pub mod program_gen; +mod request_dimensions; mod resolved_style_prompt; pub mod retry; pub mod script_gen; diff --git a/crates/op-orchestrator/src/plan_normalize.rs b/crates/op-orchestrator/src/plan_normalize.rs index d5e79d2f3..7007972b7 100644 --- a/crates/op-orchestrator/src/plan_normalize.rs +++ b/crates/op-orchestrator/src/plan_normalize.rs @@ -14,6 +14,9 @@ use crate::types::DesignRequest; mod plan_home_intent; use plan_home_intent::plan_is_app_home_screen; +#[path = "plan_normalize_dimensions.rs"] +mod plan_normalize_dimensions; + // multiscreen-fanout-break fix (item A) — screen-grouping tests, split out // to keep this file's inline `mod tests` from crossing the 800-line cap. #[cfg(test)] @@ -24,11 +27,18 @@ mod tests_screen_groups; #[path = "plan_normalize_nav_tests.rs"] mod tests_nav; +#[cfg(test)] +#[path = "plan_normalize_dimensions_tests.rs"] +mod tests_dimensions; + /// 规范化产出的派生信息。 #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct NormInfo { /// 根 frame 窄到移动端宽度 —— scaffold 阶段据此注入固定状态栏。 pub is_mobile: bool, + /// The request explicitly fixed both root dimensions, so fresh-root + /// cleanup must preserve the requested height instead of growing it. + pub preserve_requested_root_height: bool, } /// 移动端宽度上限(含)—— ≤ 此值视为移动端单屏。 @@ -154,6 +164,9 @@ fn ensure_requested_bottom_nav_subtask(plan: &mut OrchestratorPlan, req: &Design /// LLM 值,超出则取推断值 —— 忠实 TS `normalizeOrchestratorPlan` /// `orchestrator.ts:259-272`)。 pub fn normalize(plan: &mut OrchestratorPlan, req: &DesignRequest) -> NormInfo { + let preserve_requested_root_height = + plan_normalize_dimensions::apply_requested_root_dimensions(plan, req); + let is_mobile = plan.root_frame.width <= MOBILE_MAX_WIDTH; if is_mobile { @@ -228,7 +241,10 @@ pub fn normalize(plan: &mut OrchestratorPlan, req: &DesignRequest) -> NormInfo { } } - NormInfo { is_mobile } + NormInfo { + is_mobile, + preserve_requested_root_height, + } } #[cfg(test)] diff --git a/crates/op-orchestrator/src/plan_normalize_dimensions.rs b/crates/op-orchestrator/src/plan_normalize_dimensions.rs new file mode 100644 index 000000000..5648fcd47 --- /dev/null +++ b/crates/op-orchestrator/src/plan_normalize_dimensions.rs @@ -0,0 +1,28 @@ +//! Request-derived root dimensions applied before the remaining plan rules. + +use crate::plan::OrchestratorPlan; +use crate::request_dimensions::requested_root_dimensions; +use crate::types::DesignRequest; + +/// Apply explicit root dimensions and report whether height was also fixed. +pub(super) fn apply_requested_root_dimensions( + plan: &mut OrchestratorPlan, + req: &DesignRequest, +) -> bool { + let Some(dimensions) = requested_root_dimensions(&req.prompt) else { + return false; + }; + let planned_width = plan.root_frame.width; + plan.root_frame.width = dimensions.width; + if let Some(height) = dimensions.height { + plan.root_frame.height = height; + } + if (planned_width - dimensions.width).abs() > f64::EPSILON { + for subtask in &mut plan.subtasks { + if (subtask.region.width - planned_width).abs() <= 1.0 { + subtask.region.width = dimensions.width; + } + } + } + dimensions.height.is_some() +} diff --git a/crates/op-orchestrator/src/plan_normalize_dimensions_tests.rs b/crates/op-orchestrator/src/plan_normalize_dimensions_tests.rs new file mode 100644 index 000000000..1a2d94841 --- /dev/null +++ b/crates/op-orchestrator/src/plan_normalize_dimensions_tests.rs @@ -0,0 +1,104 @@ +//! Explicit root-dimension regressions for `plan_normalize`. + +use super::*; +use crate::plan::{OrchestratorPlan, Region, RootFrameSpec, Subtask}; + +fn request(prompt: &str) -> DesignRequest { + DesignRequest { + prompt: prompt.into(), + model: None, + provider: None, + design_md: None, + concurrency: 1, + append_context: None, + validation_enabled: true, + visual_ref_enabled: false, + } +} + +fn subtask(id: &str, label: &str, width: f64, height: f64) -> Subtask { + Subtask { + id: id.into(), + label: label.into(), + region: Region { width, height }, + id_prefix: String::new(), + parent_frame_id: None, + elements: None, + screen: None, + generated_root_id: None, + existing_section_labels: None, + retry_feedback: None, + } +} + +fn plan(name: &str, subtasks: Vec) -> OrchestratorPlan { + OrchestratorPlan { + root_frame: RootFrameSpec { + id: "root".into(), + name: name.into(), + width: 1200.0, + height: 800.0, + layout: None, + gap: None, + padding: None, + fill: None, + }, + subtasks, + style_guide_name: None, + } +} + +#[test] +fn normalize_applies_explicit_root_dimensions_before_dashboard_sizing() { + let mut plan = plan( + "Dashboard", + vec![ + subtask("sidebar", "Sidebar Navigation", 100.0, 500.0), + subtask("kpi-metrics", "KPI Metrics", 800.0, 0.0), + ], + ); + let norm = normalize(&mut plan, &request("Design a 1440×900 analytics dashboard")); + + assert_eq!(plan.root_frame.width, 1440.0); + assert_eq!(plan.root_frame.height, 900.0); + assert!( + norm.preserve_requested_root_height, + "an explicit width-height pair must become a cleanup contract" + ); + let metric = plan + .subtasks + .iter() + .find(|subtask| subtask.id == "kpi-metrics") + .unwrap(); + assert_eq!( + metric.region.width, 1180.0, + "dashboard main width must derive from the requested 1440px root" + ); +} + +#[test] +fn normalize_updates_full_width_landing_regions_for_explicit_width() { + let mut plan = plan( + "Landing Page", + vec![ + subtask("nav", "Navigation Bar", 1200.0, 100.0), + subtask("hero", "Hero Section", 640.0, 100.0), + ], + ); + let norm = normalize( + &mut plan, + &request("Design a desktop landing page. Make the root exactly 1440px wide."), + ); + + assert_eq!(plan.root_frame.width, 1440.0); + assert_eq!(plan.root_frame.height, 800.0); + assert!( + !norm.preserve_requested_root_height, + "width-only requests must still let cleanup grow the root height" + ); + assert_eq!(plan.subtasks[0].region.width, 1440.0); + assert_eq!( + plan.subtasks[1].region.width, 640.0, + "an intentionally partial-width region must stay partial" + ); +} diff --git a/crates/op-orchestrator/src/request_dimensions.rs b/crates/op-orchestrator/src/request_dimensions.rs new file mode 100644 index 000000000..0c1e1b432 --- /dev/null +++ b/crates/op-orchestrator/src/request_dimensions.rs @@ -0,0 +1,259 @@ +//! Fact-based root dimensions explicitly requested in the user prompt. + +use regex::Regex; +use std::sync::OnceLock; + +#[derive(Debug, Clone, Copy, PartialEq)] +pub(crate) struct RequestedRootDimensions { + pub width: f64, + pub height: Option, +} + +#[derive(Debug, Clone, Copy)] +struct DimensionCandidate { + start: usize, + end: usize, + dimensions: RequestedRootDimensions, +} + +const CONTEXT_RADIUS: usize = 56; +const ROOT_CONTEXT_TERMS: &[&str] = &[ + "root", + "artboard", + "page", + "screen", + "canvas", + // "desktop dashboard" is the catalog's natural-language shorthand for + // one desktop screen, including the accepted "1440x900 desktop dashboard" + // form where no literal "screen" token is present. + "desktop", + "dashboard", + "根画板", + "画板", + "页面", + "屏幕", + "画布", +]; +const NESTED_CONTEXT_TERMS: &[&str] = &[ + "hero", + "image", + "card", + "banner", + "thumbnail", + "photo", + "插图", + "图片", + "卡片", +]; + +fn valid_dimension(value: u32) -> bool { + (240..=10_000).contains(&value) +} + +fn pair_regex() -> &'static Regex { + static PAIR: OnceLock = OnceLock::new(); + PAIR.get_or_init(|| { + Regex::new(r"(?i)([0-9]{3,5})\s*(?:px\s*)?x\s*([0-9]{3,5})(?:\s*px)?") + .expect("root dimension pair regex") + }) +} + +fn width_regex() -> &'static Regex { + static WIDTH: OnceLock = OnceLock::new(); + WIDTH.get_or_init(|| { + Regex::new(r"(?i)([0-9]{3,5})\s*(?:px|pixels?)\s+wide").expect("root width regex") + }) +} + +fn is_word_char(ch: char) -> bool { + ch.is_ascii_alphanumeric() || ch == '_' +} + +fn term_has_boundaries(text: &str, start: usize, term: &str) -> bool { + if !term.is_ascii() { + return true; + } + let before = text[..start].chars().next_back(); + let after = text[start + term.len()..].chars().next(); + before.is_none_or(|ch| !is_word_char(ch)) && after.is_none_or(|ch| !is_word_char(ch)) +} + +fn range_distance(start: usize, end: usize, other_start: usize, other_end: usize) -> usize { + if other_end <= start { + start.saturating_sub(other_end) + } else if end <= other_start { + other_start.saturating_sub(end) + } else { + 0 + } +} + +fn nearest_term_distance( + text: &str, + candidate: DimensionCandidate, + terms: &[&str], +) -> Option { + terms + .iter() + .flat_map(|term| { + text.match_indices(term) + .filter(move |(start, _)| term_has_boundaries(text, *start, term)) + .map(move |(start, _)| { + range_distance(candidate.start, candidate.end, start, start + term.len()) + }) + }) + .filter(|distance| *distance <= CONTEXT_RADIUS) + .min() +} + +fn is_root_scoped(text: &str, candidate: DimensionCandidate) -> bool { + let Some(root_distance) = nearest_term_distance(text, candidate, ROOT_CONTEXT_TERMS) else { + return false; + }; + nearest_term_distance(text, candidate, NESTED_CONTEXT_TERMS) + .is_none_or(|nested_distance| root_distance < nested_distance) +} + +fn pair_candidates(text: &str) -> Vec { + pair_regex() + .captures_iter(text) + .filter_map(|captures| { + let whole = captures.get(0)?; + let width = captures.get(1)?.as_str().parse::().ok()?; + let height = captures.get(2)?.as_str().parse::().ok()?; + (valid_dimension(width) && valid_dimension(height)).then_some(DimensionCandidate { + start: whole.start(), + end: whole.end(), + dimensions: RequestedRootDimensions { + width: f64::from(width), + height: Some(f64::from(height)), + }, + }) + }) + .collect() +} + +fn width_candidates(text: &str, pair_candidates: &[DimensionCandidate]) -> Vec { + width_regex() + .captures_iter(text) + .filter_map(|captures| { + let whole = captures.get(0)?; + if pair_candidates + .iter() + .any(|pair| whole.start() < pair.end && pair.start < whole.end()) + { + return None; + } + let width = captures.get(1)?.as_str().parse::().ok()?; + valid_dimension(width).then_some(DimensionCandidate { + start: whole.start(), + end: whole.end(), + dimensions: RequestedRootDimensions { + width: f64::from(width), + height: None, + }, + }) + }) + .collect() +} + +pub(crate) fn requested_root_dimensions(prompt: &str) -> Option { + let normalized = prompt.to_lowercase().replace('×', "x"); + let pairs = pair_candidates(&normalized); + let mut candidates = pairs.clone(); + candidates.extend(width_candidates(&normalized, &pairs)); + + // A later, explicitly root-scoped statement wins. This matters for prompts + // that first size a card/image and then state the page/root width. + candidates + .into_iter() + .filter(|candidate| is_root_scoped(&normalized, *candidate)) + .max_by_key(|candidate| candidate.start) + .map(|candidate| candidate.dimensions) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn dimensions(width: f64, height: Option) -> Option { + Some(RequestedRootDimensions { width, height }) + } + + #[test] + fn parses_dimension_pair_with_multiplication_sign() { + assert_eq!( + requested_root_dimensions("Design a 1440×900 desktop analytics dashboard"), + dimensions(1440.0, Some(900.0)) + ); + } + + #[test] + fn parses_explicit_root_and_artboard_pairs() { + assert_eq!( + requested_root_dimensions("Use a root frame sized 1366 x 768px."), + dimensions(1366.0, Some(768.0)) + ); + assert_eq!( + requested_root_dimensions("Canvas artboard: 1600px x 1000px"), + dimensions(1600.0, Some(1000.0)) + ); + } + + #[test] + fn parses_explicit_pixel_width_without_guessing_height() { + assert_eq!( + requested_root_dimensions( + "Make the root exactly 1440px wide and between 2400 and 5200px tall" + ), + dimensions(1440.0, None) + ); + } + + #[test] + fn rejects_nested_hero_image_and_card_dimensions() { + for prompt in [ + "Design a page with a hero image sized 1440×900.", + "On the dashboard, make the card 420px wide.", + "Create a screen whose thumbnail image is 640 x 360.", + ] { + assert_eq!( + requested_root_dimensions(prompt), + None, + "nested dimensions must not become the root contract: {prompt}" + ); + } + } + + #[test] + fn later_root_width_wins_over_earlier_card_width() { + assert_eq!( + requested_root_dimensions( + "Use a 420px wide card in the hero, then make the page root 1440px wide." + ), + dimensions(1440.0, None) + ); + } + + #[test] + fn later_page_pair_wins_over_earlier_image_pair() { + assert_eq!( + requested_root_dimensions( + "The hero image is 1200x600. Render the desktop page at 1440x900." + ), + dimensions(1440.0, Some(900.0)) + ); + } + + #[test] + fn ignores_aspect_ratios_and_unqualified_numbers() { + assert_eq!( + requested_root_dimensions("Crop the top 900px to a 16:10 preview"), + None + ); + assert_eq!( + requested_root_dimensions("Use a 1440x900 image without creating an artboard."), + None + ); + } +} diff --git a/crates/op-orchestrator/src/run.rs b/crates/op-orchestrator/src/run.rs index d5f85dc89..0331cf4b0 100644 --- a/crates/op-orchestrator/src/run.rs +++ b/crates/op-orchestrator/src/run.rs @@ -21,7 +21,10 @@ //! 一条独立消费方,不受此影响。 use crate::append::apply_append_context_to_plan; -use crate::cleanup::{descendant_count, finalize_design_with_summary}; +use crate::cleanup::{ + descendant_count, finalize_design_with_summary, finalize_design_with_summary_and_policy, + CleanupPolicy, +}; use crate::model_profile::resolve_model_profile; use crate::plan::{build_fallback_plan, OrchestratorPlan}; use crate::plan_normalize::{normalize, NormInfo}; diff --git a/crates/op-orchestrator/src/run_orchestrator.rs b/crates/op-orchestrator/src/run_orchestrator.rs index 4ef7dfa5f..d38621ddd 100644 --- a/crates/op-orchestrator/src/run_orchestrator.rs +++ b/crates/op-orchestrator/src/run_orchestrator.rs @@ -622,7 +622,15 @@ impl Orchestrator { // Passing them all is still correct scoping for the OTHER // whole-root cleanup passes (dedup / avatar-repair / etc.). let root_id_refs: Vec<&str> = root_ids.iter().map(String::as_str).collect(); - finalize_design_with_summary(sink, &plan, &root_id_refs, &mut quality); + finalize_design_with_summary_and_policy( + sink, + &plan, + &root_id_refs, + &mut quality, + CleanupPolicy { + preserve_requested_root_height: norm.preserve_requested_root_height, + }, + ); } on_progress(Progress::CleanupDone); // Turn the cleanup stage's tally into a user-visible credential. Only diff --git a/crates/op-orchestrator/src/run_tests_core.rs b/crates/op-orchestrator/src/run_tests_core.rs index 1ba6717b3..6a7789077 100644 --- a/crates/op-orchestrator/src/run_tests_core.rs +++ b/crates/op-orchestrator/src/run_tests_core.rs @@ -47,6 +47,47 @@ fn run_happy_path_applies_scaffold_and_subtasks() { ); } +#[test] +fn fresh_run_preserves_explicit_root_height_through_cleanup() { + let tall_section = |name: &str| { + format!( + r#"I(null, {{"type":"frame","name":"{name}","width":"fill_container","height":560,"children":[{{"type":"text","content":"{name}","fontSize":18}}]}});"# + ) + }; + let llm = ScriptedLlm::new(vec![ + ScriptResponse::Text(PLAN_JSON.into()), + ScriptResponse::Text(tall_section("Tall Hero")), + ScriptResponse::Text(tall_section("Tall Features")), + ]); + let mut sink = VecDocSink::new(); + let mut request = req(); + request.prompt = "Design a 1440×900 desktop page".into(); + request.validation_enabled = false; + + futures::executor::block_on(Orchestrator::new().run( + request, + &mut sink, + &llm, + &mut |_| {}, + &AbortFlag::new(), + &stub_providers(), + )) + .expect("explicit-size run succeeds"); + + let root = sink + .state + .active_children() + .iter() + .find(|node| node.base().name.as_deref() == Some("Page")) + .expect("generated page root"); + assert_eq!(root.width_px(), Some(1440.0)); + assert_eq!( + root.height_px(), + Some(900.0), + "fresh-run cleanup must not grow an explicitly requested root height" + ); +} + #[test] fn run_mobile_scaffold_reveals_status_bar() { let _guard = crate::agent_indicator_test_support::lock();