Clean up stale local references referring to old interval endpoints (#…

…20026)

During the interval change codepath, many local references are created
and updated, but local references for removed interval endpoints were
never cleaned up. This was causing a bug where after removing a range of
text and reverting the remove, the interval endpoints would not return
to the position at which they were created. Some existing unit tests
also used the stale references, and these only worked in the past
because we didn't do any clean up. Those tests now use the updated
references.


[AB#7044](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/7044),
#19352

---------

Co-authored-by: Scarlett Lee <[email protected]>

packages/dds/sequence/src/intervalCollection.ts

@@ -1421,6 +1421,10 @@ export class IntervalCollection<TInterval extends ISerializableInterval>
 			if (newInterval) {
 				this.addPendingChange(id, serializedInterval);
 				this.emitChange(newInterval, interval, true, false);
+				if (interval instanceof SequenceInterval) {
+					this.client?.removeLocalReferencePosition(interval.start);
+					this.client?.removeLocalReferencePosition(interval.end);
+				}
 			}
 			return newInterval;
 		}

packages/dds/sequence/src/test/intervalCollection.detached.spec.ts

@@ -88,9 +88,11 @@ describe("IntervalCollection detached", () => {
 	describe("interval changed while detached", () => {
 		it("slides immediately on segment removal", () => {
 			sharedString.insertText(0, "0123");
-			const interval = collection.add({ start: 0, end: 2 });
-			collection.change(interval.getIntervalId(), { start: 0, end: 0 });
+			const id = collection.add({ start: 0, end: 2 }).getIntervalId();
+			collection.change(id, { start: 0, end: 0 });
 			sharedString.removeText(0, 1);
+			const interval = collection.getIntervalById(id);
+			assert(interval !== undefined, "interval should be defined");
 			assert.equal((interval.start.getSegment() as TextSegment)?.text, "123");
 			assert.equal((interval.end.getSegment() as TextSegment)?.text, "123");
 		});

packages/dds/sequence/src/test/revertibles.spec.ts

@@ -283,6 +283,64 @@ describe("Sequence.Revertibles with Local Edits", () => {
 		revertSharedStringRevertibles(sharedString, revertibles.splice(0));
 		assertSequenceIntervals(sharedString, collection, [{ start: 1, end: 3 }]);
 	});
+	it("performs two acked interval removes and reverts to ensure interval returns to correct position", () => {
+		const undoRevertibles: SharedStringRevertible[] = [];
+		const redoRevertibles: SharedStringRevertible[] = [];
+		let currentRevertStack = undoRevertibles;
+
+		sharedString.insertText(0, "123456789");
+		collection.add({ start: 2, end: 2 });
+
+		sharedString.on("sequenceDelta", (op) => {
+			if (op.isLocal) {
+				appendSharedStringDeltaToRevertibles(sharedString, op, currentRevertStack);
+			}
+		});
+		collection.on("changeInterval", (interval, previousInterval, local, op, slide) => {
+			if (
+				slide === false &&
+				(interval.end !== previousInterval.end || interval.start !== previousInterval.start)
+			) {
+				appendChangeIntervalToRevertibles(
+					sharedString,
+					interval,
+					previousInterval,
+					currentRevertStack,
+				);
+			}
+		});
+		// remove "34"
+		sharedString.removeRange(2, 4);
+		containerRuntimeFactory.processAllMessages();
+		assert.equal(sharedString.getText(), "1256789");
+		assertSequenceIntervals(sharedString, collection, [{ start: 2, end: 2 }]);
+
+		// undo to reinsert "34"
+		currentRevertStack = redoRevertibles;
+		revertSharedStringRevertibles(sharedString, undoRevertibles.splice(0));
+		currentRevertStack = undoRevertibles;
+		containerRuntimeFactory.processAllMessages();
+		assert.equal(undoRevertibles.length, 0);
+		assert.equal(redoRevertibles.length, 2);
+
+		assert.equal(sharedString.getText(), "123456789");
+		assertSequenceIntervals(sharedString, collection, [{ start: 2, end: 2 }]);
+
+		// remove "5"
+		sharedString.removeRange(4, 5);
+		containerRuntimeFactory.processAllMessages();
+		assert.equal(sharedString.getText(), "12346789");
+		assert.equal(undoRevertibles.length, 1);
+		assertSequenceIntervals(sharedString, collection, [{ start: 2, end: 2 }]);
+
+		// undo to reinsert "5"
+		currentRevertStack = redoRevertibles;
+		revertSharedStringRevertibles(sharedString, undoRevertibles.splice(0));
+		currentRevertStack = undoRevertibles;
+		containerRuntimeFactory.processAllMessages();
+		assert.equal(sharedString.getText(), "123456789");
+		assertSequenceIntervals(sharedString, collection, [{ start: 2, end: 2 }]);
+	});
 });
 describe("Sequence.Revertibles with Remote Edits", () => {
 	let sharedString: SharedString;
@@ -850,6 +908,7 @@ describe("Undo/redo for string remove containing intervals", () => {
 		});
 
 		const interval = collection.add({ start: 2, end: 4 });
+		const id = interval.getIntervalId();
 
 		sharedString.removeRange(0, 6);
 
@@ -861,15 +920,17 @@ describe("Undo/redo for string remove containing intervals", () => {
 		assert.equal(revertibles.length, 1, "revertibles.length is not 1");
 		revertSharedStringRevertibles(sharedString, revertibles.splice(0));
 
+		const updatedInterval = collection.getIntervalById(id);
+		assert(updatedInterval !== undefined, "updatedInterval is undefined");
 		assert.equal(sharedString.getText(), "hello world");
 		assertSequenceIntervals(sharedString, collection, [{ start: 2, end: 4 }]);
 		assert.equal(
-			interval.start.getOffset(),
+			updatedInterval.start.getOffset(),
 			2,
 			`after remove start.getOffset() is ${interval.start.getOffset()}`,
 		);
 		assert.equal(
-			interval.end.getOffset(),
+			updatedInterval.end.getOffset(),
 			4,
 			`after remove start.getOffset() is ${interval.end.getOffset()}`,
 		);