Skip to content

fix(cli): check and snapshot wait for a slow CDN script as long as render does - #5847

Merged
miguel-heygen merged 2 commits into
mainfrom
scrubfix/navdefault
Oct 18, 2026
Merged

miguel-heygen merged 2 commits into
mainfrom
scrubfix/navdefault

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Oct 18, 2026 •

Copy link
Copy Markdown
Collaborator

What

check and snapshot (and layout, motion-shot, validate) wait for a slow page load as long as render does. Before this, a composition whose <head> loads a script from a slow CDN (GSAP is the common case) intermittently failed check and snapshot with Navigation timeout of 10000 ms exceeded, fine on retry, while render of the same project succeeded.

Why

A parser-blocking <script src> holds back DOMContentLoaded. The diagnostic commands gave the page 10 s for it through their own resolveDiagnosticNavigationTimeoutMs (and validate had a separate 10 s floor). render uses the engine's pageNavigationTimeout (60 s, same PRODUCER_PAGE_NAVIGATION_TIMEOUT_MS override). Same page, two budgets.

This is not a recent regression: the 10 s budget is the same from 0.8.152 through 0.8.171. On a 130 s project with 30 sub-compositions and 1080p videos the page itself loads in under 200 ms. Only a slow external script reaches 10 s.

How

  • The separate diagnostic budgets are deleted. resolvePageNavigationTimeout() in the engine's config.ts now owns the page-navigation env read, and resolveConfig uses it. pageNavigationTimeoutMs() (utils/renderArgs.ts) calls it directly, without resolving the whole render config, which could print render's Windows streaming-encode warning during check. The budget is raised only by a command's own longer --timeout. A zero or invalid override falls back to the engine default instead of failing instantly.
  • openSettledCompositionPage (check, snapshot), layout, motion-shot and validate use it. validate's NAV_TIMEOUT_FLOOR_MS and its helper are gone, and its comment and timeout hint now say render shares the budget.
  • The cold-Chrome allowance from fix(cli): page loads stop timing out while a busy machine's Chrome warms up #5705 (30 s for Chrome to send the first request) is unchanged.

Test plan

  • Browser test added: captureCompositionFrame.slowScript.browser.test.ts opens a composition whose <head> script is answered after 15 s, through the same openSettledCompositionPage check and snapshot use. On main it fails with Navigation timeout of 10000 ms exceeded; with the fix it passes 3 runs in a row.
  • Unit tests: pageNavigationTimeoutMs follows the engine default and its env override, ignores a zero or invalid override, and is only raised by a longer --timeout. A further test sets win32, software GPU and one worker (the case where render disables streaming encode) and requires no stderr output. It fails at the previous head, where the full resolveConfig() printed render's warning. validate's floor test is removed with the floor. src/capture, src/utils, validate and layout pass 3 runs in a row (1845).
  • Manual testing performed: a project loading GSAP from a local server that answers after 15 s. On 0.8.170 and 0.8.171, snapshot fails with the 10 s navigation timeout while render succeeds. From this branch, snapshot (2 stills) and check (passed) both succeed.
  • Documentation updated (if applicable): not applicable.
  • Comments follow CONTRIBUTING.md "Comments": one doc line on the new function, one reworded option comment, one test comment.

…nder does

check, snapshot, layout, motion-shot and validate gave the page 10 s to reach
DOMContentLoaded while render gives it the engine's page-navigation timeout
(60 s), so a parser-blocking script from a slow CDN failed the diagnostics
with "Navigation timeout of 10000 ms exceeded" on projects that render fine.
The separate 10 s budgets are gone: every caller takes the engine's value
(and its env override), raised only by a command's own longer --timeout.
The cold-Chrome allowance for sending the first request is unchanged.
@github-actions

github-actions Bot commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Edit accuracy: accurate 271 (base branch 271), smooth 160 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (0)

Unstable (1)

  • resize-scale-px-r0-nested-z100-on: tracking 0.06, pressJump 0, drop 0.28, reload 0.28, render 0.02, renderKey 0.03, undo true, teleport true / tracking 0.06, pressJump 0, drop 0.28, reload 0.28, render 0.02, renderKey 0.03, undo true, teleport true / tracking 0.06, pressJump 0, drop 0.28, reload 0.28, render 0.02, renderKey 0.03, undo true, teleport true

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at ef9e4505. No blockers, approving.

