fix(site): update `useAgentLogs` to make it more testable and add more tests #19126

Parkreiner · 2025-08-02T20:59:24Z

Changes made

Updated how useAgentLogs was defined to make it easier to inject specific data dependencies (basically making the hook more unit-testable)
Simplified the hook API to limit the amount of scope of data it needs to work
Added more test cases, and re-enabled the one test case we had previously disabled
Extracted our mock websocket code into a separate file, and added more methods to it
Updated all runtime code to accommodate new changes

…mes/logs-flake

Parkreiner · 2025-08-02T21:01:22Z

site/src/hooks/useEmbeddedMetadata.ts

 		const stableMetadataResult = useMemo<UseEmbeddedMetadataResult>(() => {
 			return {
 				metadata,
 				clearMetadataByKey: manager.clearMetadataByKey,
 			};
-		}, [metadata]);
+		}, [manager, metadata]);


A case where realistically, the value of manager won't ever change, but it technically can, since function parameters are reassignable. So better to add it to the dependencies for correctness

Parkreiner · 2025-08-02T21:02:08Z

site/src/modules/resources/AgentRow.tsx

 	const logListRef = useRef<List>(null);
 	const logListDivRef = useRef<HTMLDivElement>(null);
-	const startupLogs = useMemo(() => {
-		const allLogs = agentLogs || [];


agentLogs is no longer nullable, so a bunch of logic can be removed

Parkreiner · 2025-08-02T21:04:57Z

site/src/modules/resources/useAgentLogs.test.ts

@@ -1,60 +1,147 @@
 import { renderHook, waitFor } from "@testing-library/react";


Enough changed with the test file that it honestly might not even be worth looking at the old version of the code

Parkreiner · 2025-08-04T03:01:45Z

site/src/testHelpers/websockets.ts

+	publishError: (event: Event) => void;
+	publishClose: (event: CloseEvent) => void;
+	publishOpen: (event: Event) => void;
+	readonly isConnectionOpen: boolean;


Setup is 99% the same from the old file, just with the addition of this getter method

aslilac

I think this mock WebSocket implementation needs some tests of its own, and is probably worth its own PR as well. @jaaydenh has been working on a similar refactor of some other tests that I think uses a copy of this WebSocket mock. It'd be nice to sync on that so that we don't duplicate effort, and then we can get everyones tests reviewed. 😄

Parkreiner · 2025-08-04T18:17:27Z

@aslilac Yeah, totally fair. I'll check with Jaayden

Needed for #19126 and #18679 ## Changes made - Moved `createWebSocket` to dedicated file and addressed edge cases for making it a reliable mock - Added test cases to validate mock functionality

aslilac

I'd love to have a story for the logs_overflowed state

(sorry if this is asking a lot, I just keep seeing things that'd be nice to clean up everywhere)

aslilac · 2025-08-07T21:33:55Z

site/src/modules/resources/AgentRow.tsx

+		}
+		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(),


we're leaking in here. I feel like this should be...

Suggested change

created_at: new Date().toISOString(),

created_at: agentLogs.at(-1).created_at,

since that's probably more "correct" and deterministic

aslilac · 2025-08-07T21:39:09Z

site/src/modules/resources/useAgentLogs.test.ts

+
+function generateMockLogs(
+	logCount: number,
+	baseDate = new Date(),


Suggested change

baseDate = new Date(),

baseDate = new Date("Thu, 07 Aug 2025 21:38:12 GMT"),

as an example, how about just right now

aslilac · 2025-08-07T22:16:27Z

site/src/modules/resources/AgentRow.tsx

+		}
+		return [
+			...agentLogs,
+			{


also I know it was already like this, but is shoving a log entry on the end really the best way to do this?

this startupLogs value eventually gets passed to AgentLogs below. what if we just passed agentLogs in unmodified and added an overflowed prop that shows the warning?

site/src/modules/resources/useAgentLogs.test.ts

aslilac · 2025-08-07T22:35:55Z

site/src/modules/resources/useAgentLogs.ts

-import type { WorkspaceAgent, WorkspaceAgentLog } from "api/typesGenerated";
+import {
+	type WatchWorkspaceAgentLogsParams,
+	watchWorkspaceAgentLogs,


I wonder if rather than all of this factory stuff we could just mock watchWorkspaceAgentLogs. I think that might make it easier to get the publisher value too, but can't say for sure.

site/src/modules/resources/useAgentLogs.test.ts

aslilac · 2025-08-07T22:59:58Z

site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx

-		return <Loader />;
-	}
-
+	const logs = useAgentLogs(agent.id, true);


another "I know it was already like this" moment but nondescript booleans like this are dreadful. I'd love to see this argument go from boolean to { enabled: boolean = true } (similar to what react-query does, default to on, optimized for the happy path)

aslilac · 2025-08-07T23:12:25Z

site/src/modules/resources/useAgentLogs.test.ts

+function mountHook(options: MountHookOptions): MountHookResult {
+	const { initialAgentId, enabled = true, onError = jest.fn() } = options;
+
+	const serverResult: ServerResult = { current: undefined };


if we stick with this we could just use createRef and React.RefObject<MockWebSocketServer> instead of making our own type here

Those are not the same. createRef is for interfacing with class components, and creates a mutable object that's still part of React's data flow

This setup is ejecting a value created during React's lifecycle into an outside function. React has absolutely no idea that this exists, but it's how a lot of React testing tools work under the hood. I get the aversion to having more bespoke types, but switching to the built-in type would be lying about it actually is

Parkreiner · 2025-08-15T05:09:08Z

@aslilac I think I have this pretty close to done:

The whole hook setup was changed to remove the curried function for useAgentLogs, and the tests have been swapped to jest.spyOn
The actual components have been updated to be aware of an overflowed property for AgentLogs, and there's a story in place to show the state

The only thing I'm not super clear on is how to style it. I really think that the overflow styling should be represented as a tooltip. The actual message is super short, so we want to constrain the width pretty aggressively for good typography, and a tooltip naturally lends itself well to having those constraints without affecting the layout of the main container

The only thing is, I don't really know how to style it. I tried going through Storybook, but their search is so bad that it made it hard to see if we have any components that have the "alert tooltip" behavior already. Do you know of any existing components that fit the bill?

Parkreiner added 15 commits May 23, 2025 20:03

wip: commit progress on test update

d6e00c3

refactor: update useAgentLogs tests as unit tests

43d0ca8

docs: rewrite comment for clarity

727bddd

fix: remove unnecessary type

d46d144

fix: make sure logs have different timestamps

91a6fc1

fix: add different dates to reduce risk of false positives

ecbe7b0

Merge branch 'main' into mes/logs-flake

d13bcdc

Merge branch 'main' into mes/logs-flake

0f21097

refactor: decrease coupling

abd6553

wip: commit progress on updating flake

bade97a

Merge branch 'mes/logs-flake' of https://github.com/coder/coder into …

e11fefd

…mes/logs-flake

fix: get all tests passing

bc3d095

chore: add one more test case

550d09e

fix: update type mismatches

cc7e632

refactor: clean up some code

79c7ffd

Parkreiner self-assigned this Aug 2, 2025

Parkreiner requested a review from aslilac as a code owner August 2, 2025 20:59

Parkreiner commented Aug 2, 2025

View reviewed changes

Parkreiner added 4 commits August 2, 2025 21:41

fix: make testing boundaries more formal

43a0d3a

fix: remove premature optimization

982d3e1

fix: update setup

41c5a12

fix: update state sync logic

42cb73b

Parkreiner changed the title ~~fix: update useAgentLogs to make it more testable and add more tests~~ fix(site): update useAgentLogs to make it more testable and add more tests Aug 4, 2025

Parkreiner commented Aug 4, 2025

View reviewed changes

Merge branch 'main' into mes/logs-flake

3a5f7bb

aslilac reviewed Aug 4, 2025

View reviewed changes

Parkreiner added 2 commits August 4, 2025 18:09

fix: update wonky types

35a40df

Merge branch 'main' into mes/logs-flake

306dbc7

Parkreiner mentioned this pull request Aug 5, 2025

fix(site): add tests for createMockWebSocket #19172

Merged

Merge branch 'main' into mes/logs-flake

f49e55a

Parkreiner requested a review from aslilac August 7, 2025 21:00

Parkreiner added 2 commits August 7, 2025 21:08

fix: update tests

c2fc772

fix: format

2cabd85

aslilac reviewed Aug 7, 2025

View reviewed changes

Parkreiner added 13 commits August 9, 2025 05:16

Merge branch 'main' into mes/logs-flake

855f3ca

fix: apply initial feedback

453894b

wip: commit refactoring progress

c9f2b12

refactor: update assignment

80865fe

wip: prepare to change indents

f930b29

fix: update keygen logic

a311ac9

chore: add basic overflow message

5657536

chore: swap to tailwind

6547a2f

wip: commit progress

c818aec

Merge branch 'main' into mes/logs-flake

30bb008

refactor: switch to spy setup

dac3828

fix: get scaffolding for new AgentLogs in place

7a013d3

fix: remove bad import

b71d051

		@@ -1,60 +1,147 @@
		import { renderHook, waitFor } from "@testing-library/react";

	created_at: new Date().toISOString(),
	created_at: agentLogs.at(-1).created_at,

	baseDate = new Date(),
	baseDate = new Date("Thu, 07 Aug 2025 21:38:12 GMT"),

fix(site): update useAgentLogs to make it more testable and add more tests #19126

Are you sure you want to change the base?

fix(site): update useAgentLogs to make it more testable and add more tests #19126

Conversation

Parkreiner commented Aug 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes made

Uh oh!

Parkreiner Aug 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Parkreiner Aug 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aslilac left a comment

Choose a reason for hiding this comment

Uh oh!

Parkreiner commented Aug 4, 2025

Uh oh!

aslilac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Parkreiner commented Aug 15, 2025

Uh oh!

Uh oh!

fix(site): update `useAgentLogs` to make it more testable and add more tests #19126

fix(site): update `useAgentLogs` to make it more testable and add more tests #19126

Parkreiner commented Aug 2, 2025 •

edited

Loading

Parkreiner Aug 2, 2025 •

edited

Loading

Parkreiner Aug 2, 2025 •

edited

Loading