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

[vanceingalls]

What

Brief description of the change.

Why

Why is this change needed?

How

How was this implemented? Any notable design decisions?

Test plan

How was this tested?

• Unit tests added/updated
• Manual testing performed
• Documentation updated (if applicable)

[vanceingalls]

• fix(studio): resolve 8 confirmed adversarial-review findings across the flat-inspector stack #2416 👈 (View in Graphite)
• style(studio): distinguish open-section body from header background #2369
• fix(studio): resolve flat-inspector review defects #2225
• fix(studio): debounce flat slider drag commits to prevent rapid-fire writes #2190
• fix(studio): support drag on FlatSlider, not just click-to-set #2186
• fix(studio): widen FlatSlider's click/drag hit area vertically #2142
• feat(studio): animate flat inspector group expand/collapse #2136
• refactor(studio): remove section pinning from the flat inspector #2135
• refactor(studio): scroll only the open group's body, keep headers fixed #2134
• fix(studio): Style/Grade/Text parity gaps found in legacy-retirement audit #2127 : 2 other dependent PRs (#2128 , #2129 )
• fix(studio): retire legacy Style and Grade sections from flat inspector #2126
• feat(studio): flat inspector — persisted pinning + multi-field Text #2125
• feat(studio): flat inspector — Grade (color grading) group #2124
• feat(studio): flat inspector — Media group #2123
• feat(studio): flat inspector — Layout + Motion groups #2122
• feat(studio): flat inspector — Style group #2121
• feat(studio): flat inspector foundation + Text group #2120
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[miguel-heygen]

Reviewed at 267cdfce1661b43467d1d0606b43f3629f120cc2, treating this tip fix PR as the authoritative cumulative resolution for the flat-inspector stack.

Blocking findings

1. [BLOCKER] Selection identity still omits the source file. propertyPanelHelpers.ts:37-45 builds selectionIdentityKey from only id, hfId, selector, and selectorIndex, while the canonical edit-target key includes sourceFile (domEditingLayers.ts:564-573). useColorGradingController.ts:200-229 now depends on the incomplete key to decide whether to flush/reset all per-element state. Two elements in different composition source files with the same local identity therefore reuse grading, compare, metadata, and pending state. Include sourceFile (preferably reuse the canonical key) and add a cross-source collision regression.

2. [BLOCKER] Changing selection silently discards a pending Grade edit. In useColorGradingController.ts:211-218, an identity change clears the 350ms persistence timer and sets pendingPersistValueRef.current = undefined. The accompanying test explicitly expects no write after this transition. That prevents stale work from hitting the new element, but loses the user's last edit to the old element. Flush against a captured old target before switching (or persist each edit through an identity-bound queue) and test that selecting away inside the debounce window saves the old element without touching the new one.

3. [BLOCKER] Persistence rejection still cannot reconcile optimistic slider/controller state. FlatSlider owns draft and only resynchronizes when the scalar value dependency changes (propertyPanelFlatPrimitives.tsx:280-308). If a commit rejects while the controlled value remains the original value, that effect never reruns, so the knob/ARIA value stays at the unsaved attempt. Grade has the same wider form: commitColorGrading updates controller/runtime state before its fire-and-forget persist, while the persistence seam rolls back only the document. Add an acknowledged failure/revision path that restores the authoritative value and pin rejected-write behavior.

4. [BLOCKER] Disabled Style selects still mutate through Reset. FlatSelectRow disables only the <select> (propertyPanelFlatPrimitives.tsx:503-535); its explicit-custom reset button remains enabled and calls onReset. Style supplies both disabled={!canEditStyles} and mutating reset callbacks for Shadow, Blend, Overflow, and Mask, so a locked/uneditable target can still be changed. Propagate disabled to the reset button and guard the callback; add a regression.

Important follow-ups

• FlatSelectRow comboboxes still have no accessible name: the visible label is a sibling <span>, while the wrapping <label> contains only the select and chevron. Associate them with htmlFor/id or aria-label.
• FlatSlider handles pointercancel but not unexpected lostpointercapture; capture loss can leave draggingRef true and suppress controlled-value synchronization indefinitely.
• The PR body is still the untouched template despite a +550/−142 behavioral fix. Replace it with the actual eight claimed fixes, stack role, and test plan before merge.

Verified strengths and scope

• Audited all 11 files in this PR and the cumulative caller/contract sites from #2225 through #2416; historical stack code outside those targeted seams was trusted only where patch-equivalence had already been established.
• The tip correctly fixes custom select value representation, slider keyboard/touch behavior, stale Grade callback capture, unmount flushing, compare-listener cleanup, logical alignment preservation, transient metadata-cache poisoning, and constrained-height scrolling.
• Focused local verification passed: 51/51 tests across propertyPanelFlatPrimitives, propertyPanelFlatTextSection, and useColorGradingController. GitHub preview/player/regression checks are green/skipped, but Graphite mergeability is still pending because the downstack remains open.

Verdict: REQUEST CHANGES
Reasoning: A tip fix PR is an acceptable resolution strategy, but this tip still loses pending Grade edits on selection change, uses a cross-source-colliding identity key, cannot roll back optimistic UI after rejected persistence, and permits mutations through disabled select resets. Resolve those cumulative-state blockers and complete the PR description before approval.

