From 3f9a1bd031ad54e3958ee2b8526ebd858170445c Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Tue, 13 May 2025 19:29:27 +0000 Subject: [PATCH 01/10] fix: duplicated agent logs --- site/src/api/api.ts | 36 ++---- site/src/api/queries/workspaces.ts | 16 +-- .../resources/AgentLogs/useAgentLogs.test.tsx | 51 ++------- .../resources/AgentLogs/useAgentLogs.ts | 108 +++++++----------- site/src/modules/resources/AgentRow.tsx | 15 +-- .../DownloadAgentLogsButton.stories.tsx | 2 +- .../resources/DownloadAgentLogsButton.tsx | 2 +- .../WorkspaceBuildPageView.tsx | 25 ++-- .../DownloadLogsDialog.stories.tsx | 4 +- .../WorkspaceActions/DownloadLogsDialog.tsx | 2 +- .../WorkspaceActions.stories.tsx | 2 +- .../WorkspacePage/WorkspacePage.test.tsx | 13 ++- 12 files changed, 102 insertions(+), 174 deletions(-) diff --git a/site/src/api/api.ts b/site/src/api/api.ts index 688ba0432e22b..657a27f219215 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -19,7 +19,11 @@ * * For example, `utils/delay` must be imported using `../utils/delay` instead. */ -import globalAxios, { type AxiosInstance, isAxiosError } from "axios"; +import globalAxios, { + type AxiosInstance, + type AxiosRequestConfig, + isAxiosError, +} from "axios"; import type dayjs from "dayjs"; import userAgentParser from "ua-parser-js"; import { OneWayWebSocket } from "../utils/OneWayWebSocket"; @@ -221,11 +225,11 @@ export const watchBuildLogsByTemplateVersionId = ( export const watchWorkspaceAgentLogs = ( agentId: string, - { after, onMessage, onDone, onError }: WatchWorkspaceAgentLogsOptions, + params?: WatchWorkspaceAgentLogsParams, ) => { const searchParams = new URLSearchParams({ follow: "true", - after: after.toString(), + after: params?.after ? params?.after.toString() : "", }); /** @@ -237,32 +241,14 @@ export const watchWorkspaceAgentLogs = ( searchParams.set("no_compression", ""); } - const socket = createWebSocket( - `/api/v2/workspaceagents/${agentId}/logs`, + return new OneWayWebSocket({ + apiRoute: `/api/v2/workspaceagents/${agentId}/logs`, searchParams, - ); - - socket.addEventListener("message", (event) => { - const logs = JSON.parse(event.data) as TypesGen.WorkspaceAgentLog[]; - onMessage(logs); - }); - - socket.addEventListener("error", () => { - onError(new Error("socket errored")); }); - - socket.addEventListener("close", () => { - onDone?.(); - }); - - return socket; }; -type WatchWorkspaceAgentLogsOptions = { - after: number; - onMessage: (logs: TypesGen.WorkspaceAgentLog[]) => void; - onDone?: () => void; - onError: (error: Error) => void; +type WatchWorkspaceAgentLogsParams = { + after?: number; }; type WatchBuildLogsByBuildIdOptions = { diff --git a/site/src/api/queries/workspaces.ts b/site/src/api/queries/workspaces.ts index 39008f5c712a3..53c03ea8898c8 100644 --- a/site/src/api/queries/workspaces.ts +++ b/site/src/api/queries/workspaces.ts @@ -5,6 +5,7 @@ import type { ProvisionerLogLevel, UsageAppName, Workspace, + WorkspaceAgentLog, WorkspaceBuild, WorkspaceBuildParameter, WorkspacesRequest, @@ -16,6 +17,7 @@ import type { QueryClient, QueryOptions, UseMutationOptions, + UseQueryOptions, } from "react-query"; import { disabledRefetchOptions } from "./util"; import { workspaceBuildsKey } from "./workspaceBuilds"; @@ -337,20 +339,14 @@ export const buildLogs = (workspace: Workspace) => { }; }; -export const agentLogsKey = (workspaceId: string, agentId: string) => [ - "workspaces", - workspaceId, - "agents", - agentId, - "logs", -]; +export const agentLogsKey = (agentId: string) => ["agents", agentId, "logs"]; -export const agentLogs = (workspaceId: string, agentId: string) => { +export const agentLogs = (agentId: string) => { return { - queryKey: agentLogsKey(workspaceId, agentId), + queryKey: agentLogsKey(agentId), queryFn: () => API.getWorkspaceAgentLogs(agentId), ...disabledRefetchOptions, - }; + } satisfies UseQueryOptions; }; // workspace usage options diff --git a/site/src/modules/resources/AgentLogs/useAgentLogs.test.tsx b/site/src/modules/resources/AgentLogs/useAgentLogs.test.tsx index e1aaccc40d6f7..0a1625e714c07 100644 --- a/site/src/modules/resources/AgentLogs/useAgentLogs.test.tsx +++ b/site/src/modules/resources/AgentLogs/useAgentLogs.test.tsx @@ -2,44 +2,29 @@ import { act, renderHook, waitFor } from "@testing-library/react"; import { API } from "api/api"; import * as APIModule from "api/api"; import { agentLogsKey } from "api/queries/workspaces"; -import type { WorkspaceAgentLog } from "api/typesGenerated"; +import type { WorkspaceAgent, WorkspaceAgentLog } from "api/typesGenerated"; import WS from "jest-websocket-mock"; import { type QueryClient, QueryClientProvider } from "react-query"; -import { MockWorkspace, MockWorkspaceAgent } from "testHelpers/entities"; +import { MockWorkspaceAgent } from "testHelpers/entities"; import { createTestQueryClient } from "testHelpers/renderHelpers"; -import { type UseAgentLogsOptions, useAgentLogs } from "./useAgentLogs"; +import { useAgentLogs } from "./useAgentLogs"; afterEach(() => { WS.clean(); }); describe("useAgentLogs", () => { - it("should not fetch logs if disabled", async () => { - const queryClient = createTestQueryClient(); - const fetchSpy = jest.spyOn(API, "getWorkspaceAgentLogs"); - const wsSpy = jest.spyOn(APIModule, "watchWorkspaceAgentLogs"); - renderUseAgentLogs(queryClient, { - workspaceId: MockWorkspace.id, - agentId: MockWorkspaceAgent.id, - agentLifeCycleState: "ready", - enabled: false, - }); - expect(fetchSpy).not.toHaveBeenCalled(); - expect(wsSpy).not.toHaveBeenCalled(); - }); - it("should return existing logs without network calls if state is off", async () => { const queryClient = createTestQueryClient(); queryClient.setQueryData( - agentLogsKey(MockWorkspace.id, MockWorkspaceAgent.id), + agentLogsKey(MockWorkspaceAgent.id), generateLogs(5), ); const fetchSpy = jest.spyOn(API, "getWorkspaceAgentLogs"); const wsSpy = jest.spyOn(APIModule, "watchWorkspaceAgentLogs"); const { result } = renderUseAgentLogs(queryClient, { - workspaceId: MockWorkspace.id, - agentId: MockWorkspaceAgent.id, - agentLifeCycleState: "off", + ...MockWorkspaceAgent, + lifecycle_state: "off", }); await waitFor(() => { expect(result.current).toHaveLength(5); @@ -55,9 +40,8 @@ describe("useAgentLogs", () => { .mockResolvedValueOnce(generateLogs(5)); jest.spyOn(APIModule, "watchWorkspaceAgentLogs"); const { result } = renderUseAgentLogs(queryClient, { - workspaceId: MockWorkspace.id, - agentId: MockWorkspaceAgent.id, - agentLifeCycleState: "ready", + ...MockWorkspaceAgent, + lifecycle_state: "ready", }); await waitFor(() => { expect(result.current).toHaveLength(5); @@ -77,11 +61,7 @@ describe("useAgentLogs", () => { MockWorkspaceAgent.id }/logs?follow&after=${logs[logs.length - 1].id}`, ); - const { result } = renderUseAgentLogs(queryClient, { - workspaceId: MockWorkspace.id, - agentId: MockWorkspaceAgent.id, - agentLifeCycleState: "starting", - }); + const { result } = renderUseAgentLogs(queryClient, MockWorkspaceAgent); await waitFor(() => { expect(result.current).toHaveLength(5); }); @@ -102,11 +82,7 @@ describe("useAgentLogs", () => { MockWorkspaceAgent.id }/logs?follow&after=${logs[logs.length - 1].id}`, ); - const { result } = renderUseAgentLogs(queryClient, { - workspaceId: MockWorkspace.id, - agentId: MockWorkspaceAgent.id, - agentLifeCycleState: "starting", - }); + const { result } = renderUseAgentLogs(queryClient, MockWorkspaceAgent); await waitFor(() => { expect(result.current).toHaveLength(5); }); @@ -120,11 +96,8 @@ describe("useAgentLogs", () => { }); }); -function renderUseAgentLogs( - queryClient: QueryClient, - options: UseAgentLogsOptions, -) { - return renderHook(() => useAgentLogs(options), { +function renderUseAgentLogs(queryClient: QueryClient, agent: WorkspaceAgent) { + return renderHook(() => useAgentLogs(agent), { wrapper: ({ children }) => ( {children} ), diff --git a/site/src/modules/resources/AgentLogs/useAgentLogs.ts b/site/src/modules/resources/AgentLogs/useAgentLogs.ts index a53f1d882dc60..0ed6842de04d3 100644 --- a/site/src/modules/resources/AgentLogs/useAgentLogs.ts +++ b/site/src/modules/resources/AgentLogs/useAgentLogs.ts @@ -1,95 +1,73 @@ import { watchWorkspaceAgentLogs } from "api/api"; import { agentLogs } from "api/queries/workspaces"; import type { + WorkspaceAgent, WorkspaceAgentLifecycle, WorkspaceAgentLog, } from "api/typesGenerated"; +import { displayError } from "components/GlobalSnackbar/utils"; import { useEffectEvent } from "hooks/hookPolyfills"; -import { useEffect, useRef } from "react"; +import { useEffect } from "react"; import { useQuery, useQueryClient } from "react-query"; -export type UseAgentLogsOptions = Readonly<{ - workspaceId: string; - agentId: string; - agentLifeCycleState: WorkspaceAgentLifecycle; - enabled?: boolean; -}>; +const ON_GOING_STATES: WorkspaceAgentLifecycle[] = ["starting", "created"]; -/** - * Defines a custom hook that gives you all workspace agent logs for a given - * workspace.Depending on the status of the workspace, all logs may or may not - * be available. - */ export function useAgentLogs( - options: UseAgentLogsOptions, + agent: WorkspaceAgent, ): readonly WorkspaceAgentLog[] | undefined { - const { workspaceId, agentId, agentLifeCycleState, enabled = true } = options; const queryClient = useQueryClient(); - const queryOptions = agentLogs(workspaceId, agentId); - const { data: logs, isFetched } = useQuery({ ...queryOptions, enabled }); - - // Track the ID of the last log received when the initial logs response comes - // back. If the logs are not complete, the ID will mark the start point of the - // Web sockets response so that the remaining logs can be received over time - const lastQueriedLogId = useRef(0); - useEffect(() => { - const isAlreadyTracking = lastQueriedLogId.current !== 0; - if (isAlreadyTracking) { - return; - } + const agentLogsOptions = agentLogs(agent.id); + const shouldUseSocket = ON_GOING_STATES.includes(agent.lifecycle_state); + const { data: logs } = useQuery({ + ...agentLogsOptions, + enabled: !shouldUseSocket, + }); - const lastLog = logs?.at(-1); - if (lastLog !== undefined) { - lastQueriedLogId.current = lastLog.id; - } - }, [logs]); + const appendAgentLogs = useEffectEvent( + async (newLogs: WorkspaceAgentLog[]) => { + await queryClient.cancelQueries(agentLogsOptions.queryKey); + queryClient.setQueryData( + agentLogsOptions.queryKey, + (oldLogs) => (oldLogs ? [...oldLogs, ...newLogs] : newLogs), + ); + }, + ); - const addLogs = useEffectEvent((newLogs: WorkspaceAgentLog[]) => { - queryClient.setQueryData( - queryOptions.queryKey, - (oldData: WorkspaceAgentLog[] = []) => [...oldData, ...newLogs], - ); + const refreshAgentLogs = useEffectEvent(() => { + queryClient.invalidateQueries(agentLogsOptions.queryKey); }); useEffect(() => { - // Stream data only for new logs. Old logs should be loaded beforehand - // using a regular fetch to avoid overloading the websocket with all - // logs at once. - if (!isFetched) { + if (!shouldUseSocket) { return; } - // If the agent is off, we don't need to stream logs. This is the only state - // where the Coder API can't receive logs for the agent from third-party - // apps like envbuilder. - if (agentLifeCycleState === "off") { - return; - } + const socket = watchWorkspaceAgentLogs(agent.id, { after: 0 }); + socket.addEventListener("message", (e) => { + if (e.parseError) { + console.warn("Error parsing agent log: ", e.parseError); + return; + } + appendAgentLogs(e.parsedMessage); + }); - const socket = watchWorkspaceAgentLogs(agentId, { - after: lastQueriedLogId.current, - onMessage: (newLogs) => { - // Prevent new logs getting added when a connection is not open - if (socket.readyState !== WebSocket.OPEN) { - return; - } - addLogs(newLogs); - }, - onError: (error) => { - // For some reason Firefox and Safari throw an error when a websocket - // connection is close in the middle of a message and because of that we - // can't safely show to the users an error message since most of the - // time they are just internal stuff. This does not happen to Chrome at - // all and I tried to find better way to "soft close" a WS connection on - // those browsers without success. - console.error(error); - }, + 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(); + // For some reason, after closing the socket, a few logs still getting + // generated in the BE. This is a workaround to avoid we don't display + // them in the UI. + refreshAgentLogs(); }; - }, [addLogs, agentId, agentLifeCycleState, isFetched]); + }, [agent.id, shouldUseSocket, appendAgentLogs, refreshAgentLogs]); return logs; } diff --git a/site/src/modules/resources/AgentRow.tsx b/site/src/modules/resources/AgentRow.tsx index c4d104501fd67..d94b6995bf463 100644 --- a/site/src/modules/resources/AgentRow.tsx +++ b/site/src/modules/resources/AgentRow.tsx @@ -88,12 +88,7 @@ export const AgentRow: FC = ({ ["starting", "start_timeout"].includes(agent.lifecycle_state) && hasStartupFeatures, ); - const agentLogs = useAgentLogs({ - workspaceId: workspace.id, - agentId: agent.id, - agentLifeCycleState: agent.lifecycle_state, - enabled: showLogs, - }); + const agentLogs = useAgentLogs(agent); const logListRef = useRef(null); const logListDivRef = useRef(null); const startupLogs = useMemo(() => { @@ -160,7 +155,13 @@ export const AgentRow: FC = ({ select: (res) => res.containers.filter((c) => c.status === "running"), // TODO: Implement a websocket connection to get updates on containers // without having to poll. - refetchInterval: 10_000, + refetchInterval: (data) => { + if (!data) { + return false; + } + + return data.length > 0 ? 10_000 : false; + }, }); return ( diff --git a/site/src/modules/resources/DownloadAgentLogsButton.stories.tsx b/site/src/modules/resources/DownloadAgentLogsButton.stories.tsx index 5d39ab7d74412..74b1df7258059 100644 --- a/site/src/modules/resources/DownloadAgentLogsButton.stories.tsx +++ b/site/src/modules/resources/DownloadAgentLogsButton.stories.tsx @@ -15,7 +15,7 @@ const meta: Meta = { parameters: { queries: [ { - key: agentLogsKey(MockWorkspace.id, MockWorkspaceAgent.id), + key: agentLogsKey(MockWorkspaceAgent.id), data: generateLogs(5), }, ], diff --git a/site/src/modules/resources/DownloadAgentLogsButton.tsx b/site/src/modules/resources/DownloadAgentLogsButton.tsx index b884a250c7fcb..dfc00433a8063 100644 --- a/site/src/modules/resources/DownloadAgentLogsButton.tsx +++ b/site/src/modules/resources/DownloadAgentLogsButton.tsx @@ -23,7 +23,7 @@ export const DownloadAgentLogsButton: FC = ({ const [isDownloading, setIsDownloading] = useState(false); const fetchLogs = async () => { - const queryOpts = agentLogs(workspaceId, agent.id); + const queryOpts = agentLogs(agent.id); let logs = queryClient.getQueryData( queryOpts.queryKey, ); diff --git a/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx b/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx index e291497a58fe0..3fe44ee4db19c 100644 --- a/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx +++ b/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx @@ -212,13 +212,9 @@ export const WorkspaceBuildPageView: FC = ({ )} - {tabState.value === "build" ? ( - - ) : ( - + {tabState.value === "build" && } + {tabState.value !== "build" && selectedAgent && ( + )} @@ -286,15 +282,12 @@ const BuildLogsContent: FC<{ logs?: ProvisionerJobLog[] }> = ({ logs }) => { ); }; -const AgentLogsContent: FC<{ workspaceId: string; agent: WorkspaceAgent }> = ({ - agent, - workspaceId, -}) => { - const logs = useAgentLogs({ - workspaceId, - agentId: agent.id, - agentLifeCycleState: agent.lifecycle_state, - }); +type AgentLogsContentProps = { + agent: WorkspaceAgent; +}; + +const AgentLogsContent: FC = ({ agent }) => { + const logs = useAgentLogs(agent); if (!logs) { return ; diff --git a/site/src/pages/WorkspacePage/WorkspaceActions/DownloadLogsDialog.stories.tsx b/site/src/pages/WorkspacePage/WorkspaceActions/DownloadLogsDialog.stories.tsx index 14bd1b4a6d6b7..961057142b0d6 100644 --- a/site/src/pages/WorkspacePage/WorkspaceActions/DownloadLogsDialog.stories.tsx +++ b/site/src/pages/WorkspacePage/WorkspaceActions/DownloadLogsDialog.stories.tsx @@ -20,7 +20,7 @@ const meta: Meta = { data: generateLogs(200), }, { - key: agentLogsKey(MockWorkspace.id, MockWorkspaceAgent.id), + key: agentLogsKey(MockWorkspaceAgent.id), data: generateLogs(400), }, ], @@ -41,7 +41,7 @@ export const Loading: Story = { data: undefined, }, { - key: agentLogsKey(MockWorkspace.id, MockWorkspaceAgent.id), + key: agentLogsKey(MockWorkspaceAgent.id), data: undefined, }, ], diff --git a/site/src/pages/WorkspacePage/WorkspaceActions/DownloadLogsDialog.tsx b/site/src/pages/WorkspacePage/WorkspaceActions/DownloadLogsDialog.tsx index 0beeb795a09b6..ee0f18ff93e0c 100644 --- a/site/src/pages/WorkspacePage/WorkspaceActions/DownloadLogsDialog.tsx +++ b/site/src/pages/WorkspacePage/WorkspaceActions/DownloadLogsDialog.tsx @@ -53,7 +53,7 @@ export const DownloadLogsDialog: FC = ({ const agentLogQueries = useQueries({ queries: allUniqueAgents.map((agent) => ({ - ...agentLogs(workspace.id, agent.id), + ...agentLogs(agent.id), enabled: open, })), }); diff --git a/site/src/pages/WorkspacePage/WorkspaceActions/WorkspaceActions.stories.tsx b/site/src/pages/WorkspacePage/WorkspaceActions/WorkspaceActions.stories.tsx index a67690f929b5f..73086b493c029 100644 --- a/site/src/pages/WorkspacePage/WorkspaceActions/WorkspaceActions.stories.tsx +++ b/site/src/pages/WorkspacePage/WorkspaceActions/WorkspaceActions.stories.tsx @@ -202,7 +202,7 @@ export const OpenDownloadLogs: Story = { data: generateLogs(200), }, { - key: agentLogsKey(Mocks.MockWorkspace.id, Mocks.MockWorkspaceAgent.id), + key: agentLogsKey(Mocks.MockWorkspaceAgent.id), data: generateLogs(400), }, ], diff --git a/site/src/pages/WorkspacePage/WorkspacePage.test.tsx b/site/src/pages/WorkspacePage/WorkspacePage.test.tsx index a9f78f3610983..c71160abf2beb 100644 --- a/site/src/pages/WorkspacePage/WorkspacePage.test.tsx +++ b/site/src/pages/WorkspacePage/WorkspacePage.test.tsx @@ -33,6 +33,7 @@ import { } from "testHelpers/renderHelpers"; import { server } from "testHelpers/server"; import WorkspacePage from "./WorkspacePage"; +import { OneWayWebSocket } from "utils/OneWayWebSocket"; const { API, MissingBuildParameters } = apiModule; @@ -49,12 +50,12 @@ const renderWorkspacePage = async ( jest .spyOn(API, "getDeploymentConfig") .mockResolvedValueOnce(MockDeploymentConfig); - jest - .spyOn(apiModule, "watchWorkspaceAgentLogs") - .mockImplementation((_, options) => { - options.onDone?.(); - return new WebSocket(""); - }); + jest.spyOn(apiModule, "watchWorkspaceAgentLogs").mockImplementation( + () => + new OneWayWebSocket({ + apiRoute: "", + }), + ); renderWithAuth(, { ...options, From cd050ccc87c00575c6f8c3ce71f14917dead7ff5 Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Tue, 13 May 2025 19:30:41 +0000 Subject: [PATCH 02/10] Fix fmt --- site/src/api/api.ts | 6 +----- site/src/pages/WorkspacePage/WorkspacePage.test.tsx | 2 +- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/site/src/api/api.ts b/site/src/api/api.ts index 657a27f219215..4db955e17d3fe 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -19,11 +19,7 @@ * * For example, `utils/delay` must be imported using `../utils/delay` instead. */ -import globalAxios, { - type AxiosInstance, - type AxiosRequestConfig, - isAxiosError, -} from "axios"; +import globalAxios, { type AxiosInstance, isAxiosError } from "axios"; import type dayjs from "dayjs"; import userAgentParser from "ua-parser-js"; import { OneWayWebSocket } from "../utils/OneWayWebSocket"; diff --git a/site/src/pages/WorkspacePage/WorkspacePage.test.tsx b/site/src/pages/WorkspacePage/WorkspacePage.test.tsx index c71160abf2beb..32a7a79729efc 100644 --- a/site/src/pages/WorkspacePage/WorkspacePage.test.tsx +++ b/site/src/pages/WorkspacePage/WorkspacePage.test.tsx @@ -32,8 +32,8 @@ import { renderWithAuth, } from "testHelpers/renderHelpers"; import { server } from "testHelpers/server"; -import WorkspacePage from "./WorkspacePage"; import { OneWayWebSocket } from "utils/OneWayWebSocket"; +import WorkspacePage from "./WorkspacePage"; const { API, MissingBuildParameters } = apiModule; From ffe3d849e5cce1f4f5fc5370d22505214e681aba Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Wed, 14 May 2025 12:22:59 +0000 Subject: [PATCH 03/10] Rollback containers intervals --- site/src/modules/resources/AgentRow.tsx | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/site/src/modules/resources/AgentRow.tsx b/site/src/modules/resources/AgentRow.tsx index d94b6995bf463..30d488547d461 100644 --- a/site/src/modules/resources/AgentRow.tsx +++ b/site/src/modules/resources/AgentRow.tsx @@ -155,13 +155,7 @@ export const AgentRow: FC = ({ select: (res) => res.containers.filter((c) => c.status === "running"), // TODO: Implement a websocket connection to get updates on containers // without having to poll. - refetchInterval: (data) => { - if (!data) { - return false; - } - - return data.length > 0 ? 10_000 : false; - }, + refetchInterval: 10_000, }); return ( From 72387347c010093fee70ab5e5122eba577a80dd2 Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Wed, 14 May 2025 12:31:31 +0000 Subject: [PATCH 04/10] Simplify logic --- .../resources/AgentLogs/useAgentLogs.test.tsx | 2 +- .../resources/AgentLogs/useAgentLogs.ts | 54 ++++++------------- site/src/modules/resources/AgentRow.tsx | 2 +- .../WorkspaceBuildPageView.tsx | 2 +- 4 files changed, 18 insertions(+), 42 deletions(-) diff --git a/site/src/modules/resources/AgentLogs/useAgentLogs.test.tsx b/site/src/modules/resources/AgentLogs/useAgentLogs.test.tsx index 0a1625e714c07..92074a85e7a03 100644 --- a/site/src/modules/resources/AgentLogs/useAgentLogs.test.tsx +++ b/site/src/modules/resources/AgentLogs/useAgentLogs.test.tsx @@ -97,7 +97,7 @@ describe("useAgentLogs", () => { }); function renderUseAgentLogs(queryClient: QueryClient, agent: WorkspaceAgent) { - return renderHook(() => useAgentLogs(agent), { + return renderHook(() => useAgentLogs(agent, true), { wrapper: ({ children }) => ( {children} ), diff --git a/site/src/modules/resources/AgentLogs/useAgentLogs.ts b/site/src/modules/resources/AgentLogs/useAgentLogs.ts index 0ed6842de04d3..30ab3e1d2bec4 100644 --- a/site/src/modules/resources/AgentLogs/useAgentLogs.ts +++ b/site/src/modules/resources/AgentLogs/useAgentLogs.ts @@ -1,54 +1,34 @@ import { watchWorkspaceAgentLogs } from "api/api"; -import { agentLogs } from "api/queries/workspaces"; -import type { - WorkspaceAgent, - WorkspaceAgentLifecycle, - WorkspaceAgentLog, -} from "api/typesGenerated"; +import type { WorkspaceAgent, WorkspaceAgentLog } from "api/typesGenerated"; import { displayError } from "components/GlobalSnackbar/utils"; -import { useEffectEvent } from "hooks/hookPolyfills"; -import { useEffect } from "react"; -import { useQuery, useQueryClient } from "react-query"; - -const ON_GOING_STATES: WorkspaceAgentLifecycle[] = ["starting", "created"]; +import { useEffect, useState } from "react"; export function useAgentLogs( agent: WorkspaceAgent, + enabled: boolean, ): readonly WorkspaceAgentLog[] | undefined { - const queryClient = useQueryClient(); - const agentLogsOptions = agentLogs(agent.id); - const shouldUseSocket = ON_GOING_STATES.includes(agent.lifecycle_state); - const { data: logs } = useQuery({ - ...agentLogsOptions, - enabled: !shouldUseSocket, - }); - - const appendAgentLogs = useEffectEvent( - async (newLogs: WorkspaceAgentLog[]) => { - await queryClient.cancelQueries(agentLogsOptions.queryKey); - queryClient.setQueryData( - agentLogsOptions.queryKey, - (oldLogs) => (oldLogs ? [...oldLogs, ...newLogs] : newLogs), - ); - }, - ); - - const refreshAgentLogs = useEffectEvent(() => { - queryClient.invalidateQueries(agentLogsOptions.queryKey); - }); + const [logs, setLogs] = useState(); useEffect(() => { - if (!shouldUseSocket) { + if (!enabled) { + // Clean up the logs when the agent is not enabled. So it can receive logs + // from the beginning without duplicating the logs. + setLogs(undefined); return; } + // 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); return; } - appendAgentLogs(e.parsedMessage); + setLogs((logs) => + logs ? [...logs, ...e.parsedMessage] : [...e.parsedMessage], + ); }); socket.addEventListener("error", (e) => { @@ -62,12 +42,8 @@ export function useAgentLogs( return () => { socket.close(); - // For some reason, after closing the socket, a few logs still getting - // generated in the BE. This is a workaround to avoid we don't display - // them in the UI. - refreshAgentLogs(); }; - }, [agent.id, shouldUseSocket, appendAgentLogs, refreshAgentLogs]); + }, [agent.id, enabled]); return logs; } diff --git a/site/src/modules/resources/AgentRow.tsx b/site/src/modules/resources/AgentRow.tsx index 30d488547d461..2faceab104767 100644 --- a/site/src/modules/resources/AgentRow.tsx +++ b/site/src/modules/resources/AgentRow.tsx @@ -88,7 +88,7 @@ export const AgentRow: FC = ({ ["starting", "start_timeout"].includes(agent.lifecycle_state) && hasStartupFeatures, ); - const agentLogs = useAgentLogs(agent); + const agentLogs = useAgentLogs(agent, showLogs); const logListRef = useRef(null); const logListDivRef = useRef(null); const startupLogs = useMemo(() => { diff --git a/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx b/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx index 3fe44ee4db19c..9e39dd492791e 100644 --- a/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx +++ b/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx @@ -287,7 +287,7 @@ type AgentLogsContentProps = { }; const AgentLogsContent: FC = ({ agent }) => { - const logs = useAgentLogs(agent); + const logs = useAgentLogs(agent, true); if (!logs) { return ; From 5160e287a054b492911c92ea71df2f6e1d90d35b Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Wed, 14 May 2025 13:23:54 +0000 Subject: [PATCH 05/10] Fix tests --- .../resources/AgentLogs/useAgentLogs.test.tsx | 115 ------------------ site/src/modules/resources/AgentRow.tsx | 2 +- .../modules/resources/useAgentLogs.test.ts | 54 ++++++++ .../resources/{AgentLogs => }/useAgentLogs.ts | 0 .../WorkspaceBuildPageView.tsx | 2 +- 5 files changed, 56 insertions(+), 117 deletions(-) delete mode 100644 site/src/modules/resources/AgentLogs/useAgentLogs.test.tsx create mode 100644 site/src/modules/resources/useAgentLogs.test.ts rename site/src/modules/resources/{AgentLogs => }/useAgentLogs.ts (100%) diff --git a/site/src/modules/resources/AgentLogs/useAgentLogs.test.tsx b/site/src/modules/resources/AgentLogs/useAgentLogs.test.tsx deleted file mode 100644 index 92074a85e7a03..0000000000000 --- a/site/src/modules/resources/AgentLogs/useAgentLogs.test.tsx +++ /dev/null @@ -1,115 +0,0 @@ -import { act, renderHook, waitFor } from "@testing-library/react"; -import { API } from "api/api"; -import * as APIModule from "api/api"; -import { agentLogsKey } from "api/queries/workspaces"; -import type { WorkspaceAgent, WorkspaceAgentLog } from "api/typesGenerated"; -import WS from "jest-websocket-mock"; -import { type QueryClient, QueryClientProvider } from "react-query"; -import { MockWorkspaceAgent } from "testHelpers/entities"; -import { createTestQueryClient } from "testHelpers/renderHelpers"; -import { useAgentLogs } from "./useAgentLogs"; - -afterEach(() => { - WS.clean(); -}); - -describe("useAgentLogs", () => { - it("should return existing logs without network calls if state is off", async () => { - const queryClient = createTestQueryClient(); - queryClient.setQueryData( - agentLogsKey(MockWorkspaceAgent.id), - generateLogs(5), - ); - const fetchSpy = jest.spyOn(API, "getWorkspaceAgentLogs"); - const wsSpy = jest.spyOn(APIModule, "watchWorkspaceAgentLogs"); - const { result } = renderUseAgentLogs(queryClient, { - ...MockWorkspaceAgent, - lifecycle_state: "off", - }); - await waitFor(() => { - expect(result.current).toHaveLength(5); - }); - expect(fetchSpy).not.toHaveBeenCalled(); - expect(wsSpy).not.toHaveBeenCalled(); - }); - - it("should fetch logs when empty", async () => { - const queryClient = createTestQueryClient(); - const fetchSpy = jest - .spyOn(API, "getWorkspaceAgentLogs") - .mockResolvedValueOnce(generateLogs(5)); - jest.spyOn(APIModule, "watchWorkspaceAgentLogs"); - const { result } = renderUseAgentLogs(queryClient, { - ...MockWorkspaceAgent, - lifecycle_state: "ready", - }); - await waitFor(() => { - expect(result.current).toHaveLength(5); - }); - expect(fetchSpy).toHaveBeenCalledWith(MockWorkspaceAgent.id); - }); - - it("should fetch logs and connect to websocket", async () => { - const queryClient = createTestQueryClient(); - const logs = generateLogs(5); - const fetchSpy = jest - .spyOn(API, "getWorkspaceAgentLogs") - .mockResolvedValueOnce(logs); - const wsSpy = jest.spyOn(APIModule, "watchWorkspaceAgentLogs"); - new WS( - `ws://localhost/api/v2/workspaceagents/${ - MockWorkspaceAgent.id - }/logs?follow&after=${logs[logs.length - 1].id}`, - ); - const { result } = renderUseAgentLogs(queryClient, MockWorkspaceAgent); - await waitFor(() => { - expect(result.current).toHaveLength(5); - }); - expect(fetchSpy).toHaveBeenCalledWith(MockWorkspaceAgent.id); - expect(wsSpy).toHaveBeenCalledWith(MockWorkspaceAgent.id, { - after: logs[logs.length - 1].id, - onMessage: expect.any(Function), - onError: expect.any(Function), - }); - }); - - it("update logs from websocket messages", async () => { - const queryClient = createTestQueryClient(); - const logs = generateLogs(5); - jest.spyOn(API, "getWorkspaceAgentLogs").mockResolvedValueOnce(logs); - const server = new WS( - `ws://localhost/api/v2/workspaceagents/${ - MockWorkspaceAgent.id - }/logs?follow&after=${logs[logs.length - 1].id}`, - ); - const { result } = renderUseAgentLogs(queryClient, MockWorkspaceAgent); - await waitFor(() => { - expect(result.current).toHaveLength(5); - }); - await server.connected; - act(() => { - server.send(JSON.stringify(generateLogs(3))); - }); - await waitFor(() => { - expect(result.current).toHaveLength(8); - }); - }); -}); - -function renderUseAgentLogs(queryClient: QueryClient, agent: WorkspaceAgent) { - return renderHook(() => useAgentLogs(agent, true), { - wrapper: ({ children }) => ( - {children} - ), - }); -} - -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/AgentRow.tsx b/site/src/modules/resources/AgentRow.tsx index 2faceab104767..88e089d14dec6 100644 --- a/site/src/modules/resources/AgentRow.tsx +++ b/site/src/modules/resources/AgentRow.tsx @@ -30,7 +30,7 @@ import { AgentDevcontainerCard } from "./AgentDevcontainerCard"; import { AgentLatency } from "./AgentLatency"; import { AGENT_LOG_LINE_HEIGHT } from "./AgentLogs/AgentLogLine"; import { AgentLogs } from "./AgentLogs/AgentLogs"; -import { useAgentLogs } from "./AgentLogs/useAgentLogs"; +import { useAgentLogs } from "./useAgentLogs"; import { AgentMetadata } from "./AgentMetadata"; import { AgentStatus } from "./AgentStatus"; import { AgentVersion } from "./AgentVersion"; diff --git a/site/src/modules/resources/useAgentLogs.test.ts b/site/src/modules/resources/useAgentLogs.test.ts new file mode 100644 index 0000000000000..f48de8ebd70d4 --- /dev/null +++ b/site/src/modules/resources/useAgentLogs.test.ts @@ -0,0 +1,54 @@ +import { renderHook } 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"; + +/** + * 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 + */ + +describe("useAgentLogs", () => { + afterEach(() => { + WS.clean(); + }); + + 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))); + expect(result.current).toHaveLength(3); + + // Disable the hook + rerender({ enabled: false }); + expect(result.current).toBeUndefined(); + + // Enable the hook again + rerender({ enabled: true }); + await server.connected; + server.send(JSON.stringify(generateLogs(3))); + expect(result.current).toHaveLength(3); + }); +}); + +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/AgentLogs/useAgentLogs.ts b/site/src/modules/resources/useAgentLogs.ts similarity index 100% rename from site/src/modules/resources/AgentLogs/useAgentLogs.ts rename to site/src/modules/resources/useAgentLogs.ts diff --git a/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx b/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx index 9e39dd492791e..f35b5b8465425 100644 --- a/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx +++ b/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx @@ -21,7 +21,7 @@ import { useSearchParamsKey } from "hooks/useSearchParamsKey"; import { BuildAvatar } from "modules/builds/BuildAvatar/BuildAvatar"; import { DashboardFullPage } from "modules/dashboard/DashboardLayout"; import { AgentLogs } from "modules/resources/AgentLogs/AgentLogs"; -import { useAgentLogs } from "modules/resources/AgentLogs/useAgentLogs"; +import { useAgentLogs } from "modules/resources/useAgentLogs"; import { WorkspaceBuildData, WorkspaceBuildDataSkeleton, From 6d33649da10f9a437a2b2651a90e73025a5a7db6 Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Wed, 14 May 2025 13:27:21 +0000 Subject: [PATCH 06/10] FMT --- site/src/modules/resources/AgentRow.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/modules/resources/AgentRow.tsx b/site/src/modules/resources/AgentRow.tsx index 88e089d14dec6..50f7148d9308c 100644 --- a/site/src/modules/resources/AgentRow.tsx +++ b/site/src/modules/resources/AgentRow.tsx @@ -30,7 +30,6 @@ import { AgentDevcontainerCard } from "./AgentDevcontainerCard"; import { AgentLatency } from "./AgentLatency"; import { AGENT_LOG_LINE_HEIGHT } from "./AgentLogs/AgentLogLine"; import { AgentLogs } from "./AgentLogs/AgentLogs"; -import { useAgentLogs } from "./useAgentLogs"; import { AgentMetadata } from "./AgentMetadata"; import { AgentStatus } from "./AgentStatus"; import { AgentVersion } from "./AgentVersion"; @@ -40,6 +39,7 @@ import { PortForwardButton } from "./PortForwardButton"; import { AgentSSHButton } from "./SSHButton/SSHButton"; import { TerminalLink } from "./TerminalLink/TerminalLink"; import { VSCodeDesktopButton } from "./VSCodeDesktopButton/VSCodeDesktopButton"; +import { useAgentLogs } from "./useAgentLogs"; export interface AgentRowProps { agent: WorkspaceAgent; From 38a8d62270532f2644851439ee1be85b22a35754 Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Wed, 14 May 2025 16:25:39 +0000 Subject: [PATCH 07/10] Apply review comments --- site/src/api/api.ts | 2 +- site/src/modules/resources/useAgentLogs.ts | 10 ++++------ site/src/pages/WorkspacePage/WorkspacePage.test.tsx | 7 +------ 3 files changed, 6 insertions(+), 13 deletions(-) diff --git a/site/src/api/api.ts b/site/src/api/api.ts index a275c25fd6675..5163677fe42b6 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -225,7 +225,7 @@ export const watchWorkspaceAgentLogs = ( ) => { const searchParams = new URLSearchParams({ follow: "true", - after: params?.after ? params?.after.toString() : "", + after: params?.after?.toString() ?? "", }); /** diff --git a/site/src/modules/resources/useAgentLogs.ts b/site/src/modules/resources/useAgentLogs.ts index 30ab3e1d2bec4..d7f810483a693 100644 --- a/site/src/modules/resources/useAgentLogs.ts +++ b/site/src/modules/resources/useAgentLogs.ts @@ -6,14 +6,14 @@ import { useEffect, useState } from "react"; export function useAgentLogs( agent: WorkspaceAgent, enabled: boolean, -): readonly WorkspaceAgentLog[] | undefined { - const [logs, setLogs] = useState(); +): 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. - setLogs(undefined); + setLogs([]); return; } @@ -26,9 +26,7 @@ export function useAgentLogs( console.warn("Error parsing agent log: ", e.parseError); return; } - setLogs((logs) => - logs ? [...logs, ...e.parsedMessage] : [...e.parsedMessage], - ); + setLogs((logs) => [...logs, ...e.parsedMessage]); }); socket.addEventListener("error", (e) => { diff --git a/site/src/pages/WorkspacePage/WorkspacePage.test.tsx b/site/src/pages/WorkspacePage/WorkspacePage.test.tsx index 3c268d90ea8a5..76b25450ad5e8 100644 --- a/site/src/pages/WorkspacePage/WorkspacePage.test.tsx +++ b/site/src/pages/WorkspacePage/WorkspacePage.test.tsx @@ -51,12 +51,7 @@ const renderWorkspacePage = async ( jest .spyOn(API, "getDeploymentConfig") .mockResolvedValueOnce(MockDeploymentConfig); - jest.spyOn(apiModule, "watchWorkspaceAgentLogs").mockImplementation( - () => - new OneWayWebSocket({ - apiRoute: "", - }), - ); + jest.spyOn(apiModule, "watchWorkspaceAgentLogs"); renderWithAuth(, { ...options, From a7e13a0362fe3853f4a342d955f5dec680ff9ada Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Wed, 14 May 2025 16:35:43 +0000 Subject: [PATCH 08/10] Mock websocket on storybook to support OneWayWebsocket --- site/src/testHelpers/storybook.tsx | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/site/src/testHelpers/storybook.tsx b/site/src/testHelpers/storybook.tsx index 84b2f3dd1a4b8..939ff91cb6c6c 100644 --- a/site/src/testHelpers/storybook.tsx +++ b/site/src/testHelpers/storybook.tsx @@ -75,6 +75,8 @@ export const withWebSocket = (Story: FC, { parameters }: StoryContext) => { let callEventsDelay: number; window.WebSocket = class WebSocket { + public readyState = 1; + addEventListener(type: string, callback: CallbackFn) { listeners.set(type, callback); @@ -93,6 +95,8 @@ export const withWebSocket = (Story: FC, { parameters }: StoryContext) => { }, 0); } + removeEventListener(type: string, callback: CallbackFn) {} + close() {} } as unknown as typeof window.WebSocket; From c98a8ddfd019c4cb5f7e14a4f5ba4c9645b12897 Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Wed, 14 May 2025 16:36:24 +0000 Subject: [PATCH 09/10] FMT --- site/src/pages/WorkspacePage/WorkspacePage.test.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/site/src/pages/WorkspacePage/WorkspacePage.test.tsx b/site/src/pages/WorkspacePage/WorkspacePage.test.tsx index 76b25450ad5e8..3f217a86a3aad 100644 --- a/site/src/pages/WorkspacePage/WorkspacePage.test.tsx +++ b/site/src/pages/WorkspacePage/WorkspacePage.test.tsx @@ -33,7 +33,6 @@ import { renderWithAuth, } from "testHelpers/renderHelpers"; import { server } from "testHelpers/server"; -import { OneWayWebSocket } from "utils/OneWayWebSocket"; import WorkspacePage from "./WorkspacePage"; const { API, MissingBuildParameters } = apiModule; From f385ce919ab1954f4c84e2ad69a04d43bf06c86b Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Wed, 14 May 2025 16:43:19 +0000 Subject: [PATCH 10/10] Fix test --- 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 f48de8ebd70d4..8480f756611d2 100644 --- a/site/src/modules/resources/useAgentLogs.test.ts +++ b/site/src/modules/resources/useAgentLogs.test.ts @@ -33,7 +33,7 @@ describe("useAgentLogs", () => { // Disable the hook rerender({ enabled: false }); - expect(result.current).toBeUndefined(); + expect(result.current).toHaveLength(0); // Enable the hook again rerender({ enabled: true });