Skip to content

refactor: redesign workspace status on workspaces table #17425

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Apr 17, 2025
Merged

refactor: redesign workspace status on workspaces table #17425

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8212659
refactor: redesign workspace status on workspces table
BrunoQuaresma Apr 16, 2025
43685a6
FMT
BrunoQuaresma Apr 16, 2025
c693ebf
Fix storybook
BrunoQuaresma Apr 16, 2025
f1f43d0
Apply PR comments
BrunoQuaresma Apr 17, 2025
4bdb3aa
Merge branch 'main' of https://github.com/coder/coder into bq/refacto…
BrunoQuaresma Apr 17, 2025
bbe9d8f
Fix merging conflict
BrunoQuaresma Apr 17, 2025
236f9dd
Merge branch 'main' into bq/refactor-status-column
BrunoQuaresma Apr 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion site/src/components/Badge/Badge.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,12 @@ export const badgeVariants = cva(
default:
"border-transparent bg-surface-secondary text-content-secondary shadow",
warning:
"border-transparent bg-surface-orange text-content-warning shadow",
"border border-solid border-border-warning bg-surface-orange text-content-warning shadow",
destructive:
"border border-solid border-border-destructive bg-surface-red text-content-highlight-red shadow",
Comment on lines +20 to +22
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to say that border-solid is set automatically via border, but I guess it's good to be explicit here

Copy link
Collaborator Author

@BrunoQuaresma BrunoQuaresma Apr 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not 😢 . The reason is because we don't use the TailwindCSS pre-flight right now because of MUI - to avoid overlaping resets and styles.

},
size: {
xs: "text-2xs font-regular h-5 [&_svg]:hidden rounded px-1.5",
sm: "text-2xs font-regular h-5.5 [&_svg]:size-icon-xs",
md: "text-xs font-medium [&_svg]:size-icon-sm",
},
Expand Down
6 changes: 5 additions & 1 deletion site/src/index.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,12 @@
--surface-grey: 240 5% 96%;
--surface-orange: 34 100% 92%;
--surface-sky: 201 94% 86%;
--surface-red: 0 93% 94%;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a good way to get up to speed on how we have colors set up right now, especially for themes? This red is pretty bright and desaturated, and I'm wondering how it looks against the light theme

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you can check them in Figma - You may need to learn how to see the tokens. I'm not sure if we are following this, but in the past I think we decided to use TailwindCSS colors as reference.

--border-default: 240 6% 90%;
--border-success: 142 76% 36%;
--border-destructive: 0 84% 60%;
--border-hover: 240, 5%, 34%;
--border-warning: 27 96% 61%;
--border-hover: 240 5% 34%;
--overlay-default: 240 5% 84% / 80%;
--radius: 0.5rem;
--highlight-purple: 262 83% 58%;
Expand Down Expand Up @@ -65,9 +67,11 @@
--surface-grey: 240 6% 10%;
--surface-orange: 13 81% 15%;
--surface-sky: 204 80% 16%;
--surface-red: 0 75% 15%;
--border-default: 240 4% 16%;
--border-success: 142 76% 36%;
--border-destructive: 0 91% 71%;
--border-warning: 31 97% 72%;
--border-hover: 240, 5%, 34%;
--overlay-default: 240 10% 4% / 80%;
--highlight-purple: 252 95% 85%;
Expand Down
12 changes: 5 additions & 7 deletions site/src/modules/workspaces/WorkspaceDormantBadge/WorkspaceDormantBadge.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
import AutoDeleteIcon from "@mui/icons-material/AutoDelete";
import RecyclingIcon from "@mui/icons-material/Recycling";
import Tooltip from "@mui/material/Tooltip";
import type { Workspace } from "api/typesGenerated";
import { Pill } from "components/Pill/Pill";
import { Badge } from "components/Badge/Badge";
import { formatDistanceToNow } from "date-fns";
import type { FC } from "react";

