fix(studio): resolve the 4 cumulative blockers from the #2416 tip re-review

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 <select>
  an aria-label from the row's label text.
- FlatSlider: handle lostpointercapture the same as pointercancel — capture
  can be lost without either firing first (another element steals it, or
  the browser reclaims it for a scroll/touch gesture), which previously
  left the dragging flag stuck and the knob permanently unable to sync to
  external value changes.

New regression tests for all of the above; full studio suite still at the
known pre-existing baseline (55 failures unrelated to this stack).
This commit is contained in:
Vance Ingalls
2026-07-14 16:28:33 -07:00
parent 5dd9efe555
commit 1deb0dc970
5 changed files with 196 additions and 15 deletions
@@ -774,6 +774,54 @@ describe("FlatSlider — Grade extensions", () => {
expect(track.getAttribute("aria-valuenow")).toBe("80");
act(() => root.unmount());
});
it("resets the dragging state on lostpointercapture even without a prior pointerup/pointercancel", () => {
const { host, root } = renderInto(
<FlatSlider
label="Opacity"
value={10}
min={0}
max={100}
tier="explicitCustom"
displayValue="10%"
onCommit={vi.fn()}
/>,
);
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 }),
});
act(() => {
track.dispatchEvent(
new PointerEvent("pointerdown", { bubbles: true, clientX: 30, pointerId: 1 }),
);
});
expect(track.getAttribute("aria-valuenow")).toBe("30");
act(() => {
// Capture lost WITHOUT a pointerup/pointercancel first — e.g. another
// element steals it, or the browser reclaims it for a scroll gesture.
track.dispatchEvent(new Event("lostpointercapture", { bubbles: true }));
});
act(() => {
root.render(
<FlatSlider
label="Opacity"
value={99}
min={0}
max={100}
tier="explicitCustom"
displayValue="99%"
onCommit={vi.fn()}
/>,
);
});
// If lostpointercapture hadn't cleared the dragging flag, this external
// value change would be silently ignored (mid-drag echo suppression)
// forever — the knob would be stuck at 30.
expect(track.getAttribute("aria-valuenow")).toBe("99");
act(() => root.unmount());
});
});
describe("FlatSelectRow", () => {
@@ -813,6 +861,28 @@ describe("FlatSelectRow", () => {
act(() => root.unmount());
});
it("disables the reset button (and gives the select an accessible name) when the row itself is disabled", () => {
const onReset = vi.fn();
const { host, root } = renderInto(
<FlatSelectRow
label="Shadow"
value="soft"
options={["none", "soft", "lift", "glow"]}
tier="explicitCustom"
disabled
onChange={vi.fn()}
onReset={onReset}
/>,
);
const select = host.querySelector<HTMLSelectElement>("select");
expect(select?.getAttribute("aria-label")).toBe("Shadow");
const reset = host.querySelector<HTMLButtonElement>('[data-flat-select-reset="true"]');
expect(reset?.disabled).toBe(true);
act(() => reset?.dispatchEvent(new MouseEvent("click", { bubbles: true })));
expect(onReset).not.toHaveBeenCalled();
act(() => root.unmount());
});
it("fires onChange when the select value changes", () => {
const onChange = vi.fn();
const { host, root } = renderInto(
@@ -404,6 +404,14 @@ export function FlatSlider({
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) — without this,
// draggingRef stays stuck true and the knob permanently stops
// syncing to the committed value prop.
draggingRef.current = false;
}}
onKeyDown={(e) => {
if (disabled) return;
const next = sliderKeyTarget(e.key, draft, min, max, step);
@@ -507,6 +515,7 @@ export function FlatSelectRow({
<select
value={value}
disabled={disabled}
aria-label={label || undefined}
onChange={(e) => 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"
>
<RotateCcw size={11} />
</button>
@@ -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<DomEditSelection, "id" | "hfId" | "selector" | "selectorIndex">,
element: Pick<DomEditSelection, "id" | "hfId" | "selector" | "selectorIndex" | "sourceFile">,
): string {
return [
element.id ?? "",
element.hfId ?? "",
element.selector ?? "",
String(element.selectorIndex ?? ""),
element.sourceFile ?? "",
].join("|");
}
@@ -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();
});
@@ -189,10 +189,22 @@ export function useColorGradingController({
const [mediaMetadata, setMediaMetadata] = useState<MediaMetadata | null>(null);
const persistTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
const pendingPersistValueRef = useRef<string | null | undefined>(undefined);
const pendingPersistGradingRef = useRef<NormalizedHfColorGrading | 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);
const statusTimersRef = useRef<number[]>([]);
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],