Skip to content
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

Jump to bottom

chore: enhance delete repo UX #364

Merged
merged 1 commit into from
Jul 11, 2023
Merged

chore: enhance delete repo UX #364

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
101 changes: 77 additions & 24 deletions ui/src/pages/repos.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { useQuery, useMutation, gql } from "@apollo/client";
import { useQuery, useMutation, gql, useApolloClient } from "@apollo/client";
import React, { useState, useEffect, useCallback } from "react";

import Link from "@mui/material/Link";
Expand Down Expand Up @@ -32,13 +32,35 @@ import {
DialogActions,
DialogContent,
DialogTitle,
useTheme,
} from "@mui/material";
import { useAuth } from "../lib/auth";
import { GoogleSignin } from "./login";
import { timeDifference } from "../lib/utils";
import { useSnackbar } from "notistack";

function RepoLine({ repo, deletable, sharable, runtimeInfo, onDeleteRepo }) {
const GET_REPOS = gql`
query GetRepos {
myRepos {
name
id
public
updatedAt
createdAt
}
}
`;

function RepoLine({
repo,
deletable,
sharable,
runtimeInfo,
onDeleteRepo,
deleting,
}) {
const { me } = useMe();
const theme = useTheme();
const [killRuntime] = useMutation(
gql`
mutation killRuntime($sessionId: String!) {
Expand All @@ -50,14 +72,25 @@ function RepoLine({ repo, deletable, sharable, runtimeInfo, onDeleteRepo }) {
}
);

// haochen: any reason not using Loading state from useMutation?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch. We should use the loading status from useMutation.

const [killing, setKilling] = useState(false);
return (
<TableRow
key={repo.id}
sx={{ "&:last-child td, &:last-child th": { border: 0 } }}
>
<TableCell align="center">
<Link component={ReactLink} to={`/repo/${repo.id}`}>
<Link
component={ReactLink}
to={`/repo/${repo.id}`}
sx={
deleting && {
color: theme.palette.action.disabled,
textDecorationColor: theme.palette.action.disabled,
pointerEvents: "none",
}
}
>
<Box
sx={{
display: "flex",
Expand Down Expand Up @@ -94,20 +127,25 @@ function RepoLine({ repo, deletable, sharable, runtimeInfo, onDeleteRepo }) {
{deletable && (
<Tooltip title="Delete Repo">
<IconButton
disabled={deleting}
size="small"
onClick={() => {
// FIXME ensure the runtime is killed
onDeleteRepo(repo);
}}
>
<DeleteIcon fontSize="inherit" />
{deleting ? (
<CircularProgress size="14px" />
) : (
<DeleteIcon fontSize="inherit" />
)}
</IconButton>
</Tooltip>
)}
{runtimeInfo ? (
<Tooltip title="Kill runtime">
<IconButton
disabled={killing}
disabled={killing || deleting}
size="small"
onClick={async () => {
// FIXME when to set killing=false?
Expand Down Expand Up @@ -202,14 +240,29 @@ function RepoList({ repos }) {
const [clickedRepo, setClickedRepo] = useState<
{ id: string; name: string } | undefined
>();
const [deleteRepo] = useMutation(
const [isConfirmDeleteDialogOpen, setConfirmDeleteDialogOpen] =
useState(false);
const { enqueueSnackbar } = useSnackbar();
const client = useApolloClient();
const [deleteRepo, deleteRepoResult] = useMutation(
gql`
mutation deleteRepo($id: ID!) {
deleteRepo(id: $id)
}
`,
{
refetchQueries: ["GetRepos"],
onCompleted() {
client.writeQuery({
query: GET_REPOS,
data: {
myRepos: repos.filter((repo) => repo.id !== clickedRepo?.id),
},
});
enqueueSnackbar("Successfully deleted repo", { variant: "success" });
},
onError() {
enqueueSnackbar("Failed to delete repo", { variant: "error" });
},
Comment on lines -212 to +265
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we don't need to manually update a cache here. The deleting variable is enough to achieve the black-out effect in your gif.

In my understanding, the cache-update mechanism is most useful when we want to delete the repo instantly without waiting for deleteRepo mutation returns, which isn't the case here as you are still waiting for the completion of deleteRepo mutation. I can see that the waiting for the second GET_REPOS request is avoided, but that's like reducing from 2s to 1s; not that important. The cache-update is for reducing waiting time from 1s to 0s.

I'd rather do refetchQueries to reduce the complexity of the logic. What do you think?

Copy link
Contributor Author

@sumtsui sumtsui Jul 10, 2023 •
edited

Choose a reason for hiding this comment

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

@lihebi Good point, managing apollo cache will add logic overhead. But if we rely on refetchQueries, can't achieve UI in gif. The deleted repo will not disappear right away when deleting become false. It will disappear after refetchQueries done. so there will be some noticeable delay between success toast show and the repo disappears.

I think this behavior can seem clumsy to the user, but not a big issue, up to us to see if it is worth fixing.

In my understanding, the cache-update mechanism is most useful when we want to delete the repo instantly without waiting for deleteRepo mutation returns

actually common practice with apollo cache is to wait till the mutation api succeeded then remove cache, because the mutation might fail.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the insight. Yep, let's use this manual cache-update here.

}
);
// FIXME once ttl is reached, the runtime is killed, but this query is not
Expand All @@ -224,13 +277,14 @@ function RepoList({ repos }) {
`);

const onConfirmDeleteRepo = useCallback(() => {
setConfirmDeleteDialogOpen(false);
deleteRepo({
variables: {
id: clickedRepo?.id,
},
});
setClickedRepo(undefined);
}, [clickedRepo, deleteRepo]);
}).then(() => setClickedRepo(undefined));
}, [clickedRepo?.id, deleteRepo]);

return (
<>
<TableContainer component={Paper}>
Expand All @@ -249,6 +303,9 @@ function RepoList({ repos }) {
<RepoLine
repo={repo}
deletable={true}
deleting={
repo.id === clickedRepo?.id && deleteRepoResult.loading
}
sharable={true}
runtimeInfo={
loading
Expand All @@ -258,34 +315,30 @@ function RepoList({ repos }) {
)
}
key={repo.id}
onDeleteRepo={setClickedRepo}
onDeleteRepo={(repo) => {
setClickedRepo(repo);
setConfirmDeleteDialogOpen(true);
}}
/>
))}
</TableBody>
</Table>
</TableContainer>
<ConfirmDeleteDialog
repoName={clickedRepo?.name}
open={Boolean(clickedRepo)}
handleCancel={() => setClickedRepo(undefined)}
open={isConfirmDeleteDialogOpen}
handleCancel={() => {
setClickedRepo(undefined);
setConfirmDeleteDialogOpen(false);
}}
handleConfirm={onConfirmDeleteRepo}
/>
</>
);
}

function MyRepos() {
const { loading, error, data } = useQuery(gql`
query GetRepos {
myRepos {
name
id
public
updatedAt
createdAt
}
}
`);
const { loading, error, data } = useQuery(GET_REPOS);

if (loading) {
return <CircularProgress />;
Expand Down