fix(lint): exempt caption cues from track density#2461
Conversation
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
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 plaincaptionand namespaced variants.subtitle(?:[-_](?:group|line|cue|text))?mirrors for the subtitle-flavor.cg-.+catches the CLI's short-formcg-Ngroup prefix. ✓ isCaptionCuereads both class tokens (whitespace-split, filtered) andid. Tokens don't need to be the sole class — one match wins.idcompared whole. ✓- Placement of the guard.
if (isCaptionCue(tag)) continue;sits after the tag-exempt check and before thedata-startrequirement, 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 withcg-is exempted — a manually-authored class likecg-highlighton a non-caption element would silently skip the density check. Fine in practice since the CLI ownscg-as its caption-group prefix, but worth documenting so future collisions are avoidable. Suggestcg-\d+(numeric-suffix only) if tighter matching is wanted. -
🟢 Same off-topic
resolve.mjsprettier reflow + skills-manifest hash11f3025c7c97dd8fthat #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.
vanceingalls
left a comment
There was a problem hiding this comment.
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-19absorbs the three token shapes the linter already recognizes (caption[-_]…,subtitle[-_]…,cg-…), which keeps this consistent with the existing caption detection inpackages/lint/src/rules/captions.ts:42,74,156,184.cue/textare new additions but map cleanly onto the transcript vocabulary. - Guard is inserted after the tag-exempt short-circuit and before the
data-startgate atcomposition.ts:308-310, so the exemption piggybacks the same iteration without expanding surface for other rules that reusetags.
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:
idbranch is completely untested.isCaptionCueatcomposition.ts:41-47readsreadAttr(tag.raw, "id")and matches it against the same regex, but no test authors a cue with onlyid="caption-…"and no matching class. A refactor that accidentally drops theBoolean(id && …)half would ship silently — Fallow won't catch it becausereadAttrfor id stays live-reachable via other rules, and the sole density test uses class-side matches.subtitleshape family has zero coverage. Every arm of thesubtitle(?:[-_]…)?disjunct is untested; a regression that drops that half of the regex ships silently. The test also skipscaption-word,caption-cue,caption-text, and the barecaption/subtitlecases.
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
What
timeline_track_too_densecountsWhy
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-*, andcg-*) 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
@hyperframes/lintsuite: 376 passed@hyperframes/linttypecheck passed