[WIP] Symphony: Improved Terminal Experience (#160)

[pedramamini]

Maestro Symphony Contribution

Working on #160 via Maestro Symphony.

Status: In Progress
Started: 2026-01-09T13:22:23.820Z

---

This PR will be updated automatically when the Auto Run completes.

[claude]

Code Review: XTerminal Component (Phase 1)

Thanks for this well-structured PR implementing the foundation for full terminal emulation! This is a solid Phase 1 that sets up xterm.js properly. Here's my detailed feedback:

✅ Strengths

1. Excellent Documentation: The file header comment clearly explains what the component manages - this will help future maintainers.

2. Proper Lifecycle Management: Terminal instance creation and cleanup is handled correctly with proper disposal of all addons.

3. Theme Integration: The mapMaestroThemeToXterm function intelligently maps Maestro themes to xterm colors with sensible dark/light defaults.

4. WebGL with Fallback: The try-catch for WebGL addon with context loss handling and canvas fallback is robust.

5. Debounced Resize: Using 100ms debounce for resize operations prevents performance issues.

6. Imperative Handle: The XTerminalHandle interface provides a clean API for parent components to control the terminal.

🔍 Issues & Concerns

1. IPC Handler Missing (Critical)

The component calls window.maestro.process.resize(sessionId, cols, rows) but doesn't verify this handler exists. Need to check:

• Is the resize IPC handler implemented in src/main/index.ts?
• Does it route to the correct PTY process?

Recommendation: Add this to src/main/index.ts:

ipcMain.handle('process:resize', async (_, sessionId: string, cols: number, rows: number) => {
  const session = activeSessions.get(sessionId);
  if (session?.pty) {
    session.pty.resize(cols, rows);
  }
});

2. Session ID Format Confusion

The prop type says sessionId should be {sessionId}-terminal-{tabId} format, but:

• The code uses it directly in window.maestro.process.write(sessionId, data)
• The onData handler compares sid === sessionId
• This suggests the suffix format may not be needed, or the comparison logic is wrong

Recommendation: Clarify in CLAUDE.md or code comments which format is actually expected. If the terminal suffix is needed, update the comparison logic.

3. Memory Leak Risk: ResizeObserver

The ResizeObserver is created with handleResize in the dependency array, which means a new observer is created every time handleResize changes (whenever sessionId or onResize changes). This could create orphaned observers.

Fix:

// Remove handleResize from dependency array
useEffect(() => {
  if (!containerRef.current) return;
  
  const resizeObserver = new ResizeObserver(() => {
    // Inline the resize logic or use a stable ref
    if (resizeTimeoutRef.current) clearTimeout(resizeTimeoutRef.current);
    resizeTimeoutRef.current = setTimeout(() => {
      if (fitAddonRef.current && terminalRef.current) {
        fitAddonRef.current.fit();
        const { cols, rows } = terminalRef.current;
        onResize?.(cols, rows);
        window.maestro.process.resize(sessionId, cols, rows);
      }
    }, 100);
  });
  
  resizeObserver.observe(containerRef.current);
  return () => resizeObserver.disconnect();
}, []); // Empty deps - only create once

4. Timeout Cleanup Missing

The resizeTimeoutRef is never cleared on unmount, which could cause setState calls on unmounted components.

Fix: Add cleanup to the main initialization effect:

return () => {
  if (resizeTimeoutRef.current) clearTimeout(resizeTimeoutRef.current);
  titleDisposable.dispose();
  webglAddon?.dispose();
  term.dispose();
  // ...
};

5. Data Handler Subscription Cleanup

The onData handler from window.maestro.process.onData is subscribed in one useEffect, but it depends on sessionId. If sessionId changes, the old subscription may not be cleaned up properly if the IPC mechanism doesn't handle it.

Recommendation: Verify that the unsubscribe function returned by window.maestro.process.onData properly removes the specific listener, not all listeners.

6. Unicode11 Activation

term.unicode.activeVersion = '11' is set unconditionally, but the unicode property may not exist if the addon fails to load.

Fix:

try {
  const unicode11Addon = new Unicode11Addon();
  term.loadAddon(unicode11Addon);
  term.unicode.activeVersion = '11';
} catch (e) {
  console.warn('[XTerminal] Unicode11 addon failed to load', e);
}

🤔 Questions for Clarification

1. Integration Plan: This component isn't used anywhere yet. When will it replace the current terminal rendering in TerminalOutput.tsx? Should the connection be part of this PR or a follow-up?

2. PTY Session Handling: How does this component coordinate with the existing PTY processes? The current system spawns both aiPid and terminalPid - which PID should this component connect to?

3. Search Addon: The SearchAddon is loaded but the imperative handle's search methods aren't integrated with Maestro's existing Cmd+F search functionality in TerminalOutput.tsx. Is this planned for Phase 2?

4. Shell vs AI Mode: The current TerminalOutput.tsx handles both shell logs (session.shellLogs) and AI logs (session.aiTabs[].logs). Will XTerminal only replace shell mode, or both?

📋 Suggestions for Phase 2

1. Copy Mode: Add support for xterm's selection mode (terminal.select()) to integrate with the existing copy functionality.

2. Link Handling: The WebLinksAddon is loaded but could be configured with a custom link handler to integrate with Maestro's file linking system.

3. Keyboard Shortcuts: Integrate with Maestro's existing keyboard shortcuts (Cmd+F for search, etc.) from src/renderer/constants/shortcuts.ts.

4. Performance Metrics: Consider adding performance monitoring for WebGL context losses and canvas fallbacks (could integrate with src/shared/performance-metrics.ts).

🔒 Security Notes

✅ No security concerns - the component properly sanitizes input through xterm.js's built-in handling and doesn't execute arbitrary commands directly.

📝 Code Quality

• TypeScript: ✅ Well-typed with proper interfaces
• React Patterns: ✅ Proper hooks usage, forwardRef, and imperative handle
• Comments: ✅ Excellent inline documentation
• Performance: ⚠️ Some optimization opportunities (see memory leak issues above)

🧪 Testing Recommendations

Before merging, test:

1. Rapid window resizing (check for memory leaks)
2. Theme switching while terminal is active
3. Long-running commands with continuous output (scrollback buffer)
4. Unicode/emoji rendering (verify Unicode11 addon works)
5. WebGL context loss recovery (simulate by changing GPU settings)
6. Session switching with multiple terminals

🎯 Overall Assessment

This is a solid foundation for terminal emulation with xterm.js. The main concerns are:

• Memory leak in ResizeObserver (needs fix)
• Missing IPC handler verification (critical for functionality)
• Unclear integration path with existing codebase

Recommendation: Request changes to fix the memory leak and clarify the IPC integration, then approve. The code quality is high and the architecture is sound.

---

Would love to see the integration plan for Phase 2! This PR sets up xterm.js well, but it needs to be wired into the existing session management system to be functional.

[claude]

Pull Request Review: Improved Terminal Experience (#160)

Overview

This PR implements Phase 1 & 2 of the terminal emulation enhancement, introducing xterm.js foundation and multi-tab terminal support. The implementation is well-structured and follows established patterns from the AI tabs system.

✅ Strengths

Code Quality

• Excellent documentation: Both XTerminal.tsx and terminalTabHelpers.ts have comprehensive JSDoc comments
• Consistent patterns: Terminal tab helpers mirror the established tabHelpers.ts pattern for AI tabs, ensuring maintainability
• Clean separation of concerns: XTerminal component handles terminal lifecycle, helpers handle state management
• Proper TypeScript usage: Interfaces are well-defined with clear optionality (TerminalTab, ClosedTerminalTab)

Architecture

• Migration path: Deprecated fields (shellLogs, terminalPid) are clearly marked with migration comments
• Graceful fallbacks: WebGL renderer falls back to canvas with proper error handling (XTerminal.tsx:154-167)
• Imperative handle pattern: Proper use of forwardRef and useImperativeHandle for parent component control

🔍 Issues & Recommendations

1. Memory Leak Risk in XTerminal (Medium Priority)

Location: src/renderer/components/XTerminal.tsx:106

The resizeTimeoutRef timeout is not cleared on component unmount:

// Missing cleanup
const resizeTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);

Fix: Add cleanup in the main useEffect:

return () => {
  if (resizeTimeoutRef.current) {
    clearTimeout(resizeTimeoutRef.current);
  }
  titleDisposable.dispose();
  webglAddon?.dispose();
  term.dispose();
  // ...
};

2. Missing Dependencies in useCallback (Medium Priority)

Location: src/renderer/components/XTerminal.tsx:110-123

The handleResize callback is missing onResize in its dependency array, which could cause stale closure issues:

const handleResize = useCallback(() => {
  // ...
}, [sessionId, onResize]); // Add onResize here

This violates React Hooks exhaustive-deps rule and could lead to calling stale onResize references.

3. Empty Dependency Array Concern (Low Priority)

Location: src/renderer/components/XTerminal.tsx:192

The initialization useEffect has an empty dependency array with this comment:

}, []); // Only run on mount - theme/font updates handled separately

