Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 37 additions & 7 deletions packages/browseros-agent/apps/server/src/browser/browser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,20 @@ export class Browser {
return this.pages.get(pageId)
}

private async resolvePageInfo(pageId: number): Promise<PageInfo | undefined> {
const cached = this.pages.get(pageId)
if (cached) return cached

await this.listPages()
return this.pages.get(pageId)
}

private unknownPageError(pageId: number): Error {
return new Error(
`Unknown page ${pageId}. Use list_pages to see available pages.`,
)
}

async refreshPageInfo(pageId: number): Promise<PageInfo | undefined> {
let info = this.pages.get(pageId)
if (!info) {
Expand Down Expand Up @@ -591,15 +605,31 @@ export class Browser {
}

async closePage(page: number): Promise<void> {
const info = this.pages.get(page)
if (!info)
throw new Error(
`Unknown page ${page}. Use list_pages to see available pages.`,
)
await this.cdp.Browser.closeTab({ tabId: info.tabId })
let info = await this.resolvePageInfo(page)
if (!info) throw this.unknownPageError(page)
const targetIdsToDetach = new Set([info.targetId])

try {
await this.cdp.Browser.closeTab({ targetId: info.targetId })
} catch (error) {
const refreshed = await this.refreshPageInfo(page)
if (
!refreshed ||
(refreshed.targetId === info.targetId && refreshed.tabId === info.tabId)
) {
throw error
}
Comment on lines +616 to +621
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 The retry guard checks both targetId AND tabId to decide whether to retry. This means if only tabId changes (while targetId stays the same) the method retries closeTab with the same targetId that just failed, then surfaces the error from the second attempt rather than the original one. Since closeTab is called by targetId, only a changed targetId can make the retry meaningful. The tabId condition should be dropped.

Suggested change
if (
!refreshed ||
(refreshed.targetId === info.targetId && refreshed.tabId === info.tabId)
) {
throw error
}
if (!refreshed || refreshed.targetId === info.targetId) {
throw error
}
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros-agent/apps/server/src/browser/browser.ts
Line: 616-621

Comment:
The retry guard checks both `targetId` AND `tabId` to decide whether to retry. This means if only `tabId` changes (while `targetId` stays the same) the method retries `closeTab` with the *same* `targetId` that just failed, then surfaces the error from the second attempt rather than the original one. Since `closeTab` is called by `targetId`, only a changed `targetId` can make the retry meaningful. The `tabId` condition should be dropped.

```suggestion
      if (!refreshed || refreshed.targetId === info.targetId) {
        throw error
      }
```

How can I resolve this? If you propose a fix, please make it concise.


info = refreshed
targetIdsToDetach.add(info.targetId)
await this.cdp.Browser.closeTab({ targetId: info.targetId })
}

this.consoleCollector.detach(page)
this.pages.delete(page)
this.sessions.delete(info.targetId)
for (const targetId of targetIdsToDetach) {
this.sessions.delete(targetId)
}
}

// --- Navigation ---
Expand Down
187 changes: 187 additions & 0 deletions packages/browseros-agent/apps/server/tests/browser/browser.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,187 @@
import { describe, it } from 'bun:test'
import assert from 'node:assert'
import type {
CloseTabParams,
GetTabInfoParams,
TabInfo,
} from '@browseros/cdp-protocol/domains/browser'
import type { ProtocolApi } from '@browseros/cdp-protocol/protocol-api'
import type { CdpBackend } from '../../src/browser/backends/types'
import { Browser } from '../../src/browser/browser'

class FakeCdpBackend {
readonly closeTabCalls: CloseTabParams[] = []
private nextSessionId = 1
private readonly protocolSessions = new Map<string, ProtocolApi>()
tabs: TabInfo[]

Browser = {
getTabs: async () => ({ tabs: this.tabs }),
getTabInfo: async (params: GetTabInfoParams) => {
const tab = this.tabs.find(
(t) => t.tabId === params.tabId || t.targetId === params.targetId,
)
if (!tab) throw new Error('Unknown page')
return { tab }
},
closeTab: async (params: CloseTabParams) => {
this.closeTabCalls.push(params)
const index = this.tabs.findIndex(
(t) => t.tabId === params.tabId || t.targetId === params.targetId,
)
if (index === -1) throw new Error('Unknown page')
this.tabs.splice(index, 1)
},
}

Target = {
on: () => () => {},
attachToTarget: async () => {
const sessionId = `session-${this.nextSessionId++}`
this.protocolSessions.set(sessionId, createProtocolSession())
return { sessionId }
},
}

constructor(tabs: TabInfo[]) {
this.tabs = tabs
}

async connect(): Promise<void> {}
async disconnect(): Promise<void> {}
isConnected(): boolean {
return true
}
async getTargets(): Promise<[]> {
return []
}
session(sessionId: string): ProtocolApi {
const session = this.protocolSessions.get(sessionId)
if (!session) throw new Error(`Unknown session ${sessionId}`)
return session
}
onSessionEvent(): () => void {
return () => {}
}
}

function createProtocolSession(): ProtocolApi {
return {
Page: { enable: async () => {} },
DOM: { enable: async () => {} },
Runtime: { enable: async () => {} },
Log: { enable: async () => {} },
Accessibility: { enable: async () => {} },
} as unknown as ProtocolApi
}

function createTab(overrides: Partial<TabInfo>): TabInfo {
return {
tabId: 101,
targetId: 'target-101',
url: 'https://example.com',
title: 'Example',
isActive: false,
isLoading: false,
loadProgress: 1,
isPinned: false,
isHidden: false,
windowId: 1,
index: 0,
...overrides,
}
}

function createBrowser(tabs: TabInfo[]): {
browser: Browser
cdp: FakeCdpBackend
} {
const cdp = new FakeCdpBackend(tabs)
return {
browser: new Browser(cdp as unknown as CdpBackend),
cdp,
}
}

function getSessionCache(browser: Browser): Map<string, string> {
return (browser as unknown as { sessions: Map<string, string> }).sessions
}

async function attachSession(
browser: Browser,
targetId: string,
pageId: number,
): Promise<void> {
await (
browser as unknown as {
attachToPage(targetId: string, pageId: number): Promise<string>
}
).attachToPage(targetId, pageId)
}

describe('Browser', () => {
it('closes listed pages by target ID', async () => {
const { browser, cdp } = createBrowser([
createTab({ tabId: 401, targetId: 'target-401' }),
])

const pages = await browser.listPages()
assert.strictEqual(pages.length, 1)

await browser.closePage(pages[0].pageId)

assert.deepStrictEqual(cdp.closeTabCalls, [{ targetId: 'target-401' }])
assert.strictEqual(cdp.tabs.length, 0)
})

it('refreshes pages before treating closePage page IDs as unknown', async () => {
const { browser, cdp } = createBrowser([
createTab({ tabId: 402, targetId: 'target-402' }),
])

await browser.closePage(1)

assert.deepStrictEqual(cdp.closeTabCalls, [{ targetId: 'target-402' }])
assert.strictEqual(cdp.tabs.length, 0)
})

it('retries closePage when the tab target changes before close', async () => {
const { browser, cdp } = createBrowser([
createTab({ tabId: 403, targetId: 'target-403-old' }),
])

const pages = await browser.listPages()
cdp.tabs[0].targetId = 'target-403-new'

await browser.closePage(pages[0].pageId)

assert.deepStrictEqual(cdp.closeTabCalls, [
{ targetId: 'target-403-old' },
{ targetId: 'target-403-new' },
])
assert.strictEqual(cdp.tabs.length, 0)
})

it('clears stale session cache when closePage retries with a refreshed target ID', async () => {
const { browser, cdp } = createBrowser([
createTab({ tabId: 404, targetId: 'target-404-old' }),
])

const pages = await browser.listPages()
await attachSession(browser, 'target-404-old', pages[0].pageId)
assert.deepStrictEqual(
[...getSessionCache(browser).keys()],
['target-404-old'],
)

cdp.tabs[0].targetId = 'target-404-new'

await browser.closePage(pages[0].pageId)

assert.deepStrictEqual(cdp.closeTabCalls, [
{ targetId: 'target-404-old' },
{ targetId: 'target-404-new' },
])
assert.deepStrictEqual([...getSessionCache(browser).keys()], [])
})
})
Loading