Skip to content

Undo groups: library styles and field scrubs close only the group they open - #3287

Open
dotCooCoo wants to merge 1 commit into
storytold:mainfrom
dotCooCoo:fix/undo-groups-close-only-their-document
Open

dotCooCoo wants to merge 1 commit into
storytold:mainfrom
dotCooCoo:fix/undo-groups-close-only-their-document

Conversation

@dotCooCoo

@dotCooCoo dotCooCoo commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

What and why

Using a library's character or paragraph style (library.use) and scrubbing a numeric field each make one undo step by opening an undo group in the active document. Both closed it with Session::end_undo_group, which closes the open undo group of every document. So an undo group open in another document, such as one begun with edit.undoGroup.begin for an MCP command_batch with atomic, was closed too, and that document's next edits became undo steps of their own. When the style failed to apply, or Escape ended the scrub, the other document's group was canceled as well, and the edits made in it were undone. Inside an undo group already open in the same document, a scrub closed that group, Escape undid the edits made in it before the drag, and a style that failed to apply left the style it had added.

  • Session::begin_undo_scope and Session::end_undo_scope make the edits in between one undo step of the active document. The scope opens an undo group there, or joins the group already open, so that its edits are part of that group's step.
  • Ending a scope closes only the group it opened, in its own document, even when another document became active in the meantime. A canceled scope undoes only its own edits, also inside a group that was already open. edit.undoGroup.end (end_undo_group) still closes the open group of every document.
  • library.use with a character or paragraph style, and a field scrub, use the scope. Escape during a scrub inside an open group puts the document back as it was before the drag, also for the Transform panel's Shear field, whose value rests at 0°.
  • The Paragraph and Character panels' Reset Panel get an undo group in Paragraph: Reset Panel also resets the Mojikumi Set and tab stops #2731 and Character: Anti-aliasing Method sets how a type object's edges are drawn as pixels #2832, and there it closes only the active document's group. This change does not edit those panels.

How it was tested

New tests, each failing on main:

  • tests_journal::an_undo_scope_closes_only_its_own_documents_group and an_undo_scope_in_an_open_group_takes_back_only_its_own_edits (vectorcraft-engine) end and cancel scopes with groups open in two documents, once while the other document is active, and inside an open group before and after its first edit, checking the groups, the undo steps and the journal. They do not build on main, which has no undo scopes.
  • tests_library::a_library_style_leaves_another_documents_undo_group_open (vectorcraft-engine) uses a style in one document and draws in the other, whose edit must join its open group's step.
  • tests_library::a_library_style_that_fails_takes_back_only_its_own_edits and a_library_style_that_fails_in_an_open_group_takes_back_its_own_edits (vectorcraft-engine) apply a paragraph style with nothing selected, in a second document and inside the group's own document. The style it added is taken back, and the group stays open with the rectangle drawn in it.
  • tests_scrub::a_scrub_leaves_another_documents_undo_group_open, a_scrub_inside_an_open_undo_group_joins_its_step and a_scrub_closes_its_group_in_its_own_document_after_a_switch (vectorcraft-ui-egui) scrub W with Escape and with a release while another document has an open group, inside the document's own open group, and with another document made active during the drag.

Targeted checks on the final tree: cargo clippy -p <crate> --all-targets -- -D warnings on vectorcraft-engine and vectorcraft-ui-egui; the new tests and the existing tests_scrub, tests_library and an_undo_group_is_one_undo_step tests; cargo fmt --all -- --check; cargo xtask brands, layers and assets.

On Windows 11 (x86-64) with rustc 1.99.1, on top of main at bc1ae082:

  • CRAFT_FONTS_DIR=<craft-fonts checkout> cargo xtask ci passes its 10 steps: 8,578 tests passed, 0 failed, 13 ignored.

git merge-tree reports no conflicts with main at bc1ae082.

Checklist

  • cargo xtask ci passes (fmt, clippy, tests, assets, brands, layers, wasm).
  • 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

    • Improved undo handling when editing or scrubbing across multiple documents.
    • Failed operations now roll back only their own changes without disrupting existing undo history.
    • Undo groups remain associated with the document where they began, even when switching documents.
    • Scrub cancellations reliably undo the current drag while preserving earlier edits.
  • Tests

    • Added coverage for multi-document undo behavior, grouped edits, failed style applications, and scrub operations.

…y open

