feat(git): improve PR panel (#207)

2026-01-23 19:54:28 +02:00
parent 614277eaa1
commit da20c647e2
9 changed files with 306 additions and 83 deletions
@@ -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/<base>` when available)
+- head branch ref
+- committed range diff: `git diff <base>...<head>` (file list from `git diff --name-only <base>...<head>`)

 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
@@ -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();
@@ -254,6 +254,20 @@ struct CombinedStatusResponse {
    statuses: Vec<CombinedStatusEntry>,
 }

+#[derive(Debug, Deserialize)]
+struct CheckRunEntry {
+    #[serde(default)]
+    status: Option<String>,
+    #[serde(default)]
+    conclusion: Option<String>,
+}
+
+#[derive(Debug, Deserialize)]
+struct CheckRunsResponse {
+    #[serde(default)]
+    check_runs: Vec<CheckRunEntry>,
+}
+
 #[derive(Debug, Deserialize)]
 struct PermissionResponse {
    permission: String,
@@ -849,41 +863,93 @@ pub async fn github_pr_status(
    );
    let pr = github_get_json::<PullDetailsResponse>(&pr_url, &stored.access_token).await?;

-    // Checks summary
+    // Checks summary: prefer check-runs (Actions), fallback to classic statuses
    let mut checks: Option<GitHubChecksSummary> = 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::<CombinedStatusResponse>(&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::<CheckRunsResponse>(&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::<CombinedStatusResponse>(&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)
@@ -233,9 +233,8 @@ export const CommandAutocomplete = React.forwardRef<CommandAutocompleteHandle, C
    <div
      ref={containerRef}
      className="absolute z-[100] min-w-0 w-full max-w-[450px] max-h-64 bg-background border border-border rounded-xl shadow-none bottom-full mb-2 left-0 flex flex-col"
-      style={{ touchAction: 'pan-y' }}
    >
-      <ScrollableOverlay outerClassName="flex-1 min-h-0" className="px-0 pb-2">
+      <ScrollableOverlay outerClassName="flex-1 min-h-0" className="px-0 pb-2" fillContainer={false}>
        {loading ? (
          <div className="flex items-center justify-center py-4">
            <RiRefreshLine className="h-4 w-4 animate-spin text-muted-foreground" />
@@ -251,7 +251,7 @@ export const PullRequestSection: React.FC<{
          {checks ? (
            <span className="inline-flex items-center gap-2 typography-micro text-muted-foreground">
              <span className={`h-2 w-2 rounded-full ${statusColor(checks.state)}`} />
-              {checks.total > 0 ? `${checks.success}/${checks.total}` : checks.state}
+              {checks.total > 0 ? `${checks.success}/${checks.total} checks` : `${checks.state} checks`}
            </span>
          ) : null}
        </div>
@@ -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 || '')
@@ -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<JsonRecord>(statusResp);
-    if (statusResp.ok && statusJson) {
-      const statuses = Array.isArray(statusJson.statuses) ? (statusJson.statuses as unknown[]) : [];
+    const runsJson = await jsonOrNull<JsonRecord>(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<JsonRecord>(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;
@@ -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');
@@ -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())