From f1e5640a269a392e8566befab6153486746035ea Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Wed, 13 May 2026 23:27:49 +0800 Subject: [PATCH] Confirm risky worktree archive (#994) --- docs/design.md | 2 +- .../src/components/sidebar-workspace-list.tsx | 142 +++++++++--------- packages/app/src/git/use-actions.ts | 49 ++++-- .../src/git/worktree-archive-warning.test.ts | 60 ++++++++ .../app/src/git/worktree-archive-warning.ts | 79 ++++++++++ .../src/hooks/use-sidebar-workspaces-list.ts | 6 + .../utils/sidebar-project-row-model.test.ts | 2 + .../app/src/utils/sidebar-shortcuts.test.ts | 2 + 8 files changed, 261 insertions(+), 81 deletions(-) create mode 100644 packages/app/src/git/worktree-archive-warning.test.ts create mode 100644 packages/app/src/git/worktree-archive-warning.ts diff --git a/docs/design.md b/docs/design.md index 0c786b1c1..6fe71191d 100644 --- a/docs/design.md +++ b/docs/design.md @@ -216,7 +216,7 @@ The bespoke pills in `packages/app/src/screens/settings/host-page.tsx:97-116`, ` - Raw DOM APIs without an `isWeb` guard. - Spacing values outside the scale. `padding: 20` and `gap: 10` are wrong. - Color changes for disabled state. Opacity only. -- Destructive actions without `confirmDialog`. Restart, remove, archive, and any future destructive action are confirmed. +- Destructive actions without `confirmDialog`. Restart, remove, and future destructive actions are confirmed. Worktree archive is confirmed only when git runtime reports uncommitted changes or unpushed commits; clean pushed worktrees archive immediately. - Bespoke status pills. `` is the pill primitive. - Raw `Modal` for a focused task. `` is the modal primitive. - Importing `ActivityIndicator` directly. `` is the loading primitive. diff --git a/packages/app/src/components/sidebar-workspace-list.tsx b/packages/app/src/components/sidebar-workspace-list.tsx index 83f82a006..9bbe408c2 100644 --- a/packages/app/src/components/sidebar-workspace-list.tsx +++ b/packages/app/src/components/sidebar-workspace-list.tsx @@ -106,6 +106,7 @@ import { requireWorkspaceExecutionDirectory, resolveWorkspaceExecutionDirectory, } from "@/utils/workspace-execution"; +import { confirmRiskyWorktreeArchive } from "@/git/worktree-archive-warning"; import { archiveWorkspaceOptimistically, archiveWorkspacesOptimistically, @@ -1474,92 +1475,93 @@ function WorkspaceRowWithMenu({ }); }, [activeWorkspaceSelection, workspace.serverId, workspace.workspaceId]); - const handleArchiveWorktree = useCallback(() => { + const archiveWorktreeAfterConfirmation = useCallback(async () => { if (isArchiving) { return; } - void (async () => { - const confirmed = await confirmDialog({ - title: "Archive worktree?", - message: `Archive "${workspace.name}"?\n\nThe worktree will be removed from disk, terminals will be stopped, and agents inside will be archived.\n\nYour branch is still accessible if you committed.`, - confirmLabel: "Archive", - cancelLabel: "Cancel", - destructive: true, + const confirmed = await confirmRiskyWorktreeArchive({ + worktreeName: workspace.name, + isDirty: workspace.archiveHasUncommittedChanges, + aheadOfOrigin: workspace.archiveUnpushedCommitCount, + diffStat: workspace.diffStat, + }); + + if (!confirmed) { + return; + } + let archiveDirectory: string; + try { + archiveDirectory = requireWorkspaceExecutionDirectory({ + workspaceId: workspace.workspaceId, + workspaceDirectory: workspace.workspaceDirectory, }); + } catch (error) { + toast.error(error instanceof Error ? error.message : "Workspace path not available"); + return; + } - if (!confirmed) { - return; - } - let archiveDirectory: string; - try { - archiveDirectory = requireWorkspaceExecutionDirectory({ - workspaceId: workspace.workspaceId, - workspaceDirectory: workspace.workspaceDirectory, - }); - } catch (error) { - toast.error(error instanceof Error ? error.message : "Workspace path not available"); - return; - } + if (!archiveDirectory) { + toast.error("Workspace path not available"); + return; + } - if (!archiveDirectory) { - toast.error("Workspace path not available"); - return; - } + redirectAfterArchive(); - redirectAfterArchive(); - - void archiveWorktree({ - serverId: workspace.serverId, - cwd: archiveDirectory, - worktreePath: archiveDirectory, - }).catch((error) => { - const message = error instanceof Error ? error.message : "Failed to archive worktree"; - toast.error(message); - }); - })(); + void archiveWorktree({ + serverId: workspace.serverId, + cwd: archiveDirectory, + worktreePath: archiveDirectory, + }).catch((error) => { + const message = error instanceof Error ? error.message : "Failed to archive worktree"; + toast.error(message); + }); }, [archiveWorktree, isArchiving, redirectAfterArchive, toast, workspace]); - const handleArchiveWorkspace = useCallback(() => { + const handleArchiveWorktree = useCallback(() => { + void archiveWorktreeAfterConfirmation(); + }, [archiveWorktreeAfterConfirmation]); + + const hideWorkspaceAfterConfirmation = useCallback(async () => { if (isArchivingWorkspace) { return; } - void (async () => { - const confirmed = await confirmDialog({ - title: "Hide workspace?", - message: `Hide "${workspace.name}" from the sidebar?\n\nFiles on disk will not be changed.`, - confirmLabel: "Hide", - cancelLabel: "Cancel", - destructive: true, + const confirmed = await confirmDialog({ + title: "Hide workspace?", + message: `Hide "${workspace.name}" from the sidebar?\n\nFiles on disk will not be changed.`, + confirmLabel: "Hide", + cancelLabel: "Cancel", + destructive: true, + }); + if (!confirmed) { + return; + } + + const client = getHostRuntimeStore().getClient(workspace.serverId); + if (!client) { + toast.error("Host is not connected"); + return; + } + + setIsArchivingWorkspace(true); + try { + await archiveWorkspaceOptimistically({ + client, + workspace, + afterHide: redirectAfterArchive, }); - if (!confirmed) { - return; - } - - const client = getHostRuntimeStore().getClient(workspace.serverId); - if (!client) { - toast.error("Host is not connected"); - return; - } - - setIsArchivingWorkspace(true); - void (async () => { - try { - await archiveWorkspaceOptimistically({ - client, - workspace, - afterHide: redirectAfterArchive, - }); - } catch (error) { - toast.error(error instanceof Error ? error.message : "Failed to hide workspace"); - } finally { - setIsArchivingWorkspace(false); - } - })(); - })(); + } catch (error) { + toast.error(error instanceof Error ? error.message : "Failed to hide workspace"); + } finally { + setIsArchivingWorkspace(false); + } }, [isArchivingWorkspace, redirectAfterArchive, toast, workspace]); + const handleArchiveWorkspace = useCallback(() => { + void hideWorkspaceAfterConfirmation(); + }, [hideWorkspaceAfterConfirmation]); + const handleCopyPath = useCallback(() => { let copyTargetDirectory: string; try { @@ -1589,7 +1591,7 @@ function WorkspaceRowWithMenu({ priority: 0, handle: () => { if (isWorktree) { - handleArchiveWorktree(); + void archiveWorktreeAfterConfirmation(); } else { handleArchiveWorkspace(); } diff --git a/packages/app/src/git/use-actions.ts b/packages/app/src/git/use-actions.ts index d72117433..8fe31b3d3 100644 --- a/packages/app/src/git/use-actions.ts +++ b/packages/app/src/git/use-actions.ts @@ -11,6 +11,7 @@ import { useToast } from "@/contexts/toast-context"; import { useSessionStore } from "@/stores/session-store"; import { resolveWorkspaceIdByExecutionDirectory } from "@/utils/workspace-execution"; import { buildWorkspaceArchiveRedirectRoute } from "@/utils/workspace-archive-navigation"; +import { confirmRiskyWorktreeArchive } from "@/git/worktree-archive-warning"; export type { GitActionId, GitAction, GitActions } from "@/git/policy"; @@ -169,6 +170,11 @@ export function useGitActions({ serverId, cwd, icons }: UseGitActionsInput): Use cwd, enabled: isGit, }); + const baseRefLabel = useMemo(() => formatBaseRefLabel(baseRef), [baseRef]); + const branchLabel = resolveBranchLabel({ + currentBranch: gitStatus?.currentBranch, + notGit, + }); // Ship default persistence const shipDefaultStorageKey = useMemo(() => { @@ -389,31 +395,59 @@ export function useGitActions({ serverId, cwd, icons }: UseGitActionsInput): Use }); }, [baseRef, cwd, runMergeFromBase, serverId, toast, toastActionError, toastActionSuccess]); - const handleArchiveWorktree = useCallback(() => { + const archiveWorktreeAfterConfirmation = useCallback(async () => { const worktreePath = status?.cwd; if (!worktreePath) { toast.error("Worktree path unavailable"); return; } + const workspaces = useSessionStore.getState().sessions[serverId]?.workspaces; + const workspaceList = Array.from(workspaces?.values() ?? []); + const workspace = workspaceList.find( + (candidate) => candidate.workspaceDirectory === worktreePath, + ); + const confirmed = await confirmRiskyWorktreeArchive({ + worktreeName: workspace?.name ?? branchLabel, + isDirty: gitStatus?.isDirty, + aheadOfOrigin: gitStatus?.aheadOfOrigin, + diffStat: workspace?.diffStat ?? null, + }); + if (!confirmed) { + return; + } + const archivedWorkspaceId = resolveWorkspaceIdByExecutionDirectory({ - workspaces: workspaces?.values(), + workspaces: workspaceList, workspaceDirectory: worktreePath, }) ?? worktreePath; router.replace( buildWorkspaceArchiveRedirectRoute({ serverId, archivedWorkspaceId, - workspaces: workspaces?.values() ?? [], + workspaces: workspaceList, }) as Href, ); void runArchiveWorktree({ serverId, cwd, worktreePath }).catch((err) => { toastActionError(err, "Failed to archive worktree"); }); - }, [cwd, runArchiveWorktree, serverId, status, toast, toastActionError]); + }, [ + branchLabel, + cwd, + gitStatus?.aheadOfOrigin, + gitStatus?.isDirty, + runArchiveWorktree, + serverId, + status?.cwd, + toast, + toastActionError, + ]); + + const handleArchiveWorktree = useCallback(() => { + void archiveWorktreeAfterConfirmation(); + }, [archiveWorktreeAfterConfirmation]); - const baseRefLabel = useMemo(() => formatBaseRefLabel(baseRef), [baseRef]); const derived = deriveGitActionsState({ isGit, status, @@ -437,11 +471,6 @@ export function useGitActions({ serverId, cwd, icons }: UseGitActionsInput): Use shouldPromoteArchive, } = derived; - const branchLabel = resolveBranchLabel({ - currentBranch: gitStatus?.currentBranch, - notGit, - }); - const handlePrAction = useCallback(() => { if (prStatus?.url) { openURLInNewTab(prStatus.url); diff --git a/packages/app/src/git/worktree-archive-warning.test.ts b/packages/app/src/git/worktree-archive-warning.test.ts new file mode 100644 index 000000000..2d9e6c2f5 --- /dev/null +++ b/packages/app/src/git/worktree-archive-warning.test.ts @@ -0,0 +1,60 @@ +import { describe, expect, it } from "vitest"; + +import { + buildWorktreeArchiveConfirmationMessage, + buildWorktreeArchiveRiskReasons, +} from "@/git/worktree-archive-warning"; + +describe("worktree archive warning", () => { + it("does not require a confirmation for clean and pushed worktrees", () => { + expect( + buildWorktreeArchiveConfirmationMessage({ + worktreeName: "feature", + isDirty: false, + aheadOfOrigin: 0, + diffStat: null, + }), + ).toBeNull(); + }); + + it("explains uncommitted line changes", () => { + expect( + buildWorktreeArchiveRiskReasons({ + isDirty: true, + aheadOfOrigin: 0, + diffStat: { additions: 12, deletions: 1 }, + }), + ).toEqual(["Uncommitted changes (12 added lines, 1 deleted line)"]); + }); + + it("treats nonzero diff stats as dirty when dirty state is missing", () => { + expect( + buildWorktreeArchiveRiskReasons({ + isDirty: undefined, + aheadOfOrigin: 0, + diffStat: { additions: 4, deletions: 0 }, + }), + ).toEqual(["Uncommitted changes (4 added lines)"]); + }); + + it("explains unpushed commits", () => { + expect( + buildWorktreeArchiveRiskReasons({ + isDirty: false, + aheadOfOrigin: 2, + diffStat: null, + }), + ).toEqual(["2 unpushed commits"]); + }); + + it("includes every archive risk in the confirmation copy", () => { + expect( + buildWorktreeArchiveConfirmationMessage({ + worktreeName: "risky-feature", + isDirty: true, + aheadOfOrigin: 1, + diffStat: { additions: 1, deletions: 3 }, + }), + ).toBe("Uncommitted changes (1 added line, 3 deleted lines)\n1 unpushed commit"); + }); +}); diff --git a/packages/app/src/git/worktree-archive-warning.ts b/packages/app/src/git/worktree-archive-warning.ts new file mode 100644 index 000000000..280c19848 --- /dev/null +++ b/packages/app/src/git/worktree-archive-warning.ts @@ -0,0 +1,79 @@ +import { confirmDialog } from "@/utils/confirm-dialog"; + +export interface WorktreeArchiveRisk { + isDirty?: boolean | null; + aheadOfOrigin?: number | null; + diffStat?: { additions: number; deletions: number } | null; +} + +export interface WorktreeArchiveConfirmationInput extends WorktreeArchiveRisk { + worktreeName: string; +} + +function pluralize(count: number, singular: string, plural = `${singular}s`): string { + return count === 1 ? singular : plural; +} + +function formatDiffStat(diffStat: WorktreeArchiveRisk["diffStat"]): string | null { + if (!diffStat) { + return null; + } + + const parts: string[] = []; + if (diffStat.additions > 0) { + parts.push(`${diffStat.additions} added ${pluralize(diffStat.additions, "line")}`); + } + if (diffStat.deletions > 0) { + parts.push(`${diffStat.deletions} deleted ${pluralize(diffStat.deletions, "line")}`); + } + + return parts.length > 0 ? parts.join(", ") : null; +} + +export function buildWorktreeArchiveRiskReasons(input: WorktreeArchiveRisk): string[] { + const reasons: string[] = []; + const diffStat = input.diffStat; + const hasDiffStatChanges = diffStat ? diffStat.additions > 0 || diffStat.deletions > 0 : false; + const hasUncommittedChanges = + input.isDirty === true || (input.isDirty == null && hasDiffStatChanges); + + if (hasUncommittedChanges) { + const diffStatLabel = formatDiffStat(diffStat); + reasons.push(diffStatLabel ? `Uncommitted changes (${diffStatLabel})` : "Uncommitted changes"); + } + + if ((input.aheadOfOrigin ?? 0) > 0) { + const aheadOfOrigin = input.aheadOfOrigin ?? 0; + reasons.push(`${aheadOfOrigin} unpushed ${pluralize(aheadOfOrigin, "commit")}`); + } + + return reasons; +} + +export function buildWorktreeArchiveConfirmationMessage( + input: WorktreeArchiveConfirmationInput, +): string | null { + const reasons = buildWorktreeArchiveRiskReasons(input); + if (reasons.length === 0) { + return null; + } + + return reasons.join("\n"); +} + +export async function confirmRiskyWorktreeArchive( + input: WorktreeArchiveConfirmationInput, +): Promise { + const message = buildWorktreeArchiveConfirmationMessage(input); + if (!message) { + return true; + } + + return await confirmDialog({ + title: `Archive "${input.worktreeName}"?`, + message, + confirmLabel: "Archive", + cancelLabel: "Cancel", + destructive: true, + }); +} diff --git a/packages/app/src/hooks/use-sidebar-workspaces-list.ts b/packages/app/src/hooks/use-sidebar-workspaces-list.ts index feffcdda8..963b4041c 100644 --- a/packages/app/src/hooks/use-sidebar-workspaces-list.ts +++ b/packages/app/src/hooks/use-sidebar-workspaces-list.ts @@ -31,6 +31,8 @@ export interface SidebarWorkspaceEntry { statusBucket: SidebarStateBucket; archivingAt: string | null; diffStat: { additions: number; deletions: number } | null; + archiveHasUncommittedChanges: boolean | null; + archiveUnpushedCommitCount: number | null; scripts: WorkspaceDescriptor["scripts"]; hasRunningScripts: boolean; } @@ -69,6 +71,8 @@ function createStructuralWorkspaceEntry(input: { statusBucket: "done", archivingAt: null, diffStat: null, + archiveHasUncommittedChanges: null, + archiveUnpushedCommitCount: null, scripts: [], hasRunningScripts: false, }; @@ -91,6 +95,8 @@ export function createSidebarWorkspaceEntry(input: { statusBucket: input.workspace.status, archivingAt: input.workspace.archivingAt, diffStat: input.workspace.diffStat, + archiveHasUncommittedChanges: input.workspace.gitRuntime?.isDirty ?? null, + archiveUnpushedCommitCount: input.workspace.gitRuntime?.aheadOfOrigin ?? null, scripts: input.workspace.scripts, hasRunningScripts: input.workspace.scripts.some((script) => script.lifecycle === "running"), }; diff --git a/packages/app/src/utils/sidebar-project-row-model.test.ts b/packages/app/src/utils/sidebar-project-row-model.test.ts index 9972b4085..461357058 100644 --- a/packages/app/src/utils/sidebar-project-row-model.test.ts +++ b/packages/app/src/utils/sidebar-project-row-model.test.ts @@ -20,6 +20,8 @@ function workspace(overrides: Partial = {}): SidebarWorks name: "paseo", statusBucket: "done", diffStat: null, + archiveHasUncommittedChanges: null, + archiveUnpushedCommitCount: null, scripts: [], hasRunningScripts: false, ...overrides, diff --git a/packages/app/src/utils/sidebar-shortcuts.test.ts b/packages/app/src/utils/sidebar-shortcuts.test.ts index 2cd200d64..b9e3c0ec4 100644 --- a/packages/app/src/utils/sidebar-shortcuts.test.ts +++ b/packages/app/src/utils/sidebar-shortcuts.test.ts @@ -25,6 +25,8 @@ function workspace(input: { statusBucket: "done", archivingAt: null, diffStat: null, + archiveHasUncommittedChanges: null, + archiveUnpushedCommitCount: null, scripts: [], hasRunningScripts: false, };