Skip to content

feat(lint): warn on non-seek-safe GSAP className animation#1828

Open
miguel-heygen wants to merge 1 commit into
mainfrom
feat/lint-gsap-classname-not-seek-safe
Open

feat(lint): warn on non-seek-safe GSAP className animation#1828
miguel-heygen wants to merge 1 commit into
mainfrom
feat/lint-gsap-classname-not-seek-safe

Conversation

@miguel-heygen

Copy link
Copy Markdown
Collaborator

Problem

GSAP's className special property (e.g. tl.set(el, { className: "+=locked" }), especially with the relative +=class / -=class add/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 though lint / validate / inspect all pass clean and give no signal. The root cause only surfaces via a snapshot contact sheet. Switching to tl.call() + classList.add/remove fixes 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, severity warning (not error: existing compositions use className and 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 a className property, reading it via the acorn parser's existing per-tween properties (no parser change needed). It only matches the literal className GSAP var, not a DOM class= attribute or an unrelated .className in 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 warning
  • tl.to(el, { className: "active", duration: 1 }) emits the warning
  • a normal transform/opacity tween does NOT emit it
  • the finding is a warning and never counts as an error

The new rule code is also added to the GSAP rule catalog in the frame-worker skill sub-agents (faceless-explainer, pr-to-video, product-launch-video).

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.
@miguel-heygen miguel-heygen marked this pull request as ready for review July 1, 2026 05:00

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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".
  2. The presets are fine and the rule is too broad — the fragile case the PR body calls out is the relative +=class / -=class syntax (stateful add/remove). Absolute-string sets ({ className: "caption-word is-active" }) fully overwrite el.className on every seek hit — the same mechanism as classList.add via tl.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 at gsap.ts:39,138 — a two-line filter.
  3. 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.md are complete for the 3 files that enumerate the rule list (music-to-video/sub-agents/frame-worker.md uses a different structure that points at hyperframes-core for generic rules; consistent with the finding above).
  • Parser doesn't need to change — properties: Object.keys(animation.properties) at gsap.ts:137 naturally surfaces className as a var key. Verified by the test suite passing.
  • severity: "warning" correctly doesn't bump errorCount (hyperframeLinter.ts:49 filters by severity === "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 .className DOM property access or class="…" 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants