fix(terminal): preserve terminal state when switching projects (#358)

* fix(terminal): preserve terminal state when switching projects Fixes terminal state loss when switching between project tabs (#342). Two issues addressed: 1. PTY health check: Added checkTerminalPtyAlive IPC method to detect terminals with stale state (no live PTY process). restoreTerminalSessions now removes dead terminals and restores from disk instead of skipping. 2. Buffer preservation: Added SerializeAddon to capture terminal buffer with ANSI escape codes before disposal. This preserves the shell prompt, colors, and output history when switching back to a project. Closes #342 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(terminal): address PR review findings Addresses 5 findings from Auto Claude PR Review: 1. [HIGH] Race condition protection: Added restoringProjects Set to prevent concurrent restore calls for the same project 2. [HIGH] Unnecessary disk restore: Skip disk restore when some terminals are still alive to avoid duplicates 3. [HIGH] Double dispose vulnerability: Added isDisposedRef guard to prevent corrupted serialization on rapid unmount/StrictMode 4. [MEDIUM] SerializeAddon disposal: Explicitly call dispose() before setting ref to null 5. [MEDIUM] projectPath validation: Added input validation at function start 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(terminal): allow disk restore when alive terminals exist CodeRabbit review finding: When a project has mixed alive and dead terminals, the early return was preventing disk restore, causing dead terminals to be permanently lost. The fix removes the early return since addRestoredTerminal() already has duplicate protection (checks terminal ID before adding). This allows dead terminals to be safely restored from disk while alive terminals remain unaffected. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(terminal): remove unused aliveTerminals variable CodeQL flagged unused variable after previous fix removed the early return that was using it. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
2025-12-27 22:20:52 +01:00
parent 4e71361b2d
commit 7881b2d1e9
8 changed files with 126 additions and 7 deletions
@@ -655,6 +655,22 @@ export function registerTerminalHandlers(
      }
    }
  );
+
+  // Check if a terminal's PTY process is alive
+  ipcMain.handle(
+    IPC_CHANNELS.TERMINAL_CHECK_PTY_ALIVE,
+    async (_, terminalId: string): Promise<IPCResult<{ alive: boolean }>> => {
+      try {
+        const alive = terminalManager.isTerminalAlive(terminalId);
+        return { success: true, data: { alive } };
+      } catch (error) {
+        return {
+          success: false,
+          error: error instanceof Error ? error.message : 'Failed to check terminal status'
+        };
+      }
+    }
+  );
 }

 /**
@@ -279,6 +279,13 @@ export class TerminalManager {
    }
  }

+  /**
+   * Check if a terminal's PTY process is alive
+   */
+  isTerminalAlive(terminalId: string): boolean {
+    return this.terminals.has(terminalId);
+  }
+
  /**
   * Handle terminal data output
   */
@@ -46,6 +46,7 @@ export interface TerminalAPI {
    cols?: number,
    rows?: number
  ) => Promise<IPCResult<import('../../shared/types').SessionDateRestoreResult>>;
+  checkTerminalPtyAlive: (terminalId: string) => Promise<IPCResult<{ alive: boolean }>>;

  // Terminal Event Listeners
  onTerminalOutput: (callback: (id: string, data: string) => void) => () => void;
