From 1cff77f5fae821b2a6f755942e10adb6c65d9bc0 Mon Sep 17 00:00:00 2001 From: damocles Date: Mon, 29 Jun 2026 01:42:47 +0200 Subject: [PATCH] fix(#2088): merge pr review bodies into hive-forge comments listing --- hive-forge/src/verbs/comments.rs | 119 ++++++++++++++++++++++++++++++- 1 file changed, 117 insertions(+), 2 deletions(-) diff --git a/hive-forge/src/verbs/comments.rs b/hive-forge/src/verbs/comments.rs index 610dc1c6..a2dc6cf2 100644 --- a/hive-forge/src/verbs/comments.rs +++ b/hive-forge/src/verbs/comments.rs @@ -14,6 +14,15 @@ //! on this long thread?" without scrolling through the whole //! history. //! +//! For PRs, review *bodies* (the summary text submitted with an +//! approve / request-changes / comment review) are merged in too: +//! they live in the `pulls//reviews` object, NOT the +//! issues/comments thread, so plain comment listings used to miss +//! them entirely and reviewers/authors silently lost feedback +//! (#2088). They're always included regardless of `--limit`/`--tail` +//! (reviews are few + high-signal) and tagged `[review: STATE]` so +//! they're distinguishable from issue-thread comments. +//! //! Use the global `--json` flag for JSON output. use anyhow::Result; @@ -48,10 +57,13 @@ pub struct Args { pub fn run(client: &Client, args: Args) -> Result<()> { let repo = client.repo(); - let comments = match args.tail { + let thread = match args.tail { Some(n) => fetch_tail(client, repo, args.number, n)?, None => fetch_head(client, repo, args.number, args.limit)?, }; + // Merge in PR review bodies (empty for issues — degrades to a + // no-op) so review feedback isn't silently dropped (#2088). + let comments = merge_chronological(thread, fetch_review_bodies(client, repo, args.number)); // Reading the thread clears its unread notification so the // read-before-comment guard (in `comment`) lets a reply through. notify::mark_read_best_effort(client, repo, args.number); @@ -66,6 +78,8 @@ pub fn run(client: &Client, args: Args) -> Result<()> { "updated_at": c.get("updated_at"), "body": c.get("body"), "url": c.get("html_url"), + "kind": c.get("kind").and_then(Value::as_str).unwrap_or("comment"), + "state": c.get("state"), }) }) .collect(); @@ -79,13 +93,80 @@ pub fn run(client: &Client, args: Args) -> Result<()> { .unwrap_or("?"); let ts = c.get("created_at").and_then(Value::as_str).unwrap_or("?"); let body = c.get("body").and_then(Value::as_str).unwrap_or(""); - println!("**{user} @ {ts}**: {body}"); + if c.get("kind").and_then(Value::as_str) == Some("review") { + let state = c.get("state").and_then(Value::as_str).unwrap_or("?"); + println!("**{user} @ {ts}** [review: {state}]: {body}"); + } else { + println!("**{user} @ {ts}**: {body}"); + } println!(); } Ok(()) } } +/// Fetch a PR's review *bodies* and normalise them to the comment +/// shape so they merge alongside issue-thread comments. +/// +/// Review summaries live in the `pulls//reviews` object, not the +/// issues/comments thread, so the plain comment listing misses them +/// (#2088). Best-effort: returns empty on any error — notably when +/// `number` is an issue (no reviews endpoint) — so callers degrade +/// gracefully. Skips PENDING reviews (not yet visible to others) and +/// empty-body reviews (a bare approval adds nothing to the thread). +/// `created_at` is synthesised from the review's `submitted_at` so +/// the chronological merge sorts uniformly; `kind:"review"` + the +/// review `state` tag the entry for display. +fn fetch_review_bodies(client: &Client, repo: &str, number: u64) -> Vec { + let reviews = client + .get_json(&format!("/repos/{repo}/pulls/{number}/reviews")) + .ok() + .and_then(|v| v.as_array().cloned()) + .unwrap_or_default(); + reviews + .into_iter() + .filter_map(|r| { + let state = r.get("state").and_then(Value::as_str).unwrap_or(""); + if state == "PENDING" { + return None; + } + if r.get("body") + .and_then(Value::as_str) + .unwrap_or("") + .trim() + .is_empty() + { + return None; + } + Some(json!({ + "id": r.get("id"), + "user": r.get("user"), + "created_at": r.get("submitted_at"), + "updated_at": r.get("submitted_at"), + "body": r.get("body"), + "html_url": r.get("html_url"), + "kind": "review", + "state": state, + })) + }) + .collect() +} + +/// Merge issue-thread comments with review bodies and sort the +/// combined list chronologically by `created_at`. ISO-8601 timestamps +/// sort lexicographically in time order, so a plain string compare is +/// correct; the sort is stable, so same-timestamp entries keep fetch +/// order. +fn merge_chronological(mut items: Vec, reviews: Vec) -> Vec { + items.extend(reviews); + items.sort_by(|a, b| { + let ka = a.get("created_at").and_then(Value::as_str).unwrap_or(""); + let kb = b.get("created_at").and_then(Value::as_str).unwrap_or(""); + ka.cmp(kb) + }); + items +} + /// Fetch the first page's worth of comments (existing behaviour). fn fetch_head(client: &Client, repo: &str, number: u64, limit: u64) -> Result> { let v = client.get_json(&format!( @@ -206,4 +287,38 @@ mod tests { // small-thread case above. assert_eq!(tail_plan(5, 100), (1, 1)); } + + #[test] + fn merge_interleaves_reviews_by_timestamp() { + // A review submitted between two comments must land between + // them, not appended at the end — that's the whole #2088 fix. + let comments = vec![ + json!({"created_at": "2026-06-29T01:00:00Z", "body": "c1"}), + json!({"created_at": "2026-06-29T01:20:00Z", "body": "c2"}), + ]; + let reviews = + vec![json!({"created_at": "2026-06-29T01:10:00Z", "body": "r1", "kind": "review"})]; + let merged = merge_chronological(comments, reviews); + let bodies: Vec<&str> = merged + .iter() + .map(|v| v.get("body").and_then(Value::as_str).unwrap()) + .collect(); + assert_eq!(bodies, vec!["c1", "r1", "c2"]); + } + + #[test] + fn merge_with_no_reviews_is_identity() { + // Issues have no reviews → fetch_review_bodies returns empty → + // the merge must leave the comment thread untouched. + let comments = vec![ + json!({"created_at": "2026-06-29T01:00:00Z", "body": "c1"}), + json!({"created_at": "2026-06-29T01:20:00Z", "body": "c2"}), + ]; + let merged = merge_chronological(comments, vec![]); + let bodies: Vec<&str> = merged + .iter() + .map(|v| v.get("body").and_then(Value::as_str).unwrap()) + .collect(); + assert_eq!(bodies, vec!["c1", "c2"]); + } }