Skip to content

Bug 2080970 - Migrate dialog tools to BiDi #234

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hbenl merged 3 commits into from
Nov 4, 2026
Merged

Bug 2080970 - Migrate dialog tools to BiDi #234

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
31b06b4
chore: add mcpTest
hbenl Nov 3, 2026
452e7ff
Bug 2080970 - Migrate dialog tools to BiDi
hbenl Nov 3, 2026
81a8dfd
docs: use mcpTest instead of startMcpServer
hbenl Nov 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/testing.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ release and demo tooling only.

Pick the kind of test from where the logic under test lives.

1. **MCP harness** (`tests/integration/mcp/`, `startMcpServer` from
1. **MCP harness** (`tests/integration/mcp/`, `mcpTest` from
`tests/helpers/mcp.ts`). Starts the server in a child process and calls tools
through an MCP client. For logic in tool handlers and in the server
lifecycle (start, reconnect, restart, close). One file per tool module, plus
Expand Down
26 changes: 19 additions & 7 deletions src/firefox/pages.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,12 +164,17 @@ export class PageManagement {
* @param promptText - Optional text to enter in prompt dialog
*/
async acceptDialog(promptText?: string): Promise<void> {
const contextId = this.getCurrentContextId();
if (!contextId) {
throw new Error(`Cannot accept dialog: no browsing context ID`);
}

try {
const alert = await this.driver.switchTo().alert();
if (promptText !== undefined) {
await alert.sendKeys(promptText);
}
await alert.accept();
await this.sendBiDiCommand('browsingContext.handleUserPrompt', {
context: contextId,
accept: true,
userText: promptText,
});

@juliandescottes juliandescottes Nov 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will cause a regression in one edge case, when prompt is called with a default value, eg prompt("favorite color", "pink"). Previously the default value would be used if promptText was undefined, but here we will default to empty string.

We can workaround by listening to userPromptOpened and storing the defaultValue, but that sounds quite hacky.

To be honest I think this is rather a BiDi spec issue, because there is no easy way to use the default value of a prompt, but maybe we should still work around this here.

Or we can also wait and handle that when we fix Bug 2081600 - [firefox-devtools-mcp] Report open alert/confirm/prompt dialogs and the tools that handle them, which will require to listen to this event anyway.

@hbenl hbenl Nov 4, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's handle this together with 2081600.

} catch (error) {
throw new Error(
`Failed to accept dialog: ${error instanceof Error ? error.message : String(error)}`
Expand All @@ -181,9 +186,16 @@ export class PageManagement {
* Dismiss dialog (alert/confirm/prompt)
*/
async dismissDialog(): Promise<void> {
const contextId = this.getCurrentContextId();
if (!contextId) {
throw new Error(`Cannot dismiss dialog: no browsing context ID`);
}

try {
const alert = await this.driver.switchTo().alert();
await alert.dismiss();
await this.sendBiDiCommand('browsingContext.handleUserPrompt', {
context: contextId,
accept: false,
});
} catch (error) {
throw new Error(
`Failed to dismiss dialog: ${error instanceof Error ? error.message : String(error)}`
Expand Down
30 changes: 4 additions & 26 deletions src/tools/utilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,19 +87,8 @@ export const handleAcceptDialog = defineToolHandler<typeof acceptDialogTool>(
const { getFirefox } = await import('../index.js');
const firefox = await getFirefox();

try {
await firefox.acceptDialog(promptText);
return successResponse(promptText ? `Accepted: "${promptText}"` : 'Accepted');
} catch (error) {
const errorMsg = (error as Error).message;

// Concise error for no active dialog
if (errorMsg.includes('no such alert') || errorMsg.includes('No dialog')) {
throw new Error('No active dialog');
}

throw error;
}
await firefox.acceptDialog(promptText);
return successResponse(promptText ? `Accepted: "${promptText}"` : 'Accepted');
}
);

Expand All @@ -108,19 +97,8 @@ export const handleDismissDialog = defineToolHandler<typeof dismissDialogTool>(
const { getFirefox } = await import('../index.js');
const firefox = await getFirefox();

try {
await firefox.dismissDialog();
return successResponse('Dismissed');
} catch (error) {
const errorMsg = (error as Error).message;

// Concise error for no active dialog
if (errorMsg.includes('no such alert') || errorMsg.includes('No dialog')) {
throw new Error('No active dialog');
}

throw error;
}
await firefox.dismissDialog();
return successResponse('Dismissed');
}
);

Expand Down
9 changes: 9 additions & 0 deletions tests/helpers/mcp.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { mkdtempSync } from 'node:fs';
import { tmpdir } from 'node:os';
import { join, resolve } from 'node:path';
import { fileURLToPath } from 'node:url';
import { test } from 'vitest';
import { removeDir } from './fs.js';
import type { ToolArgsOf } from '@/types/tool-args.js';
import type { MODULES } from '@/tools/index.js';
Expand Down Expand Up @@ -88,3 +89,11 @@ export async function startMcpServer(serverArgs: string[] = []): Promise<McpTest
},
};
}

export const mcpTest = test

@juliandescottes juliandescottes Nov 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixture is really nice, we don't have that many mcp/ tests, but that looks a lot better.

Should we update docs/testing.md to mention this instead of startMcpServer?

.extend('mcpServerArgs', { scope: 'file' }, [] as string[])
.extend('mcp', { scope: 'file' }, async ({ mcpServerArgs }, { onCleanup }) => {
const server = await startMcpServer(mcpServerArgs);
onCleanup(() => server.close());
return server;
});
117 changes: 51 additions & 66 deletions tests/integration/mcp/script.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,37 +2,22 @@
* Integration tests for evaluate_script, called through the MCP server
*/

import { describe, it, expect, assert, beforeAll, afterAll, beforeEach } from 'vitest';
import { expect, assert } from 'vitest';
import { existsSync, readFileSync } from 'node:fs';
import { join } from 'node:path';
import { fixtureUrl } from '../../helpers/firefox.js';
import { startMcpServer, type McpTestServer } from '../../helpers/mcp.js';
import { mcpTest, type McpTestServer } from '../../helpers/mcp.js';

describe('evaluate_script (MCP)', () => {
let server: McpTestServer | undefined;
mcpTest.override('mcpServerArgs', ['--tools', 'pages', 'snapshot', 'script']);

function mcp(): McpTestServer {
if (!server) {
throw new Error('MCP server did not start');
}
return server;
}

beforeAll(async () => {
server = await startMcpServer(['--tools', 'pages', 'snapshot', 'script']);
});

afterAll(async () => {
await server?.close();
});

beforeEach(async () => {
const result = await mcp().callTool('navigate_page', { url: fixtureUrl('simple.html') });
mcpTest.describe('evaluate_script (MCP)', () => {
mcpTest.beforeEach(async ({ mcp }) => {
const result = await mcp.callTool('navigate_page', { url: fixtureUrl('simple.html') });
expect(result.isError).toBe(false);
});

it('returns the value of the function', async () => {
const result = await mcp().callTool('evaluate_script', {
mcpTest('returns the value of the function', async ({ mcp }) => {
const result = await mcp.callTool('evaluate_script', {
function: '() => ({ title: document.title, count: 1 + 1 })',
});

Expand All @@ -42,21 +27,21 @@ describe('evaluate_script (MCP)', () => {
expect(result.text).toContain('"count": 2');
});

it('awaits a returned promise', async () => {
const result = await mcp().callTool('evaluate_script', {
mcpTest('awaits a returned promise', async ({ mcp }) => {
const result = await mcp.callTool('evaluate_script', {
function: '() => new Promise((r) => setTimeout(() => r("done"), 50))',
});

expect(result.isError).toBe(false);
expect(result.text).toContain('"done"');
});

it('passes snapshot UIDs as element arguments', async () => {
const snapshot = await mcp().callTool('take_snapshot');
mcpTest('passes snapshot UIDs as element arguments', async ({ mcp }) => {
const snapshot = await mcp.callTool('take_snapshot');
const uid = /uid=(\S+) button "Click Me"/.exec(snapshot.text)?.[1];
assert(uid, 'snapshot contains the button uid');

const result = await mcp().callTool('evaluate_script', {
const result = await mcp.callTool('evaluate_script', {
function: '(el) => el.id',
args: [{ uid }],
});
Expand All @@ -65,9 +50,9 @@ describe('evaluate_script (MCP)', () => {
expect(result.text).toContain('"clickBtn"');
});

it('reports an unknown UID', async () => {
await mcp().callTool('take_snapshot');
const result = await mcp().callTool('evaluate_script', {
mcpTest('reports an unknown UID', async ({ mcp }) => {
await mcp.callTool('take_snapshot');
const result = await mcp.callTool('evaluate_script', {
function: '(el) => el.id',
args: [{ uid: 'does-not-exist' }],
});
Expand All @@ -76,8 +61,8 @@ describe('evaluate_script (MCP)', () => {
expect(result.text).toContain('does-not-exist');
});

it('reports an exception thrown by the function', async () => {
const result = await mcp().callTool('evaluate_script', {
mcpTest('reports an exception thrown by the function', async ({ mcp }) => {
const result = await mcp.callTool('evaluate_script', {
function: '() => { throw new Error("failure in page"); }',
});

Expand All @@ -86,14 +71,14 @@ describe('evaluate_script (MCP)', () => {
expect(result.text).toContain('failure in page');
});

it('rejects a plain expression', async () => {
const result = await mcp().callTool('evaluate_script', { function: 'document.title' });
mcpTest('rejects a plain expression', async ({ mcp }) => {
const result = await mcp.callTool('evaluate_script', { function: 'document.title' });

expect(result.isError).toBe(true);
});

it('times out a function that never settles', async () => {
const result = await mcp().callTool('evaluate_script', {
mcpTest('times out a function that never settles', async ({ mcp }) => {
const result = await mcp.callTool('evaluate_script', {
function: '() => new Promise(() => {})',
timeout: 200,
});
Expand All @@ -102,14 +87,14 @@ describe('evaluate_script (MCP)', () => {
expect(result.text).toContain('timed out (exceeded 200ms)');
});

describe('sandbox', () => {
it('does not see page globals', async () => {
await mcp().callTool('evaluate_script', { function: '() => { window.pageValue = 42; }' });
mcpTest.describe('sandbox', () => {
mcpTest('does not see page globals', async ({ mcp }) => {
await mcp.callTool('evaluate_script', { function: '() => { window.pageValue = 42; }' });

const page = await mcp().callTool('evaluate_script', {
const page = await mcp.callTool('evaluate_script', {
function: '() => typeof window.pageValue',
});
const sandboxed = await mcp().callTool('evaluate_script', {
const sandboxed = await mcp.callTool('evaluate_script', {
function: '() => typeof window.pageValue',
sandbox: 'isolated',
});
Expand All @@ -118,8 +103,8 @@ describe('evaluate_script (MCP)', () => {
expect(sandboxed.text).toContain('"undefined"');
});

it('shares the page DOM', async () => {
const result = await mcp().callTool('evaluate_script', {
mcpTest('shares the page DOM', async ({ mcp }) => {
const result = await mcp.callTool('evaluate_script', {
function: '() => document.getElementById("title").textContent',
sandbox: 'isolated',
});
Expand All @@ -128,17 +113,17 @@ describe('evaluate_script (MCP)', () => {
expect(result.text).toContain('"Simple Test Page"');
});

it('keeps its state across calls with the same name', async () => {
await mcp().callTool('evaluate_script', {
mcpTest('keeps its state across calls with the same name', async ({ mcp }) => {
await mcp.callTool('evaluate_script', {
function: '() => { window.sandboxValue = "kept"; }',
sandbox: 'first',
});

const same = await mcp().callTool('evaluate_script', {
const same = await mcp.callTool('evaluate_script', {
function: '() => window.sandboxValue ?? null',
sandbox: 'first',
});
const other = await mcp().callTool('evaluate_script', {
const other = await mcp.callTool('evaluate_script', {
function: '() => window.sandboxValue ?? null',
sandbox: 'second',
});
Expand All @@ -148,14 +133,14 @@ describe('evaluate_script (MCP)', () => {
});
});

describe('saveTo', () => {
function outputDir(): string {
return join(mcp().home, '.firefox-mcp', 'output');
mcpTest.describe('saveTo', () => {
function outputDir(mcp: McpTestServer): string {
return join(mcp.home, '.firefox-mcp', 'output');
}

it('writes the result to the given file without a preview', async () => {
const filePath = join(outputDir(), 'result.json');
const result = await mcp().callTool('evaluate_script', {
mcpTest('writes the result to the given file without a preview', async ({ mcp }) => {
const filePath = join(outputDir(mcp), 'result.json');
const result = await mcp.callTool('evaluate_script', {
function: '() => "x".repeat(5000)',
saveTo: filePath,
});
Expand All @@ -166,10 +151,10 @@ describe('evaluate_script (MCP)', () => {
expect(JSON.parse(readFileSync(filePath, 'utf8'))).toBe('x'.repeat(5000));
});

it('returns a truncated preview when asked', async () => {
const result = await mcp().callTool('evaluate_script', {
mcpTest('returns a truncated preview when asked', async ({ mcp }) => {
const result = await mcp.callTool('evaluate_script', {
function: '() => "x".repeat(5000)',
saveTo: join(outputDir(), 'preview.json'),
saveTo: join(outputDir(mcp), 'preview.json'),
preview: 100,
});

Expand All @@ -179,20 +164,20 @@ describe('evaluate_script (MCP)', () => {
expect(result.text).not.toContain('x'.repeat(5000));
});

it('ignores a non-positive preview', async () => {
const result = await mcp().callTool('evaluate_script', {
mcpTest('ignores a non-positive preview', async ({ mcp }) => {
const result = await mcp.callTool('evaluate_script', {
function: '() => 1',
saveTo: join(outputDir(), 'no-preview.json'),
saveTo: join(outputDir(mcp), 'no-preview.json'),
preview: -1,
});

expect(result.isError).toBe(false);
expect(result.text).not.toContain('Preview:');
});

it('writes "undefined" when the function returns nothing', async () => {
const filePath = join(outputDir(), 'undefined.json');
const result = await mcp().callTool('evaluate_script', {
mcpTest('writes "undefined" when the function returns nothing', async ({ mcp }) => {
const filePath = join(outputDir(mcp), 'undefined.json');
const result = await mcp.callTool('evaluate_script', {
function: '() => undefined',
saveTo: filePath,
});
Expand All @@ -201,15 +186,15 @@ describe('evaluate_script (MCP)', () => {
expect(readFileSync(filePath, 'utf8')).toBe('undefined');
});

it('generates a file in the output directory when saveTo is true', async () => {
const result = await mcp().callTool('evaluate_script', {
mcpTest('generates a file in the output directory when saveTo is true', async ({ mcp }) => {
const result = await mcp.callTool('evaluate_script', {
function: '() => 1',
saveTo: true,
});

expect(result.isError).toBe(false);
const savedPath = /Result saved to: (.+) \(/.exec(result.text)?.[1];
expect(savedPath).toContain(join(outputDir(), 'evaluate-script-'));
expect(savedPath).toContain(join(outputDir(mcp), 'evaluate-script-'));
expect(existsSync(savedPath as string)).toBe(true);
});
});
Expand Down
Loading
Loading