vinzenz/hyperhive
vinzenz/hyperhive
Watch
0
0
Fork
You've already forked hyperhive
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

hive-forge: smaller comments/timeline defaults, report truncation instead of hiding it

Browse source
This commit is contained in:
damocles 2026-08-03 19:01:30 +02:00 • committed by mara
commit c7d0e4c317
2 changed files with 144 additions and 53 deletions
Download patch file Download diff file Expand all files Collapse all files

152
hive-forge/src/verbs/comments.rs
View file

@ -1,30 +1,25 @@
//! `comments <number> [--limit N | --tail N]` — list comments on an
//! issue or PR. Replaces the curl fallback; `--tail`
//! handles the paging-for-long-threads
//! awkwardness).
//! issue or PR. Replaces the curl fallback.
//!
//! - `--limit N` (default 50, Forgejo's cap) returns the first N
//! comments — same shape this verb has always had.
//! - `--tail N` returns the *last* N comments in chronological
//! order. Reads the issue's `comments` count first to compute
//! which page contains the tail, then fetches only `ceil(N/50) +
//! 1` pages. No upstream cap — the work is bounded by `N`, not
//! by the thread's length, so it stays cheap even on threads with
//! thousands of comments. Use this for "what was the conclusion
//! on this long thread?" without scrolling through the whole
//! history.
//! - `--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.
//!
//! For PRs, review *bodies* (the summary text submitted with an
//! approve / request-changes / comment review) are merged in too:
//! they live in the `pulls/<n>/reviews` object, NOT the
//! issues/comments thread, so plain comment listings used to miss
//! them entirely and reviewers/authors silently lost feedback —
//! the gap this fix closes. 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.
//! Review *bodies* on PRs are always merged in too (`pulls/<n>/reviews`
//! isn't the issues/comments thread, so a plain listing used to miss
//! them), tagged `[review: STATE]`.
//!
//! Use the global `--json` flag for JSON output.
//! `--json` output is an object (`{"comments": [...], "more_before": N,
//! "more_after": N}`), not a bare array, so a script can read the
//! truncation counts too.
use anyhow::Result;
use clap::Args as ClapArgs;
@ -46,7 +41,7 @@ pub struct Args {
pub(crate) number: u64,
/// Number of comments from the start of the thread (max 50).
/// Mutually exclusive with `--tail`.
#[arg(long, default_value_t = 50, conflicts_with = "tail")]
#[arg(long, default_value_t = 10, conflicts_with = "tail")]
limit: u64,
/// Return the last `N` comments (chronological). Mutually exclusive
/// with `--limit`.
@ -56,9 +51,22 @@ pub struct Args {
pub fn run(client: &Client, args: Args) -> Result<()> {
let repo = client.repo();
let thread = match args.tail {
Some(n) => fetch_tail(client, args.number, n)?,
None => fetch_head(client, args.number, args.limit)?,
let total = fetch_total(client, args.number)?;
// Truncation counts: how many comments sit outside the window we're
// about to show, on each side. 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). 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) = if let Some(n) = args.tail {
let thread = fetch_tail(client, args.number, n, total)?;
let more_before = total.saturating_sub(thread.len());
(thread, more_before, 0)
} else {
let thread = fetch_head(client, args.number, args.limit)?;
let more_after = total.saturating_sub(thread.len());
(thread, 0, more_after)
};
// Merge in PR review bodies (empty for issues — degrades to a
// no-op) so review feedback isn't silently dropped.
@ -82,7 +90,11 @@ pub fn run(client: &Client, args: Args) -> Result<()> {
})
})
.collect();
print_json(&Value::Array(trimmed))
print_json(&json!({
"comments": trimmed,
"more_before": more_before,
"more_after": more_after,
}))
} else {
for c in &comments {
let user = c
@ -100,10 +112,47 @@ pub fn run(client: &Client, args: Args) -> Result<()> {
}
println!();
}
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)"
)),
}
}
/// 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).
@ -197,28 +246,22 @@ fn fetch_head(client: &Client, number: u64, limit: u64) -> Result<Vec<Value>> {
}
/// 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: read the issue's `comments` count
/// first 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) -> Result<Vec<Value>> {
if n == 0 {
/// 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)?;
let issue = client.api().issue_get_issue(owner, name, idx).send()?;
let total = issue
.comments
.and_then(|c| usize::try_from(c).ok())
.unwrap_or(0);
if total == 0 {
return Ok(Vec::new());
}
// 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);
@ -330,6 +373,31 @@ mod tests {
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 →

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

@ -10,16 +10,27 @@
//! a human-readable form by default; pass the global `--json` flag
//! for the raw API shape.
//!
//! `--tail N` is a follow-up (the timeline endpoint doesn't expose a
//! total-count field so we can't use the count-then-page trick that
//! `comments --tail` uses; future shape probably mirrors
//! `comments --tail` once Forgejo grows a `count` query or we accept
//! the trailing-slice cost).
//! Default `--limit` is 10 (was 50): this is the verb `hive-forge-notify`
//! points agents at for "there's new activity, go look" — a big default
//! was the same silent-truncation trap `comments`' old default was.
//!
//! **`more` is a boolean, not an exact count**, unlike `comments`'s
//! `more_before`/`more_after`. `comments` gets an exact count for free
//! from the issue's own `comments` field; the timeline endpoint has no
//! equivalent total (mixed event types, no `count` query), and
//! `forgejo-api`'s typed `.send()` doesn't surface response headers
//! (checked: `X-Total-Count`, if Forgejo even sends one, is consumed
//! inside `FromResponse` and never reaches the caller — getting it would
//! mean bypassing the typed endpoint for a raw request, a bigger change
//! than this fix warrants). So this over-fetches by one: ask for
//! `limit + 1`, and if that many come back, trim to `limit` and report
//! "there's more" without saying how much. `--tail` stays a real
//! follow-up if an exact count/tail becomes worth the cost.
use anyhow::Result;
use clap::Args as ClapArgs;
use forgejo_api::structs::IssueGetCommentsAndTimelineQuery;
use serde_json::Value;
use serde_json::{Value, json};
use crate::client::{Client, index};
use crate::verbs::print_json;
@ -28,13 +39,17 @@ use crate::verbs::print_json;
pub struct Args {
/// Issue or PR number.
pub(crate) number: u64,
/// Page size (Forgejo caps at 50). Returns the first `N` events.
#[arg(long, default_value_t = 50)]
/// Return the first `N` events. Default kept small on purpose — see
/// the module doc comment.
#[arg(long, default_value_t = 10)]
limit: u64,
}
pub fn run(client: &Client, args: Args) -> Result<()> {
let (owner, name) = client.owner_repo()?;
// Over-fetch by one to detect truncation without an exact total —
// see the module doc comment for why there's no count query here.
let fetch_limit = args.limit.saturating_add(1);
let (_, events) = client
.api()
.issue_get_comments_and_timeline(
@ -43,13 +58,16 @@ pub fn run(client: &Client, args: Args) -> Result<()> {
index(args.number)?,
IssueGetCommentsAndTimelineQuery::default(),
)
.page_size(u32::try_from(args.limit).unwrap_or(u32::MAX))
.page_size(u32::try_from(fetch_limit).unwrap_or(u32::MAX))
.send()?;
let limit = usize::try_from(args.limit).unwrap_or(usize::MAX);
let more = events.len() > limit;
let events: Vec<_> = events.into_iter().take(limit).collect();
// 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 v = serde_json::to_value(&events)?;
if client.json_mode() {
return print_json(&v);
return print_json(&json!({ "events": v, "more": more }));
}
let Some(events) = v.as_array() else {
return print_json(&v);
@ -57,6 +75,11 @@ pub fn run(client: &Client, args: Args) -> Result<()> {
for ev in events {
print_event(ev);
}
if more {
println!(
"(more activity not shown — raise --limit to see it; exact count not available)"
);
}
Ok(())
}