Skip to content

Commit c7c8f50

Browse files
authored
refactor: move waitForEventsAfterAction to McpPage (#1780)
Splits out from #1244 per review feedback. 'waitForEventsAfterAction' previously lived in 'McpContext' and always used the selected page's CPU/network throttling settings. With pageId routing, a tool can target a different page than the selected one, meaning wrong throttling multipliers were applied. Moving the method to 'McpPage' fixes this: each tool now calls 'page.waitForEventsAfterAction(...)' and gets the correct page's emulation settings. 'getNetworkMultiplierFromString' is extracted to 'WaitForHelper.ts' to avoid a circular import (McpContext → McpPage already exists). Unblocks #1777.
1 parent 66ced45 commit c7c8f50

File tree

9 files changed

+89
-86
lines changed

9 files changed

+89
-86
lines changed

src/McpContext.ts

Lines changed: 1 addition & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ import {
4848
type InstalledExtension,
4949
} from './utils/ExtensionRegistry.js';
5050
import {saveTemporaryFile} from './utils/files.js';
51-
import {WaitForHelper} from './WaitForHelper.js';
51+
import {getNetworkMultiplierFromString} from './WaitForHelper.js';
5252

5353
interface McpContextOptions {
5454
// Whether the DevTools windows are exposed as pages for debugging of DevTools.
@@ -62,23 +62,6 @@ interface McpContextOptions {
6262
const DEFAULT_TIMEOUT = 5_000;
6363
const NAVIGATION_TIMEOUT = 10_000;
6464

65-
function getNetworkMultiplierFromString(condition: string | null): number {
66-
const puppeteerCondition =
67-
condition as keyof typeof PredefinedNetworkConditions;
68-
69-
switch (puppeteerCondition) {
70-
case 'Fast 4G':
71-
return 1;
72-
case 'Slow 4G':
73-
return 2.5;
74-
case 'Fast 3G':
75-
return 5;
76-
case 'Slow 3G':
77-
return 10;
78-
}
79-
return 1;
80-
}
81-
8265
export class McpContext implements Context {
8366
browser: Browser;
8467
logger: Debugger;
@@ -851,31 +834,6 @@ export class McpContext implements Context {
851834
return this.#traceResults;
852835
}
853836

854-
getWaitForHelper(
855-
page: Page,
856-
cpuMultiplier: number,
857-
networkMultiplier: number,
858-
) {
859-
return new WaitForHelper(page, cpuMultiplier, networkMultiplier);
860-
}
861-
862-
waitForEventsAfterAction(
863-
action: () => Promise<unknown>,
864-
options?: {timeout?: number},
865-
): Promise<void> {
866-
const page = this.#getSelectedMcpPage();
867-
const cpuMultiplier = page.cpuThrottlingRate;
868-
const networkMultiplier = getNetworkMultiplierFromString(
869-
page.networkConditions,
870-
);
871-
const waitForHelper = this.getWaitForHelper(
872-
page.pptrPage,
873-
cpuMultiplier,
874-
networkMultiplier,
875-
);
876-
return waitForHelper.waitForEventsAfterAction(action, options);
877-
}
878-
879837
getNetworkRequestStableId(request: HTTPRequest): number {
880838
return this.#networkCollector.getIdForResource(request);
881839
}

src/McpPage.ts

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,10 @@ import type {
1818
TextSnapshot,
1919
TextSnapshotNode,
2020
} from './types.js';
21+
import {
22+
getNetworkMultiplierFromString,
23+
WaitForHelper,
24+
} from './WaitForHelper.js';
2125

2226
/**
2327
* Per-page state wrapper. Consolidates dialog, snapshot, emulation,
@@ -91,6 +95,25 @@ export class McpPage implements ContextPage {
9195
return this.emulationSettings.colorScheme ?? null;
9296
}
9397

98+
// Public for testability: tests spy on this method to verify throttle multipliers.
99+
createWaitForHelper(
100+
cpuMultiplier: number,
101+
networkMultiplier: number,
102+
): WaitForHelper {
103+
return new WaitForHelper(this.pptrPage, cpuMultiplier, networkMultiplier);
104+
}
105+
106+
waitForEventsAfterAction(
107+
action: () => Promise<unknown>,
108+
options?: {timeout?: number},
109+
): Promise<void> {
110+
const helper = this.createWaitForHelper(
111+
this.cpuThrottlingRate,
112+
getNetworkMultiplierFromString(this.networkConditions),
113+
);
114+
return helper.waitForEventsAfterAction(action, options);
115+
}
116+
94117
dispose(): void {
95118
this.pptrPage.off('dialog', this.#dialogHandler);
96119
}

src/WaitForHelper.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
import {logger} from './logger.js';
88
import type {Page, Protocol, CdpPage} from './third_party/index.js';
9+
import type {PredefinedNetworkConditions} from './third_party/index.js';
910

1011
export class WaitForHelper {
1112
#abortController = new AbortController();
@@ -160,3 +161,22 @@ export class WaitForHelper {
160161
}
161162
}
162163
}
164+
165+
export function getNetworkMultiplierFromString(
166+
condition: string | null,
167+
): number {
168+
const puppeteerCondition =
169+
condition as keyof typeof PredefinedNetworkConditions;
170+
171+
switch (puppeteerCondition) {
172+
case 'Fast 4G':
173+
return 1;
174+
case 'Slow 4G':
175+
return 2.5;
176+
case 'Fast 3G':
177+
return 5;
178+
case 'Slow 3G':
179+
return 10;
180+
}
181+
return 1;
182+
}

src/tools/ToolDefinition.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -172,10 +172,6 @@ export type Context = Readonly<{
172172
data: Uint8Array<ArrayBufferLike>,
173173
filename: string,
174174
): Promise<{filename: string}>;
175-
waitForEventsAfterAction(
176-
action: () => Promise<unknown>,
177-
options?: {timeout?: number},
178-
): Promise<void>;
179175
waitForTextOnPage(
180176
text: string[],
181177
timeout?: number,
@@ -213,6 +209,10 @@ export type ContextPage = Readonly<{
213209

214210
getDialog(): Dialog | undefined;
215211
clearDialog(): void;
212+
waitForEventsAfterAction(
213+
action: () => Promise<unknown>,
214+
options?: {timeout?: number},
215+
): Promise<void>;
216216
}>;
217217

218218
export function defineTool<Schema extends zod.ZodRawShape>(

src/tools/inPage.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ export const executeInPageTool = definePageTool({
7171
.describe('The JSON-stringified parameters to pass to the tool'),
7272
},
7373
handler: async (request, response, context) => {
74-
const page = context.getSelectedMcpPage();
74+
const page = request.page;
7575
const toolName = request.params.toolName;
7676
let params: Record<string, unknown> = {};
7777
if (request.params.params) {

src/tools/input.ts

Lines changed: 14 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -58,11 +58,11 @@ export const click = definePageTool({
5858
dblClick: dblClickSchema,
5959
includeSnapshot: includeSnapshotSchema,
6060
},
61-
handler: async (request, response, context) => {
61+
handler: async (request, response) => {
6262
const uid = request.params.uid;
6363
const handle = await request.page.getElementByUid(uid);
6464
try {
65-
await context.waitForEventsAfterAction(async () => {
65+
await request.page.waitForEventsAfterAction(async () => {
6666
await handle.asLocator().click({
6767
count: request.params.dblClick ? 2 : 1,
6868
});
@@ -97,9 +97,9 @@ export const clickAt = definePageTool({
9797
dblClick: dblClickSchema,
9898
includeSnapshot: includeSnapshotSchema,
9999
},
100-
handler: async (request, response, context) => {
100+
handler: async (request, response) => {
101101
const page = request.page;
102-
await context.waitForEventsAfterAction(async () => {
102+
await page.waitForEventsAfterAction(async () => {
103103
await page.pptrPage.mouse.click(request.params.x, request.params.y, {
104104
clickCount: request.params.dblClick ? 2 : 1,
105105
});
@@ -130,11 +130,11 @@ export const hover = definePageTool({
130130
),
131131
includeSnapshot: includeSnapshotSchema,
132132
},
133-
handler: async (request, response, context) => {
133+
handler: async (request, response) => {
134134
const uid = request.params.uid;
135135
const handle = await request.page.getElementByUid(uid);
136136
try {
137-
await context.waitForEventsAfterAction(async () => {
137+
await request.page.waitForEventsAfterAction(async () => {
138138
await handle.asLocator().hover();
139139
});
140140
response.appendResponseLine(`Successfully hovered over the element`);
@@ -217,7 +217,6 @@ async function fillFormElement(
217217
}
218218
}
219219

220-
// here
221220
export const fill = definePageTool({
222221
name: 'fill',
223222
description: `Type text into a input, text area or select an option from a <select> element.`,
@@ -236,7 +235,7 @@ export const fill = definePageTool({
236235
},
237236
handler: async (request, response, context) => {
238237
const page = request.page;
239-
await context.waitForEventsAfterAction(async () => {
238+
await page.waitForEventsAfterAction(async () => {
240239
await fillFormElement(
241240
request.params.uid,
242241
request.params.value,
@@ -262,9 +261,9 @@ export const typeText = definePageTool({
262261
text: zod.string().describe('The text to type'),
263262
submitKey: submitKeySchema,
264263
},
265-
handler: async (request, response, context) => {
264+
handler: async (request, response) => {
266265
const page = request.page;
267-
await context.waitForEventsAfterAction(async () => {
266+
await page.waitForEventsAfterAction(async () => {
268267
await page.pptrPage.keyboard.type(request.params.text);
269268
if (request.params.submitKey) {
270269
await page.pptrPage.keyboard.press(
@@ -290,13 +289,13 @@ export const drag = definePageTool({
290289
to_uid: zod.string().describe('The uid of the element to drop into'),
291290
includeSnapshot: includeSnapshotSchema,
292291
},
293-
handler: async (request, response, context) => {
292+
handler: async (request, response) => {
294293
const fromHandle = await request.page.getElementByUid(
295294
request.params.from_uid,
296295
);
297296
const toHandle = await request.page.getElementByUid(request.params.to_uid);
298297
try {
299-
await context.waitForEventsAfterAction(async () => {
298+
await request.page.waitForEventsAfterAction(async () => {
300299
await fromHandle.drag(toHandle);
301300
await new Promise(resolve => setTimeout(resolve, 50));
302301
await toHandle.drop(fromHandle);
@@ -334,7 +333,7 @@ export const fillForm = definePageTool({
334333
handler: async (request, response, context) => {
335334
const page = request.page;
336335
for (const element of request.params.elements) {
337-
await context.waitForEventsAfterAction(async () => {
336+
await page.waitForEventsAfterAction(async () => {
338337
await fillFormElement(
339338
element.uid,
340339
element.value,
@@ -415,12 +414,12 @@ export const pressKey = definePageTool({
415414
),
416415
includeSnapshot: includeSnapshotSchema,
417416
},
418-
handler: async (request, response, context) => {
417+
handler: async (request, response) => {
419418
const page = request.page;
420419
const tokens = parseKey(request.params.key);
421420
const [key, ...modifiers] = tokens;
422421

423-
await context.waitForEventsAfterAction(async () => {
422+
await page.waitForEventsAfterAction(async () => {
424423
for (const modifier of modifiers) {
425424
await page.pptrPage.keyboard.down(modifier);
426425
}

src/tools/pages.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ export const newPage = defineTool({
119119
request.params.isolatedContext,
120120
);
121121

122-
await context.waitForEventsAfterAction(
122+
await page.waitForEventsAfterAction(
123123
async () => {
124124
await page.pptrPage.goto(request.params.url, {
125125
timeout: request.params.timeout,
@@ -166,7 +166,7 @@ export const navigatePage = definePageTool({
166166
),
167167
...timeoutSchema,
168168
},
169-
handler: async (request, response, context) => {
169+
handler: async (request, response) => {
170170
const page = request.page;
171171
const options = {
172172
timeout: request.params.timeout,
@@ -206,7 +206,7 @@ export const navigatePage = definePageTool({
206206
page.pptrPage.on('dialog', dialogHandler);
207207

208208
try {
209-
await context.waitForEventsAfterAction(
209+
await page.waitForEventsAfterAction(
210210
async () => {
211211
switch (request.params.type) {
212212
case 'url':

src/tools/script.ts

Lines changed: 20 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,11 @@ Example with arguments: \`(el) => {
7777
}
7878

7979
const worker = await getWebWorker(context, serviceWorkerId);
80-
await performEvaluation(worker, fnString, [], response, context);
80+
await context
81+
.getSelectedMcpPage()
82+
.waitForEventsAfterAction(async () => {
83+
await performEvaluation(worker, fnString, [], response);
84+
});
8185
return;
8286
}
8387

@@ -97,7 +101,9 @@ Example with arguments: \`(el) => {
97101

98102
const evaluatable = await getPageOrFrame(page, frames);
99103

100-
await performEvaluation(evaluatable, fnString, args, response, context);
104+
await mcpPage.waitForEventsAfterAction(async () => {
105+
await performEvaluation(evaluatable, fnString, args, response);
106+
});
101107
} finally {
102108
void Promise.allSettled(args.map(arg => arg.dispose()));
103109
}
@@ -110,24 +116,21 @@ const performEvaluation = async (
110116
fnString: string,
111117
args: Array<JSHandle<unknown>>,
112118
response: Response,
113-
context: Context,
114119
) => {
115120
const fn = await evaluatable.evaluateHandle(`(${fnString})`);
116121
try {
117-
await context.waitForEventsAfterAction(async () => {
118-
const result = await evaluatable.evaluate(
119-
async (fn, ...args) => {
120-
// @ts-expect-error no types for function fn
121-
return JSON.stringify(await fn(...args));
122-
},
123-
fn,
124-
...args,
125-
);
126-
response.appendResponseLine('Script ran on page and returned:');
127-
response.appendResponseLine('```json');
128-
response.appendResponseLine(`${result}`);
129-
response.appendResponseLine('```');
130-
});
122+
const result = await evaluatable.evaluate(
123+
async (fn, ...args) => {
124+
// @ts-expect-error no types for function fn
125+
return JSON.stringify(await fn(...args));
126+
},
127+
fn,
128+
...args,
129+
);
130+
response.appendResponseLine('Script ran on page and returned:');
131+
response.appendResponseLine('```json');
132+
response.appendResponseLine(`${result}`);
133+
response.appendResponseLine('```');
131134
} finally {
132135
void fn.dispose();
133136
}

tests/McpContext.test.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -75,13 +75,13 @@ describe('McpContext', () => {
7575
cpuThrottlingRate: 2,
7676
networkConditions: 'Slow 3G',
7777
});
78-
const stub = sinon.spy(context, 'getWaitForHelper');
78+
const stub = sinon.spy(page, 'createWaitForHelper');
7979

80-
await context.waitForEventsAfterAction(async () => {
80+
await page.waitForEventsAfterAction(async () => {
8181
// trigger the waiting only
8282
});
8383

84-
sinon.assert.calledWithExactly(stub, page.pptrPage, 2, 10);
84+
sinon.assert.calledWithExactly(stub, 2, 10);
8585
});
8686
});
8787

0 commit comments

Comments
 (0)