vinzenz/hyperhive
vinzenz/hyperhive
Watch
0
0
Fork
You've already forked hyperhive
 0
Code Activity

forge cli: show comment ids and resolved state on inline review comments, --only unresolved, and list them in pr timeline

Browse source
This commit is contained in:
damocles 2026-11-01 01:54:55 +01:00 • committed by mara
commit 219d85a15d
4 changed files with 335 additions and 44 deletions
Download patch file Download diff file Expand all files Collapse all files

6
docs/tools/forge-cli.md
View file

@ -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 <BODY>` — Optional body / message for the review (used with --approve, --request-changes, or --comment)
* `--only <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

4
docs/tools/forge.md
View file

@ -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

240
hive-forge/src/verbs/pr_reviews.rs
View file

@ -1,6 +1,8 @@
//! `pr reviews <number>` — 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<String>,
/// 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<Only>,
}
/// 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<String>
/// 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<Value> {
pub(crate) fn fetch_inline_comments(client: &Client, pr: u64, review_id: i64) -> Vec<Value> {
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<Value>
}
/// 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<Only>) -> Result<()> {
let (owner, name) = client.owner_repo()?;
let idx = index(number)?;
let reviews = all_pages(|page, size| {
@ -110,21 +126,101 @@ 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<Value>, only: Option<Only>) -> Vec<Value> {
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<Only>,
) -> Result<()> {
let trimmed: Vec<Value> = reviews
.iter()
.map(|r| {
.filter_map(|r| {
let id = r.get("id").and_then(Value::as_i64).unwrap_or(0);
let inline: Vec<Value> = if id > 0 {
fetch_inline_comments(client, number, id)
let inline = if id > 0 {
visible(fetch_inline_comments(client, number, id), only)
} else {
vec![]
};
if only.is_some() && inline.is_empty() {
return None;
}
let inline: Vec<Value> = inline
.iter()
.map(|c| {
json!({
@ -132,13 +228,12 @@ fn list_reviews_json(client: &Client, number: u64, reviews: &[Value]) -> Result<
"path": c.get("path"),
"line": c.get("position"),
"body": c.get("body"),
"resolved_by": resolved_by(c),
"outdated": is_outdated(c),
})
})
.collect()
} else {
vec![]
};
json!({
.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<Only>) {
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<Value>| -> Vec<i64> { 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");
}
}

117
hive-forge/src/verbs/timeline.rs
View file

@ -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<i64> = events
.iter()
.filter(|ev| kind(ev).as_deref() == Some("code"))
.filter_map(review_id)
.collect();
let mut fetched: std::collections::HashMap<i64, Vec<Value>> = 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"
);
}
}