diff --git a/frontend/lib/hooks/__tests__/useInteractiveTerminal.test.tsx b/frontend/lib/hooks/__tests__/useInteractiveTerminal.test.tsx new file mode 100644 index 0000000..78887fb --- /dev/null +++ b/frontend/lib/hooks/__tests__/useInteractiveTerminal.test.tsx @@ -0,0 +1,323 @@ +import { renderHook, act } from '@testing-library/react'; +import { useInteractiveTerminal } from '../useInteractiveTerminal'; + +// Mock socket.io-client +const mockSocket = { + on: jest.fn(), + emit: jest.fn(), + disconnect: jest.fn(), + connected: true, +}; + +jest.mock('socket.io-client', () => ({ + io: jest.fn(() => mockSocket), +})); + +// Mock xterm +const mockTerminal = { + loadAddon: jest.fn(), + open: jest.fn(), + write: jest.fn(), + onData: jest.fn(), + dispose: jest.fn(), +}; + +const mockFitAddon = { + fit: jest.fn(), + proposeDimensions: jest.fn(() => ({ cols: 80, rows: 24 })), +}; + +jest.mock('@xterm/xterm', () => ({ + Terminal: jest.fn(() => mockTerminal), +})); + +jest.mock('@xterm/addon-fit', () => ({ + FitAddon: jest.fn(() => mockFitAddon), +})); + +// Mock API client +jest.mock('@/lib/api', () => ({ + apiClient: { + getToken: jest.fn(() => 'test-token'), + }, + API_BASE_URL: 'http://localhost:3000', +})); + +describe('useInteractiveTerminal', () => { + const defaultProps = { + open: true, + containerId: 'container123', + containerName: 'test-container', + isMobile: false, + onFallback: jest.fn(), + }; + + beforeEach(() => { + jest.clearAllMocks(); + // Reset mock socket event handlers + mockSocket.on.mockClear(); + mockSocket.emit.mockClear(); + mockSocket.disconnect.mockClear(); + mockTerminal.dispose.mockClear(); + }); + + it('should return terminalRef and cleanup function', () => { + const { result } = renderHook(() => + useInteractiveTerminal(defaultProps) + ); + + expect(result.current.terminalRef).toBeDefined(); + expect(typeof result.current.cleanup).toBe('function'); + }); + + it('should not initialize terminal when open is false', async () => { + const { io } = require('socket.io-client'); + + renderHook(() => + useInteractiveTerminal({ + ...defaultProps, + open: false, + }) + ); + + // Wait for potential async operations + await act(async () => { + await new Promise(resolve => setTimeout(resolve, 100)); + }); + + expect(io).not.toHaveBeenCalled(); + }); + + describe('effect dependency stability', () => { + it('should re-initialize when onFallback reference changes (demonstrates the bug this fix prevents)', async () => { + const { io } = require('socket.io-client'); + + // Create a ref div for the terminal + const mockDiv = document.createElement('div'); + + const { rerender } = renderHook( + (props) => { + const hook = useInteractiveTerminal(props); + // Simulate ref being available + if (hook.terminalRef.current === null) { + (hook.terminalRef as any).current = mockDiv; + } + return hook; + }, + { + initialProps: { + ...defaultProps, + onFallback: jest.fn(), // First callback instance + }, + } + ); + + await act(async () => { + await new Promise(resolve => setTimeout(resolve, 200)); + }); + + const initialCallCount = io.mock.calls.length; + + // Rerender with a NEW function reference (simulating unstable callback) + // This WILL cause re-init because onFallback is in the dependency array + // The fix is in useTerminalModalState which now memoizes handleFallback + rerender({ + ...defaultProps, + onFallback: jest.fn(), // New callback instance + }); + + await act(async () => { + await new Promise(resolve => setTimeout(resolve, 200)); + }); + + const finalCallCount = io.mock.calls.length; + + // This test DOCUMENTS that unstable onFallback causes re-initialization + // The actual fix ensures onFallback from useTerminalModalState is stable + expect(finalCallCount).toBeGreaterThan(initialCallCount); + }); + + it('should only re-initialize when open, containerId, or isMobile changes', async () => { + const { io } = require('socket.io-client'); + const stableOnFallback = jest.fn(); + + const mockDiv = document.createElement('div'); + + const { rerender } = renderHook( + (props) => { + const hook = useInteractiveTerminal(props); + if (hook.terminalRef.current === null) { + (hook.terminalRef as any).current = mockDiv; + } + return hook; + }, + { + initialProps: { + ...defaultProps, + onFallback: stableOnFallback, + }, + } + ); + + await act(async () => { + await new Promise(resolve => setTimeout(resolve, 200)); + }); + + const initialCallCount = io.mock.calls.length; + + // Rerender with same props (stable reference) + rerender({ + ...defaultProps, + onFallback: stableOnFallback, // Same reference + }); + + await act(async () => { + await new Promise(resolve => setTimeout(resolve, 200)); + }); + + // Should NOT reinitialize with same props + expect(io.mock.calls.length).toBe(initialCallCount); + }); + + it('should re-initialize when containerId changes', async () => { + const { io } = require('socket.io-client'); + const stableOnFallback = jest.fn(); + + const mockDiv = document.createElement('div'); + + const { rerender } = renderHook( + (props) => { + const hook = useInteractiveTerminal(props); + if (hook.terminalRef.current === null) { + (hook.terminalRef as any).current = mockDiv; + } + return hook; + }, + { + initialProps: { + ...defaultProps, + onFallback: stableOnFallback, + }, + } + ); + + await act(async () => { + await new Promise(resolve => setTimeout(resolve, 200)); + }); + + const initialCallCount = io.mock.calls.length; + + // Rerender with different containerId + rerender({ + ...defaultProps, + containerId: 'different-container', + onFallback: stableOnFallback, + }); + + await act(async () => { + await new Promise(resolve => setTimeout(resolve, 200)); + }); + + // SHOULD reinitialize with new containerId + expect(io.mock.calls.length).toBeGreaterThan(initialCallCount); + }); + }); + + it('should cleanup on unmount', async () => { + const mockDiv = document.createElement('div'); + + const { unmount } = renderHook( + () => { + const hook = useInteractiveTerminal(defaultProps); + if (hook.terminalRef.current === null) { + (hook.terminalRef as any).current = mockDiv; + } + return hook; + } + ); + + await act(async () => { + await new Promise(resolve => setTimeout(resolve, 200)); + }); + + unmount(); + + await act(async () => { + await new Promise(resolve => setTimeout(resolve, 100)); + }); + + // Verify cleanup was called + expect(mockSocket.disconnect).toHaveBeenCalled(); + }); + + it('should call cleanup function when invoked manually', () => { + const { result } = renderHook(() => useInteractiveTerminal(defaultProps)); + + act(() => { + result.current.cleanup(); + }); + + // Manual cleanup should work without errors + expect(result.current.cleanup).toBeDefined(); + }); +}); + +describe('useInteractiveTerminal reconnection loop detection', () => { + const testProps = { + open: true, + containerId: 'container123', + containerName: 'test-container', + isMobile: false, + onFallback: jest.fn(), + }; + + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('should not create multiple connections in rapid succession with stable props', async () => { + const { io } = require('socket.io-client'); + const mockDiv = document.createElement('div'); + + // Track connection timing + const connectionTimes: number[] = []; + io.mockImplementation(() => { + connectionTimes.push(Date.now()); + return mockSocket; + }); + + const stableOnFallback = jest.fn(); + + const { rerender } = renderHook( + (props) => { + const hook = useInteractiveTerminal(props); + if (hook.terminalRef.current === null) { + (hook.terminalRef as any).current = mockDiv; + } + return hook; + }, + { + initialProps: { + ...testProps, + onFallback: stableOnFallback, + }, + } + ); + + // Simulate multiple rapid rerenders (like React Strict Mode or state updates) + for (let i = 0; i < 5; i++) { + rerender({ + ...testProps, + onFallback: stableOnFallback, + }); + } + + await act(async () => { + await new Promise(resolve => setTimeout(resolve, 500)); + }); + + // With stable props, should only have 1 connection (initial mount) + // A reconnection loop would show multiple connections + expect(connectionTimes.length).toBeLessThanOrEqual(2); // Allow for initial + StrictMode double-mount + }); +}); diff --git a/frontend/lib/hooks/__tests__/useTerminalModalState.test.tsx b/frontend/lib/hooks/__tests__/useTerminalModalState.test.tsx index ff411cb..fb17b24 100644 --- a/frontend/lib/hooks/__tests__/useTerminalModalState.test.tsx +++ b/frontend/lib/hooks/__tests__/useTerminalModalState.test.tsx @@ -125,4 +125,72 @@ describe('useTerminalModalState', () => { expect(result.current.isMobile).toBe(false); }); + + describe('handler stability (useCallback memoization)', () => { + it('should return stable handleFallback reference across renders', () => { + const { result, rerender } = renderHook(() => useTerminalModalState()); + + const firstHandleFallback = result.current.handleFallback; + + // Trigger a re-render + rerender(); + + const secondHandleFallback = result.current.handleFallback; + + // Handler should be the same reference (memoized with useCallback) + expect(firstHandleFallback).toBe(secondHandleFallback); + }); + + it('should return stable handleModeChange reference across renders', () => { + const { result, rerender } = renderHook(() => useTerminalModalState()); + + const firstHandler = result.current.handleModeChange; + + rerender(); + + const secondHandler = result.current.handleModeChange; + + expect(firstHandler).toBe(secondHandler); + }); + + it('should return stable handleRetryInteractive reference across renders', () => { + const { result, rerender } = renderHook(() => useTerminalModalState()); + + const firstHandler = result.current.handleRetryInteractive; + + rerender(); + + const secondHandler = result.current.handleRetryInteractive; + + expect(firstHandler).toBe(secondHandler); + }); + + it('should return stable reset reference across renders', () => { + const { result, rerender } = renderHook(() => useTerminalModalState()); + + const firstHandler = result.current.reset; + + rerender(); + + const secondHandler = result.current.reset; + + expect(firstHandler).toBe(secondHandler); + }); + + it('should maintain handler stability even after state changes', () => { + const { result, rerender } = renderHook(() => useTerminalModalState()); + + const firstHandleFallback = result.current.handleFallback; + + // Trigger state change + act(() => { + result.current.handleFallback('Test error'); + }); + + rerender(); + + // Handler should still be the same reference + expect(result.current.handleFallback).toBe(firstHandleFallback); + }); + }); }); diff --git a/frontend/lib/hooks/useTerminalModalState.ts b/frontend/lib/hooks/useTerminalModalState.ts index 7b8cf77..bc8c310 100644 --- a/frontend/lib/hooks/useTerminalModalState.ts +++ b/frontend/lib/hooks/useTerminalModalState.ts @@ -1,9 +1,13 @@ -import { useState } from 'react'; +import { useState, useCallback } from 'react'; import { useMediaQuery, useTheme } from '@mui/material'; /** * Comprehensive hook for managing TerminalModal state * Handles mode switching, fallback logic, and UI state + * + * IMPORTANT: All handlers are memoized with useCallback to prevent + * unnecessary re-renders in dependent hooks (e.g., useInteractiveTerminal) + * which would cause WebSocket reconnection loops. */ export function useTerminalModalState() { const theme = useTheme(); @@ -14,40 +18,40 @@ export function useTerminalModalState() { const [fallbackReason, setFallbackReason] = useState(''); const [showFallbackNotification, setShowFallbackNotification] = useState(false); - const handleFallback = (reason: string) => { + const handleFallback = useCallback((reason: string) => { console.warn('Falling back to simple mode:', reason); setInteractiveFailed(true); setFallbackReason(reason); setMode('simple'); setShowFallbackNotification(false); - }; + }, []); - const handleModeChange = ( + const handleModeChange = useCallback(( event: React.MouseEvent, newMode: 'simple' | 'interactive' | null, ) => { if (newMode !== null) { - if (newMode === 'interactive' && interactiveFailed) { + if (newMode === 'interactive') { setInteractiveFailed(false); setFallbackReason(''); } setMode(newMode); } - }; + }, []); - const handleRetryInteractive = () => { + const handleRetryInteractive = useCallback(() => { setInteractiveFailed(false); setFallbackReason(''); setShowFallbackNotification(false); setMode('interactive'); - }; + }, []); - const reset = () => { + const reset = useCallback(() => { setMode('interactive'); setInteractiveFailed(false); setFallbackReason(''); setShowFallbackNotification(false); - }; + }, []); return { isMobile,