From 5eaa5029d029ae00fd776badcea45a6f5895f827 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Fri, 21 Nov 2025 13:51:42 -0800 Subject: [PATCH 1/9] confirm delete worktree --- .../routers/workspaces/workspaces.test.ts | 190 ++++++++++++++++++ .../lib/trpc/routers/workspaces/workspaces.ts | 77 ++++++- .../WorkspaceTabs/DeleteWorkspaceDialog.tsx | 97 +++++++++ .../TopBar/WorkspaceTabs/WorkspaceItem.tsx | 102 +++++----- 4 files changed, 419 insertions(+), 47 deletions(-) create mode 100644 apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts create mode 100644 apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsx diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts b/apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts new file mode 100644 index 00000000000..7baaa28e759 --- /dev/null +++ b/apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts @@ -0,0 +1,190 @@ +import { describe, expect, it, mock } from "bun:test"; +import { createWorkspacesRouter } from "./workspaces"; +import * as gitUtils from "./utils/git"; + +// Mock the git utilities +mock.module("./utils/git", () => ({ + createWorktree: mock(() => Promise.resolve()), + removeWorktree: mock(() => Promise.resolve()), + generateBranchName: mock(() => "test-branch-123"), +})); + +// Mock the database +const mockDb = { + data: { + workspaces: [ + { + id: "workspace-1", + projectId: "project-1", + worktreeId: "worktree-1", + name: "Test Workspace", + tabOrder: 0, + createdAt: Date.now(), + updatedAt: Date.now(), + lastOpenedAt: Date.now(), + }, + ], + worktrees: [ + { + id: "worktree-1", + projectId: "project-1", + path: "/path/to/worktree", + branch: "test-branch", + createdAt: Date.now(), + }, + ], + projects: [ + { + id: "project-1", + name: "Test Project", + mainRepoPath: "/path/to/repo", + color: "#ff0000", + tabOrder: 0, + createdAt: Date.now(), + lastOpenedAt: Date.now(), + }, + ], + settings: { + lastActiveWorkspaceId: "workspace-1", + }, + }, + update: mock(async (fn: (data: typeof mockDb.data) => void) => { + fn(mockDb.data); + }), +}; + +describe("workspaces router - delete", () => { + it("should successfully delete workspace and remove worktree", async () => { + const router = createWorkspacesRouter(); + + // Mock removeWorktree to succeed + const removeWorktreeMock = mock(() => Promise.resolve()); + mock.module("./utils/git", () => ({ + ...gitUtils, + removeWorktree: removeWorktreeMock, + })); + + const caller = router.createCaller({ db: mockDb as any }); + + const result = await caller.delete({ id: "workspace-1" }); + + expect(result.success).toBe(true); + expect(removeWorktreeMock).toHaveBeenCalledWith( + "/path/to/repo", + "/path/to/worktree", + ); + expect(mockDb.data.workspaces).toHaveLength(0); + expect(mockDb.data.worktrees).toHaveLength(0); + }); + + it("should fail deletion if worktree removal fails", async () => { + // Reset mock data + mockDb.data.workspaces = [ + { + id: "workspace-1", + projectId: "project-1", + worktreeId: "worktree-1", + name: "Test Workspace", + tabOrder: 0, + createdAt: Date.now(), + updatedAt: Date.now(), + lastOpenedAt: Date.now(), + }, + ]; + mockDb.data.worktrees = [ + { + id: "worktree-1", + projectId: "project-1", + path: "/path/to/worktree", + branch: "test-branch", + createdAt: Date.now(), + }, + ]; + + const router = createWorkspacesRouter(); + + // Mock removeWorktree to fail + const removeWorktreeMock = mock(() => + Promise.reject(new Error("Failed to remove worktree")), + ); + mock.module("./utils/git", () => ({ + ...gitUtils, + removeWorktree: removeWorktreeMock, + })); + + const caller = router.createCaller({ db: mockDb as any }); + + const result = await caller.delete({ id: "workspace-1" }); + + expect(result.success).toBe(false); + expect(result.error).toContain("Failed to remove worktree"); + // Workspace should NOT be removed from DB if worktree removal fails + expect(mockDb.data.workspaces).toHaveLength(1); + expect(mockDb.data.worktrees).toHaveLength(1); + }); +}); + +describe("workspaces router - canDelete", () => { + it("should return true when worktree can be deleted", async () => { + const router = createWorkspacesRouter(); + + // Mock git to return worktree list + const mockGit = { + raw: mock(() => + Promise.resolve("/path/to/worktree\n/path/to/other-worktree"), + ), + }; + const mockSimpleGit = mock(() => mockGit); + mock.module("simple-git", () => ({ + default: mockSimpleGit, + })); + + const caller = router.createCaller({ db: mockDb as any }); + + const result = await caller.canDelete({ id: "workspace-1" }); + + expect(result.canDelete).toBe(true); + expect(result.reason).toBeNull(); + expect(result.warning).toBeNull(); + }); + + it("should return warning when worktree doesn't exist in git", async () => { + const router = createWorkspacesRouter(); + + // Mock git to return worktree list without our worktree + const mockGit = { + raw: mock(() => Promise.resolve("/path/to/other-worktree")), + }; + const mockSimpleGit = mock(() => mockGit); + mock.module("simple-git", () => ({ + default: mockSimpleGit, + })); + + const caller = router.createCaller({ db: mockDb as any }); + + const result = await caller.canDelete({ id: "workspace-1" }); + + expect(result.canDelete).toBe(true); + expect(result.warning).toContain("not found in git"); + }); + + it("should return false when git check fails", async () => { + const router = createWorkspacesRouter(); + + // Mock git to throw error + const mockGit = { + raw: mock(() => Promise.reject(new Error("Git error"))), + }; + const mockSimpleGit = mock(() => mockGit); + mock.module("simple-git", () => ({ + default: mockSimpleGit, + })); + + const caller = router.createCaller({ db: mockDb as any }); + + const result = await caller.canDelete({ id: "workspace-1" }); + + expect(result.canDelete).toBe(false); + expect(result.reason).toContain("Failed to check worktree status"); + }); +}); diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts b/apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts index 873084249ce..fc306cf08fc 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts @@ -196,6 +196,72 @@ export const createWorkspacesRouter = () => { return { success: true }; }), + canDelete: publicProcedure + .input(z.object({ id: z.string() })) + .query(async ({ input }) => { + const workspace = db.data.workspaces.find((w) => w.id === input.id); + + if (!workspace) { + return { + canDelete: false, + reason: "Workspace not found", + workspace: null, + }; + } + + const worktree = db.data.worktrees.find( + (wt) => wt.id === workspace.worktreeId, + ); + const project = db.data.projects.find( + (p) => p.id === workspace.projectId, + ); + + // Check if worktree can be removed + if (worktree && project) { + try { + // Dry-run: verify the worktree exists in git + const git = await import("simple-git").then((m) => m.default); + const gitInstance = git(project.mainRepoPath); + const worktrees = await gitInstance.raw(["worktree", "list"]); + + // Check if our worktree path is in the list + const worktreeExists = worktrees.includes(worktree.path); + + if (!worktreeExists) { + // Worktree doesn't exist in git, but we can still delete the workspace + return { + canDelete: true, + reason: null, + workspace, + warning: + "Worktree not found in git (may have been manually removed)", + }; + } + + return { + canDelete: true, + reason: null, + workspace, + warning: null, + }; + } catch (error) { + return { + canDelete: false, + reason: `Failed to check worktree status: ${error instanceof Error ? error.message : String(error)}`, + workspace, + }; + } + } + + // No worktree to remove, can safely delete + return { + canDelete: true, + reason: null, + workspace, + warning: "No associated worktree found", + }; + }), + delete: publicProcedure .input(z.object({ id: z.string() })) .mutation(async ({ input }) => { @@ -212,14 +278,23 @@ export const createWorkspacesRouter = () => { (p) => p.id === workspace.projectId, ); + // Always attempt to remove the worktree first if (worktree && project) { try { await removeWorktree(project.mainRepoPath, worktree.path); } catch (error) { - console.error("Failed to remove worktree:", error); + // If worktree removal fails, return error and don't proceed with DB cleanup + const errorMessage = + error instanceof Error ? error.message : String(error); + console.error("Failed to remove worktree:", errorMessage); + return { + success: false, + error: `Failed to remove worktree: ${errorMessage}`, + }; } } + // Only proceed with DB cleanup if worktree was successfully removed (or doesn't exist) await db.update((data) => { data.workspaces = data.workspaces.filter((w) => w.id !== input.id); diff --git a/apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsx b/apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsx new file mode 100644 index 00000000000..bbe640cebb0 --- /dev/null +++ b/apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsx @@ -0,0 +1,97 @@ +import { + AlertDialog, + AlertDialogAction, + AlertDialogCancel, + AlertDialogContent, + AlertDialogDescription, + AlertDialogFooter, + AlertDialogHeader, + AlertDialogTitle, +} from "@superset/ui/alert-dialog"; +import { useState } from "react"; +import { trpc } from "renderer/lib/trpc"; +import { useDeleteWorkspace } from "renderer/react-query/workspaces"; + +interface DeleteWorkspaceDialogProps { + workspaceId: string; + workspaceName: string; + open: boolean; + onOpenChange: (open: boolean) => void; +} + +export function DeleteWorkspaceDialog({ + workspaceId, + workspaceName, + open, + onOpenChange, +}: DeleteWorkspaceDialogProps) { + const [isDeleting, setIsDeleting] = useState(false); + const deleteWorkspace = useDeleteWorkspace(); + + // Query to check if workspace can be deleted + const { data: canDeleteData, isLoading } = trpc.workspaces.canDelete.useQuery( + { id: workspaceId }, + { enabled: open }, // Only run when dialog is open + ); + + const handleDelete = async () => { + setIsDeleting(true); + try { + await deleteWorkspace.mutateAsync({ id: workspaceId }); + onOpenChange(false); + } catch (error) { + console.error("Failed to delete workspace:", error); + } finally { + setIsDeleting(false); + } + }; + + const canDelete = canDeleteData?.canDelete ?? true; + const reason = canDeleteData?.reason; + const warning = canDeleteData?.warning; + + return ( + + + + Delete Workspace + + {isLoading ? ( + Checking workspace status... + ) : !canDelete ? ( + + Cannot delete workspace: {reason} + + ) : ( + <> + Are you sure you want to delete "{workspaceName}"? + {warning && ( + + Warning: {warning} + + )} + + This will remove the workspace and its associated git + worktree. This action cannot be undone. + + + )} + + + + Cancel + { + e.preventDefault(); + handleDelete(); + }} + disabled={!canDelete || isDeleting || isLoading} + className="bg-destructive text-white hover:bg-destructive/90" + > + {isDeleting ? "Deleting..." : "Delete"} + + + + + ); +} diff --git a/apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx b/apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx index bb50eb77cd8..d4fb91f9e74 100644 --- a/apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx +++ b/apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx @@ -1,12 +1,13 @@ import { Button } from "@superset/ui/button"; import { cn } from "@superset/ui/utils"; +import { useState } from "react"; import { useDrag, useDrop } from "react-dnd"; import { HiMiniXMark } from "react-icons/hi2"; import { - useDeleteWorkspace, useReorderWorkspaces, useSetActiveWorkspace, } from "renderer/react-query/workspaces"; +import { DeleteWorkspaceDialog } from "./DeleteWorkspaceDialog"; const WORKSPACE_TYPE = "WORKSPACE"; @@ -32,8 +33,8 @@ export function WorkspaceItem({ onMouseLeave, }: WorkspaceItemProps) { const setActive = useSetActiveWorkspace(); - const deleteWorkspace = useDeleteWorkspace(); const reorderWorkspaces = useReorderWorkspaces(); + const [showDeleteDialog, setShowDeleteDialog] = useState(false); const [{ isDragging }, drag] = useDrag( () => ({ @@ -62,51 +63,60 @@ export function WorkspaceItem({ }); return ( -
- {/* Main workspace button */} - + {/* Main workspace button */} + - -
+ + + + + ); } From 5a3fbc2567a2a7ee9e6798559efd78e60533e55e Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Fri, 21 Nov 2025 13:55:57 -0800 Subject: [PATCH 2/9] confirm delete worktree --- bun.lock | 7 +- packages/ui/package.json | 4 +- packages/ui/src/alert-dialog.tsx | 1 + packages/ui/src/components/alert-dialog.tsx | 155 ++++++++++++++++++++ 4 files changed, 165 insertions(+), 2 deletions(-) create mode 100644 packages/ui/src/alert-dialog.tsx create mode 100644 packages/ui/src/components/alert-dialog.tsx diff --git a/bun.lock b/bun.lock index c7974cdd73f..61eac705fce 100644 --- a/bun.lock +++ b/bun.lock @@ -360,6 +360,7 @@ "name": "@superset/ui", "version": "0.0.0", "dependencies": { + "@radix-ui/react-alert-dialog": "^1.1.15", "@radix-ui/react-context-menu": "^2.2.16", "@radix-ui/react-dialog": "^1.1.15", "@radix-ui/react-dropdown-menu": "^2.1.16", @@ -368,7 +369,7 @@ "@radix-ui/react-scroll-area": "^1.2.10", "@radix-ui/react-select": "^2.2.6", "@radix-ui/react-separator": "^1.1.8", - "@radix-ui/react-slot": "^1.2.3", + "@radix-ui/react-slot": "^1.2.4", "@radix-ui/react-tooltip": "^1.2.8", "class-variance-authority": "^0.7.1", "clsx": "^2.1.1", @@ -863,6 +864,8 @@ "@radix-ui/primitive": ["@radix-ui/primitive@1.1.3", "", {}, "sha512-JTF99U/6XIjCBo0wqkU5sK10glYe27MRRsfwoiq5zzOEZLHU3A3KCMa5X/azekYRCJ0HlwI0crAXS/5dEHTzDg=="], + "@radix-ui/react-alert-dialog": ["@radix-ui/react-alert-dialog@1.1.15", "", { "dependencies": { "@radix-ui/primitive": "1.1.3", "@radix-ui/react-compose-refs": "1.1.2", "@radix-ui/react-context": "1.1.2", "@radix-ui/react-dialog": "1.1.15", "@radix-ui/react-primitive": "2.1.3", "@radix-ui/react-slot": "1.2.3" }, "peerDependencies": { "@types/react": "*", "@types/react-dom": "*", "react": "^16.8 || ^17.0 || ^18.0 || ^19.0 || ^19.0.0-rc", "react-dom": "^16.8 || ^17.0 || ^18.0 || ^19.0 || ^19.0.0-rc" }, "optionalPeers": ["@types/react", "@types/react-dom"] }, "sha512-oTVLkEw5GpdRe29BqJ0LSDFWI3qu0vR1M0mUkOQWDIUnY/QIkLpgDMWuKxP94c2NAC2LGcgVhG1ImF3jkZ5wXw=="], + "@radix-ui/react-arrow": ["@radix-ui/react-arrow@1.1.7", "", { "dependencies": { "@radix-ui/react-primitive": "2.1.3" }, "peerDependencies": { "@types/react": "*", "@types/react-dom": "*", "react": "^16.8 || ^17.0 || ^18.0 || ^19.0 || ^19.0.0-rc", "react-dom": "^16.8 || ^17.0 || ^18.0 || ^19.0 || ^19.0.0-rc" }, "optionalPeers": ["@types/react", "@types/react-dom"] }, "sha512-F+M1tLhO+mlQaOWspE8Wstg+z6PwxwRd8oQ8IXceWz92kfAmalTRf0EjrouQeo7QssEPfCn05B4Ihs1K9WQ/7w=="], "@radix-ui/react-collection": ["@radix-ui/react-collection@1.1.7", "", { "dependencies": { "@radix-ui/react-compose-refs": "1.1.2", "@radix-ui/react-context": "1.1.2", "@radix-ui/react-primitive": "2.1.3", "@radix-ui/react-slot": "1.2.3" }, "peerDependencies": { "@types/react": "*", "@types/react-dom": "*", "react": "^16.8 || ^17.0 || ^18.0 || ^19.0 || ^19.0.0-rc", "react-dom": "^16.8 || ^17.0 || ^18.0 || ^19.0 || ^19.0.0-rc" }, "optionalPeers": ["@types/react", "@types/react-dom"] }, "sha512-Fh9rGN0MoI4ZFUNyfFVNU4y9LUz93u9/0K+yLgA2bwRojxM8JU1DyvvMBabnZPBgMWREAJvU2jjVzq+LrFUglw=="], @@ -3839,6 +3842,8 @@ "@npmcli/move-file/rimraf": ["rimraf@3.0.2", "", { "dependencies": { "glob": "^7.1.3" }, "bin": { "rimraf": "bin.js" } }, "sha512-JZkJMZkAGFFPP2YqXZXPbMlMBgsxzE8ILs4lMIX/2o0L9UBw9O/Y3o6wFw/i9YLapcUJWwqbi3kdxIPdC62TIA=="], + "@radix-ui/react-alert-dialog/@radix-ui/react-slot": ["@radix-ui/react-slot@1.2.3", "", { "dependencies": { "@radix-ui/react-compose-refs": "1.1.2" }, "peerDependencies": { "@types/react": "*", "react": "^16.8 || ^17.0 || ^18.0 || ^19.0 || ^19.0.0-rc" }, "optionalPeers": ["@types/react"] }, "sha512-aeNmHnBxbi2St0au6VBVC7JXFlhLlOnvIIlePNniyUNAClzmtAUEY8/pBiK3iHjufOlwA+c20/8jngo7xcrg8A=="], + "@radix-ui/react-collection/@radix-ui/react-slot": ["@radix-ui/react-slot@1.2.3", "", { "dependencies": { "@radix-ui/react-compose-refs": "1.1.2" }, "peerDependencies": { "@types/react": "*", "react": "^16.8 || ^17.0 || ^18.0 || ^19.0 || ^19.0.0-rc" }, "optionalPeers": ["@types/react"] }, "sha512-aeNmHnBxbi2St0au6VBVC7JXFlhLlOnvIIlePNniyUNAClzmtAUEY8/pBiK3iHjufOlwA+c20/8jngo7xcrg8A=="], "@radix-ui/react-dialog/@radix-ui/react-slot": ["@radix-ui/react-slot@1.2.3", "", { "dependencies": { "@radix-ui/react-compose-refs": "1.1.2" }, "peerDependencies": { "@types/react": "*", "react": "^16.8 || ^17.0 || ^18.0 || ^19.0 || ^19.0.0-rc" }, "optionalPeers": ["@types/react"] }, "sha512-aeNmHnBxbi2St0au6VBVC7JXFlhLlOnvIIlePNniyUNAClzmtAUEY8/pBiK3iHjufOlwA+c20/8jngo7xcrg8A=="], diff --git a/packages/ui/package.json b/packages/ui/package.json index c02a1404edf..0cae180d790 100644 --- a/packages/ui/package.json +++ b/packages/ui/package.json @@ -4,6 +4,7 @@ "private": true, "type": "module", "exports": { + "./alert-dialog": "./src/components/alert-dialog.tsx", "./button": "./src/components/button.tsx", "./input": "./src/components/input.tsx", "./card": "./src/components/card.tsx", @@ -29,6 +30,7 @@ "typecheck": "tsc --noEmit" }, "dependencies": { + "@radix-ui/react-alert-dialog": "^1.1.15", "@radix-ui/react-context-menu": "^2.2.16", "@radix-ui/react-dialog": "^1.1.15", "@radix-ui/react-dropdown-menu": "^2.1.16", @@ -37,7 +39,7 @@ "@radix-ui/react-scroll-area": "^1.2.10", "@radix-ui/react-select": "^2.2.6", "@radix-ui/react-separator": "^1.1.8", - "@radix-ui/react-slot": "^1.2.3", + "@radix-ui/react-slot": "^1.2.4", "@radix-ui/react-tooltip": "^1.2.8", "class-variance-authority": "^0.7.1", "clsx": "^2.1.1", diff --git a/packages/ui/src/alert-dialog.tsx b/packages/ui/src/alert-dialog.tsx new file mode 100644 index 00000000000..e4509e9d976 --- /dev/null +++ b/packages/ui/src/alert-dialog.tsx @@ -0,0 +1 @@ +export * from "./components/alert-dialog"; diff --git a/packages/ui/src/components/alert-dialog.tsx b/packages/ui/src/components/alert-dialog.tsx new file mode 100644 index 00000000000..9d776e860bf --- /dev/null +++ b/packages/ui/src/components/alert-dialog.tsx @@ -0,0 +1,155 @@ +import * as React from "react" +import * as AlertDialogPrimitive from "@radix-ui/react-alert-dialog" + +import { cn } from "../lib/utils" +import { buttonVariants } from "./button" + +function AlertDialog({ + ...props +}: React.ComponentProps) { + return +} + +function AlertDialogTrigger({ + ...props +}: React.ComponentProps) { + return ( + + ) +} + +function AlertDialogPortal({ + ...props +}: React.ComponentProps) { + return ( + + ) +} + +function AlertDialogOverlay({ + className, + ...props +}: React.ComponentProps) { + return ( + + ) +} + +function AlertDialogContent({ + className, + ...props +}: React.ComponentProps) { + return ( + + + + + ) +} + +function AlertDialogHeader({ + className, + ...props +}: React.ComponentProps<"div">) { + return ( +
+ ) +} + +function AlertDialogFooter({ + className, + ...props +}: React.ComponentProps<"div">) { + return ( +
+ ) +} + +function AlertDialogTitle({ + className, + ...props +}: React.ComponentProps) { + return ( + + ) +} + +function AlertDialogDescription({ + className, + ...props +}: React.ComponentProps) { + return ( + + ) +} + +function AlertDialogAction({ + className, + ...props +}: React.ComponentProps) { + return ( + + ) +} + +function AlertDialogCancel({ + className, + ...props +}: React.ComponentProps) { + return ( + + ) +} + +export { + AlertDialog, + AlertDialogPortal, + AlertDialogOverlay, + AlertDialogTrigger, + AlertDialogContent, + AlertDialogHeader, + AlertDialogFooter, + AlertDialogTitle, + AlertDialogDescription, + AlertDialogAction, + AlertDialogCancel, +} From 6c7cea57396bfff51f94adb6e0282e95cfc67e7f Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Fri, 21 Nov 2025 15:37:53 -0800 Subject: [PATCH 3/9] fix alert dialog --- apps/desktop/src/renderer/globals.css | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/apps/desktop/src/renderer/globals.css b/apps/desktop/src/renderer/globals.css index defccbc0aa5..ec67d42cbb3 100644 --- a/apps/desktop/src/renderer/globals.css +++ b/apps/desktop/src/renderer/globals.css @@ -129,12 +129,26 @@ html, body { + width: 100vw; + height: 100vh; + min-height: 100vh; + margin: 0; + overflow: hidden; /* prevent scroll-lock adjustments from collapsing layout */ user-select: none; scroll-behavior: smooth; -webkit-app-region: no-drag; -webkit-font-smoothing: antialiased; } + /* Ensure the React root fills the viewport and provides a positioning context */ + app { + display: block; + position: relative; + width: 100vw; + height: 100vh; + min-height: 100vh; + } + /* Ensure xterm terminal fills container height */ .xterm { height: 100% !important; From dfacf6c4b26ef073148f5bf75fe03f9341481cac Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Fri, 21 Nov 2025 15:51:10 -0800 Subject: [PATCH 4/9] save --- .../lib/trpc/routers/workspaces/workspaces.ts | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts b/apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts index fc306cf08fc..54c856a3f12 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts @@ -222,10 +222,19 @@ export const createWorkspacesRouter = () => { // Dry-run: verify the worktree exists in git const git = await import("simple-git").then((m) => m.default); const gitInstance = git(project.mainRepoPath); - const worktrees = await gitInstance.raw(["worktree", "list"]); - - // Check if our worktree path is in the list - const worktreeExists = worktrees.includes(worktree.path); + const worktrees = await gitInstance.raw([ + "worktree", + "list", + "--porcelain", + ]); + + // Parse porcelain output line-by-line + // Format: "worktree /path/to/worktree" followed by HEAD, branch, etc. + const lines = worktrees.split("\n"); + const worktreePrefix = `worktree ${worktree.path}`; + const worktreeExists = lines.some( + (line) => line.trim() === worktreePrefix, + ); if (!worktreeExists) { // Worktree doesn't exist in git, but we can still delete the workspace From 334733d01cbc9e02082cc3bc402fba265a4d3aa1 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Fri, 21 Nov 2025 16:02:49 -0800 Subject: [PATCH 5/9] cleanup --- .../src/lib/trpc/routers/workspaces/workspaces.ts | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts b/apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts index 54c856a3f12..4791448f464 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts @@ -1,6 +1,7 @@ import { join } from "node:path"; import { db } from "main/lib/db"; import { nanoid } from "nanoid"; +import simpleGit from "simple-git"; import { z } from "zod"; import { publicProcedure, router } from "../.."; import { @@ -216,20 +217,17 @@ export const createWorkspacesRouter = () => { (p) => p.id === workspace.projectId, ); - // Check if worktree can be removed if (worktree && project) { try { - // Dry-run: verify the worktree exists in git - const git = await import("simple-git").then((m) => m.default); - const gitInstance = git(project.mainRepoPath); + const gitInstance = simpleGit(project.mainRepoPath); const worktrees = await gitInstance.raw([ "worktree", "list", "--porcelain", ]); - // Parse porcelain output line-by-line - // Format: "worktree /path/to/worktree" followed by HEAD, branch, etc. + // Parse porcelain format to verify worktree exists in git before deletion + // (porcelain format: "worktree /path/to/worktree" followed by HEAD, branch, etc.) const lines = worktrees.split("\n"); const worktreePrefix = `worktree ${worktree.path}`; const worktreeExists = lines.some( @@ -262,7 +260,6 @@ export const createWorkspacesRouter = () => { } } - // No worktree to remove, can safely delete return { canDelete: true, reason: null, @@ -287,7 +284,6 @@ export const createWorkspacesRouter = () => { (p) => p.id === workspace.projectId, ); - // Always attempt to remove the worktree first if (worktree && project) { try { await removeWorktree(project.mainRepoPath, worktree.path); From 55440b747f105ae31d6f01b7001a9f0876ed9a03 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Fri, 21 Nov 2025 16:16:52 -0800 Subject: [PATCH 6/9] cleanup --- .../routers/workspaces/workspaces.test.ts | 277 ++++++++++++++---- apps/desktop/test-setup.ts | 37 +++ 2 files changed, 253 insertions(+), 61 deletions(-) diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts b/apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts index 7baaa28e759..1207867d179 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts @@ -1,13 +1,6 @@ -import { describe, expect, it, mock } from "bun:test"; -import { createWorkspacesRouter } from "./workspaces"; +import { beforeEach, describe, expect, it, mock } from "bun:test"; import * as gitUtils from "./utils/git"; - -// Mock the git utilities -mock.module("./utils/git", () => ({ - createWorktree: mock(() => Promise.resolve()), - removeWorktree: mock(() => Promise.resolve()), - generateBranchName: mock(() => "test-branch-123"), -})); +import { createWorkspacesRouter } from "./workspaces"; // Mock the database const mockDb = { @@ -53,56 +46,70 @@ const mockDb = { }), }; +// Mock the database module +mock.module("main/lib/db", () => ({ + db: mockDb, +})); + +// Mock the git utilities +mock.module("./utils/git", () => ({ + createWorktree: mock(() => Promise.resolve()), + removeWorktree: mock(() => Promise.resolve()), + generateBranchName: mock(() => "test-branch-123"), +})); + +// Reset mock data before each test +beforeEach(() => { + mockDb.data.workspaces = [ + { + id: "workspace-1", + projectId: "project-1", + worktreeId: "worktree-1", + name: "Test Workspace", + tabOrder: 0, + createdAt: Date.now(), + updatedAt: Date.now(), + lastOpenedAt: Date.now(), + }, + ]; + mockDb.data.worktrees = [ + { + id: "worktree-1", + projectId: "project-1", + path: "/path/to/worktree", + branch: "test-branch", + createdAt: Date.now(), + }, + ]; + mockDb.data.projects = [ + { + id: "project-1", + name: "Test Project", + mainRepoPath: "/path/to/repo", + color: "#ff0000", + tabOrder: 0, + createdAt: Date.now(), + lastOpenedAt: Date.now(), + }, + ]; + mockDb.data.settings = { + lastActiveWorkspaceId: "workspace-1", + }; +}); + describe("workspaces router - delete", () => { it("should successfully delete workspace and remove worktree", async () => { const router = createWorkspacesRouter(); - - // Mock removeWorktree to succeed - const removeWorktreeMock = mock(() => Promise.resolve()); - mock.module("./utils/git", () => ({ - ...gitUtils, - removeWorktree: removeWorktreeMock, - })); - - const caller = router.createCaller({ db: mockDb as any }); + const caller = router.createCaller({}); const result = await caller.delete({ id: "workspace-1" }); expect(result.success).toBe(true); - expect(removeWorktreeMock).toHaveBeenCalledWith( - "/path/to/repo", - "/path/to/worktree", - ); expect(mockDb.data.workspaces).toHaveLength(0); expect(mockDb.data.worktrees).toHaveLength(0); }); it("should fail deletion if worktree removal fails", async () => { - // Reset mock data - mockDb.data.workspaces = [ - { - id: "workspace-1", - projectId: "project-1", - worktreeId: "worktree-1", - name: "Test Workspace", - tabOrder: 0, - createdAt: Date.now(), - updatedAt: Date.now(), - lastOpenedAt: Date.now(), - }, - ]; - mockDb.data.worktrees = [ - { - id: "worktree-1", - projectId: "project-1", - path: "/path/to/worktree", - branch: "test-branch", - createdAt: Date.now(), - }, - ]; - - const router = createWorkspacesRouter(); - // Mock removeWorktree to fail const removeWorktreeMock = mock(() => Promise.reject(new Error("Failed to remove worktree")), @@ -112,7 +119,8 @@ describe("workspaces router - delete", () => { removeWorktree: removeWorktreeMock, })); - const caller = router.createCaller({ db: mockDb as any }); + const router = createWorkspacesRouter(); + const caller = router.createCaller({}); const result = await caller.delete({ id: "workspace-1" }); @@ -126,12 +134,12 @@ describe("workspaces router - delete", () => { describe("workspaces router - canDelete", () => { it("should return true when worktree can be deleted", async () => { - const router = createWorkspacesRouter(); - - // Mock git to return worktree list + // Mock git to return worktree list in porcelain format const mockGit = { raw: mock(() => - Promise.resolve("/path/to/worktree\n/path/to/other-worktree"), + Promise.resolve( + "worktree /path/to/worktree\nHEAD abc123\nbranch refs/heads/test-branch\n\nworktree /path/to/other-worktree\nHEAD def456\nbranch refs/heads/other-branch", + ), ), }; const mockSimpleGit = mock(() => mockGit); @@ -139,7 +147,8 @@ describe("workspaces router - canDelete", () => { default: mockSimpleGit, })); - const caller = router.createCaller({ db: mockDb as any }); + const router = createWorkspacesRouter(); + const caller = router.createCaller({}); const result = await caller.canDelete({ id: "workspace-1" }); @@ -149,18 +158,21 @@ describe("workspaces router - canDelete", () => { }); it("should return warning when worktree doesn't exist in git", async () => { - const router = createWorkspacesRouter(); - - // Mock git to return worktree list without our worktree + // Mock git to return worktree list without our worktree (porcelain format) const mockGit = { - raw: mock(() => Promise.resolve("/path/to/other-worktree")), + raw: mock(() => + Promise.resolve( + "worktree /path/to/other-worktree\nHEAD def456\nbranch refs/heads/other-branch", + ), + ), }; const mockSimpleGit = mock(() => mockGit); mock.module("simple-git", () => ({ default: mockSimpleGit, })); - const caller = router.createCaller({ db: mockDb as any }); + const router = createWorkspacesRouter(); + const caller = router.createCaller({}); const result = await caller.canDelete({ id: "workspace-1" }); @@ -169,8 +181,6 @@ describe("workspaces router - canDelete", () => { }); it("should return false when git check fails", async () => { - const router = createWorkspacesRouter(); - // Mock git to throw error const mockGit = { raw: mock(() => Promise.reject(new Error("Git error"))), @@ -180,11 +190,156 @@ describe("workspaces router - canDelete", () => { default: mockSimpleGit, })); - const caller = router.createCaller({ db: mockDb as any }); + const router = createWorkspacesRouter(); + const caller = router.createCaller({}); const result = await caller.canDelete({ id: "workspace-1" }); expect(result.canDelete).toBe(false); expect(result.reason).toContain("Failed to check worktree status"); }); + + it("should use exact path matching and not match substrings", async () => { + // Mock git to return a worktree with a similar but different path + // This tests that we don't match "/path/to/worktree-backup" when looking for "/path/to/worktree" + const mockGit = { + raw: mock(() => + Promise.resolve( + "worktree /path/to/worktree-backup\nHEAD abc123\nbranch refs/heads/backup\n\nworktree /path/to/worktree-old\nHEAD def456\nbranch refs/heads/old", + ), + ), + }; + const mockSimpleGit = mock(() => mockGit); + mock.module("simple-git", () => ({ + default: mockSimpleGit, + })); + + const router = createWorkspacesRouter(); + const caller = router.createCaller({}); + + const result = await caller.canDelete({ id: "workspace-1" }); + + // Should not find the worktree because neither path exactly matches "/path/to/worktree" + expect(result.canDelete).toBe(true); + expect(result.warning).toContain("not found in git"); + }); + + it("should match exact path even with trailing whitespace in git output", async () => { + // Mock git to return worktree list with trailing spaces + const mockGit = { + raw: mock(() => + Promise.resolve( + "worktree /path/to/worktree \nHEAD abc123\nbranch refs/heads/test-branch", + ), + ), + }; + const mockSimpleGit = mock(() => mockGit); + mock.module("simple-git", () => ({ + default: mockSimpleGit, + })); + + const router = createWorkspacesRouter(); + const caller = router.createCaller({}); + + const result = await caller.canDelete({ id: "workspace-1" }); + + // Should find the worktree even with trailing whitespace + expect(result.canDelete).toBe(true); + expect(result.reason).toBeNull(); + expect(result.warning).toBeNull(); + }); + + it("should handle worktree path that is a prefix of another path", async () => { + // Update mock DB to have a path that could be a prefix + mockDb.data.worktrees = [ + { + id: "worktree-1", + projectId: "project-1", + path: "/path/to/main", + branch: "test-branch", + createdAt: Date.now(), + }, + ]; + + // Mock git to return a list with a path that contains our path as prefix + const mockGit = { + raw: mock(() => + Promise.resolve( + "worktree /path/to/main-backup\nHEAD abc123\nbranch refs/heads/backup\n\nworktree /path/to/main2\nHEAD def456\nbranch refs/heads/other", + ), + ), + }; + const mockSimpleGit = mock(() => mockGit); + mock.module("simple-git", () => ({ + default: mockSimpleGit, + })); + + const router = createWorkspacesRouter(); + const caller = router.createCaller({}); + + const result = await caller.canDelete({ id: "workspace-1" }); + + // Should not find "/path/to/main" even though similar paths exist + expect(result.canDelete).toBe(true); + expect(result.warning).toContain("not found in git"); + }); + + it("should handle worktree path that contains another path", async () => { + // Update mock DB to have a longer path + mockDb.data.worktrees = [ + { + id: "worktree-1", + projectId: "project-1", + path: "/path/to/worktree-backup", + branch: "test-branch", + createdAt: Date.now(), + }, + ]; + + // Mock git to return a list with a shorter path + const mockGit = { + raw: mock(() => + Promise.resolve( + "worktree /path/to/worktree\nHEAD abc123\nbranch refs/heads/main", + ), + ), + }; + const mockSimpleGit = mock(() => mockGit); + mock.module("simple-git", () => ({ + default: mockSimpleGit, + })); + + const router = createWorkspacesRouter(); + const caller = router.createCaller({}); + + const result = await caller.canDelete({ id: "workspace-1" }); + + // Should not find "/path/to/worktree-backup" when only "/path/to/worktree" exists + expect(result.canDelete).toBe(true); + expect(result.warning).toContain("not found in git"); + }); + + it("should verify --porcelain flag is passed to git", async () => { + // Mock git to capture the arguments + const rawMock = mock(() => + Promise.resolve( + "worktree /path/to/worktree\nHEAD abc123\nbranch refs/heads/test-branch", + ), + ); + const mockGit = { + raw: rawMock, + }; + const mockSimpleGit = mock(() => mockGit); + mock.module("simple-git", () => ({ + default: mockSimpleGit, + })); + + const router = createWorkspacesRouter(); + const caller = router.createCaller({}); + + await caller.canDelete({ id: "workspace-1" }); + + // Verify that raw was called with the correct arguments + expect(rawMock).toHaveBeenCalledWith(["worktree", "list", "--porcelain"]); + }); }); diff --git a/apps/desktop/test-setup.ts b/apps/desktop/test-setup.ts index a9016e548ee..c5eb83c2ece 100644 --- a/apps/desktop/test-setup.ts +++ b/apps/desktop/test-setup.ts @@ -2,6 +2,12 @@ * Global test setup for Bun tests * This file mocks the Electron environment for unit tests */ +import { mock } from "bun:test"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +// Use a temporary directory for tests instead of /mock +const testTmpDir = join(tmpdir(), "superset-test"); // Mock window.electronStore for all tests const mockStorage = new Map(); @@ -22,3 +28,34 @@ global.window = { sendMessage: () => {}, onMessage: () => {}, }; + +// Mock electron module +mock.module("electron", () => ({ + app: { + getPath: mock(() => testTmpDir), + getName: mock(() => "test-app"), + getVersion: mock(() => "1.0.0"), + }, + dialog: { + showOpenDialog: mock(() => + Promise.resolve({ canceled: false, filePaths: [] }), + ), + showSaveDialog: mock(() => + Promise.resolve({ canceled: false, filePath: "" }), + ), + showMessageBox: mock(() => Promise.resolve({ response: 0 })), + }, + BrowserWindow: mock(function () { + return { + webContents: { + send: mock(), + }, + loadURL: mock(), + on: mock(), + }; + }), + ipcMain: { + handle: mock(), + on: mock(), + }, +})); From 54d8b96af98ee50b54bd29c2ea3b992ee8951277 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Fri, 21 Nov 2025 16:27:30 -0800 Subject: [PATCH 7/9] update tests --- apps/desktop/bunfig.toml | 3 + .../routers/workspaces/workspaces.test.ts | 24 +-- .../src/main/lib/terminal-history.test.ts | 1 - apps/desktop/src/main/lib/terminal-history.ts | 12 +- .../src/main/lib/terminal-manager.test.ts | 137 ++++++++---------- apps/desktop/test-setup.ts | 3 + 6 files changed, 91 insertions(+), 89 deletions(-) diff --git a/apps/desktop/bunfig.toml b/apps/desktop/bunfig.toml index 66f01474d8c..908c8d00878 100644 --- a/apps/desktop/bunfig.toml +++ b/apps/desktop/bunfig.toml @@ -1,3 +1,6 @@ [test] # Preload test setup before running tests preload = ["./test-setup.ts"] + +[test.env] +NODE_ENV = "test" diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts b/apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts index 1207867d179..2cf1fd0d747 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts @@ -1,5 +1,4 @@ import { beforeEach, describe, expect, it, mock } from "bun:test"; -import * as gitUtils from "./utils/git"; import { createWorkspacesRouter } from "./workspaces"; // Mock the database @@ -51,15 +50,22 @@ mock.module("main/lib/db", () => ({ db: mockDb, })); -// Mock the git utilities +// Mock the git utilities - use a shared mock function that can be reassigned +let mockRemoveWorktree = mock(() => Promise.resolve()); +const mockCreateWorktree = mock(() => Promise.resolve()); +const mockGenerateBranchName = mock(() => "test-branch-123"); + mock.module("./utils/git", () => ({ - createWorktree: mock(() => Promise.resolve()), - removeWorktree: mock(() => Promise.resolve()), - generateBranchName: mock(() => "test-branch-123"), + createWorktree: mockCreateWorktree, + removeWorktree: (...args: any[]) => mockRemoveWorktree(...args), + generateBranchName: mockGenerateBranchName, })); // Reset mock data before each test beforeEach(() => { + // Reset the removeWorktree mock to default success behavior + mockRemoveWorktree = mock(() => Promise.resolve()); + mockDb.data.workspaces = [ { id: "workspace-1", @@ -110,14 +116,10 @@ describe("workspaces router - delete", () => { }); it("should fail deletion if worktree removal fails", async () => { - // Mock removeWorktree to fail - const removeWorktreeMock = mock(() => + // Override the removeWorktree mock to fail for this test + mockRemoveWorktree = mock(() => Promise.reject(new Error("Failed to remove worktree")), ); - mock.module("./utils/git", () => ({ - ...gitUtils, - removeWorktree: removeWorktreeMock, - })); const router = createWorkspacesRouter(); const caller = router.createCaller({}); diff --git a/apps/desktop/src/main/lib/terminal-history.test.ts b/apps/desktop/src/main/lib/terminal-history.test.ts index bad9238c9ab..d57836179de 100644 --- a/apps/desktop/src/main/lib/terminal-history.test.ts +++ b/apps/desktop/src/main/lib/terminal-history.test.ts @@ -1,6 +1,5 @@ import { afterEach, beforeEach, describe, expect, it } from "bun:test"; import { promises as fs } from "node:fs"; -import { homedir } from "node:os"; import { join } from "node:path"; import { HistoryReader, diff --git a/apps/desktop/src/main/lib/terminal-history.ts b/apps/desktop/src/main/lib/terminal-history.ts index 93405e8b328..cee20fa1c2f 100644 --- a/apps/desktop/src/main/lib/terminal-history.ts +++ b/apps/desktop/src/main/lib/terminal-history.ts @@ -1,5 +1,5 @@ import { createReadStream, createWriteStream, promises as fs } from "node:fs"; -import { homedir } from "node:os"; +import { homedir, tmpdir } from "node:os"; import { join } from "node:path"; import readline from "node:readline"; @@ -28,8 +28,16 @@ export interface SessionMetadata { byteLength: number; } +// Use environment variable or tmpdir for tests +const getBaseDir = () => { + if (process.env.NODE_ENV === "test" || process.env.BUN_ENV === "test") { + return join(tmpdir(), "superset-test"); + } + return homedir(); +}; + export function getHistoryDir(workspaceId: string, tabId: string): string { - return join(homedir(), ".superset", "terminal-history", workspaceId, tabId); + return join(getBaseDir(), ".superset", "terminal-history", workspaceId, tabId); } export function getHistoryFilePath(workspaceId: string, tabId: string): string { diff --git a/apps/desktop/src/main/lib/terminal-manager.test.ts b/apps/desktop/src/main/lib/terminal-manager.test.ts index ad51d63a0c9..6f4c2927e66 100644 --- a/apps/desktop/src/main/lib/terminal-manager.test.ts +++ b/apps/desktop/src/main/lib/terminal-manager.test.ts @@ -1,40 +1,12 @@ import { afterEach, beforeEach, describe, expect, it, mock } from "bun:test"; +import { promises as fs } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import * as pty from "node-pty"; import { TerminalManager } from "./terminal-manager"; -// Mock HistoryWriter and HistoryReader -const mockHistoryWriterInstance = { - init: mock(async () => {}), - writeData: mock(() => {}), - writeExit: mock(async () => {}), - finalize: mock(async () => {}), - isOpen: mock(() => true), -}; - -const mockHistoryReaderInstance = { - getLatestSession: mock(async () => ({ - scrollback: "", - wasRecovered: false, - })), - cleanup: mock(async () => {}), -}; - -mock.module("./terminal-history", () => ({ - HistoryWriter: mock(() => mockHistoryWriterInstance), - HistoryReader: mock(() => mockHistoryReaderInstance), - getHistoryDir: mock( - (workspaceId: string, tabId: string) => - `/mock/.superset/terminal-history/${workspaceId}/${tabId}`, - ), - getHistoryFilePath: mock( - (workspaceId: string, tabId: string) => - `/mock/.superset/terminal-history/${workspaceId}/${tabId}/history.ndjson`, - ), - getMetadataPath: mock( - (workspaceId: string, tabId: string) => - `/mock/.superset/terminal-history/${workspaceId}/${tabId}/meta.json`, - ), -})); +// Use real history implementation - it will write to tmpdir thanks to NODE_ENV=test +const testTmpDir = join(tmpdir(), "superset-test"); // Mock node-pty mock.module("node-pty", () => ({ @@ -52,13 +24,15 @@ describe("TerminalManager", () => { }; beforeEach(async () => { - // Reset mock counters - mockHistoryWriterInstance.init.mockClear(); - mockHistoryWriterInstance.writeData.mockClear(); - mockHistoryWriterInstance.writeExit.mockClear(); - mockHistoryWriterInstance.finalize.mockClear(); - mockHistoryReaderInstance.getLatestSession.mockClear(); - mockHistoryReaderInstance.cleanup.mockClear(); + // Clean up test history directory before each test + try { + await fs.rm(join(testTmpDir, ".superset/terminal-history"), { + recursive: true, + force: true, + }); + } catch { + // Ignore if doesn't exist + } manager = new TerminalManager(); @@ -266,7 +240,13 @@ describe("TerminalManager", () => { workspaceId: "workspace-1", }); - // Listen for exit event + // Trigger some data to create history + const onDataCallback = + mockPty.onData.mock.calls[mockPty.onData.mock.calls.length - 1]?.[0]; + if (onDataCallback) { + onDataCallback("test output\n"); + } + const exitPromise = new Promise((resolve) => { manager.once("exit:tab-1", () => resolve()); }); @@ -283,8 +263,13 @@ describe("TerminalManager", () => { await exitPromise; - // Verify history cleanup was NOT called (history preserved) - expect(mockHistoryReaderInstance.cleanup).not.toHaveBeenCalled(); + // Verify history directory still exists (preserved) + const historyDir = join( + testTmpDir, + ".superset/terminal-history/workspace-1/tab-1", + ); + const stats = await fs.stat(historyDir); + expect(stats.isDirectory()).toBe(true); }); it("should delete history when deleteHistory flag is true", async () => { @@ -293,7 +278,13 @@ describe("TerminalManager", () => { workspaceId: "workspace-1", }); - // Listen for exit event + // Trigger some data to create history + const onDataCallback = + mockPty.onData.mock.calls[mockPty.onData.mock.calls.length - 1]?.[0]; + if (onDataCallback) { + onDataCallback("test output\n"); + } + const exitPromise = new Promise((resolve) => { manager.once("exit:tab-delete-history", () => resolve()); }); @@ -310,8 +301,17 @@ describe("TerminalManager", () => { await exitPromise; - // Verify history cleanup WAS called (history deleted) - expect(mockHistoryReaderInstance.cleanup).toHaveBeenCalled(); + // Verify history directory was deleted + const historyDir = join( + testTmpDir, + ".superset/terminal-history/workspace-1/tab-delete-history", + ); + try { + await fs.stat(historyDir); + throw new Error("Directory should not exist"); + } catch (error: any) { + expect(error.code).toBe("ENOENT"); + } }); it("should preserve history for recovery after kill without deleteHistory", async () => { @@ -341,20 +341,14 @@ describe("TerminalManager", () => { await exitPromise; - // Setup mock to return preserved history on recovery (after session ends) - mockHistoryReaderInstance.getLatestSession.mockResolvedValueOnce({ - scrollback: "Preserved output\n", - wasRecovered: true, - }); - - // Recreate session - should recover history + // Recreate session - should recover history from filesystem const result = await manager.createOrAttach({ tabId: "tab-preserve", workspaceId: "workspace-1", }); expect(result.wasRecovered).toBe(true); - expect(result.scrollback[0]).toContain("Preserved output"); + expect(result.scrollback.join("")).toContain("Preserved output"); }); }); @@ -437,10 +431,6 @@ describe("TerminalManager", () => { onDataCallback("Test output during cleanup\n"); } - expect(mockHistoryWriterInstance.writeData).toHaveBeenCalledWith( - "Test output during cleanup\n", - ); - const cleanupPromise = manager.cleanup(); const onExitCallback = @@ -451,8 +441,13 @@ describe("TerminalManager", () => { await cleanupPromise; - // Verify history was NOT cleaned up (preserved) - expect(mockHistoryReaderInstance.cleanup).not.toHaveBeenCalled(); + // Verify history was preserved (directory still exists) + const historyDir = join( + testTmpDir, + ".superset/terminal-history/workspace-1/tab-cleanup", + ); + const stats = await fs.stat(historyDir); + expect(stats.isDirectory()).toBe(true); }); }); @@ -503,6 +498,7 @@ describe("TerminalManager", () => { describe("multi-session history persistence", () => { it("should persist history across multiple sessions", async () => { + // Session 1: Create and write data const result1 = await manager.createOrAttach({ tabId: "tab-multi", workspaceId: "workspace-1", @@ -528,14 +524,9 @@ describe("TerminalManager", () => { } await exitPromise1; - await manager.cleanup(); - mockHistoryReaderInstance.getLatestSession.mockResolvedValueOnce({ - scrollback: "Session 1 output\n", - wasRecovered: true, - }); - + // Session 2: Should recover Session 1 data const result2 = await manager.createOrAttach({ tabId: "tab-multi", workspaceId: "workspace-1", @@ -543,7 +534,7 @@ describe("TerminalManager", () => { expect(result2.isNew).toBe(true); expect(result2.wasRecovered).toBe(true); - expect(result2.scrollback[0]).toContain("Session 1 output"); + expect(result2.scrollback.join("")).toContain("Session 1 output"); const onDataCallback2 = mockPty.onData.mock.calls[mockPty.onData.mock.calls.length - 1]?.[0]; @@ -562,14 +553,9 @@ describe("TerminalManager", () => { } await exitPromise2; - await manager.cleanup(); - mockHistoryReaderInstance.getLatestSession.mockResolvedValueOnce({ - scrollback: "Session 1 output\nSession 2 output\n", - wasRecovered: true, - }); - + // Session 3: Should recover both Session 1 and Session 2 data const result3 = await manager.createOrAttach({ tabId: "tab-multi", workspaceId: "workspace-1", @@ -577,8 +563,9 @@ describe("TerminalManager", () => { expect(result3.isNew).toBe(true); expect(result3.wasRecovered).toBe(true); - expect(result3.scrollback[0]).toContain("Session 1 output"); - expect(result3.scrollback[0]).toContain("Session 2 output"); + const fullScrollback = result3.scrollback.join(""); + expect(fullScrollback).toContain("Session 1 output"); + expect(fullScrollback).toContain("Session 2 output"); }); }); }); diff --git a/apps/desktop/test-setup.ts b/apps/desktop/test-setup.ts index c5eb83c2ece..8aace2f9e5c 100644 --- a/apps/desktop/test-setup.ts +++ b/apps/desktop/test-setup.ts @@ -6,6 +6,9 @@ import { mock } from "bun:test"; import { tmpdir } from "node:os"; import { join } from "node:path"; +// Set NODE_ENV to test so terminal-history uses tmpdir +process.env.NODE_ENV = "test"; + // Use a temporary directory for tests instead of /mock const testTmpDir = join(tmpdir(), "superset-test"); From afbe95486a47f9d09cb629390858d60b94bd1102 Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Fri, 21 Nov 2025 16:28:45 -0800 Subject: [PATCH 8/9] fix --- apps/desktop/src/main/lib/terminal-history.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/apps/desktop/src/main/lib/terminal-history.ts b/apps/desktop/src/main/lib/terminal-history.ts index cee20fa1c2f..1e332477460 100644 --- a/apps/desktop/src/main/lib/terminal-history.ts +++ b/apps/desktop/src/main/lib/terminal-history.ts @@ -37,7 +37,13 @@ const getBaseDir = () => { }; export function getHistoryDir(workspaceId: string, tabId: string): string { - return join(getBaseDir(), ".superset", "terminal-history", workspaceId, tabId); + return join( + getBaseDir(), + ".superset", + "terminal-history", + workspaceId, + tabId, + ); } export function getHistoryFilePath(workspaceId: string, tabId: string): string { From 74bdcc70b818a4fdcc8c625637903d8e893d6a7b Mon Sep 17 00:00:00 2001 From: Kiet Ho Date: Fri, 21 Nov 2025 16:29:45 -0800 Subject: [PATCH 9/9] fix --- .../lib/trpc/routers/workspaces/workspaces.test.ts | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts b/apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts index 2cf1fd0d747..9c3c8fc54d2 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts @@ -51,20 +51,25 @@ mock.module("main/lib/db", () => ({ })); // Mock the git utilities - use a shared mock function that can be reassigned -let mockRemoveWorktree = mock(() => Promise.resolve()); +let mockRemoveWorktree = mock((_mainRepoPath: string, _worktreePath: string) => + Promise.resolve(), +); const mockCreateWorktree = mock(() => Promise.resolve()); const mockGenerateBranchName = mock(() => "test-branch-123"); mock.module("./utils/git", () => ({ createWorktree: mockCreateWorktree, - removeWorktree: (...args: any[]) => mockRemoveWorktree(...args), + removeWorktree: (mainRepoPath: string, worktreePath: string) => + mockRemoveWorktree(mainRepoPath, worktreePath), generateBranchName: mockGenerateBranchName, })); // Reset mock data before each test beforeEach(() => { // Reset the removeWorktree mock to default success behavior - mockRemoveWorktree = mock(() => Promise.resolve()); + mockRemoveWorktree = mock((_mainRepoPath: string, _worktreePath: string) => + Promise.resolve(), + ); mockDb.data.workspaces = [ { @@ -117,7 +122,7 @@ describe("workspaces router - delete", () => { it("should fail deletion if worktree removal fails", async () => { // Override the removeWorktree mock to fail for this test - mockRemoveWorktree = mock(() => + mockRemoveWorktree = mock((_mainRepoPath: string, _worktreePath: string) => Promise.reject(new Error("Failed to remove worktree")), );