scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-10-08 03:10:26 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 5

refactor(graphviz): parse node classes in one place

Browse source 
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) <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-07-29 22:15:30 -04:00
parent af9aa53088
commit 85586151e5
No known key found for this signature in database
6 changed files with 36 additions and 48 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
docs/public/tutorials/multi-model.mdx
View file

@ -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

2
docs/public/workflows/stages-and-nodes.mdx
View file

@ -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 |

1
docs/public/workflows/stylesheets.mdx
View file

@ -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

33
lib/components/fabro-graphviz/src/parser/semantic.rs
View file

@ -47,6 +47,15 @@ fn convert_attrs(block: &AttrBlock) -> HashMap<String, AttrValue> {
.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<Item = &str> {
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

28
lib/components/fabro-workflow/src/transforms/import.rs
View file

@ -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<String>, 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
}"#,

18
lib/foundation/fabro-types/src/graph.rs
View file

@ -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<Duration> {
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);