fix(lint): see the grouped gsap.set that stages a whole scene at once - #3737
Conversation
Hiding several elements in one call is the shortest way to stage a scene, and it was the one form the hidden-selector extractor could not read: a multi-element array failed the target regex, which forbade commas, and a single-element array then failed the selector parse, which accepted only a quoted string or a known alias. So the two error rules that ask whether a hidden element is ever properly revealed had an empty hidden set for every grouped hide. Each part of a group now resolves on its own, and a comma-separated selector string resolves the same way. The target pattern stays paren-free so a set whose vars are a variable cannot run past its own closing paren and swallow the next call. Two false positives the wider hidden set exposed in the fullscreen-overlay rule: a fromTo at 0 seats its from-vars immediately, so hidden from-vars there mean the overlay does start hidden; and an overlay hidden by a standalone gsap.set is what that rule's own fixHint prescribes.
5ef5cca to
2f5eada
Compare
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
:large_green_circle: 2f5eada51 COMMENTED
Clean, well-scoped fix. The extractor gap is real (grouped gsap.set([a,b,c], {opacity:0}) never populated the hidden-set), the new target grammar closes it without over-consuming, and the two gsap_fullscreen_overlay_starts_visible false-positive fixes are the direct consequence of the wider hidden-set landing under a rule whose own fixHint prescribes exactly one of those shapes. Test discriminators are the right ones. Non-blocker notes below.
Verified as OK
hiddenSetTargetSelectorsinpackages/lint/src/rules/gsap.tshandles the three shapes cleanly. Bracketed list →slice(1,-1).split(",")per part, each resolved via quote regex OR alias map,undefinedparts drop out (so[]yields[]and the caller skips the call, and[header]now resolves via the alias map — the two forms called out in the body). Comma-string → resolved string is itself.split(",")d, so"#a, #b"fans out. Bare paren-free identifier → alias lookup only.patterninextractStandaloneHiddenSelectors— alt 3[^,()[\]]+?excludes both parens and commas, sogsap.set(document.querySelectorAll(".row"), staggerVars)can't match as a bare target and the whole call is silently skipped, letting the nextgsap.setstill match. Thekeeps a later gsap.set hide visible when an earlier one has non-literal varstest asserts exactly that. Alt 1\[[^[\]]*\]also disallows nested brackets, so a[foo[0], bar]shape can't slip through and mis-parse.scaleX: 0grouped test: the extractor's body regex is(?:opacity|autoAlpha)\s*:\s*0…, soscaleX: 0doesn't populate the hidden-set at all — the exclusion is structural, not accidental.- Callback-body test: the existing
indexInsideNonIifeRange+collectFunctionBodyRangesstill guards, and the// keep IIFEs (they run at parse time)comment is preserved. - fromTo-at-0 fix in
gsap_fullscreen_overlay_starts_visible: the newstartsHiddenAtZeroclause OR's inwin.fromPropertyValues !== undefined && isHiddenGsapState(win.fromPropertyValues). The!== undefinedguard keepsto()windows (nofromPropertyValues) untouched. Consistent with the pre-existingfrom()handling and with GSAP'simmediateRender: truedefault onfrom/fromTo. - Standalone-set fix:
authoredHiddenSelectorsbuilt once from all scripts, checked viaselectors.some(s => authoredHiddenSelectors.has(s))right after thestartsHiddenAtZeroclause — same shape the rule's ownfixHintrecommends, so the two rules no longer close a bounce-loop. - Blast radius across the three
extractStandaloneHiddenSelectorscall sites: the new consumer at the top of thegsapRules[0]handler is a suppress path (wider hidden-set ⇒ fewer overlay findings);gsap_cold_seek_hidden_fromto_missing_reveal/gsap_from_opacity_noopare the fire paths (wider hidden-set ⇒ more findings, matching the +2 corpus claim);gsap_timeline_set_initial_hideis another suppress path (alreadyHiddenshort-circuits before the finding). Corpus A/B "0 removed" only holds if no composition in the 1,181 tripped the suppress paths — plausible, but that's a corpus fact not a source fact.
Concerns (all non-blocking)
fromToimmediateRender is not scoped to t=0. GSAP defaultsimmediateRender: trueon bothfrom()andfromTo(), so afromTo("#overlay", {opacity:0}, {opacity:1}, 5)also seatsopacity=0at initialization even though the tween starts at 5s. The new clause only trustsfromPropertyValueswhenwin.position <= SCENE_BOUNDARY_EPSILON_SECONDS, matching the existingfrom()scoping. That's a scope-consistent choice, not a regression this PR introduced, but it leaves a residual FP surface forfromToat t>0. Comment-only fix if you agree: mention in the block comment abovestartsHiddenAtZerothat the<= epsilongate is a conservative slice, not the semantic truth.- Standalone-set ordering isn't checked.
authoredHiddenSelectorsis aSetbuilt from all scripts, so if a standalonegsap.set(overlay, {opacity:0})were written after the overlay's reveal tween in JS source order, the finding is still suppressed. In practice both fire at initialization and the compete-at-init ordering is subtle enough that a positional index would be over-engineering here — worth a one-line comment that ordering isn't enforced, though. - Bracket parts are
.split(",")d without quote awareness.["#a, #b"](a bracket-wrapped comma-string) splits to["\"#a", " #b\""], both fail the quote regex, and the whole group drops silently. Same for mixes like[a, "b, c", d]. Under-report failure mode, not a false-positive one, so it's the safe side of the trade — but the docstring inhiddenSetTargetSelectorscould note that quoted commas inside bracket parts aren't preserved. - Extractor now runs 3× per script.
extractStandaloneHiddenSelectorsis invoked from the top ofgsapRules[0], fromgsap_cold_seek_hidden_fromto_missing_reveal/gsap_from_opacity_noop, and fromgsap_timeline_set_initial_hide. SiblingcachedExtractGsapWindowsexists for the window extractor — acachedExtractStandaloneHiddenSelectorswould be a one-line add. Perf-only, non-blocker. - Body attributes both new findings to one composition described as
gsap.set(['#clip1','#clip2','#clip3','#cta'],{opacity:0})+fromTo('#clip2', …).gsap_cold_seek_hidden_fromto_missing_revealonly fires on tweens whose target is in the hidden-set, so a finding on#clip3needs a#clip3tween somewhere — the body's "on screen from the first frame, stacked over#clip1" reads like observed visual state, not a second rule-level fire on#clip3specifically. Not a code concern; if you happen to have the two finding rows handy in the corpus diff, worth pasting them in the PR body just so future readers can trace the +2.
What I didn't verify
- Corpus A/B numbers (1,181 compositions → 2 new / 0 removed / 0 changed) — no in-tree harness; taken from author's own reporting.
- Local vitest run of
packages/lint. CI'sTestjob (which runsbun run --filter '!@hyperframes/producer' testand therefore includes the lint package's vitest) is now green at HEAD, so I trust that.
State at HEAD 2f5eada51: isDraft: false, mergeStateStatus: BLOCKED, mergeable: MERGEABLE, reviewDecision: REVIEW_REQUIRED. CI at HEAD: Typecheck, Build, Lint, Format, Test, Test:runtime contract, SDK unit+contract+smoke, Producer unit+integration, Preview parity, CLI smoke, Studio load smoke, Render on windows-latest, Tests on windows-latest: studio-core, Tests on windows-latest: studio-engine-cli, Fallow audit, Analyze (JS/TS + Python + Actions), CodeQL — all pass. hyperframes OSS runs dismiss_stale=false / require_last_push_approval=true, so any approval needs to pin to this exact SHA.
— Review by Rames D Jusso
jrusso1020
left a comment
There was a problem hiding this comment.
Approving at 2f5eada515f06094ef8816dee726ed52994482a5. Read the rule myself rather than riding the prior review; the PR body's claims hold up against source.
The two gates you describe are both real. Old pattern ([^,]+?) genuinely cannot span a comma, so gsap.set([a, b, c], {...}) never matched. And a single-element array did get past it and then die in the parse, because /^(["'])([^"']+)\1$/ on ['#a'] fails on the leading bracket and aliases.get("['#a']") misses. So the hidden set was empty for every grouped hide, and the two rules that consume it had nothing to work with. Verified both by reading the pre-image, not just the description.
The paren-free constraint is the load-bearing part of the new pattern and it is worth having a test pinned to it, which you do. [^,()[\]]+? cannot match document.querySelectorAll(".row"), so when the vars are a variable and there is no { to stop at, the match simply fails at that position instead of running past the closing paren and eating the next gsap.set. A comma-permissive target would have made the extractor lose the one hide the rule needed -- a silent, data-dependent false negative, which is the worst shape for a lint rule. Good instinct to close it explicitly.
hiddenSetTargetSelectors degrades in the safe direction. A part that resolves to nothing drops out rather than voiding the group, so a partially-parseable array still contributes what it can. The known limit is that bracket splitting is not quote-aware, so ["#a, #b"] splits into "#a and #b", neither of which parses as a quoted literal or an alias, and the whole entry drops. That under-reports -- a smaller hidden set means the reveal rules ask fewer questions and produce fewer findings, never more. Correct direction to fail in, and not worth code to fix.
One pre-existing limit that now matters slightly more, since the extractor reaches more call sites: the vars group is \{([\s\S]*?)\}, which is lazy and stops at the first }. gsap.set(x, { opacity: 0, transform: { x: 0 } }) still works because opacity precedes the nested object, but { transform: { x: 0 }, opacity: 0 } truncates before the opacity and misses the hide. Under-reports again, and it predates this PR -- noting it only because widening the target set makes it reachable from more places.
The two false-positive fixes read correctly. Or-ing fromPropertyValues into startsHiddenAtZero matches how fromTo seats its from-vars, and gating it on <= SCENE_BOUNDARY_EPSILON_SECONDS is a conservative slice of immediateRender rather than the full semantics -- fine, since erring toward "starts hidden" suppresses a finding rather than inventing one. The authoredHiddenSelectors check is sound: extractStandaloneHiddenSelectors skips non-IIFE callback bodies, so what survives is top-level and IIFE code that genuinely runs at parse time, which is the t=0 the rule is asking about. tl.set(...) correctly never matches.
On the corpus result: 2 new, 0 removed, 0 changed across 1,181 compositions is the right shape for this change -- the extractor only ever grows the hidden set, so anything but "0 removed" would have meant a regression somewhere. Worth stating in the PR body that the direction is structural, not just observed.
CI is green at this head.
-- Rames
The hole
extractStandaloneHiddenSelectorsbuilds the hidden-selector set that bothgsap_cold_seek_hidden_fromto_missing_revealandgsap_from_opacity_noopwork from. It could not read an array target — the shortest and most natural way to stage a scene:Two independent gates blocked it:
([^,]+?), which cannot contain a comma, so a multi-element array never matched at all;So for every grouped hide the hidden set was empty, and the two error rules that ask "was this hidden element ever properly revealed?" had nothing to ask about. A comma-separated selector string (
gsap.set("#a, #b", …)) was silently half-broken the same way.The change
hiddenSetTargetSelectorsresolves each part of a group on its own; a part that resolves to nothing drops out instead of voiding the whole group. A resolved string is split on commas too, so both grouping forms are handled by one path.The target pattern is now an explicit alternation — bracketed list, one quoted string, or a bare paren-free expression. Keeping it paren-free matters: a comma-permissive target would let
gsap.set(document.querySelectorAll(".row"), staggerVars)(whose vars are a variable, so there is no{to stop at) run past its own closing paren and consume the nextgsap.set, losing the only hide the rule needed. There is a test for exactly that.Two false positives the wider hidden set exposed
Both in
gsap_fullscreen_overlay_starts_visible:fromToat 0 seats its from-vars immediately, exactly asfrom()does — so hidden from-vars there mean the overlay does start hidden. The rule was reading only the destination.gsap.setis what this rule's ownfixHintprescribes, so it now consults the source-level hidden set.Verification
Corpus A/B on 1,181 real compositions (extracted from production zips, same runner both sides, findings sorted and diffed):
gsap.set(['#clip1','#clip2','#clip3','#cta'], { opacity: 0 })followed byfromTo('#clip2', { opacity: 1, x: 1080 }, { x: 0, … }).fromTorenders immediately by default, so the visible from-state lands at t=0 and undoes the hide —#clip2and#clip3are on screen from the first frame, stacked over#clip1.Tests: 8 new cases — the three grouped forms and the comma-string form firing, the
scaleX: 0group and the callback-body group staying quiet, the paren-safety case, and the two overlay false positives.557 passed.tsc --noEmit0 errors,oxlint0,oxfmt --checkclean.Scope
This closes the extractor hole. It does not close the neighbouring gap that an element with no reveal tween of its own is never examined at all, because these rules iterate tweens rather than hidden elements — that one is a bigger change and belongs on its own.
🤖 Generated with Claude Code