mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-09-12 23:29:50 +00:00
fix(core,producer,cli): pre-flight validation for empty/malformed sub-compositions (#1831)
* fix(core,producer,cli): pre-flight validation for empty/malformed sub-compositions The #1 render failure bucket in production telemetry (PostHog project 356858, dashboard 1783183 "HyperFrames — Bottom-Line & Activation"; ~65-69K occurrences / ~27-28K affected users over 30 days, ~80% via AI-agent authoring flows) is a `data-composition-src` reference pointing at a scene file that is empty, malformed, or missing. Root cause, traced end-to-end: - The literal error "Composition HTML is empty or could not be parsed: <path>" is real (not a PostHog paraphrase) — thrown by a since-reverted guard in packages/core/src/compiler/inlineSubCompositions.ts (#1364), then changed to a silent skip in #1678 to avoid aborting renders on partial content during authoring. #1629 added per-assembler guards for 3 skill workflows (product-launch-video, faceless-explainer, pr-to-video), but general-video and hand-authored flows — where the dominant filename `scene-title.html` (40K+/68K of the bucket) originates — have no assembler and thus no guard. #1678 assumed the assembler guards from #1629 covered this pre-render; they only covered 3 of the many authoring flows. - On current `main`, an empty/malformed data-composition-src file no longer crashes or throws during render — it's silently dropped by the tolerant inliner. Reproduced locally: `hyperframes render` on a project with an empty scene-title.html "succeeds" after ~93s (two 45s pollSubCompositionTimelines timeouts) with the scene silently missing from the output video. `hyperframes validate` also reports "No console errors" for the same broken project. - The raw `Cannot destructure property 'firstElementChild' of 'documentElement' as it is null` crash reproduces directly against linkedom (the DOMParser polyfill packages/cli/src/utils/dom.ts installs in the real CLI runtime) for empty and non-HTML input — confirmed with a standalone repro script, not just inferred. jsdom/happy-dom (used in this repo's own test environment) are spec-compliant and never produce a null documentElement, which is why this needed a linkedom-specific test file. Fix: - New shared helper `checkSubCompositionUsability` (packages/core/src/compiler/subCompositionValidity.ts) is the single source of truth for "is this data-composition-src file usable" — mirrors the inliner's own parse/template/body logic so all callers agree. - `inlineSubCompositions.ts` (preview/studio bundling) now uses the shared helper internally but keeps its #1678 tolerant skip-and-continue behavior unchanged — mid-authoring iteration on a partial project must keep working. `onMissingComposition` now also receives a human-readable reason. - New render-only pre-flight (`assertSubCompositionsUsable` in packages/producer/src/services/htmlCompiler.ts) walks every data-composition-src reference (including nested ones, root-relative, matching parseSubCompositions' own resolution) before any compilation work starts, and throws naming every offending file at once. This is unconditional — not gated behind --strict — because a render that silently drops a scene is strictly worse than one that refuses to start. Confirmed locally: render now fails in ~0.4s with an actionable message instead of "succeeding" after 93s with a missing scene. - New `hyperframes lint` rule `missing_or_empty_sub_composition` (packages/cli/src/utils/lintProject.ts) surfaces the same check as a file-scoped, actionable lint error (already unconditional — lint exits 1 on any error). - `hyperframes validate` now also runs this check before launching a browser, so it no longer reports "No console errors" for a project with a broken sub-composition. - `packages/core/src/parsers/htmlParser.ts`: guarded every `documentElement`-may-be-null access (parseHtml, updateElementInHtml, addElementToHtml, removeElementFromHtml, extractCompositionMetadata, validateCompositionHtml) with a new typed `CompositionHtmlParseError` (or, for validateCompositionHtml's collect-and-report contract, a typed validation failure) instead of a raw crash. Tests: empty file, whitespace-only, malformed/non-HTML, missing file, nested sub-compositions (both happy path and broken-grandchild), and the happy path — at the shared-helper, lint, and render pre-flight layers. Not changed: the AI-agent authoring skills (skills/*). general-video and hand-authored flows have no assemble-index.mjs equivalent to guard, so the fix is at the CLI/render layer instead — flow-agnostic, covers every authoring path, and the skills' existing "run lint/validate and stop on failure" guidance now actually catches this class of mistake once run. Not run in this environment: the producer package's full regression-harness test suite (`bun test` in packages/producer) — it performs heavy real rendering (S3 asset downloads, Google Fonts fetches, full video encodes) and did not complete in a reasonable time in this sandbox. Verified instead via the targeted test file for all touched code (76/76 passing), whole-repo typecheck/build/oxlint, `fallow audit` (complexity/duplication/dead-code gate, clean), and manual end-to-end CLI runs (render/lint/validate) against reproduction projects, including a nested sub-composition scenario. CI should run the full producer suite before merge. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * refactor(parsers,lint): port empty-composition pre-flight to extracted packages Rebased onto main, which extracted @hyperframes/lint from core (lint depends only on parsers, not core). Relocate checkSubCompositionUsability from core to @hyperframes/parsers so both core (inliner) and lint can consume it without a core<->lint cycle; core keeps a @deprecated re-export shim. Correctness fixes from code review: - checkSubCompositionUsability now returns "no-composition-root" when the <template>/<body> content has no [data-composition-id] element (previously a marker-free placeholder body passed both guards). - lint's missing/empty sub-composition rule now only checks files reachable via data-composition-src from the root (matching render pre-flight), instead of a raw filesystem walk that false-positived on orphaned files. - drop `as string` cast in inlineSubCompositions in favor of an explicit null guard (per CLAUDE.md). Review-comment items: - move EmptyCompositionError JSDoc above the class (was above the adapter fn). - correct stale circular-ref comment to match actual silent-skip behavior. - rewrite self-contradicting lint message ("silently drop") to describe the new loud render-pre-flight abort. - add the __PLACEHOLDER__ (/^__[A-Z_]+__$/) skip to the render pre-flight so it agrees with lint. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
James Russo
co-authored by
Claude Opus 4.8
Claude Opus 4.8
parent
9c4d9e50a0
commit
cf573f7f3f
@@ -492,31 +492,160 @@ describe("detectRenderModeHints", () => {
|
||||
}
|
||||
});
|
||||
|
||||
it("compileForRender skips empty sub-composition files instead of aborting", async () => {
|
||||
const projectDir = mkdtempSync(join(tmpdir(), "hf-empty-subcomp-"));
|
||||
// Shared fixture builder for the assertSubCompositionsUsable / EmptyCompositionError
|
||||
// pre-flight tests below. `subCompFiles` is a map of compositions/-relative
|
||||
// filename to raw file content (empty string / malformed text / valid HTML).
|
||||
// `hosts` is the data-composition-src host markup injected into the root
|
||||
// composition's timeline div, in order, at 1s each.
|
||||
function makeSubCompProject(
|
||||
dirPrefix: string,
|
||||
hosts: Array<{ id: string; src: string }>,
|
||||
subCompFiles: Record<string, string>,
|
||||
): string {
|
||||
const projectDir = mkdtempSync(join(tmpdir(), dirPrefix));
|
||||
const compositionsDir = join(projectDir, "compositions");
|
||||
mkdirSync(compositionsDir, { recursive: true });
|
||||
const hostMarkup = hosts
|
||||
.map(
|
||||
(h, i) =>
|
||||
`<div data-composition-id="${h.id}" data-composition-src="${h.src}" data-start="${i}" data-duration="1"></div>`,
|
||||
)
|
||||
.join("\n ");
|
||||
writeFileSync(
|
||||
join(projectDir, "index.html"),
|
||||
`<!DOCTYPE html>
|
||||
<html>
|
||||
<head></head>
|
||||
<body>
|
||||
<div data-composition-id="main" data-width="100" data-height="100" data-start="0" data-duration="1">
|
||||
<div data-composition-id="intro" data-composition-src="compositions/intro.html" data-start="0" data-duration="1"></div>
|
||||
<div data-composition-id="main" data-width="100" data-height="100" data-start="0" data-duration="${hosts.length}">
|
||||
${hostMarkup}
|
||||
</div>
|
||||
<script>
|
||||
window.__timelines = window.__timelines || {};
|
||||
window.__timelines.main = { duration: function() { return 1; } };
|
||||
window.__timelines.main = { duration: function() { return ${hosts.length}; } };
|
||||
</script>
|
||||
</body>
|
||||
</html>`,
|
||||
);
|
||||
writeFileSync(join(compositionsDir, "intro.html"), "");
|
||||
for (const [name, content] of Object.entries(subCompFiles)) {
|
||||
writeFileSync(join(compositionsDir, name), content);
|
||||
}
|
||||
return projectDir;
|
||||
}
|
||||
|
||||
function validSubCompHtml(compId: string, label: string, nestedSrc?: string): string {
|
||||
const inner = nestedSrc
|
||||
? `<div data-composition-id="${label}" data-composition-src="${nestedSrc}" data-start="0" data-duration="1"></div>`
|
||||
: `<div class="title">${label}</div>`;
|
||||
return `<!doctype html><html><body>
|
||||
<div data-composition-id="${compId}" data-width="100" data-height="100">
|
||||
${inner}
|
||||
</div>
|
||||
</body></html>`;
|
||||
}
|
||||
|
||||
it("compileForRender aborts with EmptyCompositionError when a sub-composition file is empty", async () => {
|
||||
// The shared inliner (inlineSubCompositions.ts, packages/core) stays
|
||||
// tolerant of empty/unparsable sub-compositions — it skips the scene and
|
||||
// keeps going, silently, so preview/studio can keep iterating on a
|
||||
// partially-authored project. That tolerance is intentional and tested
|
||||
// separately in packages/core/src/compiler/inlineSubCompositions.test.ts.
|
||||
//
|
||||
// But a *render* that silently drops a scene produces a materially
|
||||
// broken video with no visible error — worse than refusing to render.
|
||||
// compileForRender (render-only) runs a pre-flight check
|
||||
// (assertSubCompositionsUsable, using the same checkSubCompositionUsability
|
||||
// helper the inliner and hyperframes lint use) before any compilation
|
||||
// work starts, and aborts immediately instead of silently producing a
|
||||
// broken render 45+ seconds later.
|
||||
const projectDir = makeSubCompProject(
|
||||
"hf-empty-subcomp-",
|
||||
[{ id: "intro", src: "compositions/intro.html" }],
|
||||
{ "intro.html": "" },
|
||||
);
|
||||
|
||||
await expect(
|
||||
compileForRender(projectDir, join(projectDir, "index.html"), projectDir),
|
||||
).rejects.toThrow(/compositions\/intro\.html/);
|
||||
});
|
||||
|
||||
it("compileForRender aborts naming every unusable sub-composition at once", async () => {
|
||||
const projectDir = makeSubCompProject(
|
||||
"hf-empty-subcomp-multi-",
|
||||
[
|
||||
{ id: "intro", src: "compositions/intro.html" },
|
||||
{ id: "outro", src: "compositions/outro.html" },
|
||||
],
|
||||
{ "intro.html": "", "outro.html": "not valid html at all, just text" },
|
||||
);
|
||||
|
||||
await expect(
|
||||
compileForRender(projectDir, join(projectDir, "index.html"), projectDir),
|
||||
).rejects.toThrow(/compositions\/intro\.html[\s\S]*compositions\/outro\.html/);
|
||||
});
|
||||
|
||||
it("compileForRender aborts when a data-composition-src reference points at a missing file", async () => {
|
||||
const projectDir = makeSubCompProject(
|
||||
"hf-missing-subcomp-",
|
||||
[{ id: "intro", src: "compositions/does-not-exist.html" }],
|
||||
{},
|
||||
);
|
||||
|
||||
await expect(
|
||||
compileForRender(projectDir, join(projectDir, "index.html"), projectDir),
|
||||
).rejects.toThrow(/compositions\/does-not-exist\.html/);
|
||||
});
|
||||
|
||||
it("compileForRender succeeds when the sub-composition file is valid (happy path)", async () => {
|
||||
const projectDir = makeSubCompProject(
|
||||
"hf-valid-subcomp-",
|
||||
[{ id: "intro", src: "compositions/intro.html" }],
|
||||
{ "intro.html": validSubCompHtml("intro", "Hello") },
|
||||
);
|
||||
|
||||
const result = await compileForRender(projectDir, join(projectDir, "index.html"), projectDir);
|
||||
expect(result.html).toContain("data-composition-id");
|
||||
});
|
||||
|
||||
it("compileForRender succeeds when a valid sub-composition itself references a nested valid sub-composition", async () => {
|
||||
// Regression guard: data-composition-src is always root-relative, even
|
||||
// from within a nested sub-composition (matches parseSubCompositions,
|
||||
// which threads the original projectDir unchanged through every
|
||||
// recursion level — never dirname(parentFile)). The pre-flight check
|
||||
// must resolve nested references the same way, or it aborts renders
|
||||
// that would have actually succeeded (false-positive abort).
|
||||
//
|
||||
// parent.html lives in compositions/ and references child.html using the
|
||||
// same root-relative "compositions/..." form — not "./child.html".
|
||||
const projectDir = makeSubCompProject(
|
||||
"hf-nested-subcomp-valid-",
|
||||
[{ id: "parent", src: "compositions/parent.html" }],
|
||||
{
|
||||
"parent.html": validSubCompHtml("parent", "child", "compositions/child.html"),
|
||||
"child.html": validSubCompHtml("child", "Nested Hello"),
|
||||
},
|
||||
);
|
||||
|
||||
const result = await compileForRender(projectDir, join(projectDir, "index.html"), projectDir);
|
||||
expect(result.html).toContain("data-composition-id");
|
||||
});
|
||||
|
||||
it("compileForRender aborts naming a broken nested (grandchild) sub-composition", async () => {
|
||||
// child.html is empty — the grandchild scene, referenced root-relative
|
||||
// from parent.html which itself lives in compositions/.
|
||||
const projectDir = makeSubCompProject(
|
||||
"hf-nested-subcomp-broken-",
|
||||
[{ id: "parent", src: "compositions/parent.html" }],
|
||||
{
|
||||
"parent.html": validSubCompHtml("parent", "child", "compositions/child.html"),
|
||||
"child.html": "",
|
||||
},
|
||||
);
|
||||
|
||||
await expect(
|
||||
compileForRender(projectDir, join(projectDir, "index.html"), projectDir),
|
||||
).rejects.toThrow(/compositions\/child\.html/);
|
||||
});
|
||||
});
|
||||
|
||||
describe("detectShaderTransitionUsage", () => {
|
||||
|
||||
@@ -24,6 +24,10 @@ import {
|
||||
type UnresolvedElement,
|
||||
} from "@hyperframes/core";
|
||||
import { inlineSubCompositions as inlineSubCompositionsShared } from "@hyperframes/core/compiler";
|
||||
import {
|
||||
checkSubCompositionUsability,
|
||||
type ParsableDocumentLike,
|
||||
} from "@hyperframes/parsers/sub-composition-validity";
|
||||
import { extractMediaMetadata, extractAudioMetadata } from "../utils/ffprobe.js";
|
||||
import { isPathInside, toExternalAssetKey } from "../utils/paths.js";
|
||||
import {
|
||||
@@ -59,6 +63,131 @@ export interface CompiledComposition {
|
||||
hasShaderTransitions: boolean;
|
||||
}
|
||||
|
||||
/** Adapts linkedom's `parseHTML` to the `checkSubCompositionUsability` contract. */
|
||||
function parseSubCompHtmlForValidity(html: string): ParsableDocumentLike {
|
||||
return parseHTML(html).document as unknown as ParsableDocumentLike;
|
||||
}
|
||||
|
||||
/**
|
||||
* Thrown by {@link assertSubCompositionsUsable} when one or more
|
||||
* `data-composition-src` references resolve to a missing, empty, or
|
||||
* unparsable file. This is the render-path enforcement of the #1 render
|
||||
* failure bucket in production telemetry: a scene-authoring step (most
|
||||
* commonly an AI agent) writes the `data-composition-src` reference before,
|
||||
* or without ever, writing valid content into the scene file.
|
||||
*
|
||||
* Unlike the tolerant inliner (`packages/core/src/compiler/inlineSubCompositions.ts`,
|
||||
* intentionally kept lenient for preview/studio so mid-authoring iteration
|
||||
* doesn't break bundling), a render that silently drops a scene produces a
|
||||
* materially broken video with no visible error — strictly worse than
|
||||
* refusing to render. This check runs before any compilation work starts so
|
||||
* the failure is immediate and names every offending file at once, instead
|
||||
* of surfacing 45+ seconds later as a `pollSubCompositionTimelines` timeout
|
||||
* or a raw `Cannot destructure property 'firstElementChild' of
|
||||
* 'documentElement' as it is null` crash deep inside linkedom.
|
||||
*
|
||||
* Not exported — nothing needs `instanceof` narrowing on this today. Callers
|
||||
* catch it generically (`catch (err: unknown)`, matching on `.message`) the
|
||||
* same way they handle every other compile-time failure. Kept as a class
|
||||
* (not a plain `throw new Error(...)`) so the aggregated multi-file message
|
||||
* construction has a single, testable home.
|
||||
*/
|
||||
class EmptyCompositionError extends Error {
|
||||
readonly code = "EMPTY_COMPOSITION" as const;
|
||||
readonly problems: ReadonlyArray<{ srcPath: string; detail: string }>;
|
||||
|
||||
constructor(problems: ReadonlyArray<{ srcPath: string; detail: string }>) {
|
||||
const lines = problems.map((p) => ` - ${p.srcPath}: ${p.detail}`);
|
||||
super(
|
||||
`${problems.length} composition file${problems.length === 1 ? "" : "s"} referenced by ` +
|
||||
`data-composition-src cannot be rendered:\n${lines.join("\n")}\n\n` +
|
||||
"Check that each file referenced by data-composition-src contains valid HTML with a " +
|
||||
"<template> or <body> containing a [data-composition-id] element. If a scene-authoring " +
|
||||
"step is still running, wait for it to finish before referencing the file.",
|
||||
);
|
||||
this.name = "EmptyCompositionError";
|
||||
this.problems = problems;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Recursively walk every `data-composition-src` reference reachable from
|
||||
* `html` (including nested sub-compositions) and verify each resolves to a
|
||||
* usable file — exists, non-empty, parses to HTML with renderable content.
|
||||
* Uses the same `checkSubCompositionUsability` helper the tolerant inliner
|
||||
* and `hyperframes lint` use, so all three agree on what counts as usable.
|
||||
*
|
||||
* Throws {@link EmptyCompositionError} naming every offending file at once
|
||||
* (not just the first one hit) if any reference is unusable. Call this
|
||||
* before any compilation work starts — it deliberately duplicates a small
|
||||
* amount of file-reading work that `parseSubCompositions` also does, in
|
||||
* exchange for failing in milliseconds instead of after the browser has
|
||||
* already launched and waited out a capture timeout.
|
||||
*/
|
||||
// fallow-ignore-next-line complexity
|
||||
function assertSubCompositionsUsable(
|
||||
html: string,
|
||||
projectDir: string,
|
||||
visited: Set<string> = new Set(),
|
||||
): void {
|
||||
const { document } = parseHTML(html);
|
||||
const hosts = [...document.querySelectorAll("[data-composition-src]")];
|
||||
const problems: Array<{ srcPath: string; detail: string }> = [];
|
||||
|
||||
for (const el of hosts) {
|
||||
const srcPath = el.getAttribute("data-composition-src");
|
||||
if (!srcPath) continue;
|
||||
if (/^__[A-Z_]+__$/.test(srcPath)) continue; // template placeholder, not a real reference — matches lint's skip
|
||||
|
||||
const filePath = resolve(projectDir, srcPath);
|
||||
// Circular reference guard. parseSubCompositions (below) silently
|
||||
// `continue`s on a repeat visit with no reporting at all — mirror that
|
||||
// silence here rather than pretend it surfaces an error somewhere else.
|
||||
if (visited.has(filePath)) continue;
|
||||
|
||||
if (!existsSync(filePath)) {
|
||||
problems.push({ srcPath, detail: "the file does not exist" });
|
||||
continue;
|
||||
}
|
||||
|
||||
const fileHtml = readFileSync(filePath, "utf-8");
|
||||
const validity = checkSubCompositionUsability(fileHtml, parseSubCompHtmlForValidity);
|
||||
if (!validity.ok) {
|
||||
problems.push({
|
||||
srcPath,
|
||||
detail: validity.detail ?? "the file is empty or could not be parsed",
|
||||
});
|
||||
continue;
|
||||
}
|
||||
|
||||
// Recurse into nested sub-compositions so a broken scene three levels
|
||||
// deep is still named directly instead of surfacing as a parent-level
|
||||
// "no error, just missing content" mystery.
|
||||
//
|
||||
// Pass `projectDir` unchanged (not dirname(filePath)) — data-composition-src
|
||||
// is always resolved root-relative, even from within a nested
|
||||
// sub-composition. This must match parseSubCompositions' own recursive
|
||||
// call below exactly (it threads the original projectDir through every
|
||||
// level too), or this pre-flight check resolves nested references to the
|
||||
// wrong path and aborts renders that would have actually succeeded.
|
||||
const nestedVisited = new Set(visited);
|
||||
nestedVisited.add(filePath);
|
||||
try {
|
||||
assertSubCompositionsUsable(fileHtml, projectDir, nestedVisited);
|
||||
} catch (err) {
|
||||
if (err instanceof EmptyCompositionError) {
|
||||
problems.push(...err.problems);
|
||||
} else {
|
||||
throw err;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
if (problems.length > 0) {
|
||||
throw new EmptyCompositionError(problems);
|
||||
}
|
||||
}
|
||||
|
||||
export type RenderModeHintCode = "iframe" | "requestAnimationFrame" | "htmlInCanvas";
|
||||
|
||||
export interface RenderModeHint {
|
||||
@@ -617,8 +746,14 @@ function inlineSubCompositions(
|
||||
parseHtml: (htmlStr: string) => parseHTML(htmlStr).document as unknown as Document,
|
||||
scriptErrorLabel: "[Compiler] Composition script failed",
|
||||
compoundAuthoredRoot: true,
|
||||
onMissingComposition: (srcPath: string) => {
|
||||
console.warn(`[Compiler] Composition file missing or empty: ${srcPath}`);
|
||||
onMissingComposition: (srcPath: string, reason?: string) => {
|
||||
// In the render path this is normally unreachable — compileForRender
|
||||
// calls assertSubCompositionsUsable() before any of this runs, so a
|
||||
// hit here means the file changed on disk between that pre-flight
|
||||
// check and this later inline step (e.g. a concurrent scene-writer).
|
||||
console.warn(
|
||||
`[Compiler] Skipping sub-composition "${srcPath}": ${reason ?? "the file is missing or empty"}.`,
|
||||
);
|
||||
},
|
||||
},
|
||||
);
|
||||
@@ -1367,6 +1502,15 @@ export async function compileForRender(
|
||||
options: CompileForRenderOptions = {},
|
||||
): Promise<CompiledComposition> {
|
||||
const rawHtml = rewriteUnresolvableGsapToCdn(readFileSync(htmlPath, "utf-8"), projectDir);
|
||||
|
||||
// Pre-flight: every data-composition-src reference must resolve to a
|
||||
// usable file before we spend any time compiling, launching a browser, or
|
||||
// waiting out a capture timeout. See EmptyCompositionError for why this is
|
||||
// unconditional (not gated behind --strict like lint warnings) — a render
|
||||
// that silently drops a scene is strictly worse than one that refuses to
|
||||
// start.
|
||||
assertSubCompositionsUsable(rawHtml, projectDir);
|
||||
|
||||
const { html: compiledHtml, unresolvedCompositions } = await compileHtmlFile(
|
||||
rawHtml,
|
||||
projectDir,
|
||||
|
||||
Reference in New Issue
Block a user