feat: Make model.save() optimistic with rollback on failure

Apply saved params to the model immediately so MobX observers update
without waiting for the server, and roll the model back to its previous
state when the request fails. New models with a client-generated id are
optimistically added to the store and removed again on failure.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019LA4pYSG3Z2X8Xqo598JGn

app/models/Document.ts

@@ -1,7 +1,15 @@
 import { addDays, differenceInDays } from "date-fns";
 import i18n, { t } from "i18next";
 import { capitalize, floor } from "es-toolkit/compat";
-import { action, autorun, comparer, computed, observable, set } from "mobx";
+import {
+  action,
+  autorun,
+  comparer,
+  computed,
+  observable,
+  set,
+  toJS,
+} from "mobx";
 import type {
   JSONObject,
   NavigationNode,
@@ -562,6 +570,18 @@ export default class Document extends ArchivableModel implements Searchable {
     const params = fields ?? this.toAPI();
     this.isSaving = true;
 
+    // Snapshot the current values of the saved fields so that the optimistic
+    // update below can be rolled back if saving fails.
+    const previousValues: Record<string, unknown> = {};
+    for (const key in params) {
+      // @ts-expect-error TODO
+      previousValues[key] = toJS(this[key]);
+    }
+
+    // optimistically set the new values on the model so that observers update
+    // immediately, without waiting for the server to respond
+    set(this, { ...params, ...fields });
+
     try {
       const model = await this.store.save(
         { ...params, ...fields, id: this.id },
@@ -574,6 +594,10 @@ export default class Document extends ArchivableModel implements Searchable {
       this.persistedAttributes = this.toAPI();
 
       return model;
+    } catch (err) {
+      // roll the model back to its state before saving began
+      set(this, previousValues);
+      throw err;
     } finally {
       this.isSaving = false;
     }

app/models/base/Model.test.ts

@@ -0,0 +1,119 @@
+import { vi } from "vitest";
+import Collection from "~/models/Collection";
+import stores from "~/stores";
+import { client } from "~/utils/ApiClient";
+
+const post = vi.mocked(client.post);
+
+describe("Model#save", () => {
+  beforeEach(() => {
+    post.mockReset();
+  });
+
+  describe("updating an existing model", () => {
+    test("should optimistically apply changes and keep them on success", async () => {
+      const collection = stores.collections.add({
+        id: "update-success",
+        name: "Before",
+      });
+
+      let resolve!: (value: unknown) => void;
+      post.mockReturnValueOnce(
+        new Promise((res) => {
+          resolve = res;
+        })
+      );
+
+      const promise = collection.save({ name: "After" });
+
+      // the change is visible before the server has responded
+      expect(collection.name).toBe("After");
+      expect(collection.isSaving).toBe(true);
+      expect(collection.isDirty()).toBe(true);
+
+      resolve({
+        data: { id: "update-success", name: "After" },
+        policies: [],
+      });
+      await promise;
+
+      expect(collection.name).toBe("After");
+      expect(collection.isSaving).toBe(false);
+      expect(collection.isDirty()).toBe(false);
+    });
+
+    test("should roll back optimistic changes on failure", async () => {
+      const collection = stores.collections.add({
+        id: "update-failure",
+        name: "Before",
+      });
+
+      post.mockRejectedValueOnce(new Error("server error"));
+
+      const promise = collection.save({ name: "After" });
+
+      // the change is visible before the server has responded
+      expect(collection.name).toBe("After");
+
+      await expect(promise).rejects.toThrow("server error");
+
+      expect(collection.name).toBe("Before");
+      expect(collection.isSaving).toBe(false);
+      expect(collection.isDirty()).toBe(false);
+    });
+
+    test("should not roll back changes made to the model before saving", async () => {
+      const collection = stores.collections.add({
+        id: "update-premutation",
+        name: "Before",
+      });
+      collection.name = "Edited";
+
+      post.mockRejectedValueOnce(new Error("server error"));
+
+      await expect(collection.save()).rejects.toThrow("server error");
+
+      // changes made directly to the model before calling save are kept so
+      // that they are not lost and the save can be retried
+      expect(collection.name).toBe("Edited");
+      expect(collection.isDirty()).toBe(true);
+    });
+  });
+
+  describe("creating a new model", () => {
+    test("should optimistically add the model to the store and keep it on success", async () => {
+      const collection = new Collection({ name: "Draft" }, stores.collections);
+      collection.id = "create-success";
+      expect(collection.isNew).toBe(true);
+
+      post.mockResolvedValueOnce({
+        data: { id: "create-success", name: "Draft" },
+        policies: [],
+      });
+
+      await collection.save();
+
+      expect(stores.collections.get("create-success")).toBe(collection);
+      expect(collection.isNew).toBe(false);
+    });
+
+    test("should remove the model from the store on failure", async () => {
+      const collection = new Collection({ name: "Draft" }, stores.collections);
+      collection.id = "create-failure";
+      expect(collection.isNew).toBe(true);
+
+      post.mockRejectedValueOnce(new Error("server error"));
+
+      const promise = collection.save();
+
+      // the model is in the store before the server has responded
+      expect(stores.collections.get("create-failure")).toBe(collection);
+      expect(collection.isNew).toBe(false);
+
+      await expect(promise).rejects.toThrow("server error");
+
+      expect(stores.collections.get("create-failure")).toBeUndefined();
+      expect(collection.isNew).toBe(true);
+    });
+  });
+});

app/models/base/Model.ts

@@ -1,5 +1,5 @@
 import { isEqual, pick } from "es-toolkit/compat";
-import { observable, action, toJS } from "mobx";
+import { observable, action, runInAction, toJS } from "mobx";
 import type { JSONObject } from "@shared/types";
 import type Store from "~/stores/base/Store";
 import Logger from "~/utils/Logger";
@@ -82,11 +82,16 @@ export default abstract class Model {
   }
 
   /**
-   * Persists the model to the server API
+   * Persists the model to the server API.
+   *
+   * The change is applied optimistically – the given params are set on the
+   * model immediately so that observers of the store update without waiting
+   * for the server to respond, and are rolled back if the request fails.
    *
    * @param params Specific fields to save, if not provided the model will be serialized
    * @param options Options to pass to the store
    * @returns A promise that resolves with the updated model
+   * @throws The original error if the request fails, after the model has been rolled back.
    */
   save = async (
     params?: Record<string, unknown>,
@@ -95,22 +100,45 @@ export default abstract class Model {
     const isNew = this.isNew;
     this.isSaving = true;
 
-    try {
-      // ensure that the id is passed if the document has one
-      if (!params) {
-        params = this.toAPI();
-      }
+    // ensure that the id is passed if the document has one
+    const data: Record<string, unknown> = params ?? this.toAPI();
 
+    // Snapshot the current state of the model so that the optimistic update
+    // below can be rolled back if saving fails.
+    const previousAttributes = this.persistedAttributes;
+    const previousValues: Record<string, unknown> = {};
+    for (const key in data) {
+      // @ts-expect-error TODO
+      previousValues[key] = toJS(this[key]);
+    }
+    const previousId = this.id;
+    let addedToStore = false;
+
+    try {
       if (isNew) {
         LifecycleManager.executeHooks(this.constructor, "beforeCreate", this);
       } else {
         LifecycleManager.executeHooks(this.constructor, "beforeUpdate", this);
       }
 
+      // Optimistically apply the new values so that observers of the store
+      // update immediately, without waiting for the server to respond.
+      runInAction(() => {
+        this.updateData(data);
+        // Keep the attributes last received from the server so that `isDirty`
+        // stays accurate while the request is in flight.
+        this.persistedAttributes = previousAttributes;
+
+        if (isNew && this.id && !this.store.get(this.id)) {
+          this.store.add(this);
+          addedToStore = true;
+        }
+      });
+
       const model = await this.store.save(
         {
-          ...params,
-          id: this.id,
+          ...data,
+          id: previousId,
         },
         {
           ...options,
@@ -119,7 +147,17 @@ export default abstract class Model {
       );
 
       // if saving is successful set the new values on the model itself
-      this.updateData(Object.assign({}, params, model));
+      this.updateData(Object.assign({}, data, model));
+
+      // if the server responded with a different model to the one added
+      // optimistically then remove the temporary entry from the store
+      if (addedToStore && model !== this) {
+        runInAction(() => {
+          if (this.store.get(previousId) === this) {
+            this.store.data.delete(previousId);
+          }
+        });
+      }
 
       if (isNew) {
         LifecycleManager.executeHooks(this.constructor, "afterCreate", this);
@@ -128,6 +166,18 @@ export default abstract class Model {
       }
 
       return model;
+    } catch (err) {
+      // roll the model back to its state before saving began
+      runInAction(() => {
+        this.updateData(previousValues);
+        this.persistedAttributes = previousAttributes;
+        this.isNew = isNew;
+
+        if (addedToStore && this.store.get(previousId) === this) {
+          this.store.data.delete(previousId);
+        }
+      });
+      throw err;
     } finally {
       this.isSaving = false;
     }