Repository navigation
fix(cli): parse capture and grade-compare --timeout as a duration - #7715
Conversation
capture read --timeout with parseInt, so `--timeout 1s` became a 1 ms page-load timeout. grade-compare dropped an invalid value without a warning. Both now use the shared parseTimeoutMs helper that check and snapshot already use, keeping their defaults of 120000 ms and 5000 ms. Fixes heygen-com#7671
Preview approval requiredThis pull request comes from a fork, so Mintlify won't build a preview until someone with write access approves commit |
![heygengenesis[bot]](https://avatars.githubusercontent.com/in/2913576?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: COMMENT (needs human) · 0 blocker · 0 important · 1 nit
The code is ready to approve. CI hasn't run because this is a fork PR and every workflow is action_required. A maintainer needs to approve the workflow runs, and the required checks have to pass before this merges.
Scope
Read all 5 files end to end, plus utils/timeoutArg.ts, captureAttempt.ts and navigateForCapture.ts, to see how the timeout gets used. Ran it locally at aac5352: capture.test.ts, grade-compare.test.ts and timeoutArg.test.ts (64 passed); oxlint and oxfmt are clean, and tsc reports no errors in the changed files. No prior reviews to dedupe against.
— Genesis
| skipVision: args["skip-vision"] as boolean, | ||
| maxScreenshots, | ||
| timeout: args.timeout ? parseInt(args.timeout as string) : undefined, | ||
| timeout: parseTimeoutMs(args.timeout, DEFAULT_CAPTURE_TIMEOUT_MS), |
There was a problem hiding this comment.
nit: capture --timeout 0 changes meaning with no warning. Before, "0" is a truthy string, so it passed through as 0. navigateForCapture then ran goto with timeout: 0, which in Puppeteer means wait forever. Now parseTimeoutMs returns the 120000 fallback for ms <= 0 and prints nothing. It's undocumented, so this is fine if intended. If anyone relied on 0 to mean no limit, it's worth a line in the PR body, or a warning from the helper when the value is <= 0.
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at aac535278bf1ee0f110cd87615212d08e8074617.
Need: yes, interpret duration flags consistently · Size: ok · Reuse: existing parseTimeoutMs used by check/snapshot · Simpler: already minimal · Structure: fine (checked command argument and downstream timeout paths; no split)
Risk: low — localized argument parsing with unchanged default durations.
Thanks for covering both commands and updating their help and docs. capture.ts:242 now forwards milliseconds from the shared parser, and grade-compare.ts:294 uses the same conversion before the existing render-ready path. Defaults remain 120000 and 5000 ms; malformed durations warn on stderr, preserving JSON stdout. No blocking findings. As Genesis noted, the undocumented capture --timeout 0 now falls back to 120000 ms rather than disabling the navigation timeout; that compatibility note is nonblocking.
Validation: bun run build passes. On Node 24.22.2, vitest run --project unit src/commands/capture.test.ts src/commands/grade-compare.test.ts src/utils/timeoutArg.test.ts src/capture/navigateForCapture.test.ts --maxWorkers=2: 103 passed. Capture tests exercise the command forwarding milliseconds to captureWebsite; grade-compare tests exercise argument parsing, and I traced that value into the render-ready consumer. Restoring the two old parseInt expressions makes six timeout regressions fail; restoring this head makes all 11 timeout-selected tests pass. Oxlint and oxfmt checks pass on all four changed TypeScript files. Temporary edits restored.
CI workflows at this head are awaiting fork-run authorization (action_required); the available success checks do not mean the test workflows ran. I did not repeat the author's browser capture or grade-sheet walkthrough.
Verdict: APPROVE
Reasoning: The two affected commands now use the established duration parser, and the focused regression coverage exercises units, bare milliseconds, omitted arguments and malformed input.
— Rames
What
capture --timeoutandgrade-compare --timeoutnow accept a duration such as30sor2m, ascheckandsnapshotalready do. A value that is not a duration now prints a warning and uses the command's default.Why
Both commands read
--timeoutwith their ownparseInt. That turned1sinto 1 ms incapture, and it threw away invalid input ingrade-comparewithout saying so. The sharedparseTimeoutMshelper already handles units and warnings forcheckandsnapshot.Related work
Fixes #7671
How
packages/cli/src/commands/capture.ts:parseTimeoutMs(args.timeout, DEFAULT_CAPTURE_TIMEOUT_MS). The default stays 120000 ms. The CLI now passes it explicitly, where before it passedundefinedandcaptureAttemptfilled in the same value.packages/cli/src/commands/grade-compare.ts:parseGradeCompareArgsusesparseTimeoutMs(args.timeout, DEFAULT_RENDER_READY_TIMEOUT_MS). The default stays 5000 ms.--timeouthelp descriptions and their rows indocs/packages/cli.mdxnow mention units such as30s.Test plan
capture.test.tsruns the command withcaptureWebsitemocked.1s,2m,45000and an omitted flag reach capture as 1000, 120000, 45000 and 120000 ms.abcwarns and uses 120000.grade-compare.test.ts:parseGradeCompareArgsreads1s,1500ms,8000and an omitted flag as 1000, 1500, 8000 and 5000 ms.abcwarns and uses 5000.main:1sand2m(they become 1 ms and 2 ms), the omitted capture timeout (undefined), andabcin both commands (no warning).capture --local-fileon a small page:--timeout 1ms(what1sused to become) fails withcapture navigation timed out.--timeout 1sloads the page and writes the capture.--timeout abcwarns and captures with the 120000 ms default.grade-compareon a 320×180 frame with two grades:1s,1msand an omitted flag all write the comparison sheet.abcwarns and uses 5000 ms.--helpoutputs show the new descriptions.docs/packages/cli.mdx, both--timeoutrows.Also run:
oxlint,oxfmt --check,tsc --noEmit(cli, core, studio, scripts),fallow audit --base upstream/main,comment-ratchet,check-comment-citations,check:cli-process-ownership. All clean.Not covered
compare --timeoutstill uses its own localparseTimeoutMsincompare.ts. It wasn't part of cli:capture --timeout 1smeans 1 ms andgrade-compare --timeout abcsilently becomes 5000 #7671, so I left it alone.