From fc70d5f3310b25d7088d351375d1824202814f16 Mon Sep 17 00:00:00 2001
From: Vance Ingalls <vance@heygen.com>
Date: Tue, 14 Jul 2026 12:55:28 -0700
Subject: [PATCH] fix(studio): guard slider release reentrancy, scope Grade
 persist to schedule-time callback
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Fixes the four blockers from the #2416 re-review at head d6a40c38b:

- FlatSlider's onPointerUp calls releasePointerCapture() explicitly, which
  fires lostpointercapture SYNCHRONOUSLY in real browsers — the prior
  unconditional onLostPointerCapture resync ran mid-onPointerUp, flipping
  draggingRef false before onPointerUp's own check, silently dropping every
  normal drag-release's final commitDraft(). happy-dom doesn't replicate the
  synchronous cascade, so this shipped without a failing test. Added an
  explicitReleaseRef flag set right before each deliberate
  releasePointerCapture() call so onLostPointerCapture can tell "our own
  release, caller's logic already handles it" apart from a genuine external
  capture loss. Added a regression test that monkey-patches
  releasePointerCapture to reproduce the real-browser ordering.

- persistColorGradingValue read onSetAttributeLiveRef.current (reassigned
  every render) instead of the callback live when the debounced edit was
  scheduled — a timer for element A firing after a re-render for element B
  would wrongly call B's callback with A's data. Removed the ref; the
  callback is now an explicit parameter captured by commitColorGrading's own
  closure (added to its useCallback deps) and threaded through to
  persistColorGradingValue and flushPendingPersist.

- flushPendingPersist passed () => true as its isLatestAttempt checker,
  bypassing the per-commit version guard entirely. Now calls
  bumpDomEditCommitVersion(gradingVersionRef) like a regular debounced
  commit, so a newer edit landing before the flushed write settles still
  wins the race.

- The selection-identity cleanup effect stopped clearing statusTimersRef
  during an earlier refactor — stale RUNTIME_STATUS_REFRESH_DELAYS timers
  for an outgoing element could fire after switching selection and stamp
  the new element's runtimeStatus with the old element's answer. Restored
  the clear in the same effect cleanup.

Also gave the Custom LUT and "Copy grade to" scope <select> controls
aria-labels — both had their visible text in a sibling span/text node, so
neither had an accessible name.

Full studio suite still at the known pre-existing 55-failure baseline
(variablePromoteIntegration, useGsapPropertyDebounce, sdkCutover(Parity),
sdkResolverShadow), zero new regressions. Typecheck, oxlint, oxfmt clean.
---
 ...pertyPanelFlatColorGradingSection.test.tsx |  8 +++
 .../propertyPanelFlatColorGradingSection.tsx  |  2 +
 .../propertyPanelFlatPrimitives.test.tsx      | 44 +++++++++++++++
 .../editor/propertyPanelFlatPrimitives.tsx    | 40 +++++++++++---
 .../editor/useColorGradingController.ts       | 55 ++++++++++++++-----
 5 files changed, 126 insertions(+), 23 deletions(-)

