From 1deb0dc97066edcb56bbc05e8b8269f4fd22a6e1 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Tue, 14 Jul 2026 02:14:21 -0700 Subject: [PATCH] fix(studio): resolve the 4 cumulative blockers from the #2416 tip re-review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes the Deepwork tip re-review's four remaining blockers plus its additive findings: - selectionIdentityKey: add sourceFile as a 5th identity component. The same local id/selector can legitimately recur across different composition files (host vs. an inlined sub-composition, or two unrelated sub-comps) — without sourceFile, those collided onto the same identity key and reused stale controller state across a selection change that should have reset it. - useColorGradingController: flush (not discard) a pending Grade edit when selection changes before the 350ms debounce fires. The prior fix correctly stopped it from landing on the WRONG (new) target, but cancelling outright silently dropped the user's in-flight edit instead of writing it to the element it was authored for — using the onSetAttributeLive closure captured for the outgoing render, which (via commitDataAttribute's own useCallback deps) is still bound to the outgoing selection. - useColorGradingController: revert to the last confirmed-good grading when a persist rejects, instead of leaving the optimistic (never-actually- saved) value showing indefinitely. Tracks a separate confirmedGradingRef, updated only on a successful persist. - FlatSelectRow: disable the reset button when the row itself is disabled (it previously ignored disabled entirely, same class of bug as the FlatSlider reset button fixed earlier) and give the underlying onChange(e.target.value)} className={`appearance-none bg-transparent text-right font-mono text-[11px] outline-none disabled:cursor-not-allowed ${VALUE_TIER_VALUE_CLASS[tier]}`} > @@ -531,8 +540,9 @@ export function FlatSelectRow({ type="button" data-flat-select-reset="true" title="Remove — fall back to default" + disabled={disabled} onClick={onReset} - className="flex-shrink-0 text-panel-text-3 opacity-0 transition-opacity hover:text-panel-text-1 group-hover:opacity-100" + className="flex-shrink-0 text-panel-text-3 opacity-0 transition-opacity hover:text-panel-text-1 group-hover:opacity-100 disabled:cursor-not-allowed disabled:opacity-40" > diff --git a/packages/studio/src/components/editor/propertyPanelHelpers.ts b/packages/studio/src/components/editor/propertyPanelHelpers.ts index 13500fa1c..8fcb0ce98 100644 --- a/packages/studio/src/components/editor/propertyPanelHelpers.ts +++ b/packages/studio/src/components/editor/propertyPanelHelpers.ts @@ -30,18 +30,22 @@ export function isSelectedElementHidden( } /** - * 4-part element identity for keying panel remounts on selection change — + * 5-part element identity for keying panel remounts on selection change — * id or selector alone collides for id-less same-selector siblings, leaving - * mount-initialized state pointed at the previous element. + * mount-initialized state pointed at the previous element. sourceFile is + * required too: the same local id/selector can legitimately recur across + * different composition files (host vs. an inlined sub-composition, or two + * unrelated sub-comps), and without it those collide onto the same key. */ export function selectionIdentityKey( - element: Pick, + element: Pick, ): string { return [ element.id ?? "", element.hfId ?? "", element.selector ?? "", String(element.selectorIndex ?? ""), + element.sourceFile ?? "", ].join("|"); } diff --git a/packages/studio/src/components/editor/useColorGradingController.test.ts b/packages/studio/src/components/editor/useColorGradingController.test.ts index e3fdf5379..e9284a52b 100644 --- a/packages/studio/src/components/editor/useColorGradingController.test.ts +++ b/packages/studio/src/components/editor/useColorGradingController.test.ts @@ -129,6 +129,30 @@ describe("useColorGradingController", () => { vi.useRealTimers(); }); + it("reverts to the last confirmed-good grading when a persist rejects", async () => { + vi.useFakeTimers(); + const onSetAttributeLive = vi.fn().mockRejectedValue(new Error("disk full")); + const { root, getState } = renderHook(onSetAttributeLive); + act(() => { + getState().commitColorGrading(freshPopGrading()); + }); + expect(getState().grading.preset).toBe("fresh-pop"); + act(() => { + vi.advanceTimersByTime(400); + }); + // The rejection settles on a microtask, not a timer — flush it. + await act(async () => { + await Promise.resolve(); + await Promise.resolve(); + }); + // Reverted to "neutral" (the last confirmed-good value, from before this + // commit) instead of permanently showing "fresh-pop" as if it had saved. + expect(getState().grading.preset).toBe("neutral"); + expect(getState().runtimeStatus.state).toBe("unavailable"); + act(() => root.unmount()); + vi.useRealTimers(); + }); + it("resetGrading returns to the neutral preset", () => { const { root, getState } = renderHook(vi.fn()); act(() => { @@ -159,7 +183,25 @@ describe("useColorGradingController", () => { act(() => root.unmount()); }); - it("cancels a pending persist scheduled for the previous element when selection changes before it flushes", () => { + it("also resets when the same local id/selector recurs in a different source file", () => { + // Same id, same selector, same selectorIndex — only sourceFile differs. + // Without sourceFile in the identity key, this would collide with the + // first element (e.g. host composition vs. an inlined sub-composition, + // or two unrelated sub-comps that happen to share a local id). + const { root, getState, rerenderWithElement } = renderHook( + vi.fn(), + makeElement({ id: "bg", sourceFile: "index.html" }), + ); + act(() => { + getState().commitColorGrading(freshPopGrading()); + }); + expect(getState().grading.preset).toBe("fresh-pop"); + rerenderWithElement(makeElement({ id: "bg", sourceFile: "sub-comp.html" })); + expect(getState().grading.preset).toBe("neutral"); + act(() => root.unmount()); + }); + + it("flushes — rather than discards — a pending persist for the previous element when selection changes before it fires", () => { vi.useFakeTimers(); const onSetAttributeLive = vi.fn(); const { root, getState, rerenderWithElement } = renderHook( @@ -169,16 +211,24 @@ describe("useColorGradingController", () => { act(() => { getState().commitColorGrading(freshPopGrading()); }); - // Switch selection before the 350ms debounce flushes — the queued write - // targeted the OLD element and must not land on whatever is selected now. + // Switch selection before the 350ms debounce fires — the in-flight edit + // must be written immediately (targeting the OUTGOING element's own + // commit callback), not silently dropped just because a debounce timer + // hadn't elapsed yet. act(() => { vi.advanceTimersByTime(200); }); rerenderWithElement(makeElement({ id: "s2-bg" })); + expect(onSetAttributeLive).toHaveBeenCalledTimes(1); + const [attr, value] = onSetAttributeLive.mock.calls[0] as [string, string]; + expect(attr).toBe("color-grading"); + expect(value).toContain("fresh-pop"); + // And it must not ALSO fire again once the (now-cleared) original timer + // window would have elapsed. act(() => { vi.advanceTimersByTime(400); }); - expect(onSetAttributeLive).not.toHaveBeenCalled(); + expect(onSetAttributeLive).toHaveBeenCalledTimes(1); act(() => root.unmount()); vi.useRealTimers(); }); diff --git a/packages/studio/src/components/editor/useColorGradingController.ts b/packages/studio/src/components/editor/useColorGradingController.ts index 4710e906a..d2aa58715 100644 --- a/packages/studio/src/components/editor/useColorGradingController.ts +++ b/packages/studio/src/components/editor/useColorGradingController.ts @@ -189,10 +189,22 @@ export function useColorGradingController({ const [mediaMetadata, setMediaMetadata] = useState(null); const persistTimerRef = useRef | null>(null); const pendingPersistValueRef = useRef(undefined); + const pendingPersistGradingRef = useRef(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 + // that was never written. + const confirmedGradingRef = useRef(grading); const statusTimersRef = useRef([]); const onSetAttributeLiveRef = useRef(onSetAttributeLive); const latestGradingRef = useRef(grading); const compareEnabledRef = useRef(compareEnabled); + // Captured before reassignment below — still bound to whatever selection + // was current on the PREVIOUS render. `commitDataAttribute` (the eventual + // callee) closes over `domEditSelection` in its own useCallback deps, so a + // selection change mints an entirely new `onSetAttributeLive` closure; this + // stale reference is exactly what still targets the outgoing element. + const previousOnSetAttributeLive = onSetAttributeLiveRef.current; onSetAttributeLiveRef.current = onSetAttributeLive; latestGradingRef.current = grading; compareEnabledRef.current = compareEnabled; @@ -214,11 +226,22 @@ export function useColorGradingController({ clearTimeout(persistTimerRef.current); persistTimerRef.current = null; } + // Flush — don't discard — a still-pending edit for the OUTGOING element. + // Cancelling the debounce without writing would silently drop whatever + // 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), + ); + } pendingPersistValueRef.current = undefined; + pendingPersistGradingRef.current = null; for (const timer of statusTimersRef.current) clearTimeout(timer); statusTimersRef.current = []; const freshGrading = readColorGradingFromElement(element); latestGradingRef.current = freshGrading; + confirmedGradingRef.current = freshGrading; setGrading(freshGrading); setCompareEnabled(false); compareEnabledRef.current = false; @@ -300,11 +323,30 @@ export function useColorGradingController({ refreshRuntimeStatus(); }, [refreshRuntimeStatus]); - const persistColorGradingValue = useCallback((value: string | null) => { - return trackStudioPendingEdit( - onSetAttributeLiveRef.current(COLOR_GRADING_DATA_KEY, value ?? null), - ); - }, []); + const persistColorGradingValue = useCallback( + (value: string | null, attemptedGrading: NormalizedHfColorGrading) => { + const result = onSetAttributeLiveRef.current(COLOR_GRADING_DATA_KEY, value ?? null); + 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" }); + }, + ), + ); + }, + [], + ); const flushPendingPersist = useCallback(() => { if (persistTimerRef.current) { @@ -313,8 +355,10 @@ export function useColorGradingController({ } if (pendingPersistValueRef.current === undefined) return undefined; const value = pendingPersistValueRef.current; + const attemptedGrading = pendingPersistGradingRef.current ?? latestGradingRef.current; pendingPersistValueRef.current = undefined; - return persistColorGradingValue(value); + pendingPersistGradingRef.current = null; + return persistColorGradingValue(value, attemptedGrading); }, [persistColorGradingValue]); useEffect(() => addStudioPendingEditFlushListener(flushPendingPersist), [flushPendingPersist]); @@ -399,11 +443,14 @@ export function useColorGradingController({ pendingPersistValueRef.current = isHfColorGradingActive(nextGrading) ? serializeHfColorGrading(nextGrading) : null; + pendingPersistGradingRef.current = nextGrading; 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); + void persistColorGradingValue(value ?? null, attemptedGrading); }, 350); }, [persistColorGradingValue, postColorGrading, postCompare, scheduleRuntimeStatusRefresh],