fix(producer): honor variables + outputResolution in HTTP render server (#1152)

* fix(producer): honor variables + outputResolution in HTTP render server The producer HTTP server's parseRenderOptions read only fps/quality/workers/gpu/debug/entryFile/format from the request body. `variables` and `outputResolution` were silently dropped, so any caller of the server render path (the cloud-render sidecar that experiment-framework POSTs to) got the composition's declared variable defaults and its intrinsic dimensions regardless of what was requested. RenderConfig already supports both fields (the local CLI `render` command passes them); the server just never forwarded them. Wire them through RenderInput, parseRenderOptions, and a shared buildRenderJobConfig used by the sync + streaming handlers. outputResolution now drives the same resolveDeviceScaleFactor supersampling path the local CLI uses, so a 4k render against a matching-aspect composition produces true 4k. Validation: a non-object `variables` or an unknown `outputResolution` returns a clean 400 instead of being silently ignored. Also extracts resolvePreparedRenderOutput + parseRenderOverrides helpers to keep both handlers DRY. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(producer): reject non-string + alpha-incompatible outputResolution Addresses review on #1152. - A non-string `outputResolution` (e.g. a JSON number) was coerced to `undefined` by parseRenderOverrides and silently ignored — the same silent-drop this validation exists to prevent. Now rejected with a 400. - `outputResolution` + an alpha format (webm/mov) is rejected up front: supersampling runs through a deviceScaleFactor the alpha capture path can't apply, so resolveDeviceScaleFactor throws mid-render. Guarding it here makes the producer self-defending for every caller (not just the CLI / external API), and closes the 1080p-webm regression window during the producer-honors-outputResolution rollout. Extracted validateOutputResolutionOverride to keep validateRenderOverrides under the complexity gate. +2 prepareRenderBody tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-09-03 04:38:33 +00:00 · 2026-06-01 17:59:08 -04:00
parent a01a266efa
commit 8b6d35e226
2 changed files with 225 additions and 36 deletions
@@ -0,0 +1,87 @@
+import { describe, expect, it } from "bun:test";
+import { mkdtempSync, writeFileSync } from "node:fs";
+import { join } from "node:path";
+import { tmpdir } from "node:os";
+
+import { parseRenderOptions, prepareRenderBody } from "./server.js";
+
+describe("parseRenderOptions — variables", () => {
+  it("forwards a plain JSON object", () => {
+    const opts = parseRenderOptions({ variables: { title: "Hello", n: 3 } });
+    expect(opts.variables).toEqual({ title: "Hello", n: 3 });
+  });
+
+  it("drops non-object variables to undefined", () => {
+    expect(parseRenderOptions({ variables: [1, 2] }).variables).toBeUndefined();
+    expect(parseRenderOptions({ variables: "nope" }).variables).toBeUndefined();
+    expect(parseRenderOptions({ variables: null }).variables).toBeUndefined();
+    expect(parseRenderOptions({}).variables).toBeUndefined();
+  });
+});
+
+describe("parseRenderOptions — outputResolution", () => {
+  it("normalizes canonical presets and aliases", () => {
+    expect(parseRenderOptions({ outputResolution: "landscape-4k" }).outputResolution).toBe(
+      "landscape-4k",
+    );
+    expect(parseRenderOptions({ outputResolution: "4k" }).outputResolution).toBe("landscape-4k");
+    expect(parseRenderOptions({ outputResolution: "portrait-4k" }).outputResolution).toBe(
+      "portrait-4k",
+    );
+    expect(parseRenderOptions({ outputResolution: "square" }).outputResolution).toBe("square");
+  });
+
+  it("drops unknown / non-string resolutions to undefined", () => {
+    expect(parseRenderOptions({ outputResolution: "8k" }).outputResolution).toBeUndefined();
+    expect(parseRenderOptions({ outputResolution: 123 }).outputResolution).toBeUndefined();
+    expect(parseRenderOptions({}).outputResolution).toBeUndefined();
+  });
+});
+
+describe("prepareRenderBody — validation", () => {
+  it("rejects an explicitly-supplied non-object variables", async () => {
+    const result = await prepareRenderBody({ variables: [1, 2], html: "<html></html>" });
+    expect(result).toHaveProperty("error");
+    expect((result as { error: string }).error).toContain("variables must be a JSON object");
+  });
+
+  it("rejects an explicitly-supplied invalid outputResolution", async () => {
+    const result = await prepareRenderBody({ outputResolution: "8k", html: "<html></html>" });
+    expect(result).toHaveProperty("error");
+    expect((result as { error: string }).error).toContain("Invalid outputResolution");
+  });
+
+  it("rejects a non-string outputResolution instead of silently dropping it", async () => {
+    const result = await prepareRenderBody({ outputResolution: 123, html: "<html></html>" });
+    expect(result).toHaveProperty("error");
+    expect((result as { error: string }).error).toContain("must be a string preset");
+  });
+
+  it("rejects outputResolution combined with an alpha format (webm/mov)", async () => {
+    for (const format of ["webm", "mov"]) {
+      const result = await prepareRenderBody({
+        outputResolution: "4k",
+        format,
+        html: "<html></html>",
+      });
+      expect(result).toHaveProperty("error");
+      expect((result as { error: string }).error).toContain("can't supersample");
+    }
+  });
+
+  it("threads variables + outputResolution into the prepared render input", async () => {
+    const dir = mkdtempSync(join(tmpdir(), "hf-server-test-"));
+    writeFileSync(join(dir, "index.html"), "<html><body></body></html>", "utf-8");
+
+    const result = await prepareRenderBody({
+      projectDir: dir,
+      variables: { title: "Q4" },
+      outputResolution: "4k",
+    });
+
+    expect(result).toHaveProperty("prepared");
+    const { input } = (result as { prepared: { input: Record<string, unknown> } }).prepared;
+    expect(input.variables).toEqual({ title: "Q4" });
+    expect(input.outputResolution).toBe("landscape-4k");
+  });
+});
@@ -38,7 +38,7 @@ import { prepareHyperframeLintBody, runHyperframeLint } from "./services/hyperfr
 import { resolveRenderPaths } from "./utils/paths.js";
 import { defaultLogger, type ProducerLogger } from "./logger.js";
 import { Semaphore } from "./utils/semaphore.js";
-import { parseFps } from "@hyperframes/core";
+import { parseFps, normalizeResolutionFlag, type CanvasResolution } from "@hyperframes/core";

 // ---------------------------------------------------------------------------
 // Types
@@ -76,6 +76,19 @@ interface RenderInput {
  useGpu: boolean;
  debug: boolean;
  entryFile?: string;
+  /**
+   * data-composition-variables overrides forwarded into the render config.
+   * Without this the HTTP/server render path silently rendered the
+   * composition's declared defaults, ignoring per-request overrides.
+   */
+  variables?: Record<string, unknown>;
+  /**
+   * Output resolution preset (e.g. `landscape-4k`). Drives the same
+   * `resolveDeviceScaleFactor` supersampling path the local CLI uses — Chrome
+   * renders at a higher devicePixelRatio so the captured screenshot lands at
+   * the requested dimensions. Aspect ratio must match the composition.
+   */
+  outputResolution?: CanvasResolution;
 }

 interface PreparedRenderInput {
@@ -83,7 +96,7 @@ interface PreparedRenderInput {
  cleanupProjectDir?: string;
 }

-function parseRenderOptions(body: Record<string, unknown>): Omit<RenderInput, "projectDir"> {
+export function parseRenderOptions(body: Record<string, unknown>): Omit<RenderInput, "projectDir"> {
  // Accept either a JSON `number` (integer fps) or a JSON `string` (rational
  // like "30000/1001"). Falls back to 30 fps on parse failure to preserve the
  // forgiving behaviour the original whitelist had — the producer surfaces a
@@ -114,12 +127,127 @@ function parseRenderOptions(body: Record<string, unknown>): Omit<RenderInput, "p
    ["mp4", "webm", "mov"].includes(body.format as string) ? body.format : undefined
  ) as "mp4" | "webm" | "mov" | undefined;

-  return { outputPath, fps, quality, workers, useGpu, debug, entryFile, format };
+  const { variables, outputResolution } = parseRenderOverrides(body);
+
+  return {
+    outputPath,
+    fps,
+    quality,
+    workers,
+    useGpu,
+    debug,
+    entryFile,
+    format,
+    variables,
+    outputResolution,
+  };
 }

-async function prepareRenderBody(
+function isPlainObject(value: unknown): value is Record<string, unknown> {
+  return typeof value === "object" && value !== null && !Array.isArray(value);
+}
+
+/**
+ * Parse the lenient form of the variable + resolution overrides used by
+ * `parseRenderOptions`. Invalid shapes coerce to `undefined` here;
+ * `validateRenderOverrides` separately rejects explicitly-supplied bad values
+ * with a 400 so they aren't silently ignored.
+ */
+function parseRenderOverrides(body: Record<string, unknown>): {
+  variables?: Record<string, unknown>;
+  outputResolution?: CanvasResolution;
+} {
+  // Only forward a plain JSON object. Arrays / primitives / null → undefined.
+  const variables = isPlainObject(body.variables) ? body.variables : undefined;
+  // Accept canonical presets and aliases ("4k", "landscape-4k", …).
+  const outputResolution =
+    typeof body.outputResolution === "string"
+      ? normalizeResolutionFlag(body.outputResolution)
+      : undefined;
+  return { variables, outputResolution };
+}
+
+/**
+ * Build the `createRenderJob` config from a prepared render input. Shared by
+ * the sync (`render`) and streaming (`render-stream`) handlers so the field
+ * set — including `variables` and `outputResolution` — stays in one place.
+ */
+function buildRenderJobConfig(input: RenderInput, log: ProducerLogger) {
+  return {
+    fps: input.fps,
+    quality: input.quality,
+    format: input.format,
+    workers: input.workers,
+    useGpu: input.useGpu,
+    debug: input.debug,
+    entryFile: input.entryFile,
+    variables: input.variables,
+    outputResolution: input.outputResolution,
+    logger: log,
+  };
+}
+
+/**
+ * Resolve the destination path for a prepared render and ensure its parent
+ * directory exists. Shared by the sync + streaming handlers (their only
+ * difference is how a `prepareRenderBody` error is surfaced — JSON vs SSE —
+ * which stays in each handler).
+ */
+function resolvePreparedRenderOutput(
+  prepared: PreparedRenderInput,
+  rendersDir: string,
+  log: ProducerLogger,
+): { input: RenderInput; cleanupProjectDir?: string; absoluteOutputPath: string } {
+  const { input, cleanupProjectDir } = prepared;
+  const absoluteOutputPath = resolveOutputPath(input.projectDir, input.outputPath, rendersDir, log);
+  const outputDir = dirname(absoluteOutputPath);
+  if (!existsSync(outputDir)) mkdirSync(outputDir, { recursive: true });
+  return { input, cleanupProjectDir, absoluteOutputPath };
+}
+
+/**
+ * Validate explicitly-supplied render overrides that can't be sanely coerced.
+ * Returns an error string for a clean 400, or `undefined` when the body is
+ * acceptable (including when the fields are simply absent).
+ */
+function validateRenderOverrides(body: Record<string, unknown>): string | undefined {
+  if (body.variables !== undefined && !isPlainObject(body.variables)) {
+    return 'variables must be a JSON object keyed by variable id (e.g. {"title":"Hello"})';
+  }
+  return validateOutputResolutionOverride(body);
+}
+
+/**
+ * Validate an explicitly-supplied `outputResolution`. Rejects (a) non-string
+ * values, which parseRenderOverrides would otherwise silently coerce to
+ * `undefined`; (b) unknown presets; and (c) the alpha-format combination —
+ * outputResolution drives deviceScaleFactor supersampling, which the webm/mov
+ * capture path can't apply (resolveDeviceScaleFactor throws mid-render), so we
+ * reject it here for a clean 400 regardless of which caller sent it.
+ */
+function validateOutputResolutionOverride(body: Record<string, unknown>): string | undefined {
+  if (body.outputResolution === undefined) return undefined;
+  if (typeof body.outputResolution !== "string") {
+    return 'outputResolution must be a string preset (e.g. "4k", "landscape-4k")';
+  }
+  const normalized = normalizeResolutionFlag(body.outputResolution);
+  if (body.outputResolution.trim().length > 0 && normalized === undefined) {
+    return `Invalid outputResolution "${body.outputResolution}". Must be one of: landscape, portrait, landscape-4k, portrait-4k, square, square-4k (aliases: 1080p, 4k, …).`;
+  }
+  if (normalized !== undefined && (body.format === "webm" || body.format === "mov")) {
+    return `outputResolution is not supported with format "${body.format}" — the alpha (webm/mov) capture path can't supersample. Use format "mp4", or omit outputResolution to render at the composition's native dimensions.`;
+  }
+  return undefined;
+}
+
+export async function prepareRenderBody(
  body: Record<string, unknown>,
 ): Promise<{ prepared: PreparedRenderInput } | { error: string }> {
+  // Reject explicitly-supplied-but-malformed overrides up front so the caller
+  // gets a clear 400 instead of a silently-ignored value.
+  const overrideError = validateRenderOverrides(body);
+  if (overrideError) return { error: overrideError };
+
  const options = parseRenderOptions(body);
  const projectDir = typeof body.projectDir === "string" ? body.projectDir : undefined;
  if (projectDir) {
@@ -322,15 +450,11 @@ export function createRenderHandlers(options: HandlerOptions = {}): RenderHandle
      return c.json({ success: false, requestId, error: preparedResult.error }, 400);
    }

-    const { input, cleanupProjectDir } = preparedResult.prepared;
-    const absoluteOutputPath = resolveOutputPath(
-      input.projectDir,
-      input.outputPath,
+    const { input, cleanupProjectDir, absoluteOutputPath } = resolvePreparedRenderOutput(
+      preparedResult.prepared,
      rendersDir,
      log,
    );
-    const outputDir = dirname(absoluteOutputPath);
-    if (!existsSync(outputDir)) mkdirSync(outputDir, { recursive: true });

    const release = await renderSemaphore.acquire();

@@ -341,16 +465,7 @@ export function createRenderHandlers(options: HandlerOptions = {}): RenderHandle
      quality: input.quality,
    });

-    const job = createRenderJob({
-      fps: input.fps,
-      quality: input.quality,
-      format: input.format,
-      workers: input.workers,
-      useGpu: input.useGpu,
-      debug: input.debug,
-      entryFile: input.entryFile,
-      logger: log,
-    });
+    const job = createRenderJob(buildRenderJobConfig(input, log));

    let lastLoggedPct = -10;
    try {
@@ -443,28 +558,15 @@ export function createRenderHandlers(options: HandlerOptions = {}): RenderHandle
        return;
      }

-      const { input, cleanupProjectDir } = preparedResult.prepared;
-      const absoluteOutputPath = resolveOutputPath(
-        input.projectDir,
-        input.outputPath,
+      const { input, cleanupProjectDir, absoluteOutputPath } = resolvePreparedRenderOutput(
+        preparedResult.prepared,
        rendersDir,
        log,
      );
-      const outputDir = dirname(absoluteOutputPath);
-      if (!existsSync(outputDir)) mkdirSync(outputDir, { recursive: true });

      log.info("render-stream started", { requestId, projectDir: input.projectDir });

-      const job = createRenderJob({
-        fps: input.fps,
-        quality: input.quality,
-        format: input.format,
-        workers: input.workers,
-        useGpu: input.useGpu,
-        debug: input.debug,
-        entryFile: input.entryFile,
-        logger: log,
-      });
+      const job = createRenderJob(buildRenderJobConfig(input, log));
      const abortController = new AbortController();
      const onRequestAbort = () =>
        abortController.abort(new RenderCancelledError("request_aborted"));