@@ -133,6 +134,9 @@ export const createTerminalAPI = (): TerminalAPI => ({
  ): Promise<IPCResult<import('../../shared/types').SessionDateRestoreResult>> =>
    ipcRenderer.invoke(IPC_CHANNELS.TERMINAL_RESTORE_FROM_DATE, date, projectPath, cols, rows),

+  checkTerminalPtyAlive: (terminalId: string): Promise<IPCResult<{ alive: boolean }>> =>
+    ipcRenderer.invoke(IPC_CHANNELS.TERMINAL_CHECK_PTY_ALIVE, terminalId),
+
  // Terminal Event Listeners
  onTerminalOutput: (
    callback: (id: string, data: string) => void
@@ -2,6 +2,7 @@ import { useEffect, useRef, useCallback } from 'react';
 import { Terminal as XTerm } from '@xterm/xterm';
 import { FitAddon } from '@xterm/addon-fit';
 import { WebLinksAddon } from '@xterm/addon-web-links';
+import { SerializeAddon } from '@xterm/addon-serialize';
 import { terminalBufferManager } from '../../lib/terminal-buffer-manager';

 interface UseXtermOptions {
@@ -14,7 +15,9 @@ export function useXterm({ terminalId, onCommandEnter, onResize }: UseXtermOptio
  const terminalRef = useRef<HTMLDivElement>(null);
  const xtermRef = useRef<XTerm | null>(null);
  const fitAddonRef = useRef<FitAddon | null>(null);
+  const serializeAddonRef = useRef<SerializeAddon | null>(null);
  const commandBufferRef = useRef<string>('');
+  const isDisposedRef = useRef<boolean>(false);

  // Initialize xterm.js UI
  useEffect(() => {
@@ -57,9 +60,11 @@ export function useXterm({ terminalId, onCommandEnter, onResize }: UseXtermOptio

    const fitAddon = new FitAddon();
    const webLinksAddon = new WebLinksAddon();
+    const serializeAddon = new SerializeAddon();

    xterm.loadAddon(fitAddon);
    xterm.loadAddon(webLinksAddon);
+    xterm.loadAddon(serializeAddon);

    xterm.open(terminalRef.current);

@@ -69,8 +74,10 @@ export function useXterm({ terminalId, onCommandEnter, onResize }: UseXtermOptio

    xtermRef.current = xterm;
    fitAddonRef.current = fitAddon;
+    serializeAddonRef.current = serializeAddon;

    // Replay buffered output if this is a remount or restored session
+    // This now includes ANSI codes for proper formatting/colors/prompt
    const bufferedOutput = terminalBufferManager.get(terminalId);
    if (bufferedOutput && bufferedOutput.length > 0) {
      xterm.write(bufferedOutput);
@@ -150,12 +157,41 @@ export function useXterm({ terminalId, onCommandEnter, onResize }: UseXtermOptio
    }
  }, []);

+  /**
+   * Serialize the terminal buffer before disposal.
+   * This preserves ANSI escape codes for colors, formatting, and the prompt.
+   */
+  const serializeBuffer = useCallback(() => {
+    if (xtermRef.current && serializeAddonRef.current) {
+      try {
+        const serialized = serializeAddonRef.current.serialize();
+        if (serialized && serialized.length > 0) {
+          terminalBufferManager.set(terminalId, serialized);
+        }
+      } catch (error) {
+        console.error('[useXterm] Failed to serialize terminal buffer:', error);
+      }
+    }
+  }, [terminalId]);
+
  const dispose = useCallback(() => {
+    // Guard against double dispose (can happen in React StrictMode or rapid unmount)
+    if (isDisposedRef.current) return;
+    isDisposedRef.current = true;
+
+    // Serialize buffer before disposing to preserve ANSI formatting
+    serializeBuffer();
+
    if (xtermRef.current) {
      xtermRef.current.dispose();
      xtermRef.current = null;
    }
-  }, []);
+    if (serializeAddonRef.current) {
+      serializeAddonRef.current.dispose();
+      serializeAddonRef.current = null;
+    }
+    fitAddonRef.current = null;
+  }, [serializeBuffer]);

  return {
    terminalRef,
@@ -71,6 +71,11 @@ export const terminalMock = {

  saveTerminalBuffer: async () => {},

+  checkTerminalPtyAlive: async () => ({
+    success: true,
+    data: { alive: false }
+  }),
+
  // Terminal Event Listeners (no-op in browser)
  onTerminalOutput: () => () => {},
  onTerminalExit: () => () => {},
@@ -212,20 +212,67 @@ export const useTerminalStore = create<TerminalState>((set, get) => ({
  },
 }));

+// Track in-progress restore operations to prevent race conditions
+const restoringProjects = new Set<string>();
+
 /**
 * Restore terminal sessions for a project from persisted storage
 */
 export async function restoreTerminalSessions(projectPath: string): Promise<void> {
-  const store = useTerminalStore.getState();
-
-  // Don't restore if we already have terminals for THIS project
-  const projectTerminals = store.terminals.filter(t => t.projectPath === projectPath);
-  if (projectTerminals.length > 0) {
-    debugLog('[TerminalStore] Terminals already exist for this project, skipping session restore');
+  // Validate input
+  if (!projectPath || typeof projectPath !== 'string') {
+    debugLog('[TerminalStore] Invalid projectPath, skipping restore');
    return;
  }

+  // Prevent concurrent restores for same project (race condition protection)
+  if (restoringProjects.has(projectPath)) {
+    debugLog('[TerminalStore] Already restoring terminals for this project, skipping');
+    return;
+  }
+  restoringProjects.add(projectPath);
+
  try {
+    const store = useTerminalStore.getState();
+
+    // Get terminals for this project that exist in state
+    const projectTerminals = store.terminals.filter(t => t.projectPath === projectPath);
+
+    if (projectTerminals.length > 0) {
+      // Check if PTY processes are alive for existing terminals
+      const aliveChecks = await Promise.all(
+        projectTerminals.map(async (terminal) => {
+          try {
+            const result = await window.electronAPI.checkTerminalPtyAlive(terminal.id);
+            return { terminal, alive: result.success && result.data?.alive === true };
+          } catch {
+            return { terminal, alive: false };
+          }
+        })
+      );
+
+      // Remove dead terminals from store (they have state but no PTY process)
+      const deadTerminals = aliveChecks.filter(c => !c.alive);
+
+      for (const { terminal } of deadTerminals) {
+        debugLog(`[TerminalStore] Removing dead terminal: ${terminal.id}`);
+        store.removeTerminal(terminal.id);
+      }
+
+      // If all terminals were alive, we're done
+      if (deadTerminals.length === 0) {
+        debugLog('[TerminalStore] All terminals have live PTY processes');
+        return;
+      }
+
+      // Note: We don't skip disk restore when alive terminals exist because:
+      // 1. Dead terminals were removed from state above
+      // 2. addRestoredTerminal() has duplicate protection (checks terminal ID)
+      // 3. Disk restore will safely only add back the dead terminals
+      debugLog(`[TerminalStore] ${deadTerminals.length} terminals had dead PTY, will restore from disk`);
+    }
+
+    // Restore from disk
    const result = await window.electronAPI.getTerminalSessions(projectPath);
    if (!result.success || !result.data || result.data.length === 0) {
      return;
@@ -239,5 +286,7 @@ export async function restoreTerminalSessions(projectPath: string): Promise<void
    store.setHasRestoredSessions(true);
  } catch (error) {
    debugError('[TerminalStore] Error restoring sessions:', error);
+  } finally {
+    restoringProjects.delete(projectPath);
  }
 }
@@ -69,6 +69,7 @@ export const IPC_CHANNELS = {
  TERMINAL_GET_SESSION_DATES: 'terminal:getSessionDates',
  TERMINAL_GET_SESSIONS_FOR_DATE: 'terminal:getSessionsForDate',
  TERMINAL_RESTORE_FROM_DATE: 'terminal:restoreFromDate',
+  TERMINAL_CHECK_PTY_ALIVE: 'terminal:checkPtyAlive',

  // Terminal events (main -> renderer)
  TERMINAL_OUTPUT: 'terminal:output',
@@ -178,6 +178,7 @@ export interface ElectronAPI {
  getTerminalSessionsForDate: (date: string, projectPath: string) => Promise<IPCResult<TerminalSession[]>>;
  restoreTerminalSessionsFromDate: (date: string, projectPath: string, cols?: number, rows?: number) => Promise<IPCResult<SessionDateRestoreResult>>;
  saveTerminalBuffer: (terminalId: string, serialized: string) => Promise<void>;
+  checkTerminalPtyAlive: (terminalId: string) => Promise<IPCResult<{ alive: boolean }>>;

  // Terminal event listeners
  onTerminalOutput: (callback: (id: string, data: string) => void) => () => void;