fix(lint): drop false media_in_subcomposition rule#2765
Conversation
jrusso1020
left a comment
There was a problem hiding this comment.
Additive review at 47a6a722, concurring with @magi. I verified the central claim at source rather than restating Magi's cites.
The rule's premise is false — removal is correct. The runtime drives media at any nesting depth: flat discovery (runtime/media.ts:77, runtime/init.ts:1702 — querySelectorAll("video, audio"), not scoped to root), per-element host resolution (init.ts:724/1858/2927, startResolver.ts:120 — closest("[data-composition-id]")), and local data-start rebased by the ancestor composition offset (init.ts:569-578: "data-start is authored relative to the media element's OWN sub-composition"). That's exactly the machinery for seeking nested media at global time. Matches Magi's sub-composition-video regression fixture rendering with visual parity + 0.956 audio correlation, and @miguel's hands-on verification.
The correction is complete — the false claim is removed from every site, not just the lint rule (Rule 2). I grepped the head: no residual never seeks / renders blank / DIRECT child of host claim about sub-comp media remains in non-test source. Specifically purged/corrected:
packages/lint/src/rules/media.ts— themedia_in_subcompositionclosure removed;media.test.tsflips to a positive assertion ("does not flag<video>inside a sub-composition (runtime drives nested media)").skills/{faceless-explainer,pr-to-video}/scripts/assemble-index.mjs— the pre-render guard ② (media-in-subcomp HARD FAIL) removed + renumbered, with a comment now citingruntime/{media,startResolver}.ts. These are the "pre-render guards" the description refers to.packages/cli/src/registry/registryComponents.test.ts— dropsmedia_in_subcompositionfrom the invalid-media allowlist.skills/hyperframes-core/references/variables-and-media.mdnow states the correct behavior and preserves the one real constraint: a sub-composition timeline cannot target host-root elements (a timeline-selector scope rule), so host-root media motion belongs on the main timeline at global time. That's the kernel the old rule garbled — good that it's kept.
One thing gates merge — CI (Rule 5): Skills: manifest in sync is red at this head. The PR bumps four skill hashes in skills-manifest.json, but CI's regen disagrees — the committed manifest is out of sync; re-run the manifest generator and commit the result before merge. Separately, CLI: npx shim and Producer: … tests show failures but at 0s with no producer/CLI-runtime code in this diff, so they read as infra/setup rather than this change — worth a re-run to confirm.
Verdict: COMMENT — the fix is correct, verified at source, and complete across all sites; it's ready on the merits and only gated on regenerating the skills manifest (and confirming the two 0s failures are infra). Happy to approve once the manifest check is green.
— Jerrai
The media_in_subcomposition rule blanket-errored every <video>/<audio>
inside a sub-composition, claiming nested media is "never seeked/decoded
and renders blank/black". This is false: the runtime discovers media with
a flat document.querySelectorAll("video, audio"), resolves each element's
host composition via closest("[data-composition-id]"), and rebases its
local data-start by the accumulated absolute start of every ancestor
composition (packages/core/src/runtime/{media,startResolver}.ts). Media
seeks and decodes at any nesting depth, verified end to end through the
producer render path.
- Remove the rule and flip its test to assert nested media is NOT flagged.
- Drop the now-dead media_in_subcomposition clause from the registry
components test.
- Drop the equivalent pre-render guard from the faceless-explainer and
pr-to-video assemble scripts.
- Correct the reference docs (hyperframes-core SKILL, data-attributes,
variables-and-media, composition-patterns; hyperframes-cli
lint-validate-inspect): media works at any depth. Preserve the one real
constraint, that a sub-comp timeline cannot reach host-root elements, so
host-root media motion is authored on the main timeline.
47a6a72 to
f067bc6
Compare
What
Removes the
media_in_subcompositionlint rule, which errored on every<video>/<audio>placed inside a sub-composition, and corrects the docs and pre-render guards that repeated the same false claim.Why
The rule asserted that nested media is "never seeked/decoded and renders blank/black". That is not true of the runtime.
Media discovery is flat: the runtime collects elements with a single
document.querySelectorAll("video, audio"), resolves each element's host composition viaclosest("[data-composition-id]"), and rebases its localdata-startby the accumulated absolute start of every ancestor composition (packages/core/src/runtime/media.ts,startResolver.ts). Nesting depth never enters the lookup, so media seeks and decodes correctly at any depth.The practical cost was worse than a noisy warning: the rule was an error, so authors were pushed to hoist media out of the sub-composition it belongs to, or to silence a check that was right about nothing.
How
packages/lint/src/rules/media.tsand flip its test to assert nested media is not flagged.media_in_subcompositionclause from the registry components test.faceless-explainerandpr-to-videoassemble-index.mjsscripts, which enforced the same rule outside the linter.hyperframes-coreSKILL,data-attributes,variables-and-media,composition-patterns, andhyperframes-clilint-validate-inspect.The one real constraint is preserved in the docs rather than dropped with the rule: a sub-composition timeline cannot reach host-root elements, so host-root media motion is still authored on the main timeline. That is a timeline-scoping rule, not a media-nesting rule, and it is what the deleted rule appears to have been a garbled version of.
Net: 49 insertions, 89 deletions.
Test plan
Unit tests added/updated
Manual testing performed
Documentation updated (if applicable)
packages/lint/src/rules/media.test.ts(21 passed): the former positive case now asserts nested<video>/<audio>produce no diagnostic.packages/cli/src/registry/registryComponents.test.ts(2 passed).Traced the runtime path end to end through the producer render to confirm nested media resolves its absolute start and decodes.
Not covered: no new test asserts the sub-composition timeline scoping constraint, which was already documented behaviour and is unchanged by this PR.