fix(studio): resolve 8 confirmed adversarial-review findings across the flat-inspector stack

Fixes issues raised in the Deepwork re-review of #2120-#2190 that weren't
covered by #2225's earlier fix pass:

- useColorGradingController: reset grading/compare/mediaMetadata state (and
  cancel pending persist/status timers) when selection changes to a
  different element — this hook is called unconditionally on every render
  (unlike legacy ColorGradingSection, remounted via a selectionIdentityKey
  React key), so switching selection reused the previous element's state.
- useColorGradingController: stop permanently caching a non-OK
  /media/metadata response as null — a transient server error poisoned the
  HDR banner for that asset for the whole page lifetime.
- FlatSelectRow: preserve a valid authored value outside the preset list
  (e.g. mix-blend-mode: difference, an arbitrary object-position) instead of
  silently misrepresenting it as the first preset — touching the control
  would overwrite real persisted state.
- FlatSlider: the throttled trailing commit now reads onCommit through a
  ref updated every render instead of closing over it at schedule time — a
  caller whose onCommit spreads other current state (Grade's per-detail
  commits) could otherwise have a delayed commit revert whatever the user
  changed on a different control in the same 40ms window.
- FlatSlider: flush a still-queued trailing commit on unmount instead of
  dropping it, and disable the reset button when the slider itself is
  disabled.
- FlatSlider: add touch-action: none to the track so touch drags don't
  compete with page scroll.
- FlatColorGradingAccessory: clean up the compare-hold's window listeners
  on unmount, not only on release — switching selection mid-hold used to
  leak them.
- Align (flat Text): re-clicking the option already visually active for a
  logical start/end value no longer rewrites it to the physical left/right,
  preserving RTL semantics.
- FlatSegmentedRow: give every option an accessible name and aria-pressed
  state — two visually-identical glyph buttons (upright/italic "A") had no
  way to be told apart by assistive tech.
- PropertyPanelFlat: the panel body falls back to its own scroll when the
  collapsed group headers alone exceed the available height, so groups
  can't become permanently unreachable in a short pane.

