From 0338be97fdc548fd4a179561810c0c9320b61388 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miguel=20=C3=81ngel?= <miguel.sierra@heygen.com>
Date: Sat, 4 Jul 2026 14:08:22 -0700
Subject: [PATCH] fix(cli): validate navigation timeout honors --timeout, hints
 on CDN scripts (#1929)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

`validate` navigated the page with a hardcoded 10s timeout that ignored
the --timeout option. A composition that loads GSAP (or any library)
from a CDN <script> in <head> blocks `domcontentloaded` until that
script finishes downloading; on a slow network that exceeds 10s and
validate fails with an opaque "Navigation timeout of 10000ms exceeded"
— even though the full render (much larger budget) rides it out fine,
and even though --timeout (the documented "wait longer for slow loads"
knob) had no effect on navigation. The only recourse was to change the
composition (vendor the script locally).

Reported precisely, with the exact error and the observation that
render's 60s budget masks it while validate's 10s trips.

Fix:
- resolveNavigationTimeoutMs(optTimeout) = max(10s floor, --timeout), so
  --timeout now also extends the navigation budget. Default behavior is
  unchanged: the default --timeout (3000) stays clamped to the 10s floor.
- navigationTimeoutHint() replaces Puppeteer's opaque timeout error with
  an actionable message naming the likely cause (a blocking CDN <script>)
  and the two fixes (vendor locally / raise --timeout). Any non-timeout
  error is rethrown unchanged.
- --timeout help text updated to note it also governs navigation.

Both helpers are pure and exported; validateInBrowser wires them around
the single page.goto. No behavior change for compositions that navigate
within 10s.

Test: resolveNavigationTimeoutMs (floor kept for unset/small/zero,
raised past the floor) and navigationTimeoutHint (rewrites a nav-timeout
error with CDN + --timeout guidance; returns null for other errors so
the caller rethrows as-is). validate suite 14 tests pass.
---
 packages/cli/src/commands/validate.test.ts | 38 ++++++++++++++++++++
 packages/cli/src/commands/validate.ts      | 40 ++++++++++++++++++++--
 2 files changed, 76 insertions(+), 2 deletions(-)
diff --git a/packages/cli/src/commands/validate.test.ts b/packages/cli/src/commands/validate.test.ts
index 1030612c5..08277945b 100644
--- a/packages/cli/src/commands/validate.test.ts
+++ b/packages/cli/src/commands/validate.test.ts
@@ -1,7 +1,9 @@
 import { describe, expect, it } from "vitest";
 import {
   extractCompositionErrorsFromLint,
+  navigationTimeoutHint,
   raceMediaReady,
+  resolveNavigationTimeoutMs,
   shouldIgnoreRequestFailure,
 } from "./validate.js";
 import type { ProjectLintResult } from "../utils/lintProject.js";
@@ -192,3 +194,39 @@ describe("extractCompositionErrorsFromLint", () => {
     expect(errors.map((e) => e.text)).toEqual(["scene-a is empty", "scene-b is empty"]);
   });
 });
