From 219d85a15d5f964f4df6d43ea96b0c8eccb862da Mon Sep 17 00:00:00 2001 From: damocles Date: Sun, 1 Nov 2026 01:54:55 +0100 Subject: [PATCH] forge cli: show comment ids and resolved state on inline review comments, --only unresolved, and list them in pr timeline --- docs/tools/forge-cli.md | 6 + docs/tools/forge.md | 4 +- hive-forge/src/verbs/pr_reviews.rs | 252 ++++++++++++++++++++++++----- hive-forge/src/verbs/timeline.rs | 117 +++++++++++++- 4 files changed, 335 insertions(+), 44 deletions(-) diff --git a/docs/tools/forge-cli.md b/docs/tools/forge-cli.md index 8e6610f6..114c146e 100644 --- a/docs/tools/forge-cli.md +++ b/docs/tools/forge-cli.md @@ -607,6 +607,12 @@ List a PR's reviews, or submit one * `--request-changes` — Request changes on the PR (submit a `REQUEST_CHANGES` review) * `--comment` — Leave a comment review (submit a COMMENT review) * `-m`, `--body ` — Optional body / message for the review (used with --approve, --request-changes, or --comment) +* `--only ` — When listing: narrow the inline comments shown (and leave out the reviews with none left). `unresolved` = still open on the current head, that is neither resolved nor outdated: what a re-review has to check + + Possible values: + - `unresolved`: + Neither resolved nor outdated + diff --git a/docs/tools/forge.md b/docs/tools/forge.md index ef4f71b6..26a6dec7 100644 --- a/docs/tools/forge.md +++ b/docs/tools/forge.md @@ -77,7 +77,9 @@ hive-forge pr edit 42 --base main # retarget a stacked PR onto main hive-forge pr 42 # PR metadata as JSON hive-forge pr create --title "..." --head my-branch --push # also `git push forge my-branch` hive-forge pr create --title "..." --head my-branch --label area/ops # repeatable, same as issue create -hive-forge pr reviews 42 # list reviews; inline comments included per review +hive-forge pr reviews 42 # list reviews; each inline comment as `[#id path:line] text`, tagged (resolved by X) / (outdated) +hive-forge pr reviews 42 --only unresolved # only inline comments still open on the current head +hive-forge pr timeline 42 # review rows list their line comments (id, path:line, state) too hive-forge pr reviews 42 --approve # submit APPROVED review hive-forge pr reviews 42 --request-changes -m "msg" # submit REQUEST_CHANGES review hive-forge pr reviews 42 --comment -m "msg" # submit COMMENT review diff --git a/hive-forge/src/verbs/pr_reviews.rs b/hive-forge/src/verbs/pr_reviews.rs index b00b4ffb..859727cb 100644 --- a/hive-forge/src/verbs/pr_reviews.rs +++ b/hive-forge/src/verbs/pr_reviews.rs @@ -1,6 +1,8 @@ //! `pr reviews ` — list reviews, or submit one via `--approve` / //! `--request-changes` / `--comment`. +use std::fmt::Write as _; + use anyhow::{Result, bail}; use clap::Args as ClapArgs; use forgejo_api::structs::CreatePullReviewOptions; @@ -30,6 +32,20 @@ pub struct Args { /// --request-changes, or --comment). #[arg(long, short = 'm')] body: Option, + + /// When listing: narrow the inline comments shown (and leave out the + /// reviews with none left). `unresolved` = still open on the current + /// head, that is neither resolved nor outdated: what a re-review has to + /// check. + #[arg(long, value_enum, conflicts_with_all = ["approve", "request_changes", "comment"])] + only: Option, +} + +/// Which inline comments `--only` keeps. +#[derive(Clone, Copy, PartialEq, Eq, clap::ValueEnum)] +enum Only { + /// Neither resolved nor outdated. + Unresolved, } pub fn run(client: &Client, args: Args) -> Result<()> { @@ -49,7 +65,7 @@ pub fn run(client: &Client, args: Args) -> Result<()> { if args.body.is_some() { bail!("--body requires one of --approve / --request-changes / --comment"); } - list_reviews(client, args.number) + list_reviews(client, args.number, args.only) } } @@ -77,7 +93,7 @@ fn submit_review(client: &Client, number: u64, event: &str, body: Option /// Fetch inline diff comments for a single review, serialized back to /// the API's JSON shape. Returns an empty vec on any error (missing /// review, network failure) so callers can degrade gracefully. -fn fetch_inline_comments(client: &Client, pr: u64, review_id: i64) -> Vec { +pub(crate) fn fetch_inline_comments(client: &Client, pr: u64, review_id: i64) -> Vec { let Ok((owner, name)) = client.owner_repo() else { return Vec::new(); }; @@ -95,7 +111,7 @@ fn fetch_inline_comments(client: &Client, pr: u64, review_id: i64) -> Vec } /// List all reviews for a PR, dispatching to the appropriate output mode. -fn list_reviews(client: &Client, number: u64) -> Result<()> { +fn list_reviews(client: &Client, number: u64, only: Option) -> Result<()> { let (owner, name) = client.owner_repo()?; let idx = index(number)?; let reviews = all_pages(|page, size| { @@ -110,35 +126,114 @@ fn list_reviews(client: &Client, number: u64) -> Result<()> { let reviews = serde_json::to_value(reviews)?; let reviews = reviews.as_array().cloned().unwrap_or_default(); if client.json_mode() { - list_reviews_json(client, number, &reviews) + list_reviews_json(client, number, &reviews, only) } else { - list_reviews_text(client, number, &reviews); + list_reviews_text(client, number, &reviews, only); Ok(()) } } +/// Where an inline comment sits: `path:line`, or the bare `path` when +/// Forgejo dropped the position because the line left the diff. Control +/// characters in the path become spaces so one cannot shape the output. +pub(crate) fn comment_location(c: &Value) -> String { + let path: String = c + .get("path") + .and_then(Value::as_str) + .unwrap_or("?") + .chars() + .map(|ch| if ch.is_control() { ' ' } else { ch }) + .collect(); + match c.get("position").and_then(Value::as_u64).filter(|&l| l > 0) { + Some(line) => format!("{path}:{line}"), + None => path, + } +} + +/// An inline comment whose anchored line is no longer in the diff (a push +/// moved it): Forgejo drops `position` then. +fn is_outdated(c: &Value) -> bool { + c.get("position") + .and_then(Value::as_u64) + .is_none_or(|l| l == 0) +} + +/// Login of whoever resolved the conversation, if anyone did. +fn resolved_by(c: &Value) -> Option<&str> { + c.get("resolver") + .and_then(|r| r.get("login")) + .and_then(Value::as_str) +} + +/// Still needs an answer on the current head: neither resolved nor outdated. +fn is_open(c: &Value) -> bool { + resolved_by(c).is_none() && !is_outdated(c) +} + +/// One inline comment as text: `[#id path:line] body`, continuation lines +/// indented, `(resolved by X)` / `(outdated)` after the body. Shared by +/// `pr reviews` and `pr timeline`. +pub(crate) fn render_inline_comment(c: &Value) -> String { + let id = c.get("id").and_then(Value::as_i64).unwrap_or(0); + let body = c + .get("body") + .and_then(Value::as_str) + .unwrap_or("") + .trim() + .replace('\n', "\n "); + let mut out = format!("[#{id} {}] {body}", comment_location(c)); + if let Some(who) = resolved_by(c) { + write!(out, " (resolved by {who})").ok(); + } + if is_outdated(c) { + out.push_str(" (outdated)"); + } + out +} + +/// The comments to show for one review: all of them, or with `--only` just +/// the matching ones. +fn visible(comments: Vec, only: Option) -> Vec { + match only { + Some(Only::Unresolved) => comments.into_iter().filter(is_open).collect(), + None => comments, + } +} + /// JSON output: one object per review, with an inline `comments` array. -fn list_reviews_json(client: &Client, number: u64, reviews: &[Value]) -> Result<()> { +/// With `--only`, only reviews that still have an open comment. +fn list_reviews_json( + client: &Client, + number: u64, + reviews: &[Value], + only: Option, +) -> Result<()> { let trimmed: Vec = reviews .iter() - .map(|r| { + .filter_map(|r| { let id = r.get("id").and_then(Value::as_i64).unwrap_or(0); - let inline: Vec = if id > 0 { - fetch_inline_comments(client, number, id) - .iter() - .map(|c| { - json!({ - "id": c.get("id"), - "path": c.get("path"), - "line": c.get("position"), - "body": c.get("body"), - }) - }) - .collect() + let inline = if id > 0 { + visible(fetch_inline_comments(client, number, id), only) } else { vec![] }; - json!({ + if only.is_some() && inline.is_empty() { + return None; + } + let inline: Vec = inline + .iter() + .map(|c| { + json!({ + "id": c.get("id"), + "path": c.get("path"), + "line": c.get("position"), + "body": c.get("body"), + "resolved_by": resolved_by(c), + "outdated": is_outdated(c), + }) + }) + .collect(); + Some(json!({ "id": r.get("id"), "state": r.get("state"), // forgejo's staleness bits: `stale` = head moved since the @@ -152,21 +247,28 @@ fn list_reviews_json(client: &Client, number: u64, reviews: &[Value]) -> Result< "body": r.get("body"), "comments_count": r.get("comments_count"), "comments": inline, - }) + })) }) .collect(); print_json(&Value::Array(trimmed)) } /// Human-readable output: Markdown-style heading per review, inline -/// comments as `[path:line] body` (line omitted for PR-level comments). -fn list_reviews_text(client: &Client, number: u64, reviews: &[Value]) { - if reviews.is_empty() { - println!("(no reviews)"); - return; - } +/// comments via [`render_inline_comment`]. With `--only`, only reviews +/// that still have an open comment, and only those comments. +fn list_reviews_text(client: &Client, number: u64, reviews: &[Value], only: Option) { + let mut shown = false; for r in reviews { let id = r.get("id").and_then(Value::as_i64).unwrap_or(0); + let inline = if id > 0 { + visible(fetch_inline_comments(client, number, id), only) + } else { + vec![] + }; + if only.is_some() && inline.is_empty() { + continue; + } + shown = true; let user = r .get("user") .and_then(|u| u.get("login")) @@ -186,23 +288,91 @@ fn list_reviews_text(client: &Client, number: u64, reviews: &[Value]) { "" }; println!("### review by {user} ({state}){flag}"); - if !body.is_empty() { + if !body.is_empty() && only.is_none() { println!("{body}"); } - if id > 0 { - for c in &fetch_inline_comments(client, number, id) { - let path = c.get("path").and_then(Value::as_str).unwrap_or("?"); - let cbody = c.get("body").and_then(Value::as_str).unwrap_or("").trim(); - // PR-level comments have no position; omit `:line` when - // absent (also true for a comment whose anchored line has - // since fallen out of the diff — forgejo drops `position` - // in that case too, same display fallback). - match c.get("position").and_then(Value::as_u64) { - Some(line) => println!(" [{path}:{line}] {cbody}"), - None => println!(" [{path}] {cbody}"), - } - } + for c in &inline { + println!(" {}", render_inline_comment(c)); } println!(); } + if !shown { + println!( + "{}", + if only.is_some() { + "(no matching inline comments)" + } else { + "(no reviews)" + } + ); + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn comment(id: i64, position: u64, resolver: Option<&str>) -> Value { + json!({ + "id": id, + "path": "src/a.rs", + "position": position, + "body": "off by one", + "resolver": resolver.map(|login| json!({"login": login})), + }) + } + + #[test] + fn location_is_path_line_or_bare_path() { + assert_eq!(comment_location(&comment(1, 42, None)), "src/a.rs:42"); + assert_eq!(comment_location(&comment(1, 0, None)), "src/a.rs"); + } + + #[test] + fn location_neutralises_control_characters() { + let c = json!({"path": "a\nb\tc.rs", "position": 3}); + assert_eq!(comment_location(&c), "a b c.rs:3"); + } + + #[test] + fn open_means_neither_resolved_nor_outdated() { + assert!(is_open(&comment(1, 42, None))); + assert!(!is_open(&comment(1, 42, Some("mara")))); + assert!(!is_open(&comment(1, 0, None))); + } + + #[test] + fn unresolved_filter_keeps_only_open_comments() { + let all = vec![ + comment(1, 42, None), + comment(2, 42, Some("mara")), + comment(3, 0, None), + ]; + let ids = + |v: Vec| -> Vec { v.iter().filter_map(|c| c["id"].as_i64()).collect() }; + assert_eq!(ids(visible(all.clone(), Some(Only::Unresolved))), vec![1]); + assert_eq!(ids(visible(all, None)), vec![1, 2, 3]); + } + + #[test] + fn render_shows_id_location_body_and_state_tags() { + assert_eq!( + render_inline_comment(&comment(7, 42, None)), + "[#7 src/a.rs:42] off by one" + ); + assert_eq!( + render_inline_comment(&comment(7, 42, Some("mara"))), + "[#7 src/a.rs:42] off by one (resolved by mara)" + ); + assert_eq!( + render_inline_comment(&comment(7, 0, None)), + "[#7 src/a.rs] off by one (outdated)" + ); + } + + #[test] + fn render_indents_continuation_lines() { + let c = json!({"id": 1, "path": "f.rs", "position": 2, "body": "one\ntwo"}); + assert_eq!(render_inline_comment(&c), "[#1 f.rs:2] one\n two"); + } } diff --git a/hive-forge/src/verbs/timeline.rs b/hive-forge/src/verbs/timeline.rs index 8c65d4c6..ddd2782a 100644 --- a/hive-forge/src/verbs/timeline.rs +++ b/hive-forge/src/verbs/timeline.rs @@ -32,6 +32,7 @@ use serde_json::{Value, json}; use time::format_description::well_known::Rfc3339; use crate::client::{Client, index}; +use crate::verbs::pr_reviews::{fetch_inline_comments, render_inline_comment}; use crate::verbs::{MAX_LIMIT, NullableVec, PAGE_SIZE, clamp_limit, parse_rfc3339, print_json}; #[derive(ClapArgs)] @@ -85,7 +86,10 @@ pub fn run(client: &Client, args: Args) -> Result<()> { }; // Serialize back to the API's JSON shape so the per-type render // arms (and their tests) keep working on plain `Value`s. - let v = serde_json::to_value(&events)?; + let mut v = serde_json::to_value(&events)?; + if let Some(arr) = v.as_array_mut() { + attach_inline_comments(client, args.number, arr); + } if client.json_mode() { return print_json(&json!({ "events": v, @@ -285,6 +289,52 @@ fn fetch_since( Ok((events, more)) } +/// Attach the inline (line) comments to the review events of a PR timeline. +/// +/// The timeline row for a line comment carries no file or line, and the +/// `review` row none of its comments, so both are looked up per review: each +/// `code` row gets its comment under `inline` (path, position, resolver), and +/// a `review` row with no `code` row of its own in this window gets all its +/// comments under `inline_comments`, so they show up whichever way the forge +/// lists them, and never twice. One fetch per distinct review; a failed one +/// leaves the rows as they were. +fn attach_inline_comments(client: &Client, number: u64, events: &mut [Value]) { + let review_id = |ev: &Value| { + ev.get("review_id") + .and_then(Value::as_i64) + .filter(|&id| id > 0) + }; + let kind = |ev: &Value| ev.get("type").and_then(Value::as_str).map(str::to_owned); + let with_code: std::collections::HashSet = events + .iter() + .filter(|ev| kind(ev).as_deref() == Some("code")) + .filter_map(review_id) + .collect(); + let mut fetched: std::collections::HashMap> = std::collections::HashMap::new(); + for ev in events.iter_mut() { + let (Some(rid), Some(k)) = (review_id(ev), kind(ev)) else { + continue; + }; + if k != "code" && k != "review" { + continue; + } + let comments = fetched + .entry(rid) + .or_insert_with(|| fetch_inline_comments(client, number, rid)); + if k == "code" { + let id = ev.get("id").and_then(Value::as_i64); + if let Some(c) = comments + .iter() + .find(|c| c.get("id").and_then(Value::as_i64) == id) + { + ev["inline"] = c.clone(); + } + } else if !with_code.contains(&rid) && !comments.is_empty() { + ev["inline_comments"] = Value::Array(comments.clone()); + } + } +} + /// Render one timeline event as a single `**actor @ ts**: summary` /// line. Comment rows inline their full body; structured event types /// (label, assignees, close, etc.) get a one-line human summary @@ -439,7 +489,24 @@ fn format_event(ev: &Value) -> String { format!("{verb} #{number} ({title})") } "changed_target_branch" => "changed target branch".to_owned(), - "review" => "submitted a review".to_owned(), + "review" => { + let mut out = "submitted a review".to_owned(); + let inline = ev.get("inline_comments").and_then(Value::as_array); + for c in inline.into_iter().flatten() { + out.push_str("\n "); + out.push_str(&render_inline_comment(c)); + } + out + } + // A line comment: the comment text, led by where it sits and its + // id when `attach_inline_comments` could look that up. + "code" => { + let body = ev.get("body").and_then(Value::as_str).unwrap_or(""); + match ev.get("inline") { + Some(c) => format!("commented on a line\n {}", render_inline_comment(c)), + None => format!("commented on a line: {body}"), + } + } "lock" => "locked the conversation".to_owned(), "unlock" => "unlocked the conversation".to_owned(), // Future / unknown types: surface the raw label so we don't @@ -699,4 +766,50 @@ mod tests { }); assert_eq!(format_event(&ev), "**? @ 2026-05-31T12:00:00Z**: closed"); } + + #[test] + fn code_event_without_lookup_shows_the_bare_body() { + let ev = serde_json::json!({ + "type": "code", "user": {"login": "argus"}, + "created_at": "2026-11-01T01:00:00Z", "body": "off by one", + }); + assert_eq!( + format_event(&ev), + "**argus @ 2026-11-01T01:00:00Z**: commented on a line: off by one" + ); + } + + #[test] + fn code_event_with_lookup_shows_id_location_and_state() { + let ev = serde_json::json!({ + "type": "code", "user": {"login": "argus"}, + "created_at": "2026-11-01T01:00:00Z", "body": "off by one", + "inline": {"id": 9, "path": "src/a.rs", "position": 42, "body": "off by one", + "resolver": {"login": "mara"}}, + }); + assert_eq!( + format_event(&ev), + "**argus @ 2026-11-01T01:00:00Z**: commented on a line\n [#9 src/a.rs:42] off by one (resolved by mara)" + ); + } + + #[test] + fn review_event_lists_its_inline_comments() { + let ev = serde_json::json!({ + "type": "review", "user": {"login": "argus"}, + "created_at": "2026-11-01T01:00:00Z", + "inline_comments": [{"id": 9, "path": "src/a.rs", "position": 42, "body": "x"}], + }); + assert_eq!( + format_event(&ev), + "**argus @ 2026-11-01T01:00:00Z**: submitted a review\n [#9 src/a.rs:42] x" + ); + // Without the lookup it stays the one-line summary. + let bare = serde_json::json!({"type": "review", "user": {"login": "argus"}, + "created_at": "2026-11-01T01:00:00Z"}); + assert_eq!( + format_event(&bare), + "**argus @ 2026-11-01T01:00:00Z**: submitted a review" + ); + } }