fix(studio): overlay jump, delete-all, split polish [9/10]#1191
fix(studio): overlay jump, delete-all, split polish [9/10]#1191miguel-heygen wants to merge 5 commits into
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
6e391b2 to
eca150f
Compare
82a5989 to
6c8acf6
Compare
eca150f to
dcbe8da
Compare
790ef3f to
99fc9ee
Compare
3a8e22f to
0eb2b93
Compare
1937dd3 to
c048e70
Compare
0eb2b93 to
626983d
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
Polish + bugfix PR. +175/-40. Five concerns named in the body — let me verify each is correctly scoped:
1. gsapAnimatesProperty guard preventing reapplyBoxSizes overwrite
The mechanism is sound: when a GSAP tween interpolates width/height, the bounding-box reapply path must skip those properties. Two checks:
- Guard granularity: per-property, not per-element. If GSAP animates
widthbut notheight,reapplyBoxSizesshould still setheight. Verify the guard doesn't blanket-skip. - Guard timing: must fire before the reapply, not after. A late guard reverts the GSAP-interpolated value.
2. Delete All Keyframes → handleGsapDeleteAnimation fallback
When "delete all keyframes" runs removeAllKeyframesFromScript from #1167, the function collapses to flat tween with the last keyframe's properties. So "delete all" actually means "convert to flat tween" — not "remove the entire animation".
The fallback to handleGsapDeleteAnimation extends this: if the user truly wants to remove the whole tween, the fallback path catches that intent. Verify:
- Trigger condition: when does
handleGsapDeleteAnimationfire vs the keyframe-collapse path? Worth a comment in the code explaining the discriminator. - Confirmation prompt: "delete all" is destructive at two different scales (drop keyframes vs drop the whole tween). User should know which one they're getting.
3. Wire split through component chain
App → StudioPreviewArea → NLELayout → Timeline. Verify prop-drilling didn't break a single intermediate (a missing prop at any layer is silent — TypeScript catches Required props, but optional ones can no-op).
4. Split restricted to media (video/audio/img) — hidden for divs/compositions
This is the client-side gate from my #1189 comment. Both PRs should be hardening together — confirm splitElementInHtml server-side also gates on the same element-type check.
5. Clip context menu onContextMenu prop
Verify it doesn't double-fire if a parent (Timeline / NLELayout) also has an onContextMenu handler. React events bubble; if the parent has a non-e.stopPropagation() handler, both menus could render.
Architecture is fine — five independent fixes, each bounded. None of them should affect the main keyframe flow. Test coverage would benefit from at least one "split on a tween-targeted div" test (should be hidden, but the negative-case test is good defense).
Review by Jerrai (hyperframes specialist)
…yBoxSizes guard - Fix overlay bounding box jump: reapplyBoxSizes now skips elements whose width/height are animated by GSAP (gsapAnimatesProperty check prevents studio CSS from overwriting GSAP interpolated values) - Delete All Keyframes removes entire animation (handleGsapDeleteAnimation) with fallback to first animation when no keyframed anim exists - Wire split clip through App → StudioPreviewArea → NLELayout → Timeline (onSplitElement prop, toolbar button, S hotkey, clip context menu) - Add onContextMenu to TimelineClip for right-click clip context menu
Sub-compositions (data-composition-src) cannot be meaningfully split — the clone would load the same source and fight for the same timeline. Block with a specific toast message and disable in the context menu.
c048e70 to
aae066c
Compare
626983d to
7149806
Compare

Summary
gsapAnimatesPropertyguard preventsreapplyBoxSizesfrom overwriting GSAP-interpolated width/heighthandleGsapDeleteAnimationwith fallback)+175 LOC