fix(registry): remove invalid media from caption components#2454
fix(registry): remove invalid media from caption components#2454miguel-heygen wants to merge 2 commits into
Conversation
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 76e489e603078a7782f3121979b32f9c01aa5dfa.
What this does
Root-fix to a field-reported bug: 14 caption-* registry components (installable snippets that mount as sub-compositions) shipped empty <video> placeholders that tripped both media_missing_src (no source) and media_in_subcomposition (media inside a sub-composition). The PR:
- Removes the empty
<video>element from each of 14 caption components' installable HTML snippets AND their orphan CSS (#wp-video,#bg-video,#avatar-video,#gl-video, etc.). - Removes the same demo videos from the parallel
demo.htmlfiles for visual parity. - Adds
packages/cli/src/registry/registryComponents.test.ts— a registry-wide regression that walks everyregistry/components/*/registry-item.jsonmanifest, lints eachhyperframes:snippetHTML file vialintHyperframeHtml, and fails if any producesmedia_in_subcompositionormedia_missing_src.
Net diff: +42 / −488 — almost all deletion, which is the right shape when the fix is "stop shipping content that violates the contract."
Verification
- Registry-wide reproduction confirmed. The 14 fixed components exactly match the list of
caption-*directories that contained<video>in their installable snippet: caption-clip-wipe, caption-editorial-emphasis, caption-emoji-pop, caption-glitch-rgb, caption-gradient-fill, caption-kinetic-slam, caption-matrix-decode, caption-neon-accent, caption-neon-glow, caption-parallax-layers, caption-particle-burst, caption-pill-karaoke, caption-texture, caption-weight-shift. Zero remaining<video>in installable snippets across the entireregistry/components/tree post-fix (verified via grep). caption-blend-differencecorrectly untouched. Its<video>appears only inside HTML comment blocks (documentation of composition-setup patterns for consumers). Real HTML parsers (whichlintHyperframeHtmluses) skip comment contents, so the test passes and the doc example remains readable.- Test scoping is right. Manifest-driven filter (
type === "hyperframes:snippet"AND.html) excludes non-installable assets like previews, images, anddemo.htmlfiles (which aren't in the manifest'sfilesarray). Demos are legitimately allowed to have media because they're rendered as root compositions in preview contexts, not as sub-compositions. - Orphan CSS scrubbed alongside each element. Removed CSS rules (
#wp-video { position: absolute; inset: 0; z-index: 0; ... }etc.) match one-to-one with the removed elements. No dangling selectors, and no other rules in these components reference those IDs. background: transparenton caption roots preserved. The caption components remain true overlays that composite onto whatever the parent composition renders — the pre-PR videos were demo-only visual filler for authors, never load-bearing pixels for installation consumers.- No stale doc/changelog references to the removed IDs (verified via grep — only hit in
docs/changelog.mdx:65mentions "avatar-video" in an unrelated HeyGen recipe context, not the removed#avatar-videoelement). - Test failure output is actionable.
invalidMediaarray populated as<name>/<file.path>: <finding.code>— a future regression names the exact violation. Vitest'stoEqual([])mismatch shows the full array in the failure message.
Adversarial pass
- Lint-scope narrowness. The test hard-codes exactly
media_in_subcompositionandmedia_missing_src— the two codes this bug family exercised. If a future lint rule adds another sub-composition contract violation (saymedia_autoplay_in_subcomposition), it would bypass this guard. Consider either: (a) expanding to anylint.severity === "error"finding for sub-composition-scoped installables, or (b) parameterizing the list. Not blocking — the two codes cover the reported and adjacent contract violations. Nit / future-proofing. - Snippet-type filter tied to a magic string.
file.type !== "hyperframes:snippet"skips any file whose manifest type isn't that exact string. If a future component type is introduced (e.g.,hyperframes:template), it'd bypass the test silently. A// If new snippet-type strings are added, extend this filter — otherwise this test won't lint them.comment above the filter line documents the maintenance obligation without over-engineering. Nit. - JS-created media not covered. Static HTML linting misses runtime-created
<video>elements. No current component ships JS, but if future components include init scripts that mount media, this test won't catch it. Note-only; the current class is fully addressed. - Non-installable component types elsewhere. The 25-directory registry includes
grain-overlay,motion-blur,grid-pixelate-wipe,morph-textetc. — the scan-every-manifest pattern extends coverage to them automatically. Verified those directories have zero<video>/<audio>/<source>in installable snippets (they were never contract-violating; the fix scope was accurately named as caption components). Good. - Fallow audit reporting FAILURE at the moment but CI is still running per Magi's message. Preliminary; will re-evaluate when it settles. Not blocking pending completion.
Verdict framing
Comprehensive root-fix: extends the field-reported bug into the class-level violation, removes all offenders + their orphan CSS, and lands a manifest-driven regression that catches every future addition through the SAME class. Two nits are documentation/future-proofing rather than fixes. LGTM from my side.
vanceingalls
left a comment
There was a problem hiding this comment.
Reviewed at 76e489e603078a7782f3121979b32f9c01aa5dfa. REQUEST CHANGES — close on the class-fix scope, short on the regression's second-class enforcement.
Exemplar posture — credit where due
This is exactly the shape Miguel's directive is asking for: one field report → registry-wide reproduction found 13 sibling sites → all 14 installable snippets + their demos fixed in one PR + a new global regression that would have caught the class before it shipped. The +42 / −488 deletion-shape is the right silhouette for "stop shipping content that violates the contract." Nothing bandaid about this. My push-back below is on the regression under-enforcing what its own PR body claims — the class-fix itself lands correctly.
Strengths
- Sweep is genuinely complete for the class. All 25 subdirectories of
registry/components/audited; the two untouched captions are clean by inspection —registry/components/caption-highlight/caption-highlight.htmlhas no<video>in any form, andregistry/components/caption-blend-difference/caption-blend-difference.html:53uses<video>only inside HTML-comment blocks (bounds at :1-26 and :48-83). No dead sweep sites. - Removed CSS matches removed elements one-for-one. Verified for
caption-clip-wipe.html— the#wp-video { position: absolute; inset: 0; z-index: 0; ... }block that dropped with the element has no remaining referrers in the file. Not a dangling-selector job. - Test scaffolding is dynamic, not hardcoded.
packages/cli/src/registry/registryComponents.test.ts:22usesreaddirSync(componentsDir, { withFileTypes: true })and filtersmanifest.filesbytype === "hyperframes:snippet"— so a future component gets audited automatically. Good future-proofing on the scan surface. (One caveat below on the finding-code surface.) - Manifest-driven filter is precise. Skips
demo.html(not in the manifest'sfiles[]) and previews correctly, so this is genuinely an "installable" audit, not a broad HTML sweep. That's the right scoping choice — but see F2 below on demo coverage.
Blocker
media_in_subcompositionenforcement is aspirational, not functional.packages/cli/src/registry/registryComponents.test.ts:32filters findings on bothmedia_in_subcompositionandmedia_missing_src, and the PR body says the regression "lints every installable HTML snippet for invalid nested or source-less media." But the test callslintHyperframeHtml(readFileSync(...))at :31 with no options —isSubCompositiondefaults toundefined, and the rule atpackages/lint/src/rules/media.ts:351opens withif (!options.isSubComposition) return findings;. Somedia_in_subcompositioncan never fire from this call site. The filter branch on that code is dead. Every other snippet-lint caller in the repo passes it explicitly — seepackages/lint/src/project.ts:240(isSubComposition: true). Concrete failure mode this leaves open: a future PR reintroduces<video src="foo.mp4" data-start="0" id="bg-video">into a caption snippet —media_missing_srcwon't fire (src is present),media_in_subcompositionwon't fire (option not set), and the test silently passes on the exact regression this PR claims to prevent. Fix: pass{ isSubComposition: true }tolintHyperframeHtmlin the test.
Important
demo.htmlis edited by this PR but not guarded by the regression. The PR deletes<video>markup from all 14demo.htmlfiles (28 of 29 files in the diff), yet the test atregistryComponents.test.ts:24filters ontype === "hyperframes:snippet"anddemo.htmlisn't in any component's manifestfiles[](audited across all 25 manifests). So future regressions to demos silently bypass CI. Rames read this as "demos are legitimately allowed to have media because they're rendered as root compositions" — if that framing is correct, this PR's demo edits are voluntary tidying, not part of the class-fix, and the PR body's "matching demos" claim overstates the coverage. Either way, the disposition should be explicit in the PR body: are demos part of the class or not? If yes, the regression should include a demo-html walker. If no, tighten the description.
Nits
packages/cli/src/registry/registryComponents.test.ts:17— Fallow flags cognitive complexity 17 (threshold 15). A tiny refactor to awalkComponent(entry) → invalidMedia[]helper drops the nested-loop count and preserves the failure-message shape.- The finding-code allow-list at
:32is a hardcoded two-code list. Once F1 is fixed, considerif (finding.severity !== "error") continue;— auto-picks up any new error-severity contract rule (e.g. a futuremedia_autoplay_in_subcomposition) without a maintenance step. packages/cli/src/registry/registryComponents.test.ts:26—JSON.parse(readFileSync(...))onregistry-item.jsonthrows ENOENT + crashes the whole test if any component subdir ever lacks the manifest. Wrap in a try/skip or add a "manifest present" assertion so a missing-manifest bug reports as a named test failure, not an uncaught ENOENT.
Divergence from @james-russo-rames-d-jusso
Rames landed LGTM (COMMENTED) with two nits on lint-scope narrowness (two hardcoded codes) and JS-created media not covered. Overlap on the "two-hardcoded-codes" surface — I've expanded it into the concrete failure mode (F1's dead filter) which turns Rames's nit-tier observation into a blocker: the second code isn't just narrow, it's non-functional at this call site. Rames noted the demos-vs-installables framing but treated it as scoping intent; I'm calling F2 as PR-body/test-scope misalignment that should be resolved either way. Non-overlap: Rames didn't cross-reference packages/lint/src/project.ts:240 for the sub-composition-linting convention, which is what makes F1 concrete rather than speculative.
CI
Fallow audit failing on the new test's cognitive complexity (minor); CLI smoke (required), Test, Tests on windows-latest, Render on windows-latest, Render catalog previews, Analyze (javascript-typescript), Smoke: global install still pending as of head SHA time — recheck before merge that all required checks land green.
Verdict: REQUEST CHANGES
Reasoning: Class-fix scope is complete and the exemplar posture is right, but the regression as written only enforces one of the two contract-violation codes the PR body claims — a future PR that reintroduces sourced media into a caption sub-composition would ship silently. Two-line fix (pass { isSubComposition: true }) closes it.
— Via
vanceingalls
left a comment
There was a problem hiding this comment.
R2 verification at 4117d775851c1fdf2dc67620027f031687e095bb — comment-only pending stamp routing. RIGHT direction, R1 blocker cleanly resolved.
R1 finding audit at new SHA
- F1 (blocker) —
media_in_subcompositiondead filter → ✅ RESOLVED.packages/cli/src/registry/registryComponents.test.ts:25-27now callslintHyperframeHtml(readFileSync(...), { isSubComposition: true }). Full dispatch chain verified end-to-end:HyperframeLinterOptions.isSubComposition?: boolean(packages/lint/src/types.ts:22-24) →buildLintContext(html, options)propagates ontoLintContext.options(packages/lint/src/context.ts) →lintHyperframeHtmlpasses ctx to rules (packages/lint/src/hyperframeLinter.ts:30-34) →media_in_subcompositionrule atpackages/lint/src/rules/media.ts:349-366readsoptions.isSubCompositionand exits early only when falsy. Passingtruefrom the test now actually activates the rule. The concrete failure mode I described in R1 (a future<video src="foo.mp4" data-start="0" id="bg-video">reintroduction slipping past) would now tripmedia_in_subcompositionunder this call site. - F2 (important) —
demo.htmlunguarded → ✅ RESOLVED. New second test atpackages/cli/src/registry/registryComponents.test.ts:61-70("ships demos without source-less media") plusinvalidDemoMedia(entryName)helper at:39-47walks everydemo.htmland filters findings formedia_missing_src. The helper correctly does NOT passisSubComposition(demos are root compositions, per Rames's R1 framing) and correctly checks the specific class that motivated the demo edits — grep of the removed demo markup confirms every deleted<video>hadid="…"+data-start="0"+ nosrc, exactly the pattern that tripsmedia_missing_srcatpackages/lint/src/rules/media.ts:478-486. Demo-class regression is now CI-caught. - F3 (nit) — cognitive complexity 17 → ✅ RESOLVED. Test refactored into two focused helpers (
invalidInstallableMedia,invalidDemoMedia) — the twoit()bodies are now ~4-line loops. Fallow audit at head SHA reports SUCCESS instatusCheckRollup, confirming the complexity gate passes. - F4 (nit) — hardcoded finding-code allow-list → ❌ NOT ADDRESSED. Line 29 still hardcodes
finding.code !== "media_in_subcomposition" && finding.code !== "media_missing_src". Considerif (finding.severity !== "error") continue;for the installable test to auto-pick up future contract-error codes. Deferred is fine. - F5 (nit) — ENOENT on missing manifest → ❌ NOT ADDRESSED.
packages/cli/src/registry/registryComponents.test.ts:18-20still parsesregistry-item.jsonunguarded. Deferred is fine.
New affordances audit
- Demo source-less-media guard (
invalidDemoMedia). In-scope, not scope creep — it's the F2 resolution reframed. Correct semantics: noisSubComposition(demos are root), filters onmedia_missing_src(matches the removed markup shape). Note the demo test only checksmedia_missing_src, not other error-severity findings — a demo could still ship with aplaceholder_media_urlor aself_closing_media_tagwithout CI catching it. Not blocking; the class this PR is closing is source-less media specifically. Same F4-adjacent extrapolation opportunity. - Lower-complexity helpers (
invalidInstallableMedia,invalidDemoMedia). Semantics preserved: still aggregate toinvalidMediastring arrays with the same${name}/${path}: ${code}shape, stillexpect(invalidMedia).toEqual([])gates. Refactor is byte-safe on aggregation behavior.
Extrapolation lens (per Miguel 2026-07-14)
The F4 opportunity — swapping the two-code allow-list for severity: "error" — is the extrapolation candidate this PR surfaces. Same shape as the class-fix logic: "if a new error-severity contract rule for sub-composition installables lands (e.g., media_autoplay_in_subcomposition), does this regression cover it?" With the current code, no — a maintenance step is required. With severity === "error", coverage is automatic. Not calling it a blocker at R2 (F4 was already a nit at R1 and this is a legitimate accept-follow-up call), but flagging as should-fix-in-a-follow-up so the guard doesn't need re-touching when the next contract rule lands.
Divergence from prior reviewers
Rames landed LGTM+nits at R1 on 76e489e, treating the two-code allow-list narrowness as a future-proofing note. I extended that observation into F1 (dead filter → blocker) at R1 by cross-referencing packages/lint/src/project.ts:240's existing isSubComposition: true convention. R2 addresses F1 concretely — the compose picture is now: Rames's LGTM (class-fix + demo-scoping intent) + my closed blocker (regression enforceability) both landed correctly.
Envelope
Clean. No Co-Authored-By: Claude on either commit; no 🤖 Generated with in the PR body.
CI
Head SHA has multiple in-progress checks (CLI smoke (required), Test, Typecheck, Analyze (javascript-typescript), regression-shards 1-8, Tests on windows-latest, Render on windows-latest, Producer: integration tests, Build, Perf: *, Render catalog previews, Preview parity, CLI: npx shim (windows-latest)) — no failures at freshness pull, but the required CLI smoke needs to land green before merge. mergeStateStatus: BLOCKED currently reflects my prior CHANGES_REQUESTED review.
Verdict: COMMENT (LGTM in substance — pending Vai stamp routing per Via's default posture on bot-driven PRs)
Reasoning: R1 blocker cleanly resolved with real dispatch-chain wiring, not a rebase. F2 also resolved via the new demo guard. Two nits deferred as follow-up. Class-fix + regression compose to prevent both reported and adjacent contract violations from shipping.
— Via
Summary
Verification
Review notes
This intentionally fixes the class rather than only caption-editorial-emphasis. Demo files receive the same removal because the empty videos had no source and did not contribute pixels; their visual backgrounds remain authored on the composition root.