From 02e81c79749eb535dd02df8d2ce6854f054f7ada Mon Sep 17 00:00:00 2001 From: Fini Date: Sun, 24 May 2026 12:57:31 +0800 Subject: [PATCH] fix(editor-core,host): SetNode{Stroke,Fill}Hex accept $variable refs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex stop-time review caught a real regression in LintPreValidator: when the doc declares a `color-border` variable, op-design-lint's `border_stroke()` emits the suggested stroke fill as `$color-border` (a design-token reference, not raw hex). The host adapter decomposes `SetStroke` into `SetNodeStrokeHex` + `SetNodeStrokeWidth`. But `cmd_set_node_stroke_hex` strict-parsed the hex via `parse_hex_rgb` and rejected the `$`-prefixed string with `return false`, while `SetNodeStrokeWidth` still applied successfully — so `any_applied = true` and `applied_count += 1` overreported success while the stroke color was silently dropped. The TS counterpart (`fixes.rs::set_stroke_from_json` via `serde_json::from_value::`) treats `color` as a free `String` and accepts `$ref` verbatim. The underlying `set_primary_{stroke,fill}_hex` already writes `slot.color = hex.to_string()` unconditionally — only the cmd-layer gate was wrong. Fix: - `cmd_set_node_stroke_hex` + `cmd_set_node_fill_hex` (op-editor-core): let `$`-prefixed strings bypass `parse_hex_rgb`. Literal hex paths unchanged; doc-comment notes the design-token contract. - New fixture `invisible-container-with-var.json` (op-design-lint): doc declares `color-border` var so the detector emits `$color-border` ref. Regen'd golden confirms the ref reaches the Issue's `suggestedValue`. - New equivalence test `equivalence_invisible_container_with_var` in `plan.rs`: `detect_and_plan + apply` produces same doc as `detect_and_fix`, AND the plan carries the `$color-border` ref (not a resolved hex). - New parity test `parity_invisible_container_with_var` in `pre_validator.rs`: cross-crate adapter round-trip preserves the ref through EditorCommand application. All 5 `Skipped*Provider` and 4 `PreValidator`-path stubs still in place. drift guard re-runs clean (14 goldens, same as committed). `cargo test` op-design-lint 143+13 / op-host-desktop 102 / op-editor-core 280 / op-orchestrator 585+1 all green. `cargo clippy --workspace -- -D warnings` clean. `cargo fmt --all -- --check` clean. --- crates/op-design-lint/src/plan.rs | 57 +++++++++++++++++++ .../docs/invisible-container-with-var.json | 23 ++++++++ .../golden/invisible-container-with-var.json | 19 +++++++ crates/op-design-lint/tests/parity.rs | 4 ++ .../op-editor-core/src/command_node_attrs.rs | 23 +++++++- crates/op-host-desktop/src/pre_validator.rs | 18 ++++++ 6 files changed, 142 insertions(+), 2 deletions(-) create mode 100644 crates/op-design-lint/tests/fixtures/docs/invisible-container-with-var.json create mode 100644 crates/op-design-lint/tests/fixtures/golden/invisible-container-with-var.json diff --git a/crates/op-design-lint/src/plan.rs b/crates/op-design-lint/src/plan.rs index 5bd35756b..762c16f63 100644 --- a/crates/op-design-lint/src/plan.rs +++ b/crates/op-design-lint/src/plan.rs @@ -338,6 +338,63 @@ mod tests { ); } + /// 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"`. 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/src/pre_validator.rs b/crates/op-host-desktop/src/pre_validator.rs index 9aa755ca3..ea1015bc1 100644 --- a/crates/op-host-desktop/src/pre_validator.rs +++ b/crates/op-host-desktop/src/pre_validator.rs @@ -399,4 +399,22 @@ mod tests { "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", + ); + } }