From 336336ed252969ffe32ef3f09d0c611de72dbd24 Mon Sep 17 00:00:00 2001 From: noiemany Date: Wed, 30 Sep 2026 13:50:31 +0300 Subject: [PATCH 1/2] fix(review #21): pin auto sessions to one browser, real DPR for maxWidth, GIF follows replaced tab - Multi-browser: an "auto" session is pinned to the browser its first call went to. A browser connecting mid-session no longer takes over its calls (a batch did click in "work" then type in "home" with the same tabId). If the pinned browser disconnects, calls fail with a clear error instead of switching silently; browser_select_browser re-pins. - Screenshot maxWidth at DPR 2: real Chrome reports equal device and CSS viewport widths, so the ratio came out 1 and maxWidth 800 gave 1600 px. Read window.devicePixelRatio, and if the image is still wider than maxWidth, re-capture once scaled by the real image size. - GIF: replaceFrozenTab moves the recording to the new tab id (frames keep coming; the router records on replacedTabId's successor), and status/stop/export resolve the old id without needing the closed tab. Tests: 4 new, all failing on the old code. Co-Authored-By: Claude Opus 5.5 --- extension/handlers/gif.js | 18 ++++++++++--- extension/handlers/tabs.js | 31 ++++++++++++++++----- extension/lib/page-exec.js | 9 ++++++- extension/lib/router.js | 6 +++-- extension/lib/state.js | 4 +++ mcp-server/src/bridge-connections.ts | 27 +++++++++++++++++-- tests/bridge-multibrowser.test.ts | 25 +++++++++++++++++ tests/extension-router.test.ts | 25 +++++++++++++++++ tests/screenshot-cdp.test.ts | 40 +++++++++++++++++++++++++++- 9 files changed, 169 insertions(+), 16 deletions(-) diff --git a/extension/handlers/gif.js b/extension/handlers/gif.js index a376c36..28382f3 100644 --- a/extension/handlers/gif.js +++ b/extension/handlers/gif.js @@ -6,6 +6,7 @@ * server writes the file — the GIF bytes never go to the agent. */ import { resolveTab } from '../lib/page-exec.js'; +import { gifRecordings as recordings, replacedTabs } from '../lib/state.js'; import { encodeGif, drawMarker } from '../lib/gif-encoder.js'; import { handleScreenshot } from './tabs.js'; @@ -19,8 +20,14 @@ export const GIF_FRAME_TOOLS = new Set([ const MAX_FRAMES_CAP = 500; /** Export travels in parts: the daemon's WebSocket frames are capped at 1 MB. */ export const GIF_PART_BYTES = 600_000; -/** tabId -> { frames, width, maxFrames, activate, recording, skipped, startedAt } */ -const recordings = new Map(); +/** recordings (lib/state.js): tabId -> { frames, width, maxFrames, activate, recording, skipped, startedAt } */ + +/** The tab now holding this id's recording (a frozen tab replaced mid-recording hands it on). */ +export function currentTabId(tabId) { + let id = tabId; + for (let i = 0; i < 10 && !recordings.has(id) && replacedTabs.has(id); i++) id = replacedTabs.get(id); + return id; +} export function isRecording(tabId) { const r = recordings.get(tabId); @@ -85,8 +92,11 @@ async function encodeRecording(rec) { } export async function handleGif(params) { - const { tabId, action } = params; - await resolveTab(tabId); + const { action } = params; + // An id replaced mid-recording resolves to its replacement. + const tabId = action === 'start' ? params.tabId : currentTabId(params.tabId); + // Only capturing needs a live tab: stop/status/export/clear work on the frames already taken. + if (action === 'start' || action === 'frame') await resolveTab(tabId); switch (action) { case 'start': { const rec = { diff --git a/extension/handlers/tabs.js b/extension/handlers/tabs.js index 0395170..c68b2e3 100644 --- a/extension/handlers/tabs.js +++ b/extension/handlers/tabs.js @@ -56,6 +56,16 @@ export function imageSize(b64) { * one in its window, so the user's view is never switched, and can downscale * (`scale`) or cap the width (`maxWidth`) to save image tokens. */ +/** Device pixels per CSS pixel: window.devicePixelRatio, else the layout-metrics ratio. */ +async function pageDpr(send, metrics, vv) { + try { + const r = await withTimeout(send('Runtime.evaluate', { expression: 'window.devicePixelRatio', returnByValue: true }), 1500, 'devicePixelRatio'); + const v = Number(r?.result?.value); + if (v > 0 && v < 16) return v; + } catch { /* frozen or restricted page: fall back */ } + return metrics.visualViewport?.clientWidth > 0 ? metrics.visualViewport.clientWidth / vv.clientWidth : 1; +} + async function cdpScreenshot(tabId, { format, quality, scale, maxWidth, fullPage, region }) { return withCdp(tabId, async (send) => { let metrics = await send('Page.getLayoutMetrics'); @@ -78,25 +88,34 @@ async function cdpScreenshot(tabId, { format, quality, scale, maxWidth, fullPage height = Math.max(1, Math.min(region.height, vv.clientHeight - originY)); } let s = Math.min(region ? 4 : 1, Math.max(0.05, scale ?? (region ? 2 : 1))); - // The capture comes out at clip.scale × devicePixelRatio (device pixels): - // the deprecated device-pixel metrics against the CSS ones give the ratio. - const dpr = metrics.visualViewport?.clientWidth > 0 ? metrics.visualViewport.clientWidth / vv.clientWidth : 1; + // The capture comes out at clip.scale × devicePixelRatio (device pixels). + // The layout metrics can't be trusted for that ratio (real Chrome at DPR 2 + // reports equal device and CSS viewport widths), so ask the page. + const dpr = await pageDpr(send, metrics, vv); if (maxWidth && width * s * dpr > maxWidth) s = maxWidth / (width * dpr); - const { data } = await withTimeout(send('Page.captureScreenshot', { + const capture = (clipScale) => withTimeout(send('Page.captureScreenshot', { format, ...(format === 'jpeg' ? { quality } : {}), captureBeyondViewport: !!fullPage && !region, clip: { x: fullPage && !region ? 0 : vv.pageX + originX, y: fullPage && !region ? 0 : vv.pageY + originY, - width, height, scale: s, + width, height, scale: clipScale, }, }), CDP_CAPTURE_TIMEOUT_MS, 'Page.captureScreenshot'); + let { data } = await capture(s); + let size = imageSize(data); + // maxWidth is a promise about the IMAGE: if the ratio was still off, shrink + // by what the real image shows and capture once more. + if (maxWidth && size?.width > maxWidth + 1) { + s = s * (maxWidth / size.width); + ({ data } = await capture(s)); + size = imageSize(data); + } // How image pixels map to the viewport coordinates click/hover/scroll take: // viewportX = origin[0] + imageX / scale (fullPage: page coordinates instead). // The real image size is the ground truth (it includes the device pixel // ratio: an 800 px viewport at DPR 2 is a 1600 px image, scale 2). - const size = imageSize(data); const imgW = size?.width || Math.round(width * s * dpr); const imgH = size?.height || Math.round(height * s * dpr); const frame = { diff --git a/extension/lib/page-exec.js b/extension/lib/page-exec.js index 958840e..ddd0883 100644 --- a/extension/lib/page-exec.js +++ b/extension/lib/page-exec.js @@ -2,7 +2,7 @@ * Page-execution primitives (extracted from background.js): tab resolution, * the locator guard, and safeExec. Everything a handler needs to touch a page. */ -import { fallbackByTab, wedgedTabs, tabLocks, persistSessionState } from './state.js'; +import { fallbackByTab, wedgedTabs, tabLocks, persistSessionState, gifRecordings, replacedTabs } from './state.js'; import { PAGE_DOM_INSTALL, PAGE_DOM_VERSION } from './page-dom.js'; /** @@ -111,6 +111,13 @@ export async function replaceFrozenTab(tab, url, sessionId = null) { tabLocks.lock(fresh.id, owner); persistSessionState(); } + // A GIF recording moves with the tab: frames keep coming, export still works. + const rec = gifRecordings.get(tab.id); + if (rec) { + gifRecordings.delete(tab.id); + gifRecordings.set(fresh.id, rec); + } + replacedTabs.set(tab.id, fresh.id); chrome.tabs.remove(tab.id).catch(() => { /* already gone */ }); return fresh; } diff --git a/extension/lib/router.js b/extension/lib/router.js index 16334e9..f98ded9 100644 --- a/extension/lib/router.js +++ b/extension/lib/router.js @@ -250,8 +250,10 @@ export async function handleMessage(msg) { sendToolResponse(id, result); // GIF recording: capture the page after the action (the reply is already // sent; the tab mutex keeps the next call from racing the capture). - if (GIF_FRAME_TOOLS.has(tool) && isRecording(tabId) && !(result && result.success === false)) { - await recordFrame(tabId, tool, result); + // A frozen tab replaced by this call records on its replacement. + const frameTab = typeof result?.replacedTabId === 'number' ? (result.tabId ?? result.reloaded ?? tabId) : tabId; + if (GIF_FRAME_TOOLS.has(tool) && isRecording(frameTab) && !(result && result.success === false)) { + await recordFrame(frameTab, tool, result); } } catch (err) { sendResponse(id, { success: false, error: err.message || String(err) }); diff --git a/extension/lib/state.js b/extension/lib/state.js index 6e61ed7..9d7c20b 100644 --- a/extension/lib/state.js +++ b/extension/lib/state.js @@ -41,6 +41,10 @@ export const fallbackByTab = new Map(); * Cleared when the tab starts a new navigation or is closed. */ export const wedgedTabs = new Map(); +/** browser_gif recordings: tabId -> recording (here so a replaced frozen tab can hand its recording over). */ +export const gifRecordings = new Map(); +/** Frozen tabs replaced by replaceFrozenTab: old tabId -> new tabId. */ +export const replacedTabs = new Map(); /** * isNew feature: Map of "fingerprints" (role|name) from the * PREVIOUS snapshot. The next snapshot marks any ref whose fingerprint isn't diff --git a/mcp-server/src/bridge-connections.ts b/mcp-server/src/bridge-connections.ts index 1f4c701..757b014 100644 --- a/mcp-server/src/bridge-connections.ts +++ b/mcp-server/src/bridge-connections.ts @@ -36,11 +36,16 @@ export function identityOf(info?: { browserId?: unknown; browserLabel?: unknown * The set of extension connections plus each session's browser choice. * Routing: a session's selected browser, else the default — the most * recently connected live browser (so one browser behaves exactly as before). + * An "auto" session is pinned to the browser its first call went to: tab ids + * belong to one browser, so a browser connecting mid-session (or mid-batch, + * or between a call and its retry) must not take over that session's calls. */ export class ExtensionConnections { private conns = new Set(); /** sessionId -> browserId chosen with browser_select_browser. */ private sessionBrowser = new Map(); + /** sessionId -> browserId an "auto" session was pinned to by its first call. */ + private autoBrowser = new Map(); add(conn: ExtensionConnection): void { this.conns.add(conn); } has(conn: ExtensionConnection): boolean { return this.conns.has(conn); } @@ -70,8 +75,19 @@ export class ExtensionConnections { if (chosen) return chosen; throw new Error(`Selected browser "${want}" is not connected. browser_list_browsers shows the connected ones (browser_select_browser "auto" = default).`); } + const pinned = sessionId ? this.autoBrowser.get(sessionId) : undefined; + if (pinned) { + const same = this.live().find((c) => c.browserId === pinned); + if (same) return same; + // Its browser is gone: moving on to another browser is only safe when + // no other one could be confused with it — never switch silently. + if (this.live().length > 0) { + throw new Error(`This session's browser "${pinned}" disconnected. Its tab ids are not valid in the other connected browser(s): reconnect it, or call browser_select_browser (then browser_tabs list) to continue in another one.`); + } + } const primary = this.primary(); if (!primary) throw new Error('Chrome extension not connected. Make sure the Browser Controller extension is installed and enabled.'); + if (sessionId) this.autoBrowser.set(sessionId, primary.browserId); return primary; } @@ -83,7 +99,7 @@ export class ExtensionConnections { /** browser_list_browsers: every connected extension, with this session's choice. */ list(sessionId?: string): Record { const primary = this.primary(); - const selected = sessionId ? this.sessionBrowser.get(sessionId) : undefined; + const selected = sessionId ? (this.sessionBrowser.get(sessionId) ?? this.autoBrowser.get(sessionId)) : undefined; return { success: true, browsers: this.live().map((c) => ({ @@ -94,6 +110,7 @@ export class ExtensionConnections { ...((selected ? selected === c.browserId : c === primary) ? { selected: true } : {}), })), ...(selected ? { selectedBrowserId: selected } : {}), + ...(sessionId && !this.sessionBrowser.has(sessionId) && selected ? { pinnedAuto: true } : {}), }; } @@ -101,6 +118,8 @@ export class ExtensionConnections { select(sessionId: string | undefined, browserId: unknown): Record { if (!sessionId) throw new Error('Selecting a browser needs a client session (connect through the Browser Controller MCP server).'); const id = typeof browserId === 'string' ? browserId.trim() : ''; + // An explicit choice (or "auto" again) re-pins the session. + this.autoBrowser.delete(sessionId); if (!id || id === 'auto') { this.sessionBrowser.delete(sessionId); return { success: true, selected: 'auto', browserId: this.primary()?.browserId ?? null }; @@ -111,10 +130,14 @@ export class ExtensionConnections { return { success: true, selected: conn.browserId, label: conn.label }; } - releaseSession(sessionId: string): void { this.sessionBrowser.delete(sessionId); } + releaseSession(sessionId: string): void { + this.sessionBrowser.delete(sessionId); + this.autoBrowser.delete(sessionId); + } clear(): void { this.conns.clear(); this.sessionBrowser.clear(); + this.autoBrowser.clear(); } } diff --git a/tests/bridge-multibrowser.test.ts b/tests/bridge-multibrowser.test.ts index 49d198e..0b131b0 100644 --- a/tests/bridge-multibrowser.test.ts +++ b/tests/bridge-multibrowser.test.ts @@ -90,6 +90,31 @@ describe('multi-browser bridge', () => { await expect(bridge.callTool('browser_tabs', { action: 'list' }, 's1')).rejects.toThrow(/not connected/); }); + it('an auto session stays on its browser when another one connects (batch/retry safe)', async () => { + const { bridge, port: p } = await startBridge(); + const work = await fakeBrowser(p, 'work'); + // Step 1 of a batch: click in "work" (the only browser). + expect(await bridge.callTool('browser_click', { tabId: 7 }, 's1')).toEqual({ from: 'work' }); + const home = await fakeBrowser(p, 'home'); // newer: becomes the default + // Step 2 must not land in "home" with the same tabId. + expect(await bridge.callTool('browser_type', { tabId: 7, text: 'x' }, 's1')).toEqual({ from: 'work' }); + expect(work.calls).toEqual(['browser_click', 'browser_type']); + expect(home.calls).toEqual([]); + // A new session takes the default; list shows each session's own browser. + expect(await bridge.callTool('browser_tabs', { action: 'list' }, 's2')).toEqual({ from: 'home' }); + const listed = await (bridge.callTool('browser_list_browsers', {}, 's1') as Promise); + expect(listed).toMatchObject({ selectedBrowserId: 'work', pinnedAuto: true }); + expect(listed.browsers.find((b: any) => b.selected).browserId).toBe('work'); + // Its browser gone: an error, never a silent switch to the other browser. + work.ws.close(); + await new Promise((r) => setTimeout(r, 60)); + await expect(bridge.callTool('browser_type', { tabId: 7, text: 'y' }, 's1')).rejects.toThrow(/"work" disconnected/); + expect(home.calls).toEqual(['browser_tabs']); + // Selecting "auto" again re-pins to the current default. + await bridge.callTool('browser_select_browser', { browserId: 'auto' }, 's1'); + expect(await bridge.callTool('browser_tabs', { action: 'list' }, 's1')).toEqual({ from: 'home' }); + }); + it('releasing a session forgets its browser choice', async () => { const { bridge, port: p } = await startBridge(); await fakeBrowser(p, 'work'); diff --git a/tests/extension-router.test.ts b/tests/extension-router.test.ts index c87fcfc..3e2807c 100644 --- a/tests/extension-router.test.ts +++ b/tests/extension-router.test.ts @@ -132,6 +132,31 @@ describe("extension router (handleMessage)", () => { wedgedTabs.clear(); }); + it('a GIF recording follows a frozen tab to its replacement (frames continue, old id still answers)', async () => { + const { replaceFrozenTab } = await import('../extension/lib/page-exec.js'); + const { gifRecordings, replacedTabs } = await import('../extension/lib/state.js'); + const { handleGif, isRecording } = await import('../extension/handlers/gif.js'); + (globalThis as any).chrome.tabs.create = async () => ({ id: 99 }); + (globalThis as any).chrome.tabs.remove = async () => {}; + const rec = { frames: [{}, {}], width: 800, maxFrames: 300, activate: true, recording: true, skipped: 0, startedAt: Date.now() }; + gifRecordings.set(3, rec); + wedgedTabs.set(3, Date.now()); + const fresh = await replaceFrozenTab({ id: 3, windowId: 1, index: 0, active: true }, null, null); + expect(fresh.id).toBe(99); + expect(gifRecordings.get(99)).toBe(rec); + expect(gifRecordings.has(3)).toBe(false); + expect(isRecording(99)).toBe(true); // the router keeps adding frames on the new tab + // The old id (closed tab) still reaches the recording: no tab lookup for status/stop/export. + const realGet = (globalThis as any).chrome.tabs.get; + (globalThis as any).chrome.tabs.get = async (id: number) => { if (id === 3) throw new Error('No tab with id: 3'); return realGet(id); }; + expect(await handleGif({ tabId: 3, action: 'status' })).toMatchObject({ success: true, recording: true, frames: 2 }); + expect(await handleGif({ tabId: 3, action: 'stop' })).toMatchObject({ success: true, recording: false, frames: 2 }); + (globalThis as any).chrome.tabs.get = realGet; + gifRecordings.clear(); + replacedTabs.clear(); + wedgedTabs.clear(); + }); + it("converts a THROWN handler error into a wire-level error", async () => { // click with neither ref nor selector throws in requireTarget. await handleMessage({ diff --git a/tests/screenshot-cdp.test.ts b/tests/screenshot-cdp.test.ts index d6f8b97..efe8c6e 100644 --- a/tests/screenshot-cdp.test.ts +++ b/tests/screenshot-cdp.test.ts @@ -6,6 +6,10 @@ const updates: Array<[number, unknown]> = []; let captureHangs = false; let dpr = 1; let imageData = 'CDPDATA'; +/** What window.devicePixelRatio reports (null = the page can't answer). */ +let pageDpr: number | null = null; +/** When set, the capture is as big as a real one: clip × scale × this true ratio. */ +let trueDpr: number | null = null; /** A tiny PNG header (signature + IHDR) of the given size, base64. */ function pngOf(w: number, h: number): string { const b = Buffer.alloc(33); @@ -39,7 +43,13 @@ function pngOf(w: number, h: number): string { onDetach: { addListener: () => {} }, sendCommand: async (_t: { tabId: number }, method: string, params: Record = {}) => { cdp.push({ method, params }); - if (method === 'Runtime.evaluate') return { result: { value: 1000 } }; + if (method === 'Runtime.evaluate') { + if (String(params.expression).includes('devicePixelRatio')) { + if (pageDpr === null) throw new Error('no page'); + return { result: { value: pageDpr } }; + } + return { result: { value: 1000 } }; + } if (method === 'Page.getLayoutMetrics') { return { cssVisualViewport: { clientWidth: 1000, clientHeight: 600, pageX: 0, pageY: 50 }, @@ -50,6 +60,10 @@ function pngOf(w: number, h: number): string { if (method === 'Page.captureScreenshot') { const tab = tabs.get(_t.tabId); if (captureHangs || !tab?.active) return new Promise(() => {}); + if (trueDpr !== null) { + const clip = params.clip as { width: number; height: number; scale: number }; + return { data: pngOf(Math.round(clip.width * clip.scale * trueDpr), Math.round(clip.height * clip.scale * trueDpr)) }; + } return { data: imageData }; } return {}; @@ -67,6 +81,8 @@ describe('browser_screenshot over CDP', () => { captureHangs = false; dpr = 1; imageData = 'CDPDATA'; + pageDpr = null; + trueDpr = null; tabs.clear(); tabs.set(1, { id: 1, url: 'https://a.test', windowId: 7, active: true }); tabs.set(2, { id: 2, url: 'https://b.test', windowId: 7, active: false }); @@ -94,6 +110,28 @@ describe('browser_screenshot over CDP', () => { expect(small).toMatchObject({ width: 800, frame: { scale: 0.8 } }); }); + it('maxWidth holds at DPR 2 even when the layout metrics report no ratio (real Chrome)', async () => { + // Real Chrome at DPR 2: device and CSS viewport widths come back equal. + dpr = 1; + trueDpr = 2; + pageDpr = 2; + const res = await handleScreenshot({ tabId: 1, format: 'png', maxWidth: 800 }); + const caps = cdp.filter((c) => c.method === 'Page.captureScreenshot'); + expect(caps).toHaveLength(1); + expect(caps[0].params).toMatchObject({ clip: { scale: 0.4 } }); + expect(res).toMatchObject({ width: 800, height: 480, frame: { scale: 0.8 } }); + }); + + it('maxWidth is enforced from the real image when no ratio source is right', async () => { + dpr = 1; + trueDpr = 2; // the page can't tell (pageDpr null) and the metrics say 1 + const res = await handleScreenshot({ tabId: 1, format: 'png', maxWidth: 800 }); + const caps = cdp.filter((c) => c.method === 'Page.captureScreenshot'); + expect(caps).toHaveLength(2); // one re-capture, scaled by what the image showed + expect(caps[1].params).toMatchObject({ clip: { scale: 0.4 } }); + expect(res).toMatchObject({ width: 800, frame: { scale: 0.8 } }); + }); + it('fullPage clips the whole content', async () => { await handleScreenshot({ tabId: 1, format: 'png', fullPage: true }); const cap = cdp.find((c) => c.method === 'Page.captureScreenshot')!; From ac286a25507f6b5eba943852c082acb11688db25 Mon Sep 17 00:00:00 2001 From: noiemany Date: Wed, 30 Sep 2026 14:03:22 +0300 Subject: [PATCH 2/2] fix(review #22): GIF alias under the replacement's lock, keep pin on failed select, frame on recovery navigate - Router: a browser_gif call naming a replaced tab is rewritten to the replacement id BEFORE the lock check and per-tab queue, so another session can't stop/clear/export/capture a locked replacement via the old id. - browser_select_browser: a failed selection no longer drops the auto pin (the next call used to run in the newest browser with the old browser's tab ids). - Frozen-tab recovery navigate (mutex-bypass path) records a frame of the replacement tab, queued on its mutex. Tests: 3 new, failing on the old code. Co-Authored-By: Claude Opus 5.5 --- extension/lib/router.js | 22 ++++++++++-- mcp-server/src/bridge-connections.ts | 6 ++-- tests/bridge-multibrowser.test.ts | 3 ++ tests/extension-router.test.ts | 50 ++++++++++++++++++++++++++++ 4 files changed, 77 insertions(+), 4 deletions(-) diff --git a/extension/lib/router.js b/extension/lib/router.js index f98ded9..20be733 100644 --- a/extension/lib/router.js +++ b/extension/lib/router.js @@ -15,7 +15,7 @@ import { handleTabs, handleConsole, handleNetwork, handleScreenshot, handleResiz import { handleRunAction, handleUploadFile } from '../handlers/cdp.js'; import { handleIntercept } from '../handlers/intercept.js'; import { handleObserve, handleAct } from '../handlers/agent-api.js'; -import { handleGif, isRecording, recordFrame, GIF_FRAME_TOOLS } from '../handlers/gif.js'; +import { handleGif, isRecording, recordFrame, GIF_FRAME_TOOLS, currentTabId } from '../handlers/gif.js'; // sessionId arrives as a first-class top-level field on the WS message (audit // M1) — the daemon no longer injects it into params. We read it here so the @@ -113,6 +113,13 @@ function extractTabId(_tool, params) { return typeof params.tabId === "number" ? params.tabId : null; } +/** The new tab id when this call replaced a frozen tab (navigate/reload), else null. */ +function replacementOf(result) { + if (typeof result?.replacedTabId !== 'number') return null; + const fresh = result.tabId ?? result.reloaded; + return typeof fresh === 'number' ? fresh : null; +} + export async function handleMessage(msg) { // Control messages (non-tool) from the daemon. These carry a `type` and no // `tool`; handle them here before the tool-dispatch path assumes a tool call. @@ -170,6 +177,11 @@ export async function handleMessage(msg) { if (tool === "browser_navigate" && typeof p.tabId !== "number") { p.tabId = (await getActiveTab()).id; } + // A GIF call naming a replaced frozen tab targets its replacement, so the + // lock check and the per-tab queue below apply to the tab it really touches. + if (tool === 'browser_gif' && p.action !== 'start' && typeof p.tabId === 'number') { + p.tabId = currentTabId(p.tabId); + } const tabId = extractTabId(tool, p); // A tool call without an id can never be answered: it used to collide in @@ -218,6 +230,12 @@ export async function handleMessage(msg) { controller.signal, ); sendToolResponse(id, result); + // Recovery navigate replaced a frozen tab: record the new page, queued + // on the replacement's mutex like any other capture. + const fresh = replacementOf(result); + if (fresh != null && GIF_FRAME_TOOLS.has(tool) && isRecording(fresh)) { + await tabMutex.run(fresh, () => recordFrame(fresh, tool, result)); + } } catch (err) { sendResponse(id, { success: false, error: err.message || String(err) }); } finally { @@ -251,7 +269,7 @@ export async function handleMessage(msg) { // GIF recording: capture the page after the action (the reply is already // sent; the tab mutex keeps the next call from racing the capture). // A frozen tab replaced by this call records on its replacement. - const frameTab = typeof result?.replacedTabId === 'number' ? (result.tabId ?? result.reloaded ?? tabId) : tabId; + const frameTab = replacementOf(result) ?? tabId; if (GIF_FRAME_TOOLS.has(tool) && isRecording(frameTab) && !(result && result.success === false)) { await recordFrame(frameTab, tool, result); } diff --git a/mcp-server/src/bridge-connections.ts b/mcp-server/src/bridge-connections.ts index 757b014..9c58ca6 100644 --- a/mcp-server/src/bridge-connections.ts +++ b/mcp-server/src/bridge-connections.ts @@ -118,14 +118,16 @@ export class ExtensionConnections { select(sessionId: string | undefined, browserId: unknown): Record { if (!sessionId) throw new Error('Selecting a browser needs a client session (connect through the Browser Controller MCP server).'); const id = typeof browserId === 'string' ? browserId.trim() : ''; - // An explicit choice (or "auto" again) re-pins the session. - this.autoBrowser.delete(sessionId); if (!id || id === 'auto') { + // "auto" again re-pins the session to the current default. + this.autoBrowser.delete(sessionId); this.sessionBrowser.delete(sessionId); return { success: true, selected: 'auto', browserId: this.primary()?.browserId ?? null }; } const conn = this.live().find((c) => c.browserId === id || c.label === id); + // A failed selection changes nothing: the session keeps its pin. if (!conn) throw new Error(`No connected browser "${id}". browser_list_browsers shows the connected ones.`); + this.autoBrowser.delete(sessionId); this.sessionBrowser.set(sessionId, conn.browserId); return { success: true, selected: conn.browserId, label: conn.label }; } diff --git a/tests/bridge-multibrowser.test.ts b/tests/bridge-multibrowser.test.ts index 0b131b0..937564d 100644 --- a/tests/bridge-multibrowser.test.ts +++ b/tests/bridge-multibrowser.test.ts @@ -105,6 +105,9 @@ describe('multi-browser bridge', () => { const listed = await (bridge.callTool('browser_list_browsers', {}, 's1') as Promise); expect(listed).toMatchObject({ selectedBrowserId: 'work', pinnedAuto: true }); expect(listed.browsers.find((b: any) => b.selected).browserId).toBe('work'); + // A failed selection keeps the pin. + await expect(bridge.callTool('browser_select_browser', { browserId: 'nope' }, 's1')).rejects.toThrow(/No connected browser/); + expect(await bridge.callTool('browser_type', { tabId: 7, text: 'z' }, 's1')).toEqual({ from: 'work' }); // Its browser gone: an error, never a silent switch to the other browser. work.ws.close(); await new Promise((r) => setTimeout(r, 60)); diff --git a/tests/extension-router.test.ts b/tests/extension-router.test.ts index 3e2807c..d204785 100644 --- a/tests/extension-router.test.ts +++ b/tests/extension-router.test.ts @@ -132,6 +132,56 @@ describe("extension router (handleMessage)", () => { wedgedTabs.clear(); }); + it('a GIF call by the old id is checked against the lock of the replacement tab', async () => { + const { gifRecordings, replacedTabs } = await import('../extension/lib/state.js'); + const rec = { frames: [{}], width: 800, maxFrames: 300, activate: true, recording: true, skipped: 0, startedAt: Date.now() }; + gifRecordings.set(99, rec); + replacedTabs.set(3, 99); + tabStore.set(99, { id: 99, windowId: 1, url: 'https://a.test', title: 'A', active: true }); + tabLocks.lock(99, 'session-a'); + // Another session stops "tab 3": it must queue behind 99's lock, not stop the owner's recording. + await handleMessage({ id: 'g1', tool: 'browser_gif', params: { tabId: 3, action: 'stop' }, sessionId: 'session-b' }); + await flush(); + expect(rec.recording).toBe(true); + expect(sent.find((f) => f.id === 'g1' && f.success === true)).toBeUndefined(); + // The owner using the old id reaches its recording. + await handleMessage({ id: 'g2', tool: 'browser_gif', params: { tabId: 3, action: 'stop' }, sessionId: 'session-a' }); + await flush(); + expect(rec.recording).toBe(false); + tabLocks.release(99); + await flush(); + gifRecordings.clear(); + replacedTabs.clear(); + tabStore.delete(99); + }); + + it('recovery navigate of a frozen, recording tab captures a frame of the replacement', async () => { + const { gifRecordings, replacedTabs } = await import('../extension/lib/state.js'); + const rec = { frames: [] as unknown[], width: 800, maxFrames: 300, activate: true, recording: true, skipped: 0, startedAt: Date.now() }; + gifRecordings.set(3, rec); + wedgedTabs.set(3, Date.now()); + tabStore.set(3, { id: 3, windowId: 1, url: 'https://frozen.test', title: 'F', active: true }); + (globalThis as any).chrome.tabs.create = async () => { tabStore.set(99, { id: 99, windowId: 1, url: 'about:blank', title: '', active: true }); return { id: 99 }; }; + (globalThis as any).chrome.tabs.remove = async () => {}; + // The replacement finishes loading right away. + (globalThis as any).chrome.tabs.onUpdated = { + addListener: (fn: (id: number, info: unknown, tab: unknown) => void) => setTimeout(() => fn(99, { status: 'complete' }, tabStore.get(99)), 5), + removeListener: () => {}, + }; + await handleMessage({ id: 'n1', tool: 'browser_navigate', params: { tabId: 3, url: 'https://example.com/x', snapshot: false }, sessionId: 'session-a' }); + for (let i = 0; i < 20 && rec.frames.length + rec.skipped === 0; i++) await flush(); + expect(gifRecordings.get(99)).toBe(rec); + // A capture was attempted on tab 99 (the mock has no real CDP, so it may be skipped). + expect(sent.find((f) => f.id === 'n1')).toMatchObject({ success: true }); + expect(rec.frames.length + rec.skipped).toBe(1); + delete (globalThis as any).chrome.tabs.onUpdated; + gifRecordings.clear(); + replacedTabs.clear(); + wedgedTabs.clear(); + tabStore.delete(3); + tabStore.delete(99); + }); + it('a GIF recording follows a frozen tab to its replacement (frames continue, old id still answers)', async () => { const { replaceFrozenTab } = await import('../extension/lib/page-exec.js'); const { gifRecordings, replacedTabs } = await import('../extension/lib/state.js');