chore: refactor frontend to use workspace status directly by presleyp · Pull Request #4361 · coder/coder

presleyp · 2022-10-04T15:59:13Z

The frontend used to derive the workspace status from the build transition and job status, but now the backend supplies the status. This allows us to delete code and get rid of some indirection in one of the most complex areas of the frontend.

workspace status copy remains in "common", because it's used in the workspace status badge on the workspace page and the workspaces page
workspace action button copy is in "workspacePage", under "actionButton" for the verbs and under "disabledButton" for the statuses on disabled buttons (they are not always displayed identically to the workspace status badge)
consistent naming: pending, not queued; failed, not error; running, not started
there is no loading workspace status or action button because the workspace status is loaded with the workspace, and these things aren't rendered if the workspace is still loading (feel free to double check me!)

presleyp · 2022-10-04T16:20:38Z

Just spotted a bug in the deletion banner, will fix!

presleyp · 2022-10-04T16:30:35Z

-      <AlertTitle>{Language.bannerTitle}</AlertTitle>
-    </Alert>
+    <Maybe condition={workspace.latest_build.status === "deleted"}>
+      <Alert


No change to the Alert here, just wrapping in a Maybe and getting rid of the isWorkspaceDeleted call

presleyp · 2022-10-04T16:33:40Z

 dayjs.extend(utc)
 dayjs.extend(minMax)

-// all the possible states returned by the API


The backend gives us the WorkspaceStatus type and the actual workspace status now, so we can delete this. The display language is deleted just because it's moved into i18n.

Kira-Pilot · 2022-10-04T19:55:54Z

+      )
+      expect(screen.getByTestId("secondary-ctas")).toHaveTextContent(
+        t("actionButton.delete", { ns: "workspacePage" }),
+      )


Thanks for doing all these translations!!!

Kira-Pilot · 2022-10-04T20:42:57Z

 // 'Secondary' actions are ctas housed within the popover
 export const WorkspaceStateActions: StateActionsType = {
-  [WorkspaceStateEnum.starting]: {
+  starting: {


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?

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.

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 :)

Me too honestly! Just following the generated type.

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

Kira-Pilot · 2022-10-04T20:58:01Z

+    "starting": "Starting...",
+    "stopping": "Stopping...",
+    "deleting": "Deleting..."
+  },


Same sort of comment here: I like linking the language to the actual typing that's coming back from the API - that way we get a 1:1 mapping of BE state and UI. In the past, I've managed with constants like:

export const ActionButtonLabels = { [WorkspaceStatus.starting]: i18n.t('workspacePage:actionButton.starting'), [WorkspaceStatus.stopping]: i18n.t('workspacePage:actionButton.stopping'), etc... } as const;

But that was with enums. @presleyp or @jsjoeio again, is there a way to do this with literals? I feel like I could come over to the dark side if there is.

I think we could define this but with "starting" instead of [WorkspaceStatus.starting], which is not as clear that it's typesafe, but is typesafe. And then we'd know that every value of WorkspaceStatus mapped to some text. How's that? (@jsjoeio def interested in your input on all of this when you're available!)

That would be nice, if you feel it's a good add. Not a dealbreaker if you don't!

Also yeah, so curious to hear if there's a way to directly index.

I ended up just changing the way I handle text for the disabled buttons, so that it's not a dynamic lookup.

I'm not sure how i18n could let us pull in TypeScript types 🤔 Ahhh I see @Kira-Pilot's comment above. I think that's a great idea! Happy to look into this after this PR is merged too.

Kira-Pilot

Nice!

BrunoQuaresma

Great work! 👍

