-
Notifications
You must be signed in to change notification settings - Fork 926
feat(site): add workspace timings #15068
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Related to coder/internal#44 |
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.
Fantastic work! I'll leave the frontend review to the folks who know best. I'm really excited to see this be added.
Question: when a script occurs, does the error indicator sit to the left of it in the timeline overview view, or is it always aligned to the far left?
<Tooltip | ||
title={ | ||
<TooltipTitle> | ||
Script exited with {t.exitCode} code |
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.
Script exited with {t.exitCode} code | |
Script exited with code {t.exitCode} |
Is there a way to make the exit code more visually distinct? Perhaps bold, with a different colour, monospace font, etc?
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.
This is really cool!!
site/src/api/queries/workspaces.ts
Outdated
|
||
export const timings = (workspaceId: string) => { | ||
return { | ||
queryKey: ["workspaces", workspaceId, "timings"], |
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.
Should this include the build ID or something? I think this means if a workspace gets rebuilt it could cache the same timings because the workspace ID is the same, and I suppose you might have to refresh the page to get the new timings?
Somewhat related, when I create a new workspace I have to refresh or navigate away and back again before the timings load.
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.
Good catch!
scale: number; | ||
}; | ||
|
||
export const XAxis: FC<XAxisProps> = ({ ticks, scale, ...htmlProps }) => { |
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.
I am not sure if this component is related, but I found that if I:
- Stop a running workspace, wait for it to stop
- Click "Workspaces"
- Go back into my now-stopped workspace
Then the timings run off edge of the component and I am not able to see them. It seems to fix itself if I navigate away and come back again. Weirdly, the same issue does not appear to happen if I start a stopped workspace then navigate away and back again, it only happens when I stop my workspace.
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.
Another thing is if I resize my screen then the timings and legend get cut off, idk if it is worth adding resizing logic but maybe we can just throw in a horizontal scrollbar.
// The X axis should occupy all available space. If there is extra space, | ||
// increase the column width accordingly. Use a CSS variable to propagate the | ||
// value to the child components. |
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.
Not important, but this comment has a bunch of space at the beginning 😄
minWidth: 24, | ||
// Increase hover area | ||
position: "relative", | ||
"&::after": { |
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.
Neat trick!
overflow: "hidden", | ||
textOverflow: "ellipsis", |
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.
Could we add a hover, maybe a tooltip, in case someone wants to see the full name?
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.
The user can already do it by hovering over the bar.
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.
Ohhh I see. My instinct was to hover the name.
const scales = [100, 500, 5_000]; | ||
|
||
const pickScale = (totalTime: number): number => { | ||
const reversedScales = scales.slice().reverse(); |
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.
Looks like this is the only place where scales
is used? We could just define scales
already reversed.
return reversedScales[0]; | ||
}; | ||
|
||
export const makeTicks = (time: number) => { |
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.
Do we need to be worried about small screens? These ticks can get really cramped in a small screen. I wonder if we should just set a minimum width and add horizontal scrollbars.
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.
Hmm, it should use horizontal scrollbars already, but I will investigate that. We should be concerned about tablet viewports.
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.
Found the issue and it is fixed!
{Array.from({ length: nOfBlocks }, (_, i) => i + 1).map((n) => ( | ||
<div key={n} css={styles.block} style={{ minWidth: blockSize }} /> | ||
))} |
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.
Hopefully this is the right spot, but one thing that threw me off is that my template has five resources, and I saw five blue blocks, but only three of them show up in the chart when I click into the bar. Edit: I think because we filter Coder resources in the sub-chart but not the top-level chart.
It was also a little surprising that the blue blocks were all even width instead of matching up with their timings.
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.
Fixed!
* Color scheme for the bar. If not passed the default gray color will be | ||
* used. |
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.
I kept thinking the gray box was a checkbox 😆
I wonder if we could change the shape or default color a bit, but then again I might be the only one dumb enough not to realize it was the legend color.
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.
lol, @chrifro wdyt?
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.
You are referring to the "state refresh" color right?
We could totally change that color. Either to orange if we also want to draw more attention to the "state refresh"
orange-bg: #431407
orange-stroke: #FDBA74 (we use the same color already on different views)
or if we want to keep it more subtle to grey
grey-bg: #18181B
grey-stroke: #71717A
something to keep in mind: the colors will likely slightly change as part of #14780. Then the difference between the newly introduced grey colors and the bg will be more obvious (see screenshot).
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.
Sorry for the late reply, yes that is what I meant! When there are two or more, it is pretty obvious they are a legend, but with just one I thought it was supposed to be a checkbox, in particular the dark or grey ones. This might just be me being dumb though.
> | ||
{/** We only want to expand stages with more than one resource */} | ||
{t.resources > 1 ? ( | ||
<ClickableBar |
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.
I am seeing a case where my plan
stage is clickable, but there are no timings or rows when I click in. The resources are data.coder_provisioner.me
, data.coder_workspace_owner.me
, and data.coder_workspace.me
.
I think because we are filtering out Coder resources in the resource chart, but not in the top-level chart. Not sure if that means we should also filter them out in the top-level, or if we should include rows for them.
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.
Fixed.
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.
Looks great @BrunoQuaresma 👏 ✨
One comment non-blocking:
- what do you think of adjusting the height of the whole drop-down to fit the content? Currently, there is a lot more bottom spacing on the initial view vs the different stages (see screenshots).
Consequences: either the box height gets smaller for the stage views as well or the box height changes for the stage views. Thoughts?


@chrifro Hm... I see what you’re trying to achieve, but I usually prefer to have elements with dynamic data and unpredictable sizes set to a fixed height to avoid layout shifting. In my opinion, we could release the component as it is and see if we receive any feedback from users on this. Does that sound like a good plan? |
) : ( | ||
<KeyboardArrowDown css={{ fontSize: 16, marginRight: 16 }} /> | ||
)} | ||
<span>Provisioning time</span> |
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.
Nit: this is not just provisioning time since it also includes agent timings.
We should use a more descriptive name, like "Build timeline".
Demo:
Screen.Recording.2024-10-18.at.15.48.28.mp4