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/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/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 e46dabdb7ca83..ded5ad11166e6 100644 --- a/site/src/modules/resources/AgentLogs/AgentLogs.tsx +++ b/site/src/modules/resources/AgentLogs/AgentLogs.tsx @@ -1,170 +1,162 @@ -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 } 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 = { + 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; +} + type AgentLogsProps = Omit< ComponentProps, - "children" | "itemSize" | "itemCount" + "children" | "itemSize" | "itemCount" | "itemKey" > & { 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]); + ({ 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 + const logSourceByID = groupLogSourcesById(sources); + const getLogSource = (id: string) => 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); +
+ {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. +

+ )} - 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; - } + logs[index].id || 0} + > + {({ index, style }) => { + const log = logs[index]; + const logSource = getLogSource(log.sourceId); - let nextChangesSource = false; - if (index < logs.length - 1) { - nextChangesSource = - 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 ( - index > 0 && - getLogSource(logs[index - 1].sourceId).id === log.sourceId - ) { - icon = ( -
+ 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 = (
({ - height: nextChangesSource ? "50%" : "100%", - width: 2, - background: theme.experimental.l1.outline, - borderRadius: 2, - })} + role="presentation" + className="size-3.5 mr-2 shrink-0 rounded-full" + style={{ + background: determineScriptDisplayColor( + logSource.display_name, + ), + }} /> - {nextChangesSource && ( + ); + assignedIcon = true; + } + + 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 = ( +
({ - height: 2, - width: "50%", - top: "calc(50% - 2px)", - left: "calc(50% - 1px)", - background: theme.experimental.l1.outline, - borderRadius: 2, - position: "absolute", - })} + // 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 && ( +
+ )} +
+ ); + } - return ( - - {logSource.display_name} - {assignedIcon && ( - -
- No icon specified! - - )} - - } - > - {icon} - - } - /> - ); - }} - + return ( + + {logSource.display_name} + {assignedIcon && ( + +
+ No icon specified! + + )} + + } + > + {icon} + + } + /> + ); + }} + +
); }, ); @@ -172,7 +164,7 @@ export const AgentLogs = forwardRef( // 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", @@ -191,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>; diff --git a/site/src/modules/resources/AgentRow.tsx b/site/src/modules/resources/AgentRow.tsx index 3cf757a15c2ab..666ae30cac01b 100644 --- a/site/src/modules/resources/AgentRow.tsx +++ b/site/src/modules/resources/AgentRow.tsx @@ -19,7 +19,6 @@ import { useCallback, useEffect, useLayoutEffect, - useMemo, useRef, useState, } from "react"; @@ -76,25 +75,9 @@ export const AgentRow: FC = ({ ["starting", "start_timeout"].includes(agent.lifecycle_state) && hasStartupFeatures, ); - const agentLogs = useAgentLogs(agent, showLogs); + const agentLogs = useAgentLogs({ agentId: agent.id, enabled: showLogs }); const logListRef = useRef(null); const logListDivRef = useRef(null); - const startupLogs = useMemo(() => { - const allLogs = agentLogs || []; - - const logs = [...allLogs]; - if (agent.logs_overflowed) { - logs.push({ - 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); useEffect(() => { @@ -106,9 +89,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. @@ -312,7 +295,8 @@ export const AgentRow: FC = ({ width={width} css={styles.startupLogs} onScroll={handleLogScroll} - logs={startupLogs.map((l) => ({ + overflowed={true} + logs={agentLogs.map((l) => ({ id: l.id, level: l.level, output: l.output, diff --git a/site/src/modules/resources/useAgentLogs.test.ts b/site/src/modules/resources/useAgentLogs.test.ts index 186087c871299..4d9a04d01d814 100644 --- a/site/src/modules/resources/useAgentLogs.test.ts +++ b/site/src/modules/resources/useAgentLogs.test.ts @@ -1,60 +1,197 @@ import { renderHook, waitFor } from "@testing-library/react"; import type { WorkspaceAgentLog } from "api/typesGenerated"; -import WS from "jest-websocket-mock"; +import { act } from "react"; 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.skip("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))); - await waitFor(() => { - expect(result.current).toHaveLength(3); +import { + type MockWebSocketServer, + createMockWebSocket, +} from "testHelpers/websockets"; +import { OneWayWebSocket } from "utils/OneWayWebSocket"; +import { + type CreateUseAgentLogsOptions, + createUseAgentLogs, +} from "./useAgentLogs"; + +const millisecondsInOneMinute = 60_000; + +function generateMockLogs( + logCount: number, + 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 + // 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, + created_at: logDate.toISOString(), + level: "info", + output: `Log ${i}`, + source_id: "", + }; + }); +} + +// 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 ServerResult = { current: MockWebSocketServer | undefined }; + +type MountHookOptions = Readonly<{ + initialAgentId: string; + enabled?: boolean; + onError?: CreateUseAgentLogsOptions["onError"]; +}>; + +type MountHookResult = Readonly<{ + rerender: (props: { agentId: string; enabled: boolean }) => void; + serverResult: ServerResult; + + // Note: the `current` property is only "halfway" readonly; the value is + // readonly, but the key is still mutable + hookResult: { current: readonly WorkspaceAgentLog[] }; +}>; + +function mountHook(options: MountHookOptions): MountHookResult { + const { initialAgentId, enabled = true, onError = jest.fn() } = options; + + const serverResult: ServerResult = { current: undefined }; + 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, mockServer] = createMockWebSocket(url); + serverResult.current = mockServer; + return mockSocket; + }, + }); + }, + }); + + const { result: hookResult, rerender } = renderHook( + (props) => useAgentLogs(props), + { initialProps: { agentId: initialAgentId, enabled: enabled } }, + ); + + return { rerender, serverResult, hookResult }; +} + +describe("useAgentLogs", () => { + it("Automatically sorts logs that are received out of order", async () => { + const { hookResult, serverResult } = mountHook({ + initialAgentId: MockWorkspaceAgent.id, }); - // Disable the hook - rerender({ enabled: false }); - await waitFor(() => { - expect(result.current).toHaveLength(0); + const logs = generateMockLogs(10, new Date("september 9, 1999")); + const reversed = logs.toReversed(); + + for (const log of reversed) { + await act(async () => { + serverResult.current?.publishMessage( + new MessageEvent("message", { data: JSON.stringify([log]) }), + ); + }); + } + await waitFor(() => expect(hookResult.current).toEqual(logs)); + }); + + it("Never creates a connection if hook is disabled on mount", () => { + const { serverResult } = mountHook({ + initialAgentId: MockWorkspaceAgent.id, + enabled: false, }); - // Enable the hook again - rerender({ enabled: true }); - await server.connected; - server.send(JSON.stringify(generateLogs(3))); + expect(serverResult.current).toBe(undefined); + }); + + it("Automatically closes the socket connection when the hook is disabled", async () => { + const { serverResult, rerender } = mountHook({ + initialAgentId: MockWorkspaceAgent.id, + }); + + expect(serverResult.current?.isConnectionOpen).toBe(true); + rerender({ agentId: MockWorkspaceAgent.id, enabled: false }); await waitFor(() => { - expect(result.current).toHaveLength(3); + expect(serverResult.current?.isConnectionOpen).toBe(false); }); }); -}); -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: "", - })); -} + it("Automatically closes the old connection when the agent ID changes", () => { + const { serverResult, rerender } = mountHook({ + initialAgentId: MockWorkspaceAgent.id, + }); + + const serverConn1 = serverResult.current; + expect(serverConn1?.isConnectionOpen).toBe(true); + + rerender({ + enabled: true, + agentId: `${MockWorkspaceAgent.id}-new-value`, + }); + + 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 { 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 + enabled: false, + onError, + }); + + const errorEvent = new Event("error"); + await act(async () => serverResult.current?.publishError(errorEvent)); + expect(onError).not.toHaveBeenCalled(); + + rerender({ agentId: MockWorkspaceAgent.id, enabled: true }); + await act(async () => serverResult.current?.publishError(errorEvent)); + expect(onError).toHaveBeenCalledTimes(1); + }); + + // 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, + }); + + // 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), + }); + 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 + // 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), + }); + 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 d7f810483a693..34e32129fd25e 100644 --- a/site/src/modules/resources/useAgentLogs.ts +++ b/site/src/modules/resources/useAgentLogs.ts @@ -1,47 +1,97 @@ -import { watchWorkspaceAgentLogs } from "api/api"; -import type { WorkspaceAgent, WorkspaceAgentLog } from "api/typesGenerated"; +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 function useAgentLogs( - agent: WorkspaceAgent, - enabled: boolean, -): readonly WorkspaceAgentLog[] { - const [logs, setLogs] = useState([]); +export type CreateUseAgentLogsOptions = Readonly<{ + onError: (errorEvent: Event) => void; + createSocket: ( + agentId: string, + params?: WatchWorkspaceAgentLogsParams, + ) => OneWayWebSocket; +}>; - 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. +type UseAgentLogsOptions = Readonly<{ + agentId: string; + enabled?: boolean; +}>; + +export function createUseAgentLogs(createOptions: CreateUseAgentLogsOptions) { + const { createSocket, onError } = createOptions; + + return function useAgentLogs( + 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 + // 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); + } + if (enabled && !prevEnabled) { + setPrevEnabled(true); } - // 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(agentId, { after: 0 }); + socket.addEventListener("message", (e) => { + if (e.parseError) { + console.warn("Error parsing agent log: ", e.parseError); + return; + } + + if (e.parsedMessage.length === 0) { + return; + } + + setLogs((logs) => { + const newLogs = [...logs, ...e.parsedMessage]; + newLogs.sort((l1, l2) => { + const d1 = new Date(l1.created_at).getTime(); + const d2 = new Date(l2.created_at).getTime(); + return d1 - d2; + }); + return newLogs; + }); + }); + + socket.addEventListener("error", (e) => { + onError(e); + socket.close(); + }); + + return () => socket.close(); + }, [agentId, enabled]); + + return logs; + }; } + +// The baseline implementation to use for production +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/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx b/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx index 75849e0790f67..9b7052b650288 100644 --- a/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx +++ b/site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx @@ -7,7 +7,6 @@ import type { import { Alert } from "components/Alert/Alert"; import { ErrorAlert } from "components/Alert/ErrorAlert"; import { Loader } from "components/Loader/Loader"; -import type { Line } from "components/Logs/LogLine"; import { Margins } from "components/Margins/Margins"; import { FullWidthPageHeader, @@ -308,24 +307,20 @@ type AgentLogsContentProps = { }; const AgentLogsContent: FC = ({ agent }) => { - const logs = useAgentLogs(agent, true); - - if (!logs) { - return ; - } - + 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%" /> ); };