refactor(cg): rename ChangeKind::Geometry → Layout; add grida-dev mutation bench#670
Merged
softmarshmallow merged 2 commits intomainfrom Apr 20, 2026
Merged
refactor(cg): rename ChangeKind::Geometry → Layout; add grida-dev mutation bench#670softmarshmallow merged 2 commits intomainfrom
softmarshmallow merged 2 commits intomainfrom
Conversation
…ation bench Rename (cg/runtime/invalidation) Motion changes — leaf `AffineTransform`, Container/Tray `position + rotation` — route through `ChangeKind::Layout` instead of `Geometry`. In the current node schema a leaf's `AffineTransform` IS its layout-placement field (leaves lack a separate `position` slot the way `Container`/`Tray` have), so a motion change on a leaf IS a layout change. The new name documents the actual semantic. Splitting layout-position from post-layout transform on leaves is follow-up schema work. Tooling (grida-dev) `bench --mutation <kind>` unified runner covering translate-root / translate-leaf / resize / paint / delete / all. Measures real-world mutation scenarios end-to-end through `apply_changes`; reports per-frame `apply_changes` percentiles separately from total frame time. Pre-picks sensible targets per kind (first root, deepest leaf, first paintable/resizable leaf). Adds `MutationCommand::SetFill` and `MutationCommand::Delete` to the editor mutation API so the bench covers paint and structural deletes alongside the existing Translate and Resize.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
WalkthroughThe PR renames the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9958a52628
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+216
to
+218
| if matches!(scene.graph.get_node(&id), Ok(Node::InitialContainer(_))) { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
find_leaf_from returns early for InitialContainer, so pick_leaf_target never explores that root's children. On scenes where the root is an InitialContainer, --mutation translate-leaf (and --mutation all) fails with "no leaf target" even though valid leaf descendants exist.
Useful? React with 👍 / 👎.
Comment on lines
+304
to
+306
| if matches!(scene.graph.get_node(&id), Ok(Node::InitialContainer(_))) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
In collect_leaves, hitting an InitialContainer immediately continues, which skips traversing its subtree. If the scene root is an InitialContainer, delete target collection becomes empty and run_delete_pass returns zeroed stats without running mutations, producing misleading delete benchmark output.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
crates/grida-dev/src/bench/args.rs (1)
48-61: Considerclap::ValueEnumfor parse-time validation of mutation kinds.Currently, the mutation kind is accepted as
Option<String>and validated at runtime inrun_mutation_kindafter the scene loads. AValueEnumwould catch typos (e.g.,--mutation transalte-root) at parse time, automatically generatepossible_valuesin--help, and ensure compile-time exhaustiveness against the dispatch logic.Optional refactor
+#[derive(clap::ValueEnum, Debug, Clone, Copy)] +pub enum MutationKind { + TranslateRoot, + TranslateLeaf, + Resize, + Paint, + Delete, + All, +} ... - #[arg(long = "mutation")] - pub mutation: Option<String>, + #[arg(long = "mutation", value_enum)] + pub mutation: Option<MutationKind>,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-dev/src/bench/args.rs` around lines 48 - 61, Change the mutation flag from Option<String> to an Option<ValueEnum> so clap validates variants at parse time: define an enum MutationKind (derive(Clone, Debug, clap::ValueEnum)) with variants for translate_root, translate_leaf, resize, paint, delete, all (use #[clap(rename_all = "kebab-case")] or per-variant #[clap(rename = "...")] to preserve the CLI names like "translate-root"), change the struct field pub mutation: Option<MutationKind>, and update the dispatch in run_mutation_kind to match on MutationKind instead of stringly-typed values (adjust any helper signatures that currently take &str/String to take MutationKind or convert as needed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/grida-dev/src/bench/runner.rs`:
- Around line 243-268: pick_paint_target is returning paint-capable parents
because it returns as soon as a paintable Node variant is matched; change it so
that after matching a paintable node (in pick_paint_target) you check whether it
is a leaf (no children) via scene.graph.get_children(&id).map_or(true, |c|
c.is_empty()) and only return Some(id) when that is true; if the node has
children, do not return—continue by pushing/iterating its children as currently
implemented. Ensure you still ignore InitialContainer and preserve the DFS
order.
- Around line 214-230: The traversal incorrectly treats InitialContainer nodes
as terminal and skips their subtrees; update find_leaf_from (and the similar
logic in collect_leaves) to not return/stop when scene.graph.get_node(&id)
matches Node::InitialContainer(_), but instead continue normal traversal into
its children (i.e., remove or change the early return for InitialContainer so
you recurse into get_children(&id)); ensure you only return None for
missing/error nodes and that InitialContainer is not treated as a leaf so
frames==0 and translate-leaf properly find and act on descendant nodes.
In `@crates/grida-dev/src/editor/mutation.rs`:
- Around line 83-92: The Delete arm currently uses unwrap_or(*id) so deleting a
root emits a stale (removed) id with ChangeKind::Full; change the
MutationCommand::Delete handling: call scene.graph.get_parent(id) and if it
returns None (root) then after scene.graph.remove_subtree(*id) return an empty
vec and emit a scene-wide/global change flag instead of ( *id, ChangeKind::Full
) — otherwise, for non-root parents keep the existing behavior; update the match
arm around get_parent, remove_subtree and the ChangeKind usage (or alternatively
add a clear comment next to the unwrap_or(*id) explaining the intentional
stale-id-for-Full contract if you prefer documenting rather than changing
behavior).
- Around line 275-328: The function set_fill_solid currently treats TextSpan as
having fills ignored and silently skips Image nodes; update it so TextSpan
behavior is acknowledged (it is valid to set n.fills on TextSpan because fills
participate in rendering) and either handle Image by converting the solid Paint
into an ImagePaint::Tint and assigning it to ImageNodeRec.fill, or add a clear
doc comment on set_fill_solid/SetFill (and note MutationCommand::SetFill)
stating images are intentionally unsupported and why (ImageNodeRec stores a
single ImagePaint). Locate these changes in set_fill_solid, the TextSpan match
arm, and the Image match arm / ImageNodeRec handling and implement the
conversion to ImagePaint::Tint or add the explicit comment accordingly.
---
Nitpick comments:
In `@crates/grida-dev/src/bench/args.rs`:
- Around line 48-61: Change the mutation flag from Option<String> to an
Option<ValueEnum> so clap validates variants at parse time: define an enum
MutationKind (derive(Clone, Debug, clap::ValueEnum)) with variants for
translate_root, translate_leaf, resize, paint, delete, all (use
#[clap(rename_all = "kebab-case")] or per-variant #[clap(rename = "...")] to
preserve the CLI names like "translate-root"), change the struct field pub
mutation: Option<MutationKind>, and update the dispatch in run_mutation_kind to
match on MutationKind instead of stringly-typed values (adjust any helper
signatures that currently take &str/String to take MutationKind or convert as
needed).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 89fca92f-6e28-4739-b0db-86bcb06d48b3
📒 Files selected for processing (9)
crates/grida-canvas/src/runtime/invalidation/change_kind.rscrates/grida-canvas/src/runtime/invalidation/differ.rscrates/grida-canvas/src/runtime/invalidation/lens.rscrates/grida-canvas/src/runtime/invalidation/scene_dirty.rscrates/grida-canvas/src/runtime/scene.rscrates/grida-dev/src/bench/args.rscrates/grida-dev/src/bench/report.rscrates/grida-dev/src/bench/runner.rscrates/grida-dev/src/editor/mutation.rs
Comment on lines
+214
to
+230
| fn find_leaf_from(scene: &Scene, id: cg::node::schema::NodeId) -> Option<cg::node::schema::NodeId> { | ||
| use cg::node::schema::Node; | ||
| if matches!(scene.graph.get_node(&id), Ok(Node::InitialContainer(_))) { | ||
| return None; | ||
| } | ||
| if let Some(children) = scene.graph.get_children(&id) { | ||
| if children.is_empty() { | ||
| return Some(id); | ||
| } | ||
| for c in children { | ||
| if let Some(leaf) = find_leaf_from(scene, *c) { | ||
| return Some(leaf); | ||
| } | ||
| } | ||
| return Some(id); | ||
| } | ||
| Some(id) |
There was a problem hiding this comment.
Traverse through InitialContainer instead of dropping its subtree.
Both helpers skip InitialContainer by returning/continuing before visiting its children. If a loaded scene roots real content under an InitialContainer, translate-leaf reports no target and delete returns zero stats despite valid descendants. collect_leaves also deletes one node for frames == 0.
🐛 Proposed fix
fn find_leaf_from(scene: &Scene, id: cg::node::schema::NodeId) -> Option<cg::node::schema::NodeId> {
use cg::node::schema::Node;
if matches!(scene.graph.get_node(&id), Ok(Node::InitialContainer(_))) {
- return None;
+ return scene
+ .graph
+ .get_children(&id)
+ .and_then(|children| children.iter().find_map(|c| find_leaf_from(scene, *c)));
}
if let Some(children) = scene.graph.get_children(&id) {
if children.is_empty() {
return Some(id);
}
@@
fn collect_leaves(scene: &Scene, n: usize) -> Vec<cg::node::schema::NodeId> {
use cg::node::schema::Node;
+ if n == 0 {
+ return Vec::new();
+ }
+
let mut out = Vec::with_capacity(n);
let mut stack: Vec<cg::node::schema::NodeId> = scene.graph.roots().to_vec();
stack.reverse();
while let Some(id) = stack.pop() {
- if matches!(scene.graph.get_node(&id), Ok(Node::InitialContainer(_))) {
- continue;
- }
+ let skip_self = matches!(scene.graph.get_node(&id), Ok(Node::InitialContainer(_)));
match scene.graph.get_children(&id) {
- Some(children) if !children.is_empty() => {
+ Some(children) if skip_self || !children.is_empty() => {
for c in children.iter().rev() {
stack.push(*c);
}
}
- _ => {
+ _ if !skip_self => {
out.push(id);
if out.len() >= n {
break;
}
}
+ _ => {}
}
}
out
}Also applies to: 298-322
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-dev/src/bench/runner.rs` around lines 214 - 230, The traversal
incorrectly treats InitialContainer nodes as terminal and skips their subtrees;
update find_leaf_from (and the similar logic in collect_leaves) to not
return/stop when scene.graph.get_node(&id) matches Node::InitialContainer(_),
but instead continue normal traversal into its children (i.e., remove or change
the early return for InitialContainer so you recurse into get_children(&id));
ensure you only return None for missing/error nodes and that InitialContainer is
not treated as a leaf so frames==0 and translate-leaf properly find and act on
descendant nodes.
Comment on lines
+243
to
+268
| /// Pick a leaf that supports paint (fills) via `MutationCommand::SetFill`. | ||
| fn pick_paint_target(scene: &Scene) -> Option<cg::node::schema::NodeId> { | ||
| use cg::node::schema::Node; | ||
| // DFS iteratively, pick the first leaf with fills. | ||
| let mut stack: Vec<cg::node::schema::NodeId> = scene.graph.roots().to_vec(); | ||
| while let Some(id) = stack.pop() { | ||
| match scene.graph.get_node(&id) { | ||
| Ok(Node::InitialContainer(_)) => {} | ||
| Ok(Node::Rectangle(_)) | ||
| | Ok(Node::Ellipse(_)) | ||
| | Ok(Node::RegularPolygon(_)) | ||
| | Ok(Node::RegularStarPolygon(_)) | ||
| | Ok(Node::TextSpan(_)) | ||
| | Ok(Node::Path(_)) | ||
| | Ok(Node::Polygon(_)) | ||
| | Ok(Node::Vector(_)) => return Some(id), | ||
| _ => {} | ||
| } | ||
| if let Some(children) = scene.graph.get_children(&id) { | ||
| for c in children.iter().rev() { | ||
| stack.push(*c); | ||
| } | ||
| } | ||
| } | ||
| None | ||
| } |
There was a problem hiding this comment.
Only return paint targets after confirming they are leaves.
pick_paint_target currently returns the first paint-capable node before checking children, so a paintable parent can be selected even though the benchmark says it targets a leaf.
🎯 Proposed fix
let mut stack: Vec<cg::node::schema::NodeId> = scene.graph.roots().to_vec();
while let Some(id) = stack.pop() {
+ let children = scene.graph.get_children(&id);
+ let is_leaf = children.is_none_or(|children| children.is_empty());
+
match scene.graph.get_node(&id) {
Ok(Node::InitialContainer(_)) => {}
Ok(Node::Rectangle(_))
| Ok(Node::Ellipse(_))
| Ok(Node::RegularPolygon(_))
| Ok(Node::RegularStarPolygon(_))
| Ok(Node::TextSpan(_))
| Ok(Node::Path(_))
| Ok(Node::Polygon(_))
- | Ok(Node::Vector(_)) => return Some(id),
+ | Ok(Node::Vector(_))
+ if is_leaf =>
+ {
+ return Some(id);
+ }
_ => {}
}
- if let Some(children) = scene.graph.get_children(&id) {
+ if let Some(children) = children {
for c in children.iter().rev() {
stack.push(*c);
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Pick a leaf that supports paint (fills) via `MutationCommand::SetFill`. | |
| fn pick_paint_target(scene: &Scene) -> Option<cg::node::schema::NodeId> { | |
| use cg::node::schema::Node; | |
| // DFS iteratively, pick the first leaf with fills. | |
| let mut stack: Vec<cg::node::schema::NodeId> = scene.graph.roots().to_vec(); | |
| while let Some(id) = stack.pop() { | |
| match scene.graph.get_node(&id) { | |
| Ok(Node::InitialContainer(_)) => {} | |
| Ok(Node::Rectangle(_)) | |
| | Ok(Node::Ellipse(_)) | |
| | Ok(Node::RegularPolygon(_)) | |
| | Ok(Node::RegularStarPolygon(_)) | |
| | Ok(Node::TextSpan(_)) | |
| | Ok(Node::Path(_)) | |
| | Ok(Node::Polygon(_)) | |
| | Ok(Node::Vector(_)) => return Some(id), | |
| _ => {} | |
| } | |
| if let Some(children) = scene.graph.get_children(&id) { | |
| for c in children.iter().rev() { | |
| stack.push(*c); | |
| } | |
| } | |
| } | |
| None | |
| } | |
| /// Pick a leaf that supports paint (fills) via `MutationCommand::SetFill`. | |
| fn pick_paint_target(scene: &Scene) -> Option<cg::node::schema::NodeId> { | |
| use cg::node::schema::Node; | |
| // DFS iteratively, pick the first leaf with fills. | |
| let mut stack: Vec<cg::node::schema::NodeId> = scene.graph.roots().to_vec(); | |
| while let Some(id) = stack.pop() { | |
| let children = scene.graph.get_children(&id); | |
| let is_leaf = children.is_none_or(|children| children.is_empty()); | |
| match scene.graph.get_node(&id) { | |
| Ok(Node::InitialContainer(_)) => {} | |
| Ok(Node::Rectangle(_)) | |
| | Ok(Node::Ellipse(_)) | |
| | Ok(Node::RegularPolygon(_)) | |
| | Ok(Node::RegularStarPolygon(_)) | |
| | Ok(Node::TextSpan(_)) | |
| | Ok(Node::Path(_)) | |
| | Ok(Node::Polygon(_)) | |
| | Ok(Node::Vector(_)) | |
| if is_leaf => | |
| { | |
| return Some(id); | |
| } | |
| _ => {} | |
| } | |
| if let Some(children) = children { | |
| for c in children.iter().rev() { | |
| stack.push(*c); | |
| } | |
| } | |
| } | |
| None | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-dev/src/bench/runner.rs` around lines 243 - 268,
pick_paint_target is returning paint-capable parents because it returns as soon
as a paintable Node variant is matched; change it so that after matching a
paintable node (in pick_paint_target) you check whether it is a leaf (no
children) via scene.graph.get_children(&id).map_or(true, |c| c.is_empty()) and
only return Some(id) when that is true; if the node has children, do not
return—continue by pushing/iterating its children as currently implemented.
Ensure you still ignore InitialContainer and preserve the DFS order.
Comment on lines
+83
to
+92
| MutationCommand::Delete { id } => { | ||
| // Report the parent (or `*id` for a root) with `Full` | ||
| // so the renderer full-rebuilds. The cg crate currently | ||
| // has no structural-remove fast path. | ||
| let parent = scene.graph.get_parent(id).unwrap_or(*id); | ||
| match scene.graph.remove_subtree(*id) { | ||
| Ok(_) => vec![(parent, ChangeKind::Full)], | ||
| Err(_) => Vec::new(), | ||
| } | ||
| } |
There was a problem hiding this comment.
Delete on a root node reports a stale id with ChangeKind::Full.
For a root, get_parent(id) returns Err/None, so the fallback reports (*id, Full) after remove_subtree(*id) — i.e. the reported node no longer exists in the graph. In practice this is safe because Full routes to rebuild_scene_caches() (a full rebuild from the current scene state) and the per-node invalidate_node / compositor.invalidate / atlas.free_node calls are tolerant of unknown ids, but emitting a change keyed on a removed id is easy to misread.
Consider returning an empty vec + a scene-wide GlobalFlag for root deletes, or at minimum documenting the "stale id is intentional for Full" contract next to the unwrap_or(*id).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-dev/src/editor/mutation.rs` around lines 83 - 92, The Delete arm
currently uses unwrap_or(*id) so deleting a root emits a stale (removed) id with
ChangeKind::Full; change the MutationCommand::Delete handling: call
scene.graph.get_parent(id) and if it returns None (root) then after
scene.graph.remove_subtree(*id) return an empty vec and emit a scene-wide/global
change flag instead of ( *id, ChangeKind::Full ) — otherwise, for non-root
parents keep the existing behavior; update the match arm around get_parent,
remove_subtree and the ChangeKind usage (or alternatively add a clear comment
next to the unwrap_or(*id) explaining the intentional stale-id-for-Full contract
if you prefer documenting rather than changing behavior).
Comment on lines
+275
to
+328
| /// Overwrite the node's fills with a single solid paint, if the node | ||
| /// supports fills. Returns `true` when the mutation landed. Emits | ||
| /// `ChangeKind::Paint` via `MutationCommand::SetFill`. | ||
| fn set_fill_solid(graph: &mut SceneGraph, id: &NodeId, color: CGColor) -> bool { | ||
| let Ok(node) = graph.get_node_mut(id) else { | ||
| return false; | ||
| }; | ||
| let paint = Paint::Solid(SolidPaint::new_color(color)); | ||
| match node { | ||
| Node::Rectangle(n) => { | ||
| n.fills = Paints::new([paint]); | ||
| true | ||
| } | ||
| Node::Ellipse(n) => { | ||
| n.fills = Paints::new([paint]); | ||
| true | ||
| } | ||
| Node::RegularPolygon(n) => { | ||
| n.fills = Paints::new([paint]); | ||
| true | ||
| } | ||
| Node::RegularStarPolygon(n) => { | ||
| n.fills = Paints::new([paint]); | ||
| true | ||
| } | ||
| Node::Line(_) => false, // lines use stroke, not fill | ||
| Node::TextSpan(n) => { | ||
| n.fills = Paints::new([paint]); | ||
| true | ||
| } | ||
| Node::AttributedText(_) => false, // per-span fills; out of scope here | ||
| Node::Path(n) => { | ||
| n.fills = Paints::new([paint]); | ||
| true | ||
| } | ||
| Node::Polygon(n) => { | ||
| n.fills = Paints::new([paint]); | ||
| true | ||
| } | ||
| Node::Vector(n) => { | ||
| n.fills = Paints::new([paint]); | ||
| true | ||
| } | ||
| Node::Container(n) => { | ||
| n.fills = Paints::new([paint]); | ||
| true | ||
| } | ||
| Node::Tray(n) => { | ||
| n.fills = Paints::new([paint]); | ||
| true | ||
| } | ||
| _ => false, | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify TextSpan fills are consumed by the painter / layer builder,
# and inspect ImageNodeRec's fill shape to confirm the Image branch decision.
rg -nP --type=rust -C3 '\bTextSpanNodeRec\b'
rg -nP --type=rust -C3 '\bfills\b' -g 'crates/grida-canvas/src/painter/**'
rg -nP --type=rust -C3 '\bImageNodeRec\b' -g 'crates/grida-canvas/src/node/**'
rg -nP --type=rust -C5 '\brebuild_layer_vec\b'Repository: gridaco/grida
Length of output: 50370
🏁 Script executed:
#!/bin/bash
# Find TextSpanNodeRec struct definition with fills field
echo "=== TextSpanNodeRec struct definition ==="
rg -nA 20 'pub struct TextSpanNodeRec' crates/grida-canvas/src/node/schema.rs
echo -e "\n=== Search for rebuild_layer_vec ==="
rg -nB 2 -A 10 'fn rebuild_layer_vec' crates/grida-canvas/src/painter/
echo -e "\n=== ImageNodeRec struct definition ==="
rg -nA 15 'pub struct ImageNodeRec' crates/grida-canvas/src/node/schema.rs
echo -e "\n=== lens::paint_of implementation ==="
rg -nB 2 -A 10 'pub fn paint_of|fn paint_of' crates/grida-canvas/src/Repository: gridaco/grida
Length of output: 2926
🏁 Script executed:
#!/bin/bash
# Get complete TextSpanNodeRec struct with fills field
echo "=== TextSpanNodeRec with fills field ==="
rg -nA 50 'pub struct TextSpanNodeRec' crates/grida-canvas/src/node/schema.rs | head -80
echo -e "\n=== lens::paint_of complete implementation ==="
rg -nA 60 'pub fn paint_of' crates/grida-canvas/src/runtime/invalidation/lens.rs | grep -A 60 "pub fn paint_of"
echo -e "\n=== Search for text layer building (PainterTextLayer, rebuild, build) ==="
rg -n 'PainterTextLayer|fn.*text.*layer|Node::TextSpan.*=>' crates/grida-canvas/src/painter/layer.rs | head -20Repository: gridaco/grida
Length of output: 4940
🏁 Script executed:
#!/bin/bash
# Find fills field in TextSpanNodeRec
echo "=== TextSpanNodeRec including fills ==="
rg -n 'pub fills:' crates/grida-canvas/src/node/schema.rs | grep -A 2 -B 2 2
# Search for TextSpan in lens::paint_of
echo -e "\n=== TextSpan and Image in lens::paint_of ==="
rg -n 'Node::TextSpan|Node::Image|Node::AttributedText' crates/grida-canvas/src/runtime/invalidation/lens.rs
# Get the complete TextSpan match in layer.rs
echo -e "\n=== TextSpan handling in layer.rs ==="
sed -n '1293,1400p' crates/grida-canvas/src/painter/layer.rsRepository: gridaco/grida
Length of output: 5708
🏁 Script executed:
#!/bin/bash
# Get complete lens::paint_of for TextSpan and Image
echo "=== Complete lens::paint_of for TextSpan and Image cases ==="
sed -n '207,230p' crates/grida-canvas/src/runtime/invalidation/lens.rs
# Look for text painting logic that actually uses fills
echo -e "\n=== Text painting that reads fills ==="
rg -n 'paint.*text|draw.*text.*fill' crates/grida-canvas/src/painter/ | grep -i fill | head -15
# Check Image in lens::paint_of more carefully
echo -e "\n=== Image case in lens::paint_of ==="
sed -n '207,215p' crates/grida-canvas/src/runtime/invalidation/lens.rs
# Look for where PainterPictureTextLayer is rendered/consumed
echo -e "\n=== Where PainterPictureTextLayer.fills is used ==="
rg -n 'PainterPictureTextLayer|PainterTextLayer' crates/grida-canvas/src/painter/ | head -25Repository: gridaco/grida
Length of output: 2276
🏁 Script executed:
#!/bin/bash
# Find where PainterPictureTextLayer.fills is actually consumed/rendered
echo "=== Rendering of PainterPictureTextLayer - check painter.rs ==="
rg -n 'PainterPictureTextLayer|Text\(' crates/grida-canvas/src/painter/painter.rs | head -20
# Search for render/draw command handling in painter
echo -e "\n=== How PainterPictureLayer::Text is rendered ==="
rg -n 'PainterPictureLayer::Text|Text(' crates/grida-canvas/src/painter/painter.rs | grep -i 'match\|render\|draw' -A 2
# Look at the actual text rendering code
echo -e "\n=== Text rendering in painter.rs ==="
rg -n 'fn render_text|fn draw_text' crates/grida-canvas/src/painter/painter.rs
# Check if fills are used in text rendering
echo -e "\n=== Text layer fields used in rendering ==="
sed -n '1800,1900p' crates/grida-canvas/src/painter/painter.rs | grep -i 'fills\|strokes'Repository: gridaco/grida
Length of output: 973
🏁 Script executed:
#!/bin/bash
# Get context around text rendering that uses fills
echo "=== Text rendering consuming fills ==="
sed -n '1730,1900p' crates/grida-canvas/src/painter/painter.rs
# Also check the second occurrence at line 2371
echo -e "\n=== Second PainterPictureLayer::Text handling ==="
sed -n '2365,2400p' crates/grida-canvas/src/painter/painter.rsRepository: gridaco/grida
Length of output: 10901
TextSpan fills participates in rendering; clarify SetFill Image handling and document the constraint.
The TextSpan concern should be dismissed: TextSpanNodeRec::fills actively feeds into both the attributed-text and uniform-text rendering paths (painter.rs:1799, 1876). The lens returns fills: None not because fills are ignored, but because text paint changes also affect layout metrics, so the invalidation system treats the whole node conservatively rather than tracking paint changes separately. However, the command silently succeeds with a side effect on the data model.
For Image: ImageNodeRec intentionally stores a single fill: ImagePaint (not Paints) to align with web patterns. SetFill falls through to _ => false and silently skips images. Either add an Image branch that converts the solid color to ImagePaint::Tint, or add a doc comment on SetFill explicitly noting that images are unsupported and why.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-dev/src/editor/mutation.rs` around lines 275 - 328, The function
set_fill_solid currently treats TextSpan as having fills ignored and silently
skips Image nodes; update it so TextSpan behavior is acknowledged (it is valid
to set n.fills on TextSpan because fills participate in rendering) and either
handle Image by converting the solid Paint into an ImagePaint::Tint and
assigning it to ImageNodeRec.fill, or add a clear doc comment on
set_fill_solid/SetFill (and note MutationCommand::SetFill) stating images are
intentionally unsupported and why (ImageNodeRec stores a single ImagePaint).
Locate these changes in set_fill_solid, the TextSpan match arm, and the Image
match arm / ImageNodeRec handling and implement the conversion to
ImagePaint::Tint or add the explicit comment accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ChangeKind::Geometry→ChangeKind::Layout(cg/runtime/invalidation). Motion changes — leafAffineTransform, Container/Trayposition + rotation— now route throughChangeKind::Layout. In the current node schema a leaf'sAffineTransformIS its layout-placement field (leaves lack a separatepositionslot the wayContainer/Trayhave), so a motion change on a leaf IS a layout change. The new name documents the actual semantic. Splitting layout-position from post-layout transform on leaves is follow-up schema work.bench --mutation <kind>to grida-dev coveringtranslate-root | translate-leaf | resize | paint | delete | all. Measures real-world mutation scenarios end-to-end throughapply_changes; reports per-frameapply_changespercentiles separately from total frame time. Pre-picks sensible targets per kind (first root, deepest leaf, first paintable/resizable leaf).MutationCommand::SetFillandMutationCommand::Deleteto the editor mutation API so the bench covers paint and structural deletes alongside the existing Translate and Resize.This is a checkpoint landing the measurement tooling + the terminology cleanup. Follow-up session will redesign around Layout as a first-class dirty-flaggable lifecycle stage (Taffy
mark_dirtyintegration), starting with the node-schema fix described in the rename's commit message.Also includes the pre-existing
chore: bump next to 16.2.4already on canary.Test plan
cargo test -p cg --release --lib— 670 tests passcargo check -p cg -p grida-devcleancargo fmt+ clippy via lefthook pre-commitcargo run -p grida-dev --release -- bench ./fixtures/test-grida/bench.grida --scene 0 --mutation all --frames 15— runs clean, produces sensible p50s (paint ~8ms, resize ~19ms, delete ~19ms, translate-leaf ~140µs, translate-root ~130µs on flat-grid 10K)Summary by CodeRabbit
New Features
--mutationflag, enabling performance testing for translate, resize, paint, and delete operations.SetFillfor color operations andDeletefor node removal, expanding testing capabilities.Chores