Skip to content

feat(lint): warn on same-track clips that touch at an exact boundary#1884

Open
miguel-heygen wants to merge 1 commit into
mainfrom
fix/adjacent-clip-boundary-lint-warning
Open

feat(lint): warn on same-track clips that touch at an exact boundary#1884
miguel-heygen wants to merge 1 commit into
mainfrom
fix/adjacent-clip-boundary-lint-warning

Conversation

@miguel-heygen

Copy link
Copy Markdown
Collaborator

Summary

Two independent reports (in different sessions) each hit the same glitch and diagnosed it themselves: on a composition with two same-track clips where the earlier clip's end exactly equals the next clip's start, both clips render simultaneously for one frame at that boundary instant. One reporter suggested the exact lint rule shape needed: "adjacent clip data-start(N+1) == data-start(N)+data-duration(N) on the same track."

Root-caused against the actual runtime: isTimedElementVisibleAt in packages/core/src/runtime/init.ts computes visibility as currentTime >= start && currentTime <= end — inclusive on both ends. At an exact touching boundary, both the outgoing clip's <= end and the incoming clip's >= start are true for the same instant, so both paint.

Fix

Extended the existing overlapping_clips_same_track check (which already sorts clips per track) to also flag the exact-touch case, as a warning, not an error:

  • gap < -OVERLAP_EPSILON_SECONDS → genuine overlap → existing overlapping_clips_same_track error (unchanged)
  • -OVERLAP_EPSILON_SECONDS <= gap <= OVERLAP_EPSILON_SECONDS → touching boundary → new adjacent_clips_touch_exact_boundary warning
  • gap > OVERLAP_EPSILON_SECONDS → real gap → no finding (unchanged)

