fix: flag visible GSAP transition overlays#1686
Conversation
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: LGTM (with two scope nits)
Head SHA: d01c30040daef1ddb7e26b808c40eeb85041be21
Base: main (standalone, not stacked)
Issue grounding: #1641 — reporter's index_full.html had #tr-flash-1…#tr-flash-9 as position:fixed; inset:0; background:#fff divs, all inline-styled and ID-named. Static dedup then reused the all-white early frames. The repro shape and the lint's detection shape line up exactly.
What the PR does
Two new lint behaviors plus a root-tag fix:
findRootTaginpackages/core/src/lint/utils.ts:80-95now accepts<body data-composition-id ...>as the composition root (kills the misleadingroot_missing_*errors on the repro).- New
gsap_fullscreen_overlay_starts_visiblerule inpackages/core/src/lint/rules/gsap.ts:564-608. Fires when anid'd element with inline-styleposition:fixed|absolute+ zero inset + opaque background has a GSAP tween making it visible, a later GSAP tween hiding it, and notl.set(target, hidden, 0)(or inlineopacity:0) at frame zero. Fix-hint is always correct: addopacity:0to CSS ortl.set("#sel", { opacity: 0 }, 0)before the reveal.
GSAP regime-contract coverage (the big question)
This is a validator-layer-only change. There is no generator-side or runtime-side coupling: the rule reads static HTML at submission time. So unlike STUDIO-5215 / HF #1448, the three-layer drift risk doesn't apply here — there's nothing on the generator or runtime side that needs to move in lockstep.
Cross-PR interaction with #1618 / #1656 / #1662
None at runtime. Those PRs write style.visibility = "hidden" on render ticks (clip ancestor walks, page-side compositor handshake). This rule runs in the lint pipeline against the authored HTML before any runtime writes happen. No double-fire, no missed-hidden interaction. Confirmed by tracing every getComputedStyle / runtime style reference — the new rule doesn't read either.
Toggle / flag gating
Hardcoded into the always-on gsapRules registry. No flag. Default-on in prod. Severity error.
Tests (Pattern #7 — negative defaults)
packages/core/src/lint/rules/gsap.test.ts:147-214 covers:
- positive: visible-at-zero + fade-in + fade-out → error;
- negative via inline
opacity:0; - negative via
tl.set(target, { opacity: 0 }, 0).
Both gate predicates have negative coverage. Good.
Nits (non-blocking — call author judgment)
Nit 1 — silent scope gap: detection is inline-style + id-only (gsap.ts:565-572).
The rule only fires when:
- the element has an
idattribute (no class-based overlays); - the overlay's
position/inset/backgroundare set viastyle="..."(no CSS<style>block resolution); - the GSAP target is a single-
#idselector (getSingleIdSelectorat 313-316 — group selectors"#a, #b"slip through).
The rule name says "fullscreen overlay starts visible" — but it really catches "inline-styled, id'd, single-id-targeted fullscreen overlay starts visible." For the #1641 reporter this is fine (their tr-flash-N divs are all three), but a future composition that authors the overlay CSS in a <style> block, or uses .flash class targets, or batches gsap.to("#a, #b", ...) won't trigger. The fix-hint is still always correct when it does fire, so this isn't a band-aid — it's just narrow first-cut coverage. Worth a follow-up TODO if you want broader catch later.
Nit 2 — micro-duplication risk on multi-timeline compositions (gsap.ts:565 inside for (const script of scripts)).
The overlay loop is nested inside the per-script loop. For each script the loop re-scans all tags. If two <script> blocks each register a timeline that touches the same #tr-flash-1, the same finding gets pushed twice. Real-world rare (one timeline per composition is the norm), but a Set-keyed-by-id dedup at the end of the script loop would be cheap insurance. Not blocking.
CI
Required green: Detect changes, Format, Build, Lint, Typecheck, Test, Fallow audit, Preview parity, Perf (load/fps/scrub/drift/parity), CodeQL (actions, javascript-typescript, python), SDK unit+contract+smoke, Test runtime contract, Studio load smoke, CLI smoke, Smoke global install, Semantic PR title, File size check. Three retries on regression-shards / Windows render are still IN_PROGRESS; nothing failed.
Author proof
PR body lists local bun test ... — 79 pass, lint repro on /tmp/hf-issue-1641/repro reports 9 gsap_fullscreen_overlay_starts_visible and zero root_missing_*, and a bug-render.mp4 browser-proof recording. Reproducible chain documented end-to-end.
Review by Via
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
R1 review @ HEAD d01c3004
Solid detection design — narrowly scoped to inline-styled, full-frame, opaque overlays + verified GSAP visibility evidence. Test fixtures cleanly cover the canonical bug (positive + initial-hidden + hard-kill-at-zero negatives). findRootTag body-as-root extension is a real ergonomic win (the issue's root_missing_* noise drops out cleanly), with all downstream rootTag consumers (core.ts, composition.ts, gsap.ts) treating body-as-root identically to a <div> root — no consumer breaks. CI all green on required checks.
That said, a few false-negative seams worth covering before merge.
| Severity | File:Line | Finding |
|---|---|---|
| 🛑 | packages/core/src/lint/rules/gsap.ts:586-588 |
tl.from({opacity:0}, t) reveal is a false negative. This is the same bug shape (overlay visible from t=0 → reveals at t=N), but isVisibleGsapState({opacity:0}) returns false, so firstVisible never matches and the rule silently skips. tl.from is GSAP-idiomatic for reveals; the parser correctly classifies the method but the rule treats from and to identically by reading propertyValues. Either special-case win.method === "from" to invert opacity semantics, or check fromProperties (the parser stores fromTo's start values there — and could be extended to from similarly). |
| 🛑 | packages/core/src/lint/rules/gsap.ts:575 |
getSingleIdSelector rejects grouped/list selectors. tl.to("#tr-flash-1, #tr-flash-2", {opacity:1}, 7.92) is a common idiom for the issue's exact pattern (the PR body says #1641's repro has #tr-flash-1 through #tr-flash-9 — were these animated individually, or as a list?). The regex ^#[A-Za-z0-9_-]+$ only matches single-id. Same trap for tl.to(["#tr-flash-1", "#tr-flash-2"], …) (array form, which the parser may serialize as a stringified selector). Suggest splitting on , and matching any group via getSingleIdSelector. |
packages/core/src/lint/rules/gsap.ts:565 |
Iterates over tags which includes <body> itself. If a user's <body> has id="x" + style="position:fixed;inset:0;background:#fff", the rule will flag the body element as an overlay. Vanishingly rare in practice but not impossible (especially with the new body-as-root acceptance — authors now have a reason to put attrs on body). Cheap guard: skip when tag.name === "body". |
|
packages/core/src/lint/rules/gsap.test.ts:147-215 |
Test coverage gaps. Three tests, all on tl.to reveal. Missing: tl.from({opacity:0}) reveal (per finding above), tl.fromTo({opacity:0},{opacity:1}) reveal (should pass since propertyValues correctly captures the TO state of 1, but worth pinning), grouped-selector reveal ("#a, #b"), autoAlpha reveal, class-selector overlay (.tr-flash × N elements, current rule needs id to match), CSS-class .tr-flash { background:#fff; position:fixed; inset:0 } (the rule ONLY checks inline style=, missing the CSS-class form entirely — see next finding). |
|
packages/core/src/lint/rules/gsap.ts:568-570 |
CSS-class-defined overlays escape detection. styleLooksFullFrameOverlay only reads inline style= via readAttr(tag.raw, "style"). An overlay styled via <style>.tr-flash { position:fixed; inset:0; background:#fff }</style> + <div id="tr-flash-1" class="tr-flash"> looks bug-identical at render but the rule skips it (no inline style). The PR's repro evidence is inline-styled flashes, but the same bug class lives in any HF composition that scopes overlay CSS into a <style> block. Cross-checking the styles block on the LintContext + matching by #id/.class would close the seam. |
|
| 🟡 | packages/core/src/lint/rules/gsap.ts:590-593 |
laterHidden gate seems overconstrained. The rule requires a subsequent hide-tween at position >= firstVisible.position before flagging. If author wrote tl.to("#tr-flash-1", {opacity:1}, 7.92) and forgot the hide-out entirely, the overlay covers everything from 7.92s onward + the entire pre-7.92 window — more broken than the canonical pattern, but the rule says nothing. Intentional (the laterHidden proves "transition overlay" semantics vs persistent overlay)? Worth a comment for the next reader. |
| 🟡 | Issue #1641 closure scope | The issue body says "the issue is not caused by visibility tweens or CSS changes. Even after removing all tl.set('#sceneN', {visibility:'hidden'}) calls, the render still produces only blank frames for the first ~57 seconds" — explicitly framing this as a non-overlay-related bug at multi-scene scale (11 scenes, ~2000 frames). PR body says reproduction confirmed #tr-flash-1..#tr-flash-9 starting visible. Two possible reads: (a) Miguel found the actual root cause and the issue body's "not visibility" framing was misdiagnosed, or (b) this PR fixes A bug surfaced by the repro but not THE bug the issue describes. If (a), worth amending the PR body to say why the user's "removing visibility tweens made it worse" was a different bug from the overlay-flash issue. If (b), don't close #1641 — file a follow-up for the deeper scene-state-on-seek bug. |
| 🟢 | packages/core/src/lint/utils.ts:82-97 |
body-as-root extension cleanly preserves existing semantics. Only fast-paths when at least one of the three data-* attrs is present, otherwise falls through to the pre-existing <body> → first-child resolution. All 5 rootTag consumers behave identically with body-as-root (no id reads beyond optional ?? undefined, no element-name dependence). Good. |
| 🟢 | packages/core/src/lint/rules/gsap.ts:329-358 |
CSS overlay detection is sensibly conservative. transparent / none / rgba(...,0) / hsla(...,0) correctly excluded as opaque-background candidates; `position: fixed |
CI: Required checks green at HEAD (CodeQL, Lint, Test, Typecheck, Build, Preview parity, Perf, CLI smoke, Test: runtime contract). A few regression shards still IN_PROGRESS; nothing red.
Verdict: Approve once 🛑 finding 1 (tl.from false negative) + 🛑 finding 2 (grouped-selector false negative) are either fixed or explicitly punted to a follow-up. Both ship-blocking because the rule's "fix" is to detect the bug, and the most common idiom for the bug evades detection.
I'm not stamping — flagging <https://github.com/U0ARJFN5S6Q|@Rames Jusso> for the stamp once Miguel addresses the from/grouped-selector seams.
Review by Rames D Jusso
|
Addressed the review nits/blockers in 10be0b0:\n\n- deduped duplicate overlay findings across multiple scripts\n- covered |
3 participants
Summary
<body data-composition-id data-width data-height>as a valid composition root, removing the misleadingroot_missing_*lint error from the repro.gsap_fullscreen_overlay_starts_visiblefor full-frame opaque GSAP transition overlays that start visible before their first opacity/visibility tween.tl.from(... { opacity: 0 })reveal tweens, grouped GSAP selectors like#a, #b, simple id/class stylesheet rules, and duplicate findings across multiple scripts.opacity: 0ortl.set("#overlay", { opacity: 0 }, 0).#tr-flash-1through#tr-flash-9starting visible; static dedup only reused those frames.Test plan
bun run build:hyperframes-runtimebun test packages/core/src/lint/rules/core.test.ts packages/core/src/lint/rules/gsap.test.ts— 83 passbunx oxfmt --check packages/core/src/lint/utils.ts packages/core/src/lint/rules/core.test.ts packages/core/src/lint/rules/gsap.ts packages/core/src/lint/rules/gsap.test.tsbunx oxlint packages/core/src/lint/rules/gsap.ts packages/core/src/lint/rules/gsap.test.ts packages/core/src/lint/utils.ts packages/core/src/lint/rules/core.test.tsgit diff --checkgsap_fullscreen_overlay_starts_visibleerrors and noroot_missing_*error.opacity:0overlay render shows content in early frames.agent-browser: opened and recorded/tmp/hf-issue-1641/repro/bug-render.mp4.