Improved Store API, more resilient handling of actions to eliminate p…

…otential concurrency issues
excalidraw · Apr 18, 2024 · 1f2cb61 · 1f2cb61
1 parent 530617b
commit 1f2cb61
Show file tree

Hide file tree

Showing 15 changed files with 333 additions and 187 deletions.
diff --git a/excalidraw-app/App.tsx b/excalidraw-app/App.tsx
@@ -26,7 +26,8 @@ import {
   LiveCollaborationTrigger,
   TTDDialog,
   TTDDialogTrigger,
-} from "../packages/excalidraw/index";
+  StoreAction,
+} from "../packages/excalidraw";
 import {
   AppState,
   ExcalidrawImperativeAPI,
@@ -438,7 +439,7 @@ const ExcalidrawWrapper = () => {
             excalidrawAPI.updateScene({
               ...data.scene,
               ...restore(data.scene, null, null, { repairBindings: true }),
-              commitToStore: true,
+              storeAction: StoreAction.CAPTURE,
             });
           }
         });
@@ -469,6 +470,7 @@ const ExcalidrawWrapper = () => {
           setLangCode(langCode);
           excalidrawAPI.updateScene({
             ...localDataState,
+            storeAction: StoreAction.UPDATE,
           });
           LibraryIndexedDBAdapter.load().then((data) => {
             if (data) {
@@ -604,6 +606,7 @@ const ExcalidrawWrapper = () => {
           if (didChange) {
             excalidrawAPI.updateScene({
               elements,
+              storeAction: StoreAction.UPDATE,
             });
           }
         }

diff --git a/excalidraw-app/collab/Collab.tsx b/excalidraw-app/collab/Collab.tsx
@@ -13,10 +13,11 @@ import {
   OrderedExcalidrawElement,
 } from "../../packages/excalidraw/element/types";
 import {
+  StoreAction,
   getSceneVersion,
   restoreElements,
   zoomToFitBounds,
-} from "../../packages/excalidraw/index";
+} from "../../packages/excalidraw";
 import { Collaborator, Gesture } from "../../packages/excalidraw/types";
 import {
   assertNever,
@@ -356,6 +357,7 @@ class Collab extends PureComponent<CollabProps, CollabState> {
 
       this.excalidrawAPI.updateScene({
         elements,
+        storeAction: StoreAction.UPDATE,
       });
     }
   };
@@ -506,6 +508,7 @@ class Collab extends PureComponent<CollabProps, CollabState> {
       // to database even if deleted before creating the room.
       this.excalidrawAPI.updateScene({
         elements,
+        storeAction: StoreAction.UPDATE,
       });
 
       this.saveCollabRoomToFirebase(getSyncableElements(elements));
@@ -743,6 +746,7 @@ class Collab extends PureComponent<CollabProps, CollabState> {
   ) => {
     this.excalidrawAPI.updateScene({
       elements,
+      storeAction: StoreAction.UPDATE,
     });
 
     this.loadImageFiles();

diff --git a/excalidraw-app/collab/Portal.tsx b/excalidraw-app/collab/Portal.tsx
@@ -19,6 +19,7 @@ import throttle from "lodash.throttle";
 import { newElementWith } from "../../packages/excalidraw/element/mutateElement";
 import { encryptData } from "../../packages/excalidraw/data/encryption";
 import type { Socket } from "socket.io-client";
+import { StoreAction } from "../../packages/excalidraw";
 
 class Portal {
   collab: TCollabClass;
@@ -127,6 +128,7 @@ class Portal {
           }
           return element;
         }),
+      storeAction: StoreAction.UPDATE,
     });
   }, FILE_UPLOAD_TIMEOUT);
 

diff --git a/excalidraw-app/data/FileManager.ts b/excalidraw-app/data/FileManager.ts
@@ -1,3 +1,4 @@
+import { StoreAction } from "../../packages/excalidraw";
 import { compressData } from "../../packages/excalidraw/data/encode";
 import { newElementWith } from "../../packages/excalidraw/element/mutateElement";
 import { isInitializedImageElement } from "../../packages/excalidraw/element/typeChecks";
@@ -238,5 +239,6 @@ export const updateStaleImageStatuses = (params: {
         }
         return element;
       }),
+    storeAction: StoreAction.UPDATE,
   });
 };
diff --git a/excalidraw-app/data/index.ts b/excalidraw-app/data/index.ts
@@ -269,7 +269,6 @@ export const loadScene = async (
     // in the scene database/localStorage, and instead fetch them async
     // from a different database
     files: data.files,
-    commitToStore: false,
   };
 };
 

diff --git a/excalidraw-app/tests/collab.test.tsx b/excalidraw-app/tests/collab.test.tsx
@@ -12,7 +12,7 @@ import {
   createRedoAction,
   createUndoAction,
 } from "../../packages/excalidraw/actions/actionHistory";
-import { newElementWith } from "../../packages/excalidraw";
+import { StoreAction, newElementWith } from "../../packages/excalidraw";
 
 const { h } = window;
 
@@ -90,15 +90,15 @@ describe("collaboration", () => {
 
     updateSceneData({
       elements: syncInvalidIndices([rect1, rect2]),
-      commitToStore: true,
+      storeAction: StoreAction.CAPTURE,
     });
 
     updateSceneData({
       elements: syncInvalidIndices([
         rect1,
         newElementWith(h.elements[1], { isDeleted: true }),
       ]),
-      commitToStore: true,
+      storeAction: StoreAction.CAPTURE,
     });
 
     await waitFor(() => {
@@ -145,6 +145,7 @@ describe("collaboration", () => {
     // simulate force deleting the element remotely
     updateSceneData({
       elements: syncInvalidIndices([rect1]),
+      storeAction: StoreAction.UPDATE,
     });
 
     await waitFor(() => {
@@ -182,7 +183,7 @@ describe("collaboration", () => {
         h.elements[0],
         newElementWith(h.elements[1], { x: 100 }),
       ]),
-      commitToStore: true,
+      storeAction: StoreAction.CAPTURE,
     });
 
     await waitFor(() => {
@@ -217,6 +218,7 @@ describe("collaboration", () => {
     // simulate force deleting the element remotely
     updateSceneData({
       elements: syncInvalidIndices([rect1]),
+      storeAction: StoreAction.UPDATE,
     });
 
     // snapshot was correctly updated and marked the element as deleted

diff --git a/packages/excalidraw/actions/actionHistory.tsx b/packages/excalidraw/actions/actionHistory.tsx
@@ -8,7 +8,7 @@ import { KEYS } from "../keys";
 import { arrayToMap } from "../utils";
 import { isWindows } from "../constants";
 import { SceneElementsMap } from "../element/types";
-import { IStore, StoreAction } from "../store";
+import { Store, StoreAction } from "../store";
 import { useEmitter } from "../hooks/useEmitter";
 
 const writeData = (
@@ -40,7 +40,7 @@ const writeData = (
   return { storeAction: StoreAction.NONE };
 };
 
-type ActionCreator = (history: History, store: IStore) => Action;
+type ActionCreator = (history: History, store: Store) => Action;
 
 export const createUndoAction: ActionCreator = (history, store) => ({
   name: "undo",

diff --git a/packages/excalidraw/actions/types.ts b/packages/excalidraw/actions/types.ts
@@ -8,7 +8,7 @@ import {
   UIAppState,
 } from "../types";
 import { MarkOptional } from "../utility-types";
-import { StoreAction } from "../store";
+import { StoreActionType } from "../store";
 
 export type ActionSource =
   | "ui"
@@ -26,7 +26,7 @@ export type ActionResult =
         "offsetTop" | "offsetLeft" | "width" | "height"
       > | null;
       files?: BinaryFiles | null;
-      storeAction: keyof typeof StoreAction;
+      storeAction: StoreActionType;
       replaceFiles?: boolean;
     }
   | false;

diff --git a/packages/excalidraw/components/App.tsx b/packages/excalidraw/components/App.tsx
@@ -183,7 +183,6 @@ import {
   ExcalidrawIframeElement,
   ExcalidrawEmbeddableElement,
   Ordered,
-  OrderedExcalidrawElement,
 } from "../element/types";
 import { getCenter, getDistance } from "../gesture";
 import {
@@ -412,7 +411,7 @@ import { ElementCanvasButton } from "./MagicButton";
 import { MagicIcon, copyIcon, fullscreenIcon } from "./icons";
 import { EditorLocalStorage } from "../data/EditorLocalStorage";
 import FollowMode from "./FollowMode/FollowMode";
-import { IStore, Store, StoreAction } from "../store";
+import { Store, StoreAction } from "../store";
 import { AnimationFrameHandler } from "../animation-frame-handler";
 import { AnimatedTrail } from "../animated-trail";
 import { LaserTrails } from "../laser-trails";
@@ -543,7 +542,7 @@ class App extends React.Component<AppProps, AppState> {
   public library: AppClassProperties["library"];
   public libraryItemsFromStorage: LibraryItems | undefined;
   public id: string;
-  private store: IStore;
+  private store: Store;
   private history: History;
   private excalidrawContainerValue: {
     container: HTMLDivElement | null;
@@ -2123,7 +2122,7 @@ class App extends React.Component<AppProps, AppState> {
               }
               return el;
             }),
-            commitToStore: true,
+            storeAction: StoreAction.CAPTURE,
           });
         }
       },
@@ -2810,7 +2809,7 @@ class App extends React.Component<AppProps, AppState> {
       );
     }
 
-    this.store.capture(elementsMap, this.state);
+    this.store.commit(elementsMap, this.state);
 
     // Do not notify consumers if we're still loading the scene. Among other
     // potential issues, this fixes a case where the tab isn't focused during
@@ -3683,51 +3682,38 @@ class App extends React.Component<AppProps, AppState> {
       elements?: SceneData["elements"];
       appState?: Pick<AppState, K> | null;
       collaborators?: SceneData["collaborators"];
-      commitToStore?: SceneData["commitToStore"];
+      storeAction?: SceneData["storeAction"];
     }) => {
       const nextElements = syncInvalidIndices(sceneData.elements ?? []);
 
-      if (sceneData.commitToStore) {
-        this.store.shouldCaptureIncrement();
-      }
-
-      if (sceneData.elements || sceneData.appState) {
-        let nextCommittedAppState = this.state;
-        let nextCommittedElements: Map<string, OrderedExcalidrawElement>;
-
-        if (sceneData.appState) {
-          nextCommittedAppState = {
-            ...this.state,
-            ...sceneData.appState, // Here we expect just partial appState
-          };
-        }
+      if (sceneData.storeAction && sceneData.storeAction !== StoreAction.NONE) {
+        const prevCommittedAppState = this.store.snapshot.appState;
+        const prevCommittedElements = this.store.snapshot.elements;
 
-        const prevElements = this.scene.getElementsIncludingDeleted();
-
-        if (sceneData.elements) {
-          /**
-           * We need to schedule a snapshot update, as in case `commitToStore` is false  (i.e. remote update),
-           * as it's essential for computing local changes after the async action is completed (i.e. not to include remote changes in the diff).
-           *
-           * This is also a breaking change for all local `updateScene` calls without set `commitToStore` to true,
-           * as it makes such updates impossible to undo (previously they were undone coincidentally with the switch to the whole snapshot captured by the history).
-           *
-           * WARN: be careful here as moving it elsewhere could break the history for remote client without noticing
-           * - we need to find a way to test two concurrent client updates simultaneously, while having access to both stores & histories.
-           */
-          this.store.shouldUpdateSnapshot();
+        const nextCommittedAppState = sceneData.appState
+          ? Object.assign({}, prevCommittedAppState, sceneData.appState) // new instance, with partial appstate applied to previously captured one, including hidden prop inside `prevCommittedAppState`
+          : prevCommittedAppState;
 
-          // TODO#7348: deprecate once exchanging just store increments between clients
-          nextCommittedElements = this.store.ignoreUncomittedElements(
-            arrayToMap(prevElements),
-            arrayToMap(nextElements),
+        const nextCommittedElements = sceneData.elements
+          ? this.store.filterUncomittedElements(
+              this.scene.getElementsMapIncludingDeleted(), // Only used to detect uncomitted local elements
+              arrayToMap(nextElements), // We expect all (already reconciled) elements
+            )
+          : prevCommittedElements;
+
+        // WARN: store action always performs deep clone of changed elements, for ephemeral remote updates (i.e. remote dragging, resizing, drawing) we might consider doing something smarter
+        // NOT scheduling actions after re-render, as it might cause unexpected concurrency issues
+        if (sceneData.storeAction === StoreAction.CAPTURE) {
+          this.store.captureIncrement(
+            nextCommittedElements,
+            nextCommittedAppState,
+          );
+        } else if (sceneData.storeAction === StoreAction.UPDATE) {
+          this.store.updateSnapshot(
+            nextCommittedElements,
+            nextCommittedAppState,
           );
-        } else {
-          nextCommittedElements = arrayToMap(prevElements);
         }
-
-        // WARN: Performs deep clone of changed elements, for ephemeral remote updates (i.e. remote dragging, resizing, drawing) we might consider doing something smarter
-        this.store.capture(nextCommittedElements, nextCommittedAppState);
       }
 
       if (sceneData.appState) {
@@ -5704,6 +5690,7 @@ class App extends React.Component<AppProps, AppState> {
             this.state,
           ),
         },
+        storeAction: StoreAction.UPDATE,
       });
       return;
     }
@@ -7993,6 +7980,7 @@ class App extends React.Component<AppProps, AppState> {
           appState: {
             draggingElement: null,
           },
+          storeAction: StoreAction.UPDATE,
         });
 
         return;
@@ -8165,6 +8153,7 @@ class App extends React.Component<AppProps, AppState> {
           elements: this.scene
             .getElementsIncludingDeleted()
             .filter((el) => el.id !== resizingElement.id),
+          storeAction: StoreAction.UPDATE,
         });
       }
 
@@ -9228,13 +9217,12 @@ class App extends React.Component<AppProps, AppState> {
       }
 
       if (ret.type === MIME_TYPES.excalidraw) {
-        // Restore the fractional indices by mutating elements and update the
-        // store snapshot, otherwise we would end up with duplicate indices
+        // restore the fractional indices by mutating elements
         syncInvalidIndices(elements.concat(ret.data.elements));
-        this.store.snapshot = this.store.snapshot.clone(
-          arrayToMap(elements),
-          this.state,
-        );
+
+        // update the store snapshot for old elements, otherwise we would end up with duplicated fractional indices on undo
+        this.store.updateSnapshot(arrayToMap(elements), this.state);
+
         this.setState({ isLoading: true });
         this.syncActionResult({
           ...ret.data,

diff --git a/packages/excalidraw/element/linearElementEditor.ts b/packages/excalidraw/element/linearElementEditor.ts
@@ -49,7 +49,7 @@ import { getBoundTextElement, handleBindTextResize } from "./textElement";
 import { DRAGGING_THRESHOLD } from "../constants";
 import { Mutable } from "../utility-types";
 import { ShapeCache } from "../scene/ShapeCache";
-import { IStore } from "../store";
+import { Store } from "../store";
 
 const editorMidPointsCache: {
   version: number | null;
@@ -642,7 +642,7 @@ export class LinearElementEditor {
   static handlePointerDown(
     event: React.PointerEvent<HTMLElement>,
     appState: AppState,
-    store: IStore,
+    store: Store,
     scenePointer: { x: number; y: number },
     linearElementEditor: LinearElementEditor,
     app: AppClassProperties,

diff --git a/packages/excalidraw/index.tsx b/packages/excalidraw/index.tsx
@@ -251,6 +251,8 @@ export {
   bumpVersion,
 } from "./element/mutateElement";
 
+export { StoreAction } from "./store";
+
 export { parseLibraryTokensFromUrl, useHandleLibrary } from "./data/library";
 
 export {