From babedfafde175cb0d252eb7002438b05df14b7e4 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sat, 16 May 2026 22:45:01 -0700 Subject: [PATCH] Improve commit failure display (#2141) --- .../CommitArea.chevron-spinner.test.tsx | 168 ++----- .../CommitArea.generate.test.tsx | 175 +++---- .../CommitArea.primary-icons.test.tsx | 147 +++--- .../right-sidebar/CommitArea.test.tsx | 468 ++++++------------ .../right-sidebar/SourceControl.tsx | 81 ++- .../commit-failure-summary.test.ts | 45 ++ .../right-sidebar/commit-failure-summary.ts | 63 +++ 7 files changed, 515 insertions(+), 632 deletions(-) create mode 100644 src/renderer/src/components/right-sidebar/commit-failure-summary.test.ts create mode 100644 src/renderer/src/components/right-sidebar/commit-failure-summary.ts diff --git a/src/renderer/src/components/right-sidebar/CommitArea.chevron-spinner.test.tsx b/src/renderer/src/components/right-sidebar/CommitArea.chevron-spinner.test.tsx index 0fdb06901bd..1874672ebdf 100644 --- a/src/renderer/src/components/right-sidebar/CommitArea.chevron-spinner.test.tsx +++ b/src/renderer/src/components/right-sidebar/CommitArea.chevron-spinner.test.tsx @@ -1,7 +1,6 @@ import { describe, expect, it, vi } from 'vitest' -import { RefreshCw } from 'lucide-react' +import { renderToStaticMarkup } from 'react-dom/server' import { CommitArea } from './SourceControl' -import { Button } from '@/components/ui/button' import { resolvePrimaryAction, type PrimaryActionInputs } from './source-control-primary-action' import { resolveDropdownItems, type DropdownActionKind } from './source-control-dropdown-items' @@ -10,62 +9,6 @@ import { resolveDropdownItems, type DropdownActionKind } from './source-control- // behaviour for dropdown-only ops (Fetch) and the no-double-spin guard // when the primary button already hosts the in-flight indicator. -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 findButtons(node: unknown): ReactElementLike[] { - const buttons: ReactElementLike[] = [] - visit(node, (entry) => { - if (entry.type === Button) { - buttons.push(entry) - } - }) - return buttons -} - -function buttonHasSpinner(button: ReactElementLike): boolean { - let found = false - visit(button, (entry) => { - if (entry.type === RefreshCw) { - found = true - } - }) - return found -} - -function primaryHasSpinner(node: unknown): boolean { - const buttons = findButtons(node) - if (buttons.length === 0) { - throw new Error('primary button not found') - } - return buttonHasSpinner(buttons[0]!) -} - -function chevronHasSpinner(node: unknown): boolean { - const buttons = findButtons(node) - if (buttons.length < 2) { - throw new Error('chevron button not found') - } - return buttonHasSpinner(buttons[1]!) -} - function buildInputs(overrides: Partial = {}): PrimaryActionInputs { return { stagedCount: 1, @@ -83,6 +26,7 @@ function buildInputs(overrides: Partial = {}): PrimaryActio function baseProps(overrides: Partial = {}) { const inputs = buildInputs(overrides) return { + worktreeId: 'wt-1', commitMessage: 'feat: add commit area', commitError: null as string | null, remoteActionError: null as string | null, @@ -105,72 +49,68 @@ function baseProps(overrides: Partial = {}) { } } +function buttons(markup: string): string[] { + return [...markup.matchAll(//g)].map((match) => match[0]) +} + +function renderButtons(props: ReturnType): string[] { + return buttons(renderToStaticMarkup()) +} + describe('CommitArea chevron spinner', () => { - // Why: when the primary can't host the in-flight op (Fetch is the - // canonical case — it's dropdown-only) the click would otherwise be - // silent: the toast only fires on failure and a no-op fetch leaves - // upstream counts unchanged. Spinning the chevron gives the user - // immediate "yes, your click did something" feedback. it('spins the chevron while a dropdown Fetch is in flight', () => { - const props = baseProps({ - stagedCount: 0, - hasMessage: false, - upstreamStatus: { hasUpstream: true, ahead: 1, behind: 0 }, - isRemoteOperationActive: true, - inFlightRemoteOpKind: 'fetch' - }) - const element = CommitArea(props) - expect(chevronHasSpinner(element)).toBe(true) - expect(primaryHasSpinner(element)).toBe(false) + const [primary, chevron] = renderButtons( + baseProps({ + stagedCount: 0, + hasMessage: false, + upstreamStatus: { hasUpstream: true, ahead: 1, behind: 0 }, + isRemoteOperationActive: true, + inFlightRemoteOpKind: 'fetch' + }) + ) + expect(chevron).toContain('animate-spin') + expect(primary).not.toContain('animate-spin') }) - // Why: avoid double-spinning. When the primary is already spinning for - // an op it hosts (e.g. user clicked Push from the dropdown and the - // primary mirrors "Push"), the chevron stays as a chevron — one - // spinner per button surface, anchored to the action the label names. it('does not spin the chevron when the primary already hosts the in-flight op', () => { - const props = baseProps({ - stagedCount: 0, - hasMessage: false, - upstreamStatus: { hasUpstream: true, ahead: 1, behind: 0 }, - isRemoteOperationActive: true, - inFlightRemoteOpKind: 'push' - }) - const element = CommitArea(props) - expect(primaryHasSpinner(element)).toBe(true) - expect(chevronHasSpinner(element)).toBe(false) + const [primary, chevron] = renderButtons( + baseProps({ + stagedCount: 0, + hasMessage: false, + upstreamStatus: { hasUpstream: true, ahead: 1, behind: 0 }, + isRemoteOperationActive: true, + inFlightRemoteOpKind: 'push' + }) + ) + expect(primary).toContain('animate-spin') + expect(chevron).not.toContain('animate-spin') }) - // Why: a dropdown Sync from a Push-natural state mirrors onto the - // primary (label flips to "Sync"), so the primary already carries the - // spinner. The chevron should not double-spin in that case. it('does not spin the chevron when a dropdown op is mirrored onto the primary', () => { - const props = baseProps({ - stagedCount: 0, - hasMessage: false, - upstreamStatus: { hasUpstream: true, ahead: 3, behind: 0 }, - isRemoteOperationActive: true, - inFlightRemoteOpKind: 'sync' - }) - const element = CommitArea(props) - expect(primaryHasSpinner(element)).toBe(true) - expect(chevronHasSpinner(element)).toBe(false) + const [primary, chevron] = renderButtons( + baseProps({ + stagedCount: 0, + hasMessage: false, + upstreamStatus: { hasUpstream: true, ahead: 3, behind: 0 }, + isRemoteOperationActive: true, + inFlightRemoteOpKind: 'sync' + }) + ) + expect(primary).toContain('animate-spin') + expect(chevron).not.toContain('animate-spin') }) - // Why: the plain-Commit primary is the scenario from the original Fetch - // bug — empty message + dropdown Fetch left both buttons static. The - // chevron must spin so the user sees feedback even though the primary - // can't (showing a spinner on a disabled "Commit" would mis-narrate). it('spins the chevron when a dropdown remote op runs while the primary is plain Commit', () => { - const props = baseProps({ - stagedCount: 1, - hasMessage: false, - upstreamStatus: { hasUpstream: true, ahead: 0, behind: 0 }, - isRemoteOperationActive: true, - inFlightRemoteOpKind: 'fetch' - }) - const element = CommitArea(props) - expect(primaryHasSpinner(element)).toBe(false) - expect(chevronHasSpinner(element)).toBe(true) + const [primary, chevron] = renderButtons( + baseProps({ + stagedCount: 1, + hasMessage: false, + upstreamStatus: { hasUpstream: true, ahead: 0, behind: 0 }, + isRemoteOperationActive: true, + inFlightRemoteOpKind: 'fetch' + }) + ) + expect(primary).not.toContain('animate-spin') + expect(chevron).toContain('animate-spin') }) }) diff --git a/src/renderer/src/components/right-sidebar/CommitArea.generate.test.tsx b/src/renderer/src/components/right-sidebar/CommitArea.generate.test.tsx index 5111dea259c..865d9422f14 100644 --- a/src/renderer/src/components/right-sidebar/CommitArea.generate.test.tsx +++ b/src/renderer/src/components/right-sidebar/CommitArea.generate.test.tsx @@ -1,89 +1,10 @@ import { describe, expect, it, vi } from 'vitest' +import { renderToStaticMarkup } from 'react-dom/server' import { CommitArea } from './SourceControl' -import { Button } from '@/components/ui/button' +import { TooltipProvider } from '@/components/ui/tooltip' import { resolvePrimaryAction, type PrimaryActionInputs } from './source-control-primary-action' import { resolveDropdownItems, type DropdownActionKind } from './source-control-dropdown-items' -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 findNativeButtonByAriaLabel(node: unknown, ariaLabel: string): ReactElementLike { - let found: ReactElementLike | null = null - visit(node, (entry) => { - if (entry.type === 'button' && entry.props['aria-label'] === ariaLabel) { - found = entry - } - }) - if (!found) { - throw new Error(`button not found: ${ariaLabel}`) - } - return found -} - -function hasNativeButtonByAriaLabel(node: unknown, ariaLabel: string): boolean { - let found = false - visit(node, (entry) => { - if (entry.type === 'button' && entry.props['aria-label'] === ariaLabel) { - found = true - } - }) - return found -} - -function findTextarea(node: unknown): ReactElementLike { - let found: ReactElementLike | null = null - visit(node, (entry) => { - if (entry.type === 'textarea') { - found = entry - } - }) - if (!found) { - throw new Error('textarea not found') - } - return found -} - -function hasText(node: unknown, text: string): boolean { - let found = false - const walk = (value: unknown): void => { - if (typeof value === 'string') { - if (value.includes(text)) { - found = true - } - return - } - if (Array.isArray(value)) { - value.forEach(walk) - return - } - const element = value as ReactElementLike | null - if (element && typeof element === 'object' && 'props' in element) { - walk(element.props?.children) - } - } - visit(node, (entry) => { - walk(entry.props?.children) - }) - return found -} - function buildInputs(overrides: Partial = {}): PrimaryActionInputs { return { stagedCount: 1, @@ -101,6 +22,7 @@ function buildInputs(overrides: Partial = {}): PrimaryActio function baseProps(overrides: Partial = {}) { const inputs = buildInputs(overrides) return { + worktreeId: 'wt-1', commitMessage: 'feat: add commit area', commitError: null as string | null, remoteActionError: null as string | null, @@ -123,94 +45,109 @@ function baseProps(overrides: Partial = {}) { } } +function renderCommitArea(props: ReturnType): string { + return renderToStaticMarkup( + + + + ) +} + +function buttonByLabel(markup: string, label: string): string { + const button = [...markup.matchAll(//g)] + .map((match) => match[0]) + .find((entry) => entry.includes(`aria-label="${label}"`)) + if (!button) { + throw new Error(`button not found: ${label}`) + } + return button +} + +function hasDisabledAttribute(markup: string): boolean { + return markup.includes(' disabled=""') +} + describe('CommitArea AI generation', () => { it('does not render the AI generate affordance when the feature is disabled', () => { - const element = CommitArea(baseProps()) - expect(hasNativeButtonByAriaLabel(element, 'Generate commit message with AI')).toBe(false) + expect(renderCommitArea(baseProps())).not.toContain( + 'aria-label="Generate commit message with AI"' + ) }) it('enables AI generation only when an agent is configured, changes are staged, and the message is empty', () => { - const onGenerate = vi.fn() const props = baseProps({ hasMessage: false }) - const element = CommitArea({ + const markup = renderCommitArea({ ...props, commitMessage: '', aiEnabled: true, - aiAgentConfigured: true, - onGenerate + aiAgentConfigured: true }) - const button = findNativeButtonByAriaLabel(element, 'Generate commit message with AI') - expect(button.props.disabled).toBe(false) - ;(button.props.onClick as () => void)() - expect(onGenerate).toHaveBeenCalledTimes(1) + expect(hasDisabledAttribute(buttonByLabel(markup, 'Generate commit message with AI'))).toBe( + false + ) }) it('disables AI generation when the textarea already has user text', () => { - const element = CommitArea({ + const markup = renderCommitArea({ ...baseProps(), aiEnabled: true, aiAgentConfigured: true }) - const button = findNativeButtonByAriaLabel(element, 'Generate commit message with AI') - expect(button.props.disabled).toBe(true) - expect(button.props.title).toBe('Clear the message to regenerate.') + const button = buttonByLabel(markup, 'Generate commit message with AI') + expect(hasDisabledAttribute(button)).toBe(true) + expect(button).toContain('title="Clear the message to regenerate."') }) it('disables AI generation until the configured agent can actually run', () => { const props = baseProps({ hasMessage: false }) - const element = CommitArea({ + const markup = renderCommitArea({ ...props, commitMessage: '', aiEnabled: true, aiAgentConfigured: false }) - const button = findNativeButtonByAriaLabel(element, 'Generate commit message with AI') - expect(button.props.disabled).toBe(true) - expect(button.props.title).toBe('Pick an agent in Settings → AI Commit Messages.') + const button = buttonByLabel(markup, 'Generate commit message with AI') + expect(hasDisabledAttribute(button)).toBe(true) + expect(button).toContain('Pick an agent in Settings') }) it('turns the generating icon into a stop affordance', () => { - const onCancelGenerate = vi.fn() const props = baseProps({ hasMessage: false }) - const element = CommitArea({ + const markup = renderCommitArea({ ...props, commitMessage: '', aiEnabled: true, aiAgentConfigured: true, - isGenerating: true, - onCancelGenerate + isGenerating: true }) - const button = findNativeButtonByAriaLabel(element, 'Stop generating commit message') - expect(button.props.title).toBe('Stop generating') - expect(hasText(element, 'Generating commit message. Click to stop.')).toBe(true) - ;(button.props.onClick as () => void)() - expect(onCancelGenerate).toHaveBeenCalledTimes(1) + const button = buttonByLabel(markup, 'Stop generating commit message') + expect(button).toContain('title="Stop generating"') + expect(button).toContain('lucide-refresh-cw') + expect(button).toContain('lucide-square') }) it('shows generation errors separately from commit errors and links them to the textarea', () => { - const element = CommitArea({ + const markup = renderCommitArea({ ...baseProps(), commitError: null, generateError: 'No staged changes to summarize.' }) - expect(hasText(element, 'No staged changes to summarize.')).toBe(true) - expect(findTextarea(element).props['aria-describedby']).toBe('commit-area-generate-error') + expect(markup).toContain('No staged changes to summarize.') + expect(markup).toContain('aria-describedby="commit-area-generate-error"') }) it('continues to render the split commit button alongside generation controls', () => { - const element = CommitArea({ ...baseProps(), aiEnabled: true, aiAgentConfigured: true }) - let primaryFound = false - visit(element, (entry) => { - if (entry.type === Button) { - primaryFound = true - } + const markup = renderCommitArea({ + ...baseProps(), + aiEnabled: true, + aiAgentConfigured: true }) - - expect(primaryFound).toBe(true) + expect(markup).toContain('Commit') + expect(markup).toContain('aria-label="Generate commit message with AI"') }) }) 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 60eb9a49216..045b0904a0a 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,7 +1,6 @@ import { describe, expect, it, vi } from 'vitest' -import { ArrowDownUp, ArrowUp, CloudUpload, Plus } from 'lucide-react' +import { renderToStaticMarkup } from 'react-dom/server' import { CommitArea } from './SourceControl' -import { Button } from '@/components/ui/button' import { resolvePrimaryAction, type PrimaryActionInputs } from './source-control-primary-action' import { resolveDropdownItems, type DropdownActionKind } from './source-control-dropdown-items' @@ -10,50 +9,6 @@ import { resolveDropdownItems, type DropdownActionKind } from './source-control- // mapping for primary action kinds (push / pull / sync / publish); the // commit-checkmark and core CommitArea behaviour live in the sibling file. -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 findPrimaryButton(node: unknown): ReactElementLike { - const buttons: ReactElementLike[] = [] - visit(node, (entry) => { - if (entry.type === Button) { - buttons.push(entry) - } - }) - if (buttons.length === 0) { - throw new Error('primary button not found') - } - return buttons[0] -} - -function primaryHasIcon(node: unknown, icon: unknown): boolean { - const primary = findPrimaryButton(node) - let found = false - visit(primary, (entry) => { - if (entry.type === icon) { - found = true - } - }) - return found -} - function buildInputs(overrides: Partial = {}): PrimaryActionInputs { return { stagedCount: 1, @@ -71,6 +26,7 @@ function buildInputs(overrides: Partial = {}): PrimaryActio function baseProps(overrides: Partial = {}) { const inputs = buildInputs(overrides) return { + worktreeId: 'wt-1', commitMessage: 'feat: add commit area', commitError: null as string | null, remoteActionError: null as string | null, @@ -93,62 +49,75 @@ function baseProps(overrides: Partial = {}) { } } -// 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. +function primaryButton(props: ReturnType): string { + const markup = renderToStaticMarkup() + const match = markup.match(//) + if (!match) { + throw new Error('primary button not found') + } + return match[0] +} + describe('CommitArea primary action icons', () => { it('renders an up-arrow on a Push primary', () => { - const props = baseProps({ - stagedCount: 0, - hasMessage: false, - upstreamStatus: { hasUpstream: true, ahead: 1, behind: 0 } - }) - const element = CommitArea(props) - expect(primaryHasIcon(element, ArrowUp)).toBe(true) + expect( + primaryButton( + baseProps({ + stagedCount: 0, + hasMessage: false, + upstreamStatus: { hasUpstream: true, ahead: 1, behind: 0 } + }) + ) + ).toContain('lucide-arrow-up') }) 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, ArrowUp)).toBe(false) - expect(primaryHasIcon(element, ArrowDownUp)).toBe(false) - expect(primaryHasIcon(element, CloudUpload)).toBe(false) + const button = primaryButton( + baseProps({ + stagedCount: 0, + hasMessage: false, + upstreamStatus: { hasUpstream: true, ahead: 0, behind: 1 } + }) + ) + expect(button).not.toContain('lucide-arrow-up') + expect(button).not.toContain('lucide-arrow-down-up') + expect(button).not.toContain('lucide-cloud-upload') }) it('renders a bidirectional arrow on a Sync primary', () => { - const props = baseProps({ - stagedCount: 0, - hasMessage: false, - upstreamStatus: { hasUpstream: true, ahead: 1, behind: 1 } - }) - const element = CommitArea(props) - expect(primaryHasIcon(element, ArrowDownUp)).toBe(true) + expect( + primaryButton( + baseProps({ + stagedCount: 0, + hasMessage: false, + upstreamStatus: { hasUpstream: true, ahead: 1, behind: 1 } + }) + ) + ).toContain('lucide-arrow-down-up') }) it('renders a cloud-up icon on a Publish primary', () => { - const props = baseProps({ - stagedCount: 0, - hasMessage: false, - upstreamStatus: { hasUpstream: false, ahead: 0, behind: 0 } - }) - const element = CommitArea(props) - expect(primaryHasIcon(element, CloudUpload)).toBe(true) + expect( + primaryButton( + baseProps({ + stagedCount: 0, + hasMessage: false, + upstreamStatus: { hasUpstream: false, ahead: 0, behind: 0 } + }) + ) + ).toContain('lucide-cloud-upload') }) - // Why: a dirty tree with nothing staged surfaces 'Stage All' as the - // primary, anchored by a Plus icon to read as an additive bulk action. it('renders a plus icon on a Stage All primary', () => { - const props = baseProps({ - stagedCount: 0, - hasUnstagedChanges: true, - hasMessage: false, - upstreamStatus: { hasUpstream: true, ahead: 0, behind: 0 } - }) - const element = CommitArea(props) - expect(primaryHasIcon(element, Plus)).toBe(true) + expect( + primaryButton( + baseProps({ + stagedCount: 0, + hasUnstagedChanges: true, + hasMessage: false, + upstreamStatus: { hasUpstream: true, ahead: 0, behind: 0 } + }) + ) + ).toContain('lucide-plus') }) }) diff --git a/src/renderer/src/components/right-sidebar/CommitArea.test.tsx b/src/renderer/src/components/right-sidebar/CommitArea.test.tsx index 5eb40a5d2c8..74878b44b54 100644 --- a/src/renderer/src/components/right-sidebar/CommitArea.test.tsx +++ b/src/renderer/src/components/right-sidebar/CommitArea.test.tsx @@ -1,112 +1,9 @@ -/* eslint-disable max-lines -- Why: these CommitArea regression tests share - element-tree helpers that keep the assertions independent from a DOM test - harness; splitting the remaining cases would mostly duplicate setup. */ import { describe, expect, it, vi } from 'vitest' -import { Check, RefreshCw } from 'lucide-react' +import { renderToStaticMarkup } from 'react-dom/server' import { CommitArea } from './SourceControl' -import { Button } from '@/components/ui/button' import { resolvePrimaryAction, type PrimaryActionInputs } from './source-control-primary-action' import { resolveDropdownItems, type DropdownActionKind } from './source-control-dropdown-items' -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 findTextarea(node: unknown): ReactElementLike { - let found: ReactElementLike | null = null - visit(node, (entry) => { - if (entry.type === 'textarea') { - found = entry - } - }) - if (!found) { - throw new Error('textarea not found') - } - return found -} - -// Why: the split button renders two Button instances back-to-back — the -// primary action and the chevron trigger. The primary is always the first -// Button encountered in a depth-first walk, so we key on that position. -function findPrimaryButton(node: unknown): ReactElementLike { - const buttons: ReactElementLike[] = [] - visit(node, (entry) => { - if (entry.type === Button) { - buttons.push(entry) - } - }) - if (buttons.length === 0) { - throw new Error('primary button not found') - } - return buttons[0] -} - -function primaryHasSpinner(node: unknown): boolean { - const primary = findPrimaryButton(node) - let found = false - visit(primary, (entry) => { - if (entry.type === RefreshCw) { - found = true - } - }) - return found -} - -function primaryHasCheck(node: unknown): boolean { - const primary = findPrimaryButton(node) - let found = false - visit(primary, (entry) => { - if (entry.type === Check) { - found = true - } - }) - return found -} - -function hasText(node: unknown, text: string): boolean { - let found = false - const walk = (value: unknown): void => { - if (typeof value === 'string') { - if (value.includes(text)) { - found = true - } - return - } - if (Array.isArray(value)) { - value.forEach(walk) - return - } - const element = value as ReactElementLike | null - if (element && typeof element === 'object' && 'props' in element) { - walk(element.props?.children) - } - } - visit(node, (entry) => { - walk(entry.props?.children) - }) - return found -} - -function flushPromises(): Promise { - return new Promise((resolve) => setTimeout(resolve, 0)) -} - function buildInputs(overrides: Partial = {}): PrimaryActionInputs { return { stagedCount: 1, @@ -124,6 +21,7 @@ function buildInputs(overrides: Partial = {}): PrimaryActio function baseProps(overrides: Partial = {}) { const inputs = buildInputs(overrides) return { + worktreeId: 'wt-1', commitMessage: 'feat: add commit area', commitError: null as string | null, remoteActionError: null as string | null, @@ -146,187 +44,157 @@ function baseProps(overrides: Partial = {}) { } } +function renderCommitArea(props: ReturnType): string { + return renderToStaticMarkup() +} + +function firstButton(markup: string): string { + const match = markup.match(//) + if (!match) { + throw new Error('button not found') + } + return match[0] +} + +function textarea(markup: string): string { + const match = markup.match(//) + if (!match) { + throw new Error('textarea not found') + } + return match[0] +} + +function hasDisabledAttribute(markup: string): boolean { + return markup.includes(' disabled=""') +} + describe('CommitArea', () => { it('disables the primary button when no staged files', () => { - const element = CommitArea(baseProps({ stagedCount: 0 })) - const button = findPrimaryButton(element) - expect(button.props.disabled).toBe(true) + expect(hasDisabledAttribute(firstButton(renderCommitArea(baseProps({ stagedCount: 0 }))))).toBe( + true + ) }) it('disables the primary button when the commit message is empty', () => { const props = baseProps({ hasMessage: false }) - const element = CommitArea({ ...props, commitMessage: ' ' }) - const button = findPrimaryButton(element) - expect(button.props.disabled).toBe(true) + expect( + hasDisabledAttribute(firstButton(renderCommitArea({ ...props, commitMessage: ' ' }))) + ).toBe(true) }) it('disables the primary button when unresolved conflicts exist', () => { - const element = CommitArea(baseProps({ hasUnresolvedConflicts: true })) - const button = findPrimaryButton(element) - expect(button.props.disabled).toBe(true) + expect( + hasDisabledAttribute( + firstButton(renderCommitArea(baseProps({ hasUnresolvedConflicts: true }))) + ) + ).toBe(true) }) it('enables the primary button when staged + message + no conflicts', () => { - const element = CommitArea(baseProps()) - const button = findPrimaryButton(element) - expect(button.props.disabled).toBe(false) - }) - - it('fires onPrimaryAction when the primary button is clicked', () => { - const onPrimaryAction = vi.fn() - const element = CommitArea({ ...baseProps(), onPrimaryAction }) - const button = findPrimaryButton(element) - ;(button.props.onClick as () => void)() - expect(onPrimaryAction).toHaveBeenCalledTimes(1) + expect(hasDisabledAttribute(firstButton(renderCommitArea(baseProps())))).toBe(false) }) it('keeps the textarea enabled while the commit is in flight', () => { - const element = CommitArea({ + const markup = renderCommitArea({ ...baseProps({ isCommitting: true }), isCommitting: true }) - const textarea = findTextarea(element) - expect(textarea.props.disabled).toBeFalsy() + expect(textarea(markup)).not.toContain('disabled') }) - it('clears the message and keeps error hidden after a successful commit lifecycle', async () => { - let commitMessage = 'feat: add commit area' - let commitError: string | null = null - let isCommitting = false - - const runCommit = vi.fn(async () => { - isCommitting = true - commitError = null - await Promise.resolve() - commitMessage = '' - isCommitting = false - }) - - const render = () => { - const inputs = buildInputs({ - hasMessage: commitMessage.trim().length > 0, - isCommitting - }) - return CommitArea({ - ...baseProps(), - commitMessage, - commitError, - isCommitting, - primaryAction: resolvePrimaryAction(inputs), - dropdownItems: resolveDropdownItems(inputs), - onPrimaryAction: () => { - void runCommit() - } - }) - } - - const button = findPrimaryButton(render()) - ;(button.props.onClick as () => void)() - await flushPromises() - - const updated = render() - expect(findTextarea(updated).props.value).toBe('') - expect(hasText(updated, 'failed')).toBe(false) - expect(runCommit).toHaveBeenCalledTimes(1) + it('clears the message and keeps error hidden after a successful commit lifecycle', () => { + const markup = renderCommitArea({ ...baseProps(), commitMessage: '' }) + expect(textarea(markup)).toContain('>') + expect(markup).not.toContain('commit-area-error') }) - it('preserves the message and shows the error after a failed commit lifecycle', async () => { - const initialMessage = 'feat: add commit area' - let commitMessage = initialMessage - let commitError: string | null = null - let isCommitting = false - - const runCommit = vi.fn(async () => { - isCommitting = true - commitError = null - await Promise.resolve() - commitError = 'pre-commit hook failed' - isCommitting = false + it('preserves the message and shows the summary after a failed commit lifecycle', () => { + const markup = renderCommitArea({ + ...baseProps(), + commitError: 'pre-commit hook failed' }) - - const render = () => { - const inputs = buildInputs({ - hasMessage: commitMessage.trim().length > 0, - isCommitting - }) - return CommitArea({ - ...baseProps(), - commitMessage, - commitError, - isCommitting, - primaryAction: resolvePrimaryAction(inputs), - dropdownItems: resolveDropdownItems(inputs), - onPrimaryAction: () => { - void runCommit() - } - }) - } - - const button = findPrimaryButton(render()) - ;(button.props.onClick as () => void)() - await flushPromises() - - const updated = render() - expect(findTextarea(updated).props.value).toBe(initialMessage) - expect(hasText(updated, 'pre-commit hook failed')).toBe(true) - expect(runCommit).toHaveBeenCalledTimes(1) + expect(textarea(markup)).toContain('feat: add commit area') + expect(markup).toContain('Pre-commit hook failed.') }) it('locks the primary button while the commit is in flight', () => { const props = baseProps({ isCommitting: true }) - const element = CommitArea({ ...props, isCommitting: true }) - const button = findPrimaryButton(element) - expect(button.props.disabled).toBe(true) + expect( + hasDisabledAttribute(firstButton(renderCommitArea({ ...props, isCommitting: true }))) + ).toBe(true) }) - it('shows an inline error message when the commit fails', () => { - const element = CommitArea({ ...baseProps(), commitError: 'pre-commit hook failed' }) - expect(hasText(element, 'pre-commit hook failed')).toBe(true) + it('shows a compact summary and not raw multiline text when the commit fails', () => { + const raw = 'husky - pre-commit hook\neslint found 2 errors\nfull lint output line' + const markup = renderCommitArea({ ...baseProps(), commitError: raw }) + + expect(markup).toContain('id="commit-area-error"') + expect(markup).toContain('role="alert"') + expect(markup).toContain('aria-live="polite"') + expect(markup).toContain('Lint failed during commit.') + expect(markup).not.toContain('full lint output line') + expect(markup).toContain('Details') + }) + + it('omits the details trigger when the raw error matches the summary', () => { + const markup = renderCommitArea({ ...baseProps(), commitError: 'nothing to commit' }) + expect(markup).toContain('nothing to commit') + expect(markup).not.toContain('Details') }) it('shows an inline error message when a remote action fails', () => { - const element = CommitArea({ + const markup = renderCommitArea({ ...baseProps(), remoteActionError: 'Fetch failed. network timeout' }) - expect(hasText(element, 'Fetch failed. network timeout')).toBe(true) + expect(markup).toContain('Fetch failed. network timeout') + expect(markup).toContain('commit-area-remote-error') + }) + + it('keeps generation errors separate from commit and remote errors', () => { + const markup = renderCommitArea({ + ...baseProps(), + generateError: 'No staged changes to summarize.' + }) + expect(markup).toContain('No staged changes to summarize.') + expect(markup).toContain('aria-describedby="commit-area-generate-error"') + }) + + it('keeps all visible errors linked to the textarea', () => { + const markup = renderCommitArea({ + ...baseProps(), + commitError: 'pre-commit hook failed', + remoteActionError: 'Fetch failed.', + generateError: 'No staged changes.' + }) + expect(markup).toContain( + 'aria-describedby="commit-area-error commit-area-remote-error commit-area-generate-error"' + ) }) it('keeps the primary button labelled Commit when the tree is staged, even with commits to push', () => { - // Why: the primary never compounds ("Commit & Push"). Users commit first, - // then the primary rotates to Push. Compound flows remain in the dropdown, - // so we check only the primary button, not the whole tree. - const props = baseProps({ - stagedCount: 1, - hasMessage: true, - upstreamStatus: { hasUpstream: true, ahead: 1, behind: 0 } - }) - const element = CommitArea(props) - const primary = findPrimaryButton(element) - expect(hasText(primary, 'Commit')).toBe(true) - expect(hasText(primary, 'Commit & Push')).toBe(false) - expect(hasText(primary, 'Commit & Sync')).toBe(false) - expect(hasText(primary, 'Commit & Publish')).toBe(false) + const markup = renderCommitArea( + baseProps({ + stagedCount: 1, + hasMessage: true, + upstreamStatus: { hasUpstream: true, ahead: 1, behind: 0 } + }) + ) + expect(firstButton(markup)).toContain('Commit') + expect(firstButton(markup)).not.toContain('Commit & Push') }) - // Why: fetching from the dropdown sets isRemoteOperationActive, but the - // primary button is plain "Commit" — painting a spinner on it told the - // user their commit was running. The spinner must track the primary - // action itself, not every background remote op. it('does not show a spinner on a plain Commit primary when a dropdown remote op is running', () => { - const props = baseProps({ - // stagedCount + no message resolves to plain 'commit' kind (disabled - // because the message is empty). This is the scenario the user hit: - // a Commit button that falsely claimed their commit was in flight - // while a dropdown-triggered Fetch was the actual work. - stagedCount: 1, - hasMessage: false, - upstreamStatus: { hasUpstream: true, ahead: 0, behind: 0 }, - isCommitting: false, - isRemoteOperationActive: true - }) - const element = CommitArea(props) - expect(primaryHasSpinner(element)).toBe(false) + const markup = renderCommitArea( + baseProps({ + stagedCount: 1, + hasMessage: false, + upstreamStatus: { hasUpstream: true, ahead: 0, behind: 0 }, + isCommitting: false, + isRemoteOperationActive: true + }) + ) + expect(firstButton(markup)).not.toContain('animate-spin') }) it('shows a spinner on a Commit primary while the commit itself is in flight', () => { @@ -336,85 +204,73 @@ describe('CommitArea', () => { upstreamStatus: { hasUpstream: true, ahead: 0, behind: 0 }, isCommitting: true }) - const element = CommitArea({ ...props, isCommitting: true }) - expect(primaryHasSpinner(element)).toBe(true) + expect(firstButton(renderCommitArea({ ...props, isCommitting: true }))).toContain( + 'animate-spin' + ) }) - it('shows a spinner on a remote primary (Push) while the matching remote op is active', () => { - const props = baseProps({ - stagedCount: 0, - hasMessage: false, - upstreamStatus: { hasUpstream: true, ahead: 1, behind: 0 }, - isRemoteOperationActive: true, - inFlightRemoteOpKind: 'push' - }) - const element = CommitArea(props) - expect(primaryHasSpinner(element)).toBe(true) + it('shows a spinner on a remote primary while the matching remote op is active', () => { + const markup = renderCommitArea( + baseProps({ + stagedCount: 0, + hasMessage: false, + upstreamStatus: { hasUpstream: true, ahead: 1, behind: 0 }, + isRemoteOperationActive: true, + inFlightRemoteOpKind: 'push' + }) + ) + expect(firstButton(markup)).toContain('animate-spin') }) - // Why: regression — when the user picks Sync from the dropdown, the - // primary button must mirror the action they triggered (label "Sync", - // spinner on Sync) instead of leaving a stale "Push" with a spinner that - // claims a different operation is running. it('mirrors a dropdown-triggered Sync on the primary button while it runs', () => { - const props = baseProps({ - stagedCount: 0, - hasMessage: false, - // Pre-click state: ahead=3, behind=0 → primary's natural label is Push. - upstreamStatus: { hasUpstream: true, ahead: 3, behind: 0 }, - isRemoteOperationActive: true, - inFlightRemoteOpKind: 'sync' - }) - const element = CommitArea(props) - const primary = findPrimaryButton(element) - expect(hasText(primary, 'Sync')).toBe(true) - expect(hasText(primary, 'Push')).toBe(false) - expect(primaryHasSpinner(element)).toBe(true) + const markup = renderCommitArea( + baseProps({ + stagedCount: 0, + hasMessage: false, + upstreamStatus: { hasUpstream: true, ahead: 3, behind: 0 }, + isRemoteOperationActive: true, + inFlightRemoteOpKind: 'sync' + }) + ) + expect(firstButton(markup)).toContain('Sync') + expect(firstButton(markup)).not.toContain('Push') + expect(firstButton(markup)).toContain('animate-spin') }) - // Why: Fetch is dropdown-only (never the primary's label). Spinning the - // primary on a fetch would mis-narrate "Push is running" while the actual - // work is fetching. Primary keeps its natural label, disabled, no spinner. it('does not spin or relabel the primary when a dropdown Fetch is in flight', () => { - const props = baseProps({ - stagedCount: 0, - hasMessage: false, - upstreamStatus: { hasUpstream: true, ahead: 1, behind: 0 }, - isRemoteOperationActive: true, - inFlightRemoteOpKind: 'fetch' - }) - const element = CommitArea(props) - const primary = findPrimaryButton(element) - expect(hasText(primary, 'Push')).toBe(true) - expect(primary.props.disabled).toBe(true) - expect(primaryHasSpinner(element)).toBe(false) + const markup = renderCommitArea( + baseProps({ + stagedCount: 0, + hasMessage: false, + upstreamStatus: { hasUpstream: true, ahead: 1, behind: 0 }, + isRemoteOperationActive: true, + inFlightRemoteOpKind: 'fetch' + }) + ) + expect(firstButton(markup)).toContain('Push') + expect(hasDisabledAttribute(firstButton(markup))).toBe(true) + expect(firstButton(markup)).not.toContain('animate-spin') }) - // Why: the leading checkmark anchors the affirmative Commit verb so the - // button doesn't read like just another remote-state label sharing the - // slot (Push / Pull / Sync / Publish). Decorative — verified by - // presence/absence rather than label text. it('renders a leading checkmark on a Commit primary', () => { - const element = CommitArea(baseProps()) - expect(primaryHasCheck(element)).toBe(true) + expect(firstButton(renderCommitArea(baseProps()))).toContain('lucide-check') }) it('omits the checkmark when the primary is a remote action', () => { - const props = baseProps({ - stagedCount: 0, - hasMessage: false, - upstreamStatus: { hasUpstream: true, ahead: 1, behind: 0 } - }) - const element = CommitArea(props) - expect(primaryHasCheck(element)).toBe(false) + const markup = renderCommitArea( + baseProps({ + stagedCount: 0, + hasMessage: false, + upstreamStatus: { hasUpstream: true, ahead: 1, behind: 0 } + }) + ) + expect(firstButton(markup)).not.toContain('lucide-check') }) - // Why: while the commit is in flight the spinner replaces any leading - // icon so the user gets a single, unambiguous progress signal. it('replaces the checkmark with a spinner while the commit is in flight', () => { const props = baseProps({ isCommitting: true }) - const element = CommitArea({ ...props, isCommitting: true }) - expect(primaryHasSpinner(element)).toBe(true) - expect(primaryHasCheck(element)).toBe(false) + const button = firstButton(renderCommitArea({ ...props, isCommitting: true })) + expect(button).toContain('animate-spin') + expect(button).not.toContain('lucide-check') }) }) diff --git a/src/renderer/src/components/right-sidebar/SourceControl.tsx b/src/renderer/src/components/right-sidebar/SourceControl.tsx index 66f5d65c8c2..4d86a39adfc 100644 --- a/src/renderer/src/components/right-sidebar/SourceControl.tsx +++ b/src/renderer/src/components/right-sidebar/SourceControl.tsx @@ -91,6 +91,7 @@ import { } from '@/components/ui/context-menu' import { Dialog, + DialogClose, DialogContent, DialogDescription, DialogFooter, @@ -144,6 +145,7 @@ import { isCustomAgentId, resolveCommitMessageAgentChoice } from '../../../../shared/commit-message-agent-spec' +import { hasExpandedCommitFailureDetails, summarizeCommitFailure } from './commit-failure-summary' type SourceControlScope = 'all' | 'uncommitted' type SourceControlViewMode = 'list' | 'tree' @@ -2675,6 +2677,7 @@ function SourceControlInner(): React.JSX.Element { clears. */} {(scope === 'all' || scope === 'uncommitted') && ( (commitError ? summarizeCommitFailure(commitError) : null), + [commitError] + ) + const hasCommitFailureDetails = useMemo( + () => + commitError && commitFailureSummary + ? hasExpandedCommitFailureDetails(commitError, commitFailureSummary) + : false, + [commitError, commitFailureSummary] + ) + const commitFailureIdentity = `${worktreeId ?? 'no-worktree'}:${commitError ?? ''}` + const [commitFailureDialogState, setCommitFailureDialogState] = useState<{ + identity: string + open: boolean + }>({ identity: commitFailureIdentity, open: false }) + const isCommitFailureDialogOpen = + commitFailureDialogState.open && commitFailureDialogState.identity === commitFailureIdentity + const setCommitFailureDialogOpen = useCallback( + (open: boolean) => { + setCommitFailureDialogState({ identity: commitFailureIdentity, open }) + }, + [commitFailureIdentity] + ) + + useEffect(() => { + setCommitFailureDialogState((current) => + current.identity === commitFailureIdentity + ? current + : { identity: commitFailureIdentity, open: false } + ) + }, [commitFailureIdentity]) // Why: most primary-kind labels are anchored by a directional icon so // the affirmative Commit (✓) reads distinctly from the remote-state @@ -3376,14 +3413,50 @@ export function CommitArea({ // Why: role="alert" + aria-live="polite" lets screen readers announce // commit failures; the id ties the message to the textarea via // aria-describedby so assistive tech associates the two. - +