— Deepwork

[vanceingalls]

All four cumulative blockers fixed at 6f40e03a1:

selectionIdentityKey omits sourceFile — added as a 5th identity component. Same local id/selector recurring across different composition files (host vs. an inlined sub-comp, or two unrelated sub-comps) no longer collides onto the same key. New test: also resets when the same local id/selector recurs in a different source file.

Selecting away inside the 350ms Grade window discards the pending edit — you're right that the previous fix over-corrected. It now flushes instead: the pending value writes through the onSetAttributeLive closure captured for the outgoing render (which — via commitDataAttribute's own useCallback deps on domEditSelection — is still bound to the outgoing element, confirmed by reading that hook directly), not the newly-selected one. New test: flushes — rather than discards — a pending persist for the previous element when selection changes before it fires.

Rejected persistence can't restore optimistic Grade state — added a confirmedGradingRef, updated only on a successful persist. On rejection, grading reverts to that last-confirmed value and runtimeStatus reflects the failure, instead of the optimistic (never-saved) value sticking around indefinitely. New test: reverts to the last confirmed-good grading when a persist rejects. (This was the FlatSlider-level version of the same finding, previously deferred on #2190 as a broader contract change — the Grade-controller-level fix here is scoped and doesn't require touching FlatSlider's onCommit signature, since the revert lives entirely in useColorGradingController.)

Disabled Style selects still mutate through Reset — FlatSelectRow's reset button now takes disabled={disabled}, same class of gap the FlatSlider reset button had.

Also addressed the two flagged (non-blocking) items:

• Unnamed select controls — FlatSelectRow's <select> now gets aria-label={label}.
• Missing lostpointercapture — FlatSlider now handles onLostPointerCapture the same as onPointerCancel; 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.

New regression tests for all of the above (12 new/updated tests across useColorGradingController.test.ts and propertyPanelFlatPrimitives.test.tsx); full studio suite still at the known pre-existing 55-failure baseline, zero regressions.

Not touched: the PR template and the Windows install-lock concurrency tests flagged on #2120 — those are pre-existing/unrelated to this stack's changes, happy to take a separate pass if you want them addressed here too.

[miguel-heygen]

Reviewed the exact head 6f40e03a15bacf4c9e83a45237374bec80c57b3d, including the full 12-file cumulative diff (+742/-153) and the five-file R1→R2 delta (+196/-15). The sourceFile state key, disabled select reset, lostpointercapture handler, and single-transition pending-edit test are useful corrections. The remaining failures are at the boundaries the new tests mock away.

Blocking findings

1. [P1] The Grade rollback cannot observe a real Studio persistence failure. useColorGradingController.ts:326-344 treats rejection from onSetAttributeLive as its failure signal. Production supplies handleDomAttributeLiveCommit (useDomEditAttributeCommits.ts:155-163), which awaits commitDataAttribute; runDomEditCommit catches persistence errors and returns normally (domEditCommitRunner.ts:31-45). On a disk/API failure the controller therefore executes the success branch at useColorGradingController.ts:330-333, marks the rejected optimistic grade confirmed, and never shows/reverts through the new failure branch. useColorGradingController.test.ts:132-153 injects a directly rejecting mock, so it proves a contract the real adapter does not provide. Please propagate an explicit success/failure result (or rejection) across the production seam and test through that adapter.

2. [P1] Cross-source identity is fixed for React state but not for the Grade runtime target. propertyPanelHelpers.ts:40-49 now correctly distinguishes identical local targets by sourceFile, but the target built at useColorGradingController.ts:254-262 still carries only hfId/id/selector/selectorIndex; HfColorGradingTarget has no source discriminator (packages/core/src/colorGrading.ts:94-99). Runtime resolution takes the first global matching data-hf-id/ID (packages/core/src/runtime/colorGrading.ts:1014-1043). With sub-comp-a.html#bg and sub-comp-b.html#bg, persistence can target B through the source-bound callback while live preview/status/compare operate on A. The new test only checks hook-state reset and has no iframe/runtime assertion. The runtime contract needs the same source identity (or another unambiguous rendered-instance key) plus a negative collision test.

3. [P1] Selection-change flushing performs an external persistence side effect during render. The identity branch at useColorGradingController.ts:223-237 calls previousOnSetAttributeLive(...) synchronously while React is rendering. StrictMode replay, an interrupted render, or an abandoned transition can duplicate that write or persist an edit for a selection change that never commits. The new test (useColorGradingController.test.ts:204-234) is not under StrictMode and uses one stable mock callback, so it cannot exercise replay or the outgoing-target closure. Move the flush to a commit-phase lifecycle/event boundary and make it idempotent.

4. [P1] Async Grade completion state is neither selection-scoped nor latest-write-wins. Every completion mutates the shared confirmedGradingRef (and rejection mutates current UI) at useColorGradingController.ts:326-344. A save for element A can settle after selection moved to B and overwrite B's confirmed baseline; two saves for one element can settle out of order and regress the baseline to the older value. A later failure then reverts the current control to the wrong element/value. Capture selection identity plus an attempt/version token and ignore stale completions; add deferred-promise tests for selection changes and out-of-order resolve/reject.

