Repository navigation
fix(cli): capture takes a refused screenshot once more before naming it - #7222
Conversation
![heygengenesis[bot]](https://avatars.githubusercontent.com/in/2913576?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes at 5a9096ec. The one blocker is CI; the change itself looks right.
Blocker: the Comments workflow fails at this head (comment-ratchet, merge-base d452a6fad):
NEW file over its package's comment share:
packages/cli/src/utils/screenshotOnceMore.tsis 13.3% (2/15), the package is 9.7%
The function was moved without comments, and this PR added a new two-line JSDoc to it. Cutting that to one line (1/14 = 7.1%) passes, for example /** Retries once on a transient Chrome refusal; a second failure rethrows the first error. */. Dropping it also works, since the function name already says most of it.
What I checked
- Fails-without-the-fix line: present and accurate. With only
capture/screenshotCapture.tsreverted to main, both named tests fail (takes again, once, a tile Chrome refused…andnames the tile Chrome refused twice…, 2 failed / 31 passed). With the PR, they pass. - Local run passes:
screenshotCapture,screenshotOnceMoreand everycommands/snapshot*unit test (14 files, 224 passed), matching the body. - Move: the move is clean.
snapshot.tswas the only importer, andsnapshot.test.ts's source-string assertion still matches.capture/already imports@hyperframes/enginestatically (assetDownloader.ts), so the new import adds no load cost. - Behaviour: a non-transient error still fails on the first try, and a double refusal still lands in
failuresunder its tile name.
Non-blocking
isTransientBrowserErroralso matchesTarget closed,Session closedandbrowser has disconnected. Once the browser is gone, every remaining tile now makes two quick failing calls instead of one. That's harmless, andsnapshotalready behaves the same way.
Other CI at this head: PR captures, regression and Player perf are green. CI, CodeQL, preview-regression and Windows render verification were still running.
There was a problem hiding this comment.
Approving at ca3253a2.
The only change since my review at 5a9096ec is the doc comment in packages/cli/src/utils/screenshotOnceMore.ts, now one line: 1 insertion, 2 deletions, no code changed. That was the sole blocker, and the Comments workflow now passes at this head. Everything else from the earlier review carries over unchanged: the fails-without-the-fix line is verified (both named tests fail on main's screenshotCapture.ts), and the 224 local unit tests pass.
CI at this head: Comments, regression, PR captures, Player perf and preview-regression are green. CI, CodeQL and Windows render verification were still running at approval, and their runs on the previous head were cancelled by this push. Those checks still gate the merge.
Edit accuracy: accurate 267 (base branch 267), smooth 114 of thoseThe gate passes. Quarantined, measured but not gated (8)
Unstable (1)
|
There was a problem hiding this comment.
Approving at 7e7e5ffa.
What changed since my approval at ca3253a2: the only change is in packages/cli/src/capture/coreExtractionPhase.test.ts, a test file; the production code is the same as at the approved head. At ca3253a2, CI failed on Linux (Test (cli 1/2)) and on Windows (engine-cli-1). Both failures were the same test: core extraction screenshot budget > names the screenshot Chrome refused and keeps the rest. It refused a single call, and that call is now retried and succeeds. The test now refuses calls 2 and 3 (the first try and the retry for scroll-100.png) and is renamed to "…refused twice…". It still asserts the same named warning and that the other shots are kept. This mirrors the screenshotCapture.test.ts change already in the PR.
I missed this test in my earlier local run because I only ran screenshotCapture, screenshotOnceMore and snapshot*. This time I ran the whole src/capture unit suite on this head: 55 files, 625 tests passed.
CI at this head: CI passed, including Test (cli 1/2) and (cli 2/2). Windows render verification passed, including engine-cli-1 and engine-cli-2. Comments, CodeQL, regression, Player perf, preview-regression and PR captures are also green.
What
capturenow takes a refused screenshot once more before giving up on it, assnapshotalready does. A shot that is refused twice is still named in the JSONwarnings(could not capture screenshots/scroll-055.png (scrolled to 3780px): ...), and the capture goes on to the next tiles and the full-page image.Why
A Mac report:
capture <site> --skip-visiongot 13 of 21 screenshots and no full-page image, each missing one refused withProtocol error (Page.captureScreenshot): Unable to capture screenshot. The engine already counts that message as a passing browser error (isTransientBrowserError), andsnapshotretries it once;captureskipped the shot on the first refusal.This is a mitigation. The root cause is still being looked for: 15 captures of 5 long public pages on a GitHub
macos-latestarm64 runner (0.9.13, Node 24) had no refusal at all, and Linux had none either.Related work
None open on this path.
Size
A lone fix: no open PR changes
capture's screenshots, so nothing else can carry it.How
screenshotOnceMoremoves fromcommands/snapshot.tstoutils/screenshotOnceMore.ts, unchanged, and its test moves with it.snapshotimports it from there.captureScrollScreenshotsandcaptureFullPagePlatecallpage.screenshotthrough it. Only a passing refusal is retried; any other error fails at once, as before. The retry reports the first error if the second try fails too.Test plan
Unit tests added/updated
Manual testing performed
Documentation updated (if applicable)
Comments follow CONTRIBUTING.md "Comments": they say why, not what, and a bug fix says what the code must do and how to reproduce the bug
New:
takes again, once, a tile Chrome refused, so a passing refusal costs no shot. One refusal, and every tile and the plate are written with no failure.Changed: the named-failure tests in
screenshotCapture.test.tsandcoreExtractionPhase.test.tsnow refuse the same tile twice, since one refusal no longer loses it.fails without the fix: both tests fail on main's
screenshotCapture.tsand pass here.Every
capture/test andscreenshotOnceMore.test.ts(629 passed) 3 runs in a row, and everycommands/snapshot*unit test (224 passed), on Linux. oxfmt and oxlint are clean on the changed files.Not done: a real capture with a refusal, because no machine I ran on reproduces one. The retry runs only on that error, so pages that capture fine today take the same path.