feat(studio): GSAP tween editing in Design panel#1102
Conversation
Fallow audit reportFound 90 findings. Duplication (54, showing 50)
Showing 50 of 54 findings. Run fallow locally or inspect the CI output for the full report. Health (36)
Generated by fallow. |
miguel-heygen
force-pushed
the
feat/gsap-design-panel
branch
2 times, most recently
from
31ebed3 to
a901232
Compare
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Read the 17-file diff with focus on env-flag gating, the new mutation pathway, parser-rewrite test coverage, and the selection-position fix. Substantial feature add, env-flag gated default-false, CI green on the fast lane (regression-shards still in flight at time of review). No blockers given the flag; a few items worth tightening before flipping it for production.
Verified
-
Env-flag gating works at two layers.
STUDIO_GSAP_PANEL_ENABLED(default false) is consumed inPropertyPanel.tsxas a conditional render gate ({STUDIO_GSAP_PANEL_ENABLED && ...}) AND inuseDomEditSession.tsas a projectId-nulling gate (STUDIO_GSAP_PANEL_ENABLED ? (projectId ?? null) : null). With the flag off,useGsapAnimationsForElementshort-circuits to empty (no fetches, no parses), no DOM is rendered, and the sevenhandle*callbacks are still wired through context but never invoked from a UI surface. Clean.- Caveat: this is a runtime gate (reads
import.meta.env), not a build-time constant. The 563-lineGsapAnimationSection.tsx+ 250-lineuseGsapScriptCommits.ts+ 61-lineuseGsapTweenCache.tsare still in every Studio bundle even when the flag is false. Fine for a bug-bash flag; if this stays default-false in prod for weeks, consider a build-time constant for tree-shaking.
- Caveat: this is a runtime gate (reads
-
Mutation pathway coordinates with the existing infra.
persistScriptEditdoes: implicitwriteIfChanged(newHtml === originalHtmlshort-circuit) →writeProjectFile→editHistory.recordEdit({ files: { [path]: { before, after } }, coalesceKey, kind: "manual" })→domEditSaveTimestampRef.current = Date.now()→ cache-version bump. So undo/redo is wired, slider-drag coalescing is namespaced (gsap:<animId>:<property>), and the mutation-probe debounce timestamp gets stamped. Per-propertysoftReload: truekeeps the preview from full-reloading on every slider tick (re-parse via cache version bump). Sensible. -
Selection position fix.
reapplyPositionEditsAfterSeeknow fires from both the pointer-click handler AND the timeline-element click inuseDomSelection, plus from the load-sync effect inuseDomEditSession. The newmanualEdits.test.tsblock covers strip-GSAP-translate, strip-while-preserving-scale, and multi-call no-drift — three real cases. Good. -
Parser rewrite (regex → Recast/Babel AST). The test deltas in
gsapParser.test.tsinvert the prior "known limitation" tests (negative numbers, unsupported-property filtering) into "now works" assertions — that's the right shape for documenting capability gains. Determinism contract preserved: the edit writes the modified script to disk, runtime parses the modified script normally in the iframe; "same script bytes → same timeline" invariant intact.
Concerns (non-blocking, default-false mitigates urgency)
-
Parallel mutation pathway, not consolidated with hf#1091's
parseMutationBody/writeIfChanged. Zero references to either function across this PR.persistScriptEditre-implements the read → diff-check → write → record-edit → invalidate-cache → bump-probe-timestamp dance manually. It works today, but it's now a second write path running in parallel to the consolidated one. The drift risk is the obvious one: if hf#1091's path later adds validation / locking / normalization / a different debounce window, the GSAP path won't pick it up. Worth either consolidating (route GSAP edits through the same function) or adding a short comment inuseGsapScriptCommitsexplaining why script-content edits take a separate route (e.g., they can't fit the structured-mutation contract — if that's the case). -
Test coverage gap on the editor logic itself. PR claims "63 tests pass" but the diff's new test coverage is: ~3 assertion-updates in
gsapParser.test.tsfor the AST-parser capability gains, plus the 60-lineapplyStudioPathOffsetblock inmanualEdits.test.ts. That leaves:useGsapScriptCommits.ts(250 lines, the entire mutation pathway) — no unit tests.useGsapTweenCache.ts(61 lines, the fetch+version cache) — no unit tests.GsapAnimationSection.tsx(563 lines, the UI logic) — no unit tests.
The leverage-y add is a unit test on
persistScriptEdit(with stubwriteProjectFile+editHistory) verifying the round-trip on a representative script preserves non-edited tweens — that catches a whole class of "AST modification corrupted a sibling tween" regressions cheaply. Plus a round-trip / property-style test for the parser itself (parse → serialize → parse → assert equality) to catch quiet semantic losses the existing fixture tests don't see. The flag means you have time to add these before flipping. -
PR body doesn't mention the env-flag. Anyone reading the PR can't tell it's gated default-false. Recommend a one-line note: "Behind
VITE_STUDIO_ENABLE_GSAP_PANEL(default false) for tomorrow's bug bash." This matters for merge-safety context for future readers. -
extractGsapScriptContentfirst-match heuristic. Returns the first<script>containing__timelinesOR (timelineAND.to(). For the standard hyperframes single-timeline-per-composition shape, fine. For compositions with multiple GSAP-looking scripts (rare but possible — e.g., a<script>mentioning "timeline" in a string literal), only the first wins. Note for future hardening if author-supplied compositions break the heuristic. -
addGsapAnimationsilent no-op when noid/selector. Ifselection.idis missing andselection.selectoris empty, the functionreturns with no user-visible feedback. Cheap fix: disable the "Add Animation" button or surface a toast when the selection can't be addressed by a selector. -
PROPERTY_DEFAULTSarbitrary numbers.width: 100,height: 100are arbitrary — when a user adds awidthtween to an element with natural width 400, it snaps to 100. Minor UX; reading the element's current rendered value would be friendlier.
Adjacency checks
- hf#1091 (mutation consolidation): see concern 1 — the new path runs parallel to it, not through it.
- hf#1076 (playhead clamp): not touched here; orthogonal.
- hf#1085 (selection sync): this PR extends
reapplyPositionEditsAfterSeekcalls. Composes cleanly with the manualEdits tests.
Verdict
Substantial well-structured feature with the right gating posture (default-false, two-layer runtime gate verified). Determinism preserved by design. The mutation-pathway-duplication and editor-logic test gap are the two highest-leverage items to address before flipping the flag for production — neither blocks merging behind the flag for tomorrow's bug bash. James/Miguel to merge.
— Rames Jusso (hyperframes)
miguel-heygen
force-pushed
the
feat/gsap-design-panel
branch
from
a901232 to
ad1dcfe
Compare
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Reviewed independently of Rames; agreeing with the overall framing (default-false flag mitigates merge urgency). Surfacing findings not already covered, sorted by what to fix before flipping the flag for the bug bash — bug-bashers will hit the first three on the very first edit.
Blockers before flipping the flag
1. Every pointer-move / keystroke during property edit writes the file. No debounce. useGsapScriptCommits.persistScriptEdit is fire-and-forget (void persistScriptEdit(...)) with no debouncing. The property field is wired with scrub liveCommit in AnimationCard, so a single scrub-drag from x=0 to x=200 will fire 200 persistScriptEdit calls — each one does fetch(...) → writeProjectFile → editHistory.recordEdit → bumpCache → fetch(...). Two consequences: (a) the file API gets hammered, and (b) parallel writes can land out of order — the second-to-last write may end up as the final on-disk state. editHistory's coalesceKey merges history entries but does nothing to backpressure the I/O. Either debounce inside persistScriptEdit, or hold the in-flight edit in memory and serialize writes (one write outstanding at a time, with a "pending newer" slot).
2. The "live preview" callback wiring is dead code. AnimationCard.scrubProperty calls onLivePreview?.() which is never passed. PropertyPanel instantiates <GsapAnimationSection> without onLivePreview / onLivePreviewEnd props. Combined with softReload: true on updateGsapProperty, the iframe never reloads on property edits — so the preview never reflects changes until the user touches a meta field (duration/ease/position) that does trigger reloadPreview. The PR's own test plan says "Edit property value → preview reflects change" — that path is currently broken. Either wire the live-preview callback through (mutating the GSAP tween's vars live in the iframe) or drop softReload: true so the iframe reloads on property commit.
3. Parse-fail + Add Animation destroys the user's script. parseGsapScript has try { ... } catch { return { animations: [], timelineVar: "tl", preamble: "", postamble: "" }; }. If parsing fails (any syntax error in the user's script), addAnimationToScript then calls serializeWithContext(parsed, [newAnim]), which writes ${preamble}\n${lines}\n${postamble} with preamble: "" — i.e., a script with no gsap.timeline() declaration and no window.__timelines["..."] = tl registration. The composition stops rendering entirely.
This is a regression from the old code path: old addAnimationToScript called serializeGsapAnimations with a hardcoded const tl = gsap.timeline({ paused: true }); preamble, so parse-fail-then-add at least produced a valid (if minimal) script. The new serializeWithContext trusts parsed.preamble as authoritative.
Simplest fix: in each mutation entry point, if parsed.preamble === "" and parsed.animations.length === 0, return the original script unchanged. Even better, surface "couldn't parse script — edit it manually" in the panel.
Important (pre-existing footguns, newly reachable from UI)
4. serializeObject does not escape quotes in string values. \${key}: "${value}"`— ifvaluecontains", the output is malformed JS. With the AST parser now accepting any string (test was renamed to "extracts all GSAP properties including non-standard ones", e.g., backgroundColor: "red"), and the panel falling back to string via parseNumericOrStringwhen input isn't numeric, a user typingred"; alert(1); //into any property field produces an executing JS snippet in the script. Same code existed pre-PR but was largely unreachable (regex parser only matched simple props). Either escape"and\on serialize, orJSON.stringify` the string value.
5. Non-literal property values are silently dropped on round-trip. extractLiteralValue returns undefined for any node that isn't a literal (no identifier reads, no template strings, no function calls, no arithmetic). objectExpressionToRecord stores those as undefined, and tweenCallToAnimation filters out non-number|string values, so a user's tl.to("#el", { x: someVar, opacity: 1 }, 0) round-trips to tl.to("#el", { opacity: 1 }, 0) — someVar is gone. The old regex parser at least matched identifier patterns and stored them as the string "someVar", which survived round-trip incorrectly-as-a-string but at least survived. The new parser silently loses the user's reference.
For a generic GSAP composition feature this can be lived with; for a "Design panel that edits user-authored scripts," it's a real footgun. Either: (a) refuse to mutate any tween that contains non-literal vars (mark the card read-only with a "non-literal — edit in source" tag), or (b) preserve the raw source range and splice mutations into it. Option (a) is one if-check; ship that for the bug bash.
Nits
-
html.replace(scriptContent, modified)—String.prototype.replace(str, str)interprets$&,$', etc. in the replacement. Extremely unlikely with GSAP property values but cheap to harden (.replace(scriptContent, () => modified)sidesteps it). -
Animation IDs are positional (
anim-${index+1}) and reassigned every parse. Stable for stable order, but if an add/remove lands between a scrub-start and a scrub-commit, thecoalesceKeygsap:anim-2:opacitycan address a different tween than the one the user dragged. Lower-probability than #1 but the same root concurrency hole; fix #1 and this gets mostly mitigated. -
fallow auditred on this PR. Several CRAP scores >100 (tweenCallToAnimation106, plus the existingapplyDomSelection420 from the new branches), plus duplication betweenserializeGsapAnimationsandserializeWithContext— those two functions diverged for no real reason, easy to consolidate behind a single internal helper. Not load-bearing for correctness; flagging since it's the only red on the rollup.
— Vai
miguel-heygen
force-pushed
the
feat/gsap-design-panel
branch
3 times, most recently
from
dbce773 to
899d0ca
Compare
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Architectural risk follow-up
A few risks from the broader structural analysis that didn't make it into the per-file comments:
[important] GSAP position labels silently coerce to 0
tweenCallToAnimation in gsapParser.ts:
const position = typeof posVal === "number" ? posVal : 0;GSAP positions are routinely strings: "+=1", "-=0.5", ">", "<", label references. All of these become 0 on round-trip — silent timeline corruption for any composition using relative or label positioning. Fix: widen position type to number | string and pass strings through unchanged.
[important] Two serializers have diverged
serializeGsapAnimations (exported, hardcodes a valid preamble) and serializeWithContext (private, trusts parsed.preamble as-is) differ by exactly one line — the preamble source. This duplication is what enables the parse-fail empty-state bug: if there were a single serializer that always emitted a valid preamble, a failed parse couldn't produce an unrenderable script. Consolidate into one function parameterized on preamble strategy.
[nit] AST in the middle, regex at the edges
Preamble extraction uses a regex against the raw script string; postamble uses script.lastIndexOf(...). Both can misfire if a string literal elsewhere in the script contains the matched substring. The parser already has full AST access — preamble and postamble should be derived from AST node byte ranges (everything before the first tween call node, everything after the last) rather than re-parsing the text.
[nit] addGsapAnimation silent no-op without a selector
If selection.id is missing and selection.selector is empty, the function returns without adding an animation and with no user-visible feedback — no toast, no disabled state on the button. Should surface an error or disable "Add Animation" when no selector is available.
[nit] Zero unit tests on the mutation layer
useGsapScriptCommits (~250 lines) and useGsapTweenCache (~61 lines) have no tests. Highest-value additions: (a) a round-trip test (parse → serialize → parse → assert IR equality) covering all four methods, negative numbers, string positions, and non-literal values; (b) a test verifying that editing tween A doesn't corrupt tween B's properties.
— Vai
miguel-heygen
force-pushed
the
feat/gsap-design-panel
branch
4 times, most recently
from
a5394fe to
d0c61f7
Compare
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Sub-Composition Edge Cases
Investigated how the GSAP parser handles sub-compositions (loaded via data-composition-src). Found two P0 destructive bugs:
P0 — window.__timelines registration destroyed on sub-comp edit
serializeGsapAnimations hardcodes const tl = gsap.timeline({ paused: true }); and has no postamble parameter. The window.__timelines["comp-id"] = tl; registration line lives in the postamble. After any edit to a sub-composition's GSAP script, the registration is gone and the composition breaks at runtime.
Fix: Serialize must wrap output with parsed.preamble and parsed.postamble instead of hardcoding the timeline declaration.
P0 — Second timeline deleted on any edit
findTimelineVar only matches the first gsap.timeline() declaration. Tweens on a second timeline variable (e.g., bgTl.to(...)) are completely ignored by the parser. On save, only the first timeline's tweens are re-serialized — the second timeline is silently deleted.
Fix: Either restrict the UI to single-timeline files (show a warning, disable editing) or extend the parser to handle multiple named timeline variables.
P1 — Timeline without const/let/var declaration
If a sub-comp assigns the timeline directly without a local variable (window.__timelines["id"] = gsap.timeline(...) without a named local var), the regex fails and timelineVar defaults to "tl". No tweens are matched — the panel shows zero animations silently.
Other edge cases (P2, correctly handled)
| Edge Case | Severity | Status |
|---|---|---|
<template> hides scripts from DOMParser |
P2 | Safe today — scripts are siblings, not children of <template> |
| Multiple scripts in one file | P2 | First-match heuristic picks wrong script |
| Nested sub-compositions | P2 | Not traversed — zero animations shown |
String positions ("+=1", "<", labels) coerced to 0 |
P2 | Timing relationships destroyed on round-trip |
| Element ID collisions across compositions | OK | Correctly handled via sourceFile scoping |
Items #4 and #5 in the pre-flag-on list. The two P0s must be fixed before the bug bash if sub-compositions will be tested.
miguel-heygen
force-pushed
the
feat/gsap-design-panel
branch
2 times, most recently
from
79539fa to
260014e
Compare
Add an ANIMATION section to the Design panel that lets users inspect and edit GSAP tween properties directly — no code editing required. - Recast AST parser replaces regex for GSAP script manipulation - Collapsible per-tween cards with video-editor language - Editable properties with add/remove, easing curve preview - DOMParser for HTML script extraction (no regex) - Selection position fix: force reapply before rect reads - GSAP translate doubling fix: Proxy on window.__timelines auto-wraps seek/play/pause the instant a timeline registers, closing the gap between GSAP init and seek wrapper installation - PropertyPanel visual offset: X/Y show actual position - Input validation: duration/position reject negatives - Feature flag: VITE_STUDIO_ENABLE_GSAP_PANEL (default false)
miguel-heygen
force-pushed
the
feat/gsap-design-panel
branch
2 times, most recently
from
2941dd9 to
da4af87
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
miguel-heygen
force-pushed
the
feat/gsap-design-panel
branch
from
da4af87 to
d8ce2e4
Compare
…eedback Root cause fix for element position doubling during drag: - sourceMutation.ts unconditionally prepended data- to attribute names that already had the prefix, producing data-data-hf-studio-* in the persisted HTML. reapplyPathOffsets could not find those elements, so GSAP's baked translate was never stripped. - Guard against double prefix; migrate legacy attributes on the fly. - Revert gsapTranslate compensation in drag — with reapply working, the raw CSS var offset is the correct initial value. Parser and serializer hardening (PR review feedback): - Scope resolution via AST: resolve const/let/var declarations so variable references in tween properties are editable. - String positions preserved instead of coerced to 0. - Consolidated two diverged serializers into one with preamble/postamble. - Parse-fail safety: mutation functions return original script unchanged. - Quote escaping and non-identifier key quoting. - html.replace uses function replacement to avoid $& interpretation. Studio UX improvements: - Debounced property writes (150ms). - Preview reloads after property edits. - Auto-generate unique element ID when adding animations. - New animation starts at current playhead time. - Interactive draggable bezier handles on speed curve for custom eases. - String positions display correctly throughout. - Dead code cleanup (fallow). Tests: 113 passing across 4 test files.
…slate stripGsapTranslateFromTransform was zeroing all of m41/m42 in the transform matrix, which destroyed legitimate GSAP animation values (e.g. a to() tween animating y: -20). The fix subtracts only the known studio CSS var offset from the matrix, preserving the animation contribution. This means tweens that animate x/y now render correctly alongside manual position offsets.
useGsapTweenCache now fetches and parses the script once per file+version, then filters per element via useMemo. Switching between elements in the same composition is now instant — no re-fetch. Previously every element selection triggered a separate API fetch + parse cycle, causing a visible loading delay before animation cards appeared.
miguel-heygen
force-pushed
the
feat/gsap-design-panel
branch
2 times, most recently
from
563f669 to
502fab7
Compare
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Re-reviewed against head 502fab7a (confirmed pre-flight; not stale). Substantial progress on the punch list — verified each item against the diff. Two items still warrant attention before flipping the flag, plus the red CI to clean up.
Verified resolved
| Finding | Status | Verification |
|---|---|---|
| Vai #1 — debounce on scrub writes | ✅ | DEBOUNCE_MS = 150 in useGsapScriptCommits.ts; pendingPropertyEditRef + clearTimeout/setTimeout pattern; rapid scrubs collapse to one write at the trailing edge. |
Vai #2 — onLivePreview dead, preview never updates |
✅ (different mechanism) | Resolved by removing softReload: true from the property-edit path. Every debounced edit now triggers reloadPreview(). During a scrub the 150ms debounce groups → one reload at scrub-end. Less smooth than a real in-iframe live preview, but functionally correct. |
| Vai #3 — parse-fail destroys preamble | ✅ (outer guard) | isParseFailure(parsed) check at the top of updateAnimationInScript / removeAnimationFromScript; addAnimationToScript returns { script, id: "" }. The catch block still returns { preamble: "" } internally but it's now gated behind the outer guard so it can never reach the serializer. Worth a comment in the catch noting "callers must check isParseFailure before using preamble/postamble" to keep the contract visible for future contributors. |
Magi P0 #1 — serializeGsapAnimations drops postamble |
✅ | Consolidated. serializeGsapAnimations now takes { preamble?, postamble? } in options; all mutation functions pass parsed.preamble + parsed.postamble through. Two-serializer divergence closed. |
Vai important — position string coercion to 0 |
✅ | `position: number |
| Vance/Vai discussion — references as values silently dropped | ✅ (literal-const case) | New collectScopeBindings visits VariableDeclarators and builds a Map<name, value>; extractLiteralValue(node, scope) consults it. const x = 100; tl.to("#el", { x: x }) resolves to { x: 100 }. Caveat: round-trip writes back 100, not x — the variable reference is destroyed on edit (Vai's earlier "option 2 with literal write-back" trade-off, which Miguel accepted). Non-literal expressions (width / 2, getX(), etc.) are still unresolved. |
Rames #5 — addGsapAnimation silent no-op without selector |
✅ | Auto-generates a unique element ID (tag, tag-2, …) and sets it on the element when none present. |
| Rames #2 — test coverage gap | ✅ (substantially) | 63 → 113 tests across 4 files. gsapParser.test.ts grew +141, manualEdits.test.ts +101, sourceMutation.test.ts +38 (new), manualOffsetDrag.test.ts +35/-22. Solid parser-level + source-mutation-level coverage. useGsapScriptCommits.ts hook itself still doesn't have direct unit tests, but the parser tests cover the underlying transformations. |
Root cause — data-data-hf-studio-* prefix doubling |
✅ | sourceMutation.ts now guards with op.property.startsWith("data-") ? op.property : \data-${op.property}`. Tests in sourceMutation.test.ts` pin all five studio data-attrs against double-prefix. This is the real drag-drift root cause and the fix is solid. |
Still needs attention
-
Magi P0 #2 — second timeline silently deleted on edit (status: layout-dependent, needs author confirmation).
findTimelineVarstill only matches the firstgsap.timeline()VariableDeclarator.findAllTweenCallsonly finds calls on that timelineVar. So a secondconst bgTl = gsap.timeline(...)declaration is invisible to the IR. Whether it survives the round-trip depends entirely on where it sits in the script relative to the first timeline's last call:- If the second timeline's declaration + calls all sit after the first timeline's last call → postamble preservation (regex-based) captures them verbatim → survives.
- If interleaved or before → not captured → silently deleted on serialize.
Recommend: either restrict the editor UI to compositions with a single
gsap.timeline()(detect at parse time, disable the panel for multi-timeline files with a banner), or add an explicit test that confirms the postamble case Magi flagged actually round-trips. A quick fixture with the canonical multi-timeline layout would settle it. -
Magi P1 —
window.__timelines["id"] = gsap.timeline(...)inline assignment. Still unhandled.findTimelineVaronly visitsVariableDeclarators, so this pattern returnsnull→ fallback"tl"→findAllTweenCallsfinds nothing → panel shows zero animations for the composition. Non-destructive (no edit can be triggered) but the panel is functionally broken for that pattern. Either supportAssignmentExpressiontowindow.__timelines[...]or disable the panel with a banner.
Other observations
-
Debounce uses a single
pendingPropertyEditRef. If the user drags property A's slider, then within the 150ms window starts dragging property B, A's pending edit is overwritten and never persisted. Edge case (humans don't usually switch sliders that fast), but worth a per-(animationId, property)bucket if you see it surface during the bash. Quick fix:Map<string, PendingEdit>keyed by\${animId}:${prop}`` with one timer per key. -
PROPERTY_DEFAULTSarbitrary numbers (width: 100, height: 100) — when a user adds awidthproperty to a tween on an element with natural width 400, it snaps to 100. Cosmetic. Reading the element's current rendered value would be friendlier. -
PR body still doesn't mention the env-flag. The merged-commit message has the
VITE_STUDIO_ENABLE_GSAP_PANEL (default false)note, but the PR summary doesn't. Future readers can't tell the feature is gated. One-line add to the summary. -
CI is RED — Preflight (lint + format), Format, Fallow audit, and downstream
regression/preview-regression/player-perfall failed on this push (the regression jobs were cascade-skipped after preflight). Likely a missedoxfmt --no-cache/oxlint --no-cache --fixon the diff before pushing. The fallow failure may want a closer look since the prior commit message said "Dead code cleanup (fallow)" — possibly a new fallow finding from the +600 new lines. Must be green before merge regardless.
Verdict
Major progress — all three of Vai's blockers, Magi's P0 #1, both of Vai's important items, and the references-as-values concern Vance raised are addressed. The single big destructive-on-edit failure mode (parse-fail erasing the preamble) is closed via the outer-guard pattern. Test coverage substantially expanded.
The two remaining concerns are both about which composition shapes the editor supports, not whether it corrupts files for supported shapes:
- Magi P0 #2 (multi-timeline) — layout-dependent; either confirm safe-by-postamble or add UI gating + a test.
- Magi P1 (inline
window.__timelines[...] = gsap.timeline(...)assignment) — still unhandled.
Plus the red CI to clean up. With those three in place, this is in good shape for the bash.
— Rames Jusso (hyperframes)
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Re-review verdict: prior blockers addressed, but regression (required check) is red
Solid work hardening the parser/serializer. All three prior must-fix blockers are addressed and well-tested.
Prior blockers — all fixed
1. Scrub write debounce — fixed. useGsapScriptCommits.ts line 157-169: updateGsapProperty now stores the latest pending write in a ref and flushes after a 150ms debounce. Pointer-move scrubs coalesce to a single file write at the end. Path is scrub onPointerMove → onCommit → commitProperty → updateGsapProperty → debounced flushPendingPropertyEdit → persistScriptEdit.
2. onLivePreview / preview-not-updating — fixed (via the "drop softReload" alternative). persistScriptEdit defaults softReload: false and no caller in this hook overrides it, so every commit triggers reloadPreview(). The user-visible bug (no iframe update on property edit) is gone. See important note below about the dead plumbing.
3. Parse-fail preamble regression — fixed. gsapParser.ts line 404-406: new isParseFailure helper. All three mutation entry points (updateAnimationInScript L414, addAnimationToScript L429, removeAnimationFromScript L444) early-return the original script unchanged. Tests at gsapParser.test.ts:584-608 cover all three. Additionally, extractAndReplaceScript is no-op when modified === scriptContent, so even on the addAnimation path the file write is skipped.
Follow-up review items
- String position coercion to 0 — fixed.
position: number | stringend-to-end; serializer round-trips"+=1"/"<"/"-=0.5". Tested. - Two divergent serializers — fixed.
serializeWithContextremoved; singleserializeGsapAnimationstakespreamble/postambleas options. serializeObjectquote escaping — fixed. Now usesJSON.stringify(value)for strings. Tests cover quotes and backslashes (gsapParser.test.ts:610-659).- Non-literal property values silently dropped — NOT addressed. Still drops with no UI affordance (
tweenCallToAnimationL260-266). See "Important" below.
Blocker
-
regressionrequired check is RED. Both CI runs fail because Preflight (oxfmt --check) reports format issues in:packages/studio/src/components/editor/manualEditsDom.tspackages/studio/src/hooks/useGsapTweenCache.ts
Reproduced locally — the diff is trivial whitespace (e.g. multi-line signature wrapping in
useGsapTweenCache.ts).regressionis in the repo's required-status-checks ruleset, so this blocks merge regardless of the actual regression-shards job (which is SKIPPED). Runbun run formatand push.
Important (non-blocking, but should be tracked before flipping STUDIO_GSAP_PANEL_ENABLED)
addGsapAnimationauto-id not persisted to source.useGsapScriptCommits.ts:213mutates the iframe DOM withel.setAttribute("id", id), then the new tween targets#<auto-id>. The auto-id only exists in the live iframe — afterreloadPreview(), the iframe re-renders from the source HTML (which never received the id), so the new tween targets a non-existent element. Narrow trigger (only when bothselection.idandselection.selectorare absent), but a real correctness bug. Either persist the id via the existingonSetHtmlAttributepath or refuse to add a tween until the element has a stable id.- Non-literal property values silently destroyed on edit. If a tween has
opacity: someFn()(unresolvable), the property is dropped during parse (tweenCallToAnimationL263-266). Then if the user edits any other property on that tween, the serializer re-emits without the dropped property — silently losing user code. Should mark such tweens read-only in the card with a visible warning, or refuse to serialize back when any source property was unresolved.
Nits
- Dead
onLivePreviewplumbing inGsapAnimationSection.tsx. The optionalonLivePreview/onLivePreviewEndprops are still defined (L32-33), threaded throughAnimationCard(L267-268, L528-529, L546-547), and invoked inscrubProperty(L293) — butPropertyPanel.tsxL358-366 never passes them, so they're alwaysundefined. Now thatsoftReloadis gone, this plumbing has no purpose. Either delete it or wire a realgsap.set()on the live timeline. - No in-flight write serialization on
persistScriptEdit. Debounce coalesces tight bursts, but if a user scrubs continuously >150ms and the file write takes longer than the next flush, twopersistScriptEditcalls can race on the same file. Narrow race in practice; flag for a future write-queue or in-flight guard. updateGsapMetaaccepts onlyposition?: numberwhile the parser preserves string positions. UI-side:commitPosition(GsapAnimationSection.tsx:307-314) parses to number and rejects non-finite. So a"+=1"tween survives round-trip if untouched, but any edit to its position field coerces to number. Acceptable for now; document expectation if you keep this.
What I checked
Read both worktree and PR diff against main. Verified all 3 prior blockers in gsapParser.ts, useGsapScriptCommits.ts, GsapAnimationSection.tsx, PropertyPanel.tsx, propertyPanelPrimitives.tsx. Confirmed test coverage in gsapParser.test.ts and sourceMutation.test.ts. Reproduced format failure locally with oxfmt --check. Inspected ruleset required-status-checks. Feature is behind STUDIO_GSAP_PANEL_ENABLED (default false).
— Vai
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Re-reviewed 21af9dc9 (pre-flight confirmed). Substantial progress — most items resolved. Two have residual concerns worth surfacing before flipping the flag.
Fully resolved
- CI: Preflight (lint + format), Lint, Test: runtime contract, Studio: load smoke all ✅ on this push. The two oxfmt files Vai called out are clean. ✅
- Auto-id persistence to source (the in-memory-only bug Vai caught): now writes via
/api/projects/.../file-mutations/patch-element/...with[{ type: "html-attribute", property: "id", value: id }]. ✅ The source-state write now happens — but with two correctness concerns (see below).
Partially resolved — worth flagging
1. Auto-id source-write has a race + a targeting concern.
el.setAttribute("id", id);
selector = `#${id}`;
const targetPath = selection.sourceFile || activeCompPath || "index.html";
const pid = projectIdRef.current;
if (pid) {
void fetch(`/api/projects/.../file-mutations/patch-element/${targetPath}`, {
method: "POST",
body: JSON.stringify({
target: { selector: selection.selector || el.tagName.toLowerCase() },
operations: [{ type: "html-attribute", property: "id", value: id }],
}),
});
}
// ↓ immediately falls through to:
void persistScriptEdit(...); // awaits writeProjectFile + recordEdit, then reloadPreview()- Race: the id-patch fetch is
void fetch(...), not awaited.persistScriptEditis awaited and triggersreloadPreview()once its writeProjectFile + recordEdit complete. The id-patch may complete after reload → iframe rebuilds from source that still lacks the id → new tween's#idselector points to nothing. Two ways to fix:awaitthe id patch beforepersistScriptEdit, or sequence them in a single promise chain that gates the reload until both writes complete. - Targeting:
target: { selector: selection.selector || el.tagName.toLowerCase() }. Falls back to bare tag ("div","span", …) when no selector is available. For files with multiple elements of that tag, the patch hits the first match, which may not be the element the user clicked. The same situation that caused us to need an auto-id in the first place — an element with no addressable handle — means the patch can't reliably address it. Belt-and-braces: pick an element-stable handle (DOM position, a unique data-attr added at probe time) for the target.
2. Multi-timeline gate is a soft banner, not a hard gate — destructive path still latent for interleaved layouts.
The banner reads: "This file has multiple GSAP timelines. Only the first timeline is editable — edits won't affect tweens on other timelines." That's optimistic framing, and may not match what the serializer actually does for some script layouts.
Trace: parseGsapScript returns first-timeline tweens in IR; serializer writes preamble + regenerated lines + postamble. Postamble is extracted via lastIndexOf(\${timelineVar}.`)` — case-sensitive substring. Two script layouts to consider:
- Trailing layout (all of timeline A's calls, then timeline B's declaration + calls, then registrations):
lastIndexOf("tl.")hits the lasttl.to(...); postamble = everything after, which captures B's declaration + tweens + registrations. ✓ Safe — B survives verbatim. - Interleaved / B-declared-early layout (timeline A declaration, B declaration before A's first call, then A and B calls interleaved): the
const bgTl = ...line sits between preamble (which stops at A's declaration) and the first A-call. It's not in preamble, not in IR (parser only sees A), and not in postamble (which starts after A's last call). The line is silently dropped on serialize. The downstreamwindow.__timelines["bg"] = bgTl;registration then referencesundefined. Runtime error.
The banner doesn't prevent this — the user is told the other timelines are safe; the edit may corrupt them.
Recommend either:
- Hard-gate the panel when
multipleTimelines: true— render the banner but no animation cards, no add button. Safe for any layout. - Or verify with a fixture test that all production composition layouts are in the "trailing" shape (no interleaving, no early B-declaration). If a composition author can produce the interleaved layout, the soft banner doesn't close the destructive path.
Still open
3. Magi P1 — window.__timelines["id"] = gsap.timeline(...) (inline assignment). The new visitAssignmentExpression counts this case toward timelineCount, but doesn't extract the timeline variable from the MemberExpression left side (if (left?.type === "Identifier") timelineVar = left.name — window.__timelines["id"] is a MemberExpression, not an Identifier, so the guard fails). Effect: timelineVar falls back to "tl", findAllTweenCalls(ast, "tl") finds none, the panel shows zero animations with no banner. Non-destructive, but the broken-UX case is unchanged.
Quick win if a fix is in scope: when timelineVar can't be extracted but timelineCount > 0, surface a distinct banner — "This composition uses an unsupported timeline registration pattern. Animation editing isn't available." Closes the silent-zero-animations UX.
4. CI: Fallow audit still RED on this push. Lint/format/build/test/studio-smoke all went green, but Fallow audit failed at 2026-05-28T05:57:15Z. The +86-line commit may have tripped a new fallow threshold (complexity, unit-size, duplication). Worth a glance at the fallow output to confirm it's not flagging something substantive in the new code.
Verdict
Major progress — the destructive paths Vai and Magi flagged are mostly closed. The auto-id race + targeting and the multi-timeline soft-banner are the two paths that could still bite during the bash. The right call before flipping the flag for real users (post-bash) is:
awaitthe id patch beforepersistScriptEdit, and tighten the target selector to something element-stable.- Hard-gate the panel on
multipleTimelines: true, OR add a fixture test pinning the trailing-layout-only assumption. - Surface a separate banner for the unsupported-timeline-pattern case so users aren't confused by an empty panel.
- Resolve the Fallow audit.
For the bash itself, the soft-banner + race are probably acceptable risks (bash users are sophisticated, and the trigger conditions are narrow). For prod flip, all four want to be closed.
— Rames Jusso (hyperframes)
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
GSAP design panel is a substantial body of work — the AST-based parse with scope resolution is the right call over the previous regex parser, the selective transform stripping in manualEditsDom.ts is well-reasoned, and parse-fail safety + 113 tests on the happy paths give real coverage. Solid architecture overall.
Two correctness issues block me from approving, plus one concern on the existing multi-timeline UX. Detail below.
Blocker 1 — auto-id write race in addGsapAnimation
packages/studio/src/hooks/useGsapScriptCommits.ts (the new file). When the selected element has no id, the code auto-generates one and writes it to source via a fire-and-forget fetch, then immediately calls persistScriptEdit to write the new tween:
if (pid) {
void fetch(
`/api/projects/${pid}/file-mutations/patch-element/${targetPath}`,
{ method: "POST", ... body: { operations: [{ type: "html-attribute", property: "id", value: id }] } },
);
}
// ...
void persistScriptEdit(selection, (script) => addAnimationToScript(script, { targetSelector: selector, ... }), ...);persistScriptEdit is async and starts with await readSourceFile(targetPath) — that read fires immediately, in parallel with the patch-element POST. Two failure modes:
- Clobber.
readSourceFilewins the race against the server-side id patch →originalHtmlis the pre-id snapshot →extractAndReplaceScriptruns on it →writeProjectFile(targetPath, newHtml)lands after the id patch, overwriting the just-written id. The new tween references#hero-2, but the source has no element with that id. Selector breaks silently until the user nudges something else. - Reload before patch lands.
persistScriptEditfinishes (id-less source) and callsreloadPreview()before the patch-element fetch's server-side write completes. Iframe reloads against the id-less source.
There's also no error handling on the patch-element fetch — if it 4xx/5xx, the user sees a tween they can't edit and has no signal why.
Fix: await the patch-element fetch before calling persistScriptEdit, or (preferred) fold the id assignment into the same HTML transform that writes the tween so it's one atomic write to source. The atomic approach also gives you undo coherence — right now an undo only reverts the script change, not the id write.
Blocker 2 — multi-timeline data loss on edit (not just "non-editable")
packages/core/src/parsers/gsapParser.ts + useGsapScriptCommits.ts. The banner warns "edits won't affect tweens on other timelines" — but the actual behavior on an interleaved layout is worse: editing the first timeline deletes the second timeline from source.
Reconstruction path:
preamble: regex match^[\s\S]*?(?:const|let|var)\s+${timelineVar}\s*=\s*gsap\.timeline\s*\(...)— non-greedy to the first timeline declaration.postamble:script.slice(script.lastIndexOf("${timelineVar}.") + ...)— everything after the lasttl.*call.- Anything between those — i.e. a
const tl2 = gsap.timeline(); tl2.to(...)block sitting between twotl.to(...)calls — falls into a gap. It's not inpreamble, not inpostamble, not in the parsedanimations[]. It is then erased whenextractAndReplaceScriptdoeshtml.replace(scriptContent, modified).
Minimal repro:
const tl = gsap.timeline({ paused: true });
tl.to('#a', { x: 100 });
const tl2 = gsap.timeline({ paused: true });
tl2.to('#b', { y: 50 });
tl.to('#c', { x: 200 });Open this in studio, edit any property on the #a or #c tween → tl2 and its to('#b') are gone from source. (Same <script> block; multi-script layouts are unaffected because extractGsapScriptContent only touches one block.)
This needs to be a hard gate, not a soft banner. Either:
- Disable the Add/Edit/Delete actions when
multipleTimelines === trueand replace the banner with a clear "this file is read-only in the editor until..." message, or - Preserve unknown content: rather than reconstructing from preamble/postamble, splice each tween edit in place in the source AST and serialize back. (Bigger change, but actually correct.)
A test verifying second-timeline content round-trips through an edit would have caught this — the parser-level multipleTimelines flag is set, but no test asserts source preservation after a serialize.
Multi-timeline detection is also under-counting
findTimelineVar only walks VariableDeclarator and one assignment shape. It misses:
let tl; tl = gsap.timeline()(declarator without init + later assignment lands; declaration path won't seegsap.timeline())window.tl = gsap.timeline()(MemberExpression left side; the code handlesIdentifieronly)- Timelines created inside arrow functions / IIFEs / conditionals on the top level (still walks via
recast.types.visitso probably fine, but worth a test)
Less critical, but if Blocker 2 stays a soft banner, these are paths where the banner won't even fire and the user has no signal.
Important (not blockers)
- Fallow audit is failing with 90 findings — 36 health, 54 duplication. New code adds critical CRAP scores on
addGsapAnimation(132),tweenCallToAnimation(116),resolveNode(315), andEaseCurveSection's arrow (116/349 elsewhere in gsap.ts). The twofallow-ignore-next-lineannotations cover onlyuseGsapScriptCommitsanduseDomEditSession. Whether this is a required gate or not, this is a lot of complexity debt landing in one shot in code that's going to need iteration. At minimum, breakaddGsapAnimation(selector resolution / id-patch / animation insert are three distinct steps), and splittweenCallToAnimationper call shape. extractAndReplaceScriptuseshtml.replace(scriptContent, ...)— string replace on free-form HTML. If the script content happens to contain a substring that also appears elsewhere (e.g., a<style>block with identical text — unlikely but not impossible), you replace the wrong region. Safer to splice by offset using the sameDOMParseralready used byextractGsapScriptContent, or anchor the replacement to the surrounding<script>tags.debounceTimerRefflush on unmount.updateGsapPropertysets asetTimeout, but if the component unmounts (panel collapse, selection change) before the timer fires, the pending edit is lost and the user's last drag-released value never lands. Need a cleanup effect that callsflushPendingPropertyEditon unmount.addAnimationToScriptusesid: \anim-${Date.now()}` — fine in practice, but two rapidaddGsapAnimationcalls within the same ms would collide. Counter or UUID is cheap.isParseFailurereturns true for any script with zero animations + no preamble. Empty<script>blocks (legitimate, pre-tween authoring) would silently no-op every edit. AparseError: trueflag fromparseGsapScriptwould be more honest than inferring failure from emptiness.
Nits
findAllTweenCallscollects every${timelineVar}.method(...)regardless of whether the call is inside anif/conditional/loop. That's probably fine for studio-authored content, but worth noting as a known limitation.- Banner copy says "edits won't affect tweens on other timelines" — actually accurate today is "edits to first timeline DELETE other timelines." Until Blocker 2 is fixed, the copy understates the risk.
extractGsapScriptContentheuristic(text.includes("timeline") && text.includes(".to("))will false-match any script that mentions those substrings (e.g., a comment). Worth a tighter signal.
CI
CLI smoke (required): green.Preflight (lint + format): green across all detect-changes shards.Build,Test,Typecheck,Studio: load smoke, perf shards, windows tests: all green.Fallow audit: red (see above).regression-shards(1-8): still pending — re-check before merge.
REQUEST CHANGES on the two correctness blockers. Once the auto-id race is fixed (atomic transform) and the multi-timeline path is a hard gate (or made truly preserving), happy to take another look.
— Vai
5 participants
Closes #1092
Summary
GSAP tween editing in the Design panel — select any element with GSAP animations and edit properties, easing, timing, and positions directly. Behind
VITE_STUDIO_ENABLE_GSAP_PANEL(default false).Architecture
flowchart TB subgraph "Parse Pipeline" A[HTML Source File] -->|extractGsapScriptContent| B["<script> block"] B -->|Recast + Babel AST| C[AST] C -->|collectScopeBindings| D["Scope Map<br/>const FADE = 0.8<br/>const OFFSET = -60"] C -->|findTimelineVar| E["Timeline var: tl"] C -->|findAllTweenCalls| F["TweenCallInfo[]"] F -->|tweenCallToAnimation + scope| G["GsapAnimation[]<br/>(resolved values)"] end subgraph "Scope Resolution" D -.->|resolve Identifiers| F H["const X = 50"] -.-> I["tl.to('#el', {y: X})"] I -.->|resolves to| J["y: 50 (editable)"] K["const A = 10<br/>const B = A * 2"] -.-> L["B resolves to 20"] end subgraph "Edit Pipeline" G --> M[Design Panel UI] M -->|user edits property| N[updateAnimationInScript] M -->|user changes ease| N M -->|user adds animation| O[addAnimationToScript] M -->|user deletes animation| P[removeAnimationFromScript] N & O & P -->|serializeGsapAnimations| Q[Modified script string] Q -->|extractAndReplaceScript| R[Modified HTML] R -->|debounced 150ms| S[writeProjectFile] S --> T[Iframe reload] end subgraph "Position Safety" U["GSAP bakes CSS translate<br/>into transform matrix"] -->|reapplyPathOffsets| V["Strip only baked offset<br/>Preserve animation values"] V --> W["Element position =<br/>CSS var offset + animation delta"] X["data-hf-studio-path-offset<br/>(correct attribute name)"] -->|queryStudioElements| V Y["Legacy data-data-hf-*<br/>(double prefix)"] -->|auto-migrate| X endKey design decisions
AST scope resolution over runtime queries: variables like
const FADE = 0.8are resolved statically from the AST, making all properties editable without iframe communication. Handles expressions (A * 2), negatives, chained references, and template literals.Selective transform stripping:
stripGsapTranslateFromTransformsubtracts only the known CSS var offset from the GSAP matrix, preserving animation contributions (e.g.,y: -20from ato()tween). Previous approach zeroed all of m41/m42, which destroyed legitimate animation values.String position preservation: GSAP positions like
"+=1","<","footerIn"are preserved as strings through parse → edit → serialize, not coerced to0. The UI displays them as-is in the card header and "Starts at" field.Parse-fail safety: if the AST parser throws on malformed JS, all mutation functions return the original script unchanged — no data loss.
Debounced writes: property scrub-drags fire one write per 150ms instead of per-pointer-move, preventing file API races and excessive history entries.
Edge cases handled
const X = 50; {y: X})const HALF = BASE / 2)"+=1","<", labels)"--glow")JSON.stringifyfor safe quotinghtml.replacewith$&in values() => modifieddata-prefix in persisted HTML__timelinesregistrationWhat changed
sourceMutation.tsdoubledata-prefix bug, selective GSAP transform strippingserializeGsapAnimationswith preamble/postambleTest plan
bun run build— all packages passbun test— 113 tests pass (parser + sourceMutation + manual edits + drag)--glow) serialize without syntax errors