Skip to content

fix(lint): exempt caption cues from track density#2461

Merged
miguel-heygen merged 2 commits into
mainfrom
fix/caption-track-density
Jul 15, 2026
Merged

fix(lint): exempt caption cues from track density#2461
miguel-heygen merged 2 commits into
mainfrom
fix/caption-track-density

Conversation

@miguel-heygen

Copy link
Copy Markdown
Collaborator

What

  • exclude caption/subtitle cue elements from timeline_track_too_dense counts
  • preserve the density warning for ordinary timed scene elements
  • add regression coverage for common caption cue class/id forms

Why

A field report used 26 verbatim transcript cues. Splitting them across one, two, or four tracks still warned because the rule treats every timed DOM element as scene complexity and caps each track at three elements. Transcript captions inherently contain many short cues, so the suggested split cannot satisfy the rule and produces a permanent false positive.

Source: https://heygen.slack.com/archives/C0BGC335AQY/p1784081830116249

How

Recognize the caption naming forms already used throughout the linter (caption-*, caption_*, subtitle-*, and cg-*) and skip those timed cues before accumulating density counts. Media, composition mounts, and ordinary scene elements keep their existing behavior.

The separate single-sample contrast warning from the same report is intentionally not changed because the affected composition fixture was not available for a load-bearing reproduction.

Test plan

  • Unit tests added/updated
  • Manual testing performed
  • Documentation updated (not applicable)
  • Focused composition rule test: 117 passed
  • Full @hyperframes/lint suite: 376 passed
  • @hyperframes/lint typecheck passed
  • Core + Studio pre-commit typechecks passed
  • oxlint + oxfmt passed
  • fallow audit passed when run directly (the worktree hook could not create its nested temporary worktree)

@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 221afabe2072ffd7b635c7417dc9fecadc905610.

What this does

Extends the timeline_track_too_dense rule at packages/lint/src/rules/composition.ts:307-308 with a class/id-based exemption for caption cues. Pre-PR, transcript caption tracks (which routinely stack many short cues on one data-track-index) would trip the 3-elements-per-track density warning. Post-PR: any element whose class or id matches CAPTION_CUE_TOKEN (a case-insensitive regex covering caption[-_]{group,word,line,block,cue,text}, subtitle[-_]{group,line,cue,text}, and cg-.+) skips the density counter, alongside the existing tag-based exemption set (audio/script/style/video).

Test at composition.test.ts:176-190 builds a 4-cue caption track on data-track-index="2" covering three variants (caption-group, caption-line, caption_block, cg-4) and asserts no timeline_track_too_dense finding.

Also bundles the same off-topic skills/media-use/scripts/resolve.mjs prettier reflow + skills-manifest hash 11f3025c7c97dd8f that HF#2463 and HF#2464 also carry.

Verification

  • Regex covers the CLI-generated caption class shapes. caption(?:[-_](?:group|word|line|block|cue|text))? matches both - and _ separators, both plain caption and namespaced variants. subtitle(?:[-_](?:group|line|cue|text))? mirrors for the subtitle-flavor. cg-.+ catches the CLI's short-form cg-N group prefix. ✓
  • isCaptionCue reads both class tokens (whitespace-split, filtered) and id. Tokens don't need to be the sole class — one match wins. id compared whole. ✓
  • Placement of the guard. if (isCaptionCue(tag)) continue; sits after the tag-exempt check and before the data-start requirement, so it correctly skips density accounting without affecting other rules that iterate the same tag list. ✓
  • CI: 47 SUCCESS + 2 SKIPPED at head.

Concerns

  • 🟢 cg-.+ catch-all is broad. Any element with a class token starting with cg- is exempted — a manually-authored class like cg-highlight on a non-caption element would silently skip the density check. Fine in practice since the CLI owns cg- as its caption-group prefix, but worth documenting so future collisions are avoidable. Suggest cg-\d+ (numeric-suffix only) if tighter matching is wanted.

  • 🟢 Same off-topic resolve.mjs prettier reflow + skills-manifest hash 11f3025c7c97dd8f that #2463 and #2464 also carry. Whichever of the three merges last will conflict on the manifest hash — trivial rebase but a growing pattern. Worth splitting formatter-only touches into their own PR.

Verdict framing

Small, correct rule extension backed by a targeted regression. LGTM from my side.

Review by Rames D Jusso

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

Reviewed at 221afabe2072ffd7b635c7417dc9fecadc905610. Additive to @james-russo-rames-d-jusso — he covered the shape-family regex coverage, isCaptionCue class+id parity, guard placement, and the off-topic resolve.mjs/manifest-hash bundle shared with #2463/#2464. Below are two gaps I did not see him raise.