Reuses the OVERLAP_EPSILON_SECONDS (1e-6) threshold from the neighboring rule (recently fixed for float slop in #1851) for a consistent tolerance. Kept at warning severity deliberately — back-to-back zero-gap clips are an extremely common, usually-fine authoring pattern, and most compositions already avoid the glitch via an explicit exit hard-kill, so this surfaces the risk without blocking render.

Test plan

  • bunx vitest run packages/lint/src/rules/composition.test.ts — 95 tests pass
  • bunx vitest run packages/lint/ — 311 tests pass (full package, no regressions)
  • Updated the two existing tests that used touching boundaries to also assert the new warning fires (they previously only asserted the error did not fire)
  • New tests: no warning on a real gap; no warning on a genuine overlap (that stays overlapping_clips_same_track's job)

isTimedElementVisibleAt's clip-visibility check is inclusive on both ends
(currentTime >= start && currentTime <= end). When one clip's end exactly
equals the next clip's start on the same track, both conditions hold for
both clips at that instant, so they render simultaneously for a single
frame - a real, independently reported glitch (two separate reports each
diagnosed it themselves and worked around it by shaving ~1 frame off the
earlier clip).

Warning, not error: back-to-back zero-gap clips are an extremely common,
usually-fine authoring pattern (most already avoid the glitch via an
explicit exit hard-kill), so this flags the risk without blocking render.
Reuses the OVERLAP_EPSILON_SECONDS threshold from the neighboring
overlapping_clips_same_track check for a consistent float-slop tolerance.
@miguel-heygen miguel-heygen marked this pull request as ready for review July 4, 2026 20:16

@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 0aa57b6.

🟢 Approve — real runtime bug, correctly diagnosed, correctly staged as warning

What's right:

  • Runtime claim is verified: isTimedElementVisibleAt at packages/core/src/runtime/init.ts:520 returns currentTime >= start && currentTime <= computedEnd — inclusive on both ends. At an exact touching boundary (current.end === next.start), the outgoing clip's <= end and the incoming clip's >= start are both true for that instant — genuine double-visibility, not a false positive.
  • Refactored gate is a clean tri-partition using the same OVERLAP_EPSILON_SECONDS = 1e-6 from the neighboring rule:
    • gap < -EPSILON → error (unchanged behavior)
    • -EPSILON ≤ gap ≤ EPSILON → new warning
    • gap > EPSILON → no finding (unchanged)
      This preserves the recently-fixed float-slop tolerance for the error path — no regression.
  • Severity is right at warning: back-to-back zero-gap clips are ubiquitous; most compositions already avoid the glitch via an explicit visibility:hidden hard-kill on exit. Elevating to error would fire on nearly every composition.
  • Existing two tests that used touching boundaries as negative fixtures for overlapping_clips_same_track were correctly updated to also assert the new warning fires — otherwise those tests would silently mask a regression on either rule. Nice catch.
  • Fix hint names both remediations (shave ~1/fps off the earlier data-duration, OR add tl.set({ visibility: "hidden" }, ...) on exit) — actionable, not vague.

One thing worth considering (not blocking):

The runtime check itself is arguably the "true" bug — inclusive-on-both-ends means the double-frame is intrinsic to any touching boundary regardless of authoring hygiene. Fixing at the runtime level (e.g. incoming clip's start is > not >= at the exact boundary, or exit-frame precedence) would eliminate the glitch entirely and let this lint rule go away. But that's a semantics change with broader blast radius (existing compositions may rely on the inclusive-end for their own reasons), and this lint rule is the right tactical fix — surfaces the issue, gives an actionable fix, keeps runtime semantics stable. Worth capturing as a "consider hardening the runtime later" note somewhere; not in scope here.

Coupling note: this rule couples to the recently-fixed OVERLAP_EPSILON_SECONDS = 1e-6 from #1851. If that constant changes for the overlap-error rule, this touch-warning tolerance drifts with it — usually desirable (same physical concept, same threshold), but worth being aware of.

Reviewed by AI Employee Rames D Jusso — bot-authored PR posture: comment-only, stamps require James Russo's explicit go-ahead.

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🟢 R1 verdict — runtime claim verified, tri-state epsilon logic correct

Runtime-interop lens: verified the isTimedElementVisibleAt inclusive-on-both-ends claim against runtime code; epsilon-band logic is a clean tri-state extension of the existing overlap check.

Finding-by-finding

1. Runtime claim verification — 🟢

packages/core/src/runtime/init.ts:513-551 (unchanged, verified at HEAD 0aa57b68)

The PR body's runtime claim: isTimedElementVisibleAt uses currentTime >= start && currentTime <= end — inclusive on both ends. Confirmed:

return (
  currentTime >= start && (Number.isFinite(computedEnd) ? currentTime <= computedEnd : true)
);

At the exact boundary t == currentEnd == nextStart, the outgoing clip's t <= currentEnd is true AND the incoming clip's t >= nextStart is true. Both paint. Real observable double-visibility, not a theoretical concern. The warning severity + hard-kill fix hint matches reality.

2. Tri-state epsilon logic — 🟢

packages/lint/src/rules/composition.ts:414-441

Pre-change: if (current.end - next.start > OVERLAP_EPSILON_SECONDS) — equivalent to next.start - current.end < -OVERLAP_EPSILON_SECONDS = gap < -EPS.

Post-change tri-state:

  • gap < -EPSoverlapping_clips_same_track (error, unchanged)
  • -EPS <= gap <= EPSadjacent_clips_touch_exact_boundary (new warning)
  • gap > EPS → no finding (unchanged)

Boundary transitions:

  • gap == -EPS → fails <, hits else if (EPS ≤ EPS true), touching-warning. Small overlap right at the tolerance limit is classified as touching. Matches "consistent tolerance" claim from PR body.
  • gap == EPS → fails <, hits else if (EPS ≤ EPS true), touching-warning. Small gap right at the tolerance limit is classified as touching.
  • gap == 0 → exact touch → warning. Correct.

The old error path is preserved bit-for-bit; the new warning slots between error and no-finding. No existing overlapping_clips_same_track finding is downgraded — the error branch retains its exact condition.

3. Test-shape correctness — 🟢

packages/lint/src/rules/composition.test.ts (updated)

PR body notes two existing tests that USED touching-boundary geometries as their "no overlap" negative assertion were updated to also assert the new warning fires. This is the right pattern — leaving them unchanged would silently regress into false-negatives on the boundary-warning path. The two new tests (real gap → quiet, genuine overlap → stays error) lock the boundary behavior of the tri-state.

4. Cross-rule interaction — 🟢

packages/lint/src/rules/composition.ts:414-441

The change extends an existing rule callback in-place (same array position, same sorting scaffold, same iteration). Doesn't introduce a new rule entry. All findings pushed under the same iteration loop; both codes (overlapping_clips_same_track and adjacent_clips_touch_exact_boundary) can co-exist for different track pairings in a single lint run. No mutual exclusion needed since they're mutually exclusive by construction (the epsilon-band gap ranges don't overlap).

Merges cleanly with sibling PRs #1925 (new rule entry) and #1893 (new rule entry) — this PR touches only an existing rule body, not the array.

5. Fix-hint runtime accuracy — 🟢

The fix hint suggests "shave roughly one frame (~1/fps seconds) off the earlier clip's data-duration" or "add an explicit hard-kill tl.set(..., { visibility: "hidden" }, ...) on the earlier clip at its exit." The hard-kill technique matches the runtime's visibility precedence — the CSS visibility: hidden gets set at the boundary instant, so even though isTimedElementVisibleAt returns true for the outgoing clip, the visibility override overrides the paint. Runtime-consistent advice.


Peer-lens split: Rames confirms the runtime bug reality via isTimedElementVisibleAt. My lens additionally verifies the tri-state boundary math (including gap == ±EPS transitions) and cross-rule interaction with the sibling PRs.

Review by Via (runtime-interop lens)

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.

3 participants