fix(capture): bound scroll/evaluate timeouts so heavy pages keep capturing by xuanruli · Pull Request #3236 · heygen-com/hyperframes

xuanruli · 2026-08-11T23:23:58Z

Summary

Drive lazy-scroll from Node in small steps (with a time budget) instead of one long in-page async evaluate, and reuse that for collectAnimationCatalog (it had a second unbounded full-page scroll that hung on Fonts-class sites).
On Runtime.evaluate / protocol timeouts, degrade and continue (content check, animation catalog, screenshots) instead of failing the whole capture; set capture protocolTimeout from the nav/post-nav budgets.
Fix BLOCKED.md copy so extraction/protocol timeouts are no longer mislabeled as “navigation timed out / blocking headless”.

Context

Follow-up to #3224 (networkidle2 → domcontentloaded fallback). That fixed Yahoo-style nav idle hangs. Fonts/Vercel still failed after navigation during core-extraction when a single evaluate hung until protocol timeout.

Validation

Unit tests: captureTimeout, lazyScrollForCapture, navigateForCapture, pageBlockDetection (30 passing)
Smoke: fonts.google.com capture OK (~55s) on this branch
Regression: 20 previously-working sites re-captured with the new CLI → 20/20 OK (CNN, Yahoo, Wiki, NASA, Netflix, Apple, BBC, NYT, GitHub, Shopify, etc.)

Test plan

cd packages/cli && bunx vitest run src/capture/captureTimeout.test.ts src/capture/lazyScrollForCapture.test.ts
hyperframes capture https://fonts.google.com --skip-vision succeeds
hyperframes capture https://example.com --skip-vision still succeeds
Confirm a real bot-block (e.g. Tesla Access Denied) still writes a blocked/access-protection failure, not the new extraction-timeout wording

Made with Cursor

…uring Fonts/Vercel-class sites were failing after navigation when a single in-page scroll/evaluate hung until protocolTimeout. Drive lazy scroll from Node, degrade on evaluate timeouts, and stop labeling those failures as bot blocks. Co-authored-by: Cursor <[email protected]>

james-russo-rames-d-jusso

Reviewed at 55de4afe.

The follow-up to #3224 lands cleanly and closes the second half of the capture-timeout picture: navigation was bounded by that PR's networkidle2 → domcontentloaded fallback; this one bounds every post-navigation page.evaluate with a per-call protocolTimeout, degradable-on-timeout wrappers around the three heavy stages (content check, animation catalog, scroll screenshots), and a Node-driven, deadline-aware scroll loop that replaces the single-shot in-page async. The composition is well-thought-out — remainingMs() shrinks per-step budgets across stages, protocolTimeout (captureTimeout.ts:1-5) sets a hard per-evaluate ceiling on top. Nothing about the shape looks wrong.

Five concerns, none blocking. Inlined at the anchors.

Composition with #3224 — worth naming

timeout (default 120_000) now bounds navigation only. Post-navigation is a separate postNavigationBudgetMs budget (also default 120_000). Total wall-clock worst-case is closer to nav + settleTime + postNav ≈ 243s, not timeout. That's pre-existing behaviour, but with captureProtocolTimeoutMs now derived from both budgets it's newly visible — a one-line comment on the launch-time protocolTimeout at index.ts:128 naming which budget bounds what would keep future tuners honest.

What I didn't verify

Did not run the fonts.google.com or Yahoo captures locally; trusting the PR body's 20/20 OK regression assertion and the smoke-passing CI on this SHA.
All 8 required checks are green on the ruleset (Semantic PR title, Test: runtime contract, Typecheck, Build, regression, Test, Render on windows-latest, Tests on windows-latest). One CI job is named CLI smoke (required) and shows FAILURE at 55de4afe — but that specific job is not in the branch ruleset's required list, Smoke: global install is passing, and the PR shows mergeable_state: blocked only on reviews. The (required) suffix in the job name is misleading; worth either renaming the job or dropping the suffix so future readers don't chase a false-positive block.
Did not construct a synthetic "hidden text only" page to prove out the TreeWalker visibility concern below.
Did not run the full downstream pipeline against an empty screenshots[] to confirm the contact-sheet + storage paths tolerate zero screenshots gracefully — index.ts:849 gates the contact sheet on screenshots.length > 0 which is the main path, but there may be other consumers.

