feat(lint): warn when a sub-composition slot blanks before the host (#1540)#1542
Conversation
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at ccb5b116. Nice scoping — rule is targeted at the high-signal shape from #1540 and rightly leaves the runtime semantics alone. A few notes, all non-blocking.
Concerns
tags is flat — tailCovered treats descendants of the candidate as siblings. extractOpenTags returns every open tag in the source; the rule's timed array filters out only the root tag. If a data-composition-src mount contains an inner element with data-start/data-duration that happens to extend to the host's end (e.g. a stray inline timed div in the host file, or content the author put inside the slot div by mistake), tailCovered would suppress the warning even though that descendant lives inside the slot that's about to blank. By convention data-composition-src divs are empty in source (content comes from the external file), so this is a low-probability false negative — but worth one of two cheap mitigations: (a) restrict tailCovered to siblings of the candidate (parent-aware iteration) or (b) require the suppressing element to itself have data-composition-src or class~="clip". Existing rules (timeline_track_too_dense, overlapping_clips_same_track) all treat tags as flat too, so a comment noting the deliberate flat-scope is also acceptable here.
Two siblings, both shorter than host, no tail-coverer. If two sub-comp mounts both start at ~0 and both end before the host (e.g. start=0,dur=10 and start=0,dur=12, host=60), each one's tailCovered check sees the OTHER's end=10/12, which is also < rootDuration - EPSILON, so both warn. Correct outcome (both clips blank the slot), but the message text reads "its slot will be blank" twice on what is really the same gap. Probably fine — two distinct warnings is louder/clearer than one — just flagging in case you'd rather collapse.
Nits
- Message and fixHint duplicate "data-duration is the slot's visible window" — the message is already clear; the fixHint could lead with the actionable line ("Set this sub-composition's data-duration to X to fill the host window…") and drop the restated semantics. (nit)
- Issue reference in the message. Lint messages that link to the canonical issue are dramatically more useful when an LLM hits them. Consider appending
(see #1540)or a docs anchor — the skill doc you added is the natural target. (nit) - Boundary at exactly
START_TOLERANCE=0.5.t.start > 0.5excludes0.5itself; the test atstart=0.3confirms intent. Consider one test pinning the boundary (start=0.5→ fires;start=0.51→ silent) so future refactors don't drift the tolerance silently. (nit) fallow-ignore-next-line complexity— matches the existing project-wide convention (10+ uses inpackages/core/src), confirmed not a typo. No action needed; mentioning for the record in case a future contributor wonders.
What I verified
- Rule registers via the existing
compositionRulesarray →hyperframeLinter.tsre-spreads it; no separate registration list to update. rootTagisfindRootTag= first non-script/style element inside<body>; correct anchor for "the host composition."- Skill doc cross-references #911/#917 and #1540 correctly, distinguishes hold-through-slot from blank-before-host, names the new rule code so an LLM grepping for the warning will land on the explanation.
- Test suite covers the issue repro shape + 8 negative cases (intentional intro, child≥host, non-mount, missing root duration, late start, unknown-duration sibling). Solid boundary coverage.
What I did NOT verify
- Did not run
bun testlocally (no bun in my env). Relying on the PR body's61/61 pass. - Did not trace the rule against real-world
index.htmlfiles inexamples/orregistry/to confirm zero false positives on existing scaffolds. Worth a quicknpx hyperframes lintsweep overexamples/before merge if not already done.
— Rames D Jusso
ccb5b11 to
25df1a3
Compare
|
Pushed
Left as-is intentionally:
On the runtime-semantics question for @vai: the PR encodes |
vanceingalls
left a comment
There was a problem hiding this comment.
Review at HEAD 25df1a3f — addendum to Rames' approve-with-nits above.
The author has, with admirable dispatch, already squashed in patches answering three of Rames' four nits — sound deductions, but a chronological alibi is owed.
What the new HEAD silently absorbed
Rames reviewed ccb5b116; this is 25df1a3f. The author has:
- Added a code comment at
tailCovereddeclaring the flat-tag scope deliberate and naming the empty-by-conventiondata-composition-srcinvariant that bounds the FN window. Addresses Rames' concern #1 the right way (comment, consistent with sibling rules incomposition.ts). - Added a
start=0.5boundary test pinningSTART_TOLERANCEinclusive. Addresses nit #3. - Trimmed the
fixHint— the actionable line ("Set this sub-composition's data-duration to X…") now leads. Addresses nit #1.
Not changed: two-short-sibling double-fire (Rames' concern #2, explicit "optional") and (see #1540) (nit #2). Both author-choice; no other rule in packages/core/src/lint/rules/*.ts links an issue, and the skill-info doc already does the LLM-grep job.
Independent observations
-
EPSILONandSTART_TOLERANCEboth0.5s, named separately, sharing the magic. Defensible (sub-frame tolerance), but on a 6s host with a 5.4s child the tail-tolerance silently swallows ~10% blank. Real-world hosts are long (#1540 repro was 331s), so zero practical exposure. A one-line comment naming why both constants share0.5would pre-empt a future reader trying to tune them apart. -
Deliberate scope under-coverage at
start≠0. A late-starting (start=40, dur=5) sole sub-comp on a 60s host can blank too, but the negative test pins this silent. The PR body ("sole/dominant external mount starting at ~0") and the skill-info doc ("leaving a single full-bleed sub-composition") agree — documented scope, not a silent gap. Flagging so a future bug report of this shape doesn't catch a reviewer off-guard. -
Partial zero-FP sweep:
registry/examples/decision-tree/index.html(15s/15s) does not fire. Rames' broaderexamples/sweep is reasonable; rule scope is narrow enough that the risk is low. -
Skill-info is the highest-leverage piece, as Miguel and Rames called. The rule code is named verbatim, the
#911/#917vs#1540distinction is correctly drawn, and runtime semantics precede lint behavior — exactly the order an LLM hitting the warning needs.
Recommendation
CLEAR. Rames' concerns are either patched or author-choice-defensible at this HEAD. The EPSILON-comment note is a nit, not a hold.
Review by Via
A sub-composition mount whose data-duration ends before the host composition's window leaves its slot blank for the remainder. The runtime behavior is correct (data-duration is the slot's visible window and takes precedence), but a full-bleed sub-composition shorter than the composition is almost always an authoring mistake that fails silently (issue #1540). Add the subcomposition_blanks_before_host rule, scoped narrowly to the high-signal shape — a sole/dominant external mount starting at ~0 whose window ends before the host's — so it stays silent on intentional short clips. Document the slot-window semantics in the sub-compositions reference, distinguishing the hold-through-slot case (#911/#917) from the blank-when-shorter-than-host case.
25df1a3 to
f741e5f
Compare
|
Heads up @vai — HEAD moved to The two-short-sibling double-fire and the |
What & why
Closes the silent-blank authoring trap reported in #1540.
A sub-composition mount whose
data-durationends before its host composition's window leaves the slot blank for the remaining time. The runtime behavior is correct and unchanged:data-durationis the slot's visible window and takes precedence over the child's GSAP timeline — a 15s clip on a 331s composition is supposed to end at 15s. This is distinct from #911 (a child whose GSAP timeline was shorter than its slot — fixed by #917, which holds the last frame through the slot). #1540 is a child whose owndata-durationis shorter than the host, which is almost always an authoring mistake that fails silently.So this PR surfaces the mismatch instead of changing the (correct) runtime.
Changes
subcomposition_blanks_before_host(packages/core/src/lint/rules/composition.ts). Scoped narrowly to the high-signal shape — a sole/dominant external mount (data-composition-src) starting at ~0 whose window ends before the host's, with no other clip covering the tail. This keeps it silent on intentional short clips (e.g. a 15s intro followed by another clip), which is the failure mode that gets lint rules disabled.composition.test.ts) covering the Sub-composition slot still blanks at child data-duration when host continues (after #911 fix in 0.6.110) #1540 shape plus every silent case: intentional intro, child matching/longer than host, non-mount element, missing root duration, late start, and an unknown-duration sibling covering the tail.skills/hyperframes-core/references/sub-compositions.md): document the slot-window semantics, distinguishing the hold-through-slot case (Sub-composition slot goes black after GSAP timeline ends, regardless of host data-duration #911/fix: hold external sub-compositions through host duration #917) from the blank-when-shorter-than-host case (Sub-composition slot still blanks at child data-duration when host continues (after #911 fix in 0.6.110) #1540).Verification
bun run build— exit 0, typecheck cleanbunx vitest run src/lint/rules/composition.test.ts— 61/61 passoxlint— 0 warnings, 0 errors