feat(site): show previous agent scripts logs #12233
Conversation
There was a problem hiding this comment.
This file was extracted from the AgentRow component to be reused
BrunoQuaresma
requested review from
a team and
Parkreiner
and removed request for
a team
BrunoQuaresma
changed the title
BrunoQuaresma
changed the title
There was a problem hiding this comment.
Big fan of the file changes – looks really clean now.
Once this PR is out, would you be okay with me adding some accessibility code? Mainly want to add some stuff from the APG example for tabs
There was a problem hiding this comment.
100% I'm not attached to my code at all 😆
| color: theme.palette.text.primary, | ||
| position: "relative", | ||
|
|
||
| "&:before": { |
There was a problem hiding this comment.
Is this for the accent underline? I'm wondering if it might be more direct to add a bottom border to all the tabs.
As in, give all the tabs the same border thickness and line style, but not the same color. If the tab's active, it could use the primary accent color, and if not, it could use the same color as the background, and blend them together
There was a problem hiding this comment.
Yes. In the way you suggested it does not vertically align the content correctly since it is off by 1px at the bottom so you also would need to add a border to the top which starts to make the CSS more "complicated" IMO. I just think the before is easier to understand and implement.
| const logSourceByID = useMemo(() => { | ||
| const sourcesById: { [id: string]: WorkspaceAgentLogSource } = {}; | ||
| for (const source of sources) { | ||
| sourcesById[source.id] = source; | ||
| } | ||
| return sourcesById; | ||
| }, [sources]); |
There was a problem hiding this comment.
I know this file's using React Window for list virtualization, but do we know how big the dataset is expected to get? I'm wondering if the useMemo call here is actually doing all that much
There was a problem hiding this comment.
do we know how big the dataset is expected to get?
It can be a lot. It depends on the user's use case. It can have 3+ scripts, and each script can have more than +200 log lines for example.
I'm wondering if the useMemo call here is actually doing all that much
Didn't understand
| @@ -128,6 +143,23 @@ export const WorkspaceBuildPageView: FC<WorkspaceBuildPageViewProps> = ({ | |||
| </Sidebar> | |||
|
|
|||
| <div css={{ height: "100%", overflowY: "auto", width: "100%" }}> | |||
| <Tabs active={tab.value}> | |||
| <TabsList> | |||
| <TabLink to={`?${LOGS_TAB_KEY}=build`} value="build"> | |||
There was a problem hiding this comment.
I'm wondering if there's a better way to expose the props for this component. Something like this:
<Tabs active={tab.value} searchParamsKey="logs">
<TabsList>
<TabLink value="build" />
</TabsList>
</Tabs>If searchParamsKey is defined, then Tabs would treat all TabLinks as relative, and pass down information to them to let them know to format their links differently. And the to value for each link wouldn't be necessary, because you'd be able to piece together the full param from the container's key and the link's value
There was a problem hiding this comment.
I would not like to make Tabs attached to search parameters for now.
| const agents = build.resources | ||
| .filter((r) => r.agents) | ||
| .flatMap((r) => r.agents) as WorkspaceAgent[]; | ||
| const selectedAgent = agents.find((a) => a.id === tab.value); |
There was a problem hiding this comment.
Could this be inlined inside the JSX, since it's only used in one spot?
There was a problem hiding this comment.
Yeap, I just find it easier to read. Wdyt?
PS: If the workspace has multiple agents, they will be displayed as tabs like coder_agent.a, coder_agent.b, and so on.
Close #7409
Demo:
Screen.Recording.2024-02-20.at.15.09.45.mov