diff --git a/TODO.md b/TODO.md index 242b14d..1b5af91 100644 --- a/TODO.md +++ b/TODO.md @@ -1,5 +1,8 @@ - [x] improve: pull requests - add a tab for drafts so PRs in this state don't populate the "open" tab. -- [ ] improve: pull requests - add a checkmark left of the PR entry in the PR list if the user in question (me) has already approved it. +- [x] improve: pull requests - add a checkmark left of the PR entry in the PR list if the user in question (me) has already approved it. +- [ ] improve: pull requests - add a `🙏` emoji left of the PR entry in the PR list if the user in question (me) has been requested as reviewer. + - also change the checkmark displayed for PRs approved by me to the `👍` emoji. + - make the symbols displayed for these states configurable. - [ ] chore: check to make sure we're mocking everything in tests, not pinging/fetching real data from real websites/APIs. - [ ] 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. diff --git a/docs/editing.md b/docs/editing.md index 147f868..6cfea33 100644 --- a/docs/editing.md +++ b/docs/editing.md @@ -408,6 +408,10 @@ The selector initially lists local revisions and known tracking refs **without c 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. +A **✓ immediately before `#number`** marks your active approval, separately from the `>` selection arrow. “You” means the account authenticated to that forge's API—not the repository's Git author or SSH identity. Other users' approvals do not count. A later request for changes or dismissal removes the mark; comment-only and pending reviews do not replace an existing approval. The mark is not the PR's overall approval decision or a guarantee that you reviewed its current head; the forge controls approval resets after new commits. + +Approval reads reuse the same API credentials as listing. GitHub filters published [reviews by author and decision state](https://docs.github.com/en/graphql/reference/pulls#pullrequest); GitLab filters merge requests by the [authenticated approver](https://docs.gitlab.com/api/graphql/reference/). Both batch approval data with size metadata after resolving the authenticated account. Gitea needs [paginated review reads per PR](https://docs.gitea.com/api/operations/repo-list-pull-reviews/), so its pages can take longer to load. Anonymous, denied, missing, or failed approval lookups leave the mark blank without preventing browsing; a blank is not proof that you have not approved. Reopen the picker to fetch current approval state. No approval or other remote write is performed. + 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 a9aaf17..6bceb08 100644 --- a/src/forge.rs +++ b/src/forge.rs @@ -14,9 +14,15 @@ pub(crate) struct PullRequestSummary { pub description: String, pub closed: bool, pub draft: bool, + pub approved: bool, pub created_at: String, } +struct PullRequestMetadata { + size: Option<(u64, u64)>, + approved: bool, +} + pub(crate) struct PullRequestPage { pub entries: Vec, pub has_more: bool, @@ -413,7 +419,7 @@ impl Forge { .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); + let metadata = self.metadata_with(values, &mut request); for value in values { let id = number( value, @@ -423,7 +429,9 @@ impl Forge { "number" }, )?; - let entry = summary(self.provider, value, sizes.get(&id).copied())?; + let details = metadata.get(&id); + let mut entry = summary(self.provider, value, details.and_then(|d| d.size))?; + entry.approved = details.is_some_and(|d| d.approved); if entry.closed != closed { return Err("Forge API returned a pull request in the wrong category".into()); } @@ -441,29 +449,68 @@ impl Forge { Ok(PullRequestPage { entries, has_more }) } - /// One statistics query per page, never one detail request per pull request. - fn sizes_with( + /// Batch GitHub/GitLab metadata; Gitea exposes reviews only per pull request. + fn metadata_with( &self, values: &[Value], request: &mut impl FnMut(&str, Option<&str>) -> Result, - ) -> BTreeMap { + ) -> BTreeMap { if values.is_empty() { return BTreeMap::new(); } + let api = self + .endpoint + .split_once(if self.provider == Provider::Gitlab { + "/projects/" + } else { + "/repos/" + }) + .unwrap() + .0; + let viewer = request(&format!("{api}/user"), None).ok(); + let username = viewer.as_ref().and_then(|viewer| { + viewer + .get(if self.provider == Provider::Gitlab { + "username" + } else { + "login" + }) + .and_then(Value::as_str) + .filter(|name| !name.is_empty()) + }); if self.provider == Provider::Gitea { + let viewer = viewer.as_ref().and_then(|viewer| number(viewer, "id").ok()); return values .iter() .filter_map(|value| { + let id = number(value, "number").ok()?; Some(( - value.get("number")?.as_u64()?, - ( - value.get("additions")?.as_u64()?, - value.get("deletions")?.as_u64()?, - ), + id, + PullRequestMetadata { + size: value + .get("additions") + .and_then(Value::as_u64) + .zip(value.get("deletions").and_then(Value::as_u64)), + // ponytail: Gitea has no batch reviews API; per-PR reads until it does. + approved: viewer.is_some_and(|viewer| { + self.gitea_approved_with(id, viewer, request) + .unwrap_or(false) + }), + }, )) }) .collect(); } + let reviews = if self.provider == Provider::Github { + username.map_or_else(String::new, |username| { + format!( + "reviews(author:{},states:[APPROVED,CHANGES_REQUESTED,DISMISSED],last:1){{nodes{{state author{{login}}}}}}", + serde_json::to_string(username).unwrap() + ) + }) + } else { + String::new() + }; let mut fields = String::new(); for value in values { let Some(id) = value @@ -480,7 +527,7 @@ impl Forge { match self.provider { Provider::Github => write!( fields, - "p{id}:pullRequest(number:{id}){{additions deletions}}" + "p{id}:pullRequest(number:{id}){{additions deletions {reviews}}}" ) .unwrap(), Provider::Gitlab => write!( @@ -491,6 +538,30 @@ impl Forge { Provider::Gitea => unreachable!(), } } + let approval_args = if self.provider == Provider::Gitlab { + username.map(|username| { + let ids: Vec<_> = values + .iter() + .filter_map(|value| value.get("iid")?.as_u64().map(|id| id.to_string())) + .collect(); + format!( + "iids:{},approvedBy:[{}],first:{}", + serde_json::to_string(&ids).unwrap(), + serde_json::to_string(username).unwrap(), + ids.len() + ) + }) + } else { + None + }; + if let Some(args) = &approval_args { + use std::fmt::Write; + write!( + fields, + "approved:mergeRequests({args}){{nodes{{iid}} pageInfo{{hasNextPage endCursor}}}}" + ) + .unwrap(); + } 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; @@ -512,7 +583,7 @@ impl Forge { ) }; let body = serde_json::json!({ "query": query }).to_string(); - // Missing/denied statistics must not prevent browsing or imply a zero-size PR. + // Missing/denied metadata must not prevent browsing or imply approval. let Ok(response) = request(&url, Some(&body)) else { return BTreeMap::new(); }; @@ -524,23 +595,125 @@ impl Forge { let Some(entries) = response.pointer(container).and_then(Value::as_object) else { return BTreeMap::new(); }; - entries + let mut metadata: BTreeMap<_, _> = entries .iter() .filter_map(|(alias, value)| { - let value = if self.provider == Provider::Gitlab { - value.get("diffStatsSummary")? + let stats = if self.provider == Provider::Gitlab { + value.get("diffStatsSummary") } else { - value + Some(value) }; + let size = stats.and_then(|stats| { + stats + .get("additions")? + .as_u64() + .zip(stats.get("deletions")?.as_u64()) + }); + let approved = self.provider == Provider::Github + && username.is_some_and(|username| { + value + .pointer("/reviews/nodes") + .and_then(Value::as_array) + .is_some_and(|reviews| { + reviews.len() == 1 + && reviews[0]["state"] == "APPROVED" + && reviews[0] + .pointer("/author/login") + .and_then(Value::as_str) + .is_some_and(|author| author.eq_ignore_ascii_case(username)) + }) + }); Some(( - alias.strip_prefix('p')?.parse().ok()?, - ( - value.get("additions")?.as_u64()?, - value.get("deletions")?.as_u64()?, - ), + alias.strip_prefix('p')?.parse::().ok()?, + PullRequestMetadata { size, approved }, )) }) - .collect() + .collect(); + if let Some(args) = approval_args { + let mut response = response; + let mut cursor = None::; + loop { + let Some(approvals) = response.pointer("/data/project/approved") else { + break; + }; + if let Some(nodes) = approvals.get("nodes").and_then(Value::as_array) { + for node in nodes { + if let Some(details) = node + .get("iid") + .and_then(Value::as_str) + .and_then(|id| id.parse::().ok()) + .and_then(|id| metadata.get_mut(&id)) + { + details.approved = true; + } + } + } + if approvals.pointer("/pageInfo/hasNextPage") != Some(&Value::Bool(true)) { + break; + } + let Some(after) = approvals + .pointer("/pageInfo/endCursor") + .and_then(Value::as_str) + .filter(|after| !after.is_empty() && Some(*after) != cursor.as_deref()) + else { + break; + }; + cursor = Some(after.to_owned()); + let body = serde_json::json!({ + "query": format!( + "query{{project(fullPath:{}){{approved:mergeRequests({args},after:{}){{nodes{{iid}} pageInfo{{hasNextPage endCursor}}}}}}}}", + serde_json::to_string(&self.repository).unwrap(), + serde_json::to_string(after).unwrap() + ) + }).to_string(); + match request(&url, Some(&body)) { + Ok(next) => response = next, + Err(_) => break, + } + } + } + metadata + } + + fn gitea_approved_with( + &self, + id: u64, + viewer: u64, + request: &mut impl FnMut(&str, Option<&str>) -> Result, + ) -> Result { + let mut latest = None::<(u64, bool)>; + for page in 1.. { + let response = request( + &format!("{}/{id}/reviews?limit=50&page={page}", self.endpoint), + None, + )?; + let reviews = response + .as_array() + .ok_or("Forge API returned a non-array review list")?; + if reviews.is_empty() { + return Ok(latest.is_some_and(|(_, approved)| approved)); + } + for review in reviews { + if review.pointer("/user/id").and_then(Value::as_u64) != Some(viewer) { + continue; + } + let state = field(review, "state")?; + match state { + "COMMENT" | "PENDING" | "REQUEST_REVIEW" => continue, + "APPROVED" | "REQUEST_CHANGES" => {} + _ => return Err("Forge API returned invalid review state".into()), + } + let id = number(review, "id")?; + let dismissed = review + .get("dismissed") + .and_then(Value::as_bool) + .ok_or("Forge API returned invalid dismissed flag")?; + if latest.is_none_or(|(previous, _)| id > previous) { + latest = Some((id, state == "APPROVED" && !dismissed)); + } + } + } + unreachable!() } fn resolve_with( @@ -743,6 +916,7 @@ fn summary( }, closed, draft, + approved: false, created_at: created_at(value)?, }) } diff --git a/src/main.rs b/src/main.rs index 9008423..e405529 100644 --- a/src/main.rs +++ b/src/main.rs @@ -107,6 +107,7 @@ Local reviews: :diff.comment anchors exact Visual text, Visual Line rows, or the Snapshot anchors stay immutable; stale comments lose gutter markers; no publishing Pull requests: :pull_requests [REMOTE]; GitHub/GitLab/Gitea Open/Closed/Drafts/All tabs, type to search Enter opens the immutable PR head against its merge base; API auth uses provider tokens or gh/glab + ✓ before the PR number marks your API account's active approval; blank may mean unavailable Resolve jj conflicts: :resolve opens the current working-copy conflicts in a dedicated workspace Editable marker-free Draft vs readonly Alternative; :resolve.history shows inputs/base config.resolve_history_layout = \"columns\" (left/base/right) or \"base_below\" (left/right above base) diff --git a/src/repository.rs b/src/repository.rs index 58fea38..c1a351f 100644 --- a/src/repository.rs +++ b/src/repository.rs @@ -1631,7 +1631,12 @@ impl Repository { remote: remote.clone(), number: pull.number, }, - label: format!("#{} {}", pull.number, pull.title), + label: format!( + "{}#{} {}", + if pull.approved { "✓ " } else { " " }, + pull.number, + pull.title + ), description: pull.description, category: if pull.closed { Category::Closed diff --git a/src/tui.rs b/src/tui.rs index 6f2ea1e..d8465cb 100644 --- a/src/tui.rs +++ b/src/tui.rs @@ -3559,7 +3559,11 @@ fn draw_diff( lines.push(Line::styled(visible_line(&label, 0, width), style)); lines.push(Line::styled( visible_line( - &format!(" {}", display_text(&revision.description)), + &format!( + "{}{}", + if pull_requests { " " } else { " " }, + display_text(&revision.description) + ), 0, width, ), diff --git a/tests/unit/forge.rs b/tests/unit/forge.rs index 26e73b2..a8a9c21 100644 --- a/tests/unit/forge.rs +++ b/tests/unit/forge.rs @@ -7,6 +7,200 @@ fn parse(url: &str, provider: Option<&str>) -> Result { }) } +#[test] +fn github_personal_approvals_use_the_viewer_and_last_decisive_review() { + let forge = parse("https://github.com/owner/repo", None).unwrap(); + let values: Vec<_> = (1..=8).map(|id| json!({"number": id})).collect(); + let metadata = forge.metadata_with(&values, &mut |url, body| { + if url.ends_with("/user") { + assert_eq!(url, "https://api.github.com/user"); + return Ok(json!({"login": "viewer"})); + } + let body: Value = serde_json::from_str(body.unwrap()).unwrap(); + let query = body["query"].as_str().unwrap(); + assert!(query.contains("author:\"viewer\"")); + assert!(query.contains("states:[APPROVED,CHANGES_REQUESTED,DISMISSED],last:1")); + assert!(!query.contains("mutation")); + Ok(json!({"data":{"repository":{ + "p1":{"additions":3,"deletions":4,"reviews":{"nodes":[{"state":"APPROVED","author":{"login":"VIEWER"}}]}}, + "p2":{"reviews":{"nodes":[{"state":"CHANGES_REQUESTED","author":{"login":"viewer"}}]}}, + "p3":{"reviews":{"nodes":[{"state":"DISMISSED","author":{"login":"viewer"}}]}}, + "p4":{"reviews":null}, + "p5":{"reviews":{"nodes":[{"state":"COMMENTED","author":{"login":"viewer"}}]}}, + "p6":{"reviews":{"nodes":[{"state":"APPROVED","author":{"login":"someone-else"}}]}}, + "p7":{"reviews":{"nodes":[{"state":"PENDING","author":{"login":"viewer"}}]}}, + "p8":{"reviews":{"nodes":[{"state":"APPROVED","author":{"login":"viewer"}}]}} + }}})) + }); + assert_eq!( + metadata + .iter() + .filter_map(|(id, details)| details.approved.then_some(*id)) + .collect::>(), + [1, 8] + ); + assert_eq!(metadata[&1].size, Some((3, 4))); + assert_eq!(metadata[&8].size, None); +} + +#[test] +fn gitlab_personal_approvals_filter_by_viewer_and_follow_connection_pages() { + let forge = parse("https://gitlab.com/group/repo", None).unwrap(); + let values = [json!({"iid":7}), json!({"iid":8}), json!({"iid":9})]; + let metadata = forge.metadata_with(&values, &mut |url, body| { + if url.ends_with("/user") { + assert_eq!(url, "https://gitlab.com/api/v4/user"); + return Ok(json!({"username":"viewer"})); + } + let body: Value = serde_json::from_str(body.unwrap()).unwrap(); + let query = body["query"].as_str().unwrap(); + assert!(query.contains("approvedBy:[\"viewer\"]")); + assert!(query.contains("iids:[\"7\",\"8\",\"9\"]")); + assert!(!query.contains("mutation")); + if query.contains("after:\"next-page\"") { + return Ok(json!({"data":{"project":{"approved":{ + "nodes":[{"iid":"8"}], + "pageInfo":{"hasNextPage":false,"endCursor":null} + }}}})); + } + Ok(json!({"data":{"project":{ + "p7":{"diffStatsSummary":{"additions":5,"deletions":6}}, + "p8":{"diffStatsSummary":null}, + "p9":{"diffStatsSummary":{"additions":0,"deletions":0}}, + "approved":{ + "nodes":[{"iid":"7"},{"iid":"999"}], + "pageInfo":{"hasNextPage":true,"endCursor":"next-page"} + } + }}})) + }); + assert!(metadata[&7].approved && metadata[&8].approved); + assert!(!metadata[&9].approved); + assert!(!metadata.contains_key(&999)); + assert_eq!(metadata[&7].size, Some((5, 6))); + assert_eq!(metadata[&8].size, None); +} + +#[test] +fn gitea_personal_approvals_ignore_other_users_and_honor_later_decisions_and_failures() { + let forge = parse("https://codeberg.org/owner/repo", None).unwrap(); + let own = |id, state: &str, dismissed| json!({"id":id,"user":{"id":42},"state":state,"dismissed":dismissed}); + let approved = own(10, "APPROVED", false); + for (pages, expected) in [ + ( + vec![ + Ok(json!([approved.clone()])), + Ok(json!([ + own(30, "COMMENT", false), + own(31, "PENDING", false), + own(32, "REQUEST_REVIEW", false) + ])), + Ok(json!([])), + ], + true, + ), + ( + vec![ + Ok(json!([approved.clone()])), + Ok(json!([own(20, "REQUEST_CHANGES", false)])), + Ok(json!([])), + ], + false, + ), + ( + vec![ + Ok(json!([approved.clone()])), + Ok(json!([own(20, "APPROVED", true)])), + Ok(json!([])), + ], + false, + ), + ( + vec![ + Ok(json!([{"id":10,"user":{"id":7},"state":"APPROVED","dismissed":false}])), + Ok(json!([])), + ], + false, + ), + ( + vec![ + Ok(json!([approved.clone()])), + Err("later review page failed".into()), + ], + false, + ), + ( + vec![Ok(json!([{"id":10,"user":{"id":42},"state":"APPROVED"}]))], + false, + ), + // Select by review identity, not response order; a later approval replaces a rejection. + ( + vec![ + Ok(json!([ + own(20, "APPROVED", false), + own(10, "REQUEST_CHANGES", false) + ])), + Ok(json!([])), + ], + true, + ), + ] { + let mut pages = pages.into_iter(); + let metadata = forge.metadata_with( + &[json!({"number":7,"additions":1,"deletions":2})], + &mut |url, body| { + assert!(body.is_none()); + if url.ends_with("/user") { + assert_eq!(url, "https://codeberg.org/api/v1/user"); + return Ok(json!({"id":42})); + } + assert!(url.starts_with( + "https://codeberg.org/api/v1/repos/owner/repo/pulls/7/reviews?limit=50&page=" + )); + pages.next().expect("unexpected review page") + }, + ); + assert_eq!(metadata[&7].approved, expected); + assert_eq!(metadata[&7].size, Some((1, 2))); + } +} + +#[test] +fn unavailable_viewer_or_metadata_never_fabricates_personal_approval() { + for (url, provider) in [ + ("https://github.com/owner/repo", Provider::Github), + ("https://gitlab.com/group/repo", Provider::Gitlab), + ("https://codeberg.org/owner/repo", Provider::Gitea), + ] { + let forge = parse(url, None).unwrap(); + for unavailable_viewer in [true, false] { + let metadata = forge.metadata_with( + &[json!({"number":7,"iid":7,"additions":0,"deletions":0})], + &mut |url, body| { + if url.ends_with("/user") { + return if unavailable_viewer { + Err("no API credentials".into()) + } else { + Ok(json!({"login":"viewer","username":"viewer","id":42})) + }; + } + if provider == Provider::Gitea { + assert!( + !unavailable_viewer, + "anonymous listings must not fetch reviews" + ); + assert!(body.is_none()); + } + Err("metadata denied".into()) + }, + ); + assert!(metadata.values().all(|details| !details.approved)); + if provider == Provider::Gitea { + assert_eq!(metadata[&7].size, Some((0, 0))); + } + } + } +} + #[test] fn draft_summaries_keep_open_closed_and_merged_states_distinct() { for provider in [Provider::Github, Provider::Gitlab, Provider::Gitea] { @@ -62,6 +256,9 @@ fn pull_request_sizes_include_zero_and_preserve_unknown_statistics() { .unwrap(); let page = forge .page_with(false, 1, |url, body| { + if url.ends_with("/user") { + return Err("unauthenticated fixture".into()); + } if let Some(body) = body { let query: Value = serde_json::from_str(body).unwrap(); let query = query["query"].as_str().unwrap(); @@ -140,6 +337,9 @@ fn pages_are_bounded_newest_first_summaries_without_historical_metadata() { ] { let forge = parse(url, None).unwrap(); let page = forge.page_with(false, 3, |url, body| { + if url.ends_with("/user") { + return Err("unauthenticated fixture".into()); + } if body.is_some() { return Err("statistics unavailable".into()); } @@ -179,6 +379,9 @@ fn gitlab_closed_combines_closed_and_merged_pages_without_open_requests() { let mut states = Vec::new(); let result = forge .page_with(true, page, |url, body| { + if url.ends_with("/user") { + return Err("unauthenticated fixture".into()); + } if body.is_some() { return Err("statistics unavailable".into()); } @@ -215,6 +418,9 @@ 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, body| { + if url.ends_with("/user") { + return Err("unauthenticated fixture".into()); + } if body.is_some() { return Err("statistics unavailable".into()); }