jsjoeio · 2022-10-05T17:03:32Z

 export const WithCancel = Template.bind({})
 WithCancel.args = {
-  primaryAction: <DisabledButton workspaceState={WorkspaceStateEnum.deleting} />,
+  primaryAction: <DisabledButton workspaceStatus="deleting" />,


Curious why we're no longer using the enum here?

The WorkspaceStatus type is defined by the backend now

jsjoeio · 2022-10-05T17:28:55Z

 */
 const canAcceptJobs = (workspaceStatus: WorkspaceStatus) =>
-  ["started", "stopped", "deleted", "error", "canceled"].includes(workspaceStatus)
+  ["running", "stopped", "deleted", "failed", "canceled"].includes(workspaceStatus)


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

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

jsjoeio · 2022-10-05T17:29:47Z

-import { WorkspaceStateEnum } from "util/workspace"

 // the button types we have
 export enum ButtonTypesEnum {


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

These are similar to WorkspaceStatus but not identical.

jsjoeio · 2022-10-05T17:30:01Z

 // 'Secondary' actions are ctas housed within the popover
 export const WorkspaceStateActions: StateActionsType = {
-  [WorkspaceStateEnum.starting]: {
+  starting: {


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

jsjoeio · 2022-10-05T17:31:43Z

+    "starting": "Starting...",
+    "stopping": "Stopping...",
+    "deleting": "Deleting..."
+  },


I'm not sure how i18n could let us pull in TypeScript types 🤔 Ahhh I see @Kira-Pilot's comment above. I think that's a great idea! Happy to look into this after this PR is merged too.

jsjoeio · 2022-10-05T17:32:11Z

  latest_build: {
    ...MockWorkspaceBuildStop,
    job: MockRunningProvisionerJob,
+    status: "stopping",


ideally we're pulling these values from the types that way they stay in sync

Yeah, since the generated type is a union of strings, I don't know how to do that. Do you have a way? Or would I just have to create an enum to layer on top of the generated type?

Hmm...we might have to dig in. At first glance, I'm not sure. I mean you do have the TypesGen.Workspace there so maybe we don't have to worry. We could look later!

presleyp · 2022-10-05T18:36:24Z

Since CI was stuck, I went ahead and added my additional refactor to this PR. It puts canAcceptJobs info in the big workspace status -> info mapping, which is more typesafe, and handles both possible overrides to primaryAction in the same place so it's more obvious which buttons you'll get. @Kira-Pilot @jsjoeio

jsjoeio

Great work!

presleyp added 7 commits October 4, 2022 14:36

Add/update copy

330f2af

Update mocks

c38352d

Handle disabled button labels separately

7dc2ef2

Use workspace status directly, use i18n

56f927e

Update stories and tests

a363534

Fix optimistic update in xservice to use status, pending

1992e08

Merge branch 'main' into ws-status/presleyp

5072691

presleyp requested a review from a team as a code owner October 4, 2022 15:59

presleyp requested review from BrunoQuaresma and removed request for a team October 4, 2022 15:59

Rename started to running in story

d06c432

presleyp requested a review from Kira-Pilot October 4, 2022 16:02

Fix deletion banner conditional

b9829b5

presleyp commented Oct 4, 2022

View reviewed changes

Kira-Pilot reviewed Oct 4, 2022

View reviewed changes

Kira-Pilot approved these changes Oct 4, 2022

View reviewed changes

BrunoQuaresma approved these changes Oct 4, 2022

View reviewed changes

Send label to disabled button

23c607e

jsjoeio reviewed Oct 5, 2022

View reviewed changes

Refactor workspace actions

5e10ddf

jsjoeio approved these changes Oct 5, 2022

View reviewed changes

presleyp merged commit 9a670b9 into main Oct 5, 2022

presleyp deleted the ws-status/presleyp branch October 5, 2022 20:20

BrunoQuaresma mentioned this pull request Oct 7, 2022

Incorrect workspace status - and message saying workspace building #4095

Closed

Conversation

presleyp commented Oct 4, 2022

Uh oh!

presleyp commented Oct 4, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kira-Pilot left a comment

Choose a reason for hiding this comment

Uh oh!

BrunoQuaresma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

presleyp commented Oct 5, 2022

Uh oh!

jsjoeio left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants