diff --git a/crates/op-orchestrator/src/app_shell.rs b/crates/op-orchestrator/src/app_shell.rs new file mode 100644 index 000000000..67a5df5e2 --- /dev/null +++ b/crates/op-orchestrator/src/app_shell.rs @@ -0,0 +1,318 @@ +//! App-shell restructure — a single deterministic structural post-pass that +//! turns a flat-vertical desktop dashboard whose FIRST child is a full-width +//! sidebar into a horizontal `[sidebar(fixed) | content-column(fill)]` shell. +//! +//! Weak models (glm-5.x etc.) routinely emit a "Sidebar Navigation" as the +//! first child of a vertical root at the *full* page width, so it renders as a +//! full-width top band instead of a narrow left column. The orchestrator's old +//! plan-level "dashboard bespoke scaffold" (that pre-built the two-column root) +//! was removed in 346bcaa8; this is its lightweight node-tree replacement — one +//! pass, no row/slot bin-packing, no placeholder-height estimation. +//! +//! Runs once over each assembled page-root in `cleanup::run_cleanup_passes` — +//! the whole-doc finalize point SHARED by the orchestrator (per-subtask role +//! passes already ran) and the agentic loop (`apply_loop_finalize` ran the +//! whole-doc role passes) — so the moved sections keep their resolved roles. +//! +//! Both broken shapes the orchestrator emits are handled: a VERTICAL root with +//! the sidebar stacked as a full-width band, and a HORIZONTAL root with the +//! sidebar AND every content section crammed into one row. Detection is +//! intentionally strict (see [`detect`]); the adversarial design review flagged +//! restaurant "Menu" pages, "Navy" hero sections, top-nav bars, mobile/tablet +//! roots, and multi-screen files as the false-positives to avoid, so the gate +//! leans on a STRONG sidebar-name token + a real dashboard-content gate (a short +//! numeric header height is rejected, but `fit_content` sidebars are allowed). + +use jian_ops_schema::node::PenNode; +use serde_json::{json, Value}; + +/// Fixed left-sidebar column width. Mirrors the surviving sizing constant in +/// `dashboard_columns.rs` for parity with the removed scaffold. +const SIDEBAR_WIDTH: f64 = 260.0; +/// Desktop floor — excludes phones/tablets. Mirrors +/// `cleanup_desktop_dashboard::DESKTOP_DASHBOARD_MIN_WIDTH`. +const DESKTOP_MIN_WIDTH: f64 = 900.0; +/// A real sidebar is full-height; a 64–96px full-width band is a header. +const MIN_SIDEBAR_HEIGHT: f64 = 200.0; + +/// Restructure a flat-vertical desktop dashboard whose first child is a +/// full-width sidebar into a horizontal `[sidebar(fixed) | content(fill)]` +/// app-shell. Mutates the page-root wrapper in place via the same serialize → +/// mutate `Value` → deserialize round-trip the section passes use. Returns +/// `true` iff it restructured; no-op + `false` when the strict detection +/// criteria do not hold or the round-trip fails (the node is never dropped). +pub(crate) fn reshape_sidebar_to_app_shell(wrapper: &mut PenNode) -> bool { + let Ok(mut v) = serde_json::to_value(&*wrapper) else { + return false; + }; + if !detect(&v) { + return false; + } + if !restructure(&mut v) { + return false; + } + match serde_json::from_value::(v) { + Ok(new_node) => { + *wrapper = new_node; + true + } + // Bad round-trip: leave the wrapper exactly as it was. + Err(_) => false, + } +} + +// ── Value readers (mirror the tolerant accessors the sibling passes use) ── + +/// `width`/`height`/`gap` as f64, tolerant of numeric strings like `"605"`. +/// Returns `None` for keyword sizings (`fill_container` / `fit_content`). +fn num(v: &Value, key: &str) -> Option { + let field = v.get(key)?; + field + .as_f64() + .or_else(|| field.as_str().and_then(|s| s.parse::().ok())) +} + +fn layout_str(v: &Value) -> Option<&str> { + v.get("layout").and_then(Value::as_str) +} + +/// Lowercased `name + id` identity text used for keyword matching. +fn ident_text(v: &Value) -> String { + let name = v.get("name").and_then(Value::as_str).unwrap_or(""); + let id = v.get("id").and_then(Value::as_str).unwrap_or(""); + format!("{name} {id}").to_lowercase() +} + +/// A STRONG left-rail signal. Deliberately excludes the bare `"nav"` / `"menu"` +/// substrings (they match "Navy Hero", restaurant "Menu", "Navigation Guide"), +/// and excludes anything that reads as a top bar. +fn is_sidebar_named(t: &str) -> bool { + let strong = t.contains("sidebar") + || t.contains("side bar") + || t.contains("side nav") + || t.contains("side-nav") + || t.contains("left rail") + || t.contains("left nav") + || t.contains("nav rail") + || t.contains("navigation rail") + || t.contains("nav-rail"); + strong && !is_topbar_named(t) +} + +fn is_topbar_named(t: &str) -> bool { + t.contains("top bar") + || t.contains("topbar") + || t.contains("top nav") + || t.contains("top-nav") + || t.contains("header") + || t.contains("app bar") + || t.contains("appbar") + || t.contains("breadcrumb") +} + +/// True when a section subtree carries a dashboard-grade signal (a table, a +/// metric/KPI/stat block, or a chart). Used as the STRUCTURAL intent gate — far +/// more robust than a root-name keyword (the bug root "Barbershop Client +/// Management" carries no dashboard word, but its sections are "Key Metrics" + +/// "Client Table"). Recurses the whole section. +fn section_has_dashboard_signal(v: &Value) -> bool { + let t = ident_text(v); + if t.contains("table") + || t.contains("metric") + || t.contains("stat") + || t.contains("kpi") + || t.contains("chart") + || t.contains("graph") + || t.contains("analytics") + || t.contains("data grid") + || t.contains("datagrid") + { + return true; + } + v.get("children") + .and_then(Value::as_array) + .is_some_and(|kids| kids.iter().any(section_has_dashboard_signal)) +} + +/// A child that is itself a whole screen/page (multi-screen file guard): named +/// screen/page, or both wide and tall enough to be a standalone artboard. +fn is_screen_like(v: &Value) -> bool { + let t = ident_text(v); + if t.contains("screen") || t.contains("artboard") || t.contains(" page") || t.ends_with("page") + { + return true; + } + matches!(num(v, "width"), Some(w) if w >= DESKTOP_MIN_WIDTH) + && matches!(num(v, "height"), Some(h) if h >= 700.0) +} + +/// Vertical / none / absent layout (a column or absolute container — NOT a row). +fn is_column_layout(v: &Value) -> bool { + !matches!(layout_str(v), Some("horizontal")) +} + +// ── Detection ── + +/// All criteria must hold (see module doc for the false-positives each guards). +fn detect(v: &Value) -> bool { + // 1. Desktop width (numeric ≥ 900 — excludes mobile/tablet; a + // keyword-width root is treated as out-of-scope, not reshaped). + let Some(root_w) = num(v, "width") else { + return false; + }; + if root_w < DESKTOP_MIN_WIDTH { + return false; + } + // 2. Sidebar + at least two content sections. A *vertical* root has the + // sidebar stacked as a full-width band; a *horizontal* root has the + // sidebar AND every content section crammed into one row (the orchestrator + // assembles both shapes). Both are handled — only the already-correct + // `[sidebar | content]` 2-child app-shell is left alone, which the ≥3 + // floor + the `Main Content` idempotency guard below cover. + let Some(kids) = v.get("children").and_then(Value::as_array) else { + return false; + }; + if kids.len() < 3 { + return false; + } + // Idempotency: never re-wrap a root that already carries our content column. + if kids.iter().any(|c| ident_text(c).contains("main content")) { + return false; + } + // 9. Multi-screen file: never app-shell a wrapper of standalone screens. + if kids.iter().filter(|c| is_screen_like(c)).count() >= 2 { + return false; + } + let first = &kids[0]; + // 4. First child reads as a sidebar (strong tokens, not a top bar). + if !is_sidebar_named(&ident_text(first)) { + return false; + } + // 5. First child is a column, not a horizontal nav row. + if !is_column_layout(first) { + return false; + } + // 6. First child is NOT a short top strip. A header/top-bar band is a small + // EXPLICIT numeric height (64–96px); skip those. A `fit_content` / + // `fill_container` (non-numeric) height is allowed — real sidebars are + // frequently authored that way — because the STRONG sidebar-name token + // (criterion 4) + the dashboard-content gate (criterion 8) already carry + // the specificity; height is only used to reject short numeric headers. + if matches!(num(first, "height"), Some(h) if h < MIN_SIDEBAR_HEIGHT) { + return false; + } + // 7. First child spans ~full width (the actual bug signature). A child + // already narrower than half the root is an existing left column → skip. + let first_is_full_width = match first.get("width") { + Some(Value::String(s)) if s == "fill_container" => true, + None => true, + _ => matches!(num(first, "width"), Some(w) if w >= 0.9 * root_w), + }; + if !first_is_full_width { + return false; + } + if matches!(num(first, "width"), Some(w) if w < 0.5 * root_w) { + return false; + } + // 8. STRUCTURAL dashboard gate — ≥2 content sections carry a table / metric + // / chart signal. Kills restaurant-menu / landing / portfolio / settings + // false-positives without depending on the (too-narrow) root name. + let dashboard_sections = kids[1..] + .iter() + .filter(|s| section_has_dashboard_signal(s)) + .count(); + dashboard_sections >= 2 +} + +// ── Restructure ── + +/// Recursively retarget any descendant whose width was sized to (about) the OLD +/// full root width down to `fill_container`, so full-width sections/dividers +/// fill their new narrower column instead of overflowing it (taffy never +/// shrinks the cross axis). Targeted: only ~full-width Number widths change. +fn fill_full_width_descendants(v: &mut Value, old_root_w: f64) { + if let Some(w) = num(v, "width") { + if w >= 0.9 * old_root_w { + if let Some(obj) = v.as_object_mut() { + obj.insert("width".into(), json!("fill_container")); + } + } + } + if let Some(kids) = v.get_mut("children").and_then(Value::as_array_mut) { + for c in kids.iter_mut() { + fill_full_width_descendants(c, old_root_w); + } + } +} + +/// Detection already passed: split the wrapper's children into `[sidebar | +/// content-column]` and flip the wrapper to a horizontal app-shell. Returns +/// `false` only if the tree shape changed under us (defensive). +fn restructure(v: &mut Value) -> bool { + let root_w = match num(v, "width") { + Some(w) => w, + None => return false, + }; + let content_gap = num(v, "gap").filter(|g| *g > 0.0).unwrap_or(24.0); + let content_id = format!( + "{}-content", + v.get("id").and_then(Value::as_str).unwrap_or("root") + ); + + let Some(obj) = v.as_object_mut() else { + return false; + }; + let Some(kids) = obj.get_mut("children").and_then(Value::as_array_mut) else { + return false; + }; + if kids.len() < 3 { + return false; + } + let mut sidebar = kids.remove(0); + let mut content_sections: Vec = std::mem::take(kids); + + // Sidebar → fixed narrow vertical column that stretches to the row height. + // `height: fill_container` is the LOAD-BEARING cross-axis stretch (jian maps + // `alignItems: stretch` to FlexStart, so the wrapper's alignItems alone + // would NOT stretch it). Clip + width-retarget keep its old full-width + // descendants from bleeding over the content column. + if let Some(s) = sidebar.as_object_mut() { + s.insert("width".into(), json!(SIDEBAR_WIDTH)); + s.insert("height".into(), json!("fill_container")); + if s.get("layout").and_then(Value::as_str) != Some("vertical") { + s.insert("layout".into(), json!("vertical")); + } + s.insert("clipContent".into(), json!(true)); + } + fill_full_width_descendants(&mut sidebar, root_w); + + // Content sections → fill the new column instead of the old full root width. + for section in content_sections.iter_mut() { + fill_full_width_descendants(section, root_w); + } + + let content = json!({ + "type": "frame", + "id": content_id, + "name": "Main Content", + "width": "fill_container", + "height": "fit_content", + "layout": "vertical", + "gap": content_gap, + "children": content_sections, + }); + + obj.insert("children".into(), json!([sidebar, content])); + obj.insert("layout".into(), json!("horizontal")); + obj.insert("gap".into(), json!(0)); + obj.insert("alignItems".into(), json!("stretch")); + // The old height was the SUM of the vertical stack (incl. the sidebar); the + // horizontal shell only needs the taller column. Track content instead of + // leaving ~600px of dead canvas below. + obj.insert("height".into(), json!("fit_content")); + true +} + +#[cfg(test)] +#[path = "app_shell_tests.rs"] +mod tests; diff --git a/crates/op-orchestrator/src/app_shell_tests.rs b/crates/op-orchestrator/src/app_shell_tests.rs new file mode 100644 index 000000000..5783914c5 --- /dev/null +++ b/crates/op-orchestrator/src/app_shell_tests.rs @@ -0,0 +1,337 @@ +//! Tests for the app-shell restructure pass. The positive case is the reported +//! glm barbershop dashboard; the negatives are the false-positives the +//! adversarial design review flagged (top-nav, short header, mobile, already +//! horizontal, already-narrow sidebar, too-few sections, restaurant "Menu", +//! "Navy" hero, `fit_content` nav, no dashboard content, multi-screen file). + +use super::*; + +fn node(v: Value) -> PenNode { + serde_json::from_value::(v).expect("valid PenNode fixture") +} + +fn val(n: &PenNode) -> Value { + serde_json::to_value(n).expect("serialize PenNode") +} + +/// A leaf section frame with a name + sizing (children optional). +fn section(name: &str, width: Value, height: Value) -> Value { + json!({ + "type": "frame", "id": name.replace(' ', "-"), "name": name, + "width": width, "height": height, "layout": "vertical", "children": [] + }) +} + +/// The reported bug shape: vertical 1200-wide root, full-width sidebar first. +fn bug_wrapper() -> PenNode { + node(json!({ + "type": "frame", "id": "root", "name": "Barbershop Client Management", + "width": 1200, "height": 1775, "layout": "vertical", "gap": 32, + "children": [ + { "type": "frame", "id": "n2", "name": "Sidebar Navigation", + "width": 1200, "height": 605, "layout": "vertical", + "children": [ section("Logo", json!(1152), json!(27)) ] }, + section("Top Header", json!(1200), json!(94)), + section("Key Metrics", json!(1200), json!(117)), + section("Client Table Section", json!(1200), json!(488)), + section("Upcoming Appointments", json!(1200), json!(391)), + ] + })) +} + +#[test] +fn positive_full_width_sidebar_dashboard_restructured() { + let mut w = bug_wrapper(); + assert!( + reshape_sidebar_to_app_shell(&mut w), + "bug shape must restructure" + ); + let v = val(&w); + assert_eq!( + layout_str(&v), + Some("horizontal"), + "root flips to horizontal" + ); + let kids = v["children"].as_array().unwrap(); + assert_eq!(kids.len(), 2, "[sidebar | content]"); + + let sidebar = &kids[0]; + assert!(ident_text(sidebar).contains("sidebar")); + assert_eq!( + num(sidebar, "width"), + Some(SIDEBAR_WIDTH), + "sidebar pinned to 260" + ); + assert_eq!( + sidebar["height"], + json!("fill_container"), + "sidebar stretches" + ); + assert_eq!(sidebar["clipContent"], json!(true)); + + let content = &kids[1]; + assert_eq!(content["name"], json!("Main Content")); + assert_eq!(content["width"], json!("fill_container")); + assert_eq!(layout_str(content), Some("vertical")); + let sections: Vec<&str> = content["children"] + .as_array() + .unwrap() + .iter() + .map(|s| s["name"].as_str().unwrap()) + .collect(); + assert_eq!( + sections, + [ + "Top Header", + "Key Metrics", + "Client Table Section", + "Upcoming Appointments" + ], + "sections moved into the content column IN ORDER" + ); +} + +#[test] +fn positive_full_width_sections_retargeted_to_fill() { + let mut w = bug_wrapper(); + reshape_sidebar_to_app_shell(&mut w); + let v = val(&w); + let content = &v["children"][1]; + // The 1200-wide sections must become fill_container so they don't overflow + // the ~940 content column. + for s in content["children"].as_array().unwrap() { + assert_eq!( + s["width"], + json!("fill_container"), + "section {} retargeted", + s["name"] + ); + } + // The sidebar's 1152-wide logo child likewise fills the 260 column. + let logo = &v["children"][0]["children"][0]; + assert_eq!(logo["width"], json!("fill_container")); +} + +fn assert_untouched(mut w: PenNode, why: &str) { + let before = val(&w); + assert!( + !reshape_sidebar_to_app_shell(&mut w), + "must NOT restructure: {why}" + ); + assert_eq!(val(&w), before, "node unchanged: {why}"); +} + +#[test] +fn negative_top_nav_untouched() { + assert_untouched( + node(json!({ + "type": "frame", "id": "r", "name": "Dashboard", "width": 1200, "layout": "vertical", + "children": [ + { "type": "frame", "id": "tn", "name": "Top Navigation", "width": 1200, + "height": 72, "layout": "horizontal", "children": [] }, + section("Key Metrics", json!(1200), json!(117)), + section("Client Table", json!(1200), json!(400)), + ] + })), + "top navigation (topbar keyword + horizontal + short)", + ); +} + +#[test] +fn negative_header_short_strip_untouched() { + assert_untouched( + node(json!({ + "type": "frame", "id": "r", "name": "Dashboard", "width": 1200, "layout": "vertical", + "children": [ + section("Header", json!(1200), json!(80)), + section("Metrics Grid", json!(1200), json!(117)), + section("Data Table", json!(1200), json!(400)), + ] + })), + "short full-width header, not a sidebar", + ); +} + +#[test] +fn negative_mobile_untouched() { + assert_untouched( + node(json!({ + "type": "frame", "id": "r", "name": "App", "width": 390, "layout": "vertical", + "children": [ + section("Sidebar Navigation", json!(390), json!(605)), + section("Key Metrics", json!(390), json!(117)), + section("Table", json!(390), json!(400)), + ] + })), + "mobile width < 900", + ); +} + +#[test] +fn negative_already_horizontal_untouched() { + assert_untouched( + node(json!({ + "type": "frame", "id": "r", "name": "Dashboard", "width": 1200, "layout": "horizontal", + "children": [ + section("Sidebar Navigation", json!(260), json!(605)), + section("Main Content", json!("fill_container"), json!("fit_content")), + ] + })), + "already app-shelled (horizontal root)", + ); +} + +#[test] +fn negative_already_narrow_sidebar_untouched() { + assert_untouched( + node(json!({ + "type": "frame", "id": "r", "name": "Dashboard", "width": 1200, "layout": "vertical", + "children": [ + section("Sidebar Navigation", json!(240), json!(605)), + section("Key Metrics", json!(1200), json!(117)), + section("Client Table", json!(1200), json!(400)), + ] + })), + "sidebar already a narrow left column (240 < 0.5*1200)", + ); +} + +#[test] +fn negative_fewer_than_two_content_sections() { + assert_untouched( + node(json!({ + "type": "frame", "id": "r", "name": "Dashboard", "width": 1200, "layout": "vertical", + "children": [ + section("Sidebar Navigation", json!(1200), json!(605)), + section("Client Table", json!(1200), json!(400)), + ] + })), + "only one content section (len < 3)", + ); +} + +#[test] +fn negative_restaurant_menu_untouched() { + assert_untouched( + node(json!({ + "type": "frame", "id": "r", "name": "Restaurant", "width": 1280, "layout": "vertical", + "children": [ + section("Menu", json!("fill_container"), json!(900)), + section("About", json!(1280), json!(300)), + section("Hours", json!(1280), json!(200)), + section("Footer", json!(1280), json!(120)), + ] + })), + "'Menu' is not a strong sidebar token (no sidebar/rail)", + ); +} + +#[test] +fn negative_navy_hero_untouched() { + assert_untouched( + node(json!({ + "type": "frame", "id": "r", "name": "Landing", "width": 1440, "layout": "vertical", + "children": [ + section("Navy Hero", json!("fill_container"), json!(640)), + section("Features", json!(1440), json!(400)), + section("Pricing", json!(1440), json!(500)), + ] + })), + "'Navy Hero' contains 'nav' substring but is not a sidebar", + ); +} + +#[test] +fn negative_weak_nav_name_untouched() { + // A WEAK nav name ("Navigation", no "sidebar"/rail token) is the real + // false-positive risk now that fit_content heights are allowed — it must be + // excluded by the strong-token name gate (criterion 4), not the height. + assert_untouched( + node(json!({ + "type": "frame", "id": "r", "name": "Dashboard", "width": 1200, "layout": "vertical", + "children": [ + { "type": "frame", "id": "nav", "name": "Navigation", + "width": "fill_container", "height": "fit_content", "layout": "vertical", + "children": [] }, + section("Key Metrics", json!(1200), json!(117)), + section("Client Table", json!(1200), json!(400)), + ] + })), + "'Navigation' is not a strong sidebar token (criterion 4)", + ); +} + +#[test] +fn positive_horizontal_root_fit_content_sidebar_restructured() { + // The orchestrator also emits the bug as a HORIZONTAL root with the sidebar + // AND every section crammed into one row, the sidebar sized fill_container / + // fit_content (the op-smoke barbershop output). This must restructure too. + let mut w = node(json!({ + "type": "frame", "id": "r", "name": "Barbershop Dashboard", + "width": 1200, "layout": "horizontal", "gap": 24, + "children": [ + { "type": "frame", "id": "sb", "name": "Left Sidebar Navigation", + "width": "fill_container", "height": "fit_content", "layout": "vertical", + "children": [] }, + section("Top Header Bar", json!("fill_container"), json!("fit_content")), + section("Key Metrics Row", json!("fill_container"), json!("fit_content")), + section("Recent Clients Table", json!("fill_container"), json!("fit_content")), + ] + })); + assert!( + reshape_sidebar_to_app_shell(&mut w), + "horizontal-root fit_content sidebar must restructure" + ); + let v = val(&w); + assert_eq!(layout_str(&v), Some("horizontal")); + let kids = v["children"].as_array().unwrap(); + assert_eq!(kids.len(), 2, "[sidebar | Main Content]"); + assert_eq!( + num(&kids[0], "width"), + Some(SIDEBAR_WIDTH), + "sidebar narrowed to 260" + ); + assert_eq!(kids[1]["name"], json!("Main Content")); + let inner: Vec<&str> = kids[1]["children"] + .as_array() + .unwrap() + .iter() + .map(|s| s["name"].as_str().unwrap()) + .collect(); + assert_eq!( + inner, + ["Top Header Bar", "Key Metrics Row", "Recent Clients Table"], + "the row sections moved into the content column" + ); +} + +#[test] +fn negative_no_dashboard_content_untouched() { + assert_untouched( + node(json!({ + "type": "frame", "id": "r", "name": "Marketing Site", "width": 1280, "layout": "vertical", + "children": [ + section("Sidebar Navigation", json!(1280), json!(605)), + section("About Us", json!(1280), json!(300)), + section("Our Team", json!(1280), json!(300)), + section("Contact", json!(1280), json!(200)), + ] + })), + "no table/metric/chart sections (structural dashboard gate)", + ); +} + +#[test] +fn negative_multiscreen_wrapper_untouched() { + assert_untouched( + node(json!({ + "type": "frame", "id": "r", "name": "Flows", "width": 1440, "layout": "vertical", + "children": [ + section("Navigation Rail Screen", json!(1440), json!(900)), + section("Detail Screen", json!(1440), json!(900)), + section("Settings Screen", json!(1440), json!(900)), + ] + })), + "multiple standalone screens, not dashboard sections", + ); +} diff --git a/crates/op-orchestrator/src/cleanup.rs b/crates/op-orchestrator/src/cleanup.rs index f21ad12e8..7b85a146e 100644 --- a/crates/op-orchestrator/src/cleanup.rs +++ b/crates/op-orchestrator/src/cleanup.rs @@ -673,6 +673,14 @@ pub fn finalize_design(sink: &mut dyn DocSink, plan: &OrchestratorPlan, root_ids pub fn run_cleanup_passes(sink: &mut dyn DocSink, plan: &OrchestratorPlan, root_ids: &[&str]) { for root_id in root_ids { + // FIRST: turn a flat-vertical desktop dashboard with a full-width + // sidebar into a horizontal [sidebar | content] app-shell, so the + // remaining cleanup passes (esp. `adjust_root_height_to_content`) see + // the corrected shape. This is the shared whole-doc finalize point for + // BOTH the orchestrator (per-subtask role passes already ran) and the + // agentic loop (whole-doc role passes ran in `apply_loop_finalize`), so + // the moved sections keep their resolved roles. + restructure_app_shell_root(sink, root_id); remove_duplicate_status_bars(sink, root_id); repair_light_mobile_nav_surfaces(sink, root_id); repair_mobile_content_sections(sink, root_id); @@ -685,6 +693,26 @@ pub fn run_cleanup_passes(sink: &mut dyn DocSink, plan: &OrchestratorPlan, root_ } } +/// Restructure the page-root in place when it is a flat-vertical desktop +/// dashboard whose first child is a full-width sidebar (see +/// [`crate::app_shell::reshape_sidebar_to_app_shell`] for the strict gate). The +/// whole subtree is swapped via `ReplaceSubtree` (the new root carries the +/// reorganized children, so `drop_children: true` loses nothing). +fn restructure_app_shell_root(sink: &mut dyn DocSink, root_id: &str) { + let Some(root) = find_root(sink.state(), root_id) else { + return; + }; + let mut new_root = root.clone(); + if crate::app_shell::reshape_sidebar_to_app_shell(&mut new_root) { + sink.apply(EditorCommand::ReplaceSubtree { + node_id: NodeId::new(root_id.to_string()), + node: Box::new(new_root), + drop_children: true, + page_id: None, + }); + } +} + /// Strip the REDUNDANT border off a filled, shadowed container. When a /// frame / group / rectangle has a fill AND a drop shadow AND a stroke, /// the stroke is a "莫名其妙" hairline — the shadow already separates the diff --git a/crates/op-orchestrator/src/lib.rs b/crates/op-orchestrator/src/lib.rs index b1e7c5340..f374f01c5 100644 --- a/crates/op-orchestrator/src/lib.rs +++ b/crates/op-orchestrator/src/lib.rs @@ -41,6 +41,7 @@ pub(crate) mod validation_fixes_b3; mod variable_binding; pub mod variables; +pub mod app_shell; pub mod append; pub mod cleanup; pub(crate) mod cleanup_layout; diff --git a/crates/op-orchestrator/src/loop_finalize.rs b/crates/op-orchestrator/src/loop_finalize.rs index 958d816d1..0dc899034 100644 --- a/crates/op-orchestrator/src/loop_finalize.rs +++ b/crates/op-orchestrator/src/loop_finalize.rs @@ -192,6 +192,12 @@ pub fn apply_loop_finalize(state: &mut EditorState) { crate::role_post_pass::enforce_surface_color_discipline(forest); } + // The app-shell restructure (flat-vertical sidebar dashboard → horizontal + // [sidebar | content]) runs inside `cleanup::finalize_design` below, the + // whole-doc finalize point SHARED with the orchestrator path — so both the + // agentic loop and the orchestrator get it exactly once, after roles are + // resolved. See `cleanup::run_cleanup_passes`. + // -- Per-root cleanup (`cleanup::finalize_design`) over every top-level // root, via a borrowed-state DocSink. The cleanup passes are whole-root // (they take the page-root id and recurse), so they always run over the diff --git a/crates/op-orchestrator/src/loop_finalize_tests.rs b/crates/op-orchestrator/src/loop_finalize_tests.rs index 3d2ab7a8a..ef763340d 100644 --- a/crates/op-orchestrator/src/loop_finalize_tests.rs +++ b/crates/op-orchestrator/src/loop_finalize_tests.rs @@ -177,3 +177,79 @@ fn loop_finalize_on_empty_doc_is_noop() { apply_loop_finalize(&mut state); assert!(state.active_children().is_empty()); } + +/// The reported glm bug shape (a single page-root wrapper whose first child is a +/// full-width sidebar) is restructured into a horizontal `[sidebar | Main +/// Content]` app-shell by `apply_loop_finalize`, AND the restructure survives +/// the subsequent Class-A passes — in particular the sidebar is NOT re-widened +/// by `fix_horizontal_overflow` (the guard added for this). +#[test] +fn loop_finalize_restructures_sidebar_dashboard() { + let mut state = state_with_forest(json!([ + { + "type": "frame", "id": "root", "name": "Barbershop Client Management", + "width": 1200, "height": 1775, "layout": "vertical", "gap": 32, + "children": [ + { "type": "frame", "id": "sb", "name": "Sidebar Navigation", + "width": 1200, "height": 605, "layout": "vertical", + "children": [ + { "type": "frame", "id": "logo", "name": "Logo", + "width": 1152, "height": 27, "layout": "vertical", "children": [] } + ] }, + { "type": "frame", "id": "hd", "name": "Top Header", + "width": 1200, "height": 94, "layout": "vertical", "children": [] }, + { "type": "frame", "id": "km", "name": "Key Metrics", + "width": 1200, "height": 117, "layout": "horizontal", "children": [] }, + { "type": "frame", "id": "ct", "name": "Client Table Section", + "width": 1200, "height": 488, "layout": "vertical", "children": [] }, + { "type": "frame", "id": "ua", "name": "Upcoming Appointments", + "width": 1200, "height": 391, "layout": "vertical", "children": [] } + ] + } + ])); + apply_loop_finalize(&mut state); + + let root = &state.active_children()[0]; + let rv = serde_json::to_value(root).unwrap(); + assert_eq!( + rv.get("layout").and_then(|l| l.as_str()), + Some("horizontal"), + "root restructured to a horizontal app-shell" + ); + assert_eq!( + rv["children"].as_array().unwrap().len(), + 2, + "[sidebar | Main Content]" + ); + + // Sidebar stayed a narrow left column through the whole sequence (the + // fix_horizontal_overflow guard must NOT re-widen this vertical frame). + let sidebar = find_by_name(state.active_children(), "Sidebar Navigation").unwrap(); + let sv = serde_json::to_value(sidebar).unwrap(); + assert!( + sv["width"].as_f64().is_some_and(|w| w <= 300.0), + "sidebar must stay narrow, got {:?}", + sv["width"] + ); + + // The data sections moved into the new content column, in order. + let content = + find_by_name(state.active_children(), "Main Content").expect("content column created"); + let cv = serde_json::to_value(content).unwrap(); + let names: Vec = cv["children"] + .as_array() + .unwrap() + .iter() + .map(|s| s["name"].as_str().unwrap_or("").to_string()) + .collect(); + assert_eq!( + names, + [ + "Top Header", + "Key Metrics", + "Client Table Section", + "Upcoming Appointments" + ], + "data sections nested under Main Content in order" + ); +} diff --git a/crates/op-orchestrator/src/role_layout_post_pass.rs b/crates/op-orchestrator/src/role_layout_post_pass.rs index 410a4f796..3e896c550 100644 --- a/crates/op-orchestrator/src/role_layout_post_pass.rs +++ b/crates/op-orchestrator/src/role_layout_post_pass.rs @@ -40,6 +40,20 @@ fn padding_lr(node: &Value) -> (f64, f64) { } pub(crate) fn fix_horizontal_overflow(node: &mut Value, canvas_width: f64) { + // Summing child widths as a ROW is only valid for a row layout. A + // `vertical` column stacks its children (widths don't sum — the max + // applies) and a `none` container positions them absolutely; running the + // row-sum on those wrongly widens them — e.g. a narrow vertical sidebar of N + // `fill_container` items (each counted as 80px) sums to ~80*N and gets + // re-widened to a fraction of the canvas, undoing the app-shell pass. A + // frame with no explicit `layout` defaults to a row, so only the explicit + // column / absolute cases are skipped. + if matches!( + node.get("layout").and_then(Value::as_str), + Some("vertical") | Some("none") + ) { + return; + } let parent_w = size_number(node, "width"); if parent_w <= 0.0 { return;