While the theme/font are handled in separate effects, containerRef, fontFamily, fontSize, and theme are used during initialization. If the component is being used in a context where these props change during mount (e.g., hot module reloading), this could cause issues.

Recommendation: Consider if this is intentional. If so, the comment is good. Otherwise, add a comment explaining why initial values are locked in.

4. Type Safety: Missing Type Import (Low Priority)

Location: src/renderer/components/XTerminal.tsx:20

The Theme type is imported from '../../shared/theme-types' but I don't see this file in the diff. Verify this path exists and the import is correct.

5. Terminal Tab State Consistency (Low Priority)

Location: src/renderer/utils/terminalTabHelpers.ts:81-86

When creating a ClosedTerminalTab, the PID is reset to 0 and state to 'idle':

tab: { ...tab, pid: 0, state: 'idle' },

This is correct since PTY sessions can't be restored, but consider if exitCode should also be cleared since it's no longer meaningful for a "reopenable" terminal that will start fresh.

6. Regex Vulnerability (Very Low Risk)

Location: src/renderer/utils/terminalTabHelpers.ts:59

The regex pattern /^(.+)-terminal-(.+)$/ uses greedy quantifiers with .+. For maliciously crafted session IDs with many hyphens, this could cause catastrophic backtracking.

Recommendation: Use a more specific pattern:

const match = terminalSessionId.match(/^([^-]+(?:-[^-]+)*)-terminal-([^-]+)$/);

However, given that session IDs are generated by generateId(), this is extremely low risk in practice.

🎯 Performance Considerations

Positive

• ✅ Debounced resize handler (100ms) prevents excessive reflows
• ✅ WebGL rendering enabled for GPU acceleration
• ✅ Scrollback limited to 10,000 lines to prevent memory bloat

Areas to Monitor

• The ResizeObserver fires frequently during drag operations. The 100ms debounce is reasonable but consider if visual lag is noticeable
• WebGL context loss is handled, but ensure webglAddonRef.current = null prevents memory leaks from disposed addons

🔒 Security Assessment

No Critical Issues

• ✅ No command injection risks (uses IPC for PTY communication)
• ✅ No XSS vectors (xterm.js handles output rendering safely)
• ✅ No direct shell execution in renderer process

🧪 Test Coverage Recommendations

The PR doesn't include tests. Consider adding:

1. Unit tests for terminalTabHelpers.ts:

   • Test parseTerminalSessionId with edge cases (no hyphens, multiple -terminal- substrings)
   • Test createTerminalTab default values
   • Test getActiveTerminalTab fallback behavior

2. Integration tests for XTerminal:

   • Test theme changes update terminal colors
   • Test font changes trigger resize
   • Test cleanup on unmount (verify no memory leaks)

📋 Minor Suggestions

1. Consistency: The max closed tabs constant differs between AI tabs (25) and terminal tabs (10). Document why terminal tabs have a lower limit.

2. Type Export: Consider exporting TerminalTab and ClosedTerminalTab from a central types file for consistency with AITab.

3. Error Handling: Add try-catch around term.loadAddon() calls for non-WebGL addons in case future xterm.js versions have breaking changes.

✨ Conclusion

Overall Assessment: This is high-quality foundational work that follows the project's established patterns well. The issues identified are relatively minor and mostly preventative.

Recommendation: ✅ Approve with minor changes

Required Before Merge:

1. Fix the memory leak in resizeTimeoutRef cleanup
2. Fix the handleResize dependency array

Nice to Have:

3. Address the empty dependency array concern (add clarifying comment if intentional)
4. Add basic unit tests for terminal tab helpers

The terminal emulation foundation is solid and ready for Phase 3+ implementations. Great work maintaining consistency with existing patterns! 🎉

[claude]

