Skip to content

Commit a29a137

Browse files
EhabYEhabY
committed
Review comments
1 parent e32ba50 commit a29a137

File tree

8 files changed

+90
-67
lines changed

8 files changed

+90
-67
lines changed

‎src/agentMetadataHelper.ts

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -36,10 +36,20 @@ export function createAgentMetadataWatcher(
3636
},
3737
};
3838

39+
const handleError = (error: unknown) => {
40+
watcher.error = error;
41+
onChange.fire(null);
42+
};
43+
3944
socket.addEventListener("message", (event) => {
4045
try {
46+
if (event.parseError) {
47+
handleError(event.parseError);
48+
return;
49+
}
50+
4151
const metadata = AgentMetadataEventSchemaArray.parse(
42-
event.parsedMessage!.data,
52+
event.parsedMessage.data,
4353
);
4454

4555
// Overwrite metadata if it changed.
@@ -48,15 +58,11 @@ export function createAgentMetadataWatcher(
4858
onChange.fire(null);
4959
}
5060
} catch (error) {
51-
watcher.error = error;
52-
onChange.fire(null);
61+
handleError(error);
5362
}
5463
});
5564

56-
socket.addEventListener("error", (error) => {
57-
watcher.error = error;
58-
onChange.fire(null);
59-
});
65+
socket.addEventListener("error", handleError);
6066

6167
return watcher;
6268
}

‎src/api.ts

