From cd30a335d1b059b7fbc02e5ce4636b615d804d49 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sat, 9 May 2026 16:32:07 -0700 Subject: [PATCH] fix: address review findings (#1633) --- .../CommitArea.primary-icons.test.tsx | 14 ++++---- .../right-sidebar/SourceControl.tsx | 35 ++++++++++--------- .../sidebar/delete-worktree-flow.ts | 18 ++++++++-- 3 files changed, 42 insertions(+), 25 deletions(-) diff --git a/src/renderer/src/components/right-sidebar/CommitArea.primary-icons.test.tsx b/src/renderer/src/components/right-sidebar/CommitArea.primary-icons.test.tsx index 214fa42d2a9..975170fdd36 100644 --- a/src/renderer/src/components/right-sidebar/CommitArea.primary-icons.test.tsx +++ b/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 = {}) { } } -// 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', () => { diff --git a/src/renderer/src/components/right-sidebar/SourceControl.tsx b/src/renderer/src/components/right-sidebar/SourceControl.tsx index a6cc9754576..ed050bce648 100644 --- a/src/renderer/src/components/right-sidebar/SourceControl.tsx +++ b/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 ? ( - ) : ( + ) : PrimaryIcon ? (