What changed: the diagnostic budgets are gone: resolveDiagnosticNavigationTimeoutMs, validate's NAV_TIMEOUT_FLOOR_MS and resolveNavigationTimeoutMs. git grep finds no remaining callers. pageNavigationTimeoutMs(min) now returns max(resolveConfig().pageNavigationTimeout, min), falling back to DEFAULT_CONFIG (60 s) when the configured value is ≤ 0. I checked this against the PR's claims in engine/src/config.ts. envNum returns the fallback for an empty or non-finite value, so "invalid" gives 60 s, and "0" or a negative value gives 0, which then falls back to 60 s. Render reads the same field, so check, snapshot, layout, motion-shot and validate now share render's budget and override. Every importer of renderArgs.ts already imports @hyperframes/engine, so the new top-level import adds no load cost. The gotoWhenSent cold-Chrome allowance is untouched.

Behavior to know about (deliberate, not blocking): a page that really can't load now takes up to 60 s to fail check, snapshot, layout or motion-shot instead of 10 s. Validate's default goes from 10 s to 60 s.

Non-blocking nits:

  • pageNavigationTimeoutMs calls the full resolveConfig() on every navigation (layout navigates twice) only to read one field. resolveConfig also prints [hyperframes] Windows compound-workaround auto-detected, disabling streaming-encode … to stderr when a narrow set of conditions holds: win32, software GPU forced, and concurrency exactly 1. Under those env settings, check and snapshot would now print a render-only warning. Reading just that env value, or passing overrides that skip the branch, would avoid it.
  • validate.ts:53-56: the comment says "render tolerates but validate's tighter budget does not", and the hint text says "even when the full render succeeds". Both are stale now that the budgets match.

Tests: the new browser test answers /slow.js after 15 s and goes through the same openSettledCompositionPage that check and snapshot use. Its 90 s timeout leaves room for the 60 s budget. The renderArgs unit tests cover the default, the env override, zero and invalid overrides, and raise-only --timeout. The commit is signed and there is no .github/ diff.

CI is green at this head (run 39140195092; Windows 39140195206). All required checks pass, except Skills: manifest in sync, which was skipped because no skill files changed. In Test (runtime), captureCompositionFrame.slowScript.browser.test.ts ran and passed in 17.9 s, which is past the old 10 s budget, so the test really exercised the new budget. renderArgs.test.ts ran 30 tests, all passing.

No live run.

— Rames

…engine

resolvePageNavigationTimeout() owns the page-navigation env read for render
and the diagnostics, so check and snapshot no longer resolve the whole render
config, which printed render's Windows streaming-encode warning. validate's
comment and timeout hint now say render shares the budget.
@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Both nits folded in at 09fcdd4a:

  • Stray warning: resolvePageNavigationTimeout() in engine/src/config.ts now owns the page-navigation env read (same parsing), resolveConfig uses it, and pageNavigationTimeoutMs calls only that, so check and snapshot never resolve the full render config. New test: win32 + PRODUCER_BROWSER_GPU_MODE=software + PRODUCER_MAX_WORKERS=1 requires no stderr output. It failed at ef9e4505 (the warning printed once) and passes now.
  • validate: the comment and the timeout hint no longer say render's budget is looser. The hint names PRODUCER_PAGE_NAVIGATION_TIMEOUT_MS next to --timeout.

src/utils, src/capture, validate and layout pass 3 runs in a row (1845), and the engine config tests pass (113).

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review at 09fcdd4a. Both of my nits are addressed, so I'm approving again.

  • No full resolveConfig() per navigation: the engine now exports resolvePageNavigationTimeout(), which reads only PRODUCER_PAGE_NAVIGATION_TIMEOUT_MS. Unset or "" gives the default and a non-finite value gives the default, which is exactly the old envNum behavior. resolveConfig itself now calls it for pageNavigationTimeout, so render's value is byte-equivalent and the Lambda and render paths are unchanged. pageNavigationTimeoutMs uses it, so check and snapshot no longer run the Windows streaming-encode branch. The new test stubs win32, software GPU and PRODUCER_MAX_WORKERS=1, then asserts console.error is never called; the old code path would have printed there. Zero and negative overrides still fall back to 60 s through the existing > 0 guard.
  • Stale wording: the validate.ts comment and hint no longer call validate's budget the tighter one. The hint now says render shares the budget and names PRODUCER_PAGE_NAVIGATION_TIMEOUT_MS.
  • The browserFallback mock adds resolvePageNavigationTimeout. Both commits are signed.

CI: the check list shows the required jobs as "skipping" because duplicate runs 39142333351 and 39142333357 skipped every job. The real runs at this head passed all of them: CI 39142319172 (Test, Typecheck, Build, Test: runtime contract, Studio: edit accuracy gate) and Windows 39142318977 (Render on windows-latest, Tests on windows-latest). Semantic PR title, captures, reachability and regression also pass.

No live run.

— Rames

@miguel-heygen
miguel-heygen added this pull request to the merge queue Oct 18, 2026
Merged via the queue into main with commit 18fc390 Oct 18, 2026
119 of 120 checks passed
@miguel-heygen
miguel-heygen deleted the scrubfix/navdefault branch October 18, 2026 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants