Skip to content

Commit 51cdbe6

Browse files
waleedlatif1waleedlatif1
authored
fix(desktop-browser): ask the user before a page's alert, confirm, or leave-site prompt is answered (#8432)
* fix(desktop-browser): ask the user before a page's alert, confirm, or leave-site prompt is answered * fix(desktop-browser): ask only about the user's own on-screen page, never the agent's * fix(desktop-browser): label frame dialogs with their own origin and cover reload leave prompts * fix(desktop-browser): preserve native dialog ownership and keyboard focus
1 parent d0d00b1 commit 51cdbe6

9 files changed

Lines changed: 877 additions & 54 deletions

File tree

‎apps/desktop/e2e/browser-page-dialogs.spec.ts‎

Lines changed: 515 additions & 0 deletions
Large diffs are not rendered by default.

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

Lines changed: 43 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -61,11 +61,22 @@ function createOopifFrameFixture() {
6161
}
6262
}
6363

64+
/** Callbacks for a page with no user to ask, so the shell answers every dialog. */
65+
const shellAnswersDialogs = {
66+
claimUserDialog: () => false,
67+
onDialogClosed: () => {},
68+
claimUserLeave: () => false,
69+
}
70+
6471
describe('browser-agent CDP instrumentation', () => {
6572
it('leaves file chooser dialogs native so users can upload files', async () => {
6673
const contents = new WebContentsView().webContents
6774

68-
await ensureInstrumented(contents, { onDialog: vi.fn(), dialogResponse: () => null })
75+
await ensureInstrumented(contents, {
76+
onDialog: vi.fn(),
77+
dialogResponse: () => null,
78+
...shellAnswersDialogs,
79+
})
6980

7081
expect(contents.debugger.sendCommand).toHaveBeenCalledWith('Page.enable', undefined)
7182
expect(contents.debugger.sendCommand).not.toHaveBeenCalledWith(
@@ -86,10 +97,18 @@ describe('browser-agent CDP instrumentation', () => {
8697
})
8798

8899
await expect(
89-
ensureInstrumented(contents, { onDialog: vi.fn(), dialogResponse: () => null })
100+
ensureInstrumented(contents, {
101+
onDialog: vi.fn(),
102+
dialogResponse: () => null,
103+
...shellAnswersDialogs,
104+
})
90105
).rejects.toThrow('setup acknowledgement lost')
91106
await expect(
92-
ensureInstrumented(contents, { onDialog: vi.fn(), dialogResponse: () => null })
107+
ensureInstrumented(contents, {
108+
onDialog: vi.fn(),
109+
dialogResponse: () => null,
110+
...shellAnswersDialogs,
111+
})
93112
).resolves.toBeUndefined()
94113

95114
expect(autoAttachAttempts).toBe(2)
@@ -98,7 +117,11 @@ describe('browser-agent CDP instrumentation', () => {
98117
it('dismisses an OOPIF dialog on the flattened child session', async () => {
99118
const contents = new WebContentsView().webContents
100119
const onDialog = vi.fn()
101-
await ensureInstrumented(contents, { onDialog, dialogResponse: () => null })
120+
await ensureInstrumented(contents, {
121+
onDialog,
122+
dialogResponse: () => null,
123+
...shellAnswersDialogs,
124+
})
102125
const listener = vi
103126
.mocked(contents.debugger.on)
104127
.mock.calls.find(([event]) => event === 'message')?.[1] as
@@ -132,7 +155,7 @@ describe('browser-agent CDP instrumentation', () => {
132155
const contents = new WebContentsView().webContents
133156
const onDialog = vi.fn()
134157
const dialogResponse = vi.fn(() => ({ accept: true }))
135-
await ensureInstrumented(contents, { onDialog, dialogResponse })
158+
await ensureInstrumented(contents, { onDialog, dialogResponse, ...shellAnswersDialogs })
136159
const listener = vi
137160
.mocked(contents.debugger.on)
138161
.mock.calls.find(([event]) => event === 'message')?.[1] as
@@ -275,7 +298,11 @@ describe('browser-agent CDP instrumentation', () => {
275298
async (treeKind) => {
276299
const contents = new WebContentsView().webContents
277300
const { child, frameTree } = createOopifFrameFixture()
278-
await ensureInstrumented(contents, { onDialog: vi.fn(), dialogResponse: () => null })
301+
await ensureInstrumented(contents, {
302+
onDialog: vi.fn(),
303+
dialogResponse: () => null,
304+
...shellAnswersDialogs,
305+
})
279306
const listener = vi
280307
.mocked(contents.debugger.on)
281308
.mock.calls.find(([event]) => event === 'message')?.[1] as
@@ -378,7 +405,11 @@ describe('browser-agent CDP instrumentation', () => {
378405
it('falls back to the root target when OOPIF isolated-world creation fails', async () => {
379406
const contents = new WebContentsView().webContents
380407
const { child, frameTree } = createOopifFrameFixture()
381-
await ensureInstrumented(contents, { onDialog: vi.fn(), dialogResponse: () => null })
408+
await ensureInstrumented(contents, {
409+
onDialog: vi.fn(),
410+
dialogResponse: () => null,
411+
...shellAnswersDialogs,
412+
})
382413
const listener = vi
383414
.mocked(contents.debugger.on)
384415
.mock.calls.find(([event]) => event === 'message')?.[1] as
@@ -458,7 +489,11 @@ describe('browser-agent file input handles', () => {
458489
async function fileInputFixture(childSession = false) {
459490
const contents = new WebContentsView().webContents
460491
const { child, frameTree } = createOopifFrameFixture()
461-
await ensureInstrumented(contents, { onDialog: vi.fn(), dialogResponse: () => null })
492+
await ensureInstrumented(contents, {
493+
onDialog: vi.fn(),
494+
dialogResponse: () => null,
495+
...shellAnswersDialogs,
496+
})
462497
if (childSession) {
463498
const onMessage = vi.mocked(contents.debugger.on).mock.calls[0]?.[1] as
464499
| ((event: unknown, method: string, params: unknown, sessionId?: string) => void)

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

Lines changed: 48 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import { createLogger } from '@sim/logger'
1313
import { getErrorMessage } from '@sim/utils/errors'
1414
import { interruptibleSleep } from '@sim/utils/helpers'
1515
import { isRecordLike } from '@sim/utils/object'
16+
import { truncateAtCodePoint } from '@sim/utils/string'
1617
import type { NativeImage, WebContents, WebFrameMain } from 'electron'
1718

1819
const logger = createLogger('BrowserAgentCdp')
@@ -38,8 +39,14 @@ export interface DialogResponse {
3839
export interface CdpCallbacks {
3940
/** A JS dialog was handled; the driver surfaces it to the model. */
4041
onDialog: (dialog: PageDialog) => void
41-
/** The running action's requested answer; dialogs are dismissed when it has none. */
42+
/** The running action's requested answer; defaults to declining the dialog. */
4243
dialogResponse: () => DialogResponse | null
44+
/** Leaves a visible user-owned alert or confirm to Electron's native dialog. */
45+
claimUserDialog: () => boolean
46+
/** The dialog ended through a user answer, a CDP answer, or page teardown. */
47+
onDialogClosed: () => void
48+
/** True when the user, not the shell, decides this beforeunload. */
49+
claimUserLeave: () => boolean
4350
}
4451

4552
/** Per-tab callbacks, so a background tab's events reach ITS driver, not the
@@ -189,37 +196,55 @@ function handleDebuggerEvent(
189196
return
190197
}
191198
const callbacks = callbacksByContents.get(contents)
199+
if (method === 'Page.javascriptDialogClosed') {
200+
callbacks?.onDialogClosed()
201+
return
202+
}
192203
if (method === 'Page.javascriptDialogOpening') {
193204
const type = String(params.type ?? 'dialog')
194-
const message = String(params.message ?? '').slice(0, 500)
195-
// Dialogs never stay open: beforeunload is accepted (navigation proceeds),
196-
// and alert/confirm follow the running action's requested answer, defaulting
197-
// to dismissal so an unexpected dialog can never block the page.
198-
const accept = type === 'beforeunload' || callbacks?.dialogResponse()?.accept === true
199-
const answer = { accept }
200-
void (async () => {
201-
let handled = false
202-
try {
203-
await send(contents, 'Page.handleJavaScriptDialog', answer, parentSessionId)
204-
handled = true
205-
} catch {
206-
// Some Chromium builds surface an OOPIF's tab-modal dialog on its
207-
// flattened session but accept the answer only on the root target.
208-
if (parentSessionId) {
209-
try {
210-
await send(contents, 'Page.handleJavaScriptDialog', answer)
211-
handled = true
212-
} catch {}
213-
}
214-
}
205+
const rawMessage = String(params.message ?? '')
206+
const message = truncateAtCodePoint(rawMessage, 500, '')
207+
const requested = callbacks?.dialogResponse() ?? null
208+
if (!requested && (type === 'alert' || type === 'confirm') && callbacks?.claimUserDialog()) {
209+
return
210+
}
211+
if (type === 'beforeunload' && callbacks?.claimUserLeave()) {
212+
// The user's Leave replays the navigation; this unload stays cancelled.
213+
void answerDialog(contents, { accept: false }, parentSessionId)
214+
return
215+
}
216+
// CDP unblocks JavaScript, but Electron may retain the native dialog until navigation.
217+
const accept = type === 'beforeunload' || requested?.accept === true
218+
void answerDialog(contents, { accept }, parentSessionId).then((handled) => {
215219
if (handled) logger.info('Handled page dialog', { type, accept })
216220
else logger.warn('Could not handle page dialog', { type })
217221
callbacks?.onDialog({ type, message, handled, accepted: handled && accept })
218-
})()
222+
})
219223
return
220224
}
221225
}
222226

227+
async function answerDialog(
228+
contents: WebContents,
229+
answer: { accept: boolean },
230+
parentSessionId: string | undefined
231+
): Promise<boolean> {
232+
try {
233+
await send(contents, 'Page.handleJavaScriptDialog', answer, parentSessionId)
234+
return true
235+
} catch {
236+
// Some Chromium builds surface an OOPIF's tab-modal dialog on its
237+
// flattened session but accept the answer only on the root target.
238+
if (!parentSessionId) return false
239+
try {
240+
await send(contents, 'Page.handleJavaScriptDialog', answer)
241+
return true
242+
} catch {
243+
return false
244+
}
245+
}
246+
}
247+
223248
interface ProtocolFrame {
224249
id: string
225250
parentId?: string

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

Lines changed: 32 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -554,6 +554,7 @@ function recordNotice(notice: string): void {
554554
function pageStateFor(contents: WebContents, tabId: string): BrowserPageState {
555555
const issue = session.pageIssueForContents(contents)
556556
const mediaPermissionRequest = session.mediaPermissionRequestForContents(contents)
557+
const dialog = session.pageDialogForContents(contents)
557558
return {
558559
scopeId: session.getBrowserScopeId(),
559560
tabId,
@@ -564,6 +565,7 @@ function pageStateFor(contents: WebContents, tabId: string): BrowserPageState {
564565
canGoForward: session.canGoForward(contents),
565566
...(issue ? { issue } : {}),
566567
...(mediaPermissionRequest ? { mediaPermissionRequest } : {}),
568+
...(dialog ? { dialog } : {}),
567569
}
568570
}
569571

@@ -615,6 +617,10 @@ function instrumentTab(contents: WebContents): void {
615617
const requested = driverScopeState().dialogResponse
616618
return requested?.contents === contents ? requested.response : null
617619
}),
620+
claimUserDialog: () =>
621+
session.withBrowserScope(scopeId, () => session.claimUserDialog(contents)),
622+
onDialogClosed: inScope(() => session.notePageDialogClosed(contents)),
623+
claimUserLeave: () => session.withBrowserScope(scopeId, () => session.claimUserLeave(contents)),
618624
}
619625
void (async () => {
620626
let lastError: unknown
@@ -5221,6 +5227,14 @@ export async function executeTool(
52215227
) {
52225228
throw new ToolError('This browser action was cancelled before it started.')
52235229
}
5230+
session.withBrowserScope(resolvedScopeId, () => {
5231+
const automation = session.automationTab()
5232+
if (automation && session.hasPendingPageDialog(automation.view.webContents)) {
5233+
throw new ToolError(
5234+
'The user is answering a dialog on this page. Wait for their answer before using it.'
5235+
)
5236+
}
5237+
})
52245238
state.activeToolCallId = toolCallId ?? null
52255239
const executionController = new AbortController()
52265240
let cancelActiveExecution: () => void = () => {}
@@ -5456,6 +5470,16 @@ export async function handlePanelAction(
54565470
}
54575471
return
54585472
}
5473+
if (action.action === 'enable-page-dialogs') {
5474+
session.enablePageDialogs()
5475+
return
5476+
}
5477+
if (action.action === 'respond-dialog') {
5478+
if (typeof action.requestId === 'string' && typeof action.allowed === 'boolean') {
5479+
session.respondToPageDialog(action.requestId, action.allowed)
5480+
}
5481+
return
5482+
}
54595483
if (action.action === 'respond-site-permission') {
54605484
/** Older renderers can still send a response to the retired task-navigation prompt. */
54615485
return
@@ -5471,8 +5495,11 @@ export async function handlePanelAction(
54715495
action.url,
54725496
{ agentOwned: false }
54735497
)
5474-
session.prepareExplicitNavigation(contents)
5475-
void contents.loadURL(action.url).catch(() => {})
5498+
const url = action.url
5499+
session.navigateForUser(contents, () => {
5500+
session.prepareExplicitNavigation(contents)
5501+
void contents.loadURL(url).catch(() => {})
5502+
})
54765503
session.focusPageForUser(contents)
54775504
}
54785505
return
@@ -5495,13 +5522,13 @@ export async function handlePanelAction(
54955522
const contents = tab.view.webContents
54965523
switch (action.action) {
54975524
case 'reload':
5498-
session.reloadPage(contents)
5525+
session.navigateForUser(contents, () => session.reloadPage(contents))
54995526
return
55005527
case 'back':
5501-
session.goBack(contents)
5528+
session.navigateForUser(contents, () => session.goBack(contents))
55025529
return
55035530
case 'forward':
5504-
session.goForward(contents)
5531+
session.navigateForUser(contents, () => session.goForward(contents))
55055532
return
55065533
case 'print':
55075534
contents.print({ printBackground: true })

0 commit comments

Comments
 (0)