Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,237 @@
import { getAuthToken } from "../../../../../trpc/auth-token";
import { getRelayUrl } from "../../../../../trpc/relay-url";

export type TerminalConnectionState = "connecting" | "reconnecting" | "error";

type TerminalServerMessage =
| { type: "attached"; terminalId: string }
| { type: "title"; title: string | null }
| { type: "error"; message: string }
| { type: "exit"; exitCode: number; signal: number };

export type TerminalControlMessage = TerminalServerMessage;

type TerminalClientMessage =
| { type: "input"; data: string }
| { type: "resize"; cols: number; rows: number };

interface TerminalConnectionTarget {
workspaceId: string;
terminalId: string;
routingKey: string;
}

interface TerminalConnectionHandlers {
onBinary: (bytes: Uint8Array) => void;
onControl: (message: TerminalControlMessage) => void;
onStateChange: (state: TerminalConnectionState) => void;
}

const BASE_RECONNECT_DELAY_MS = 500;
const MAX_RECONNECT_DELAY_MS = 10_000;
const MAX_RECONNECT_ATTEMPTS = 12;

// Owns the terminal WebSocket lifecycle: exponential-backoff reconnect on an
// unexpected close, plus page-visibility recovery. Mobile browsers freeze
// backgrounded tabs and drop the socket; the visibility, pageshow, resume and
// online listeners reconnect the moment the page comes back. The server keys
// sessions by terminalId and adopts/respawns the PTY on reattach, so reopening
// the same URL resumes the session.
export class TerminalConnection {
private readonly target: TerminalConnectionTarget;
private readonly handlers: TerminalConnectionHandlers;
private socket: WebSocket | null = null;
private state: TerminalConnectionState = "connecting";
private generation = 0;
private reconnectAttempt = 0;
private reconnectTimer: ReturnType<typeof setTimeout> | null = null;
private hasReceivedBytes = false;
private everAttached = false;
private terminated = false;
private disposed = false;

constructor(
target: TerminalConnectionTarget,
handlers: TerminalConnectionHandlers,
) {
this.target = target;
this.handlers = handlers;
}

start() {
if (typeof document !== "undefined") {
document.addEventListener("visibilitychange", this.handleResume);
document.addEventListener("resume", this.handleResume);
}
if (typeof window !== "undefined") {
window.addEventListener("pageshow", this.handleResume);
window.addEventListener("online", this.handleResume);
}
void this.connect();
}

dispose() {
this.disposed = true;
this.cancelReconnect();
if (typeof document !== "undefined") {
document.removeEventListener("visibilitychange", this.handleResume);
document.removeEventListener("resume", this.handleResume);
}
if (typeof window !== "undefined") {
window.removeEventListener("pageshow", this.handleResume);
window.removeEventListener("online", this.handleResume);
}
this.teardownSocket();
}

send(message: TerminalClientMessage) {
const socket = this.socket;
if (!socket || socket.readyState !== WebSocket.OPEN) return;
socket.send(JSON.stringify(message));
}

private connect = async () => {
if (this.disposed || this.terminated) return;
this.cancelReconnect();
this.teardownSocket();
const generation = ++this.generation;
this.emitState(this.everAttached ? "reconnecting" : "connecting");

let url: string;
try {
url = await this.buildUrl();
} catch {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: This catch block swallows connection-setup errors and immediately retries, which makes persistent auth/URL failures hard to diagnose in production. Log or otherwise surface the caught error before scheduling reconnect.

(Based on your team's feedback about handling async failures explicitly and avoiding silent catches.)

View Feedback

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/app/workspaces/[workspaceId]/components/WebTerminal/TerminalConnection.ts, line 103:

<comment>This catch block swallows connection-setup errors and immediately retries, which makes persistent auth/URL failures hard to diagnose in production. Log or otherwise surface the caught error before scheduling reconnect.

(Based on your team's feedback about handling async failures explicitly and avoiding silent catches.) </comment>

<file context>
@@ -0,0 +1,237 @@
+		let url: string;
+		try {
+			url = await this.buildUrl();
+		} catch {
+			if (generation !== this.generation || this.disposed) return;
+			this.scheduleReconnect();
</file context>

if (generation !== this.generation || this.disposed) return;
this.scheduleReconnect();
return;
}
if (generation !== this.generation || this.disposed || this.terminated) {
return;
}

let socket: WebSocket;
try {
socket = new WebSocket(url);
} catch {
this.scheduleReconnect();
return;
}
socket.binaryType = "arraybuffer";
this.socket = socket;
this.attachListeners(socket);
};

private async buildUrl(): Promise<string> {
const token = await getAuthToken();
const base = getRelayUrl().replace(/^http/, "ws").replace(/\/$/, "");
const url = new URL(
`${base}/hosts/${this.target.routingKey}/terminal/${encodeURIComponent(
this.target.terminalId,
)}`,
);
url.searchParams.set("workspaceId", this.target.workspaceId);
url.searchParams.set("themeType", "dark");
url.searchParams.set("token", token);
// Once xterm holds scrollback, skip the daemon ring-buffer re-dump on
// reattach; the in-memory buffer still replays output missed offline.
if (this.hasReceivedBytes) url.searchParams.set("replay", "0");
return url.toString();
}

private attachListeners(socket: WebSocket) {
socket.onmessage = (event) => {
if (this.socket !== socket) return;
if (event.data instanceof ArrayBuffer) {
this.hasReceivedBytes = true;
this.handlers.onBinary(new Uint8Array(event.data));
return;
}
let message: TerminalServerMessage;
try {
message = JSON.parse(String(event.data)) as TerminalServerMessage;
} catch {
return;
}
if (message.type === "attached") {
this.reconnectAttempt = 0;
this.everAttached = true;
} else if (message.type === "exit" || message.type === "error") {
this.terminated = true;
this.cancelReconnect();
}
this.handlers.onControl(message);
};

socket.onclose = () => {
if (this.socket !== socket) return;
this.socket = null;
if (this.terminated || this.disposed) return;
this.scheduleReconnect();
};
}

private teardownSocket() {
const socket = this.socket;
this.socket = null;
if (!socket) return;
socket.onmessage = null;
socket.onclose = null;
try {
Comment on lines +177 to +179
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 teardownSocket nulls onmessage and onclose but skips onerror. The onerror handler is never set in this file today, so this is harmless — but socket.close() can synchronously dispatch an error event in some environments before the connection closes, and a future contributor adding an onerror for diagnostics would have a stale callback fire after teardown. Nulling it here keeps the cleanup symmetrical.

Suggested change
socket.onmessage = null;
socket.onclose = null;
try {
socket.onmessage = null;
socket.onclose = null;
socket.onerror = null;
try {
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/src/app/workspaces/[workspaceId]/components/WebTerminal/TerminalConnection.ts
Line: 177-179

Comment:
`teardownSocket` nulls `onmessage` and `onclose` but skips `onerror`. The `onerror` handler is never set in this file today, so this is harmless — but `socket.close()` can synchronously dispatch an error event in some environments before the connection closes, and a future contributor adding an `onerror` for diagnostics would have a stale callback fire after teardown. Nulling it here keeps the cleanup symmetrical.

```suggestion
		socket.onmessage = null;
		socket.onclose = null;
		socket.onerror = null;
		try {
```

How can I resolve this? If you propose a fix, please make it concise.

socket.close();
} catch {
// best-effort
}
}

private scheduleReconnect() {
if (this.reconnectTimer !== null) return;
if (this.terminated || this.disposed) return;
if (this.reconnectAttempt >= MAX_RECONNECT_ATTEMPTS) {
this.emitState("error");
return;
}
this.emitState("reconnecting");
// Frozen tabs don't run timers; the visibility listener reconnects on
// resume instead of burning the attempt budget on a timer that won't fire.
if (typeof document !== "undefined" && document.hidden) return;

const delay = Math.min(
BASE_RECONNECT_DELAY_MS * 2 ** this.reconnectAttempt,
MAX_RECONNECT_DELAY_MS,
);
this.reconnectAttempt += 1;
this.reconnectTimer = setTimeout(() => {
this.reconnectTimer = null;
void this.connect();
}, delay);
}

private cancelReconnect() {
if (this.reconnectTimer !== null) {
clearTimeout(this.reconnectTimer);
this.reconnectTimer = null;
}
}

private handleResume = () => {
if (this.disposed || this.terminated) return;
if (typeof document !== "undefined" && document.hidden) return;
this.reconnectAttempt = 0;
const socket = this.socket;
if (
socket &&
(socket.readyState === WebSocket.OPEN ||
socket.readyState === WebSocket.CONNECTING)
) {
return;
}
this.cancelReconnect();
void this.connect();
};
Comment on lines +216 to +230
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Concurrent resume events trigger parallel getAuthToken() calls

When the device wakes, visibilitychange and online can fire in the same event loop turn. Both calls to handleResume see this.socket === null (the WebSocket isn't created until after the async buildUrl() resolves), so both invoke connect(). The second call increments the generation and invalidates the first, so only one socket is ever opened — but both getAuthToken() fetches run concurrently and the first one's result is silently discarded. A lightweight in-flight flag on handleResume would avoid the wasted work.

Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/src/app/workspaces/[workspaceId]/components/WebTerminal/TerminalConnection.ts
Line: 216-230

Comment:
**Concurrent resume events trigger parallel `getAuthToken()` calls**

When the device wakes, `visibilitychange` and `online` can fire in the same event loop turn. Both calls to `handleResume` see `this.socket === null` (the WebSocket isn't created until after the async `buildUrl()` resolves), so both invoke `connect()`. The second call increments the generation and invalidates the first, so only one socket is ever opened — but both `getAuthToken()` fetches run concurrently and the first one's result is silently discarded. A lightweight in-flight flag on `handleResume` would avoid the wasted work.

How can I resolve this? If you propose a fix, please make it concise.


private emitState(state: TerminalConnectionState) {
if (this.state === state) return;
this.state = state;
this.handlers.onStateChange(state);
}
}
Loading
Loading