fix: address review findings (#1633)

src/renderer/src/components/right-sidebar/CommitArea.primary-icons.test.tsx

@@ -1,5 +1,5 @@
 import { describe, expect, it, vi } from 'vitest'
-import { ArrowDown, ArrowDownUp, ArrowUp, CloudUpload } from 'lucide-react'
+import { ArrowDownUp, ArrowUp, CloudUpload } from 'lucide-react'
 import { CommitArea } from './SourceControl'
 import { Button } from '@/components/ui/button'
 import { resolvePrimaryAction, type PrimaryActionInputs } from './source-control-primary-action'
@@ -83,9 +83,9 @@ function baseProps(overrides: Partial<PrimaryActionInputs> = {}) {
   }
 }
 
-// Why: each remote primary kind is anchored by a directional icon — Push
-// ↑, Pull ↓, Sync ↕, Publish ☁︎↑ — so the verb's direction is visible at a
-// glance and the slot doesn't read as a row of identical pills.
+// Why: remote primaries other than Pull are anchored by a directional
+// icon — Push ↑, Sync ↕, Publish ☁︎↑. Pull is intentionally icon-less
+// because the down-arrow read as a download/save affordance.
 describe('CommitArea primary action icons', () => {
   it('renders an up-arrow on a Push primary', () => {
     const props = baseProps({
@@ -97,14 +97,16 @@ describe('CommitArea primary action icons', () => {
     expect(primaryHasIcon(element, ArrowUp)).toBe(true)
   })
 
-  it('renders a down-arrow on a Pull primary', () => {
+  it('renders no directional icon on a Pull primary', () => {
     const props = baseProps({
       stagedCount: 0,
       hasMessage: false,
       upstreamStatus: { hasUpstream: true, ahead: 0, behind: 1 }
     })
     const element = CommitArea(props)
-    expect(primaryHasIcon(element, ArrowDown)).toBe(true)
+    expect(primaryHasIcon(element, ArrowUp)).toBe(false)
+    expect(primaryHasIcon(element, ArrowDownUp)).toBe(false)
+    expect(primaryHasIcon(element, CloudUpload)).toBe(false)
   })
 
   it('renders a bidirectional arrow on a Sync primary', () => {

src/renderer/src/components/right-sidebar/SourceControl.tsx

@@ -1,7 +1,6 @@
 /* eslint-disable max-lines */
 import React, { useCallback, useEffect, useMemo, useRef, useState } from 'react'
 import {
-  ArrowDown,
   ArrowDownUp,
   ArrowUp,
   ChevronDown,
@@ -114,17 +113,20 @@ const STATUS_ICONS: Record<
 }
 
 // Why: directional signifiers ahead of each primary action label. Commit
-// (✓) is affirmative; Push (↑) and Pull (↓) point in the direction data
-// flows; Sync (↕) is bidirectional; Publish gets a cloud-up to distinguish
-// the first-time publish from a subsequent push. Keeping the mapping
-// outside the render function avoids reallocating it on every render.
-const PRIMARY_ICONS: Record<
-  PrimaryAction['kind'],
-  React.ComponentType<{ className?: string; 'aria-hidden'?: boolean | 'true' | 'false' }>
+// (✓) is affirmative; Push (↑) points in the direction data flows; Sync
+// (↕) is bidirectional; Publish gets a cloud-up to distinguish the
+// first-time publish from a subsequent push. Pull is intentionally
+// icon-less — the down-arrow read as a download/save affordance and was
+// removed. Keeping the mapping outside the render function avoids
+// reallocating it on every render.
+const PRIMARY_ICONS: Partial<
+  Record<
+    PrimaryAction['kind'],
+    React.ComponentType<{ className?: string; 'aria-hidden'?: boolean | 'true' | 'false' }>
+  >
 > = {
   commit: Check,
   push: ArrowUp,
-  pull: ArrowDown,
   sync: ArrowDownUp,
   publish: CloudUpload
 }
@@ -1825,11 +1827,12 @@ export function CommitArea({
   // tooltips.
   const showChevronSpinner = (isCommitting || isRemoteOperationActive) && !showSpinner
 
-  // Why: each primary-kind label is anchored by a directional icon so the
-  // affirmative Commit (✓) reads distinctly from the remote-state labels
-  // sharing this slot — Push (↑), Pull (↓), Sync (↕), Publish (☁︎↑). The
-  // icon is decorative; the label and title attribute carry the meaning
-  // for assistive tech.
+  // Why: most primary-kind labels are anchored by a directional icon so
+  // the affirmative Commit (✓) reads distinctly from the remote-state
+  // labels sharing this slot — Push (↑), Sync (↕), Publish (☁︎↑). Pull is
+  // intentionally icon-less because the down-arrow read as a
+  // download/save affordance. The icon is decorative; the label and
+  // title attribute carry the meaning for assistive tech.
   const PrimaryIcon = PRIMARY_ICONS[primaryAction.kind]
 
   return (
@@ -1865,9 +1868,9 @@ export function CommitArea({
         >
           {showSpinner ? (
             <RefreshCw className="size-3.5 animate-spin" />
-          ) : (
+          ) : PrimaryIcon ? (
             <PrimaryIcon className="size-3.5" aria-hidden="true" />
-          )}
+          ) : null}
           {primaryAction.label}
         </Button>
         <DropdownMenu>

src/renderer/src/components/sidebar/delete-worktree-flow.ts

@@ -4,6 +4,18 @@ import { getWorktreeMapFromState } from '@/store/selectors'
 import { activateAndRevealWorktree } from '@/lib/worktree-activation'
 import { getDeleteWorktreeToastCopy } from './delete-worktree-toast'
 
+// Why: a failed delete almost always means the worktree still has changes
+// that need attention (uncommitted work, unpushed commits, conflicts). The
+// "View" affordance should surface those changes directly, not just bring
+// the worktree into focus, so the user lands on the diff panel where the
+// blocking work is visible.
+function viewWorktreeDiff(worktreeId: string): void {
+  activateAndRevealWorktree(worktreeId)
+  const state = useAppStore.getState()
+  state.setRightSidebarTab('source-control')
+  state.setRightSidebarOpen(true)
+}
+
 /**
  * Shared delete-with-toast flow used by both DeleteWorktreeDialog (confirm
  * path) and WorktreeContextMenu (skip-confirm path). Centralizes the error
@@ -34,7 +46,7 @@ export function runWorktreeDeleteWithToast(worktreeId: string, worktreeName: str
         duration: 10000,
         cancel: {
           label: 'View',
-          onClick: () => activateAndRevealWorktree(worktreeId)
+          onClick: () => viewWorktreeDiff(worktreeId)
         },
         action: canForceDelete
           ? {
@@ -49,7 +61,7 @@ export function runWorktreeDeleteWithToast(worktreeId: string, worktreeName: str
                         description: forceResult.error,
                         action: {
                           label: 'View',
-                          onClick: () => activateAndRevealWorktree(worktreeId)
+                          onClick: () => viewWorktreeDiff(worktreeId)
                         }
                       })
                     }
@@ -59,7 +71,7 @@ export function runWorktreeDeleteWithToast(worktreeId: string, worktreeName: str
                       description: err instanceof Error ? err.message : String(err),
                       action: {
                         label: 'View',
-                        onClick: () => activateAndRevealWorktree(worktreeId)
+                        onClick: () => viewWorktreeDiff(worktreeId)
                       }
                     })
                   })