diff --git a/hive-forge/src/verbs/comments.rs b/hive-forge/src/verbs/comments.rs index 3779a809..b18a3453 100644 --- a/hive-forge/src/verbs/comments.rs +++ b/hive-forge/src/verbs/comments.rs @@ -6,8 +6,11 @@ //! - `--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. Internally pages through every comment on the issue, -//! then slices the tail. Use this for "what was the conclusion +//! 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. //! @@ -20,13 +23,10 @@ use serde_json::{Value, json}; use crate::client::Client; use crate::verbs::print_json; -/// Hard cap on `--tail` pagination so a confused caller can't -/// accidentally paginate forever on a runaway-comment thread. -/// 20 pages × Forgejo's 50-per-page cap = 1000 comments which -/// covers every issue in the hyperhive repo today with headroom. -/// Tune up if a future thread breaches it (the truncated tail -/// will surface as a smaller-than-expected slice). -const TAIL_MAX_PAGES: u32 = 20; +/// Forgejo's per-page comment cap. The API caps `limit` at 50 even +/// if a higher value is requested; pin it explicitly so the math +/// downstream doesn't depend on a hidden default. +const PAGE_SIZE: u64 = 50; #[derive(ClapArgs)] pub struct Args { @@ -88,35 +88,115 @@ fn fetch_head(client: &Client, repo: &str, number: u64, limit: u64) -> Result/comments` oldest-first so the trailing slice maps -/// directly to "most recent N". +/// Fetch the last `n` comments on an issue/PR in chronological order. /// -/// When the fetch saturates `TAIL_MAX_PAGES` (i.e. all 20 pages came -/// back full at 50/page) we can't tell whether more comments exist -/// upstream. Surface a stderr warning so the caller knows the -/// "tail" is anchored to the first 1000 comments, not necessarily -/// the actual newest of the thread. False positive: a thread with -/// EXACTLY 1000 comments will warn even though the slice IS the -/// real tail — acceptable noise given how rare 1000-comment threads -/// are (closes argus 🟡 on PR #770). +/// Forgejo orders `/issues//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, repo: &str, number: u64, n: usize) -> Result> { - let all = client.get_json_all( - &format!("/repos/{repo}/issues/{number}/comments?limit=50"), - TAIL_MAX_PAGES, - )?; - let total = all.len(); - if total >= TAIL_MAX_PAGES as usize * 50 { - eprintln!( - "warning: hit pagination cap ({TAIL_MAX_PAGES} pages × 50 = \ - {total} comments); --tail slice is anchored to the first \ - {total} comments and may not include the actual newest of \ - the thread." - ); + if n == 0 { + return Ok(Vec::new()); } - if total <= n { - return Ok(all); + let issue = client.get_json(&format!("/repos/{repo}/issues/{number}"))?; + let total = issue + .get("comments") + .and_then(Value::as_u64) + .unwrap_or(0) as usize; + 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); + let page_size = PAGE_SIZE as usize; + // 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 = Vec::with_capacity(n + page_size); + for page in start_page..=last_page { + let v = client.get_json(&format!( + "/repos/{repo}/issues/{number}/comments?limit={PAGE_SIZE}&page={page}" + ))?; + let arr = v.as_array().cloned().unwrap_or_default(); + 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(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()) +} + +#[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 = PAGE_SIZE as usize; + 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)); } - Ok(all.into_iter().skip(total - n).collect()) }