Using a library's character or paragraph style, and scrubbing a
numeric field, closed their undo step with Session::end_undo_group,
which closes the open undo group of every document. An undo group open
in another document, such as one begun with edit.undoGroup.begin for
an MCP command_batch with atomic, was closed too, and that document's
next edits became undo steps of their own. When the style failed to
apply, or Escape ended the scrub, the other document's group was
canceled as well and the edits made in it were undone. Inside an undo
group already open in the same document, a scrub closed that group,
Escape undid the edits made in it before the drag, and a style that
failed to apply left the style it had added.

Session::begin_undo_scope and end_undo_scope make the edits in between
one undo step of the active document: the scope opens an undo group
there, or joins the one already open. Ending a scope closes only the
group it opened, in its own document, even when another document
became active in the meantime, and a canceled scope undoes only its
own edits, also inside a group that was already open. library.use and
the scrub use the scope, so Escape inside an open group also takes
back a scrub of the Transform panel's Shear field. edit.undoGroup.end
still closes the open group of every document.
@coderabbitai

coderabbitai Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: storytold/vectorcraft/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: a7b6f5b6-1886-4ba6-b507-eafc5fe21fcc
📥 Commits

Reviewing files that changed from the base of the PR and between bc1ae08 and b0b5a42.

📒 Files selected for processing: 7
  • crates/engine/src/cmd/library.rs
  • crates/engine/src/lib.rs
  • crates/engine/src/tests_journal.rs
  • crates/engine/src/tests_library.rs
  • crates/ui-egui/src/lib.rs
  • crates/ui-egui/src/scrub.rs
  • crates/ui-egui/src/tests_scrub.rs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.

📝 Walkthrough

The change adds session-level undo scopes that can open or join a document’s undo group and support cancellation. Library commands and numeric-field scrubbing now use these scopes. Tests cover rollback, existing groups, and document switching.

Changes: Scoped undo behavior

Layer / File(s) Summary
Session undo scope lifecycle
crates/engine/src/lib.rs, crates/engine/src/tests_journal.rs
Session now records the originating document and journal position for each undo scope. It can close that document’s group and cancel scope-local changes. Journal tests cover scopes opened alone or within an existing group.
Library command integration
crates/engine/src/cmd/library.rs, crates/engine/src/tests_library.rs
in_one_step uses undo scopes and cancels them when its callback fails. Tests cover style application across documents and within existing undo groups.
Scrub undo integration
crates/ui-egui/src/lib.rs, crates/ui-egui/src/scrub.rs, crates/ui-egui/src/tests_scrub.rs
The UI stores the scope for an active scrub and ends it with the cancellation status. Tests cover scrubbing within existing groups and switching documents during a scrub.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant LibraryCommand
  participant Session
  participant Document
  participant Journal
  LibraryCommand->>Session: begin_undo_scope()
  Session->>Document: open or join undo group
  Session-->>LibraryCommand: return UndoScope
  LibraryCommand->>Session: end_undo_scope(scope, cancel)
  Session->>Document: close or restore originating group
  Session->>Journal: truncate scope entries on cancellation
Loading

Suggested reviewers: bflatastic, danngreen

Merge Risk: 🔵 Low · up to b0b5a

Switching documents during a numeric scrub can disrupt undo or action history for subsequent edits. This is a narrow case that warrants owner awareness or follow-up, but does not by itself block merging.

Security architecture risk: 🔵 Low · up to b0b5a

Undo cancellation is better isolated between documents, but it can still remove recorded commands for edits preserved in another document. The supported impact is confined to the same editing session; no new privilege-escalation path was established.

Retained concerns

  • Medium · reliability · inferred: Document-local rollback and session-global journal truncation have incompatible ownership. An active scrub in document A can span an authorized activation and edit in document B. Canceling A now preserves B’s open group and edits, but truncates the journal from A’s recorded boundary, removing B’s intervening commands. Recorded actions and replay can therefore omit edits that remain visible. Truncation existed before this PR, but the base canceled all open groups; the new isolation behavior preserves B without correspondingly isolating journal cleanup.
Security review details

Security Blast Radius

  • inferred — The supported failure scope is other open documents sharing the same Session and its journal. Triggering the established interleaving requires existing command access during an active scrub; the inspected path does not establish access to a separate tenant, service, or credential domain.

Trust Boundaries and Controls

  • observed — The scope’s stored UID controls completion and cancellation, not subsequent command targeting. document.activate changes the active document and resets engine tool interaction, while scrub state remains held separately by the UI. Numeric field results continue through the shared command runner against the active document.
Pre-merge checks: Passed 5
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: library styles and field scrubs now close only the undo group they open.
Description check Passed The description includes all required sections, explains the motivation and implementation, documents extensive tests, and completes the repository checklist.
Docstring Coverage Passed Docstring coverage is 80.77% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 26 functions across 7 files.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant