Transaction labels for undo redo bug fix#27202
Transaction labels for undo redo bug fix#27202daesunp wants to merge 8 commits intomicrosoft:mainfrom
Conversation
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (439 lines, 4 files), I've queued these reviewers:
How this works
|
There was a problem hiding this comment.
Pull request overview
Fixes undo/redo metadata so that transaction labels (including nested label trees) are preserved when generating revert commits and cloned revertibles.
Changes:
- Adds test coverage ensuring undo/redo commits inherit and reuse the original transaction label tree.
- Captures the active label tree when creating revertibles so revert commits can emit matching labels.
- Threads the captured label tree through revert/clone paths to keep labels consistent across undo/redo.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/dds/tree/src/test/shared-tree/schematizingTreeView.spec.ts | Adds tests asserting label/label-tree behavior for undo, redo, unlabeled edits, cloning, and nested transactions. |
| packages/dds/tree/src/shared-tree/treeCheckout.ts | Persists label-tree context into revertibles and uses it when applying undo/redo so revert commits emit consistent label metadata. |
| // Capture the label tree active when the commit was produced so that the | ||
| // resulting revert commit can inherit the same transaction labels. | ||
| const revertible = this.createRevertible( | ||
| revision, | ||
| kind, | ||
| this, | ||
| onRevertibleDisposed, | ||
| this.labelTreeNode, | ||
| ); |
There was a problem hiding this comment.
getRevertible currently reads this.labelTreeNode at the time the listener calls getRevertible(). Since labelTreeNode is mutable shared state (and can change during the same changed emit, e.g. if another listener triggers an undo/redo), this can cause the created revertible to capture the wrong label tree (or none). Capture the label tree for the commit into a local const before emitting the event and close over that snapshot when creating the revertible.
| // Inherit the original commit's transaction labels on the revert commit, so that hosts using | ||
| // labels to scope undo/redo see the revert grouped with the edit it is inverting. | ||
| // The same captured tree is reused for revert-of-revert without introducing new nesting. | ||
| this.labelTreeNode = labelTree; | ||
| try { | ||
| this.#transaction.activeBranch.apply( | ||
| change, | ||
| kind === CommitKind.Default || kind === CommitKind.Redo | ||
| ? CommitKind.Undo | ||
| : CommitKind.Redo, | ||
| ); | ||
| } finally { | ||
| this.labelTreeNode = undefined; | ||
| } |
There was a problem hiding this comment.
The revert path temporarily assigns this.labelTreeNode = labelTree and then unconditionally clears it to undefined. If an undo/redo is triggered from within a changed event for a labeled transaction, this will clear the original commit’s label tree mid-emit and can affect later listeners (and/or revertible creation) that still expect labelTreeNode to reflect the original commit. Save/restore the previous labelTreeNode (and mostRecentlyClosedLabelNode if relevant) instead of always setting undefined in the finally.
There was a problem hiding this comment.
Updated to keep track of previousLabelTreeNode to restore instead of setting to undefined
| const revertible = this.createRevertible( | ||
| revision, | ||
| kind, | ||
| this, | ||
| onRevertibleDisposed, | ||
| this.labelTreeNode, | ||
| ); |
There was a problem hiding this comment.
labelTree is passed/stored by reference, and TransactionLabels.tree is exposed to external listeners as a mutable object. With this change, future undo/redo label metadata will depend on whether any listener mutated the previously-emitted label tree. Consider deep-cloning (or otherwise making immutable) the captured LabelTree when storing it on a revertible so undo/redo labels remain stable.
There was a problem hiding this comment.
added cloneLabelTree function to pass in deep clone instead of the labelTree directly
| const { change, revision } = commit; | ||
|
|
||
| // Snapshot the label tree for this commit before any listener runs. | ||
| const commitLabelTree = |
There was a problem hiding this comment.
Should we do this inside getRevertible to avoid the clone cost when getRevertible isn't called?
There was a problem hiding this comment.
getRevertible can't be called more than once, so it shouldn't result in any duplicated computation (I think)
There was a problem hiding this comment.
Updated with suggestion
There was a problem hiding this comment.
I ended up moving this back to outside the factory, as I realized it doesn't cover one of the issues copilot brought up if the labelTree was mutated during a changed emit. I added a unit test to cover this scenario as well.
I also realized that if we have a separately started transaction inside the changed event with a label, that label would be nested into the just closed labelTree as we do not clear it before the changed event is emitted. I don't think this is what we want (I think two separate labelTree roots would make more sense). I added a TODO comment to follow up and fix this as well.
Description
Fixes a bug when you have pass a label into a transaction, once you perform an undo, the resulting revertible no longer has the label.