From 91d4b56ec4ddb4521f6bc78bc27ff3dcb4d7c492 Mon Sep 17 00:00:00 2001 From: hannesrudolph Date: Wed, 11 Jun 2025 22:00:47 -0600 Subject: [PATCH] fix: reset terminal busy state after manual commands complete (#4319) - Add terminal.busy = true when shell execution starts - Add terminal.busy = false when shell execution ends for both Roo and non-Roo terminals - Add comprehensive tests for busy flag management - Fixes issue where terminals got stuck in busy state after manual commands --- src/integrations/terminal/TerminalRegistry.ts | 3 + .../__tests__/TerminalRegistry.test.ts | 211 ++++++++++++++++++ 2 files changed, 214 insertions(+) diff --git a/src/integrations/terminal/TerminalRegistry.ts b/src/integrations/terminal/TerminalRegistry.ts index d31368541e..af334611c3 100644 --- a/src/integrations/terminal/TerminalRegistry.ts +++ b/src/integrations/terminal/TerminalRegistry.ts @@ -59,6 +59,7 @@ export class TerminalRegistry { if (terminal) { terminal.setActiveStream(stream) + terminal.busy = true // Mark terminal as busy when shell execution starts } else { console.error( "[onDidStartTerminalShellExecution] Shell execution started, but not from a Roo-registered terminal:", @@ -99,6 +100,7 @@ export class TerminalRegistry { { terminalId: terminal?.id, command: process?.command, exitCode: e.exitCode }, ) + terminal.busy = false return } @@ -113,6 +115,7 @@ export class TerminalRegistry { // Signal completion to any waiting processes. terminal.shellExecutionComplete(exitDetails) + terminal.busy = false // Mark terminal as not busy when shell execution ends }, ) diff --git a/src/integrations/terminal/__tests__/TerminalRegistry.test.ts b/src/integrations/terminal/__tests__/TerminalRegistry.test.ts index 283e5b73c4..d8926c8759 100644 --- a/src/integrations/terminal/__tests__/TerminalRegistry.test.ts +++ b/src/integrations/terminal/__tests__/TerminalRegistry.test.ts @@ -2,21 +2,39 @@ import { Terminal } from "../Terminal" import { TerminalRegistry } from "../TerminalRegistry" +import * as vscode from "vscode" const PAGER = process.platform === "win32" ? "" : "cat" // Mock vscode.window.createTerminal const mockCreateTerminal = jest.fn() +// Event handlers for testing +let mockStartHandler: any = null +let mockEndHandler: any = null + jest.mock("vscode", () => ({ window: { createTerminal: (...args: any[]) => { mockCreateTerminal(...args) return { + name: "Roo Code", exitStatus: undefined, + dispose: jest.fn(), + show: jest.fn(), + hide: jest.fn(), + sendText: jest.fn(), } }, onDidCloseTerminal: jest.fn().mockReturnValue({ dispose: jest.fn() }), + onDidStartTerminalShellExecution: jest.fn().mockImplementation((handler) => { + mockStartHandler = handler + return { dispose: jest.fn() } + }), + onDidEndTerminalShellExecution: jest.fn().mockImplementation((handler) => { + mockEndHandler = handler + return { dispose: jest.fn() } + }), }, ThemeIcon: jest.fn(), })) @@ -28,6 +46,15 @@ jest.mock("execa", () => ({ describe("TerminalRegistry", () => { beforeEach(() => { mockCreateTerminal.mockClear() + + // Reset event handlers + mockStartHandler = null + mockEndHandler = null + + // Clear terminals array for each test + ;(TerminalRegistry as any).terminals = [] + ;(TerminalRegistry as any).nextTerminalId = 1 + ;(TerminalRegistry as any).isInitialized = false }) describe("createTerminal", () => { @@ -113,4 +140,188 @@ describe("TerminalRegistry", () => { } }) }) + + describe("busy flag management", () => { + let mockVsTerminal: any + + beforeEach(() => { + mockVsTerminal = { + name: "Roo Code", + exitStatus: undefined, + dispose: jest.fn(), + show: jest.fn(), + hide: jest.fn(), + sendText: jest.fn(), + } + mockCreateTerminal.mockReturnValue(mockVsTerminal) + }) + + // Helper function to get the created Roo terminal and its underlying VSCode terminal + const createTerminalAndGetVsTerminal = (path: string = "/test/path") => { + const rooTerminal = TerminalRegistry.createTerminal(path, "vscode") + // Get the actual VSCode terminal that was created and stored + const vsTerminal = (rooTerminal as any).terminal + return { rooTerminal, vsTerminal } + } + + it("should initialize terminal with busy = false", () => { + const terminal = TerminalRegistry.createTerminal("/test/path", "vscode") + expect(terminal.busy).toBe(false) + }) + + it("should set busy = true when shell execution starts", () => { + // Initialize the registry to set up event handlers + TerminalRegistry.initialize() + + // Create a terminal and get the actual VSCode terminal + const { rooTerminal, vsTerminal } = createTerminalAndGetVsTerminal() + expect(rooTerminal.busy).toBe(false) + + // Simulate shell execution start event + const execution = { + commandLine: { value: "echo test" }, + read: jest.fn().mockReturnValue({}), + } as any + + if (mockStartHandler) { + mockStartHandler({ + terminal: vsTerminal, + execution, + }) + } + + expect(rooTerminal.busy).toBe(true) + }) + + it("should set busy = false when shell execution ends for Roo terminals", () => { + // Initialize the registry to set up event handlers + TerminalRegistry.initialize() + + // Create a terminal and get the actual VSCode terminal + const { rooTerminal, vsTerminal } = createTerminalAndGetVsTerminal() + rooTerminal.busy = true + + // Set up a mock process to simulate running state + const mockProcess = { + command: "echo test", + isHot: false, + hasUnretrievedOutput: () => false, + } + rooTerminal.process = mockProcess as any + + // Simulate shell execution end event + const execution = { + commandLine: { value: "echo test" }, + } as any + + if (mockEndHandler) { + mockEndHandler({ + terminal: vsTerminal, + execution, + exitCode: 0, + }) + } + + expect(rooTerminal.busy).toBe(false) + }) + + it("should set busy = false when shell execution ends for non-Roo terminals (manual commands)", () => { + // Initialize the registry to set up event handlers + TerminalRegistry.initialize() + + // Simulate a shell execution end event for a terminal not in our registry + const unknownVsTerminal = { + name: "Unknown Terminal", + } + + const execution = { + commandLine: { value: "sleep 30" }, + } as any + + // This should not throw an error and should handle the case gracefully + expect(() => { + if (mockEndHandler) { + mockEndHandler({ + terminal: unknownVsTerminal, + execution, + exitCode: 0, + }) + } + }).not.toThrow() + }) + + it("should handle busy flag reset when terminal process is not running", () => { + // Initialize the registry to set up event handlers + TerminalRegistry.initialize() + + // Create a terminal and get the actual VSCode terminal + const { rooTerminal, vsTerminal } = createTerminalAndGetVsTerminal() + rooTerminal.busy = true + + // Ensure terminal.running returns false (no active process) + Object.defineProperty(rooTerminal, "running", { + get: () => false, + configurable: true, + }) + + // Simulate shell execution end event + const execution = { + commandLine: { value: "echo test" }, + } as any + + if (mockEndHandler) { + mockEndHandler({ + terminal: vsTerminal, + execution, + exitCode: 0, + }) + } + + // Should reset busy flag even when not running + expect(rooTerminal.busy).toBe(false) + }) + + it("should maintain busy state during command execution lifecycle", () => { + // Initialize the registry to set up event handlers + TerminalRegistry.initialize() + + // Create a terminal and get the actual VSCode terminal + const { rooTerminal, vsTerminal } = createTerminalAndGetVsTerminal() + expect(rooTerminal.busy).toBe(false) + + // Start execution + const execution = { + commandLine: { value: "npm test" }, + read: jest.fn().mockReturnValue({}), + } as any + + if (mockStartHandler) { + mockStartHandler({ + terminal: vsTerminal, + execution, + }) + } + + expect(rooTerminal.busy).toBe(true) + + // Set up mock process for running state + const mockProcess = { + command: "npm test", + isHot: true, + hasUnretrievedOutput: () => true, + } + rooTerminal.process = mockProcess as any + + // End execution + if (mockEndHandler) { + mockEndHandler({ + terminal: vsTerminal, + execution, + exitCode: 0, + }) + } + + expect(rooTerminal.busy).toBe(false) + }) + }) })