fix(core): warn on GSAP boundary exits without hard kill#474
fix(core): warn on GSAP boundary exits without hard kill#474miguel-heygen merged 2 commits intomainfrom
Conversation
jrusso1020
left a comment
There was a problem hiding this comment.
Verdict: approve — real bug, reproduced locally, fix is correctly scoped
Local verification
Built three fixtures from scratch against da4ce98d and ran the actual CLI (bun run --filter @hyperframes/cli dev lint <fixture> --json):
-
Buggy — fade-out ending exactly at the
3.00sclip start boundary, no hard kill:{ "code": "gsap_exit_missing_hard_kill", "severity": "warning", "selector": "#headline", "message": "GSAP exit on \"#headline\" ends at the 3.00s clip start boundary without a matching tl.set hard kill. Non-linear seeking can land after the fade and leave stale visibility state.", "fixHint": "Add `tl.set(\"#headline\", { opacity: 0 }, 3.00)` after the exit tween, or use autoAlpha/visibility if that is the authored hidden state." } -
Fixed — same fade-out but with
tl.set("#headline", { opacity: 0, visibility: "hidden" }, 3)appended:warningCount: 0, rule correctly silences. -
Edge — buggy fixture WITHOUT
class="clip"on the scene containers:gsap_exit_missing_hard_killdoesn't fire (becausecollectClipStartBoundariesrequiresclass="clip"), BUT the existingtimed_element_missing_clip_classwarning fires on both scene divs — so authors are already funneled toward the canonicalclass="clip"shape by a separate rule. The two rules compose correctly; no coverage gap in practice.
Also ran:
bun run --filter @hyperframes/core test src/lint/rules/gsap.test.ts— 33/33 pass, including the two new tests.bun run --filter @hyperframes/core test— 527/527 pass.bun run --filter @hyperframes/core typecheck— clean.
Staff review — architecture + correctness
The PR does the minimum plumbing to extend the existing GsapWindow parser with a new piece of metadata (propertyValues) and then composes four small pure predicates (isHiddenGsapState, isSceneBoundaryExit, isHardKillSet, findMatchingSceneBoundary) into a single new rule. No new abstractions, no reshuffling of gsap.ts's structure. The rule shares the same gsapWindows pass as overlapping_gsap_tweens, so the detection cost is negligible.
The semantics I verified line up with the real failure mode described in #473:
- Why the tween-position check is right:
win.end = position + effectiveDurationforto/fromTo, so atl.to("#headline", { opacity: 0, duration: 0.3 }, 2.7)hasend = 3.0.SCENE_BOUNDARY_EPSILON_SECONDS = 0.05means a boundary at exactly3.0smatches, and a boundary at2.97swould also match (within 1 frame at 30fps). That's the correct tolerance for authored times — tight enough not to catch mid-clip fades that intentionally end before a boundary, loose enough to absorb rounding noise. - Why excluding
from/setinisSceneBoundaryExitis right:frominterpolates FROM a state to the current one, so it ends at the target's original state rather than at the author's written hidden state — no determinism concern at the boundary.sethaseffectiveDuration = 0, whichwin.end <= win.positionalready rejects, but the explicit method check makes intent obvious. - Hard-kill same-selector match:
isHardKillSetchecks bothtargetSelectormatch ANDisHiddenGsapStateon thetl.set's propertyValues. That means a fade-out viaopacity: 0paired with a hard-kill viavisibility: "hidden"correctly satisfies the rule — both are "hidden" states by the runtime's logic, so either side of the pair works. Right tolerance.
Non-blocking observations
These are all consistent with the PR's stated "heuristic-based" design goal — none are worth holding the merge on, but worth putting on the cleanup list for the next pass:
-
Sub-composition scope vs. clip boundaries.
clipStartBoundariesis collected once over ALL.cliptags in the document, then matched against tweens from every registered timeline. A tween on a sub-composition whose fade-out happens to end at a root-level clip'sdata-start(same numeric coincidence) could false-positive without an actual authoring bug at that scope. Rare in practice because the same-selector hard-kill check narrows it further, but scoping boundaries per-composition would tighten this. -
Expression-valued props bypass detection.
parseLooseObjectLiteralhandles literal scalars ("str"/'str'/ numbers /true/false) but can't evaluateopacity: hidden ? 0 : 1oropacity: gsap.utils.random(0, 0.1). Those will silently skip the rule. Fine for heuristic-by-design; flag if you ever want to harden. -
Fix hint is always
{ opacity: 0 }. If the author's exit tween usedautoAlpha: 0orvisibility: "hidden", the suggested kill in{ opacity: 0 }loses their intent. The trailing "or use autoAlpha/visibility if that is the authored hidden state" in the hint covers it, but a targeted hint that echoes the detected property would be a small UX win later. -
The
regex-based method pattern inextractGsapWindows. Unchanged by this PR, but worth a mental note: the method-match regex drives bothparseGsapScript's animation index and the property-value extraction. The existing comment on line 63 (if (!/^\s*["']/.test(args)) continue;) warns that non-selector first-args drift the index — the same caveat applies topropertyValuesbeing attributed to the wrong animation if the regex ever misaligns. No action needed now; just context for anyone touching the parser.
CI state
All green on da4ce98d:
- Format / Typecheck / Lint / Test / Build / Test: runtime contract / Smoke: global install / Tests on windows-latest / Render on windows-latest / CodeQL / player-perf / Perf (fps/scrub/drift/parity/load) / regression-shards (all shards)
Ship it.
Review by Rames Jusso
Merge activity
|
Fixes #473.
Problem
The HyperFrames skill now tells agents to add deterministic
tl.set()hard-kills after elements fade out at beat / scene boundaries, but the linter did not enforce that rule outside the narrow caption-specific check.That made the rule easy for sub-agents to ignore: an element could fade to
opacity: 0exactly as the next clip starts, with no explicit hidden-state set at the boundary. During non-linear seeking or frame capture, that leaves the final visibility state dependent on tween interpolation instead of an authored deterministic kill.What this fixes
This PR adds a generalized GSAP lint warning for scene-boundary exits:
to/fromToexit tweens that end at or near a clipdata-startboundaryopacity: 0,autoAlpha: 0,visibility: "hidden", anddisplay: "none"as hidden exit statestl.set(...)hidden state at the same boundarygsap_exit_missing_hard_killwith the selector, boundary time, source snippet, and a fix hint that preserves the authored hidden property when possibleWhy
Clip boundaries are the exact points where rendered frames are most sensitive to stale DOM state. A fade-out tween describes a transition, but it does not give the linter or the authoring model an explicit deterministic state to land on when seeking around the boundary.
The existing caption rule proved the class of bug was worth catching, but it only applied to caption-loop patterns. The issue in #473 is broader: any element inside a timed composition can exit at a scene boundary and need the same deterministic cleanup.
Root cause
The GSAP lint rule parser already calculated tween windows and clip metadata existed in the lint context, but no rule connected those two facts:
data-startvalues were not used as scene-boundary checkpoints for GSAP exitsThis PR extends the existing GSAP window metadata with parsed property values, then checks hidden-state exits against same-composition clip start boundaries and same-selector
tl.setcalls.Verification
Local checks
bun run --filter @hyperframes/core test src/lint/rules/gsap.test.tsbunx oxlint packages/core/src/lint/rules/gsap.ts packages/core/src/lint/rules/gsap.test.tsbunx oxfmt --check packages/core/src/lint/rules/gsap.ts packages/core/src/lint/rules/gsap.test.tsbun run --filter @hyperframes/core typecheckbun run --filter @hyperframes/core testbun run --filter @hyperframes/core buildCLI verification
Verified against local fixtures where
#headlineexits at the next clip boundary without a hard kill:gsap_exit_missing_hard_killfor#headlineat3.00stl.set("#headline", { autoAlpha: 0 }, 3.00)subtimeline exit no longer matches an unrelated root composition boundaryBrowser verification
Verified the Studio lint flow with
agent-browseragainst the autoAlpha fixture:http://127.0.0.1:43174/#project/issue-473-autoalphaLintbutton{ autoAlpha: 0 }fix hintqa-artifacts/issue-473/Notes
tmp/issue-473-*fixtures andqa-artifacts/issue-473browser proof are local-only and are not part of this PR