fix(collab): Fix Duplicate commands make editors become out-of-sync

Duplicate commands, when serialized, did not store enough information to be completely reconstructed

Author: personalizedrefrigerator

packages/js-draw/src/commands/Duplicate.test.ts

@@ -0,0 +1,47 @@
+import Duplicate from './Duplicate';
+import { Color4, Path } from '@js-draw/math';
+import { EditorImage, pathToRenderable, SerializableCommand, Stroke } from '../lib';
+import createEditor from '../testing/createEditor';
+
+const getAllStrokeIds = (editorImage: EditorImage) => {
+	const strokes = editorImage.getAllElements().filter((elem) => elem instanceof Stroke);
+	return strokes.map((stroke) => stroke.getId());
+};
+
+describe('Duplicate', () => {
+	test('deserialized Duplicate commands should create clones with the same IDs', async () => {
+		const editor = createEditor();
+
+		const stroke = new Stroke([
+			pathToRenderable(Path.fromString('m0,0 l10,10 l-10,0'), { fill: Color4.red }),
+		]);
+		await editor.dispatch(editor.image.addElement(stroke));
+
+		const command = new Duplicate([stroke]);
+		command.apply(editor);
+
+		// Should have duplicated [element]
+		const strokes1 = getAllStrokeIds(editor.image);
+		expect(strokes1).toHaveLength(2);
+		// Should not have removed the first element
+		expect(strokes1).toContain(stroke.getId());
+		// Clone should have a different ID
+		expect(strokes1.filter((id) => id !== stroke.getId())).toHaveLength(1);
+
+		// Apply a deserialized copy of the command
+		const serialized = command.serialize();
+		const deserialized = SerializableCommand.deserialize(serialized, editor);
+
+		command.unapply(editor);
+		deserialized.apply(editor);
+
+		// The copy should produce a clone with the same ID
+		const strokes2 = getAllStrokeIds(editor.image);
+		expect(strokes1).toEqual(strokes2);
+
+		// It should be possible to unapply the deserialized command
+		deserialized.unapply(editor);
+
+		expect(getAllStrokeIds(editor.image)).toEqual([stroke.getId()]);
+	});
+});

packages/js-draw/src/commands/Duplicate.ts

@@ -2,6 +2,7 @@ import AbstractComponent from '../components/AbstractComponent';
 import describeComponentList from '../components/util/describeComponentList';
 import Editor from '../Editor';
 import { EditorLocalization } from '../localization';
+import { assertIsStringArray } from '../util/assertions';
 import Erase from './Erase';
 import SerializableCommand from './SerializableCommand';
 
@@ -32,10 +33,23 @@ export default class Duplicate extends SerializableCommand {
 	private duplicates: AbstractComponent[];
 	private reverse: Erase;
 
-	public constructor(private toDuplicate: AbstractComponent[]) {
+	public constructor(
+		private toDuplicate: AbstractComponent[],
+
+		// @internal -- IDs given to the duplicate elements
+		idsForDuplicates?: string[],
+	) {
 		super('duplicate');
 
-		this.duplicates = toDuplicate.map((elem) => elem.clone());
+		this.duplicates = toDuplicate.map((elem, idx) => {
+			// For collaborative editing, it's important for the clones to have
+			// the same IDs as the originals
+			if (idsForDuplicates && idsForDuplicates[idx]) {
+				return elem.cloneWithId(idsForDuplicates[idx]);
+			} else {
+				return elem.clone();
+			}
+		});
 		this.reverse = new Erase(this.duplicates);
 	}
 
@@ -63,13 +77,42 @@ export default class Duplicate extends SerializableCommand {
 	}
 
 	protected serializeToJSON() {
-		return this.toDuplicate.map((elem) => elem.getId());
+		return {
+			originalIds: this.toDuplicate.map((elem) => elem.getId()),
+			cloneIds: this.duplicates.map((elem) => elem.getId()),
+		};
 	}
 
 	static {
 		SerializableCommand.register('duplicate', (json: any, editor: Editor) => {
-			const elems = json.map((id: string) => editor.image.lookupElement(id));
-			return new Duplicate(elems);
+			let originalIds;
+			let cloneIds;
+			// Compatibility with older editors
+			if (Array.isArray(json)) {
+				originalIds = json;
+				cloneIds = [];
+			} else {
+				originalIds = json.originalIds;
+				cloneIds = json.cloneIds;
+			}
+			assertIsStringArray(originalIds);
+			assertIsStringArray(cloneIds);
+
+			// Resolve to elements -- only keep the elements that can be found in the image.
+			const resolvedElements = [];
+			const filteredCloneIds = [];
+			for (let i = 0; i < originalIds.length; i++) {
+				const originalId = originalIds[i];
+				const foundElement = editor.image.lookupElement(originalId);
+				if (!foundElement) {
+					console.warn('Duplicate command: Could not find element with ID', originalId);
+				} else {
+					filteredCloneIds.push(cloneIds[i]);
+					resolvedElements.push(foundElement);
+				}
+			}
+
+			return new Duplicate(resolvedElements, filteredCloneIds);
 		});
 	}
 }

packages/js-draw/src/components/AbstractComponent.ts

@@ -9,6 +9,7 @@ import UnresolvedSerializableCommand from '../commands/UnresolvedCommand';
 import Viewport from '../Viewport';
 import { Point2 } from '@js-draw/math';
 import describeTransformation from '../util/describeTransformation';
+import { assertIsString } from '../util/assertions';
 
 export type LoadSaveData = string[] | Record<symbol, string | number>;
 export type LoadSaveDataTable = Record<string, Array<LoadSaveData>>;
@@ -426,6 +427,18 @@ export default abstract class AbstractComponent {
 		return clone;
 	}
 
+	/**
+	 * Creates a copy of this component with a particular `id`.
+	 * This is used internally by {@link Duplicate} when deserializing.
+	 *
+	 * @internal -- users of the library shouldn't need this.
+	 */
+	public cloneWithId(cloneId: string) {
+		const clone = this.clone();
+		clone.id = cloneId;
+		return clone;
+	}
+
 	/**
 	 * **Optional method**: Divides this component into sections roughly along the given path,
 	 * removing parts that are roughly within `shape`.
@@ -494,6 +507,8 @@ export default abstract class AbstractComponent {
 			throw new Error(`Element with data ${json} cannot be deserialized.`);
 		}
 
+		assertIsString(json.id);
+
 		const instance = this.deserializationCallbacks[json.name]!(json.data);
 		instance.id = json.id;