Expand Down Expand Up @@ -35,9 +33,9 @@ export const WorkspaceDormantBadge: FC<WorkspaceDormantBadgeProps> = ({
</>
}
>
<Pill role="status" icon={<AutoDeleteIcon />} type="error">
<Badge role="status" variant="destructive" size="xs">
Deletion Pending
</Pill>
</Badge>
</Tooltip>
) : (
<Tooltip
Expand All @@ -50,9 +48,9 @@ export const WorkspaceDormantBadge: FC<WorkspaceDormantBadgeProps> = ({
</>
}
>
<Pill role="status" icon={<RecyclingIcon />} type="warning">
<Badge role="status" variant="warning" size="xs">
Dormant
</Pill>
</Badge>
</Tooltip>
);
};
96 changes: 66 additions & 30 deletions site/src/pages/WorkspacesPage/WorkspacesTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ import { AvatarData } from "components/Avatar/AvatarData";
import { AvatarDataSkeleton } from "components/Avatar/AvatarDataSkeleton";
import { InfoTooltip } from "components/InfoTooltip/InfoTooltip";
import { Stack } from "components/Stack/Stack";
import {
StatusIndicator,
StatusIndicatorDot,
type StatusIndicatorProps,
} from "components/StatusIndicator/StatusIndicator";
import {
Table,
TableBody,
Expand All @@ -25,19 +30,26 @@ import {
TableLoaderSkeleton,
TableRowSkeleton,
} from "components/TableLoader/TableLoader";
import dayjs from "dayjs";
import relativeTime from "dayjs/plugin/relativeTime";
import { useClickableTableRow } from "hooks/useClickableTableRow";
import { useDashboard } from "modules/dashboard/useDashboard";
import { WorkspaceAppStatus } from "modules/workspaces/WorkspaceAppStatus/WorkspaceAppStatus";
import { WorkspaceDormantBadge } from "modules/workspaces/WorkspaceDormantBadge/WorkspaceDormantBadge";
import { WorkspaceOutdatedTooltip } from "modules/workspaces/WorkspaceOutdatedTooltip/WorkspaceOutdatedTooltip";
import { WorkspaceStatusBadge } from "modules/workspaces/WorkspaceStatusBadge/WorkspaceStatusBadge";
import { LastUsed } from "pages/WorkspacesPage/LastUsed";
import { type FC, type ReactNode, useMemo } from "react";
import { useNavigate } from "react-router-dom";
import { cn } from "utils/cn";
import { getDisplayWorkspaceTemplateName } from "utils/workspace";
import {
type GetWorkspaceDisplayStatusType,
getDisplayWorkspaceStatus,
getDisplayWorkspaceTemplateName,
lastUsedMessage,
} from "utils/workspace";
import { WorkspacesEmpty } from "./WorkspacesEmpty";

dayjs.extend(relativeTime);

export interface WorkspacesTableProps {
workspaces?: readonly Workspace[];
checkedWorkspaces: readonly Workspace[];
Expand Down Expand Up @@ -125,8 +137,7 @@ export const WorkspacesTable: FC<WorkspacesTableProps> = ({
</TableHead>
{hasAppStatus && <TableHead className="w-2/6">Activity</TableHead>}
<TableHead className="w-2/6">Template</TableHead>
<TableHead className="w-1/6">Last used</TableHead>
<TableHead className="w-1/6">Status</TableHead>
<TableHead className="w-2/6">Status</TableHead>
<TableHead className="w-0" />
</TableRow>
</TableHeader>
Expand Down Expand Up @@ -248,26 +259,7 @@ export const WorkspacesTable: FC<WorkspacesTableProps> = ({
/>
</TableCell>

<TableCell>
<LastUsed lastUsedAt={workspace.last_used_at} />
</TableCell>

<TableCell>
<div className="flex items-center gap-2">
<WorkspaceStatusBadge workspace={workspace} />
{workspace.latest_build.status === "running" &&
!workspace.health.healthy && (
<InfoTooltip
type="warning"
title="Workspace is unhealthy"
message="Your workspace is running but some agents are unhealthy."
/>
)}
{workspace.dormant_at && (
<WorkspaceDormantBadge workspace={workspace} />
)}
</div>
</TableCell>
<WorkspaceStatusCell workspace={workspace} />

<TableCell>
<div className="flex pl-4">
Expand Down Expand Up @@ -345,14 +337,11 @@ const TableLoader: FC<TableLoaderProps> = ({ canCheckWorkspaces }) => {
<TableCell className="w-2/6">
<AvatarDataSkeleton />
</TableCell>
<TableCell className="w-1/6">
<Skeleton variant="text" width="75%" />
</TableCell>
<TableCell className="w-1/6">
<TableCell className="w-2/6">
<Skeleton variant="text" width="75%" />
</TableCell>
<TableCell className="w-0">
<Skeleton variant="text" width="75%" />
<Skeleton variant="text" width="25%" />
</TableCell>
</TableRowSkeleton>
</TableLoaderSkeleton>
Expand All @@ -362,3 +351,50 @@ const TableLoader: FC<TableLoaderProps> = ({ canCheckWorkspaces }) => {
const cantBeChecked = (workspace: Workspace) => {
return ["deleting", "pending"].includes(workspace.latest_build.status);
};

type WorkspaceStatusCellProps = {
workspace: Workspace;
};

const variantByStatusType: Record<
GetWorkspaceDisplayStatusType,
StatusIndicatorProps["variant"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if it's worth splitting off the variant property into a separate type, and exporting that instead. Chaining props like this feels like fragile, over-coupled dependencies

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chaining props like this feels like fragile, over-coupled dependencies

I think this makes sense when the defined type is dependent on the another one. So, if I change the props onStatusIndicator it will be reflect on variantByStatusTypewithout any extra effort. Besides that, I think having something like:

type StatusIndicatorVariant = StatusIndicatorProps["variant"]

It just works as an alias IMO.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting more thoughts on it, I just think types dependency may warn us about components being too coupled, which is not always bad, but definitely something to evaluate.

> = {
active: "pending",
inactive: "inactive",
success: "success",
error: "failed",
danger: "warning",
};

const WorkspaceStatusCell: FC<WorkspaceStatusCellProps> = ({ workspace }) => {
const { text, type } = getDisplayWorkspaceStatus(
workspace.latest_build.status,
workspace.latest_build.job,
);

return (
<TableCell>
<div className="flex flex-col">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why this is a flex column? I would expect that when there isn't any gap set, you would get the same result if you were to remove the classes entirely

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is true, let me check.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it has to be because of the inline elements that are inside like the span.

<StatusIndicator variant={variantByStatusType[type]}>
<StatusIndicatorDot />
{text}
{workspace.latest_build.status === "running" &&
!workspace.health.healthy && (
<InfoTooltip
type="warning"
title="Workspace is unhealthy"
message="Your workspace is running but some agents are unhealthy."
/>
)}
{workspace.dormant_at && (
<WorkspaceDormantBadge workspace={workspace} />
)}
</StatusIndicator>
<span className="text-xs font-medium text-content-secondary ml-6">
{lastUsedMessage(workspace.last_used_at)}
</span>
</div>
</TableCell>
);
};
25 changes: 25 additions & 0 deletions site/src/utils/workspace.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ export const getDisplayWorkspaceStatus = (
case undefined:
return {
text: "Loading",
type: "active",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't flag line 174, but could we get rid of the as const declarations, and add an explicit return type to the function? The current setup won't provide many safety nets if we accidentally add an extra field or make a typo

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would also remove the need for the ReturnType-derived type on line 245

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will take a look on those 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done ✅

icon: <PillSpinner />,
} as const;
case "running":
Expand Down Expand Up @@ -241,6 +242,10 @@ export const getDisplayWorkspaceStatus = (
}
};

export type GetWorkspaceDisplayStatusType = ReturnType<
typeof getDisplayWorkspaceStatus
>["type"];

export const hasJobError = (workspace: TypesGen.Workspace) => {
return workspace.latest_build.job.error !== undefined;
};
Expand Down Expand Up @@ -307,3 +312,23 @@ const FALLBACK_ICON = "/icon/widgets.svg";
export const getResourceIconPath = (resourceType: string): string => {
return BUILT_IN_ICON_PATHS[resourceType] ?? FALLBACK_ICON;
};

export const lastUsedMessage = (lastUsedAt: string | Date) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can a return type be added here?

const t = dayjs(lastUsedAt);
const now = dayjs();
let message = t.fromNow();

if (t.isAfter(now.subtract(1, "hour"))) {
message = "Now";
} else if (t.isAfter(now.subtract(3, "day"))) {
Copy link
Contributor

@bcpeinhardt bcpeinhardt Apr 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I crazy or do these middle branches not do anything since message already defaults to t.fromNow() on line 319?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They do because the conditions are not using the message value to compare things... maybe I'm missing something 🤔 but in my tests and storybook things are working as expected - I guess haha

message = t.fromNow();
} else if (t.isAfter(now.subtract(1, "month"))) {
message = t.fromNow();
} else if (t.isAfter(now.subtract(100, "year"))) {
message = t.fromNow();
} else {
message = "Never";
}

return message;
};
2 changes: 2 additions & 0 deletions site/tailwind.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,12 @@ module.exports = {
grey: "hsl(var(--surface-grey))",
orange: "hsl(var(--surface-orange))",
sky: "hsl(var(--surface-sky))",
red: "hsl(var(--surface-red))",
},
border: {
DEFAULT: "hsl(var(--border-default))",
destructive: "hsl(var(--border-destructive))",
warning: "hsl(var(--border-warning))",
success: "hsl(var(--border-success))",
hover: "hsl(var(--border-hover))",
},
Expand Down
Loading