-
Notifications
You must be signed in to change notification settings - Fork 963
feat(desktop): delete local branch on workspace deletion #1255
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.
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
Changes from all commits
c8b76d2
fa24d57
67ca562
8dd7099
220d180
95f60ff
b451ecb
ca01b46
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,10 @@ export function BehaviorSettings({ visibleItems }: BehaviorSettingsProps) { | |
| SETTING_ITEM_ID.BEHAVIOR_CONFIRM_QUIT, | ||
| visibleItems, | ||
| ); | ||
| const showDeleteLocalBranch = isItemVisible( | ||
| SETTING_ITEM_ID.BEHAVIOR_DELETE_LOCAL_BRANCH, | ||
| visibleItems, | ||
| ); | ||
| const showBranchPrefix = isItemVisible( | ||
| SETTING_ITEM_ID.BEHAVIOR_BRANCH_PREFIX, | ||
| visibleItems, | ||
|
|
@@ -62,6 +66,33 @@ export function BehaviorSettings({ visibleItems }: BehaviorSettingsProps) { | |
| setConfirmOnQuit.mutate({ enabled }); | ||
| }; | ||
|
|
||
| const { data: deleteLocalBranch, isLoading: isDeleteBranchLoading } = | ||
| electronTrpc.settings.getDeleteLocalBranch.useQuery(); | ||
| const setDeleteLocalBranch = | ||
| electronTrpc.settings.setDeleteLocalBranch.useMutation({ | ||
| onMutate: async ({ enabled }) => { | ||
| await utils.settings.getDeleteLocalBranch.cancel(); | ||
| const previous = utils.settings.getDeleteLocalBranch.getData(); | ||
| utils.settings.getDeleteLocalBranch.setData(undefined, enabled); | ||
| return { previous }; | ||
| }, | ||
| onError: (_err, _vars, context) => { | ||
| if (context?.previous !== undefined) { | ||
| utils.settings.getDeleteLocalBranch.setData( | ||
| undefined, | ||
| context.previous, | ||
| ); | ||
| } | ||
| }, | ||
| onSettled: () => { | ||
| utils.settings.getDeleteLocalBranch.invalidate(); | ||
| }, | ||
| }); | ||
|
|
||
| const handleDeleteBranchToggle = (enabled: boolean) => { | ||
| setDeleteLocalBranch.mutate({ enabled }); | ||
| }; | ||
|
|
||
| // TODO: remove telemetry query/mutation/handler once telemetry procedures are removed | ||
| const { data: telemetryEnabled, isLoading: isTelemetryLoading } = | ||
| electronTrpc.settings.getTelemetryEnabled.useQuery(); | ||
|
Comment on lines
96
to
98
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This 🤖 Prompt for AI Agents |
||
|
|
@@ -171,6 +202,29 @@ export function BehaviorSettings({ visibleItems }: BehaviorSettingsProps) { | |
| </div> | ||
| )} | ||
|
|
||
| {showDeleteLocalBranch && ( | ||
| <div className="flex items-center justify-between"> | ||
| <div className="space-y-0.5"> | ||
| <Label | ||
| htmlFor="delete-local-branch" | ||
| className="text-sm font-medium" | ||
| > | ||
| Delete local branch on workspace removal | ||
| </Label> | ||
| <p className="text-xs text-muted-foreground"> | ||
| Also delete the local git branch when deleting a worktree | ||
| workspace | ||
| </p> | ||
| </div> | ||
| <Switch | ||
| id="delete-local-branch" | ||
| checked={deleteLocalBranch ?? false} | ||
| onCheckedChange={handleDeleteBranchToggle} | ||
| disabled={isDeleteBranchLoading || setDeleteLocalBranch.isPending} | ||
| /> | ||
| </div> | ||
| )} | ||
|
|
||
| {showBranchPrefix && ( | ||
| <div className="flex items-center justify-between"> | ||
| <div className="space-y-0.5"> | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -9,6 +9,7 @@ import { | |||||||||||||||||||||||||||||||||||||||||||||||||||
| import { Button } from "@superset/ui/button"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { toast } from "@superset/ui/sonner"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { Tooltip, TooltipContent, TooltipTrigger } from "@superset/ui/tooltip"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { useState } from "react"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { electronTrpc } from "renderer/lib/electron-trpc"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| useCloseWorkspace, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -33,6 +34,18 @@ export function DeleteWorkspaceDialog({ | |||||||||||||||||||||||||||||||||||||||||||||||||||
| const isBranch = workspaceType === "branch"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| const deleteWorkspace = useDeleteWorkspace(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| const closeWorkspace = useCloseWorkspace(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| const setDeleteLocalBranchSetting = | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| electronTrpc.settings.setDeleteLocalBranch.useMutation(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| const { data: deleteLocalBranchDefault } = | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| electronTrpc.settings.getDeleteLocalBranch.useQuery(undefined, { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| enabled: open && !isBranch, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| const [deleteLocalBranch, setDeleteLocalBranch] = useState<boolean | null>( | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| null, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| const deleteLocalBranchChecked = | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| deleteLocalBranch ?? deleteLocalBranchDefault ?? false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+44
to
+48
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Local switch state is not reset when the dialog reopens.
Consider resetting local state when the dialog opens: Proposed fixAdd a reset when the dialog opens, e.g. by using // 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 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| const { data: gitStatusData, isLoading: isLoadingGitStatus } = | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| electronTrpc.workspaces.canDelete.useQuery( | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -85,13 +98,22 @@ export function DeleteWorkspaceDialog({ | |||||||||||||||||||||||||||||||||||||||||||||||||||
| const handleDelete = () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| onOpenChange(false); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| setDeleteLocalBranchSetting.mutate({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| enabled: deleteLocalBranchChecked, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| toast.promise( | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| deleteWorkspace.mutateAsync({ id: workspaceId }).then((result) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!result.success) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new Error(result.error ?? "Failed to delete"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| return result; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| deleteWorkspace | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| .mutateAsync({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| id: workspaceId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| deleteLocalBranch: deleteLocalBranchChecked, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| .then((result) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!result.success) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new Error(result.error ?? "Failed to delete"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| return result; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| loading: `Deleting "${workspaceName}"...`, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| success: (result) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -183,7 +205,7 @@ export function DeleteWorkspaceDialog({ | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| {!isLoading && canDelete && hasWarnings && ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| <div className="px-4 pb-2"> | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| <div className="text-sm text-amber-700 dark:text-amber-300 bg-amber-100 dark:bg-amber-950/50 border border-amber-200 dark:border-amber-800 rounded px-2 py-1.5"> | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| <div className="text-xs text-yellow-700 dark:text-yellow-400 bg-yellow-50 dark:bg-yellow-500/10 border border-yellow-200 dark:border-yellow-500/20 rounded-md px-2.5 py-1.5"> | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| {hasChanges && hasUnpushedCommits | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| ? "Has uncommitted changes and unpushed commits" | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| : hasChanges | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -193,6 +215,19 @@ export function DeleteWorkspaceDialog({ | |||||||||||||||||||||||||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| )} | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| {!isLoading && canDelete && ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| <div className="px-4 pb-2"> | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| <label className="flex items-center gap-2 text-xs text-muted-foreground cursor-pointer select-none"> | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| <input | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| type="checkbox" | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| checked={deleteLocalBranchChecked} | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| onChange={(e) => setDeleteLocalBranch(e.target.checked)} | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| /> | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| Also delete local branch | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| </label> | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| )} | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+218
to
+229
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing label association on the "Also delete local branch" switch. The 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| <AlertDialogFooter className="px-4 pb-4 pt-2 flex-row justify-end gap-2"> | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| <Button | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| variant="ghost" | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| ALTER TABLE `settings` ADD `delete_local_branch` integer;--> statement-breakpoint | ||
| ALTER TABLE `settings` DROP COLUMN `telemetry_enabled`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider guarding against deletion of the default/current branch.
git branch -Dwill force-delete any branch, includingmain/masteror 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
branchmatches 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