fix(studio): remove Ask agent popup, fix preview selection, fix manual edits in renderer#963
Conversation
…l edits in renderer Three interrelated studio UX and rendering fixes: 1. Remove the "Ask agent" popup that auto-triggered when clicking large raster elements in the preview. The modal intercepted clicks meant for editable elements and blocked normal selection workflow. 2. Rewrite preview click selection to respect visual stacking order. The previous scoring algorithm weighted DOM depth at 10,000× per level, causing elements inside sub-compositions to beat visually- on-top elements (e.g., clicking Pip Studio selected Sf Chrome instead). The new algorithm trusts elementsFromPoint order and only prefers a deeper candidate when it is a descendant of the current pick — never jumping to an unrelated element painted behind it. 3. Fix manual edits (resize) not surviving video rendering. The producer's seek-reapply script handled translate and rotation but was missing box-size (width/height) reapplication after each GSAP seek. Also added data-hf-studio-box-size to the detection list in htmlCompiler so the script is injected for resize-only edits.
…l edits When GSAP animates an element (e.g. scale, opacity), it captures the element's translate into its internal transform matrix (m41/m42). The render reapply script was setting the CSS `translate` property but leaving GSAP's translate baked into `transform`, causing the manual edit offset to be ignored or doubled. Port the same stripGsapTranslateFromTransform logic the studio uses: parse the transform matrix, zero out m41/m42, and remove or rewrite the transform property so the CSS `translate` takes effect cleanly.
miguel-heygen
force-pushed
the
worktree-fix+remove-ask-agent-popup
branch
from
a475b90 to
6498807
Compare
Fallow audit reportFound 87 findings. Duplication (69, showing 50)
Showing 50 of 69 findings. Run fallow locally or inspect the CI output for the full report. Health (18)
Generated by fallow. |
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed full files on the changed paths. Three well-scoped fixes; the visual-stacking-order selection algorithm + stripGsapTranslateFromTransform are the substantive pieces. A few additive notes focused on render-time determinism and test coverage of the new render-script logic — areas Vance / Vai are less likely to surface. Posting as COMMENT, not stamping.
Strong points
- Scoring algorithm cleanup in
domEditingElement.tsis the right direction. The previousgetElementDepth(el) * 10_000was a hardcoded depth weight that fundamentally broke for any visually-layered composition; replacing with "trustelementsFromPointorder, only swap for acontains()-descendant" matches the spec semantics. Dead-code removal (isEditableTextLeafForScoring,getVisualElementScore, thegetElementDepth+isTextBearingTagimports) is clean. stripGsapTranslateFromTransform— them41/m42baking is a real, subtle GSAP gotcha. The implementation correctly: parses DOMMatrix, returns early when no translate is baked, zeros, and removes-the-property-when-identity vs. rewrites-when-not.try/catchfor non-parseable transforms is appropriate. TheMatrixCtorresolution viael.ownerDocument.defaultViewcorrectly handles cross-realm DOMMatrix.- Removing the auto-popup is a clean UX revert — the manual "Ask agent" via context menu stays as the explicit path, which is the right tradeoff.
htmlCompiler.tsone-liner — addingdata-hf-studio-box-size="true"to the HF_POSITION_ATTRS detection list is the smallest possible fix for the "script not injected for resize-only edits" gap. ✓
Concerns worth addressing before merge
1. No new tests for the new render-script logic (important)
Test plan says "50 tests pass across domEditing.test.ts and manualEditsRenderScript.test.ts" — but those tests pre-date this PR. The newly-added code paths are not directly covered:
stripGsapTranslateFromTransform: the new DOMMatrix parsing is the highest-value test target — easy regression vector. Four cases worth pinning: (a)transform: "none"early-return, (b) identity matrix after zeroing m41/m42 → property removed, (c) translate-only matrix → property removed, (d) scale+translate → matrix rewritten with m41/m42 = 0 and other components preserved, (e) non-parseable transform → no-op (the catch path).- Box-size reapplication: the new
boxSizeElsloop has no test asserting width/height get reapplied from the CSS variables.
Manual "Rendered a composition with a manually-moved PiP video — position survives" is good signal but unverifiable without the render. Unit tests for these paths would have caught at least case (d) — the scale+translate scenario — which is the actual failure mode the PR describes (GSAP scaling baking translate into the transform matrix).
2. stripGsapTranslateFromTransform only applied to offset elements
Inside reapplyAll, the strip-call happens only inside the PATH_OFFSET_ATTR loop:
const boxSizeEls = ...; // no strip applied here
const rotEls = ...; // also no strip applied hereA real composition can have an element with both data-hf-studio-rotation="true" and data-hf-studio-path-offset="true" (drag + rotate). In that case the rotation branch runs second, after the offset branch already stripped the matrix — fine. But what about an element with only rotation + a GSAP-animated translate? The rotation reapplication would write transform: rotate(...) and the GSAP-baked translate could still clobber it. Worth tracing whether rotation-only elements need the same strip treatment, or whether they're guaranteed to also carry the PATH_OFFSET attribute when GSAP-animated.
3. Visual-stacking selection — backwards-compat for previously-tuned cases
The previous scoring had specific bonuses (isEditableTextLeafForScoring → +2000, smaller-element bonus, depth × 10000). Those weights were presumably tuned over time for specific element-picking cases. The new algorithm drops all of them. Worth manually verifying on a few existing compositions:
- A text leaf deeply nested under an absolute-positioned wrapper (e.g. caption inside a sub-comp inside a stacked layer)
- Z-indexed overlays where the topmost-in-DOM is not the topmost-visually
- SVG/canvas elements where
elementsFromPointreturns wrapper + child
If the PiP-over-Sf-Chrome trigger case is the only one verified, please flag any regression risk on text-leaf selection in the PR — that was the most-tuned bonus and a likely behavior-change vector.
4. Fallow audit — pre-existing complexity, but new stripGsapTranslateFromTransform is over threshold
Fallow flagged stripGsapTranslateFromTransform at CRAP 49.5 (threshold 30, cyclomatic 13). Not a blocker since the function is mechanically straightforward, but if you want to ship under the threshold the helper splits cleanly into: parseTransformMatrix(transform): DOMMatrix | null and applyMatrixWithoutTranslate(el, matrix): void. Worth doing if the audit gates merge; not worth doing if it's just a notice.
The other Fallow findings (duplication clusters, CRAP scores on parseSubCompositions / inlineSubCompositions / etc.) are pre-existing — none introduced here. The handlePreviewCanvasMouseDown CRAP 132 is amusing in that this PR actually reduces its complexity by removing the auto-popup branches; the audit score might re-baseline post-merge.
5. Possible dead export: isLargeRasterDomEditSelection
usePreviewInteraction.ts drops the import of isLargeRasterDomEditSelection (and getPreviewLocalPointer, buildRasterClickSelectionContext). Quick grep on the other call sites would tell you whether the exports themselves are now unreferenced. If they are, removing them keeps the diff complete; if other code still uses them, ignore this note.
Non-blocking
useDomEditSession.ts: the 4 props removed (preloadAgentPromptSnippet,setAgentPromptSelectionContext,setAgentModalAnchorPoint,setAgentModalOpen) — confirmuseAskAgentModalstill destructures them for the context-menu path (which stays). If yes, the destructure was just unused inuseDomEditSession's caller chain — fine.- The DOMMatrix
MatrixCtorresolution viael.ownerDocument.defaultViewcorrectly handles iframes/realms — nice touch.
Overall: three real, well-scoped fixes; biggest gap is the missing unit tests for the new render-script logic (specifically the matrix-translate-strip). The selection algorithm change is the highest behavior-change risk surface — the more compositions you can verify against before merge, the better.
— Rames Jusso
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Three independent studio fixes, each well-scoped within its file. The auto-popup removal is a clear UX win (the heuristic was overzealous on large-background compositions) and the manual context-menu Ask-agent path is preserved. The new visual-stacking-first selection algorithm in domEditingElement.ts is more principled than the old multi-factor score — the invariant "only descend into the current pick, never jump to an unrelated element painted behind" is correct and the comment makes that explicit. The stripGsapTranslateFromTransform port correctly handles the is2D identity case (remove the property cleanly) vs. preserving other matrix components.
Findings
important — Testing gap on the actual reported bug (fix 2). The existing resolveVisualDomEditSelectionTarget tests all pass under the new algorithm, but none of them exercise the bug scenario described in the PR body (PiP video painted on top of a sub-composition root that contains an Sf Chrome image as a sibling, where the deeper element should NOT win). The existing tests use ancestor/descendant pairs where the old depth-weighted score and the new descend-only algorithm both happen to pick the same target, so they don't pin the regression. Worth a minimal fixture: pointer at a coordinate where elementsFromPoint returns [pipVideo, subCompRoot, sfChromeImg] and pipVideo is a sibling-stack (not a descendant) of the others — assert it picks pipVideo. Easy to add and prevents this from silently regressing.
important — Testing gap on the renderer fix (fix 3). manualEditsRenderScript.test.ts only exercises createStudioManualEditsRenderBodyScript. The PR's actual fix is in createStudioPositionSeekReapplyScript — both the new box-size reapplication branch and stripGsapTranslateFromTransform. Neither has a test. The PR body claims "50 tests pass" — they do, but they don't cover what this PR added. The GSAP-clobbering case in particular is subtle (m41/m42 zeroed, identity-matrix-then-remove vs. preserve-then-rewrite); without a test, a refactor that drops the rewrite branch will land silently. Suggest at least one happy-dom test: GSAP-baked translate in transform, run reapplyAll on a data-hf-studio-path-offset element, assert the transform property no longer carries non-zero m41/m42.
important — Behavior change worth a one-line note. The old algorithm could prefer a smaller-area visual leaf even when it was not topmost in paint order (smaller-area bonus + visual-leaf bonus). The new algorithm strictly prefers topmost-and-descendants-of-topmost. Any workflow that relied on "click the large container painted on top to select the small inner leaf behind it" changes. The new behavior is almost certainly the one we want, but it's worth a sentence in the PR body and ideally a pinning test. The rubric calls this out — behavior changes should be explicit, not implicit.
nit — Dead exports after this PR lands. buildRasterClickSelectionContext and getPreviewLocalPointer in studioPreviewHelpers.ts lose their only production consumer in this PR. isLargeRasterDomEditSelection in domEditing.ts keeps its tests but no longer has a production caller. Knip should flag these; happy to be a follow-up cleanup PR.
nit — DOMMatrix cross-realm caveat. stripGsapTranslateFromTransform early-returns if DOMMatrix isn't on el.ownerDocument.defaultView. In the rendering iframe this is correct, but it means the function silently no-ops in realms without DOMMatrix — leaving GSAP's bake intact. Probably never matters in production but worth knowing if a happy-dom test is added later.
nit — Perf in reapplyAll. The new box-size loop runs on every seek even though width/height are static once applied. Mirrors the offset/rotation pattern so consistency is fine; just noting the cost compounds with element count. Could short-circuit if style.width === w already.
CI
Required checks green or in-progress on 6498807e: Build, Typecheck, Test, Test: runtime contract, Semantic PR title all SUCCESS; regression-shards (8 shards) and Windows render/Tests still IN_PROGRESS. Non-required Fallow audit is failing — but the only new function flagged is the introduced stripGsapTranslateFromTransform (CRAP 49.5, cyclomatic 13). It's a small function; the complexity is mostly the m.is2D+identity check. Not blocking, but easy to simplify with early returns.
Verdict: APPROVE
Reasoning: Three fixes are correct, scoped, and useful. The testing gap is real but the structural patterns mirror existing tested code closely enough that I'm comfortable shipping with a follow-up to backfill coverage rather than blocking.
Review by Vai
…rotation, remove dead exports Addresses review feedback from Rames and Vai: 1. Add 7 new tests for createStudioPositionSeekReapplyScript: box-size reapplication, GSAP translate stripping (identity removal, scale+translate preservation, transform:none no-op), and rotation- only elements with GSAP-baked translate. 2. Add pinning test for the PiP-over-sub-composition selection bug: elementsFromPoint returns [pipVideo, subCompRoot, sfChromeImg] as siblings — assert the topmost (pipVideo) wins. 3. Apply stripGsapTranslateFromTransform to rotation-only elements too, not just path-offset elements. A rotation-only element with a GSAP-animated translate would have its position clobbered. 4. Remove dead exports: getPreviewLocalPointer, buildRasterClickSelectionContext, getPreviewPlayer, seekStudioPreview, PreviewPlayerCompat, PreviewLocalPointer from studioPreviewHelpers.ts. Unexport resolvePreviewLocalPointer.
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Re-review on f7d63fbe. All five concerns from my prior pass + all of Vai's concerns are addressed:
- ✅ Six new unit tests in
manualEditsRenderScript.test.tsforcreateStudioPositionSeekReapplyScript— covers all the cases I flagged:reapplies box-size after seek(the box-size loop)strips GSAP translate from transform after reapplying path offset(the m41/m42 strip)preserves non-translate components when stripping GSAP transform(scale-preserved scenario)removes transform entirely when it becomes identity after stripping translate(identity →removeProperty)no-ops when transform is 'none'(early-return)strips GSAP translate for rotation-only elements(the rotation-branch concern I raised)
- ✅ Rotation branch now also calls
stripGsapTranslateFromTransform(el)(line ~252) — closes the "rotation-only + GSAP-baked translate" gap. The new test directly verifies this path. - ✅ Visual-stacking selection algorithm now has a regression test —
prefers the visually-on-top sibling over a deeper element in a separate visual layerindomEditing.test.tspins the exact PiP-over-Sf-Chrome bug shape Miguel found while stress-testing. As Vai noted, the previous selection tests used ancestor/descendant pairs that didn't discriminate old vs new logic — this one actually does. - ✅ Dead-code cleanup in
studioPreviewHelpers.ts—getPreviewLocalPointer,buildRasterClickSelectionContext,getPreviewPlayer,seekStudioPreview,PreviewPlayerCompatinterface,resolvePreviewLocalPointerall removed. The orphans Vai called out as a "fallow will catch them next sweep" follow-up are gone in this PR. Diff is meaningfully complete now. - ✅
.filesize-allowlistupdated to allowdomEditing.test.ts's growth from the new test — reasonable bookkeeping.
Non-blocking:
- Fallow
stripGsapTranslateFromTransformCRAP 49.5 (threshold 30) still present — non-blocking per Vai's verdict. The function splits cleanly into parse + apply if you ever want to clear it, but the test coverage now makes the complexity less worrisome.
CI at f7d63fbe: required green (Build, Test, Typecheck, Lint, CLI smoke, Preview parity, Windows render, CodeQL). Fallow audit still failing on the pre-existing complexity warnings + the now-tested stripGsapTranslateFromTransform — non-gating.
No further blockers from my side. Holding off on the stamp per merge policy; happy to defer to Vai's APPROVE and your final call.
— Rames Jusso
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Re-review on f7d63fbe (head moved from 6498807e).
Miguel addressed all three actionable findings from my prior review, plus the dead-export nit. Concrete verification:
Findings — re-verification
important — Selection-bug pinning test (fix 2): ADDRESSED. packages/studio/src/components/editor/domEditing.test.ts:325 adds a fixture that exercises the exact regression scenario — elementsFromPoint returns [pipStudio, subComp, sfChrome] as a sibling stack (PiP positioned absolute, z-index: 15, separate from sub-comp's child sf-chrome), and asserts pipStudio wins. This is precisely the case the old depth-weighted algorithm would have lost (deeper raster sf-chrome would have beaten the topmost smaller PiP), and the new descend-only algorithm correctly picks the topmost. Clean pin.
important — Renderer fix test coverage (fix 3): ADDRESSED. packages/core/src/studio-api/helpers/manualEditsRenderScript.test.ts:386 adds six new tests against createStudioPositionSeekReapplyScript:
- box-size reapply after seek (
width/heightrestored from CSS vars), - GSAP translate stripped, m41/m42 zeroed after path-offset reapply,
- non-translate matrix components preserved (scale 0.5 kept, m41/m42 removed),
- identity-after-strip → transform property removed entirely,
transform: noneis a no-op (no accidental write),- rotation-only path strips GSAP translate too.
Importantly, the test wires __hf.seek with a stub that bakes matrix(…, m41, m42) into transform, then asserts the wrapped seek strips it — that's exactly the GSAP-clobbering case I flagged. Solid coverage.
Bonus correctness fix (not in original PR). manualEditsRenderScript.ts:254 — stripGsapTranslateFromTransform is now also called for ROTATION_PROP elements, not just path-offset. A rotation-only element with a GSAP-baked translate would have been positionally clobbered in the original PR; now caught by the dedicated test at line 555. Good catch in the response cycle.
nit — Dead exports (orphan symbols): ADDRESSED. studioPreviewHelpers.ts drops getPreviewLocalPointer, buildRasterClickSelectionContext, getPreviewPlayer, seekStudioPreview, PreviewPlayerCompat, and PreviewLocalPointer; resolvePreviewLocalPointer is now unexported. isLargeRasterDomEditSelection remains exported in domEditing.ts:22 with only test consumers — small follow-up if Knip flags it, not blocking.
important — Behavior-change note in PR body: STILL OPEN. The PR body still doesn't call out that the selection algorithm now strictly prefers topmost-and-descendants-of-topmost; the new pinning test documents the win but a body note would help future readers. Worth one sentence; not a blocker.
nit — Fallow stripGsapTranslateFromTransform CRAP (49.5). Still flagged because the new tests don't drive every branch (e.g., DOMMatrix cross-realm early-return, the is2D false branch). Non-required check; the function is well-tested for behavior, and CRAP-vs-branches is a fair trade. Acceptable as-is.
CI
Required: Build, Typecheck, Test, Test: runtime contract, Lint, CLI smoke (required), Preflight (lint + format), Format, Semantic PR title, File size check, Smoke: global install — all SUCCESS on f7d63fbe. regression-shards (8) + Render/Tests on windows-latest IN_PROGRESS (same as previous commit; not blockers historically). Non-required Fallow audit still FAIL (87 findings, mostly pre-existing; the only new entry attributable to this PR is stripGsapTranslateFromTransform at CRAP 49.5).
Verdict: APPROVE
Reasoning: Both important findings from the prior review are fixed with targeted tests that pin the actual reported bugs (PiP sibling-stack, GSAP-baked translate clobbering). Bonus correctness fix for rotation elements added in response. Dead-export cleanup done. Only the body-note nit and the Fallow CRAP nit remain, neither blocking.
Review by Vai (re-review)
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Re-review at HEAD f7d63fbe. Verdict: APPROVE — all important findings addressed.
Status of prior findings
- important — selection bug pinning test → ADDRESSED.
domEditing.test.ts:325adds the sibling-stack[pipStudio, subComp, sfChrome]test that assertspipStudiowins. Exactly the discriminating case the prior tests lacked. - important —
createStudioPositionSeekReapplyScripttests → ADDRESSED. 6 new tests atmanualEditsRenderScript.test.ts:386+covering box-size reapply, GSAP strip, identity removal, non-translate preservation,transform:noneno-op, rotation case. - bonus correctness fix:
stripGsapTranslateFromTransformnow applied to rotation-only elements (manualEditsRenderScript.ts:254). Caught a case the prior diff missed. - nit — dead exports → ADDRESSED.
getPreviewLocalPointer,buildRasterClickSelectionContext,PreviewPlayerCompatremoved.isLargeRasterDomEditSelectionremains — small follow-up.
Still-open (non-blocking):
- body behavior-change note about smaller-leaf-behind no longer winning — cosmetic.
- Fallow CRAP 49.5 on
stripGsapTranslateFromTransformunchanged; non-required, behavior now pinned by the new tests so the function can be split later without regression risk.
CI status (verbatim)
Required all SUCCESS — Build, Typecheck, Test, Test: runtime contract, Lint, CLI smoke (required), Preflight, Format, Semantic PR title, File size, Smoke. regression-shards (1-8) + Windows render + Tests on windows-latest PENDING. Non-required Fallow audit FAILURE (pre-existing CRAP, non-gating).
Verdict: APPROVE.
Re-review by Vai
Merge activity
|
3 participants
Summary
Three interrelated studio UX and rendering fixes, plus a critical fix for manual edits not surviving video export.
1. Remove "Ask agent" popup on preview click
The "Ask agent" modal auto-triggered whenever clicking on a large raster element (images, backgrounds covering >40% of the viewport). This intercepted clicks meant for editable elements underneath, making it very annoying to select elements in compositions with large backgrounds or sub-composition screenshots.
Removed: the
isLargeRasterDomEditSelectioncheck +setAgentModalOpen(true)trigger inusePreviewInteraction.ts. The manual "Ask agent" button via the context menu still works — only the auto-popup is gone.Files:
usePreviewInteraction.ts,useDomEditSession.ts2. Fix preview click selecting wrong element across visual layers
Clicking the PiP video overlay would select the Sf Chrome image behind it, because the scoring algorithm weighted DOM depth at 10,000× per level. Elements inside sub-compositions (deeper in the DOM tree) always won over visually-on-top elements at the root level, regardless of z-order.
Fix: replaced the weighted scoring with a visual-stacking-order-first algorithm.
resolveVisualDomEditSelectionTargetnow trustselementsFromPointorder (topmost first) and only prefers a deeper candidate when it's a direct descendant of the current pick — never jumping to an unrelated element painted behind it.Files:
domEditingElement.ts3. Fix manual edits not surviving video export
Two gaps in the producer's seek-reapply script (
studioPositionSeekReapplyRuntime):a) Missing box-size reapplication. The script handled translate and rotation but not width/height. Also,
data-hf-studio-box-sizewas absent from the detection list inhtmlCompiler.ts, so the script wasn't even injected for compositions with resize-only edits.b) GSAP transform matrix clobbering translate. When GSAP animates an element (e.g.
scale,opacity), it captures the element's translate into its internal transform matrix (m41/m42). The render script was setting the CSStranslateproperty but leaving GSAP's translate baked intotransform, causing the manual edit offset to be ignored. Ported the samestripGsapTranslateFromTransformlogic the studio uses: parse the DOMMatrix, zero out m41/m42, and remove or rewrite thetransformproperty so the CSStranslatetakes effect cleanly.Files:
manualEditsRenderScript.ts,htmlCompiler.tsTest plan
bun test— 50 tests pass acrossdomEditing.test.tsandmanualEditsRenderScript.test.ts