Whiteboard undo redo bug (microsoft#11260)

* Return undefined during empty detach trait

* return undefined when inserting empty traits

* Fix previous commit for inserting empty trait

* update test case

* fix lint errors

* fix lint error

* revert previous commit

* fix logic for inserting empty trait

* Add test case for empty detach

* fix lint errors

* Updated test titles, changed test case logic.

* Changed test case formatting

* fix linting error

* Updated traitlabel, and fixed testcase

* prevent residual builtNodes after insert

* replace undefined with continue for empty traits

* build but skips insert/detach of empty traits

* update test cases for skip instead of undefined

* Simplified test case

* Add use of helper function

* Added expectDefined to test case

* test to check if built/detached nodes are cleared

* Add test case for single empty insert

* fix lint errors

* added whitespace

* added logging to revert function

* Comment addressing input/output array size

* cleaned up code for checking if logger is defined

experimental/dds/tree/src/HistoryEditFactory.ts

@@ -3,6 +3,7 @@
  * Licensed under the MIT License.
  */
 
+import { ITelemetryLogger } from '@fluidframework/common-definitions';
 import { DetachedSequenceId, isDetachedSequenceId, NodeId } from './Identifiers';
 import { assert, fail } from './Common';
 import { rangeFromStableRange } from './TreeViewUtilities';
@@ -28,14 +29,19 @@ import { getChangeNodeFromViewNode } from './SerializationUtilities';
  * @param changes - the changes for which to produce an inverse.
  * @param before - a view of the tree state before `changes` are/were applied - used as a basis for generating the inverse.
  * @returns if the changes could be reverted, a sequence of changes _r_ that will produce `before` if applied to a view _A_, where _A_ is the result of
- * applying `changes` to `before`. Applying _r_ to views other than _A_ is legal but may cause the changes to fail to apply or may
- * not be a true semantic inverse. If the changes could not be reverted given the state of `before`, returns undefined.
+ * applying `changes` to `before`. Note that the size of the array of reverted changes may not be the same as the input array, and may even be empty in cases where
+ * the view did not change. Applying _r_ to views other than _A_ is legal but may cause the changes to fail to apply or may not be a true semantic inverse.
+ * If the changes could not be reverted given the state of `before`, returns undefined.
  *
  * TODO: what should this do if `changes` fails to apply to `before`?
  * TODO:#68574: Pass a view that corresponds to the appropriate Fluid reference sequence number rather than the view just before
  * @internal
  */
-export function revert(changes: readonly ChangeInternal[], before: RevisionView): ChangeInternal[] | undefined {
+export function revert(
+	changes: readonly ChangeInternal[],
+	before: RevisionView,
+	logger?: ITelemetryLogger
+): ChangeInternal[] | undefined {
 	const result: ChangeInternal[] = [];
 
 	const builtNodes = new Map<DetachedSequenceId, NodeId[]>();
@@ -74,9 +80,19 @@ export function revert(changes: readonly ChangeInternal[], before: RevisionView)
 				const nodesDetached = detachedNodes.get(source);
 
 				if (nodesBuilt !== undefined) {
+					if (nodesBuilt.length === 0) {
+						builtNodes.delete(source);
+						logger?.sendTelemetryEvent({ eventName: 'reverting insertion of empty traits' });
+						continue;
+					}
 					result.unshift(createInvertedInsert(change, nodesBuilt));
 					builtNodes.delete(source);
 				} else if (nodesDetached !== undefined) {
+					if (nodesDetached.length === 0) {
+						detachedNodes.delete(source);
+						logger?.sendTelemetryEvent({ eventName: 'reverting insertion of empty traits' });
+						continue;
+					}
 					result.unshift(createInvertedInsert(change, nodesDetached, true));
 					detachedNodes.delete(source);
 				} else {
@@ -96,6 +112,11 @@ export function revert(changes: readonly ChangeInternal[], before: RevisionView)
 				}
 				const { invertedDetach, detachedNodeIds } = invert;
 
+				if (detachedNodeIds.length === 0) {
+					logger?.sendTelemetryEvent({ eventName: 'reverting detachment of empty traits' });
+					continue;
+				}
+
 				if (destination !== undefined) {
 					if (builtNodes.has(destination) || detachedNodes.has(destination)) {
 						// Malformed: destination was already used by a prior build or detach

experimental/dds/tree/src/SharedTree.ts

@@ -1517,7 +1517,7 @@ export class SharedTree extends SharedObject<ISharedTreeEvents> implements NodeI
 	 * @internal
 	 */
 	public revertChanges(changes: readonly InternalizedChange[], before: RevisionView): ChangeInternal[] | undefined {
-		return revert(changes as unknown as readonly ChangeInternal[], before);
+		return revert(changes as unknown as readonly ChangeInternal[], before, this.logger);
 	}
 
 	/**

experimental/dds/tree/src/test/HistoryEditFactory.tests.ts

@@ -5,7 +5,7 @@
 
 import { expect } from 'chai';
 import { revert } from '../HistoryEditFactory';
-import { DetachedSequenceId } from '../Identifiers';
+import { DetachedSequenceId, TraitLabel } from '../Identifiers';
 import { ChangeInternal, DetachInternal, Side, StablePlaceInternal, StableRangeInternal } from '../persisted-types';
 import { expectDefined } from './utilities/TestCommon';
 import { refreshTestTree } from './utilities/TestUtilities';
@@ -19,10 +19,10 @@ describe('revert', () => {
 		const firstBuild = ChangeInternal.build([node], firstDetachedId);
 		const insertedNodeId = 1 as DetachedSequenceId;
 		const insertedBuild = ChangeInternal.build([firstDetachedId], insertedNodeId);
-		const insertChange = ChangeInternal.insert(insertedNodeId, {
-			referenceTrait: testTree.left.traitLocation,
-			side: Side.After,
-		});
+		const insertChange = ChangeInternal.insert(
+			insertedNodeId,
+			StablePlaceInternal.atStartOf(testTree.left.traitLocation)
+		);
 		const result = expectDefined(revert([firstBuild, insertedBuild, insertChange], testTree.view));
 		expect(result.length).to.equal(1);
 		const revertedChange = result[0] as DetachInternal;
@@ -39,17 +39,89 @@ describe('revert', () => {
 		const secondBuild = ChangeInternal.build([secondNode], secondDetachedId);
 		const insertedNodeId = 2 as DetachedSequenceId;
 		const insertedBuild = ChangeInternal.build([firstDetachedId, secondDetachedId], insertedNodeId);
-		const insertChange = ChangeInternal.insert(insertedNodeId, {
-			referenceTrait: testTree.left.traitLocation,
-			side: Side.After,
-		});
+		const insertChange = ChangeInternal.insert(
+			insertedNodeId,
+			StablePlaceInternal.atStartOf(testTree.left.traitLocation)
+		);
 		const result = expectDefined(revert([firstBuild, secondBuild, insertedBuild, insertChange], testTree.view));
 		expect(result.length).to.equal(1);
 		const revertedChange = result[0] as DetachInternal;
 		expect(revertedChange.source.start.referenceSibling).to.deep.equal(firstNode.identifier);
 		expect(revertedChange.source.end.referenceSibling).to.deep.equal(secondNode.identifier);
 	});
 
+	it('handles reverting the insert of empty nodes, with subsequent non-empty nodes', () => {
+		// build and insert of empty traits
+		const emptyTraitNodeId = 0 as DetachedSequenceId;
+		const emptyTraitBuild = ChangeInternal.build([], emptyTraitNodeId);
+		const emptyTraitInsert = ChangeInternal.insert(
+			emptyTraitNodeId,
+			StablePlaceInternal.atStartOf(testTree.left.traitLocation)
+		);
+
+		// build and insert of non-empty traits
+		const firstDetachedId = 1 as DetachedSequenceId;
+		const firstNode = testTree.buildLeafInternal();
+		const firstBuild = ChangeInternal.build([firstNode], firstDetachedId);
+		const insertedNodeId = 3 as DetachedSequenceId;
+		const insertedBuild = ChangeInternal.build([firstDetachedId], insertedNodeId);
+		const insertChange = ChangeInternal.insert(
+			insertedNodeId,
+			StablePlaceInternal.atStartOf(testTree.left.traitLocation)
+		);
+		const result = expectDefined(
+			revert([emptyTraitBuild, emptyTraitInsert, firstBuild, insertedBuild, insertChange], testTree.view)
+		);
+		expect(result.length).to.equal(1);
+		const revertedChange = result[0] as DetachInternal;
+		expect(revertedChange.source.start.referenceSibling).to.deep.equal(firstNode.identifier);
+	});
+
+	/** This is a regression test for a bug where we make sure that any built/detached nodes are cleared when any
+	 *  empty insert/detach change is skipped once encountered. The expected outcome is undefined, as during the second
+	 *  empty insert (with the same DetachSequenceId), there should be no such node in the builtNodes.
+	 */
+	it('handles reverting the insert of empty nodes, with subsequent empty nodes of same DetachedSequenceId', () => {
+		const emptyTraitNodeId = 0 as DetachedSequenceId;
+		const emptyTraitBuild = ChangeInternal.build([], emptyTraitNodeId);
+		const emptyTraitInsert = ChangeInternal.insert(
+			emptyTraitNodeId,
+			StablePlaceInternal.atStartOf(testTree.left.traitLocation)
+		);
+		const result = revert([emptyTraitBuild, emptyTraitInsert, emptyTraitInsert], testTree.view);
+		expect(result).to.be.undefined;
+	});
+
+	it('handles reverting the detach of an empty trait', () => {
+		const insertedNodeId = 0 as DetachedSequenceId;
+		const result = expectDefined(
+			revert(
+				[
+					ChangeInternal.detach(
+						StableRangeInternal.all({
+							label: 'someNonExistentTraitLabel' as TraitLabel,
+							parent: testTree.identifier,
+						}),
+						insertedNodeId
+					),
+				],
+				testTree.view
+			)
+		);
+		expect(result).to.have.lengthOf(0);
+	});
+
+	it('handles reverting the insert of an empty trait', () => {
+		const emptyTraitNodeId = 0 as DetachedSequenceId;
+		const emptyTraitBuild = ChangeInternal.build([], emptyTraitNodeId);
+		const emptyTraitInsert = ChangeInternal.insert(
+			emptyTraitNodeId,
+			StablePlaceInternal.atStartOf(testTree.left.traitLocation)
+		);
+		const result = expectDefined(revert([emptyTraitBuild, emptyTraitInsert], testTree.view));
+		expect(result).to.have.lengthOf(0);
+	});
+
 	describe('returns undefined for reverts that require more context than the view directly before the edit', () => {
 		describe('because the edit conflicted', () => {
 			it('when reverting a detach of a node that is not in the tree', () => {