From 1bd641d39f58461566e4a3f07fa00f84f34e87ad Mon Sep 17 00:00:00 2001 From: Rudy Mizrahi Celekli <47457359+rudycelekli@users.noreply.github.com> Date: Mon, 5 Oct 2026 14:10:00 -0400 Subject: [PATCH] fix(browser): distinguish duplicate screenshot frame events (#612) * fix(browser): distinguish duplicate screenshot frame events Signed-off-by: Rudy Celekli <47457359+rudycelekli@users.noreply.github.com> * test(browser): preserve unknown with-scope loader bindings Signed-off-by: Rudy Celekli <47457359+rudycelekli@users.noreply.github.com> --------- Signed-off-by: Rudy Celekli <47457359+rudycelekli@users.noreply.github.com> --- src/browser/CdpScreenshot.ts | 26 +++++++++++++++-- .../cdpScreenshotDocumentIdentity.test.ts | 29 +++++++++++++++++++ tests/fixtures/fakeCdpBrowserEvents.ts | 14 ++++++++- tests/fixtures/fakeCdpBrowserTypes.ts | 1 + 4 files changed, 67 insertions(+), 3 deletions(-) create mode 100644 tests/boundary/browser/cdpScreenshotDocumentIdentity.test.ts diff --git a/src/browser/CdpScreenshot.ts b/src/browser/CdpScreenshot.ts index 981b88db06..54c3278da0 100644 --- a/src/browser/CdpScreenshot.ts +++ b/src/browser/CdpScreenshot.ts @@ -56,7 +56,15 @@ export const captureCdpScreenshot = async ( const mainFrame = captureFrames(before, origins, 1).items[0]; if (authorized === undefined || mainFrame === undefined) throw new BrowserObservationError("inspect_web_page", "target_not_allowed"); - const navigation = { changed: false, leftScope: false }; + const initialFrame = recordValue( + recordValue(recordValue(before)?.frameTree)?.frame, + ); + const navigation = { + changed: false, + leftScope: false, + url: beforeUrl, + loaderId: stringValue(initialFrame?.loaderId), + }; const removeListener = context.connection.onEvent((event) => { if (event.sessionId !== context.sessionId) return; observeScreenshotNavigation(event, mainFrame.frame_id, origins, navigation); @@ -125,13 +133,27 @@ const observeScreenshotNavigation = ( event: CdpEvent, mainFrameId: string, origins: ReadonlySet, - state: { changed: boolean; leftScope: boolean }, + state: { + changed: boolean; + leftScope: boolean; + readonly url: string | undefined; + readonly loaderId: string | undefined; + }, ): void => { if (!isMainFrameNavigation(event, mainFrameId)) return; const params = recordValue(event.params); if (params === undefined) return; const frame = event.method === "Page.frameNavigated" ? recordValue(params.frame) : params; + if ( + event.method === "Page.frameNavigated" && + state.loaderId !== undefined && + state.loaderId !== "" && + stringValue(frame?.loaderId) === state.loaderId && + stringValue(frame?.url) === state.url + ) + return; + state.changed = true; if (allowedSanitizedUrl(frame?.url, origins) === undefined) state.leftScope = true; diff --git a/tests/boundary/browser/cdpScreenshotDocumentIdentity.test.ts b/tests/boundary/browser/cdpScreenshotDocumentIdentity.test.ts new file mode 100644 index 0000000000..012ad9663f --- /dev/null +++ b/tests/boundary/browser/cdpScreenshotDocumentIdentity.test.ts @@ -0,0 +1,29 @@ +import { describe, expect, it, onTestFinished } from "vitest"; +import { CdpBrowserProvider } from "../../../src/browser/CdpBrowserProvider.js"; +import { captureWebScreenshotInputSchema } from "../../../src/domain/webScreenshot.js"; +import { startFakeCdpBrowser } from "../../fixtures/fakeCdpBrowser.js"; +describe("screenshot committed document identity", () => { + it.each(["loader-main", "loader-reloaded"])( + "distinguishes %s for the same URL", + async (screenshotDocumentLoader) => { + const browser = await startFakeCdpBrowser({ screenshotDocumentLoader }); + onTestFinished(async () => { + await browser.close(); + }); + const result = await new CdpBrowserProvider().captureScreenshot( + captureWebScreenshotInputSchema.parse({ + cdp_endpoint: browser.endpoint, + allowed_origins: [browser.allowedOrigin], + target_id: "allowed-page", + }), + ); + if (screenshotDocumentLoader === "loader-main") + expect(result.ok).toBe(true); + else + expect(result).toMatchObject({ + ok: false, + error: { reason: "target_changed" }, + }); + }, + ); +}); diff --git a/tests/fixtures/fakeCdpBrowserEvents.ts b/tests/fixtures/fakeCdpBrowserEvents.ts index ca5cfa240f..b72723e6d5 100644 --- a/tests/fixtures/fakeCdpBrowserEvents.ts +++ b/tests/fixtures/fakeCdpBrowserEvents.ts @@ -13,7 +13,7 @@ export const emitEvents = ( emitRuntimeEvents(socket, command, port, options); emitWebMcpEvents(socket, command, port, options); emitSessionTimeline(socket, command, port, options); - emitCaptureNavigation(socket, command, options); + emitCaptureNavigation(socket, command, port, options); }; const emitWebMcpEvents = ( @@ -462,8 +462,20 @@ const emitRuntimeEvents = ( const emitCaptureNavigation = ( socket: WebSocket, command: FakeCdpCommand, + port: number, options: FakeOptions, ): void => { + if ( + command.method === "Page.captureScreenshot" && + options.screenshotDocumentLoader !== undefined + ) + event(socket, "Page.frameNavigated", command.sessionId, { + frame: { + id: "frame-main", + url: `http://127.0.0.1:${String(port)}/app?token=frame-secret`, + loaderId: options.screenshotDocumentLoader, + }, + }); const navigationUrl = command.method === "DOMSnapshot.captureSnapshot" ? options.navigateDuringCaptureUrl diff --git a/tests/fixtures/fakeCdpBrowserTypes.ts b/tests/fixtures/fakeCdpBrowserTypes.ts index fd4f68817b..be62648283 100644 --- a/tests/fixtures/fakeCdpBrowserTypes.ts +++ b/tests/fixtures/fakeCdpBrowserTypes.ts @@ -42,6 +42,7 @@ export interface FakeOptions { readonly navigateDuringObservationUrl?: string; readonly navigateDuringCaptureUrl?: string; readonly navigateDuringScreenshotUrl?: string; + readonly screenshotDocumentLoader?: string; readonly extraCollections?: boolean; readonly foreignSessionEvents?: boolean; readonly redirectToDisallowedOrigin?: boolean;