PR Review: Improved Terminal Experience (#160)

This PR introduces multi-tab terminal support with xterm.js integration. Here are my findings:

✅ Strengths

1. Well-Structured Architecture - Clean separation with terminalTabHelpers.ts, proper React patterns
2. Excellent Migration Strategy - Backwards compatibility in App.tsx:842-858 handles sessions without terminal tabs
3. Robust XTerminal Component - WebGL fallback, proper cleanup, debounced resize, theme sync
4. Security Conscious - No shell injection vectors, proper IPC boundaries

⚠️ Critical Issues

1. Missing Main Process Implementation
The PR adds renderer-side components but no corresponding main process changes:

• Missing: PTY spawning logic for multi-tab support in process-manager.ts
• Missing: IPC handlers for window.maestro.process.resize (XTerminal.tsx:120)
• Missing: Tab-specific PTY routing
• Impact: Code will compile but cannot function without backend

2. Memory Leak in XTerminal.tsx
resizeTimeoutRef is not cleared in cleanup (line 183-191). Add clearTimeout in useEffect cleanup.

3. Type Safety Issues
terminalTabs and activeTerminalTabId are optional but migration ensures they are always populated. Should be required fields.

4. Missing UI Components
No tab bar, keyboard shortcuts, or user interaction handlers for terminal tabs.

5. No Error Handling
Missing error handling for window.maestro.process.resize and write failures.

🧪 Testing Gaps

• No unit tests for terminalTabHelpers.ts
• No integration tests for terminal tab lifecycle
• No tests for migration logic (critical!)

📊 Performance Concerns

• Multiple PTY processes = memory multiplier
• xterm.js + WebGL = GPU memory per tab
• 10k scrollback × N tabs = potential memory issues

Recommendations: configurable scrollback, lazy-load terminals, tab limits

✅ Before Merge

Blockers:

1. Add main process PTY management
2. Fix memory leak in resize timeout
3. Make terminalTabs/activeTerminalTabId required
4. Add error handling for IPC

Should Have:
5. Add UI components for terminal tabs
6. Add tests for migration logic
7. Update CLAUDE.md

🎯 Assessment

• Code Quality: B+ (well-structured but incomplete)
• Completeness: 40% (renderer only, missing backend + UI)
• Risk: Medium-High

Verdict: ⚠️ Needs Work - Solid foundation but incomplete. Either add feature flag + mark WIP, OR complete with main process implementation.

---

Reviewed by Claude Code

[claude]

PR Review: Improved Terminal Experience (#160)

Overview

This PR introduces multi-tab terminal support with xterm.js for full PTY emulation. This is a significant architectural improvement that brings proper terminal emulation to Maestro.

✅ Strengths

1. Clean Architecture

• Well-organized separation between main process (PTY management) and renderer (xterm.js UI)
• New XTerminal.tsx component is self-contained with proper lifecycle management
• Helper utilities in terminalTabHelpers.ts mirror the existing tabHelpers.ts pattern nicely

2. Type Safety

• New TerminalTab interface is well-defined with clear field documentation
• Proper type definitions exported through preload.ts
• Good use of TypeScript discriminated unions for tab state ('idle' | 'busy' | 'exited')

3. Backwards Compatibility

• Migration logic in App.tsx:838-857 handles sessions without terminal tabs gracefully
• Creates default terminal tab for legacy sessions
• Resets runtime state (PIDs, process state) on app restart - excellent!

4. Resource Management

• Proper cleanup in XTerminal.tsx with disposables and effect cleanup functions
• WebGL renderer with graceful fallback to canvas
• PTY process lifecycle properly managed

5. User Experience

• Theme synchronization between Maestro themes and xterm.js
• Font customization support
• Closed tab undo functionality (max 10 tabs)
• Search functionality built-in

⚠️ Issues & Concerns

1. Critical: Missing File

The XTerminal.tsx component is truncated in the diff (line 242: "// Expose imperative handle f"). This suggests the file is incomplete or the diff was cut off. This needs to be verified before merge.

// src/renderer/components/XTerminal.tsx:242
// Expose imperative handle f

Action Required: Ensure the complete XTerminal.tsx file is included in the PR.

2. Security: Shell Execution

In process-manager.ts:1378-1422, the spawnTerminalTab method uses shell configuration from settings:

const shellToUse = config.shell || settingsStore.get('defaultShell', 'zsh');
const customShellPath = settingsStore.get('customShellPath', '');

Concern: If customShellPath is not validated, a malicious settings file could execute arbitrary commands.

Recommendation: Add path validation to ensure it points to a legitimate shell executable:

import { existsSync } from 'fs';
import { resolve } from 'path';

if (customShellPath && customShellPath.trim()) {
  const resolvedPath = resolve(customShellPath.trim());
  if (!existsSync(resolvedPath)) {
    logger.warn('[ProcessManager] Custom shell path does not exist', 'ProcessManager', { path: resolvedPath });
    // Fall back to default
  } else {
    shellToUse = resolvedPath;
  }
}

3. Performance: Debounce Tuning

In XTerminal.tsx:121, resize is debounced at 100ms:

}, 100); // 100ms debounce

Observation: This is reasonable, but consider if it should match the persistence debounce (2000ms in useDebouncedPersistence.ts) or be configurable.

Recommendation: Consider extracting to a constant for consistency:

const RESIZE_DEBOUNCE_MS = 100;

4. Memory: Scrollback Limit

In XTerminal.tsx:136, scrollback is set to 10,000 lines:

scrollback: 10000, // 10k lines of scrollback

Concern: With multiple terminal tabs, this could consume significant memory (10k lines × number of tabs × terminal buffer size).

Recommendation: Either:

1. Make scrollback configurable in settings
2. Document memory implications
3. Consider a more conservative default (e.g., 5000)

5. Bug: Terminal Data Routing Logic

In process-manager.ts:517-526, there's a conditional check for terminal mode:

// For terminal mode with xterm.js, send RAW data - xterm handles all rendering
// This includes control sequences, colors, cursor movement, etc.
if (isTerminal) {
  logger.debug('[ProcessManager] PTY onData (raw terminal)', 'ProcessManager', {
    sessionId,
    pid: ptyProcess.pid,
    dataLength: data.length,
  });
  this.emit('data', sessionId, data);
  return;
}

Issue: The isTerminal variable is derived from toolType === 'terminal' earlier in the function. However, for the new terminal tabs, the sessionId format is {sessionId}-terminal-{tabId}, but toolType might still be the parent session's tool type.

Recommendation: Verify that isTerminal is correctly detected for terminal tab sessions. Consider using:

const isTerminal = toolType === 'terminal' || sessionId.includes('-terminal-');

6. Race Condition: Terminal Tab Spawning

In App.tsx, terminal tabs are created during session initialization but spawning happens later. There's no explicit tracking of when the PTY is ready.

Concern: UI might attempt to write to a terminal before the PTY process is spawned.

Recommendation: Add state tracking for PTY readiness or queue writes until PTY is confirmed spawned.

7. Inconsistent State Management

The TerminalTab interface includes state: 'idle' | 'busy' | 'exited', but I don't see where state transitions from 'idle' to 'busy' when commands are running.

Missing: Process output parsing to detect when commands start/end.

Recommendation: Either:

1. Add a mechanism to track command execution state (similar to AI agent busy state)
2. Remove the 'busy' state if not yet implemented
3. Document that this is a future enhancement

8. Missing Error Handling

In XTerminal.tsx, there's no error boundary or try-catch around critical operations like:

• Terminal initialization
• Addon loading
• IPC communication

Recommendation: Wrap the terminal initialization in try-catch and provide user feedback if initialization fails.

🧪 Test Coverage

Missing:

• Unit tests for terminalTabHelpers.ts functions
• Integration tests for terminal tab lifecycle
• Tests for backwards compatibility migration
• Tests for PTY session ID parsing

Recommendation: Add test coverage before merging, especially for:

describe('terminalTabHelpers', () => {
  test('parseTerminalSessionId handles valid format', () => {
    const result = parseTerminalSessionId('session123-terminal-tab456');
    expect(result).toEqual({ sessionId: 'session123', tabId: 'tab456' });
  });

  test('parseTerminalSessionId handles invalid format', () => {
    const result = parseTerminalSessionId('invalid-format');
    expect(result).toBeNull();
  });
});

📝 Documentation

Missing:

• Update CLAUDE.md with terminal tab architecture
• Document the PTY session ID format ({sessionId}-terminal-{tabId})
• Add keyboard shortcuts for terminal tab management
• Document memory considerations for multiple terminal tabs

Suggestion: Add a section to CLAUDE.md:

### 10. Terminal Tabs (NEW)

Each session supports multiple terminal tabs with full PTY emulation:

