Skip to content

Conversation

@shivammittal274
Copy link
Contributor

No description provided.

@github-actions github-actions bot added the fix label Jan 21, 2026
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 21, 2026

Greptile Summary

This PR migrates the browser_close_window tool from the controller-based (chrome.windows API) implementation to a CDP-based implementation that can bypass beforeunload dialogs.

Key Changes:

  • Removed controller-based closeWindow tool from window-management.ts and registry (reduced tool count from 37 to 36)
  • Added CDP-based closeWindow tool in pages.ts that uses Target.closeTarget to bypass beforeunload dialogs
  • Implemented closeWindowByWindowId() method in McpContext that iterates through all browser targets to find and close those matching the specified windowId
  • Added --remote-debugging-port flag in web-ext.config.ts to enable HTTP-based CDP connections
  • Modified start-all.ts build script to launch agent first, wait for CDP availability, then start server (prevents race condition)

Technical Approach:
The implementation accesses the internal _targetId property via type assertion to query Browser.getWindowForTarget. While functional, this approach is brittle as _targetId is not part of puppeteer's public API.

Infrastructure Improvements:
The build script changes ensure CDP is ready before the server attempts to connect, eliminating startup race conditions.

Confidence Score: 4/5

  • This PR is safe to merge with minor concerns about API stability
  • The core functionality is sound - moving window closing to CDP enables bypassing beforeunload dialogs as intended. The build script improvements properly handle startup ordering. However, the score is 4 instead of 5 due to accessing puppeteer's private _targetId property via type assertion, which could break in future puppeteer updates. The implementation is functional but relies on an internal API that isn't guaranteed to remain stable.
  • Pay attention to apps/server/src/browser/cdp/context.ts:422-424 which accesses private _targetId property - consider refactoring to use public APIs for long-term stability

Important Files Changed

Filename Overview
apps/server/src/browser/cdp/context.ts Added closeWindowByWindowId method that accesses internal _targetId property via type assertion, iterates through all targets to find matching windowId
apps/server/src/tools/cdp-based/pages.ts Added CDP-based browser_close_window tool that bypasses beforeunload dialogs
scripts/build/start-all.ts Added waitForCdp function and reordered startup to launch agent first, wait for CDP availability, then start server

Sequence Diagram

sequenceDiagram
    participant Client as AI Agent/MCP Client
    participant Server as HTTP Server (Hono)
    participant Tool as closeWindow Tool
    participant Context as McpContext
    participant Browser as Puppeteer Browser
    participant CDP as Chrome DevTools Protocol

    Client->>Server: POST /tools/browser_close_window
    Server->>Tool: handler(request, response, context)
    Tool->>Context: closeWindowByWindowId(windowId)
    
    Context->>Browser: targets()
    Browser-->>Context: Target[]
    
    loop For each target
        Context->>Context: Extract _targetId (via type assertion)
        Context->>CDP: createCDPSession()
        CDP-->>Context: CDPSession
        
        Context->>CDP: Browser.getWindowForTarget({targetId})
        CDP-->>Context: {windowId: number}
        
        alt windowId matches
            Context->>CDP: Target.closeTarget({targetId})
            Note over Context: Bypasses beforeunload dialogs
            Context->>Context: closedCount++
        end
        
        Context->>CDP: session.detach()
    end
    
    alt closedCount === 0
        Context-->>Tool: throw Error("No targets found")
        Tool-->>Client: Error response
    else closedCount > 0
        Context-->>Tool: Success
        Tool->>Tool: response.appendResponseLine()
        Tool->>Tool: response.setIncludePages(true)
        Tool-->>Client: Success response with pages
    end
Loading

@claude
Copy link

claude bot commented Jan 21, 2026

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

if (env.BROWSEROS_CDP_PORT) {
chromiumArgs.push(`--browseros-cdp-port=${env.BROWSEROS_CDP_PORT}`)
// Enable HTTP-based CDP so the server can connect
chromiumArgs.push(`--remote-debugging-port=${env.BROWSEROS_CDP_PORT}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not needed. The chromium starts the CDP port on BROWSEROS_CDP_PORT

Copy link
Contributor

Choose a reason for hiding this comment

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

if this is not working, there might be bug on chromium I'll take a look. let me know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes its not working before that, After adding, cdp tools are added.

if (env.BROWSEROS_CDP_PORT) {
chromiumArgs.push(`--browseros-cdp-port=${env.BROWSEROS_CDP_PORT}`)
// Enable HTTP-based CDP so the server can connect
chromiumArgs.push(`--remote-debugging-port=${env.BROWSEROS_CDP_PORT}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

if this is not working, there might be bug on chromium I'll take a look. let me know.

@shadowfax92 shadowfax92 merged commit 8354ad5 into main Jan 23, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants