Address review comments

* Better error handling for HTTP errors
* Refactor magic numbers
* Use strict types and avoid casting

Author: EhabY

src/api/coderApi.ts

@@ -14,7 +14,7 @@ import {
 	type WorkspaceAgentLog,
 } from "coder/site/src/api/typesGenerated";
 import * as vscode from "vscode";
-import { type ClientOptions, type CloseEvent, type ErrorEvent } from "ws";
+import { type ClientOptions } from "ws";
 
 import { CertificateError } from "../error";
 import { getHeaderCommand, getHeaders } from "../headers";
@@ -31,7 +31,12 @@ import {
 	HttpClientLogLevel,
 } from "../logging/types";
 import { sizeOf } from "../logging/utils";
-import { type UnidirectionalStream } from "../websocket/eventStreamConnection";
+import { HttpStatusCode } from "../websocket/codes";
+import {
+	type UnidirectionalStream,
+	type CloseEvent,
+	type ErrorEvent,
+} from "../websocket/eventStreamConnection";
 import {
 	OneWayWebSocket,
 	type OneWayWebSocketInit,
@@ -336,8 +341,8 @@ export class CoderApi extends Api {
 			const handleError = (event: ErrorEvent) => {
 				cleanup();
 				const is404 =
-					event.message?.includes("404") ||
-					event.error?.message?.includes("404");
+					event.message?.includes(String(HttpStatusCode.NOT_FOUND)) ||
+					event.error?.message?.includes(String(HttpStatusCode.NOT_FOUND));
 
 				if (is404 && onNotFound) {
 					connection.close();

src/websocket/codes.ts

@@ -0,0 +1,55 @@
+/**
+ * WebSocket close codes (RFC 6455) and HTTP status codes for socket connections.
+ * @see https://www.rfc-editor.org/rfc/rfc6455#section-7.4.1
+ */
+
+/** WebSocket close codes defined in RFC 6455 */
+export const WebSocketCloseCode = {
+	/** Normal closure - connection successfully completed */
+	NORMAL: 1000,
+	/** Endpoint going away (server shutdown) */
+	GOING_AWAY: 1001,
+	/** Protocol error - connection cannot be recovered */
+	PROTOCOL_ERROR: 1002,
+	/** Unsupported data type received - connection cannot be recovered */
+	UNSUPPORTED_DATA: 1003,
+	/** Abnormal closure - connection closed without close frame (network issues) */
+	ABNORMAL: 1006,
+} as const;
+
+/** HTTP status codes used for socket creation and connection logic */
+export const HttpStatusCode = {
+	/** Authentication or permission denied */
+	FORBIDDEN: 403,
+	/** Endpoint not found */
+	NOT_FOUND: 404,
+	/** Resource permanently gone */
+	GONE: 410,
+	/** Protocol upgrade required */
+	UPGRADE_REQUIRED: 426,
+} as const;
+
+/**
+ * WebSocket close codes indicating unrecoverable errors.
+ * These appear in close events and should stop reconnection attempts.
+ */
+export const UNRECOVERABLE_WS_CLOSE_CODES = new Set<number>([
+	WebSocketCloseCode.PROTOCOL_ERROR,
+	WebSocketCloseCode.UNSUPPORTED_DATA,
+]);
+
+/**
+ * HTTP status codes indicating unrecoverable errors during handshake.
+ * These appear during socket creation and should stop reconnection attempts.
+ */
+export const UNRECOVERABLE_HTTP_CODES = new Set<number>([
+	HttpStatusCode.FORBIDDEN,
+	HttpStatusCode.GONE,
+	HttpStatusCode.UPGRADE_REQUIRED,
+]);
+
+/** Close codes indicating intentional closure - do not reconnect */
+export const NORMAL_CLOSURE_CODES = new Set<number>([
+	WebSocketCloseCode.NORMAL,
+	WebSocketCloseCode.GOING_AWAY,
+]);

src/websocket/eventStreamConnection.ts

@@ -1,11 +1,16 @@
 import { type WebSocketEventType } from "coder/site/src/utils/OneWayWebSocket";
 import {
-	type CloseEvent,
+	type CloseEvent as WsCloseEvent,
 	type Event as WsEvent,
-	type ErrorEvent,
-	type MessageEvent,
+	type ErrorEvent as WsErrorEvent,
+	type MessageEvent as WsMessageEvent,
 } from "ws";
 
+export type Event = Omit<WsEvent, "type" | "target">;
+export type CloseEvent = Omit<WsCloseEvent, "type" | "target">;
+export type ErrorEvent = Omit<WsErrorEvent, "type" | "target">;
+export type MessageEvent = Omit<WsMessageEvent, "type" | "target">;
+
 // Event payload types matching OneWayWebSocket
 export type ParsedMessageEvent<TData> = Readonly<
 	| {
@@ -24,7 +29,7 @@ export type EventPayloadMap<TData> = {
 	close: CloseEvent;
 	error: ErrorEvent;
 	message: ParsedMessageEvent<TData>;
-	open: WsEvent;
+	open: Event;
 };
 
 export type EventHandler<TData, TEvent extends WebSocketEventType> = (

src/websocket/reconnectingWebSocket.ts

@@ -1,5 +1,11 @@
+import {
+	WebSocketCloseCode,
+	NORMAL_CLOSURE_CODES,
+	UNRECOVERABLE_WS_CLOSE_CODES,
+	UNRECOVERABLE_HTTP_CODES,
+} from "./codes";
+
 import type { WebSocketEventType } from "coder/site/src/utils/OneWayWebSocket";
-import type { CloseEvent } from "ws";
 
 import type { Logger } from "../logging/logger";
 
@@ -16,17 +22,16 @@ export type ReconnectingWebSocketOptions = {
 	jitterFactor?: number;
 };
 
-// 403 Forbidden, 410 Gone, 426 Upgrade Required, 1002/1003 Protocol errors
-const UNRECOVERABLE_CLOSE_CODES = new Set([403, 410, 426, 1002, 1003]);
-
 export class ReconnectingWebSocket<TData = unknown>
 	implements UnidirectionalStream<TData>
 {
 	readonly #socketFactory: SocketFactory<TData>;
 	readonly #logger: Logger;
 	readonly #apiRoute: string;
 	readonly #options: Required<ReconnectingWebSocketOptions>;
-	readonly #eventHandlers = {
+	readonly #eventHandlers: {
+		[K in WebSocketEventType]: Set<EventHandler<TData, K>>;
+	} = {
 		open: new Set<EventHandler<TData, "open">>(),
 		close: new Set<EventHandler<TData, "close">>(),
 		error: new Set<EventHandler<TData, "error">>(),
@@ -86,18 +91,14 @@ export class ReconnectingWebSocket<TData = unknown>
 		event: TEvent,
 		callback: EventHandler<TData, TEvent>,
 	): void {
-		(this.#eventHandlers[event] as Set<EventHandler<TData, TEvent>>).add(
-			callback,
-		);
+		this.#eventHandlers[event].add(callback);
 	}
 
 	removeEventListener<TEvent extends WebSocketEventType>(
 		event: TEvent,
 		callback: EventHandler<TData, TEvent>,
 	): void {
-		(this.#eventHandlers[event] as Set<EventHandler<TData, TEvent>>).delete(
-			callback,
-		);
+		this.#eventHandlers[event].delete(callback);
 	}
 
 	reconnect(): void {
@@ -117,14 +118,7 @@ export class ReconnectingWebSocket<TData = unknown>
 		}
 
 		// connect() will close any existing socket
-		this.connect().catch((error) => {
-			if (!this.#isDisposed) {
-				this.#logger.warn(
-					`Manual reconnection failed for ${this.#apiRoute}: ${error instanceof Error ? error.message : String(error)}`,
-				);
-				this.scheduleReconnect();
-			}
-		});
+		this.connect().catch((error) => this.handleConnectionError(error));
 	}
 
 	close(code?: number, reason?: string): void {
@@ -135,12 +129,10 @@ export class ReconnectingWebSocket<TData = unknown>
 		// Fire close handlers synchronously before disposing
 		if (this.#currentSocket) {
 			this.executeHandlers("close", {
-				code: code ?? 1000,
-				reason: reason ?? "",
+				code: code ?? WebSocketCloseCode.NORMAL,
+				reason: reason ?? "Normal closure",
 				wasClean: true,
-				type: "close",
-				target: this.#currentSocket,
-			} as CloseEvent);
+			});
 		}
 
 		this.dispose(code, reason);
@@ -155,7 +147,10 @@ export class ReconnectingWebSocket<TData = unknown>
 		try {
 			// Close any existing socket before creating a new one
 			if (this.#currentSocket) {
-				this.#currentSocket.close(1000, "Replacing connection");
+				this.#currentSocket.close(
+					WebSocketCloseCode.NORMAL,
+					"Replacing connection",
+				);
 				this.#currentSocket = null;
 			}
 
@@ -182,7 +177,7 @@ export class ReconnectingWebSocket<TData = unknown>
 
 				this.executeHandlers("close", event);
 
-				if (UNRECOVERABLE_CLOSE_CODES.has(event.code)) {
+				if (UNRECOVERABLE_WS_CLOSE_CODES.has(event.code)) {
 					this.#logger.error(
 						`WebSocket connection closed with unrecoverable error code ${event.code}`,
 					);
@@ -191,7 +186,7 @@ export class ReconnectingWebSocket<TData = unknown>
 				}
 
 				// Don't reconnect on normal closure
-				if (event.code === 1000 || event.code === 1001) {
+				if (NORMAL_CLOSURE_CODES.has(event.code)) {
 					return;
 				}
 
@@ -223,14 +218,7 @@ export class ReconnectingWebSocket<TData = unknown>
 
 		this.#reconnectTimeoutId = setTimeout(() => {
 			this.#reconnectTimeoutId = null;
-			this.connect().catch((error) => {
-				if (!this.#isDisposed) {
-					this.#logger.warn(
-						`WebSocket connection failed for ${this.#apiRoute}: ${error instanceof Error ? error.message : String(error)}`,
-					);
-					this.scheduleReconnect();
-				}
-			});
+			this.connect().catch((error) => this.handleConnectionError(error));
 		}, delayMs);
 
 		this.#backoffMs = Math.min(this.#backoffMs * 2, this.#options.maxBackoffMs);
@@ -240,20 +228,56 @@ export class ReconnectingWebSocket<TData = unknown>
 		event: TEvent,
 		eventData: Parameters<EventHandler<TData, TEvent>>[0],
 	): void {
-		const handlers = this.#eventHandlers[event] as Set<
-			EventHandler<TData, TEvent>
-		>;
-		for (const handler of handlers) {
+		for (const handler of this.#eventHandlers[event]) {
 			try {
 				handler(eventData);
 			} catch (error) {
 				this.#logger.error(
-					`Error in ${event} handler for ${this.#apiRoute}: ${error instanceof Error ? error.message : String(error)}`,
+					`Error in ${event} handler for ${this.#apiRoute}`,
+					error,
 				);
 			}
 		}
 	}
 
+	/**
+	 * Checks if the error is unrecoverable and disposes the connection,
+	 * otherwise schedules a reconnect.
+	 */
+	private handleConnectionError(error: unknown): void {
+		if (this.#isDisposed) {
+			return;
+		}
+
+		if (this.isUnrecoverableHttpError(error)) {
+			this.#logger.error(
+				`Unrecoverable HTTP error during connection for ${this.#apiRoute}`,
+				error,
+			);
+			this.dispose();
+			return;
+		}
+
+		this.#logger.warn(
+			`WebSocket connection failed for ${this.#apiRoute}`,
+			error,
+		);
+		this.scheduleReconnect();
+	}
+
+	/**
+	 * Check if an error contains an unrecoverable HTTP status code.
+	 */
+	private isUnrecoverableHttpError(error: unknown): boolean {
+		const errorMessage = error instanceof Error ? error.message : String(error);
+		for (const code of UNRECOVERABLE_HTTP_CODES) {
+			if (errorMessage.includes(String(code))) {
+				return true;
+			}
+		}
+		return false;
+	}
+
 	private dispose(code?: number, reason?: string): void {
 		if (this.#isDisposed) {
 			return;

src/websocket/sseConnection.ts

@@ -6,19 +6,14 @@ import { EventSource } from "eventsource";
 import { createStreamingFetchAdapter } from "../api/streamingFetchAdapter";
 import { type Logger } from "../logging/logger";
 
+import { WebSocketCloseCode } from "./codes";
 import { getQueryString } from "./utils";
 
-import type {
-	CloseEvent as WsCloseEvent,
-	ErrorEvent as WsErrorEvent,
-	Event as WsEvent,
-	MessageEvent as WsMessageEvent,
-} from "ws";
-
 import type {
 	UnidirectionalStream,
 	ParsedMessageEvent,
 	EventHandler,
+	ErrorEvent as WsErrorEvent,
 } from "./eventStreamConnection";
 
 export type SseConnectionInit = {
@@ -66,7 +61,7 @@ export class SseConnection implements UnidirectionalStream<ServerSentEvent> {
 
 	private setupEventHandlers(): void {
 		this.eventSource.addEventListener("open", () =>
-			this.invokeCallbacks(this.callbacks.open, {} as WsEvent, "open"),
+			this.invokeCallbacks(this.callbacks.open, {}, "open"),
 		);
 
 		this.eventSource.addEventListener("data", (event: MessageEvent) => {
@@ -84,10 +79,10 @@ export class SseConnection implements UnidirectionalStream<ServerSentEvent> {
 				this.invokeCallbacks(
 					this.callbacks.close,
 					{
-						code: 1006,
+						code: WebSocketCloseCode.ABNORMAL,
 						reason: "Connection lost",
 						wasClean: false,
-					} as WsCloseEvent,
+					},
 					"close",
 				);
 			}
@@ -117,7 +112,7 @@ export class SseConnection implements UnidirectionalStream<ServerSentEvent> {
 		return {
 			error: error,
 			message: errorMessage,
-		} as WsErrorEvent;
+		};
 	}
 
 	public addEventListener<TEvent extends WebSocketEventType>(
@@ -158,7 +153,7 @@ export class SseConnection implements UnidirectionalStream<ServerSentEvent> {
 	private parseMessage(
 		event: MessageEvent,
 	): ParsedMessageEvent<ServerSentEvent> {
-		const wsEvent = { data: event.data } as WsMessageEvent;
+		const wsEvent = { data: event.data };
 		try {
 			return {
 				sourceEvent: wsEvent,
@@ -207,14 +202,16 @@ export class SseConnection implements UnidirectionalStream<ServerSentEvent> {
 		this.invokeCallbacks(
 			this.callbacks.close,
 			{
-				code: code ?? 1000,
+				code: code ?? WebSocketCloseCode.NORMAL,
 				reason: reason ?? "Normal closure",
 				wasClean: true,
-			} as WsCloseEvent,
+			},
 			"close",
 		);
 
-		Object.values(this.callbacks).forEach((callbackSet) => callbackSet.clear());
+		for (const callbackSet of Object.values(this.callbacks)) {
+			callbackSet.clear();
+		}
 		this.messageWrappers.clear();
 	}
 }