Watch
0
0
Fork
You've already forked hyperhive
0
hyperhive/hive-forge/src/verbs/comments.rs

535 lines
22 KiB
Rust
Raw Blame History

This file contains ambiguous Unicode characters

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

//! `comments <number> [--limit N | --tail N]` — list comments on an
//! issue or PR. Replaces the curl fallback.
//!
//! - `--limit N` (default 10, was 50) returns the first N comments. A
//! big default was a silent trap on long threads: a plain `comments`
//! call reads as "I've read this thread" when it's really "I've read
//! the oldest N of it".
//! - `--tail N` returns the *last* N comments, chronological. Reads the
//! issue's `comments` count first to compute which page holds the
//! tail, then fetches only `ceil(N/50) + 1` pages — bounded by `N`,
//! not thread length, so it stays cheap on threads with thousands of
//! comments.
//! - **The count of comments outside whatever window is shown is
//! always reported** — never a silent truncation.
//! - `--since <RFC3339>` filters to comments at or after that
//! timestamp instead of a head/tail window — a cursor: feed the
//! last-seen row's own `created_at` back in next time to fetch only
//! what's new. Mutually exclusive with `--tail`. No total exists for
//! a since-filtered query, so this uses the same over-fetch-by-one
//! trick `timeline`'s `--limit` does, and `--limit` is clamped the
//! same way (see `crate::verbs::MAX_LIMIT`).
//!
//! Review *bodies* on PRs are always merged in too (`pulls/<n>/reviews`
//! isn't the issues/comments thread), tagged `[review: STATE]`, with a
//! `(N line comment(s) — see 'pr reviews')` pointer when a review has
//! inline comments — this verb never inlines those, `pr reviews` does.
//!
//! `--json` output is an object (`{"comments": [...], "more_before": N,
//! "more_after": N, "since_more": bool}`), not a bare array, so a
//! script can read the truncation info too.
use anyhow::Result;
use clap::Args as ClapArgs;
use forgejo_api::structs::IssueGetCommentsQuery;
use serde_json::{Value, json};
use time::OffsetDateTime;
use crate::client::{Client, index};
use crate::notify;
use crate::verbs::{MAX_LIMIT, PAGE_SIZE, clamp_limit, parse_rfc3339, print_json, rfc3339};
#[derive(ClapArgs)]
pub struct Args {
/// Issue or PR number.
pub(crate) number: u64,
/// Number of comments from the start of the thread, or (with
/// `--since`) the most this call returns — capped at
/// [`crate::verbs::MAX_LIMIT`] in the latter case. Mutually
/// exclusive with `--tail`.
#[arg(long, default_value_t = 10, conflicts_with = "tail")]
limit: u64,
/// Return the last `N` comments (chronological). Mutually exclusive
/// with `--limit`/`--since`.
#[arg(long, conflicts_with = "since")]
tail: Option<usize>,
/// Only show comments at or after this RFC3339 timestamp (same
/// format this verb's own output prints). Mutually exclusive with
/// `--tail`.
#[arg(long)]
since: Option<String>,
}
pub fn run(client: &Client, args: Args) -> Result<()> {
let repo = client.repo();
// Truncation info: what sits outside the window we're about to show.
// A `--tail` window has nothing after it (it ends at the thread's
// current end); a `--limit`/default window has nothing before it (it
// starts at the thread's beginning); a `--since` window is a boolean
// "there's more" (no total exists for a since-filtered query) rather
// than an exact count. `more_before`/`more_after` are derived from
// `thread.len()` rather than the requested `n`/`limit` so a
// shrunk-under-us thread (comments deleted mid-fetch) still reports
// accurately instead of the number we merely asked for.
let (thread, more_before, more_after, since_more, since_clamped) =
if let Some(since_str) = &args.since {
let since = parse_rfc3339(since_str)?;
let (limit, clamped) = clamp_limit(args.limit);
let (thread, more) = fetch_since(client, args.number, since, limit)?;
(thread, 0, 0, more, clamped)
} else if let Some(n) = args.tail {
let total = fetch_total(client, args.number)?;
let thread = fetch_tail(client, args.number, n, total)?;
let more_before = total.saturating_sub(thread.len());
(thread, more_before, 0, false, false)
} else {
let total = fetch_total(client, args.number)?;
let thread = fetch_head(client, args.number, args.limit)?;
let more_after = total.saturating_sub(thread.len());
(thread, 0, more_after, false, false)
};
// Merge in PR review bodies (empty for issues — degrades to a
// no-op) so review feedback isn't silently dropped.
let comments = merge_chronological(thread, fetch_review_bodies(client, 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);
if client.json_mode() {
let trimmed: Vec<Value> = comments
.iter()
.map(|c| {
json!({
"id": c.get("id"),
"user": c.get("user").and_then(|u| u.get("login")),
"created_at": c.get("created_at"),
"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"),
"comments_count": c.get("comments_count"),
"attachments": attachment_json_from_value(c),
})
})
.collect();
print_json(&json!({
"comments": trimmed,
"more_before": more_before,
"more_after": more_after,
"since_more": since_more,
"since_limit_clamped": since_clamped,
}))
} else {
for c in &comments {
let user = c
.get("user")
.and_then(|u| u.get("login"))
.and_then(Value::as_str)
.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("");
if c.get("kind").and_then(Value::as_str) == Some("review") {
let state = c.get("state").and_then(Value::as_str).unwrap_or("?");
let n = c.get("comments_count").and_then(Value::as_i64).unwrap_or(0);
if n > 0 {
println!(
"**{user} @ {ts}** [review: {state}] ({n} line comment(s) — see `hive-forge pr reviews {}`): {body}",
args.number
);
} else {
println!("**{user} @ {ts}** [review: {state}]: {body}");
}
} else {
println!("**{user} @ {ts}**: {body}");
}
for line in attachment_lines_from_value(c) {
println!("{line}");
}
println!();
}
if since_clamped {
println!(
"(--limit {} is above the {MAX_LIMIT} cap --since can reliably detect truncation at — clamped)",
args.limit
);
}
if since_more {
println!(
"(more comments since this timestamp not shown — raise --limit or bump --since; exact count not available)"
);
}
if let Some(note) = truncation_note(more_before, more_after) {
println!("{note}");
}
Ok(())
}
}
/// A one-line truncation note when the shown window doesn't cover the
/// whole thread — never a silent gap. `None` when both are zero (the
/// whole thread rode the wire). Returns a `String` rather than printing
/// directly so the three cases are unit-testable.
fn truncation_note(more_before: usize, more_after: usize) -> Option<String> {
match (more_before > 0, more_after > 0) {
(false, false) => None,
(true, false) => Some(format!(
"({more_before} earlier comment(s) not shown — use --tail to see the start of the thread)"
)),
(false, true) => Some(format!(
"({more_after} more comment(s) not shown — use --tail N to see the latest instead)"
)),
(true, true) => Some(format!(
"({more_before} earlier + {more_after} later comment(s) not shown — use --tail N)"
)),
}
}
/// Attachment display lines for a single comment's already-serialized
/// `assets` field — the shape this module's merge/render pipeline works
/// on (comments arrive as `Value`s, not the typed `Comment` struct, once
/// merged with synthesized review entries). Review entries carry no
/// `assets` field at all, so they naturally yield nothing here. Mirrors
/// [`crate::verbs::attachment_line`]'s `[file: <name>] <url>` format.
fn attachment_lines_from_value(c: &Value) -> Vec<String> {
c.get("assets")
.and_then(Value::as_array)
.into_iter()
.flatten()
.filter_map(|a| {
let name = a.get("name").and_then(Value::as_str).unwrap_or("?");
let url = a.get("browser_download_url").and_then(Value::as_str)?;
Some(format!("[file: {name}] {url}"))
})
.collect()
}
/// JSON form of [`attachment_lines_from_value`]'s data, for `--json`
/// output — same `{"name", "url"}` shape [`crate::verbs::attachment_json`]
/// produces from the typed struct.
fn attachment_json_from_value(c: &Value) -> Vec<Value> {
c.get("assets")
.and_then(Value::as_array)
.into_iter()
.flatten()
.map(|a| {
json!({
"name": a.get("name"),
"url": a.get("browser_download_url"),
})
})
.collect()
}
/// Total comment count on an issue/PR, straight off the issue object.
/// Used both to plan `--tail`'s pagination and to report how many
/// comments sit outside whatever window ends up shown.
fn fetch_total(client: &Client, number: u64) -> Result<usize> {
let (owner, name) = client.owner_repo()?;
let issue = client
.api()
.issue_get_issue(owner, name, index(number)?)
.send()?;
Ok(issue
.comments
.and_then(|c| usize::try_from(c).ok())
.unwrap_or(0))
}
/// Serialize a typed comment page back to the JSON `Value` shape the
/// merge + render pipeline works on (the structs serialize to the API
/// wire shape, so downstream field access is unchanged).
fn to_values<T: serde::Serialize>(items: Vec<T>) -> Result<Vec<Value>> {
items
.into_iter()
.map(|c| serde_json::to_value(&c).map_err(Into::into))
.collect()
}
/// 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/<n>/reviews` object, not the
/// issues/comments thread, so the plain comment listing misses them
/// — the review-body gap this fixes. 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 reviews with neither a body nor line
/// comments (a bare approval adds nothing to the thread). Deliberately
/// does NOT inline the per-line comments themselves: merging them into
/// this glance-first thread would spam anyone who just wants status.
/// `comments_count` carries a pointer to `pr reviews <n>` instead,
/// which prints the full per-line detail — see that verb
/// (`pr_reviews.rs`) for the exhaustive form. `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, number: u64) -> Vec<Value> {
let Ok((owner, name)) = client.owner_repo() else {
return Vec::new();
};
let Ok(idx) = index(number) else {
return Vec::new();
};
let reviews = client
.api()
.repo_list_pull_reviews(owner, name, idx)
.send()
.map(|(_, reviews)| reviews)
.unwrap_or_default();
reviews
.into_iter()
.filter_map(|r| {
let state = r.state.as_deref().unwrap_or("").to_owned();
if state == "PENDING" {
return None;
}
let comments_count = r.comments_count.unwrap_or(0);
if r.body.as_deref().unwrap_or("").trim().is_empty() && comments_count == 0 {
return None;
}
let submitted = rfc3339(r.submitted_at);
Some(json!({
"id": r.id,
"user": r.user,
"created_at": submitted,
"updated_at": submitted,
"body": r.body,
"html_url": r.html_url,
"kind": "review",
"state": state,
"comments_count": comments_count,
}))
})
.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<Value>, reviews: Vec<Value>) -> Vec<Value> {
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, number: u64, limit: u64) -> Result<Vec<Value>> {
let (owner, name) = client.owner_repo()?;
let (_, comments) = client
.api()
.issue_get_comments(
owner,
name,
index(number)?,
IssueGetCommentsQuery::default(),
)
.page_size(u32::try_from(limit).unwrap_or(u32::MAX))
.send()?;
to_values(comments)
}
/// Fetch the last `n` comments on an issue/PR in chronological order.
/// `total` (the thread's comment count, from [`fetch_total`]) drives the
/// pagination plan and is the caller's, not fetched again here.
///
/// Forgejo orders `/issues/<n>/comments` oldest-first and has no
/// `direction=desc` knob, so naive "page everything and slice" pages
/// from the WRONG end on long threads — the first 1000 comments
/// instead of the last `n`. Fix: use `total` to know how many exist,
/// then start paginating from the page that contains item
/// `total - n`. Work is bounded by `ceil(n/50) + 1` page fetches,
/// regardless of thread length.
fn fetch_tail(client: &Client, number: u64, n: usize, total: usize) -> Result<Vec<Value>> {
if n == 0 || total == 0 {
return Ok(Vec::new());
}
let (owner, name) = client.owner_repo()?;
let idx = index(number)?;
// Cap `n` at the actual total so the math below stays in range
// when the caller asks for more comments than exist.
let n = n.min(total);
let page_size = usize::try_from(PAGE_SIZE).unwrap_or(usize::MAX);
// 0-based index of the first comment we want; integer-divide to
// get the 1-based page that contains it.
let start_idx = total - n;
let start_page = (start_idx / page_size) + 1;
let last_page = (total - 1) / page_size + 1;
let mut merged: Vec<Value> = Vec::with_capacity(n + page_size);
for page in start_page..=last_page {
let (_, arr) = client
.api()
.issue_get_comments(owner, name, idx, IssueGetCommentsQuery::default())
.page(u32::try_from(page).unwrap_or(u32::MAX))
.page_size(u32::try_from(PAGE_SIZE).unwrap_or(u32::MAX))
.send()?;
if arr.is_empty() {
// Page came back empty — either we miscounted (comments
// deleted between the issue GET and now) or upstream's
// playing tricks. Stop rather than spin.
break;
}
merged.extend(to_values(arr)?);
}
// The first fetched page contains items from `start_page` × 50
// back; we overshoot by `start_idx % 50` items. Slice the tail
// to exactly `n` (or fewer if the count shrank under us).
let overshoot = merged.len().saturating_sub(n);
Ok(merged.into_iter().skip(overshoot).collect())
}
/// Fetch comments at or after `since`, capped at `limit` (already
/// clamped to [`crate::verbs::MAX_LIMIT`] by the caller). No total exists
/// for a since-filtered query — Forgejo's API reports none — so this
/// uses the same over-fetch-by-one trick `timeline`'s `--limit` does:
/// ask for `limit + 1`, and a full extra row means there's more. Returns
/// `(comments, more)`, `more` a boolean rather than an exact count.
fn fetch_since(
client: &Client,
number: u64,
since: OffsetDateTime,
limit: u64,
) -> Result<(Vec<Value>, bool)> {
let (owner, name) = client.owner_repo()?;
let query = IssueGetCommentsQuery {
since: Some(since),
..Default::default()
};
let fetch_limit = limit.saturating_add(1);
let (_, comments) = client
.api()
.issue_get_comments(owner, name, index(number)?, query)
.page_size(u32::try_from(fetch_limit).unwrap_or(u32::MAX))
.send()?;
let mut values = to_values(comments)?;
let limit = usize::try_from(limit).unwrap_or(usize::MAX);
let more = values.len() > limit;
values.truncate(limit);
Ok((values, more))
}
#[cfg(test)]
mod tests {
use super::*;
/// Pure helper mirroring the page-arithmetic in `fetch_tail`:
/// given a total comment count + requested tail size, return
/// the (`start_page`, `last_page`) pair the network loop would
/// walk. Lets us pin the pagination plan — the part that's
/// easy to off-by-one — without touching the network.
fn tail_plan(total: usize, n: usize) -> (usize, usize) {
let n = n.min(total);
let page_size = usize::try_from(PAGE_SIZE).unwrap_or(usize::MAX);
let start_idx = total - n;
let start_page = (start_idx / page_size) + 1;
let last_page = (total - 1) / page_size + 1;
(start_page, last_page)
}
#[test]
fn tail_plan_small_thread() {
// 5 total, tail 2 → page 1 covers everything; trim happens
// via the merged.len() - n overshoot calculation, not pages.
assert_eq!(tail_plan(5, 2), (1, 1));
}
#[test]
fn tail_plan_exact_page_boundary() {
// 50 total, tail 3 → all on page 1 (items 1..50).
assert_eq!(tail_plan(50, 3), (1, 1));
}
#[test]
fn tail_plan_crosses_page_boundary() {
// 51 total, tail 3 → start_idx=48 lives on page 1, item 51
// lives on page 2; fetch both.
assert_eq!(tail_plan(51, 3), (1, 2));
}
#[test]
fn tail_plan_large_thread_bounded_pages() {
// 5000 total, tail 3 → start_idx=4997 lives on page 100,
// last_page=100. ONE page fetch on a 5000-comment thread —
// the whole point of swapping to count-then-page (vs the
// old "page everything, then slice the wrong end").
assert_eq!(tail_plan(5000, 3), (100, 100));
}
#[test]
fn tail_plan_large_thread_spans_two_pages() {
// 5000 total, tail 60 → start_idx=4940 on page 99, item 5000
// on page 100. Two fetches even for n > PAGE_SIZE.
assert_eq!(tail_plan(5000, 60), (99, 100));
}
#[test]
fn tail_plan_n_exceeds_total() {
// 5 total, tail 100 → cap n at total; same plan as the
// 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 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 truncation_note_silent_when_nothing_hidden() {
assert_eq!(truncation_note(0, 0), None);
}
#[test]
fn truncation_note_after_only_points_at_tail() {
let note = truncation_note(0, 40).unwrap();
assert!(note.contains("40"), "{note}");
assert!(note.contains("--tail"), "{note}");
}
#[test]
fn truncation_note_before_only_mentions_earlier() {
let note = truncation_note(12, 0).unwrap();
assert!(note.contains("12"), "{note}");
assert!(note.contains("earlier"), "{note}");
}
#[test]
fn truncation_note_both_sides_mentions_both_counts() {
let note = truncation_note(3, 7).unwrap();
assert!(note.contains('3') && note.contains('7'), "{note}");
}
#[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"]);
}
}