fix(studio): stabilize manual drag targets#1393
Conversation
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
hyperframes#1393 — fix(studio): stabilize manual drag targets
Two real bugs targeted with deterministic fixes, not bandaids. The "drag started from stale hover candidate" path is yanked out entirely (correct root cause: a hover-promotion code path was racing the selected overlay), and the #stage-confusion case is handled by reading the composition root's own declared dimensions (data-width / data-height) instead of guessing. Tests cover both regressions. CI green where it matters. One concern on the root-layer heuristic's false-positive surface, two nits.
Concerns
• packages/studio/src/components/editor/domEditingRootLayer.ts:46-56 — the root-layer heuristic locks any direct-body-child whose rect approximately matches the composition's data-width/data-height. Test fixture uses a structural stage div with no other intent. But: a user-authored full-bleed background layer (<div id="hero" style="position:absolute; left:0; top:0; width:1920px; height:1080px"> — common pattern for hero-image scenes) would also match this heuristic and become silently un-draggable / un-resizable with the toast "The root composition defines the preview bounds." There's no escape hatch (no data-hf-allow-root-edit opt-out, no source-attribute check to distinguish "structural stage" from "fullbleed user layer"). Worth verifying with a fixture: does a real hf-keyframes-test composition have full-bleed user-authored layers that would now silently become read-only? If so, consider gating on a positive signal (e.g. element has no data-hf-id AND has no inline position: absolute) rather than just bounds match.
• packages/studio/src/hooks/useDomEditCommits.ts:296-307 — commitPositionPatchToHtml now rethrows from its .catch. Three callers (handleDomManualEditsReset:418, handleDomZIndexReorderCommit:517,532) are unawaited void ... .catch(() => undefined). That swallowing is fine for those use-cases, but it means save_failure telemetry already fired inside commitPositionPatchToHtml AND a second swallowed catch — slight risk of double-counted failures depending on where you read it. Not a blocker; just verify the save_failure telemetry counts you read in dashboards still mean what you think they mean.
Nits
• packages/studio/src/components/editor/PropertyPanel.tsx:175-180,205-211,218 — wrapping the void calls in Promise.resolve(...).catch(() => undefined) works but is the wordier shape. Since the callbacks are now typed Promise<void> | void, a more idiomatic version is Promise.resolve(onSetManualOffset(...)).catch(() => undefined) — which is what you did. Skip if intentional; the typing reads fine. (nit)
• packages/studio/src/components/editor/domEditingRootLayer.ts:34-44 — matchesCompositionRootBounds reads getBoundingClientRect() a second time (the size-resolver already called it once at line 25). Tiny: it's an additional layout sync inside the layer-resolution hot path. If the resolver runs per pointermove anywhere, fold the rect lookup into one read. Doesn't matter at typical selection cadence. (nit)
Questions
• Was the previous behavior — clicking an unselected, hoverable element with canApplyManualOffset and also dragging in the same gesture — used in the wild? With the removal, the new flow is two-gesture: click to select, then drag the selection box. That's stricter and matches what the test name says ("starts movement from the selected bounds"). Just confirming you've verified with a designer/PM that the two-gesture cost is acceptable — the old single-gesture path was probably how power users moved things fast.
• You add isCompositionRoot to the capability resolver's args but the existing isCompositionHost branch (master view) below it returns a separate capability shape with canEditStyles: false. The root branch returns canEditStyles: true. Is that intentional — root layer is style-editable but composition host in master is not? Worth a sentence in the heuristic file noting the asymmetry.
What I didn't verify
• Runtime / SDK / player interop on the new selection-box-only drag entry — that's Vai's lane.
• Coherence with hf-keyframes-test fixtures I don't have local. The PR body's browser verification (#stage drag writes no offset, #cta drag writes one offset, Undo returns to zero) is the right shape; I trust the recorded artifact pending a designer's sanity check on the new two-gesture flow.
• Cross-check vs. #1366 / #1389 / #1388 — all three are still OPEN against main and this PR is also against main, not stacked on them. So no rebase-coherence question; the commit pathway here (queueDomEditSave → persistDomEditOperations → fetch + patch + record) is the same shape #1366 will eventually wrap. The new Promise.reject returns from the commit handlers do propagate correctly through the save queue — which means once #1366 lands its circuit breaker, a string of GSAP-blocked or root-layer-blocked attempts could trip the breaker spuriously. Worth a comment between Miguel and whoever lands #1366 second to confirm the breaker classifies these UX blocks as non-server failures and doesn't count them toward open-state.
Review by Rames D Jusso
miguel-heygen
force-pushed
the
fix/studio-cta-drag-target
branch
from
2283383 to
db78359
Compare
3 participants
Problem
Studio manual drag had two bad target paths in
hf-keyframes-test:#ctacould write an offset to the wrong element while the overlay appeared correct.#stageis the visual 1920x1080 canvas, but the composition metadata lives on<html data-composition-id ...>. Studio treated#stageas a normal movable layer, so dragging it moved the coordinate basis the overlay depends on and left Undo with no reliable saved edit to reverse.Fix
hoverSelectionRef.data-width/data-heightis selectable and style-editable, but cannot receive manual offset/size/rotation edits.Tests
agent-browser: attempted#stagedrag writes no offset and keeps Undo disabled;#ctadrag writes exactly one offset to#cta; Undo returns the fixture to zero offsets.Local browser artifacts:
artifacts/studio-cta-drag/stage-cta-drag-fix.webm,artifacts/studio-cta-drag/stage-cta-drag-fix-final.png.