fix(lint): stop scene-exit hard-kill rules from contradicting gsap_animates_clip_element#1846
Conversation
…imates_clip_element
Two independent post-release feedback reports of the same contradiction:
scene_layer_missing_visibility_kill / gsap_exit_missing_hard_kill tell you to
add `tl.set(selector, { visibility: "hidden" }, t)` on an exiting scene
element, but when that element is also class="clip", the exact tl.set they
recommend is then flagged by gsap_animates_clip_element (the framework
already owns visibility/display on clip elements). One report worked around
it by wrapping the scene's content in an inner non-clip div and asked that
the fix hint mention that pattern.
Both rules now detect when the exiting/flagged selector is a clip element
(scene_layer_missing_visibility_kill checks the tag's class list directly;
gsap_exit_missing_hard_kill reuses the clipIds/clipClasses maps already built
in its enclosing rule) and, only in that case, point at the inner-wrapper
pattern instead of a tl.set on the clip element itself. Non-clip targets are
unaffected — same fix hint as before.
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at d9aacd3092 (batch review, Group C lint quality; layering on Magi's own vetting).
Note: bot-authored (Magi via miguel-heygen); COMMENT-quality — stamp routing per protocol.
Summary — reroutes the fixHint copy of gsap_exit_missing_hard_kill and scene_layer_missing_visibility_kill to the inner-wrapper pattern when the exiting/flagged selector is itself a clip element. Resolves the contradiction against gsap_animates_clip_element (which errors on tl.set(clip, { visibility })).
Verified
- Detection unchanged, only hint copy changes. The
hasHardKillcheck and the finding emission run before the branch; if the author DID add a hard kill, both rules still exit early. Non-clip targets get the original hint verbatim. No false negatives introduced. - Symmetric fix across two structurally-different rules.
gsap_exit_missing_hard_killreuses theclipIds/clipClassesmaps already built in its enclosing closure (packages/lint/src/rules/gsap.ts:521-538).scene_layer_missing_visibility_killis a separate top-level rule with no closure access, so it readsclassattr on the flagged tag directly. Both routes converge onclass="clip"— the same signalgsap_animates_clip_elementuses at:678-680. Consistent. - Scene-inside-clip nesting:
<div class="clip"><div id="scene1"...>— scene1 isn't itself clip,isClip=false, gets the plain hinttl.set("#scene1", ...). Since scene1 isn't a clip element,gsap_animates_clip_elementwon't fire on it. No contradiction. Correct. - Two tests, one per rule, assert (a) hint mentions "clip element" and "inner", (b) hint does NOT contain the direct
tl.seton the clip selector. Targeted at both parts of the contradiction.
Nits
- 🟡 The inner-wrapper hint template still emits the numeric boundary time (
boundary.toFixed(2)) even though the actual selector is a placeholder ("<inner-selector>"). Author needs to substitute two things; the timing is still a useful reference number. Fine as-is. - 🟡 The hint text is duplicated between the two rules (
"is a clip element — the framework already manages its visibility. Wrap the scene's content in an inner non-clip <div>..."). If a third rule joins this contradiction someday, worth pulling into a shared helper. Not for this PR.
Questions
- ↩️ Is there any consumer (a build step, docs generator, telemetry) that dedupes findings by exact
fixHintstring? If yes, the divergence between clip-case and non-clip-case hints for the samecodewould produce two hint-buckets in whatever dashboard reads them. Probably not — but worth agit grep 'fixHint'if you have downstream aggregation.
— Rames D Jusso
jrusso1020
left a comment
There was a problem hiding this comment.
APPROVE — verified CI green (0 fail / 0 pending) + no open CR at this head. Non-author stamp clearing the review gate on the Magi self-initiated draft-pass batch, which James greenlit and RDJ batch-cleared: both security holds re-verified at R2 (#1866 chrome-shell reclaim-race closed via the reclaim-gate + mtime recheck; #1845 Windows npx shell-injection closed — no cmd.exe, node <npx-cli.js> with pure argv), zero drift on the other nine, all green.
Merge via Magi's normal path (no admin-merge). Ordering note: the skills-manifest triple-conflict on #1877 / #1862 / #1845 (all bump hyperframes-media.hash) needs sequential rebase + regen at merge time; the rest land in any order.
— Rames Jusso
Root-caused from two independent post-release feedback reports of the same lint contradiction.
The contradiction
scene_layer_missing_visibility_killandgsap_exit_missing_hard_killtell you to addtl.set(selector, { visibility: "hidden" }, t)on an exiting scene element. When that element is alsoclass="clip", the exacttl.setthey recommend is then flagged bygsap_animates_clip_element("the framework manages clip visibility via visibility — do not animate these properties on clip elements"). Two users hit this independently:That second report describes exactly the workaround this fix teaches: wrap the scene's content in an inner non-clip
<div>and move the exit tween + hard kill onto that wrapper.Fix
Both rules now detect when the flagged/exiting selector is a clip element and, only in that case, point the
fixHintat the inner-wrapper pattern instead of atl.seton the clip element itself:gsap_exit_missing_hard_killreuses theclipIds/clipClassesmaps already built in its enclosing rule.scene_layer_missing_visibility_kill(a separate, older id-pattern rule) checks the flagged tag's class list directly, since it doesn't share that closure.Non-clip targets are unaffected — identical fix hint as before.
Tests
Two new cases in
gsap.test.ts(one per rule) asserting the fix hint mentions the inner-wrapper pattern and does NOT suggest atl.seton the clip selector. All 67 existing + new tests in the suite pass.