[WIP] Symphony: Improved Terminal Experience (#160)#486
Open
pedramamini wants to merge 24 commits intomainfrom
Open
[WIP] Symphony: Improved Terminal Experience (#160)#486pedramamini wants to merge 24 commits intomainfrom
pedramamini wants to merge 24 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughAdds a persistent PTY-backed Terminal Tab System: xterm.js-based terminal UI with multi-tab management, IPC spawn support, session persistence/restore for terminal tabs, keyboard shortcuts, theme ANSI colors, store/hooks integration, tests, and documentation updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App as App.tsx
participant MainPanel as MainPanel
participant TabBar as TabBar
participant TerminalView as TerminalView
participant IPC as IPC Handler
participant PTY as PTY Process
User->>App: Ctrl+Shift+` (create terminal tab)
App->>App: useTerminalTabHandlers.createTerminalTab()
App->>MainPanel: render/update with new tab
User->>TabBar: select terminal tab
TabBar->>App: onTerminalTabSelect(tabId)
App->>MainPanel: set inputMode='terminal'
MainPanel->>TerminalView: mount active terminal
TerminalView->>IPC: invoke 'process:spawnTerminalTab'(sessionId,cwd,shell,cols,rows)
IPC->>PTY: spawn PTY
PTY-->>IPC: { pid, success }
IPC-->>TerminalView: pid/result
TerminalView->>TerminalView: update tab PID/state
User->>TerminalView: types input
TerminalView->>IPC: write input to PTY
PTY-->>TerminalView: stdout/stderr data
TerminalView->>User: display output (via XTerminal)
sequenceDiagram
participant Store as Session Store
participant Helpers as terminalTabHelpers
participant Persist as Debounced Persistence
participant Restore as Session Restoration
Store->>Helpers: createTerminalTab(shell,cwd) -> TerminalTab{id,pid:0,state:'idle'}
Store->>Store: addTerminalTab & update unifiedTabOrder
Persist->>Persist: clean terminalTabs (pid=0,state='idle',exitCode=undef)
Persist->>Store: persist cleaned session
Restore->>Store: read persisted session
Restore->>Helpers: ensure terminalTabs non-empty (create default if missing)
Restore->>Store: restore session with terminal metadata and reset runtime state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Adds full xterm.js terminal emulator foundation: - Install @xterm/xterm, addon-fit, addon-webgl, addon-web-links, addon-search, addon-unicode11 - Create XTerminal.tsx: forwardRef component with XTerminalHandle interface - WebGL renderer with canvas fallback on context loss - FitAddon with debounced ResizeObserver (100ms) - SearchAddon, WebLinksAddon, Unicode11Addon - IPC integration via window.maestro.process.onData/write/resize - mapThemeToXterm() helper for Maestro Theme → xterm.js ITheme - Dark/light ANSI color defaults (Phase 11 will add proper theme fields) - scrollback: 10000 lines
Adds TerminalTab interface and extends the unified tab system to support
terminal tabs as a third tab type alongside AI and file tabs.
Changes:
- src/renderer/types/index.ts:
- Add TerminalTab interface (id, name, shellType, pid, cwd, createdAt,
state, exitCode, scrollTop, searchQuery)
- Extend UnifiedTabRef type to 'ai' | 'file' | 'terminal'
- Extend UnifiedTab discriminated union with terminal variant
- Extend ClosedTabEntry with terminal variant
- Add terminalTabs: TerminalTab[] and activeTerminalTabId: string | null
to Session interface
- Add DEPRECATED comments to shellLogs and terminalPid
- src/renderer/utils/tabHelpers.ts:
- Update buildUnifiedTabs to handle terminal refs separately
- Update getRepairedUnifiedTabOrder to track terminal IDs in their own set
- Update ensureInUnifiedTabOrder type parameter to include 'terminal'
- Update NavigateToUnifiedTabResult.type to include 'terminal'
- Update ReopenUnifiedClosedTabResult.tabType to include 'terminal'
- Update navigateToUnifiedTabByIndex with explicit 'file' branch and
terminal placeholder (returns null until Phase 3)
- Update getCurrentUnifiedTabIndex to check activeTerminalTabId first
- Update showUnreadOnly loops to treat terminal tabs like file tabs
- Update restoreClosedUnifiedTab with explicit 'file' branch and
terminal placeholder (returns null until Phase 3)
- Add terminalTabs/activeTerminalTabId to session creation in createSessionFromHistoryEntry
- src/renderer/components/TabBar.tsx:
- Change file tab else branch to explicit else-if (type === 'file')
- Add terminal else branch returning null (implemented in Phase 8)
- Session creation sites (add terminalTabs: [], activeTerminalTabId: null):
- src/renderer/hooks/session/useSessionCrud.ts
- src/renderer/hooks/symphony/useSymphonyContribution.ts
- src/renderer/hooks/wizard/useWizardHandlers.ts
- src/renderer/utils/worktreeSession.ts
No runtime behavior changes — purely type additions and structural
preparation for the terminal tab system.
Creates src/renderer/utils/terminalTabHelpers.ts with 14 pure functions for terminal tab CRUD operations, following the established tabHelpers.ts pattern. Functions: createTerminalTab, getActiveTerminalTab, getTerminalTabDisplayName, getTerminalSessionId, parseTerminalSessionId, addTerminalTab, closeTerminalTab, selectTerminalTab, renameTerminalTab, reorderTerminalTabs, updateTerminalTabState, updateTerminalTabPid, updateTerminalTabCwd, hasRunningTerminalProcess. All functions are pure (take Session, return new Session). closeTerminalTab adds to unifiedClosedTabHistory for Cmd+Shift+T undo support and refuses to close the last terminal tab. selectTerminalTab nulls activeFileTabId to enforce single active non-AI tab invariant.
… for terminal tabs - Initialize sessions with a default terminal tab (using user's defaultShell setting) in all 5 session creation sites: useSessionCrud, useWizardHandlers, useSymphonyContribution, worktreeSession, and tabHelpers buildMergeSession - Add initial terminal tab to unifiedTabOrder at creation time - Add migration in restoreSession: sessions without terminalTabs get a default tab created and added to unifiedTabOrder; activeTerminalTabId undefined defaults to null - Reset terminal tab runtime state (pid, state, exitCode) on restore so dead PTYs aren't referenced after restart - Strip terminal tab runtime state in prepareSessionForPersistence to keep sessions.json clean across restarts
- PtySpawner: bypass stripControlSequences for session IDs containing
-terminal- (xterm.js handles its own rendering; filtering breaks it)
- ProcessConfig: add optional cols/rows fields for PTY dimensions
- PtySpawner: use config.cols/rows in pty.spawn() (defaults 100/30)
- ProcessManager: add spawnTerminalTab() convenience wrapper
- IPC handler: process:spawnTerminalTab with SSH remote support
- Preload API: expose spawnTerminalTab on window.maestro.process
- global.d.ts: add TypeScript type for spawnTerminalTab
- Verified exit listener correctly handles {id}-terminal-{tabId} format
- Create TerminalView.tsx as a memo(forwardRef) component with TerminalViewHandle exposing clearActiveTerminal, focusActiveTerminal, searchActiveTerminal, searchNext, searchPrevious via useImperativeHandle - Maintain Map<string, XTerminalHandle> for per-tab terminal instances - Lazy PTY spawn via useEffect when active tab has pid === 0 - Subscribe to process.onExit() to update tab state when PTY exits - Render all terminal tabs in absolute-positioned stack; only active tab visible - Show exit overlay with exit code when terminal process ends - Export createTabStateChangeHandler/createTabPidChangeHandler factory functions to wire session store updates from MainPanel - Update MainPanel.tsx to render TerminalView when inputMode === 'terminal' instead of TerminalOutput; hide InputArea in terminal mode (xterm.js handles input) - Subscribe to defaultShell and fontSize from settingsStore - Create terminalViewRef for future imperative calls (search, clear, focus)
- Add createTerminalTab, closeTerminalTab, selectTerminalTab, renameTerminalTab actions to tabStore.ts with PTY kill on close - Add selectActiveTerminalTab and selectTerminalTabs selectors to tabStore.ts - Implement terminal tab restore in reopenUnifiedClosedTab (creates fresh PTY) - Add useTerminalTabHandlers hook to useTabHandlers.ts for component integration - Update useInputMode toggleInputMode to activate first terminal tab on switch
- Add TerminalTabItem component to TabBar with Terminal icon, state colors, exit code badge, hover overlay (Rename/Close/Close Others/Close Left/Close Right), and drag-and-drop support - Add terminal tab props to TabBarProps and wire through MainPanel - Show TabBar in both AI and terminal modes (not just AI mode) - Add handleRequestTerminalTabRename wrapper in App.tsx to open rename modal (1-arg interface) - Extend handleRenameTab in useSessionLifecycle to delegate to renameTerminalTabHelper for terminal tabs - Export useTerminalTabHandlers and TerminalTabHandlersReturn from hooks/tabs/index.ts - Add terminal tab handler deps to useMainPanelProps
- Add TERMINAL_SHORTCUTS export to shortcuts.ts with newTerminalTab (Ctrl+Shift+`) and clearTerminal (Cmd+K) - Extend MainPanelHandle with clearActiveTerminal(), focusActiveTerminal(), openTerminalSearch() methods - Add terminalSearchOpen state in MainPanel; wire searchOpen/onSearchClose props to TerminalView - Add handleOpenTerminalTab and mainPanelRef to keyboardHandlerRef context in App.tsx - Guard Ctrl+[A-Z] sequences in terminal mode so xterm.js receives them (Ctrl+C, Ctrl+D, etc.) - Route Cmd+W/Cmd+Shift+[]/Cmd+1-9/Cmd+0 to terminal tab navigation when inputMode === 'terminal' - Override Cmd+K to clear terminal (instead of Quick Actions) when in terminal mode - Add Cmd+F handler to open terminal search overlay when in terminal mode - Add Ctrl+Shift+` handler to create new terminal tab from any mode (switches to terminal mode if needed) - Focus active terminal after Cmd+J mode toggle to terminal mode
- Add TerminalSearchBar component (absolute top-right overlay) with incremental search, previous/next navigation, "No results" indicator, LayerStack registration for Escape handling, and auto-focus on open - Wire TerminalSearchBar into TerminalView: close on tab switch, return focus to terminal on close, delegate search calls to XTerminalHandle - Add useEffect in MainPanel to reset terminalSearchOpen when inputMode leaves 'terminal' mode
- Extend ThemeColors interface with 17 optional ANSI fields (16-color palette + selection) for terminal emulation, maintaining backward compatibility with custom themes - Add curated ANSI palettes to all 16 themes using official palettes where available (Dracula, Nord, Catppuccin, Gruvbox, Solarized, etc.) - Update mapThemeToXterm() in XTerminal.tsx to use per-theme ANSI colors with mode-appropriate defaults (dark/light) as fallback - Update TerminalOutput.tsx ansiConverter to use all 16 ANSI theme colors (expanded from 8 to 16 entries) with theme fallbacks - Fix cssCustomProperties.ts and CustomThemeBuilder.tsx to handle the new optional ANSI fields without type errors
Create TerminalTabRenameModal component and wire it into AppSessionModals.
The modal is shown when double-clicking a terminal tab or using the hover
overlay rename action. It displays the terminal-specific title, helper text
('Leave empty to use the default name'), and pre-fills with the current
custom name. Saving an empty string restores the auto-generated 'Terminal N'
display. Reuses the existing renameTab modal store state and handleRenameTab
lifecycle hook, which already handles terminal tab vs AI tab dispatch.
- TerminalOutput.tsx: remove all inputMode==='terminal' branches; terminal display is now exclusively handled by TerminalView/XTerminal. Remove unused ElapsedTimeDisplay component definition. - TerminalView.tsx: add shared spawnPtyForTab useCallback (used for both initial spawn and Retry); add spawn-failure overlay (AlertCircle + Retry button); write ANSI shell-exit message to xterm buffer on state transition; write dim "Starting terminal..." loading indicator on mount; remove stale eslint-disable-next-line directives. - process.ts: add TODO comment and runtime deprecation warning to process:runCommand (callers should migrate to process:spawnTerminalTab). - useBatchedSessionUpdates.ts: guard shellLogs append behind !(session.terminalTabs?.length) legacy fallback with TODO comment. - TabBar.tsx: add shell-type/cwd tooltip to terminal tab items; widen name truncation limit from 120px to 150px. - New test: terminalTabHelpers.test.ts — 34 unit tests covering all pure helper functions (createTerminalTab, getTerminalTabDisplayName, getTerminalSessionId/parseTerminalSessionId, addTerminalTab, closeTerminalTab, selectTerminalTab, updateTerminalTabState, updateTerminalTabPid, renameTerminalTab, reorderTerminalTabs). - Test fixes: add terminalTabs/activeTerminalTabId to createMockSession in agentStore.test.ts and tabHelpers.test.ts; add process:spawnTerminalTab to process.test.ts expected channels list. - CLAUDE.md: add terminal feature rows to Key Files table.
- Fix migration bug in useSessionRestoration: when unifiedTabOrder is
undefined during terminal tab migration, build full order from all
existing tab types (AI + file + terminal) instead of creating an
incomplete order that omitted AI/file tabs.
- Add 17 new tests for terminal tab persistence:
- useDebouncedPersistence: 6 tests verifying runtime state is stripped
and metadata is preserved when sessions are persisted to disk
- useSessionRestoration: 11 tests verifying tab metadata survives
restore, runtime state resets (pid=0/idle), migration creates default
tabs with correct unifiedTabOrder, and activeTerminalTabId is preserved
- Fix 2 pre-existing test failures (useSessionCrud, useSymphonyContribution)
that had stale expectations: new sessions correctly include both AI and
terminal tab refs in unifiedTabOrder (length 2, not 1).
- Update useSessionRestoration test: 'builds unifiedTabOrder from aiTabs
when missing' now correctly expects both AI and terminal tab refs when
restoring a legacy session without unifiedTabOrder.
Corrects two wrong field names in the spawnTerminalTab SSH handler: - sshResult.config.user → sshResult.config.username - sshResult.config.keyPath → sshResult.config.privateKeyPath These match the actual SshRemoteConfig interface in shared/types.ts. The bug caused TypeScript compilation errors blocking `npm run lint`.
searchNext() and searchPrevious() were passing empty string '' to xterm.js SearchAddon's findNext/findPrevious, so they never found any matches. Added lastSearchQueryRef to store the last search term. search() now saves the query and uses incremental:true for smooth search-as-you-type UX. searchNext() and searchPrevious() reuse the stored query without incremental so they correctly advance to the next/previous occurrence in the full scrollback buffer.
Fix navigateToUnifiedTabByIndex() in tabHelpers.ts to properly handle terminal tabs instead of returning null with a stale Phase 3 TODO comment. Terminal tabs now activate correctly via Cmd+1-9 unified tab navigation, setting activeTerminalTabId, clearing activeFileTabId, and switching inputMode to 'terminal'. All other Phase 7 requirements (tabStore CRUD actions, selectors, useTerminalTabHandlers hook, inputMode toggle wiring) were already implemented in prior phases.
…egacy fallback) Wrap all shellLogs append sites with !(s.terminalTabs?.length) so that sessions using the new xterm.js terminal tab system never write to the legacy shellLogs field. Affected files: - useInputProcessing.ts: user command entry + runCommand error - useAgentListeners.ts: terminal process exit + command exit (non-zero) - useRemoteHandlers.ts: user command entry + runCommand error (remote) - useInterruptHandler.ts: kill log + error log useBatchedSessionUpdates.ts already had this guard from a prior commit. Each site gets a TODO comment for future removal once migration is done.
…t-disable Remove unused `enterToSendTerminal` store subscription from MainPanel.tsx and the now-unnecessary `react-hooks/exhaustive-deps` disable directive from XTerminal.tsx. Both `npm run lint` and `npm run lint:eslint` pass cleanly with 0 errors and 0 warnings.
…x stale terminal-mode tests
- Added `terminalTabs: []` and `activeTerminalTabId: null` to all 47 mock session
factory functions across the test suite (createMockSession/createSession helpers)
- Fixed 29 previously failing tests:
* agentStore.test.ts: added `commitCommandPrompt: ''` to prompts mock
* MainPanel.test.tsx: updated TabBar-in-terminal-mode test to reflect new behavior
(unified tab system shows TabBar in both AI and terminal modes)
* TerminalOutput.test.tsx: removed 27 stale terminal-mode tests for behavior now
handled by TerminalView/XTerminal (shellLogs rendering, isTerminal=true code paths)
* auto-scroll.test.tsx: removed terminal-mode auto-scroll suppression test
- All 509 test files pass (21570 tests), lint passes
Documents xterm.js integration and terminal tab system: TerminalTab interface, Session fields (terminalTabs, activeTerminalTabId), features (persistent PTY, spawn failure UX, exit message), and key files table. Also adds entry to the Table of Contents. CLAUDE.md already had terminal entries added in a prior commit.
Collaborator
Author
Symphony Contribution SummaryThis pull request was created using Maestro Symphony - connecting AI-powered contributors with open source projects. Contribution Stats
Powered by Maestro • Learn about Symphony |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/__tests__/renderer/hooks/useMergeTransferHandlers.test.ts (1)
138-170:⚠️ Potential issue | 🟠 MajorApply
overridesincreateMockSessionto avoid misleading tests.Line 138 accepts
overrides, but the returned object never spreads it. That makes many test setups silently ineffective.Proposed fix
function createMockSession(overrides: Partial<Session> = {}): Session { return { id: 'session-1', name: 'Test Agent', state: 'idle', busySource: undefined, toolType: 'claude-code', aiTabs: [ { id: 'tab-1', name: 'Tab 1', inputValue: '', data: [], logs: [ { id: 'log-1', timestamp: Date.now(), source: 'user', text: 'Hello' }, { id: 'log-2', timestamp: Date.now(), source: 'ai', text: 'Hi there' }, ], stagedImages: [], agentSessionId: 'agent-1', starred: false, createdAt: Date.now(), }, ], activeTabId: 'tab-1', inputMode: 'ai', isGitRepo: false, cwd: '/test', projectRoot: '/test/project', shellLogs: [], shellCwd: '/test', terminalTabs: [], activeTerminalTabId: null, + ...overrides, } as unknown as Session; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/hooks/useMergeTransferHandlers.test.ts` around lines 138 - 170, The factory createMockSession currently ignores the overrides parameter, causing tests to not reflect custom values; update createMockSession to merge overrides into the returned Session (e.g., spread overrides into the top-level session object and into nested fields if needed) so callers can override id, name, aiTabs, activeTabId, etc.; locate the createMockSession function in the test and apply overrides by spreading the overrides object into the returned object (and handle nested overrides like aiTabs when necessary) to ensure tests use the provided values.
🧹 Nitpick comments (9)
src/__tests__/renderer/hooks/useSymphonyContribution.test.ts (1)
517-523: Tighten terminal tab identity assertion.Line 523 confirms a terminal ref exists, but not that it matches the session’s active terminal tab. Adding that check makes this regression-proof.
Suggested test hardening
expect(aiRef).toBeDefined(); expect(aiRef!.id).toBe(session.activeTabId); expect(termRef).toBeDefined(); + expect(termRef!.id).toBe(session.activeTerminalTabId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/hooks/useSymphonyContribution.test.ts` around lines 517 - 523, The test currently only verifies a terminal ref exists in session.unifiedTabOrder; tighten it by asserting the terminal ref identity matches the session's active terminal tab: after finding termRef, add an assertion that termRef!.id equals the session's active terminal tab id (use session.activeTerminalTabId or, if the code model uses a different property, the session property that represents the active terminal tab), similar to how aiRef is compared to session.activeTabId, so the test ensures the terminal tab is the expected active terminal instance.ARCHITECTURE.md (1)
1158-1165: Keep IPC API documentation internally consistent.Line 1164 documents
process:spawnTerminalTab, but thewindow.maestro.processAPI list earlier (Line 155) doesn’t mention it. Add it there as well so the architecture guide remains self-consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ARCHITECTURE.md` around lines 1158 - 1165, The ARCHITECTURE.md API list for window.maestro.process is missing the process:spawnTerminalTab entry; add an entry for process:spawnTerminalTab to the earlier window.maestro.process API list so it matches the documented handler (process:spawnTerminalTab) described later and the implementation in src/main/ipc/handlers/process.ts, include a brief description that it spawns a terminal tab (SSH support) to keep the documentation self-consistent.src/__tests__/renderer/components/MainPanel.test.tsx (1)
748-754: Strengthen this terminal-mode TabBar assertion.Right now it only verifies container presence. Adding a tab-item assertion makes the test less prone to false positives.
Suggested test hardening
- // TabBar renders in both AI and terminal modes when aiTabs exist - expect(screen.queryByTestId('tab-bar')).toBeInTheDocument(); + // TabBar renders in both AI and terminal modes when unified tabs exist + expect(screen.getByTestId('tab-bar')).toBeInTheDocument(); + expect(screen.getByTestId('tab-tab-1')).toBeInTheDocument();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/MainPanel.test.tsx` around lines 748 - 754, The test currently only checks that the TabBar container exists; strengthen it by asserting that at least one actual tab item is rendered inside it: after rendering MainPanel with createSession({ inputMode: 'terminal' }) and locating screen.getByTestId('tab-bar'), use testing-library's within(...) to assert there is a child element representing a tab (e.g., expect(within(tabBar).getAllByRole('tab').length).toBeGreaterThan(0) or expect(within(tabBar).getByTestId('tab-item')).toBeInTheDocument()), adding any needed import for within from '@testing-library/react'.src/__tests__/renderer/hooks/useSessionCrud.test.ts (1)
498-505: Make terminal-tab existence explicit before indexing.
The assertion is valid, but checkingterminalTabslength first makes failures clearer and avoids brittle direct indexing.Suggested test hardening
// New sessions include both an AI tab and a default terminal tab expect(session.unifiedTabOrder).toHaveLength(2); + expect(session.terminalTabs.length).toBeGreaterThan(0); const aiRef = session.unifiedTabOrder.find((r) => r.type === 'ai'); const termRef = session.unifiedTabOrder.find((r) => r.type === 'terminal'); expect(aiRef).toBeDefined(); expect(aiRef!.id).toBe(session.activeTabId); expect(termRef).toBeDefined(); - expect(termRef!.id).toBe(session.terminalTabs[0].id); + expect(termRef!.id).toBe(session.terminalTabs[0]!.id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/hooks/useSessionCrud.test.ts` around lines 498 - 505, The test indexes session.terminalTabs[0] without first asserting the terminalTabs array contains entries; add an explicit check (e.g., expect(session.terminalTabs).toHaveLength/greaterThan(0) or expect(session.terminalTabs[0]).toBeDefined()) before referencing session.terminalTabs[0].id so failures are clearer and the assertion comparing termRef!.id to session.terminalTabs[0].id (using session.unifiedTabOrder, aiRef, termRef) is no longer brittle.src/renderer/hooks/session/useSessionCrud.ts (1)
192-192: Consider platform-aware shell fallback.The hardcoded
'zsh'fallback may not be ideal on Windows wherezshis uncommon. Consider using a platform-aware default:Proposed platform-aware fallback
-const initialTerminalTab = createTerminalTab(currentDefaults.defaultShell || 'zsh', workingDir, null); +const fallbackShell = process.platform === 'win32' ? 'powershell.exe' : 'zsh'; +const initialTerminalTab = createTerminalTab(currentDefaults.defaultShell || fallbackShell, workingDir, null);Note: This is optional since
defaultShellshould typically be configured in settings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/session/useSessionCrud.ts` at line 192, The fallback to 'zsh' in the createTerminalTab call is not platform-aware; update the fallback to choose an appropriate default based on the OS (e.g., use 'powershell' or 'cmd.exe' on Windows and 'zsh'/'bash' on macOS/Linux). Locate the call to createTerminalTab in useSessionCrud.ts where currentDefaults.defaultShell is used and replace the hardcoded 'zsh' fallback with a small utility or inline check that inspects process.platform (or navigator.platform in renderer) and returns a platform-appropriate shell string before calling createTerminalTab.src/main/process-manager/ProcessManager.ts (1)
248-255: Avoid hardcodingzshas the non-Windows default shell.Defaulting to
zshon all non-Windows platforms can fail on Linux environments wherezshis not installed. Preferprocess.env.SHELLfirst, then a safer fallback.♻️ Proposed fallback update
- const shell = config.shell || (process.platform === 'win32' ? 'powershell.exe' : 'zsh'); + const shell = + config.shell || + (process.platform === 'win32' + ? 'powershell.exe' + : process.env.SHELL || '/bin/bash');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/process-manager/ProcessManager.ts` around lines 248 - 255, The code in ProcessManager that sets const shell = config.shell || (process.platform === 'win32' ? 'powershell.exe' : 'zsh') should avoid defaulting to zsh; change the fallback to prefer process.env.SHELL when config.shell is undefined, and then fall back to a safer system shell like '/bin/bash' or '/bin/sh' (still keep 'powershell.exe' for win32). Update the shell resolution where the shell variable is defined (refer to the shell constant in ProcessManager and its use in the logger.info/PTY spawn) so non-Windows platforms use process.env.SHELL first and a portable fallback if that is empty.src/renderer/utils/terminalTabHelpers.ts (1)
91-98: UselastIndexOffor composite terminal session ID parsing.Line 92 uses
indexOf; switching tolastIndexOfis safer ifsessionIdever contains the separator token.🔧 Suggested tweak
- const separatorIndex = terminalSessionId.indexOf(separator); + const separatorIndex = terminalSessionId.lastIndexOf(separator);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/utils/terminalTabHelpers.ts` around lines 91 - 98, The current parsing of composite terminalSessionId uses terminalSessionId.indexOf(separator) which can fail if the sessionId portion contains the separator; change to terminalSessionId.lastIndexOf(separator) so the split picks the final separator occurrence, keep the existing sentinel check for -1, and leave the subsequent substring logic that derives sessionId and tabId unchanged (refer to variables terminalSessionId, separator, separatorIndex, sessionId, tabId in terminalTabHelpers.ts).src/renderer/components/TerminalOutput.tsx (1)
1579-1582: Consider pruning unreachable terminal branches in this AI-only component.Line 1579-Line 1581 hardcodes AI mode, so terminal-only branches (
isTerminalpaths and related helpers) now add dead complexity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/TerminalOutput.tsx` around lines 1579 - 1582, The TerminalOutput component currently hardcodes isTerminal = false and isAIMode = true, leaving terminal-only branches and helpers dead; remove the unreachable terminal-mode logic by deleting the isTerminal variable and any conditional branches, JSX paths, helper functions, or imports solely used for terminal rendering (e.g., TerminalView-related code and terminal-specific helpers) within the TerminalOutput component so it only contains AI-mode rendering and state; ensure you update or remove any props, effects, or tests that reference these terminal branches to avoid unused-symbol warnings.src/renderer/utils/tabHelpers.ts (1)
53-58: Consider explicit type check for defensive coding.The
elsebranch implicitly treats any non-'ai', non-'file' type as 'terminal'. While currently correct given the type union, an explicit check would prevent silent bugs if new tab types are added.🔧 Suggested defensive check
- } else { + } else if (ref.type === 'terminal') { const tab = terminalTabMap.get(ref.id); if (tab) { result.push({ type: 'terminal', id: ref.id, data: tab }); terminalTabMap.delete(ref.id); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/utils/tabHelpers.ts` around lines 53 - 58, The else branch currently assumes any non-'ai'/'file' ref is a terminal; change it to an explicit check like if (ref.type === 'terminal') so only terminalTabMap is accessed for terminal refs, and add a defensive fallback (e.g., warn/log or skip) for unknown ref.type values; update the block that uses terminalTabMap.get(ref.id), result.push({ type: 'terminal', id: ref.id, data: tab }), and terminalTabMap.delete(ref.id) so those actions only run under the explicit 'terminal' check to avoid silent bugs if new tab types are introduced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/process-manager/ProcessManager.ts`:
- Around line 239-267: spawnTerminalTab currently lacks SSH context: extend its
config signature to accept sshRemoteConfig (and the SSH store adapter used
elsewhere), forward those fields into the spawn() call, and when sshRemoteConfig
is present wrap the spawn with wrapSpawnWithSsh() from ssh-spawn-wrapper.ts
using the same sshRemoteConfig and SSH store adapter so the terminal tab
participates in SSH remote execution; reference the spawnTerminalTab method,
spawn(...) call, wrapSpawnWithSsh(), and the sshRemoteConfig/SSH store adapter
symbols when making the changes.
In `@src/renderer/components/TabBar.tsx`:
- Around line 1991-1996: The auto-scroll logic still computes the "active" id as
activeFileTabId || activeTabId, which ignores terminal tabs; update all places
in TabBar that use that expression (including the areas referencing
activeFileTabId, activeTabId around the tab scrolling/selection logic) to
include activeTerminalTabId with the correct precedence—e.g. compute
currentActiveTabId = activeTerminalTabId || activeFileTabId || activeTabId—and
use currentActiveTabId for auto-scroll, selection and any comparisons so
selecting a terminal tab correctly scrolls it into view (check occurrences near
the onTerminalTabSelect/onTerminalTabClose handlers as well).
- Around line 1751-1772: The tab container rendered for TerminalTabItem is not
keyboard focusable or operable; update the element (the div using
ref={setTabRef} and event handlers like onClick={handleTabSelect},
onDoubleClick={handleDoubleClick}, onMouseDown, onMouseEnter/Leave) to be
keyboard-accessible by adding tabIndex={0}, a proper ARIA role (e.g.,
role="button" or role="tab" as appropriate), onFocus/onBlur handlers (reuse or
add handlers similar to handleMouseEnter/Leave to manage focus state) and an
onKeyDown handler that maps Enter/Space to the same activation logic as
handleTabSelect (and optionally maps Delete/Cmd+W to close). Ensure these
changes use the existing functions (handleTabSelect, setTabRef) or small new
helpers for key handling and do not remove existing pointer handlers.
- Around line 2071-2082: The terminal-tab branch in the unifiedTabs filter
always returns ut.id === activeTerminalTabId, which can surface terminal tabs
during the unread-only AI view; update the terminal case in the filter (the
callback passed to unifiedTabs.filter) to only return true when ut.id ===
activeTerminalTabId AND the current inputMode indicates terminal (check the
inputMode variable), e.g. gate the terminal branch with inputMode === 'terminal'
so terminal tabs are only shown when the UI is actually in terminal mode.
In `@src/renderer/components/TerminalSearchBar.tsx`:
- Around line 32-43: The component currently only resets local state (setQuery,
setHasResults) when the bar closes or the query is emptied; update the useEffect
that runs on isOpen and add a separate useEffect that watches query so that when
query === '' you call the terminal engine's clear/stop search API (e.g., invoke
clearSearchOnEngine or terminalEngine.clearSearch()) to clear delegated search
state; also in the Enter/submit handler (the onKeyDown or handleSubmit function
that navigates on Enter) short-circuit and return early if query.trim() === ''
so Enter does nothing when the query is empty. Ensure you reference and call the
actual engine instance/name used in this file (e.g., terminalEngine,
searchService) from both the close-path in the isOpen useEffect and the
query-empty watcher.
- Around line 127-152: The icon-only buttons for search navigation lack explicit
accessible names; add descriptive ARIA labels to each button (e.g.,
aria-label="Previous match" on the button that calls handlePrevious and contains
<ChevronUp>, aria-label="Next match" on the handleNext/<ChevronDown> button, and
aria-label="Close search" on the onClose/<X> button) while keeping the existing
title text and styling; ensure the labels reflect keyboard hints if desired
(e.g., "Previous match (Shift+Enter)"), and verify disabled logic remains
unchanged.
In `@src/renderer/components/TerminalTabRenameModal.tsx`:
- Around line 23-25: The component TerminalTabRenameModal currently initializes
value via useState(currentName ?? '') which only runs on first mount, causing
stale input when the modal is reopened; add a useEffect in
TerminalTabRenameModal that watches [isOpen, currentName] and calls
setValue(currentName ?? '') (typically when isOpen becomes true) so the input is
synced each time the modal opens or the currentName changes while keeping the
existing early return for !isOpen.
In `@src/renderer/components/TerminalView.tsx`:
- Around line 95-128: spawnPtyForTab can be re-entered for the same tab while a
previous spawnTerminalTab request is still pending; add a per-tab in-flight
guard (e.g., a Set or Map keyed by tab.id) checked at the top of spawnPtyForTab
to return early if the tab is already pending, mark the tab as in-flight before
calling window.maestro.process.spawnTerminalTab, and always remove the in-flight
marker in both .then (on both success and result.success=false) and .catch paths
(after calling onTabPidChange or onTabStateChange) so duplicate PTY spawns for
the same tab are prevented. Use the existing symbols spawnPtyForTab,
spawnTerminalTab, activeTab, onTabPidChange, and onTabStateChange to locate
where to add the guard and cleanup.
- Around line 260-267: The ref callback writes the "Starting terminal..."
message every render while tab.pid === 0 and tab.state === 'idle' because the
callback is recreated; fix by tracking whether the loading message was already
written for a given tab (use a component-scoped ref/set like
loadingMessageSentRef: Ref<Set<string>>), check
loadingMessageSentRef.current.has(tab.id) before calling handle.write, add
tab.id to the set after writing, and remove the id from the set when the tab
transitions out of the idle cycle (e.g., in an effect that watches tab.pid or
tab.state and clears loadingMessageSentRef.current.delete(tab.id) when pid !== 0
or state !== 'idle'); reference terminalRefs, tab.id, tab.pid, tab.state, and
handle.write to locate the change.
In `@src/renderer/hooks/keyboard/useMainKeyboardHandler.ts`:
- Around line 787-792: The close-tab shortcut branch checks ctx.isTabShortcut(e,
'closeTab') but only calls e.preventDefault() when there are multiple
terminalTabs and an activeTerminalTabId, allowing Cmd/Ctrl+W to fall through and
close the window when a single terminal tab is open; change the logic so that
when ctx.isTabShortcut(e, 'closeTab') is true you always call e.preventDefault()
first, then conditionally call ctx.handleCloseTerminalTab(activeTerminalTabId)
and trackShortcut('closeTab') only if terminalTabs.length > 1 and
activeTerminalTabId exists; reference ctx.isTabShortcut, terminalTabs,
activeTerminalTabId, ctx.handleCloseTerminalTab, e.preventDefault, and
trackShortcut when making the change.
- Around line 75-87: The early-return in useMainKeyboardHandler that ignores
Ctrl+<key> when ctx.activeSession?.inputMode === 'terminal' is too broad and
blocks Windows/Linux app shortcuts; narrow this behavior to macOS only by adding
an isMac/platform check before returning. Update the conditional that uses
ctx.activeSession?.inputMode, e.ctrlKey, !e.metaKey, !e.altKey, and !(e.shiftKey
&& e.code === 'Backquote') so it first verifies running on macOS (e.g.,
process.platform === 'darwin' or a shared isMac flag) and only then returns to
allow macOS-style Ctrl control sequences to be handled by xterm.js while
preserving Ctrl shortcuts on other OSes.
In `@src/renderer/hooks/session/useBatchedSessionUpdates.ts`:
- Around line 253-255: The current guard in useBatchedSessionUpdates that only
applies shellLogs when updatedSession.terminalTabs is empty causes buffered
stdout/stderr to be dropped for sessions with terminal tabs; change the
condition at the shell logs branch from "if ((shellStdout || shellStderr) &&
!(updatedSession.terminalTabs?.length))" to simply "if (shellStdout ||
shellStderr)" so shellLogs are always applied to updatedSession.shellLogs (or
appended if existing). Ensure you only apply shellLogs once (avoid duplicating
if terminal-tab-specific handlers also process output) and keep references to
updatedSession, shellStdout, shellStderr, and shellLogs when making the change.
In `@src/renderer/hooks/utils/useDebouncedPersistence.ts`:
- Around line 99-113: In useDebouncedPersistence, normalize and revalidate the
session's active terminal tab id against the sanitized terminalTabs before
returning: after creating cleanedTerminalTabs (and before assigning
activeTabId/newActiveTabId to the returned object) check that newActiveTabId (or
activeTerminalTabId) exists in cleanedTerminalTabs by matching tab.id; if it
does not, set activeTabId to a safe value (e.g., the first
cleanedTerminalTabs[0]?.id or undefined) so you never persist a stale ID that no
longer corresponds to a cleaned terminal tab.
In `@src/renderer/stores/tabStore.ts`:
- Around line 489-495: The current closeTerminalTab calls
window.maestro.process.kill before validating the close, which may kill the PTY
even if closeTerminalTabHelper refuses (e.g., last terminal); modify
closeTerminalTab to first call getActiveSession() and pass that session to
closeTerminalTabHelper(session, tabId), inspect the helper's result to confirm
the tab was actually removed (for example by comparing terminal lists or a
returned flag), only then call
window.maestro.process.kill(getTerminalSessionId(session.id, tabId)) and finally
call updateActiveSession(updatedSession); reference functions: closeTerminalTab,
getActiveSession, closeTerminalTabHelper, getTerminalSessionId,
window.maestro.process.kill, updateActiveSession.
In `@src/renderer/utils/terminalTabHelpers.ts`:
- Around line 278-290: The function updateTerminalTabState currently only sets
exitCode when provided, leaving prior exitCode values intact; update it so when
updating a tab's state (in updateTerminalTabState) and exitCode is not provided
you explicitly clear any previous exitCode for that tab (e.g., omit the property
or set exitCode to undefined/null) so stale exit codes don't persist across
state transitions; locate the tab update logic inside updateTerminalTabState
(the mapping over terminalTabs where tab.id === tabId) and change the object
spread to remove or reset exitCode when exitCode === undefined.
---
Outside diff comments:
In `@src/__tests__/renderer/hooks/useMergeTransferHandlers.test.ts`:
- Around line 138-170: The factory createMockSession currently ignores the
overrides parameter, causing tests to not reflect custom values; update
createMockSession to merge overrides into the returned Session (e.g., spread
overrides into the top-level session object and into nested fields if needed) so
callers can override id, name, aiTabs, activeTabId, etc.; locate the
createMockSession function in the test and apply overrides by spreading the
overrides object into the returned object (and handle nested overrides like
aiTabs when necessary) to ensure tests use the provided values.
---
Nitpick comments:
In `@ARCHITECTURE.md`:
- Around line 1158-1165: The ARCHITECTURE.md API list for window.maestro.process
is missing the process:spawnTerminalTab entry; add an entry for
process:spawnTerminalTab to the earlier window.maestro.process API list so it
matches the documented handler (process:spawnTerminalTab) described later and
the implementation in src/main/ipc/handlers/process.ts, include a brief
description that it spawns a terminal tab (SSH support) to keep the
documentation self-consistent.
In `@src/__tests__/renderer/components/MainPanel.test.tsx`:
- Around line 748-754: The test currently only checks that the TabBar container
exists; strengthen it by asserting that at least one actual tab item is rendered
inside it: after rendering MainPanel with createSession({ inputMode: 'terminal'
}) and locating screen.getByTestId('tab-bar'), use testing-library's within(...)
to assert there is a child element representing a tab (e.g.,
expect(within(tabBar).getAllByRole('tab').length).toBeGreaterThan(0) or
expect(within(tabBar).getByTestId('tab-item')).toBeInTheDocument()), adding any
needed import for within from '@testing-library/react'.
In `@src/__tests__/renderer/hooks/useSessionCrud.test.ts`:
- Around line 498-505: The test indexes session.terminalTabs[0] without first
asserting the terminalTabs array contains entries; add an explicit check (e.g.,
expect(session.terminalTabs).toHaveLength/greaterThan(0) or
expect(session.terminalTabs[0]).toBeDefined()) before referencing
session.terminalTabs[0].id so failures are clearer and the assertion comparing
termRef!.id to session.terminalTabs[0].id (using session.unifiedTabOrder, aiRef,
termRef) is no longer brittle.
In `@src/__tests__/renderer/hooks/useSymphonyContribution.test.ts`:
- Around line 517-523: The test currently only verifies a terminal ref exists in
session.unifiedTabOrder; tighten it by asserting the terminal ref identity
matches the session's active terminal tab: after finding termRef, add an
assertion that termRef!.id equals the session's active terminal tab id (use
session.activeTerminalTabId or, if the code model uses a different property, the
session property that represents the active terminal tab), similar to how aiRef
is compared to session.activeTabId, so the test ensures the terminal tab is the
expected active terminal instance.
In `@src/main/process-manager/ProcessManager.ts`:
- Around line 248-255: The code in ProcessManager that sets const shell =
config.shell || (process.platform === 'win32' ? 'powershell.exe' : 'zsh') should
avoid defaulting to zsh; change the fallback to prefer process.env.SHELL when
config.shell is undefined, and then fall back to a safer system shell like
'/bin/bash' or '/bin/sh' (still keep 'powershell.exe' for win32). Update the
shell resolution where the shell variable is defined (refer to the shell
constant in ProcessManager and its use in the logger.info/PTY spawn) so
non-Windows platforms use process.env.SHELL first and a portable fallback if
that is empty.
In `@src/renderer/components/TerminalOutput.tsx`:
- Around line 1579-1582: The TerminalOutput component currently hardcodes
isTerminal = false and isAIMode = true, leaving terminal-only branches and
helpers dead; remove the unreachable terminal-mode logic by deleting the
isTerminal variable and any conditional branches, JSX paths, helper functions,
or imports solely used for terminal rendering (e.g., TerminalView-related code
and terminal-specific helpers) within the TerminalOutput component so it only
contains AI-mode rendering and state; ensure you update or remove any props,
effects, or tests that reference these terminal branches to avoid unused-symbol
warnings.
In `@src/renderer/hooks/session/useSessionCrud.ts`:
- Line 192: The fallback to 'zsh' in the createTerminalTab call is not
platform-aware; update the fallback to choose an appropriate default based on
the OS (e.g., use 'powershell' or 'cmd.exe' on Windows and 'zsh'/'bash' on
macOS/Linux). Locate the call to createTerminalTab in useSessionCrud.ts where
currentDefaults.defaultShell is used and replace the hardcoded 'zsh' fallback
with a small utility or inline check that inspects process.platform (or
navigator.platform in renderer) and returns a platform-appropriate shell string
before calling createTerminalTab.
In `@src/renderer/utils/tabHelpers.ts`:
- Around line 53-58: The else branch currently assumes any non-'ai'/'file' ref
is a terminal; change it to an explicit check like if (ref.type === 'terminal')
so only terminalTabMap is accessed for terminal refs, and add a defensive
fallback (e.g., warn/log or skip) for unknown ref.type values; update the block
that uses terminalTabMap.get(ref.id), result.push({ type: 'terminal', id:
ref.id, data: tab }), and terminalTabMap.delete(ref.id) so those actions only
run under the explicit 'terminal' check to avoid silent bugs if new tab types
are introduced.
In `@src/renderer/utils/terminalTabHelpers.ts`:
- Around line 91-98: The current parsing of composite terminalSessionId uses
terminalSessionId.indexOf(separator) which can fail if the sessionId portion
contains the separator; change to terminalSessionId.lastIndexOf(separator) so
the split picks the final separator occurrence, keep the existing sentinel check
for -1, and leave the subsequent substring logic that derives sessionId and
tabId unchanged (refer to variables terminalSessionId, separator,
separatorIndex, sessionId, tabId in terminalTabHelpers.ts).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (98)
ARCHITECTURE.mdCLAUDE.mdpackage.jsonsrc/__tests__/main/ipc/handlers/process.test.tssrc/__tests__/renderer/components/AgentSessionsModal.test.tsxsrc/__tests__/renderer/components/AppConfirmModals.test.tsxsrc/__tests__/renderer/components/AppModals-selfSourced.test.tsxsrc/__tests__/renderer/components/FileExplorerPanel.test.tsxsrc/__tests__/renderer/components/GroupChatInput.test.tsxsrc/__tests__/renderer/components/HistoryPanel.test.tsxsrc/__tests__/renderer/components/InlineWizard/WizardInputPanel.test.tsxsrc/__tests__/renderer/components/InputArea.test.tsxsrc/__tests__/renderer/components/MainPanel.test.tsxsrc/__tests__/renderer/components/MergeSessionModal.test.tsxsrc/__tests__/renderer/components/QuickActionsModal.test.tsxsrc/__tests__/renderer/components/RenameSessionModal.test.tsxsrc/__tests__/renderer/components/SendToAgentModal.test.tsxsrc/__tests__/renderer/components/SessionList.test.tsxsrc/__tests__/renderer/components/TerminalOutput.test.tsxsrc/__tests__/renderer/components/ThinkingStatusPill.test.tsxsrc/__tests__/renderer/components/WorktreeRunSection.test.tsxsrc/__tests__/renderer/components/auto-scroll.test.tsxsrc/__tests__/renderer/hooks/useAgentListeners.test.tssrc/__tests__/renderer/hooks/useAtMentionCompletion.test.tssrc/__tests__/renderer/hooks/useAvailableAgents.test.tssrc/__tests__/renderer/hooks/useGroupManagement.test.tssrc/__tests__/renderer/hooks/useInputHandlers.test.tssrc/__tests__/renderer/hooks/useInterruptHandler.test.tssrc/__tests__/renderer/hooks/useKeyboardNavigation.test.tssrc/__tests__/renderer/hooks/useMergeSession.test.tssrc/__tests__/renderer/hooks/useMergeTransferHandlers.test.tssrc/__tests__/renderer/hooks/useModalHandlers.test.tssrc/__tests__/renderer/hooks/usePromptComposerHandlers.test.tssrc/__tests__/renderer/hooks/useQueueHandlers.test.tssrc/__tests__/renderer/hooks/useQueueProcessing.test.tssrc/__tests__/renderer/hooks/useQuickActionsHandlers.test.tssrc/__tests__/renderer/hooks/useRemoteHandlers.test.tssrc/__tests__/renderer/hooks/useSendToAgent.test.tssrc/__tests__/renderer/hooks/useSessionCrud.test.tssrc/__tests__/renderer/hooks/useSessionLifecycle.test.tssrc/__tests__/renderer/hooks/useSessionRestoration.test.tssrc/__tests__/renderer/hooks/useSummarizeHandler.test.tssrc/__tests__/renderer/hooks/useSymphonyContribution.test.tssrc/__tests__/renderer/hooks/useTabCompletion.test.tssrc/__tests__/renderer/hooks/useTabExportHandlers.test.tssrc/__tests__/renderer/hooks/useTabHandlers.test.tssrc/__tests__/renderer/hooks/utils/useDebouncedPersistence.test.tssrc/__tests__/renderer/stores/agentStore.test.tssrc/__tests__/renderer/stores/sessionStore.test.tssrc/__tests__/renderer/stores/tabStore.test.tssrc/__tests__/renderer/utils/contextExtractor.test.tssrc/__tests__/renderer/utils/sessionValidation.test.tssrc/__tests__/renderer/utils/tabHelpers.test.tssrc/__tests__/renderer/utils/terminalTabHelpers.test.tssrc/__tests__/web/mobile/AllSessionsView.test.tsxsrc/__tests__/web/mobile/App.test.tsxsrc/__tests__/web/mobile/SessionPillBar.test.tsxsrc/main/ipc/handlers/process.tssrc/main/preload/process.tssrc/main/process-manager/ProcessManager.tssrc/main/process-manager/spawners/PtySpawner.tssrc/main/process-manager/types.tssrc/renderer/App.tsxsrc/renderer/components/AppModals.tsxsrc/renderer/components/CustomThemeBuilder.tsxsrc/renderer/components/MainPanel.tsxsrc/renderer/components/TabBar.tsxsrc/renderer/components/TerminalOutput.tsxsrc/renderer/components/TerminalSearchBar.tsxsrc/renderer/components/TerminalTabRenameModal.tsxsrc/renderer/components/TerminalView.tsxsrc/renderer/components/XTerminal.tsxsrc/renderer/constants/shortcuts.tssrc/renderer/global.d.tssrc/renderer/hooks/agent/useAgentListeners.tssrc/renderer/hooks/agent/useInterruptHandler.tssrc/renderer/hooks/input/useInputMode.tssrc/renderer/hooks/input/useInputProcessing.tssrc/renderer/hooks/keyboard/useMainKeyboardHandler.tssrc/renderer/hooks/props/useMainPanelProps.tssrc/renderer/hooks/remote/useRemoteHandlers.tssrc/renderer/hooks/session/useBatchedSessionUpdates.tssrc/renderer/hooks/session/useSessionCrud.tssrc/renderer/hooks/session/useSessionLifecycle.tssrc/renderer/hooks/session/useSessionRestoration.tssrc/renderer/hooks/symphony/useSymphonyContribution.tssrc/renderer/hooks/tabs/index.tssrc/renderer/hooks/tabs/useTabHandlers.tssrc/renderer/hooks/utils/useDebouncedPersistence.tssrc/renderer/hooks/wizard/useWizardHandlers.tssrc/renderer/stores/tabStore.tssrc/renderer/types/index.tssrc/renderer/utils/tabHelpers.tssrc/renderer/utils/terminalTabHelpers.tssrc/renderer/utils/worktreeSession.tssrc/shared/theme-types.tssrc/shared/themes.tssrc/web/utils/cssCustomProperties.ts
Comment on lines
+239
to
+267
| spawnTerminalTab(config: { | ||
| sessionId: string; | ||
| cwd: string; | ||
| shell?: string; | ||
| shellArgs?: string; | ||
| shellEnvVars?: Record<string, string>; | ||
| cols?: number; | ||
| rows?: number; | ||
| }): SpawnResult { | ||
| const shell = config.shell || (process.platform === 'win32' ? 'powershell.exe' : 'zsh'); | ||
| logger.info('[ProcessManager] Spawning terminal tab PTY', 'ProcessManager', { | ||
| sessionId: config.sessionId, | ||
| cwd: config.cwd, | ||
| shell, | ||
| cols: config.cols || 80, | ||
| rows: config.rows || 24, | ||
| }); | ||
| return this.spawn({ | ||
| sessionId: config.sessionId, | ||
| toolType: 'terminal', | ||
| cwd: config.cwd, | ||
| command: shell, | ||
| args: [], | ||
| shell, | ||
| shellArgs: config.shellArgs, | ||
| shellEnvVars: config.shellEnvVars, | ||
| cols: config.cols || 80, | ||
| rows: config.rows || 24, | ||
| }); |
There was a problem hiding this comment.
Propagate SSH remote config through terminal-tab spawns.
Line 239 defines a config shape that cannot carry SSH execution context, and Line 256 forwards to spawn() without any SSH remote config field. This makes terminal-tab spawning unable to explicitly participate in SSH wrapping on this path.
Based on learnings: When implementing features that spawn agent processes, support SSH remote execution by checking for sshRemoteConfig, using wrapSpawnWithSsh() from ssh-spawn-wrapper.ts, and passing the SSH store adapter.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/process-manager/ProcessManager.ts` around lines 239 - 267,
spawnTerminalTab currently lacks SSH context: extend its config signature to
accept sshRemoteConfig (and the SSH store adapter used elsewhere), forward those
fields into the spawn() call, and when sshRemoteConfig is present wrap the spawn
with wrapSpawnWithSsh() from ssh-spawn-wrapper.ts using the same sshRemoteConfig
and SSH store adapter so the terminal tab participates in SSH remote execution;
reference the spawnTerminalTab method, spawn(...) call, wrapSpawnWithSsh(), and
the sshRemoteConfig/SSH store adapter symbols when making the changes.
Comment on lines
+253
to
+255
| // Apply shell logs (legacy fallback — only when no terminal tabs present) | ||
| // TODO: Remove shellLogs once terminal tabs migration is complete | ||
| if ((shellStdout || shellStderr) && !(updatedSession.terminalTabs?.length)) { |
There was a problem hiding this comment.
Shell output can be silently dropped for terminal-enabled sessions.
Line 255 blocks shellLogs updates whenever terminalTabs is non-empty. With terminal tabs now commonly present by default, buffered stdout/stderr entering this path can be discarded.
Safer interim fix
- // Apply shell logs (legacy fallback — only when no terminal tabs present)
- // TODO: Remove shellLogs once terminal tabs migration is complete
- if ((shellStdout || shellStderr) && !(updatedSession.terminalTabs?.length)) {
+ // Apply shell logs (legacy fallback)
+ // TODO: Remove shellLogs once terminal output is fully migrated to terminal tabs
+ if (shellStdout || shellStderr) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Apply shell logs (legacy fallback — only when no terminal tabs present) | |
| // TODO: Remove shellLogs once terminal tabs migration is complete | |
| if ((shellStdout || shellStderr) && !(updatedSession.terminalTabs?.length)) { | |
| // Apply shell logs (legacy fallback) | |
| // TODO: Remove shellLogs once terminal output is fully migrated to terminal tabs | |
| if (shellStdout || shellStderr) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/session/useBatchedSessionUpdates.ts` around lines 253 -
255, The current guard in useBatchedSessionUpdates that only applies shellLogs
when updatedSession.terminalTabs is empty causes buffered stdout/stderr to be
dropped for sessions with terminal tabs; change the condition at the shell logs
branch from "if ((shellStdout || shellStderr) &&
!(updatedSession.terminalTabs?.length))" to simply "if (shellStdout ||
shellStderr)" so shellLogs are always applied to updatedSession.shellLogs (or
appended if existing). Ensure you only apply shellLogs once (avoid duplicating
if terminal-tab-specific handlers also process output) and keep references to
updatedSession, shellStdout, shellStderr, and shellLogs when making the change.
Comment on lines
278
to
290
| export function updateTerminalTabState( | ||
| session: Session, | ||
| tabId: string, | ||
| state: TerminalTab['state'], | ||
| exitCode?: number | ||
| ): Session { | ||
| const terminalTabs = session.terminalTabs || []; | ||
| return { | ||
| ...session, | ||
| terminalTabs: terminalTabs.map((tab) => | ||
| tab.id === tabId | ||
| ? { ...tab, state, ...(exitCode !== undefined ? { exitCode } : {}) } | ||
| : tab |
There was a problem hiding this comment.
Clear stale exitCode when transitioning terminal tab state.
Line 289 only writes exitCode when provided; prior values can persist across later state changes and misrepresent process outcomes.
🛠️ Suggested fix
export function updateTerminalTabState(
session: Session,
tabId: string,
state: TerminalTab['state'],
exitCode?: number
): Session {
const terminalTabs = session.terminalTabs || [];
return {
...session,
terminalTabs: terminalTabs.map((tab) =>
tab.id === tabId
- ? { ...tab, state, ...(exitCode !== undefined ? { exitCode } : {}) }
+ ? {
+ ...tab,
+ state,
+ exitCode: state === 'exited' ? exitCode : undefined,
+ }
: tab
),
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function updateTerminalTabState( | |
| session: Session, | |
| tabId: string, | |
| state: TerminalTab['state'], | |
| exitCode?: number | |
| ): Session { | |
| const terminalTabs = session.terminalTabs || []; | |
| return { | |
| ...session, | |
| terminalTabs: terminalTabs.map((tab) => | |
| tab.id === tabId | |
| ? { ...tab, state, ...(exitCode !== undefined ? { exitCode } : {}) } | |
| : tab | |
| export function updateTerminalTabState( | |
| session: Session, | |
| tabId: string, | |
| state: TerminalTab['state'], | |
| exitCode?: number | |
| ): Session { | |
| const terminalTabs = session.terminalTabs || []; | |
| return { | |
| ...session, | |
| terminalTabs: terminalTabs.map((tab) => | |
| tab.id === tabId | |
| ? { | |
| ...tab, | |
| state, | |
| exitCode: state === 'exited' ? exitCode : undefined, | |
| } | |
| : tab | |
| ), | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/utils/terminalTabHelpers.ts` around lines 278 - 290, The
function updateTerminalTabState currently only sets exitCode when provided,
leaving prior exitCode values intact; update it so when updating a tab's state
(in updateTerminalTabState) and exitCode is not provided you explicitly clear
any previous exitCode for that tab (e.g., omit the property or set exitCode to
undefined/null) so stale exit codes don't persist across state transitions;
locate the tab update logic inside updateTerminalTabState (the mapping over
terminalTabs where tab.id === tabId) and change the object spread to remove or
reset exitCode when exitCode === undefined.
Greptile SummaryThis PR implements persistent PTY-backed terminal tabs with full xterm.js integration, replacing the legacy shell command execution system. The implementation is well-architected and follows Maestro's existing patterns. Key Changes:
Architecture Highlights:
Issues Found:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant TabStore
participant TerminalView
participant XTerminal
participant IPC
participant ProcessManager
participant PTY
User->>TabStore: createTerminalTab()
TabStore->>TabStore: addTerminalTab(session, tab)
Note over TabStore: Updates session with new tab<br/>Sets inputMode to 'terminal'
TabStore->>TerminalView: Renders with new tab
TerminalView->>TerminalView: useEffect detects tab.pid === 0
TerminalView->>IPC: spawnTerminalTab({sessionId, cwd, shell})
IPC->>ProcessManager: spawnTerminalTab(config)
ProcessManager->>PTY: spawn(shell, args, {cols, rows})
PTY-->>ProcessManager: pid
ProcessManager-->>IPC: {success: true, pid}
IPC-->>TerminalView: result
TerminalView->>TabStore: onTabPidChange(tabId, pid)
Note over TabStore: Tab state: idle → busy
TerminalView->>XTerminal: Create xterm.js instance
XTerminal->>XTerminal: Load addons (WebGL, Search, FitAddon)
XTerminal->>XTerminal: Subscribe to IPC onData
User->>XTerminal: Types command
XTerminal->>IPC: write(sessionId, data)
IPC->>PTY: write to stdin
PTY-->>IPC: onData (stdout/stderr)
IPC-->>XTerminal: data event
XTerminal->>XTerminal: terminal.write(data)
Note over XTerminal: ANSI codes rendered with colors
User->>User: Cmd+F
User->>TerminalSearchBar: Opens search
TerminalSearchBar->>XTerminal: search(query)
XTerminal->>XTerminal: searchAddon.findNext(query)
Note over XTerminal: Highlights matches in buffer
PTY-->>IPC: exit event (code)
IPC-->>TerminalView: onExit(sessionId, code)
TerminalView->>TabStore: onTabStateChange(tabId, 'exited', code)
TerminalView->>XTerminal: write("Shell exited (code: X)")
Note over TabStore: Tab state: busy → exited
Last reviewed commit: b120a90 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
94
to
120
| // Shared spawn function — used both on mount and for retry | ||
| const spawnPtyForTab = useCallback( | ||
| (tab: TerminalTab) => { | ||
| const terminalSessionId = getTerminalSessionId(session.id, tab.id); | ||
| const tabId = tab.id; | ||
|
|
||
| window.maestro.process | ||
| .spawnTerminalTab({ | ||
| sessionId: terminalSessionId, | ||
| cwd: tab.cwd || session.cwd, | ||
| shell: defaultShell || undefined, | ||
| shellArgs, | ||
| shellEnvVars, | ||
| }) | ||
| .then((result) => { | ||
| if (result.success) { | ||
| onTabPidChange(tabId, result.pid); | ||
| } else { | ||
| onTabStateChange(tabId, 'exited', 1); | ||
| } | ||
| }) | ||
| .catch(() => { | ||
| onTabStateChange(tabId, 'exited', 1); | ||
| }); | ||
| }, | ||
| [session.id, session.cwd, defaultShell, shellArgs, shellEnvVars] | ||
| ); |
There was a problem hiding this comment.
Missing dependencies in useCallback — onTabPidChange and onTabStateChange should be in the dependency array to avoid stale closures
Suggested change
| // Shared spawn function — used both on mount and for retry | |
| const spawnPtyForTab = useCallback( | |
| (tab: TerminalTab) => { | |
| const terminalSessionId = getTerminalSessionId(session.id, tab.id); | |
| const tabId = tab.id; | |
| window.maestro.process | |
| .spawnTerminalTab({ | |
| sessionId: terminalSessionId, | |
| cwd: tab.cwd || session.cwd, | |
| shell: defaultShell || undefined, | |
| shellArgs, | |
| shellEnvVars, | |
| }) | |
| .then((result) => { | |
| if (result.success) { | |
| onTabPidChange(tabId, result.pid); | |
| } else { | |
| onTabStateChange(tabId, 'exited', 1); | |
| } | |
| }) | |
| .catch(() => { | |
| onTabStateChange(tabId, 'exited', 1); | |
| }); | |
| }, | |
| [session.id, session.cwd, defaultShell, shellArgs, shellEnvVars] | |
| ); | |
| const spawnPtyForTab = useCallback( | |
| (tab: TerminalTab) => { | |
| const terminalSessionId = getTerminalSessionId(session.id, tab.id); | |
| const tabId = tab.id; | |
| window.maestro.process | |
| .spawnTerminalTab({ | |
| sessionId: terminalSessionId, | |
| cwd: tab.cwd || session.cwd, | |
| shell: defaultShell || undefined, | |
| shellArgs, | |
| shellEnvVars, | |
| }) | |
| .then((result) => { | |
| if (result.success) { | |
| onTabPidChange(tabId, result.pid); | |
| } else { | |
| onTabStateChange(tabId, 'exited', 1); | |
| } | |
| }) | |
| .catch(() => { | |
| onTabStateChange(tabId, 'exited', 1); | |
| }); | |
| }, | |
| [session.id, session.cwd, defaultShell, shellArgs, shellEnvVars, onTabPidChange, onTabStateChange] | |
| ); |
Comment on lines
122
to
128
| // Spawn PTY when active tab changes and has no PID yet | ||
| useEffect(() => { | ||
| if (!activeTab || activeTab.pid !== 0 || activeTab.state === 'exited') { | ||
| return; | ||
| } | ||
| spawnPtyForTab(activeTab); | ||
| }, [activeTab?.id]); |
There was a problem hiding this comment.
Missing spawnPtyForTab in dependency array — when dependencies change, the effect should re-run with the updated callback
Suggested change
| // Spawn PTY when active tab changes and has no PID yet | |
| useEffect(() => { | |
| if (!activeTab || activeTab.pid !== 0 || activeTab.state === 'exited') { | |
| return; | |
| } | |
| spawnPtyForTab(activeTab); | |
| }, [activeTab?.id]); | |
| // Spawn PTY when active tab changes and has no PID yet | |
| useEffect(() => { | |
| if (!activeTab || activeTab.pid !== 0 || activeTab.state === 'exited') { | |
| return; | |
| } | |
| spawnPtyForTab(activeTab); | |
| }, [activeTab?.id, spawnPtyForTab]); |
- TerminalView: Guard duplicate PTY spawns with in-flight ref; fix missing useCallback deps (onTabPidChange, onTabStateChange, sessionSshRemoteConfig); prevent repeated loading message writes per tab; pass sessionSshRemoteConfig through to spawnTerminalTab IPC - TabBar/TerminalTabItem: Add tabIndex, role, aria-selected, onFocus, onBlur, onKeyDown for full keyboard accessibility - TabBar auto-scroll: include activeTerminalTabId and inputMode so terminal tabs scroll into view on selection - TabBar unread filter: gate terminal tabs by inputMode === 'terminal' to prevent leaking them into AI-mode filtered view - TerminalSearchBar: add type/aria-label to icon buttons; clear terminal engine search state on bar close or empty query; guard Enter navigation when query is empty - TerminalTabRenameModal: sync value with currentName/isOpen via useEffect to fix stale state when modal reopens for a new tab - useMainKeyboardHandler: scope Ctrl-passthrough to macOS only to avoid breaking Ctrl+F/W/K on Windows/Linux; always preventDefault on closeTab shortcut in terminal mode regardless of tab count - tabStore: validate close before kill to prevent orphaned PTY when closeTerminalTabHelper refuses (last-tab guard) - useDebouncedPersistence: validate and normalize activeTerminalTabId against cleaned terminal tabs before persisting - terminalTabHelpers: always overwrite exitCode on state transitions to clear stale values from prior process runs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/components/TerminalView.tsx (1)
153-158: Missing dependencies inuseEffect— intentional but worth documenting.The effect runs on
activeTab?.idchanges but referencessearchOpenandonSearchClosewithout including them in the dependency array. While this appears intentional (we only want to close search when the tab changes, not when these props change), consider adding a comment to clarify this is deliberate.📝 Suggested clarification
// Close search when the active terminal tab changes useEffect(() => { + // Intentionally only depends on activeTab?.id — we want to close search + // when switching tabs, not when searchOpen/onSearchClose props change if (searchOpen) { onSearchClose?.(); } + // eslint-disable-next-line react-hooks/exhaustive-deps }, [activeTab?.id]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/TerminalView.tsx` around lines 153 - 158, The useEffect in TerminalView referencing searchOpen and onSearchClose but only depending on activeTab?.id can be confusing; add an inline comment above the useEffect explaining that the omission of searchOpen and onSearchClose from the dependency array is intentional so the search only closes on tab changes (not when those props toggle), and mention the specific symbols: useEffect, activeTab?.id, searchOpen, and onSearchClose to make intent clear for future readers and linters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/renderer/components/TerminalView.tsx`:
- Around line 153-158: The useEffect in TerminalView referencing searchOpen and
onSearchClose but only depending on activeTab?.id can be confusing; add an
inline comment above the useEffect explaining that the omission of searchOpen
and onSearchClose from the dependency array is intentional so the search only
closes on tab changes (not when those props toggle), and mention the specific
symbols: useEffect, activeTab?.id, searchOpen, and onSearchClose to make intent
clear for future readers and linters.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/renderer/components/TabBar.tsxsrc/renderer/components/TerminalSearchBar.tsxsrc/renderer/components/TerminalTabRenameModal.tsxsrc/renderer/components/TerminalView.tsxsrc/renderer/hooks/keyboard/useMainKeyboardHandler.tssrc/renderer/hooks/utils/useDebouncedPersistence.tssrc/renderer/stores/tabStore.tssrc/renderer/utils/terminalTabHelpers.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/renderer/components/TerminalTabRenameModal.tsx
- src/renderer/hooks/keyboard/useMainKeyboardHandler.ts
- src/renderer/hooks/utils/useDebouncedPersistence.ts
Collaborator
Author
|
@coderabbitai review @greptile review |
|
✅ Actions performedReview triggered.
|
Add inline comment and eslint-disable to the search-close effect that intentionally omits searchOpen/onSearchClose from the dependency array. The effect should fire only on tab changes, not prop changes — the omission is deliberate and now documented for future readers.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/renderer/components/TerminalView.tsx (1)
174-187: Prune removed tab IDs fromprevTabStatesRef.This map is append-only today. With frequent tab churn, it will retain stale IDs indefinitely.
♻️ Proposed refactor
useEffect(() => { const terminalTabs = session.terminalTabs || []; + const currentTabIds = new Set(terminalTabs.map((tab) => tab.id)); + for (const tabId of prevTabStatesRef.current.keys()) { + if (!currentTabIds.has(tabId)) { + prevTabStatesRef.current.delete(tabId); + } + } + for (const tab of terminalTabs) { const prev = prevTabStatesRef.current.get(tab.id); if (prev !== undefined && prev !== 'exited' && tab.state === 'exited') { const handle = terminalRefs.current.get(tab.id); if (handle) { const code = tab.exitCode ?? 0; handle.write(`\r\n\x1b[33mShell exited (code: ${code}).\x1b[0m Press Ctrl+Shift+\` for new terminal.\r\n`); } } prevTabStatesRef.current.set(tab.id, tab.state); } }, [session.terminalTabs]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/TerminalView.tsx` around lines 174 - 187, The useEffect in TerminalView that updates prevTabStatesRef is currently append-only and leaks stale tab IDs; after iterating session.terminalTabs (in the effect that references prevTabStatesRef and terminalRefs) compute the set of current tab ids from session.terminalTabs and remove any keys from prevTabStatesRef.current that are not in that set so stale IDs are pruned; keep the existing logic that writes exit messages (use the same tab.id, tab.state, tab.exitCode, and terminalRefs handles) and perform the pruning step once per effect run after updating/setting current tab states.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/components/TerminalView.tsx`:
- Around line 108-129: The spawnTerminalTab call can hang and leave
spawnInFlightRef.current with tabId forever; wrap
window.maestro.process.spawnTerminalTab(...) in a timeout/fail-safe (e.g.,
Promise.race with a timer that rejects or returns a failure after a configurable
timeout like 30s) so the .then/.catch/.finally path always runs; ensure the
timeout path triggers onTabStateChange(tabId, 'exited', 1) or onTabPidChange as
appropriate and that spawnInFlightRef.current.delete(tabId) still executes in
the finally handler so retries are not blocked (refer to spawnTerminalTab,
spawnInFlightRef, onTabStateChange, onTabPidChange, and tabId).
---
Nitpick comments:
In `@src/renderer/components/TerminalView.tsx`:
- Around line 174-187: The useEffect in TerminalView that updates
prevTabStatesRef is currently append-only and leaks stale tab IDs; after
iterating session.terminalTabs (in the effect that references prevTabStatesRef
and terminalRefs) compute the set of current tab ids from session.terminalTabs
and remove any keys from prevTabStatesRef.current that are not in that set so
stale IDs are pruned; keep the existing logic that writes exit messages (use the
same tab.id, tab.state, tab.exitCode, and terminalRefs handles) and perform the
pruning step once per effect run after updating/setting current tab states.
Comment on lines
+108
to
+129
| window.maestro.process | ||
| .spawnTerminalTab({ | ||
| sessionId: terminalSessionId, | ||
| cwd: tab.cwd || session.cwd, | ||
| shell: defaultShell || undefined, | ||
| shellArgs, | ||
| shellEnvVars, | ||
| sessionSshRemoteConfig: session.sessionSshRemoteConfig, | ||
| }) | ||
| .then((result) => { | ||
| if (result.success) { | ||
| onTabPidChange(tabId, result.pid); | ||
| } else { | ||
| onTabStateChange(tabId, 'exited', 1); | ||
| } | ||
| }) | ||
| .catch(() => { | ||
| onTabStateChange(tabId, 'exited', 1); | ||
| }) | ||
| .finally(() => { | ||
| spawnInFlightRef.current.delete(tabId); | ||
| }); |
There was a problem hiding this comment.
Add a spawn timeout/fail-safe to avoid permanent in-flight lock.
If spawnTerminalTab never resolves/rejects, the tab stays in spawnInFlightRef forever and retries are blocked indefinitely.
💡 Proposed fix
const spawnPtyForTab = useCallback(
(tab: TerminalTab) => {
const tabId = tab.id;
// Guard: skip if a spawn is already in flight for this tab
if (spawnInFlightRef.current.has(tabId)) return;
spawnInFlightRef.current.add(tabId);
const terminalSessionId = getTerminalSessionId(session.id, tabId);
+ const SPAWN_TIMEOUT_MS = 15000;
+ let timeoutId: ReturnType<typeof setTimeout> | undefined;
- window.maestro.process
- .spawnTerminalTab({
+ const spawnPromise = window.maestro.process.spawnTerminalTab({
sessionId: terminalSessionId,
cwd: tab.cwd || session.cwd,
shell: defaultShell || undefined,
shellArgs,
shellEnvVars,
sessionSshRemoteConfig: session.sessionSshRemoteConfig,
- })
+ });
+
+ const timeoutPromise = new Promise<never>((_, reject) => {
+ timeoutId = setTimeout(() => reject(new Error('spawn timeout')), SPAWN_TIMEOUT_MS);
+ });
+
+ Promise.race([spawnPromise, timeoutPromise])
.then((result) => {
if (result.success) {
onTabPidChange(tabId, result.pid);
} else {
onTabStateChange(tabId, 'exited', 1);
}
})
.catch(() => {
onTabStateChange(tabId, 'exited', 1);
})
.finally(() => {
+ if (timeoutId) clearTimeout(timeoutId);
spawnInFlightRef.current.delete(tabId);
});
},
[session.id, session.cwd, session.sessionSshRemoteConfig, defaultShell, shellArgs, shellEnvVars, onTabPidChange, onTabStateChange]
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/TerminalView.tsx` around lines 108 - 129, The
spawnTerminalTab call can hang and leave spawnInFlightRef.current with tabId
forever; wrap window.maestro.process.spawnTerminalTab(...) in a
timeout/fail-safe (e.g., Promise.race with a timer that rejects or returns a
failure after a configurable timeout like 30s) so the .then/.catch/.finally path
always runs; ensure the timeout path triggers onTabStateChange(tabId, 'exited',
1) or onTabPidChange as appropriate and that
spawnInFlightRef.current.delete(tabId) still executes in the finally handler so
retries are not blocked (refer to spawnTerminalTab, spawnInFlightRef,
onTabStateChange, onTabPidChange, and tabId).
Comment on lines
+161
to
+168
| }, [activeTab?.id]); | ||
|
|
||
| // Subscribe to PTY exit events for terminal tabs in this session | ||
| useEffect(() => { | ||
| const cleanup = window.maestro.process.onExit((exitSessionId: string, code: number) => { | ||
| const parsed = parseTerminalSessionId(exitSessionId); | ||
| if (!parsed || parsed.sessionId !== session.id) return; | ||
| onTabStateChange(parsed.tabId, 'exited', code); |
There was a problem hiding this comment.
missing onTabStateChange in dependency array — the effect subscribes to process exit events using onTabStateChange in the callback, but if this prop changes, the event handler will use a stale closure
Suggested change
| }, [activeTab?.id]); | |
| // Subscribe to PTY exit events for terminal tabs in this session | |
| useEffect(() => { | |
| const cleanup = window.maestro.process.onExit((exitSessionId: string, code: number) => { | |
| const parsed = parseTerminalSessionId(exitSessionId); | |
| if (!parsed || parsed.sessionId !== session.id) return; | |
| onTabStateChange(parsed.tabId, 'exited', code); | |
| }, [session.id, onTabStateChange]); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Maestro Symphony Contribution
Closes #160
Contributed via Maestro Symphony.
Status: In Progress
Started: 2026-03-01T08:46:08.940Z
This PR will be updated automatically when the Auto Run completes.
Summary by CodeRabbit
New Features
Documentation