Skip to content

chore: refactor frontend to use workspace status directly #4361

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 11 commits into from
Oct 5, 2022
Merged

chore: refactor frontend to use workspace status directly #4361

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
330f2af
Add/update copy
presleyp Oct 4, 2022
c38352d
Update mocks
presleyp Oct 4, 2022
7dc2ef2
Handle disabled button labels separately
presleyp Oct 4, 2022
56f927e
Use workspace status directly, use i18n
presleyp Oct 4, 2022
a363534
Update stories and tests
presleyp Oct 4, 2022
1992e08
Fix optimistic update in xservice to use status, pending
presleyp Oct 4, 2022
5072691
Merge branch 'main' into ws-status/presleyp
presleyp Oct 4, 2022
d06c432
Rename started to running in story
presleyp Oct 4, 2022
b9829b5
Fix deletion banner conditional
presleyp Oct 4, 2022
23c607e
Send label to disabled button
presleyp Oct 5, 2022
5e10ddf
Refactor workspace actions
presleyp Oct 5, 2022
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
Prev Previous commit
Next Next commit
Use workspace status directly, use i18n
  • Loading branch information
@presleyp
presleyp committed Oct 4, 2022
commit 56f927e11bbabafbe64516e5853b5cba9ce835f6
35 changes: 14 additions & 21 deletions site/src/components/DropdownButton/ActionCtas.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,74 +5,66 @@ import CloudQueueIcon from "@material-ui/icons/CloudQueue"
import CropSquareIcon from "@material-ui/icons/CropSquare"
import DeleteOutlineIcon from "@material-ui/icons/DeleteOutline"
import PlayCircleOutlineIcon from "@material-ui/icons/PlayCircleOutline"
import { WorkspaceStatus } from "api/typesGenerated"
import { LoadingButton } from "components/LoadingButton/LoadingButton"
import { FC } from "react"
import { useTranslation } from "react-i18next"
import { combineClasses } from "util/combineClasses"
import { WorkspaceStateEnum } from "util/workspace"
import { WorkspaceActionButton } from "../WorkspaceActionButton/WorkspaceActionButton"

export const Language = {
start: "Start",
stop: "Stop",
delete: "Delete",
cancel: "Cancel",
update: "Update",
updating: "Updating",
// these labels are used in WorkspaceActions.tsx
starting: "Starting...",
stopping: "Stopping...",
deleting: "Deleting...",
}

interface WorkspaceAction {
handleAction: () => void
}

export const UpdateButton: FC<React.PropsWithChildren<WorkspaceAction>> = ({ handleAction }) => {
const styles = useStyles()
const { t } = useTranslation("workspacePage")

return (
<Button className={styles.actionButton} startIcon={<CloudQueueIcon />} onClick={handleAction}>
{Language.update}
{t("actionButton.update")}
</Button>
)
}

export const StartButton: FC<React.PropsWithChildren<WorkspaceAction>> = ({ handleAction }) => {
const styles = useStyles()
const { t } = useTranslation("workspacePage")

return (
<WorkspaceActionButton
className={styles.actionButton}
icon={<PlayCircleOutlineIcon />}
onClick={handleAction}
label={Language.start}
label={t("actionButton.start")}
/>
)
}

export const StopButton: FC<React.PropsWithChildren<WorkspaceAction>> = ({ handleAction }) => {
const styles = useStyles()
const { t } = useTranslation("workspacePage")

return (
<WorkspaceActionButton
className={styles.actionButton}
icon={<CropSquareIcon />}
onClick={handleAction}
label={Language.stop}
label={t("actionButton.stop")}
/>
)
}