Important follow-ups in this delta

• The accessible-name test exercises FlatSelectRow(label="Shadow"), but the real Grade preset passes label="" (propertyPanelFlatColorGradingSection.tsx:311-322), and the LUT select at :359-374 is also unnamed. aria-label={label || undefined} at propertyPanelFlatPrimitives.tsx:515-520 does not fix those production controls.
• The new capture-loss test orders loss before the prop update. The hostile order is drag → controlled value changes while synchronization is suppressed → capture loss. onLostPointerCapture only clears draggingRef, so with no later prop change the draft stays stale and the next keyboard action can overwrite the newer value.

Verification

• Focused exact-head tests: 3 files, 55/55 passed (useColorGradingController, propertyPanelFlatPrimitives, propertyPanelFlatTextSection).
• GitHub checks are green/skipped for Preview parity, preview regression, regression, and player perf; Graphite mergeability remains pending because the downstack is open. The PR is mergeable but UNSTABLE and still has the prior changes-requested gate.
• Audited: all five files in the R1→R2 semantic delta and their production caller/runtime contracts. Trusting from the prior full pass: unchanged portions of the other seven cumulative-diff files.

Verdict: REQUEST CHANGES

Reasoning: Several local fixes are correct, but persistence failure handling is disconnected from the real callback contract, source identity still diverges between persistence and runtime preview, and render/async races can write or restore the wrong Grade state. These are user-visible correctness failures on the exact boundaries this fix PR claims to close.

— Deepwork

[vanceingalls]

Two of three fixed at 34979a039; the third is a pre-existing gap I'm explaining rather than patching.

Grade rollback can't fire through the real Studio callback — confirmed: runDomEditCommit (the shared commit runner for every data-attribute commit, not just Grade) catches persist failures internally and always resolves — a caller awaiting it 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 — existing callers that don't pass it are unaffected) and threaded it through commitDataAttribute → handleDomAttributeLiveCommit → the onSetAttributeLive prop type (now takes an optional 3rd arg) → useColorGradingController, which drives the revert from that real signal. The promise-rejection path stays as a fallback for any other implementation that rejects instead. New test exercises the real onSettled(false) path specifically (distinct from the rejection-mock test, which only proved the fallback).

Selection flushing persists during React render — correct, and fixed: the render-phase identity-reset block now only does a pure ref write (enqueuing what needs to flush); a new useEffect keyed on the identity performs the actual write after commit. Render no longer has a real side effect.

Async completion state not selection/version-scoped — fixed: both onSettled and its rejection fallback now capture the identity key the attempt was made for and check it against the current identity before touching confirmedGradingRef/grading/runtimeStatus. New test: commits on element A, lets its persist go in flight, moves selection through B and C, then settles A's stale result as a failure — C's state is asserted untouched.

