Skip to content

feat(site): add stop and start batch actions #10565

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 9 commits into from
Nov 8, 2023
Merged

feat(site): add stop and start batch actions #10565

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7038c8b
Refactor MoreMnu to be more flexible
BrunoQuaresma Nov 7, 2023
07c1d29
Update menus to use the new way
BrunoQuaresma Nov 7, 2023
8994d1b
Add base
BrunoQuaresma Nov 7, 2023
dd32703
Fix wrong deps
BrunoQuaresma Nov 7, 2023
894629d
Add bulk actions
BrunoQuaresma Nov 7, 2023
85696d3
Extract batch logic into a hook
BrunoQuaresma Nov 7, 2023
59ed10f
Fix missed step to close delete all modal
BrunoQuaresma Nov 7, 2023
eebbdd4
Fix tests
BrunoQuaresma Nov 7, 2023
78aa58f
Fix fmt
BrunoQuaresma Nov 7, 2023
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
Extract batch logic into a hook
  • Loading branch information
@BrunoQuaresma
BrunoQuaresma committed Nov 7, 2023
commit 85696d3f60aadfd95e3620d7dbb73c00dd90c75d
141 changes: 141 additions & 0 deletions site/src/pages/WorkspacesPage/BatchActions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
import TextField from "@mui/material/TextField";
import { Box } from "@mui/system";
import { deleteWorkspace, startWorkspace, stopWorkspace } from "api/api";
import { Workspace } from "api/typesGenerated";
import { ConfirmDialog } from "components/Dialogs/ConfirmDialog/ConfirmDialog";
import { displayError } from "components/GlobalSnackbar/utils";
import { useState } from "react";
import { useMutation } from "react-query";
import { MONOSPACE_FONT_FAMILY } from "theme/constants";

export const useBatchActions = (options: {
onSuccess: () => Promise<void>;
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be typed as () => (void | Promise<void>), just so components are free to use sync or async logic when it makes sense for them

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This component has a very specific use case and should only be used in one place so I would not try to make its interface generic to be re-used

}) => {
const { onSuccess } = options;

const startAllMutation = useMutation({
mutationFn: async (workspaces: Workspace[]) => {
return Promise.all(
workspaces.map((w) =>
startWorkspace(w.id, w.latest_build.template_version_id),
),
);
},
onSuccess,
onError: () => {
displayError("Failed to start workspaces");
},
});

const stopAllMutation = useMutation({
mutationFn: async (workspaces: Workspace[]) => {
return Promise.all(workspaces.map((w) => stopWorkspace(w.id)));
},
onSuccess,
onError: () => {
displayError("Failed to stop workspaces");
},
});

const deleteAllMutation = useMutation({
mutationFn: async (workspaces: Workspace[]) => {
return Promise.all(workspaces.map((w) => deleteWorkspace(w.id)));
},
onSuccess,
onError: () => {
displayError("Failed to delete workspaces");
},
});

return {
startAll: startAllMutation.mutateAsync,
stopAll: stopAllMutation.mutateAsync,
deleteAll: deleteAllMutation.mutateAsync,
isLoading:
startAllMutation.isLoading ||
stopAllMutation.isLoading ||
deleteAllMutation.isLoading,
};
};

type BatchDeleteConfirmationProps = {
checkedWorkspaces: Workspace[];
open: boolean;
isLoading: boolean;
onClose: () => void;
onConfirm: () => void;
};

