Skip to content

Commit 3d02f39

Browse files
refactor: address review feedback on navigation reporting
- Rename WaitForEventsResult.navigated to navigatedToUrl, returning the URL directly instead of a boolean. - Move appendNavigationIfAny into WaitForHelper as appendWaitForResult so it can be reused by other callers and matches the result interface. - Update input.ts and script.ts to use the shared helper. - Drop the misleading doc comment about same-document navigations.
1 parent d281454 commit 3d02f39

3 files changed

Lines changed: 37 additions & 37 deletions

File tree

src/WaitForHelper.ts

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -162,16 +162,27 @@ export class WaitForHelper {
162162
this.#abortController.abort();
163163
}
164164

165-
return {navigated};
165+
return {
166+
...(navigated ? {navigatedToUrl: this.#page.url()} : {}),
167+
};
166168
}
167169
}
168170

169171
export interface WaitForEventsResult {
170172
/**
171-
* Whether a cross-document navigation started and finished during the
172-
* action. Same-document (history API) navigations are not reported.
173+
* The URL the page navigated to during the action, if a navigation
174+
* occurred.
173175
*/
174-
navigated: boolean;
176+
navigatedToUrl?: string;
177+
}
178+
179+
export function appendWaitForResult(
180+
response: {appendResponseLine(value: string): void},
181+
result: WaitForEventsResult,
182+
): void {
183+
if (result.navigatedToUrl) {
184+
response.appendResponseLine(`Page navigated to ${result.navigatedToUrl}.`);
185+
}
175186
}
176187

177188
export function getNetworkMultiplierFromString(

src/tools/input.ts

Lines changed: 14 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -10,10 +10,13 @@ import {zod} from '../third_party/index.js';
1010
import type {ElementHandle, KeyInput} from '../third_party/index.js';
1111
import type {TextSnapshotNode} from '../types.js';
1212
import {parseKey} from '../utils/keyboard.js';
13-
import type {WaitForEventsResult} from '../WaitForHelper.js';
13+
import {
14+
appendWaitForResult,
15+
type WaitForEventsResult,
16+
} from '../WaitForHelper.js';
1417

1518
import {ToolCategory} from './categories.js';
16-
import type {ContextPage, Response} from './ToolDefinition.js';
19+
import type {ContextPage} from './ToolDefinition.js';
1720
import {definePageTool} from './ToolDefinition.js';
1821

1922
const dblClickSchema = zod
@@ -43,16 +46,6 @@ function handleActionError(error: unknown, uid: string) {
4346
);
4447
}
4548