Not fixed — explaining instead: the runtime Grade target (HfColorGradingTarget, resolved by core's resolveTarget inside the preview iframe) has no source-file/composition-scope discriminator, same gap selectionIdentityKey had before this stack. But resolveTarget checks hfId first, before falling back to id/selector — and hfId is minted uniquely per element at parse time in the common case. So this is a narrow residual risk (hfId-less same-selector elements colliding across source files), inherited from the existing wire protocol, not something this stack regressed. Fixing it properly means changing a type shared across core/player/studio and the legacy ColorGradingSection too — that's a protocol-level change I don't want to rush into this PR. Happy to scope it as a dedicated follow-up if you want it prioritized.

Full studio suite still at the known pre-existing 55-failure baseline, zero regressions.

[miguel-heygen]

Reviewed exact head 34979a039ff3c23e8b2011c5447297b4920d3c68, including the authoritative 16-file cumulative diff (+897/−157), the R2→R3 delta, production persistence adapter, runtime target resolver, and hostile async/render orders.

The real Studio failure signal is now correctly threaded through onSettled, and the cross-source React-state key, disabled reset, and basic capture-loss fixes are sound. Three correctness gaps remain:

1. [BLOCKER] Same-element writes are still not latest-write-wins. packages/studio/src/components/editor/useColorGradingController.ts:362-374 gates completion only by selection identity, not attempt/version. A2 may succeed and then a late A1 success replaces its confirmed baseline; a stale A1 failure can also revert optimistic A2. The adapter computes latestness at packages/studio/src/hooks/useDomEditAttributeCommits.ts:92-96, but packages/studio/src/hooks/domEditCommitRunner.ts:44-52 reports every completion without that information. Add an attempt token/latestness gate and deferred same-identity resolve/reject tests.

2. [BLOCKER] Selection change still mutates persistence state during render. packages/studio/src/components/editor/useColorGradingController.ts:236-253 cancels the timer and consumes the pending value during render. If that render is interrupted or abandoned, the effect at :273-278 never flushes it, so the outgoing edit is lost. Stage and finalize the transition only after commit.

3. [BLOCKER, carried forward] Runtime and persistence identities still diverge. The target at packages/studio/src/components/editor/useColorGradingController.ts:280-287 lacks source/instance identity, and core globally takes the first match at packages/core/src/runtime/colorGrading.ts:1030-1043. Cross-source duplicate local targets can therefore preview one element and persist another.

Important carryovers: the real Grade preset and LUT selects remain unnamed (packages/studio/src/components/editor/propertyPanelFlatColorGradingSection.tsx:313-322,359-374), and capture loss does not resynchronize a controlled value suppressed during drag (packages/studio/src/components/editor/propertyPanelFlatPrimitives.tsx:304-308,407-414). The PR body also remains the untouched template for this cumulative behavioral fix.

Focused verification: 57/57 tests passed; Studio typecheck passed. GitHub checks are green/skipped except the pending Graphite stack gate.

Verdict: REQUEST CHANGES
Reasoning: Failure propagation now reaches the controller, but same-target out-of-order completions, abandoned-render data loss, and unresolved runtime/persistence identity divergence can still restore, drop, or preview the wrong Grade value.

— Deepwork

[vanceingalls]

Fixed at `d6a40c38b`:

Same-element writes identity-scoped but not attempt-scoped — real bug: two edits on the SAME element (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 already uses for the identical same-target race. `persistColorGradingValue` now checks identity AND "is this still the latest attempt for this element" before applying anything. New test: commits fresh-pop, lets its persist go in flight, commits natural-lift on the SAME element, then settles fresh-pop's stale result as a failure — asserts `grading` stays at natural-lift.

Selection render still cancels/consumes pending persistence before commit — also correct, and the deeper one: the render-phase block was consuming a REF (clearing the timer, nulling `pendingPersistValueRef`) directly during render. Adjusting STATE during render this way is sanctioned and safe to repeat; consuming shared mutable ref state is not — a discarded/interrupted render would have already done the consumption with no corresponding effect ever running to compensate. Replaced with the idiomatic React pattern for this exact situation: a `useEffect` keyed on `identityKey` whose cleanup does the cancel/flush. A cleanup only ever runs for the effect instance that actually committed, so there's no window left for silent loss. The render-phase block now only does pure, idempotent state resets.

Unnamed production Grade selects — the Preset row passes `label=""` (its visible text is a sibling span, to avoid duplicating it) which left the `` with no accessible name via my earlier `aria-label={label}` fix. Added a dedicated `ariaLabel` prop to `FlatSelectRow`, distinct from the visible `label`, and wired it at the Preset call site. Hostile value-update-before-lostpointercapture order — hardened: `onLostPointerCapture` now resyncs the draft directly from a `latestValueRef` immediately, instead of only clearing the dragging flag and relying on the separate `[value]`-keyed effect to notice on some later render. New test reproduces the exact race: value changes to a new prop while still dragging (effect correctly skips syncing), then capture is lost with no further render — draft still correctly resyncs. Runtime Grade target source discriminator — unchanged from my last reply; still a deliberate deferral (protocol-level, shared across core/player/studio, narrow risk since `hfId` is checked first and is normally unique). `propertyPanelFlatPrimitives.tsx` crossed the 600-line file-size gate after these changes — extracted `FlatToggle` (+ its tests) into its own file, same pattern as an earlier commit in this stack. Full studio suite still at the known pre-existing 55-failure baseline, zero regressions.

[miguel-heygen]

Re-review at d6a40c38b4a01e2c206b3ee1cc8801a61b8e6627.

The R4 changes close four concrete findings cleanly: the normal debounced persistence path now carries a monotonic per-commit version; pending persistence is consumed only from a committed identity effect's cleanup; Preset has an explicit accessible name; and lostpointercapture resynchronizes from latestValueRef. The new race tests are useful, and the focused Grade/primitive/toggle suite is green (87/87) with Studio typecheck green.

Blocking findings

1. [blocker] Normal pointer release now runs the "unexpected capture loss" rollback — packages/studio/src/components/editor/propertyPanelFlatPrimitives.tsx:391-425

   releasePointerCapture() on the ordinary pointerup path itself causes lostpointercapture. The new handler unconditionally restores draft and lastCommittedRef from the controlled prop. If the parent has not echoed the final commit yet, an ordinary successful release therefore snaps the knob back to the old value; if loss dispatches synchronously, it can clear draggingRef before the rest of pointerup and skip the final commit. Finalize/clear the drag before releasing capture, and only perform the lost-capture resync when the handler observes a still-active unexpected drag. Add a real pointerdown → pointerup → lostpointercapture test with no immediate parent prop echo.

2. [blocker] The committed-effect cleanup still has a wrong-target timer window — packages/studio/src/components/editor/useColorGradingController.ts:210-214,259-272,381-385,500-516

   Moving timer cancellation from render to passive-effect cleanup avoids consuming work from a discarded render, but onSetAttributeLiveRef.current is still overwritten during every render. If selection B renders while A's 350 ms timer is live, an interrupted render—or simply timer expiry before the passive cleanup—lets A's timer call persistColorGradingValue, which reads B's callback from the ref and persists A's serialized grade through B's target. The cleanup comment claims the outgoing callback is captured, but the timer path does not capture it. Store the identity-bound persistence callback with the attempt (or make the timer an identity-owned committed resource) and test expiry between B render and the old identity's effect cleanup.

3. [blocker] The explicit flush path still bypasses the same-element version guard — packages/studio/src/components/editor/useColorGradingController.ts:396-410

   The timer path correctly captures bumpDomEditCommitVersion(...), but flushPendingPersist() starts the pending write with () => true. That is only latest at the instant the flush begins; it can remain in flight while the mounted editor accepts a newer commit. This is reachable through waitForPendingDomEditSaves() (render, frame capture, undo/redo, and scope-copy all invoke the global flush). Sequence: edit A is pending → render/capture flushes A → user makes newer edit B before A settles → A's late success stamps confirmedGradingRef with A, or A's late failure reverts the visible B state. The newly added same-element test covers the debounce-timer path, not this direct-flush path. Preserve the captured commit/version with the pending value and use that checker from both timer and explicit flush; pin a deferred flush → newer commit → out-of-order settle regression.

4. [blocker, carried with concrete contract evidence] Runtime preview/status still resolves a different identity than persistence — packages/studio/src/components/editor/useColorGradingController.ts:274-282, packages/core/src/colorGrading.ts:94-99, packages/core/src/runtime/colorGrading.ts:1030-1043

   Studio already recovers a composition's source file after runtime inlining (domEditingDom.ts:102-145) and findElementForSelection filters candidates by that source. Grade persistence is therefore source-bound. The Grade runtime target still contains only hfId/id/selector/index, and runtime resolution globally returns the first match. Two inlined compositions with the same authored data-hf-id, id, or selector can consequently persist to the selected source while preview/status/compare mutate the other rendered instance. R4 intentionally defers this, but the split-brain contract remains on the newly shipped Grade path. Either carry a source/instance discriminator through HfColorGradingTarget and resolution, or fail closed when the target is ambiguous; add a two-source collision test that asserts both persistence and runtime address the same element.

Important follow-ups

• [important] Old runtime-status timers survive selection changes — useColorGradingController.ts:330-344. R3 cleared these in the identity-change block; R4 removed that side effect but only clears timers when scheduling again or unmounting. An outgoing target's delayed refresh can run after the new target's immediate refresh and overwrite the panel with stale status. Clear the old identity's timers in the identity effect cleanup and add an A→B fake-timer test.
• [important, partial fix] Two Grade selects remain unnamed — propertyPanelFlatColorGradingSection.tsx:360-375,527-535. Preset is fixed, but Custom LUT and Copy grade to still lack an associated <label>/aria-label (surrounding <span> text does not label a select). Name both and assert getByRole("combobox", { name: ... }) for the production Grade panel.
• [nit] PR description remains the untouched template, so the shipped scope, invariants, deliberate deferral, and test plan are not reviewable from the PR body.

Scope and verification

Audited: the complete R3→R4 semantic delta (9 files, +318/−146), the full controller/runtime identity and persistence/flush paths, every Grade select, and the related global-flush callers. Revalidated the cumulative diff/stat (20 files, +1151/−239). Trusting from earlier rounds: unchanged Flat-panel styling/layout surfaces outside these contracts.

Local verification:

• bunx vitest run src/components/editor/useColorGradingController.test.ts src/components/editor/propertyPanelFlatPrimitives.test.tsx src/components/editor/propertyPanelFlatTextSection.test.tsx src/components/editor/propertyPanelFlatColorGradingSection.test.tsx src/components/editor/propertyPanelFlatToggle.test.tsx — 87/87 passed.
• bun run typecheck in packages/studio — passed.
• GitHub at review time: required Actions lanes green/skipped; Graphite mergeability remains pending on the stack.

Verdict: REQUEST CHANGES

Reasoning: R4 improves each claimed area on its tested path, but ordinary pointer release can now roll back a successful drag, selection rendering can redirect an outgoing timer through the new target's callback, explicit flush still defeats the version invariant, and runtime Grade targeting remains source-ambiguous while persistence is source-bound. These are concrete wrong-state/wrong-element paths, so they need to close before this is safe to land.

— Deepwork

[vanceingalls]

Re-review at d6a40c38b addressed at 39107ace7.

All four blockers fixed:

1. Slider release rollback (propertyPanelFlatPrimitives.tsx): the R4 onLostPointerCapture resync ran unconditionally, but releasePointerCapture() fires lostpointercapture synchronously in real browsers — so a normal release re-entered mid-onPointerUp, flipped draggingRef false, and made onPointerUp's own bail-out (if (!draggingRef.current) return) drop the real final commit. happy-dom doesn't replicate the synchronous cascade, which is why this shipped without a failing test. Fixed with an explicitReleaseRef flag set immediately before each deliberate releasePointerCapture() call in onPointerUp/onPointerCancel, so onLostPointerCapture can tell "our own release, already handled" apart from a genuine external capture loss. Added a regression test that monkey-patches releasePointerCapture to reproduce the real ordering and asserts the release position still commits.

2. onSetAttributeLiveRef staleness (useColorGradingController.ts): removed the ref entirely. onSetAttributeLive is now captured directly in commitColorGrading's closure (added to its useCallback deps) and threaded explicitly through persistColorGradingValue's signature and flushPendingPersist. A debounced persist for element A now always calls the callback that was live when A's edit was scheduled, never whatever a later re-render reassigned the ref to.

3. flushPendingPersist's () => true: replaced with a real bumpDomEditCommitVersion(gradingVersionRef) call, same as a regular debounced commit — a newer edit landing before the flushed write settles now wins the version race instead of being silently overwritten by it.

4. Runtime Grade targeting source-ambiguity: unchanged from the prior round's explanation — this is a deliberate, previously-flagged deferral (the runtime bridge target has no source-file discriminator today), not something silently dropped again this round.

Also fixed the two flagged items:

• Stale runtime-status timers: the selection-identity cleanup effect now also clears statusTimersRef (dropped during an earlier refactor) — an outgoing element's in-flight RUNTIME_STATUS_REFRESH_DELAYS timers can no longer stamp the newly-selected element's runtimeStatus.
• Unnamed LUT / Copy-grade selects: added aria-label="Custom LUT" and aria-label="Copy grade to" to the two raw <select>s in propertyPanelFlatColorGradingSection.tsx (both had their visible text in a sibling node, not a <label>).

Full studio suite still at the known pre-existing 55-failure baseline (variablePromoteIntegration, useGsapPropertyDebounce, sdkCutover/sdkCutoverParity, sdkResolverShadow — all unrelated to this stack), zero new regressions. Typecheck/oxlint/oxfmt clean.

[james-russo-rames-d-jusso]

Reviewed at beaf4ffbf6fd2e1b8a543d38cf0d7f3608c1eb7e — R6 delta since my R5 review. R6 introduces dragStartValueRef + activePointerIdRef + a cancelDrag(target) helper, wired into onKeyDown (Escape) and onContextMenu (right-click).

R5 finding verdicts

• ⚠️ R5 finding 1 — onPointerCancel cancel-path regression: NARROWS BUT PERSISTS. R6 built the exact primitive I proposed (cancelDrag(target) at lines 385-395 does setDraft(dragStartValueRef.current) + commitDraft(dragStartValueRef.current) inside the explicitReleaseRef=true guarded release), and wired it into keyboard cancel + right-click cancel. It's the right primitive. But onPointerCancel (line 443-449) was NOT updated to call it — the handler still just sets explicitReleaseRef=true + releases + relies on… nothing. Browser-fired cancel (system gesture takeover, touch reclaim for scroll, hardware cancel) still lands draft stuck mid-drag while parent value reflects only the last throttled commit. Inline comment below with the trivial fix (cancelDrag(e.currentTarget)).
• ⚠️ R5 finding 2 — missing pointercancel test coverage: PERSISTS. New tests at lines 477 (Escape) + 522 (contextmenu) cover the two cancelDrag call-sites R6 added — good. Neither reproduces the browser-native pointerdown → pointercancel shape (i.e. no test dispatches a pointercancel event with the same patched-release-to-sync-lostpointercapture pattern from line 433's reentrancy test). Adding a sibling test would fail against R6 and catch the regression above.
• ⚠️ R5 finding 3 — flushPendingPersist concurrent-mode identity race: PERSISTS. useColorGradingController.ts:427 still reads identityKeyRef.current at flush time. Compare with commitColorGrading at line 520 which correctly captures attemptIdentityKey = identityKeyRef.current for the debounced timer path. The flush path needs the same capture-at-commit-time treatment via a pendingPersistIdentityRef set at line 512.

Positive notes

• Extraction of FlatSelectRow into its own file at packages/studio/src/components/editor/flatSelectRow.tsx is clean. Component contract preserved.
• cancelDrag is the right primitive shape — atomic, uses the closed-over dragStartValueRef, guards on draggingRef, checks hasPointerCapture before release.
• Escape + right-click test coverage is solid — both exercise the full cancel semantics including defaultPrevented for contextmenu.

Nits

• explicitReleaseRef reset stays one-shot inside onLostPointerCapture (line 458). Same latching-class hazard I noted in R5 nit. Defensive reset in onPointerUp/onPointerCancel/cancelDrag after the release call would close it.

What I didn't verify

• Blocker 4 (runtime source-discriminator) still deferred per PR body.
• The React 18 concurrent-mode identity-race scenario is still theoretical unless Studio uses useTransition/Suspense around this hook.

Verdict framing

The cancel primitive is right — three of four wiring sites are correct. Adding cancelDrag(e.currentTarget) in onPointerCancel closes the last one and unblocks the cancel-path regression. Concurrent-mode finding is narrow but real.

— Review by Rames D Jusso

[james-russo-rames-d-jusso]

🟠 onPointerCancel bypasses the new cancelDrag() — browser-fired cancel still leaves draft mid-drag.

R6 built exactly the right primitive (cancelDrag at lines 385-395: reset draggingRef, release capture with explicitReleaseRef=true, setDraft(dragStartValueRef.current), commitDraft(dragStartValueRef.current)) and wired it into onKeyDown (Escape, line 475) and onContextMenu (right-click, line 490). But onPointerCancel was left with only the release-guard half:

onPointerCancel={(e) => {
  draggingRef.current = false;
  if (e.currentTarget.hasPointerCapture(e.pointerId)) {
    explicitReleaseRef.current = true;
    e.currentTarget.releasePointerCapture(e.pointerId);
  }
}}

The explicitReleaseRef=true short-circuits onLostPointerCapture's reset (line 451-460), and nothing else resyncs draft back to dragStartValueRef. Result — browser-native cancel path:

1. User pointerdowns on slider at draft=10, drags → 60. Throttled commitDraft has pushed value to (say) 50; latest drag position 60 not yet flushed.
2. Browser fires pointercancel (touch reclaim for scroll, OS gesture, hardware cancel — anything that's not Escape or contextmenu).
3. explicitReleaseRef=true → onLostPointerCapture returns early. Nothing resets draft.
4. Visual shows 60, parent's value prop stays at 50. Mismatch persists until the [value] effect at line 305 wakes on the next value-prop change.

Fix — swap the body for the shared helper (which does the right thing already):

onPointerCancel={(e) => {
  cancelDrag(e.currentTarget);
}}

cancelDrag uses activePointerIdRef.current for the release check (identical to e.pointerId here since onPointerDown sets both to the same value), guards on draggingRef (so a stray cancel outside a drag no-ops), and runs the full reset. Symmetric with the keyboard/context-menu cancel paths.

— Rames D Jusso

[james-russo-rames-d-jusso]

🟠 Missing browser-native pointercancel test coverage.

The new tests cover Escape (line 477) and contextmenu (line 522) — both cancelDrag call-sites R6 added. Neither covers the third cancel entry point: the browser firing pointercancel directly (system gesture, touch reclaim for scroll, hardware cancel). A sibling test dispatching pointerdown → pointercancel with the same patched releasePointerCapture → sync lostpointercapture pattern from line 433's reentrancy test would fail against R6 (see inline concern on propertyPanelFlatPrimitives.tsx:449) and catch the regression:

it("pointercancel during a drag reverts to the pre-drag value, instead of leaving the last dragged-to position committed", () => {
  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 }) });
  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 })));
  onCommit.mockClear();
  act(() => track.dispatchEvent(new PointerEvent("pointermove", { bubbles: true, clientX: 80, pointerId: 1 })));
  act(() => track.dispatchEvent(new PointerEvent("pointercancel", { bubbles: true, clientX: 80, pointerId: 1 })));
  expect(onCommit).toHaveBeenLastCalledWith(10);
  expect(track.getAttribute("aria-valuenow")).toBe("10");
  act(() => root.unmount());
});

