From 73f7767eddce1d359c4f2cf342bd9a5a8b6013b9 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Wed, 19 Aug 2026 01:33:29 -0700 Subject: [PATCH] feat(sidebar): name the host when a delete batch collides on one id (#15423) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(sidebar): name the host when a delete batch collides on one id Two hosts can publish the same worktreeId, so a batch confirmation showed two rows with identical names and paths and nothing to tell them apart. Label each row with its execution host, but only when the batch actually contains a same-id collision — an unconditional chip is noise. * fix(sidebar): qualify colliding delete targets by saved host * fix(sidebar): preserve delete host collision scope --- .../sidebar/DeleteWorktreeDialog.test.tsx | 20 ++- .../sidebar/DeleteWorktreeDialog.tsx | 9 +- .../DeleteWorktreeTargetPreview.test.tsx | 154 ++++++++++++++++++ .../sidebar/DeleteWorktreeTargetPreview.tsx | 94 +++++++++-- 4 files changed, 265 insertions(+), 12 deletions(-) create mode 100644 src/renderer/src/components/sidebar/DeleteWorktreeTargetPreview.test.tsx diff --git a/src/renderer/src/components/sidebar/DeleteWorktreeDialog.test.tsx b/src/renderer/src/components/sidebar/DeleteWorktreeDialog.test.tsx index 2bea76ee2b9..25b103138f2 100644 --- a/src/renderer/src/components/sidebar/DeleteWorktreeDialog.test.tsx +++ b/src/renderer/src/components/sidebar/DeleteWorktreeDialog.test.tsx @@ -1,3 +1,7 @@ +// @vitest-environment happy-dom + +import '@testing-library/jest-dom/vitest' +import { screen } from '@testing-library/react' import { renderToStaticMarkup } from 'react-dom/server' import type { ButtonHTMLAttributes, ReactNode } from 'react' import { beforeEach, describe, expect, it, vi } from 'vitest' @@ -20,6 +24,10 @@ const mocks = vi.hoisted(() => { openSettingsTarget: vi.fn(), openSettingsPage: vi.fn(), settings: null, + sshTargetLabels: new Map(), + sshConnectionStates: new Map(), + runtimeEnvironments: [] as { id: string; name: string }[], + runtimeStatusByEnvironmentId: new Map(), gitStatusByWorktree: {} as Record, setGitStatus: vi.fn(), deleteStateByWorktreeId: {} as Record< @@ -162,6 +170,11 @@ describe('DeleteWorktreeDialog lineage copy', () => { mocks.state.modalData = {} mocks.state.allWorktrees.mockReturnValue([]) mocks.state.repos = [] + mocks.state.sshTargetLabels = new Map() + mocks.state.sshConnectionStates = new Map() + mocks.state.runtimeEnvironments = [] + mocks.state.runtimeStatusByEnvironmentId = new Map() + document.body.innerHTML = '' mocks.state.worktreeLineageById = {} mocks.state.gitStatusByWorktree = {} mocks.state.deleteStateByWorktreeId = {} @@ -169,7 +182,7 @@ describe('DeleteWorktreeDialog lineage copy', () => { vi.mocked(runWorktreeDeletesInParallel).mockResolvedValue([]) }) - it('previews the confirmed host row when workspace ids collide', async () => { + it('labels the confirmed single-host target when workspace ids collide', async () => { const local = { ...makeWorktree('shared', '/workspaces/local'), instanceId: 'local-instance', @@ -186,13 +199,18 @@ describe('DeleteWorktreeDialog lineage copy', () => { worktreeId: ssh.id, worktreeDeleteIdentities: [{ id: ssh.id, instanceId: ssh.instanceId, hostId: ssh.hostId }] } + mocks.state.sshTargetLabels = new Map([['builder', 'Build server']]) mocks.state.allWorktrees.mockReturnValue([local, ssh]) const { default: DeleteWorktreeDialog } = await import('./DeleteWorktreeDialog') const markup = renderToStaticMarkup() + document.body.innerHTML = markup expect(markup).toContain('SSH target') expect(markup).not.toContain('Local sibling') + expect(screen.getByRole('region')).toHaveAccessibleName( + 'SSH target /workspaces/ssh Build server' + ) }) it('keeps Space safety-checked confirmation non-force', async () => { diff --git a/src/renderer/src/components/sidebar/DeleteWorktreeDialog.tsx b/src/renderer/src/components/sidebar/DeleteWorktreeDialog.tsx index b742641c190..5dc53aa064a 100644 --- a/src/renderer/src/components/sidebar/DeleteWorktreeDialog.tsx +++ b/src/renderer/src/components/sidebar/DeleteWorktreeDialog.tsx @@ -35,6 +35,7 @@ import { useConfirmedWorktreeDeleteTargets } from './use-confirmed-worktree-dele import { runLineageDeleteAll } from './delete-worktree-lineage-delete-all' import { runDialogForceDelete } from './delete-worktree-dialog-force-delete' import { getDeleteStateForWorktreeHost } from './worktree-delete-state-host-match' +import { useSidebarHostScopeOptions } from './use-sidebar-host-scope-options' const DeleteWorktreeDialog = React.memo(function DeleteWorktreeDialog() { const activeModal = useAppStore((s) => s.activeModal) @@ -49,7 +50,11 @@ const DeleteWorktreeDialog = React.memo(function DeleteWorktreeDialog() { const openSettingsTarget = useAppStore((s) => s.openSettingsTarget) const openSettingsPage = useAppStore((s) => s.openSettingsPage) const gitStatusByWorktree = useAppStore((s) => s.gitStatusByWorktree) - + const { hostOptions } = useSidebarHostScopeOptions() + const hostLabelById = useMemo( + () => new Map(hostOptions.map((host) => [host.id, host.label])), + [hostOptions] + ) const isOpen = activeModal === 'delete-worktree' const worktreeId = typeof modalData.worktreeId === 'string' ? modalData.worktreeId : '' const worktreeIds = useMemo( @@ -373,6 +378,8 @@ const DeleteWorktreeDialog = React.memo(function DeleteWorktreeDialog() { isBatchDelete={isBatchDelete} worktree={worktree} worktrees={worktrees} + collisionWorktrees={allWorktrees} + hostLabelById={hostLabelById} deleteStateByWorktreeId={deleteStateByWorktreeId} dirtyChangeCountsByWorktreeId={dirtyChangeCountsByWorktreeId} /> diff --git a/src/renderer/src/components/sidebar/DeleteWorktreeTargetPreview.test.tsx b/src/renderer/src/components/sidebar/DeleteWorktreeTargetPreview.test.tsx new file mode 100644 index 00000000000..937d015f76c --- /dev/null +++ b/src/renderer/src/components/sidebar/DeleteWorktreeTargetPreview.test.tsx @@ -0,0 +1,154 @@ +// @vitest-environment happy-dom + +import '@testing-library/jest-dom/vitest' +import { cleanup, render, screen, within } from '@testing-library/react' +import { afterEach, describe, expect, it } from 'vitest' +import { DeleteWorktreeTargetPreview } from './DeleteWorktreeTargetPreview' +import { buildSidebarHostOptions } from './sidebar-host-options' +import type { Worktree } from '../../../../shared/worktree/types' +import type { ExecutionHostId } from '../../../../shared/execution-host' + +function buildHostLabels( + hostLabelOverrides?: ReadonlyMap +): ReadonlyMap { + return new Map( + buildSidebarHostOptions({ + repos: [ + { connectionId: 'qa-linux-42' }, + { connectionId: null, executionHostId: 'runtime:runtime-7' } + ], + sshTargetLabels: new Map([['qa-linux-42', 'QA Linux']]), + settings: { activeRuntimeEnvironmentId: null }, + runtimeEnvironments: [{ id: 'runtime-7', name: 'Build Mac' }], + hostLabelOverrides + }).map((host) => [host.id, host.label]) + ) +} + +const savedHostLabels = buildHostLabels() + +function makeWorktree(id: string, displayName: string, hostId?: Worktree['hostId']): Worktree { + return { + id, + repoId: 'repo1', + path: `/work/${displayName}`, + head: 'abc123', + branch: 'main', + isBare: false, + isMainWorktree: false, + displayName, + ...(hostId ? { hostId } : {}) + } as Worktree +} + +function renderPreview(args: { + worktrees: readonly Worktree[] + collisionWorktrees?: readonly Worktree[] + worktree?: Worktree | null + isBatchDelete?: boolean + hostLabelById?: ReadonlyMap +}): void { + render( + + ) +} + +afterEach(cleanup) + +describe('DeleteWorktreeTargetPreview host labels', () => { + it('binds saved SSH and runtime host names to their colliding batch rows', () => { + renderPreview({ + worktrees: [ + makeWorktree('shared', 'collide', 'ssh:qa-linux-42'), + makeWorktree('shared', 'collide', 'runtime:runtime-7') + ] + }) + + const sshRow = screen.getByRole('listitem', { name: /QA Linux/ }) + const runtimeRow = screen.getByRole('listitem', { name: /Build Mac/ }) + expect(sshRow).toHaveAccessibleName('collide /work/collide QA Linux') + expect(within(sshRow).getByText('QA Linux')).toBeVisible() + expect(within(sshRow).queryByText('Build Mac')).not.toBeInTheDocument() + expect(runtimeRow).toHaveAccessibleName('collide /work/collide Build Mac') + expect(within(runtimeRow).getByText('Build Mac')).toBeVisible() + expect(within(runtimeRow).queryByText('QA Linux')).not.toBeInTheDocument() + }) + + it('uses configured display-label overrides for colliding hosts', () => { + const worktrees = [ + makeWorktree('shared', 'collide', 'ssh:qa-linux-42'), + makeWorktree('shared', 'collide', 'runtime:runtime-7') + ] + renderPreview({ + worktrees, + hostLabelById: buildHostLabels( + new Map([ + ['ssh:qa-linux-42', 'SSH override'], + ['runtime:runtime-7', 'Runtime override'] + ]) + ) + }) + + expect(screen.getByRole('listitem', { name: /SSH override/ })).toHaveTextContent('SSH override') + expect(screen.getByRole('listitem', { name: /Runtime override/ })).toHaveTextContent( + 'Runtime override' + ) + }) + + it('keeps an unqualified colliding target distinct from local', () => { + renderPreview({ + worktrees: [makeWorktree('shared', 'collide'), makeWorktree('shared', 'collide', 'local')] + }) + + const unknownRow = screen.getByRole('listitem', { name: /Unknown host/ }) + expect(within(unknownRow).getByText('Unknown host')).toBeVisible() + expect(screen.getAllByRole('listitem')).toHaveLength(2) + }) + + it('omits host metadata from every ordinary batch row', () => { + renderPreview({ + worktrees: [ + makeWorktree('one', 'alpha', 'local'), + makeWorktree('two', 'beta', 'ssh:qa-linux-42') + ] + }) + + const alphaRow = screen.getByRole('listitem', { name: 'alpha /work/alpha' }) + const betaRow = screen.getByRole('listitem', { name: 'beta /work/beta' }) + expect(within(alphaRow).queryByText(savedHostLabels.get('local')!)).not.toBeInTheDocument() + expect(within(betaRow).queryByText('QA Linux')).not.toBeInTheDocument() + }) + + it('includes the host in a colliding single target region and its accessible name', () => { + const sshWorktree = makeWorktree('shared', 'collide', 'ssh:qa-linux-42') + const runtimeWorktree = makeWorktree('shared', 'unselected', 'runtime:runtime-7') + renderPreview({ + isBatchDelete: false, + worktree: sshWorktree, + worktrees: [sshWorktree], + collisionWorktrees: [sshWorktree, runtimeWorktree] + }) + + const target = screen.getByRole('region', { name: /QA Linux/ }) + expect(target).toHaveAccessibleName('collide /work/collide QA Linux') + expect(within(target).getByText('QA Linux')).toBeVisible() + expect(screen.queryByText('unselected')).not.toBeInTheDocument() + }) + + it('omits the host from an ordinary single target region', () => { + const sshWorktree = makeWorktree('one', 'alpha', 'ssh:qa-linux-42') + renderPreview({ isBatchDelete: false, worktree: sshWorktree, worktrees: [sshWorktree] }) + + const target = screen.getByRole('region', { name: 'alpha /work/alpha' }) + expect(target).toHaveAccessibleName('alpha /work/alpha') + expect(within(target).queryByText('QA Linux')).not.toBeInTheDocument() + }) +}) diff --git a/src/renderer/src/components/sidebar/DeleteWorktreeTargetPreview.tsx b/src/renderer/src/components/sidebar/DeleteWorktreeTargetPreview.tsx index 34a0b2bc781..fa5a47715ca 100644 --- a/src/renderer/src/components/sidebar/DeleteWorktreeTargetPreview.tsx +++ b/src/renderer/src/components/sidebar/DeleteWorktreeTargetPreview.tsx @@ -1,4 +1,4 @@ -import type { JSX } from 'react' +import { useId, type JSX } from 'react' import { LoaderCircle } from 'lucide-react' import { ScrollArea } from '@/components/ui/scroll-area' import type { Worktree } from '../../../../shared/worktree/types' @@ -6,35 +6,87 @@ import { getWorktreeHostIdentity } from '../../../../shared/worktree/host-qualif import { DeleteWorktreeDirtyChangeHint } from './DeleteWorktreeDirtyChangeHint' import type { AppState } from '@/store/types' import { getDeleteStateForWorktreeHost } from './worktree-delete-state-host-match' +import { + getExecutionHostLabel, + parseExecutionHostId, + type ExecutionHostId +} from '../../../../shared/execution-host' +import { translate } from '@/i18n/i18n' + +function getCollisionIds(worktrees: readonly Worktree[]): ReadonlySet { + const seen = new Set() + const collisions = new Set() + for (const item of worktrees) { + if (seen.has(item.id)) { + collisions.add(item.id) + } else { + seen.add(item.id) + } + } + return collisions +} + +function getTargetHostLabel( + worktree: Worktree, + hostLabelById: ReadonlyMap +): string { + const hostId = parseExecutionHostId(worktree.hostId)?.id + return hostId + ? (hostLabelById.get(hostId) ?? getExecutionHostLabel(hostId)) + : translate('components.workspace.cleanup.host.unknown', 'Unknown host') +} export function DeleteWorktreeTargetPreview({ isBatchDelete, worktree, worktrees, + collisionWorktrees, + hostLabelById, deleteStateByWorktreeId, dirtyChangeCountsByWorktreeId }: { isBatchDelete: boolean worktree: Worktree | null worktrees: readonly Worktree[] + collisionWorktrees: readonly Worktree[] + hostLabelById: ReadonlyMap deleteStateByWorktreeId: AppState['deleteStateByWorktreeId'] dirtyChangeCountsByWorktreeId: ReadonlyMap }): JSX.Element | null { + const targetIdPrefix = useId() + const collisionIds = getCollisionIds(collisionWorktrees) if (isBatchDelete) { return ( -
- {worktrees.map((item) => { +
+ {worktrees.map((item, index) => { const itemDeleteState = getDeleteStateForWorktreeHost(item, deleteStateByWorktreeId) + const labelIds = { + name: `${targetIdPrefix}-${index}-name`, + path: `${targetIdPrefix}-${index}-path`, + host: `${targetIdPrefix}-${index}-host` + } + const showHost = collisionIds.has(item.id) return (
-
{item.displayName}
-
{item.path}
+
+ {item.displayName} +
+
+ {item.path} +
+ {showHost ? ( +
+ {getTargetHostLabel(item, hostLabelById)} +
+ ) : null} -
{worktree.displayName}
-
{worktree.path}
+ if (!worktree) { + return null + } + const labelIds = { + name: `${targetIdPrefix}-name`, + path: `${targetIdPrefix}-path`, + host: `${targetIdPrefix}-host` + } + const showHost = collisionIds.has(worktree.id) + return ( +
+
+ {worktree.displayName} +
+
+ {worktree.path} +
+ {showHost ? ( +
+ {getTargetHostLabel(worktree, hostLabelById)} +
+ ) : null}
- ) : null + ) }