diff --git a/docs/tools/forge-cli.md b/docs/tools/forge-cli.md index 12b0b192b..8e6610f69 100644 --- a/docs/tools/forge-cli.md +++ b/docs/tools/forge-cli.md @@ -607,12 +607,6 @@ 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` keeps every comment nobody has resolved yet: what a re-review has to check - - Possible values: - - `unresolved`: - Not resolved yet. Comments without a position stay in: one might sit on a line a push moved or on the whole file, and hiding it could hide a live finding - diff --git a/docs/tools/forge.md b/docs/tools/forge.md index 41680e78e..ef4f71b67 100644 --- a/docs/tools/forge.md +++ b/docs/tools/forge.md @@ -77,9 +77,7 @@ 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; each inline comment as `[#id path:line] text`, tagged (resolved by X) / (no position) -hive-forge pr reviews 42 --only unresolved # every inline comment nobody has resolved yet -hive-forge pr timeline 42 # review rows list their line comments (id, path:line, state) too +hive-forge pr reviews 42 # list reviews; inline comments included per review 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 84c1b862a..b00b4ffbe 100644 --- a/hive-forge/src/verbs/pr_reviews.rs +++ b/hive-forge/src/verbs/pr_reviews.rs @@ -1,8 +1,6 @@ //! `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; @@ -32,21 +30,6 @@ 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` keeps every comment nobody has - /// resolved yet: 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 { - /// Not resolved yet. Comments without a position stay in: one might sit on - /// a line a push moved or on the whole file, and hiding it could hide a - /// live finding. - Unresolved, } pub fn run(client: &Client, args: Args) -> Result<()> { @@ -66,7 +49,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, args.only) + list_reviews(client, args.number) } } @@ -94,7 +77,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. -pub(crate) fn fetch_inline_comments(client: &Client, pr: u64, review_id: i64) -> Vec { +fn fetch_inline_comments(client: &Client, pr: u64, review_id: i64) -> Vec { let Ok((owner, name)) = client.owner_repo() else { return Vec::new(); }; @@ -112,7 +95,7 @@ pub(crate) fn fetch_inline_comments(client: &Client, pr: u64, review_id: i64) -> } /// List all reviews for a PR, dispatching to the appropriate output mode. -fn list_reviews(client: &Client, number: u64, only: Option) -> Result<()> { +fn list_reviews(client: &Client, number: u64) -> Result<()> { let (owner, name) = client.owner_repo()?; let idx = index(number)?; let reviews = all_pages(|page, size| { @@ -127,117 +110,35 @@ fn list_reviews(client: &Client, number: u64, only: Option) -> 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, only) + list_reviews_json(client, number, &reviews) } else { - list_reviews_text(client, number, &reviews, only); + list_reviews_text(client, number, &reviews); 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 with no line position. Forgejo drops `position` when a -/// push moved the anchored line out of the diff, but a file-level comment or -/// one on a removed line may lack it too, so this is a hint, not a verdict. -fn has_no_position(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) -} - -/// Nobody has resolved the conversation yet. Position plays no part: a -/// comment without one may be a live finding, and a filter built for a -/// re-review gate must not hide it. -fn is_unresolved(c: &Value) -> bool { - resolved_by(c).is_none() -} - -/// One inline comment as text: `[#id path:line] body`, continuation lines -/// indented, `(resolved by X)` / `(no position: ...)` 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 has_no_position(c) { - out.push_str(" (no position: outdated or file-level)"); - } - 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_unresolved).collect(), - None => comments, - } -} - /// JSON output: one object per review, with an inline `comments` array. -/// With `--only`, only reviews that still have an open comment. -fn list_reviews_json( - client: &Client, - number: u64, - reviews: &[Value], - only: Option, -) -> Result<()> { +fn list_reviews_json(client: &Client, number: u64, reviews: &[Value]) -> Result<()> { let trimmed: Vec = reviews .iter() - .filter_map(|r| { + .map(|r| { 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) + 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() } else { vec![] }; - 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), - "no_position": has_no_position(c), - }) - }) - .collect(); - Some(json!({ + json!({ "id": r.get("id"), "state": r.get("state"), // forgejo's staleness bits: `stale` = head moved since the @@ -251,28 +152,21 @@ fn list_reviews_json( "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 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; +/// 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; + } 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")) @@ -292,95 +186,23 @@ fn list_reviews_text(client: &Client, number: u64, reviews: &[Value], only: Opti "" }; println!("### review by {user} ({state}){flag}"); - if !body.is_empty() && only.is_none() { + if !body.is_empty() { println!("{body}"); } - for c in &inline { - println!(" {}", render_inline_comment(c)); + 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}"), + } + } } 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 unresolved_means_only_that_nobody_resolved_it() { - assert!(is_unresolved(&comment(1, 42, None))); - assert!(!is_unresolved(&comment(1, 42, Some("mara")))); - // No position does not hide a comment: it may be a live finding. - assert!(is_unresolved(&comment(1, 0, None))); - } - - #[test] - fn unresolved_filter_drops_only_resolved_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, 3] - ); - 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 (no position: outdated or file-level)" - ); - } - - #[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 ddd2782ad..8c65d4c6f 100644 --- a/hive-forge/src/verbs/timeline.rs +++ b/hive-forge/src/verbs/timeline.rs @@ -32,7 +32,6 @@ 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)] @@ -86,10 +85,7 @@ 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 mut v = serde_json::to_value(&events)?; - if let Some(arr) = v.as_array_mut() { - attach_inline_comments(client, args.number, arr); - } + let v = serde_json::to_value(&events)?; if client.json_mode() { return print_json(&json!({ "events": v, @@ -289,52 +285,6 @@ 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 @@ -489,24 +439,7 @@ fn format_event(ev: &Value) -> String { format!("{verb} #{number} ({title})") } "changed_target_branch" => "changed target branch".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}"), - } - } + "review" => "submitted a review".to_owned(), "lock" => "locked the conversation".to_owned(), "unlock" => "unlocked the conversation".to_owned(), // Future / unknown types: surface the raw label so we don't @@ -766,50 +699,4 @@ 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" - ); - } }