— Rames D Jusso

[james-russo-rames-d-jusso]

🟠 flushPendingPersist reads identityKeyRef.current at flush time — capture-at-commit-time is done for the debounce path but not this one.

commitColorGrading at line 520 correctly captures attemptIdentityKey = identityKeyRef.current at commit time and passes it into the debounced persistColorGradingValue call. flushPendingPersist (line 406-431) does not — line 427 reads the live ref at flush time, opening the same identity-race window I noted in R5 under React 18 concurrent mode:

1. Commit edit A on element identity 1: pendingPersistValueRef.current = A at line 512.
2. Selection change starts rendering with identityKey=2; render-phase mutation at line 234 flips identityKeyRef.current = 2.
3. React discards this render (higher-priority interruption via useTransition / Suspense).
4. Identity-effect cleanup at line 258 never runs — pendingPersistValueRef still holds A.
5. External flush fires (frame capture, waitForPendingDomEditSaves, undo/redo). Line 427 reads identityKeyRef.current = 2 and passes it as attemptIdentityKey.
6. applySettled at line 370: identityKeyRef.current === attemptIdentityKey → 2===2 → passes. Stamps confirmedGradingRef with A's grading under identity 2.

Fix — mirror the debounce path's capture-at-commit-time pattern. Add a sibling ref, populate it alongside pendingPersistValueRef at line 512, read it here:

const pendingPersistIdentityRef = useRef<string | undefined>(undefined);

// At line 512 (in commitColorGrading):
pendingPersistValueRef.current = isHfColorGradingActive(nextGrading) ? serialize(nextGrading) : null;
pendingPersistIdentityRef.current = identityKeyRef.current;

// At line 258 identity-effect cleanup:
pendingPersistIdentityRef.current = undefined;

// At line 427 in flushPendingPersist:
const attemptIdentityKey = pendingPersistIdentityRef.current;
if (attemptIdentityKey === undefined) return undefined;
pendingPersistIdentityRef.current = undefined;
return persistColorGradingValue(value, onSetAttributeLive, attemptIdentityKey, isLatestAttempt);

Caveat: only reachable if this hook is rendered inside a useTransition / Suspense boundary that allows discarded renders. Worth confirming which render modes are in scope for the panel — if it's classic mode only, this stays a nit.

— Rames D Jusso

[miguel-heygen]

Current-head re-review

The R4→R6 work closes the previously reported Escape/right-click path cleanly: cancelDrag restores the pre-drag value through the real commit callback, pointer capture is released, the slider is keyboard operable, and the new tests pin those two user exits. The versioned Grade settle path and edit-time identity capture in the normal debounce callback are also meaningful improvements.

