Applying & emitting increments on undo / redo

excalidraw-app/App.tsx

@@ -109,7 +109,7 @@ import Trans from "../packages/excalidraw/components/Trans";
 import { ShareDialog, shareDialogStateAtom } from "./share/ShareDialog";
 import CollabError, { collabErrorIndicatorAtom } from "./collab/CollabError";
 import type { RemoteExcalidrawElement } from "../packages/excalidraw/data/reconcile";
-import type { StoreIncrementEvent } from "../packages/excalidraw/store";
+import type { StoreIncrement } from "../packages/excalidraw/store";
 import {
   CommandPalette,
   DEFAULT_CATEGORIES,
@@ -689,13 +689,14 @@ const ExcalidrawWrapper = () => {
     }
   };
 
-  const onIncrement = (increment: StoreIncrementEvent) => {
+  const onIncrement = (increment: StoreIncrement) => {
     // ephemerals are not part of this (which is alright)
     // - wysiwyg, dragging elements / points, mouse movements, etc.
     const { elementsChange } = increment;
 
     // some appState like selections should also be transfered (we could even persist it)
     if (!elementsChange.isEmpty()) {
+      console.log(elementsChange)
       syncAPI?.push("durable", [elementsChange]);
     }
   };

excalidraw-app/tests/collab.test.tsx

@@ -125,7 +125,7 @@ describe("collaboration", () => {
       expect(h.elements).toEqual([expect.objectContaining(rect1Props)]);
     });
 
-    const undoAction = createUndoAction(h.history, h.store);
+    const undoAction = createUndoAction(h.history);
     act(() => h.app.actionManager.executeAction(undoAction));
 
     // with explicit undo (as addition) we expect our item to be restored from the snapshot!
@@ -157,7 +157,7 @@ describe("collaboration", () => {
       expect(h.elements).toEqual([expect.objectContaining(rect1Props)]);
     });
 
-    const redoAction = createRedoAction(h.history, h.store);
+    const redoAction = createRedoAction(h.history);
     act(() => h.app.actionManager.executeAction(redoAction));
 
     // with explicit redo (as removal) we again restore the element from the snapshot!

packages/excalidraw/actions/actionHistory.tsx

@@ -46,9 +46,9 @@ const executeHistoryAction = (
   return { storeAction: StoreAction.NONE };
 };
 
-type ActionCreator = (history: History, store: Store) => Action;
+type ActionCreator = (history: History) => Action;
 
-export const createUndoAction: ActionCreator = (history, store) => ({
+export const createUndoAction: ActionCreator = (history) => ({
   name: "undo",
   label: "buttons.undo",
   icon: UndoIcon,
@@ -56,11 +56,7 @@ export const createUndoAction: ActionCreator = (history, store) => ({
   viewMode: false,
   perform: (elements, appState, value, app) =>
     executeHistoryAction(app, appState, () =>
-      history.undo(
+      history.undo(arrayToMap(elements) as SceneElementsMap, appState),
-        arrayToMap(elements) as SceneElementsMap, // TODO: #7348 refactor action manager to already include `SceneElementsMap`
-        appState,
-        store.snapshot,
-      ),
     ),
   keyTest: (event) =>
     event[KEYS.CTRL_OR_CMD] && matchKey(event, KEYS.Z) && !event.shiftKey,
@@ -87,19 +83,15 @@ export const createUndoAction: ActionCreator = (history, store) => ({
   },
 });
 
-export const createRedoAction: ActionCreator = (history, store) => ({
+export const createRedoAction: ActionCreator = (history) => ({
   name: "redo",
   label: "buttons.redo",
   icon: RedoIcon,
   trackEvent: { category: "history" },
   viewMode: false,
-  perform: (elements, appState, _, app) =>
+  perform: (elements, appState, __, app) =>
     executeHistoryAction(app, appState, () =>
-      history.redo(
+      history.redo(arrayToMap(elements) as SceneElementsMap, appState),
-        arrayToMap(elements) as SceneElementsMap, // TODO: #7348 refactor action manager to already include `SceneElementsMap`
-        appState,
-        store.snapshot,
-      ),
     ),
   keyTest: (event) =>
     (event[KEYS.CTRL_OR_CMD] && event.shiftKey && matchKey(event, KEYS.Z)) ||

packages/excalidraw/change.ts

@@ -370,6 +370,7 @@ class Delta<T> {
       );
     }
 
+    // TODO: order the keys based on the most common ones to change (i.e. x/y, width/height, isDeleted, etc.)
     for (const key of keys) {
       const object1Value = object1[key as keyof T];
       const object2Value = object2[key as keyof T];
@@ -796,37 +797,41 @@ export class AppStateChange implements Change<AppState> {
   }
 }
 
-type ElementPartial<T extends ExcalidrawElement = ExcalidrawElement> =
+type ElementPartial<T extends ExcalidrawElement = ExcalidrawElement> = Omit<
-  ElementUpdate<Ordered<T>>;
+  ElementUpdate<Ordered<T>>,
+  "seed"
+>;
+
+type ElementsChangeOptions = {
+  id: string;
+  shouldRedistribute: boolean;
+};
 
 /**
  * Elements change is a low level primitive to capture a change between two sets of elements.
  * It does so by encapsulating forward and backward `Delta`s, allowing to time-travel in both directions.
  */
 export class ElementsChange implements Change<SceneElementsMap> {
-  public readonly id: string;
-
   private constructor(
+    public readonly id: string,
     private readonly added: Record<string, Delta<ElementPartial>>,
     private readonly removed: Record<string, Delta<ElementPartial>>,
     private readonly updated: Record<string, Delta<ElementPartial>>,
-    options: { changeId: string },
+  ) {}
-  ) {
-    this.id = options.changeId;
-  }
 
   public static create(
     added: Record<string, Delta<ElementPartial>>,
     removed: Record<string, Delta<ElementPartial>>,
     updated: Record<string, Delta<ElementPartial>>,
-    options: { changeId: string; shouldRedistribute: boolean } = {
+    options: ElementsChangeOptions = {
-      changeId: randomId(),
+      id: randomId(),
       shouldRedistribute: false,
     },
   ) {
+    const { id, shouldRedistribute } = options;
     let change: ElementsChange;
 
-    if (options.shouldRedistribute) {
+    if (shouldRedistribute) {
       const nextAdded: Record<string, Delta<ElementPartial>> = {};
       const nextRemoved: Record<string, Delta<ElementPartial>> = {};
       const nextUpdated: Record<string, Delta<ElementPartial>> = {};
@@ -847,13 +852,9 @@ export class ElementsChange implements Change<SceneElementsMap> {
         }
       }
 
-      change = new ElementsChange(nextAdded, nextRemoved, nextUpdated, {
+      change = new ElementsChange(id, nextAdded, nextRemoved, nextUpdated);
-        changeId: options.changeId,
-      });
     } else {
-      change = new ElementsChange(added, removed, updated, {
+      change = new ElementsChange(id, added, removed, updated);
-        changeId: options.changeId,
-      });
     }
 
     if (import.meta.env.DEV || import.meta.env.MODE === ENV.TEST) {
@@ -1000,7 +1001,7 @@ export class ElementsChange implements Change<SceneElementsMap> {
     const { id, added, removed, updated } = JSON.parse(payload);
 
     return ElementsChange.create(added, removed, updated, {
-      changeId: id,
+      id,
       shouldRedistribute: false,
     });
   }
@@ -1021,6 +1022,7 @@ export class ElementsChange implements Change<SceneElementsMap> {
     const updated = inverseInternal(this.updated);
 
     // notice we inverse removed with added not to break the invariants
+    // notice we force generate a new id
     return ElementsChange.create(removed, added, updated);
   }
 
@@ -1089,7 +1091,7 @@ export class ElementsChange implements Change<SceneElementsMap> {
     const updated = applyLatestChangesInternal(this.updated);
 
     return ElementsChange.create(added, removed, updated, {
-      changeId: this.id,
+      id: this.id,
       shouldRedistribute: true, // redistribute the deltas as `isDeleted` could have been updated
     });
   }
@@ -1232,7 +1234,7 @@ export class ElementsChange implements Change<SceneElementsMap> {
           }
         } else if (type === "added") {
           // for additions the element does not have to exist (i.e. remote update)
-          // TODO: the version itself might be different!
+          // CFDO: the version itself might be different!
           element = newElementWith(
             { id, version: 1 } as OrderedExcalidrawElement,
             {
@@ -1602,7 +1604,8 @@ export class ElementsChange implements Change<SceneElementsMap> {
   private static stripIrrelevantProps(
     partial: Partial<OrderedExcalidrawElement>,
   ): ElementPartial {
-    const { id, updated, version, versionNonce, ...strippedPartial } = partial;
+    const { id, updated, version, versionNonce, seed, ...strippedPartial } =
+      partial;
 
     return strippedPartial;
   }

packages/excalidraw/cloudflare/changes.ts

@@ -4,7 +4,7 @@ import type {
   SERVER_CHANGE,
 } from "../sync/protocol";
 
-// TODO: add senderId, possibly roomId as well
+// CFDO: add senderId, possibly roomId as well
 export class DurableChangesRepository implements ChangesRepository {
   constructor(private storage: DurableObjectStorage) {
     // #region DEV ONLY
@@ -24,7 +24,7 @@ export class DurableChangesRepository implements ChangesRepository {
       const prevVersion = this.getLastVersion();
       const nextVersion = prevVersion + changes.length;
 
-      // TODO: in theory payload could contain array of changes, if we would need to optimize writes
+      // CFDO: in theory payload could contain array of changes, if we would need to optimize writes
       for (const [index, change] of changes.entries()) {
         const version = prevVersion + index + 1;
         // unique id ensures that we don't acknowledge the same change twice

packages/excalidraw/cloudflare/room.ts

@@ -21,11 +21,11 @@ export class DurableRoom extends DurableObject {
     super(ctx, env);
 
     this.ctx.blockConcurrencyWhile(async () => {
-      // TODO: snapshot should likely be a transient store
+      // CFDO: snapshot should likely be a transient store
-      // TODO: loaded the latest state from the db
+      // CFDO: loaded the latest state from the db
       this.snapshot = {
-        // TODO: start persisting acknowledged version (not a scene version!)
+        // CFDO: start persisting acknowledged version (not a scene version!)
-        // TODO: we don't persist appState, should we?
+        // CFDO: we don't persist appState, should we?
         appState: {},
         elements: new Map(),
         version: 0,

packages/excalidraw/cloudflare/worker.ts

@@ -4,13 +4,12 @@ export { DurableRoom } from "./room";
  * Worker relay for Durable Room.
  */
 export default {
-  // TODO: ensure it's wss in the prod
   async fetch(
     request: Request,
     env: Env,
     ctx: ExecutionContext,
   ): Promise<Response> {
-    // TODO: only auth user should reach this
+    // CFDO: only auth user should reach this
     const upgrade = request.headers.get("upgrade");
     if (!upgrade || upgrade !== "websocket") {
       return new Response(null, { status: 426 /* upgrade required */ });
@@ -25,7 +24,7 @@ export default {
       return new Response(null, { status: 403 /* forbidden */ });
     }
 
-    // TODO: double check that the scene exists
+    // CFDO: double check that the scene exists
     const roomId = url.searchParams.get("roomId");
     if (!roomId) {
       return new Response(null, { status: 400 /* bad request */ });

packages/excalidraw/components/App.tsx

@@ -710,7 +710,7 @@ class App extends React.Component<AppProps, AppState> {
     this.visibleElements = [];
 
     this.store = new Store();
-    this.history = new History();
+    this.history = new History(this.store);
 
     if (excalidrawAPI) {
       const api: ExcalidrawImperativeAPI = {
@@ -759,15 +759,11 @@ class App extends React.Component<AppProps, AppState> {
     };
 
     this.fonts = new Fonts(this.scene);
-    this.history = new History();
+    this.history = new History(this.store);
 
     this.actionManager.registerAll(actions);
-    this.actionManager.registerAction(
+    this.actionManager.registerAction(createUndoAction(this.history));
-      createUndoAction(this.history, this.store),
+    this.actionManager.registerAction(createRedoAction(this.history));
-    );
-    this.actionManager.registerAction(
-      createRedoAction(this.history, this.store),
-    );
   }
 
   private onWindowMessage(event: MessageEvent) {
@@ -1821,6 +1817,10 @@ class App extends React.Component<AppProps, AppState> {
     return this.scene.getElementsIncludingDeleted();
   };
 
+  public getSceneElementsMapIncludingDeleted = () => {
+    return this.scene.getElementsMapIncludingDeleted();
+  }
+
   public getSceneElements = () => {
     return this.scene.getNonDeletedElements();
   };
@@ -2463,7 +2463,7 @@ class App extends React.Component<AppProps, AppState> {
     }
 
     this.store.onStoreIncrementEmitter.on((increment) => {
-      this.history.record(increment.elementsChange, increment.appStateChange);
+      this.history.record(increment);
       this.props.onIncrement?.(increment);
     });
 

packages/excalidraw/history.ts

@@ -1,9 +1,12 @@
-import type { AppStateChange, ElementsChange } from "./change";
-import type { SceneElementsMap } from "./element/types";
 import { Emitter } from "./emitter";
-import type { Snapshot } from "./store";
+import type { SceneElementsMap } from "./element/types";
+import type { Store, StoreIncrement } from "./store";
 import type { AppState } from "./types";
 
+type HistoryEntry = StoreIncrement & {
+  skipRecording?: true;
+};
+
 type HistoryStack = HistoryEntry[];
 
 export class HistoryChangedEvent {
@@ -18,8 +21,8 @@ export class History {
     [HistoryChangedEvent]
   >();
 
-  private readonly undoStack: HistoryStack = [];
+  public readonly undoStack: HistoryStack = [];
-  private readonly redoStack: HistoryStack = [];
+  public readonly redoStack: HistoryStack = [];
 
   public get isUndoStackEmpty() {
     return this.undoStack.length === 0;
@@ -29,6 +32,8 @@ export class History {
     return this.redoStack.length === 0;
   }
 
+  constructor(private readonly store: Store) {}
+
   public clear() {
     this.undoStack.length = 0;
     this.redoStack.length = 0;
@@ -37,13 +42,8 @@ export class History {
   /**
    * Record a local change which will go into the history
    */
-  public record(
+  public record(entry: HistoryEntry) {
-    elementsChange: ElementsChange,
+    if (!entry.skipRecording && !entry.isEmpty()) {
-    appStateChange: AppStateChange,
-  ) {
-    const entry = HistoryEntry.create(appStateChange, elementsChange);
-
-    if (!entry.isEmpty()) {
       // we have the latest changes, no need to `applyLatest`, which is done within `History.push`
       this.undoStack.push(entry.inverse());
 
@@ -60,29 +60,19 @@ export class History {
     }
   }
 
-  public undo(
+  public undo(elements: SceneElementsMap, appState: AppState) {
-    elements: SceneElementsMap,
-    appState: AppState,
-    snapshot: Readonly<Snapshot>,
-  ) {
     return this.perform(
       elements,
       appState,
-      snapshot,
       () => History.pop(this.undoStack),
       (entry: HistoryEntry) => History.push(this.redoStack, entry, elements),
     );
   }
 
-  public redo(
+  public redo(elements: SceneElementsMap, appState: AppState) {
-    elements: SceneElementsMap,
-    appState: AppState,
-    snapshot: Readonly<Snapshot>,
-  ) {
     return this.perform(
       elements,
       appState,
-      snapshot,
       () => History.pop(this.redoStack),
       (entry: HistoryEntry) => History.push(this.undoStack, entry, elements),
     );
@@ -91,7 +81,6 @@ export class History {
   private perform(
     elements: SceneElementsMap,
     appState: AppState,
-    snapshot: Readonly<Snapshot>,
     pop: () => HistoryEntry | null,
     push: (entry: HistoryEntry) => void,
   ): [SceneElementsMap, AppState] | void {
@@ -109,10 +98,17 @@ export class History {
       // iterate through the history entries in case they result in no visible changes
       while (historyEntry) {
         try {
+          // skip re-recording the history entry, as it gets emitted and is manually pushed to the undo / redo stack
+          Object.assign(historyEntry, { skipRecording: true });
+          // CFDO: consider encapsulating element & appState update inside applyIncrement
           [nextElements, nextAppState, containsVisibleChange] =
-            historyEntry.applyTo(nextElements, nextAppState, snapshot);
+            this.store.applyIncrementTo(
+              historyEntry,
+              nextElements,
+              nextAppState,
+            );
         } finally {
-          // make sure to always push / pop, even if the increment is corrupted
+          // make sure to always push, even if the increment is corrupted
           push(historyEntry);
         }
 
@@ -123,6 +119,10 @@ export class History {
         historyEntry = pop();
       }
 
+      if (nextElements === null || nextAppState === null) {
+        return;
+      }
+
       return [nextElements, nextAppState];
     } finally {
       // trigger the history change event before returning completely
@@ -156,55 +156,3 @@ export class History {
     return stack.push(updatedEntry);
   }
 }
-
-export class HistoryEntry {
-  private constructor(
-    public readonly appStateChange: AppStateChange,
-    public readonly elementsChange: ElementsChange,
-  ) {}
-
-  public static create(
-    appStateChange: AppStateChange,
-    elementsChange: ElementsChange,
-  ) {
-    return new HistoryEntry(appStateChange, elementsChange);
-  }
-
-  public inverse(): HistoryEntry {
-    return new HistoryEntry(
-      this.appStateChange.inverse(),
-      this.elementsChange.inverse(),
-    );
-  }
-
-  public applyTo(
-    elements: SceneElementsMap,
-    appState: AppState,
-    snapshot: Readonly<Snapshot>,
-  ): [SceneElementsMap, AppState, boolean] {
-    const [nextElements, elementsContainVisibleChange] =
-      this.elementsChange.applyTo(elements, snapshot.elements);
-
-    const [nextAppState, appStateContainsVisibleChange] =
-      this.appStateChange.applyTo(appState, nextElements);
-
-    const appliedVisibleChanges =
-      elementsContainVisibleChange || appStateContainsVisibleChange;
-
-    return [nextElements, nextAppState, appliedVisibleChanges];
-  }
-
-  /**
-   * Apply latest (remote) changes to the history entry, creates new instance of `HistoryEntry`.
-   */
-  public applyLatestChanges(elements: SceneElementsMap): HistoryEntry {
-    const updatedElementsChange =
-      this.elementsChange.applyLatestChanges(elements);
-
-    return HistoryEntry.create(this.appStateChange, updatedElementsChange);
-  }
-
-  public isEmpty(): boolean {
-    return this.appStateChange.isEmpty() && this.elementsChange.isEmpty();
-  }
-}

packages/excalidraw/package.json

@@ -62,7 +62,7 @@
     "@excalidraw/random-username": "1.1.0",
     "@radix-ui/react-popover": "1.0.3",
     "@radix-ui/react-tabs": "1.0.2",
-    "@types/async-lock": "1.4.2",
+    "@types/async-lock": "^1.4.2",
     "async-lock": "^1.4.1",
     "browser-fs-access": "0.29.1",
     "canvas-roundrect-polyfill": "0.0.1",

packages/excalidraw/store.ts

@@ -3,7 +3,10 @@ import { AppStateChange, ElementsChange } from "./change";
 import { ENV } from "./constants";
 import { newElementWith } from "./element/mutateElement";
 import { deepCopyElement } from "./element/newElement";
-import type { OrderedExcalidrawElement } from "./element/types";
+import type {
+  OrderedExcalidrawElement,
+  SceneElementsMap,
+} from "./element/types";
 import { Emitter } from "./emitter";
 import type { AppState, ObservedAppState } from "./types";
 import type { ValueOf } from "./utility-types";
@@ -73,82 +76,33 @@ export const StoreAction = {
 export type StoreActionType = ValueOf<typeof StoreAction>;
 
 /**
- * Represent an increment to the Store.
+ * Store which captures the observed changes and emits them as `StoreIncrement` events.
  */
-export class StoreIncrementEvent {
+export class Store {
-  constructor(
+  public readonly onStoreIncrementEmitter = new Emitter<[StoreIncrement]>();
-    public readonly elementsChange: ElementsChange,
-    public readonly appStateChange: AppStateChange,
-  ) {}
-}
-
-/**
- * Store which captures the observed changes and emits them as `StoreIncrementEvent` events.
- *
- * @experimental this interface is experimental and subject to change.
- */
-export interface IStore {
-  onStoreIncrementEmitter: Emitter<[StoreIncrementEvent]>;
-  get snapshot(): Snapshot;
-  set snapshot(snapshot: Snapshot);
-
-  /**
-   * Use to schedule update of the snapshot, useful on updates for which we don't need to calculate increments (i.e. remote updates).
-   */
-  shouldUpdateSnapshot(): void;
-
-  /**
-   * Use to schedule calculation of a store increment.
-   */
-  shouldCaptureIncrement(): void;
-
-  /**
-   * Based on the scheduled operation, either only updates store snapshot or also calculates increment and emits the result as a `StoreIncrementEvent`.
-   *
-   * @emits StoreIncrementEvent when increment is calculated.
-   */
-  commit(
-    elements: Map<string, OrderedExcalidrawElement> | undefined,
-    appState: AppState | ObservedAppState | undefined,
-  ): void;
-
-  /**
-   * Clears the store instance.
-   */
-  clear(): void;
-
-  /**
-   * Filters out yet uncomitted elements from `nextElements`, which are part of in-progress local async actions (ephemerals) and thus were not yet commited to the snapshot.
-   *
-   * This is necessary in updates in which we receive reconciled elements, already containing elements which were not yet captured by the local store (i.e. collab).
-   */
-  filterUncomittedElements(
-    prevElements: Map<string, OrderedExcalidrawElement>,
-    nextElements: Map<string, OrderedExcalidrawElement>,
-  ): Map<string, OrderedExcalidrawElement>;
-}
-
-export class Store implements IStore {
-  public readonly onStoreIncrementEmitter = new Emitter<
-    [StoreIncrementEvent]
-  >();
 
   private scheduledActions: Set<StoreActionType> = new Set();
-  private _snapshot = Snapshot.empty();
+  private _snapshot = StoreSnapshot.empty();
 
   public get snapshot() {
     return this._snapshot;
   }
 
-  public set snapshot(snapshot: Snapshot) {
+  public set snapshot(snapshot: StoreSnapshot) {
     this._snapshot = snapshot;
   }
 
+  /**
+   * Use to schedule calculation of a store increment.
+   */
   // TODO: Suspicious that this is called so many places. Seems error-prone.
   public shouldCaptureIncrement = () => {
     this.scheduleAction(StoreAction.CAPTURE);
   };
 
+  /**
+   * Use to schedule update of the snapshot, useful on updates for which we don't need to calculate increments (i.e. remote updates).
+   */
   public shouldUpdateSnapshot = () => {
     this.scheduleAction(StoreAction.UPDATE);
   };
@@ -158,6 +112,11 @@ export class Store implements IStore {
     this.satisfiesScheduledActionsInvariant();
   };
 
+  /**
+   * Based on the scheduled operation, either only updates store snapshot or also calculates increment and emits the result as a `StoreIncrement`.
+   *
+   * @emits StoreIncrement when increment is calculated.
+   */
   public commit = (
     elements: Map<string, OrderedExcalidrawElement> | undefined,
     appState: AppState | ObservedAppState | undefined,
@@ -176,6 +135,11 @@ export class Store implements IStore {
     }
   };
 
+  /**
+   * Performs diff calculation, calculates and emits the increment.
+   *
+   * @emits StoreIncrement when increment is calculated.
+   */
   public captureIncrement = (
     elements: Map<string, OrderedExcalidrawElement> | undefined,
     appState: AppState | ObservedAppState | undefined,
@@ -186,18 +150,18 @@ export class Store implements IStore {
     // Optimisation, don't continue if nothing has changed
     if (prevSnapshot !== nextSnapshot) {
       // Calculate and record the changes based on the previous and next snapshot
-      const elementsChange = nextSnapshot.meta.didElementsChange
+      const elementsChange = nextSnapshot.metadata.didElementsChange
         ? ElementsChange.calculate(prevSnapshot.elements, nextSnapshot.elements)
         : ElementsChange.empty();
 
-      const appStateChange = nextSnapshot.meta.didAppStateChange
+      const appStateChange = nextSnapshot.metadata.didAppStateChange
         ? AppStateChange.calculate(prevSnapshot.appState, nextSnapshot.appState)
         : AppStateChange.empty();
 
       if (!elementsChange.isEmpty() || !appStateChange.isEmpty()) {
         // Notify listeners with the increment
         this.onStoreIncrementEmitter.trigger(
-          new StoreIncrementEvent(elementsChange, appStateChange),
+          new StoreIncrement(elementsChange, appStateChange),
         );
       }
 
@@ -206,6 +170,9 @@ export class Store implements IStore {
     }
   };
 
+  /**
+   * Updates the snapshot without performing any diff calculation.
+   */
   public updateSnapshot = (
     elements: Map<string, OrderedExcalidrawElement> | undefined,
     appState: AppState | ObservedAppState | undefined,
@@ -218,6 +185,11 @@ export class Store implements IStore {
     }
   };
 
+  /**
+   * Filters out yet uncomitted elements from `nextElements`, which are part of in-progress local async actions (ephemerals) and thus were not yet commited to the snapshot.
+   *
+   * This is necessary in updates in which we receive reconciled elements, already containing elements which were not yet captured by the local store (i.e. collab).
+   */
   public filterUncomittedElements = (
     prevElements: Map<string, OrderedExcalidrawElement>,
     nextElements: Map<string, OrderedExcalidrawElement>,
@@ -245,8 +217,35 @@ export class Store implements IStore {
     return nextElements;
   };
 
+  /**
+   * Apply and emit increment.
+   *
+   * @emits StoreIncrement when increment is applied.
+   */
+  public applyIncrementTo = (
+    increment: StoreIncrement,
+    elements: SceneElementsMap,
+    appState: AppState,
+  ): [SceneElementsMap, AppState, boolean] => {
+    const [nextElements, elementsContainVisibleChange] =
+      increment.elementsChange.applyTo(elements, this.snapshot.elements);
+
+    const [nextAppState, appStateContainsVisibleChange] =
+      increment.appStateChange.applyTo(appState, nextElements);
+
+    const appliedVisibleChanges =
+      elementsContainVisibleChange || appStateContainsVisibleChange;
+
+    this.onStoreIncrementEmitter.trigger(increment);
+
+    return [nextElements, nextAppState, appliedVisibleChanges];
+  };
+
+  /**
+   * Clears the store instance.
+   */
   public clear = (): void => {
-    this.snapshot = Snapshot.empty();
+    this.snapshot = StoreSnapshot.empty();
     this.scheduledActions = new Set();
   };
 
@@ -262,11 +261,42 @@ export class Store implements IStore {
   };
 }
 
-export class Snapshot {
+/**
+ * Represent an increment to the Store.
+ */
+export class StoreIncrement {
+  constructor(
+    public readonly elementsChange: ElementsChange,
+    public readonly appStateChange: AppStateChange,
+  ) {}
+
+  public inverse(): StoreIncrement {
+    return new StoreIncrement(
+      this.elementsChange.inverse(),
+      this.appStateChange.inverse(),
+    );
+  }
+
+  /**
+   * Apply latest (remote) changes to the increment, creates new instance of `StoreIncrement`.
+   */
+  public applyLatestChanges(elements: SceneElementsMap): StoreIncrement {
+    const updatedElementsChange =
+      this.elementsChange.applyLatestChanges(elements);
+
+    return new StoreIncrement(updatedElementsChange, this.appStateChange);
+  }
+
+  public isEmpty() {
+    return this.elementsChange.isEmpty() && this.appStateChange.isEmpty();
+  }
+}
+
+export class StoreSnapshot {
   private constructor(
     public readonly elements: Map<string, OrderedExcalidrawElement>,
     public readonly appState: ObservedAppState,
-    public readonly meta: {
+    public readonly metadata: {
       didElementsChange: boolean;
       didAppStateChange: boolean;
       isEmpty?: boolean;
@@ -278,7 +308,7 @@ export class Snapshot {
   ) {}
 
   public static empty() {
-    return new Snapshot(
+    return new StoreSnapshot(
       new Map(),
       getObservedAppState(getDefaultAppState() as AppState),
       { didElementsChange: false, didAppStateChange: false, isEmpty: true },
@@ -286,7 +316,7 @@ export class Snapshot {
   }
 
   public isEmpty() {
-    return this.meta.isEmpty;
+    return this.metadata.isEmpty;
   }
 
   /**
@@ -316,10 +346,14 @@ export class Snapshot {
       return this;
     }
 
-    const snapshot = new Snapshot(nextElementsSnapshot, nextAppStateSnapshot, {
+    const snapshot = new StoreSnapshot(
-      didElementsChange,
+      nextElementsSnapshot,
-      didAppStateChange,
+      nextAppStateSnapshot,
-    });
+      {
+        didElementsChange,
+        didAppStateChange,
+      },
+    );
 
     return snapshot;
   }

packages/excalidraw/sync/client.ts

@@ -7,7 +7,6 @@ import type { CLIENT_CHANGE, PUSH_PAYLOAD, SERVER_CHANGE } from "./protocol";
 import throttle from "lodash.throttle";
 
 export class ExcalidrawSyncClient {
-  // TODO: add prod url
   private static readonly HOST_URL = import.meta.env.DEV
     ? "ws://localhost:8787"
     : "https://excalidraw-sync.marcel-529.workers.dev";
@@ -46,11 +45,11 @@ export class ExcalidrawSyncClient {
     this.api = api;
     this.roomId = roomId;
 
-    // TODO: persist in idb
+    // CFDO: persist in idb
     this.lastAcknowledgedVersion = 0;
   }
 
-  // TODO: throttle does not work that well here (after some period it tries to reconnect too often)
+  // CFDO: throttle does not work that well here (after some period it tries to reconnect too often)
   public reconnect = throttle(
     async () => {
       try {
@@ -164,7 +163,7 @@ export class ExcalidrawSyncClient {
     );
   };
 
-  // TODO: could be an array buffer
+  // CFDO: could be an array buffer
   private onMessage = (event: MessageEvent) => {
     const [result, error] = Utils.try(() => JSON.parse(event.data as string));
 
@@ -202,7 +201,7 @@ export class ExcalidrawSyncClient {
     const payload: PUSH_PAYLOAD = { type, changes: [] };
 
     if (type === "durable") {
-      // TODO: persist in idb (with insertion order)
+      // CFDO: persist in idb (with insertion order)
       for (const change of changes) {
         this.queuedChanges.set(change.id, {
           queuedAt: Date.now(),
@@ -231,7 +230,7 @@ export class ExcalidrawSyncClient {
     });
   }
 
-  // TODO: refactor by applying all operations to store, not to the elements
+  // CFDO: refactor by applying all operations to store, not to the elements
   private handleAcknowledged(payload: { changes: Array<SERVER_CHANGE> }) {
     const { changes: remoteChanges } = payload;
 
@@ -254,7 +253,7 @@ export class ExcalidrawSyncClient {
           // local change acknowledge by the server, safe to remove
           this.queuedChanges.delete(remoteChange.id);
         } else {
-          // TODO: we might not need to be that strict here
+          // CFDO: we might not need to be that strict here
           if (this.lastAcknowledgedVersion + 1 !== remoteChange.version) {
             throw new Error(
               `Received out of order change, expected "${
@@ -282,7 +281,7 @@ export class ExcalidrawSyncClient {
       );
 
       // apply local changes
-      // TODO: only necessary when remote changes modified same element properties!
+      // CFDO: only necessary when remote changes modified same element properties!
       for (const localChange of this.localChanges) {
         [elements] = localChange.applyTo(
           elements,

packages/excalidraw/sync/protocol.ts

@@ -32,7 +32,7 @@ export interface ChangesRepository {
   getLastVersion(): number;
 }
 
-// TODO: should come from the shared types package
+// CFDO: should come from the shared types package
 export type ExcalidrawElement = {
   id: string;
   type: any;

packages/excalidraw/sync/server.ts

@@ -11,7 +11,7 @@ import type {
   SERVER_MESSAGE,
 } from "./protocol";
 
-// TODO: message could be binary (cbor, protobuf, etc.)
+// CFDO: message could be binary (cbor, protobuf, etc.)
 
 /**
  * Core excalidraw sync logic.
@@ -48,7 +48,7 @@ export class ExcalidrawSyncServer {
         return this.pull(client, payload);
       case "push":
         // apply each one-by-one to avoid race conditions
-        // TODO: in theory we do not need to block ephemeral appState changes
+        // CFDO: in theory we do not need to block ephemeral appState changes
         return this.lock.acquire("push", () => this.push(client, payload));
       default:
         console.error(`Unknown message type: ${type}`);
@@ -56,7 +56,7 @@ export class ExcalidrawSyncServer {
   }
 
   private pull(client: WebSocket, payload: PULL_PAYLOAD) {
-    // TODO: test for invalid payload
+    // CFDO: test for invalid payload
     const lastAcknowledgedClientVersion = payload.lastAcknowledgedVersion;
     const lastAcknowledgedServerVersion =
       this.changesRepository.getLastVersion();
@@ -70,7 +70,7 @@ export class ExcalidrawSyncServer {
     }
 
     if (versionΔ < 0) {
-      // TODO: restore the client from the snapshot / deltas?
+      // CFDO: restore the client from the snapshot / deltas?
       console.error(
         `Panic! Client claims to have higher acknowledged version than the latest one on the server!`,
       );
@@ -78,7 +78,7 @@ export class ExcalidrawSyncServer {
     }
 
     if (versionΔ > 0) {
-      // TODO: for versioning we need deletions, but not for the "snapshot" update
+      // CFDO: for versioning we need deletions, but not for the "snapshot" update
       const changes = this.changesRepository.getSinceVersion(
         lastAcknowledgedClientVersion,
       );
@@ -99,7 +99,7 @@ export class ExcalidrawSyncServer {
         return this.relay(client, { changes });
       case "durable":
         const [acknowledged, error] = Utils.try(() => {
-          // TODO: try to apply the changes to the snapshot
+          // CFDO: try to apply the changes to the snapshot
           return this.changesRepository.saveAll(changes);
         });
 

packages/excalidraw/tests/history.test.tsx

@@ -16,7 +16,6 @@ import { fireEvent, queryByTestId, waitFor } from "@testing-library/react";
 import { createUndoAction, createRedoAction } from "../actions/actionHistory";
 import { actionToggleViewMode } from "../actions/actionToggleViewMode";
 import { EXPORT_DATA_TYPES, MIME_TYPES } from "../constants";
-import type { AppState } from "../types";
 import { arrayToMap } from "../utils";
 import {
   COLOR_PALETTE,
@@ -42,11 +41,11 @@ import {
 } from "../actions";
 import { vi } from "vitest";
 import { queryByText } from "@testing-library/react";
-import { HistoryEntry } from "../history";
 import { AppStateChange, ElementsChange } from "../change";
-import { Snapshot, StoreAction } from "../store";
+import { StoreAction, StoreIncrement } from "../store";
 import type { LocalPoint, Radians } from "../../math";
 import { pointFrom } from "../../math";
+import type { AppState } from "../types.js";
 
 const { h } = window;
 
@@ -65,7 +64,8 @@ const checkpoint = (name: string) => {
     ...strippedAppState
   } = h.state;
   expect(strippedAppState).toMatchSnapshot(`[${name}] appState`);
-  expect(h.history).toMatchSnapshot(`[${name}] history`);
+  expect(h.history.undoStack).toMatchSnapshot(`[${name}] undo stack`);
+  expect(h.history.redoStack).toMatchSnapshot(`[${name}] redo stack`);
   expect(h.elements.length).toMatchSnapshot(`[${name}] number of elements`);
   h.elements
     .map(({ seed, versionNonce, ...strippedElement }) => strippedElement)
@@ -99,14 +99,16 @@ describe("history", () => {
 
       API.setElements([rect]);
 
-      const corrupedEntry = HistoryEntry.create(
+      const corrupedEntry = new StoreIncrement(
-        AppStateChange.empty(),
         ElementsChange.empty(),
+        AppStateChange.empty(),
       );
 
-      vi.spyOn(corrupedEntry, "applyTo").mockImplementation(() => {
+      vi.spyOn(corrupedEntry.elementsChange, "applyTo").mockImplementation(
-        throw new Error("Oh no, I am corrupted!");
+        () => {
-      });
+          throw new Error("Oh no, I am corrupted!");
+        },
+      );
 
       (h.history as any).undoStack.push(corrupedEntry);
 
@@ -119,7 +121,6 @@ describe("history", () => {
             h.history.undo(
               arrayToMap(h.elements) as SceneElementsMap,
               appState,
-              Snapshot.empty(),
             ) as any,
         );
       } catch (e) {
@@ -140,7 +141,6 @@ describe("history", () => {
             h.history.redo(
               arrayToMap(h.elements) as SceneElementsMap,
               appState,
-              Snapshot.empty(),
             ) as any,
         );
       } catch (e) {
@@ -437,8 +437,8 @@ describe("history", () => {
         expect(h.history.isUndoStackEmpty).toBeTruthy();
       });
 
-      const undoAction = createUndoAction(h.history, h.store);
+      const undoAction = createUndoAction(h.history);
-      const redoAction = createRedoAction(h.history, h.store);
+      const redoAction = createRedoAction(h.history);
       // noop
       API.executeAction(undoAction);
       expect(h.elements).toEqual([
@@ -514,8 +514,8 @@ describe("history", () => {
         expect.objectContaining({ id: "B", isDeleted: false }),
       ]);
 
-      const undoAction = createUndoAction(h.history, h.store);
+      const undoAction = createUndoAction(h.history);
-      const redoAction = createRedoAction(h.history, h.store);
+      const redoAction = createRedoAction(h.history);
       API.executeAction(undoAction);
 
       expect(API.getSnapshot()).toEqual([
@@ -1714,8 +1714,8 @@ describe("history", () => {
         />,
       );
 
-      const undoAction = createUndoAction(h.history, h.store);
+      const undoAction = createUndoAction(h.history);
-      const redoAction = createRedoAction(h.history, h.store);
+      const redoAction = createRedoAction(h.history);
 
       await waitFor(() => {
         expect(h.elements).toEqual([expect.objectContaining({ id: "A" })]);
@@ -1764,7 +1764,7 @@ describe("history", () => {
         />,
       );
 
-      const undoAction = createUndoAction(h.history, h.store);
+      const undoAction = createUndoAction(h.history);
 
       await waitFor(() => {
         expect(h.elements).toEqual([expect.objectContaining({ id: "A" })]);

packages/excalidraw/types.ts

@@ -40,7 +40,7 @@ import type { IMAGE_MIME_TYPES, MIME_TYPES } from "./constants";
 import type { ContextMenuItems } from "./components/ContextMenu";
 import type { SnapLine } from "./snapping";
 import type { Merge, MaybePromise, ValueOf, MakeBrand } from "./utility-types";
-import type { StoreActionType, StoreIncrementEvent } from "./store";
+import type { StoreActionType, StoreIncrement } from "./store";
 
 export type SocketId = string & { _brand: "SocketId" };
 
@@ -498,7 +498,7 @@ export interface ExcalidrawProps {
     appState: AppState,
     files: BinaryFiles,
   ) => void;
-  onIncrement?: (event: StoreIncrementEvent) => void;
+  onIncrement?: (event: StoreIncrement) => void;
   initialData?:
     | (() => MaybePromise<ExcalidrawInitialDataState | null>)
     | MaybePromise<ExcalidrawInitialDataState | null>;
@@ -785,7 +785,7 @@ export interface ExcalidrawImperativeAPI {
     ) => void,
   ) => UnsubscribeCallback;
   onIncrement: (
-    callback: (event: StoreIncrementEvent) => void,
+    callback: (event: StoreIncrement) => void,
   ) => UnsubscribeCallback;
   onPointerDown: (
     callback: (

yarn.lock

@@ -3273,7 +3273,7 @@
   resolved "https://registry.yarnpkg.com/@types/aria-query/-/aria-query-5.0.4.tgz#1a31c3d378850d2778dabb6374d036dcba4ba708"
   integrity sha512-rfT93uj5s0PRL7EzccGMs3brplhcrghnDoV26NqKhCAS1hVo+WdNsPvE/yb6ilfr5hi2MEk6d5EWJTKdxg8jVw==
 
-"@types/async-lock@1.4.2":
+"@types/async-lock@^1.4.2":
   version "1.4.2"
   resolved "https://registry.yarnpkg.com/@types/async-lock/-/async-lock-1.4.2.tgz#c2037ba1d6018de766c2505c3abe3b7b6b244ab4"
   integrity sha512-HlZ6Dcr205BmNhwkdXqrg2vkFMN2PluI7Lgr8In3B3wE5PiQHhjRqtW/lGdVU9gw+sM0JcIDx2AN+cW8oSWIcw==