From d7ed770ed807bfdbca32b46c68fd1870ef0144f9 Mon Sep 17 00:00:00 2001 From: Andy <119136210+AndyMik90@users.noreply.github.com> Date: Sat, 17 Jan 2026 23:16:44 +0100 Subject: [PATCH] fix: enforce 12 terminal limit per project (#1264) * fix 12 max limit terminals per project * fix(tests): address PR review findings and CI failures - Extract duplicated terminal counting logic to helper function (getActiveProjectTerminalCount) to improve maintainability - Add comprehensive rationale comment for 12-terminal limit explaining memory/resource constraints - Add debug logging when terminal limit is reached for better observability - Document that addRestoredTerminal intentionally bypasses limit to preserve user state from previous sessions - Fix macOS test failure: use globalThis instead of window in requestAnimationFrame mock (terminal-copy-paste.test.ts) - Fix Windows test timeouts: add 15000ms timeouts to all subprocess-spawn tests for slower CI environments - Increase PRDetail.integration.test.tsx timeout to 15000ms Co-Authored-By: Claude Opus 4.5 --------- Co-authored-by: Claude Opus 4.5 --- .../integration/subprocess-spawn.test.ts | 16 +++---- .../integration/terminal-copy-paste.test.ts | 3 +- .../__tests__/PRDetail.integration.test.tsx | 2 +- .../src/renderer/stores/terminal-store.ts | 45 ++++++++++++------- 4 files changed, 39 insertions(+), 27 deletions(-) diff --git a/apps/frontend/src/__tests__/integration/subprocess-spawn.test.ts b/apps/frontend/src/__tests__/integration/subprocess-spawn.test.ts index d77041b5..48a4e5f5 100644 --- a/apps/frontend/src/__tests__/integration/subprocess-spawn.test.ts +++ b/apps/frontend/src/__tests__/integration/subprocess-spawn.test.ts @@ -253,7 +253,7 @@ describe('Subprocess Spawn Integration', () => { ]), expect.any(Object) ); - }); + }, 15000); // Increase timeout for Windows CI it('should emit log events from stdout', async () => { const { AgentManager } = await import('../../main/agent'); @@ -269,7 +269,7 @@ describe('Subprocess Spawn Integration', () => { mockStdout.emit('data', Buffer.from('Test log output\n')); expect(logHandler).toHaveBeenCalledWith('task-1', 'Test log output\n'); - }); + }, 15000); // Increase timeout for Windows CI it('should emit log events from stderr', async () => { const { AgentManager } = await import('../../main/agent'); @@ -285,7 +285,7 @@ describe('Subprocess Spawn Integration', () => { mockStderr.emit('data', Buffer.from('Progress: 50%\n')); expect(logHandler).toHaveBeenCalledWith('task-1', 'Progress: 50%\n'); - }); + }, 15000); // Increase timeout for Windows CI it('should emit exit event when process exits', async () => { const { AgentManager } = await import('../../main/agent'); @@ -302,7 +302,7 @@ describe('Subprocess Spawn Integration', () => { // Exit event includes taskId, exit code, and process type expect(exitHandler).toHaveBeenCalledWith('task-1', 0, expect.any(String)); - }); + }, 15000); // Increase timeout for Windows CI it('should emit error event when process errors', async () => { const { AgentManager } = await import('../../main/agent'); @@ -318,7 +318,7 @@ describe('Subprocess Spawn Integration', () => { mockProcess.emit('error', new Error('Spawn failed')); expect(errorHandler).toHaveBeenCalledWith('task-1', 'Spawn failed'); - }); + }, 15000); // Increase timeout for Windows CI it('should kill task and remove from tracking', async () => { const { AgentManager } = await import('../../main/agent'); @@ -339,7 +339,7 @@ describe('Subprocess Spawn Integration', () => { expect(mockProcess.kill).toHaveBeenCalledWith('SIGTERM'); } expect(manager.isRunning('task-1')).toBe(false); - }); + }, 15000); // Increase timeout for Windows CI it('should return false when killing non-existent task', async () => { const { AgentManager } = await import('../../main/agent'); @@ -348,7 +348,7 @@ describe('Subprocess Spawn Integration', () => { const result = manager.killTask('nonexistent'); expect(result).toBe(false); - }); + }, 15000); // Increase timeout for Windows CI it('should track running tasks', async () => { const { AgentManager } = await import('../../main/agent'); @@ -391,7 +391,7 @@ describe('Subprocess Spawn Integration', () => { expect.any(Array), expect.any(Object) ); - }); + }, 15000); // Increase timeout for Windows CI it('should kill all running tasks', async () => { const { AgentManager } = await import('../../main/agent'); diff --git a/apps/frontend/src/__tests__/integration/terminal-copy-paste.test.ts b/apps/frontend/src/__tests__/integration/terminal-copy-paste.test.ts index e8cb7fae..5ac07282 100644 --- a/apps/frontend/src/__tests__/integration/terminal-copy-paste.test.ts +++ b/apps/frontend/src/__tests__/integration/terminal-copy-paste.test.ts @@ -80,7 +80,8 @@ describe('Terminal copy/paste integration', () => { // Mock requestAnimationFrame for xterm.js integration tests global.requestAnimationFrame = vi.fn((callback: FrameRequestCallback) => { // Synchronously execute the callback to avoid timing issues in tests - callback.call(window, 0); + // Use globalThis instead of window for cross-platform compatibility + callback.call(globalThis, 0); return 0; }) as unknown as Mock; diff --git a/apps/frontend/src/renderer/components/github-prs/components/__tests__/PRDetail.integration.test.tsx b/apps/frontend/src/renderer/components/github-prs/components/__tests__/PRDetail.integration.test.tsx index 8f3c7d27..ccd674fc 100644 --- a/apps/frontend/src/renderer/components/github-prs/components/__tests__/PRDetail.integration.test.tsx +++ b/apps/frontend/src/renderer/components/github-prs/components/__tests__/PRDetail.integration.test.tsx @@ -185,7 +185,7 @@ describe('PRDetail - Clean Review State Reset Integration', () => { const postCleanReviewButtonAfterChange = screen.queryByRole('button', { name: /post clean review/i }); expect(postCleanReviewButtonAfterChange).toBeInTheDocument(); unmount(); - }); + }, 15000); // Increased timeout for slower CI environments (Windows) it('should show clean review success message after posting clean review', async () => { const { unmount } = renderPRDetail(); diff --git a/apps/frontend/src/renderer/stores/terminal-store.ts b/apps/frontend/src/renderer/stores/terminal-store.ts index b943b753..eb4b7dd5 100644 --- a/apps/frontend/src/renderer/stores/terminal-store.ts +++ b/apps/frontend/src/renderer/stores/terminal-store.ts @@ -138,16 +138,33 @@ interface TerminalState { getWorktreeCount: () => number; } +/** + * Helper function to count active (non-exited) terminals for a specific project. + * Extracted to avoid duplicating the counting logic across multiple methods. + * + * @param terminals - The array of all terminals + * @param projectPath - The project path to filter by + * @returns The count of active terminals for the given project + */ +function getActiveProjectTerminalCount(terminals: Terminal[], projectPath?: string): number { + return terminals.filter(t => t.status !== 'exited' && t.projectPath === projectPath).length; +} + export const useTerminalStore = create((set, get) => ({ terminals: [], layouts: [], activeTerminalId: null, - maxTerminals: Infinity, // No limit on terminals + // Maximum terminals per project - limited to 12 to prevent excessive memory usage + // from terminal buffers (~1MB each) and PTY process resource exhaustion. + // Each terminal maintains a scrollback buffer and associated xterm.js state. + maxTerminals: 12, hasRestoredSessions: false, addTerminal: (cwd?: string, projectPath?: string) => { const state = get(); - if (state.terminals.length >= state.maxTerminals) { + const activeCount = getActiveProjectTerminalCount(state.terminals, projectPath); + if (activeCount >= state.maxTerminals) { + debugLog(`[TerminalStore] Cannot add terminal: limit of ${state.maxTerminals} reached for project ${projectPath}`); return null; } @@ -180,6 +197,11 @@ export const useTerminalStore = create((set, get) => ({ return existingTerminal; } + // NOTE: Restored terminals are intentionally exempt from the per-project limit. + // This preserves user state from previous sessions - if a user had 12 terminals + // before closing the app, they should get all 12 back on restore. + // The limit only applies to newly created terminals. + const restoredTerminal: Terminal = { id: session.id, title: session.title, @@ -223,10 +245,9 @@ export const useTerminalStore = create((set, get) => ({ return existingTerminal; } - // Use the same logic as canAddTerminal - count only non-exited terminals - // This ensures consistency and doesn't block new terminals when only exited ones exist - const activeTerminalCount = state.terminals.filter(t => t.status !== 'exited').length; - if (activeTerminalCount >= state.maxTerminals) { + const activeCount = getActiveProjectTerminalCount(state.terminals, projectPath); + if (activeCount >= state.maxTerminals) { + debugLog(`[TerminalStore] Cannot add external terminal: limit of ${state.maxTerminals} reached for project ${projectPath}`); return null; } @@ -384,17 +405,7 @@ export const useTerminalStore = create((set, get) => ({ canAddTerminal: (projectPath?: string) => { const state = get(); - // Count only non-exited terminals, optionally filtered by project - const activeTerminals = state.terminals.filter(t => { - // Exclude exited terminals from the count - if (t.status === 'exited') return false; - // If projectPath specified, only count terminals for that project (or legacy without projectPath) - if (projectPath) { - return t.projectPath === projectPath || !t.projectPath; - } - return true; - }); - return activeTerminals.length < state.maxTerminals; + return getActiveProjectTerminalCount(state.terminals, projectPath) < state.maxTerminals; }, getTerminalsForProject: (projectPath: string) => {