diff --git a/packages/studio/src/hooks/sdkSelfWriteRegistry.test.ts b/packages/studio/src/hooks/sdkSelfWriteRegistry.test.ts new file mode 100644 index 0000000000..3f6ee63544 --- /dev/null +++ b/packages/studio/src/hooks/sdkSelfWriteRegistry.test.ts @@ -0,0 +1,67 @@ +import { describe, expect, it, beforeEach } from "vitest"; +import { + hashContent, + markSelfWrite, + isSelfWriteEcho, + resetSelfWriteRegistry, +} from "./sdkSelfWriteRegistry"; +import { shouldReloadOnFileChange } from "./useSdkSession"; + +describe("sdkSelfWriteRegistry (finding #14)", () => { + beforeEach(() => resetSelfWriteRegistry()); + + it("recognizes the echo of bytes we just wrote", () => { + markSelfWrite("/comp.html", "A"); + expect(isSelfWriteEcho("/comp.html", "A")).toBe(true); + }); + + it("does NOT match different content on the same path (an undo's reverted bytes)", () => { + markSelfWrite("/comp.html", "A"); + expect(isSelfWriteEcho("/comp.html", "REVERTED")).toBe(false); + }); + + it("is keyed per file — a self-write to one file can't mask a change to another", () => { + markSelfWrite("/a.html", "A"); + expect(isSelfWriteEcho("/b.html", "A")).toBe(false); + }); + + it("consumes a matched entry so a later genuine external write isn't suppressed", () => { + markSelfWrite("/comp.html", "A"); + expect(isSelfWriteEcho("/comp.html", "A")).toBe(true); + // A second arrival of identical bytes is NOT our echo — must reload. + expect(isSelfWriteEcho("/comp.html", "A")).toBe(false); + }); + + it("expires entries past the TTL so a stale self-write can't suppress forever", () => { + const t0 = 1_000_000; + markSelfWrite("/comp.html", "A", t0); + // 3 s later (> 2 s TTL) the entry is gone. + expect(isSelfWriteEcho("/comp.html", "A", t0 + 3000)).toBe(false); + }); + + it("hashes are stable and distinguish different content", () => { + expect(hashContent("x")).toBe(hashContent("x")); + expect(hashContent("x")).not.toBe(hashContent("y")); + }); +}); + +describe("shouldReloadOnFileChange (finding #14)", () => { + beforeEach(() => resetSelfWriteRegistry()); + + it("suppresses the reload when content matches a registered self-write (cutover echo)", () => { + markSelfWrite("/comp.html", "SELF"); + expect(shouldReloadOnFileChange("/comp.html", "SELF", true)).toBe(false); + }); + + it("reloads on an undo write even inside the suppress window (content differs)", () => { + // The cutover registered SELF; the undo writes REVERTED bytes within the + // same 2 s window. Time-only suppression dropped this; identity reloads it. + markSelfWrite("/comp.html", "SELF"); + expect(shouldReloadOnFileChange("/comp.html", "REVERTED", true)).toBe(true); + }); + + it("falls back to the time window only when content is unavailable", () => { + expect(shouldReloadOnFileChange("/comp.html", null, true)).toBe(false); + expect(shouldReloadOnFileChange("/comp.html", null, false)).toBe(true); + }); +}); diff --git a/packages/studio/src/hooks/sdkSelfWriteRegistry.ts b/packages/studio/src/hooks/sdkSelfWriteRegistry.ts new file mode 100644 index 0000000000..3b6976d1f9 --- /dev/null +++ b/packages/studio/src/hooks/sdkSelfWriteRegistry.ts @@ -0,0 +1,77 @@ +/** + * Self-write identity registry — discriminates an SDK cutover ECHO from a genuine + * external write (notably undo/redo) in the file-change reload-suppression path. + * + * The old suppression was purely time-based: any file-change within 2 s of the + * shared `domEditSaveTimestampRef` was swallowed. But BOTH an SDK cutover + * self-write AND an undo write set that same timestamp, so the window could not + * tell "the echo of the bytes I just wrote" (suppress) from "the reverted bytes + * an undo just wrote" (must reload). An undo that landed inside the window was + * silently dropped, leaving the in-memory SDK doc on stale pre-undo content. + * + * Fix: tag each cutover self-write with the CONTENT it wrote (by hash). A + * file-change reload is suppressed only when the new on-disk content matches a + * recently-registered self-write hash — i.e. it is provably our own echo. Undo + * writes are never registered (they don't flow through persistSdkSerialize), so + * their content won't match and the reload always fires. Identity, not a clock. + */ + +const SELF_WRITE_TTL_MS = 2000; + +interface SelfWriteEntry { + hash: string; + at: number; +} + +// Module-scoped: the studio process has a single SDK session lifecycle at a time +// and persists are funnelled through one persistSdkSerialize. Keyed by file path +// so a self-write to one file can't mask a real external change to another. +const registry = new Map(); + +/** + * Stable 32-bit FNV-1a hash of content. Collisions only risk SUPPRESSING a real + * reload, and only within the 2 s TTL for the exact same file — negligible, and + * strictly safer than the prior time-only window it replaces. + */ +export function hashContent(content: string): string { + let h = 0x811c9dc5; + for (let i = 0; i < content.length; i++) { + h ^= content.charCodeAt(i); + h = Math.imul(h, 0x01000193); + } + return (h >>> 0).toString(16); +} + +function prune(entries: SelfWriteEntry[], now: number): SelfWriteEntry[] { + return entries.filter((e) => now - e.at < SELF_WRITE_TTL_MS); +} + +/** Record that WE wrote `content` to `path` (an SDK cutover self-write). */ +export function markSelfWrite(path: string, content: string, now: number = Date.now()): void { + const next = prune(registry.get(path) ?? [], now); + next.push({ hash: hashContent(content), at: now }); + registry.set(path, next); +} + +/** + * True when `content` matches a self-write registered for `path` within the TTL. + * Consumes the matched entry so a later genuinely-external write of identical + * bytes isn't suppressed forever. + */ +export function isSelfWriteEcho(path: string, content: string, now: number = Date.now()): boolean { + const entries = prune(registry.get(path) ?? [], now); + const hash = hashContent(content); + const idx = entries.findIndex((e) => e.hash === hash); + if (idx === -1) { + registry.set(path, entries); + return false; + } + entries.splice(idx, 1); + registry.set(path, entries); + return true; +} + +/** Test-only: drop all registered self-writes. */ +export function resetSelfWriteRegistry(): void { + registry.clear(); +} diff --git a/packages/studio/src/hooks/useGsapPropertyDebounce.test.ts b/packages/studio/src/hooks/useGsapPropertyDebounce.test.ts new file mode 100644 index 0000000000..4c2a67fa9f --- /dev/null +++ b/packages/studio/src/hooks/useGsapPropertyDebounce.test.ts @@ -0,0 +1,70 @@ +// @vitest-environment happy-dom +import { describe, it, expect } from "vitest"; +import { openComposition } from "@hyperframes/sdk"; +import { createMemoryAdapter } from "@hyperframes/sdk/adapters/memory"; +import { parseGsapScriptAcorn } from "@hyperframes/core/gsap-parser-acorn"; +import { mergeTweenProperties } from "./useGsapPropertyDebounce"; +import { extractGsapScriptText } from "../utils/gsapSoftReload"; + +const HTML = ` +
+ +`; + +const FROMTO_HTML = ` +
+ +`; + +function tweenProps(comp: { serialize(): string }) { + const parsed = parseGsapScriptAcorn(extractGsapScriptText(comp.serialize()) ?? ""); + const anim = parsed.animations[0]; + return { id: anim?.id, properties: anim?.properties, fromProperties: anim?.fromProperties }; +} + +describe("setGsapTween replace semantics (finding #1)", () => { + it("REGRESSION: a single-key set drops the tween's other animated props", async () => { + // This documents the bug the merge fixes: setGsapTween REPLACES the property + // set, so sending only the edited key loses the siblings. + const comp = await openComposition(HTML, { persist: createMemoryAdapter() }); + const id = tweenProps(comp).id ?? ""; + comp.setGsapTween(id, { properties: { x: 200 } }); + const after = tweenProps(comp); + expect(after.properties).toEqual({ x: 200 }); + expect(after.properties).not.toHaveProperty("y"); + expect(after.properties).not.toHaveProperty("opacity"); + }); +}); + +describe("mergeTweenProperties (finding #1)", () => { + it("editing x preserves y and opacity through a real SDK write", async () => { + const comp = await openComposition(HTML, { persist: createMemoryAdapter() }); + const id = tweenProps(comp).id ?? ""; + // Mirror the send site: merge the single edited prop into the existing set. + const merged = mergeTweenProperties(comp, id, { x: 200 }, "to"); + expect(merged).toEqual({ x: 200, y: 50, opacity: 1 }); + comp.setGsapTween(id, { properties: merged }); + const after = tweenProps(comp); + expect(after.properties).toMatchObject({ x: 200, y: 50, opacity: 1 }); + }); + + it("editing a from-property preserves the other from-properties", async () => { + const comp = await openComposition(FROMTO_HTML, { persist: createMemoryAdapter() }); + const id = tweenProps(comp).id ?? ""; + const merged = mergeTweenProperties(comp, id, { x: 25 }, "from"); + expect(merged).toEqual({ x: 25, y: 0 }); + }); + + it("returns the single edit unchanged when the tween id is unknown", async () => { + const comp = await openComposition(HTML, { persist: createMemoryAdapter() }); + expect(mergeTweenProperties(comp, "no-such-id", { x: 5 }, "to")).toEqual({ x: 5 }); + }); +}); diff --git a/packages/studio/src/hooks/useGsapPropertyDebounce.ts b/packages/studio/src/hooks/useGsapPropertyDebounce.ts index 0d408e02b7..60e031c81b 100644 --- a/packages/studio/src/hooks/useGsapPropertyDebounce.ts +++ b/packages/studio/src/hooks/useGsapPropertyDebounce.ts @@ -1,16 +1,46 @@ import { useCallback, useEffect, useRef } from "react"; import type { Composition } from "@hyperframes/sdk"; +import { parseGsapScriptAcorn } from "@hyperframes/core/gsap-parser-acorn"; import type { DomEditSelection } from "../components/editor/domEditingTypes"; import { sdkGsapTweenPersist, sdkGsapRemovePropertyPersist, type CutoverDeps, } from "../utils/sdkCutover"; +import { extractGsapScriptText } from "../utils/gsapSoftReload"; import { PROPERTY_DEFAULTS } from "./gsapScriptCommitHelpers"; import type { SafeGsapCommitMutation } from "./gsapScriptCommitTypes"; const DEBOUNCE_MS = 150; +/** + * The SDK `setGsapTween` 'set' path REPLACES a tween's editable property set + * (engine `handleSetGsapTween` → `updateAnimationInScript`), so sending only the + * single edited key would silently drop the tween's other animated props. Mirror + * the legacy server path (`{ ...anim.properties, [property]: val }`): read the + * tween's CURRENT properties from the in-memory SDK doc and merge the one edit in, + * so REPLACE semantics preserve siblings. Returns the single-key map unchanged + * when the tween/script can't be found (best-effort; before===after then falls + * back to the server path). + */ +export function mergeTweenProperties( + sdkSession: Composition, + animationId: string, + edited: Record, + kind: "to" | "from", +): Record { + try { + const script = extractGsapScriptText(sdkSession.serialize()); + if (!script) return { ...edited }; + const anim = parseGsapScriptAcorn(script).animations.find((a) => a.id === animationId); + if (!anim) return { ...edited }; + const existing = kind === "from" ? (anim.fromProperties ?? {}) : anim.properties; + return { ...existing, ...edited }; + } catch { + return { ...edited }; + } +} + interface SdkPropertyDeps { sdkSession?: Composition | null; sdkDeps?: CutoverDeps | null; @@ -29,17 +59,32 @@ export function useGsapPropertyDebounce( } | null>(null); const debounceTimerRef = useRef | null>(null); + // The caller passes `sdk` as a fresh object literal every render. Keying any + // callback's deps on it (esp. flushPendingPropertyEdit, whose identity drives + // the unmount-flush cleanup effect) re-fires the cleanup on EVERY parent + // re-render — so a playhead tick mid-slider-drag would flush + record an undo + // entry per render. Hold the latest value in a ref instead so every callback + // reads current deps without re-subscribing on identity churn. + const sdkRef = useRef(sdk); + sdkRef.current = sdk; + const flushPendingPropertyEdit = useCallback(async () => { const pending = pendingPropertyEditRef.current; if (!pending) return; pendingPropertyEditRef.current = null; const { selection, animationId, property, value } = pending; - const { sdkSession, sdkDeps, activeCompPath } = sdk ?? {}; + const { sdkSession, sdkDeps, activeCompPath } = sdkRef.current ?? {}; if (sdkSession && sdkDeps) { const targetPath = selection.sourceFile || activeCompPath || "index.html"; const handled = await sdkGsapTweenPersist( targetPath, - { kind: "set", animationId, properties: { properties: { [property]: value } } }, + { + kind: "set", + animationId, + properties: { + properties: mergeTweenProperties(sdkSession, animationId, { [property]: value }, "to"), + }, + }, sdkSession, sdkDeps, { label: `Edit GSAP ${property}`, coalesceKey: `gsap:${animationId}:${property}` }, @@ -55,7 +100,7 @@ export function useGsapPropertyDebounce( softReload: true, }, ); - }, [commitMutationSafely, sdk]); + }, [commitMutationSafely]); const updateGsapProperty = useCallback( ( @@ -93,12 +138,23 @@ export function useGsapPropertyDebounce( const cs = el.ownerDocument.defaultView?.getComputedStyle(el); defaultValue = cs ? Number.parseFloat(cs.opacity) || 1 : 1; } - const { sdkSession, sdkDeps, activeCompPath } = sdk ?? {}; + const { sdkSession, sdkDeps, activeCompPath } = sdkRef.current ?? {}; if (sdkSession && sdkDeps) { const targetPath = selection.sourceFile || activeCompPath || "index.html"; const handled = await sdkGsapTweenPersist( targetPath, - { kind: "set", animationId, properties: { properties: { [property]: defaultValue } } }, + { + kind: "set", + animationId, + properties: { + properties: mergeTweenProperties( + sdkSession, + animationId, + { [property]: defaultValue }, + "to", + ), + }, + }, sdkSession, sdkDeps, { label: `Add GSAP ${property}` }, @@ -111,12 +167,12 @@ export function useGsapPropertyDebounce( { label: `Add GSAP ${property}` }, ); }, - [commitMutationSafely, sdk], + [commitMutationSafely], ); const removeProperty = useCallback( async (selection: DomEditSelection, animationId: string, property: string, from: boolean) => { - const { sdkSession, sdkDeps, activeCompPath } = sdk ?? {}; + const { sdkSession, sdkDeps, activeCompPath } = sdkRef.current ?? {}; if (sdkSession && sdkDeps) { const targetPath = selection.sourceFile || activeCompPath || "index.html"; const handled = await sdkGsapRemovePropertyPersist( @@ -148,7 +204,7 @@ export function useGsapPropertyDebounce( ); } }, - [commitMutationSafely, sdk], + [commitMutationSafely], ); const removeGsapProperty = useCallback( @@ -164,12 +220,23 @@ export function useGsapPropertyDebounce( property: string, value: number | string, ) => { - const { sdkSession, sdkDeps, activeCompPath } = sdk ?? {}; + const { sdkSession, sdkDeps, activeCompPath } = sdkRef.current ?? {}; if (sdkSession && sdkDeps) { const targetPath = selection.sourceFile || activeCompPath || "index.html"; const handled = await sdkGsapTweenPersist( targetPath, - { kind: "set", animationId, properties: { fromProperties: { [property]: value } } }, + { + kind: "set", + animationId, + properties: { + fromProperties: mergeTweenProperties( + sdkSession, + animationId, + { [property]: value }, + "from", + ), + }, + }, sdkSession, sdkDeps, { @@ -188,13 +255,13 @@ export function useGsapPropertyDebounce( }, ); }, - [commitMutationSafely, sdk], + [commitMutationSafely], ); const addGsapFromProperty = useCallback( async (selection: DomEditSelection, animationId: string, property: string) => { const defaultValue = PROPERTY_DEFAULTS[property] ?? 0; - const { sdkSession, sdkDeps, activeCompPath } = sdk ?? {}; + const { sdkSession, sdkDeps, activeCompPath } = sdkRef.current ?? {}; if (sdkSession && sdkDeps) { const targetPath = selection.sourceFile || activeCompPath || "index.html"; const handled = await sdkGsapTweenPersist( @@ -202,7 +269,14 @@ export function useGsapPropertyDebounce( { kind: "set", animationId, - properties: { fromProperties: { [property]: defaultValue } }, + properties: { + fromProperties: mergeTweenProperties( + sdkSession, + animationId, + { [property]: defaultValue }, + "from", + ), + }, }, sdkSession, sdkDeps, @@ -216,7 +290,7 @@ export function useGsapPropertyDebounce( { label: `Add GSAP from-${property}` }, ); }, - [commitMutationSafely, sdk], + [commitMutationSafely], ); const removeGsapFromProperty = useCallback( diff --git a/packages/studio/src/hooks/useGsapPropertyDebounceFlush.test.ts b/packages/studio/src/hooks/useGsapPropertyDebounceFlush.test.ts new file mode 100644 index 0000000000..7890f0d007 --- /dev/null +++ b/packages/studio/src/hooks/useGsapPropertyDebounceFlush.test.ts @@ -0,0 +1,85 @@ +// @vitest-environment happy-dom +import React, { act, useState } from "react"; +import { createRoot } from "react-dom/client"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { useGsapPropertyDebounce } from "./useGsapPropertyDebounce"; +import type { DomEditSelection } from "../components/editor/domEditingTypes"; + +globalThis.IS_REACT_ACT_ENVIRONMENT = true; + +// The SDK path is gated on STUDIO_SDK_CUTOVER_ENABLED; keep it OFF so the flush +// routes through commitMutationSafely (the spy we count), keeping the test about +// flush TIMING, not the SDK write path. +vi.mock("../components/editor/manualEditingAvailability", () => ({ + STUDIO_SDK_CUTOVER_ENABLED: false, +})); +vi.mock("../utils/studioTelemetry", () => ({ trackStudioEvent: vi.fn() })); + +const selection = { sourceFile: "index.html" } as unknown as DomEditSelection; + +describe("useGsapPropertyDebounce flush stability (finding #7)", () => { + let container: HTMLDivElement; + beforeEach(() => { + vi.useFakeTimers(); + container = document.createElement("div"); + document.body.appendChild(container); + }); + afterEach(() => { + vi.useRealTimers(); + container.remove(); + }); + + it("re-rendering the parent while an edit is pending does NOT flush early or duplicate commits", () => { + const commitMutationSafely = vi.fn(); + let queueEdit: (() => void) | null = null; + let forceRerender: (() => void) | null = null; + + function Harness() { + const [tick, setTick] = useState(0); + forceRerender = () => setTick((t) => t + 1); + // A FRESH sdk wrapper literal every render — the exact churn that, before + // the ref-stabilization fix, re-fired the unmount-flush cleanup effect. + const ops = useGsapPropertyDebounce(commitMutationSafely, { + sdkSession: null, + sdkDeps: null, + activeCompPath: "index.html", + }); + queueEdit = () => ops.updateGsapProperty(selection, "tw-1", "x", tick + 1); + return React.createElement("div", null, String(tick)); + } + + const root = createRoot(container); + act(() => { + root.render(React.createElement(Harness)); + }); + + // Queue one pending edit. + act(() => { + queueEdit?.(); + }); + expect(commitMutationSafely).not.toHaveBeenCalled(); + + // Re-render the parent several times BEFORE the debounce elapses. The bug + // flushed (and recorded a commit) on every re-render via the cleanup effect. + act(() => { + forceRerender?.(); + }); + act(() => { + forceRerender?.(); + }); + act(() => { + forceRerender?.(); + }); + expect(commitMutationSafely).not.toHaveBeenCalled(); + + // The debounce fires exactly once. + act(() => { + vi.advanceTimersByTime(200); + }); + expect(commitMutationSafely).toHaveBeenCalledTimes(1); + + act(() => { + root.unmount(); + }); + }); +}); diff --git a/packages/studio/src/hooks/useGsapScriptCommits.ts b/packages/studio/src/hooks/useGsapScriptCommits.ts index 936e1fb8b0..1c15af70d5 100644 --- a/packages/studio/src/hooks/useGsapScriptCommits.ts +++ b/packages/studio/src/hooks/useGsapScriptCommits.ts @@ -115,6 +115,28 @@ export function useGsapScriptCommits({ projectIdRef, activeCompPath, previewIfra }, [previewIframeRef, reloadPreview, onCacheInvalidate], ); + // Reuse the SAME per-file serializer the legacy commitMutation path uses, so + // SDK gsap-write flushes serialize against legacy commits AND each other — + // overlapping same-file read-modify-writes can't interleave and lose an edit. + const serializeByFile = useCallback( + (key: string, task: () => Promise): Promise => serializerRef.current(key, task), + [], + ); + // Read the on-disk bytes of targetPath so the SDK GSAP persist captures the + // exact prior content as its undo `before` (matching the style/delete paths), + // instead of a normalized full-DOM re-emit that would reformat the whole file. + const readProjectFileContent = useCallback( + async (path: string): Promise => { + const pid = projectIdRef.current; + if (!pid) throw new Error("No active project"); + const res = await fetch(`/api/projects/${pid}/files/${encodeURIComponent(path)}`); + if (!res.ok) throw new Error(`Failed to read ${path}`); + const data = (await res.json()) as { content?: string }; + if (typeof data.content !== "string") throw new Error(`Missing file contents for ${path}`); + return data.content; + }, + [projectIdRef], + ); const sdkDeps = useMemo( () => writeProjectFile @@ -125,6 +147,8 @@ export function useGsapScriptCommits({ projectIdRef, activeCompPath, previewIfra domEditSaveTimestampRef, refresh: sdkRefresh, compositionPath: activeCompPath, + serialize: serializeByFile, + readProjectFile: readProjectFileContent, } : null, [ @@ -134,6 +158,8 @@ export function useGsapScriptCommits({ projectIdRef, activeCompPath, previewIfra domEditSaveTimestampRef, sdkRefresh, activeCompPath, + serializeByFile, + readProjectFileContent, ], ); diff --git a/packages/studio/src/hooks/useSdkSession.ts b/packages/studio/src/hooks/useSdkSession.ts index 1743814fb6..e7b8c7e23e 100644 --- a/packages/studio/src/hooks/useSdkSession.ts +++ b/packages/studio/src/hooks/useSdkSession.ts @@ -4,6 +4,7 @@ import { openComposition } from "@hyperframes/sdk"; import { createHttpAdapter } from "@hyperframes/sdk/adapters/http"; import type { Composition } from "@hyperframes/sdk"; import { readStudioFileChangePath } from "../components/editor/manualEdits"; +import { isSelfWriteEcho } from "./sdkSelfWriteRegistry"; /** * True when an external file-change payload targets the active composition and @@ -24,14 +25,40 @@ export function shouldReloadSdkSession(payload: unknown, activeCompPath: string * stale. The session has NO persist queue — Studio is the sole file writer; see * the open effect below. */ -// Time-window heuristic: suppress file-change reloads for 2 s after our own -// SDK cutover write, to avoid an echo-reload on the write we just committed. -// Footgun: if 2 s is too short (slow FS / network) the reload fires anyway; -// if too long it masks a legitimate external edit. The long-term shape is a -// sequence number or content hash threaded through the persist event so the -// comparison is exact rather than time-based. +// Reload-suppression baseline: a file-change within this window of our own SDK +// cutover write is a CANDIDATE echo, but the decision is content-identity based +// (isSelfWriteEcho) not time-only — so an undo write that lands inside the window +// still reloads (its reverted bytes were never registered as a self-write). The +// window only bounds how long a registered self-write stays suppressible. const SELF_WRITE_SUPPRESS_MS = 2000; +/** Best-effort read of the changed file's content from a file-change payload. */ +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; + if ("data" in record) return readFileChangeContent(record.data); + return null; +} + +/** + * Decide whether a file-change for the active composition should reload the SDK + * session. `content` is the new on-disk bytes (from the payload or a re-read); + * pass null when unavailable. Content-identity wins: a change whose bytes match a + * registered self-write is our own echo (suppress). Without content we can't prove + * identity, so we fall back to the time window ONLY to suppress an echo — an undo + * write outside the window (or any non-self-write) still reloads. Exported for test. + */ +export function shouldReloadOnFileChange( + activeCompPath: string, + content: string | null, + withinSuppressWindow: boolean, +): boolean { + if (content != null) return !isSelfWriteEcho(activeCompPath, content); + // No content to compare — preserve the old time-window echo suppression. + return !withinSuppressWindow; +} + export interface SdkSessionHandle { session: Composition | null; /** @@ -53,15 +80,30 @@ export function useSdkSession( // ── Re-open on external change to the active composition ── useEffect(() => { if (!activeCompPath) return; + const compPath = activeCompPath; + const readAdapter = + projectId != null + ? createHttpAdapter({ projectFilesUrl: `/api/projects/${projectId}` }) + : null; const handler = (payload?: unknown) => { - if (!shouldReloadSdkSession(payload, activeCompPath)) return; - // Suppress reload triggered by our own SDK cutover write. - if ( - domEditSaveTimestampRef && - Date.now() - domEditSaveTimestampRef.current < SELF_WRITE_SUPPRESS_MS - ) + if (!shouldReloadSdkSession(payload, compPath)) return; + const withinWindow = + !!domEditSaveTimestampRef && + Date.now() - domEditSaveTimestampRef.current < SELF_WRITE_SUPPRESS_MS; + const decide = (content: string | null) => { + if (shouldReloadOnFileChange(compPath, content, withinWindow)) setReloadToken((t) => t + 1); + }; + const payloadContent = readFileChangeContent(payload); + // Prefer payload content; otherwise re-read so the decision is by IDENTITY + // (an undo's reverted bytes won't match a registered self-write → reload). + if (payloadContent != null || !readAdapter) { + decide(payloadContent); return; - setReloadToken((t) => t + 1); + } + readAdapter + .read(compPath) + .then((c) => decide(typeof c === "string" ? c : null)) + .catch(() => decide(null)); }; if (import.meta.hot) { import.meta.hot.on("hf:file-change", handler); @@ -72,7 +114,7 @@ export function useSdkSession( es.addEventListener("file-change", handler); return () => es.close(); // eslint-disable-next-line react-hooks/exhaustive-deps - }, [activeCompPath]); + }, [activeCompPath, projectId]); // ── Open / re-open the session ── useEffect(() => { diff --git a/packages/studio/src/hooks/useTimelineEditing.ts b/packages/studio/src/hooks/useTimelineEditing.ts index ffe5c7a45f..688d4127a5 100644 --- a/packages/studio/src/hooks/useTimelineEditing.ts +++ b/packages/studio/src/hooks/useTimelineEditing.ts @@ -188,6 +188,9 @@ export function useTimelineEditing({ reloadPreview, domEditSaveTimestampRef, compositionPath: activeCompPath, + // Capture on-disk bytes as the undo `before` so undoing a timing move + // restores the file verbatim, not a normalized full-DOM re-emit. + readProjectFile: (path) => readFileContent(projectIdRef.current ?? "", path), }, { label: "Move timeline clip", coalesceKey }, ).then((handled) => { @@ -297,6 +300,9 @@ export function useTimelineEditing({ reloadPreview, domEditSaveTimestampRef, compositionPath: activeCompPath, + // Capture on-disk bytes as the undo `before` so undoing a timing + // resize restores the file verbatim, not a normalized full-DOM re-emit. + readProjectFile: (path) => readFileContent(projectIdRef.current ?? "", path), }, { label: "Resize timeline clip", coalesceKey }, ).then((handled) => { diff --git a/packages/studio/src/utils/sdkCutover.test.ts b/packages/studio/src/utils/sdkCutover.test.ts index 9e32d1f3c9..17279a8903 100644 --- a/packages/studio/src/utils/sdkCutover.test.ts +++ b/packages/studio/src/utils/sdkCutover.test.ts @@ -455,6 +455,153 @@ describe("sdkTimingPersist", () => { expect(result).toBe(false); expect(deps.writeProjectFile).not.toHaveBeenCalled(); }); + + // Finding #12: undo baseline must be the EXACT on-disk bytes (matching the + // style/delete paths), not a normalized SDK serialize() re-emit — otherwise + // undoing a timing edit reformats the whole file. + it("records the on-disk content (not serialize()) as the undo before when a reader is provided", async () => { + const deps = { + ...makeDeps(), + readProjectFile: vi.fn().mockResolvedValue("EXACT ON-DISK BYTES"), + }; + const session = makeSession(true); + await sdkTimingPersist("hf-clip", "/comp.html", { start: 3 }, session, deps); + expect(deps.readProjectFile).toHaveBeenCalledWith("/comp.html"); + expect(deps.editHistory.recordEdit).toHaveBeenCalledWith( + expect.objectContaining({ + files: { + "/comp.html": { before: "EXACT ON-DISK BYTES", after: "after" }, + }, + }), + ); + }); + + it("falls back to serialize() before when the reader throws", async () => { + const deps = { + ...makeDeps(), + readProjectFile: vi.fn().mockRejectedValue(new Error("read failed")), + }; + const session = makeSession(true); + await sdkTimingPersist("hf-clip", "/comp.html", { start: 3 }, session, deps); + expect(deps.editHistory.recordEdit).toHaveBeenCalledWith( + expect.objectContaining({ + files: { "/comp.html": { before: "before", after: "after" } }, + }), + ); + }); +}); + +describe("sdkGsapTweenPersist — undo baseline (finding #12)", () => { + const makeRef = (val: T): MutableRefObject => ({ current: val }); + const makeSession = () => + ({ + getElement: vi.fn().mockReturnValue({ id: "hf-box" }), + setGsapTween: vi.fn(), + serialize: vi + .fn() + .mockReturnValueOnce("serialized-before") + .mockReturnValue("after"), + batch: vi.fn((fn: () => void) => fn()), + }) as unknown as Parameters[2]; + + it("records the on-disk content as the undo before, not serialize()", async () => { + const deps = { + editHistory: { recordEdit: vi.fn().mockResolvedValue(undefined) }, + writeProjectFile: vi.fn().mockResolvedValue(undefined), + reloadPreview: vi.fn(), + domEditSaveTimestampRef: makeRef(0), + readProjectFile: vi.fn().mockResolvedValue("on-disk gsap bytes"), + }; + const session = makeSession(); + await sdkGsapTweenPersist( + "/comp.html", + { kind: "set", animationId: "tw-1", properties: { ease: "power3.in" } }, + session, + deps, + ); + expect(deps.editHistory.recordEdit).toHaveBeenCalledWith( + expect.objectContaining({ + files: { + "/comp.html": { before: "on-disk gsap bytes", after: "after" }, + }, + }), + ); + }); +}); + +describe("sdkGsapTweenPersist — per-file serialization (finding #8)", () => { + const makeRef = (val: T): MutableRefObject => ({ current: val }); + + it("routes the read-modify-write through the keyed serializer so same-file flushes can't interleave", async () => { + const order: string[] = []; + let writeResolve: (() => void) | null = null; + const deps = { + editHistory: { recordEdit: vi.fn().mockResolvedValue(undefined) }, + // First write blocks until we release it, so without serialization the + // second op's serialize()/dispatch would interleave ahead of it. + writeProjectFile: vi.fn().mockImplementation((_p: string, content: string) => { + order.push(`write-start:${content}`); + if (content === "after-1") { + return new Promise((res) => { + writeResolve = () => { + order.push(`write-done:${content}`); + res(); + }; + }); + } + order.push(`write-done:${content}`); + return Promise.resolve(); + }), + reloadPreview: vi.fn(), + domEditSaveTimestampRef: makeRef(0), + // A real per-key serializer: tasks under the same key run strictly in order. + serialize: (() => { + const inFlight = new Map>(); + return (key: string, task: () => Promise): Promise => { + const prior = inFlight.get(key) ?? Promise.resolve(); + const next = prior.then(task, task); + inFlight.set(key, next); + return next as Promise; + }; + })(), + }; + + let serializeCall = 0; + const session = { + getElement: vi.fn().mockReturnValue({ id: "hf-box" }), + setGsapTween: vi.fn(() => order.push("dispatch")), + serialize: vi.fn(() => { + serializeCall++; + // before-1, after-1, before-2, after-2 + return `${serializeCall % 2 === 1 ? "before" : "after"}-${Math.ceil(serializeCall / 2)}`; + }), + batch: vi.fn((fn: () => void) => fn()), + } as unknown as Parameters[2]; + + const p1 = sdkGsapTweenPersist( + "/comp.html", + { kind: "set", animationId: "tw-1", properties: { ease: "a" } }, + session, + deps, + ); + const p2 = sdkGsapTweenPersist( + "/comp.html", + { kind: "set", animationId: "tw-1", properties: { ease: "b" } }, + session, + deps, + ); + // Let the first op reach its (blocked) write before releasing it. + await Promise.resolve(); + await Promise.resolve(); + writeResolve?.(); + await Promise.all([p1, p2]); + + // The second op's write must NOT start before the first op's write completes. + const firstWriteDone = order.indexOf("write-done:after-1"); + const secondWriteStart = order.indexOf("write-start:after-2"); + expect(firstWriteDone).toBeGreaterThanOrEqual(0); + expect(secondWriteStart).toBeGreaterThan(firstWriteDone); + }); }); describe("sdkGsapTweenPersist", () => { diff --git a/packages/studio/src/utils/sdkCutover.ts b/packages/studio/src/utils/sdkCutover.ts index e6f38dac86..fcd95f12d4 100644 --- a/packages/studio/src/utils/sdkCutover.ts +++ b/packages/studio/src/utils/sdkCutover.ts @@ -5,6 +5,7 @@ import type { EditHistoryKind } from "./editHistory"; import type { PatchOperation } from "./sourcePatcher"; import { STUDIO_SDK_CUTOVER_ENABLED } from "../components/editor/manualEditingAvailability"; import { trackStudioEvent } from "./studioTelemetry"; +import { markSelfWrite } from "../hooks/sdkSelfWriteRegistry"; const CUTOVER_OP_TYPES = new Set([ "inline-style", @@ -90,6 +91,42 @@ export interface CutoverDeps { * — otherwise we'd write the full active-comp serialization into that file. */ compositionPath?: string | null; + /** + * Optional per-key task serializer (the same `gsap-file:${file}` serializer the + * legacy `commitMutation` uses). When provided, every GSAP-op persist routes its + * read-serialize → dispatch → serialize → write through it so two concurrent + * same-file flushes can't interleave their read-modify-write and lose an edit. + * Absent (e.g. in unit tests) → ops run unserialized as before. + */ + serialize?: (key: string, task: () => Promise) => Promise; + /** + * Optional reader for the on-disk content of targetPath. Timing/GSAP persists + * use it to capture the EXACT prior bytes as the undo-history `before`, so undo + * restores the file verbatim instead of a normalized SDK re-emit (which would + * reformat the whole file). The style/delete paths already thread originalContent + * in explicitly; this gives timing/GSAP parity without touching every call site. + * Absent → falls back to the SDK's pre-edit serialize() (the prior behavior). + */ + readProjectFile?: (path: string) => Promise; +} + +/** + * Capture the undo-history `before` baseline for timing/GSAP persists: the exact + * on-disk bytes when a reader is available (so undo restores them verbatim), + * falling back to the SDK's pre-edit serialization when it isn't. Never throws — + * a failed read degrades to the serialized fallback rather than aborting the edit. + */ +async function captureOnDiskBefore( + deps: CutoverDeps, + targetPath: string, + serializedFallback: string, +): Promise { + if (!deps.readProjectFile) return serializedFallback; + try { + return await deps.readProjectFile(targetPath); + } catch { + return serializedFallback; + } } /** True when targetPath isn't the composition the SDK session models. */ @@ -115,6 +152,11 @@ async function persistSdkSerialize( options?: CutoverOptions, ): Promise { deps.domEditSaveTimestampRef.current = Date.now(); + // Tag this write with the exact content (by hash) so the file-change + // reload-suppression can recognize its own echo by IDENTITY, not just a 2 s + // clock — an undo write (different bytes, not registered here) then always + // reloads instead of being swallowed by the time window. + markSelfWrite(targetPath, after); await deps.writeProjectFile(targetPath, after); await deps.editHistory.recordEdit({ label: options?.label ?? "Edit layer", @@ -178,16 +220,15 @@ export async function sdkTimingPersist( if (!sdkSession || !sdkSession.getElement(hfId)) return false; if (wrongCompositionFile(deps, targetPath)) return false; try { - // `before` is the SDK's serialized state, which is the true pre-edit - // content only while every edit routes through this session. During the - // dark-launch transition (server still writes some paths) the in-memory - // SDK doc can drift from disk, so this `before` may not match the file's - // actual prior bytes. Acceptable for v1; revisit once cutover is always-on. - const before = sdkSession.serialize(); + const serializedBefore = sdkSession.serialize(); sdkSession.batch(() => sdkSession.setTiming(hfId, timingUpdate)); const after = sdkSession.serialize(); - if (after === before) return false; - await persistSdkSerialize(after, targetPath, before, deps, options); + if (after === serializedBefore) return false; + // Undo baseline = exact on-disk bytes (matching the style/delete paths), so + // undoing a timing edit restores the file verbatim instead of a normalized + // full-DOM re-emit. Falls back to serializedBefore when no reader is wired. + const undoBefore = await captureOnDiskBefore(deps, targetPath, serializedBefore); + await persistSdkSerialize(after, targetPath, undoBefore, deps, options); trackStudioEvent("sdk_cutover_success", { hfId, opCount: 1 }); return true; } catch (err) { @@ -238,18 +279,30 @@ async function dispatchGsapOpAndPersist( if (!STUDIO_SDK_CUTOVER_ENABLED) return false; if (!sdkSession) return false; if (wrongCompositionFile(deps, targetPath)) return false; - try { - const before = sdkSession.serialize(); - dispatch(sdkSession); - const after = sdkSession.serialize(); - if (after === before) return false; - await persistSdkSerialize(after, targetPath, before, deps, options); - trackStudioEvent("sdk_cutover_success", { opCount: 1 }); - return true; - } catch (err) { - trackStudioEvent("sdk_cutover_fallback", { error: String(err) }); - return false; - } + const session = sdkSession; + // Route the whole read-serialize → dispatch → serialize → write through the + // per-file serializer (when provided) so overlapping same-file flushes can't + // interleave their read-modify-write and drop an edit, matching the legacy + // commitMutation path's `gsap-file:${file}` serialization. + const run = async (): Promise => { + try { + const serializedBefore = session.serialize(); + dispatch(session); + const after = session.serialize(); + if (after === serializedBefore) return false; + // Undo baseline = exact on-disk bytes (matching the style/delete paths), so + // undoing a GSAP edit restores the file verbatim instead of a normalized + // full-DOM re-emit. Falls back to serializedBefore when no reader is wired. + const undoBefore = await captureOnDiskBefore(deps, targetPath, serializedBefore); + await persistSdkSerialize(after, targetPath, undoBefore, deps, options); + trackStudioEvent("sdk_cutover_success", { opCount: 1 }); + return true; + } catch (err) { + trackStudioEvent("sdk_cutover_fallback", { error: String(err) }); + return false; + } + }; + return deps.serialize ? deps.serialize(`gsap-file:${targetPath}`, run) : run(); } export function sdkGsapKeyframePersist(