From f2eb119c627322ee287fd34bf0539a343ddca4ad Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Tue, 31 Mar 2026 01:27:39 -0700 Subject: [PATCH] feat: display CI conflict details and improve merge-conflict UX (#234) - Add PRConflictSummary type and derive conflict metadata (base ref, commits behind, conflicting files) via local git merge-tree - Fix parseUnmergedEntry to use space-separated parsing (porcelain v2 unmerged entries are not tab-separated) - Enhance rebase detection by checking rebase-merge/ and rebase-apply/ directories in addition to REBASE_HEAD - Show conflicting files list and commits-behind count in ChecksPanel - Disable merge button with tooltip when PR has conflicts - Add operation banners (merge/rebase/cherry-pick) in SourceControl and WorktreeCard - Reorder section list so unstaged changes appear above staged - Extract conflict-summary, issues, and gh-utils into separate modules - Add 10s timeout to git fetch in conflict summary derivation - Fix test fixtures to match real porcelain v2 format --- src/main/git/status.test.ts | 6 +- src/main/git/status.ts | 41 +++-- src/main/github/client.test.ts | 125 +++++++++++++- src/main/github/client.ts | 159 +++--------------- src/main/github/conflict-summary.ts | 148 ++++++++++++++++ src/main/github/gh-utils.ts | 62 +++++++ src/main/github/issues.ts | 77 +++++++++ .../components/editor/ConflictComponents.tsx | 16 +- .../components/right-sidebar/ChecksPanel.tsx | 101 ++++++++--- .../components/right-sidebar/PRActions.tsx | 123 ++++++++------ .../right-sidebar/SourceControl.tsx | 81 ++++++--- .../right-sidebar/checks-helpers.tsx | 61 +++++-- .../src/components/sidebar/WorktreeCard.tsx | 24 ++- src/shared/types.ts | 8 + 14 files changed, 749 insertions(+), 283 deletions(-) create mode 100644 src/main/github/conflict-summary.ts create mode 100644 src/main/github/gh-utils.ts create mode 100644 src/main/github/issues.ts diff --git a/src/main/git/status.test.ts b/src/main/git/status.test.ts index b4099a00900..6cc8616fe81 100644 --- a/src/main/git/status.test.ts +++ b/src/main/git/status.test.ts @@ -165,7 +165,7 @@ describe('getStatus', () => { existsSyncMock.mockImplementation((target: string) => target.endsWith('MERGE_HEAD')) execFileAsyncMock.mockResolvedValueOnce({ stdout: - 'u UU N... 100644 100644 100644 100644 aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb cccccccccccccccccccccccccccccccccccccccc\tsrc/app.ts\n' + 'u UU N... 100644 100644 100644 100644 aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb cccccccccccccccccccccccccccccccccccccccc src/app.ts\n' }) const result = await getStatus('/repo') @@ -187,7 +187,7 @@ describe('getStatus', () => { existsSyncMock.mockReturnValue(false) execFileAsyncMock.mockResolvedValueOnce({ stdout: - 'u UD N... 100644 100644 000000 100644 aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb cccccccccccccccccccccccccccccccccccccccc\tsrc/deleted.ts\n' + 'u UD N... 100644 100644 000000 100644 aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb cccccccccccccccccccccccccccccccccccccccc src/deleted.ts\n' }) const result = await getStatus('/repo') @@ -208,7 +208,7 @@ describe('getStatus', () => { }) execFileAsyncMock.mockResolvedValueOnce({ stdout: - 'u AU N... 100644 100644 100644 100644 aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb cccccccccccccccccccccccccccccccccccccccc\tsrc/new.ts\n' + 'u AU N... 100644 100644 100644 100644 aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb cccccccccccccccccccccccccccccccccccccccc src/new.ts\n' }) const result = await getStatus('/repo') diff --git a/src/main/git/status.ts b/src/main/git/status.ts index c406fe0f128..35293146a29 100644 --- a/src/main/git/status.ts +++ b/src/main/git/status.ts @@ -130,18 +130,21 @@ async function parseUnmergedEntry( worktreePath: string, line: string ): Promise { - const tabIndex = line.indexOf('\t') - if (tabIndex === -1) { - return null - } - - const metadata = line.slice(0, tabIndex) - const filePath = line.slice(tabIndex + 1) - const parts = metadata.split(' ') + // Why: porcelain v2 unmerged entries are fully space-separated (like type-1 + // ordinary entries), NOT tab-separated. The format is: + // u

+ // The path starts at field index 10 and may contain spaces, so we join the + // remaining fields. The earlier tab-based parsing silently dropped all + // unmerged entries because the tab was never present. + const parts = line.split(' ') const xy = parts[1] const modeStage1 = parts[3] const modeStage2 = parts[4] const modeStage3 = parts[5] + const filePath = parts.slice(10).join(' ') + if (!filePath) { + return null + } // Why: submodule conflicts (mode 160000) are out of scope for v1. // Presenting them with normal file-conflict UX would be misleading because @@ -227,35 +230,45 @@ async function getConflictCompatibilityStatus( // cleaned up by the time we check. In that case we fall back to 'unknown' for // one poll cycle, which is acceptable. The renderer uses this to label the // merge summary ("Merge conflicts" vs "Rebase conflicts" vs generic "Conflicts"). +// +// Why we also check rebase-merge/ and rebase-apply/ directories: during a +// rebase, REBASE_HEAD only exists when the *current* step has a conflict. +// Between steps (or after resolving and before `git rebase --continue`), the +// rebase is still in progress but REBASE_HEAD is absent. The rebase-merge/ or +// rebase-apply/ directory persists for the entire rebase, so checking it +// catches the "rebase in progress, no conflicts on current step" case. async function detectConflictOperation(worktreePath: string): Promise { const gitDir = await resolveGitDir(worktreePath) const mergeHead = path.join(gitDir, 'MERGE_HEAD') const rebaseHead = path.join(gitDir, 'REBASE_HEAD') const cherryPickHead = path.join(gitDir, 'CHERRY_PICK_HEAD') + const rebaseMergeDir = path.join(gitDir, 'rebase-merge') + const rebaseApplyDir = path.join(gitDir, 'rebase-apply') let hasMergeHead = false let hasRebaseHead = false let hasCherryPickHead = false + let hasRebaseDir = false try { hasMergeHead = existsSync(mergeHead) hasRebaseHead = existsSync(rebaseHead) hasCherryPickHead = existsSync(cherryPickHead) + hasRebaseDir = existsSync(rebaseMergeDir) || existsSync(rebaseApplyDir) } catch { return 'unknown' } - if (Number(hasMergeHead) + Number(hasRebaseHead) + Number(hasCherryPickHead) !== 1) { - return 'unknown' - } - if (hasMergeHead) { return 'merge' } - if (hasRebaseHead) { + if (hasRebaseHead || hasRebaseDir) { return 'rebase' } - return 'cherry-pick' + if (hasCherryPickHead) { + return 'cherry-pick' + } + return 'unknown' } async function resolveGitDir(worktreePath: string): Promise { diff --git a/src/main/github/client.test.ts b/src/main/github/client.test.ts index cea82707d7d..7ab88b8b655 100644 --- a/src/main/github/client.test.ts +++ b/src/main/github/client.test.ts @@ -33,7 +33,11 @@ describe('getPRForBranch', () => { statusCheckRollup: [], updatedAt: '2026-03-28T00:00:00Z', isDraft: false, - mergeable: 'MERGEABLE' + mergeable: 'MERGEABLE', + baseRefName: 'main', + headRefName: 'feature/test', + baseRefOid: 'base-oid', + headRefOid: 'head-oid' } ]) }) @@ -59,7 +63,7 @@ describe('getPRForBranch', () => { '--limit', '1', '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable' + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid' ], { cwd: '/repo-root', encoding: 'utf-8' } ) @@ -78,7 +82,11 @@ describe('getPRForBranch', () => { statusCheckRollup: [], updatedAt: '2026-03-28T00:00:00Z', isDraft: true, - mergeable: 'CONFLICTING' + mergeable: 'CONFLICTING', + baseRefName: 'main', + headRefName: 'feature/test', + baseRefOid: 'base-oid', + headRefOid: 'head-oid' }) }) @@ -92,7 +100,7 @@ describe('getPRForBranch', () => { 'view', 'feature/test', '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable' + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid' ], { cwd: '/non-github-repo', encoding: 'utf-8' } ) @@ -101,6 +109,115 @@ describe('getPRForBranch', () => { expect(pr?.mergeable).toBe('CONFLICTING') }) + it('derives a read-only conflict summary for conflicting PRs when the base ref exists locally', async () => { + execFileAsyncMock + .mockResolvedValueOnce({ stdout: 'git@github.com:acme/widgets.git\n' }) + .mockResolvedValueOnce({ + stdout: JSON.stringify([ + { + number: 42, + title: 'Fix PR discovery', + state: 'OPEN', + url: 'https://github.com/acme/widgets/pull/42', + statusCheckRollup: [], + updatedAt: '2026-03-28T00:00:00Z', + isDraft: false, + mergeable: 'CONFLICTING', + baseRefName: 'main', + headRefName: 'feature/test', + baseRefOid: 'base-oid', + headRefOid: 'head-oid' + } + ]) + }) + .mockResolvedValueOnce({ stdout: '' }) + .mockResolvedValueOnce({ stdout: 'latest-base-oid\n' }) + .mockResolvedValueOnce({ stdout: 'merge-base-oid\n' }) + .mockResolvedValueOnce({ stdout: '3\n' }) + .mockResolvedValueOnce({ stdout: 'result-tree-oid\u0000src/a.ts\u0000src/b.ts\u0000' }) + + const pr = await getPRForBranch('/repo-root', 'feature/test') + + expect(pr?.conflictSummary).toEqual({ + baseRef: 'main', + baseCommit: 'latest-', + commitsBehind: 3, + files: ['src/a.ts', 'src/b.ts'] + }) + }) + + it('keeps conflicted file paths when git merge-tree exits 1 with stdout', async () => { + execFileAsyncMock + .mockResolvedValueOnce({ stdout: 'git@github.com:acme/widgets.git\n' }) + .mockResolvedValueOnce({ + stdout: JSON.stringify([ + { + number: 42, + title: 'Fix PR discovery', + state: 'OPEN', + url: 'https://github.com/acme/widgets/pull/42', + statusCheckRollup: [], + updatedAt: '2026-03-28T00:00:00Z', + isDraft: false, + mergeable: 'CONFLICTING', + baseRefName: 'main', + headRefName: 'feature/test', + baseRefOid: 'base-oid', + headRefOid: 'head-oid' + } + ]) + }) + .mockResolvedValueOnce({ stdout: '' }) + .mockResolvedValueOnce({ stdout: 'latest-base-oid\n' }) + .mockResolvedValueOnce({ stdout: 'merge-base-oid\n' }) + .mockResolvedValueOnce({ stdout: '2\n' }) + .mockRejectedValueOnce({ + stdout: 'result-tree-oid\u0000src/conflict.ts\u0000' + }) + + const pr = await getPRForBranch('/repo-root', 'feature/test') + + expect(pr?.conflictSummary?.files).toEqual(['src/conflict.ts']) + }) + + it('falls back to GitHub baseRefOid when fetching or resolving the base ref fails', async () => { + execFileAsyncMock + .mockResolvedValueOnce({ stdout: 'git@github.com:acme/widgets.git\n' }) + .mockResolvedValueOnce({ + stdout: JSON.stringify([ + { + number: 42, + title: 'Fix PR discovery', + state: 'OPEN', + url: 'https://github.com/acme/widgets/pull/42', + statusCheckRollup: [], + updatedAt: '2026-03-28T00:00:00Z', + isDraft: false, + mergeable: 'CONFLICTING', + baseRefName: 'main', + headRefName: 'feature/test', + baseRefOid: 'base-oid', + headRefOid: 'head-oid' + } + ]) + }) + .mockRejectedValueOnce(new Error('fetch failed')) + .mockRejectedValueOnce(new Error('missing refs/remotes/origin/main')) + .mockRejectedValueOnce(new Error('missing origin/main')) + .mockResolvedValueOnce({ stdout: 'merge-base-oid\n' }) + .mockResolvedValueOnce({ stdout: '1\n' }) + .mockResolvedValueOnce({ stdout: 'result-tree-oid\u0000src/fallback.ts\u0000' }) + + const pr = await getPRForBranch('/repo-root', 'feature/test') + + expect(pr?.conflictSummary).toEqual({ + baseRef: 'main', + baseCommit: 'base-oi', + commitsBehind: 1, + files: ['src/fallback.ts'] + }) + }) + it('returns null for empty branch (e.g. during rebase with detached HEAD)', async () => { const pr = await getPRForBranch('/repo-root', '') expect(pr).toBeNull() diff --git a/src/main/github/client.ts b/src/main/github/client.ts index 24b6420e00b..a9ef3ea1509 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -1,74 +1,17 @@ -import { execFile } from 'child_process' -import { promisify } from 'util' -import type { PRInfo, PRMergeableState, IssueInfo, PRCheckDetail } from '../../shared/types' +import type { PRInfo, PRMergeableState, PRCheckDetail } from '../../shared/types' +import { getPRConflictSummary } from './conflict-summary' +import { execFileAsync, acquire, release, getOwnerRepo } from './gh-utils' +export { _resetOwnerRepoCache } from './gh-utils' +export { getIssue, listIssues } from './issues' import { mapCheckRunRESTStatus, mapCheckRunRESTConclusion, mapCheckStatus, mapCheckConclusion, mapPRState, - deriveCheckStatus, - mapIssueInfo + deriveCheckStatus } from './mappers' -const execFileAsync = promisify(execFile) - -// Concurrency limiter - max 4 parallel gh processes -const MAX_CONCURRENT = 4 -let running = 0 -const queue: (() => void)[] = [] - -function acquire(): Promise { - if (running < MAX_CONCURRENT) { - running++ - return Promise.resolve() - } - return new Promise((resolve) => - queue.push(() => { - running++ - resolve() - }) - ) -} - -function release(): void { - running-- - const next = queue.shift() - if (next) { - next() - } -} - -// ── Owner/repo resolution for gh api --cache ────────────────────────── -const ownerRepoCache = new Map() - -/** @internal — exposed for tests only */ -export function _resetOwnerRepoCache(): void { - ownerRepoCache.clear() -} - -async function getOwnerRepo(repoPath: string): Promise<{ owner: string; repo: string } | null> { - if (ownerRepoCache.has(repoPath)) { - return ownerRepoCache.get(repoPath)! - } - try { - const { stdout } = await execFileAsync('git', ['remote', 'get-url', 'origin'], { - cwd: repoPath, - encoding: 'utf-8' - }) - const match = stdout.trim().match(/github\.com[:/]([^/]+)\/([^/.]+?)(?:\.git)?$/) - if (match) { - const result = { owner: match[1], repo: match[2] } - ownerRepoCache.set(repoPath, result) - return result - } - } catch { - // ignore — non-GitHub remote or no remote - } - ownerRepoCache.set(repoPath, null) - return null -} - /** * Get PR info for a given branch using gh CLI. * Returns null if gh is not installed, or no PR exists for the branch. @@ -95,6 +38,10 @@ export async function getPRForBranch(repoPath: string, branch: string): Promise< updatedAt: string isDraft?: boolean mergeable: string + baseRefName?: string + headRefName?: string + baseRefOid?: string + headRefOid?: string } | null = null if (ownerRepo) { @@ -112,7 +59,7 @@ export async function getPRForBranch(repoPath: string, branch: string): Promise< '--limit', '1', '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable' + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid' ], { cwd: repoPath, @@ -129,7 +76,7 @@ export async function getPRForBranch(repoPath: string, branch: string): Promise< 'view', branchName, '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable' + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid' ], { cwd: repoPath, @@ -143,6 +90,11 @@ export async function getPRForBranch(repoPath: string, branch: string): Promise< return null } + const conflictSummary = + data.mergeable === 'CONFLICTING' && data.baseRefName && data.baseRefOid && data.headRefOid + ? await getPRConflictSummary(repoPath, data.baseRefName, data.baseRefOid, data.headRefOid) + : undefined + return { number: data.number, title: data.title, @@ -150,7 +102,8 @@ export async function getPRForBranch(repoPath: string, branch: string): Promise< url: data.url, checksStatus: deriveCheckStatus(data.statusCheckRollup), updatedAt: data.updatedAt, - mergeable: (data.mergeable as PRMergeableState) ?? 'UNKNOWN' + mergeable: (data.mergeable as PRMergeableState) ?? 'UNKNOWN', + conflictSummary } } catch { return null @@ -159,80 +112,6 @@ export async function getPRForBranch(repoPath: string, branch: string): Promise< } } -/** - * Get a single issue by number. - * Uses gh api --cache so 304 Not Modified responses don't count against the rate limit. - */ -export async function getIssue(repoPath: string, issueNumber: number): Promise { - const ownerRepo = await getOwnerRepo(repoPath) - await acquire() - try { - if (ownerRepo) { - const { stdout } = await execFileAsync( - 'gh', - [ - 'api', - '--cache', - '300s', - `repos/${ownerRepo.owner}/${ownerRepo.repo}/issues/${issueNumber}` - ], - { cwd: repoPath, encoding: 'utf-8' } - ) - const data = JSON.parse(stdout) - return mapIssueInfo(data) - } - // Fallback for non-GitHub remotes - const { stdout } = await execFileAsync( - 'gh', - ['issue', 'view', String(issueNumber), '--json', 'number,title,state,url,labels'], - { cwd: repoPath, encoding: 'utf-8' } - ) - const data = JSON.parse(stdout) - return mapIssueInfo(data) - } catch { - return null - } finally { - release() - } -} - -/** - * List issues for a repo. - * Uses gh api --cache so 304 Not Modified responses don't count against the rate limit. - */ -export async function listIssues(repoPath: string, limit = 20): Promise { - const ownerRepo = await getOwnerRepo(repoPath) - await acquire() - try { - if (ownerRepo) { - const { stdout } = await execFileAsync( - 'gh', - [ - 'api', - '--cache', - '120s', - `repos/${ownerRepo.owner}/${ownerRepo.repo}/issues?per_page=${limit}&state=open&sort=updated&direction=desc` - ], - { cwd: repoPath, encoding: 'utf-8' } - ) - const data = JSON.parse(stdout) as unknown[] - return data.map((d) => mapIssueInfo(d as Parameters[0])) - } - // Fallback for non-GitHub remotes - const { stdout } = await execFileAsync( - 'gh', - ['issue', 'list', '--json', 'number,title,state,url,labels', '--limit', String(limit)], - { cwd: repoPath, encoding: 'utf-8' } - ) - const data = JSON.parse(stdout) as unknown[] - return data.map((d) => mapIssueInfo(d as Parameters[0])) - } catch { - return [] - } finally { - release() - } -} - /** * Get detailed check statuses for a PR. * When branch is provided, uses gh api --cache with the check-runs REST endpoint diff --git a/src/main/github/conflict-summary.ts b/src/main/github/conflict-summary.ts new file mode 100644 index 00000000000..b76e208c6ed --- /dev/null +++ b/src/main/github/conflict-summary.ts @@ -0,0 +1,148 @@ +import { execFile } from 'child_process' +import { promisify } from 'util' +import type { PRConflictSummary } from '../../shared/types' + +const execFileAsync = promisify(execFile) + +export async function getPRConflictSummary( + repoPath: string, + baseRefName: string, + baseRefOid: string, + headRefOid: string +): Promise { + try { + // Why: the renderer only needs a read-only merge-conflict snapshot. We + // derive it from local git state so the PR card can show GitHub-style + // detail without spending additional gh API calls on every refresh. We use + // GitHub's head OID directly because the registered repo path may not have + // a matching local branch name for the PR head. For the base side, prefer a + // freshly-fetched remote-tracking ref so Orca matches GitHub's portal, + // which compares against the latest base branch tip rather than the PR's + // older pinned baseRefOid snapshot. + const latestBaseOid = await resolveLatestBaseOid(repoPath, baseRefName, baseRefOid) + const mergeBase = await resolveMergeBase(repoPath, headRefOid, latestBaseOid) + const [commitsBehind, files] = await Promise.all([ + countCommits(repoPath, `${headRefOid}..${latestBaseOid}`), + loadConflictingFiles(repoPath, mergeBase, headRefOid, latestBaseOid) + ]) + + return { + baseRef: baseRefName, + baseCommit: latestBaseOid.slice(0, 7), + commitsBehind, + files + } + } catch { + return undefined + } +} + +async function resolveLatestBaseOid( + repoPath: string, + baseRefName: string, + fallbackBaseOid: string +): Promise { + const remoteName = 'origin' + + try { + // Why: cap the fetch at 10 s so slow or unreachable remotes don't block + // the conflict-summary derivation indefinitely. + await execFileAsync('git', ['fetch', '--quiet', remoteName, baseRefName], { + cwd: repoPath, + encoding: 'utf-8', + timeout: 10_000 + }) + } catch { + // Why: fetching the base ref keeps the conflict list aligned with GitHub's + // live mergeability view, but the card must still render offline. If fetch + // fails, fall back to the base OID GitHub already gave us. + } + + for (const ref of [`refs/remotes/${remoteName}/${baseRefName}`, `${remoteName}/${baseRefName}`]) { + try { + const { stdout } = await execFileAsync('git', ['rev-parse', '--verify', ref], { + cwd: repoPath, + encoding: 'utf-8' + }) + const oid = stdout.trim() + if (oid) { + return oid + } + } catch { + // Try the next ref form before falling back to GitHub's baseRefOid. + } + } + + return fallbackBaseOid +} + +async function resolveMergeBase( + repoPath: string, + headOid: string, + baseOid: string +): Promise { + const { stdout } = await execFileAsync('git', ['merge-base', headOid, baseOid], { + cwd: repoPath, + encoding: 'utf-8' + }) + return stdout.trim() +} + +async function countCommits(repoPath: string, range: string): Promise { + const { stdout } = await execFileAsync('git', ['rev-list', '--count', range], { + cwd: repoPath, + encoding: 'utf-8' + }) + return Number.parseInt(stdout.trim(), 10) || 0 +} + +async function loadConflictingFiles( + repoPath: string, + mergeBase: string, + headOid: string, + baseOid: string +): Promise { + let stdout = '' + try { + const result = await execFileAsync( + 'git', + [ + 'merge-tree', + '--write-tree', + '--name-only', + '-z', + '--no-messages', + '--merge-base', + mergeBase, + headOid, + baseOid + ], + { + cwd: repoPath, + encoding: 'utf-8' + } + ) + stdout = result.stdout + } catch (error) { + const stdoutFromError = + typeof error === 'object' && error && 'stdout' in error && typeof error.stdout === 'string' + ? error.stdout + : '' + + // Why: `git merge-tree --write-tree` exits with status 1 when it finds + // conflicts, but still writes the conflicted file list to stdout. Treat + // that stdout as the useful result instead of dropping the summary. + if (!stdoutFromError) { + throw error + } + stdout = stdoutFromError + } + + const entries = stdout.split('\0').filter(Boolean) + if (entries.length === 0) { + return [] + } + + const [, ...files] = entries + return files +} diff --git a/src/main/github/gh-utils.ts b/src/main/github/gh-utils.ts new file mode 100644 index 00000000000..54a90a2eee6 --- /dev/null +++ b/src/main/github/gh-utils.ts @@ -0,0 +1,62 @@ +import { execFile } from 'child_process' +import { promisify } from 'util' + +export const execFileAsync = promisify(execFile) + +// Concurrency limiter - max 4 parallel gh processes +const MAX_CONCURRENT = 4 +let running = 0 +const queue: (() => void)[] = [] + +export function acquire(): Promise { + if (running < MAX_CONCURRENT) { + running++ + return Promise.resolve() + } + return new Promise((resolve) => + queue.push(() => { + running++ + resolve() + }) + ) +} + +export function release(): void { + running-- + const next = queue.shift() + if (next) { + next() + } +} + +// ── Owner/repo resolution for gh api --cache ────────────────────────── +const ownerRepoCache = new Map() + +/** @internal — exposed for tests only */ +export function _resetOwnerRepoCache(): void { + ownerRepoCache.clear() +} + +export async function getOwnerRepo( + repoPath: string +): Promise<{ owner: string; repo: string } | null> { + if (ownerRepoCache.has(repoPath)) { + return ownerRepoCache.get(repoPath)! + } + try { + const { stdout } = await execFileAsync('git', ['remote', 'get-url', 'origin'], { + cwd: repoPath, + encoding: 'utf-8' + }) + const match = stdout.trim().match(/github\.com[:/]([^/]+)\/([^/.]+?)(?:\.git)?$/) + if (match) { + const result = { owner: match[1], repo: match[2] } + ownerRepoCache.set(repoPath, result) + return result + } + } catch { + // ignore — non-GitHub remote or no remote + } + ownerRepoCache.set(repoPath, null) + return null +} diff --git a/src/main/github/issues.ts b/src/main/github/issues.ts new file mode 100644 index 00000000000..3fa3dcbf920 --- /dev/null +++ b/src/main/github/issues.ts @@ -0,0 +1,77 @@ +import type { IssueInfo } from '../../shared/types' +import { mapIssueInfo } from './mappers' +import { execFileAsync, acquire, release, getOwnerRepo } from './gh-utils' + +/** + * Get a single issue by number. + * Uses gh api --cache so 304 Not Modified responses don't count against the rate limit. + */ +export async function getIssue(repoPath: string, issueNumber: number): Promise { + const ownerRepo = await getOwnerRepo(repoPath) + await acquire() + try { + if (ownerRepo) { + const { stdout } = await execFileAsync( + 'gh', + [ + 'api', + '--cache', + '300s', + `repos/${ownerRepo.owner}/${ownerRepo.repo}/issues/${issueNumber}` + ], + { cwd: repoPath, encoding: 'utf-8' } + ) + const data = JSON.parse(stdout) + return mapIssueInfo(data) + } + // Fallback for non-GitHub remotes + const { stdout } = await execFileAsync( + 'gh', + ['issue', 'view', String(issueNumber), '--json', 'number,title,state,url,labels'], + { cwd: repoPath, encoding: 'utf-8' } + ) + const data = JSON.parse(stdout) + return mapIssueInfo(data) + } catch { + return null + } finally { + release() + } +} + +/** + * List issues for a repo. + * Uses gh api --cache so 304 Not Modified responses don't count against the rate limit. + */ +export async function listIssues(repoPath: string, limit = 20): Promise { + const ownerRepo = await getOwnerRepo(repoPath) + await acquire() + try { + if (ownerRepo) { + const { stdout } = await execFileAsync( + 'gh', + [ + 'api', + '--cache', + '120s', + `repos/${ownerRepo.owner}/${ownerRepo.repo}/issues?per_page=${limit}&state=open&sort=updated&direction=desc` + ], + { cwd: repoPath, encoding: 'utf-8' } + ) + const data = JSON.parse(stdout) as unknown[] + return data.map((d) => mapIssueInfo(d as Parameters[0])) + } + // Fallback for non-GitHub remotes + const { stdout } = await execFileAsync( + 'gh', + ['issue', 'list', '--json', 'number,title,state,url,labels', '--limit', String(limit)], + { cwd: repoPath, encoding: 'utf-8' } + ) + const data = JSON.parse(stdout) as unknown[] + return data.map((d) => mapIssueInfo(d as Parameters[0])) + } catch { + return [] + } finally { + release() + } +} diff --git a/src/renderer/src/components/editor/ConflictComponents.tsx b/src/renderer/src/components/editor/ConflictComponents.tsx index 8725f2e7307..a0d95317cd1 100644 --- a/src/renderer/src/components/editor/ConflictComponents.tsx +++ b/src/renderer/src/components/editor/ConflictComponents.tsx @@ -1,5 +1,5 @@ import React from 'react' -import { CircleCheck, GitMerge, TriangleAlert, X } from 'lucide-react' +import { CircleCheck, GitMerge, RefreshCw, TriangleAlert, X } from 'lucide-react' import { Button } from '@/components/ui/button' import { cn } from '@/lib/utils' import type { OpenFile } from '@/store/slices/editor' @@ -16,7 +16,7 @@ export const CONFLICT_KIND_LABELS: Record = { } export const CONFLICT_HINT_MAP: Record = { - both_modified: 'Open and edit the final contents', + both_modified: 'Resolve the conflict markers', both_added: 'Choose which version to keep, or combine them', deleted_by_us: 'Decide whether to restore the file', deleted_by_them: 'Decide whether to keep the file or accept deletion', @@ -59,7 +59,11 @@ export function ConflictBanner({ {label} conflict · {CONFLICT_KIND_LABELS[conflict.conflictKind]} -
{CONFLICT_HINT_MAP[conflict.conflictKind]}
+ {/* Why: the hint is omitted here because the file is already open in the + editor below. Showing "Open and edit…" or similar would be confusing + when the user is already viewing/editing the working-tree contents. + The hint is still shown in ConflictPlaceholderView and the review + list where it provides actionable guidance. */} {!isUnresolved && (
Session-local continuity state. Git is no longer reporting this file as unmerged. @@ -151,10 +155,8 @@ export function ConflictReviewPanel({
-
diff --git a/src/renderer/src/components/right-sidebar/ChecksPanel.tsx b/src/renderer/src/components/right-sidebar/ChecksPanel.tsx index 4e09ccc04fe..1fa9a2dd7d8 100644 --- a/src/renderer/src/components/right-sidebar/ChecksPanel.tsx +++ b/src/renderer/src/components/right-sidebar/ChecksPanel.tsx @@ -3,7 +3,13 @@ import { LoaderCircle, ExternalLink, RefreshCw, Check, X, Pencil } from 'lucide- import { useAppStore } from '@/store' import { cn } from '@/lib/utils' import PRActions from './PRActions' -import { PullRequestIcon, prStateColor, MergeConflictWarning, ChecksList } from './checks-helpers' +import { + PullRequestIcon, + prStateColor, + ConflictingFilesSection, + MergeConflictNotice, + ChecksList +} from './checks-helpers' import type { PRInfo, PRCheckDetail } from '../../../../shared/types' export default function ChecksPanel(): React.JSX.Element { @@ -12,6 +18,7 @@ export default function ChecksPanel(): React.JSX.Element { const repos = useAppStore((s) => s.repos) const prCache = useAppStore((s) => s.prCache) const fetchPRForBranch = useAppStore((s) => s.fetchPRForBranch) + const gitConflictOperationByWorktree = useAppStore((s) => s.gitConflictOperationByWorktree) const fetchPRChecks = useAppStore((s) => s.fetchPRChecks) @@ -26,6 +33,7 @@ export default function ChecksPanel(): React.JSX.Element { const pollRef = useRef | null>(null) const pollIntervalRef = useRef(30_000) // start at 30s, backs off to 120s const prevChecksRef = useRef('') + const conflictSummaryRefreshKeyRef = useRef(null) // Find active worktree and repo const { worktree, repo } = useMemo(() => { @@ -46,6 +54,9 @@ export default function ChecksPanel(): React.JSX.Element { const prCacheKey = repo && branch ? `${repo.path}::${branch}` : '' const pr: PRInfo | null = prCacheKey ? (prCache[prCacheKey]?.data ?? null) : null const prNumber = pr?.number ?? null + const conflictOperation = activeWorktreeId + ? (gitConflictOperationByWorktree[activeWorktreeId] ?? 'unknown') + : 'unknown' // Fetch PR data when the active worktree/branch changes useEffect(() => { @@ -54,6 +65,25 @@ export default function ChecksPanel(): React.JSX.Element { } }, [repo, branch, fetchPRForBranch]) + useEffect(() => { + if (!repo || !branch || !pr || pr.mergeable !== 'CONFLICTING') { + conflictSummaryRefreshKeyRef.current = null + return + } + + const refreshKey = `${repo.path}::${branch}::${pr.number}` + if (conflictSummaryRefreshKeyRef.current === refreshKey) { + return + } + + // Why: the checks panel is the one place where stale conflict metadata is + // visibly wrong. Force-refresh conflicting PRs once when the panel sees + // them so we don't keep rendering cached branch summaries or empty file + // lists from an older payload. + conflictSummaryRefreshKeyRef.current = refreshKey + void fetchPRForBranch(repo.path, branch, { force: true }) + }, [repo, branch, pr, fetchPRForBranch]) + // Fetch checks via cached store method const fetchChecks = useCallback( async ({ @@ -211,27 +241,47 @@ export default function ChecksPanel(): React.JSX.Element { } if (!pr) { + // Why: during a rebase/merge/cherry-pick the worktree is on a detached + // HEAD, so there is no branch to look up a PR for. Showing "No pull + // request found" is misleading — the PR still exists on the original + // branch. Show an operation-aware message instead. + const operationInProgress = conflictOperation !== 'unknown' + const operationLabel = + conflictOperation === 'rebase' + ? 'Rebase' + : conflictOperation === 'merge' + ? 'Merge' + : conflictOperation === 'cherry-pick' + ? 'Cherry-pick' + : null + return (
-
No pull request found
-
- Push your branch and open a PR to see checks here +
+ {operationInProgress ? `${operationLabel} in progress` : 'No pull request found'}
- +
+ {operationInProgress + ? 'PR checks will be available after the operation completes' + : 'Push your branch and open a PR to see checks here'} +
+ {!operationInProgress && ( + + )}
) } @@ -319,17 +369,20 @@ export default function ChecksPanel(): React.JSX.Element {
)} - {/* Merge conflict warning — surfaced from GitHub's mergeable state so the - user knows they must resolve conflicts before the PR can merge. */} - - {/* Merge / Delete Worktree actions */} {worktree && repo && ( )} - + + + {/* Why: when the PR has merge conflicts and no checks have been fetched, + showing "No checks configured" is misleading — checks may exist but + simply cannot run until conflicts are resolved. Hide the empty state. */} + {!(pr.mergeable === 'CONFLICTING' && checks.length === 0 && !checksLoading) && ( + + )} ) } diff --git a/src/renderer/src/components/right-sidebar/PRActions.tsx b/src/renderer/src/components/right-sidebar/PRActions.tsx index 92a8f035345..725a5b1625e 100644 --- a/src/renderer/src/components/right-sidebar/PRActions.tsx +++ b/src/renderer/src/components/right-sidebar/PRActions.tsx @@ -3,6 +3,7 @@ import { LoaderCircle, GitMerge, ChevronDown, Trash2 } from 'lucide-react' import { useAppStore } from '@/store' import { cn } from '@/lib/utils' import { Button } from '@/components/ui/button' +import { Tooltip, TooltipTrigger, TooltipContent, TooltipProvider } from '@/components/ui/tooltip' import type { PRInfo, Repo, Worktree } from '../../../../shared/types' const MERGE_METHODS = ['squash', 'merge', 'rebase'] as const @@ -72,58 +73,84 @@ export default function PRActions({ openModal('delete-worktree', { worktreeId: worktree.id }) }, [worktree.id, openModal]) + // Why: merging a PR with unresolved conflicts would fail on GitHub anyway; + // disabling the button prevents a confusing error and signals the user must + // resolve conflicts first. + const hasConflicts = pr.mergeable === 'CONFLICTING' + if (pr.state === 'open') { return (
-
- - - {mergeMenuOpen && ( -
- {MERGE_METHODS.map((method) => ( - - ))} -
- )} -
+ + + {mergeMenuOpen && ( +
+ {MERGE_METHODS.map((method) => ( + + ))} +
+ )} +
+ + + {hasConflicts && ( + + Merge conflicts must be resolved before merging + + )} + + {mergeError &&
{mergeError}
} ) diff --git a/src/renderer/src/components/right-sidebar/SourceControl.tsx b/src/renderer/src/components/right-sidebar/SourceControl.tsx index c758f24c8d9..dee7e8f8d5e 100644 --- a/src/renderer/src/components/right-sidebar/SourceControl.tsx +++ b/src/renderer/src/components/right-sidebar/SourceControl.tsx @@ -15,6 +15,7 @@ import { ArrowRightLeft, FolderOpen, GitMerge, + GitPullRequestArrow, TriangleAlert, CircleCheck } from 'lucide-react' @@ -63,7 +64,11 @@ const STATUS_ICONS: Record< copied: FilePlus } -const SECTION_ORDER = ['staged', 'unstaged', 'untracked'] as const +// 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 +// user won't miss them. +const SECTION_ORDER = ['unstaged', 'staged', 'untracked'] as const const SECTION_LABELS: Record<(typeof SECTION_ORDER)[number], string> = { staged: 'Staged Changes', unstaged: 'Changes', @@ -82,20 +87,6 @@ const CONFLICT_KIND_LABELS: Record = { both_deleted: 'Both deleted' } -// Why: hint text is derived at render time in the renderer, not returned by the -// main process on GitUncommittedEntry. This keeps UI copy out of the IPC layer -// and the main-process parser. See also ConflictComponents.tsx for the editor- -// side copy of this map. -const CONFLICT_HINT_MAP: Record = { - both_modified: 'Open and edit the final contents', - both_added: 'Choose which version to keep, or combine them', - deleted_by_us: 'Decide whether to restore the file', - deleted_by_them: 'Decide whether to keep the file or accept deletion', - added_by_us: 'Review whether to keep the added file', - added_by_them: 'Review the added file before keeping it', - both_deleted: 'Resolve in Git or restore one side before editing' -} - export default function SourceControl(): React.JSX.Element { const activeWorktreeId = useAppStore((s) => s.activeWorktreeId) const rightSidebarTab = useAppStore((s) => s.rightSidebarTab) @@ -473,6 +464,15 @@ export default function SourceControl(): React.JSX.Element { /> )} + {/* Why: show operation banner when rebase/merge/cherry-pick is in progress + but there are no unresolved conflicts (e.g. between rebase steps, or + after resolving all conflicts before running --continue). The + ConflictSummaryCard handles the "has conflicts" case above. */} + {unresolvedConflictReviewEntries.length === 0 && conflictOperation !== 'unknown' && ( +
+ +
+ )} {scope === 'all' && showGenericEmptyState ? ( +
- {conflictLabel && conflictHint && ( -
- {conflictLabel} · {conflictHint} -
+ {conflictLabel && ( +
{conflictLabel}
)} {entry.conflictStatus ? ( diff --git a/src/renderer/src/components/right-sidebar/checks-helpers.tsx b/src/renderer/src/components/right-sidebar/checks-helpers.tsx index 30a53e8a1b0..15895e22c33 100644 --- a/src/renderer/src/components/right-sidebar/checks-helpers.tsx +++ b/src/renderer/src/components/right-sidebar/checks-helpers.tsx @@ -5,11 +5,11 @@ import { CircleDashed, CircleMinus, GitPullRequest, - AlertTriangle + Files } from 'lucide-react' import { ExternalLink } from 'lucide-react' import { cn } from '@/lib/utils' -import type { PRInfo, PRMergeableState, PRCheckDetail } from '../../../../shared/types' +import type { PRInfo, PRCheckDetail } from '../../../../shared/types' export const PullRequestIcon = GitPullRequest @@ -33,26 +33,51 @@ export const CHECK_COLOR: Record = { timed_out: 'text-rose-500' } -/** Shown when GitHub reports the PR branch has merge conflicts. */ -export function MergeConflictWarning({ - mergeable -}: { - mergeable: PRMergeableState -}): React.JSX.Element | null { - if (mergeable !== 'CONFLICTING') { +export function ConflictingFilesSection({ pr }: { pr: PRInfo }): React.JSX.Element | null { + const files = pr.conflictSummary?.files ?? [] + if (pr.mergeable !== 'CONFLICTING' || files.length === 0) { return null } + return ( -
- -
-
- This branch has conflicts that must be resolved before merging -
-
- Resolve conflicts on GitHub or locally, then push -
+
+
+ This branch has conflicts that must be resolved
+
+ It's {pr.conflictSummary!.commitsBehind} commit + {pr.conflictSummary!.commitsBehind === 1 ? '' : 's'} behind (base commit:{' '} + {pr.conflictSummary!.baseCommit}) +
+
+ +
Conflicting files
+
+
+ {files.map((filePath) => ( +
+
+ {filePath} +
+
+ ))} +
+
+ ) +} + +/** Fallback shown when GitHub reports merge conflicts but no file list is available yet. */ +export function MergeConflictNotice({ pr }: { pr: PRInfo }): React.JSX.Element | null { + if (pr.mergeable !== 'CONFLICTING' || (pr.conflictSummary?.files.length ?? 0) > 0) { + return null + } + + return ( +
+
+ This branch has conflicts that must be resolved +
+
Refreshing conflict details…
) } diff --git a/src/renderer/src/components/sidebar/WorktreeCard.tsx b/src/renderer/src/components/sidebar/WorktreeCard.tsx index c73947dec7d..62c3d2e1dc5 100644 --- a/src/renderer/src/components/sidebar/WorktreeCard.tsx +++ b/src/renderer/src/components/sidebar/WorktreeCard.tsx @@ -4,7 +4,7 @@ import { useAppStore } from '@/store' import { Badge } from '@/components/ui/badge' import { HoverCard, HoverCardTrigger, HoverCardContent } from '@/components/ui/hover-card' import { Tooltip, TooltipTrigger, TooltipContent } from '@/components/ui/tooltip' -import { Bell, LoaderCircle, CircleDot, CircleCheck, CircleX } from 'lucide-react' +import { Bell, GitMerge, LoaderCircle, CircleDot, CircleCheck, CircleX } from 'lucide-react' import StatusIndicator from './StatusIndicator' import WorktreeContextMenu from './WorktreeContextMenu' import { cn } from '@/lib/utils' @@ -16,6 +16,7 @@ import type { IssueInfo, PRState, CheckStatus, + GitConflictOperation, TerminalTab } from '../../../../shared/types' import type { Status } from './StatusIndicator' @@ -47,6 +48,12 @@ function checksLabel(status: CheckStatus): string { } } +const CONFLICT_OPERATION_LABELS: Record, string> = { + merge: 'Merging', + rebase: 'Rebasing', + 'cherry-pick': 'Cherry-picking' +} + // ── Stable empty array for tabs fallback ───────────────────────── const EMPTY_TABS: TerminalTab[] = [] @@ -121,6 +128,7 @@ const WorktreeCard = React.memo(function WorktreeCard({ ) const deleteState = useAppStore((s) => s.deleteStateByWorktreeId[worktree.id]) + const conflictOperation = useAppStore((s) => s.gitConflictOperationByWorktree[worktree.id]) // ── GRANULAR selectors: only subscribe to THIS worktree's data ── const tabs = useAppStore((s) => s.tabsByWorktree[worktree.id] ?? EMPTY_TABS) @@ -323,6 +331,20 @@ const WorktreeCard = React.memo(function WorktreeCard({ {branch} )} + + {/* Why: the conflict operation (merge/rebase/cherry-pick) is the + only signal that the worktree is in an incomplete operation state. + Showing it on the card lets the user spot worktrees that need + attention without switching to them first. */} + {conflictOperation && conflictOperation !== 'unknown' && ( + + + {CONFLICT_OPERATION_LABELS[conflictOperation]} + + )}
{/* Meta section: Issue / PR Links / Comment */} diff --git a/src/shared/types.ts b/src/shared/types.ts index 667b0da9ee2..8670121a9e1 100644 --- a/src/shared/types.ts +++ b/src/shared/types.ts @@ -97,6 +97,13 @@ export type CheckStatus = 'pending' | 'success' | 'failure' | 'neutral' export type PRMergeableState = 'MERGEABLE' | 'CONFLICTING' | 'UNKNOWN' +export type PRConflictSummary = { + baseRef: string + baseCommit: string + commitsBehind: number + files: string[] +} + export type PRInfo = { number: number title: string @@ -105,6 +112,7 @@ export type PRInfo = { checksStatus: CheckStatus updatedAt: string mergeable: PRMergeableState + conflictSummary?: PRConflictSummary } export type PRCheckDetail = {