+
+// Regression: `validate` used a hardcoded 10s page-navigation timeout that
+// ignored --timeout, so a composition loading GSAP from a CDN <script> (which
+// blocks domcontentloaded) failed with an opaque "Navigation timeout of 10000ms"
+// even though the full render's larger budget rode it out — with no knob to
+// extend it. resolveNavigationTimeoutMs makes --timeout raise the nav budget
+// (never below the 10s floor); navigationTimeoutHint replaces the opaque error.
+describe("resolveNavigationTimeoutMs", () => {
+  it("keeps the 10s floor when --timeout is unset or smaller", () => {
+    expect(resolveNavigationTimeoutMs(undefined)).toBe(10000);
+    expect(resolveNavigationTimeoutMs(3000)).toBe(10000); // the default --timeout
+    expect(resolveNavigationTimeoutMs(0)).toBe(10000);
+  });
+
+  it("raises the navigation budget to --timeout when it exceeds the floor", () => {
+    expect(resolveNavigationTimeoutMs(30000)).toBe(30000);
+  });
+});
+
+describe("navigationTimeoutHint", () => {
+  it("replaces a Puppeteer navigation-timeout error with an actionable CDN/--timeout hint", () => {
+    const hinted = navigationTimeoutHint(
+      new Error("Navigation timeout of 10000 ms exceeded"),
+      10000,
+    );
+    expect(hinted).toBeInstanceOf(Error);
+    expect(hinted?.message).toContain("10000ms");
+    expect(hinted?.message).toContain("CDN");
+    expect(hinted?.message).toContain("--timeout");
+  });
+
+  it("returns null for any non-navigation-timeout error so the caller rethrows it as-is", () => {
+    expect(navigationTimeoutHint(new Error("net::ERR_CONNECTION_REFUSED"), 10000)).toBeNull();
+    expect(navigationTimeoutHint("some string failure", 10000)).toBeNull();
+  });
+});
diff --git a/packages/cli/src/commands/validate.ts b/packages/cli/src/commands/validate.ts
index a298860d3..29ecf80af 100644
--- a/packages/cli/src/commands/validate.ts
+++ b/packages/cli/src/commands/validate.ts
@@ -33,6 +33,33 @@ interface ContrastEntry {
 const CONTRAST_SAMPLES = 5;
 const SEEK_SETTLE_MS = 150;
 const MEDIA_EXTENSIONS = /\.(aac|flac|m4a|mov|mp3|mp4|oga|ogg|wav|webm)$/i;
+// Floor for the initial page navigation. A blocking external <script> (GSAP
+// from a CDN, etc.) delays `domcontentloaded`; the actual render (much larger
+// budget) rides it out, so validate's navigation must be at least as patient as
+// the user's --timeout, never stuck below this floor.
+const NAV_TIMEOUT_FLOOR_MS = 10000;
+
+// Navigation budget = the larger of the floor and the user's --timeout, so
+// `--timeout` (already the "wait longer for slow loads" knob for media/settle)
+// also extends navigation instead of being ignored by a hardcoded 10s.
+export function resolveNavigationTimeoutMs(optTimeout?: number): number {
+  return Math.max(NAV_TIMEOUT_FLOOR_MS, optTimeout ?? 0);
+}
+
+// Turn Puppeteer's opaque "Navigation timeout of Nms exceeded" into an
+// actionable message: the usual cause is a blocking CDN <script> that render
+// tolerates but validate's tighter budget does not. Returns a replacement Error
+// for a navigation timeout, or null for any other error (caller rethrows as-is).
+export function navigationTimeoutHint(err: unknown, navTimeoutMs: number): Error | null {
+  const msg = err instanceof Error ? err.message : String(err);
+  if (!/navigation timeout/i.test(msg)) return null;
+  return new Error(
+    `Page navigation timed out after ${navTimeoutMs}ms. A blocking external <script> ` +
+      `(e.g. GSAP loaded from a CDN) can delay page load past this budget even when the ` +
+      `full render succeeds. Vendor the script locally (recommended for deterministic ` +
+      `renders), or re-run with a longer --timeout.`,
+  );
+}
 
 export function shouldIgnoreRequestFailure(
   url: string,
@@ -353,7 +380,14 @@ async function validateInBrowser(
       }
     });
 
-    await page.goto(server.url, { waitUntil: "domcontentloaded", timeout: 10000 });
+    const navTimeoutMs = resolveNavigationTimeoutMs(opts.timeout);
+    try {
+      await page.goto(server.url, { waitUntil: "domcontentloaded", timeout: navTimeoutMs });
+    } catch (err) {
+      const hinted = navigationTimeoutHint(err, navTimeoutMs);
+      if (hinted) throw hinted;
+      throw err;
+    }
     await new Promise((r) => setTimeout(r, opts.timeout ?? 3000));
 
     for (const w of await auditClipDurations(page, analyzeClipMediaFit, opts.timeout ?? 3000)) {
@@ -472,7 +506,9 @@ Examples:
     },
     timeout: {
       type: "string",
-      description: "Ms to wait for scripts to settle (default: 3000)",
+      description:
+        "Ms to wait for scripts to settle and media to load (default: 3000). Also raises the " +
+        "page-navigation budget above its 10s floor when a slow external <script> needs longer.",
       default: "3000",
     },
   },
