Fix many syncing issues (#952)

.gitignore

@@ -9,3 +9,5 @@ static
 yarn-debug.log*
 yarn-error.log*
 yarn.lock
+.envrc
+firebase/

src/actions/actionCanvas.tsx

@@ -9,6 +9,7 @@ import { KEYS } from "../keys";
 import { getShortcutKey } from "../utils";
 import useIsMobile from "../is-mobile";
 import { register } from "./register";
+import { newElementWith } from "../element/mutateElement";
 
 export const actionChangeViewBackgroundColor = register({
   name: "changeViewBackgroundColor",
@@ -33,9 +34,11 @@ export const actionChangeViewBackgroundColor = register({
 export const actionClearCanvas = register({
   name: "clearCanvas",
   commitToHistory: () => true,
-  perform: () => {
+  perform: elements => {
     return {
-      elements: [],
+      elements: elements.map(element =>
+        newElementWith(element, { isDeleted: true }),
+      ),
       appState: getDefaultAppState(),
     };
   },

src/actions/actionHistory.tsx

@@ -7,8 +7,11 @@ import { SceneHistory } from "../history";
 import { ExcalidrawElement } from "../element/types";
 import { AppState } from "../types";
 import { KEYS } from "../keys";
+import { getElementMap } from "../element";
+import { newElementWith } from "../element/mutateElement";
 
 const writeData = (
+  prevElements: readonly ExcalidrawElement[],
   appState: AppState,
   updater: () => { elements: ExcalidrawElement[]; appState: AppState } | null,
 ) => {
@@ -19,13 +22,32 @@ const writeData = (
     !appState.draggingElement
   ) {
     const data = updater();
+    if (data === null) {
+      return {};
+    }
 
-    return data === null
-      ? {}
-      : {
-          elements: data.elements,
-          appState: { ...appState, ...data.appState },
-        };
+    const prevElementMap = getElementMap(prevElements);
+    const nextElements = data.elements;
+    const nextElementMap = getElementMap(nextElements);
+    return {
+      elements: nextElements
+        .map(nextElement =>
+          newElementWith(
+            prevElementMap[nextElement.id] || nextElement,
+            nextElement,
+          ),
+        )
+        .concat(
+          prevElements
+            .filter(
+              prevElement => !nextElementMap.hasOwnProperty(prevElement.id),
+            )
+            .map(prevElement =>
+              newElementWith(prevElement, { isDeleted: true }),
+            ),
+        ),
+      appState: { ...appState, ...data.appState },
+    };
   }
   return {};
 };
@@ -37,7 +59,8 @@ type ActionCreator = (history: SceneHistory) => Action;
 
 export const createUndoAction: ActionCreator = history => ({
   name: "undo",
-  perform: (_, appState) => writeData(appState, () => history.undoOnce()),
+  perform: (elements, appState) =>
+    writeData(elements, appState, () => history.undoOnce()),
   keyTest: testUndo(false),
   PanelComponent: ({ updateData }) => (
     <ToolButton
@@ -52,7 +75,8 @@ export const createUndoAction: ActionCreator = history => ({
 
 export const createRedoAction: ActionCreator = history => ({
   name: "redo",
-  perform: (_, appState) => writeData(appState, () => history.redoOnce()),
+  perform: (elements, appState) =>
+    writeData(elements, appState, () => history.redoOnce()),
   keyTest: testUndo(true),
   PanelComponent: ({ updateData }) => (
     <ToolButton

src/appState.ts

@@ -34,7 +34,6 @@ export function getDefaultAppState(): AppState {
     openMenu: null,
     lastPointerDownWith: "mouse",
     selectedElementIds: {},
-    deletedIds: {},
     collaborators: new Map(),
   };
 }

src/components/App.tsx

@@ -18,6 +18,10 @@ import {
   getCursorForResizingElement,
   getPerfectElementSize,
   normalizeDimensions,
+  getElementMap,
+  getDrawingVersion,
+  getSyncableElements,
+  hasNonDeletedElements,
 } from "../element";
 import {
   deleteSelectedElements,
@@ -160,6 +164,7 @@ export class App extends React.Component<any, AppState> {
   socketInitialized: boolean = false; // we don't want the socket to emit any updates until it is fully initalized
   roomID: string | null = null;
   roomKey: string | null = null;
+  lastBroadcastedOrReceivedSceneVersion: number = -1;
 
   actionManager: ActionManager;
   canvasOnlyActions = ["selectAll"];
@@ -275,15 +280,11 @@ export class App extends React.Component<any, AppState> {
             iv,
           );
 
-          let deletedIds = this.state.deletedIds;
           switch (decryptedData.type) {
             case "INVALID_RESPONSE":
               return;
             case "SCENE_UPDATE":
-              const {
-                elements: remoteElements,
-                appState: remoteAppState,
-              } = decryptedData.payload;
+              const { elements: remoteElements } = decryptedData.payload;
               const restoredState = restore(remoteElements || [], null, {
                 scrollToContent: true,
               });
@@ -295,32 +296,7 @@ export class App extends React.Component<any, AppState> {
               } else {
                 // create a map of ids so we don't have to iterate
                 // over the array more than once.
-                const localElementMap = elements.reduce(
-                  (
-                    acc: { [key: string]: ExcalidrawElement },
-                    element: ExcalidrawElement,
-                  ) => {
-                    acc[element.id] = element;
-                    return acc;
-                  },
-                  {},
-                );
-
-                deletedIds = { ...deletedIds };
-
-                for (const [id, remoteDeletedEl] of Object.entries(
-                  remoteAppState.deletedIds,
-                )) {
-                  if (
-                    !localElementMap[id] ||
-                    // don't remove local element if it's newer than the one
-                    //  deleted on remote
-                    remoteDeletedEl.version >= localElementMap[id].version
-                  ) {
-                    deletedIds[id] = remoteDeletedEl;
-                    delete localElementMap[id];
-                  }
-                }
+                const localElementMap = getElementMap(elements);
 
                 // Reconcile
                 elements = restoredState.elements
@@ -342,17 +318,27 @@ export class App extends React.Component<any, AppState> {
                     ) {
                       elements.push(localElementMap[element.id]);
                       delete localElementMap[element.id];
-                    } else {
-                      if (deletedIds.hasOwnProperty(element.id)) {
-                        if (element.version > deletedIds[element.id].version) {
-                          elements.push(element);
-                          delete deletedIds[element.id];
-                          delete localElementMap[element.id];
-                        }
+                    } else if (
+                      localElementMap.hasOwnProperty(element.id) &&
+                      localElementMap[element.id].version === element.version &&
+                      localElementMap[element.id].versionNonce !==
+                        element.versionNonce
+                    ) {
+                      // resolve conflicting edits deterministically by taking the one with the lowest versionNonce
+                      if (
+                        localElementMap[element.id].versionNonce <
+                        element.versionNonce
+                      ) {
+                        elements.push(localElementMap[element.id]);
                       } else {
+                        // it should be highly unlikely that the two versionNonces are the same. if we are
+                        // really worried about this, we can replace the versionNonce with the socket id.
                         elements.push(element);
-                        delete localElementMap[element.id];
                       }
+                      delete localElementMap[element.id];
+                    } else {
+                      elements.push(element);
+                      delete localElementMap[element.id];
                     }
 
                     return elements;
@@ -360,9 +346,15 @@ export class App extends React.Component<any, AppState> {
                   // add local elements that weren't deleted or on remote
                   .concat(...Object.values(localElementMap));
               }
-              this.setState({
-                deletedIds,
-              });
+              this.lastBroadcastedOrReceivedSceneVersion = getDrawingVersion(
+                elements,
+              );
+              // We haven't yet implemented multiplayer undo functionality, so we clear the undo stack
+              // when we receive any messages from another peer. This UX can be pretty rough -- if you
+              // undo, a user makes a change, and then try to redo, your element(s) will be lost. However,
+              // right now we think this is the right tradeoff.
+              history.clear();
+              this.setState({});
               if (this.socketInitialized === false) {
                 this.socketInitialized = true;
               }
@@ -370,13 +362,13 @@ export class App extends React.Component<any, AppState> {
             case "MOUSE_LOCATION":
               const { socketID, pointerCoords } = decryptedData.payload;
               this.setState(state => {
-                if (state.collaborators.has(socketID)) {
-                  const user = state.collaborators.get(socketID)!;
-                  user.pointer = pointerCoords;
-                  state.collaborators.set(socketID, user);
-                  return state;
+                if (!state.collaborators.has(socketID)) {
+                  state.collaborators.set(socketID, {});
                 }
-                return null;
+                const user = state.collaborators.get(socketID)!;
+                user.pointer = pointerCoords;
+                state.collaborators.set(socketID, user);
+                return state;
               });
               break;
           }
@@ -428,24 +420,16 @@ export class App extends React.Component<any, AppState> {
   };
 
   private broadcastSceneUpdate = () => {
-    const deletedIds = { ...this.state.deletedIds };
-    const _elements = elements.filter(element => {
-      if (element.id in deletedIds) {
-        delete deletedIds[element.id];
-      }
-      return element.id !== this.state.editingElement?.id;
-    });
     const data: SocketUpdateDataSource["SCENE_UPDATE"] = {
       type: "SCENE_UPDATE",
       payload: {
-        elements: _elements,
-        appState: {
-          viewBackgroundColor: this.state.viewBackgroundColor,
-          name: this.state.name,
-          deletedIds,
-        },
+        elements: getSyncableElements(elements),
       },
     };
+    this.lastBroadcastedOrReceivedSceneVersion = Math.max(
+      this.lastBroadcastedOrReceivedSceneVersion,
+      getDrawingVersion(elements),
+    );
     return this._broadcastSocketData(
       data as typeof data & { _brand: "socketUpdateData" },
     );
@@ -840,7 +824,7 @@ export class App extends React.Component<any, AppState> {
                       action: () => this.pasteFromClipboard(null),
                     },
                     probablySupportsClipboardBlob &&
-                      elements.length > 0 && {
+                      hasNonDeletedElements(elements) && {
                         label: t("labels.copyAsPng"),
                         action: this.copyToClipboardAsPng,
                       },
@@ -1102,6 +1086,7 @@ export class App extends React.Component<any, AppState> {
       const pnt = points[points.length - 1];
       pnt[0] = x - originX;
       pnt[1] = y - originY;
+      mutateElement(multiElement);
       invalidateShapeForElement(multiElement);
       this.setState({});
       return;
@@ -1485,6 +1470,7 @@ export class App extends React.Component<any, AppState> {
           },
         }));
         multiElement.points.push([x - rx, y - ry]);
+        mutateElement(multiElement);
         invalidateShapeForElement(multiElement);
       } else {
         this.setState(prevState => ({
@@ -1494,6 +1480,7 @@ export class App extends React.Component<any, AppState> {
           },
         }));
         element.points.push([0, 0]);
+        mutateElement(element);
         invalidateShapeForElement(element);
         elements = [...elements, element];
         this.setState({
@@ -1548,20 +1535,19 @@ export class App extends React.Component<any, AppState> {
 
         const dx = element.x + width + p1[0];
         const dy = element.y + height + p1[1];
+        p1[0] = absPx - element.x;
+        p1[1] = absPy - element.y;
         mutateElement(element, {
           x: dx,
           y: dy,
         });
-        p1[0] = absPx - element.x;
-        p1[1] = absPy - element.y;
       } else {
+        p1[0] -= deltaX;
+        p1[1] -= deltaY;
         mutateElement(element, {
           x: element.x + deltaX,
           y: element.y + deltaY,
         });
-
-        p1[0] -= deltaX;
-        p1[1] -= deltaY;
       }
     };
 
@@ -1586,6 +1572,7 @@ export class App extends React.Component<any, AppState> {
         p1[0] += deltaX;
         p1[1] += deltaY;
       }
+      mutateElement(element);
     };
 
     const onPointerMove = (event: PointerEvent) => {
@@ -1925,6 +1912,8 @@ export class App extends React.Component<any, AppState> {
           pnt[0] = dx;
           pnt[1] = dy;
         }
+
+        mutateElement(draggingElement, { points });
       } else {
         if (event.shiftKey) {
           ({ width, height } = getPerfectElementSize(
@@ -2005,6 +1994,7 @@ export class App extends React.Component<any, AppState> {
             x - draggingElement.x,
             y - draggingElement.y,
           ]);
+          mutateElement(draggingElement);
           invalidateShapeForElement(draggingElement);
           this.setState({
             multiElement: this.state.draggingElement,
@@ -2263,13 +2253,20 @@ export class App extends React.Component<any, AppState> {
     if (scrollBars) {
       currentScrollBars = scrollBars;
     }
-    const scrolledOutside = !atLeastOneVisibleElement && elements.length > 0;
+    const scrolledOutside =
+      !atLeastOneVisibleElement && hasNonDeletedElements(elements);
     if (this.state.scrolledOutside !== scrolledOutside) {
       this.setState({ scrolledOutside: scrolledOutside });
     }
     this.saveDebounced();
-    if (history.isRecording()) {
+
+    if (
+      getDrawingVersion(elements) > this.lastBroadcastedOrReceivedSceneVersion
+    ) {
       this.broadcastSceneUpdate();
+    }
+
+    if (history.isRecording()) {
       history.pushEntry(this.state, elements);
       history.skipRecording();
     }

src/data/index.ts

@@ -13,6 +13,7 @@ import { serializeAsJSON } from "./json";
 import { ExportType } from "../scene/types";
 import { restore } from "./restore";
 import { restoreFromLocalStorage } from "./localStorage";
+import { hasNonDeletedElements } from "../element";
 
 export { loadFromBlob } from "./blob";
 export { saveAsJSON, loadFromJSON } from "./json";
@@ -35,7 +36,6 @@ export type SocketUpdateDataSource = {
     type: "SCENE_UPDATE";
     payload: {
       elements: readonly ExcalidrawElement[];
-      appState: Pick<AppState, "viewBackgroundColor" | "name" | "deletedIds">;
     };
   };
   MOUSE_LOCATION: {
@@ -288,7 +288,7 @@ export async function exportCanvas(
     scale?: number;
   },
 ) {
-  if (!elements.length) {
+  if (!hasNonDeletedElements(elements)) {
     return window.alert(t("alerts.cannotExportEmptyCanvas"));
   }
   // calculate smallest area to fit the contents in

src/data/restore.ts

@@ -52,7 +52,8 @@ export function restore(
 
       return {
         ...element,
-        version: element.version || 0,
+        // all elements must have version > 0 so getDrawingVersion() will pick up newly added elements
+        version: element.version || 1,
         id: element.id || nanoid(),
         fillStyle: element.fillStyle || "hachure",
         strokeWidth: element.strokeWidth || 1,

src/element/index.ts

@@ -1,3 +1,6 @@
+import { ExcalidrawElement } from "./types";
+import { isInvisiblySmallElement } from "./sizeHelpers";
+
 export { newElement, newTextElement, duplicateElement } from "./newElement";
 export {
   getElementAbsoluteCoords,
@@ -24,3 +27,28 @@ export {
   normalizeDimensions,
 } from "./sizeHelpers";
 export { showSelectedShapeActions } from "./showSelectedShapeActions";
+
+export function getSyncableElements(elements: readonly ExcalidrawElement[]) {
+  // There are places in Excalidraw where synthetic invisibly small elements are added and removed.
+  // It's probably best to keep those local otherwise there might be a race condition that
+  // gets the app into an invalid state. I've never seen it happen but I'm worried about it :)
+  return elements.filter(el => !isInvisiblySmallElement(el));
+}
+
+export function getElementMap(elements: readonly ExcalidrawElement[]) {
+  return getSyncableElements(elements).reduce(
+    (acc: { [key: string]: ExcalidrawElement }, element: ExcalidrawElement) => {
+      acc[element.id] = element;
+      return acc;
+    },
+    {},
+  );
+}
+
+export function getDrawingVersion(elements: readonly ExcalidrawElement[]) {
+  return elements.reduce((acc, el) => acc + el.version, 0);
+}
+
+export function hasNonDeletedElements(elements: readonly ExcalidrawElement[]) {
+  return elements.some(element => !element.isDeleted);
+}

src/element/mutateElement.ts

@@ -1,4 +1,5 @@
 import { ExcalidrawElement, ExcalidrawTextElement } from "./types";
+import { randomSeed } from "roughjs/bin/math";
 
 type ElementUpdate<TElement extends ExcalidrawElement> = Omit<
   Partial<TElement>,
@@ -10,17 +11,25 @@ type ElementUpdate<TElement extends ExcalidrawElement> = Omit<
 // the same drawing.
 export function mutateElement(
   element: ExcalidrawElement,
-  updates: ElementUpdate<ExcalidrawElement>,
+  updates?: ElementUpdate<ExcalidrawElement>,
 ) {
-  Object.assign(element, updates);
+  if (updates) {
+    Object.assign(element, updates);
+  }
   (element as any).version++;
+  (element as any).versionNonce = randomSeed();
 }
 
 export function newElementWith(
   element: ExcalidrawElement,
   updates: ElementUpdate<ExcalidrawElement>,
 ): ExcalidrawElement {
-  return { ...element, ...updates, version: element.version + 1 };
+  return {
+    ...element,
+    ...updates,
+    version: element.version + 1,
+    versionNonce: randomSeed(),
+  };
 }
 
 // This function tracks updates of text elements for the purposes for collaboration.
@@ -32,11 +41,17 @@ export function mutateTextElement(
 ): void {
   Object.assign(element, updates);
   (element as any).version++;
+  (element as any).versionNonce = randomSeed();
 }
 
 export function newTextElementWith(
   element: ExcalidrawTextElement,
   updates: ElementUpdate<ExcalidrawTextElement>,
 ): ExcalidrawTextElement {
-  return { ...element, ...updates, version: element.version + 1 };
+  return {
+    ...element,
+    ...updates,
+    version: element.version + 1,
+    versionNonce: randomSeed(),
+  };
 }

src/element/newElement.ts

@@ -34,6 +34,8 @@ export function newElement(
     seed: randomSeed(),
     points: [] as Point[],
     version: 1,
+    versionNonce: 0,
+    isDeleted: false,
   };
   return element;
 }

src/history.ts

@@ -13,6 +13,11 @@ export class SceneHistory {
   private stateHistory: string[] = [];
   private redoStack: string[] = [];
 
+  clear() {
+    this.stateHistory.length = 0;
+    this.redoStack.length = 0;
+  }
+
   private generateEntry(
     appState: AppState,
     elements: readonly ExcalidrawElement[],

src/renderer/renderScene.ts

@@ -24,7 +24,7 @@ function colorForClientId(clientId: string) {
 }
 
 export function renderScene(
-  elements: readonly ExcalidrawElement[],
+  allElements: readonly ExcalidrawElement[],
   appState: AppState,
   selectionElement: ExcalidrawElement | null,
   scale: number,
@@ -49,6 +49,8 @@ export function renderScene(
     return { atLeastOneVisibleElement: false };
   }
 
+  const elements = allElements.filter(element => !element.isDeleted);
+
   const context = canvas.getContext("2d")!;
 
   // When doing calculations based on canvas width we should used normalized one

src/scene/comparisons.ts

@@ -25,6 +25,9 @@ export function getElementAtPosition(
   let hitElement = null;
   // We need to to hit testing from front (end of the array) to back (beginning of the array)
   for (let i = elements.length - 1; i >= 0; --i) {
+    if (elements[i].isDeleted) {
+      continue;
+    }
     if (hitTest(elements[i], appState, x, y, zoom)) {
       hitElement = elements[i];
       break;
@@ -42,6 +45,9 @@ export function getElementContainingPosition(
   let hitElement = null;
   // We need to to hit testing from front (end of the array) to back (beginning of the array)
   for (let i = elements.length - 1; i >= 0; --i) {
+    if (elements[i].isDeleted) {
+      continue;
+    }
     const [x1, y1, x2, y2] = getElementAbsoluteCoords(elements[i]);
     if (x1 < x && x < x2 && y1 < y && y < y2) {
       hitElement = elements[i];

src/scene/selection.ts

@@ -1,6 +1,7 @@
 import { ExcalidrawElement } from "../element/types";
 import { getElementAbsoluteCoords } from "../element";
 import { AppState } from "../types";
+import { newElementWith } from "../element/mutateElement";
 
 export function getElementsWithinSelection(
   elements: readonly ExcalidrawElement[],
@@ -34,24 +35,16 @@ export function deleteSelectedElements(
   elements: readonly ExcalidrawElement[],
   appState: AppState,
 ) {
-  const deletedIds: AppState["deletedIds"] = {};
   return {
-    elements: elements.filter(el => {
+    elements: elements.map(el => {
       if (appState.selectedElementIds[el.id]) {
-        deletedIds[el.id] = {
-          version: el.version,
-        };
-        return false;
+        return newElementWith(el, { isDeleted: true });
       }
-      return true;
+      return el;
     }),
     appState: {
       ...appState,
       selectedElementIds: {},
-      deletedIds: {
-        ...appState.deletedIds,
-        ...deletedIds,
-      },
     },
   };
 }

src/types.ts

@@ -34,7 +34,6 @@ export type AppState = {
   openMenu: "canvas" | "shape" | null;
   lastPointerDownWith: PointerType;
   selectedElementIds: { [id: string]: boolean };
-  deletedIds: { [id: string]: { version: ExcalidrawElement["version"] } };
   collaborators: Map<string, { pointer?: { x: number; y: number } }>;
 };