New regression tests for all of the above; full studio suite 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 2e44a15d19
commit 5dd9efe555
11 changed files with 550 additions and 142 deletions
@@ -516,7 +516,7 @@ export function PropertyPanelFlat({
onUngroup={onUngroup}
showUngroup={Boolean(onUngroup && element.dataAttributes["hf-group"] != null)}
/>
<div data-flat-panel-body="true" className="flex min-h-0 flex-1 flex-col overflow-hidden">
<div data-flat-panel-body="true" className="flex min-h-0 flex-1 flex-col overflow-y-auto">
{beforeOpen.map((g) => (
<FlatGroupHeader
key={g.id}
@@ -1,4 +1,4 @@
import { useMemo, useRef, useState } from "react";
import { useEffect, useMemo, useRef, useState } from "react";
import {
HF_COLOR_GRADING_PRESETS,
isHfColorGradingActive,
@@ -32,6 +32,18 @@ export function FlatColorGradingAccessory({
}) {
const { grading, compareEnabled, runtimeStatus, commitCompare, resetGrading } = state;
const gradingActive = isHfColorGradingActive(grading);
// Tracks the active hold's cleanup so it can be torn down on unmount too —
// without this, switching selection away mid-hold (unmounting this
// accessory) leaves the pointerup/pointercancel/blur listeners registered
// on `window` forever, each holding a closure over the old commitCompare.
const releaseRef = useRef<(() => void) | null>(null);
useEffect(
() => () => {
releaseRef.current?.();
releaseRef.current = null;
},
[],
);
return (
<span className="flex items-center gap-2.5">
@@ -50,7 +62,9 @@ export function FlatColorGradingAccessory({
window.removeEventListener("pointerup", release);
window.removeEventListener("pointercancel", release);
window.removeEventListener("blur", release);
releaseRef.current = null;
};
releaseRef.current = release;
window.addEventListener("pointerup", release);
window.addEventListener("pointercancel", release);
window.addEventListener("blur", release);
@@ -203,8 +203,8 @@ export function LayoutFlexBlock({
<FlatSegmentedRow
label="Direction"
options={[
{ key: "row", node: "→ Row", active: direction === "row" },
{ key: "column", node: "Column", active: direction === "column" },
{ key: "row", node: "→ Row", label: "Row", active: direction === "row" },
{ key: "column", node: "Column", label: "Column", active: direction === "column" },
]}
disabled={disabled}
onChange={(next) => void onSetStyle("flex-direction", next)}
@@ -0,0 +1,102 @@
import {
buildInsetClipPathSides,
buildInsetClipPathValue,
formatNumericValue,
formatPxMetricValue,
getClipPathInsetPx,
inferClipPathPreset,
parseInsetClipPathSides,
parsePxMetricValue,
type ClipPathInsetSides,
} from "./propertyPanelHelpers";
import { FlatSlider } from "./propertyPanelFlatPrimitives";
import { MetricField } from "./propertyPanelPrimitives";
/* ------------------------------------------------------------------ */
/* Flat Mask inset — uniform slider + per-side fields */
/* (split out of propertyPanelFlatStyleSections.tsx to stay under the */
/* 600-line file-size gate) */
/* ------------------------------------------------------------------ */
export function FlatMaskInsetRows({
clipPathValue,
radiusValue,
disabled,
onSetStyle,
}: {
clipPathValue: string;
radiusValue: number;
disabled: boolean;
onSetStyle: (prop: string, value: string) => void | Promise<void>;
}) {
const clipPathPreset = inferClipPathPreset(clipPathValue);
const parsedClipInsets = parseInsetClipPathSides(clipPathValue);
const clipInsetValue = getClipPathInsetPx(clipPathValue);
const clipInsetSides = parsedClipInsets ?? {
top: clipInsetValue,
right: clipInsetValue,
bottom: clipInsetValue,
left: clipInsetValue,
radius: radiusValue,
};
const showClipInsetSides = clipPathPreset === "inset" || parsedClipInsets != null;
const commitClipInsetSide = (side: keyof ClipPathInsetSides, nextValue: string) => {
const next = parsePxMetricValue(nextValue);
if (next == null) return;
const sides: ClipPathInsetSides = {
top: clipInsetSides.top,
right: clipInsetSides.right,
bottom: clipInsetSides.bottom,
left: clipInsetSides.left,
};
sides[side] = next;
void onSetStyle("clip-path", buildInsetClipPathSides(sides, clipInsetSides.radius));
};
return (
<>
<FlatSlider
label="Mask inset"
value={clipInsetValue}
min={0}
max={Math.max(120, Math.ceil(clipInsetValue))}
step={1}
tier={clipInsetValue > 0 ? "explicitCustom" : "default"}
displayValue={`${formatNumericValue(clipInsetValue)}px`}
disabled={disabled}
onCommit={(next) =>
void onSetStyle("clip-path", buildInsetClipPathValue(next, radiusValue))
}
/>
{showClipInsetSides && (
<div className="grid grid-cols-4 gap-2">
<MetricField
label="T"
value={formatPxMetricValue(clipInsetSides.top)}
disabled={disabled}
onCommit={(next) => commitClipInsetSide("top", next)}
/>
<MetricField
label="R"
value={formatPxMetricValue(clipInsetSides.right)}
disabled={disabled}
onCommit={(next) => commitClipInsetSide("right", next)}
/>
<MetricField
label="B"
value={formatPxMetricValue(clipInsetSides.bottom)}
disabled={disabled}
onCommit={(next) => commitClipInsetSide("bottom", next)}
/>
<MetricField
label="L"
value={formatPxMetricValue(clipInsetSides.left)}
disabled={disabled}
onCommit={(next) => commitClipInsetSide("left", next)}
/>
</div>
)}
</>
);
}
@@ -89,8 +89,8 @@ describe("FlatSegmentedRow", () => {
<FlatSegmentedRow
label="Align"
options={[
{ key: "left", node: "L", active: false },
{ key: "right", node: "R", active: true },
{ key: "left", node: "L", label: "left", active: false },
{ key: "right", node: "R", label: "right", active: true },
]}
onChange={onChange}
/>,
@@ -105,6 +105,25 @@ describe("FlatSegmentedRow", () => {
expect(onChange).toHaveBeenCalledWith("left");
act(() => root.unmount());
});
it("gives each option an accessible name and pressed state — glyphs alone (e.g. two 'A' buttons) aren't a valid accessible name", () => {
const { host, root } = renderInto(
<FlatSegmentedRow
label="Case · Style"
options={[
{ key: "normal", node: "A", label: "upright", active: true },
{ key: "italic", node: "A", label: "italic", active: false },
]}
onChange={vi.fn()}
/>,
);
const options = host.querySelectorAll<HTMLButtonElement>('[data-flat-segment="true"]');
expect(options[0]?.getAttribute("aria-label")).toBe("upright");
expect(options[0]?.getAttribute("aria-pressed")).toBe("true");
expect(options[1]?.getAttribute("aria-label")).toBe("italic");
expect(options[1]?.getAttribute("aria-pressed")).toBe("false");
act(() => root.unmount());
});
});
describe("FlatGroupHeader", () => {
@@ -554,6 +573,130 @@ describe("FlatSlider — Grade extensions", () => {
act(() => root.unmount());
});
it("disables the reset button when the slider itself is disabled", () => {
const onReset = vi.fn();
const { host, root } = renderInto(
<FlatSlider
label="Exposure"
value={20}
min={0}
max={100}
tier="explicitCustom"
displayValue="20"
disabled
onReset={onReset}
onCommit={vi.fn()}
/>,
);
const resetButton = host.querySelector<HTMLButtonElement>('[data-flat-slider-reset="true"]');
expect(resetButton?.disabled).toBe(true);
act(() => resetButton?.dispatchEvent(new MouseEvent("click", { bubbles: true })));
expect(onReset).not.toHaveBeenCalled();
act(() => root.unmount());
});
it("a trailing throttled commit uses the current render's onCommit, not the one captured when it was scheduled", () => {
vi.useFakeTimers();
const onCommitA = vi.fn();
const { host, root } = renderInto(
<FlatSlider
label="Exposure"
value={0}
min={-100}
max={100}
tier="explicitCustom"
displayValue="0"
onCommit={onCommitA}
/>,
);
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: 200, top: 0, height: 20, right: 200, bottom: 20 }),
});
act(() => {
// Leading-edge commit fires synchronously with onCommitA (clientX 150
// on a -100..100 track maps to 50, distinct from the initial value 0).
track.dispatchEvent(
new PointerEvent("pointerdown", { bubbles: true, clientX: 150, pointerId: 1 }),
);
});
expect(onCommitA).toHaveBeenCalledTimes(1);
act(() => {
// Within the 40ms throttle window — queues a trailing commit (to 80,
// distinct from the just-committed 50) instead of firing immediately.
track.dispatchEvent(
new PointerEvent("pointermove", { bubbles: true, clientX: 180, pointerId: 1 }),
);
});
expect(onCommitA).toHaveBeenCalledTimes(1);
// Simulate the real-world race: something else causes this slider to
// re-render with a NEW onCommit closure before the queued timer fires
// (e.g. Grade's per-detail onCommit spreads the render-time whole
// grading object, so a different control committing in between produces
// a fresh closure). The stale closure must not win.
const onCommitB = vi.fn();
act(() => {
root.render(
<FlatSlider
label="Exposure"
value={0}
min={-100}
max={100}
tier="explicitCustom"
displayValue="0"
onCommit={onCommitB}
/>,
);
});
act(() => {
vi.advanceTimersByTime(45);
});
expect(onCommitB).toHaveBeenCalledTimes(1);
expect(onCommitB).toHaveBeenCalledWith(80);
expect(onCommitA).toHaveBeenCalledTimes(1);
act(() => root.unmount());
vi.useRealTimers();
});
it("flushes a still-queued trailing commit on unmount instead of dropping it", () => {
vi.useFakeTimers();
const onCommit = vi.fn();
const { host, root } = renderInto(
<FlatSlider
label="Opacity"
value={5}
min={0}
max={100}
tier="explicitCustom"
displayValue="5%"
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: 200, top: 0, height: 20, right: 200, bottom: 20 }),
});
act(() => {
track.dispatchEvent(
new PointerEvent("pointerdown", { bubbles: true, clientX: 20, pointerId: 1 }),
);
});
expect(onCommit).toHaveBeenCalledTimes(1);
act(() => {
// Queues a trailing commit that never gets to fire before unmount.
track.dispatchEvent(
new PointerEvent("pointermove", { bubbles: true, clientX: 160, pointerId: 1 }),
);
});
expect(onCommit).toHaveBeenCalledTimes(1);
act(() => root.unmount());
expect(onCommit).toHaveBeenCalledTimes(2);
expect(onCommit).toHaveBeenNthCalledWith(2, 80);
vi.useRealTimers();
});
it("supports keyboard operation: focusable, arrow keys step, Home/End clamp to range", () => {
const onCommit = vi.fn();
const { host, root } = renderInto(
@@ -728,6 +871,31 @@ describe("FlatSelectRow — label/value options", () => {
expect(options).toEqual(["normal", "multiply", "screen"]);
act(() => root.unmount());
});
it("preserves a valid authored value outside the preset list instead of misrepresenting it as the first option", () => {
const onChange = vi.fn();
const { host, root } = renderInto(
<FlatSelectRow
label="Blend"
value="difference"
options={["normal", "multiply", "screen", "overlay"]}
tier="explicitCustom"
onChange={onChange}
/>,
);
const select = host.querySelector<HTMLSelectElement>("select");
// A native <select> whose `value` matches no <option> falls back to
// selectedIndex 0 — silently showing "normal" as selected even though
// the real persisted value is "difference". The row must add an option
// for the current value so it's genuinely representable.
expect(select?.value).toBe("difference");
const options = Array.from(host.querySelectorAll("option")).map((o) => o.textContent);
expect(options).toContain("difference");
// And reselecting the (still-present) first preset must be an explicit
// user choice, not something that already happened silently.
expect(onChange).not.toHaveBeenCalled();
act(() => root.unmount());
});
});
describe("FlatToggle", () => {
@@ -87,6 +87,10 @@ export function FlatRow({
export interface FlatSegmentOption {
key: string;
node: ReactNode;
/** Accessible name — the glyph alone (e.g. two indistinguishable "A"
* buttons for upright vs. italic) isn't a valid accessible name on its
* own. */
label: string;
active: boolean;
}
@@ -114,6 +118,8 @@ export function FlatSegmentedRow({
<button
type="button"
data-flat-segment="true"
aria-label={option.label}
aria-pressed={option.active}
disabled={disabled}
onClick={() => onChange(option.key)}
className={`px-1.5 py-1 text-[11px] transition-colors disabled:cursor-not-allowed ${
@@ -286,6 +292,14 @@ export function FlatSlider({
// prop, so the release flush must dedupe against what we just sent, not
// against the stale prop, or the same value commits twice.
const lastCommittedRef = useRef(value);
// Always the current render's onCommit — read inside the throttle timer
// instead of closing over the callback at schedule time. A caller whose
// onCommit spreads other current state (e.g. Grade's "...grading, details:
// {...}") would otherwise let a queued trailing commit fire ~40ms later
// with a stale snapshot and silently revert whatever the user changed on a
// different control in between.
const onCommitRef = useRef(onCommit);
onCommitRef.current = onCommit;
useEffect(() => {
if (draggingRef.current) return;
@@ -294,7 +308,14 @@ export function FlatSlider({
}, [value]);
useEffect(
() => () => {
if (commitTimerRef.current) clearTimeout(commitTimerRef.current);
if (commitTimerRef.current) {
clearTimeout(commitTimerRef.current);
// Flush rather than drop a still-queued edit — this only fires if the
// component unmounts mid-drag (e.g. selection changes away), and
// silently discarding the user's last dragged position would look
// like data loss.
if (pendingRef.current !== null) onCommitRef.current(pendingRef.current);
}
},
[],
);
@@ -316,7 +337,7 @@ export function FlatSlider({
lastCommitAtRef.current = Date.now();
if (nextDraft !== lastCommittedRef.current) {
lastCommittedRef.current = nextDraft;
onCommit(nextDraft);
onCommitRef.current(nextDraft);
}
};
const scheduleCommit = (nextDraft: number) => {
@@ -346,6 +367,7 @@ export function FlatSlider({
aria-valuemax={max}
aria-disabled={disabled}
tabIndex={disabled ? -1 : 0}
style={{ touchAction: "none" }}
className={`relative h-5 flex-1 ${disabled ? "cursor-not-allowed" : "cursor-pointer"}`}
onPointerDown={(e) => {
if (disabled) return;
@@ -429,8 +451,9 @@ export function FlatSlider({
type="button"
data-flat-slider-reset="true"
title="Remove — fall back to default"
disabled={disabled}
onClick={onReset}
className="text-panel-text-3 hover:text-panel-text-1"
className="text-panel-text-3 hover:text-panel-text-1 disabled:cursor-not-allowed disabled:opacity-40"
>
<RotateCcw size={11} />
</button>
@@ -465,6 +488,17 @@ export function FlatSelectRow({
const normalizedOptions = options.map((option) =>
typeof option === "string" ? { value: option, label: option } : option,
);
// A valid authored value outside the preset list (e.g. a `mix-blend-mode`
// or `object-position` this row doesn't offer as a preset) must not be
// silently misrepresented as the first option — the native <select> falls
// back to selectedIndex 0 when `value` matches no <option>, and reselecting
// that visible-but-wrong preset overwrites the real persisted value. Prepend
// the current value so it's always representable, matching legacy
// `SelectField`'s same guard.
const renderedOptions =
value && !normalizedOptions.some((option) => option.value === value)
? [{ value, label: value }, ...normalizedOptions]
: normalizedOptions;
return (
<div className="group flex min-h-[30px] items-center justify-between">
<span className={`text-[11px] ${VALUE_TIER_LABEL_CLASS[tier]}`}>{label}</span>
@@ -476,7 +510,7 @@ export function FlatSelectRow({
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]}`}
>
{normalizedOptions.map((option) => (
{renderedOptions.map((option) => (
<option key={option.value} value={option.value}>
{option.label}
</option>
@@ -11,24 +11,19 @@ import {
import {
buildBoxShadowPresetValue,
buildClipPathValue,
buildInsetClipPathSides,
buildInsetClipPathValue,
buildStrokeStyleUpdates,
buildStrokeWidthStyleUpdates,
extractBackgroundImageUrl,
formatNumericValue,
formatPxMetricValue,
getClipPathInsetPx,
getCssFilterFunctionPx,
inferBoxShadowPreset,
inferClipPathPreset,
normalizePanelPxValue,
parseInsetClipPathSides,
parseNumericValue,
parsePxMetricValue,
setCssFilterFunctionPx,
type BoxShadowPreset,
type ClipPathInsetSides,
} from "./propertyPanelHelpers";
import {
FlatRow,
@@ -36,7 +31,7 @@ import {
FlatSelectRow,
FlatSlider,
} from "./propertyPanelFlatPrimitives";
import { MetricField } from "./propertyPanelPrimitives";
import { FlatMaskInsetRows } from "./propertyPanelFlatMaskInsetRows";
import { resolveValueTier } from "./propertyPanelValueTier";
import { ColorField } from "./propertyPanelColor";
import { GradientField, ImageFillField } from "./propertyPanelFill";
@@ -96,9 +91,14 @@ function FlatFillFields({
<FlatSegmentedRow
label="Fill"
options={[
{ key: "Solid", node: "Solid", active: preferredFillMode === "Solid" },
{ key: "Gradient", node: "Gradient", active: preferredFillMode === "Gradient" },
{ key: "Image", node: "Image", active: preferredFillMode === "Image" },
{ key: "Solid", node: "Solid", label: "Solid", active: preferredFillMode === "Solid" },
{
key: "Gradient",
node: "Gradient",
label: "Gradient",
active: preferredFillMode === "Gradient",
},
{ key: "Image", node: "Image", label: "Image", active: preferredFillMode === "Image" },
]}
disabled={styleEditingDisabled}
onChange={handleFillModeChange}
@@ -435,90 +435,6 @@ function FlatOverflowMaskRows({
);
}
// Flat Mask inset — uniform slider + per-side fields.
function FlatMaskInsetRows({
clipPathValue,
radiusValue,
disabled,
onSetStyle,
}: {
clipPathValue: string;
radiusValue: number;
disabled: boolean;
onSetStyle: (prop: string, value: string) => void | Promise<void>;
}) {
const clipPathPreset = inferClipPathPreset(clipPathValue);
const parsedClipInsets = parseInsetClipPathSides(clipPathValue);
const clipInsetValue = getClipPathInsetPx(clipPathValue);
const clipInsetSides = parsedClipInsets ?? {
top: clipInsetValue,
right: clipInsetValue,
bottom: clipInsetValue,
left: clipInsetValue,
radius: radiusValue,
};
const showClipInsetSides = clipPathPreset === "inset" || parsedClipInsets != null;
const commitClipInsetSide = (side: keyof ClipPathInsetSides, nextValue: string) => {
const next = parsePxMetricValue(nextValue);
if (next == null) return;
const sides: ClipPathInsetSides = {
top: clipInsetSides.top,
right: clipInsetSides.right,
bottom: clipInsetSides.bottom,
left: clipInsetSides.left,
};
sides[side] = next;
void onSetStyle("clip-path", buildInsetClipPathSides(sides, clipInsetSides.radius));
};
return (
<>
<FlatSlider
label="Mask inset"
value={clipInsetValue}
min={0}
max={Math.max(120, Math.ceil(clipInsetValue))}
step={1}
tier={clipInsetValue > 0 ? "explicitCustom" : "default"}
displayValue={`${formatNumericValue(clipInsetValue)}px`}
disabled={disabled}
onCommit={(next) =>
void onSetStyle("clip-path", buildInsetClipPathValue(next, radiusValue))
}
/>
{showClipInsetSides && (
<div className="grid grid-cols-4 gap-2">
<MetricField
label="T"
value={formatPxMetricValue(clipInsetSides.top)}
disabled={disabled}
onCommit={(next) => commitClipInsetSide("top", next)}
/>
<MetricField
label="R"
value={formatPxMetricValue(clipInsetSides.right)}
disabled={disabled}
onCommit={(next) => commitClipInsetSide("right", next)}
/>
<MetricField
label="B"
value={formatPxMetricValue(clipInsetSides.bottom)}
disabled={disabled}
onCommit={(next) => commitClipInsetSide("bottom", next)}
/>
<MetricField
label="L"
value={formatPxMetricValue(clipInsetSides.left)}
disabled={disabled}
onCommit={(next) => commitClipInsetSide("left", next)}
/>
</div>
)}
</>
);
}
/* ------------------------------------------------------------------ */
/* Flat Opacity slider */
/* ------------------------------------------------------------------ */
@@ -210,7 +210,7 @@ describe("FlatTextFieldEditor controls", () => {
act(() => root.unmount());
});
it("lights up 'right' for text-align: end and commits the concrete 'right' value on click", () => {
it("lights up 'right' for text-align: end but re-clicking it is a no-op — preserves the logical value", () => {
const onSetTextFieldStyle = vi.fn();
const { host, root } = renderInto(
<FlatTextSection
@@ -227,8 +227,32 @@ describe("FlatTextFieldEditor controls", () => {
const rightButton = alignButtons.find((button) => button.textContent === "R");
expect(rightButton).not.toBeUndefined();
expect(rightButton?.className).toContain("border-panel-accent");
// Clicking the option that's already visually active for "end" must NOT
// rewrite it to the physical "right" — that would destroy the logical
// semantics and break RTL content, where "end" and "right" differ.
act(() => rightButton?.dispatchEvent(new MouseEvent("click", { bubbles: true })));
expect(onSetTextFieldStyle).toHaveBeenCalledWith("a", "text-align", "right");
expect(onSetTextFieldStyle).not.toHaveBeenCalled();
act(() => root.unmount());
});
it("commits a genuine align change away from a logical value", () => {
const onSetTextFieldStyle = vi.fn();
const { host, root } = renderInto(
<FlatTextSection
element={makeSingleFieldElement({ computedStyles: { "text-align": "end" } })}
styles={{}}
fontAssets={[]}
onSetText={vi.fn()}
onSetTextFieldStyle={onSetTextFieldStyle}
onAddTextField={vi.fn()}
onRemoveTextField={vi.fn()}
/>,
);
const alignButtons = segmentedRowButtons(host, "Align");
const centerButton = alignButtons.find((button) => button.textContent === "C");
expect(centerButton).not.toBeUndefined();
act(() => centerButton?.dispatchEvent(new MouseEvent("click", { bubbles: true })));
expect(onSetTextFieldStyle).toHaveBeenCalledWith("a", "text-align", "center");
act(() => root.unmount());
});
@@ -33,10 +33,10 @@ const ALIGN_OPTIONS = [
];
const CASE_OPTIONS = [
{ key: "none", node: "" },
{ key: "uppercase", node: "AG" },
{ key: "lowercase", node: "ag" },
{ key: "capitalize", node: "Ag" },
{ key: "none", label: "none", node: "" },
{ key: "uppercase", label: "uppercase", node: "AG" },
{ key: "lowercase", label: "lowercase", node: "ag" },
{ key: "capitalize", label: "capitalize", node: "Ag" },
];
function FlatTextFieldEditor({
@@ -167,12 +167,23 @@ function FlatTextFieldEditor({
options={ALIGN_OPTIONS.map((option) => ({
key: option.key,
node: option.node,
label: option.label,
active:
align === option.key ||
(option.key === "left" && align === "start") ||
(option.key === "right" && align === "end"),
}))}
onChange={(next) => onSetTextFieldStyle(field.key, "text-align", next)}
onChange={(next) => {
// Re-clicking the option that's already visually active for a
// logical value (authored "start"/"end") must not rewrite it to
// the physical "left"/"right" — that destroys the logical
// semantics and is wrong for RTL content. Only write when the
// user actually picked a different alignment.
if ((next === "left" && align === "start") || (next === "right" && align === "end")) {
return;
}
onSetTextFieldStyle(field.key, "text-align", next);
}}
/>
<FlatSegmentedRow
label="Case · Style"
@@ -180,10 +191,11 @@ function FlatTextFieldEditor({
...CASE_OPTIONS.map((option) => ({
key: option.key,
node: option.node,
label: option.label,
active: textTransform === option.key,
})),
{ key: "normal", node: "A", active: fontStyle === "normal" },
{ key: "italic", node: "A", active: fontStyle === "italic" },
{ key: "normal", node: "A", label: "upright", active: fontStyle === "normal" },
{ key: "italic", node: "A", label: "italic", active: fontStyle === "italic" },
]}
spacerAfterIndex={2}
onChange={(next) => {
@@ -53,35 +53,48 @@ function makeElement(overrides: Partial<DomEditSelection> = {}): DomEditSelectio
function HookHost({
onState,
onSetAttributeLive,
element,
}: {
onState: (state: ReturnType<typeof useColorGradingController>) => void;
onSetAttributeLive: (attr: string, value: string | null) => void;
element: DomEditSelection;
}) {
const state = useColorGradingController({
projectId: "proj",
element: makeElement(),
element,
onSetAttributeLive,
});
onState(state);
return null;
}
function renderHook(onSetAttributeLive: (attr: string, value: string | null) => void) {
function renderHook(
onSetAttributeLive: (attr: string, value: string | null) => void,
initialElement: DomEditSelection = makeElement(),
) {
const host = document.createElement("div");
document.body.append(host);
const root = createRoot(host);
let latest: ReturnType<typeof useColorGradingController> | undefined;
const renderWith = (element: DomEditSelection) => {
act(() => {
root.render(
React.createElement(HookHost, {
onState: (s: ReturnType<typeof useColorGradingController>) => (latest = s),
onSetAttributeLive,
element,
}),
);
});
};
renderWith(initialElement);
return {
root,
get state() {
rerenderWithElement: renderWith,
// A method, not a getter — `const { state } = renderHook(...)` would
// destructure a getter into a one-time snapshot, silently going stale
// after the first state change. Call `.getState()` fresh every time.
getState(): ReturnType<typeof useColorGradingController> {
if (!latest) throw new Error("hook did not render");
return latest;
},
@@ -90,19 +103,20 @@ function renderHook(onSetAttributeLive: (attr: string, value: string | null) =>
describe("useColorGradingController", () => {
it("starts with the neutral (inactive) grading and idle compare state", () => {
const { root, state } = renderHook(vi.fn());
expect(state.grading.preset).toBe("neutral");
expect(state.compareEnabled).toBe(false);
const { root, getState } = renderHook(vi.fn());
expect(getState().grading.preset).toBe("neutral");
expect(getState().compareEnabled).toBe(false);
act(() => root.unmount());
});
it("commitColorGrading updates grading state synchronously and schedules a debounced persist", async () => {
vi.useFakeTimers();
const onSetAttributeLive = vi.fn();
const { root, state } = renderHook(onSetAttributeLive);
const { root, getState } = renderHook(onSetAttributeLive);
act(() => {
state.commitColorGrading(freshPopGrading());
getState().commitColorGrading(freshPopGrading());
});
expect(getState().grading.preset).toBe("fresh-pop");
expect(onSetAttributeLive).not.toHaveBeenCalled();
act(() => {
vi.advanceTimersByTime(400);
@@ -116,14 +130,95 @@ describe("useColorGradingController", () => {
});
it("resetGrading returns to the neutral preset", () => {
const { root, state } = renderHook(vi.fn());
const { root, getState } = renderHook(vi.fn());
act(() => {
state.commitColorGrading(freshPopGrading());
getState().commitColorGrading(freshPopGrading());
});
act(() => {
state.resetGrading();
getState().resetGrading();
});
expect(state.grading.preset).toBe("neutral");
expect(getState().grading.preset).toBe("neutral");
act(() => root.unmount());
});
it("resets grading/compare state when selection changes to a different element", () => {
const { root, getState, rerenderWithElement } = renderHook(
vi.fn(),
makeElement({ id: "s1-bg" }),
);
act(() => {
getState().commitColorGrading(freshPopGrading());
});
expect(getState().grading.preset).toBe("fresh-pop");
// A different element, with no persisted grading of its own — without a
// reset, this hook (unlike the legacy component it was extracted from,
// which remounts via a `key={selectionIdentityKey}`) would keep showing
// the previous element's grading.
rerenderWithElement(makeElement({ id: "s2-bg" }));
expect(getState().grading.preset).toBe("neutral");
act(() => root.unmount());
});
it("cancels a pending persist scheduled for the previous element when selection changes before it flushes", () => {
vi.useFakeTimers();
const onSetAttributeLive = vi.fn();
const { root, getState, rerenderWithElement } = renderHook(
onSetAttributeLive,
makeElement({ id: "s1-bg" }),
);
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.
act(() => {
vi.advanceTimersByTime(200);
});
rerenderWithElement(makeElement({ id: "s2-bg" }));
act(() => {
vi.advanceTimersByTime(400);
});
expect(onSetAttributeLive).not.toHaveBeenCalled();
act(() => root.unmount());
vi.useRealTimers();
});
it("does not permanently cache a non-OK media/metadata response — the next mount retries", async () => {
const videoWithSrc = () => {
const el = document.createElement("video");
el.setAttribute("src", "clip.mp4");
return el;
};
const fetchMock = vi
.fn()
.mockResolvedValueOnce({ ok: false } as Response)
.mockResolvedValueOnce({
ok: true,
json: () =>
Promise.resolve({ metadata: { kind: "video", color: { dynamicRange: "hdr" } } }),
} as unknown as Response);
vi.stubGlobal("fetch", fetchMock);
const first = renderHook(vi.fn(), makeElement({ id: "retry-asset", element: videoWithSrc() }));
await act(async () => {
await Promise.resolve();
});
expect(first.getState().mediaMetadata).toBeNull();
act(() => first.root.unmount());
// A second, independent mount for the SAME asset path — if the failed
// response had been cached, this would never re-fetch and mediaMetadata
// would stay null forever.
const second = renderHook(
vi.fn(),
makeElement({ id: "retry-asset-2", element: videoWithSrc() }),
);
await act(async () => {
await Promise.resolve();
});
expect(fetchMock).toHaveBeenCalledTimes(2);
expect(second.getState().mediaMetadata?.color.dynamicRange).toBe("hdr");
act(() => second.root.unmount());
vi.unstubAllGlobals();
});
});
@@ -12,7 +12,7 @@ import {
trackStudioPendingEdit,
} from "../../utils/studioPendingEdits";
import type { DomEditSelection } from "./domEditing";
import { stripQueryAndHash } from "./propertyPanelHelpers";
import { selectionIdentityKey, stripQueryAndHash } from "./propertyPanelHelpers";
import {
acceptStudioRuntimeMessage,
postRuntimeControlMessage,
@@ -196,6 +196,38 @@ export function useColorGradingController({
onSetAttributeLiveRef.current = onSetAttributeLive;
latestGradingRef.current = grading;
compareEnabledRef.current = compareEnabled;
// 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 this — it resolves in the same render
// pass instead of flashing the stale state for one frame via useEffect.
const identityKey = selectionIdentityKey(element);
const identityKeyRef = useRef(identityKey);
if (identityKeyRef.current !== identityKey) {
identityKeyRef.current = identityKey;
if (persistTimerRef.current) {
clearTimeout(persistTimerRef.current);
persistTimerRef.current = null;
}
pendingPersistValueRef.current = undefined;
for (const timer of statusTimersRef.current) clearTimeout(timer);
statusTimersRef.current = [];
const freshGrading = readColorGradingFromElement(element);
latestGradingRef.current = freshGrading;
setGrading(freshGrading);
setCompareEnabled(false);
compareEnabledRef.current = false;
setApplyScope("source-file");
setApplyBusy(false);
setRuntimeStatus({ state: "pending", message: "Waiting for runtime" });
setMediaMetadata(null);
}
const target = useMemo(
(): HfColorGradingTarget => ({
id: element.id ?? null,
@@ -225,18 +257,29 @@ export function useColorGradingController({
)}`,
{ signal: controller.signal },
)
.then((response) => (response.ok ? response.json() : null))
.then((data: MediaMetadataResponse | null) => {
.then(async (response) => {
if (!response.ok) return { ok: false as const };
const data: MediaMetadataResponse | null = await response.json();
return { ok: true as const, metadata: data?.metadata ?? null };
})
.then((result) => {
if (controller.signal.aborted) return;
const metadata = data?.metadata ?? null;
MEDIA_METADATA_CACHE.set(cacheKey, metadata);
setMediaMetadata(metadata);
// Only cache a definitive answer from a successful response — a non-OK
// status is a transient/server failure, not a stable "no metadata"
// result, and caching it would suppress the HDR banner for this asset
// for the page's whole lifetime. Leave the key absent so the next
// selection retries.
if (!result.ok) {
setMediaMetadata(null);
return;
}
MEDIA_METADATA_CACHE.set(cacheKey, result.metadata);
setMediaMetadata(result.metadata);
})
.catch(() => {
// Don't cache a transient fetch failure — a cached null would suppress
// the HDR banner for this asset for the page's whole lifetime. Leave the
// key absent so the next selection retries. (A successful response with
// no metadata still caches null above, which IS a stable answer.)
// Same reasoning as the non-OK branch above: don't cache a network-
// level fetch failure either.
if (!controller.signal.aborted) setMediaMetadata(null);
});
return () => controller.abort();
}, [projectId, selectedAssetPath]);