From 0a165d062a64974a5f98c646b2df7beaba99ced6 Mon Sep 17 00:00:00 2001 From: Fredrik Ekholdt Date: Sat, 27 Jun 2026 10:01:06 +0200 Subject: [PATCH 1/2] Fix client validation flickers --- .changeset/fix-save-content-flicker.md | 5 ++ examples/next/app/blogs/[blog]/page.val.ts | 2 +- examples/next/val.config.ts | 2 +- packages/ui/spa/ValSyncEngine.test.ts | 88 ++++++++++++++++++++++ packages/ui/spa/ValSyncEngine.ts | 32 ++++++++ 5 files changed, 127 insertions(+), 2 deletions(-) create mode 100644 .changeset/fix-save-content-flicker.md diff --git a/.changeset/fix-save-content-flicker.md b/.changeset/fix-save-content-flicker.md new file mode 100644 index 000000000..b9c6a35d6 --- /dev/null +++ b/.changeset/fix-save-content-flicker.md @@ -0,0 +1,5 @@ +--- +"@valbuild/ui": patch +--- + +Fix content flicker on save where a field 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. diff --git a/examples/next/app/blogs/[blog]/page.val.ts b/examples/next/app/blogs/[blog]/page.val.ts index a833f4d7e..36c783ba7 100644 --- a/examples/next/app/blogs/[blog]/page.val.ts +++ b/examples/next/app/blogs/[blog]/page.val.ts @@ -200,7 +200,7 @@ export default c.define( }, }, "/blogs/blog-13": { - title: "Blog 1", + title: "Hallo hvordan", content: [ { tag: "p", diff --git a/examples/next/val.config.ts b/examples/next/val.config.ts index ed2762cdb..cfde56f86 100644 --- a/examples/next/val.config.ts +++ b/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: { diff --git a/packages/ui/spa/ValSyncEngine.test.ts b/packages/ui/spa/ValSyncEngine.test.ts index 3c0e775d8..e9039a372 100644 --- a/packages/ui/spa/ValSyncEngine.test.ts +++ b/packages/ui/spa/ValSyncEngine.test.ts @@ -68,6 +68,65 @@ 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("basic reset", async () => { const { s, c, config } = initVal(); const tester = new SyncEngineTester( @@ -804,6 +863,32 @@ class SyncEngineTester { }; } + postSave( + // eslint-disable-next-line @typescript-eslint/no-unused-vars + _req: any, + ): any { + // Model fs-mode /save: apply every pending patch to the backing sources + // and then delete the patches, so a subsequent /patches read returns empty. + for (const patchData of this.fakePatches) { + if (!patchData.patch) { + continue; + } + const patchRes = applyPatch( + deepClone(this.fakeSources[patchData.path]), + this.ops, + patchData.patch, + ); + if (patchRes.kind === "ok") { + this.fakeSources[patchData.path] = patchRes.value; + } + } + this.fakePatches = []; + return { + status: 200, + json: {}, + }; + } + removeFakeResponse( route: R, method: M, @@ -856,6 +941,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: { diff --git a/packages/ui/spa/ValSyncEngine.ts b/packages/ui/spa/ValSyncEngine.ts index 0147fc163..5b415bdd6 100644 --- a/packages/ui/spa/ValSyncEngine.ts +++ b/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,12 @@ export class ValSyncEngine { this.invalidatePendingClientSidePatchIds(); this.invalidateSyncedServerSidePatchIds(); this.invalidateSavedServerSidePatchIds(); + // 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; From 4a78351591169e7ec7b3277b883ea8d2058dcaf0 Mon Sep 17 00:00:00 2001 From: Fredrik Ekholdt Date: Tue, 28 Jul 2026 09:39:57 +0200 Subject: [PATCH 2/2] Fix Save button flicker and address review feedback - publish() now invalidates the globalServerSidePatchIds snapshot when it empties it in fs mode, so the Save button transitions enabled -> disabled cleanly instead of briefly flicking back to enabled until the next stat/sync. - The fake /save and /sources/~ in SyncEngineTester now filter `file` ops before applyPatch (which rejects them with "Cannot apply a file patch here"), mirroring applySourceFilePatches in ValOps. Previously a patch containing file ops silently failed to persist in the fake fs-mode save. postSave also honors req.body.patchIds and reports apply failures as a 400 (like the real endpoint, which bails before writing sources or deleting patches) instead of swallowing them. - Revert the unrelated blog title change in the Next.js example. Co-Authored-By: Claude Opus 5 --- .changeset/fix-save-content-flicker.md | 5 +- examples/next/app/blogs/[blog]/page.val.ts | 2 +- packages/ui/spa/ValSyncEngine.test.ts | 176 +++++++++++++++++++-- packages/ui/spa/ValSyncEngine.ts | 5 + 4 files changed, 176 insertions(+), 12 deletions(-) diff --git a/.changeset/fix-save-content-flicker.md b/.changeset/fix-save-content-flicker.md index b9c6a35d6..3b946018d 100644 --- a/.changeset/fix-save-content-flicker.md +++ b/.changeset/fix-save-content-flicker.md @@ -2,4 +2,7 @@ "@valbuild/ui": patch --- -Fix content flicker on save where a field 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. +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. diff --git a/examples/next/app/blogs/[blog]/page.val.ts b/examples/next/app/blogs/[blog]/page.val.ts index 36c783ba7..a833f4d7e 100644 --- a/examples/next/app/blogs/[blog]/page.val.ts +++ b/examples/next/app/blogs/[blog]/page.val.ts @@ -200,7 +200,7 @@ export default c.define( }, }, "/blogs/blog-13": { - title: "Hallo hvordan", + title: "Blog 1", content: [ { tag: "p", diff --git a/packages/ui/spa/ValSyncEngine.test.ts b/packages/ui/spa/ValSyncEngine.test.ts index e9039a372..aba022f04 100644 --- a/packages/ui/spa/ValSyncEngine.test.ts +++ b/packages/ui/spa/ValSyncEngine.test.ts @@ -127,6 +127,128 @@ describe("ValSyncEngine", () => { ).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( @@ -801,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] = {}; @@ -864,24 +990,54 @@ class SyncEngineTester { } postSave( - // eslint-disable-next-line @typescript-eslint/no-unused-vars - _req: any, - ): any { - // Model fs-mode /save: apply every pending patch to the backing sources - // and then delete the patches, so a subsequent /patches read returns empty. + req: InferReq, + ): z.infer { + // 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 = + {}; for (const patchData of this.fakePatches) { - if (!patchData.patch) { + 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(this.fakeSources[patchData.path]), + deepClone(savedSources[patchData.path]), this.ops, - patchData.patch, + sourceFileOps, ); if (patchRes.kind === "ok") { - this.fakeSources[patchData.path] = patchRes.value; + 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, diff --git a/packages/ui/spa/ValSyncEngine.ts b/packages/ui/spa/ValSyncEngine.ts index 5b415bdd6..24c8ab2a9 100644 --- a/packages/ui/spa/ValSyncEngine.ts +++ b/packages/ui/spa/ValSyncEngine.ts @@ -3155,6 +3155,11 @@ 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).