diff --git a/packages/cli/src/commands/cloudrun.ts b/packages/cli/src/commands/cloudrun.ts index 326806225..65c2bda02 100644 --- a/packages/cli/src/commands/cloudrun.ts +++ b/packages/cli/src/commands/cloudrun.ts @@ -31,6 +31,7 @@ import { validateVariablesAgainstProject, } from "../utils/variables.js"; import { normalizeErrorMessage } from "../utils/errorMessage.js"; +import { readAllowedCompositionFpsFromDir } from "../utils/compositionFps.js"; export const examples: Example[] = [ ["Deploy the Cloud Run render stack", "hyperframes cloudrun deploy --project my-gcp-project"], @@ -499,7 +500,8 @@ async function runRender(args: Record): Promise { console.error("[cloudrun render] --width and --height are required."); process.exit(1); } - const fps = parseIntFlag(args.fps) ?? 30; + const fps = + parseIntFlag(args.fps) ?? readAllowedCompositionFpsFromDir(projectDir, [24, 30, 60]) ?? 30; if (fps !== 24 && fps !== 30 && fps !== 60) { console.error(`[cloudrun render] --fps must be 24, 30, or 60; got ${fps}.`); process.exit(1); @@ -609,7 +611,8 @@ async function runRenderBatch(args: Record): Promise { console.error("[cloudrun render-batch] --width and --height are required."); process.exit(1); } - const fps = parseIntFlag(args.fps) ?? 30; + const fps = + parseIntFlag(args.fps) ?? readAllowedCompositionFpsFromDir(projectDir, [24, 30, 60]) ?? 30; if (fps !== 24 && fps !== 30 && fps !== 60) { console.error(`[cloudrun render-batch] --fps must be 24, 30, or 60; got ${fps}.`); process.exit(1); diff --git a/packages/cli/src/commands/lambda.ts b/packages/cli/src/commands/lambda.ts index 6530d3bdd..f5ad901a3 100644 --- a/packages/cli/src/commands/lambda.ts +++ b/packages/cli/src/commands/lambda.ts @@ -17,6 +17,7 @@ import { } from "@hyperframes/core"; import type { Example } from "./_examples.js"; import { c } from "../ui/colors.js"; +import { readAllowedCompositionFpsFromDir } from "../utils/compositionFps.js"; export const examples: Example[] = [ ["Deploy the Lambda render stack to AWS", "hyperframes lambda deploy"], @@ -297,7 +298,10 @@ export default defineCommand({ console.error("[lambda render] --width and --height are required."); process.exit(1); } - const fpsRaw = parseIntFlag(args.fps) ?? 30; + const fpsRaw = + parseIntFlag(args.fps) ?? + readAllowedCompositionFpsFromDir(projectDir, [24, 30, 60]) ?? + 30; if (fpsRaw !== 24 && fpsRaw !== 30 && fpsRaw !== 60) { console.error(`[lambda render] --fps must be 24, 30, or 60; got ${fpsRaw}.`); process.exit(1); @@ -349,7 +353,10 @@ export default defineCommand({ console.error("[lambda render-batch] --width and --height are required."); process.exit(1); } - const fpsRaw = parseIntFlag(args.fps) ?? 30; + const fpsRaw = + parseIntFlag(args.fps) ?? + readAllowedCompositionFpsFromDir(projectDir, [24, 30, 60]) ?? + 30; if (fpsRaw !== 24 && fpsRaw !== 30 && fpsRaw !== 60) { console.error(`[lambda render-batch] --fps must be 24, 30, or 60; got ${fpsRaw}.`); process.exit(1); diff --git a/packages/cli/src/commands/render.test.ts b/packages/cli/src/commands/render.test.ts index 6bae67b2a..8df4a75e0 100644 --- a/packages/cli/src/commands/render.test.ts +++ b/packages/cli/src/commands/render.test.ts @@ -506,4 +506,21 @@ describe("checkRenderResolutionPreflight", () => { }); }); +describe("render fps arg definition", () => { + it("declares no citty default for --fps (so data-fps resolution can run)", async () => { + // Regression guard: a `default: "30"` here makes citty set args.fps="30" + // on omission, which short-circuits resolveDefaultFpsArg (explicitFps is + // never null) and silently reverts the command to always-30 — the exact + // no-op caught in review. The "30" fallback must live at the + // parseFps(fpsArg ?? "30") call, not on the arg. + const cmd = (await import("./render.js")).default; + // citty types `args` as Resolvable (it could be a promise/factory); in + // practice it's the literal object, so read it through a plain record. + const args = cmd.args as unknown as Record; + const fpsArg = args.fps; + expect(fpsArg).toBeDefined(); + expect(fpsArg?.default).toBeUndefined(); + }); +}); + // Variables-helper tests live in `../utils/variables.test.ts`. diff --git a/packages/cli/src/commands/render.ts b/packages/cli/src/commands/render.ts index ba2ee8374..b5afaf12b 100644 --- a/packages/cli/src/commands/render.ts +++ b/packages/cli/src/commands/render.ts @@ -10,6 +10,7 @@ import { parseGifLoopArg, resolveBrowserTimeoutMsArg, resolveCompositionEntryArg, + resolveDefaultFpsArg, } from "../utils/renderArgs.js"; export const examples: Example[] = [ @@ -171,8 +172,12 @@ export default defineCommand({ description: "Frame rate. Accepts integer (24, 25, 30, 50, 60, 120, 240) or " + "ffmpeg-style rational (30000/1001 for NTSC 29.97, 24000/1001 for " + - "23.976, 60000/1001 for 59.94). Range 1-240.", - default: "30", + "23.976, 60000/1001 for 59.94). Range 1-240. " + + "Defaults to the composition's root data-fps, else 30.", + // No `default` here on purpose: citty would set args.fps="30" on + // omission, which would make explicitFps always non-null and short- + // circuit the data-fps resolution below (resolveDefaultFpsArg). The + // "30" fallback lives at the parseFps(fpsArg ?? "30") call instead. }, quality: { type: "string", @@ -382,15 +387,24 @@ export default defineCommand({ // ── Resolve project ──────────────────────────────────────────────────── const project = resolveProject(args.dir); + // ── Resolve composition entry file ───────────────────────────────────── + // Needed early: fps default below must read the actual render target, not + // always index.html. + const entryFile = resolveCompositionEntryArg(args.composition, project.dir, statSync); + // ── Validate fps ─────────────────────────────────────────────────────── // Accept either integer (`30`) or ffmpeg-style rational (`30000/1001`). // The whitelist-based validator was replaced with a sane numeric range so // legitimate framerates (NTSC trio, PAL, 120/240 slow-mo) work without // CLI gymnastics. The exact rational survives end-to-end into FFmpeg's // `-r` / `-framerate` flags via `fpsToFfmpegArg`. - const fpsParse = parseFps(args.fps ?? "30"); + // Precedence: explicit --fps, else the composition's root data-fps, else 30. + // Honoring data-fps matches the runtime — render used to silently force 30 + // even when the composition declared e.g. data-fps="24". + const fpsArg = resolveDefaultFpsArg(args.fps, project.dir, project.indexPath, entryFile); + const fpsParse = parseFps(fpsArg ?? "30"); if (!fpsParse.ok) { - errorBox("Invalid fps", formatFpsParseError(args.fps ?? "30", fpsParse.reason)); + errorBox("Invalid fps", formatFpsParseError(fpsArg ?? "30", fpsParse.reason)); process.exit(1); } let fps: Fps = fpsParse.value; @@ -659,12 +673,11 @@ export default defineCommand({ console.log(c.warn(" GIF output is capped at 30fps. Use --fps 15 for smaller files.")); } - // ── Validate browser-timeout (seconds) and composition entry file ──── - // Both validators live in `utils/renderArgs.ts` so the parse/reject + // ── Validate browser-timeout (seconds) ─────────────────────────────── + // This validator lives in `utils/renderArgs.ts` so the parse/reject // branches are unit-testable without `process.exit`. See issue #1199 - // for the original EISDIR / silent-timeout-0 footguns this guards. + // for the original silent-timeout-0 footgun this guards. const pageNavigationTimeoutMs = resolveBrowserTimeoutMsArg(args["browser-timeout"]); - const entryFile = resolveCompositionEntryArg(args.composition, project.dir, statSync); // ── Preflight batch rows before browser/lint work ──────────────────── let batchModule: typeof import("./batchRender.js") | undefined; diff --git a/packages/cli/src/utils/compositionFps.test.ts b/packages/cli/src/utils/compositionFps.test.ts new file mode 100644 index 000000000..84ce2c4f3 --- /dev/null +++ b/packages/cli/src/utils/compositionFps.test.ts @@ -0,0 +1,107 @@ +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { readAllowedCompositionFpsFromDir, readCompositionFps } from "./compositionFps.js"; + +const wrap = (body: string) => `${body}`; + +describe("readCompositionFps", () => { + it("reads data-fps from the explicit data-root composition element", () => { + const html = wrap('
x
'); + expect(readCompositionFps(html)).toBe("24"); + }); + + it("reads data-fps from the outermost composition when no data-root is marked", () => { + const html = wrap( + '
x
', + ); + expect(readCompositionFps(html)).toBe("48"); + }); + + it("preserves a fractional rate verbatim for parseFps to validate", () => { + const html = wrap( + '
x
', + ); + expect(readCompositionFps(html)).toBe("30000/1001"); + }); + + it("returns null when the root has no data-fps", () => { + expect(readCompositionFps(wrap('
x
'))).toBeNull(); + }); + + it("returns null when there is no composition root", () => { + expect(readCompositionFps(wrap("
plain
"))).toBeNull(); + }); + + it("returns null for a blank data-fps", () => { + expect( + readCompositionFps(wrap('
x
')), + ).toBeNull(); + }); +}); + +describe("readAllowedCompositionFpsFromDir", () => { + const projectDirs: string[] = []; + const allowedCloudFps = [24, 30, 60] as const; + + afterEach(() => { + for (const dir of projectDirs.splice(0)) { + rmSync(dir, { recursive: true, force: true }); + } + }); + + function makeProject(indexBody: string): string { + const dir = mkdtempSync(join(tmpdir(), "hyperframes-composition-fps-")); + projectDirs.push(dir); + writeFileSync(join(dir, "index.html"), wrap(indexBody)); + return dir; + } + + it("uses cloud-allowed data-fps=60 as the default", () => { + const dir = makeProject( + '
x
', + ); + + expect(readAllowedCompositionFpsFromDir(dir, allowedCloudFps)).toBe(60); + }); + + it("uses cloud-allowed data-fps=24 as the default", () => { + const dir = makeProject( + '
x
', + ); + + expect(readAllowedCompositionFpsFromDir(dir, allowedCloudFps)).toBe(24); + }); + + it("returns null for data-fps=48 so cloud callers keep their ?? 30 fallback", () => { + const dir = makeProject( + '
x
', + ); + + const declared = readAllowedCompositionFpsFromDir(dir, allowedCloudFps); + expect(declared).toBeNull(); + expect(declared ?? 30).toBe(30); + }); + + it("returns null for fractional data-fps because cloud fps must be an integer", () => { + const dir = makeProject( + '
x
', + ); + + expect(readAllowedCompositionFpsFromDir(dir, allowedCloudFps)).toBeNull(); + }); + + it("returns null when index.html has no data-fps", () => { + const dir = makeProject('
x
'); + + expect(readAllowedCompositionFpsFromDir(dir, allowedCloudFps)).toBeNull(); + }); + + it("returns null when index.html cannot be read", () => { + const dir = mkdtempSync(join(tmpdir(), "hyperframes-composition-fps-missing-")); + projectDirs.push(dir); + + expect(readAllowedCompositionFpsFromDir(dir, allowedCloudFps)).toBeNull(); + }); +}); diff --git a/packages/cli/src/utils/compositionFps.ts b/packages/cli/src/utils/compositionFps.ts new file mode 100644 index 000000000..8286febbd --- /dev/null +++ b/packages/cli/src/utils/compositionFps.ts @@ -0,0 +1,59 @@ +import { readFileSync } from "node:fs"; +import { join } from "node:path"; +import { parseHTML } from "linkedom"; + +/** + * Read a composition's declared frame rate from its root element's `data-fps` + * attribute — the same attribute the runtime honors (core/runtime/init.ts) — so + * `hyperframes render` can default to it instead of a hard-coded 30 when `--fps` + * is not passed. Returns the raw attribute string (for the caller to validate + * via `parseFps`, which supports fractional rates like `30000/1001`), or `null` + * when no root `data-fps` is present. + * + * Root resolution mirrors the runtime: prefer an explicit + * `[data-composition-id][data-root="true"]`, else the outermost + * `[data-composition-id]` (one with no `[data-composition-id]` ancestor). + */ +export function readCompositionFps(html: string): string | null { + let doc: Document; + try { + doc = parseHTML(html).document as unknown as Document; + } catch { + return null; + } + + const explicitRoot = doc.querySelector('[data-composition-id][data-root="true"]'); + const root = + explicitRoot ?? + Array.from(doc.querySelectorAll("[data-composition-id]")).find( + (el) => !el.parentElement?.closest("[data-composition-id]"), + ) ?? + null; + + const raw = root?.getAttribute("data-fps")?.trim(); + return raw ? raw : null; +} + +/** + * Cloud render backends (Lambda, Cloud Run) accept only an integer fps + * from a small fixed allowed set (currently {24, 30, 60}) — unlike local + * `render`, they can't take an arbitrary/fractional data-fps. Reads + * `/index.html` and returns its declared data-fps as a number + * ONLY when it parses to an integer AND is a member of `allowed`; + * otherwise `null` so the caller keeps its own existing default (30). + */ +export function readAllowedCompositionFpsFromDir( + projectDir: string, + allowed: readonly number[], +): number | null { + let html: string; + try { + html = readFileSync(join(projectDir, "index.html"), "utf8"); + } catch { + return null; + } + const raw = readCompositionFps(html); + if (raw == null) return null; + const n = Number(raw); + return Number.isInteger(n) && allowed.includes(n) ? n : null; +} diff --git a/packages/cli/src/utils/renderArgs.test.ts b/packages/cli/src/utils/renderArgs.test.ts index c1fc17d3c..397d8cc51 100644 --- a/packages/cli/src/utils/renderArgs.test.ts +++ b/packages/cli/src/utils/renderArgs.test.ts @@ -1,11 +1,13 @@ import { describe, expect, it } from "vitest"; -import type { Stats } from "node:fs"; -import { resolve } from "node:path"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync, type Stats } from "node:fs"; +import { tmpdir } from "node:os"; +import { dirname, join, resolve } from "node:path"; import { MAX_PAGE_NAVIGATION_TIMEOUT_SECONDS, parseBrowserTimeoutMsArg, parseCompositionEntryArg, parseGifLoopArg, + resolveDefaultFpsArg, type BrowserTimeoutParseResult, type CompositionEntryParseResult, } from "./renderArgs.js"; @@ -173,6 +175,70 @@ describe("parseCompositionEntryArg", () => { }); }); +describe("resolveDefaultFpsArg", () => { + function writeComposition(path: string, fps: string): void { + writeFileSync( + path, + `
x
`, + ); + } + + function makeProject(): { + dir: string; + indexPath: string; + entryFile: string; + cleanup: () => void; + } { + const dir = mkdtempSync(join(tmpdir(), "hyperframes-render-fps-")); + const indexPath = join(dir, "index.html"); + const entryFile = "compositions/intro.html"; + const entryPath = join(dir, entryFile); + writeComposition(indexPath, "24"); + mkdirSync(dirname(entryPath), { recursive: true }); + return { + dir, + indexPath, + entryFile, + cleanup: () => rmSync(dir, { recursive: true, force: true }), + }; + } + + it("reads data-fps from the --composition target instead of index.html", () => { + const project = makeProject(); + try { + writeComposition(join(project.dir, project.entryFile), "60"); + + expect( + resolveDefaultFpsArg(undefined, project.dir, project.indexPath, project.entryFile), + ).toBe("60"); + } finally { + project.cleanup(); + } + }); + + it("falls back to index.html data-fps when --composition is not set", () => { + const project = makeProject(); + try { + expect(resolveDefaultFpsArg(undefined, project.dir, project.indexPath, undefined)).toBe("24"); + } finally { + project.cleanup(); + } + }); + + it("keeps an explicit --fps value ahead of any composition default", () => { + const project = makeProject(); + try { + writeComposition(join(project.dir, project.entryFile), "60"); + + expect(resolveDefaultFpsArg("120", project.dir, project.indexPath, project.entryFile)).toBe( + "120", + ); + } finally { + project.cleanup(); + } + }); +}); + describe("parseGifLoopArg", () => { it("accepts absent flag, bounds, and integers", () => { expect(parseGifLoopArg(undefined)).toEqual({ ok: true, value: undefined }); diff --git a/packages/cli/src/utils/renderArgs.ts b/packages/cli/src/utils/renderArgs.ts index 1e5338280..f7cd98a5c 100644 --- a/packages/cli/src/utils/renderArgs.ts +++ b/packages/cli/src/utils/renderArgs.ts @@ -9,9 +9,11 @@ * `timeout: 0` footguns at the rate of "one per missing branch". */ +import { readFileSync, type Stats } from "node:fs"; import { resolve, sep } from "node:path"; -import { type Stats } from "node:fs"; +import { parseFps } from "@hyperframes/core"; import { errorBox } from "../ui/format.js"; +import { readCompositionFps } from "./compositionFps.js"; // ── --browser-timeout ────────────────────────────────────────────────── @@ -220,6 +222,32 @@ export function resolveCompositionEntryArg( return result.value; } +// ── default fps ──────────────────────────────────────────────────────── + +/** + * Resolve the fps argument that local `render` should parse: explicit --fps, + * else the actual composition entry file's root data-fps when valid, else + * undefined so the caller can apply its final "30" default. + */ +export function resolveDefaultFpsArg( + explicitFps: string | undefined, + projectDir: string, + indexPath: string, + entryFile: string | undefined, +): string | undefined { + if (explicitFps != null) return explicitFps; + try { + const fpsSourcePath = entryFile ? resolve(projectDir, entryFile) : indexPath; + const declared = readCompositionFps(readFileSync(fpsSourcePath, "utf8")); + if (declared != null && parseFps(declared).ok) { + return declared; + } + } catch { + // Unreadable composition file — fall back to the default fps in render.ts. + } + return undefined; +} + export type GifLoopParseResult = | { ok: true; value: number | undefined } | { ok: false; message: string };