From 6455b0af256dddda7db26652ef4769481d738d77 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Tue, 2 Jun 2026 00:46:41 -0700 Subject: [PATCH] Move PR comment action into the comments header (#4458) - Replace the collapsed composer row with a compact plus action in the comments header, opening the composer only when requested. - Hide clean branch compare summaries and remove the inactive view-mode toggle from compare toolbar actions. --- .../SourceControl.commit-drafts.test.ts | 195 ------------------ .../SourceControl.compare-summary.test.ts | 145 +++++++++++++ .../right-sidebar/SourceControl.tsx | 143 +++---------- .../checks-panel-content.test.tsx | 23 ++- .../right-sidebar/checks-panel-content.tsx | 77 ++++--- .../right-panel-comment-composer.tsx | 10 +- 6 files changed, 240 insertions(+), 353 deletions(-) create mode 100644 src/renderer/src/components/right-sidebar/SourceControl.compare-summary.test.ts diff --git a/src/renderer/src/components/right-sidebar/SourceControl.commit-drafts.test.ts b/src/renderer/src/components/right-sidebar/SourceControl.commit-drafts.test.ts index 7e6de0863e4..28b554997cb 100644 --- a/src/renderer/src/components/right-sidebar/SourceControl.commit-drafts.test.ts +++ b/src/renderer/src/components/right-sidebar/SourceControl.commit-drafts.test.ts @@ -1,78 +1,13 @@ import { describe, expect, it, vi } from 'vitest' -import { ListTree } from 'lucide-react' -import { Button } from '@/components/ui/button' import { buildResolveConflictsPrompt, - CompareSummary, - CompareSummaryToolbarButton, - getNextSourceControlViewMode, normalizeSourceControlViewMode, pickDefaultSourceControlAgent, readCommitDraftForWorktree, refreshSourceControlAfterRemoteAction, - requestSourceControlViewModePreferenceWrite, shouldRenderCommitArea, - type SourceControlViewModePreferenceWriteState, writeCommitDraftForWorktree } from './SourceControl' -import type { GitBranchCompareSummary } from '../../../../shared/types' - -type ReactElementLike = { - type: unknown - props: Record -} - -function visit(node: unknown, cb: (node: ReactElementLike) => void): void { - if (node == null || typeof node === 'string' || typeof node === 'number') { - return - } - if (Array.isArray(node)) { - node.forEach((entry) => visit(entry, cb)) - return - } - const element = node as ReactElementLike - cb(element) - if (element.props?.children) { - visit(element.props.children, cb) - } -} - -function findInnerButton(node: unknown): ReactElementLike { - let found: ReactElementLike | null = null - visit(node, (entry) => { - if (entry.type === Button) { - found = entry - } - }) - if (!found) { - throw new Error('inner Button not found') - } - return found -} - -function findCompareSummaryToolbarButton(node: unknown, label: string): ReactElementLike { - let found: ReactElementLike | null = null - visit(node, (entry) => { - if (entry.type === CompareSummaryToolbarButton && entry.props.label === label) { - found = entry - } - }) - if (!found) { - throw new Error(`toolbar button not found: ${label}`) - } - return found -} - -const readySummary: GitBranchCompareSummary = { - baseRef: 'origin/main', - baseOid: 'base', - compareRef: 'feature', - headOid: 'head', - mergeBase: 'base', - changedFiles: 2, - commitsAhead: 1, - status: 'ready' -} describe('SourceControl commit drafts by worktree', () => { it('returns an empty draft when the selected worktree has no message', () => { @@ -207,134 +142,4 @@ describe('SourceControl view mode preference', () => { expect(normalizeSourceControlViewMode('list')).toBe('list') expect(normalizeSourceControlViewMode('tree')).toBe('tree') }) - - it('toggles between list and tree', () => { - expect(getNextSourceControlViewMode('list')).toBe('tree') - expect(getNextSourceControlViewMode('tree')).toBe('list') - }) - - it('does not persist the fallback list mode before settings hydrate', () => { - const writeState: SourceControlViewModePreferenceWriteState = { - writeChain: Promise.resolve(), - writeSeq: 0 - } - const setOptimisticMode = vi.fn() - const updateSettings = vi.fn() - - const result = requestSourceControlViewModePreferenceWrite({ - hydrated: false, - currentMode: 'list', - writeState, - setOptimisticMode, - updateSettings - }) - - expect(result).toBeNull() - expect(setOptimisticMode).not.toHaveBeenCalled() - expect(updateSettings).not.toHaveBeenCalled() - }) - - it('queues rapid toggle writes so the last intent clears optimistic state', async () => { - const writeState: SourceControlViewModePreferenceWriteState = { - writeChain: Promise.resolve(), - writeSeq: 0 - } - const optimisticModes: ('list' | 'tree' | null)[] = [] - const firstWrite: { resolve: (() => void) | null } = { resolve: null } - const updateSettings = vi.fn( - ({ sourceControlViewMode }: { sourceControlViewMode: 'list' | 'tree' }) => { - if (sourceControlViewMode === 'tree') { - return new Promise((resolve) => { - firstWrite.resolve = resolve - }) - } - return Promise.resolve() - } - ) - - expect( - requestSourceControlViewModePreferenceWrite({ - hydrated: true, - currentMode: 'list', - writeState, - setOptimisticMode: (mode) => optimisticModes.push(mode), - updateSettings - }) - ).toBe('tree') - await Promise.resolve() - - expect( - requestSourceControlViewModePreferenceWrite({ - hydrated: true, - currentMode: 'tree', - writeState, - setOptimisticMode: (mode) => optimisticModes.push(mode), - updateSettings - }) - ).toBe('list') - await Promise.resolve() - - expect(updateSettings).toHaveBeenCalledTimes(1) - expect(updateSettings).toHaveBeenLastCalledWith({ sourceControlViewMode: 'tree' }) - - expect(firstWrite.resolve).not.toBeNull() - firstWrite.resolve?.() - await writeState.writeChain - await Promise.resolve() - - expect(updateSettings).toHaveBeenCalledTimes(2) - expect(updateSettings).toHaveBeenLastCalledWith({ sourceControlViewMode: 'list' }) - expect(optimisticModes).toEqual(['tree', 'list', null]) - }) - - it('wires the compare toolbar toggle label and click handler from the rendered mode', () => { - const onToggleViewMode = vi.fn() - const node = CompareSummary({ - summary: readySummary, - viewMode: 'tree', - onChangeBaseRef: vi.fn(), - onToggleViewMode, - onRetry: vi.fn() - }) - - const toggle = findCompareSummaryToolbarButton(node, 'Show changes as list') - expect(toggle.props.disabled).toBeUndefined() - - const onClick = toggle.props.onClick - expect(typeof onClick).toBe('function') - if (typeof onClick === 'function') { - onClick() - } - expect(onToggleViewMode).toHaveBeenCalledTimes(1) - }) - - it('renders the hydrated-disabled toolbar toggle as inert', () => { - const onToggleViewMode = vi.fn() - const node = CompareSummary({ - summary: readySummary, - viewMode: 'list', - onChangeBaseRef: vi.fn(), - onToggleViewMode, - viewModeToggleDisabled: true, - onRetry: vi.fn() - }) - const toggle = findCompareSummaryToolbarButton(node, 'Show changes as tree') - expect(toggle.props.disabled).toBe(true) - - const button = findInnerButton( - CompareSummaryToolbarButton({ - icon: ListTree, - label: 'Show changes as tree', - onClick: onToggleViewMode, - disabled: true - }) - ) - expect(button.props['aria-disabled']).toBe(true) - const onClick = button.props.onClick - expect(typeof onClick).toBe('function') - if (typeof onClick === 'function') { - onClick() - } - expect(onToggleViewMode).not.toHaveBeenCalled() - }) }) diff --git a/src/renderer/src/components/right-sidebar/SourceControl.compare-summary.test.ts b/src/renderer/src/components/right-sidebar/SourceControl.compare-summary.test.ts new file mode 100644 index 00000000000..044291e1347 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/SourceControl.compare-summary.test.ts @@ -0,0 +1,145 @@ +import { describe, expect, it, vi } from 'vitest' +import { + CompareSummary, + CompareSummaryToolbarButton, + shouldShowCompareSummary +} from './SourceControl' +import type { GitBranchCompareSummary } from '../../../../shared/types' + +type ReactElementLike = { + type: unknown + props: Record +} + +function visit(node: unknown, cb: (node: ReactElementLike) => void): void { + if (node == null || typeof node === 'string' || typeof node === 'number') { + return + } + if (Array.isArray(node)) { + node.forEach((entry) => visit(entry, cb)) + return + } + const element = node as ReactElementLike + cb(element) + if (element.props?.children) { + visit(element.props.children, cb) + } +} + +function collectText(node: unknown): string { + if (node == null) { + return '' + } + if (typeof node === 'string' || typeof node === 'number') { + return String(node) + } + if (Array.isArray(node)) { + return node.map(collectText).join('') + } + const element = node as ReactElementLike + return collectText(element.props?.children) +} + +function findCompareSummaryToolbarButton(node: unknown, label: string): ReactElementLike { + let found: ReactElementLike | null = null + visit(node, (entry) => { + if (entry.type === CompareSummaryToolbarButton && entry.props.label === label) { + found = entry + } + }) + if (!found) { + throw new Error(`toolbar button not found: ${label}`) + } + return found +} + +function collectCompareSummaryToolbarLabels(node: unknown): string[] { + const labels: string[] = [] + visit(node, (entry) => { + if (entry.type === CompareSummaryToolbarButton && typeof entry.props.label === 'string') { + labels.push(entry.props.label) + } + }) + return labels +} + +const readySummary: GitBranchCompareSummary = { + baseRef: 'origin/main', + baseOid: 'base', + compareRef: 'feature', + headOid: 'head', + mergeBase: 'base', + changedFiles: 2, + commitsAhead: 1, + status: 'ready' +} + +describe('SourceControl compare summary', () => { + it('wires toolbar actions without rendering the dead view-mode toggle', () => { + const onChangeBaseRef = vi.fn() + const onRetry = vi.fn() + const node = CompareSummary({ + summary: readySummary, + onChangeBaseRef, + onRetry + }) + + expect(collectCompareSummaryToolbarLabels(node)).toEqual([ + 'Change base ref', + 'Refresh branch compare' + ]) + + const changeBaseRef = findCompareSummaryToolbarButton(node, 'Change base ref').props.onClick + if (typeof changeBaseRef === 'function') { + changeBaseRef() + } + expect(onChangeBaseRef).toHaveBeenCalledTimes(1) + + const refresh = findCompareSummaryToolbarButton(node, 'Refresh branch compare').props.onClick + if (typeof refresh === 'function') { + refresh() + } + expect(onRetry).toHaveBeenCalledTimes(1) + }) + + it('omits the whole compare row when the branch has no commits ahead', () => { + const cleanSummary = { ...readySummary, commitsAhead: 0 } + const node = CompareSummary({ + summary: cleanSummary, + onChangeBaseRef: vi.fn(), + onRetry: vi.fn() + }) + + expect(shouldShowCompareSummary(cleanSummary)).toBe(false) + expect(node).toBeNull() + const text = collectText(node) + expect(text).not.toContain('0 commits ahead') + expect(text).not.toContain('origin/main') + }) + + it('keeps non-zero summary copy compact', () => { + const node = CompareSummary({ + summary: readySummary, + onChangeBaseRef: vi.fn(), + onRetry: vi.fn() + }) + + const text = collectText(node) + expect(text).toContain('1 ahead') + expect(text).not.toContain('1 commit ahead of origin/main') + }) + + it('omits the view-mode toggle from unavailable compare rows', () => { + const node = CompareSummary({ + summary: { + ...readySummary, + status: 'error', + errorMessage: 'Unable to compare' + }, + onChangeBaseRef: vi.fn(), + onRetry: vi.fn() + }) + + expect(collectCompareSummaryToolbarLabels(node)).toEqual(['Change base ref', 'Retry']) + }) +}) diff --git a/src/renderer/src/components/right-sidebar/SourceControl.tsx b/src/renderer/src/components/right-sidebar/SourceControl.tsx index 774945091a9..2e03ccb72a8 100644 --- a/src/renderer/src/components/right-sidebar/SourceControl.tsx +++ b/src/renderer/src/components/right-sidebar/SourceControl.tsx @@ -20,8 +20,6 @@ import { FolderOpen, GitMerge, GitPullRequestArrow, - List, - ListTree, MessageSquare, Trash, Trash2, @@ -160,7 +158,6 @@ import type { GitConflictKind, GitConflictOperation, GitStatusEntry, - GlobalSettings, SourceControlViewMode, TuiAgent } from '../../../../shared/types' @@ -371,57 +368,6 @@ export function normalizeSourceControlViewMode(value: unknown): SourceControlVie return value === 'tree' || value === 'list' ? value : 'list' } -export function getNextSourceControlViewMode(mode: SourceControlViewMode): SourceControlViewMode { - return mode === 'tree' ? 'list' : 'tree' -} - -export type SourceControlViewModePreferenceWriteState = { - writeChain: Promise - writeSeq: number -} - -export function requestSourceControlViewModePreferenceWrite({ - hydrated, - currentMode, - writeState, - setOptimisticMode, - updateSettings -}: { - hydrated: boolean - currentMode: SourceControlViewMode - writeState: SourceControlViewModePreferenceWriteState - setOptimisticMode: (mode: SourceControlViewMode | null) => void - updateSettings: ( - updates: Pick - ) => Promise -}): SourceControlViewMode | null { - if (!hydrated) { - return null - } - const next = getNextSourceControlViewMode(currentMode) - const writeSeq = writeState.writeSeq + 1 - writeState.writeSeq = writeSeq - setOptimisticMode(next) - - // Why: settings writes cross IPC. Queue them so rapid toolbar clicks keep - // the user's final intent as the persisted value even if earlier writes - // would otherwise resolve after later clicks. - const write = writeState.writeChain - .catch(() => undefined) - .then(() => updateSettings({ sourceControlViewMode: next })) - .then(() => undefined) - writeState.writeChain = write - void write - .finally(() => { - if (writeState.writeSeq === writeSeq) { - setOptimisticMode(null) - } - }) - .catch(() => undefined) - - return next -} - type GitStatusSourceControlTreeNode = SourceControlTreeNode< GitStatusEntry, (typeof SECTION_ORDER)[number] @@ -1122,7 +1068,6 @@ function SourceControlInner(): React.JSX.Element { const isRemoteOperationActive = useAppStore((s) => s.isRemoteOperationActive) const inFlightRemoteOpKind = useAppStore((s) => s.inFlightRemoteOpKind) const settings = useAppStore((s) => s.settings) - const updateSettings = useAppStore((s) => s.updateSettings) const openSettingsTarget = useAppStore((s) => s.openSettingsTarget) const openSettingsPage = useAppStore((s) => s.openSettingsPage) const hostedReviewCache = useAppStore((s) => s.hostedReviewCache) @@ -1279,26 +1224,10 @@ function SourceControlInner(): React.JSX.Element { const [collapsedSections, setCollapsedSections] = useState>( createDefaultCollapsedSections ) - const [optimisticSourceControlViewMode, setOptimisticSourceControlViewMode] = - useState(null) - const sourceControlViewModeWriteStateRef = useRef({ - writeChain: Promise.resolve(), - writeSeq: 0 - }) const persistedSourceControlViewMode = normalizeSourceControlViewMode( settings?.sourceControlViewMode ) - const sourceControlViewMode = optimisticSourceControlViewMode ?? persistedSourceControlViewMode - const isSourceControlViewModeHydrated = settings !== null - const handleToggleSourceControlViewMode = useCallback(() => { - requestSourceControlViewModePreferenceWrite({ - hydrated: isSourceControlViewModeHydrated, - currentMode: sourceControlViewMode, - writeState: sourceControlViewModeWriteStateRef.current, - setOptimisticMode: setOptimisticSourceControlViewMode, - updateSettings - }) - }, [isSourceControlViewModeHydrated, sourceControlViewMode, updateSettings]) + const sourceControlViewMode = persistedSourceControlViewMode const [collapsedTreeDirs, setCollapsedTreeDirs] = useState>(new Set()) const [baseRefDialogOpen, setBaseRefDialogOpen] = useState(false) const [pendingDiscard, setPendingDiscard] = useState(null) @@ -4169,14 +4098,11 @@ function SourceControlInner(): React.JSX.Element { )} - {scope === 'all' && ( + {scope === 'all' && shouldShowCompareSummary(branchSummary) && (
setBaseRefDialogOpen(true)} - onToggleViewMode={handleToggleSourceControlViewMode} - viewModeToggleDisabled={!isSourceControlViewModeHydrated} onRetry={() => void refreshBranchCompare()} />
@@ -5946,21 +5872,25 @@ export function CommitArea({ ) } +export function shouldShowCompareSummary(summary: GitBranchCompareSummary | null): boolean { + if (!summary || summary.status === 'loading') { + return true + } + if (summary.status !== 'ready') { + return true + } + return typeof summary.commitsAhead === 'number' && summary.commitsAhead > 0 +} + export function CompareSummary({ summary, - viewMode, onChangeBaseRef, - onToggleViewMode, - viewModeToggleDisabled, onRetry }: { summary: GitBranchCompareSummary | null - viewMode: SourceControlViewMode onChangeBaseRef: () => void - onToggleViewMode: () => void - viewModeToggleDisabled?: boolean onRetry: () => void -}): React.JSX.Element { +}): React.JSX.Element | null { if (!summary || summary.status === 'loading') { return (
@@ -5982,37 +5912,34 @@ export function CompareSummary({ label="Change base ref" onClick={onChangeBaseRef} /> -
) } + const commitsAhead = summary.commitsAhead + const showCommitsAhead = typeof commitsAhead === 'number' && commitsAhead > 0 + const commitsAheadTitle = showCommitsAhead + ? `${commitsAhead} ${commitsAhead === 1 ? 'commit' : 'commits'} ahead of ${summary.baseRef}` + : undefined + + if (!showCommitsAhead) { + return null + } + return (
- {summary.commitsAhead !== undefined && ( - - {summary.commitsAhead} commits ahead of {summary.baseRef} - - )} + + + {commitsAhead} ahead +
- void - disabled?: boolean }): React.JSX.Element { return ( @@ -6041,17 +5966,9 @@ export function CompareSummaryToolbarButton({ type="button" variant="ghost" size="icon-xs" - className={cn( - 'text-muted-foreground hover:text-foreground', - disabled && 'cursor-not-allowed opacity-50' - )} + className="text-muted-foreground hover:text-foreground" aria-label={label} - aria-disabled={disabled} - onClick={() => { - if (!disabled) { - onClick() - } - }} + onClick={onClick} > diff --git a/src/renderer/src/components/right-sidebar/checks-panel-content.test.tsx b/src/renderer/src/components/right-sidebar/checks-panel-content.test.tsx index 6975b4c44cb..a5eab1d8fd8 100644 --- a/src/renderer/src/components/right-sidebar/checks-panel-content.test.tsx +++ b/src/renderer/src/components/right-sidebar/checks-panel-content.test.tsx @@ -1,6 +1,7 @@ import React from 'react' import { renderToStaticMarkup } from 'react-dom/server' import { describe, expect, it } from 'vitest' +import { TooltipProvider } from '@/components/ui/tooltip' import type { PRCheckDetail, PRComment, PRInfo } from '../../../../shared/types' import { CheckJobLogTail, @@ -11,6 +12,10 @@ import { PRTriageStrip } from './checks-panel-content' +function renderWithTooltips(element: React.ReactElement): string { + return renderToStaticMarkup(React.createElement(TooltipProvider, null, element)) +} + function makePR(overrides: Partial = {}): PRInfo { return { number: 42, @@ -102,7 +107,7 @@ describe('MergeConflictNotice', () => { }) describe('PRCommentsList', () => { - it('places the collapsed add-comment action after existing comments', () => { + it('places the collapsed add-comment action in the comments header', () => { const comments: PRComment[] = [ { id: 1, @@ -114,7 +119,7 @@ describe('PRCommentsList', () => { } ] - const markup = renderToStaticMarkup( + const markup = renderWithTooltips( React.createElement(PRCommentsList, { comments, commentsLoading: false, @@ -122,14 +127,16 @@ describe('PRCommentsList', () => { }) ) - expect(markup.indexOf('Existing review context')).toBeLessThan( - markup.indexOf('Add a comment...') + expect(markup.indexOf('aria-label="Add comment"')).toBeLessThan( + markup.indexOf('Existing review context') ) + expect(markup).toContain('lucide-plus') + expect(markup).not.toContain('Add a comment...') expect(markup).not.toContain('Add a PR comment') }) - it('uses the collapsed composer as the empty comments state', () => { - const markup = renderToStaticMarkup( + it('uses the header plus action as the empty comments state', () => { + const markup = renderWithTooltips( React.createElement(PRCommentsList, { comments: [], commentsLoading: false, @@ -137,7 +144,9 @@ describe('PRCommentsList', () => { }) ) - expect(markup).toContain('Start conversation...') + expect(markup).toContain('aria-label="Start conversation"') + expect(markup).toContain('lucide-plus') + expect(markup).not.toContain('Start conversation...') expect(markup).not.toContain('No comments yet') expect(markup).not.toContain('Add a comment') expect((markup.match(/lucide-message-square/g) ?? []).length).toBe(1) diff --git a/src/renderer/src/components/right-sidebar/checks-panel-content.tsx b/src/renderer/src/components/right-sidebar/checks-panel-content.tsx index 73708c9baee..809e69bd134 100644 --- a/src/renderer/src/components/right-sidebar/checks-panel-content.tsx +++ b/src/renderer/src/components/right-sidebar/checks-panel-content.tsx @@ -12,6 +12,7 @@ import { Copy, Check, MessageSquare, + Plus, ChevronDown, ChevronRight, Sparkles, @@ -22,6 +23,7 @@ import { } from 'lucide-react' import { ExternalLink } from 'lucide-react' import { Button } from '@/components/ui/button' +import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip' import { Accordion, AccordionContent, @@ -1564,34 +1566,20 @@ export function PRCommentsList({ setIsAddingComment(false) }, []) - const renderAddCommentSurface = (empty: boolean): React.JSX.Element => ( + const renderAddCommentComposer = (empty: boolean): React.JSX.Element => (
- {isAddingComment ? ( - ({ ok: false, error: 'Commenting unavailable.' }))} - /> - ) : ( - // Why: the empty comments state should be a single composer affordance; - // duplicating "no comments" copy or the header icon makes the panel noisy. - - )} + ({ ok: false, error: 'Commenting unavailable.' }))} + />
) @@ -1599,12 +1587,37 @@ export function PRCommentsList({
{/* Header */}
-
+
Comments {comments.length > 0 && ( {comments.length} )} + {onAddComment && !isAddingComment && ( + + + + + + {commentsDisabled && commentsDisabledReason + ? commentsDisabledReason + : comments.length === 0 + ? 'Start conversation' + : 'Add comment'} + + + )}
{comments.length > 0 && (
@@ -1640,12 +1653,14 @@ export function PRCommentsList({
- ) : comments.length === 0 && onAddComment ? ( - renderAddCommentSurface(true) + ) : comments.length === 0 && isAddingComment && onAddComment ? ( + renderAddCommentComposer(true) ) : comments.length === 0 ? ( -
- No comments -
+ !onAddComment && ( +
+ No comments +
+ ) ) : visibleComments.length === 0 ? (
{getPRCommentAudienceEmptyLabel(commentFilter)} @@ -1684,7 +1699,7 @@ export function PRCommentsList({ })}
)} - {onAddComment && comments.length > 0 && renderAddCommentSurface(false)} + {onAddComment && comments.length > 0 && isAddingComment && renderAddCommentComposer(false)}
) } diff --git a/src/renderer/src/components/right-sidebar/right-panel-comment-composer.tsx b/src/renderer/src/components/right-sidebar/right-panel-comment-composer.tsx index b51b7ac3023..2cf89db96ff 100644 --- a/src/renderer/src/components/right-sidebar/right-panel-comment-composer.tsx +++ b/src/renderer/src/components/right-sidebar/right-panel-comment-composer.tsx @@ -1,5 +1,5 @@ import React, { useCallback, useEffect, useRef, useState } from 'react' -import { Bold, Code2, Italic, List, LoaderCircle, Quote, Send } from 'lucide-react' +import { Bold, Code2, Italic, List, Quote } from 'lucide-react' import { Button } from '@/components/ui/button' import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip' import { ShortcutKeyCombo } from '@/components/ShortcutKeyCombo' @@ -232,15 +232,11 @@ export function RightPanelCommentComposer({