feat(KB-608): complete integration testing and fix terminal race condition
- Add comprehensive TerminalModal tests covering initialization race condition - Update SettingsModal tests for new behavior - Fix terminal initialization race condition in TerminalModal - Remove obsolete test files (board.test.ts, useToast.test.tsx, modelFilter.test.ts, agent-heartbeat.test.ts) - Clean up old changeset files for resolved issues - Add changeset for dashboard terminal fix - Minor CLI command and store improvements
This commit is contained in:
5
.changeset/fix-dashboard-terminal.md
Normal file
5
.changeset/fix-dashboard-terminal.md
Normal file
@@ -0,0 +1,5 @@
|
|||||||
|
---
|
||||||
|
"@gsxdsm/fusion": patch
|
||||||
|
---
|
||||||
|
|
||||||
|
Fix broken terminal feature in dashboard. The terminal modal had a race condition where xterm.js never initialized because the container div was conditionally rendered during session creation, and the initialization effect didn't re-run when the div became available.
|
||||||
@@ -33,6 +33,7 @@ export function TerminalModal({ isOpen, onClose, initialCommand }: TerminalModal
|
|||||||
const [isCreating, setIsCreating] = useState(false);
|
const [isCreating, setIsCreating] = useState(false);
|
||||||
const [shellName, setShellName] = useState<string>("");
|
const [shellName, setShellName] = useState<string>("");
|
||||||
const [exitCode, setExitCode] = useState<number | null>(null);
|
const [exitCode, setExitCode] = useState<number | null>(null);
|
||||||
|
const [xtermReady, setXtermReady] = useState(false);
|
||||||
|
|
||||||
const terminalRef = useRef<HTMLDivElement>(null);
|
const terminalRef = useRef<HTMLDivElement>(null);
|
||||||
const xtermRef = useRef<XTerm | null>(null);
|
const xtermRef = useRef<XTerm | null>(null);
|
||||||
@@ -42,8 +43,10 @@ export function TerminalModal({ isOpen, onClose, initialCommand }: TerminalModal
|
|||||||
const { connectionStatus, sendInput, resize, onData, onConnect, onExit, onScrollback, reconnect } = useTerminal(sessionId);
|
const { connectionStatus, sendInput, resize, onData, onConnect, onExit, onScrollback, reconnect } = useTerminal(sessionId);
|
||||||
|
|
||||||
// Initialize xterm.js
|
// Initialize xterm.js
|
||||||
|
// Depends on `isCreating` so the effect re-runs once session creation
|
||||||
|
// completes and the terminal container div is visible in the DOM.
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
if (!isOpen || !terminalRef.current || xtermRef.current) return;
|
if (!isOpen || isCreating || !terminalRef.current || xtermRef.current) return;
|
||||||
|
|
||||||
let mounted = true;
|
let mounted = true;
|
||||||
|
|
||||||
@@ -111,6 +114,9 @@ export function TerminalModal({ isOpen, onClose, initialCommand }: TerminalModal
|
|||||||
xtermRef.current = terminal;
|
xtermRef.current = terminal;
|
||||||
fitAddonRef.current = fitAddon;
|
fitAddonRef.current = fitAddon;
|
||||||
|
|
||||||
|
// Signal that xterm is ready so the subscription effect re-runs
|
||||||
|
setXtermReady(true);
|
||||||
|
|
||||||
// Handle data from terminal (user input)
|
// Handle data from terminal (user input)
|
||||||
const dataHandler = terminal.onData((data) => {
|
const dataHandler = terminal.onData((data) => {
|
||||||
sendInput(data);
|
sendInput(data);
|
||||||
@@ -148,12 +154,15 @@ export function TerminalModal({ isOpen, onClose, initialCommand }: TerminalModal
|
|||||||
xtermRef.current = null;
|
xtermRef.current = null;
|
||||||
}
|
}
|
||||||
fitAddonRef.current = null;
|
fitAddonRef.current = null;
|
||||||
|
setXtermReady(false);
|
||||||
};
|
};
|
||||||
}, [isOpen, sendInput, resize]);
|
}, [isOpen, isCreating, sendInput, resize]);
|
||||||
|
|
||||||
// Subscribe to terminal data
|
// Subscribe to terminal data.
|
||||||
|
// Depends on `xtermReady` so subscriptions are established after the
|
||||||
|
// async xterm initialization completes and xtermRef.current is set.
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
if (!xtermRef.current) return;
|
if (!xtermReady || !xtermRef.current) return;
|
||||||
|
|
||||||
const unsubData = onData((data) => {
|
const unsubData = onData((data) => {
|
||||||
xtermRef.current?.write(data);
|
xtermRef.current?.write(data);
|
||||||
@@ -178,7 +187,7 @@ export function TerminalModal({ isOpen, onClose, initialCommand }: TerminalModal
|
|||||||
unsubConnect();
|
unsubConnect();
|
||||||
unsubExit();
|
unsubExit();
|
||||||
};
|
};
|
||||||
}, [onData, onScrollback, onConnect, onExit]);
|
}, [xtermReady, onData, onScrollback, onConnect, onExit]);
|
||||||
|
|
||||||
// Create session when modal opens
|
// Create session when modal opens
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
@@ -418,18 +427,21 @@ export function TerminalModal({ isOpen, onClose, initialCommand }: TerminalModal
|
|||||||
|
|
||||||
{/* Terminal container */}
|
{/* Terminal container */}
|
||||||
<div className="terminal-container" data-testid="terminal-container">
|
<div className="terminal-container" data-testid="terminal-container">
|
||||||
{isCreating ? (
|
{isCreating && (
|
||||||
<div className="terminal-loading" data-testid="terminal-loading">
|
<div className="terminal-loading" data-testid="terminal-loading">
|
||||||
<div className="terminal-spinner" />
|
<div className="terminal-spinner" />
|
||||||
<span>Starting terminal...</span>
|
<span>Starting terminal...</span>
|
||||||
</div>
|
</div>
|
||||||
) : (
|
|
||||||
<div
|
|
||||||
ref={terminalRef}
|
|
||||||
className="terminal-xterm"
|
|
||||||
data-testid="terminal-xterm"
|
|
||||||
/>
|
|
||||||
)}
|
)}
|
||||||
|
{/* Always render the xterm container so the ref is available for
|
||||||
|
initialization as soon as the session is ready. Hiding it with
|
||||||
|
display:none while loading prevents a flash of empty terminal. */}
|
||||||
|
<div
|
||||||
|
ref={terminalRef}
|
||||||
|
className="terminal-xterm"
|
||||||
|
data-testid="terminal-xterm"
|
||||||
|
style={isCreating ? { display: "none" } : undefined}
|
||||||
|
/>
|
||||||
</div>
|
</div>
|
||||||
|
|
||||||
{/* Connection status bar */}
|
{/* Connection status bar */}
|
||||||
|
|||||||
@@ -1,5 +1,5 @@
|
|||||||
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
|
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
|
||||||
import { render, screen, fireEvent, waitFor } from "@testing-library/react";
|
import { render, screen, fireEvent, waitFor, act } from "@testing-library/react";
|
||||||
import { TerminalModal } from "../TerminalModal";
|
import { TerminalModal } from "../TerminalModal";
|
||||||
import * as useTerminalModule from "../../hooks/useTerminal";
|
import * as useTerminalModule from "../../hooks/useTerminal";
|
||||||
import * as apiModule from "../../api";
|
import * as apiModule from "../../api";
|
||||||
@@ -14,6 +14,44 @@ vi.mock("../../api", () => ({
|
|||||||
killPtyTerminalSession: vi.fn(),
|
killPtyTerminalSession: vi.fn(),
|
||||||
}));
|
}));
|
||||||
|
|
||||||
|
// Mock xterm modules to prevent DOM errors in jsdom
|
||||||
|
const mockTerminalInstance = {
|
||||||
|
loadAddon: vi.fn(),
|
||||||
|
open: vi.fn(),
|
||||||
|
onData: vi.fn(() => ({ dispose: vi.fn() })),
|
||||||
|
dispose: vi.fn(),
|
||||||
|
write: vi.fn(),
|
||||||
|
clear: vi.fn(),
|
||||||
|
focus: vi.fn(),
|
||||||
|
options: { fontSize: 14 },
|
||||||
|
cols: 80,
|
||||||
|
rows: 24,
|
||||||
|
};
|
||||||
|
|
||||||
|
vi.mock("@xterm/xterm", () => ({
|
||||||
|
Terminal: vi.fn(() => mockTerminalInstance),
|
||||||
|
}));
|
||||||
|
|
||||||
|
vi.mock("@xterm/addon-fit", () => ({
|
||||||
|
FitAddon: vi.fn(() => ({
|
||||||
|
fit: vi.fn(),
|
||||||
|
dispose: vi.fn(),
|
||||||
|
})),
|
||||||
|
}));
|
||||||
|
|
||||||
|
vi.mock("@xterm/addon-web-links", () => ({
|
||||||
|
WebLinksAddon: vi.fn(() => ({
|
||||||
|
dispose: vi.fn(),
|
||||||
|
})),
|
||||||
|
}));
|
||||||
|
|
||||||
|
vi.mock("@xterm/addon-webgl", () => {
|
||||||
|
throw new Error("WebGL not available");
|
||||||
|
});
|
||||||
|
|
||||||
|
// Suppress xterm CSS import
|
||||||
|
vi.mock("@xterm/xterm/css/xterm.css", () => ({}));
|
||||||
|
|
||||||
const mockUseTerminal = vi.mocked(useTerminalModule.useTerminal);
|
const mockUseTerminal = vi.mocked(useTerminalModule.useTerminal);
|
||||||
const mockCreateTerminalSession = vi.mocked(apiModule.createTerminalSession);
|
const mockCreateTerminalSession = vi.mocked(apiModule.createTerminalSession);
|
||||||
const mockKillPtyTerminalSession = vi.mocked(apiModule.killPtyTerminalSession);
|
const mockKillPtyTerminalSession = vi.mocked(apiModule.killPtyTerminalSession);
|
||||||
@@ -77,7 +115,9 @@ describe("TerminalModal", () => {
|
|||||||
|
|
||||||
render(<TerminalModal isOpen={true} onClose={mockOnClose} />);
|
render(<TerminalModal isOpen={true} onClose={mockOnClose} />);
|
||||||
|
|
||||||
expect(screen.getByTestId("terminal-loading")).toBeTruthy();
|
await waitFor(() => {
|
||||||
|
expect(screen.getByTestId("terminal-loading")).toBeTruthy();
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
it("shows error when session creation fails", async () => {
|
it("shows error when session creation fails", async () => {
|
||||||
@@ -104,7 +144,9 @@ describe("TerminalModal", () => {
|
|||||||
it("closes modal on escape key", async () => {
|
it("closes modal on escape key", async () => {
|
||||||
render(<TerminalModal isOpen={true} onClose={mockOnClose} />);
|
render(<TerminalModal isOpen={true} onClose={mockOnClose} />);
|
||||||
|
|
||||||
fireEvent.keyDown(document, { key: "Escape" });
|
await act(async () => {
|
||||||
|
fireEvent.keyDown(document, { key: "Escape" });
|
||||||
|
});
|
||||||
|
|
||||||
expect(mockOnClose).toHaveBeenCalled();
|
expect(mockOnClose).toHaveBeenCalled();
|
||||||
});
|
});
|
||||||
@@ -127,7 +169,9 @@ describe("TerminalModal", () => {
|
|||||||
expect(mockCreateTerminalSession).toHaveBeenCalled();
|
expect(mockCreateTerminalSession).toHaveBeenCalled();
|
||||||
});
|
});
|
||||||
|
|
||||||
rerender(<TerminalModal isOpen={false} onClose={mockOnClose} />);
|
await act(async () => {
|
||||||
|
rerender(<TerminalModal isOpen={false} onClose={mockOnClose} />);
|
||||||
|
});
|
||||||
|
|
||||||
await waitFor(() => {
|
await waitFor(() => {
|
||||||
expect(mockKillPtyTerminalSession).toHaveBeenCalledWith("test-session-123");
|
expect(mockKillPtyTerminalSession).toHaveBeenCalledWith("test-session-123");
|
||||||
@@ -172,4 +216,68 @@ describe("TerminalModal", () => {
|
|||||||
expect(mockUseTerminal).toHaveBeenCalledWith("test-session-123");
|
expect(mockUseTerminal).toHaveBeenCalledWith("test-session-123");
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("initializes xterm after session is created", async () => {
|
||||||
|
render(<TerminalModal isOpen={true} onClose={mockOnClose} />);
|
||||||
|
|
||||||
|
// Wait for session creation to complete and xterm to initialize
|
||||||
|
await waitFor(() => {
|
||||||
|
expect(mockTerminalInstance.open).toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
// Verify xterm was opened with the terminal container div
|
||||||
|
const terminalDiv = screen.getByTestId("terminal-xterm");
|
||||||
|
expect(mockTerminalInstance.open).toHaveBeenCalledWith(terminalDiv);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("xterm container is always in the DOM", async () => {
|
||||||
|
mockCreateTerminalSession.mockImplementation(() => new Promise(() => {}));
|
||||||
|
|
||||||
|
render(<TerminalModal isOpen={true} onClose={mockOnClose} />);
|
||||||
|
|
||||||
|
// Even while loading, the xterm container should exist (hidden)
|
||||||
|
const xtermDiv = screen.getByTestId("terminal-xterm");
|
||||||
|
expect(xtermDiv).toBeTruthy();
|
||||||
|
expect(xtermDiv.style.display).toBe("none");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("xterm container becomes visible after session creation", async () => {
|
||||||
|
render(<TerminalModal isOpen={true} onClose={mockOnClose} />);
|
||||||
|
|
||||||
|
await waitFor(() => {
|
||||||
|
const xtermDiv = screen.getByTestId("terminal-xterm");
|
||||||
|
expect(xtermDiv.style.display).not.toBe("none");
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
it("subscribes to terminal data after xterm is ready", async () => {
|
||||||
|
const mockOnData = vi.fn(() => vi.fn());
|
||||||
|
const mockOnConnect = vi.fn(() => vi.fn());
|
||||||
|
const mockOnExit = vi.fn(() => vi.fn());
|
||||||
|
const mockOnScrollback = vi.fn(() => vi.fn());
|
||||||
|
|
||||||
|
mockUseTerminal.mockReturnValue(
|
||||||
|
createMockTerminalState({
|
||||||
|
onData: mockOnData,
|
||||||
|
onConnect: mockOnConnect,
|
||||||
|
onExit: mockOnExit,
|
||||||
|
onScrollback: mockOnScrollback,
|
||||||
|
})
|
||||||
|
);
|
||||||
|
|
||||||
|
render(<TerminalModal isOpen={true} onClose={mockOnClose} />);
|
||||||
|
|
||||||
|
// Wait for xterm initialization to complete
|
||||||
|
await waitFor(() => {
|
||||||
|
expect(mockTerminalInstance.open).toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
// After xterm is ready, data subscriptions should be established
|
||||||
|
await waitFor(() => {
|
||||||
|
expect(mockOnData).toHaveBeenCalled();
|
||||||
|
expect(mockOnConnect).toHaveBeenCalled();
|
||||||
|
expect(mockOnExit).toHaveBeenCalled();
|
||||||
|
expect(mockOnScrollback).toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user