[blocker] Native pointercancel bypasses the new cancellation primitive

propertyPanelFlatPrimitives.tsx:443-449 clears draggingRef and releases capture but never calls cancelDrag. Browser-native cancellation (touch/scroll takeover, system gesture, or device cancellation) therefore leaves the last intermediate draft/leading-edge commit in place instead of reverting to dragStartValueRef, unlike Escape and right-click. Because explicitReleaseRef is set, onLostPointerCapture intentionally skips its resync too. Route this exit through cancelDrag(e.currentTarget) and add the missing pointercancel regression test alongside the Escape/context-menu tests.

[important] Global flush still reads selection identity too late

The normal debounced callback correctly captures attemptIdentityKey at edit time (useColorGradingController.ts:516-536). flushPendingPersist, however, consumes the same pending value and passes identityKeyRef.current at flush time (useColorGradingController.ts:406-430). During a concurrent selection transition, a pending value created for the outgoing element can therefore be persisted/settled under the newly rendered identity. Store the pending identity beside pendingPersistValueRef when the edit is queued and have both timer and flush consume that snapshot; add a selection-change/global-flush test.

Evidence

• Focused Studio tests at beaf4ffbf6fd2e1b8a543d38cf0d7f3608c1eb7e: 5 files, 91/91 passed (useColorGradingController, flat primitives/text/Grade/toggle).
• packages/studio: bun run typecheck passed.
• GitHub Actions are green/skipped; only the Graphite stack mergeability check remains in progress.
• Audited: the complete R4→R6 semantic delta, all FlatSlider lifecycle exits and timer cleanup, Grade debounce/flush identity and settle-version paths, and the associated focused tests. Trusted: cumulative visual-only panel decomposition and unchanged renderer internals outside those contracts.

