Commit Graph
3 Commits
Author SHA1 Message Date
Vance Ingalls e820e18092 fix(studio): atomic timing pin, expanded-list Hide All, repeated-host matching, pointercancel revert
Fixes real bugs from two independent re-reviews (#2225 @ 65954c3804,
#2416 @ beaf4ffbf6):

- FlatTimingRow's pinRange committed a pinned start+duration range through
  TWO sequential onSetAttribute calls. Each resolves domEditSelection fresh
  from current hook state, so a selection change between the two awaits
  could misdirect the second write at the newly-selected element instead of
  the one being edited, and a failure of just the second call left the pair
  half-applied (inconsistent inferred/explicit state). Added
  commitDataAttributes/handleDomAttributesCommit (mirroring
  onCommitAnimatedProperties's same-shaped fix for GSAP property batches):
  one PatchOperation[] persist call against an explicit, caller-supplied
  selection — not the "current" one — threaded through as the new optional
  onSetAttributes prop. pinRange uses it when provided, falls back to the
  old sequential behavior otherwise.

- Hide All silently dropped nested sub-composition children: a selection
  inside a sub-comp with no timeline-store entry of its own resolves to a
  virtual `sourceFile#domId` key (the fallback branch exists so the
  expansion hook can later resolve it via clipParentMap), but
  toggleTimelineElementHidden only searched the RAW store list, which never
  contains that key. useTimelineElementVisibilityEditing now resolves
  against useExpandedTimelineElements() instead, matching the track-based
  toggle's existing approach — the expanded list synthesizes a real,
  patchable TimelineElement (matching key/domId/sourceFile) for each visible
  child whenever its host is currently expanded.

- Two composition hosts importing the same sub-composition collapsed to
  the first one: findMatchingTimelineElementId ORed domId/selector/
  compositionSrc matches with equal priority in a single per-element scan,
  so `.find()` could stop at an EARLIER, unrelated host that merely shared
  the compositionSrc, before the scan ever reached the correct domId/
  selector match further down the list. Restructured to try domId, then
  selector, across the WHOLE list first; compositionSrc-only matching is
  now a true last resort for when neither identifies a specific element.

- FlatSlider's native pointercancel handler (a platform-level gesture abort
  — scroll/touch takeover, pen leaving range) manually duplicated the
  pointer-capture release logic instead of calling cancelDrag, so it never
  reverted to the pre-drag value — leaving whatever intermediate position
  the pointer last reached committed, unlike the Escape/right-click paths
  added in the previous round. Now calls cancelDrag directly.

- useColorGradingController's flushPendingPersist read identityKeyRef.current
  fresh at flush time rather than a value snapshotted when the edit was
  scheduled. Defensive fix: added pendingPersistIdentityRef, set alongside
  pendingPersistValueRef in commitColorGrading, read by flushPendingPersist
  instead of the live ref — closes the gap regardless of how unlikely the
  actual race is given the identity-cleanup effect's existing eager-flush
  behavior.

Two prior findings re-verified as already fixed further up this same
Graphite stack (not re-fixed here, per established stack-order handling):
metadata-cache negative-caching (267cdfce1) and cross-file
selectionIdentityKey (6f40e03a1), both landing after #2225's reviewed head.

StudioRightPanel.tsx crossed the 600-line file-size gate after wiring the
new onSetAttributes prop through; extracted the inspector split-pane resize
handlers (previously inlined) into their own useInspectorSplitResize hook.

New regression tests: repeated-composition-host resolution, atomic vs.
fallback pinRange commit paths, pointercancel revert. Full studio suite
still at the known pre-existing 55-failure baseline, zero new regressions.
Typecheck/oxlint/oxfmt clean.
2026-07-14 16:28:33 -07:00
Vance Ingalls 539e027b60 fix(studio): wire Grade rollback through the real commit path, scope async completions
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.
2026-07-14 16:28:33 -07:00
Miguel Ángel e6e0d97cc5 fix(studio): surface persist failures with toast and guarded revert (#1910)
* test(studio): add design-panel QA fixture and triage matrix

Fixture project covering all panel-editable element archetypes,
plus the QA findings matrix from the design-panel bug campaign.

* fix(studio): make canvas selection hit intended elements

- honor author pointer-events:none in hit-testing (was selecting invisible overlays)
- pause playback before mousedown sampling; fall back to hover selection on null resolve
- invalidate committed selection when the active composition changes
- double-click keeps selection and defers to multi-candidate click cycling

* fix(studio): close remaining selection-layer review findings

- hoverSelection fallback now wired at all 3 mousedown call sites (box-click,
  blocked-drag, plain overlay click) instead of just the overlay path
- pointer-events override detection reads computed style, not inline style,
  so a CSS-class opt-in (not just inline style=) on a descendant is honored
- defensively remove the pointer-events override before the group-fallback
  check too, closing a theoretical gap in the no-elementsFromPoint branch
- a click that resolves to nothing (dead-zone / deselect) no longer leaves
  playback paused if it was already playing

* fix(studio-server): child-scoped patch operations with batch abort

- PatchOperation gains optional childSelector/childIndex resolved under the matched parent
- pre-pass resolves every op target; any miss aborts the batch with matched:false, no partial write
- style-decl parsing extracted to sourceStyleMutation to stay under the file-size cap
- new ./source-mutation subpath export (mirrors ./finite-mutation)

* fix(studio): per-child patch op builders and persist-seam harness

- buildTextFieldChildLocator indexes over the parent's full same-tag child list
- buildTextFieldChildOperations emits per-field ops for same-shape multi-field edits
- SDK cutover declines child-scoped batches (hfId mapping would hit the parent)
- persist-seam integration harness drives real client ops through patchElementInHtml

* fix(studio): fail closed on unresolved text-field child index

buildTextFieldChildLocator guessed a synthetic field's position by
counting same-tag "child" fields elsewhere in the array whenever
sourceChildIndex was absent. That heuristic is unreachable today (the
count-mismatch guard in buildTextFieldChildOperations already refuses
add/remove edits before it's reached) but would silently locate the
wrong element for a future caller that wires up synthetic-field
support without also computing a real sourceChildIndex. Return null
instead so the caller falls back to the unsupported-structure path.

* fix(studio): surface persist failures with toast and guarded revert

- matched:false and persist errors toast, warn structurally, and revert the optimistic write
- reverts guarded by a per-property version counter so stale failures never stomp newer edits
- structural text-field edits refuse persist instead of writing escaped markup
- multi-field child edits persist via per-child ops; shared commit runner extracted

* fix(studio): revert data-attribute and html-attribute commits on persist failure

commitDataAttribute and handleDomHtmlAttributeCommit toasted on failure but
never reverted the optimistic attribute write, leaving the preview showing an
edit that never reached disk (the exact bug this PR closes for style commits).
Extracted into useDomEditAttributeCommits.ts (useDomEditTextCommits.ts was at
the file-size cap) and routed through runDomEditCommit with a per-target+
attribute version guard, mirroring handleDomStyleCommit.

* fix(studio): close coupled persist-hook review findings

Three findings from R2 review that must land together: a patch-rejection
toast doubled up with the generic persist-failure toast (StudioSaveHttpError
had no alreadyToasted marker), a failed prepareContent write (e.g. font-face
injection) reverted and re-toasted a change the server had already
persisted, and text-commit shouldRevert only rolled back on one narrow
error type instead of any persist failure.
2026-07-03 18:22:22 -07:00