fix: fix duplicated agent logs #17806
Conversation
…licated-agent-logs
site/src/api/api.ts
Outdated
| ) => { | ||
| const searchParams = new URLSearchParams({ | ||
| follow: "true", | ||
| after: after.toString(), | ||
| after: params?.after ? params?.after.toString() : "", |
There was a problem hiding this comment.
Could we make this params?.after.toString() ?? ""?
| jest.spyOn(apiModule, "watchWorkspaceAgentLogs").mockImplementation( | ||
| () => | ||
| new OneWayWebSocket({ | ||
| apiRoute: "", |
There was a problem hiding this comment.
Does it make sense to pass in an empty string for the API route? I don't know if we've updated the class since I merged it in originally, but I remember that it was originally made so that you couldn't update the URL after instantiation
A socket like this would always be useless, right?
| ): readonly WorkspaceAgentLog[] | undefined { | ||
| const [logs, setLogs] = useState<WorkspaceAgentLog[]>(); |
There was a problem hiding this comment.
What do you think of making the hook so that it always returns an array, and if we don't have logs, we return an empty array?
I feel like that might help clean up some of the render logic long-term, but I also don't know if we need to differentiate between logs not existing because we haven't gotten anything back from the server yet, versus logs not existing because the server explicitly told us that it doesn't have anything
There was a problem hiding this comment.
It'd also simplify line 29's state update logic
There was a problem hiding this comment.
For now, I don't think it would be a problem. I like the idea 👍
There was a problem hiding this comment.
LGTM and tested in my dogfood env, but deferring to @Parkreiner for approval on the TS side.
Fix #16355