Skip to content

feat: fix glow for CDP page-based tool calls#376

Open
shadowfax92 wants to merge 2 commits intomainfrom
fix/glow-codex
Open

feat: fix glow for CDP page-based tool calls#376
shadowfax92 wants to merge 2 commits intomainfrom
fix/glow-codex

Conversation

@shadowfax92
Copy link
Contributor

Summary

  • Restore sidepanel glow after CDP page-based tool migration by enriching streamed tool inputs with tabId when tools provide page/pageId.
  • Add a server-side page->tab resolution path in Browser and wire it into GeminiAgent tool-call streaming updates.
  • Harden sidepanel glow lookup with a lightweight pageId -> tabId cache and add unit coverage for enrichment logic.

Design

The fix keeps the new page-first CDP tool contract intact and adds compatibility metadata at stream time. Before each tool execution, the server resolves pageId to tabId from authoritative Browser state and emits an updated tool-input-available event for the same toolCallId, allowing AI SDK to update the existing tool part in-place. The extension glow hook then reliably receives tabId while still supporting page-centric workflows.

Test plan

  • bun --env-file=.env.development test apps/server/tests/tools/framework.test.ts
  • bun run --filter @browseros/server typecheck
  • bunx biome check apps/agent/entrypoints/sidepanel/index/useNotifyActiveTab.tsx apps/server/src/tools/framework.ts apps/server/src/agent/gemini-agent.ts apps/server/src/agent/session.ts apps/server/src/api/services/chat-service.ts apps/server/src/browser/browser.ts apps/server/tests/tools/framework.test.ts
  • bun run --filter @browseros/agent typecheck (fails in this environment due Node heap OOM; rerun with higher memory in CI/local)

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 27, 2026

Greptile Summary

This PR restores sidepanel glow functionality after the migration to CDP page-based tool calls by enriching streamed tool inputs with tabId metadata. The implementation adds dual-path enrichment to support both the legacy GeminiAgent and new AiSdkAgent architectures:

Key Changes:

  • Added Browser.resolvePageIdToTabId() method for authoritative page→tab resolution
  • Created enrichToolInputWithTabId() helper that injects tabId when tools provide page/pageId
  • Integrated enrichment into GeminiAgent's tool execution loop (writes updated inputs to UI stream)
  • Added stream transform in ChatV2Service to enrich tool-input-available chunks
  • Enhanced client-side useNotifyActiveTab hook with page→tab cache fallback
  • Added comprehensive unit tests for enrichment logic

Design Rationale:
The fix preserves the page-first CDP tool contract while adding compatibility metadata at stream time. Before each tool execution, the server resolves pageId to tabId and emits enriched tool inputs, allowing the extension glow system to reliably locate tabs while maintaining page-centric workflows.

Issues Found:

  • Missing error handling for CDP disconnection in enrichment paths could break streaming
  • Client-side cache in useNotifyActiveTab grows unbounded (minor impact)

Confidence Score: 3/5

  • Generally safe but needs error handling for CDP failures
  • The implementation is well-tested and architecturally sound, but lacks defensive error handling in critical enrichment paths. If CDP disconnects during enrichment, the stream transform will throw and break the UI stream. The core logic is correct and preserves the page-based tool contract as intended.
  • Pay close attention to apps/server/src/agent/tool-loop/service.ts (stream transform needs error handling) and apps/server/src/tools/framework.ts (enrichment needs try-catch)

Important Files Changed

Filename Overview
apps/server/src/tools/framework.ts Adds enrichToolInputWithTabId function and getNumberField helper; missing error handling for CDP failures
apps/server/src/agent/tool-loop/glow-enrichment.ts New file with enrichToolInputChunkForGlow function; lacks error handling for Browser failures
apps/server/src/agent/tool-loop/service.ts Pipes UI stream through enrichment transform; async transform without error handling could break stream on CDP disconnect
apps/agent/entrypoints/sidepanel/index/useNotifyActiveTab.tsx Adds client-side page-to-tab cache for fallback lookup; cache grows unbounded but low impact
apps/server/src/agent/gemini-agent.ts Enriches tool inputs before streaming; correctly preserves page-based tool contract, minor error handling gap

Sequence Diagram

sequenceDiagram
    participant Client as Sidepanel UI
    participant Service as ChatV2Service
    participant Agent as AiSdkAgent
    participant Browser as Browser
    participant Hook as useNotifyActiveTab
    
    Client->>Service: User message
    Service->>Agent: createAgentUIStream
    Agent->>Agent: Process tool call
    Agent->>Agent: Emit tool-input-available(page: 5)
    Agent->>Service: Stream chunk
    Service->>Browser: resolvePageIdToTabId(5)
    Browser->>Browser: listPages()
    Browser-->>Service: tabId: 99
    Service->>Service: enrichToolInputChunkForGlow
    Service->>Client: Stream enriched chunk(page: 5, tabId: 99)
    Client->>Hook: Update with enriched input
    Hook->>Hook: Cache page→tab mapping
    Hook->>Hook: Activate glow on tab 99
    Hook->>Client: chrome.tabs.sendMessage(99)
Loading

Last reviewed commit: bedc904

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

10 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

const enrichedStream = stream.pipeThrough(
new TransformStream<UIMessageChunk, UIMessageChunk>({
transform: async (chunk, controller) => {
const enrichedChunk = await enrichToolInputChunkForGlow(
Copy link
Contributor

Choose a reason for hiding this comment

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

if CDP disconnects, enrichToolInputChunkForGlow will throw and break the stream

Suggested change
const enrichedChunk = await enrichToolInputChunkForGlow(
const enrichedChunk = await enrichToolInputChunkForGlow(
chunk,
this.deps.browser,
).catch(() => chunk)
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/server/src/agent/tool-loop/service.ts
Line: 161

Comment:
if CDP disconnects, `enrichToolInputChunkForGlow` will throw and break the stream

```suggestion
          const enrichedChunk = await enrichToolInputChunkForGlow(
            chunk,
            this.deps.browser,
          ).catch(() => chunk)
```

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

return args
}

const tabId = await browser.resolvePageIdToTabId(pageId)
Copy link
Contributor

Choose a reason for hiding this comment

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

if CDP disconnects, resolvePageIdToTabId will throw during listPages(). wrap in try-catch to gracefully handle

Suggested change
const tabId = await browser.resolvePageIdToTabId(pageId)
const tabId = await browser.resolvePageIdToTabId(pageId).catch(() => undefined)
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/server/src/tools/framework.ts
Line: 65

Comment:
if CDP disconnects, `resolvePageIdToTabId` will throw during `listPages()`. wrap in try-catch to gracefully handle

```suggestion
  const tabId = await browser.resolvePageIdToTabId(pageId).catch(() => undefined)
```

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

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.

1 participant