From b9d21d1f18ff4e3b7dcddf9017a8f1de28ab31cd Mon Sep 17 00:00:00 2001 From: Gaurav Bhola Date: Thu, 2 Apr 2026 19:12:28 -0700 Subject: [PATCH] fix(web): address bugbot review issues from second pass - Replace Node.js Buffer.byteLength() with TextEncoder in MuxProvider (browser-safe UTF-8 byte counting for the ring buffer) - Remove write-only sessionSubscribedRef from MuxProvider - Remove dead DirectTerminalLocation, DirectTerminalWsUrlOptions, buildDirectTerminalWsUrl and their tests (superseded by mux) - Call closeTerminal(sessionId) on DirectTerminal unmount so PTY processes are released and openedTerminalsRef stays accurate - Add cleanup return to mux effect in useSessionEvents so pending refresh timers and abort controllers are cleared on unmount Co-Authored-By: Claude Opus 4.6 --- .../web/src/components/DirectTerminal.tsx | 39 +----------- .../__tests__/DirectTerminal.test.ts | 63 +------------------ packages/web/src/hooks/useSessionEvents.ts | 14 +++-- packages/web/src/providers/MuxProvider.tsx | 7 +-- 4 files changed, 17 insertions(+), 106 deletions(-) diff --git a/packages/web/src/components/DirectTerminal.tsx b/packages/web/src/components/DirectTerminal.tsx index ce5a59928..1fdae977f 100644 --- a/packages/web/src/components/DirectTerminal.tsx +++ b/packages/web/src/components/DirectTerminal.tsx @@ -25,20 +25,6 @@ interface DirectTerminalProps { reloadCommand?: string; } -interface DirectTerminalLocation { - protocol: string; - hostname: string; - host: string; - port: string; -} - -interface DirectTerminalWsUrlOptions { - location: DirectTerminalLocation; - sessionId: string; - proxyWsPath?: string; - directTerminalPort?: string; -} - type TerminalVariant = "agent" | "orchestrator"; @@ -106,26 +92,6 @@ export function buildTerminalThemes(variant: TerminalVariant): { dark: ITheme; l return { dark, light }; } -export function buildDirectTerminalWsUrl({ - location, - sessionId, - proxyWsPath, - directTerminalPort, -}: DirectTerminalWsUrlOptions): string { - const protocol = location.protocol === "https:" ? "wss:" : "ws:"; - if (proxyWsPath) { - // Path-based proxy uses host so non-standard ports are preserved. - return `${protocol}//${location.host}${proxyWsPath}?session=${encodeURIComponent(sessionId)}`; - } - - if (location.port === "" || location.port === "443" || location.port === "80") { - return `${protocol}//${location.hostname}/ao-terminal-ws?session=${encodeURIComponent(sessionId)}`; - } - - const port = directTerminalPort ?? "14801"; - return `${protocol}//${location.hostname}:${port}/ws?session=${encodeURIComponent(sessionId)}`; -} - /** * Direct xterm.js terminal with native WebSocket connection. * Implements Extended Device Attributes (XDA) handler to enable @@ -149,7 +115,7 @@ export function DirectTerminal({ const searchParams = useSearchParams(); const { resolvedTheme } = useTheme(); const terminalThemes = useMemo(() => buildTerminalThemes(variant), [variant]); - const { subscribeTerminal, writeTerminal, resizeTerminal: resizeTerminalMux, openTerminal, status: muxStatus } = useMux(); + const { subscribeTerminal, writeTerminal, resizeTerminal: resizeTerminalMux, openTerminal, closeTerminal, status: muxStatus } = useMux(); const terminalRef = useRef(null); const terminalInstance = useRef(null); @@ -407,6 +373,7 @@ export function DirectTerminal({ inputDisposable?.dispose(); inputDisposable = null; unsubscribe?.(); + closeTerminal(sessionId); terminal.dispose(); }; }) @@ -420,7 +387,7 @@ export function DirectTerminal({ mounted = false; cleanup?.(); }; - }, [sessionId, variant, subscribeTerminal, writeTerminal, resizeTerminalMux, openTerminal]); + }, [sessionId, variant, subscribeTerminal, writeTerminal, resizeTerminalMux, openTerminal, closeTerminal]); // Live theme switching without terminal recreation useEffect(() => { diff --git a/packages/web/src/components/__tests__/DirectTerminal.test.ts b/packages/web/src/components/__tests__/DirectTerminal.test.ts index cc1d6f690..cf7b9b483 100644 --- a/packages/web/src/components/__tests__/DirectTerminal.test.ts +++ b/packages/web/src/components/__tests__/DirectTerminal.test.ts @@ -1,66 +1,5 @@ import { describe, it, expect } from "vitest"; -import { buildDirectTerminalWsUrl, buildTerminalThemes } from "@/components/DirectTerminal"; - -describe("buildDirectTerminalWsUrl", () => { - it("keeps non-standard port when proxy path override is set", () => { - const wsUrl = buildDirectTerminalWsUrl({ - location: { - protocol: "https:", - hostname: "example.com", - host: "example.com:8443", - port: "8443", - }, - sessionId: "session-1", - proxyWsPath: "/ao-terminal-ws", - }); - - expect(wsUrl).toBe("wss://example.com:8443/ao-terminal-ws?session=session-1"); - }); - - it("uses proxy path without port when default port is used", () => { - const wsUrl = buildDirectTerminalWsUrl({ - location: { - protocol: "https:", - hostname: "example.com", - host: "example.com", - port: "", - }, - sessionId: "session-2", - proxyWsPath: "/ao-terminal-ws", - }); - - expect(wsUrl).toBe("wss://example.com/ao-terminal-ws?session=session-2"); - }); - - it("uses default path-based endpoint on standard ports when no proxy override is set", () => { - const wsUrl = buildDirectTerminalWsUrl({ - location: { - protocol: "https:", - hostname: "example.com", - host: "example.com", - port: "443", - }, - sessionId: "session-3", - }); - - expect(wsUrl).toBe("wss://example.com/ao-terminal-ws?session=session-3"); - }); - - it("uses direct terminal port on non-standard ports when no proxy override is set", () => { - const wsUrl = buildDirectTerminalWsUrl({ - location: { - protocol: "http:", - hostname: "localhost", - host: "localhost:3000", - port: "3000", - }, - sessionId: "session-4", - directTerminalPort: "14888", - }); - - expect(wsUrl).toBe("ws://localhost:14888/ws?session=session-4"); - }); -}); +import { buildTerminalThemes } from "@/components/DirectTerminal"; const HEX_RE = /^#[0-9a-fA-F]{6}$/; const ANSI_KEYS = [ diff --git a/packages/web/src/hooks/useSessionEvents.ts b/packages/web/src/hooks/useSessionEvents.ts index 42282b795..a46ab6735 100644 --- a/packages/web/src/hooks/useSessionEvents.ts +++ b/packages/web/src/hooks/useSessionEvents.ts @@ -204,12 +204,18 @@ export function useSessionEvents( if (currentMembershipKey !== snapshotMembershipKey) { pendingMembershipKeyRef.current = snapshotMembershipKey; scheduleRefresh(); - return; - } - - if (Date.now() - lastRefreshAtRef.current >= STALE_REFRESH_INTERVAL_MS) { + } else if (Date.now() - lastRefreshAtRef.current >= STALE_REFRESH_INTERVAL_MS) { scheduleRefresh(); } + + return () => { + if (refreshTimerRef.current) { + clearTimeout(refreshTimerRef.current); + refreshTimerRef.current = null; + } + activeRefreshControllerRef.current?.abort(); + activeRefreshControllerRef.current = null; + }; }, [muxSessions, scheduleRefresh]); useEffect(() => { diff --git a/packages/web/src/providers/MuxProvider.tsx b/packages/web/src/providers/MuxProvider.tsx index 962ddedbd..8f1209a0d 100644 --- a/packages/web/src/providers/MuxProvider.tsx +++ b/packages/web/src/providers/MuxProvider.tsx @@ -82,7 +82,6 @@ export function MuxProvider({ children }: { children: ReactNode }) { const reconnectAttempt = useRef(0); const reconnectTimer = useRef | null>(null); const runtimeConfigRef = useRef<{ directTerminalPort?: string; proxyWsPath?: string }>({}); - const sessionSubscribedRef = useRef(false); const connect = useCallback(() => { if (wsRef.current) { @@ -113,7 +112,6 @@ export function MuxProvider({ children }: { children: ReactNode }) { } // Always subscribe to sessions - sessionSubscribedRef.current = true; const subMsg: ClientMessage = { ch: "subscribe", topics: ["sessions"], @@ -140,8 +138,9 @@ export function MuxProvider({ children }: { children: ReactNode }) { buffer.push(msg.data); buffersRef.current.set(msg.id, buffer); + const encoder = new TextEncoder(); let bytes = bufferBytesRef.current.get(msg.id) ?? 0; - bytes += Buffer.byteLength(msg.data, "utf8"); + bytes += encoder.encode(msg.data).length; bufferBytesRef.current.set(msg.id, bytes); // Trim buffer if needed (50KB limit) @@ -149,7 +148,7 @@ export function MuxProvider({ children }: { children: ReactNode }) { if (bytes > RING_BUFFER_MAX) { while (bytes > RING_BUFFER_MAX && buffer.length > 0) { const removed = buffer.shift() ?? ""; - bytes -= Buffer.byteLength(removed, "utf8"); + bytes -= encoder.encode(removed).length; } bufferBytesRef.current.set(msg.id, bytes); }