From d6e00c3dd4df9bdccfbad27289fac3d7d3b6de9b Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Fri, 23 May 2025 20:03:57 +0000 Subject: [PATCH 01/27] wip: commit progress on test update --- .../modules/resources/useAgentLogs.test.ts | 117 ++++++++----- site/src/modules/resources/useAgentLogs.ts | 90 ++++++---- site/src/testHelpers/websockets.ts | 152 +++++++++++++++++ site/src/utils/OneWayWebSocket.test.ts | 154 ++---------------- 4 files changed, 289 insertions(+), 224 deletions(-) create mode 100644 site/src/testHelpers/websockets.ts diff --git a/site/src/modules/resources/useAgentLogs.test.ts b/site/src/modules/resources/useAgentLogs.test.ts index a5339e00c87eb..72168ff6f16f1 100644 --- a/site/src/modules/resources/useAgentLogs.test.ts +++ b/site/src/modules/resources/useAgentLogs.test.ts @@ -1,60 +1,89 @@ -import { renderHook, waitFor } from "@testing-library/react"; +import { + renderHook, + type RenderHookResult, + waitFor, +} from "@testing-library/react"; import type { WorkspaceAgentLog } from "api/typesGenerated"; -import WS from "jest-websocket-mock"; import { MockWorkspaceAgent } from "testHelpers/entities"; -import { useAgentLogs } from "./useAgentLogs"; +import { createUseAgentLogs } from "./useAgentLogs"; +import { + createMockWebSocket, + type MockWebSocketPublisher, +} from "testHelpers/websockets"; +import { OneWayWebSocket } from "utils/OneWayWebSocket"; -/** - * TODO: WS does not support multiple tests running at once in isolation so we - * have one single test that test the most common scenario. - * Issue: https://github.com/romgain/jest-websocket-mock/issues/172 - */ +function generateMockLogs(count: number): WorkspaceAgentLog[] { + return Array.from({ length: count }, (_, i) => ({ + id: i, + created_at: new Date().toISOString(), + level: "info", + output: `Log ${i}`, + source_id: "", + })); +} -describe("useAgentLogs", () => { - afterEach(() => { - WS.clean(); +type MountHookResult = Readonly< + RenderHookResult & { + publisher: MockWebSocketPublisher; + } +>; + +function mountHook(): MountHookResult { + let publisher!: MockWebSocketPublisher; + const useAgentLogs = createUseAgentLogs((agentId, params) => { + return new OneWayWebSocket({ + apiRoute: `/api/v2/workspaceagents/${agentId}/logs`, + searchParams: new URLSearchParams({ + follow: "true", + after: params?.after?.toString() || "0", + }), + websocketInit: (url) => { + const [mockSocket, mockPub] = createMockWebSocket(url); + publisher = mockPub; + return mockSocket; + }, + }); }); - it("clear logs when disabled to avoid duplicates", async () => { - const server = new WS( - `ws://localhost/api/v2/workspaceagents/${ - MockWorkspaceAgent.id - }/logs?follow&after=0`, - ); - const { result, rerender } = renderHook( - ({ enabled }) => useAgentLogs(MockWorkspaceAgent, enabled), - { initialProps: { enabled: true } }, - ); - await server.connected; - - // Send 3 logs - server.send(JSON.stringify(generateLogs(3))); + const { result, rerender, unmount } = renderHook( + ({ enabled }) => useAgentLogs(MockWorkspaceAgent, enabled), + { initialProps: { enabled: true } }, + ); + + return { result, rerender, unmount, publisher }; +} + +describe("useAgentLogs", () => { + it("clears logs when hook becomes disabled (protection to avoid duplicate logs when hook goes back to being re-enabled)", async () => { + const { result, rerender, publisher } = mountHook(); + + // Verify that logs can be received after mount + const initialLogs = generateMockLogs(3); + const initialEvent = new MessageEvent("message", { + data: JSON.stringify(initialLogs), + }); + publisher.publishMessage(initialEvent); await waitFor(() => { - expect(result.current).toHaveLength(3); + // Using expect.arrayContaining to account for the fact that we're + // not guaranteed to receive WebSocket events in order + expect(result.current).toEqual(expect.arrayContaining(initialLogs)); }); - // Disable the hook + // Disable the hook (and have the hook close the connection behind the + // scenes) rerender({ enabled: false }); - await waitFor(() => { - expect(result.current).toHaveLength(0); - }); + await waitFor(() => expect(result.current).toHaveLength(0)); - // Enable the hook again + // Re-enable the hook (creating an entirely new connection), and send + // new logs rerender({ enabled: true }); - await server.connected; - server.send(JSON.stringify(generateLogs(3))); + const newLogs = generateMockLogs(3); + const newEvent = new MessageEvent("message", { + data: JSON.stringify(newLogs), + }); + publisher.publishMessage(newEvent); await waitFor(() => { - expect(result.current).toHaveLength(3); + expect(result.current).toEqual(expect.arrayContaining(newLogs)); }); }); }); - -function generateLogs(count: number): WorkspaceAgentLog[] { - return Array.from({ length: count }, (_, i) => ({ - id: i, - created_at: new Date().toISOString(), - level: "info", - output: `Log ${i}`, - source_id: "", - })); -} diff --git a/site/src/modules/resources/useAgentLogs.ts b/site/src/modules/resources/useAgentLogs.ts index d7f810483a693..63ea4afbb99c1 100644 --- a/site/src/modules/resources/useAgentLogs.ts +++ b/site/src/modules/resources/useAgentLogs.ts @@ -3,45 +3,63 @@ import type { WorkspaceAgent, WorkspaceAgentLog } from "api/typesGenerated"; import { displayError } from "components/GlobalSnackbar/utils"; import { useEffect, useState } from "react"; -export function useAgentLogs( - agent: WorkspaceAgent, - enabled: boolean, -): readonly WorkspaceAgentLog[] { - const [logs, setLogs] = useState([]); - - useEffect(() => { - if (!enabled) { - // Clean up the logs when the agent is not enabled. So it can receive logs - // from the beginning without duplicating the logs. +export function createUseAgentLogs( + createSocket: typeof watchWorkspaceAgentLogs, +) { + return function useAgentLogs( + agent: WorkspaceAgent, + enabled: boolean, + ): readonly WorkspaceAgentLog[] { + const [logs, setLogs] = useState([]); + + // Clean up the logs when the agent is not enabled, using a mid-render + // sync to remove any risk of screen flickering. Clearing the logs helps + // ensure that if the hook flips back to being enabled, we can receive a + // fresh set of logs from the beginning with zero risk of duplicates. + const [prevEnabled, setPrevEnabled] = useState(enabled); + if (!enabled && prevEnabled) { setLogs([]); - return; + setPrevEnabled(false); } - // Always fetch the logs from the beginning. We may want to optimize this in - // the future, but it would add some complexity in the code that maybe does - // not worth it. - const socket = watchWorkspaceAgentLogs(agent.id, { after: 0 }); - socket.addEventListener("message", (e) => { - if (e.parseError) { - console.warn("Error parsing agent log: ", e.parseError); + useEffect(() => { + if (!enabled) { return; } - setLogs((logs) => [...logs, ...e.parsedMessage]); - }); - - socket.addEventListener("error", (e) => { - console.error("Error in agent log socket: ", e); - displayError( - "Unable to watch the agent logs", - "Please try refreshing the browser", - ); - socket.close(); - }); - - return () => { - socket.close(); - }; - }, [agent.id, enabled]); - - return logs; + + // Always fetch the logs from the beginning. We may want to optimize + // this in the future, but it would add some complexity in the code + // that might not be worth it. + const socket = createSocket(agent.id, { after: 0 }); + socket.addEventListener("message", (e) => { + if (e.parseError) { + console.warn("Error parsing agent log: ", e.parseError); + return; + } + setLogs((logs) => [...logs, ...e.parsedMessage]); + }); + + socket.addEventListener("error", (e) => { + console.error("Error in agent log socket: ", e); + displayError( + "Unable to watch the agent logs", + "Please try refreshing the browser", + ); + socket.close(); + }); + + return () => socket.close(); + + // createSocket shouldn't ever change for the lifecycle of the hook, + // but Biome isn't smart enough to detect constant dependencies for + // higher-order hooks. Adding it to the array (even though it + // shouldn't ever be needed) seemed like the least fragile way to + // resolve the warning. + }, [createSocket, agent.id, enabled]); + + return logs; + }; } + +// The baseline implementation to use for production +export const useAgentLogs = createUseAgentLogs(watchWorkspaceAgentLogs); diff --git a/site/src/testHelpers/websockets.ts b/site/src/testHelpers/websockets.ts new file mode 100644 index 0000000000000..3d98f543d0684 --- /dev/null +++ b/site/src/testHelpers/websockets.ts @@ -0,0 +1,152 @@ +import type { WebSocketEventType } from "utils/OneWayWebSocket"; + +export type MockWebSocketPublisher = Readonly<{ + publishMessage: (event: MessageEvent) => void; + publishError: (event: ErrorEvent) => void; + publishClose: (event: CloseEvent) => void; + publishOpen: (event: Event) => void; +}>; + +export type CreateMockWebSocketOptions = Readonly<{ + // The URL to use to initialize the mock socket. This should match the + // "real" URL that you would pass to the built-in WebSocket constructor. + url: string; + + // The additional WebSocket protocols to use when initializing. This should + // match the real protocols that you would pass to the built-in WebSocket + // constructor. + protocols?: string | string[]; + + // Indicates whether the mock socket should stay open after calling the + // .close method, so that it can be reused for a new connection. Defaults to + // false (meaning that the socket becomes completely unusable the first time + // after .close is called). + persistAfterClose?: boolean; +}>; + +export function createMockWebSocket( + url: string, + protocols?: string | string[], +): readonly [WebSocket, MockWebSocketPublisher] { + type EventMap = { + message: MessageEvent; + error: ErrorEvent; + close: CloseEvent; + open: Event; + }; + type CallbackStore = { + [K in keyof EventMap]: ((event: EventMap[K]) => void)[]; + }; + + let activeProtocol: string; + if (Array.isArray(protocols)) { + activeProtocol = protocols[0] ?? ""; + } else if (typeof protocols === "string") { + activeProtocol = protocols; + } else { + activeProtocol = ""; + } + + let closed = false; + const store: CallbackStore = { + message: [], + error: [], + close: [], + open: [], + }; + + const mockSocket: WebSocket = { + CONNECTING: 0, + OPEN: 1, + CLOSING: 2, + CLOSED: 3, + + url, + protocol: activeProtocol, + readyState: 1, + binaryType: "blob", + bufferedAmount: 0, + extensions: "", + onclose: null, + onerror: null, + onmessage: null, + onopen: null, + send: jest.fn(), + dispatchEvent: jest.fn(), + + addEventListener: ( + eventType: E, + callback: WebSocketEventMap[E], + ) => { + if (closed) { + return; + } + + const subscribers = store[eventType]; + const cb = callback as unknown as CallbackStore[E][0]; + if (!subscribers.includes(cb)) { + subscribers.push(cb); + } + }, + + removeEventListener: ( + eventType: E, + callback: WebSocketEventMap[E], + ) => { + if (closed) { + return; + } + + const subscribers = store[eventType]; + const cb = callback as unknown as CallbackStore[E][0]; + if (subscribers.includes(cb)) { + const updated = store[eventType].filter((c) => c !== cb); + store[eventType] = updated as unknown as CallbackStore[E]; + } + }, + + close: () => { + closed = true; + }, + }; + + const publisher: MockWebSocketPublisher = { + publishOpen: (event) => { + if (closed) { + return; + } + for (const sub of store.open) { + sub(event); + } + }, + + publishError: (event) => { + if (closed) { + return; + } + for (const sub of store.error) { + sub(event); + } + }, + + publishMessage: (event) => { + if (closed) { + return; + } + for (const sub of store.message) { + sub(event); + } + }, + + publishClose: (event) => { + if (closed) { + return; + } + for (const sub of store.close) { + sub(event); + } + }, + }; + + return [mockSocket, publisher] as const; +} diff --git a/site/src/utils/OneWayWebSocket.test.ts b/site/src/utils/OneWayWebSocket.test.ts index c6b00b593111f..65ab9cb084b8b 100644 --- a/site/src/utils/OneWayWebSocket.test.ts +++ b/site/src/utils/OneWayWebSocket.test.ts @@ -8,144 +8,10 @@ */ import { - type OneWayMessageEvent, - OneWayWebSocket, - type WebSocketEventType, -} from "./OneWayWebSocket"; - -type MockPublisher = Readonly<{ - publishMessage: (event: MessageEvent) => void; - publishError: (event: ErrorEvent) => void; - publishClose: (event: CloseEvent) => void; - publishOpen: (event: Event) => void; -}>; - -function createMockWebSocket( - url: string, - protocols?: string | string[], -): readonly [WebSocket, MockPublisher] { - type EventMap = { - message: MessageEvent; - error: ErrorEvent; - close: CloseEvent; - open: Event; - }; - type CallbackStore = { - [K in keyof EventMap]: ((event: EventMap[K]) => void)[]; - }; - - let activeProtocol: string; - if (Array.isArray(protocols)) { - activeProtocol = protocols[0] ?? ""; - } else if (typeof protocols === "string") { - activeProtocol = protocols; - } else { - activeProtocol = ""; - } - - let closed = false; - const store: CallbackStore = { - message: [], - error: [], - close: [], - open: [], - }; - - const mockSocket: WebSocket = { - CONNECTING: 0, - OPEN: 1, - CLOSING: 2, - CLOSED: 3, - - url, - protocol: activeProtocol, - readyState: 1, - binaryType: "blob", - bufferedAmount: 0, - extensions: "", - onclose: null, - onerror: null, - onmessage: null, - onopen: null, - send: jest.fn(), - dispatchEvent: jest.fn(), - - addEventListener: ( - eventType: E, - callback: WebSocketEventMap[E], - ) => { - if (closed) { - return; - } - - const subscribers = store[eventType]; - const cb = callback as unknown as CallbackStore[E][0]; - if (!subscribers.includes(cb)) { - subscribers.push(cb); - } - }, - - removeEventListener: ( - eventType: E, - callback: WebSocketEventMap[E], - ) => { - if (closed) { - return; - } - - const subscribers = store[eventType]; - const cb = callback as unknown as CallbackStore[E][0]; - if (subscribers.includes(cb)) { - const updated = store[eventType].filter((c) => c !== cb); - store[eventType] = updated as unknown as CallbackStore[E]; - } - }, - - close: () => { - closed = true; - }, - }; - - const publisher: MockPublisher = { - publishOpen: (event) => { - if (closed) { - return; - } - for (const sub of store.open) { - sub(event); - } - }, - - publishError: (event) => { - if (closed) { - return; - } - for (const sub of store.error) { - sub(event); - } - }, - - publishMessage: (event) => { - if (closed) { - return; - } - for (const sub of store.message) { - sub(event); - } - }, - - publishClose: (event) => { - if (closed) { - return; - } - for (const sub of store.close) { - sub(event); - } - }, - }; - - return [mockSocket, publisher] as const; -} + createMockWebSocket, + type MockWebSocketPublisher, +} from "testHelpers/websockets"; +import { type OneWayMessageEvent, OneWayWebSocket } from "./OneWayWebSocket"; describe(OneWayWebSocket.name, () => { const dummyRoute = "/api/v2/blah"; @@ -167,7 +33,7 @@ describe(OneWayWebSocket.name, () => { }); it("Lets a consumer add an event listener of each type", () => { - let publisher!: MockPublisher; + let publisher!: MockWebSocketPublisher; const oneWay = new OneWayWebSocket({ apiRoute: dummyRoute, websocketInit: (url, protocols) => { @@ -207,7 +73,7 @@ describe(OneWayWebSocket.name, () => { }); it("Lets a consumer remove an event listener of each type", () => { - let publisher!: MockPublisher; + let publisher!: MockWebSocketPublisher; const oneWay = new OneWayWebSocket({ apiRoute: dummyRoute, websocketInit: (url, protocols) => { @@ -252,7 +118,7 @@ describe(OneWayWebSocket.name, () => { }); it("Only calls each callback once if callback is added multiple times", () => { - let publisher!: MockPublisher; + let publisher!: MockWebSocketPublisher; const oneWay = new OneWayWebSocket({ apiRoute: dummyRoute, websocketInit: (url, protocols) => { @@ -294,7 +160,7 @@ describe(OneWayWebSocket.name, () => { }); it("Lets consumers register multiple callbacks for each event type", () => { - let publisher!: MockPublisher; + let publisher!: MockWebSocketPublisher; const oneWay = new OneWayWebSocket({ apiRoute: dummyRoute, websocketInit: (url, protocols) => { @@ -375,7 +241,7 @@ describe(OneWayWebSocket.name, () => { }); it("Gives consumers pre-parsed versions of message events", () => { - let publisher!: MockPublisher; + let publisher!: MockWebSocketPublisher; const oneWay = new OneWayWebSocket({ apiRoute: dummyRoute, websocketInit: (url, protocols) => { @@ -405,7 +271,7 @@ describe(OneWayWebSocket.name, () => { }); it("Exposes parsing error if message payload could not be parsed as JSON", () => { - let publisher!: MockPublisher; + let publisher!: MockWebSocketPublisher; const oneWay = new OneWayWebSocket({ apiRoute: dummyRoute, websocketInit: (url, protocols) => { From 43d0ca8ec095b4cfe21aa50bbd49b886aa747e6d Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Fri, 23 May 2025 20:25:25 +0000 Subject: [PATCH 02/27] refactor: update useAgentLogs tests as unit tests --- .../modules/resources/useAgentLogs.test.ts | 56 ++++++++++++------- 1 file changed, 35 insertions(+), 21 deletions(-) diff --git a/site/src/modules/resources/useAgentLogs.test.ts b/site/src/modules/resources/useAgentLogs.test.ts index 72168ff6f16f1..085496486dbcf 100644 --- a/site/src/modules/resources/useAgentLogs.test.ts +++ b/site/src/modules/resources/useAgentLogs.test.ts @@ -1,8 +1,4 @@ -import { - renderHook, - type RenderHookResult, - waitFor, -} from "@testing-library/react"; +import { renderHook, waitFor } from "@testing-library/react"; import type { WorkspaceAgentLog } from "api/typesGenerated"; import { MockWorkspaceAgent } from "testHelpers/entities"; import { createUseAgentLogs } from "./useAgentLogs"; @@ -22,14 +18,28 @@ function generateMockLogs(count: number): WorkspaceAgentLog[] { })); } -type MountHookResult = Readonly< - RenderHookResult & { - publisher: MockWebSocketPublisher; - } ->; +// A mutable object holding the most recent mock WebSocket connection. This +// value will change as the hook opens/closes new connections +type PublisherResult = { + current: MockWebSocketPublisher; +}; + +type MountHookResult = Readonly<{ + // Note: the value of `current` should be readonly, but the `current` + // property itself should be mutable + hookResult: { + current: readonly WorkspaceAgentLog[]; + }; + rerender: (props: { enabled: boolean }) => void; + publisherResult: PublisherResult; +}>; function mountHook(): MountHookResult { - let publisher!: MockWebSocketPublisher; + // Have to cheat the types a little bit to avoid a chicken-and-the-egg + // scenario. publisherResult will be initialized with an undefined current + // value, but it'll be guaranteed not to be undefined by the time this + // function returns. + const publisherResult: Partial = { current: undefined }; const useAgentLogs = createUseAgentLogs((agentId, params) => { return new OneWayWebSocket({ apiRoute: `/api/v2/workspaceagents/${agentId}/logs`, @@ -38,41 +48,45 @@ function mountHook(): MountHookResult { after: params?.after?.toString() || "0", }), websocketInit: (url) => { - const [mockSocket, mockPub] = createMockWebSocket(url); - publisher = mockPub; + const [mockSocket, mockPublisher] = createMockWebSocket(url); + publisherResult.current = mockPublisher; return mockSocket; }, }); }); - const { result, rerender, unmount } = renderHook( + const { result, rerender } = renderHook( ({ enabled }) => useAgentLogs(MockWorkspaceAgent, enabled), { initialProps: { enabled: true } }, ); - return { result, rerender, unmount, publisher }; + return { + rerender, + hookResult: result, + publisherResult: publisherResult as PublisherResult, + }; } describe("useAgentLogs", () => { it("clears logs when hook becomes disabled (protection to avoid duplicate logs when hook goes back to being re-enabled)", async () => { - const { result, rerender, publisher } = mountHook(); + const { hookResult, publisherResult, rerender } = mountHook(); // Verify that logs can be received after mount const initialLogs = generateMockLogs(3); const initialEvent = new MessageEvent("message", { data: JSON.stringify(initialLogs), }); - publisher.publishMessage(initialEvent); + publisherResult.current.publishMessage(initialEvent); await waitFor(() => { // Using expect.arrayContaining to account for the fact that we're // not guaranteed to receive WebSocket events in order - expect(result.current).toEqual(expect.arrayContaining(initialLogs)); + expect(hookResult.current).toEqual(expect.arrayContaining(initialLogs)); }); // Disable the hook (and have the hook close the connection behind the // scenes) rerender({ enabled: false }); - await waitFor(() => expect(result.current).toHaveLength(0)); + await waitFor(() => expect(hookResult.current).toHaveLength(0)); // Re-enable the hook (creating an entirely new connection), and send // new logs @@ -81,9 +95,9 @@ describe("useAgentLogs", () => { const newEvent = new MessageEvent("message", { data: JSON.stringify(newLogs), }); - publisher.publishMessage(newEvent); + publisherResult.current.publishMessage(newEvent); await waitFor(() => { - expect(result.current).toEqual(expect.arrayContaining(newLogs)); + expect(hookResult.current).toEqual(expect.arrayContaining(newLogs)); }); }); }); From 727bdddaa30c38a83809022acd3de393962d09c2 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Fri, 23 May 2025 20:30:10 +0000 Subject: [PATCH 03/27] docs: rewrite comment for clarity --- site/src/modules/resources/useAgentLogs.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/modules/resources/useAgentLogs.test.ts b/site/src/modules/resources/useAgentLogs.test.ts index 085496486dbcf..54161bf35ce7e 100644 --- a/site/src/modules/resources/useAgentLogs.test.ts +++ b/site/src/modules/resources/useAgentLogs.test.ts @@ -18,7 +18,7 @@ function generateMockLogs(count: number): WorkspaceAgentLog[] { })); } -// A mutable object holding the most recent mock WebSocket connection. This +// A mutable object holding the most recent mock WebSocket publisher. The inner // value will change as the hook opens/closes new connections type PublisherResult = { current: MockWebSocketPublisher; From d46d1444ef0e54e599f721a1273ddc54d2a046ec Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Fri, 23 May 2025 20:31:23 +0000 Subject: [PATCH 04/27] fix: remove unnecessary type --- site/src/testHelpers/websockets.ts | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/site/src/testHelpers/websockets.ts b/site/src/testHelpers/websockets.ts index 3d98f543d0684..ac356f1c8dc28 100644 --- a/site/src/testHelpers/websockets.ts +++ b/site/src/testHelpers/websockets.ts @@ -7,23 +7,6 @@ export type MockWebSocketPublisher = Readonly<{ publishOpen: (event: Event) => void; }>; -export type CreateMockWebSocketOptions = Readonly<{ - // The URL to use to initialize the mock socket. This should match the - // "real" URL that you would pass to the built-in WebSocket constructor. - url: string; - - // The additional WebSocket protocols to use when initializing. This should - // match the real protocols that you would pass to the built-in WebSocket - // constructor. - protocols?: string | string[]; - - // Indicates whether the mock socket should stay open after calling the - // .close method, so that it can be reused for a new connection. Defaults to - // false (meaning that the socket becomes completely unusable the first time - // after .close is called). - persistAfterClose?: boolean; -}>; - export function createMockWebSocket( url: string, protocols?: string | string[], From 91a6fc12d304bb0aac59c48535fee964fd8ad3f0 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Fri, 23 May 2025 21:04:13 +0000 Subject: [PATCH 05/27] fix: make sure logs have different timestamps --- .../modules/resources/useAgentLogs.test.ts | 31 +++++++++++++------ site/src/utils/OneWayWebSocket.test.ts | 2 +- 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/site/src/modules/resources/useAgentLogs.test.ts b/site/src/modules/resources/useAgentLogs.test.ts index 54161bf35ce7e..eedc7c4ad6393 100644 --- a/site/src/modules/resources/useAgentLogs.test.ts +++ b/site/src/modules/resources/useAgentLogs.test.ts @@ -1,21 +1,32 @@ import { renderHook, waitFor } from "@testing-library/react"; import type { WorkspaceAgentLog } from "api/typesGenerated"; import { MockWorkspaceAgent } from "testHelpers/entities"; -import { createUseAgentLogs } from "./useAgentLogs"; import { - createMockWebSocket, type MockWebSocketPublisher, + createMockWebSocket, } from "testHelpers/websockets"; import { OneWayWebSocket } from "utils/OneWayWebSocket"; +import { createUseAgentLogs } from "./useAgentLogs"; + +const millisecondsInOneMinute = 60_000; -function generateMockLogs(count: number): WorkspaceAgentLog[] { - return Array.from({ length: count }, (_, i) => ({ - id: i, - created_at: new Date().toISOString(), - level: "info", - output: `Log ${i}`, - source_id: "", - })); +function generateMockLogs( + logCount: number, + baseDate = new Date(), +): readonly WorkspaceAgentLog[] { + return Array.from({ length: logCount }, (_, i) => { + // Make sure that the logs generated each have unique timestamps, so + // that we can test whether they're being sorted properly before being + // returned by the hook + const logDate = new Date(baseDate.getTime() + i * millisecondsInOneMinute); + return { + id: i, + created_at: logDate.toISOString(), + level: "info", + output: `Log ${i}`, + source_id: "", + }; + }); } // A mutable object holding the most recent mock WebSocket publisher. The inner diff --git a/site/src/utils/OneWayWebSocket.test.ts b/site/src/utils/OneWayWebSocket.test.ts index 65ab9cb084b8b..b334732c5b5e8 100644 --- a/site/src/utils/OneWayWebSocket.test.ts +++ b/site/src/utils/OneWayWebSocket.test.ts @@ -8,8 +8,8 @@ */ import { - createMockWebSocket, type MockWebSocketPublisher, + createMockWebSocket, } from "testHelpers/websockets"; import { type OneWayMessageEvent, OneWayWebSocket } from "./OneWayWebSocket"; From ecbe7b0466c01cf692a4ffcf73edcf43c1153dfa Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Fri, 23 May 2025 21:06:42 +0000 Subject: [PATCH 06/27] fix: add different dates to reduce risk of false positives --- site/src/modules/resources/useAgentLogs.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/site/src/modules/resources/useAgentLogs.test.ts b/site/src/modules/resources/useAgentLogs.test.ts index eedc7c4ad6393..85206171bf7d5 100644 --- a/site/src/modules/resources/useAgentLogs.test.ts +++ b/site/src/modules/resources/useAgentLogs.test.ts @@ -83,7 +83,7 @@ describe("useAgentLogs", () => { const { hookResult, publisherResult, rerender } = mountHook(); // Verify that logs can be received after mount - const initialLogs = generateMockLogs(3); + const initialLogs = generateMockLogs(3, new Date("april 5, 1997")); const initialEvent = new MessageEvent("message", { data: JSON.stringify(initialLogs), }); @@ -102,7 +102,7 @@ describe("useAgentLogs", () => { // Re-enable the hook (creating an entirely new connection), and send // new logs rerender({ enabled: true }); - const newLogs = generateMockLogs(3); + const newLogs = generateMockLogs(3, new Date("october 3, 2005")); const newEvent = new MessageEvent("message", { data: JSON.stringify(newLogs), }); From abd655389d39d53ca25236bc52b15bb6e89bf9c5 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Sat, 2 Aug 2025 14:01:35 +0000 Subject: [PATCH 07/27] refactor: decrease coupling --- site/src/api/api.ts | 2 +- site/src/modules/resources/useAgentLogs.ts | 18 ++++++++---------- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/site/src/api/api.ts b/site/src/api/api.ts index 2b21ddf1e8a08..5fda11cedd107 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -264,7 +264,7 @@ export const watchWorkspaceAgentLogs = ( }); }; -type WatchWorkspaceAgentLogsParams = { +export type WatchWorkspaceAgentLogsParams = { after?: number; }; diff --git a/site/src/modules/resources/useAgentLogs.ts b/site/src/modules/resources/useAgentLogs.ts index 63ea4afbb99c1..0dcb57dc46eae 100644 --- a/site/src/modules/resources/useAgentLogs.ts +++ b/site/src/modules/resources/useAgentLogs.ts @@ -1,11 +1,15 @@ -import { watchWorkspaceAgentLogs } from "api/api"; +import { watchWorkspaceAgentLogs, type WatchWorkspaceAgentLogsParams } from "api/api"; import type { WorkspaceAgent, WorkspaceAgentLog } from "api/typesGenerated"; import { displayError } from "components/GlobalSnackbar/utils"; import { useEffect, useState } from "react"; +import type { OneWayWebSocket } from "utils/OneWayWebSocket"; -export function createUseAgentLogs( - createSocket: typeof watchWorkspaceAgentLogs, -) { +export type CreateSocket = ( + agentId: string, + params?: WatchWorkspaceAgentLogsParams, +) => OneWayWebSocket; + +export function createUseAgentLogs(createSocket: CreateSocket) { return function useAgentLogs( agent: WorkspaceAgent, enabled: boolean, @@ -49,12 +53,6 @@ export function createUseAgentLogs( }); return () => socket.close(); - - // createSocket shouldn't ever change for the lifecycle of the hook, - // but Biome isn't smart enough to detect constant dependencies for - // higher-order hooks. Adding it to the array (even though it - // shouldn't ever be needed) seemed like the least fragile way to - // resolve the warning. }, [createSocket, agent.id, enabled]); return logs; From bade97aa444f371a1b161df08d34d2fcd1e83a33 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Sat, 2 Aug 2025 14:29:09 +0000 Subject: [PATCH 08/27] wip: commit progress on updating flake --- site/src/hooks/useEmbeddedMetadata.ts | 3 +- .../modules/resources/useAgentLogs.test.ts | 38 ++++++++++++++----- site/src/testHelpers/websockets.ts | 18 +++++---- 3 files changed, 39 insertions(+), 20 deletions(-) diff --git a/site/src/hooks/useEmbeddedMetadata.ts b/site/src/hooks/useEmbeddedMetadata.ts index f4a1d59528677..64926755ece33 100644 --- a/site/src/hooks/useEmbeddedMetadata.ts +++ b/site/src/hooks/useEmbeddedMetadata.ts @@ -229,13 +229,12 @@ export function makeUseEmbeddedMetadata( manager.getMetadata, ); - // biome-ignore lint/correctness/useExhaustiveDependencies(manager.clearMetadataByKey): baked into containing hook const stableMetadataResult = useMemo(() => { return { metadata, clearMetadataByKey: manager.clearMetadataByKey, }; - }, [metadata]); + }, [manager, metadata]); return stableMetadataResult; }; diff --git a/site/src/modules/resources/useAgentLogs.test.ts b/site/src/modules/resources/useAgentLogs.test.ts index 85206171bf7d5..e733abd969929 100644 --- a/site/src/modules/resources/useAgentLogs.test.ts +++ b/site/src/modules/resources/useAgentLogs.test.ts @@ -79,20 +79,40 @@ function mountHook(): MountHookResult { } describe("useAgentLogs", () => { - it("clears logs when hook becomes disabled (protection to avoid duplicate logs when hook goes back to being re-enabled)", async () => { + it("Automatically sorts logs that are received out of order", async () => { + const { hookResult, publisherResult } = mountHook(); + const logs = generateMockLogs(10, new Date("september 9, 1999")); + const reversed = logs.toReversed(); + + for (const log of reversed) { + publisherResult.current.publishMessage( + new MessageEvent("message", { + data: JSON.stringify(log), + }), + ) + } + await waitFor(() => expect(hookResult.current).toEqual(logs)); + }); + + it("Automatically closes the socket connection when the hook is disabled", async () => { + const { publisherResult, rerender } = mountHook(); + expect(publisherResult.current.isConnectionOpen()).toBe(true); + rerender({ enabled: false }); + await waitFor(() => { + expect(publisherResult.current.isConnectionOpen()).toBe(false); + }) + }); + + it("Clears logs when hook becomes disabled (protection to avoid duplicate logs when hook goes back to being re-enabled)", async () => { const { hookResult, publisherResult, rerender } = mountHook(); - // Verify that logs can be received after mount + // Send initial logs so that we have something to clear out later const initialLogs = generateMockLogs(3, new Date("april 5, 1997")); const initialEvent = new MessageEvent("message", { data: JSON.stringify(initialLogs), }); publisherResult.current.publishMessage(initialEvent); - await waitFor(() => { - // Using expect.arrayContaining to account for the fact that we're - // not guaranteed to receive WebSocket events in order - expect(hookResult.current).toEqual(expect.arrayContaining(initialLogs)); - }); + await waitFor(() => expect(hookResult.current).toEqual(initialLogs)); // Disable the hook (and have the hook close the connection behind the // scenes) @@ -107,8 +127,6 @@ describe("useAgentLogs", () => { data: JSON.stringify(newLogs), }); publisherResult.current.publishMessage(newEvent); - await waitFor(() => { - expect(hookResult.current).toEqual(expect.arrayContaining(newLogs)); - }); + await waitFor(() => expect(hookResult.current).toEqual(newLogs)); }); }); diff --git a/site/src/testHelpers/websockets.ts b/site/src/testHelpers/websockets.ts index ac356f1c8dc28..e4d3b7cb54bf1 100644 --- a/site/src/testHelpers/websockets.ts +++ b/site/src/testHelpers/websockets.ts @@ -5,6 +5,7 @@ export type MockWebSocketPublisher = Readonly<{ publishError: (event: ErrorEvent) => void; publishClose: (event: CloseEvent) => void; publishOpen: (event: Event) => void; + isConnectionOpen: () => boolean; }>; export function createMockWebSocket( @@ -30,7 +31,7 @@ export function createMockWebSocket( activeProtocol = ""; } - let closed = false; + let isOpen = true; const store: CallbackStore = { message: [], error: [], @@ -61,7 +62,7 @@ export function createMockWebSocket( eventType: E, callback: WebSocketEventMap[E], ) => { - if (closed) { + if (!isOpen) { return; } @@ -76,7 +77,7 @@ export function createMockWebSocket( eventType: E, callback: WebSocketEventMap[E], ) => { - if (closed) { + if (!isOpen) { return; } @@ -89,13 +90,14 @@ export function createMockWebSocket( }, close: () => { - closed = true; + isOpen = false; }, }; const publisher: MockWebSocketPublisher = { + isConnectionOpen: () => isOpen, publishOpen: (event) => { - if (closed) { + if (!isOpen) { return; } for (const sub of store.open) { @@ -104,7 +106,7 @@ export function createMockWebSocket( }, publishError: (event) => { - if (closed) { + if (!isOpen) { return; } for (const sub of store.error) { @@ -113,7 +115,7 @@ export function createMockWebSocket( }, publishMessage: (event) => { - if (closed) { + if (!isOpen) { return; } for (const sub of store.message) { @@ -122,7 +124,7 @@ export function createMockWebSocket( }, publishClose: (event) => { - if (closed) { + if (!isOpen) { return; } for (const sub of store.close) { From bc3d095aaa15e3695b2ee5e735dca7c407b8aa9a Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Sat, 2 Aug 2025 15:05:51 +0000 Subject: [PATCH 09/27] fix: get all tests passing --- .../modules/resources/useAgentLogs.test.ts | 20 +++++++------- site/src/modules/resources/useAgentLogs.ts | 27 +++++++++++++++++-- 2 files changed, 36 insertions(+), 11 deletions(-) diff --git a/site/src/modules/resources/useAgentLogs.test.ts b/site/src/modules/resources/useAgentLogs.test.ts index e733abd969929..0132fc575da0a 100644 --- a/site/src/modules/resources/useAgentLogs.test.ts +++ b/site/src/modules/resources/useAgentLogs.test.ts @@ -7,6 +7,7 @@ import { } from "testHelpers/websockets"; import { OneWayWebSocket } from "utils/OneWayWebSocket"; import { createUseAgentLogs } from "./useAgentLogs"; +import { act } from "react"; const millisecondsInOneMinute = 60_000; @@ -82,14 +83,15 @@ describe("useAgentLogs", () => { it("Automatically sorts logs that are received out of order", async () => { const { hookResult, publisherResult } = mountHook(); const logs = generateMockLogs(10, new Date("september 9, 1999")); - const reversed = logs.toReversed(); - for (const log of reversed) { - publisherResult.current.publishMessage( - new MessageEvent("message", { - data: JSON.stringify(log), - }), - ) + for (const log of logs.toReversed()) { + act(() => { + publisherResult.current.publishMessage( + new MessageEvent("message", { + data: JSON.stringify([log]), + }), + ) + }) } await waitFor(() => expect(hookResult.current).toEqual(logs)); }); @@ -111,7 +113,7 @@ describe("useAgentLogs", () => { const initialEvent = new MessageEvent("message", { data: JSON.stringify(initialLogs), }); - publisherResult.current.publishMessage(initialEvent); + act(() => publisherResult.current.publishMessage(initialEvent)); await waitFor(() => expect(hookResult.current).toEqual(initialLogs)); // Disable the hook (and have the hook close the connection behind the @@ -126,7 +128,7 @@ describe("useAgentLogs", () => { const newEvent = new MessageEvent("message", { data: JSON.stringify(newLogs), }); - publisherResult.current.publishMessage(newEvent); + act(() => publisherResult.current.publishMessage(newEvent)); await waitFor(() => expect(hookResult.current).toEqual(newLogs)); }); }); diff --git a/site/src/modules/resources/useAgentLogs.ts b/site/src/modules/resources/useAgentLogs.ts index 0dcb57dc46eae..6c32e4faf7455 100644 --- a/site/src/modules/resources/useAgentLogs.ts +++ b/site/src/modules/resources/useAgentLogs.ts @@ -34,19 +34,42 @@ export function createUseAgentLogs(createSocket: CreateSocket) { // Always fetch the logs from the beginning. We may want to optimize // this in the future, but it would add some complexity in the code // that might not be worth it. + const createdAtMap = new Map(); const socket = createSocket(agent.id, { after: 0 }); socket.addEventListener("message", (e) => { if (e.parseError) { console.warn("Error parsing agent log: ", e.parseError); return; } - setLogs((logs) => [...logs, ...e.parsedMessage]); + + if (e.parsedMessage.length === 0) { + return; + } + + setLogs((logs) => { + const newLogs = [...logs, ...e.parsedMessage]; + newLogs.sort((l1, l2) => { + let d1 = createdAtMap.get(l1.created_at); + if (d1 === undefined) { + d1 = (new Date(l1.created_at)).getTime(); + createdAtMap.set(l1.created_at, d1); + } + let d2 = createdAtMap.get(l2.created_at); + if (d2 === undefined) { + d2 = (new Date(l2.created_at)).getTime(); + createdAtMap.set(l2.created_at, d2); + } + return d1 - d2; + }); + + return newLogs; + }); }); socket.addEventListener("error", (e) => { console.error("Error in agent log socket: ", e); displayError( - "Unable to watch the agent logs", + "Unable to watch agent logs", "Please try refreshing the browser", ); socket.close(); From 550d09e422a886eb5b7710dbb908d70f946f5c55 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Sat, 2 Aug 2025 20:38:56 +0000 Subject: [PATCH 10/27] chore: add one more test case --- .../modules/resources/useAgentLogs.test.ts | 58 +++++++++++-------- site/src/modules/resources/useAgentLogs.ts | 8 +-- 2 files changed, 39 insertions(+), 27 deletions(-) diff --git a/site/src/modules/resources/useAgentLogs.test.ts b/site/src/modules/resources/useAgentLogs.test.ts index 0132fc575da0a..4a2910d352d13 100644 --- a/site/src/modules/resources/useAgentLogs.test.ts +++ b/site/src/modules/resources/useAgentLogs.test.ts @@ -17,8 +17,8 @@ function generateMockLogs( ): readonly WorkspaceAgentLog[] { return Array.from({ length: logCount }, (_, i) => { // Make sure that the logs generated each have unique timestamps, so - // that we can test whether they're being sorted properly before being - // returned by the hook + // that we can test whether the hook is sorting them properly as it's + // receiving them over time const logDate = new Date(baseDate.getTime() + i * millisecondsInOneMinute); return { id: i, @@ -32,25 +32,22 @@ function generateMockLogs( // A mutable object holding the most recent mock WebSocket publisher. The inner // value will change as the hook opens/closes new connections -type PublisherResult = { - current: MockWebSocketPublisher; -}; +type PublisherResult = { current: MockWebSocketPublisher; }; type MountHookResult = Readonly<{ - // Note: the value of `current` should be readonly, but the `current` - // property itself should be mutable - hookResult: { - current: readonly WorkspaceAgentLog[]; - }; - rerender: (props: { enabled: boolean }) => void; + rerender: (props: { agentId: string, enabled: boolean }) => void; publisherResult: PublisherResult; + + // Note: the `current` property is only "halfway" readonly; the value is + // readonly, but the key is still mutable + hookResult: { current: readonly WorkspaceAgentLog[]; }; }>; -function mountHook(): MountHookResult { +function mountHook(initialAgentId: string): MountHookResult { // Have to cheat the types a little bit to avoid a chicken-and-the-egg // scenario. publisherResult will be initialized with an undefined current - // value, but it'll be guaranteed not to be undefined by the time this - // function returns. + // value, but it'll be guaranteed not to be undefined by the time mountHook + // returns its value. const publisherResult: Partial = { current: undefined }; const useAgentLogs = createUseAgentLogs((agentId, params) => { return new OneWayWebSocket({ @@ -68,8 +65,8 @@ function mountHook(): MountHookResult { }); const { result, rerender } = renderHook( - ({ enabled }) => useAgentLogs(MockWorkspaceAgent, enabled), - { initialProps: { enabled: true } }, + ({ agentId, enabled }) => useAgentLogs(agentId, enabled), + { initialProps: { agentId: initialAgentId, enabled: true }, }, ); return { @@ -81,10 +78,11 @@ function mountHook(): MountHookResult { describe("useAgentLogs", () => { it("Automatically sorts logs that are received out of order", async () => { - const { hookResult, publisherResult } = mountHook(); + const { hookResult, publisherResult } = mountHook(MockWorkspaceAgent.id); const logs = generateMockLogs(10, new Date("september 9, 1999")); + const reversed = logs.toReversed(); - for (const log of logs.toReversed()) { + for (const log of reversed) { act(() => { publisherResult.current.publishMessage( new MessageEvent("message", { @@ -97,16 +95,30 @@ describe("useAgentLogs", () => { }); it("Automatically closes the socket connection when the hook is disabled", async () => { - const { publisherResult, rerender } = mountHook(); + const { publisherResult, rerender } = mountHook(MockWorkspaceAgent.id); expect(publisherResult.current.isConnectionOpen()).toBe(true); - rerender({ enabled: false }); + rerender({ agentId: MockWorkspaceAgent.id, enabled: false }); await waitFor(() => { expect(publisherResult.current.isConnectionOpen()).toBe(false); }) }); + it("Automatically closes the old connection when the agent ID changes", () => { + const { publisherResult, rerender } = mountHook(MockWorkspaceAgent.id); + const publisher1 = publisherResult.current; + expect(publisher1.isConnectionOpen()).toBe(true); + + const newAgentId = `${MockWorkspaceAgent.id}-2`; + rerender({ agentId: newAgentId, enabled: true }); + + const publisher2 = publisherResult.current; + expect(publisher1.isConnectionOpen()).toBe(false); + expect(publisher2.isConnectionOpen()).toBe(true); + }); + it("Clears logs when hook becomes disabled (protection to avoid duplicate logs when hook goes back to being re-enabled)", async () => { - const { hookResult, publisherResult, rerender } = mountHook(); + const { hookResult, publisherResult, rerender } + = mountHook(MockWorkspaceAgent.id); // Send initial logs so that we have something to clear out later const initialLogs = generateMockLogs(3, new Date("april 5, 1997")); @@ -118,12 +130,12 @@ describe("useAgentLogs", () => { // Disable the hook (and have the hook close the connection behind the // scenes) - rerender({ enabled: false }); + rerender({ agentId: MockWorkspaceAgent.id, enabled: false }); await waitFor(() => expect(hookResult.current).toHaveLength(0)); // Re-enable the hook (creating an entirely new connection), and send // new logs - rerender({ enabled: true }); + rerender({ agentId: MockWorkspaceAgent.id, enabled: true }); const newLogs = generateMockLogs(3, new Date("october 3, 2005")); const newEvent = new MessageEvent("message", { data: JSON.stringify(newLogs), diff --git a/site/src/modules/resources/useAgentLogs.ts b/site/src/modules/resources/useAgentLogs.ts index 6c32e4faf7455..1df53810e6e4a 100644 --- a/site/src/modules/resources/useAgentLogs.ts +++ b/site/src/modules/resources/useAgentLogs.ts @@ -1,5 +1,5 @@ import { watchWorkspaceAgentLogs, type WatchWorkspaceAgentLogsParams } from "api/api"; -import type { WorkspaceAgent, WorkspaceAgentLog } from "api/typesGenerated"; +import type { WorkspaceAgentLog } from "api/typesGenerated"; import { displayError } from "components/GlobalSnackbar/utils"; import { useEffect, useState } from "react"; import type { OneWayWebSocket } from "utils/OneWayWebSocket"; @@ -11,7 +11,7 @@ export type CreateSocket = ( export function createUseAgentLogs(createSocket: CreateSocket) { return function useAgentLogs( - agent: WorkspaceAgent, + agentId: string, enabled: boolean, ): readonly WorkspaceAgentLog[] { const [logs, setLogs] = useState([]); @@ -35,7 +35,7 @@ export function createUseAgentLogs(createSocket: CreateSocket) { // this in the future, but it would add some complexity in the code // that might not be worth it. const createdAtMap = new Map(); - const socket = createSocket(agent.id, { after: 0 }); + const socket = createSocket(agentId, { after: 0 }); socket.addEventListener("message", (e) => { if (e.parseError) { console.warn("Error parsing agent log: ", e.parseError); @@ -76,7 +76,7 @@ export function createUseAgentLogs(createSocket: CreateSocket) { }); return () => socket.close(); - }, [createSocket, agent.id, enabled]); + }, [createSocket, agentId, enabled]); return logs; }; From cc7e63232ab5664eac9eee6e0233425196aa17c9 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Sat, 2 Aug 2025 20:49:40 +0000 Subject: [PATCH 11/27] fix: update type mismatches --- site/src/modules/resources/AgentRow.tsx | 2 +- .../modules/resources/useAgentLogs.test.ts | 21 ++++++++++--------- site/src/modules/resources/useAgentLogs.ts | 11 ++++++---- .../WorkspaceBuildPageView.tsx | 7 +------ 4 files changed, 20 insertions(+), 21 deletions(-) diff --git a/site/src/modules/resources/AgentRow.tsx b/site/src/modules/resources/AgentRow.tsx index ab0e5884c48e9..bfc998277249a 100644 --- a/site/src/modules/resources/AgentRow.tsx +++ b/site/src/modules/resources/AgentRow.tsx @@ -76,7 +76,7 @@ export const AgentRow: FC = ({ ["starting", "start_timeout"].includes(agent.lifecycle_state) && hasStartupFeatures, ); - const agentLogs = useAgentLogs(agent, showLogs); + const agentLogs = useAgentLogs(agent.id, showLogs); const logListRef = useRef(null); const logListDivRef = useRef(null); const startupLogs = useMemo(() => { diff --git a/site/src/modules/resources/useAgentLogs.test.ts b/site/src/modules/resources/useAgentLogs.test.ts index 4a2910d352d13..f2fb922fb3ed2 100644 --- a/site/src/modules/resources/useAgentLogs.test.ts +++ b/site/src/modules/resources/useAgentLogs.test.ts @@ -1,5 +1,6 @@ import { renderHook, waitFor } from "@testing-library/react"; import type { WorkspaceAgentLog } from "api/typesGenerated"; +import { act } from "react"; import { MockWorkspaceAgent } from "testHelpers/entities"; import { type MockWebSocketPublisher, @@ -7,7 +8,6 @@ import { } from "testHelpers/websockets"; import { OneWayWebSocket } from "utils/OneWayWebSocket"; import { createUseAgentLogs } from "./useAgentLogs"; -import { act } from "react"; const millisecondsInOneMinute = 60_000; @@ -32,15 +32,15 @@ function generateMockLogs( // A mutable object holding the most recent mock WebSocket publisher. The inner // value will change as the hook opens/closes new connections -type PublisherResult = { current: MockWebSocketPublisher; }; +type PublisherResult = { current: MockWebSocketPublisher }; type MountHookResult = Readonly<{ - rerender: (props: { agentId: string, enabled: boolean }) => void; + rerender: (props: { agentId: string; enabled: boolean }) => void; publisherResult: PublisherResult; // Note: the `current` property is only "halfway" readonly; the value is // readonly, but the key is still mutable - hookResult: { current: readonly WorkspaceAgentLog[]; }; + hookResult: { current: readonly WorkspaceAgentLog[] }; }>; function mountHook(initialAgentId: string): MountHookResult { @@ -66,7 +66,7 @@ function mountHook(initialAgentId: string): MountHookResult { const { result, rerender } = renderHook( ({ agentId, enabled }) => useAgentLogs(agentId, enabled), - { initialProps: { agentId: initialAgentId, enabled: true }, }, + { initialProps: { agentId: initialAgentId, enabled: true } }, ); return { @@ -88,8 +88,8 @@ describe("useAgentLogs", () => { new MessageEvent("message", { data: JSON.stringify([log]), }), - ) - }) + ); + }); } await waitFor(() => expect(hookResult.current).toEqual(logs)); }); @@ -100,7 +100,7 @@ describe("useAgentLogs", () => { rerender({ agentId: MockWorkspaceAgent.id, enabled: false }); await waitFor(() => { expect(publisherResult.current.isConnectionOpen()).toBe(false); - }) + }); }); it("Automatically closes the old connection when the agent ID changes", () => { @@ -117,8 +117,9 @@ describe("useAgentLogs", () => { }); it("Clears logs when hook becomes disabled (protection to avoid duplicate logs when hook goes back to being re-enabled)", async () => { - const { hookResult, publisherResult, rerender } - = mountHook(MockWorkspaceAgent.id); + const { hookResult, publisherResult, rerender } = mountHook( + MockWorkspaceAgent.id, + ); // Send initial logs so that we have something to clear out later const initialLogs = generateMockLogs(3, new Date("april 5, 1997")); diff --git a/site/src/modules/resources/useAgentLogs.ts b/site/src/modules/resources/useAgentLogs.ts index 1df53810e6e4a..aa2da5c247d69 100644 --- a/site/src/modules/resources/useAgentLogs.ts +++ b/site/src/modules/resources/useAgentLogs.ts @@ -1,10 +1,13 @@ -import { watchWorkspaceAgentLogs, type WatchWorkspaceAgentLogsParams } from "api/api"; +import { + type WatchWorkspaceAgentLogsParams, + watchWorkspaceAgentLogs, +} from "api/api"; import type { WorkspaceAgentLog } from "api/typesGenerated"; import { displayError } from "components/GlobalSnackbar/utils"; import { useEffect, useState } from "react"; import type { OneWayWebSocket } from "utils/OneWayWebSocket"; -export type CreateSocket = ( +type CreateSocket = ( agentId: string, params?: WatchWorkspaceAgentLogsParams, ) => OneWayWebSocket; @@ -51,12 +54,12 @@ export function createUseAgentLogs(createSocket: CreateSocket) { newLogs.sort((l1, l2) => { let d1 = createdAtMap.get(l1.created_at); if (d1 === undefined) { - d1 = (new Date(l1.created_at)).getTime(); + d1 = new Date(l1.created_at).getTime(); createdAtMap.set(l1.created_at, d1); } let d2 = createdAtMap.get(l2.created_at); if (d2 === undefined) { - d2 = (new Date(l2.created_at)).getTime(); + d2 = new Date(l2.created_at).getTime(); createdAtMap.set(l2.created_at, d2); } return d1 - d2; diff --git a/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx b/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx index 3a45653557dcc..dbd1aa7cf21e2 100644 --- a/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx +++ b/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx @@ -308,12 +308,7 @@ type AgentLogsContentProps = { }; const AgentLogsContent: FC = ({ agent }) => { - const logs = useAgentLogs(agent, true); - - if (!logs) { - return ; - } - + const logs = useAgentLogs(agent.id, true); return ( Date: Sat, 2 Aug 2025 20:55:16 +0000 Subject: [PATCH 12/27] refactor: clean up some code --- site/src/modules/resources/AgentRow.tsx | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/site/src/modules/resources/AgentRow.tsx b/site/src/modules/resources/AgentRow.tsx index bfc998277249a..18926506571d3 100644 --- a/site/src/modules/resources/AgentRow.tsx +++ b/site/src/modules/resources/AgentRow.tsx @@ -6,6 +6,7 @@ import type { Template, Workspace, WorkspaceAgent, + WorkspaceAgentLog, WorkspaceAgentMetadata, } from "api/typesGenerated"; import { Button } from "components/Button/Button"; @@ -79,21 +80,21 @@ export const AgentRow: FC = ({ const agentLogs = useAgentLogs(agent.id, showLogs); const logListRef = useRef(null); const logListDivRef = useRef(null); - const startupLogs = useMemo(() => { - const allLogs = agentLogs || []; - - const logs = [...allLogs]; - if (agent.logs_overflowed) { - logs.push({ + const startupLogs = useMemo(() => { + if (!agent.logs_overflowed) { + return agentLogs; + } + return [ + ...agentLogs, + { id: -1, level: "error", output: "Startup logs exceeded the max size of 1MB, and will not continue to be written to the database! Logs will continue to be written to the /tmp/coder-startup-script.log file in the workspace.", created_at: new Date().toISOString(), source_id: "", - }); - } - return logs; + }, + ]; }, [agentLogs, agent.logs_overflowed]); const [bottomOfLogs, setBottomOfLogs] = useState(true); From 43a0d3a91952a8eaf82912b4464e36164d5042be Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Sat, 2 Aug 2025 21:41:55 +0000 Subject: [PATCH 13/27] fix: make testing boundaries more formal --- .../modules/resources/useAgentLogs.test.ts | 102 ++++++++++++------ site/src/modules/resources/useAgentLogs.ts | 26 +++-- site/src/testHelpers/websockets.ts | 4 +- 3 files changed, 91 insertions(+), 41 deletions(-) diff --git a/site/src/modules/resources/useAgentLogs.test.ts b/site/src/modules/resources/useAgentLogs.test.ts index f2fb922fb3ed2..99ad052f1ced4 100644 --- a/site/src/modules/resources/useAgentLogs.test.ts +++ b/site/src/modules/resources/useAgentLogs.test.ts @@ -7,7 +7,7 @@ import { createMockWebSocket, } from "testHelpers/websockets"; import { OneWayWebSocket } from "utils/OneWayWebSocket"; -import { createUseAgentLogs } from "./useAgentLogs"; +import { type OnError, createUseAgentLogs } from "./useAgentLogs"; const millisecondsInOneMinute = 60_000; @@ -30,9 +30,16 @@ function generateMockLogs( }); } -// A mutable object holding the most recent mock WebSocket publisher. The inner -// value will change as the hook opens/closes new connections -type PublisherResult = { current: MockWebSocketPublisher }; +// A mutable object holding the most recent mock WebSocket publisher. Inner +// value will be undefined if the hook is disabled on mount, but will otherwise +// have some kind of value +type PublisherResult = { current: MockWebSocketPublisher | undefined }; + +type MountHookOptions = Readonly<{ + initialAgentId: string; + enabled?: boolean; + onError?: OnError; +}>; type MountHookResult = Readonly<{ rerender: (props: { agentId: string; enabled: boolean }) => void; @@ -43,12 +50,10 @@ type MountHookResult = Readonly<{ hookResult: { current: readonly WorkspaceAgentLog[] }; }>; -function mountHook(initialAgentId: string): MountHookResult { - // Have to cheat the types a little bit to avoid a chicken-and-the-egg - // scenario. publisherResult will be initialized with an undefined current - // value, but it'll be guaranteed not to be undefined by the time mountHook - // returns its value. - const publisherResult: Partial = { current: undefined }; +function mountHook(options: MountHookOptions): MountHookResult { + const { initialAgentId, enabled = true, onError = jest.fn() } = options; + + const publisherResult: PublisherResult = { current: undefined }; const useAgentLogs = createUseAgentLogs((agentId, params) => { return new OneWayWebSocket({ apiRoute: `/api/v2/workspaceagents/${agentId}/logs`, @@ -62,29 +67,32 @@ function mountHook(initialAgentId: string): MountHookResult { return mockSocket; }, }); - }); + }, onError); const { result, rerender } = renderHook( ({ agentId, enabled }) => useAgentLogs(agentId, enabled), - { initialProps: { agentId: initialAgentId, enabled: true } }, + { initialProps: { agentId: initialAgentId, enabled: enabled } }, ); return { rerender, hookResult: result, - publisherResult: publisherResult as PublisherResult, + publisherResult, }; } describe("useAgentLogs", () => { it("Automatically sorts logs that are received out of order", async () => { - const { hookResult, publisherResult } = mountHook(MockWorkspaceAgent.id); + const { hookResult, publisherResult } = mountHook({ + initialAgentId: MockWorkspaceAgent.id, + }); + const logs = generateMockLogs(10, new Date("september 9, 1999")); const reversed = logs.toReversed(); for (const log of reversed) { act(() => { - publisherResult.current.publishMessage( + publisherResult.current?.publishMessage( new MessageEvent("message", { data: JSON.stringify([log]), }), @@ -94,54 +102,86 @@ describe("useAgentLogs", () => { await waitFor(() => expect(hookResult.current).toEqual(logs)); }); + it("Never creates a connection if hook is disabled on mount", () => { + const { publisherResult } = mountHook({ + initialAgentId: MockWorkspaceAgent.id, + enabled: false, + }); + + expect(publisherResult.current).toBe(undefined); + }); + it("Automatically closes the socket connection when the hook is disabled", async () => { - const { publisherResult, rerender } = mountHook(MockWorkspaceAgent.id); - expect(publisherResult.current.isConnectionOpen()).toBe(true); + const { publisherResult, rerender } = mountHook({ + initialAgentId: MockWorkspaceAgent.id, + }); + + expect(publisherResult.current?.isConnectionOpen()).toBe(true); rerender({ agentId: MockWorkspaceAgent.id, enabled: false }); await waitFor(() => { - expect(publisherResult.current.isConnectionOpen()).toBe(false); + expect(publisherResult.current?.isConnectionOpen()).toBe(false); }); }); it("Automatically closes the old connection when the agent ID changes", () => { - const { publisherResult, rerender } = mountHook(MockWorkspaceAgent.id); + const { publisherResult, rerender } = mountHook({ + initialAgentId: MockWorkspaceAgent.id, + }); + const publisher1 = publisherResult.current; - expect(publisher1.isConnectionOpen()).toBe(true); + expect(publisher1?.isConnectionOpen()).toBe(true); const newAgentId = `${MockWorkspaceAgent.id}-2`; rerender({ agentId: newAgentId, enabled: true }); const publisher2 = publisherResult.current; - expect(publisher1.isConnectionOpen()).toBe(false); - expect(publisher2.isConnectionOpen()).toBe(true); + expect(publisher1?.isConnectionOpen()).toBe(false); + expect(publisher2?.isConnectionOpen()).toBe(true); + }); + + it("Calls error callback when error is received (but only while hook is enabled)", async () => { + const onError = jest.fn(); + const { publisherResult, rerender } = mountHook({ + initialAgentId: MockWorkspaceAgent.id, + // Start off disabled so that we can check that the callback is + // never called when there is no connection + enabled: false, + onError, + }); + + const errorEvent = new Event("error"); + act(() => publisherResult.current?.publishError(errorEvent)); + expect(onError).not.toHaveBeenCalled(); + + rerender({ agentId: MockWorkspaceAgent.id, enabled: true }); + act(() => publisherResult.current?.publishError(errorEvent)); + expect(onError).toHaveBeenCalledTimes(1); }); it("Clears logs when hook becomes disabled (protection to avoid duplicate logs when hook goes back to being re-enabled)", async () => { - const { hookResult, publisherResult, rerender } = mountHook( - MockWorkspaceAgent.id, - ); + const { hookResult, publisherResult, rerender } = mountHook({ + initialAgentId: MockWorkspaceAgent.id, + }); // Send initial logs so that we have something to clear out later const initialLogs = generateMockLogs(3, new Date("april 5, 1997")); const initialEvent = new MessageEvent("message", { data: JSON.stringify(initialLogs), }); - act(() => publisherResult.current.publishMessage(initialEvent)); + act(() => publisherResult.current?.publishMessage(initialEvent)); await waitFor(() => expect(hookResult.current).toEqual(initialLogs)); - // Disable the hook (and have the hook close the connection behind the - // scenes) + // Disable the hook rerender({ agentId: MockWorkspaceAgent.id, enabled: false }); await waitFor(() => expect(hookResult.current).toHaveLength(0)); - // Re-enable the hook (creating an entirely new connection), and send - // new logs + // Re-enable the hook and send new logs rerender({ agentId: MockWorkspaceAgent.id, enabled: true }); const newLogs = generateMockLogs(3, new Date("october 3, 2005")); const newEvent = new MessageEvent("message", { data: JSON.stringify(newLogs), }); - act(() => publisherResult.current.publishMessage(newEvent)); + act(() => publisherResult.current?.publishMessage(newEvent)); await waitFor(() => expect(hookResult.current).toEqual(newLogs)); }); }); diff --git a/site/src/modules/resources/useAgentLogs.ts b/site/src/modules/resources/useAgentLogs.ts index aa2da5c247d69..dfe350faf1df0 100644 --- a/site/src/modules/resources/useAgentLogs.ts +++ b/site/src/modules/resources/useAgentLogs.ts @@ -12,7 +12,12 @@ type CreateSocket = ( params?: WatchWorkspaceAgentLogsParams, ) => OneWayWebSocket; -export function createUseAgentLogs(createSocket: CreateSocket) { +export type OnError = (errorEvent: Event) => void; + +export function createUseAgentLogs( + createSocket: CreateSocket, + onError: OnError, +) { return function useAgentLogs( agentId: string, enabled: boolean, @@ -70,20 +75,25 @@ export function createUseAgentLogs(createSocket: CreateSocket) { }); socket.addEventListener("error", (e) => { - console.error("Error in agent log socket: ", e); - displayError( - "Unable to watch agent logs", - "Please try refreshing the browser", - ); + onError(e); socket.close(); }); return () => socket.close(); - }, [createSocket, agentId, enabled]); + }, [createSocket, onError, agentId, enabled]); return logs; }; } // The baseline implementation to use for production -export const useAgentLogs = createUseAgentLogs(watchWorkspaceAgentLogs); +export const useAgentLogs = createUseAgentLogs( + watchWorkspaceAgentLogs, + (errorEvent) => { + console.error("Error in agent log socket: ", errorEvent); + displayError( + "Unable to watch agent logs", + "Please try refreshing the browser", + ); + }, +); diff --git a/site/src/testHelpers/websockets.ts b/site/src/testHelpers/websockets.ts index e4d3b7cb54bf1..5090b47db2c9e 100644 --- a/site/src/testHelpers/websockets.ts +++ b/site/src/testHelpers/websockets.ts @@ -2,7 +2,7 @@ import type { WebSocketEventType } from "utils/OneWayWebSocket"; export type MockWebSocketPublisher = Readonly<{ publishMessage: (event: MessageEvent) => void; - publishError: (event: ErrorEvent) => void; + publishError: (event: Event) => void; publishClose: (event: CloseEvent) => void; publishOpen: (event: Event) => void; isConnectionOpen: () => boolean; @@ -14,7 +14,7 @@ export function createMockWebSocket( ): readonly [WebSocket, MockWebSocketPublisher] { type EventMap = { message: MessageEvent; - error: ErrorEvent; + error: Event; close: CloseEvent; open: Event; }; From 982d3e117fca633766855a1c0b6b1706b3e5b715 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Sat, 2 Aug 2025 21:47:27 +0000 Subject: [PATCH 14/27] fix: remove premature optimization --- site/src/modules/resources/useAgentLogs.ts | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/site/src/modules/resources/useAgentLogs.ts b/site/src/modules/resources/useAgentLogs.ts index dfe350faf1df0..c1ecbb60cfbeb 100644 --- a/site/src/modules/resources/useAgentLogs.ts +++ b/site/src/modules/resources/useAgentLogs.ts @@ -42,7 +42,6 @@ export function createUseAgentLogs( // Always fetch the logs from the beginning. We may want to optimize // this in the future, but it would add some complexity in the code // that might not be worth it. - const createdAtMap = new Map(); const socket = createSocket(agentId, { after: 0 }); socket.addEventListener("message", (e) => { if (e.parseError) { @@ -57,19 +56,10 @@ export function createUseAgentLogs( setLogs((logs) => { const newLogs = [...logs, ...e.parsedMessage]; newLogs.sort((l1, l2) => { - let d1 = createdAtMap.get(l1.created_at); - if (d1 === undefined) { - d1 = new Date(l1.created_at).getTime(); - createdAtMap.set(l1.created_at, d1); - } - let d2 = createdAtMap.get(l2.created_at); - if (d2 === undefined) { - d2 = new Date(l2.created_at).getTime(); - createdAtMap.set(l2.created_at, d2); - } + const d1 = new Date(l1.created_at).getTime(); + const d2 = new Date(l2.created_at).getTime(); return d1 - d2; }); - return newLogs; }); }); From 41c5a124e63886ced4287d4ebe5398b10d04c583 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Mon, 4 Aug 2025 02:43:42 +0000 Subject: [PATCH 15/27] fix: update setup --- .../modules/resources/useAgentLogs.test.ts | 61 +++++++++++-------- site/src/modules/resources/useAgentLogs.ts | 29 +++++---- site/src/testHelpers/websockets.ts | 7 ++- 3 files changed, 54 insertions(+), 43 deletions(-) diff --git a/site/src/modules/resources/useAgentLogs.test.ts b/site/src/modules/resources/useAgentLogs.test.ts index 99ad052f1ced4..6f39684c7e552 100644 --- a/site/src/modules/resources/useAgentLogs.test.ts +++ b/site/src/modules/resources/useAgentLogs.test.ts @@ -7,7 +7,10 @@ import { createMockWebSocket, } from "testHelpers/websockets"; import { OneWayWebSocket } from "utils/OneWayWebSocket"; -import { type OnError, createUseAgentLogs } from "./useAgentLogs"; +import { + type CreateUseAgentLogsOptions, + createUseAgentLogs, +} from "./useAgentLogs"; const millisecondsInOneMinute = 60_000; @@ -30,15 +33,15 @@ function generateMockLogs( }); } -// A mutable object holding the most recent mock WebSocket publisher. Inner -// value will be undefined if the hook is disabled on mount, but will otherwise -// have some kind of value +// A mutable object holding the most recent mock WebSocket publisher that was +// created when initializing a mock WebSocket. Inner value will be undefined if +// the hook is disabled on mount, but will always be defined otherwise type PublisherResult = { current: MockWebSocketPublisher | undefined }; type MountHookOptions = Readonly<{ initialAgentId: string; enabled?: boolean; - onError?: OnError; + onError?: CreateUseAgentLogsOptions["onError"]; }>; type MountHookResult = Readonly<{ @@ -54,20 +57,23 @@ function mountHook(options: MountHookOptions): MountHookResult { const { initialAgentId, enabled = true, onError = jest.fn() } = options; const publisherResult: PublisherResult = { current: undefined }; - const useAgentLogs = createUseAgentLogs((agentId, params) => { - return new OneWayWebSocket({ - apiRoute: `/api/v2/workspaceagents/${agentId}/logs`, - searchParams: new URLSearchParams({ - follow: "true", - after: params?.after?.toString() || "0", - }), - websocketInit: (url) => { - const [mockSocket, mockPublisher] = createMockWebSocket(url); - publisherResult.current = mockPublisher; - return mockSocket; - }, - }); - }, onError); + const useAgentLogs = createUseAgentLogs({ + onError, + createSocket: (agentId, params) => { + return new OneWayWebSocket({ + apiRoute: `/api/v2/workspaceagents/${agentId}/logs`, + searchParams: new URLSearchParams({ + follow: "true", + after: params?.after?.toString() || "0", + }), + websocketInit: (url) => { + const [mockSocket, mockPublisher] = createMockWebSocket(url); + publisherResult.current = mockPublisher; + return mockSocket; + }, + }); + }, + }); const { result, rerender } = renderHook( ({ agentId, enabled }) => useAgentLogs(agentId, enabled), @@ -116,10 +122,10 @@ describe("useAgentLogs", () => { initialAgentId: MockWorkspaceAgent.id, }); - expect(publisherResult.current?.isConnectionOpen()).toBe(true); + expect(publisherResult.current?.isConnectionOpen).toBe(true); rerender({ agentId: MockWorkspaceAgent.id, enabled: false }); await waitFor(() => { - expect(publisherResult.current?.isConnectionOpen()).toBe(false); + expect(publisherResult.current?.isConnectionOpen).toBe(false); }); }); @@ -129,14 +135,17 @@ describe("useAgentLogs", () => { }); const publisher1 = publisherResult.current; - expect(publisher1?.isConnectionOpen()).toBe(true); + expect(publisher1?.isConnectionOpen).toBe(true); - const newAgentId = `${MockWorkspaceAgent.id}-2`; - rerender({ agentId: newAgentId, enabled: true }); + rerender({ + enabled: true, + agentId: `${MockWorkspaceAgent.id}-new-value`, + }); const publisher2 = publisherResult.current; - expect(publisher1?.isConnectionOpen()).toBe(false); - expect(publisher2?.isConnectionOpen()).toBe(true); + expect(publisher1).not.toBe(publisher2); + expect(publisher1?.isConnectionOpen).toBe(false); + expect(publisher2?.isConnectionOpen).toBe(true); }); it("Calls error callback when error is received (but only while hook is enabled)", async () => { diff --git a/site/src/modules/resources/useAgentLogs.ts b/site/src/modules/resources/useAgentLogs.ts index c1ecbb60cfbeb..61360b2243d43 100644 --- a/site/src/modules/resources/useAgentLogs.ts +++ b/site/src/modules/resources/useAgentLogs.ts @@ -7,17 +7,16 @@ import { displayError } from "components/GlobalSnackbar/utils"; import { useEffect, useState } from "react"; import type { OneWayWebSocket } from "utils/OneWayWebSocket"; -type CreateSocket = ( - agentId: string, - params?: WatchWorkspaceAgentLogsParams, -) => OneWayWebSocket; - -export type OnError = (errorEvent: Event) => void; +export type CreateUseAgentLogsOptions = Readonly<{ + onError: (errorEvent: Event) => void; + createSocket: ( + agentId: string, + params?: WatchWorkspaceAgentLogsParams, + ) => OneWayWebSocket; +}>; -export function createUseAgentLogs( - createSocket: CreateSocket, - onError: OnError, -) { +export function createUseAgentLogs(options: CreateUseAgentLogsOptions) { + const { createSocket, onError } = options; return function useAgentLogs( agentId: string, enabled: boolean, @@ -70,20 +69,20 @@ export function createUseAgentLogs( }); return () => socket.close(); - }, [createSocket, onError, agentId, enabled]); + }, [agentId, enabled]); return logs; }; } // The baseline implementation to use for production -export const useAgentLogs = createUseAgentLogs( - watchWorkspaceAgentLogs, - (errorEvent) => { +export const useAgentLogs = createUseAgentLogs({ + createSocket: watchWorkspaceAgentLogs, + onError: (errorEvent) => { console.error("Error in agent log socket: ", errorEvent); displayError( "Unable to watch agent logs", "Please try refreshing the browser", ); }, -); +}); diff --git a/site/src/testHelpers/websockets.ts b/site/src/testHelpers/websockets.ts index 5090b47db2c9e..560f3e09617cd 100644 --- a/site/src/testHelpers/websockets.ts +++ b/site/src/testHelpers/websockets.ts @@ -5,7 +5,7 @@ export type MockWebSocketPublisher = Readonly<{ publishError: (event: Event) => void; publishClose: (event: CloseEvent) => void; publishOpen: (event: Event) => void; - isConnectionOpen: () => boolean; + readonly isConnectionOpen: boolean; }>; export function createMockWebSocket( @@ -95,7 +95,10 @@ export function createMockWebSocket( }; const publisher: MockWebSocketPublisher = { - isConnectionOpen: () => isOpen, + get isConnectionOpen() { + return isOpen; + }, + publishOpen: (event) => { if (!isOpen) { return; From 42cb73b7de2c91c3676b2bf9f2a7826e1c701f34 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Mon, 4 Aug 2025 02:50:11 +0000 Subject: [PATCH 16/27] fix: update state sync logic --- .../modules/resources/useAgentLogs.test.ts | 29 +++++++++++-------- site/src/modules/resources/useAgentLogs.ts | 3 ++ 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/site/src/modules/resources/useAgentLogs.test.ts b/site/src/modules/resources/useAgentLogs.test.ts index 6f39684c7e552..68b98b7623274 100644 --- a/site/src/modules/resources/useAgentLogs.test.ts +++ b/site/src/modules/resources/useAgentLogs.test.ts @@ -180,17 +180,22 @@ describe("useAgentLogs", () => { act(() => publisherResult.current?.publishMessage(initialEvent)); await waitFor(() => expect(hookResult.current).toEqual(initialLogs)); - // Disable the hook - rerender({ agentId: MockWorkspaceAgent.id, enabled: false }); - await waitFor(() => expect(hookResult.current).toHaveLength(0)); - - // Re-enable the hook and send new logs - rerender({ agentId: MockWorkspaceAgent.id, enabled: true }); - const newLogs = generateMockLogs(3, new Date("october 3, 2005")); - const newEvent = new MessageEvent("message", { - data: JSON.stringify(newLogs), - }); - act(() => publisherResult.current?.publishMessage(newEvent)); - await waitFor(() => expect(hookResult.current).toEqual(newLogs)); + // Need to do the following steps multiple times to make sure that we + // don't break anything after the first disable + const mockDates: readonly string[] = ["october 3, 2005", "august 1, 2025"]; + for (const md of mockDates) { + // Disable the hook to clear current logs + rerender({ agentId: MockWorkspaceAgent.id, enabled: false }); + await waitFor(() => expect(hookResult.current).toHaveLength(0)); + + // Re-enable the hook and send new logs + rerender({ agentId: MockWorkspaceAgent.id, enabled: true }); + const newLogs = generateMockLogs(3, new Date(md)); + const newEvent = new MessageEvent("message", { + data: JSON.stringify(newLogs), + }); + act(() => publisherResult.current?.publishMessage(newEvent)); + await waitFor(() => expect(hookResult.current).toEqual(newLogs)); + } }); }); diff --git a/site/src/modules/resources/useAgentLogs.ts b/site/src/modules/resources/useAgentLogs.ts index 61360b2243d43..f4d39638214e7 100644 --- a/site/src/modules/resources/useAgentLogs.ts +++ b/site/src/modules/resources/useAgentLogs.ts @@ -32,6 +32,9 @@ export function createUseAgentLogs(options: CreateUseAgentLogsOptions) { setLogs([]); setPrevEnabled(false); } + if (enabled && !prevEnabled) { + setPrevEnabled(true); + } useEffect(() => { if (!enabled) { From 35a40df30a7ab35dae6278b21713ae8a0f46b4e7 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Mon, 4 Aug 2025 18:09:02 +0000 Subject: [PATCH 17/27] fix: update wonky types --- site/src/testHelpers/websockets.ts | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/site/src/testHelpers/websockets.ts b/site/src/testHelpers/websockets.ts index 560f3e09617cd..418326f20a6fd 100644 --- a/site/src/testHelpers/websockets.ts +++ b/site/src/testHelpers/websockets.ts @@ -60,32 +60,30 @@ export function createMockWebSocket( addEventListener: ( eventType: E, - callback: WebSocketEventMap[E], + callback: (event: WebSocketEventMap[E]) => void, ) => { if (!isOpen) { return; } const subscribers = store[eventType]; - const cb = callback as unknown as CallbackStore[E][0]; - if (!subscribers.includes(cb)) { - subscribers.push(cb); + if (!subscribers.includes(callback)) { + subscribers.push(callback); } }, removeEventListener: ( eventType: E, - callback: WebSocketEventMap[E], + callback: (event: WebSocketEventMap[E]) => void, ) => { if (!isOpen) { return; } const subscribers = store[eventType]; - const cb = callback as unknown as CallbackStore[E][0]; - if (subscribers.includes(cb)) { - const updated = store[eventType].filter((c) => c !== cb); - store[eventType] = updated as unknown as CallbackStore[E]; + if (subscribers.includes(callback)) { + const updated = store[eventType].filter((c) => c !== callback); + store[eventType] = updated as CallbackStore[E]; } }, From c2fc7722f0b7fade6816efef09f68bb34093d858 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Thu, 7 Aug 2025 21:08:07 +0000 Subject: [PATCH 18/27] fix: update tests --- .../modules/resources/useAgentLogs.test.ts | 72 +++++++++---------- 1 file changed, 33 insertions(+), 39 deletions(-) diff --git a/site/src/modules/resources/useAgentLogs.test.ts b/site/src/modules/resources/useAgentLogs.test.ts index 68b98b7623274..c93addc4ca65e 100644 --- a/site/src/modules/resources/useAgentLogs.test.ts +++ b/site/src/modules/resources/useAgentLogs.test.ts @@ -3,14 +3,14 @@ import type { WorkspaceAgentLog } from "api/typesGenerated"; import { act } from "react"; import { MockWorkspaceAgent } from "testHelpers/entities"; import { - type MockWebSocketPublisher, + type MockWebSocketServer, createMockWebSocket, } from "testHelpers/websockets"; import { OneWayWebSocket } from "utils/OneWayWebSocket"; import { type CreateUseAgentLogsOptions, createUseAgentLogs, -} from "./useAgentLogs"; +} from "./useAgentLogs" const millisecondsInOneMinute = 60_000; @@ -33,10 +33,10 @@ function generateMockLogs( }); } -// A mutable object holding the most recent mock WebSocket publisher that was +// A mutable object holding the most recent mock WebSocket server that was // created when initializing a mock WebSocket. Inner value will be undefined if // the hook is disabled on mount, but will always be defined otherwise -type PublisherResult = { current: MockWebSocketPublisher | undefined }; +type ServerResult = { current: MockWebSocketServer | undefined }; type MountHookOptions = Readonly<{ initialAgentId: string; @@ -46,7 +46,7 @@ type MountHookOptions = Readonly<{ type MountHookResult = Readonly<{ rerender: (props: { agentId: string; enabled: boolean }) => void; - publisherResult: PublisherResult; + serverResult: ServerResult; // Note: the `current` property is only "halfway" readonly; the value is // readonly, but the key is still mutable @@ -56,7 +56,7 @@ type MountHookResult = Readonly<{ function mountHook(options: MountHookOptions): MountHookResult { const { initialAgentId, enabled = true, onError = jest.fn() } = options; - const publisherResult: PublisherResult = { current: undefined }; + const serverResult: ServerResult = { current: undefined }; const useAgentLogs = createUseAgentLogs({ onError, createSocket: (agentId, params) => { @@ -67,29 +67,25 @@ function mountHook(options: MountHookOptions): MountHookResult { after: params?.after?.toString() || "0", }), websocketInit: (url) => { - const [mockSocket, mockPublisher] = createMockWebSocket(url); - publisherResult.current = mockPublisher; + const [mockSocket, mockServer] = createMockWebSocket(url); + serverResult.current = mockServer; return mockSocket; }, }); }, }); - const { result, rerender } = renderHook( + const { result: hookResult, rerender } = renderHook( ({ agentId, enabled }) => useAgentLogs(agentId, enabled), { initialProps: { agentId: initialAgentId, enabled: enabled } }, ); - return { - rerender, - hookResult: result, - publisherResult, - }; + return { rerender, serverResult, hookResult }; } describe("useAgentLogs", () => { it("Automatically sorts logs that are received out of order", async () => { - const { hookResult, publisherResult } = mountHook({ + const { hookResult, serverResult } = mountHook({ initialAgentId: MockWorkspaceAgent.id, }); @@ -98,10 +94,8 @@ describe("useAgentLogs", () => { for (const log of reversed) { act(() => { - publisherResult.current?.publishMessage( - new MessageEvent("message", { - data: JSON.stringify([log]), - }), + serverResult.current?.publishMessage( + new MessageEvent("message", { data: JSON.stringify([log]) }), ); }); } @@ -109,48 +103,48 @@ describe("useAgentLogs", () => { }); it("Never creates a connection if hook is disabled on mount", () => { - const { publisherResult } = mountHook({ + const { serverResult } = mountHook({ initialAgentId: MockWorkspaceAgent.id, enabled: false, }); - expect(publisherResult.current).toBe(undefined); + expect(serverResult.current).toBe(undefined); }); it("Automatically closes the socket connection when the hook is disabled", async () => { - const { publisherResult, rerender } = mountHook({ + const { serverResult, rerender } = mountHook({ initialAgentId: MockWorkspaceAgent.id, }); - expect(publisherResult.current?.isConnectionOpen).toBe(true); + expect(serverResult.current?.isConnectionOpen).toBe(true); rerender({ agentId: MockWorkspaceAgent.id, enabled: false }); await waitFor(() => { - expect(publisherResult.current?.isConnectionOpen).toBe(false); + expect(serverResult.current?.isConnectionOpen).toBe(false); }); }); it("Automatically closes the old connection when the agent ID changes", () => { - const { publisherResult, rerender } = mountHook({ + const { serverResult, rerender } = mountHook({ initialAgentId: MockWorkspaceAgent.id, }); - const publisher1 = publisherResult.current; - expect(publisher1?.isConnectionOpen).toBe(true); + const serverConn1 = serverResult.current; + expect(serverConn1?.isConnectionOpen).toBe(true); rerender({ enabled: true, agentId: `${MockWorkspaceAgent.id}-new-value`, }); - const publisher2 = publisherResult.current; - expect(publisher1).not.toBe(publisher2); - expect(publisher1?.isConnectionOpen).toBe(false); - expect(publisher2?.isConnectionOpen).toBe(true); + const serverConn2 = serverResult.current; + expect(serverConn1).not.toBe(serverConn2); + expect(serverConn1?.isConnectionOpen).toBe(false); + expect(serverConn2?.isConnectionOpen).toBe(true); }); it("Calls error callback when error is received (but only while hook is enabled)", async () => { const onError = jest.fn(); - const { publisherResult, rerender } = mountHook({ + const { serverResult, rerender } = mountHook({ initialAgentId: MockWorkspaceAgent.id, // Start off disabled so that we can check that the callback is // never called when there is no connection @@ -159,25 +153,25 @@ describe("useAgentLogs", () => { }); const errorEvent = new Event("error"); - act(() => publisherResult.current?.publishError(errorEvent)); + act(() => serverResult.current?.publishError(errorEvent)); expect(onError).not.toHaveBeenCalled(); rerender({ agentId: MockWorkspaceAgent.id, enabled: true }); - act(() => publisherResult.current?.publishError(errorEvent)); + act(() => serverResult.current?.publishError(errorEvent)); expect(onError).toHaveBeenCalledTimes(1); }); it("Clears logs when hook becomes disabled (protection to avoid duplicate logs when hook goes back to being re-enabled)", async () => { - const { hookResult, publisherResult, rerender } = mountHook({ + const { hookResult, serverResult, rerender } = mountHook({ initialAgentId: MockWorkspaceAgent.id, }); // Send initial logs so that we have something to clear out later const initialLogs = generateMockLogs(3, new Date("april 5, 1997")); - const initialEvent = new MessageEvent("message", { + const initialEvent = new MessageEvent("message", { data: JSON.stringify(initialLogs), }); - act(() => publisherResult.current?.publishMessage(initialEvent)); + act(() => serverResult.current?.publishMessage(initialEvent)); await waitFor(() => expect(hookResult.current).toEqual(initialLogs)); // Need to do the following steps multiple times to make sure that we @@ -191,10 +185,10 @@ describe("useAgentLogs", () => { // Re-enable the hook and send new logs rerender({ agentId: MockWorkspaceAgent.id, enabled: true }); const newLogs = generateMockLogs(3, new Date(md)); - const newEvent = new MessageEvent("message", { + const newEvent = new MessageEvent("message", { data: JSON.stringify(newLogs), }); - act(() => publisherResult.current?.publishMessage(newEvent)); + act(() => serverResult.current?.publishMessage(newEvent)); await waitFor(() => expect(hookResult.current).toEqual(newLogs)); } }); From 2cabd851caefa6101a63030c3c70b9594a52f84c Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Thu, 7 Aug 2025 21:14:32 +0000 Subject: [PATCH 19/27] fix: format --- site/src/modules/resources/useAgentLogs.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/modules/resources/useAgentLogs.test.ts b/site/src/modules/resources/useAgentLogs.test.ts index c93addc4ca65e..69e915cbb7553 100644 --- a/site/src/modules/resources/useAgentLogs.test.ts +++ b/site/src/modules/resources/useAgentLogs.test.ts @@ -10,7 +10,7 @@ import { OneWayWebSocket } from "utils/OneWayWebSocket"; import { type CreateUseAgentLogsOptions, createUseAgentLogs, -} from "./useAgentLogs" +} from "./useAgentLogs"; const millisecondsInOneMinute = 60_000; From 453894b8117322687236cda165caa645b6ef8bad Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Sat, 9 Aug 2025 05:36:05 +0000 Subject: [PATCH 20/27] fix: apply initial feedback --- site/src/modules/resources/AgentRow.tsx | 4 ++-- .../src/modules/resources/useAgentLogs.test.ts | 18 ++++++++++-------- site/src/modules/resources/useAgentLogs.ts | 14 ++++++++++---- .../WorkspaceBuildPageView.tsx | 2 +- 4 files changed, 23 insertions(+), 15 deletions(-) diff --git a/site/src/modules/resources/AgentRow.tsx b/site/src/modules/resources/AgentRow.tsx index b540fc65f841a..826016746794f 100644 --- a/site/src/modules/resources/AgentRow.tsx +++ b/site/src/modules/resources/AgentRow.tsx @@ -77,7 +77,7 @@ export const AgentRow: FC = ({ ["starting", "start_timeout"].includes(agent.lifecycle_state) && hasStartupFeatures, ); - const agentLogs = useAgentLogs(agent.id, showLogs); + const agentLogs = useAgentLogs({ agentId: agent.id, enabled: showLogs }); const logListRef = useRef(null); const logListDivRef = useRef(null); const startupLogs = useMemo(() => { @@ -91,7 +91,7 @@ export const AgentRow: FC = ({ level: "error", output: "Startup logs exceeded the max size of 1MB, and will not continue to be written to the database! Logs will continue to be written to the /tmp/coder-startup-script.log file in the workspace.", - created_at: new Date().toISOString(), + created_at: agentLogs.at(-1)?.created_at || new Date().toISOString(), source_id: "", }, ]; diff --git a/site/src/modules/resources/useAgentLogs.test.ts b/site/src/modules/resources/useAgentLogs.test.ts index 69e915cbb7553..4d9a04d01d814 100644 --- a/site/src/modules/resources/useAgentLogs.test.ts +++ b/site/src/modules/resources/useAgentLogs.test.ts @@ -16,7 +16,7 @@ const millisecondsInOneMinute = 60_000; function generateMockLogs( logCount: number, - baseDate = new Date(), + baseDate = new Date("April 1, 1970"), ): readonly WorkspaceAgentLog[] { return Array.from({ length: logCount }, (_, i) => { // Make sure that the logs generated each have unique timestamps, so @@ -76,7 +76,7 @@ function mountHook(options: MountHookOptions): MountHookResult { }); const { result: hookResult, rerender } = renderHook( - ({ agentId, enabled }) => useAgentLogs(agentId, enabled), + (props) => useAgentLogs(props), { initialProps: { agentId: initialAgentId, enabled: enabled } }, ); @@ -93,7 +93,7 @@ describe("useAgentLogs", () => { const reversed = logs.toReversed(); for (const log of reversed) { - act(() => { + await act(async () => { serverResult.current?.publishMessage( new MessageEvent("message", { data: JSON.stringify([log]) }), ); @@ -153,15 +153,17 @@ describe("useAgentLogs", () => { }); const errorEvent = new Event("error"); - act(() => serverResult.current?.publishError(errorEvent)); + await act(async () => serverResult.current?.publishError(errorEvent)); expect(onError).not.toHaveBeenCalled(); rerender({ agentId: MockWorkspaceAgent.id, enabled: true }); - act(() => serverResult.current?.publishError(errorEvent)); + await act(async () => serverResult.current?.publishError(errorEvent)); expect(onError).toHaveBeenCalledTimes(1); }); - it("Clears logs when hook becomes disabled (protection to avoid duplicate logs when hook goes back to being re-enabled)", async () => { + // This is a protection to avoid duplicate logs when the hook goes back to + // being re-enabled + it("Clears logs when hook becomes disabled", async () => { const { hookResult, serverResult, rerender } = mountHook({ initialAgentId: MockWorkspaceAgent.id, }); @@ -171,7 +173,7 @@ describe("useAgentLogs", () => { const initialEvent = new MessageEvent("message", { data: JSON.stringify(initialLogs), }); - act(() => serverResult.current?.publishMessage(initialEvent)); + await act(async () => serverResult.current?.publishMessage(initialEvent)); await waitFor(() => expect(hookResult.current).toEqual(initialLogs)); // Need to do the following steps multiple times to make sure that we @@ -188,7 +190,7 @@ describe("useAgentLogs", () => { const newEvent = new MessageEvent("message", { data: JSON.stringify(newLogs), }); - act(() => serverResult.current?.publishMessage(newEvent)); + await act(async () => serverResult.current?.publishMessage(newEvent)); await waitFor(() => expect(hookResult.current).toEqual(newLogs)); } }); diff --git a/site/src/modules/resources/useAgentLogs.ts b/site/src/modules/resources/useAgentLogs.ts index f4d39638214e7..34e32129fd25e 100644 --- a/site/src/modules/resources/useAgentLogs.ts +++ b/site/src/modules/resources/useAgentLogs.ts @@ -15,12 +15,18 @@ export type CreateUseAgentLogsOptions = Readonly<{ ) => OneWayWebSocket; }>; -export function createUseAgentLogs(options: CreateUseAgentLogsOptions) { - const { createSocket, onError } = options; +type UseAgentLogsOptions = Readonly<{ + agentId: string; + enabled?: boolean; +}>; + +export function createUseAgentLogs(createOptions: CreateUseAgentLogsOptions) { + const { createSocket, onError } = createOptions; + return function useAgentLogs( - agentId: string, - enabled: boolean, + options: UseAgentLogsOptions, ): readonly WorkspaceAgentLog[] { + const { agentId, enabled = true } = options; const [logs, setLogs] = useState([]); // Clean up the logs when the agent is not enabled, using a mid-render diff --git a/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx b/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx index bcd451736d424..122d0a3ca4258 100644 --- a/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx +++ b/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx @@ -308,7 +308,7 @@ type AgentLogsContentProps = { }; const AgentLogsContent: FC = ({ agent }) => { - const logs = useAgentLogs(agent.id, true); + const logs = useAgentLogs({ agentId: agent.id }); return ( Date: Sat, 9 Aug 2025 06:24:33 +0000 Subject: [PATCH 21/27] wip: commit refactoring progress --- .../modules/resources/AgentLogs/AgentLogs.tsx | 67 +++++++++++-------- site/src/modules/resources/AgentRow.tsx | 24 ++----- .../WorkspaceBuildPageView.tsx | 7 +- 3 files changed, 47 insertions(+), 51 deletions(-) diff --git a/site/src/modules/resources/AgentLogs/AgentLogs.tsx b/site/src/modules/resources/AgentLogs/AgentLogs.tsx index e46dabdb7ca83..f71cf6e5327cc 100644 --- a/site/src/modules/resources/AgentLogs/AgentLogs.tsx +++ b/site/src/modules/resources/AgentLogs/AgentLogs.tsx @@ -2,27 +2,52 @@ import type { Interpolation, Theme } from "@emotion/react"; import Tooltip from "@mui/material/Tooltip"; import type { WorkspaceAgentLogSource } from "api/typesGenerated"; import type { Line } from "components/Logs/LogLine"; -import { type ComponentProps, forwardRef, useMemo } from "react"; +import { type ComponentProps, forwardRef, ReactNode } from "react"; import { FixedSizeList as List } from "react-window"; import { AGENT_LOG_LINE_HEIGHT, AgentLogLine } from "./AgentLogLine"; +const fallbackLog: WorkspaceAgentLogSource = { + created_at: "", + display_name: "Logs", + icon: "", + id: "00000000-0000-0000-0000-000000000000", + workspace_agent_id: "", +}; + +function groupLogSourcesById(sources: readonly WorkspaceAgentLogSource[]): Record { + const sourcesById: Record = {}; + for (const source of sources) { + sourcesById[source.id] = source; + } + return sourcesById; +} + +const message: ReactNode = ( +

+ Startup logs exceeded the max size of 1MB, and will not continue to be + written to the database! Logs will continue to be written to the + /tmp/coder-startup-script.log file in the workspace. +

+) + type AgentLogsProps = Omit< ComponentProps, "children" | "itemSize" | "itemCount" > & { logs: readonly Line[]; sources: readonly WorkspaceAgentLogSource[]; + overflowed: boolean; }; export const AgentLogs = forwardRef( ({ logs, sources, ...listProps }, ref) => { - const logSourceByID = useMemo(() => { - const sourcesById: { [id: string]: WorkspaceAgentLogSource } = {}; - for (const source of sources) { - sourcesById[source.id] = source; - } - return sourcesById; - }, [sources]); + // getLogSource always returns a valid log source. We need + // this to support deployments that were made before + // `coder_script` was created and that haven't updated yet + const logSourceByID = groupLogSourcesById(sources); + const getLogSource = (id: string): WorkspaceAgentLogSource => { + return logSourceByID[id] || fallbackLog; + }; return ( ( > {({ index, style }) => { const log = logs[index]; - // getLogSource always returns a valid log source. - // This is necessary to support deployments before `coder_script`. - // Existed that haven't restarted their agents. - const getLogSource = (id: string): WorkspaceAgentLogSource => { - return ( - logSourceByID[id] || { - created_at: "", - display_name: "Logs", - icon: "", - id: "00000000-0000-0000-0000-000000000000", - workspace_agent_id: "", - } - ); - }; const logSource = getLogSource(log.sourceId); let assignedIcon = false; @@ -85,9 +96,9 @@ export const AgentLogs = forwardRef( assignedIcon = true; } - let nextChangesSource = false; + let doesNextLineHaveDifferentSource = false; if (index < logs.length - 1) { - nextChangesSource = + doesNextLineHaveDifferentSource = getLogSource(logs[index + 1].sourceId).id !== log.sourceId; } // We don't want every line to repeat the icon, because @@ -114,13 +125,13 @@ export const AgentLogs = forwardRef(
({ - height: nextChangesSource ? "50%" : "100%", + height: doesNextLineHaveDifferentSource ? "50%" : "100%", width: 2, background: theme.experimental.l1.outline, borderRadius: 2, })} /> - {nextChangesSource && ( + {doesNextLineHaveDifferentSource && (
({ @@ -140,7 +151,7 @@ export const AgentLogs = forwardRef( return ( ( // These colors were picked at random. Feel free // to add more, adjust, or change! Users will not // depend on these colors. -const scriptDisplayColors = [ +const scriptDisplayColors: readonly string[] = [ "#85A3B2", "#A37EB2", "#C29FDE", diff --git a/site/src/modules/resources/AgentRow.tsx b/site/src/modules/resources/AgentRow.tsx index 826016746794f..09851d1d8f9cf 100644 --- a/site/src/modules/resources/AgentRow.tsx +++ b/site/src/modules/resources/AgentRow.tsx @@ -6,7 +6,6 @@ import type { Template, Workspace, WorkspaceAgent, - WorkspaceAgentLog, WorkspaceAgentMetadata, } from "api/typesGenerated"; import { Button } from "components/Button/Button"; @@ -80,22 +79,6 @@ export const AgentRow: FC = ({ const agentLogs = useAgentLogs({ agentId: agent.id, enabled: showLogs }); const logListRef = useRef(null); const logListDivRef = useRef(null); - const startupLogs = useMemo(() => { - if (!agent.logs_overflowed) { - return agentLogs; - } - return [ - ...agentLogs, - { - id: -1, - level: "error", - output: - "Startup logs exceeded the max size of 1MB, and will not continue to be written to the database! Logs will continue to be written to the /tmp/coder-startup-script.log file in the workspace.", - created_at: agentLogs.at(-1)?.created_at || new Date().toISOString(), - source_id: "", - }, - ]; - }, [agentLogs, agent.logs_overflowed]); const [bottomOfLogs, setBottomOfLogs] = useState(true); useEffect(() => { @@ -107,9 +90,9 @@ export const AgentRow: FC = ({ useLayoutEffect(() => { // If we're currently watching the bottom, we always want to stay at the bottom. if (bottomOfLogs && logListRef.current) { - logListRef.current.scrollToItem(startupLogs.length - 1, "end"); + logListRef.current.scrollToItem(agentLogs.length - 1, "end"); } - }, [showLogs, startupLogs, bottomOfLogs]); + }, [showLogs, agentLogs, bottomOfLogs]); // This is a bit of a hack on the react-window API to get the scroll position. // If we're scrolled to the bottom, we want to keep the list scrolled to the bottom. @@ -313,7 +296,8 @@ export const AgentRow: FC = ({ width={width} css={styles.startupLogs} onScroll={handleLogScroll} - logs={startupLogs.map((l) => ({ + overflowed={agent.logs_overflowed} + logs={agentLogs.map((l) => ({ id: l.id, level: l.level, output: l.output, diff --git a/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx b/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx index 122d0a3ca4258..60f618ee0bc4f 100644 --- a/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx +++ b/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx @@ -311,16 +311,17 @@ const AgentLogsContent: FC = ({ agent }) => { const logs = useAgentLogs({ agentId: agent.id }); return ( ((l) => ({ + height={560} + width="100%" + logs={logs.map((l) => ({ id: l.id, output: l.output, time: l.created_at, level: l.level, sourceId: l.source_id, }))} - height={560} - width="100%" /> ); }; From 80865fe01f5889e1749d0d6e49a20111f8d90e1a Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Sat, 9 Aug 2025 06:27:18 +0000 Subject: [PATCH 22/27] refactor: update assignment --- .../modules/resources/AgentLogs/AgentLogs.tsx | 23 ++++++++++--------- site/src/modules/resources/AgentRow.tsx | 1 - .../WorkspaceBuildPageView.tsx | 1 - 3 files changed, 12 insertions(+), 13 deletions(-) diff --git a/site/src/modules/resources/AgentLogs/AgentLogs.tsx b/site/src/modules/resources/AgentLogs/AgentLogs.tsx index f71cf6e5327cc..4aff15a5006fc 100644 --- a/site/src/modules/resources/AgentLogs/AgentLogs.tsx +++ b/site/src/modules/resources/AgentLogs/AgentLogs.tsx @@ -2,7 +2,7 @@ import type { Interpolation, Theme } from "@emotion/react"; import Tooltip from "@mui/material/Tooltip"; import type { WorkspaceAgentLogSource } from "api/typesGenerated"; import type { Line } from "components/Logs/LogLine"; -import { type ComponentProps, forwardRef, ReactNode } from "react"; +import { type ComponentProps, type ReactNode, forwardRef } from "react"; import { FixedSizeList as List } from "react-window"; import { AGENT_LOG_LINE_HEIGHT, AgentLogLine } from "./AgentLogLine"; @@ -14,7 +14,9 @@ const fallbackLog: WorkspaceAgentLogSource = { workspace_agent_id: "", }; -function groupLogSourcesById(sources: readonly WorkspaceAgentLogSource[]): Record { +function groupLogSourcesById( + sources: readonly WorkspaceAgentLogSource[], +): Record { const sourcesById: Record = {}; for (const source of sources) { sourcesById[source.id] = source; @@ -28,7 +30,7 @@ const message: ReactNode = ( written to the database! Logs will continue to be written to the /tmp/coder-startup-script.log file in the workspace.

-) +); type AgentLogsProps = Omit< ComponentProps, @@ -96,20 +98,19 @@ export const AgentLogs = forwardRef( assignedIcon = true; } - let doesNextLineHaveDifferentSource = false; - if (index < logs.length - 1) { - doesNextLineHaveDifferentSource = - getLogSource(logs[index + 1].sourceId).id !== log.sourceId; - } + const doesNextLineHaveDifferentSource = + index < logs.length - 1 && + getLogSource(logs[index + 1].sourceId).id !== log.sourceId; + // We don't want every line to repeat the icon, because // that is ugly and repetitive. This removes the icon // for subsequent lines of the same source and shows a // line instead, visually indicating they are from the // same source. - if ( + const shouldHideSource = index > 0 && - getLogSource(logs[index - 1].sourceId).id === log.sourceId - ) { + getLogSource(logs[index - 1].sourceId).id === log.sourceId; + if (shouldHideSource) { icon = (
Date: Sat, 9 Aug 2025 06:32:57 +0000 Subject: [PATCH 23/27] wip: prepare to change indents --- .../modules/resources/AgentLogs/AgentLogs.tsx | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/site/src/modules/resources/AgentLogs/AgentLogs.tsx b/site/src/modules/resources/AgentLogs/AgentLogs.tsx index 4aff15a5006fc..5754b69af23a2 100644 --- a/site/src/modules/resources/AgentLogs/AgentLogs.tsx +++ b/site/src/modules/resources/AgentLogs/AgentLogs.tsx @@ -26,9 +26,11 @@ function groupLogSourcesById( const message: ReactNode = (

- Startup logs exceeded the max size of 1MB, and will not continue to be - written to the database! Logs will continue to be written to the - /tmp/coder-startup-script.log file in the workspace. + Startup logs exceeded the max size of{" "} + 1MB, and will not continue + to be written to the database! Logs will continue to be written to the + /tmp/coder-startup-script.log file in the + workspace.

); @@ -43,13 +45,11 @@ type AgentLogsProps = Omit< export const AgentLogs = forwardRef( ({ logs, sources, ...listProps }, ref) => { - // getLogSource always returns a valid log source. We need - // this to support deployments that were made before - // `coder_script` was created and that haven't updated yet + // getLogSource must always returns a valid log source. We need this to + // support deployments that were made before `coder_script` was created + // and that haven't updated to a newer Coder version yet const logSourceByID = groupLogSourcesById(sources); - const getLogSource = (id: string): WorkspaceAgentLogSource => { - return logSourceByID[id] || fallbackLog; - }; + const getLogSource = (id: string) => logSourceByID[id] || fallbackLog; return ( Date: Sat, 9 Aug 2025 06:48:00 +0000 Subject: [PATCH 24/27] fix: update keygen logic --- site/src/modules/resources/AgentLogs/AgentLogs.tsx | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/site/src/modules/resources/AgentLogs/AgentLogs.tsx b/site/src/modules/resources/AgentLogs/AgentLogs.tsx index 5754b69af23a2..85b15644cdc15 100644 --- a/site/src/modules/resources/AgentLogs/AgentLogs.tsx +++ b/site/src/modules/resources/AgentLogs/AgentLogs.tsx @@ -36,7 +36,7 @@ const message: ReactNode = ( type AgentLogsProps = Omit< ComponentProps, - "children" | "itemSize" | "itemCount" + "children" | "itemSize" | "itemCount" | "itemKey" > & { logs: readonly Line[]; sources: readonly WorkspaceAgentLogSource[]; @@ -44,7 +44,7 @@ type AgentLogsProps = Omit< }; export const AgentLogs = forwardRef( - ({ logs, sources, ...listProps }, ref) => { + ({ logs, sources, overflowed, ...listProps }, ref) => { // getLogSource must always returns a valid log source. We need this to // support deployments that were made before `coder_script` was created // and that haven't updated to a newer Coder version yet @@ -53,11 +53,12 @@ export const AgentLogs = forwardRef( return ( logs[index].id || 0} > {({ index, style }) => { const log = logs[index]; From 565753655fa6924fe4d878433f4fe4b54f213220 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Sat, 9 Aug 2025 06:50:53 +0000 Subject: [PATCH 25/27] chore: add basic overflow message --- .../modules/resources/AgentLogs/AgentLogs.tsx | 251 +++++++++--------- 1 file changed, 127 insertions(+), 124 deletions(-) diff --git a/site/src/modules/resources/AgentLogs/AgentLogs.tsx b/site/src/modules/resources/AgentLogs/AgentLogs.tsx index 85b15644cdc15..654a7dce96d86 100644 --- a/site/src/modules/resources/AgentLogs/AgentLogs.tsx +++ b/site/src/modules/resources/AgentLogs/AgentLogs.tsx @@ -2,7 +2,7 @@ import type { Interpolation, Theme } from "@emotion/react"; import Tooltip from "@mui/material/Tooltip"; import type { WorkspaceAgentLogSource } from "api/typesGenerated"; import type { Line } from "components/Logs/LogLine"; -import { type ComponentProps, type ReactNode, forwardRef } from "react"; +import { type ComponentProps, forwardRef } from "react"; import { FixedSizeList as List } from "react-window"; import { AGENT_LOG_LINE_HEIGHT, AgentLogLine } from "./AgentLogLine"; @@ -24,16 +24,6 @@ function groupLogSourcesById( return sourcesById; } -const message: ReactNode = ( -

- Startup logs exceeded the max size of{" "} - 1MB, and will not continue - to be written to the database! Logs will continue to be written to the - /tmp/coder-startup-script.log file in the - workspace. -

-); - type AgentLogsProps = Omit< ComponentProps, "children" | "itemSize" | "itemCount" | "itemKey" @@ -52,132 +42,145 @@ export const AgentLogs = forwardRef( const getLogSource = (id: string) => logSourceByID[id] || fallbackLog; return ( - logs[index].id || 0} - > - {({ index, style }) => { - const log = logs[index]; - const logSource = getLogSource(log.sourceId); +
+ logs[index].id || 0} + > + {({ index, style }) => { + const log = logs[index]; + const logSource = getLogSource(log.sourceId); - let assignedIcon = false; - let icon: JSX.Element; - // If no icon is specified, we show a deterministic - // colored circle to identify unique scripts. - if (logSource.icon) { - icon = ( - - ); - } else { - icon = ( -
- ); - assignedIcon = true; - } + let assignedIcon = false; + let icon: JSX.Element; + // If no icon is specified, we show a deterministic + // colored circle to identify unique scripts. + if (logSource.icon) { + icon = ( + + ); + } else { + icon = ( +
+ ); + assignedIcon = true; + } - const doesNextLineHaveDifferentSource = - index < logs.length - 1 && - getLogSource(logs[index + 1].sourceId).id !== log.sourceId; + const doesNextLineHaveDifferentSource = + index < logs.length - 1 && + getLogSource(logs[index + 1].sourceId).id !== log.sourceId; - // We don't want every line to repeat the icon, because - // that is ugly and repetitive. This removes the icon - // for subsequent lines of the same source and shows a - // line instead, visually indicating they are from the - // same source. - const shouldHideSource = - index > 0 && - getLogSource(logs[index - 1].sourceId).id === log.sourceId; - if (shouldHideSource) { - icon = ( -
+ // We don't want every line to repeat the icon, because + // that is ugly and repetitive. This removes the icon + // for subsequent lines of the same source and shows a + // line instead, visually indicating they are from the + // same source. + const shouldHideSource = + index > 0 && + getLogSource(logs[index - 1].sourceId).id === log.sourceId; + if (shouldHideSource) { + icon = (
({ - height: doesNextLineHaveDifferentSource ? "50%" : "100%", - width: 2, - background: theme.experimental.l1.outline, - borderRadius: 2, - })} - /> - {doesNextLineHaveDifferentSource && ( + css={{ + width: 14, + height: 14, + marginRight: 8, + display: "flex", + justifyContent: "center", + position: "relative", + flexShrink: 0, + }} + >
({ - height: 2, - width: "50%", - top: "calc(50% - 2px)", - left: "calc(50% - 1px)", + height: doesNextLineHaveDifferentSource ? "50%" : "100%", + width: 2, background: theme.experimental.l1.outline, borderRadius: 2, - position: "absolute", })} /> - )} -
+ {doesNextLineHaveDifferentSource && ( +
({ + height: 2, + width: "50%", + top: "calc(50% - 2px)", + left: "calc(50% - 1px)", + background: theme.experimental.l1.outline, + borderRadius: 2, + position: "absolute", + })} + /> + )} +
+ ); + } + + return ( + + {logSource.display_name} + {assignedIcon && ( + +
+ No icon specified! + + )} + + } + > + {icon} + + } + /> ); - } + }} + - return ( - - {logSource.display_name} - {assignedIcon && ( - -
- No icon specified! - - )} - - } - > - {icon} - - } - /> - ); - }} - + {overflowed && ( +

+ Startup logs exceeded the max size of{" "} + 1MB, and will not + continue to be written to the database! Logs will continue to be + written to the + /tmp/coder-startup-script.log{" "} + file in the workspace. +

+ )} +
); }, ); From 6547a2fcbd14ac3d174ab5df92ac15262bbf439a Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Sat, 9 Aug 2025 07:34:04 +0000 Subject: [PATCH 26/27] chore: swap to tailwind --- .../resources/AgentLogs/AgentLogs.stories.tsx | 9 +- .../modules/resources/AgentLogs/AgentLogs.tsx | 94 ++++++------------- 2 files changed, 36 insertions(+), 67 deletions(-) diff --git a/site/src/modules/resources/AgentLogs/AgentLogs.stories.tsx b/site/src/modules/resources/AgentLogs/AgentLogs.stories.tsx index 081ffdc506564..18bae61736bda 100644 --- a/site/src/modules/resources/AgentLogs/AgentLogs.stories.tsx +++ b/site/src/modules/resources/AgentLogs/AgentLogs.stories.tsx @@ -10,6 +10,7 @@ const meta: Meta = { sources: MockSources, logs: MockLogs, height: MockLogs.length * AGENT_LOG_LINE_HEIGHT, + overflowed: false, }, parameters: { layout: "fullscreen", @@ -19,6 +20,10 @@ const meta: Meta = { export default meta; type Story = StoryObj; -const Default: Story = {}; +export const Default: Story = {}; -export { Default as AgentLogs }; +export const Overflowed: Story = { + args: { + overflowed: true, + }, +}; diff --git a/site/src/modules/resources/AgentLogs/AgentLogs.tsx b/site/src/modules/resources/AgentLogs/AgentLogs.tsx index 654a7dce96d86..c9db88f9eb1da 100644 --- a/site/src/modules/resources/AgentLogs/AgentLogs.tsx +++ b/site/src/modules/resources/AgentLogs/AgentLogs.tsx @@ -1,9 +1,9 @@ -import type { Interpolation, Theme } from "@emotion/react"; import Tooltip from "@mui/material/Tooltip"; import type { WorkspaceAgentLogSource } from "api/typesGenerated"; import type { Line } from "components/Logs/LogLine"; import { type ComponentProps, forwardRef } from "react"; import { FixedSizeList as List } from "react-window"; +import { cn } from "utils/cn"; import { AGENT_LOG_LINE_HEIGHT, AgentLogLine } from "./AgentLogLine"; const fallbackLog: WorkspaceAgentLogSource = { @@ -43,10 +43,25 @@ export const AgentLogs = forwardRef( return (
+ {overflowed && ( +

+ Startup logs exceeded the max size of{" "} + 1MB, and will not + continue to be written to the database! Logs will continue to be + written to the + + /tmp/coder-startup-script.log + {" "} + file in the workspace. +

+ )} + logs[index].id || 0} @@ -64,26 +79,18 @@ export const AgentLogs = forwardRef( ); } else { icon = (
); @@ -104,38 +111,18 @@ export const AgentLogs = forwardRef( getLogSource(logs[index - 1].sourceId).id === log.sourceId; if (shouldHideSource) { icon = ( -
+
({ - height: doesNextLineHaveDifferentSource ? "50%" : "100%", - width: 2, - background: theme.experimental.l1.outline, - borderRadius: 2, - })} + // dashed-line class comes from LogLine + className={cn( + "dashed-line w-0.5 rounded-[2px] bg-surface-tertiary h-full", + doesNextLineHaveDifferentSource && "h-1/2", + )} /> {doesNextLineHaveDifferentSource && (
({ - height: 2, - width: "50%", - top: "calc(50% - 2px)", - left: "calc(50% - 1px)", - background: theme.experimental.l1.outline, - borderRadius: 2, - position: "absolute", - })} + role="presentation" + className="dashed-line h-[2px] w-1/2 top-[calc(50%-2px)] left-[calc(50%-1px)] rounded-[2px] absolute bg-surface-tertiary" /> )}
@@ -169,17 +156,6 @@ export const AgentLogs = forwardRef( ); }} - - {overflowed && ( -

- Startup logs exceeded the max size of{" "} - 1MB, and will not - continue to be written to the database! Logs will continue to be - written to the - /tmp/coder-startup-script.log{" "} - file in the workspace. -

- )}
); }, @@ -207,15 +183,3 @@ const determineScriptDisplayColor = (displayName: string): string => { }, 0); return scriptDisplayColors[Math.abs(hash) % scriptDisplayColors.length]; }; - -const styles = { - logs: (theme) => ({ - backgroundColor: theme.palette.background.paper, - paddingTop: 16, - - // We need this to be able to apply the padding top from startupLogs - "& > div": { - position: "relative", - }, - }), -} satisfies Record>; From c818aece65f4da494618233f294d7df6410794d0 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Tue, 12 Aug 2025 13:54:37 +0000 Subject: [PATCH 27/27] wip: commit progress --- site/src/modules/resources/AgentLogs/AgentLogs.tsx | 2 +- site/src/modules/resources/AgentRow.tsx | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/site/src/modules/resources/AgentLogs/AgentLogs.tsx b/site/src/modules/resources/AgentLogs/AgentLogs.tsx index c9db88f9eb1da..ded5ad11166e6 100644 --- a/site/src/modules/resources/AgentLogs/AgentLogs.tsx +++ b/site/src/modules/resources/AgentLogs/AgentLogs.tsx @@ -44,7 +44,7 @@ export const AgentLogs = forwardRef( return (
{overflowed && ( -

+

Startup logs exceeded the max size of{" "} 1MB, and will not continue to be written to the database! Logs will continue to be diff --git a/site/src/modules/resources/AgentRow.tsx b/site/src/modules/resources/AgentRow.tsx index 912ca63dea9f4..666ae30cac01b 100644 --- a/site/src/modules/resources/AgentRow.tsx +++ b/site/src/modules/resources/AgentRow.tsx @@ -295,7 +295,7 @@ export const AgentRow: FC = ({ width={width} css={styles.startupLogs} onScroll={handleLogScroll} - overflowed={agent.logs_overflowed} + overflowed={true} logs={agentLogs.map((l) => ({ id: l.id, level: l.level,