Feature: Full Terminal Emulation#327
Open
calebsmithdev wants to merge 110 commits intoRunMaestro:mainfrom
Open
Feature: Full Terminal Emulation#327calebsmithdev wants to merge 110 commits intoRunMaestro:mainfrom
calebsmithdev wants to merge 110 commits intoRunMaestro:mainfrom
Conversation
Author
|
A couple of additional notes:
|
8 tasks
Collaborator
|
@greptile review |
|
I notice ✅ Actions performedReview triggered.
|
Collaborator
|
@calebsmithdev finally getting around to this glorious contribution of yours. we've been refactoring a bunch, would you rebase your PR? |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds full terminal mode: xterm.js-based terminal component (XTerminal) with multi-tab terminal sessions, per-tab PTYs (spawnTerminalTab IPC/preload API), ProcessManager/PtySpawner updates (cols/rows, SSH), session persistence/migration for terminalTabs, ANSI theming, keyboard shortcuts, many tests, and docs/shortcut updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant TerminalView as TerminalView
participant XTerminal as XTerminal
participant Preload as Preload (renderer→main)
participant ProcessMgr as ProcessManager (main)
participant PTY as node-pty
User->>TerminalView: Open New Tab / Select Tab
TerminalView->>Preload: spawnTerminalTab(config: sessionId, cwd, shell, cols, rows)
Preload->>ProcessMgr: IPC process:spawnTerminalTab
ProcessMgr->>PTY: spawn(shell, args, env, cwd, cols, rows)
PTY-->>ProcessMgr: returns pid + handle
ProcessMgr-->>Preload: { pid, success }
Preload-->>TerminalView: reply { pid, success }
TerminalView->>XTerminal: attach PTY output stream
User->>XTerminal: Keyboard input
XTerminal->>Preload: process.write(sessionId, data)
Preload->>PTY: stdin.write(data)
PTY->>Preload: data events
Preload->>TerminalView: processData events
TerminalView->>XTerminal: write(data) -> render to User
sequenceDiagram
participant PTY as node-pty
participant DataListener as Main Data Listener
participant WebServer as Web Server
participant Renderer as Renderer Clients/Websocket
PTY->>DataListener: emit data (sessionId-terminal-tab)
DataListener->>WebServer: broadcastToSessionClients(session_output with full terminal sessionId)
DataListener->>Renderer: safeSend IPC to renderer
Renderer->>Renderer: display output
Renderer->>Preload: process:interrupt (user Ctrl+C)
Preload->>ProcessMgr: IPC interrupt handler
ProcessMgr->>PTY: send SIGINT
PTY->>DataListener: exit event
DataListener->>WebServer: broadcastToSessionClients(session_exit with full terminal sessionId)
Renderer->>Renderer: update tab state to exited
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryReplaces log-based terminal output rendering with full xterm.js terminal emulation backed by persistent PTY sessions. Adds multi-tab terminal support with complete lifecycle management including create/close/reopen/reorder/rename operations. Implements terminal-specific keyboard shortcuts, search functionality, and ANSI-aware theming. Major Changes:
The implementation follows established patterns from AI tab management, maintains backwards compatibility through migration helpers, and includes proper error handling for spawn failures and PTY exit scenarios. Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant App.tsx
participant TerminalView
participant XTerminal
participant IPC
participant ProcessManager
participant PTY
User->>App.tsx: Switch to Terminal Mode
App.tsx->>TerminalView: Render with terminalTabs[]
TerminalView->>XTerminal: Mount terminal instance
XTerminal->>XTerminal: Initialize xterm.js + addons
Note over XTerminal: FitAddon, WebGL, Search, Unicode
TerminalView->>IPC: spawnTerminalTab(sessionId, cwd, shell)
IPC->>ProcessManager: spawnTerminalTab()
ProcessManager->>PTY: pty.spawn(shell, args, {cwd, env})
PTY-->>ProcessManager: pid
ProcessManager-->>IPC: {pid, success: true}
IPC-->>TerminalView: PTY spawned
TerminalView->>App.tsx: onTabPidChange(tabId, pid)
loop PTY Output Stream
PTY->>ProcessManager: onData(rawBytes)
ProcessManager->>IPC: emit('process:data', sessionId, rawBytes)
IPC->>XTerminal: write(rawBytes)
XTerminal->>XTerminal: Render ANSI to terminal
end
User->>XTerminal: Type command
XTerminal->>IPC: process.write(sessionId, input)
IPC->>PTY: ptyProcess.write(input)
User->>App.tsx: Ctrl+Shift+` (New Tab)
App.tsx->>TerminalView: handleNewTab()
TerminalView->>App.tsx: onNewTab()
App.tsx->>App.tsx: addTerminalTab(session)
Note over App.tsx: Creates new tab, spawns PTY
PTY->>ProcessManager: onExit(exitCode)
ProcessManager->>IPC: emit('process:exit', sessionId, exitCode)
IPC->>TerminalView: onExit handler
TerminalView->>XTerminal: write(SHELL_EXIT_MESSAGE)
TerminalView->>App.tsx: onTabStateChange(tabId, 'exited', exitCode)
User->>TerminalView: Close Tab
TerminalView->>IPC: process.kill(sessionId)
IPC->>PTY: ptyProcess.kill()
TerminalView->>App.tsx: onTabClose(tabId)
App.tsx->>App.tsx: closeTerminalTab(session, tabId)
Note over App.tsx: Removes tab, updates history
Last reviewed commit: fd2f935 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/components/TerminalOutput.tsx (1)
816-823:⚠️ Potential issue | 🟠 MajorDon’t swallow clipboard failures with
console.error.
The current try/catch logs and continues, which violates the error-handling policy and hides failures. Let the rejection bubble or report viacaptureExceptionand rethrow.As per coding guidelines: "Do NOT silently swallow exceptions with try-catch-console.error blocks. Let unhandled exceptions bubble up to Sentry for error tracking. Handle only expected/recoverable errors explicitly with specific error codes. Use Sentry utilities (captureException, captureMessage) from 'src/utils/sentry' for explicit error reporting with context."🔧 Suggested fix
-const copyToClipboard = useCallback(async (text: string) => { - try { - await navigator.clipboard.writeText(text); - setShowCopiedNotification(true); - setTimeout(() => setShowCopiedNotification(false), 1500); - } catch (err) { - console.error('Failed to copy to clipboard:', err); - } -}, []); +const copyToClipboard = useCallback(async (text: string) => { + try { + await navigator.clipboard.writeText(text); + setShowCopiedNotification(true); + setTimeout(() => setShowCopiedNotification(false), 1500); + } catch (err) { + captureException(err, { tags: { area: 'terminal-output', action: 'copy' } }); + throw err; + } +}, []);+import { captureException } from '../utils/sentry';🤖 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 816 - 823, The catch in copyToClipboard currently swallows failures with console.error; update it to report the error via the project's Sentry helper and rethrow so the rejection can bubble: import and call captureException(err) from the Sentry util in the catch block (or remove the try/catch entirely), and ensure you rethrow the caught error instead of only logging it; keep the success path that sets setShowCopiedNotification and the timeout unchanged.
🧹 Nitpick comments (4)
src/renderer/hooks/utils/useDebouncedPersistence.ts (1)
128-128: The double cast is intentional and justified by design, but a proper type definition would improve maintainability.The double cast exists because
terminalTabsForPersistenceintentionally omits runtime-only fields (pid,state,exitCode,scrollTop) from the persisted structure. On app restart, these are repopulated from process events. While this design is sound and the cast works, defining aPersistedTerminalTabtype (containing onlyid,name,shellType,cwd,createdAt) would eliminate the need for the double cast and make the intent explicit:export type PersistedTerminalTab = Pick<TerminalTab, 'id' | 'name' | 'shellType' | 'cwd' | 'createdAt'>;Then return the constructed object as
PersistedTerminalTab[], requiring only a singleas Sessioncast when assigning to the final object.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/utils/useDebouncedPersistence.ts` at line 128, The double cast at the end of terminalTabsForPersistence can be replaced by introducing an explicit PersistedTerminalTab type to document the persisted shape: define export type PersistedTerminalTab = Pick<TerminalTab, 'id' | 'name' | 'shellType' | 'cwd' | 'createdAt'>, use PersistedTerminalTab[] as the return/variable type for terminalTabsForPersistence (so you only need a single as Session cast when assembling the final Session object), and update any references in useDebouncedPersistence.ts to use PersistedTerminalTab instead of relying on the double cast.src/renderer/components/NewInstanceModal.tsx (1)
456-487: Prefer the active terminal tab’s shell type when duplicating.Using the first tab can mismatch user expectations if the active tab uses a different shell. Consider prioritizing
activeTerminalTabIdwith a fallback to the first tab.♻️ Proposed tweak
- const sourceTerminalShellType = sourceSession?.terminalTabs?.[0]?.shellType; + const sourceTerminalShellType = + sourceSession?.terminalTabs?.find((tab) => tab.id === sourceSession.activeTerminalTabId) + ?.shellType ?? sourceSession?.terminalTabs?.[0]?.shellType;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/NewInstanceModal.tsx` around lines 456 - 487, The duplication uses the first terminal tab's shellType which can mismatch the active tab; update how sourceTerminalShellType is derived by checking sourceSession?.terminalTabs for the tab whose id equals sourceSession?.activeTerminalTabId and using its shellType, falling back to sourceSession?.terminalTabs?.[0]?.shellType if not found, then pass that (possibly undefined) value directly to onCreate (so you can call onCreate once with the computed sourceTerminalShellType) — reference sourceSession, terminalTabs, activeTerminalTabId, sourceTerminalShellType, and onCreate to locate the change.src/renderer/components/MainPanel.tsx (2)
1739-1757: Inline callbacks create new function references on every render.These inline arrow functions wrapping
props.onTerminalTab*callbacks will cause TerminalView to receive new function props on each render. If TerminalView or its children are memoized, this could defeat memoization.This is a common pattern and may be acceptable given xterm.js likely manages its own update cycle, but consider memoizing these with
useCallbackif performance profiling shows unnecessary re-renders.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/MainPanel.tsx` around lines 1739 - 1757, Inline arrow props (onTabSelect, onTabClose, onNewTab, onTabRename, onRequestRename, onTabReorder, onTabStateChange, onTabCwdChange, onTabPidChange) create new function references every render; memoize them with useCallback and reference props handlers (e.g., props.onTerminalTabSelect, props.onTerminalTabClose, etc.) and activeSession.id in dependency arrays, then pass those memoized handlers into TerminalView instead of the inline arrows; ensure onRequestTerminalTabRename is also memoized if it is a function prop being forwarded.
1735-1736: Use user-configurable font size instead of hardcoded value.The
fontSize={14}is hardcoded whilefontFamilyis passed from props. A user-configurablefontSizesetting exists in the settings hook (useSettings.ts) and is persisted, but it's not being passed to MainPanel or TerminalView. For consistency with howfontFamilyis handled, this should use the user-configured font size setting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/MainPanel.tsx` around lines 1735 - 1736, The fontSize is hardcoded to 14 in MainPanel when rendering TerminalView; use the user-configurable setting instead by sourcing the font size the same way fontFamily is handled—either accept a fontSize prop on MainPanel and forward it to TerminalView (fontSize={props.fontSize || activeTerminalTab?.fontSize || default}) or import/use the useSettings hook inside MainPanel to read settings.fontSize and pass that to TerminalView (fallback to 14 only if no setting/prop exists). Update the TerminalView prop usage (and any activeTerminalTab references) to consume this fontSize value rather than the literal 14.
🤖 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 62-83: spawnTerminalTab accepts cols/rows but they are never
forwarded because ProcessConfig lacks those fields and PtySpawner.spawn ignores
them; to fix, add cols?: number and rows?: number to the ProcessConfig
interface, update spawnTerminalTab (function spawnTerminalTab) to extract cols
and rows from config and pass them into this.spawn, and modify
PtySpawner.spawn() to read config.cols/config.rows and use those values instead
of the hardcoded cols: 100, rows: 30 so terminal sizing is honored.
- Around line 62-83: spawnTerminalTab currently hardcodes a default shell
('zsh') and ignores cols/rows from its config; replace the hardcoded fallback
with the existing platform-aware utility (e.g. import and call getDefaultShell()
or similar) and use its result when shell is not provided, and thread the cols
and rows values from spawnTerminalTab's config into the call to this.spawn so
the PtySpawner receives the requested dimensions (or remove cols/rows from the
config interface if sizing should remain fixed); update references in
spawnTerminalTab and the call-site of this.spawn to include cols and rows so
PtySpawner uses them.
In `@src/renderer/App.tsx`:
- Around line 8688-8705: The shellCommandHistory array is unbounded in the
setSessions updater for sessionId; mirror the aiCommandHistory cap by trimming
history to the most recent 50 entries after pushing trimmedCommand. In the
setSessions callback (the function that maps sessions and updates
state/busySource/shellCommandHistory), after creating history and pushing
trimmedCommand, ensure you slice or otherwise limit history to 50 items (e.g.,
keep history.slice(-50)) before returning the updated session object so
persisted session state cannot grow without bound.
- Around line 3795-3808: In handleClearActiveTerminal, avoid ignoring promise
rejections from window.maestro.process.write and guard against writing to
inactive tabs: after resolving terminalViewRef.current case, when finding
session via sessionsRef.current and activeTab via getActiveTerminalTab(session)
check that activeTab.state indicates an active/connected terminal and that
activeTab.pid (or equivalent PID field) is present/valid before calling
getTerminalSessionId and write; replace the current void
window.maestro.process.write(...) with an awaited call or attach a .catch to
handle/recover logging errors from the promise (e.g., try/catch around await or
window.maestro.process.write(...).catch(...)) so rejections are handled
gracefully.
In `@src/renderer/components/TerminalSearchBar.tsx`:
- Around line 22-91: The input auto-focus currently uses inputRef and useEffect;
change to a ref callback on the input element that sets inputRef.current and
immediately calls el?.focus() and el?.select() when isOpen is true, remove the
focus/select logic from the useEffect (keep the isOpen guard for other logic),
and add an explicit tabIndex (tabIndex={0} or tabIndex={-1} per guidelines) to
the input; ensure the input keeps the outline-none class and that inputRef,
useEffect referencing inputRef, and handleKeyDown remain coherent with the ref
callback approach (use inputRef for selection or other imperative calls).
In `@src/renderer/components/TerminalTabBar.tsx`:
- Around line 77-105: The tab item div in TerminalTabBar (the returned JSX block
for each tab) is not keyboard-focusable; add tabIndex={0} (or tabIndex={-1} if
you manage focus programmatically) and include the outline-none class in its
className to satisfy renderer accessibility guidelines, and where tabs should
autofocus use a ref callback like ref={(el) => el?.focus()} on the same element
(update handlers on the div such as onClick/onKeyDown as needed to preserve
keyboard interactions).
In `@src/renderer/components/TerminalTabRenameModal.tsx`:
- Around line 42-53: The Modal in TerminalTabRenameModal is using
MODAL_PRIORITIES.CONFIRM which conflicts with the dedicated layering for
terminal-tab renames; update the priority prop on the Modal (in
TerminalTabRenameModal) to use MODAL_PRIORITIES.TERMINAL_TAB_RENAME from
constants/modalPriorities.ts and ensure the component relies on the app's layer
stack (remove any local Escape handling if present) so the modal registers with
the correct layer instead of using the generic CONFIRM priority.
In `@src/renderer/components/TerminalView.tsx`:
- Around line 133-176: The catch block inside spawnPtyForTab should not silently
swallow errors; import and call the project's Sentry utilities (captureException
and/or captureMessage from src/utils/sentry) in the catch to report the failure
with context (include session.id, tab.id, terminalSessionId, and any relevant
payload like cwd/shell), then continue to run the existing cleanup (checking
getLatestTab and calling onTabStateChange/onTabPidChange) so state remains
consistent; reference the spawnPtyForTab function and the
window.maestro.process.spawnTerminalTab call when adding the Sentry capture.
In `@src/renderer/components/XTerminal.tsx`:
- Around line 195-210: The resize handler currently swallows errors (see
handleResize where addonsRef.current.fit.fit() and
window.maestro.process.resize(...) are called and the .catch(()=>{}) is used);
replace the silent swallow with explicit error reporting by importing and
calling the project's Sentry helper (captureException or captureMessage from
'src/utils/sentry') with context (sessionId, cols, rows) or remove the catch so
the error can bubble to global handlers; apply the same change to the other
silent-swallows in this file around the 243-257 region so WebGL/resize failures
are reported instead of being ignored.
- Around line 444-452: The terminal container div (the one using
ref={containerRef} and data attributes sessionId, theme.id, fontFamily,
fontSize) should be made focusable: add tabIndex={0} (or tabIndex={-1} if you
don't want it in the tab order) and include the outline-none class in its
className, and change the ref usage so the element is assigned to containerRef
and immediately focused for auto-focus (use a ref callback like ref={(el) => {
/* assign to containerRef and call el?.focus() */ }}). Ensure you keep the
existing data-* attributes (data-session-id, data-theme-id, data-font-family,
data-font-size) and preserve containerRef usage while implementing the ref
callback to set focus.
---
Outside diff comments:
In `@src/renderer/components/TerminalOutput.tsx`:
- Around line 816-823: The catch in copyToClipboard currently swallows failures
with console.error; update it to report the error via the project's Sentry
helper and rethrow so the rejection can bubble: import and call
captureException(err) from the Sentry util in the catch block (or remove the
try/catch entirely), and ensure you rethrow the caught error instead of only
logging it; keep the success path that sets setShowCopiedNotification and the
timeout unchanged.
---
Nitpick comments:
In `@src/renderer/components/MainPanel.tsx`:
- Around line 1739-1757: Inline arrow props (onTabSelect, onTabClose, onNewTab,
onTabRename, onRequestRename, onTabReorder, onTabStateChange, onTabCwdChange,
onTabPidChange) create new function references every render; memoize them with
useCallback and reference props handlers (e.g., props.onTerminalTabSelect,
props.onTerminalTabClose, etc.) and activeSession.id in dependency arrays, then
pass those memoized handlers into TerminalView instead of the inline arrows;
ensure onRequestTerminalTabRename is also memoized if it is a function prop
being forwarded.
- Around line 1735-1736: The fontSize is hardcoded to 14 in MainPanel when
rendering TerminalView; use the user-configurable setting instead by sourcing
the font size the same way fontFamily is handled—either accept a fontSize prop
on MainPanel and forward it to TerminalView (fontSize={props.fontSize ||
activeTerminalTab?.fontSize || default}) or import/use the useSettings hook
inside MainPanel to read settings.fontSize and pass that to TerminalView
(fallback to 14 only if no setting/prop exists). Update the TerminalView prop
usage (and any activeTerminalTab references) to consume this fontSize value
rather than the literal 14.
In `@src/renderer/components/NewInstanceModal.tsx`:
- Around line 456-487: The duplication uses the first terminal tab's shellType
which can mismatch the active tab; update how sourceTerminalShellType is derived
by checking sourceSession?.terminalTabs for the tab whose id equals
sourceSession?.activeTerminalTabId and using its shellType, falling back to
sourceSession?.terminalTabs?.[0]?.shellType if not found, then pass that
(possibly undefined) value directly to onCreate (so you can call onCreate once
with the computed sourceTerminalShellType) — reference sourceSession,
terminalTabs, activeTerminalTabId, sourceTerminalShellType, and onCreate to
locate the change.
In `@src/renderer/hooks/utils/useDebouncedPersistence.ts`:
- Line 128: The double cast at the end of terminalTabsForPersistence can be
replaced by introducing an explicit PersistedTerminalTab type to document the
persisted shape: define export type PersistedTerminalTab = Pick<TerminalTab,
'id' | 'name' | 'shellType' | 'cwd' | 'createdAt'>, use PersistedTerminalTab[]
as the return/variable type for terminalTabsForPersistence (so you only need a
single as Session cast when assembling the final Session object), and update any
references in useDebouncedPersistence.ts to use PersistedTerminalTab instead of
relying on the double cast.
Collaborator
|
This PR closes #160 |
calebsmithdev
force-pushed
the
symphony/issue-160-mlbwxdxc
branch
from
fd2f935 to
4770392
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/__tests__/renderer/constants/themes.test.ts (1)
145-179: Consider extracting hardcoded color values into named constants for maintainability.The tests at lines 160-172 assert specific hex values (e.g.,
'#bd93f9','#81a1c1'). While intentional for verifying distinctive palettes, these values are duplicated from the theme definitions insrc/shared/themes.ts.If palette values change during design iterations, both locations must be updated. Consider importing theme-specific ANSI constants (e.g.,
DRACULA_ANSI_COLORS,NORD_ANSI_COLORS) fromthemes.tsand comparing against those to reduce duplication and maintenance overhead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/constants/themes.test.ts` around lines 145 - 179, The tests duplicate hardcoded color literals for theme palettes; instead import the canonical palette constants from your theme module (e.g., DRACULA_ANSI_COLORS, NORD_ANSI_COLORS, GRUVBOX_DARK_ANSI_COLORS) and use those in assertions against THEMES (e.g., compare THEMES.dracula.colors.ansiBlue to DRACULA_ANSI_COLORS.ansiBlue) and selection colors to their corresponding constants, while keeping the existing distinctness checks versus DARK_ANSI_COLORS; update the imports at the top of the test file and replace the literal hex strings with references to the imported constants.
🤖 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/__tests__/renderer/components/TerminalOutput.test.tsx`:
- Around line 178-185: The test's expectation is wrong: TerminalOutput sets
backgroundColor using session.inputMode === 'ai' ? theme.colors.bgMain :
theme.colors.bgActivity, so when you create a session with inputMode: 'terminal'
the component should render bgActivity. Update the test in
TerminalOutput.test.tsx (the case that calls createDefaultSession({ inputMode:
'terminal' }) and asserts on defaultTheme.colors.bgMain) to expect
defaultTheme.colors.bgActivity instead, keeping the same props creation via
createDefaultProps and render call.
- Around line 212-221: The test fixture uses the obsolete "tabs" property
causing getActiveTab (and tabHelpers.ts) to not find tabs; update the session
fixture in TerminalOutput.test.tsx (and any createDefaultSession helper usage)
to set aiTabs: AITab[] instead of tabs, ensuring entries include id,
agentSessionId, logs, and isUnread so getActiveTab(session) and TerminalOutput
render the active tab logs correctly.
---
Duplicate comments:
In `@src/renderer/components/TerminalTabBar.tsx`:
- Around line 78-102: The tab divs in TerminalTabBar.tsx are interactive but not
keyboard-focusable; add tabIndex (use tabIndex={active ? 0 : -1} or tabIndex={0}
for all) and include the "outline-none" utility in the className, and for
auto-focus of the active tab use a ref callback like ref={(el) => active &&
el?.focus()} on the same div; ensure you reference the existing div with
data-testid={`terminal-tab-${tab.id}`} and preserve existing drag/click handlers
when adding these attributes.
In `@src/renderer/components/TerminalTabRenameModal.tsx`:
- Around line 42-53: The Modal in TerminalTabRenameModal should use the
dedicated modal priority constant instead of MODAL_PRIORITIES.CONFIRM: replace
the priority prop on the Modal (inside the TerminalTabRenameModal component) to
MODAL_PRIORITIES.TERMINAL_TAB_RENAME (from modalPriorities.ts); also ensure
you’re not handling Escape locally in this component so the modal is
registered/stacked correctly with the layer stack.
In `@src/renderer/components/TerminalView.tsx`:
- Around line 11-12: The catch that swallows PTY spawn failures in TerminalView
should capture the exception to Sentry before proceeding; import
captureException from 'src/utils/sentry' and, inside the existing catch where
the PTY/spawn is handled (the try-catch that initializes the terminal/PTY for
the XTerminal/XTerminalHandle), call captureException(err, { tags: { tabId: <tab
identifier> }, extra: { cmd: <command>, cwd: <cwd> } }) to surface the error,
then continue with the current cleanup/mark-exited logic (do not silently
swallow the error). Ensure the import and call reference the same symbols used
in this file (XTerminal, XTerminalHandle and the local tabId/cmd/cwd variables)
and avoid removing the existing cleanup code.
In `@src/renderer/components/XTerminal.tsx`:
- Around line 444-452: The terminal container JSX is not focusable or
auto-focused; update the <div> that uses containerRef (the element with
data-session-id={sessionId}, data-theme-id={theme.id},
data-font-family={fontFamily}, data-font-size={fontSize}) to include
tabIndex={0} (or tabIndex={-1} per desired tab behavior) and add the
outline-none class, and change the ref to an auto-focus callback that sets
containerRef and calls focus (e.g., ref={(el) => { containerRef.current = el;
el?.focus(); }}), ensuring the renderer focus guidance is satisfied.
- Around line 195-257: The code currently swallows resize promise rejections and
WebGL initialization errors; update handleResize and the WebGL init block to
report these to Sentry: import captureException/captureMessage from
'src/utils/sentry' and replace the no-op .catch(() => {}) on
window.maestro.process.resize(sessionId, cols, rows) with a .catch(err =>
captureException(err, { extra: { sessionId, cols, rows, location:
'XTerminal.handleResize' } })); in the WebglAddon try/catch, call
captureException(error, { extra: { location: 'XTerminal.WebglAddon.init' } }) in
the catch branch, and inside webglInstance.onContextLoss use captureMessage or
captureException to report the context loss (include sessionId and that you're
falling back to canvas) and still dispose and clear addonsRef.current.webgl as
now.
---
Nitpick comments:
In `@src/__tests__/renderer/constants/themes.test.ts`:
- Around line 145-179: The tests duplicate hardcoded color literals for theme
palettes; instead import the canonical palette constants from your theme module
(e.g., DRACULA_ANSI_COLORS, NORD_ANSI_COLORS, GRUVBOX_DARK_ANSI_COLORS) and use
those in assertions against THEMES (e.g., compare THEMES.dracula.colors.ansiBlue
to DRACULA_ANSI_COLORS.ansiBlue) and selection colors to their corresponding
constants, while keeping the existing distinctness checks versus
DARK_ANSI_COLORS; update the imports at the top of the test file and replace the
literal hex strings with references to the imported constants.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 257-273: handleTabClose currently awaits
window.maestro.process.kill(getTerminalSessionId(session.id, tabId)) but errors
can bubble as unhandled rejections; wrap the kill call with a try/catch (inside
the existing try/finally) or add a .catch to it and call captureException(error)
from the renderer-side ../utils/sentry so failures are reported; keep the
existing finally block that calls onTabClose(tabId) and ensure you import
captureException and reference getTerminalSessionId,
shellExitedTabsRef.current.delete, and focusedTabId unchanged.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/hooks/utils/useDebouncedPersistence.ts (1)
87-139:⚠️ Potential issue | 🟠 MajorRehydrate dropped terminal tab fields on session restore.
PersistedTerminalTabpersists only 5 fields (id,name,shellType,cwd,createdAt), droppingpid,state,exitCode,scrollTop, andsearchQuery. When sessions are restored inuseSessionRestoration.ts, the code only handles the case whereterminalTabsis empty; it does not rehydrate missing runtime fields on individual persisted tabs. This leaves themundefined, causing downstream code (e.g.,TerminalView.tsxcheckingtab.pid > 0ortab.state === 'exited') to behave unexpectedly. Also ensureactiveTerminalTabIdis validated against the restoredterminalTabslist to prevent pointing to non-existent tabs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/utils/useDebouncedPersistence.ts` around lines 87 - 139, PersistedTerminalTab only stored five fields so restore logic in useSessionRestoration.ts must rehydrate missing runtime fields (pid, state, exitCode, scrollTop, searchQuery) with safe defaults when reconstructing terminalTabs so downstream checks in TerminalView.tsx (e.g., tab.pid > 0 or tab.state === 'exited') behave predictably; update the restore path that consumes terminalTabsForPersistence/persistedSession to map each persisted tab to a full TerminalTab shape (e.g., pid: 0, state: 'exited' or other valid default from Session types, exitCode: undefined/null, scrollTop: 0, searchQuery: '') and also validate activeTerminalTabId against the restored terminalTabs (if missing, set activeTerminalTabId to the first restored tab id or undefined) so activeTerminalTabId never points at a non-existent tab.
🤖 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/spawners/PtySpawner.ts`:
- Around line 129-142: The logger.debug in the ptyProcess.onData handler is
currently logging raw terminal output via dataPreview; change it to avoid
including raw data by replacing dataPreview with a safe metadata field (e.g.,
dataLength: data.length) or guard inclusion behind an explicit opt‑in config
flag; update the ptyProcess.onData block (referencing ptyProcess.onData,
isTerminal, logger.debug, sessionId, ptyProcess.pid) so it only logs metadata
(sessionId, pid, dataLength) unless a new config/optIn flag is enabled, and
ensure the emitter.emit('data', sessionId, data) behavior remains unchanged.
- Around line 30-31: Pty dimensions (cols/rows) must be validated and clamped
before calling pty.spawn in class PtySpawner: ensure each value is a finite
positive integer (e.g., Number.isFinite and >0), coerce to an integer with
Math.floor, and fall back to sensible defaults (cols=100, rows=30) for invalid
values; apply this validation/normalization where cols and rows are passed to
pty.spawn (the cols/rows usage in PtySpawner) and the other place that also
passes cols/rows to pty.spawn so zero, negative, NaN or infinite values never
reach node-pty.
In `@src/renderer/components/TerminalOutput.tsx`:
- Line 34: In copyToClipboard, stop rethrowing the error after calling
captureException so the function doesn't cause unhandled promise rejections in
fire-and-forget onClick handlers; instead, call captureException(error) and then
return (or return a resolved value) to swallow the error. Locate the
copyToClipboard function and remove the throw statement that follows
captureException, ensuring the error is logged but not rethrown to callers (used
by onClick handlers around line ~830).
In `@src/renderer/components/TerminalView.tsx`:
- Around line 140-148: The spawnTerminalTab call currently always uses
defaultShell and doesn't pass SSH config; update the call to use the per-tab
shell by replacing the shell argument with tab.shellType ?? defaultShell
(referencing spawnTerminalTab, terminalSessionId, tab.shellType, defaultShell),
and then extend the spawnTerminalTab API signature and the ProcessManager
handler to accept and forward sessionSshRemoteConfig (mirror how
process:runCommand handles SSH by adding a sessionSshRemoteConfig parameter
through the IPC/handler and into the PTY spawn path) so SSH sessions get the
config when terminal tabs are created.
---
Outside diff comments:
In `@src/renderer/hooks/utils/useDebouncedPersistence.ts`:
- Around line 87-139: PersistedTerminalTab only stored five fields so restore
logic in useSessionRestoration.ts must rehydrate missing runtime fields (pid,
state, exitCode, scrollTop, searchQuery) with safe defaults when reconstructing
terminalTabs so downstream checks in TerminalView.tsx (e.g., tab.pid > 0 or
tab.state === 'exited') behave predictably; update the restore path that
consumes terminalTabsForPersistence/persistedSession to map each persisted tab
to a full TerminalTab shape (e.g., pid: 0, state: 'exited' or other valid
default from Session types, exitCode: undefined/null, scrollTop: 0, searchQuery:
'') and also validate activeTerminalTabId against the restored terminalTabs (if
missing, set activeTerminalTabId to the first restored tab id or undefined) so
activeTerminalTabId never points at a non-existent tab.
…lize terminal dimensions
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/main/process-manager/ProcessManager.ts (1)
150-165: Consider extracting shared shell argument parsing utility.The
parseShellArgslogic here duplicates the quoted-argument parsing inPtySpawner.ts(lines 71-87). Both handle the same pattern of tokenizing shell arguments while stripping quotes.💡 Suggested approach
Extract to a shared utility (e.g., in
src/main/utils/shell-escape.tsalongsidebuildShellCommand):export function parseShellArgs(shellArgs?: string): string[] { if (!shellArgs || !shellArgs.trim()) { return []; } const parsedArgs = shellArgs.match(/(?:[^\s"']+|"[^"]*"|'[^']*')+/g) || []; return parsedArgs.map((arg) => { if ((arg.startsWith('"') && arg.endsWith('"')) || (arg.startsWith("'") && arg.endsWith("'"))) { return arg.slice(1, -1); } return arg; }); }Then import and use in both
ProcessManager.tsandPtySpawner.ts.🤖 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 150 - 165, The parseShellArgs implementation in ProcessManager.parseShellArgs duplicates logic in PtySpawner (tokenizing quoted shell args); extract this into a shared utility function (e.g., export function parseShellArgs in a new module alongside buildShellCommand) and replace both ProcessManager.parseShellArgs and the equivalent code in PtySpawner with imports of that shared parseShellArgs to avoid duplication and keep behavior identical.src/main/ipc/handlers/process.ts (1)
77-90: Consider importingSpawnTerminalTabConfigfrom preload to avoid duplication.This type is identical to
SpawnTerminalTabConfigdefined insrc/main/preload/process.ts(lines 55-68). Importing the shared type would reduce maintenance burden and ensure consistency.♻️ Suggested refactor
+import type { SpawnTerminalTabConfig } from '../../preload/process'; // ... at line 77 - type SpawnTerminalTabConfig = { - sessionId: string; - cwd: string; - shell?: string; - shellArgs?: string; - shellEnvVars?: Record<string, string>; - cols?: number; - rows?: number; - sessionSshRemoteConfig?: { - enabled: boolean; - remoteId: string | null; - workingDirOverride?: string; - }; - };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/process.ts` around lines 77 - 90, Duplicate type definition: remove the local SpawnTerminalTabConfig type and import the single source-of-truth type from the preload module instead; update the file to import SpawnTerminalTabConfig (the same exported type defined in preload) and replace the local type declaration with that import, ensuring any references (e.g., functions or handlers using SpawnTerminalTabConfig) keep their types and adjust exports/imports accordingly so the type is not redefined locally.src/__tests__/main/ipc/handlers/process.test.ts (1)
42-51: Mock defaultmodelSourcemay be inconsistent with test overrides.The base mock at line 45 returns
modelSource: 'none', but tests at lines 1185 and 1645 override it tomodelSource: 'default'. If'default'is the correct production behavior, consider updating the base mock for consistency.♻️ Suggested fix
vi.mock('../../../../main/utils/agent-args', () => ({ buildAgentArgs: vi.fn((agent, opts) => opts.baseArgs || []), applyAgentConfigOverrides: vi.fn((agent, args, opts) => ({ args, - modelSource: 'none' as const, + modelSource: 'default' as const, customArgsSource: 'none' as const, customEnvSource: 'none' as const, effectiveCustomEnvVars: undefined, })), getContextWindowValue: vi.fn(() => 0), }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/ipc/handlers/process.test.ts` around lines 42 - 51, The base mock for applyAgentConfigOverrides currently returns modelSource: 'none', which contradicts tests that override to modelSource: 'default'; update the mock in applyAgentConfigOverrides to return modelSource: 'default' (and ensure customArgsSource/customEnvSource values remain consistent) so the default mock behavior matches the tests; locate the mock setup around buildAgentArgs/applyAgentConfigOverrides/getContextWindowValue and change the modelSource literal there.
🤖 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/ipc/handlers/process.ts`:
- Around line 579-614: The process:spawnTerminalTab handler doesn't apply global
settings (customShellPath and shellEnvVars), causing terminal tabs to differ
from process:spawn; fix it by loading the same settings used in process:spawn
and passing them into processManager.spawnTerminalTab: retrieve settingsStore
values (customShellPath and shellEnvVars), compute shellToUse = config.shell ||
customShellPath || getDefaultShell(), mergeShellEnvVars = {
...settings.shellEnvVars, ...config.shellEnvVars }, and include shell:
shellToUse and shellEnvVars: mergeShellEnvVars in the object passed to
spawnTerminalTab (the call in process:spawnTerminalTab that invokes
processManager.spawnTerminalTab). Also keep existing SSH handling
(sshRemoteConfig) as-is.
---
Nitpick comments:
In `@src/__tests__/main/ipc/handlers/process.test.ts`:
- Around line 42-51: The base mock for applyAgentConfigOverrides currently
returns modelSource: 'none', which contradicts tests that override to
modelSource: 'default'; update the mock in applyAgentConfigOverrides to return
modelSource: 'default' (and ensure customArgsSource/customEnvSource values
remain consistent) so the default mock behavior matches the tests; locate the
mock setup around buildAgentArgs/applyAgentConfigOverrides/getContextWindowValue
and change the modelSource literal there.
In `@src/main/ipc/handlers/process.ts`:
- Around line 77-90: Duplicate type definition: remove the local
SpawnTerminalTabConfig type and import the single source-of-truth type from the
preload module instead; update the file to import SpawnTerminalTabConfig (the
same exported type defined in preload) and replace the local type declaration
with that import, ensuring any references (e.g., functions or handlers using
SpawnTerminalTabConfig) keep their types and adjust exports/imports accordingly
so the type is not redefined locally.
In `@src/main/process-manager/ProcessManager.ts`:
- Around line 150-165: The parseShellArgs implementation in
ProcessManager.parseShellArgs duplicates logic in PtySpawner (tokenizing quoted
shell args); extract this into a shared utility function (e.g., export function
parseShellArgs in a new module alongside buildShellCommand) and replace both
ProcessManager.parseShellArgs and the equivalent code in PtySpawner with imports
of that shared parseShellArgs to avoid duplication and keep behavior identical.
Author
|
@pedramamini Code has been merged in and the CodeRabbit changes have been made - it's in a good state now for review. The same issue applies above, where I don't have a good way to get back to a session once the terminal is initiated. I am open to suggestions and can get them implemented with some tokens. |
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
2 participants
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.
Note: This is a Maestro Symphony PR
Summary
Changes by Groupings
Foundation (Phases 1-2)
XTerminalwith lifecycle/addon wiring.TerminalTab,ClosedTerminalTab) and terminal tab helper utilities.Backend Changes (Phases 3-4)
terminalTabs+activeTerminalTabId.process:spawnTerminalTabIPC + preload bridge and renderer typing.Frontend Integration (Phases 5-7)
TerminalTabBarandTerminalView; integrated terminal-mode rendering inMainPanel.Features (Phases 8-11)
Cmd/Ctrl+F,Cmd/Ctrl+G).Cleanup and Testing/Polish (Phases 12-13)
TerminalOutputas AI-log-only rendering.CLAUDE.md,CONTRIBUTING.md,docs/keyboard-shortcuts.md).Testing Recap (Doc References)
Testing and verification details are documented in the same folder as the implementation README:
./docs/Terminal-13.md(comprehensive test and polish validation)./docs/Terminal-12.md(cleanup/migration verification)./docs/Terminal-10.md(theme/ANSI verification)./docs/Terminal-9.md(terminal search verification)./docs/Terminal-8.md(keyboard shortcut verification)./docs/Terminal-4.md(main-process/IPC verification)Screenshots
Summary by CodeRabbit
New Features
Documentation