fix(editor-core,host): SetNode{Stroke,Fill}Hex accept $variable refs
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::<PenStroke>`) 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.
This commit is contained in:
parent
fe7f173906
commit
02e81c7974
|
|
@ -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"`.
|
||||
|
|
|
|||
23
crates/op-design-lint/tests/fixtures/docs/invisible-container-with-var.json
vendored
Normal file
23
crates/op-design-lint/tests/fixtures/docs/invisible-container-with-var.json
vendored
Normal file
|
|
@ -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" }]
|
||||
}
|
||||
]
|
||||
}
|
||||
]
|
||||
}
|
||||
19
crates/op-design-lint/tests/fixtures/golden/invisible-container-with-var.json
vendored
Normal file
19
crates/op-design-lint/tests/fixtures/golden/invisible-container-with-var.json
vendored
Normal file
|
|
@ -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)"
|
||||
}
|
||||
]
|
||||
|
|
@ -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!(
|
||||
|
|
|
|||
|
|
@ -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 {
|
||||
|
|
|
|||
|
|
@ -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",
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue