From f3c4adba946353a4f0c43ef39cae2353750df2b8 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 3 May 2026 17:14:39 -0700 Subject: [PATCH 001/277] fix(sidebar): preserve scroll position across worktree delete (#1380) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deleting an unselected, scrolled-past worktree snapped the sidebar back toward the selected item. The virtualizer's viewport key includes row count so it can remount cleanly when rows change (avoiding stale height measurements that caused card overlap) — but the fresh mount started at scrollTop 0. Persist the scroll offset in a parent-owned ref, seed the new virtualizer via initialOffset, and re-apply scrollTop in a useLayoutEffect with a retry loop that tolerates the transient window where the browser clamps scrollTop while the virtualizer is still measuring rows. Co-authored-by: Orca --- .../src/components/sidebar/WorktreeList.tsx | 69 ++++++++++++++++++- 1 file changed, 68 insertions(+), 1 deletion(-) diff --git a/src/renderer/src/components/sidebar/WorktreeList.tsx b/src/renderer/src/components/sidebar/WorktreeList.tsx index e3045d9e200..4d90a4ac37f 100644 --- a/src/renderer/src/components/sidebar/WorktreeList.tsx +++ b/src/renderer/src/components/sidebar/WorktreeList.tsx @@ -81,6 +81,13 @@ type VirtualizedWorktreeViewportProps = { worktrees: Worktree[] repoMap: Map prCache: Record | null + // Why: the viewport remounts when the row structure changes (see + // viewportResetKey) so the virtualizer's measurementsCache cannot hold + // heights tied to shifted indices. A fresh virtualizer would otherwise + // start at scrollTop 0, which makes the sidebar snap to the top whenever + // a worktree is deleted. The parent persists the last observed scrollTop + // in a ref and seeds the new virtualizer via initialOffset. + scrollOffsetRef: React.MutableRefObject } const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewport({ @@ -96,7 +103,8 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp clearPendingRevealWorktreeId, worktrees, repoMap, - prCache + prCache, + scrollOffsetRef }: VirtualizedWorktreeViewportProps) { const scrollRef = useRef(null) const activeWorktreeRowIndex = useMemo( @@ -110,6 +118,12 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp estimateSize: () => 120, overscan: 10, gap: 6, + // Why: tells the virtualizer to start its internal scrollOffset at the + // ref value rather than 0, so the first getVirtualItems() call after + // remount picks the correct window of rows. The sibling useLayoutEffect + // mirrors this onto the actual scrollElement.scrollTop so the DOM and + // virtualizer stay aligned across remounts. + initialOffset: () => scrollOffsetRef.current, getItemKey: (index) => { const row = rows[index] if (!row) { @@ -119,6 +133,51 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp } }) + // Why: the viewport remounts when row structure changes (see + // viewportResetKey). The fresh DOM element starts at scrollTop=0, which + // snaps the sidebar back to the top every time a worktree is added or + // deleted. Restoring the last observed scrollTop from a ref before the + // browser paints keeps the user's scroll position stable across remounts. + // + // The saved offset is only captured via our scroll listener; we + // suppress saving during the initial restoration pass so that the + // browser's clamp-to-current-scrollHeight (temporarily smaller because + // the virtualizer has not yet measured every row) doesn't overwrite the + // user's intended offset with a clamped value. + useLayoutEffect(() => { + const el = scrollRef.current + if (!el) { + return + } + const targetOffset = scrollOffsetRef.current + let restoring = targetOffset > 0 + if (restoring) { + el.scrollTop = targetOffset + } + const onScroll = (): void => { + if (restoring) { + // Virtualizer has not yet produced its final totalSize, so the + // browser may clamp our applied scrollTop to a lower value. Keep + // re-applying the target offset each tick until the DOM accepts + // it, then start recording user-driven scrolls. + if (el.scrollTop === targetOffset) { + restoring = false + return + } + if (el.scrollHeight - el.clientHeight >= targetOffset) { + el.scrollTop = targetOffset + if (el.scrollTop === targetOffset) { + restoring = false + } + } + return + } + scrollOffsetRef.current = el.scrollTop + } + el.addEventListener('scroll', onScroll, { passive: true }) + return () => el.removeEventListener('scroll', onScroll) + }, [scrollOffsetRef]) + React.useEffect(() => { if (!pendingRevealWorktreeId) { return @@ -436,6 +495,9 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp }) const WorktreeList = React.memo(function WorktreeList() { + // Why: persists the sidebar scroll offset across the VirtualizedWorktreeViewport + // remount that row-structure changes trigger. See viewportResetKey. + const sidebarScrollOffsetRef = useRef(0) // ── Granular selectors (each is a primitive or shallow-stable ref) ── const allWorktrees = useAllWorktrees() const repoMap = useRepoMap() @@ -678,6 +740,10 @@ const WorktreeList = React.memo(function WorktreeList() { // an item while its header is added — net row count unchanged). Including // the header keys ensures the virtualizer remounts when group structure // changes, preventing stale height measurements from causing overlap. + // We also key on rows.length so add/delete invalidates the virtualizer's + // per-index measurementsCache; scroll position is preserved across the + // remount via the ref below so deleting an off-screen worktree doesn't + // snap the sidebar back to the top. const viewportResetKey = useMemo(() => { const headers = rows .filter((r): r is GroupHeaderRow => r.type === 'header') @@ -791,6 +857,7 @@ const WorktreeList = React.memo(function WorktreeList() { worktrees={worktrees} repoMap={repoMap} prCache={prCache} + scrollOffsetRef={sidebarScrollOffsetRef} /> ) }) From 9802a7f59f631cd0bb298c523f93ae989c26e408 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sun, 3 May 2026 17:19:52 -0700 Subject: [PATCH 002/277] feat(source-control): commit action in sidebar panel (#1373) Co-authored-by: Orca Co-authored-by: Alexander Saavedra --- config/vitest.config.ts | 2 +- src/main/git/commit.test.ts | 81 +++++ src/main/git/status.ts | 28 ++ src/main/ipc/filesystem.test.ts | 105 ++++++- src/main/ipc/filesystem.ts | 23 ++ src/main/providers/ssh-git-provider.test.ts | 13 + src/main/providers/ssh-git-provider.ts | 10 + src/main/providers/types.ts | 1 + src/preload/api-types.ts | 12 +- src/preload/index.ts | 5 + src/relay/git-handler-staging.test.ts | 126 ++++++++ src/relay/git-handler-status-ops.ts | 77 +++++ src/relay/git-handler-test-setup.ts | 76 +++++ src/relay/git-handler-worktree-ops.ts | 110 +++++++ src/relay/git-handler.test.ts | 98 +----- src/relay/git-handler.ts | 137 ++------- .../right-sidebar/CommitArea.test.tsx | 188 +++++++++++ .../SourceControl.commit-drafts.test.ts | 22 ++ .../right-sidebar/SourceControl.tsx | 291 +++++++++++++++++- 19 files changed, 1179 insertions(+), 226 deletions(-) create mode 100644 src/main/git/commit.test.ts create mode 100644 src/relay/git-handler-staging.test.ts create mode 100644 src/relay/git-handler-status-ops.ts create mode 100644 src/relay/git-handler-test-setup.ts create mode 100644 src/relay/git-handler-worktree-ops.ts create mode 100644 src/renderer/src/components/right-sidebar/CommitArea.test.tsx create mode 100644 src/renderer/src/components/right-sidebar/SourceControl.commit-drafts.test.ts diff --git a/config/vitest.config.ts b/config/vitest.config.ts index ac207b5364b..bee38dc32c9 100644 --- a/config/vitest.config.ts +++ b/config/vitest.config.ts @@ -10,6 +10,6 @@ export default defineConfig({ }, test: { environment: 'node', - include: ['src/**/*.test.ts'] + include: ['src/**/*.test.ts', 'src/**/*.test.tsx'] } }) diff --git a/src/main/git/commit.test.ts b/src/main/git/commit.test.ts new file mode 100644 index 00000000000..465488d268f --- /dev/null +++ b/src/main/git/commit.test.ts @@ -0,0 +1,81 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { gitExecFileAsyncMock } = vi.hoisted(() => ({ + gitExecFileAsyncMock: vi.fn() +})) + +vi.mock('./runner', () => ({ + gitExecFileAsync: gitExecFileAsyncMock, + gitExecFileAsyncBuffer: vi.fn() +})) + +import { commitChanges } from './status' + +describe('commitChanges', () => { + beforeEach(() => { + gitExecFileAsyncMock.mockReset() + }) + + it('returns success when git commit completes', async () => { + gitExecFileAsyncMock.mockResolvedValue({ stdout: '[main abc123] message\n', stderr: '' }) + + const result = await commitChanges('/repo', 'feat: add commit action') + + expect(gitExecFileAsyncMock).toHaveBeenCalledWith(['commit', '-m', 'feat: add commit action'], { + cwd: '/repo' + }) + expect(result).toEqual({ success: true }) + }) + + it('returns stderr when commit fails (e.g. pre-commit hook)', async () => { + gitExecFileAsyncMock.mockRejectedValue({ + stderr: 'pre-commit hook failed: lint errors\n' + }) + + const result = await commitChanges('/repo', 'feat: commit with lint errors') + + expect(result).toEqual({ + success: false, + error: 'pre-commit hook failed: lint errors\n' + }) + }) + + it('returns stdout when git writes to stdout (e.g. nothing to commit)', async () => { + gitExecFileAsyncMock.mockRejectedValue({ + stdout: 'nothing to commit, working tree clean\n', + stderr: '' + }) + + const result = await commitChanges('/repo', 'feat: empty commit') + + expect(result).toEqual({ + success: false, + error: 'nothing to commit, working tree clean\n' + }) + }) + + it('prefers stderr over stdout when both are present', async () => { + gitExecFileAsyncMock.mockRejectedValue({ + stdout: 'some stdout output\n', + stderr: 'hook rejected\n' + }) + + const result = await commitChanges('/repo', 'feat: both channels') + + expect(result).toEqual({ + success: false, + error: 'hook rejected\n' + }) + }) + + it('falls back to Error.message when stdout/stderr are empty', async () => { + gitExecFileAsyncMock.mockRejectedValue(new Error('spawn git ENOENT')) + + const result = await commitChanges('/repo', 'feat: missing git') + + expect(result).toEqual({ + success: false, + error: 'spawn git ENOENT' + }) + }) +}) diff --git a/src/main/git/status.ts b/src/main/git/status.ts index 4c9345fc182..f05c510bdf7 100644 --- a/src/main/git/status.ts +++ b/src/main/git/status.ts @@ -659,6 +659,34 @@ export async function unstageFile(worktreePath: string, filePath: string): Promi await gitExecFileAsync(['restore', '--staged', '--', filePath], { cwd: worktreePath }) } +export async function commitChanges( + worktreePath: string, + message: string +): Promise<{ success: boolean; error?: string }> { + try { + await gitExecFileAsync(['commit', '-m', message], { cwd: worktreePath }) + return { success: true } + } catch (error) { + // Why: surface whichever channel carries the useful message. Pre-commit/GPG + // hook failures write to stderr; "nothing to commit, working tree clean" + // writes to stdout. Try stderr first, fall back to stdout, then error.message. + const readStringField = (field: string): string | null => { + if (typeof error === 'object' && error && field in error) { + const v = (error as Record)[field] + if (typeof v === 'string' && v.length > 0) { + return v + } + } + return null + } + const errorMessage = + readStringField('stderr') ?? + readStringField('stdout') ?? + (error instanceof Error ? error.message : 'Commit failed') + return { success: false, error: errorMessage } + } +} + /** * Discard working tree changes for a file. */ diff --git a/src/main/ipc/filesystem.test.ts b/src/main/ipc/filesystem.test.ts index 6c68a269b80..8898a5220c7 100644 --- a/src/main/ipc/filesystem.test.ts +++ b/src/main/ipc/filesystem.test.ts @@ -13,6 +13,7 @@ const { openMock, realpathMock, lstatMock, + commitChangesMock, getStatusMock, getDiffMock, getBranchCompareMock, @@ -23,7 +24,8 @@ const { bulkUnstageFilesMock, discardChangesMock, listWorktreesMock, - getSshFilesystemProviderMock + getSshFilesystemProviderMock, + getSshGitProviderMock } = vi.hoisted(() => ({ handleMock: vi.fn(), trashItemMock: vi.fn(), @@ -34,6 +36,7 @@ const { openMock: vi.fn(), realpathMock: vi.fn(), lstatMock: vi.fn(), + commitChangesMock: vi.fn(), getStatusMock: vi.fn(), getDiffMock: vi.fn(), getBranchCompareMock: vi.fn(), @@ -44,7 +47,8 @@ const { bulkUnstageFilesMock: vi.fn(), discardChangesMock: vi.fn(), listWorktreesMock: vi.fn(), - getSshFilesystemProviderMock: vi.fn() + getSshFilesystemProviderMock: vi.fn(), + getSshGitProviderMock: vi.fn() })) vi.mock('electron', () => ({ @@ -67,6 +71,7 @@ vi.mock('fs/promises', () => ({ })) vi.mock('../git/status', () => ({ + commitChanges: commitChangesMock, getStatus: getStatusMock, getDiff: getDiffMock, getBranchCompare: getBranchCompareMock, @@ -87,7 +92,7 @@ vi.mock('../providers/ssh-filesystem-dispatch', () => ({ })) vi.mock('../providers/ssh-git-dispatch', () => ({ - getSshGitProvider: vi.fn().mockReturnValue(null) + getSshGitProvider: getSshGitProviderMock })) import { registerFilesystemHandlers } from './filesystem' @@ -148,6 +153,7 @@ describe('registerFilesystemHandlers', () => { openMock, realpathMock, lstatMock, + commitChangesMock, getStatusMock, getDiffMock, getBranchCompareMock, @@ -158,7 +164,8 @@ describe('registerFilesystemHandlers', () => { bulkUnstageFilesMock, discardChangesMock, listWorktreesMock, - getSshFilesystemProviderMock + getSshFilesystemProviderMock, + getSshGitProviderMock ]) { mock.mockReset() } @@ -182,6 +189,7 @@ describe('registerFilesystemHandlers', () => { } ]) trashItemMock.mockResolvedValue(undefined) + getSshGitProviderMock.mockReturnValue(null) statMock.mockResolvedValue({ size: 10, isDirectory: () => false, mtimeMs: 123 }) openMock.mockResolvedValue({ read: vi.fn(async (buffer: Buffer) => { @@ -545,6 +553,95 @@ describe('registerFilesystemHandlers', () => { expect(getBranchCompareMock).toHaveBeenCalledWith(WORKTREE_FEATURE_PATH, 'origin/main') }) + it('routes local git:commit through commitChanges and returns success', async () => { + commitChangesMock.mockResolvedValue({ success: true }) + + registerFilesystemHandlers(store as never) + + await expect( + handlers.get('git:commit')!(null, { + worktreePath: WORKTREE_FEATURE_PATH, + message: 'feat: ship commit' + }) + ).resolves.toEqual({ success: true }) + + expect(commitChangesMock).toHaveBeenCalledWith(WORKTREE_FEATURE_PATH, 'feat: ship commit') + }) + + it('returns local commit hook failure payload from git:commit', async () => { + commitChangesMock.mockResolvedValue({ success: false, error: 'hook failed' }) + + registerFilesystemHandlers(store as never) + + await expect( + handlers.get('git:commit')!(null, { + worktreePath: WORKTREE_FEATURE_PATH, + message: 'feat: ship commit' + }) + ).resolves.toEqual({ success: false, error: 'hook failed' }) + }) + + it('routes ssh git:commit through the SSH provider instead of local commitChanges', async () => { + const sshCommitMock = vi.fn().mockResolvedValue({ success: true }) + getSshGitProviderMock.mockReturnValue({ commit: sshCommitMock }) + + registerFilesystemHandlers(store as never) + + await expect( + handlers.get('git:commit')!(null, { + worktreePath: '/remote/repo', + message: 'feat: remote commit', + connectionId: 'conn-1' + }) + ).resolves.toEqual({ success: true }) + + expect(sshCommitMock).toHaveBeenCalledWith('/remote/repo', 'feat: remote commit') + expect(commitChangesMock).not.toHaveBeenCalled() + }) + + it('rejects git:commit with empty message and does not call commitChanges', async () => { + registerFilesystemHandlers(store as never) + + await expect( + handlers.get('git:commit')!(null, { + worktreePath: WORKTREE_FEATURE_PATH, + message: '' + }) + ).rejects.toThrow('Commit message is required') + + expect(commitChangesMock).not.toHaveBeenCalled() + }) + + it('rejects git:commit with whitespace-only message and does not call commitChanges', async () => { + registerFilesystemHandlers(store as never) + + await expect( + handlers.get('git:commit')!(null, { + worktreePath: WORKTREE_FEATURE_PATH, + message: ' ' + }) + ).rejects.toThrow('Commit message is required') + + expect(commitChangesMock).not.toHaveBeenCalled() + }) + + it('rejects git:commit with whitespace-only message before SSH dispatch', async () => { + const sshCommitMock = vi.fn().mockResolvedValue({ success: true }) + getSshGitProviderMock.mockReturnValue({ commit: sshCommitMock }) + + registerFilesystemHandlers(store as never) + + await expect( + handlers.get('git:commit')!(null, { + worktreePath: '/remote/repo', + message: '\n', + connectionId: 'conn-1' + }) + ).rejects.toThrow('Commit message is required') + + expect(sshCommitMock).not.toHaveBeenCalled() + }) + it('allows git operations on worktrees outside repo/workspace roots', async () => { // Linked worktrees can live anywhere on disk (e.g. ~/.codex/worktrees/). // As long as the path matches a worktree reported by `git worktree list` diff --git a/src/main/ipc/filesystem.ts b/src/main/ipc/filesystem.ts index 756e702c9dd..7f91ef70a31 100644 --- a/src/main/ipc/filesystem.ts +++ b/src/main/ipc/filesystem.ts @@ -28,6 +28,7 @@ import { getStatus, detectConflictOperation, getDiff, + commitChanges, stageFile, unstageFile, bulkStageFiles, @@ -505,6 +506,28 @@ export function registerFilesystemHandlers(store: Store): void { } ) + ipcMain.handle( + 'git:commit', + async ( + _event, + args: { worktreePath: string; message: string; connectionId?: string } + ): Promise<{ success: boolean; error?: string }> => { + // Why: validate at the IPC boundary so the renderer gets a clear error instead of an opaque execFile failure. + if (typeof args.message !== 'string' || args.message.trim().length === 0) { + throw new Error('Commit message is required') + } + if (args.connectionId) { + const provider = getSshGitProvider(args.connectionId) + if (!provider) { + throw new Error(`No git provider for connection "${args.connectionId}"`) + } + return provider.commit(args.worktreePath, args.message) + } + const worktreePath = await resolveRegisteredWorktreePath(args.worktreePath, store) + return commitChanges(worktreePath, args.message) + } + ) + ipcMain.handle( 'git:branchCompare', async ( diff --git a/src/main/providers/ssh-git-provider.test.ts b/src/main/providers/ssh-git-provider.test.ts index 3ccfbb11746..0dc4ac02060 100644 --- a/src/main/providers/ssh-git-provider.test.ts +++ b/src/main/providers/ssh-git-provider.test.ts @@ -41,6 +41,19 @@ describe('SshGitProvider', () => { expect(result).toEqual(statusResult) }) + it('commit sends git.commit request', async () => { + const commitResult = { success: true } + mux.request.mockResolvedValue(commitResult) + + const result = await provider.commit('/home/user/repo', 'feat: add source control commit') + + expect(mux.request).toHaveBeenCalledWith('git.commit', { + worktreePath: '/home/user/repo', + message: 'feat: add source control commit' + }) + expect(result).toEqual(commitResult) + }) + it('getDiff sends git.diff request', async () => { const diffResult = { kind: 'text', originalContent: '', modifiedContent: 'hello' } mux.request.mockResolvedValue(diffResult) diff --git a/src/main/providers/ssh-git-provider.ts b/src/main/providers/ssh-git-provider.ts index 75ced3966cf..a9fb8bc06a1 100644 --- a/src/main/providers/ssh-git-provider.ts +++ b/src/main/providers/ssh-git-provider.ts @@ -26,6 +26,16 @@ export class SshGitProvider implements IGitProvider { return (await this.mux.request('git.status', { worktreePath })) as GitStatusResult } + async commit( + worktreePath: string, + message: string + ): Promise<{ success: boolean; error?: string }> { + return (await this.mux.request('git.commit', { + worktreePath, + message + })) as { success: boolean; error?: string } + } + async getDiff( worktreePath: string, filePath: string, diff --git a/src/main/providers/types.ts b/src/main/providers/types.ts index 6c31de31b45..c2d07d8d6cf 100644 --- a/src/main/providers/types.ts +++ b/src/main/providers/types.ts @@ -132,6 +132,7 @@ export type IFilesystemProvider = { export type IGitProvider = { getStatus(worktreePath: string): Promise + commit(worktreePath: string, message: string): Promise<{ success: boolean; error?: string }> getDiff( worktreePath: string, filePath: string, diff --git a/src/preload/api-types.ts b/src/preload/api-types.ts index 75c661e851b..b1b500973c3 100644 --- a/src/preload/api-types.ts +++ b/src/preload/api-types.ts @@ -19,7 +19,7 @@ import type { GitBranchCompareResult, GitConflictOperation, GitDiffResult, - GitStatusEntry, + GitStatusResult, GitHubAssignableUser, GitHubPRFile, GitHubPRFileContents, @@ -759,10 +759,7 @@ export type PreloadApi = { onFsChanged: (callback: (payload: FsChangedPayload) => void) => () => void } git: { - status: (args: { - worktreePath: string - connectionId?: string - }) => Promise<{ entries: GitStatusEntry[] }> + status: (args: { worktreePath: string; connectionId?: string }) => Promise conflictOperation: (args: { worktreePath: string connectionId?: string @@ -791,6 +788,11 @@ export type PreloadApi = { oldPath?: string connectionId?: string }) => Promise + commit: (args: { + worktreePath: string + message: string + connectionId?: string + }) => Promise<{ success: boolean; error?: string }> stage: (args: { worktreePath: string filePath: string diff --git a/src/preload/index.ts b/src/preload/index.ts index bb52c07c437..067d9274f1d 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -1272,6 +1272,11 @@ const api = { oldPath?: string connectionId?: string }): Promise => ipcRenderer.invoke('git:branchDiff', args), + commit: (args: { + worktreePath: string + message: string + connectionId?: string + }): Promise<{ success: boolean; error?: string }> => ipcRenderer.invoke('git:commit', args), stage: (args: { worktreePath: string filePath: string diff --git a/src/relay/git-handler-staging.test.ts b/src/relay/git-handler-staging.test.ts new file mode 100644 index 00000000000..9deae4e1971 --- /dev/null +++ b/src/relay/git-handler-staging.test.ts @@ -0,0 +1,126 @@ +/** + * Tests for GitHandler commit and bulk-staging operations. + * + * Why: split from git-handler.test.ts to stay under the oxlint max-lines (300) limit. + */ +import { describe, expect, it, beforeEach, afterEach } from 'vitest' +import * as path from 'path' +import * as fs from 'fs/promises' +import { mkdtempSync, writeFileSync } from 'fs' +import { tmpdir } from 'os' +import { execFileSync } from 'child_process' +import { GitHandler } from './git-handler' +import { RelayContext } from './context' +import { + createMockDispatcher, + gitInit, + gitCommit, + type MockDispatcher, + type RelayDispatcher +} from './git-handler-test-setup' + +describe('GitHandler — commit & staging', () => { + let dispatcher: MockDispatcher + let tmpDir: string + + beforeEach(() => { + tmpDir = mkdtempSync(path.join(tmpdir(), 'relay-git-staging-')) + dispatcher = createMockDispatcher() + const ctx = new RelayContext() + ctx.registerRoot(tmpDir) + // eslint-disable-next-line no-new + new GitHandler(dispatcher as unknown as RelayDispatcher, ctx) + }) + + afterEach(async () => { + await fs.rm(tmpDir, { recursive: true, force: true }) + }) + + describe('commit', () => { + it('commits staged changes and returns success', async () => { + gitInit(tmpDir) + writeFileSync(path.join(tmpDir, 'file.txt'), 'content') + gitCommit(tmpDir, 'initial') + writeFileSync(path.join(tmpDir, 'file.txt'), 'changed') + execFileSync('git', ['add', 'file.txt'], { cwd: tmpDir, stdio: 'pipe' }) + + const result = (await dispatcher.callRequest('git.commit', { + worktreePath: tmpDir, + message: 'feat: relay commit' + })) as { success: boolean; error?: string } + + expect(result).toEqual({ success: true }) + const latestMessage = execFileSync('git', ['log', '-1', '--format=%s'], { + cwd: tmpDir, + encoding: 'utf-8' + }).trim() + expect(latestMessage).toBe('feat: relay commit') + }) + + // Why: covers the error-extraction path in commitChangesRelay + // (git-handler-worktree-ops.ts). Running `git commit` with nothing staged + // exits non-zero and writes a "nothing to commit" message; we assert the + // relay surfaces a non-empty error string so the UI can display it. + it('returns a non-empty error when the commit fails', async () => { + gitInit(tmpDir) + + const result = (await dispatcher.callRequest('git.commit', { + worktreePath: tmpDir, + message: 'no changes' + })) as { success: boolean; error?: string } + + expect(result.success).toBe(false) + expect(typeof result.error).toBe('string') + expect((result.error ?? '').length).toBeGreaterThan(0) + // Why: exact phrasing can vary across git versions, so match the + // stable substring "nothing" rather than the full "nothing to commit". + expect((result.error ?? '').toLowerCase()).toContain('nothing') + }) + }) + + describe('bulkStage and bulkUnstage', () => { + it('stages multiple files', async () => { + gitInit(tmpDir) + writeFileSync(path.join(tmpDir, 'a.txt'), 'a') + writeFileSync(path.join(tmpDir, 'b.txt'), 'b') + gitCommit(tmpDir, 'initial') + + writeFileSync(path.join(tmpDir, 'a.txt'), 'a-modified') + writeFileSync(path.join(tmpDir, 'b.txt'), 'b-modified') + + await dispatcher.callRequest('git.bulkStage', { + worktreePath: tmpDir, + filePaths: ['a.txt', 'b.txt'] + }) + + const output = execFileSync('git', ['diff', '--cached', '--name-only'], { + cwd: tmpDir, + encoding: 'utf-8' + }) + expect(output).toContain('a.txt') + expect(output).toContain('b.txt') + }) + + it('unstages multiple files', async () => { + gitInit(tmpDir) + writeFileSync(path.join(tmpDir, 'a.txt'), 'a') + writeFileSync(path.join(tmpDir, 'b.txt'), 'b') + gitCommit(tmpDir, 'initial') + + writeFileSync(path.join(tmpDir, 'a.txt'), 'changed') + writeFileSync(path.join(tmpDir, 'b.txt'), 'changed') + execFileSync('git', ['add', '.'], { cwd: tmpDir, stdio: 'pipe' }) + + await dispatcher.callRequest('git.bulkUnstage', { + worktreePath: tmpDir, + filePaths: ['a.txt', 'b.txt'] + }) + + const output = execFileSync('git', ['diff', '--cached', '--name-only'], { + cwd: tmpDir, + encoding: 'utf-8' + }) + expect(output.trim()).toBe('') + }) + }) +}) diff --git a/src/relay/git-handler-status-ops.ts b/src/relay/git-handler-status-ops.ts new file mode 100644 index 00000000000..5f55d861449 --- /dev/null +++ b/src/relay/git-handler-status-ops.ts @@ -0,0 +1,77 @@ +/** + * Status and conflict-detection operations extracted from git-handler.ts. + * + * Why: oxlint max-lines (300) requires splitting large files. + * These functions are pure data operations on git state — no class coupling. + */ +import * as path from 'path' +import { existsSync } from 'fs' +import { readFile } from 'fs/promises' +import { parseStatusOutput, parseUnmergedEntry } from './git-handler-utils' +import type { GitExec } from './git-handler-ops' + +export async function resolveGitDir(worktreePath: string): Promise { + const dotGitPath = path.join(worktreePath, '.git') + try { + const contents = await readFile(dotGitPath, 'utf-8') + const match = contents.match(/^gitdir:\s*(.+)\s*$/m) + if (match) { + return path.resolve(worktreePath, match[1]) + } + } catch { + // .git is a directory, not a file + } + return dotGitPath +} + +export async function detectConflictOperation(worktreePath: string): Promise { + const gitDir = await resolveGitDir(worktreePath) + try { + if (existsSync(path.join(gitDir, 'MERGE_HEAD'))) { + return 'merge' + } + if ( + existsSync(path.join(gitDir, 'rebase-merge')) || + existsSync(path.join(gitDir, 'rebase-apply')) + ) { + return 'rebase' + } + if (existsSync(path.join(gitDir, 'CHERRY_PICK_HEAD'))) { + return 'cherry-pick' + } + } catch { + // fs error — treat as no conflict operation + } + return 'unknown' +} + +export async function getStatusOp( + git: GitExec, + validatePath: (p: string) => void, + params: Record +): Promise<{ entries: Record[]; conflictOperation: string }> { + const worktreePath = params.worktreePath as string + validatePath(worktreePath) + const conflictOperation = await detectConflictOperation(worktreePath) + const entries: Record[] = [] + + try { + const { stdout } = await git( + ['status', '--porcelain=v2', '--untracked-files=all'], + worktreePath + ) + const parsed = parseStatusOutput(stdout) + entries.push(...parsed.entries) + + for (const uLine of parsed.unmergedLines) { + const entry = parseUnmergedEntry(worktreePath, uLine) + if (entry) { + entries.push(entry) + } + } + } catch { + // not a git repo or git not available + } + + return { entries, conflictOperation } +} diff --git a/src/relay/git-handler-test-setup.ts b/src/relay/git-handler-test-setup.ts new file mode 100644 index 00000000000..8e5dc7ef0c3 --- /dev/null +++ b/src/relay/git-handler-test-setup.ts @@ -0,0 +1,76 @@ +/** + * Shared test utilities for git-handler tests. + * + * Why: oxlint max-lines (300) requires splitting large test suites. + * This module exports the mock dispatcher factory and git helpers + * so multiple test files can reuse them without duplication. + */ +import { vi } from 'vitest' +import { execFileSync } from 'child_process' +import type { RelayDispatcher } from './dispatcher' + +// Why: declare an explicit type so the inferred return type of +// createMockDispatcher doesn't transitively reference `@vitest/spy`'s +// internal `Procedure` type (from `vi.fn(...)`). Without this annotation, +// TS2883 fires under `pnpm run tc:node` because the generated .d.ts would +// need to name a type that isn't portably resolvable from this module. +export type MockDispatcher = { + onRequest: ( + method: string, + handler: ( + params: Record, + context: { isStale: () => boolean } + ) => Promise + ) => void + onNotification: (method: string, handler: (params: Record) => void) => void + notify: (method: string, params?: Record) => void + _requestHandlers: Map< + string, + (params: Record, context: { isStale: () => boolean }) => Promise + > + callRequest(method: string, params?: Record): Promise +} + +export function createMockDispatcher(): MockDispatcher { + const requestHandlers = new Map< + string, + (params: Record, context: { isStale: () => boolean }) => Promise + >() + + return { + onRequest: vi.fn( + ( + method: string, + handler: ( + params: Record, + context: { isStale: () => boolean } + ) => Promise + ) => { + requestHandlers.set(method, handler) + } + ), + onNotification: vi.fn(), + notify: vi.fn(), + _requestHandlers: requestHandlers, + async callRequest(method: string, params: Record = {}) { + const handler = requestHandlers.get(method) + if (!handler) { + throw new Error(`No handler for ${method}`) + } + return handler(params, { isStale: () => false }) + } + } +} + +export function gitInit(dir: string): void { + execFileSync('git', ['init'], { cwd: dir, stdio: 'pipe' }) + execFileSync('git', ['config', 'user.email', 'test@test.com'], { cwd: dir, stdio: 'pipe' }) + execFileSync('git', ['config', 'user.name', 'Test'], { cwd: dir, stdio: 'pipe' }) +} + +export function gitCommit(dir: string, message: string): void { + execFileSync('git', ['add', '.'], { cwd: dir, stdio: 'pipe' }) + execFileSync('git', ['commit', '-m', message, '--allow-empty'], { cwd: dir, stdio: 'pipe' }) +} + +export type { RelayDispatcher } diff --git a/src/relay/git-handler-worktree-ops.ts b/src/relay/git-handler-worktree-ops.ts new file mode 100644 index 00000000000..66ea18398d5 --- /dev/null +++ b/src/relay/git-handler-worktree-ops.ts @@ -0,0 +1,110 @@ +/** + * Worktree management and commit operations for the relay git handler. + * + * Why: extracted from git-handler-ops.ts to keep all relay files under + * the oxlint max-lines (300) limit. + */ +import * as path from 'path' +import type { GitExec } from './git-handler-ops' + +// ─── Worktree management ───────────────────────────────────────────── + +export async function addWorktreeOp( + git: GitExec, + validatePath: (p: string) => void, + params: Record +): Promise { + const repoPath = params.repoPath as string + validatePath(repoPath) + const branchName = params.branchName as string + const targetDir = params.targetDir as string + validatePath(targetDir) + const base = params.base as string | undefined + const track = params.track as boolean | undefined + + // Why: a branchName starting with '-' would be interpreted as a git flag, + // potentially changing the command's semantics (e.g. "--detach"). + if (branchName.startsWith('-') || (base && base.startsWith('-'))) { + throw new Error('Branch name and base ref must not start with "-"') + } + + const args = ['worktree', 'add'] + if (track) { + args.push('--track') + } + args.push('-b', branchName, targetDir) + if (base) { + args.push(base) + } + + await git(args, repoPath) +} + +export async function removeWorktreeOp( + git: GitExec, + validatePath: (p: string) => void, + params: Record +): Promise { + const worktreePath = params.worktreePath as string + validatePath(worktreePath) + const force = params.force as boolean | undefined + + let repoPath = worktreePath + try { + const { stdout } = await git(['rev-parse', '--git-common-dir'], worktreePath) + const commonDir = stdout.trim() + if (commonDir && commonDir !== '.git') { + repoPath = path.resolve(worktreePath, commonDir, '..') + } + } catch { + // fall through with worktreePath as repo + } + + const args = ['worktree', 'remove'] + if (force) { + args.push('--force') + } + args.push(worktreePath) + await git(args, repoPath) + await git(['worktree', 'prune'], repoPath) +} + +// ─── Commit ────────────────────────────────────────────────────────── + +export async function commitChangesRelay( + git: GitExec, + worktreePath: string, + message: string +): Promise<{ success: boolean; error?: string }> { + // Why: defense-in-depth. The IPC handler at src/main/ipc/filesystem.ts validates + // the message, but a relay caller (future automation, or an SSH client connecting + // to the relay directly) could bypass that path. Reject empty/whitespace messages + // here so we surface a clear error instead of git's opaque failure. + if (typeof message !== 'string' || message.trim().length === 0) { + return { success: false, error: 'Commit message is required' } + } + + try { + await git(['commit', '-m', message], worktreePath) + return { success: true } + } catch (error) { + // Why: surface whichever channel carries the useful message. Pre-commit/GPG + // hook failures write to stderr; "nothing to commit, working tree clean" + // writes to stdout. Try stderr first, fall back to stdout, then error.message. + // Mirrors commitChanges in src/main/git/status.ts — keep the two paths in sync. + const readStringField = (field: string): string | null => { + if (typeof error === 'object' && error && field in error) { + const v = (error as Record)[field] + if (typeof v === 'string' && v.length > 0) { + return v + } + } + return null + } + const errorMessage = + readStringField('stderr') ?? + readStringField('stdout') ?? + (error instanceof Error ? error.message : 'Commit failed') + return { success: false, error: errorMessage } + } +} diff --git a/src/relay/git-handler.test.ts b/src/relay/git-handler.test.ts index 1cf6ff160fe..9541ffcca0a 100644 --- a/src/relay/git-handler.test.ts +++ b/src/relay/git-handler.test.ts @@ -1,51 +1,21 @@ -import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest' +import { describe, expect, it, beforeEach, afterEach } from 'vitest' import { GitHandler } from './git-handler' import { RelayContext } from './context' -import type { RelayDispatcher } from './dispatcher' import * as fs from 'fs/promises' import * as path from 'path' import { mkdtempSync, writeFileSync } from 'fs' import { tmpdir } from 'os' import { execFileSync } from 'child_process' - -function createMockDispatcher() { - const requestHandlers = new Map) => Promise>() - const notificationHandlers = new Map) => void>() - - return { - onRequest: vi.fn( - (method: string, handler: (params: Record) => Promise) => { - requestHandlers.set(method, handler) - } - ), - onNotification: vi.fn((method: string, handler: (params: Record) => void) => { - notificationHandlers.set(method, handler) - }), - notify: vi.fn(), - _requestHandlers: requestHandlers, - async callRequest(method: string, params: Record = {}) { - const handler = requestHandlers.get(method) - if (!handler) { - throw new Error(`No handler for ${method}`) - } - return handler(params) - } - } -} - -function gitInit(dir: string): void { - execFileSync('git', ['init'], { cwd: dir, stdio: 'pipe' }) - execFileSync('git', ['config', 'user.email', 'test@test.com'], { cwd: dir, stdio: 'pipe' }) - execFileSync('git', ['config', 'user.name', 'Test'], { cwd: dir, stdio: 'pipe' }) -} - -function gitCommit(dir: string, message: string): void { - execFileSync('git', ['add', '.'], { cwd: dir, stdio: 'pipe' }) - execFileSync('git', ['commit', '-m', message, '--allow-empty'], { cwd: dir, stdio: 'pipe' }) -} +import { + createMockDispatcher, + gitInit, + gitCommit, + type MockDispatcher, + type RelayDispatcher +} from './git-handler-test-setup' describe('GitHandler', () => { - let dispatcher: ReturnType + let dispatcher: MockDispatcher let tmpDir: string beforeEach(() => { @@ -53,6 +23,7 @@ describe('GitHandler', () => { dispatcher = createMockDispatcher() const ctx = new RelayContext() ctx.registerRoot(tmpDir) + // eslint-disable-next-line no-new new GitHandler(dispatcher as unknown as RelayDispatcher, ctx) }) @@ -63,6 +34,7 @@ describe('GitHandler', () => { it('registers all expected handlers', () => { const methods = Array.from(dispatcher._requestHandlers.keys()) expect(methods).toContain('git.status') + expect(methods).toContain('git.commit') expect(methods).toContain('git.diff') expect(methods).toContain('git.stage') expect(methods).toContain('git.unstage') @@ -75,6 +47,8 @@ describe('GitHandler', () => { expect(methods).toContain('git.listWorktrees') expect(methods).toContain('git.addWorktree') expect(methods).toContain('git.removeWorktree') + expect(methods).toContain('git.exec') + expect(methods).toContain('git.isGitRepo') }) describe('status', () => { @@ -284,50 +258,4 @@ describe('GitHandler', () => { expect(result[0].isMainWorktree).toBe(true) }) }) - - describe('bulkStage and bulkUnstage', () => { - it('stages multiple files', async () => { - gitInit(tmpDir) - writeFileSync(path.join(tmpDir, 'a.txt'), 'a') - writeFileSync(path.join(tmpDir, 'b.txt'), 'b') - gitCommit(tmpDir, 'initial') - - writeFileSync(path.join(tmpDir, 'a.txt'), 'a-modified') - writeFileSync(path.join(tmpDir, 'b.txt'), 'b-modified') - - await dispatcher.callRequest('git.bulkStage', { - worktreePath: tmpDir, - filePaths: ['a.txt', 'b.txt'] - }) - - const output = execFileSync('git', ['diff', '--cached', '--name-only'], { - cwd: tmpDir, - encoding: 'utf-8' - }) - expect(output).toContain('a.txt') - expect(output).toContain('b.txt') - }) - - it('unstages multiple files', async () => { - gitInit(tmpDir) - writeFileSync(path.join(tmpDir, 'a.txt'), 'a') - writeFileSync(path.join(tmpDir, 'b.txt'), 'b') - gitCommit(tmpDir, 'initial') - - writeFileSync(path.join(tmpDir, 'a.txt'), 'changed') - writeFileSync(path.join(tmpDir, 'b.txt'), 'changed') - execFileSync('git', ['add', '.'], { cwd: tmpDir, stdio: 'pipe' }) - - await dispatcher.callRequest('git.bulkUnstage', { - worktreePath: tmpDir, - filePaths: ['a.txt', 'b.txt'] - }) - - const output = execFileSync('git', ['diff', '--cached', '--name-only'], { - cwd: tmpDir, - encoding: 'utf-8' - }) - expect(output.trim()).toBe('') - }) - }) }) diff --git a/src/relay/git-handler.ts b/src/relay/git-handler.ts index 7e312bea072..7f34fcfec83 100644 --- a/src/relay/git-handler.ts +++ b/src/relay/git-handler.ts @@ -1,23 +1,19 @@ import { execFile } from 'child_process' import { promisify } from 'util' -import { existsSync } from 'fs' -import { readFile, rm } from 'fs/promises' +import { rm } from 'fs/promises' import * as path from 'path' import type { RelayDispatcher } from './dispatcher' import type { RelayContext } from './context' import { expandTilde } from './context' -import { - parseStatusOutput, - parseUnmergedEntry, - parseBranchDiff, - parseWorktreeList -} from './git-handler-utils' +import { parseBranchDiff, parseWorktreeList } from './git-handler-utils' import { computeDiff, branchCompare as branchCompareOp, branchDiffEntries, validateGitExecArgs } from './git-handler-ops' +import { commitChangesRelay, addWorktreeOp, removeWorktreeOp } from './git-handler-worktree-ops' +import { detectConflictOperation, getStatusOp } from './git-handler-status-ops' const execFileAsync = promisify(execFile) const MAX_GIT_BUFFER = 10 * 1024 * 1024 @@ -35,6 +31,7 @@ export class GitHandler { private registerHandlers(): void { this.dispatcher.onRequest('git.status', (p) => this.getStatus(p)) + this.dispatcher.onRequest('git.commit', (p) => this.commit(p)) this.dispatcher.onRequest('git.diff', (p) => this.getDiff(p)) this.dispatcher.onRequest('git.stage', (p) => this.stage(p)) this.dispatcher.onRequest('git.unstage', (p) => this.unstage(p)) @@ -73,66 +70,7 @@ export class GitHandler { } private async getStatus(params: Record) { - const worktreePath = params.worktreePath as string - this.context.validatePath(worktreePath) - const conflictOperation = await this.detectConflictOperation(worktreePath) - const entries: Record[] = [] - - try { - const { stdout } = await this.git( - ['status', '--porcelain=v2', '--untracked-files=all'], - worktreePath - ) - - const parsed = parseStatusOutput(stdout) - entries.push(...parsed.entries) - - for (const uLine of parsed.unmergedLines) { - const entry = parseUnmergedEntry(worktreePath, uLine) - if (entry) { - entries.push(entry) - } - } - } catch { - // Not a git repo or git not available - } - - return { entries, conflictOperation } - } - - private async detectConflictOperation(worktreePath: string): Promise { - const gitDir = await this.resolveGitDir(worktreePath) - try { - if (existsSync(path.join(gitDir, 'MERGE_HEAD'))) { - return 'merge' - } - if ( - existsSync(path.join(gitDir, 'rebase-merge')) || - existsSync(path.join(gitDir, 'rebase-apply')) - ) { - return 'rebase' - } - if (existsSync(path.join(gitDir, 'CHERRY_PICK_HEAD'))) { - return 'cherry-pick' - } - } catch { - // fs error - } - return 'unknown' - } - - private async resolveGitDir(worktreePath: string): Promise { - const dotGitPath = path.join(worktreePath, '.git') - try { - const contents = await readFile(dotGitPath, 'utf-8') - const match = contents.match(/^gitdir:\s*(.+)\s*$/m) - if (match) { - return path.resolve(worktreePath, match[1]) - } - } catch { - // .git is a directory - } - return dotGitPath + return getStatusOp(this.git.bind(this), this.context.validatePath.bind(this.context), params) } private async getDiff(params: Record) { @@ -162,6 +100,15 @@ export class GitHandler { await this.git(['add', '--', filePath], worktreePath) } + private async commit( + params: Record + ): Promise<{ success: boolean; error?: string }> { + const worktreePath = params.worktreePath as string + this.context.validatePath(worktreePath) + const message = params.message as string + return commitChangesRelay(this.git.bind(this), worktreePath, message) + } + private async unstage(params: Record) { const worktreePath = params.worktreePath as string this.context.validatePath(worktreePath) @@ -224,7 +171,7 @@ export class GitHandler { private async conflictOperation(params: Record) { const worktreePath = params.worktreePath as string this.context.validatePath(worktreePath) - return this.detectConflictOperation(worktreePath) + return detectConflictOperation(worktreePath) } private async branchCompare(params: Record) { @@ -301,54 +248,14 @@ export class GitHandler { } private async addWorktree(params: Record) { - const repoPath = params.repoPath as string - this.context.validatePath(repoPath) - const branchName = params.branchName as string - const targetDir = params.targetDir as string - this.context.validatePath(targetDir) - const base = params.base as string | undefined - const track = params.track as boolean | undefined - - // Why: a branchName starting with '-' would be interpreted as a git flag, - // potentially changing the command's semantics (e.g. "--detach"). - if (branchName.startsWith('-') || (base && base.startsWith('-'))) { - throw new Error('Branch name and base ref must not start with "-"') - } - - const args = ['worktree', 'add'] - if (track) { - args.push('--track') - } - args.push('-b', branchName, targetDir) - if (base) { - args.push(base) - } - - await this.git(args, repoPath) + return addWorktreeOp(this.git.bind(this), this.context.validatePath.bind(this.context), params) } private async removeWorktree(params: Record) { - const worktreePath = params.worktreePath as string - this.context.validatePath(worktreePath) - const force = params.force as boolean | undefined - - let repoPath = worktreePath - try { - const { stdout } = await this.git(['rev-parse', '--git-common-dir'], worktreePath) - const commonDir = stdout.trim() - if (commonDir && commonDir !== '.git') { - repoPath = path.resolve(worktreePath, commonDir, '..') - } - } catch { - // Fall through with worktreePath as repo - } - - const args = ['worktree', 'remove'] - if (force) { - args.push('--force') - } - args.push(worktreePath) - await this.git(args, repoPath) - await this.git(['worktree', 'prune'], repoPath) + return removeWorktreeOp( + this.git.bind(this), + this.context.validatePath.bind(this.context), + params + ) } } diff --git a/src/renderer/src/components/right-sidebar/CommitArea.test.tsx b/src/renderer/src/components/right-sidebar/CommitArea.test.tsx new file mode 100644 index 00000000000..6506aebc277 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/CommitArea.test.tsx @@ -0,0 +1,188 @@ +import { describe, expect, it, vi } from 'vitest' +import { CommitArea } 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 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 findCommitButton(node: unknown): ReactElementLike { + let found: ReactElementLike | null = null + visit(node, (entry) => { + if (entry.type === Button) { + found = entry + } + }) + if (!found) { + throw new Error('commit button not found') + } + return found +} + +function hasText(node: unknown, text: string): boolean { + let found = false + visit(node, (entry) => { + const children = entry.props?.children + if (typeof children === 'string' && children.includes(text)) { + found = true + } + }) + return found +} + +function flushPromises(): Promise { + return new Promise((resolve) => setTimeout(resolve, 0)) +} + +const baseProps = { + stagedCount: 1, + hasUnresolvedConflicts: false, + commitMessage: 'feat: add commit area', + commitError: null as string | null, + isCommitting: false, + onCommitMessageChange: vi.fn(), + onCommitSuccess: vi.fn() +} + +describe('CommitArea', () => { + it('disables commit button when no staged files', () => { + const element = CommitArea({ ...baseProps, stagedCount: 0 }) + const button = findCommitButton(element) + expect(button.props.disabled).toBe(true) + }) + + it('disables commit button when message is empty', () => { + const element = CommitArea({ ...baseProps, commitMessage: ' ' }) + const button = findCommitButton(element) + expect(button.props.disabled).toBe(true) + }) + + it('disables commit button when unresolved conflicts exist', () => { + const element = CommitArea({ ...baseProps, hasUnresolvedConflicts: true }) + const button = findCommitButton(element) + expect(button.props.disabled).toBe(true) + }) + + it('enables commit button with staged files, message, and no conflicts', () => { + const element = CommitArea(baseProps) + const button = findCommitButton(element) + expect(button.props.disabled).toBe(false) + }) + + it('triggers commit when the button is clicked', () => { + const onCommitSuccess = vi.fn() + const element = CommitArea({ ...baseProps, onCommitSuccess }) + const button = findCommitButton(element) + ;(button.props.onClick as () => void)() + expect(onCommitSuccess).toHaveBeenCalledTimes(1) + }) + + it('clears message and keeps error hidden after 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 = () => + CommitArea({ + ...baseProps, + commitMessage, + commitError, + isCommitting, + onCommitSuccess: () => { + void runCommit() + } + }) + + const button = findCommitButton(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('preserves message and shows error after 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 + }) + + const render = () => + CommitArea({ + ...baseProps, + commitMessage, + commitError, + isCommitting, + onCommitSuccess: () => { + void runCommit() + } + }) + + const button = findCommitButton(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) + }) + + it('locks the button while commit is in flight', () => { + const element = CommitArea({ ...baseProps, isCommitting: true }) + const button = findCommitButton(element) + expect(button.props.disabled).toBe(true) + }) + + it('shows an inline error message when commit fails', () => { + const element = CommitArea({ ...baseProps, commitError: 'pre-commit hook failed' }) + expect(hasText(element, 'pre-commit hook failed')).toBe(true) + }) +}) 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 new file mode 100644 index 00000000000..0ae7bff19a0 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/SourceControl.commit-drafts.test.ts @@ -0,0 +1,22 @@ +import { describe, expect, it } from 'vitest' +import { readCommitDraftForWorktree, writeCommitDraftForWorktree } from './SourceControl' + +describe('SourceControl commit drafts by worktree', () => { + it('returns an empty draft when the selected worktree has no message', () => { + expect(readCommitDraftForWorktree({}, 'wt-a')).toBe('') + }) + + it('restores each worktree draft when switching between worktrees', () => { + let drafts = {} + + drafts = writeCommitDraftForWorktree(drafts, 'wt-a', 'feat: message for A') + expect(readCommitDraftForWorktree(drafts, 'wt-a')).toBe('feat: message for A') + + drafts = writeCommitDraftForWorktree(drafts, 'wt-b', 'fix: message for B') + expect(readCommitDraftForWorktree(drafts, 'wt-b')).toBe('fix: message for B') + + // Why: switching back must keep the prior draft for that worktree rather + // than leaking the active worktree's text into all worktree views. + expect(readCommitDraftForWorktree(drafts, 'wt-a')).toBe('feat: message for A') + }) +}) diff --git a/src/renderer/src/components/right-sidebar/SourceControl.tsx b/src/renderer/src/components/right-sidebar/SourceControl.tsx index d10e843bf91..414901f51a5 100644 --- a/src/renderer/src/components/right-sidebar/SourceControl.tsx +++ b/src/renderer/src/components/right-sidebar/SourceControl.tsx @@ -25,7 +25,7 @@ import { X } from 'lucide-react' import { useAppStore } from '@/store' -import { useActiveWorktree, useRepoById } from '@/store/selectors' +import { useActiveWorktree, useRepoById, useWorktreeMap } from '@/store/selectors' import { detectLanguage } from '@/lib/language-detect' import { basename, dirname, joinPath } from '@/lib/path' import { cn } from '@/lib/utils' @@ -93,6 +93,23 @@ const SECTION_LABELS: Record<(typeof SECTION_ORDER)[number], string> = { const BRANCH_REFRESH_INTERVAL_MS = 5000 +type CommitDraftsByWorktree = Record + +export function readCommitDraftForWorktree( + drafts: CommitDraftsByWorktree, + worktreeId: string | null | undefined +): string { + return drafts[worktreeId ?? ''] ?? '' +} + +export function writeCommitDraftForWorktree( + drafts: CommitDraftsByWorktree, + worktreeId: string, + value: string +): CommitDraftsByWorktree { + return { ...drafts, [worktreeId]: value } +} + const CONFLICT_KIND_LABELS: Record = { both_modified: 'Both modified', both_added: 'Both added', @@ -105,8 +122,14 @@ const CONFLICT_KIND_LABELS: Record = { function SourceControlInner(): React.JSX.Element { const sourceControlRef = useRef(null) + // Why: React setState is async, so a rapid double-click on the Commit + // button can both pass the isCommitting state guard before the disabled + // state re-renders. A ref flipped synchronously at the start of + // handleCommit gives us a true single-flight lock. + const commitInFlightRef = useRef>({}) const activeWorktree = useActiveWorktree() const activeWorktreeId = useAppStore((s) => s.activeWorktreeId) + const worktreeMap = useWorktreeMap() const rightSidebarTab = useAppStore((s) => s.rightSidebarTab) const activeRepo = useRepoById(activeWorktree?.repoId ?? null) const gitStatusByWorktree = useAppStore((s) => s.gitStatusByWorktree) @@ -118,6 +141,7 @@ function SourceControlInner(): React.JSX.Element { const updateRepo = useAppStore((s) => s.updateRepo) const beginGitBranchCompareRequest = useAppStore((s) => s.beginGitBranchCompareRequest) const setGitBranchCompareResult = useAppStore((s) => s.setGitBranchCompareResult) + const setGitStatus = useAppStore((s) => s.setGitStatus) const revealInExplorer = useAppStore((s) => s.revealInExplorer) const trackConflictPath = useAppStore((s) => s.trackConflictPath) const openDiff = useAppStore((s) => s.openDiff) @@ -181,7 +205,21 @@ function SourceControlInner(): React.JSX.Element { // falsy until we have a real answer from the main process. const [defaultBaseRef, setDefaultBaseRef] = useState(null) const [filterQuery, setFilterQuery] = useState('') + // Why: commit drafts/errors are worktree-scoped during the mounted session, + // so switching worktrees restores each draft instead of wiping it. + const [commitDrafts, setCommitDrafts] = useState({}) + const [commitErrors, setCommitErrors] = useState>({}) + // Why: keep commit-in-flight state per-worktree. A single boolean would be + // cleared when the user switched worktrees, letting them double-click Commit + // on worktree A after briefly navigating to B and back while A's original + // commit is still running. + const [commitInFlightByWorktree, setCommitInFlightByWorktree] = useState>( + {} + ) + const isCommitting = commitInFlightByWorktree[activeWorktreeId ?? ''] ?? false const filterInputRef = useRef(null) + const commitMessage = readCommitDraftForWorktree(commitDrafts, activeWorktreeId) + const commitError = commitErrors[activeWorktreeId ?? ''] ?? null const isFolder = activeRepo ? isFolderRepo(activeRepo) : false const worktreePath = activeWorktree?.path ?? null @@ -312,6 +350,48 @@ function SourceControlInner(): React.JSX.Element { const [isExecutingBulk, setIsExecutingBulk] = useState(false) + const unresolvedConflicts = useMemo( + () => entries.filter((entry) => entry.conflictStatus === 'unresolved' && entry.conflictKind), + [entries] + ) + const unresolvedConflictReviewEntries = useMemo( + () => + unresolvedConflicts.map((entry) => ({ + path: entry.path, + conflictKind: entry.conflictKind! + })), + [unresolvedConflicts] + ) + + // Why: orphaned draft/error/in-flight entries accumulate when worktrees are + // removed from the store (long sessions with many create/destroy cycles). + // Prune them so a deleted-then-reused worktree ID doesn't inherit stale + // state — especially commitInFlightRef, which would permanently disable + // Commit for that ID if left stuck at `true`. + useEffect(() => { + const pruneRecord = (prev: Record): Record => { + let changed = false + const next: Record = {} + for (const key of Object.keys(prev)) { + if (worktreeMap.has(key)) { + next[key] = prev[key] + } else { + changed = true + } + } + return changed ? next : prev + } + setCommitDrafts((prev) => pruneRecord(prev)) + setCommitErrors((prev) => pruneRecord(prev)) + setCommitInFlightByWorktree((prev) => pruneRecord(prev)) + // Refs don't need setState — mutate in place to drop stale keys. + for (const key of Object.keys(commitInFlightRef.current)) { + if (!worktreeMap.has(key)) { + delete commitInFlightRef.current[key] + } + } + }, [worktreeMap]) + // Why: the sidebar no longer uses key={activeWorktreeId} to force a full // remount on worktree switch (that caused an IPC storm on Windows). // Instead, reset worktree-specific local state here so the previous @@ -329,8 +409,90 @@ function SourceControlInner(): React.JSX.Element { // repos and back to re-trigger the resolver. setFilterQuery('') setIsExecutingBulk(false) + // Why: no reset for commit-in-flight state — it now lives in a per-worktree + // map, so it cannot leak across worktrees. Resetting here would actually + // clear in-flight state for the *incoming* worktree if the user is coming + // back to a worktree mid-commit, re-enabling the button while the commit + // still runs. }, [activeWorktreeId]) + const handleCommit = useCallback(async (): Promise => { + if (!activeWorktreeId || !worktreePath) { + return + } + const message = commitMessage.trim() + if (!message || grouped.staged.length === 0 || unresolvedConflicts.length > 0) { + return + } + + if (commitInFlightRef.current[activeWorktreeId]) { + return + } + commitInFlightRef.current[activeWorktreeId] = true + + const connectionId = getConnectionId(activeWorktreeId) ?? undefined + setCommitInFlightByWorktree((prev) => ({ ...prev, [activeWorktreeId]: true })) + setCommitErrors((prev) => ({ ...prev, [activeWorktreeId]: null })) + try { + const commitResult = await window.api.git.commit({ + worktreePath, + message, + connectionId + }) + if (!commitResult.success) { + setCommitErrors((prev) => ({ + ...prev, + [activeWorktreeId]: commitResult.error ?? 'Commit failed' + })) + return + } + + // Why: the textarea stays enabled during the in-flight commit (only the + // button is disabled), so the user can keep typing after clicking Commit. + // Unconditionally clearing the draft here would silently discard those + // in-progress edits — the commit used the OLD `message` captured in this + // closure, so the dropped text would never have been committed either. + // Only clear when the current draft still matches what we committed. + setCommitDrafts((prev) => { + const current = prev[activeWorktreeId] + if (current !== undefined && current.trim() !== message) { + // User typed more after submit — preserve their in-progress edits. + return prev + } + return writeCommitDraftForWorktree(prev, activeWorktreeId, '') + }) + setCommitErrors((prev) => ({ ...prev, [activeWorktreeId]: null })) + // Why: the commit already succeeded. If the follow-up status refresh fails + // (e.g., transient IPC error), log it but do NOT overwrite the cleared + // commitError with a misleading "Commit failed" — the existing status poll + // in useGitStatusPolling will refresh the UI shortly anyway. + try { + const status = await window.api.git.status({ + worktreePath, + connectionId + }) + setGitStatus(activeWorktreeId, status) + } catch (refreshError) { + console.error('[SourceControl] post-commit status refresh failed', refreshError) + } + } catch (error) { + setCommitErrors((prev) => ({ + ...prev, + [activeWorktreeId]: error instanceof Error ? error.message : 'Commit failed' + })) + } finally { + setCommitInFlightByWorktree((prev) => ({ ...prev, [activeWorktreeId]: false })) + commitInFlightRef.current[activeWorktreeId] = false + } + }, [ + activeWorktreeId, + commitMessage, + grouped.staged.length, + unresolvedConflicts.length, + setGitStatus, + worktreePath + ]) + const handleOpenDiff = useCallback( (entry: GitStatusEntry) => { if (!activeWorktreeId || !worktreePath) { @@ -455,19 +617,6 @@ function SourceControlInner(): React.JSX.Element { } }, [worktreePath, bulkUnstagePaths, clearSelection, activeWorktreeId]) - const unresolvedConflicts = useMemo( - () => entries.filter((entry) => entry.conflictStatus === 'unresolved' && entry.conflictKind), - [entries] - ) - const unresolvedConflictReviewEntries = useMemo( - () => - unresolvedConflicts.map((entry) => ({ - path: entry.path, - conflictKind: entry.conflictKind! - })), - [unresolvedConflicts] - ) - const refreshBranchCompare = useCallback(async () => { if (!activeWorktreeId || !worktreePath || !effectiveBaseRef || isFolder) { return @@ -717,8 +866,12 @@ function SourceControlInner(): React.JSX.Element { {/* Why: Diff-comments live on the worktree and apply across every diff view the user opens. The header row expands inline to show per-file comment previews plus a Copy-all action so the user can hand the - set off to whichever tool they want without leaving the sidebar. */} - {activeWorktreeId && worktreePath && ( + set off to whichever tool they want without leaving the sidebar. + Hidden when count is 0: notes are created from the diff view, so + an empty Notes shelf in the sidebar is pure chrome — it adds a + border, a row of space, and an expand control that only reveals + a redirect hint. */} + {activeWorktreeId && worktreePath && diffCommentCount > 0 && (