diff --git a/packages/studio/src/components/editor/propertyPanelFlatColorGradingSection.test.tsx b/packages/studio/src/components/editor/propertyPanelFlatColorGradingSection.test.tsx
index 9899dcd9d..db615065e 100644
--- a/packages/studio/src/components/editor/propertyPanelFlatColorGradingSection.test.tsx
+++ b/packages/studio/src/components/editor/propertyPanelFlatColorGradingSection.test.tsx
@@ -278,6 +278,7 @@ describe("FlatColorGradingSection — Preset + LUT", () => {
     act(() => lutToggle?.dispatchEvent(new MouseEvent("click", { bubbles: true })));
     const lutSelect = host.querySelector<HTMLSelectElement>('[data-flat-grade-lut-select="true"]');
     if (!lutSelect) throw new Error("expected a LUT catalog select");
+    expect(lutSelect.getAttribute("aria-label")).toBe("Custom LUT");
     act(() => {
       lutSelect.value = "assets/luts/cool.cube";
       lutSelect.dispatchEvent(new Event("change", { bubbles: true }));
@@ -611,4 +612,11 @@ describe("FlatColorGradingSection — HDR banner and Apply scope", () => {
     expect(onApplyToScope).toHaveBeenCalledTimes(1);
     act(() => root.unmount());
   });
+
+  it("gives the Copy-grade-to scope select an accessible name — the visible text sits in a sibling span, not a <label>", () => {
+    const { host, root } = renderInto(<FlatColorGradingSection {...neutralPropsBase()} />);
+    const scopeSelect = host.querySelector<HTMLSelectElement>('[aria-label="Copy grade to"]');
+    expect(scopeSelect).not.toBeNull();
+    act(() => root.unmount());
+  });
 });
diff --git a/packages/studio/src/components/editor/propertyPanelFlatColorGradingSection.tsx b/packages/studio/src/components/editor/propertyPanelFlatColorGradingSection.tsx
index 80c654759..ed42e6d3e 100644
--- a/packages/studio/src/components/editor/propertyPanelFlatColorGradingSection.tsx
+++ b/packages/studio/src/components/editor/propertyPanelFlatColorGradingSection.tsx
@@ -359,6 +359,7 @@ export function FlatColorGradingSection({
               </span>
               <select
                 data-flat-grade-lut-select="true"
+                aria-label="Custom LUT"
                 value={lut?.src ?? ""}
                 onChange={(e) => {
                   const src = e.target.value;
@@ -525,6 +526,7 @@ export function FlatColorGradingSection({
           <span className="flex items-center gap-1.5 text-[11px] text-panel-text-2">
             Copy grade to
             <select
+              aria-label="Copy grade to"
               value={applyScope}
               onChange={(e) => onSetApplyScope(e.target.value as "source-file" | "project")}
               disabled={applyBusy}
diff --git a/packages/studio/src/components/editor/propertyPanelFlatPrimitives.test.tsx b/packages/studio/src/components/editor/propertyPanelFlatPrimitives.test.tsx
index ed30efc24..9346704b8 100644
--- a/packages/studio/src/components/editor/propertyPanelFlatPrimitives.test.tsx
+++ b/packages/studio/src/components/editor/propertyPanelFlatPrimitives.test.tsx
@@ -429,6 +429,50 @@ describe("FlatSlider", () => {
     expect(onCommit).not.toHaveBeenCalled();
     act(() => root.unmount());
   });
+
+  it("still commits the release position when releasePointerCapture synchronously fires lostpointercapture (real-browser behavior happy-dom doesn't replicate)", () => {
+    const onCommit = vi.fn();
+    const { host, root } = renderInto(
+      <FlatSlider
+        label="Opacity"
+        value={10}
+        min={0}
+        max={100}
+        tier="explicitCustom"
+        displayValue="10%"
+        onCommit={onCommit}
+      />,
+    );
+    const track = host.querySelector<HTMLElement>('[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 }),
+    });
+    // Real browsers fire lostpointercapture SYNCHRONOUSLY, mid-call, when
+    // releasePointerCapture() is invoked — happy-dom does not replicate this,
+    // so patch it in to reproduce the exact reentrancy hazard onPointerUp
+    // must guard against.
+    const originalRelease = track.releasePointerCapture.bind(track);
+    track.releasePointerCapture = (pointerId: number) => {
+      originalRelease(pointerId);
+      track.dispatchEvent(new Event("lostpointercapture", { bubbles: true }));
+    };
+    act(() => {
+      track.dispatchEvent(
+        new PointerEvent("pointerdown", { bubbles: true, clientX: 30, pointerId: 1 }),
+      );
+    });
+    act(() => {
+      track.dispatchEvent(
+        new PointerEvent("pointerup", { bubbles: true, clientX: 80, pointerId: 1 }),
+      );
+    });
+    // The real release position (80), not a rollback to the pre-drag value (10)
+    // caused by onLostPointerCapture resyncing mid-handler.
+    expect(onCommit).toHaveBeenLastCalledWith(80);
+    expect(track.getAttribute("aria-valuenow")).toBe("80");
+    act(() => root.unmount());
+  });
 });
 
 describe("FlatSlider — Grade extensions", () => {
diff --git a/packages/studio/src/components/editor/propertyPanelFlatPrimitives.tsx b/packages/studio/src/components/editor/propertyPanelFlatPrimitives.tsx
index 3b463002d..21572a51d 100644
--- a/packages/studio/src/components/editor/propertyPanelFlatPrimitives.tsx
+++ b/packages/studio/src/components/editor/propertyPanelFlatPrimitives.tsx
@@ -305,6 +305,19 @@ export function FlatSlider({
   // depend on ordering between the native event and the [value] effect.
   const latestValueRef = useRef(value);
   latestValueRef.current = value;
+  // releasePointerCapture() (called explicitly below in onPointerUp/
+  // onPointerCancel) fires lostpointercapture SYNCHRONOUSLY in real
+  // browsers — i.e. onLostPointerCapture runs mid-onPointerUp, BEFORE
+  // onPointerUp's own draggingRef check and final commitDraft. Without this
+  // flag, a NORMAL release would have onLostPointerCapture reset
+  // draggingRef/draft to the stale value first, making onPointerUp's own
+  // "if (!draggingRef.current) return" bail out and silently drop the
+  // real final-position commit. Set right before each explicit release
+  // call so onLostPointerCapture can tell "our own release, the caller's
+  // own logic already handles it" apart from a genuine EXTERNAL capture
+  // loss (another element steals it, or the browser reclaims it for a
+  // scroll/touch gesture) where no other handler is about to run.
+  const explicitReleaseRef = useRef(false);
 
   useEffect(() => {
     if (draggingRef.current) return;
@@ -390,6 +403,7 @@ export function FlatSlider({
         }}
         onPointerUp={(e) => {
           if (e.currentTarget.hasPointerCapture(e.pointerId)) {
+            explicitReleaseRef.current = true;
             e.currentTarget.releasePointerCapture(e.pointerId);
           }
           if (disabled) return;
@@ -406,19 +420,27 @@ export function FlatSlider({
         onPointerCancel={(e) => {
           draggingRef.current = false;
           if (e.currentTarget.hasPointerCapture(e.pointerId)) {
+            explicitReleaseRef.current = true;
             e.currentTarget.releasePointerCapture(e.pointerId);
           }
         }}
         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). 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.
+          if (explicitReleaseRef.current) {
+            // Our own onPointerUp/onPointerCancel just released capture —
+            // their own logic already handles (or intentionally leaves)
+            // draggingRef/draft correctly. Resyncing here too would race
+            // onPointerUp's still-pending final commitDraft(stepped) below
+            // this call, since draggingRef flipping false would make its
+            // own "if (!draggingRef.current) return" bail out first.
+            explicitReleaseRef.current = false;
+            return;
+          }
+          // A genuine EXTERNAL capture loss (another element steals it, or
+          // the browser reclaims it for a scroll/touch gesture) — no other
+          // handler is about to run, so 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).
           draggingRef.current = false;
           setDraft(latestValueRef.current);
           lastCommittedRef.current = latestValueRef.current;
diff --git a/packages/studio/src/components/editor/useColorGradingController.ts b/packages/studio/src/components/editor/useColorGradingController.ts
index 38aa939cf..19921f9f1 100644
--- a/packages/studio/src/components/editor/useColorGradingController.ts
+++ b/packages/studio/src/components/editor/useColorGradingController.ts
@@ -207,10 +207,8 @@ export function useColorGradingController({
   // out from under B's newer optimistic state.
   const gradingVersionRef = useRef(0);
   const statusTimersRef = useRef<number[]>([]);
-  const onSetAttributeLiveRef = useRef(onSetAttributeLive);
   const latestGradingRef = useRef(grading);
   const compareEnabledRef = useRef(compareEnabled);
-  onSetAttributeLiveRef.current = onSetAttributeLive;
   latestGradingRef.current = grading;
   compareEnabledRef.current = compareEnabled;
 
@@ -258,6 +256,12 @@ export function useColorGradingController({
   // instance (for the NEW identity) is established.
   useEffect(() => {
     return () => {
+      // Stale runtime-status timers scheduled for the OUTGOING element must
+      // not fire after this: readRuntimeColorGradingStatus closes over
+      // `target`, so an old timer firing post-switch would stamp the NEW
+      // element's runtimeStatus with the OLD element's answer.
+      for (const timer of statusTimersRef.current) clearTimeout(timer);
+      statusTimersRef.current = [];
       if (persistTimerRef.current) {
         clearTimeout(persistTimerRef.current);
         persistTimerRef.current = null;
@@ -349,6 +353,11 @@ export function useColorGradingController({
       attemptedGrading: NormalizedHfColorGrading,
       attemptIdentityKey: string,
       isLatestAttempt: () => boolean,
+      setAttributeLive: (
+        attr: string,
+        value: string | null,
+        onSettled?: (ok: boolean) => void,
+      ) => void | Promise<void>,
     ) => {
       // Two guards, not one: identity (selection moved to a DIFFERENT
       // element — that element already got its own confirmedGradingRef
@@ -378,11 +387,12 @@ export function useColorGradingController({
       // 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,
-      );
+      // `setAttributeLive` is passed in by the caller rather than read from a
+      // ref: a debounced persist for element A must call the callback that
+      // was live when A's edit was SCHEDULED, not whatever the ref holds by
+      // the time the timer fires — a "latest" ref would let a stale timer
+      // wrongly target a since-selected element B's callback.
+      const result = setAttributeLive(COLOR_GRADING_DATA_KEY, value ?? null, applySettled);
       return trackStudioPendingEdit(
         Promise.resolve(result).then(
           () => undefined,
@@ -403,12 +413,22 @@ export function useColorGradingController({
     const attemptedGrading = pendingPersistGradingRef.current ?? latestGradingRef.current;
     pendingPersistValueRef.current = undefined;
     pendingPersistGradingRef.current = null;
-    // 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]);
+    // A flush cancels the pending debounce timer above, so this becomes the
+    // one-and-only in-flight attempt for this element — bump the version so
+    // it still registers as "the latest attempt" against the same guard a
+    // regular debounced commit uses, instead of unconditionally claiming
+    // that title. Without this, a newer edit landing after the flush starts
+    // but before it settles would have its own eventual settle silently
+    // lose the version race to this unconditionally-"latest" flush.
+    const isLatestAttempt = bumpDomEditCommitVersion(gradingVersionRef);
+    return persistColorGradingValue(
+      value,
+      attemptedGrading,
+      identityKeyRef.current,
+      isLatestAttempt,
+      onSetAttributeLive,
+    );
+  }, [onSetAttributeLive, persistColorGradingValue]);
 
   useEffect(() => addStudioPendingEditFlushListener(flushPendingPersist), [flushPendingPersist]);
 
@@ -513,10 +533,17 @@ export function useColorGradingController({
           attemptedGrading,
           attemptIdentityKey,
           isLatestAttempt,
+          onSetAttributeLive,
         );
       }, 350);
     },
-    [persistColorGradingValue, postColorGrading, postCompare, scheduleRuntimeStatusRefresh],
+    [
+      onSetAttributeLive,
+      persistColorGradingValue,
+      postColorGrading,
+      postCompare,
+      scheduleRuntimeStatusRefresh,
+    ],
   );
 
   const commitCompare = useCallback(