Lines changed: 26 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { AxiosInstance } from "axios";
22
import { spawn } from "child_process";
33
import { Api } from "coder/site/src/api/api";
44
import { Workspace } from "coder/site/src/api/typesGenerated";
5-
import fs from "fs/promises";
5+
import fs from "fs";
66
import { ProxyAgent } from "proxy-agent";
77
import * as vscode from "vscode";
88
import { errToStr } from "./api-helper";
@@ -40,7 +40,7 @@ export function needToken(): boolean {
4040
/**
4141
* Create a new agent based off the current settings.
4242
*/
43-
export async function createHttpAgent(): Promise<ProxyAgent> {
43+
export function createHttpAgent(): ProxyAgent {
4444
const cfg = vscode.workspace.getConfiguration();
4545
const insecure = Boolean(cfg.get("coder.insecure"));
4646
const certFile = expandPath(
@@ -60,9 +60,9 @@ export async function createHttpAgent(): Promise<ProxyAgent> {
6060
cfg.get("coder.proxyBypass"),
6161
);
6262
},
63-
cert: certFile === "" ? undefined : await fs.readFile(certFile),
64-
key: keyFile === "" ? undefined : await fs.readFile(keyFile),
65-
ca: caFile === "" ? undefined : await fs.readFile(caFile),
63+
cert: certFile === "" ? undefined : fs.readFileSync(certFile),
64+
key: keyFile === "" ? undefined : fs.readFileSync(keyFile),
65+
ca: caFile === "" ? undefined : fs.readFileSync(caFile),
6666
servername: altHost === "" ? undefined : altHost,
6767
// rejectUnauthorized defaults to true, so we need to explicitly set it to
6868
// false if we want to allow self-signed certificates.
@@ -90,7 +90,6 @@ export function makeCoderSdk(
9090
restClient.setSessionToken(token);
9191
}
9292

93-
// Logging interceptor
9493
addLoggingInterceptors(restClient.getAxiosInstance(), storage.output);
9594

9695
restClient.getAxiosInstance().interceptors.request.use(async (config) => {
@@ -104,7 +103,7 @@ export function makeCoderSdk(
104103
// Configure proxy and TLS.
105104
// Note that by default VS Code overrides the agent. To prevent this, set
106105
// `http.proxySupport` to `on` or `off`.
107-
const agent = await createHttpAgent();
106+
const agent = createHttpAgent();
108107
config.httpsAgent = agent;
109108
config.httpAgent = agent;
110109
config.proxy = false;
@@ -242,33 +241,39 @@ export async function waitForBuild(
242241
logs.forEach((log) => writeEmitter.fire(log.output + "\r\n"));
243242

244243
await new Promise<void>((resolve, reject) => {
245-
const rejectError = (error: unknown) => {
246-
const baseUrlRaw = restClient.getAxiosInstance().defaults.baseURL!;
247-
return reject(
248-
new Error(
249-
`Failed to watch workspace build on ${baseUrlRaw}: ${errToStr(error, "no further details")}`,
250-
),
251-
);
252-
};
253-
254244
const socket = webSocketClient.watchBuildLogsByBuildId(
255245
workspace.latest_build.id,
256246
logs,
257247
);
248+
258249
const closeHandler = () => {
259250
resolve();
260251
};
261-
socket.addEventListener("close", closeHandler);
252+
262253
socket.addEventListener("message", (data) => {
263-
const log = data.parsedMessage!;
264-
writeEmitter.fire(log.output + "\r\n");
254+
if (data.parseError) {
255+
writeEmitter.fire(
256+
errToStr(data.parseError, "Failed to parse message") + "\r\n",
257+
);
258+
} else {
259+
writeEmitter.fire(data.parsedMessage.output + "\r\n");
260+
}
265261
});
262+
266263
socket.addEventListener("error", (error) => {
267-
// Do not want to trigger the close handler.
264+
// Do not want to trigger the close handler and resolve the promise normally.
268265
socket.removeEventListener("close", closeHandler);
269266
socket.close();
270-
rejectError(error);
267+
268+
const baseUrlRaw = restClient.getAxiosInstance().defaults.baseURL;
269+
return reject(
270+
new Error(
271+
`Failed to watch workspace build on ${baseUrlRaw}: ${errToStr(error, "no further details")}`,
272+
),
273+
);
271274
});
275+
276+
socket.addEventListener("close", closeHandler);
272277
});
273278

274279
writeEmitter.fire("Build complete\r\n");

‎src/extension.ts

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import axios, { isAxiosError } from "axios";
33
import { getErrorMessage } from "coder/site/src/api/errors";
44
import * as module from "module";
55
import * as vscode from "vscode";
6-
import { createHttpAgent, makeCoderSdk, needToken } from "./api";
6+
import { makeCoderSdk, needToken } from "./api";
77
import { errToStr } from "./api-helper";
88
import { Commands } from "./commands";
99
import { CertificateError, getErrorDetail } from "./error";
@@ -68,13 +68,7 @@ export async function activate(ctx: vscode.ExtensionContext): Promise<void> {
6868
storage,
6969
);
7070

71-
// TODO this won't get updated when users change their settings; Listen to changes and update this
72-
const httpAgent = await createHttpAgent();
73-
const webSocketClient = new CoderWebSocketClient(
74-
restClient,
75-
httpAgent,
76-
storage,
77-
);
71+
const webSocketClient = new CoderWebSocketClient(restClient, storage);
7872
const myWorkspacesProvider = new WorkspaceProvider(
7973
WorkspaceQuery.Mine,
8074
restClient,

‎src/inbox.ts

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,8 +47,16 @@ export class Inbox implements vscode.Disposable {
4747
});
4848

4949
this.#socket.addEventListener("message", (data) => {
50-
const inboxMessage = data.parsedMessage!;
51-
vscode.window.showInformationMessage(inboxMessage.notification.title);
50+
if (data.parseError) {
51+
this.#storage.output.error(
52+
"Failed to parse inbox message",
53+
data.parseError,
54+
);
55+
} else {
56+
vscode.window.showInformationMessage(
57+
data.parsedMessage.notification.title,
58+
);
59+
}
5260
});
5361
}
5462

‎src/logging/netLog.ts

Lines changed: 29 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,12 @@ import {
44
type AxiosError,
55
isAxiosError,
66
} from "axios";
7+
import { getErrorMessage } from "coder/site/src/api/errors";
78
import { Buffer } from "node:buffer";
89
import crypto from "node:crypto";
910
import type * as vscode from "vscode";
1011
import { errToStr } from "../api-helper";
12+
import { getErrorDetail } from "../error";
1113

1214
export interface RequestMeta {
1315
requestId: string;
@@ -58,10 +60,9 @@ export function logRequestStart(
5860
config: InternalAxiosRequestConfig,
5961
): void {
6062
const method = (config.method ?? "GET").toUpperCase();
61-
const url = config.url || "";
62-
const len = config.headers?.["content-length"] as string | undefined;
63-
const lenStr = len ? ` (${len}b)` : "";
64-
logger.trace(`→ ${shortId(requestId)} ${method} ${url}${lenStr}`);
63+
const url = extractUri(config);
64+
const len = extractContentLength(config.headers);
65+
logger.trace(`→ ${shortId(requestId)} ${method} ${url} ${len}`);
6566
}
6667

6768
export function logRequestSuccess(
@@ -70,15 +71,23 @@ export function logRequestSuccess(
7071
response: AxiosResponse,
7172
): void {
7273
const method = (response.config.method ?? "GET").toUpperCase();
73-
const url = response.config.url || "";
74-
const len = response.headers?.["content-length"] as string | undefined;
74+
const url = extractUri(response.config);
7575
const ms = Date.now() - meta.startedAt;
76-
const lenStr = len ? ` (${len}b)` : "";
76+
const len = extractContentLength(response.headers);
7777
logger.trace(
78-
`← ${shortId(meta.requestId)} ${response.status} ${method} ${url} ${ms}ms${lenStr}`,
78+
`← ${shortId(meta.requestId)} ${response.status} ${method} ${url} ${ms}ms${len}`,
7979
);
8080
}
8181

82+
function extractUri(config: InternalAxiosRequestConfig | undefined): string {
83+
return config?.url || "<no url>";
84+
}
85+
86+
function extractContentLength(headers: Record<string, unknown>): string {
87+
const len = headers["content-length"];
88+
return len && typeof len === "string" ? `(${len}b)` : "<unknown size>";
89+
}
90+
8291
export function logRequestError(
8392
logger: vscode.LogOutputChannel,
8493
error: AxiosError | unknown,
@@ -87,23 +96,27 @@ export function logRequestError(
8796
const config = error.config as RequestConfigWithMeta | undefined;
8897
const meta = config?.metadata;
8998
const method = (config?.method ?? "GET").toUpperCase();
90-
const url = config?.url || "";
91-
const requestId = meta?.requestId ?? "unknown";
99+
const url = extractUri(config);
100+
const requestId = meta?.requestId || "unknown";
92101
const ms = meta ? Date.now() - meta.startedAt : "?";
93102

103+
const msg = getErrorMessage(error, "No error message");
104+
const detail = getErrorDetail(error) ?? "";
94105
if (error.response) {
95106
// Response error (4xx, 5xx status codes)
96-
const msg =
107+
const responseData =
97108
error.response.statusText || String(error.response.data).slice(0, 100);
109+
const errorInfo = [msg, detail, responseData].filter(Boolean).join(" - ");
98110
logger.error(
99-
`← ${shortId(requestId)} ${error.response.status} ${method} ${url} ${ms}ms - ${msg}`,
111+
`← ${shortId(requestId)} ${error.response.status} ${method} ${url} ${ms}ms - ${errorInfo}`,
100112
error,
101113
);
102114
} else {
103115
// Request error (network, timeout, etc)
104116
const reason = error.code || error.message || "Network error";
117+
const errorInfo = [msg, detail, reason].filter(Boolean).join(" - ");
105118
logger.error(
106-
`✗ ${shortId(requestId)} ${method} ${url} ${ms}ms - ${reason}`,
119+
`✗ ${shortId(requestId)} ${method} ${url} ${ms}ms - ${errorInfo}`,
107120
error,
108121
);
109122
}
@@ -161,13 +174,13 @@ export class WsLogger {
161174
const statsStr = stats.length > 0 ? ` [${stats.join(", ")}]` : "";
162175

163176
this.logger.trace(
164-
`✗ WS ${shortId(this.id)} closed${codeStr}${reasonStr}${statsStr}`,
177+
`▣ WS ${shortId(this.id)} closed${codeStr}${reasonStr}${statsStr}`,
165178
);
166179
}
167180

168-
logError(error: unknown): void {
181+
logError(error: unknown, message: string): void {
169182
const ms = Date.now() - this.startedAt;
170-
const errorMsg = errToStr(error, "connection error");
183+
const errorMsg = message || errToStr(error, "connection error");
171184
this.logger.error(
172185
`✗ WS ${shortId(this.id)} error ${ms}ms - ${errorMsg}`,
173186
error,

‎src/remote.ts

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ import {
1616
formatMetadataError,
1717
} from "./agentMetadataHelper";
1818
import {
19-
createHttpAgent,
2019
makeCoderSdk,
2120
needToken,
2221
startWorkspaceIfStoppedOrFailed,
@@ -126,10 +125,8 @@ export class Remote {
126125
case "stopping": {
127126
writeEmitter = initWriteEmitterAndTerminal();
128127
this.storage.output.info(`Waiting for ${workspaceName}...`);
129-
const httpAgent = await createHttpAgent();
130128
const webSocketClient = new CoderWebSocketClient(
131129
restClient,
132-
httpAgent,
133130
this.storage,
134131
);
135132
workspace = await waitForBuild(
@@ -500,10 +497,8 @@ export class Remote {
500497
}
501498
}
502499

503-
const httpAgent = await createHttpAgent();
504500
const webSocketClient = new CoderWebSocketClient(
505501
workspaceRestClient,
506-
httpAgent,
507502
this.storage,
508503
);
509504

‎src/websocket/webSocketClient.ts

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,8 @@ import {
66
Workspace,
77
WorkspaceAgent,
88
} from "coder/site/src/api/typesGenerated";
9-
import { ProxyAgent } from "proxy-agent";
109
import { ClientOptions } from "ws";
11-
import { coderSessionTokenHeader } from "../api";
10+
import { coderSessionTokenHeader, createHttpAgent } from "../api";
1211
import { WsLogger } from "../logging/netLog";
1312
import { Storage } from "../storage";
1413
import { OneWayCodeWebSocket } from "./oneWayCodeWebSocket";
@@ -17,14 +16,12 @@ import { OneWayCodeWebSocket } from "./oneWayCodeWebSocket";
1716
* WebSocket client for Coder API connections.
1817
*
1918
* Automatically configures logging for WebSocket events:
20-
* - Connection attempts and successful opens at the trace level
21-
* - Connection closes at the trace level
19+
* - Connection attempts, successful opens and closes at the trace level
2220
* - Errors at the error level
2321
*/
2422
export class CoderWebSocketClient {
2523
constructor(
2624
private readonly client: Api,
27-
private readonly httpAgent: ProxyAgent,
2825
private readonly storage: Storage,
2926
) {}
3027

@@ -91,11 +88,12 @@ export class CoderWebSocketClient {
9188
coderSessionTokenHeader
9289
] as string | undefined;
9390

91+
const httpAgent = createHttpAgent();
9492
const webSocket = new OneWayCodeWebSocket<TData>({
9593
location: baseUrl,
9694
...configs,
9795
options: {
98-
agent: this.httpAgent,
96+
agent: httpAgent,
9997
followRedirects: true,
10098
headers: {
10199
...(token ? { [coderSessionTokenHeader]: token } : {}),
@@ -123,7 +121,7 @@ export class CoderWebSocketClient {
123121
});
124122

125123
webSocket.addEventListener("error", (event) => {
126-
wsLogger.logError(event?.error ?? event);
124+
wsLogger.logError(event.error, event.message);
127125
});
128126

129127
return webSocket;

‎src/workspaceMonitor.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,8 +49,12 @@ export class WorkspaceMonitor implements vscode.Disposable {
4949

5050
socket.addEventListener("message", (event) => {
5151
try {
52+
if (event.parseError) {
53+
this.notifyError(event.parseError);
54+
return;
55+
}
5256
// Perhaps we need to parse this and validate it.
53-
const newWorkspaceData = event.parsedMessage!.data as Workspace;
57+
const newWorkspaceData = event.parsedMessage.data as Workspace;
5458
this.update(newWorkspaceData);
5559
this.maybeNotify(newWorkspaceData);
5660
this.onChange.fire(newWorkspaceData);

0 commit comments

Comments
 (0)