fix(render): pre-flight aspect-ratio / alpha preset mismatch with actionable guidance (#1843)

Users pick an --resolution preset whose orientation/aspect ratio (or alpha/HDR mode) conflicts with the composition; the render fails deep in the compiler with a cryptic message. ~8K err / ~1K users. - New shared pure helper checkOutputResolutionCompatibility in @hyperframes/parsers — single source of truth for aspect/alpha/HDR/downsample/non-integer-scale constraints; suggests the matching-orientation, tier-preserving preset. - CLI render pre-flight aborts early (before browser/ffmpeg) with an actionable, fix-suggesting message; resolveDeviceScaleFactor delegates to the same helper for identical defense-in-depth messages. - Suggest (not auto-select); defers when dims can't be determined rather than guessing. - suggestMatchingPreset keys tier off the -4k suffix so square-family swaps (square + landscape-4k -> square-4k) aren't downgraded to HD. - render.js DOM polyfill made a lazy import; render.test cold-import beforeAll hooks given a 30s timeout to absorb CI contention. Render-reliability workstream P1-3. Success measured on PostHog dashboard 1783183. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-09-03 12:54:29 +00:00 · 2026-07-01 19:27:08 -07:00
parent c0c3abf0f1
commit 6be46813a2
10 changed files with 554 additions and 53 deletions
@@ -62,18 +62,19 @@ vi.mock("../browser/preflight.js", () => ({
 describe("renderLocal browser GPU config", () => {
  const savedEnv = new Map<string, string | undefined>();
  // Pre-resolve once. The first dynamic `import("./render.js")` in this file
-  // takes >5 s on Windows runners (cold module load) — long enough to blow
-  // vitest's default 5 s timeout in whichever test happens to be first. When
-  // that test times out, its leaked late `createRenderJob` call lands AFTER
-  // the next test's `beforeEach` clears `producerState.createdJobs`, shifting
-  // index 0 and corrupting unrelated assertions. Importing once in
-  // `beforeAll` keeps every test fast and isolated.
+  // cold-loads a heavy module graph (core + engine + producer, incl. linkedom)
+  // and takes >5 s on Windows runners — and materially longer under the full
+  // parallel monorepo test run, where it can exceed the default 10 s hook
+  // timeout on a contended CI runner. Importing once in `beforeAll` keeps every
+  // test fast and isolated; the explicit 30 s hook timeout absorbs cold-import
+  // contention so this doesn't flake (the failure was a pre-existing
+  // `Hook timed out in 10000ms`, reproducible on `main` under load).
  let renderLocal: typeof import("./render.js").renderLocal;
  let resolveBrowserGpuForCli: typeof import("./render.js").resolveBrowserGpuForCli;

  beforeAll(async () => {
    ({ renderLocal, resolveBrowserGpuForCli } = await import("./render.js"));
-  });
+  }, 30_000);

  function setEnv(key: string, value: string) {
    if (!savedEnv.has(key)) savedEnv.set(key, process.env[key]);
@@ -416,4 +417,74 @@ describe("renderLocal browser GPU config", () => {
  });
 });

+describe("checkRenderResolutionPreflight", () => {
+  let checkRenderResolutionPreflight: typeof import("./render.js").checkRenderResolutionPreflight;
+
+  // 30 s hook timeout: cold-importing render.js (heavy graph) can exceed the
+  // default 10 s under parallel CI contention. See the note on the
+  // "renderLocal browser GPU config" beforeAll above.
+  beforeAll(async () => {
+    ({ checkRenderResolutionPreflight } = await import("./render.js"));
+  }, 30_000);
+
+  // Dims must be read the same way the producer's compiler reads them:
+  // `data-width` / `data-height` on the `[data-composition-id]` root.
+  const comp = (w: number, h: number) =>
+    `<html><body><div data-composition-id="root" data-width="${w}" data-height="${h}"></div></body></html>`;
+  const portraitHtml = comp(1080, 1920);
+  const landscapeHtml = comp(1920, 1080);
+  const noModes = { alphaRequested: false, hdrRequested: false } as const;
+
+  it("returns undefined when no outputResolution is requested", async () => {
+    expect(await checkRenderResolutionPreflight(portraitHtml, undefined, noModes)).toBeUndefined();
+  });
+
+  it("returns undefined when the preset matches the composition orientation", async () => {
+    expect(await checkRenderResolutionPreflight(portraitHtml, "portrait", noModes)).toBeUndefined();
+  });
+
+  it("returns a suggestion when a landscape preset is used on a portrait composition", async () => {
+    const message = await checkRenderResolutionPreflight(portraitHtml, "landscape", noModes);
+    expect(message).toBeDefined();
+    expect(message).toContain("--resolution portrait");
+  });
+
+  it("suggests landscape for a landscape composition rendered with a portrait preset", async () => {
+    const message = await checkRenderResolutionPreflight(landscapeHtml, "portrait", noModes);
+    expect(message).toContain("--resolution landscape");
+  });
+
+  it("preserves the 4K tier when suggesting a matching preset (square comp + landscape-4k → square-4k)", async () => {
+    // Tier-aware suggestion is the load-bearing new behavior; square-4k is the
+    // preset that only surfaces via a same-tier swap, so guard it explicitly.
+    const message = await checkRenderResolutionPreflight(comp(2160, 2160), "landscape-4k", noModes);
+    expect(message).toContain("--resolution square-4k");
+  });
+
+  it("does not false-abort a landscape registry-block composition (data-width/height, no data-resolution)", async () => {
+    // Regression guard: registry blocks carry data-width/height and no
+    // data-resolution — a preset-snapping heuristic would misread this as
+    // portrait and wrongly reject the correct --resolution landscape.
+    expect(
+      await checkRenderResolutionPreflight(landscapeHtml, "landscape", noModes),
+    ).toBeUndefined();
+  });
+
+  it("flags alpha output combined with outputResolution", async () => {
+    const message = await checkRenderResolutionPreflight(landscapeHtml, "landscape-4k", {
+      alphaRequested: true,
+      hdrRequested: false,
+    });
+    expect(message).toContain("alpha output");
+  });
+
+  it("returns undefined when composition dimensions can't be determined (defers to the pipeline)", async () => {
+    // No [data-composition-id] root / no data-width/height → defer, never guess.
+    expect(await checkRenderResolutionPreflight("", "landscape", noModes)).toBeUndefined();
+    expect(
+      await checkRenderResolutionPreflight("<html><body></body></html>", "landscape", noModes),
+    ).toBeUndefined();
+  });
+});
+
 // Variables-helper tests live in `../utils/variables.test.ts`.
@@ -81,6 +81,7 @@ import {
 } from "@hyperframes/engine";
 import {
  normalizeResolutionFlag,
+  checkOutputResolutionCompatibility,
  parseFps,
  fpsToNumber,
  fpsToFfmpegArg,
@@ -786,6 +787,36 @@ export default defineCommand({
      }
    }

+    // ── Pre-flight: output-resolution vs composition compatibility ────────
+    // Catch a preset whose orientation/aspect ratio (or alpha/HDR mode)
+    // conflicts with the composition BEFORE the browser and ffmpeg spin up —
+    // otherwise this surfaces cryptically deep inside the render compiler
+    // (resolveDeviceScaleFactor). Best-effort: a composition we can't read or
+    // whose dimensions aren't a known preset falls through to the pipeline's
+    // own defense-in-depth check rather than blocking a render we can't reason
+    // about. See render-reliability workstream P1-3.
+    if (outputResolution) {
+      let resolutionIssue: string | undefined;
+      try {
+        const renderTarget = entryFile ? resolve(project.dir, entryFile) : project.indexPath;
+        resolutionIssue = await checkRenderResolutionPreflight(
+          readFileSync(renderTarget, "utf8"),
+          outputResolution,
+          {
+            alphaRequested: format === "webm" || format === "mov" || format === "png-sequence",
+            hdrRequested: args.hdr ?? false,
+          },
+        );
+      } catch {
+        // Unreadable file is non-fatal here — the render pipeline will surface
+        // the real problem with full context.
+      }
+      if (resolutionIssue) {
+        errorBox("Output resolution incompatible", resolutionIssue);
+        process.exit(1);
+      }
+    }
+
    // ── Validate HDR/SDR mutual exclusion ────────────────────────────────
    if (args.hdr && args.sdr) {
      console.error("Error: --hdr and --sdr are mutually exclusive.");
@@ -993,6 +1024,71 @@ export function resolveBrowserGpuForCli(
  return "auto";
 }

+/**
+ * Read a composition's dimensions from the SAME source the producer's compiler
+ * uses — `data-width` / `data-height` on the `[data-composition-id]` root (see
+ * htmlCompiler.ts). Returns `undefined` when they can't be determined (no root,
+ * missing/invalid attrs, unparseable HTML). Note the producer *defaults* a
+ * missing attr to 1080; this pre-flight deliberately defers instead (returns
+ * `undefined`) rather than guess a dimension the author didn't declare, so it
+ * never false-aborts — the producer's defense-in-depth still catches that case.
+ *
+ * Deriving dims any other way (e.g. `data-resolution` or a `#stage` heuristic)
+ * risks disagreeing with the actual render: most compositions (all registry
+ * blocks) carry `data-width/height` and no `data-resolution`, so a parallel
+ * heuristic could false-abort a valid render. `DOMParser` isn't shipped by
+ * Node — the CLI polyfills it via linkedom, imported lazily so the heavy DOM
+ * library stays out of `render.js`'s module-load graph (it cold-imports at
+ * >5 s already; a static linkedom import tips the render test suite's import
+ * hook over its timeout — see the note on `renderLocal browser GPU config`).
+ */
+async function readCompositionDimensions(
+  compositionHtml: string,
+): Promise<{ width: number; height: number } | undefined> {
+  try {
+    const { ensureDOMParser } = await import("../utils/dom.js");
+    ensureDOMParser();
+    const doc = new DOMParser().parseFromString(compositionHtml, "text/html");
+    const rootEl = doc.querySelector("[data-composition-id]");
+    const width = parseInt(rootEl?.getAttribute("data-width") ?? "", 10);
+    const height = parseInt(rootEl?.getAttribute("data-height") ?? "", 10);
+    if (width > 0 && height > 0) return { width, height };
+  } catch {
+    // Unreadable / unparseable composition — fall through to `undefined`.
+  }
+  return undefined;
+}
+
+/**
+ * Render pre-flight: return an actionable message when the chosen
+ * `outputResolution` preset is incompatible with the composition's
+ * orientation/aspect ratio, or with the alpha/HDR mode — or `undefined` when
+ * the combination is fine (or can't be determined statically).
+ *
+ * Extracted (and exported) so the CLI wiring around `process.exit` stays a
+ * thin adapter and the branch logic is unit-testable. See render-reliability
+ * workstream P1-3.
+ */
+export async function checkRenderResolutionPreflight(
+  compositionHtml: string,
+  outputResolution: CanvasResolution | undefined,
+  modes: { alphaRequested: boolean; hdrRequested: boolean },
+): Promise<string | undefined> {
+  if (!outputResolution) return undefined;
+  const dims = await readCompositionDimensions(compositionHtml);
+  // Couldn't determine the composition's actual dimensions — defer to the
+  // pipeline's own defense-in-depth check rather than guess.
+  if (!dims) return undefined;
+  const compat = checkOutputResolutionCompatibility({
+    compositionWidth: dims.width,
+    compositionHeight: dims.height,
+    outputResolution,
+    alphaRequested: modes.alphaRequested,
+    hdrRequested: modes.hdrRequested,
+  });
+  return compat.ok ? undefined : compat.message;
+}
+
 const DOCKER_IMAGE_PREFIX = "hyperframes-renderer";

 function dockerImageTag(version: string): string {