Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the node graph's metadata handling by removing the Upstream generic from the Vector type and replacing the hardcoded upstream_data field with a flexible attribute-based system. It introduces editor:layer and editor:merged_layers attributes to manage layer identities and merged content for click-target preservation. The source_node_id node is replaced by parent_layer and a new write_attribute node, which enables per-row attribute assignment. Feedback points out a potential performance bottleneck in the write_attribute node due to per-row allocations and clones, suggesting optimization for cases where the attribute value is constant across all rows.
| for index in 0..content.len() { | ||
| let row = content.clone_row(index).expect("index is within bounds"); | ||
| let owned_ctx = OwnedContextImpl::from(ctx.clone()).with_vararg(Box::new(Table::new_from_row(row))).with_index(index); | ||
| let v = value.eval(owned_ctx.into_context()).await; | ||
| content.set_attribute(&name, index, v); | ||
| } |
There was a problem hiding this comment.
The write_attribute node evaluates the value node for every row in the table. For each iteration, it performs several allocations and clones: content.clone_row(index), Table::new_from_row(row), and OwnedContextImpl::from(...).
While this provides flexibility for per-row attribute calculation, it may become a performance bottleneck for tables with a large number of rows (e.g., complex vector paths). If the value node is constant for all rows (like the parent_layer node used in the default definitions), this overhead is redundant.
Consider if the context can be partially reused or if a more efficient way to apply constant attributes can be implemented.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 27 files
Confidence score: 3/5
- There is a meaningful regression risk in
node-graph/libraries/graphic-types/src/graphic.rs: legacyGraphicmigration appears to drop serialized transform/alpha values, which can change visual appearance when older documents are opened. - Given the 7/10 severity and 8/10 confidence, this looks user-facing rather than a minor housekeeping concern, so merge risk is moderate until migration behavior is confirmed.
- Pay close attention to
node-graph/libraries/graphic-types/src/graphic.rs- migration logic may lose transform/alpha data from legacy serialized graphics.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="node-graph/libraries/graphic-types/src/graphic.rs">
<violation number="1" location="node-graph/libraries/graphic-types/src/graphic.rs:510">
P1: Legacy `Graphic` migration drops serialized transform/alpha values, which can alter appearance when opening older documents.
(Based on your team's feedback about validating real data-loss risk in format migrations.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…:merged_layers", backed by a new 'Write Attribute' node (#4061) * Fix click target propagation with the Rasterize node * Add the 'Write Attribute' node * Remove tag_layer in favor of the new Write Attribute node, prune redundant attribute writes * Replace the Vector<Upstream> type argument with the "editor:merged_layers" attribute
…:merged_layers", backed by a new 'Write Attribute' node (#4061) * Fix click target propagation with the Rasterize node * Add the 'Write Attribute' node * Remove tag_layer in favor of the new Write Attribute node, prune redundant attribute writes * Replace the Vector<Upstream> type argument with the "editor:merged_layers" attribute
…:merged_layers", backed by a new 'Write Attribute' node (GraphiteEditor#4061) * Fix click target propagation with the Rasterize node * Add the 'Write Attribute' node * Remove tag_layer in favor of the new Write Attribute node, prune redundant attribute writes * Replace the Vector<Upstream> type argument with the "editor:merged_layers" attribute
1 participant
Replaces two pieces of bespoke attribute machinery with the dynamic attribute system, by way of a new general-purpose 'Write Attribute' proto node:
source_node_idproto + attribute →editor:layerattribute. The Merge and Artboard layer subgraphs no longer call a specialsource_node_idproto to stamp the owning layer'sNodeIdonto each row. They now wire a 'Parent Layer' proto node (which derives theNodeIdfrom the document path) intowrite_attribute("editor:layer", …). Per-row evaluation attribute stamping spread acrossgraphic-typesandraster-typesis deleted alongside.Vector<Upstream>generic +merged_layersfield →editor:merged_layersattribute.Vectorloses its type parameter and themerged_layers: Option<Table<Graphic>>field. The upstream-content snapshot used by 'Boolean Operation', 'Flatten Path', and 'Morph' now lives on the row as theeditor:merged_layersattribute, written via the samewrite_attributemechanism. The Rasterize node also writes this attribute (snapshotting its destructively-merged input), giving the renderer a path to recurse through and surface the original layers' click targets — the same routing 'Boolean Operation' already used.The new 'Write Attribute' node is a generic
(table, name, value-node) → tableprimitive: walks each row, evaluates the value node with the row's index in context, and writes the result to the named attribute column. Generic across the standard element types and value types.Partly closes #3779.