diff --git a/packages/studio/src/components/editor/propertyPanel3dTransform.tsx b/packages/studio/src/components/editor/propertyPanel3dTransform.tsx index e38cc58eeb..af23372e64 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 1ee79c0eae..509517be6f 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 9c6de843a1..831c80467a 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 29c167e57d..ce99e13f8e 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 96b2ed3154..383323b2e6 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 0000000000..e07c1eab6b --- /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 ec4a948aba..976921a1f4 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 935ceb3f67..84a61f80df 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 7bce8dbbcc..1922929bfe 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 df61e55867..51bc8ce6fd 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 defec9f81f..e664db4900 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 5bc19ec4af..177d8c5c5d 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 0000000000..ac394c0c56 --- /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 667f3caea9..8cabc37ac9 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],