diff --git a/packages/studio/src/components/editor/propertyPanelFlatColorGradingSection.test.tsx b/packages/studio/src/components/editor/propertyPanelFlatColorGradingSection.test.tsx index bf128ca0a..9899dcd9d 100644 --- a/packages/studio/src/components/editor/propertyPanelFlatColorGradingSection.test.tsx +++ b/packages/studio/src/components/editor/propertyPanelFlatColorGradingSection.test.tsx @@ -238,6 +238,10 @@ describe("FlatColorGradingSection — Preset + LUT", () => { ); if (!presetSelect) throw new Error("expected a preset select"); expect(presetSelect.value).toBe("neutral"); + // The visible "Preset" label is a sibling span outside FlatSelectRow + // (label="" there, to avoid rendering it twice) — the select still + // needs its own accessible name via the dedicated ariaLabel prop. + expect(presetSelect.getAttribute("aria-label")).toBe("Preset"); act(() => { presetSelect.value = "fresh-pop"; presetSelect.dispatchEvent(new Event("change", { bubbles: true })); diff --git a/packages/studio/src/components/editor/propertyPanelFlatColorGradingSection.tsx b/packages/studio/src/components/editor/propertyPanelFlatColorGradingSection.tsx index 3018bcf1f..80c654759 100644 --- a/packages/studio/src/components/editor/propertyPanelFlatColorGradingSection.tsx +++ b/packages/studio/src/components/editor/propertyPanelFlatColorGradingSection.tsx @@ -312,6 +312,7 @@ export function FlatColorGradingSection({ Preset { expect(track.getAttribute("aria-valuenow")).toBe("99"); act(() => root.unmount()); }); + + it("resyncs immediately from the latest value on lostpointercapture, even when the value changed WHILE still dragging", () => { + const { host, root } = renderInto( + , + ); + const track = host.querySelector('[data-flat-slider-track="true"]'); + if (!track) throw new Error("expected a track element"); + Object.defineProperty(track, "getBoundingClientRect", { + value: () => ({ left: 0, width: 100, top: 0, height: 20, right: 100, bottom: 20 }), + }); + act(() => { + track.dispatchEvent( + new PointerEvent("pointerdown", { bubbles: true, clientX: 30, pointerId: 1 }), + ); + }); + expect(track.getAttribute("aria-valuenow")).toBe("30"); + // Value changes to 99 WHILE still dragging — the [value] sync effect + // must skip it (draggingRef is still true), so draft stays at 30. + act(() => { + root.render( + , + ); + }); + expect(track.getAttribute("aria-valuenow")).toBe("30"); + act(() => { + // Capture lost with NO further render afterward — if the resync + // depended on a subsequent [value] effect run rather than reading + // latestValueRef directly, this would leave the knob stuck at 30. + track.dispatchEvent(new Event("lostpointercapture", { bubbles: true })); + }); + expect(track.getAttribute("aria-valuenow")).toBe("99"); + act(() => root.unmount()); + }); }); describe("FlatSelectRow", () => { @@ -967,44 +1015,3 @@ describe("FlatSelectRow — label/value options", () => { act(() => root.unmount()); }); }); - -describe("FlatToggle", () => { - it("renders the off state with a dim label and dim knob, and fires onChange(true) on click", () => { - const onChange = vi.fn(); - const { host, root } = renderInto( - , - ); - const label = host.querySelector('[data-flat-toggle-label="true"]'); - expect(label?.className).toContain("text-panel-text-3"); - const pill = host.querySelector('[data-flat-toggle="true"]'); - expect(pill).not.toBeNull(); - act(() => pill?.dispatchEvent(new MouseEvent("click", { bubbles: true }))); - expect(onChange).toHaveBeenCalledWith(true); - act(() => root.unmount()); - }); - - it("renders the on state with an emphasized label and mint knob, and fires onChange(false) on click", () => { - const onChange = vi.fn(); - const { host, root } = renderInto(); - const label = host.querySelector('[data-flat-toggle-label="true"]'); - expect(label?.className).toContain("text-panel-text-2"); - const knob = host.querySelector('[data-flat-toggle-knob="true"]'); - expect(knob?.className).toContain("bg-panel-accent"); - const pill = host.querySelector('[data-flat-toggle="true"]'); - act(() => pill?.dispatchEvent(new MouseEvent("click", { bubbles: true }))); - expect(onChange).toHaveBeenCalledWith(false); - act(() => root.unmount()); - }); - - it("does not fire onChange when disabled", () => { - const onChange = vi.fn(); - const { host, root } = renderInto( - , - ); - const pill = host.querySelector('[data-flat-toggle="true"]'); - expect(pill?.disabled).toBe(true); - act(() => pill?.dispatchEvent(new MouseEvent("click", { bubbles: true }))); - expect(onChange).not.toHaveBeenCalled(); - act(() => root.unmount()); - }); -}); diff --git a/packages/studio/src/components/editor/propertyPanelFlatPrimitives.tsx b/packages/studio/src/components/editor/propertyPanelFlatPrimitives.tsx index 7e5afca27..3b463002d 100644 --- a/packages/studio/src/components/editor/propertyPanelFlatPrimitives.tsx +++ b/packages/studio/src/components/editor/propertyPanelFlatPrimitives.tsx @@ -300,6 +300,11 @@ export function FlatSlider({ // different control in between. const onCommitRef = useRef(onCommit); onCommitRef.current = onCommit; + // Always this render's committed value — read directly (not via the + // effect below) by onLostPointerCapture, so the resync there doesn't + // depend on ordering between the native event and the [value] effect. + const latestValueRef = useRef(value); + latestValueRef.current = value; useEffect(() => { if (draggingRef.current) return; @@ -407,10 +412,16 @@ export function FlatSlider({ onLostPointerCapture={() => { // Capture can be lost without either pointerup or pointercancel // firing first (e.g. another element steals it, or the browser - // reclaims it for a scroll/touch gesture) — without this, - // draggingRef stays stuck true and the knob permanently stops - // syncing to the committed value prop. + // reclaims it for a scroll/touch gesture). Resync immediately and + // directly from latestValueRef, rather than only clearing + // draggingRef and waiting for the [value] effect to notice — + // that effect depends on `value` actually changing again to + // re-run, so if this event and any concurrent value update are + // ordered unfavorably, the knob could otherwise stay stuck at + // its mid-drag position indefinitely. draggingRef.current = false; + setDraft(latestValueRef.current); + lastCommittedRef.current = latestValueRef.current; }} onKeyDown={(e) => { if (disabled) return; @@ -478,6 +489,7 @@ export function FlatSlider({ export function FlatSelectRow({ label, + ariaLabel, value, options, tier, @@ -486,6 +498,11 @@ export function FlatSelectRow({ onReset, }: { label: string; + /** Accessible name when a caller renders the visible label OUTSIDE this + * row (label="" to avoid a duplicate) — e.g. Grade's "Preset" row, which + * shows its own label span and would otherwise leave the 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]}`} > @@ -551,49 +568,3 @@ export function FlatSelectRow({ ); } - -/* ------------------------------------------------------------------ */ -/* FlatToggle — 24×14 pill switch */ -/* ------------------------------------------------------------------ */ - -export function FlatToggle({ - label, - checked, - disabled, - onChange, -}: { - label: string; - checked: boolean; - disabled?: boolean; - onChange: (next: boolean) => void; -}) { - return ( -
- - {label} - - -
- ); -} diff --git a/packages/studio/src/components/editor/propertyPanelFlatToggle.test.tsx b/packages/studio/src/components/editor/propertyPanelFlatToggle.test.tsx new file mode 100644 index 000000000..e16956937 --- /dev/null +++ b/packages/studio/src/components/editor/propertyPanelFlatToggle.test.tsx @@ -0,0 +1,63 @@ +// @vitest-environment happy-dom + +import React, { act } from "react"; +import { createRoot } from "react-dom/client"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { FlatToggle } from "./propertyPanelFlatToggle"; + +(globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + +afterEach(() => { + document.body.innerHTML = ""; +}); + +function renderInto(node: React.ReactElement) { + const host = document.createElement("div"); + document.body.append(host); + const root = createRoot(host); + act(() => { + root.render(node); + }); + return { host, root }; +} + +describe("FlatToggle", () => { + it("renders the off state with a dim label and dim knob, and fires onChange(true) on click", () => { + const onChange = vi.fn(); + const { host, root } = renderInto( + , + ); + const label = host.querySelector('[data-flat-toggle-label="true"]'); + expect(label?.className).toContain("text-panel-text-3"); + const pill = host.querySelector('[data-flat-toggle="true"]'); + expect(pill).not.toBeNull(); + act(() => pill?.dispatchEvent(new MouseEvent("click", { bubbles: true }))); + expect(onChange).toHaveBeenCalledWith(true); + act(() => root.unmount()); + }); + + it("renders the on state with an emphasized label and mint knob, and fires onChange(false) on click", () => { + const onChange = vi.fn(); + const { host, root } = renderInto(); + const label = host.querySelector('[data-flat-toggle-label="true"]'); + expect(label?.className).toContain("text-panel-text-2"); + const knob = host.querySelector('[data-flat-toggle-knob="true"]'); + expect(knob?.className).toContain("bg-panel-accent"); + const pill = host.querySelector('[data-flat-toggle="true"]'); + act(() => pill?.dispatchEvent(new MouseEvent("click", { bubbles: true }))); + expect(onChange).toHaveBeenCalledWith(false); + act(() => root.unmount()); + }); + + it("does not fire onChange when disabled", () => { + const onChange = vi.fn(); + const { host, root } = renderInto( + , + ); + const pill = host.querySelector('[data-flat-toggle="true"]'); + expect(pill?.disabled).toBe(true); + act(() => pill?.dispatchEvent(new MouseEvent("click", { bubbles: true }))); + expect(onChange).not.toHaveBeenCalled(); + act(() => root.unmount()); + }); +}); diff --git a/packages/studio/src/components/editor/propertyPanelFlatToggle.tsx b/packages/studio/src/components/editor/propertyPanelFlatToggle.tsx new file mode 100644 index 000000000..509430646 --- /dev/null +++ b/packages/studio/src/components/editor/propertyPanelFlatToggle.tsx @@ -0,0 +1,47 @@ +/* ------------------------------------------------------------------ */ +/* FlatToggle — 24×14 pill switch */ +/* (split out of propertyPanelFlatPrimitives.tsx to stay under the */ +/* 600-line file-size gate) */ +/* ------------------------------------------------------------------ */ + +export function FlatToggle({ + label, + checked, + disabled, + onChange, +}: { + label: string; + checked: boolean; + disabled?: boolean; + onChange: (next: boolean) => void; +}) { + return ( +
+ + {label} + + +
+ ); +} diff --git a/packages/studio/src/components/editor/useColorGradingController.test.ts b/packages/studio/src/components/editor/useColorGradingController.test.ts index a871ca15c..5e74587e6 100644 --- a/packages/studio/src/components/editor/useColorGradingController.test.ts +++ b/packages/studio/src/components/editor/useColorGradingController.test.ts @@ -13,6 +13,12 @@ function freshPopGrading() { return next; } +function naturalLiftGrading() { + const next = normalizeHfColorGrading({ preset: "natural-lift", intensity: 1 }); + if (!next) throw new Error("expected natural-lift preset to normalize"); + return next; +} + (globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true; afterEach(() => { @@ -231,6 +237,66 @@ describe("useColorGradingController", () => { vi.useRealTimers(); }); + it("a stale in-flight persist for edit A does not clobber edit B's state — SAME element, no selection change", async () => { + vi.useFakeTimers(); + let resolveA: (() => void) | undefined; + let capturedOnSettledA: ((ok: boolean) => void) | undefined; + const onSetAttributeLive = vi + .fn() + // Edit A (fresh-pop): captures its onSettled and never resolves until + // resolveA() is called below — simulates a slow persist. + .mockImplementationOnce( + (_attr: string, _value: string | null, onSettled?: (ok: boolean) => void) => { + capturedOnSettledA = onSettled; + return new Promise((resolve) => { + resolveA = resolve; + }); + }, + ) + // Edit B (natural-lift): settles immediately and successfully. + .mockImplementationOnce( + (_attr: string, _value: string | null, onSettled?: (ok: boolean) => void) => { + onSettled?.(true); + return Promise.resolve(); + }, + ); + const { root, getState } = renderHook(onSetAttributeLive); + + act(() => { + getState().commitColorGrading(freshPopGrading()); + }); + act(() => { + vi.advanceTimersByTime(400); + }); + expect(onSetAttributeLive).toHaveBeenCalledTimes(1); // A's persist now in flight + + // B commits on the SAME element before A's persist has settled. + act(() => { + getState().commitColorGrading(naturalLiftGrading()); + }); + act(() => { + vi.advanceTimersByTime(400); + }); + expect(onSetAttributeLive).toHaveBeenCalledTimes(2); // B's persist has already settled (mock resolves sync) + expect(getState().grading.preset).toBe("natural-lift"); + + // NOW A's stale persist finally settles as a FAILURE — must not revert + // `grading` (which now correctly shows B's newer edit) back to the + // pre-A baseline ("neutral"), and must not stamp confirmedGradingRef + // with A's now-superseded attempt on success either. + act(() => { + capturedOnSettledA?.(false); + resolveA?.(); + }); + await act(async () => { + await Promise.resolve(); + await Promise.resolve(); + }); + expect(getState().grading.preset).toBe("natural-lift"); + 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 ebbcefbaf..38aa939cf 100644 --- a/packages/studio/src/components/editor/useColorGradingController.ts +++ b/packages/studio/src/components/editor/useColorGradingController.ts @@ -13,6 +13,7 @@ import { } from "../../utils/studioPendingEdits"; import type { DomEditSelection } from "./domEditing"; import { selectionIdentityKey, stripQueryAndHash } from "./propertyPanelHelpers"; +import { bumpDomEditCommitVersion } from "../../hooks/domEditCommitRunner"; import { acceptStudioRuntimeMessage, postRuntimeControlMessage, @@ -194,65 +195,45 @@ 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 // that was never written. const confirmedGradingRef = useRef(grading); + // Monotonic per-commit version — guards against TWO edits on the SAME + // element racing (not just a selection change). If edit A's persist is + // still in flight when edit B commits, A's eventual settle must not stamp + // confirmedGradingRef with its now-superseded value or revert `grading` + // out from under B's newer optimistic state. + const gradingVersionRef = useRef(0); 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; - // Reset all per-element state when the selection changes to a different + // Reset all per-element STATE when the selection changes to a different // element — unlike the legacy ColorGradingSection (remounted via a // `key={selectionIdentityKey(element)}` from its parent), this hook is // called unconditionally on every render, so nothing naturally remounts it. // 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 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. + // compare/mediaMetadata state. Adjusting state during render (comparing + // against a ref) is React's documented pattern for STATE updates + // specifically — it resolves in the same render pass instead of flashing + // stale state for one frame, and is safe to repeat if React discards and + // re-runs this render, since every value here is a pure function of + // `element`. It must NOT be used for side effects or for consuming + // shared mutable state (like the pending-persist timer/value) — a + // discarded render would have already consumed them with no corresponding + // effect ever running to compensate. That part happens below, in an + // effect's cleanup, which is guaranteed to run only for a render that + // actually committed. const identityKey = selectionIdentityKey(element); const identityKeyRef = useRef(identityKey); if (identityKeyRef.current !== identityKey) { identityKeyRef.current = identityKey; - if (persistTimerRef.current) { - 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) { - queuedOutgoingFlushRef.current = { - setAttributeLive: previousOnSetAttributeLive, - value: 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; @@ -265,16 +246,29 @@ 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). + // Flushes — never discards — a still-pending edit when selection moves to + // a different element, and cancels the debounce timer. Implemented as an + // effect CLEANUP (not the render-phase block above, and not a queued ref + // consumed by a separate effect): a cleanup only ever runs for the + // specific effect instance that actually committed for `identityKey`, so + // there's no window where a discarded/interrupted render could have + // already consumed pendingPersistValueRef without this ever running to + // compensate. The cleanup's closure captures onSetAttributeLive/target + // bound to the OUTGOING identity, since it runs before the next effect + // instance (for the NEW identity) is established. useEffect(() => { - const queued = queuedOutgoingFlushRef.current; - if (!queued) return; - queuedOutgoingFlushRef.current = null; - trackStudioPendingEdit(queued.setAttributeLive(COLOR_GRADING_DATA_KEY, queued.value)); + return () => { + if (persistTimerRef.current) { + clearTimeout(persistTimerRef.current); + persistTimerRef.current = null; + } + if (pendingPersistValueRef.current === undefined) return; + const value = pendingPersistValueRef.current; + pendingPersistValueRef.current = undefined; + pendingPersistGradingRef.current = null; + trackStudioPendingEdit(onSetAttributeLive(COLOR_GRADING_DATA_KEY, value)); + }; + // eslint-disable-next-line react-hooks/exhaustive-deps -- identityKey is the intended trigger; see comment above }, [identityKey]); const target = useMemo( @@ -354,13 +348,18 @@ export function useColorGradingController({ value: string | null, attemptedGrading: NormalizedHfColorGrading, attemptIdentityKey: string, + isLatestAttempt: () => boolean, ) => { - // 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. + // Two guards, not one: identity (selection moved to a DIFFERENT + // element — that element already got its own confirmedGradingRef + // baseline from the reset block) and version (a NEWER edit landed on + // the SAME element — e.g. the user dragged Exposure, then Contrast, + // before Exposure's persist settled; Exposure settling afterward must + // not stamp confirmedGradingRef with its now-superseded value or + // revert `grading` out from under Contrast's newer optimistic state). const applySettled = (ok: boolean) => { if (identityKeyRef.current !== attemptIdentityKey) return; + if (!isLatestAttempt()) return; if (ok) { confirmedGradingRef.current = attemptedGrading; return; @@ -404,7 +403,11 @@ export function useColorGradingController({ const attemptedGrading = pendingPersistGradingRef.current ?? latestGradingRef.current; pendingPersistValueRef.current = undefined; pendingPersistGradingRef.current = null; - return persistColorGradingValue(value, attemptedGrading, identityKeyRef.current); + // A direct flush (unmount / explicit "flush all pending edits") reads + // pendingPersistValueRef synchronously right now, not a stored version + // from an earlier commit — there's nothing else it could be racing + // against, so it's trivially "the latest attempt" by construction. + return persistColorGradingValue(value, attemptedGrading, identityKeyRef.current, () => true); }, [persistColorGradingValue]); useEffect(() => addStudioPendingEditFlushListener(flushPendingPersist), [flushPendingPersist]); @@ -495,13 +498,22 @@ export function useColorGradingController({ // moved on, at which point identityKeyRef.current would no longer // describe the element this edit was actually made for. const attemptIdentityKey = identityKeyRef.current; + // Bumps a monotonic version and hands back a checker bound to THIS + // specific commit — reused from the same primitive the DOM-attribute + // commit runner uses for the identical same-target-rapid-edits race. + const isLatestAttempt = bumpDomEditCommitVersion(gradingVersionRef); 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, attemptIdentityKey); + void persistColorGradingValue( + value ?? null, + attemptedGrading, + attemptIdentityKey, + isLatestAttempt, + ); }, 350); }, [persistColorGradingValue, postColorGrading, postCompare, scheduleRuntimeStatusRefresh],