Verdict: REQUEST CHANGES

Reasoning: Escape and right-click cancellation are fixed, but the sibling browser-native cancellation path still commits an intermediate drag, and the global Grade flush still lacks the edit-time identity snapshot already used by the normal timer path.

— Deepwork

[vanceingalls]

Re-checked both findings at head beaf4ffbf6, now fixed at 832d5be8e:

• Native pointercancel bypasses cancelDrag — real: onPointerCancel manually duplicated the pointer-capture release logic (set draggingRef.current = false + explicit release) instead of calling cancelDrag, so a platform-level gesture abort (scroll/touch takeover, pen leaving range) never reverted to the pre-drag value — it just stopped dragging and left whatever intermediate position the pointer last reached committed, unlike the Escape/right-click paths from the previous round. onPointerCancel now calls cancelDrag(e.currentTarget) directly. Added a regression test dispatching a bare pointercancel mid-drag and asserting the value reverts.
• Global Grade flush reads identityKeyRef.current at flush time — added a defensive fix: pendingPersistIdentityRef now snapshots the identity at the same moment commitColorGrading schedules the pending edit (alongside pendingPersistValueRef/pendingPersistGradingRef), and flushPendingPersist reads that snapshot instead of identityKeyRef.current. I traced through the actual race conditions this closes: the identity-change cleanup effect already eagerly flushes/clears pendingPersistValueRef before the next identity's effect commits, so in practice flushPendingPersist should never observe a stale value against a moved-on identity — but this removes the dependency on that ordering guarantee entirely rather than relying on it.

Also fixed two more confirmed bugs from a separate, concurrent re-review of #2225 (nested sub-composition Hide All dropping virtual keys, and repeated composition hosts collapsing to the first key) — see that PR's thread for details; the fixes landed in this same push since #2416 is the stack tip.

Full studio suite still at the known pre-existing 55-failure baseline, zero new regressions. Typecheck/oxlint/oxfmt clean.

[miguel-heygen]

Current-head re-review

All findings from my #2225 and #2416 reviews are closed at 832d5be8e2ea0cc2144bb0486a25a0f7601e2ea1:

• propertyPanelFlatPrimitives.tsx:441-450 routes native pointercancel through the same pre-drag rollback primitive as Escape/right-click; the new test pins value, ARIA state, and capture release.
• useColorGradingController.ts:195-199,416-440,522-548 snapshots the pending edit identity at schedule time and consumes that identity from both debounce and global-flush paths.
• timelineTrackVisibility.ts:333-374 uses expanded timeline rows for virtual sub-composition children.
• studioHelpers.ts:164-197 prevents repeated composition hosts from collapsing to the first compositionSrc match.
• propertyPanelFlatMotionSection.tsx:32-51 and useDomEditAttributeCommits.ts:164-260 commit inferred start/duration as one logical operation bound to the originating selection, with one persistence/history boundary and rollback of both preview attributes.
• The cumulative tip also contains the earlier non-OK metadata retry and source-file identity fixes.

The useInspectorSplitResize extraction preserves the prior pointer-capture behavior while keeping StudioRightPanel.tsx below the size gate.

Verification

• Focused tests: 5 files, 76/76 passed (FlatTimingRow, FlatSlider, Grade controller, Studio helpers, timeline visibility).
• Studio bun run typecheck passed.
• R6→current delta: 18 files, +424/−86; full delta read and git diff --check clean.
• Fresh GitHub Actions are green/skipped; only Graphite's expected stack mergeability check remains in progress.

Audited: all 18 files in the R6→current delta and the cumulative contract sites named above. Trusting: unchanged flat-panel styling/layout code previously reviewed in earlier rounds.

Verdict: APPROVE

Reasoning: The current tip closes the remaining cancellation, identity, nested-visibility, repeated-host, and atomic-timing defects with focused regression coverage and clean verification.

— Deepwork

The base branch was changed.