Repository navigation
test(browser): wait for the popup fixture's first paint during startup, not in the first test - #30854
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 Walkthrough
Priority: ⬇️ Low Merge Risk: 🟡 Moderate · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 4 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title is concise, specific, and accurately describes the main change: moving the popup fixture's first-paint wait into startup. Description check The description is complete and relevant. It explains the problem, change, rationale, alternatives, testing, scope, and checklist items. The maintainer exception reasonably supports marking the linked… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description is complete and relevant. It explains the problem, change, rationale, alternatives, testing, scope, and checklist items. The maintainer exception reasonably supports marking the linked issue as N/A.
-
✨ Finishing Touches: 💡 2
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
2e35d28f-da0d-497e-8db2-3104bdf2d850
📒 Files selected for processing: 3
.github/workflows/pr.ymlsrc/main/browser/browser-popup-compatibility-cdp-fixture.tssrc/main/browser/popup-flake-diagnostic-fixture.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.
…and the main process
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
1e2201bb-9167-4619-83eb-f6aa7b09dde9
📒 Files selected for processing: 4
.github/workflows/pr.ymlsrc/main/browser/browser-popup-compatibility-cdp-fixture.tssrc/main/browser/browser-popup-compatibility-electron-fixture.tssrc/main/browser/popup-flake-main-diagnostic.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.
…p, not in the first test
Jinwoo-H
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
4504cf7c-8d13-4c20-a3f7-93dd4004bb6c
📒 Files selected for processing: 1
src/main/browser/browser-popup-compatibility-cdp-fixture.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.
| // A fresh runner's first launch can take tens of seconds to start Chromium's GPU process, and | ||
| // no hidden page paints until it does; charge that to startup, not the first test's poll. | ||
| stopFrameCaptures.set( | ||
| page, | ||
| await startHiddenRendererFrameCapture(page, (hasFrame) => | ||
| waitForFixture(hasFrame, 45_000).catch((error: unknown) => { | ||
| throw new Error(`No first frame for ${name}`, { cause: error }) | ||
| }) | ||
| ) | ||
| } | ||
| ) | ||
| } | ||
| }) | ||
| }, 90_000) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
set -eu
printf '%s\n' '--- fixture setup and helper references ---'
nl -ba src/main/browser/browser-popup-compatibility-cdp-fixture.ts | sed -n '1,260p'
printf '%s\n' '--- four-page callers ---'
nl -ba src/main/browser/browser-popup-background-compatibility.electron.test.ts | sed -n '1,45p'
nl -ba src/main/browser/browser-popup-adapter-compatibility.electron.test.ts | sed -n '1,40p'
printf '%s\n' '--- changed hunk against merge base ---'
git diff --unified=25 c5d03fc475c01fa342d527715a8aa519114b4177 4d1308b1060c1182432daaa180eaf9e4a8978c5f -- src/main/browser/browser-popup-compatibility-cdp-fixture.tsRepository: stablyai/orca
Length of output: 20815
🏁 Script executed:
set -eu
printf '%s\n' '--- capture helper ---'
nl -ba src/main/browser/hidden-renderer-frame-capture-fixture.ts | sed -n '1,180p'
printf '%s\n' '--- comparable capture usages ---'
rg -n -F --glob '*.ts' -e 'startHiddenRendererFrameCapture' -e 'stopFrameCaptures' src/main/browser || test "$?" -eq 1Repository: stablyai/orca
Length of output: 2647
Share the setup budget across page captures.
The four-page configurations start captures sequentially. Each capture can wait 45 seconds, while beforeAll has one 90-second timeout. Two slow pages can consume the full setup budget before a later page reaches its own timeout. CI then reports the setup timeout instead of the page-specific No first frame for ${name} error and its stderr cause.
Use concurrent captures and wait for all capture attempts to settle before propagating a failure.
Suggested fix
--- "a/src/main/browser/browser-popup-compatibility-cdp-fixture.ts"
+++ "b/src/main/browser/browser-popup-compatibility-cdp-fixture.ts"
@@ -192,34 +192,43 @@
...(nativeControls ? ['local-control', 'route-control'] : []),
...(adapterProbe ? ['probe-one', 'probe-two'] : [])
]
- for (const name of names) {
- await waitForFixture(() =>
- Boolean(
- browser
- ?.contexts()
- .flatMap((context) => context.pages())
- .some((page) => page.url() === `${origin}/${name}`)
- )
- )
- const page = browser
- .contexts()
- .flatMap((context) => context.pages())
- .find((candidate) => candidate.url() === `${origin}/${name}`)
- if (!page) {
- throw new Error('Missing guest page')
- }
- pages.set(name, page)
- // A fresh runner's first launch can take tens of seconds to start Chromium's GPU process, and
- // no hidden page paints until it does; charge that to startup, not the first test's poll.
- stopFrameCaptures.set(
- page,
- await startHiddenRendererFrameCapture(page, (hasFrame) =>
- waitForFixture(hasFrame, 45_000).catch((error: unknown) => {
- throw new Error(`No first frame for ${name}`, { cause: error })
- })
- )
- )
+ const captureResults = await Promise.allSettled(
+ names.map(async (name) => {
+ await waitForFixture(() =>
+ Boolean(
+ browser
+ ?.contexts()
+ .flatMap((context) => context.pages())
+ .some((page) => page.url() === `${origin}/${name}`)
+ )
+ )
+ const page = browser
+ .contexts()
+ .flatMap((context) => context.pages())
+ .find((candidate) => candidate.url() === `${origin}/${name}`)
+ if (!page) {
+ throw new Error('Missing guest page')
+ }
+ pages.set(name, page)
+ // A fresh runner's first launch can take tens of seconds to start Chromium's GPU process, and
+ // no hidden page paints until it does; charge that to startup, not the first test's poll.
+ const stop = await startHiddenRendererFrameCapture(page, (hasFrame) =>
+ waitForFixture(hasFrame, 45_000).catch((error: unknown) => {
+ throw new Error(`No first frame for ${name}`, { cause: error })
+ })
+ )
+ return [page, stop] as const
+ })
+ )
+ for (const result of captureResults) {
+ if (result.status === 'fulfilled') {
+ stopFrameCaptures.set(...result.value)
+ }
+ }
+ const failedCapture = captureResults.find((result) => result.status === 'rejected')
+ if (failedCapture?.status === 'rejected') {
+ throw failedCapture.reason
}
}, 90_000)
afterEach(async () => {
…clicking them Fresh sandboxed iframes run in their own renderer and can finish loading before the browser routes pointer input to them, so Playwright's click landed on the parent and no popup opened. Hover-poll until the new frame sees trusted input before clicking, in one helper shared by the adapter, native and background popup probes. Share the cold first-frame budget from #30854 with popup-origin-chrome, which still waited only 10s for the same one-time GPU startup.
…clicking them (#30966) Fresh sandboxed iframes run in their own renderer and can finish loading before the browser routes pointer input to them, so Playwright's click landed on the parent and no popup opened. Hover-poll until the new frame sees trusted input before clicking, in one helper shared by the adapter, native and background popup probes. Share the cold first-frame budget from #30854 with popup-origin-chrome, which still waited only 10s for the same one-time GPU startup.
…p, not in the first test (stablyai#30854) * test(browser): temporary timing probe for the popup first-frame flake * test(browser): extend the temporary popup flake probe to cold caches and the main process * test(browser): wait for the popup fixture's first paint during startup, not in the first test * test(browser): name the page whose first frame never arrived --------- Co-authored-by: E2E Test <e2e@test.local> (cherry picked from commit 449cbb0)
ELI5
A required Linux check that tests browser popups has been failing at random on unrelated PRs since about 12:30 ET on 2026-10-19. Nothing is wrong with popups. The test starts a hidden copy of the app, and on a brand-new CI machine the very first launch can take several seconds before it can draw anything, and the failing runs suggest it sometimes takes more than 30. The test only allowed 10 seconds for that, and it counted that time against the first test. This PR moves the wait into the setup step, so a slow first launch is waited out once at startup and no test fails for it.
What Changed
src/main/browser/browser-popup-compatibility-cdp-fixture.ts:No product code changes.
Why
Root cause. Temporary timing probes on this PR, run in the real Linux package job and since removed, showed this:
No commit caused this. The suspected commits (#30537, #30611, #30452, #30652, #30490) don't touch the frame capture, and #30490 landed after the first failure. The frame-capture wait was added in #28879 (2026-10-14).
Alternatives considered:
--disable-gpuor pre-warm the runner in the workflow: these change the probe's environment or add CI steps, and nothing showed they would remove the cold start.Linked Issue
N/A (maintainer CI fix; no issue)
Visual Proof
N/A: test-only change, nothing users see.
Testing
macOS: the native, plain, background and adapter popup Electron files pass locally with
ORCA_BACKGROUND_LAUNCH=1(hidden windows).Linux: the probe commits on this PR measured the cold and warm launches described above. At head 722f2ce, the package job (run 39322204950) passed 4 of 4 attempts: jobs 118302356105, 118316780416, 118325853714 and 118344791129. The first test took 0.44 to 0.5 s instead of 2.5 to 10 s, and the native file's total was 9.9 to 17.5 s because setup now absorbs the cold start.
Separate, already present on main and not touched here:
browser-popup-adapter-compatibility.electron.test.tssometimes fails inside a test body (childAtpoll). It reproduced 1 in 5 times on unmodified main locally and once on CI (2026-10-19 09:30Z).I manually tested these changes locally
Automated tests added/updated, or explained why not below
Review
Agent skill upstream boundary
Notes
Ensure no issues in: Security, Cross-platoform support (Linux, Windows, Mac), Remote SSH, Mobile, general backwards compatibility, performance
Test fixture only. No effect on SSH, mobile or Windows behaviour.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)