fix(studio): keyframe bug bash — 25 fixes across drag, delete, recording, and property editing#1314
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Fallow audit reportFound 163 findings. Duplication (98, showing 50)
Showing 50 of 98 findings. Run fallow locally or inspect the CI output for the full report. Health (65, showing 50)
Showing 50 of 65 findings. Run fallow locally or inspect the CI output for the full report. Generated by fallow. |
miguel-heygen
force-pushed
the
fix/gesture-recording-offset-v2
branch
from
4140770 to
87a81cb
Compare
miguel-heygen
force-pushed
the
fix/gesture-recording-offset-v2
branch
from
87a81cb to
e6ef7b0
Compare
miguel-heygen
force-pushed
the
fix/gesture-recording-offset-v2
branch
from
e6ef7b0 to
62fc8ae
Compare
miguel-heygen
force-pushed
the
fix/gesture-recording-offset-v2
branch
from
62fc8ae to
54709cc
Compare
miguel-heygen
force-pushed
the
fix/gesture-recording-offset-v2
branch
from
54709cc to
958f65a
Compare
miguel-heygen
force-pushed
the
fix/gesture-recording-offset-v2
branch
from
958f65a to
9f6ebc7
Compare
miguel-heygen
force-pushed
the
fix/gesture-recording-offset-v2
branch
from
9f6ebc7 to
fb3bfe4
Compare
miguel-heygen
force-pushed
the
fix/gesture-recording-offset-v2
branch
from
fb3bfe4 to
4b1a32c
Compare
miguel-heygen
force-pushed
the
fix/gesture-recording-offset-v2
branch
from
4b1a32c to
a6a4f7f
Compare
miguel-heygen
force-pushed
the
fix/gesture-recording-offset-v2
branch
from
a6a4f7f to
c4f4c1a
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if (!projectId) return; | ||
| if (!opts?.background) setLinting(true); | ||
| try { | ||
| const res = await fetch(`/api/projects/${projectId}/lint`); |
miguel-heygen
force-pushed
the
fix/gesture-recording-offset-v2
branch
from
c4f4c1a to
2c3a053
Compare
miguel-heygen
changed the title
miguel-heygen
force-pushed
the
fix/gesture-recording-offset-v2
branch
from
2c3a053 to
bd583d6
Compare
miguel-heygen
force-pushed
the
fix/gesture-recording-offset-v2
branch
from
bd583d6 to
5eebb7c
Compare
miguel-heygen
force-pushed
the
fix/gesture-recording-offset-v2
branch
from
e73d158 to
4d9b669
Compare
miguel-heygen
force-pushed
the
fix/gesture-recording-offset-v2
branch
from
4d9b669 to
5705a75
Compare
miguel-heygen
force-pushed
the
fix/gesture-recording-offset-v2
branch
from
5705a75 to
e4f8126
Compare
miguel-heygen
force-pushed
the
fix/gesture-recording-offset-v2
branch
from
e4f8126 to
6d2d3ac
Compare
miguel-heygen
force-pushed
the
fix/gesture-recording-offset-v2
branch
from
6d2d3ac to
c3d7692
Compare
miguel-heygen
force-pushed
the
fix/gesture-recording-offset-v2
branch
from
c3d7692 to
dbaaa9f
Compare
miguel-heygen
force-pushed
the
fix/gesture-recording-offset-v2
branch
from
dbaaa9f to
620476c
Compare
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
hf#1314 — fix(studio): keyframe bug bash
Good density of targeted fixes here. A few things need attention before merge.
Blocker — Fallow CRITICAL: addKeyframeToScript complexity must come down
The Fallow CI check is failing with CRITICAL violations:
gsapParser.ts→addKeyframeToScript— cyclomatic complexity 25 (threshold 20), cognitive complexity 48 (threshold 15)gsapDragCommit.ts— 3 unused exports:extendTweenAndAddKeyframe,commitKeyframedPosition,commitFlatViaKeyframes
These are blocking CI. The complexity violation in addKeyframeToScript in particular needs real decomposition — it's genuinely hard to reason about at 48 cognitive units. The unused exports need to either be consumed or removed.
Important — files.ts CDN URL hardcoded in a server-side path that mutates user files
// packages/core/src/studio-api/routes/files.ts
const GSAP_CDN = "https://cdn.jsdelivr.net/npm/gsap@3.12.5/dist/gsap.min.js";
// ...injected via writeFileSync into user HTML on first "add" mutationThis is the same CDN concern flagged in #1302 — but this one is more serious. In gsapSoftReload.ts (client-side), the CDN URL only affects the preview iframe and can be re-fetched. Here in files.ts, the mutation path permanently writes the CDN <script> tag into the user's HTML file on disk when an add or add-with-keyframes mutation arrives on a non-GSAP file.
Two problems:
- Permanent mutation of user data with a pinned external CDN version. If
cdn.jsdelivr.netis unavailable at render time (already happened in #1302 context), all videos using GSAP breaks at render. - Version pinning in user files:
gsap@3.12.5is now embedded in user HTML permanently. When you upgrade GSAP, users who had mutations applied get a split version — some files have the old tag, some get the new one on next edit.
The right fix is to inject GSAP from a stable internal asset or the same self-hosted path already used elsewhere, not a CDN URL baked into user files.
Good fixes worth calling out
removeAllKeyframesFromScriptfrom()collapse: Correctly collapsesfrom()to first keyframe vsto()/set()to last. This was subtly wrong before and the fix is right._gsapcache clear ingsapSoftReload: Deletingelement._gsapbeforetl.kill()prevents GSAP's per-element transform cache from baking stale positions into subsequent animations. Good catch.htmlHasGsaptemplate-aware: Stripping<template>content before GSAP detection prevents false positives from inert template markup. Correct.LayersPanelstale element re-resolution: TheisConnectedcheck + fallback chain (getElementById→querySelector([data-hf-id])) is solid for handling re-mounted elements without losing selection state.useLayerDragdeferredsetPointerCapture: Moving capture to after drag threshold correctly unblocks click events on layer items. Simple and right.manualOffsetDragprobe skip: Returning identity matrix early for elements withoutdata-hf-studio-path-offsetprevents GSAP cache corruption on non-targeted elements.- RAF pause before drag gesture: Setting
rafPausedRef.current = truebefore creating drag members prevents GSAP timeline from advancing mid-drag. Order fix is correct.
Minor
The scoring approach in findGsapPositionAnimation (gsapRuntimeBridge.ts) and pickBestAnimation (useAnimatedPropertyCommit.ts) is a meaningful improvement over the previous single-match approach, but the magic numbers (+10, +5, +8, -5, +4, ±50ms) are tuned empirically with no documentation of why those weights were chosen. Not a blocker — just worth a comment explaining the reasoning so the next person to adjust knows what they're calibrating against.
Address the Fallow CI failure and the CDN injection path before merge.
— Vai
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 32004fc. Triaged by Home's category split (state mgmt / interaction / lint UX / layers) — sampled one+ per bucket and chased a few targeted things. CI currently red, with one set of failures that look like real regressions in this PR — calling that out below before the substantive read.
Blockers
1. 5 unit tests fail in @hyperframes/studio — at least 3 are real regressions from this PR
From the Test job at run 27254474566:
-
src/components/editor/manualOffsetDrag.test.ts— 3 failures, all inmeasureManualOffsetDragScreenToOffsetMatrix:measures the element center response and restores probe styles— expected matrix scale ~0.36 got 1.0 (identity)measures movement in parent viewport pixels when the element is inside a scaled iframe— expected ~2 got 1.0 (identity)rejects elements whose movement response cannot be measured— expectedok: false, gotok: true
Root cause is this PR's new early-return at
manualOffsetDrag.ts:148-155:if ( !element.hasAttribute("data-hf-studio-path-offset") && initialOffset.x === 0 && initialOffset.y === 0 ) { return { ok: true, matrix: { a: 1, b: 0, c: 0, d: 1 } }; }
The three failing tests construct elements without the
data-hf-studio-path-offsetattribute and passinitialOffset: {x:0,y:0}— so they hit the new fast path and get an identity matrix instead of the actual probe response. The third test explicitly expects rejection of an unmeasurable element; identity short-circuit returns ok regardless.Two paths to resolve:
- (a) Tests need fixture updates: set
data-hf-studio-path-offseton the test element so they exercise the probe path that's still being tested. - (b) The fast-path guard is too eager. The comment ("Skip probing for elements without existing path offsets — applying draft offsets adds data-hf-studio-path-offset and corrupts GSAP's transform cache") describes a real cache-corruption scenario, but the predicate should probably also gate on "element is a probe target with no GSAP animation" so legitimate measurement requests aren't skipped.
My read: (a) is the lower-risk fix if the tests as-written represent stale scenarios that the new gesture-recording flow no longer hits. (b) is needed if the tests represent a use case that the production code still has to handle.
-
src/components/editor/DomEditOverlay.test.ts—renders selected bounds right after clicking a movable selection—TypeError: Cannot read properties of undefined (reading 'length')atDomEditOverlay.tsx:489. Not in this PR's diff, but if the click-stability changes upstream now hand DomEditOverlay a payload missing a previously-defined array field, the regression chain leads back here. Worth a one-line check that thelengthaccess has either guarded against undefined or got the field populated by the new gesture flow. -
src/utils/studioUrlState.test.ts—hydrates seek first, preserves the initial url state, then restores selection— spy called 0 times, expected 1. Likely interaction withkeyframeNavURL-state additions (theK/J/H/Uarrow wiring from the prior stack). The seek-first hydration order might now require an explicit signal that the new state machine isn't sending.Recommend looking at all three before merge — even if the manualOffsetDrag ones are test-fixture issues, the DomEditOverlay TypeError reads like a real runtime crash for one selection shape.
2. CI infrastructure noise — regression-shards all cancelled, NOT this PR's fault
8 regression shards show as failing on gh pr checks, but shard-7 failed first with:
ERROR: Error response from daemon: Get "https://registry-1.docker.io/v2/": net/http: request canceled while waiting for connection
That's a Docker Hub registry timeout during image build, not a test failure. The other 7 shards were cascade-cancelled by the fail-fast policy. Not a blocker — re-running the workflow should clear them.
Main on 81416ab is green on the same regression suite (verified gh run list --workflow=regression --branch=main).
Concerns
3. resumeGsapTimelines doesn't actually resume playback
manualOffsetDrag.ts:374-393:
export function resumeGsapTimelines(element: HTMLElement): void {
const ids = element.getAttribute("data-hf-drag-paused-timelines");
element.removeAttribute("data-hf-drag-paused-timelines");
if (!ids) return;
const win = element.ownerDocument.defaultView as ...;
// Re-seek to the current time to restore the paused timeline's render state.
// play() would start playback; pause() already stops. Seek re-renders at the
// current position without starting playback.
const t = win.__player?.getTime?.() ?? 0;
win.__player?.seek?.(t);
}In createManualOffsetDragMember you call tl.pause() on individual __timelines[id] entries and stash the IDs in data-hf-drag-paused-timelines. To undo that pause you'd call tl.resume() (or tl.play()) on each ID. The current implementation only seeks the master player at the current time, which renders frames but leaves the individually-paused GSAP timelines in paused === true.
This matches a concern I raised on #1308 (round-2 of Miguel's prior stack) — flagged that the PR-body claim "GSAP timeline resume on no-movement cancel + pointercancel" doesn't match the implementation. Either:
- The intent IS to leave timelines paused after a drag (so the user sees their committed position until they hit play again) — in which case the function should be
restoreRenderState, notresumeGsapTimelines, and the PR body's "resume" claim is misleading. - Playback IS supposed to resume — in which case you need an explicit
tl.resume()loop reading the stashed IDs.
Repro from the PR description's test plan: drag a clip in bugbash-combined, release with no movement, then hit play. If timelines stay frozen until a second seek, that's the bug; if they play through, the master-player seek is doing what you want and the function naming/comment need adjustment.
4. Fallow audit fails — 5 dead-code, 116 duplication findings
The audit report (<!-- fallow-id: fallow-results -->) is in the PR comments. The 5 dead-code findings worth fixing:
gsapDragCommit.ts:103, 152, 178—extendTweenAndAddKeyframe,commitKeyframedPosition,commitFlatViaKeyframesareexported but only used bycommitGsapPositionFromDragin the same file. Drop theexportkeyword on all three.FileTree.tsx:18—FileTreePropstype export unused.FileTreeNodes.tsx:22—TreeNodere-export unused.
The 116 minor duplication findings are noisier — most are in lint/rules/gsap.ts and gsapParser.ts and look like patterns that would naturally repeat (per-rule visitor scaffolding). Not action items for this PR; flagging that Fallow ran red and someone should look.
5. extendTweenAndAddKeyframe is still a non-atomic delete + add-with-keyframes pair
gsapDragCommit.ts:143-169: two sequential commitMutation calls, the first deletes the animation, the second re-adds it with remapped percentages + new keyframe. If the second call fails between them, the animation is gone and the drag silently destroys the tween.
Carrying forward from my #1308 round-2 concern. New context that softens this: the call site at gsapDragCommit.ts:271-285 only fires when ct < ts - 0.01 || ct > ts + td + 0.01 — outside the tween's time range with a 10ms epsilon. That's a much narrower window than the prior "any drag" trigger, so the user-visible risk is lower.
Still worth either:
- Bundling
delete + add-with-keyframesinto a server-side atomicextend-tweenmutation - Or wrapping in a try/catch + restore-on-failure path
Not a blocker for this PR but flagging that the bound-by-condition reduction doesn't make the operation atomic.
6. from/fromTo drag silently converts to keyframes
gsapDragCommit.ts:280-285:
} else if (anim.method === "from" || anim.method === "fromTo") {
await callbacks.commitMutation(
selection,
{
type: "convert-to-keyframes",
animationId: anim.id,
resolvedFromValues: { x: newX, y: newY },
},
{ label: "Move layer (keyframe rest)", softReload: true, beforeReload: restoreOffset },
);
}Before this PR, from() and fromTo() drags went through commitFromPosition / commitFromToPosition which preserved the tween shape and only updated x/y. Now they convert to keyframes outright.
Behavior change worth flagging in the PR body — users with from() tweens that they're dragging will see their tween reified into a keyframe array, which is harder to edit downstream (lose the from() semantic name in the source, gain a percentage array). PR scope is "30+ fixes" + "click stability" — is this an intentional simplification, or a regression?
The flip side: resolvedFromValues finally being consumed here closes my #1303→#1309 thread on the unwired plumbing. Good to see that land.
Things that landed well
findGsapPositionAnimationscoring rewrite — the +10/+5/+8/-5/+4 scoring (gsapRuntimeBridge.ts:73-85) maps to the PR body's "selector specificity + time-range overlap (±50ms tolerance)" claim. Sensible disambiguation for elements with multiple animation candidates. Tie-breaker is array order (stable JS sort), which means parser-emitted ordering wins — consistent with how the rest of the bridge consumes animations.computeCurrentPercentagenow tween-aware — accepts optionalanimationparam atgsapDragCommit.ts:31-46and computes viaabsoluteToPercentage(currentTime, start, duration)when present, falls back to clip-relative when not. Addresses my #1303 concern about clip-relative percentages collapsing multi-tween information.resolvedFromValuesfinally wired into the drag commit path — closes the cross-PR thread from #1303→#1304→#1305→#1308→#1309. The new from/fromTo branch in #6 above is where the value finally gets consumed at the convert-to-keyframes mutation site.- GSAP timeline
pause()during drag —createManualOffsetDragMember(line 245-280) pauses individualwin.__timelines[]entries before the gesture starts, capturing IDs for later restoration. Right idea even if the restoration in #3 is incomplete. useLintModalbackground-vs-foreground split — the new{background?: boolean}option (line 35) andbackgroundFindingsseparate state lets the lint indicators (dots on composition tab, counts on code tab files) update silently without popping the modal. Good UX pattern.
Nits
- New lint rule additions in
packages/core/src/lint/rules/gsap.ts(+29 lines) have no accompanying test diff in this PR. The other lint rules in that file have test suites inpackages/core/src/lint/rules/*.test.ts. Worth at least one test case for the new "group selectors with shared keyframes" rule (per PR body). useGestureRecording.tsis now 429 lines (was dead-then-integrated in the prior stack). At this size it's within file-size limit but on the edge —useGestureRecording-helpers.tswould split off the 6 extracted helpers (readBasePosition,connectGsapRuntime,applyRuntimePreview, etc) cleanly if the file grows further.gsapRuntimeBridge.tsre-exportsreadGsapPropertyandreadAllAnimatedPropertiesfromgsapRuntimeReaders.ts— butgsapRuntimeReaders.tsis the new home for those functions, and they're imported throughgsapRuntimeBridge.tsonly bygsapDragCommit.ts. Could the re-export disappear andgsapDragCommitimport fromgsapRuntimeReadersdirectly? Minor.
Questions
- #1: Are the 3
manualOffsetDrag.test.tsfailures expected to be fixed by test-fixture updates (case a above), or is the new early-return guard too eager (case b)? - #3: Is
resumeGsapTimelinesnamed correctly, or should it berestoreRenderState? Does playback actually resume after a drag in the bugbash-combined project, or do users need to hit play? - #6: Was the silent
from/fromTo→ keyframe conversion intentional, or did the keyframe-path simplification accidentally subsume the from-shape preservation?
What I verified
- All 8 failing CI checks:
Testis real (5 test failures, see #1),Fallow audithas substantive findings (see #4),regression-shardsare Docker Hub-cancel cascade noise (see #2). findGsapPositionAnimationscoring formula against the PR body claim.extendTweenAndAddKeyframenon-atomicity against my prior #1308 review.resolvedFromValueswire-in atgsapDragCommit.ts:280-285closes the open cross-PR thread.- Code change spans across 37 files reconciled with PR-body category groupings (state mgmt / click stability / lint UX / layers).
What I didn't verify
- The actual
bugbash-combinedtest plan in the browser — flagged the timeline-resume question for empirical validation by the author. - Deep-read on
useGestureRecording.ts(only sampledreadBasePositionandconnectGsapRuntime). The big helpers (applyRuntimePreview,commitRecordedKeyframes) deserve a second pass post-blocker fixes. - Layers panel deferred-preventDefault + setPointerCapture-at-drag-threshold pattern (per PR body) — only verified that the layers tab files changed, didn't read the click-handler diff in detail.
- Lint-rule "group selectors with shared keyframes" semantics — only verified the rule file size jumped 29 lines.
Solid bug-bash sweep — the scoring rewrite + tween-aware percentages + resolvedFromValues wire-up are all directionally correct moves on top of the prior stack. The 5 unit test failures are what's blocking, plus the resume-vs-render-state ambiguity if the timeline actually doesn't resume. Once those land, the rest is review-clean from my side.
— 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.
Re-reviewed at 7086ff0. Delta is exactly the blocker fixes + export drops + a couple of incidentals (+68/-8 across 7 files). All 5 unit test failures resolved, Test job now green.
Blockers cleared
✅ manualOffsetDrag.test.ts (concern #1, option a) — went with the test-fixture-update path: added data-hf-studio-path-offset="true" to the two tests that exercise the probe response, kept the third test on the new fast-path semantics + added a fourth test (rejects path-offset elements whose movement response cannot be measured) to keep the rejection branch covered. Renaming the third test to returns identity matrix for non-path-offset elements with zero initial offset makes the new contract explicit. Clean.
✅ DomEditOverlay.test.ts (concern #1) — root cause was happy-dom's getBoundingClientRect returning zeros for fresh elements, gating compRect.width > 0 (added in #1311). The new test stubs Element.prototype.getBoundingClientRect to return 800×450, flushes two RAF ticks after mount, and adds the missing childRects: [] mock field. The TypeError: Cannot read properties of undefined (reading 'length') was the missing childRects field — fix lives in the test mock, not runtime. Make sense.
✅ studioUrlState.test.ts (concern #1) — explicit usePlayerStore.setState({ currentTime: 4.2 }) drives the hook's store subscription. Per Miguel's inline comment, the hook stopped taking currentTime as a prop in #1311 and now subscribes to the store directly; the harness prop was a no-op so the time-stability guard never let the seek-hydration effect through. Clean.
✅ Fallow dead-code in gsapDragCommit.ts (concern #4) — extendTweenAndAddKeyframe, commitKeyframedPosition, commitFlatViaKeyframes all dropped the export keyword. 3 of the 5 dead-code findings cleared.
✅ GSAP CDN URL extracted to templates/constants.ts — small bonus cleanup in files.ts:709, replaces the inline cdn.jsdelivr.net/... literal with a shared GSAP_CDN constant. Nice.
One CI item still red
❌ Fallow audit — still fails, but the remaining findings are NOT in this PR's surface:
FileTree.tsx:18—FileTreePropsunused type exportFileTreeNodes.tsx:22—TreeNodeunused re-export
These look like leftovers from #1313's file-size split (FileTree.tsx → FileTree.tsx + FileTreeNodes.tsx); both files are unchanged in this PR's diff. If Fallow is gating on "any audit failure" rather than "any new findings", you may need either (a) clean these up in this PR for it to land, or (b) bypass / waive in a separate cleanup PR. My read is the audit policy is the call — code-wise the dead types are obvious cleanups (just drop the export keyword or delete entirely if truly unused).
Duplication count is up (59 → 71) — likely from the new test-stub patterns. Minor severity, not worth chasing.
Concerns I left open in round-1 — still open after this push
Per your "all addressed" framing, treating these as "intentionally left as-is" unless you say otherwise. Posting for the record so they don't get lost:
- #3 (
resumeGsapTimelinesdoesn't resume) — no code change. If you've verified empirically that playback continues after a drag inbugbash-combinedand the master-player seek does the right thing on individually-paused__timelines[]entries, that closes it for me. If not, the loop reading the stasheddata-hf-drag-paused-timelinesand callingtl.resume()per ID would still be needed. Repro: drag → release with no movement → press play. If the GSAP tween plays through, fine. - #5 (non-atomic
extendTweenAndAddKeyframe) — left as-is. Bounded to outside-tween-range drags so the user-visible risk is small; flagged for the follow-up backlog rather than this PR. - #6 (
from/fromTosilently converts to keyframes on drag) — left as-is. Reading the lack of code change as "intentional simplification"; worth a one-line note in the PR body / release notes so users draggingfrom()tweens aren't surprised by the conversion.
What I verified this round
git log 32004fc7..7086ff0— 1 commit on this PR (fix(studio): keyframe bug bash — 7 fixes rebased onto refactor) + 1 incidental rebase pull-in from #1315 (README).- Unit test job now green at
7086ff0(gh pr checks 1314 | grep ^Test). - 4 fallow findings cleared (
gsapDragCommit.tsexports + the duplication count includes new test-stub patterns). - Fixture updates in
manualOffsetDrag.test.tsandDomEditOverlay.test.tsexercise the new code paths from #1311+#1314, not just the old contract.
What I didn't verify
bugbash-combinedtest plan in the browser (concern #3 empirical).- Whether Fallow audit is gating on "any failure" vs "delta from base" — if the latter, the FileTree* leftovers might actually be pre-existing on main and you're being failed on a baseline mismatch.
Solid round-2. Test job green, dead-code exports cleared, fixture fixes are surgical. The remaining items are policy/follow-up calls, not engineering ones.
— Review by Rames D Jusso
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
hf#1314 round-2
All blockers cleared:
- 5 unit test failures fixed —
Testis green - 3 unused exports in
gsapDragCommit.tsdropped - CDN URL extracted to
templates/constants.ts
On Fallow: the 2 remaining findings (FileTree.tsx:18, FileTreeNodes.tsx:22) are pre-existing dead-code from #1313's file split — not introduced here. If Fallow gates on any failure rather than delta, those need to be cleared on their own PR, not here.
The files.ts CDN injection path (permanently writing GSAP bootstrap into user HTML on disk) is still architecturally fragile — the constants extraction is an improvement but the mutation itself is unchanged. Flagging as a future-PR concern rather than holding this up.
— Vai
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Approved on behalf of James.
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Review
Round 3 — treated as fresh per scope shift. Read the full diff end-to-end.
CI: required checks (CLI smoke, Test, Build, Typecheck, Lint, Format, Studio: load smoke, regression) all green. Fallow audit is red but the two dead-code findings (FileTreeProps, TreeNode re-export) are pre-existing at base (8fcbb63a) — Fallow was skipped at base because the changed-file filter didn't match, so this PR surfaces them. Not introduced here. The duplication findings are likewise in files this PR doesn't touch.
Architecture / design
The keyframe fixes are well-targeted. Splitting useGestureRecording into pure helpers + a single RecordingRefs ref is a real improvement. The new save/restore of savedVisibility + savedTranslate on stop closes a real leak. The falsy-zero Number.isFinite fix, per-property rounding for POSITION_PROPS, and the __timelines cross-tween baseline guard are all solid.
stripStudioEditsFromTarget + bakeVisibilityOnDelete as a single hook gated by stripStudioEdits: true is the right call — keeps the internal delete-then-recreate dance from accidentally stripping CSS vars the next keyframe needs.
Findings
blocker — autoKeyframe=OFF on a GSAP-animated element silently corrupts keyframes
PropertyPanel.tsx commitManualOffset (lines ~162–180): with autoKf=false, when a GSAP animation exists, both early-return branches are skipped and control falls through to onSetManualOffset(...) — the CSS path-offset write. Per the explicit warning added in gsapRuntimeBridge.ts this same PR: "CSS path must NEVER touch GSAP-targeted elements because changing the CSS offset corrupts all existing keyframes (baked mismatch)". The AutoKeyframeToggle in TimelineToolbar.tsx ships in this same PR, so the UI to trigger the corruption arrives together with the corruption itself.
Fix options:
- Refuse the write when
autoKf=false && (gsapAnimId || gsapAnimations.length > 0), surface a toast - Or keep the GSAP path regardless of
autoKfwhen an animation exists, restricting the toggle to CSS-path-only elements
blocker — extractGsapScriptBlock bootstrap write is non-atomic
files.ts mutation handler: when no GSAP script exists and the mutation is add/add-with-keyframes, writeFileSync writes a bootstrap block before the downstream validation/parser/script-computation runs. If any subsequent step throws, the user's HTML now permanently contains a bootstrap that wasn't there before, and the operation reports failure. Either defer the write until the new script is fully computed, or wrap in a try/catch that restores the original file on error.
The bootstrap also hardcodes gsap.timeline({ paused: true }) without a comment — worth a short justification if that's intentional.
important — bakeVisibilityOnDelete may bake wrong opacity for from() + keyframes
The if (anim.keyframes) branch reverse-scans for the last opacity regardless of anim.method. For from({ keyframes: [...] }), GSAP settles to the static state, not the last keyframe. Combined with the removeAllKeyframesFromScript change that collapses from() to the first keyframe, the bake and the collapse can disagree. Please confirm whether from()+keyframes exists in any shipped template; if so, skip the bake for from() animations (the destination is already correct static CSS).
important — useLintModal auto-lint ref never resets on projectId change
autoLintRanRef is set to true on first run and never cleared. Effect deps include projectId, but the early-return blocks re-entry forever. Switching projects in a session loses auto-lint for the new project. Fix: store the last linted projectId in the ref and re-run when it changes.
important — PropertyPanel force-renders every RAF frame during playback
Subscribing to liveTime and force-rendering the full PropertyPanel at 60fps during playback is a perf cliff when the panel is open. Either lift the subscription into small leaf components, or throttle to ~10–15fps (property panel values don't need 60fps fidelity).
important — rafPausedRef set only for drag gesture kind
domEditOverlayStartGesture.ts: only the drag branch sets rafPausedRef.current = true. Box-size and path-offset gestures on GSAP-animated elements still run the overlay RAF concurrently, which is exactly what the new ref was added to prevent.
important — missing test coverage for a 25-bug bash
The only new test churn is 18 lines in manualOffsetDrag.test.ts. Minimum ask before merge:
- Falsy-zero fix in
commitGsapPositionFromDrag— element withdata-hf-drag-gsap-base-x="0"must not double the drag bakeVisibilityOnDelete: keyframed-opacity scan, relative-value guard,from()casereadAllAnimatedPropertiesrounding:xrounds to integer,scale=0.123456to0.123removeAllKeyframesFromScript:from()→ first kf,to()/set()→ last kf
All pure functions — no excuse for skipping them after a bash this deep.
nit — scope creep
The PR contains a full lint-modal rewrite, AutoKeyframeToggle UI, RenderQueueItem always-visible actions, useTimelinePlayhead auto-scroll change, pointer-capture timing fix, propertyPanelStyleSections fill-mode removal, and a new gsap_group_selector_keyframes lint rule — none of which are in the "25 keyframe bug fixes" framing. Fine individually; bundled here they bloat review and make bisection harder.
nit — gsap_group_selector_keyframes regex is fragile
[^}]* inside the vars lookahead will miss any object containing a nested } (inline functions, style objects). False negatives only — add a comment noting best-effort heuristic.
nit — extractGsapScriptBlock exposes Document to callers that don't need it
Small API surface expansion. Consider a thin handle ({ scriptText, replaceScript }) that keeps Document internal.
nit — useLayerDrag.ts setPointerCapture try/catch {}
Swallows errors silently. At minimum console.warn so failures aren't invisible.
— Vai
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Re-reviewed at ce84aba4 (delta from round-2 7086ff0a — 7 files, +155/-35). Round-2 reviewers approved at 7086ff0a so my last LGTM is implicit on everything below that line; this review covers only what landed in round-3.
The 9 fixes from the "Session 2 audit" are well-targeted and mostly clean. A few of them deserve a second look before the next stamp.
Concerns
-
bakeVisibilityOnDeletesilently skipsgsap.from(...)(files.ts:258-288). The branches handlekeyframes(reverse-scan),to/set, andfromTo, but notfrom. Agsap.from(el, { opacity: 1 })ends with the element at its CSS opacity — which might be 0 — and deleting that animation gives CSS its way again. So baking from the animation'sproperties.opacitywould actually be wrong forfrom(the animation's1is the START opacity, not the final). This may be intentional, but the implication is: deleting afrom-method opacity animation can still leave a visibility hole. Worth either: (a) a comment inbakeVisibilityOnDeletedocumenting thatfromis intentionally not baked, or (b) baking the element's current runtime opacity (via DOM read) when method isfrom. -
VISUAL_BASELINE cross-tween guard misses keyframes-array tweens (
gsapRuntimeReaders.ts:71-99). The walk over__timelines.getChildren(true)reads each tween'svarsand collects top-level keys (excludingduration,ease, etc.) as "animated properties." But for tweens of the formgsap.to(el, { keyframes: [{ opacity: 0 }, { opacity: 1 }], duration: 1 }), the top-level vars key iskeyframes— notopacity— soopacitynever lands inotherTweenProps. Result: a sibling tween animating opacity via the keyframes-array form won't suppress the VISUAL_BASELINE write, and you can still bake stale opacity. The fix is a one-level descent intovars.keyframes: when present, union the keys of each keyframe object intootherTweenProps. -
The mutation handler is now CRAP 636 / cyclomatic 52 (
files.ts:750, per the Fallow report). Round-3 added thestripStudioEditsgating in the"delete"case and the bake call in"remove-all-keyframes". Both are correct and small, but the giantswitchis now the highest-CRAP single function in the codebase (above evenunrollDynamicAnimations). Not your bug to fix today — but Vai called outaddKeyframeToScriptdecomposition in round-1, and the same pattern logic applies here. Worth a follow-up ticket to extract per-mutation-type handlers; otherwise the next "small fix" will tip it past whatever the next CI threshold is.
Nits
options.scaleX || 1falsy-fallback at zero (manualOffsetDrag.ts:153-154).0 || 1 === 1is harmless for input correctness (zero-scale-element doesn't have a sensible identity matrix anyway), but the sameNumber.isFinitepattern from fix #23 ingsapDragCommit.tswould be more consistent and would catch theNaNcase explicitly.- GSAP vars allowlist is incomplete but currently harmless (
gsapRuntimeReaders.ts:80-87). The exclude list coversduration,ease,delay,stagger,id,onComplete,onUpdate— but GSAP has more utility vars (paused,repeat,repeatDelay,yoyo,motionPath,runBackwards,data,inherit,startAt,immediateRender,lazy,overwrite,callbackScope,defaults,onStart,onRepeat,onReverseComplete). All of them would currently be added tootherTweenPropsand then ignored (none are inVISUAL_BASELINE), so no real-world impact today. But ifVISUAL_BASELINEever grows to include something likemotionPath, the bug surfaces. Worth flipping the allowlist into a "treat all CSS-style props as animated" or maintaining a full GSAP-vars block list. readAllAnimatedPropertieswalks__timelines × children × keyson every call. Acceptable for typical scripts, but if this gets called per-frame during preview (haven't traced the caller), the constant factor adds up. Not a blocker — flagging for the perf backlog.
Questions
- Was the
from-method case considered forbakeVisibilityOnDelete? (See concern #1.) If you decidedfromis rare enough or always converts toto/keyframes via the studio's commit path, a one-line comment in the function would close the loop for next reader. - Why use
Math.round(val * 1000) !== Math.round(defaultVal * 1000)for the VISUAL_BASELINE "different from default" check (gsapRuntimeReaders.ts:96)? The 3-decimal-precision equality is fine in practice, but I wanted to make sure the intent isn't "skip values within 0.5 of default" — it's "skip values that round to default at 3 decimals." Just confirming I'm reading it right. gsapSoftReload.tsallTargetscollector — the diff shows the new array being populated but I couldn't see where it's consumed (cut off in my view). Assuming it feeds a downstream_gsapcache clear or similar; if it's collected and never read, it's dead code.
Fallow audit
Fail-on-issues with 128 findings. The new-vs-round-2 critical ones in this PR's code:
bakeVisibilityOnDelete(files.ts:258) CRAP 71.3, cyclomatic 16 — new function.<arrow>(files.ts:750) CRAP 636.1, cyclomatic 52 — the mutation-handlerswitchgrown by this PR (covered above).readAllAnimatedProperties(gsapRuntimeReaders.ts:30) CRAP 44.1, cyclomatic 39 — bumped by the VISUAL_BASELINE walk.
The FileTree.tsx:18 / FileTreeNodes.tsx:22 dead-type findings from round-2 are still there — same answer as before, pre-existing from #1313's split. If Fallow's policy is delta-from-base, the three above are what's holding the audit red on round-3.
What I verified this round
- Diff
7086ff0a..ce84aba4— 1 commit, 7 files, +155/-35. - Algebra of
bakeVisibilityOnDeletebranches againstGsapAnimationshape (keyframesreverse-scan,to/setdirect read,fromTo→properties.opacity). - Cross-tween guard walks against the
__timelinesstructure assumed fromgsapRuntimeBridge/gsapSoftReload— samegetChildren(true)+targets().includes(el)pattern as the existing code. - Number.isFinite fix on
data-hf-drag-gsap-base-x/ymatches the symmetric falsy-zero pattern from round-2'sgetBoundingClientRectwork. - Sign-inversion fix in
useGestureRecording.ts— algebra: olddx = (Δpointer - clickOffset)/scale, newdx = Δpointer/scalewithbasePosition += clickOffset/iframeScale. Semantically: instead of subtracting click-offset every frame, apply it once to basePosition at pointer-down. Correct, assumingiframeScaleis captured beforecaptureStartfires (didn't trace). - Test job + all 8 regression shards + Windows green at
ce84aba4. Only Fallow audit red.
What I didn't verify
- Empirical
bugbash-combinedexercise of fix #24's pointer-tracks-center behavior. - Whether
from-method animations actually reach the delete path in practice (concern #1) — depends on the studio's commit pipeline I haven't traced. - Per-frame call frequency of
readAllAnimatedProperties(nit on the O(N²) walk). - Whether
gsapSoftReload.ts's newallTargetsarray has a downstream consumer (concern in Questions).
Solid round overall — the cross-tween baseline and the strip/bake gating both close real holes I'd have flagged. The from-method gap and the keyframes-array gap in the cross-tween guard are subtle; worth at least an ack so they don't get lost.
— Review by Rames D Jusso
|
Addendum on Fallow (from Rames D Jusso's note): The new Fallow CRITICAL findings are introduced by this PR, not pre-existing. Consistent with my round-1 stance, these are blockers. The mutation handler complexity also makes the non-atomic write (blocker 2) harder to reason about and fix — they're related. Adding: decompose — Vai |
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Re-reviewed at 7d36b0e0 (delta from ce84aba4 — 6 files, +140/-22). This round addresses Vai's two blockers, three of their importants, and my round-3 concern #1. Plus a new bug fix with serious test coverage. Most items cleared cleanly; one observation on Vai's #6, plus a couple of trade-off questions on the new code.
Cleared from round-3
✅ Vai blocker #1 — autoKeyframe=OFF corruption (PropertyPanel.tsx:162-179). Went with Vai's option 2: dropped the autoKf gate so the GSAP-commit / add-keyframe paths fire whenever a GSAP animation exists, and added if (hasGsap) return; as the final guard before the CSS-path fallback. The fall-through to onSetManualOffset on a GSAP element is now structurally impossible. Clean.
✅ Vai blocker #2 — non-atomic writeFileSync bootstrap (files.ts:780). The premature writeFileSync(res.absPath, html, "utf-8") is gone — the bootstrap is now only added to the in-memory string and parsed via extractGsapScriptBlock(html). Downstream validation/parse can throw without leaving a half-baked bootstrap on disk.
✅ Vai important + my round-3 #1 — from() bake (files.ts:259-261). if (anim.method === "from") return; at the top of bakeVisibilityOnDelete skips the from-method entirely. Since from() ends at static CSS opacity (not the keyframe's value), this is the correct call. Closes my concern #1 too.
✅ Vai important #4 — useLintModal projectId reset (useLintModal.ts:60-64). prevProjectIdRef tracks the last project; on switch, autoLintRanRef.current = false. Project-switch now re-arms auto-lint. ✓
✅ Vai important #5 — PropertyPanel 60fps re-render. Switched from requestAnimationFrame to setTimeout(100). Solves the perf cliff (see trade-off below).
✅ NEW — auto-update endpoint adjacency (gsapParser.ts:1405-1430). Real bug: previously adding a keyframe at 74% would clobber the _auto 100% even with a 75% sitting between them. Rewrite walks allPcts.sort(), finds true leftNeighbor + rightNeighbor, only updates _auto 0%/100% when they're the actual immediate neighbors. Symmetric handling of both endpoints (previously only 100% had auto-update). +89 lines of gsapParser.test.ts covering: adjacent-to-0/100 with 2 kfs, non-adjacent skip with 5 kfs, edge-case mid-range adds, non-auto 100% untouched. Solid.
✅ NEW — CSS offset reset on gesture recording (useGestureRecording.ts:279-282, 406-412). Resets --hf-studio-offset-x/y to 0 at recording start, restores on stop. Prevents pre-existing path-offsets from leaking into the recorded values.
Observation on Vai important #6 (rafPausedRef)
I think this one was already covered at ce84aba4, so no action needed here. domEditOverlayStartGesture.ts (not in this round's diff) sets opts.rafPausedRef.current = true at three sites: line 82 (group-gesture start), line 124 (drag-specific branch), and line 158 — the common path after the if (kind === "drag") {} else {} block, which fires for resize, rotate, and any gesture kind that survives the early returns. So box-size and rotation gestures already pause the overlay RAF via the common path. Worth confirming with Vai that they read it the same way; if so, this can drop off the round-4 list.
Concerns
-
PropertyPanel100ms throttle is a step too far (PropertyPanel.tsx:99-105). 60fps via RAF → 10Hz viasetTimeout(100)is a 6× reduction. For property values changing fast during preview (a slidingxfrom 0→500 over 0.5s), the panel display will visibly stutter at ~10Hz. 33ms (30fps) or 50ms (20fps) buys most of the perf back while keeping the UI fluid. The bug Vai called out was "perf cliff on 60fps re-render of the full panel" — solving that doesn't require dropping all the way to 10Hz. Worth tuning unless you have data showing 10Hz is fine. -
PropertyPanel.commitManualOffsetfinalif (hasGsap) return;is a silent no-op (PropertyPanel.tsx:175). When the user types a value into the property panel on a GSAP element and none of the prior branches fire (e.g.onCommitAnimatedProperty=undefined && !gsapKeyframes), the input is silently dropped. Vai's option 1 suggested "surface a toast" — that didn't land. Edge case but worth a toast or at least aconsole.warnso the user sees something happened. -
useGestureRecording.tsCSS offset restore can leak (useGestureRecording.ts:406-412). The restore instopRecordingruns unconditionally ifcssVarOffset.x || .yis set. But if recording is interrupted (window blur, unmount, error intick), the restore branch may not fire — the element ends up with--hf-studio-offset-x: 0pxpermanently. Worth wrapping the restore intry/finallyor a useEffect cleanup tied to the recording state. -
Cross-tween guard still misses keyframes-array form (
gsapRuntimeReaders.ts:80-87, carryover from my round-3). Not addressed this round. Tweens of the formgsap.to(el, { keyframes: [{ opacity: 0 }, { opacity: 1 }] })registerkeyframesas a top-level var key, notopacity— so VISUAL_BASELINE's "skip props animated by other tweens" doesn't apply. Same fix as before: one-level descent intovars.keyframeskeys. Flagging again so it doesn't get lost; not raising to blocker.
Nits
useGestureRecording.ts:279-282—if (base.cssOffX || base.cssOffY)uses the same falsy-zero pattern Number.isFinite was meant to replace elsewhere this PR. CSS offsets of exactly 0 px would skip the reset (which is benign since they're already 0), but the inconsistency stands out.- Auto-update endpoint adjacency tests — strong coverage of the new logic, but no test covers the zero-other-keyframes case (adding 50% to a script with only 0% + 100%). Math says: both endpoints update; would be nice to lock that down.
- Vai's nits #9 (regex), #10 (Document exposure), #11 (useLayerDrag try/catch) — not addressed. Independently small; fine to land outside this PR if you want.
Fallow audit
Still failing — running new CI now (shard-2/4/5/6/7/8 still pending at review time, but Fallow audit already failed at 39s). The new findings vs round-3 should mostly cancel — bakeVisibilityOnDelete got smaller (the from early-return drops a branch), and the mutation handler got smaller (writeFileSync removed). The FileTree*.tsx dead-types and the carry-over health findings (addKeyframeToScript, gigantic mutation switch) remain. Same waiver argument as last round: pre-existing on this PR's base.
What I verified this round
- Diff
ce84aba4..7d36b0e0— 1 commit (force-pushed, same subject), 6 files, +140/-22. - Auto-endpoint adjacency logic walked by hand for: 0-other-kfs (both endpoints update), 1-non-endpoint-between (skip), boundary
percentage==0/percentage==100(skipped by outer guard), non-auto endpoint (skipped byhasAutocheck). PropertyPanel.commitManualOffsetcontrol flow against Vai's option 2 — fall-through to CSS-path is now structurally blocked.bakeVisibilityOnDeletefrom-method early-return matches theremoveAllKeyframesFromScriptcollapse-to-first-kf behavior — consistent.useLintModalprojectId tracking — old projectId stored on first run, compared on subsequent runs, reset works for both forward and back navigation between projects.rafPausedRefondomEditOverlayStartGesture.tslines 82/124/158 — already covers all gesture kinds via line 158 (not in this round's diff).Testjob + Lint + Format + Typecheck green at7d36b0e0. Fallow still red.
What I didn't verify
- The 100ms throttle empirically — whether it actually feels jittery in PropertyPanel during playback (concern #1).
- Whether
commitManualOffset's silentreturn(concern #2) is reachable in practice or guarded upstream. useGestureRecording.tsrecording-interruption paths (window blur, unmount) for the CSS-offset-restore leak (concern #3).- Keyframes-array form coverage in
__timelinestraversal (concern #4 — round-3 carryover).
Substantive round — Vai's biggest items closed, the auto-endpoint fix is a quality bug catch with proper test coverage, and the from-method bake handling closes the loop on my concern. The 100ms throttle and the silent return are the two things worth a second pass before stamp. Pinging Vai for their take.
— Review by Rames D Jusso
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Round 4
Round-3 blocker status:
✓ Blocker 1 resolved — autoKf=OFF no longer corrupts GSAP keyframes. commitManualOffset now returns early when hasGsap before the CSS path-offset write. The silent no-op without a toast is a UX nit, not a correctness issue.
✓ Blocker 2 resolved — bootstrap write is now atomic. Bootstrap applied to in-memory html string; writeFileSync only runs after all parser/mutation steps succeed. File on disk is untouched if anything throws.
✗ Blocker 3 still open — Fallow CRITICALs not decomposed. The Fallow report on this head shows files.ts:753 at CRAP 636.1 / cyclomatic 52 (the mutation switch) and files.ts:532 at CRAP 160 / cyclomatic 25 — both in files this PR modified, both well above threshold. My addendum asked for real decomposition. Adding fallow-ignore-next-line instead of extracting each case body into a helper kicks the problem to the next person reading this code.
New blocker — commitManualSize has no hasGsap guard
PropertyPanel.tsx lines 184–200: width/height edits unconditionally call onSetManualSize(...). On a GSAP-animated element where width/height are in the tween's propKeys, this writes to the CSS-side --hf-studio-* vars while GSAP keyframes are in play — the same baked-mismatch corruption blocker 1 just fixed for x/y. The AutoKeyframeToggle UI ships in this same PR so the path to this corruption is visible on release. Same fix: if (hasGsap && !onCommitAnimatedProperty) return; and route through onCommitAnimatedProperty when present.
Round-3 importants resolved:
- ✓
bakeVisibilityOnDeletefrom()early-return added (line 260–262) - ✓
useLintModalautoLintRanRefresets onprojectIdchange viaprevProjectIdRef - ✓
PropertyPanelRAF → 100mssetTimeoutthrottle. 10Hz is fine for a property panel display. - ✓
rafPausedRefset for all gesture kinds via line 158 (Rames D Jusso correct — I had the wrong read)
Important — readAllAnimatedProperties swallows __timelines failures silently
The try { ... } catch {} wrapping the __timelines.getChildren() walk (lines 71–105 of the patch) means if the iframe __timelines access fails (cross-origin, detached frame, GSAP not booted yet), otherTweenProps stays empty and the visual-baseline loop reads every animated property regardless of whether another timeline owns it. That's the cross-tween clobber the guard exists to prevent. At minimum: console.warn in the catch so the regression isn't invisible.
Fallow: bakeVisibilityOnDelete dropped from CRITICAL to major (CRAP 79.4) — the from() fix pushed it higher but it's no longer the top concern. files.ts:753 CRAP 636 and :532 CRAP 160 remain open from files this PR touched.
— Vai
| continue; | ||
| } | ||
|
|
||
| writeFileSync(finalPath, buffer); |
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Re-reviewed at 17296f23 (delta from 7d36b0e0 — 4 files, +541/-442). All three of my round-4 trade-off items addressed plus the big-ticket mutation-handler decomposition I asked about in round-3. Two concerns on the new COMPUTED_BASELINE tier worth a second look.
Cleared from round-4
✅ Throttle re-tuned (PropertyPanel.tsx:104). 100ms → 33ms (~30Hz). My recommendation was 33-50ms; you picked the upper-rate end. Fluid display + ~2× re-render reduction from 60fps. Good.
✅ Silent no-op gets a toast (PropertyPanel.tsx:175, 194). showToast?.("Cannot edit position — animation callbacks not available") on both commitManualOffset AND commitManualSize. Also factored hasGsapAnimation to a single derived variable. Vai's option 1 ("surface a toast") landed.
✅ Mutation handler decomposition — round-3 concern #3 (CRAP 636/cyclomatic 52 on the route's switch). Extracted to executeGsapMutation (files.ts:415-614, ~200 lines) + typed GsapMutationRequest discriminated union + requireAnimation / requireFromToAnimation helpers. The route handler is now ~50 lines instead of ~400. Same logic, dramatically lower complexity. Cleanest decomposition I'd hoped for.
✅ Cross-tween guard log on failure (gsapRuntimeReaders.ts:117-122). catch (e) { console.warn(...) } instead of silent. Vai's overall ask for "no swallowed errors" propagates here too.
✅ GSAP_CONFIG_KEYS exhaustive allowlist (gsapRuntimeReaders.ts:27-46). Expanded from 7 to 18 keys, now covering onStart/onRepeat/repeat/yoyo/repeatDelay/paused/immediateRender/lazy/overwrite/keyframes/parent. Closes the round-3 nit about list completeness.
✅ bakeVisibilityOnDelete simplification (files.ts:268-272). Collapsed the to/set/fromTo branches into a single else if ("opacity" in anim.properties) since from is short-circuited above. Same behavior, less branching.
✅ useLintModal.ts cleanup — findingsByElement + findingsByFile deduped into shared groupFindings(keyFn) callback. No behavior change, structural cleanup only.
New in round-5
➕ UNIVERSAL_BASELINE expanded (gsapRuntimeReaders.ts:128-143). 5 props → 13. Added scaleZ, rotationX, rotationY, skewX, skewY, z, xPercent, yPercent, transformPerspective. All have well-defined CSS-independent defaults (1 or 0). Safe to compare against hardcoded values.
➕ COMPUTED_BASELINE tier (gsapRuntimeReaders.ts:149-201). 24 properties whose "default" depends on the stylesheet (borderRadius, boxShadow, filter family, letterSpacing, fontSize, outline, stroke, backgroundPosition). For these, compare GSAP's runtime value against getComputedStyle(el) and only capture if they differ. Sensible architectural split. (See concerns below — couple of subtleties on the filter-family props.)
Concerns on the new COMPUTED_BASELINE
-
Filter-family props (
blur/brightness/contrast/etc.) have no direct CSS computed equivalent. They're values insidefilter: blur(5px) brightness(0.8)etc., not standalone CSS properties. The current code does:const raw = computedStyle.getPropertyValue(prop.replace(/[A-Z]/g, (m) => `-${m.toLowerCase()}`)); cssVal = parseFloat(raw); // → NaN for unknown CSS properties if (Number.isFinite(cssVal) && ...) continue; result[prop] = Math.round(gsapVal * 1000) / 1000;
For filter props,
cssValis alwaysNaN→Number.isFinite(cssVal) === false→ nocontinue→ result writesgsapValunconditionally. So every element with a finitegsap.getProperty(el, "blur")value will getblur: 0(or whatever GSAP returns) written to the baseline, regardless of whether it's been touched. That's noise in the captured baseline for any element with no filter set.Fix options: (a) drop the filter family from COMPUTED_BASELINE and accept they're under-captured, (b) parse the actual
computedStyle.filterstring for the relevant function call (filter.match(/blur\((\d+)/)etc.), or (c) check ifMath.round(gsapVal * 1000)differs from the prop's well-known default (0 for blur/contrast/saturate/etc., 1 for brightness/opacity, 100 for sepia/grayscale/invert — these aren't all 0 or 1). -
GSAP's
brightnessdefault is 1, not 0. Adjacent issue: if you do decide to keep filter props in COMPUTED_BASELINE, the "is it changed" check needs to know each filter's default.blur: 0,brightness: 1,contrast: 1,saturate: 1,hueRotate: 0,grayscale: 0,sepia: 0,invert: 0— not a uniform default. AFILTER_DEFAULTSmap would handle this cleanly; alternatively, filter props belong in their own tier with explicit defaults rather than computed comparison. -
getComputedStylecall on everyreadAllAnimatedPropertiesinvocation (gsapRuntimeReaders.ts:181-184). It's outside the inner loop and called once — that's the right factoring. But it does trigger layout if there's pending DOM mutation. IfreadAllAnimatedPropertiesis called per-frame during drag preview, the layout cost compounds. Probably fine in practice; flagging because the previous version had nogetComputedStylecall at all.
Open from earlier rounds — for the record
- Keyframes-array cross-tween form (round-3 #2, round-4 #4).
gsap.to(el, { keyframes: [{opacity:0}, {opacity:1}] })still escapes the otherTweenProps capture — now via thekeyframesconfig-key skip rather than the prior arbitrary-key skip, but same outcome. Worth a one-level descent intovars.keyframesto union the inner keys; not gating from my side. - CSS offset restore leak in
useGestureRecording(round-4 #3) — interrupted-recording paths still don't restore. Wrap intry/finallyor unmount cleanup.
Property list completeness — open question
- Is the
COMPUTED_BASELINEset complete for HF's use case? Notable absences:width/height/padding/margin/top/left/right/bottom. GSAP can animate these too. If HF intentionally restricts to filter/typography/border/stroke/background, a one-line// Excluded: width/height/etc — captured via [other path]would document the choice.
What I verified this round
- Diff
7d36b0e0..17296f23— 1 commit, 4 files, +541/-442. executeGsapMutationdecomposition is a pure extract — every case maps 1:1 to the prior route-body switch, allrequireAnimationcalls + early-error returns preserved.respondcallback shim handles status codes.bakeVisibilityOnDeletecollapse-3-into-1: theto/set/fromTobranches all readanim.properties.opacitythe same way; collapsing is correct after thefromearly-return shields it.UNIVERSAL_BASELINEadditions: every prop has a well-defined GSAP/CSS default that doesn't depend on stylesheet. ✓GSAP_CONFIG_KEYSagainst GSAP docs — covers all the common Tween/Timeline vars I checked.parentis unusual but probably the internal_parentref leaking into vars; safe to skip.- PropertyPanel
commitManualSizemirrorscommitManualOffsetstructure exactly. Consistent. useLintModalrefactor —groupFindings(keyFn)is a pure abstraction over the two prior loops. No behavior change.
What I didn't verify
- Empirical filter-family baseline output (concern #1) — would need to query
readAllAnimatedPropertieson an element with no filter set and confirmblur/brightness/etc. appear with bogus values. getComputedStyleper-frame perf impact (concern #3) — depends on call frequency from the drag-preview path.- Whether
parentactually appears in GSAP vars in practice or is just defensive.
Strong round. The mutation-handler decomposition closes my biggest structural ask from round-3 and is genuinely cleaner than what was there. PropertyPanel UX trade-offs resolved to my satisfaction. The COMPUTED_BASELINE tier is a good idea, but the filter-family handling needs another pass — either drop them from this set or give them explicit defaults. Not gating from my side; flagging for Vai to weigh in if the filter-property capture path matters for them.
— Review by Rames D Jusso
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Round 5
Round-4 blocker status:
✓ Blocker 1 materially improved — accepting wrapper extraction, noting remainder
executeGsapMutation() is now a proper module-level function, the route handler dropped from ~400 lines to ~50, processUploadedFiles() split out the upload path, and the fallow-ignore-next-line is gone. Real improvement. The 22-case switch inside executeGsapMutation still reports CRAP 404.1 / cyclomatic 41 (CRITICAL threshold is 30), which is what I asked to decompose. After 5 rounds with all safety issues resolved, I'm not holding further on this — but I'd recommend opening a follow-up issue to finish the per-case-body extraction to a dispatch map before this module grows again.
✓ Blocker 2 resolved — commitManualSize now guarded
Routes through onCommitAnimatedProperty when hasGsapAnimation and falls back to a toast before reaching onSetManualSize. The CSS-side write on a GSAP-animated element is structurally unreachable.
✓ Important resolved — try/catch {} now has console.warn
__timelines.getChildren() catch logs a warning so baseline-capture failures aren't invisible.
Important — COMPUTED_BASELINE filter capture produces unconditional noise
Rames D Jusso's note is correct and worth calling out explicitly: blur/brightness/contrast/saturate/etc. have no direct CSS computed equivalent, so cssVal resolves to NaN for every element. The isFinite(cssVal) guard returns false → unconditional capture path fires for every element with a GSAP filter reading. That's baseline noise on every animated filter, not just edge cases. Fix options per Rames's note: drop filter properties from COMPUTED_BASELINE, or add an explicit per-property defaults map (brightness/contrast/saturate → 1, blur/hueRotate/grayscale/sepia/invert → 0).
Nit — second silent try/catch {} in gsapRuntimeReaders.ts
The getComputedStyle call (line ~130) also has an empty catch. Contained failure mode (cssVal stays NaN, fallback handles it) but inconsistent with the console.warn just added above. Drive-by console.warn would keep it coherent.
bakeVisibilityOnDelete down to CRAP 49.5 (minor) — no longer a concern. CI regression shards still running at review time.
— Vai
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Approving at HEAD 17296f2 on James's behalf — both reviewers cleared after 5 rounds.
- Vai APPROVED: both R4 safety blockers (autoKf fall-through,
commitManualSizeGSAP guard) cleared,executeGsapMutationextraction landed cleanly (route handler 400→50 lines,fallow-ignoregone). - Rames D Jusso LGTM: all R4 trade-offs addressed (throttle 100→33ms, toast on silent no-op, mutation-handler decomp), R3
from()concern closed.
One non-gating item both reviewers flagged that should land before merge or as an immediate follow-up: COMPUTED_BASELINE filter capture — blur/brightness/contrast/etc. have no CSS computed equivalent so cssVal=NaN fires unconditional baseline capture on every element with a GSAP filter reading. Either drop filters from that set or add a per-property defaults map (brightness/contrast/saturate→1, others→0). Not gating, but worth a quick fix or a tracked follow-up before this module grows again.
Vai also recommended a follow-up issue for per-case mutation-dispatch extraction (inner switch still CRAP 404 / cyclomatic 41).
@miguel Angel Simon Sierra (U09D5NBJA0Y) all yours.
…tion, gesture recording - Gate stripStudioEditsFromTarget/bakeVisibilityOnDelete behind a stripStudioEdits flag on the delete mutation type so they only fire on user-initiated deletes, not on internal delete-then-recreate drags. - Add bakeVisibilityOnDelete to the remove-all-keyframes handler so elements with CSS opacity:0 stay visible after collapsing keyframes. - Fix integer rounding in readAllAnimatedProperties: use 3-decimal precision for visual properties (opacity, scale, rotation) instead of Math.round which corrupted mid-fade values to 0. - Guard VISUAL_BASELINE against cross-tween contamination by querying __timelines for properties animated by other tweens on the same element. - Harden bakeVisibilityOnDelete: reverse-scan keyframes for the last one containing opacity, guard against relative values (+=/-=/*=), and add Number.isFinite check. - Fix falsy-zero doubling in drag commit: replace || fallback with Number.isFinite so a base GSAP position of 0 is correctly preserved. - Fix gesture recording sign inversion: remove pointerElementOffset subtraction from dx/dy formula and instead apply it once to basePosition so the element center tracks the pointer. - Fix TypeScript build errors in gsapSoftReload.ts (6 double-casts). - Strip all diagnostic logs from production code.
5 participants
Summary
Fixes 25 bugs across the keyframe editing, drag commit, animation deletion, gesture recording, and property panel systems. Spanning two sessions of investigation and fixes with a comprehensive audit between them.
Session 1: Core keyframe infrastructure (9 fixes)
Gesture recording
Design panel
5. Subscribe to liveTime for real-time value refresh during playback
6. Use update-keyframe instead of add-keyframe when editing existing keyframe
7. Prefer single-element tween over group selector when multiple target same element
Path offset system
8. reapplyPathOffsets passes updateBase:false to prevent corrupting translate base
9. gsapAnimatesProperty handles array-form keyframes
Session 1: Server-side safety net (4 fixes)
stripStudioEditsFromTargetremoves staledata-hf-studio-path-offsetand CSS var offsets when an animation is deletedbakeVisibilityOnDeletepreserves element visibility when GSAP animation is removed and CSSopacity:0would take overeditScaleX/Yinstead of hardcoded 1:1Session 1: File refactor + lint (3 fixes)
Session 2: Regression fixes from audit (9 fixes)
stripStudioEditsflag on the delete mutation type so strip/bake only fires on user-initiated deletes, not internal delete-then-recreate drags inextendTweenAndAddKeyframebakeVisibilityOnDeleteto theremove-all-keyframeshandler so CSSopacity:0elements stay visible after collapsing to a flat tween__timelinesto exclude properties animated by other tweens on the same element+=/-=/*=values and addsNumber.isFinitecheck|| gsapPos.xwithNumber.isFinitecheck so a GSAP base position of 0 is preserved correctlypointerElementOffsetsubtraction from dx/dy formula, applied once tobasePositionso element center tracks the pointeras Record<string, unknown>→as unknown as Record<string, unknown>casts ingsapSoftReload.tsAll diagnostic logs stripped from production code.
Files changed
packages/core/src/studio-api/routes/files.tsextractGsapScriptBlockexposes document;stripStudioEditsFromTarget;bakeVisibilityOnDeletewith reverse-scan + relative value guard;stripStudioEditsflag gating; bake on remove-all-keyframespackages/studio/src/hooks/gsapRuntimeReaders.tsPOSITION_PROPSper-property rounding; VISUAL_BASELINE cross-tween guard via__timelinestraversalpackages/studio/src/hooks/gsapDragCommit.tsNumber.isFinite); extend-tween keyframe remappingpackages/studio/src/hooks/useGsapScriptCommits.tsstripStudioEdits: trueon user-level deletepackages/studio/src/hooks/useGestureRecording.tspackages/studio/src/utils/gsapSoftReload.tspackages/studio/src/components/editor/manualOffsetDrag.tsTest plan
bunx tsc --noEmitpasses for studio and corebun test --filter manualOffsetDragpasses (11/11)