feat(desktop): delete local branch on workspace deletion#1255
Conversation
Add a checkbox to the worktree delete dialog that lets users also remove the associated local git branch. The preference is persisted in settings so it remembers the last-used choice. Branch deletion is best-effort and won't block workspace removal if it fails.
📝 WalkthroughWalkthroughAdds a persisted "delete local branch" setting, UI controls to toggle it, TRPC procedures to get/set the setting, an optional deleteLocalBranch flag on workspace deletion, a git utility to delete local branches, and a DB migration adding the setting column. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as DeleteWorkspaceDialog
participant Settings as Settings Router
participant DeleteProc as Delete Procedure
participant Git as Git Utilities
participant DB as Local Database
User->>UI: Open delete dialog
UI->>Settings: getDeleteLocalBranch()
Settings->>DB: SELECT settings (id:1)
DB-->>Settings: { deleteLocalBranch }
Settings-->>UI: return default
User->>UI: Toggle checkbox, click Delete
UI->>Settings: setDeleteLocalBranch({ enabled })
Settings->>DB: UPSERT settings (id:1)
DB-->>Settings: OK
UI->>DeleteProc: delete({ id, deleteLocalBranch })
DeleteProc->>DB: remove workspace & worktree records
DB-->>DeleteProc: OK
alt deleteLocalBranch == true and branch exists
DeleteProc->>Git: deleteLocalBranch(mainRepoPath, branch)
Git->>Git: run `git -C <path> branch -D <branch>`
Git-->>DeleteProc: success / error (logged)
end
DeleteProc-->>UI: { success: true }
UI-->>User: close dialog
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts`:
- Around line 636-661: In deleteLocalBranch, add a safety check to avoid
deleting the repository default (or current) branch: before running
execFileAsync in deleteLocalBranch({ mainRepoPath, branch }), call git to
determine the repo default branch (e.g., run git -C mainRepoPath symbolic-ref
--short refs/remotes/origin/HEAD or git -C mainRepoPath rev-parse --abbrev-ref
origin/HEAD) and also check the currently checked-out branch (git -C
mainRepoPath rev-parse --abbrev-ref HEAD); if branch equals the default branch
or the current branch, log and return early instead of running git branch -D;
keep using getGitEnv() and execFileAsync for these checks and preserve existing
error handling for other failures.
In `@packages/local-db/src/schema/schema.ts`:
- Line 158: The new schema added the deleteLocalBranch column
(delete_local_branch) in packages/local-db/src/schema/schema.ts but there is no
matching migration; create a migration file named
0018_add_delete_local_branch.sql in packages/local-db/drizzle/ containing a
single ALTER TABLE statement to add the integer column (`ALTER TABLE settings
ADD delete_local_branch integer;`) so the database schema and the
deleteLocalBranch field stay in sync; follow the existing migration naming/order
pattern used by files like 0015_add_notification_sounds_muted.sql and
0016_add_telemetry_enabled.sql.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/components/DeleteWorkspaceDialog/DeleteWorkspaceDialog.tsx (1)
45-49: Local checkbox state doesn't reset on dialog reopen.If the user toggles the checkbox, then cancels (without deleting), the local
deleteLocalBranchstate retains the toggled value across dialog open/close cycles (since the component stays mounted). On the next open, the checkbox will show the locally-toggled value rather than the persisted server default.Consider resetting the local state when the dialog opens:
Proposed fix
+import { useEffect, useState } from "react"; -import { useState } from "react"; ... const [deleteLocalBranch, setDeleteLocalBranch] = useState<boolean | null>( null, ); +useEffect(() => { + if (open) { + setDeleteLocalBranch(null); + } +}, [open]); const deleteLocalBranchChecked = deleteLocalBranch ?? deleteLocalBranchDefault ?? false;
| export async function deleteLocalBranch({ | ||
| mainRepoPath, | ||
| branch, | ||
| }: { | ||
| mainRepoPath: string; | ||
| branch: string; | ||
| }): Promise<void> { | ||
| const env = await getGitEnv(); | ||
|
|
||
| try { | ||
| await execFileAsync( | ||
| "git", | ||
| ["-C", mainRepoPath, "branch", "-D", branch], | ||
| { env, timeout: 10_000 }, | ||
| ); | ||
| console.log( | ||
| `[workspace/delete] Deleted local branch "${branch}"`, | ||
| ); | ||
| } catch (error) { | ||
| const errorMessage = error instanceof Error ? error.message : String(error); | ||
| console.error( | ||
| `[workspace/delete] Failed to delete local branch "${branch}": ${errorMessage}`, | ||
| ); | ||
| throw new Error(`Failed to delete local branch "${branch}": ${errorMessage}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider guarding against deletion of the default/current branch.
git branch -D will force-delete any branch, including main/master or the repo's default branch. If a user happens to create a worktree on the default branch and checks "Also delete local branch," this would delete it. While git would refuse to delete a currently checked-out branch, bare repos and edge cases could still be problematic.
Consider adding a safety check, either here or in the caller, to skip deletion when branch matches the project's default branch.
Proposed guard
export async function deleteLocalBranch({
mainRepoPath,
branch,
+ defaultBranch,
}: {
mainRepoPath: string;
branch: string;
+ defaultBranch?: string;
}): Promise<void> {
+ const protectedBranches = ["main", "master", "develop"];
+ if (defaultBranch) protectedBranches.push(defaultBranch);
+ if (protectedBranches.includes(branch)) {
+ console.warn(
+ `[workspace/delete] Refusing to delete protected branch "${branch}"`,
+ );
+ return;
+ }
const env = await getGitEnv();🤖 Prompt for AI Agents
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts` around lines 636 -
661, In deleteLocalBranch, add a safety check to avoid deleting the repository
default (or current) branch: before running execFileAsync in deleteLocalBranch({
mainRepoPath, branch }), call git to determine the repo default branch (e.g.,
run git -C mainRepoPath symbolic-ref --short refs/remotes/origin/HEAD or git -C
mainRepoPath rev-parse --abbrev-ref origin/HEAD) and also check the currently
checked-out branch (git -C mainRepoPath rev-parse --abbrev-ref HEAD); if branch
equals the default branch or the current branch, log and return early instead of
running git branch -D; keep using getGitEnv() and execFileAsync for these checks
and preserve existing error handling for other failures.
| notificationSoundsMuted: integer("notification_sounds_muted", { | ||
| mode: "boolean", | ||
| }), | ||
| deleteLocalBranch: integer("delete_local_branch", { mode: "boolean" }), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat packages/local-db/src/schema/schema.ts | head -200 | tail -50Repository: superset-sh/superset
Length of output: 1666
🏁 Script executed:
# Look for migration files related to this change
find . -type f \( -name "*migration*" -o -name "*migrate*" \) | grep -E "\.(ts|js|sql)$" | head -20Repository: superset-sh/superset
Length of output: 112
🏁 Script executed:
# Search for references to delete_local_branch in the codebase
rg "delete_local_branch" --type ts --type js --type sql -lRepository: superset-sh/superset
Length of output: 102
🏁 Script executed:
# Check the migration file to see if it includes delete_local_branch
cat packages/db/drizzle/0000_initial_migration.sql | grep -A 5 -B 5 "delete_local_branch"Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Also check if there are any other drizzle migration files
find packages -name "*.sql" -o -name "*migration*.ts" | sortRepository: superset-sh/superset
Length of output: 2132
🏁 Script executed:
# Verify the full settings table schema definition
rg "export const settings = sqliteTable" -A 40 packages/local-db/src/schema/schema.tsRepository: superset-sh/superset
Length of output: 1745
🏁 Script executed:
# Search for the migration file that adds delete_local_branch to local-db
rg "delete_local_branch" packages/local-db/drizzle/ -A 2 -B 2Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check the most recent local-db migration files
tail -5 packages/local-db/drizzle/*.sql | catRepository: superset-sh/superset
Length of output: 105
🏁 Script executed:
# List all local-db migration files with their content summary
for file in packages/local-db/drizzle/*.sql; do
echo "=== $(basename $file) ==="
head -3 "$file"
done | tail -40Repository: superset-sh/superset
Length of output: 2628
Migration file missing for new delete_local_branch column.
The schema definition exists in schema.ts, but there is no corresponding migration file in packages/local-db/drizzle/. Following the pattern of recent migrations (e.g., 0015_add_notification_sounds_muted.sql, 0016_add_telemetry_enabled.sql), create migration file 0018_add_delete_local_branch.sql with:
ALTER TABLE `settings` ADD `delete_local_branch` integer;Without this migration, the schema and database will be out of sync.
🤖 Prompt for AI Agents
In `@packages/local-db/src/schema/schema.ts` at line 158, The new schema added the
deleteLocalBranch column (delete_local_branch) in
packages/local-db/src/schema/schema.ts but there is no matching migration;
create a migration file named 0018_add_delete_local_branch.sql in
packages/local-db/drizzle/ containing a single ALTER TABLE statement to add the
integer column (`ALTER TABLE settings ADD delete_local_branch integer;`) so the
database schema and the deleteLocalBranch field stay in sync; follow the
existing migration naming/order pattern used by files like
0015_add_notification_sounds_muted.sql and 0016_add_telemetry_enabled.sql.
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
Add a switch in Features settings to control the default for deleting local branches on workspace removal. Also generate drizzle migration 0018 for the new delete_local_branch column.
Use id/htmlFor association instead of wrapping Checkbox in label to avoid Radix ref conflict that caused maximum update depth error.
Radix Checkbox causes maximum update depth error inside AlertDialog. Switch component works without ref conflicts.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/behavior/components/BehaviorSettings/BehaviorSettings.tsx`:
- Around line 96-98: The getTelemetryEnabled query in the BehaviorSettings
component (electronTrpc.settings.getTelemetryEnabled.useQuery) runs
unconditionally and will break after migration drops telemetry_enabled; disable
it by adding query options { enabled: false } (or remove the telemetry-related
lines entirely). Locate the useQuery call and change it to
electronTrpc.settings.getTelemetryEnabled.useQuery(undefined, { enabled: false
}) so the hook does not execute, and remove or keep the TODO comment about
removing telemetry once migrations are complete; also remove any code that reads
telemetryEnabled/isTelemetryLoading if you choose to delete telemetry support.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/components/DeleteWorkspaceDialog/DeleteWorkspaceDialog.tsx`:
- Around line 45-49: The local toggle state deleteLocalBranch in
DeleteWorkspaceDialog is only initialized once and isn't reset when the dialog
reopens, so it can incorrectly override deleteLocalBranchDefault; update the
component to reset deleteLocalBranch to null whenever the dialog is opened
(i.e., when the open prop transitions true) — implement this by adding an effect
keyed on open (or handling open changes via onOpenChange) that sets
setDeleteLocalBranch(null) on open so the deleteLocalBranchChecked expression
(deleteLocalBranch ?? deleteLocalBranchDefault ?? false) will honor the server
default on each open.
- Around line 219-230: The "Also delete local branch" span in
DeleteWorkspaceDialog isn't associated with the Switch, so the label text
doesn't toggle the switch; update the JSX to replace or wrap the span with a
Label element that uses htmlFor matching an id prop on the Switch (e.g., give
the Switch an id like "delete-local-branch" and change the <span> to <Label
htmlFor="delete-local-branch">) and keep the same props wired to
deleteLocalBranchChecked and setDeleteLocalBranch so clicking the text toggles
the Switch.
| // TODO: remove telemetry query/mutation/handler once telemetry procedures are removed | ||
| const { data: telemetryEnabled, isLoading: isTelemetryLoading } = | ||
| electronTrpc.settings.getTelemetryEnabled.useQuery(); |
There was a problem hiding this comment.
getTelemetryEnabled query fires unconditionally despite the column being dropped in this PR's migration.
This useQuery executes on every render of BehaviorSettings regardless of the false && gate on line 281. After migration 0018 drops telemetry_enabled, this will likely produce runtime errors. Consider wrapping with enabled: false or removing the telemetry code entirely.
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/behavior/components/BehaviorSettings/BehaviorSettings.tsx`
around lines 96 - 98, The getTelemetryEnabled query in the BehaviorSettings
component (electronTrpc.settings.getTelemetryEnabled.useQuery) runs
unconditionally and will break after migration drops telemetry_enabled; disable
it by adding query options { enabled: false } (or remove the telemetry-related
lines entirely). Locate the useQuery call and change it to
electronTrpc.settings.getTelemetryEnabled.useQuery(undefined, { enabled: false
}) so the hook does not execute, and remove or keep the TODO comment about
removing telemetry once migrations are complete; also remove any code that reads
telemetryEnabled/isTelemetryLoading if you choose to delete telemetry support.
| const [deleteLocalBranch, setDeleteLocalBranch] = useState<boolean | null>( | ||
| null, | ||
| ); | ||
| const deleteLocalBranchChecked = | ||
| deleteLocalBranch ?? deleteLocalBranchDefault ?? false; |
There was a problem hiding this comment.
Local switch state is not reset when the dialog reopens.
deleteLocalBranch is initialized to null only on mount. Since DeleteWorkspaceDialog stays mounted in its parent WorkspaceListItem, re-opening the dialog (Cancel → reopen) keeps the previous local toggle value, which takes precedence over the server default via the ?? chain on line 48–49. This means a user who toggles the switch and cancels will see their unsaved toggle state next time, which is inconsistent with the "persisted preference" model.
Consider resetting local state when the dialog opens:
Proposed fix
Add a reset when the dialog opens, e.g. by using onOpenChange or an effect keyed on open:
// At top of component, after the useState:
const prevOpen = useRef(false);
if (open && !prevOpen.current) {
setDeleteLocalBranch(null); // reset to server default on open
}
prevOpen.current = open;Or, if acceptable, simply pass a key prop to force remount when open changes (though this re-fetches queries too).
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/components/DeleteWorkspaceDialog/DeleteWorkspaceDialog.tsx`
around lines 45 - 49, The local toggle state deleteLocalBranch in
DeleteWorkspaceDialog is only initialized once and isn't reset when the dialog
reopens, so it can incorrectly override deleteLocalBranchDefault; update the
component to reset deleteLocalBranch to null whenever the dialog is opened
(i.e., when the open prop transitions true) — implement this by adding an effect
keyed on open (or handling open changes via onOpenChange) that sets
setDeleteLocalBranch(null) on open so the deleteLocalBranchChecked expression
(deleteLocalBranch ?? deleteLocalBranchDefault ?? false) will honor the server
default on each open.
| {!isLoading && canDelete && ( | ||
| <div className="flex items-center justify-between px-4 pb-2"> | ||
| <span className="text-xs text-muted-foreground"> | ||
| Also delete local branch | ||
| </span> | ||
| <Switch | ||
| checked={deleteLocalBranchChecked} | ||
| onCheckedChange={(checked) => setDeleteLocalBranch(checked)} | ||
| className="scale-75" | ||
| /> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Missing label association on the "Also delete local branch" switch.
The <span> text label isn't linked to the Switch (no id/htmlFor pairing), so clicking the text doesn't toggle the switch. Other settings toggles in this PR (e.g., BehaviorSettings.tsx line 208–220) use <Label htmlFor="..."> with a matching id on the Switch.
Proposed fix
<div className="flex items-center justify-between px-4 pb-2">
- <span className="text-xs text-muted-foreground">
+ <label htmlFor="delete-local-branch-switch" className="text-xs text-muted-foreground">
Also delete local branch
- </span>
+ </label>
<Switch
+ id="delete-local-branch-switch"
checked={deleteLocalBranchChecked}
onCheckedChange={(checked) => setDeleteLocalBranch(checked)}
className="scale-75"
/>
</div>📝 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.
| {!isLoading && canDelete && ( | |
| <div className="flex items-center justify-between px-4 pb-2"> | |
| <span className="text-xs text-muted-foreground"> | |
| Also delete local branch | |
| </span> | |
| <Switch | |
| checked={deleteLocalBranchChecked} | |
| onCheckedChange={(checked) => setDeleteLocalBranch(checked)} | |
| className="scale-75" | |
| /> | |
| </div> | |
| )} | |
| {!isLoading && canDelete && ( | |
| <div className="flex items-center justify-between px-4 pb-2"> | |
| <label htmlFor="delete-local-branch-switch" className="text-xs text-muted-foreground"> | |
| Also delete local branch | |
| </label> | |
| <Switch | |
| id="delete-local-branch-switch" | |
| checked={deleteLocalBranchChecked} | |
| onCheckedChange={(checked) => setDeleteLocalBranch(checked)} | |
| className="scale-75" | |
| /> | |
| </div> | |
| )} |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/components/DeleteWorkspaceDialog/DeleteWorkspaceDialog.tsx`
around lines 219 - 230, The "Also delete local branch" span in
DeleteWorkspaceDialog isn't associated with the Switch, so the label text
doesn't toggle the switch; update the JSX to replace or wrap the span with a
Label element that uses htmlFor matching an id prop on the Switch (e.g., give
the Switch an id like "delete-local-branch" and change the <span> to <Label
htmlFor="delete-local-branch">) and keep the same props wired to
deleteLocalBranchChecked and setDeleteLocalBranch so clicking the text toggles
the Switch.
1 participant
Summary
Test plan
git branchno longer lists the branchSummary by CodeRabbit
New Features
Chores