Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ export function DashboardSidebarWorkspaceItem({
handleOpenInFinder,
isActive,
isDeleteDialogOpen,
isDeleting,
isRenaming,
moveWorkspaceToSection,
removeWorkspaceFromSidebar,
Expand Down Expand Up @@ -129,7 +128,6 @@ export function DashboardSidebarWorkspaceItem({
onConfirm={handleDelete}
title={`Delete "${name || branch}"?`}
description="This will permanently delete the workspace."
isPending={isDeleting}
/>
)}
</>
Expand Down Expand Up @@ -191,7 +189,6 @@ export function DashboardSidebarWorkspaceItem({
onConfirm={handleDelete}
title={`Delete "${name || branch}"?`}
description="This will permanently delete the workspace."
isPending={isDeleting}
/>
)}
</>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,11 @@ import { toast } from "@superset/ui/sonner";
import { useMatchRoute, useNavigate } from "@tanstack/react-router";
import { useState } from "react";
import { apiTrpcClient } from "renderer/lib/api-trpc-client";
import { getDeleteFocusTargetWorkspaceId } from "renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/utils/getDeleteFocusTargetWorkspaceId";
import { getFlattenedV2WorkspaceIds } from "renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/utils/getFlattenedV2WorkspaceIds";
import { navigateToV2Workspace } from "renderer/routes/_authenticated/_dashboard/utils/workspace-navigation";
import { useDashboardSidebarState } from "renderer/routes/_authenticated/hooks/useDashboardSidebarState";
import { useCollections } from "renderer/routes/_authenticated/providers/CollectionsProvider";

interface UseDashboardSidebarWorkspaceItemActionsOptions {
workspaceId: string;
Expand All @@ -17,13 +21,13 @@ export function useDashboardSidebarWorkspaceItemActions({
}: UseDashboardSidebarWorkspaceItemActionsOptions) {
const navigate = useNavigate();
const matchRoute = useMatchRoute();
const collections = useCollections();
const { createSection, moveWorkspaceToSection, removeWorkspaceFromSidebar } =
useDashboardSidebarState();

const [isRenaming, setIsRenaming] = useState(false);
const [renameValue, setRenameValue] = useState(workspaceName);
const [isDeleteDialogOpen, setIsDeleteDialogOpen] = useState(false);
const [isDeleting, setIsDeleting] = useState(false);

const isActive = !!matchRoute({
to: "/v2-workspace/$workspaceId",
Expand Down Expand Up @@ -65,23 +69,36 @@ export function useDashboardSidebarWorkspaceItemActions({
}
};

const handleDelete = async () => {
setIsDeleting(true);
try {
const handleDelete = () => {
const focusTargetId = isActive
? getDeleteFocusTargetWorkspaceId(
getFlattenedV2WorkspaceIds(collections),
workspaceId,
)
: null;

setIsDeleteDialogOpen(false);

const deletePromise = (async () => {
await apiTrpcClient.v2Workspace.delete.mutate({ id: workspaceId });
removeWorkspaceFromSidebar(workspaceId);
setIsDeleteDialogOpen(false);
toast.success("Workspace deleted");
if (isActive) {
navigate({ to: "/" });
}
} catch (error) {
toast.error(
})();

toast.promise(deletePromise, {
loading: "Deleting workspace...",
success: "Workspace deleted",
error: (error) =>
`Failed to delete: ${error instanceof Error ? error.message : "Unknown error"}`,
);
} finally {
setIsDeleting(false);
}
});
Comment on lines +82 to +92
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Misleading error toast if navigation rejects after a successful delete

The deletePromise IIFE chains removeWorkspaceFromSidebar and the navigation call after the server mutate. If the mutate succeeds but navigateToV2Workspace (or navigate({ to: "/" })) throws, deletePromise rejects and toast.promise will display:

"Failed to delete: <navigation error>"

…even though the workspace was already deleted on the server and removed from the sidebar. In practice, in-app TanStack Router navigations almost never throw, so the risk is very low — but if it does happen the user could believe the workspace still exists and retry the delete unnecessarily.

One option is to isolate the navigation so its failure doesn't propagate to the promise used by toast.promise:

Suggested change
const deletePromise = (async () => {
await apiTrpcClient.v2Workspace.delete.mutate({ id: workspaceId });
removeWorkspaceFromSidebar(workspaceId);
setIsDeleteDialogOpen(false);
toast.success("Workspace deleted");
if (isActive) {
navigate({ to: "/" });
if (focusTargetId) {
await navigateToV2Workspace(focusTargetId, navigate);
} else {
await navigate({ to: "/" });
}
}
} catch (error) {
toast.error(
})();
toast.promise(deletePromise, {
loading: "Deleting workspace...",
success: "Workspace deleted",
error: (error) =>
`Failed to delete: ${error instanceof Error ? error.message : "Unknown error"}`,
);
} finally {
setIsDeleting(false);
}
});
const deletePromise = (async () => {
await apiTrpcClient.v2Workspace.delete.mutate({ id: workspaceId });
removeWorkspaceFromSidebar(workspaceId);
})();
toast.promise(deletePromise, {
loading: "Deleting workspace...",
success: "Workspace deleted",
error: (error) =>
`Failed to delete: ${error instanceof Error ? error.message : "Unknown error"}`,
});
void deletePromise.then(() => {
if (!isActive) return;
if (focusTargetId) {
void navigateToV2Workspace(focusTargetId, navigate);
} else {
void navigate({ to: "/" });
}
});


void deletePromise.then(() => {
if (!isActive) return;
if (focusTargetId) {
void navigateToV2Workspace(focusTargetId, navigate);
} else {
void navigate({ to: "/" });
}
});
};

const handleCreateSection = () => {
Expand All @@ -106,7 +123,6 @@ export function useDashboardSidebarWorkspaceItemActions({
handleOpenInFinder,
isActive,
isDeleteDialogOpen,
isDeleting,
isRenaming,
moveWorkspaceToSection,
removeWorkspaceFromSidebar,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
export function getDeleteFocusTargetWorkspaceId(
flattenedWorkspaceIds: readonly string[],
deletedWorkspaceId: string,
): string | null {
const index = flattenedWorkspaceIds.indexOf(deletedWorkspaceId);
if (index === -1) return null;
return (
flattenedWorkspaceIds[index - 1] ?? flattenedWorkspaceIds[index + 1] ?? null
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { getDeleteFocusTargetWorkspaceId } from "./getDeleteFocusTargetWorkspaceId";
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
import type { AppCollections } from "renderer/routes/_authenticated/providers/CollectionsProvider/collections";

type TopLevelItem =
| { kind: "workspace"; tabOrder: number; workspaceId: string }
| { kind: "section"; tabOrder: number; sectionId: string };

export function getFlattenedV2WorkspaceIds(
collections: Pick<
AppCollections,
"v2SidebarProjects" | "v2SidebarSections" | "v2WorkspaceLocalState"
>,
): string[] {
const projects = Array.from(
collections.v2SidebarProjects.state.values(),
).sort((left, right) => left.tabOrder - right.tabOrder);
const allSections = Array.from(collections.v2SidebarSections.state.values());
const allWorkspaces = Array.from(
collections.v2WorkspaceLocalState.state.values(),
);

const result: string[] = [];

for (const project of projects) {
const projectWorkspaces = allWorkspaces.filter(
(workspace) => workspace.sidebarState.projectId === project.projectId,
);
const projectSections = allSections.filter(
(section) => section.projectId === project.projectId,
);

const topLevelItems: TopLevelItem[] = [];
for (const workspace of projectWorkspaces) {
if (workspace.sidebarState.sectionId == null) {
topLevelItems.push({
kind: "workspace",
tabOrder: workspace.sidebarState.tabOrder,
workspaceId: workspace.workspaceId,
});
}
}
for (const section of projectSections) {
topLevelItems.push({
kind: "section",
tabOrder: section.tabOrder,
sectionId: section.sectionId,
});
}
topLevelItems.sort((left, right) => {
if (left.tabOrder !== right.tabOrder) {
return left.tabOrder - right.tabOrder;
}
if (left.kind === right.kind) return 0;
return left.kind === "section" ? -1 : 1;
});

for (const item of topLevelItems) {
if (item.kind === "workspace") {
result.push(item.workspaceId);
continue;
}
const sectionWorkspaces = projectWorkspaces
.filter(
(workspace) => workspace.sidebarState.sectionId === item.sectionId,
)
.sort(
(left, right) =>
left.sidebarState.tabOrder - right.sidebarState.tabOrder,
);
for (const workspace of sectionWorkspaces) {
result.push(workspace.workspaceId);
}
}
}

return result;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { getFlattenedV2WorkspaceIds } from "./getFlattenedV2WorkspaceIds";
Loading