feat(lint): warn on non-seek-safe GSAP className animation#1828
feat(lint): warn on non-seek-safe GSAP className animation#1828miguel-heygen wants to merge 1 commit into
Conversation
Add gsap_classname_not_seek_safe (warning) to the GSAP lint rules. It fires on any GSAP tween method (set/to/from/fromTo) whose vars include a className property. GSAP's className special property is stateful and assumes sequential playback. HyperFrames renders by seeking a paused timeline to arbitrary frame times, so className add/remove (especially the relative +=class / -=class syntax) does not apply reliably under seek: it silently degrades and leaves the class unapplied, with no lint/validate/runtime error. The fix hint points to tl.call(() => el.classList.add/remove(...)) at the desired time, or animating the concrete CSS properties directly. Severity is warning, not error: existing compositions use className and it works in live sequential preview, so this guides without breaking builds. The acorn GSAP parser already surfaces className in a tween's properties, so the rule reads it via the existing extraction path with no parser change. Tests cover the relative +=class set, a plain className to(), a normal transform/opacity tween that must not fire, and that the finding is a warning that never counts as an error. The new code is also added to the GSAP rule catalog in the frame-worker skill sub-agents.
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at e2d804030300161a359c2e0f6dba89f45922852a.
TL;DR: The mechanism is real and the guardrail is well-scoped (properties-check on the existing parser, no parser change; severity=warning so it can't break CI). But the rule as-written will fire on 13 checked-in frame-preset caption-skin templates whose OWN inline comments assert the pattern IS seek-safe — either the presets are wrong (comments lie) or the rule is too broad. Please resolve the same-repo contradiction before landing. Also worth mirroring the guidance into the canonical determinism doc, not just the 3 sub-agent frame-worker READMEs.
🟠 Same-repo contradiction with 13 frame-preset caption skins
skills/hyperframes-creative/frame-presets/{biennale-yellow,blockframe,blue-professional,bold-poster,broadside,capsule,cartesian,claude,cobalt-grid,coral,creative-mode,daisy-days,editorial-forest}/caption-skin.html each contain the shape
// 3-state karaoke via className SETS — seek-safe (gsap.set applies on
// the engine's frame-by-frame seek); reverse-seek also correctly resets
tl.set(el, { className: "caption-word" }, start);
tl.set(el, { className: "caption-word is-active" }, at);
tl.set(el, { className: "caption-word is-spoken" }, nextAt);
and the file-header block at :19 in each: State changes use gsap.set({className}) (applied on the engine's frame-by-frame seek). The rule as-written will emit gsap_classname_not_seek_safe on every one of these tweens on lint, even though the presets are engineered specifically for the seek renderer.
That's a real conflict to resolve, one of three ways:
- The presets are actually broken — then the PR needs to migrate them to
tl.call(() => el.classList.add(...))(and update the header comments in the same PR), otherwise the same repo ships a canary that says "do this" next to a warning that says "don't do this". - The presets are fine and the rule is too broad — the fragile case the PR body calls out is the relative
+=class/-=classsyntax (stateful add/remove). Absolute-string sets ({ className: "caption-word is-active" }) fully overwriteel.classNameon every seek hit — the same mechanism asclassList.addviatl.call, just via.set. Narrowing the rule to strings that start with+=or-=would catch the actual danger without lighting up the preset library.win.propertyValues["className"]is already available on the window atgsap.ts:39,138— a two-line filter. - Both stances are defensible; keep the rule broad but accept the noise — then at minimum update the preset header comments to acknowledge "this fires a warning but it's the seek-safe absolute-string form" so the next reader doesn't get whiplash. Warning-not-error at least keeps CI green, but 13 files worth of "known false-positive" warnings will erode the signal.
I'd lean (2) — the PR body's mechanism story is specifically about +=class accumulation ("It fires for any GSAP tween method … whose vars include a className property"), and the narrower rule is both more accurate and self-consistent with the presets. But this is your call, and it depends on whether you've actually caught the absolute-string form silently failing under seek (the PR body says the failure was a +=locked case).
🟠 Canonical determinism doc not updated
The PR adds the rule reference to 3 sub-agent files (skills/{faceless-explainer,pr-to-video,product-launch-video}/sub-agents/frame-worker.md) but not to skills/hyperframes-core/references/determinism-rules.md, which is the canonical "Animation Runtime Contract" that every skill and reader points at. A frame-worker in music-to-video (whose sub-agent doesn't enumerate specific lint rules) or a new skill that inherits from hyperframes-core won't see the guidance. Suggest adding a bullet to the "Animation Runtime Contract" list, ordered alongside the existing **Do not** gsap.set() clip elements from later scenes / **Do not** call tl.play() items.
🟡 Test coverage missing the actual preset shape
The tests cover: relative +=class set (gsap.test.ts:1385), .to with absolute className + duration (:1404), non-className tween negative (:1423), severity-is-warning (:1441). The most-common in-repo pattern — tl.set(el, { className: "absolute-string" }, absoluteBeat) — is not tested. Add a case for it so the intent is unambiguous either way (fires because rule is intentionally broad, or does-not-fire once narrowed per the finding above).
🟡 String positions silently skipped
extractGsapWindows at gsap.ts:128 skips animations whose position isn't a number ("+=1", "<", ">-0.5"), so tl.set(el, { className: "+=locked" }, "+=0.5") will not emit a warning even though it's exactly the shape the PR body warns about. This is inherited from the shared window-extraction, not new here — but the rule's docstring should acknowledge the blind spot (or a follow-up ticket to lint relative-position tweens for a subset of properties). Not a blocker.
↩️ Merge state is DIRTY
skills-manifest.json has a conflict with main (only the hash entries in that file — no code conflict). Rebase before merging.
Sanity checks that pass
- No same-file sibling protection/exemption list in
packages/lint/src/rules/gsap.ts— the HF#1819 same-file-disagreement pattern doesn't apply. - Sub-agent updates in
frame-worker.mdare complete for the 3 files that enumerate the rule list (music-to-video/sub-agents/frame-worker.mduses a different structure that points athyperframes-corefor generic rules; consistent with the finding above). - Parser doesn't need to change —
properties: Object.keys(animation.properties)atgsap.ts:137naturally surfacesclassNameas a var key. Verified by the test suite passing. severity: "warning"correctly doesn't bumperrorCount(hyperframeLinter.ts:49filters byseverity === "error").- Message + fixHint blame the actual mechanism (stateful, seek-degrades, +=/-= especially) and offer both
tl.call+ concrete-CSS-prop paths. Reader-facing quality is good. - Rule doesn't false-positive on
.classNameDOM property access orclass="…"HTML attribute — it's strictly a vars-object key match.
Verdict
Not ready to leave the queue yet — the 13-preset contradiction is load-bearing (either the docs lie or the rule fires 13 warnings on shipped skills). Once (1) that's resolved, (2) the canonical doc is updated, (3) rebased for the manifest conflict, this is a good preventive guardrail. Warning-not-error is the right call for the ambiguity window.
Review by Rames D Jusso
Problem
GSAP's
classNamespecial property (e.g.tl.set(el, { className: "+=locked" }), especially with the relative+=class/-=classadd/remove syntax, and with arrays of targets) silently breaks in a HyperFrames render. The base CSS class ends up unapplied on affected elements (rendering as unstyled top-left text) even thoughlint/validate/inspectall pass clean and give no signal. The root cause only surfaces via a snapshot contact sheet. Switching totl.call()+classList.add/removefixes it.Why it is not seek-safe
HyperFrames renders by seeking a paused timeline to arbitrary frame times. GSAP's className plugin is stateful and assumes sequential playback, so under seek it does not apply reliably: it silently degrades rather than erroring. No existing gate catches it. A lint warning is the right preventive fix.
The rule
New rule
gsap_classname_not_seek_safe, severitywarning(not error: existing compositions useclassNameand it works in live sequential preview, so this guides without breaking builds). It fires for any GSAP tween method (set/to/from/fromTo) whose vars include aclassNameproperty, reading it via the acorn parser's existing per-tweenproperties(no parser change needed). It only matches the literalclassNameGSAP var, not a DOMclass=attribute or an unrelated.classNamein JS.fixHint: toggle the class at the desired time with
tl.call(() => el.classList.add/remove(...)), or animate the concrete CSS properties directly instead of toggling a class.Tests
packages/lint/src/rules/gsap.test.ts:tl.set(el, { className: "+=locked" })emits the warningtl.to(el, { className: "active", duration: 1 })emits the warningThe new rule code is also added to the GSAP rule catalog in the
frame-workerskill sub-agents (faceless-explainer, pr-to-video, product-launch-video).