From da20c647e23ff2f8d791b80a29ff6529d7c31c22 Mon Sep 17 00:00:00 2001 From: Bohdan Triapitsyn Date: Fri, 23 Jan 2026 19:54:28 +0200 Subject: [PATCH] feat(git): improve PR panel (#207) --- docs/github-features-plan.md | 44 ++++-- .../desktop/src-tauri/src/commands/git.rs | 11 +- .../desktop/src-tauri/src/commands/github.rs | 126 +++++++++++++----- .../components/chat/CommandAutocomplete.tsx | 3 +- .../views/git/PullRequestSection.tsx | 2 +- packages/vscode/src/gitService.ts | 26 +++- packages/vscode/src/githubPr.ts | 64 +++++++-- packages/web/server/index.js | 84 +++++++++--- packages/web/server/lib/git-service.js | 29 +++- 9 files changed, 306 insertions(+), 83 deletions(-) diff --git a/docs/github-features-plan.md b/docs/github-features-plan.md index 53763fe9..a5118ebb 100644 --- a/docs/github-features-plan.md +++ b/docs/github-features-plan.md @@ -53,11 +53,13 @@ Git tab layout ## Feature A: Git Tab PR Panel (Create / Status / Merge) +Status: implemented. + ### Intent While working on a feature branch, show PR status and actions inside the Git tab, without leaving the app. ### Placement -Insert a new section between Commit and History in `packages/ui/src/components/views/GitView.tsx`. +Implemented between Commit and History in `packages/ui/src/components/views/GitView.tsx`. ### Visibility Rules - Show only if: @@ -66,7 +68,7 @@ Insert a new section between Commit and History in `packages/ui/src/components/v Base branch source (reuse existing config): - `activeProject.worktreeDefaults.baseBranch` from `packages/ui/src/stores/useProjectsStore.ts` -- fallback to `status.tracking` remote HEAD logic later if needed (not required for v1). +- fallback default: `main` ### UI States 1) GitHub not connected @@ -95,26 +97,23 @@ Base branch source (reuse existing config): - If user has merge permission and PR is mergeable: - merge method dropdown (merge/squash/rebase) - “Merge” button +- If PR is draft: + - “Ready” button (mark ready for review) + - Merge disabled until ready - If cannot merge: - disable merge button + show “Open in GitHub” ### AI “Generate description” -Mirror commit message generation approach. +Implemented as a PR-specific generator (separate prompt/endpoint/command). Inputs: -- base branch -- branch name -- git diff for base...HEAD -- optionally selected files +- base branch ref (prefers `origin/` when available) +- head branch ref +- committed range diff: `git diff ...` (file list from `git diff --name-only ...`) Output: -- title suggestion (optional) -- body with sections: - - Summary - - Testing - - Notes - -Reuse the same LLM infra used by commit generation in `packages/ui/src/components/views/GitView.tsx` (search for existing generation call and parallel it). +- `title` (<= 80 chars, no commit-style prefixes) +- `body` (GFM markdown sections: Summary/Testing/Notes) ### Required GitHub API Calls - Resolve repo from git remote URL (origin) @@ -122,11 +121,28 @@ Reuse the same LLM infra used by commit generation in `packages/ui/src/component - Create PR - Get PR details + checks - Merge PR +- Mark PR ready for review (GraphQL) + +Checks logic (implemented): +- prefer GitHub Actions check-runs (`/commits/{sha}/check-runs`) +- fallback to classic commit statuses (`/commits/{sha}/status`) ### Implementation Notes - Web runtime should use server endpoints + Octokit (token stays server-side). - Desktop/vscode should use their runtime handlers (similar to GitHub auth) to avoid exposing token. +Implemented code pointers +- UI section: `packages/ui/src/components/views/git/PullRequestSection.tsx` +- Web server endpoints: + - `GET /api/github/pr/status` + - `POST /api/github/pr/create` + - `POST /api/github/pr/merge` + - `POST /api/github/pr/ready` +- PR description generator: + - `POST /api/git/pr-description` + - Desktop: `generate_pr_description` + - VS Code: `api:git/pr-description` + ## Feature B: Start Session From GitHub Issue ### Intent diff --git a/packages/desktop/src-tauri/src/commands/git.rs b/packages/desktop/src-tauri/src/commands/git.rs index 5221bd52..383f2e0d 100644 --- a/packages/desktop/src-tauri/src/commands/git.rs +++ b/packages/desktop/src-tauri/src/commands/git.rs @@ -2353,7 +2353,16 @@ pub async fn generate_pr_description( } // 1. Collect PR range diffs (base...head) - let range = format!("{}...{}", base.trim(), head.trim()); + let base_ref = base.trim(); + let head_ref = head.trim(); + let origin_candidate = format!("refs/remotes/origin/{}", base_ref); + let resolved_base = if run_git(&["rev-parse", "--verify", &origin_candidate], &root).await.is_ok() { + format!("origin/{}", base_ref) + } else { + base_ref.to_string() + }; + + let range = format!("{}...{}", resolved_base, head_ref); let files = { let args = vec!["diff", "--name-only", range.as_str()]; let raw = run_git(&args, &root).await.unwrap_or_default(); diff --git a/packages/desktop/src-tauri/src/commands/github.rs b/packages/desktop/src-tauri/src/commands/github.rs index 691660c7..54e1a79d 100644 --- a/packages/desktop/src-tauri/src/commands/github.rs +++ b/packages/desktop/src-tauri/src/commands/github.rs @@ -254,6 +254,20 @@ struct CombinedStatusResponse { statuses: Vec, } +#[derive(Debug, Deserialize)] +struct CheckRunEntry { + #[serde(default)] + status: Option, + #[serde(default)] + conclusion: Option, +} + +#[derive(Debug, Deserialize)] +struct CheckRunsResponse { + #[serde(default)] + check_runs: Vec, +} + #[derive(Debug, Deserialize)] struct PermissionResponse { permission: String, @@ -849,41 +863,93 @@ pub async fn github_pr_status( ); let pr = github_get_json::(&pr_url, &stored.access_token).await?; - // Checks summary + // Checks summary: prefer check-runs (Actions), fallback to classic statuses let mut checks: Option = None; - let status_url = format!( - "{}/{}/{}/commits/{}/status", + + let check_runs_url = format!( + "{}/{}/{}/commits/{}/check-runs", API_PULLS_URL_PREFIX, repo.owner, repo.repo, pr.head.sha ); - if let Ok(status) = github_get_json::(&status_url, &stored.access_token).await { - let mut success = 0; - let mut failure = 0; - let mut pending = 0; - for s in status.statuses.iter() { - match s.state.as_str() { - "success" => success += 1, - "failure" | "error" => failure += 1, - "pending" => pending += 1, - _ => {} + + if let Ok(runs) = github_get_json::(&check_runs_url, &stored.access_token).await { + if !runs.check_runs.is_empty() { + let mut success = 0; + let mut failure = 0; + let mut pending = 0; + + for run in runs.check_runs.iter() { + let status = run.status.as_deref().unwrap_or(""); + let conclusion = run.conclusion.as_deref().unwrap_or(""); + if status == "queued" || status == "in_progress" { + pending += 1; + continue; + } + if conclusion.is_empty() { + pending += 1; + continue; + } + if conclusion == "success" || conclusion == "neutral" || conclusion == "skipped" { + success += 1; + } else { + failure += 1; + } } + + let total = success + failure + pending; + let state = if failure > 0 { + "failure" + } else if pending > 0 { + "pending" + } else if total > 0 { + "success" + } else { + "unknown" + }; + checks = Some(GitHubChecksSummary { + state: state.to_string(), + total, + success, + failure, + pending, + }); + } + } + + if checks.is_none() { + let status_url = format!( + "{}/{}/{}/commits/{}/status", + API_PULLS_URL_PREFIX, repo.owner, repo.repo, pr.head.sha + ); + if let Ok(status) = github_get_json::(&status_url, &stored.access_token).await { + let mut success = 0; + let mut failure = 0; + let mut pending = 0; + for s in status.statuses.iter() { + match s.state.as_str() { + "success" => success += 1, + "failure" | "error" => failure += 1, + "pending" => pending += 1, + _ => {} + } + } + let total = success + failure + pending; + let state = if failure > 0 { + "failure" + } else if pending > 0 { + "pending" + } else if total > 0 { + "success" + } else { + "unknown" + }; + checks = Some(GitHubChecksSummary { + state: state.to_string(), + total, + success, + failure, + pending, + }); } - let total = success + failure + pending; - let state = if failure > 0 { - "failure" - } else if pending > 0 { - "pending" - } else if total > 0 { - "success" - } else { - "unknown" - }; - checks = Some(GitHubChecksSummary { - state: state.to_string(), - total, - success, - failure, - pending, - }); } // Permissions (best-effort) diff --git a/packages/ui/src/components/chat/CommandAutocomplete.tsx b/packages/ui/src/components/chat/CommandAutocomplete.tsx index c1ea97f2..3fce50f9 100644 --- a/packages/ui/src/components/chat/CommandAutocomplete.tsx +++ b/packages/ui/src/components/chat/CommandAutocomplete.tsx @@ -233,9 +233,8 @@ export const CommandAutocomplete = React.forwardRef - + {loading ? (
diff --git a/packages/ui/src/components/views/git/PullRequestSection.tsx b/packages/ui/src/components/views/git/PullRequestSection.tsx index aba07d0f..c5aa86ba 100644 --- a/packages/ui/src/components/views/git/PullRequestSection.tsx +++ b/packages/ui/src/components/views/git/PullRequestSection.tsx @@ -251,7 +251,7 @@ export const PullRequestSection: React.FC<{ {checks ? ( - {checks.total > 0 ? `${checks.success}/${checks.total}` : checks.state} + {checks.total > 0 ? `${checks.success}/${checks.total} checks` : `${checks.state} checks`} ) : null}
diff --git a/packages/vscode/src/gitService.ts b/packages/vscode/src/gitService.ts index b25e4457..c60dc525 100644 --- a/packages/vscode/src/gitService.ts +++ b/packages/vscode/src/gitService.ts @@ -699,7 +699,18 @@ export async function getGitRangeDiff( if (!baseRef || !headRef) { return { diff: '' }; } - const args = ['diff', '--no-color', `-U${Math.max(0, contextLines)}`, `${baseRef}...${headRef}`, '--', filePath]; + + let resolvedBase = baseRef; + try { + const verify = await execGit(['rev-parse', '--verify', `refs/remotes/origin/${baseRef}`], directory); + if (verify.exitCode === 0) { + resolvedBase = `origin/${baseRef}`; + } + } catch { + // ignore + } + + const args = ['diff', '--no-color', `-U${Math.max(0, contextLines)}`, `${resolvedBase}...${headRef}`, '--', filePath]; const result = await execGit(args, directory); return { diff: result.stdout }; } @@ -717,7 +728,18 @@ export async function getGitRangeFiles( if (!baseRef || !headRef) { return []; } - const args = ['diff', '--name-only', `${baseRef}...${headRef}`]; + + let resolvedBase = baseRef; + try { + const verify = await execGit(['rev-parse', '--verify', `refs/remotes/origin/${baseRef}`], directory); + if (verify.exitCode === 0) { + resolvedBase = `origin/${baseRef}`; + } + } catch { + // ignore + } + + const args = ['diff', '--name-only', `${resolvedBase}...${headRef}`]; const result = await execGit(args, directory); if (result.exitCode !== 0) return []; return String(result.stdout || '') diff --git a/packages/vscode/src/githubPr.ts b/packages/vscode/src/githubPr.ts index 0f97b066..f71889af 100644 --- a/packages/vscode/src/githubPr.ts +++ b/packages/vscode/src/githubPr.ts @@ -180,24 +180,66 @@ export const getPullRequestStatus = async ( let checks: GitHubChecksSummary | null = null; if (pr.headSha) { - const statusResp = await githubFetch( - `${API_BASE}/repos/${repo.owner}/${repo.repo}/commits/${pr.headSha}/status`, + // Prefer check-runs (Actions) + const runsResp = await githubFetch( + `${API_BASE}/repos/${repo.owner}/${repo.repo}/commits/${pr.headSha}/check-runs`, accessToken, ); - const statusJson = await jsonOrNull(statusResp); - if (statusResp.ok && statusJson) { - const statuses = Array.isArray(statusJson.statuses) ? (statusJson.statuses as unknown[]) : []; + const runsJson = await jsonOrNull(runsResp); + const runs = Array.isArray((runsJson as JsonRecord | null)?.check_runs) + ? ((runsJson as JsonRecord).check_runs as unknown[]) + : []; + + if (runsResp.ok && runs.length > 0) { const counts = { success: 0, failure: 0, pending: 0 }; - statuses.forEach((s) => { - const st = readString((s as JsonRecord | null)?.state); - if (st === 'success') counts.success += 1; - else if (st === 'failure' || st === 'error') counts.failure += 1; - else if (st === 'pending') counts.pending += 1; + runs.forEach((r) => { + const rec = (r && typeof r === 'object') ? (r as JsonRecord) : null; + const status = readString(rec?.status); + const conclusion = readString(rec?.conclusion); + if (status === 'queued' || status === 'in_progress') { + counts.pending += 1; + return; + } + if (!conclusion) { + counts.pending += 1; + return; + } + if (conclusion === 'success' || conclusion === 'neutral' || conclusion === 'skipped') { + counts.success += 1; + } else { + counts.failure += 1; + } }); const total = counts.success + counts.failure + counts.pending; - const state2 = counts.failure > 0 ? 'failure' : (counts.pending > 0 ? 'pending' : (total > 0 ? 'success' : 'unknown')); + const state2 = counts.failure > 0 + ? 'failure' + : (counts.pending > 0 ? 'pending' : (total > 0 ? 'success' : 'unknown')); checks = { state: state2, total, ...counts }; } + + // Fallback: classic statuses + if (!checks) { + const statusResp = await githubFetch( + `${API_BASE}/repos/${repo.owner}/${repo.repo}/commits/${pr.headSha}/status`, + accessToken, + ); + const statusJson = await jsonOrNull(statusResp); + if (statusResp.ok && statusJson) { + const statuses = Array.isArray(statusJson.statuses) ? (statusJson.statuses as unknown[]) : []; + const counts = { success: 0, failure: 0, pending: 0 }; + statuses.forEach((s) => { + const st = readString((s as JsonRecord | null)?.state); + if (st === 'success') counts.success += 1; + else if (st === 'failure' || st === 'error') counts.failure += 1; + else if (st === 'pending') counts.pending += 1; + }); + const total = counts.success + counts.failure + counts.pending; + const state2 = counts.failure > 0 + ? 'failure' + : (counts.pending > 0 ? 'pending' : (total > 0 ? 'success' : 'unknown')); + checks = { state: state2, total, ...counts }; + } + } } let canMerge = false; diff --git a/packages/web/server/index.js b/packages/web/server/index.js index 441d4f23..90ca28a2 100644 --- a/packages/web/server/index.js +++ b/packages/web/server/index.js @@ -4094,26 +4094,70 @@ async function main(options = {}) { return res.json({ connected: true, repo, branch, pr: null, checks: null, canMerge: false }); } - // Checks summary (combined status) + // Checks summary: prefer check-runs (Actions), fallback to classic statuses. let checks = null; - try { - const combined = await octokit.rest.repos.getCombinedStatusForRef({ - owner: repo.owner, - repo: repo.repo, - ref: prData.head?.sha, - }); - const statuses = Array.isArray(combined?.data?.statuses) ? combined.data.statuses : []; - const counts = { success: 0, failure: 0, pending: 0 }; - statuses.forEach((s) => { - if (s.state === 'success') counts.success += 1; - else if (s.state === 'failure' || s.state === 'error') counts.failure += 1; - else if (s.state === 'pending') counts.pending += 1; - }); - const total = counts.success + counts.failure + counts.pending; - const state = counts.failure > 0 ? 'failure' : (counts.pending > 0 ? 'pending' : (total > 0 ? 'success' : 'unknown')); - checks = { state, total, ...counts }; - } catch { - checks = null; + const sha = prData.head?.sha; + if (sha) { + try { + const runs = await octokit.rest.checks.listForRef({ + owner: repo.owner, + repo: repo.repo, + ref: sha, + per_page: 100, + }); + const checkRuns = Array.isArray(runs?.data?.check_runs) ? runs.data.check_runs : []; + if (checkRuns.length > 0) { + const counts = { success: 0, failure: 0, pending: 0 }; + for (const run of checkRuns) { + const status = run?.status; + const conclusion = run?.conclusion; + if (status === 'queued' || status === 'in_progress') { + counts.pending += 1; + continue; + } + if (!conclusion) { + counts.pending += 1; + continue; + } + if (conclusion === 'success' || conclusion === 'neutral' || conclusion === 'skipped') { + counts.success += 1; + } else { + counts.failure += 1; + } + } + const total = counts.success + counts.failure + counts.pending; + const state = counts.failure > 0 + ? 'failure' + : (counts.pending > 0 ? 'pending' : (total > 0 ? 'success' : 'unknown')); + checks = { state, total, ...counts }; + } + } catch { + // ignore and fall back + } + + if (!checks) { + try { + const combined = await octokit.rest.repos.getCombinedStatusForRef({ + owner: repo.owner, + repo: repo.repo, + ref: sha, + }); + const statuses = Array.isArray(combined?.data?.statuses) ? combined.data.statuses : []; + const counts = { success: 0, failure: 0, pending: 0 }; + statuses.forEach((s) => { + if (s.state === 'success') counts.success += 1; + else if (s.state === 'failure' || s.state === 'error') counts.failure += 1; + else if (s.state === 'pending') counts.pending += 1; + }); + const total = counts.success + counts.failure + counts.pending; + const state = counts.failure > 0 + ? 'failure' + : (counts.pending > 0 ? 'pending' : (total > 0 ? 'success' : 'unknown')); + checks = { state, total, ...counts }; + } catch { + checks = null; + } + } } // Permission check (best-effort) @@ -4814,7 +4858,7 @@ async function main(options = {}) { } } if (diffs.length === 0) { - return res.status(400).json({ error: 'No diffs available for selected files' }); + return res.status(400).json({ error: 'No diffs available for base...head' }); } const diffSummaries = diffs.map(({ path, diff }) => `FILE: ${path}\n${diff}`).join('\n\n'); diff --git a/packages/web/server/lib/git-service.js b/packages/web/server/lib/git-service.js index df725fb9..aa7625b7 100644 --- a/packages/web/server/lib/git-service.js +++ b/packages/web/server/lib/git-service.js @@ -440,11 +440,24 @@ export async function getRangeDiff(directory, { base, head, path, contextLines = throw new Error('base and head are required'); } + // Prefer remote-tracking base ref so merged commits don't reappear + // when local base branch is stale (common when user stays on feature branch). + let resolvedBase = baseRef; + const originCandidate = `refs/remotes/origin/${baseRef}`; + try { + const verified = await git.raw(['rev-parse', '--verify', originCandidate]); + if (verified && verified.trim()) { + resolvedBase = `origin/${baseRef}`; + } + } catch { + // ignore + } + const args = ['diff', '--no-color']; if (typeof contextLines === 'number' && !Number.isNaN(contextLines)) { args.push(`-U${Math.max(0, contextLines)}`); } - args.push(`${baseRef}...${headRef}`); + args.push(`${resolvedBase}...${headRef}`); if (path) { args.push('--', path); } @@ -459,7 +472,19 @@ export async function getRangeFiles(directory, { base, head } = {}) { if (!baseRef || !headRef) { throw new Error('base and head are required'); } - const raw = await git.raw(['diff', '--name-only', `${baseRef}...${headRef}`]); + + let resolvedBase = baseRef; + const originCandidate = `refs/remotes/origin/${baseRef}`; + try { + const verified = await git.raw(['rev-parse', '--verify', originCandidate]); + if (verified && verified.trim()) { + resolvedBase = `origin/${baseRef}`; + } + } catch { + // ignore + } + + const raw = await git.raw(['diff', '--name-only', `${resolvedBase}...${headRef}`]); return String(raw || '') .split('\n') .map((l) => l.trim())