fix(capture): fall back from networkidle2 to domcontentloaded on nav timeout - #3224

The extraction is clean: packages/cli/src/capture/index.ts:242-251 keeps warnings/progress at the integration boundary, and navigateForCapture.test.ts:50-80 pins the normal fallback plus the non-timeout passthrough rather than swallowing arbitrary navigation errors.

blocker — the fallback can exceed the caller's total navigation timeout. packages/cli/src/capture/navigateForCapture.ts:53 computes Math.max(totalTimeoutMs - networkIdleTimeoutMs, networkIdleTimeoutMs). Once the first attempt is capped at 30s, every caller timeout in (30s, 60s) receives another 30s instead of the remaining budget. For example, --timeout 31000 can spend ~30s on networkidle2 and then ~30s on domcontentloaded, nearly doubling the documented page-load timeout. This also contradicts the PR body’s “using the remaining budget” contract. Use the positive remainder (totalTimeoutMs - networkIdleTimeoutMs) and add a discriminating test around 31s or 45s that asserts the second goto receives 1s or 15s respectively. The existing 120s test only exercises the branch where max(remaining, attempt) happens to return the correct 90s.

CI was still in flight during review; no completed exact-head check was failing when I posted. The code blocker is independent of CI.

Verdict: REQUEST CHANGES
Reasoning: The fallback behavior is otherwise well-contained and tested, but its timeout calculation violates the caller-visible total budget for a reachable range of --timeout values.

— Magi

Exact-head follow-up at 31eaa375afdd235ad84a85d9ab39717160d4f808.

The prior blocker is closed at packages/cli/src/capture/navigateForCapture.ts:53: the fallback now receives exactly totalTimeoutMs - networkIdleTimeoutMs, so the two navigation attempts remain inside the documented caller budget. The new table test at navigateForCapture.test.ts:75-98 is discriminating rather than duplicative: it pins both 31s → 1s and 45s → 15s, the range where the prior Math.max implementation failed while the existing 120s case stayed green.

I re-read the complete two-file fix delta and found no new issue. The exact-head required context (regression) is terminal-success; no required context is pending or failing.

Verdict: APPROVE
Reasoning: The reachable timeout-budget violation is fixed with boundary-focused regression coverage, and the exact-head required gate is green.

— Magi

Reviewed at 31eaa375.

Small, focused pattern — try networkidle2 for a capped attempt, fall back to domcontentloaded only on Puppeteer navigation timeout, keep the settle + scroll path downstream. Retry semantics (not replace), non-timeout errors re-throw, warnings + progress signalling on the fallback path.

The amend from a6246ded → 31eaa375 closes the fallback-timeout overshoot Magi flagged: navigateForCapture.ts:53 is now totalTimeoutMs - networkIdleTimeoutMs, and the new it.each at navigateForCapture.test.ts:75-95 pins the --timeout=31_000 → 1_000 and --timeout=45_000 → 15_000 cases explicitly. Contract now matches the PR body's "remaining budget" wording. Verified against the four cases that matter: 120_000 → 30 + 90 (default), 31_000 → 30 + 1 (tight but honoured), 30_000 → 30 + throw (guard fires, no fallback), and the non-timeout ERR_CONNECTION_REFUSED re-throw. All consistent.

Concerns — not blocking, worth an owner

• Semantic change to the networkidle2 budget is worth pinning against the PR's own test plan. Pre-PR, the full caller timeout (default 120s) was available to networkidle2. Post-PR, capped at NETWORK_IDLE_ATTEMPT_MS = 30_000. The unchecked box in the PR body — "a quiet marketing site — should still finish on the networkidle2 path with no fallback warning" — is now conditional on the site settling under 30s. Any site whose real steady-state is 30-60s (heavy React/analytics but ultimately quiet) will now fall over to domcontentloaded where it previously succeeded on networkidle2, and the resulting screenshot depends on settleTime (3s default) + the 15s lazy-scroll budget being enough. Worth naming a concrete site whose measured networkidle2 time is >30s and confirming the fallback path still captures acceptable-quality output — not just that it doesn't warn.

• isNavigationTimeoutError is a message-string regex against a moving target. navigateForCapture.ts:25-28 does /navigation timeout/i on err.message. puppeteer-core@^25.2.1 (per packages/cli/package.json) exports TimeoutError — a stable, instanceof-checkable class. A future Puppeteer bump that reworded the message (or a middleware that wrapped it) would silently disable the fallback and re-introduce the yahoo.com hang, and the test wouldn't notice because it asserts on the same string it emits. err instanceof TimeoutError as the primary check, with the regex as a secondary for wrapped errors, would be more durable.

• Root-cause reminder — this is a bandaid, not a fix, and worth marking as such. yahoo.com's networkidle2 hang is symptomatic of a class of pages (Yahoo, Reddit, most ad-heavy sites) that hold >2 in-flight requests indefinitely because of third-party heartbeats / analytics / real-time chat widgets. The longer-term fix is likely per-origin filtering via page.setRequestInterception (block or throttle known-noisy third-party origins during capture, or filter idle-tracking to first-party origins only). The fallback is the pragmatic call for now, but the underlying "capture accepts a genuinely-not-quiet page" seam remains — a follow-up ticket in the same milestone would keep the debt visible.

Nits

• No aggregate observability on the fallback. The warning lands in warnings[] and via progress("warn", ...), both consumer-visible surfaces. There's no structured metric or phase event carrying the fallback fact, so "how often does the fallback fire across capture jobs" isn't answerable without log-scraping. If capture is metered anywhere (Datadog / PostHog), a counter increment on fellBackFromNetworkIdle=true would keep the signal alive.
• NETWORK_IDLE_ATTEMPT_MS = 30_000 has no rationale comment. The 30s value is load-bearing (see the semantic-change concern above); a one-line comment naming the tradeoff — long enough for most modern SPAs to settle, short enough to bound a well-behaved page from being punished by an outlier tail — would keep future tuners honest.

What I didn't verify

• Did not repro yahoo.com or a quiet marketing site locally; trusting the PR body's repro + the observability the warning gives operators.
• Did not confirm puppeteer-core@^25.2.1's TimeoutError export at runtime, only via the pinned version + upstream reference.
• Did not run the fresh Windows/Linux CI on 31eaa375 — some jobs were still in progress when I posted; the code review is independent of CI.
• Did not check whether the pre-existing pageContentCheck.textLength < 100 low-content warning downstream would flag a domcontentloaded-fallback-captured page that failed to render its JS-driven content — but that path exists (index.ts:286-293) and would catch the most obvious "we captured a shell" regression.

Verdict

LGTM from my side at 31eaa375. Magi's blocker is resolved by the amend; my concerns above are non-blocking. Not stamping.

— Review by Rames D Jusso