Repository navigation
Conversation
…k, inside the 1 s gesture window
📝 Walkthrough
Priority: ⬇️ Low Merge Risk: 🔵 Low · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly identifies the browser test flake, its opaque-frame context, and the expired gesture window. It accurately summarizes the primary change. Description check The description follows the required template and explains the failure, mechanism, rationale, testing, visual-proof exception, and checklist status. It provides sufficient detail for review. 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.
- Fix all pre-merge checks with AI
✨ 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: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
f6d93772-b1c4-4834-89cb-089fa04d685f
📒 Files selected for processing: 1
src/main/browser/browser-popup-background-cdp-probe.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.
| // Electron does not forward opaque OOPIF input to the root route gesture gate, and that gate | ||
| // honors a gesture for BROWSER_GUEST_INPUT_GESTURE_WINDOW_MS (1 s). Settle the frame target | ||
| // first so only one click separates the gesture from the open on a loaded runner. The trial | ||
| // carries no modifiers: Playwright presses them even in a trial, which would fake a gesture. | ||
| await locator.click({ trial: true }) | ||
| await page.locator('#background-gesture').click() | ||
| } | ||
| await locator.click(click) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
rg -n 'class Locator|async click\\(|trial|performAction' node_modules/.pnpm/playwright-core@1.64.0*/node_modules/playwright-core/lib/client/locator.js node_modules/.pnpm/playwright-core@1.64.0*/node_modules/playwright-core/lib/client/frame.js node_modules/playwright-core/lib/server/dom.js 2>/dev/null
rg -n 'playwright-core@1\\.64\\.0|playwright-core: 1\\.64\\.0' pnpm-lock.yamlRepository: stablyai/orca
Length of output: 151
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- dependency declaration ---'
rg -n -F -- 'playwright-core' package.json pnpm-lock.yaml | head -20 || true
printf '%s\n' '--- changed helper ---'
nl -ba src/main/browser/browser-popup-background-cdp-probe.ts | sed -n '82,122p'
printf '%s\n' '--- relevant callers ---'
rg -n -F -- 'clickBackgroundPopupProbe' src/main/browser || true
printf '%s\n' '--- local Playwright implementation candidates ---'
find . -path '*playwright-core*' -type f \( -name 'locator.js' -o -name 'frame.js' -o -name 'dom.js' \) -print 2>/dev/null | head -30Repository: stablyai/orca
Length of output: 3516
🌐 Web query:
Playwright 1.64.0 locator.click trial frame.click implementation source
💡 Result:
In Playwright **v1.64.0**, `locator.click()` is a thin wrapper: it calls `this._frame.click(this._selector, { strict: true, ...options })`. The `strict: true` is supplied by the locator; spreading `options` afterward lets caller options override it. ([github.com](https://github.com/microsoft/playwright/blob/v1.64.0/packages/playwright-core/src/client/locator.ts))
For `trial: true`, the click runs the actionability checks but skips the actual click. The server-side `frames.ts` implementation is in the tagged source; the API docs describe this behavior. ([github.com](https://github.com/microsoft/playwright/blob/v1.64.0/packages/playwright-core/src/server/frames.ts))
**Tagged implementation:** [`locator.ts`](https://github.com/microsoft/playwright/blob/v1.64.0/packages/playwright-core/src/client/locator.ts) · [`frame.ts`](https://github.com/microsoft/playwright/blob/v1.64.0/packages/playwright-core/src/client/frame.ts) · [`frames.ts`](https://github.com/microsoft/playwright/blob/v1.64.0/packages/playwright-core/src/server/frames.ts)
Citations:
- 1: https://github.com/microsoft/playwright/blob/v1.64.0/packages/playwright-core/src/client/locator.ts
- 2: https://github.com/microsoft/playwright/blob/v1.64.0/packages/playwright-core/src/server/frames.ts
🌐 Web query:
Playwright v1.64.0 frames.ts Frame.click _retryWithProgressIfNotConnected _performPointerAction trial actionability source lines
💡 Result:
In **Playwright v1.64.0**, `Frame.click` and `_retryWithProgressIfNotConnected` are in `packages/playwright-core/src/server/frames.ts`:
- **`_retryWithProgressIfNotConnected`**: lines **1104–1152**. It repeatedly resolves the target and retries if the element is detached. ([raw.githubusercontent.com](https://raw.githubusercontent.com/microsoft/playwright/v1.64.0/packages/playwright-core/src/server/frames.ts))
- **`Frame.click`**: lines **1160–1162**. It calls that retry helper, then delegates to `handle._click(...)`. ([raw.githubusercontent.com](https://raw.githubusercontent.com/microsoft/playwright/v1.64.0/packages/playwright-core/src/server/frames.ts))
- **`_performPointerAction`**: it is **not in `frames.ts`** in this version—the click path delegates to the element handle. So the pointer-action and trial/actionability implementation must be traced in the relevant DOM/element-handle source, not this file. ([raw.githubusercontent.com](https://raw.githubusercontent.com/microsoft/playwright/v1.64.0/packages/playwright-core/src/server/frames.ts))
The cited source is specifically the **v1.64.0** tag.
Citations:
- 1: https://raw.githubusercontent.com/microsoft/playwright/v1.64.0/packages/playwright-core/src/server/frames.ts
- 2: https://raw.githubusercontent.com/microsoft/playwright/v1.64.0/packages/playwright-core/src/server/frames.ts
- 3: https://raw.githubusercontent.com/microsoft/playwright/v1.64.0/packages/playwright-core/src/server/frames.ts
🌐 Web query:
raw GitHub microsoft playwright v1.64.0 packages/playwright-core/src/server/dom.ts _click _performPointerAction trial waitForEnabled
💡 Result:
In the **v1.64.0** source, `_click` configures a pointer action with `waitForEnabled: true`. `_performPointerAction` uses that flag to check for the element to be **visible, enabled, and stable** before proceeding. With `trial: true`, it still performs the actionability checks and hit-target setup, but the hit-target interceptor receives the trial flag; the mouse click itself is still called in the action path. ([raw.githubusercontent.com](https://raw.githubusercontent.com/microsoft/playwright/v1.64.0/packages/playwright-core/src/server/dom.ts))
[View `dom.ts` at v1.64.0](https://raw.githubusercontent.com/microsoft/playwright/v1.64.0/packages/playwright-core/src/server/dom.ts) ([raw.githubusercontent.com](https://raw.githubusercontent.com/microsoft/playwright/v1.64.0/packages/playwright-core/src/server/dom.ts))
Citations:
- 1: https://raw.githubusercontent.com/microsoft/playwright/v1.64.0/packages/playwright-core/src/server/dom.ts
- 2: https://raw.githubusercontent.com/microsoft/playwright/v1.64.0/packages/playwright-core/src/server/dom.ts
🏁 Script executed:
set -o pipefail
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
curl -fsSL https://raw.githubusercontent.com/microsoft/playwright/v1.64.0/packages/playwright-core/src/server/frames.ts -o "$tmpdir/frames.ts"
curl -fsSL https://raw.githubusercontent.com/microsoft/playwright/v1.64.0/packages/playwright-core/src/server/dom.ts -o "$tmpdir/dom.ts"
printf '%s\n' '--- frames.ts relevant methods ---'
nl -ba "$tmpdir/frames.ts" | sed -n '1100,1170p'
printf '%s\n' '--- dom.ts relevant symbols ---'
rg -n -F -e 'async _click' -e '_performPointerAction' -e 'trial' -e 'waitForEnabled' "$tmpdir/dom.ts"
printf '%s\n' '--- dom.ts relevant implementation ---'
nl -ba "$tmpdir/dom.ts" | sed -n '360,570p'Repository: stablyai/orca
Length of output: 17723
Do not rely on the trial click to cache the final target.
locator.click(click) starts a new Playwright click after the root gesture. Playwright 1.64.0 resolves the target again and then checks visibility, enabled state, stability, scrolling, and the hit target before dispatching the click. That work can consume the one-second gesture window, so the opaque-frame flake can still occur. Use a final-click path that reuses the prepared target and does not repeat this work after the gesture.
|
This fix did not work: the same probe ( What is established:
The open question for whoever owns the browser popup gate: does the window-open for an opaque OOPIF ever get a gesture from the modifier keydown or the frame click, or only from the earlier root click? Measuring the gap between the gesture and the window-open in the fixture would settle it. |
|
Superseded by #28799, which runs the Electron browser probes one at a time outside the unit shards. That addresses the shared-runner load these opaque-probe failures pointed to; my click-ordering change did not fix it on its own. If the opaque probe still fails after #28799, logging when the gesture and the window-open each happen in the fixture is the next step. |
ELI5
One browser test sometimes fails on CI for no reason related to the PR it runs on. The test clicks the page once to give Orca a "the user just clicked" signal, then clicks a link inside an embedded frame to open a background tab. Orca only trusts that signal for one second. On a busy CI machine, Playwright can take longer than a second to find and ready the link inside the frame. Orca then correctly refuses the tab, and the test waits forever for a request that will never come. This PR makes the test get the link ready before the first click, so the two clicks happen back to back.
What Changed
clickBackgroundPopupProbeinsrc/main/browser/browser-popup-background-cdp-probe.tsused to do this for theopaquecontext:#background-gestureon the root page (the gesture).#background-openinside the opaque cross-origin frame, wait for it to be actionable, then click it.The page gesture only counts for
BROWSER_GUEST_INPUT_GESTURE_WINDOW_MS(1,000 ms,browser-guest-input-gesture.ts). For opaque frames, the root click is the only gesture Orca sees, because Electron doesn't forward opaque out-of-process frame input to the route's gesture tracker. Step 2's frame resolution and actionability checks run after the gesture, inside that window.Now the target is settled first with a
trialclick: actionability checks only, no action, then the gesture, then the real click. The trial click carries no modifiers or button, because Playwright presses modifiers even in a trial, and a key press would count as a gesture and let the test pass for the wrong reason.No product code changes.
Why
expect.poll(() => status().native.requests.length). But when the gesture has expired, no request is ever made, so waiting longer cannot help. A longer poll would only make this failure slower.status()re-reads the fixture's status file on every call, and each window-open rewrites it.opaqueprobe, the only context whose gesture comes from a separate earlier click.local/opaque), both mine, plus feat(acp): Antigravity in the Chat UI over its ACP server #28595 (route/opaque), a teammate's.beforeAll(Electron startup timing out under load), never at the probe. The root cause is therefore argued from the code and the CI pattern, not from a local repro.Linked Issue
N/A (maintainer; CI flake found on #28761).
Visual Proof
N/A: test-only change.
Testing
I manually tested these changes locally
Automated tests added/updated, or explained why not below
browser-popup-background-compatibility.electron.test.tspasses locally (15/15) on repeated runs.pnpm tc:nodeand oxlint (including the low-evidence-pattern config) pass.AI Disclosure
Review
Agent skill upstream boundary
Notes
Test-only. If the flake recurs on opaque probes after this, the next step is to measure the gap between the gesture and the window-open in the fixture, not to widen the product's gesture window.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)