Fix client validation flickers

.changeset/fix-save-content-flicker.md

@@ -0,0 +1,8 @@
+---
+"@valbuild/ui": patch
+---
+
+Fix two flickers on save:
+
+- Field content briefly reverted to its pre-edit value before settling on the saved value. In fs mode, `publish()` now bakes the optimistic (patched) value into the local sources as it drops the just-saved patches, so the displayed value stays stable until the next sources sync.
+- The Save button briefly flicked back to enabled right after saving. `publish()` now invalidates the server-side patch-id snapshot when it empties it, so the button transitions enabled → disabled cleanly instead of reading a stale value until the next sync.

examples/next/val.config.ts

@@ -1,7 +1,7 @@
 import { initVal } from "@valbuild/next";
 
 const { s, c, val, config, nextAppRouter, externalPageRouter } = initVal({
-  // project: "valbuild/val-examples-next",
+  project: "valbuild/val-examples-next",
   root: "/examples/next",
   defaultTheme: "dark",
   ai: {

packages/ui/spa/ValSyncEngine.test.ts

@@ -68,6 +68,187 @@ describe("ValSyncEngine", () => {
     ).toStrictEqual("value 1 from store 1");
   });
 
+  test("fs publish keeps the patched value (no flicker back to base)", async () => {
+    // Regression test for the save-flicker: after publish() drops the now-saved
+    // patches in fs mode, serverSources still holds the un-patched base. Without
+    // baking the optimistic value into serverSources first, the field would
+    // momentarily revert to the pre-patch value until the next /sources/~ sync.
+    const { s, c, config } = initVal();
+    const tester = new SyncEngineTester(
+      "fs",
+      [c.define("/test.val.ts", s.string().minLength(2), "Foo")],
+      config,
+    );
+    const syncEngine = await tester.createInitializedSyncEngine();
+    expect(
+      syncEngine.getSourceSnapshot(toModuleFilePath("/test.val.ts")).data,
+    ).toStrictEqual("Foo");
+
+    syncEngine.addPatch(
+      toSourcePath("/test.val.ts"),
+      "string",
+      [{ op: "replace", path: [], value: "FooBar" }],
+      tester.getNextNow(),
+    );
+    // Optimistic value is shown immediately.
+    expect(
+      syncEngine.getSourceSnapshot(toModuleFilePath("/test.val.ts")).data,
+    ).toStrictEqual("FooBar");
+
+    // serverSources is still the un-patched base at this point (the studio reads
+    // /sources/~ with apply_patches=false), so publishing must bake the patched
+    // value in as it drops the patch chain.
+    const patchIds = syncEngine.getPendingClientSidePatchIdsSnapshot();
+    expect(patchIds.length).toBeGreaterThan(0);
+    expect(
+      await syncEngine.publish(patchIds, undefined, tester.getNextNow()),
+    ).toMatchObject({
+      status: "done",
+    });
+
+    // A field re-rendering after publish (fresh creatorId → fresh getPatchedSource
+    // recompute, i.e. what HMR / the next sync's source invalidation triggers)
+    // must NOT see the pre-patch value flicker through. This is the assertion
+    // that fails without the fix (it would recompute base "Foo" + no patches).
+    expect(
+      syncEngine.getSourceSnapshot(
+        toModuleFilePath("/test.val.ts"),
+        "field-rerender-after-publish",
+      ).data,
+    ).toStrictEqual("FooBar");
+
+    // And it stays "FooBar" after the follow-up stat-triggered sync.
+    tester.simulatePassingOfSeconds(5);
+    expect(await tester.simulateStatCallback(syncEngine)).toMatchObject({
+      status: "done",
+    });
+    expect(
+      syncEngine.getSourceSnapshot(toModuleFilePath("/test.val.ts")).data,
+    ).toStrictEqual("FooBar");
+  });
+
+  test("fs publish clears the server-side patch-id snapshot (no Save button re-enable)", async () => {
+    // Regression test for the Save button flicker: after publish() empties
+    // globalServerSidePatchIds in fs mode, its snapshot must be invalidated too.
+    // Otherwise getGlobalServerSidePatchIdsSnapshot() (which the button reads as
+    // pendingServerSidePatchIds) stays stale and non-empty until the next
+    // stat/sync, so the button briefly flips back to enabled when publish()'s
+    // finally clears publishDisabled.
+    const { s, c, config } = initVal();
+    const tester = new SyncEngineTester(
+      "fs",
+      [c.define("/test.val.ts", s.string().minLength(2), "Foo")],
+      config,
+    );
+    const syncEngine = await tester.createInitializedSyncEngine();
+    syncEngine.addPatch(
+      toSourcePath("/test.val.ts"),
+      "string",
+      [{ op: "replace", path: [], value: "FooBar" }],
+      tester.getNextNow(),
+    );
+    // Push the patch to the server and promote it to a global server-side patch
+    // id via the stat callback — this is the button's "enabled" precondition.
+    expect(await syncEngine.sync(tester.getNextNow())).toMatchObject({
+      status: "done",
+    });
+    expect(await tester.simulateStatCallback(syncEngine)).toMatchObject({
+      status: "done",
+    });
+    const patchIds = syncEngine.getGlobalServerSidePatchIdsSnapshot();
+    expect(patchIds.length).toBeGreaterThan(0);
+
+    expect(
+      await syncEngine.publish(patchIds, undefined, tester.getNextNow()),
+    ).toMatchObject({
+      status: "done",
+    });
+
+    // Immediately after publish (before any further stat/sync) the server-side
+    // patch-id snapshot must be empty — this fails without the invalidation.
+    expect(syncEngine.getGlobalServerSidePatchIdsSnapshot()).toStrictEqual([]);
+  });
+
+  test("fs publish persists patches that contain file ops", async () => {
+    // File ops carry binary content, not document mutations, so applyPatch
+    // rejects them: the fake /save must filter them out (as the real server
+    // does) before applying the rest of the patch. Otherwise an image / gallery
+    // patch never persists and the value reverts on the next sources sync.
+    const { s, c, config } = initVal();
+    const ref = "/public/val/test_a1b2c.png";
+    const metadata = {
+      width: 10,
+      height: 20,
+      mimeType: "image/png",
+      alt: "",
+    };
+    const tester = new SyncEngineTester(
+      "fs",
+      [
+        c.define(
+          "/gallery.val.ts",
+          s.record(
+            s.object({
+              width: s.number(),
+              height: s.number(),
+              mimeType: s.string(),
+              alt: s.string(),
+            }),
+          ),
+          {},
+        ),
+      ],
+      config,
+    );
+    const syncEngine = await tester.createInitializedSyncEngine();
+    // Same shape as ModuleGallery: the metadata entry is added by a JSON op and
+    // the binary is carried by a file op on the same path (in the real flow
+    // `value` is the sha256 of the already uploaded content).
+    syncEngine.addPatch(
+      toSourcePath("/gallery.val.ts"),
+      "record",
+      [
+        { op: "add", path: [ref], value: metadata },
+        {
+          op: "file",
+          path: [ref],
+          filePath: ref,
+          value:
+            "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
+          remote: false,
+          metadata,
+        },
+      ],
+      tester.getNextNow(),
+    );
+    expect(await syncEngine.sync(tester.getNextNow())).toMatchObject({
+      status: "done",
+    });
+    expect(await tester.simulateStatCallback(syncEngine)).toMatchObject({
+      status: "done",
+    });
+    const patchIds = syncEngine.getGlobalServerSidePatchIdsSnapshot();
+    expect(patchIds.length).toBeGreaterThan(0);
+    expect(
+      await syncEngine.publish(patchIds, undefined, tester.getNextNow()),
+    ).toMatchObject({
+      status: "done",
+    });
+    // /save must have applied the non-file half of the patch to the sources...
+    expect(tester.fakeSources["/gallery.val.ts"]).toStrictEqual({
+      [ref]: metadata,
+    });
+    // ...so the entry survives the follow-up stat-triggered sources sync, where
+    // it comes from the server instead of the now-dropped patch chain.
+    tester.simulatePassingOfSeconds(5);
+    expect(await tester.simulateStatCallback(syncEngine)).toMatchObject({
+      status: "done",
+    });
+    expect(
+      syncEngine.getSourceSnapshot(toModuleFilePath("/gallery.val.ts")).data,
+    ).toStrictEqual({ [ref]: metadata });
+  });
+
   test("basic reset", async () => {
     const { s, c, config } = initVal();
     const tester = new SyncEngineTester(
@@ -742,10 +923,14 @@ class SyncEngineTester {
       if (!patch) {
         continue;
       }
+      // File ops carry binary content rather than document mutations, so
+      // applyPatch rejects them outright. The real server filters them out the
+      // same way before applying (see applySourceFilePatches in ValOps).
+      const sourceFileOps = patch.filter((op) => op.op !== "file");
       const patchRes = applyPatch(
         deepClone(this.fakeSources[moduleFilePath]),
         this.ops,
-        patch,
+        sourceFileOps,
       );
       if (!modules[moduleFilePath]) {
         modules[moduleFilePath] = {};
@@ -804,6 +989,62 @@ class SyncEngineTester {
     };
   }
 
+  postSave(
+    req: InferReq<Api["/save"]["POST"]["req"]>,
+  ): z.infer<Api["/save"]["POST"]["res"]> {
+    // Model fs-mode /save: apply the requested patches to the backing sources
+    // and then delete every patch (fs mode deletes all of them), so a
+    // subsequent /patches read returns empty.
+    const requestedPatchIds = new Set(req.body.patchIds);
+    const savedSources = { ...this.fakeSources };
+    const sourceFilePatchErrors: Record<ModuleFilePath, { message: string }[]> =
+      {};
+    for (const patchData of this.fakePatches) {
+      if (!patchData.patch || !requestedPatchIds.has(patchData.patchId)) {
+        continue;
+      }
+      // File ops carry binary content rather than document mutations: the real
+      // /save writes those to disk separately and filters them out before
+      // applying the rest (applyPatch errors on them). A file-only patch
+      // therefore leaves the source untouched instead of failing to save.
+      const sourceFileOps = patchData.patch.filter((op) => op.op !== "file");
+      const patchRes = applyPatch(
+        deepClone(savedSources[patchData.path]),
+        this.ops,
+        sourceFileOps,
+      );
+      if (patchRes.kind === "ok") {
+        savedSources[patchData.path] = patchRes.value;
+      } else {
+        if (!sourceFilePatchErrors[patchData.path]) {
+          sourceFilePatchErrors[patchData.path] = [];
+        }
+        sourceFilePatchErrors[patchData.path].push({
+          message: patchRes.error.message,
+        });
+      }
+    }
+    if (Object.keys(sourceFilePatchErrors).length > 0) {
+      // The real /save bails out before writing sources or deleting patches.
+      return {
+        status: 400,
+        json: {
+          message: "Failed to save patches",
+          details: {
+            sourceFilePatchErrors,
+            binaryFilePatchErrors: {},
+          },
+        },
+      };
+    }
+    this.fakeSources = savedSources;
+    this.fakePatches = [];
+    return {
+      status: 200,
+      json: {},
+    };
+  }
+
   removeFakeResponse<R extends keyof FakeApi, M extends keyof FakeApi[R]>(
     route: R,
     method: M,
@@ -856,6 +1097,9 @@ class SyncEngineTester {
       if (route === "/schema" && method === "GET") {
         return this.getSchema(req);
       }
+      if (route === "/save" && method === "POST") {
+        return this.postSave(req);
+      }
       return {
         status: 404,
         json: {

packages/ui/spa/ValSyncEngine.ts

@@ -3110,7 +3110,33 @@ export class ValSyncEngine {
           status: "retry",
         } as const;
       } else {
+        // In fs mode /save applies exactly these patches to the .val files and
+        // then deletes them. Since serverSources still holds the *un-patched*
+        // base (we read /sources/~ with apply_patches=false), dropping the patch
+        // chain below would momentarily revert affected fields to their
+        // pre-patch value until the next stat-triggered /sources/~ refresh.
+        // Bake the current optimistic (patched) value into serverSources first
+        // so the base swaps out from under the optimistic view atomically and
+        // the displayed value never changes. The later /sources/~ overwrites
+        // serverSources with the authoritative value (self-healing if it
+        // differs). Only safe in fs mode: in http mode the committed patches
+        // persist server-side and are re-applied by syncPatches, so the base
+        // must stay un-patched (baking would double-apply).
+        const affectedModules: ModuleFilePath[] = [];
         if (this.mode === "fs") {
+          for (const moduleFilePath of this.patchIdsByModuleFilePath.keys()) {
+            const patched = this.getPatchedSource(moduleFilePath);
+            if (patched !== undefined && this.serverSources) {
+              this.serverSources[moduleFilePath] = patched;
+              if (this.patchedSourcesCache !== null) {
+                this.patchedSourcesCache = {
+                  ...this.patchedSourcesCache,
+                  [moduleFilePath]: undefined,
+                };
+              }
+              affectedModules.push(moduleFilePath);
+            }
+          }
           // In fs mode we delete all patch ids, so we start fresh
           this.globalServerSidePatchIds = [];
           console.debug("Deleting all patch ids");
@@ -3129,6 +3155,17 @@ export class ValSyncEngine {
         this.invalidatePendingClientSidePatchIds();
         this.invalidateSyncedServerSidePatchIds();
         this.invalidateSavedServerSidePatchIds();
+        // We emptied globalServerSidePatchIds above (fs), so its snapshot must be
+        // invalidated too — otherwise the Save button re-reads a stale non-empty
+        // value when the finally clears publishDisabled, briefly flicking back to
+        // enabled before the next stat/sync corrects it.
+        this.invalidateGlobalServerSidePatchIds();
+        // Notify subscribers so they re-read the freshly baked-in base. The
+        // value is unchanged from the optimistic view, so there's no flicker;
+        // only the snapshot's `optimistic` flag flips to false (now published).
+        for (const moduleFilePath of affectedModules) {
+          this.invalidateSource(moduleFilePath);
+        }
         return {
           status: "done",
         } as const;