Skip to content

navy meadow 16 #126

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

Closed
Kitenite wants to merge 2 commits into from
Closed

navy meadow 16 #126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5eaa502
confirm delete worktree
Kitenite Nov 21, 2025
5a3fbc2
confirm delete worktree
Kitenite Nov 21, 2025
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
190 changes: 190 additions & 0 deletions apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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);
}),
};
Comment on lines +13 to +54
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Nov 22, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

Router under test doesn’t actually use mockDb

In these tests you construct mockDb and pass it via router.createCaller({ db: mockDb as any }), then assert against mockDb.data. However, createWorkspacesRouter imports and uses the db singleton from "main/lib/db" directly (e.g., db.data.workspaces, db.update(...)) rather than ctx.db.

Unless "main/lib/db" is being mocked or initialized elsewhere to point at mockDb, these tests will not exercise mockDb at all and may either:

  • operate against the real in-memory DB (making the assertions on mockDb.data meaningless), or
  • throw “Database not initialized” if _db isn’t set.

I’d recommend either:

  • Refactoring the router to read ctx.db instead of the imported db, so the context you pass to createCaller is actually used; or
  • Mocking "main/lib/db" in this test to export { db: mockDb } so the router’s imported db is your mock.

I need to verify the concerns raised in the review comment by examining the actual test file and router implementation. Let me gather the necessary information:

🏁 Script executed:

#!/bin/bash
# Find and examine the test file
find . -path "*/apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts" -type f | head -1

Length of output: 65

🏁 Script executed:

#!/bin/bash
# Locate the workspaces router implementation
find . -path "*/workspaces.ts" -o -path "*/workspaces/index.ts" | grep -E "routers/workspaces" | grep -v test | head -5

Length of output: 116

🏁 Script executed:

#!/bin/bash
# Search for createWorkspacesRouter definition
rg -n "createWorkspacesRouter|export.*workspaces" --type=ts --type=tsx | grep -v test | head -20

Length of output: 27

🏁 Script executed:

#!/bin/bash
# Check for mock.module calls targeting "main/lib/db"
rg -n "mock\.module.*main/lib/db|mock\.module.*db" --type=ts --type=tsx | head -20

Length of output: 27

🏁 Script executed:

#!/bin/bash
# Look for the db singleton import/export in main/lib/db
find . -path "*/main/lib/db.ts" -o -path "*/main/lib/db/index.ts" | head -1

Length of output: 39

🏁 Script executed:

#!/bin/bash
# Search for test file imports and setup
rg -n "import.*db|import.*createCaller|describe|beforeEach" "apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts" -A 2 | head -40

Length of output: 513

🏁 Script executed:

#!/bin/bash
# Get the full workspaces router test file
cat "apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts" 2>/dev/null || echo "File not found, attempting alternative paths..." && find . -name "workspaces.test.ts" -path "*/routers/workspaces/*" -type f

Length of output: 5075

🏁 Script executed:

#!/bin/bash
# Examine the actual router implementation (workspaces router)
find . -path "*/routers/workspaces.ts" -o -path "*/routers/workspaces/index.ts" | grep -v test | xargs cat 2>/dev/null | head -100

Length of output: 108

🏁 Script executed:

#!/bin/bash
# Search more broadly for createWorkspacesRouter
rg "export.*createWorkspacesRouter|function createWorkspacesRouter|const createWorkspacesRouter" --type=ts --type=tsx -B 2 -A 10 | head -50

Length of output: 27

🏁 Script executed:

#!/bin/bash
# Check db usage pattern in workspaces router - look for ctx.db vs imported db
rg "ctx\.db|from.*main/lib/db" --type=ts --type=tsx | grep -i workspace | head -20

Length of output: 27

🏁 Script executed:

#!/bin/bash
# Look at db module structure to understand export pattern
find . -path "*/main/lib/db*" -type f | head -10

Length of output: 81

🏁 Script executed:

#!/bin/bash
# Search for db.data pattern usage in workspaces context
rg "db\.data\." --type=ts --type=tsx | grep -i workspace | head -10

Length of output: 27

🏁 Script executed:

#!/bin/bash
# Find all test setup code including mock.module calls
find . -name "workspaces.test.ts" -path "*/routers/workspaces/*" -exec cat {} \;

Length of output: 5009

🏁 Script executed:

#!/bin/bash
# More targeted search for the exact test file with full context
cat "apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts" 2>/dev/null || cat "./apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts" 2>/dev/null || rg "mockDb.*=" --type=ts -B 5 -A 50 | head -100

Length of output: 5009

🏁 Script executed:

#!/bin/bash
# Check test file structure with grep
grep -n "import\|mock\.module\|describe\|it\|createCaller" "apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts" 2>/dev/null | head -50

Length of output: 2229

🏁 Script executed:

#!/bin/bash
# Find the workspace router directory and list its contents
find . -type d -path "*/routers/workspaces" | head -1 | xargs ls -la

Length of output: 355

🏁 Script executed:

