diff --git a/.changeset/curly-states-see.md b/.changeset/curly-states-see.md new file mode 100644 index 0000000000..0d1ff62117 --- /dev/null +++ b/.changeset/curly-states-see.md @@ -0,0 +1,5 @@ +--- +"roo-cline": patch +--- + +Close the browsertool properly when a remote browser is configured but a fallback local one is used diff --git a/src/services/browser/BrowserSession.ts b/src/services/browser/BrowserSession.ts index 699b8c7315..7704caba0c 100644 --- a/src/services/browser/BrowserSession.ts +++ b/src/services/browser/BrowserSession.ts @@ -21,6 +21,7 @@ export class BrowserSession { private page?: Page private currentMousePosition?: string private lastConnectionAttempt?: number + private isUsingRemoteBrowser: boolean = false constructor(context: vscode.ExtensionContext) { this.context = context @@ -70,6 +71,7 @@ export class BrowserSession { defaultViewport: this.getViewport(), // headless: false, }) + this.isUsingRemoteBrowser = false } /** @@ -86,6 +88,7 @@ export class BrowserSession { console.log(`Connected to remote browser at ${chromeHostUrl}`) this.context.globalState.update("cachedChromeHostUrl", chromeHostUrl) this.lastConnectionAttempt = Date.now() + this.isUsingRemoteBrowser = true return true } catch (error) { @@ -192,15 +195,12 @@ export class BrowserSession { if (this.browser || this.page) { console.log("closing browser...") - const remoteBrowserEnabled = this.context.globalState.get("remoteBrowserEnabled") as boolean | undefined - if (remoteBrowserEnabled && this.browser) { + if (this.isUsingRemoteBrowser && this.browser) { await this.browser.disconnect().catch(() => {}) } else { await this.browser?.close().catch(() => {}) - this.resetBrowserState() } - - // this.resetBrowserState() + this.resetBrowserState() } return {} } @@ -212,6 +212,7 @@ export class BrowserSession { this.browser = undefined this.page = undefined this.currentMousePosition = undefined + this.isUsingRemoteBrowser = false } async doAction(action: (page: Page) => Promise): Promise { diff --git a/src/services/browser/__tests__/BrowserSession.spec.ts b/src/services/browser/__tests__/BrowserSession.spec.ts new file mode 100644 index 0000000000..0ba43a382c --- /dev/null +++ b/src/services/browser/__tests__/BrowserSession.spec.ts @@ -0,0 +1,234 @@ +// npx vitest services/browser/__tests__/BrowserSession.spec.ts + +import { describe, it, expect, vi, beforeEach } from "vitest" +import { BrowserSession } from "../BrowserSession" +import { discoverChromeHostUrl, tryChromeHostUrl } from "../browserDiscovery" +import { fileExistsAtPath } from "../../../utils/fs" + +// Mock dependencies +vi.mock("vscode", () => ({ + ExtensionContext: vi.fn(), + Uri: { + file: vi.fn((path) => ({ fsPath: path })), + }, +})) + +// Mock puppeteer-core +vi.mock("puppeteer-core", () => { + const mockBrowser = { + newPage: vi.fn().mockResolvedValue({ + goto: vi.fn().mockResolvedValue(undefined), + on: vi.fn(), + off: vi.fn(), + screenshot: vi.fn().mockResolvedValue("mockScreenshotBase64"), + url: vi.fn().mockReturnValue("https://example.com"), + }), + pages: vi.fn().mockResolvedValue([]), + close: vi.fn().mockResolvedValue(undefined), + disconnect: vi.fn().mockResolvedValue(undefined), + } + + return { + Browser: vi.fn(), + Page: vi.fn(), + TimeoutError: class TimeoutError extends Error {}, + launch: vi.fn().mockResolvedValue(mockBrowser), + connect: vi.fn().mockResolvedValue(mockBrowser), + } +}) + +// Mock PCR +vi.mock("puppeteer-chromium-resolver", () => { + return { + default: vi.fn().mockResolvedValue({ + puppeteer: { + launch: vi.fn().mockImplementation(async () => { + const { launch } = await import("puppeteer-core") + return launch() + }), + }, + executablePath: "/mock/path/to/chromium", + }), + } +}) + +// Mock fs +vi.mock("fs/promises", () => ({ + mkdir: vi.fn().mockResolvedValue(undefined), + readFile: vi.fn(), + writeFile: vi.fn(), + access: vi.fn(), +})) + +// Mock fileExistsAtPath +vi.mock("../../../utils/fs", () => ({ + fileExistsAtPath: vi.fn().mockResolvedValue(false), +})) + +// Mock browser discovery functions +vi.mock("../browserDiscovery", () => ({ + discoverChromeHostUrl: vi.fn().mockResolvedValue(null), + tryChromeHostUrl: vi.fn().mockResolvedValue(false), +})) + +// Mock delay +vi.mock("delay", () => ({ + default: vi.fn().mockResolvedValue(undefined), +})) + +// Mock p-wait-for +vi.mock("p-wait-for", () => ({ + default: vi.fn().mockResolvedValue(undefined), +})) + +describe("BrowserSession", () => { + let browserSession: BrowserSession + let mockContext: any + + beforeEach(() => { + vi.clearAllMocks() + + // Set up mock context + mockContext = { + globalState: { + get: vi.fn(), + update: vi.fn(), + }, + globalStorageUri: { + fsPath: "/mock/global/storage/path", + }, + extensionUri: { + fsPath: "/mock/extension/path", + }, + } + + // Create browser session + browserSession = new BrowserSession(mockContext) + }) + + describe("Remote browser disabled", () => { + it("should launch a local browser when remote browser is disabled", async () => { + // Mock context to indicate remote browser is disabled + mockContext.globalState.get.mockImplementation((key: string) => { + if (key === "remoteBrowserEnabled") return false + return undefined + }) + + await browserSession.launchBrowser() + + const puppeteerCore = await import("puppeteer-core") + + // Verify that a local browser was launched + expect(puppeteerCore.launch).toHaveBeenCalled() + + // Verify that remote browser connection was not attempted + expect(discoverChromeHostUrl).not.toHaveBeenCalled() + expect(tryChromeHostUrl).not.toHaveBeenCalled() + + expect((browserSession as any).isUsingRemoteBrowser).toBe(false) + }) + }) + + describe("Remote browser successfully connects", () => { + it("should connect to a remote browser when enabled and connection succeeds", async () => { + // Mock context to indicate remote browser is enabled + mockContext.globalState.get.mockImplementation((key: string) => { + if (key === "remoteBrowserEnabled") return true + if (key === "remoteBrowserHost") return "http://remote-browser:9222" + return undefined + }) + + // Mock successful remote browser connection + vi.mocked(tryChromeHostUrl).mockResolvedValue(true) + + await browserSession.launchBrowser() + + const puppeteerCore = await import("puppeteer-core") + + // Verify that connect was called + expect(puppeteerCore.connect).toHaveBeenCalled() + + // Verify that local browser was not launched + expect(puppeteerCore.launch).not.toHaveBeenCalled() + + expect((browserSession as any).isUsingRemoteBrowser).toBe(true) + }) + }) + + describe("Remote browser enabled but falls back to local", () => { + it("should fall back to local browser when remote connection fails", async () => { + // Mock context to indicate remote browser is enabled + mockContext.globalState.get.mockImplementation((key: string) => { + if (key === "remoteBrowserEnabled") return true + if (key === "remoteBrowserHost") return "http://remote-browser:9222" + return undefined + }) + + // Mock failed remote browser connection + vi.mocked(tryChromeHostUrl).mockResolvedValue(false) + vi.mocked(discoverChromeHostUrl).mockResolvedValue(null) + + await browserSession.launchBrowser() + + // Import puppeteer-core to check if launch was called + const puppeteerCore = await import("puppeteer-core") + + // Verify that local browser was launched as fallback + expect(puppeteerCore.launch).toHaveBeenCalled() + + // Verify that isUsingRemoteBrowser is false + expect((browserSession as any).isUsingRemoteBrowser).toBe(false) + }) + }) + + describe("closeBrowser", () => { + it("should close a local browser properly", async () => { + const puppeteerCore = await import("puppeteer-core") + + // Create a mock browser directly + const mockBrowser = { + newPage: vi.fn().mockResolvedValue({}), + pages: vi.fn().mockResolvedValue([]), + close: vi.fn().mockResolvedValue(undefined), + disconnect: vi.fn().mockResolvedValue(undefined), + } + + // Set browser and page on the session + ;(browserSession as any).browser = mockBrowser + ;(browserSession as any).page = {} + ;(browserSession as any).isUsingRemoteBrowser = false + + await browserSession.closeBrowser() + + // Verify that browser.close was called + expect(mockBrowser.close).toHaveBeenCalled() + expect(mockBrowser.disconnect).not.toHaveBeenCalled() + + // Verify that browser state was reset + expect((browserSession as any).browser).toBeUndefined() + expect((browserSession as any).page).toBeUndefined() + expect((browserSession as any).isUsingRemoteBrowser).toBe(false) + }) + + it("should disconnect from a remote browser properly", async () => { + // Create a mock browser directly + const mockBrowser = { + newPage: vi.fn().mockResolvedValue({}), + pages: vi.fn().mockResolvedValue([]), + close: vi.fn().mockResolvedValue(undefined), + disconnect: vi.fn().mockResolvedValue(undefined), + } + + // Set browser and page on the session + ;(browserSession as any).browser = mockBrowser + ;(browserSession as any).page = {} + ;(browserSession as any).isUsingRemoteBrowser = true + + await browserSession.closeBrowser() + + // Verify that browser.disconnect was called + expect(mockBrowser.disconnect).toHaveBeenCalled() + expect(mockBrowser.close).not.toHaveBeenCalled() + }) + }) +})