export const BatchDeleteConfirmation = (
props: BatchDeleteConfirmationProps,
) => {
const { checkedWorkspaces, open, onClose, onConfirm, isLoading } = props;
const [confirmation, setConfirmation] = useState({ value: "", error: false });
Copy link
Member

Choose a reason for hiding this comment

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

What values is value expected to have right now? So far, it looks like "" or DELETE?
I'm wondering if value could have its own type, if we're only going to be supporting a discrete set of operations

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is a good idea but it does not work well for "dynamic" values since it can be whatever the user types and not only the "final" result. Makes sense? The values can be "D", "DE", "DEL", "DELE" or anything like "OTHER".


const confirmDeletion = async () => {
setConfirmation((c) => ({ ...c, error: false }));

if (confirmation.value !== "DELETE") {
setConfirmation((c) => ({ ...c, error: true }));
return;
}

onConfirm();
};

return (
<ConfirmDialog
type="delete"
open={open}
confirmLoading={isLoading}
onConfirm={confirmDeletion}
onClose={() => {
onClose();
setConfirmation({ value: "", error: false });
}}
title={`Delete ${checkedWorkspaces?.length} ${
checkedWorkspaces.length === 1 ? "workspace" : "workspaces"
}`}
description={
<form
onSubmit={async (e) => {
e.preventDefault();
await confirmDeletion();
}}
>
<Box>
Deleting these workspaces is irreversible! Are you sure you want to
proceed? Type{" "}
<Box
component="code"
sx={{
fontFamily: MONOSPACE_FONT_FAMILY,
color: (theme) => theme.palette.text.primary,
fontWeight: 600,
}}
>
`DELETE`
</Box>{" "}
to confirm.
</Box>
<TextField
value={confirmation.value}
required
autoFocus
fullWidth
inputProps={{
"aria-label": "Type DELETE to confirm",
}}
placeholder="Type DELETE to confirm"
sx={{ mt: 2 }}
onChange={(e) => {
setConfirmation((c) => ({ ...c, value: e.currentTarget.value }));
}}
error={confirmation.error}
helperText={confirmation.error && "Please type DELETE to confirm"}
/>
</form>
}
/>
);
};
174 changes: 13 additions & 161 deletions site/src/pages/WorkspacesPage/WorkspacesPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,21 +14,11 @@ import { useTemplateFilterMenu, useStatusFilterMenu } from "./filter/menus";
import { useSearchParams } from "react-router-dom";
import { useFilter } from "components/Filter/filter";
import { useUserFilterMenu } from "components/Filter/UserFilter";
import {
deleteWorkspace,
getWorkspaces,
startWorkspace,
stopWorkspace,
} from "api/api";
import { ConfirmDialog } from "components/Dialogs/ConfirmDialog/ConfirmDialog";
import Box from "@mui/material/Box";
import { MONOSPACE_FONT_FAMILY } from "theme/constants";
import TextField from "@mui/material/TextField";
import { displayError } from "components/GlobalSnackbar/utils";
import { getErrorMessage } from "api/errors";
import { getWorkspaces } from "api/api";
import { useEffectEvent } from "hooks/hookPolyfills";
import { useMutation, useQuery } from "react-query";
import { useQuery } from "react-query";
import { templates } from "api/queries/templates";
import { BatchDeleteConfirmation, useBatchActions } from "./BatchActions";

function useSafeSearchParams() {
// Have to wrap setSearchParams because React Router doesn't make sure that
Expand Down Expand Up @@ -97,31 +87,13 @@ const WorkspacesPage: FC = () => {
}, [experimentEnabled, data, filterProps.filter.query]);
const updateWorkspace = useWorkspaceUpdate(queryKey);
const [checkedWorkspaces, setCheckedWorkspaces] = useState<Workspace[]>([]);
Copy link
Member

@Parkreiner Parkreiner Nov 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Something I've been wondering about for a little bit: does it make sense to copy the checked workspaces into a separate piece of state, or could this be replaced with a Set object containing the IDs of workspaces that have been checked? That way, the original array and the Set could be cross-referenced

I know the current implementation simplifies the setup for calling the batch action functions, but I guess I've just been worried that with state values that lean more redundant, there might be bugs where they get out of sync down the line

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 think so, I just don't see this being used very often. Would you have any extra resources sharing the benefits of this approach?

Copy link
Member

Choose a reason for hiding this comment

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

So, there's a really good example I can point to, but sadly, it was from Josh Comeau's Joy of React course, and that's pricey

A lot of the new official React documentation makes a lot of references to avoiding duplicate state, though. This section touches on it a little bit

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 see, so instead of objects we could use ids, is that right?

Copy link
Member

Choose a reason for hiding this comment

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

Right, maybe a Set isn't the right data structure, but the idea is that it would have nothing but IDs in it

const [isDeletingAll, setIsDeletingAll] = useState(false);
const [isConfirmingDeleteAll, setIsConfirmingDeleteAll] = useState(false);
Copy link
Member

Choose a reason for hiding this comment

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

Really like the name change

const [urlSearchParams] = searchParamsResult;
const { entitlements } = useDashboard();
const canCheckWorkspaces =
entitlements.features["workspace_batch_actions"].enabled;
const permissions = usePermissions();