#!/bin/bash
# List files in the test directory
ls -la "apps/desktop/src/lib/trpc/routers/workspaces/" 2>/dev/null || ls -la "./apps/desktop/src/lib/trpc/routers/workspaces/" 2>/dev/null || echo "Attempting find..."  && find . -type d -name workspaces -path "*/routers/*"

Length of output: 402

🏁 Script executed:

#!/bin/bash
# Direct comprehensive check
if [ -f "apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts" ]; then
  echo "=== TEST FILE ===" && cat "apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts"
else
  echo "Finding test file..." && find . -name "workspaces.test.ts" -type f
fi

Length of output: 5027

🏁 Script executed:

#!/bin/bash
# Check the router implementation 
if [ -f "apps/desktop/src/lib/trpc/routers/workspaces.ts" ]; then
  echo "=== ROUTER FILE ===" && cat "apps/desktop/src/lib/trpc/routers/workspaces.ts"
else
  echo "Looking for router..." && find . -path "*/routers/workspaces*" -name "*.ts" -not -name "*.test.ts" | head -5
fi

Length of output: 198

🏁 Script executed:

#!/bin/bash
# Search for both files systematically
echo "Looking for test file..."
find . -name "workspaces.test.ts" 2>/dev/null | head -3
echo ""
echo "Looking for router file..."
find . -path "*/lib/trpc/routers/workspaces.ts" -o -path "*/lib/trpc/routers/workspaces/index.ts" 2>/dev/null | head -3

Length of output: 174

🏁 Script executed:

#!/bin/bash
# Final comprehensive attempt - check what files exist in the target directory
find . -type f -path "*/apps/desktop/src/lib/trpc/routers/workspaces/*" 2>/dev/null | head -20

Length of output: 242

🏁 Script executed:

#!/bin/bash
# Read the actual workspaces router implementation
cat "apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts"

Length of output: 9955

🏁 Script executed:

#!/bin/bash
# Read the db module to understand structure
cat "apps/desktop/src/main/lib/db/index.ts"

Length of output: 726

🏁 Script executed:

#!/bin/bash
# Search for ctx.db or imported db usage in workspaces.ts
grep -n "ctx\|db\|import" "apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts" | head -50

Length of output: 1298

Router uses imported db singleton, not ctx.db—mockDb is unused

The review comment is correct. The router definitively imports and uses the db singleton from "main/lib/db" (line 2 of workspaces.ts), accessing it directly via db.data.* and db.update() throughout all resolvers. The router does not accept or use ctx.db at all.

Since mockDb is passed to createCaller({ db: mockDb as any }) but the router never reads it, the tests do not exercise the mock. Instead:

  • If _db is uninitialized, resolvers will throw "Database not initialized. Call initDb() first."
  • If _db happens to be set, tests will operate against the real singleton, making assertions on mockDb.data meaningless.

To fix this:

  1. Mock "main/lib/db" at the test top-level to export { db: mockDb }, so the router's import resolves to your mock; or
  2. Refactor the router to accept and use ctx.db instead of the imported singleton.
🤖 Prompt for AI Agents 
In apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts around lines
13–54, the test constructs mockDb but the router imports and uses the singleton
from "main/lib/db" so mockDb is never used; fix by mocking the module import at
the top of the test so "main/lib/db" exports { db: mockDb } (ensure the mock
export shape matches the real db API including .data and .update) so the router
resolves to the mock during tests; alternatively, refactor the router to use
ctx.db instead of importing the singleton so tests can pass a db via
createCaller—pick one approach and implement consistent module-mocking or
refactor accordingly.


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");
});
});
77 changes: 76 additions & 1 deletion apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }) => {
Expand All @@ -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);

Expand Down
97 changes: 97 additions & 0 deletions ...sktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 (
<AlertDialog open={open} onOpenChange={onOpenChange}>
<AlertDialogContent>
<AlertDialogHeader>
<AlertDialogTitle>Delete Workspace</AlertDialogTitle>
<AlertDialogDescription>
{isLoading ? (
<span>Checking workspace status...</span>
) : !canDelete ? (
<span className="text-destructive">
Cannot delete workspace: {reason}
</span>
) : (
<>
Are you sure you want to delete "{workspaceName}"?
{warning && (
<span className="block mt-2 text-yellow-600 dark:text-yellow-400">
Warning: {warning}
</span>
)}
<span className="block mt-2">
This will remove the workspace and its associated git
worktree. This action cannot be undone.
</span>
</>
)}
</AlertDialogDescription>
</AlertDialogHeader>
<AlertDialogFooter>
<AlertDialogCancel disabled={isDeleting}>Cancel</AlertDialogCancel>
<AlertDialogAction
onClick={(e: React.MouseEvent) => {
e.preventDefault();
handleDelete();
}}
disabled={!canDelete || isDeleting || isLoading}
className="bg-destructive text-white hover:bg-destructive/90"
>
{isDeleting ? "Deleting..." : "Delete"}
</AlertDialogAction>
</AlertDialogFooter>
</AlertDialogContent>
</AlertDialog>
);
}
Loading