From 30701d1f8d743e44ead01a46bd8b1287faf4fe80 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 24 Sep 2026 15:13:11 -0700 Subject: [PATCH 01/10] fix(browser): press chord modifiers as real keys A chord such as Control+Shift+Y was sent as one key-down with a modifier bitmask, so pages that track held keys never saw Control or Shift go down. Modifiers are now pressed in order before the main key and released in reverse, as a physical keyboard delivers them, and a bare modifier key can be pressed on its own. --- .../src/main/browser-agent/keyboard.test.ts | 58 +++++++++++++++++-- .../src/main/browser-agent/keyboard.ts | 58 ++++++++++++++++++- 2 files changed, 111 insertions(+), 5 deletions(-) diff --git a/apps/desktop/src/main/browser-agent/keyboard.test.ts b/apps/desktop/src/main/browser-agent/keyboard.test.ts index 26d1566abc3..e7ee3011096 100644 --- a/apps/desktop/src/main/browser-agent/keyboard.test.ts +++ b/apps/desktop/src/main/browser-agent/keyboard.test.ts @@ -8,6 +8,7 @@ import { cdpModifiers, dispatchKeyCombo, KeyDispatchError, + modifierKeyEvents, parseKeyCombo, parseModifiers, } from '@/main/browser-agent/keyboard' @@ -173,6 +174,49 @@ describe('buildKeyDispatchPlan', () => { }) }) +describe('modifierKeyEvents', () => { + it('presses each chord modifier in order and releases them in reverse', () => { + const { downs, ups } = modifierKeyEvents(parseKeyCombo('Control+Shift+Y', 'linux'), 'linux') + + expect(downs).toEqual([ + expect.objectContaining({ + type: 'rawKeyDown', + key: 'Control', + code: 'ControlLeft', + modifiers: 2, + }), + expect.objectContaining({ + type: 'rawKeyDown', + key: 'Shift', + code: 'ShiftLeft', + modifiers: 2 | 8, + }), + ]) + expect(ups).toEqual([ + expect.objectContaining({ type: 'keyUp', key: 'Shift', modifiers: 2 }), + expect.objectContaining({ type: 'keyUp', key: 'Control', modifiers: 0 }), + ]) + }) + + it('presses a bare modifier as its own key with its flag set', () => { + const combo = parseKeyCombo('Control', 'linux') + const [down, up] = buildKeyDispatchPlan(combo, 'linux') + + expect(down).toMatchObject({ + type: 'rawKeyDown', + key: 'Control', + code: 'ControlLeft', + modifiers: 2, + }) + expect(up).toMatchObject({ type: 'keyUp', key: 'Control' }) + expect(modifierKeyEvents(combo, 'linux')).toEqual({ downs: [], ups: [] }) + }) + + it('sends no extra events for a key without modifiers', () => { + expect(modifierKeyEvents(parseKeyCombo('a', 'linux'), 'linux')).toEqual({ downs: [], ups: [] }) + }) +}) + describe('dispatchKeyCombo', () => { it('keeps agent-issued modifier shortcuts out of the Electron application menu', async () => { const contents = new WebContentsView().webContents @@ -202,6 +246,7 @@ describe('dispatchKeyCombo', () => { it('best-effort releases a key and reports a partial dispatch when key-up fails', async () => { const contents = new WebContentsView().webContents vi.mocked(contents.debugger.sendCommand) + .mockResolvedValueOnce({}) .mockResolvedValueOnce({}) .mockRejectedValueOnce(new Error('key-up response lost')) .mockRejectedValueOnce(new Error('cleanup unavailable')) @@ -219,12 +264,17 @@ describe('dispatchKeyCombo', () => { const keyEvents = vi .mocked(contents.debugger.sendCommand) .mock.calls.filter(([method]) => method === 'Input.dispatchKeyEvent') - expect(keyEvents).toHaveLength(3) - expect(keyEvents[1]).toEqual(keyEvents[2]) - expect(keyEvents[1]).toEqual([ + // Meta down, A down, A up (lost), then cleanup re-releases A and releases the held Meta. + expect(keyEvents).toHaveLength(5) + expect(keyEvents[2]).toEqual(keyEvents[3]) + expect(keyEvents[2]).toEqual([ 'Input.dispatchKeyEvent', expect.objectContaining({ type: 'keyUp', key: 'a' }), ]) + expect(keyEvents[4]).toEqual([ + 'Input.dispatchKeyEvent', + expect.objectContaining({ type: 'keyUp', key: 'Meta' }), + ]) expect(contents.setIgnoreMenuShortcuts).toHaveBeenNthCalledWith(1, true) expect(contents.setIgnoreMenuShortcuts).toHaveBeenNthCalledWith(2, false) }) @@ -256,7 +306,7 @@ describe('dispatchKeyCombo', () => { }) await expect(dispatchKeyCombo(contents, parseKeyCombo('Cmd+A'))).resolves.toBeUndefined() - expect(contents.debugger.sendCommand).toHaveBeenCalledTimes(2) + expect(contents.debugger.sendCommand).toHaveBeenCalledTimes(4) }) it('keeps the application menu isolated until overlapping dispatches finish', async () => { diff --git a/apps/desktop/src/main/browser-agent/keyboard.ts b/apps/desktop/src/main/browser-agent/keyboard.ts index bb72ce448b8..e3bd5b11ed1 100644 --- a/apps/desktop/src/main/browser-agent/keyboard.ts +++ b/apps/desktop/src/main/browser-agent/keyboard.ts @@ -51,6 +51,14 @@ const NAMED_KEYS: Record = { '`': { key: '`', code: 'Backquote', keyCode: 192 }, plus: { key: '+', code: 'Equal', keyCode: 187 }, insert: { key: 'Insert', code: 'Insert', keyCode: 45 }, + control: { key: 'Control', code: 'ControlLeft', keyCode: 17 }, + ctrl: { key: 'Control', code: 'ControlLeft', keyCode: 17 }, + shift: { key: 'Shift', code: 'ShiftLeft', keyCode: 16 }, + alt: { key: 'Alt', code: 'AltLeft', keyCode: 18 }, + option: { key: 'Alt', code: 'AltLeft', keyCode: 18 }, + meta: { key: 'Meta', code: 'MetaLeft', keyCode: 91 }, + cmd: { key: 'Meta', code: 'MetaLeft', keyCode: 91 }, + command: { key: 'Meta', code: 'MetaLeft', keyCode: 91 }, ...Object.fromEntries( Array.from({ length: 12 }, (_, index) => [ `f${index + 1}`, @@ -100,6 +108,14 @@ const BASE_FOR_SHIFTED_CHARACTER: Record = Object.fromEntries( Object.entries(SHIFTED_CHARACTERS).map(([base, shifted]) => [shifted, base]) ) +/** Modifier keys in the order a chord presses them, each with the flag its key-down sets. */ +const MODIFIER_KEYS: readonly { flag: keyof KeyModifiers; descriptor: KeyDescriptor }[] = [ + { flag: 'ctrl', descriptor: NAMED_KEYS.control }, + { flag: 'alt', descriptor: NAMED_KEYS.alt }, + { flag: 'shift', descriptor: NAMED_KEYS.shift }, + { flag: 'meta', descriptor: NAMED_KEYS.meta }, +] + export interface KeyModifiers { ctrl: boolean meta: boolean @@ -144,6 +160,8 @@ export function parseKeyCombo( const modifiers = parseModifiers(parts.slice(0, -1), platform) const keyPart = parts[parts.length - 1] const named = NAMED_KEYS[keyPart.toLowerCase()] + const ownModifier = named && MODIFIER_KEYS.find((modifier) => modifier.descriptor === named) + if (ownModifier) modifiers[ownModifier.flag] = true if (named) { const key = modifiers.shift ? (SHIFTED_CHARACTERS[named.key] ?? named.key) : named.key return { ...named, key, ...modifiers } @@ -297,6 +315,39 @@ export function buildKeyDispatchPlan( return [down, { ...base, type: 'keyUp' }] } +/** + * The separate modifier key presses around a chord's main key: a real keyboard sends Control, then + * Shift, then Y, and releases in reverse, so pages that track held keys see each modifier. Each + * key-down carries the modifiers held so far, and each key-up the ones still held. + */ +export function modifierKeyEvents( + rawCombo: ParsedCombo, + platform: NodeJS.Platform = process.platform +): { downs: cdp.CdpKeyEvent[]; ups: cdp.CdpKeyEvent[] } { + const combo = normalizeComboForPlatform(rawCombo, platform) + const held = { ctrl: false, meta: false, shift: false, alt: false } + const downs: cdp.CdpKeyEvent[] = [] + const ups: cdp.CdpKeyEvent[] = [] + for (const { flag, descriptor } of MODIFIER_KEYS) { + if (!combo[flag] || descriptor.key === combo.key) continue + held[flag] = true + const event = { + key: descriptor.key, + code: descriptor.code, + windowsVirtualKeyCode: descriptor.keyCode, + } + downs.push({ ...event, type: 'rawKeyDown', modifiers: cdpModifiers(held) }) + ups.unshift({ ...event, type: 'keyUp', modifiers: 0 }) + } + let remaining = { ...held } + for (const up of ups) { + const flag = MODIFIER_KEYS.find((modifier) => modifier.descriptor.key === up.key)?.flag + if (flag) remaining = { ...remaining, [flag]: false } + up.modifiers = cdpModifiers(remaining) + } + return { downs, ups } +} + /** * Presses a combo through the trusted pipeline. Throws on CDP failure. * @@ -308,6 +359,7 @@ export function buildKeyDispatchPlan( */ export async function dispatchKeyCombo(contents: WebContents, combo: ParsedCombo): Promise { const [down, up] = buildKeyDispatchPlan(combo) + const modifierKeys = modifierKeyEvents(combo) const isolatesApplicationMenu = combo.ctrl || combo.meta || combo.alt if (isolatesApplicationMenu) { const depth = applicationMenuIsolationDepth.get(contents) ?? 0 @@ -320,14 +372,18 @@ export async function dispatchKeyCombo(contents: WebContents, combo: ParsedCombo // CDP acknowledgement during navigation/process swap. In that ambiguous // case cleanup is required and a synthetic retry could double-act. keyDownDispatched = true + for (const event of modifierKeys.downs) await cdp.dispatchKeyEvent(contents, event) await cdp.dispatchKeyEvent(contents, down) await cdp.dispatchKeyEvent(contents, up) + for (const event of modifierKeys.ups) await cdp.dispatchKeyEvent(contents, event) } catch (error) { if (keyDownDispatched && !contents.isDestroyed()) { // Like pointer cleanup, this is best effort. The original key-up may // have reached Blink before its CDP response was lost; a duplicate // release is harmless, while omitting it can leave input state stuck. - await cdp.dispatchKeyEvent(contents, up).catch(() => {}) + for (const release of [up, ...modifierKeys.ups]) { + await cdp.dispatchKeyEvent(contents, release).catch(() => {}) + } } throw new KeyDispatchError( getErrorMessage(error, 'Trusted key dispatch failed'), From ab8c9918a9563889fe7dab5f6a93f0c2c94e15dd Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 24 Sep 2026 15:22:19 -0700 Subject: [PATCH 02/10] feat(browser): let clicks press and hold Clicks released the button immediately, so press-and-hold controls could not be operated. A click now accepts holdMs (0 to 10000, single clicks only) and keeps the button down that long before release. --- .../src/main/browser-agent/cdp.test.ts | 20 +++++++++++++++++++ apps/desktop/src/main/browser-agent/cdp.ts | 12 +++++++++-- .../main/browser-agent/context-menu.test.ts | 14 +++++++++++-- apps/desktop/src/main/browser-agent/driver.ts | 13 ++++++++++++ .../mothership/generated/tool-catalog-v1.ts | 10 ++++++++++ .../mothership/generated/tool-schemas-v1.ts | 10 ++++++++++ 6 files changed, 75 insertions(+), 4 deletions(-) diff --git a/apps/desktop/src/main/browser-agent/cdp.test.ts b/apps/desktop/src/main/browser-agent/cdp.test.ts index 36fb408ae4d..44c05485683 100644 --- a/apps/desktop/src/main/browser-agent/cdp.test.ts +++ b/apps/desktop/src/main/browser-agent/cdp.test.ts @@ -1,4 +1,5 @@ import { getErrorMessage } from '@sim/utils/errors' +import { toRecord } from '@sim/utils/object' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' vi.mock('electron', () => import('@/test/electron-mock')) @@ -16,6 +17,7 @@ import { ensureInstrumented, evaluateInIsolatedFrame, insertText, + PRIMARY_CLICK, releaseFileInput, resolveFileInput, setColorScheme, @@ -255,6 +257,24 @@ describe('browser-agent CDP instrumentation', () => { ]) }) + it('holds the button down for holdMs before releasing it', async () => { + const contents = new WebContentsView().webContents + const types = () => + vi.mocked(contents.debugger.sendCommand).mock.calls.map(([, params]) => toRecord(params).type) + vi.useFakeTimers() + try { + const click = clickAt(contents, 5, 6, false, { ...PRIMARY_CLICK, holdMs: 1500 }) + await vi.advanceTimersByTimeAsync(1000) + expect(types()).toEqual(['mousePressed']) + + await vi.advanceTimersByTimeAsync(500) + await click + expect(types()).toEqual(['mousePressed', 'mouseReleased']) + } finally { + vi.useRealTimers() + } + }) + it('releases the mouse after a partial click failure', async () => { const contents = new WebContentsView().webContents vi.mocked(contents.debugger.sendCommand) diff --git a/apps/desktop/src/main/browser-agent/cdp.ts b/apps/desktop/src/main/browser-agent/cdp.ts index 8a542b46cad..7587e598e61 100644 --- a/apps/desktop/src/main/browser-agent/cdp.ts +++ b/apps/desktop/src/main/browser-agent/cdp.ts @@ -954,9 +954,16 @@ export interface PointerClick { clickCount: 1 | 2 | 3 /** CDP modifier bitmask (Alt=1, Ctrl=2, Meta=4, Shift=8). */ modifiers: number + /** How long the button stays down before release; press-and-hold controls need it. */ + holdMs: number } -export const PRIMARY_CLICK: PointerClick = { button: 'left', clickCount: 1, modifiers: 0 } +export const PRIMARY_CLICK: PointerClick = { + button: 'left', + clickCount: 1, + modifiers: 0, + holdMs: 0, +} const BUTTON_MASKS: Record = { left: 1, right: 2, middle: 4 } const agentContextClicks = new WeakMap() @@ -984,7 +991,7 @@ export async function clickAt( click: PointerClick = PRIMARY_CLICK ): Promise { if (moveBeforePress) await moveMouse(contents, x, y) - const { button, clickCount, modifiers } = click + const { button, clickCount, modifiers, holdMs } = click const buttons = BUTTON_MASKS[button] let pressed = false try { @@ -1005,6 +1012,7 @@ export async function clickAt( modifiers, clickCount: count, }) + if (holdMs > 0) await sleep(holdMs) await sendInput(contents, 'Input.dispatchMouseEvent', { type: 'mouseReleased', x, diff --git a/apps/desktop/src/main/browser-agent/context-menu.test.ts b/apps/desktop/src/main/browser-agent/context-menu.test.ts index 2dafb8d0593..9e3772f55bb 100644 --- a/apps/desktop/src/main/browser-agent/context-menu.test.ts +++ b/apps/desktop/src/main/browser-agent/context-menu.test.ts @@ -228,7 +228,12 @@ describe('attachAgentContextMenu', () => { ContextMenuListener, ][] const onContextMenu = listeners.find(([event]) => event === 'context-menu')![1] - await clickAt(contents, 10, 20, false, { button: 'right', clickCount: 1, modifiers: 0 }) + await clickAt(contents, 10, 20, false, { + button: 'right', + clickCount: 1, + modifiers: 0, + holdMs: 0, + }) vi.mocked(Menu.buildFromTemplate).mockClear() onContextMenu({}, params()) @@ -252,7 +257,12 @@ describe('attachAgentContextMenu', () => { ][] const onInput = listeners.find(([event]) => event === 'input-event')?.[1] const onContextMenu = listeners.find(([event]) => event === 'context-menu')![1] - await clickAt(contents, 10, 20, false, { button: 'right', clickCount: 1, modifiers: 0 }) + await clickAt(contents, 10, 20, false, { + button: 'right', + clickCount: 1, + modifiers: 0, + holdMs: 0, + }) vi.mocked(Menu.buildFromTemplate).mockClear() onInput?.({}, { type: inputEvent }) diff --git a/apps/desktop/src/main/browser-agent/driver.ts b/apps/desktop/src/main/browser-agent/driver.ts index 2ba47c613f0..72e6623ac90 100644 --- a/apps/desktop/src/main/browser-agent/driver.ts +++ b/apps/desktop/src/main/browser-agent/driver.ts @@ -1122,6 +1122,9 @@ const POINTER_BUTTONS: ReadonlySet = new Set(['left', 'right', 'middle'] const MAX_KEY_REPEAT = 50 /** The optional click gesture shared by `browser_click` and `browser_click_at`. */ +/** Longest press-and-hold a click may request; well inside the click tool's watchdog. */ +const MAX_POINTER_HOLD_MS = 10_000 + function pointerClick(params: Record): cdp.PointerClick { const button = str(params, 'button') ?? 'left' if (!POINTER_BUTTONS.has(button)) throw new ToolError('button must be left, right, or middle.') @@ -1133,10 +1136,20 @@ function pointerClick(params: Record): cdp.PointerClick { if (!Array.isArray(names) || names.length > 4 || names.some((name) => typeof name !== 'string')) { throw new ToolError('modifiers must be a list of modifier names such as ["Shift"] or ["Mod"].') } + const holdMs = num(params, 'holdMs') ?? 0 + if (!Number.isInteger(holdMs) || holdMs < 0 || holdMs > MAX_POINTER_HOLD_MS) { + throw new ToolError( + `holdMs must be a whole number of milliseconds from 0 to ${MAX_POINTER_HOLD_MS}.` + ) + } + if (holdMs > 0 && clickCount !== 1) { + throw new ToolError('holdMs applies to a single press; use clickCount 1.') + } return { button: button as cdp.PointerClick['button'], clickCount, modifiers: cdpModifiers(parseModifiers(names)), + holdMs, } } diff --git a/apps/sim/lib/mothership/generated/tool-catalog-v1.ts b/apps/sim/lib/mothership/generated/tool-catalog-v1.ts index d3ae90b8a85..8402223f29c 100644 --- a/apps/sim/lib/mothership/generated/tool-catalog-v1.ts +++ b/apps/sim/lib/mothership/generated/tool-catalog-v1.ts @@ -529,6 +529,11 @@ export const BrowserClick: ToolCatalogEntry = { description: "The element id to act on (from the current tab's most recent browser_snapshot). Treat refs as invalid across tab switches or later snapshots.", }, + holdMs: { + type: 'number', + description: + 'Keep the button pressed this many milliseconds before releasing (0 to 10000, default 0), for press-and-hold controls. Single clicks only.', + }, modifiers: { type: 'array', description: @@ -667,6 +672,11 @@ export const BrowserClickAt: ToolCatalogEntry = { }, required: ['accept'], }, + holdMs: { + type: 'number', + description: + 'Keep the button pressed this many milliseconds before releasing (0 to 10000, default 0), for press-and-hold controls. Single clicks only.', + }, modifiers: { type: 'array', description: diff --git a/apps/sim/lib/mothership/generated/tool-schemas-v1.ts b/apps/sim/lib/mothership/generated/tool-schemas-v1.ts index 4542a91bf0c..a7cd19a9349 100644 --- a/apps/sim/lib/mothership/generated/tool-schemas-v1.ts +++ b/apps/sim/lib/mothership/generated/tool-schemas-v1.ts @@ -218,6 +218,11 @@ export const TOOL_RUNTIME_SCHEMAS: Record = { description: "The element id to act on (from the current tab's most recent browser_snapshot). Treat refs as invalid across tab switches or later snapshots.", }, + holdMs: { + type: 'number', + description: + 'Keep the button pressed this many milliseconds before releasing (0 to 10000, default 0), for press-and-hold controls. Single clicks only.', + }, modifiers: { type: 'array', description: @@ -390,6 +395,11 @@ export const TOOL_RUNTIME_SCHEMAS: Record = { }, required: ['accept'], }, + holdMs: { + type: 'number', + description: + 'Keep the button pressed this many milliseconds before releasing (0 to 10000, default 0), for press-and-hold controls. Single clicks only.', + }, modifiers: { type: 'array', description: From 8eea2d0279681877631e335711338869d0c546a7 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 24 Sep 2026 15:23:50 -0700 Subject: [PATCH 03/10] fix(browser): read disablePortal modals hidden with their app root MUI hides every child except a modal's mount node; a disablePortal modal mounts inside the app root it just hid, so every node had an aria-hidden ancestor and the snapshot came back empty while the dialog was on screen. Visibility checks now skip aria-hidden only on ancestors above the single topmost modal hidden that way; portaled modals and other hidden regions are unchanged. --- .../main/browser-agent/page-functions.test.ts | 79 ++++ .../src/main/browser-agent/page-functions.ts | 341 +++++++++++++++++- 2 files changed, 411 insertions(+), 9 deletions(-) diff --git a/apps/desktop/src/main/browser-agent/page-functions.test.ts b/apps/desktop/src/main/browser-agent/page-functions.test.ts index 3a5a0d693d7..920e18ec150 100644 --- a/apps/desktop/src/main/browser-agent/page-functions.test.ts +++ b/apps/desktop/src/main/browser-agent/page-functions.test.ts @@ -2438,3 +2438,82 @@ describe('setFocusedInputValue', () => { expect(other.value).toBe('') }) }) + +describe('modal hidden together with its own app root', () => { + const showAll = (): void => { + for (const element of Array.from(document.body.querySelectorAll('*'))) visible(element) + } + + it('reads, clicks, and reports a disablePortal dialog inside the aria-hidden root', () => { + document.body.innerHTML = ` + ` + showAll() + + const outline = outlineOf(runSerialized(collectSnapshot, [])) + + expect(outline).toContain('dialog') + expect(outline).toContain('Email input') + expect(outline).toContain('Send') + expect(outline).not.toContain('Compose') + expect(outline).not.toContain('icon') + const clicked = runSerialized(clickElement, [refFor(outline, 'Send'), false]) as { + error?: string + } + expect(clicked.error).toBeUndefined() + expect((runSerialized(readPageActionState, []) as { dialogs: string[] }).dialogs).toContain( + 'New Message' + ) + }) + + it('keeps a portaled modal scoped exactly as before', () => { + document.body.innerHTML = ` + +
` + showAll() + + const outline = outlineOf(collectSnapshot()) + + expect(outline).toContain('Send') + expect(outline).not.toContain('Compose') + }) + + it('keeps an aria-hidden region hidden when no modal is open', () => { + document.body.innerHTML = ` + + ` + showAll() + + const outline = outlineOf(collectSnapshot()) + + expect(outline).toContain('Shown action') + expect(outline).not.toContain('Hidden action') + }) + + it('exposes only the topmost of stacked disablePortal modals', () => { + document.body.innerHTML = ` + ` + showAll() + + const outline = outlineOf(collectSnapshot()) + + expect(outline).toContain('Confirm') + expect(outline).not.toContain('Discard') + expect((readPageActionState() as { dialogs: string[] }).dialogs).toEqual(['Upper']) + }) +}) diff --git a/apps/desktop/src/main/browser-agent/page-functions.ts b/apps/desktop/src/main/browser-agent/page-functions.ts index 86c622d10d8..154ee983d0f 100644 --- a/apps/desktop/src/main/browser-agent/page-functions.ts +++ b/apps/desktop/src/main/browser-agent/page-functions.ts @@ -179,6 +179,56 @@ export function collectSnapshot(startingElementId = 0, elementId?: number): unkn const visibility = new WeakMap() + /** + * The open modal the page aria-hid together with everything else. MUI's + * ModalManager aria-hides every child except the modal's mount node, + * and a `disablePortal` modal mounts inside the app root it just hid, so its + * own ancestor carries aria-hidden. Ancestors ABOVE this modal skip only the + * aria-hidden test. Null when any modal is visible unmodified (the portaled + * case) or no single topmost modal is contained by all of its hidden + * ancestors. + */ + let exemptModal: Element | null | undefined + const getExemptModal = (): Element | null => { + if (exemptModal !== undefined) return exemptModal + exemptModal = null + const rendered: Array<{ modal: Element; hidden: Element[] }> = [] + for (const modal of Array.from( + document.querySelectorAll('[aria-modal="true"], dialog[open]') + )) { + const rect = modal.getBoundingClientRect() + if (rect.width <= 0 || rect.height <= 0 || modal.getAttribute('aria-hidden') === 'true') + continue + const hidden: Element[] = [] + let visible = true + for (let current: Element | null = modal; current && visible; ) { + const style = current.ownerDocument.defaultView?.getComputedStyle(current) + const opacity = Number.parseFloat(style?.opacity || '1') + visible = Boolean( + style && + style.display !== 'none' && + style.visibility !== 'hidden' && + style.contentVisibility !== 'hidden' && + (!Number.isFinite(opacity) || opacity > 0.01) && + !current.hasAttribute('hidden') + ) + if (current.getAttribute('aria-hidden') === 'true') hidden.push(current) + if (current.parentElement) current = current.parentElement + else { + const root = current.getRootNode() + current = 'host' in root ? (root.host as Element) : null + } + } + if (visible) rendered.push({ modal, hidden }) + } + if (rendered.some(({ hidden }) => hidden.length === 0)) return exemptModal + const topmost = rendered.filter(({ hidden }) => + hidden.every((ancestor) => rendered.every(({ modal }) => ancestor.contains(modal))) + ) + if (topmost.length === 1) exemptModal = topmost[0].modal + return exemptModal + } + const isVisible = (el: Element): boolean => { const cached = visibility.get(el) if (cached !== undefined) return cached @@ -194,6 +244,7 @@ export function collectSnapshot(startingElementId = 0, elementId?: number): unkn return false } let visible = true + let aboveExemptModal = false for (let current: Element | null = el; current && visible; ) { const currentView: Window | null = current.ownerDocument.defaultView const style = currentView?.getComputedStyle(current) @@ -205,8 +256,9 @@ export function collectSnapshot(startingElementId = 0, elementId?: number): unkn style.contentVisibility !== 'hidden' && (!Number.isFinite(opacity) || opacity > 0.01) && !current.hasAttribute('hidden') && - current.getAttribute('aria-hidden') !== 'true' + (aboveExemptModal || current.getAttribute('aria-hidden') !== 'true') ) + if (current === getExemptModal()) aboveExemptModal = true if (current.parentElement) current = current.parentElement else { const root = current.getRootNode() @@ -772,9 +824,12 @@ export function collectSnapshot(startingElementId = 0, elementId?: number): unkn // to zero size while mounting a replacement. Re-check live so a parked // node can fall through to the same strict, unique recovery used for a // detached node. + /** Re-derived live with the rest of this check, not reused from snapshot time. */ + exemptModal = undefined const isCurrentlyVisible = (candidate: Element): boolean => { const rect = candidate.getBoundingClientRect() if (rect.width <= 0 || rect.height <= 0) return false + let aboveExemptModal = false for (let current: Element | null = candidate; current; ) { const currentView: Window | null = current.ownerDocument.defaultView const style = currentView?.getComputedStyle(current) @@ -786,10 +841,11 @@ export function collectSnapshot(startingElementId = 0, elementId?: number): unkn style.contentVisibility === 'hidden' || (Number.isFinite(opacity) && opacity <= 0.01) || current.hasAttribute('hidden') || - current.getAttribute('aria-hidden') === 'true' + (!aboveExemptModal && current.getAttribute('aria-hidden') === 'true') ) { return false } + if (current === getExemptModal()) aboveExemptModal = true if (current.parentElement) current = current.parentElement else { const root = current.getRootNode() @@ -1009,8 +1065,51 @@ export function clickElement( if (!el.isConnected) return { error: 'stale', reason: window.__simAgentStaleReason } } + /** The aria-hidden modal exemption documented on collectSnapshot, inlined for serialization. */ + let exemptModal: Element | null | undefined + const getExemptModal = (): Element | null => { + if (exemptModal !== undefined) return exemptModal + exemptModal = null + const rendered: Array<{ modal: Element; hidden: Element[] }> = [] + for (const modal of Array.from( + document.querySelectorAll('[aria-modal="true"], dialog[open]') + )) { + const rect = modal.getBoundingClientRect() + if (rect.width <= 0 || rect.height <= 0 || modal.getAttribute('aria-hidden') === 'true') + continue + const hidden: Element[] = [] + let visible = true + for (let current: Element | null = modal; current && visible; ) { + const style = current.ownerDocument.defaultView?.getComputedStyle(current) + const opacity = Number.parseFloat(style?.opacity || '1') + visible = Boolean( + style && + style.display !== 'none' && + style.visibility !== 'hidden' && + style.contentVisibility !== 'hidden' && + (!Number.isFinite(opacity) || opacity > 0.01) && + !current.hasAttribute('hidden') + ) + if (current.getAttribute('aria-hidden') === 'true') hidden.push(current) + if (current.parentElement) current = current.parentElement + else { + const root = current.getRootNode() + current = 'host' in root ? (root.host as Element) : null + } + } + if (visible) rendered.push({ modal, hidden }) + } + if (rendered.some(({ hidden }) => hidden.length === 0)) return exemptModal + const topmost = rendered.filter(({ hidden }) => + hidden.every((ancestor) => rendered.every(({ modal }) => ancestor.contains(modal))) + ) + if (topmost.length === 1) exemptModal = topmost[0].modal + return exemptModal + } + const view = el.ownerDocument.defaultView if (!view) return { error: 'stale', reason: window.__simAgentStaleReason } + let aboveExemptModal = false for (let current: Element | null = el; current; ) { const currentView: Window | null = current.ownerDocument.defaultView const style = currentView?.getComputedStyle(current) @@ -1022,10 +1121,11 @@ export function clickElement( style.contentVisibility === 'hidden' || (Number.isFinite(opacity) && opacity <= 0.01) || current.hasAttribute('hidden') || - current.getAttribute('aria-hidden') === 'true' + (!aboveExemptModal && current.getAttribute('aria-hidden') === 'true') ) { return { error: 'not-visible' } } + if (current === getExemptModal()) aboveExemptModal = true if (current.parentElement) current = current.parentElement else { const root = current.getRootNode() @@ -1501,6 +1601,49 @@ export function focusElementForTyping(id: number, moveFocus = true): unknown { rect.bottom - rect.top > 1 ) if (rects.length === 0) return { error: 'not-visible' } + /** The aria-hidden modal exemption documented on collectSnapshot, inlined for serialization. */ + let exemptModal: Element | null | undefined + const getExemptModal = (): Element | null => { + if (exemptModal !== undefined) return exemptModal + exemptModal = null + const rendered: Array<{ modal: Element; hidden: Element[] }> = [] + for (const modal of Array.from( + document.querySelectorAll('[aria-modal="true"], dialog[open]') + )) { + const rect = modal.getBoundingClientRect() + if (rect.width <= 0 || rect.height <= 0 || modal.getAttribute('aria-hidden') === 'true') + continue + const hidden: Element[] = [] + let visible = true + for (let current: Element | null = modal; current && visible; ) { + const style = current.ownerDocument.defaultView?.getComputedStyle(current) + const opacity = Number.parseFloat(style?.opacity || '1') + visible = Boolean( + style && + style.display !== 'none' && + style.visibility !== 'hidden' && + style.contentVisibility !== 'hidden' && + (!Number.isFinite(opacity) || opacity > 0.01) && + !current.hasAttribute('hidden') + ) + if (current.getAttribute('aria-hidden') === 'true') hidden.push(current) + if (current.parentElement) current = current.parentElement + else { + const root = current.getRootNode() + current = 'host' in root ? (root.host as Element) : null + } + } + if (visible) rendered.push({ modal, hidden }) + } + if (rendered.some(({ hidden }) => hidden.length === 0)) return exemptModal + const topmost = rendered.filter(({ hidden }) => + hidden.every((ancestor) => rendered.every(({ modal }) => ancestor.contains(modal))) + ) + if (topmost.length === 1) exemptModal = topmost[0].modal + return exemptModal + } + + let aboveExemptModal = false for (let current: Element | null = editable; current; current = composedParent(current)) { const currentView: Window | null = current.ownerDocument.defaultView const style = currentView?.getComputedStyle(current) @@ -1512,10 +1655,11 @@ export function focusElementForTyping(id: number, moveFocus = true): unknown { style.contentVisibility === 'hidden' || (Number.isFinite(opacity) && opacity <= 0.01) || current.hasAttribute('hidden') || - current.getAttribute('aria-hidden') === 'true' + (!aboveExemptModal && current.getAttribute('aria-hidden') === 'true') ) { return { error: 'not-visible' } } + if (current === getExemptModal()) aboveExemptModal = true } if (moveFocus) { @@ -2230,6 +2374,48 @@ export function readPageActionState( const observedDocument = observedElement?.ownerDocument ?? registeredElement?.ownerDocument ?? document const observedWindow = observedDocument.defaultView ?? window + /** The aria-hidden modal exemption documented on collectSnapshot, inlined for serialization. */ + let exemptModal: Element | null | undefined + const getExemptModal = (): Element | null => { + if (exemptModal !== undefined) return exemptModal + exemptModal = null + const rendered: Array<{ modal: Element; hidden: Element[] }> = [] + for (const modal of Array.from( + document.querySelectorAll('[aria-modal="true"], dialog[open]') + )) { + const rect = modal.getBoundingClientRect() + if (rect.width <= 0 || rect.height <= 0 || modal.getAttribute('aria-hidden') === 'true') + continue + const hidden: Element[] = [] + let visible = true + for (let current: Element | null = modal; current && visible; ) { + const style = current.ownerDocument.defaultView?.getComputedStyle(current) + const opacity = Number.parseFloat(style?.opacity || '1') + visible = Boolean( + style && + style.display !== 'none' && + style.visibility !== 'hidden' && + style.contentVisibility !== 'hidden' && + (!Number.isFinite(opacity) || opacity > 0.01) && + !current.hasAttribute('hidden') + ) + if (current.getAttribute('aria-hidden') === 'true') hidden.push(current) + if (current.parentElement) current = current.parentElement + else { + const root = current.getRootNode() + current = 'host' in root ? (root.host as Element) : null + } + } + if (visible) rendered.push({ modal, hidden }) + } + if (rendered.some(({ hidden }) => hidden.length === 0)) return exemptModal + const topmost = rendered.filter(({ hidden }) => + hidden.every((ancestor) => rendered.every(({ modal }) => ancestor.contains(modal))) + ) + if (topmost.length === 1) exemptModal = topmost[0].modal + return exemptModal + } + const isEffectivelyRendered = (element: Element): boolean => { const rect = element.getBoundingClientRect() const view = element.ownerDocument.defaultView @@ -2244,6 +2430,7 @@ export function readPageActionState( ) { return false } + let aboveExemptModal = false for (let current: Element | null = element; current; ) { const currentView: Window | null = current.ownerDocument.defaultView const style = currentView?.getComputedStyle(current) @@ -2255,10 +2442,11 @@ export function readPageActionState( style.contentVisibility === 'hidden' || (Number.isFinite(opacity) && opacity <= 0.01) || current.hasAttribute('hidden') || - current.getAttribute('aria-hidden') === 'true' + (!aboveExemptModal && current.getAttribute('aria-hidden') === 'true') ) { return false } + if (current === getExemptModal()) aboveExemptModal = true if (current.parentElement) current = current.parentElement else { const root = current.getRootNode() @@ -2411,6 +2599,7 @@ export function readPageActionState( const rect = element.getBoundingClientRect() const view = element.ownerDocument.defaultView if (!view || rect.width <= 0 || rect.height <= 0) return false + let aboveExemptModal = false for (let current: Element | null = element; current; ) { const style = view.getComputedStyle(current) if ( @@ -2418,10 +2607,11 @@ export function readPageActionState( style.visibility === 'hidden' || Number.parseFloat(style.opacity || '1') <= 0.01 || current.hasAttribute('hidden') || - current.getAttribute('aria-hidden') === 'true' + (!aboveExemptModal && current.getAttribute('aria-hidden') === 'true') ) { return false } + if (current === getExemptModal()) aboveExemptModal = true if (current.parentElement) current = current.parentElement else { const root = current.getRootNode() @@ -2528,6 +2718,48 @@ export function scrollPage(direction: string, amount?: number, elementId?: numbe const delta = towardStart ? -distance : distance const scrollingElement = (document.scrollingElement || document.documentElement) as HTMLElement + /** The aria-hidden modal exemption documented on collectSnapshot, inlined for serialization. */ + let exemptModal: Element | null | undefined + const getExemptModal = (): Element | null => { + if (exemptModal !== undefined) return exemptModal + exemptModal = null + const rendered: Array<{ modal: Element; hidden: Element[] }> = [] + for (const modal of Array.from( + document.querySelectorAll('[aria-modal="true"], dialog[open]') + )) { + const rect = modal.getBoundingClientRect() + if (rect.width <= 0 || rect.height <= 0 || modal.getAttribute('aria-hidden') === 'true') + continue + const hidden: Element[] = [] + let visible = true + for (let current: Element | null = modal; current && visible; ) { + const style = current.ownerDocument.defaultView?.getComputedStyle(current) + const opacity = Number.parseFloat(style?.opacity || '1') + visible = Boolean( + style && + style.display !== 'none' && + style.visibility !== 'hidden' && + style.contentVisibility !== 'hidden' && + (!Number.isFinite(opacity) || opacity > 0.01) && + !current.hasAttribute('hidden') + ) + if (current.getAttribute('aria-hidden') === 'true') hidden.push(current) + if (current.parentElement) current = current.parentElement + else { + const root = current.getRootNode() + current = 'host' in root ? (root.host as Element) : null + } + } + if (visible) rendered.push({ modal, hidden }) + } + if (rendered.some(({ hidden }) => hidden.length === 0)) return exemptModal + const topmost = rendered.filter(({ hidden }) => + hidden.every((ancestor) => rendered.every(({ modal }) => ancestor.contains(modal))) + ) + if (topmost.length === 1) exemptModal = topmost[0].modal + return exemptModal + } + const isVisible = (element: Element): boolean => { const rect = element.getBoundingClientRect() const view = element.ownerDocument.defaultView @@ -2540,6 +2772,7 @@ export function scrollPage(direction: string, amount?: number, elementId?: numbe ) { return false } + let aboveExemptModal = false for (let current: Element | null = element; current; ) { const currentView: Window | null = current.ownerDocument.defaultView const style = currentView?.getComputedStyle(current) @@ -2551,10 +2784,11 @@ export function scrollPage(direction: string, amount?: number, elementId?: numbe style.contentVisibility === 'hidden' || (Number.isFinite(opacity) && opacity <= 0.01) || current.hasAttribute('hidden') || - current.getAttribute('aria-hidden') === 'true' + (!aboveExemptModal && current.getAttribute('aria-hidden') === 'true') ) { return false } + if (current === getExemptModal()) aboveExemptModal = true if (current.parentElement) current = current.parentElement else { const root = current.getRootNode() @@ -3021,6 +3255,49 @@ export function getElementScreenshotRect(id: number): unknown { const rect = element.getBoundingClientRect() const view = element.ownerDocument.defaultView if (!view) return { error: 'stale', reason: window.__simAgentStaleReason } + /** The aria-hidden modal exemption documented on collectSnapshot, inlined for serialization. */ + let exemptModal: Element | null | undefined + const getExemptModal = (): Element | null => { + if (exemptModal !== undefined) return exemptModal + exemptModal = null + const rendered: Array<{ modal: Element; hidden: Element[] }> = [] + for (const modal of Array.from( + document.querySelectorAll('[aria-modal="true"], dialog[open]') + )) { + const rect = modal.getBoundingClientRect() + if (rect.width <= 0 || rect.height <= 0 || modal.getAttribute('aria-hidden') === 'true') + continue + const hidden: Element[] = [] + let visible = true + for (let current: Element | null = modal; current && visible; ) { + const style = current.ownerDocument.defaultView?.getComputedStyle(current) + const opacity = Number.parseFloat(style?.opacity || '1') + visible = Boolean( + style && + style.display !== 'none' && + style.visibility !== 'hidden' && + style.contentVisibility !== 'hidden' && + (!Number.isFinite(opacity) || opacity > 0.01) && + !current.hasAttribute('hidden') + ) + if (current.getAttribute('aria-hidden') === 'true') hidden.push(current) + if (current.parentElement) current = current.parentElement + else { + const root = current.getRootNode() + current = 'host' in root ? (root.host as Element) : null + } + } + if (visible) rendered.push({ modal, hidden }) + } + if (rendered.some(({ hidden }) => hidden.length === 0)) return exemptModal + const topmost = rendered.filter(({ hidden }) => + hidden.every((ancestor) => rendered.every(({ modal }) => ancestor.contains(modal))) + ) + if (topmost.length === 1) exemptModal = topmost[0].modal + return exemptModal + } + + let aboveExemptModal = false for (let current: Element | null = element; current; ) { const currentView: Window | null = current.ownerDocument.defaultView const style = currentView?.getComputedStyle(current) @@ -3032,10 +3309,11 @@ export function getElementScreenshotRect(id: number): unknown { style.contentVisibility === 'hidden' || (Number.isFinite(opacity) && opacity <= 0.01) || current.hasAttribute('hidden') || - current.getAttribute('aria-hidden') === 'true' + (!aboveExemptModal && current.getAttribute('aria-hidden') === 'true') ) { return { error: 'not-visible' } } + if (current === getExemptModal()) aboveExemptModal = true if (current.parentElement) current = current.parentElement else { const root = current.getRootNode() @@ -3170,7 +3448,51 @@ export function readChildFrameElementState( rect.left < view.innerWidth && rect.top < view.innerHeight ) + + /** The aria-hidden modal exemption documented on collectSnapshot, inlined for serialization. */ + let exemptModal: Element | null | undefined + const getExemptModal = (): Element | null => { + if (exemptModal !== undefined) return exemptModal + exemptModal = null + const rendered: Array<{ modal: Element; hidden: Element[] }> = [] + for (const modal of Array.from( + document.querySelectorAll('[aria-modal="true"], dialog[open]') + )) { + const rect = modal.getBoundingClientRect() + if (rect.width <= 0 || rect.height <= 0 || modal.getAttribute('aria-hidden') === 'true') + continue + const hidden: Element[] = [] + let visible = true + for (let current: Element | null = modal; current && visible; ) { + const style = current.ownerDocument.defaultView?.getComputedStyle(current) + const opacity = Number.parseFloat(style?.opacity || '1') + visible = Boolean( + style && + style.display !== 'none' && + style.visibility !== 'hidden' && + style.contentVisibility !== 'hidden' && + (!Number.isFinite(opacity) || opacity > 0.01) && + !current.hasAttribute('hidden') + ) + if (current.getAttribute('aria-hidden') === 'true') hidden.push(current) + if (current.parentElement) current = current.parentElement + else { + const root = current.getRootNode() + current = 'host' in root ? (root.host as Element) : null + } + } + if (visible) rendered.push({ modal, hidden }) + } + if (rendered.some(({ hidden }) => hidden.length === 0)) return exemptModal + const topmost = rendered.filter(({ hidden }) => + hidden.every((ancestor) => rendered.every(({ modal }) => ancestor.contains(modal))) + ) + if (topmost.length === 1) exemptModal = topmost[0].modal + return exemptModal + } + let pointMappingReliable = true + let aboveExemptModal = false for (let current: Element | null = element; visible && current; ) { const style = view?.getComputedStyle(current) if ( @@ -3179,11 +3501,12 @@ export function readChildFrameElementState( style.visibility === 'hidden' || Number.parseFloat(style.opacity || '1') <= 0.01 || current.hasAttribute('hidden') || - current.getAttribute('aria-hidden') === 'true' + (!aboveExemptModal && current.getAttribute('aria-hidden') === 'true') ) { visible = false break } + if (current === getExemptModal()) aboveExemptModal = true if (style.transform && style.transform !== 'none') { try { if (typeof DOMMatrixReadOnly !== 'function') { From d2d230dd9315207238779170bb77761ddbb7f89c Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 24 Sep 2026 15:25:55 -0700 Subject: [PATCH 04/10] fix(browser): stage agent downloads synchronously so Electron never opens a Save dialog The save path was set only after an asynchronous non-conflicting-name lookup. When Electron's own path step won that race it opened a native Save dialog, so an agent download stalled forever at full size (and a user would see a surprise dialog). Downloads now write to a hidden staging file set during will-download and are renamed to the allocated name when they complete. --- .../src/main/browser-agent/session.test.ts | 236 +++++++++++++++--- .../desktop/src/main/browser-agent/session.ts | 183 +++++++++----- 2 files changed, 327 insertions(+), 92 deletions(-) diff --git a/apps/desktop/src/main/browser-agent/session.test.ts b/apps/desktop/src/main/browser-agent/session.test.ts index f08aa52c767..98aefd050fe 100644 --- a/apps/desktop/src/main/browser-agent/session.test.ts +++ b/apps/desktop/src/main/browser-agent/session.test.ts @@ -1,6 +1,6 @@ -import { mkdtempSync, writeFileSync } from 'node:fs' +import { existsSync, mkdirSync, mkdtempSync, readdirSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' -import { join } from 'node:path' +import { basename, dirname, join } from 'node:path' import type { MenuItemConstructorOptions, WebContents } from 'electron' import { beforeEach, describe, expect, it, vi } from 'vitest' @@ -304,11 +304,32 @@ function mockDownloadItem({ const handler = item.once.mock.calls.find(([eventName]) => eventName === 'done')?.[1] as | ((event: unknown, nextState: MockDownloadDoneState) => void) | undefined + const savePath = item.setSavePath.mock.calls.at(-1)?.[0] as string | undefined + if (state === 'completed' && savePath) writeFileSync(savePath, filename) handler?.({}, state) }, } } +/** Asserts Electron was only ever given the hidden staging file in `directory`. */ +function expectOnlyStagingSavePath( + item: { setSavePath: ReturnType }, + directory: string +) { + expect(item.setSavePath).toHaveBeenCalledOnce() + const savePath = item.setSavePath.mock.calls[0]?.[0] as string + expect(dirname(savePath)).toBe(directory) + expect(basename(savePath)).toMatch(/^\.sim-download-/) + return savePath +} + +/** Visible files in a download directory, excluding in-flight staging files. */ +function finishedDownloadFiles(directory: string): string[] { + return readdirSync(directory) + .filter((name) => !name.startsWith('.')) + .sort() +} + function startMockDownload(contents: MockView['webContents'], download: MockDownloadHarness): void { const webSession = contents.session as typeof contents.session & { on: ReturnType @@ -3357,7 +3378,7 @@ describe('browser-agent session', () => { expect(item.resume).not.toHaveBeenCalled() await vi.waitFor(() => expect(item.resume).toHaveBeenCalledOnce()) expect(item.cancel).not.toHaveBeenCalled() - expect(item.setSavePath).toHaveBeenCalledWith(join(directory, 'report.csv')) + const stagingPath = expectOnlyStagingSavePath(item, directory) expect(item.once).toHaveBeenCalledWith('done', expect.any(Function)) expect(onDownloadsChanged).toHaveBeenLastCalledWith({ scopeId: 'chat-test', @@ -3375,13 +3396,17 @@ describe('browser-agent session', () => { const done = item.once.mock.calls.find(([eventName]) => eventName === 'done')?.[1] as | ((event: unknown, state: 'completed') => void) | undefined - writeFileSync(join(directory, 'report.csv'), 'report') + writeFileSync(stagingPath, 'report') done?.({}, 'completed') - expect(session.getBrowserDownloadsState('chat-test').downloads[0]).toMatchObject({ - filename: 'report.csv', - state: 'completed', - }) + await vi.waitFor(() => + expect(session.getBrowserDownloadsState('chat-test').downloads[0]).toMatchObject({ + filename: 'report.csv', + state: 'completed', + }) + ) + expect(existsSync(stagingPath)).toBe(false) + expect(finishedDownloadFiles(directory)).toEqual(['report.csv']) expect(snapshots.get('chat-test')?.downloads[0]).toMatchObject({ filename: 'report.csv', state: 'completed', @@ -3410,6 +3435,131 @@ describe('browser-agent session', () => { }) }) + it('sets a staging save path before will-download returns so Electron never prompts', () => { + const directory = mkdtempSync(join(tmpdir(), 'sim-browser-downloads-')) + session = freshSession(win, {}, undefined, { + getDirectory: () => directory, + getFreeDiskBytes: () => Number.MAX_SAFE_INTEGER, + }) + const contents = (session.ensureTab().view as unknown as MockView).webContents + const download = mockDownloadItem({ filename: 'report.csv', totalBytes: 100 }) + + startMockDownload(contents, download) + + const stagingPath = expectOnlyStagingSavePath(download.item, directory) + expect(stagingPath).not.toBe(join(directory, 'report.csv')) + expect(download.item.cancel).not.toHaveBeenCalled() + }) + + it('waits for a slow filename allocation before publishing the completed file', async () => { + const directory = mkdtempSync(join(tmpdir(), 'sim-browser-downloads-')) + const { persistence, snapshots } = memoryBrowserPersistence() + const pathProbe = deferred() + session = freshSession(win, {}, persistence, { + getDirectory: () => directory, + getFreeDiskBytes: () => Number.MAX_SAFE_INTEGER, + pathExists: () => pathProbe.promise, + }) + const contents = (session.ensureTab().view as unknown as MockView).webContents + const download = mockDownloadItem({ filename: 'slow.bin', totalBytes: 100 }) + + startMockDownload(contents, download) + const stagingPath = expectOnlyStagingSavePath(download.item, directory) + download.emitDone('completed') + await Promise.resolve() + + expect(session.getBrowserDownloadsState('chat-test').downloads[0]).toMatchObject({ + filename: 'slow.bin', + state: 'progressing', + }) + expect(existsSync(stagingPath)).toBe(true) + + pathProbe.resolve(false) + + await vi.waitFor(() => + expect(session.getBrowserDownloadsState('chat-test').downloads[0]).toMatchObject({ + filename: 'slow.bin', + state: 'completed', + }) + ) + expect(existsSync(stagingPath)).toBe(false) + expect(finishedDownloadFiles(directory)).toEqual(['slow.bin']) + const { id } = session.getBrowserDownloadsState('chat-test').downloads[0] + expect(session.completedBrowserDownload('chat-test', id)).toEqual({ + filename: 'slow.bin', + savePath: join(directory, 'slow.bin'), + }) + expect(snapshots.get('chat-test')?.downloads[0]).toMatchObject({ + state: 'completed', + savePath: join(directory, 'slow.bin'), + }) + }) + + it('interrupts a completed download whose staging file cannot be moved into place', async () => { + const directory = mkdtempSync(join(tmpdir(), 'sim-browser-downloads-')) + session = freshSession(win, {}, undefined, { + getDirectory: () => directory, + getFreeDiskBytes: () => Number.MAX_SAFE_INTEGER, + }) + const contents = (session.ensureTab().view as unknown as MockView).webContents + const download = mockDownloadItem({ filename: 'blocked.bin', totalBytes: 100 }) + + startMockDownload(contents, download) + const stagingPath = expectOnlyStagingSavePath(download.item, directory) + await vi.waitFor(() => expect(download.item.resume).toHaveBeenCalledOnce()) + mkdirSync(join(directory, 'blocked.bin')) + download.emitDone('completed') + + await vi.waitFor(() => + expect(session.getBrowserDownloadsState('chat-test').downloads[0]).toMatchObject({ + filename: 'blocked.bin', + state: 'interrupted', + }) + ) + await vi.waitFor(() => expect(existsSync(stagingPath)).toBe(false)) + const { id } = session.getBrowserDownloadsState('chat-test').downloads[0] + expect(session.completedBrowserDownload('chat-test', id)).toBeNull() + + vi.mocked(Menu.buildFromTemplate).mockClear() + session.showBrowserDownloadsMenu('chat-test', win, { x: 10, y: 20 }) + const template = vi.mocked(Menu.buildFromTemplate).mock.calls[0]?.[0] as + | MenuItemConstructorOptions[] + | undefined + expect(template?.[0]?.sublabel).toContain( + 'the finished download could not be moved to its destination' + ) + }) + + it('removes the staging file when a download is cancelled or interrupted', async () => { + const directory = mkdtempSync(join(tmpdir(), 'sim-browser-downloads-')) + session = freshSession(win, {}, undefined, { + getDirectory: () => directory, + getFreeDiskBytes: () => Number.MAX_SAFE_INTEGER, + }) + const contents = (session.ensureTab().view as unknown as MockView).webContents + const cancelled = mockDownloadItem({ filename: 'cancelled.bin', totalBytes: 100 }) + const interrupted = mockDownloadItem({ filename: 'interrupted.bin', totalBytes: 100 }) + + startMockDownload(contents, cancelled) + startMockDownload(contents, interrupted) + await vi.waitFor(() => expect(cancelled.item.resume).toHaveBeenCalledOnce()) + await vi.waitFor(() => expect(interrupted.item.resume).toHaveBeenCalledOnce()) + const stagingPaths = [ + expectOnlyStagingSavePath(cancelled.item, directory), + expectOnlyStagingSavePath(interrupted.item, directory), + ] + for (const stagingPath of stagingPaths) writeFileSync(stagingPath, 'partial') + + cancelled.emitDone('cancelled') + interrupted.emitDone('interrupted') + + await vi.waitFor(() => expect(readdirSync(directory)).toEqual([])) + expect(session.getBrowserDownloadsState('chat-test').downloads).toEqual([ + expect.objectContaining({ filename: 'interrupted.bin', state: 'interrupted' }), + expect.objectContaining({ filename: 'cancelled.bin', state: 'cancelled' }), + ]) + }) + it('does not let a pre-allocation progress event consume the admission probe', async () => { const directory = mkdtempSync(join(tmpdir(), 'sim-browser-downloads-')) const getFreeDiskBytes = vi.fn(() => Number.MAX_SAFE_INTEGER) @@ -3610,8 +3760,8 @@ describe('browser-agent session', () => { firstProbe.resolve(false) secondProbe.reject(new Error('late path lookup failure')) await vi.advanceTimersByTimeAsync(0) - expect(first.item.setSavePath).not.toHaveBeenCalled() - expect(second.item.setSavePath).not.toHaveBeenCalled() + expectOnlyStagingSavePath(first.item, directory) + expectOnlyStagingSavePath(second.item, directory) expect(first.item.resume).not.toHaveBeenCalled() expect(second.item.resume).not.toHaveBeenCalled() @@ -3619,10 +3769,9 @@ describe('browser-agent session', () => { second.emitDone('cancelled') const replacement = mockDownloadItem({ filename: 'hung-path.bin', totalBytes: 100 }) startMockDownload(contents, replacement) - await vi.waitFor(() => - expect(replacement.item.setSavePath).toHaveBeenCalledWith(join(directory, 'hung-path.bin')) - ) await vi.waitFor(() => expect(replacement.item.resume).toHaveBeenCalledOnce()) + replacement.emitDone('completed') + await vi.waitFor(() => expect(finishedDownloadFiles(directory)).toEqual(['hung-path.bin'])) } finally { vi.useRealTimers() } @@ -3694,7 +3843,7 @@ describe('browser-agent session', () => { startMockDownload(contents, download) await vi.waitFor(() => expect(download.item.cancel).toHaveBeenCalledOnce()) - expect(download.item.setSavePath).not.toHaveBeenCalled() + expectOnlyStagingSavePath(download.item, notDirectory) expect(download.item.resume).not.toHaveBeenCalled() expect(session.getBrowserDownloadsState('chat-test').downloads[0]).toMatchObject({ filename: 'cannot-save.bin', @@ -3735,7 +3884,6 @@ describe('browser-agent session', () => { startMockDownload(contents, download) await vi.waitFor(() => expect(download.item.resume).toHaveBeenCalledOnce()) - const firstSavePath = download.item.setSavePath.mock.calls[0]?.[0] download.setReceivedBytes(2 * 1024 ** 3 + 1) download.emitUpdated() @@ -3750,7 +3898,9 @@ describe('browser-agent session', () => { const replacement = mockDownloadItem({ filename: 'stream.bin', totalBytes: 100 }) startMockDownload(contents, replacement) expect(replacement.item.cancel).not.toHaveBeenCalled() - await vi.waitFor(() => expect(replacement.item.setSavePath).toHaveBeenCalledWith(firstSavePath)) + await vi.waitFor(() => expect(replacement.item.resume).toHaveBeenCalledOnce()) + replacement.emitDone('completed') + await vi.waitFor(() => expect(finishedDownloadFiles(directory)).toEqual(['stream.bin'])) }) it('throttles free-disk checks while stopping promptly after the interval', async () => { @@ -3867,9 +4017,11 @@ describe('browser-agent session', () => { .mockReturnValueOnce(firstProbe.promise) .mockReturnValueOnce(secondProbe.promise) .mockReturnValue(Number.MAX_SAFE_INTEGER) + const pathExists = vi.fn((_path: string) => false) session = freshSession(win, {}, persistence, { getDirectory: () => directory, getFreeDiskBytes, + pathExists, }) const contents = (session.ensureTab().view as unknown as MockView).webContents const first = mockDownloadItem({ filename: 'same-name.bin', totalBytes: 100 }) @@ -3878,12 +4030,11 @@ describe('browser-agent session', () => { startMockDownload(contents, first) startMockDownload(contents, second) await vi.waitFor(() => expect(getFreeDiskBytes).toHaveBeenCalledTimes(2)) - const allocatedPaths = [ - first.item.setSavePath.mock.calls[0]?.[0], - second.item.setSavePath.mock.calls[0]?.[0], - ] - expect(new Set(allocatedPaths).size).toBe(2) - expect(allocatedPaths).toContain(join(directory, 'same-name.bin')) + expect(pathExists.mock.calls.map(([candidate]) => candidate)).toEqual([ + join(directory, 'same-name.bin'), + join(directory, 'same-name.bin'), + expect.stringMatching(/same-name \(.+\)\.bin$/), + ]) await session.clearProfileStorage() @@ -3903,9 +4054,6 @@ describe('browser-agent session', () => { const nextContents = (session.ensureTab().view as unknown as MockView).webContents const replacement = mockDownloadItem({ filename: 'same-name.bin', totalBytes: 100 }) startMockDownload(nextContents, replacement) - await vi.waitFor(() => - expect(replacement.item.setSavePath).toHaveBeenCalledWith(join(directory, 'same-name.bin')) - ) await vi.waitFor(() => expect(replacement.item.resume).toHaveBeenCalledOnce()) expect(replacement.item.cancel).not.toHaveBeenCalled() @@ -3913,8 +4061,17 @@ describe('browser-agent session', () => { second.emitDone('cancelled') const concurrent = mockDownloadItem({ filename: 'same-name.bin', totalBytes: 100 }) startMockDownload(nextContents, concurrent) - await vi.waitFor(() => expect(concurrent.item.setSavePath).toHaveBeenCalledOnce()) - expect(concurrent.item.setSavePath).not.toHaveBeenCalledWith(join(directory, 'same-name.bin')) + await vi.waitFor(() => expect(concurrent.item.resume).toHaveBeenCalledOnce()) + concurrent.emitDone('completed') + await vi.waitFor(() => + expect(finishedDownloadFiles(directory)).toEqual([ + expect.stringMatching(/^same-name \(.+\)\.bin$/), + ]) + ) + replacement.emitDone('completed') + await vi.waitFor(() => expect(finishedDownloadFiles(directory)).toHaveLength(2)) + expect(finishedDownloadFiles(directory)).toContain('same-name.bin') + await vi.waitFor(() => expect(readdirSync(directory)).toHaveLength(2)) }) it('does not reserve a late filename after profile teardown starts', async () => { @@ -3931,7 +4088,7 @@ describe('browser-agent session', () => { await Promise.resolve() expect(download.item.cancel).toHaveBeenCalledOnce() - expect(download.item.setSavePath).not.toHaveBeenCalled() + expectOnlyStagingSavePath(download.item, directory) expect(download.item.resume).not.toHaveBeenCalled() }) @@ -3970,15 +4127,21 @@ describe('browser-agent session', () => { queueMicrotask(() => session.disposeBrowserScope('chat-first')) secondPathProbe.resolve(false) - await vi.waitFor(() => - expect(second.item.setSavePath).toHaveBeenCalledWith(join(directory, 'shared.bin')) - ) - expect(first.item.setSavePath).not.toHaveBeenCalled() + await vi.waitFor(() => expect(second.item.resume).toHaveBeenCalledOnce()) + expectOnlyStagingSavePath(first.item, directory) + expect(first.item.resume).not.toHaveBeenCalled() const third = mockDownloadItem({ filename: 'shared.bin', totalBytes: 100 }) startMockDownload(thirdContents, third) - await vi.waitFor(() => expect(third.item.setSavePath).toHaveBeenCalledOnce()) - expect(third.item.setSavePath).not.toHaveBeenCalledWith(join(directory, 'shared.bin')) + await vi.waitFor(() => expect(third.item.resume).toHaveBeenCalledOnce()) + third.emitDone('completed') + await vi.waitFor(() => + expect(finishedDownloadFiles(directory)).toEqual([ + expect.stringMatching(/^shared \(.+\)\.bin$/), + ]) + ) + second.emitDone('completed') + await vi.waitFor(() => expect(finishedDownloadFiles(directory)).toContain('shared.bin')) }) it('cancels only the disposed scope and ignores its late download callbacks', async () => { @@ -4017,7 +4180,7 @@ describe('browser-agent session', () => { disposedDownload.emitUpdated() disposedDownload.emitDone('cancelled') - expect(disposedDownload.item.setSavePath).not.toHaveBeenCalled() + expectOnlyStagingSavePath(disposedDownload.item, directory) expect(disposedDownload.item.resume).not.toHaveBeenCalled() expect(session.getBrowserDownloadsState('chat-disposed').downloads).toEqual([]) expect(onDownloadsChanged).not.toHaveBeenCalledWith( @@ -4101,10 +4264,11 @@ describe('browser-agent session', () => { expect(rejected.item.cancel).toHaveBeenCalledOnce() first.emitDone('completed') + await vi.waitFor(() => expect(finishedDownloadFiles(directory)).toEqual(['first.txt'])) const replacement = mockDownloadItem({ filename: 'fourth.txt', totalBytes: 100 }) startMockDownload(contents, replacement) expect(replacement.item.cancel).not.toHaveBeenCalled() - await vi.waitFor(() => expect(replacement.item.setSavePath).toHaveBeenCalledOnce()) + await vi.waitFor(() => expect(replacement.item.resume).toHaveBeenCalledOnce()) }) it('bounds active browser downloads across tasks', () => { diff --git a/apps/desktop/src/main/browser-agent/session.ts b/apps/desktop/src/main/browser-agent/session.ts index c9fed5fed65..b27d57579ea 100644 --- a/apps/desktop/src/main/browser-agent/session.ts +++ b/apps/desktop/src/main/browser-agent/session.ts @@ -1,6 +1,6 @@ import { AsyncLocalStorage } from 'node:async_hooks' import { existsSync } from 'node:fs' -import { statfs } from 'node:fs/promises' +import { rename, rm, statfs } from 'node:fs/promises' import { join } from 'node:path' import type { BrowserDataKind, @@ -23,7 +23,7 @@ import type { } from '@sim/desktop-bridge' import { createLogger } from '@sim/logger' import { getErrorMessage } from '@sim/utils/errors' -import { generateId } from '@sim/utils/id' +import { generateId, generateShortId } from '@sim/utils/id' import type { BrowserWindow, BrowserWindowConstructorOptions, @@ -423,7 +423,17 @@ interface ActiveBrowserDownload { item: DownloadItem diskCheckInFlight: boolean lastDiskCheckAt: number + /** + * Electron's download delegate opens a native Save dialog unless a path is + * set before `will-download` returns, so bytes land in this random hidden + * file and move to the asynchronously allocated `savePath` on completion. + */ + stagingPath: string + /** The reserved final destination, once allocation has chosen one. */ savePath?: string + /** Settles with the final destination, or null when allocation failed. */ + destination: Promise + finished: boolean scopeId: string terminal: boolean limitReason?: string @@ -640,7 +650,7 @@ function checkBrowserDownloadDiskSpace( check: 'admission' | 'progress', now = Date.now() ): void { - if (active.terminal || active.limitReason || active.diskCheckInFlight) return + if (active.terminal || active.finished || active.limitReason || active.diskCheckInFlight) return if ( check === 'progress' && now - active.lastDiskCheckAt < BROWSER_DOWNLOAD_DISK_CHECK_INTERVAL_MS @@ -699,6 +709,61 @@ function releaseActiveBrowserDownloadPath( } } +function discardStagedBrowserDownload(active: ActiveBrowserDownload): void { + void rm(active.stagingPath, { force: true }).catch((error) => { + logger.warn('Could not remove a staged agent browser download', { + error: getErrorMessage(error), + filename: active.download.filename, + }) + }) +} + +/** Moves a completed staging file to its final name; resolves to that name. */ +async function moveStagedBrowserDownload(active: ActiveBrowserDownload): Promise { + const destination = await active.destination + if (!destination || active.limitReason || active.terminal) { + throw new Error( + active.limitReason ?? 'Stopped: the download destination could not be prepared safely' + ) + } + try { + await rename(active.stagingPath, destination) + } catch (error) { + logger.warn('Could not move a finished agent browser download to its destination', { + error: getErrorMessage(error), + filename: active.download.filename, + }) + throw new Error('Stopped: the finished download could not be moved to its destination') + } + return destination +} + +function finishBrowserDownload(active: ActiveBrowserDownload): void { + const { download } = active + const liveScopeId = resolveBrowserScopeId(active.scopeId) + if ( + suspendedBrowserScopes.has(liveScopeId) || + !browserScopeStates.has(liveScopeId) || + !browserDownloadsByScope.get(liveScopeId)?.includes(download) + ) { + return + } + trimBrowserDownloads(liveScopeId) + publishBrowserDownloads(liveScopeId) + withBrowserScope(liveScopeId, persistBrowserSession) + if (download.state === 'completed') { + logger.info('Agent browser download completed', { filename: download.filename }) + if (process.platform === 'darwin' && download.savePath) { + app.dock?.downloadFinished(download.savePath) + } + } else if (download.state === 'interrupted') { + logger.warn('Agent browser download interrupted', { + filename: download.filename, + reason: download.interruptionReason, + }) + } +} + function cancelActiveBrowserDownloads(scopeId?: string): void { const resolvedScopeId = scopeId === undefined ? null : resolveBrowserScopeId(scopeId) const downloads = [...activeBrowserDownloads].filter( @@ -1489,26 +1554,40 @@ function configureBrowserDownloads(ses: Session): void { const download = createTrackedBrowserDownload(item, 'progressing') const { filename } = download - try { - item.pause() - } catch (error) { - const reason = 'Stopped: the download could not be paused for a disk-space safety check' + const rejectBeforeTracking = (reason: string, message: string, error: unknown) => { download.interruptionReason = reason download.state = 'interrupted' try { item.cancel() } catch (cancelError) { - logger.warn('Could not cancel an agent browser download after pause failed', { + logger.warn('Could not cancel an agent browser download after setup failed', { error: getErrorMessage(cancelError), filename, }) } recordBrowserDownload(scopeId, download) withBrowserScope(scopeId, persistBrowserSession) - logger.warn('Agent browser download could not be paused for admission', { - error: getErrorMessage(error), - filename, - }) + logger.warn(message, { error: getErrorMessage(error), filename }) + } + const stagingPath = join(directory, `.sim-download-${generateShortId()}`) + try { + item.setSavePath(stagingPath) + } catch (error) { + rejectBeforeTracking( + 'Stopped: the download destination could not be prepared safely', + 'Could not set the staging destination for an agent browser download', + error + ) + return + } + try { + item.pause() + } catch (error) { + rejectBeforeTracking( + 'Stopped: the download could not be paused for a disk-space safety check', + 'Agent browser download could not be paused for admission', + error + ) return } const active: ActiveBrowserDownload = { @@ -1517,6 +1596,9 @@ function configureBrowserDownloads(ses: Session): void { item, diskCheckInFlight: false, lastDiskCheckAt: 0, + stagingPath, + destination: Promise.resolve(null), + finished: false, scopeId, terminal: false, } @@ -1547,31 +1629,36 @@ function configureBrowserDownloads(ses: Session): void { publishBrowserDownloads(liveScopeId) }) item.once('done', (_doneEvent, state) => { - releaseActiveBrowserDownload(active) - const liveScopeId = resolveBrowserScopeId(scopeId) - if ( - suspendedBrowserScopes.has(liveScopeId) || - !browserScopeStates.has(liveScopeId) || - !browserDownloadsByScope.get(liveScopeId)?.includes(download) - ) { + active.finished = true + updateDownloadProgress(download, item) + if (state !== 'completed' || active.limitReason || active.terminal) { + const tornDown = active.terminal + releaseActiveBrowserDownload(active) + discardStagedBrowserDownload(active) + download.savePath = active.savePath + if (active.limitReason) download.state = 'interrupted' + else if (tornDown && state === 'completed') download.state = 'cancelled' + else download.state = state + finishBrowserDownload(active) return } - updateDownloadProgress(download, item) - download.state = active.limitReason ? 'interrupted' : state - trimBrowserDownloads(liveScopeId) - publishBrowserDownloads(liveScopeId) - withBrowserScope(liveScopeId, persistBrowserSession) - if (download.state === 'completed') { - logger.info('Agent browser download completed', { filename }) - if (process.platform === 'darwin' && active.savePath) { - app.dock?.downloadFinished(active.savePath) - } - } else if (download.state === 'interrupted') { - logger.warn('Agent browser download interrupted', { - filename, - reason: download.interruptionReason, + void moveStagedBrowserDownload(active) + .then( + (savePath) => { + download.savePath = savePath + download.state = 'completed' + }, + (error: unknown) => { + discardStagedBrowserDownload(active) + download.savePath = active.savePath + download.interruptionReason = getErrorMessage(error) + download.state = 'interrupted' + } + ) + .finally(() => { + releaseActiveBrowserDownload(active) + finishBrowserDownload(active) }) - } }) let allocationExpired = false const allocation = uniqueDownloadPath(directory, filename, { @@ -1596,7 +1683,7 @@ function configureBrowserDownloads(ses: Session): void { return true }, }) - void withBrowserDownloadTimeout( + active.destination = withBrowserDownloadTimeout( allocation, BROWSER_DOWNLOAD_PATH_ALLOCATION_TIMEOUT_MS, 'Browser download path allocation timed out', @@ -1607,7 +1694,7 @@ function configureBrowserDownloads(ses: Session): void { .then((savePath) => { if (active.terminal || !activeBrowserDownloads.has(active)) { releaseActiveBrowserDownloadPath(active, savePath ?? undefined) - return + return null } if (!savePath) { cancelBrowserDownloadForLimit( @@ -1615,30 +1702,13 @@ function configureBrowserDownloads(ses: Session): void { 'Stopped: a safe non-conflicting download filename could not be allocated' ) publishActiveBrowserDownload(active) - return - } - download.savePath = savePath - try { - item.setSavePath(savePath) - } catch (error) { - releaseActiveBrowserDownloadPath(active, savePath) - active.savePath = undefined - download.savePath = undefined - logger.warn('Could not set the destination for an agent browser download', { - error: getErrorMessage(error), - filename, - }) - cancelBrowserDownloadForLimit( - active, - 'Stopped: the download destination could not be prepared safely' - ) - publishActiveBrowserDownload(active) - return + return null } checkBrowserDownloadDiskSpace(active, 'admission') + return savePath }) .catch((error) => { - if (active.terminal || !activeBrowserDownloads.has(active)) return + if (active.terminal || !activeBrowserDownloads.has(active)) return null logger.warn('Could not allocate an agent browser download destination', { error: getErrorMessage(error), filename, @@ -1648,6 +1718,7 @@ function configureBrowserDownloads(ses: Session): void { 'Stopped: the download destination could not be prepared safely' ) publishActiveBrowserDownload(active) + return null }) }) } From e44dca853309b9d69e78af539463c24f25c4ed60 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 24 Sep 2026 15:38:13 -0700 Subject: [PATCH 05/10] feat(browser): mark snapshot elements that appeared since the previous snapshot After an action the agent saw a fresh snapshot but no hint of what changed, so a popup, suggestion list, or validation message looked like any other line. The page now remembers which elements an earlier snapshot of the document listed, and later snapshots mark the rest new (the first snapshot marks nothing). --- .../main/browser-agent/page-functions.test.ts | 18 ++++++++++++++++++ .../src/main/browser-agent/page-functions.ts | 13 ++++++++++++- 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/apps/desktop/src/main/browser-agent/page-functions.test.ts b/apps/desktop/src/main/browser-agent/page-functions.test.ts index 920e18ec150..7db42f649f2 100644 --- a/apps/desktop/src/main/browser-agent/page-functions.test.ts +++ b/apps/desktop/src/main/browser-agent/page-functions.test.ts @@ -115,6 +115,7 @@ beforeEach(() => { for (const state of window.__simAgentMutationStates ?? []) state.observer.disconnect() window.__simAgentMutationStates = undefined window.__simAgentNextElementId = 0 + window.__simAgentShownElements = undefined window.__simAgentResolveElement = undefined installDomShims() Reflect.deleteProperty(document, 'activeElement') @@ -748,6 +749,23 @@ describe('collectSnapshot', () => { expect(outline).not.toContain('button "x" [ref=999]') }) + it('marks elements that appeared since the previous snapshot as new', () => { + document.body.innerHTML = '' + visible(document.querySelector('button') as HTMLButtonElement) + expect(outlineOf(collectSnapshot())).not.toContain(' new') + + const dialog = document.createElement('div') + dialog.innerHTML = '' + document.body.append(dialog) + dialog.querySelectorAll('*').forEach((el) => visible(el as HTMLElement)) + const lines = outlineOf(collectSnapshot()).split('\n') + + expect(lines.find((line) => line.includes('"Compose"'))).not.toMatch(/ new$/) + expect(lines.find((line) => line.includes('"Recipients"'))).toMatch(/ new$/) + expect(lines.find((line) => line.includes('"Send"'))).toMatch(/ new$/) + expect(outlineOf(collectSnapshot())).not.toContain(' new') + }) + it('sanitizes a malicious role so it cannot forge a second snapshot line', () => { document.body.innerHTML = '
' const control = visible(document.querySelector('div') as HTMLDivElement) diff --git a/apps/desktop/src/main/browser-agent/page-functions.ts b/apps/desktop/src/main/browser-agent/page-functions.ts index 154ee983d0f..cd0c7905c60 100644 --- a/apps/desktop/src/main/browser-agent/page-functions.ts +++ b/apps/desktop/src/main/browser-agent/page-functions.ts @@ -41,6 +41,8 @@ declare global { observedRoots: WeakSet }> __simAgentNextElementId?: number + /** Elements an earlier snapshot of this document listed; a later snapshot marks the rest `new`. */ + __simAgentShownElements?: WeakSet /** Why the last __simAgentResolveElement call returned null — read by the * shared stale-error producers so a refusal names its cause instead of * the blanket "the page changed". Cleared on every successful resolve. */ @@ -160,6 +162,11 @@ export function collectSnapshot(startingElementId = 0, elementId?: number): unkn context: string }> = [] window.__simAgentElements = registry + const previouslyShown = window.__simAgentShownElements + const shown = previouslyShown ?? new WeakSet() + window.__simAgentShownElements = shown + /** Whether an element appeared since the previous snapshot; the first snapshot marks nothing. */ + const isNew = (el: Element): boolean => previouslyShown !== undefined && !shown.has(el) const lines: string[] = [] let truncated = false let refCount = 0 @@ -553,6 +560,8 @@ export function collectSnapshot(startingElementId = 0, elementId?: number): unkn parts.push(`${attribute}=${value}`) } } + if (isNew(el)) parts.push('new') + shown.add(el) const suffix = parts.length > 0 ? ` ${parts.join(' ')}` : '' const lineIndex = lines.length if (push(`${indent}- ${role} ${quote(name)} [ref=${id}]${suffix}`)) { @@ -574,8 +583,10 @@ export function collectSnapshot(startingElementId = 0, elementId?: number): unkn if (!text) return const id = registerElement(el, roleFor(el), text) textLineCount++ + const marker = isNew(el) ? ' new' : '' + shown.add(el) const lineIndex = lines.length - if (push(`${indent}- text ${quote(text)} [ref=${id}]`)) refLineIndexes[id] = lineIndex + if (push(`${indent}- text ${quote(text)} [ref=${id}]${marker}`)) refLineIndexes[id] = lineIndex } const headingLevel = (el: Element): number | null => { From 9da1870b3ebda761a060fac9b10dcecc4a7497e8 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 24 Sep 2026 17:45:37 -0700 Subject: [PATCH 06/10] refactor(browser): tighten the computer-use desktop changes - Modifier aliases (Ctrl, Cmd, Command, Option) share one descriptor with their canonical key, so a bare alias sets its own flag; a bare modifier's key-up reports it released. - The framed-control fallback no longer treats a held click as a plain click, and a batch refuses press-and-hold so eight holds cannot outlast its watchdog. - installPageHelpers caches the modal lookup per invocation, and one serializePageCall builds the page expression for the driver and the tests. - Download completion drops a branch whose state was never published. --- .../main/browser-agent/context-menu.test.ts | 16 +- .../src/main/browser-agent/driver.test.ts | 11 + apps/desktop/src/main/browser-agent/driver.ts | 14 +- .../src/main/browser-agent/keyboard.test.ts | 15 +- .../src/main/browser-agent/keyboard.ts | 70 ++-- .../main/browser-agent/page-functions.test.ts | 6 +- .../src/main/browser-agent/page-functions.ts | 389 ++++-------------- .../src/main/browser-agent/session.test.ts | 11 +- .../desktop/src/main/browser-agent/session.ts | 18 +- 9 files changed, 169 insertions(+), 381 deletions(-) diff --git a/apps/desktop/src/main/browser-agent/context-menu.test.ts b/apps/desktop/src/main/browser-agent/context-menu.test.ts index 9e3772f55bb..0ab0c6cd395 100644 --- a/apps/desktop/src/main/browser-agent/context-menu.test.ts +++ b/apps/desktop/src/main/browser-agent/context-menu.test.ts @@ -4,7 +4,7 @@ import { describe, expect, it, vi } from 'vitest' vi.mock('electron', () => import('@/test/electron-mock')) import { Menu, WebContentsView } from 'electron' -import { clickAt } from '@/main/browser-agent/cdp' +import { clickAt, PRIMARY_CLICK } from '@/main/browser-agent/cdp' import { attachAgentContextMenu, BASE_ZOOM_FACTOR, @@ -228,12 +228,7 @@ describe('attachAgentContextMenu', () => { ContextMenuListener, ][] const onContextMenu = listeners.find(([event]) => event === 'context-menu')![1] - await clickAt(contents, 10, 20, false, { - button: 'right', - clickCount: 1, - modifiers: 0, - holdMs: 0, - }) + await clickAt(contents, 10, 20, false, { ...PRIMARY_CLICK, button: 'right' }) vi.mocked(Menu.buildFromTemplate).mockClear() onContextMenu({}, params()) @@ -257,12 +252,7 @@ describe('attachAgentContextMenu', () => { ][] const onInput = listeners.find(([event]) => event === 'input-event')?.[1] const onContextMenu = listeners.find(([event]) => event === 'context-menu')![1] - await clickAt(contents, 10, 20, false, { - button: 'right', - clickCount: 1, - modifiers: 0, - holdMs: 0, - }) + await clickAt(contents, 10, 20, false, { ...PRIMARY_CLICK, button: 'right' }) vi.mocked(Menu.buildFromTemplate).mockClear() onInput?.({}, { type: inputEvent }) diff --git a/apps/desktop/src/main/browser-agent/driver.test.ts b/apps/desktop/src/main/browser-agent/driver.test.ts index 9f3a4daf86a..e3d1aabd3c6 100644 --- a/apps/desktop/src/main/browser-agent/driver.test.ts +++ b/apps/desktop/src/main/browser-agent/driver.test.ts @@ -3624,6 +3624,17 @@ describe('credential protection', () => { error: expect.stringContaining('Batch action 0'), }) expect(observed).toMatchObject({ ok: false, error: expect.stringContaining('cannot observe') }) + + const held = await driver.executeTool('chat-test', 'browser_batch', { + actions: [ + { tool: 'browser_click', args: { elementId: 0, holdMs: 2000 } }, + { tool: 'browser_click', args: { elementId: 0 } }, + ], + }) + expect(held).toMatchObject({ + ok: false, + error: expect.stringContaining('cannot press and hold'), + }) }) it('keeps element ids valid when an observed action is refused before dispatch', async () => { diff --git a/apps/desktop/src/main/browser-agent/driver.ts b/apps/desktop/src/main/browser-agent/driver.ts index 72e6623ac90..30d86866487 100644 --- a/apps/desktop/src/main/browser-agent/driver.ts +++ b/apps/desktop/src/main/browser-agent/driver.ts @@ -76,6 +76,7 @@ import { resolveFileInputTarget, scrollPage, selectOptionInElement, + serializePageCall, setFocusedInputValue, typeIntoElement, } from '@/main/browser-agent/page-functions' @@ -187,6 +188,9 @@ function parseBatchActions(params: Record): BatchAction[] { if ('observe' in action.args) { throw new ToolError(`Batch action ${index} cannot observe; pass observe on the batch itself.`) } + if (num(action.args, 'holdMs')) { + throw new ToolError(`Batch action ${index} cannot press and hold; run it as its own click.`) + } return { tool: action.tool, args: action.args } }) } @@ -1121,10 +1125,10 @@ const POINTER_BUTTONS: ReadonlySet = new Set(['left', 'right', 'middle'] /** Enough to walk a slider or list by keyboard in one call without flooding the page. */ const MAX_KEY_REPEAT = 50 -/** The optional click gesture shared by `browser_click` and `browser_click_at`. */ /** Longest press-and-hold a click may request; well inside the click tool's watchdog. */ const MAX_POINTER_HOLD_MS = 10_000 +/** The optional click gesture shared by `browser_click` and `browser_click_at`. */ function pointerClick(params: Record): cdp.PointerClick { const button = str(params, 'button') ?? 'left' if (!POINTER_BUTTONS.has(button)) throw new ToolError('button must be left, right, or middle.') @@ -1176,7 +1180,9 @@ function uploadPaths(params: Record): string[] { } function isPrimaryClick(click: cdp.PointerClick): boolean { - return click.button === 'left' && click.clickCount === 1 && click.modifiers === 0 + return ( + click.button === 'left' && click.clickCount === 1 && click.modifiers === 0 && click.holdMs === 0 + ) } const DIALOG_ANSWERING_TOOLS: ReadonlySet = new Set([ @@ -1287,7 +1293,7 @@ async function execInPage( 'The active tab is blank. Call browser_navigate before using page inspection or interaction tools.' ) } - const invocation = `(${String(fn)}).apply(null, ${JSON.stringify(args)})` + const invocation = serializePageCall(fn as (...args: never[]) => unknown, args) const expression = typeof notAfter === 'number' ? `(Date.now() >= ${Math.floor(notAfter)} ? ({error: "expired"}) : ${invocation})` @@ -3274,7 +3280,7 @@ async function executeToolInner( } else { if (!isPrimaryClick(click)) { throw new ToolError( - 'This framed control has no reliable pointer position, so only a plain left click can activate it. Use browser_screenshot and browser_click_at for other buttons, click counts, or modifiers.' + 'This framed control has no reliable pointer position, so only a plain left click can activate it. Use browser_screenshot and browser_click_at for other buttons, click counts, holds, or modifiers.' ) } const activationKey = prepared.activationKey diff --git a/apps/desktop/src/main/browser-agent/keyboard.test.ts b/apps/desktop/src/main/browser-agent/keyboard.test.ts index e7ee3011096..61fc56b9972 100644 --- a/apps/desktop/src/main/browser-agent/keyboard.test.ts +++ b/apps/desktop/src/main/browser-agent/keyboard.test.ts @@ -208,10 +208,23 @@ describe('modifierKeyEvents', () => { code: 'ControlLeft', modifiers: 2, }) - expect(up).toMatchObject({ type: 'keyUp', key: 'Control' }) + expect(up).toMatchObject({ type: 'keyUp', key: 'Control', modifiers: 0 }) expect(modifierKeyEvents(combo, 'linux')).toEqual({ downs: [], ups: [] }) }) + it('treats modifier aliases like their canonical key', () => { + for (const [alias, key, flag] of [ + ['Ctrl', 'Control', 2], + ['Option', 'Alt', 1], + ['Cmd', 'Meta', 4], + ['Command', 'Meta', 4], + ] as const) { + const [down, up] = buildKeyDispatchPlan(parseKeyCombo(alias, 'linux'), 'linux') + expect(down).toMatchObject({ key, modifiers: flag }) + expect(up).toMatchObject({ type: 'keyUp', key, modifiers: 0 }) + } + }) + it('sends no extra events for a key without modifiers', () => { expect(modifierKeyEvents(parseKeyCombo('a', 'linux'), 'linux')).toEqual({ downs: [], ups: [] }) }) diff --git a/apps/desktop/src/main/browser-agent/keyboard.ts b/apps/desktop/src/main/browser-agent/keyboard.ts index e3bd5b11ed1..c718d7d0e75 100644 --- a/apps/desktop/src/main/browser-agent/keyboard.ts +++ b/apps/desktop/src/main/browser-agent/keyboard.ts @@ -16,6 +16,11 @@ interface KeyDescriptor { keyCode: number } +const CONTROL_KEY: KeyDescriptor = { key: 'Control', code: 'ControlLeft', keyCode: 17 } +const SHIFT_KEY: KeyDescriptor = { key: 'Shift', code: 'ShiftLeft', keyCode: 16 } +const ALT_KEY: KeyDescriptor = { key: 'Alt', code: 'AltLeft', keyCode: 18 } +const META_KEY: KeyDescriptor = { key: 'Meta', code: 'MetaLeft', keyCode: 91 } + const NAMED_KEYS: Record = { enter: { key: 'Enter', code: 'Enter', keyCode: 13 }, escape: { key: 'Escape', code: 'Escape', keyCode: 27 }, @@ -51,14 +56,14 @@ const NAMED_KEYS: Record = { '`': { key: '`', code: 'Backquote', keyCode: 192 }, plus: { key: '+', code: 'Equal', keyCode: 187 }, insert: { key: 'Insert', code: 'Insert', keyCode: 45 }, - control: { key: 'Control', code: 'ControlLeft', keyCode: 17 }, - ctrl: { key: 'Control', code: 'ControlLeft', keyCode: 17 }, - shift: { key: 'Shift', code: 'ShiftLeft', keyCode: 16 }, - alt: { key: 'Alt', code: 'AltLeft', keyCode: 18 }, - option: { key: 'Alt', code: 'AltLeft', keyCode: 18 }, - meta: { key: 'Meta', code: 'MetaLeft', keyCode: 91 }, - cmd: { key: 'Meta', code: 'MetaLeft', keyCode: 91 }, - command: { key: 'Meta', code: 'MetaLeft', keyCode: 91 }, + control: CONTROL_KEY, + ctrl: CONTROL_KEY, + shift: SHIFT_KEY, + alt: ALT_KEY, + option: ALT_KEY, + meta: META_KEY, + cmd: META_KEY, + command: META_KEY, ...Object.fromEntries( Array.from({ length: 12 }, (_, index) => [ `f${index + 1}`, @@ -108,12 +113,12 @@ const BASE_FOR_SHIFTED_CHARACTER: Record = Object.fromEntries( Object.entries(SHIFTED_CHARACTERS).map(([base, shifted]) => [shifted, base]) ) -/** Modifier keys in the order a chord presses them, each with the flag its key-down sets. */ +/** Modifier keys in the fixed order a chord presses them, each with the flag its key-down sets. */ const MODIFIER_KEYS: readonly { flag: keyof KeyModifiers; descriptor: KeyDescriptor }[] = [ - { flag: 'ctrl', descriptor: NAMED_KEYS.control }, - { flag: 'alt', descriptor: NAMED_KEYS.alt }, - { flag: 'shift', descriptor: NAMED_KEYS.shift }, - { flag: 'meta', descriptor: NAMED_KEYS.meta }, + { flag: 'ctrl', descriptor: CONTROL_KEY }, + { flag: 'alt', descriptor: ALT_KEY }, + { flag: 'shift', descriptor: SHIFT_KEY }, + { flag: 'meta', descriptor: META_KEY }, ] export interface KeyModifiers { @@ -312,7 +317,11 @@ export function buildKeyDispatchPlan( ...(text !== undefined ? { text } : {}), ...(commands.length > 0 ? { commands } : {}), } - return [down, { ...base, type: 'keyUp' }] + const ownModifier = MODIFIER_KEYS.find(({ descriptor }) => descriptor.key === combo.key) + const upModifiers = ownModifier + ? cdpModifiers({ ...combo, [ownModifier.flag]: false }) + : modifiers + return [down, { ...base, type: 'keyUp', modifiers: upModifiers }] } /** @@ -326,25 +335,22 @@ export function modifierKeyEvents( ): { downs: cdp.CdpKeyEvent[]; ups: cdp.CdpKeyEvent[] } { const combo = normalizeComboForPlatform(rawCombo, platform) const held = { ctrl: false, meta: false, shift: false, alt: false } - const downs: cdp.CdpKeyEvent[] = [] - const ups: cdp.CdpKeyEvent[] = [] - for (const { flag, descriptor } of MODIFIER_KEYS) { - if (!combo[flag] || descriptor.key === combo.key) continue + const pressed = MODIFIER_KEYS.filter( + ({ flag, descriptor }) => combo[flag] && descriptor.key !== combo.key + ) + const event = ({ key, code, keyCode }: KeyDescriptor) => ({ + key, + code, + windowsVirtualKeyCode: keyCode, + }) + const downs = pressed.map(({ flag, descriptor }) => { held[flag] = true - const event = { - key: descriptor.key, - code: descriptor.code, - windowsVirtualKeyCode: descriptor.keyCode, - } - downs.push({ ...event, type: 'rawKeyDown', modifiers: cdpModifiers(held) }) - ups.unshift({ ...event, type: 'keyUp', modifiers: 0 }) - } - let remaining = { ...held } - for (const up of ups) { - const flag = MODIFIER_KEYS.find((modifier) => modifier.descriptor.key === up.key)?.flag - if (flag) remaining = { ...remaining, [flag]: false } - up.modifiers = cdpModifiers(remaining) - } + return { ...event(descriptor), type: 'rawKeyDown' as const, modifiers: cdpModifiers(held) } + }) + const ups = [...pressed].reverse().map(({ flag, descriptor }) => { + held[flag] = false + return { ...event(descriptor), type: 'keyUp' as const, modifiers: cdpModifiers(held) } + }) return { downs, ups } } diff --git a/apps/desktop/src/main/browser-agent/page-functions.test.ts b/apps/desktop/src/main/browser-agent/page-functions.test.ts index 7db42f649f2..cc93ff2aa7e 100644 --- a/apps/desktop/src/main/browser-agent/page-functions.test.ts +++ b/apps/desktop/src/main/browser-agent/page-functions.test.ts @@ -11,6 +11,7 @@ import { getElementScreenshotRect, getViewportInfo, hoverElement, + installPageHelpers, pageContainsText, pressKeyOnPage, readActiveElementState, @@ -22,6 +23,7 @@ import { resolveFileInputTarget, scrollPage, selectOptionInElement, + serializePageCall, setFocusedInputValue, typeIntoElement, } from '@/main/browser-agent/page-functions' @@ -60,8 +62,7 @@ function installDomShims(): void { * fails here exactly as it would in a real page. */ function runSerialized(fn: (...args: never[]) => unknown, args: unknown[]): unknown { - const expression = `(${String(fn)}).apply(null, ${JSON.stringify(args)})` - return new Function(`return ${expression}`)() + return new Function(`return ${serializePageCall(fn, args)}`)() } function visible(el: T): T { @@ -116,6 +117,7 @@ beforeEach(() => { window.__simAgentMutationStates = undefined window.__simAgentNextElementId = 0 window.__simAgentShownElements = undefined + installPageHelpers() window.__simAgentResolveElement = undefined installDomShims() Reflect.deleteProperty(document, 'activeElement') diff --git a/apps/desktop/src/main/browser-agent/page-functions.ts b/apps/desktop/src/main/browser-agent/page-functions.ts index cd0c7905c60..50d3a5e8562 100644 --- a/apps/desktop/src/main/browser-agent/page-functions.ts +++ b/apps/desktop/src/main/browser-agent/page-functions.ts @@ -3,7 +3,9 @@ * The driver serializes each function's source (`String(fn)`) and calls it * with JSON-encoded arguments, so every function here MUST be fully * self-contained: no imports, no closed-over variables, only its own - * arguments and page globals. Helpers live INSIDE the function that uses them. + * arguments and page globals. Helpers live INSIDE the function that uses them; + * the one exception is {@link installPageHelpers}, which {@link serializePageCall} + * runs before every function so shared helpers exist once. * * The element registry (`window.__simAgentElements`) is rebuilt by every * snapshot and naturally cleared by navigation. A snapshot also installs a @@ -43,6 +45,8 @@ declare global { __simAgentNextElementId?: number /** Elements an earlier snapshot of this document listed; a later snapshot marks the rest `new`. */ __simAgentShownElements?: WeakSet + /** Installed by {@link installPageHelpers} before every page function; cached per call. */ + __simAgentFindExemptModal: () => Element | null /** Why the last __simAgentResolveElement call returned null — read by the * shared stale-error producers so a refusal names its cause instead of * the blanket "the page changed". Cleared on every successful resolve. */ @@ -50,6 +54,68 @@ declare global { } } +/** The expression that runs a page function in the page, with the shared helpers installed first. */ +export function serializePageCall(fn: (...args: never[]) => unknown, args: unknown[]): string { + return `((${String(installPageHelpers)})(), (${String(fn)}).apply(null, ${JSON.stringify(args)}))` +} + +/** + * Installs the helpers page functions share. Page functions are serialized and run standalone, + * so the driver runs this installer before each one instead of inlining shared code into every + * function. + */ +export function installPageHelpers(): void { + /** + * The open modal the page aria-hid together with everything else. MUI's ModalManager + * aria-hides every child except the modal's mount node, and a `disablePortal` modal + * mounts inside the app root it just hid, so its own ancestor carries aria-hidden. Visibility + * checks skip only the aria-hidden test on ancestors above this modal. Null when any modal is + * visible unmodified (the portaled case) or no single topmost modal is contained by all of its + * hidden ancestors. + */ + let exemptModal: Element | null | undefined + window.__simAgentFindExemptModal = (): Element | null => { + if (exemptModal === undefined) exemptModal = findExemptModal() + return exemptModal + } + function findExemptModal(): Element | null { + const rendered: Array<{ modal: Element; hidden: Element[] }> = [] + for (const modal of Array.from( + document.querySelectorAll('[aria-modal="true"], dialog[open]') + )) { + const rect = modal.getBoundingClientRect() + if (rect.width <= 0 || rect.height <= 0 || modal.getAttribute('aria-hidden') === 'true') + continue + const hidden: Element[] = [] + let visible = true + for (let current: Element | null = modal; current && visible; ) { + const style = current.ownerDocument.defaultView?.getComputedStyle(current) + const opacity = Number.parseFloat(style?.opacity || '1') + visible = Boolean( + style && + style.display !== 'none' && + style.visibility !== 'hidden' && + style.contentVisibility !== 'hidden' && + (!Number.isFinite(opacity) || opacity > 0.01) && + !current.hasAttribute('hidden') + ) + if (current.getAttribute('aria-hidden') === 'true') hidden.push(current) + if (current.parentElement) current = current.parentElement + else { + const root = current.getRootNode() + current = 'host' in root ? (root.host as Element) : null + } + } + if (visible) rendered.push({ modal, hidden }) + } + if (rendered.some(({ hidden }) => hidden.length === 0)) return null + const topmost = rendered.filter(({ hidden }) => + hidden.every((ancestor) => rendered.every(({ modal }) => ancestor.contains(modal))) + ) + return topmost.length === 1 ? topmost[0].modal : null + } +} + /** * Builds the page snapshot: a structural outline (headings, landmarks) with * interactive elements carrying numeric ids, walking open shadow roots and @@ -165,7 +231,7 @@ export function collectSnapshot(startingElementId = 0, elementId?: number): unkn const previouslyShown = window.__simAgentShownElements const shown = previouslyShown ?? new WeakSet() window.__simAgentShownElements = shown - /** Whether an element appeared since the previous snapshot; the first snapshot marks nothing. */ + /** Whether no earlier snapshot of this document listed the element; the first snapshot marks nothing. */ const isNew = (el: Element): boolean => previouslyShown !== undefined && !shown.has(el) const lines: string[] = [] let truncated = false @@ -186,56 +252,6 @@ export function collectSnapshot(startingElementId = 0, elementId?: number): unkn const visibility = new WeakMap() - /** - * The open modal the page aria-hid together with everything else. MUI's - * ModalManager aria-hides every child except the modal's mount node, - * and a `disablePortal` modal mounts inside the app root it just hid, so its - * own ancestor carries aria-hidden. Ancestors ABOVE this modal skip only the - * aria-hidden test. Null when any modal is visible unmodified (the portaled - * case) or no single topmost modal is contained by all of its hidden - * ancestors. - */ - let exemptModal: Element | null | undefined - const getExemptModal = (): Element | null => { - if (exemptModal !== undefined) return exemptModal - exemptModal = null - const rendered: Array<{ modal: Element; hidden: Element[] }> = [] - for (const modal of Array.from( - document.querySelectorAll('[aria-modal="true"], dialog[open]') - )) { - const rect = modal.getBoundingClientRect() - if (rect.width <= 0 || rect.height <= 0 || modal.getAttribute('aria-hidden') === 'true') - continue - const hidden: Element[] = [] - let visible = true - for (let current: Element | null = modal; current && visible; ) { - const style = current.ownerDocument.defaultView?.getComputedStyle(current) - const opacity = Number.parseFloat(style?.opacity || '1') - visible = Boolean( - style && - style.display !== 'none' && - style.visibility !== 'hidden' && - style.contentVisibility !== 'hidden' && - (!Number.isFinite(opacity) || opacity > 0.01) && - !current.hasAttribute('hidden') - ) - if (current.getAttribute('aria-hidden') === 'true') hidden.push(current) - if (current.parentElement) current = current.parentElement - else { - const root = current.getRootNode() - current = 'host' in root ? (root.host as Element) : null - } - } - if (visible) rendered.push({ modal, hidden }) - } - if (rendered.some(({ hidden }) => hidden.length === 0)) return exemptModal - const topmost = rendered.filter(({ hidden }) => - hidden.every((ancestor) => rendered.every(({ modal }) => ancestor.contains(modal))) - ) - if (topmost.length === 1) exemptModal = topmost[0].modal - return exemptModal - } - const isVisible = (el: Element): boolean => { const cached = visibility.get(el) if (cached !== undefined) return cached @@ -265,7 +281,7 @@ export function collectSnapshot(startingElementId = 0, elementId?: number): unkn !current.hasAttribute('hidden') && (aboveExemptModal || current.getAttribute('aria-hidden') !== 'true') ) - if (current === getExemptModal()) aboveExemptModal = true + if (current === window.__simAgentFindExemptModal()) aboveExemptModal = true if (current.parentElement) current = current.parentElement else { const root = current.getRootNode() @@ -835,8 +851,6 @@ export function collectSnapshot(startingElementId = 0, elementId?: number): unkn // to zero size while mounting a replacement. Re-check live so a parked // node can fall through to the same strict, unique recovery used for a // detached node. - /** Re-derived live with the rest of this check, not reused from snapshot time. */ - exemptModal = undefined const isCurrentlyVisible = (candidate: Element): boolean => { const rect = candidate.getBoundingClientRect() if (rect.width <= 0 || rect.height <= 0) return false @@ -856,7 +870,7 @@ export function collectSnapshot(startingElementId = 0, elementId?: number): unkn ) { return false } - if (current === getExemptModal()) aboveExemptModal = true + if (current === window.__simAgentFindExemptModal()) aboveExemptModal = true if (current.parentElement) current = current.parentElement else { const root = current.getRootNode() @@ -1076,48 +1090,6 @@ export function clickElement( if (!el.isConnected) return { error: 'stale', reason: window.__simAgentStaleReason } } - /** The aria-hidden modal exemption documented on collectSnapshot, inlined for serialization. */ - let exemptModal: Element | null | undefined - const getExemptModal = (): Element | null => { - if (exemptModal !== undefined) return exemptModal - exemptModal = null - const rendered: Array<{ modal: Element; hidden: Element[] }> = [] - for (const modal of Array.from( - document.querySelectorAll('[aria-modal="true"], dialog[open]') - )) { - const rect = modal.getBoundingClientRect() - if (rect.width <= 0 || rect.height <= 0 || modal.getAttribute('aria-hidden') === 'true') - continue - const hidden: Element[] = [] - let visible = true - for (let current: Element | null = modal; current && visible; ) { - const style = current.ownerDocument.defaultView?.getComputedStyle(current) - const opacity = Number.parseFloat(style?.opacity || '1') - visible = Boolean( - style && - style.display !== 'none' && - style.visibility !== 'hidden' && - style.contentVisibility !== 'hidden' && - (!Number.isFinite(opacity) || opacity > 0.01) && - !current.hasAttribute('hidden') - ) - if (current.getAttribute('aria-hidden') === 'true') hidden.push(current) - if (current.parentElement) current = current.parentElement - else { - const root = current.getRootNode() - current = 'host' in root ? (root.host as Element) : null - } - } - if (visible) rendered.push({ modal, hidden }) - } - if (rendered.some(({ hidden }) => hidden.length === 0)) return exemptModal - const topmost = rendered.filter(({ hidden }) => - hidden.every((ancestor) => rendered.every(({ modal }) => ancestor.contains(modal))) - ) - if (topmost.length === 1) exemptModal = topmost[0].modal - return exemptModal - } - const view = el.ownerDocument.defaultView if (!view) return { error: 'stale', reason: window.__simAgentStaleReason } let aboveExemptModal = false @@ -1136,7 +1108,7 @@ export function clickElement( ) { return { error: 'not-visible' } } - if (current === getExemptModal()) aboveExemptModal = true + if (current === window.__simAgentFindExemptModal()) aboveExemptModal = true if (current.parentElement) current = current.parentElement else { const root = current.getRootNode() @@ -1612,47 +1584,6 @@ export function focusElementForTyping(id: number, moveFocus = true): unknown { rect.bottom - rect.top > 1 ) if (rects.length === 0) return { error: 'not-visible' } - /** The aria-hidden modal exemption documented on collectSnapshot, inlined for serialization. */ - let exemptModal: Element | null | undefined - const getExemptModal = (): Element | null => { - if (exemptModal !== undefined) return exemptModal - exemptModal = null - const rendered: Array<{ modal: Element; hidden: Element[] }> = [] - for (const modal of Array.from( - document.querySelectorAll('[aria-modal="true"], dialog[open]') - )) { - const rect = modal.getBoundingClientRect() - if (rect.width <= 0 || rect.height <= 0 || modal.getAttribute('aria-hidden') === 'true') - continue - const hidden: Element[] = [] - let visible = true - for (let current: Element | null = modal; current && visible; ) { - const style = current.ownerDocument.defaultView?.getComputedStyle(current) - const opacity = Number.parseFloat(style?.opacity || '1') - visible = Boolean( - style && - style.display !== 'none' && - style.visibility !== 'hidden' && - style.contentVisibility !== 'hidden' && - (!Number.isFinite(opacity) || opacity > 0.01) && - !current.hasAttribute('hidden') - ) - if (current.getAttribute('aria-hidden') === 'true') hidden.push(current) - if (current.parentElement) current = current.parentElement - else { - const root = current.getRootNode() - current = 'host' in root ? (root.host as Element) : null - } - } - if (visible) rendered.push({ modal, hidden }) - } - if (rendered.some(({ hidden }) => hidden.length === 0)) return exemptModal - const topmost = rendered.filter(({ hidden }) => - hidden.every((ancestor) => rendered.every(({ modal }) => ancestor.contains(modal))) - ) - if (topmost.length === 1) exemptModal = topmost[0].modal - return exemptModal - } let aboveExemptModal = false for (let current: Element | null = editable; current; current = composedParent(current)) { @@ -1670,7 +1601,7 @@ export function focusElementForTyping(id: number, moveFocus = true): unknown { ) { return { error: 'not-visible' } } - if (current === getExemptModal()) aboveExemptModal = true + if (current === window.__simAgentFindExemptModal()) aboveExemptModal = true } if (moveFocus) { @@ -2385,47 +2316,6 @@ export function readPageActionState( const observedDocument = observedElement?.ownerDocument ?? registeredElement?.ownerDocument ?? document const observedWindow = observedDocument.defaultView ?? window - /** The aria-hidden modal exemption documented on collectSnapshot, inlined for serialization. */ - let exemptModal: Element | null | undefined - const getExemptModal = (): Element | null => { - if (exemptModal !== undefined) return exemptModal - exemptModal = null - const rendered: Array<{ modal: Element; hidden: Element[] }> = [] - for (const modal of Array.from( - document.querySelectorAll('[aria-modal="true"], dialog[open]') - )) { - const rect = modal.getBoundingClientRect() - if (rect.width <= 0 || rect.height <= 0 || modal.getAttribute('aria-hidden') === 'true') - continue - const hidden: Element[] = [] - let visible = true - for (let current: Element | null = modal; current && visible; ) { - const style = current.ownerDocument.defaultView?.getComputedStyle(current) - const opacity = Number.parseFloat(style?.opacity || '1') - visible = Boolean( - style && - style.display !== 'none' && - style.visibility !== 'hidden' && - style.contentVisibility !== 'hidden' && - (!Number.isFinite(opacity) || opacity > 0.01) && - !current.hasAttribute('hidden') - ) - if (current.getAttribute('aria-hidden') === 'true') hidden.push(current) - if (current.parentElement) current = current.parentElement - else { - const root = current.getRootNode() - current = 'host' in root ? (root.host as Element) : null - } - } - if (visible) rendered.push({ modal, hidden }) - } - if (rendered.some(({ hidden }) => hidden.length === 0)) return exemptModal - const topmost = rendered.filter(({ hidden }) => - hidden.every((ancestor) => rendered.every(({ modal }) => ancestor.contains(modal))) - ) - if (topmost.length === 1) exemptModal = topmost[0].modal - return exemptModal - } const isEffectivelyRendered = (element: Element): boolean => { const rect = element.getBoundingClientRect() @@ -2457,7 +2347,7 @@ export function readPageActionState( ) { return false } - if (current === getExemptModal()) aboveExemptModal = true + if (current === window.__simAgentFindExemptModal()) aboveExemptModal = true if (current.parentElement) current = current.parentElement else { const root = current.getRootNode() @@ -2622,7 +2512,7 @@ export function readPageActionState( ) { return false } - if (current === getExemptModal()) aboveExemptModal = true + if (current === window.__simAgentFindExemptModal()) aboveExemptModal = true if (current.parentElement) current = current.parentElement else { const root = current.getRootNode() @@ -2729,48 +2619,6 @@ export function scrollPage(direction: string, amount?: number, elementId?: numbe const delta = towardStart ? -distance : distance const scrollingElement = (document.scrollingElement || document.documentElement) as HTMLElement - /** The aria-hidden modal exemption documented on collectSnapshot, inlined for serialization. */ - let exemptModal: Element | null | undefined - const getExemptModal = (): Element | null => { - if (exemptModal !== undefined) return exemptModal - exemptModal = null - const rendered: Array<{ modal: Element; hidden: Element[] }> = [] - for (const modal of Array.from( - document.querySelectorAll('[aria-modal="true"], dialog[open]') - )) { - const rect = modal.getBoundingClientRect() - if (rect.width <= 0 || rect.height <= 0 || modal.getAttribute('aria-hidden') === 'true') - continue - const hidden: Element[] = [] - let visible = true - for (let current: Element | null = modal; current && visible; ) { - const style = current.ownerDocument.defaultView?.getComputedStyle(current) - const opacity = Number.parseFloat(style?.opacity || '1') - visible = Boolean( - style && - style.display !== 'none' && - style.visibility !== 'hidden' && - style.contentVisibility !== 'hidden' && - (!Number.isFinite(opacity) || opacity > 0.01) && - !current.hasAttribute('hidden') - ) - if (current.getAttribute('aria-hidden') === 'true') hidden.push(current) - if (current.parentElement) current = current.parentElement - else { - const root = current.getRootNode() - current = 'host' in root ? (root.host as Element) : null - } - } - if (visible) rendered.push({ modal, hidden }) - } - if (rendered.some(({ hidden }) => hidden.length === 0)) return exemptModal - const topmost = rendered.filter(({ hidden }) => - hidden.every((ancestor) => rendered.every(({ modal }) => ancestor.contains(modal))) - ) - if (topmost.length === 1) exemptModal = topmost[0].modal - return exemptModal - } - const isVisible = (element: Element): boolean => { const rect = element.getBoundingClientRect() const view = element.ownerDocument.defaultView @@ -2799,7 +2647,7 @@ export function scrollPage(direction: string, amount?: number, elementId?: numbe ) { return false } - if (current === getExemptModal()) aboveExemptModal = true + if (current === window.__simAgentFindExemptModal()) aboveExemptModal = true if (current.parentElement) current = current.parentElement else { const root = current.getRootNode() @@ -3266,47 +3114,6 @@ export function getElementScreenshotRect(id: number): unknown { const rect = element.getBoundingClientRect() const view = element.ownerDocument.defaultView if (!view) return { error: 'stale', reason: window.__simAgentStaleReason } - /** The aria-hidden modal exemption documented on collectSnapshot, inlined for serialization. */ - let exemptModal: Element | null | undefined - const getExemptModal = (): Element | null => { - if (exemptModal !== undefined) return exemptModal - exemptModal = null - const rendered: Array<{ modal: Element; hidden: Element[] }> = [] - for (const modal of Array.from( - document.querySelectorAll('[aria-modal="true"], dialog[open]') - )) { - const rect = modal.getBoundingClientRect() - if (rect.width <= 0 || rect.height <= 0 || modal.getAttribute('aria-hidden') === 'true') - continue - const hidden: Element[] = [] - let visible = true - for (let current: Element | null = modal; current && visible; ) { - const style = current.ownerDocument.defaultView?.getComputedStyle(current) - const opacity = Number.parseFloat(style?.opacity || '1') - visible = Boolean( - style && - style.display !== 'none' && - style.visibility !== 'hidden' && - style.contentVisibility !== 'hidden' && - (!Number.isFinite(opacity) || opacity > 0.01) && - !current.hasAttribute('hidden') - ) - if (current.getAttribute('aria-hidden') === 'true') hidden.push(current) - if (current.parentElement) current = current.parentElement - else { - const root = current.getRootNode() - current = 'host' in root ? (root.host as Element) : null - } - } - if (visible) rendered.push({ modal, hidden }) - } - if (rendered.some(({ hidden }) => hidden.length === 0)) return exemptModal - const topmost = rendered.filter(({ hidden }) => - hidden.every((ancestor) => rendered.every(({ modal }) => ancestor.contains(modal))) - ) - if (topmost.length === 1) exemptModal = topmost[0].modal - return exemptModal - } let aboveExemptModal = false for (let current: Element | null = element; current; ) { @@ -3324,7 +3131,7 @@ export function getElementScreenshotRect(id: number): unknown { ) { return { error: 'not-visible' } } - if (current === getExemptModal()) aboveExemptModal = true + if (current === window.__simAgentFindExemptModal()) aboveExemptModal = true if (current.parentElement) current = current.parentElement else { const root = current.getRootNode() @@ -3460,48 +3267,6 @@ export function readChildFrameElementState( rect.top < view.innerHeight ) - /** The aria-hidden modal exemption documented on collectSnapshot, inlined for serialization. */ - let exemptModal: Element | null | undefined - const getExemptModal = (): Element | null => { - if (exemptModal !== undefined) return exemptModal - exemptModal = null - const rendered: Array<{ modal: Element; hidden: Element[] }> = [] - for (const modal of Array.from( - document.querySelectorAll('[aria-modal="true"], dialog[open]') - )) { - const rect = modal.getBoundingClientRect() - if (rect.width <= 0 || rect.height <= 0 || modal.getAttribute('aria-hidden') === 'true') - continue - const hidden: Element[] = [] - let visible = true - for (let current: Element | null = modal; current && visible; ) { - const style = current.ownerDocument.defaultView?.getComputedStyle(current) - const opacity = Number.parseFloat(style?.opacity || '1') - visible = Boolean( - style && - style.display !== 'none' && - style.visibility !== 'hidden' && - style.contentVisibility !== 'hidden' && - (!Number.isFinite(opacity) || opacity > 0.01) && - !current.hasAttribute('hidden') - ) - if (current.getAttribute('aria-hidden') === 'true') hidden.push(current) - if (current.parentElement) current = current.parentElement - else { - const root = current.getRootNode() - current = 'host' in root ? (root.host as Element) : null - } - } - if (visible) rendered.push({ modal, hidden }) - } - if (rendered.some(({ hidden }) => hidden.length === 0)) return exemptModal - const topmost = rendered.filter(({ hidden }) => - hidden.every((ancestor) => rendered.every(({ modal }) => ancestor.contains(modal))) - ) - if (topmost.length === 1) exemptModal = topmost[0].modal - return exemptModal - } - let pointMappingReliable = true let aboveExemptModal = false for (let current: Element | null = element; visible && current; ) { @@ -3517,7 +3282,7 @@ export function readChildFrameElementState( visible = false break } - if (current === getExemptModal()) aboveExemptModal = true + if (current === window.__simAgentFindExemptModal()) aboveExemptModal = true if (style.transform && style.transform !== 'none') { try { if (typeof DOMMatrixReadOnly !== 'function') { diff --git a/apps/desktop/src/main/browser-agent/session.test.ts b/apps/desktop/src/main/browser-agent/session.test.ts index 98aefd050fe..bffaa822a87 100644 --- a/apps/desktop/src/main/browser-agent/session.test.ts +++ b/apps/desktop/src/main/browser-agent/session.test.ts @@ -4017,11 +4017,9 @@ describe('browser-agent session', () => { .mockReturnValueOnce(firstProbe.promise) .mockReturnValueOnce(secondProbe.promise) .mockReturnValue(Number.MAX_SAFE_INTEGER) - const pathExists = vi.fn((_path: string) => false) session = freshSession(win, {}, persistence, { getDirectory: () => directory, getFreeDiskBytes, - pathExists, }) const contents = (session.ensureTab().view as unknown as MockView).webContents const first = mockDownloadItem({ filename: 'same-name.bin', totalBytes: 100 }) @@ -4030,11 +4028,6 @@ describe('browser-agent session', () => { startMockDownload(contents, first) startMockDownload(contents, second) await vi.waitFor(() => expect(getFreeDiskBytes).toHaveBeenCalledTimes(2)) - expect(pathExists.mock.calls.map(([candidate]) => candidate)).toEqual([ - join(directory, 'same-name.bin'), - join(directory, 'same-name.bin'), - expect.stringMatching(/same-name \(.+\)\.bin$/), - ]) await session.clearProfileStorage() @@ -4071,7 +4064,9 @@ describe('browser-agent session', () => { replacement.emitDone('completed') await vi.waitFor(() => expect(finishedDownloadFiles(directory)).toHaveLength(2)) expect(finishedDownloadFiles(directory)).toContain('same-name.bin') - await vi.waitFor(() => expect(readdirSync(directory)).toHaveLength(2)) + await vi.waitFor(() => + expect(readdirSync(directory).filter((name) => name.startsWith('.'))).toEqual([]) + ) }) it('does not reserve a late filename after profile teardown starts', async () => { diff --git a/apps/desktop/src/main/browser-agent/session.ts b/apps/desktop/src/main/browser-agent/session.ts index b27d57579ea..2e6a3c06f90 100644 --- a/apps/desktop/src/main/browser-agent/session.ts +++ b/apps/desktop/src/main/browser-agent/session.ts @@ -425,14 +425,16 @@ interface ActiveBrowserDownload { lastDiskCheckAt: number /** * Electron's download delegate opens a native Save dialog unless a path is - * set before `will-download` returns, so bytes land in this random hidden - * file and move to the asynchronously allocated `savePath` on completion. + * set before `will-download` returns, so bytes land in this randomly named + * dot-file (hidden on POSIX) and move to the asynchronously allocated + * `savePath` on completion. */ stagingPath: string /** The reserved final destination, once allocation has chosen one. */ savePath?: string /** Settles with the final destination, or null when allocation failed. */ destination: Promise + /** Set once Electron reports the item done, so a late disk check never resumes or cancels it. */ finished: boolean scopeId: string terminal: boolean @@ -1554,7 +1556,7 @@ function configureBrowserDownloads(ses: Session): void { const download = createTrackedBrowserDownload(item, 'progressing') const { filename } = download - const rejectBeforeTracking = (reason: string, message: string, error: unknown) => { + const failDownloadSetup = (reason: string, message: string, error: unknown) => { download.interruptionReason = reason download.state = 'interrupted' try { @@ -1573,7 +1575,7 @@ function configureBrowserDownloads(ses: Session): void { try { item.setSavePath(stagingPath) } catch (error) { - rejectBeforeTracking( + failDownloadSetup( 'Stopped: the download destination could not be prepared safely', 'Could not set the staging destination for an agent browser download', error @@ -1583,7 +1585,7 @@ function configureBrowserDownloads(ses: Session): void { try { item.pause() } catch (error) { - rejectBeforeTracking( + failDownloadSetup( 'Stopped: the download could not be paused for a disk-space safety check', 'Agent browser download could not be paused for admission', error @@ -1597,6 +1599,7 @@ function configureBrowserDownloads(ses: Session): void { diskCheckInFlight: false, lastDiskCheckAt: 0, stagingPath, + /** Replaced below by the allocation, whose callbacks need this record to exist first. */ destination: Promise.resolve(null), finished: false, scopeId, @@ -1632,13 +1635,10 @@ function configureBrowserDownloads(ses: Session): void { active.finished = true updateDownloadProgress(download, item) if (state !== 'completed' || active.limitReason || active.terminal) { - const tornDown = active.terminal releaseActiveBrowserDownload(active) discardStagedBrowserDownload(active) download.savePath = active.savePath - if (active.limitReason) download.state = 'interrupted' - else if (tornDown && state === 'completed') download.state = 'cancelled' - else download.state = state + download.state = active.limitReason ? 'interrupted' : state finishBrowserDownload(active) return } From 90f0319b5ff42d3c2283a57f9eebe9b38259d088 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 24 Sep 2026 18:01:32 -0700 Subject: [PATCH 07/10] fix(browser): close the edge cases review found in the desktop agent changes - A held right-click renews its agent context-menu marker before release, so Windows' release-time menu stays suppressed after a hold over one second. - A failed chord releases only the keys whose press was attempted. - A disk probe that resolves after a download completed can no longer cancel it. - A disablePortal modal nested inside another open modal is picked as topmost. - holdMs is an integer bounded to 0-10000 in the tool contract. --- .../src/main/browser-agent/cdp.test.ts | 23 +++++++++++ apps/desktop/src/main/browser-agent/cdp.ts | 9 ++++- .../src/main/browser-agent/keyboard.test.ts | 18 +++++++++ .../src/main/browser-agent/keyboard.ts | 25 ++++++++---- .../main/browser-agent/page-functions.test.ts | 15 ++++++++ .../src/main/browser-agent/page-functions.ts | 8 ++-- .../src/main/browser-agent/session.test.ts | 38 +++++++++++++++++++ .../desktop/src/main/browser-agent/session.ts | 16 ++++++-- .../mothership/generated/tool-catalog-v1.ts | 8 +++- .../mothership/generated/tool-schemas-v1.ts | 8 +++- 10 files changed, 148 insertions(+), 20 deletions(-) diff --git a/apps/desktop/src/main/browser-agent/cdp.test.ts b/apps/desktop/src/main/browser-agent/cdp.test.ts index 44c05485683..852e12dd3c0 100644 --- a/apps/desktop/src/main/browser-agent/cdp.test.ts +++ b/apps/desktop/src/main/browser-agent/cdp.test.ts @@ -14,6 +14,7 @@ import { import { captureScreenshot, clickAt, + consumeAgentContextMenu, ensureInstrumented, evaluateInIsolatedFrame, insertText, @@ -275,6 +276,28 @@ describe('browser-agent CDP instrumentation', () => { } }) + it('keeps a held right-click marked as the agent context menu until release', async () => { + const contents = new WebContentsView().webContents + vi.useFakeTimers() + try { + const rightHold = { ...PRIMARY_CLICK, button: 'right' as const, holdMs: 1500 } + await Promise.all([ + clickAt(contents, 5, 6, false, rightHold), + vi.advanceTimersByTimeAsync(1500), + ]) + expect(consumeAgentContextMenu(contents)).toBe(true) + + const click = clickAt(contents, 5, 6, false, rightHold) + await vi.advanceTimersByTimeAsync(0) + expect(consumeAgentContextMenu(contents)).toBe(true) + await vi.advanceTimersByTimeAsync(1500) + await click + expect(consumeAgentContextMenu(contents)).toBe(false) + } finally { + vi.useRealTimers() + } + }) + it('releases the mouse after a partial click failure', async () => { const contents = new WebContentsView().webContents vi.mocked(contents.debugger.sendCommand) diff --git a/apps/desktop/src/main/browser-agent/cdp.ts b/apps/desktop/src/main/browser-agent/cdp.ts index 7587e598e61..edfe4faf629 100644 --- a/apps/desktop/src/main/browser-agent/cdp.ts +++ b/apps/desktop/src/main/browser-agent/cdp.ts @@ -1012,7 +1012,14 @@ export async function clickAt( modifiers, clickCount: count, }) - if (holdMs > 0) await sleep(holdMs) + if (holdMs > 0) { + await sleep(holdMs) + // Windows opens the context menu on release, after the hold; renew a marker a + // press-time menu has not already consumed. + if (button === 'right' && agentContextClicks.has(contents)) { + agentContextClicks.set(contents, Date.now()) + } + } await sendInput(contents, 'Input.dispatchMouseEvent', { type: 'mouseReleased', x, diff --git a/apps/desktop/src/main/browser-agent/keyboard.test.ts b/apps/desktop/src/main/browser-agent/keyboard.test.ts index 61fc56b9972..8bb094c7302 100644 --- a/apps/desktop/src/main/browser-agent/keyboard.test.ts +++ b/apps/desktop/src/main/browser-agent/keyboard.test.ts @@ -1,3 +1,4 @@ +import { toRecord } from '@sim/utils/object' import { describe, expect, it, vi } from 'vitest' vi.mock('electron', () => import('@/test/electron-mock')) @@ -312,6 +313,23 @@ describe('dispatchKeyCombo', () => { ]) }) + it('releases only the chord keys whose press was attempted', async () => { + const contents = new WebContentsView().webContents + vi.mocked(contents.debugger.sendCommand) + .mockResolvedValueOnce({}) + .mockRejectedValueOnce(new Error('shift-down response lost')) + + await expect(dispatchKeyCombo(contents, parseKeyCombo('Ctrl+Shift+K'))).rejects.toMatchObject({ + name: KeyDispatchError.name, + keyDownDispatched: true, + }) + + const keys = vi + .mocked(contents.debugger.sendCommand) + .mock.calls.map(([, params]) => `${toRecord(params).type} ${toRecord(params).key}`) + expect(keys).toEqual(['rawKeyDown Control', 'rawKeyDown Shift', 'keyUp Shift', 'keyUp Control']) + }) + it('does not turn menu-restoration cleanup failure into a duplicate key retry signal', async () => { const contents = new WebContentsView().webContents vi.mocked(contents.setIgnoreMenuShortcuts).mockImplementation((ignored: boolean) => { diff --git a/apps/desktop/src/main/browser-agent/keyboard.ts b/apps/desktop/src/main/browser-agent/keyboard.ts index c718d7d0e75..7cf9c269eef 100644 --- a/apps/desktop/src/main/browser-agent/keyboard.ts +++ b/apps/desktop/src/main/browser-agent/keyboard.ts @@ -372,28 +372,37 @@ export async function dispatchKeyCombo(contents: WebContents, combo: ParsedCombo if (depth === 0) contents.setIgnoreMenuShortcuts(true) applicationMenuIsolationDepth.set(contents, depth + 1) } - let keyDownDispatched = false + const presses = [...modifierKeys.downs, down] + let pressesAttempted = 0 try { - // Mark before awaiting: Blink may receive the key-down and then lose the + // Count before awaiting: Blink may receive a key-down and then lose the // CDP acknowledgement during navigation/process swap. In that ambiguous // case cleanup is required and a synthetic retry could double-act. - keyDownDispatched = true - for (const event of modifierKeys.downs) await cdp.dispatchKeyEvent(contents, event) - await cdp.dispatchKeyEvent(contents, down) + for (const press of presses) { + pressesAttempted++ + await cdp.dispatchKeyEvent(contents, press) + } await cdp.dispatchKeyEvent(contents, up) for (const event of modifierKeys.ups) await cdp.dispatchKeyEvent(contents, event) } catch (error) { - if (keyDownDispatched && !contents.isDestroyed()) { + if (pressesAttempted > 0 && !contents.isDestroyed()) { // Like pointer cleanup, this is best effort. The original key-up may // have reached Blink before its CDP response was lost; a duplicate // release is harmless, while omitting it can leave input state stuck. - for (const release of [up, ...modifierKeys.ups]) { + // Only keys whose press was attempted are released: a key-up for a key + // that never went down can itself trigger a page's keyup handler. + const heldModifiers = Math.min(pressesAttempted, modifierKeys.downs.length) + const releases = [ + ...(pressesAttempted === presses.length ? [up] : []), + ...modifierKeys.ups.slice(modifierKeys.ups.length - heldModifiers), + ] + for (const release of releases) { await cdp.dispatchKeyEvent(contents, release).catch(() => {}) } } throw new KeyDispatchError( getErrorMessage(error, 'Trusted key dispatch failed'), - keyDownDispatched + pressesAttempted > 0 ) } finally { if (isolatesApplicationMenu) { diff --git a/apps/desktop/src/main/browser-agent/page-functions.test.ts b/apps/desktop/src/main/browser-agent/page-functions.test.ts index cc93ff2aa7e..684a88b3f4f 100644 --- a/apps/desktop/src/main/browser-agent/page-functions.test.ts +++ b/apps/desktop/src/main/browser-agent/page-functions.test.ts @@ -2536,4 +2536,19 @@ describe('modal hidden together with its own app root', () => { expect(outline).not.toContain('Discard') expect((readPageActionState() as { dialogs: string[] }).dialogs).toEqual(['Upper']) }) + + it('exposes a disablePortal modal nested inside another open modal', () => { + document.body.innerHTML = ` + ` + showAll() + + const outline = outlineOf(collectSnapshot()) + + expect(outline).toContain('Confirm') + expect(outline).not.toContain('Discard') + }) }) diff --git a/apps/desktop/src/main/browser-agent/page-functions.ts b/apps/desktop/src/main/browser-agent/page-functions.ts index 50d3a5e8562..0dea8e91596 100644 --- a/apps/desktop/src/main/browser-agent/page-functions.ts +++ b/apps/desktop/src/main/browser-agent/page-functions.ts @@ -71,7 +71,7 @@ export function installPageHelpers(): void { * mounts inside the app root it just hid, so its own ancestor carries aria-hidden. Visibility * checks skip only the aria-hidden test on ancestors above this modal. Null when any modal is * visible unmodified (the portaled case) or no single topmost modal is contained by all of its - * hidden ancestors. + * hidden ancestors. A modal nested inside another open modal is the topmost of the two. */ let exemptModal: Element | null | undefined window.__simAgentFindExemptModal = (): Element | null => { @@ -109,8 +109,10 @@ export function installPageHelpers(): void { if (visible) rendered.push({ modal, hidden }) } if (rendered.some(({ hidden }) => hidden.length === 0)) return null - const topmost = rendered.filter(({ hidden }) => - hidden.every((ancestor) => rendered.every(({ modal }) => ancestor.contains(modal))) + const topmost = rendered.filter( + ({ modal, hidden }) => + hidden.every((ancestor) => rendered.every((other) => ancestor.contains(other.modal))) && + !rendered.some((other) => other.modal !== modal && modal.contains(other.modal)) ) return topmost.length === 1 ? topmost[0].modal : null } diff --git a/apps/desktop/src/main/browser-agent/session.test.ts b/apps/desktop/src/main/browser-agent/session.test.ts index bffaa822a87..70d590dca12 100644 --- a/apps/desktop/src/main/browser-agent/session.test.ts +++ b/apps/desktop/src/main/browser-agent/session.test.ts @@ -3495,6 +3495,44 @@ describe('browser-agent session', () => { }) }) + it('ignores a progress disk probe that resolves after the download completed', async () => { + vi.useFakeTimers({ toFake: ['Date'] }) + try { + const directory = mkdtempSync(join(tmpdir(), 'sim-browser-downloads-')) + const progressProbe = deferred() + const getFreeDiskBytes = vi + .fn<(directory: string) => number | Promise>() + .mockReturnValueOnce(Number.MAX_SAFE_INTEGER) + .mockReturnValueOnce(progressProbe.promise) + session = freshSession(win, {}, undefined, { + getDirectory: () => directory, + getFreeDiskBytes, + }) + const contents = (session.ensureTab().view as unknown as MockView).webContents + const download = mockDownloadItem({ filename: 'late-probe.bin', totalBytes: 100 }) + + startMockDownload(contents, download) + await vi.waitFor(() => expect(download.item.resume).toHaveBeenCalledOnce()) + vi.setSystemTime(Date.now() + 1_000) + download.emitUpdated() + expect(getFreeDiskBytes).toHaveBeenCalledTimes(2) + + download.emitDone('completed') + progressProbe.resolve(0) + + await vi.waitFor(() => + expect(session.getBrowserDownloadsState('chat-test').downloads[0]).toMatchObject({ + filename: 'late-probe.bin', + state: 'completed', + }) + ) + expect(download.item.cancel).not.toHaveBeenCalled() + expect(finishedDownloadFiles(directory)).toEqual(['late-probe.bin']) + } finally { + vi.useRealTimers() + } + }) + it('interrupts a completed download whose staging file cannot be moved into place', async () => { const directory = mkdtempSync(join(tmpdir(), 'sim-browser-downloads-')) session = freshSession(win, {}, undefined, { diff --git a/apps/desktop/src/main/browser-agent/session.ts b/apps/desktop/src/main/browser-agent/session.ts index 2e6a3c06f90..1b9790b4745 100644 --- a/apps/desktop/src/main/browser-agent/session.ts +++ b/apps/desktop/src/main/browser-agent/session.ts @@ -647,6 +647,16 @@ function publishActiveBrowserDownload(active: ActiveBrowserDownload): void { publishBrowserDownloads(liveScopeId) } +/** A disk probe that resolves after its download finished or stopped must not cancel it. */ +function isDiskCheckStale(active: ActiveBrowserDownload): boolean { + return ( + active.terminal || + active.finished || + Boolean(active.limitReason) || + !activeBrowserDownloads.has(active) + ) +} + function checkBrowserDownloadDiskSpace( active: ActiveBrowserDownload, check: 'admission' | 'progress', @@ -664,9 +674,7 @@ function checkBrowserDownloadDiskSpace( active.diskCheckInFlight = true void browserDownloadFreeDiskBytes(active.directory) .then((freeDiskBytes) => { - if (active.terminal || active.limitReason || !activeBrowserDownloads.has(active)) { - return - } + if (isDiskCheckStale(active)) return const requiredFreeDiskBytes = MIN_BROWSER_DOWNLOAD_FREE_DISK_BYTES + activeDownloadReservations(check === 'admission' ? active : undefined) @@ -683,7 +691,7 @@ function checkBrowserDownloadDiskSpace( if (check === 'admission' && active.download.state === 'progressing') active.item.resume() }) .catch((error) => { - if (active.terminal || active.limitReason || !activeBrowserDownloads.has(active)) return + if (isDiskCheckStale(active)) return logger.warn('Could not complete an agent browser download disk-space check', { error: getErrorMessage(error), }) diff --git a/apps/sim/lib/mothership/generated/tool-catalog-v1.ts b/apps/sim/lib/mothership/generated/tool-catalog-v1.ts index 8402223f29c..d0c39e2dec4 100644 --- a/apps/sim/lib/mothership/generated/tool-catalog-v1.ts +++ b/apps/sim/lib/mothership/generated/tool-catalog-v1.ts @@ -530,9 +530,11 @@ export const BrowserClick: ToolCatalogEntry = { "The element id to act on (from the current tab's most recent browser_snapshot). Treat refs as invalid across tab switches or later snapshots.", }, holdMs: { - type: 'number', + type: 'integer', description: 'Keep the button pressed this many milliseconds before releasing (0 to 10000, default 0), for press-and-hold controls. Single clicks only.', + minimum: 0, + maximum: 10000, }, modifiers: { type: 'array', @@ -673,9 +675,11 @@ export const BrowserClickAt: ToolCatalogEntry = { required: ['accept'], }, holdMs: { - type: 'number', + type: 'integer', description: 'Keep the button pressed this many milliseconds before releasing (0 to 10000, default 0), for press-and-hold controls. Single clicks only.', + minimum: 0, + maximum: 10000, }, modifiers: { type: 'array', diff --git a/apps/sim/lib/mothership/generated/tool-schemas-v1.ts b/apps/sim/lib/mothership/generated/tool-schemas-v1.ts index a7cd19a9349..5177f5e8341 100644 --- a/apps/sim/lib/mothership/generated/tool-schemas-v1.ts +++ b/apps/sim/lib/mothership/generated/tool-schemas-v1.ts @@ -219,9 +219,11 @@ export const TOOL_RUNTIME_SCHEMAS: Record = { "The element id to act on (from the current tab's most recent browser_snapshot). Treat refs as invalid across tab switches or later snapshots.", }, holdMs: { - type: 'number', + type: 'integer', description: 'Keep the button pressed this many milliseconds before releasing (0 to 10000, default 0), for press-and-hold controls. Single clicks only.', + minimum: 0, + maximum: 10000, }, modifiers: { type: 'array', @@ -396,9 +398,11 @@ export const TOOL_RUNTIME_SCHEMAS: Record = { required: ['accept'], }, holdMs: { - type: 'number', + type: 'integer', description: 'Keep the button pressed this many milliseconds before releasing (0 to 10000, default 0), for press-and-hold controls. Single clicks only.', + minimum: 0, + maximum: 10000, }, modifiers: { type: 'array', From 67e1326ab2dbecd5c5844a3745cdd2b67e3d2e70 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 24 Sep 2026 18:20:46 -0700 Subject: [PATCH 08/10] fix(browser): harden holds, modal exemption, and download moves - An aborted press-and-hold rejects and releases the button at once instead of staying held into the next action. - The disablePortal exemption applies only when a child hides the modal (MUI's mechanism), so a dialog the app hid itself stays hidden, and it is looked up per document so same-origin iframes get it too. - A completed download's move retries transient EBUSY/EPERM/EACCES errors, as Chromium's own final rename does, instead of discarding the file. - Keyboard docs describe the separate modifier presses; the partial-release test covers a failure at the first modifier. --- .../src/main/browser-agent/cdp.test.ts | 20 +++++++ apps/desktop/src/main/browser-agent/cdp.ts | 13 +++-- apps/desktop/src/main/browser-agent/driver.ts | 6 +-- .../src/main/browser-agent/keyboard.test.ts | 11 ++++ .../src/main/browser-agent/keyboard.ts | 10 ++-- .../main/browser-agent/page-functions.test.ts | 33 ++++++++++++ .../src/main/browser-agent/page-functions.ts | 52 +++++++++++-------- .../src/main/browser-agent/session.test.ts | 39 +++++++++++++- .../desktop/src/main/browser-agent/session.ts | 41 +++++++++++---- 9 files changed, 182 insertions(+), 43 deletions(-) diff --git a/apps/desktop/src/main/browser-agent/cdp.test.ts b/apps/desktop/src/main/browser-agent/cdp.test.ts index 852e12dd3c0..5aaf5cd74fd 100644 --- a/apps/desktop/src/main/browser-agent/cdp.test.ts +++ b/apps/desktop/src/main/browser-agent/cdp.test.ts @@ -276,6 +276,26 @@ describe('browser-agent CDP instrumentation', () => { } }) + it('releases a held button as soon as its click is aborted', async () => { + const contents = new WebContentsView().webContents + const types = () => + vi.mocked(contents.debugger.sendCommand).mock.calls.map(([, params]) => toRecord(params).type) + vi.useFakeTimers() + try { + const controller = new AbortController() + const hold = { ...PRIMARY_CLICK, holdMs: 10_000 } + const click = clickAt(contents, 5, 6, false, hold, controller.signal) + await vi.advanceTimersByTimeAsync(100) + expect(types()).toEqual(['mousePressed']) + + controller.abort() + await expect(click).rejects.toMatchObject({ name: 'AbortError' }) + expect(types()).toEqual(['mousePressed', 'mouseReleased']) + } finally { + vi.useRealTimers() + } + }) + it('keeps a held right-click marked as the agent context menu until release', async () => { const contents = new WebContentsView().webContents vi.useFakeTimers() diff --git a/apps/desktop/src/main/browser-agent/cdp.ts b/apps/desktop/src/main/browser-agent/cdp.ts index edfe4faf629..deea9bb17e8 100644 --- a/apps/desktop/src/main/browser-agent/cdp.ts +++ b/apps/desktop/src/main/browser-agent/cdp.ts @@ -11,7 +11,7 @@ import type { BrowserTheme } from '@sim/browser-protocol' import { createLogger } from '@sim/logger' import { getErrorMessage } from '@sim/utils/errors' -import { sleep } from '@sim/utils/helpers' +import { interruptibleSleep, sleep } from '@sim/utils/helpers' import { isRecordLike } from '@sim/utils/object' import type { NativeImage, WebContents, WebFrameMain } from 'electron' @@ -983,12 +983,18 @@ export function clearAgentContextMenu(contents: WebContents): void { agentContextClicks.delete(contents) } +/** + * Clicks at viewport coordinates. `signal` ends a press-and-hold early: the click rejects with the + * abort reason and the button is released at once, so a cancelled or timed-out click cannot stay + * held into the next action. That release can still activate the control under the pointer. + */ export async function clickAt( contents: WebContents, x: number, y: number, moveBeforePress = true, - click: PointerClick = PRIMARY_CLICK + click: PointerClick = PRIMARY_CLICK, + signal?: AbortSignal ): Promise { if (moveBeforePress) await moveMouse(contents, x, y) const { button, clickCount, modifiers, holdMs } = click @@ -1013,12 +1019,13 @@ export async function clickAt( clickCount: count, }) if (holdMs > 0) { - await sleep(holdMs) + await interruptibleSleep(holdMs, signal) // Windows opens the context menu on release, after the hold; renew a marker a // press-time menu has not already consumed. if (button === 'right' && agentContextClicks.has(contents)) { agentContextClicks.set(contents, Date.now()) } + signal?.throwIfAborted() } await sendInput(contents, 'Input.dispatchMouseEvent', { type: 'mouseReleased', diff --git a/apps/desktop/src/main/browser-agent/driver.ts b/apps/desktop/src/main/browser-agent/driver.ts index 30d86866487..1a1c699b6d2 100644 --- a/apps/desktop/src/main/browser-agent/driver.ts +++ b/apps/desktop/src/main/browser-agent/driver.ts @@ -3223,7 +3223,7 @@ async function executeToolInner( try { assertCurrentExecution() assertElementActionCurrent(contents, elementId, target) - await cdp.clickAt(contents, x, y, false, click) + await cdp.clickAt(contents, x, y, false, click, signal) trusted = true activation = 'native-pointer' } catch (error) { @@ -3263,7 +3263,7 @@ async function executeToolInner( try { assertCurrentExecution() assertElementActionCurrent(contents, elementId, target) - await cdp.clickAt(contents, finalTopPoint.x, finalTopPoint.y, false, click) + await cdp.clickAt(contents, finalTopPoint.x, finalTopPoint.y, false, click, signal) trusted = true activation = 'native-pointer' prepared = finalSurface @@ -4642,7 +4642,7 @@ async function executeToolInner( assertCurrentExecution() assertActiveContents(contents, clickNavigationEpoch) try { - await cdp.clickAt(contents, x, y, true, click) + await cdp.clickAt(contents, x, y, true, click, signal) } catch (error) { const rescued = navigationRescue(contents, clickNavigationEpoch, urlAtDispatch, { trusted: true, diff --git a/apps/desktop/src/main/browser-agent/keyboard.test.ts b/apps/desktop/src/main/browser-agent/keyboard.test.ts index 8bb094c7302..e71a9514b17 100644 --- a/apps/desktop/src/main/browser-agent/keyboard.test.ts +++ b/apps/desktop/src/main/browser-agent/keyboard.test.ts @@ -328,6 +328,17 @@ describe('dispatchKeyCombo', () => { .mocked(contents.debugger.sendCommand) .mock.calls.map(([, params]) => `${toRecord(params).type} ${toRecord(params).key}`) expect(keys).toEqual(['rawKeyDown Control', 'rawKeyDown Shift', 'keyUp Shift', 'keyUp Control']) + + const early = new WebContentsView().webContents + vi.mocked(early.debugger.sendCommand).mockRejectedValueOnce(new Error('control-down lost')) + await expect(dispatchKeyCombo(early, parseKeyCombo('Ctrl+Shift+K'))).rejects.toMatchObject({ + name: KeyDispatchError.name, + }) + expect( + vi + .mocked(early.debugger.sendCommand) + .mock.calls.map(([, params]) => `${toRecord(params).type} ${toRecord(params).key}`) + ).toEqual(['rawKeyDown Control', 'keyUp Control']) }) it('does not turn menu-restoration cleanup failure into a duplicate key retry signal', async () => { diff --git a/apps/desktop/src/main/browser-agent/keyboard.ts b/apps/desktop/src/main/browser-agent/keyboard.ts index 7cf9c269eef..ed755763b81 100644 --- a/apps/desktop/src/main/browser-agent/keyboard.ts +++ b/apps/desktop/src/main/browser-agent/keyboard.ts @@ -1,7 +1,8 @@ /** * Keyboard machinery for `browser_press_key` and internal key dispatch: - * parsing "Cmd+Shift+Z"-style combos and building the trusted CDP - * keyDown/keyUp pair. Pure logic except {@link dispatchKeyCombo}. + * parsing "Cmd+Shift+Z"-style combos and building the trusted CDP key events — + * each modifier's own press and release around the main key's keyDown/keyUp + * pair. Pure logic except {@link dispatchKeyCombo}. */ import { getErrorMessage } from '@sim/utils/errors' import type { WebContents } from 'electron' @@ -360,8 +361,9 @@ export function modifierKeyEvents( * Electron normally lets modified key events escape a focused WebContents to * application-menu accelerators. Agent input must stay inside the browser — a * page-level Cmd shortcut must never reload, close, or open a native window in - * Sim — so menu handling is suspended for the complete CDP down/up pair. The - * depth counter keeps overlapping tool calls from re-enabling it too early. + * Sim — so menu handling is suspended for the whole chord, modifier presses + * included. The depth counter keeps overlapping tool calls from re-enabling it + * too early. */ export async function dispatchKeyCombo(contents: WebContents, combo: ParsedCombo): Promise { const [down, up] = buildKeyDispatchPlan(combo) diff --git a/apps/desktop/src/main/browser-agent/page-functions.test.ts b/apps/desktop/src/main/browser-agent/page-functions.test.ts index 684a88b3f4f..817093cf526 100644 --- a/apps/desktop/src/main/browser-agent/page-functions.test.ts +++ b/apps/desktop/src/main/browser-agent/page-functions.test.ts @@ -2537,6 +2537,39 @@ describe('modal hidden together with its own app root', () => { expect((readPageActionState() as { dialogs: string[] }).dialogs).toEqual(['Upper']) }) + it('keeps a dialog the app aria-hid below its root hidden', () => { + document.body.innerHTML = ` +
+ + +
` + showAll() + + const outline = outlineOf(collectSnapshot()) + + expect(outline).toContain('Shown action') + expect(outline).not.toContain('Ghost') + }) + + it('reads a disablePortal dialog inside a same-origin iframe', () => { + const frame = visible(document.createElement('iframe')) + document.body.append(frame) + const inner = frame.contentDocument as Document + inner.body.innerHTML = ` + ` + for (const element of Array.from(inner.body.querySelectorAll('*'))) visible(element) + + const outline = outlineOf(collectSnapshot()) + + expect(outline).toContain('Framed send') + expect(outline).not.toContain('Framed compose') + }) + it('exposes a disablePortal modal nested inside another open modal', () => { document.body.innerHTML = `