46-
function appendNavigationIfAny(
47-
page: ContextPage,
48-
response: Response,
49-
result: WaitForEventsResult,
50-
) {
51-
if (result.navigated) {
52-
response.appendResponseLine(`Page navigated to ${page.pptrPage.url()}.`);
53-
}
54-
}
55-
5649
export const click = definePageTool({
5750
name: 'click',
5851
description: `Clicks on the provided element`,
@@ -83,7 +76,7 @@ export const click = definePageTool({
8376
? `Successfully double clicked on the element`
8477
: `Successfully clicked on the element`,
8578
);
86-
appendNavigationIfAny(request.page, response, result);
79+
appendWaitForResult(response, result);
8780
if (request.params.includeSnapshot) {
8881
response.includeSnapshot();
8982
}
@@ -121,7 +114,7 @@ export const clickAt = definePageTool({
121114
? `Successfully double clicked at the coordinates`
122115
: `Successfully clicked at the coordinates`,
123116
);
124-
appendNavigationIfAny(page, response, result);
117+
appendWaitForResult(response, result);
125118
if (request.params.includeSnapshot) {
126119
response.includeSnapshot();
127120
}
@@ -151,7 +144,7 @@ export const hover = definePageTool({
151144
await handle.asLocator().hover();
152145
});
153146
response.appendResponseLine(`Successfully hovered over the element`);
154-
appendNavigationIfAny(request.page, response, result);
147+
appendWaitForResult(response, result);
155148
if (request.params.includeSnapshot) {
156149
response.includeSnapshot();
157150
}
@@ -258,7 +251,7 @@ export const fill = definePageTool({
258251
);
259252
});
260253
response.appendResponseLine(`Successfully filled out the element`);
261-
appendNavigationIfAny(page, response, result);
254+
appendWaitForResult(response, result);
262255
if (request.params.includeSnapshot) {
263256
response.includeSnapshot();
264257
}
@@ -289,7 +282,7 @@ export const typeText = definePageTool({
289282
response.appendResponseLine(
290283
`Typed text "${request.params.text}${request.params.submitKey ? ` + ${request.params.submitKey}` : ''}"`,
291284
);
292-
appendNavigationIfAny(page, response, result);
285+
appendWaitForResult(response, result);
293286
},
294287
});
295288

@@ -317,7 +310,7 @@ export const drag = definePageTool({
317310
await toHandle.drop(fromHandle);
318311
});
319312
response.appendResponseLine(`Successfully dragged an element`);
320-
appendNavigationIfAny(request.page, response, result);
313+
appendWaitForResult(response, result);
321314
if (request.params.includeSnapshot) {
322315
response.includeSnapshot();
323316
}
@@ -349,7 +342,7 @@ export const fillForm = definePageTool({
349342
},
350343
handler: async (request, response, context) => {
351344
const page = request.page;
352-
let lastResult: WaitForEventsResult = {navigated: false};
345+
let lastResult: WaitForEventsResult = {};
353346
for (const element of request.params.elements) {
354347
lastResult = await page.waitForEventsAfterAction(async () => {
355348
await fillFormElement(
@@ -361,7 +354,7 @@ export const fillForm = definePageTool({
361354
});
362355
}
363356
response.appendResponseLine(`Successfully filled out the form`);
364-
appendNavigationIfAny(page, response, lastResult);
357+
appendWaitForResult(response, lastResult);
365358
if (request.params.includeSnapshot) {
366359
response.includeSnapshot();
367360
}
@@ -451,7 +444,7 @@ export const pressKey = definePageTool({
451444
response.appendResponseLine(
452445
`Successfully pressed key: ${request.params.key}`,
453446
);
454-
appendNavigationIfAny(page, response, result);
447+
appendWaitForResult(response, result);
455448
if (request.params.includeSnapshot) {
456449
response.includeSnapshot();
457450
}

src/tools/script.ts

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import {zod} from '../third_party/index.js';
88
import type {Frame, JSHandle, Page, WebWorker} from '../third_party/index.js';
99
import type {ExtensionServiceWorker} from '../types.js';
10+
import {appendWaitForResult} from '../WaitForHelper.js';
1011

1112
import {ToolCategory} from './categories.js';
1213
import type {Context, Response} from './ToolDefinition.js';
@@ -77,15 +78,12 @@ Example with arguments: \`(el) => {
7778
}
7879

7980
const worker = await getWebWorker(context, serviceWorkerId);
80-
const selectedPage = context.getSelectedMcpPage();
81-
const result = await selectedPage.waitForEventsAfterAction(async () => {
82-
await performEvaluation(worker, fnString, [], response);
83-
});
84-
if (result.navigated) {
85-
response.appendResponseLine(
86-
`Page navigated to ${selectedPage.pptrPage.url()}.`,
87-
);
88-
}
81+
const result = await context
82+
.getSelectedMcpPage()
83+
.waitForEventsAfterAction(async () => {
84+
await performEvaluation(worker, fnString, [], response);
85+
});
86+
appendWaitForResult(response, result);
8987
return;
9088
}
9189

@@ -108,9 +106,7 @@ Example with arguments: \`(el) => {
108106
const result = await mcpPage.waitForEventsAfterAction(async () => {
109107
await performEvaluation(evaluatable, fnString, args, response);
110108
});
111-
if (result.navigated) {
112-
response.appendResponseLine(`Page navigated to ${page.url()}.`);
113-
}
109+
appendWaitForResult(response, result);
114110
} finally {
115111
void Promise.allSettled(args.map(arg => arg.dispose()));
116112
}

0 commit comments

Comments
 (0)