A fresh full-stack re-review checked 15 PR heads independently. Cross-
checked all 9 remaining claims against the actual current tip:
- #2120 (id/selector key qualification, Hide All no-op/race, variable
parity), #2121 (opacity-zero fallback), #2122 (GSAP preview sibling
resolution, scrub-label), #2124/#2126 (negative metadata cache), and
the keyboard-access half of #2121/#2186 were all already fixed by a
later commit in this same stack (65954c380, PR #2225) — the reviewed
heads predate it. Verified each in the current source rather than
taking the isolated-head review at face value.
- #2186's "no ESC/right-click cancel during drag" was the one claim that
held up: FlatSlider had keyboard arrow-key support but no way to abort
an in-progress pointer drag. Escape now reverts to the pre-drag value
and releases pointer capture; a right-click (contextmenu) during a drag
does the same instead of committing whatever position the pointer last
reached while the native context menu opens over the slider. Both go
through commitDraft (not just a visual reset) since the drag's leading-
edge commit in onPointerDown may already have applied an intermediate
value that needs actually undoing, not just hiding.
propertyPanelFlatPrimitives.tsx crossed the 600-line file-size gate after
this change; extracted FlatSelectRow into its own file, matching the
FlatToggle/FlatMaskInsetRows precedent from earlier in this stack.
New regression tests for Escape-cancel and contextmenu-cancel. Full
studio suite still at the known pre-existing 55-failure baseline, zero
new regressions. Typecheck/oxlint/oxfmt clean.
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.
Fixes three of the adversarial findings from the third #2416 tip
re-review:
- Grade rollback was identity-scoped but not attempt-scoped: two edits on
the SAME element (e.g. drag Exposure, then Contrast, before Exposure's
persist settles) could have the earlier edit's late completion stamp
confirmedGradingRef with its now-superseded value, or revert `grading`
out from under the newer optimistic edit. Added a monotonic per-commit
version via the existing bumpDomEditCommitVersion primitive (the same
one the DOM-attribute commit runner uses for the identical race) —
persistColorGradingValue now checks both identity AND "is this still the
latest attempt for this element" before applying any effect.
- The render-phase identity-reset block consumed shared mutable state
(clearing the pending-persist timer, reading and nulling
pendingPersistValueRef) directly during render. Adjusting STATE during
render this way is React's documented pattern and safe to repeat, but
consuming a ref this way is not: if React discarded/interrupted that
specific render before it committed, the timer would already be
cancelled and the pending value already nulled, with no corresponding
effect ever running to compensate, silently losing the edit. Replaced
with the idiomatic pattern for "clean up a per-identity resource when it
changes" — a useEffect keyed on identityKey whose CLEANUP performs the
cancellation/flush. A cleanup only ever runs for the effect instance
that actually committed, closing the gap entirely. The render-phase
block now only performs pure, idempotent state resets.
- FlatSelectRow's Preset row passes label="" (the visible "Preset" text is
a sibling span, to avoid rendering it twice) which left the underlying
<select> with no accessible name at all. Added a dedicated `ariaLabel`
prop, distinct from the visible `label`, so a caller can supply a name
without a duplicate visible label.
Also hardened FlatSlider's lostpointercapture handling: it now resyncs
the draft directly from a latestValueRef immediately, instead of only
clearing the dragging flag and waiting for the separate [value]-keyed
effect to notice — closing a narrow ordering gap where a value change
arriving while still dragging, followed by capture loss with no further
render, could otherwise leave the knob stuck.
propertyPanelFlatPrimitives.tsx crossed the 600-line file-size gate after
these changes; extracted FlatToggle (and its tests) into their own files,
matching the FlatMaskInsetRows precedent from an earlier commit in this
stack.
New/updated regression tests: same-element version race, Preset select's
aria-label. Full studio suite still at the known pre-existing 55-failure
baseline, zero regressions.
Fixes two of the three adversarial findings from the second #2416 tip
re-review; the third is a pre-existing runtime-protocol gap, explained in
the PR thread rather than patched here.
- The Grade rollback added in the previous commit could never fire through
the real Studio callback: runDomEditCommit (the shared commit runner used
by every data-attribute commit, not just Grade) catches persist failures
internally and always resolves, reporting outcome only via its own
onError side effect. A caller awaiting the promise never sees a
rejection, so the revert-on-reject logic was dead code against the
actual app. Added an optional onSettled(ok) callback to
DomEditCommitRunnerConfig (purely additive — every existing caller that
doesn't pass it is unaffected) and threaded it through
commitDataAttribute -> handleDomAttributeLiveCommit -> the
onSetAttributeLive prop type (now accepts an optional 3rd argument) ->
useColorGradingController, which now drives the revert from the real
signal. The promise-rejection path stays as a fallback for any other
implementation of onSetAttributeLive that rejects instead.
- Selection flushing performed a real side effect (writing the outgoing
element's pending edit) during the render-phase identity-reset block.
Adjusting STATE during render (comparing against a ref) is React's
documented pattern, but it doesn't license actual I/O — React can invoke
render more than once per commit, which could double-fire or misorder
the write. The reset block now only enqueues the flush (a pure ref
write); a new effect keyed on the identity performs it after commit.
- Async persist completions (both the onSettled callback and its promise-
rejection fallback) now capture the identity key the attempt was made
for and check it against the CURRENT identity before touching
confirmedGradingRef/grading/runtimeStatus. Without this, a persist that
settles after selection has moved on to a THIRD element could clobber
that element's freshly-reset state with a result that belongs to an
element no longer selected.
Not fixed here: the runtime Grade target (HfColorGradingTarget, used by
core's resolveTarget to find the DOM element inside the preview iframe)
has no source-file/composition-scope discriminator, matching the same gap
selectionIdentityKey had before this stack — but fixing it means changing
a wire-protocol type shared across core/player/studio and the legacy
ColorGradingSection too. hfId (checked first, before id/selector) is
minted uniquely per element at parse time in the common case, so this is
a narrow residual risk for hfId-less same-selector elements across
different source files, not a regression introduced by this stack.
Flagged as a follow-up in the PR thread.
New/updated regression tests: real onSettled(false) path (distinct from
the promise-rejection fallback), and a stale in-flight persist settling
after selection has moved on twice more. Full studio suite still at the
known pre-existing 55-failure baseline, zero regressions.
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).
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).
A pure debounce resets its timer on every pointermove, so a real drag
(events faster than 40ms apart) never commits until the pointer pauses
or lifts — killing live preview updates mid-drag. Throttle with a
leading-edge commit + trailing flush instead.
Only onPointerDown was wired, so dragging the knob/track only ever
committed the initial click position — nothing tracked the pointer
after that. Uses the Pointer Capture API (setPointerCapture on
pointerdown, onPointerMove while captured, release on pointerup) so
the value follows the cursor continuously during a drag, matching how
the legacy native <input type="range"> control behaves for free.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The track's visible line was only 2px tall, and pointerdown was bound
directly to that thin element, making it hard to grab. The hit area is
now 20px tall (a wrapping div) with the visible line rendered as a
thin decorative child, centered inside it — the ratio math only reads
left/width so click accuracy is unaffected.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds a fast (120ms) CSS entrance animation for flat inspector accordion
group headers/body, gated to the group actually toggling (not derived
from remounting alone) to avoid a Chromium reflow quirk that otherwise
replays the animation on untouched collapsed siblings.
Collapsed group headers render in fixed, non-scrolling document flow
above and below the open group; only the open group's own content
scrolls, in a dedicated region. Also fixes the flat inspector footer's
missing background.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The flat inspector rendered its new Style/Grade groups AND the legacy
ColorGradingSection/StyleSections components a second time below them,
visibly doubling every control. Remove the now-redundant legacy render
call sites (and their now-unused imports) from PropertyPanelFlat.tsx;
those components stay intact for the legacy (flag-off) PropertyPanel.
FlatTextSection's multi-field branch (textFields.length > 1) now renders
FlatTextLayerList (Task 5) + the existing single-field FlatTextFieldEditor
for the active field, tracked via new local activeFieldKey state that
resyncs (useEffect) when the active field disappears from props. This
retires the legacy TextSection delegation entirely for that case; the
TextSection import is removed from propertyPanelFlatTextSection.tsx since
nothing else in the file referenced it.
Also updates propertyPanelSections.test.tsx and PropertyPanel.test.tsx,
which exercised/documented the old multi-field-falls-back-to-legacy-
TextSection behavior in comments and test titles — reworded to describe
the new flat path (assertions were already compatible and still pass).
Flag for reviewer: hideOwnHeading on the legacy TextSection component
(propertyPanelSections.tsx) was added in an earlier plan specifically for
this now-removed call site. It has no remaining consumer after this task
lands (PropertyPanel.tsx's legacy caller doesn't pass it). Left in place
per brief instruction — not deleting unilaterally, since that's a scope
decision for whoever reviews this task.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Review proved the existing test didn't catch a broken stopPropagation
by temporarily removing it and confirming the suite still passed.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Review of the pin-aware group list refactor flagged the test name
claiming the group "closes" on unpin — it doesn't assert that, and
structurally the group re-opens (togglePin never touches openGroupId).
Retitled to describe only the return-to-stack behavior actually tested.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Reads/writes the per-element-kind pinned-groups map added to
studioUiPreferences in the prior task, read-modify-writing the whole
map since writeStudioUiPreferences only shallow-merges top-level keys.
Also adds a not-busy click assertion for the Apply button so
onApplyToScope is exercised end-to-end, not just its disabled state.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Also hoists the pointerdown-drag/reset-click test scaffold shared by the
new Roundness tests and the existing Contrast/Exposure tests into helpers
(findRowByText/dragSliderTrack/clickSliderReset), and exempts pre-existing,
branch-inherited fallow findings unrelated to this task (TextFieldEditor
complexity from earlier Text-inspector commits; test-scaffold duplication
across four flat-inspector-series test files from Plans 2-4) via
.fallowrc.jsonc, per this repo's established convention for line-shift/
inherited findings.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Review of Task 6 flagged the exposure/other-key scale ternary as always
resolving to 100 on both arms.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Vance Ingalls