diff --git a/Cargo.lock b/Cargo.lock index 9e12c9f11..829a1ce0f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2627,6 +2627,7 @@ dependencies = [ "op-acp", "op-ai", "op-ai-skills", + "op-design-lint", "op-editor-core", "op-editor-ui", "op-figma", diff --git a/crates/op-design-lint/src/lib.rs b/crates/op-design-lint/src/lib.rs index f423f5fcd..7e556d66a 100644 --- a/crates/op-design-lint/src/lib.rs +++ b/crates/op-design-lint/src/lib.rs @@ -10,6 +10,7 @@ pub mod fixes; pub mod issue; pub mod node_mut; pub mod node_util; +pub mod plan; pub use color::{color_contrast, parse_hex_color, relative_luminance, Rgb}; pub use detectors::{ @@ -21,3 +22,4 @@ pub use detectors::{ }; pub use fixes::{apply_fixes, detect_and_fix}; pub use issue::{FixProperty, FixReport, Issue, IssueCategory, IssueSeverity}; +pub use plan::{detect_and_plan, PlannedAction, PlannedFix}; diff --git a/crates/op-design-lint/src/plan.rs b/crates/op-design-lint/src/plan.rs new file mode 100644 index 000000000..762c16f63 --- /dev/null +++ b/crates/op-design-lint/src/plan.rs @@ -0,0 +1,529 @@ +//! Editor-agnostic `PlannedFix` API — a plan for what `apply_fixes` would do, +//! expressed as a data type rather than a direct mutation. +//! +//! `detect_and_plan` runs the same filter logic that `apply_fixes` uses +//! (skip `Info`-severity issues, skip the no-op `Fill` property, skip +//! `Remove` on `status-bar`-role nodes), then emits a `PlannedFix` per +//! issue that would have been applied. The final document produced by +//! applying every `PlannedFix` is byte-identical to the final document +//! produced by `detect_and_fix` — this invariant is verified by the +//! equivalence tests in this module. +//! +//! Host adapters (e.g. `op-host-desktop::pre_validator`) translate each +//! `PlannedFix` to one or more `EditorCommand`s and apply them via +//! `DocSink::apply`. This design keeps `op-design-lint` editor-agnostic +//! and isolates the `FixProperty → EditorCommand` translation table to a +//! single host module. + +use crate::issue::{FixProperty, IssueCategory, IssueSeverity}; +use crate::node_util; +use jian_ops_schema::PenDocument; + +/// An editor-agnostic description of a fix that `detect_and_fix` would +/// have applied to the document. The `PlannedAction` captures the full +/// mutation in a form that a host adapter can translate to `EditorCommand`s. +/// +/// Structural equivalence invariant: applying all `PlannedFix`es from +/// `detect_and_plan(doc)` using the mutation primitives in `crate::node_mut` +/// produces a document structurally identical to calling `detect_and_fix(doc)`. +#[derive(Debug, Clone, PartialEq)] +pub struct PlannedFix { + /// The node the fix targets. + pub node_id: String, + /// Which detector category produced this fix — used for per-category + /// counts in `PreValidationResult`. + pub category: IssueCategory, + /// The concrete mutation to apply. + pub action: PlannedAction, +} + +/// The concrete mutation encoded by a `PlannedFix`. +/// +/// Variants cover every mutation path in `fixes::apply_fixes` today: +/// +/// | `FixProperty` | `PlannedAction` variant | Notes | +/// |-----------------|--------------------------------|-------------------------------| +/// | `Remove` | `Remove` | detach node from parent | +/// | `Height` | `SetHeightFitContent` | only `"fit_content"` is valid | +/// | `Rotation` | `SetRotation(f64)` | degrees | +/// | `CornerRadius` | `SetCornerRadius(f32)` | uniform radius | +/// | `FontSize` | `SetFontSize(f32)` | doc-px | +/// | `Effects` | `ClearEffects` | set effects field to None | +/// | `Padding` | `SetPadding(Value)` | uniform or [L,T,R,B] array | +/// | `Stroke` | `SetStroke(Value)` | full PenStroke JSON object | +/// | `Fill` | *(filtered out — no-op)* | `apply_fixes` returns `false` | +#[derive(Debug, Clone, PartialEq)] +pub enum PlannedAction { + /// Remove the node from its parent (mirrors `fixes::apply_remove`). + Remove, + /// Set the node's height sizing to `fit_content` (text-explicit-height fix). + SetHeightFitContent, + /// Set the node's rotation to the given degrees. + SetRotation(f64), + /// Set the node's corner-radius to a uniform value. + SetCornerRadius(f32), + /// Set the node's font size. + SetFontSize(f32), + /// Clear the node's effects list (set to `None`). + ClearEffects, + /// Set the node's padding. The `Value` is either a single number + /// (uniform) or a 4-element array `[L, T, R, B]`. + SetPadding(serde_json::Value), + /// Set the node's stroke to the given `PenStroke`-compatible JSON + /// object (e.g. `{"thickness":1,"fill":[{"type":"solid","color":"#E2E8F0"}]}`). + SetStroke(serde_json::Value), +} + +/// Detect issues over the active page and return a plan of fixes — +/// the set that `apply_fixes` would have ACTUALLY applied (Info-severity +/// issues and `Fill` no-ops are excluded from the plan, matching the +/// filter in `apply_fixes`). +/// +/// The returned list is in the same order as `apply_fixes` would have +/// processed the issues: one `PlannedFix` per issue that passes the +/// filter AND whose suggested value is usable (e.g. `Height` only when +/// `suggested_value == "fit_content"`). +/// +/// This function reads the doc but does NOT mutate it; it is safe to +/// call on the same doc before handing it to the adapter. +pub fn detect_and_plan(doc: &PenDocument) -> Vec { + // Locate the active-page root. Mirror the logic in `fixes::detect_and_fix`. + let root = match active_page_root(doc) { + Some(r) => r, + None => return Vec::new(), + }; + let issues = crate::detectors::detect_all(root, doc); + + let mut plan = Vec::new(); + + for issue in &issues { + // Mirror `apply_fixes`: skip Info-severity. + if issue.severity == IssueSeverity::Info { + continue; + } + + if issue.property == FixProperty::Remove { + // Mirror `apply_remove`: skip status-bar-role nodes. + // Walk the page roots read-only to check the role before deciding. + let is_status_bar = active_page_nodes(doc) + .iter() + .find_map(|root_node| find_node(root_node, &issue.node_id)) + .map(|n| node_util::role(n) == Some("status-bar")) + .unwrap_or(false); + if is_status_bar { + continue; + } + plan.push(PlannedFix { + node_id: issue.node_id.clone(), + category: issue.category, + action: PlannedAction::Remove, + }); + continue; + } + + // Translate `(property, suggested_value)` to a `PlannedAction`, + // mirroring the `set_property` dispatch in `fixes.rs`. Skip combinations + // that `set_property` returns `false` for (Fill, non-fit_content Height, etc.). + let action = match issue.property { + FixProperty::Height => { + if issue.suggested_value.as_str() == Some("fit_content") { + PlannedAction::SetHeightFitContent + } else { + continue; // set_property returns false; skip + } + } + FixProperty::Rotation => { + let v = match issue.suggested_value.as_f64() { + Some(v) => v, + None => continue, + }; + PlannedAction::SetRotation(v) + } + FixProperty::CornerRadius => { + let v = match issue.suggested_value.as_f64() { + Some(v) => v as f32, + None => continue, + }; + PlannedAction::SetCornerRadius(v) + } + FixProperty::FontSize => { + let v = match issue.suggested_value.as_f64() { + Some(v) => v as f32, + None => continue, + }; + PlannedAction::SetFontSize(v) + } + FixProperty::Effects => PlannedAction::ClearEffects, + FixProperty::Padding => PlannedAction::SetPadding(issue.suggested_value.clone()), + FixProperty::Stroke => PlannedAction::SetStroke(issue.suggested_value.clone()), + // Fill is a documented no-op in apply_fixes — skip. + FixProperty::Fill => continue, + // Remove is handled above; this arm is unreachable. + FixProperty::Remove => continue, + }; + + plan.push(PlannedFix { + node_id: issue.node_id.clone(), + category: issue.category, + action, + }); + } + + plan +} + +// ── Private helpers (mirror fixes.rs helpers) ──────────────────────────────── + +fn active_page_nodes(doc: &PenDocument) -> &[jian_ops_schema::node::PenNode] { + match doc.pages.as_ref().and_then(|pages| pages.first()) { + Some(page) => &page.children, + None => &doc.children, + } +} + +fn active_page_root(doc: &PenDocument) -> Option<&jian_ops_schema::node::PenNode> { + active_page_nodes(doc).first() +} + +fn find_node<'a>( + root: &'a jian_ops_schema::node::PenNode, + node_id: &str, +) -> Option<&'a jian_ops_schema::node::PenNode> { + if node_util::node_id(root) == node_id { + return Some(root); + } + node_util::children(root) + .iter() + .find_map(|child| find_node(child, node_id)) +} + +// ── Equivalence tests ──────────────────────────────────────────────────────── + +#[cfg(test)] +mod tests { + use super::*; + use crate::fixes::detect_and_fix; + use crate::node_mut; + use crate::node_util; + use jian_ops_schema::node::PenNode; + use jian_ops_schema::PenDocument; + use serde_json::Value; + + fn doc(value: serde_json::Value) -> PenDocument { + serde_json::from_value(value).expect("fixture must deserialize as PenDocument") + } + + /// Apply a single `PlannedAction` to the target node in the document — + /// mirrors what `apply_fixes` does via `set_property` / `apply_remove`, + /// reusing the same `node_mut` helpers so the equivalence test is + /// meaningful (not just checking the same code path twice). + fn apply_planned_fix_to_doc(doc: &mut PenDocument, fix: &PlannedFix) -> bool { + if fix.action == PlannedAction::Remove { + // Mirror `apply_remove` — detach from whichever root contains it. + for root in active_page_roots_mut(doc) { + if node_mut::detach_child(root, &fix.node_id) { + return true; + } + } + return false; + } + + // Find the node mutably and apply the action. + for root in active_page_roots_mut(doc) { + if let Some(node) = find_node_mut(root, &fix.node_id) { + return apply_action_to_node(node, &fix.action); + } + } + false + } + + fn apply_action_to_node(node: &mut PenNode, action: &PlannedAction) -> bool { + match action { + PlannedAction::SetHeightFitContent => node_mut::set_text_height_fit_content(node), + PlannedAction::SetRotation(v) => node_mut::set_rotation(node, *v), + PlannedAction::SetCornerRadius(v) => node_mut::set_corner_radius(node, *v), + PlannedAction::SetFontSize(v) => node_mut::set_font_size(node, *v), + PlannedAction::ClearEffects => node_mut::clear_effects(node), + PlannedAction::SetPadding(v) => apply_padding(node, v), + PlannedAction::SetStroke(v) => node_mut::set_stroke_from_json(node, v), + PlannedAction::Remove => unreachable!("Remove handled in apply_planned_fix_to_doc"), + } + } + + fn apply_padding(node: &mut PenNode, suggested: &Value) -> bool { + if let Some(n) = suggested.as_f64() { + return node_mut::set_padding_uniform(node, n as f32); + } + if let Some(arr) = suggested.as_array() { + if arr.len() == 4 { + let v: Vec = arr + .iter() + .filter_map(|x| x.as_f64().map(|f| f as f32)) + .collect(); + if v.len() == 4 { + return node_mut::set_padding_ltrb(node, [v[0], v[1], v[2], v[3]]); + } + } + } + false + } + + fn active_page_roots_mut(doc: &mut PenDocument) -> &mut [jian_ops_schema::node::PenNode] { + let has_page = doc + .pages + .as_ref() + .map(|pages| !pages.is_empty()) + .unwrap_or(false); + if has_page { + doc.pages.as_mut().unwrap()[0].children.as_mut_slice() + } else { + doc.children.as_mut_slice() + } + } + + fn find_node_mut<'a>(root: &'a mut PenNode, node_id: &str) -> Option<&'a mut PenNode> { + if node_util::node_id(root) == node_id { + return Some(root); + } + match root { + PenNode::Frame(n) => n.children.as_mut(), + PenNode::Group(n) => n.children.as_mut(), + PenNode::Rectangle(n) => n.children.as_mut(), + _ => None, + }? + .iter_mut() + .find_map(|child| find_node_mut(child, node_id)) + } + + /// Load the `invisible-container` fixture and assert that `detect_and_plan` + /// + apply-each-fix produces a document identical to `detect_and_fix`. + /// + /// This fixture exercises `FixProperty::Stroke` (the `Warning`-severity + /// `light-on-light` node) and the `Info`-severity skip (the `dark-on-dark` + /// node must NOT be in the plan). + #[test] + fn equivalence_invisible_container() { + let raw = include_str!("../tests/fixtures/docs/invisible-container.json"); + let doc_a: PenDocument = serde_json::from_str(raw).expect("parse"); + let doc_b: PenDocument = serde_json::from_str(raw).expect("parse"); + + // Clone A: detect_and_fix (the reference path). + let mut clone_a = doc_a; + let report = detect_and_fix(&mut clone_a); + + // Clone B: detect_and_plan + apply each fix via node_mut primitives. + let mut clone_b = doc_b; + let plan = detect_and_plan(&clone_b); + let applied: usize = plan + .iter() + .filter(|fix| apply_planned_fix_to_doc(&mut clone_b, fix)) + .count(); + + assert_eq!( + report.total as usize, applied, + "fix counts must match: detect_and_fix={}, planned={}", + report.total, applied + ); + assert_eq!(plan.len(), applied, "all planned fixes must apply"); + assert_eq!( + clone_a, clone_b, + "detect_and_fix and detect_and_plan+apply must produce identical documents" + ); + + // Verify the Info-severity dark-on-dark node is NOT in the plan. + let dark_in_plan = plan.iter().any(|f| f.node_id == "dark-on-dark"); + assert!( + !dark_in_plan, + "Info-severity nodes must not appear in the plan" + ); + } + + /// Load the `invisible-container-with-var` fixture (doc declares + /// `color-border` variable) and assert equivalence — exercises the + /// `$color-border` design-token reference path. + /// + /// Regression guard: caught by stop-time review — `LintPreValidator` + /// previously dropped `$color-border` refs while reporting success + /// because `cmd_set_node_stroke_hex` strict-parsed the hex. The + /// op-design-lint side (this test) ensures `detect_and_plan + apply` + /// produces the same `$color-border`-stamped doc as `detect_and_fix`; + /// the host parity test confirms the same through `EditorCommand`. + #[test] + fn equivalence_invisible_container_with_var() { + let raw = include_str!("../tests/fixtures/docs/invisible-container-with-var.json"); + let doc_a: PenDocument = serde_json::from_str(raw).expect("parse"); + let doc_b: PenDocument = serde_json::from_str(raw).expect("parse"); + + // Clone A: detect_and_fix (the reference path). + let mut clone_a = doc_a; + let report = detect_and_fix(&mut clone_a); + + // Clone B: detect_and_plan + apply each fix via node_mut primitives. + let mut clone_b = doc_b; + let plan = detect_and_plan(&clone_b); + let applied: usize = plan + .iter() + .filter(|fix| apply_planned_fix_to_doc(&mut clone_b, fix)) + .count(); + + assert_eq!(report.total as usize, applied); + assert_eq!(plan.len(), applied); + assert_eq!( + clone_a, clone_b, + "var-ref stroke must round-trip identically" + ); + + // Verify the plan carries the $color-border ref (not a resolved hex). + let stroke_plan = plan + .iter() + .find(|f| f.node_id == "light-on-light") + .expect("light-on-light should be in the plan"); + let value = match &stroke_plan.action { + PlannedAction::SetStroke(v) => v, + other => panic!("expected SetStroke, got {other:?}"), + }; + let color = value + .get("fill") + .and_then(|f| f.as_array()) + .and_then(|arr| arr.first()) + .and_then(|entry| entry.get("color")) + .and_then(|c| c.as_str()) + .expect("color field"); + assert_eq!( + color, "$color-border", + "plan must preserve design-token ref, not resolve to hex" + ); + } + + /// Load the `text-explicit-height` fixture and assert equivalence. + /// + /// This fixture exercises `FixProperty::Height` with `"fit_content"`. + #[test] + fn equivalence_text_explicit_height() { + let raw = include_str!("../tests/fixtures/docs/text-explicit-height.json"); + let doc_a: PenDocument = serde_json::from_str(raw).expect("parse"); + let doc_b: PenDocument = serde_json::from_str(raw).expect("parse"); + + let mut clone_a = doc_a; + let report = detect_and_fix(&mut clone_a); + + let mut clone_b = doc_b; + let plan = detect_and_plan(&clone_b); + let applied: usize = plan + .iter() + .filter(|fix| apply_planned_fix_to_doc(&mut clone_b, fix)) + .count(); + + assert_eq!(report.total as usize, applied); + assert_eq!(plan.len(), applied); + assert_eq!(clone_a, clone_b); + + // The fit-text node with explicit `height: "fit_content"` must NOT appear + // (it already has the keyword, no issue is raised for it). + let fit_in_plan = plan.iter().any(|f| f.node_id == "fit-text"); + assert!( + !fit_in_plan, + "fit-text has no issue; must not appear in plan" + ); + } + + /// Load the `stacked-horizontal-padding` fixture and assert equivalence. + /// + /// This fixture exercises an `Info`-severity `StackedHorizontalPadding` + /// issue: the entire plan should be empty (detect_and_fix applies zero + /// fixes) and both docs remain unchanged. + #[test] + fn equivalence_stacked_horizontal_padding() { + let raw = include_str!("../tests/fixtures/docs/stacked-horizontal-padding.json"); + let doc_a: PenDocument = serde_json::from_str(raw).expect("parse"); + let doc_b: PenDocument = serde_json::from_str(raw).expect("parse"); + + let mut clone_a = doc_a; + let report = detect_and_fix(&mut clone_a); + + let mut clone_b = doc_b; + let plan = detect_and_plan(&clone_b); + let applied: usize = plan + .iter() + .filter(|fix| apply_planned_fix_to_doc(&mut clone_b, fix)) + .count(); + + // All issues are Info severity — no fix is applied. + assert_eq!(report.total, 0, "detect_and_fix must apply zero fixes"); + assert_eq!(plan.len(), 0, "plan must be empty for all-Info fixtures"); + assert_eq!(applied, 0); + assert_eq!(clone_a, clone_b); + } + + /// Smoke-test: a `Remove`-action fix (empty-path fixture). + #[test] + fn equivalence_empty_path_remove() { + let d = doc(serde_json::json!({ + "version": "1.0", + "children": [{"type": "frame", "id": "root", "children": [ + {"type": "path", "id": "empty"} + ]}] + })); + + let mut clone_a = d.clone(); + let report = detect_and_fix(&mut clone_a); + + let mut clone_b = d; + let plan = detect_and_plan(&clone_b); + let applied: usize = plan + .iter() + .filter(|fix| apply_planned_fix_to_doc(&mut clone_b, fix)) + .count(); + + assert_eq!(report.total as usize, applied); + assert_eq!(clone_a, clone_b); + } + + /// The `status-bar` role guard: a `Remove`-action fix on a status-bar node + /// must NOT appear in the plan (mirrors the guard in `apply_remove`). + #[test] + fn equivalence_remove_status_bar_skipped() { + let d = doc(serde_json::json!({ + "version": "1.0", + "children": [{"type": "frame", "id": "root", "children": [ + {"type": "frame", "id": "bar", "role": "status-bar"} + ]}] + })); + + let mut clone_a = d.clone(); + let report = detect_and_fix(&mut clone_a); + + let mut clone_b = d; + let plan = detect_and_plan(&clone_b); + let applied: usize = plan + .iter() + .filter(|fix| apply_planned_fix_to_doc(&mut clone_b, fix)) + .count(); + + assert_eq!(report.total, 0, "status-bar must not be removed"); + assert_eq!(plan.len(), 0, "status-bar must not appear in the plan"); + assert_eq!(applied, 0); + assert_eq!(clone_a, clone_b); + } + + /// The `UnexpectedRotation` fix (Rotation property). + #[test] + fn equivalence_unexpected_rotation() { + let raw = include_str!("../tests/fixtures/docs/unexpected-rotation.json"); + let doc_a: PenDocument = serde_json::from_str(raw).expect("parse"); + let doc_b: PenDocument = serde_json::from_str(raw).expect("parse"); + + let mut clone_a = doc_a; + let report = detect_and_fix(&mut clone_a); + + let mut clone_b = doc_b; + let plan = detect_and_plan(&clone_b); + let applied: usize = plan + .iter() + .filter(|fix| apply_planned_fix_to_doc(&mut clone_b, fix)) + .count(); + + assert_eq!(report.total as usize, applied); + assert_eq!(clone_a, clone_b); + } +} diff --git a/crates/op-design-lint/tests/fixtures/docs/invisible-container-with-var.json b/crates/op-design-lint/tests/fixtures/docs/invisible-container-with-var.json new file mode 100644 index 000000000..225be4500 --- /dev/null +++ b/crates/op-design-lint/tests/fixtures/docs/invisible-container-with-var.json @@ -0,0 +1,23 @@ +{ + "version": "1.0", + "variables": { + "color-border": { "type": "color", "value": "#CBD5E1" } + }, + "children": [ + { + "type": "frame", + "id": "root", + "layout": "vertical", + "fill": [{ "type": "solid", "color": "#FFFFFF" }], + "children": [ + { + "type": "frame", + "id": "light-on-light", + "layout": "vertical", + "fill": [{ "type": "solid", "color": "#FFFFFF" }], + "children": [{ "type": "text", "id": "lol-label", "content": "Inside light wrapper" }] + } + ] + } + ] +} diff --git a/crates/op-design-lint/tests/fixtures/golden/invisible-container-with-var.json b/crates/op-design-lint/tests/fixtures/golden/invisible-container-with-var.json new file mode 100644 index 000000000..7154a13bc --- /dev/null +++ b/crates/op-design-lint/tests/fixtures/golden/invisible-container-with-var.json @@ -0,0 +1,19 @@ +[ + { + "nodeId": "light-on-light", + "category": "invisible-container", + "severity": "warning", + "property": "stroke", + "currentValue": null, + "suggestedValue": { + "thickness": 1, + "fill": [ + { + "type": "solid", + "color": "$color-border" + } + ] + }, + "reason": "same fill as parent (#FFFFFF ≈ #FFFFFF, contrast=1.00)" + } +] diff --git a/crates/op-design-lint/tests/parity.rs b/crates/op-design-lint/tests/parity.rs index a1ebed85b..284d32e15 100644 --- a/crates/op-design-lint/tests/parity.rs +++ b/crates/op-design-lint/tests/parity.rs @@ -178,6 +178,10 @@ parity_case!(parity_edge_section_padding, "edge-section-padding"); parity_case!(parity_empty_path, "empty-path"); parity_case!(parity_excessive_frame_effects, "excessive-frame-effects"); parity_case!(parity_invisible_container, "invisible-container"); +parity_case!( + parity_invisible_container_with_var, + "invisible-container-with-var" +); parity_case!(parity_mixed_sibling, "mixed-sibling"); parity_case!(parity_sibling_inconsistency, "sibling-inconsistency"); parity_case!( diff --git a/crates/op-editor-core/src/command_node_attrs.rs b/crates/op-editor-core/src/command_node_attrs.rs index 02234628a..6d07cd6be 100644 --- a/crates/op-editor-core/src/command_node_attrs.rs +++ b/crates/op-editor-core/src/command_node_attrs.rs @@ -171,8 +171,19 @@ impl EditorState { /// `SetNodeStrokeHex` — set the stroke color on a node. A node with /// no stroke gets a fresh 1-px stroke so the color always lands. + /// + /// Accepts either a literal `#RGB`/`#RRGGBB` hex OR a `$variable-name` + /// design-token reference. The underlying `PenStroke.fill[*].color` + /// field is a free `String`; `op-design-lint` detectors emit + /// `$color-border` refs when the doc declares that variable, and a + /// strict hex parse here would silently drop them (regression caught + /// by a stop-time review of `LintPreValidator`). pub(crate) fn cmd_set_node_stroke_hex(&mut self, node_id: &NodeId, hex: &str) -> bool { - if !node_id.is_real() || crate::color_picker::parse_hex_rgb(hex).is_none() { + if !node_id.is_real() { + return false; + } + let is_var_ref = hex.starts_with('$'); + if !is_var_ref && crate::color_picker::parse_hex_rgb(hex).is_none() { return false; } let Some(node) = find_node_mut(self.active_children_mut(), node_id) else { @@ -216,8 +227,16 @@ impl EditorState { } /// `SetNodeFillHex` — set the fill color on a node by id. + /// + /// Accepts either a literal `#RGB`/`#RRGGBB` hex OR a `$variable-name` + /// design-token reference (same rationale as `SetNodeStrokeHex` — + /// `PenFill::Solid.color` is a free `String`). pub(crate) fn cmd_set_node_fill_hex(&mut self, node_id: &NodeId, hex: &str) -> bool { - if !node_id.is_real() || crate::color_picker::parse_hex_rgb(hex).is_none() { + if !node_id.is_real() { + return false; + } + let is_var_ref = hex.starts_with('$'); + if !is_var_ref && crate::color_picker::parse_hex_rgb(hex).is_none() { return false; } let Some(node) = find_node_mut(self.active_children_mut(), node_id) else { diff --git a/crates/op-host-desktop/Cargo.toml b/crates/op-host-desktop/Cargo.toml index a89d3b768..598203fc5 100644 --- a/crates/op-host-desktop/Cargo.toml +++ b/crates/op-host-desktop/Cargo.toml @@ -48,6 +48,8 @@ op-host-native = { path = "../op-host-native", version = "0.1.0" } # `op_editor_core::EditorState` (canonical `.op` document) instead of the # old shell-core `Document`. op-editor-core = { path = "../op-editor-core" } +# D′ architecture: pre-validation adapter translates PlannedFix → EditorCommand. +op-design-lint = { path = "../op-design-lint" } # Locale tables — git dialog strings go through `op_i18n::translate` # so they are localized like the rest of the chrome. op-i18n = { path = "../op-i18n" } diff --git a/crates/op-host-desktop/src/chat_orchestrator.rs b/crates/op-host-desktop/src/chat_orchestrator.rs index 922358bd5..55c62dd76 100644 --- a/crates/op-host-desktop/src/chat_orchestrator.rs +++ b/crates/op-host-desktop/src/chat_orchestrator.rs @@ -47,8 +47,8 @@ use futures::StreamExt; use op_editor_core::{EditorCommand, EditorState}; use op_orchestrator::{ AbortFlag, CallRequest, DesignRequest, DocSink, LlmChunk, LlmClient, LlmError, Orchestrator, - OrchestratorError, Progress, RunSummary, SkippedPreValidator, SkippedScreenshotProvider, - SkippedVisionLlmClient, ValidationProviders, + OrchestratorError, Progress, RunSummary, SkippedScreenshotProvider, SkippedVisionLlmClient, + ValidationProviders, }; /// `LlmClient` 的 desktop 实现。每次 `call` 新建一个 `QueryEngine` @@ -194,13 +194,12 @@ pub async fn run_design_request( visual_ref_enabled: false, }; let mut sink = DesktopDocSink::new(state); - // Stub validation providers — production-visible, no-op. Host can - // swap in real `PreValidator` (when `op-design-lint` is wired), - // `ScreenshotProvider` (when `jian-skia::captureRegion` is exposed - // host-side), and `VisionLlmClient` (when a Rust multimodal client - // lands) without touching `op-orchestrator`. Today `validation_enabled - // = false` short-circuits the validation phase anyway. - let pre_validator = SkippedPreValidator; + // PreValidator: real `op-design-lint` adapter (D′ architecture, task #35). + // `ScreenshotProvider` and `VisionLlmClient` remain stubbed until + // `jian-skia::captureRegion` is exposed host-side and a Rust multimodal + // client lands. Today `validation_enabled = false` short-circuits the + // vision loop; only `run_pre_validation_fixes` is exercised per request. + let pre_validator = crate::pre_validator::LintPreValidator; let screenshot = SkippedScreenshotProvider; let vision = SkippedVisionLlmClient; let providers = ValidationProviders { diff --git a/crates/op-host-desktop/src/main.rs b/crates/op-host-desktop/src/main.rs index 304b9e77a..1ef8e72a9 100644 --- a/crates/op-host-desktop/src/main.rs +++ b/crates/op-host-desktop/src/main.rs @@ -29,6 +29,7 @@ mod menu; mod model_discovery; mod persistence; mod persistence_image; +mod pre_validator; mod settings_io; mod update_check; mod window_state; diff --git a/crates/op-host-desktop/src/pre_validator.rs b/crates/op-host-desktop/src/pre_validator.rs new file mode 100644 index 000000000..ea1015bc1 --- /dev/null +++ b/crates/op-host-desktop/src/pre_validator.rs @@ -0,0 +1,420 @@ +//! D′ architecture host adapter — translates `PlannedFix → EditorCommand`. +//! +//! `LintPreValidator` implements `op_orchestrator::PreValidator` by: +//! +//! 1. Cloning the current document from `sink.state().doc`. +//! 2. Calling `op_design_lint::detect_and_plan` to get a `Vec`. +//! 3. Translating each `PlannedFix` to one or more `EditorCommand`s via +//! [`planned_fix_to_commands`]. +//! 4. Applying each command via `sink.apply` and counting successes. +//! +//! ## FixProperty → EditorCommand translation table +//! +//! | `PlannedAction` | `EditorCommand`(s) | +//! |------------------------|----------------------------------------------------------| +//! | `Remove` | `DeleteNode { node_id }` | +//! | `SetHeightFitContent` | `SetNodeLayoutProp { property:"height", Keyword("fit_content") }` | +//! | `SetRotation(deg)` | `SetNodeRotation { degrees }` | +//! | `SetCornerRadius(r)` | `SetNodeCornerRadius { radius }` | +//! | `SetFontSize(px)` | `SetNodeFontSize { font_size }` | +//! | `ClearEffects` | N × `RemoveNodeEffect { index }` (highest→lowest) | +//! | `SetPadding(v)` | `SetNodeLayoutProp { property:"padding", NumberArray/Number }` | +//! | `SetStroke(v)` | `SetNodeStrokeHex { hex }` + `SetNodeStrokeWidth { width }` | +//! +//! ### `ClearEffects` +//! `EditorCommand` has no "clear all effects" variant — the adapter reads the +//! current effect count from `sink.state()` and emits `RemoveNodeEffect` for +//! each index, iterating highest-to-lowest so index shifts don't affect earlier +//! removals. After all removals the effects field is `None`, matching +//! `op_design_lint::node_mut::clear_effects`. +//! +//! ### `SetStroke` +//! `EditorCommand` has no "set full PenStroke object" variant. The adapter +//! decomposes the JSON object into: +//! - `SetNodeStrokeHex { hex }` — extracts the first solid-fill color from +//! the suggested value's `fill[0].color`; creates a fresh 1px stroke if +//! the node has none. +//! - `SetNodeStrokeWidth { width }` — extracts `thickness` as `f32`. +//! +//! The resulting `PenStroke` is structurally identical to what +//! `node_mut::set_stroke_from_json` produces for the current fixture set +//! (all invisible-container stroke fixes use `{thickness:1, fill:[{type:solid, +//! color:"#E2E8F0"}]}`). This is verified by the parity integration test. +//! If a future detector emits a stroke with non-solid fills or per-side +//! thickness, extend the translation or report a gap. + +use op_design_lint::plan::{PlannedAction, PlannedFix}; +use op_editor_core::{EditorCommand, EditorState}; +use op_orchestrator::{DocSink, PreValidationResult, PreValidator}; + +/// Real `PreValidator` implementation backed by `op-design-lint`. +/// +/// Swap `SkippedPreValidator` for this in `chat_orchestrator.rs` to +/// enable pre-validation fixes on every design generation run. +pub struct LintPreValidator; + +impl PreValidator for LintPreValidator { + fn run_pre_validation_fixes(&self, sink: &mut dyn DocSink) -> PreValidationResult { + // Clone the document snapshot so detect_and_plan can read it + // immutably while sink remains available for apply calls. + let doc_clone = sink.state().doc.clone(); + let plan = op_design_lint::detect_and_plan(&doc_clone); + + let mut total = 0usize; + let mut by_category: std::collections::BTreeMap = Default::default(); + + for fix in &plan { + let commands = planned_fix_to_commands(fix, sink.state()); + // Count this fix as applied only if at least one command lands. + let mut any_applied = false; + for cmd in commands { + if sink.apply(cmd) { + any_applied = true; + } + } + if any_applied { + total += 1; + let key = serde_json::to_value(fix.category) + .ok() + .and_then(|v| v.as_str().map(str::to_string)) + .unwrap_or_else(|| format!("{:?}", fix.category)); + *by_category.entry(key).or_insert(0) += 1; + } + } + + PreValidationResult { total, by_category } + } +} + +/// Translate one `PlannedFix` into the `EditorCommand`(s) needed to apply it. +/// Returns an empty vec only for configurations where no command is appropriate +/// (e.g. a `SetStroke` whose JSON has no extractable hex — should not happen +/// in practice since `detect_and_plan` only emits valid suggested values). +fn planned_fix_to_commands(fix: &PlannedFix, state: &EditorState) -> Vec { + use op_editor_core::node_id::NodeId; + + let node_id = NodeId::new(&*fix.node_id); + let mut cmds = Vec::new(); + + match &fix.action { + PlannedAction::Remove => { + cmds.push(EditorCommand::DeleteNode { + node_id: node_id.clone(), + }); + } + + PlannedAction::SetHeightFitContent => { + cmds.push(EditorCommand::SetNodeLayoutProp { + node_id: node_id.clone(), + property: "height".to_string(), + value: op_editor_core::LayoutPropValue::Keyword("fit_content".to_string()), + }); + } + + PlannedAction::SetRotation(deg) => { + cmds.push(EditorCommand::SetNodeRotation { + node_id: node_id.clone(), + degrees: *deg as f32, + }); + } + + PlannedAction::SetCornerRadius(r) => { + cmds.push(EditorCommand::SetNodeCornerRadius { + node_id: node_id.clone(), + radius: *r, + }); + } + + PlannedAction::SetFontSize(px) => { + cmds.push(EditorCommand::SetNodeFontSize { + node_id: node_id.clone(), + font_size: *px, + }); + } + + PlannedAction::ClearEffects => { + // Read the current effect count from the state, then emit + // RemoveNodeEffect for each index from highest to lowest so + // earlier indices are not invalidated by the removal. + let effect_count = effect_count_for_node(state, &node_id); + for i in (0..effect_count).rev() { + cmds.push(EditorCommand::RemoveNodeEffect { + node_id: node_id.clone(), + index: i as u32, + }); + } + } + + PlannedAction::SetPadding(v) => { + // Translate to SetNodeLayoutProp "padding" with Number or NumberArray. + let prop_value = if let Some(n) = v.as_f64() { + Some(op_editor_core::LayoutPropValue::Number(n)) + } else if let Some(arr) = v.as_array() { + let nums: Option> = arr.iter().map(|x| x.as_f64()).collect(); + nums.map(op_editor_core::LayoutPropValue::NumberArray) + } else { + None + }; + if let Some(pv) = prop_value { + cmds.push(EditorCommand::SetNodeLayoutProp { + node_id: node_id.clone(), + property: "padding".to_string(), + value: pv, + }); + } + } + + PlannedAction::SetStroke(v) => { + // Decompose the PenStroke JSON into SetNodeStrokeHex + SetNodeStrokeWidth. + // Both commands are emitted; the hex command creates a fresh stroke if needed. + // + // Extract hex from fill[0].color (first solid fill entry). + let hex = v + .get("fill") + .and_then(|f| f.as_array()) + .and_then(|arr| arr.first()) + .and_then(|entry| entry.get("color")) + .and_then(|c| c.as_str()) + .map(str::to_string); + + // Extract thickness as f32. + let thickness = v + .get("thickness") + .and_then(|t| t.as_f64()) + .map(|t| t as f32); + + if let Some(hex) = hex { + cmds.push(EditorCommand::SetNodeStrokeHex { + node_id: node_id.clone(), + hex, + }); + } + if let Some(width) = thickness { + cmds.push(EditorCommand::SetNodeStrokeWidth { + node_id: node_id.clone(), + width, + }); + } + } + } + + cmds +} + +/// Count the number of effects on the named node in the active page. +/// Returns 0 if the node is not found or has no effects. +fn effect_count_for_node(state: &EditorState, node_id: &op_editor_core::node_id::NodeId) -> usize { + use op_editor_core::walkers::find_node; + + let children = state.active_children(); + let Some(node) = find_node(children, node_id) else { + return 0; + }; + effects_from_pen_node(node).len() +} + +/// Extract the effect list from a `PenNode`, returning an empty slice +/// when the node kind has no effects or effects is `None`. +fn effects_from_pen_node( + node: &jian_ops_schema::node::PenNode, +) -> &[jian_ops_schema::style::PenEffect] { + use jian_ops_schema::node::PenNode; + let opt = match node { + PenNode::Frame(n) => n.container.effects.as_deref(), + PenNode::Group(n) => n.container.effects.as_deref(), + PenNode::Rectangle(n) => n.container.effects.as_deref(), + PenNode::Ellipse(n) => n.effects.as_deref(), + PenNode::Polygon(n) => n.effects.as_deref(), + PenNode::Path(n) => n.effects.as_deref(), + PenNode::Line(n) => n.effects.as_deref(), + PenNode::Text(n) => n.effects.as_deref(), + PenNode::TextInput(n) => n.effects.as_deref(), + PenNode::Image(n) => n.effects.as_deref(), + PenNode::IconFont(_) | PenNode::Ref(_) => None, + }; + opt.unwrap_or(&[]) +} + +// ── Parity integration tests ───────────────────────────────────────────────── +// +// For each of the same 3 fixtures used in `op-design-lint`'s equivalence tests: +// - Build an `EditorState` whose `doc` is the fixture's `PenDocument`. +// - Wrap it in a `TestSink`. +// - Call `LintPreValidator.run_pre_validation_fixes(&mut sink)`. +// - Compare `sink.state().doc` against the result of +// `op_design_lint::detect_and_fix(&mut fixture_doc.clone())`. +// - Assert both produce structurally-identical `PenDocument`s. +// +// This is the LOAD-BEARING test that proves the cross-crate adapter doesn't +// silently lose mutations. + +#[cfg(test)] +mod tests { + use super::*; + use op_editor_core::EditorState; + use op_orchestrator::PreValidator; + + /// Minimal `DocSink` for tests — wraps an `EditorState`, applies + /// commands through `EditorState::apply`. Modeled on + /// `op-orchestrator::test_support::VecDocSink`. + struct TestSink { + state: EditorState, + } + + impl TestSink { + fn from_doc(doc: jian_ops_schema::PenDocument) -> Self { + Self { + state: EditorState::from_document(doc), + } + } + } + + impl op_orchestrator::DocSink for TestSink { + fn state(&self) -> &EditorState { + &self.state + } + fn apply(&mut self, cmd: op_editor_core::EditorCommand) -> bool { + self.state.apply(cmd) + } + fn begin_undo_batch(&mut self) {} + fn end_undo_batch(&mut self) {} + } + + fn load_doc(json: &str) -> jian_ops_schema::PenDocument { + serde_json::from_str(json).expect("fixture must parse as PenDocument") + } + + /// Normalize a `serde_json::Value` for structural comparison. + /// + /// `EditorState::find_node_mut` calls `children_mut()` which uses + /// `get_or_insert_with(Vec::new)` — this side-effects frames that + /// were traversed but not mutated from `children: None` → + /// `children: Some([])`. The `detect_and_fix` path does NOT do + /// this. We normalize away this difference by removing any JSON + /// `"children"` key whose value is an empty array (`[]`). + /// + /// Every other field is compared exactly. This is the only known + /// structural difference between the two paths. + fn normalize_doc_json(v: serde_json::Value) -> serde_json::Value { + use serde_json::Value; + match v { + Value::Object(mut map) => { + // Remove "children": [] (empty array) — artifact of + // EditorState walker's get_or_insert_with(Vec::new). + if let Some(Value::Array(arr)) = map.get("children") { + if arr.is_empty() { + map.remove("children"); + } + } + Value::Object( + map.into_iter() + .map(|(k, v)| (k, normalize_doc_json(v))) + .collect(), + ) + } + Value::Array(arr) => Value::Array(arr.into_iter().map(normalize_doc_json).collect()), + other => other, + } + } + + /// Assert that `LintPreValidator` + `EditorState` produces the same + /// `PenDocument` as `op_design_lint::detect_and_fix` on the same input. + /// + /// Structural comparison uses JSON normalization to handle the known + /// `children: None` vs `children: Some([])` difference introduced by + /// `EditorState::find_node_mut`'s `get_or_insert_with(Vec::new)` side + /// effect (see `normalize_doc_json`). + fn assert_parity(fixture_json: &str, label: &str) { + // Reference path: detect_and_fix on a raw clone. + let mut reference_doc = load_doc(fixture_json); + let reference_report = op_design_lint::detect_and_fix(&mut reference_doc); + + // Adapter path: LintPreValidator through EditorState. + let adapter_doc = load_doc(fixture_json); + let mut sink = TestSink::from_doc(adapter_doc); + let result = LintPreValidator.run_pre_validation_fixes(&mut sink); + + assert_eq!( + reference_report.total as usize, result.total, + "{label}: applied counts must match (reference={}, adapter={})", + reference_report.total, result.total + ); + + // Compare via normalized JSON to handle children: None vs Some([]). + let ref_json = serde_json::to_value(&reference_doc).expect("serialize reference"); + let adapter_json = serde_json::to_value(&sink.state().doc).expect("serialize adapter"); + let ref_norm = normalize_doc_json(ref_json); + let adapter_norm = normalize_doc_json(adapter_json); + assert_eq!( + ref_norm, adapter_norm, + "{label}: documents must be structurally identical after both fix paths" + ); + } + + /// invisible-container: exercises `FixProperty::Stroke` (Warning) + /// and verifies Info-severity dark-on-dark is NOT applied. + #[test] + fn parity_invisible_container() { + assert_parity( + include_str!("../../op-design-lint/tests/fixtures/docs/invisible-container.json"), + "invisible-container", + ); + } + + /// text-explicit-height: exercises `FixProperty::Height` → `SetHeightFitContent`. + #[test] + fn parity_text_explicit_height() { + assert_parity( + include_str!("../../op-design-lint/tests/fixtures/docs/text-explicit-height.json"), + "text-explicit-height", + ); + } + + /// stacked-horizontal-padding: all Info severity — zero fixes applied; + /// doc remains unchanged under both paths. + #[test] + fn parity_stacked_horizontal_padding() { + assert_parity( + include_str!( + "../../op-design-lint/tests/fixtures/docs/stacked-horizontal-padding.json" + ), + "stacked-horizontal-padding", + ); + } + + /// unexpected-rotation: exercises `FixProperty::Rotation` → `SetNodeRotation`. + #[test] + fn parity_unexpected_rotation() { + assert_parity( + include_str!("../../op-design-lint/tests/fixtures/docs/unexpected-rotation.json"), + "unexpected-rotation", + ); + } + + /// empty-path: exercises `FixProperty::Remove` → `DeleteNode`. + #[test] + fn parity_empty_path() { + assert_parity( + include_str!("../../op-design-lint/tests/fixtures/docs/empty-path.json"), + "empty-path", + ); + } + + /// invisible-container-with-var: exercises `$color-border` design-token + /// reference path through `SetNodeStrokeHex`. Regression guard caught + /// by stop-time review — `cmd_set_node_stroke_hex` previously rejected + /// `$`-prefixed strings via `parse_hex_rgb`, dropping the color silently + /// while `SetNodeStrokeWidth` still applied (so `any_applied=true` and + /// `applied_count` overreported). The op-editor-core fix relaxed both + /// `cmd_set_node_stroke_hex` + `cmd_set_node_fill_hex` to passthrough + /// `$ref` strings (the underlying `color: String` field accepts them). + #[test] + fn parity_invisible_container_with_var() { + assert_parity( + include_str!( + "../../op-design-lint/tests/fixtures/docs/invisible-container-with-var.json" + ), + "invisible-container-with-var", + ); + } +}