Transaction labels for undo redo bug fix

packages/dds/tree/src/shared-tree/treeCheckout.ts

@@ -134,6 +134,19 @@ function* collectTreeLabels(node: LabelTree): IterableIterator<unknown> {
 	}
 }
 
+/**
+ * Deep-clones a {@link LabelTree} so the result is independent of the source.
+ * Used when capturing the label tree on a revertible so that subsequent mutations
+ * (by the framework or by external listeners reading {@link TransactionLabels.tree})
+ * cannot affect the labels the revertible will emit.
+ */
+function cloneLabelTree(tree: LabelTree): LabelTree {
+	return {
+		label: tree.label,
+		sublabels: tree.sublabels.map(cloneLabelTree),
+	};
+}
+
 /**
  * Builds the labels set for a change event from the label tree.
  * If the tree exists and contains at least one defined label, returns a set of all
@@ -713,6 +726,10 @@ export class TreeCheckout implements ITreeCheckout {
 				const kind = event.type === "append" ? event.kind : CommitKind.Default;
 				const { change, revision } = commit;
 
+				// Snapshot the label tree for this commit before any listener runs.
+				const commitLabelTree =
+					this.labelTreeNode === undefined ? undefined : cloneLabelTree(this.labelTreeNode);
+
 				const getRevertible = hasSchemaChange(change)
 					? undefined
 					: (onRevertibleDisposed?: (revertible: RevertibleAlpha) => void) => {
@@ -731,6 +748,7 @@ export class TreeCheckout implements ITreeCheckout {
 								kind,
 								this,
 								onRevertibleDisposed,
+								commitLabelTree,
 							);
 							this.revertibleCommitBranches.set(
 								revision,
@@ -1008,13 +1026,16 @@ export class TreeCheckout implements ITreeCheckout {
 	 * @param kind - The {@link CommitKind} that produced this revertible (e.g., Default, Undo, Redo).
 	 * @param checkout - The {@link TreeCheckout} instance this revertible belongs to.
 	 * @param onRevertibleDisposed - Callback function that will be called when the revertible is disposed.
+	 * @param labelTree - The {@link LabelTree} (if any) active when the original commit was produced.
+	 * The revert commit inherits these labels so that hosts can scope undo/redo by transaction label.
 	 * @returns A {@link RevertibleAlpha} object.
 	 */
 	private createRevertible(
 		revision: RevisionTag,
 		kind: CommitKind,
 		checkout: TreeCheckout,
 		onRevertibleDisposed: ((revertible: RevertibleAlpha) => void) | undefined,
+		labelTree: LabelTree | undefined,
 	): RevertibleAlpha {
 		const commitBranches = checkout.revertibleCommitBranches;
 
@@ -1030,7 +1051,7 @@ export class TreeCheckout implements ITreeCheckout {
 					throw new UsageError("Unable to revert a revertible that has been disposed.");
 				}
 
-				const revertMetrics = checkout.revertRevertible(revision, kind);
+				const revertMetrics = checkout.revertRevertible(revision, kind, labelTree);
 				checkout.logger?.sendTelemetryEvent({
 					eventName: TreeCheckout.revertTelemetryEventName,
 					...revertMetrics,
@@ -1060,7 +1081,13 @@ export class TreeCheckout implements ITreeCheckout {
 
 				targetCheckout.revertibleCommitBranches.set(revision, revertibleBranch.fork());
 
-				return this.createRevertible(revision, kind, targetCheckout, onRevertibleDisposed);
+				return this.createRevertible(
+					revision,
+					kind,
+					targetCheckout,
+					onRevertibleDisposed,
+					labelTree,
+				);
 			},
 			dispose: () => {
 				if (revertible.status === RevertibleStatus.Disposed) {
@@ -1321,7 +1348,11 @@ export class TreeCheckout implements ITreeCheckout {
 		this.revertibles.delete(revertible);
 	}
 
-	private revertRevertible(revision: RevisionTag, kind: CommitKind): RevertMetrics {
+	private revertRevertible(
+		revision: RevisionTag,
+		kind: CommitKind,
+		labelTree: LabelTree | undefined,
+	): RevertMetrics {
 		this.editLock.checkUnlocked("Reverting a commit");
 		if (this.transaction.size > 0) {
 			throw new UsageError("Undo is not yet supported during transactions.");
@@ -1364,12 +1395,23 @@ export class TreeCheckout implements ITreeCheckout {
 			);
 		}
 
-		this.#transaction.activeBranch.apply(
-			change,
-			kind === CommitKind.Default || kind === CommitKind.Redo
-				? CommitKind.Undo
-				: CommitKind.Redo,
-		);
+		// Install the original commit's label tree so the revert commit's metadata inherits
+		// the same labels — letting hosts scope undo/redo by label. Reusing the captured tree
+		// (rather than wrapping it) ensures revert-of-revert does not introduce new nesting.
+		const previousLabelTreeNode = this.labelTreeNode;
+		this.labelTreeNode = labelTree;
+		try {
+			this.#transaction.activeBranch.apply(
+				change,
+				kind === CommitKind.Default || kind === CommitKind.Redo
+					? CommitKind.Undo
+					: CommitKind.Redo,
+			);
+		} finally {
+			// Restore rather than clear: a revert triggered from inside another labeled
+			// transaction's changed event must not wipe that outer commit's label state.
+			this.labelTreeNode = previousLabelTreeNode;
+		}
 
 		// Derive some stats about the reversion to return to the caller.
 		let revertAge = 0;

packages/dds/tree/src/test/shared-tree/schematizingTreeView.spec.ts

@@ -8,7 +8,7 @@ import { strict as assert, fail } from "node:assert";
 import { UsageError } from "@fluidframework/telemetry-utils/internal";
 import { validateUsageError } from "@fluidframework/test-runtime-utils/internal";
 
-import type { TransactionLabels } from "../../core/index.js";
+import { CommitKind, type RevertibleAlpha, type TransactionLabels } from "../../core/index.js";
 import { MockNodeIdentifierManager, TreeStatus } from "../../feature-libraries/index.js";
 import {
 	ForestTypeExpensiveDebug,
@@ -1454,6 +1454,174 @@ describe("SchematizingSimpleTreeView", () => {
 			assert.equal(receivedLabels.tree.sublabels[1]?.label, "after");
 		});
 
+		it("revert commit inherits the original commit's label", () => {
+			const view = getTestObjectView();
+
+			const events: { kind: CommitKind; label: unknown; labels: TransactionLabels }[] = [];
+			let revertible: RevertibleAlpha | undefined;
+			view.checkout.events.on("changed", (meta, getRevertible) => {
+				if (meta.isLocal) {
+					events.push({ kind: meta.kind, label: meta.label, labels: meta.labels });
+					if (meta.kind === CommitKind.Default) {
+						revertible = getRevertible?.();
+					}
+				}
+			});
+
+			const testLabel = "testLabel";
+			view.runTransaction(
+				() => {
+					view.root.content = 1;
+				},
+				{ label: testLabel },
+			);
+
+			assert(revertible !== undefined);
+			revertible.revert();
+
+			assert.equal(events.length, 2);
+			assert.equal(events[1]?.kind, CommitKind.Undo);
+			assert.equal(events[1]?.label, testLabel);
+			assert.deepEqual(events[1]?.labels.tree, { label: testLabel, sublabels: [] });
+		});
+
+		it("revert of revert reuses the original label tree", () => {
+			const view = getTestObjectView();
+
+			const events: { kind: CommitKind; labels: TransactionLabels }[] = [];
+			const revertibles: RevertibleAlpha[] = [];
+			view.checkout.events.on("changed", (meta, getRevertible) => {
+				if (meta.isLocal) {
+					events.push({ kind: meta.kind, labels: meta.labels });
+					const r = getRevertible?.();
+					if (r !== undefined) {
+						revertibles.push(r);
+					}
+				}
+			});
+
+			const testLabel = "testLabel";
+			view.runTransaction(
+				() => {
+					view.root.content = 1;
+				},
+				{ label: testLabel },
+			);
+
+			revertibles[0]?.revert(); // undo
+			revertibles[1]?.revert(); // redo
+
+			assert.deepEqual(
+				events.map((e) => e.kind),
+				[CommitKind.Default, CommitKind.Undo, CommitKind.Redo],
+			);
+
+			// All three commits share the same label tree — revert-of-revert does not introduce new nesting.
+			const expectedTree = { label: testLabel, sublabels: [] };
+			assert.deepEqual(events[0]?.labels.tree, expectedTree);
+			assert.deepEqual(events[1]?.labels.tree, expectedTree);
+			assert.deepEqual(events[2]?.labels.tree, expectedTree);
+		});
+
+		it("revert of an unlabeled edit produces empty labels", () => {
+			const view = getTestObjectView();
+
+			const events: { kind: CommitKind; label: unknown; labels: TransactionLabels }[] = [];
+			let revertible: RevertibleAlpha | undefined;
+			view.checkout.events.on("changed", (meta, getRevertible) => {
+				if (meta.isLocal) {
+					events.push({ kind: meta.kind, label: meta.label, labels: meta.labels });
+					if (meta.kind === CommitKind.Default) {
+						revertible = getRevertible?.();
+					}
+				}
+			});
+
+			view.runTransaction(() => {
+				view.root.content = 1;
+			});
+
+			assert(revertible !== undefined);
+			revertible.revert();
+
+			assert.equal(events.length, 2);
+			assert.equal(events[1]?.kind, CommitKind.Undo);
+			assert.equal(events[1]?.label, undefined);
+			assert.equal(events[1]?.labels.size, 0);
+		});
+
+		it("cloned revertible inherits the original commit's labels", () => {
+			const sourceView = getTestObjectView();
+
+			let sourceRevertible: RevertibleAlpha | undefined;
+			sourceView.checkout.events.on("changed", (meta, getRevertible) => {
+				if (meta.isLocal && meta.kind === CommitKind.Default) {
+					sourceRevertible = getRevertible?.();
+				}
+			});
+
+			const testLabel = "testLabel";
+			sourceView.runTransaction(
+				() => {
+					sourceView.root.content = 1;
+				},
+				{ label: testLabel },
+			);
+
+			// Fork after the labeled commit so the cloned revertible's source commit is reachable on the target.
+			const targetView = sourceView.fork();
+			let undoLabels: TransactionLabels | undefined;
+			targetView.checkout.events.on("changed", (meta) => {
+				if (meta.isLocal && meta.kind === CommitKind.Undo) {
+					undoLabels = meta.labels;
+				}
+			});
+
+			assert(sourceRevertible !== undefined);
+			sourceRevertible.clone(targetView).revert();
+
+			assert.deepEqual(undoLabels?.tree, { label: testLabel, sublabels: [] });
+		});
+
+		it("revert of a nested transaction preserves the nested label structure", () => {
+			const view = getTestObjectView();
+
+			const events: { kind: CommitKind; label: unknown; labels: TransactionLabels }[] = [];
+			let revertible: RevertibleAlpha | undefined;
+			view.checkout.events.on("changed", (meta, getRevertible) => {
+				if (meta.isLocal) {
+					events.push({ kind: meta.kind, label: meta.label, labels: meta.labels });
+					if (meta.kind === CommitKind.Default) {
+						revertible = getRevertible?.();
+					}
+				}
+			});
+
+			view.runTransaction(
+				() => {
+					view.runTransaction(
+						() => {
+							view.root.content = 1;
+						},
+						{ label: "inner" },
+					);
+				},
+				{ label: "outer" },
+			);
+
+			assert(revertible !== undefined);
+			revertible.revert();
+
+			assert.equal(events.length, 2);
+			assert.equal(events[1]?.kind, CommitKind.Undo);
+			// metadata.label is the outermost label; labels.tree captures the full nesting.
+			assert.equal(events[1]?.label, "outer");
+			assert.deepEqual(events[1]?.labels.tree, {
+				label: "outer",
+				sublabels: [{ label: "inner", sublabels: [] }],
+			});
+		});
+
 		it("inner labels are surfaced with undefined root when outer transaction has no label", () => {
 			const view = getTestObjectView();