mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-09-03 04:38:33 +00:00
fix: add media rendering guardrails to prevent silent failures (#112)
## Summary - **Lint rules** catch media elements missing `id` (renderer silently skips them), missing `src`, `preload="none"` (blocks renderer), and video nested in timed divs (freezes playback). Upgraded `video_nested_in_timed_element` from warning to error. - **Compiler** strips `preload="none"` from media during compilation. Runs parallel, cached keyframe interval analysis via ffprobe — warns on sparse keyframes (>2s) that cause seek failures and audio/video desync. Suggested ffmpeg command preserves audio (`-c:a copy`). - **Pre-render lint** lints `index.html` + all `compositions/*.html` sub-compositions before render via shared `lintProject()` helper. Warns by default; `--strict` blocks on errors, `--strict-all` blocks on errors + warnings. - **Render orchestrator** logs a hint to retry with `--workers 1` when parallel capture times out on video-heavy compositions. - **Refactor**: extracted `runFfprobe()` + `parseProbeJson()` helpers to deduplicate ~80 lines of spawn boilerplate across 3 ffprobe functions. Extracted `shouldBlockRender()` so strict flag tests exercise production code. Shared `lintProject()` used by both `lint` and `render` commands. ## Context Discovered during a real composition build session where: 1. `<audio>` without `id` rendered silently (preview worked fine because runtime queries `[data-start]`, but renderer queries `[id][src]`) 2. `<video>` inside timed `<div>` froze on first frame 3. `preload="none"` caused 45s renderer timeout 4. YouTube clips with sparse keyframes from `yt-dlp --download-sections` caused audio/video desync 5. Parallel workers timed out on video-heavy compositions ## Test plan - [x] Core: 365/365 tests passing (5 new lint tests) - [x] Engine: 24/24 tests passing - [x] CLI: 14/14 tests passing (7 lintProject + 7 shouldBlockRender) - [x] Lint + format hooks pass - [ ] Manual: create a composition with `<audio data-start="0" src="test.wav">` (no id) — verify `npx hyperframes lint` catches it - [ ] Manual: run `npx hyperframes render --strict` with lint errors — verify it blocks - [ ] Manual: run `npx hyperframes render --strict-all` with lint warnings — verify it blocks
This commit is contained in:
Vance Ingalls
@@ -7,6 +7,8 @@ import { createRequire } from "node:module";
|
||||
import * as clack from "@clack/prompts";
|
||||
import { c } from "../ui/colors.js";
|
||||
import { isDevMode } from "../utils/env.js";
|
||||
import { lintProject } from "../utils/lintProject.js";
|
||||
import { formatLintFindings } from "../utils/lintFormat.js";
|
||||
|
||||
/**
|
||||
* Try to start a server on the given port, auto-incrementing up to maxAttempts
|
||||
@@ -71,6 +73,19 @@ export default defineCommand({
|
||||
const isImplicitCwd = !rawArg || rawArg === "." || rawArg === "./";
|
||||
const projectName = isImplicitCwd ? basename(process.env.PWD ?? dir) : basename(dir);
|
||||
|
||||
// Lint before starting — surface issues for the agent to fix.
|
||||
// dev.ts doesn't use resolveProject() because it needs to proceed even without index.html.
|
||||
const indexPath = join(dir, "index.html");
|
||||
if (existsSync(indexPath)) {
|
||||
const project = { dir, name: projectName, indexPath };
|
||||
const lintResult = lintProject(project);
|
||||
if (lintResult.totalErrors > 0 || lintResult.totalWarnings > 0) {
|
||||
console.log();
|
||||
for (const line of formatLintFindings(lintResult)) console.log(line);
|
||||
console.log();
|
||||
}
|
||||
}
|
||||
|
||||
if (isDevMode()) {
|
||||
return runDevMode(dir, projectName);
|
||||
}
|
||||
|
||||
@@ -1,11 +1,8 @@
|
||||
import { defineCommand } from "citty";
|
||||
import { readFileSync } from "node:fs";
|
||||
import { join } from "node:path";
|
||||
import { lintHyperframeHtml } from "@hyperframes/core/lint";
|
||||
import type { HyperframeLintFinding } from "@hyperframes/core/lint";
|
||||
import { walkDir } from "@hyperframes/core/studio-api";
|
||||
import { c } from "../ui/colors.js";
|
||||
import { resolveProject } from "../utils/project.js";
|
||||
import { lintProject } from "../utils/lintProject.js";
|
||||
import { formatLintFindings } from "../utils/lintFormat.js";
|
||||
import { withMeta } from "../utils/updateCheck.js";
|
||||
|
||||
export default defineCommand({
|
||||
@@ -16,62 +13,32 @@ export default defineCommand({
|
||||
},
|
||||
async run({ args }) {
|
||||
const project = resolveProject(args.dir);
|
||||
const htmlFiles = walkDir(project.dir).filter((f) => f.endsWith(".html"));
|
||||
|
||||
const allFindings: (HyperframeLintFinding & { file: string })[] = [];
|
||||
let totalErrors = 0;
|
||||
let totalWarnings = 0;
|
||||
|
||||
for (const file of htmlFiles) {
|
||||
const html = readFileSync(join(project.dir, file), "utf-8");
|
||||
const result = lintHyperframeHtml(html, { filePath: file });
|
||||
for (const f of result.findings) {
|
||||
allFindings.push({ ...f, file });
|
||||
}
|
||||
totalErrors += result.errorCount;
|
||||
totalWarnings += result.warningCount;
|
||||
}
|
||||
const lintResult = lintProject(project);
|
||||
|
||||
if (args.json) {
|
||||
console.log(
|
||||
JSON.stringify(
|
||||
withMeta({
|
||||
ok: totalErrors === 0,
|
||||
findings: allFindings,
|
||||
errorCount: totalErrors,
|
||||
warningCount: totalWarnings,
|
||||
filesScanned: htmlFiles.length,
|
||||
}),
|
||||
null,
|
||||
2,
|
||||
),
|
||||
);
|
||||
process.exit(totalErrors > 0 ? 1 : 0);
|
||||
const combined = {
|
||||
ok: lintResult.totalErrors === 0,
|
||||
errorCount: lintResult.totalErrors,
|
||||
warningCount: lintResult.totalWarnings,
|
||||
findings: lintResult.results.flatMap((r) => r.result.findings),
|
||||
};
|
||||
console.log(JSON.stringify(withMeta(combined), null, 2));
|
||||
process.exit(combined.ok ? 0 : 1);
|
||||
}
|
||||
|
||||
console.log(
|
||||
`${c.accent("◆")} Linting ${c.accent(project.name)} (${htmlFiles.length} HTML files)`,
|
||||
);
|
||||
const fileCount = lintResult.results.length;
|
||||
const fileLabel = fileCount === 1 ? lintResult.results[0]!.file : `${fileCount} files`;
|
||||
console.log(`${c.accent("◆")} Linting ${c.accent(project.name + "/" + fileLabel)}`);
|
||||
console.log();
|
||||
|
||||
if (allFindings.length === 0) {
|
||||
if (lintResult.totalErrors === 0 && lintResult.totalWarnings === 0) {
|
||||
console.log(`${c.success("◇")} ${c.success("0 errors, 0 warnings")}`);
|
||||
return;
|
||||
}
|
||||
|
||||
for (const finding of allFindings) {
|
||||
const prefix = finding.severity === "error" ? c.error("✗") : c.warn("⚠");
|
||||
const loc = finding.elementId ? ` ${c.accent(`[${finding.elementId}]`)}` : "";
|
||||
console.log(
|
||||
`${prefix} ${c.bold(finding.code)}${loc}: ${finding.message} ${c.dim(finding.file)}`,
|
||||
);
|
||||
if (finding.fixHint) {
|
||||
console.log(` ${c.dim(`Fix: ${finding.fixHint}`)}`);
|
||||
}
|
||||
}
|
||||
const lines = formatLintFindings(lintResult, { showElementId: true, showSummary: true });
|
||||
for (const line of lines) console.log(line);
|
||||
|
||||
const summaryIcon = totalErrors > 0 ? c.error("◇") : c.success("◇");
|
||||
console.log(`\n${summaryIcon} ${totalErrors} error(s), ${totalWarnings} warning(s)`);
|
||||
process.exit(totalErrors > 0 ? 1 : 0);
|
||||
process.exit(lintResult.totalErrors > 0 ? 1 : 0);
|
||||
},
|
||||
});
|
||||
|
||||
@@ -3,6 +3,8 @@ import { existsSync, mkdirSync, statSync } from "node:fs";
|
||||
import { cpus, freemem } from "node:os";
|
||||
import { resolve, dirname, join } from "node:path";
|
||||
import { resolveProject } from "../utils/project.js";
|
||||
import { lintProject, shouldBlockRender } from "../utils/lintProject.js";
|
||||
import { formatLintFindings } from "../utils/lintFormat.js";
|
||||
import { loadProducer } from "../utils/producer.js";
|
||||
import { c } from "../ui/colors.js";
|
||||
import { formatBytes, formatDuration, errorBox } from "../ui/format.js";
|
||||
@@ -75,6 +77,16 @@ Examples:
|
||||
description: "Suppress verbose output",
|
||||
default: false,
|
||||
},
|
||||
strict: {
|
||||
type: "boolean",
|
||||
description: "Fail render on lint errors",
|
||||
default: false,
|
||||
},
|
||||
"strict-all": {
|
||||
type: "boolean",
|
||||
description: "Fail render on lint errors AND warnings",
|
||||
default: false,
|
||||
},
|
||||
},
|
||||
async run({ args }) {
|
||||
// ── Resolve project ────────────────────────────────────────────────────
|
||||
@@ -131,6 +143,8 @@ Examples:
|
||||
const useDocker = args.docker ?? false;
|
||||
const useGpu = args.gpu ?? false;
|
||||
const quiet = args.quiet ?? false;
|
||||
const strictAll = args["strict-all"] ?? false;
|
||||
const strictErrors = (args.strict ?? false) || strictAll;
|
||||
|
||||
// ── Print render plan ─────────────────────────────────────────────────
|
||||
const workerCount = workers ?? defaultWorkerCount();
|
||||
@@ -193,6 +207,31 @@ Examples:
|
||||
}
|
||||
}
|
||||
|
||||
// ── Pre-render lint ──────────────────────────────────────────────────
|
||||
{
|
||||
const lintResult = lintProject(project);
|
||||
if (!quiet && (lintResult.totalErrors > 0 || lintResult.totalWarnings > 0)) {
|
||||
console.log("");
|
||||
for (const line of formatLintFindings(lintResult, { errorsFirst: true })) console.log(line);
|
||||
if (
|
||||
shouldBlockRender(
|
||||
strictErrors,
|
||||
strictAll,
|
||||
lintResult.totalErrors,
|
||||
lintResult.totalWarnings,
|
||||
)
|
||||
) {
|
||||
const mode = strictAll ? "--strict-all" : "--strict";
|
||||
console.log("");
|
||||
console.log(c.error(` Aborting render due to lint issues (${mode} mode).`));
|
||||
console.log("");
|
||||
process.exit(1);
|
||||
}
|
||||
console.log(c.dim(" Continuing render despite lint issues. Use --strict to block."));
|
||||
console.log("");
|
||||
}
|
||||
}
|
||||
|
||||
// ── Render ────────────────────────────────────────────────────────────
|
||||
if (useDocker) {
|
||||
await renderDocker(project.dir, outputPath, {
|
||||
|
||||
@@ -0,0 +1,51 @@
|
||||
import { c } from "../ui/colors.js";
|
||||
import type { ProjectLintResult } from "./lintProject.js";
|
||||
|
||||
export interface LintFormatOptions {
|
||||
/** Show elementId in brackets after the code (default: true) */
|
||||
showElementId?: boolean;
|
||||
/** Show summary line with error/warning counts (default: false) */
|
||||
showSummary?: boolean;
|
||||
/** Group errors before warnings per file (default: false — interleaved) */
|
||||
errorsFirst?: boolean;
|
||||
}
|
||||
|
||||
/**
|
||||
* Format lint findings for console output. Used by lint, render, and dev commands.
|
||||
*/
|
||||
export function formatLintFindings(
|
||||
{ results, totalErrors, totalWarnings }: ProjectLintResult,
|
||||
options: LintFormatOptions = {},
|
||||
): string[] {
|
||||
const { showElementId = true, showSummary = false, errorsFirst = false } = options;
|
||||
const lines: string[] = [];
|
||||
const multiFile = results.length > 1;
|
||||
|
||||
for (const { file, result } of results) {
|
||||
if (result.findings.length === 0) continue;
|
||||
|
||||
const format = (finding: (typeof result.findings)[0]) => {
|
||||
const prefix = finding.severity === "error" ? c.error("✗") : c.warn("⚠");
|
||||
const fileLabel = multiFile ? c.dim(`[${file}] `) : "";
|
||||
const loc =
|
||||
showElementId && finding.elementId ? ` ${c.accent(`[${finding.elementId}]`)}` : "";
|
||||
lines.push(` ${prefix} ${fileLabel}${c.bold(finding.code)}${loc}: ${finding.message}`);
|
||||
if (finding.fixHint) lines.push(` ${c.dim(`Fix: ${finding.fixHint}`)}`);
|
||||
};
|
||||
|
||||
if (errorsFirst) {
|
||||
for (const f of result.findings) if (f.severity === "error") format(f);
|
||||
for (const f of result.findings) if (f.severity === "warning") format(f);
|
||||
} else {
|
||||
for (const f of result.findings) format(f);
|
||||
}
|
||||
}
|
||||
|
||||
if (showSummary) {
|
||||
const icon = totalErrors > 0 ? c.error("◇") : c.success("◇");
|
||||
lines.push("");
|
||||
lines.push(`${icon} ${totalErrors} error(s), ${totalWarnings} warning(s)`);
|
||||
}
|
||||
|
||||
return lines;
|
||||
}
|
||||
@@ -0,0 +1,169 @@
|
||||
import { describe, it, expect, afterEach } from "vitest";
|
||||
import { mkdirSync, writeFileSync, rmSync } from "node:fs";
|
||||
import { join } from "node:path";
|
||||
import { tmpdir } from "node:os";
|
||||
import { lintProject, shouldBlockRender } from "./lintProject.js";
|
||||
import type { ProjectDir } from "./project.js";
|
||||
|
||||
function tmpProject(name: string): string {
|
||||
const dir = join(tmpdir(), `hf-test-${name}-${Date.now()}`);
|
||||
mkdirSync(dir, { recursive: true });
|
||||
return dir;
|
||||
}
|
||||
|
||||
function validHtml(compId = "main"): string {
|
||||
return `<html><body>
|
||||
<div data-composition-id="${compId}" data-width="1920" data-height="1080"></div>
|
||||
<script>window.__timelines = window.__timelines || {}; window.__timelines["${compId}"] = gsap.timeline({ paused: true });</script>
|
||||
</body></html>`;
|
||||
}
|
||||
|
||||
function htmlWithMissingMediaId(): string {
|
||||
return `<html><body>
|
||||
<div data-composition-id="main" data-width="1920" data-height="1080">
|
||||
<audio data-start="0" data-duration="10" src="narration.wav"></audio>
|
||||
</div>
|
||||
<script>window.__timelines = window.__timelines || {}; window.__timelines["main"] = gsap.timeline({ paused: true });</script>
|
||||
</body></html>`;
|
||||
}
|
||||
|
||||
function htmlWithPreloadNone(): string {
|
||||
return `<html><body>
|
||||
<div data-composition-id="captions" data-width="1920" data-height="1080">
|
||||
<video id="v1" data-start="0" data-duration="10" src="clip.mp4" muted playsinline preload="none"></video>
|
||||
</div>
|
||||
<script>window.__timelines = window.__timelines || {}; window.__timelines["captions"] = gsap.timeline({ paused: true });</script>
|
||||
</body></html>`;
|
||||
}
|
||||
|
||||
let dirs: string[] = [];
|
||||
|
||||
function makeProject(indexHtml: string, subComps?: Record<string, string>): ProjectDir {
|
||||
const dir = tmpProject("lint");
|
||||
dirs.push(dir);
|
||||
writeFileSync(join(dir, "index.html"), indexHtml);
|
||||
if (subComps) {
|
||||
const compsDir = join(dir, "compositions");
|
||||
mkdirSync(compsDir, { recursive: true });
|
||||
for (const [name, html] of Object.entries(subComps)) {
|
||||
writeFileSync(join(compsDir, name), html);
|
||||
}
|
||||
}
|
||||
return { dir, name: "test-project", indexPath: join(dir, "index.html") };
|
||||
}
|
||||
|
||||
afterEach(() => {
|
||||
for (const d of dirs) {
|
||||
rmSync(d, { recursive: true, force: true });
|
||||
}
|
||||
dirs = [];
|
||||
});
|
||||
|
||||
describe("lintProject", () => {
|
||||
it("returns zero errors/warnings for a clean project", () => {
|
||||
const project = makeProject(validHtml());
|
||||
const { totalErrors, totalWarnings, results } = lintProject(project);
|
||||
|
||||
expect(totalErrors).toBe(0);
|
||||
expect(totalWarnings).toBe(0);
|
||||
expect(results).toHaveLength(1);
|
||||
expect(results[0]!.file).toBe("index.html");
|
||||
});
|
||||
|
||||
it("detects errors in index.html", () => {
|
||||
const project = makeProject(htmlWithMissingMediaId());
|
||||
const { totalErrors, results } = lintProject(project);
|
||||
|
||||
expect(totalErrors).toBeGreaterThan(0);
|
||||
const mediaFinding = results[0]!.result.findings.find((f) => f.code === "media_missing_id");
|
||||
expect(mediaFinding).toBeDefined();
|
||||
});
|
||||
|
||||
it("lints sub-compositions in compositions/ directory", () => {
|
||||
const project = makeProject(validHtml(), {
|
||||
"captions.html": htmlWithMissingMediaId(),
|
||||
});
|
||||
const { totalErrors, results } = lintProject(project);
|
||||
|
||||
expect(results).toHaveLength(2);
|
||||
expect(results[1]!.file).toBe("compositions/captions.html");
|
||||
expect(totalErrors).toBeGreaterThan(0);
|
||||
const subFindings = results[1]!.result.findings;
|
||||
expect(subFindings.some((f) => f.code === "media_missing_id")).toBe(true);
|
||||
});
|
||||
|
||||
it("aggregates errors across index.html and sub-compositions", () => {
|
||||
const project = makeProject(htmlWithMissingMediaId(), {
|
||||
"overlay.html": htmlWithMissingMediaId(),
|
||||
});
|
||||
const { totalErrors, results } = lintProject(project);
|
||||
|
||||
expect(results).toHaveLength(2);
|
||||
// Both files have media_missing_id errors
|
||||
const rootErrors = results[0]!.result.errorCount;
|
||||
const subErrors = results[1]!.result.errorCount;
|
||||
expect(totalErrors).toBe(rootErrors + subErrors);
|
||||
});
|
||||
|
||||
it("aggregates warnings from sub-compositions", () => {
|
||||
const project = makeProject(validHtml(), {
|
||||
"captions.html": htmlWithPreloadNone(),
|
||||
});
|
||||
const { totalWarnings, results } = lintProject(project);
|
||||
|
||||
expect(results).toHaveLength(2);
|
||||
expect(totalWarnings).toBeGreaterThan(0);
|
||||
const preloadWarning = results[1]!.result.findings.find((f) => f.code === "media_preload_none");
|
||||
expect(preloadWarning).toBeDefined();
|
||||
});
|
||||
|
||||
it("handles project with no compositions/ directory", () => {
|
||||
const project = makeProject(validHtml());
|
||||
// No compositions/ dir created
|
||||
const { results } = lintProject(project);
|
||||
|
||||
expect(results).toHaveLength(1);
|
||||
});
|
||||
|
||||
it("ignores non-HTML files in compositions/", () => {
|
||||
const project = makeProject(validHtml(), {
|
||||
"captions.html": validHtml("captions"),
|
||||
});
|
||||
// Add a non-HTML file
|
||||
writeFileSync(join(project.dir, "compositions", "readme.txt"), "not html");
|
||||
|
||||
const { results } = lintProject(project);
|
||||
|
||||
expect(results).toHaveLength(2); // index.html + captions.html, not readme.txt
|
||||
});
|
||||
});
|
||||
|
||||
describe("shouldBlockRender", () => {
|
||||
it("default: does not block on errors", () => {
|
||||
expect(shouldBlockRender(false, false, 5, 0)).toBe(false);
|
||||
});
|
||||
|
||||
it("default: does not block on warnings", () => {
|
||||
expect(shouldBlockRender(false, false, 0, 3)).toBe(false);
|
||||
});
|
||||
|
||||
it("--strict: blocks on errors", () => {
|
||||
expect(shouldBlockRender(true, false, 1, 0)).toBe(true);
|
||||
});
|
||||
|
||||
it("--strict: does not block on warnings only", () => {
|
||||
expect(shouldBlockRender(true, false, 0, 5)).toBe(false);
|
||||
});
|
||||
|
||||
it("--strict-all: blocks on errors", () => {
|
||||
expect(shouldBlockRender(true, true, 1, 0)).toBe(true);
|
||||
});
|
||||
|
||||
it("--strict-all: blocks on warnings", () => {
|
||||
expect(shouldBlockRender(true, true, 0, 1)).toBe(true);
|
||||
});
|
||||
|
||||
it("--strict-all: does not block when clean", () => {
|
||||
expect(shouldBlockRender(true, true, 0, 0)).toBe(false);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,56 @@
|
||||
import { existsSync, readFileSync, readdirSync } from "node:fs";
|
||||
import { join, resolve } from "node:path";
|
||||
import { lintHyperframeHtml, type HyperframeLintResult } from "@hyperframes/core/lint";
|
||||
import type { ProjectDir } from "./project.js";
|
||||
|
||||
export interface ProjectLintResult {
|
||||
results: Array<{ file: string; result: HyperframeLintResult }>;
|
||||
totalErrors: number;
|
||||
totalWarnings: number;
|
||||
}
|
||||
|
||||
/**
|
||||
* Lint the root index.html and all sub-compositions in the compositions/ directory.
|
||||
* Returns aggregated results across all files.
|
||||
*/
|
||||
export function lintProject(project: ProjectDir): ProjectLintResult {
|
||||
const results: Array<{ file: string; result: HyperframeLintResult }> = [];
|
||||
let totalErrors = 0;
|
||||
let totalWarnings = 0;
|
||||
|
||||
// Lint root composition
|
||||
const rootHtml = readFileSync(project.indexPath, "utf-8");
|
||||
const rootResult = lintHyperframeHtml(rootHtml, { filePath: project.indexPath });
|
||||
results.push({ file: "index.html", result: rootResult });
|
||||
totalErrors += rootResult.errorCount;
|
||||
totalWarnings += rootResult.warningCount;
|
||||
|
||||
// Lint sub-compositions in compositions/ directory
|
||||
const compositionsDir = resolve(project.dir, "compositions");
|
||||
if (existsSync(compositionsDir)) {
|
||||
const files = readdirSync(compositionsDir).filter((f) => f.endsWith(".html"));
|
||||
for (const file of files) {
|
||||
const filePath = join(compositionsDir, file);
|
||||
const html = readFileSync(filePath, "utf-8");
|
||||
const result = lintHyperframeHtml(html, { filePath });
|
||||
results.push({ file: `compositions/${file}`, result });
|
||||
totalErrors += result.errorCount;
|
||||
totalWarnings += result.warningCount;
|
||||
}
|
||||
}
|
||||
|
||||
return { results, totalErrors, totalWarnings };
|
||||
}
|
||||
|
||||
/**
|
||||
* Determine whether a render should be blocked based on lint results and strict mode.
|
||||
* --strict blocks on errors; --strict-all blocks on errors or warnings.
|
||||
*/
|
||||
export function shouldBlockRender(
|
||||
strictErrors: boolean,
|
||||
strictAll: boolean,
|
||||
totalErrors: number,
|
||||
totalWarnings: number,
|
||||
): boolean {
|
||||
return (strictErrors && totalErrors > 0) || (strictAll && (totalErrors > 0 || totalWarnings > 0));
|
||||
}
|
||||
Reference in New Issue
Block a user