Skip to content

Focus the extension when receiving bridge commands #7633

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Sep 3, 2025
Merged

Focus the extension when receiving bridge commands #7633

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4ce37c2
Focus the extension when receiving bridge commands
cte Sep 3, 2025
71b210c
Simplify and fix tests
cte Sep 3, 2025
ca86c7b
More progress
cte Sep 3, 2025
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
35 changes: 31 additions & 4 deletions packages/cloud/src/bridge/BaseChannel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,12 @@
import type { Socket } from "socket.io-client"
import * as vscode from "vscode"

import { ExtensionMetadata } from "@roo-code/types"

export interface BaseChannelOptions {
instanceId: string
extensionMetadata: ExtensionMetadata
}

/**
* Abstract base class for communication channels in the bridge system.
Expand All @@ -11,9 +19,11 @@ import type { Socket } from "socket.io-client"
export abstract class BaseChannel<TCommand = unknown, TEventName extends string = string, TEventData = unknown> {
protected socket: Socket | null = null
protected readonly instanceId: string
protected readonly extensionMetadata: ExtensionMetadata

constructor(instanceId: string) {
this.instanceId = instanceId
constructor(options: BaseChannelOptions) {
this.instanceId = options.instanceId
this.extensionMetadata = options.extensionMetadata
}

/**
Expand Down Expand Up @@ -81,9 +91,26 @@ export abstract class BaseChannel<TCommand = unknown, TEventName extends string
}

/**
* Handle incoming commands - must be implemented by subclasses.
* Handle incoming commands - template method that ensures common functionality
* is executed before subclass-specific logic.
*
* This method should be called by subclasses to handle commands.
* It will execute common functionality and then delegate to the abstract
* handleCommandImplementation method.
*/
public async handleCommand(command: TCommand): Promise<void> {
// Common functionality: focus the sidebar
await vscode.commands.executeCommand(`${this.extensionMetadata.name}.SidebarProvider.focus`)
Copy link

@roomote roomote bot Sep 3, 2025

Choose a reason for hiding this comment

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

Good implementation of the template method pattern! The separation between common functionality (focus) and subclass-specific logic is clean.

However, I have two concerns about the focus command execution:

  1. Potential Race Condition: The focus command is executed asynchronously, but we don't wait for confirmation that the focus actually succeeded before proceeding. Could this lead to commands being processed before the UI is ready?

  2. Error Handling: Consider wrapping this in a try-catch block. If the focus command fails (e.g., extension not fully loaded), should we still proceed with command handling?


// Delegate to subclass-specific implementation
await this.handleCommandImplementation(command)
}

/**
* Handle command-specific logic - must be implemented by subclasses.
* This method is called after common functionality has been executed.
*/
public abstract handleCommand(command: TCommand): Promise<void>
protected abstract handleCommandImplementation(command: TCommand): Promise<void>

