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 <noreply@anthropic.com>
This commit is contained in:
parent
f2986bf863
commit
b9d21d1f18
|
|
@ -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<HTMLDivElement>(null);
|
||||
const terminalInstance = useRef<TerminalType | null>(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(() => {
|
||||
|
|
|
|||
|
|
@ -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 = [
|
||||
|
|
|
|||
|
|
@ -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(() => {
|
||||
|
|
|
|||
|
|
@ -82,7 +82,6 @@ export function MuxProvider({ children }: { children: ReactNode }) {
|
|||
const reconnectAttempt = useRef(0);
|
||||
const reconnectTimer = useRef<ReturnType<typeof setTimeout> | 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);
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue