Conversation
|
Warning Rate limit exceeded@Kitenite has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 42 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
WalkthroughAdds TRPC canDelete; changes delete to attempt Git worktree removal before DB cleanup and abort on failure. Introduces a DeleteWorkspaceDialog UI integrated into WorkspaceItem, adds a Radix-based alert-dialog component and export, expands tests (workspaces, terminal-manager/history) to use more realistic environments, and updates global CSS and test setup. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WorkspaceItem
participant DeleteDialog
participant TRPC
participant Git
participant DB
User->>WorkspaceItem: click delete
WorkspaceItem->>DeleteDialog: open dialog
DeleteDialog->>TRPC: canDelete(workspaceId)
TRPC->>Git: list worktrees (--porcelain)
alt worktree present
Git-->>TRPC: entry found
TRPC-->>DeleteDialog: canDelete: true
else worktree missing
Git-->>TRPC: no entry
TRPC-->>DeleteDialog: canDelete: true + warning
else git error
Git-->>TRPC: error
TRPC-->>DeleteDialog: canDelete: false + reason
end
User->>DeleteDialog: confirm delete
DeleteDialog->>TRPC: delete(workspaceId)
TRPC->>Git: removeWorktree(path)
alt removal success or none
Git-->>TRPC: removed/none
TRPC->>DB: remove workspace, update project state
DB-->>TRPC: success
TRPC-->>DeleteDialog: success
else removal failure
Git-->>TRPC: error
TRPC-->>DeleteDialog: error (DB unchanged)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts (1)
13-54: Improve test isolation with proper setup/teardown.The shared
mockDbobject is mutated across tests, which can cause tests to interfere with each other. Test 1 empties the arrays (lines 76-77), and test 2 manually restores them (lines 82-102), creating brittle dependencies between tests.Apply this pattern to ensure each test starts with fresh state:
+import { beforeEach } from "bun:test"; + +const createMockDb = () => ({ + 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: any) => { + fn(mockDb.data); + }), +}); + +let mockDb: ReturnType<typeof createMockDb>; + +beforeEach(() => { + mockDb = createMockDb(); +});Then remove the manual state reset in test 2 (lines 82-102).
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsx (1)
28-35: Reset state when dialog closes.The
isDeletingstate isn't reset when the dialog is closed externally (viaonOpenChange(false)from outside). This could leave the dialog in a "deleting" state if reopened.Add an effect to reset state when the dialog closes:
+import { useEffect, useState } from "react"; -import { useState } from "react"; export function DeleteWorkspaceDialog({ workspaceId, workspaceName, open, onOpenChange, }: DeleteWorkspaceDialogProps) { const [isDeleting, setIsDeleting] = useState(false); const deleteWorkspace = useDeleteWorkspace(); + useEffect(() => { + if (!open) { + setIsDeleting(false); + } + }, [open]); + // Query to check if workspace can be deleted const { data: canDeleteData, isLoading } = trpc.workspaces.canDelete.useQuery(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts(1 hunks)apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts(2 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (2)
apps/desktop/src/main/lib/db/index.ts (1)
db(19-26)apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (1)
removeWorktree(68-81)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts (2)
apps/desktop/src/lib/trpc/index.ts (1)
router(15-15)apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (1)
createWorkspacesRouter(12-386)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx (2)
packages/ui/src/lib/utils.ts (1)
cn(4-6)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsx (1)
DeleteWorkspaceDialog(22-97)
🔇 Additional comments (3)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx (1)
66-120: LGTM! Clean integration of the confirmation dialog.The refactor successfully replaces direct deletion with a dialog-based workflow. The state management is straightforward, and the separation of concerns between the workspace item and deletion logic is clear.
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (1)
281-295: LGTM! Atomic deletion with proper error handling.The updated delete flow correctly ensures worktree removal happens before database cleanup, and properly handles failures by preventing partial deletions. The error messages are informative and the transaction semantics are sound.
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts (1)
60-65: Router is mocked after creation, preventing per-test mocks from taking effect.The code creates the
routerinstance at line 59 before callingmock.module()at lines 62-65. IfcreateWorkspacesRouter()imports and caches./utils/gitat instantiation time, subsequent re-mocking won't affect the already-created router. Per-testmock.module()calls must occur before the code that uses the mocked module, or the module must be re-imported after the mock is registered.Additionally, spreading
...gitUtils(imported at the top level before any test-specific mocking) will use the original module's exports, not test-specific overrides.To fix:
- Mock before creating the router, OR
- Re-import the module inside the test after calling
mock.module(), OR- Pass git utilities as dependencies to
createWorkspacesRouter()to enable injection per test
| 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); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Display deletion errors to the user.
When deletion fails, the error is only logged to the console (line 43). Users won't see any feedback indicating why the deletion failed.
Add error state and display it in the dialog:
export function DeleteWorkspaceDialog({
workspaceId,
workspaceName,
open,
onOpenChange,
}: DeleteWorkspaceDialogProps) {
const [isDeleting, setIsDeleting] = useState(false);
+ const [deleteError, setDeleteError] = useState<string | null>(null);
const deleteWorkspace = useDeleteWorkspace();
// Query to check if workspace can be deleted
const { data: canDeleteData, isLoading } = trpc.workspaces.canDelete.useQuery(
{ id: workspaceId },
{ enabled: open },
);
const handleDelete = async () => {
setIsDeleting(true);
+ setDeleteError(null);
try {
await deleteWorkspace.mutateAsync({ id: workspaceId });
onOpenChange(false);
} catch (error) {
- console.error("Failed to delete workspace:", error);
+ const message = error instanceof Error ? error.message : "Failed to delete workspace";
+ setDeleteError(message);
} finally {
setIsDeleting(false);
}
};Then display the error in the dialog description (after line 78):
</>
)}
+ {deleteError && (
+ <span className="block mt-2 text-destructive">
+ {deleteError}
+ </span>
+ )}
</AlertDialogDescription>Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsx
around lines 37–47 and the dialog render (after line 78), add an error state and
surface it to the user: create a state like error:string|null and set it to null
before starting deletion, in the catch block setError to the caught error
message (use error?.message || String(error)), keep setIsDeleting logic as-is,
and then render the error text in the dialog description area (after line 78) so
failures are visible to the user (style as an inline error message and ensure
it’s cleared when the dialog opens or when retrying).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/ui/src/alert-dialog.tsx (1)
1-1: Barrel export is unreachable due to package.json configuration.This barrel export is correctly implemented, but the export path in
packages/ui/package.json(line 7) points directly to"./src/components/alert-dialog.tsx"instead of this file, making this barrel export unreachable.Refer to the review comment on
packages/ui/package.jsonline 7 for the resolution.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
packages/ui/package.json(3 hunks)packages/ui/src/alert-dialog.tsx(1 hunks)packages/ui/src/components/alert-dialog.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/ui/src/components/alert-dialog.tsx (1)
packages/ui/src/lib/utils.ts (1)
cn(4-6)
🔇 Additional comments (9)
packages/ui/src/components/alert-dialog.tsx (8)
1-5: LGTM!The imports are clean and follow standard patterns for wrapping Radix UI primitives with custom styling.
7-27: LGTM!The wrapper components correctly forward props to Radix primitives and consistently add
data-slotattributes for styling hooks.
29-43: LGTM!The overlay component properly handles animations and applies appropriate styling for a modal backdrop.
45-62: LGTM!The content component correctly nests the overlay and content within a portal, with appropriate responsive styling and animations for a centered modal dialog.
64-91: LGTM!The header and footer components provide sensible responsive layouts, with the footer's column-reverse pattern ensuring the primary action appears first on mobile devices.
93-117: LGTM!The title and description components correctly wrap Radix primitives to maintain proper accessibility attributes while applying appropriate visual hierarchy through styling.
119-141: LGTM!The action components apply appropriate button styling while preserving Radix's built-in accessibility and keyboard handling. The outlined variant for cancel provides clear visual distinction.
143-155: LGTM!All defined components are properly exported for consumption.
packages/ui/package.json (1)
33-33: Compatibility verified with React 19.Both
@radix-ui/react-alert-dialog@^1.1.15and@radix-ui/react-slot@^1.2.4list React 19 in their peerDependencies and are officially compatible with React 19.1.1. No action needed.
| "private": true, | ||
| "type": "module", | ||
| "exports": { | ||
| "./alert-dialog": "./src/components/alert-dialog.tsx", |
There was a problem hiding this comment.
Export path bypasses the barrel export file.
The export path points directly to "./src/components/alert-dialog.tsx", but a barrel export file exists at packages/ui/src/alert-dialog.tsx that re-exports from "./components/alert-dialog". This makes the barrel export unreachable and creates unnecessary indirection.
Either update the export path to use the barrel export, or remove the barrel export file entirely. Looking at the rest of the exports in this file, they all point directly to component files, so removing the barrel export at packages/ui/src/alert-dialog.tsx would be more consistent.
Apply this diff to align the export path:
- "./alert-dialog": "./src/components/alert-dialog.tsx",
+ "./alert-dialog": "./src/alert-dialog.tsx",OR remove the barrel export file packages/ui/src/alert-dialog.tsx entirely to match the pattern of other exports.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "./alert-dialog": "./src/components/alert-dialog.tsx", | |
| "./alert-dialog": "./src/alert-dialog.tsx", |
🤖 Prompt for AI Agents
In packages/ui/package.json around line 7 the export "./alert-dialog":
"./src/components/alert-dialog.tsx" bypasses the barrel file at
packages/ui/src/alert-dialog.tsx; to fix, remove the redundant barrel file
packages/ui/src/alert-dialog.tsx (or alternatively change the export to point to
"./alert-dialog" if you prefer to keep the barrel) so exports are consistent
with the rest of the package.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/desktop/src/renderer/globals.css (1)
132-150: Viewport-filling layout approach looks solid.The changes ensure the HTML root and React app element properly fill the entire Electron viewport and prevent scroll-lock layout shifts. The viewport-filling strategy with
overflow: hiddenis well-suited for this use case.Note: The
min-height: 100vhproperty at lines 134 and 149 is redundant whenheight: 100vhis already explicitly set, since the explicit height takes precedence. Consider removing themin-heightdeclarations for clarity, unless there's a specific reason to retain them (e.g., legacy browser compatibility).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; }
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
apps/desktop/test-setup.ts (2)
5-10: Temp dir choice is fine, but consider per‑run/per‑test isolationUsing
join(tmpdir(), "superset-test")is reasonable, but it reuses a single path across all runs. That can leave stale files between runs or cause interference if tests run in parallel processes. Consider switching to a unique directory per run (e.g., viamkdtempor a random suffix) and optionally cleaning it up in global test hooks.
32-61: Electron mock shape looks good; double‑check usage patterns (constructor vs functions & dialog responses)The overall
mock.module("electron", ...)shape is solid, but there are a couple of gotchas to verify:
BrowserWindowis amock(function () { ... }). Ensure all call sites use it in a way compatible with Bun’smock(especially ifnew BrowserWindow(...)is used), and that tests don’t rely on real BrowserWindow methods beyond the ones you stubbed.dialog.showOpenDialogandshowSaveDialogalways resolve to “not canceled” but with empty/blank paths. If app code assumesfilePaths[0]/filePathis non‑empty whenevercanceled === false, tests may fail in non‑obvious ways; you might want to return a dummy path instead or explicitly handle the “no selection” case in app code.Everything else (app getters,
ipcMain.handle/on) aligns well with typical usage and should be easy to assert against in tests.apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (3)
4-4: Staticsimple-gitimport and test mocking implicationsSwitching to a static default import is fine for the desktop (Node/Electron) runtime, but it has two side effects:
simple-gitis now eagerly loaded when this module is evaluated, even if no procedure ever callscanDeleteordelete.- In the tests,
mock.module("simple-git", ...)is called aftercreateWorkspacesRouteris imported, and this file uses a static import. Depending on Bun’smock.modulesemantics, that may mean the router keeps a reference to the realsimple-gitimplementation instead of the mock.Consider either moving the
mock.module("simple-git", ...)calls into a test-setup that runs before./workspacesis imported, or reverting to a dynamic import insidecanDeleteto make mocking and lazy loading more predictable. Please verify how this behaves underbun:testin this repo.
200-269:canDeleteworktree detection looks solid; consider tightening response shapeThe
canDeleteimplementation correctly:
- Looks up the workspace/worktree/project in the DB.
- Uses
git worktree list --porcelainand an exactline.trim() === \worktree ${worktree.path}`` check to avoid substring/path-prefix issues.- Differentiates between “worktree not found in git” (allowed with a warning), “git check failed” (blocked with a reason), and “no associated worktree” (allowed with a different warning).
One optional improvement: make the returned object shape fully consistent across all branches by always including a
warningfield (e.g.warning: nullwhen there is no warning, including in the “workspace not found” and “git check fails” cases). That keeps the API simpler for callers that want to render bothreasonandwarningwithout extra undefined checks.
287-303: Align delete behavior withcanDelete’s “worktree missing in git” scenarioRight now,
deleteunconditionally attemptsremoveWorktree(project.mainRepoPath, worktree.path)whenever a worktree record and project exist, and treats any failure as fatal (no DB cleanup). However,canDeleteexplicitly has a “worktree not found in git (may have been manually removed)” case where it returnscanDelete: trueplus a warning.If a caller follows the intended flow (call
canDelete, seecanDelete: truewith that warning, then calldelete), they might still get a failed deletion and no DB cleanup ifremoveWorktreeerrors in that “missing worktree” state.To avoid that mismatch, consider:
- Reusing the same porcelain-based existence check in
deleteand skippingremoveWorktreewhen the worktree is not registered in git, or- Relaxing this catch block to treat “worktree not found”–style errors as non-fatal while still failing on genuine git errors.
That would keep the actual deletion behavior aligned with what
canDeleteadvertised to the UI.apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts (1)
135-345:canDeletetests are comprehensive; consider also asserting the workspace payloadThe
canDeletesuite exercises a wide range of scenarios (exact match, missing worktree, git error, substring/prefix pitfalls, trailing whitespace, and verifying--porcelainis passed), which gives strong confidence in the git parsing logic.As an optional enhancement, you might also:
- Assert that
result.workspaceis the expected workspace object in the successful/warning cases, and- (Optionally) add a small test for the “workspace not found” path, confirming
canDelete: false,workspace: null, and the expectedreason.That would fully lock in both the behavioral and structural aspects of the API for downstream consumers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts(1 hunks)apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts(3 hunks)apps/desktop/test-setup.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (1)
apps/desktop/src/main/lib/db/index.ts (1)
db(19-26)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts (2)
apps/desktop/src/lib/trpc/index.ts (1)
router(15-15)apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (1)
createWorkspacesRouter(13-391)
🔇 Additional comments (2)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts (2)
5-53: Verifymock.moduleorder with statically imported router and dependenciesIn this file,
createWorkspacesRouter(which statically importsdbfrom"main/lib/db"andsimpleGitfrom"simple-git") is imported at the top, while themock.module("main/lib/db", ...)and variousmock.module("simple-git", ...)calls are registered later inside the file and individual tests.Depending on how
bun:testimplementsmock.module, those mocks may not affect the already-evaluated imports inside./workspaces, meaning the router could still be using the real DB proxy and realsimple-gitinstead of these mocks.It’s worth double-checking that:
- The mocks actually intercept the bindings used inside
createWorkspacesRouter, and- If not, that you either move the
mock.modulecalls into a shared test-setup that runs before importing./workspaces, or switch the router/tests to use dynamic imports after mocks are registered.Please confirm this under your current Bun version so these tests remain reliable.
Also applies to: 61-98
100-133: Delete tests correctly cover success and failure behaviorThese two tests do a good job of pinning down the intended delete semantics:
- Successful path: asserts
success === trueand that both workspace and worktree entries are removed from the mock DB.- Failure path: explicitly mocks
removeWorktreeto throw, then assertssuccess === false, the error message contains the expected prefix, and — importantly — that the workspace and worktree records remain in the DB.This nicely guards against regressions where DB cleanup might still run after a failed worktree removal.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts (1)
112-121: Consider verifying the git operation was invoked.The test correctly validates the deletion outcome, but doesn't assert that
mockRemoveWorktreewas called with the expected arguments.Add this assertion before line 118:
const result = await caller.delete({ id: "workspace-1" }); + expect(mockRemoveWorktree).toHaveBeenCalledWith("/path/to/repo", "/path/to/worktree"); expect(result.success).toBe(true);apps/desktop/src/main/lib/terminal-history.ts (1)
2-2: Test-aware base dir for terminal history looks good; consider sharing the base-path constantUsing
tmpdir()/superset-testunderNODE_ENV/BUN_ENV === "test"keeps production behavior unchanged while preventing tests from touching the real home directory, which is a solid pattern. The only minor risk is duplication of the"superset-test"base path and.superset/terminal-historylayout between this module and tests; if this ever changes, tests and implementation must be updated in sync. You might later factor the base path pieces into shared constants (or export a small helper) consumed by tests to avoid drift, but it’s not strictly necessary right now.Also applies to: 31-37, 40-46
apps/desktop/src/main/lib/terminal-manager.test.ts (1)
2-4: Tmpdir-based test setup matches history implementation; watch for duplication and potential parallelismPointing tests at
join(tmpdir(), "superset-test")and cleaning.superset/terminal-historybefore each run aligns nicely with the newgetBaseDir()logic and avoids polluting real user homes. Two optional considerations:
- The
"superset-test"suffix and.superset/terminal-historypath are now duplicated here and in the implementation; extracting shared constants or a small helper later would reduce drift risk.- If Bun runs tests in parallel across files sharing this tmp root, the
fs.rminbeforeEachcould cause cross-test interference; using a per-test subdirectory (e.g., incorporating a unique suffix) would harden against that scenario.Also applies to: 8-9, 27-35
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/desktop/bunfig.toml(1 hunks)apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts(1 hunks)apps/desktop/src/main/lib/terminal-history.test.ts(0 hunks)apps/desktop/src/main/lib/terminal-history.ts(2 hunks)apps/desktop/src/main/lib/terminal-manager.test.ts(11 hunks)apps/desktop/test-setup.ts(2 hunks)
💤 Files with no reviewable changes (1)
- apps/desktop/src/main/lib/terminal-history.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/test-setup.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts (2)
apps/desktop/src/lib/trpc/index.ts (1)
router(15-15)apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (1)
createWorkspacesRouter(13-391)
🔇 Additional comments (8)
apps/desktop/bunfig.toml (1)
5-6: Idiomatic Bun configuration for test environment variables.The new
[test.env]section correctly formalizes theNODE_ENV = "test"configuration at the Bun config level, complementing the existing preload setup. This is a clean, explicit approach that takes precedence over any code-level environment variable setting.apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts (2)
53-65: LGTM with a note on the mock pattern.The pattern of using a reassignable
let mockRemoveWorktreethat's invoked via a closure (lines 62-63) works correctly and allows per-test customization. However, this indirect reference pattern could be confusing to maintainers.
209-327: Excellent edge case coverage for path matching.These tests thoroughly validate exact path matching behavior, including substring exclusion, prefix conflicts, and trailing whitespace handling. The comprehensive coverage ensures the porcelain format parsing is robust.
apps/desktop/src/main/lib/terminal-manager.test.ts (5)
243-248: Realistic history-creation flow in “kill and preserve history” test looks solidSimulating PTY output via the captured
onDatacallback before callingkill, then asserting that the corresponding history directory exists under the test tmpdir, exercises the full integration path (manager → history writer → filesystem) instead of relying on mocks. This is a good, behavior-focused assertion for the default “preserve history” case, and the path you’re checking matches the implementation’s layout for test environments.Also applies to: 266-272
281-286: Delete-history-on-kill behavior is well covered with filesystem assertionsTriggering data via
onDatato ensure history exists, then killing withdeleteHistory: trueand asserting thatfs.statfails withENOENTgives a clear signal that the on-disk history directory was actually removed. The try/catch pattern ensures the test fails if the directory still exists or if a different error occurs, which is appropriate for validating the cleanup behavior.Also applies to: 304-314
344-352: Recovery test correctly validates preserved scrollback contentThe “preserve history for recovery” test now does more than just check the boolean flag: it recreates the session and asserts that
scrollback.join("")contains the previously written"Preserved output". That directly validates that the manager and history reader are wired correctly to reconstruct prior output, not just that some history existed. This looks like a meaningful improvement in coverage.
445-450: Cleanup-preserves-history assertion matches intended semanticsAfter writing output and invoking
manager.cleanup(), asserting that the history directory under the test tmpdir still exists gives a concrete guarantee that global cleanup does not wipe history by default. This pairs well with the delete-history-specific tests and clearly documents the intended contract ofcleanup.
501-501: Multi-session history persistence test thoroughly exercises append-and-recover behaviorThe three-session test that:
- Writes
"Session 1 output"then cleans up and verifies it’s recovered in session 2, and- Writes
"Session 2 output"and verifies that session 3 recovers both messagesdoes a good job of validating that history is appended and read across multiple lifecycle resets, not just a single crash/kill. Using
scrollback.join("")for assertions is consistent with other tests and keeps the expectations straightforward. This provides strong end-to-end coverage for the history persistence contract.Also applies to: 529-529, 537-538, 558-558, 566-568
| describe("workspaces router - canDelete", () => { | ||
| it("should return true when worktree can be deleted", async () => { | ||
| // Mock git to return worktree list in porcelain format | ||
| const mockGit = { | ||
| raw: mock(() => | ||
| 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); | ||
| mock.module("simple-git", () => ({ | ||
| default: mockSimpleGit, | ||
| })); | ||
|
|
||
| const router = createWorkspacesRouter(); | ||
| const caller = router.createCaller({}); | ||
|
|
||
| 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 () => { | ||
| // Mock git to return worktree list without our worktree (porcelain format) | ||
| const mockGit = { | ||
| 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 router = createWorkspacesRouter(); | ||
| const caller = router.createCaller({}); | ||
|
|
||
| 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 () => { | ||
| // 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 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"]); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor simple-git mocking for better test isolation.
Each canDelete test re-mocks the simple-git module inline (e.g., lines 153–155, 177–179, etc.). Because mock.module() affects global module resolution, this approach can cause test isolation issues—especially if tests run in parallel or if mocks don't properly reset between runs.
Refactor to a module-level mock with per-test configuration, similar to the mockRemoveWorktree pattern:
+// Mock simple-git at module level
+let mockGitRaw = mock(() => Promise.resolve(""));
+const mockGit = {
+ raw: (...args: any[]) => mockGitRaw(...args),
+};
+const mockSimpleGit = mock(() => mockGit);
+mock.module("simple-git", () => ({
+ default: mockSimpleGit,
+}));
+
// Reset mock data before each test
beforeEach(() => {
// Reset the removeWorktree mock to default success behavior
mockRemoveWorktree = mock((_mainRepoPath: string, _worktreePath: string) =>
Promise.resolve(),
);
+
+ // Reset git mock to default behavior
+ mockGitRaw = mock(() => Promise.resolve(""));
mockDb.data.workspaces = [Then in each test, reassign mockGitRaw instead of re-mocking the module:
it("should return true when worktree can be deleted", async () => {
- // Mock git to return worktree list in porcelain format
- const mockGit = {
- raw: mock(() =>
- 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);
- mock.module("simple-git", () => ({
- default: mockSimpleGit,
- }));
+ mockGitRaw = mock(() =>
+ 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 router = createWorkspacesRouter();Apply this pattern to all canDelete tests.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts around lines
142-352, the tests repeatedly call mock.module("simple-git", ...) inside each it
block which pollutes global module resolution and breaks isolation; refactor by
adding a module-level mock for simple-git that exports a controllable mockGit
object (e.g., a top-level mockGitRaw function and mockSimpleGit that returns {
raw: mockGitRaw }) and replace all per-test mock.module(...) calls with
assignments to mockGitRaw inside beforeEach/each test so tests only reassign the
raw behavior rather than re-registering the module; update every canDelete test
to use the shared mock (and reset mockGitRaw between tests) and remove the
inline mock.module invocations.
1 participant
Summary by CodeRabbit
New Features
UI
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.