export const DeleteButton: FC<React.PropsWithChildren<WorkspaceAction>> = ({ handleAction }) => {
const styles = useStyles()
const { t } = useTranslation("workspacePage")

return (
<WorkspaceActionButton
className={styles.actionButton}
icon={<DeleteOutlineIcon />}
onClick={handleAction}
label={Language.delete}
label={t("actionButton.delete")}
/>
)
}
Expand All @@ -92,15 +84,16 @@ export const CancelButton: FC<React.PropsWithChildren<WorkspaceAction>> = ({ han
}

interface DisabledProps {
workspaceState: WorkspaceStateEnum
workspaceStatus: WorkspaceStatus
}

export const DisabledButton: FC<React.PropsWithChildren<DisabledProps>> = ({ workspaceState }) => {
export const DisabledButton: FC<React.PropsWithChildren<DisabledProps>> = ({ workspaceStatus }) => {
const styles = useStyles()
const { t } = useTranslation("workspacePage")

return (
<Button disabled className={styles.actionButton}>
{workspaceState}
{t(`disabledButton.${workspaceStatus}`)}
</Button>
)
}
Expand Down
34 changes: 15 additions & 19 deletions site/src/components/WorkspaceActions/WorkspaceActions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
import { DropdownButton } from "components/DropdownButton/DropdownButton"
import { FC, ReactNode, useMemo } from "react"
import { getWorkspaceStatus, WorkspaceStateEnum, WorkspaceStatus } from "util/workspace"
import { Workspace } from "../../api/typesGenerated"
import { useTranslation } from "react-i18next"
import { Workspace, WorkspaceStatus } from "../../api/typesGenerated"
import {
ActionLoadingButton,
DeleteButton,
DisabledButton,
Language,
StartButton,
StopButton,
UpdateButton,
Expand All @@ -18,7 +17,7 @@ import { ButtonMapping, ButtonTypesEnum, WorkspaceStateActions } from "./constan
* so check whether workspace job status has reached completion (whether successful or not).
*/
const canAcceptJobs = (workspaceStatus: WorkspaceStatus) =>
["started", "stopped", "deleted", "error", "canceled"].includes(workspaceStatus)
["running", "stopped", "deleted", "failed", "canceled"].includes(workspaceStatus)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch! This approach makes it easy to fall out of sync with valid WorkspaceStatus values. Here's one way we could fix this:

const CAN_ACCEPT_JOBS_STATUS: Array<
  Extract<
    WorkspaceStatus,
    "running" | "stopped" | "deleted" | "failed" | "canceled"
  >
> = ["running", "stopped", "deleted", "failed", "canceled"];
const canAcceptJobs = (workspaceStatus: WorkspaceStatus) => CAN_ACCEPT_JOBS_STATUS.includes(workspaceStatus)

TypeScript Playground

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm actually fixing this in a future refactor, thanks for pointing it out!


export interface WorkspaceActionsProps {
workspace: Workspace
Expand All @@ -40,42 +39,39 @@ export const WorkspaceActions: FC<WorkspaceActionsProps> = ({
handleCancel,
isUpdating,
}) => {
const workspaceStatus: keyof typeof WorkspaceStateEnum = getWorkspaceStatus(
workspace.latest_build,
)
const workspaceState = WorkspaceStateEnum[workspaceStatus]
const { t } = useTranslation("workspacePage")
const workspaceStatus = workspace.latest_build.status

const canBeUpdated = workspace.outdated && canAcceptJobs(workspaceStatus)

// actions are the primary and secondary CTAs that appear in the workspace actions dropdown
const actions = useMemo(() => {
if (!canBeUpdated) {
return WorkspaceStateActions[workspaceState]
return WorkspaceStateActions[workspaceStatus]
}

// if an update is available, we make the update button the primary CTA
// and move the former primary CTA to the secondary actions list
const updatedActions = { ...WorkspaceStateActions[workspaceState] }
const updatedActions = { ...WorkspaceStateActions[workspaceStatus] }
updatedActions.secondary = [updatedActions.primary, ...updatedActions.secondary]
updatedActions.primary = ButtonTypesEnum.update

return updatedActions
}, [canBeUpdated, workspaceState])
}, [canBeUpdated, workspaceStatus])

// A mapping of button type to the corresponding React component
const buttonMapping: ButtonMapping = {
[ButtonTypesEnum.update]: <UpdateButton handleAction={handleUpdate} />,
[ButtonTypesEnum.updating]: <ActionLoadingButton label={Language.updating} />,
[ButtonTypesEnum.updating]: <ActionLoadingButton label={t("actionButton.updating")} />,
[ButtonTypesEnum.start]: <StartButton handleAction={handleStart} />,
[ButtonTypesEnum.starting]: <ActionLoadingButton label={Language.starting} />,
[ButtonTypesEnum.starting]: <ActionLoadingButton label={t("actionButton.starting")} />,
[ButtonTypesEnum.stop]: <StopButton handleAction={handleStop} />,
[ButtonTypesEnum.stopping]: <ActionLoadingButton label={Language.stopping} />,
[ButtonTypesEnum.stopping]: <ActionLoadingButton label={t("actionButton.stopping")} />,
[ButtonTypesEnum.delete]: <DeleteButton handleAction={handleDelete} />,
[ButtonTypesEnum.deleting]: <ActionLoadingButton label={Language.deleting} />,
[ButtonTypesEnum.canceling]: <DisabledButton workspaceState={workspaceState} />,
[ButtonTypesEnum.disabled]: <DisabledButton workspaceState={workspaceState} />,
[ButtonTypesEnum.queued]: <DisabledButton workspaceState={workspaceState} />,
[ButtonTypesEnum.loading]: <DisabledButton workspaceState={workspaceState} />,
[ButtonTypesEnum.deleting]: <ActionLoadingButton label={t("actionButton.deleting")} />,
[ButtonTypesEnum.canceling]: <DisabledButton workspaceStatus={workspaceStatus} />,
[ButtonTypesEnum.disabled]: <DisabledButton workspaceStatus={workspaceStatus} />,
[ButtonTypesEnum.pending]: <DisabledButton workspaceStatus={workspaceStatus} />,
}

return (
Expand Down
39 changes: 17 additions & 22 deletions site/src/components/WorkspaceActions/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { WorkspaceStatus } from "api/typesGenerated"
import { ReactNode } from "react"
import { WorkspaceStateEnum } from "util/workspace"

// the button types we have
export enum ButtonTypesEnum {
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume these are based on the WorkspaceStatus? We could leverage those types here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are similar to WorkspaceStatus but not identical.

Expand All @@ -14,82 +14,77 @@ export enum ButtonTypesEnum {
// disabled buttons
canceling = "canceling",
disabled = "disabled",
queued = "queued",
loading = "loading",
pending = "pending",
}

export type ButtonMapping = {
[key in ButtonTypesEnum]: ReactNode
}

type StateActionsType = {
[key in WorkspaceStateEnum]: {
type StateActionsType = Record<
WorkspaceStatus,
{
primary: ButtonTypesEnum
secondary: ButtonTypesEnum[]
canCancel: boolean
}
}
>

// A mapping of workspace state to button type
// 'Primary' actions are the main ctas
// 'Secondary' actions are ctas housed within the popover
export const WorkspaceStateActions: StateActionsType = {
[WorkspaceStateEnum.starting]: {
starting: {
Copy link
Member

Choose a reason for hiding this comment

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

One of the reasons we made WorkspaceStateEnum was so that we could have a definitive mapping of the state returned from the BE to button UI state. Although this simplification makes my heart sing, I wonder if we're losing a little bit of that clarity and safety here. It is no longer immediately apparent that the keys in this data structure map to the types in the API's WorkspaceStatus. And the keys here aren't really typed anymore, right? This is a case where I really love enums and wish the API were returning an enum. It would make indexing much easier. @presleyp or @jsjoeio are there any easy workarounds with string literals I'm missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's still typesafe because the keys have to be values of WorkspaceStatus. I could be misremembering where I did this check, but I think it complained if I added a key that didn't belong or left one out.

Copy link
Member

Choose a reason for hiding this comment

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

Ahhhhh, I didn't even see that. You're right - still typesafe! Feel free to ignore the clarify part of my comment - I'm just still figuring out where I land on this whole enum literal debate :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Me too honestly! Just following the generated type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point! I left a comment above related to this.

primary: ButtonTypesEnum.starting,
secondary: [],
canCancel: true,
},
[WorkspaceStateEnum.started]: {
running: {
primary: ButtonTypesEnum.stop,
secondary: [ButtonTypesEnum.delete],
canCancel: false,
},
[WorkspaceStateEnum.stopping]: {
stopping: {
primary: ButtonTypesEnum.stopping,
secondary: [],
canCancel: true,
},
[WorkspaceStateEnum.stopped]: {
stopped: {
primary: ButtonTypesEnum.start,
secondary: [ButtonTypesEnum.delete],
canCancel: false,
},
[WorkspaceStateEnum.canceled]: {
canceled: {
primary: ButtonTypesEnum.start,
secondary: [ButtonTypesEnum.stop, ButtonTypesEnum.delete],
canCancel: false,
},
// in the case of an error
[WorkspaceStateEnum.error]: {
failed: {
primary: ButtonTypesEnum.start, // give the user the ability to start a workspace again
secondary: [ButtonTypesEnum.delete], // allows the user to delete
canCancel: false,
},
/**
* disabled states
*/
[WorkspaceStateEnum.canceling]: {
canceling: {
primary: ButtonTypesEnum.canceling,
secondary: [],
canCancel: false,
},
[WorkspaceStateEnum.deleting]: {
deleting: {
primary: ButtonTypesEnum.deleting,
secondary: [],
canCancel: true,
},
[WorkspaceStateEnum.deleted]: {
deleted: {
primary: ButtonTypesEnum.disabled,
secondary: [],
canCancel: false,
},
[WorkspaceStateEnum.queued]: {
primary: ButtonTypesEnum.queued,
secondary: [],
canCancel: false,
},
[WorkspaceStateEnum.loading]: {
primary: ButtonTypesEnum.loading,
pending: {
primary: ButtonTypesEnum.pending,
secondary: [],
canCancel: false,
},
Expand Down
3 changes: 1 addition & 2 deletions site/src/components/WorkspaceDeletedBanner/WorkspaceDeletedBanner.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import Alert from "@material-ui/lab/Alert"
import AlertTitle from "@material-ui/lab/AlertTitle"
import { FC } from "react"
import * as TypesGen from "../../api/typesGenerated"
import { isWorkspaceDeleted } from "../../util/workspace"

const Language = {
bannerTitle: "This workspace has been deleted and cannot be edited.",
Expand All @@ -22,7 +21,7 @@ export const WorkspaceDeletedBanner: FC<React.PropsWithChildren<WorkspaceDeleted
}) => {
const styles = useStyles()

if (!isWorkspaceDeleted(workspace)) {
if (workspace.latest_build.status === "deleted") {
return null
}

Expand Down
14 changes: 6 additions & 8 deletions site/src/components/WorkspaceStatusBadge/WorkspaceStatusBadge.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import { Pill } from "components/Pill/Pill"
import i18next from "i18next"
import React from "react"
import { PaletteIndex } from "theme/palettes"
import { getWorkspaceStatus } from "util/workspace"

const LoadingIcon: React.FC = () => {
return <CircularProgress size={10} style={{ color: "#FFF" }} />
Expand All @@ -20,19 +19,18 @@ export const getStatus = (
text: string
icon: React.ReactNode
} => {
const status = getWorkspaceStatus(build)
const { t } = i18next

switch (status) {
switch (build.status) {
case undefined:
return {
text: t("workspaceStatus.loading", { ns: "common" }),
icon: <LoadingIcon />,
}
case "started":
case "running":
return {
type: "success",
text: t("workspaceStatus.started", { ns: "common" }),
text: t("workspaceStatus.running", { ns: "common" }),
icon: <PlayIcon />,
}
case "starting":
Expand Down Expand Up @@ -77,16 +75,16 @@ export const getStatus = (
text: t("workspaceStatus.canceled", { ns: "common" }),
icon: <ErrorIcon />,
}
case "error":
case "failed":
return {
type: "error",
text: t("workspaceStatus.failed", { ns: "common" }),
icon: <ErrorIcon />,
}
case "queued":
case "pending":
return {
type: "info",
text: t("workspaceStatus.queued", { ns: "common" }),
text: t("workspaceStatus.pending", { ns: "common" }),
icon: <LoadingIcon />,
}
}
Expand Down
Loading