You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Graphs: aligned rows given with a design's art first stay rows of its stack - #7504
With aligned, the rows and effects a command names are rows of the first target's stack. #7320 left the art of a graph design in a series out of the targets before those rows were matched, so a command given that art first took its rows from the next object and edited a different row there. For example, with the art's stack [red fill, blue fill] and a mark's [blue fill, stroke], appearance.removeItem with index: 1 and aligned: true removed the mark's stroke instead of its blue fill.
appearance.setItem, appearance.removeItem, appearance.duplicateItem and the effect commands now match aligned rows and effects against the targets as given, the design's art included, and then leave the art out of the edit.
An aligned appearance.moveItem or effect.move is refused when the art's stack differs from the others, as is an aligned appearance.duplicateItem with to.
effect.apply reports the row the effect landed on in the object it edited.
Without aligned, an item the art lacks is still taken from the other objects' own stacks.
docs/mcp.md states the rule.
How it was tested
The head is 0986c6dab5, one commit on main at 1e4e3534f5; git merge-tree reports no conflict with main at 2a56e3395. It conflicts with #7368 in crates/engine/src/cmd/appearance.rs, crates/engine/src/cmd/effectcmd.rs and docs/mcp.md, and with #7412 in crates/engine/src/cmd/appearance.rs; the one that merges second keeps both sides. The commands below ran on Linux on the head.
aligned_rows_given_with_a_graph_designs_art_first_are_rows_of_its_stack in crates/engine/src/cmd/graph/tests.rs gives a mark the art's stack with a different fill below it, then, with the art first:
appearance.removeItem and appearance.setItem with aligned edit the mark's row equal to the art's fill, not the row at the same index;
effect.apply with aligned puts the effect on that row and reports it;
aligned appearance.moveItem, effect.move and appearance.duplicateItem with to are refused and add no undo step;
effect.apply without aligned and an item only the mark has edits the mark.
The test fails with each part of the change undone.
cargo fmt --all -- --check: clean for the changed files (crates/svg/src/lib.rs on main is reported).
No panics in shipped code: no unwrap(), expect(), panic!, unreachable!, todo! or unimplemented! outside tests, and no unchecked indexing on data from files, parameters or
messages. Errors are returned as Result and shown to the user
(why and how).
Clean-room: nothing copied from Adobe products or GPL/AGPL code (AGENTS.md).
Every new asset has a row in ASSETS.md.
Summary by CodeRabbit
Bug Fixes
Aligned appearance and effect operations now resolve row positions consistently when graph design artwork is part of a series.
Graph design artwork is excluded from aligned edits and reordering, while other selected objects receive the operation at the corresponding row.
Aligned moves and duplications are rejected when selected objects’ appearance rows cannot be matched.
Documentation
Clarified how appearance and effect commands behave when applied to graph design artwork, including cases that produce an error.
… stack
With `aligned`, the rows and effects a command names are rows of the
first target's stack. Since the art of a graph design in a series was
left out of the targets before those rows were matched, a command given
that art first took its rows from the next object instead and edited a
different row there.
- appearance.setItem, appearance.removeItem, appearance.duplicateItem
and the effect commands match aligned rows and effects against the
targets as given, the design's art included, then leave the art out
of the edit.
- An aligned appearance.moveItem or effect.move is refused when the
art's stack differs from the others, as is an aligned
appearance.duplicateItem with `to`.
- effect.apply reports the row the effect landed on in the object it
edited.
- Without `aligned`, an item the art lacks is still taken from the
other objects' own stacks.
docs/mcp.md states the rule.
Reviewing files that changed from the base of the PR and between 0986c6d and d17d6f1.
Files selected for processing: 4
crates/engine/src/cmd/appearance.rs
crates/engine/src/cmd/effectcmd.rs
crates/engine/src/cmd/graph/tests.rs
docs/mcp.md
No actionable comments were generated in the recent review.
Walkthrough
Aligned appearance and effect commands now resolve rows against targets that include graph-design art. They exclude graph-design art from edits, while checking alignment against the full target set. Regression tests and MCP documentation cover this behavior.
Appearance commands resolve aligned rows across all targets before filtering graph-design art from edits. Aligned moves check appearance differences across all targets.
Aligned effect targeting and validation crates/engine/src/cmd/effectcmd.rs, crates/engine/src/cmd/graph/tests.rs, docs/mcp.md
Effect commands use aligned roots and exclude graph-design art from edits. Effect results use mapped targets. Regression tests and documentation describe aligned behavior and its errors.
This change makes aligned appearance and effect commands resolve rows against a graph design's art first, so they no longer edit the wrong row on the next object. It includes a regression test and updated documentation. No merge-blocking risk was identified.
Pre-merge checks: 4
Check name
Status
Explanation
Title check
The title names the main change: aligned rows given with a graph design's art first are matched as rows of that design's stack. The changeset (appearance.rs, effectcmd.rs, graph tests, docs/mcp.md) im…
Description check
The description follows the template's required sections: 'What and why' explains the bug, the fix and the affected commands, and links the prior work (#7320). 'How it was tested' lists the regression…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Title check
Explanation
The title names the main change: aligned rows given with a graph design's art first are matched as rows of that design's stack. The changeset (appearance.rs, effectcmd.rs, graph tests, docs/mcp.md) implements exactly this behaviour, so the title is specific and accurate.
Full details: Description check
Explanation
The description follows the template's required sections: 'What and why' explains the bug, the fix and the affected commands, and links the prior work (#7320). 'How it was tested' lists the regression test, the commands run and their results. The checklist is present. Two items are unticked: the cargo xtask ci item is open and the description states it has not been checked. The other two checklist items are ticked. The template asks for asset rows in ASSETS.md, which the author ticks but the PR does not show new assets, so this is a minor point.
Finishing Touches
Generate unit tests (beta)
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Reviewing files that changed from the base of the PR and between 2e9c9ab and 0986c6d.
Files selected for processing: 4
crates/engine/src/cmd/appearance.rs
crates/engine/src/cmd/effectcmd.rs
crates/engine/src/cmd/graph/tests.rs
docs/mcp.md
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.
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
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.
What and why
Follows #7320.
With
aligned, the rows and effects a command names are rows of the first target's stack. #7320 left the art of a graph design in a series out of the targets before those rows were matched, so a command given that art first took its rows from the next object and edited a different row there. For example, with the art's stack[red fill, blue fill]and a mark's[blue fill, stroke],appearance.removeItemwithindex: 1andaligned: trueremoved the mark's stroke instead of its blue fill.appearance.setItem,appearance.removeItem,appearance.duplicateItemand the effect commands now match aligned rows and effects against the targets as given, the design's art included, and then leave the art out of the edit.appearance.moveItemoreffect.moveis refused when the art's stack differs from the others, as is an alignedappearance.duplicateItemwithto.effect.applyreports the row the effect landed on in the object it edited.aligned, an item the art lacks is still taken from the other objects' own stacks.docs/mcp.mdstates the rule.How it was tested
The head is
0986c6dab5, one commit onmainat1e4e3534f5;git merge-treereports no conflict withmainat2a56e3395. It conflicts with #7368 incrates/engine/src/cmd/appearance.rs,crates/engine/src/cmd/effectcmd.rsanddocs/mcp.md, and with #7412 incrates/engine/src/cmd/appearance.rs; the one that merges second keeps both sides. The commands below ran on Linux on the head.aligned_rows_given_with_a_graph_designs_art_first_are_rows_of_its_stackincrates/engine/src/cmd/graph/tests.rsgives a mark the art's stack with a different fill below it, then, with the art first:appearance.removeItemandappearance.setItemwithalignededit the mark's row equal to the art's fill, not the row at the same index;effect.applywithalignedputs the effect on that row and reports it;appearance.moveItem,effect.moveandappearance.duplicateItemwithtoare refused and add no undo step;effect.applywithoutalignedand an item only the mark has edits the mark.The test fails with each part of the change undone.
cargo fmt --all -- --check: clean for the changed files (crates/svg/src/lib.rsonmainis reported).cargo check --workspace --tests: clean.cargo clippy -p vectorcraft-engine --all-targets -- -D warnings: clean.cargo test -p vectorcraft-engine --lib -- graph::tests appearance effect: 552 passed.Checklist
cargo xtask cipasses (fmt, clippy, tests, assets, brands, layers, wasm).unwrap(),expect(),panic!,unreachable!,todo!orunimplemented!outside tests, and no unchecked indexing on data from files, parameters ormessages. Errors are returned as
Resultand shown to the user(why and how).
ASSETS.md.Summary by CodeRabbit
Bug Fixes
Documentation