fix: address review findings (#4513)

This commit is contained in:
Jinjing 2026-06-02 14:58:33 -07:00 committed by GitHub
parent b7b39d2c42
commit 231088b099
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
13 changed files with 335 additions and 17 deletions

View File

@ -102,9 +102,10 @@ export async function getStatus(
if (line.startsWith('# branch.head ')) {
const branchHead = line.slice('# branch.head '.length).trim()
// Why: undefined (not '') for detached/empty so renderer's
// `identity.branch ?? worktree.branch` preserves the prior branch
// value when git can't report one, instead of overwriting it with ''.
// Why: undefined (not '') keeps this parser transport-compatible.
// Renderer refresh code turns "head without branch" into an explicit
// detached-HEAD clear signal while legacy missing-identity payloads
// still preserve the prior branch.
branch = branchHead && branchHead !== '(detached)' ? `refs/heads/${branchHead}` : undefined
continue
}

View File

@ -0,0 +1,45 @@
import React from 'react'
import { GitCommitHorizontal } from 'lucide-react'
import { Badge } from '@/components/ui/badge'
import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip'
import { cn } from '@/lib/utils'
import type { WorktreeGitIdentityDisplay } from '@/lib/worktree-git-identity-display'
type DetachedHeadDisplay = Extract<WorktreeGitIdentityDisplay, { kind: 'detached' }>
type DetachedHeadBadgeProps = {
display: DetachedHeadDisplay
label?: 'sidebar' | 'source-control'
side?: React.ComponentProps<typeof TooltipContent>['side']
className?: string
}
export function DetachedHeadBadge({
display,
label = 'source-control',
side = 'right',
className
}: DetachedHeadBadgeProps): React.JSX.Element {
const visibleLabel = label === 'sidebar' ? display.sidebarLabel : display.sourceControlLabel
return (
<Tooltip>
<TooltipTrigger asChild>
<Badge
variant="outline"
className={cn(
'h-[18px] shrink-0 gap-1 rounded px-1.5 text-[10px] font-medium leading-none',
'border-[color:color-mix(in_srgb,var(--git-decoration-modified)_30%,transparent)] bg-[color:color-mix(in_srgb,var(--git-decoration-modified)_8%,transparent)] text-[color:var(--git-decoration-modified)]',
className
)}
>
<GitCommitHorizontal className="size-2.5" />
{visibleLabel}
</Badge>
</TooltipTrigger>
<TooltipContent side={side} sideOffset={8}>
{display.tooltip}
</TooltipContent>
</Tooltip>
)
}

View File

@ -22,6 +22,7 @@ import { getGitHubPRCacheKey, getGitHubRepoCacheKey } from '@/store/slices/githu
import { useActiveWorktree, useRepoById } from '@/store/selectors'
import { cn } from '@/lib/utils'
import { Button } from '@/components/ui/button'
import { DetachedHeadBadge } from '@/components/DetachedHeadBadge'
import {
DropdownMenu,
DropdownMenuContent,
@ -99,6 +100,7 @@ import { installWindowVisibilityInterval } from '@/lib/window-visibility-interva
import { useMountedRef } from '@/hooks/useMountedRef'
import { callRuntimeRpc, getActiveRuntimeTarget } from '@/runtime/runtime-rpc-client'
import { gitLabPipelineJobsToPRChecks } from '../../../../shared/gitlab-pipeline-checks'
import { getWorktreeGitIdentityDisplay } from '@/lib/worktree-git-identity-display'
const RUNTIME_SSH_STATUS_REFRESH_MS = 3000
const GIT_STATUS_FAILURE_RETRY_MS = 3000
@ -350,7 +352,9 @@ export default function ChecksPanel(): React.JSX.Element {
const gitStatusSnapshotInFlightContextRef = useRef<string | null>(null)
const gitStatusSnapshotRerunContextRef = useRef<string | null>(null)
const gitStatusSnapshotRetryTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null)
const branch = activeWorktree ? activeWorktree.branch.replace(/^refs\/heads\//, '') : ''
const gitIdentityDisplay = activeWorktree ? getWorktreeGitIdentityDisplay(activeWorktree) : null
const detachedHeadDisplay = gitIdentityDisplay?.kind === 'detached' ? gitIdentityDisplay : null
const branch = gitIdentityDisplay?.kind === 'branch' ? gitIdentityDisplay.branchName : ''
const activeWorktreePath = activeWorktree?.path ?? null
const activeWorktreePushTarget = activeWorktree?.pushTarget ?? null
const runtimeEnvironmentId = settings?.activeRuntimeEnvironmentId?.trim() || null
@ -2410,6 +2414,11 @@ export default function ChecksPanel(): React.JSX.Element {
/>
)}
<div className="px-4 py-6">
{detachedHeadDisplay && (
<div className="mb-3">
<DetachedHeadBadge display={detachedHeadDisplay} side="bottom" />
</div>
)}
<div className="text-sm font-medium text-foreground">{emptyStateCopy.title}</div>
<div className="mt-1 text-xs text-muted-foreground">{emptyStateCopy.description}</div>
{!operationInProgress && (
@ -2477,6 +2486,8 @@ export default function ChecksPanel(): React.JSX.Element {
onLinkAnotherPullRequest={handleLinkAnotherPullRequest}
/>
{detachedHeadDisplay && <DetachedHeadBadge display={detachedHeadDisplay} side="bottom" />}
{/* Review title */}
{editingTitle ? (
<div className="flex items-center gap-1">

View File

@ -43,6 +43,7 @@ import { isFolderRepo } from '../../../../shared/repo-kind'
import { Tooltip, TooltipTrigger, TooltipContent, TooltipProvider } from '@/components/ui/tooltip'
import { Popover, PopoverContent, PopoverTrigger } from '@/components/ui/popover'
import { Button } from '@/components/ui/button'
import { DetachedHeadBadge } from '@/components/DetachedHeadBadge'
import {
DropdownMenu,
DropdownMenuContent,
@ -181,6 +182,7 @@ import {
import type { SourceControlAiOperation } from '../../../../shared/source-control-ai-types'
import { getCommitMessageModelDiscoveryHostKeyForScope } from '../../../../shared/commit-message-host-key'
import { getRuntimeGitScope } from '@/runtime/runtime-git-client'
import { getWorktreeGitIdentityDisplay } from '@/lib/worktree-git-identity-display'
import { getRepositorySourceControlAiSectionId } from '@/components/settings/repository-settings-targets'
import {
getCommitFailureDialogWorktreeKey,
@ -1310,7 +1312,9 @@ function SourceControlInner(): React.JSX.Element {
const isFolder = activeRepo ? isFolderRepo(activeRepo) : false
const worktreePath = activeWorktree?.path ?? null
const branchName = activeWorktree?.branch.replace(/^refs\/heads\//, '') ?? 'HEAD'
const gitIdentityDisplay = activeWorktree ? getWorktreeGitIdentityDisplay(activeWorktree) : null
const detachedHeadDisplay = gitIdentityDisplay?.kind === 'detached' ? gitIdentityDisplay : null
const branchName = gitIdentityDisplay?.kind === 'branch' ? gitIdentityDisplay.branchName : ''
const sourceControlAiDiscoveryHostKey = useMemo(
() =>
getCommitMessageModelDiscoveryHostKeyForScope(
@ -4098,6 +4102,12 @@ function SourceControlInner(): React.JSX.Element {
)}
</div>
{detachedHeadDisplay && (
<div className="border-b border-border px-3 py-2">
<DetachedHeadBadge display={detachedHeadDisplay} side="bottom" />
</div>
)}
{scope === 'all' && shouldShowCompareSummary(branchSummary) && (
<div className="border-b border-border px-3 py-2">
<CompareSummary

View File

@ -128,4 +128,26 @@ describe('refreshGitStatusForWorktree', () => {
})
expect(deps.setGitStatus).toHaveBeenCalledWith('wt-3', status)
})
it('clears stale branch identity when git status reports detached HEAD', async () => {
const status: GitStatusResult = {
entries: [],
conflictOperation: 'unknown',
head: 'abc123456789'
}
const gitStatus = vi.fn().mockResolvedValue(status)
vi.stubGlobal('window', { api: { git: { status: gitStatus } } })
const deps = makeDeps()
await refreshGitStatusForWorktree({
worktreeId: 'wt-detached',
worktreePath: '/repo',
deps
})
expect(deps.updateWorktreeGitIdentity).toHaveBeenCalledWith('wt-detached', {
head: 'abc123456789',
branch: null
})
})
})

View File

@ -10,7 +10,7 @@ export type GitStatusRefreshDeps = {
setGitStatus: (worktreeId: string, status: GitStatusResult) => void
updateWorktreeGitIdentity: (
worktreeId: string,
identity: { head?: string; branch?: string }
identity: { head?: string; branch?: string | null }
) => void
setUpstreamStatus: (worktreeId: string, status: GitUpstreamStatus) => void
fetchUpstreamStatus: (
@ -48,7 +48,9 @@ export async function refreshGitStatusForWorktree({
// gives us the new identity without a separate worktree-list poll.
deps.updateWorktreeGitIdentity(worktreeId, {
head: status.head,
branch: status.branch
// Why: detached HEAD reports a head oid and no branch. Pass null as an
// explicit clear signal so stale branch names don't linger in the UI.
branch: status.branch ?? (status.head ? null : undefined)
})
if (pushTarget) {
// Why: porcelain status reports Git's configured upstream. Source Control

View File

@ -203,7 +203,7 @@ describe('WorktreeCard quick actions', () => {
expect(markup).toContain('tabindex="0"')
})
it('does not reserve an empty metadata row for detached git worktrees', () => {
it('renders detached HEAD identity in detailed card metadata', () => {
worktreeCardProperties = []
const markup = renderToStaticMarkup(
@ -216,7 +216,9 @@ describe('WorktreeCard quick actions', () => {
)
expect(markup).toContain('orca')
expect(markup).not.toContain('data-worktree-card-meta-row=""')
expect(markup).toContain('data-worktree-card-meta-row=""')
expect(markup).toContain('Detached HEAD @ abc123')
expect(markup).toContain('Detached HEAD at abc123. You are viewing a commit, not a branch.')
expect(markup).toContain('tabindex="0"')
})

View File

@ -33,7 +33,7 @@ import type {
IssueInfo,
LinearIssue
} from '../../../../shared/types'
import { branchDisplayName, CONFLICT_OPERATION_LABELS } from './WorktreeCardHelpers'
import { CONFLICT_OPERATION_LABELS } from './WorktreeCardHelpers'
import {
WorktreeCardDetailsHover,
hasWorktreeCardDetails,
@ -53,6 +53,8 @@ import {
canShowWorkspaceDeleteQuickAction,
useWorkspaceDeleteModifierPressed
} from './workspace-delete-quick-action'
import { DetachedHeadBadge } from '@/components/DetachedHeadBadge'
import { getWorktreeGitIdentityDisplay } from '@/lib/worktree-git-identity-display'
type WorktreeCardProps = {
worktree: Worktree
@ -201,7 +203,9 @@ const WorktreeCard = React.memo(function WorktreeCard({
repo?.connectionId ? (s.sshTargetLabels.get(repo.connectionId) ?? '') : ''
)
const branch = branchDisplayName(worktree.branch)
const gitIdentityDisplay = getWorktreeGitIdentityDisplay(worktree)
const detachedHeadDisplay = gitIdentityDisplay?.kind === 'detached' ? gitIdentityDisplay : null
const branch = gitIdentityDisplay?.kind === 'branch' ? gitIdentityDisplay.branchName : ''
const isFolder = repo ? isFolderRepo(repo) : false
const hostedReviewCacheKey =
repo && branch
@ -589,6 +593,7 @@ const WorktreeCard = React.memo(function WorktreeCard({
const cacheTtlMs = useAppStore((s) => s.settings?.promptCacheTtlMs ?? 0)
const showInlineRepoBadge = compactCards && !!repo && !hideRepoBadge && !isFolder
const showRepoBadgeInMetaRow = !compactCards && !!repo && !hideRepoBadge
const showDetachedHeadInMetaRow = !compactCards && !isFolder && detachedHeadDisplay !== null
const showBranch =
!isFolder && branch.length > 0 && (!compactCards || branch !== worktree.displayName)
// Why: rebases already surface in source control; keep dense cards from
@ -605,12 +610,13 @@ const WorktreeCard = React.memo(function WorktreeCard({
const showTitleRowPrimary = compactCards && worktree.isMainWorktree && !isFolder
const showMetaRowDetails = !compactCards && (hasDetails || hasPorts)
// Why: detailed cards need a stable metadata lane only when it has content.
// Detached git worktrees can have no branch text, and grouped project
// views hide the repo badge; don't reserve a blank metadata lane in that case.
// Grouped project views can hide the repo badge; don't reserve a blank
// metadata lane unless branch or detached-head identity has content.
const hasDetailedMetaRowContent = Boolean(
(showRepoBadgeInMetaRow && repo) ||
isFolder ||
showBranch ||
showDetachedHeadInMetaRow ||
showConflictOperationBadge ||
cacheStartedAt != null ||
showMetaRowDetails
@ -961,6 +967,13 @@ const WorktreeCard = React.memo(function WorktreeCard({
<span className="min-w-0 text-[11px] text-muted-foreground truncate leading-none">
{branch}
</span>
) : showDetachedHeadInMetaRow && detachedHeadDisplay ? (
<DetachedHeadBadge
display={detachedHeadDisplay}
label="sidebar"
side="right"
className="h-[16px]"
/>
) : null}
{showConflictOperationBadge && (

View File

@ -0,0 +1,53 @@
import { describe, expect, it } from 'vitest'
import {
getDetachedHeadTooltip,
getWorktreeGitIdentityDisplay,
shortGitHead
} from './worktree-git-identity-display'
describe('worktree git identity display', () => {
it('shows a branch name when the worktree has a branch ref', () => {
expect(
getWorktreeGitIdentityDisplay({
branch: 'refs/heads/review/merge-queue',
head: 'abcdef123456'
})
).toEqual({ kind: 'branch', branchName: 'review/merge-queue' })
})
it('shows detached HEAD labels when branch is empty and head is known', () => {
expect(
getWorktreeGitIdentityDisplay({
branch: '',
head: 'abcdef123456'
})
).toEqual({
kind: 'detached',
shortHead: 'abcdef1',
sidebarLabel: 'Detached HEAD @ abcdef1',
sourceControlLabel: 'Detached HEAD · abcdef1',
tooltip: 'Detached HEAD at abcdef1. You are viewing a commit, not a branch.'
})
})
it('treats missing branch from git status as detached when head is known', () => {
expect(
getWorktreeGitIdentityDisplay({
branch: undefined,
head: '1234567890'
})
).toMatchObject({ kind: 'detached', shortHead: '1234567' })
})
it('returns null when neither branch nor head is known', () => {
expect(getWorktreeGitIdentityDisplay({ branch: '', head: '' })).toBeNull()
})
})
describe('detached HEAD copy', () => {
it('formats the required tooltip copy', () => {
expect(getDetachedHeadTooltip(shortGitHead('abc123456789'))).toBe(
'Detached HEAD at abc1234. You are viewing a commit, not a branch.'
)
})
})

View File

@ -0,0 +1,43 @@
export type WorktreeGitIdentityDisplay =
| {
kind: 'branch'
branchName: string
}
| {
kind: 'detached'
shortHead: string
sidebarLabel: string
sourceControlLabel: string
tooltip: string
}
export function shortGitHead(head: string | null | undefined): string {
return (head ?? '').trim().slice(0, 7)
}
export function getDetachedHeadTooltip(shortHead: string): string {
return `Detached HEAD at ${shortHead}. You are viewing a commit, not a branch.`
}
export function getWorktreeGitIdentityDisplay(input: {
branch?: string | null
head?: string | null
}): WorktreeGitIdentityDisplay | null {
const branchName = (input.branch ?? '').replace(/^refs\/heads\//, '').trim()
if (branchName) {
return { kind: 'branch', branchName }
}
const shortHead = shortGitHead(input.head)
if (!shortHead) {
return null
}
return {
kind: 'detached',
shortHead,
sidebarLabel: `Detached HEAD @ ${shortHead}`,
sourceControlLabel: `Detached HEAD · ${shortHead}`,
tooltip: getDetachedHeadTooltip(shortHead)
}
}

View File

@ -168,7 +168,7 @@ export type WorktreeSlice = {
purgeWorktreeTerminalState: (worktreeIds: string[]) => void
updateWorktreeGitIdentity: (
worktreeId: string,
identity: { head?: string; branch?: string }
identity: { head?: string; branch?: string | null }
) => void
updateWorktreeBaseStatus: (event: WorktreeBaseStatusEvent) => void
updateWorktreeRemoteBranchConflict: (event: WorktreeRemoteBranchConflictEvent) => void

View File

@ -1052,6 +1052,108 @@ describe('updateWorktreeGitIdentity', () => {
expect(store.getState().worktreesByRepo.repo1[0].displayName).toBe('My Cool Work')
})
it('clears stale branch identity for detached HEAD updates', () => {
const store = createTestStore()
const existing = makeWorktree({
id: 'repo1::/path/wt1',
repoId: 'repo1',
path: '/path/wt1',
head: 'old-head',
branch: 'refs/heads/review-branch',
displayName: 'Restore PR review'
})
store.setState({ worktreesByRepo: { repo1: [existing] }, sortEpoch: 3 } as Partial<AppState>)
store.getState().updateWorktreeGitIdentity('repo1::/path/wt1', {
head: 'new-head',
branch: null
})
expect(store.getState().worktreesByRepo.repo1[0]).toMatchObject({
head: 'new-head',
branch: '',
displayName: 'Restore PR review'
})
expect(store.getState().sortEpoch).toBe(4)
})
it('keeps an auto-derived title when detached HEAD clears the branch', () => {
const store = createTestStore()
const existing = makeWorktree({
id: 'repo1::/path/wt1',
repoId: 'repo1',
path: '/path/wt1',
head: 'old-head',
branch: 'refs/heads/review-branch',
displayName: 'review-branch'
})
store.setState({ worktreesByRepo: { repo1: [existing] } } as Partial<AppState>)
store.getState().updateWorktreeGitIdentity('repo1::/path/wt1', {
head: 'new-head',
branch: null
})
expect(store.getState().worktreesByRepo.repo1[0]).toMatchObject({
branch: '',
displayName: 'review-branch'
})
})
it('resumes following branch names after an auto-derived title crosses detached HEAD', () => {
const store = createTestStore()
const existing = makeWorktree({
id: 'repo1::/path/wt1',
repoId: 'repo1',
path: '/path/wt1',
head: 'old-head',
branch: 'refs/heads/review-branch',
displayName: 'review-branch'
})
store.setState({ worktreesByRepo: { repo1: [existing] } } as Partial<AppState>)
store.getState().updateWorktreeGitIdentity('repo1::/path/wt1', {
head: 'detached-head',
branch: null
})
store.getState().updateWorktreeGitIdentity('repo1::/path/wt1', {
head: 'reattached-head',
branch: 'refs/heads/main'
})
expect(store.getState().worktreesByRepo.repo1[0]).toMatchObject({
branch: 'refs/heads/main',
displayName: 'main'
})
})
it('preserves custom detached titles when a branch returns', () => {
const store = createTestStore()
const existing = makeWorktree({
id: 'repo1::/path/wt1',
repoId: 'repo1',
path: '/path/wt1',
head: 'detached-head',
branch: '',
displayName: 'Restore PR review'
})
store.setState({ worktreesByRepo: { repo1: [existing] } } as Partial<AppState>)
store.getState().updateWorktreeGitIdentity('repo1::/path/wt1', {
head: 'reattached-head',
branch: 'refs/heads/main'
})
expect(store.getState().worktreesByRepo.repo1[0]).toMatchObject({
branch: 'refs/heads/main',
displayName: 'Restore PR review'
})
})
})
describe('createWorktree base status merge', () => {

View File

@ -45,6 +45,7 @@ const ACTIVE_WORKTREE_TERMINAL_PREP_DELAY_MS = 300
const ACTIVE_WORKTREE_TERMINAL_PREP_INPUT_QUIET_MS = 450
const ACTIVE_WORKTREE_TERMINAL_PREP_IDLE_TIMEOUT_MS = 180
const pendingActivationTerminalPrepCancels = new Map<string, () => void>()
const detachedHeadAutoDerivedDisplayNames = new Map<string, string>()
function countTerminalLayoutLeaves(node: TerminalPaneLayoutNode | null | undefined): number {
if (!node) {
@ -922,15 +923,28 @@ export const createWorktreeSlice: StateCreator<AppState, [], [], WorktreeSlice>
return worktree
}
const nextHead = identity.head ?? worktree.head
const nextBranch = identity.branch ?? worktree.branch
const nextBranch = identity.branch === null ? '' : (identity.branch ?? worktree.branch)
if (nextHead === worktree.head && nextBranch === worktree.branch) {
return worktree
}
changed = true
// Why: terminal branch switches only patch branch/head here; auto-derived
// titles need the same branch derivation that full worktree listing uses.
const wasAutoDerived = worktree.displayName === branchName(worktree.branch)
const nextDisplayName = wasAutoDerived ? branchName(nextBranch) : worktree.displayName
const currentBranchName = branchName(worktree.branch)
const wasAutoDerived = worktree.displayName === currentBranchName
const wasDetachedAutoDerived =
worktree.branch === '' &&
nextBranch !== '' &&
detachedHeadAutoDerivedDisplayNames.get(worktreeId) === worktree.displayName
const nextDisplayName =
(wasAutoDerived || wasDetachedAutoDerived) && nextBranch
? branchName(nextBranch)
: worktree.displayName
if (identity.branch === null && wasAutoDerived) {
detachedHeadAutoDerivedDisplayNames.set(worktreeId, worktree.displayName)
} else if (identity.branch !== undefined) {
detachedHeadAutoDerivedDisplayNames.delete(worktreeId)
}
return { ...worktree, head: nextHead, branch: nextBranch, displayName: nextDisplayName }
})