feat(sidebar): name the host when a delete batch collides on one id (#15423)

* 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
This commit is contained in:
Brennan Benson
2026-08-19 01:33:29 -07:00
committed by GitHub
parent 4fc8b65792
commit 73f7767edd
4 changed files with 265 additions and 12 deletions
@@ -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<string, string>(),
sshConnectionStates: new Map(),
runtimeEnvironments: [] as { id: string; name: string }[],
runtimeStatusByEnvironmentId: new Map(),
gitStatusByWorktree: {} as Record<string, { path: string; status: 'modified' }[]>,
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(<DeleteWorktreeDialog />)
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 () => {
@@ -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}
/>
@@ -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<ExecutionHostId, string>
): ReadonlyMap<ExecutionHostId, string> {
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<ExecutionHostId, string>
}): void {
render(
<DeleteWorktreeTargetPreview
isBatchDelete={args.isBatchDelete ?? true}
worktree={args.worktree ?? null}
worktrees={args.worktrees}
collisionWorktrees={args.collisionWorktrees ?? args.worktrees}
hostLabelById={args.hostLabelById ?? savedHostLabels}
deleteStateByWorktreeId={{}}
dirtyChangeCountsByWorktreeId={new Map()}
/>
)
}
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()
})
})
@@ -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<string> {
const seen = new Set<string>()
const collisions = new Set<string>()
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<ExecutionHostId, string>
): 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<ExecutionHostId, string>
deleteStateByWorktreeId: AppState['deleteStateByWorktreeId']
dirtyChangeCountsByWorktreeId: ReadonlyMap<string, number>
}): JSX.Element | null {
const targetIdPrefix = useId()
const collisionIds = getCollisionIds(collisionWorktrees)
if (isBatchDelete) {
return (
<ScrollArea className="max-h-48 rounded-md border border-border/70 bg-muted/35 text-xs">
<div className="space-y-1 px-3 py-2">
{worktrees.map((item) => {
<div className="space-y-1 px-3 py-2" role="list">
{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 (
<div
key={getWorktreeHostIdentity(item)}
role="listitem"
aria-labelledby={`${labelIds.name} ${labelIds.path}${showHost ? ` ${labelIds.host}` : ''}`}
className="min-w-0 border-b border-border/50 py-1 last:border-0"
>
<div className="flex min-w-0 items-start gap-2">
<div className="min-w-0 flex-1">
<div className="break-all font-medium text-foreground">{item.displayName}</div>
<div className="mt-0.5 break-all text-muted-foreground">{item.path}</div>
<div id={labelIds.name} className="break-all font-medium text-foreground">
{item.displayName}
</div>
<div id={labelIds.path} className="mt-0.5 break-all text-muted-foreground">
{item.path}
</div>
{showHost ? (
<div id={labelIds.host} className="mt-0.5 text-muted-foreground">
{getTargetHostLabel(item, hostLabelById)}
</div>
) : null}
<DeleteWorktreeDirtyChangeHint
changeCount={dirtyChangeCountsByWorktreeId.get(
item.hostId ? getWorktreeHostIdentity(item) : item.id
@@ -58,15 +110,37 @@ export function DeleteWorktreeTargetPreview({
)
}
return worktree ? (
<div className="rounded-md border border-border/70 bg-muted/35 px-3 py-2 text-xs">
<div className="break-all font-medium text-foreground">{worktree.displayName}</div>
<div className="mt-1 break-all text-muted-foreground">{worktree.path}</div>
if (!worktree) {
return null
}
const labelIds = {
name: `${targetIdPrefix}-name`,
path: `${targetIdPrefix}-path`,
host: `${targetIdPrefix}-host`
}
const showHost = collisionIds.has(worktree.id)
return (
<div
role="region"
aria-labelledby={`${labelIds.name} ${labelIds.path}${showHost ? ` ${labelIds.host}` : ''}`}
className="rounded-md border border-border/70 bg-muted/35 px-3 py-2 text-xs"
>
<div id={labelIds.name} className="break-all font-medium text-foreground">
{worktree.displayName}
</div>
<div id={labelIds.path} className="mt-1 break-all text-muted-foreground">
{worktree.path}
</div>
{showHost ? (
<div id={labelIds.host} className="mt-0.5 text-muted-foreground">
{getTargetHostLabel(worktree, hostLabelById)}
</div>
) : null}
<DeleteWorktreeDirtyChangeHint
changeCount={dirtyChangeCountsByWorktreeId.get(
worktree.hostId ? getWorktreeHostIdentity(worktree) : worktree.id
)}
/>
</div>
) : null
)
}