Review fixes

packages/element/src/Scene.ts

@@ -6,14 +6,13 @@ import {
   toBrandedType,
   isDevEnv,
   isTestEnv,
-  isReadonlyArray,
+  toArray,
 } from "@excalidraw/common";
 import { isNonDeletedElement } from "@excalidraw/element";
 import { isFrameLikeElement } from "@excalidraw/element/typeChecks";
 import { getElementsInGroup } from "@excalidraw/element/groups";
 
 import {
-  orderByFractionalIndex,
   syncInvalidIndices,
   syncMovedIndices,
   validateFractionalIndices,
@@ -268,19 +267,13 @@ class Scene {
   }
 
   replaceAllElements(nextElements: ElementsMapOrArray) {
-    // ts doesn't like `Array.isArray` of `instanceof Map`
-    if (!isReadonlyArray(nextElements)) {
-      // need to order by fractional indices to get the correct order
-      nextElements = orderByFractionalIndex(
-        Array.from(nextElements.values()) as OrderedExcalidrawElement[],
-      );
-    }
-
+    // we do trust the insertion order on the map, though maybe we shouldn't and should prefer order defined by fractional indices
+    const _nextElements = toArray(nextElements);
     const nextFrameLikes: ExcalidrawFrameLikeElement[] = [];
 
-    validateIndicesThrottled(nextElements);
+    validateIndicesThrottled(_nextElements);
 
-    this.elements = syncInvalidIndices(nextElements);
+    this.elements = syncInvalidIndices(_nextElements);
     this.elementsMap.clear();
     this.elements.forEach((element) => {
       if (isFrameLikeElement(element)) {

packages/element/src/delta.ts

@@ -316,7 +316,7 @@ export class Delta<T> {
   }
 
   /**
-   * Returns all the object1 keys that have distinct values.
+   * Returns sorted object1 keys that have distinct values.
    */
   public static getLeftDifferences<T extends {}>(
     object1: T,
@@ -325,11 +325,11 @@ export class Delta<T> {
   ) {
     return Array.from(
       this.distinctKeysIterator("left", object1, object2, skipShallowCompare),
-    );
+    ).sort();
   }
 
   /**
-   * Returns all the object2 keys that have distinct values.
+   * Returns sorted object2 keys that have distinct values.
    */
   public static getRightDifferences<T extends {}>(
     object1: T,
@@ -338,7 +338,7 @@ export class Delta<T> {
   ) {
     return Array.from(
       this.distinctKeysIterator("right", object1, object2, skipShallowCompare),
-    );
+    ).sort();
   }
 
   /**
@@ -430,7 +430,8 @@ export class AppStateDelta implements DeltaContainer<AppState> {
     const delta = Delta.calculate(
       prevAppState,
       nextAppState,
-      undefined,
+      // making the order of keys in deltas stable for hashing purposes
+      AppStateDelta.orderAppStateKeys,
       AppStateDelta.postProcess,
     );
 
@@ -539,40 +540,6 @@ export class AppStateDelta implements DeltaContainer<AppState> {
     return Delta.isEmpty(this.delta);
   }
 
-  /**
-   * It is necessary to post process the partials in case of reference values,
-   * for which we need to calculate the real diff between `deleted` and `inserted`.
-   */
-  private static postProcess<T extends ObservedAppState>(
-    deleted: Partial<T>,
-    inserted: Partial<T>,
-  ): [Partial<T>, Partial<T>] {
-    try {
-      Delta.diffObjects(
-        deleted,
-        inserted,
-        "selectedElementIds",
-        // ts language server has a bit trouble resolving this, so we are giving it a little push
-        (_) => true as ValueOf<T["selectedElementIds"]>,
-      );
-      Delta.diffObjects(
-        deleted,
-        inserted,
-        "selectedGroupIds",
-        (prevValue) => (prevValue ?? false) as ValueOf<T["selectedGroupIds"]>,
-      );
-    } catch (e) {
-      // if postprocessing fails it does not make sense to bubble up, but let's make sure we know about it
-      console.error(`Couldn't postprocess appstate change deltas.`);
-
-      if (isTestEnv() || isDevEnv()) {
-        throw e;
-      }
-    } finally {
-      return [deleted, inserted];
-    }
-  }
-
   /**
    * Mutates `nextAppState` be filtering out state related to deleted elements.
    *
@@ -807,6 +774,51 @@ export class AppStateDelta implements DeltaContainer<AppState> {
       ObservedElementsAppState
     >;
   }
+
+  /**
+   * It is necessary to post process the partials in case of reference values,
+   * for which we need to calculate the real diff between `deleted` and `inserted`.
+   */
+  private static postProcess<T extends ObservedAppState>(
+    deleted: Partial<T>,
+    inserted: Partial<T>,
+  ): [Partial<T>, Partial<T>] {
+    try {
+      Delta.diffObjects(
+        deleted,
+        inserted,
+        "selectedElementIds",
+        // ts language server has a bit trouble resolving this, so we are giving it a little push
+        (_) => true as ValueOf<T["selectedElementIds"]>,
+      );
+      Delta.diffObjects(
+        deleted,
+        inserted,
+        "selectedGroupIds",
+        (prevValue) => (prevValue ?? false) as ValueOf<T["selectedGroupIds"]>,
+      );
+    } catch (e) {
+      // if postprocessing fails it does not make sense to bubble up, but let's make sure we know about it
+      console.error(`Couldn't postprocess appstate change deltas.`);
+
+      if (isTestEnv() || isDevEnv()) {
+        throw e;
+      }
+    } finally {
+      return [deleted, inserted];
+    }
+  }
+
+  private static orderAppStateKeys(partial: Partial<ObservedAppState>) {
+    const orderedPartial: { [key: string]: unknown } = {};
+
+    for (const key of Object.keys(partial).sort()) {
+      // relying on insertion order
+      orderedPartial[key] = partial[key as keyof ObservedAppState];
+    }
+
+    return orderedPartial as Partial<ObservedAppState>;
+  }
 }
 
 type ElementPartial<T extends ExcalidrawElement = ExcalidrawElement> = Omit<

packages/element/src/index.ts

@@ -1,3 +1,5 @@
+import { toIterable } from "@excalidraw/common";
+
 import { isInvisiblySmallElement } from "./sizeHelpers";
 import { isLinearElementType } from "./typeChecks";
 
@@ -5,6 +7,7 @@ import type {
   ExcalidrawElement,
   NonDeletedExcalidrawElement,
   NonDeleted,
+  ElementsMapOrArray,
 } from "./types";
 
 /**
@@ -16,12 +19,10 @@ export const getSceneVersion = (elements: readonly ExcalidrawElement[]) =>
 /**
  * Hashes elements' versionNonce (using djb2 algo). Order of elements matters.
  */
-export const hashElementsVersion = (
-  elements: readonly ExcalidrawElement[],
-): number => {
+export const hashElementsVersion = (elements: ElementsMapOrArray): number => {
   let hash = 5381;
-  for (let i = 0; i < elements.length; i++) {
-    hash = (hash << 5) + hash + elements[i].versionNonce;
+  for (const element of toIterable(elements)) {
+    hash = (hash << 5) + hash + element.versionNonce;
   }
   return hash >>> 0; // Ensure unsigned 32-bit integer
 };

packages/element/src/store.ts

@@ -6,6 +6,7 @@ import {
   isTestEnv,
   randomId,
   Emitter,
+  toIterable,
 } from "@excalidraw/common";
 
 import type { DTO, ValueOf } from "@excalidraw/common/utility-types";
@@ -158,25 +159,25 @@ export class Store {
   ): SceneElementsMap {
     const movedElements = new Map<string, ExcalidrawElement>();
 
-    for (const [id, prevElement] of prevElements.entries()) {
-      const nextElement = nextElements.get(id);
+    for (const prevElement of toIterable(prevElements)) {
+      const nextElement = nextElements.get(prevElement.id);
 
       if (!nextElement) {
         // Nothing to care about here, element was forcefully deleted
         continue;
       }
 
-      const elementSnapshot = this.snapshot.elements.get(id);
+      const elementSnapshot = this.snapshot.elements.get(prevElement.id);
 
       // Checks for in progress async user action
       if (!elementSnapshot) {
         // Detected yet uncomitted local element
-        nextElements.delete(id);
+        nextElements.delete(prevElement.id);
       } else if (elementSnapshot.version < prevElement.version) {
         // Element was already commited, but the snapshot version is lower than current local version
-        nextElements.set(id, elementSnapshot);
+        nextElements.set(prevElement.id, elementSnapshot);
         // Mark the element as potentially moved, as it could have
-        movedElements.set(id, elementSnapshot);
+        movedElements.set(prevElement.id, elementSnapshot);
       }
     }
 
@@ -601,10 +602,10 @@ export class StoreSnapshot {
   public getChangedElements(prevSnapshot: StoreSnapshot) {
     const changedElements: Record<string, OrderedExcalidrawElement> = {};
 
-    for (const [id, nextElement] of this.elements.entries()) {
+    for (const nextElement of toIterable(this.elements)) {
       // Due to the structural clone inside `maybeClone`, we can perform just these reference checks
-      if (prevSnapshot.elements.get(id) !== nextElement) {
-        changedElements[id] = nextElement;
+      if (prevSnapshot.elements.get(nextElement.id) !== nextElement) {
+        changedElements[nextElement.id] = nextElement;
       }
     }
 
@@ -794,26 +795,26 @@ export class StoreSnapshot {
 
     const changedElements: SceneElementsMap = new Map() as SceneElementsMap;
 
-    for (const [id, prevElement] of this.elements) {
-      const nextElement = nextElements.get(id);
+    for (const prevElement of toIterable(this.elements)) {
+      const nextElement = nextElements.get(prevElement.id);
 
       if (!nextElement) {
         // element was deleted
         changedElements.set(
-          id,
+          prevElement.id,
           newElementWith(prevElement, { isDeleted: true }),
         );
       }
     }
 
-    for (const [id, nextElement] of nextElements) {
-      const prevElement = this.elements.get(id);
+    for (const nextElement of toIterable(nextElements)) {
+      const prevElement = this.elements.get(nextElement.id);
 
       if (
         !prevElement || // element was added
         prevElement.version < nextElement.version // element was updated
       ) {
-        changedElements.set(id, nextElement);
+        changedElements.set(nextElement.id, nextElement);
       }
     }
 
@@ -821,9 +822,7 @@ export class StoreSnapshot {
       return;
     }
 
-    const changedElementsHash = hashElementsVersion(
-      Array.from(changedElements.values()),
-    );
+    const changedElementsHash = hashElementsVersion(changedElements);
 
     if (
       options.shouldCompareHashes &&
@@ -843,15 +842,15 @@ export class StoreSnapshot {
   private createElementsSnapshot(changedElements: SceneElementsMap) {
     const clonedElements = new Map() as SceneElementsMap;
 
-    for (const [id, prevElement] of this.elements) {
+    for (const prevElement of toIterable(this.elements)) {
       // Clone previous elements, never delete, in case nextElements would be just a subset of previous elements
       // i.e. during collab, persist or whenenever isDeleted elements get cleared
-      clonedElements.set(id, prevElement);
+      clonedElements.set(prevElement.id, prevElement);
     }
 
-    for (const [id, changedElement] of changedElements) {
+    for (const changedElement of toIterable(changedElements)) {
       // TODO: consider just creating new instance, once we can ensure that all reference properties on every element are immutable
-      clonedElements.set(id, deepCopyElement(changedElement));
+      clonedElements.set(changedElement.id, deepCopyElement(changedElement));
     }
 
     return clonedElements;