Repository navigation
fix(cli): check waits for the composition as long as snapshot and render do - #6947
miguel-heygen wants to merge 4 commits into
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
There was a problem hiding this comment.
Verdict: APPROVE · 0 blocker · 1 important · 1 nit · head 9d3c07ca
The default check timeout now comes from one place, and the body has the fails without the fix: line naming the new test.
- important: this change makes the timeout hint wrong.
packages/cli/src/utils/checkBrowser.ts:385and:542still sayraise --timeout (for example 20s). Now that the default is 45 s, a user who hits that warning and follows the hint would lower the budget. Change the example to something above the default (e.g.2m) or drop the number. - nit: the doc comment at
checkBrowser.ts:197still says check's render-ready wait "defaults to 3000ms (DEFAULT_CHECK_OPTIONS.timeout)".
Scope
Read the full diff. Traced options.timeout at head through idleWindow (still max(120s, timeout+10s), so 120 s), the navigation and render-ready timeouts of openSettledCompositionPage (×3), and auditClipDurations. That last one now waits up to 45 s for media that never loads, which matches what snapshot and render allow. The cli package already depends on @hyperframes/engine, and DEFAULT_CONFIG.playerReadyTimeout is the fixed 45_000, separate from env resolution. CI and Docs were still running when I reviewed. No earlier reviews.
— Genesis V2
Edit accuracy: accurate 271 (base branch 271), smooth 114 of thoseThe gate passes. Quarantined, measured but not gated (0) |
9d3c07c to
8bf6fd3
Compare
There was a problem hiding this comment.
Verdict: REQUEST CHANGES — 2 resolved, 0 open, 1 new (blocker) · 9d3c07ca → 8bf6fd36
The 20s hint and the 3000ms comment are fixed. The blocker is that this head breaks two checks in packages/cli/src/utils/checkBrowser.ts:
- Preflight
format:checkfails on this file (job). The cause is the double blank line afterRENDER_READY_WARNING_SUFFIXand the tworenderReadyWarningSuffix:values split onto their own lines. Preflight failing also turns Windows render verification and preview-regression red. Fix:bun run format. - Comments ratchet fails (run):
checkBrowser.tscomment share rose from 4.0% to 4.1%, and a file's share may only go down. The new/** What a render-ready warning tells… */line is the added comment. Drop it; the constant's name already says it.
— Genesis V2
What
hyperframes checknow waits for the composition to become render-ready for 45 s by default, the same constantsnapshotandrenderuse (DEFAULT_CONFIG.playerReadyTimeout). The--timeoutflag default andDEFAULT_CHECK_OPTIONS.timeoutboth come from it; the docs line is corrected (it said 3000).Why
On slow machines and heavy scenes (a 4-core, 4 GB Windows laptop; a local 3D scene)
checkfailed with "Runtime did not become render-ready within 10000ms" whilesnapshotandrenderof the same project were allowed 45 s. The 10 s lived in two places (the option default and the flag default), apart from the constant the other commands use.Related work
Refs the report that
checkshould wait while the page still makes progress and fail only when it stalls; this change only removes the disagreement with snapshot/render and does not add progress detection.How
DEFAULT_CHECK_OPTIONS.timeouttakesDEFAULT_CONFIG.playerReadyTimeout; the flag's default isString(DEFAULT_CHECK_OPTIONS.timeout), so there is one owner. An explicit--timeoutstill wins.Test plan
Unit test added:
waits for the composition as long as snapshot and render do unless --timeout says otherwise(packages/cli/src/commands/check.test.ts). fails without the fix: before the change it received 10000 where 45000 was expected; after,check.test.ts157 passed,checkBrowser.test.ts68 passed.Manual testing performed: not run on a slow machine.
Documentation updated (if applicable)
Comments follow CONTRIBUTING.md "Comments"
Review fix: the warning that tells a person to raise
--timeout(checkBrowser.ts, two places) named20s, which now lowers the 45 s default. It is one exported constant (RENDER_READY_WARNING_SUFFIX) whose example is twice the default (90s), and the stale3000msdoc comment now says 45 s. fails without the fix:names a --timeout above the default, so following it never lowers the wait(checkBrowser.test.ts) is red alone with the example shrunk (expected 18000 to be greater than 45000);checkBrowser.test.ts+check.test.ts226 pass.Size
A lone one-constant fix (the change is the constant, one test and one docs line); nothing open to fold it into.