fix(site): update useAgentLogs to make it more testable and add more tests
#19126
refactor: switch to spy setup
- Loading branch information
commit dac38281e572293b752bfab06c7db554fca6e4ce
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,97 +1,73 @@ | ||
| import { watchWorkspaceAgentLogs } from "api/api"; | ||
| import type { WorkspaceAgentLog } from "api/typesGenerated"; | ||
| import { displayError } from "components/GlobalSnackbar/utils"; | ||
| import { useEffect, useState } from "react"; | ||
|
|
||
| type UseAgentLogsOptions = Readonly<{ | ||
| agentId: string; | ||
| enabled?: boolean; | ||
| }>; | ||
|
|
||
| export function useAgentLogs( | ||
| options: UseAgentLogsOptions, | ||
| ): readonly WorkspaceAgentLog[] { | ||
| const { agentId, enabled = true } = options; | ||
| const [logs, setLogs] = useState<readonly WorkspaceAgentLog[]>([]); | ||
|
|
||
| // 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([]); | ||
| setPrevEnabled(false); | ||
| } | ||
| if (enabled && !prevEnabled) { | ||
| setPrevEnabled(true); | ||
| } | ||
|
|
||
| useEffect(() => { | ||
| if (!enabled) { | ||
| 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 might not be worth it. | ||
| const socket = watchWorkspaceAgentLogs(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) => { | ||
| console.error("Error in agent log socket: ", e); | ||
| displayError( | ||
| "Unable to watch agent logs", | ||
| "Please try refreshing the browser", | ||
| ); | ||
| socket.close(); | ||
| }); | ||
|
|
||
| return () => socket.close(); | ||
| }, [agentId, enabled]); | ||
|
|
||
| return logs; | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we stick with this we could just use
createRefandReact.RefObject<MockWebSocketServer>instead of making our own type hereThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are not the same.
createRefis for interfacing with class components, and creates a mutable object that's still part of React's data flowThis 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