From 131780fe9627ee7fb17cb871ef6a144a6ef02735 Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Tue, 4 Aug 2026 11:01:16 -0700 Subject: [PATCH 01/19] fix(engine): verify explicit browserGpuMode=hardware instead of trusting it Chrome's hardware GL args are advisory: with no usable GPU it silently falls back to software WebGL and the capture runs at CPU speed. Run the existing WebGL probe for explicit hardware mode too and warn loudly with the platform's remediation. The requested mode is still honoured. Fixes #2967 --- .../src/services/browserManager.test.ts | 29 +++++++- .../engine/src/services/browserManager.ts | 74 +++++++++++++++---- 2 files changed, 87 insertions(+), 16 deletions(-) diff --git a/packages/engine/src/services/browserManager.test.ts b/packages/engine/src/services/browserManager.test.ts index db6e7e468..2421d42c7 100644 --- a/packages/engine/src/services/browserManager.test.ts +++ b/packages/engine/src/services/browserManager.test.ts @@ -241,11 +241,35 @@ describe("resolveBrowserGpuMode", () => { expect(mode).toBe("software"); }); - it("passes 'hardware' through unchanged without probing", async () => { + it("passes 'hardware' through unchanged", async () => { + setMockWebGlProbe({ hasWebGL: true, vendor: "NVIDIA", renderer: "NVIDIA GeForce RTX 3070" }); const mode = await resolveBrowserGpuMode("hardware"); expect(mode).toBe("hardware"); }); + it("warns when explicit 'hardware' probes to software, but still honours it", async () => { + // heygen-com/hyperframes#2967: `--browser-gpu` inside a container with no + // GPU passthrough rendered 19186 frames on CPU with no diagnostic. + setMockWebGlProbe({ + hasWebGL: true, + vendor: "Google Inc. (Google)", + renderer: "ANGLE (Google, Vulkan 1.3.0 (SwiftShader Device))", + }); + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const mode = await resolveBrowserGpuMode("hardware", { platform: "linux" }); + expect(mode).toBe("hardware"); + const warning = warn.mock.calls.map((call) => String(call[0])).join("\n"); + expect(warning).toContain("browserGpuMode=hardware was requested"); + expect(warning).toContain("--gpus all"); + }); + + it("stays quiet when explicit 'hardware' probes to hardware", async () => { + setMockWebGlProbe({ hasWebGL: true, vendor: "NVIDIA", renderer: "NVIDIA GeForce RTX 3070" }); + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + expect(await resolveBrowserGpuMode("hardware")).toBe("hardware"); + expect(warn).not.toHaveBeenCalled(); + }); + it("falls back to 'software' when the probe browser cannot launch", async () => { // No chromePath, env unset, and (in the test env) no system Chrome to find // → puppeteer.launch will throw → caller catches → software fallback. @@ -272,7 +296,8 @@ describe("resolveBrowserGpuMode", () => { expect(second).toBe("software"); // Reset and re-probe to confirm the test-only reset works. _resetAutoBrowserGpuModeCacheForTests(); - const third = await resolveBrowserGpuMode("hardware"); + setMockWebGlProbe({ hasWebGL: true, vendor: "NVIDIA", renderer: "NVIDIA GeForce RTX 3070" }); + const third = await resolveBrowserGpuMode("auto"); expect(third).toBe("hardware"); }); diff --git a/packages/engine/src/services/browserManager.ts b/packages/engine/src/services/browserManager.ts index 762626eb3..9346f1802 100644 --- a/packages/engine/src/services/browserManager.ts +++ b/packages/engine/src/services/browserManager.ts @@ -507,14 +507,25 @@ async function probeAutoBrowserGpuMode(options: { /** * Resolve `browserGpuMode` to a concrete `"software" | "hardware"` answer. * - * For `"software"` / `"hardware"` this is a pure pass-through. For `"auto"` - * it launches a tiny Chrome with the platform's hardware GPU args, runs a - * one-shot WebGL availability probe, and falls back to `"software"` if - * hardware-mode WebGL is unavailable. The Promise is cached for the process - * lifetime, so concurrent callers (parallel workers) share the same probe. + * For `"software"` this is a pure pass-through. For `"auto"` it launches a + * tiny Chrome with the platform's hardware GPU args, runs a one-shot WebGL + * availability probe, and falls back to `"software"` if hardware-mode WebGL + * is unavailable. The Promise is cached for the process lifetime, so + * concurrent callers (parallel workers) share the same probe. * - * Any failure (Chrome launch error, navigation timeout, missing canvas API, - * etc.) is treated as a `"software"` fallback. The render path with + * `"hardware"` (an explicit `--browser-gpu` / `PRODUCER_BROWSER_GPU_MODE= + * hardware`) is honoured verbatim — the operator asked for it — but runs the + * SAME probe to VERIFY it, because Chrome's hardware GL args are advisory: + * with no usable GPU in the sandbox (no `/dev/dri`, no NVIDIA container + * runtime, missing EGL/driver libraries) Chrome silently falls back to + * software WebGL and the render just runs at CPU speed. Without this check + * the only trace is a buried `Automatic fallback to software WebGL` browser + * warning — heygen-com/hyperframes#2967 rendered 19186 frames on CPU while + * `--browser-gpu` was set and nothing said so. The probe result never + * changes the returned mode; it only makes the fallback loud. + * + * Any probe failure (Chrome launch error, navigation timeout, missing canvas + * API, etc.) is treated as a `"software"` result. The render path with * SwiftShader always works, so a misclassification toward software is the * safe failure mode; misclassifying toward hardware would error on the real * render. @@ -527,21 +538,56 @@ export function resolveBrowserGpuMode( platform?: NodeJS.Platform; } = {}, ): Promise<"software" | "hardware"> { - if (mode !== "auto") return Promise.resolve(mode); - if (_autoBrowserGpuModeCache) return _autoBrowserGpuModeCache; + if (mode === "software") return Promise.resolve(mode); - _autoBrowserGpuModeCache = probeAutoBrowserGpuMode(options); - return _autoBrowserGpuModeCache; + _autoBrowserGpuModeCache ??= probeAutoBrowserGpuMode(options); + if (mode === "auto") return _autoBrowserGpuModeCache; + + return _autoBrowserGpuModeCache.then((probed) => { + if (probed === "software") { + console.warn(buildUnverifiedHardwareGpuWarning(options.platform ?? process.platform)); + } + return "hardware"; + }); } /** - * Single observability surface for the auto-detect outcome. Logged exactly + * Warning text for "you asked for hardware GPU, the probe found none". + * + * Names the observable symptom (the render still completes, just on CPU) and + * the platform's actual remediation, so the operator doesn't have to infer it + * from Chrome's `Automatic fallback to software WebGL` warning. Exported for + * tests. + */ +export function buildUnverifiedHardwareGpuWarning(platform: NodeJS.Platform | string): string { + const remediation = + platform === "linux" + ? "Inside Docker, the container needs GPU passthrough: `--gpus all` with the NVIDIA " + + "Container Toolkit installed, or `--device /dev/dri` for Mesa/AMD/Intel. The image " + + "also needs the matching userspace driver + libEGL. Verify with " + + "`hyperframes render --browser-gpu` and watch for this warning disappearing." + : "Check that the host exposes a GPU to this process and that the graphics drivers are " + + "installed."; + return ( + "[hyperframes] browserGpuMode=hardware was requested, but the WebGL probe found no " + + "hardware GPU — Chrome will silently fall back to software WebGL and the capture will " + + "run at CPU speed. Honouring the explicit request anyway.\n" + + ` ${remediation}\n` + + " Pass --no-browser-gpu to select deterministic SwiftShader instead of waiting on a " + + "hardware path that is not there." + ); +} + +/** + * Single observability surface for the GPU probe outcome. Logged exactly * once per process (the probe runs once); without this line, a regression * to "always software even with a GPU present" would be invisible in - * production. Goes to stderr to stay out of stdout pipelines. + * production. Goes to stderr to stay out of stdout pipelines. Says "probe" + * rather than "auto" because explicit `browserGpuMode=hardware` runs the + * same probe to verify itself. */ function logResolvedBrowserGpuMode(resolved: "hardware" | "software", reason: string): void { - console.error(`[hyperframes] browserGpuMode auto → ${resolved} (${reason})`); + console.error(`[hyperframes] browserGpuMode probe → ${resolved} (${reason})`); } function createBrowserLaunchFingerprint( From 6703ea7e04701b490c5fb563db4f532fbc8aec89 Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Tue, 4 Aug 2026 11:04:35 -0700 Subject: [PATCH 02/19] fix(engine): warn once per process about unverified hardware GPU --- packages/engine/src/services/browserManager.test.ts | 5 +++++ packages/engine/src/services/browserManager.ts | 10 +++++++++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/packages/engine/src/services/browserManager.test.ts b/packages/engine/src/services/browserManager.test.ts index 2421d42c7..c1b0f3d1c 100644 --- a/packages/engine/src/services/browserManager.test.ts +++ b/packages/engine/src/services/browserManager.test.ts @@ -261,6 +261,11 @@ describe("resolveBrowserGpuMode", () => { const warning = warn.mock.calls.map((call) => String(call[0])).join("\n"); expect(warning).toContain("browserGpuMode=hardware was requested"); expect(warning).toContain("--gpus all"); + // Once per process, not once per worker — the render path resolves the + // mode for the probe browser plus every parallel worker. + await resolveBrowserGpuMode("hardware", { platform: "linux" }); + await resolveBrowserGpuMode("hardware", { platform: "linux" }); + expect(warn).toHaveBeenCalledTimes(1); }); it("stays quiet when explicit 'hardware' probes to hardware", async () => { diff --git a/packages/engine/src/services/browserManager.ts b/packages/engine/src/services/browserManager.ts index 9346f1802..c4b3c966b 100644 --- a/packages/engine/src/services/browserManager.ts +++ b/packages/engine/src/services/browserManager.ts @@ -438,6 +438,7 @@ let _autoBrowserGpuModeCache: Promise<"software" | "hardware"> | undefined; /** Test-only: reset the cached probe result. */ export function _resetAutoBrowserGpuModeCacheForTests(): void { _autoBrowserGpuModeCache = undefined; + _unverifiedHardwareGpuWarned = false; } async function getPuppeteerOrNull(): Promise { @@ -544,13 +545,20 @@ export function resolveBrowserGpuMode( if (mode === "auto") return _autoBrowserGpuModeCache; return _autoBrowserGpuModeCache.then((probed) => { - if (probed === "software") { + // Warn once per process, not once per caller: `createCaptureSession` + // resolves the mode for the probe browser AND every parallel worker, so + // an un-deduplicated warning prints N+1 times and buries itself. + if (probed === "software" && !_unverifiedHardwareGpuWarned) { + _unverifiedHardwareGpuWarned = true; console.warn(buildUnverifiedHardwareGpuWarning(options.platform ?? process.platform)); } return "hardware"; }); } +/** One-shot latch for the explicit-hardware-probed-to-software warning. */ +let _unverifiedHardwareGpuWarned = false; + /** * Warning text for "you asked for hardware GPU, the probe found none". * From f69c4a0e3aa8feb1110b52d538f8ca127481c95d Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Tue, 4 Aug 2026 13:14:23 -0700 Subject: [PATCH 03/19] fix(engine): distinguish probe failure from a genuinely absent GPU A probe that could not run is no evidence about the GPU, so pointing the operator at GPU passthrough hid broken Chrome installs behind a phantom problem. Carry a cause off the probe and emit the matching remediation. Also un-exports buildUnverifiedHardwareGpuWarning (Fallow: engine test files are not audit entry points, so a test-only import would not have counted as a consumer) and covers the non-linux branch via the spy. --- .../src/services/browserManager.test.ts | 75 ++++++++++++++----- .../engine/src/services/browserManager.ts | 75 ++++++++++++++----- 2 files changed, 114 insertions(+), 36 deletions(-) diff --git a/packages/engine/src/services/browserManager.test.ts b/packages/engine/src/services/browserManager.test.ts index c1b0f3d1c..dfa068e5d 100644 --- a/packages/engine/src/services/browserManager.test.ts +++ b/packages/engine/src/services/browserManager.test.ts @@ -275,6 +275,34 @@ describe("resolveBrowserGpuMode", () => { expect(warn).not.toHaveBeenCalled(); }); + it("gives non-linux hosts the generic remediation, not the Docker one", async () => { + setMockWebGlProbe({ + hasWebGL: true, + vendor: "Google Inc. (Google)", + renderer: "ANGLE (Google, Vulkan 1.3.0 (SwiftShader Device))", + }); + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + expect(await resolveBrowserGpuMode("hardware", { platform: "darwin" })).toBe("hardware"); + const warning = String(warn.mock.calls[0]?.[0]); + expect(warning).toContain("host exposes a GPU"); + expect(warning).not.toContain("--gpus all"); + }); + + it("does not blame the GPU when the probe itself failed to launch", async () => { + // A probe that could not run is NO evidence about the GPU. Sending this + // operator to `--gpus all` would hide a broken Chrome install behind a + // phantom passthrough problem. + _setPuppeteerForTests({ + launch: vi.fn().mockRejectedValue(new Error("spawn ENOENT /bad/chrome")), + } as unknown as PuppeteerNode); + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + expect(await resolveBrowserGpuMode("hardware", { platform: "linux" })).toBe("hardware"); + const warning = String(warn.mock.calls[0]?.[0]); + expect(warning).toContain("GPU probe could not run"); + expect(warning).toContain("hyperframes doctor"); + expect(warning).not.toContain("--gpus all"); + }); + it("falls back to 'software' when the probe browser cannot launch", async () => { // No chromePath, env unset, and (in the test env) no system Chrome to find // → puppeteer.launch will throw → caller catches → software fallback. @@ -306,27 +334,40 @@ describe("resolveBrowserGpuMode", () => { expect(third).toBe("hardware"); }); - it("deduplicates concurrent auto-mode probes by caching the in-flight Promise", async () => { + it("deduplicates concurrent probes so only one Chrome launches", async () => { // Parallel coordinator fires N workers via Promise.all — without Promise- // level caching, a `--workers 4` render against a no-GPU host would launch - // 4 simultaneous probe Chromes. Verify all concurrent callers get the - // exact same Promise reference (proving the probe runs once, not N times). - const p1 = resolveBrowserGpuMode("auto", { - chromePath: "/definitely/not/a/real/chrome/binary", - browserTimeout: 2000, + // 4 simultaneous probe Chromes. Assert the launch count directly rather + // than Promise identity: `"auto"` and `"hardware"` now each adapt the + // shared cached Promise via `.then`, so identity is no longer the + // invariant — "the probe browser starts exactly once" is. + const { launch } = setMockWebGlProbe({ + hasWebGL: true, + vendor: "Google Inc. (Google)", + renderer: "ANGLE (Google, Vulkan 1.3.0 (SwiftShader Device))", }); - const p2 = resolveBrowserGpuMode("auto", { - chromePath: "/definitely/not/a/real/chrome/binary", - browserTimeout: 2000, - }); - const p3 = resolveBrowserGpuMode("auto", { - chromePath: "/definitely/not/a/real/chrome/binary", - browserTimeout: 2000, - }); - expect(p1).toBe(p2); - expect(p2).toBe(p3); - const results = await Promise.all([p1, p2, p3]); + const results = await Promise.all([ + resolveBrowserGpuMode("auto", { browserTimeout: 2000 }), + resolveBrowserGpuMode("auto", { browserTimeout: 2000 }), + resolveBrowserGpuMode("auto", { browserTimeout: 2000 }), + ]); expect(results).toEqual(["software", "software", "software"]); + expect(launch).toHaveBeenCalledTimes(1); + }); + + it("shares the one probe across mixed 'auto' and 'hardware' callers", async () => { + const { launch } = setMockWebGlProbe({ + hasWebGL: true, + vendor: "NVIDIA", + renderer: "NVIDIA GeForce RTX 3070", + }); + const results = await Promise.all([ + resolveBrowserGpuMode("auto"), + resolveBrowserGpuMode("hardware"), + resolveBrowserGpuMode("auto"), + ]); + expect(results).toEqual(["hardware", "hardware", "hardware"]); + expect(launch).toHaveBeenCalledTimes(1); }); it.each([ diff --git a/packages/engine/src/services/browserManager.ts b/packages/engine/src/services/browserManager.ts index c4b3c966b..899c3498b 100644 --- a/packages/engine/src/services/browserManager.ts +++ b/packages/engine/src/services/browserManager.ts @@ -422,7 +422,25 @@ export const _probeBeginFrameSupportForTests = probeBeginFrameSupport; export const _closeBrowserAfterFailedProbeForTests = closeBrowserAfterFailedProbe; /** - * Cached *in-flight or resolved* probe Promise for `resolveBrowserGpuMode("auto", ...)`. + * Outcome of the one-shot WebGL probe. + * + * `cause` distinguishes the two ways a probe lands on `"software"`, because + * they need OPPOSITE remediation: + * - `"no-gpu"` — the probe ran and Chrome reported a software renderer + * (SwiftShader / llvmpipe). Remediation: GPU passthrough. + * - `"probe-error"` — the probe itself failed (Chrome couldn't launch, bad + * executable path, sandbox denied). We have NO evidence + * about the GPU either way; telling the operator to fix GPU + * passthrough would send them chasing the wrong problem. + */ +interface GpuProbeOutcome { + mode: "software" | "hardware"; + cause?: "no-gpu" | "probe-error"; +} + +/** + * Cached *in-flight or resolved* probe Promise, shared by BOTH the `"auto"` + * and explicit `"hardware"` entry points of `resolveBrowserGpuMode`. * * Caching the Promise (rather than the resolved value) deduplicates concurrent * callers — the parallel coordinator runs N workers via `Promise.all`, so a @@ -430,10 +448,8 @@ export const _closeBrowserAfterFailedProbeForTests = closeBrowserAfterFailedProb * simultaneous probe Chromes. The first call assigns the Promise and every * other concurrent caller awaits the same one, paying the ~240 ms probe cost * exactly once per process lifetime. - * - * Exported for tests; production callers go through `resolveBrowserGpuMode`. */ -let _autoBrowserGpuModeCache: Promise<"software" | "hardware"> | undefined; +let _autoBrowserGpuModeCache: Promise | undefined; /** Test-only: reset the cached probe result. */ export function _resetAutoBrowserGpuModeCacheForTests(): void { @@ -479,7 +495,7 @@ async function probeAutoBrowserGpuMode(options: { chromePath?: string; browserTimeout?: number; platform?: NodeJS.Platform; -}): Promise<"software" | "hardware"> { +}): Promise { const platform = options.platform ?? process.platform; const browserTimeout = options.browserTimeout ?? DEFAULT_CONFIG.browserTimeout; const executablePath = options.chromePath ?? resolveHeadlessShellPath({}); @@ -487,7 +503,7 @@ async function probeAutoBrowserGpuMode(options: { if (ppt === null) { logResolvedBrowserGpuMode("software", "puppeteer unavailable"); - return "software"; + return { mode: "software", cause: "probe-error" }; } try { @@ -498,10 +514,10 @@ async function probeAutoBrowserGpuMode(options: { }); const resolved = resolveWebGlProbeMode(info); logResolvedBrowserGpuMode(resolved, describeWebGlProbe(info)); - return resolved; + return resolved === "hardware" ? { mode: "hardware" } : { mode: "software", cause: "no-gpu" }; } catch (err) { logResolvedBrowserGpuMode("software", formatProbeFailure(err)); - return "software"; + return { mode: "software", cause: "probe-error" }; } } @@ -542,32 +558,53 @@ export function resolveBrowserGpuMode( if (mode === "software") return Promise.resolve(mode); _autoBrowserGpuModeCache ??= probeAutoBrowserGpuMode(options); - if (mode === "auto") return _autoBrowserGpuModeCache; + if (mode === "auto") return _autoBrowserGpuModeCache.then((probed) => probed.mode); return _autoBrowserGpuModeCache.then((probed) => { - // Warn once per process, not once per caller: `createCaptureSession` + // Warn once per cache lifetime, not once per caller: `createCaptureSession` // resolves the mode for the probe browser AND every parallel worker, so // an un-deduplicated warning prints N+1 times and buries itself. - if (probed === "software" && !_unverifiedHardwareGpuWarned) { + if (probed.mode === "software" && !_unverifiedHardwareGpuWarned) { _unverifiedHardwareGpuWarned = true; - console.warn(buildUnverifiedHardwareGpuWarning(options.platform ?? process.platform)); + console.warn( + buildUnverifiedHardwareGpuWarning(options.platform ?? process.platform, probed.cause), + ); } return "hardware"; }); } -/** One-shot latch for the explicit-hardware-probed-to-software warning. */ +/** + * Latch for the explicit-hardware-probed-to-software warning: fires once per + * cache lifetime (re-armed by `_resetAutoBrowserGpuModeCacheForTests`). + */ let _unverifiedHardwareGpuWarned = false; /** - * Warning text for "you asked for hardware GPU, the probe found none". + * Warning text for "you asked for hardware GPU and we could not confirm it". * - * Names the observable symptom (the render still completes, just on CPU) and - * the platform's actual remediation, so the operator doesn't have to infer it - * from Chrome's `Automatic fallback to software WebGL` warning. Exported for - * tests. + * Splits on `cause` because the two failure shapes need opposite remediation. + * A probe that RAN and saw SwiftShader is a GPU-passthrough problem. A probe + * that could not run tells us nothing about the GPU — pointing that operator + * at `--gpus all` would send them chasing a phantom while their Chrome + * install is the actual fault. */ -export function buildUnverifiedHardwareGpuWarning(platform: NodeJS.Platform | string): string { +function buildUnverifiedHardwareGpuWarning( + platform: NodeJS.Platform | string, + cause: GpuProbeOutcome["cause"], +): string { + if (cause === "probe-error") { + return ( + "[hyperframes] browserGpuMode=hardware was requested, but the GPU probe could not run, " + + "so hardware acceleration is UNVERIFIED — if Chrome falls back to software WebGL the " + + "capture will run at CPU speed. Honouring the explicit request anyway.\n" + + " This is a probe failure, not evidence of a missing GPU: see the " + + "`browserGpuMode probe → software (probe failed ...)` line above for the underlying " + + "error, which usually means Chrome could not launch (bad HYPERFRAMES_BROWSER_PATH, " + + "missing shared libraries, or a denied sandbox) rather than a GPU problem.\n" + + " Run `hyperframes doctor` to check the Chrome install." + ); + } const remediation = platform === "linux" ? "Inside Docker, the container needs GPU passthrough: `--gpus all` with the NVIDIA " + From cde5bae4c5a6afa167faf827ab1073e66964cd35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20=C3=81ngel?= Date: Tue, 4 Aug 2026 13:23:25 -0700 Subject: [PATCH 04/19] test(studio): pin text-field Backspace routing (#2988) --- .../hooks/useAppHotkeys.textEditing.test.tsx | 215 ++++++++++++++++++ 1 file changed, 215 insertions(+) create mode 100644 packages/studio/src/hooks/useAppHotkeys.textEditing.test.tsx diff --git a/packages/studio/src/hooks/useAppHotkeys.textEditing.test.tsx b/packages/studio/src/hooks/useAppHotkeys.textEditing.test.tsx new file mode 100644 index 000000000..d1d2b6a53 --- /dev/null +++ b/packages/studio/src/hooks/useAppHotkeys.textEditing.test.tsx @@ -0,0 +1,215 @@ +// @vitest-environment happy-dom + +import React, { act, useRef, useState } from "react"; +import { createRoot, type Root } from "react-dom/client"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { TextAreaField } from "../components/editor/propertyPanelSections"; +import type { DomEditSelection } from "../components/editor/domEditing"; +import type { LeftSidebarHandle } from "../components/sidebar/LeftSidebar"; +import { usePlayerStore } from "../player/store/playerStore"; +import { useAppHotkeys } from "./useAppHotkeys"; + +(globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + +const timelineDelete = vi.fn(async () => undefined); +const domDelete = vi.fn(async () => undefined); +const keyframeDelete = vi.fn(); +const textCommits = vi.fn(); +let root: Root | null = null; + +function parentWithTextChild(): DomEditSelection { + const element = document.createElement("section"); + element.id = "selected-card"; + const child = document.createElement("span"); + child.textContent = "Kicker"; + element.append(child); + return { + element, + id: "selected-card", + selector: "#selected-card", + selectorIndex: 0, + label: "Selected card", + tagName: "section", + sourceFile: "index.html", + compositionPath: "index.html", + isCompositionHost: false, + isInsideLockedComposition: false, + boundingBox: { x: 0, y: 0, width: 320, height: 180 }, + textContent: "Kicker", + dataAttributes: {}, + inlineStyles: {}, + computedStyles: {}, + textFields: [ + { + key: "child:0:span", + label: "Content", + value: "Kicker", + tagName: "span", + attributes: [], + inlineStyles: {}, + computedStyles: {}, + source: "child", + sourceChildIndex: 0, + }, + ], + capabilities: { + canSelect: true, + canEditStyles: true, + canCrop: false, + canMove: true, + canResize: true, + canApplyManualOffset: true, + canApplyManualSize: true, + canApplyManualRotation: true, + }, + }; +} + +function Harness() { + const [value, setValue] = useState("Kicker"); + const [selectionRefreshed, setSelectionRefreshed] = useState(false); + const selectionRef = useRef(parentWithTextChild()); + const clearSelectionRef = useRef<() => void>(() => undefined); + const saveTimestampRef = useRef(0); + const leftSidebarRef = useRef(null); + + useAppHotkeys({ + handleTimelineElementDelete: timelineDelete, + handleTimelineElementSplit: vi.fn(async () => undefined), + handleDomEditElementDelete: domDelete, + domEditSelectionRef: selectionRef, + clearDomSelectionRef: clearSelectionRef, + editHistory: { + undo: vi.fn(async () => ({ ok: false })), + redo: vi.fn(async () => ({ ok: false })), + state: { undo: [], redo: [] }, + }, + readOptionalProjectFile: vi.fn(async () => ""), + readProjectFile: vi.fn(async () => ""), + writeProjectFile: vi.fn(async () => undefined), + domEditSaveTimestampRef: saveTimestampRef, + showToast: vi.fn(), + syncHistoryPreviewAfterApply: vi.fn(async () => undefined), + waitForPendingDomEditSaves: vi.fn(async () => undefined), + leftSidebarRef, + handleCopy: vi.fn(() => false), + handlePaste: vi.fn(async () => undefined), + handleCut: vi.fn(async () => false), + onResetKeyframes: vi.fn(() => false), + onDeleteSelectedKeyframes: keyframeDelete, + }); + + return ( + <> + { + textCommits(next); + setValue(next); + // Text persistence rebuilds the selected parent's text-field model + // from the preview. Model that refresh: it must not transfer keyboard + // ownership away from the still-mounted Content editor. + setSelectionRefreshed(true); + }} + /> + + + ); +} + +function setTextareaValue(textarea: HTMLTextAreaElement, value: string) { + const setter = Object.getOwnPropertyDescriptor(HTMLTextAreaElement.prototype, "value")?.set; + if (!setter) throw new Error("expected native textarea value setter"); + setter.call(textarea, value); + textarea.dispatchEvent(new Event("input", { bubbles: true })); + textarea.setSelectionRange(value.length, value.length); +} + +function pressBackspace(target: HTMLElement): KeyboardEvent { + const event = new KeyboardEvent("keydown", { + key: "Backspace", + bubbles: true, + cancelable: true, + }); + target.dispatchEvent(event); + return event; +} + +beforeEach(() => { + vi.useFakeTimers(); + timelineDelete.mockClear(); + domDelete.mockClear(); + keyframeDelete.mockClear(); + textCommits.mockClear(); + usePlayerStore.getState().reset(); + usePlayerStore.getState().setElements([ + { + id: "selected-card", + tag: "section", + start: 0, + duration: 10, + track: 0, + }, + ]); + usePlayerStore.getState().setSelectedElementId("selected-card"); +}); + +afterEach(() => { + if (root) act(() => root?.unmount()); + root = null; + document.body.innerHTML = ""; + usePlayerStore.getState().reset(); + vi.useRealTimers(); +}); + +describe("useAppHotkeys text-field ownership", () => { + it("keeps two Backspaces in a child text editor across its scheduled save", () => { + const host = document.createElement("div"); + document.body.append(host); + root = createRoot(host); + act(() => root?.render()); + + const textarea = host.querySelector("textarea"); + const canvas = host.querySelector('[data-testid="canvas"]'); + if (!textarea || !canvas) throw new Error("expected text editor and canvas target"); + + textarea.focus(); + textarea.setSelectionRange(textarea.value.length, textarea.value.length); + + const first = pressBackspace(textarea); + act(() => setTextareaValue(textarea, "Kicke")); + act(() => vi.advanceTimersByTime(120)); + + expect(first.defaultPrevented).toBe(false); + expect(textCommits).toHaveBeenLastCalledWith("Kicke"); + expect(canvas.dataset.selectionRefreshed).toBe("true"); + expect(document.activeElement).toBe(textarea); + expect(textarea.selectionStart).toBe(5); + expect(textarea.selectionEnd).toBe(5); + + const second = pressBackspace(textarea); + act(() => setTextareaValue(textarea, "Kick")); + act(() => vi.advanceTimersByTime(120)); + + expect(second.defaultPrevented).toBe(false); + expect(textarea.value).toBe("Kick"); + expect(textCommits.mock.calls).toEqual([["Kicke"], ["Kick"]]); + expect(document.activeElement).toBe(textarea); + expect(textarea.selectionStart).toBe(4); + expect(textarea.selectionEnd).toBe(4); + expect(timelineDelete).not.toHaveBeenCalled(); + expect(domDelete).not.toHaveBeenCalled(); + expect(keyframeDelete).not.toHaveBeenCalled(); + + canvas.focus(); + const canvasDelete = pressBackspace(canvas); + + expect(canvasDelete.defaultPrevented).toBe(true); + expect(timelineDelete).toHaveBeenCalledTimes(1); + expect(domDelete).not.toHaveBeenCalled(); + expect(keyframeDelete).not.toHaveBeenCalled(); + }); +}); From 552419c52d16fa5a5e8d15a9263bc5f41e596360 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20=C3=81ngel?= Date: Tue, 4 Aug 2026 13:26:43 -0700 Subject: [PATCH 05/19] fix(studio): make inspector commits transactional (#2987) * fix(studio): make inspector commits transactional * fix(studio): make inspector persistence atomic * fix(studio): preserve synchronous gesture semantics --- .../editor/propertyPanel3dTransform.tsx | 2 +- .../editor/propertyPanelCommitField.tsx | 39 ++++- .../propertyPanelFlatPrimitives.test.tsx | 69 ++++++++ .../editor/propertyPanelFlatPrimitives.tsx | 4 +- .../editor/propertyPanelPrimitives.tsx | 4 +- .../propertyPanelTransformCommit.test.ts | 57 +++++++ .../editor/propertyPanelTransformCommit.ts | 28 ++-- .../components/editor/propertyPanelTypes.ts | 12 +- .../useInspectorGestureTransaction.test.tsx | 65 ++++++++ .../editor/useInspectorGestureTransaction.ts | 76 +++++++-- .../hooks/useAnimatedPropertyCommit.test.tsx | 149 +++++++++++++++++- .../src/hooks/useAnimatedPropertyCommit.ts | 138 ++++++++++------ .../src/hooks/useDomGeometryCommits.test.tsx | 61 +++++++ .../studio/src/hooks/useDomGeometryCommits.ts | 20 +++ 14 files changed, 634 insertions(+), 90 deletions(-) create mode 100644 packages/studio/src/components/editor/propertyPanelTransformCommit.test.ts create mode 100644 packages/studio/src/hooks/useDomGeometryCommits.test.tsx diff --git a/packages/studio/src/components/editor/propertyPanel3dTransform.tsx b/packages/studio/src/components/editor/propertyPanel3dTransform.tsx index e38cc58ee..af23372e6 100644 --- a/packages/studio/src/components/editor/propertyPanel3dTransform.tsx +++ b/packages/studio/src/components/editor/propertyPanel3dTransform.tsx @@ -253,7 +253,7 @@ function Transform3dField({ onCommit={(next) => { const v = parse(next); if (v != null && onCommitAnimatedProperty) { - void onCommitAnimatedProperty(ctx.element, prop, v); + return onCommitAnimatedProperty(ctx.element, prop, v); } }} /> diff --git a/packages/studio/src/components/editor/propertyPanelCommitField.tsx b/packages/studio/src/components/editor/propertyPanelCommitField.tsx index 1ee79c0ea..509517be6 100644 --- a/packages/studio/src/components/editor/propertyPanelCommitField.tsx +++ b/packages/studio/src/components/editor/propertyPanelCommitField.tsx @@ -21,7 +21,7 @@ export function CommitField({ liveCommit?: boolean; align?: "left" | "right"; onPreview?: (nextValue: string) => void; - onCommit: (nextValue: string) => void; + onCommit: (nextValue: string) => void | Promise; }) { const [draft, setDraft] = useState(value); const valueRef = useRef(value); @@ -29,6 +29,19 @@ export function CommitField({ const inputRef = useRef(null); const focusedRef = useRef(false); const dirtyRef = useRef(false); + const commitGenerationRef = useRef(0); + const pendingCommitRef = useRef<{ + baseline: string; + optimistic: string; + } | null>(null); + const lastValueRef = useRef(value); + if (!Object.is(lastValueRef.current, value)) { + lastValueRef.current = value; + if (!Object.is(pendingCommitRef.current?.optimistic, value)) { + commitGenerationRef.current += 1; + pendingCommitRef.current = null; + } + } valueRef.current = value; draftRef.current = draft; @@ -67,14 +80,34 @@ export function CommitField({ }, 250); }; const cancelGesture = () => { + commitGenerationRef.current += 1; clearGestureSettleTimer(); gestureActiveRef.current = false; gestureTransaction.cancel(); }; const commitDraft = (nextValue: string) => { + const generation = ++commitGenerationRef.current; setDraft(nextValue); onPreview?.(nextValue); - if (nextValue !== valueRef.current) onCommit(nextValue); + if (nextValue !== valueRef.current) { + const baseline = valueRef.current; + pendingCommitRef.current = { baseline, optimistic: nextValue }; + const rollback = () => { + if (generation !== commitGenerationRef.current) return; + pendingCommitRef.current = null; + // The source write is authoritative. A rejected mutation must not leave + // the field showing an optimistic value that will disappear on seek. + setDraft(baseline); + onPreview?.(baseline); + }; + try { + void Promise.resolve(onCommit(nextValue)).then(() => { + if (generation === commitGenerationRef.current) pendingCommitRef.current = null; + }, rollback); + } catch { + rollback(); + } + } }; const cancelGestureFromKeyEvent = (event: React.KeyboardEvent) => { if (!gestureActiveRef.current) return false; @@ -89,6 +122,7 @@ export function CommitField({ const nextDraft = adjustNumericToken(draftRef.current, direction, event); if (!nextDraft) return; event.preventDefault(); + commitGenerationRef.current += 1; dirtyRef.current = false; gestureActiveRef.current = true; gestureTransaction.preview(nextDraft); @@ -148,6 +182,7 @@ export function CommitField({ focusedRef.current = true; }} onChange={(event) => { + commitGenerationRef.current += 1; settleGesture(); dirtyRef.current = true; setDraft(event.target.value); diff --git a/packages/studio/src/components/editor/propertyPanelFlatPrimitives.test.tsx b/packages/studio/src/components/editor/propertyPanelFlatPrimitives.test.tsx index 9c6de843a..831c80467 100644 --- a/packages/studio/src/components/editor/propertyPanelFlatPrimitives.test.tsx +++ b/packages/studio/src/components/editor/propertyPanelFlatPrimitives.test.tsx @@ -90,6 +90,75 @@ describe("FlatRow", () => { act(() => root.unmount()); }); + it("restores its durable value when an async commit rejects", async () => { + let rejectCommit: ((error: Error) => void) | null = null; + const onCommit = vi.fn( + () => + new Promise((_resolve, reject) => { + rejectCommit = reject; + }), + ); + const row = (value: string) => ( + + ); + const { host, root } = renderInto(row("22px")); + const input = host.querySelector("input"); + if (!input) throw new Error("expected an input"); + act(() => { + const nativeInputValueSetter = Object.getOwnPropertyDescriptor( + window.HTMLInputElement.prototype, + "value", + )?.set; + nativeInputValueSetter?.call(input, "99px"); + input.dispatchEvent(new Event("input", { bubbles: true })); + input.dispatchEvent(new Event("focusout", { bubbles: true })); + }); + // The parent can echo the preview before persistence settles. That is not a + // durable acknowledgement and must not invalidate the pending rollback. + act(() => root.render(row("99px"))); + await act(async () => { + rejectCommit?.(new Error("save failed")); + await Promise.resolve(); + }); + + expect(onCommit).toHaveBeenCalledWith("99px"); + expect(input.value).toBe("22px"); + act(() => root.unmount()); + }); + + it("does not let an older rejected commit overwrite a newer draft", async () => { + let rejectCommit: ((error: Error) => void) | null = null; + const onCommit = vi.fn( + () => + new Promise((_resolve, reject) => { + rejectCommit = reject; + }), + ); + const { host, root } = renderInto( + , + ); + const input = host.querySelector("input"); + if (!input) throw new Error("expected an input"); + const nativeInputValueSetter = Object.getOwnPropertyDescriptor( + window.HTMLInputElement.prototype, + "value", + )?.set; + act(() => { + nativeInputValueSetter?.call(input, "99px"); + input.dispatchEvent(new Event("input", { bubbles: true })); + input.dispatchEvent(new Event("focusout", { bubbles: true })); + nativeInputValueSetter?.call(input, "100px"); + input.dispatchEvent(new Event("input", { bubbles: true })); + }); + await act(async () => { + rejectCommit?.(new Error("old save failed")); + await Promise.resolve(); + }); + + expect(input.value).toBe("100px"); + act(() => root.unmount()); + }); + it("persists a rapid numeric arrow-key burst as one commit", () => { vi.useFakeTimers(); const onCommit = vi.fn(); diff --git a/packages/studio/src/components/editor/propertyPanelFlatPrimitives.tsx b/packages/studio/src/components/editor/propertyPanelFlatPrimitives.tsx index 29c167e57..ce99e13f8 100644 --- a/packages/studio/src/components/editor/propertyPanelFlatPrimitives.tsx +++ b/packages/studio/src/components/editor/propertyPanelFlatPrimitives.tsx @@ -35,7 +35,7 @@ export function FlatRow({ /** Renders a trailing 10px caret-down, for select-backed rows. */ dropdown?: boolean; onPreview?: (nextValue: string) => void; - onCommit: (nextValue: string) => void; + onCommit: (nextValue: string) => void | Promise; onReset?: () => void; }) { const track = useTrackDesignInput(); @@ -59,7 +59,7 @@ export function FlatRow({ onPreview={onPreview} onCommit={(nextValue) => { track("metric", label); - onCommit(nextValue); + return onCommit(nextValue); }} /> diff --git a/packages/studio/src/components/editor/propertyPanelPrimitives.tsx b/packages/studio/src/components/editor/propertyPanelPrimitives.tsx index 96b2ed315..383323b2e 100644 --- a/packages/studio/src/components/editor/propertyPanelPrimitives.tsx +++ b/packages/studio/src/components/editor/propertyPanelPrimitives.tsx @@ -29,14 +29,14 @@ export function MetricField({ scrub?: boolean; suffix?: string; tooltip?: string; - onCommit: (nextValue: string) => void; + onCommit: (nextValue: string) => void | Promise; }) { const track = useTrackDesignInput(); const scrubRef = useRef<{ startX: number; startValue: number; pointerId: number } | null>(null); const commit = useCallback( (nextValue: string) => { if (nextValue !== value) track("metric", label); - onCommit(nextValue); + return onCommit(nextValue); }, [label, onCommit, track, value], ); diff --git a/packages/studio/src/components/editor/propertyPanelTransformCommit.test.ts b/packages/studio/src/components/editor/propertyPanelTransformCommit.test.ts new file mode 100644 index 000000000..e07c1eab6 --- /dev/null +++ b/packages/studio/src/components/editor/propertyPanelTransformCommit.test.ts @@ -0,0 +1,57 @@ +// @vitest-environment happy-dom + +import { describe, expect, it, vi } from "vitest"; +import type { DomEditSelection } from "./domEditingTypes"; +import { GsapEditBlockedError } from "../../hooks/gsapEditOutcome"; +import { createTransformCommitHandlers } from "./propertyPanelTransformCommit"; + +describe("createTransformCommitHandlers", () => { + it.each([ + [ + "position", + (handlers: ReturnType) => + handlers.commitManualOffset("x", "20px"), + ], + [ + "size", + (handlers: ReturnType) => + handlers.commitManualSize("width", "200px"), + ], + [ + "rotation", + (handlers: ReturnType) => + handlers.commitManualRotation("45"), + ], + ])("propagates blocked %s edits so the field can roll back", async (_name, commit) => { + const blocked = new GsapEditBlockedError("unroll-required"); + const onCommitAnimatedProperty = vi.fn().mockRejectedValue(blocked); + const onSetManualOffset = vi.fn(); + const onSetManualSize = vi.fn(); + const onSetManualRotation = vi.fn(); + const element = { + id: "box", + selector: "#box", + element: document.createElement("div"), + boundingBox: { width: 100, height: 100 }, + } as unknown as DomEditSelection; + const handlers = createTransformCommitHandlers({ + element, + styles: {}, + hasGsapAnimation: true, + gsapAnimId: "#box-to-position", + gsapKeyframes: null, + currentPct: 0, + onCommitAnimatedProperty, + onAddKeyframe: undefined, + onSetManualOffset, + onSetManualSize, + onSetManualRotation, + showToast: vi.fn(), + }); + + await expect(commit(handlers)).rejects.toBe(blocked); + expect(onSetManualOffset).not.toHaveBeenCalled(); + expect(onSetManualSize).not.toHaveBeenCalled(); + expect(onSetManualRotation).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/studio/src/components/editor/propertyPanelTransformCommit.ts b/packages/studio/src/components/editor/propertyPanelTransformCommit.ts index ec4a948ab..976921a1f 100644 --- a/packages/studio/src/components/editor/propertyPanelTransformCommit.ts +++ b/packages/studio/src/components/editor/propertyPanelTransformCommit.ts @@ -41,13 +41,13 @@ export function createTransformCommitHandlers({ // Route a transform value into the GSAP animation (or a new keyframe) when the // element is animated. Returns true when handled, so callers fall through to // the manual-transform path only for non-animated elements. - const commitAnimatedTransformValue = ( + const commitAnimatedTransformValue = async ( property: string, value: number, noCallbacksMessage: string, - ): boolean => { + ): Promise => { if (onCommitAnimatedProperty && hasGsapAnimation) { - void onCommitAnimatedProperty(element, property, value); + await onCommitAnimatedProperty(element, property, value); return true; } if (gsapKeyframes && gsapAnimId && onAddKeyframe) { @@ -62,11 +62,11 @@ export function createTransformCommitHandlers({ return false; }; - const commitManualOffset = (axis: "x" | "y", nextValue: string) => { + const commitManualOffset = async (axis: "x" | "y", nextValue: string) => { const parsed = parsePxMetricValue(nextValue); if (parsed == null) return; if ( - commitAnimatedTransformValue( + await commitAnimatedTransformValue( axis, parsed, "Cannot edit position — animation callbacks not available", @@ -74,20 +74,20 @@ export function createTransformCommitHandlers({ ) return; const current = readStudioPathOffset(element.element); - void Promise.resolve( + await Promise.resolve( onSetManualOffset(element, { x: axis === "x" ? parsed : current.x, y: axis === "y" ? parsed : current.y, }), - ).catch(() => undefined); + ); }; // fallow-ignore-next-line complexity - const commitManualSize = (axis: "width" | "height", nextValue: string) => { + const commitManualSize = async (axis: "width" | "height", nextValue: string) => { const parsed = parsePxMetricValue(nextValue); if (parsed == null || parsed <= 0) return; if (onCommitAnimatedProperty && hasGsapAnimation) { - void onCommitAnimatedProperty(element, axis, parsed); + await onCommitAnimatedProperty(element, axis, parsed); return; } if (hasGsapAnimation) { @@ -103,26 +103,26 @@ export function createTransformCommitHandlers({ current.height > 0 ? current.height : (parsePxMetricValue(styles.height ?? "") ?? element.boundingBox.height); - void Promise.resolve( + await Promise.resolve( onSetManualSize(element, { width: axis === "width" ? parsed : width, height: axis === "height" ? parsed : height, }), - ).catch(() => undefined); + ); }; - const commitManualRotation = (nextValue: string) => { + const commitManualRotation = async (nextValue: string) => { const parsed = Number.parseFloat(nextValue); if (!Number.isFinite(parsed)) return; if ( - commitAnimatedTransformValue( + await commitAnimatedTransformValue( "rotation", parsed, "Cannot edit rotation — animation callbacks not available", ) ) return; - void Promise.resolve(onSetManualRotation(element, { angle: parsed })).catch(() => undefined); + await Promise.resolve(onSetManualRotation(element, { angle: parsed })); }; return { commitManualOffset, commitManualSize, commitManualRotation }; diff --git a/packages/studio/src/components/editor/propertyPanelTypes.ts b/packages/studio/src/components/editor/propertyPanelTypes.ts index 935ceb3f6..84a61f80d 100644 --- a/packages/studio/src/components/editor/propertyPanelTypes.ts +++ b/packages/studio/src/components/editor/propertyPanelTypes.ts @@ -71,9 +71,15 @@ export interface PropertyPanelProps { onProgress?: (progress: BackgroundRemovalProgress) => void; }, ) => Promise; - onSetManualOffset: (element: DomEditSelection, next: { x: number; y: number }) => void; - onSetManualSize: (element: DomEditSelection, next: { width: number; height: number }) => void; - onSetManualRotation: (element: DomEditSelection, next: { angle: number }) => void; + onSetManualOffset: ( + element: DomEditSelection, + next: { x: number; y: number }, + ) => void | Promise; + onSetManualSize: ( + element: DomEditSelection, + next: { width: number; height: number }, + ) => void | Promise; + onSetManualRotation: (element: DomEditSelection, next: { angle: number }) => void | Promise; onSetText: (value: string, fieldKey?: string) => void; onSetTextFieldStyle: (fieldKey: string, property: string, value: string) => void; onPreviewTextFieldStyle?: (fieldKey: string, property: string, value: string) => void; diff --git a/packages/studio/src/components/editor/useInspectorGestureTransaction.test.tsx b/packages/studio/src/components/editor/useInspectorGestureTransaction.test.tsx index 7bce8dbbc..1922929bf 100644 --- a/packages/studio/src/components/editor/useInspectorGestureTransaction.test.tsx +++ b/packages/studio/src/components/editor/useInspectorGestureTransaction.test.tsx @@ -8,6 +8,30 @@ import { useInspectorGestureTransaction } from "./useInspectorGestureTransaction (globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true; describe("useInspectorGestureTransaction", () => { + it("restores the durable baseline when an inspector commit rejects", async () => { + const host = document.createElement("div"); + const root = createRoot(host); + const onPreview = vi.fn(); + const onCommit = vi.fn().mockRejectedValue(new Error("save failed")); + let gesture: ReturnType> | null = null; + + function Probe() { + gesture = useInspectorGestureTransaction({ sourceValue: 10, onPreview, onCommit }); + return null; + } + + act(() => root.render()); + act(() => { + gesture?.preview(25); + gesture?.settle(); + }); + expect(onPreview.mock.calls.map(([value]) => value)).toEqual([25]); + await act(async () => Promise.resolve()); + + expect(onPreview.mock.calls.map(([value]) => value)).toEqual([25, 10]); + act(() => root.unmount()); + }); + it("keeps a new gesture active when the prior async commit is acknowledged", () => { const host = document.createElement("div"); const root = createRoot(host); @@ -34,4 +58,45 @@ describe("useInspectorGestureTransaction", () => { act(() => root.unmount()); }); + + it("does not let an older rejected commit roll back a newer successful gesture", async () => { + const host = document.createElement("div"); + const root = createRoot(host); + const onPreview = vi.fn(); + let rejectFirst: ((error: Error) => void) | null = null; + const onCommit = vi + .fn() + .mockImplementationOnce((value: number) => { + onPreview(value); + return new Promise((_resolve, reject) => { + rejectFirst = reject; + }); + }) + .mockImplementationOnce((value: number) => { + onPreview(value); + return Promise.resolve(); + }); + let gesture: ReturnType> | null = null; + + function Probe() { + gesture = useInspectorGestureTransaction({ sourceValue: 10, onPreview, onCommit }); + return null; + } + + act(() => root.render()); + act(() => { + gesture?.preview(20); + gesture?.settle(); + gesture?.preview(30); + gesture?.settle(); + }); + await act(async () => { + rejectFirst?.(new Error("old save failed")); + await Promise.resolve(); + }); + + expect(onCommit.mock.calls.map(([value]) => value)).toEqual([20, 30]); + expect(onPreview).toHaveBeenLastCalledWith(30); + act(() => root.unmount()); + }); }); diff --git a/packages/studio/src/components/editor/useInspectorGestureTransaction.ts b/packages/studio/src/components/editor/useInspectorGestureTransaction.ts index df61e5586..51bc8ce6f 100644 --- a/packages/studio/src/components/editor/useInspectorGestureTransaction.ts +++ b/packages/studio/src/components/editor/useInspectorGestureTransaction.ts @@ -1,5 +1,9 @@ import { useCallback, useEffect, useRef, useState } from "react"; +function isPromiseCommit(result: void | Promise): result is Promise { + return Boolean(result && typeof result.then === "function"); +} + /** One owner for continuous inspector edits: preview freely, persist once. */ export function useInspectorGestureTransaction({ sourceValue, @@ -8,44 +12,96 @@ export function useInspectorGestureTransaction({ }: { sourceValue: T; onPreview: (value: T) => void; - onCommit: (value: T) => void; + onCommit: (value: T) => void | Promise; }) { const sourceRef = useRef(sourceValue); const activeRef = useRef<{ before: T; latest: T } | null>(null); const previewRef = useRef(onPreview); const commitRef = useRef(onCommit); - if (!activeRef.current) sourceRef.current = sourceValue; + const generationRef = useRef(0); + const pendingRef = useRef<{ before: T; latest: T } | null>(null); + const awaitingSourceAckRef = useRef<{ generation: number; value: T } | null>(null); + const lastSourceValueRef = useRef(sourceValue); + if (!Object.is(lastSourceValueRef.current, sourceValue)) { + lastSourceValueRef.current = sourceValue; + const matchesSourceAck = Object.is(awaitingSourceAckRef.current?.value, sourceValue); + const matchesOptimisticValue = + (activeRef.current && Object.is(activeRef.current.latest, sourceValue)) || + (pendingRef.current && Object.is(pendingRef.current.latest, sourceValue)) || + matchesSourceAck; + if (matchesSourceAck) awaitingSourceAckRef.current = null; + if (!matchesOptimisticValue) { + generationRef.current += 1; + activeRef.current = null; + pendingRef.current = null; + awaitingSourceAckRef.current = null; + sourceRef.current = sourceValue; + } else { + sourceRef.current = sourceValue; + } + } previewRef.current = onPreview; commitRef.current = onCommit; const begin = useCallback(() => { if (!activeRef.current) { + generationRef.current += 1; activeRef.current = { before: sourceRef.current, latest: sourceRef.current }; } }, []); const preview = useCallback((value: T) => { if (!activeRef.current) { + generationRef.current += 1; activeRef.current = { before: sourceRef.current, latest: sourceRef.current }; } activeRef.current.latest = value; previewRef.current(value); }, []); + const rollbackCommit = useCallback((active: { before: T; latest: T }, generation: number) => { + if (generation !== generationRef.current) return; + pendingRef.current = null; + if (awaitingSourceAckRef.current?.generation === generation) { + awaitingSourceAckRef.current = null; + } + sourceRef.current = active.before; + previewRef.current(active.before); + }, []); + const settle = useCallback(() => { const active = activeRef.current; activeRef.current = null; if (active && !Object.is(active.before, active.latest)) { + const generation = ++generationRef.current; sourceRef.current = active.latest; - // Restore the captured baseline before the persistent commit captures - // rollback state. The commit reapplies `latest` synchronously, so this - // is not visible but a failed save can now correctly restore `before`. - previewRef.current(active.before); - commitRef.current(active.latest); + pendingRef.current = active; + awaitingSourceAckRef.current = { generation, value: active.latest }; + try { + const result = commitRef.current(active.latest); + if (isPromiseCommit(result)) { + void result.then( + () => { + if (generation === generationRef.current) pendingRef.current = null; + }, + () => rollbackCommit(active, generation), + ); + } else if (generation === generationRef.current) { + pendingRef.current = null; + // Synchronous inspector consumers historically restore their preview + // after persisting (color pickers close, curves release the pointer). + // Async source mutations keep the optimistic preview until the write + // resolves so they do not flash back to the baseline while pending. + previewRef.current(active.before); + } + } catch { + rollbackCommit(active, generation); + } } - }, []); + }, [rollbackCommit]); const cancel = useCallback(() => { + generationRef.current += 1; const active = activeRef.current; activeRef.current = null; if (active && !Object.is(active.before, active.latest)) { @@ -66,7 +122,7 @@ export function useInspectorGestureDraft({ }: { sourceValue: T; onPreview: (value: T) => void; - onCommit: (value: T) => void; + onCommit: (value: T) => void | Promise; }) { const [draft, setDraft] = useState(sourceValue); const transaction = useInspectorGestureTransaction({ @@ -77,7 +133,7 @@ export function useInspectorGestureDraft({ }, onCommit: (next) => { setDraft(next); - onCommit(next); + return onCommit(next); }, }); diff --git a/packages/studio/src/hooks/useAnimatedPropertyCommit.test.tsx b/packages/studio/src/hooks/useAnimatedPropertyCommit.test.tsx index defec9f81..e664db490 100644 --- a/packages/studio/src/hooks/useAnimatedPropertyCommit.test.tsx +++ b/packages/studio/src/hooks/useAnimatedPropertyCommit.test.tsx @@ -43,17 +43,43 @@ function renderHookWith( animations: GsapAnimation[], onMutation: (mutation: Record, label: string) => unknown | Promise, onReady: (commit: Commit) => void, + bumpGsapCache = vi.fn(), + onBatch?: ( + calls: Array<{ mutation: Record; options: { label: string } }>, + label: string, + ) => unknown | Promise, ) { function Harness() { - const { commitAnimatedProperties } = useAnimatedPropertyCommit({ - selectedGsapAnimations: animations, - gsapCommitMutation: async (_sel, mutation, options) => { + const gsapCommitMutation = Object.assign( + async ( + _sel: DomEditSelection, + mutation: Record, + options: { label: string }, + ) => { await onMutation(mutation, options.label); }, + onBatch + ? { + batch: async ( + calls: Array<{ + selection: DomEditSelection; + mutation: Record; + options: { label: string }; + }>, + options: { label: string }, + ) => { + await onBatch(calls, options.label); + }, + } + : {}, + ); + const { commitAnimatedProperties } = useAnimatedPropertyCommit({ + selectedGsapAnimations: animations, + gsapCommitMutation, addGsapAnimation: vi.fn(), convertToKeyframes: vi.fn(), previewIframeRef: { current: null }, - bumpGsapCache: vi.fn(), + bumpGsapCache, }); onReady(commitAnimatedProperties); return null; @@ -87,8 +113,53 @@ describe("useAnimatedPropertyCommit — ownership and rejection propagation", () act(() => root.unmount()); }); + it("rejects runtime-computed property ownership before sending a mutation", async () => { + const runtimePosition = { + ...keyframedAnim, + hasUnresolvedKeyframes: true, + } as GsapAnimation; + const mutations: Array> = []; + let commit!: Commit; + const root = renderHookWith( + [runtimePosition], + (mutation) => mutations.push(mutation), + (ready) => (commit = ready), + ); + + await expect(commit(selection, { x: 50 })).rejects.toMatchObject({ + reason: "source-uneditable", + }); + expect(mutations).toHaveLength(0); + act(() => root.unmount()); + }); + + it("rejects every property before a mixed-group commit can partially persist", async () => { + const helperOpacity = { + id: "#box-to-visual", + targetSelector: "#box", + propertyGroup: "visual", + method: "to", + properties: { opacity: 0.5 }, + provenance: { kind: "helper", fn: "fade", callSite: 1 }, + } as unknown as GsapAnimation; + const mutations: Array> = []; + let commit!: Commit; + const root = renderHookWith( + [helperOpacity], + (mutation) => mutations.push(mutation), + (ready) => (commit = ready), + ); + + await expect(commit(selection, { x: 50, opacity: 0.8 })).rejects.toMatchObject({ + reason: "unroll-required", + }); + expect(mutations).toHaveLength(0); + act(() => root.unmount()); + }); + it("rethrows a persistence failure to the telemetry wrapper", async () => { const failure = new Error("save failed"); + const bumpGsapCache = vi.fn(); let commit!: Commit; const root = renderHookWith( [keyframedAnim], @@ -96,9 +167,11 @@ describe("useAnimatedPropertyCommit — ownership and rejection propagation", () throw failure; }, (ready) => (commit = ready), + bumpGsapCache, ); await expect(commit(selection, { x: 50 })).rejects.toBe(failure); + expect(bumpGsapCache).toHaveBeenCalledTimes(1); act(() => root.unmount()); }); }); @@ -107,7 +180,13 @@ function renderCommitHook( mutations: Array>, onReady: (commit: Commit) => void, ) { - return renderHookWith([keyframedAnim], (mutation) => mutations.push(mutation), onReady); + return renderHookWith( + [keyframedAnim], + (mutation) => { + mutations.push(mutation); + }, + onReady, + ); } // Regression (#1808): a "3D transform" / design-panel property edit on an @@ -160,7 +239,9 @@ describe("commitStaticSet group routing", () => { ) { return renderHookWith( [positionSet], - (mutation, label) => committed.push({ mutation, label }), + (mutation, label) => { + committed.push({ mutation, label }); + }, onReady, ); } @@ -208,7 +289,9 @@ describe("commitStaticSet group routing", () => { let commit!: Commit; renderHookWith( [positionSet, instantSizeHold], - (mutation, label) => committed.push({ mutation, label }), + (mutation, label) => { + committed.push({ mutation, label }); + }, (c) => (commit = c), ); @@ -226,4 +309,56 @@ describe("commitStaticSet group routing", () => { expect(committed.some(({ mutation }) => mutation.type === "add")).toBe(false); expect(committed[0]!.mutation.animationId).not.toBe(positionSet.id); }); + + it("persists multiple property groups in one atomic batch", async () => { + const committed: Array<{ mutation: Record; label: string }> = []; + const batches: Array<{ + calls: Array<{ mutation: Record; options: { label: string } }>; + label: string; + }> = []; + let commit!: Commit; + const root = renderHookWith( + [positionSet], + (mutation, label) => committed.push({ mutation, label }), + (ready) => (commit = ready), + vi.fn(), + (calls, label) => batches.push({ calls, label }), + ); + + await act(async () => { + await commit(selection, { x: 400, width: 500 }); + }); + + expect(committed).toHaveLength(0); + expect(batches).toHaveLength(1); + expect(batches[0]!.label).toBe("Set properties"); + expect(batches[0]!.calls.map(({ mutation }) => mutation)).toEqual([ + { + type: "update-properties", + animationId: positionSet.id, + properties: { x: 400 }, + }, + { + type: "add", + targetSelector: "#box", + method: "set", + position: 0, + properties: { width: 500 }, + global: true, + }, + ]); + act(() => root.unmount()); + }); + + it("fails before sending anything when an atomic multi-group batch is unavailable", async () => { + const committed: Array<{ mutation: Record; label: string }> = []; + let commit!: Commit; + const root = renderStaticHook(committed, (ready) => (commit = ready)); + + await expect(commit(selection, { x: 400, width: 500 })).rejects.toThrow( + "Atomic GSAP property batch is unavailable", + ); + expect(committed).toHaveLength(0); + act(() => root.unmount()); + }); }); diff --git a/packages/studio/src/hooks/useAnimatedPropertyCommit.ts b/packages/studio/src/hooks/useAnimatedPropertyCommit.ts index 5bc19ec4a..177d8c5c5 100644 --- a/packages/studio/src/hooks/useAnimatedPropertyCommit.ts +++ b/packages/studio/src/hooks/useAnimatedPropertyCommit.ts @@ -24,22 +24,16 @@ import { import { resolveTweenStart, resolveTweenDuration } from "../utils/globalTimeCompiler"; import { roundTo3 } from "../utils/rounding"; import { commitWholePropertyOffset } from "./gsapWholePropertyOffsetCommit"; -import { assertGsapEditPersisted, directEditOutcomeForProperties } from "./gsapEditOutcome"; +import { + assertGsapEditPersisted, + directEditOutcomeForProperties, + GsapEditBlockedError, +} from "./gsapEditOutcome"; +import type { CommitMutation, CommitMutationCall } from "./gsapScriptCommitTypes"; interface CommitAnimatedPropertyDeps { selectedGsapAnimations: GsapAnimation[]; - gsapCommitMutation: - | (( - selection: DomEditSelection, - mutation: Record, - options: { - label: string; - coalesceKey?: string; - softReload?: boolean; - skipReload?: boolean; - }, - ) => Promise) - | null; + gsapCommitMutation: CommitMutation | null; addGsapAnimation: ( selection: DomEditSelection, method: "to" | "from" | "set" | "fromTo", @@ -110,7 +104,7 @@ async function maybeAutoKeyframeSet( ); } -type Commit = NonNullable; +type Commit = CommitMutation; /** Undo-history label for a static-set commit, from the group it writes. */ const STATIC_SET_LABELS: Partial, string>> = { @@ -140,6 +134,17 @@ async function commitSetProps( animations: GsapAnimation[], commit: Commit, ): Promise { + const call = buildSetPropsCall(selection, setAnim, propEntries, selector); + await commit(call.selection, call.mutation, call.options); + await maybeAutoKeyframeSet(selection, setAnim, animations, commit); +} + +function buildSetPropsCall( + selection: DomEditSelection, + setAnim: GsapAnimation, + propEntries: [string, number | string][], + selector: string | null, +): CommitMutationCall { const properties = Object.fromEntries(propEntries); const numericProps: SetPatchProps = {}; for (const [k, v] of propEntries) { @@ -155,16 +160,15 @@ async function commitSetProps( }, } : undefined; - await commit( + return { selection, - { type: "update-properties", animationId: setAnim.id, properties }, - { + mutation: { type: "update-properties", animationId: setAnim.id, properties }, + options: { label: staticSetLabel(propEntries), softReload: true, ...(instantPatch ? { instantPatch } : {}), }, - ); - await maybeAutoKeyframeSet(selection, setAnim, animations, commit); + }; } /** @@ -180,7 +184,25 @@ async function commitStaticSet( animations: GsapAnimation[], commit: Commit, ): Promise { - if (!selector) return; + const calls = planStaticSetCalls(selection, propEntries, selector, animations); + const only = calls[0]; + if (!only) return; + if (calls.length === 1) { + await commit(only.selection, only.mutation, only.options); + return; + } + if (!commit.batch) { + throw new Error("Atomic GSAP property batch is unavailable"); + } + await commit.batch(calls, { + label: staticSetLabel(propEntries), + softReload: true, + }); +} + +function groupStaticSetEntries( + propEntries: [string, number | string][], +): Map { // One commit per PROPERTY GROUP, each into a static write that owns that group — // never a live tween, and never a foreign-group write (a width edit used to // merge into the element's position set, producing a mixed write the split @@ -194,9 +216,22 @@ async function commitStaticSet( batch.push(entry); byGroup.set(group, batch); } - const staticWrites = animations.filter( - (a) => isInstantHold(a) && tweenTargetsElement(a.targetSelector, selector, selection.element), - ); + return byGroup; +} + +function planStaticSetCalls( + selection: DomEditSelection, + propEntries: [string, number | string][], + selector: string | null, + animations: GsapAnimation[], +): CommitMutationCall[] { + const byGroup = groupStaticSetEntries(propEntries); + const staticWrites = selector + ? animations.filter( + (a) => + isInstantHold(a) && tweenTargetsElement(a.targetSelector, selector, selection.element), + ) + : []; // Resolve every group's target BEFORE committing anything, and coalesce // groups that land on the SAME write into one commit: the snapshot is captured // once, so if two groups resolved to one legacy mixed write, a first @@ -212,13 +247,12 @@ async function commitStaticSet( newSetBatches.push(batch); } } - for (const [targetWrite, batch] of byTargetWrite) { - await commitSetProps(selection, targetWrite, batch, selector, animations, commit); - } - // Fresh adds don't reshape existing sets, so their ids can't go stale. - for (const batch of newSetBatches) { - await addGlobalStaticSet(selection, batch, commit); - } + return [ + ...[...byTargetWrite].map(([targetWrite, batch]) => + buildSetPropsCall(selection, targetWrite, batch, selector), + ), + ...newSetBatches.map((batch) => buildGlobalStaticSetCall(selection, batch)), + ]; } /** @@ -244,11 +278,10 @@ function findGroupOwningStaticWrite( * the timeline (matches the manual-drag UX). The global-set instant patch applies * it straight to the element so the first edit shows with no soft-reload flash. */ -async function addGlobalStaticSet( +function buildGlobalStaticSetCall( selection: DomEditSelection, batch: [string, number | string][], - commit: Commit, -): Promise { +): CommitMutationCall { const numericProps: SetPatchProps = {}; for (const [k, v] of batch) { if (typeof v === "number") numericProps[k as keyof SetPatchProps] = v; @@ -257,10 +290,10 @@ async function addGlobalStaticSet( // selector is the bare class an id-less element yields, which would hold every // sibling. No one-element form means no write at all (see writeTargetSelector). const target = writeTargetSelector(selection); - if (!target) return; - await commit( + if (!target) throw new GsapEditBlockedError("no-selector"); + return { selection, - { + mutation: { type: "add", targetSelector: target, method: "set", @@ -268,7 +301,7 @@ async function addGlobalStaticSet( properties: Object.fromEntries(batch), global: true, }, - { + options: { label: staticSetLabel(batch), softReload: true, ...(Object.keys(numericProps).length > 0 @@ -280,7 +313,7 @@ async function addGlobalStaticSet( } : {}), }, - ); + }; } /** Convert-if-flat, then write ALL props into ONE keyframe at the playhead. */ @@ -418,6 +451,9 @@ export function useAnimatedPropertyCommit(deps: CommitAnimatedPropertyDeps) { selector, primaryProp, ); + if (!anim && !writeTargetSelector(selection)) { + throw new GsapEditBlockedError("no-selector"); + } // Whether the element is animated at all. A 3D edit only creates/edits // keyframes when it IS — a static element (no keyframes on any of its tweens) // gets a `tl.set`, never new keyframes (matches manual drag / resize / rotate). @@ -472,12 +508,15 @@ export function useAnimatedPropertyCommit(deps: CommitAnimatedPropertyDeps) { return; } - // Existing static hold on a NON-animated element — merge the props into the - // same write (maybeAutoKeyframeSet no-ops when nothing else is keyframed). - if (anim && isInstantHold(anim)) { - await commitSetProps( + // Static element (no keyframes anywhere) — persist as a `tl.set`, never + // keyframes (incl. the no-animation case, which creates a fresh set). + // Route the complete property set through the group-aware planner even + // when pickBestAnimation found one existing set: a mixed X+width edit + // must update the position set AND create a size set atomically rather + // than contaminating the first set with a foreign property group. + if (!elementHasKeyframes) { + await commitStaticSet( selection, - anim, propEntries, selector, selectedGsapAnimations, @@ -486,11 +525,12 @@ export function useAnimatedPropertyCommit(deps: CommitAnimatedPropertyDeps) { return; } - // Static element (no keyframes anywhere) — persist as a `tl.set`, never - // keyframes (incl. the no-animation case, which creates a fresh set). - if (!elementHasKeyframes) { - await commitStaticSet( + // Existing static hold on an otherwise animated element — merge the props + // into the same write, then auto-keyframe it against the sibling tween. + if (anim && isInstantHold(anim)) { + await commitSetProps( selection, + anim, propEntries, selector, selectedGsapAnimations, @@ -509,7 +549,7 @@ export function useAnimatedPropertyCommit(deps: CommitAnimatedPropertyDeps) { // one-element form the edit is dropped rather than written onto every // class sibling (see writeTargetSelector). const newTweenTarget = writeTargetSelector(selection); - if (selector && newTweenTarget) { + if (newTweenTarget) { const template = selectedGsapAnimations.find((a) => !!a.keyframes); const tStart = template ? (resolveTweenStart(template) ?? 0) : 0; const tDur = template ? resolveTweenDuration(template) || 1 : 1; @@ -539,7 +579,7 @@ export function useAnimatedPropertyCommit(deps: CommitAnimatedPropertyDeps) { ); return; } - bumpGsapCache(); + throw new GsapEditBlockedError("no-selector"); } catch (error) { bumpGsapCache(); throw error; diff --git a/packages/studio/src/hooks/useDomGeometryCommits.test.tsx b/packages/studio/src/hooks/useDomGeometryCommits.test.tsx new file mode 100644 index 000000000..ac394c0c5 --- /dev/null +++ b/packages/studio/src/hooks/useDomGeometryCommits.test.tsx @@ -0,0 +1,61 @@ +// @vitest-environment happy-dom + +import React, { act } from "react"; +import { createRoot } from "react-dom/client"; +import { describe, expect, it, vi } from "vitest"; +import type { DomEditSelection } from "../components/editor/domEditingTypes"; +import { + applyStudioBoxSize, + applyStudioPathOffset, + applyStudioRotation, + readStudioBoxSize, + readStudioPathOffset, + readStudioRotation, +} from "../components/editor/manualEdits"; +import { useDomGeometryCommits } from "./useDomGeometryCommits"; + +(globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + +describe("useDomGeometryCommits rollback", () => { + it("restores every optimistic geometry mutation when persistence rejects", async () => { + const element = document.createElement("div"); + element.id = "box"; + document.body.append(element); + applyStudioPathOffset(element, { x: 10, y: 20 }); + applyStudioBoxSize(element, { width: 100, height: 80 }); + applyStudioRotation(element, { angle: 15 }); + const selection = { + id: "box", + selector: "#box", + element, + } as unknown as DomEditSelection; + const failure = new Error("save failed"); + const commitPositionPatchToHtml = vi.fn().mockRejectedValue(failure); + let commits: ReturnType | null = null; + const host = document.createElement("div"); + const root = createRoot(host); + + function Probe() { + commits = useDomGeometryCommits({ + previewIframeRef: { current: null }, + showToast: vi.fn(), + commitPositionPatchToHtml, + }); + return null; + } + + act(() => root.render()); + await expect(commits!.handleDomPathOffsetCommit(selection, { x: 50, y: 60 })).rejects.toBe( + failure, + ); + await expect( + commits!.handleDomBoxSizeCommit(selection, { width: 200, height: 160 }, { x: 30, y: 40 }), + ).rejects.toBe(failure); + await expect(commits!.handleDomRotationCommit(selection, { angle: 45 })).rejects.toBe(failure); + + expect(readStudioPathOffset(element)).toEqual({ x: 10, y: 20 }); + expect(readStudioBoxSize(element)).toEqual({ width: 100, height: 80 }); + expect(readStudioRotation(element)).toEqual({ angle: 15 }); + act(() => root.unmount()); + }); +}); diff --git a/packages/studio/src/hooks/useDomGeometryCommits.ts b/packages/studio/src/hooks/useDomGeometryCommits.ts index 667f3caea..8cabc37ac 100644 --- a/packages/studio/src/hooks/useDomGeometryCommits.ts +++ b/packages/studio/src/hooks/useDomGeometryCommits.ts @@ -4,6 +4,12 @@ import { applyStudioPathOffset, applyStudioBoxSize, applyStudioRotation, + captureStudioPathOffset, + captureStudioBoxSize, + captureStudioRotation, + restoreStudioPathOffset, + restoreStudioBoxSize, + restoreStudioRotation, clearStudioPathOffset, clearStudioBoxSize, clearStudioRotation, @@ -51,10 +57,14 @@ export function useDomGeometryCommits({ showToast(error.message, "error"); return Promise.reject(error); } + const before = captureStudioPathOffset(selection.element); applyStudioPathOffset(selection.element, next); return commitPositionPatchToHtml(selection, buildPathOffsetPatches(selection.element), { label: "Move layer", coalesceKey: `path-offset:${getDomEditTargetKey(selection)}`, + }).catch((error) => { + restoreStudioPathOffset(selection.element, before); + throw error; }); }, [commitPositionPatchToHtml, previewIframeRef, showToast], @@ -71,6 +81,8 @@ export function useDomGeometryCommits({ showToast(error.message, "error"); return Promise.reject(error); } + const beforeSize = captureStudioBoxSize(selection.element); + const beforeOffset = offset ? captureStudioPathOffset(selection.element) : null; applyStudioBoxSize(selection.element, next); // Anchored-corner resize (NW/NE/SW) also moves the element to keep the // opposite corner fixed. Apply the offset and emit BOTH patch sets in a @@ -86,6 +98,10 @@ export function useDomGeometryCommits({ return commitPositionPatchToHtml(selection, patches, { label: "Resize layer box", coalesceKey: `box-size:${getDomEditTargetKey(selection)}`, + }).catch((error) => { + restoreStudioBoxSize(selection.element, beforeSize); + if (beforeOffset) restoreStudioPathOffset(selection.element, beforeOffset); + throw error; }); }, [commitPositionPatchToHtml, previewIframeRef, showToast], @@ -98,10 +114,14 @@ export function useDomGeometryCommits({ showToast(error.message, "error"); return Promise.reject(error); } + const before = captureStudioRotation(selection.element); applyStudioRotation(selection.element, next); return commitPositionPatchToHtml(selection, buildRotationPatches(selection.element), { label: "Rotate layer", coalesceKey: `rotation:${getDomEditTargetKey(selection)}`, + }).catch((error) => { + restoreStudioRotation(selection.element, before); + throw error; }); }, [commitPositionPatchToHtml, previewIframeRef, showToast], From 47131385445c0f8ee4203f8d6ff4f5600d8c8b2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20=C3=81ngel?= Date: Tue, 4 Aug 2026 13:58:19 -0700 Subject: [PATCH 06/19] fix(studio): drain pending edits before reload (#2989) * fix(studio): drain pending edits before reload * fix(studio): address drain review feedback (#2989) - prioritize conflicts and clear recovered DOM queue errors - cover delayed blur effects and missing drain branches - document stacked consumers and extend write-token retention * test(studio): satisfy drain audit gate (#2989) - share the editor-save hook harness across drain regressions - extract settled failure inspection from the drain loop --- .../studio/src/hooks/useEditorSave.test.tsx | 131 +++++++++++++++ packages/studio/src/hooks/useEditorSave.ts | 152 +++++++++++++----- .../studio/src/utils/domEditSaveQueue.test.ts | 38 ++++- packages/studio/src/utils/domEditSaveQueue.ts | 26 ++- .../src/utils/studioFileVersion.test.ts | 29 +++- .../studio/src/utils/studioFileVersion.ts | 30 ++++ .../src/utils/studioPendingEdits.test.ts | 109 ++++++++++++- .../studio/src/utils/studioPendingEdits.ts | 41 ++++- .../studio/src/utils/studioSaveDiagnostics.ts | 5 + 9 files changed, 513 insertions(+), 48 deletions(-) create mode 100644 packages/studio/src/hooks/useEditorSave.test.tsx diff --git a/packages/studio/src/hooks/useEditorSave.test.tsx b/packages/studio/src/hooks/useEditorSave.test.tsx new file mode 100644 index 000000000..ee5b7bf82 --- /dev/null +++ b/packages/studio/src/hooks/useEditorSave.test.tsx @@ -0,0 +1,131 @@ +// @vitest-environment happy-dom +import { act } from "react"; +import { createRoot } from "react-dom/client"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { useEditorSave, type EditorSaveHandle } from "./useEditorSave"; +import { StudioFileConflictError } from "../utils/studioSaveDiagnostics"; + +(globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + +type WriteProjectFile = (path: string, content: string, expectedContent?: string) => Promise; + +async function mountEditorSave(writeProjectFile: WriteProjectFile) { + const captured: { handle: EditorSaveHandle | null } = { handle: null }; + + function Probe() { + captured.handle = useEditorSave({ + editingPathRef: { current: "index.html" }, + projectIdRef: { current: "project-a" }, + readProjectFile: vi.fn(async () => "before"), + writeProjectFile, + recordEdit: vi.fn(async () => undefined), + domEditSaveTimestampRef: { current: 0 }, + setRefreshKey: vi.fn(), + showToast: vi.fn(), + }); + return null; + } + + const root = createRoot(document.createElement("div")); + await act(async () => root.render()); + if (!captured.handle) throw new Error("Editor save handle was not mounted"); + + return { + handle: captured.handle, + unmount: () => act(async () => root.unmount()), + }; +} + +describe("useEditorSave pending work", () => { + beforeEach(() => { + vi.stubGlobal( + "requestAnimationFrame", + vi.fn(() => 41), + ); + vi.stubGlobal("cancelAnimationFrame", vi.fn()); + }); + + afterEach(() => vi.unstubAllGlobals()); + + it("exposes and flushes the latest rAF-buffered source candidate", async () => { + const writeProjectFile = vi.fn(async () => undefined); + const mounted = await mountEditorSave(writeProjectFile); + act(() => mounted.handle.handleContentChange("studio candidate")); + + expect(mounted.handle.getPendingCandidate()).toEqual({ + projectId: "project-a", + path: "index.html", + content: "studio candidate", + }); + await expect(mounted.handle.flushPendingSave()).resolves.toEqual({ status: "clean" }); + expect(writeProjectFile).toHaveBeenCalledWith("index.html", "studio candidate", "before"); + + await mounted.unmount(); + }); + + it("joins an in-flight source save instead of writing the frozen candidate twice", async () => { + let frame: FrameRequestCallback | null = null; + vi.stubGlobal( + "requestAnimationFrame", + vi.fn((callback: FrameRequestCallback) => { + frame = callback; + return 42; + }), + ); + let finishWrite!: () => void; + const writeProjectFile = vi + .fn() + .mockImplementationOnce( + () => + new Promise((resolve) => { + finishWrite = resolve; + }), + ) + .mockResolvedValue(undefined); + const mounted = await mountEditorSave(writeProjectFile); + act(() => mounted.handle.handleContentChange("candidate")); + act(() => frame?.(0)); + await vi.waitFor(() => expect(writeProjectFile).toHaveBeenCalledOnce()); + + const drained = mounted.handle.flushPendingSave(); + expect(writeProjectFile).toHaveBeenCalledOnce(); + finishWrite(); + await expect(drained).resolves.toEqual({ status: "clean" }); + expect(writeProjectFile).toHaveBeenCalledOnce(); + await mounted.unmount(); + }); + + it("preserves conflict details when flushing a buffered source candidate", async () => { + const conflict = new StudioFileConflictError({ + filePath: "index.html", + currentVersion: "external-v2", + currentContent: "external", + attemptedContent: "studio candidate", + }); + const mounted = await mountEditorSave(async () => { + throw conflict; + }); + act(() => mounted.handle.handleContentChange("studio candidate")); + + await expect(mounted.handle.flushPendingSave()).resolves.toEqual({ + status: "conflict", + error: conflict, + }); + + await mounted.unmount(); + }); + + it("discards an rAF-buffered candidate without persisting it", async () => { + const writeProjectFile = vi.fn(async () => undefined); + const mounted = await mountEditorSave(writeProjectFile); + act(() => mounted.handle.handleContentChange("discard me")); + act(() => mounted.handle.discardPendingSave()); + + expect(mounted.handle.getPendingCandidate()).toBeNull(); + await expect(mounted.handle.flushPendingSave()).resolves.toEqual({ status: "clean" }); + expect(writeProjectFile).not.toHaveBeenCalled(); + expect(cancelAnimationFrame).toHaveBeenCalledWith(41); + + await mounted.unmount(); + }); +}); diff --git a/packages/studio/src/hooks/useEditorSave.ts b/packages/studio/src/hooks/useEditorSave.ts index f9e04a977..f00fd9246 100644 --- a/packages/studio/src/hooks/useEditorSave.ts +++ b/packages/studio/src/hooks/useEditorSave.ts @@ -2,6 +2,10 @@ import { useCallback, useRef } from "react"; import { saveProjectFilesWithHistory } from "../utils/studioFileHistory"; import type { EditHistoryKind } from "../utils/editHistory"; import { trackStudioEvent } from "../utils/studioTelemetry"; +import { + StudioFileConflictError, + type StudioSaveDrainResult, +} from "../utils/studioSaveDiagnostics"; interface RecordEditInput { label: string; @@ -21,6 +25,25 @@ interface UseEditorSaveOptions { showToast: (message: string, tone?: "error" | "info") => void; } +export interface EditorSaveCandidate { + projectId: string; + path: string; + content: string; +} + +export type EditorSaveDrainResult = StudioSaveDrainResult; + +export interface EditorSaveHandle { + saveRafRef: React.MutableRefObject; + handleContentChange: (content: string) => void; + /** Read by the external-reload reconciliation introduced in stack PR #2993. */ + getPendingCandidate: () => EditorSaveCandidate | null; + /** Wired into the external-reload drain by stack PR #2993. */ + flushPendingSave: () => Promise; + /** Used by PR #2993 when the external version wins. */ + discardPendingSave: () => void; +} + export function useEditorSave({ editingPathRef, projectIdRef, @@ -30,12 +53,70 @@ export function useEditorSave({ domEditSaveTimestampRef, setRefreshKey, showToast, -}: UseEditorSaveOptions) { +}: UseEditorSaveOptions): EditorSaveHandle { const saveRafRef = useRef(null); const refreshRafRef = useRef(null); // One error toast per burst of failures — every keystroke retries the save, // and error toasts persist until dismissed, so don't stack duplicates. const lastFailureToastAtRef = useRef(0); + const pendingCandidateRef = useRef(null); + const inFlightRef = useRef | null>(null); + const inFlightCandidateRef = useRef(null); + + const reportFailure = useCallback( + (path: string, error: unknown) => { + trackStudioEvent("save_failure", { + source: "code_editor", + error_message: error instanceof Error ? error.message : "unknown", + }); + const now = Date.now(); + if (now - lastFailureToastAtRef.current > 5000) { + lastFailureToastAtRef.current = now; + showToast( + `Couldn't save ${path} — your latest edits are NOT persisted. Check the preview server; editing again retries the save.`, + "error", + ); + } + }, + [showToast], + ); + + const persistCandidate = useCallback( + (candidate: EditorSaveCandidate): Promise => { + const task = saveProjectFilesWithHistory({ + projectId: candidate.projectId, + label: "Edit source", + kind: "source", + coalesceKey: `source:${candidate.path}`, + files: { [candidate.path]: candidate.content }, + readFile: readProjectFile, + writeFile: writeProjectFile, + recordEdit, + }) + .then(() => { + if (pendingCandidateRef.current === candidate) pendingCandidateRef.current = null; + if (refreshRafRef.current != null) cancelAnimationFrame(refreshRafRef.current); + refreshRafRef.current = requestAnimationFrame(() => setRefreshKey((k) => k + 1)); + return { status: "clean" }; + }) + .catch((error: unknown) => { + reportFailure(candidate.path, error); + return error instanceof StudioFileConflictError + ? { status: "conflict", error } + : { status: "failed", error }; + }) + .finally(() => { + if (inFlightRef.current === task) { + inFlightRef.current = null; + inFlightCandidateRef.current = null; + } + }); + inFlightRef.current = task; + inFlightCandidateRef.current = candidate; + return task; + }, + [readProjectFile, recordEdit, reportFailure, setRefreshKey, writeProjectFile], + ); const handleContentChange = useCallback( (content: string) => { @@ -44,53 +125,46 @@ export function useEditorSave({ const path = editingPathRef.current; if (!path) return; + const candidate = { projectId: pid, path, content }; + pendingCandidateRef.current = candidate; + if (saveRafRef.current != null) cancelAnimationFrame(saveRafRef.current); saveRafRef.current = requestAnimationFrame(() => { + saveRafRef.current = null; domEditSaveTimestampRef.current = Date.now(); - saveProjectFilesWithHistory({ - projectId: pid, - label: "Edit source", - kind: "source", - coalesceKey: `source:${path}`, - files: { [path]: content }, - readFile: readProjectFile, - writeFile: writeProjectFile, - recordEdit, - }) - .then(() => { - if (refreshRafRef.current != null) cancelAnimationFrame(refreshRafRef.current); - refreshRafRef.current = requestAnimationFrame(() => setRefreshKey((k) => k + 1)); - }) - .catch((error) => { - trackStudioEvent("save_failure", { - source: "code_editor", - error_message: error instanceof Error ? error.message : "unknown", - }); - const now = Date.now(); - if (now - lastFailureToastAtRef.current > 5000) { - lastFailureToastAtRef.current = now; - showToast( - `Couldn't save ${path} — your latest edits are NOT persisted. Check the preview server; editing again retries the save.`, - "error", - ); - } - }); + void persistCandidate(candidate); }); }, - [ - domEditSaveTimestampRef, - editingPathRef, - projectIdRef, - readProjectFile, - recordEdit, - setRefreshKey, - showToast, - writeProjectFile, - ], + [domEditSaveTimestampRef, editingPathRef, projectIdRef, persistCandidate], ); + const flushPendingSave = useCallback(async (): Promise => { + if (saveRafRef.current != null) { + cancelAnimationFrame(saveRafRef.current); + saveRafRef.current = null; + } + const candidate = pendingCandidateRef.current; + if (candidate && candidate === inFlightCandidateRef.current && inFlightRef.current) { + return inFlightRef.current; + } + if (candidate) { + domEditSaveTimestampRef.current = Date.now(); + return persistCandidate(candidate); + } + return (await inFlightRef.current) ?? { status: "clean" }; + }, [domEditSaveTimestampRef, persistCandidate]); + + const discardPendingSave = useCallback(() => { + if (saveRafRef.current != null) cancelAnimationFrame(saveRafRef.current); + saveRafRef.current = null; + pendingCandidateRef.current = null; + }, []); + return { saveRafRef, handleContentChange, + getPendingCandidate: () => pendingCandidateRef.current, + flushPendingSave, + discardPendingSave, }; } diff --git a/packages/studio/src/utils/domEditSaveQueue.test.ts b/packages/studio/src/utils/domEditSaveQueue.test.ts index 33b82444e..f1166f7ee 100644 --- a/packages/studio/src/utils/domEditSaveQueue.test.ts +++ b/packages/studio/src/utils/domEditSaveQueue.test.ts @@ -1,6 +1,6 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { createDomEditSaveQueue } from "./domEditSaveQueue"; -import { StudioSaveHttpError } from "./studioSaveDiagnostics"; +import { StudioFileConflictError, StudioSaveHttpError } from "./studioSaveDiagnostics"; describe("dom edit save queue", () => { afterEach(() => { @@ -115,6 +115,23 @@ describe("dom edit save queue", () => { queue.destroy(); }); + it("clears a stale drain failure after a successful save", async () => { + const failure = new Error("temporary failure"); + const queue = createDomEditSaveQueue(); + + await expect( + queue.enqueue(async () => { + throw failure; + }), + ).rejects.toBe(failure); + await expect(queue.waitForIdle()).resolves.toEqual({ status: "failed", error: failure }); + + await queue.enqueue(async () => undefined); + + await expect(queue.waitForIdle()).resolves.toEqual({ status: "clean" }); + queue.destroy(); + }); + it("pauses immediately on a file conflict instead of retrying stale work", async () => { const onOpen = vi.fn(); const queue = createDomEditSaveQueue({ failureThreshold: 5, onOpen }); @@ -133,4 +150,23 @@ describe("dom edit save queue", () => { await expect(queue.enqueue(async () => {})).rejects.toThrow("Auto-save is paused"); queue.destroy(); }); + + it("returns the original conflict from a drain instead of erasing it", async () => { + const conflict = new StudioFileConflictError({ + filePath: "index.html", + currentVersion: "external-v2", + currentContent: "external", + attemptedContent: "studio", + }); + const queue = createDomEditSaveQueue(); + + await expect( + queue.enqueue(async () => { + throw conflict; + }), + ).rejects.toBe(conflict); + + await expect(queue.waitForIdle()).resolves.toEqual({ status: "conflict", error: conflict }); + queue.destroy(); + }); }); diff --git a/packages/studio/src/utils/domEditSaveQueue.ts b/packages/studio/src/utils/domEditSaveQueue.ts index fc722868b..280c8dadd 100644 --- a/packages/studio/src/utils/domEditSaveQueue.ts +++ b/packages/studio/src/utils/domEditSaveQueue.ts @@ -1,4 +1,9 @@ -import { getStudioSaveErrorMessage, getStudioSaveStatusCode } from "./studioSaveDiagnostics"; +import { + getStudioSaveErrorMessage, + getStudioSaveStatusCode, + StudioFileConflictError, + type StudioSaveDrainResult, +} from "./studioSaveDiagnostics"; interface DomEditSaveQueueOpenEvent { consecutiveFailures: number; @@ -14,11 +19,13 @@ interface DomEditSaveQueueOptions { export interface DomEditSaveQueue { enqueue: (save: () => Promise) => Promise; - waitForIdle: () => Promise; + waitForIdle: () => Promise; reset: () => void; destroy: () => void; } +export type DomEditSaveDrainResult = StudioSaveDrainResult; + const DEFAULT_FAILURE_THRESHOLD = 5; export class DomEditSaveQueueOpenError extends Error { @@ -34,11 +41,13 @@ export function createDomEditSaveQueue(options: DomEditSaveQueueOptions = {}): D let tail = Promise.resolve(); let consecutiveFailures = 0; let breakerOpen = false; + let drainError: unknown = null; const reset = (notify = true) => { const wasOpen = breakerOpen; consecutiveFailures = 0; breakerOpen = false; + drainError = null; if (notify && wasOpen) options.onReset?.(); }; @@ -55,9 +64,13 @@ export function createDomEditSaveQueue(options: DomEditSaveQueueOptions = {}): D const run = async (save: () => Promise): Promise => { try { const result = await save(); - if (!breakerOpen) consecutiveFailures = 0; + if (!breakerOpen) { + consecutiveFailures = 0; + drainError = null; + } return result; } catch (error) { + drainError = error; consecutiveFailures += 1; if (getStudioSaveStatusCode(error) === 409 || consecutiveFailures >= failureThreshold) open(error); @@ -76,8 +89,13 @@ export function createDomEditSaveQueue(options: DomEditSaveQueueOptions = {}): D return queued; }, - async waitForIdle() { + async waitForIdle(): Promise { await tail.catch(() => undefined); + if (drainError instanceof StudioFileConflictError) { + return { status: "conflict", error: drainError }; + } + if (drainError != null) return { status: "failed", error: drainError }; + return { status: "clean" }; }, reset, diff --git a/packages/studio/src/utils/studioFileVersion.test.ts b/packages/studio/src/utils/studioFileVersion.test.ts index ee9980a9a..f17b39501 100644 --- a/packages/studio/src/utils/studioFileVersion.test.ts +++ b/packages/studio/src/utils/studioFileVersion.test.ts @@ -1,5 +1,11 @@ import { describe, expect, it } from "vitest"; -import { studioExpectedFileVersion, studioFileContentVersion } from "./studioFileVersion"; +import { + consumeStudioWriteToken, + markStudioWriteToken, + resetStudioWriteTokens, + studioExpectedFileVersion, + studioFileContentVersion, +} from "./studioFileVersion"; describe("studioFileContentVersion", () => { it("matches the strong SHA-256 ETag format used by studio-server", async () => { @@ -34,3 +40,24 @@ describe("studioFileContentVersion", () => { expect(await studioExpectedFileVersion(versions, "untracked.html")).toBeUndefined(); }); }); + +describe("studio write-token echo identity", () => { + it("suppresses exactly one matching API write receipt without hiding path-only external writes", () => { + resetStudioWriteTokens(); + markStudioWriteToken("studio-write-1"); + + expect(consumeStudioWriteToken("studio-write-1")).toBe(true); + expect(consumeStudioWriteToken("studio-write-1")).toBe(false); + expect(consumeStudioWriteToken(null)).toBe(false); + }); + + it("keeps a token through a slow write and expires abandoned identity state", () => { + resetStudioWriteTokens(); + markStudioWriteToken("slow-studio-write", 1_000); + + expect(consumeStudioWriteToken("slow-studio-write", 61_000)).toBe(true); + + markStudioWriteToken("abandoned-studio-write", 1_000); + expect(consumeStudioWriteToken("abandoned-studio-write", 301_000)).toBe(false); + }); +}); diff --git a/packages/studio/src/utils/studioFileVersion.ts b/packages/studio/src/utils/studioFileVersion.ts index 99da9df12..2737c069b 100644 --- a/packages/studio/src/utils/studioFileVersion.ts +++ b/packages/studio/src/utils/studioFileVersion.ts @@ -1,3 +1,33 @@ +// A token is marked before the request starts, so its lifetime must cover slow writes, +// retries, and the subsequent file-change echo without retaining abandoned tokens forever. +const WRITE_TOKEN_TTL_MS = 5 * 60_000; +const studioWriteTokens = new Map(); + +function pruneStudioWriteTokens(now: number): void { + for (const [token, createdAt] of studioWriteTokens) { + if (now - createdAt >= WRITE_TOKEN_TTL_MS) studioWriteTokens.delete(token); + } +} + +/** Marked by the Studio file writer introduced in external-change stack PR #2990. */ +export function markStudioWriteToken(token: string, now: number = Date.now()): void { + pruneStudioWriteTokens(now); + studioWriteTokens.set(token, now); +} + +/** Consumed by the external-change coordinator introduced in stack PR #2991. */ +export function consumeStudioWriteToken(token: string | null, now: number = Date.now()): boolean { + pruneStudioWriteTokens(now); + if (!token || !studioWriteTokens.has(token)) return false; + studioWriteTokens.delete(token); + return true; +} + +/** Resets module-level echo identity between PR #2991 coordinator tests. */ +export function resetStudioWriteTokens(): void { + studioWriteTokens.clear(); +} + /** Browser-safe SHA-256 version matching studio-server's strong ETag format. */ export async function studioFileContentVersion(content: string): Promise { const bytes = new TextEncoder().encode(content); diff --git a/packages/studio/src/utils/studioPendingEdits.test.ts b/packages/studio/src/utils/studioPendingEdits.test.ts index e9230a1a2..d9837a325 100644 --- a/packages/studio/src/utils/studioPendingEdits.test.ts +++ b/packages/studio/src/utils/studioPendingEdits.test.ts @@ -5,6 +5,7 @@ import { flushStudioPendingEdits, trackStudioPendingEdit, } from "./studioPendingEdits"; +import { StudioFileConflictError } from "./studioSaveDiagnostics"; describe("studio pending edit flush", () => { it("waits for mounted panels to persist pending local edits", async () => { @@ -12,13 +13,119 @@ describe("studio pending edit flush", () => { const remove = addStudioPendingEditFlushListener(persist); try { - await flushStudioPendingEdits(); + await expect(flushStudioPendingEdits()).resolves.toEqual({ status: "clean" }); expect(persist).toHaveBeenCalledTimes(1); } finally { remove(); } }); + it("commits the focused debounced field before draining pending work", async () => { + const input = document.createElement("textarea"); + document.body.append(input); + const persist = vi.fn(async () => undefined); + input.addEventListener("blur", () => { + trackStudioPendingEdit(persist()); + }); + input.focus(); + + await expect(flushStudioPendingEdits()).resolves.toEqual({ status: "clean" }); + + expect(document.activeElement).not.toBe(input); + expect(persist).toHaveBeenCalledOnce(); + input.remove(); + }); + + it("waits for a post-blur effect to register its pending edit listener", async () => { + const input = document.createElement("textarea"); + document.body.append(input); + const persist = vi.fn(async () => undefined); + let removeListener: (() => void) | undefined; + let registrationDone: Promise | undefined; + input.addEventListener("blur", () => { + registrationDone = new Promise((resolve) => { + setTimeout(() => { + removeListener = addStudioPendingEditFlushListener(persist); + resolve(); + }, 0); + }); + }); + input.focus(); + + try { + await expect(flushStudioPendingEdits()).resolves.toEqual({ status: "clean" }); + + expect(persist).toHaveBeenCalledOnce(); + } finally { + await registrationDone; + removeListener?.(); + input.remove(); + } + }); + + it("preserves a pending edit failure instead of reporting a clean drain", async () => { + const failure = new Error("field save failed"); + const remove = addStudioPendingEditFlushListener(async () => { + throw failure; + }); + + try { + await expect(flushStudioPendingEdits()).resolves.toEqual({ + status: "failed", + error: failure, + }); + } finally { + remove(); + } + }); + + it("keeps the full typed conflict payload for the external-change decision", async () => { + const conflict = new StudioFileConflictError({ + filePath: "index.html", + currentVersion: "v2", + currentContent: "external", + attemptedContent: "studio", + }); + const remove = addStudioPendingEditFlushListener(async () => { + throw conflict; + }); + + try { + await expect(flushStudioPendingEdits()).resolves.toEqual({ + status: "conflict", + error: conflict, + }); + } finally { + remove(); + } + }); + + it("prioritizes a conflict when pending edits fail with mixed errors", async () => { + const failure = new Error("field save failed"); + const conflict = new StudioFileConflictError({ + filePath: "index.html", + currentVersion: "v2", + currentContent: "external", + attemptedContent: "studio", + }); + const removeFailure = addStudioPendingEditFlushListener(async () => { + throw failure; + }); + const removeConflict = addStudioPendingEditFlushListener(async () => { + throw conflict; + }); + + try { + await expect(flushStudioPendingEdits()).resolves.toEqual({ + status: "conflict", + error: conflict, + }); + } finally { + removeFailure(); + removeConflict(); + } + }); + it("waits for edits already started by unmounted panels", async () => { const steps: string[] = []; let resolvePersist!: () => void; diff --git a/packages/studio/src/utils/studioPendingEdits.ts b/packages/studio/src/utils/studioPendingEdits.ts index 911907f96..876ce7e43 100644 --- a/packages/studio/src/utils/studioPendingEdits.ts +++ b/packages/studio/src/utils/studioPendingEdits.ts @@ -1,11 +1,32 @@ +import { StudioFileConflictError, type StudioSaveDrainResult } from "./studioSaveDiagnostics"; + const STUDIO_FLUSH_PENDING_EDITS_EVENT = "hf-studio-flush-pending-edits"; interface StudioFlushPendingEditsDetail { promises: Array>; } +export type StudioPendingEditsDrainResult = StudioSaveDrainResult; + const pendingEditPromises = new Set>(); +function waitForPostBlurEffects(): Promise { + return new Promise((resolve) => setTimeout(resolve, 0)); +} + +function inspectDrainFailures(results: PromiseSettledResult[]): { + conflict?: StudioFileConflictError; + firstFailure?: PromiseRejectedResult; +} { + let firstFailure: PromiseRejectedResult | undefined; + for (const result of results) { + if (result.status !== "rejected") continue; + if (result.reason instanceof StudioFileConflictError) return { conflict: result.reason }; + firstFailure ??= result; + } + return { firstFailure }; +} + export function trackStudioPendingEdit( result: Promise | unknown, ): Promise | undefined { @@ -19,16 +40,32 @@ export function trackStudioPendingEdit( return promise; } -export async function flushStudioPendingEdits(): Promise { +export async function flushStudioPendingEdits(): Promise { + const active = document.activeElement; + if ( + active instanceof HTMLElement && + active.matches('input, textarea, select, [contenteditable="true"], [role="textbox"]') + ) { + active.blur(); + // ponytail: Preserve synchronous/microtask blur commits, then cross one task boundary + // so React effects triggered by the blur can register their flush listener. + await Promise.resolve(); + await waitForPostBlurEffects(); + } const detail: StudioFlushPendingEditsDetail = { promises: [] }; window.dispatchEvent( new CustomEvent(STUDIO_FLUSH_PENDING_EDITS_EVENT, { detail }), ); + let firstFailure: PromiseRejectedResult | undefined; while (detail.promises.length > 0 || pendingEditPromises.size > 0) { const promises = [...detail.promises, ...pendingEditPromises]; detail.promises = []; - await Promise.allSettled(promises); + const results = await Promise.allSettled(promises); + const batchFailures = inspectDrainFailures(results); + if (batchFailures.conflict) return { status: "conflict", error: batchFailures.conflict }; + firstFailure ??= batchFailures.firstFailure; } + return firstFailure ? { status: "failed", error: firstFailure.reason } : { status: "clean" }; } export function addStudioPendingEditFlushListener( diff --git a/packages/studio/src/utils/studioSaveDiagnostics.ts b/packages/studio/src/utils/studioSaveDiagnostics.ts index 837af11c4..f635d02c9 100644 --- a/packages/studio/src/utils/studioSaveDiagnostics.ts +++ b/packages/studio/src/utils/studioSaveDiagnostics.ts @@ -56,6 +56,11 @@ export class StudioFileConflictError extends StudioSaveHttpError { } } +export type StudioSaveDrainResult = + | { status: "clean" } + | { status: "conflict"; error: StudioFileConflictError } + | { status: "failed"; error: Failure }; + function readNumericProperty(value: object, key: string): number | undefined { const record = value as Record; const property = record[key]; From b30a23402e2db19b43cb0283e1553048746fb8bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20=C3=81ngel?= Date: Tue, 4 Aug 2026 14:19:42 -0700 Subject: [PATCH 07/19] feat(studio): preserve external file conflicts (#2990) * fix(studio): drain pending edits before reload * fix(studio): address drain review feedback (#2989) - prioritize conflicts and clear recovered DOM queue errors - cover delayed blur effects and missing drain branches - document stacked consumers and extend write-token retention * test(studio): satisfy drain audit gate (#2989) - share the editor-save hook harness across drain regressions - extract settled failure inspection from the drain loop * feat(studio): preserve external file conflicts * fix(studio): isolate retry write receipts * test(studio): cover external conflict recovery safety --- .fallowrc.jsonc | 11 ++ bun.lock | 3 + packages/studio/package.json | 1 + .../src/contexts/FileManagerContext.tsx | 12 ++ .../useFileManager.projectOwnership.test.tsx | 146 ++++++++++++++- packages/studio/src/hooks/useFileManager.ts | 35 +++- .../src/utils/externalConflictStorage.test.ts | 95 ++++++++++ .../src/utils/externalConflictStorage.ts | 172 ++++++++++++++++++ 8 files changed, 465 insertions(+), 10 deletions(-) create mode 100644 packages/studio/src/utils/externalConflictStorage.test.ts create mode 100644 packages/studio/src/utils/externalConflictStorage.ts diff --git a/.fallowrc.jsonc b/.fallowrc.jsonc index b3f221dbe..fd1cc03ff 100644 --- a/.fallowrc.jsonc +++ b/.fallowrc.jsonc @@ -162,6 +162,17 @@ "produceDrawElementFrame", ], }, + // External-conflict persistence is the #2990 stack boundary. The coordinator + // consumes these exports in child PR #2991; keep the primitive independently reviewable. + { + "file": "packages/studio/src/utils/externalConflictStorage.ts", + "exports": [ + "persistExternalConflictSnapshot", + "persistExternalFailureSnapshot", + "loadExternalConflictSnapshot", + "deleteExternalConflictSnapshot", + ], + }, // CLI command files: every command exports a const `examples` per the // convention documented in CLAUDE.md. This is a namespace barrel, not a // collision. diff --git a/bun.lock b/bun.lock index 85a5cbd6f..af5e10cce 100644 --- a/bun.lock +++ b/bun.lock @@ -343,6 +343,7 @@ "@types/react-dom": "19", "@vitejs/plugin-react": "^4.0.0", "autoprefixer": "^10.4.0", + "fake-indexeddb": "^6.2.5", "postcss": "^8.4.0", "puppeteer-core": "^25.2.1", "tailwindcss": "^3.4.0", @@ -1487,6 +1488,8 @@ "extend": ["extend@3.0.2", "", {}, "sha512-fjquC59cD7CyW6urNXK0FBufkZcoiGG80wTuPujX590cB5Ttln20E2UB4S/WARVqhXffZl2LNgS+gQdPIIim/g=="], + "fake-indexeddb": ["fake-indexeddb@6.2.5", "", {}, "sha512-CGnyrvbhPlWYMngksqrSSUT1BAVP49dZocrHuK0SvtR0D5TMs5wP0o3j7jexDJW01KSadjBp1M/71o/KR3nD1w=="], + "fallow": ["fallow@2.75.0", "", { "dependencies": { "detect-libc": "2.1.2" }, "optionalDependencies": { "@fallow-cli/darwin-arm64": "2.75.0", "@fallow-cli/darwin-x64": "2.75.0", "@fallow-cli/linux-arm64-gnu": "2.75.0", "@fallow-cli/linux-arm64-musl": "2.75.0", "@fallow-cli/linux-x64-gnu": "2.75.0", "@fallow-cli/linux-x64-musl": "2.75.0", "@fallow-cli/win32-arm64-msvc": "2.75.0", "@fallow-cli/win32-x64-msvc": "2.75.0" }, "bin": { "fallow": "bin/fallow", "fallow-lsp": "bin/fallow-lsp", "fallow-mcp": "bin/fallow-mcp" } }, "sha512-0/2cquNI/cDLP/LzcCbkwI4hMzkX4tE0VY3/69n3PBBeqFpbM2oai+2Cb0sB8dXB8MDUGPVoPJjDW5GiUo7a1A=="], "fast-deep-equal": ["fast-deep-equal@3.1.3", "", {}, "sha512-f3qQ9oQy9j2AhBe/H9VC91wLmKBCCU/gDOnKNAYG5hswO7BLKj09Hc5HYNz9cGI++xlpDCIgDaitVs03ATR84Q=="], diff --git a/packages/studio/package.json b/packages/studio/package.json index 4219fa97e..aeb1878a2 100644 --- a/packages/studio/package.json +++ b/packages/studio/package.json @@ -85,6 +85,7 @@ "@types/react-dom": "19", "@vitejs/plugin-react": "^4.0.0", "autoprefixer": "^10.4.0", + "fake-indexeddb": "^6.2.5", "postcss": "^8.4.0", "puppeteer-core": "^25.2.1", "tailwindcss": "^3.4.0", diff --git a/packages/studio/src/contexts/FileManagerContext.tsx b/packages/studio/src/contexts/FileManagerContext.tsx index 36120d9d4..73270943a 100644 --- a/packages/studio/src/contexts/FileManagerContext.tsx +++ b/packages/studio/src/contexts/FileManagerContext.tsx @@ -27,9 +27,13 @@ export function FileManagerProvider({ editingPathRef, projectIdRef, saveRafRef, + flushPendingSourceSave, + discardPendingSourceSave, + getPendingSourceCandidate, importedFontAssetsRef, readProjectFile, writeProjectFile, + overwriteExternalConflict, readOptionalProjectFile, observeProjectFileVersion, updateEditingFileContent, @@ -67,9 +71,13 @@ export function FileManagerProvider({ editingPathRef, projectIdRef, saveRafRef, + flushPendingSourceSave, + discardPendingSourceSave, + getPendingSourceCandidate, importedFontAssetsRef, readProjectFile, writeProjectFile, + overwriteExternalConflict, readOptionalProjectFile, observeProjectFileVersion, updateEditingFileContent, @@ -101,9 +109,13 @@ export function FileManagerProvider({ editingPathRef, projectIdRef, saveRafRef, + flushPendingSourceSave, + discardPendingSourceSave, + getPendingSourceCandidate, importedFontAssetsRef, readProjectFile, writeProjectFile, + overwriteExternalConflict, readOptionalProjectFile, observeProjectFileVersion, updateEditingFileContent, diff --git a/packages/studio/src/hooks/useFileManager.projectOwnership.test.tsx b/packages/studio/src/hooks/useFileManager.projectOwnership.test.tsx index 2c86f5104..8d167fdd9 100644 --- a/packages/studio/src/hooks/useFileManager.projectOwnership.test.tsx +++ b/packages/studio/src/hooks/useFileManager.projectOwnership.test.tsx @@ -20,15 +20,65 @@ vi.mock("./useEditorSave", () => ({ useEditorSave: () => ({ saveRafRef: { current: null }, handleContentChange: vi.fn(), + getPendingCandidate: vi.fn(() => null), + flushPendingSave: vi.fn(async () => ({ status: "clean" as const })), + discardPendingSave: vi.fn(), }), })); import { useFileManager } from "./useFileManager"; +import { resetStudioWriteTokens, studioFileContentVersion } from "../utils/studioFileVersion"; +import { StudioFileConflictError } from "../utils/studioSaveDiagnostics"; (globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true; +function useTestFileManager(projectId: string) { + return useFileManager({ + projectId, + showToast: vi.fn(), + recordEdit: vi.fn(async () => {}), + domEditSaveTimestampRef: { current: 0 }, + setRefreshKey: vi.fn(), + }); +} + +async function mountTestFileManager(projectId = "project-a") { + const captured: { manager: ReturnType | null } = { manager: null }; + function Probe() { + captured.manager = useTestFileManager(projectId); + return null; + } + const root = createRoot(document.createElement("div")); + await act(async () => root.render()); + const manager = captured.manager; + if (!manager) throw new Error("file manager did not render"); + return { manager, root }; +} + +async function mountOverwriteRequest(response: Response) { + const fetchMock = vi.fn((_url: string, init?: RequestInit) => { + if (init?.method !== "PUT") { + throw new Error("overwrite unexpectedly performed a preflight read"); + } + return Promise.resolve(response); + }); + vi.stubGlobal("fetch", fetchMock); + return { ...(await mountTestFileManager()), fetchMock }; +} + +function createOverwriteConflict(currentVersion: string | null, currentContent: string | null) { + return new StudioFileConflictError({ + filePath: "index.html", + currentVersion, + currentContent, + attemptedContent: "STUDIO", + }); +} + describe("useFileManager project ownership", () => { afterEach(() => { + resetStudioWriteTokens(); + vi.useRealTimers(); vi.unstubAllGlobals(); }); @@ -54,13 +104,7 @@ describe("useFileManager project ownership", () => { const captured: { manager: ReturnType | null } = { manager: null }; function Probe({ projectId }: { projectId: string }) { - captured.manager = useFileManager({ - projectId, - showToast: vi.fn(), - recordEdit: vi.fn(async () => {}), - domEditSaveTimestampRef: { current: 0 }, - setRefreshKey: vi.fn(), - }); + captured.manager = useTestFileManager(projectId); return null; } @@ -106,4 +150,92 @@ describe("useFileManager project ownership", () => { await act(async () => root.unmount()); }); + + it("uses a fresh write token when a lost response retries a committed save", async () => { + vi.useFakeTimers(); + let putAttempt = 0; + const fetchMock = vi.fn((_url: string, init?: RequestInit) => { + if (!init?.method) { + return Promise.resolve({ + ok: true, + json: async () => ({ content: "BEFORE", version: "v1" }), + } as Response); + } + putAttempt += 1; + if (putAttempt === 1) return Promise.reject(new TypeError("response lost")); + return Promise.resolve({ ok: true, json: async () => ({ version: "v2" }) } as Response); + }); + vi.stubGlobal("fetch", fetchMock); + + const { manager, root } = await mountTestFileManager(); + await manager.readProjectFile("index.html"); + + const write = manager.writeProjectFile("index.html", "AFTER"); + await vi.runAllTimersAsync(); + await write; + + const writeTokens = fetchMock.mock.calls + .filter(([, init]) => init?.method === "PUT") + .map(([, init]) => new Headers(init?.headers).get("X-Hyperframes-Write-Token")); + expect(writeTokens).toHaveLength(2); + expect(writeTokens[0]).toBeTruthy(); + expect(writeTokens[1]).not.toBe(writeTokens[0]); + + await act(async () => root.unmount()); + }); + + it("overwrites the exact external content version with an If-Match precondition", async () => { + const { manager, root, fetchMock } = await mountOverwriteRequest({ + ok: true, + json: async () => ({ version: "v3" }), + } as Response); + const conflict = createOverwriteConflict("v2", "EXTERNAL"); + + await manager.overwriteExternalConflict(conflict); + + const [, init] = fetchMock.mock.calls[0] ?? []; + const headers = new Headers(init?.headers); + expect(init).toMatchObject({ method: "PUT", body: "STUDIO" }); + expect(headers.get("If-Match")).toBe(await studioFileContentVersion("EXTERNAL")); + expect(headers.get("If-None-Match")).toBeNull(); + await act(async () => root.unmount()); + }); + + it("preserves a newer third-party edit when a content-less conflict version is stale", async () => { + const { manager, root, fetchMock } = await mountOverwriteRequest({ + ok: false, + status: 409, + json: async () => ({ currentVersion: "v3", currentContent: "THIRD PARTY" }), + } as Response); + const conflict = createOverwriteConflict("v2", null); + + await expect(manager.overwriteExternalConflict(conflict)).rejects.toMatchObject({ + name: "StudioFileConflictError", + currentVersion: "v3", + currentContent: "THIRD PARTY", + attemptedContent: "STUDIO", + }); + + const [, init] = fetchMock.mock.calls[0] ?? []; + const headers = new Headers(init?.headers); + expect(headers.get("If-Match")).toBe("v2"); + expect(headers.get("If-None-Match")).toBeNull(); + await act(async () => root.unmount()); + }); + + it("uses create-only semantics when the conflicted file was deleted", async () => { + const { manager, root, fetchMock } = await mountOverwriteRequest({ + ok: true, + json: async () => ({ version: "v1" }), + } as Response); + const conflict = createOverwriteConflict(null, null); + + await manager.overwriteExternalConflict(conflict); + + const [, init] = fetchMock.mock.calls[0] ?? []; + const headers = new Headers(init?.headers); + expect(headers.get("If-Match")).toBeNull(); + expect(headers.get("If-None-Match")).toBe("*"); + await act(async () => root.unmount()); + }); }); diff --git a/packages/studio/src/hooks/useFileManager.ts b/packages/studio/src/hooks/useFileManager.ts index 7a54808f6..28eaeb88d 100644 --- a/packages/studio/src/hooks/useFileManager.ts +++ b/packages/studio/src/hooks/useFileManager.ts @@ -10,7 +10,11 @@ import { StudioFileConflictError, StudioSaveNetworkError, } from "../utils/studioSaveDiagnostics"; -import { createStudioWriteToken, studioExpectedFileVersion } from "../utils/studioFileVersion"; +import { + createStudioWriteToken, + markStudioWriteToken, + studioExpectedFileVersion, +} from "../utils/studioFileVersion"; import { useFileTree } from "./useFileTree"; import { useEditorSave } from "./useEditorSave"; @@ -117,8 +121,11 @@ export function useFileManager({ throw await createStudioSaveHttpError(preflight, `Failed to read ${path} before save`); } } - const writeToken = createStudioWriteToken(); await retryStudioSave(async () => { + // Each request gets its own receipt identity. If a committed request loses its response, + // the retry can produce a second filesystem receipt that must be suppressed independently. + const writeToken = createStudioWriteToken(); + markStudioWriteToken(writeToken); let response: Response; try { response = await fetch( @@ -191,7 +198,7 @@ export function useFileManager({ // ── Editor save (debounced content change) ── - const { saveRafRef, handleContentChange } = useEditorSave({ + const editorSave = useEditorSave({ editingPathRef, projectIdRef, readProjectFile, @@ -201,6 +208,24 @@ export function useFileManager({ setRefreshKey, showToast, }); + const { saveRafRef, handleContentChange } = editorSave; + + const overwriteExternalConflict = useCallback( + async (conflict: StudioFileConflictError) => { + if (conflict.currentContent != null) { + await writeProjectFile( + conflict.filePath, + conflict.attemptedContent, + conflict.currentContent, + ); + } else { + fileVersions.set(conflict.filePath, conflict.currentVersion); + await writeProjectFile(conflict.filePath, conflict.attemptedContent); + } + updateEditingFileContent(conflict.filePath, conflict.attemptedContent); + }, + [fileVersions, updateEditingFileContent, writeProjectFile], + ); // ── File select ── @@ -491,11 +516,15 @@ export function useFileManager({ editingPathRef, projectIdRef, saveRafRef, + flushPendingSourceSave: editorSave.flushPendingSave, + discardPendingSourceSave: editorSave.discardPendingSave, + getPendingSourceCandidate: editorSave.getPendingCandidate, importedFontAssetsRef, // Core I/O readProjectFile, writeProjectFile, + overwriteExternalConflict, readOptionalProjectFile, observeProjectFileVersion, updateEditingFileContent, diff --git a/packages/studio/src/utils/externalConflictStorage.test.ts b/packages/studio/src/utils/externalConflictStorage.test.ts new file mode 100644 index 000000000..8febb47e8 --- /dev/null +++ b/packages/studio/src/utils/externalConflictStorage.test.ts @@ -0,0 +1,95 @@ +import { IDBFactory } from "fake-indexeddb"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { + createMemoryExternalConflictStorage, + deleteExternalConflictSnapshot, + loadExternalConflictSnapshot, + persistExternalConflictSnapshot, + type ExternalConflictSnapshot, +} from "./externalConflictStorage"; +import { StudioFileConflictError } from "./studioSaveDiagnostics"; + +describe("external conflict snapshots", () => { + beforeEach(() => { + vi.stubGlobal("indexedDB", new IDBFactory()); + }); + + afterEach(() => { + vi.restoreAllMocks(); + vi.unstubAllGlobals(); + }); + + it("durably records both complete file versions before resolution", async () => { + const storage = createMemoryExternalConflictStorage(); + const snapshot: ExternalConflictSnapshot = { + kind: "conflict", + projectId: "project-a", + filePath: "index.html", + externalVersion: "v2", + externalContent: "external full file", + studioContent: "studio full file", + createdAt: 100, + }; + + await storage.set(snapshot); + expect(await storage.get("project-a", "index.html")).toEqual(snapshot); + await storage.delete("project-a", "index.html"); + expect(await storage.get("project-a", "index.html")).toBeNull(); + }); + + it("records a failed final Studio candidate for remount recovery", async () => { + const storage = createMemoryExternalConflictStorage(); + const snapshot: ExternalConflictSnapshot = { + kind: "failed", + projectId: "project-a", + filePath: "index.html", + externalVersion: null, + externalContent: null, + studioContent: "recover me", + failureMessage: "network unavailable", + createdAt: 101, + }; + + await storage.set(snapshot); + expect(await storage.get("project-a", "index.html")).toEqual(snapshot); + }); + + it("persists, loads, and deletes a snapshot through the production IndexedDB adapter", async () => { + vi.spyOn(Date, "now").mockReturnValue(1234); + const conflict = new StudioFileConflictError({ + filePath: "index.html", + currentVersion: "v2", + currentContent: "external full file", + attemptedContent: "studio full file", + }); + + await persistExternalConflictSnapshot("indexeddb-project", conflict); + await expect(loadExternalConflictSnapshot("indexeddb-project", "index.html")).resolves.toEqual({ + kind: "conflict", + projectId: "indexeddb-project", + filePath: "index.html", + externalVersion: "v2", + externalContent: "external full file", + studioContent: "studio full file", + createdAt: 1234, + }); + await deleteExternalConflictSnapshot("indexeddb-project", "index.html"); + await expect( + loadExternalConflictSnapshot("indexeddb-project", "index.html"), + ).resolves.toBeNull(); + }); + + it("rejects persistence when IndexedDB is unavailable", async () => { + vi.stubGlobal("indexedDB", undefined); + const conflict = new StudioFileConflictError({ + filePath: "index.html", + currentVersion: "v2", + currentContent: "external", + attemptedContent: "studio", + }); + + await expect(persistExternalConflictSnapshot("project-a", conflict)).rejects.toThrow( + "IndexedDB is unavailable; conflict recovery snapshot was not saved", + ); + }); +}); diff --git a/packages/studio/src/utils/externalConflictStorage.ts b/packages/studio/src/utils/externalConflictStorage.ts new file mode 100644 index 000000000..7b783a10f --- /dev/null +++ b/packages/studio/src/utils/externalConflictStorage.ts @@ -0,0 +1,172 @@ +import type { StudioFileConflictError } from "./studioSaveDiagnostics"; + +interface ExternalRecoverySnapshotBase { + projectId: string; + filePath: string; + externalVersion: string | null; + externalContent: string | null; + studioContent: string; + createdAt: number; +} + +export type ExternalConflictSnapshot = + | (ExternalRecoverySnapshotBase & { kind: "conflict" }) + | (ExternalRecoverySnapshotBase & { kind: "failed"; failureMessage: string }); + +export interface ExternalConflictStorage { + get(projectId: string, filePath: string): Promise; + set(snapshot: ExternalConflictSnapshot): Promise; + delete(projectId: string, filePath: string): Promise; +} + +const DB_NAME = "hyperframes-studio-external-conflicts"; +const DB_VERSION = 1; +const STORE_NAME = "file-conflicts"; + +function key(projectId: string, filePath: string): string { + return `${projectId}\0${filePath}`; +} + +export function createMemoryExternalConflictStorage(): ExternalConflictStorage { + const snapshots = new Map(); + return { + async get(projectId, filePath) { + const snapshot = snapshots.get(key(projectId, filePath)); + return snapshot ? structuredClone(snapshot) : null; + }, + async set(snapshot) { + snapshots.set(key(snapshot.projectId, snapshot.filePath), structuredClone(snapshot)); + }, + async delete(projectId, filePath) { + snapshots.delete(key(projectId, filePath)); + }, + }; +} + +function openDb(): Promise { + return new Promise((resolve, reject) => { + if (!globalThis.indexedDB) { + reject(new Error("IndexedDB is unavailable; conflict recovery snapshot was not saved")); + return; + } + const request = globalThis.indexedDB.open(DB_NAME, DB_VERSION); + request.onupgradeneeded = () => { + if (!request.result.objectStoreNames.contains(STORE_NAME)) { + request.result.createObjectStore(STORE_NAME); + } + }; + request.onerror = () => reject(request.error ?? new Error("Failed to open conflict storage")); + request.onsuccess = () => resolve(request.result); + }); +} + +function withStore( + mode: IDBTransactionMode, + callback: (store: IDBObjectStore) => IDBRequest, +): Promise { + return openDb().then( + (db) => + new Promise((resolve, reject) => { + const transaction = db.transaction(STORE_NAME, mode); + const request = callback(transaction.objectStore(STORE_NAME)); + let result!: T; + request.onerror = () => reject(request.error ?? new Error("Conflict storage failed")); + request.onsuccess = () => { + result = request.result; + }; + transaction.oncomplete = () => { + db.close(); + resolve(result); + }; + transaction.onerror = () => { + db.close(); + reject(transaction.error ?? new Error("Conflict storage transaction failed")); + }; + transaction.onabort = () => { + db.close(); + reject(transaction.error ?? new Error("Conflict storage transaction was aborted")); + }; + }), + ); +} + +function createIndexedDbExternalConflictStorage(): ExternalConflictStorage { + return { + async get(projectId, filePath) { + return ( + (await withStore("readonly", (store) => + store.get(key(projectId, filePath)), + )) ?? null + ); + }, + async set(snapshot) { + await withStore("readwrite", (store) => + store.put(snapshot, key(snapshot.projectId, snapshot.filePath)), + ); + }, + async delete(projectId, filePath) { + await withStore("readwrite", (store) => store.delete(key(projectId, filePath))); + }, + }; +} + +const indexedDbStorage = createIndexedDbExternalConflictStorage(); + +/** + * Persist a conflict before offering destructive recovery actions. + * Callers must await this promise: rejection means the Studio draft was not saved durably and + * must remain available in memory while the storage failure is surfaced to the user. + */ +export async function persistExternalConflictSnapshot( + projectId: string, + conflict: StudioFileConflictError, +): Promise { + await indexedDbStorage.set({ + kind: "conflict", + projectId, + filePath: conflict.filePath, + externalVersion: conflict.currentVersion, + externalContent: conflict.currentContent, + studioContent: conflict.attemptedContent, + createdAt: Date.now(), + }); +} + +/** + * Persist the final Studio candidate after its file write fails. + * Callers must await this promise: rejection means the Studio draft was not saved durably and + * must remain available in memory while the storage failure is surfaced to the user. + */ +export async function persistExternalFailureSnapshot( + projectId: string, + filePath: string, + studioContent: string, + externalVersion: string | null, + externalContent: string | null, + error: unknown, +): Promise { + await indexedDbStorage.set({ + kind: "failed", + projectId, + filePath, + externalVersion, + externalContent, + studioContent, + failureMessage: error instanceof Error ? error.message : String(error), + createdAt: Date.now(), + }); +} + +export async function loadExternalConflictSnapshot( + projectId: string, + filePath: string, +): Promise { + return indexedDbStorage.get(projectId, filePath); +} + +export async function deleteExternalConflictSnapshot( + projectId: string, + filePath: string, +): Promise { + await indexedDbStorage.delete(projectId, filePath); +} From a99caad581496f2ad8e132b750ced73be8cf3e51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20=C3=81ngel?= Date: Tue, 4 Aug 2026 15:20:44 -0700 Subject: [PATCH 08/19] feat(studio): coordinate external file changes (#2991) --- .../useExternalFileChangeCoordinator.test.tsx | 221 +++++++++ .../hooks/useExternalFileChangeCoordinator.ts | 429 ++++++++++++++++++ 2 files changed, 650 insertions(+) create mode 100644 packages/studio/src/hooks/useExternalFileChangeCoordinator.test.tsx create mode 100644 packages/studio/src/hooks/useExternalFileChangeCoordinator.ts diff --git a/packages/studio/src/hooks/useExternalFileChangeCoordinator.test.tsx b/packages/studio/src/hooks/useExternalFileChangeCoordinator.test.tsx new file mode 100644 index 000000000..3e0c7168d --- /dev/null +++ b/packages/studio/src/hooks/useExternalFileChangeCoordinator.test.tsx @@ -0,0 +1,221 @@ +// @vitest-environment happy-dom +import { act } from "react"; +import { createRoot } from "react-dom/client"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { StudioFileConflictError } from "../utils/studioSaveDiagnostics"; +import { markStudioWriteToken, resetStudioWriteTokens } from "../utils/studioFileVersion"; +import { + useExternalFileChangeCoordinator, + type ExternalFileChangeCoordinatorHandle, +} from "./useExternalFileChangeCoordinator"; + +(globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + +type HotHandler = (payload?: unknown) => void; +type CoordinatorOptions = Parameters[0]; +const roots: Array> = []; +let handler: HotHandler | null; + +async function mountCoordinator(overrides: Partial = {}) { + const captured: { handle: ExternalFileChangeCoordinatorHandle | null } = { handle: null }; + const defaults: CoordinatorOptions = { + projectId: "project-a", + activeCompPath: "index.html", + pendingTimelineEditPathRef: { current: new Set() }, + drainPendingChanges: vi.fn(async () => ({ status: "clean" as const })), + reloadPreview: vi.fn(), + reloadSdkSession: vi.fn(), + persistConflictSnapshot: vi.fn(async () => undefined), + discardPendingChanges: vi.fn(), + overwriteConflict: vi.fn(async () => undefined), + readProjectFile: vi.fn(async () => "external"), + }; + const options = { ...defaults, ...overrides }; + const root = createRoot(document.createElement("div")); + roots.push(root); + function Probe() { + captured.handle = useExternalFileChangeCoordinator(options); + return null; + } + await act(async () => root.render()); + return { captured, options }; +} + +describe("external file change coordinator", () => { + beforeEach(() => { + handler = null; + resetStudioWriteTokens(); + vi.stubGlobal("__HF_STUDIO_HOT_TEST_ADAPTER__", { + on: (_event: string, next: HotHandler) => { + handler = next; + }, + off: () => { + handler = null; + }, + }); + }); + + afterEach(async () => { + while (roots.length > 0) await act(async () => roots.pop()?.unmount()); + vi.unstubAllGlobals(); + }); + + it("drains before reloading Preview and SDK exactly once", async () => { + const order: string[] = []; + const { captured } = await mountCoordinator({ + drainPendingChanges: async () => { + order.push("drain"); + return { status: "clean" }; + }, + reloadPreview: () => order.push("preview"), + reloadSdkSession: () => order.push("sdk"), + }); + await act(async () => handler?.({ path: "index.html", content: "external", version: "v2" })); + expect(order).toEqual(["drain", "preview", "sdk"]); + expect(captured.handle?.blocked).toBeNull(); + }); + + it("suppresses an exact Studio write receipt", async () => { + const drainPendingChanges = vi.fn(async () => ({ status: "clean" as const })); + const reloadPreview = vi.fn(); + const reloadSdkSession = vi.fn(); + await mountCoordinator({ drainPendingChanges, reloadPreview, reloadSdkSession }); + markStudioWriteToken("studio-write-1"); + await act(async () => + handler?.({ path: "index.html", content: "studio", writeToken: "studio-write-1" }), + ); + expect(drainPendingChanges).not.toHaveBeenCalled(); + expect(reloadPreview).not.toHaveBeenCalled(); + expect(reloadSdkSession).not.toHaveBeenCalled(); + }); + + it("does not suppress a racing external write by path alone", async () => { + const pendingTimelineEditPathRef = { current: new Set(["index.html"]) }; + const drainPendingChanges = vi.fn(async () => ({ status: "clean" as const })); + const reloadPreview = vi.fn(); + const reloadSdkSession = vi.fn(); + await mountCoordinator({ + pendingTimelineEditPathRef, + drainPendingChanges, + reloadPreview, + reloadSdkSession, + }); + await act(async () => handler?.({ path: "index.html", content: "agent edit", version: "v2" })); + expect(pendingTimelineEditPathRef.current).not.toContain("index.html"); + expect(drainPendingChanges).toHaveBeenCalledOnce(); + expect(reloadPreview).toHaveBeenCalledOnce(); + expect(reloadSdkSession).toHaveBeenCalledOnce(); + }); + + it("blocks both reloads and retains a complete conflict", async () => { + const conflict = new StudioFileConflictError({ + filePath: "index.html", + currentVersion: "v2", + currentContent: "external", + attemptedContent: "studio", + }); + const persistConflictSnapshot = vi.fn(async () => undefined); + const { captured, options } = await mountCoordinator({ + drainPendingChanges: async () => ({ status: "conflict", error: conflict }), + persistConflictSnapshot, + }); + await act(async () => handler?.({ path: "index.html", content: "external", version: "v2" })); + expect(persistConflictSnapshot).toHaveBeenCalledWith("project-a", conflict); + expect(captured.handle?.blocked).toMatchObject({ status: "conflict", error: conflict }); + expect(options.reloadPreview).not.toHaveBeenCalled(); + expect(options.reloadSdkSession).not.toHaveBeenCalled(); + }); + + it("ignores stale drain completion after a newer generation", async () => { + const drains: Array<(result: { status: "clean" }) => void> = []; + const { options } = await mountCoordinator({ + drainPendingChanges: () => new Promise((resolve) => drains.push(resolve)), + }); + act(() => { + handler?.({ path: "index.html", content: "first", version: "v2" }); + handler?.({ path: "index.html", content: "second", version: "v3" }); + }); + await act(async () => drains[0]?.({ status: "clean" })); + expect(options.reloadPreview).not.toHaveBeenCalled(); + await act(async () => drains[1]?.({ status: "clean" })); + expect(options.reloadPreview).toHaveBeenCalledOnce(); + expect(options.reloadSdkSession).toHaveBeenCalledOnce(); + }); + + it("restores a durable unresolved conflict after remount", async () => { + const { captured } = await mountCoordinator({ + recoveryFilePath: "index.html", + loadConflictSnapshot: vi.fn(async () => ({ + kind: "conflict" as const, + projectId: "project-a", + filePath: "index.html", + externalVersion: "v2", + externalContent: "external", + studioContent: "studio", + createdAt: 100, + })), + }); + await vi.waitFor(() => expect(captured.handle?.blocked?.status).toBe("conflict")); + expect(captured.handle?.blocked).toMatchObject({ + error: { currentContent: "external", attemptedContent: "studio" }, + }); + }); + + it("retains the final local candidate when a drain fails", async () => { + const failure = new Error("network unavailable"); + const persistFailureSnapshot = vi.fn(async () => undefined); + const deleteConflictSnapshot = vi.fn(async () => undefined); + const { captured } = await mountCoordinator({ + drainPendingChanges: vi + .fn() + .mockResolvedValueOnce({ status: "failed" as const, error: failure }) + .mockResolvedValueOnce({ status: "clean" as const }), + getPendingCandidate: () => ({ path: "index.html", content: "final local candidate" }), + persistFailureSnapshot, + deleteConflictSnapshot, + }); + await act(async () => handler?.({ path: "index.html" })); + expect(captured.handle?.blocked).toMatchObject({ + status: "failed", + error: failure, + studioContent: "final local candidate", + }); + expect(persistFailureSnapshot).toHaveBeenCalledWith( + "project-a", + "index.html", + "final local candidate", + null, + null, + failure, + ); + await act(async () => captured.handle?.retry()); + expect(deleteConflictSnapshot).toHaveBeenCalledWith("project-a", "index.html"); + }); + + it("restores and overwrites from a durable failed draft", async () => { + const overwriteConflict = vi.fn(async () => undefined); + const { captured } = await mountCoordinator({ + recoveryFilePath: "index.html", + overwriteConflict, + loadConflictSnapshot: vi.fn(async () => ({ + kind: "failed" as const, + projectId: "project-a", + filePath: "index.html", + externalVersion: "v2", + externalContent: "external", + studioContent: "recover me", + failureMessage: "network unavailable", + createdAt: 100, + })), + }); + await vi.waitFor(() => expect(captured.handle?.blocked?.status).toBe("failed")); + expect(captured.handle?.blocked).toMatchObject({ + studioContent: "recover me", + recovered: true, + }); + await act(async () => captured.handle?.keepStudioFile()); + expect(overwriteConflict).toHaveBeenCalledWith( + expect.objectContaining({ attemptedContent: "recover me", currentVersion: "v2" }), + ); + }); +}); diff --git a/packages/studio/src/hooks/useExternalFileChangeCoordinator.ts b/packages/studio/src/hooks/useExternalFileChangeCoordinator.ts new file mode 100644 index 000000000..4d1684285 --- /dev/null +++ b/packages/studio/src/hooks/useExternalFileChangeCoordinator.ts @@ -0,0 +1,429 @@ +import { useCallback, useEffect, useRef, useState, type MutableRefObject } from "react"; +import { readStudioFileChangePath } from "../components/editor/manualEdits"; +import { StudioFileConflictError } from "../utils/studioSaveDiagnostics"; +import type { ExternalConflictSnapshot } from "../utils/externalConflictStorage"; +import { isSelfWriteEcho } from "./sdkSelfWriteRegistry"; +import { consumeStudioWriteToken } from "../utils/studioFileVersion"; + +type ExternalChangeDrainResult = + | { status: "clean" } + | { status: "conflict"; error: StudioFileConflictError } + | { status: "failed"; error: unknown }; + +export type ExternalFileChangeBlockedState = + | { + status: "conflict"; + generation: number; + error: StudioFileConflictError; + payload: unknown; + } + | { + status: "failed"; + generation: number; + path: string; + error: unknown; + payload: unknown; + studioContent: string | null; + recovered: boolean; + }; + +interface ExternalFileChangeCoordinatorOptions { + projectId: string | null; + activeCompPath: string | null; + recoveryFilePath?: string | null; + pendingTimelineEditPathRef: MutableRefObject>; + drainPendingChanges: () => Promise; + getPendingCandidate?: () => { path: string; content: string } | null; + discardPendingChanges: () => void; + reloadPreview: () => void; + reloadSdkSession: (path: string) => void; + persistConflictSnapshot: (projectId: string, conflict: StudioFileConflictError) => Promise; + persistFailureSnapshot?: ( + projectId: string, + filePath: string, + studioContent: string, + externalVersion: string | null, + externalContent: string | null, + error: unknown, + ) => Promise; + loadConflictSnapshot?: ( + projectId: string, + filePath: string, + ) => Promise; + deleteConflictSnapshot?: (projectId: string, filePath: string) => Promise; + overwriteConflict: (conflict: StudioFileConflictError) => Promise; + readProjectFile: (path: string) => Promise; + onUseExternalFile?: (path: string, content: string) => void; + resetSaveQueues?: () => void; +} + +export interface ExternalFileChangeCoordinatorHandle { + blocked: ExternalFileChangeBlockedState | null; + retry: () => Promise; + useExternalFile: () => Promise; + keepStudioFile: () => Promise; +} + +interface HotTestAdapter { + on(event: string, handler: (payload?: unknown) => void): void; + off(event: string, handler: (payload?: unknown) => void): void; +} + +function testHotAdapter(): HotTestAdapter | null { + const value = (globalThis as { __HF_STUDIO_HOT_TEST_ADAPTER__?: unknown }) + .__HF_STUDIO_HOT_TEST_ADAPTER__; + if (!value || typeof value !== "object") return null; + const candidate = value as Partial; + return typeof candidate.on === "function" && typeof candidate.off === "function" + ? (candidate as HotTestAdapter) + : null; +} + +function readFileChangeContent(payload: unknown): string | null { + if (!payload || typeof payload !== "object") return null; + const record = payload as Record; + if (typeof record.content === "string") return record.content; + return "data" in record ? readFileChangeContent(record.data) : null; +} + +function readFileChangeVersion(payload: unknown): string | null { + if (!payload || typeof payload !== "object") return null; + const record = payload as Record; + if (typeof record.version === "string") return record.version; + return "data" in record ? readFileChangeVersion(record.data) : null; +} + +function readFileChangeWriteToken(payload: unknown): string | null { + if (!payload || typeof payload !== "object") return null; + const record = payload as Record; + if (typeof record.writeToken === "string") return record.writeToken; + return "data" in record ? readFileChangeWriteToken(record.data) : null; +} + +function eventIdentity(path: string, payload: unknown): string | null { + const version = readFileChangeVersion(payload); + if (version) return `${path}\0${version}`; + const content = readFileChangeContent(payload); + return content == null ? null : `${path}\0${content.length}\0${content}`; +} + +export function useExternalFileChangeCoordinator({ + projectId, + activeCompPath, + recoveryFilePath = activeCompPath, + pendingTimelineEditPathRef, + drainPendingChanges, + getPendingCandidate, + discardPendingChanges, + reloadPreview, + reloadSdkSession, + persistConflictSnapshot, + persistFailureSnapshot, + loadConflictSnapshot, + deleteConflictSnapshot, + overwriteConflict, + readProjectFile, + onUseExternalFile, + resetSaveQueues, +}: ExternalFileChangeCoordinatorOptions): ExternalFileChangeCoordinatorHandle { + const [blocked, setBlocked] = useState(null); + const generationRef = useRef(0); + const mountedRef = useRef(true); + const lastEventIdentityRef = useRef(null); + const blockedRef = useRef(blocked); + const snapshotWriteTailRef = useRef>(Promise.resolve()); + blockedRef.current = blocked; + + useEffect(() => { + mountedRef.current = true; + return () => { + mountedRef.current = false; + generationRef.current += 1; + }; + }, []); + + useEffect(() => { + generationRef.current += 1; + setBlocked(null); + lastEventIdentityRef.current = null; + }, [projectId, activeCompPath]); + + useEffect(() => { + if (!projectId || !recoveryFilePath || !loadConflictSnapshot) return; + const generation = ++generationRef.current; + let cancelled = false; + void loadConflictSnapshot(projectId, recoveryFilePath) + .then((snapshot) => { + if (cancelled || !snapshot || !mountedRef.current || generation !== generationRef.current) { + return; + } + const payload = { + path: snapshot.filePath, + version: snapshot.externalVersion, + content: snapshot.externalContent, + }; + if (snapshot.kind === "failed") { + setBlocked({ + status: "failed", + generation, + path: snapshot.filePath, + error: new Error(snapshot.failureMessage), + payload, + studioContent: snapshot.studioContent, + recovered: true, + }); + } else { + const error = new StudioFileConflictError({ + filePath: snapshot.filePath, + currentVersion: snapshot.externalVersion, + currentContent: snapshot.externalContent, + attemptedContent: snapshot.studioContent, + }); + setBlocked({ status: "conflict", generation, error, payload }); + } + }) + .catch(() => { + // Storage may be unavailable in restricted browser contexts. A failed + // best-effort restore must not create an unhandled rejection or block + // a project that has no known recovery record. + }); + return () => { + cancelled = true; + }; + }, [loadConflictSnapshot, projectId, recoveryFilePath]); + + const reloadAcceptedGeneration = useCallback( + (path: string) => { + reloadPreview(); + reloadSdkSession(path); + }, + [reloadPreview, reloadSdkSession], + ); + + const persistSnapshotInOrder = useCallback(async (write: () => Promise) => { + const next = snapshotWriteTailRef.current.catch(() => undefined).then(write); + snapshotWriteTailRef.current = next.then( + () => undefined, + () => undefined, + ); + await next; + }, []); + + const processChange = useCallback( + // fallow-ignore-next-line complexity + async (payload: unknown, allowDuplicate = false) => { + const path = readStudioFileChangePath(payload); + if (!path || !projectId) return; + const pendingTimelinePaths = pendingTimelineEditPathRef.current; + // The old path-only suppression could drop a real agent/user write that + // raced ahead of the timeline write receipt. Clear the legacy marker but + // decide ownership only from the exact write token/content below. + pendingTimelinePaths.delete(path); + + const content = readFileChangeContent(payload); + if (consumeStudioWriteToken(readFileChangeWriteToken(payload))) return; + if (content != null && isSelfWriteEcho(path, content)) return; + + const identity = eventIdentity(path, payload); + if (!allowDuplicate && identity != null && identity === lastEventIdentityRef.current) return; + lastEventIdentityRef.current = identity; + const generation = ++generationRef.current; + const result = await drainPendingChanges(); + if (!mountedRef.current || generation !== generationRef.current) return; + + if (result.status === "clean") { + const previousBlocked = blockedRef.current; + if (previousBlocked?.status === "failed" && deleteConflictSnapshot) { + try { + await deleteConflictSnapshot(projectId, path); + } catch (error) { + if (mountedRef.current && generation === generationRef.current) { + setBlocked({ ...previousBlocked, generation, error }); + } + return; + } + } + if (!mountedRef.current || generation !== generationRef.current) return; + setBlocked(null); + reloadAcceptedGeneration(path); + return; + } + if (result.status === "failed") { + const candidate = getPendingCandidate?.(); + const studioContent = candidate?.path === path ? candidate.content : null; + let error = result.error; + if (studioContent != null && persistFailureSnapshot) { + try { + await persistSnapshotInOrder(() => + persistFailureSnapshot( + projectId, + path, + studioContent, + readFileChangeVersion(payload), + content, + result.error, + ), + ); + } catch (snapshotError) { + error = new Error( + `Studio could not save the edit or its recovery snapshot: ${ + snapshotError instanceof Error ? snapshotError.message : String(snapshotError) + }`, + { cause: result.error }, + ); + } + } + if (!mountedRef.current || generation !== generationRef.current) return; + setBlocked({ + status: "failed", + generation, + path, + error, + payload, + studioContent, + recovered: false, + }); + return; + } + try { + await persistSnapshotInOrder(() => persistConflictSnapshot(projectId, result.error)); + } catch (error) { + if (!mountedRef.current || generation !== generationRef.current) return; + setBlocked({ + status: "failed", + generation, + path, + error, + payload, + studioContent: result.error.attemptedContent, + recovered: false, + }); + return; + } + if (!mountedRef.current || generation !== generationRef.current) return; + setBlocked({ status: "conflict", generation, error: result.error, payload }); + }, + [ + projectId, + pendingTimelineEditPathRef, + drainPendingChanges, + deleteConflictSnapshot, + getPendingCandidate, + persistConflictSnapshot, + persistFailureSnapshot, + persistSnapshotInOrder, + reloadAcceptedGeneration, + ], + ); + + useEffect(() => { + const handler = (payload?: unknown) => processChange(payload); + const adapter = testHotAdapter(); + if (adapter) { + adapter.on("hf:file-change", handler); + return () => adapter.off("hf:file-change", handler); + } + if (import.meta.hot) { + import.meta.hot.on("hf:file-change", handler); + return () => import.meta.hot?.off?.("hf:file-change", handler); + } + const eventSource = new EventSource("/api/events"); + eventSource.addEventListener("file-change", handler); + return () => eventSource.close(); + }, [processChange]); + + const retry = useCallback(async () => { + const current = blockedRef.current; + if (!current || current.status === "conflict" || current.recovered) return; + resetSaveQueues?.(); + lastEventIdentityRef.current = null; + await processChange(current.payload, true); + }, [processChange, resetSaveQueues]); + + const useExternalFile = useCallback( + // fallow-ignore-next-line complexity + async () => { + const current = blockedRef.current; + if (!current || !projectId || current.generation !== generationRef.current) return; + const path = current.status === "conflict" ? current.error.filePath : current.path; + const external = + current.status === "conflict" && current.error.currentContent != null + ? current.error.currentContent + : await readProjectFile(path); + if (current.generation !== generationRef.current) return; + discardPendingChanges(); + resetSaveQueues?.(); + onUseExternalFile?.(path, external); + await deleteConflictSnapshot?.(projectId, path); + setBlocked(null); + reloadAcceptedGeneration(path); + }, + [ + deleteConflictSnapshot, + discardPendingChanges, + onUseExternalFile, + projectId, + readProjectFile, + reloadAcceptedGeneration, + resetSaveQueues, + ], + ); + + // fallow-ignore-next-line complexity + const keepStudioFile = useCallback(async () => { + const current = blockedRef.current; + if (!current || !projectId) return; + if (current.generation !== generationRef.current) return; + let conflict: StudioFileConflictError; + if (current.status === "conflict") { + conflict = current.error; + } else { + if (!current.recovered || current.studioContent == null) return; + try { + const currentContent = + readFileChangeContent(current.payload) ?? (await readProjectFile(current.path)); + conflict = new StudioFileConflictError({ + filePath: current.path, + currentVersion: readFileChangeVersion(current.payload), + currentContent, + attemptedContent: current.studioContent, + }); + } catch (error) { + if (current.generation === generationRef.current) setBlocked({ ...current, error }); + return; + } + } + try { + await overwriteConflict(conflict); + } catch (error) { + if (current.generation === generationRef.current) { + setBlocked({ + status: "failed", + generation: current.generation, + path: conflict.filePath, + error, + payload: current.payload, + studioContent: conflict.attemptedContent, + recovered: current.status === "failed" && current.recovered, + }); + } + return; + } + if (current.generation !== generationRef.current) return; + discardPendingChanges(); + resetSaveQueues?.(); + await deleteConflictSnapshot?.(projectId, conflict.filePath); + setBlocked(null); + reloadAcceptedGeneration(conflict.filePath); + }, [ + deleteConflictSnapshot, + discardPendingChanges, + overwriteConflict, + projectId, + readProjectFile, + reloadAcceptedGeneration, + resetSaveQueues, + ]); + + return { blocked, retry, useExternalFile, keepStudioFile }; +} From 8a0dccfc72224d993efdb0ae8d95fc0e020327d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20=C3=81ngel?= Date: Tue, 4 Aug 2026 15:43:20 -0700 Subject: [PATCH 09/19] feat(studio): add external conflict recovery UI (#2992) --- .../ExternalFileConflictBanner.test.tsx | 116 ++++++++ .../components/ExternalFileConflictBanner.tsx | 255 ++++++++++++++++++ 2 files changed, 371 insertions(+) create mode 100644 packages/studio/src/components/ExternalFileConflictBanner.test.tsx create mode 100644 packages/studio/src/components/ExternalFileConflictBanner.tsx diff --git a/packages/studio/src/components/ExternalFileConflictBanner.test.tsx b/packages/studio/src/components/ExternalFileConflictBanner.test.tsx new file mode 100644 index 000000000..37112e62e --- /dev/null +++ b/packages/studio/src/components/ExternalFileConflictBanner.test.tsx @@ -0,0 +1,116 @@ +// @vitest-environment happy-dom +import { act } from "react"; +import { createRoot } from "react-dom/client"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { StudioFileConflictError } from "../utils/studioSaveDiagnostics"; +import type { ExternalFileChangeCoordinatorHandle } from "../hooks/useExternalFileChangeCoordinator"; +import { ExternalFileConflictBanner } from "./ExternalFileConflictBanner"; + +(globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + +describe("ExternalFileConflictBanner", () => { + afterEach(() => { + document.body.replaceChildren(); + vi.restoreAllMocks(); + }); + + it("keeps destructive choices explicit and exposes both full versions for review", async () => { + const container = document.createElement("div"); + document.body.append(container); + const root = createRoot(container); + const conflict = new StudioFileConflictError({ + filePath: "index.html", + currentVersion: "v2", + currentContent: "external", + attemptedContent: "studio", + }); + const coordinator: ExternalFileChangeCoordinatorHandle = { + blocked: { status: "conflict", generation: 1, error: conflict, payload: {} }, + retry: vi.fn(async () => undefined), + useExternalFile: vi.fn(async () => undefined), + keepStudioFile: vi.fn(async () => undefined), + }; + + await act(async () => root.render()); + expect(document.querySelector('[role="alert"]')?.textContent).toContain( + "Preview is paused so neither version is lost", + ); + expect(document.body.textContent).toContain("Discard Studio edits and reload file"); + expect(document.body.textContent).toContain("Overwrite file with Studio version"); + + const review = Array.from(document.querySelectorAll("button")).find((button) => + button.textContent?.includes("Review or export both"), + ); + await act(async () => review?.click()); + expect(document.querySelector('[role="dialog"]')).not.toBeNull(); + expect(Array.from(document.querySelectorAll("textarea"), (field) => field.value)).toEqual([ + "external", + "studio", + ]); + + await act(async () => root.unmount()); + }); + + it("lets authors review and export the local candidate after a drain failure", async () => { + const container = document.createElement("div"); + document.body.append(container); + const root = createRoot(container); + const coordinator: ExternalFileChangeCoordinatorHandle = { + blocked: { + status: "failed", + generation: 1, + path: "index.html", + error: new Error("offline"), + payload: {}, + studioContent: "recover me", + recovered: false, + }, + retry: vi.fn(async () => undefined), + useExternalFile: vi.fn(async () => undefined), + keepStudioFile: vi.fn(async () => undefined), + }; + + await act(async () => root.render()); + const review = Array.from(document.querySelectorAll("button")).find((button) => + button.textContent?.includes("Review or export Studio draft"), + ); + await act(async () => review?.click()); + expect(document.querySelector("textarea")?.value).toBe("recover me"); + expect(document.body.textContent).toContain("Copy"); + expect(document.body.textContent).toContain("Download"); + await act(async () => root.unmount()); + }); + + it("does not offer a fake retry after remount and instead offers explicit overwrite", async () => { + const container = document.createElement("div"); + document.body.append(container); + const root = createRoot(container); + const keepStudioFile = vi.fn(async () => undefined); + const coordinator: ExternalFileChangeCoordinatorHandle = { + blocked: { + status: "failed", + generation: 1, + path: "index.html", + error: new Error("offline"), + payload: { path: "index.html", version: "v2", content: "external" }, + studioContent: "recovered draft", + recovered: true, + }, + retry: vi.fn(async () => undefined), + useExternalFile: vi.fn(async () => undefined), + keepStudioFile, + }; + + await act(async () => root.render()); + expect(document.body.textContent).not.toContain("Retry save"); + const overwrite = Array.from(document.querySelectorAll("button")).find((button) => + button.textContent?.includes("Overwrite file with recovered Studio draft"), + ); + expect(overwrite).toBeTruthy(); + vi.spyOn(window, "confirm").mockReturnValue(true); + await act(async () => overwrite?.click()); + expect(keepStudioFile).toHaveBeenCalledOnce(); + + await act(async () => root.unmount()); + }); +}); diff --git a/packages/studio/src/components/ExternalFileConflictBanner.tsx b/packages/studio/src/components/ExternalFileConflictBanner.tsx new file mode 100644 index 000000000..1abddf20c --- /dev/null +++ b/packages/studio/src/components/ExternalFileConflictBanner.tsx @@ -0,0 +1,255 @@ +import { useRef, useState } from "react"; +import type { + ExternalFileChangeBlockedState, + ExternalFileChangeCoordinatorHandle, +} from "../hooks/useExternalFileChangeCoordinator"; +import { useDialogBehavior } from "./ui/useDialogBehavior"; + +function errorMessage(error: unknown): string { + return error instanceof Error ? error.message : String(error); +} + +function downloadText(filename: string, content: string): void { + const url = URL.createObjectURL(new Blob([content], { type: "text/html;charset=utf-8" })); + const anchor = document.createElement("a"); + anchor.href = url; + anchor.download = filename; + anchor.click(); + URL.revokeObjectURL(url); +} + +function ConflictReview({ + conflict, + onClose, +}: { + conflict: Extract; + onClose: () => void; +}) { + const containerRef = useRef(null); + useDialogBehavior({ open: true, onClose, containerRef }); + const external = conflict.error.currentContent ?? "(The server did not return file contents.)"; + const studio = conflict.error.attemptedContent; + + return ( +
+
event.stopPropagation()} + > +
+
+

+ Review both versions of {conflict.error.filePath} +

+

+ Reviewing or exporting does not change either version. +

+
+ +
+
+ {[ + { title: "File on disk", content: external, suffix: "external" }, + { title: "Unsaved Studio version", content: studio, suffix: "studio" }, + ].map((side) => ( +
+
+

{side.title}

+
+ + +
+
+