fix(cli): honor check navigation timeout - #2860
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
d990288 to
0844818
Compare
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 0844818.
Clean, surgical fix. resolveDiagnosticNavigationTimeoutMs(env, minimumTimeoutMs = 0) returning Math.max(configured, minimumTimeoutMs) preserves every existing invariant:
- Default
--timeout 3000→Math.max(10_000, 3_000) = 10_000— the 10s floor still holds, matching the pre-fix behavior. --timeout 30_000→Math.max(10_000, 30_000) = 30_000— the actual fix, navigation now extends with the user's setting.PRODUCER_PAGE_NAVIGATION_TIMEOUT_MS=90_000+--timeout 30_000→Math.max(90_000, 30_000) = 90_000— env override still wins.- Undefined/negative minimum →
Math.max(default, 0)= default; the optional 2nd parameter is safe to omit for existing callers (captureCompositionFrame,inspect,snapshot, wherever else) that don't have a user-facing knob. Verified in code: onlyrunBrowserCheckandcaptureFindingCropswere modified to passnavigationTimeoutMs: options.timeout; no other call sites are affected.
Verified the timeout actually reaches page.goto({ timeout }) (unswallowed by the settled-page wrapper — openSettledCompositionPage passes it through directly at packages/cli/src/capture/captureCompositionFrame.ts:188).
Nits
- Test naming ambiguity. The new test in
renderArgs.test.tsreads "raises the diagnostic navigation budget to a larger caller-provided minimum" — but the second assertion (resolveDiagnosticNavigationTimeoutMs({}, 3_000) === 10_000) tests the case where the minimum is SMALLER than the default (the floor still holds). Consider a title that names both directions, e.g. "returns the greater of env-configured / caller-minimum / 10s floor," or split into two smaller tests. Minor. - Semantic shift the docs already flag. Users who set
--timeout 30_000to give the renderer more time now also implicitly extend the navigation budget above the 10s floor. Total worst-case wall time is30s navigation + 30s render-ready = 60s, up from10s + 30s = 40s. The--helpand docs update calls this out clearly ("also raises the page-navigation budget above its 10s floor") — so users have the visibility. Not a defect; just a semantic worth naming.
What I didn't verify
- Whether the
snapshottop-level command has its ownopenSettledCompositionPagecall site that should also thread the user's timeout — the PR body says "Snapshot finding-crop sessions use the same timeout contract," which is thecaptureFindingCropsinside the check flow, not the standalonesnapshotcommand. If snapshot's separate command has a--timeoutflag, it might benefit from the same treatment; scope-appropriate to defer.
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: APPROVE — grade A — rubric CORRECT
Root-cause fit: The fix flows --timeout (ms) from check's only two navigation entry points (runBrowserCheck + captureFindingCrops) into openSettledCompositionPage, which forwards it to resolveDiagnosticNavigationTimeoutMs as a Math.max(configured, minimum) floor over the existing env / 10s baseline. Every page.goto the check command touches routes through that one helper — complete single-choke-point fix, not a unit-partial patch.
Claims verified (each with file:line at HEAD):
- "add explicit navigation budget to shared opener" —
captureCompositionFrame.ts:64-67newnavigationTimeoutMs?field;:188threaded intopage.goto. - "resolve as max of global override and caller minimum" —
renderArgs.ts:126-134,Math.max(configured, minimumTimeoutMs). - "thread into main audit + snapshot crop" —
checkBrowser.ts:168+:229both passnavigationTimeoutMs: options.timeout. - "10s floor + env override intact" —
renderArgs.ts:132;renderArgs.test.ts:123asserts this. - Docs + CLI help + skill ref updated (
check.ts:88-91,cli.mdx:545,lint-validate-inspect.md:39). - +54/-7, 9 files — matches PR body claim.
Adversarial findings: None found. parseCheckOptions (check.ts:170) floors timeout at 500ms via Math.max(500, positiveInteger(...)), so NaN/negative/0 never reach the resolver. Undefined navigationTimeoutMs on unrelated callers (compare.ts:282, snapshot.ts:261, grade-compare.ts:557) hits the default = 0 param → preserves prior 10s behavior. Units are ms end-to-end. No empty catches, TODOs, or console noise in touched code.
CI state: All required checks green.
Suggested next step: APPROVE as-is. Follow-up (non-blocking): consider whether snapshot / compare / grade-compare should adopt the same --timeout-as-nav-floor contract for consistency — currently only check gets the elevated budget.
— Review by Via
What
Make
hyperframes check --timeoutextend both the initial render-ready wait and the page-navigation gate. The historical 10-second navigation floor andPRODUCER_PAGE_NAVIGATION_TIMEOUT_MSoverride remain intact. Snapshot finding-crop sessions use the same timeout contract.Why
Media-heavy checks could fail at the fixed 10-second navigation gate even when users passed a larger
--timeoutvalue. The CLI accepted the timeout for render readiness but did not apply it consistently to page navigation.How
Test plan
Commands:
bun run --cwd packages/cli test— 2221 passed, 2 skippedbun run --cwd packages/cli typecheckbunx oxlint <changed TypeScript files>bunx oxfmt --check <changed files>bun run --cwd packages/cli buildnode packages/cli/dist/cli.js check --help