From 36194492c8ee90476513713eced1a06182e9238e Mon Sep 17 00:00:00 2001 From: Bruno Quaresma Date: Mon, 7 Nov 2022 13:08:06 -0300 Subject: [PATCH 1/4] Revert "Revert "fix: Optimistically update the UI when a workspace action is triggered (#4898)" (#4912)" This reverts commit 267b81af8320be9309382e1d8560b52f6e5c5de2. --- .../WorkspacePage/WorkspacePage.test.tsx | 24 +++--- .../xServices/workspace/workspaceXService.ts | 75 +++++++++++++++++-- 2 files changed, 81 insertions(+), 18 deletions(-) diff --git a/site/src/pages/WorkspacePage/WorkspacePage.test.tsx b/site/src/pages/WorkspacePage/WorkspacePage.test.tsx index bacd99e967542..c05090d02622f 100644 --- a/site/src/pages/WorkspacePage/WorkspacePage.test.tsx +++ b/site/src/pages/WorkspacePage/WorkspacePage.test.tsx @@ -92,16 +92,6 @@ afterAll(() => { }) describe("WorkspacePage", () => { - it("requests a stop job when the user presses Stop", async () => { - const stopWorkspaceMock = jest - .spyOn(api, "stopWorkspace") - .mockResolvedValueOnce(MockWorkspaceBuild) - testButton( - t("actionButton.stop", { ns: "workspacePage" }), - stopWorkspaceMock, - ) - }) - it("requests a delete job when the user presses Delete and confirms", async () => { const user = userEvent.setup() const deleteWorkspaceMock = jest @@ -140,11 +130,23 @@ describe("WorkspacePage", () => { const startWorkspaceMock = jest .spyOn(api, "startWorkspace") .mockImplementation(() => Promise.resolve(MockWorkspaceBuild)) - testButton( + await testButton( t("actionButton.start", { ns: "workspacePage" }), startWorkspaceMock, ) }) + + it("requests a stop job when the user presses Stop", async () => { + const stopWorkspaceMock = jest + .spyOn(api, "stopWorkspace") + .mockResolvedValueOnce(MockWorkspaceBuild) + + await testButton( + t("actionButton.stop", { ns: "workspacePage" }), + stopWorkspaceMock, + ) + }) + it("requests cancellation when the user presses Cancel", async () => { server.use( rest.get( diff --git a/site/src/xServices/workspace/workspaceXService.ts b/site/src/xServices/workspace/workspaceXService.ts index 83111dc2a0a9f..c01691ba6ef17 100644 --- a/site/src/xServices/workspace/workspaceXService.ts +++ b/site/src/xServices/workspace/workspaceXService.ts @@ -40,6 +40,27 @@ const moreBuildsAvailable = ( return event.data.latest_build.updated_at !== latestBuildInTimeline.updated_at } +const updateWorkspaceStatus = ( + status: TypesGen.WorkspaceStatus, + workspace?: TypesGen.Workspace, +) => { + if (!workspace) { + throw new Error("Workspace not defined") + } + + return { + ...workspace, + latest_build: { + ...workspace.latest_build, + status, + }, + } +} + +const isUpdated = (newDateStr: string, oldDateStr: string): boolean => { + return new Date(oldDateStr).getTime() - new Date(newDateStr).getTime() > 0 +} + const Language = { getTemplateWarning: "Error updating workspace: latest template could not be fetched.", @@ -252,6 +273,7 @@ export const workspaceMachine = createMachine( on: { REFRESH_WORKSPACE: { actions: ["refreshWorkspace"], + cond: "hasUpdates", }, EVENT_SOURCE_ERROR: { target: "error", @@ -325,7 +347,7 @@ export const workspaceMachine = createMachine( }, }, requestingStart: { - entry: "clearBuildError", + entry: ["clearBuildError", "updateStatusToStarting"], invoke: { src: "startWorkspace", id: "startWorkspace", @@ -344,7 +366,7 @@ export const workspaceMachine = createMachine( }, }, requestingStop: { - entry: "clearBuildError", + entry: ["clearBuildError", "updateStatusToStopping"], invoke: { src: "stopWorkspace", id: "stopWorkspace", @@ -363,7 +385,7 @@ export const workspaceMachine = createMachine( }, }, requestingDelete: { - entry: "clearBuildError", + entry: ["clearBuildError", "updateStatusToDeleting"], invoke: { src: "deleteWorkspace", id: "deleteWorkspace", @@ -382,7 +404,11 @@ export const workspaceMachine = createMachine( }, }, requestingCancel: { - entry: ["clearCancellationMessage", "clearCancellationError"], + entry: [ + "clearCancellationMessage", + "clearCancellationError", + "updateStatusToCanceling", + ], invoke: { src: "cancelWorkspace", id: "cancelWorkspace", @@ -430,9 +456,7 @@ export const workspaceMachine = createMachine( on: { REFRESH_TIMELINE: { target: "#workspaceState.ready.timeline.gettingBuilds", - cond: { - type: "moreBuildsAvailable", - }, + cond: "moreBuildsAvailable", }, }, }, @@ -599,9 +623,46 @@ export const workspaceMachine = createMachine( }), { to: "scheduleBannerMachine" }, ), + // Optimistically updates. So when the user clicks on stop, we can show + // the "stopping" state right away without having to wait 0.5s ~ 2s to + // display the visual feedback to the user. + updateStatusToStarting: assign({ + workspace: ({ workspace }) => + updateWorkspaceStatus("starting", workspace), + }), + updateStatusToStopping: assign({ + workspace: ({ workspace }) => + updateWorkspaceStatus("stopping", workspace), + }), + updateStatusToDeleting: assign({ + workspace: ({ workspace }) => + updateWorkspaceStatus("deleting", workspace), + }), + updateStatusToCanceling: assign({ + workspace: ({ workspace }) => + updateWorkspaceStatus("canceling", workspace), + }), }, guards: { moreBuildsAvailable, + // We only want to update the workspace when there are changes to it to + // avoid re-renderings and allow optimistically updates to improve the UI. + // When updating the workspace every second, the optimistic updates that + // were applied before get lost since it will be rewrite. + hasUpdates: ({ workspace }, event: { data: TypesGen.Workspace }) => { + if (!workspace) { + throw new Error("Workspace not defined") + } + const isWorkspaceUpdated = isUpdated( + event.data.updated_at, + workspace.updated_at, + ) + const isBuildUpdated = isUpdated( + event.data.latest_build.updated_at, + workspace.latest_build.updated_at, + ) + return isWorkspaceUpdated || isBuildUpdated + }, }, services: { getWorkspace: async (_, event) => { From 9c7c2103bc262c365727a162f600f5921e502158 Mon Sep 17 00:00:00 2001 From: Bruno Quaresma Date: Mon, 7 Nov 2022 16:09:56 +0000 Subject: [PATCH 2/4] No need to check updates --- .../xServices/workspace/workspaceXService.ts | 23 ------------------- 1 file changed, 23 deletions(-) diff --git a/site/src/xServices/workspace/workspaceXService.ts b/site/src/xServices/workspace/workspaceXService.ts index c01691ba6ef17..68f3553d24acf 100644 --- a/site/src/xServices/workspace/workspaceXService.ts +++ b/site/src/xServices/workspace/workspaceXService.ts @@ -57,10 +57,6 @@ const updateWorkspaceStatus = ( } } -const isUpdated = (newDateStr: string, oldDateStr: string): boolean => { - return new Date(oldDateStr).getTime() - new Date(newDateStr).getTime() > 0 -} - const Language = { getTemplateWarning: "Error updating workspace: latest template could not be fetched.", @@ -273,7 +269,6 @@ export const workspaceMachine = createMachine( on: { REFRESH_WORKSPACE: { actions: ["refreshWorkspace"], - cond: "hasUpdates", }, EVENT_SOURCE_ERROR: { target: "error", @@ -645,24 +640,6 @@ export const workspaceMachine = createMachine( }, guards: { moreBuildsAvailable, - // We only want to update the workspace when there are changes to it to - // avoid re-renderings and allow optimistically updates to improve the UI. - // When updating the workspace every second, the optimistic updates that - // were applied before get lost since it will be rewrite. - hasUpdates: ({ workspace }, event: { data: TypesGen.Workspace }) => { - if (!workspace) { - throw new Error("Workspace not defined") - } - const isWorkspaceUpdated = isUpdated( - event.data.updated_at, - workspace.updated_at, - ) - const isBuildUpdated = isUpdated( - event.data.latest_build.updated_at, - workspace.latest_build.updated_at, - ) - return isWorkspaceUpdated || isBuildUpdated - }, }, services: { getWorkspace: async (_, event) => { From e94335ffd8945f8ef1742c11899816bc133a1bbd Mon Sep 17 00:00:00 2001 From: Bruno Quaresma Date: Mon, 7 Nov 2022 16:19:14 +0000 Subject: [PATCH 3/4] Update status to pending --- .../WorkspaceActions/WorkspaceActions.tsx | 2 +- .../xServices/workspace/workspaceXService.ts | 58 +++++++------------ 2 files changed, 21 insertions(+), 39 deletions(-) diff --git a/site/src/components/WorkspaceActions/WorkspaceActions.tsx b/site/src/components/WorkspaceActions/WorkspaceActions.tsx index 66b6a18d9278a..503ae6e7947d6 100644 --- a/site/src/components/WorkspaceActions/WorkspaceActions.tsx +++ b/site/src/components/WorkspaceActions/WorkspaceActions.tsx @@ -64,7 +64,7 @@ export const WorkspaceActions: FC = ({ ), [ButtonTypesEnum.pending]: ( - + ), } diff --git a/site/src/xServices/workspace/workspaceXService.ts b/site/src/xServices/workspace/workspaceXService.ts index 68f3553d24acf..13b8afa1b8b77 100644 --- a/site/src/xServices/workspace/workspaceXService.ts +++ b/site/src/xServices/workspace/workspaceXService.ts @@ -40,23 +40,6 @@ const moreBuildsAvailable = ( return event.data.latest_build.updated_at !== latestBuildInTimeline.updated_at } -const updateWorkspaceStatus = ( - status: TypesGen.WorkspaceStatus, - workspace?: TypesGen.Workspace, -) => { - if (!workspace) { - throw new Error("Workspace not defined") - } - - return { - ...workspace, - latest_build: { - ...workspace.latest_build, - status, - }, - } -} - const Language = { getTemplateWarning: "Error updating workspace: latest template could not be fetched.", @@ -342,7 +325,7 @@ export const workspaceMachine = createMachine( }, }, requestingStart: { - entry: ["clearBuildError", "updateStatusToStarting"], + entry: ["clearBuildError", "updateStatusToPending"], invoke: { src: "startWorkspace", id: "startWorkspace", @@ -361,7 +344,7 @@ export const workspaceMachine = createMachine( }, }, requestingStop: { - entry: ["clearBuildError", "updateStatusToStopping"], + entry: ["clearBuildError", "updateStatusToPending"], invoke: { src: "stopWorkspace", id: "stopWorkspace", @@ -380,7 +363,7 @@ export const workspaceMachine = createMachine( }, }, requestingDelete: { - entry: ["clearBuildError", "updateStatusToDeleting"], + entry: ["clearBuildError", "updateStatusToPending"], invoke: { src: "deleteWorkspace", id: "deleteWorkspace", @@ -402,7 +385,7 @@ export const workspaceMachine = createMachine( entry: [ "clearCancellationMessage", "clearCancellationError", - "updateStatusToCanceling", + "updateStatusToPending", ], invoke: { src: "cancelWorkspace", @@ -618,24 +601,23 @@ export const workspaceMachine = createMachine( }), { to: "scheduleBannerMachine" }, ), - // Optimistically updates. So when the user clicks on stop, we can show - // the "stopping" state right away without having to wait 0.5s ~ 2s to + // Optimistically update. So when the user clicks on stop, we can show + // the "pending" state right away without having to wait 0.5s ~ 2s to // display the visual feedback to the user. - updateStatusToStarting: assign({ - workspace: ({ workspace }) => - updateWorkspaceStatus("starting", workspace), - }), - updateStatusToStopping: assign({ - workspace: ({ workspace }) => - updateWorkspaceStatus("stopping", workspace), - }), - updateStatusToDeleting: assign({ - workspace: ({ workspace }) => - updateWorkspaceStatus("deleting", workspace), - }), - updateStatusToCanceling: assign({ - workspace: ({ workspace }) => - updateWorkspaceStatus("canceling", workspace), + updateStatusToPending: assign({ + workspace: ({ workspace }) => { + if (!workspace) { + throw new Error("Workspace not defined") + } + + return { + ...workspace, + latest_build: { + ...workspace.latest_build, + status: "pending" as TypesGen.WorkspaceStatus, + }, + } + }, }), }, guards: { From 603d74a5fcf35a295941075d65c25eeebbfec88c Mon Sep 17 00:00:00 2001 From: Bruno Quaresma Date: Mon, 7 Nov 2022 16:27:25 +0000 Subject: [PATCH 4/4] Fix minor styles --- site/src/components/DropdownButton/ActionCtas.tsx | 3 +-- site/src/components/DropdownButton/DropdownButton.tsx | 2 +- site/src/components/LoadingButton/LoadingButton.tsx | 4 ++-- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/site/src/components/DropdownButton/ActionCtas.tsx b/site/src/components/DropdownButton/ActionCtas.tsx index 7f104da3833dd..0186797a9703d 100644 --- a/site/src/components/DropdownButton/ActionCtas.tsx +++ b/site/src/components/DropdownButton/ActionCtas.tsx @@ -160,7 +160,6 @@ const useStyles = makeStyles((theme) => ({ // this is all custom to work with our button wrapper loadingButton: { border: "none", - borderLeft: "1px solid #333740", // MUI disabled button - borderRadius: "3px 0px 0px 3px", + borderRadius: `${theme.shape.borderRadius} 0px 0px ${theme.shape.borderRadius}`, }, })) diff --git a/site/src/components/DropdownButton/DropdownButton.tsx b/site/src/components/DropdownButton/DropdownButton.tsx index f7ffcdbb187b7..c4067f9293ecc 100644 --- a/site/src/components/DropdownButton/DropdownButton.tsx +++ b/site/src/components/DropdownButton/DropdownButton.tsx @@ -95,7 +95,7 @@ const useStyles = makeStyles((theme) => ({ borderLeft: `1px solid ${theme.palette.divider}`, borderRadius: `0px ${theme.shape.borderRadius}px ${theme.shape.borderRadius}px 0px`, minWidth: "unset", - width: "63px", // matching cancel button so button grouping doesn't grow in size + width: "64px", // matching cancel button so button grouping doesn't grow in size "& .MuiButton-label": { marginRight: "8px", }, diff --git a/site/src/components/LoadingButton/LoadingButton.tsx b/site/src/components/LoadingButton/LoadingButton.tsx index 6efb4e4aa7f7e..60c0d4997bc31 100644 --- a/site/src/components/LoadingButton/LoadingButton.tsx +++ b/site/src/components/LoadingButton/LoadingButton.tsx @@ -31,7 +31,7 @@ export const LoadingButton: FC> = ({ {children} {loading && (
- +
)} {Boolean(loadingLabel) && loadingLabel} @@ -63,7 +63,7 @@ const useStyles = makeStyles((theme) => ({ top: "50%", left: "50%", height: 22, // centering loading icon - width: 18, + width: 16, }, spinner: { color: theme.palette.text.disabled,