feat(core): route motion-path mutations through studio-api + fix clip stamping#1555
feat(core): route motion-path mutations through studio-api + fix clip stamping#1555miguel-heygen wants to merge 3 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. |
085862a to
c8aee05
Compare
775d644 to
070799e
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
Holding per two independent verifications (Rames D Jusso + Via) of the same blocker. Calling out one specifically because it's a regression of work that landed earlier today.
🛑 Silent removal of the 5 readiness adapters from runtime/init.ts
packages/core/src/runtime/init.ts:9-13 + :1918-1924 — the five adapter registrations from https://github.com/heygen-com/hyperframes/pull/1548|#1548 (createMapboxAdapter / createLeafletAdapter / createGoogleMapsAdapter / createMaplibreAdapter / createD3Adapter) are removed in this diff. The factories themselves (and their .test.ts siblings) survive in runtime/adapters/, but the wiring at the registry — the thing that actually makes them participate in getReadyPromise() polling — is gone.
#1548 merged at 2026-06-18T05:21:28Z (~10 hours ago) specifically to gate __renderReady on map/viz async asset loading. If #1555 lands as-is, any composition that uses mapbox / leaflet / google-maps / maplibre / d3 will silently never reach render-readiness — the exact bug class #1548 was built to prevent. This is a real production regression, not just a code-style concern.
Most likely cause is a Graphite restack mishap when #1548 merged mid-stack and the conflict resolution dropped the wiring rather than preserved it. The benign path is one rebase + re-add the import + the 5 factory calls; the malignant path would require intentional rationale I can't find anywhere in the PR description.
Ask: confirm intentional (with rationale + regression-window acknowledgement) OR re-restack to preserve #1548's wiring.
🛑 HOLD_SYNC_MUTATION_TYPES set is incomplete
Echoing Rames D Jusso (packages/core/src/studio-api/routes/files.ts:534-548) — the set covers keyframe / motion-path-point mutations + delete but misses 5 mutation types that also change tween start or create a tween at position > 0:
add-motion-path—addMotionPathToScriptauthors a 2-anchor motionPath at caller'sposition; ifposition > 0, fresh tween snaps from CSS-base →(0,0)on start.add(generic) — can create position tweens viaproperties: {x, y}.update-meta—body.updates.positionchanges start; existing hold becomes stale.shift-positions/scale-positions— bulk-shift; pre-existing holds end up at wrong t.split-animations— produces new tweens at new positions.
syncPositionHoldsBeforeKeyframes is idempotent + script-scoped, so an over-broad set just costs one extra parse. Cheap expansion.
Non-blocking notes
Echoing Rames D Jusso:
init.ts:1440-1448—timedClipInFlowWeakMap cachesgetComputedStyle(el).positionlazily, never invalidates. Studio-edit-then-replay round-trip → stale cache picks wrong display branch. Either invalidate on style-position mutation or document the invariant.init.ts:1555-1559— visible branch uses rawtimedClipInFlow.get(rawNode), hidden branch usesisTimedClipInFlow(rawNode)(force-populates). Works by happy accident; align both or comment why.
Plus: PR body is the unfilled template across this whole stack. Worth filling in even a one-paragraph description before merge.
Once the adapter regression is addressed (re-add the wiring OR explicit rationale + downstream owner ack), happy to re-review.
070799e to
227aebe
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
Re-reviewed at 227aebe4. Top blocker closed — adapter wiring restored at init.ts:9-13, :1939-1943, so the 5 readiness-adapter registrations from #1548 are back. Thanks for the fast restack.
Reduced-scope REQUEST_CHANGES on the remaining items, per Via's R2 verification:
1. HOLD_SYNC_MUTATION_TYPES set still incomplete at routes/files.ts:534-548.
7 keyframe-adjacent types added in the restack (good) — remove-all-keyframes, add-with-keyframes, replace-with-keyframes, convert-to-keyframes, materialize-keyframes, delete, delete-all-for-selector. The 5 position-changing-non-keyframe types still need to be added:
add-motion-path—addMotionPathToScriptauthors a 2-anchor motionPath at caller'sposition; ifposition > 0, fresh tween snaps from CSS-base →(0,0)on start.add(generic) — can create position tweens viaproperties: {x, y}.update-meta—body.updates.positionchanges start; existing hold becomes stale.shift-positions/scale-positions— bulk-shift; pre-existing holds end up at wrong t.split-animations— produces new tweens at new positions.
Cheap: syncPositionHoldsBeforeKeyframes is idempotent + script-scoped, so an over-broad set just costs one parse.
2. timedClipInFlow WeakMap stale-cache at init.ts:1445-1452.
Still no invalidation hook. Studio-edit-then-replay round-trip → stale cache picks wrong display branch. Either timedClipInFlow.delete(el) on style-position mutation, OR explicit invariant comment naming the round-trip case as out-of-scope.
3. init.ts:1560-1563 — visible/hidden branch asymmetry.
Visible branch uses raw timedClipInFlow.get(rawNode) (returns undefined for new nodes); hidden branch uses isTimedClipInFlow(rawNode) (force-populates). Works by happy accident — align both or comment why.
Once these three land, happy to re-approve. Also: PR body is still the unfilled template; please fill in before merge.
c8aee05 to
80d84c7
Compare
a01743d to
1dbedb6
Compare
80d84c7 to
ba09f10
Compare
1dbedb6 to
6aafc8a
Compare
ba09f10 to
4ee9b9f
Compare
d133735 to
b79f553
Compare
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at b79f5537 (R3 — verifying R2 follow-through on the three open items + new commit b79f5537 "strip legacy path-offset/rotation + drop obsolete studio lint rule"). Stack-aware.
R2 items, verified at HEAD
1. ✅ Adapter wiring — confirmed restored at init.ts:9-13 (imports) + :1954-1958 (registrations). The 5 readiness adapters from #1548 are back. Top R2 blocker resolved.
2. 🛑 HOLD_SYNC_MUTATION_TYPES still incomplete — routes/files.ts:594-611 at HEAD has the 13-entry set, but add-motion-path, add (generic), update-meta, shift-positions, scale-positions, split-animations are STILL missing. The mechanism is identical to what R2 described — any mutation that creates or shifts a position tween at position > 0 leaves the pre-keyframe hold stale, producing the one-frame snap-to-CSS-base symptom the hold-sync was built to prevent. Particularly load-bearing for add-motion-path because #1554's addMotionPathToScript is the entry point for the new motion-path UX. Cheap fix: syncPositionHoldsBeforeKeyframes is idempotent + script-scoped, so an over-broad set just costs one extra parse.
3. 🛑 timedClipInFlow + timedClipIsLeaf WeakMap stale-cache (init.ts:1442-1471) — both caches still lack an invalidation hook. Studio-edit-then-replay round-trip (e.g. user toggles position:relative→absolute, or wraps a leaf in a new timed container) → stale cached value picks wrong display branch. The leaf check (el.querySelector('[data-start]') === null) is especially fragile — adding a timed child to a previously-leaf element is the canonical hot-edit operation. Either invalidate on style-position / DOM-mutation, OR document the round-trip case as out-of-scope with an explicit invariant comment.
4. 🛑 Visible/hidden branch asymmetry (init.ts:1575-1579) — still:
if (isVisibleNow) {
if (timedClipInFlow.get(rawNode)) rawNode.style.removeProperty("display");
} else if (isTimedClipInFlow(rawNode) && isTimedClipLeaf(rawNode)) {
rawNode.style.display = "none";
}
Visible branch uses raw .get() (undefined for nodes that never went through the hidden branch); hidden branch uses isTimedClipInFlow() (force-populates). The hidden→visible transition works because the hidden branch always runs first for an in-flow leaf; but the first-ever-visible call for an in-flow leaf returns undefined → style.removeProperty("display") is a no-op, which is benign IF nothing else set display:none. Today nothing else does. But this works by happy accident; align both branches via isTimedClipInFlow(rawNode) OR add a comment naming the invariant.
New findings on commit b79f5537
5. packages/core/src/lint/rules/gsap.ts:855-890). The gsap_studio_edit_blocked rule is REMOVED with rationale "obsolete studio lint rule." The rule warned when a user-authored timeline targets #id / .class selectors that Studio's drag/resize then can't write back. The PR's premise is that #1554's mutation surface now CAN write back to those targets, so the warning is no longer informative. Verify: does #1554's writer actually round-trip tl.to("#id", { x, y }, 0) style author code, or only the studio-generated tl.to("[data-hf-id=\"…\"]", …) form? If the writer only handles data-hf-id selectors, deleting the lint rule restores a silent-skip footgun that the rule was specifically calling out. (Looked at gsapParser.ts write paths quickly — they look selector-agnostic via findTargetElement, but a regression test pinning round-trip on a #id-selector tween would convert this from "concern" to "verified.")
6. TIMELINE_REGISTRY_ASSIGN_PATTERN (packages/core/src/lint/utils.ts:24). The deleted rule was the only consumer (verified via repo-wide code search). Exported regex with no consumer is dead code; safe to remove in this PR or follow-up.
7. stripStudioEditsFromTarget rotation branch — sibling-asymmetry feedback (routes/files.ts:316-372). The expanded path-offset + rotation strip is well-shaped (touched flag per-attribute, both branches mirror each other on attribute lifecycle). The new branch correctly handles data-hf-studio-rotation / data-hf-studio-rotation-draft / data-hf-studio-original-rotate / data-hf-studio-original-inline-rotate / data-hf-studio-original-rotation-transform-origin. Two questions:
- The
keyframesWritePositionhelper (routes/files.ts:370-381) covers position but there's nokeyframesWriteRotationequivalent.add-with-keyframes/replace-with-keyframescallstripStudioEditsFromTargetunconditionally when keyframes write position — so a keyframe set that writes ROTATION but not position won't strip the rotation channel, leaving the legacy CSS var to double with the new tween. Suggest akeyframesWritesGroup(kfs, 'position' | 'rotation')helper. - The generic
addcase at:909-918correctly checks bothpositionandrotationviaclassifyPropertyGroup. Good consistency on that path.
8. splitElementInHtml clone descendant data-hf-id strip (sourceMutation.ts:330-333). Correct fix — descendants would have duplicated data-hf-ids otherwise. But the test added at :511-526 only covers a single-element split with no descendants; the asymmetry was discovered by inspection. Suggest a fixture with <div id="title"><span data-hf-id="…">Hi</span></div> to pin the descendant-strip behavior.
Concerns (carryover from R1, non-blocking)
- PR body has been updated nicely with the single-source manual offset + rotation rationale. Thank you.
Questions
- The
hdr-hlg-regressionknown-issue flag in the PR body (final ~4 frames, PSNR ~9.7 dB) — is this still open? If yes, what's the rollback story if it doesn't reproduce post-merge? Worth either landing a fix or filing a tracking issue with clear ownership before merge.
Stamp routability
— Rames D Jusso
vanceingalls
left a comment
There was a problem hiding this comment.
Reviewed at b79f5537. Concur with @james-russo-rames-d-jusso's R3 — two of three R2 carryover items remain open, with the third structurally defensible but documentation-light. Plus one net-new minor blocker and the lint-rule deletion warrants verification.
R2 carryover — verified at HEAD:
🛑 HOLD_SYNC_MUTATION_TYPES (R3 item 2): STILL INCOMPLETE. routes/files.ts:594-611 at b79f5537 carries 13 entries; the 5 jrusso/Rames-named position-changing-non-keyframe types remain absent: add-motion-path (recast handler at L1097), generic add (L676/L957), update-meta (L673/L954), shift-positions / scale-positions (L853/L858, L1177/L1183), split-animations (L816/L1140). Each can create or shift a position tween's first keyframe value — exactly the condition the set is supposed to gate.
This is structurally the decorative-gate pattern: the read site fires (HOLD_SYNC_MUTATION_TYPES.has(body.type) at L1677) but the write site (syncPositionHoldsBeforeKeyframes) never runs for those mutation types because they're never set members. Read path real, gating value 0% for those types. Same shape as decorative billing gates, just in a non-billing domain — the user-visible failure mode is a one-frame snap-to-CSS-base instead of a security/billing exposure, but the structural lie is identical. Cheap fix: idempotent + script-scoped, over-broad set just costs one extra parse. Particularly load-bearing for add-motion-path because #1554's addMotionPathToScript 2-anchor default authors (0,0) → (x,y) and is the canonical entry point for the new motion-path UX (Rames flagged this from the #1554 side).
🛑 timedClipInFlow + timedClipIsLeaf WeakMap stale-cache (R3 item 3): STILL OPEN. init.ts:1442-1471 at HEAD has both caches with no .delete(el) invalidation hook anywhere in the file (grep confirmed). Studio-edit-then-replay round-trip leaves stale cache; the leaf check (el.querySelector('[data-start]') === null) is especially fragile under hot-edit. Either invalidate on style-position / DOM-mutation OR document the round-trip case as out-of-scope with an explicit invariant comment.
🟡 Visible/hidden branch asymmetry (R3 item 4): STRUCTURALLY DEFENSIBLE, DOCUMENTATION-LIGHT. init.ts:1576-1578 at HEAD: visible branch uses raw .get(), hidden branch uses isTimedClipInFlow() wrapper. The asymmetry works because the hidden branch always runs first for an in-flow leaf, so the cache is populated by the time visible-branch reads it; first-ever-visible call returning undefined is a no-op no-op (nothing else sets display:none). I soft-pedal Rames's R2/R3 from blocker to NIT — the invariant holds today, but a one-line comment naming the assumption ("visible-branch raw .get() relies on hidden-branch having populated the cache first; if a future caller sets display:none outside this path, this needs isTimedClipInFlow() here too") would prevent the next reader re-deriving the invariant from scratch.
Net-new (minor blocker on cutover dark-launch surface):
executeGsapMutationAcorn at routes/files.ts:628-884 is MISSING cases for update-motion-path-point, add-motion-path-point, remove-motion-path-point, add-motion-path. The recast path (executeGsapMutationRecast at L885+) handles all four. The acorn default: branch at L883 returns { error: "unknown mutation type: ..." } with HTTP 400, so flipping STUDIO_SDK_CUTOVER_ENABLED=true hard-fails every motion-path mutation in Studio. Dark-launch breakage (flag off in prod), not active regression — but PR body says "Route the new keyframe / motion-path mutations through the studio-api source-mutation layer and file route" without qualifying that one side is unrouted. Either: (a) add the cases to the acorn switch (even as 501 Not Implemented), (b) explicitly fall through to recast for these types, or (c) disclose in the PR body. Maps to band-aid pattern #3 (silent scope gap on stated coverage). [verify-full-dispatch-chain] applies: producer side in #1554 ships 4 new motion-path mutation types; consumer side in this PR routes them in only ONE of TWO dispatch branches.
Concur with @james-russo-rames-d-jusso on R3 items 5-8:
- Item 5 (deleted
gsap_studio_edit_blockedlint rule): verifiedisElementGsapTargetedsurvives atpackages/studio/src/hooks/gsapTargetCache.ts:49with 4 active consumers inuseDomGeometryCommits.tsthat now show an explicit toast (GSAP_CSS_FALLBACK_BLOCKED_MESSAGE) instead of silently dropping the save. The rule's premise ("silently skips saving") is obsolete. BUT Rames's deeper question — "does the writer round-trip#id-selector tweens, or onlydata-hf-id?" — is the real verification ask. If the writer only handlesdata-hf-idselectors, deleting the lint rule restores a silent-skip footgun. Worth a regression test pinning round-trip on a#id-selector tween. NIT-level until that's verified. - Item 6 (orphan
TIMELINE_REGISTRY_ASSIGN_PATTERN): concur, safe to remove. - Item 7 (
keyframesWritePositionno rotation analog): concur,add-with-keyframes/replace-with-keyframeswriting rotation-only keyframes won't strip the legacy CSS var. - Item 8 (
splitElementInHtmldescendant-strip fixture missing): concur, the test at:511-526covers single-element only; descendant case discovered by inspection deserves a regression fixture.
Verdict: BLOCK. Items 2-4 are R2 carryover, item 5 needs round-trip verification, item 7 is a sibling-symmetry gap, plus the acorn dispatch dark-launch breakage. Once items 2 + 3 land (item 4 acceptable with a comment) + acorn dispatch covers motion-path types (or disclosure), happy to re-evaluate.
Review by Via
… stamping Wire the new mutations into the file save route. Only authored clips suppress descendant stamping, so auto-stamped animated scenes can inline-expand. Hide in-flow timed clips with `display:none` only when they are LEAF clips (no nested timed clips). `display:none` on a container removes its whole subtree, hiding descendants that are still inside their own visibility window — e.g. an in-flow composition root whose effective window clamps to the timeline end would black out a child video that should still show (the hdr-hlg regression). Containers keep `visibility:hidden`, which a visible descendant can override; only leaves leave the flow, which is all the split-overlap case needs.
…lint rule A position or rotation add/set mutation makes the GSAP timeline the single source of truth for that channel, so any lingering --hf-studio-offset / --hf-studio-rotation CSS var must be cleared to avoid double-applying. stripStudioEditsFromTarget now clears both channels, and the add-strip fires for the position AND rotation property groups. Also removes the obsolete `gsap_studio_edit_blocked` lint rule: it warned that Studio cannot save drag/resize edits to elements in a registered timeline — the exact premise the single-source work inverts (the timeline is now the edit target). Removed the rule, its now-unused TIMELINE_REGISTRY_ASSIGN_PATTERN import, and its 5 tests.
… cache, strip rotation channel - HOLD_SYNC_MUTATION_TYPES: add add-motion-path (load-bearing — addMotionPathToScript authors past t=0 → first-frame snap-to-(0,0) without the hold), update-meta, shift-positions, scale-positions, split-animations. (add stays out: flat tweens only, syncPositionHoldsBeforeKeyframes is a no-op for non-keyframed tweens.) - init.ts: timedClip in-flow/leaf WeakMaps now invalidate on clipTreeSignature change; visible/hidden branches both go through isTimedClipInFlow (was .get() by accident). - keyframesWriteRotation mirrors keyframesWritePosition so a rotation-only keyframe set strips the stale --hf-studio-rotation channel.
4ee9b9f to
fd0a505
Compare
b79f553 to
1612463
Compare

Stack: GSAP keyframe + motion-path editing — studio-api wiring + runtime clip fixes (#1553 → #1561).
What
Two things:
runtime/init.ts.Why
visibility:hiddenstill reserve their layout box, so a split clip's two halves stacked instead of overlapping.How
sourceMutation.ts+routes/files.ts: dispatch motion-path/keyframe ops through studio-api.init.ts: ancestor-suppression now counts authoreddata-startonly (hasAuthoredTimedAncestor); in-flow timed clips hide withdisplay:nonewhile positioned clips keepvisibility:hidden.Test plan
bun run test(core) greenhdr-hlg-regressionshard fails on the final ~4 frames (PSNR ~9.7 dB). Tracking whether the in-flow-clip hiding change interacts with HDR layered capture.Single-source manual offset + rotation (added)
add/setmutations that write a position or rotation now strip the matching legacy CSS var (--hf-studio-offset/--hf-studio-rotation) from the target — matchingadd-with-keyframes/replace-with-keyframes. Once the GSAP timeline owns that channel (single source of truth), the legacy var must be cleared so it can't double-apply.