fix(studio): resolve 8 confirmed adversarial-review findings across the flat-inspector stack#2416
Conversation
…he 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).
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Reviewed at 267cdfce1661b43467d1d0606b43f3629f120cc2, treating this tip fix PR as the authoritative cumulative resolution for the flat-inspector stack.
Blocking findings
-
[BLOCKER] Selection identity still omits the source file.
propertyPanelHelpers.ts:37-45buildsselectionIdentityKeyfrom onlyid,hfId,selector, andselectorIndex, while the canonical edit-target key includessourceFile(domEditingLayers.ts:564-573).useColorGradingController.ts:200-229now 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. IncludesourceFile(preferably reuse the canonical key) and add a cross-source collision regression. -
[BLOCKER] Changing selection silently discards a pending Grade edit. In
useColorGradingController.ts:211-218, an identity change clears the 350ms persistence timer and setspendingPersistValueRef.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. -
[BLOCKER] Persistence rejection still cannot reconcile optimistic slider/controller state.
FlatSliderownsdraftand only resynchronizes when the scalarvaluedependency 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:commitColorGradingupdates 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. -
[BLOCKER] Disabled Style selects still mutate through Reset.
FlatSelectRowdisables only the<select>(propertyPanelFlatPrimitives.tsx:503-535); its explicit-custom reset button remains enabled and callsonReset. Style supplies bothdisabled={!canEditStyles}and mutating reset callbacks for Shadow, Blend, Overflow, and Mask, so a locked/uneditable target can still be changed. Propagatedisabledto the reset button and guard the callback; add a regression.
Important follow-ups
FlatSelectRowcomboboxes still have no accessible name: the visible label is a sibling<span>, while the wrapping<label>contains only the select and chevron. Associate them withhtmlFor/idoraria-label.FlatSliderhandlespointercancelbut not unexpectedlostpointercapture; capture loss can leavedraggingReftrue 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, anduseColorGradingController. 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
…review Fixes the Deepwork tip re-review's four remaining blockers plus its additive findings: - selectionIdentityKey: add sourceFile as a 5th identity component. The same local id/selector can legitimately recur across different composition files (host vs. an inlined sub-composition, or two unrelated sub-comps) — without sourceFile, those collided onto the same identity key and reused stale controller state across a selection change that should have reset it. - useColorGradingController: flush (not discard) a pending Grade edit when selection changes before the 350ms debounce fires. The prior fix correctly stopped it from landing on the WRONG (new) target, but cancelling outright silently dropped the user's in-flight edit instead of writing it to the element it was authored for — using the onSetAttributeLive closure captured for the outgoing render, which (via commitDataAttribute's own useCallback deps) is still bound to the outgoing selection. - useColorGradingController: revert to the last confirmed-good grading when a persist rejects, instead of leaving the optimistic (never-actually- saved) value showing indefinitely. Tracks a separate confirmedGradingRef, updated only on a successful persist. - FlatSelectRow: disable the reset button when the row itself is disabled (it previously ignored disabled entirely, same class of bug as the FlatSlider reset button fixed earlier) and give the underlying <select> an aria-label from the row's label text. - FlatSlider: handle lostpointercapture the same as pointercancel — capture can be lost without either firing first (another element steals it, or the browser reclaims it for a scroll/touch gesture), which previously left the dragging flag stuck and the knob permanently unable to sync to external value changes. New regression tests for all of the above; full studio suite still at the known pre-existing baseline (55 failures unrelated to this stack).
|
All four cumulative blockers fixed at
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 Rejected persistence can't restore optimistic Grade state — added a Disabled Style selects still mutate through Reset — Also addressed the two flagged (non-blocking) items:
New regression tests for all of the above (12 new/updated tests across 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
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
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
-
[P1] The Grade rollback cannot observe a real Studio persistence failure.
useColorGradingController.ts:326-344treats rejection fromonSetAttributeLiveas its failure signal. Production supplieshandleDomAttributeLiveCommit(useDomEditAttributeCommits.ts:155-163), which awaitscommitDataAttribute;runDomEditCommitcatches persistence errors and returns normally (domEditCommitRunner.ts:31-45). On a disk/API failure the controller therefore executes the success branch atuseColorGradingController.ts:330-333, marks the rejected optimistic grade confirmed, and never shows/reverts through the new failure branch.useColorGradingController.test.ts:132-153injects 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. -
[P1] Cross-source identity is fixed for React state but not for the Grade runtime target.
propertyPanelHelpers.ts:40-49now correctly distinguishes identical local targets bysourceFile, but the target built atuseColorGradingController.ts:254-262still carries onlyhfId/id/selector/selectorIndex;HfColorGradingTargethas no source discriminator (packages/core/src/colorGrading.ts:94-99). Runtime resolution takes the first global matchingdata-hf-id/ID (packages/core/src/runtime/colorGrading.ts:1014-1043). Withsub-comp-a.html#bgandsub-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. -
[P1] Selection-change flushing performs an external persistence side effect during render. The identity branch at
useColorGradingController.ts:223-237callspreviousOnSetAttributeLive(...)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. -
[P1] Async Grade completion state is neither selection-scoped nor latest-write-wins. Every completion mutates the shared
confirmedGradingRef(and rejection mutates current UI) atuseColorGradingController.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 passeslabel=""(propertyPanelFlatColorGradingSection.tsx:311-322), and the LUT select at:359-374is also unnamed.aria-label={label || undefined}atpropertyPanelFlatPrimitives.tsx:515-520does not fix those production controls. - The new capture-loss test orders loss before the prop update. The hostile order is drag → controlled
valuechanges while synchronization is suppressed → capture loss.onLostPointerCaptureonly clearsdraggingRef, 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
UNSTABLEand 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
2 participants
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?