diff --git a/src/main/git/remote.test.ts b/src/main/git/remote.test.ts new file mode 100644 index 00000000000..4d3f626f8a5 --- /dev/null +++ b/src/main/git/remote.test.ts @@ -0,0 +1,134 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { gitExecFileAsyncMock } = vi.hoisted(() => ({ + gitExecFileAsyncMock: vi.fn() +})) + +vi.mock('./runner', () => ({ + gitExecFileAsync: gitExecFileAsyncMock +})) + +import { gitFetch, gitPull, gitPush } from './remote' + +describe('git remote operations', () => { + beforeEach(() => { + gitExecFileAsyncMock.mockReset() + }) + + it('pushes with --set-upstream regardless of publish flag', async () => { + // Why: every push uses --set-upstream so worktrees that were created + // tracking the BASE ref (origin/main) get their upstream repointed to + // origin/ on first push. Without that the local branch keeps + // tracking origin/main forever and the UI's ahead/behind read via + // @{u} measures "ahead of base" rather than "ahead of remote branch". + // Both publish=true and publish=false take the same path now; the + // parameter is preserved in the signature for IPC compatibility but + // is no longer load-bearing. + gitExecFileAsyncMock.mockResolvedValue({ stdout: '', stderr: '' }) + + await gitPush('/repo', true) + await gitPush('/repo', false) + + expect(gitExecFileAsyncMock).toHaveBeenNthCalledWith( + 1, + ['push', '--set-upstream', 'origin', 'HEAD'], + { cwd: '/repo' } + ) + expect(gitExecFileAsyncMock).toHaveBeenNthCalledWith( + 2, + ['push', '--set-upstream', 'origin', 'HEAD'], + { cwd: '/repo' } + ) + }) + + it('maps non-fast-forward push failures to an actionable message', async () => { + gitExecFileAsyncMock.mockRejectedValueOnce(new Error('remote rejected: non-fast-forward')) + + await expect(gitPush('/repo', false)).rejects.toThrow( + 'Push rejected: remote has newer commits (non-fast-forward). Please pull or sync first.' + ) + }) + + it('passes through clean tail line when push error does not match known patterns', async () => { + gitExecFileAsyncMock.mockRejectedValueOnce( + new Error('Command failed: git push\nfatal: something obscure happened') + ) + + await expect(gitPush('/repo', false)).rejects.toThrow('fatal: something obscure happened') + }) + + it('strips embedded credentials from push error messages', async () => { + gitExecFileAsyncMock.mockRejectedValueOnce( + new Error( + 'Command failed: git push\nhttps://x-access-token:ghp_abc@github.com/foo/bar.git\nfatal: remote error' + ) + ) + + let caught: Error | undefined + try { + await gitPush('/repo', false) + } catch (error) { + caught = error as Error + } + + expect(caught).toBeInstanceOf(Error) + expect(caught?.message).not.toContain('ghp_abc') + expect(caught?.message).not.toContain('x-access-token') + }) + + it('strips token-only credentials (https://TOKEN@host) from push error messages', async () => { + gitExecFileAsyncMock.mockRejectedValueOnce( + new Error( + 'Command failed: git push\nhttps://ghp_onlyToken@github.com/foo/bar.git\nfatal: remote error' + ) + ) + + let caught: Error | undefined + try { + await gitPush('/repo', false) + } catch (error) { + caught = error as Error + } + + expect(caught).toBeInstanceOf(Error) + expect(caught?.message).not.toContain('ghp_onlyToken') + }) + + it('falls back to a generic message for non-Error rejections', async () => { + gitExecFileAsyncMock.mockRejectedValueOnce('string') + + await expect(gitPush('/repo', false)).rejects.toThrow('Git remote operation failed.') + }) + + it("runs pull with the user's configured strategy", async () => { + gitExecFileAsyncMock.mockResolvedValue({ stdout: '', stderr: '' }) + + await gitPull('/repo') + + expect(gitExecFileAsyncMock).toHaveBeenCalledWith(['pull'], { cwd: '/repo' }) + }) + + it('normalizes pull authentication errors to a friendly message', async () => { + gitExecFileAsyncMock.mockRejectedValueOnce(new Error('Authentication failed')) + + await expect(gitPull('/repo')).rejects.toThrow( + 'Authentication failed. Check your remote credentials.' + ) + }) + + it('runs fetch with prune', async () => { + gitExecFileAsyncMock.mockResolvedValue({ stdout: '', stderr: '' }) + + await gitFetch('/repo') + + expect(gitExecFileAsyncMock).toHaveBeenCalledWith(['fetch', '--prune'], { cwd: '/repo' }) + }) + + it('normalizes fetch authentication errors to a friendly message', async () => { + gitExecFileAsyncMock.mockRejectedValueOnce(new Error('Authentication failed')) + + await expect(gitFetch('/repo')).rejects.toThrow( + 'Authentication failed. Check your remote credentials.' + ) + }) +}) diff --git a/src/main/git/remote.ts b/src/main/git/remote.ts new file mode 100644 index 00000000000..2d26c5c75b1 --- /dev/null +++ b/src/main/git/remote.ts @@ -0,0 +1,46 @@ +import { normalizeGitErrorMessage } from '../../shared/git-remote-error' +import { gitExecFileAsync } from './runner' + +export async function gitPush(worktreePath: string, _publish = false): Promise { + try { + // Why: always pass --set-upstream so that worktrees Orca creates with + // `git worktree add --track -b ` (which initially + // track the BASE — e.g. origin/main) get their upstream repointed to + // origin/ on first push. Without this the local branch keeps + // tracking origin/main forever, so ahead/behind reads via @{u} measure + // "ahead of base" rather than "ahead of remote branch", and the primary + // button never rotates from "Push" to "Commit" after a successful push. + // + // The `publish` flag becomes redundant under this strategy — every push + // sets upstream, including the first. We keep the parameter in the + // signature so callers don't need to change, but it's no longer + // load-bearing. On an already-published branch --set-upstream is a + // no-op for the tracking config and a regular push otherwise. + // + // Branch-vs-base reporting (the "Committed on Branch" section) is + // unaffected because it uses branchCompare against an explicit baseRef + // from worktree config, not the upstream relationship. + await gitExecFileAsync(['push', '--set-upstream', 'origin', 'HEAD'], { cwd: worktreePath }) + } catch (error) { + throw new Error(normalizeGitErrorMessage(error, 'push')) + } +} + +export async function gitPull(worktreePath: string): Promise { + // Why: plain `git pull` uses the user's configured pull strategy (merge by + // default) so diverged branches reconcile instead of erroring out. Conflicts + // surface through the existing conflict-resolution flow. + try { + await gitExecFileAsync(['pull'], { cwd: worktreePath }) + } catch (error) { + throw new Error(normalizeGitErrorMessage(error, 'pull')) + } +} + +export async function gitFetch(worktreePath: string): Promise { + try { + await gitExecFileAsync(['fetch', '--prune'], { cwd: worktreePath }) + } catch (error) { + throw new Error(normalizeGitErrorMessage(error, 'fetch')) + } +} diff --git a/src/main/git/upstream.test.ts b/src/main/git/upstream.test.ts new file mode 100644 index 00000000000..9f8a0ddc58e --- /dev/null +++ b/src/main/git/upstream.test.ts @@ -0,0 +1,62 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { gitExecFileAsyncMock } = vi.hoisted(() => ({ + gitExecFileAsyncMock: vi.fn() +})) + +vi.mock('./runner', () => ({ + gitExecFileAsync: gitExecFileAsyncMock +})) + +import { getUpstreamStatus } from './upstream' + +const missingTrackingRefError = new Error( + "fatal: ambiguous argument 'HEAD@{u}': unknown revision or path not in the working tree.\n" + + "Use '--' to separate paths from revisions, like this:\n" + + "'git [...] -- [...]'" +) + +describe('getUpstreamStatus', () => { + beforeEach(() => { + gitExecFileAsyncMock.mockReset() + }) + + it('returns upstream and ahead/behind counts when tracking is configured', async () => { + gitExecFileAsyncMock + .mockResolvedValueOnce({ stdout: 'origin/main\n' }) + .mockResolvedValueOnce({ stdout: '2\t3\n' }) + + const result = await getUpstreamStatus('/repo') + + expect(result).toEqual({ + hasUpstream: true, + upstreamName: 'origin/main', + ahead: 2, + behind: 3 + }) + }) + + it('returns hasUpstream=false when upstream is missing', async () => { + gitExecFileAsyncMock.mockRejectedValueOnce(new Error('fatal: no upstream configured')) + + const result = await getUpstreamStatus('/repo') + + expect(result).toEqual({ + hasUpstream: false, + ahead: 0, + behind: 0 + }) + }) + + it('returns hasUpstream=false when the configured tracking ref is missing', async () => { + gitExecFileAsyncMock.mockRejectedValueOnce(missingTrackingRefError) + + const result = await getUpstreamStatus('/repo') + + expect(result).toEqual({ + hasUpstream: false, + ahead: 0, + behind: 0 + }) + }) +}) diff --git a/src/main/git/upstream.ts b/src/main/git/upstream.ts new file mode 100644 index 00000000000..4b2146edcc6 --- /dev/null +++ b/src/main/git/upstream.ts @@ -0,0 +1,61 @@ +import type { GitUpstreamStatus } from '../../shared/types' +import { isNoUpstreamError, normalizeGitErrorMessage } from '../../shared/git-remote-error' +import { gitExecFileAsync } from './runner' + +export async function getUpstreamStatus(worktreePath: string): Promise { + try { + const { stdout: upstreamStdout } = await gitExecFileAsync( + ['rev-parse', '--abbrev-ref', 'HEAD@{u}'], + { + cwd: worktreePath + } + ) + const upstreamName = upstreamStdout.trim() + if (!upstreamName) { + return { hasUpstream: false, ahead: 0, behind: 0 } + } + + const { stdout: countsStdout } = await gitExecFileAsync( + ['rev-list', '--left-right', '--count', 'HEAD...@{u}'], + { + cwd: worktreePath + } + ) + + const tokens = countsStdout.trim().split(/\s+/) + if (tokens.length !== 2) { + // Why: 'rev-list --left-right --count HEAD...@{u}' must emit exactly two + // tokens; anything else (empty stdout, truncation, unexpected locale) is a + // real failure and must not be silently reported as "in sync" 0/0. + throw new Error(`Unexpected git rev-list output: ${JSON.stringify(countsStdout)}`) + } + const ahead = Number.parseInt(tokens[0]!, 10) + const behind = Number.parseInt(tokens[1]!, 10) + if (!Number.isFinite(ahead) || !Number.isFinite(behind) || ahead < 0 || behind < 0) { + throw new Error(`Unparseable git rev-list counts: ${JSON.stringify(countsStdout)}`) + } + + return { + hasUpstream: true, + upstreamName, + ahead, + behind + } + } catch (error) { + // Why: we only swallow clearly-no-upstream signals — that's an expected + // state, not a failure. Other errors (auth, corruption, "not a git + // repository", sparse-checkout) should surface to the user so they can + // act on them. The shared isNoUpstreamError helper intentionally omits + // broad phrases like "no such branch" to avoid masking real errors. + if (isNoUpstreamError(error)) { + return { + hasUpstream: false, + ahead: 0, + behind: 0 + } + } + // Why: parity with gitPush/gitPull/gitFetch — normalize before crossing + // the IPC boundary so renderers don't see execFile stderr preambles or local paths. + throw new Error(normalizeGitErrorMessage(error, 'upstream')) + } +} diff --git a/src/main/ipc/filesystem.ts b/src/main/ipc/filesystem.ts index 7f91ef70a31..c50fdb5a218 100644 --- a/src/main/ipc/filesystem.ts +++ b/src/main/ipc/filesystem.ts @@ -11,6 +11,7 @@ import type { GitBranchCompareResult, GitConflictOperation, GitDiffResult, + GitUpstreamStatus, GitStatusResult, MarkdownDocument, SearchOptions, @@ -37,6 +38,8 @@ import { getBranchCompare, getBranchDiff } from '../git/status' +import { getUpstreamStatus } from '../git/upstream' +import { gitFetch, gitPull, gitPush } from '../git/remote' import { getRemoteFileUrl } from '../git/repo' import { resolveAuthorizedPath, @@ -546,6 +549,76 @@ export function registerFilesystemHandlers(store: Store): void { } ) + ipcMain.handle( + 'git:upstreamStatus', + async ( + _event, + args: { worktreePath: string; connectionId?: string } + ): Promise => { + if (args.connectionId) { + const provider = getSshGitProvider(args.connectionId) + if (!provider) { + throw new Error(`No git provider for connection "${args.connectionId}"`) + } + return provider.getUpstreamStatus(args.worktreePath) + } + const worktreePath = await resolveRegisteredWorktreePath(args.worktreePath, store) + return getUpstreamStatus(worktreePath) + } + ) + + ipcMain.handle( + 'git:fetch', + async (_event, args: { worktreePath: string; connectionId?: string }): Promise => { + if (args.connectionId) { + const provider = getSshGitProvider(args.connectionId) + if (!provider) { + throw new Error(`No git provider for connection "${args.connectionId}"`) + } + return provider.fetchRemote(args.worktreePath) + } + const worktreePath = await resolveRegisteredWorktreePath(args.worktreePath, store) + await gitFetch(worktreePath) + } + ) + + ipcMain.handle( + 'git:push', + async ( + _event, + args: { worktreePath: string; publish?: boolean; connectionId?: string } + ): Promise => { + // Why: coerce to strict boolean at the IPC boundary so a malformed + // renderer payload (e.g. string 'false') can't silently enable + // --set-upstream mode. Mirrors the relay handler in src/relay/git-handler.ts. + const publish = args.publish === true + if (args.connectionId) { + const provider = getSshGitProvider(args.connectionId) + if (!provider) { + throw new Error(`No git provider for connection "${args.connectionId}"`) + } + return provider.pushBranch(args.worktreePath, publish) + } + const worktreePath = await resolveRegisteredWorktreePath(args.worktreePath, store) + await gitPush(worktreePath, publish) + } + ) + + ipcMain.handle( + 'git:pull', + async (_event, args: { worktreePath: string; connectionId?: string }): Promise => { + if (args.connectionId) { + const provider = getSshGitProvider(args.connectionId) + if (!provider) { + throw new Error(`No git provider for connection "${args.connectionId}"`) + } + return provider.pullBranch(args.worktreePath) + } + const worktreePath = await resolveRegisteredWorktreePath(args.worktreePath, store) + await gitPull(worktreePath) + } + ) + ipcMain.handle( 'git:branchDiff', async ( diff --git a/src/main/providers/ssh-git-provider.test.ts b/src/main/providers/ssh-git-provider.test.ts index 0dc4ac02060..0a0c759b515 100644 --- a/src/main/providers/ssh-git-provider.test.ts +++ b/src/main/providers/ssh-git-provider.test.ts @@ -128,6 +128,39 @@ describe('SshGitProvider', () => { expect(result).toEqual(compareResult) }) + it('getUpstreamStatus sends git.upstreamStatus request', async () => { + const upstreamResult = { hasUpstream: true, upstreamName: 'origin/main', ahead: 1, behind: 0 } + mux.request.mockResolvedValue(upstreamResult) + + const result = await provider.getUpstreamStatus('/home/user/repo') + expect(mux.request).toHaveBeenCalledWith('git.upstreamStatus', { + worktreePath: '/home/user/repo' + }) + expect(result).toEqual(upstreamResult) + }) + + it('pushBranch sends git.push request and forwards publish mode', async () => { + await provider.pushBranch('/home/user/repo', true) + expect(mux.request).toHaveBeenCalledWith('git.push', { + worktreePath: '/home/user/repo', + publish: true + }) + }) + + it('pullBranch sends git.pull request', async () => { + await provider.pullBranch('/home/user/repo') + expect(mux.request).toHaveBeenCalledWith('git.pull', { + worktreePath: '/home/user/repo' + }) + }) + + it('fetchRemote sends git.fetch request', async () => { + await provider.fetchRemote('/home/user/repo') + expect(mux.request).toHaveBeenCalledWith('git.fetch', { + worktreePath: '/home/user/repo' + }) + }) + it('getBranchDiff sends git.branchDiff request', async () => { const diffs = [{ kind: 'text', originalContent: '', modifiedContent: 'new' }] mux.request.mockResolvedValue(diffs) diff --git a/src/main/providers/ssh-git-provider.ts b/src/main/providers/ssh-git-provider.ts index a9fb8bc06a1..fde8b89b1a6 100644 --- a/src/main/providers/ssh-git-provider.ts +++ b/src/main/providers/ssh-git-provider.ts @@ -6,6 +6,7 @@ import type { GitDiffResult, GitBranchCompareResult, GitConflictOperation, + GitUpstreamStatus, GitWorktreeInfo } from '../../shared/types' @@ -83,6 +84,24 @@ export class SshGitProvider implements IGitProvider { })) as GitBranchCompareResult } + async getUpstreamStatus(worktreePath: string): Promise { + return (await this.mux.request('git.upstreamStatus', { + worktreePath + })) as GitUpstreamStatus + } + + async pushBranch(worktreePath: string, publish = false): Promise { + await this.mux.request('git.push', { worktreePath, publish }) + } + + async pullBranch(worktreePath: string): Promise { + await this.mux.request('git.pull', { worktreePath }) + } + + async fetchRemote(worktreePath: string): Promise { + await this.mux.request('git.fetch', { worktreePath }) + } + async getBranchDiff( worktreePath: string, baseRef: string, diff --git a/src/main/providers/types.ts b/src/main/providers/types.ts index 6f36505ec2a..f7468377e18 100644 --- a/src/main/providers/types.ts +++ b/src/main/providers/types.ts @@ -5,6 +5,7 @@ import type { GitDiffResult, GitBranchCompareResult, GitConflictOperation, + GitUpstreamStatus, GitWorktreeInfo, SearchOptions, SearchResult @@ -146,6 +147,10 @@ export type IGitProvider = { discardChanges(worktreePath: string, filePath: string): Promise detectConflictOperation(worktreePath: string): Promise getBranchCompare(worktreePath: string, baseRef: string): Promise + getUpstreamStatus(worktreePath: string): Promise + pushBranch(worktreePath: string, publish?: boolean): Promise + pullBranch(worktreePath: string): Promise + fetchRemote(worktreePath: string): Promise getBranchDiff( worktreePath: string, baseRef: string, diff --git a/src/preload/api-types.ts b/src/preload/api-types.ts index 71a83947045..a18aa14508e 100644 --- a/src/preload/api-types.ts +++ b/src/preload/api-types.ts @@ -20,6 +20,7 @@ import type { GitConflictOperation, GitDiffResult, GitStatusResult, + GitUpstreamStatus, GitHubAssignableUser, GitHubPRFile, GitHubPRFileContents, @@ -915,6 +916,17 @@ export type PreloadApi = { baseRef: string connectionId?: string }) => Promise + upstreamStatus: (args: { + worktreePath: string + connectionId?: string + }) => Promise + fetch: (args: { worktreePath: string; connectionId?: string }) => Promise + push: (args: { + worktreePath: string + publish?: boolean + connectionId?: string + }) => Promise + pull: (args: { worktreePath: string; connectionId?: string }) => Promise branchDiff: (args: { worktreePath: string compare: { diff --git a/src/preload/index.ts b/src/preload/index.ts index 3043c5715ea..39cdc0e4a95 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -17,6 +17,7 @@ import type { GitHubAssignableUser, GitHubCommentResult, GitHubWorkItem, + GitUpstreamStatus, GhosttyImportPreview, ListWorkItemsResult, MemorySnapshot, @@ -1523,6 +1524,19 @@ const api = { baseRef: string connectionId?: string }): Promise => ipcRenderer.invoke('git:branchCompare', args), + upstreamStatus: (args: { + worktreePath: string + connectionId?: string + }): Promise => ipcRenderer.invoke('git:upstreamStatus', args), + fetch: (args: { worktreePath: string; connectionId?: string }): Promise => + ipcRenderer.invoke('git:fetch', args), + push: (args: { + worktreePath: string + publish?: boolean + connectionId?: string + }): Promise => ipcRenderer.invoke('git:push', args), + pull: (args: { worktreePath: string; connectionId?: string }): Promise => + ipcRenderer.invoke('git:pull', args), branchDiff: (args: { worktreePath: string compare: { baseRef: string; baseOid: string; headOid: string; mergeBase: string } diff --git a/src/relay/git-handler.test.ts b/src/relay/git-handler.test.ts index 9541ffcca0a..a9c756cf7bd 100644 --- a/src/relay/git-handler.test.ts +++ b/src/relay/git-handler.test.ts @@ -1,3 +1,4 @@ +/* eslint-disable max-lines */ import { describe, expect, it, beforeEach, afterEach } from 'vitest' import { GitHandler } from './git-handler' import { RelayContext } from './context' @@ -43,6 +44,10 @@ describe('GitHandler', () => { expect(methods).toContain('git.discard') expect(methods).toContain('git.conflictOperation') expect(methods).toContain('git.branchCompare') + expect(methods).toContain('git.upstreamStatus') + expect(methods).toContain('git.fetch') + expect(methods).toContain('git.push') + expect(methods).toContain('git.pull') expect(methods).toContain('git.branchDiff') expect(methods).toContain('git.listWorktrees') expect(methods).toContain('git.addWorktree') @@ -245,6 +250,123 @@ describe('GitHandler', () => { }) }) + describe('remote operations', () => { + it('returns upstream divergence for tracked branches', async () => { + gitInit(tmpDir) + writeFileSync(path.join(tmpDir, 'base.txt'), 'base') + gitCommit(tmpDir, 'initial') + + const result = (await dispatcher.callRequest('git.upstreamStatus', { + worktreePath: tmpDir + })) as { hasUpstream: boolean; upstreamName?: string; ahead: number; behind: number } + + expect(result.hasUpstream).toBe(false) + expect(result.ahead).toBe(0) + expect(result.behind).toBe(0) + }) + + it('reports ahead/behind counts against a real upstream remote', async () => { + // Why: the upstream branch exists but isn't configured — exercise the + // full path through `git rev-parse HEAD@{u}` + `rev-list --left-right` + // so a future refactor can't silently break the happy-path roundtrip + // the no-upstream test doesn't cover. + const bareDir = mkdtempSync(path.join(tmpdir(), 'relay-git-bare-')) + try { + execFileSync('git', ['init', '--bare'], { cwd: bareDir, stdio: 'pipe' }) + + gitInit(tmpDir) + writeFileSync(path.join(tmpDir, 'base.txt'), 'base') + gitCommit(tmpDir, 'initial') + const firstSha = execFileSync('git', ['rev-parse', 'HEAD'], { + cwd: tmpDir, + encoding: 'utf-8' + }).trim() + const branch = execFileSync('git', ['rev-parse', '--abbrev-ref', 'HEAD'], { + cwd: tmpDir, + encoding: 'utf-8' + }).trim() + + execFileSync('git', ['remote', 'add', 'origin', bareDir], { + cwd: tmpDir, + stdio: 'pipe' + }) + execFileSync('git', ['push', '--set-upstream', 'origin', branch], { + cwd: tmpDir, + stdio: 'pipe' + }) + + // Add two local commits (ahead=2), then reset behind the remote tip + // and add one different commit so we end up ahead=1, behind=0 vs. + // upstream; then reset to first commit to produce behind=1 ahead=0. + writeFileSync(path.join(tmpDir, 'ahead1.txt'), 'a1') + gitCommit(tmpDir, 'ahead1') + writeFileSync(path.join(tmpDir, 'ahead2.txt'), 'a2') + gitCommit(tmpDir, 'ahead2') + // Push so remote is at ahead2 (so after we reset below, we are behind). + execFileSync('git', ['push', 'origin', branch], { cwd: tmpDir, stdio: 'pipe' }) + // Reset local back to the first commit: 0 ahead, 2 behind. + execFileSync('git', ['reset', '--hard', firstSha], { cwd: tmpDir, stdio: 'pipe' }) + + const result = (await dispatcher.callRequest('git.upstreamStatus', { + worktreePath: tmpDir + })) as { hasUpstream: boolean; upstreamName?: string; ahead: number; behind: number } + + expect(result.hasUpstream).toBe(true) + expect(result.upstreamName).toBe(`origin/${branch}`) + expect(result.ahead).toBe(0) + expect(result.behind).toBe(2) + } finally { + await fs.rm(bareDir, { recursive: true, force: true }) + } + }) + + it('fetches from a configured remote without throwing', async () => { + const bareDir = mkdtempSync(path.join(tmpdir(), 'relay-git-bare-')) + try { + execFileSync('git', ['init', '--bare'], { cwd: bareDir, stdio: 'pipe' }) + + gitInit(tmpDir) + writeFileSync(path.join(tmpDir, 'base.txt'), 'base') + gitCommit(tmpDir, 'initial') + const branch = execFileSync('git', ['rev-parse', '--abbrev-ref', 'HEAD'], { + cwd: tmpDir, + encoding: 'utf-8' + }).trim() + execFileSync('git', ['remote', 'add', 'origin', bareDir], { + cwd: tmpDir, + stdio: 'pipe' + }) + execFileSync('git', ['push', '--set-upstream', 'origin', branch], { + cwd: tmpDir, + stdio: 'pipe' + }) + + await expect( + dispatcher.callRequest('git.fetch', { worktreePath: tmpDir }) + ).resolves.not.toThrow() + + // FETCH_HEAD is created by any successful fetch, confirming the + // remote was actually contacted (not just silently no-op'd). + await expect(fs.access(path.join(tmpDir, '.git', 'FETCH_HEAD'))).resolves.toBeUndefined() + } finally { + await fs.rm(bareDir, { recursive: true, force: true }) + } + }) + + it('rethrows upstreamStatus failures that are not "no upstream configured"', async () => { + // Why: the handler's catch is narrowed to only swallow the expected + // "no upstream" signal. A non-repo path should surface its error rather + // than silently returning hasUpstream=false, which would mask auth or + // corruption failures in production. + const nonRepoDir = path.join(tmpDir, 'not-a-repo') + await fs.mkdir(nonRepoDir, { recursive: true }) + + await expect( + dispatcher.callRequest('git.upstreamStatus', { worktreePath: nonRepoDir }) + ).rejects.toThrow(/not a git repository/i) + }) + }) + describe('listWorktrees', () => { it('lists worktrees for a repo', async () => { gitInit(tmpDir) diff --git a/src/relay/git-handler.ts b/src/relay/git-handler.ts index 7f34fcfec83..49123ad2dc8 100644 --- a/src/relay/git-handler.ts +++ b/src/relay/git-handler.ts @@ -14,6 +14,7 @@ import { } from './git-handler-ops' import { commitChangesRelay, addWorktreeOp, removeWorktreeOp } from './git-handler-worktree-ops' import { detectConflictOperation, getStatusOp } from './git-handler-status-ops' +import { normalizeGitErrorMessage, isNoUpstreamError } from '../shared/git-remote-error' const execFileAsync = promisify(execFile) const MAX_GIT_BUFFER = 10 * 1024 * 1024 @@ -40,6 +41,10 @@ export class GitHandler { this.dispatcher.onRequest('git.discard', (p) => this.discard(p)) this.dispatcher.onRequest('git.conflictOperation', (p) => this.conflictOperation(p)) this.dispatcher.onRequest('git.branchCompare', (p) => this.branchCompare(p)) + this.dispatcher.onRequest('git.upstreamStatus', (p) => this.upstreamStatus(p)) + this.dispatcher.onRequest('git.fetch', (p) => this.fetch(p)) + this.dispatcher.onRequest('git.push', (p) => this.push(p)) + this.dispatcher.onRequest('git.pull', (p) => this.pull(p)) this.dispatcher.onRequest('git.branchDiff', (p) => this.branchDiff(p)) this.dispatcher.onRequest('git.listWorktrees', (p) => this.listWorktrees(p)) this.dispatcher.onRequest('git.addWorktree', (p) => this.addWorktree(p)) @@ -193,6 +198,104 @@ export class GitHandler { }) } + private async upstreamStatus(params: Record) { + const worktreePath = params.worktreePath as string + this.context.validatePath(worktreePath) + + try { + const { stdout: upstreamStdout } = await this.git( + ['rev-parse', '--abbrev-ref', 'HEAD@{u}'], + worktreePath + ) + const upstreamName = upstreamStdout.trim() + if (!upstreamName) { + return { hasUpstream: false, ahead: 0, behind: 0 } + } + const { stdout: countsStdout } = await this.git( + ['rev-list', '--left-right', '--count', 'HEAD...@{u}'], + worktreePath + ) + const tokens = countsStdout.trim().split(/\s+/) + if (tokens.length !== 2) { + // Why: 'rev-list --left-right --count HEAD...@{u}' must emit exactly two + // tokens; anything else (empty stdout, SSH transport truncation, unexpected + // locale) is a real failure and must not be silently reported as "in sync" 0/0. + throw new Error(`Unexpected git rev-list output: ${JSON.stringify(countsStdout)}`) + } + const ahead = Number.parseInt(tokens[0]!, 10) + const behind = Number.parseInt(tokens[1]!, 10) + if (!Number.isFinite(ahead) || !Number.isFinite(behind) || ahead < 0 || behind < 0) { + throw new Error(`Unparseable git rev-list counts: ${JSON.stringify(countsStdout)}`) + } + return { + hasUpstream: true, + upstreamName, + ahead, + behind + } + } catch (error) { + // Why: we only swallow the 'no upstream configured' error — that's an + // expected state, not a failure. Other errors (auth, corruption, network) + // should surface to the user so they can act on them. + if (isNoUpstreamError(error)) { + return { hasUpstream: false, ahead: 0, behind: 0 } + } + // Why: match fetch/push/pull normalization so execFile preamble and local + // paths don't leak to the renderer. + throw new Error(normalizeGitErrorMessage(error, 'upstream')) + } + } + + private async fetch(params: Record) { + const worktreePath = params.worktreePath as string + this.context.validatePath(worktreePath) + try { + await this.git(['fetch', '--prune'], worktreePath) + } catch (error) { + // Why: mirror the local gitFetch normalization so SSH users see the same + // actionable messages instead of raw git stderr (which varies across + // versions/locales and may embed credentials). + throw new Error(normalizeGitErrorMessage(error, 'fetch')) + } + } + + private async push(params: Record) { + const worktreePath = params.worktreePath as string + this.context.validatePath(worktreePath) + // Why: always pass --set-upstream (mirrors src/main/git/remote.ts). + // Orca's worktrees initially track the BASE ref (origin/main) because + // they're created via `git worktree add --track -b + // ` — without --set-upstream the local branch keeps tracking + // the base after the first push, so ahead/behind via @{u} measures + // "ahead of base" instead of "ahead of remote branch", and the UI's + // primary button never rotates from "Push" to "Commit". The `publish` + // flag is preserved in the param shape for IPC compatibility but is no + // longer load-bearing. On an already-published branch --set-upstream is + // a no-op for the tracking config. + void params.publish + try { + await this.git(['push', '--set-upstream', 'origin', 'HEAD'], worktreePath) + } catch (error) { + // Why: mirror the local gitPush normalization so SSH users see the same + // "non-fast-forward / pull first" guidance instead of raw git stderr. + throw new Error(normalizeGitErrorMessage(error, 'push')) + } + } + + private async pull(params: Record) { + const worktreePath = params.worktreePath as string + this.context.validatePath(worktreePath) + // Why: plain `git pull` uses the user's configured pull strategy (merge by + // default) so diverged branches reconcile instead of erroring out. + try { + await this.git(['pull'], worktreePath) + } catch (error) { + // Why: mirror the local gitPull normalization so SSH users see the same + // actionable messages instead of raw git stderr. + throw new Error(normalizeGitErrorMessage(error, 'pull')) + } + } + private async branchDiff(params: Record) { const worktreePath = params.worktreePath as string this.context.validatePath(worktreePath) 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 new file mode 100644 index 00000000000..0d84e6517ce --- /dev/null +++ b/src/renderer/src/components/right-sidebar/CommitArea.chevron-spinner.test.tsx @@ -0,0 +1,166 @@ +import { describe, expect, it, vi } from 'vitest' +import { RefreshCw } from 'lucide-react' +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' + +// Why: split out from CommitArea.test.tsx so each file stays under the +// project's max-lines budget. These tests cover the chevron spinner +// 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, + hasUnstagedChanges: false, + hasMessage: true, + hasUnresolvedConflicts: false, + isCommitting: false, + isRemoteOperationActive: false, + upstreamStatus: { hasUpstream: true, ahead: 0, behind: 0 }, + ...overrides + } +} + +function baseProps(overrides: Partial = {}) { + const inputs = buildInputs(overrides) + return { + commitMessage: 'feat: add commit area', + commitError: null as string | null, + isCommitting: inputs.isCommitting, + isRemoteOperationActive: inputs.isRemoteOperationActive, + inFlightRemoteOpKind: inputs.inFlightRemoteOpKind ?? null, + primaryAction: resolvePrimaryAction(inputs), + dropdownItems: resolveDropdownItems(inputs), + onCommitMessageChange: vi.fn(), + onPrimaryAction: vi.fn(), + onDropdownAction: vi.fn() as (kind: DropdownActionKind) => void + } +} + +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) + }) + + // 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) + }) + + // 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) + }) + + // 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) + }) +}) 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 new file mode 100644 index 00000000000..214fa42d2a9 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/CommitArea.primary-icons.test.tsx @@ -0,0 +1,129 @@ +import { describe, expect, it, vi } from 'vitest' +import { ArrowDown, ArrowDownUp, ArrowUp, CloudUpload } from 'lucide-react' +import { CommitArea } from './SourceControl' +import { Button } from '@/components/ui/button' +import { resolvePrimaryAction, type PrimaryActionInputs } from './source-control-primary-action' +import { resolveDropdownItems, type DropdownActionKind } from './source-control-dropdown-items' + +// Why: split out from CommitArea.test.tsx so each file stays under the +// project's max-lines budget. These tests cover the directional-icon +// 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, + hasUnstagedChanges: false, + hasMessage: true, + hasUnresolvedConflicts: false, + isCommitting: false, + isRemoteOperationActive: false, + upstreamStatus: { hasUpstream: true, ahead: 0, behind: 0 }, + ...overrides + } +} + +function baseProps(overrides: Partial = {}) { + const inputs = buildInputs(overrides) + return { + commitMessage: 'feat: add commit area', + commitError: null as string | null, + isCommitting: inputs.isCommitting, + isRemoteOperationActive: inputs.isRemoteOperationActive, + inFlightRemoteOpKind: inputs.inFlightRemoteOpKind ?? null, + primaryAction: resolvePrimaryAction(inputs), + dropdownItems: resolveDropdownItems(inputs), + onCommitMessageChange: vi.fn(), + onPrimaryAction: vi.fn(), + onDropdownAction: vi.fn() as (kind: DropdownActionKind) => void + } +} + +// Why: each remote primary kind is anchored by a directional icon — Push +// ↑, Pull ↓, Sync ↕, Publish ☁︎↑ — so the verb's direction is visible at a +// glance and the slot doesn't read as a row of identical pills. +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) + }) + + it('renders a down-arrow on a Pull primary', () => { + const props = baseProps({ + stagedCount: 0, + hasMessage: false, + upstreamStatus: { hasUpstream: true, ahead: 0, behind: 1 } + }) + const element = CommitArea(props) + expect(primaryHasIcon(element, ArrowDown)).toBe(true) + }) + + 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) + }) + + 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) + }) +}) diff --git a/src/renderer/src/components/right-sidebar/CommitArea.test.tsx b/src/renderer/src/components/right-sidebar/CommitArea.test.tsx index 6506aebc277..cf4d95bcddd 100644 --- a/src/renderer/src/components/right-sidebar/CommitArea.test.tsx +++ b/src/renderer/src/components/right-sidebar/CommitArea.test.tsx @@ -1,6 +1,9 @@ import { describe, expect, it, vi } from 'vitest' +import { Check, RefreshCw } from 'lucide-react' 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 @@ -35,26 +38,64 @@ function findTextarea(node: unknown): ReactElementLike { return found } -function findCommitButton(node: unknown): ReactElementLike { - let found: ReactElementLike | null = null +// 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) { - found = entry + buttons.push(entry) } }) - if (!found) { - throw new Error('commit button not found') + 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 - visit(node, (entry) => { - const children = entry.props?.children - if (typeof children === 'string' && children.includes(text)) { - found = true + 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 } @@ -63,50 +104,79 @@ 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() +function buildInputs(overrides: Partial = {}): PrimaryActionInputs { + return { + stagedCount: 1, + hasUnstagedChanges: false, + hasMessage: true, + hasUnresolvedConflicts: false, + isCommitting: false, + isRemoteOperationActive: false, + upstreamStatus: { hasUpstream: true, ahead: 0, behind: 0 }, + ...overrides + } +} + +function baseProps(overrides: Partial = {}) { + const inputs = buildInputs(overrides) + return { + commitMessage: 'feat: add commit area', + commitError: null as string | null, + isCommitting: inputs.isCommitting, + isRemoteOperationActive: inputs.isRemoteOperationActive, + inFlightRemoteOpKind: inputs.inFlightRemoteOpKind ?? null, + primaryAction: resolvePrimaryAction(inputs), + dropdownItems: resolveDropdownItems(inputs), + onCommitMessageChange: vi.fn(), + onPrimaryAction: vi.fn(), + onDropdownAction: vi.fn() as (kind: DropdownActionKind) => void + } } describe('CommitArea', () => { - it('disables commit button when no staged files', () => { - const element = CommitArea({ ...baseProps, stagedCount: 0 }) - const button = findCommitButton(element) + 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) }) - it('disables commit button when message is empty', () => { - const element = CommitArea({ ...baseProps, commitMessage: ' ' }) - const button = findCommitButton(element) + 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) }) - it('disables commit button when unresolved conflicts exist', () => { - const element = CommitArea({ ...baseProps, hasUnresolvedConflicts: true }) - const button = findCommitButton(element) + 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) }) - it('enables commit button with staged files, message, and no conflicts', () => { - const element = CommitArea(baseProps) - const button = findCommitButton(element) + 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('triggers commit when the button is clicked', () => { - const onCommitSuccess = vi.fn() - const element = CommitArea({ ...baseProps, onCommitSuccess }) - const button = findCommitButton(element) + 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(onCommitSuccess).toHaveBeenCalledTimes(1) + expect(onPrimaryAction).toHaveBeenCalledTimes(1) }) - it('clears message and keeps error hidden after successful commit lifecycle', async () => { + it('keeps the textarea enabled while the commit is in flight', () => { + const element = CommitArea({ + ...baseProps({ isCommitting: true }), + isCommitting: true + }) + const textarea = findTextarea(element) + expect(textarea.props.disabled).toBeFalsy() + }) + + 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 @@ -119,18 +189,25 @@ describe('CommitArea', () => { isCommitting = false }) - const render = () => - CommitArea({ - ...baseProps, + const render = () => { + const inputs = buildInputs({ + hasMessage: commitMessage.trim().length > 0, + isCommitting + }) + return CommitArea({ + ...baseProps(), commitMessage, commitError, isCommitting, - onCommitSuccess: () => { + primaryAction: resolvePrimaryAction(inputs), + dropdownItems: resolveDropdownItems(inputs), + onPrimaryAction: () => { void runCommit() } }) + } - const button = findCommitButton(render()) + const button = findPrimaryButton(render()) ;(button.props.onClick as () => void)() await flushPromises() @@ -140,7 +217,7 @@ describe('CommitArea', () => { expect(runCommit).toHaveBeenCalledTimes(1) }) - it('preserves message and shows error after failed commit lifecycle', async () => { + 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 @@ -154,18 +231,25 @@ describe('CommitArea', () => { isCommitting = false }) - const render = () => - CommitArea({ - ...baseProps, + const render = () => { + const inputs = buildInputs({ + hasMessage: commitMessage.trim().length > 0, + isCommitting + }) + return CommitArea({ + ...baseProps(), commitMessage, commitError, isCommitting, - onCommitSuccess: () => { + primaryAction: resolvePrimaryAction(inputs), + dropdownItems: resolveDropdownItems(inputs), + onPrimaryAction: () => { void runCommit() } }) + } - const button = findCommitButton(render()) + const button = findPrimaryButton(render()) ;(button.props.onClick as () => void)() await flushPromises() @@ -175,14 +259,141 @@ describe('CommitArea', () => { expect(runCommit).toHaveBeenCalledTimes(1) }) - it('locks the button while commit is in flight', () => { - const element = CommitArea({ ...baseProps, isCommitting: true }) - const button = findCommitButton(element) + 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) }) - it('shows an inline error message when commit fails', () => { - const element = CommitArea({ ...baseProps, commitError: 'pre-commit hook failed' }) + 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('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) + }) + + // 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) + }) + + it('shows a spinner on a Commit primary while the commit itself is in flight', () => { + const props = baseProps({ + stagedCount: 1, + hasMessage: true, + upstreamStatus: { hasUpstream: true, ahead: 0, behind: 0 }, + isCommitting: true + }) + const element = CommitArea({ ...props, isCommitting: true }) + expect(primaryHasSpinner(element)).toBe(true) + }) + + 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) + }) + + // 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) + }) + + // 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) + }) + + // 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) + }) + + 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) + }) + + // 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) + }) }) diff --git a/src/renderer/src/components/right-sidebar/SourceControl.tsx b/src/renderer/src/components/right-sidebar/SourceControl.tsx index a3ffba6143f..942c9ae5da6 100644 --- a/src/renderer/src/components/right-sidebar/SourceControl.tsx +++ b/src/renderer/src/components/right-sidebar/SourceControl.tsx @@ -1,7 +1,11 @@ /* eslint-disable max-lines */ import React, { useCallback, useEffect, useMemo, useRef, useState } from 'react' import { + ArrowDown, + ArrowDownUp, + ArrowUp, ChevronDown, + CloudUpload, Minus, Plus, RefreshCw, @@ -32,6 +36,23 @@ import { cn } from '@/lib/utils' import { isFolderRepo } from '../../../../shared/repo-kind' import { Tooltip, TooltipTrigger, TooltipContent, TooltipProvider } from '@/components/ui/tooltip' import { Button } from '@/components/ui/button' +import { + DropdownMenu, + DropdownMenuContent, + DropdownMenuItem, + DropdownMenuSeparator, + DropdownMenuTrigger +} from '@/components/ui/dropdown-menu' +import { + resolvePrimaryAction, + type PrimaryAction, + type RemoteOpKind +} from './source-control-primary-action' +import { + resolveDropdownItems, + type DropdownActionKind, + type DropdownEntry +} from './source-control-dropdown-items' import { BulkActionBar } from './BulkActionBar' import { useSourceControlSelection, type FlatEntry } from './useSourceControlSelection' import { @@ -70,6 +91,7 @@ import type { GitConflictKind, GitConflictOperation, GitStatusEntry, + GitUpstreamStatus, PRInfo } from '../../../../shared/types' import { STATUS_COLORS, STATUS_LABELS } from './status-display' @@ -88,6 +110,22 @@ const STATUS_ICONS: Record< copied: FilePlus } +// Why: directional signifiers ahead of each primary action label. Commit +// (✓) is affirmative; Push (↑) and Pull (↓) point in the direction data +// flows; Sync (↕) is bidirectional; Publish gets a cloud-up to distinguish +// the first-time publish from a subsequent push. Keeping the mapping +// outside the render function avoids reallocating it on every render. +const PRIMARY_ICONS: Record< + PrimaryAction['kind'], + React.ComponentType<{ className?: string; 'aria-hidden'?: boolean | 'true' | 'false' }> +> = { + commit: Check, + push: ArrowUp, + pull: ArrowDown, + sync: ArrowDownUp, + publish: CloudUpload +} + // Why: unstaged ("Changes") is listed first so that conflict files — which // are assigned area:'unstaged' by the parser — appear above "Staged Changes". // This keeps unresolved conflicts visible at the top of the list where the @@ -101,6 +139,10 @@ const SECTION_LABELS: Record<(typeof SECTION_ORDER)[number], string> = { const BRANCH_REFRESH_INTERVAL_MS = 5000 +// Why: the pure state-machine logic now lives in +// ./source-control-primary-action.ts. It is imported directly by callers +// (tests and other components) instead of going through this module. + type CommitDraftsByWorktree = Record export function readCommitDraftForWorktree( @@ -144,12 +186,19 @@ function SourceControlInner(): React.JSX.Element { const gitConflictOperationByWorktree = useAppStore((s) => s.gitConflictOperationByWorktree) const gitBranchChangesByWorktree = useAppStore((s) => s.gitBranchChangesByWorktree) const gitBranchCompareSummaryByWorktree = useAppStore((s) => s.gitBranchCompareSummaryByWorktree) + const remoteStatusesByWorktree = useAppStore((s) => s.remoteStatusesByWorktree) + const isRemoteOperationActive = useAppStore((s) => s.isRemoteOperationActive) + const inFlightRemoteOpKind = useAppStore((s) => s.inFlightRemoteOpKind) const prCache = useAppStore((s) => s.prCache) const fetchPRForBranch = useAppStore((s) => s.fetchPRForBranch) 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 fetchUpstreamStatus = useAppStore((s) => s.fetchUpstreamStatus) + const pushBranch = useAppStore((s) => s.pushBranch) + const pullBranch = useAppStore((s) => s.pullBranch) + const syncBranch = useAppStore((s) => s.syncBranch) + const fetchBranch = useAppStore((s) => s.fetchBranch) const revealInExplorer = useAppStore((s) => s.revealInExplorer) const trackConflictPath = useAppStore((s) => s.trackConflictPath) const openDiff = useAppStore((s) => s.openDiff) @@ -245,6 +294,13 @@ function SourceControlInner(): React.JSX.Element { const conflictOperation = activeWorktreeId ? (gitConflictOperationByWorktree[activeWorktreeId] ?? 'unknown') : 'unknown' + // Why: leave undefined until fetchUpstreamStatus resolves for this worktree. + // Substituting a synthetic { hasUpstream: false } flashes "Publish Branch" + // on every worktree switch — resolvePrimaryAction treats it as an + // unpublished branch until the real status lands a moment later. + const remoteStatus: GitUpstreamStatus | undefined = activeWorktreeId + ? remoteStatusesByWorktree[activeWorktreeId] + : undefined const rightSidebarOpen = useAppStore((s) => s.rightSidebarOpen) // Why: gate polling on both the active tab AND the sidebar being open. // The sidebar now stays mounted when closed (for performance), so without @@ -426,17 +482,19 @@ function SourceControlInner(): React.JSX.Element { // still runs. }, [activeWorktreeId]) - const handleCommit = useCallback(async (): Promise => { + // Why: returns true on success so compound actions ("Commit & Push" etc.) + // can skip the follow-up remote operation when the commit itself failed. + const handleCommit = useCallback(async (): Promise => { if (!activeWorktreeId || !worktreePath) { - return + return false } const message = commitMessage.trim() if (!message || grouped.staged.length === 0 || unresolvedConflicts.length > 0) { - return + return false } if (commitInFlightRef.current[activeWorktreeId]) { - return + return false } commitInFlightRef.current[activeWorktreeId] = true @@ -454,7 +512,7 @@ function SourceControlInner(): React.JSX.Element { ...prev, [activeWorktreeId]: commitResult.error ?? 'Commit failed' })) - return + return false } // Why: the textarea stays enabled during the in-flight commit (only the @@ -472,37 +530,210 @@ function SourceControlInner(): React.JSX.Element { 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) + // Why: flip branchSummary to 'loading' synchronously so the empty-state + // guard + // (!hasUncommittedEntries && branchSummary.status === 'ready' && + // branchEntries.length === 0) + // doesn't briefly read true between setGitStatus clearing the + // uncommitted list and the next branchCompare poll landing the new + // commit. Without this flip "No changes on this branch" flashes for + // the full poll-interval window. + // + // Then fire-and-forget refreshBranchCompare so the "Committed on + // Branch" section repopulates as soon as the IPC returns instead of + // waiting up to 5 seconds for the next poll. Unawaited on purpose: + // compound flows (runCompoundCommitAction) need handleCommit to + // resolve immediately so the push step starts without delay. Errors + // here are best-effort — the polling tick will retry. + if (effectiveBaseRef) { + beginGitBranchCompareRequest( + activeWorktreeId, + `${activeWorktreeId}:${effectiveBaseRef}:${Date.now()}:post-commit`, + effectiveBaseRef + ) } + void refreshBranchCompareRef.current() + return true } catch (error) { setCommitErrors((prev) => ({ ...prev, [activeWorktreeId]: error instanceof Error ? error.message : 'Commit failed' })) + return false } finally { setCommitInFlightByWorktree((prev) => ({ ...prev, [activeWorktreeId]: false })) commitInFlightRef.current[activeWorktreeId] = false } }, [ activeWorktreeId, + beginGitBranchCompareRequest, commitMessage, + effectiveBaseRef, grouped.staged.length, unresolvedConflicts.length, - setGitStatus, worktreePath ]) + // Why: a single dispatcher for every remote-only action the split button or + // chevron dropdown can trigger. Keeps the error-swallow pattern in one + // place — store slices already surface actionable toasts, so additional + // try/catch here would duplicate the notification. + const runRemoteAction = useCallback( + async (kind: 'push' | 'pull' | 'sync' | 'fetch' | 'publish'): Promise => { + if (!activeWorktreeId || !worktreePath) { + return + } + const connectionId = getConnectionId(activeWorktreeId) ?? undefined + try { + if (kind === 'publish') { + await pushBranch(activeWorktreeId, worktreePath, true, connectionId) + return + } + if (kind === 'push') { + await pushBranch(activeWorktreeId, worktreePath, false, connectionId) + return + } + if (kind === 'pull') { + await pullBranch(activeWorktreeId, worktreePath, connectionId) + return + } + if (kind === 'fetch') { + await fetchBranch(activeWorktreeId, worktreePath, connectionId) + return + } + await syncBranch(activeWorktreeId, worktreePath, connectionId) + } catch { + // Why: remote action failures are surfaced by editor-slice actions to keep + // one consistent toast path and avoid duplicate notifications in the UI. + } + }, + [activeWorktreeId, fetchBranch, pullBranch, pushBranch, syncBranch, worktreePath] + ) + + // Why: compound actions must commit first and only run the follow-up remote + // op when the commit succeeds. handleCommit's return value carries that + // signal — a failure leaves commitError populated and short-circuits here + // so we never push a commit the user didn't actually land. The primary + // button never takes this path (it always emits a single-action kind); + // compound flows are reached only from the dropdown, which offers + // 'commit_push' and 'commit_sync' (there is no 'Commit & Publish' row). + const runCompoundCommitAction = useCallback( + async (remoteKind: 'push' | 'sync'): Promise => { + const ok = await handleCommit() + if (!ok) { + return + } + await runRemoteAction(remoteKind) + }, + [handleCommit, runRemoteAction] + ) + + const hasUnstagedChanges = grouped.unstaged.length > 0 || grouped.untracked.length > 0 + + const primaryAction: PrimaryAction = useMemo( + () => + resolvePrimaryAction({ + stagedCount: grouped.staged.length, + hasUnstagedChanges, + hasMessage: commitMessage.trim().length > 0, + hasUnresolvedConflicts: unresolvedConflicts.length > 0, + isCommitting, + isRemoteOperationActive, + upstreamStatus: remoteStatus, + inFlightRemoteOpKind + }), + [ + commitMessage, + grouped.staged.length, + hasUnstagedChanges, + isCommitting, + isRemoteOperationActive, + inFlightRemoteOpKind, + remoteStatus, + unresolvedConflicts.length + ] + ) + + const dropdownItems: DropdownEntry[] = useMemo( + () => + resolveDropdownItems({ + stagedCount: grouped.staged.length, + hasUnstagedChanges, + hasMessage: commitMessage.trim().length > 0, + hasUnresolvedConflicts: unresolvedConflicts.length > 0, + isCommitting, + isRemoteOperationActive, + upstreamStatus: remoteStatus, + inFlightRemoteOpKind + }), + [ + commitMessage, + grouped.staged.length, + hasUnstagedChanges, + isCommitting, + isRemoteOperationActive, + inFlightRemoteOpKind, + remoteStatus, + unresolvedConflicts.length + ] + ) + + // Why: maps both the primary button click and any chevron dropdown item + // click to the right handler. Commit-ish kinds flow through handleCommit + // (which returns a boolean); compound actions use runCompoundCommitAction; + // pure remote actions go through runRemoteAction. + const handleActionInvoke = useCallback( + (kind: DropdownActionKind): void => { + switch (kind) { + case 'commit': + void handleCommit() + return + case 'commit_push': + void runCompoundCommitAction('push') + return + case 'commit_sync': + void runCompoundCommitAction('sync') + return + case 'push': + case 'pull': + case 'sync': + case 'fetch': + case 'publish': + void runRemoteAction(kind) + return + default: { + // Why: exhaustiveness check — if a new DropdownActionKind is added + // to the union, TypeScript will flag this assignment so we can't + // silently drop a case. + const _exhaustive: never = kind + void _exhaustive + } + } + }, + [handleCommit, runCompoundCommitAction, runRemoteAction] + ) + + // Why: PrimaryActionKind is narrowed to the single-action kinds the + // primary can emit ('commit' | 'push' | 'pull' | 'sync' | 'publish') — + // compound commit_* kinds are dropdown-only. An exhaustive switch keeps + // the mapping honest: if a new PrimaryActionKind is added, TypeScript + // lights up the missing case instead of silently falling through. + const handlePrimaryClick = useCallback((): void => { + switch (primaryAction.kind) { + case 'commit': + case 'push': + case 'pull': + case 'sync': + case 'publish': + handleActionInvoke(primaryAction.kind) + return + default: { + const _exhaustive: never = primaryAction.kind + void _exhaustive + } + } + }, [handleActionInvoke, primaryAction.kind]) + const handleOpenDiff = useCallback( (entry: GitStatusEntry) => { if (!activeWorktreeId || !worktreePath) { @@ -748,6 +979,18 @@ function SourceControlInner(): React.JSX.Element { return () => window.clearInterval(intervalId) }, [activeWorktreeId, effectiveBaseRef, isBranchVisible, isFolder, worktreePath]) + useEffect(() => { + // Why: gate on isBranchVisible so we don't spawn git processes while the + // sidebar is closed. Store-slice remote operations refresh upstream-status + // on success anyway, so the user's first sidebar open will show accurate + // state. + if (!activeWorktreeId || !worktreePath || isFolder || !isBranchVisible) { + return + } + const connectionId = getConnectionId(activeWorktreeId) ?? undefined + void fetchUpstreamStatus(activeWorktreeId, worktreePath, connectionId) + }, [activeWorktreeId, fetchUpstreamStatus, isBranchVisible, isFolder, worktreePath]) + const toggleSection = useCallback((section: string) => { setCollapsedSections((prev) => { const next = new Set(prev) @@ -1137,13 +1380,23 @@ function SourceControlInner(): React.JSX.Element { /> )} - {(scope === 'all' || scope === 'uncommitted') && hasUncommittedEntries && ( + {/* Why: keep CommitArea mounted across all source-control states. + The split-button primary rotates through Push / Pull / Sync / + Publish on a clean tree and disables Commit with a "Nothing to + commit" tooltip when nothing is staged — gating on + hasUncommittedEntries (added by #1448 for the older Commit-only + design) would unmount the whole action surface on clean + worktrees and tear it down mid-commit when the staged list + clears. */} + {(scope === 'all' || scope === 'uncommitted') && ( 0} commitMessage={commitMessage} commitError={commitError} isCommitting={isCommitting} + isRemoteOperationActive={isRemoteOperationActive} + inFlightRemoteOpKind={inFlightRemoteOpKind} + primaryAction={primaryAction} + dropdownItems={dropdownItems} onCommitMessageChange={(value) => { if (!activeWorktreeId) { return @@ -1152,9 +1405,8 @@ function SourceControlInner(): React.JSX.Element { writeCommitDraftForWorktree(prev, activeWorktreeId, value) ) }} - onCommitSuccess={() => { - void handleCommit() - }} + onPrimaryAction={handlePrimaryClick} + onDropdownAction={handleActionInvoke} /> )} @@ -1409,47 +1661,62 @@ const SourceControl = React.memo(SourceControlInner) export default SourceControl type CommitAreaProps = { - stagedCount: number - hasUnresolvedConflicts: boolean commitMessage: string commitError: string | null isCommitting: boolean + isRemoteOperationActive: boolean + inFlightRemoteOpKind: RemoteOpKind | null + primaryAction: PrimaryAction + dropdownItems: DropdownEntry[] onCommitMessageChange: (message: string) => void - onCommitSuccess: () => void + onPrimaryAction: () => void + onDropdownAction: (kind: DropdownActionKind) => void } export function CommitArea({ - stagedCount, - hasUnresolvedConflicts, commitMessage, commitError, isCommitting, + isRemoteOperationActive, + inFlightRemoteOpKind, + primaryAction, + dropdownItems, onCommitMessageChange, - onCommitSuccess + onPrimaryAction, + onDropdownAction }: CommitAreaProps): React.JSX.Element { // Why: cap at 12 rows so a pasted multi-page commit message doesn't push // the Commit button off-screen. The textarea keeps `resize-none` (matching // the existing style) — the browser scrolls internally past 12 rows. const rows = Math.min(12, Math.max(2, commitMessage.split('\n').length)) - const hasMessage = commitMessage.trim().length > 0 - const isCommitDisabled = - isCommitting || !hasMessage || stagedCount === 0 || hasUnresolvedConflicts + // Why: only spin the primary when its label matches what's actually + // running. resolvePrimaryAction overrides the primary kind to mirror the + // in-flight op (e.g. user picks Sync from the dropdown → primary becomes + // "Sync"), so the equality check spins the button for any primary- + // eligible remote op the user triggered. Background ops the primary + // doesn't show (Fetch) leave primaryAction.kind unchanged and the + // mismatch keeps the spinner off — the disabled state alone is enough + // signal there. Commit still spins on isCommitting because that path + // doesn't go through inFlightRemoteOpKind. + const showSpinner = + primaryAction.kind === 'commit' + ? isCommitting + : isRemoteOperationActive && primaryAction.kind === inFlightRemoteOpKind + // Why: when the primary doesn't host the in-flight op (e.g. Fetch, or any + // dropdown action that mismatches the primary's natural label) the click + // would otherwise be silent — the toast only fires on failure and a + // no-op fetch leaves status counts unchanged. Spinning the chevron gives + // the user immediate feedback that the action they picked is running, + // while still leaving the menu reachable to read the disabled-row + // tooltips. + const showChevronSpinner = (isCommitting || isRemoteOperationActive) && !showSpinner - // Why: when the button is disabled, the title surfaces the reason so the - // user doesn't have to guess why Commit is greyed out. Part-2 may extend - // this into a split button (primary action + dropdown for Push / Sync / - // Commit & Push); the label stays as a plain "Commit" here so the shape - // lines up cleanly with the forthcoming "Remote Updates" section beneath it. - let disabledReason: string | undefined - if (isCommitting) { - disabledReason = 'Commit in progress…' - } else if (hasUnresolvedConflicts) { - disabledReason = 'Resolve conflicts before committing' - } else if (stagedCount === 0) { - disabledReason = 'Stage at least one file to commit' - } else if (!hasMessage) { - disabledReason = 'Enter a commit message to commit' - } + // Why: each primary-kind label is anchored by a directional icon so the + // affirmative Commit (✓) reads distinctly from the remote-state labels + // sharing this slot — Push (↑), Pull (↓), Sync (↕), Publish (☁︎↑). The + // icon is decorative; the label and title attribute carry the meaning + // for assistive tech. + const PrimaryIcon = PRIMARY_ICONS[primaryAction.kind] return (
@@ -1462,20 +1729,81 @@ export function CommitArea({ aria-describedby={commitError ? 'commit-area-error' : undefined} className="mt-0.5 w-full resize-none rounded-md border border-border bg-background px-2 py-1.5 text-xs text-foreground outline-none placeholder:text-muted-foreground/70 focus-visible:ring-1 focus-visible:ring-ring" /> - {/* Why: match the "Squash and merge" button in PRActions - (size="xs", px-3 text-[11px]) so the sidebar has a consistent - action-button shape across Source Control and Checks. */} - + {/* Why: primary + chevron sit together as a visual split button so the + edit → commit → push loop stays in a single vertical band. The + chevron exposes the full action surface (fetch, pull, sync, + publish, compound commits) without forcing morphing labels to + carry every possible intent. */} +
+ {/* Why: match the "Squash and merge" button in PRActions + (size="xs", px-3 text-[11px]) so the sidebar has a consistent + action-button shape across Source Control and Checks. The primary + and chevron share a single rounded rectangle — rounded-r-none on + the primary and rounded-l-none + border-l on the chevron make the + pair read as one split button instead of two detached buttons. */} + + + + + + + {dropdownItems.map((entry, index) => + entry.kind === 'separator' ? ( + + ) : ( + { + if (entry.disabled) { + event.preventDefault() + return + } + onDropdownAction(entry.kind) + }} + > + {entry.label} + + ) + )} + + +
{commitError && ( // Why: role="alert" + aria-live="polite" lets screen readers announce // commit failures; the id ties the message to the textarea via diff --git a/src/renderer/src/components/right-sidebar/source-control-dropdown-items.test.ts b/src/renderer/src/components/right-sidebar/source-control-dropdown-items.test.ts new file mode 100644 index 00000000000..9da3f7caf2d --- /dev/null +++ b/src/renderer/src/components/right-sidebar/source-control-dropdown-items.test.ts @@ -0,0 +1,191 @@ +import { describe, expect, it } from 'vitest' +import { resolveDropdownItems } from './source-control-dropdown-items' +import type { PrimaryActionInputs } from './source-control-primary-action' + +// Why: a shared defaults object keeps each case row terse while making the +// "this is the one knob that differs from the baseline" intent obvious. +function inputs(overrides: Partial = {}): PrimaryActionInputs { + return { + stagedCount: 0, + hasUnstagedChanges: false, + hasMessage: false, + hasUnresolvedConflicts: false, + isCommitting: false, + isRemoteOperationActive: false, + upstreamStatus: undefined, + ...overrides + } +} + +describe('resolveDropdownItems', () => { + it('renders every row — Commit through Publish — for a staged, tracked, ahead+behind branch', () => { + const items = resolveDropdownItems( + inputs({ + stagedCount: 1, + hasMessage: true, + upstreamStatus: { hasUpstream: true, ahead: 2, behind: 3 } + }) + ) + const kinds = items.map((item) => item.kind) + expect(kinds).toEqual([ + 'commit', + 'commit_push', + 'commit_sync', + 'separator', + 'push', + 'pull', + 'sync', + 'fetch', + 'publish' + ]) + }) + + it('disables compound commit actions when no staged files', () => { + const items = resolveDropdownItems( + inputs({ upstreamStatus: { hasUpstream: true, ahead: 1, behind: 0 } }) + ) + const byKind = Object.fromEntries( + items.filter((e) => e.kind !== 'separator').map((e) => [e.kind, e]) + ) + expect(byKind.commit.disabled).toBe(true) + expect(byKind.commit_push.disabled).toBe(true) + expect(byKind.commit_sync.disabled).toBe(true) + }) + + it('disables push actions but keeps Fetch enabled when branch has no upstream', () => { + const items = resolveDropdownItems( + inputs({ + stagedCount: 1, + hasMessage: true, + upstreamStatus: { hasUpstream: false, ahead: 0, behind: 0 } + }) + ) + const byKind = Object.fromEntries( + items.filter((e) => e.kind !== 'separator').map((e) => [e.kind, e]) + ) + expect(byKind.push.disabled).toBe(true) + expect(byKind.commit_push.disabled).toBe(true) + expect(byKind.publish.disabled).toBe(false) + expect(byKind.fetch.disabled).toBe(false) + }) + + it('disables Publish Branch when branch already has an upstream', () => { + const items = resolveDropdownItems( + inputs({ + upstreamStatus: { hasUpstream: true, ahead: 0, behind: 0 } + }) + ) + const byKind = Object.fromEntries( + items.filter((e) => e.kind !== 'separator').map((e) => [e.kind, e]) + ) + expect(byKind.publish.disabled).toBe(true) + }) + + it('renders counts on action labels when > 0', () => { + const items = resolveDropdownItems( + inputs({ upstreamStatus: { hasUpstream: true, ahead: 3, behind: 2 } }) + ) + const byKind = Object.fromEntries( + items.filter((e) => e.kind !== 'separator').map((e) => [e.kind, e]) + ) + expect(byKind.push.label).toBe('Push (3)') + expect(byKind.pull.label).toBe('Pull (2)') + expect(byKind.sync.label).toBe('Sync (↓2 ↑3)') + }) + + it('omits counts from labels when ahead/behind are 0', () => { + const items = resolveDropdownItems( + inputs({ upstreamStatus: { hasUpstream: true, ahead: 0, behind: 0 } }) + ) + const byKind = Object.fromEntries( + items.filter((e) => e.kind !== 'separator').map((e) => [e.kind, e]) + ) + expect(byKind.push.label).toBe('Push') + expect(byKind.pull.label).toBe('Pull') + expect(byKind.sync.label).toBe('Sync') + }) + + it('locks every item while a remote op is running', () => { + const items = resolveDropdownItems( + inputs({ + isRemoteOperationActive: true, + stagedCount: 1, + hasMessage: true, + upstreamStatus: { hasUpstream: true, ahead: 2, behind: 3 } + }) + ) + for (const entry of items) { + if (entry.kind !== 'separator') { + expect(entry.disabled).toBe(true) + } + } + }) + + it('disables remote rows with a loading tooltip when upstreamStatus is undefined', () => { + // Why: mirrors the primary-action guard — while fetchUpstreamStatus is in + // flight we must not let the user click Publish on an already-tracked + // branch (which would re-run `git push -u` and clobber the upstream). + const items = resolveDropdownItems( + inputs({ stagedCount: 1, hasMessage: true, upstreamStatus: undefined }) + ) + const byKind = Object.fromEntries( + items.filter((e) => e.kind !== 'separator').map((e) => [e.kind, e]) + ) + const loadingBlocked = [ + 'commit_push', + 'commit_sync', + 'push', + 'pull', + 'sync', + 'fetch', + 'publish' + ] as const + for (const kind of loadingBlocked) { + expect(byKind[kind].disabled).toBe(true) + expect(byKind[kind].title).toBe('Checking branch status…') + } + // Commit itself does not depend on upstream — it remains enabled when + // staged + message are present and no commit is in flight. + expect(byKind.commit.disabled).toBe(false) + }) + + it('keeps Fetch enabled and surfaces publish-first tooltips when upstream is absent', () => { + // Why: sibling to the upstreamStatus=undefined test above. Once the fetch + // resolves to hasUpstream=false, the dropdown should explain that the + // user needs to publish first (rather than leaving the loading copy). + const items = resolveDropdownItems( + inputs({ upstreamStatus: { hasUpstream: false, ahead: 0, behind: 0 } }) + ) + const byKind = Object.fromEntries( + items.filter((e) => e.kind !== 'separator').map((e) => [e.kind, e]) + ) + expect(byKind.push.title).toBe('Publish the branch first to push commits') + expect(byKind.pull.title).toBe('Publish the branch first to pull commits') + expect(byKind.sync.title).toBe('Publish the branch first to sync commits') + expect(byKind.fetch.title).toBe('Fetch from remote without merging') + expect(byKind.fetch.disabled).toBe(false) + expect(byKind.publish.title).toBe('Publish this branch to origin') + expect(byKind.publish.disabled).toBe(false) + }) + + it('omits counts from compound commit labels even when ahead/behind are nonzero', () => { + // Why: the commit itself changes ahead/behind, so pre-commit counts would + // be stale the moment the action fires. Plain Push/Pull/Sync continue to + // carry counts because no commit is interposed. + const items = resolveDropdownItems( + inputs({ + stagedCount: 1, + hasMessage: true, + upstreamStatus: { hasUpstream: true, ahead: 2, behind: 3 } + }) + ) + const byKind = Object.fromEntries( + items.filter((e) => e.kind !== 'separator').map((e) => [e.kind, e]) + ) + expect(byKind.commit_push.label).toBe('Commit & Push') + expect(byKind.commit_sync.label).toBe('Commit & Sync') + // Sanity check: plain counterparts still carry counts. + expect(byKind.push.label).toBe('Push (2)') + expect(byKind.sync.label).toBe('Sync (↓3 ↑2)') + }) +}) diff --git a/src/renderer/src/components/right-sidebar/source-control-dropdown-items.ts b/src/renderer/src/components/right-sidebar/source-control-dropdown-items.ts new file mode 100644 index 00000000000..78a9eac4d14 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/source-control-dropdown-items.ts @@ -0,0 +1,212 @@ +// Why: split from source-control-primary-action because the primary and dropdown are independent derivations with different priority ladders; together they exceed the max-lines budget and tangle unrelated concerns. + +import type { PrimaryActionInputs } from './source-control-primary-action' + +export type DropdownActionKind = + | 'commit' + | 'commit_push' + | 'commit_sync' + | 'push' + | 'pull' + | 'sync' + | 'fetch' + | 'publish' + +export type DropdownItem = { + kind: DropdownActionKind + label: string + title: string + disabled: boolean +} + +export type DropdownSeparator = { kind: 'separator' } + +export type DropdownEntry = DropdownItem | DropdownSeparator + +function describePushCount(ahead: number): string { + return `Push ${ahead} commit${ahead === 1 ? '' : 's'}` +} + +function describePullCount(behind: number): string { + return `Pull ${behind} commit${behind === 1 ? '' : 's'}` +} + +function describeSyncCounts(ahead: number, behind: number): string { + return `Pull ${behind}, push ${ahead}` +} + +function formatCountLabel(base: string, count: number): string { + return count > 0 ? `${base} (${count})` : base +} + +function formatSyncLabel(base: string, ahead: number, behind: number): string { + if (ahead === 0 && behind === 0) { + return base + } + return `${base} (↓${behind} ↑${ahead})` +} + +/** + * Resolve the chevron dropdown items. Every item is always rendered so the + * menu shape stays stable across states; inapplicable rows are disabled + * with a tooltip reason rather than hidden. + */ +export function resolveDropdownItems(inputs: PrimaryActionInputs): DropdownEntry[] { + const { + stagedCount, + hasMessage, + hasUnresolvedConflicts, + isCommitting, + isRemoteOperationActive, + upstreamStatus + } = inputs + + const hasStaged = stagedCount > 0 + // Why: mirror the primary-action guard. When upstreamStatus is undefined, + // fetchUpstreamStatus hasn't resolved for this worktree yet. Collapsing that + // to hasUpstream=false would re-enable Publish Branch on an already-tracked + // branch during the post-worktree-switch transient window, and a click there + // would re-run `git push -u` and clobber the real upstream. Every + // upstream-dependent row disables itself while loading so the primary + // button's stable-frame guarantee extends to the dropdown. + const upstreamLoading = upstreamStatus === undefined + const hasUpstream = upstreamStatus?.hasUpstream ?? false + const ahead = upstreamStatus?.ahead ?? 0 + const behind = upstreamStatus?.behind ?? 0 + + // Why: any in-flight commit or remote operation should lock the whole menu. + // A running push shouldn't let a second pull/sync click queue up behind it + // on a stale status snapshot. + const globalBusy = isCommitting || isRemoteOperationActive + + const commitDisabledReason = (() => { + if (hasUnresolvedConflicts) { + return 'Resolve conflicts before committing' + } + if (!hasStaged) { + return 'Stage at least one file to commit' + } + if (!hasMessage) { + return 'Enter a commit message to commit' + } + return null + })() + const canCommit = !globalBusy && commitDisabledReason === null + const commitItem: DropdownItem = { + kind: 'commit', + label: 'Commit', + title: commitDisabledReason ?? 'Commit staged changes', + disabled: !canCommit + } + + // Why: compound commit labels omit counts because the commit itself changes + // ahead/behind — surfacing pre-commit numbers would be misleading (e.g. + // "Commit & Push (2)" would still read "2" after the commit lands at 3). + // On an unpublished branch, Commit & Push is unavailable: the user must + // Publish Branch first (offered via the primary action), after which + // Commit & Push becomes enabled. Tooltips mirror pushItem/syncItem copy + // so the "publish first" instruction is consistent across the menu. + const commitPushTitle = upstreamLoading + ? 'Checking branch status…' + : !hasUpstream + ? 'Publish the branch first to push commits' + : (commitDisabledReason ?? 'Commit staged changes and push') + const commitPushItem: DropdownItem = { + kind: 'commit_push', + label: 'Commit & Push', + title: commitPushTitle, + disabled: globalBusy || upstreamLoading || !hasUpstream || commitDisabledReason !== null + } + + const commitSyncTitle = (() => { + if (upstreamLoading) { + return 'Checking branch status…' + } + if (!hasUpstream) { + // Why: mirror pushItem/syncItem — direct the user to Publish Branch + // (the primary action on an unpublished branch) rather than naming a + // nonexistent compound action. + return 'Publish the branch first to sync commits' + } + if (behind === 0) { + return 'Nothing to pull — use Commit & Push instead' + } + return commitDisabledReason ?? 'Commit, then pull and push' + })() + const commitSyncItem: DropdownItem = { + kind: 'commit_sync', + label: 'Commit & Sync', + title: commitSyncTitle, + disabled: + globalBusy || upstreamLoading || !hasUpstream || behind === 0 || commitDisabledReason !== null + } + + const pushItem: DropdownItem = { + kind: 'push', + label: formatCountLabel('Push', ahead), + title: upstreamLoading + ? 'Checking branch status…' + : !hasUpstream + ? 'Publish the branch first to push commits' + : ahead === 0 + ? 'Nothing to push' + : describePushCount(ahead), + disabled: globalBusy || upstreamLoading || !hasUpstream || ahead === 0 + } + + const pullItem: DropdownItem = { + kind: 'pull', + label: formatCountLabel('Pull', behind), + title: upstreamLoading + ? 'Checking branch status…' + : !hasUpstream + ? 'Publish the branch first to pull commits' + : behind === 0 + ? 'Nothing to pull' + : describePullCount(behind), + disabled: globalBusy || upstreamLoading || !hasUpstream || behind === 0 + } + + const syncItem: DropdownItem = { + kind: 'sync', + label: formatSyncLabel('Sync', ahead, behind), + title: upstreamLoading + ? 'Checking branch status…' + : !hasUpstream + ? 'Publish the branch first to sync commits' + : ahead === 0 && behind === 0 + ? 'Branch is up to date' + : describeSyncCounts(ahead, behind), + disabled: globalBusy || upstreamLoading || !hasUpstream || (ahead === 0 && behind === 0) + } + + const fetchItem: DropdownItem = { + kind: 'fetch', + label: 'Fetch', + title: upstreamLoading ? 'Checking branch status…' : 'Fetch from remote without merging', + disabled: globalBusy || upstreamLoading + } + + const publishItem: DropdownItem = { + kind: 'publish', + label: 'Publish Branch', + title: upstreamLoading + ? 'Checking branch status…' + : hasUpstream + ? 'Branch is already published' + : 'Publish this branch to origin', + disabled: globalBusy || upstreamLoading || hasUpstream + } + + return [ + commitItem, + commitPushItem, + commitSyncItem, + { kind: 'separator' }, + pushItem, + pullItem, + syncItem, + fetchItem, + publishItem + ] +} diff --git a/src/renderer/src/components/right-sidebar/source-control-primary-action.test.ts b/src/renderer/src/components/right-sidebar/source-control-primary-action.test.ts new file mode 100644 index 00000000000..c6cce6b9a02 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/source-control-primary-action.test.ts @@ -0,0 +1,256 @@ +import { describe, expect, it } from 'vitest' +import { resolvePrimaryAction, type PrimaryActionInputs } from './source-control-primary-action' + +// Why: a shared defaults object keeps each case row terse while making the +// "this is the one knob that differs from the baseline" intent obvious. +function inputs(overrides: Partial = {}): PrimaryActionInputs { + return { + stagedCount: 0, + hasUnstagedChanges: false, + hasMessage: false, + hasUnresolvedConflicts: false, + isCommitting: false, + isRemoteOperationActive: false, + upstreamStatus: undefined, + ...overrides + } +} + +const upstreamInSync = { + hasUpstream: true, + upstreamName: 'origin/main', + ahead: 0, + behind: 0 +} + +describe('resolvePrimaryAction', () => { + it('returns a disabled Commit while a commit is in flight', () => { + const result = resolvePrimaryAction( + inputs({ isCommitting: true, stagedCount: 1, hasMessage: true }) + ) + expect(result).toEqual({ + kind: 'commit', + label: 'Commit', + title: 'Commit in progress…', + disabled: true + }) + }) + + it('keeps the contextual label but disables it while a remote op is in flight', () => { + const result = resolvePrimaryAction( + inputs({ + isRemoteOperationActive: true, + upstreamStatus: { hasUpstream: true, ahead: 0, behind: 3 } + }) + ) + expect(result).toEqual({ + kind: 'pull', + label: 'Pull', + title: 'Remote operation in progress…', + disabled: true + }) + }) + + // Why: when the user picks an action from the dropdown that doesn't match + // the primary's natural label, the primary must mirror the user-triggered + // action (label + kind) so the spinner narrates the right thing. Without + // this, picking "Sync" from the dropdown while the primary reads "Push" + // would spin a "Push" button that is not actually pushing. + it('mirrors the in-flight remote op kind on the primary while a remote op runs', () => { + const result = resolvePrimaryAction( + inputs({ + isRemoteOperationActive: true, + // Pre-click natural state would resolve to Push (ahead-only). + upstreamStatus: { hasUpstream: true, ahead: 3, behind: 0 }, + inFlightRemoteOpKind: 'sync' + }) + ) + expect(result).toEqual({ + kind: 'sync', + label: 'Sync', + title: 'Sync in progress…', + disabled: true + }) + }) + + it('mirrors an in-flight Pull on the primary even when natural label is Push', () => { + const result = resolvePrimaryAction( + inputs({ + isRemoteOperationActive: true, + upstreamStatus: { hasUpstream: true, ahead: 3, behind: 0 }, + inFlightRemoteOpKind: 'pull' + }) + ) + expect(result.kind).toBe('pull') + expect(result.label).toBe('Pull') + expect(result.title).toBe('Pull in progress…') + expect(result.disabled).toBe(true) + }) + + it('keeps the natural Publish label and tooltip when an in-flight Publish matches', () => { + // Why: when the in-flight kind matches the natural primary kind we + // preserve the candidate's full label (the natural state-machine row + // owns the wording) rather than overriding to a stripped-down version. + const result = resolvePrimaryAction( + inputs({ + isRemoteOperationActive: true, + upstreamStatus: { hasUpstream: false, ahead: 0, behind: 0 }, + inFlightRemoteOpKind: 'publish' + }) + ) + expect(result.kind).toBe('publish') + expect(result.label).toBe('Publish Branch') + expect(result.title).toBe('Remote operation in progress…') + expect(result.disabled).toBe(true) + }) + + // Why: Fetch is dropdown-only and never appears as the primary's label. + // When fetch is in flight, the primary must keep its natural label and + // tooltip so the button doesn't claim "Fetch" is a primary action — and + // the CommitArea spinner suppression hangs off the kind mismatch. + it('keeps the natural primary label when an in-flight Fetch is dropdown-only', () => { + const result = resolvePrimaryAction( + inputs({ + isRemoteOperationActive: true, + upstreamStatus: { hasUpstream: true, ahead: 3, behind: 0 }, + inFlightRemoteOpKind: 'fetch' + }) + ) + expect(result).toEqual({ + kind: 'push', + label: 'Push', + title: 'Remote operation in progress…', + disabled: true + }) + }) + + it('blocks commits while unresolved conflicts exist', () => { + const result = resolvePrimaryAction( + inputs({ hasUnresolvedConflicts: true, stagedCount: 2, hasMessage: true }) + ) + expect(result).toEqual({ + kind: 'commit', + label: 'Commit', + title: 'Resolve conflicts before committing', + disabled: true + }) + }) + + // Why: the primary button never compounds ("Commit & Push" etc.) — it + // always reads "Commit" whenever there are staged files with a message, + // regardless of remote state. Compound flows remain available from the + // dropdown; after the commit lands, the primary naturally rotates to + // Push / Sync / Publish Branch. + it('returns plain Commit for staged+message regardless of upstream state', () => { + const upstreams = [ + undefined, + { hasUpstream: false as const, ahead: 0, behind: 0 }, + { hasUpstream: true as const, ahead: 0, behind: 0 }, + { hasUpstream: true as const, ahead: 3, behind: 0 }, + { hasUpstream: true as const, ahead: 2, behind: 1 }, + { hasUpstream: true as const, ahead: 0, behind: 4 } + ] + for (const upstreamStatus of upstreams) { + const result = resolvePrimaryAction( + inputs({ stagedCount: 1, hasMessage: true, upstreamStatus }) + ) + expect(result.kind).toBe('commit') + expect(result.label).toBe('Commit') + expect(result.disabled).toBe(false) + } + }) + + it('disables Commit with a message-needed hint when staged but no message', () => { + const result = resolvePrimaryAction(inputs({ stagedCount: 1, hasMessage: false })) + expect(result).toEqual({ + kind: 'commit', + label: 'Commit', + title: 'Enter a commit message to commit', + disabled: true + }) + }) + + it('returns Publish Branch on a clean tree when no upstream exists', () => { + const result = resolvePrimaryAction( + inputs({ upstreamStatus: { hasUpstream: false, ahead: 0, behind: 0 } }) + ) + expect(result).toEqual({ + kind: 'publish', + label: 'Publish Branch', + title: 'Publish this branch to origin', + disabled: false + }) + }) + + it('returns Sync when clean + tracked + diverged both ways', () => { + const result = resolvePrimaryAction( + inputs({ upstreamStatus: { hasUpstream: true, ahead: 2, behind: 3 } }) + ) + expect(result).toEqual({ + kind: 'sync', + label: 'Sync', + title: 'Pull 3, push 2', + disabled: false + }) + }) + + it('returns Pull when clean + behind-only', () => { + const result = resolvePrimaryAction( + inputs({ upstreamStatus: { hasUpstream: true, ahead: 0, behind: 4 } }) + ) + expect(result.kind).toBe('pull') + expect(result.label).toBe('Pull') + expect(result.title).toBe('Pull 4 commits') + }) + + it('uses singular copy for a single-commit pull', () => { + const result = resolvePrimaryAction( + inputs({ upstreamStatus: { hasUpstream: true, ahead: 0, behind: 1 } }) + ) + expect(result.title).toBe('Pull 1 commit') + }) + + it('returns Push when clean + ahead-only', () => { + const result = resolvePrimaryAction( + inputs({ upstreamStatus: { hasUpstream: true, ahead: 3, behind: 0 } }) + ) + expect(result).toEqual({ + kind: 'push', + label: 'Push', + title: 'Push 3 commits', + disabled: false + }) + }) + + it('returns a disabled up-to-date Commit when tracked branch is clean and in sync', () => { + const result = resolvePrimaryAction(inputs({ upstreamStatus: upstreamInSync })) + expect(result).toEqual({ + kind: 'commit', + label: 'Commit', + title: 'Nothing to commit. Branch is up to date.', + disabled: true + }) + }) + + it('asks the user to stage files when unstaged changes exist on an in-sync branch', () => { + const result = resolvePrimaryAction( + inputs({ hasUnstagedChanges: true, upstreamStatus: upstreamInSync }) + ) + expect(result).toEqual({ + kind: 'commit', + label: 'Commit', + title: 'Stage at least one file to commit', + disabled: true + }) + }) + + it('returns a disabled Commit when clean and upstream status not yet resolved', () => { + const result = resolvePrimaryAction(inputs()) + expect(result).toEqual({ + kind: 'commit', + label: 'Commit', + title: 'Stage at least one file to commit', + disabled: true + }) + }) +}) diff --git a/src/renderer/src/components/right-sidebar/source-control-primary-action.ts b/src/renderer/src/components/right-sidebar/source-control-primary-action.ts new file mode 100644 index 00000000000..4b958e3198d --- /dev/null +++ b/src/renderer/src/components/right-sidebar/source-control-primary-action.ts @@ -0,0 +1,245 @@ +// Why: split from the combined primary+dropdown module because the primary and dropdown are independent derivations with different priority ladders; together they exceed the max-lines budget and tangle unrelated concerns. + +import type { GitUpstreamStatus } from '../../../../shared/types' + +// Why: this module owns the pure state-machine logic for the Source Control +// primary action (split button). Keeping the logic outside the React component +// makes it straightforward to unit-test each row of the priority table without +// spinning up a renderer. + +// Why: the primary button collapses to one-label-per-action. Compound +// kinds ('commit_push', 'commit_sync', 'commit_publish') live in +// DropdownActionKind only — never on the primary — so they are not part +// of this union. Narrowing the type here is load-bearing: it lets +// `handlePrimaryClick` switch exhaustively over only the kinds the +// primary can actually emit, and it kills the compound-commit branch in +// the isRemoteOperationActive tooltip below at compile time. +export type PrimaryActionKind = 'commit' | 'push' | 'pull' | 'sync' | 'publish' + +// Why: the in-flight remote op tracker stores which action the user actually +// triggered, so the primary button can mirror that label/spinner instead of +// claiming a stale or unrelated operation is running. 'fetch' is included +// because Fetch participates in the busy flag, but it is intentionally NOT +// in PrimaryActionKind — Fetch is dropdown-only, so when fetch is in flight +// the primary keeps its natural label and CommitArea suppresses the spinner. +export type RemoteOpKind = 'push' | 'pull' | 'sync' | 'fetch' | 'publish' + +export type PrimaryAction = { + kind: PrimaryActionKind + label: string + title: string + disabled: boolean +} + +export type PrimaryActionInputs = { + stagedCount: number + hasUnstagedChanges: boolean + hasMessage: boolean + hasUnresolvedConflicts: boolean + isCommitting: boolean + isRemoteOperationActive: boolean + upstreamStatus: GitUpstreamStatus | undefined + // Why: which remote op is currently running, when one is. null when no + // remote op is in flight. Used by the in-flight branch below to mirror + // the user-triggered action on the primary button instead of leaving a + // stale label that no longer matches what the slice is doing. + inFlightRemoteOpKind?: RemoteOpKind | null +} + +const PRIMARY_LABEL_BY_KIND: Record, string> = { + push: 'Push', + pull: 'Pull', + sync: 'Sync', + publish: 'Publish Branch' +} + +function describePushCount(ahead: number): string { + return `Push ${ahead} commit${ahead === 1 ? '' : 's'}` +} + +function describePullCount(behind: number): string { + return `Pull ${behind} commit${behind === 1 ? '' : 's'}` +} + +function describeSyncCounts(ahead: number, behind: number): string { + return `Pull ${behind}, push ${ahead}` +} + +/** + * Resolve the primary split-button action. + * + * Priority order mirrors the design-doc state machine: + * 1. In-flight commit locks the primary to a disabled "Commit". + * 2. In-flight remote operation keeps the current label but disables it. + * 3. Unresolved conflicts block the commit path entirely. + * 4. Has staged files + message → plain "Commit" (compound flows live in + * the dropdown; after the commit lands, step 6 rotates the primary to + * the appropriate single remote action). + * 5. Has staged files + no message → disabled "Commit" with a reason. + * 6. Clean tree → adaptive remote action (or disabled "Commit" no-op). + * + * An undefined upstream status means fetchUpstreamStatus has not resolved + * yet for this worktree. We return a disabled Commit so the button has a + * stable frame until the real status lands — otherwise it would flash + * through "Publish Branch" on every worktree switch. + */ +export function resolvePrimaryAction(inputs: PrimaryActionInputs): PrimaryAction { + const { + stagedCount, + hasUnstagedChanges, + hasMessage, + hasUnresolvedConflicts, + isCommitting, + isRemoteOperationActive, + upstreamStatus, + inFlightRemoteOpKind + } = inputs + + // 1. Commit in flight — lock the primary no matter what else is true. + if (isCommitting) { + return { + kind: 'commit', + label: 'Commit', + title: 'Commit in progress…', + disabled: true + } + } + + // 2. Remote op in flight — disable the primary. When the in-flight op + // is a primary-eligible kind that doesn't match the primary's natural + // label, mirror the in-flight kind so the user sees the action they + // actually triggered (e.g. "Sync" when they picked Sync from the + // dropdown while the primary's natural state was "Push"). When the + // in-flight op matches the primary's natural kind we keep the natural + // label so its richer detail (counts like "Push 3 commits") survives. + // Fetch and unknown in-flight kinds leave the primary's natural label + // intact; CommitArea's spinner suppresses itself via the kind-mismatch + // check so a non-matching in-flight op doesn't visually claim the + // primary as its host. + if (isRemoteOperationActive) { + const candidate = resolvePrimaryAction({ ...inputs, isRemoteOperationActive: false }) + const inFlightIsPrimaryKind = + inFlightRemoteOpKind === 'push' || + inFlightRemoteOpKind === 'pull' || + inFlightRemoteOpKind === 'sync' || + inFlightRemoteOpKind === 'publish' + + if (inFlightIsPrimaryKind && candidate.kind !== inFlightRemoteOpKind) { + const label = PRIMARY_LABEL_BY_KIND[inFlightRemoteOpKind] + return { + kind: inFlightRemoteOpKind, + label, + title: `${label} in progress…`, + disabled: true + } + } + + // Why: when the candidate label is "Commit", the generic "remote + // operation in progress…" tooltip mismatches the visible label. Point + // the user at the fact that the commit will wait, keeping the label and + // the explanation consistent. Conflicts take precedence over the remote + // tooltip because resolving them is the only action the user can start + // while the remote op runs. + const title = hasUnresolvedConflicts + ? 'Resolve conflicts before committing' + : candidate.kind === 'commit' + ? 'Remote operation in progress — try again once it finishes' + : 'Remote operation in progress…' + return { + ...candidate, + title, + disabled: true + } + } + + // 3. Unresolved conflicts block any commit path. + if (hasUnresolvedConflicts) { + return { + kind: 'commit', + label: 'Commit', + title: 'Resolve conflicts before committing', + disabled: true + } + } + + const hasStaged = stagedCount > 0 + + // 4. Has staged files + message → plain Commit. The primary button never + // compounds ("Commit & Push" etc.) — after the commit lands, the primary + // naturally rotates to the appropriate remote action (Push / Sync / + // Publish Branch) via step 6 below. Users who want the one-click + // compound flow can still reach it from the dropdown. + if (hasStaged && hasMessage) { + return { + kind: 'commit', + label: 'Commit', + title: 'Commit staged changes', + disabled: false + } + } + + // 5. Has staged files but no message — user just needs to type something. + if (hasStaged && !hasMessage) { + return { + kind: 'commit', + label: 'Commit', + title: 'Enter a commit message to commit', + disabled: true + } + } + + // 6. Clean tree + no staged files → adaptive remote action. + if (!upstreamStatus) { + return { + kind: 'commit', + label: 'Commit', + title: 'Stage at least one file to commit', + disabled: true + } + } + + if (!upstreamStatus.hasUpstream) { + return { + kind: 'publish', + label: 'Publish Branch', + title: 'Publish this branch to origin', + disabled: false + } + } + + if (upstreamStatus.ahead > 0 && upstreamStatus.behind > 0) { + return { + kind: 'sync', + label: 'Sync', + title: describeSyncCounts(upstreamStatus.ahead, upstreamStatus.behind), + disabled: false + } + } + if (upstreamStatus.behind > 0) { + return { + kind: 'pull', + label: 'Pull', + title: describePullCount(upstreamStatus.behind), + disabled: false + } + } + if (upstreamStatus.ahead > 0) { + return { + kind: 'push', + label: 'Push', + title: describePushCount(upstreamStatus.ahead), + disabled: false + } + } + + // Clean + tracked + in sync — distinguish truly clean from work that still + // needs staging before commit can proceed. + return { + kind: 'commit', + label: 'Commit', + title: hasUnstagedChanges + ? 'Stage at least one file to commit' + : 'Nothing to commit. Branch is up to date.', + disabled: true + } +} diff --git a/src/renderer/src/components/right-sidebar/useGitStatusPolling.ts b/src/renderer/src/components/right-sidebar/useGitStatusPolling.ts index 6a08376c519..97a0f88e861 100644 --- a/src/renderer/src/components/right-sidebar/useGitStatusPolling.ts +++ b/src/renderer/src/components/right-sidebar/useGitStatusPolling.ts @@ -13,6 +13,7 @@ export function useGitStatusPolling(): void { const activeWorktreeId = useAppStore((s) => s.activeWorktreeId) const fetchWorktrees = useAppStore((s) => s.fetchWorktrees) const setGitStatus = useAppStore((s) => s.setGitStatus) + const fetchUpstreamStatus = useAppStore((s) => s.fetchUpstreamStatus) const setConflictOperation = useAppStore((s) => s.setConflictOperation) const conflictOperationByWorktree = useAppStore((s) => s.gitConflictOperationByWorktree) const repoMap = useRepoMap() @@ -55,10 +56,11 @@ export function useGitStatusPolling(): void { connectionId })) as GitStatusResult setGitStatus(activeWorktreeId, status) + await fetchUpstreamStatus(activeWorktreeId, worktreePath, connectionId) } catch { // ignore } - }, [activeRepoSupportsGit, activeWorktreeId, worktreePath, setGitStatus]) + }, [activeRepoSupportsGit, activeWorktreeId, fetchUpstreamStatus, worktreePath, setGitStatus]) useEffect(() => { void fetchStatus() diff --git a/src/renderer/src/store/slices/editor.test.ts b/src/renderer/src/store/slices/editor.test.ts index 48a6bf8e7fc..ad26fb55b4a 100644 --- a/src/renderer/src/store/slices/editor.test.ts +++ b/src/renderer/src/store/slices/editor.test.ts @@ -582,6 +582,439 @@ describe('createEditorSlice combined diff exclusions', () => { }) }) +describe('createEditorSlice remote branch actions', () => { + const gitStatusMock = vi.fn() + const gitUpstreamStatusMock = vi.fn() + const gitPushMock = vi.fn() + const gitPullMock = vi.fn() + const gitFetchMock = vi.fn() + + beforeEach(() => { + toastErrorMock.mockReset() + gitStatusMock.mockReset() + gitUpstreamStatusMock.mockReset() + gitPushMock.mockReset() + gitPullMock.mockReset() + gitFetchMock.mockReset() + + gitStatusMock.mockResolvedValue({ entries: [], conflictOperation: 'unknown' }) + gitUpstreamStatusMock.mockResolvedValue({ + hasUpstream: true, + upstreamName: 'origin/main', + ahead: 1, + behind: 0 + }) + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + ;(globalThis as any).window = (globalThis as any).window ?? {} + // eslint-disable-next-line @typescript-eslint/no-explicit-any + ;(globalThis as any).window.api = { + git: { + status: gitStatusMock, + upstreamStatus: gitUpstreamStatusMock, + push: gitPushMock, + pull: gitPullMock, + fetch: gitFetchMock + } + } + }) + + it('stores upstream status per worktree', () => { + const store = createEditorStore() + + store.getState().setUpstreamStatus('wt-1', { + hasUpstream: true, + upstreamName: 'origin/main', + ahead: 2, + behind: 1 + }) + + expect(store.getState().remoteStatusesByWorktree['wt-1']).toEqual({ + hasUpstream: true, + upstreamName: 'origin/main', + ahead: 2, + behind: 1 + }) + }) + + it('runs pull and refreshes status + upstream on success', async () => { + const store = createEditorStore() + store.getState().setGitStatus('wt-1', { + conflictOperation: 'unknown', + entries: [{ path: 'src/app.ts', status: 'modified', area: 'unstaged' }] + }) + + await store.getState().pullBranch('wt-1', '/repo') + + expect(gitPullMock).toHaveBeenCalledWith({ + worktreePath: '/repo', + connectionId: undefined + }) + expect(toastErrorMock).not.toHaveBeenCalled() + }) + + it('surfaces a readable toast when pull reports local changes would be overwritten', async () => { + const store = createEditorStore() + gitPullMock.mockRejectedValueOnce( + new Error( + 'error: Your local changes to the following files would be overwritten by merge:\n\tsrc/app.ts\nPlease commit your changes or stash them before you merge.\nAborting' + ) + ) + + await expect(store.getState().pullBranch('wt-1', '/repo')).rejects.toThrow() + + expect(toastErrorMock).toHaveBeenCalledWith( + 'Pull blocked — commit or stash your local changes first.' + ) + }) + + it('runs publish branch through push with publish=true', async () => { + // Why: pushBranch no longer awaits a post-op git status / upstream + // refresh. The 3s git-status poll and the upstream-status effect in the + // sidebar reconcile state shortly after the IPC returns; keeping the + // mutation tight stops compound flows from stalling between commit and + // push. + const store = createEditorStore() + + await store.getState().pushBranch('wt-1', '/repo', true) + + expect(gitPushMock).toHaveBeenCalledWith({ + worktreePath: '/repo', + publish: true, + connectionId: undefined + }) + expect(store.getState().isRemoteOperationActive).toBe(false) + }) + + it('preserves actionable publish errors and avoids refresh on failure', async () => { + const store = createEditorStore() + const publishError = new Error( + 'Push rejected: remote has newer commits (non-fast-forward). Please pull or sync first.' + ) + gitPushMock.mockRejectedValueOnce(publishError) + + await expect(store.getState().pushBranch('wt-1', '/repo', true)).rejects.toThrow( + publishError.message + ) + + expect(toastErrorMock).toHaveBeenCalledWith( + 'Push rejected — remote has changes. Pull first, then try again.' + ) + expect(gitStatusMock).not.toHaveBeenCalled() + expect(gitUpstreamStatusMock).not.toHaveBeenCalled() + expect(store.getState().isRemoteOperationActive).toBe(false) + }) + + it('maps publish updates-were-rejected into a clean actionable toast', async () => { + const store = createEditorStore() + const publishError = new Error( + 'Updates were rejected because the tip of your current branch is behind its remote counterpart.' + ) + gitPushMock.mockRejectedValueOnce(publishError) + + await expect(store.getState().pushBranch('wt-1', '/repo', true)).rejects.toThrow( + publishError.message + ) + + expect(toastErrorMock).toHaveBeenCalledWith( + 'Push rejected — remote has changes. Pull first, then try again.' + ) + expect(gitStatusMock).not.toHaveBeenCalled() + expect(gitUpstreamStatusMock).not.toHaveBeenCalled() + expect(store.getState().isRemoteOperationActive).toBe(false) + }) + + it('maps raw publish wrapper errors into a cleaner actionable toast', async () => { + const store = createEditorStore() + const rawPublishError = new Error( + 'git push failed: Command failed: git push --set-upstream origin feature-branch\nremote: Repository not found.\nfatal: Authentication failed for https://github.com/acme/private-repo.git' + ) + gitPushMock.mockRejectedValueOnce(rawPublishError) + + await expect(store.getState().pushBranch('wt-1', '/repo', true)).rejects.toThrow( + rawPublishError.message + ) + + expect(toastErrorMock).toHaveBeenCalledWith( + 'Publish Branch failed. Authentication failed for https://github.com/acme/private-repo.git. Check your remote access and try again.' + ) + }) + + it('uses a fallback message for generic publish errors', async () => { + const store = createEditorStore() + const publishError = new Error('error: RPC failed; curl 56 GnuTLS recv error') + gitPushMock.mockRejectedValueOnce(publishError) + + await expect(store.getState().pushBranch('wt-1', '/repo', true)).rejects.toThrow( + publishError.message + ) + + expect(toastErrorMock).toHaveBeenCalledWith( + 'Publish Branch failed. Check your remote access and try again.' + ) + expect(gitStatusMock).not.toHaveBeenCalled() + expect(gitUpstreamStatusMock).not.toHaveBeenCalled() + expect(store.getState().isRemoteOperationActive).toBe(false) + }) + + it('maps non-fast-forward push errors into a clean actionable toast', async () => { + const store = createEditorStore() + const pushError = new Error( + 'Updates were rejected because the tip of your current branch is behind its remote counterpart.' + ) + gitPushMock.mockRejectedValueOnce(pushError) + + await expect(store.getState().pushBranch('wt-1', '/repo', false)).rejects.toThrow( + pushError.message + ) + + expect(toastErrorMock).toHaveBeenCalledWith( + 'Push rejected — remote has changes. Pull first, then try again.' + ) + expect(gitStatusMock).not.toHaveBeenCalled() + expect(gitUpstreamStatusMock).not.toHaveBeenCalled() + expect(store.getState().isRemoteOperationActive).toBe(false) + }) + + it('maps non-fast-forward keyword push errors into a clean actionable toast', async () => { + const store = createEditorStore() + const pushError = new Error('Push rejected: remote has newer commits (non-fast-forward).') + gitPushMock.mockRejectedValueOnce(pushError) + + await expect(store.getState().pushBranch('wt-1', '/repo', false)).rejects.toThrow( + pushError.message + ) + + expect(toastErrorMock).toHaveBeenCalledWith( + 'Push rejected — remote has changes. Pull first, then try again.' + ) + expect(gitStatusMock).not.toHaveBeenCalled() + expect(gitUpstreamStatusMock).not.toHaveBeenCalled() + expect(store.getState().isRemoteOperationActive).toBe(false) + }) + + it('uses a fallback message for generic push errors', async () => { + const store = createEditorStore() + const pushError = new Error('network timeout') + gitPushMock.mockRejectedValueOnce(pushError) + + await expect(store.getState().pushBranch('wt-1', '/repo', false)).rejects.toThrow( + pushError.message + ) + + expect(toastErrorMock).toHaveBeenCalledWith('Push failed. Check your connection and try again.') + expect(gitStatusMock).not.toHaveBeenCalled() + expect(gitUpstreamStatusMock).not.toHaveBeenCalled() + expect(store.getState().isRemoteOperationActive).toBe(false) + }) + + it('uses a fallback remote failure message when push rejects without Error', async () => { + const store = createEditorStore() + gitPushMock.mockRejectedValueOnce('failure') + + await expect(store.getState().pushBranch('wt-1', '/repo', false)).rejects.toBe('failure') + + expect(toastErrorMock).toHaveBeenCalledWith('Remote operation failed') + }) + + it('runs fetchBranch and clears the busy flag on success', async () => { + // Why: fetchBranch no longer awaits a post-op upstream refresh. + // useGitStatusPolling and the sidebar's upstream effect handle the + // reconcile, keeping the mutation focused on the single IPC. + const store = createEditorStore() + await store.getState().fetchBranch('wt-1', '/repo') + + expect(gitFetchMock).toHaveBeenCalledWith({ + worktreePath: '/repo', + connectionId: undefined + }) + expect(store.getState().isRemoteOperationActive).toBe(false) + expect(toastErrorMock).not.toHaveBeenCalled() + }) + + it('surfaces a toast and clears the busy flag when fetch fails', async () => { + const store = createEditorStore() + gitFetchMock.mockRejectedValueOnce(new Error('network timeout')) + + await expect(store.getState().fetchBranch('wt-1', '/repo')).rejects.toThrow('network timeout') + + expect(toastErrorMock).toHaveBeenCalledWith('network timeout') + expect(gitUpstreamStatusMock).not.toHaveBeenCalled() + expect(store.getState().isRemoteOperationActive).toBe(false) + }) + + it('preserves prior upstream status when fetch fails', async () => { + // Why: a transient upstream fetch failure (network blip, auth prompt + // timeout) must not erase the last-known ahead/behind counts — doing so + // would briefly flip the UI to an unknown/no-upstream state that + // misrepresents the branch's relationship to its remote. + const store = createEditorStore() + store.getState().setUpstreamStatus('wt-1', { + hasUpstream: true, + upstreamName: 'origin/main', + ahead: 2, + behind: 1 + }) + gitUpstreamStatusMock.mockRejectedValueOnce(new Error('transient failure')) + + await store.getState().fetchUpstreamStatus('wt-1', '/repo') + + expect(store.getState().remoteStatusesByWorktree['wt-1']).toEqual({ + hasUpstream: true, + upstreamName: 'origin/main', + ahead: 2, + behind: 1 + }) + }) + + it('keeps isRemoteOperationActive true while any remote op is in flight', async () => { + // Why: a bare boolean races across worktrees — if push A finishes while + // pull B is still running, flipping the flag off would prematurely + // re-enable B's button. The refcount-derived boolean must stay true + // until every in-flight remote op has finished. + const store = createEditorStore() + + let resolveA: () => void = () => {} + let resolveB: () => void = () => {} + gitPushMock + .mockImplementationOnce( + () => + new Promise((resolve) => { + resolveA = resolve + }) + ) + .mockImplementationOnce( + () => + new Promise((resolve) => { + resolveB = resolve + }) + ) + + const pushA = store.getState().pushBranch('wt-1', '/a') + // Kick microtasks so pushA has begun and flipped the flag on. + await Promise.resolve() + expect(store.getState().isRemoteOperationActive).toBe(true) + + const pushB = store.getState().pushBranch('wt-2', '/b') + await Promise.resolve() + expect(store.getState().isRemoteOperationActive).toBe(true) + + resolveA() + await pushA.catch(() => {}) + // B is still running, so the busy flag must remain true. + expect(store.getState().isRemoteOperationActive).toBe(true) + + resolveB() + await pushB.catch(() => {}) + expect(store.getState().isRemoteOperationActive).toBe(false) + }) + + it('runs syncBranch end-to-end (fetch+pull+push) on success', async () => { + // Why: syncBranch no longer awaits a post-op git status / upstream + // refresh. The polling layer reconciles state after the mutation + // returns; the in-mutation upstream-status read remains because it + // gates whether the inner push stage runs. + const store = createEditorStore() + + await store.getState().syncBranch('wt-1', '/repo') + + expect(gitFetchMock).toHaveBeenCalledWith({ + worktreePath: '/repo', + connectionId: undefined + }) + expect(gitPullMock).toHaveBeenCalledWith({ + worktreePath: '/repo', + connectionId: undefined + }) + // ahead=1 in the default mock, so sync pushes. + expect(gitPushMock).toHaveBeenCalledWith({ + worktreePath: '/repo', + connectionId: undefined + }) + expect(toastErrorMock).not.toHaveBeenCalled() + expect(store.getState().isRemoteOperationActive).toBe(false) + }) + + it('skips the inner push when syncBranch sees ahead=0', async () => { + // Why: guards against a no-op push round-trip after a pure fast-forward + // pull. See syncBranch's ahead>0 guard in editor.ts. + const store = createEditorStore() + gitUpstreamStatusMock.mockResolvedValueOnce({ + hasUpstream: true, + upstreamName: 'origin/main', + ahead: 0, + behind: 0 + }) + + await store.getState().syncBranch('wt-1', '/repo') + + expect(gitFetchMock).toHaveBeenCalled() + expect(gitPullMock).toHaveBeenCalled() + expect(gitPushMock).not.toHaveBeenCalled() + expect(toastErrorMock).not.toHaveBeenCalled() + }) + + it('surfaces a sync-labeled toast when syncBranch inner push fails with auth error', async () => { + // Why: the user invoked Sync — the toast must read "Sync failed..." even + // though the underlying step is push. Detail extraction still surfaces + // the actionable fatal/remote line so auth/protected-branch reasons stay + // visible. + const store = createEditorStore() + const authError = new Error( + 'git push failed: Command failed: git push origin feature\nremote: Repository not found.\nfatal: Authentication failed for https://github.com/acme/private-repo.git' + ) + gitPushMock.mockRejectedValueOnce(authError) + + await expect(store.getState().syncBranch('wt-1', '/repo')).rejects.toThrow(authError.message) + + expect(toastErrorMock).toHaveBeenCalledTimes(1) + expect(toastErrorMock).toHaveBeenCalledWith( + 'Sync failed. Authentication failed for https://github.com/acme/private-repo.git. Check your remote access and try again.' + ) + expect(store.getState().isRemoteOperationActive).toBe(false) + }) + + it('surfaces a single sync-labeled toast when syncBranch inner push is non-fast-forward', async () => { + // Why: under sync, NFF means the remote raced ahead between fetch and + // push — sync just pulled, so the bare "Pull first" guidance is wrong. + // Surface a sync-shaped retry hint instead. + const store = createEditorStore() + const pushError = new Error( + 'Updates were rejected because the tip of your current branch is behind its remote counterpart.' + ) + gitPushMock.mockRejectedValueOnce(pushError) + + await expect(store.getState().syncBranch('wt-1', '/repo')).rejects.toThrow(pushError.message) + + // No double-toast from the outer catch. + expect(toastErrorMock).toHaveBeenCalledTimes(1) + expect(toastErrorMock).toHaveBeenCalledWith( + 'Sync failed — remote moved while syncing. Try again.' + ) + }) + + it('surfaces the pull-blocked toast when syncBranch pull stage fails', async () => { + // Why: failures in sync's fetch/pull/status stages flow through the + // outer catch's generic path; push-specific framing only applies to + // the inner push stage. + const store = createEditorStore() + gitPullMock.mockRejectedValueOnce( + new Error( + 'error: Your local changes to the following files would be overwritten by merge:\n\tsrc/app.ts\nPlease commit your changes or stash them before you merge.\nAborting' + ) + ) + + await expect(store.getState().syncBranch('wt-1', '/repo')).rejects.toThrow() + + expect(toastErrorMock).toHaveBeenCalledTimes(1) + expect(toastErrorMock).toHaveBeenCalledWith( + 'Pull blocked — commit or stash your local changes first.' + ) + expect(gitPushMock).not.toHaveBeenCalled() + expect(store.getState().isRemoteOperationActive).toBe(false) + }) +}) + describe('createEditorSlice activateMarkdownLink', () => { const openUrlMock = vi.fn() const openFileUriMock = vi.fn() diff --git a/src/renderer/src/store/slices/editor.ts b/src/renderer/src/store/slices/editor.ts index 2a55f4364bd..33ec90fd583 100644 --- a/src/renderer/src/store/slices/editor.ts +++ b/src/renderer/src/store/slices/editor.ts @@ -14,10 +14,13 @@ import type { GitConflictStatusSource, GitStatusEntry, GitStatusResult, + GitUpstreamStatus, SearchResult, WorkspaceSessionState, WorkspaceVisibleTabType } from '../../../../shared/types' +import { stripCredentialsFromMessage } from '../../../../shared/git-remote-error' +import type { RemoteOpKind } from '@/components/right-sidebar/source-control-primary-action' export type DiffSource = | 'unstaged' @@ -265,6 +268,38 @@ export type EditorSlice = { // Why: lightweight updater for conflict operation only, used to clear stale // "Rebasing"/"Merging" badges on non-active worktrees without a full git status poll. setConflictOperation: (worktreeId: string, operation: GitConflictOperation) => void + remoteStatusesByWorktree: Record + setUpstreamStatus: (worktreeId: string, status: GitUpstreamStatus) => void + // Why: refcount-backed busy flag. A bare boolean races across worktrees — + // push on A finishing while pull on B is still in flight would flip the + // flag off and prematurely re-enable B's button. beginRemoteOperation / + // endRemoteOperation must be paired (begin at the start of the async + // operation, end in finally) so the derived boolean only flips to false + // once every in-flight remote op has finished. + isRemoteOperationActive: boolean + remoteOperationDepth: number + // Why: surfaces *which* remote op the user actually triggered so the + // primary button can mirror it (label + spinner) rather than leaving a + // stale label from before the dropdown click. Cleared when depth hits 0. + // Last-write-wins on concurrent ops, which is fine — the UI disables + // every entry while busy, so concurrent ops can't be initiated through it. + inFlightRemoteOpKind: RemoteOpKind | null + beginRemoteOperation: (kind?: RemoteOpKind) => void + endRemoteOperation: () => void + fetchUpstreamStatus: ( + worktreeId: string, + worktreePath: string, + connectionId?: string + ) => Promise + pushBranch: ( + worktreeId: string, + worktreePath: string, + publish?: boolean, + connectionId?: string + ) => Promise + pullBranch: (worktreeId: string, worktreePath: string, connectionId?: string) => Promise + syncBranch: (worktreeId: string, worktreePath: string, connectionId?: string) => Promise + fetchBranch: (worktreeId: string, worktreePath: string, connectionId?: string) => Promise gitBranchChangesByWorktree: Record gitBranchCompareSummaryByWorktree: Record gitBranchCompareRequestKeyByWorktree: Record @@ -342,6 +377,109 @@ function openWorkspaceEditorItem( return created?.id ?? fileId } +const REMOTE_OPERATION_FAILED_MESSAGE = 'Remote operation failed' +const REMOTE_OPERATION_DETAIL_MAX_LENGTH = 200 + +// Why: arbitrarily long git stderr lines (for instance, a multi-kilobyte +// server-side pre-receive hook message) should not blow up the toast. Cap the +// detail length so the toast stays readable; the underlying error is still +// rethrown for console/logs if a caller needs the full payload. +function truncateDetail(detail: string): string { + if (detail.length <= REMOTE_OPERATION_DETAIL_MAX_LENGTH) { + return detail + } + return `${detail.slice(0, REMOTE_OPERATION_DETAIL_MAX_LENGTH).trimEnd()}...` +} + +function extractPublishFailureDetail(message: string): string | null { + const normalized = message.replace(/\r\n/g, '\n') + const lines = normalized + .split('\n') + .map((line) => line.trim()) + .filter(Boolean) + const fatalLine = lines.find((line) => line.startsWith('fatal:')) + if (fatalLine) { + return truncateDetail(stripCredentialsFromMessage(fatalLine.slice('fatal:'.length).trim())) + } + const remoteLine = lines.find((line) => line.startsWith('remote:')) + if (remoteLine) { + return truncateDetail(stripCredentialsFromMessage(remoteLine.slice('remote:'.length).trim())) + } + return null +} + +function resolveRemoteOperationErrorMessage( + error: unknown, + options?: { publish?: boolean; isPush?: boolean; isSync?: boolean } +): string { + if (!(error instanceof Error)) { + return REMOTE_OPERATION_FAILED_MESSAGE + } + + // Why: under sync, the inner push runs *after* a successful pull, so a + // non-fast-forward at that point means the remote raced ahead between + // fetch and push — not "user forgot to pull". Saying "Pull first" would + // be wrong (sync just did). Branch isSync above the shared NFF path so + // sync gets a sync-shaped message instead of inheriting the push wording. + if ( + options?.isSync && + /non-fast-forward|fetch first|updates were rejected/i.test(error.message) + ) { + return 'Sync failed — remote moved while syncing. Try again.' + } + + // Why: non-fast-forward/rejected detection is shared across publish and push so + // both paths surface the same actionable toast regardless of operation type. + if (/non-fast-forward|fetch first|updates were rejected/i.test(error.message)) { + return 'Push rejected — remote has changes. Pull first, then try again.' + } + + // Why: `git pull` / merge refuses to run when the working tree has changes + // that would be overwritten; surface a single readable line instead of the + // multi-line git stderr (which lists every affected path). + if ( + /local changes.*would be overwritten|Please commit your changes or stash them/i.test( + error.message + ) + ) { + return 'Pull blocked — commit or stash your local changes first.' + } + + if (options?.publish) { + // Why: publish failures often bubble up as raw wrapped git/IPC payloads; this + // keeps the toast human-readable while preserving the actionable fatal reason. + const detail = extractPublishFailureDetail(error.message) + if (detail) { + return `Publish Branch failed. ${detail}. Check your remote access and try again.` + } + + return 'Publish Branch failed. Check your remote access and try again.' + } + + if (options?.isSync) { + // Why: the user invoked Sync — surface "Sync failed" rather than leaking + // the inner-step name ("Push failed"). Detail extraction matches push so + // auth / protected-branch reasons stay actionable. + const detail = extractPublishFailureDetail(error.message) + if (detail) { + return `Sync failed. ${detail}. Check your remote access and try again.` + } + return 'Sync failed. Check your connection and try again.' + } + + if (options?.isPush) { + // Why: surfacing fatal/remote lines from git is more actionable than a generic + // connection message for auth errors, protected branches, etc. + const detail = extractPublishFailureDetail(error.message) + if (detail) { + return `Push failed. ${detail}. Check your remote access and try again.` + } + return 'Push failed. Check your connection and try again.' + } + + return error.message +} + export const createEditorSlice: StateCreator = (set, get) => ({ editorDrafts: {}, setEditorDraft: (fileId, content) => @@ -1711,6 +1849,152 @@ export const createEditorSlice: StateCreator = (s }) } }), + remoteStatusesByWorktree: {}, + setUpstreamStatus: (worktreeId, status) => + set((s) => ({ + remoteStatusesByWorktree: { + ...s.remoteStatusesByWorktree, + [worktreeId]: status + } + })), + isRemoteOperationActive: false, + remoteOperationDepth: 0, + inFlightRemoteOpKind: null, + beginRemoteOperation: (kind) => + set((s) => ({ + remoteOperationDepth: s.remoteOperationDepth + 1, + isRemoteOperationActive: true, + // Why: last-write-wins. The UI disables every action entry while busy, + // so a second remote op can't be started from inside Orca. If a + // background caller (future) triggers one, surfacing the most recent + // kind matches "what the user is currently watching". + inFlightRemoteOpKind: kind ?? s.inFlightRemoteOpKind + })), + endRemoteOperation: () => + set((s) => { + const next = Math.max(0, s.remoteOperationDepth - 1) + return { + remoteOperationDepth: next, + isRemoteOperationActive: next > 0, + // Why: only clear the in-flight kind when no remote op remains. Until + // depth reaches 0 some other op is still running and its label/ + // spinner should keep displaying. + inFlightRemoteOpKind: next > 0 ? s.inFlightRemoteOpKind : null + } + }), + fetchUpstreamStatus: async (worktreeId, worktreePath, connectionId) => { + try { + const status = await window.api.git.upstreamStatus({ + worktreePath, + connectionId + }) + get().setUpstreamStatus(worktreeId, status) + } catch (error) { + // Why: on error we leave the prior status in place rather than writing a + // synthetic {hasUpstream:false} — that would flash 'Publish Branch' on a + // tracked branch after any transient IPC hiccup and a user click would + // re-publish, clobbering the upstream relationship. If the branch is + // genuinely newly unpublished, the polling effect will eventually correct + // the status on success. + console.error('fetchUpstreamStatus failed', error) + } + }, + pushBranch: async (worktreeId, worktreePath, publish = false, connectionId) => { + // Why: don't *await* a post-op git status / upstream refresh here. + // Chaining awaited refreshes inside the mutation extends the gap before + // compound flows (runCompoundCommitAction → runRemoteAction) reach the + // next step. But we still need a near-immediate upstream refresh so + // the primary button label rotates from "Push" to "Commit" as soon as + // ahead=0 — the polling layer is on a 3s interval, which is long + // enough to read as a stuck label. Solution: fire the upstream refresh + // as fire-and-forget so it doesn't block the mutation but updates the + // store as soon as the IPC resolves. + get().beginRemoteOperation(publish ? 'publish' : 'push') + try { + await window.api.git.push({ worktreePath, publish, connectionId }) + } catch (error) { + toast.error(resolveRemoteOperationErrorMessage(error, { publish, isPush: true })) + throw error + } finally { + get().endRemoteOperation() + } + void get().fetchUpstreamStatus(worktreeId, worktreePath, connectionId) + }, + pullBranch: async (worktreeId, worktreePath, connectionId) => { + get().beginRemoteOperation('pull') + try { + await window.api.git.pull({ worktreePath, connectionId }) + } catch (error) { + toast.error(resolveRemoteOperationErrorMessage(error)) + throw error + } finally { + get().endRemoteOperation() + } + void get().fetchUpstreamStatus(worktreeId, worktreePath, connectionId) + }, + syncBranch: async (worktreeId, worktreePath, connectionId) => { + // Why: same shape as pushBranch / pullBranch — fire-and-forget the + // post-op upstream refresh after the busy flag clears so the primary + // button label rotates immediately when the IPC resolves. + get().beginRemoteOperation('sync') + // Why: the inner push stage toasts with { isSync: true } so its failure + // surfaces a "Sync failed..." message instead of "Push failed..." — the + // user invoked Sync; the underlying push is implementation detail. The + // outer catch must then skip toasting to avoid a double-toast. + let pushStageToastShown = false + try { + await window.api.git.fetch({ worktreePath, connectionId }) + await window.api.git.pull({ worktreePath, connectionId }) + // Why: push only if the pull left local commits that aren't on the + // remote. After a merge pull the ahead count can be >0 (local commits + + // the new merge commit) or 0 (pure fast-forward), and we avoid a + // no-op push round-trip in the fast-forward case. + const upstreamStatus = await window.api.git.upstreamStatus({ + worktreePath, + connectionId + }) + if (upstreamStatus.ahead > 0) { + try { + await window.api.git.push({ worktreePath, connectionId }) + } catch (error) { + // Why: format under the user-facing operation (sync) rather than + // the inner step (push) — the user clicked Sync and shouldn't see + // a "Push failed" toast for a step they didn't directly invoke. + toast.error(resolveRemoteOperationErrorMessage(error, { isSync: true })) + pushStageToastShown = true + throw error + } + } + } catch (error) { + if (!pushStageToastShown) { + // Why: same isSync framing for fetch/pull/upstream-status failures so + // every sync failure path consistently reads as "Sync failed..." (or + // a more specific actionable message like "Pull blocked..." when the + // shared classifiers match first). + toast.error(resolveRemoteOperationErrorMessage(error, { isSync: true })) + } + throw error + } finally { + get().endRemoteOperation() + } + void get().fetchUpstreamStatus(worktreeId, worktreePath, connectionId) + }, + fetchBranch: async (worktreeId, worktreePath, connectionId) => { + // Why: same shape as pushBranch / pullBranch — fire-and-forget the + // upstream refresh after the busy flag clears. Fetch updates the + // remote refs only, so the visible signal we want is the new + // ahead/behind counts on the upstream-status payload. + get().beginRemoteOperation('fetch') + try { + await window.api.git.fetch({ worktreePath, connectionId }) + } catch (error) { + toast.error(resolveRemoteOperationErrorMessage(error)) + throw error + } finally { + get().endRemoteOperation() + } + void get().fetchUpstreamStatus(worktreeId, worktreePath, connectionId) + }, gitBranchChangesByWorktree: {}, gitBranchCompareSummaryByWorktree: {}, gitBranchCompareRequestKeyByWorktree: {}, diff --git a/src/shared/git-remote-error.test.ts b/src/shared/git-remote-error.test.ts new file mode 100644 index 00000000000..7a069e6f117 --- /dev/null +++ b/src/shared/git-remote-error.test.ts @@ -0,0 +1,22 @@ +import { describe, expect, it } from 'vitest' +import { isNoUpstreamError } from './git-remote-error' + +describe('isNoUpstreamError', () => { + it('treats a missing HEAD@{u} tracking ref as no upstream', () => { + const error = new Error( + "fatal: ambiguous argument 'HEAD@{u}': unknown revision or path not in the working tree.\n" + + "Use '--' to separate paths from revisions, like this:\n" + + "'git [...] -- [...]'" + ) + + expect(isNoUpstreamError(error)).toBe(true) + }) + + it('does not treat unrelated ambiguous refs as no upstream', () => { + const error = new Error( + "fatal: ambiguous argument 'feature': unknown revision or path not in the working tree." + ) + + expect(isNoUpstreamError(error)).toBe(false) + }) +}) diff --git a/src/shared/git-remote-error.ts b/src/shared/git-remote-error.ts new file mode 100644 index 00000000000..717da5d8ea0 --- /dev/null +++ b/src/shared/git-remote-error.ts @@ -0,0 +1,98 @@ +// Why: git's stderr often embeds the full remote URL, which can include a +// credential. Redact carefully: classic `user:password@` forms always carry +// a credential on any scheme (HTTPS, ssh://, git://, git+ssh://), but a +// lone `user@` is a credential ONLY for HTTP(S) (e.g. token-only PATs like +// `https://ghp_xxx@host`). For `ssh://git@host/...` the `git` login is +// required by the SSH remote — stripping it would produce a broken URL in +// the surfaced error and hide which remote actually failed. The two +// scheme-scoped patterns below keep SSH user-info intact while still +// scrubbing passwords on any scheme and HTTPS token-only forms. +const USERPASS_URL_PATTERN = /([a-z][a-z0-9+.-]*:\/\/)[^\s/@:]+:[^\s/@]+@/gi +const HTTPS_TOKEN_URL_PATTERN = /(https?:\/\/)[^\s/@:]+@/gi + +export function stripCredentialsFromMessage(message: string): string { + return message.replace(USERPASS_URL_PATTERN, '$1').replace(HTTPS_TOKEN_URL_PATTERN, '$1') +} + +function extractTailLine(message: string): string { + // Why: execFile rejections prefix the message with "Command failed: git ..." + // followed by the full stderr. The meaningful diagnostic is typically the + // last non-empty line; surfacing the full blob risks leaking local paths or + // environment details to the UI. + const lines = message + .split('\n') + .map((line) => line.trim()) + .filter((line) => line.length > 0) + return lines.at(-1) ?? message +} + +export type GitRemoteOperation = 'push' | 'pull' | 'fetch' | 'upstream' + +export function normalizeGitErrorMessage(error: unknown, operation?: GitRemoteOperation): string { + if (!(error instanceof Error)) { + return 'Git remote operation failed.' + } + + // Why: scrub credentials up-front so every downstream branch — including + // any future refactor that returns a substring of `raw` — operates on + // already-redacted text. The fast-path branches below return fixed + // literals today, but this hardens against accidental leakage later. + const raw = stripCredentialsFromMessage(error.message) + + // Why: `non-fast-forward` / `fetch first` can appear on fetch (after a + // remote force-push updating a tracking ref) and on pull (with + // `pull.ff=only`), so the "pull or sync first" guidance only makes sense + // when the user was actually pushing. For other operations, fall through + // to the generic tail-line path. `operation === undefined` keeps the + // legacy push-shaped message for any caller that hasn't been updated yet. + if ( + (operation === 'push' || operation === undefined) && + (raw.includes('non-fast-forward') || raw.includes('fetch first')) + ) { + return 'Push rejected: remote has newer commits (non-fast-forward). Please pull or sync first.' + } + + if (raw.includes('could not read Username') || raw.includes('Authentication failed')) { + return 'Authentication failed. Check your remote credentials.' + } + + if (raw.includes('Could not resolve host') || raw.includes('Network is unreachable')) { + return 'Network error. Check your connection.' + } + + if (raw.includes('no tracking information') || raw.includes('no upstream')) { + return 'Branch has no upstream. Publish the branch first.' + } + + // Fallthrough: extract only the tail stderr line. `raw` was already + // credential-scrubbed at the top of the function, so no further scrub needed. + return extractTailLine(raw) +} + +// Why: we only swallow clearly-no-upstream signals — an expected state, not a +// failure. Other errors ('not a git repository', 'corrupt', auth failures, +// sparse-checkout errors, etc.) must fall through to the caller so users can +// act on them. We explicitly avoid matching `HEAD@{u}` alone because execFile +// wraps errors with "Command failed: git rev-parse --abbrev-ref HEAD@{u}…", +// which would cause every non-repo/corrupt failure to spuriously look like +// no-upstream. We also do NOT match 'no such branch' — that phrase is too +// broad and can mask real errors on corrupt refs or sparse-checkout failures. +// Additionally gate the phrase match on a `fatal:` prefix: git always +// prefixes these diagnostics with `fatal:`, so requiring it prevents +// `HEAD does not point` / `Needed a single revision` from matching unrelated +// output (e.g. hook stdout, progress lines) and silently hiding real +// corrupt-repo / unborn-HEAD / ambiguous-ref failures behind a spurious +// "0 ahead / 0 behind, no upstream" UI state. The one ambiguous-ref +// exception is HEAD@{u}: git emits it when branch config points at a +// tracking ref that is missing locally, which is the same expected UX state. +const NO_UPSTREAM_PHRASE_PATTERN = + /no upstream configured|no tracking information|HEAD does not point|Needed a single revision|ambiguous argument 'HEAD@\{u\}'/i +const FATAL_PREFIX_PATTERN = /(^|\n)fatal:/i + +export function isNoUpstreamError(error: unknown): boolean { + if (!(error instanceof Error)) { + return false + } + const message = error.message + return FATAL_PREFIX_PATTERN.test(message) && NO_UPSTREAM_PHRASE_PATTERN.test(message) +} diff --git a/src/shared/types.ts b/src/shared/types.ts index f83802006d8..5184fe82f47 100644 --- a/src/shared/types.ts +++ b/src/shared/types.ts @@ -1588,6 +1588,17 @@ export type GitStatusResult = { conflictOperation: GitConflictOperation } +// Why: when hasUpstream is false, ahead/behind are placeholder zeros, not a +// "sync" signal — callers must check hasUpstream before treating 0/0 as in-sync. +// Kept separate from GitStatusResult because upstream lookup can fail for +// reasons unrelated to working-tree status (e.g., no upstream is expected). +export type GitUpstreamStatus = { + hasUpstream: boolean + upstreamName?: string + ahead: number + behind: number +} + export type GitBranchChangeStatus = 'modified' | 'added' | 'deleted' | 'renamed' | 'copied' export type GitBranchChangeEntry = {