From 539e027b605a6ce35ea5462f56d59ec540ed79a1 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Tue, 14 Jul 2026 11:52:49 -0700 Subject: [PATCH] fix(studio): wire Grade rollback through the real commit path, scope async completions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes two of the three adversarial findings from the second #2416 tip re-review; the third is a pre-existing runtime-protocol gap, explained in the PR thread rather than patched here. - The Grade rollback added in the previous commit could never fire through the real Studio callback: runDomEditCommit (the shared commit runner used by every data-attribute commit, not just Grade) catches persist failures internally and always resolves, reporting outcome only via its own onError side effect. A caller awaiting the promise never sees a rejection, so the revert-on-reject logic was dead code against the actual app. Added an optional onSettled(ok) callback to DomEditCommitRunnerConfig (purely additive — every existing caller that doesn't pass it is unaffected) and threaded it through commitDataAttribute -> handleDomAttributeLiveCommit -> the onSetAttributeLive prop type (now accepts an optional 3rd argument) -> useColorGradingController, which now drives the revert from the real signal. The promise-rejection path stays as a fallback for any other implementation of onSetAttributeLive that rejects instead. - Selection flushing performed a real side effect (writing the outgoing element's pending edit) during the render-phase identity-reset block. Adjusting STATE during render (comparing against a ref) is React's documented pattern, but it doesn't license actual I/O — React can invoke render more than once per commit, which could double-fire or misorder the write. The reset block now only enqueues the flush (a pure ref write); a new effect keyed on the identity performs it after commit. - Async persist completions (both the onSettled callback and its promise- rejection fallback) now capture the identity key the attempt was made for and check it against the CURRENT identity before touching confirmedGradingRef/grading/runtimeStatus. Without this, a persist that settles after selection has moved on to a THIRD element could clobber that element's freshly-reset state with a result that belongs to an element no longer selected. Not fixed here: the runtime Grade target (HfColorGradingTarget, used by core's resolveTarget to find the DOM element inside the preview iframe) has no source-file/composition-scope discriminator, matching the same gap selectionIdentityKey had before this stack — but fixing it means changing a wire-protocol type shared across core/player/studio and the legacy ColorGradingSection too. hfId (checked first, before id/selector) is minted uniquely per element at parse time in the common case, so this is a narrow residual risk for hfId-less same-selector elements across different source files, not a regression introduced by this stack. Flagged as a follow-up in the PR thread. New/updated regression tests: real onSettled(false) path (distinct from the promise-rejection fallback), and a stale in-flight persist settling after selection has moved on twice more. Full studio suite still at the known pre-existing 55-failure baseline, zero regressions. --- .../propertyPanelColorGradingSection.tsx | 6 +- .../components/editor/propertyPanelTypes.ts | 6 +- .../editor/useColorGradingController.test.ts | 80 ++++++++++++++- .../editor/useColorGradingController.ts | 99 ++++++++++++++----- .../studio/src/hooks/domEditCommitRunner.ts | 11 +++ .../src/hooks/useDomEditAttributeCommits.ts | 5 +- 6 files changed, 179 insertions(+), 28 deletions(-) diff --git a/packages/studio/src/components/editor/propertyPanelColorGradingSection.tsx b/packages/studio/src/components/editor/propertyPanelColorGradingSection.tsx index d37425181..7331cba4e 100644 --- a/packages/studio/src/components/editor/propertyPanelColorGradingSection.tsx +++ b/packages/studio/src/components/editor/propertyPanelColorGradingSection.tsx @@ -138,7 +138,11 @@ export function ColorGradingSection({ assets: string[]; previewIframeRef?: RefObject; onImportAssets?: (files: FileList, dir?: string) => Promise; - onSetAttributeLive: (attr: string, value: string | null) => void | Promise; + onSetAttributeLive: ( + attr: string, + value: string | null, + onSettled?: (ok: boolean) => void, + ) => void | Promise; onApplyScope?: ( scope: "source-file" | "project", value: string | null, diff --git a/packages/studio/src/components/editor/propertyPanelTypes.ts b/packages/studio/src/components/editor/propertyPanelTypes.ts index 833264e01..ba862349f 100644 --- a/packages/studio/src/components/editor/propertyPanelTypes.ts +++ b/packages/studio/src/components/editor/propertyPanelTypes.ts @@ -33,7 +33,11 @@ export interface PropertyPanelProps { onUngroup?: () => void; onSetStyle: (prop: string, value: string) => void | Promise; onSetAttribute: (attr: string, value: string) => void | Promise; - onSetAttributeLive: (attr: string, value: string | null) => void | Promise; + onSetAttributeLive: ( + attr: string, + value: string | null, + onSettled?: (ok: boolean) => void, + ) => void | Promise; onApplyColorGradingScope?: ( scope: "source-file" | "project", value: string | null, diff --git a/packages/studio/src/components/editor/useColorGradingController.test.ts b/packages/studio/src/components/editor/useColorGradingController.test.ts index e9284a52b..a871ca15c 100644 --- a/packages/studio/src/components/editor/useColorGradingController.test.ts +++ b/packages/studio/src/components/editor/useColorGradingController.test.ts @@ -129,7 +129,37 @@ describe("useColorGradingController", () => { vi.useRealTimers(); }); - it("reverts to the last confirmed-good grading when a persist rejects", async () => { + it("reverts to the last confirmed-good grading via the real onSettled(false) signal (matches runDomEditCommit, which never rejects)", async () => { + // The actual Studio commit runner (runDomEditCommit) catches persist + // failures internally and always resolves — it reports outcome only + // through the onSettled callback passed as the 3rd argument. A mock + // that only rejects would validate a path the real callback never takes. + vi.useFakeTimers(); + const onSetAttributeLive = vi.fn( + (_attr: string, _value: string | null, onSettled?: (ok: boolean) => void) => { + onSettled?.(false); + return Promise.resolve(); + }, + ); + const { root, getState } = renderHook(onSetAttributeLive); + act(() => { + getState().commitColorGrading(freshPopGrading()); + }); + expect(getState().grading.preset).toBe("fresh-pop"); + act(() => { + vi.advanceTimersByTime(400); + }); + await act(async () => { + await Promise.resolve(); + await Promise.resolve(); + }); + expect(getState().grading.preset).toBe("neutral"); + expect(getState().runtimeStatus.state).toBe("unavailable"); + act(() => root.unmount()); + vi.useRealTimers(); + }); + + it("reverts to the last confirmed-good grading when a persist rejects (fallback for a non-onSettled implementation)", async () => { vi.useFakeTimers(); const onSetAttributeLive = vi.fn().mockRejectedValue(new Error("disk full")); const { root, getState } = renderHook(onSetAttributeLive); @@ -153,6 +183,54 @@ describe("useColorGradingController", () => { vi.useRealTimers(); }); + it("a stale in-flight persist result does not touch state after selection has moved on to a THIRD element", async () => { + vi.useFakeTimers(); + let resolveA: (() => void) | undefined; + let capturedOnSettledA: ((ok: boolean) => void) | undefined; + const onSetAttributeLive = vi.fn( + (_attr: string, _value: string | null, onSettled?: (ok: boolean) => void) => { + capturedOnSettledA = onSettled; + return new Promise((resolve) => { + resolveA = resolve; + }); + }, + ); + const { root, getState, rerenderWithElement } = renderHook( + onSetAttributeLive, + makeElement({ id: "s1-bg" }), + ); + act(() => { + getState().commitColorGrading(freshPopGrading()); + }); + // Let the debounce fire while still on s1-bg — the persist call is now + // genuinely in flight (its promise won't settle until resolveA() below). + act(() => { + vi.advanceTimersByTime(400); + }); + expect(onSetAttributeLive).toHaveBeenCalledTimes(1); + + // Selection moves twice more while s1-bg's persist is still pending. + rerenderWithElement(makeElement({ id: "s2-bg" })); + rerenderWithElement(makeElement({ id: "s3-bg" })); + expect(getState().grading.preset).toBe("neutral"); // s3-bg's own fresh state + + // NOW the stale s1-bg persist finally settles as a failure. + act(() => { + capturedOnSettledA?.(false); + resolveA?.(); + }); + await act(async () => { + await Promise.resolve(); + await Promise.resolve(); + }); + + // s3-bg's state must be untouched by a result that belongs to s1-bg. + expect(getState().grading.preset).toBe("neutral"); + expect(getState().runtimeStatus.state).not.toBe("unavailable"); + act(() => root.unmount()); + vi.useRealTimers(); + }); + it("resetGrading returns to the neutral preset", () => { const { root, getState } = renderHook(vi.fn()); act(() => { diff --git a/packages/studio/src/components/editor/useColorGradingController.ts b/packages/studio/src/components/editor/useColorGradingController.ts index d2aa58715..ebbcefbaf 100644 --- a/packages/studio/src/components/editor/useColorGradingController.ts +++ b/packages/studio/src/components/editor/useColorGradingController.ts @@ -168,7 +168,11 @@ export function useColorGradingController({ projectId: string; element: DomEditSelection; previewIframeRef?: RefObject; - onSetAttributeLive: (attr: string, value: string | null) => void | Promise; + onSetAttributeLive: ( + attr: string, + value: string | null, + onSettled?: (ok: boolean) => void, + ) => void | Promise; onApplyScope?: ( scope: "source-file" | "project", value: string | null, @@ -190,6 +194,12 @@ export function useColorGradingController({ const persistTimerRef = useRef | null>(null); const pendingPersistValueRef = useRef(undefined); const pendingPersistGradingRef = useRef(null); + // Populated (pure ref write only) during the render-phase identity-change + // reset below; the actual write happens in an effect, never during render. + const queuedOutgoingFlushRef = useRef<{ + setAttributeLive: typeof onSetAttributeLive; + value: string | null; + } | null>(null); // The last grading value actually confirmed saved — distinct from `grading` // (the optimistic value shown immediately on commit). A rejected persist // reverts to this instead of leaving the UI permanently showing a value @@ -216,8 +226,11 @@ export function useColorGradingController({ // Without this, switching selection reuses the previous element's grading/ // compare/mediaMetadata state and can commit stale pending work onto the // new target. Adjusting state during render (comparing against a ref) is - // React's documented pattern for this — it resolves in the same render - // pass instead of flashing the stale state for one frame via useEffect. + // React's documented pattern for STATE updates specifically — resolving in + // the same render pass instead of flashing stale state for one frame. It + // does NOT license side effects: only pure ref/state writes happen in this + // block. The actual outgoing-element flush is enqueued here and performed + // in the effect below, after commit. const identityKey = selectionIdentityKey(element); const identityKeyRef = useRef(identityKey); if (identityKeyRef.current !== identityKey) { @@ -231,9 +244,10 @@ export function useColorGradingController({ // the user just changed; targeting it at the callback bound to the OLD // selection (captured above) keeps it from landing on the new element. if (pendingPersistValueRef.current !== undefined) { - trackStudioPendingEdit( - previousOnSetAttributeLive(COLOR_GRADING_DATA_KEY, pendingPersistValueRef.current), - ); + queuedOutgoingFlushRef.current = { + setAttributeLive: previousOnSetAttributeLive, + value: pendingPersistValueRef.current, + }; } pendingPersistValueRef.current = undefined; pendingPersistGradingRef.current = null; @@ -251,6 +265,18 @@ export function useColorGradingController({ setMediaMetadata(null); } + // Performs the outgoing-element flush queued above — deliberately in an + // effect (post-commit), not inline in the render-phase block, since + // writing to disk is a real side effect and must not run during render + // (React may call render more than once per commit without this code ever + // becoming visible). + useEffect(() => { + const queued = queuedOutgoingFlushRef.current; + if (!queued) return; + queuedOutgoingFlushRef.current = null; + trackStudioPendingEdit(queued.setAttributeLive(COLOR_GRADING_DATA_KEY, queued.value)); + }, [identityKey]); + const target = useMemo( (): HfColorGradingTarget => ({ id: element.id ?? null, @@ -324,24 +350,44 @@ export function useColorGradingController({ }, [refreshRuntimeStatus]); const persistColorGradingValue = useCallback( - (value: string | null, attemptedGrading: NormalizedHfColorGrading) => { - const result = onSetAttributeLiveRef.current(COLOR_GRADING_DATA_KEY, value ?? null); + ( + value: string | null, + attemptedGrading: NormalizedHfColorGrading, + attemptIdentityKey: string, + ) => { + // Selection may move on to a different element while this is in + // flight — the identity-reset block already gave THAT element its own + // confirmedGradingRef baseline, so a result arriving for an element + // we've left must not touch its state. + const applySettled = (ok: boolean) => { + if (identityKeyRef.current !== attemptIdentityKey) return; + if (ok) { + confirmedGradingRef.current = attemptedGrading; + return; + } + // Persist failed — the optimistic grading was never actually saved. + // Revert to the last confirmed-good value instead of leaving the + // control showing an unsaved state as if it succeeded. + const reverted = confirmedGradingRef.current; + latestGradingRef.current = reverted; + setGrading(reverted); + setRuntimeStatus({ state: "unavailable", message: "Save failed — reverted" }); + }; + // `onSettled` is the real signal — the underlying commit runner + // (runDomEditCommit) intentionally swallows persist failures so a + // caller `await`-ing this promise never sees a rejection; a rejection + // handler here alone would be dead code against the actual Studio + // callback. The `.catch` below is a fallback for any OTHER + // implementation of onSetAttributeLive that rejects instead. + const result = onSetAttributeLiveRef.current( + COLOR_GRADING_DATA_KEY, + value ?? null, + applySettled, + ); return trackStudioPendingEdit( Promise.resolve(result).then( - () => { - confirmedGradingRef.current = attemptedGrading; - }, - () => { - // Persist failed — the optimistic grading was never actually - // saved. Revert to the last confirmed-good value instead of - // leaving the control showing an unsaved state as if it succeeded. - // Handled here (not rethrown) — nothing downstream awaits this - // promise's rejection; callers fire it with `void`. - const reverted = confirmedGradingRef.current; - latestGradingRef.current = reverted; - setGrading(reverted); - setRuntimeStatus({ state: "unavailable", message: "Save failed — reverted" }); - }, + () => undefined, + () => applySettled(false), ), ); }, @@ -358,7 +404,7 @@ export function useColorGradingController({ const attemptedGrading = pendingPersistGradingRef.current ?? latestGradingRef.current; pendingPersistValueRef.current = undefined; pendingPersistGradingRef.current = null; - return persistColorGradingValue(value, attemptedGrading); + return persistColorGradingValue(value, attemptedGrading, identityKeyRef.current); }, [persistColorGradingValue]); useEffect(() => addStudioPendingEditFlushListener(flushPendingPersist), [flushPendingPersist]); @@ -444,13 +490,18 @@ export function useColorGradingController({ ? serializeHfColorGrading(nextGrading) : null; pendingPersistGradingRef.current = nextGrading; + // Captured now (edit time), not read fresh inside the timer — the + // timer fires 350ms later and may run after selection has already + // moved on, at which point identityKeyRef.current would no longer + // describe the element this edit was actually made for. + const attemptIdentityKey = identityKeyRef.current; persistTimerRef.current = setTimeout(() => { const value = pendingPersistValueRef.current; const attemptedGrading = pendingPersistGradingRef.current ?? nextGrading; pendingPersistValueRef.current = undefined; pendingPersistGradingRef.current = null; persistTimerRef.current = null; - void persistColorGradingValue(value ?? null, attemptedGrading); + void persistColorGradingValue(value ?? null, attemptedGrading, attemptIdentityKey); }, 350); }, [persistColorGradingValue, postColorGrading, postCompare, scheduleRuntimeStatusRefresh], diff --git a/packages/studio/src/hooks/domEditCommitRunner.ts b/packages/studio/src/hooks/domEditCommitRunner.ts index 9e11e6e3c..516e6b463 100644 --- a/packages/studio/src/hooks/domEditCommitRunner.ts +++ b/packages/studio/src/hooks/domEditCommitRunner.ts @@ -7,6 +7,15 @@ interface DomEditCommitRunnerConfig { onError: (error: unknown) => void; shouldResync: () => boolean; resync: () => void | Promise; + /** + * Reports success/failure without changing this function's own resolve- + * always contract — `persist` failures are handled here (revert + onError) + * and never rethrown, so callers awaiting `runDomEditCommit` can't observe + * failure via rejection. A caller that needs to react to a specific + * commit's outcome (e.g. reverting its OWN optimistic state) can pass this + * instead of relying on a rejection that will never come. + */ + onSettled?: (ok: boolean) => void; } interface CommitVersionRef { @@ -34,11 +43,13 @@ export async function runDomEditCommit(config: DomEditCommitRunnerConfig): Promi try { await config.persist(); + config.onSettled?.(true); } catch (error) { if (config.shouldRevert(error)) { config.revert(); } config.onError(error); + config.onSettled?.(false); } if (!config.shouldResync()) return; diff --git a/packages/studio/src/hooks/useDomEditAttributeCommits.ts b/packages/studio/src/hooks/useDomEditAttributeCommits.ts index 34d7a1599..12aa8ff0a 100644 --- a/packages/studio/src/hooks/useDomEditAttributeCommits.ts +++ b/packages/studio/src/hooks/useDomEditAttributeCommits.ts @@ -25,6 +25,7 @@ interface DataAttributeCommitOptions { coalescePrefix: string; skipRefresh: boolean; refreshAfter?: boolean; + onSettled?: (ok: boolean) => void; } function resolveFullAttrName(attr: string, prefixData: boolean | undefined): string { @@ -128,6 +129,7 @@ export function useDomEditAttributeCommits({ onError: (error) => reportDomEditPersistFailure(domEditSelection, [op], error, showToast), shouldResync: () => isLatestCommit() && !!options.refreshAfter, resync: () => refreshDomEditSelectionFromPreview(domEditSelection), + onSettled: options.onSettled, }); }, [ @@ -153,11 +155,12 @@ export function useDomEditAttributeCommits({ ); const handleDomAttributeLiveCommit = useCallback( - async (attr: string, value: string | null) => { + async (attr: string, value: string | null, onSettled?: (ok: boolean) => void) => { await commitDataAttribute(attr, value, { label: `Edit ${attr.replace(/^(data-)?/, "").replace(/-/g, " ")}`, coalescePrefix: "attr-live", skipRefresh: true, + onSettled, }); }, [commitDataAttribute],