mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-09-03 04:38:33 +00:00
fix(studio): guard slider release reentrancy, scope Grade persist to schedule-time callback
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.
This commit is contained in:
@@ -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());
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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}
|
||||
|
||||
@@ -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", () => {
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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(
|
||||
|
||||
Reference in New Issue
Block a user