Position

RIGHT direction — should ship. The field report (26 transcript cues on one track tripping timeline_track_too_dense on every 1/2/4-track split) is a genuine false positive: transcript cadence is speech-driven, not scene-design-driven, and the density rule's "3 timed elements per track" heuristic isn't the right frame. Exempting caption cues at packages/lint/src/rules/composition.ts:308 is the correct localized fix.

Strengths

  • The regex source-of-truth at packages/lint/src/rules/composition.ts:18-19 absorbs the three token shapes the linter already recognizes (caption[-_]…, subtitle[-_]…, cg-…), which keeps this consistent with the existing caption detection in packages/lint/src/rules/captions.ts:42,74,156,184. cue/text are new additions but map cleanly onto the transcript vocabulary.
  • Guard is inserted after the tag-exempt short-circuit and before the data-start gate at composition.ts:308-310, so the exemption piggybacks the same iteration without expanding surface for other rules that reuse tags.

Findings

🟡 important — test scope narrower than guard scope (composition.test.ts:176-190)

Per feedback_guard_scope_vs_static_test_scope, static-test scope should equal-or-exceed the guard's runtime scope. CAPTION_CUE_TOKEN disjuncts across roughly a dozen arms (bare caption / subtitle, six caption[-_]… suffixes, four subtitle[-_]… suffixes, cg-.+), and isCaptionCue also matches on id. The regression test covers only the class channel across three shape families (dashed caption-group/caption-line, underscored caption_block, cg-glob).

Two gaps are worth closing before merge:

  • id branch is completely untested. isCaptionCue at composition.ts:41-47 reads readAttr(tag.raw, "id") and matches it against the same regex, but no test authors a cue with only id="caption-…" and no matching class. A refactor that accidentally drops the Boolean(id && …) half would ship silently — Fallow won't catch it because readAttr for id stays live-reachable via other rules, and the sole density test uses class-side matches.
  • subtitle shape family has zero coverage. Every arm of the subtitle(?:[-_]…)? disjunct is untested; a regression that drops that half of the regex ships silently. The test also skips caption-word, caption-cue, caption-text, and the bare caption/subtitle cases.

Suggest parameterizing the exemption test — it.each over ["caption", "caption-group", "caption_word", "caption-cue", "caption-text", "subtitle-line", "subtitle_cue", "cg-42"] for the class channel, and a second it.each for the id channel. About 15 lines of test additions to eliminate the guard-scope-vs-test-scope gap for the whole regex surface.

🟢 nit — cg-.+ broad, caption[-_]… narrow (asymmetric suffix matching)

Sibling to what Rames flagged about cg-.+ catching non-caption cg- classes, but pointing the opposite direction: the caption arm has no .+ escape hatch. A perfectly reasonable authoring pattern for individual transcript cues is id="caption-1", id="caption-2", …, id="caption-26" (numeric suffix on the base token, mirroring the cg-N idiom). Those ids don't match caption(?:[-_](?:group|word|line|block|cue|text))? — the anchored $ after the fixed suffix list excludes numeric extensions. Same for caption-container (used at registry/examples/warm-grain/compositions/captions.html) and caption-copy/caption-pill in the frame presets, though those are typically wrappers rather than per-cue elements, so the practical hit rate is low.

Not a blocker for the specific field report (which uses cg-*), but if you want the exemption to be shape-symmetric with cg-.+, consider tightening the cg- arm to cg-[\w-]+ while widening the caption/subtitle arms to accept a numeric-or-token suffix (caption(?:[-_][\w-]+)?). Trade-off is exactly what Rames flagged in the opposite direction — broader matching accepts unintended collisions. Judgement call; noting it so the asymmetry is intentional rather than incidental.

Envelope

Miguel-authored. Envelope check skipped per repo convention.

Verdict: APPROVE
Reasoning: Field-report-driven fix at the right site, correct guard placement, CI 30/30 SUCCESS at head. The test-scope gap is fixable in ~15 additional lines and doesn't gate the shipping of the primary fix. Extrapolation surface (other density-adjacent rules that share the "cues are legitimately dense" assumption) is empty — timeline_track_too_dense is the only per-track density counter in composition.ts, so there's nothing to close as a wider class.

— Via

@miguel-heygen miguel-heygen merged commit f3800f3 into main Jul 15, 2026
70 of 71 checks passed
@miguel-heygen miguel-heygen deleted the fix/caption-track-density branch July 15, 2026 06:05
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