From db9642fac11f2a5ed6f968d57d899dfb39bd5912 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Mon, 6 Oct 2025 07:16:46 +0000 Subject: [PATCH 1/2] feat: Improve terminal reuse logic to prevent proliferation This change improves the terminal reuse logic to prevent the creation of unnecessary terminals. The previous implementation was too strict and would create new terminals even when idle ones were available. The new logic introduces a fallback mechanism to reuse idle terminals even if their current working directory has drifted. When a terminal with a drifted CWD is reused, a `cd` command is prepended to the command being executed to ensure it runs in the correct directory. This change also includes: - Updates to the `RooTerminal` interface and `BaseTerminal` class to support the new logic. - Comprehensive unit tests to validate the new terminal reuse behavior. --- src/integrations/terminal/BaseTerminal.ts | 1 + src/integrations/terminal/Terminal.ts | 12 +- src/integrations/terminal/TerminalRegistry.ts | 10 ++ .../terminal/__tests__/Terminal.spec.ts | 103 ++++++++++++++++++ .../__tests__/TerminalRegistry.spec.ts | 79 ++++++++++++++ src/integrations/terminal/types.ts | 1 + 6 files changed, 204 insertions(+), 2 deletions(-) create mode 100644 src/integrations/terminal/__tests__/Terminal.spec.ts diff --git a/src/integrations/terminal/BaseTerminal.ts b/src/integrations/terminal/BaseTerminal.ts index a79d417b0..a8560303b 100644 --- a/src/integrations/terminal/BaseTerminal.ts +++ b/src/integrations/terminal/BaseTerminal.ts @@ -20,6 +20,7 @@ export abstract class BaseTerminal implements RooTerminal { protected streamClosed: boolean public taskId?: string + public requestedCwd?: string public process?: RooTerminalProcess public completedProcesses: RooTerminalProcess[] = [] diff --git a/src/integrations/terminal/Terminal.ts b/src/integrations/terminal/Terminal.ts index 8bf2072f3..fc0da38fd 100644 --- a/src/integrations/terminal/Terminal.ts +++ b/src/integrations/terminal/Terminal.ts @@ -1,6 +1,7 @@ import * as vscode from "vscode" import pWaitFor from "p-wait-for" +import { arePathsEqual } from "../../utils/path" import type { RooTerminalCallbacks, RooTerminalProcessResultPromise } from "./types" import { BaseTerminal } from "./BaseTerminal" import { TerminalProcess } from "./TerminalProcess" @@ -47,7 +48,14 @@ export class Terminal extends BaseTerminal { this.busy = true const process = new TerminalProcess(this) - process.command = command + const currentCwd = this.getCurrentWorkingDirectory() + let commandToRun = command + + if (this.requestedCwd && !arePathsEqual(this.requestedCwd, currentCwd)) { + // Wrap path in quotes to handle spaces + commandToRun = `cd "${this.requestedCwd}" && ${command}` + } + process.command = commandToRun this.process = process // Set up event handlers from callbacks before starting process. @@ -76,7 +84,7 @@ export class Terminal extends BaseTerminal { ShellIntegrationManager.zshCleanupTmpDir(this.id) // Run the command in the terminal - process.run(command) + process.run(commandToRun) }) .catch(() => { console.log(`[Terminal ${this.id}] Shell integration not available. Command execution aborted.`) diff --git a/src/integrations/terminal/TerminalRegistry.ts b/src/integrations/terminal/TerminalRegistry.ts index 6e0531beb..0e6bf7123 100644 --- a/src/integrations/terminal/TerminalRegistry.ts +++ b/src/integrations/terminal/TerminalRegistry.ts @@ -192,12 +192,22 @@ export class TerminalRegistry { }) } + // Third priority: find any idle terminal with the same provider, + // preferably one with the same task ID. + if (!terminal) { + const idleTerminals = terminals.filter((t) => !t.busy && t.provider === provider) + if (idleTerminals.length > 0) { + terminal = idleTerminals.find((t) => t.taskId === taskId) ?? idleTerminals[0] + } + } + // If no suitable terminal found, create a new one. if (!terminal) { terminal = this.createTerminal(cwd, provider) } terminal.taskId = taskId + terminal.requestedCwd = cwd return terminal } diff --git a/src/integrations/terminal/__tests__/Terminal.spec.ts b/src/integrations/terminal/__tests__/Terminal.spec.ts new file mode 100644 index 000000000..c0da79bff --- /dev/null +++ b/src/integrations/terminal/__tests__/Terminal.spec.ts @@ -0,0 +1,103 @@ +// npx vitest run src/integrations/terminal/__tests__/Terminal.spec.ts + +import * as vscode from "vscode" +import { Terminal } from "../Terminal" +import { TerminalProcess } from "../TerminalProcess" +import { vi } from "vitest" + +// Mock dependencies +vi.mock("vscode", () => ({ + window: { + createTerminal: vi.fn().mockReturnValue({ + shellIntegration: { + cwd: { fsPath: "/initial/cwd" }, + }, + sendText: vi.fn(), + exitStatus: undefined, + }), + }, + ThemeIcon: vi.fn(), +})) + +vi.mock("p-wait-for", () => ({ + default: vi.fn().mockResolvedValue(true), +})) + +vi.mock("../TerminalProcess") +vi.mock("../../../utils/path", () => ({ + arePathsEqual: vi.fn((a, b) => a === b), +})) + +describe("Terminal", () => { + describe("runCommand", () => { + let terminal: Terminal + let mockRun: any + let mockProcess: any + + beforeEach(() => { + terminal = new Terminal(1, undefined, "/initial/cwd") + mockRun = vi.fn() + mockProcess = { + run: mockRun, + on: vi.fn(), + once: vi.fn(), + emit: vi.fn(), + } + vi.mocked(TerminalProcess).mockImplementation((): any => { + return mockProcess + }) + }) + + afterEach(() => { + vi.clearAllMocks() + }) + + it("should prepend cd command if requestedCwd differs from currentCwd", () => { + terminal.requestedCwd = "/requested/cwd" + vi.spyOn(terminal, "getCurrentWorkingDirectory").mockReturnValue("/initial/cwd") + + const callbacks = { + onLine: vi.fn(), + onCompleted: vi.fn(), + onShellExecutionStarted: vi.fn(), + onShellExecutionComplete: vi.fn(), + } + + terminal.runCommand("ls", callbacks) + + expect(mockProcess.command).toBe('cd "/requested/cwd" && ls') + expect(mockRun).toHaveBeenCalledWith('cd "/requested/cwd" && ls') + }) + + it("should not prepend cd command if requestedCwd is the same as currentCwd", () => { + terminal.requestedCwd = "/initial/cwd" + vi.spyOn(terminal, "getCurrentWorkingDirectory").mockReturnValue("/initial/cwd") + const callbacks = { + onLine: vi.fn(), + onCompleted: vi.fn(), + onShellExecutionStarted: vi.fn(), + onShellExecutionComplete: vi.fn(), + } + + terminal.runCommand("ls", callbacks) + + expect(mockProcess.command).toBe("ls") + expect(mockRun).toHaveBeenCalledWith("ls") + }) + + it("should not prepend cd command if requestedCwd is not set", () => { + vi.spyOn(terminal, "getCurrentWorkingDirectory").mockReturnValue("/initial/cwd") + const callbacks = { + onLine: vi.fn(), + onCompleted: vi.fn(), + onShellExecutionStarted: vi.fn(), + onShellExecutionComplete: vi.fn(), + } + + terminal.runCommand("ls", callbacks) + + expect(mockProcess.command).toBe("ls") + expect(mockRun).toHaveBeenCalledWith("ls") + }) + }) +}) \ No newline at end of file diff --git a/src/integrations/terminal/__tests__/TerminalRegistry.spec.ts b/src/integrations/terminal/__tests__/TerminalRegistry.spec.ts index d3912caf4..ed7420d35 100644 --- a/src/integrations/terminal/__tests__/TerminalRegistry.spec.ts +++ b/src/integrations/terminal/__tests__/TerminalRegistry.spec.ts @@ -3,9 +3,14 @@ import * as vscode from "vscode" import { Terminal } from "../Terminal" import { TerminalRegistry } from "../TerminalRegistry" +import { arePathsEqual } from "../../../utils/path" const PAGER = process.platform === "win32" ? "" : "cat" +vi.mock("../../../utils/path", () => ({ + arePathsEqual: vi.fn((a, b) => a === b), +})) + vi.mock("execa", () => ({ execa: vi.fn(), })) @@ -119,4 +124,78 @@ describe("TerminalRegistry", () => { } }) }) + + describe("getOrCreateTerminal", () => { + let createTerminalSpy: any + + beforeEach(() => { + // Reset terminals before each test + ;(TerminalRegistry as any).terminals = [] + createTerminalSpy = vi.spyOn(TerminalRegistry, "createTerminal") + }) + + afterEach(() => { + createTerminalSpy.mockRestore() + }) + + it("should create a new terminal if none exist", async () => { + await TerminalRegistry.getOrCreateTerminal("/test/path", "task1") + expect(createTerminalSpy).toHaveBeenCalledWith("/test/path", "vscode") + }) + + it("should reuse a terminal with the same task ID and cwd", async () => { + const existingTerminal = new Terminal(1, undefined, "/test/path") + existingTerminal.taskId = "task1" + ;(TerminalRegistry as any).terminals.push(existingTerminal) + + const terminal = await TerminalRegistry.getOrCreateTerminal("/test/path", "task1") + expect(terminal).toBe(existingTerminal) + expect(createTerminalSpy).not.toHaveBeenCalled() + }) + + it("should reuse an idle terminal with the same cwd", async () => { + const existingTerminal = new Terminal(1, undefined, "/test/path") + ;(TerminalRegistry as any).terminals.push(existingTerminal) + + const terminal = await TerminalRegistry.getOrCreateTerminal("/test/path", "task2") + expect(terminal).toBe(existingTerminal) + expect(createTerminalSpy).not.toHaveBeenCalled() + }) + + it("should reuse an idle terminal with a different cwd if no better option is available", async () => { + const existingTerminal = new Terminal(1, undefined, "/other/path") + ;(TerminalRegistry as any).terminals.push(existingTerminal) + + const terminal = await TerminalRegistry.getOrCreateTerminal("/test/path", "task1") + expect(terminal).toBe(existingTerminal) + expect(createTerminalSpy).not.toHaveBeenCalled() + expect(terminal.requestedCwd).toBe("/test/path") + }) + + it("should prioritize reusing a terminal with the same task ID when cwd has drifted", async () => { + const terminal1 = new Terminal(1, undefined, "/other/path1") + terminal1.taskId = "task1" + const terminal2 = new Terminal(2, undefined, "/other/path2") + terminal2.taskId = "task2" + ;(TerminalRegistry as any).terminals.push(terminal1, terminal2) + + const terminal = await TerminalRegistry.getOrCreateTerminal("/test/path", "task1") + expect(terminal).toBe(terminal1) + expect(createTerminalSpy).not.toHaveBeenCalled() + }) + + it("should create a new terminal if all existing terminals are busy", async () => { + const existingTerminal = new Terminal(1, undefined, "/test/path") + existingTerminal.busy = true + ;(TerminalRegistry as any).terminals.push(existingTerminal) + + await TerminalRegistry.getOrCreateTerminal("/test/path", "task1") + expect(createTerminalSpy).toHaveBeenCalledWith("/test/path", "vscode") + }) + + it("should set the requestedCwd on the returned terminal", async () => { + const terminal = await TerminalRegistry.getOrCreateTerminal("/test/path", "task1") + expect(terminal.requestedCwd).toBe("/test/path") + }) + }) }) diff --git a/src/integrations/terminal/types.ts b/src/integrations/terminal/types.ts index 65d521ba6..bd85017a8 100644 --- a/src/integrations/terminal/types.ts +++ b/src/integrations/terminal/types.ts @@ -9,6 +9,7 @@ export interface RooTerminal { running: boolean taskId?: string process?: RooTerminalProcess + requestedCwd?: string getCurrentWorkingDirectory(): string isClosed: () => boolean runCommand: (command: string, callbacks: RooTerminalCallbacks) => RooTerminalProcessResultPromise From f9325346c54d0c2b25fcb5febe354496ab3d00be Mon Sep 17 00:00:00 2001 From: daniel-lxs Date: Thu, 9 Oct 2025 13:01:34 -0500 Subject: [PATCH 2/2] fix: Make terminal tests async to properly wait for command execution --- .../terminal/__tests__/Terminal.spec.ts | 29 ++++++++++++------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/src/integrations/terminal/__tests__/Terminal.spec.ts b/src/integrations/terminal/__tests__/Terminal.spec.ts index c0da79bff..a2cbc3a0e 100644 --- a/src/integrations/terminal/__tests__/Terminal.spec.ts +++ b/src/integrations/terminal/__tests__/Terminal.spec.ts @@ -52,7 +52,7 @@ describe("Terminal", () => { vi.clearAllMocks() }) - it("should prepend cd command if requestedCwd differs from currentCwd", () => { + it("should prepend cd command if requestedCwd differs from currentCwd", async () => { terminal.requestedCwd = "/requested/cwd" vi.spyOn(terminal, "getCurrentWorkingDirectory").mockReturnValue("/initial/cwd") @@ -65,11 +65,14 @@ describe("Terminal", () => { terminal.runCommand("ls", callbacks) - expect(mockProcess.command).toBe('cd "/requested/cwd" && ls') - expect(mockRun).toHaveBeenCalledWith('cd "/requested/cwd" && ls') + // Wait for the async operation to complete + await vi.waitFor(() => { + expect(mockProcess.command).toBe('cd "/requested/cwd" && ls') + expect(mockRun).toHaveBeenCalledWith('cd "/requested/cwd" && ls') + }) }) - it("should not prepend cd command if requestedCwd is the same as currentCwd", () => { + it("should not prepend cd command if requestedCwd is the same as currentCwd", async () => { terminal.requestedCwd = "/initial/cwd" vi.spyOn(terminal, "getCurrentWorkingDirectory").mockReturnValue("/initial/cwd") const callbacks = { @@ -81,11 +84,14 @@ describe("Terminal", () => { terminal.runCommand("ls", callbacks) - expect(mockProcess.command).toBe("ls") - expect(mockRun).toHaveBeenCalledWith("ls") + // Wait for the async operation to complete + await vi.waitFor(() => { + expect(mockProcess.command).toBe("ls") + expect(mockRun).toHaveBeenCalledWith("ls") + }) }) - it("should not prepend cd command if requestedCwd is not set", () => { + it("should not prepend cd command if requestedCwd is not set", async () => { vi.spyOn(terminal, "getCurrentWorkingDirectory").mockReturnValue("/initial/cwd") const callbacks = { onLine: vi.fn(), @@ -96,8 +102,11 @@ describe("Terminal", () => { terminal.runCommand("ls", callbacks) - expect(mockProcess.command).toBe("ls") - expect(mockRun).toHaveBeenCalledWith("ls") + // Wait for the async operation to complete + await vi.waitFor(() => { + expect(mockProcess.command).toBe("ls") + expect(mockRun).toHaveBeenCalledWith("ls") + }) }) }) -}) \ No newline at end of file +})