Skip to content

Commit 67e1326

Browse files
committed
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 <body> 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.
1 parent 90f0319 commit 67e1326

9 files changed

Lines changed: 182 additions & 43 deletions

File tree

‎apps/desktop/src/main/browser-agent/cdp.test.ts‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -276,6 +276,26 @@ describe('browser-agent CDP instrumentation', () => {
276276
}
277277
})
278278

279+
it('releases a held button as soon as its click is aborted', async () => {
280+
const contents = new WebContentsView().webContents
281+
const types = () =>
282+
vi.mocked(contents.debugger.sendCommand).mock.calls.map(([, params]) => toRecord(params).type)
283+
vi.useFakeTimers()
284+
try {
285+
const controller = new AbortController()
286+
const hold = { ...PRIMARY_CLICK, holdMs: 10_000 }
287+
const click = clickAt(contents, 5, 6, false, hold, controller.signal)
288+
await vi.advanceTimersByTimeAsync(100)
289+
expect(types()).toEqual(['mousePressed'])
290+
291+
controller.abort()
292+
await expect(click).rejects.toMatchObject({ name: 'AbortError' })
293+
expect(types()).toEqual(['mousePressed', 'mouseReleased'])
294+
} finally {
295+
vi.useRealTimers()
296+
}
297+
})
298+
279299
it('keeps a held right-click marked as the agent context menu until release', async () => {
280300
const contents = new WebContentsView().webContents
281301
vi.useFakeTimers()

‎apps/desktop/src/main/browser-agent/cdp.ts‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
import type { BrowserTheme } from '@sim/browser-protocol'
1212
import { createLogger } from '@sim/logger'
1313
import { getErrorMessage } from '@sim/utils/errors'
14-
import { sleep } from '@sim/utils/helpers'
14+
import { interruptibleSleep, sleep } from '@sim/utils/helpers'
1515
import { isRecordLike } from '@sim/utils/object'
1616
import type { NativeImage, WebContents, WebFrameMain } from 'electron'
1717

@@ -983,12 +983,18 @@ export function clearAgentContextMenu(contents: WebContents): void {
983983
agentContextClicks.delete(contents)
984984
}
985985

986+
/**
987+
* Clicks at viewport coordinates. `signal` ends a press-and-hold early: the click rejects with the
988+
* abort reason and the button is released at once, so a cancelled or timed-out click cannot stay
989+
* held into the next action. That release can still activate the control under the pointer.
990+
*/
986991
export async function clickAt(
987992
contents: WebContents,
988993
x: number,
989994
y: number,
990995
moveBeforePress = true,
991-
click: PointerClick = PRIMARY_CLICK
996+
click: PointerClick = PRIMARY_CLICK,
997+
signal?: AbortSignal
992998
): Promise<void> {
993999
if (moveBeforePress) await moveMouse(contents, x, y)
9941000
const { button, clickCount, modifiers, holdMs } = click
@@ -1013,12 +1019,13 @@ export async function clickAt(
10131019
clickCount: count,
10141020
})
10151021
if (holdMs > 0) {
1016-
await sleep(holdMs)
1022+
await interruptibleSleep(holdMs, signal)
10171023
// Windows opens the context menu on release, after the hold; renew a marker a
10181024
// press-time menu has not already consumed.
10191025
if (button === 'right' && agentContextClicks.has(contents)) {
10201026
agentContextClicks.set(contents, Date.now())
10211027
}
1028+
signal?.throwIfAborted()
10221029
}
10231030
await sendInput(contents, 'Input.dispatchMouseEvent', {
10241031
type: 'mouseReleased',

‎apps/desktop/src/main/browser-agent/driver.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3223,7 +3223,7 @@ async function executeToolInner(
32233223
try {
32243224
assertCurrentExecution()
32253225
assertElementActionCurrent(contents, elementId, target)
3226-
await cdp.clickAt(contents, x, y, false, click)
3226+
await cdp.clickAt(contents, x, y, false, click, signal)
32273227
trusted = true
32283228
activation = 'native-pointer'
32293229
} catch (error) {
@@ -3263,7 +3263,7 @@ async function executeToolInner(
32633263
try {
32643264
assertCurrentExecution()
32653265
assertElementActionCurrent(contents, elementId, target)
3266-
await cdp.clickAt(contents, finalTopPoint.x, finalTopPoint.y, false, click)
3266+
await cdp.clickAt(contents, finalTopPoint.x, finalTopPoint.y, false, click, signal)
32673267
trusted = true
32683268
activation = 'native-pointer'
32693269
prepared = finalSurface
@@ -4642,7 +4642,7 @@ async function executeToolInner(
46424642
assertCurrentExecution()
46434643
assertActiveContents(contents, clickNavigationEpoch)
46444644
try {
4645-
await cdp.clickAt(contents, x, y, true, click)
4645+
await cdp.clickAt(contents, x, y, true, click, signal)
46464646
} catch (error) {
46474647
const rescued = navigationRescue(contents, clickNavigationEpoch, urlAtDispatch, {
46484648
trusted: true,

‎apps/desktop/src/main/browser-agent/keyboard.test.ts‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -328,6 +328,17 @@ describe('dispatchKeyCombo', () => {
328328
.mocked(contents.debugger.sendCommand)
329329
.mock.calls.map(([, params]) => `${toRecord(params).type} ${toRecord(params).key}`)
330330
expect(keys).toEqual(['rawKeyDown Control', 'rawKeyDown Shift', 'keyUp Shift', 'keyUp Control'])
331+
332+
const early = new WebContentsView().webContents
333+
vi.mocked(early.debugger.sendCommand).mockRejectedValueOnce(new Error('control-down lost'))
334+
await expect(dispatchKeyCombo(early, parseKeyCombo('Ctrl+Shift+K'))).rejects.toMatchObject({
335+
name: KeyDispatchError.name,
336+
})
337+
expect(
338+
vi
339+
.mocked(early.debugger.sendCommand)
340+
.mock.calls.map(([, params]) => `${toRecord(params).type} ${toRecord(params).key}`)
341+
).toEqual(['rawKeyDown Control', 'keyUp Control'])
331342
})
332343

333344
it('does not turn menu-restoration cleanup failure into a duplicate key retry signal', async () => {

‎apps/desktop/src/main/browser-agent/keyboard.ts‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
/**
22
* Keyboard machinery for `browser_press_key` and internal key dispatch:
3-
* parsing "Cmd+Shift+Z"-style combos and building the trusted CDP
4-
* keyDown/keyUp pair. Pure logic except {@link dispatchKeyCombo}.
3+
* parsing "Cmd+Shift+Z"-style combos and building the trusted CDP key events —
4+
* each modifier's own press and release around the main key's keyDown/keyUp
5+
* pair. Pure logic except {@link dispatchKeyCombo}.
56
*/
67
import { getErrorMessage } from '@sim/utils/errors'
78
import type { WebContents } from 'electron'
@@ -360,8 +361,9 @@ export function modifierKeyEvents(
360361
* Electron normally lets modified key events escape a focused WebContents to
361362
* application-menu accelerators. Agent input must stay inside the browser — a
362363
* page-level Cmd shortcut must never reload, close, or open a native window in
363-
* Sim — so menu handling is suspended for the complete CDP down/up pair. The
364-
* depth counter keeps overlapping tool calls from re-enabling it too early.
364+
* Sim — so menu handling is suspended for the whole chord, modifier presses
365+
* included. The depth counter keeps overlapping tool calls from re-enabling it
366+
* too early.
365367
*/
366368
export async function dispatchKeyCombo(contents: WebContents, combo: ParsedCombo): Promise<void> {
367369
const [down, up] = buildKeyDispatchPlan(combo)

‎apps/desktop/src/main/browser-agent/page-functions.test.ts‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2537,6 +2537,39 @@ describe('modal hidden together with its own app root', () => {
25372537
expect((readPageActionState() as { dialogs: string[] }).dialogs).toEqual(['Upper'])
25382538
})
25392539

2540+
it('keeps a dialog the app aria-hid below its root hidden', () => {
2541+
document.body.innerHTML = `
2542+
<div id="__next"><main>
2543+
<button>Shown action</button>
2544+
<div class="carousel-slide" aria-hidden="true">
2545+
<div role="dialog" aria-modal="true" aria-label="Offscreen"><button>Ghost</button></div>
2546+
</div>
2547+
</main></div>`
2548+
showAll()
2549+
2550+
const outline = outlineOf(collectSnapshot())
2551+
2552+
expect(outline).toContain('Shown action')
2553+
expect(outline).not.toContain('Ghost')
2554+
})
2555+
2556+
it('reads a disablePortal dialog inside a same-origin iframe', () => {
2557+
const frame = visible(document.createElement('iframe'))
2558+
document.body.append(frame)
2559+
const inner = frame.contentDocument as Document
2560+
inner.body.innerHTML = `
2561+
<div id="root" aria-hidden="true">
2562+
<button>Framed compose</button>
2563+
<div role="dialog" aria-modal="true" aria-label="Framed"><button>Framed send</button></div>
2564+
</div>`
2565+
for (const element of Array.from(inner.body.querySelectorAll('*'))) visible(element)
2566+
2567+
const outline = outlineOf(collectSnapshot())
2568+
2569+
expect(outline).toContain('Framed send')
2570+
expect(outline).not.toContain('Framed compose')
2571+
})
2572+
25402573
it('exposes a disablePortal modal nested inside another open modal', () => {
25412574
document.body.innerHTML = `
25422575
<div id="__next" aria-hidden="true"><main>

‎apps/desktop/src/main/browser-agent/page-functions.ts‎

Lines changed: 29 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ declare global {
4646
/** Elements an earlier snapshot of this document listed; a later snapshot marks the rest `new`. */
4747
__simAgentShownElements?: WeakSet<Element>
4848
/** Installed by {@link installPageHelpers} before every page function; cached per call. */
49-
__simAgentFindExemptModal: () => Element | null
49+
__simAgentIsExemptModal: (element: Element) => boolean
5050
/** Why the last __simAgentResolveElement call returned null — read by the
5151
* shared stale-error producers so a refusal names its cause instead of
5252
* the blanket "the page changed". Cleared on every successful resolve. */
@@ -66,23 +66,27 @@ export function serializePageCall(fn: (...args: never[]) => unknown, args: unkno
6666
*/
6767
export function installPageHelpers(): void {
6868
/**
69-
* The open modal the page aria-hid together with everything else. MUI's ModalManager
69+
* The open modal each document aria-hid together with everything else. MUI's ModalManager
7070
* aria-hides every <body> child except the modal's mount node, and a `disablePortal` modal
7171
* mounts inside the app root it just hid, so its own ancestor carries aria-hidden. Visibility
72-
* checks skip only the aria-hidden test on ancestors above this modal. Null when any modal is
73-
* visible unmodified (the portaled case) or no single topmost modal is contained by all of its
74-
* hidden ancestors. A modal nested inside another open modal is the topmost of the two.
72+
* checks skip only the aria-hidden test on ancestors above this modal. A document has none when
73+
* any modal is visible unmodified (the portaled case), when anything other than a <body> child
74+
* hides a modal (the app hid that dialog itself), or when no single innermost modal is contained
75+
* by every hidden ancestor. Looked up per document, so a same-origin iframe gets its own.
7576
*/
76-
let exemptModal: Element | null | undefined
77-
window.__simAgentFindExemptModal = (): Element | null => {
78-
if (exemptModal === undefined) exemptModal = findExemptModal()
79-
return exemptModal
77+
const exemptModals = new Map<Document, Element | null>()
78+
window.__simAgentIsExemptModal = (element: Element): boolean => {
79+
const doc = element.ownerDocument
80+
let modal = exemptModals.get(doc)
81+
if (modal === undefined) {
82+
modal = findExemptModal(doc)
83+
exemptModals.set(doc, modal)
84+
}
85+
return modal === element
8086
}
81-
function findExemptModal(): Element | null {
87+
function findExemptModal(doc: Document): Element | null {
8288
const rendered: Array<{ modal: Element; hidden: Element[] }> = []
83-
for (const modal of Array.from(
84-
document.querySelectorAll('[aria-modal="true"], dialog[open]')
85-
)) {
89+
for (const modal of Array.from(doc.querySelectorAll('[aria-modal="true"], dialog[open]'))) {
8690
const rect = modal.getBoundingClientRect()
8791
if (rect.width <= 0 || rect.height <= 0 || modal.getAttribute('aria-hidden') === 'true')
8892
continue
@@ -106,7 +110,9 @@ export function installPageHelpers(): void {
106110
current = 'host' in root ? (root.host as Element) : null
107111
}
108112
}
109-
if (visible) rendered.push({ modal, hidden })
113+
if (visible && hidden.every((ancestor) => ancestor.parentElement === doc.body)) {
114+
rendered.push({ modal, hidden })
115+
}
110116
}
111117
if (rendered.some(({ hidden }) => hidden.length === 0)) return null
112118
const topmost = rendered.filter(
@@ -283,7 +289,7 @@ export function collectSnapshot(startingElementId = 0, elementId?: number): unkn
283289
!current.hasAttribute('hidden') &&
284290
(aboveExemptModal || current.getAttribute('aria-hidden') !== 'true')
285291
)
286-
if (current === window.__simAgentFindExemptModal()) aboveExemptModal = true
292+
if (window.__simAgentIsExemptModal(current)) aboveExemptModal = true
287293
if (current.parentElement) current = current.parentElement
288294
else {
289295
const root = current.getRootNode()
@@ -872,7 +878,7 @@ export function collectSnapshot(startingElementId = 0, elementId?: number): unkn
872878
) {
873879
return false
874880
}
875-
if (current === window.__simAgentFindExemptModal()) aboveExemptModal = true
881+
if (window.__simAgentIsExemptModal(current)) aboveExemptModal = true
876882
if (current.parentElement) current = current.parentElement
877883
else {
878884
const root = current.getRootNode()
@@ -1110,7 +1116,7 @@ export function clickElement(
11101116
) {
11111117
return { error: 'not-visible' }
11121118
}
1113-
if (current === window.__simAgentFindExemptModal()) aboveExemptModal = true
1119+
if (window.__simAgentIsExemptModal(current)) aboveExemptModal = true
11141120
if (current.parentElement) current = current.parentElement
11151121
else {
11161122
const root = current.getRootNode()
@@ -1603,7 +1609,7 @@ export function focusElementForTyping(id: number, moveFocus = true): unknown {
16031609
) {
16041610
return { error: 'not-visible' }
16051611
}
1606-
if (current === window.__simAgentFindExemptModal()) aboveExemptModal = true
1612+
if (window.__simAgentIsExemptModal(current)) aboveExemptModal = true
16071613
}
16081614

16091615
if (moveFocus) {
@@ -2349,7 +2355,7 @@ export function readPageActionState(
23492355
) {
23502356
return false
23512357
}
2352-
if (current === window.__simAgentFindExemptModal()) aboveExemptModal = true
2358+
if (window.__simAgentIsExemptModal(current)) aboveExemptModal = true
23532359
if (current.parentElement) current = current.parentElement
23542360
else {
23552361
const root = current.getRootNode()
@@ -2514,7 +2520,7 @@ export function readPageActionState(
25142520
) {
25152521
return false
25162522
}
2517-
if (current === window.__simAgentFindExemptModal()) aboveExemptModal = true
2523+
if (window.__simAgentIsExemptModal(current)) aboveExemptModal = true
25182524
if (current.parentElement) current = current.parentElement
25192525
else {
25202526
const root = current.getRootNode()
@@ -2649,7 +2655,7 @@ export function scrollPage(direction: string, amount?: number, elementId?: numbe
26492655
) {
26502656
return false
26512657
}
2652-
if (current === window.__simAgentFindExemptModal()) aboveExemptModal = true
2658+
if (window.__simAgentIsExemptModal(current)) aboveExemptModal = true
26532659
if (current.parentElement) current = current.parentElement
26542660
else {
26552661
const root = current.getRootNode()
@@ -3133,7 +3139,7 @@ export function getElementScreenshotRect(id: number): unknown {
31333139
) {
31343140
return { error: 'not-visible' }
31353141
}
3136-
if (current === window.__simAgentFindExemptModal()) aboveExemptModal = true
3142+
if (window.__simAgentIsExemptModal(current)) aboveExemptModal = true
31373143
if (current.parentElement) current = current.parentElement
31383144
else {
31393145
const root = current.getRootNode()
@@ -3284,7 +3290,7 @@ export function readChildFrameElementState(
32843290
visible = false
32853291
break
32863292
}
3287-
if (current === window.__simAgentFindExemptModal()) aboveExemptModal = true
3293+
if (window.__simAgentIsExemptModal(current)) aboveExemptModal = true
32883294
if (style.transform && style.transform !== 'none') {
32893295
try {
32903296
if (typeof DOMMatrixReadOnly !== 'function') {

‎apps/desktop/src/main/browser-agent/session.test.ts‎

Lines changed: 38 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { existsSync, mkdirSync, mkdtempSync, readdirSync, writeFileSync } from 'node:fs'
1+
import { existsSync, mkdirSync, mkdtempSync, readdirSync, renameSync, writeFileSync } from 'node:fs'
22
import { tmpdir } from 'node:os'
33
import { basename, dirname, join } from 'node:path'
44
import type { MenuItemConstructorOptions, WebContents } from 'electron'
@@ -3533,6 +3533,43 @@ describe('browser-agent session', () => {
35333533
}
35343534
})
35353535

3536+
it('retries moving a completed download while another process briefly holds the file', async () => {
3537+
vi.useFakeTimers({ toFake: ['setTimeout'] })
3538+
try {
3539+
const directory = mkdtempSync(join(tmpdir(), 'sim-browser-downloads-'))
3540+
const busy = Object.assign(new Error('resource busy'), { code: 'EBUSY' })
3541+
const moveFile = vi
3542+
.fn<(from: string, to: string) => Promise<void>>()
3543+
.mockRejectedValueOnce(busy)
3544+
.mockRejectedValueOnce(busy)
3545+
.mockImplementation(async (from, to) => renameSync(from, to))
3546+
session = freshSession(win, {}, undefined, {
3547+
getDirectory: () => directory,
3548+
getFreeDiskBytes: () => Number.MAX_SAFE_INTEGER,
3549+
moveFile,
3550+
})
3551+
const contents = (session.ensureTab().view as unknown as MockView).webContents
3552+
const download = mockDownloadItem({ filename: 'held.bin', totalBytes: 100 })
3553+
3554+
startMockDownload(contents, download)
3555+
await vi.waitFor(() => expect(download.item.resume).toHaveBeenCalledOnce())
3556+
download.emitDone('completed')
3557+
await vi.waitFor(() => expect(moveFile).toHaveBeenCalledOnce())
3558+
await vi.advanceTimersByTimeAsync(1_000)
3559+
3560+
await vi.waitFor(() =>
3561+
expect(session.getBrowserDownloadsState('chat-test').downloads[0]).toMatchObject({
3562+
filename: 'held.bin',
3563+
state: 'completed',
3564+
})
3565+
)
3566+
expect(moveFile).toHaveBeenCalledTimes(3)
3567+
expect(finishedDownloadFiles(directory)).toEqual(['held.bin'])
3568+
} finally {
3569+
vi.useRealTimers()
3570+
}
3571+
})
3572+
35363573
it('interrupts a completed download whose staging file cannot be moved into place', async () => {
35373574
const directory = mkdtempSync(join(tmpdir(), 'sim-browser-downloads-'))
35383575
session = freshSession(win, {}, undefined, {

0 commit comments

Comments
 (0)