diff --git a/.changeset/fix-save-content-flicker.md b/.changeset/fix-save-content-flicker.md new file mode 100644 index 000000000..3b946018d --- /dev/null +++ b/.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. 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..aba022f04 100644 --- a/packages/ui/spa/ValSyncEngine.test.ts +++ b/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, + ): 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 || !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( 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: { diff --git a/packages/ui/spa/ValSyncEngine.ts b/packages/ui/spa/ValSyncEngine.ts index 0147fc163..24c8ab2a9 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,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;