fix(lint): remove root_composition_missing_data_duration#490
Conversation
miguel-heygen
left a comment
There was a problem hiding this comment.
This PR is aimed at a real false-positive class, so I would not close it. But the new predicate does not match the runtime's actual Infinity branch yet: it suppresses the warning for the media-floor case that can still emit Infinity, and keeps warning for ordinary timed element clips that runtime resolves to a finite duration.
| // which the runtime resolves to a finite value. | ||
| const hasSubComp = tags.some((t) => readAttr(t.raw, "data-composition-src") !== null); | ||
|
|
||
| if (hasMediaClip || hasSubComp) return findings; |
There was a problem hiding this comment.
This early return is too broad for the runtime condition the rule is trying to mirror. In collectRuntimeTimelinePayload, a root with no authored data-duration, a finite media window, and a loop-inflated root timeline still returns durationInFrames = Infinity (timelineLooksLoopInflated && attrDurationCandidate == null). I verified that shape with <video data-start data-duration="10"> plus a 100s root timeline: this lint rule is silent, but runtime emits Infinity. The inverse also happens for ordinary timed element clips: the PR still warns, but runtime includes those clips in maxEnd and returns a finite duration. The predicate should be realigned with the runtime before this warning is narrowed.
|
You're right on both counts — I had the runtime model wrong. Re-reading
My PR's predicate (silence when there's any media or sub-comp) gets exactly those cases backwards: it silences (2)+(4) which is an Infinity-emitting shape, and warns when there's only div/span clips, where (2) is null so loop-inflation can't even be detected and runtime falls through to the safe path. Now to the awkward bit — I think this surfaces a real policy question, not just a "tighten the predicate" fix. The runtime's true condition turns on (1) and (3), and lint can't statically observe either. The static signals available are the looping shapes from the script body — I see three honest answers: A. Keep the rule, narrow it to "looping shape detected in scripts AND no B. Deprecate this rule and let C. Move duration verification out of static lint entirely and report it from the runtime/render path when I lean B — it's the smallest surface area that still catches the original #243 motivation. Happy to implement whichever direction you want, including a v2 of this PR doing A. The current PR's predicate is wrong and I'll either revise or close it; just want to align on the policy first since the rule's value isn't obvious once the runtime model is precise. |
|
Yes, I agree with your revised model. I would choose B for the static lint surface. Once the runtime condition is written out precisely, So my preferred direction is:
If you want to keep this PR open, I would make it the B version: remove the static rule and update tests around the docs-compliant “composition without root data-duration” shape. I would not merge a narrowed media/sub-comp predicate. |
Lint cannot statically observe the runtime's true Infinity-emission condition: it requires a finite GSAP timeline duration AND a finite media/sub-comp window AND timeline > floor + 1, none of which are visible to the static linter. The looping shapes that drive the condition are already covered by `gsap_infinite_repeat` and `gsap_repeat_ceil_overshoot` (both from heygen-com#243), which point at the real authoring mistake — flagging the missing duration separately was a noisy proxy for the same signal. Per heygen-com#490 review discussion, deprecate the static rule and let those two GSAP rules carry the pre-render coverage. If perfect precision on `durationInFrames = Infinity` is needed, that belongs on the runtime/render path where `shouldEmitNonDeterministicInf` is actually known. Add regression tests pinning the removal: a docs-compliant root without `data-duration` no longer warns, and the canonical loop-inflated shape now surfaces only via `gsap_infinite_repeat` instead of two duplicate findings.
fb83f9c to
ddbf0ad
Compare
|
Pivoted to B. Force-pushed: the narrowing rule and tests are gone, replaced with two regression tests pinning the removal — one for the docs-compliant shape, one for the original Infinity-risk shape (where |
Drops the `root_composition_missing_data_duration` reference now that the rule is gone. Keeps the authoring recommendation (and explains the runtime Infinity case) but points authors at the GSAP rules that actually carry the lint signal: `gsap_infinite_repeat` and `gsap_repeat_ceil_overshoot`.
|
Spotted one more reference while sweeping the repo: |
miguel-heygen
left a comment
There was a problem hiding this comment.
This addresses my requested change. The PR now follows the B direction from the discussion: the static root_composition_missing_data_duration rule is removed instead of narrowed, and the new tests pin both the docs-compliant no-root-duration case and the original loop-risk shape where gsap_infinite_repeat carries the signal. I re-ran the focused composition/GSAP lint tests plus touched-file lint/format checks on head ddbf0ad.
|
Checked the follow-up commit |
What
Remove the
root_composition_missing_data_durationlint rule. The looping shapes it was a proxy for are already covered bygsap_infinite_repeatandgsap_repeat_ceil_overshoot.Why
Working through the runtime in #490, we landed on this precise model of
collectRuntimeTimelinePayload:So
Infinityis only emitted when all of:timelineDurationCandidatepresent)finiteWindowFloorfrom media/sub-comp windowsdata-durationA static linter cannot observe (1) or (3) — those need the real GSAP timeline. The signals it can observe —
repeat: -1, largerepeat: N,Math.ceil(d/c)overshoots — are exactly whatgsap_infinite_repeatandgsap_repeat_ceil_overshootalready flag (also from #243). Once those rules have fired,root_composition_missing_data_durationadds nothing precise; it's a noisy duplicate of the same finding.The original PR's narrowing predicate had it backwards (silenced media-floor cases that do still emit Infinity, and warned on div/span clip cases where loop-inflation can't even be detected). Trying to tighten it further still leaves an imprecise static approximation.
How
Delete the rule from
packages/core/src/lint/rules/composition.ts. No type changes required — finding codes are untyped string literals.Test plan
bunx oxlint packages/core/src/lint/rules/composition.ts packages/core/src/lint/rules/composition.test.ts— cleanbunx oxfmt --check ...— cleanbun run --filter @hyperframes/core typecheck— cleanbunx vitest run src/lint/rules/composition.test.ts(inpackages/core) — 23 tests pass (22 baseline + 2 new pinning the removal)describe("root_composition_missing_data_duration (removed)", ...):data-duration)repeat: -1driving a div clip with no media; the deprecated rule must not fire, butgsap_infinite_repeatstill surfaces the looping mistake (the actionable signal)Follow-up (out of scope)
If perfect precision on
durationInFrames = Infinityis wanted, the right place is the runtime/render path whereshouldEmitNonDeterministicInfis actually known. That would be a separate change against the runtime/producer side, not the static linter. Happy to take that on in a separate PR if there's appetite.