diff --git a/packages/dds/tree/src/shared-tree/treeCheckout.ts b/packages/dds/tree/src/shared-tree/treeCheckout.ts index 13ba631cafae..112a36dd1d66 100644 --- a/packages/dds/tree/src/shared-tree/treeCheckout.ts +++ b/packages/dds/tree/src/shared-tree/treeCheckout.ts @@ -134,6 +134,20 @@ function* collectTreeLabels(node: LabelTree): IterableIterator { } } +/** + * Deep-clones a {@link LabelTree} so the result is independent of the source. + * @remarks + * 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 @@ -621,6 +635,16 @@ export class TreeCheckout implements ITreeCheckout { // Only clear the label tree when the outermost transaction has completed. // Inner transactions' commits don't fire the "changed" event, so the label tree // must remain intact until the outermost commit reads it. + // + // TODO: clear `labelTreeNode` before the changed event is emitted rather than here + // in the finally. As-is, a `changed` listener that calls `runTransaction` re-entrantly + // will see `labelTreeNode` still set and `pushLabelFrame` will mutate the just-closed + // outer tree by pushing the new label as a sublabel — even though the two transactions + // are logically independent (the outer has already committed, `transaction.size === 0` + // when the inner starts). This conflates the inner transaction's labels with the + // outer's, breaking label-scoped undo/redo: e.g. an "auto-stamp" listener pattern + // would cause its commits to inherit the user's editor label. Moving the clear before + // emit lets re-entrant `pushLabelFrame` create a fresh root for the inner transaction. if (this.transaction.size === 0) { this.labelTreeNode = undefined; this.mostRecentlyClosedLabelNode = undefined; @@ -713,6 +737,13 @@ 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, so the captured + // value is stable against in-place mutations to `this.labelTreeNode` (e.g. by a + // re-entrant `runTransaction` from a listener) or to `metadata.labels.tree` + // (which aliases the same object). + const commitLabelTree = + this.labelTreeNode === undefined ? undefined : cloneLabelTree(this.labelTreeNode); + const getRevertible = hasSchemaChange(change) ? undefined : (onRevertibleDisposed?: (revertible: RevertibleAlpha) => void) => { @@ -731,6 +762,7 @@ export class TreeCheckout implements ITreeCheckout { kind, this, onRevertibleDisposed, + commitLabelTree, ); this.revertibleCommitBranches.set( revision, @@ -1008,6 +1040,8 @@ 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( @@ -1015,6 +1049,7 @@ export class TreeCheckout implements ITreeCheckout { kind: CommitKind, checkout: TreeCheckout, onRevertibleDisposed: ((revertible: RevertibleAlpha) => void) | undefined, + labelTree: LabelTree | undefined, ): RevertibleAlpha { const commitBranches = checkout.revertibleCommitBranches; @@ -1030,7 +1065,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 +1095,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 +1362,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 +1409,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; diff --git a/packages/dds/tree/src/test/shared-tree/schematizingTreeView.spec.ts b/packages/dds/tree/src/test/shared-tree/schematizingTreeView.spec.ts index dfc81138c866..1c5d25c34ce2 100644 --- a/packages/dds/tree/src/test/shared-tree/schematizingTreeView.spec.ts +++ b/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,266 @@ describe("SchematizingSimpleTreeView", () => { assert.equal(receivedLabels.tree.sublabels[1]?.label, "after"); }); + // Skipped: documents the desired behavior for re-entrant transactions started from a + // `changed` listener. Currently, the inner transaction's label is incorrectly pushed + // as a sublabel of the just-closed outer tree even though the two transactions are + // logically independent (the outer has already committed at that point). See the TODO + // in `runWithTransactionLabel` for the proposed fix; unskip this test once it lands. + it.skip("re-entrant transactions from a changed listener produce a separate label tree", () => { + const view = getTestObjectView(); + + let labels: TransactionLabels | undefined; + view.checkout.events.on("changed", (meta) => { + if (meta.isLocal && meta.kind === CommitKind.Default && meta.labels.has("inner")) { + labels = meta.labels; + } + }); + + // Re-entrant: when the outer commit fires, start a separate `inner` transaction. + view.checkout.events.on("changed", (meta) => { + if (meta.isLocal && meta.kind === CommitKind.Default && !meta.labels.has("inner")) { + view.runTransaction( + () => { + view.root.content = view.root.content + 1; + }, + { label: "inner" }, + ); + } + }); + + view.runTransaction( + () => { + view.root.content = 1; + }, + { label: "outer" }, + ); + + assert.deepEqual(labels?.tree, { label: "inner", sublabels: [] }); + }); + + 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("revertible's captured labels survive a re-entrant runTransaction from a changed listener", () => { + // Regression guard for the eager-clone strategy in `getRevertible`. + // Realistic scenario: an "auto-stamp" listener that runs another transaction in + // response to user edits. The framework's `pushLabelFrame` mutates the live + // `labelTreeNode` in place by pushing onto its `sublabels`, and the inner + // `runWithTransactionLabel`'s finally block clears `labelTreeNode = undefined`. + // Both happen before a later listener calls `getRevertible()` for the user's commit. + // With eager clone, the captured snapshot was taken before any listener ran, so + // the revertible's labels remain stable. + const view = getTestObjectView(); + + // Listener A (registered first): auto-stamp every user edit. Self-skips via the + // "auto" label to avoid infinite recursion on its own commits. + view.checkout.events.on("changed", (meta) => { + if (meta.isLocal && meta.kind === CommitKind.Default && !meta.labels.has("auto")) { + view.runTransaction( + () => { + view.root.content = view.root.content + 1; + }, + { label: "auto" }, + ); + } + }); + + // Listener B (registered second): capture the user's revertible (skipping the + // auto-stamp commit emitted recursively from Listener A's runTransaction). + let userRevertible: RevertibleAlpha | undefined; + let undoLabels: TransactionLabels | undefined; + view.checkout.events.on("changed", (meta, getRevertible) => { + if (meta.isLocal && meta.kind === CommitKind.Default && !meta.labels.has("auto")) { + userRevertible = getRevertible?.(); + } + if (meta.isLocal && meta.kind === CommitKind.Undo && !meta.labels.has("auto")) { + undoLabels = meta.labels; + } + }); + + view.runTransaction( + () => { + view.root.content = 1; + }, + { label: "outer" }, + ); + + assert(userRevertible !== undefined); + userRevertible.revert(); + + // The revert commit must carry only the user's "outer" label. With lazy capture, + // the inner runWithTransactionLabel's finally would have cleared `labelTreeNode` + // before this revertible was created, leaving us with an empty label set here. + assert.deepEqual(undoLabels?.tree, { label: "outer", sublabels: [] }); + assert.equal(undoLabels?.has("outer"), true); + assert.equal(undoLabels?.has("auto"), false); + }); + + 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(); diff --git a/packages/framework/react/src/test/undoRedo.test.ts b/packages/framework/react/src/test/undoRedo.test.ts index 79313d094b23..0649099bbb1c 100644 --- a/packages/framework/react/src/test/undoRedo.test.ts +++ b/packages/framework/react/src/test/undoRedo.test.ts @@ -7,6 +7,7 @@ import { strict as assert } from "node:assert"; import { validateUsageError } from "@fluidframework/test-runtime-utils/internal"; import { + CommitKind, independentView, type TreeBranchAlpha, type TreeViewAlpha, @@ -301,10 +302,11 @@ describe("createUndoRedo", () => { fireChanged: ( isLocal: boolean, getRevertible: (() => { revert(): void; dispose(): void }) | undefined, + kind?: CommitKind, ) => void; } { type Handler = ( - data: { isLocal: boolean; labels: unknown[] }, + data: { isLocal: boolean; kind: CommitKind; labels: unknown[] }, getRevertible: (() => { revert(): void; dispose(): void }) | undefined, ) => void; const handlers: Handler[] = []; @@ -321,8 +323,8 @@ describe("createUndoRedo", () => { } as unknown as TreeBranchAlpha; return { branch, - fireChanged: (isLocal, getRevertible) => { - for (const h of handlers) h({ isLocal, labels: [] }, getRevertible); + fireChanged: (isLocal, getRevertible, kind = CommitKind.Default) => { + for (const h of handlers) h({ isLocal, kind, labels: [] }, getRevertible); }, }; } @@ -356,10 +358,10 @@ describe("createUndoRedo", () => { }; // The undo revertible fires a changed event during its revert (as real SharedTree does), - // routing redoRevertible onto the redo stack. + // routing redoRevertible onto the redo stack via its CommitKind.Undo kind. fireChanged(true, () => ({ revert() { - fireChanged(true, () => redoRevertible); + fireChanged(true, () => redoRevertible, CommitKind.Undo); }, dispose() {}, })); @@ -372,7 +374,7 @@ describe("createUndoRedo", () => { manager.dispose(); }); - it("clears pendingOperation after undo revert() throws so new commits land on undo stack (C1)", () => { + it("a new commit after a failed undo lands on the undo stack", () => { const { branch, fireChanged } = createMockBranch(); const manager = createUndoRedo(branch); @@ -384,15 +386,14 @@ describe("createUndoRedo", () => { })); assert.throws(() => manager.undo()); - // A new user commit arriving after the failed undo must go to the undo stack, - // not the redo stack (which is what happens when #pendingOperation is stuck). + // A new user commit arriving after the failed undo must go to the undo stack. fireChanged(true, () => ({ revert() {}, dispose() {} })); - assert(!manager.canRedo(), "redo stack should be empty — pendingOperation was cleared"); + assert(!manager.canRedo(), "redo stack should be empty"); assert(manager.canUndo(), "new commit should be on the undo stack"); manager.dispose(); }); - it("clears pendingOperation after redo revert() throws so new commits land on undo stack (C1)", () => { + it("a new commit after a failed redo lands on the undo stack", () => { const { branch, fireChanged } = createMockBranch(); const manager = createUndoRedo(branch); @@ -405,10 +406,10 @@ describe("createUndoRedo", () => { }; // The undo revertible fires a changed event during its revert (as real SharedTree does), - // routing redoRevertible onto the redo stack. + // routing redoRevertible onto the redo stack via its CommitKind.Undo kind. fireChanged(true, () => ({ revert() { - fireChanged(true, () => redoRevertible); + fireChanged(true, () => redoRevertible, CommitKind.Undo); }, dispose() {}, })); @@ -421,7 +422,7 @@ describe("createUndoRedo", () => { // New user commit must land on the undo stack, not the redo stack. fireChanged(true, () => ({ revert() {}, dispose() {} })); - assert(!manager.canRedo(), "pendingOperation was cleared after redo failure"); + assert(!manager.canRedo(), "redo stack should be empty after failed redo"); manager.dispose(); }); }); diff --git a/packages/framework/react/src/undoRedo.ts b/packages/framework/react/src/undoRedo.ts index e59f61e8ea72..65ae2b7f26b3 100644 --- a/packages/framework/react/src/undoRedo.ts +++ b/packages/framework/react/src/undoRedo.ts @@ -3,9 +3,13 @@ * Licensed under the MIT License. */ -import { assert, oob } from "@fluidframework/core-utils/internal"; +import { oob } from "@fluidframework/core-utils/internal"; import { UsageError } from "@fluidframework/telemetry-utils/internal"; -import type { RevertibleAlpha, TreeBranchAlpha } from "@fluidframework/tree/internal"; +import { + CommitKind, + type RevertibleAlpha, + type TreeBranchAlpha, +} from "@fluidframework/tree/internal"; import { areSetsDisjoint, findLastIndex } from "./utilities.js"; @@ -200,17 +204,6 @@ class UndoRedoManager implements UndoRedo { */ readonly #branch: TreeBranchAlpha; - /** - * Set synchronously around `revert()` calls so the `changed` event handler can attribute the - * resulting commit to an undo or redo action rather than treating it as a new user commit. - * - * @remarks - * This workaround is needed because SharedTree's `revert()` does not preserve the original - * commit's labels on the resulting commit. - * TODO: AB#71256: Remove once SharedTree supports preserving commit labels on revert. - */ - #pendingOperation: { kind: "undo" | "redo"; labels: ReadonlySet } | undefined; - /** Whether or not this instance has been disposed. */ #disposed = false; @@ -229,25 +222,21 @@ class UndoRedoManager implements UndoRedo { return; } - if (this.#pendingOperation !== undefined) { - const { kind, labels } = this.#pendingOperation; - const revertible = getRevertible(); - // Route to the opposite stack, preserving the original commit's labels so that - // label-filtered canUndo/canRedo/undo/redo remain consistent after undo or redo. - if (kind === "undo") { - this.#redoStack.push({ revertible, labels }); - } else { - this.#undoStack.push({ revertible, labels }); - } + // Revert commits inherit their original commit's labels (per SharedTree's commit metadata), + // so we route by `data.kind` and pull labels straight from `data.labels`. + const commitLabels = new Set(data.labels); + + if (data.kind === CommitKind.Undo) { + this.#redoStack.push({ revertible: getRevertible(), labels: commitLabels }); + return; + } + if (data.kind === CommitKind.Redo) { + this.#undoStack.push({ revertible: getRevertible(), labels: commitLabels }); return; } - // Normal user commit: collect root-level labels from the commit metadata. - // Nested label nodes (produced by inner runTransaction calls) are not traversed — - // see UndoRedo remarks. - const commitLabels = new Set(data.labels); - - // Redo invalidation: clear redo entries whose label sets overlap with this commit's labels. + // Default: a new user commit. Invalidate redo entries whose label sets overlap with this + // commit's labels. An anonymous commit clears only anonymous redo entries. for (let i = this.#redoStack.length - 1; i >= 0; i--) { const entry = this.#redoStack[i]; if (entry === undefined) { @@ -272,14 +261,14 @@ class UndoRedoManager implements UndoRedo { if (this.#disposed) { return; } - this.#revertWhere(this.#undoStack, "undo", labelPredicate(label)); + this.#revertWhere(this.#undoStack, labelPredicate(label)); } public redo(label?: unknown): void { if (this.#disposed) { return; } - this.#revertWhere(this.#redoStack, "redo", labelPredicate(label)); + this.#revertWhere(this.#redoStack, labelPredicate(label)); } public canUndo(label?: unknown): boolean { @@ -315,32 +304,17 @@ class UndoRedoManager implements UndoRedo { /** * Reverts the top-most entry in `stack` matching `predicate`. - * @remarks No-ops if no entry matches. - * - * @param stack - The undo or redo stack to operate on. - * @param kind - Whether this is an `"undo"` or `"redo"` operation, used to route the resulting - * revertible to the opposite stack. - * @param predicate - Selects the target entry; the top-most matching entry is reverted. + * @remarks No-ops if no entry matches. The resulting commit fires the `changed` event with + * `kind: CommitKind.Undo` or `CommitKind.Redo` (matching the inverse direction of the original), + * which routes it onto the opposite stack via the `changed` handler. */ - #revertWhere( - stack: StackEntry[], - kind: "undo" | "redo", - predicate: (entry: StackEntry) => boolean, - ): void { - assert(this.#pendingOperation === undefined, "Unexpected pending operation during revert"); - + #revertWhere(stack: StackEntry[], predicate: (entry: StackEntry) => boolean): void { const index = findLastIndex(stack, predicate); if (index === -1) { return; } const entry = stack.splice(index, 1)[0] ?? oob(); - - this.#pendingOperation = { kind, labels: entry.labels }; - try { - entry.revertible.revert(); - } finally { - this.#pendingOperation = undefined; - } + entry.revertible.revert(); } }