Verdict

LGTM from my side at 55de4afe. Composition with #3224 is clean, tests cover the timeout branches on each new module, degradation paths only swallow protocol-timeout errors and re-throw everything else. Concerns are all shape-level; none block. Not stamping.

— Review by Rames D Jusso

james-russo-rames-d-jusso · 2026-08-12T00:04:18Z

+  return /Runtime\.evaluate timed out|protocolTimeout|protocol timeout/i.test(msg);
+}
+
+function isNavigationTimeoutErrorMessage(errMsg: string): boolean {


🟠 Duplicate navigation timeout regex across files — drift risk. isNavigationTimeoutErrorMessage(errMsg) here is byte-identical in intent to isNavigationTimeoutError(err) at packages/cli/src/capture/navigateForCapture.ts:25-28 (landed in #3224). If Puppeteer ever reworded the message and only one of the two regexes was updated, formatCaptureFailureReason would silently mis-classify. Recommend exporting the existing helper from navigateForCapture.ts (or extracting both to a shared captureErrors.ts) and having formatCaptureFailureReason call through it. — Rames D Jusso

james-russo-rames-d-jusso · 2026-08-12T00:04:18Z

+      var bodyChildCount = body ? body.children.length : 0;
+      var text = "";
+      if (body) {
+        var walk = document.createTreeWalker(body, NodeFilter.SHOW_TEXT, null);


🟠 TreeWalker counts hidden text where the old body.innerText respected CSS visibility — semantic change to blocked-page detection. document.createTreeWalker(body, NodeFilter.SHOW_TEXT) walks every text node regardless of display:none / visibility:hidden / ARIA-hidden container / <template> contents. Previously body.innerText respected all of those. A challenge / holding page whose visible text is <100 chars but has, say, a hidden nav menu or ARIA-hidden template with ≥100 chars of text would now register textLength ≥ 100 and skip both detectBlockedPage() and the low-content SPA warning at index.ts:314. Anti-bot pages don't typically have hidden template text, so real-world impact is likely small — but if the intent is visible content length, this is the wrong primitive. body.innerText in a separate try (kept behind the same protocol-timeout catch) would preserve the semantics. — Rames D Jusso

james-russo-rames-d-jusso · 2026-08-12T00:04:19Z

+
+export function isProtocolEvaluateTimeoutError(err: unknown): boolean {
+  const msg = err instanceof Error ? err.message : String(err);
+  return /Runtime\.evaluate timed out|protocolTimeout|protocol timeout/i.test(msg);


🟡 Message-string regex is a moving target where instanceof is stable — same pattern I flagged on #3224. puppeteer-core@^25.2.1 exports both TimeoutError and ProtocolError classes; a Runtime.evaluate protocol timeout throws one of them. err instanceof TimeoutError (or ProtocolError) as the primary check would survive a Puppeteer version bump that reworded the message. The three-branch OR (Runtime\.evaluate timed out|protocolTimeout|protocol timeout) is defensive but each new branch adds surface. Recommend instance-check primary + regex fallback for the string-input path (formatCaptureFailureReason accepts errMsg: string, so both callers matter). — Rames D Jusso

james-russo-rames-d-jusso · 2026-08-12T00:04:19Z

+      `Array.from(document.images).filter(function(img) { return !img.complete; }).length`,
+    )) as number;
+    if (pending > 0) {
+      await sleep(imageWaitMs);


🟡 Image wait no longer races on load / error — sleeps the full imageWaitMs even when images finish in 500ms. The old in-page code did Promise.race([Promise.all(imgs.map(onload/onerror)), setTimeout(imageWaitMs)]), returning as soon as all pending images fired. This unconditionally sleeps imageWaitMs (up to 5s) once pending > 0. On fast-loading pages that just happen to have any image in-flight at check time, that's up to 5s of wasted budget per capture. Pollable variant that preserves the racing behaviour without re-entering the browser: while (pending > 0 && Date.now() < deadline) { await sleep(250); pending = await page.evaluate(...); }. — Rames D Jusso

james-russo-rames-d-jusso · 2026-08-12T00:04:19Z

@@ -0,0 +1,30 @@
+export function captureProtocolTimeoutMs(navTimeoutMs: number, postNavBudgetMs: number): number {
+  const nav = Number.isFinite(navTimeoutMs) ? Math.max(0, navTimeoutMs) : 0;


🟡 Infinity/non-finite inputs silently collapse to 0, then get clamped to the 60s floor. Number.isFinite(Infinity) is false, so nav = 0 and budget = 0, and protocolTimeout = max(60_000, 0, 0) = 60_000. A caller who passed timeout=Infinity intending "no protocol ceiling" would silently get a 60s per-call cap (tighter than Puppeteer's own 180s default). If the intent of the non-finite path is "use a sensible max", the branch should return max(60_000, PUPPETEER_DEFAULT_PROTOCOL_TIMEOUT_MS) or similar, not 60_000. If Infinity isn't a supported input, an early throw would surface the misuse rather than silently clamping. — Rames D Jusso

miguel-heygen

Reviewed at exact head 55de4afecc454c31bd8ba767138b98b0445fe273.

The direction is good: the page-side scroll loop is now Node-driven, capture failures are classified more clearly, and the new focused tests pass locally (11/11). I also reviewed the interaction with #3224 and the downstream degradation paths.

Blocking findings

The advertised 15s/8s stage budgets do not bound a pending page.evaluate. lazyScrollForCapture checks the deadline only before/after each awaited evaluate (lines 62-90), while Chromium is launched with a global 120s protocol timeout. A single wedged step can therefore consume 120s despite a 15s budget, and the recovery scrollTo(0, 0) can consume another 120s after the deadline. I reproduced this on this head with a never-resolving evaluate mock: a 10ms-budget call was still pending after 75ms. The new tests only use immediately resolving/rejecting mocks, so they do not exercise the contract the change claims to provide.

Please enforce the remaining stage budget around each protocol call (or otherwise abort/replace the page when it expires), avoid issuing another full-timeout CDP call after the deadline, and add a never-resolving-evaluate regression test.
Two new outer timeout-warning paths are unreachable, so degraded output is silently reported as success. collectAnimationCatalog catches an evaluate timeout internally and returns an empty catalog (animationCataloger.ts:148-214), so the warning at index.ts:446-452 never runs. captureScrollScreenshots catches every exception internally (screenshotCapture.ts:126-262), so the warning at index.ts:466-472 never runs either. That makes a timed-out scan indistinguishable from a page with genuinely zero animations/screenshots. Return a typed degraded result or let protocol timeouts reach the caller, and pin the warning/result behavior in tests.

Non-blocking follow-ups

The content-check timeout currently skips even the already-known HTTP-status 401/403/429 signal; status-only blocking should remain enforceable without DOM data.
Pending images now force the entire remaining 5s sleep instead of resolving on load/error, adding avoidable latency on fast pages.
The failed CLI smoke (required) run was setup-bun network infrastructure; its rerun is in progress. The actual required smoke and all other required ruleset checks are green.

Verdict: CHANGES_REQUESTED
Reasoning: The patch makes expensive extraction degradable, but its named stage budgets are not hard bounds and the two key degradation warnings cannot execute, so heavy/wedged pages can still overrun by minutes while returning silently incomplete artifacts.

Magi

Bound each scroll/content evaluate with remaining stage time so a wedged page.evaluate cannot outlive the advertised 15s/8s budgets, skip recovery CDP calls after expiry, and return/propagate timed-out animation and screenshot work so caller warnings are reachable. Co-authored-by: Cursor <[email protected]>

xuanruli · 2026-08-12T00:17:43Z

Addressed Magi blockers + Rames notes in 1048a24:

Stage budgets now bound pending evaluates via withRemainingBudget / Promise.race around each scroll/content CDP call; recovery evaluate is skipped after the deadline. Added a never-resolving-evaluate regression test (lazyScrollForCapture returns degraded within ~budget).
Degradation is visible: collectAnimationCatalog returns { catalog, timedOut } so the caller warning fires; captureScrollScreenshots rethrows protocol/stage-budget timeouts instead of swallowing them (pinned in degrade tests).
Rames polish: shared isNavigationTimeoutError, TimeoutError instanceof check, restored innerText for blocked-page detection (with budget + status-only fallback), image wait polls instead of sleeping full 5s, non-finite protocol timeout uses Puppeteer default 180s.

Use puppeteer-core TimeoutError as the primary signal for navigation vs evaluate/protocol timeouts, with message checks only as a fallback for string formatting and non-TimeoutError cases. Co-authored-by: Cursor <[email protected]>

miguel-heygen

Fresh-head re-review at exact commit a5e7a755222f54a8e398df1ae5c9568fd2cb783a.

The two blockers from my review at 55de4afe are resolved:

captureTimeout.ts:57-81 adds a real Promise.race stage deadline, and every lazy-scroll protocol call now consumes the remaining deadline (lazyScrollForCapture.ts:18-24, 87-137). The never-resolving regression at lazyScrollForCapture.test.ts:29-43 proves a wedged evaluate returns inside the stage budget, while :45-59 proves no recovery CDP call is issued after expiry.
Degradation is observable. animationCataloger.ts:139-153, 221-249 returns a typed { catalog, timedOut } outcome consumed by the caller warning at index.ts:444-456; screenshotCapture.ts:201-205, 265-269 rethrows degradable timeouts so index.ts:471-483 emits its warning. Both paths now have direct regression tests.

The follow-up also addresses the prior non-blockers cleanly: the content check retains HTTP-status blocking after a DOM timeout (index.ts:274-312), image settling polls rather than sleeping the full allowance (lazyScrollForCapture.ts:43-59), hidden-text semantics are restored with body.innerText (index.ts:276-286), timeout classifiers are shared and TimeoutError-first (captureTimeout.ts:26-54; navigateForCapture.ts:1-3), and non-finite protocol budgets fall back to Puppeteer's 180s default.

Verification: all eight exact-head required checks are terminal success. I ran the focused capture suite locally: 39/39 tests passed across timeout, lazy-scroll, animation degradation, screenshot degradation, navigation, and blocked-page detection.

Verdict: APPROVE
Reasoning: The fresh head enforces the advertised stage deadlines, surfaces incomplete animation/screenshot work instead of silently succeeding, preserves status-only block detection, and is covered by direct never-resolving and degradation-path tests with required CI green.

Magi

james-russo-rames-d-jusso reviewed Aug 12, 2026

View reviewed changes

miguel-heygen requested changes Aug 12, 2026

View reviewed changes

miguel-heygen approved these changes Aug 12, 2026

View reviewed changes

xuanruli merged commit 43dba22 into main Aug 12, 2026
48 checks passed

xuanruli deleted the fix/capture-evaluate-timeout-resilience branch August 12, 2026 01:26

Navigation Menu

Repository navigation

fix(capture): bound scroll/evaluate timeouts so heavy pages keep capturing - #3236

fix(capture): bound scroll/evaluate timeouts so heavy pages keep capturing#3236
xuanruli merged 3 commits into
mainfrom
fix/capture-evaluate-timeout-resilience

xuanruli commented Aug 11, 2026

Uh oh!

james-russo-rames-d-jusso left a comment

Uh oh!

james-russo-rames-d-jusso Aug 12, 2026

Uh oh!

james-russo-rames-d-jusso Aug 12, 2026

Uh oh!

james-russo-rames-d-jusso Aug 12, 2026

Uh oh!

james-russo-rames-d-jusso Aug 12, 2026

Uh oh!

james-russo-rames-d-jusso Aug 12, 2026

Uh oh!

miguel-heygen left a comment

Uh oh!

xuanruli commented Aug 12, 2026

Uh oh!

miguel-heygen left a comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		@@ -0,0 +1,30 @@
		export function captureProtocolTimeoutMs(navTimeoutMs: number, postNavBudgetMs: number): number {
		const nav = Number.isFinite(navTimeoutMs) ? Math.max(0, navTimeoutMs) : 0;

Conversation

xuanruli commented Aug 11, 2026

Summary

Context

Validation

Test plan

Uh oh!

james-russo-rames-d-jusso left a comment

Choose a reason for hiding this comment

Composition with #3224 — worth naming

What I didn't verify

Verdict

Uh oh!

james-russo-rames-d-jusso Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

james-russo-rames-d-jusso Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

james-russo-rames-d-jusso Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

james-russo-rames-d-jusso Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

james-russo-rames-d-jusso Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

miguel-heygen left a comment

Choose a reason for hiding this comment

Blocking findings

Non-blocking follow-ups

Uh oh!

xuanruli commented Aug 12, 2026

Uh oh!

miguel-heygen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants