Fix history initialization (#2115)

src/actions/actionExport.tsx

@@ -125,7 +125,7 @@ export const actionLoadScene = register({
         ...loadedAppState,
         errorMessage: error,
       },
-      commitToHistory: false,
+      commitToHistory: true,
     };
   },
   PanelComponent: ({ updateData, appState }) => (

src/components/App.tsx

@@ -556,7 +556,11 @@ class App extends React.Component<ExcalidrawProps, AppState> {
           ),
         };
       }
-      this.syncActionResult(scene);
+      history.clear();
+      this.syncActionResult({
+        ...scene,
+        commitToHistory: true,
+      });
     }
   };
 
@@ -1163,11 +1167,12 @@ class App extends React.Component<ExcalidrawProps, AppState> {
 
       const updateScene = (
         decryptedData: SocketUpdateDataSource[SCENE.INIT | SCENE.UPDATE],
-        { scrollToContent = false }: { scrollToContent?: boolean } = {},
+        { init = false }: { init?: boolean } = {},
       ) => {
         const { elements: remoteElements } = decryptedData.payload;
 
-        if (scrollToContent) {
+        if (init) {
+          history.resumeRecording();
           this.setState({
             ...this.state,
             ...calculateScrollCenter(
@@ -1292,7 +1297,7 @@ class App extends React.Component<ExcalidrawProps, AppState> {
               return;
             case SCENE.INIT: {
               if (!this.portal.socketInitialized) {
-                updateScene(decryptedData, { scrollToContent: true });
+                updateScene(decryptedData, { init: true });
               }
               break;
             }
@@ -3626,7 +3631,7 @@ class App extends React.Component<ExcalidrawProps, AppState> {
               ...(appState || this.state),
               isLoading: false,
             },
-            commitToHistory: false,
+            commitToHistory: true,
           }),
         )
         .catch((error) => {

src/element/newElement.test.ts

@@ -1,9 +1,6 @@
-import {
-  newTextElement,
-  duplicateElement,
-  newLinearElement,
-} from "./newElement";
+import { duplicateElement } from "./newElement";
 import { mutateElement } from "./mutateElement";
+import { API } from "../tests/helpers/api";
 
 const isPrimitive = (val: any) => {
   const type = typeof val;
@@ -22,7 +19,7 @@ const assertCloneObjects = (source: any, clone: any) => {
 };
 
 it("clones arrow element", () => {
-  const element = newLinearElement({
+  const element = API.createElement({
     type: "arrow",
     x: 0,
     y: 0,
@@ -68,7 +65,8 @@ it("clones arrow element", () => {
 });
 
 it("clones text element", () => {
-  const element = newTextElement({
+  const element = API.createElement({
+    type: "text",
     x: 0,
     y: 0,
     strokeColor: "#000000",

src/tests/helpers/api.ts

@@ -1,4 +1,12 @@
-import { ExcalidrawElement } from "../../element/types";
+import {
+  ExcalidrawElement,
+  ExcalidrawGenericElement,
+  ExcalidrawTextElement,
+  ExcalidrawLinearElement,
+} from "../../element/types";
+import { newElement, newTextElement, newLinearElement } from "../../element";
+import { DEFAULT_VERTICAL_ALIGN } from "../../constants";
+import { getDefaultAppState } from "../../appState";
 
 const { h } = window;
 
@@ -29,4 +37,102 @@ export class API {
     h.app.clearSelection(null);
     expect(API.getSelectedElements().length).toBe(0);
   };
+
+  static createElement = <
+    T extends Exclude<ExcalidrawElement["type"], "selection">
+  >({
+    type,
+    id,
+    x = 0,
+    y = x,
+    width = 100,
+    height = width,
+    isDeleted = false,
+    ...rest
+  }: {
+    type: T;
+    x?: number;
+    y?: number;
+    height?: number;
+    width?: number;
+    id?: string;
+    isDeleted?: boolean;
+    // generic element props
+    strokeColor?: ExcalidrawGenericElement["strokeColor"];
+    backgroundColor?: ExcalidrawGenericElement["backgroundColor"];
+    fillStyle?: ExcalidrawGenericElement["fillStyle"];
+    strokeWidth?: ExcalidrawGenericElement["strokeWidth"];
+    strokeStyle?: ExcalidrawGenericElement["strokeStyle"];
+    strokeSharpness?: ExcalidrawGenericElement["strokeSharpness"];
+    roughness?: ExcalidrawGenericElement["roughness"];
+    opacity?: ExcalidrawGenericElement["opacity"];
+    // text props
+    text?: T extends "text" ? ExcalidrawTextElement["text"] : never;
+    fontSize?: T extends "text" ? ExcalidrawTextElement["fontSize"] : never;
+    fontFamily?: T extends "text" ? ExcalidrawTextElement["fontFamily"] : never;
+    textAlign?: T extends "text" ? ExcalidrawTextElement["textAlign"] : never;
+    verticalAlign?: T extends "text"
+      ? ExcalidrawTextElement["verticalAlign"]
+      : never;
+  }): T extends "arrow" | "line" | "draw"
+    ? ExcalidrawLinearElement
+    : T extends "text"
+    ? ExcalidrawTextElement
+    : ExcalidrawGenericElement => {
+    let element: Mutable<ExcalidrawElement> = null!;
+
+    const appState = h?.state || getDefaultAppState();
+
+    const base = {
+      x,
+      y,
+      strokeColor: rest.strokeColor ?? appState.currentItemStrokeColor,
+      backgroundColor:
+        rest.backgroundColor ?? appState.currentItemBackgroundColor,
+      fillStyle: rest.fillStyle ?? appState.currentItemFillStyle,
+      strokeWidth: rest.strokeWidth ?? appState.currentItemStrokeWidth,
+      strokeStyle: rest.strokeStyle ?? appState.currentItemStrokeStyle,
+      strokeSharpness:
+        rest.strokeSharpness ?? appState.currentItemStrokeSharpness,
+      roughness: rest.roughness ?? appState.currentItemRoughness,
+      opacity: rest.opacity ?? appState.currentItemOpacity,
+    };
+    switch (type) {
+      case "rectangle":
+      case "diamond":
+      case "ellipse":
+        element = newElement({
+          type: type as "rectangle" | "diamond" | "ellipse",
+          width,
+          height,
+          ...base,
+        });
+        break;
+      case "text":
+        element = newTextElement({
+          ...base,
+          text: rest.text || "test",
+          fontSize: rest.fontSize ?? appState.currentItemFontSize,
+          fontFamily: rest.fontFamily ?? appState.currentItemFontFamily,
+          textAlign: rest.textAlign ?? appState.currentItemTextAlign,
+          verticalAlign: rest.verticalAlign ?? DEFAULT_VERTICAL_ALIGN,
+        });
+        break;
+      case "arrow":
+      case "line":
+      case "draw":
+        element = newLinearElement({
+          type: type as "arrow" | "line" | "draw",
+          ...base,
+        });
+        break;
+    }
+    if (id) {
+      element.id = id;
+    }
+    if (isDeleted) {
+      element.isDeleted = isDeleted;
+    }
+    return element as any;
+  };
 }

src/tests/helpers/ui.ts

@@ -192,7 +192,7 @@ export class UI {
       size?: number;
       width?: number;
       height?: number;
-    },
+    } = {},
   ): T extends "arrow" | "line" | "draw"
     ? ExcalidrawLinearElement
     : T extends "text"

src/tests/history.test.tsx

@@ -0,0 +1,127 @@
+import React from "react";
+import { render, GlobalTestState } from "./test-utils";
+import App from "../components/App";
+import { UI } from "./helpers/ui";
+import { API } from "./helpers/api";
+import { getDefaultAppState } from "../appState";
+import { waitFor, fireEvent, createEvent } from "@testing-library/react";
+import { createUndoAction, createRedoAction } from "../actions/actionHistory";
+
+const { h } = window;
+
+describe("history", () => {
+  it("initializing scene should end up with single history entry", async () => {
+    render(
+      <App
+        initialData={{
+          appState: {
+            ...getDefaultAppState(),
+            zenModeEnabled: true,
+          },
+          elements: [API.createElement({ type: "rectangle", id: "A" })],
+        }}
+      />,
+    );
+
+    await waitFor(() => expect(h.state.zenModeEnabled).toBe(true));
+    await waitFor(() =>
+      expect(h.elements).toEqual([expect.objectContaining({ id: "A" })]),
+    );
+    const undoAction = createUndoAction(h.history);
+    const redoAction = createRedoAction(h.history);
+    h.app.actionManager.executeAction(undoAction);
+    expect(h.elements).toEqual([
+      expect.objectContaining({ id: "A", isDeleted: false }),
+    ]);
+    const rectangle = UI.createElement("rectangle");
+    expect(h.elements).toEqual([
+      expect.objectContaining({ id: "A" }),
+      expect.objectContaining({ id: rectangle.id }),
+    ]);
+    h.app.actionManager.executeAction(undoAction);
+    expect(h.elements).toEqual([
+      expect.objectContaining({ id: "A", isDeleted: false }),
+      expect.objectContaining({ id: rectangle.id, isDeleted: true }),
+    ]);
+
+    // noop
+    h.app.actionManager.executeAction(undoAction);
+    expect(h.elements).toEqual([
+      expect.objectContaining({ id: "A", isDeleted: false }),
+      expect.objectContaining({ id: rectangle.id, isDeleted: true }),
+    ]);
+    expect(API.getStateHistory().length).toBe(1);
+
+    h.app.actionManager.executeAction(redoAction);
+    expect(h.elements).toEqual([
+      expect.objectContaining({ id: "A", isDeleted: false }),
+      expect.objectContaining({ id: rectangle.id, isDeleted: false }),
+    ]);
+    expect(API.getStateHistory().length).toBe(2);
+  });
+
+  it("scene import via drag&drop should create new history entry", async () => {
+    render(
+      <App
+        initialData={{
+          appState: {
+            ...getDefaultAppState(),
+            viewBackgroundColor: "#FFF",
+          },
+          elements: [API.createElement({ type: "rectangle", id: "A" })],
+        }}
+      />,
+    );
+
+    await waitFor(() => expect(h.state.viewBackgroundColor).toBe("#FFF"));
+    await waitFor(() =>
+      expect(h.elements).toEqual([expect.objectContaining({ id: "A" })]),
+    );
+    const fileDropEvent = createEvent.drop(GlobalTestState.canvas);
+    const file = new Blob(
+      [
+        JSON.stringify({
+          type: "excalidraw",
+          appState: {
+            ...getDefaultAppState(),
+            viewBackgroundColor: "#000",
+          },
+          elements: [API.createElement({ type: "rectangle", id: "B" })],
+        }),
+      ],
+      {
+        type: "application/json",
+      },
+    );
+    Object.defineProperty(fileDropEvent, "dataTransfer", {
+      value: {
+        files: [file],
+        getData: (_type: string) => {
+          return "";
+        },
+      },
+    });
+    fireEvent(GlobalTestState.canvas, fileDropEvent);
+
+    await waitFor(() => expect(API.getStateHistory().length).toBe(2));
+    expect(h.state.viewBackgroundColor).toBe("#000");
+    expect(h.elements).toEqual([
+      expect.objectContaining({ id: "B", isDeleted: false }),
+    ]);
+
+    const undoAction = createUndoAction(h.history);
+    const redoAction = createRedoAction(h.history);
+    h.app.actionManager.executeAction(undoAction);
+    expect(h.elements).toEqual([
+      expect.objectContaining({ id: "A", isDeleted: false }),
+      expect.objectContaining({ id: "B", isDeleted: true }),
+    ]);
+    expect(h.state.viewBackgroundColor).toBe("#FFF");
+    h.app.actionManager.executeAction(redoAction);
+    expect(h.state.viewBackgroundColor).toBe("#000");
+    expect(h.elements).toEqual([
+      expect.objectContaining({ id: "B", isDeleted: false }),
+      expect.objectContaining({ id: "A", isDeleted: true }),
+    ]);
+  });
+});

src/tests/regressionTests.test.tsx

@@ -451,10 +451,7 @@ describe("regression tests", () => {
   });
 
   it("noop interaction after undo shouldn't create history entry", () => {
-    // NOTE: this will fail if this test case is run in isolation. There's
-    //  some leaking state or race conditions in initialization/teardown
-    //  (couldn't figure out)
-    expect(API.getStateHistory().length).toBe(0);
+    expect(API.getStateHistory().length).toBe(1);
 
     UI.clickTool("rectangle");
     mouse.down(10, 10);
@@ -468,35 +465,35 @@ describe("regression tests", () => {
 
     const secondElementEndPoint = mouse.getPosition();
 
-    expect(API.getStateHistory().length).toBe(2);
+    expect(API.getStateHistory().length).toBe(3);
 
     Keyboard.withModifierKeys({ ctrl: true }, () => {
       Keyboard.keyPress("z");
     });
 
-    expect(API.getStateHistory().length).toBe(1);
+    expect(API.getStateHistory().length).toBe(2);
 
     // clicking an element shouldn't add to history
     mouse.restorePosition(...firstElementEndPoint);
     mouse.click();
-    expect(API.getStateHistory().length).toBe(1);
+    expect(API.getStateHistory().length).toBe(2);
 
     Keyboard.withModifierKeys({ shift: true, ctrl: true }, () => {
       Keyboard.keyPress("z");
     });
 
-    expect(API.getStateHistory().length).toBe(2);
+    expect(API.getStateHistory().length).toBe(3);
 
     // clicking an element shouldn't add to history
     mouse.click();
-    expect(API.getStateHistory().length).toBe(2);
+    expect(API.getStateHistory().length).toBe(3);
 
     const firstSelectedElementId = API.getSelectedElement().id;
 
     // same for clicking the element just redo-ed
     mouse.restorePosition(...secondElementEndPoint);
     mouse.click();
-    expect(API.getStateHistory().length).toBe(2);
+    expect(API.getStateHistory().length).toBe(3);
 
     expect(API.getSelectedElement().id).not.toEqual(firstSelectedElementId);
   });

src/tests/zindex.test.tsx

@@ -3,14 +3,13 @@ import ReactDOM from "react-dom";
 import { render } from "./test-utils";
 import App from "../components/App";
 import { reseed } from "../random";
-import { newElement } from "../element";
 import {
   actionSendBackward,
   actionBringForward,
   actionBringToFront,
   actionSendToBack,
 } from "../actions";
-import { ExcalidrawElement } from "../element/types";
+import { API } from "./helpers/api";
 
 // Unmount ReactDOM from root
 ReactDOM.unmountComponentAtNode(document.getElementById("root")!);
@@ -28,21 +27,7 @@ const populateElements = (
   const selectedElementIds: any = {};
 
   h.elements = elements.map(({ id, isDeleted = false, isSelected = false }) => {
-    const element: Mutable<ExcalidrawElement> = newElement({
-      type: "rectangle",
-      x: 100,
-      y: 100,
-      strokeColor: h.state.currentItemStrokeColor,
-      backgroundColor: h.state.currentItemBackgroundColor,
-      fillStyle: h.state.currentItemFillStyle,
-      strokeWidth: h.state.currentItemStrokeWidth,
-      strokeStyle: h.state.currentItemStrokeStyle,
-      strokeSharpness: h.state.currentItemStrokeSharpness,
-      roughness: h.state.currentItemRoughness,
-      opacity: h.state.currentItemOpacity,
-    });
-    element.id = id;
-    element.isDeleted = isDeleted;
+    const element = API.createElement({ type: "rectangle", id, isDeleted });
     if (isSelected) {
       selectedElementIds[element.id] = true;
     }