/**
* Handle connection-specific logic.
Expand Down
117 changes: 65 additions & 52 deletions packages/cloud/src/bridge/BridgeOrchestrator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
type CloudUserInfo,
type ExtensionBridgeCommand,
type TaskBridgeCommand,
type ExtensionMetadata,
ConnectionState,
ExtensionSocketEvents,
TaskSocketEvents,
Expand All @@ -21,6 +22,7 @@ export interface BridgeOrchestratorOptions {
token: string
provider: TaskProviderLike
sessionId?: string
extensionMetadata: ExtensionMetadata
}

/**
Expand All @@ -39,6 +41,7 @@ export class BridgeOrchestrator {
private readonly token: string
private readonly provider: TaskProviderLike
private readonly instanceId: string
private readonly extensionMetadata: ExtensionMetadata

// Components
private socketTransport: SocketTransport
Expand All @@ -61,66 +64,69 @@ export class BridgeOrchestrator {
public static async connectOrDisconnect(
userInfo: CloudUserInfo | null,
remoteControlEnabled: boolean | undefined,
options?: BridgeOrchestratorOptions,
options: BridgeOrchestratorOptions,
): Promise<void> {
const isEnabled = BridgeOrchestrator.isEnabled(userInfo, remoteControlEnabled)
const instance = BridgeOrchestrator.instance
if (BridgeOrchestrator.isEnabled(userInfo, remoteControlEnabled)) {
await BridgeOrchestrator.connect(options)
} else {
await BridgeOrchestrator.disconnect()
}
}

if (isEnabled) {
if (!instance) {
if (!options) {
console.error(
`[BridgeOrchestrator#connectOrDisconnect] Cannot connect: options are required for connection`,
)
return
}
try {
console.log(`[BridgeOrchestrator#connectOrDisconnect] Connecting...`)
BridgeOrchestrator.instance = new BridgeOrchestrator(options)
await BridgeOrchestrator.instance.connect()
} catch (error) {
console.error(
`[BridgeOrchestrator#connectOrDisconnect] connect() failed: ${error instanceof Error ? error.message : String(error)}`,
)
}
} else {
if (
instance.connectionState === ConnectionState.FAILED ||
instance.connectionState === ConnectionState.DISCONNECTED
) {
console.log(
`[BridgeOrchestrator#connectOrDisconnect] Re-connecting... (state: ${instance.connectionState})`,
)
public static async connect(options: BridgeOrchestratorOptions) {
const instance = BridgeOrchestrator.instance

instance.reconnect().catch((error) => {
console.error(
`[BridgeOrchestrator#connectOrDisconnect] reconnect() failed: ${error instanceof Error ? error.message : String(error)}`,
)
})
} else {
console.log(
`[BridgeOrchestrator#connectOrDisconnect] Already connected or connecting (state: ${instance.connectionState})`,
)
}
if (!instance) {
try {
console.log(`[BridgeOrchestrator#connectOrDisconnect] Connecting...`)
BridgeOrchestrator.instance = new BridgeOrchestrator(options)
await BridgeOrchestrator.instance.connect()
} catch (error) {
console.error(
`[BridgeOrchestrator#connectOrDisconnect] connect() failed: ${error instanceof Error ? error.message : String(error)}`,
)
}
} else {
if (instance) {
try {
console.log(
`[BridgeOrchestrator#connectOrDisconnect] Disconnecting... (state: ${instance.connectionState})`,
)
if (
instance.connectionState === ConnectionState.FAILED ||
instance.connectionState === ConnectionState.DISCONNECTED
) {
console.log(
`[BridgeOrchestrator#connectOrDisconnect] Re-connecting... (state: ${instance.connectionState})`,
)

await instance.disconnect()
} catch (error) {
instance.reconnect().catch((error) => {
console.error(
`[BridgeOrchestrator#connectOrDisconnect] disconnect() failed: ${error instanceof Error ? error.message : String(error)}`,
`[BridgeOrchestrator#connectOrDisconnect] reconnect() failed: ${error instanceof Error ? error.message : String(error)}`,
)
} finally {
BridgeOrchestrator.instance = null
}
})
} else {
console.log(`[BridgeOrchestrator#connectOrDisconnect] Already disconnected`)
console.log(
`[BridgeOrchestrator#connectOrDisconnect] Already connected or connecting (state: ${instance.connectionState})`,
)
}
}
}

public static async disconnect() {
const instance = BridgeOrchestrator.instance

if (instance) {
try {
console.log(
`[BridgeOrchestrator#connectOrDisconnect] Disconnecting... (state: ${instance.connectionState})`,
)

await instance.disconnect()
} catch (error) {
console.error(
`[BridgeOrchestrator#connectOrDisconnect] disconnect() failed: ${error instanceof Error ? error.message : String(error)}`,
)
} finally {
BridgeOrchestrator.instance = null
}
} else {
console.log(`[BridgeOrchestrator#connectOrDisconnect] Already disconnected`)
}
}

Expand All @@ -146,6 +152,7 @@ export class BridgeOrchestrator {
this.token = options.token
this.provider = options.provider
this.instanceId = options.sessionId || crypto.randomUUID()
this.extensionMetadata = options.extensionMetadata

this.socketTransport = new SocketTransport({
url: this.socketBridgeUrl,
Expand All @@ -166,8 +173,14 @@ export class BridgeOrchestrator {
onReconnect: () => this.handleReconnect(),
})

this.extensionChannel = new ExtensionChannel(this.instanceId, this.userId, this.provider)
this.taskChannel = new TaskChannel(this.instanceId)
this.extensionChannel = new ExtensionChannel({
instanceId: this.instanceId,
userId: this.userId,
provider: this.provider,
extensionMetadata: this.extensionMetadata,
})

this.taskChannel = new TaskChannel({ instanceId: this.instanceId, extensionMetadata: this.extensionMetadata })
}

private setupSocketListeners() {
Expand Down
19 changes: 13 additions & 6 deletions packages/cloud/src/bridge/ExtensionChannel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,12 @@ import {
HEARTBEAT_INTERVAL_MS,
} from "@roo-code/types"

import { BaseChannel } from "./BaseChannel.js"
import { type BaseChannelOptions, BaseChannel } from "./BaseChannel.js"

interface ExtensionChannelOptions extends BaseChannelOptions {
userId: string
provider: TaskProviderLike
}

/**
* Manages the extension-level communication channel.
Expand All @@ -31,10 +36,11 @@ export class ExtensionChannel extends BaseChannel<
private heartbeatInterval: NodeJS.Timeout | null = null
private eventListeners: Map<RooCodeEventName, (...args: unknown[]) => void> = new Map()

constructor(instanceId: string, userId: string, provider: TaskProviderLike) {
super(instanceId)
this.userId = userId
this.provider = provider
constructor(options: ExtensionChannelOptions) {
super({ instanceId: options.instanceId, extensionMetadata: options.extensionMetadata })

this.userId = options.userId
this.provider = options.provider

this.extensionInstance = {
instanceId: this.instanceId,
Expand All @@ -53,11 +59,12 @@ export class ExtensionChannel extends BaseChannel<
this.setupListeners()
}

public async handleCommand(command: ExtensionBridgeCommand): Promise<void> {
protected async handleCommandImplementation(command: ExtensionBridgeCommand): Promise<void> {
if (command.instanceId !== this.instanceId) {
console.log(`[ExtensionChannel] command -> instance id mismatch | ${this.instanceId}`, {
messageInstanceId: command.instanceId,
})

return
}

Expand Down
11 changes: 7 additions & 4 deletions packages/cloud/src/bridge/TaskChannel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {
TaskSocketEvents,
} from "@roo-code/types"

import { BaseChannel } from "./BaseChannel.js"
import { type BaseChannelOptions, BaseChannel } from "./BaseChannel.js"

type TaskEventListener = {
[K in keyof TaskEvents]: (...args: TaskEvents[K]) => void | Promise<void>
Expand All @@ -26,6 +26,9 @@ type TaskEventMapping = {
createPayload: (task: TaskLike, ...args: any[]) => any // eslint-disable-line @typescript-eslint/no-explicit-any
}

// eslint-disable-next-line @typescript-eslint/no-empty-object-type
interface TaskChannelOptions extends BaseChannelOptions {}

/**
* Manages task-level communication channels.
* Handles task subscriptions, messaging, and task-specific commands.
Expand Down Expand Up @@ -69,11 +72,11 @@ export class TaskChannel extends BaseChannel<
},
] as const

constructor(instanceId: string) {
super(instanceId)
constructor(options: TaskChannelOptions) {
super(options)
}

public async handleCommand(command: TaskBridgeCommand): Promise<void> {
protected async handleCommandImplementation(command: TaskBridgeCommand): Promise<void> {
const task = this.subscribedTasks.get(command.taskId)

if (!task) {
Expand Down
21 changes: 19 additions & 2 deletions packages/cloud/src/bridge/__tests__/ExtensionChannel.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,13 @@ describe("ExtensionChannel", () => {
let extensionChannel: ExtensionChannel
const instanceId = "test-instance-123"
const userId = "test-user-456"
const extensionMetadata = {
name: "roo-code",
publisher: "Roocode",
version: "1.0.0",
outputChannel: "Roo Code",
sha: undefined,
}

// Track registered event listeners
const eventListeners = new Map<keyof TaskProviderEvents, Set<(...args: unknown[]) => unknown>>()
Expand Down Expand Up @@ -80,7 +87,12 @@ describe("ExtensionChannel", () => {
} as unknown as TaskProviderLike

// Create extension channel instance
extensionChannel = new ExtensionChannel(instanceId, userId, mockProvider)
extensionChannel = new ExtensionChannel({
Copy link

@roomote roomote bot Sep 3, 2025

Choose a reason for hiding this comment

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

Good job updating the tests to accommodate the new constructor signatures! However, would it be valuable to add specific tests that verify the focus command is called before command handling? This would help ensure the new behavior is properly tested.

instanceId,
extensionMetadata,
userId,
provider: mockProvider,
})
})

afterEach(() => {
Expand Down Expand Up @@ -155,7 +167,12 @@ describe("ExtensionChannel", () => {

it("should not have duplicate listeners after multiple channel creations", () => {
// Create a second channel with the same provider
const secondChannel = new ExtensionChannel("instance-2", userId, mockProvider)
const secondChannel = new ExtensionChannel({
instanceId: "instance-2",
extensionMetadata,
userId,
provider: mockProvider,
})

// Each event should have exactly 2 listeners (one from each channel)
eventListeners.forEach((listeners) => {
Expand Down
12 changes: 11 additions & 1 deletion packages/cloud/src/bridge/__tests__/TaskChannel.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,13 @@ describe("TaskChannel", () => {
let mockTask: TaskLike
const instanceId = "test-instance-123"
const taskId = "test-task-456"
const extensionMetadata = {
name: "roo-code",
publisher: "Roocode",
version: "1.0.0",
outputChannel: "Roo Code",
sha: undefined,
}

beforeEach(() => {
// Create mock socket
Expand Down Expand Up @@ -75,7 +82,10 @@ describe("TaskChannel", () => {
}

// Create task channel instance
taskChannel = new TaskChannel(instanceId)
taskChannel = new TaskChannel({
instanceId,
extensionMetadata,
})
})

afterEach(() => {
Expand Down
2 changes: 1 addition & 1 deletion packages/types/npm/package.metadata.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@roo-code/types",
"version": "1.69.0",
"version": "1.70.0",
Copy link

@roomote roomote bot Sep 3, 2025

Choose a reason for hiding this comment

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

Is this version bump following semantic versioning correctly? This seems like a minor feature addition (adding focus behavior) rather than a breaking change. Should this perhaps be 1.69.1 instead of 1.70.0?

"description": "TypeScript type definitions for Roo Code.",
"publishConfig": {
"access": "public",
Expand Down
7 changes: 7 additions & 0 deletions packages/types/src/extension.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
export interface ExtensionMetadata {
publisher: string
name: string
version: string
outputChannel: string
sha: string | undefined
}
1 change: 1 addition & 0 deletions packages/types/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ export * from "./cloud.js"
export * from "./codebase-index.js"
export * from "./events.js"
export * from "./experiment.js"
export * from "./extension.js"
export * from "./followup.js"
export * from "./global-settings.js"
export * from "./history.js"
Expand Down
Loading
Loading