fix: display explicit 'retry' button(s) when a workspace fails #10720

Parkreiner · 2023-11-15T21:51:58Z

Changes made

Makes it so that if a workspace fails, the "workspace button tray" will display a Retry button
- If the user has the ability to see debug logs, an additional "Retry (Debug)" button will also display
- We already had some logic in place for the retry debug button before, but it was only ever displayed in an error alert. If the user didn't have debug log permissions, no retry button would appear at all, and they wouldn't have a clear action to take. The alert is still there, but now, if the user has no debug permissions, they'll get the lower-level Retry button inside the alert instead
- Having the buttons in multiple spots introduces a little UI redundancy, but since this logic deals with errors, it felt like it made sense, just to make sure the user was super clear on what to do
Removes enums in favor of union types for defining workspace errors and workspace button types. The new code is more concise and has fewer imports, with no loss in type-safety
Adds in tests for Retry functionality (six cases total: three workspace transitions with two debug levels)
- Also updates the previous test utilities to parameterize the workspace being used for mocking, rather than assuming it will always be the same default mock workspace

Parkreiner · 2023-11-17T02:15:40Z

site/src/pages/WorkspacePage/Workspace.tsx

  const { saveLocal, getLocal } = useLocalStorage();

-  const buildError = Boolean(workspaceErrors[WorkspaceErrors.BUILD_ERROR]) && (


These two error JSX elements were moved into the return value, to bring them closer to where they're actually used

Parkreiner · 2023-11-17T02:18:52Z

site/src/pages/WorkspacePage/Workspace.tsx

+  // 2023-11-15 - MES - This effect will be called every single render because
+  // "now" will always change and invalidate the dependency array. Need to
+  // figure out if this effect really should run every render (possibly meaning
+  // no dependency array at all), or how to get the array stabilized (ideal)


Having the effect re-run every single render isn't ideal, but I don't think it's breaking anything

During the sprint right before Christmas, I'm planning to do an audit for every single useEffect/useLayoutEffect call we have. I know we have a couple of other wonky calls, too. Just figured it'd be good to flag this for now, if someone has the bandwidth to fix it early while working on another issue

Parkreiner · 2023-11-17T02:21:46Z

site/src/pages/WorkspacePage/WorkspaceActions/WorkspaceActions.tsx

-    [ButtonTypesEnum.starting]: (
+  // A mapping of button type to their corresponding React components
+  const buttonMapping: Record<ButtonType, ReactNode> = {
+    update: <UpdateButton handleAction={handleUpdate} />,


Lot of diffs, but only two major things changed:

Enums got factored out, cleaning up how the properties were defined

retry and retryDebug properties got added

I think that enum was my complication; thanks for fixing :)

Parkreiner · 2023-11-17T02:23:18Z

site/src/pages/WorkspacePage/WorkspaceActions/WorkspaceActions.tsx

-          ? buttonMapping[ButtonTypesEnum.updating]
-          : buttonMapping[ButtonTypesEnum.update])}
+      {canBeUpdated && (
+        <>{isUpdating ? buttonMapping.updating : buttonMapping.update}</>


Not a huge change, but I wanted to make it super explicit that the logic was accounting for three cases:

Render nothing if canBeUpdated is false

Render updating component

Render update component

Parkreiner · 2023-11-17T02:24:41Z

site/src/pages/WorkspacePage/WorkspacePage.test.tsx

+ * requests and that responses produce the correct UI.
+ *
+ * We don't need to test the UI exhaustively because Storybook does that; just
+ * enough to prove that the workspaceStatus was calculated correctly.


Just reformatted the comment to properly fit an 80-character width

Parkreiner · 2023-11-17T13:57:29Z

Going to double-check the Storybook tests later this morning – I don't have great internet where I am right now, and the pages are taking ages to load in

Kira-Pilot · 2023-11-20T19:14:54Z

site/src/pages/WorkspacePage/WorkspaceReadyPage.tsx

-          }
-        }}
+        handleBuildRetry={() => handleBuildRetry(false)}
+        handleBuildRetryDebug={() => handleBuildRetry(true)}


Instead of passing 2 props here, can we just have handleBuildRetry and then pass it a boolean in the Workspace component?

That would work – my main concern with setting up the functions the way I did was insulating the downstream components from changes, or having implementation details leak out

Right now, handleBuildRetry and handleBuildRetryDebug happen to use the same implementation under the hood, but that's just kind of by coincidence. If WorkspaceReadyPage ever gets to the point where the two functions need to look drastically different, we can change things in the one file, and none of the other components have to be any the wiser

I am aware that it does kind of increase the number of props (when we already have a lot), but keeping the other components blind to the implementation details seemed like the lesser of two evils to me

If WorkspaceReadyPage ever gets to the point where the two functions need to look drastically different, we can change things in the one file, and none of the other components have to be any the wiser

I think we might be pre-optimizing here, but I'm happy to keep as-is!