\`\`\`typescript
session.terminalTabs: TerminalTab[]
session.activeTerminalTabId: string

interface TerminalTab {
  id: string;
  pid: number;              // PTY process ID
  shellType: string;        // 'zsh', 'bash', etc.
  state: 'idle' | 'busy' | 'exited';
}

// PTY session ID format: {sessionId}-terminal-{tabId}
const ptySessionId = getTerminalSessionId(session.id, tab.id);
\`\`\`

🔒 Security Review

1. ✅ No obvious XSS vulnerabilities (xterm.js handles escaping)
2. ⚠️ Shell path injection risk (see issue Add Claude Code GitHub Workflow #2 above)
3. ✅ IPC handlers properly registered with error logging
4. ✅ No SQL injection (not using SQL here)
5. ✅ Proper use of execFileNoThrow pattern in IPC handlers

🎯 Recommendations Summary

Before Merge:

1. ✅ Verify XTerminal.tsx is complete (truncated in diff)
2. ⚠️ Add shell path validation to prevent arbitrary command execution
3. 🧪 Add basic test coverage for helper functions
4. 📝 Update CLAUDE.md with terminal tab documentation

Post-Merge:

1. Implement command execution state tracking (busy detection)
2. Add configurable scrollback limit setting
3. Add error boundary around terminal initialization
4. Consider memory profiling with many terminal tabs open

🎉 Verdict

This is a solid foundation for improved terminal experience. The architecture is clean, type-safe, and follows existing patterns. However, there are some important issues to address:

• Critical: Verify XTerminal.tsx completeness
• High Priority: Add shell path validation
• Medium Priority: Add test coverage and documentation

Recommendation: Request changes to address critical and high-priority items before merge.

---

Code Quality: ⭐⭐⭐⭐☆ (4/5)
Security: ⭐⭐⭐☆☆ (3/5 - shell path validation needed)
Test Coverage: ⭐⭐☆☆☆ (2/5 - missing tests)
Documentation: ⭐⭐⭐☆☆ (3/5 - needs CLAUDE.md update)

Great work on this feature! Looking forward to seeing it merged. 🚀

[claude]

PR Review - Terminal Emulation Foundation

This PR implements a solid foundation for xterm.js-based terminal emulation. The architecture is well-structured and follows Maestro's established patterns. Here's my detailed feedback:

---

✅ Strengths

1. Excellent Architectural Consistency

• Follows existing patterns from AI tab management (mirroring tabHelpers.ts with terminalTabHelpers.ts)
• Type system is comprehensive and well-documented
• Migration logic for backwards compatibility is well-handled
• Proper use of createTerminalTab in all session creation points

2. xterm.js Integration

• Clean separation of concerns in XTerminal.tsx
• Proper addon lifecycle management (fit, webgl, search, web-links, unicode11)
• WebGL renderer with canvas fallback is a good defensive pattern
• Theme mapping from Maestro to xterm is thoughtful and comprehensive

3. Process Management

• Good use of spawnTerminalTab convenience method
• RAW output mode for terminal sessions is correct (no filtering for xterm.js)
• Session ID format ({sessionId}-terminal-{tabId}) is clear and parseable

4. UI/UX Considerations

• Tab state visualization (green/red/yellow icons) is intuitive
• Exit code display for failed processes is helpful
• Overflow handling with sticky "new tab" button is polished

---

🔍 Issues & Concerns

1. CRITICAL: Memory Leak Risk in XTerminal.tsx

Location: src/renderer/components/XTerminal.tsx:192-241

The component has a dependency array issue that could cause memory leaks:

// Line 192: Empty dependency array
useEffect(() => {
  // ...terminal initialization
  return () => {
    titleDisposable.dispose();
    webglAddon?.dispose();
    term.dispose();
  };
}, []); // Only run on mount - theme/font updates handled separately

// Lines 232-241: sessionId/onData dependencies
useEffect(() => {
  if (!terminalRef.current) return;
  
  const disposable = terminalRef.current.onData((data) => {
    window.maestro.process.write(sessionId, data);
    onData?.(data);
  });
  
  return () => disposable.dispose();
}, [sessionId, onData]);

Problem: The onData callback changes on every render if passed as an arrow function from the parent, causing:

• Excessive listener re-registration
• Potential missed events during re-registration
• Unnecessary effect runs

Fix: Parent component should memoize the callback:

const handleTerminalData = useCallback((data: string) => {
  // handle data
}, [/* dependencies */]);

<XTerminal onData={handleTerminalData} />

Or XTerminal should use a ref pattern:

const onDataRef = useRef(onData);
onDataRef.current = onData;

useEffect(() => {
  // use onDataRef.current in the handler
}, [sessionId]); // onData no longer in deps

---

2. CRITICAL: Process Manager RAW Output Logic

Location: src/main/process-manager.ts:517-526

// For terminal mode with xterm.js, send RAW data
if (isTerminal) {
  logger.debug('[ProcessManager] PTY onData (raw terminal)', 'ProcessManager', {
    sessionId,
    pid: ptyProcess.pid,
    dataLength: data.length,
  });
  this.emit('data', sessionId, data);
  return;
}

Problem: The isTerminal flag is determined when the process spawns, but terminal tab sessions use session IDs like {sessionId}-terminal-{tabId}. There's no visible code setting isTerminal = true for these sessions.

Question: How is isTerminal set? Looking at spawn() method:

• Is it based on toolType === 'terminal'?
• Does spawnTerminalTab correctly set this flag?

Verify this code path - otherwise terminal tabs will get filtered output instead of raw output, breaking xterm.js rendering.

---

3. Race Condition: Terminal Tab Migration

Location: src/renderer/App.tsx:837-857

// Migrate sessions without terminal tabs
let terminalTabs = correctedSession.terminalTabs;
let activeTerminalTabId = correctedSession.activeTerminalTabId;
if (!terminalTabs || terminalTabs.length === 0) {
  const defaultTerminalTab = createTerminalTab(
    defaultShell || 'zsh',
    correctedSession.cwd,
    null
  );
  terminalTabs = [defaultTerminalTab];
  activeTerminalTabId = defaultTerminalTab.id;
}

Potential Issue: Migration happens during session restore, but there's no check for activeTerminalTabId validity if terminalTabs exists but is non-empty.

Edge Case: What if:

• terminalTabs = [tab1, tab2] (exists, non-empty)
• activeTerminalTabId = 'deleted-id' (points to removed tab)

Fix: Add validation:

if (!terminalTabs || terminalTabs.length === 0) {
  // ... migration
} else {
  // Validate activeTerminalTabId points to existing tab
  if (!terminalTabs.find(t => t.id === activeTerminalTabId)) {
    activeTerminalTabId = terminalTabs[0].id;
  }
  // ... reset runtime state
}

---

4. Type Safety: Missing ClosedTerminalTab Definition

The type ClosedTerminalTab is imported in src/renderer/types/index.ts but the grep showed it's defined, yet it's not exported in the type changes shown in the diff. Verify this type is properly exported.

---

5. Session Persistence Issue

Location: Session interface has these fields:

terminalTabs?: TerminalTab[];
activeTerminalTabId: string;
closedTerminalTabHistory: ClosedTerminalTab[];

Question: Are terminalTabs and activeTerminalTabId persisted to disk?

Looking at the migration code, it seems yes. However:

• Runtime state (pid, state, exitCode) is correctly reset on restore
• closedTerminalTabHistory is correctly excluded from persistence

But check prepareSessionForPersistence() - the PR summary says it's updated, but I need to verify all terminal tab fields are handled correctly.

---

6. Debounced Persistence Hook

Location: src/renderer/hooks/utils/useDebouncedPersistence.ts:19-20

The diff shows changes to this file (additions: 19, deletions: 1). This likely adds terminal tab support, but without seeing the full diff, I can't verify if terminal tab state changes trigger persistence correctly.

Verify: Changes to terminalTabs, activeTerminalTabId should trigger debounced session persistence.

---

🎯 Code Quality & Best Practices

1. Good Patterns:

• Use of memo for TerminalTabComponent prevents unnecessary re-renders
• Proper cleanup in all useEffect hooks
• ResizeObserver with debouncing (100ms) is appropriate
• forwardRef + useImperativeHandle pattern in XTerminal is correct

2. Potential Improvements:

a) Magic Numbers

// XTerminal.tsx:122
}, 100); // 100ms debounce

// XTerminal.tsx:137
scrollback: 10000, // 10k lines

Extract to constants:

const RESIZE_DEBOUNCE_MS = 100;
const TERMINAL_SCROLLBACK_LINES = 10000;

b) Hard-coded Shell Detection

// process-manager.ts:1416
command: shell || (process.platform === 'win32' ? 'powershell.exe' : 'zsh'),

This should use settings/defaults consistently with the rest of the app.

c) Missing Error Handling

// XTerminal.tsx:223-229
const unsubscribe = window.maestro.process.onData((sid, data) => {
  if (sid === sessionId && terminalRef.current) {
    terminalRef.current.write(data);
  }
});

No error handling if `write()$ throws (e.g., terminal disposed but data arrives).

---

🔒 Security Considerations

1. No Security Issues Detected

• No user input is passed to shell without going through established IPC
• Terminal session IDs are UUIDs, not user-controllable
• No XSS risk (xterm.js handles rendering safely)

2. Consideration:
Large scrollback (10,000 lines) could be a memory concern with many terminal tabs open. Consider making this configurable or implementing a more aggressive limit.

---

⚡ Performance Considerations

1. WebGL Renderer
✅ Good: WebGL with canvas fallback
✅ Good: Context loss handling with automatic fallback
⚠️ Consider: Add metrics to track WebGL vs canvas usage for debugging

2. Resize Handling
✅ Good: 100ms debounce prevents excessive resize operations
✅ Good: ResizeObserver is efficient

3. Tab Rendering
✅ Good: Use of memo prevents unnecessary re-renders
⚠️ Potential Issue: getTerminalTabDisplayName is called on every render in the tab component. Consider memoizing this within the component:

const displayName = useMemo(() => getTerminalTabDisplayName(tab, index), [tab.name, index]);

---

📋 Test Coverage

Missing Tests:
This PR has no test coverage. Given the WIP status, this is acceptable, but before merging you should add:

1. Unit tests for helpers:

   • terminalTabHelpers.ts functions
   • parseTerminalSessionId edge cases
   • createTerminalTab defaults

2. Integration tests:

   • Terminal tab creation/closing
   • Tab reordering
   • Session persistence and migration

3. E2E tests:

   • Terminal spawning and data flow
   • xterm.js lifecycle
   • Theme changes

---

📚 Documentation

Good:

• Inline JSDoc comments are comprehensive
• Type definitions are well-documented
• File headers explain component purpose

Missing:

• No update to CLAUDE.md with terminal tab patterns
• No update to ARCHITECTURE.md
• No migration guide for users

Suggest adding to CLAUDE.md:

### 11. Terminal Tab Management

Sessions support multiple terminal tabs with full PTY emulation:

```typescript
// Each session has terminal tabs
session.terminalTabs: TerminalTab[]
session.activeTerminalTabId: string

// Each tab maintains its own PTY
interface TerminalTab {
  id: string;
  pid: number;
  state: 'idle' | 'busy' | 'exited';
  shellType: string;
}

---

### 🎨 **UI/UX Review**

**Good:**
- Tab state colors are intuitive (green=success, red=error, yellow=busy)
- Drag-and-drop reordering matches AI tab behavior
- Sticky "new tab" button on overflow is nice touch
- Double-click to rename is discoverable

**Minor Issues:**

**1. Keyboard Shortcut Display**
```typescript
// TerminalTabBar.tsx:264
const shortcutKey = typeof navigator !== 'undefined' && navigator.platform?.includes('Mac') ? 'Ctrl+Shift+`' : 'Ctrl+Shift+`';

Both branches are identical! Should be:

const shortcutKey = navigator.platform?.includes('Mac') ? 'Cmd+Shift+`' : 'Ctrl+Shift+`';

2. Exit Code Display
Exit codes only show for non-zero exits, which is good, but negative exit codes (signals) aren't handled specially. Consider showing signal names (e.g., "SIGKILL" for -9).

---

🏗️ Architecture Concerns

1. Dual Process Model
The PR description confirms each session has:

• AI agent process (-ai suffix)
• Terminal process (-terminal suffix)

Question: With multi-tab terminals, does each terminal tab get its own PTY process? If so, the naming should be:

• {sessionId}-terminal-{tabId} ✅ (matches implementation)

But this means potentially many PTY processes per session. Is there a limit on terminal tabs per session?

2. State Management
Good separation between:

• Persistent state (survives app restart)
• Runtime state (reset on restore)

But verify closedTerminalTabHistory max size is enforced (MAX_CLOSED_TERMINAL_TABS = 10).

---

📝 Suggested Changes Summary

Before Merge:

1. Fix onData dependency issue in XTerminal.tsx (memory leak risk)
2. Verify isTerminal flag is set correctly for terminal tab sessions
3. Add activeTerminalTabId validation during migration
4. Fix keyboard shortcut display (Mac vs non-Mac)
5. Add error handling to terminal data write
6. Extract magic numbers to constants
7. Add tests for terminal tab helpers
8. Update CLAUDE.md with terminal tab patterns

Nice to Have:

1. Make scrollback configurable
2. Add metrics for WebGL usage
3. Memoize displayName calculation
4. Add signal name display for exit codes
5. Document terminal tab limits (if any)

---

✨ Overall Assessment

Quality: ⭐⭐⭐⭐☆ (4/5)

This is a solid foundation for terminal emulation. The architecture is sound, follows project conventions, and demonstrates good engineering practices. The main concerns are around edge cases and memory management rather than fundamental design issues.

Recommendation: Approve with changes - Fix the critical issues (onData deps, isTerminal flag, migration validation) before merging. The "nice to have" items can be follow-up PRs.

Excellent work on Phase 1-5! The phased approach with clear commit messages makes this easy to review. Looking forward to seeing the UI integration in Phase 6+.

---

Reviewed with ❤️ by Claude Code

[claude]

Pull Request Review: Terminal Emulation Implementation

This is a comprehensive and well-architected implementation of full terminal emulation using xterm.js. The code quality is excellent overall, with clear separation of concerns and adherence to the existing codebase patterns. Below are my detailed findings:

---

✅ Strengths

1. Excellent Architecture & Separation of Concerns

• Clean separation between XTerminal.tsx (xterm.js wrapper), TerminalView.tsx (orchestration), and TerminalTabBar.tsx (UI)
• Helper utilities in terminalTabHelpers.ts follow the established pattern from tabHelpers.ts
• Type definitions are comprehensive and well-documented
• IPC handlers properly isolated in process.ts

2. Strong Type Safety

• All interfaces are well-defined with clear documentation
• Proper TypeScript usage throughout
• Forward refs and imperative handles correctly typed
• Good use of type guards and optional chaining

3. Proper Resource Management

• XTerminal properly disposes of addons and terminal instances
• PTY processes killed on tab close
• ResizeObserver cleanup handled correctly
• Event listener cleanup via unsubscribe functions

4. Good User Experience Patterns

• Debounced resize handling (100ms) prevents performance issues
• WebGL renderer with canvas fallback
• Terminal state visualization (green/red/yellow icons)
• Focus management on tab switching
• Drag-and-drop tab reordering

5. Backwards Compatibility

