fix: empty snapLines arrays would cause re-render (#7454)

Co-authored-by: Lynda Lin <lynda.lin@optoma.com>
Co-authored-by: dwelle <5153846+dwelle@users.noreply.github.com>

packages/excalidraw/components/App.tsx

@@ -268,6 +268,7 @@ import {
   muteFSAbortError,
   isTestEnv,
   easeOut,
+  updateStable,
 } from "../utils";
 import {
   createSrcDoc,
@@ -4736,13 +4737,31 @@ class App extends React.Component<AppProps, AppState> {
         event,
       );
 
-      this.setState({
-        snapLines,
-        originSnapOffset: originOffset,
+      this.setState((prevState) => {
+        const nextSnapLines = updateStable(prevState.snapLines, snapLines);
+        const nextOriginOffset = prevState.originSnapOffset
+          ? updateStable(prevState.originSnapOffset, originOffset)
+          : originOffset;
+
+        if (
+          prevState.snapLines === nextSnapLines &&
+          prevState.originSnapOffset === nextOriginOffset
+        ) {
+          return null;
+        }
+        return {
+          snapLines: nextSnapLines,
+          originSnapOffset: nextOriginOffset,
+        };
       });
     } else if (!this.state.draggingElement) {
-      this.setState({
-        snapLines: [],
+      this.setState((prevState) => {
+        if (prevState.snapLines.length) {
+          return {
+            snapLines: [],
+          };
+        }
+        return null;
       });
     }
 
@@ -7227,7 +7246,7 @@ class App extends React.Component<AppProps, AppState> {
         isRotating,
       } = this.state;
 
-      this.setState({
+      this.setState((prevState) => ({
         isResizing: false,
         isRotating: false,
         resizingElement: null,
@@ -7241,10 +7260,10 @@ class App extends React.Component<AppProps, AppState> {
           multiElement || isTextElement(this.state.editingElement)
             ? this.state.editingElement
             : null,
-        snapLines: [],
+        snapLines: updateStable(prevState.snapLines, []),
 
         originSnapOffset: null,
-      });
+      }));
 
       SnapCache.setReferenceSnapPoints(null);
       SnapCache.setVisibleGaps(null);

packages/excalidraw/tests/__snapshots__/move.test.tsx.snap

@@ -1,6 +1,6 @@
 // Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html
 
-exports[`duplicate element on move when ALT is clicked > rectangle 1`] = `
+exports[`duplicate element on move when ALT is clicked > rectangle 5`] = `
 {
   "angle": 0,
   "backgroundColor": "transparent",
@@ -32,7 +32,7 @@ exports[`duplicate element on move when ALT is clicked > rectangle 1`] = `
 }
 `;
 
-exports[`duplicate element on move when ALT is clicked > rectangle 2`] = `
+exports[`duplicate element on move when ALT is clicked > rectangle 6`] = `
 {
   "angle": 0,
   "backgroundColor": "transparent",
@@ -64,7 +64,7 @@ exports[`duplicate element on move when ALT is clicked > rectangle 2`] = `
 }
 `;
 
-exports[`move element > rectangle 1`] = `
+exports[`move element > rectangle 5`] = `
 {
   "angle": 0,
   "backgroundColor": "transparent",
@@ -96,7 +96,7 @@ exports[`move element > rectangle 1`] = `
 }
 `;
 
-exports[`move element > rectangles with binding arrow 1`] = `
+exports[`move element > rectangles with binding arrow 5`] = `
 {
   "angle": 0,
   "backgroundColor": "transparent",
@@ -133,7 +133,7 @@ exports[`move element > rectangles with binding arrow 1`] = `
 }
 `;
 
-exports[`move element > rectangles with binding arrow 2`] = `
+exports[`move element > rectangles with binding arrow 6`] = `
 {
   "angle": 0,
   "backgroundColor": "transparent",
@@ -170,7 +170,7 @@ exports[`move element > rectangles with binding arrow 2`] = `
 }
 `;
 
-exports[`move element > rectangles with binding arrow 3`] = `
+exports[`move element > rectangles with binding arrow 7`] = `
 {
   "angle": 0,
   "backgroundColor": "transparent",

packages/excalidraw/tests/__snapshots__/multiPointCreate.test.tsx.snap

@@ -1,6 +1,6 @@
 // Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html
 
-exports[`multi point mode in linear elements > arrow 1`] = `
+exports[`multi point mode in linear elements > arrow 3`] = `
 {
   "angle": 0,
   "backgroundColor": "transparent",
@@ -54,7 +54,7 @@ exports[`multi point mode in linear elements > arrow 1`] = `
 }
 `;
 
-exports[`multi point mode in linear elements > line 1`] = `
+exports[`multi point mode in linear elements > line 3`] = `
 {
   "angle": 0,
   "backgroundColor": "transparent",

packages/excalidraw/tests/linearElementEditor.test.tsx

@@ -173,14 +173,14 @@ describe("Test Linear Elements", () => {
     createTwoPointerLinearElement("line");
     const line = h.elements[0] as ExcalidrawLinearElement;
 
-    expect(renderInteractiveScene).toHaveBeenCalledTimes(5);
-    expect(renderStaticScene).toHaveBeenCalledTimes(5);
+    expect(renderInteractiveScene.mock.calls.length).toMatchInlineSnapshot(`5`);
+    expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`5`);
     expect((h.elements[0] as ExcalidrawLinearElement).points.length).toEqual(2);
 
     // drag line from midpoint
     drag(midpoint, [midpoint[0] + delta, midpoint[1] + delta]);
-    expect(renderInteractiveScene).toHaveBeenCalledTimes(9);
-    expect(renderStaticScene).toHaveBeenCalledTimes(7);
+    expect(renderInteractiveScene.mock.calls.length).toMatchInlineSnapshot(`9`);
+    expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`7`);
     expect(line.points.length).toEqual(3);
     expect(line.points).toMatchInlineSnapshot(`
       [
@@ -273,8 +273,10 @@ describe("Test Linear Elements", () => {
 
       // drag line from midpoint
       drag(midpoint, [midpoint[0] + delta, midpoint[1] + delta]);
-      expect(renderInteractiveScene).toHaveBeenCalledTimes(14);
-      expect(renderStaticScene).toHaveBeenCalledTimes(6);
+      expect(renderInteractiveScene.mock.calls.length).toMatchInlineSnapshot(
+        `12`,
+      );
+      expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`6`);
 
       expect(line.points.length).toEqual(3);
       expect(line.points).toMatchInlineSnapshot(`
@@ -311,8 +313,10 @@ describe("Test Linear Elements", () => {
       // update roundness
       fireEvent.click(screen.getByTitle("Round"));
 
-      expect(renderInteractiveScene).toHaveBeenCalledTimes(10);
-      expect(renderStaticScene).toHaveBeenCalledTimes(8);
+      expect(renderInteractiveScene.mock.calls.length).toMatchInlineSnapshot(
+        `10`,
+      );
+      expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`8`);
 
       const midPointsWithRoundEdge = LinearElementEditor.getEditorMidPoints(
         h.elements[0] as ExcalidrawLinearElement,
@@ -357,8 +361,10 @@ describe("Test Linear Elements", () => {
       // Move the element
       drag(startPoint, endPoint);
 
-      expect(renderInteractiveScene).toHaveBeenCalledTimes(14);
-      expect(renderStaticScene).toHaveBeenCalledTimes(9);
+      expect(renderInteractiveScene.mock.calls.length).toMatchInlineSnapshot(
+        `13`,
+      );
+      expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`9`);
 
       expect([line.x, line.y]).toEqual([
         points[0][0] + deltaX,
@@ -416,8 +422,10 @@ describe("Test Linear Elements", () => {
           lastSegmentMidpoint[1] + delta,
         ]);
 
-        expect(renderInteractiveScene).toHaveBeenCalledTimes(21);
-        expect(renderStaticScene).toHaveBeenCalledTimes(9);
+        expect(renderInteractiveScene.mock.calls.length).toMatchInlineSnapshot(
+          `17`,
+        );
+        expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`9`);
 
         expect(line.points.length).toEqual(5);
 
@@ -457,8 +465,10 @@ describe("Test Linear Elements", () => {
         // Drag from first point
         drag(hitCoords, [hitCoords[0] - delta, hitCoords[1] - delta]);
 
-        expect(renderInteractiveScene).toHaveBeenCalledTimes(14);
-        expect(renderStaticScene).toHaveBeenCalledTimes(8);
+        expect(renderInteractiveScene.mock.calls.length).toMatchInlineSnapshot(
+          `13`,
+        );
+        expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`8`);
 
         const newPoints = LinearElementEditor.getPointsGlobalCoordinates(line);
         expect([newPoints[0][0], newPoints[0][1]]).toEqual([
@@ -484,8 +494,10 @@ describe("Test Linear Elements", () => {
         // Drag from first point
         drag(hitCoords, [hitCoords[0] + delta, hitCoords[1] + delta]);
 
-        expect(renderInteractiveScene).toHaveBeenCalledTimes(14);
-        expect(renderStaticScene).toHaveBeenCalledTimes(8);
+        expect(renderInteractiveScene.mock.calls.length).toMatchInlineSnapshot(
+          `13`,
+        );
+        expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`8`);
 
         const newPoints = LinearElementEditor.getPointsGlobalCoordinates(line);
         expect([newPoints[0][0], newPoints[0][1]]).toEqual([
@@ -519,8 +531,10 @@ describe("Test Linear Elements", () => {
         // delete 3rd point
         deletePoint(points[2]);
         expect(line.points.length).toEqual(3);
-        expect(renderInteractiveScene).toHaveBeenCalledTimes(21);
-        expect(renderStaticScene).toHaveBeenCalledTimes(9);
+        expect(renderInteractiveScene.mock.calls.length).toMatchInlineSnapshot(
+          `19`,
+        );
+        expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`9`);
 
         const newMidPoints = LinearElementEditor.getEditorMidPoints(
           line,
@@ -566,8 +580,10 @@ describe("Test Linear Elements", () => {
           lastSegmentMidpoint[0] + delta,
           lastSegmentMidpoint[1] + delta,
         ]);
-        expect(renderInteractiveScene).toHaveBeenCalledTimes(21);
-        expect(renderStaticScene).toHaveBeenCalledTimes(9);
+        expect(renderInteractiveScene.mock.calls.length).toMatchInlineSnapshot(
+          `17`,
+        );
+        expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`9`);
         expect(line.points.length).toEqual(5);
 
         expect((h.elements[0] as ExcalidrawLinearElement).points)
@@ -642,8 +658,10 @@ describe("Test Linear Elements", () => {
         // Drag from first point
         drag(hitCoords, [hitCoords[0] + delta, hitCoords[1] + delta]);
 
-        expect(renderInteractiveScene).toHaveBeenCalledTimes(14);
-        expect(renderStaticScene).toHaveBeenCalledTimes(8);
+        expect(renderInteractiveScene.mock.calls.length).toMatchInlineSnapshot(
+          `13`,
+        );
+        expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`8`);
 
         const newPoints = LinearElementEditor.getPointsGlobalCoordinates(line);
         expect([newPoints[0][0], newPoints[0][1]]).toEqual([

packages/excalidraw/tests/move.test.tsx

@@ -42,8 +42,10 @@ describe("move element", () => {
       fireEvent.pointerMove(canvas, { clientX: 60, clientY: 70 });
       fireEvent.pointerUp(canvas);
 
-      expect(renderInteractiveScene).toHaveBeenCalledTimes(6);
-      expect(renderStaticScene).toHaveBeenCalledTimes(6);
+      expect(renderInteractiveScene.mock.calls.length).toMatchInlineSnapshot(
+        `6`,
+      );
+      expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`6`);
       expect(h.state.selectionElement).toBeNull();
       expect(h.elements.length).toEqual(1);
       expect(h.state.selectedElementIds[h.elements[0].id]).toBeTruthy();
@@ -57,8 +59,8 @@ describe("move element", () => {
     fireEvent.pointerMove(canvas, { clientX: 20, clientY: 40 });
     fireEvent.pointerUp(canvas);
 
-    expect(renderInteractiveScene).toHaveBeenCalledTimes(3);
-    expect(renderStaticScene).toHaveBeenCalledTimes(2);
+    expect(renderInteractiveScene.mock.calls.length).toMatchInlineSnapshot(`3`);
+    expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`2`);
     expect(h.state.selectionElement).toBeNull();
     expect(h.elements.length).toEqual(1);
     expect([h.elements[0].x, h.elements[0].y]).toEqual([0, 40]);
@@ -84,8 +86,10 @@ describe("move element", () => {
     // select the second rectangles
     new Pointer("mouse").clickOn(rectB);
 
-    expect(renderInteractiveScene).toHaveBeenCalledTimes(24);
-    expect(renderStaticScene).toHaveBeenCalledTimes(19);
+    expect(renderInteractiveScene.mock.calls.length).toMatchInlineSnapshot(
+      `21`,
+    );
+    expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`19`);
     expect(h.state.selectionElement).toBeNull();
     expect(h.elements.length).toEqual(3);
     expect(h.state.selectedElementIds[rectB.id]).toBeTruthy();
@@ -103,8 +107,8 @@ describe("move element", () => {
     Keyboard.keyDown(KEYS.ARROW_DOWN);
 
     // Check that the arrow size has been changed according to moving the rectangle
-    expect(renderInteractiveScene).toHaveBeenCalledTimes(3);
-    expect(renderStaticScene).toHaveBeenCalledTimes(3);
+    expect(renderInteractiveScene.mock.calls.length).toMatchInlineSnapshot(`3`);
+    expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`3`);
     expect(h.state.selectionElement).toBeNull();
     expect(h.elements.length).toEqual(3);
     expect(h.state.selectedElementIds[rectB.id]).toBeTruthy();
@@ -130,8 +134,10 @@ describe("duplicate element on move when ALT is clicked", () => {
       fireEvent.pointerMove(canvas, { clientX: 60, clientY: 70 });
       fireEvent.pointerUp(canvas);
 
-      expect(renderInteractiveScene).toHaveBeenCalledTimes(6);
-      expect(renderStaticScene).toHaveBeenCalledTimes(6);
+      expect(renderInteractiveScene.mock.calls.length).toMatchInlineSnapshot(
+        `6`,
+      );
+      expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`6`);
       expect(h.state.selectionElement).toBeNull();
       expect(h.elements.length).toEqual(1);
       expect(h.state.selectedElementIds[h.elements[0].id]).toBeTruthy();
@@ -152,8 +158,8 @@ describe("duplicate element on move when ALT is clicked", () => {
 
     // TODO: This used to be 4, but binding made it go up to 5. Do we need
     // that additional render?
-    expect(renderInteractiveScene).toHaveBeenCalledTimes(5);
-    expect(renderStaticScene).toHaveBeenCalledTimes(3);
+    expect(renderInteractiveScene.mock.calls.length).toMatchInlineSnapshot(`4`);
+    expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`3`);
     expect(h.state.selectionElement).toBeNull();
     expect(h.elements.length).toEqual(2);
 

packages/excalidraw/tests/multiPointCreate.test.tsx

@@ -47,8 +47,8 @@ describe("remove shape in non linear elements", () => {
     fireEvent.pointerDown(canvas, { clientX: 30, clientY: 20 });
     fireEvent.pointerUp(canvas, { clientX: 30, clientY: 30 });
 
-    expect(renderInteractiveScene).toHaveBeenCalledTimes(5);
-    expect(renderStaticScene).toHaveBeenCalledTimes(5);
+    expect(renderInteractiveScene.mock.calls.length).toMatchInlineSnapshot(`5`);
+    expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`5`);
     expect(h.elements.length).toEqual(0);
   });
 
@@ -62,8 +62,8 @@ describe("remove shape in non linear elements", () => {
     fireEvent.pointerDown(canvas, { clientX: 30, clientY: 20 });
     fireEvent.pointerUp(canvas, { clientX: 30, clientY: 30 });
 
-    expect(renderInteractiveScene).toHaveBeenCalledTimes(5);
-    expect(renderStaticScene).toHaveBeenCalledTimes(5);
+    expect(renderInteractiveScene.mock.calls.length).toMatchInlineSnapshot(`5`);
+    expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`5`);
     expect(h.elements.length).toEqual(0);
   });
 
@@ -77,8 +77,8 @@ describe("remove shape in non linear elements", () => {
     fireEvent.pointerDown(canvas, { clientX: 30, clientY: 20 });
     fireEvent.pointerUp(canvas, { clientX: 30, clientY: 30 });
 
-    expect(renderInteractiveScene).toHaveBeenCalledTimes(5);
-    expect(renderStaticScene).toHaveBeenCalledTimes(5);
+    expect(renderInteractiveScene.mock.calls.length).toMatchInlineSnapshot(`5`);
+    expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`5`);
     expect(h.elements.length).toEqual(0);
   });
 });
@@ -110,8 +110,8 @@ describe("multi point mode in linear elements", () => {
       key: KEYS.ENTER,
     });
 
-    expect(renderInteractiveScene).toHaveBeenCalledTimes(11);
-    expect(renderStaticScene).toHaveBeenCalledTimes(9);
+    expect(renderInteractiveScene.mock.calls.length).toMatchInlineSnapshot(`9`);
+    expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`9`);
     expect(h.elements.length).toEqual(1);
 
     const element = h.elements[0] as ExcalidrawLinearElement;
@@ -153,8 +153,8 @@ describe("multi point mode in linear elements", () => {
     fireEvent.keyDown(document, {
       key: KEYS.ENTER,
     });
-    expect(renderInteractiveScene).toHaveBeenCalledTimes(11);
-    expect(renderStaticScene).toHaveBeenCalledTimes(9);
+    expect(renderInteractiveScene.mock.calls.length).toMatchInlineSnapshot(`9`);
+    expect(renderStaticScene.mock.calls.length).toMatchInlineSnapshot(`9`);
     expect(h.elements.length).toEqual(1);
 
     const element = h.elements[0] as ExcalidrawLinearElement;

packages/excalidraw/utils.ts

@@ -769,6 +769,24 @@ export const queryFocusableElements = (container: HTMLElement | null) => {
     : [];
 };
 
+/** use as a fallback after identity check (for perf reasons) */
+const _defaultIsShallowComparatorFallback = (a: any, b: any): boolean => {
+  // consider two empty arrays equal
+  if (
+    Array.isArray(a) &&
+    Array.isArray(b) &&
+    a.length === 0 &&
+    b.length === 0
+  ) {
+    return true;
+  }
+  return a === b;
+};
+
+/**
+ * Returns whether object/array is shallow equal.
+ * Considers empty object/arrays as equal (whether top-level or second-level).
+ */
 export const isShallowEqual = <
   T extends Record<string, any>,
   K extends readonly unknown[],
@@ -796,10 +814,12 @@ export const isShallowEqual = <
 
   if (comparators && Array.isArray(comparators)) {
     for (const key of comparators) {
-      const ret = objA[key] === objB[key];
+      const ret =
+        objA[key] === objB[key] ||
+        _defaultIsShallowComparatorFallback(objA[key], objB[key]);
       if (!ret) {
         if (debug) {
-          console.info(
+          console.warn(
             `%cisShallowEqual: ${key} not equal ->`,
             "color: #8B4000",
             objA[key],
@@ -818,9 +838,11 @@ export const isShallowEqual = <
     )?.[key as keyof T];
     const ret = comparator
       ? comparator(objA[key], objB[key])
-      : objA[key] === objB[key];
+      : objA[key] === objB[key] ||
+        _defaultIsShallowComparatorFallback(objA[key], objB[key]);
+
     if (!ret && debug) {
-      console.info(
+      console.warn(
         `%cisShallowEqual: ${key} not equal ->`,
         "color: #8B4000",
         objA[key],
@@ -960,3 +982,13 @@ export const cloneJSON = <T>(obj: T): T => JSON.parse(JSON.stringify(obj));
 export const isFiniteNumber = (value: any): value is number => {
   return typeof value === "number" && Number.isFinite(value);
 };
+
+export const updateStable = <T extends any[] | Record<string, any>>(
+  prevValue: T,
+  nextValue: T,
+) => {
+  if (isShallowEqual(prevValue, nextValue)) {
+    return prevValue;
+  }
+  return nextValue;
+};