Kira-Pilot · 2023-11-20T19:52:07Z

site/src/pages/WorkspacePage/WorkspaceActions/constants.ts

+/**
+ * A button type supported by the workspace actions UI
+ */
+export type ButtonType = (typeof buttonTypes)[number];


I'm not familiar with this syntax...what is going on here?

There's a little bit to unpack, but:

We take the runtime buttonTypes value and turn it into a type definition via typeof

And because buttonTypes is defined via as const, it's treated as a fixed-length tuple. So, TypeScript knows there are 16 elements in the array exactly, and it also knows the string literal that goes in each slot

Then, when we index that type definition via [number], we're saying to grab the type values at each slot. (typeof buttonTypes)[0] would give us "start", while (typeof buttonTypes)[0 | 1 | 2] would give us "start" | "starting" | "stop". So the [number] index basically says "give us everything at every numeric index you have"

At the end, we have a union of all the types, but the types are defined runtime-first via the const array, so we're free to use it as a value (like combining it with Array.includes)

I did forget to actually export the array, though, so I'll fix that

Appreciate the explanation. It would be nice to use this syntax with TypesGen.WorkspaceTransitions but idk how to make that happen on the Go side

Kira-Pilot

This looks great! Nice work.

Parkreiner added 4 commits November 15, 2023 21:36

refactor: remove workspace error enums

c28f1d8

refactor: improve co-location for serverVersion

c249a40

fix: add in retry button for failed workspaces

718972f

fix: make handleBuildRetry auto-detect debug permissions

6005cec

github-actions bot assigned Parkreiner Nov 15, 2023

Parkreiner added 8 commits November 15, 2023 22:12

chore: consolidate retry messaging

0f91d44

docs: clean up formatting for WorkspacePage.test comments

d635ecd

chore: update renderWorkspacePage to accept parameters

404e4c7

fix: restore granularity to retry operations (debug vs normal)

edecd14

chore: make workspace helpers take explicit workspace parameter

b1f6660

refactor: update how parameters for tests are defined

c3edba7

wip: add tests for workspace retries (breaks 2 previous tests

2c55fd8

fix: update old tests to be correctly parameterized

8d7fe73

Parkreiner commented Nov 17, 2023

View reviewed changes

fix: make sure file formatting goes through

91ce668

Parkreiner changed the title ~~fix: display explicit 'retry' button when a workspace fails~~ fix: display explicit 'retry' button(s) when a workspace fails Nov 17, 2023

Parkreiner requested a review from Kira-Pilot November 17, 2023 13:56

Parkreiner marked this pull request as ready for review November 17, 2023 13:56

Kira-Pilot reviewed Nov 20, 2023

View reviewed changes

Kira-Pilot approved these changes Nov 20, 2023

View reviewed changes

Parkreiner added 3 commits November 22, 2023 20:48

merge main

44f86d5

fix: update variable name for clarity

abc3ae6

fix: add export for runtime buttonTypes array

fa64932

Parkreiner merged commit 491e0e3 into main Nov 22, 2023

Parkreiner deleted the mes/bugs/retry-workspace-button branch November 22, 2023 21:03

github-actions bot locked and limited conversation to collaborators Nov 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: display explicit 'retry' button(s) when a workspace fails #10720

fix: display explicit 'retry' button(s) when a workspace fails #10720

Parkreiner commented Nov 15, 2023 •

edited

Loading

Parkreiner Nov 17, 2023 •

edited

Loading

Parkreiner Nov 17, 2023 •

edited

Loading

Parkreiner Nov 17, 2023

Kira-Pilot Nov 20, 2023

Parkreiner Nov 17, 2023

Parkreiner Nov 17, 2023

Parkreiner commented Nov 17, 2023

Kira-Pilot Nov 20, 2023

Parkreiner Nov 20, 2023

Kira-Pilot Nov 20, 2023 •

edited

Loading

Kira-Pilot Nov 20, 2023

Parkreiner Nov 20, 2023

Kira-Pilot Nov 20, 2023

Kira-Pilot left a comment

		const { saveLocal, getLocal } = useLocalStorage();

		const buildError = Boolean(workspaceErrors[WorkspaceErrors.BUILD_ERROR]) && (

fix: display explicit 'retry' button(s) when a workspace fails #10720

fix: display explicit 'retry' button(s) when a workspace fails #10720

Conversation

Parkreiner commented Nov 15, 2023 • edited Loading

Changes made

Parkreiner Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

Parkreiner Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Parkreiner commented Nov 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kira-Pilot Nov 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kira-Pilot left a comment

Choose a reason for hiding this comment

Parkreiner commented Nov 15, 2023 •

edited

Loading

Parkreiner Nov 17, 2023 •

edited

Loading

Parkreiner Nov 17, 2023 •

edited

Loading

Kira-Pilot Nov 20, 2023 •

edited

Loading