• Migration logic for sessions without terminalTabs
• Legacy fields (shellLogs, terminalPid) preserved with deprecation comments
• Runtime state reset on session restore (PIDs don't survive app restart)

6. Consistent with Codebase Patterns

• Follows existing TabBar patterns
• Uses established theme mapping approach
• Adheres to naming conventions from CLAUDE.md
• Proper integration with existing session management

---

🔍 Issues & Concerns

Critical Issues

1. Missing Terminal Tab Callbacks in App.tsx ⚠️

MainPanel.tsx defines 11 terminal tab callback props:

onTerminalTabSelect?: (sessionId: string, tabId: string) => void;
onTerminalTabClose?: (sessionId: string, tabId: string) => void;
onTerminalNewTab?: (sessionId: string) => void;
onTerminalTabRename?: (sessionId: string, tabId: string, name: string) => void;
onTerminalTabReorder?: (sessionId: string, fromIndex: number, toIndex: number) => void;
// ... etc

However, App.tsx does not implement these handlers. This means:

• Creating new terminal tabs won't work
• Closing terminal tabs won't update session state
• Renaming/reordering won't persist
• State changes (idle/busy/exited) won't be tracked

Location: src/renderer/App.tsx - need to add these handlers similar to how AI tab handlers are implemented.

2. Persistence Logic Missing

The prepareSessionForPersistence() function needs updates for terminal tabs:

// Should be added (similar to AI tabs):
terminalTabs: session.terminalTabs?.map(tab => ({
  ...tab,
  pid: 0,           // Reset runtime state
  state: 'idle',    // Reset runtime state
  exitCode: undefined
})),
// Exclude closedTerminalTabHistory from persistence

Location: src/renderer/App.tsx - prepareSessionForPersistence function

3. Input Mode Switching Logic

The PR hides InputArea when inputMode === 'terminal', but there's no visible UI to switch back to AI mode. Users could get stuck in terminal mode.

Recommendation: Add a mode toggle button or keyboard shortcut (similar to existing Cmd+T for terminal).

Medium Priority Issues

4. Raw PTY Data Security Concern 🔒

In process-manager.ts:

if (isTerminal) {
  // Send RAW data - xterm handles all rendering
  this.emit('data', sessionId, data);
  return;
}

Sending completely raw PTY output could expose sensitive data if the session is accidentally set to isTerminal when it shouldn't be. Consider adding additional validation:

if (isTerminal && managedProc?.toolType === 'terminal') {
  // Only send raw data for actual terminal sessions
  this.emit('data', sessionId, data);
  return;
}

5. Missing Error Boundary

TerminalView.tsx and XTerminal.tsx don't have error boundaries. If xterm.js crashes, it could take down the entire UI.

Recommendation: Wrap TerminalView in an ErrorBoundary component.

6. Incomplete Terminal State Tracking

The onTabStateChange callback is defined but never called from TerminalView. There's no logic to detect when a command is running (busy state) vs idle.

Recommendation: Parse shell prompts or track onData events to infer busy state.

7. CWD Tracking Not Implemented

onTabCwdChange callback exists but is never invoked. The terminal doesn't track directory changes (via cd commands).

Recommendation: Parse ANSI escape sequences (OSC 7) or prompt for working directory updates.

8. Spawn-on-Demand Race Condition

In TerminalView.tsx:93-97:

if (activeTab && activeTab.pid === 0 && activeTab.state !== 'exited') {
  spawnPtyForTab(activeTab);
}

If the user rapidly switches tabs, multiple spawn attempts could race. Should add a "spawning" state or use a ref to track in-progress spawns.

Low Priority / Nice-to-Have

9. Memory Leak Potential with Terminal Refs

TerminalView.tsx uses a Map to store terminal refs but only cleans up in setTerminalRef. If tabs are closed without proper cleanup, refs could accumulate.

Recommendation: Add cleanup in useEffect when terminalTabs changes.

10. No Terminal Search UI

XTerminal exposes search functionality (search, searchNext, etc.) but there's no UI to use it. Cmd+F won't work.

Recommendation: Add search toolbar or keyboard shortcut handler.

11. Hardcoded Scrollback Limit

scrollback: 10000 is hardcoded in XTerminal.tsx:137. Should be a configurable setting.

12. Missing Tab Hover Overlay Menu

According to CLAUDE.md, tabs should show an overlay menu after 400ms hover (copy session ID, rename, close, etc.). Terminal tabs don't implement this.

Reference: See src/renderer/components/TabBar.tsx for the pattern.

13. Incomplete TypeScript Compilation Check

The commit message states "All TypeScript compilation passes" but the PR doesn't include:

• ESLint check results
• Build output verification
• Test suite status

---

🧪 Testing Recommendations

1. Unit Tests Needed:

   • terminalTabHelpers.ts functions (especially parseTerminalSessionId)
   • Terminal tab state management logic
   • Migration logic for sessions without terminalTabs

2. Integration Tests:

   • PTY spawn/kill lifecycle
   • Tab switching with active processes
   • Session persistence and restoration

3. Manual Testing Scenarios:

   • Create 10+ terminal tabs, verify performance
   • Kill app while terminal command is running, verify clean restart
   • Test with SSH remote sessions (does xterm.js handle remote properly?)
   • Test on Windows with PowerShell

---

📋 Code Style & Convention Issues

1. Console.log in Production Code

   • XTerminal.tsx:159: console.warn for WebGL - should use logger
   • TerminalView.tsx:172: console.log for title change - should use logger
   • App.tsx:848: console.log for migration - should use logger

2. Missing JSDoc Comments

   • spawnTerminalTab in process-manager.ts has good docs ✅
   • TerminalView.tsx handlers lack documentation

3. Magic Numbers

   • 100 ms debounce in XTerminal.tsx:122 - should be a constant
   • 400 ms hover delay mentioned in TabBar pattern - not consistent

---

🔐 Security Considerations

1. Command Injection Risk: The spawnTerminalTab function properly uses shell settings from the store, avoiding injection risks. ✅

2. Sensitive Data in PTY Output: Raw terminal output could contain passwords, API keys, etc. Ensure terminal content is excluded from crash reports/logs.

3. XSS via ANSI Sequences: xterm.js handles this internally, but verify the version has no known vulnerabilities. ✅ (v6.0.0 is recent)

---

📊 Performance Considerations

1. Multiple XTerminal Instances: Rendering all terminal tabs even when invisible (visibility: hidden) could be expensive. Consider lazy rendering or unmounting inactive tabs after a delay.

2. WebGL Context Limits: Browsers limit WebGL contexts (~16). With many tabs, fallback to canvas is likely. This is handled gracefully. ✅

3. Resize Debouncing: 100ms debounce is good, but rapid tab switching could trigger many resize operations. Consider throttling instead.

---

🎯 Recommended Action Items

Before Merging:

1. ✅ Implement all terminal tab callback handlers in App.tsx
2. ✅ Add terminal tab persistence logic to prepareSessionForPersistence
3. ✅ Add mode switching UI (Cmd+K menu or button)
4. ⚠️ Add error boundary around TerminalView
5. ⚠️ Fix spawn-on-demand race condition

Post-Merge:

6. Implement busy state tracking (parse prompts)
7. Implement CWD tracking (OSC 7 sequences)
8. Add terminal search UI (Cmd+F)
9. Make scrollback configurable
10. Add tab hover overlay menu
11. Replace console.log with logger
12. Add unit tests for helpers
13. Add integration tests for PTY lifecycle

---

📝 Final Verdict

Overall Assessment: This is a high-quality implementation that demonstrates strong software engineering practices. The architecture is sound, type safety is excellent, and it integrates well with existing patterns.

Recommendation: Approve with required changes (items 1-3 above must be addressed before merging).

Confidence Level: High - the code is well-structured and follows best practices, but the missing App.tsx integration is a critical gap that prevents the feature from functioning.

---

Great work on this implementation! The phased approach in the commits makes it easy to review, and the documentation is thorough. Once the integration with App.tsx is complete, this will be a solid addition to Maestro.

[claude]

PR Review: Terminal Emulation Implementation (Phase 1-6)

This PR implements a comprehensive terminal emulation system using xterm.js with multi-tab support. Overall, this is high-quality, well-architected work that follows the existing codebase patterns closely.

✅ Strengths

1. Excellent Architecture & Consistency

• Mirrors the existing AITab pattern perfectly with TerminalTab - great consistency
• Proper separation of concerns: XTerminal (view), TerminalView (orchestration), TerminalTabBar (UI)
• Helper utilities follow the same pattern as tabHelpers.ts
• IPC handlers follow established conventions with proper error logging

2. Robust State Management

• Clean migration logic for backwards compatibility (App.tsx:837-857)
• Proper runtime state reset on session restore
• Session persistence correctly excludes runtime-only fields
• Defensive handling for undefined terminalTabs throughout

3. Smart PTY Management

• Raw output mode for xterm.js avoids double-filtering
• Lazy PTY spawning - only spawn when tab becomes active
• Proper cleanup on tab close with PTY termination
• WebGL rendering with graceful fallback to canvas

4. Code Quality

• Comprehensive JSDoc comments
• TypeScript types are complete and well-structured
• Proper use of React patterns (memo, useCallback, useRef)

⚠️ Issues & Concerns

1. Missing Session State Updates (High Priority)

The TerminalView component receives callbacks but never calls several critical ones:

• onTabCwdChange is never called - CWD tracking not implemented
• onTabRename is never called
• Tab state changes to busy are never triggered

Impact: Terminal tabs will never show yellow busy indicator, CWD won't update as user navigates, tab renaming won't work.

2. App.tsx Missing Handler Implementations (High Priority)

MainPanel receives 11 terminal callbacks but App.tsx doesn't implement handlers for them. This will cause runtime errors when users try to interact with terminal tabs.

Fix: App.tsx needs to implement all 11 handlers to update session state, similar to how AI tab handlers work.

3. Resize Race Condition (Medium Priority)

XTerminal.tsx:120 - IPC resize call could fail if PTY process doesn't exist yet or session is being destroyed. Add error handling.

4. Memory Leak Risk (Medium Priority)

TerminalView.tsx:59 - terminalRefs Map entries are never cleaned up when tabs are closed. Add cleanup effect.

5. Missing Error Boundary (Medium Priority)

The xterm.js component could crash rendering. Wrap TerminalView with ErrorBoundary.

6. Security: Shell Command Injection (Low - Mitigated)

User-provided shell path is used directly. Consider validating that the shell path is absolute, points to executable, and is in whitelist.

🎯 Testing Gaps

No tests included for:

• Terminal tab creation/destruction lifecycle
• PTY spawning with different shell configurations
• Session migration from old format
• Terminal tab helper functions

Recommendation: Add at least unit tests for terminalTabHelpers.ts

🔍 Performance Considerations

Good: Lazy PTY spawning, debounced resize, WebGL rendering

Potential Issues: All terminals mount simultaneously even if invisible - could be heavy with 10+ tabs

🚀 Recommendations

Must Fix Before Merge:

1. Implement all 11 terminal callback handlers in App.tsx - critical for functionality
2. Wire up CWD tracking and busy state detection - UX depends on this
3. Add error handling to resize IPC calls - prevent crashes

Should Fix:

4. Add cleanup for terminalRefs Map
5. Add ErrorBoundary around TerminalView
6. Implement tab rename handler

Nice to Have:

7. Add unit tests for helper functions
8. Consider lazy terminal mounting for performance

📊 Overall Assessment

Quality Score: 8/10

This is excellent foundational work that properly sets up the architecture for terminal emulation. The code is clean, well-structured, and follows best practices. However, it's incomplete - the wiring between components is missing, which will cause runtime errors.

Status: ⚠️ NEEDS WORK

The PR is marked WIP which is appropriate. Once the missing handlers are implemented and basic error handling is added, this will be production-ready.

💡 Additional Notes

• Pattern Consistency: The parallel structure with AI tabs is brilliant
• Migration Strategy: The backwards-compatible migration is well-thought-out
• Session ID Format: The {sessionId}-terminal-{tabId} format is clean and debuggable

Great work overall! Looking forward to seeing this completed. 🎉

---

Review generated with assistance from Claude Code

[claude]

Code Review - PR #166: Improved Terminal Experience

This PR implements a significant enhancement to the terminal experience by integrating xterm.js for full terminal emulation with multi-tab support. Overall, this is a well-structured and high-quality implementation that follows the project's patterns.

---

✅ Strengths

Architecture & Design

• Excellent separation of concerns: The three-component structure (XTerminal → TerminalView → TerminalTabBar) is clean and follows React best practices
• Consistent with existing patterns: Terminal tab management mirrors the AI tab system, making the codebase more maintainable
• Proper resource management: PTY processes are spawned on-demand and cleaned up on close
• Good use of React patterns: Proper use of refs, memo, useCallback, useImperativeHandle, and forwardRef

Implementation Quality

• Robust xterm.js integration: Proper addon loading with WebGL fallback, error handling for context loss
• Theme synchronization: Excellent mapping from Maestro themes to xterm ITheme with dark/light mode awareness
• Resize handling: Debounced resize with ResizeObserver prevents performance issues
• Documentation: All files have clear docblocks explaining purpose and responsibilities

Process Management

• Smart PTY lifecycle: Only spawns PTY when tab becomes active
• Raw data passthrough: Correctly sends raw PTY data to xterm.js for terminal mode
• Proper cleanup: Kills PTY processes when tabs close

---

⚠️ Issues & Concerns

1. Memory Leak: ResizeObserver Cleanup (High Priority)

Location: XTerminal.tsx:212-219

The ResizeObserver cleanup doesn't cancel pending debounced operations. If the component unmounts before the 100ms timeout fires, the timeout callback will reference stale refs and potentially cause memory leaks.

Recommendation: Clear the timeout in the cleanup function.

2. Race Condition: Double PTY Spawn (Medium Priority)

Location: TerminalView.tsx:93-97

If spawnPtyForTab is async and the user rapidly switches tabs, multiple spawn calls could be made for the same tab before pid is updated.

Recommendation: Add a connecting state or use a ref to track in-flight spawn operations.

3. Missing Error Boundaries (Medium Priority)

Location: TerminalView.tsx and XTerminal.tsx

XTerminal can throw errors during WebGL context initialization, Terminal.open(), or addon loading. These would bubble up and potentially crash the entire session view.

Recommendation: Wrap TerminalView in an ErrorBoundary (already done for other views in MainPanel.tsx).

4. Missing Migration Test (Medium Priority)

Location: App.tsx:827-863

The backwards compatibility migration (adding terminal tabs to sessions without them) is critical but has no tests.

Recommendation: Add a test case that loads a session from the old format and verifies migration.

---

🔍 Performance Considerations

Positive

• ✅ Debounced resize (100ms) prevents resize storms
• ✅ Lazy PTY spawning (only for active tab)
• ✅ WebGL acceleration with canvas fallback
• ✅ Proper use of React.memo on tab components

Potential Improvements

1. Scrollback buffer: 10,000 lines per tab could use significant memory. Consider making this configurable in settings.
2. Multiple terminals: All terminals stay in memory with invisible class. For sessions with many tabs, consider a max visible tabs limit.

---

🧪 Test Coverage

Missing Tests:

1. No unit tests for new components (XTerminal, TerminalView, TerminalTabBar)
2. No tests for terminalTabHelpers.ts utility functions
3. No integration tests for multi-tab scenarios
4. No tests for PTY lifecycle
5. No tests for session migration

Recommendation: Add test files for components and utilities.

---

📝 Minor Issues

1. Console.log left in: TerminalView.tsx:172 should use the logger service
2. Magic numbers: 100ms debounce, 10,000 scrollback - consider extracting to constants or settings
3. Unused function: _handleReopenTerminalTab not wired to keyboard shortcut

---

🎯 Recommendations Priority

Must Fix Before Merge

• Fix ResizeObserver cleanup (Issue 1)
• Add migration test (Issue 4)

Should Fix Before Merge

• Add race condition protection (Issue 2)
• Wrap TerminalView in ErrorBoundary (Issue 3)
• Remove console.log or replace with logger

Can Fix Later

• Add comprehensive test coverage
• Make scrollback configurable

---

✨ Overall Assessment

This is excellent work that significantly improves the terminal experience. The code quality is high, follows project patterns well, and the architecture is sound. The main concerns are around edge case handling (memory leaks, race conditions) and test coverage.

Recommendation: ✅ Approve with requested changes

Once the Must Fix items are addressed, this PR will be ready to merge. Great job on maintaining consistency with the existing codebase patterns!

---

Code Quality: ⭐⭐⭐⭐ (4/5)
Architecture: ⭐⭐⭐⭐⭐ (5/5)
Documentation: ⭐⭐⭐⭐⭐ (5/5)
Test Coverage: ⭐⭐ (2/5)

Overall: ⭐⭐⭐⭐ (4/5)

[claude]

PR Review: Full Terminal Emulation with xterm.js

Overview

This is a substantial and well-structured PR implementing multi-tab terminal emulation using xterm.js. The implementation follows Maestro's established patterns closely and demonstrates good architectural discipline across 8 phases.

---

✅ Strengths

Architecture & Design

• Excellent pattern consistency: Terminal tab management mirrors AI tab patterns (helpers, types, handlers), making the codebase more maintainable
• Clean separation of concerns: XTerminal (pure xterm.js wrapper), TerminalView (orchestration), TerminalTabBar (UI)
• Proper lifecycle management: PTY spawning on-demand, cleanup on close, state tracking per tab
• Backwards compatibility: Migration logic for sessions without terminalTabs (App.tsx:827-847)

Code Quality

• Strong TypeScript usage: Well-defined interfaces (TerminalTab, ClosedTerminalTab, XTerminalHandle)
• Good documentation: Comprehensive JSDoc comments explaining component responsibilities
• Defensive programming: Null-safety checks (terminalTabs || [], optional chaining)
• Imperative handles: Clean ref-based API for parent control (clearActiveTerminal, searchActiveTerminal)

User Experience

• Comprehensive keyboard shortcuts: All expected shortcuts implemented (Cmd+W, Cmd+Shift+T, Cmd+1-9, Cmd+K, Cmd+F)
• Visual state indicators: Color-coded terminal icons (green=exited 0, red=error, yellow=busy)
• Tab overflow handling: Sticky + button, smooth scrolling to active tab
• Undo support: Closed tab history (max 10) for Cmd+Shift+T

---

🔍 Issues & Concerns

1. PTY Data Handling Logic Needs Review ⚠️

Location: src/main/process-manager.ts:517-536
Issue: The isTerminal parameter is passed to stripControlSequences() but it's already false at that point (since we early-returned if true). This suggests the function signature may be misleading.
Recommendation: Review stripControlSequences() to ensure the parameter is used correctly, or simplify by removing it.

2. Missing Session Persistence Logic ❌

Location: src/renderer/App.tsx (multiple session creation points)
Issue: While closedTerminalTabHistory is properly excluded from persistence, I don't see where terminal tabs are serialized/deserialized for session storage.
Recommendation: Add explicit terminal tab persistence handling in prepareSessionForPersistence() or document why it's not needed.

3. Potential Memory Leak in Terminal Refs ⚠️

Location: src/renderer/components/TerminalView.tsx:79
Issue: When tabs are closed, refs are deleted but XTerminal cleanup relies on React unmount. If a tab is closed while PTY is still writing data, race conditions possible.
Recommendation: Ensure window.maestro.process.kill() completes before React unmounts XTerminal. Consider error boundaries.

4. Hardcoded Platform Detection Bug 🔧

Location: src/renderer/components/TerminalTabBar.tsx:264
Issue: Both branches return the same value (Ctrl+Shift+). Should be Cmd+Shift+ on Mac.

5. Missing Error Handling for PTY Spawn Failures ⚠️

Location: src/renderer/components/TerminalView.tsx:89-110
Issue: If result.success is false but no error thrown, tab remains in limbo (pid=0, state=idle). No user feedback.
Recommendation: Set state to exited with error indication and show toast notification.

6. Debounce Timeout Cleanup 🔧

Location: src/renderer/components/XTerminal.tsx:106-123
Issue: Timeout not cleaned up on component unmount. Callback may fire after unmount.

7. WebGL Addon Error Handling ⚠️

Location: src/renderer/components/XTerminal.tsx:154-167
Issue: On context loss, addon disposed but xterm.js still thinks it's loaded. Terminal may become unresponsive.

8. Keyboard Shortcut Conflicts ⚠️

Location: src/renderer/hooks/keyboard/useMainKeyboardHandler.ts:429
Concern: Cmd+K clears terminal - conflicts with macOS system shortcut and VS Code Quick Open.
Recommendation: Consider Cmd+Shift+K or make configurable.

9. Missing Test Coverage ❌

No tests found for: Terminal tab lifecycle, PTY spawn/kill/resize IPC handlers, migration logic, XTerminal ref handle methods.

---

🔒 Security Considerations

Good Practices

• Uses execFileNoThrow for shell detection (follows CLAUDE.md requirements)
• PTY sessions isolated per tab with unique IDs
• No shell injection vectors

Potential Issues

1. Shell path validation: customShellPath from settings not validated before spawn. Malicious settings could execute arbitrary binaries.
2. Environment variable injection: shellEnvVars passed directly to PTY. Could override LD_PRELOAD, PATH, etc.

---

🚀 Performance Considerations

1. Lazy terminal instance creation: All tabs render XTerminal components (hidden via invisible class). Consider conditional rendering.
2. WebGL memory: Each terminal with WebGL uses ~2-4MB GPU memory. With many tabs, this accumulates.
3. Scrollback buffer: 10,000 lines × multiple tabs = significant memory. Consider making configurable.

---

📝 Documentation Issues

1. CLAUDE.md Update Needed: Add terminal tab handlers to Session Interface section, document spawnTerminalTab API, add terminal keyboard shortcuts
2. Missing Architecture Documentation: Terminal tabs lifecycle diagram, migration strategy for old sessions
3. Type Documentation Gaps: TerminalTab.state transitions not documented (idle → busy → exited?)

---

🎯 Recommendations Summary

Must Fix Before Merge

1. ❌ Fix keyboard shortcut key display bug (TerminalTabBar.tsx:264)
2. ❌ Add timeout cleanup in resize handler (XTerminal.tsx)
3. ❌ Clarify terminal tab persistence strategy
4. ❌ Add basic error handling for PTY spawn failures

Should Fix Before Merge

5. ⚠️ Review isTerminal parameter in stripControlSequences()
6. ⚠️ Add shell path validation in process.ts IPC handler
7. ⚠️ Document terminal tab state transitions

Nice to Have

8. 🔧 Add test coverage for new components
9. 🔧 Update CLAUDE.md with terminal tab patterns
10. 🔧 Consider making scrollback buffer configurable

---

✨ Final Verdict

This is high-quality work that follows Maestro's patterns exceptionally well. The architecture is sound, the code is clean, and the feature is well-integrated. The issues identified are mostly minor bugs and missing documentation rather than fundamental design flaws.

Recommendation: Approve with minor changes - Address the Must Fix items, then merge. The Should Fix items can be follow-up PRs if needed.

Great job maintaining consistency with the existing codebase! 🎉

---

Reviewed by: Claude Sonnet 4.5
Review Date: 2026-01-09