From 85586151e5a6683f38346aec1b9cfc8ca8598526 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Wed, 29 Jul 2026 22:15:30 -0400 Subject: [PATCH] refactor(graphviz): parse node classes in one place Node classes were built in two places. The parser split the `class` attribute on commas and whitespace, but the import transform re-split the raw attribute on commas only. A space-separated class on an import placeholder became a single class name, so stylesheet rules did not match. That included the `class="fast shared"` example in the imports docs. - add `Node::add_class`, replacing the duplicate append helpers in `SemanticState` and `ImportTransform` - read `node.classes` in `placeholder_config` instead of re-parsing the raw attribute, so class splitting happens in exactly one place - name the separator rule `split_class_attr`, splitting on commas and then whitespace so empty entries need no trimming - drop the unused `Node::class` accessor that invited the re-parse - keep the comma-compatibility note in the DOT attribute reference only Co-Authored-By: Claude Opus 5 (1M context) --- docs/public/tutorials/multi-model.mdx | 2 +- docs/public/workflows/stages-and-nodes.mdx | 2 +- docs/public/workflows/stylesheets.mdx | 1 - .../fabro-graphviz/src/parser/semantic.rs | 33 ++++++++++--------- .../fabro-workflow/src/transforms/import.rs | 28 +++------------- lib/foundation/fabro-types/src/graph.rs | 18 ++++++---- 6 files changed, 36 insertions(+), 48 deletions(-) diff --git a/docs/public/tutorials/multi-model.mdx b/docs/public/tutorials/multi-model.mdx index e88e33006..33010b693 100644 --- a/docs/public/tutorials/multi-model.mdx +++ b/docs/public/tutorials/multi-model.mdx @@ -77,7 +77,7 @@ Set the `class` attribute on a node to target it with class selectors: implement [label="Implement", class="coding"] ``` -Separate multiple classes with spaces: `class="coding critical"`. Fabro also accepts commas as class separators for compatibility. +Separate multiple classes with spaces: `class="coding critical"`. ## Properties diff --git a/docs/public/workflows/stages-and-nodes.mdx b/docs/public/workflows/stages-and-nodes.mdx index fd35d013d..02a552990 100644 --- a/docs/public/workflows/stages-and-nodes.mdx +++ b/docs/public/workflows/stages-and-nodes.mdx @@ -235,7 +235,7 @@ These attributes can be set on any node type: | Attribute | Description | |---|---| | `label` | Display name shown in the graph visualization | -| `class` | CSS-like class for [model stylesheet](/workflows/stylesheets) targeting. Separate multiple classes with spaces. Commas are also accepted for compatibility. | +| `class` | CSS-like class for [model stylesheet](/workflows/stylesheets) targeting. Separate multiple classes with spaces. | | `max_visits` | Max times this node can execute in a run. Overrides the graph-level `max_node_visits` for this node. | | `goal_gate` | When `true`, the workflow fails if this node doesn't succeed | | `max_retries` | Override default retry count for this node | diff --git a/docs/public/workflows/stylesheets.mdx b/docs/public/workflows/stylesheets.mdx index cb82081a7..b0a7dad76 100644 --- a/docs/public/workflows/stylesheets.mdx +++ b/docs/public/workflows/stylesheets.mdx @@ -57,7 +57,6 @@ implement [label="Implement", class="coding critical"] ``` This node matches both `.coding` and `.critical` rules. -Fabro also accepts commas as class separators for compatibility. ## Properties diff --git a/lib/components/fabro-graphviz/src/parser/semantic.rs b/lib/components/fabro-graphviz/src/parser/semantic.rs index bac235bb6..688d1c68c 100644 --- a/lib/components/fabro-graphviz/src/parser/semantic.rs +++ b/lib/components/fabro-graphviz/src/parser/semantic.rs @@ -47,6 +47,15 @@ fn convert_attrs(block: &AttrBlock) -> HashMap { .collect() } +/// Split a `class` attribute value into individual class names. +/// +/// Classes are separated by whitespace. Commas are also accepted, because they +/// were the only separator Fabro used to recognize. Splitting on commas first +/// and then on whitespace drops empty entries without extra trimming. +fn split_class_attr(class_attr: &str) -> impl Iterator { + class_attr.split(',').flat_map(str::split_whitespace) +} + /// Derive a CSS class name from a subgraph label. fn derive_class_from_label(label: &str) -> String { label @@ -97,13 +106,6 @@ impl SemanticState { }) } - fn add_class_to_node(node: &mut Node, cls: &str) { - let cls_string = cls.to_string(); - if !node.classes.contains(&cls_string) { - node.classes.push(cls_string); - } - } - fn process_node(&mut self, node_stmt: &NodeStmt, subgraph_class: Option<&str>) { let node = self.ensure_node(&node_stmt.id); if let Some(attrs) = &node_stmt.attrs { @@ -112,19 +114,18 @@ impl SemanticState { } } if let Some(cls) = subgraph_class { - Self::add_class_to_node(node, cls); + node.add_class(cls); } - // Parse explicit class attr into classes vec - let class_str = node + // Node defaults can also set `class`, so read the merged attrs. The + // clone releases the borrow on `node.attrs` before appending. + let class_attr = node .attrs .get("class") .and_then(AttrValue::as_str) .map(String::from); - if let Some(class_str) = class_str { - for cls in class_str.split(|ch: char| ch == ',' || ch.is_whitespace()) { - if !cls.is_empty() { - Self::add_class_to_node(node, cls); - } + if let Some(class_attr) = class_attr { + for cls in split_class_attr(&class_attr) { + node.add_class(cls); } } } @@ -136,7 +137,7 @@ impl SemanticState { } let node = self.ensure_node(id); if let Some(cls) = subgraph_class { - Self::add_class_to_node(node, cls); + node.add_class(cls); } } let edge_attrs = edge_stmt diff --git a/lib/components/fabro-workflow/src/transforms/import.rs b/lib/components/fabro-workflow/src/transforms/import.rs index f2c5e74fd..e0082ba9d 100644 --- a/lib/components/fabro-workflow/src/transforms/import.rs +++ b/lib/components/fabro-workflow/src/transforms/import.rs @@ -352,11 +352,9 @@ impl ImportTransform { Self::remap_retry_target(&mut merged_node.attrs, placeholder_id); for class_name in &placeholder.class_names { - Self::push_class(&mut merged_node.classes, class_name); - } - if !placeholder.normalized_class.is_empty() { - Self::push_class(&mut merged_node.classes, &placeholder.normalized_class); + merged_node.add_class(class_name); } + merged_node.add_class(&placeholder.normalized_class); graph.nodes.insert(prefixed_id, merged_node); } @@ -413,24 +411,14 @@ impl ImportTransform { .get(placeholder_id) .ok_or_else(|| format!("missing import placeholder '{placeholder_id}'"))?; let mut default_attrs = HashMap::new(); - let mut class_names = Vec::new(); for (key, value) in &node.attrs { if key == "import" { continue; } + // The parser already split `class` into `node.classes`. if key == "class" { - if let Some(class_attr) = value.as_str() { - for class_name in class_attr.split(',') { - let class_name = class_name.trim(); - if !class_name.is_empty() - && !class_names.iter().any(|value| value == class_name) - { - class_names.push(class_name.to_string()); - } - } - } continue; } @@ -446,7 +434,7 @@ impl ImportTransform { Ok(PlaceholderOptions { default_attrs, - class_names, + class_names: node.classes.clone(), normalized_class: Self::normalize_class_name(placeholder_id), }) } @@ -620,12 +608,6 @@ impl ImportTransform { .any(|key| edge.attrs.contains_key(key)) } - fn push_class(classes: &mut Vec, class_name: &str) { - if !classes.iter().any(|value| value == class_name) { - classes.push(class_name.to_string()); - } - } - fn normalize_class_name(label: &str) -> String { label .to_lowercase() @@ -1120,7 +1102,7 @@ mod tests { let graph = apply_import( r#"digraph Deploy { start [shape=Mdiamond] - validate [import="./validate.fabro", model="haiku", backend="acp", acp.command="python fake_agent.py", class="fast, shared"] + validate [import="./validate.fabro", model="haiku", backend="acp", acp.command="python fake_agent.py", class="fast shared"] exit [shape=Msquare] start -> validate -> exit }"#, diff --git a/lib/foundation/fabro-types/src/graph.rs b/lib/foundation/fabro-types/src/graph.rs index fd2e5296d..dc0e4da16 100644 --- a/lib/foundation/fabro-types/src/graph.rs +++ b/lib/foundation/fabro-types/src/graph.rs @@ -149,6 +149,17 @@ impl Node { } } + /// Appends a class, ignoring blank names and ones already present. + /// + /// Classes accumulate from several sources — the `class` attribute, + /// enclosing subgraphs, and import placeholders — so every caller needs the + /// same de-duplicating append. + pub fn add_class(&mut self, class: &str) { + if !class.is_empty() && !self.classes.iter().any(|existing| existing == class) { + self.classes.push(class.to_string()); + } + } + fn str_attr(&self, key: &str) -> Option<&str> { self.attrs.get(key).and_then(AttrValue::as_str) } @@ -273,11 +284,6 @@ impl Node { self.str_attr("thread_id") } - #[must_use] - pub fn class(&self) -> Option<&str> { - self.str_attr("class") - } - pub fn timeout(&self) -> Option { self.attrs.get("timeout").and_then(AttrValue::as_duration) } @@ -663,7 +669,7 @@ mod tests { assert_eq!(node.fallback_retry_target(), None); assert_eq!(node.fidelity(), None); assert_eq!(node.thread_id(), None); - assert_eq!(node.class(), None); + assert!(node.classes.is_empty()); assert_eq!(node.timeout(), None); assert_eq!(node.model(), None); assert_eq!(node.provider(), None);