fix(FN-771): reset disclosure state on task creation and preserve terminal sessions across reconnects
- Reset isDisclosureExpanded in QuickEntryBox resetForm so disclosure collapses after task creation - Remove sticky disclosure persistence from QuickEntryBox component - Preserve terminal sessions across transient WebSocket disconnects with buffer/replay - Buffer and replay initial terminal state for prompt visibility on reconnect - Update tests for non-persistent disclosure, terminal reconnect, and server routes - Remove unused UsageIndicator tests and TaskDetailModal test cleanup
This commit is contained in:
@@ -9,7 +9,6 @@ import { CustomModelDropdown } from "./CustomModelDropdown";
|
||||
import { ModelSelectionModal } from "./ModelSelectionModal";
|
||||
|
||||
const STORAGE_KEY = "kb-quick-entry-text";
|
||||
const DISCLOSURE_STORAGE_KEY = "kb-quick-entry-expanded";
|
||||
|
||||
interface QuickEntryBoxProps {
|
||||
onCreate?: (input: TaskCreateInput) => Promise<void>;
|
||||
@@ -63,14 +62,8 @@ export function QuickEntryBox({ onCreate, addToast, tasks = [], availableModels,
|
||||
// isExpanded controls textarea height styling (auto-resize)
|
||||
const [isExpanded, setIsExpanded] = useState(false);
|
||||
// isDisclosureExpanded controls visibility of the controls panel (Deps, Models, etc.)
|
||||
const [isDisclosureExpanded, setIsDisclosureExpanded] = useState(() => {
|
||||
if (typeof window !== "undefined") {
|
||||
const saved = localStorage.getItem(DISCLOSURE_STORAGE_KEY);
|
||||
// Default to collapsed — only expand if user explicitly saved "true"
|
||||
return saved === "true";
|
||||
}
|
||||
return false;
|
||||
});
|
||||
// Always starts collapsed — user must explicitly toggle each session
|
||||
const [isDisclosureExpanded, setIsDisclosureExpanded] = useState(false);
|
||||
const textareaRef = useRef<HTMLTextAreaElement>(null);
|
||||
const justResetRef = useRef(false);
|
||||
|
||||
@@ -156,12 +149,12 @@ export function QuickEntryBox({ onCreate, addToast, tasks = [], availableModels,
|
||||
}
|
||||
}, [description]);
|
||||
|
||||
// Persist disclosure state to localStorage whenever it changes
|
||||
// Clean up legacy disclosure persistence key from previous versions
|
||||
useEffect(() => {
|
||||
if (typeof window !== "undefined") {
|
||||
localStorage.setItem(DISCLOSURE_STORAGE_KEY, isDisclosureExpanded.toString());
|
||||
localStorage.removeItem("kb-quick-entry-expanded");
|
||||
}
|
||||
}, [isDisclosureExpanded]);
|
||||
}, []);
|
||||
|
||||
// Cleanup on unmount
|
||||
useEffect(() => {
|
||||
@@ -231,7 +224,7 @@ export function QuickEntryBox({ onCreate, addToast, tasks = [], availableModels,
|
||||
setIsRefineMenuOpen(false);
|
||||
setIsRefining(false);
|
||||
setIsExpanded(false); // Collapse textarea height on reset
|
||||
// Note: isDisclosureExpanded is NOT reset - user preference persists
|
||||
setIsDisclosureExpanded(false); // Always reset controls to collapsed after creation
|
||||
justResetRef.current = true;
|
||||
if (textareaRef.current) {
|
||||
textareaRef.current.style.height = "auto";
|
||||
|
||||
@@ -1381,28 +1381,34 @@ describe("ListView Quick Entry", () => {
|
||||
const mockOnQuickCreate = vi.fn().mockResolvedValue(undefined);
|
||||
renderListView({ onQuickCreate: mockOnQuickCreate });
|
||||
|
||||
const input = screen.getByTestId("quick-entry-input");
|
||||
|
||||
// Focus the input to expand the QuickEntryBox
|
||||
fireEvent.focus(input);
|
||||
// Controls should start hidden (collapsed by default)
|
||||
expect(document.getElementById("quick-entry-controls")?.hasAttribute("hidden")).toBe(true);
|
||||
|
||||
// Click the disclosure toggle to expand the QuickEntryBox controls
|
||||
const toggleButton = screen.getByTestId("quick-entry-toggle");
|
||||
fireEvent.click(toggleButton);
|
||||
|
||||
// Model selector button should be visible
|
||||
const modelButton = await screen.findByTestId("quick-entry-models-button");
|
||||
expect(modelButton).toBeDefined();
|
||||
expect(document.getElementById("quick-entry-controls")?.hasAttribute("hidden")).toBe(false);
|
||||
});
|
||||
|
||||
it("shows dependency selector button when QuickEntryBox is expanded", async () => {
|
||||
const mockOnQuickCreate = vi.fn().mockResolvedValue(undefined);
|
||||
renderListView({ onQuickCreate: mockOnQuickCreate });
|
||||
|
||||
const input = screen.getByTestId("quick-entry-input");
|
||||
|
||||
// Focus the input to expand the QuickEntryBox
|
||||
fireEvent.focus(input);
|
||||
// Controls should start hidden (collapsed by default)
|
||||
expect(document.getElementById("quick-entry-controls")?.hasAttribute("hidden")).toBe(true);
|
||||
|
||||
// Click the disclosure toggle to expand the QuickEntryBox controls
|
||||
const toggleButton = screen.getByTestId("quick-entry-toggle");
|
||||
fireEvent.click(toggleButton);
|
||||
|
||||
// Dependency selector button should be visible
|
||||
const depsButton = await screen.findByTestId("quick-entry-deps-button");
|
||||
expect(depsButton).toBeDefined();
|
||||
expect(document.getElementById("quick-entry-controls")?.hasAttribute("hidden")).toBe(false);
|
||||
});
|
||||
|
||||
it("calls onQuickCreate with description when Enter is pressed", async () => {
|
||||
|
||||
@@ -822,7 +822,7 @@ describe("QuickEntryBox", () => {
|
||||
expect(textarea.classList.contains("quick-entry-input--expanded")).toBe(false);
|
||||
});
|
||||
|
||||
it("resets all state after successful creation (preserves disclosure preference)", async () => {
|
||||
it("resets all state after successful creation (disclosure resets to collapsed)", async () => {
|
||||
const { props } = renderQuickEntryBox({});
|
||||
expandQuickEntry();
|
||||
const textarea = screen.getByTestId("quick-entry-input");
|
||||
@@ -837,11 +837,11 @@ describe("QuickEntryBox", () => {
|
||||
|
||||
// After creation, input is cleared and focus is restored
|
||||
expect((textarea as HTMLTextAreaElement).value).toBe("");
|
||||
|
||||
|
||||
// With autoExpand=true (default), textarea auto-expands on focus restore
|
||||
// So the toggle should be expanded (controls visible)
|
||||
expect(screen.getByTestId("quick-entry-toggle").getAttribute("aria-expanded")).toBe("true");
|
||||
expect(document.getElementById("quick-entry-controls")?.hasAttribute("hidden")).toBe(false);
|
||||
// but disclosure resets to collapsed — controls hidden until user toggles again
|
||||
expect(screen.getByTestId("quick-entry-toggle").getAttribute("aria-expanded")).toBe("false");
|
||||
expect(document.getElementById("quick-entry-controls")?.hasAttribute("hidden")).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -927,17 +927,44 @@ describe("QuickEntryBox", () => {
|
||||
localStorage.clear();
|
||||
});
|
||||
|
||||
it("restores disclosure state from localStorage on mount", () => {
|
||||
// Pre-populate localStorage with expanded state
|
||||
it("ignores legacy kb-quick-entry-expanded localStorage on mount", () => {
|
||||
// Pre-populate localStorage with expanded state from a previous version
|
||||
localStorage.setItem("kb-quick-entry-expanded", "true");
|
||||
|
||||
renderQuickEntryBox();
|
||||
const toggleButton = screen.getByTestId("quick-entry-toggle");
|
||||
|
||||
// Should restore the saved disclosure state (expanded)
|
||||
expect(toggleButton.getAttribute("aria-expanded")).toBe("true");
|
||||
// Controls should be visible
|
||||
expect(screen.getByTestId("quick-entry-deps-button")).toBeTruthy();
|
||||
// Should NOT restore the saved disclosure state — always starts collapsed
|
||||
expect(toggleButton.getAttribute("aria-expanded")).toBe("false");
|
||||
// Controls should be hidden
|
||||
expect(document.getElementById("quick-entry-controls")?.hasAttribute("hidden")).toBe(true);
|
||||
});
|
||||
|
||||
it("saved draft description does not reveal controls panel", () => {
|
||||
// Pre-populate localStorage with saved draft text
|
||||
localStorage.setItem("kb-quick-entry-text", "Previously saved draft task");
|
||||
|
||||
renderQuickEntryBox();
|
||||
const textarea = screen.getByTestId("quick-entry-input");
|
||||
const controls = document.getElementById("quick-entry-controls");
|
||||
|
||||
// Description should be restored
|
||||
expect((textarea as HTMLTextAreaElement).value).toBe("Previously saved draft task");
|
||||
// But controls should remain hidden — draft text does not auto-expand disclosure
|
||||
expect(controls?.hasAttribute("hidden")).toBe(true);
|
||||
expect(screen.getByTestId("quick-entry-toggle").getAttribute("aria-expanded")).toBe("false");
|
||||
});
|
||||
|
||||
it("cleans up legacy kb-quick-entry-expanded key on mount", async () => {
|
||||
// Pre-populate localStorage with legacy key
|
||||
localStorage.setItem("kb-quick-entry-expanded", "true");
|
||||
|
||||
renderQuickEntryBox();
|
||||
|
||||
// The legacy key should be removed by the cleanup useEffect
|
||||
await waitFor(() => {
|
||||
expect(localStorage.getItem("kb-quick-entry-expanded")).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
it("defaults to collapsed when localStorage is empty", () => {
|
||||
@@ -950,15 +977,10 @@ describe("QuickEntryBox", () => {
|
||||
expect(document.getElementById("quick-entry-controls")?.hasAttribute("hidden")).toBe(true);
|
||||
});
|
||||
|
||||
it("updates localStorage when toggling disclosure", async () => {
|
||||
it("does not persist disclosure state to localStorage when toggling", async () => {
|
||||
renderQuickEntryBox({});
|
||||
const toggleButton = screen.getByTestId("quick-entry-toggle");
|
||||
|
||||
// Wait for initial state to be persisted (useEffect runs after mount)
|
||||
await waitFor(() => {
|
||||
expect(localStorage.getItem("kb-quick-entry-expanded")).toBe("false");
|
||||
});
|
||||
|
||||
// Initially collapsed
|
||||
expect(toggleButton.getAttribute("aria-expanded")).toBe("false");
|
||||
|
||||
@@ -967,20 +989,16 @@ describe("QuickEntryBox", () => {
|
||||
|
||||
// Should be expanded
|
||||
expect(toggleButton.getAttribute("aria-expanded")).toBe("true");
|
||||
// localStorage should be updated
|
||||
await waitFor(() => {
|
||||
expect(localStorage.getItem("kb-quick-entry-expanded")).toBe("true");
|
||||
});
|
||||
// localStorage should NOT be updated — disclosure is ephemeral
|
||||
expect(localStorage.getItem("kb-quick-entry-expanded")).toBeNull();
|
||||
|
||||
// Click to collapse
|
||||
fireEvent.click(toggleButton);
|
||||
|
||||
// Should be collapsed
|
||||
expect(toggleButton.getAttribute("aria-expanded")).toBe("false");
|
||||
// localStorage should be updated
|
||||
await waitFor(() => {
|
||||
expect(localStorage.getItem("kb-quick-entry-expanded")).toBe("false");
|
||||
});
|
||||
// localStorage still should not have the key
|
||||
expect(localStorage.getItem("kb-quick-entry-expanded")).toBeNull();
|
||||
});
|
||||
|
||||
it("aria-expanded attribute updates correctly when toggling", () => {
|
||||
|
||||
@@ -611,4 +611,58 @@ describe("TerminalModal — mobile layout contract", () => {
|
||||
expect(connectionStatus?.textContent).toBe("Disconnected");
|
||||
});
|
||||
});
|
||||
|
||||
it("delivers buffered terminal output to xterm when subscriptions are established after websocket messages", async () => {
|
||||
// This test verifies that the useTerminal hook's early message buffering
|
||||
// works correctly with TerminalModal's late-subscription pattern (xterm
|
||||
// must initialize before onData/onScrollback/onConnect are wired up).
|
||||
// The hook's buffer ensures scrollback and early shell output are not lost.
|
||||
|
||||
let capturedDataCallback: ((data: string) => void) | null = null;
|
||||
let capturedScrollbackCallback: ((data: string) => void) | null = null;
|
||||
|
||||
const mockOnData = vi.fn((cb: (data: string) => void) => {
|
||||
capturedDataCallback = cb;
|
||||
return vi.fn();
|
||||
});
|
||||
const mockOnScrollback = vi.fn((cb: (data: string) => void) => {
|
||||
capturedScrollbackCallback = cb;
|
||||
return vi.fn();
|
||||
});
|
||||
|
||||
mockUseTerminal.mockReturnValue(
|
||||
createMockTerminalState({
|
||||
connectionStatus: "connected",
|
||||
onData: mockOnData,
|
||||
onScrollback: mockOnScrollback,
|
||||
})
|
||||
);
|
||||
|
||||
render(<TerminalModal isOpen={true} onClose={mockOnClose} />);
|
||||
|
||||
// Wait for xterm to initialize and subscriptions to be established
|
||||
await waitFor(() => {
|
||||
expect(mockTerminalInstance.open).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
await waitFor(() => {
|
||||
expect(mockOnData).toHaveBeenCalled();
|
||||
expect(mockOnScrollback).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
// Now simulate late-arriving data (after subscriptions are wired)
|
||||
// This verifies the write path from callback to xterm
|
||||
act(() => {
|
||||
if (capturedDataCallback) {
|
||||
capturedDataCallback("prompt$ ");
|
||||
}
|
||||
if (capturedScrollbackCallback) {
|
||||
capturedScrollbackCallback("previous output");
|
||||
}
|
||||
});
|
||||
|
||||
// xterm should receive the data via write()
|
||||
expect(mockTerminalInstance.write).toHaveBeenCalledWith("prompt$ ");
|
||||
expect(mockTerminalInstance.write).toHaveBeenCalledWith("previous output");
|
||||
});
|
||||
});
|
||||
|
||||
@@ -185,4 +185,148 @@ describe("useTerminal", () => {
|
||||
|
||||
expect(result.current.connectionStatus).toBe("disconnected");
|
||||
});
|
||||
|
||||
describe("early message buffering", () => {
|
||||
it("replays buffered scrollback to late subscribers", () => {
|
||||
const { result } = renderHook(() => useTerminal("test-session-123"));
|
||||
|
||||
// Send scrollback BEFORE any subscriber is registered
|
||||
act(() => {
|
||||
MockWebSocket.instances[0].emitMessage({ type: "scrollback", data: "previous output" });
|
||||
});
|
||||
|
||||
const onScrollback = vi.fn();
|
||||
act(() => {
|
||||
result.current.onScrollback(onScrollback);
|
||||
});
|
||||
|
||||
// The late subscriber should receive the buffered scrollback
|
||||
expect(onScrollback).toHaveBeenCalledWith("previous output");
|
||||
});
|
||||
|
||||
it("replays buffered connected info to late subscribers", () => {
|
||||
const { result } = renderHook(() => useTerminal("test-session-123"));
|
||||
|
||||
// Send connected info BEFORE any subscriber is registered
|
||||
act(() => {
|
||||
MockWebSocket.instances[0].emitMessage({ type: "connected", shell: "/bin/zsh", cwd: "/home/user" });
|
||||
});
|
||||
|
||||
const onConnect = vi.fn();
|
||||
act(() => {
|
||||
result.current.onConnect(onConnect);
|
||||
});
|
||||
|
||||
// The late subscriber should receive the buffered connected info
|
||||
expect(onConnect).toHaveBeenCalledWith({ shell: "/bin/zsh", cwd: "/home/user" });
|
||||
});
|
||||
|
||||
it("replays buffered data messages to late subscribers", () => {
|
||||
const { result } = renderHook(() => useTerminal("test-session-123"));
|
||||
|
||||
// Send data messages BEFORE any subscriber is registered
|
||||
act(() => {
|
||||
MockWebSocket.instances[0].emitMessage({ type: "data", data: "prompt$ " });
|
||||
MockWebSocket.instances[0].emitMessage({ type: "data", data: "more output" });
|
||||
});
|
||||
|
||||
const onData = vi.fn();
|
||||
act(() => {
|
||||
result.current.onData(onData);
|
||||
});
|
||||
|
||||
// The late subscriber should receive all buffered data messages in order
|
||||
expect(onData).toHaveBeenCalledTimes(2);
|
||||
expect(onData).toHaveBeenNthCalledWith(1, "prompt$ ");
|
||||
expect(onData).toHaveBeenNthCalledWith(2, "more output");
|
||||
});
|
||||
|
||||
it("does not double-deliver messages to early subscribers", () => {
|
||||
const { result } = renderHook(() => useTerminal("test-session-123"));
|
||||
|
||||
const onScrollback = vi.fn();
|
||||
const onConnect = vi.fn();
|
||||
const onData = vi.fn();
|
||||
|
||||
// Register subscribers BEFORE messages arrive
|
||||
act(() => {
|
||||
result.current.onScrollback(onScrollback);
|
||||
result.current.onConnect(onConnect);
|
||||
result.current.onData(onData);
|
||||
});
|
||||
|
||||
act(() => {
|
||||
MockWebSocket.instances[0].emitMessage({ type: "scrollback", data: "scrollback data" });
|
||||
MockWebSocket.instances[0].emitMessage({ type: "connected", shell: "/bin/bash", cwd: "/project" });
|
||||
MockWebSocket.instances[0].emitMessage({ type: "data", data: "hello" });
|
||||
});
|
||||
|
||||
// Each callback should be called exactly once (no replay double-count)
|
||||
expect(onScrollback).toHaveBeenCalledTimes(1);
|
||||
expect(onConnect).toHaveBeenCalledTimes(1);
|
||||
expect(onData).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("replays buffered messages only once per subscriber registration", () => {
|
||||
const { result } = renderHook(() => useTerminal("test-session-123"));
|
||||
|
||||
act(() => {
|
||||
MockWebSocket.instances[0].emitMessage({ type: "scrollback", data: "buf" });
|
||||
});
|
||||
|
||||
const sub1 = vi.fn();
|
||||
const sub2 = vi.fn();
|
||||
|
||||
act(() => {
|
||||
result.current.onScrollback(sub1);
|
||||
});
|
||||
|
||||
act(() => {
|
||||
result.current.onScrollback(sub2);
|
||||
});
|
||||
|
||||
// Both subscribers should get the buffered scrollback
|
||||
expect(sub1).toHaveBeenCalledWith("buf");
|
||||
expect(sub2).toHaveBeenCalledWith("buf");
|
||||
|
||||
// New live messages should go to both without re-delivering buffer
|
||||
act(() => {
|
||||
MockWebSocket.instances[0].emitMessage({ type: "scrollback", data: "live-update" });
|
||||
});
|
||||
|
||||
expect(sub1).toHaveBeenCalledTimes(2); // buffer + live
|
||||
expect(sub2).toHaveBeenCalledTimes(2); // buffer + live
|
||||
});
|
||||
|
||||
it("clears buffer on reconnect so stale data is not replayed", () => {
|
||||
const { result } = renderHook(() => useTerminal("test-session-123"));
|
||||
|
||||
// First connection receives messages
|
||||
act(() => {
|
||||
MockWebSocket.instances[0].emitMessage({ type: "scrollback", data: "old data" });
|
||||
MockWebSocket.instances[0].emitMessage({ type: "connected", shell: "/bin/bash", cwd: "/old" });
|
||||
});
|
||||
|
||||
// Reconnect — this creates a new WebSocket
|
||||
act(() => {
|
||||
result.current.reconnect();
|
||||
});
|
||||
|
||||
// Second connection (new MockWebSocket instance at index 1)
|
||||
// The reconnect closes the old ws and opens a new one
|
||||
const newWs = MockWebSocket.instances[MockWebSocket.instances.length - 1];
|
||||
|
||||
const onScrollback = vi.fn();
|
||||
const onConnect = vi.fn();
|
||||
|
||||
act(() => {
|
||||
result.current.onScrollback(onScrollback);
|
||||
result.current.onConnect(onConnect);
|
||||
});
|
||||
|
||||
// Subscribers registered on the new connection should NOT get old buffer
|
||||
expect(onScrollback).not.toHaveBeenCalled();
|
||||
expect(onConnect).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -31,10 +31,22 @@ interface WebSocketMessage {
|
||||
rows?: number;
|
||||
}
|
||||
|
||||
/** Buffered initial message types that must survive late subscriber registration */
|
||||
interface BufferedMessages {
|
||||
scrollback: string | null;
|
||||
connected: { shell: string; cwd: string } | null;
|
||||
/** Accumulated data messages received before any subscriber registered */
|
||||
data: string[];
|
||||
}
|
||||
|
||||
const MAX_RECONNECT_ATTEMPTS = 5;
|
||||
const INITIAL_RECONNECT_DELAY = 1000;
|
||||
const HEARTBEAT_INTERVAL = 30000;
|
||||
|
||||
function createEmptyBuffer(): BufferedMessages {
|
||||
return { scrollback: null, connected: null, data: [] };
|
||||
}
|
||||
|
||||
/**
|
||||
* React hook for managing terminal WebSocket connection.
|
||||
*
|
||||
@@ -44,6 +56,9 @@ const HEARTBEAT_INTERVAL = 30000;
|
||||
* - Resize support
|
||||
* - Heartbeat ping/pong
|
||||
* - Scrollback buffer replay on connect
|
||||
* - Early message buffering: scrollback, connected, and initial data messages
|
||||
* are buffered and replayed to subscribers that register after the WebSocket
|
||||
* starts receiving events (e.g. while xterm is still initializing).
|
||||
*
|
||||
* @example
|
||||
* ```tsx
|
||||
@@ -72,9 +87,19 @@ export function useTerminal(sessionId: string | null): UseTerminalReturn {
|
||||
const onConnectCallbacksRef = useRef<Set<(info: { shell: string; cwd: string }) => void>>(new Set());
|
||||
const onScrollbackCallbacksRef = useRef<Set<(data: string) => void>>(new Set());
|
||||
|
||||
// Register callbacks
|
||||
// Buffer for initial messages received before subscribers are registered.
|
||||
// This ensures scrollback, connected info, and early shell output are
|
||||
// delivered even if TerminalModal's xterm hasn't finished initializing.
|
||||
const initialBufferRef = useRef<BufferedMessages>(createEmptyBuffer());
|
||||
|
||||
// Register callbacks — replay buffered data to late subscribers
|
||||
const onData = useCallback((callback: (data: string) => void) => {
|
||||
onDataCallbacksRef.current.add(callback);
|
||||
// Replay buffered data messages
|
||||
const buffer = initialBufferRef.current;
|
||||
if (buffer.data.length > 0) {
|
||||
buffer.data.forEach((d) => callback(d));
|
||||
}
|
||||
return () => onDataCallbacksRef.current.delete(callback);
|
||||
}, []);
|
||||
|
||||
@@ -85,11 +110,21 @@ export function useTerminal(sessionId: string | null): UseTerminalReturn {
|
||||
|
||||
const onConnect = useCallback((callback: (info: { shell: string; cwd: string }) => void) => {
|
||||
onConnectCallbacksRef.current.add(callback);
|
||||
// Replay buffered connected info
|
||||
const buffer = initialBufferRef.current;
|
||||
if (buffer.connected) {
|
||||
callback(buffer.connected);
|
||||
}
|
||||
return () => onConnectCallbacksRef.current.delete(callback);
|
||||
}, []);
|
||||
|
||||
const onScrollback = useCallback((callback: (data: string) => void) => {
|
||||
onScrollbackCallbacksRef.current.add(callback);
|
||||
// Replay buffered scrollback
|
||||
const buffer = initialBufferRef.current;
|
||||
if (buffer.scrollback) {
|
||||
callback(buffer.scrollback);
|
||||
}
|
||||
return () => onScrollbackCallbacksRef.current.delete(callback);
|
||||
}, []);
|
||||
|
||||
@@ -126,6 +161,9 @@ export function useTerminal(sessionId: string | null): UseTerminalReturn {
|
||||
wsRef.current.close();
|
||||
wsRef.current = null;
|
||||
}
|
||||
|
||||
// Clear buffers on cleanup
|
||||
initialBufferRef.current = createEmptyBuffer();
|
||||
}, []);
|
||||
|
||||
// Connect function
|
||||
@@ -147,6 +185,8 @@ export function useTerminal(sessionId: string | null): UseTerminalReturn {
|
||||
}
|
||||
|
||||
isManualCloseRef.current = false;
|
||||
// Reset buffer for new connection — previous session's data is stale
|
||||
initialBufferRef.current = createEmptyBuffer();
|
||||
setConnectionStatus("connecting");
|
||||
|
||||
// Build WebSocket URL
|
||||
@@ -175,20 +215,29 @@ export function useTerminal(sessionId: string | null): UseTerminalReturn {
|
||||
ws.onmessage = (event) => {
|
||||
try {
|
||||
const msg: WebSocketMessage = JSON.parse(event.data);
|
||||
const buffer = initialBufferRef.current;
|
||||
|
||||
switch (msg.type) {
|
||||
case "data":
|
||||
if (msg.data) {
|
||||
// Buffer data when no subscribers are registered yet
|
||||
if (onDataCallbacksRef.current.size === 0) {
|
||||
buffer.data.push(msg.data!);
|
||||
}
|
||||
onDataCallbacksRef.current.forEach((cb) => cb(msg.data!));
|
||||
}
|
||||
break;
|
||||
case "scrollback":
|
||||
if (msg.data) {
|
||||
// Buffer scrollback for late subscribers
|
||||
buffer.scrollback = msg.data;
|
||||
onScrollbackCallbacksRef.current.forEach((cb) => cb(msg.data!));
|
||||
}
|
||||
break;
|
||||
case "connected":
|
||||
if (msg.shell && msg.cwd) {
|
||||
// Buffer connected info for late subscribers
|
||||
buffer.connected = { shell: msg.shell!, cwd: msg.cwd! };
|
||||
onConnectCallbacksRef.current.forEach((cb) =>
|
||||
cb({ shell: msg.shell!, cwd: msg.cwd! })
|
||||
);
|
||||
|
||||
@@ -4922,10 +4922,9 @@ describe("Terminal session routes", () => {
|
||||
});
|
||||
|
||||
describe("Terminal WebSocket close handler", () => {
|
||||
it("kills PTY session when WebSocket closes", async () => {
|
||||
// This tests the server.ts close handler logic by verifying that
|
||||
// setupTerminalWebSocket's close handler calls killSession.
|
||||
// We import server.ts and mock the terminal service.
|
||||
it("does NOT kill PTY session when WebSocket closes (session persists for reconnect)", async () => {
|
||||
// After FN-762, closing a WebSocket must not destroy the PTY session.
|
||||
// The session survives transient disconnects and modal close/reopen cycles.
|
||||
const killSessionMock = vi.fn().mockReturnValue(true);
|
||||
const getSessionMock = vi.fn().mockReturnValue({
|
||||
id: "term-ws-test",
|
||||
@@ -4949,7 +4948,6 @@ describe("Terminal WebSocket close handler", () => {
|
||||
|
||||
vi.spyOn(terminalServiceModule, "getTerminalService").mockReturnValue(mockService as any);
|
||||
|
||||
// Dynamically import to get fresh module with the mock
|
||||
const { setupTerminalWebSocket } = await import("./server.js");
|
||||
|
||||
const app = express();
|
||||
@@ -4973,12 +4971,13 @@ describe("Terminal WebSocket close handler", () => {
|
||||
|
||||
ws.close();
|
||||
|
||||
expect(killSessionMock).toHaveBeenCalledWith("term-ws-test");
|
||||
// The session must NOT be killed on WebSocket close
|
||||
expect(killSessionMock).not.toHaveBeenCalled();
|
||||
|
||||
vi.restoreAllMocks();
|
||||
});
|
||||
|
||||
it("kills PTY session when WebSocket encounters an error", async () => {
|
||||
it("does NOT kill PTY session when WebSocket encounters an error (session persists for reconnect)", async () => {
|
||||
const killSessionMock = vi.fn().mockReturnValue(true);
|
||||
const getSessionMock = vi.fn().mockReturnValue({
|
||||
id: "term-ws-err",
|
||||
@@ -5024,7 +5023,67 @@ describe("Terminal WebSocket close handler", () => {
|
||||
|
||||
ws.emit("error", new Error("synthetic websocket failure"));
|
||||
|
||||
expect(killSessionMock).toHaveBeenCalledWith("term-ws-err");
|
||||
// The session must NOT be killed on WebSocket error
|
||||
expect(killSessionMock).not.toHaveBeenCalled();
|
||||
|
||||
vi.restoreAllMocks();
|
||||
});
|
||||
|
||||
it("cleans up data/exit subscriptions on WebSocket close without killing session", async () => {
|
||||
// Verify that WebSocket close properly unsubscribes from terminal service
|
||||
// events without destroying the underlying PTY session.
|
||||
const killSessionMock = vi.fn().mockReturnValue(true);
|
||||
const dataUnsub = vi.fn();
|
||||
const exitUnsub = vi.fn();
|
||||
const getSessionMock = vi.fn().mockReturnValue({
|
||||
id: "term-ws-unsub",
|
||||
shell: "/bin/zsh",
|
||||
cwd: "/test/project",
|
||||
});
|
||||
const getScrollbackAndClearPendingMock = vi.fn().mockReturnValue(null);
|
||||
const onDataMock = vi.fn().mockReturnValue(dataUnsub);
|
||||
const onExitMock = vi.fn().mockReturnValue(exitUnsub);
|
||||
|
||||
const mockService = {
|
||||
getSession: getSessionMock,
|
||||
getScrollbackAndClearPending: getScrollbackAndClearPendingMock,
|
||||
killSession: killSessionMock,
|
||||
write: vi.fn(),
|
||||
resize: vi.fn(),
|
||||
onData: onDataMock,
|
||||
onExit: onExitMock,
|
||||
};
|
||||
|
||||
vi.spyOn(terminalServiceModule, "getTerminalService").mockReturnValue(mockService as any);
|
||||
|
||||
const { setupTerminalWebSocket } = await import("./server.js");
|
||||
|
||||
const app = express();
|
||||
const server = http.createServer(app);
|
||||
|
||||
setupTerminalWebSocket(app, server);
|
||||
class FakeWebSocket extends EventEmitter {
|
||||
send = vi.fn();
|
||||
close = vi.fn(() => this.emit("close"));
|
||||
terminate = vi.fn();
|
||||
}
|
||||
|
||||
const ws = new FakeWebSocket();
|
||||
const wss = (app as express.Express & { terminalWsServer?: EventEmitter }).terminalWsServer;
|
||||
expect(wss).toBeTruthy();
|
||||
|
||||
wss!.emit("connection", ws, {
|
||||
url: "/api/terminal/ws?sessionId=term-ws-unsub",
|
||||
headers: { host: "127.0.0.1" },
|
||||
});
|
||||
|
||||
ws.close();
|
||||
|
||||
// Subscriptions should be cleaned up
|
||||
expect(dataUnsub).toHaveBeenCalled();
|
||||
expect(exitUnsub).toHaveBeenCalled();
|
||||
// But session should NOT be killed
|
||||
expect(killSessionMock).not.toHaveBeenCalled();
|
||||
|
||||
vi.restoreAllMocks();
|
||||
});
|
||||
|
||||
@@ -461,12 +461,10 @@ export function setupTerminalWebSocket(
|
||||
clearInterval(pingInterval);
|
||||
if (dataUnsub) dataUnsub();
|
||||
if (exitUnsub) exitUnsub();
|
||||
// Kill the PTY session to prevent session leaks
|
||||
try {
|
||||
terminalService.killSession(sessionId);
|
||||
} catch {
|
||||
// Ignore errors during cleanup — session may already be dead
|
||||
}
|
||||
// Do NOT kill the PTY session on WebSocket close — the session should
|
||||
// survive transient disconnects and modal close/reopen cycles. Sessions
|
||||
// are cleaned up through explicit kill paths (tab close, restart, shell
|
||||
// exit) or stale-session eviction.
|
||||
});
|
||||
|
||||
ws.on("error", () => {
|
||||
@@ -474,15 +472,28 @@ export function setupTerminalWebSocket(
|
||||
clearInterval(pingInterval);
|
||||
if (dataUnsub) dataUnsub();
|
||||
if (exitUnsub) exitUnsub();
|
||||
// Kill the PTY session to prevent session leaks
|
||||
try {
|
||||
terminalService.killSession(sessionId);
|
||||
} catch {
|
||||
// Ignore errors during cleanup — session may already be dead
|
||||
}
|
||||
// Do NOT kill the PTY session on WebSocket error — same rationale as
|
||||
// close: the session should persist for reconnection attempts.
|
||||
});
|
||||
});
|
||||
|
||||
// Periodic stale-session eviction (every 60 s) so that PTY sessions are
|
||||
// eventually cleaned up when clients disconnect permanently without going
|
||||
// through explicit kill paths. The eviction threshold is defined by
|
||||
// TerminalService (default 5 minutes of inactivity).
|
||||
const staleEvictionInterval = setInterval(() => {
|
||||
try {
|
||||
terminalService.evictStaleSessions();
|
||||
} catch {
|
||||
// Ignore errors during periodic eviction
|
||||
}
|
||||
}, 60_000);
|
||||
|
||||
// Stop eviction timer when the server shuts down
|
||||
server.once("close", () => {
|
||||
clearInterval(staleEvictionInterval);
|
||||
});
|
||||
|
||||
console.log("Terminal WebSocket server mounted at /api/terminal/ws");
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user