3d81266483
The issue-comments endpoint takes no page or limit, so `comments --since` cut its window client-side from a whole-list response. The leftover over-fetch-by-one request (`.page_size(limit + 1)`) was a no-op and the `--limit` clamp to MAX_LIMIT (49) guarded a truncation check that no longer needs one. Drop both: `--since T --limit N` now returns N comments. `comments --json` still emits `since_limit_clamped`, always `false`. `timeline` is untouched; its endpoint pages and its clamp stays.
508 lines
20 KiB
Rust
508 lines
20 KiB
Rust
//! `comments <number> [--limit N | --tail N]` — list comments on an
|
|
//! issue or PR. Replaces the curl fallback.
|
|
//!
|
|
//! - `--limit N` returns the first N comments (oldest-first) — an
|
|
//! explicit opt-in: a plain `comments` call used to read as "I've read
|
|
//! this thread" when it was really "the oldest N of it", so the
|
|
//! default moved to `--tail`'s window instead (see below).
|
|
//! - `--tail N` returns the *last* N comments, chronological — and with
|
|
//! neither flag given, THIS is the default (N = 10): most-recent
|
|
//! activity. Both windows are cut from one whole-thread request (see
|
|
//! `fetch_thread`).
|
|
//! - **The count of comments outside whatever window is shown is
|
|
//! always reported** — never a silent truncation. That total is the
|
|
//! length of this endpoint's list, never `issue_get_issue`'s `comments`
|
|
//! field, which also counts non-prose events this endpoint never returns.
|
|
//! - `--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. Mutually
|
|
//! exclusive with `--tail`; `--limit` windows the filtered list as
|
|
//! given, uncapped.
|
|
//!
|
|
//! 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 for inline comments.
|
|
//!
|
|
//! `--json` output is an object (`{"comments": [...], "more_before": N,
|
|
//! "more_after": N, "since_more": bool, "since_limit_clamped": false}`),
|
|
//! not a bare array, so a script can read the truncation info too.
|
|
//! `--show-reactions` adds a per-comment reaction summary — see that
|
|
//! flag's own doc for the cost.
|
|
|
|
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::{
|
|
all_pages, comment_reactions, parse_rfc3339, print_json, reaction_summary, rfc3339,
|
|
};
|
|
|
|
#[derive(ClapArgs)]
|
|
pub struct Args {
|
|
/// Issue or PR number.
|
|
pub(crate) number: u64,
|
|
/// Number of comments from the start of the thread (oldest-first),
|
|
/// or (with `--since`) the most this call returns. An explicit
|
|
/// opt-in: with neither this nor `--tail` given, the default is the
|
|
/// newest 10 (see `--tail`). Mutually exclusive with `--tail`.
|
|
#[arg(long, conflicts_with = "tail")]
|
|
limit: Option<u64>,
|
|
/// Return the last `N` comments (chronological) — the most recent
|
|
/// activity. Defaults to `N` = 10 when you pass neither `--limit`
|
|
/// nor `--tail`. 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>,
|
|
/// Fetch + display each shown comment's reaction summary. Costs one
|
|
/// extra request per comment shown — opt-in, not the default.
|
|
#[arg(long)]
|
|
show_reactions: bool,
|
|
}
|
|
|
|
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`/default window has nothing after it (it ends at the
|
|
// thread's current end); a `--limit` 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.
|
|
let (thread, more_before, more_after, since_more) = if let Some(since_str) = &args.since {
|
|
let since = parse_rfc3339(since_str)?;
|
|
let (thread, more) = fetch_since(client, args.number, since, args.limit.unwrap_or(10))?;
|
|
(thread, 0, 0, more)
|
|
} else if let Some(limit) = args.limit {
|
|
let (thread, more_after) = head(fetch_thread(client, args.number)?, limit);
|
|
(thread, 0, more_after, false)
|
|
} else {
|
|
// Default (no `--limit`/`--tail`/`--since`): same window as
|
|
// an explicit `--tail 10` — the newest activity, not the
|
|
// oldest. `args.tail` still wins if the agent passed a
|
|
// different count explicitly.
|
|
let n = args.tail.unwrap_or(10);
|
|
let (thread, more_before) = tail(fetch_thread(client, args.number)?, n);
|
|
(thread, more_before, 0, false)
|
|
};
|
|
// Merge in PR review bodies (empty for issues — degrades to a
|
|
// no-op) so review feedback isn't silently dropped.
|
|
let mut 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);
|
|
let (owner, name) = client.owner_repo()?;
|
|
attach_reactions(client, owner, name, &mut comments, args.show_reactions);
|
|
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),
|
|
"reactions": c.get("reactions").cloned().unwrap_or_else(|| json!([])),
|
|
})
|
|
})
|
|
.collect();
|
|
print_json(&json!({
|
|
"comments": trimmed,
|
|
"more_before": more_before,
|
|
"more_after": more_after,
|
|
"since_more": since_more,
|
|
"since_limit_clamped": false,
|
|
}))
|
|
} 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}");
|
|
}
|
|
if let Some(summary) = c
|
|
.get("reactions")
|
|
.and_then(Value::as_array)
|
|
.and_then(|r| reaction_summary(r))
|
|
{
|
|
println!("[reactions: {summary}]");
|
|
}
|
|
println!();
|
|
}
|
|
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 --limit 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)"
|
|
)),
|
|
}
|
|
}
|
|
|
|
/// Fetch + attach each shown comment's reaction list under a
|
|
/// `"reactions"` key, mutating in place so both the `--json` and
|
|
/// pretty-print branches see the same fetch. No-op when `show_reactions`
|
|
/// is false (the default) — the whole point of gating this behind a
|
|
/// flag is that a plain `comments` call costs nothing extra. Review
|
|
/// entries are skipped (synthesized, never have real reactions); a
|
|
/// per-comment fetch failure is swallowed rather than failing the whole
|
|
/// listing over one comment.
|
|
fn attach_reactions(
|
|
client: &Client,
|
|
owner: &str,
|
|
name: &str,
|
|
comments: &mut [Value],
|
|
show_reactions: bool,
|
|
) {
|
|
if !show_reactions {
|
|
return;
|
|
}
|
|
for c in comments.iter_mut() {
|
|
if c.get("kind").and_then(Value::as_str) == Some("review") {
|
|
continue;
|
|
}
|
|
let Some(id) = c.get("id").and_then(Value::as_u64) else {
|
|
continue;
|
|
};
|
|
if let Ok(reactions) = comment_reactions(client, owner, name, id)
|
|
&& let Some(obj) = c.as_object_mut()
|
|
{
|
|
obj.insert("reactions".to_owned(), json!(reactions));
|
|
}
|
|
}
|
|
}
|
|
|
|
/// 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()
|
|
}
|
|
|
|
/// 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 = all_pages(|page, size| {
|
|
let (_, batch) = client
|
|
.api()
|
|
.repo_list_pull_reviews(owner, name, idx)
|
|
.page(page)
|
|
.page_size(size)
|
|
.send()?;
|
|
Ok(batch)
|
|
})
|
|
.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
|
|
}
|
|
|
|
/// Every comment on an issue/PR, oldest-first, in one request. The
|
|
/// endpoint takes no page or limit, so the response is always the whole
|
|
/// thread and a window has to be cut client-side.
|
|
fn fetch_thread(client: &Client, number: u64) -> Result<Vec<Value>> {
|
|
let (owner, name) = client.owner_repo()?;
|
|
let (_, comments) = client
|
|
.api()
|
|
.issue_get_comments(
|
|
owner,
|
|
name,
|
|
index(number)?,
|
|
IssueGetCommentsQuery::default(),
|
|
)
|
|
.send()?;
|
|
to_values(comments)
|
|
}
|
|
|
|
/// The first `limit` comments of `thread`, and how many follow them.
|
|
fn head(mut thread: Vec<Value>, limit: u64) -> (Vec<Value>, usize) {
|
|
let total = thread.len();
|
|
thread.truncate(usize::try_from(limit).unwrap_or(usize::MAX));
|
|
let more_after = total - thread.len();
|
|
(thread, more_after)
|
|
}
|
|
|
|
/// The last `n` comments of `thread`, and how many precede them.
|
|
fn tail(mut thread: Vec<Value>, n: usize) -> (Vec<Value>, usize) {
|
|
let more_before = thread.len().saturating_sub(n);
|
|
thread.drain(..more_before);
|
|
(thread, more_before)
|
|
}
|
|
|
|
/// Comments at or after `since`, the first `limit` of them. The endpoint
|
|
/// takes no page or limit, so the filtered list arrives whole and `more`
|
|
/// (whether any follow the window) is read off its length. A boolean
|
|
/// rather than a count: a since-filtered window has no total to report.
|
|
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 (_, comments) = client
|
|
.api()
|
|
.issue_get_comments(owner, name, index(number)?, query)
|
|
.send()?;
|
|
let (values, more_after) = head(to_values(comments)?, limit);
|
|
Ok((values, more_after > 0))
|
|
}
|
|
|
|
#[cfg(test)]
|
|
mod tests {
|
|
use super::*;
|
|
|
|
fn thread(len: usize) -> Vec<Value> {
|
|
(0..len).map(|i| json!({ "id": i })).collect()
|
|
}
|
|
|
|
fn ids(thread: &[Value]) -> Vec<u64> {
|
|
thread.iter().filter_map(|c| c["id"].as_u64()).collect()
|
|
}
|
|
|
|
#[test]
|
|
fn head_keeps_the_first_limit_and_counts_the_rest() {
|
|
let (shown, more_after) = head(thread(60), 5);
|
|
assert_eq!(ids(&shown), vec![0, 1, 2, 3, 4]);
|
|
assert_eq!(more_after, 55);
|
|
}
|
|
|
|
#[test]
|
|
fn head_past_the_end_shows_everything() {
|
|
let (shown, more_after) = head(thread(3), 10);
|
|
assert_eq!(shown.len(), 3);
|
|
assert_eq!(more_after, 0);
|
|
}
|
|
|
|
#[test]
|
|
fn tail_keeps_the_last_n_and_counts_the_rest() {
|
|
let (shown, more_before) = tail(thread(60), 3);
|
|
assert_eq!(ids(&shown), vec![57, 58, 59]);
|
|
assert_eq!(more_before, 57);
|
|
}
|
|
|
|
#[test]
|
|
fn tail_past_the_start_shows_everything() {
|
|
let (shown, more_before) = tail(thread(3), 10);
|
|
assert_eq!(shown.len(), 3);
|
|
assert_eq!(more_before, 0);
|
|
}
|
|
|
|
#[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_points_at_limit() {
|
|
// Regression: this used to (wrongly) suggest `--tail` — the flag
|
|
// that produced the very window you're already looking at when
|
|
// more_before is nonzero. `--limit` is what actually reaches the
|
|
// start of the thread.
|
|
let note = truncation_note(12, 0).unwrap();
|
|
assert!(note.contains("12"), "{note}");
|
|
assert!(note.contains("earlier"), "{note}");
|
|
assert!(note.contains("--limit"), "{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"]);
|
|
}
|
|
}
|