fix(desktop): no confirmation for empty workspace delete#266
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds git-state checks for workspace deletion: two helpers (hasUncommittedChanges, hasUnpushedCommits), canDelete now returns hasChanges and hasUnpushedCommits and accepts skipGitChecks, backend runs parallel git checks, tests extended for git scenarios, and UI uses an initial git-status query plus a lightweight polling query and shows warnings for uncommitted/unpushed changes; WorkspaceItem can auto-delete when safe. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as WorkspaceItem / DeleteDialog
participant TRPC as trpc.workspaces.canDelete
participant Git as Git utils (simple-git)
participant Storage as Workspace storage (delete)
User->>UI: Click "Delete"
UI->>UI: handleDeleteClick (debounce, show pending)
UI->>TRPC: Query canDelete(id, skipGitChecks=false)
TRPC->>Git: hasUncommittedChanges(worktreePath)
TRPC->>Git: hasUnpushedCommits(worktreePath)
Git-->>TRPC: return booleans
TRPC-->>UI: { canDelete, activeTerminalCount, hasChanges, hasUnpushedCommits }
alt canDelete && activeTerminalCount==0 && !hasChanges && !hasUnpushedCommits
UI->>Storage: deleteWorkspace(id)
Storage-->>UI: success
UI->>User: show success toast
else
UI->>UI: open DeleteWorkspaceDialog (shows warnings)
User->>UI: confirm
UI->>Storage: deleteWorkspace(id)
Storage-->>User: success
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)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…c386 # Conflicts: # apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
AviPeltz
changed the title
AviPeltz
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (1)
373-379: Consider adding error handling for consistency.
hasUnpushedCommitsgracefully handles errors by returningfalse, buthasUncommittedChangeswill propagate exceptions ifgit.status()fails (e.g., corrupted repo, missing.git). This could causecanDeleteto fail entirely.export async function hasUncommittedChanges( worktreePath: string, ): Promise<boolean> { - const git = simpleGit(worktreePath); - const status = await git.status(); - return !status.isClean(); + try { + const git = simpleGit(worktreePath); + const status = await git.status(); + return !status.isClean(); + } catch { + // If status check fails, assume no uncommitted changes to allow deletion + return false; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts(1 hunks)apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts(3 hunks)apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts(5 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsx(3 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts
- apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx
🧰 Additional context used
📓 Path-based instructions (5)
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
For Electron interprocess communication, ALWAYS use tRPC as defined in
src/lib/trpc
Files:
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.tsapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsxapps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.tsapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsxapps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Co-locate component dependencies (utils, hooks, constants, config, tests, stories) next to the file using them
Avoidanytype in TypeScript unless absolutely necessary
Files:
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.tsapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsxapps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts
**/{components,pages}/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
**/{components,pages}/**/*.tsx: Use one folder per component with structureComponentName/ComponentName.tsxplusindex.tsfor barrel export
Create one component per file - do not use multi-component files
Files:
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsx
apps/desktop/src/{renderer,shared,preload}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules in renderer process code or shared code in the desktop app
Files:
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsx
🧬 Code graph analysis (2)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsx (1)
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (1)
hasUnpushedCommits(386-417)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (1)
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (2)
hasUncommittedChanges(373-379)hasUnpushedCommits(386-417)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (7)
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (1)
386-417: LGTM!The implementation correctly handles the upstream tracking case and gracefully falls back for branches without configured upstreams. The fallback to
falseon errors is appropriate for the deletion flow (fails open, allowing deletion when git state cannot be determined).apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsx (2)
30-58: Clever optimization with split queries.The approach of using an initial expensive git-status query with
staleTime: Number.POSITIVE_INFINITYand a separate cheap polling query for terminal count is a good pattern for balancing responsiveness with performance.One minor observation: when
gitStatusDataisundefined(still loading),canDeleteDatafalls back toterminalCountData, which hashasChanges: falseandhasUnpushedCommits: false. This means during the brief window before git status loads, the UI could show no warnings even if there are changes. However, sinceisLoadingGitStatusgates the UI with "Checking workspace status...", this is handled correctly.
116-125: LGTM!The warnings are clear, consistent with the existing warning styling, and correctly handle the case where both uncommitted changes and unpushed commits exist simultaneously.
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (4)
16-20: LGTM!The new git utility imports are correctly added and follow the existing import structure.
292-329: LGTM!The
skipGitChecksoption is a clean optimization for the polling use case. The early return path correctly skips expensive git operations while still returning terminal count, and the response shape remains consistent withhasChanges: falseandhasUnpushedCommits: falsedefaults.
358-372: Good use of parallel execution.Running both git checks concurrently with
Promise.allis efficient. One minor note: ifhasUncommittedChangesthrows (it lacks error handling unlikehasUnpushedCommits), the error will be caught at line 373 and returncanDelete: falsewith the error message, which is acceptable behavior.
385-394: Consistent response shape across all return paths.All branches of
canDeletenow returnhasChangesandhasUnpushedCommits, ensuring the frontend always receives a predictable response shape regardless of workspace state.
1 participant
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.