diff --git a/src/renderer/src/components/right-sidebar/ActionButton.test.tsx b/src/renderer/src/components/right-sidebar/ActionButton.test.tsx new file mode 100644 index 000000000..55353d298 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/ActionButton.test.tsx @@ -0,0 +1,151 @@ +import { describe, expect, it, vi } from 'vitest' +import { Plus } from 'lucide-react' +import { ActionButton } from './SourceControl' +import { Button } from '@/components/ui/button' + +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 findTooltipContentText(node: unknown): string { + const texts: string[] = [] + visit(node, (entry) => { + const typeName = + typeof entry.type === 'function' || typeof entry.type === 'object' + ? ((entry.type as { displayName?: string; name?: string }).displayName ?? + (entry.type as { displayName?: string; name?: string }).name ?? + '') + : '' + if (typeName === 'TooltipContent') { + const children = entry.props?.children + if (typeof children === 'string') { + texts.push(children) + } + } + }) + return texts.join(' ') +} + +// Why: ActionButton's onClick handler only touches these three fields, so a +// narrow event type is both correct and avoids a forbidden `as unknown as X` +// double-cast on the event value. +type MinimalMouseEvent = Pick< + React.MouseEvent, + 'preventDefault' | 'stopPropagation' | 'defaultPrevented' +> + +function makeClickEvent(): { + event: MinimalMouseEvent + preventDefault: ReturnType +} { + const preventDefault = vi.fn() + const event: MinimalMouseEvent = { + preventDefault, + stopPropagation: vi.fn(), + defaultPrevented: false + } + return { event, preventDefault } +} + +const baseProps = { + icon: Plus, + title: 'Stage all', + onClick: vi.fn() +} + +describe('ActionButton', () => { + it('forwards the title as aria-label on the inner button', () => { + const element = ActionButton({ ...baseProps, onClick: vi.fn() }) + const button = findInnerButton(element) + expect(button.props['aria-label']).toBe('Stage all') + }) + + it('renders the title as tooltip content (non-native, Radix)', () => { + const element = ActionButton({ ...baseProps, onClick: vi.fn() }) + // Why: the Radix TooltipContent is what renders the visual tooltip. + // Native `title` was removed because its styling diverges from the + // rest of the sidebar chrome. + expect(findTooltipContentText(element)).toContain('Stage all') + }) + + it('calls the onClick handler when enabled', () => { + const onClick = vi.fn() + const element = ActionButton({ ...baseProps, onClick }) + const button = findInnerButton(element) + const { event } = makeClickEvent() + ;(button.props.onClick as (e: MinimalMouseEvent) => void)(event) + expect(onClick).toHaveBeenCalledWith(event) + }) + + it('does NOT render the native disabled prop on the inner button', () => { + // Why: Radix TooltipTrigger on a DOM-disabled - ) : ( - - ) + <> + {/* Why: bulk action buttons are hover-only on + pointer devices to avoid cluttering the section + header with persistent icons. On no-hover + pointers (touch, and SSH sessions where hover + state is unreliable — see AGENTS.md "SSH Use + Case"), force them visible so they're reachable + without tabbing. One outer wrapper so that + focusing any action reveals all three siblings — + otherwise keyboard users tab into an invisible + next stop. */} +
+ {canRevertAll && ( + { + event.stopPropagation() + void handleRevertAllInArea(area) + }} + disabled={isExecutingBulk} + /> + )} + {canStageAll && ( + { + event.stopPropagation() + if (area === 'unstaged' || area === 'untracked') { + void handleStageAllInArea(area) + } + }} + disabled={isExecutingBulk} + /> + )} + {canUnstageAll && ( + { + event.stopPropagation() + void handleUnstageAll() + }} + disabled={isExecutingBulk} + /> + )} +
+ {items.some((entry) => entry.conflictStatus === 'unresolved') ? ( + + ) : ( + + )} + } /> {!isCollapsed && @@ -1420,25 +1621,31 @@ function SectionHeader({ onToggle: () => void actions?: React.ReactNode }): React.JSX.Element { + // Why: wrap the toggle button and actions in a shared rounded container + // so the hover background spans the entire row instead of clipping around + // the label. The outer div keeps the vertical spacing that separates + // sections; the inner wrapper owns the hover rectangle. return ( -
- -
{actions}
+
+
+ +
{actions}
+
) } @@ -1928,26 +2135,61 @@ function EmptyState({ ) } -function ActionButton({ +export function ActionButton({ icon: Icon, title, - onClick + onClick, + disabled }: { icon: React.ComponentType<{ className?: string }> title: string onClick: (event: React.MouseEvent) => void + disabled?: boolean }): React.JSX.Element { + // Why: use the Radix Tooltip instead of the native `title` attribute so the + // label matches the rest of the sidebar chrome (consistent styling, no OS + // delay quirks, dismissible on pointer leave). + // + // Why (no local TooltipProvider): the app root mounts a single + // TooltipProvider (see App.tsx); nesting another one here gives this subtree + // its own delay-timing state and breaks Radix's "skip the open delay when + // moving between adjacent tooltip triggers" handoff between sibling action + // buttons in the section header. + // + // Why (disabled handling): Radix's TooltipTrigger asChild on a disabled + // + + + + + + {title} + + ) } diff --git a/src/renderer/src/components/right-sidebar/discard-all-sequence.test.ts b/src/renderer/src/components/right-sidebar/discard-all-sequence.test.ts new file mode 100644 index 000000000..f7a2dcb5f --- /dev/null +++ b/src/renderer/src/components/right-sidebar/discard-all-sequence.test.ts @@ -0,0 +1,284 @@ +import { describe, expect, it, vi } from 'vitest' +import { + getDiscardAllPaths, + getStageAllPaths, + getUnstageAllPaths, + runDiscardAllForArea, + type DiscardAllArea +} from './discard-all-sequence' +import type { GitStatusEntry } from '../../../../shared/types' + +function entry(partial: Partial & { path: string }): GitStatusEntry { + return { + status: 'modified', + area: 'unstaged', + ...partial + } +} + +describe('getDiscardAllPaths', () => { + it('returns only paths in the requested area', () => { + const entries: GitStatusEntry[] = [ + entry({ path: 'a.ts', area: 'staged' }), + entry({ path: 'b.ts', area: 'unstaged' }), + entry({ path: 'c.ts', area: 'untracked', status: 'untracked' }) + ] + expect(getDiscardAllPaths(entries, 'staged')).toEqual(['a.ts']) + expect(getDiscardAllPaths(entries, 'unstaged')).toEqual(['b.ts']) + expect(getDiscardAllPaths(entries, 'untracked')).toEqual(['c.ts']) + }) + + it('skips entries with an unresolved conflict', () => { + const entries: GitStatusEntry[] = [ + entry({ path: 'clean.ts', area: 'unstaged' }), + entry({ + path: 'conflict.ts', + area: 'unstaged', + conflictKind: 'both_modified', + conflictStatus: 'unresolved' + }) + ] + // Why: `git restore --worktree --source=HEAD` on an unresolved conflict + // clears the `u` record silently before the user has reviewed it, which + // is why the per-row Stage/Discard buttons also suppress this case. + expect(getDiscardAllPaths(entries, 'unstaged')).toEqual(['clean.ts']) + }) + + it('skips entries resolved locally but not yet re-staged', () => { + const entries: GitStatusEntry[] = [ + entry({ path: 'clean.ts', area: 'unstaged' }), + entry({ + path: 'resolved.ts', + area: 'unstaged', + conflictKind: 'both_modified', + conflictStatus: 'resolved_locally' + }) + ] + // Why: discarding a locally-resolved file loses the resolution. The user + // would have to re-resolve from scratch — treat it as too dangerous to + // include in a bulk action. + expect(getDiscardAllPaths(entries, 'unstaged')).toEqual(['clean.ts']) + }) + + it('returns an empty array when nothing matches', () => { + expect(getDiscardAllPaths([], 'staged')).toEqual([]) + expect(getDiscardAllPaths([entry({ path: 'a.ts', area: 'staged' })], 'unstaged')).toEqual([]) + }) +}) + +describe('getStageAllPaths', () => { + it('returns only paths in the requested area', () => { + const entries: GitStatusEntry[] = [ + entry({ path: 'a.ts', area: 'staged' }), + entry({ path: 'b.ts', area: 'unstaged' }), + entry({ path: 'c.ts', area: 'untracked', status: 'untracked' }) + ] + expect(getStageAllPaths(entries, 'unstaged')).toEqual(['b.ts']) + expect(getStageAllPaths(entries, 'untracked')).toEqual(['c.ts']) + }) + + it('skips entries with an unresolved conflict', () => { + const entries: GitStatusEntry[] = [ + entry({ path: 'clean.ts', area: 'unstaged' }), + entry({ + path: 'conflict.ts', + area: 'unstaged', + conflictKind: 'both_modified', + conflictStatus: 'unresolved' + }) + ] + // Why: `git add` on an unresolved conflict silently clears the `u` + // record before the user has reviewed it — same hazard the per-row + // Stage button guards against. + expect(getStageAllPaths(entries, 'unstaged')).toEqual(['clean.ts']) + }) + + it('includes entries that are resolved locally', () => { + const entries: GitStatusEntry[] = [ + entry({ path: 'clean.ts', area: 'unstaged' }), + entry({ + path: 'resolved.ts', + area: 'unstaged', + conflictKind: 'both_modified', + conflictStatus: 'resolved_locally' + }) + ] + // Why: staging a locally-resolved file is the normal resolution + // workflow — it marks the conflict as finished. Unlike discard, this + // must NOT be filtered out. + expect(getStageAllPaths(entries, 'unstaged')).toEqual(['clean.ts', 'resolved.ts']) + }) + + it('returns an empty array when nothing matches', () => { + expect(getStageAllPaths([], 'unstaged')).toEqual([]) + expect(getStageAllPaths([entry({ path: 'a.ts', area: 'staged' })], 'unstaged')).toEqual([]) + }) +}) + +describe('getUnstageAllPaths', () => { + it('returns only staged-area paths', () => { + const entries: GitStatusEntry[] = [ + entry({ path: 'a.ts', area: 'staged' }), + entry({ path: 'b.ts', area: 'unstaged' }), + entry({ path: 'c.ts', area: 'untracked', status: 'untracked' }) + ] + expect(getUnstageAllPaths(entries)).toEqual(['a.ts']) + }) + + it('includes staged conflict rows', () => { + const entries: GitStatusEntry[] = [ + entry({ path: 'clean.ts', area: 'staged' }), + entry({ + path: 'conflict.ts', + area: 'staged', + conflictKind: 'both_modified', + conflictStatus: 'unresolved' + }), + entry({ + path: 'resolved.ts', + area: 'staged', + conflictKind: 'both_modified', + conflictStatus: 'resolved_locally' + }) + ] + // Why: `git reset HEAD` on a staged conflict row is safe and mirrors + // the per-row Unstage action — no conflict filter here. + expect(getUnstageAllPaths(entries)).toEqual(['clean.ts', 'conflict.ts', 'resolved.ts']) + }) + + it('returns an empty array when nothing is staged', () => { + expect(getUnstageAllPaths([])).toEqual([]) + expect(getUnstageAllPaths([entry({ path: 'a.ts', area: 'unstaged' })])).toEqual([]) + }) +}) + +describe('runDiscardAllForArea', () => { + function makeDeps( + overrides: { + bulkUnstageError?: unknown + discardOneError?: (path: string) => unknown + } = {} + ) { + const bulkUnstageCalls: string[][] = [] + const discardOneCalls: string[] = [] + const errors: unknown[] = [] + + const bulkUnstage = vi.fn(async (paths: string[]) => { + bulkUnstageCalls.push([...paths]) + if (overrides.bulkUnstageError !== undefined) { + throw overrides.bulkUnstageError + } + }) + const discardOne = vi.fn(async (path: string) => { + discardOneCalls.push(path) + if (overrides.discardOneError) { + const err = overrides.discardOneError(path) + if (err !== undefined) { + throw err + } + } + }) + const onError = vi.fn((error: unknown) => { + errors.push(error) + }) + + return { + deps: { bulkUnstage, discardOne, onError }, + bulkUnstageCalls, + discardOneCalls, + errors, + bulkUnstage, + discardOne, + onError + } + } + + it('no-ops when the path list is empty', async () => { + const ctx = makeDeps() + const result = await runDiscardAllForArea('staged', [], ctx.deps) + expect(result).toEqual({ discarded: [], failed: [], aborted: false }) + expect(ctx.bulkUnstage).not.toHaveBeenCalled() + expect(ctx.discardOne).not.toHaveBeenCalled() + }) + + it('discards unstaged paths one-by-one without bulk-unstaging', async () => { + const ctx = makeDeps() + const result = await runDiscardAllForArea('unstaged', ['a.ts', 'b.ts'], ctx.deps) + expect(result).toEqual({ discarded: ['a.ts', 'b.ts'], failed: [], aborted: false }) + expect(ctx.bulkUnstage).not.toHaveBeenCalled() + expect(ctx.discardOneCalls).toEqual(['a.ts', 'b.ts']) + }) + + it('discards untracked paths one-by-one without bulk-unstaging', async () => { + const ctx = makeDeps() + const result = await runDiscardAllForArea('untracked', ['new.ts'], ctx.deps) + expect(result).toEqual({ discarded: ['new.ts'], failed: [], aborted: false }) + expect(ctx.bulkUnstage).not.toHaveBeenCalled() + expect(ctx.discardOneCalls).toEqual(['new.ts']) + }) + + it('bulk-unstages staged paths before the per-file discard loop', async () => { + const ctx = makeDeps() + const result = await runDiscardAllForArea('staged', ['a.ts', 'b.ts'], ctx.deps) + expect(result).toEqual({ discarded: ['a.ts', 'b.ts'], failed: [], aborted: false }) + expect(ctx.bulkUnstageCalls).toEqual([['a.ts', 'b.ts']]) + expect(ctx.discardOneCalls).toEqual(['a.ts', 'b.ts']) + // Why: bulk unstage MUST happen strictly before any discard, otherwise + // the index would still hold the staged delta when the worktree was + // reset and the files would reappear as inverse changes. + expect(ctx.bulkUnstage.mock.invocationCallOrder[0]).toBeLessThan( + ctx.discardOne.mock.invocationCallOrder[0] + ) + }) + + it('aborts and skips the discard loop if bulk-unstage rejects', async () => { + const error = new Error('index locked') + const ctx = makeDeps({ bulkUnstageError: error }) + const result = await runDiscardAllForArea('staged', ['a.ts', 'b.ts'], ctx.deps) + expect(result).toEqual({ discarded: [], failed: [], aborted: true }) + // Why: a failed unstage + successful discard would leave the index with + // the staged delta and the worktree at HEAD — a worse state than we + // started in. The discard loop must not run. + expect(ctx.discardOne).not.toHaveBeenCalled() + expect(ctx.errors).toEqual([error]) + }) + + it('continues past a per-file discard failure and records it in `failed`', async () => { + const error = new Error('EPERM') + const ctx = makeDeps({ + discardOneError: (path) => (path === 'b.ts' ? error : undefined) + }) + const result = await runDiscardAllForArea('unstaged', ['a.ts', 'b.ts', 'c.ts'], ctx.deps) + // Why: best-effort continuation — one stuck file shouldn't block the + // rest of a bulk action the user explicitly triggered. + expect(result).toEqual({ + discarded: ['a.ts', 'c.ts'], + failed: ['b.ts'], + aborted: false + }) + expect(ctx.discardOneCalls).toEqual(['a.ts', 'b.ts', 'c.ts']) + // Why: `aborted` is reserved for the pre-step (bulk unstage) failing — + // per-file failures don't trip it, otherwise callers couldn't + // distinguish "nothing ran" from "some ran, some didn't". + expect(ctx.errors).toEqual([error]) + }) + + it('does not invoke the error callback on a happy-path staged run', async () => { + const ctx = makeDeps() + await runDiscardAllForArea('staged', ['a.ts'], ctx.deps) + expect(ctx.onError).not.toHaveBeenCalled() + }) + + it('does not bulk-unstage for non-staged areas even if the dep is provided', async () => { + const ctx = makeDeps() + const areas: DiscardAllArea[] = ['unstaged', 'untracked'] + for (const area of areas) { + await runDiscardAllForArea(area, ['x.ts'], ctx.deps) + } + // Why: the unstage step is specific to the staged area's two-step + // reset. Accidentally invoking it for unstaged/untracked would be a + // no-op for unstaged entries but could mask a regression where staged + // entries leak into those paths. + expect(ctx.bulkUnstage).not.toHaveBeenCalled() + }) +}) diff --git a/src/renderer/src/components/right-sidebar/discard-all-sequence.ts b/src/renderer/src/components/right-sidebar/discard-all-sequence.ts new file mode 100644 index 000000000..9425344bc --- /dev/null +++ b/src/renderer/src/components/right-sidebar/discard-all-sequence.ts @@ -0,0 +1,122 @@ +import type { GitStatusEntry } from '../../../../shared/types' + +export type DiscardAllArea = 'staged' | 'unstaged' | 'untracked' + +/** + * Collect the paths a "Discard all" bulk action should operate on for a given + * area. Unresolved and locally-resolved conflicts are excluded — discarding + * those can silently re-create the conflict or lose the resolution. + */ +export function getDiscardAllPaths( + entries: readonly GitStatusEntry[], + area: DiscardAllArea +): string[] { + return entries + .filter( + (entry) => + entry.area === area && + entry.conflictStatus !== 'unresolved' && + entry.conflictStatus !== 'resolved_locally' + ) + .map((entry) => entry.path) +} + +export type StageAllArea = 'unstaged' | 'untracked' + +/** + * Collect the paths a "Stage all" action should operate on. + * Unresolved conflict rows are excluded — `git add` on a conflicted file + * silently clears the `u` record before the user has reviewed it. + * `resolved_locally` rows are intentionally INCLUDED: staging them is how the + * user finalises the resolution and mirrors the per-row Stage button. + */ +export function getStageAllPaths(entries: readonly GitStatusEntry[], area: StageAllArea): string[] { + return entries + .filter((entry) => entry.area === area && entry.conflictStatus !== 'unresolved') + .map((entry) => entry.path) +} + +/** + * Collect the paths an "Unstage all" action should operate on. + * Every staged row is eligible — `git reset HEAD` on a staged conflict + * row is safe and mirrors the per-row Unstage action. + */ +export function getUnstageAllPaths(entries: readonly GitStatusEntry[]): string[] { + return entries.filter((entry) => entry.area === 'staged').map((entry) => entry.path) +} + +export type DiscardAllDeps = { + /** Unstage the given paths in one IPC round-trip. Only called for 'staged'. */ + bulkUnstage: (paths: string[]) => Promise + /** Discard a single path (restore working-tree to HEAD, or rm if untracked). */ + discardOne: (path: string) => Promise + /** + * Called when either the pre-step (bulkUnstage) rejects OR an individual + * `discardOne` rejects. Invoked once per failure so callers can surface + * each error (e.g. a toast per stuck file) rather than swallowing them. + */ + onError?: (error: unknown) => void +} + +export type DiscardAllResult = { + /** Paths whose `discardOne` call resolved successfully. */ + discarded: string[] + /** Paths whose `discardOne` call rejected. Best-effort: the loop continues past these. */ + failed: string[] + /** + * True only when the pre-step (bulk unstage for the 'staged' area) failed + * and we never entered the per-file discard loop. Per-file failures do + * NOT set this flag — they are reported via `failed`. + */ + aborted: boolean +} + +/** + * Run the "Discard all" sequence for a given area. + * + * For 'staged', this first bulk-unstages the paths — without that step, + * `discardOne` (which maps to `git restore --worktree --source=HEAD`) would + * reset the working tree to HEAD but leave the index carrying the staged + * delta, producing phantom inverse "Changes" rows the user thought they just + * discarded. If the unstage fails we MUST skip the discard loop entirely for + * the same reason: a stale index with a clean worktree is a worse state than + * the one the user started in. + * + * Per-file `discardOne` failures are best-effort: we continue past a failed + * file so a single stuck path does not block the rest of the bulk action. + * Failed paths are reported via `failed`, `onError` is invoked once per + * failure, and `aborted` remains `false` because the loop ran end-to-end. + */ +export async function runDiscardAllForArea( + area: DiscardAllArea, + paths: readonly string[], + deps: DiscardAllDeps +): Promise { + if (paths.length === 0) { + return { discarded: [], failed: [], aborted: false } + } + + if (area === 'staged') { + try { + await deps.bulkUnstage([...paths]) + } catch (error) { + deps.onError?.(error) + return { discarded: [], failed: [], aborted: true } + } + } + + const discarded: string[] = [] + const failed: string[] = [] + for (const path of paths) { + try { + await deps.discardOne(path) + discarded.push(path) + } catch (error) { + // Best-effort: record and continue so one stuck file doesn't block the + // rest of the bulk action. + failed.push(path) + deps.onError?.(error) + } + } + return { discarded, failed, aborted: false } +}