Uh oh!
There was an error while loading. Please reload this page.
fix(preview): keep snapshots slim unless the agent asks - #7302
fix(preview): keep snapshots slim unless the agent asks#7302gbarros-dev wants to merge 13 commits into
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Effect service conventions review: two findings on the desktop preview snapshot path. Details inline.
Posted via Macroscope — Effect Service Conventions
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This PR changes the default preview snapshot payload and wait_for matching behavior across the MCP, web, IPC, and desktop automation layers, rather than only adding an opt-in path. It also includes unresolved control-session lifecycle and stale-webview risks that require human assessment. Not approved because:
Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more. |
47b1e15 to
88686c5CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
149b56c to
3501c4dCompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
preview_snapshot dumped the accessibility tree, console, and network on every inspect. preview_wait_for also matched sidebar chrome, so agents thought a page was ready when only a nav label matched. Snapshots default to URL, main-landmark text, visible controls, and a PNG. AX, console, and network load only when include asks. Wait searches main by default and requires a visible locator.
Pointer-cursor div/td controls were never selected. An empty <main> fell back to body text, leaking sidebar chrome. A failed capturePage plus an empty CDP screenshot became a 0x0 image. Snapshots now select those controls, keep empty main text empty, and surface a typed capture failure. Tests cover the slim default, include ax, wait scope, and the failed-screenshot path.
The new snapshot tests spread makeTestPreviewWebContents(), which is typed as never. CI typecheck rejected that. The mocks are now a local webview object.
Snapshots treated every div/td/tr with computed cursor:pointer as clickable. Cursor inherits, so nested layout nodes filled the 200-cap and hid real controls. Only the outermost pointer container is kept.
Skipping any layout node whose parent had cursor:pointer dropped whole clickable areas when the pointer sat on li, section, label, or table. Only a harvested ancestor (div/td/tr) now suppresses descendants.
Uh oh!
There was an error while loading. Please reload this page.
87f7dcc to
a23b4f5CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| const epoch = (yield* Ref.get(controlEpochRef)).get(tabId) ?? 0; | ||
| const control = yield* ensureControlSession(wc); | ||
| const execute = Effect.fn("PreviewManager.executeControlAction")(function* () { | ||
| const requestedWc = yield* requireWebContents(tabId); |
There was a problem hiding this comment.
🟡 Mediumpreview/Manager.ts:1421
Automation requests that fail in requireWebContents or ensureControlSession leave a permanent running entry in actionTimeline, so later snapshots report an action that never completes. withControlSession records the event before those calls, but installs Effect.onExit(finalize) only around execute; install finalization around the entire operation (or otherwise finalize these early failures) so the entry becomes failed/interrupted.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/desktop/src/preview/Manager.ts around line 1421:
Automation requests that fail in `requireWebContents` or `ensureControlSession` leave a permanent `running` entry in `actionTimeline`, so later snapshots report an action that never completes. `withControlSession` records the event before those calls, but installs `Effect.onExit(finalize)` only around `execute`; install finalization around the entire operation (or otherwise finalize these early failures) so the entry becomes `failed`/`interrupted`.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3755a33. Configure here.
| if (tabs.has(tabId)) yield* update(tabId, { controller: "none" }); | ||
| }); | ||
| return yield* control.semaphore.withPermit(execute().pipe(Effect.onExit(finalize))); | ||
| return yield* control.semaphore.withPermit(execute(activeWc).pipe(Effect.onExit(finalize))); |
There was a problem hiding this comment.
Stale webview used after re-ensure
Medium Severity
withControlSession re-reads the tab after the first ensureControlSession, then can await a second ensure and still pass the pre-await activeWc into execute. A swap (or the detach-before-publish window of the first swap) leaves click, type, and snapshot talking to a guest the tab no longer owns.
Triggered by learned rule: PreviewManager: re-read tab state from tabsRef after mutations, don't use stale snapshots
Reviewed by Cursor Bugbot for commit 3755a33. Configure here.


preview_snapshotdumped the accessibility tree, console, and network on every inspect.preview_wait_foralso matched sidebar chrome, so agents thought a page was ready when only a nav label matched.Snapshots default to URL, main-landmark text, visible controls, and a PNG. AX, console, and network load only when
includeasks. Wait searchesmainby default and requires a visible locator.Split out of closed#7127.
Tests:
vp test run packages/contracts/src/preview.test.tsImplemented with Grok 4.6 through Grok CLI.
Note
Medium Risk
Changes default MCP/desktop snapshot shape and debugger domain wiring; agents or integrations that assumed always-on diagnostics may break until they pass
include.Overview
Preview automation snapshots are slim by default.
preview_snapshotand desktop IPC now accept optionalinclude(ax,console,network); without it callers get URL, main-landmark visible text, interactive elements, action timeline, and PNG only. CDP accessibility, log, and network domains are enabled on demand per control session instead of on every attach.preview_wait_foris scoped to real page content. Text and locator matching default to themainlandmark (plus modal/overlay roots); optionalscope: "document"searches the full page. Locator waits require visibility and skip dialog trigger slots.Snapshot harvesting adds stable element
ids, pointer-cursor table/layout nodes, and CDP screenshot fallback whencapturePagefails.accessibilityTreeis optional in the contract.Breaking for consumers: anything expecting full AX/console/network on every snapshot must pass
include.Reviewed by Cursor Bugbot for commit 3755a33. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Make preview snapshots slim by default and scope
preview_wait_forto main landmarkincludevalues (ax,console,network) to return diagnostic data. Default snapshots omit the accessibility tree and return empty console and network arrays.preview_wait_fordefaults to themainlandmark scope. It searches overlay roots, traverses shadow DOM, and ignores invisible elements unlessscopeis set todocument.Page.captureScreenshotfallback tocaptureAutomationSnapshotwhen Electron'scapturePagefails.withControlSessionin Manager.ts to resolve WebContents internally and re-check session validity before executing callbacks.preview_wait_forcalls relying on matches outside the main landmark or within invisible nodes will fail unlessscope: "document"is passed.Macroscope summarized 3755a33.