From cce3b1e46929a3a921c16e5f96dd6a521d53c2d9 Mon Sep 17 00:00:00 2001 From: Valdemar Lorenzen Date: Wed, 21 Oct 2026 10:06:48 +0200 Subject: [PATCH] feat: show line counts in pull request picker --- TODO.md | 1 + docs/editing.md | 2 + src/forge.rs | 154 ++++++++++++++++++++++++++++++++++++++++---- tests/unit/forge.rs | 99 +++++++++++++++++++++++++--- 4 files changed, 234 insertions(+), 22 deletions(-) diff --git a/TODO.md b/TODO.md index 5f58303..7ca680f 100644 --- a/TODO.md +++ b/TODO.md @@ -1,3 +1,4 @@ +- [ ] improve: pull requests - add a tab for drafts so PRs in this state don't populate the "open" tab. - [ ] feature: diff view - GitHub code review integration after the local review UX is usable. - publish local reviews explicitly: general PR comments, inline line/range comments, review summaries, approval, and requests for changes. - preserve explicit PR identity and reviewed commit IDs; validate anchors against GitHub's PR diff and recheck the PR head before submission. diff --git a/docs/editing.md b/docs/editing.md index 7db1f6c..5677688 100644 --- a/docs/editing.md +++ b/docs/editing.md @@ -406,6 +406,8 @@ The selector initially lists local revisions and known tracking refs **without c **Pull requests:** run **`:pull_requests [remote]`**, use the palette's `pull_requests` action, or bind **`config.core.pull_requests`**. The floating picker has **Open / Closed / All** tabs; Closed includes merged requests. Tab/Shift+Tab changes category, typing fuzzy-searches its number, title, author, state, and branches, and Enter immediately opens the selected request in **Branch changes** mode using its immutable head and merge base. Escape restores the source buffer. A failed list can be retried with Enter. The default remote is `origin`, or the sole configured remote; specify a name when there are multiple remotes without `origin`. +Each pull request's metadata shows added/deleted line counts after its state, before the branch names: `author · open · +123 -45 · topic → main`. Zero counts are shown explicitly; missing, denied, or failed statistics show `size unavailable` without preventing selection. Counts come from one batched read-only statistics query per nonempty GitHub/GitLab page ([GitHub](https://docs.github.com/en/graphql/reference/pulls), [GitLab](https://docs.gitlab.com/api/graphql/reference/#diffstatssummary)); [Gitea](https://docs.gitea.com/api/operations/repo-list-pull-requests/) supplies them directly in its list response. GitHub statistics require API authentication even for public repositories. Listing does not fetch source diffs. + GitHub, GitLab merge requests, and Gitea are supported. API reads require `curl`; public repositories work anonymously. Authentication uses `GH_TOKEN`/`GITHUB_TOKEN` for github.com, `GH_ENTERPRISE_TOKEN`/`GITHUB_ENTERPRISE_TOKEN` for GitHub Enterprise, `GITLAB_TOKEN` for GitLab, and `GITEA_TOKEN` for Gitea. With no matching environment token, GitHub/GitLab reuse `gh auth token --hostname HOST` / `glab auth token --hostname HOST` when available. Tokens are supplied to curl on stdin, not in subprocess arguments, and sent only to the HTTPS API; redirects are not followed. Git object fetching uses the configured remote's existing Git/SSH credentials separately. Standard GitHub/GitLab/Gitea hosts and Codeberg are detected from HTTPS or SSH remote URLs. For another host, set `remote..vamp-forge` to `github`, `gitlab`, or `gitea` in the repository's Git configuration, for example `git config remote.origin.vamp-forge gitea`. For a non-colocated jj repository, use `git --git-dir .jj/repo/store/git config remote.origin.vamp-forge gitea`. diff --git a/src/forge.rs b/src/forge.rs index 3ad9d52..28d3170 100644 --- a/src/forge.rs +++ b/src/forge.rs @@ -42,6 +42,7 @@ struct Forge { provider: Provider, host: String, endpoint: String, + repository: String, } fn encode(text: &str) -> String { @@ -237,6 +238,7 @@ impl Forge { provider, host: authority, endpoint, + repository: path.to_owned(), }) } @@ -308,14 +310,18 @@ impl Forge { .transpose() } - fn get( + fn request( &self, url: &str, + body: Option<&str>, token: Option<&str>, root: &Path, cancelled: &AtomicBool, ) -> Result { let mut headers = String::from("Accept: application/json\n"); + if body.is_some() { + headers.push_str("Content-Type: application/json\n"); + } if let Some(token) = token { let prefix = match self.provider { Provider::Github => "Authorization: Bearer", @@ -347,6 +353,9 @@ impl Forge { "--url", url, ]); + if let Some(body) = body { + command.args(["--data-binary", body]); + } let bytes = checked(run(command, headers.into_bytes(), cancelled)?).map_err( |_| "Forge API request failed; check curl, network access, and authentication", )?; @@ -374,7 +383,7 @@ impl Forge { &self, closed: bool, page: u32, - mut get: impl FnMut(&str) -> Result, + mut request: impl FnMut(&str, Option<&str>) -> Result, ) -> Result { if page == 0 { return Err("Forge page numbers start at 1".into()); @@ -394,17 +403,26 @@ impl Forge { // Gitea's default sort is descending creation time; "oldest" reverses it. Provider::Gitea => "limit=50", }; - let values = get(&format!( - "{}?state={state}&{query}&page={page}", - self.endpoint - ))?; + let values = request( + &format!("{}?state={state}&{query}&page={page}", self.endpoint), + None, + )?; let values = values .as_array() .ok_or("Forge API returned a non-array pull request list")?; // Servers may cap page sizes. Only an empty page proves exhaustion. has_more |= !values.is_empty(); + let sizes = self.sizes_with(values, &mut request); for value in values { - let entry = summary(self.provider, value)?; + let id = number( + value, + if self.provider == Provider::Gitlab { + "iid" + } else { + "number" + }, + )?; + let entry = summary(self.provider, value, sizes.get(&id).copied())?; if entry.closed != closed { return Err("Forge API returned a pull request in the wrong category".into()); } @@ -422,6 +440,108 @@ impl Forge { Ok(PullRequestPage { entries, has_more }) } + /// One statistics query per page, never one detail request per pull request. + fn sizes_with( + &self, + values: &[Value], + request: &mut impl FnMut(&str, Option<&str>) -> Result, + ) -> BTreeMap { + if values.is_empty() { + return BTreeMap::new(); + } + if self.provider == Provider::Gitea { + return values + .iter() + .filter_map(|value| { + Some(( + value.get("number")?.as_u64()?, + ( + value.get("additions")?.as_u64()?, + value.get("deletions")?.as_u64()?, + ), + )) + }) + .collect(); + } + let mut fields = String::new(); + for value in values { + let Some(id) = value + .get(if self.provider == Provider::Gitlab { + "iid" + } else { + "number" + }) + .and_then(Value::as_u64) + else { + continue; + }; + use std::fmt::Write; + match self.provider { + Provider::Github => write!( + fields, + "p{id}:pullRequest(number:{id}){{additions deletions}}" + ) + .unwrap(), + Provider::Gitlab => write!( + fields, + "p{id}:mergeRequest(iid:\"{id}\"){{diffStatsSummary{{additions deletions}}}}" + ) + .unwrap(), + Provider::Gitea => unreachable!(), + } + } + let (url, query) = if self.provider == Provider::Github { + let (owner, name) = self.repository.split_once('/').unwrap(); + let api = self.endpoint.split_once("/repos/").unwrap().0; + ( + format!("{}/graphql", api.strip_suffix("/v3").unwrap_or(api)), + format!( + "query{{repository(owner:{},name:{}){{{fields}}}}}", + serde_json::to_string(owner).unwrap(), + serde_json::to_string(name).unwrap() + ), + ) + } else { + ( + format!("https://{}/api/graphql", self.host), + format!( + "query{{project(fullPath:{}){{{fields}}}}}", + serde_json::to_string(&self.repository).unwrap() + ), + ) + }; + let body = serde_json::json!({ "query": query }).to_string(); + // Missing/denied statistics must not prevent browsing or imply a zero-size PR. + let Ok(response) = request(&url, Some(&body)) else { + return BTreeMap::new(); + }; + let container = if self.provider == Provider::Github { + "/data/repository" + } else { + "/data/project" + }; + let Some(entries) = response.pointer(container).and_then(Value::as_object) else { + return BTreeMap::new(); + }; + entries + .iter() + .filter_map(|(alias, value)| { + let value = if self.provider == Provider::Gitlab { + value.get("diffStatsSummary")? + } else { + value + }; + Some(( + alias.strip_prefix('p')?.parse().ok()?, + ( + value.get("additions")?.as_u64()?, + value.get("deletions")?.as_u64()?, + ), + )) + }) + .collect() + } + fn resolve_with( &self, id: u64, @@ -567,7 +687,11 @@ fn created_at(value: &Value) -> Result { )) } -fn summary(provider: Provider, value: &Value) -> Result { +fn summary( + provider: Provider, + value: &Value, + size: Option<(u64, u64)>, +) -> Result { let gitlab = provider == Provider::Gitlab; let (closed, merged) = status(provider, value)?; let author = author(provider, value)?; @@ -578,6 +702,10 @@ fn summary(provider: Provider, value: &Value) -> Result Result format!("{author} · {state} · {head} → {base}"), - _ => format!("{author} · {state}"), + (Some(base), Some(head)) => format!("{author} · {state} · {size} · {head} → {base}"), + _ => format!("{author} · {state} · {size}"), }, closed, created_at: created_at(value)?, @@ -788,8 +916,8 @@ pub(crate) fn page( ssh_hostname(command, destination, port, cancelled) })?; let token = forge.token(root, cancelled)?; - forge.page_with(closed, page, |url| { - forge.get(url, token.as_deref(), root, cancelled) + forge.page_with(closed, page, |url, body| { + forge.request(url, body, token.as_deref(), root, cancelled) }) } @@ -807,7 +935,7 @@ pub(crate) fn resolve( })?; let token = forge.token(root, cancelled)?; forge.resolve_with(number, |url| { - forge.get(url, token.as_deref(), root, cancelled) + forge.request(url, None, token.as_deref(), root, cancelled) }) } diff --git a/tests/unit/forge.rs b/tests/unit/forge.rs index 99f3bcd..feb8611 100644 --- a/tests/unit/forge.rs +++ b/tests/unit/forge.rs @@ -7,6 +7,82 @@ fn parse(url: &str, provider: Option<&str>) -> Result { }) } +#[test] +fn pull_request_sizes_include_zero_and_preserve_unknown_statistics() { + for (url, provider) in [ + ("https://github.com/owner/repo", Provider::Github), + ("https://github.enterprise/owner/repo", Provider::Github), + ("https://gitlab.com/group/repo", Provider::Gitlab), + ("https://codeberg.org/owner/repo", Provider::Gitea), + ] { + let forge = parse( + url, + Some(match provider { + Provider::Github => "github", + Provider::Gitlab => "gitlab", + Provider::Gitea => "gitea", + }), + ) + .unwrap(); + let page = forge + .page_with(false, 1, |url, body| { + if let Some(body) = body { + let query: Value = serde_json::from_str(body).unwrap(); + let query = query["query"].as_str().unwrap(); + assert!(query.starts_with("query{")); + assert!(!query.contains("mutation")); + if provider == Provider::Github { + assert_eq!( + url, + if forge.host == "github.com" { + "https://api.github.com/graphql" + } else { + "https://github.enterprise/api/graphql" + } + ); + assert!(query.contains("p7:pullRequest(number:7)")); + return Ok(json!({"data":{"repository":{ + "p7":{"additions":123,"deletions":45}, + "p8":{"additions":0,"deletions":0}, + "p9":null + }}})); + } + assert_eq!(url, "https://gitlab.com/api/graphql"); + assert!(query.contains("p7:mergeRequest(iid:\"7\")")); + return Ok(json!({"data":{"project":{ + "p7":{"diffStatsSummary":{"additions":123,"deletions":45}}, + "p8":{"diffStatsSummary":{"additions":0,"deletions":0}}, + "p9":{"diffStatsSummary":null} + }}})); + } + Ok(Value::Array((7..=9).map(|id| json!({ + "merged_at":null, "merged":false, + "number":id, "iid":id, "title":"Sized PR", + "state":if provider == Provider::Gitlab { "opened" } else { "open" }, + "user":{"login":"author"}, "author":{"username":"author"}, + "created_at":"2024-01-01T00:00:00Z", + "base":{"ref":"main"}, "head":{"ref":"topic"}, + "target_branch":"main", "source_branch":"topic", + "additions":match id { 7 => Some(123), 8 => Some(0), _ => None }, + "deletions":match id { 7 => Some(45), 8 => Some(0), _ => None } + })).collect())) + }) + .unwrap(); + let description = |id| { + &page + .entries + .iter() + .find(|entry| entry.number == id) + .unwrap() + .description + }; + assert!(description(7).contains("open · +123 -45 · topic → main")); + assert!(description(8).contains("open · +0 -0 · topic → main")); + assert!(description(9).contains("size unavailable")); + assert!(!description(9).contains("+0 -0")); + } +} + #[test] fn pages_are_bounded_newest_first_summaries_without_historical_metadata() { for (provider, url, query) in [ @@ -27,9 +103,10 @@ fn pages_are_bounded_newest_first_summaries_without_historical_metadata() { ), ] { let forge = parse(url, None).unwrap(); - let mut calls = 0; - let page = forge.page_with(false, 3, |url| { - calls += 1; + let page = forge.page_with(false, 3, |url, body| { + if body.is_some() { + return Err("statistics unavailable".into()); + } assert_eq!(url, format!("{}?{query}", forge.endpoint)); Ok(json!([ {"number": 1, "iid": 1, "title": "Older", "state": if provider == Provider::Gitlab { "opened" } else { "open" }, @@ -39,22 +116,20 @@ fn pages_are_bounded_newest_first_summaries_without_historical_metadata() { "created_at": "2024-01-01T00:00:00.001Z", "diff_refs": null, "head": {"sha": "not-a-commit"}} ])) }).unwrap(); - assert_eq!(calls, 1); assert_eq!( page.entries.iter().map(|p| p.number).collect::>(), [2, 1] ); - assert_eq!(page.entries[0].description, "author · open"); assert!(page.entries[1].description.contains("deleted user")); assert!( page.has_more, "A short, server-capped page is not exhaustion" ); - let empty = forge.page_with(false, 4, |_| Ok(json!([]))).unwrap(); + let empty = forge.page_with(false, 4, |_, _| Ok(json!([]))).unwrap(); assert!(empty.entries.is_empty() && !empty.has_more); assert!( forge - .page_with(false, 0, |_| panic!("Page zero must not request")) + .page_with(false, 0, |_, _| panic!("Page zero must not request")) .is_err() ); } @@ -67,7 +142,10 @@ fn gitlab_closed_combines_closed_and_merged_pages_without_open_requests() { for page in 1..=3 { let mut states = Vec::new(); let result = forge - .page_with(true, page, |url| { + .page_with(true, page, |url, body| { + if body.is_some() { + return Err("statistics unavailable".into()); + } assert!(url.ends_with(&format!("&page={page}"))); let state = if url.contains("?state=closed&") { "closed" @@ -100,7 +178,10 @@ fn gitlab_closed_combines_closed_and_merged_pages_without_open_requests() { } all.sort_unstable_by(|a, b| b.created_at.cmp(&a.created_at)); assert_eq!(all.iter().map(|p| p.number).collect::>(), [3, 2, 1]); - let raced = forge.page_with(true, 1, |url| { + let raced = forge.page_with(true, 1, |url, body| { + if body.is_some() { + return Err("statistics unavailable".into()); + } Ok(json!([{"iid": 7, "title": "Raced merge", "state": if url.contains("?state=merged&") { "merged" } else { "closed" }, "author": null, "created_at": "2024-01-01T00:00:00Z"}])) }).unwrap();