feat(studio): scale GSAP positions on resize + shift on drag + diamond edge fixes#1448
Conversation
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Verdict: Request-changes (scoped) — round-trip mismatch + zero-coverage runtime mutation
I've focused this pass on the canonical-rubric + Claude-generated-code lens; deferring HF-runtime semantics (Studio drag flow, scrub clock, render-time tween targeting) to Via.
What ships
A storage-format refactor for GSAP animation positions: clip-relative offsets instead of absolute timeline seconds. Studio hooks emit position = absoluteTime - elStart on commit; globalTimeCompiler rehydrates absolute via a new elementStart parameter (default 0); and a new applyClipRelativeOffsets() walks the captured timeline post-bind, shifting each tween's startTime by resolveStartForElement(target). 14 files, +105/-67.
Migration / forward-compat verdict — blocker
There is no read-side migration. Existing GSAP scripts on disk store absolute positions (that's what main writes today). Once this PR lands, runtime sees those absolute positions and applyClipRelativeOffsets shifts them by elStart again. A clip at data-start=5s with tl.set("#el", {...}, 5) becomes effective t=10. No version discriminator, no script-fingerprint check, no opt-out attribute.
Worse — round-trip is broken even for new output:
• keyframesToGsapAnimations (gsapSerialize.ts:255) still computes position = elementStartTime + prevKf.time (absolute).
• generators/hyperframes.ts:192 calls it with element.startTime, so the canonical HF generator produces a script with absolute positions.
• That script is then serialized and runtime-shifted again → double-shift, every newly-generated HF file.
• The inverse, gsapAnimationsToKeyframes, was updated to drop the subtraction but still takes elementStartTime as a required parameter and silently ignores it (gsapSerialize.ts:280-306). External SDK consumers will pass it and get wrong-shape data.
Two of these are public-API exports from @hyperframes/core (src/index.ts:79-80). Either:
- Update
keyframesToGsapAnimationsto emitposition = kf.time(matching the new contract), and drop theelementStartTimeparameter from both functions (breaking minor-version bump), OR - Make
applyClipRelativeOffsetsconditional on a script-level opt-in marker (e.g.// @hf:clip-relativecomment or an attribute on the timeline element), so old scripts keep absolute semantics and new ones get the shift.
Without one of these, every existing project + every generator-emitted script breaks on first reload after merge.
Determinism contract
No new Date.now() / performance.now() / Math.random() / fetch() introduced in render-time paths — verified by grep over the touched runtime + studio files. The two pre-existing Date.now() / performance.now() refs in init.ts are unrelated to this PR.
Claude-generated-code lens findings
• _updateAnimationSelector (gsapParser.ts:1328) is renamed from updateAnimationSelector with a leading underscore and has zero callers. The sole call site was rewritten to use updateAnimationInScript({position, targetSelector}). Dead code — should be deleted, not stub-renamed. (Fallow already flags 2 dead exports in gsapShared.ts for the same reason — getIframeDocument, toAbsoluteTime.)
• gsapKeyframeCommit.ts:27 declares const elStart = ... then gsapKeyframeCommit.ts:64 redeclares the identical expression inside the else branch. Shadowing; harmless but sloppy — drop the inner one.
• applyClipRelativeOffsets (init.ts:989-1015) silently skips non-HTMLElement targets via target instanceof HTMLElement. SVG tweens (<svg>, <g>, animated paths/text — common in HF compositions) will not be offset-shifted, while sibling HTMLElement tweens are. Use Element + duck-type getAttribute("data-start"), or explicitly cast to Element and call resolveStartForElement which already handles non-HTML.
• applyClipRelativeOffsets only checks targets[0] — gsap.to([".a", ".b"], ...) with mixed clip ancestors applies element-0's offset to all. Flag for Via — may not be a real-world pattern.
• _hfOffsetApplied flag is not set when the target is rejected (non-HTMLElement / no targets fn), so every re-bind re-walks those tweens. Minor perf only.
Test coverage gaps
• Zero tests for applyClipRelativeOffsets. init.test.ts (1098 lines, 30+ cases) is untouched. The single load-bearing runtime mutation in this PR has no unit test pinning: idempotency via _hfOffsetApplied, the SVG-target skip, the no-data-start fallback, the rebind-doesn't-double-shift contract. This is the highest-risk omission in the diff.
• No round-trip test. No test loads an absolute-position script, runs it through the runtime, and asserts identical render output to a clip-relative script. This is the canonical regression for storage-format changes and is exactly the test that would have caught the keyframesToGsapAnimations mismatch above.
• No "moving a clip = zero GSAP mutations" pin. The PR's headline claim ("zero GSAP mutations required") has no test — there's no drag clip; assert mutation count === 0 assertion anywhere. If a regression reintroduces a GSAP rewrite on drag, nothing fails.
• Test changes are limited to gsapParser.test.ts — the parser/serializer corpus. The studio hooks (drag, gesture, keyframe, enable-keyframes) and the runtime path are unverified.
Cross-PR coherence with recent HF stack
Checked open PRs — #1439 (Vance, drag-keyframes-with-snapping) touches Timeline.tsx / TimelineCanvas.tsx / TimelineClipDiamonds.tsx, no overlap with this PR's hook surfaces. No other in-flight conflict spotted.
Concerns
• gsapAnimationsToKeyframes no longer uses elementStartTime but keeps it as a required positional parameter. Either drop it (breaking minor bump, clean) or mark deprecated with a runtime warn. Silent-ignore is the worst option for a public export.
• _hfOffsetApplied is a string-keyed underscore-prefix marker on GSAP tween objects. If GSAP ever uses the same prefix internally, collision. Switch to a Symbol or a WeakSet<Tween> outside the function scope — both prevent leakage + collision.
• The mutation runs in bindRootTimelineIfAvailable and rebindTimelineFromResolution, both of which can fire during user edits. If a user changes data-start on a clip and Studio re-binds with the same timeline instance (early return at init.ts:1036 / 1322), the offsets from the old data-start stay applied. Via to confirm rebind-on-edit semantics.
Nits
• applyClipRelativeOffsets error handler logs to swallow("runtime.init.clipRelativeOffsets", err) — fine, but consider whether silently swallowing a mass-mutation failure leaves the user with half-shifted tweens. Worth a console.warn in dev.
• roundTo3(newStart - elStart) in gsapDragCommit.ts:122 — if elStart floats by 0.0001 from the on-disk value, the new position drifts. Minor; the previous absolute-position formula had the same risk.
Via-lane callouts
• Confirm Studio rebind-on-data-start-edit semantics — does same-instance early return leave stale offsets?
• Confirm SVG-target tweens (paths, masks, text) aren't load-bearing in current HF templates. If they are, the HTMLElement filter is a render bug not a nit.
• Confirm the headline claim ("moving a clip on the timeline now keeps animations in sync automatically with zero GSAP mutations") — i.e., that the prior version did emit per-drag GSAP mutations and this version genuinely emits zero. The PR body asserts this; no test pins it.
CI
Mostly green; Fallow audit failed with 78 findings (2 dead exports, 39 duplications, 37 health/CRAP). The dead exports + _updateAnimationSelector are introduced or aggravated by this PR. Regression shards + Windows render + CodeQL still in progress at review time.
Review by Rames D Jusso
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Verdict: Request-changes (scoped) — R1 blocker fix is conceptually right but the marker is never emitted, breaking newly-generated projects
R2 fix-up (f1cf2498) targets the right places — generator path, marker gate, resolvedStart correctness, dead-code removal, Element widening. Five of six R1 items resolve cleanly. The sixth, the round-trip / migration fix, doesn't actually close because the gating marker has no emit site outside the studio file-edit path, and the regex that's supposed to inject it doesn't match the generator's <script> tag.
R1 status
• ✅ BLOCKER 1 — Round-trip generator/runtime mismatch (partial / still open). Generator now writes clip-relative (gsapSerialize.ts:254 — position = prevKf ? prevKf.time : kf.time, no more elementStartTime + …). Runtime now gates on a marker (init.ts:991 — if (!document.querySelector("script[data-position-mode='relative']")) return). Studio file-edit route auto-injects on first save (files.ts:1180-1185). Conceptually this closes the double-shift: old absolute scripts (no marker) → runtime skips offset → render unchanged; new clip-relative scripts (marker present) → runtime adds elStart. But see new finding below — the marker injection is dead-code-path for the canonical generator output, so the "blocker resolved" claim doesn't hold in practice.
• ❌ CONCERN 1 — Zero test coverage on applyClipRelativeOffsets. init.test.ts still 1098 lines, untouched between R1 HEAD (70aae418) and R2 HEAD (f1cf2498). No tests for: marker-gate (!document.querySelector(...) early-return), _hfOffsetApplied idempotency, Element-widened SVG path, no-data-start fallback, rebind double-shift. The single load-bearing runtime mutation in this PR remains unverified.
• ❌ CONCERN 2 — "Zero GSAP mutations" headline claim has no test pin. Still no drag clip → assert mutation count === 0 test. PR's central thesis remains unverified.
• ✅ CONCERN 3 — gsapAnimationsToKeyframes ignores elementStartTime. Parameter renamed to _elementStartTime on both keyframesToGsapAnimations (gsapSerialize.ts:241) AND gsapAnimationsToKeyframes (gsapSerialize.ts:281), signaling intentional ignore via TS convention. Companion test (gsapParser.test.ts:587-617) pins clip-relative behavior — gsapAnimationsToKeyframes(animations, 2) returns keyframes[0].time === 0, keyframes[1].time === 1, ignoring the 2. Public-API signatures unchanged (no minor-version bump needed). Acceptable — though a @deprecated JSDoc would be clearer for SDK consumers than the underscore-prefix convention.
• ✅ CONCERN 4 — Dead _updateAnimationSelector. Deleted (gsapParser.ts diff shows -22 lines). The two updateAnimationSelector refs in the file are now comment-only references.
• ✅ CONCERN 5 — applyClipRelativeOffsets SVG filter. target instanceof HTMLElement widened to target instanceof Element (init.ts:1006). SVG tweens now offset-shifted alongside HTMLElement tweens. Via's lane judgment unblocked.
What changed at R2
6 files, +15/-31 across one commit (f1cf2498):
• gsapSerialize.ts — generator path emits clip-relative, params renamed _elementStartTime.
• gsapParser.ts — _updateAnimationSelector deleted.
• gsapParser.test.ts — assertions updated to clip-relative expectations.
• init.ts — marker gate added, HTMLElement → Element.
• files.ts — studio mutation route auto-injects data-position-mode="relative" on save.
• globalTimeCompiler.ts — resolvedStart path now adds elementStart (matches the new clip-relative storage; was double-shift risk on studio side).
New findings
• NEW BLOCKER — marker injection regex never matches the generator's <script> tag. files.ts:1182 searches for (<script\b[^>]*data-hyperframes-gsap[^>]*)> — anchoring on a data-hyperframes-gsap attribute. grep -rn "data-hyperframes-gsap" across the whole repo returns exactly one hit: this regex itself. No generator, exporter, or template emits a <script data-hyperframes-gsap> tag. generateHyperframesHtml (hyperframes.ts:354-360) emits a bare <script>${gsapScript}</script>. Consequence: every newly-generated HF project ships with clip-relative positions but NO marker; runtime gates off; animations fire at the start of the timeline instead of being shifted to elStart. The "old-format-unchanged" half of the migration works (those scripts also lack the marker, so they skip the offset and render correctly with absolute positions), but the "new-format-shifted" half is broken end-to-end. Either (a) change the regex anchor to the actual <script> boundary that generateHyperframesHtml emits — but that tag has no attributes to anchor on, so this needs a new emit site in the generator AND a tightened regex — or (b) emit data-position-mode="relative" directly in generateHyperframesHtml at hyperframes.ts:357 (<script data-position-mode="relative">), and emit it from every other gsap-script writer (CLI scaffold, capture, blank template). The single-place injection in files.ts only covers user-edited scripts going through the studio mutation API, not the canonical generation path. This is more severe than R1's double-shift because it silently breaks render correctness for net-new projects with no error signal.
• Sub-finding — same marker gap affects Studio's resolveTweenStart semantics. globalTimeCompiler.ts:30 unconditionally adds elementStart + animation.resolvedStart. There's no marker-gate on the studio side. If a legacy absolute-position script is loaded into Studio and re-rendered, the timeline view will compute wrong tween times (absolute + elStart). This is fine as long as Studio always re-saves on first edit (re-emitting clip-relative + marker), but the marker emit on save is broken per the previous finding, so this regression surface is real for any legacy project opened in Studio.
• resolvedStart change is consistent with the new contract. gsapParser.ts:1043 sets anim.resolvedStart = Math.max(0, start) where start comes from the parsed position. After this PR, generator writes clip-relative positions → resolvedStart holds clip-relative time → elementStart + resolvedStart produces absolute. Coherent for new format; matches the runtime contract. The fix is correct.
Via-lane callouts (unchanged from R1)
• data-start-edit + same-instance rebind: still no test; same concern.
• SVG-target prevalence: deferral resolved — filter widened to Element, so this is no longer a render-bug risk.
• Headline "zero GSAP mutations" claim: still unpinned by test.
CI
• Required checks green at HEAD: Test, Typecheck, Lint, Format, CLI smoke, Build, Studio: load smoke, SDK unit+contract+smoke, Render on windows-latest, Tests on windows-latest, Perf shards (drift/fps/load/parity/scrub), Test: runtime contract, Preview parity, CodeQL, File size check.
• Fallow audit: still failing (85 findings now, was 78 at R1). Net +7 findings post-R2; the _updateAnimationSelector rename was removed but other duplications/complexity findings persist. Not introduced by this PR's core change.
• regression-shards (shard-1, shard-2): pending at review time. Worth waiting on — they could surface the marker-gate regression empirically.
Recommended next steps
- Emit
data-position-mode="relative"directly ingenerateHyperframesHtml(hyperframes.ts:357) and any other script-emitting path (CLI scaffold, capture, blank template, the studio play.ts runtime tag). Verify withgrep -rn "<script>" packages/that every HF gsap-script writer carries the marker. - Tighten the
files.tsinjection regex to a generator-emitted anchor (e.g. the bare<script>block matching the gsap-script position), or drop it entirely if all emit sites now write the marker directly. - Add a runtime test pinning the marker gate: load marker'd + un-marker'd timeline, assert
applyClipRelativeOffsetsis no-op for the latter. - Add a round-trip test: render canonical generator output through the runtime, assert effective tween start times match
elStart + clip-relative-position.
Review by Rames D Jusso
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
R2 correction.
My R2 above was posted against f1cf2498 while HEAD had advanced to 953715e8 ~2 min earlier — sequencing artifact on my side, my apologies for the noise.
Re-verifying at 953715e8:
BLOCKER 1 (sentinel emit path) — ✅ resolved. packages/core/src/studio-api/routes/files.ts:1182 now uses block.document.querySelector("script:not([src])") + setAttribute("data-position-mode", "relative") via the DOM API. Plain <script> tags (no data-hyperframes-gsap anchor required) get the marker stamped. Two pinning tests landed in files.test.ts:545-595:
"stamps data-position-mode='relative' on the script tag after any mutation"— uses a plain<script>tag, exercises the gsap-mutations route, asserts the attribute is present. This is exactly the case my R2 said wouldn't be covered."does not duplicate data-position-mode on subsequent mutations"— idempotency.
Updated R1-item status:
- BLOCKER 1 ✅ at
953715e8 - CONCERN 1 (no
applyClipRelativeOffsetsruntime tests) ❌ still open - CONCERN 2 (zero-GSAP-mutations headline claim has no test pin) ❌ still open
- CONCERN 3-5 ✅ as previously noted
Revised verdict: approve-with-concerns. The 2 remaining items are non-blocking, but both are on the load-bearing runtime path — worth pinning before stamp.
Review by Rames D Jusso
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
R3 — all R1 items resolved. Verified at HEAD d36d9895.
BLOCKER 1 (generator-runtime contract) — ✅ closed at the source. packages/core/src/generators/hyperframes.ts:357 now emits <script data-position-mode="relative"> directly. Net-new HF projects ship with the sentinel from the canonical generator path; runtime gate doesn't depend on the studio-api mutation route to stamp it. No migration discriminator needed because the marker is structurally inseparable from emit.
CONCERN 1 (no applyClipRelativeOffsets runtime tests) — ✅ resolved. Function extracted to packages/core/src/runtime/clipRelativeOffsets.ts (+32) with 9 tests in clipRelativeOffsets.test.ts covering:
- Offset application (
shifts startTime by element's offset) - Zero-offset no-op
- Same-element multi-tween consistency
- Different elements with different offsets
- Idempotency on rebind (the load-bearing invariant — see CONCERN 2 below)
- Skip on tweens without targets fn
- Null/undefined timeline no-ops
- Position-greater-than-zero preserves both clip-start AND tween-relative offset
CONCERN 2 (zero-GSAP-mutations headline pin) — ✅ resolved differently. The "doesn't double-apply on rebind" idempotency test is the operationally-meaningful shape of the zero-mutation claim — it proves the runtime walker emits no redundant tween-position updates when the timeline rebinds. Crediting per resolved-differently (not the prescribed "drag clip → assert GSAP mutation count = 0" test, but the underlying invariant).
Bonus (not in R1 ask): files.ts:extractGsapScriptBlock now exposes scriptElement directly, and the mutation route stamps via block.scriptElement.setAttribute(...) instead of querySelector("script:not([src])"). Tighter — won't pick up a bystander plain <script> tag if the project has multiples.
CONCERN 3-5 — already ✅ in the prior R2 correction.
Revised verdict: approve. All R1 items closed (3 directly, 1 resolved-differently). The architectural shape (clip-relative storage matching AE/Premiere model) + the generator/runtime contract are both clean. CI status pending — worth confirming regression-shards lands green before stamping.
Review by Rames D Jusso
miguel-heygen
changed the base branch from
main
to
06-14-feat_studio_drag_keyframes_with_live_beat_snapping
vanceingalls
force-pushed
the
06-14-feat_studio_drag_keyframes_with_live_beat_snapping
branch
3 times, most recently
from
79c14d7 to
3ac1da4
Compare
miguel-heygen
force-pushed
the
refactor/clip-relative-gsap-positions
branch
from
64a8658 to
5fc90f0
Compare
miguel-heygen
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
miguel-heygen
force-pushed
the
refactor/clip-relative-gsap-positions
branch
from
5fc90f0 to
9fd67d2
Compare
| const res = await fetch( | ||
| `/api/projects/${projectId}/gsap-mutations/${encodeURIComponent(filePath)}`, | ||
| { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ | ||
| type: "shift-positions", | ||
| targetSelector: `#${elementId}`, | ||
| delta, | ||
| }), | ||
| }, | ||
| ); |
miguel-heygen
force-pushed
the
refactor/clip-relative-gsap-positions
branch
from
9fd67d2 to
6d4a4a6
Compare
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Re-verified at HEAD 6d4a4a6 (was 9fd67d2 in prior pass). Verdict: approve-with-concerns — diff between the two SHAs is +124/-25 across 6 files; the load-bearing shift-positions mutation and its three Vance-scenario edges are unchanged, so the prior findings still apply. The new commits inline three commit handlers into PropertyPanel.tsx (removing the import from propertyPanelTransformCommit.ts), add a null-guard to GestureTrailOverlay, and refactor resolveTweenStart to take an elementStart offset — none of which moves the needle on the concerns below.
Wholly different shape from the R1-R3 clip-relative approach reviewed earlier (which is good — keeping absolute storage matches NLE convention and avoids the migration mess). The prior clip-relative machinery (clipRelativeOffsets.ts/.test.ts, applyClipRelativeOffsets, data-position-mode sentinel, keyframesToGsapAnimations clip-relative emit) is fully gone from the tree. Revert is clean.
Vance's three scenarios — verification
- Move track start past gsap start — Handled as a uniform shift by
delta = updates.start - element.start(useTimelineEditing.ts:126). No pinning behavior. If you drag a clip from start=2 to start=5 with an animation at position=3, the animation moves to position=6 — it stays in the same place relative to the clip, not the timeline. This is the standard NLE semantics (matches Premiere/AE group-move) and is the sensible default; Vance's "pin gsap start to track start" idea is not implemented — confirm with him this isn't a regression on his original ask. - Move track start past several animation positions — Same uniform shift. Nothing collapses, nothing is removed. The
Math.max(0, …)atfiles.ts:732floor-clamps any animation whose new position would be negative to position=0. This is the messy case Vance flagged and the clamp behavior should be confirmed with him — multiple animations originally at e.g. positions 1, 2, 3 with delta=-5 all collapse to position=0 (lossy, irreversible). Re-dragging back +5 leaves them stacked at 5, not 6/7/8 — drift on idempotency lens #3. - Keyframes before new clip start — Not addressed. Because the mutation shifts every matching animation uniformly, no animation is "before the clip" after the shift unless clamped (see #2). Resize-from-start (
handleTimelineElementResize) does NOT shift, so keyframes that were inside the clip can become outside the new window. Per PR body this is intentional ("visibility window only") — but the PR has no story for what plays back from a tween whoseposition < data-start. Worth a quick check with Vance: is this NLE-clipping behavior (don't render before clip start) already handled elsewhere, or does the runtime happily fire tweens before the clip window?
Blockers
(none)
Concerns
• packages/core/src/studio-api/routes/files.ts:723-735 — Zero test coverage for the shift-positions mutation. This is the load-bearing server-side mutation that every clip drag goes through. No assertions on: uniform-delta correctness, the Math.max(0, …) clamp (Vance scenario #2), string-position skip behavior ("+=1", "<", ">" GSAP idioms — typeof !== "number" skip at line 731), delta === 0 early-return, targetSelector mismatch behavior, the 3-decimal rounding. The PR body's checklist claims "162 tests pass (parser + files API)" — but grep -rn "shift-positions" --include="*.test.ts" returns zero hits. Same omission shape as my R1 concern on applyClipRelativeOffsets on the prior implementation. Add at minimum: (a) shift by +delta moves all matching numeric positions, (b) shift by -delta with overflow clamps to 0 (and document the lossy semantics), (c) string-positioned tweens are untouched, (d) other-selector tweens are untouched.
• packages/core/src/studio-api/routes/files.ts:731 — Implicit-position and string-position tweens are silently skipped. typeof anim.position !== "number" excludes both implicitPosition: true (no position arg, GSAP sequential placement — common pattern: tl.to(target, vars) chains) and string positions ("+=1", "<", ">", labels). For chained tweens with implicitPosition, the parsed resolvedStart is computed (gsapParser.ts:1043) but the source has no position arg to mutate. Dragging the clip leaves these tweens in place on the absolute timeline — they desync from the rest of the clip's animations. Either (a) skip + warn the user via toast that some tweens couldn't shift, or (b) materialize resolvedStart into a literal position arg when shifting. Silent skip is the worst of the three options; this will produce visible animation breakage with no error signal.
• packages/core/src/studio-api/routes/files.ts:732 — Lossy clamp on negative positions has no audit trail. Math.max(0, …) silently collapses any animation that would land at negative position to 0. If the user drags back +delta, the stacked-at-0 animations stay stacked — no undo within the mutation itself. Two options: (a) skip the shift entirely if any animation would go negative (refuse + toast), or (b) preserve original positions on the original DOM element and recompute from absolute snapshot. (a) is cheaper and matches NLE behavior (you can't drag a clip to a position where its content would start before zero).
• packages/studio/src/components/editor/propertyPanelTransformCommit.ts — Still orphan dead code at HEAD (129 lines, zero callers). 6d4a4a6 finishes the inline migration in PropertyPanel.tsx (removes the import, inlines commitManualOffset / commitManualSize / commitManualRotation) — but the helper file itself is left on disk. Either delete it in this PR or split it into a follow-up; landing dead code is a maintenance trap. Also: the PropertyPanel.tsx reshuffle is unrelated scope churn vs the PR title — commitManualSize even carries a // fallow-ignore-next-line complexity pragma to silence the linter on the inlined version. The unit-size violation that fallow-ignore is hiding is real; either justify the inlining in the PR body or split it out.
• packages/studio/src/utils/globalTimeCompiler.ts:29-33 — elementStart parameter added but no caller passes it. 6d4a4a6 actually threads the offset into the three return paths (elementStart + animation.resolvedStart, etc.) — but gsapDragCommit.ts:232/251/271, gsapShared.ts:62, gsapRuntimeBridge.ts:309 all still call resolveTweenStart(animation) with the default 0. So the new behavior is silently inert in this PR. Either land it together with the wire-up, or omit until needed — dead parameters in public hot-path utilities tend to silently rot, and a defaulted elementStart=0 baked into every return path is now harder to spot the next time someone touches this file.
• packages/studio/src/hooks/useTimelineEditing.ts:144 — Network failures are console.error'd and swallowed. shiftGsapPositions throws on non-2xx but the .catch only logs. The user sees no toast, no rollback — the clip is at the new data-start (DOM mutation succeeded) but the animations are at the old positions (server mutation failed). Same drift class as Concerns #2/#3. Suggest: surface a showToast and either rollback the data-start patch or refetch and reconcile.
Nits
• packages/core/src/studio-api/routes/files.ts:732 — Math.round(x * 1000) / 1000 is the same rounding helper as elsewhere in the codebase (roundTo3). Extract for consistency.
• packages/studio/src/hooks/timelineEditingHelpers.ts:165 — targetSelector: \#${elementId}`— assumeselementIdis a valid CSS id (no escaping). If a project ever has a DOM id with a colon or period, the selector won't parse. Match how other studio paths build selectors. •packages/studio/src/hooks/useTimelineEditing.ts:127—filePath = element.sourceFile || activeCompPath || "index.html". The "index.html"final fallback should never hit ifactiveCompPath` is sane; consider failing loud instead of writing to a potentially-wrong file.
Questions
• Is the resize-from-start behavior (keyframes left at original absolute positions, clip window shrinks) the intended NLE shape? In Premiere/AE, resizing the in-point doesn't move keyframes either — but content before the clip-in just doesn't play. Confirm the HF runtime already gates tween playback on the clip window; otherwise tweens fire visibly outside the clip's visible range.
• shift-positions only acts on numeric absolute positions — by design or oversight? If the project was authored with chained += positions (the GSAP idiomatic style), this PR is a no-op for those projects. Worth a PR-body note on the supported authoring shape.
Cross-PR coherence
• HF#1439 (Vance, merged today at 3ac1da42) — keyframe-drag-with-beat-snap. Touches TimelineClipDiamonds.tsx / TimelineCanvas.tsx. No surface overlap with this PR's hook layer; gestures are distinct (clip-drag vs keyframe-drag). No ordering risk.
• Prior R1-R3 clip-relative machinery — verified gone from the tree at 6d4a4a6: no clipRelativeOffsets.{ts,test.ts}, no applyClipRelativeOffsets refs, no data-position-mode refs. Revert is clean.
Review by Rames D Jusso
miguel-heygen
force-pushed
the
refactor/clip-relative-gsap-positions
branch
5 times, most recently
from
9db31cb to
f9e4fc0
Compare
Fallow audit reportFound 72 findings. Dead code (4)
Duplication (41)
Health (27)
Generated by fallow. |
miguel-heygen
changed the title
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
R2 verification at HEAD f9e4fc09. Diff since R1 (6d4a4a6) is +160/+17/-3 net across 8 files — Miguel added test coverage for shift-positions, dropped the orphan propertyPanelTransformCommit.ts + the orphan elementStart param on resolveTweenStart, and introduced a new scale-positions mutation that hooks into the resize handler. The resize-shouldn't-shift contract in the PR body is now stale (see new concern below).
Verdict: approve-with-concerns. Five of six R1 items resolved or operationally addressed; one new load-bearing concern (scale-positions has zero test coverage — same shape as the R1 top concern, fresh) and one PR-body/code contradiction worth fixing before merge.
| R1 finding | Status | Evidence |
|---|---|---|
1. Zero test coverage on shift-positions server mutation |
✅ Resolved | packages/core/src/parsers/gsapParser.test.ts:18-69 — 5 new tests: selector-scoped shift, negative clamp, delta=0 no-op, adjacent-position collision (Via's case), string-position skip. Exactly the missing assertions. |
2. implicitPosition + string-position tweens silently skipped |
✅ Resolved-differently | gsapParser.test.ts:60-69 ("skips string positions") makes the skip an asserted contract rather than a hidden bug. Still silent at runtime — see new concern below for one-shot warning surface — but documented + tested now. |
3. Negative-position clamp at Math.max(0, …) is lossy + non-idempotent |
✅ Resolved-differently | gsapParser.ts:1338 clamp behavior unchanged but gsapParser.test.ts:33-42 ("clamps negative-going positions to zero") now codifies the chosen semantics as a tested contract. The lossiness stands as a product decision. |
4. propertyPanelTransformCommit.ts orphan dead code |
✅ Resolved | File no longer in the diff — deleted. |
5. elementStart param on resolveTweenStart unused by all callers |
✅ Resolved | globalTimeCompiler.ts no longer in the diff — change reverted. |
6. Network-failure path in useTimelineEditing.ts:144 swallows errors silently |
✅ Resolved-differently | useTimelineEditing.ts:142, :188 now .catch((err) => console.error("[Timeline] Failed to …", err)). Devtools-visible. No user-facing toast, but observable. |
New concerns (introduced by the +160 expansion):
CONCERN — scalePositionsInScript has zero test coverage. packages/core/src/parsers/gsapParser.ts:1346-1383 adds a new server mutation that ratio-scales positions and durations on clip resize. It is now called from useTimelineEditing.ts:188-205 on every resize. This is the same shape as the R1 top concern, just on the sibling mutation. The math is subtler than shift (newStart + (pos - oldStart) * ratio, plus duration scaling with Math.max(0.001, …) floor) and has more failure modes — left-edge resize when oldStart != newStart && oldDuration == newDuration collapses to a shift; zero-duration tweens floor to 1ms; negative-going positions clamp lossily; selector-scoping; the early-return when nothing changes. None of those are asserted. Recommend mirroring the 5 shiftPositionsInScript tests for scalePositionsInScript before merge: ratio scaling, left-edge resize (start-only change), right-edge resize (duration-only change), negative clamp, duration floor, selector isolation.
CONCERN — PR description contradicts the implementation. The PR body says: "Resizing a clip (left or right edge) does NOT shift positions — it changes the visibility window only, matching standard NLE behavior." The current code (useTimelineEditing.ts:186-205, files.ts:740-756) DOES scale positions + durations on resize via scaleGsapPositions. This is a meaningful product decision that diverges from the documented "NLE behavior" framing. Update the body to reflect that resize now ratio-scales keyframes — and confirm that's the intended UX with Vance (R1 reviewer signed off when the contract was "resize = window-only").
NIT — Magic numbers -5 / 105 duplicated across two files. TimelineClipDiamonds.tsx:108-112 and TimelinePropertyRows.tsx:47 both clip keyframes outside [-5, 105]% of the clip during resize-preview. Pull into a shared constant (e.g. KEYFRAME_VISIBILITY_MARGIN_PCT = 5 in a shared player constants file) so the two render paths can't drift.
NIT — TimelineClipDiamonds.tsx:108-112 filter uses scaled p = kf.percentage * resizeDurationRatio but the surrounding sort/render path reads kf.percentage raw with effPct applying the same scale. It's not wrong but the filter+effPct duplicate the scaling op. A single const scaled = effPct(kf.percentage) reused across filter + render would be cheaper to reason about, especially since effPct already has the drag-override branch.
What I didn't verify:
- Whether the new
scale-positionssemantics is what Vance and the product team actually want (resize-as-stretch vs resize-as-window). I assumed the code is the source of truth and flagged the PR body mismatch. - The
resizeDurationRatioend-to-end flow — where it's set, when it's cleared, whether it can leak stale across resize sessions. I only verified the consumer side inTimelineClipDiamonds.tsx. - That the
Math.max(0.001, …)duration floor inscalePositionsInScript:1374doesn't break any downstream consumer that expects integer-ms or 0-duration tweens.
Review by Rames D Jusso
| const res = await fetch( | ||
| `/api/projects/${projectId}/gsap-mutations/${encodeURIComponent(filePath)}`, | ||
| { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ | ||
| type: "scale-positions", | ||
| targetSelector: `#${elementId}`, | ||
| oldStart, | ||
| oldDuration, | ||
| newStart, | ||
| newDuration, | ||
| }), | ||
| }, | ||
| ); |
miguel-heygen
force-pushed
the
refactor/clip-relative-gsap-positions
branch
from
f9e4fc0 to
663f43e
Compare
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Verified at HEAD 663f43e6. Approve.
R2 verification
| R2 finding | Status | Evidence |
|---|---|---|
1. scalePositionsInScript has zero test coverage |
✅ resolved | packages/core/src/parsers/gsapParser.test.ts:2335-2389 adds 5 tests covering: proportional scaling, start-edge resize (new start + shorter duration), negative-clamp to zero, no-op on identity, string-position skip. Bonus: also adds 5 shiftPositionsInScript tests (:2280-2333). Mirrors the shape Vance asked for. |
| 2. PR body contradicts code on resize semantics | ✅ resolved | New title feat(studio): scale GSAP positions on resize + shift on drag + diamond edge fixes + body lead: "When a clip is resized (either edge), all GSAP animation positions and durations scale proportionally to fit the new clip duration" and "Modeled after After Effects' Time Stretch behavior". Resize-as-stretch UX is now the explicit contract. |
3. Duplicated -5/105 magic numbers across TimelineClipDiamonds + TimelinePropertyRows |
✅ resolved | TimelineClipDiamonds.tsx:43-44 defines + exports KF_MIN_PCT = -5 / KF_MAX_PCT = 105; TimelinePropertyRows.tsx:3 imports them. Single source of truth. |
| 4. Redundant scaling op in the diamonds filter | ✅ resolved-differently | Filter is now a clean range check (kf.percentage >= KF_MIN_PCT && <= KF_MAX_PCT at TimelineClipDiamonds.tsx:118-119). The visual clamping moved into a small named helper clampDiamondLeft (:45-47). Different shape than "extract a function," but the redundancy is gone and intent reads cleanly. |
What I also re-checked
scalePositionsInScript(gsapParser.ts:1346-1383) preserves the 0.001s duration floor, the ratio-scale formulanewStart + (pos - oldStart) * ratio, and the string-position skip. All five test cases exercise these branches.- Server handler at
files.ts:731-748short-circuits on identity (oldStart === newStart && oldDuration === newDuration) and on degenerate durations — matches the parser's own guard. - Client wiring in
useTimelineEditing.ts:189-205captures pre-mutationelement.start/element.durationin the closure beforeenqueueEditfires, soscaleGsapPositions(oldStart, oldDuration, newStart, newDuration)gets the right boundaries. Drag path mirrors this withshiftGsapPositions.
New observations (nits, non-blocking)
useTimelineEditing.ts:200-203(resize) and:144-147(drag) — on a GSAP-mutation failure the catch logs but doesn'treloadPreview(), so the iframe DOM stays ahead of the on-disk file. Probably fine since the mutation failures should be rare and the next interaction will reconcile, but worth a fallback.finally(reloadPreview)if you want belt-and-suspenders.- Diamonds
(effPct(kf.percentage) / 100) * clipWidthPxis computed once per connection-line pair and once per diamond in two separate.mappasses oversorted. Not a real perf concern at typical keyframe counts; flagging only because the redundant-op thread is fresh.
Nice cleanup pass — the tests + the title/body rewrite + the shared constants all landed cleanly. Shipping as approve.
Review by Rames D Jusso
miguel-heygen
force-pushed
the
refactor/clip-relative-gsap-positions
branch
from
663f43e to
c659331
Compare
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Approved on 663f43e6.
…iamond fixes Resize: proportionally scale all GSAP animation positions and durations to fit the new clip duration via scalePositionsInScript. This preserves clip-relative keyframe percentages — diamonds don't move during resize, nothing disappears. Modeled after After Effects Time Stretch behavior. Drag: shift all GSAP positions by the time delta (unchanged from before). Diamond rendering: - Clamp diamonds at 0%/100% so they stay fully visible at clip edges - Filter out-of-range keyframes using predicted percentages during resize - Clamp connection lines to clip boundaries - PropertyRows: same edge clamping for SVG diamonds Parser: scalePositionsInScript (proportional position + duration scaling), shiftPositionsInScript (rigid shift), scale-positions + shift-positions mutation types, 5 shift tests passing.
miguel-heygen
force-pushed
the
refactor/clip-relative-gsap-positions
branch
from
c659331 to
0a0e243
Compare
5 participants
Summary
When a clip is resized (either edge), all GSAP animation positions and durations scale proportionally to fit the new clip duration — keyframes maintain their clip-relative percentages, nothing disappears. When a clip is dragged, positions shift rigidly by the time delta. Modeled after After Effects' Time Stretch behavior.
Diamond rendering fixes ensure keyframes at clip boundaries (0% and 100%) are always fully visible instead of half-clipped.
Changes
Proportional scale on resize (
scalePositionsInScriptingsapParser.ts)newPos = newStart + (pos - oldStart) * (newDuration / oldDuration)newDur = dur * (newDuration / oldDuration)"+=0.5") left unchangedRigid shift on drag (
shiftPositionsInScript— unchanged)Diamond rendering (
TimelineClipDiamonds.tsx,TimelinePropertyRows.tsx)Server API (
files.ts)scale-positionsmutation type alongside existingshift-positionsTest plan