diff --git a/.changeset/tough-lies-say.md b/.changeset/tough-lies-say.md index baca2f29014..22fea7005ac 100644 --- a/.changeset/tough-lies-say.md +++ b/.changeset/tough-lies-say.md @@ -5,16 +5,14 @@ --- Editing a SharedTree during its change-event callbacks now consistently throws -Editing a `SharedTree` from inside one of its change-event callbacks has always been forbidden, but some paths were not being caught: edits and the start of a transaction (along with branch operations, reverts, etc.) made while the tree was emitting its post-change notification ran to completion instead of throwing. +Editing a `SharedTree` from inside one of its change-event callbacks has always been forbidden, but one path was not being caught: edits (along with branch operations, reverts, transactions, etc.) made while the tree was emitting its post-change notification ran to completion instead of throwing. Such edits would apply to the tree, trigger further change notifications, and could re-enter the same listener for the resulting commits. This can produce infinite edit loops, redundant work across clients, incorrect attribution, broken undo/redo grouping, and pollution of the outer commit's label data. -This release closes those gaps: both editing the tree and starting a transaction during a change-event callback now throw the same canonical `UsageError` as the other change-event callbacks: +This release closes that gap: editing the tree during a change-event callback now throws the same canonical `UsageError` as the other change-event callbacks: > Editing the tree is forbidden during a change event callback -> Running a transaction is forbidden during a change event callback - More generally, edits should not be made in response to changes to the document. See [Editing in response to change events](https://fluidframework.com/docs/data-structures/tree/events#editing-in-response-to-change-events) for why, and for the recommended alternatives. diff --git a/packages/dds/tree/src/shared-tree/treeCheckout.ts b/packages/dds/tree/src/shared-tree/treeCheckout.ts index a276467f805..a46806b6b34 100644 --- a/packages/dds/tree/src/shared-tree/treeCheckout.ts +++ b/packages/dds/tree/src/shared-tree/treeCheckout.ts @@ -936,15 +936,6 @@ export class TreeCheckout implements ITreeCheckout { private mountTransaction(params: RunTransactionParams | undefined, isAsync: boolean): void { this.checkNotDisposed(); - // Starting a transaction is an edit, so it is forbidden from within a change-event - // callback (where the edit lock is held), the same as direct edits. For the async - // entry point this throw is captured as a rejected promise by the `async` wrapper. - // - // Note: because runTransaction/runTransactionAsync are `@breakingMethod`, this throw also - // puts the checkout into a broken state (unlike a direct edit, which throws recoverably). - // That is the same pre-existing broken-state limitation tracked by the TODO in - // `emitChangedLocked`, not something specific to transactions. - this.editLock.checkUnlocked("Running a transaction"); if (isAsync && this.transaction.size > 0) { throw new UsageError( "An asynchronous transaction cannot be started while another transaction is already in progress.", diff --git a/packages/dds/tree/src/test/shared-tree/treeCheckout.spec.ts b/packages/dds/tree/src/test/shared-tree/treeCheckout.spec.ts index e8c43395f04..6ab9c83236e 100644 --- a/packages/dds/tree/src/test/shared-tree/treeCheckout.spec.ts +++ b/packages/dds/tree/src/test/shared-tree/treeCheckout.spec.ts @@ -1363,37 +1363,6 @@ describe("sharedTreeView", () => { error: "Disposing a view is forbidden during a change event callback", }); }); - - it("run a transaction", () => { - expectErrorDuringEdit({ - duringEdit: (view) => view.runTransaction(() => {}), - error: "Running a transaction is forbidden during a change event callback", - }); - }); - - it("run an async transaction", async () => { - const view = getView( - new TreeViewConfiguration({ enableSchemaValidation, schema: NumberNode }), - ); - view.initialize({ number: 3 }); - - // Unlike the synchronous cases above, `runTransactionAsync` surfaces the guard as a - // rejected promise rather than a synchronous throw, so it is captured and awaited here. - let asyncTransaction: Promise | undefined; - Tree.on(view.root, "nodeChanged", () => { - asyncTransaction = view.runTransactionAsync(async () => {}); - }); - - view.root.number = 0; - - assert(asyncTransaction !== undefined, "Async transaction should have been attempted."); - await assert.rejects( - asyncTransaction, - validateUsageError( - "Running a transaction is forbidden during a change event callback", - ), - ); - }); }); describe("Enrichment", () => {