// Batch mutations
const startAllMutation = useMutation({
mutationFn: async (workspaces: Workspace[]) => {
return Promise.all(
workspaces.map((w) =>
startWorkspace(w.id, w.latest_build.template_version_id),
),
);
},
onSuccess: async () => {
await refetch();
setCheckedWorkspaces([]);
},
});
const stopAllMutation = useMutation({
mutationFn: async (workspaces: Workspace[]) => {
return Promise.all(workspaces.map((w) => stopWorkspace(w.id)));
},
const batchActions = useBatchActions({
onSuccess: async () => {
await refetch();
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be awaited? I'm wondering if we could just refetch without await, so the state can be cleared out faster

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 is needed because we only want to remove the loading state from this action when the new info is on the screen.

setCheckedWorkspaces([]);
Expand Down Expand Up @@ -158,35 +130,21 @@ const WorkspacesPage: FC = () => {
onUpdateWorkspace={(workspace) => {
updateWorkspace.mutate(workspace);
}}
isRunningBatchAction={
isDeletingAll ||
startAllMutation.isLoading ||
stopAllMutation.isLoading
}
isRunningBatchAction={batchActions.isLoading}
onDeleteAll={() => {
setIsDeletingAll(true);
}}
onStartAll={async () => {
await startAllMutation.mutateAsync(checkedWorkspaces);
await refetch();
setCheckedWorkspaces([]);
}}
onStopAll={async () => {
await stopAllMutation.mutateAsync(checkedWorkspaces);
await refetch();
setCheckedWorkspaces([]);
setIsConfirmingDeleteAll(true);
}}
onStartAll={() => batchActions.startAll(checkedWorkspaces)}
onStopAll={() => batchActions.stopAll(checkedWorkspaces)}
/>

<BatchDeleteConfirmation
isLoading={batchActions.isLoading}
checkedWorkspaces={checkedWorkspaces}
open={isDeletingAll}
open={isConfirmingDeleteAll}
onConfirm={() => batchActions.deleteAll(checkedWorkspaces)}
onClose={() => {
setIsDeletingAll(false);
}}
onDelete={async () => {
await refetch();
setCheckedWorkspaces([]);
setIsConfirmingDeleteAll(false);
}}
/>
</>
Expand Down Expand Up @@ -243,109 +201,3 @@ const useWorkspacesFilter = ({
},
};
};

const BatchDeleteConfirmation = ({
checkedWorkspaces,
open,
onClose,
onDelete,
}: {
checkedWorkspaces: Workspace[];
open: boolean;
onClose: () => void;
onDelete: () => void;
}) => {
const [confirmValue, setConfirmValue] = useState("");
const [confirmError, setConfirmError] = useState(false);
const [isDeleting, setIsDeleting] = useState(false);

const close = () => {
if (isDeleting) {
return;
}

onClose();
setConfirmValue("");
setConfirmError(false);
setIsDeleting(false);
};

const confirmDeletion = async () => {
setConfirmError(false);

if (confirmValue !== "DELETE") {
setConfirmError(true);
return;
}

try {
setIsDeleting(true);
await Promise.all(checkedWorkspaces.map((w) => deleteWorkspace(w.id)));
} catch (e) {
displayError(
"Error on deleting workspaces",
getErrorMessage(e, "An error occurred while deleting the workspaces"),
);
} finally {
close();
onDelete();
}
};

return (
<ConfirmDialog
type="delete"
open={open}
confirmLoading={isDeleting}
onConfirm={confirmDeletion}
onClose={() => {
onClose();
setConfirmValue("");
setConfirmError(false);
}}
title={`Delete ${checkedWorkspaces?.length} ${
checkedWorkspaces.length === 1 ? "workspace" : "workspaces"
}`}
description={
<form
onSubmit={async (e) => {
e.preventDefault();
await confirmDeletion();
}}
>
<Box>
Deleting these workspaces is irreversible! Are you sure you want to
proceed? Type{" "}
<Box
component="code"
sx={{
fontFamily: MONOSPACE_FONT_FAMILY,
color: (theme) => theme.palette.text.primary,
fontWeight: 600,
}}
>
`DELETE`
</Box>{" "}
to confirm.
</Box>
<TextField
value={confirmValue}
required
autoFocus
fullWidth
inputProps={{
"aria-label": "Type DELETE to confirm",
}}
placeholder="Type DELETE to confirm"
sx={{ mt: 2 }}
onChange={(e) => {
setConfirmValue(e.currentTarget.value);
}}
error={confirmError}
helperText={confirmError && "Please type DELETE to confirm"}
/>
</form>
}
/>
);
};