From 93738f719c4a42c3c595dae429cfd61cad7092b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=ABl=20Kuijper?= Date: Fri, 5 Jun 2026 22:02:50 +0200 Subject: [PATCH 01/11] feat(diff): abstract PR hosting behind a provider trait; add Azure DevOps The "lumen diff --pr" PR review flow was hardcoded to GitHub and the gh CLI throughout. Introduce a PrProvider trait (src/command/diff/pr_provider.rs) so other forges can be supported, and move the existing GitHub logic behind a GitHubProvider impl. Add an AzureProvider backed by the Azure DevOps REST API (src/command/diff/azure.rs): - parses dev.azure.com and *.visualstudio.com PR URLs and remotes (HTTPS + SSH) - fetches PR metadata, the iteration change list, and blob content over REST - diffs against the merge base (iteration changes with $compareTo=0), i.e. the same three-dot view the Azure web UI shows, instead of a tip-to-tip diff - fetches blobs concurrently over pooled HTTP (reusing the existing async reqwest + tokio; no new dependencies) - supports --detect-pr and open-in-browser ('o') Why REST and not the az CLI: az has no first-class command to list a PR's changed files or fetch file content; the only CLI route is the generic `az devops invoke` passthrough, which spawns a Python process per call (2 per file) and is unusably slow for an interactive diff. REST also lets us drop the azure-devops extension requirement entirely. Auth resolves to a PAT (ADO_PAT / AZURE_DEVOPS_EXT_PAT) via HTTP Basic, or a bearer token from `az account get-access-token` (core az, no extension). Per-file viewed-state sync stays a GitHub-only capability via a default no-op on the trait (Azure DevOps has no equivalent API). The provider is selected from the PR URL, falling back to the git origin remote, so bare PR numbers route to the right forge. Adds unit tests for URL/remote parsing, provider detection, and Azure change-entry parsing. Refs #118 --- README.md | 7 +- src/command/diff/app.rs | 37 +-- src/command/diff/azure.rs | 482 +++++++++++++++++++++++++++ src/command/diff/git.rs | 77 ++--- src/command/diff/mod.rs | 92 +++-- src/command/diff/pr_provider.rs | 572 ++++++++++++++++++++++++++++++++ src/config/cli.rs | 6 +- 7 files changed, 1139 insertions(+), 134 deletions(-) create mode 100644 src/command/diff/azure.rs create mode 100644 src/command/diff/pr_provider.rs diff --git a/README.md b/README.md index 6af76de9..180ca694 100644 --- a/README.md +++ b/README.md @@ -12,7 +12,7 @@ A fast terminal diff viewer and code review TUI, written in Rust. Review `git diff`, commits, branches, or GitHub PRs side-by-side without leaving your terminal. Ships as a single static Rust binary and stays snappy on multi-thousand-line diffs. - Side-by-side diff viewer with tree-sitter syntax highlighting -- Review GitHub Pull Requests with `lumen diff --pr 123` +- Review GitHub and Azure DevOps Pull Requests with `lumen diff --pr 123` - Annotate selections, hunks, or whole files - Watch mode and stacked-commit review - Optional AI commit messages and change explanations (10+ providers) @@ -52,6 +52,8 @@ Before you begin, ensure you have: 1. `git` installed on your system 2. [fzf](https://github.com/junegunn/fzf) (optional) - Required for `lumen explain --list` command 3. [mdcat](https://github.com/swsnr/mdcat) (optional) - Required for pretty output formatting +4. [GitHub CLI (`gh`)](https://cli.github.com/) (optional) - Required for reviewing GitHub Pull Requests +5. Azure DevOps Pull Requests (optional) - Authenticate with either a Personal Access Token (`ADO_PAT`, scope: *Code → Read*) or the [Azure CLI (`az`)](https://learn.microsoft.com/cli/azure/) signed in via `az login`. The `azure-devops` extension is **not** required. ### Installation @@ -86,9 +88,10 @@ lumen diff HEAD~1 # View changes between branches lumen diff main..feature/A -# View changes in a GitHub Pull Request +# View changes in a Pull Request (GitHub or Azure DevOps) lumen diff --pr 123 # (--pr is optional) lumen diff https://github.com/owner/repo/pull/123 +lumen diff https://dev.azure.com/org/project/_git/repo/pullrequest/123 # Open the PR associated with the current branch lumen diff --detect-pr diff --git a/src/command/diff/app.rs b/src/command/diff/app.rs index 2af08187..1c7d31ab 100644 --- a/src/command/diff/app.rs +++ b/src/command/diff/app.rs @@ -37,10 +37,9 @@ fn open_tui_writer() -> io::Result> { use super::annotation::{AnnotationEditor, AnnotationEditorResult}; use super::coordinates::{extract_selected_text, PanelLayout}; -use super::git::{ - get_current_branch, load_file_diffs, load_pr_file_diffs, load_single_commit_diffs, -}; +use super::git::{get_current_branch, load_file_diffs, load_single_commit_diffs}; use super::highlight; +use super::pr_provider::{load_pr_file_diffs, pr_file_web_url}; use super::render::{ render_diff, render_empty_state, truncate_path, FilePickerItem, KeyBind, KeyBindSection, Modal, ModalContent, ModalFileStatus, ModalResult, @@ -281,8 +280,9 @@ pub fn run_app_stacked( run_app_internal(options, None, file_diffs, Some(commits), backend) } -/// Sync viewed files from GitHub to local state -fn sync_viewed_files_from_github(pr_info: &PrInfo, state: &mut AppState) { +/// Sync per-file viewed state from the hosting provider into local state. +/// No-op for providers without viewed-file support (e.g. Azure DevOps). +fn sync_viewed_files_from_provider(pr_info: &PrInfo, state: &mut AppState) { if let Ok(viewed_paths) = fetch_viewed_files(pr_info) { state.viewed_files.clear(); for (idx, diff) in state.file_diffs.iter().enumerate() { @@ -329,14 +329,14 @@ fn run_app_internal( state.init_stacked_mode(commits); } - // Load viewed files from GitHub on startup in PR mode (before TUI starts) + // Load viewed files from the provider on startup in PR mode (before TUI starts) if let Some(ref pr) = pr_info { let mut spinner = Spinner::new( spinners::Dots, format!("Syncing viewed status for {} files", state.file_diffs.len()), Color::Cyan, ); - sync_viewed_files_from_github(pr, &mut state); + sync_viewed_files_from_provider(pr, &mut state); let viewed_count = state.viewed_files.len(); spinner.success(&format!("{} files marked as viewed", viewed_count)); } @@ -389,7 +389,7 @@ fn run_app_internal( if state.needs_reload { let file_diffs = if let Some(ref pr) = pr_info { - // In PR mode, reload from GitHub + // In PR mode, reload from the hosting provider match load_pr_file_diffs(pr) { Ok(diffs) => diffs, Err(e) => { @@ -407,7 +407,7 @@ fn run_app_internal( // Re-sync viewed files from GitHub in PR mode if let Some(ref pr) = pr_info { - sync_viewed_files_from_github(pr, &mut state); + sync_viewed_files_from_provider(pr, &mut state); } } @@ -1960,14 +1960,9 @@ fn run_app_internal( if let Some(ref pr) = pr_info { if !state.file_diffs.is_empty() { let filename = &state.file_diffs[state.current_file].filename; - let file_url = format!( - "https://github.com/{}/{}/pull/{}/files#diff-{}", - pr.repo_owner, - pr.repo_name, - pr.number, - generate_file_anchor(filename) - ); - let _ = open_url(&file_url); + if let Some(file_url) = pr_file_web_url(pr, filename) { + let _ = open_url(&file_url); + } } } } @@ -2236,11 +2231,3 @@ fn open_url(url: &str) -> io::Result<()> { } Ok(()) } - -fn generate_file_anchor(filename: &str) -> String { - use sha2::{Digest, Sha256}; - - let mut hasher = Sha256::new(); - hasher.update(filename.as_bytes()); - format!("{:x}", hasher.finalize()) -} diff --git a/src/command/diff/azure.rs b/src/command/diff/azure.rs new file mode 100644 index 00000000..c72322b9 --- /dev/null +++ b/src/command/diff/azure.rs @@ -0,0 +1,482 @@ +//! Azure DevOps REST client for pull-request review. +//! +//! The `az` CLI has no first-class command to list a PR's changed files or +//! fetch file content, and the generic `az devops invoke` passthrough spawns a +//! Python process per call — unusable for an interactive diff. So we talk to the +//! Azure DevOps REST API directly. +//! +//! Diffs use the iteration-changes endpoint with `$compareTo=0`, i.e. the diff +//! against the PR's merge base — the same three-dot view the Azure web UI shows, +//! rather than a raw tip-to-tip comparison. +//! +//! Auth resolves to a PAT (`ADO_PAT` / `AZURE_DEVOPS_EXT_PAT`) via HTTP Basic, +//! or falls back to a bearer token from `az account get-access-token`. Only core +//! `az` (or a PAT) is required — not the `azure-devops` extension. + +use std::env; +use std::process::Command; +use std::sync::Arc; + +use reqwest::header::ACCEPT; +use serde_json::Value; +use tokio::sync::Semaphore; +use tokio::task::JoinSet; + +use super::git::build_file_diff; +use super::types::FileDiff; +use super::PrInfo; + +const API_VERSION: &str = "7.1"; +/// Azure DevOps OAuth resource id, used with `az account get-access-token`. +const ADO_RESOURCE: &str = "499b84ac-1321-427f-aa17-267ca6975798"; +/// Max concurrent blob fetches — keeps us well under Azure's rate limits while +/// still parallelising content retrieval. +const BLOB_CONCURRENCY: usize = 8; +/// Page size for the paginated iteration-changes endpoint. +const CHANGES_PAGE: usize = 1000; + +/// PR metadata needed to drive the diff UI. +pub struct AzurePrMeta { + pub source_ref: String, + pub target_ref: String, + pub repo_name: String, +} + +enum AdoAuth { + /// Personal access token, sent via HTTP Basic with an empty username. + Pat(String), + /// OAuth bearer token (from `az account get-access-token`). + Bearer(String), +} + +#[derive(Clone)] +struct AdoClient { + http: reqwest::Client, + /// Organisation base URL, e.g. `https://dev.azure.com/org`. + base: String, + project: String, + repo: String, + auth: Arc, +} + +impl AdoClient { + fn new(org_url: &str, project: &str, repo: &str) -> Result { + Ok(Self { + http: reqwest::Client::new(), + base: org_url.trim_end_matches('/').to_string(), + project: project.to_string(), + repo: repo.to_string(), + auth: Arc::new(resolve_auth()?), + }) + } + + fn authed(&self, rb: reqwest::RequestBuilder) -> reqwest::RequestBuilder { + match self.auth.as_ref() { + AdoAuth::Pat(pat) => rb.basic_auth("", Some(pat)), + AdoAuth::Bearer(token) => rb.bearer_auth(token), + } + } + + /// `{base}/{project}/_apis/git/...` with project URL-encoded. + fn git_url(&self, suffix: &str) -> String { + format!( + "{}/{}/_apis/git/{}", + self.base, + enc(&self.project), + suffix + ) + } + + async fn get_json(&self, url: &str) -> Result { + let resp = self + .authed(self.http.get(url)) + .header(ACCEPT, "application/json") + .send() + .await + .map_err(|e| format!("request failed: {}", e))?; + let status = resp.status(); + let body = resp.text().await.unwrap_or_default(); + if !status.is_success() { + return Err(auth_hint(status, &body)); + } + serde_json::from_str(&body).map_err(|e| format!("invalid JSON from Azure: {}", e)) + } + + /// Fetch a blob's text content. Returns empty string when `blob_id` is None + /// (the absent side of an add/delete) or on any error. + async fn blob_text(&self, blob_id: Option<&str>) -> String { + let Some(blob_id) = blob_id else { + return String::new(); + }; + let url = format!( + "{}?$format=text&api-version={}", + self.git_url(&format!("repositories/{}/blobs/{}", enc(&self.repo), blob_id)), + API_VERSION + ); + let Ok(resp) = self + .authed(self.http.get(&url)) + .header(ACCEPT, "text/plain") + .send() + .await + else { + return String::new(); + }; + if !resp.status().is_success() { + return String::new(); + } + // Read as bytes then lossy-decode so binary blobs degrade gracefully + // (build_file_diff flags them as binary downstream). + match resp.bytes().await { + Ok(bytes) => String::from_utf8_lossy(&bytes).into_owned(), + Err(_) => String::new(), + } + } + + async fn latest_iteration(&self, pr_id: u64) -> Result { + let url = format!( + "{}?api-version={}", + self.git_url(&format!( + "repositories/{}/pullRequests/{}/iterations", + enc(&self.repo), + pr_id + )), + API_VERSION + ); + let json = self.get_json(&url).await?; + json.get("value") + .and_then(|v| v.as_array()) + .and_then(|arr| arr.iter().filter_map(|i| i.get("id")?.as_u64()).max()) + .ok_or_else(|| "PR has no iterations".to_string()) + } + + /// All change entries for `iteration`, compared against the merge base + /// (`$compareTo=0`), following `$skip`/`$top` pagination. + async fn changes(&self, pr_id: u64, iteration: u64) -> Result, String> { + let mut entries = Vec::new(); + let mut skip = 0usize; + loop { + let url = format!( + "{}?$compareTo=0&$top={}&$skip={}&api-version={}", + self.git_url(&format!( + "repositories/{}/pullRequests/{}/iterations/{}/changes", + enc(&self.repo), + pr_id, + iteration + )), + CHANGES_PAGE, + skip, + API_VERSION + ); + let json = self.get_json(&url).await?; + let page = json + .get("changeEntries") + .and_then(|v| v.as_array()) + .cloned() + .unwrap_or_default(); + let page_len = page.len(); + for entry in page { + if let Some(change) = ChangeEntry::from_json(&entry) { + entries.push(change); + } + } + // `nextSkip` is the canonical "more pages" signal; fall back to a + // short page meaning we're done. + let next_skip = json.get("nextSkip").and_then(|v| v.as_u64()).unwrap_or(0); + if next_skip == 0 || page_len < CHANGES_PAGE { + break; + } + skip = next_skip as usize; + } + Ok(entries) + } + + async fn load_file_diffs(&self, pr_id: u64) -> Result, String> { + let iteration = self.latest_iteration(pr_id).await?; + let changes = self.changes(pr_id, iteration).await?; + + let sem = Arc::new(Semaphore::new(BLOB_CONCURRENCY)); + let mut set: JoinSet<(usize, FileDiff)> = JoinSet::new(); + for (idx, change) in changes.into_iter().enumerate() { + let client = self.clone(); + let sem = Arc::clone(&sem); + set.spawn(async move { + let _permit = sem.acquire_owned().await; + let old = client.blob_text(change.old_blob.as_deref()).await; + let new = client.blob_text(change.new_blob.as_deref()).await; + (idx, build_file_diff(change.path, old, new)) + }); + } + + // Reassemble in the original change order. + let mut out: Vec> = Vec::new(); + while let Some(res) = set.join_next().await { + let (idx, diff) = res.map_err(|e| format!("blob fetch task failed: {}", e))?; + if idx >= out.len() { + out.resize_with(idx + 1, || None); + } + out[idx] = Some(diff); + } + Ok(out.into_iter().flatten().collect()) + } +} + +/// One PR file change reduced to what the diff UI needs. +struct ChangeEntry { + /// Repo-relative path without a leading slash. + path: String, + /// Blob id of the new (head) side, if any. + new_blob: Option, + /// Blob id of the old (base) side, if any. + old_blob: Option, +} + +impl ChangeEntry { + fn from_json(entry: &Value) -> Option { + let item = entry.get("item"); + // Skip folders. + if item + .and_then(|i| i.get("isFolder")) + .and_then(|v| v.as_bool()) + .unwrap_or(false) + { + return None; + } + let raw_path = item + .and_then(|i| i.get("path")) + .and_then(|v| v.as_str()) + .or_else(|| entry.get("originalPath").and_then(|v| v.as_str()))?; + let new_blob = item + .and_then(|i| i.get("objectId")) + .and_then(|v| v.as_str()) + .filter(|s| !s.is_empty()) + .map(|s| s.to_string()); + let old_blob = item + .and_then(|i| i.get("originalObjectId")) + .and_then(|v| v.as_str()) + .filter(|s| !s.is_empty()) + .map(|s| s.to_string()); + Some(Self { + path: raw_path.trim_start_matches('/').to_string(), + new_blob, + old_blob, + }) + } +} + +// --------------------------------------------------------------------------- +// Public sync entry points (bridge the async client onto the sync diff path) +// --------------------------------------------------------------------------- + +pub fn fetch_pr_metadata( + org_url: &str, + project: &str, + repo: &str, + pr_id: u64, +) -> Result { + let client = AdoClient::new(org_url, project, repo)?; + block_on(async move { + // PR detail is project-scoped (not repo-scoped) in the REST API. + let url = format!( + "{}?api-version={}", + client.git_url(&format!("pullrequests/{}", pr_id)), + API_VERSION + ); + let json = client.get_json(&url).await?; + let source_ref = json + .get("sourceRefName") + .and_then(|v| v.as_str()) + .unwrap_or_default() + .to_string(); + let target_ref = json + .get("targetRefName") + .and_then(|v| v.as_str()) + .unwrap_or_default() + .to_string(); + let repo_name = json + .pointer("/repository/name") + .and_then(|v| v.as_str()) + .unwrap_or(repo) + .to_string(); + Ok(AzurePrMeta { + source_ref, + target_ref, + repo_name, + }) + }) +} + +pub fn detect_active_pr( + org_url: &str, + project: &str, + repo: &str, + branch: &str, +) -> Result { + let client = AdoClient::new(org_url, project, repo)?; + block_on(async move { + let url = format!( + "{}?searchCriteria.status=active&searchCriteria.sourceRefName={}&api-version={}", + client.git_url(&format!("repositories/{}/pullrequests", enc(repo))), + enc(&format!("refs/heads/{}", branch)), + API_VERSION + ); + let json = client.get_json(&url).await?; + json.get("value") + .and_then(|v| v.as_array()) + .and_then(|arr| arr.first()) + .and_then(|pr| pr.get("pullRequestId")) + .and_then(|v| v.as_u64()) + .ok_or_else(|| format!("No active PR found for branch {}", branch)) + }) +} + +pub fn load_pr_file_diffs(pr: &PrInfo) -> Result, String> { + let org_url = pr + .org_url + .as_deref() + .ok_or("Azure PR missing organisation URL")?; + let project = pr + .project + .as_deref() + .ok_or("Azure PR missing project")?; + let client = AdoClient::new(org_url, project, &pr.repo_name)?; + let pr_id = pr.number; + block_on(async move { client.load_file_diffs(pr_id).await }) +} + +// --------------------------------------------------------------------------- +// Helpers +// --------------------------------------------------------------------------- + +/// Run an async future to completion from the synchronous diff path. `main` is +/// a multi-threaded `#[tokio::main]`, so we mark the current worker as blocking +/// and drive the future on the existing runtime — no second runtime, no new deps. +fn block_on(fut: F) -> F::Output { + tokio::task::block_in_place(|| tokio::runtime::Handle::current().block_on(fut)) +} + +fn resolve_auth() -> Result { + for var in ["ADO_PAT", "AZURE_DEVOPS_EXT_PAT", "AZURE_DEVOPS_PAT"] { + if let Ok(pat) = env::var(var) { + if !pat.trim().is_empty() { + return Ok(AdoAuth::Pat(pat)); + } + } + } + // Fall back to an OAuth token from the Azure CLI (`az login`). + let output = Command::new("az") + .args([ + "account", + "get-access-token", + "--resource", + ADO_RESOURCE, + "-o", + "json", + ]) + .output() + .map_err(|e| { + format!( + "No Azure DevOps credentials: set ADO_PAT or install the Azure CLI and run `az login` ({})", + e + ) + })?; + if !output.status.success() { + return Err( + "No Azure DevOps credentials: set ADO_PAT, or run `az login` to use the Azure CLI." + .to_string(), + ); + } + let json: Value = serde_json::from_slice(&output.stdout) + .map_err(|e| format!("Could not parse az token output: {}", e))?; + json.get("accessToken") + .and_then(|v| v.as_str()) + .map(|t| AdoAuth::Bearer(t.to_string())) + .ok_or_else(|| "az returned no access token".to_string()) +} + +fn auth_hint(status: reqwest::StatusCode, body: &str) -> String { + if status == reqwest::StatusCode::UNAUTHORIZED || status == reqwest::StatusCode::NON_AUTHORITATIVE_INFORMATION { + return "Azure DevOps auth failed (401). Check ADO_PAT scopes (Code: Read) or run `az login`.".to_string(); + } + if status == reqwest::StatusCode::FORBIDDEN { + return "Azure DevOps returned 403. The token lacks access to this repository.".to_string(); + } + let snippet: String = body.chars().take(200).collect(); + format!("Azure DevOps request failed ({}): {}", status, snippet.trim()) +} + +/// Percent-encode one URL path/query segment (keeps RFC 3986 unreserved chars). +fn enc(segment: &str) -> String { + use std::fmt::Write; + let mut out = String::with_capacity(segment.len()); + for b in segment.bytes() { + match b { + b'A'..=b'Z' | b'a'..=b'z' | b'0'..=b'9' | b'-' | b'_' | b'.' | b'~' => { + out.push(b as char) + } + _ => { + let _ = write!(out, "%{:02X}", b); + } + } + } + out +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parses_add_edit_delete_changes() { + let add = ChangeEntry::from_json(&serde_json::json!({ + "changeType": "add", + "item": { "path": "/src/new.rs", "objectId": "newsha" } + })) + .unwrap(); + assert_eq!(add.path, "src/new.rs"); + assert_eq!(add.new_blob.as_deref(), Some("newsha")); + assert_eq!(add.old_blob, None); + + let edit = ChangeEntry::from_json(&serde_json::json!({ + "changeType": "edit", + "item": { "path": "/a.txt", "objectId": "n", "originalObjectId": "o" } + })) + .unwrap(); + assert_eq!(edit.new_blob.as_deref(), Some("n")); + assert_eq!(edit.old_blob.as_deref(), Some("o")); + + let del = ChangeEntry::from_json(&serde_json::json!({ + "changeType": "delete", + "item": { "path": "/gone.rs", "originalObjectId": "o" } + })) + .unwrap(); + assert_eq!(del.new_blob, None); + assert_eq!(del.old_blob.as_deref(), Some("o")); + } + + #[test] + fn skips_folders() { + assert!(ChangeEntry::from_json(&serde_json::json!({ + "changeType": "add", + "item": { "path": "/dir", "isFolder": true } + })) + .is_none()); + } + + #[test] + fn falls_back_to_original_path_on_rename() { + let renamed = ChangeEntry::from_json(&serde_json::json!({ + "changeType": "rename", + "item": { "objectId": "n", "originalObjectId": "o" }, + "originalPath": "/old/name.rs" + })) + .unwrap(); + assert_eq!(renamed.path, "old/name.rs"); + } + + #[test] + fn encodes_segments() { + assert_eq!(enc("My Project"), "My%20Project"); + assert_eq!(enc("refs/heads/feature/x"), "refs%2Fheads%2Ffeature%2Fx"); + assert_eq!(enc("simple-repo.git"), "simple-repo.git"); + } +} diff --git a/src/command/diff/git.rs b/src/command/diff/git.rs index 44ec6286..57d7a968 100644 --- a/src/command/diff/git.rs +++ b/src/command/diff/git.rs @@ -141,6 +141,26 @@ pub fn get_new_content(filename: &str, refs: &DiffRefs, backend: &dyn VcsBackend } } +/// Assemble a `FileDiff` from a filename and its two sides, deriving the file +/// status and binary flag from the contents. +pub fn build_file_diff(filename: String, old_content: String, new_content: String) -> FileDiff { + let status = if old_content.is_empty() && !new_content.is_empty() { + FileStatus::Added + } else if !old_content.is_empty() && new_content.is_empty() { + FileStatus::Deleted + } else { + FileStatus::Modified + }; + let is_binary = is_binary_content(&old_content) || is_binary_content(&new_content); + FileDiff { + filename, + old_content, + new_content, + status, + is_binary, + } +} + pub fn load_file_diffs(options: &DiffOptions, backend: &dyn VcsBackend) -> Vec { let refs = DiffRefs::from_options(options, backend); get_changed_files(options, backend) @@ -148,27 +168,14 @@ pub fn load_file_diffs(options: &DiffOptions, backend: &dyn VcsBackend) -> Vec Result, String> { +/// GitHub implementation of PR diff loading: list changed files via `gh pr +/// diff`, then fetch each file's base/head content via the GitHub contents API. +pub fn github_load_pr_file_diffs(pr_info: &PrInfo) -> Result, String> { let repo_arg = format!("{}/{}", pr_info.repo_owner, pr_info.repo_name); let mut spinner = Spinner::new( @@ -236,23 +243,7 @@ pub fn load_pr_file_diffs(pr_info: &PrInfo) -> Result, String> { .into_iter() .zip(contents.into_iter()) .map(|(filename, (old_content, new_content))| { - let status = if old_content.is_empty() && !new_content.is_empty() { - FileStatus::Added - } else if !old_content.is_empty() && new_content.is_empty() { - FileStatus::Deleted - } else { - FileStatus::Modified - }; - - let is_binary = - is_binary_content(&old_content) || is_binary_content(&new_content); - FileDiff { - filename, - old_content, - new_content, - status, - is_binary, - } + build_file_diff(filename, old_content, new_content) }) .collect(); @@ -473,23 +464,7 @@ pub fn load_single_commit_diffs( .get_file_content_at_ref(commit_id, path) .unwrap_or_default(); - let status = if old_content.is_empty() && !new_content.is_empty() { - FileStatus::Added - } else if !old_content.is_empty() && new_content.is_empty() { - FileStatus::Deleted - } else { - FileStatus::Modified - }; - - let is_binary = - is_binary_content(&old_content) || is_binary_content(&new_content); - FileDiff { - filename, - old_content, - new_content, - status, - is_binary, - } + build_file_diff(filename, old_content, new_content) }) .collect() } diff --git a/src/command/diff/mod.rs b/src/command/diff/mod.rs index c1ce8199..0b09c550 100644 --- a/src/command/diff/mod.rs +++ b/src/command/diff/mod.rs @@ -1,11 +1,13 @@ mod annotation; mod app; +mod azure; mod context; mod coordinates; mod diff_algo; pub mod git; mod global_search; pub mod highlight; +pub mod pr_provider; mod render; mod search; mod state; @@ -18,13 +20,16 @@ mod watcher; use std::collections::HashSet; use std::io; use std::process::{self, Command}; -use std::thread; use spinoff::{spinners, Color, Spinner}; use crate::commit_reference::CommitReference; use crate::vcs::VcsBackend; +pub use pr_provider::{ + fetch_viewed_files, mark_file_as_viewed_async, unmark_file_as_viewed_async, ProviderKind, +}; + pub struct DiffOptions { pub reference: Option, pub pr: Option, @@ -40,6 +45,7 @@ pub struct DiffOptions { #[derive(Clone)] pub struct PrInfo { + pub provider: ProviderKind, pub number: u64, pub node_id: String, pub repo_owner: String, @@ -48,9 +54,13 @@ pub struct PrInfo { pub head_ref: String, pub base_repo_owner: String, pub head_repo_owner: Option, // None if head repo was deleted (fork deleted) + /// Azure DevOps project (None for GitHub). + pub project: Option, + /// Azure DevOps organisation base URL, e.g. `https://dev.azure.com/org`. + pub org_url: Option, } -fn parse_pr_input(input: &str) -> Option<(Option, Option, u64)> { +fn github_parse_pr_input(input: &str) -> Option<(Option, Option, u64)> { // Try to parse as a URL first if input.starts_with("http://") || input.starts_with("https://") { // Extract PR number and repo info from URL @@ -78,7 +88,7 @@ fn parse_pr_input(input: &str) -> Option<(Option, Option, u64)> } } -fn resolve_origin_repo() -> Result { +fn github_resolve_origin_repo() -> Result { let output = Command::new("git") .args(["remote", "get-url", "origin"]) .output() @@ -104,8 +114,11 @@ fn resolve_origin_repo() -> Result { } } -fn fetch_pr_info(pr_input: &str, repo_override: Option<&str>) -> Result { - let (owner, repo, number) = parse_pr_input(pr_input).ok_or_else(|| { +pub(crate) fn github_fetch_pr_info( + pr_input: &str, + repo_override: Option<&str>, +) -> Result { + let (owner, repo, number) = github_parse_pr_input(pr_input).ok_or_else(|| { format!( "Invalid PR reference: {}. Use a PR number or URL.", pr_input @@ -115,7 +128,7 @@ fn fetch_pr_info(pr_input: &str, repo_override: Option<&str>) -> Result format!("{}/{}", o, r), (_, _, Some(r)) => r.to_string(), - _ => resolve_origin_repo()?, + _ => github_resolve_origin_repo()?, }; let (repo_owner, repo_name) = { @@ -161,6 +174,7 @@ fn fetch_pr_info(pr_input: &str, repo_override: Option<&str>) -> Result) -> Result Option { } /// Fetch the list of files that are marked as viewed on GitHub -pub fn fetch_viewed_files(pr_info: &PrInfo) -> Result, String> { +pub(crate) fn github_fetch_viewed_files(pr_info: &PrInfo) -> Result, String> { let query = format!( r#"query {{ repository(owner: "{}", name: "{}") {{ pullRequest(number: {}) {{ files(first: 100) {{ nodes {{ path viewerViewedState }} }} }} }} }}"#, pr_info.repo_owner, pr_info.repo_name, pr_info.number @@ -258,28 +274,11 @@ pub fn fetch_viewed_files(pr_info: &PrInfo) -> Result, String> { Ok(viewed_files) } -/// Mark a file as viewed on GitHub PR (non-blocking, spawns a thread) -pub fn mark_file_as_viewed_async(pr_info: &PrInfo, file_path: &str) { - let node_id = pr_info.node_id.clone(); - let path = file_path.to_string(); - - thread::spawn(move || { - let _ = mark_file_as_viewed_sync(&node_id, &path); - }); -} - -/// Unmark a file as viewed on GitHub PR (non-blocking, spawns a thread) -pub fn unmark_file_as_viewed_async(pr_info: &PrInfo, file_path: &str) { - let node_id = pr_info.node_id.clone(); - let path = file_path.to_string(); - - thread::spawn(move || { - let _ = unmark_file_as_viewed_sync(&node_id, &path); - }); -} - /// Mark a file as viewed on GitHub PR (blocking) -fn mark_file_as_viewed_sync(node_id: &str, file_path: &str) -> Result<(), String> { +pub(crate) fn github_mark_file_as_viewed_sync( + node_id: &str, + file_path: &str, +) -> Result<(), String> { let mutation = format!( r#"mutation {{ markFileAsViewed(input: {{ pullRequestId: "{}", path: "{}" }}) {{ clientMutationId }} }}"#, node_id, file_path @@ -299,7 +298,10 @@ fn mark_file_as_viewed_sync(node_id: &str, file_path: &str) -> Result<(), String } /// Unmark a file as viewed on GitHub PR (blocking) -fn unmark_file_as_viewed_sync(node_id: &str, file_path: &str) -> Result<(), String> { +pub(crate) fn github_unmark_file_as_viewed_sync( + node_id: &str, + file_path: &str, +) -> Result<(), String> { let mutation = format!( r#"mutation {{ unmarkFileAsViewed(input: {{ pullRequestId: "{}", path: "{}" }}) {{ clientMutationId }} }}"#, node_id, file_path @@ -318,7 +320,7 @@ fn unmark_file_as_viewed_sync(node_id: &str, file_path: &str) -> Result<(), Stri Ok(()) } -fn detect_current_branch_pr() -> Result { +pub(crate) fn github_detect_current_branch_pr() -> Result { let output = Command::new("gh") .args(["pr", "view", "--json", "number", "-q", ".number"]) .output() @@ -346,7 +348,7 @@ pub fn run_diff_ui(mut options: DiffOptions, backend: &dyn VcsBackend) -> io::Re "Detecting PR for current branch", Color::Cyan, ); - match detect_current_branch_pr() { + match pr_provider::detect_current_branch_pr(options.origin.as_deref()) { Ok(number) => { spinner.success(&format!("Detected PR #{}", number)); options.pr = Some(number); @@ -360,17 +362,8 @@ pub fn run_diff_ui(mut options: DiffOptions, backend: &dyn VcsBackend) -> io::Re // Handle PR mode if let Some(ref pr_input) = options.pr { - let spinner_msg = match parse_pr_input(pr_input) { - Some((Some(owner), Some(repo), number)) => { - format!("Fetching PR {}/{}#{}", owner, repo, number) - } - Some((_, _, number)) => { - format!("Fetching PR #{}", number) - } - None => "Fetching PR".to_string(), - }; - let mut spinner = Spinner::new(spinners::Dots, spinner_msg, Color::Cyan); - match fetch_pr_info(pr_input, options.origin.as_deref()) { + let mut spinner = Spinner::new(spinners::Dots, "Fetching PR metadata", Color::Cyan); + match pr_provider::fetch_pr_info(pr_input, options.origin.as_deref()) { Ok(pr_info) => { spinner.success("Fetched PR metadata"); return app::run_app_with_pr(options, pr_info, backend); @@ -384,18 +377,9 @@ pub fn run_diff_ui(mut options: DiffOptions, backend: &dyn VcsBackend) -> io::Re // Also check if the reference looks like a PR (number or URL) if let Some(CommitReference::Single(ref input)) = options.reference { - if input.contains("/pull/") || input.parse::().is_ok() { - let spinner_msg = match parse_pr_input(input) { - Some((Some(owner), Some(repo), number)) => { - format!("Fetching PR {}/{}#{}", owner, repo, number) - } - Some((_, _, number)) => { - format!("Fetching PR #{}", number) - } - None => "Fetching PR".to_string(), - }; - let mut spinner = Spinner::new(spinners::Dots, spinner_msg, Color::Cyan); - match fetch_pr_info(input, options.origin.as_deref()) { + if pr_provider::is_pr_reference(input) { + let mut spinner = Spinner::new(spinners::Dots, "Fetching PR metadata", Color::Cyan); + match pr_provider::fetch_pr_info(input, options.origin.as_deref()) { Ok(pr_info) => { spinner.success("Fetched PR metadata"); return app::run_app_with_pr(options, pr_info, backend); diff --git a/src/command/diff/pr_provider.rs b/src/command/diff/pr_provider.rs new file mode 100644 index 00000000..645268dc --- /dev/null +++ b/src/command/diff/pr_provider.rs @@ -0,0 +1,572 @@ +//! Pull-request hosting provider abstraction. +//! +//! `lumen diff --pr` originally only understood GitHub (it shelled out to the +//! `gh` CLI everywhere). This module introduces a [`PrProvider`] trait so other +//! forges can be supported, with `gh` for GitHub and `az` (the Azure DevOps CLI) +//! for Azure DevOps. New providers (e.g. `glab` for GitLab) only need to add an +//! impl and wire it into provider detection. + +use std::collections::HashSet; +use std::process::Command; +use std::thread; + +use super::azure; +use super::git::github_load_pr_file_diffs; +use super::types::FileDiff; +use super::{ + github_detect_current_branch_pr, github_fetch_pr_info, github_fetch_viewed_files, + github_mark_file_as_viewed_sync, github_unmark_file_as_viewed_sync, PrInfo, +}; + +/// Which hosting provider a [`PrInfo`] belongs to. Stored on `PrInfo` so the +/// runtime (viewed-file sync, open-in-browser, reloads) can route back to the +/// right provider without re-parsing the original input. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum ProviderKind { + GitHub, + Azure, +} + +impl ProviderKind { + /// The provider implementation. Both providers are zero-sized, so this hands + /// back a `'static` reference with no allocation. + pub fn handler(self) -> &'static dyn PrProvider { + match self { + ProviderKind::GitHub => &GitHubProvider, + ProviderKind::Azure => &AzureProvider, + } + } +} + +/// A pull-request hosting provider. Each method maps to one capability the diff +/// UI needs; the viewed-file sync methods default to no-ops so providers without +/// that concept (Azure DevOps) don't have to implement them. +pub trait PrProvider { + /// Does this provider recognise `input` as one of its PR URLs? + fn matches_url(&self, input: &str) -> bool; + + /// Does this provider recognise `origin` (a git remote URL) as one of its + /// repositories? Used to pick a provider for bare PR numbers. + fn matches_origin(&self, origin: &str) -> bool; + + /// Resolve a PR number/URL into full metadata. + fn fetch_pr_info(&self, input: &str, repo_override: Option<&str>) -> Result; + + /// Find the PR associated with the current branch. + fn detect_current_branch_pr(&self, repo_override: Option<&str>) -> Result; + + /// Load the file diffs for a PR. + fn load_pr_file_diffs(&self, pr: &PrInfo) -> Result, String>; + + /// Whether this provider supports syncing per-file "viewed" state. + fn supports_viewed_sync(&self) -> bool { + false + } + + /// Fetch the set of paths currently marked as viewed. + fn fetch_viewed_files(&self, _pr: &PrInfo) -> Result, String> { + Ok(HashSet::new()) + } + + /// Mark/unmark a file as viewed (blocking). + fn set_file_viewed(&self, _pr: &PrInfo, _path: &str, _viewed: bool) -> Result<(), String> { + Ok(()) + } + + /// Build a browser URL for `filename` within the PR. + fn file_web_url(&self, pr: &PrInfo, filename: &str) -> Option; +} + +// --------------------------------------------------------------------------- +// Provider selection +// --------------------------------------------------------------------------- + +/// True if `input` looks like a PR reference (a known PR URL or a bare number). +pub fn is_pr_reference(input: &str) -> bool { + GitHubProvider.matches_url(input) + || AzureProvider.matches_url(input) + || input.parse::().is_ok() +} + +fn read_origin_url() -> Option { + let output = Command::new("git") + .args(["remote", "get-url", "origin"]) + .output() + .ok()?; + if !output.status.success() { + return None; + } + let url = String::from_utf8_lossy(&output.stdout).trim().to_string(); + if url.is_empty() { + None + } else { + Some(url) + } +} + +/// Pick a provider from the git `origin` remote (and any `--origin` override), +/// defaulting to GitHub when nothing matches. +fn provider_for_origin(repo_override: Option<&str>) -> &'static dyn PrProvider { + let candidates = [repo_override.map(|s| s.to_string()), read_origin_url()]; + for candidate in candidates.into_iter().flatten() { + if AzureProvider.matches_origin(&candidate) { + return ProviderKind::Azure.handler(); + } + } + ProviderKind::GitHub.handler() +} + +/// Pick a provider from a PR URL/number, falling back to origin detection. +fn provider_for_input(input: &str, repo_override: Option<&str>) -> &'static dyn PrProvider { + if AzureProvider.matches_url(input) { + return ProviderKind::Azure.handler(); + } + if GitHubProvider.matches_url(input) { + return ProviderKind::GitHub.handler(); + } + provider_for_origin(repo_override) +} + +// --------------------------------------------------------------------------- +// Dispatchers used by the rest of the diff UI +// --------------------------------------------------------------------------- + +pub fn fetch_pr_info(input: &str, repo_override: Option<&str>) -> Result { + provider_for_input(input, repo_override).fetch_pr_info(input, repo_override) +} + +pub fn detect_current_branch_pr(repo_override: Option<&str>) -> Result { + provider_for_origin(repo_override).detect_current_branch_pr(repo_override) +} + +pub fn load_pr_file_diffs(pr: &PrInfo) -> Result, String> { + pr.provider.handler().load_pr_file_diffs(pr) +} + +pub fn fetch_viewed_files(pr: &PrInfo) -> Result, String> { + pr.provider.handler().fetch_viewed_files(pr) +} + +pub fn pr_file_web_url(pr: &PrInfo, filename: &str) -> Option { + pr.provider.handler().file_web_url(pr, filename) +} + +pub fn mark_file_as_viewed_async(pr: &PrInfo, file_path: &str) { + set_file_viewed_async(pr, file_path, true); +} + +pub fn unmark_file_as_viewed_async(pr: &PrInfo, file_path: &str) { + set_file_viewed_async(pr, file_path, false); +} + +fn set_file_viewed_async(pr: &PrInfo, file_path: &str, viewed: bool) { + if !pr.provider.handler().supports_viewed_sync() { + return; + } + let pr = pr.clone(); + let path = file_path.to_string(); + thread::spawn(move || { + let _ = pr.provider.handler().set_file_viewed(&pr, &path, viewed); + }); +} + +/// SHA-256 file anchor used by GitHub's PR "Files changed" deep links +/// (`#diff-`). +fn github_file_anchor(filename: &str) -> String { + use sha2::{Digest, Sha256}; + let mut hasher = Sha256::new(); + hasher.update(filename.as_bytes()); + format!("{:x}", hasher.finalize()) +} + +// --------------------------------------------------------------------------- +// GitHub +// --------------------------------------------------------------------------- + +pub struct GitHubProvider; + +impl PrProvider for GitHubProvider { + fn matches_url(&self, input: &str) -> bool { + input.starts_with("http") && input.contains("/pull/") + } + + fn matches_origin(&self, origin: &str) -> bool { + origin.contains("github.com") + } + + fn fetch_pr_info(&self, input: &str, repo_override: Option<&str>) -> Result { + github_fetch_pr_info(input, repo_override) + } + + fn detect_current_branch_pr(&self, _repo_override: Option<&str>) -> Result { + github_detect_current_branch_pr() + } + + fn load_pr_file_diffs(&self, pr: &PrInfo) -> Result, String> { + github_load_pr_file_diffs(pr) + } + + fn supports_viewed_sync(&self) -> bool { + true + } + + fn fetch_viewed_files(&self, pr: &PrInfo) -> Result, String> { + github_fetch_viewed_files(pr) + } + + fn set_file_viewed(&self, pr: &PrInfo, path: &str, viewed: bool) -> Result<(), String> { + if viewed { + github_mark_file_as_viewed_sync(&pr.node_id, path) + } else { + github_unmark_file_as_viewed_sync(&pr.node_id, path) + } + } + + fn file_web_url(&self, pr: &PrInfo, filename: &str) -> Option { + Some(format!( + "https://github.com/{}/{}/pull/{}/files#diff-{}", + pr.repo_owner, + pr.repo_name, + pr.number, + github_file_anchor(filename) + )) + } +} + +// --------------------------------------------------------------------------- +// Azure DevOps +// --------------------------------------------------------------------------- + +pub struct AzureProvider; + +/// The coordinates of an Azure DevOps repository / PR, parsed from a URL or a +/// git remote. +struct AzureRef { + /// Organisation base URL, e.g. `https://dev.azure.com/myorg`. + org_url: String, + /// Short organisation name, e.g. `myorg`. + org: String, + project: String, + repo: String, + /// PR id when parsed from a PR URL. + id: Option, +} + +impl AzureProvider { + fn resolve_ref(&self, input: &str, repo_override: Option<&str>) -> Result { + if let Some(parsed) = parse_azure_url(input) { + return Ok(parsed); + } + // Bare PR number: take the coordinates from --origin (if it's an Azure + // URL) or from the git `origin` remote. + let id = input + .parse::() + .map_err(|_| format!("Invalid Azure DevOps PR reference: {}", input))?; + let remote = repo_override + .filter(|o| self.matches_origin(o)) + .map(|s| s.to_string()) + .or_else(read_origin_url) + .ok_or_else(|| { + "Could not determine Azure DevOps repository. Run inside the repo or pass a PR URL." + .to_string() + })?; + let mut parsed = parse_azure_remote(&remote) + .ok_or_else(|| format!("Could not parse Azure DevOps remote: {}", remote))?; + parsed.id = Some(id); + Ok(parsed) + } +} + +impl PrProvider for AzureProvider { + fn matches_url(&self, input: &str) -> bool { + let host_ok = input.contains("dev.azure.com") || input.contains(".visualstudio.com"); + host_ok && (input.contains("/pullrequest/") || input.contains("/_git/")) + } + + fn matches_origin(&self, origin: &str) -> bool { + origin.contains("dev.azure.com") || origin.contains(".visualstudio.com") + } + + fn fetch_pr_info(&self, input: &str, repo_override: Option<&str>) -> Result { + let az = self.resolve_ref(input, repo_override)?; + let id = az + .id + .ok_or_else(|| format!("No PR id found in: {}", input))?; + + let meta = azure::fetch_pr_metadata(&az.org_url, &az.project, &az.repo, id)?; + + Ok(PrInfo { + provider: ProviderKind::Azure, + number: id, + node_id: String::new(), + repo_owner: az.org.clone(), + // Prefer the repo name the API reports; fall back to the URL's. + repo_name: if meta.repo_name.is_empty() { + az.repo + } else { + meta.repo_name + }, + base_ref: strip_ref_prefix(&meta.target_ref), + head_ref: strip_ref_prefix(&meta.source_ref), + base_repo_owner: az.org.clone(), + head_repo_owner: Some(az.org), + project: Some(az.project), + org_url: Some(az.org_url), + }) + } + + fn detect_current_branch_pr(&self, repo_override: Option<&str>) -> Result { + let remote = repo_override + .filter(|o| self.matches_origin(o)) + .map(|s| s.to_string()) + .or_else(read_origin_url) + .ok_or_else(|| "Could not determine Azure DevOps repository.".to_string())?; + let az = parse_azure_remote(&remote) + .ok_or_else(|| format!("Could not parse Azure DevOps remote: {}", remote))?; + + let branch_out = Command::new("git") + .args(["rev-parse", "--abbrev-ref", "HEAD"]) + .output() + .map_err(|e| format!("Failed to run git: {}", e))?; + let branch = String::from_utf8_lossy(&branch_out.stdout).trim().to_string(); + if branch.is_empty() { + return Err("Could not determine the current branch".to_string()); + } + + let id = azure::detect_active_pr(&az.org_url, &az.project, &az.repo, &branch)?; + Ok(id.to_string()) + } + + fn load_pr_file_diffs(&self, pr: &PrInfo) -> Result, String> { + azure::load_pr_file_diffs(pr) + } + + fn file_web_url(&self, pr: &PrInfo, filename: &str) -> Option { + let org_url = pr.org_url.as_ref()?; + let project = pr.project.as_ref()?; + Some(format!( + "{}/{}/_git/{}/pullrequest/{}?path={}", + org_url, + project, + pr.repo_name, + pr.number, + encode_path(&format!("/{}", filename)) + )) + } +} + +/// Strip a `refs/heads/` (or `refs/`) prefix from an Azure ref name. +fn strip_ref_prefix(ref_name: &str) -> String { + ref_name + .strip_prefix("refs/heads/") + .or_else(|| ref_name.strip_prefix("refs/")) + .unwrap_or(ref_name) + .to_string() +} + +/// Extract `(org_url, org, project, repo)` from the host+path segments of an +/// Azure DevOps HTTPS URL or remote. Shared by URL and remote parsing. +fn azure_coords_from_parts(parts: &[&str]) -> Option<(String, String, String, String)> { + let host = *parts.first()?; + let git_idx = parts.iter().position(|&p| p == "_git")?; + if git_idx == 0 || git_idx + 1 >= parts.len() { + return None; + } + let project = decode_component(parts[git_idx - 1]); + let repo = decode_component(parts[git_idx + 1]); + + let (org_url, org) = if host == "dev.azure.com" { + let org = (*parts.get(1)?).to_string(); + (format!("https://dev.azure.com/{}", org), org) + } else if let Some(org) = host.strip_suffix(".visualstudio.com") { + (format!("https://{}", host), org.to_string()) + } else { + return None; + }; + + Some((org_url, org, project, repo)) +} + +/// Parse an Azure DevOps PR URL into its coordinates. +/// +/// Handles `https://dev.azure.com/{org}/{project}/_git/{repo}/pullrequest/{id}` +/// and `https://{org}.visualstudio.com/{project}/_git/{repo}/pullrequest/{id}`. +fn parse_azure_url(input: &str) -> Option { + if !input.starts_with("http") { + return None; + } + let no_query = input.split('?').next().unwrap_or(input); + let no_scheme = no_query + .trim_start_matches("https://") + .trim_start_matches("http://") + .trim_end_matches('/'); + let parts: Vec<&str> = no_scheme.split('/').collect(); + let (org_url, org, project, repo) = azure_coords_from_parts(&parts)?; + + let id = parts + .iter() + .position(|p| p.eq_ignore_ascii_case("pullrequest")) + .and_then(|i| parts.get(i + 1)) + .and_then(|s| s.parse::().ok()); + + Some(AzureRef { + org_url, + org, + project, + repo, + id, + }) +} + +/// Parse an Azure DevOps git remote URL into repository coordinates. +/// +/// Handles HTTPS (`https://[org@]dev.azure.com/{org}/{project}/_git/{repo}`, +/// `https://{org}.visualstudio.com/[collection/]{project}/_git/{repo}`) and SSH +/// (`git@ssh.dev.azure.com:v3/{org}/{project}/{repo}`). +fn parse_azure_remote(remote: &str) -> Option { + let remote = remote.trim().trim_end_matches(".git"); + + // SSH: git@ssh.dev.azure.com:v3/org/project/repo + if let Some(rest) = remote.split("ssh.dev.azure.com:").nth(1) { + let mut segs = rest.trim_start_matches('/').split('/'); + // Drop a leading "v3" path component when present. + let first = segs.next()?; + let org = if first == "v3" { segs.next()? } else { first }; + let project = segs.next()?; + let repo = segs.next()?; + return Some(AzureRef { + org_url: format!("https://dev.azure.com/{}", org), + org: org.to_string(), + project: decode_component(project), + repo: decode_component(repo), + id: None, + }); + } + + // HTTPS variants share the `/_git/` marker. + let no_scheme = remote + .trim_start_matches("https://") + .trim_start_matches("http://"); + // Strip any `user@` userinfo from the host segment. + let no_userinfo = match no_scheme.split_once('@') { + Some((_, after)) if after.contains('/') => after, + _ => no_scheme, + }; + let parts: Vec<&str> = no_userinfo.split('/').collect(); + let (org_url, org, project, repo) = azure_coords_from_parts(&parts)?; + + Some(AzureRef { + org_url, + org, + project, + repo, + id: None, + }) +} + +/// Decode the small set of percent-escapes that show up in Azure path segments +/// (notably `%20` for spaces in project names). +fn decode_component(segment: &str) -> String { + segment.replace("%20", " ") +} + +/// Minimal percent-encoding for an Azure `?path=` query value. +fn encode_path(path: &str) -> String { + use std::fmt::Write; + let mut out = String::with_capacity(path.len()); + for b in path.bytes() { + match b { + b'A'..=b'Z' | b'a'..=b'z' | b'0'..=b'9' | b'-' | b'_' | b'.' | b'~' => { + out.push(b as char) + } + _ => { + let _ = write!(out, "%{:02X}", b); + } + } + } + out +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn github_matches_pull_urls() { + assert!(GitHubProvider.matches_url("https://github.com/owner/repo/pull/123")); + assert!(!GitHubProvider.matches_url("https://dev.azure.com/o/p/_git/r/pullrequest/1")); + assert!(GitHubProvider.matches_origin("git@github.com:owner/repo.git")); + } + + #[test] + fn azure_matches_pr_urls() { + assert!(AzureProvider.matches_url("https://dev.azure.com/o/p/_git/r/pullrequest/42")); + assert!(AzureProvider.matches_url("https://myorg.visualstudio.com/p/_git/r/pullrequest/7")); + assert!(!AzureProvider.matches_url("https://github.com/owner/repo/pull/123")); + } + + #[test] + fn parse_azure_devazure_url() { + let r = parse_azure_url("https://dev.azure.com/myorg/MyProject/_git/myrepo/pullrequest/55") + .expect("should parse"); + assert_eq!(r.org_url, "https://dev.azure.com/myorg"); + assert_eq!(r.project, "MyProject"); + assert_eq!(r.repo, "myrepo"); + assert_eq!(r.id, Some(55)); + } + + #[test] + fn parse_azure_visualstudio_url() { + let r = parse_azure_url("https://myorg.visualstudio.com/MyProject/_git/myrepo/pullrequest/9") + .expect("should parse"); + assert_eq!(r.org_url, "https://myorg.visualstudio.com"); + assert_eq!(r.project, "MyProject"); + assert_eq!(r.repo, "myrepo"); + assert_eq!(r.id, Some(9)); + } + + #[test] + fn parse_azure_url_with_encoded_project() { + let r = parse_azure_url("https://dev.azure.com/org/My%20Project/_git/repo/pullrequest/1") + .expect("should parse"); + assert_eq!(r.project, "My Project"); + } + + #[test] + fn parse_azure_https_remote() { + let r = parse_azure_remote("https://myorg@dev.azure.com/myorg/MyProject/_git/myrepo") + .expect("should parse"); + assert_eq!(r.org_url, "https://dev.azure.com/myorg"); + assert_eq!(r.project, "MyProject"); + assert_eq!(r.repo, "myrepo"); + } + + #[test] + fn parse_azure_ssh_remote() { + let r = parse_azure_remote("git@ssh.dev.azure.com:v3/myorg/MyProject/myrepo") + .expect("should parse"); + assert_eq!(r.org_url, "https://dev.azure.com/myorg"); + assert_eq!(r.project, "MyProject"); + assert_eq!(r.repo, "myrepo"); + } + + #[test] + fn strip_ref_prefixes() { + assert_eq!(strip_ref_prefix("refs/heads/main"), "main"); + assert_eq!(strip_ref_prefix("refs/tags/v1"), "tags/v1"); + assert_eq!(strip_ref_prefix("feature/x"), "feature/x"); + } + + #[test] + fn encodes_path_query() { + assert_eq!(encode_path("/src/main.rs"), "%2Fsrc%2Fmain.rs"); + } + + #[test] + fn is_pr_reference_detects_forms() { + assert!(is_pr_reference("123")); + assert!(is_pr_reference("https://github.com/o/r/pull/1")); + assert!(is_pr_reference("https://dev.azure.com/o/p/_git/r/pullrequest/1")); + assert!(!is_pr_reference("main..feature")); + } +} diff --git a/src/config/cli.rs b/src/config/cli.rs index b34bd815..a6295d93 100644 --- a/src/config/cli.rs +++ b/src/config/cli.rs @@ -108,11 +108,13 @@ pub enum Commands { /// Launch interactive side-by-side diff viewer Diff { /// Commit reference: SHA, HEAD, HEAD~3..HEAD, main..feature, main...feature - /// Can also be a PR number or URL (e.g., 123 or https://github.com/owner/repo/pull/123) + /// Can also be a PR number or URL (GitHub: 123 or + /// https://github.com/owner/repo/pull/123; Azure DevOps: + /// https://dev.azure.com/org/project/_git/repo/pullrequest/123) #[arg(value_parser = clap::value_parser!(CommitReference))] reference: Option, - /// View a GitHub pull request (number or URL) + /// View a pull request (number or URL; GitHub via `gh`, Azure DevOps via its REST API) #[arg(long)] pr: Option, From eca161fdbba5382e4aeb36a6336bab219b386b65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=ABl=20Kuijper?= Date: Fri, 5 Jun 2026 22:51:14 +0200 Subject: [PATCH 02/11] fix(diff): propagate Azure blob-fetch failures; dedupe percent-encoder - blob_text returns Result so a failed fetch can't silently empty a side and flip a file's status to Added/Deleted; the load fails instead. - drop the unreachable HTTP 203 branch in auth_hint. - hoist a single percent_encode helper into git.rs, used by both the Azure REST client and the PR web-URL builder. --- src/command/diff/azure.rs | 82 ++++++++++++++------------------- src/command/diff/git.rs | 17 +++++++ src/command/diff/pr_provider.rs | 23 ++------- 3 files changed, 54 insertions(+), 68 deletions(-) diff --git a/src/command/diff/azure.rs b/src/command/diff/azure.rs index c72322b9..dea254da 100644 --- a/src/command/diff/azure.rs +++ b/src/command/diff/azure.rs @@ -22,7 +22,7 @@ use serde_json::Value; use tokio::sync::Semaphore; use tokio::task::JoinSet; -use super::git::build_file_diff; +use super::git::{build_file_diff, percent_encode}; use super::types::FileDiff; use super::PrInfo; @@ -82,7 +82,7 @@ impl AdoClient { format!( "{}/{}/_apis/git/{}", self.base, - enc(&self.project), + percent_encode(&self.project), suffix ) } @@ -102,34 +102,36 @@ impl AdoClient { serde_json::from_str(&body).map_err(|e| format!("invalid JSON from Azure: {}", e)) } - /// Fetch a blob's text content. Returns empty string when `blob_id` is None - /// (the absent side of an add/delete) or on any error. - async fn blob_text(&self, blob_id: Option<&str>) -> String { + /// Fetch a blob's text content. An absent `blob_id` (the missing side of an + /// add/delete) is `Ok("")`; a failed fetch is an `Err` so it can't silently + /// empty a side and flip the file's status to Added/Deleted downstream. + async fn blob_text(&self, blob_id: Option<&str>) -> Result { let Some(blob_id) = blob_id else { - return String::new(); + return Ok(String::new()); }; let url = format!( "{}?$format=text&api-version={}", - self.git_url(&format!("repositories/{}/blobs/{}", enc(&self.repo), blob_id)), + self.git_url(&format!("repositories/{}/blobs/{}", percent_encode(&self.repo), blob_id)), API_VERSION ); - let Ok(resp) = self + let resp = self .authed(self.http.get(&url)) .header(ACCEPT, "text/plain") .send() .await - else { - return String::new(); - }; - if !resp.status().is_success() { - return String::new(); + .map_err(|e| format!("blob {} request failed: {}", blob_id, e))?; + let status = resp.status(); + if !status.is_success() { + let body = resp.text().await.unwrap_or_default(); + return Err(auth_hint(status, &body)); } // Read as bytes then lossy-decode so binary blobs degrade gracefully // (build_file_diff flags them as binary downstream). - match resp.bytes().await { - Ok(bytes) => String::from_utf8_lossy(&bytes).into_owned(), - Err(_) => String::new(), - } + let bytes = resp + .bytes() + .await + .map_err(|e| format!("blob {} read failed: {}", blob_id, e))?; + Ok(String::from_utf8_lossy(&bytes).into_owned()) } async fn latest_iteration(&self, pr_id: u64) -> Result { @@ -137,7 +139,7 @@ impl AdoClient { "{}?api-version={}", self.git_url(&format!( "repositories/{}/pullRequests/{}/iterations", - enc(&self.repo), + percent_encode(&self.repo), pr_id )), API_VERSION @@ -159,7 +161,7 @@ impl AdoClient { "{}?$compareTo=0&$top={}&$skip={}&api-version={}", self.git_url(&format!( "repositories/{}/pullRequests/{}/iterations/{}/changes", - enc(&self.repo), + percent_encode(&self.repo), pr_id, iteration )), @@ -195,22 +197,23 @@ impl AdoClient { let changes = self.changes(pr_id, iteration).await?; let sem = Arc::new(Semaphore::new(BLOB_CONCURRENCY)); - let mut set: JoinSet<(usize, FileDiff)> = JoinSet::new(); + let mut set: JoinSet> = JoinSet::new(); for (idx, change) in changes.into_iter().enumerate() { let client = self.clone(); let sem = Arc::clone(&sem); set.spawn(async move { - let _permit = sem.acquire_owned().await; - let old = client.blob_text(change.old_blob.as_deref()).await; - let new = client.blob_text(change.new_blob.as_deref()).await; - (idx, build_file_diff(change.path, old, new)) + let _permit = sem.acquire_owned().await.expect("blob semaphore not closed"); + let old = client.blob_text(change.old_blob.as_deref()).await?; + let new = client.blob_text(change.new_blob.as_deref()).await?; + Ok((idx, build_file_diff(change.path, old, new))) }); } // Reassemble in the original change order. let mut out: Vec> = Vec::new(); while let Some(res) = set.join_next().await { - let (idx, diff) = res.map_err(|e| format!("blob fetch task failed: {}", e))?; + // Outer `?`: the task panicked. Inner `?`: a blob fetch failed. + let (idx, diff) = res.map_err(|e| format!("blob fetch task failed: {}", e))??; if idx >= out.len() { out.resize_with(idx + 1, || None); } @@ -315,8 +318,8 @@ pub fn detect_active_pr( block_on(async move { let url = format!( "{}?searchCriteria.status=active&searchCriteria.sourceRefName={}&api-version={}", - client.git_url(&format!("repositories/{}/pullrequests", enc(repo))), - enc(&format!("refs/heads/{}", branch)), + client.git_url(&format!("repositories/{}/pullrequests", percent_encode(repo))), + percent_encode(&format!("refs/heads/{}", branch)), API_VERSION ); let json = client.get_json(&url).await?; @@ -394,7 +397,7 @@ fn resolve_auth() -> Result { } fn auth_hint(status: reqwest::StatusCode, body: &str) -> String { - if status == reqwest::StatusCode::UNAUTHORIZED || status == reqwest::StatusCode::NON_AUTHORITATIVE_INFORMATION { + if status == reqwest::StatusCode::UNAUTHORIZED { return "Azure DevOps auth failed (401). Check ADO_PAT scopes (Code: Read) or run `az login`.".to_string(); } if status == reqwest::StatusCode::FORBIDDEN { @@ -404,23 +407,6 @@ fn auth_hint(status: reqwest::StatusCode, body: &str) -> String { format!("Azure DevOps request failed ({}): {}", status, snippet.trim()) } -/// Percent-encode one URL path/query segment (keeps RFC 3986 unreserved chars). -fn enc(segment: &str) -> String { - use std::fmt::Write; - let mut out = String::with_capacity(segment.len()); - for b in segment.bytes() { - match b { - b'A'..=b'Z' | b'a'..=b'z' | b'0'..=b'9' | b'-' | b'_' | b'.' | b'~' => { - out.push(b as char) - } - _ => { - let _ = write!(out, "%{:02X}", b); - } - } - } - out -} - #[cfg(test)] mod tests { use super::*; @@ -475,8 +461,8 @@ mod tests { #[test] fn encodes_segments() { - assert_eq!(enc("My Project"), "My%20Project"); - assert_eq!(enc("refs/heads/feature/x"), "refs%2Fheads%2Ffeature%2Fx"); - assert_eq!(enc("simple-repo.git"), "simple-repo.git"); + assert_eq!(percent_encode("My Project"), "My%20Project"); + assert_eq!(percent_encode("refs/heads/feature/x"), "refs%2Fheads%2Ffeature%2Fx"); + assert_eq!(percent_encode("simple-repo.git"), "simple-repo.git"); } } diff --git a/src/command/diff/git.rs b/src/command/diff/git.rs index 57d7a968..6a612205 100644 --- a/src/command/diff/git.rs +++ b/src/command/diff/git.rs @@ -161,6 +161,23 @@ pub fn build_file_diff(filename: String, old_content: String, new_content: Strin } } +/// Percent-encode one URL path/query segment, keeping RFC 3986 unreserved chars. +pub(crate) fn percent_encode(segment: &str) -> String { + use std::fmt::Write; + let mut out = String::with_capacity(segment.len()); + for b in segment.bytes() { + match b { + b'A'..=b'Z' | b'a'..=b'z' | b'0'..=b'9' | b'-' | b'_' | b'.' | b'~' => { + out.push(b as char) + } + _ => { + let _ = write!(out, "%{:02X}", b); + } + } + } + out +} + pub fn load_file_diffs(options: &DiffOptions, backend: &dyn VcsBackend) -> Vec { let refs = DiffRefs::from_options(options, backend); get_changed_files(options, backend) diff --git a/src/command/diff/pr_provider.rs b/src/command/diff/pr_provider.rs index 645268dc..2ec5007a 100644 --- a/src/command/diff/pr_provider.rs +++ b/src/command/diff/pr_provider.rs @@ -11,7 +11,7 @@ use std::process::Command; use std::thread; use super::azure; -use super::git::github_load_pr_file_diffs; +use super::git::{github_load_pr_file_diffs, percent_encode}; use super::types::FileDiff; use super::{ github_detect_current_branch_pr, github_fetch_pr_info, github_fetch_viewed_files, @@ -350,7 +350,7 @@ impl PrProvider for AzureProvider { project, pr.repo_name, pr.number, - encode_path(&format!("/{}", filename)) + percent_encode(&format!("/{}", filename)) )) } } @@ -470,23 +470,6 @@ fn decode_component(segment: &str) -> String { segment.replace("%20", " ") } -/// Minimal percent-encoding for an Azure `?path=` query value. -fn encode_path(path: &str) -> String { - use std::fmt::Write; - let mut out = String::with_capacity(path.len()); - for b in path.bytes() { - match b { - b'A'..=b'Z' | b'a'..=b'z' | b'0'..=b'9' | b'-' | b'_' | b'.' | b'~' => { - out.push(b as char) - } - _ => { - let _ = write!(out, "%{:02X}", b); - } - } - } - out -} - #[cfg(test)] mod tests { use super::*; @@ -559,7 +542,7 @@ mod tests { #[test] fn encodes_path_query() { - assert_eq!(encode_path("/src/main.rs"), "%2Fsrc%2Fmain.rs"); + assert_eq!(percent_encode("/src/main.rs"), "%2Fsrc%2Fmain.rs"); } #[test] From 57b8ab5d84ece26cc4303d99fcbdcf4631b64646 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=ABl=20Kuijper?= Date: Fri, 5 Jun 2026 23:16:37 +0200 Subject: [PATCH 03/11] refactor(diff): per-provider modules + registry, drop github_ prefixes Reorganise the PR-provider abstraction so adding a forge is one module plus one registry entry, and provider-specific code stops leaking across files: - pr_provider/ becomes a directory module: github.rs (all the gh-CLI logic, moved out of diff/mod.rs and git.rs and de-prefixed) and azure/ (mod.rs for URL/remote parsing + the PrProvider impl, client.rs for the REST client). - Replace ProviderKind + handler() match + per-method dispatch with a &'static [&dyn PrProvider] registry; selection iterates it. Store the provider as &'static dyn PrProvider directly on PrInfo (trait gains : Sync so it can cross the viewed-sync threads), dropping the enum entirely. - git.rs is now generic VCS only; mod.rs no longer holds github_* functions. Behaviour-preserving; errors are still String and PrInfo still carries the Azure Option fields (addressed in follow-up commits). --- src/command/diff/git.rs | 264 +------- src/command/diff/mod.rs | 288 +------- .../{azure.rs => pr_provider/azure/client.rs} | 6 +- .../azure/mod.rs} | 265 +------- src/command/diff/pr_provider/github.rs | 624 ++++++++++++++++++ src/command/diff/pr_provider/mod.rs | 171 +++++ 6 files changed, 814 insertions(+), 804 deletions(-) rename src/command/diff/{azure.rs => pr_provider/azure/client.rs} (99%) rename src/command/diff/{pr_provider.rs => pr_provider/azure/mod.rs} (53%) create mode 100644 src/command/diff/pr_provider/github.rs create mode 100644 src/command/diff/pr_provider/mod.rs diff --git a/src/command/diff/git.rs b/src/command/diff/git.rs index 6a612205..e3a12c9e 100644 --- a/src/command/diff/git.rs +++ b/src/command/diff/git.rs @@ -1,22 +1,11 @@ use std::fs; use std::path::Path; -use std::process::Command; -use std::sync::mpsc; -use std::sync::{Arc, Mutex}; -use std::thread; - -use spinoff::{spinners, Color, Spinner}; use super::types::{is_binary_content, FileDiff, FileStatus}; -use super::{DiffOptions, PrInfo}; +use super::DiffOptions; use crate::commit_reference::CommitReference; use crate::vcs::VcsBackend; -/// Max concurrent `gh api` requests when fetching PR file contents. -/// GitHub's documented secondary rate limit caps concurrent requests at 100 -/// (shared across REST+GraphQL); 8 keeps us comfortably under that while -/// still giving a large speedup over serial fetching. -const PR_FETCH_CONCURRENCY: usize = 8; pub fn get_current_branch(backend: &dyn VcsBackend) -> String { backend @@ -190,257 +179,6 @@ pub fn load_file_diffs(options: &DiffOptions, backend: &dyn VcsBackend) -> Vec Result, String> { - let repo_arg = format!("{}/{}", pr_info.repo_owner, pr_info.repo_name); - - let mut spinner = Spinner::new( - spinners::Dots, - format!( - "Fetching file list for {}/{}#{}", - pr_info.repo_owner, pr_info.repo_name, pr_info.number - ), - Color::Cyan, - ); - - // Get PR diff to find changed files - let output = Command::new("gh") - .args([ - "pr", - "diff", - &pr_info.number.to_string(), - "--repo", - &repo_arg, - ]) - .output(); - - let output = match output { - Ok(o) => o, - Err(e) => { - let msg = format!("Failed to run gh pr diff: {}", e); - spinner.fail(&msg); - return Err(msg); - } - }; - - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - let msg = format!("gh pr diff failed: {}", stderr.trim()); - spinner.fail(&msg); - return Err(msg); - } - - let diff_output = String::from_utf8_lossy(&output.stdout); - let changed_files = parse_changed_files_from_diff(&diff_output); - let n = changed_files.len(); - - if n == 0 { - spinner.success("PR has no changed files"); - return Ok(Vec::new()); - } - - let base_repo = format!("{}/{}", pr_info.base_repo_owner, pr_info.repo_name); - let head_repo = pr_info - .head_repo_owner - .as_ref() - .map(|owner| format!("{}/{}", owner, pr_info.repo_name)) - .unwrap_or_else(|| base_repo.clone()); - - let contents = fetch_pr_file_contents_parallel( - &changed_files, - &base_repo, - &pr_info.base_ref, - &head_repo, - &pr_info.head_ref, - &mut spinner, - ); - - let file_diffs: Vec = changed_files - .into_iter() - .zip(contents.into_iter()) - .map(|(filename, (old_content, new_content))| { - build_file_diff(filename, old_content, new_content) - }) - .collect(); - - spinner.success(&format!("Fetched {} files", n)); - Ok(file_diffs) -} - -#[derive(Clone, Copy)] -enum Side { - Old, - New, -} - -struct FetchTask { - idx: usize, - filename: String, - repo: String, - git_ref: String, - side: Side, -} - -enum FetchEvent { - Started(String), - Finished { - idx: usize, - side: Side, - filename: String, - content: String, - }, -} - -/// Fetch (old, new) contents for every changed file using a bounded worker -/// pool, updating `spinner` with live progress. -fn fetch_pr_file_contents_parallel( - files: &[String], - base_repo: &str, - base_ref: &str, - head_repo: &str, - head_ref: &str, - spinner: &mut Spinner, -) -> Vec<(String, String)> { - let n = files.len(); - let mut tasks: Vec = Vec::with_capacity(2 * n); - for (idx, filename) in files.iter().enumerate() { - tasks.push(FetchTask { - idx, - filename: filename.clone(), - repo: base_repo.to_string(), - git_ref: base_ref.to_string(), - side: Side::Old, - }); - tasks.push(FetchTask { - idx, - filename: filename.clone(), - repo: head_repo.to_string(), - git_ref: head_ref.to_string(), - side: Side::New, - }); - } - // Pop from the back, so process files in listed order. - tasks.reverse(); - - let total = tasks.len(); - let queue = Arc::new(Mutex::new(tasks)); - let (tx, rx) = mpsc::channel::(); - - let worker_count = PR_FETCH_CONCURRENCY.min(total); - let mut handles = Vec::with_capacity(worker_count); - for _ in 0..worker_count { - let queue = Arc::clone(&queue); - let tx = tx.clone(); - handles.push(thread::spawn(move || loop { - let task = { queue.lock().unwrap().pop() }; - let Some(task) = task else { break }; - let _ = tx.send(FetchEvent::Started(task.filename.clone())); - let content = fetch_file_content_from_github(&task.repo, &task.git_ref, &task.filename); - let _ = tx.send(FetchEvent::Finished { - idx: task.idx, - side: task.side, - filename: task.filename, - content, - }); - })); - } - drop(tx); - - let mut contents: Vec<(String, String)> = vec![(String::new(), String::new()); n]; - let mut done = 0usize; - let mut in_flight: Vec = Vec::new(); - let mut last_finished: Option = None; - - while let Ok(ev) = rx.recv() { - match ev { - FetchEvent::Started(name) => { - in_flight.push(name); - } - FetchEvent::Finished { - idx, - side, - filename, - content, - } => { - if let Some(pos) = in_flight.iter().position(|f| f == &filename) { - in_flight.swap_remove(pos); - } - match side { - Side::Old => contents[idx].0 = content, - Side::New => contents[idx].1 = content, - } - done += 1; - last_finished = Some(filename); - } - } - spinner.update_text(format_fetch_progress(done, total, &in_flight, last_finished.as_deref())); - } - - for h in handles { - let _ = h.join(); - } - - contents -} - -fn format_fetch_progress( - done: usize, - total: usize, - in_flight: &[String], - last_finished: Option<&str>, -) -> String { - let current = if let Some(name) = in_flight.last() { - name.as_str() - } else if let Some(name) = last_finished { - name - } else { - "" - }; - if current.is_empty() { - format!("Fetching files [{}/{}]", done, total) - } else { - format!("Fetching files [{}/{}] · {}", done, total, current) - } -} - -fn fetch_file_content_from_github(repo: &str, git_ref: &str, path: &str) -> String { - let api_path = format!("repos/{}/contents/{}?ref={}", repo, path, git_ref); - let output = Command::new("gh") - .args([ - "api", - &api_path, - "-H", - "Accept: application/vnd.github.raw+json", - ]) - .output(); - - match output { - Ok(o) if o.status.success() => String::from_utf8_lossy(&o.stdout).to_string(), - _ => String::new(), - } -} - -fn parse_changed_files_from_diff(diff: &str) -> Vec { - let mut files = Vec::new(); - - for line in diff.lines() { - if line.starts_with("diff --git") { - let parts: Vec<&str> = line.split_whitespace().collect(); - if parts.len() >= 4 { - let b_path = parts[3]; - if let Some(filename) = b_path.strip_prefix("b/") { - files.push(filename.to_string()); - } else { - files.push(b_path.to_string()); - } - } - } - } - - files -} - /// Load file diffs for a single commit (comparing commit to its parent). /// Uses VcsBackend for backend-agnostic file content retrieval. pub fn load_single_commit_diffs( diff --git a/src/command/diff/mod.rs b/src/command/diff/mod.rs index 0b09c550..77cd94a4 100644 --- a/src/command/diff/mod.rs +++ b/src/command/diff/mod.rs @@ -1,6 +1,5 @@ mod annotation; mod app; -mod azure; mod context; mod coordinates; mod diff_algo; @@ -17,9 +16,8 @@ pub mod theme; mod types; mod watcher; -use std::collections::HashSet; use std::io; -use std::process::{self, Command}; +use std::process; use spinoff::{spinners, Color, Spinner}; @@ -27,7 +25,7 @@ use crate::commit_reference::CommitReference; use crate::vcs::VcsBackend; pub use pr_provider::{ - fetch_viewed_files, mark_file_as_viewed_async, unmark_file_as_viewed_async, ProviderKind, + fetch_viewed_files, mark_file_as_viewed_async, unmark_file_as_viewed_async, PrProvider, }; pub struct DiffOptions { @@ -45,7 +43,7 @@ pub struct DiffOptions { #[derive(Clone)] pub struct PrInfo { - pub provider: ProviderKind, + pub provider: &'static dyn PrProvider, pub number: u64, pub node_id: String, pub repo_owner: String, @@ -60,286 +58,6 @@ pub struct PrInfo { pub org_url: Option, } -fn github_parse_pr_input(input: &str) -> Option<(Option, Option, u64)> { - // Try to parse as a URL first - if input.starts_with("http://") || input.starts_with("https://") { - // Extract PR number and repo info from URL - // Format: https://github.com/owner/repo/pull/123 - let parts: Vec<&str> = input.trim_end_matches('/').split('/').collect(); - if parts.len() >= 2 { - if let Some(pos) = parts.iter().position(|&p| p == "pull") { - if pos + 1 < parts.len() { - if let Ok(num) = parts[pos + 1].parse::() { - // Extract owner and repo - if pos >= 2 { - let owner = parts[pos - 2].to_string(); - let repo = parts[pos - 1].to_string(); - return Some((Some(owner), Some(repo), num)); - } - return Some((None, None, num)); - } - } - } - } - None - } else { - // Try to parse as a PR number - input.parse::().ok().map(|num| (None, None, num)) - } -} - -fn github_resolve_origin_repo() -> Result { - let output = Command::new("git") - .args(["remote", "get-url", "origin"]) - .output() - .map_err(|e| format!("Failed to run git: {}", e))?; - if !output.status.success() { - return Err( - "Could not determine repository. Set origin remote or use --origin owner/repo" - .to_string(), - ); - } - let url = String::from_utf8_lossy(&output.stdout).trim().to_string(); - let url = url.strip_suffix(".git").unwrap_or(&url); - let path = url - .split("github.com") - .nth(1) - .ok_or_else(|| format!("Origin URL is not a GitHub URL: {}", url))?; - let path = path.trim_start_matches(':').trim_start_matches('/'); - let parts: Vec<&str> = path.split('/').collect(); - if parts.len() >= 2 { - Ok(format!("{}/{}", parts[0], parts[1])) - } else { - Err(format!("Could not parse owner/repo from origin URL: {}", url)) - } -} - -pub(crate) fn github_fetch_pr_info( - pr_input: &str, - repo_override: Option<&str>, -) -> Result { - let (owner, repo, number) = github_parse_pr_input(pr_input).ok_or_else(|| { - format!( - "Invalid PR reference: {}. Use a PR number or URL.", - pr_input - ) - })?; - - let repo_full = match (&owner, &repo, repo_override) { - (Some(o), Some(r), _) => format!("{}/{}", o, r), - (_, _, Some(r)) => r.to_string(), - _ => github_resolve_origin_repo()?, - }; - - let (repo_owner, repo_name) = { - let parts: Vec<&str> = repo_full.split('/').collect(); - if parts.len() != 2 { - return Err(format!("Invalid repo format: {}", repo_full)); - } - ( - owner.unwrap_or_else(|| parts[0].to_string()), - repo.unwrap_or_else(|| parts[1].to_string()), - ) - }; - - // Use GraphQL to get the PR node ID, branch refs, and repo owners - let query = format!( - r#"query {{ repository(owner: "{}", name: "{}") {{ pullRequest(number: {}) {{ id url baseRefName headRefName baseRepository {{ owner {{ login }} }} headRepository {{ owner {{ login }} }} }} }} }}"#, - repo_owner, repo_name, number - ); - - let output = Command::new("gh") - .args(["api", "graphql", "-f", &format!("query={}", query)]) - .output() - .map_err(|e| format!("Failed to run gh api graphql: {}", e))?; - - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - return Err(format!("gh api graphql failed: {}", stderr.trim())); - } - - let json_str = String::from_utf8_lossy(&output.stdout); - - // Parse the GraphQL response - let node_id = extract_json_string(&json_str, "id") - .ok_or_else(|| "Could not parse PR node ID from GraphQL response".to_string())?; - let base_ref = - extract_json_string(&json_str, "baseRefName").unwrap_or_else(|| "base".to_string()); - let head_ref = - extract_json_string(&json_str, "headRefName").unwrap_or_else(|| "head".to_string()); - - // Extract repo owners from nested structure - let base_repo_owner = - extract_nested_login(&json_str, "baseRepository").unwrap_or_else(|| repo_owner.clone()); - let head_repo_owner = extract_nested_login(&json_str, "headRepository"); - - Ok(PrInfo { - provider: ProviderKind::GitHub, - number, - node_id, - repo_owner, - repo_name, - base_ref, - head_ref, - base_repo_owner, - head_repo_owner, - project: None, - org_url: None, - }) -} - -fn extract_json_string(json: &str, key: &str) -> Option { - let pattern = format!("\"{}\":\"", key); - if let Some(start) = json.find(&pattern) { - let value_start = start + pattern.len(); - if let Some(end) = json[value_start..].find('"') { - return Some(json[value_start..value_start + end].to_string()); - } - } - None -} - -fn extract_nested_login(json: &str, parent_key: &str) -> Option { - // Look for pattern like "baseRepository":{"owner":{"login":"username"}} - // or handle null case like "headRepository":null - let pattern = format!("\"{}\":", parent_key); - if let Some(start) = json.find(&pattern) { - let after_key = &json[start + pattern.len()..]; - // Check if it's null - if after_key.trim_start().starts_with("null") { - return None; - } - // Look for login within this section - if let Some(login_start) = after_key.find("\"login\":\"") { - let value_start = login_start + 9; - let after_login = &after_key[value_start..]; - if let Some(end) = after_login.find('"') { - return Some(after_login[..end].to_string()); - } - } - } - None -} - -/// Fetch the list of files that are marked as viewed on GitHub -pub(crate) fn github_fetch_viewed_files(pr_info: &PrInfo) -> Result, String> { - let query = format!( - r#"query {{ repository(owner: "{}", name: "{}") {{ pullRequest(number: {}) {{ files(first: 100) {{ nodes {{ path viewerViewedState }} }} }} }} }}"#, - pr_info.repo_owner, pr_info.repo_name, pr_info.number - ); - - let output = Command::new("gh") - .args(["api", "graphql", "-f", &format!("query={}", query)]) - .output() - .map_err(|e| format!("Failed to run gh api graphql: {}", e))?; - - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - return Err(format!("gh api graphql failed: {}", stderr.trim())); - } - - let json_str = String::from_utf8_lossy(&output.stdout); - - // Parse the response to find viewed files - // Look for patterns like: "path":"filename","viewerViewedState":"VIEWED" - let mut viewed_files = HashSet::new(); - - // Simple parsing: find all path/viewerViewedState pairs - let mut remaining = json_str.as_ref(); - while let Some(path_start) = remaining.find("\"path\":\"") { - let path_value_start = path_start + 8; - let after_path = &remaining[path_value_start..]; - if let Some(path_end) = after_path.find('"') { - let path = &after_path[..path_end]; - - // Look for viewerViewedState after this path - let after_path_str = &after_path[path_end..]; - if let Some(state_start) = after_path_str.find("\"viewerViewedState\":\"") { - let state_value_start = state_start + 21; - let after_state = &after_path_str[state_value_start..]; - if let Some(state_end) = after_state.find('"') { - let state = &after_state[..state_end]; - if state == "VIEWED" { - viewed_files.insert(path.to_string()); - } - } - } - - remaining = &remaining[path_value_start + path_end..]; - } else { - break; - } - } - - Ok(viewed_files) -} - -/// Mark a file as viewed on GitHub PR (blocking) -pub(crate) fn github_mark_file_as_viewed_sync( - node_id: &str, - file_path: &str, -) -> Result<(), String> { - let mutation = format!( - r#"mutation {{ markFileAsViewed(input: {{ pullRequestId: "{}", path: "{}" }}) {{ clientMutationId }} }}"#, - node_id, file_path - ); - - let output = Command::new("gh") - .args(["api", "graphql", "-f", &format!("query={}", mutation)]) - .output() - .map_err(|e| format!("Failed to run gh api graphql: {}", e))?; - - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - return Err(stderr.trim().to_string()); - } - - Ok(()) -} - -/// Unmark a file as viewed on GitHub PR (blocking) -pub(crate) fn github_unmark_file_as_viewed_sync( - node_id: &str, - file_path: &str, -) -> Result<(), String> { - let mutation = format!( - r#"mutation {{ unmarkFileAsViewed(input: {{ pullRequestId: "{}", path: "{}" }}) {{ clientMutationId }} }}"#, - node_id, file_path - ); - - let output = Command::new("gh") - .args(["api", "graphql", "-f", &format!("query={}", mutation)]) - .output() - .map_err(|e| format!("Failed to run gh api graphql: {}", e))?; - - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - return Err(stderr.trim().to_string()); - } - - Ok(()) -} - -pub(crate) fn github_detect_current_branch_pr() -> Result { - let output = Command::new("gh") - .args(["pr", "view", "--json", "number", "-q", ".number"]) - .output() - .map_err(|e| format!("Failed to run gh: {}", e))?; - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - let msg = stderr.trim(); - if msg.is_empty() { - return Err("No PR found for the current branch".to_string()); - } - return Err(msg.to_string()); - } - let number = String::from_utf8_lossy(&output.stdout).trim().to_string(); - if number.is_empty() { - return Err("No PR found for the current branch".to_string()); - } - Ok(number) -} - pub fn run_diff_ui(mut options: DiffOptions, backend: &dyn VcsBackend) -> io::Result<()> { // Resolve --detect-pr into options.pr if options.detect_pr && options.pr.is_none() { diff --git a/src/command/diff/azure.rs b/src/command/diff/pr_provider/azure/client.rs similarity index 99% rename from src/command/diff/azure.rs rename to src/command/diff/pr_provider/azure/client.rs index dea254da..98d41e89 100644 --- a/src/command/diff/azure.rs +++ b/src/command/diff/pr_provider/azure/client.rs @@ -22,9 +22,9 @@ use serde_json::Value; use tokio::sync::Semaphore; use tokio::task::JoinSet; -use super::git::{build_file_diff, percent_encode}; -use super::types::FileDiff; -use super::PrInfo; +use crate::command::diff::git::{build_file_diff, percent_encode}; +use crate::command::diff::types::FileDiff; +use crate::command::diff::PrInfo; const API_VERSION: &str = "7.1"; /// Azure DevOps OAuth resource id, used with `az account get-access-token`. diff --git a/src/command/diff/pr_provider.rs b/src/command/diff/pr_provider/azure/mod.rs similarity index 53% rename from src/command/diff/pr_provider.rs rename to src/command/diff/pr_provider/azure/mod.rs index 2ec5007a..ef0badf7 100644 --- a/src/command/diff/pr_provider.rs +++ b/src/command/diff/pr_provider/azure/mod.rs @@ -1,241 +1,15 @@ -//! Pull-request hosting provider abstraction. -//! -//! `lumen diff --pr` originally only understood GitHub (it shelled out to the -//! `gh` CLI everywhere). This module introduces a [`PrProvider`] trait so other -//! forges can be supported, with `gh` for GitHub and `az` (the Azure DevOps CLI) -//! for Azure DevOps. New providers (e.g. `glab` for GitLab) only need to add an -//! impl and wire it into provider detection. - -use std::collections::HashSet; -use std::process::Command; -use std::thread; - -use super::azure; -use super::git::{github_load_pr_file_diffs, percent_encode}; -use super::types::FileDiff; -use super::{ - github_detect_current_branch_pr, github_fetch_pr_info, github_fetch_viewed_files, - github_mark_file_as_viewed_sync, github_unmark_file_as_viewed_sync, PrInfo, -}; - -/// Which hosting provider a [`PrInfo`] belongs to. Stored on `PrInfo` so the -/// runtime (viewed-file sync, open-in-browser, reloads) can route back to the -/// right provider without re-parsing the original input. -#[derive(Clone, Copy, Debug, PartialEq, Eq)] -pub enum ProviderKind { - GitHub, - Azure, -} - -impl ProviderKind { - /// The provider implementation. Both providers are zero-sized, so this hands - /// back a `'static` reference with no allocation. - pub fn handler(self) -> &'static dyn PrProvider { - match self { - ProviderKind::GitHub => &GitHubProvider, - ProviderKind::Azure => &AzureProvider, - } - } -} - -/// A pull-request hosting provider. Each method maps to one capability the diff -/// UI needs; the viewed-file sync methods default to no-ops so providers without -/// that concept (Azure DevOps) don't have to implement them. -pub trait PrProvider { - /// Does this provider recognise `input` as one of its PR URLs? - fn matches_url(&self, input: &str) -> bool; - - /// Does this provider recognise `origin` (a git remote URL) as one of its - /// repositories? Used to pick a provider for bare PR numbers. - fn matches_origin(&self, origin: &str) -> bool; - - /// Resolve a PR number/URL into full metadata. - fn fetch_pr_info(&self, input: &str, repo_override: Option<&str>) -> Result; - - /// Find the PR associated with the current branch. - fn detect_current_branch_pr(&self, repo_override: Option<&str>) -> Result; - - /// Load the file diffs for a PR. - fn load_pr_file_diffs(&self, pr: &PrInfo) -> Result, String>; - - /// Whether this provider supports syncing per-file "viewed" state. - fn supports_viewed_sync(&self) -> bool { - false - } - - /// Fetch the set of paths currently marked as viewed. - fn fetch_viewed_files(&self, _pr: &PrInfo) -> Result, String> { - Ok(HashSet::new()) - } - - /// Mark/unmark a file as viewed (blocking). - fn set_file_viewed(&self, _pr: &PrInfo, _path: &str, _viewed: bool) -> Result<(), String> { - Ok(()) - } - - /// Build a browser URL for `filename` within the PR. - fn file_web_url(&self, pr: &PrInfo, filename: &str) -> Option; -} - -// --------------------------------------------------------------------------- -// Provider selection -// --------------------------------------------------------------------------- - -/// True if `input` looks like a PR reference (a known PR URL or a bare number). -pub fn is_pr_reference(input: &str) -> bool { - GitHubProvider.matches_url(input) - || AzureProvider.matches_url(input) - || input.parse::().is_ok() -} - -fn read_origin_url() -> Option { - let output = Command::new("git") - .args(["remote", "get-url", "origin"]) - .output() - .ok()?; - if !output.status.success() { - return None; - } - let url = String::from_utf8_lossy(&output.stdout).trim().to_string(); - if url.is_empty() { - None - } else { - Some(url) - } -} - -/// Pick a provider from the git `origin` remote (and any `--origin` override), -/// defaulting to GitHub when nothing matches. -fn provider_for_origin(repo_override: Option<&str>) -> &'static dyn PrProvider { - let candidates = [repo_override.map(|s| s.to_string()), read_origin_url()]; - for candidate in candidates.into_iter().flatten() { - if AzureProvider.matches_origin(&candidate) { - return ProviderKind::Azure.handler(); - } - } - ProviderKind::GitHub.handler() -} +//! Azure DevOps provider: URL/remote parsing and the [`PrProvider`] impl. The +//! REST client lives in [`client`]. -/// Pick a provider from a PR URL/number, falling back to origin detection. -fn provider_for_input(input: &str, repo_override: Option<&str>) -> &'static dyn PrProvider { - if AzureProvider.matches_url(input) { - return ProviderKind::Azure.handler(); - } - if GitHubProvider.matches_url(input) { - return ProviderKind::GitHub.handler(); - } - provider_for_origin(repo_override) -} - -// --------------------------------------------------------------------------- -// Dispatchers used by the rest of the diff UI -// --------------------------------------------------------------------------- - -pub fn fetch_pr_info(input: &str, repo_override: Option<&str>) -> Result { - provider_for_input(input, repo_override).fetch_pr_info(input, repo_override) -} - -pub fn detect_current_branch_pr(repo_override: Option<&str>) -> Result { - provider_for_origin(repo_override).detect_current_branch_pr(repo_override) -} - -pub fn load_pr_file_diffs(pr: &PrInfo) -> Result, String> { - pr.provider.handler().load_pr_file_diffs(pr) -} - -pub fn fetch_viewed_files(pr: &PrInfo) -> Result, String> { - pr.provider.handler().fetch_viewed_files(pr) -} - -pub fn pr_file_web_url(pr: &PrInfo, filename: &str) -> Option { - pr.provider.handler().file_web_url(pr, filename) -} +mod client; -pub fn mark_file_as_viewed_async(pr: &PrInfo, file_path: &str) { - set_file_viewed_async(pr, file_path, true); -} - -pub fn unmark_file_as_viewed_async(pr: &PrInfo, file_path: &str) { - set_file_viewed_async(pr, file_path, false); -} - -fn set_file_viewed_async(pr: &PrInfo, file_path: &str, viewed: bool) { - if !pr.provider.handler().supports_viewed_sync() { - return; - } - let pr = pr.clone(); - let path = file_path.to_string(); - thread::spawn(move || { - let _ = pr.provider.handler().set_file_viewed(&pr, &path, viewed); - }); -} - -/// SHA-256 file anchor used by GitHub's PR "Files changed" deep links -/// (`#diff-`). -fn github_file_anchor(filename: &str) -> String { - use sha2::{Digest, Sha256}; - let mut hasher = Sha256::new(); - hasher.update(filename.as_bytes()); - format!("{:x}", hasher.finalize()) -} - -// --------------------------------------------------------------------------- -// GitHub -// --------------------------------------------------------------------------- - -pub struct GitHubProvider; - -impl PrProvider for GitHubProvider { - fn matches_url(&self, input: &str) -> bool { - input.starts_with("http") && input.contains("/pull/") - } - - fn matches_origin(&self, origin: &str) -> bool { - origin.contains("github.com") - } - - fn fetch_pr_info(&self, input: &str, repo_override: Option<&str>) -> Result { - github_fetch_pr_info(input, repo_override) - } - - fn detect_current_branch_pr(&self, _repo_override: Option<&str>) -> Result { - github_detect_current_branch_pr() - } - - fn load_pr_file_diffs(&self, pr: &PrInfo) -> Result, String> { - github_load_pr_file_diffs(pr) - } - - fn supports_viewed_sync(&self) -> bool { - true - } - - fn fetch_viewed_files(&self, pr: &PrInfo) -> Result, String> { - github_fetch_viewed_files(pr) - } - - fn set_file_viewed(&self, pr: &PrInfo, path: &str, viewed: bool) -> Result<(), String> { - if viewed { - github_mark_file_as_viewed_sync(&pr.node_id, path) - } else { - github_unmark_file_as_viewed_sync(&pr.node_id, path) - } - } +use std::process::Command; - fn file_web_url(&self, pr: &PrInfo, filename: &str) -> Option { - Some(format!( - "https://github.com/{}/{}/pull/{}/files#diff-{}", - pr.repo_owner, - pr.repo_name, - pr.number, - github_file_anchor(filename) - )) - } -} +use crate::command::diff::git::percent_encode; +use crate::command::diff::types::FileDiff; +use crate::command::diff::PrInfo; -// --------------------------------------------------------------------------- -// Azure DevOps -// --------------------------------------------------------------------------- +use super::{read_origin_url, PrProvider}; pub struct AzureProvider; @@ -293,10 +67,10 @@ impl PrProvider for AzureProvider { .id .ok_or_else(|| format!("No PR id found in: {}", input))?; - let meta = azure::fetch_pr_metadata(&az.org_url, &az.project, &az.repo, id)?; + let meta = client::fetch_pr_metadata(&az.org_url, &az.project, &az.repo, id)?; Ok(PrInfo { - provider: ProviderKind::Azure, + provider: &AzureProvider, number: id, node_id: String::new(), repo_owner: az.org.clone(), @@ -333,12 +107,12 @@ impl PrProvider for AzureProvider { return Err("Could not determine the current branch".to_string()); } - let id = azure::detect_active_pr(&az.org_url, &az.project, &az.repo, &branch)?; + let id = client::detect_active_pr(&az.org_url, &az.project, &az.repo, &branch)?; Ok(id.to_string()) } fn load_pr_file_diffs(&self, pr: &PrInfo) -> Result, String> { - azure::load_pr_file_diffs(pr) + client::load_pr_file_diffs(pr) } fn file_web_url(&self, pr: &PrInfo, filename: &str) -> Option { @@ -474,13 +248,6 @@ fn decode_component(segment: &str) -> String { mod tests { use super::*; - #[test] - fn github_matches_pull_urls() { - assert!(GitHubProvider.matches_url("https://github.com/owner/repo/pull/123")); - assert!(!GitHubProvider.matches_url("https://dev.azure.com/o/p/_git/r/pullrequest/1")); - assert!(GitHubProvider.matches_origin("git@github.com:owner/repo.git")); - } - #[test] fn azure_matches_pr_urls() { assert!(AzureProvider.matches_url("https://dev.azure.com/o/p/_git/r/pullrequest/42")); @@ -544,12 +311,4 @@ mod tests { fn encodes_path_query() { assert_eq!(percent_encode("/src/main.rs"), "%2Fsrc%2Fmain.rs"); } - - #[test] - fn is_pr_reference_detects_forms() { - assert!(is_pr_reference("123")); - assert!(is_pr_reference("https://github.com/o/r/pull/1")); - assert!(is_pr_reference("https://dev.azure.com/o/p/_git/r/pullrequest/1")); - assert!(!is_pr_reference("main..feature")); - } } diff --git a/src/command/diff/pr_provider/github.rs b/src/command/diff/pr_provider/github.rs new file mode 100644 index 00000000..4477941f --- /dev/null +++ b/src/command/diff/pr_provider/github.rs @@ -0,0 +1,624 @@ +//! GitHub provider: everything is driven through the `gh` CLI (GraphQL for PR +//! metadata and viewed-file state, the contents API for file blobs). + +use std::collections::HashSet; +use std::process::Command; +use std::sync::{mpsc, Arc, Mutex}; +use std::thread; + +use spinoff::{spinners, Color, Spinner}; + +use crate::command::diff::git::build_file_diff; +use crate::command::diff::types::FileDiff; +use crate::command::diff::PrInfo; + +use super::PrProvider; + +/// Max concurrent `gh api` requests when fetching PR file contents. +/// GitHub's documented secondary rate limit caps concurrent requests at 100 +/// (shared across REST+GraphQL); 8 keeps us comfortably under that while +/// still giving a large speedup over serial fetching. +const PR_FETCH_CONCURRENCY: usize = 8; + +pub struct GitHubProvider; + +impl PrProvider for GitHubProvider { + fn matches_url(&self, input: &str) -> bool { + input.starts_with("http") && input.contains("/pull/") + } + + fn matches_origin(&self, origin: &str) -> bool { + origin.contains("github.com") + } + + fn fetch_pr_info(&self, input: &str, repo_override: Option<&str>) -> Result { + fetch_pr_info(input, repo_override) + } + + fn detect_current_branch_pr(&self, _repo_override: Option<&str>) -> Result { + detect_current_branch_pr() + } + + fn load_pr_file_diffs(&self, pr: &PrInfo) -> Result, String> { + load_pr_file_diffs(pr) + } + + fn supports_viewed_sync(&self) -> bool { + true + } + + fn fetch_viewed_files(&self, pr: &PrInfo) -> Result, String> { + fetch_viewed_files(pr) + } + + fn set_file_viewed(&self, pr: &PrInfo, path: &str, viewed: bool) -> Result<(), String> { + if viewed { + mark_file_as_viewed_sync(&pr.node_id, path) + } else { + unmark_file_as_viewed_sync(&pr.node_id, path) + } + } + + fn file_web_url(&self, pr: &PrInfo, filename: &str) -> Option { + Some(format!( + "https://github.com/{}/{}/pull/{}/files#diff-{}", + pr.repo_owner, + pr.repo_name, + pr.number, + file_anchor(filename) + )) + } +} + +// --------------------------------------------------------------------------- +// PR metadata +// --------------------------------------------------------------------------- + +fn parse_pr_input(input: &str) -> Option<(Option, Option, u64)> { + // Try to parse as a URL first + if input.starts_with("http://") || input.starts_with("https://") { + // Extract PR number and repo info from URL + // Format: https://github.com/owner/repo/pull/123 + let parts: Vec<&str> = input.trim_end_matches('/').split('/').collect(); + if parts.len() >= 2 { + if let Some(pos) = parts.iter().position(|&p| p == "pull") { + if pos + 1 < parts.len() { + if let Ok(num) = parts[pos + 1].parse::() { + // Extract owner and repo + if pos >= 2 { + let owner = parts[pos - 2].to_string(); + let repo = parts[pos - 1].to_string(); + return Some((Some(owner), Some(repo), num)); + } + return Some((None, None, num)); + } + } + } + } + None + } else { + // Try to parse as a PR number + input.parse::().ok().map(|num| (None, None, num)) + } +} + +fn resolve_origin_repo() -> Result { + let output = Command::new("git") + .args(["remote", "get-url", "origin"]) + .output() + .map_err(|e| format!("Failed to run git: {}", e))?; + if !output.status.success() { + return Err( + "Could not determine repository. Set origin remote or use --origin owner/repo" + .to_string(), + ); + } + let url = String::from_utf8_lossy(&output.stdout).trim().to_string(); + let url = url.strip_suffix(".git").unwrap_or(&url); + let path = url + .split("github.com") + .nth(1) + .ok_or_else(|| format!("Origin URL is not a GitHub URL: {}", url))?; + let path = path.trim_start_matches(':').trim_start_matches('/'); + let parts: Vec<&str> = path.split('/').collect(); + if parts.len() >= 2 { + Ok(format!("{}/{}", parts[0], parts[1])) + } else { + Err(format!("Could not parse owner/repo from origin URL: {}", url)) + } +} + +fn fetch_pr_info(pr_input: &str, repo_override: Option<&str>) -> Result { + let (owner, repo, number) = parse_pr_input(pr_input).ok_or_else(|| { + format!( + "Invalid PR reference: {}. Use a PR number or URL.", + pr_input + ) + })?; + + let repo_full = match (&owner, &repo, repo_override) { + (Some(o), Some(r), _) => format!("{}/{}", o, r), + (_, _, Some(r)) => r.to_string(), + _ => resolve_origin_repo()?, + }; + + let (repo_owner, repo_name) = { + let parts: Vec<&str> = repo_full.split('/').collect(); + if parts.len() != 2 { + return Err(format!("Invalid repo format: {}", repo_full)); + } + ( + owner.unwrap_or_else(|| parts[0].to_string()), + repo.unwrap_or_else(|| parts[1].to_string()), + ) + }; + + // Use GraphQL to get the PR node ID, branch refs, and repo owners + let query = format!( + r#"query {{ repository(owner: "{}", name: "{}") {{ pullRequest(number: {}) {{ id url baseRefName headRefName baseRepository {{ owner {{ login }} }} headRepository {{ owner {{ login }} }} }} }} }}"#, + repo_owner, repo_name, number + ); + + let output = Command::new("gh") + .args(["api", "graphql", "-f", &format!("query={}", query)]) + .output() + .map_err(|e| format!("Failed to run gh api graphql: {}", e))?; + + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + return Err(format!("gh api graphql failed: {}", stderr.trim())); + } + + let json_str = String::from_utf8_lossy(&output.stdout); + + // Parse the GraphQL response + let node_id = extract_json_string(&json_str, "id") + .ok_or_else(|| "Could not parse PR node ID from GraphQL response".to_string())?; + let base_ref = + extract_json_string(&json_str, "baseRefName").unwrap_or_else(|| "base".to_string()); + let head_ref = + extract_json_string(&json_str, "headRefName").unwrap_or_else(|| "head".to_string()); + + // Extract repo owners from nested structure + let base_repo_owner = + extract_nested_login(&json_str, "baseRepository").unwrap_or_else(|| repo_owner.clone()); + let head_repo_owner = extract_nested_login(&json_str, "headRepository"); + + Ok(PrInfo { + provider: &GitHubProvider, + number, + node_id, + repo_owner, + repo_name, + base_ref, + head_ref, + base_repo_owner, + head_repo_owner, + project: None, + org_url: None, + }) +} + +fn extract_json_string(json: &str, key: &str) -> Option { + let pattern = format!("\"{}\":\"", key); + if let Some(start) = json.find(&pattern) { + let value_start = start + pattern.len(); + if let Some(end) = json[value_start..].find('"') { + return Some(json[value_start..value_start + end].to_string()); + } + } + None +} + +fn extract_nested_login(json: &str, parent_key: &str) -> Option { + // Look for pattern like "baseRepository":{"owner":{"login":"username"}} + // or handle null case like "headRepository":null + let pattern = format!("\"{}\":", parent_key); + if let Some(start) = json.find(&pattern) { + let after_key = &json[start + pattern.len()..]; + // Check if it's null + if after_key.trim_start().starts_with("null") { + return None; + } + // Look for login within this section + if let Some(login_start) = after_key.find("\"login\":\"") { + let value_start = login_start + 9; + let after_login = &after_key[value_start..]; + if let Some(end) = after_login.find('"') { + return Some(after_login[..end].to_string()); + } + } + } + None +} + +fn detect_current_branch_pr() -> Result { + let output = Command::new("gh") + .args(["pr", "view", "--json", "number", "-q", ".number"]) + .output() + .map_err(|e| format!("Failed to run gh: {}", e))?; + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + let msg = stderr.trim(); + if msg.is_empty() { + return Err("No PR found for the current branch".to_string()); + } + return Err(msg.to_string()); + } + let number = String::from_utf8_lossy(&output.stdout).trim().to_string(); + if number.is_empty() { + return Err("No PR found for the current branch".to_string()); + } + Ok(number) +} + +/// SHA-256 file anchor used by GitHub's PR "Files changed" deep links +/// (`#diff-`). +fn file_anchor(filename: &str) -> String { + use sha2::{Digest, Sha256}; + let mut hasher = Sha256::new(); + hasher.update(filename.as_bytes()); + format!("{:x}", hasher.finalize()) +} + +// --------------------------------------------------------------------------- +// Viewed-file state +// --------------------------------------------------------------------------- + +/// Fetch the list of files that are marked as viewed on GitHub +fn fetch_viewed_files(pr_info: &PrInfo) -> Result, String> { + let query = format!( + r#"query {{ repository(owner: "{}", name: "{}") {{ pullRequest(number: {}) {{ files(first: 100) {{ nodes {{ path viewerViewedState }} }} }} }} }}"#, + pr_info.repo_owner, pr_info.repo_name, pr_info.number + ); + + let output = Command::new("gh") + .args(["api", "graphql", "-f", &format!("query={}", query)]) + .output() + .map_err(|e| format!("Failed to run gh api graphql: {}", e))?; + + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + return Err(format!("gh api graphql failed: {}", stderr.trim())); + } + + let json_str = String::from_utf8_lossy(&output.stdout); + + // Parse the response to find viewed files + // Look for patterns like: "path":"filename","viewerViewedState":"VIEWED" + let mut viewed_files = HashSet::new(); + + // Simple parsing: find all path/viewerViewedState pairs + let mut remaining = json_str.as_ref(); + while let Some(path_start) = remaining.find("\"path\":\"") { + let path_value_start = path_start + 8; + let after_path = &remaining[path_value_start..]; + if let Some(path_end) = after_path.find('"') { + let path = &after_path[..path_end]; + + // Look for viewerViewedState after this path + let after_path_str = &after_path[path_end..]; + if let Some(state_start) = after_path_str.find("\"viewerViewedState\":\"") { + let state_value_start = state_start + 21; + let after_state = &after_path_str[state_value_start..]; + if let Some(state_end) = after_state.find('"') { + let state = &after_state[..state_end]; + if state == "VIEWED" { + viewed_files.insert(path.to_string()); + } + } + } + + remaining = &remaining[path_value_start + path_end..]; + } else { + break; + } + } + + Ok(viewed_files) +} + +/// Mark a file as viewed on GitHub PR (blocking) +fn mark_file_as_viewed_sync(node_id: &str, file_path: &str) -> Result<(), String> { + let mutation = format!( + r#"mutation {{ markFileAsViewed(input: {{ pullRequestId: "{}", path: "{}" }}) {{ clientMutationId }} }}"#, + node_id, file_path + ); + + let output = Command::new("gh") + .args(["api", "graphql", "-f", &format!("query={}", mutation)]) + .output() + .map_err(|e| format!("Failed to run gh api graphql: {}", e))?; + + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + return Err(stderr.trim().to_string()); + } + + Ok(()) +} + +/// Unmark a file as viewed on GitHub PR (blocking) +fn unmark_file_as_viewed_sync(node_id: &str, file_path: &str) -> Result<(), String> { + let mutation = format!( + r#"mutation {{ unmarkFileAsViewed(input: {{ pullRequestId: "{}", path: "{}" }}) {{ clientMutationId }} }}"#, + node_id, file_path + ); + + let output = Command::new("gh") + .args(["api", "graphql", "-f", &format!("query={}", mutation)]) + .output() + .map_err(|e| format!("Failed to run gh api graphql: {}", e))?; + + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + return Err(stderr.trim().to_string()); + } + + Ok(()) +} + +// --------------------------------------------------------------------------- +// File diffs (gh pr diff + parallel contents fetch) +// --------------------------------------------------------------------------- + +fn load_pr_file_diffs(pr_info: &PrInfo) -> Result, String> { + let repo_arg = format!("{}/{}", pr_info.repo_owner, pr_info.repo_name); + + let mut spinner = Spinner::new( + spinners::Dots, + format!( + "Fetching file list for {}/{}#{}", + pr_info.repo_owner, pr_info.repo_name, pr_info.number + ), + Color::Cyan, + ); + + // Get PR diff to find changed files + let output = Command::new("gh") + .args([ + "pr", + "diff", + &pr_info.number.to_string(), + "--repo", + &repo_arg, + ]) + .output(); + + let output = match output { + Ok(o) => o, + Err(e) => { + let msg = format!("Failed to run gh pr diff: {}", e); + spinner.fail(&msg); + return Err(msg); + } + }; + + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + let msg = format!("gh pr diff failed: {}", stderr.trim()); + spinner.fail(&msg); + return Err(msg); + } + + let diff_output = String::from_utf8_lossy(&output.stdout); + let changed_files = parse_changed_files_from_diff(&diff_output); + let n = changed_files.len(); + + if n == 0 { + spinner.success("PR has no changed files"); + return Ok(Vec::new()); + } + + let base_repo = format!("{}/{}", pr_info.base_repo_owner, pr_info.repo_name); + let head_repo = pr_info + .head_repo_owner + .as_ref() + .map(|owner| format!("{}/{}", owner, pr_info.repo_name)) + .unwrap_or_else(|| base_repo.clone()); + + let contents = fetch_pr_file_contents_parallel( + &changed_files, + &base_repo, + &pr_info.base_ref, + &head_repo, + &pr_info.head_ref, + &mut spinner, + ); + + let file_diffs: Vec = changed_files + .into_iter() + .zip(contents.into_iter()) + .map(|(filename, (old_content, new_content))| { + build_file_diff(filename, old_content, new_content) + }) + .collect(); + + spinner.success(&format!("Fetched {} files", n)); + Ok(file_diffs) +} + +#[derive(Clone, Copy)] +enum Side { + Old, + New, +} + +struct FetchTask { + idx: usize, + filename: String, + repo: String, + git_ref: String, + side: Side, +} + +enum FetchEvent { + Started(String), + Finished { + idx: usize, + side: Side, + filename: String, + content: String, + }, +} + +/// Fetch (old, new) contents for every changed file using a bounded worker +/// pool, updating `spinner` with live progress. +fn fetch_pr_file_contents_parallel( + files: &[String], + base_repo: &str, + base_ref: &str, + head_repo: &str, + head_ref: &str, + spinner: &mut Spinner, +) -> Vec<(String, String)> { + let n = files.len(); + let mut tasks: Vec = Vec::with_capacity(2 * n); + for (idx, filename) in files.iter().enumerate() { + tasks.push(FetchTask { + idx, + filename: filename.clone(), + repo: base_repo.to_string(), + git_ref: base_ref.to_string(), + side: Side::Old, + }); + tasks.push(FetchTask { + idx, + filename: filename.clone(), + repo: head_repo.to_string(), + git_ref: head_ref.to_string(), + side: Side::New, + }); + } + // Pop from the back, so process files in listed order. + tasks.reverse(); + + let total = tasks.len(); + let queue = Arc::new(Mutex::new(tasks)); + let (tx, rx) = mpsc::channel::(); + + let worker_count = PR_FETCH_CONCURRENCY.min(total); + let mut handles = Vec::with_capacity(worker_count); + for _ in 0..worker_count { + let queue = Arc::clone(&queue); + let tx = tx.clone(); + handles.push(thread::spawn(move || loop { + let task = { queue.lock().unwrap().pop() }; + let Some(task) = task else { break }; + let _ = tx.send(FetchEvent::Started(task.filename.clone())); + let content = fetch_file_content_from_github(&task.repo, &task.git_ref, &task.filename); + let _ = tx.send(FetchEvent::Finished { + idx: task.idx, + side: task.side, + filename: task.filename, + content, + }); + })); + } + drop(tx); + + let mut contents: Vec<(String, String)> = vec![(String::new(), String::new()); n]; + let mut done = 0usize; + let mut in_flight: Vec = Vec::new(); + let mut last_finished: Option = None; + + while let Ok(ev) = rx.recv() { + match ev { + FetchEvent::Started(name) => { + in_flight.push(name); + } + FetchEvent::Finished { + idx, + side, + filename, + content, + } => { + if let Some(pos) = in_flight.iter().position(|f| f == &filename) { + in_flight.swap_remove(pos); + } + match side { + Side::Old => contents[idx].0 = content, + Side::New => contents[idx].1 = content, + } + done += 1; + last_finished = Some(filename); + } + } + spinner.update_text(format_fetch_progress(done, total, &in_flight, last_finished.as_deref())); + } + + for h in handles { + let _ = h.join(); + } + + contents +} + +fn format_fetch_progress( + done: usize, + total: usize, + in_flight: &[String], + last_finished: Option<&str>, +) -> String { + let current = if let Some(name) = in_flight.last() { + name.as_str() + } else if let Some(name) = last_finished { + name + } else { + "" + }; + if current.is_empty() { + format!("Fetching files [{}/{}]", done, total) + } else { + format!("Fetching files [{}/{}] · {}", done, total, current) + } +} + +fn fetch_file_content_from_github(repo: &str, git_ref: &str, path: &str) -> String { + let api_path = format!("repos/{}/contents/{}?ref={}", repo, path, git_ref); + let output = Command::new("gh") + .args([ + "api", + &api_path, + "-H", + "Accept: application/vnd.github.raw+json", + ]) + .output(); + + match output { + Ok(o) if o.status.success() => String::from_utf8_lossy(&o.stdout).to_string(), + _ => String::new(), + } +} + +fn parse_changed_files_from_diff(diff: &str) -> Vec { + let mut files = Vec::new(); + + for line in diff.lines() { + if line.starts_with("diff --git") { + let parts: Vec<&str> = line.split_whitespace().collect(); + if parts.len() >= 4 { + let b_path = parts[3]; + if let Some(filename) = b_path.strip_prefix("b/") { + files.push(filename.to_string()); + } else { + files.push(b_path.to_string()); + } + } + } + } + + files +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn github_matches_pull_urls() { + assert!(GitHubProvider.matches_url("https://github.com/owner/repo/pull/123")); + assert!(!GitHubProvider.matches_url("https://dev.azure.com/o/p/_git/r/pullrequest/1")); + assert!(GitHubProvider.matches_origin("git@github.com:owner/repo.git")); + } +} diff --git a/src/command/diff/pr_provider/mod.rs b/src/command/diff/pr_provider/mod.rs new file mode 100644 index 00000000..d9622667 --- /dev/null +++ b/src/command/diff/pr_provider/mod.rs @@ -0,0 +1,171 @@ +//! Pull-request hosting provider abstraction. +//! +//! `lumen diff --pr` originally only understood GitHub (it shelled out to the +//! `gh` CLI everywhere). This module introduces a [`PrProvider`] trait so other +//! forges can be supported, with [`github`] for GitHub and [`azure`] for Azure +//! DevOps. Adding a forge (e.g. `glab` for GitLab) is one new module plus one +//! entry in [`PROVIDERS`]. + +mod azure; +mod github; + +use std::collections::HashSet; +use std::process::Command; +use std::thread; + +use super::types::FileDiff; +use super::PrInfo; + +use azure::AzureProvider; +use github::GitHubProvider; + +/// A pull-request hosting provider. Each method maps to one capability the diff +/// UI needs; the viewed-file sync methods default to no-ops so providers without +/// that concept (Azure DevOps) don't have to implement them. +/// +/// `Sync` is required so a `&'static dyn PrProvider` (stored on [`PrInfo`]) can +/// be moved into the background threads that sync viewed-file state. +pub trait PrProvider: Sync { + /// Does this provider recognise `input` as one of its PR URLs? + fn matches_url(&self, input: &str) -> bool; + + /// Does this provider recognise `origin` (a git remote URL) as one of its + /// repositories? Used to pick a provider for bare PR numbers. + fn matches_origin(&self, origin: &str) -> bool; + + /// Resolve a PR number/URL into full metadata. + fn fetch_pr_info(&self, input: &str, repo_override: Option<&str>) -> Result; + + /// Find the PR associated with the current branch. + fn detect_current_branch_pr(&self, repo_override: Option<&str>) -> Result; + + /// Load the file diffs for a PR. + fn load_pr_file_diffs(&self, pr: &PrInfo) -> Result, String>; + + /// Whether this provider supports syncing per-file "viewed" state. + fn supports_viewed_sync(&self) -> bool { + false + } + + /// Fetch the set of paths currently marked as viewed. + fn fetch_viewed_files(&self, _pr: &PrInfo) -> Result, String> { + Ok(HashSet::new()) + } + + /// Mark/unmark a file as viewed (blocking). + fn set_file_viewed(&self, _pr: &PrInfo, _path: &str, _viewed: bool) -> Result<(), String> { + Ok(()) + } + + /// Build a browser URL for `filename` within the PR. + fn file_web_url(&self, pr: &PrInfo, filename: &str) -> Option; +} + +// --------------------------------------------------------------------------- +// Provider registry & selection +// --------------------------------------------------------------------------- + +/// All compiled-in providers. Detection iterates this; adding a forge is one +/// new module plus one entry here. Both providers are zero-sized, so the +/// `&'static` references cost nothing. +static PROVIDERS: &[&dyn PrProvider] = &[&GitHubProvider, &AzureProvider]; + +/// Used when no provider matches a bare PR number's remote. +const DEFAULT_PROVIDER: &dyn PrProvider = &GitHubProvider; + +/// True if `input` looks like a PR reference (a known PR URL or a bare number). +pub fn is_pr_reference(input: &str) -> bool { + PROVIDERS.iter().any(|p| p.matches_url(input)) || input.parse::().is_ok() +} + +fn read_origin_url() -> Option { + let output = Command::new("git") + .args(["remote", "get-url", "origin"]) + .output() + .ok()?; + if !output.status.success() { + return None; + } + let url = String::from_utf8_lossy(&output.stdout).trim().to_string(); + if url.is_empty() { + None + } else { + Some(url) + } +} + +/// Pick a provider from the git `origin` remote (and any `--origin` override), +/// defaulting to GitHub when nothing matches. +fn provider_for_origin(repo_override: Option<&str>) -> &'static dyn PrProvider { + let candidates = [repo_override.map(|s| s.to_string()), read_origin_url()]; + for candidate in candidates.into_iter().flatten() { + if let Some(p) = PROVIDERS.iter().copied().find(|p| p.matches_origin(&candidate)) { + return p; + } + } + DEFAULT_PROVIDER +} + +/// Pick a provider from a PR URL/number, falling back to origin detection. +fn provider_for_input(input: &str, repo_override: Option<&str>) -> &'static dyn PrProvider { + if let Some(p) = PROVIDERS.iter().copied().find(|p| p.matches_url(input)) { + return p; + } + provider_for_origin(repo_override) +} + +// --------------------------------------------------------------------------- +// Dispatchers used by the rest of the diff UI +// --------------------------------------------------------------------------- + +pub fn fetch_pr_info(input: &str, repo_override: Option<&str>) -> Result { + provider_for_input(input, repo_override).fetch_pr_info(input, repo_override) +} + +pub fn detect_current_branch_pr(repo_override: Option<&str>) -> Result { + provider_for_origin(repo_override).detect_current_branch_pr(repo_override) +} + +pub fn load_pr_file_diffs(pr: &PrInfo) -> Result, String> { + pr.provider.load_pr_file_diffs(pr) +} + +pub fn fetch_viewed_files(pr: &PrInfo) -> Result, String> { + pr.provider.fetch_viewed_files(pr) +} + +pub fn pr_file_web_url(pr: &PrInfo, filename: &str) -> Option { + pr.provider.file_web_url(pr, filename) +} + +pub fn mark_file_as_viewed_async(pr: &PrInfo, file_path: &str) { + set_file_viewed_async(pr, file_path, true); +} + +pub fn unmark_file_as_viewed_async(pr: &PrInfo, file_path: &str) { + set_file_viewed_async(pr, file_path, false); +} + +fn set_file_viewed_async(pr: &PrInfo, file_path: &str, viewed: bool) { + if !pr.provider.supports_viewed_sync() { + return; + } + let pr = pr.clone(); + let path = file_path.to_string(); + thread::spawn(move || { + let _ = pr.provider.set_file_viewed(&pr, &path, viewed); + }); +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn is_pr_reference_detects_forms() { + assert!(is_pr_reference("123")); + assert!(is_pr_reference("https://github.com/o/r/pull/1")); + assert!(is_pr_reference("https://dev.azure.com/o/p/_git/r/pullrequest/1")); + assert!(!is_pr_reference("main..feature")); + } +} From 26a2465cc4c18de8ab09e3d37c1004ca5dc28ec9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=ABl=20Kuijper?= Date: Fri, 5 Jun 2026 23:24:33 +0200 Subject: [PATCH 04/11] refactor(diff): typed PrError instead of Result<_, String> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The PrProvider trait and its impls now return a thiserror PrError whose variant is the kind — Auth / NotFound / InvalidRef / Other — so the UI can react to the failure (e.g. prompt for credentials on Auth) rather than only display a string. Classification happens where it's reliable: - Azure REST: HTTP 401/403 -> Auth, 404 -> NotFound; missing/!ok az creds -> Auth. - Parse failures -> InvalidRef; "no PR for branch" -> NotFound. A From/From<&str> impl lets the remaining opaque CLI/transport errors fall through to Other via `?`, so internal helpers stay terse. --- src/command/diff/mod.rs | 6 +- src/command/diff/pr_provider/azure/client.rs | 68 ++++++++++++-------- src/command/diff/pr_provider/azure/mod.rs | 20 +++--- src/command/diff/pr_provider/github.rs | 48 +++++++------- src/command/diff/pr_provider/mod.rs | 50 +++++++++++--- 5 files changed, 119 insertions(+), 73 deletions(-) diff --git a/src/command/diff/mod.rs b/src/command/diff/mod.rs index 77cd94a4..d37b77e7 100644 --- a/src/command/diff/mod.rs +++ b/src/command/diff/mod.rs @@ -72,7 +72,7 @@ pub fn run_diff_ui(mut options: DiffOptions, backend: &dyn VcsBackend) -> io::Re options.pr = Some(number); } Err(e) => { - spinner.fail(&e); + spinner.fail(&e.to_string()); process::exit(1); } } @@ -87,7 +87,7 @@ pub fn run_diff_ui(mut options: DiffOptions, backend: &dyn VcsBackend) -> io::Re return app::run_app_with_pr(options, pr_info, backend); } Err(e) => { - spinner.fail(&e); + spinner.fail(&e.to_string()); process::exit(1); } } @@ -103,7 +103,7 @@ pub fn run_diff_ui(mut options: DiffOptions, backend: &dyn VcsBackend) -> io::Re return app::run_app_with_pr(options, pr_info, backend); } Err(e) => { - spinner.fail(&e); + spinner.fail(&e.to_string()); process::exit(1); } } diff --git a/src/command/diff/pr_provider/azure/client.rs b/src/command/diff/pr_provider/azure/client.rs index 98d41e89..9e51648c 100644 --- a/src/command/diff/pr_provider/azure/client.rs +++ b/src/command/diff/pr_provider/azure/client.rs @@ -24,6 +24,7 @@ use tokio::task::JoinSet; use crate::command::diff::git::{build_file_diff, percent_encode}; use crate::command::diff::types::FileDiff; +use crate::command::diff::pr_provider::PrError; use crate::command::diff::PrInfo; const API_VERSION: &str = "7.1"; @@ -60,7 +61,7 @@ struct AdoClient { } impl AdoClient { - fn new(org_url: &str, project: &str, repo: &str) -> Result { + fn new(org_url: &str, project: &str, repo: &str) -> Result { Ok(Self { http: reqwest::Client::new(), base: org_url.trim_end_matches('/').to_string(), @@ -87,7 +88,7 @@ impl AdoClient { ) } - async fn get_json(&self, url: &str) -> Result { + async fn get_json(&self, url: &str) -> Result { let resp = self .authed(self.http.get(url)) .header(ACCEPT, "application/json") @@ -99,13 +100,13 @@ impl AdoClient { if !status.is_success() { return Err(auth_hint(status, &body)); } - serde_json::from_str(&body).map_err(|e| format!("invalid JSON from Azure: {}", e)) + serde_json::from_str(&body).map_err(|e| PrError::Other(format!("invalid JSON from Azure: {}", e))) } /// Fetch a blob's text content. An absent `blob_id` (the missing side of an /// add/delete) is `Ok("")`; a failed fetch is an `Err` so it can't silently /// empty a side and flip the file's status to Added/Deleted downstream. - async fn blob_text(&self, blob_id: Option<&str>) -> Result { + async fn blob_text(&self, blob_id: Option<&str>) -> Result { let Some(blob_id) = blob_id else { return Ok(String::new()); }; @@ -134,7 +135,7 @@ impl AdoClient { Ok(String::from_utf8_lossy(&bytes).into_owned()) } - async fn latest_iteration(&self, pr_id: u64) -> Result { + async fn latest_iteration(&self, pr_id: u64) -> Result { let url = format!( "{}?api-version={}", self.git_url(&format!( @@ -148,12 +149,12 @@ impl AdoClient { json.get("value") .and_then(|v| v.as_array()) .and_then(|arr| arr.iter().filter_map(|i| i.get("id")?.as_u64()).max()) - .ok_or_else(|| "PR has no iterations".to_string()) + .ok_or_else(|| PrError::Other("PR has no iterations".to_string())) } /// All change entries for `iteration`, compared against the merge base /// (`$compareTo=0`), following `$skip`/`$top` pagination. - async fn changes(&self, pr_id: u64, iteration: u64) -> Result, String> { + async fn changes(&self, pr_id: u64, iteration: u64) -> Result, PrError> { let mut entries = Vec::new(); let mut skip = 0usize; loop { @@ -192,12 +193,12 @@ impl AdoClient { Ok(entries) } - async fn load_file_diffs(&self, pr_id: u64) -> Result, String> { + async fn load_file_diffs(&self, pr_id: u64) -> Result, PrError> { let iteration = self.latest_iteration(pr_id).await?; let changes = self.changes(pr_id, iteration).await?; let sem = Arc::new(Semaphore::new(BLOB_CONCURRENCY)); - let mut set: JoinSet> = JoinSet::new(); + let mut set: JoinSet> = JoinSet::new(); for (idx, change) in changes.into_iter().enumerate() { let client = self.clone(); let sem = Arc::clone(&sem); @@ -275,7 +276,7 @@ pub fn fetch_pr_metadata( project: &str, repo: &str, pr_id: u64, -) -> Result { +) -> Result { let client = AdoClient::new(org_url, project, repo)?; block_on(async move { // PR detail is project-scoped (not repo-scoped) in the REST API. @@ -313,7 +314,7 @@ pub fn detect_active_pr( project: &str, repo: &str, branch: &str, -) -> Result { +) -> Result { let client = AdoClient::new(org_url, project, repo)?; block_on(async move { let url = format!( @@ -328,11 +329,11 @@ pub fn detect_active_pr( .and_then(|arr| arr.first()) .and_then(|pr| pr.get("pullRequestId")) .and_then(|v| v.as_u64()) - .ok_or_else(|| format!("No active PR found for branch {}", branch)) + .ok_or_else(|| PrError::NotFound(format!("No active PR found for branch {}", branch))) }) } -pub fn load_pr_file_diffs(pr: &PrInfo) -> Result, String> { +pub fn load_pr_file_diffs(pr: &PrInfo) -> Result, PrError> { let org_url = pr .org_url .as_deref() @@ -357,7 +358,7 @@ fn block_on(fut: F) -> F::Output { tokio::task::block_in_place(|| tokio::runtime::Handle::current().block_on(fut)) } -fn resolve_auth() -> Result { +fn resolve_auth() -> Result { for var in ["ADO_PAT", "AZURE_DEVOPS_EXT_PAT", "AZURE_DEVOPS_PAT"] { if let Ok(pat) = env::var(var) { if !pat.trim().is_empty() { @@ -377,34 +378,47 @@ fn resolve_auth() -> Result { ]) .output() .map_err(|e| { - format!( + PrError::Auth(format!( "No Azure DevOps credentials: set ADO_PAT or install the Azure CLI and run `az login` ({})", e - ) + )) })?; if !output.status.success() { - return Err( + return Err(PrError::Auth( "No Azure DevOps credentials: set ADO_PAT, or run `az login` to use the Azure CLI." .to_string(), - ); + )); } let json: Value = serde_json::from_slice(&output.stdout) .map_err(|e| format!("Could not parse az token output: {}", e))?; json.get("accessToken") .and_then(|v| v.as_str()) .map(|t| AdoAuth::Bearer(t.to_string())) - .ok_or_else(|| "az returned no access token".to_string()) + .ok_or_else(|| PrError::Auth("az returned no access token".to_string())) } -fn auth_hint(status: reqwest::StatusCode, body: &str) -> String { - if status == reqwest::StatusCode::UNAUTHORIZED { - return "Azure DevOps auth failed (401). Check ADO_PAT scopes (Code: Read) or run `az login`.".to_string(); - } - if status == reqwest::StatusCode::FORBIDDEN { - return "Azure DevOps returned 403. The token lacks access to this repository.".to_string(); +fn auth_hint(status: reqwest::StatusCode, body: &str) -> PrError { + use reqwest::StatusCode; + match status { + StatusCode::UNAUTHORIZED => PrError::Auth( + "Azure DevOps auth failed (401). Check ADO_PAT scopes (Code: Read) or run `az login`." + .to_string(), + ), + StatusCode::FORBIDDEN => PrError::Auth( + "Azure DevOps returned 403. The token lacks access to this repository.".to_string(), + ), + StatusCode::NOT_FOUND => PrError::NotFound( + "Azure DevOps returned 404 (PR or repository not found).".to_string(), + ), + _ => { + let snippet: String = body.chars().take(200).collect(); + PrError::Other(format!( + "Azure DevOps request failed ({}): {}", + status, + snippet.trim() + )) + } } - let snippet: String = body.chars().take(200).collect(); - format!("Azure DevOps request failed ({}): {}", status, snippet.trim()) } #[cfg(test)] diff --git a/src/command/diff/pr_provider/azure/mod.rs b/src/command/diff/pr_provider/azure/mod.rs index ef0badf7..18115186 100644 --- a/src/command/diff/pr_provider/azure/mod.rs +++ b/src/command/diff/pr_provider/azure/mod.rs @@ -9,7 +9,7 @@ use crate::command::diff::git::percent_encode; use crate::command::diff::types::FileDiff; use crate::command::diff::PrInfo; -use super::{read_origin_url, PrProvider}; +use super::{read_origin_url, PrError, PrProvider}; pub struct AzureProvider; @@ -27,7 +27,7 @@ struct AzureRef { } impl AzureProvider { - fn resolve_ref(&self, input: &str, repo_override: Option<&str>) -> Result { + fn resolve_ref(&self, input: &str, repo_override: Option<&str>) -> Result { if let Some(parsed) = parse_azure_url(input) { return Ok(parsed); } @@ -35,7 +35,7 @@ impl AzureProvider { // URL) or from the git `origin` remote. let id = input .parse::() - .map_err(|_| format!("Invalid Azure DevOps PR reference: {}", input))?; + .map_err(|_| PrError::InvalidRef(format!("Invalid Azure DevOps PR reference: {}", input)))?; let remote = repo_override .filter(|o| self.matches_origin(o)) .map(|s| s.to_string()) @@ -45,7 +45,7 @@ impl AzureProvider { .to_string() })?; let mut parsed = parse_azure_remote(&remote) - .ok_or_else(|| format!("Could not parse Azure DevOps remote: {}", remote))?; + .ok_or_else(|| PrError::InvalidRef(format!("Could not parse Azure DevOps remote: {}", remote)))?; parsed.id = Some(id); Ok(parsed) } @@ -61,11 +61,11 @@ impl PrProvider for AzureProvider { origin.contains("dev.azure.com") || origin.contains(".visualstudio.com") } - fn fetch_pr_info(&self, input: &str, repo_override: Option<&str>) -> Result { + fn fetch_pr_info(&self, input: &str, repo_override: Option<&str>) -> Result { let az = self.resolve_ref(input, repo_override)?; let id = az .id - .ok_or_else(|| format!("No PR id found in: {}", input))?; + .ok_or_else(|| PrError::InvalidRef(format!("No PR id found in: {}", input)))?; let meta = client::fetch_pr_metadata(&az.org_url, &az.project, &az.repo, id)?; @@ -89,14 +89,14 @@ impl PrProvider for AzureProvider { }) } - fn detect_current_branch_pr(&self, repo_override: Option<&str>) -> Result { + fn detect_current_branch_pr(&self, repo_override: Option<&str>) -> Result { let remote = repo_override .filter(|o| self.matches_origin(o)) .map(|s| s.to_string()) .or_else(read_origin_url) .ok_or_else(|| "Could not determine Azure DevOps repository.".to_string())?; let az = parse_azure_remote(&remote) - .ok_or_else(|| format!("Could not parse Azure DevOps remote: {}", remote))?; + .ok_or_else(|| PrError::InvalidRef(format!("Could not parse Azure DevOps remote: {}", remote)))?; let branch_out = Command::new("git") .args(["rev-parse", "--abbrev-ref", "HEAD"]) @@ -104,14 +104,14 @@ impl PrProvider for AzureProvider { .map_err(|e| format!("Failed to run git: {}", e))?; let branch = String::from_utf8_lossy(&branch_out.stdout).trim().to_string(); if branch.is_empty() { - return Err("Could not determine the current branch".to_string()); + return Err(PrError::Other("Could not determine the current branch".to_string())); } let id = client::detect_active_pr(&az.org_url, &az.project, &az.repo, &branch)?; Ok(id.to_string()) } - fn load_pr_file_diffs(&self, pr: &PrInfo) -> Result, String> { + fn load_pr_file_diffs(&self, pr: &PrInfo) -> Result, PrError> { client::load_pr_file_diffs(pr) } diff --git a/src/command/diff/pr_provider/github.rs b/src/command/diff/pr_provider/github.rs index 4477941f..7bf5347a 100644 --- a/src/command/diff/pr_provider/github.rs +++ b/src/command/diff/pr_provider/github.rs @@ -12,7 +12,7 @@ use crate::command::diff::git::build_file_diff; use crate::command::diff::types::FileDiff; use crate::command::diff::PrInfo; -use super::PrProvider; +use super::{PrError, PrProvider}; /// Max concurrent `gh api` requests when fetching PR file contents. /// GitHub's documented secondary rate limit caps concurrent requests at 100 @@ -31,15 +31,15 @@ impl PrProvider for GitHubProvider { origin.contains("github.com") } - fn fetch_pr_info(&self, input: &str, repo_override: Option<&str>) -> Result { + fn fetch_pr_info(&self, input: &str, repo_override: Option<&str>) -> Result { fetch_pr_info(input, repo_override) } - fn detect_current_branch_pr(&self, _repo_override: Option<&str>) -> Result { + fn detect_current_branch_pr(&self, _repo_override: Option<&str>) -> Result { detect_current_branch_pr() } - fn load_pr_file_diffs(&self, pr: &PrInfo) -> Result, String> { + fn load_pr_file_diffs(&self, pr: &PrInfo) -> Result, PrError> { load_pr_file_diffs(pr) } @@ -47,11 +47,11 @@ impl PrProvider for GitHubProvider { true } - fn fetch_viewed_files(&self, pr: &PrInfo) -> Result, String> { + fn fetch_viewed_files(&self, pr: &PrInfo) -> Result, PrError> { fetch_viewed_files(pr) } - fn set_file_viewed(&self, pr: &PrInfo, path: &str, viewed: bool) -> Result<(), String> { + fn set_file_viewed(&self, pr: &PrInfo, path: &str, viewed: bool) -> Result<(), PrError> { if viewed { mark_file_as_viewed_sync(&pr.node_id, path) } else { @@ -128,12 +128,12 @@ fn resolve_origin_repo() -> Result { } } -fn fetch_pr_info(pr_input: &str, repo_override: Option<&str>) -> Result { +fn fetch_pr_info(pr_input: &str, repo_override: Option<&str>) -> Result { let (owner, repo, number) = parse_pr_input(pr_input).ok_or_else(|| { - format!( + PrError::InvalidRef(format!( "Invalid PR reference: {}. Use a PR number or URL.", pr_input - ) + )) })?; let repo_full = match (&owner, &repo, repo_override) { @@ -145,7 +145,7 @@ fn fetch_pr_info(pr_input: &str, repo_override: Option<&str>) -> Result = repo_full.split('/').collect(); if parts.len() != 2 { - return Err(format!("Invalid repo format: {}", repo_full)); + return Err(PrError::InvalidRef(format!("Invalid repo format: {}", repo_full))); } ( owner.unwrap_or_else(|| parts[0].to_string()), @@ -166,7 +166,7 @@ fn fetch_pr_info(pr_input: &str, repo_override: Option<&str>) -> Result Option { None } -fn detect_current_branch_pr() -> Result { +fn detect_current_branch_pr() -> Result { let output = Command::new("gh") .args(["pr", "view", "--json", "number", "-q", ".number"]) .output() @@ -241,13 +241,13 @@ fn detect_current_branch_pr() -> Result { let stderr = String::from_utf8_lossy(&output.stderr); let msg = stderr.trim(); if msg.is_empty() { - return Err("No PR found for the current branch".to_string()); + return Err(PrError::NotFound("No PR found for the current branch".to_string())); } - return Err(msg.to_string()); + return Err(PrError::Other(msg.to_string())); } let number = String::from_utf8_lossy(&output.stdout).trim().to_string(); if number.is_empty() { - return Err("No PR found for the current branch".to_string()); + return Err(PrError::NotFound("No PR found for the current branch".to_string())); } Ok(number) } @@ -266,7 +266,7 @@ fn file_anchor(filename: &str) -> String { // --------------------------------------------------------------------------- /// Fetch the list of files that are marked as viewed on GitHub -fn fetch_viewed_files(pr_info: &PrInfo) -> Result, String> { +fn fetch_viewed_files(pr_info: &PrInfo) -> Result, PrError> { let query = format!( r#"query {{ repository(owner: "{}", name: "{}") {{ pullRequest(number: {}) {{ files(first: 100) {{ nodes {{ path viewerViewedState }} }} }} }} }}"#, pr_info.repo_owner, pr_info.repo_name, pr_info.number @@ -279,7 +279,7 @@ fn fetch_viewed_files(pr_info: &PrInfo) -> Result, String> { if !output.status.success() { let stderr = String::from_utf8_lossy(&output.stderr); - return Err(format!("gh api graphql failed: {}", stderr.trim())); + return Err(PrError::Other(format!("gh api graphql failed: {}", stderr.trim()))); } let json_str = String::from_utf8_lossy(&output.stdout); @@ -319,7 +319,7 @@ fn fetch_viewed_files(pr_info: &PrInfo) -> Result, String> { } /// Mark a file as viewed on GitHub PR (blocking) -fn mark_file_as_viewed_sync(node_id: &str, file_path: &str) -> Result<(), String> { +fn mark_file_as_viewed_sync(node_id: &str, file_path: &str) -> Result<(), PrError> { let mutation = format!( r#"mutation {{ markFileAsViewed(input: {{ pullRequestId: "{}", path: "{}" }}) {{ clientMutationId }} }}"#, node_id, file_path @@ -332,14 +332,14 @@ fn mark_file_as_viewed_sync(node_id: &str, file_path: &str) -> Result<(), String if !output.status.success() { let stderr = String::from_utf8_lossy(&output.stderr); - return Err(stderr.trim().to_string()); + return Err(PrError::Other(stderr.trim().to_string())); } Ok(()) } /// Unmark a file as viewed on GitHub PR (blocking) -fn unmark_file_as_viewed_sync(node_id: &str, file_path: &str) -> Result<(), String> { +fn unmark_file_as_viewed_sync(node_id: &str, file_path: &str) -> Result<(), PrError> { let mutation = format!( r#"mutation {{ unmarkFileAsViewed(input: {{ pullRequestId: "{}", path: "{}" }}) {{ clientMutationId }} }}"#, node_id, file_path @@ -352,7 +352,7 @@ fn unmark_file_as_viewed_sync(node_id: &str, file_path: &str) -> Result<(), Stri if !output.status.success() { let stderr = String::from_utf8_lossy(&output.stderr); - return Err(stderr.trim().to_string()); + return Err(PrError::Other(stderr.trim().to_string())); } Ok(()) @@ -362,7 +362,7 @@ fn unmark_file_as_viewed_sync(node_id: &str, file_path: &str) -> Result<(), Stri // File diffs (gh pr diff + parallel contents fetch) // --------------------------------------------------------------------------- -fn load_pr_file_diffs(pr_info: &PrInfo) -> Result, String> { +fn load_pr_file_diffs(pr_info: &PrInfo) -> Result, PrError> { let repo_arg = format!("{}/{}", pr_info.repo_owner, pr_info.repo_name); let mut spinner = Spinner::new( @@ -390,7 +390,7 @@ fn load_pr_file_diffs(pr_info: &PrInfo) -> Result, String> { Err(e) => { let msg = format!("Failed to run gh pr diff: {}", e); spinner.fail(&msg); - return Err(msg); + return Err(PrError::Other(msg)); } }; @@ -398,7 +398,7 @@ fn load_pr_file_diffs(pr_info: &PrInfo) -> Result, String> { let stderr = String::from_utf8_lossy(&output.stderr); let msg = format!("gh pr diff failed: {}", stderr.trim()); spinner.fail(&msg); - return Err(msg); + return Err(PrError::Other(msg)); } let diff_output = String::from_utf8_lossy(&output.stdout); diff --git a/src/command/diff/pr_provider/mod.rs b/src/command/diff/pr_provider/mod.rs index d9622667..837e4e82 100644 --- a/src/command/diff/pr_provider/mod.rs +++ b/src/command/diff/pr_provider/mod.rs @@ -19,6 +19,38 @@ use super::PrInfo; use azure::AzureProvider; use github::GitHubProvider; +/// An error from a PR-provider operation. The variant is the kind, so the diff +/// UI can react to it (e.g. prompt for credentials on [`PrError::Auth`]). +/// [`PrError::Other`] is the catch-all for CLI/transport failures, and the +/// `From` impl lets internal string errors fall through to it. +#[derive(Debug, thiserror::Error)] +pub enum PrError { + /// Authentication or authorization failed (missing/insufficient token). + #[error("authentication failed: {0}")] + Auth(String), + /// The PR (or the current branch's PR) could not be found. + #[error("not found: {0}")] + NotFound(String), + /// The input couldn't be parsed as a PR reference for this provider. + #[error("invalid PR reference: {0}")] + InvalidRef(String), + /// Anything else: CLI invocation failure, transport error, bad output. + #[error("{0}")] + Other(String), +} + +impl From for PrError { + fn from(s: String) -> Self { + PrError::Other(s) + } +} + +impl From<&str> for PrError { + fn from(s: &str) -> Self { + PrError::Other(s.to_string()) + } +} + /// A pull-request hosting provider. Each method maps to one capability the diff /// UI needs; the viewed-file sync methods default to no-ops so providers without /// that concept (Azure DevOps) don't have to implement them. @@ -34,13 +66,13 @@ pub trait PrProvider: Sync { fn matches_origin(&self, origin: &str) -> bool; /// Resolve a PR number/URL into full metadata. - fn fetch_pr_info(&self, input: &str, repo_override: Option<&str>) -> Result; + fn fetch_pr_info(&self, input: &str, repo_override: Option<&str>) -> Result; /// Find the PR associated with the current branch. - fn detect_current_branch_pr(&self, repo_override: Option<&str>) -> Result; + fn detect_current_branch_pr(&self, repo_override: Option<&str>) -> Result; /// Load the file diffs for a PR. - fn load_pr_file_diffs(&self, pr: &PrInfo) -> Result, String>; + fn load_pr_file_diffs(&self, pr: &PrInfo) -> Result, PrError>; /// Whether this provider supports syncing per-file "viewed" state. fn supports_viewed_sync(&self) -> bool { @@ -48,12 +80,12 @@ pub trait PrProvider: Sync { } /// Fetch the set of paths currently marked as viewed. - fn fetch_viewed_files(&self, _pr: &PrInfo) -> Result, String> { + fn fetch_viewed_files(&self, _pr: &PrInfo) -> Result, PrError> { Ok(HashSet::new()) } /// Mark/unmark a file as viewed (blocking). - fn set_file_viewed(&self, _pr: &PrInfo, _path: &str, _viewed: bool) -> Result<(), String> { + fn set_file_viewed(&self, _pr: &PrInfo, _path: &str, _viewed: bool) -> Result<(), PrError> { Ok(()) } @@ -118,19 +150,19 @@ fn provider_for_input(input: &str, repo_override: Option<&str>) -> &'static dyn // Dispatchers used by the rest of the diff UI // --------------------------------------------------------------------------- -pub fn fetch_pr_info(input: &str, repo_override: Option<&str>) -> Result { +pub fn fetch_pr_info(input: &str, repo_override: Option<&str>) -> Result { provider_for_input(input, repo_override).fetch_pr_info(input, repo_override) } -pub fn detect_current_branch_pr(repo_override: Option<&str>) -> Result { +pub fn detect_current_branch_pr(repo_override: Option<&str>) -> Result { provider_for_origin(repo_override).detect_current_branch_pr(repo_override) } -pub fn load_pr_file_diffs(pr: &PrInfo) -> Result, String> { +pub fn load_pr_file_diffs(pr: &PrInfo) -> Result, PrError> { pr.provider.load_pr_file_diffs(pr) } -pub fn fetch_viewed_files(pr: &PrInfo) -> Result, String> { +pub fn fetch_viewed_files(pr: &PrInfo) -> Result, PrError> { pr.provider.fetch_viewed_files(pr) } From e7867278cd189afa69124a5ea188cb46d9e94f16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=ABl=20Kuijper?= Date: Fri, 5 Jun 2026 23:29:00 +0200 Subject: [PATCH 05/11] refactor(diff): ProviderData enum instead of optional-field bag PrInfo carried node_id (GitHub-only) plus project/org_url (Azure-only) as Option fields that every provider saw as None for the others. Replace them with a single ProviderData enum whose variant holds exactly the fields its forge needs, so each provider pattern-matches its own data (no spurious None, and adding a forge is a new variant rather than two more Options). Common fields (refs, repo owners) stay on PrInfo. --- src/command/diff/mod.rs | 8 +++----- src/command/diff/pr_provider/azure/client.rs | 15 ++++++--------- src/command/diff/pr_provider/azure/mod.rs | 14 ++++++++------ src/command/diff/pr_provider/github.rs | 13 +++++++------ src/command/diff/pr_provider/mod.rs | 16 ++++++++++++++++ 5 files changed, 40 insertions(+), 26 deletions(-) diff --git a/src/command/diff/mod.rs b/src/command/diff/mod.rs index d37b77e7..c6f42397 100644 --- a/src/command/diff/mod.rs +++ b/src/command/diff/mod.rs @@ -26,6 +26,7 @@ use crate::vcs::VcsBackend; pub use pr_provider::{ fetch_viewed_files, mark_file_as_viewed_async, unmark_file_as_viewed_async, PrProvider, + ProviderData, }; pub struct DiffOptions { @@ -45,17 +46,14 @@ pub struct DiffOptions { pub struct PrInfo { pub provider: &'static dyn PrProvider, pub number: u64, - pub node_id: String, pub repo_owner: String, pub repo_name: String, pub base_ref: String, pub head_ref: String, pub base_repo_owner: String, pub head_repo_owner: Option, // None if head repo was deleted (fork deleted) - /// Azure DevOps project (None for GitHub). - pub project: Option, - /// Azure DevOps organisation base URL, e.g. `https://dev.azure.com/org`. - pub org_url: Option, + /// Provider-specific data (GitHub node id, or Azure org URL + project). + pub data: ProviderData, } pub fn run_diff_ui(mut options: DiffOptions, backend: &dyn VcsBackend) -> io::Result<()> { diff --git a/src/command/diff/pr_provider/azure/client.rs b/src/command/diff/pr_provider/azure/client.rs index 9e51648c..4b28306e 100644 --- a/src/command/diff/pr_provider/azure/client.rs +++ b/src/command/diff/pr_provider/azure/client.rs @@ -24,7 +24,7 @@ use tokio::task::JoinSet; use crate::command::diff::git::{build_file_diff, percent_encode}; use crate::command::diff::types::FileDiff; -use crate::command::diff::pr_provider::PrError; +use crate::command::diff::pr_provider::{PrError, ProviderData}; use crate::command::diff::PrInfo; const API_VERSION: &str = "7.1"; @@ -334,14 +334,11 @@ pub fn detect_active_pr( } pub fn load_pr_file_diffs(pr: &PrInfo) -> Result, PrError> { - let org_url = pr - .org_url - .as_deref() - .ok_or("Azure PR missing organisation URL")?; - let project = pr - .project - .as_deref() - .ok_or("Azure PR missing project")?; + let ProviderData::Azure { org_url, project } = &pr.data else { + return Err(PrError::Other( + "Azure PR missing organisation/project data".to_string(), + )); + }; let client = AdoClient::new(org_url, project, &pr.repo_name)?; let pr_id = pr.number; block_on(async move { client.load_file_diffs(pr_id).await }) diff --git a/src/command/diff/pr_provider/azure/mod.rs b/src/command/diff/pr_provider/azure/mod.rs index 18115186..f4695dd6 100644 --- a/src/command/diff/pr_provider/azure/mod.rs +++ b/src/command/diff/pr_provider/azure/mod.rs @@ -9,7 +9,7 @@ use crate::command::diff::git::percent_encode; use crate::command::diff::types::FileDiff; use crate::command::diff::PrInfo; -use super::{read_origin_url, PrError, PrProvider}; +use super::{read_origin_url, PrError, PrProvider, ProviderData}; pub struct AzureProvider; @@ -72,7 +72,6 @@ impl PrProvider for AzureProvider { Ok(PrInfo { provider: &AzureProvider, number: id, - node_id: String::new(), repo_owner: az.org.clone(), // Prefer the repo name the API reports; fall back to the URL's. repo_name: if meta.repo_name.is_empty() { @@ -84,8 +83,10 @@ impl PrProvider for AzureProvider { head_ref: strip_ref_prefix(&meta.source_ref), base_repo_owner: az.org.clone(), head_repo_owner: Some(az.org), - project: Some(az.project), - org_url: Some(az.org_url), + data: ProviderData::Azure { + org_url: az.org_url, + project: az.project, + }, }) } @@ -116,8 +117,9 @@ impl PrProvider for AzureProvider { } fn file_web_url(&self, pr: &PrInfo, filename: &str) -> Option { - let org_url = pr.org_url.as_ref()?; - let project = pr.project.as_ref()?; + let ProviderData::Azure { org_url, project } = &pr.data else { + return None; + }; Some(format!( "{}/{}/_git/{}/pullrequest/{}?path={}", org_url, diff --git a/src/command/diff/pr_provider/github.rs b/src/command/diff/pr_provider/github.rs index 7bf5347a..794d664d 100644 --- a/src/command/diff/pr_provider/github.rs +++ b/src/command/diff/pr_provider/github.rs @@ -12,7 +12,7 @@ use crate::command::diff::git::build_file_diff; use crate::command::diff::types::FileDiff; use crate::command::diff::PrInfo; -use super::{PrError, PrProvider}; +use super::{PrError, PrProvider, ProviderData}; /// Max concurrent `gh api` requests when fetching PR file contents. /// GitHub's documented secondary rate limit caps concurrent requests at 100 @@ -52,10 +52,13 @@ impl PrProvider for GitHubProvider { } fn set_file_viewed(&self, pr: &PrInfo, path: &str, viewed: bool) -> Result<(), PrError> { + let ProviderData::GitHub { node_id } = &pr.data else { + return Ok(()); // not a GitHub PR; nothing to sync + }; if viewed { - mark_file_as_viewed_sync(&pr.node_id, path) + mark_file_as_viewed_sync(node_id, path) } else { - unmark_file_as_viewed_sync(&pr.node_id, path) + unmark_file_as_viewed_sync(node_id, path) } } @@ -187,15 +190,13 @@ fn fetch_pr_info(pr_input: &str, repo_override: Option<&str>) -> Result for PrError { } } +/// Provider-specific data carried on [`PrInfo`]. Each variant holds exactly the +/// fields its forge needs, so adding a forge is a new variant rather than more +/// `Option`s smeared across a shared struct. +#[derive(Clone, Debug)] +pub enum ProviderData { + GitHub { + /// PR node id, used by the viewed-file GraphQL mutations. + node_id: String, + }, + Azure { + /// Organisation base URL, e.g. `https://dev.azure.com/org`. + org_url: String, + project: String, + }, +} + /// A pull-request hosting provider. Each method maps to one capability the diff /// UI needs; the viewed-file sync methods default to no-ops so providers without /// that concept (Azure DevOps) don't have to implement them. From b04292be88ffbb7ae927c3022ff9b393e530be5b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=ABl=20Kuijper?= Date: Fri, 5 Jun 2026 23:30:33 +0200 Subject: [PATCH 06/11] refactor(diff): ViewedSync sub-trait replaces supports_viewed_sync bool A supports_viewed_sync() -> bool can silently drift from the fetch/set impls (override one, forget the other). Replace the three methods with a single viewed_sync() -> Option<&dyn ViewedSync>: having the impl *is* the capability, so it can't disagree with itself. GitHub returns Some(self) and implements ViewedSync; Azure inherits the None default. The fetch/set facades route through the Option, no-op when absent. --- src/command/diff/pr_provider/github.rs | 32 ++++++++++++---------- src/command/diff/pr_provider/mod.rs | 38 ++++++++++++++++---------- 2 files changed, 40 insertions(+), 30 deletions(-) diff --git a/src/command/diff/pr_provider/github.rs b/src/command/diff/pr_provider/github.rs index 794d664d..388dec31 100644 --- a/src/command/diff/pr_provider/github.rs +++ b/src/command/diff/pr_provider/github.rs @@ -12,7 +12,7 @@ use crate::command::diff::git::build_file_diff; use crate::command::diff::types::FileDiff; use crate::command::diff::PrInfo; -use super::{PrError, PrProvider, ProviderData}; +use super::{PrError, PrProvider, ProviderData, ViewedSync}; /// Max concurrent `gh api` requests when fetching PR file contents. /// GitHub's documented secondary rate limit caps concurrent requests at 100 @@ -43,15 +43,27 @@ impl PrProvider for GitHubProvider { load_pr_file_diffs(pr) } - fn supports_viewed_sync(&self) -> bool { - true + fn file_web_url(&self, pr: &PrInfo, filename: &str) -> Option { + Some(format!( + "https://github.com/{}/{}/pull/{}/files#diff-{}", + pr.repo_owner, + pr.repo_name, + pr.number, + file_anchor(filename) + )) } - fn fetch_viewed_files(&self, pr: &PrInfo) -> Result, PrError> { + fn viewed_sync(&self) -> Option<&dyn ViewedSync> { + Some(self) + } +} + +impl ViewedSync for GitHubProvider { + fn fetch(&self, pr: &PrInfo) -> Result, PrError> { fetch_viewed_files(pr) } - fn set_file_viewed(&self, pr: &PrInfo, path: &str, viewed: bool) -> Result<(), PrError> { + fn set(&self, pr: &PrInfo, path: &str, viewed: bool) -> Result<(), PrError> { let ProviderData::GitHub { node_id } = &pr.data else { return Ok(()); // not a GitHub PR; nothing to sync }; @@ -61,16 +73,6 @@ impl PrProvider for GitHubProvider { unmark_file_as_viewed_sync(node_id, path) } } - - fn file_web_url(&self, pr: &PrInfo, filename: &str) -> Option { - Some(format!( - "https://github.com/{}/{}/pull/{}/files#diff-{}", - pr.repo_owner, - pr.repo_name, - pr.number, - file_anchor(filename) - )) - } } // --------------------------------------------------------------------------- diff --git a/src/command/diff/pr_provider/mod.rs b/src/command/diff/pr_provider/mod.rs index 427baa9a..7323eacb 100644 --- a/src/command/diff/pr_provider/mod.rs +++ b/src/command/diff/pr_provider/mod.rs @@ -90,23 +90,26 @@ pub trait PrProvider: Sync { /// Load the file diffs for a PR. fn load_pr_file_diffs(&self, pr: &PrInfo) -> Result, PrError>; - /// Whether this provider supports syncing per-file "viewed" state. - fn supports_viewed_sync(&self) -> bool { - false + /// Build a browser URL for `filename` within the PR. + fn file_web_url(&self, pr: &PrInfo, filename: &str) -> Option; + + /// Per-file "viewed" state sync, if this provider supports it. Returning + /// `Some` *is* the capability — there's no separate boolean flag that can + /// drift out of step with the implementation. + fn viewed_sync(&self) -> Option<&dyn ViewedSync> { + None } +} +/// Syncing per-file "viewed" state with the forge (e.g. GitHub's PR file +/// checkboxes). Providers without the concept simply don't return one from +/// [`PrProvider::viewed_sync`]. +pub trait ViewedSync { /// Fetch the set of paths currently marked as viewed. - fn fetch_viewed_files(&self, _pr: &PrInfo) -> Result, PrError> { - Ok(HashSet::new()) - } + fn fetch(&self, pr: &PrInfo) -> Result, PrError>; /// Mark/unmark a file as viewed (blocking). - fn set_file_viewed(&self, _pr: &PrInfo, _path: &str, _viewed: bool) -> Result<(), PrError> { - Ok(()) - } - - /// Build a browser URL for `filename` within the PR. - fn file_web_url(&self, pr: &PrInfo, filename: &str) -> Option; + fn set(&self, pr: &PrInfo, path: &str, viewed: bool) -> Result<(), PrError>; } // --------------------------------------------------------------------------- @@ -179,7 +182,10 @@ pub fn load_pr_file_diffs(pr: &PrInfo) -> Result, PrError> { } pub fn fetch_viewed_files(pr: &PrInfo) -> Result, PrError> { - pr.provider.fetch_viewed_files(pr) + match pr.provider.viewed_sync() { + Some(vs) => vs.fetch(pr), + None => Ok(HashSet::new()), + } } pub fn pr_file_web_url(pr: &PrInfo, filename: &str) -> Option { @@ -195,13 +201,15 @@ pub fn unmark_file_as_viewed_async(pr: &PrInfo, file_path: &str) { } fn set_file_viewed_async(pr: &PrInfo, file_path: &str, viewed: bool) { - if !pr.provider.supports_viewed_sync() { + if pr.provider.viewed_sync().is_none() { return; } let pr = pr.clone(); let path = file_path.to_string(); thread::spawn(move || { - let _ = pr.provider.set_file_viewed(&pr, &path, viewed); + if let Some(vs) = pr.provider.viewed_sync() { + let _ = vs.set(&pr, &path, viewed); + } }); } From 3003c7ae1f04fda9b67491a91f83526563051a6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=ABl=20Kuijper?= Date: Fri, 5 Jun 2026 23:32:05 +0200 Subject: [PATCH 07/11] style(diff): rustfmt the pr_provider modules --- src/command/diff/pr_provider/azure/client.rs | 32 +++++++++++++----- src/command/diff/pr_provider/azure/mod.rs | 29 ++++++++++------ src/command/diff/pr_provider/github.rs | 35 ++++++++++++++++---- src/command/diff/pr_provider/mod.rs | 10 ++++-- 4 files changed, 77 insertions(+), 29 deletions(-) diff --git a/src/command/diff/pr_provider/azure/client.rs b/src/command/diff/pr_provider/azure/client.rs index 4b28306e..870f3f36 100644 --- a/src/command/diff/pr_provider/azure/client.rs +++ b/src/command/diff/pr_provider/azure/client.rs @@ -23,8 +23,8 @@ use tokio::sync::Semaphore; use tokio::task::JoinSet; use crate::command::diff::git::{build_file_diff, percent_encode}; -use crate::command::diff::types::FileDiff; use crate::command::diff::pr_provider::{PrError, ProviderData}; +use crate::command::diff::types::FileDiff; use crate::command::diff::PrInfo; const API_VERSION: &str = "7.1"; @@ -100,7 +100,8 @@ impl AdoClient { if !status.is_success() { return Err(auth_hint(status, &body)); } - serde_json::from_str(&body).map_err(|e| PrError::Other(format!("invalid JSON from Azure: {}", e))) + serde_json::from_str(&body) + .map_err(|e| PrError::Other(format!("invalid JSON from Azure: {}", e))) } /// Fetch a blob's text content. An absent `blob_id` (the missing side of an @@ -112,7 +113,11 @@ impl AdoClient { }; let url = format!( "{}?$format=text&api-version={}", - self.git_url(&format!("repositories/{}/blobs/{}", percent_encode(&self.repo), blob_id)), + self.git_url(&format!( + "repositories/{}/blobs/{}", + percent_encode(&self.repo), + blob_id + )), API_VERSION ); let resp = self @@ -203,7 +208,10 @@ impl AdoClient { let client = self.clone(); let sem = Arc::clone(&sem); set.spawn(async move { - let _permit = sem.acquire_owned().await.expect("blob semaphore not closed"); + let _permit = sem + .acquire_owned() + .await + .expect("blob semaphore not closed"); let old = client.blob_text(change.old_blob.as_deref()).await?; let new = client.blob_text(change.new_blob.as_deref()).await?; Ok((idx, build_file_diff(change.path, old, new))) @@ -319,7 +327,10 @@ pub fn detect_active_pr( block_on(async move { let url = format!( "{}?searchCriteria.status=active&searchCriteria.sourceRefName={}&api-version={}", - client.git_url(&format!("repositories/{}/pullrequests", percent_encode(repo))), + client.git_url(&format!( + "repositories/{}/pullrequests", + percent_encode(repo) + )), percent_encode(&format!("refs/heads/{}", branch)), API_VERSION ); @@ -404,9 +415,9 @@ fn auth_hint(status: reqwest::StatusCode, body: &str) -> PrError { StatusCode::FORBIDDEN => PrError::Auth( "Azure DevOps returned 403. The token lacks access to this repository.".to_string(), ), - StatusCode::NOT_FOUND => PrError::NotFound( - "Azure DevOps returned 404 (PR or repository not found).".to_string(), - ), + StatusCode::NOT_FOUND => { + PrError::NotFound("Azure DevOps returned 404 (PR or repository not found).".to_string()) + } _ => { let snippet: String = body.chars().take(200).collect(); PrError::Other(format!( @@ -473,7 +484,10 @@ mod tests { #[test] fn encodes_segments() { assert_eq!(percent_encode("My Project"), "My%20Project"); - assert_eq!(percent_encode("refs/heads/feature/x"), "refs%2Fheads%2Ffeature%2Fx"); + assert_eq!( + percent_encode("refs/heads/feature/x"), + "refs%2Fheads%2Ffeature%2Fx" + ); assert_eq!(percent_encode("simple-repo.git"), "simple-repo.git"); } } diff --git a/src/command/diff/pr_provider/azure/mod.rs b/src/command/diff/pr_provider/azure/mod.rs index f4695dd6..1139f26f 100644 --- a/src/command/diff/pr_provider/azure/mod.rs +++ b/src/command/diff/pr_provider/azure/mod.rs @@ -33,9 +33,9 @@ impl AzureProvider { } // Bare PR number: take the coordinates from --origin (if it's an Azure // URL) or from the git `origin` remote. - let id = input - .parse::() - .map_err(|_| PrError::InvalidRef(format!("Invalid Azure DevOps PR reference: {}", input)))?; + let id = input.parse::().map_err(|_| { + PrError::InvalidRef(format!("Invalid Azure DevOps PR reference: {}", input)) + })?; let remote = repo_override .filter(|o| self.matches_origin(o)) .map(|s| s.to_string()) @@ -44,8 +44,9 @@ impl AzureProvider { "Could not determine Azure DevOps repository. Run inside the repo or pass a PR URL." .to_string() })?; - let mut parsed = parse_azure_remote(&remote) - .ok_or_else(|| PrError::InvalidRef(format!("Could not parse Azure DevOps remote: {}", remote)))?; + let mut parsed = parse_azure_remote(&remote).ok_or_else(|| { + PrError::InvalidRef(format!("Could not parse Azure DevOps remote: {}", remote)) + })?; parsed.id = Some(id); Ok(parsed) } @@ -96,16 +97,21 @@ impl PrProvider for AzureProvider { .map(|s| s.to_string()) .or_else(read_origin_url) .ok_or_else(|| "Could not determine Azure DevOps repository.".to_string())?; - let az = parse_azure_remote(&remote) - .ok_or_else(|| PrError::InvalidRef(format!("Could not parse Azure DevOps remote: {}", remote)))?; + let az = parse_azure_remote(&remote).ok_or_else(|| { + PrError::InvalidRef(format!("Could not parse Azure DevOps remote: {}", remote)) + })?; let branch_out = Command::new("git") .args(["rev-parse", "--abbrev-ref", "HEAD"]) .output() .map_err(|e| format!("Failed to run git: {}", e))?; - let branch = String::from_utf8_lossy(&branch_out.stdout).trim().to_string(); + let branch = String::from_utf8_lossy(&branch_out.stdout) + .trim() + .to_string(); if branch.is_empty() { - return Err(PrError::Other("Could not determine the current branch".to_string())); + return Err(PrError::Other( + "Could not determine the current branch".to_string(), + )); } let id = client::detect_active_pr(&az.org_url, &az.project, &az.repo, &branch)?; @@ -269,8 +275,9 @@ mod tests { #[test] fn parse_azure_visualstudio_url() { - let r = parse_azure_url("https://myorg.visualstudio.com/MyProject/_git/myrepo/pullrequest/9") - .expect("should parse"); + let r = + parse_azure_url("https://myorg.visualstudio.com/MyProject/_git/myrepo/pullrequest/9") + .expect("should parse"); assert_eq!(r.org_url, "https://myorg.visualstudio.com"); assert_eq!(r.project, "MyProject"); assert_eq!(r.repo, "myrepo"); diff --git a/src/command/diff/pr_provider/github.rs b/src/command/diff/pr_provider/github.rs index 388dec31..9e0e0f8d 100644 --- a/src/command/diff/pr_provider/github.rs +++ b/src/command/diff/pr_provider/github.rs @@ -129,7 +129,10 @@ fn resolve_origin_repo() -> Result { if parts.len() >= 2 { Ok(format!("{}/{}", parts[0], parts[1])) } else { - Err(format!("Could not parse owner/repo from origin URL: {}", url)) + Err(format!( + "Could not parse owner/repo from origin URL: {}", + url + )) } } @@ -150,7 +153,10 @@ fn fetch_pr_info(pr_input: &str, repo_override: Option<&str>) -> Result = repo_full.split('/').collect(); if parts.len() != 2 { - return Err(PrError::InvalidRef(format!("Invalid repo format: {}", repo_full))); + return Err(PrError::InvalidRef(format!( + "Invalid repo format: {}", + repo_full + ))); } ( owner.unwrap_or_else(|| parts[0].to_string()), @@ -171,7 +177,10 @@ fn fetch_pr_info(pr_input: &str, repo_override: Option<&str>) -> Result Result { let stderr = String::from_utf8_lossy(&output.stderr); let msg = stderr.trim(); if msg.is_empty() { - return Err(PrError::NotFound("No PR found for the current branch".to_string())); + return Err(PrError::NotFound( + "No PR found for the current branch".to_string(), + )); } return Err(PrError::Other(msg.to_string())); } let number = String::from_utf8_lossy(&output.stdout).trim().to_string(); if number.is_empty() { - return Err(PrError::NotFound("No PR found for the current branch".to_string())); + return Err(PrError::NotFound( + "No PR found for the current branch".to_string(), + )); } Ok(number) } @@ -282,7 +295,10 @@ fn fetch_viewed_files(pr_info: &PrInfo) -> Result, PrError> { if !output.status.success() { let stderr = String::from_utf8_lossy(&output.stderr); - return Err(PrError::Other(format!("gh api graphql failed: {}", stderr.trim()))); + return Err(PrError::Other(format!( + "gh api graphql failed: {}", + stderr.trim() + ))); } let json_str = String::from_utf8_lossy(&output.stdout); @@ -547,7 +563,12 @@ fn fetch_pr_file_contents_parallel( last_finished = Some(filename); } } - spinner.update_text(format_fetch_progress(done, total, &in_flight, last_finished.as_deref())); + spinner.update_text(format_fetch_progress( + done, + total, + &in_flight, + last_finished.as_deref(), + )); } for h in handles { diff --git a/src/command/diff/pr_provider/mod.rs b/src/command/diff/pr_provider/mod.rs index 7323eacb..1c175d8c 100644 --- a/src/command/diff/pr_provider/mod.rs +++ b/src/command/diff/pr_provider/mod.rs @@ -150,7 +150,11 @@ fn read_origin_url() -> Option { fn provider_for_origin(repo_override: Option<&str>) -> &'static dyn PrProvider { let candidates = [repo_override.map(|s| s.to_string()), read_origin_url()]; for candidate in candidates.into_iter().flatten() { - if let Some(p) = PROVIDERS.iter().copied().find(|p| p.matches_origin(&candidate)) { + if let Some(p) = PROVIDERS + .iter() + .copied() + .find(|p| p.matches_origin(&candidate)) + { return p; } } @@ -221,7 +225,9 @@ mod tests { fn is_pr_reference_detects_forms() { assert!(is_pr_reference("123")); assert!(is_pr_reference("https://github.com/o/r/pull/1")); - assert!(is_pr_reference("https://dev.azure.com/o/p/_git/r/pullrequest/1")); + assert!(is_pr_reference( + "https://dev.azure.com/o/p/_git/r/pullrequest/1" + )); assert!(!is_pr_reference("main..feature")); } } From 2450d49a122ad48ee7f8c0f22f34195dd31cae87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=ABl=20Kuijper?= Date: Fri, 5 Jun 2026 23:44:10 +0200 Subject: [PATCH 08/11] refactor(diff): typed serde deserialization for forge responses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stop hand-navigating serde_json::Value (and, on the GitHub side, scanning raw JSON with str::find) and deserialize into small typed structs instead — the compiler now checks the wire shape and the structs document it. - azure/client.rs: a generic get:: replaces get_json; IterationList, ChangesPage/RawChange/RawItem, PrDetail, PrList, and TokenResponse model each endpoint. RawChange::into_change keeps the folder-skip / rename-fallback logic (fallible, so a method returning Option rather than a From impl). - github.rs: GraphQl + RepoNode envelopes with #[serde(rename_all = camelCase)] replace extract_json_string / extract_nested_login and the viewerViewedState string scan, which were brittle to whitespace/key order. - Add parse tests locking the GraphQL wire contract (incl. deleted head fork). Unknown fields are ignored by default, preserving the prior "read only what we need" tolerance. Behaviour-preserving; 148 tests pass. --- src/command/diff/pr_provider/azure/client.rs | 213 ++++++++++++------- src/command/diff/pr_provider/github.rs | 209 ++++++++++-------- 2 files changed, 254 insertions(+), 168 deletions(-) diff --git a/src/command/diff/pr_provider/azure/client.rs b/src/command/diff/pr_provider/azure/client.rs index 870f3f36..b5635e5f 100644 --- a/src/command/diff/pr_provider/azure/client.rs +++ b/src/command/diff/pr_provider/azure/client.rs @@ -18,7 +18,7 @@ use std::process::Command; use std::sync::Arc; use reqwest::header::ACCEPT; -use serde_json::Value; +use serde::Deserialize; use tokio::sync::Semaphore; use tokio::task::JoinSet; @@ -88,7 +88,9 @@ impl AdoClient { ) } - async fn get_json(&self, url: &str) -> Result { + /// GET `url` and deserialize the JSON body into `T`. Unknown fields are + /// ignored, so each caller's struct declares only the fields it needs. + async fn get(&self, url: &str) -> Result { let resp = self .authed(self.http.get(url)) .header(ACCEPT, "application/json") @@ -150,10 +152,11 @@ impl AdoClient { )), API_VERSION ); - let json = self.get_json(&url).await?; - json.get("value") - .and_then(|v| v.as_array()) - .and_then(|arr| arr.iter().filter_map(|i| i.get("id")?.as_u64()).max()) + let list: IterationList = self.get(&url).await?; + list.value + .iter() + .map(|i| i.id) + .max() .ok_or_else(|| PrError::Other("PR has no iterations".to_string())) } @@ -175,25 +178,19 @@ impl AdoClient { skip, API_VERSION ); - let json = self.get_json(&url).await?; - let page = json - .get("changeEntries") - .and_then(|v| v.as_array()) - .cloned() - .unwrap_or_default(); - let page_len = page.len(); - for entry in page { - if let Some(change) = ChangeEntry::from_json(&entry) { - entries.push(change); - } - } + let page: ChangesPage = self.get(&url).await?; + let page_len = page.change_entries.len(); + entries.extend( + page.change_entries + .into_iter() + .filter_map(RawChange::into_change), + ); // `nextSkip` is the canonical "more pages" signal; fall back to a // short page meaning we're done. - let next_skip = json.get("nextSkip").and_then(|v| v.as_u64()).unwrap_or(0); - if next_skip == 0 || page_len < CHANGES_PAGE { + if page.next_skip == 0 || page_len < CHANGES_PAGE { break; } - skip = next_skip as usize; + skip = page.next_skip as usize; } Ok(entries) } @@ -232,6 +229,46 @@ impl AdoClient { } } +/// The PR iterations list; we only need each iteration's numeric id. +#[derive(Deserialize)] +struct IterationList { + value: Vec, +} + +#[derive(Deserialize)] +struct Iteration { + id: u64, +} + +/// A page of the iteration-changes endpoint. +#[derive(Deserialize)] +#[serde(rename_all = "camelCase")] +struct ChangesPage { + #[serde(default)] + change_entries: Vec, + /// Canonical "more pages" cursor; `0`/absent means we're done. + #[serde(default)] + next_skip: u64, +} + +/// A `changeEntries[]` entry exactly as Azure returns it. +#[derive(Deserialize)] +struct RawChange { + item: Option, + #[serde(rename = "originalPath")] + original_path: Option, +} + +#[derive(Deserialize, Default)] +#[serde(rename_all = "camelCase")] +struct RawItem { + path: Option, + object_id: Option, + original_object_id: Option, + #[serde(default)] + is_folder: bool, +} + /// One PR file change reduced to what the diff UI needs. struct ChangeEntry { /// Repo-relative path without a leading slash. @@ -242,35 +279,19 @@ struct ChangeEntry { old_blob: Option, } -impl ChangeEntry { - fn from_json(entry: &Value) -> Option { - let item = entry.get("item"); - // Skip folders. - if item - .and_then(|i| i.get("isFolder")) - .and_then(|v| v.as_bool()) - .unwrap_or(false) - { +impl RawChange { + /// Reduce a wire entry to a [`ChangeEntry`], skipping folders and entries + /// with no path. The rename case falls back to `originalPath`. + fn into_change(self) -> Option { + let item = self.item.unwrap_or_default(); + if item.is_folder { return None; } - let raw_path = item - .and_then(|i| i.get("path")) - .and_then(|v| v.as_str()) - .or_else(|| entry.get("originalPath").and_then(|v| v.as_str()))?; - let new_blob = item - .and_then(|i| i.get("objectId")) - .and_then(|v| v.as_str()) - .filter(|s| !s.is_empty()) - .map(|s| s.to_string()); - let old_blob = item - .and_then(|i| i.get("originalObjectId")) - .and_then(|v| v.as_str()) - .filter(|s| !s.is_empty()) - .map(|s| s.to_string()); - Some(Self { + let raw_path = item.path.or(self.original_path)?; + Some(ChangeEntry { path: raw_path.trim_start_matches('/').to_string(), - new_blob, - old_blob, + new_blob: item.object_id.filter(|s| !s.is_empty()), + old_blob: item.original_object_id.filter(|s| !s.is_empty()), }) } } @@ -279,6 +300,22 @@ impl ChangeEntry { // Public sync entry points (bridge the async client onto the sync diff path) // --------------------------------------------------------------------------- +/// The PR detail endpoint, reduced to the refs and repo name we display. +#[derive(Deserialize)] +#[serde(rename_all = "camelCase")] +struct PrDetail { + #[serde(default)] + source_ref_name: String, + #[serde(default)] + target_ref_name: String, + repository: Option, +} + +#[derive(Deserialize)] +struct RepoRef { + name: Option, +} + pub fn fetch_pr_metadata( org_url: &str, project: &str, @@ -293,30 +330,30 @@ pub fn fetch_pr_metadata( client.git_url(&format!("pullrequests/{}", pr_id)), API_VERSION ); - let json = client.get_json(&url).await?; - let source_ref = json - .get("sourceRefName") - .and_then(|v| v.as_str()) - .unwrap_or_default() - .to_string(); - let target_ref = json - .get("targetRefName") - .and_then(|v| v.as_str()) - .unwrap_or_default() - .to_string(); - let repo_name = json - .pointer("/repository/name") - .and_then(|v| v.as_str()) - .unwrap_or(repo) - .to_string(); + let detail: PrDetail = client.get(&url).await?; Ok(AzurePrMeta { - source_ref, - target_ref, - repo_name, + source_ref: detail.source_ref_name, + target_ref: detail.target_ref_name, + repo_name: detail + .repository + .and_then(|r| r.name) + .unwrap_or_else(|| repo.to_string()), }) }) } +/// The active-PR search result; we take the first match's id. +#[derive(Deserialize)] +struct PrList { + value: Vec, +} + +#[derive(Deserialize)] +#[serde(rename_all = "camelCase")] +struct PrId { + pull_request_id: u64, +} + pub fn detect_active_pr( org_url: &str, project: &str, @@ -334,12 +371,10 @@ pub fn detect_active_pr( percent_encode(&format!("refs/heads/{}", branch)), API_VERSION ); - let json = client.get_json(&url).await?; - json.get("value") - .and_then(|v| v.as_array()) - .and_then(|arr| arr.first()) - .and_then(|pr| pr.get("pullRequestId")) - .and_then(|v| v.as_u64()) + let list: PrList = client.get(&url).await?; + list.value + .first() + .map(|pr| pr.pull_request_id) .ok_or_else(|| PrError::NotFound(format!("No active PR found for branch {}", branch))) }) } @@ -397,14 +432,21 @@ fn resolve_auth() -> Result { .to_string(), )); } - let json: Value = serde_json::from_slice(&output.stdout) - .map_err(|e| format!("Could not parse az token output: {}", e))?; - json.get("accessToken") - .and_then(|v| v.as_str()) - .map(|t| AdoAuth::Bearer(t.to_string())) + let token: TokenResponse = serde_json::from_slice(&output.stdout) + .map_err(|e| PrError::Other(format!("Could not parse az token output: {}", e)))?; + token + .access_token + .map(AdoAuth::Bearer) .ok_or_else(|| PrError::Auth("az returned no access token".to_string())) } +/// The `az account get-access-token --output json` response. +#[derive(Deserialize)] +struct TokenResponse { + #[serde(rename = "accessToken")] + access_token: Option, +} + fn auth_hint(status: reqwest::StatusCode, body: &str) -> PrError { use reqwest::StatusCode; match status { @@ -433,9 +475,16 @@ fn auth_hint(status: reqwest::StatusCode, body: &str) -> PrError { mod tests { use super::*; + /// Deserialize a wire change entry and reduce it the way `changes()` does. + fn change(v: serde_json::Value) -> Option { + serde_json::from_value::(v) + .unwrap() + .into_change() + } + #[test] fn parses_add_edit_delete_changes() { - let add = ChangeEntry::from_json(&serde_json::json!({ + let add = change(serde_json::json!({ "changeType": "add", "item": { "path": "/src/new.rs", "objectId": "newsha" } })) @@ -444,7 +493,7 @@ mod tests { assert_eq!(add.new_blob.as_deref(), Some("newsha")); assert_eq!(add.old_blob, None); - let edit = ChangeEntry::from_json(&serde_json::json!({ + let edit = change(serde_json::json!({ "changeType": "edit", "item": { "path": "/a.txt", "objectId": "n", "originalObjectId": "o" } })) @@ -452,7 +501,7 @@ mod tests { assert_eq!(edit.new_blob.as_deref(), Some("n")); assert_eq!(edit.old_blob.as_deref(), Some("o")); - let del = ChangeEntry::from_json(&serde_json::json!({ + let del = change(serde_json::json!({ "changeType": "delete", "item": { "path": "/gone.rs", "originalObjectId": "o" } })) @@ -463,7 +512,7 @@ mod tests { #[test] fn skips_folders() { - assert!(ChangeEntry::from_json(&serde_json::json!({ + assert!(change(serde_json::json!({ "changeType": "add", "item": { "path": "/dir", "isFolder": true } })) @@ -472,7 +521,7 @@ mod tests { #[test] fn falls_back_to_original_path_on_rename() { - let renamed = ChangeEntry::from_json(&serde_json::json!({ + let renamed = change(serde_json::json!({ "changeType": "rename", "item": { "objectId": "n", "originalObjectId": "o" }, "originalPath": "/old/name.rs" diff --git a/src/command/diff/pr_provider/github.rs b/src/command/diff/pr_provider/github.rs index 9e0e0f8d..1e3c6025 100644 --- a/src/command/diff/pr_provider/github.rs +++ b/src/command/diff/pr_provider/github.rs @@ -6,6 +6,7 @@ use std::process::Command; use std::sync::{mpsc, Arc, Mutex}; use std::thread; +use serde::Deserialize; use spinoff::{spinners, Color, Spinner}; use crate::command::diff::git::build_file_diff; @@ -136,6 +137,43 @@ fn resolve_origin_repo() -> Result { } } +/// The `gh api graphql` response envelope: `{ "data": { ... } }`. +#[derive(Deserialize)] +struct GraphQl { + data: Option, +} + +#[derive(Deserialize)] +struct RepoNode { + repository: Option>, +} + +#[derive(Deserialize)] +struct PullRequestNode { + #[serde(rename = "pullRequest")] + pull_request: Option, +} + +#[derive(Deserialize)] +#[serde(rename_all = "camelCase")] +struct PrNode { + id: String, + base_ref_name: Option, + head_ref_name: Option, + base_repository: Option, + head_repository: Option, +} + +#[derive(Deserialize)] +struct RepoOwner { + owner: Owner, +} + +#[derive(Deserialize)] +struct Owner { + login: String, +} + fn fetch_pr_info(pr_input: &str, repo_override: Option<&str>) -> Result { let (owner, repo, number) = parse_pr_input(pr_input).ok_or_else(|| { PrError::InvalidRef(format!( @@ -183,67 +221,30 @@ fn fetch_pr_info(pr_input: &str, repo_override: Option<&str>) -> Result> = serde_json::from_slice(&output.stdout) + .map_err(|e| PrError::Other(format!("could not parse gh graphql response: {}", e)))?; + let pr = resp + .data + .and_then(|d| d.repository) + .and_then(|r| r.pull_request) + .ok_or_else(|| PrError::NotFound(format!("PR #{} not found", number)))?; Ok(PrInfo { provider: &GitHubProvider, number, - repo_owner, + repo_owner: repo_owner.clone(), repo_name, - base_ref, - head_ref, - base_repo_owner, - head_repo_owner, - data: ProviderData::GitHub { node_id }, + base_ref: pr.base_ref_name.unwrap_or_else(|| "base".to_string()), + head_ref: pr.head_ref_name.unwrap_or_else(|| "head".to_string()), + base_repo_owner: pr + .base_repository + .map(|r| r.owner.login) + .unwrap_or(repo_owner), + head_repo_owner: pr.head_repository.map(|r| r.owner.login), + data: ProviderData::GitHub { node_id: pr.id }, }) } -fn extract_json_string(json: &str, key: &str) -> Option { - let pattern = format!("\"{}\":\"", key); - if let Some(start) = json.find(&pattern) { - let value_start = start + pattern.len(); - if let Some(end) = json[value_start..].find('"') { - return Some(json[value_start..value_start + end].to_string()); - } - } - None -} - -fn extract_nested_login(json: &str, parent_key: &str) -> Option { - // Look for pattern like "baseRepository":{"owner":{"login":"username"}} - // or handle null case like "headRepository":null - let pattern = format!("\"{}\":", parent_key); - if let Some(start) = json.find(&pattern) { - let after_key = &json[start + pattern.len()..]; - // Check if it's null - if after_key.trim_start().starts_with("null") { - return None; - } - // Look for login within this section - if let Some(login_start) = after_key.find("\"login\":\"") { - let value_start = login_start + 9; - let after_login = &after_key[value_start..]; - if let Some(end) = after_login.find('"') { - return Some(after_login[..end].to_string()); - } - } - } - None -} - fn detect_current_branch_pr() -> Result { let output = Command::new("gh") .args(["pr", "view", "--json", "number", "-q", ".number"]) @@ -281,6 +282,23 @@ fn file_anchor(filename: &str) -> String { // Viewed-file state // --------------------------------------------------------------------------- +#[derive(Deserialize)] +struct PrFiles { + files: FileConnection, +} + +#[derive(Deserialize)] +struct FileConnection { + nodes: Vec, +} + +#[derive(Deserialize)] +#[serde(rename_all = "camelCase")] +struct FileNode { + path: String, + viewer_viewed_state: String, +} + /// Fetch the list of files that are marked as viewed on GitHub fn fetch_viewed_files(pr_info: &PrInfo) -> Result, PrError> { let query = format!( @@ -301,40 +319,20 @@ fn fetch_viewed_files(pr_info: &PrInfo) -> Result, PrError> { ))); } - let json_str = String::from_utf8_lossy(&output.stdout); - - // Parse the response to find viewed files - // Look for patterns like: "path":"filename","viewerViewedState":"VIEWED" - let mut viewed_files = HashSet::new(); - - // Simple parsing: find all path/viewerViewedState pairs - let mut remaining = json_str.as_ref(); - while let Some(path_start) = remaining.find("\"path\":\"") { - let path_value_start = path_start + 8; - let after_path = &remaining[path_value_start..]; - if let Some(path_end) = after_path.find('"') { - let path = &after_path[..path_end]; - - // Look for viewerViewedState after this path - let after_path_str = &after_path[path_end..]; - if let Some(state_start) = after_path_str.find("\"viewerViewedState\":\"") { - let state_value_start = state_start + 21; - let after_state = &after_path_str[state_value_start..]; - if let Some(state_end) = after_state.find('"') { - let state = &after_state[..state_end]; - if state == "VIEWED" { - viewed_files.insert(path.to_string()); - } - } - } + let resp: GraphQl> = serde_json::from_slice(&output.stdout) + .map_err(|e| PrError::Other(format!("could not parse gh graphql response: {}", e)))?; + let nodes = resp + .data + .and_then(|d| d.repository) + .and_then(|r| r.pull_request) + .map(|p| p.files.nodes) + .unwrap_or_default(); - remaining = &remaining[path_value_start + path_end..]; - } else { - break; - } - } - - Ok(viewed_files) + Ok(nodes + .into_iter() + .filter(|n| n.viewer_viewed_state == "VIEWED") + .map(|n| n.path) + .collect()) } /// Mark a file as viewed on GitHub PR (blocking) @@ -645,4 +643,43 @@ mod tests { assert!(!GitHubProvider.matches_url("https://dev.azure.com/o/p/_git/r/pullrequest/1")); assert!(GitHubProvider.matches_origin("git@github.com:owner/repo.git")); } + + #[test] + fn parses_pr_info_graphql_with_deleted_head_fork() { + let body = serde_json::json!({ + "data": { "repository": { "pullRequest": { + "id": "PR_node1", "url": "https://github.com/o/r/pull/1", + "baseRefName": "main", "headRefName": "feature", + "baseRepository": { "owner": { "login": "base-owner" } }, + "headRepository": null + }}} + }); + let resp: GraphQl> = serde_json::from_value(body).unwrap(); + let pr = resp.data.unwrap().repository.unwrap().pull_request.unwrap(); + assert_eq!(pr.id, "PR_node1"); + assert_eq!(pr.base_ref_name.as_deref(), Some("main")); + assert_eq!(pr.base_repository.unwrap().owner.login, "base-owner"); + assert!(pr.head_repository.is_none()); + } + + #[test] + fn parses_viewed_state_graphql() { + let body = serde_json::json!({ + "data": { "repository": { "pullRequest": { "files": { "nodes": [ + { "path": "a.rs", "viewerViewedState": "VIEWED" }, + { "path": "b.rs", "viewerViewedState": "UNVIEWED" } + ]}}}} + }); + let resp: GraphQl> = serde_json::from_value(body).unwrap(); + let nodes = resp + .data + .unwrap() + .repository + .unwrap() + .pull_request + .unwrap() + .files + .nodes; + assert_eq!(nodes[0].viewer_viewed_state, "VIEWED"); + } } From 45e7d20d669e2433329c301405240d4ddb6a1158 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=ABl=20Kuijper?= Date: Fri, 5 Jun 2026 23:54:36 +0200 Subject: [PATCH 09/11] fix(diff): lead Azure auth with az login / AZURE_DEVOPS_EXT_PAT ADO_PAT is a made-up alias nothing sets; AZURE_DEVOPS_EXT_PAT is the conventional Azure DevOps PAT var (read by the az devops extension), and a plain az login is enough on its own via the bearer-token fallback. Reorder the PAT precedence to check the conventional var first, and point the docs, README, and error messages at az login / AZURE_DEVOPS_EXT_PAT instead of the alias. --- README.md | 2 +- src/command/diff/pr_provider/azure/client.rs | 19 +++++++++++-------- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 180ca694..75d4d122 100644 --- a/README.md +++ b/README.md @@ -53,7 +53,7 @@ Before you begin, ensure you have: 2. [fzf](https://github.com/junegunn/fzf) (optional) - Required for `lumen explain --list` command 3. [mdcat](https://github.com/swsnr/mdcat) (optional) - Required for pretty output formatting 4. [GitHub CLI (`gh`)](https://cli.github.com/) (optional) - Required for reviewing GitHub Pull Requests -5. Azure DevOps Pull Requests (optional) - Authenticate with either a Personal Access Token (`ADO_PAT`, scope: *Code → Read*) or the [Azure CLI (`az`)](https://learn.microsoft.com/cli/azure/) signed in via `az login`. The `azure-devops` extension is **not** required. +5. Azure DevOps Pull Requests (optional) - Sign in with the [Azure CLI (`az`)](https://learn.microsoft.com/cli/azure/) via `az login` (simplest), or set a Personal Access Token in `AZURE_DEVOPS_EXT_PAT` (scope: *Code → Read*). The `azure-devops` extension is **not** required. ### Installation diff --git a/src/command/diff/pr_provider/azure/client.rs b/src/command/diff/pr_provider/azure/client.rs index b5635e5f..bc7186a1 100644 --- a/src/command/diff/pr_provider/azure/client.rs +++ b/src/command/diff/pr_provider/azure/client.rs @@ -9,9 +9,11 @@ //! against the PR's merge base — the same three-dot view the Azure web UI shows, //! rather than a raw tip-to-tip comparison. //! -//! Auth resolves to a PAT (`ADO_PAT` / `AZURE_DEVOPS_EXT_PAT`) via HTTP Basic, -//! or falls back to a bearer token from `az account get-access-token`. Only core -//! `az` (or a PAT) is required — not the `azure-devops` extension. +//! Auth: `az login` alone is enough — we mint a bearer token via `az account +//! get-access-token`. Alternatively set a PAT in `AZURE_DEVOPS_EXT_PAT` (the +//! conventional var; `AZURE_DEVOPS_PAT` / `ADO_PAT` also work), sent via HTTP +//! Basic. Only core `az` (or a PAT) is required — not the `azure-devops` +//! extension. use std::env; use std::process::Command; @@ -402,7 +404,9 @@ fn block_on(fut: F) -> F::Output { } fn resolve_auth() -> Result { - for var in ["ADO_PAT", "AZURE_DEVOPS_EXT_PAT", "AZURE_DEVOPS_PAT"] { + // `AZURE_DEVOPS_EXT_PAT` is the conventional Azure DevOps PAT var (read by + // the `az devops` CLI extension); the others are accepted as aliases. + for var in ["AZURE_DEVOPS_EXT_PAT", "AZURE_DEVOPS_PAT", "ADO_PAT"] { if let Ok(pat) = env::var(var) { if !pat.trim().is_empty() { return Ok(AdoAuth::Pat(pat)); @@ -422,14 +426,13 @@ fn resolve_auth() -> Result { .output() .map_err(|e| { PrError::Auth(format!( - "No Azure DevOps credentials: set ADO_PAT or install the Azure CLI and run `az login` ({})", + "No Azure DevOps credentials: run `az login`, or set AZURE_DEVOPS_EXT_PAT ({})", e )) })?; if !output.status.success() { return Err(PrError::Auth( - "No Azure DevOps credentials: set ADO_PAT, or run `az login` to use the Azure CLI." - .to_string(), + "No Azure DevOps credentials: run `az login`, or set AZURE_DEVOPS_EXT_PAT.".to_string(), )); } let token: TokenResponse = serde_json::from_slice(&output.stdout) @@ -451,7 +454,7 @@ fn auth_hint(status: reqwest::StatusCode, body: &str) -> PrError { use reqwest::StatusCode; match status { StatusCode::UNAUTHORIZED => PrError::Auth( - "Azure DevOps auth failed (401). Check ADO_PAT scopes (Code: Read) or run `az login`." + "Azure DevOps auth failed (401). Check your PAT scopes (Code: Read) or run `az login`." .to_string(), ), StatusCode::FORBIDDEN => PrError::Auth( From bf388da7951f6ba726c7b3a8a41409501f64e707 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=ABl=20Kuijper?= Date: Sat, 6 Jun 2026 11:28:05 +0200 Subject: [PATCH 10/11] style(diff): drop AI-style section banners and narration comments The // ---- divider banners and step-narrating comments (// Try to parse as a URL first, etc.) don't match the rest of the codebase, which uses neither. Keep only the why-comments. --- src/command/diff/pr_provider/azure/client.rs | 8 -------- src/command/diff/pr_provider/github.rs | 17 ----------------- src/command/diff/pr_provider/mod.rs | 8 -------- 3 files changed, 33 deletions(-) diff --git a/src/command/diff/pr_provider/azure/client.rs b/src/command/diff/pr_provider/azure/client.rs index bc7186a1..fd556583 100644 --- a/src/command/diff/pr_provider/azure/client.rs +++ b/src/command/diff/pr_provider/azure/client.rs @@ -298,10 +298,6 @@ impl RawChange { } } -// --------------------------------------------------------------------------- -// Public sync entry points (bridge the async client onto the sync diff path) -// --------------------------------------------------------------------------- - /// The PR detail endpoint, reduced to the refs and repo name we display. #[derive(Deserialize)] #[serde(rename_all = "camelCase")] @@ -392,10 +388,6 @@ pub fn load_pr_file_diffs(pr: &PrInfo) -> Result, PrError> { block_on(async move { client.load_file_diffs(pr_id).await }) } -// --------------------------------------------------------------------------- -// Helpers -// --------------------------------------------------------------------------- - /// Run an async future to completion from the synchronous diff path. `main` is /// a multi-threaded `#[tokio::main]`, so we mark the current worker as blocking /// and drive the future on the existing runtime — no second runtime, no new deps. diff --git a/src/command/diff/pr_provider/github.rs b/src/command/diff/pr_provider/github.rs index 1e3c6025..be8c72ba 100644 --- a/src/command/diff/pr_provider/github.rs +++ b/src/command/diff/pr_provider/github.rs @@ -76,21 +76,14 @@ impl ViewedSync for GitHubProvider { } } -// --------------------------------------------------------------------------- -// PR metadata -// --------------------------------------------------------------------------- - fn parse_pr_input(input: &str) -> Option<(Option, Option, u64)> { - // Try to parse as a URL first if input.starts_with("http://") || input.starts_with("https://") { - // Extract PR number and repo info from URL // Format: https://github.com/owner/repo/pull/123 let parts: Vec<&str> = input.trim_end_matches('/').split('/').collect(); if parts.len() >= 2 { if let Some(pos) = parts.iter().position(|&p| p == "pull") { if pos + 1 < parts.len() { if let Ok(num) = parts[pos + 1].parse::() { - // Extract owner and repo if pos >= 2 { let owner = parts[pos - 2].to_string(); let repo = parts[pos - 1].to_string(); @@ -103,7 +96,6 @@ fn parse_pr_input(input: &str) -> Option<(Option, Option, u64)> } None } else { - // Try to parse as a PR number input.parse::().ok().map(|num| (None, None, num)) } } @@ -278,10 +270,6 @@ fn file_anchor(filename: &str) -> String { format!("{:x}", hasher.finalize()) } -// --------------------------------------------------------------------------- -// Viewed-file state -// --------------------------------------------------------------------------- - #[derive(Deserialize)] struct PrFiles { files: FileConnection, @@ -375,10 +363,6 @@ fn unmark_file_as_viewed_sync(node_id: &str, file_path: &str) -> Result<(), PrEr Ok(()) } -// --------------------------------------------------------------------------- -// File diffs (gh pr diff + parallel contents fetch) -// --------------------------------------------------------------------------- - fn load_pr_file_diffs(pr_info: &PrInfo) -> Result, PrError> { let repo_arg = format!("{}/{}", pr_info.repo_owner, pr_info.repo_name); @@ -391,7 +375,6 @@ fn load_pr_file_diffs(pr_info: &PrInfo) -> Result, PrError> { Color::Cyan, ); - // Get PR diff to find changed files let output = Command::new("gh") .args([ "pr", diff --git a/src/command/diff/pr_provider/mod.rs b/src/command/diff/pr_provider/mod.rs index 1c175d8c..d66ef007 100644 --- a/src/command/diff/pr_provider/mod.rs +++ b/src/command/diff/pr_provider/mod.rs @@ -112,10 +112,6 @@ pub trait ViewedSync { fn set(&self, pr: &PrInfo, path: &str, viewed: bool) -> Result<(), PrError>; } -// --------------------------------------------------------------------------- -// Provider registry & selection -// --------------------------------------------------------------------------- - /// All compiled-in providers. Detection iterates this; adding a forge is one /// new module plus one entry here. Both providers are zero-sized, so the /// `&'static` references cost nothing. @@ -169,10 +165,6 @@ fn provider_for_input(input: &str, repo_override: Option<&str>) -> &'static dyn provider_for_origin(repo_override) } -// --------------------------------------------------------------------------- -// Dispatchers used by the rest of the diff UI -// --------------------------------------------------------------------------- - pub fn fetch_pr_info(input: &str, repo_override: Option<&str>) -> Result { provider_for_input(input, repo_override).fetch_pr_info(input, repo_override) } From 33d5afa941e33068ec9b080d3f5e1f6da639bd13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=ABl=20Kuijper?= Date: Sat, 6 Jun 2026 12:04:16 +0200 Subject: [PATCH 11/11] docs: match Azure prereq line to the existing tool-first list style --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 75d4d122..9ad0d04a 100644 --- a/README.md +++ b/README.md @@ -53,7 +53,7 @@ Before you begin, ensure you have: 2. [fzf](https://github.com/junegunn/fzf) (optional) - Required for `lumen explain --list` command 3. [mdcat](https://github.com/swsnr/mdcat) (optional) - Required for pretty output formatting 4. [GitHub CLI (`gh`)](https://cli.github.com/) (optional) - Required for reviewing GitHub Pull Requests -5. Azure DevOps Pull Requests (optional) - Sign in with the [Azure CLI (`az`)](https://learn.microsoft.com/cli/azure/) via `az login` (simplest), or set a Personal Access Token in `AZURE_DEVOPS_EXT_PAT` (scope: *Code → Read*). The `azure-devops` extension is **not** required. +5. [Azure CLI (`az`)](https://learn.microsoft.com/cli/azure/) (optional) - Required for reviewing Azure DevOps Pull Requests (sign in with `az login`, or set `AZURE_DEVOPS_EXT_PAT`) ### Installation