diff --git a/hive-forge-notify/src/notify.rs b/hive-forge-notify/src/notify.rs index 57406cfb..7c9a0e95 100644 --- a/hive-forge-notify/src/notify.rs +++ b/hive-forge-notify/src/notify.rs @@ -375,12 +375,25 @@ async fn format_notification( } else { None }; + // Same widened marker as the comment path (see that branch's + // comment), pre-fetched here because `format_state_change_notification` + // stays sync — mirrors how `comment_tail` above is already fetched + // before the sync formatter is called, same reason. + let earlier_updates = if let (Some(since), Some(current)) = + (prev_updated_at, notif.thread.updated_at) + { + fetch_unseen_earlier_events(client, source, subject_api_url, since, current, own_login) + .await + } else { + None + }; format_state_change_notification( notif.thread.updated_at, ¬if.state, &meta, own_login, comment_tail, + earlier_updates, ) } } @@ -433,11 +446,13 @@ fn build_meta_suffix(subject: Option<&serde_json::Value>, is_pr: bool) -> String } /// The extra context `format_comment_notification` needs, beyond -/// `NotifMeta`, only to power the "earlier unseen comment" marker on -/// the plain-comment path — see [`count_unseen_earlier_comments`]. -/// Kept as its own small struct (rather than folded into `NotifMeta`) -/// so that struct's other consumer (the state-change formatter) doesn't -/// have to carry fields it never uses. +/// `NotifMeta`, only to power the "earlier unseen update" marker on the +/// plain-comment path — see [`fetch_unseen_earlier_events`]. The +/// state-change path wants the same two values, but fetches them itself in +/// `format_notification` before calling the (sync) state-change formatter, +/// so it never needs this bundle — kept as its own small struct rather than +/// folded into `NotifMeta` for that reason, not because the other path has +/// no use for the underlying data. struct CommentEnrichment<'a> { subject_api_url: &'a str, /// The thread's previously-delivered raw `updated_at`, from the @@ -528,11 +543,14 @@ async fn format_comment_notification( "[{kind}] {title}\nurl: {url}\n\n{author}: {body_for_embed}{truncated_mentions}" ); // Forgejo's notification API tracks one row per subject with a - // single `latest_comment_url`, so an earlier comment made on the - // same thread since we last looked is otherwise never individually - // surfaced — the blurb silently shows only the newest one. Flag a - // count rather than rendering more text (see the forge-notify - // hidden-comment issue for why). + // single `latest_comment_url`, so anything else that happened on + // the same thread since we last looked — an earlier comment, a + // label, a close/reopen, a review — is otherwise never individually + // surfaced; the blurb silently shows only the newest comment. Flag + // a count rather than rendering more text (see the forge-notify + // hidden-comment issue for why), over the full timeline rather than + // just comments (widened per mara's review call: "properly check + // the timeline"). if let (Some(since), Some(current)) = ( enrichment.prev_updated_at, payload @@ -540,7 +558,7 @@ async fn format_comment_notification( .and_then(|c| c["created_at"].as_str()) .and_then(parse_rfc3339), ) { - let unseen = count_unseen_earlier_comments( + let unseen = fetch_unseen_earlier_events( client, source, enrichment.subject_api_url, @@ -549,50 +567,47 @@ async fn format_comment_notification( own_login, ) .await; - if let Some(n) = unseen.filter(|&n| n > 0) { - let plural = if n == 1 { "" } else { "s" }; - write!( - out, - "\n\n(+{n} earlier comment{plural} since your last visit)" - ) - .ok(); - } + out.push_str(&earlier_updates_marker(unseen)); } out.push_str(meta_suffix); Some(out) } } -/// Count comments in `comments` (raw JSON array from the issue/PR -/// comments-list endpoint) that landed strictly after `since` and -/// strictly before `current`, excluding any authored by `own_login`. -/// Pure for unit testing; [`count_unseen_earlier_comments`] does the -/// network fetch and calls this on the result. -fn count_comments_between( - comments: &[serde_json::Value], +/// Count entries in `events` (raw JSON array from the issue/PR **timeline** +/// endpoint — comments AND state-change events: label, assign, close/reopen, +/// review, `pull_push`, …) that landed strictly after `since` and strictly +/// before `current`, excluding any authored by `own_login`. Deliberately +/// type-agnostic — every timeline entry the API returns is counted the same +/// way, per mara's review call ("count everything") rather than singling +/// out any one type as noise. Pure for unit testing; +/// [`fetch_unseen_earlier_events`] does the network fetch and calls this on +/// the result. +fn count_unseen_earlier_events( + events: &[serde_json::Value], since: OffsetDateTime, current: OffsetDateTime, own_login: &str, ) -> usize { - comments + events .iter() - .filter(|c| { - let created = c["created_at"].as_str().and_then(parse_rfc3339); - let author = c["user"]["login"].as_str().unwrap_or(""); + .filter(|e| { + let created = e["created_at"].as_str().and_then(parse_rfc3339); + let author = e["user"]["login"].as_str().unwrap_or(""); matches!(created, Some(t) if t > since && t < current) && (own_login.is_empty() || author != own_login) }) .count() } -/// Fetch the subject's full comment list and count how many landed -/// strictly between `since` (the thread's previously-delivered raw -/// `updated_at`) and `current` (the comment now being rendered) — the -/// comments Forgejo's one-row-per-subject `latest_comment_url` silently -/// drops between polls. `None` on any fetch/parse failure or an -/// unparseable `since`; the caller just omits the marker rather than -/// blocking delivery on it. -async fn count_unseen_earlier_comments( +/// Fetch the subject's full timeline (comments + every state-change event) +/// and count how many entries landed strictly between `since` (the thread's +/// previously-delivered raw `updated_at`) and `current` (the event now being +/// rendered) — everything Forgejo's one-row-per-subject notification +/// silently drops between polls, comment or not. `None` on any +/// fetch/parse failure or an unparseable `since`; the caller just omits the +/// marker rather than blocking delivery on it. +async fn fetch_unseen_earlier_events( client: &reqwest::Client, source: &S, subject_api_url: &str, @@ -601,9 +616,23 @@ async fn count_unseen_earlier_comments( own_login: &str, ) -> Option { let since = parse_rfc3339(since)?; - let list = fetch_json(client, &format!("{subject_api_url}/comments"), source).await?; + let list = fetch_json(client, &format!("{subject_api_url}/timeline"), source).await?; let arr = list.as_array()?; - Some(count_comments_between(arr, since, current, own_login)) + Some(count_unseen_earlier_events(arr, since, current, own_login)) +} + +/// The trailing `(+N earlier update(s) since your last visit)` marker for +/// [`fetch_unseen_earlier_events`]'s result — empty string for `None` (fetch +/// failed) or `Some(0)` (nothing hidden), shared by both delivery paths so +/// the wording can't drift between them. +fn earlier_updates_marker(unseen: Option) -> String { + match unseen { + Some(n) if n > 0 => { + let plural = if n == 1 { "" } else { "s" }; + format!("\n\n(+{n} earlier update{plural} since your last visit)") + } + _ => String::new(), + } } /// Format a notification triggered by creation or state change of the subject. @@ -618,6 +647,7 @@ fn format_state_change_notification( meta: &NotifMeta<'_>, own_login: &str, comment_tail: Option, + earlier_updates: Option, ) -> Option { // Classification uses the raw `subject.state` string extracted in // `parse_notification` — Forgejo returns "open" / "closed" / "merged" @@ -705,6 +735,11 @@ fn format_state_change_notification( if let Some(tail) = comment_tail { out.push_str(&tail); } + // Same widened "you might be missing something" marker the comment path + // carries — a state-change notification (e.g. one merge) can just as + // easily hide several intervening labels/comments/reopens as a comment + // notification can hide earlier comments. + out.push_str(&earlier_updates_marker(earlier_updates)); out.push_str(meta_suffix); Some(out) } @@ -1510,13 +1545,17 @@ mod tests { fn state_change_drops_self_authored_creation() { // No timestamps ⇒ treated as a creation; poster login == own_login. let meta = state_change_meta(serde_json::json!({ "user": { "login": "damocles" } })); - assert!(format_state_change_notification(None, "open", &meta, "damocles", None).is_none()); + assert!( + format_state_change_notification(None, "open", &meta, "damocles", None, None).is_none() + ); } #[test] fn state_change_keeps_other_authored_creation() { let meta = state_change_meta(serde_json::json!({ "user": { "login": "someone-else" } })); - assert!(format_state_change_notification(None, "open", &meta, "damocles", None).is_some()); + assert!( + format_state_change_notification(None, "open", &meta, "damocles", None, None).is_some() + ); } #[test] @@ -1530,7 +1569,8 @@ mod tests { })); let event = parse_rfc3339("2026-06-22T16:00:00Z"); assert!( - format_state_change_notification(event, "closed", &meta, "damocles", None).is_some() + format_state_change_notification(event, "closed", &meta, "damocles", None, None) + .is_some() ); } @@ -1575,6 +1615,7 @@ mod tests { &meta, "damocles", Some("\n\ncomment by argus: nice, merging".to_owned()), + None, ) .expect("merge notification must render"); assert!(out.contains("comment by argus: nice, merging")); @@ -1595,77 +1636,129 @@ mod tests { "user": { "login": "someone-else" }, "body": "the PR description", })); - let new_out = format_state_change_notification(None, "open", &meta, "damocles", None) + let new_out = format_state_change_notification(None, "open", &meta, "damocles", None, None) .expect("open notification must render"); assert!(!new_out.contains("the PR description")); - let merged_out = format_state_change_notification(None, "merged", &meta, "damocles", None) - .expect("merge notification must render"); + let merged_out = + format_state_change_notification(None, "merged", &meta, "damocles", None, None) + .expect("merge notification must render"); assert!(!merged_out.contains("the PR description")); } - /// Regression coverage for the earlier-unseen-comment gap: - /// `count_comments_between` is the pure core of that marker. + /// The widened marker (per mara's review call: "count everything") must + /// render on the state-change path too, not just the comment path — a merge/close + /// notification can hide just as many intervening events as a comment + /// notification can. #[test] - fn count_comments_between_counts_only_the_gap() { + fn format_state_change_appends_earlier_updates_marker() { + let meta = state_change_meta(serde_json::json!({ + "user": { "login": "someone-else" }, + })); + let out = + format_state_change_notification(None, "merged", &meta, "damocles", None, Some(3)) + .expect("merge notification must render"); + assert!(out.contains("(+3 earlier updates since your last visit)")); + } + + #[test] + fn format_state_change_omits_marker_when_nothing_unseen() { + let meta = state_change_meta(serde_json::json!({ + "user": { "login": "someone-else" }, + })); + let out = format_state_change_notification(None, "merged", &meta, "damocles", None, None) + .expect("merge notification must render"); + assert!(!out.contains("earlier update")); + } + + /// Regression coverage for the earlier-unseen-update gap: + /// `count_unseen_earlier_events` is the pure core of that marker. + #[test] + fn count_unseen_earlier_events_counts_only_the_gap() { let since = parse_rfc3339("2026-06-13T10:00:00Z").unwrap(); let current = parse_rfc3339("2026-06-13T17:01:00Z").unwrap(); - let comments = [ + let events = [ // Before `since` — already seen on a prior delivery, not counted. serde_json::json!({ "created_at": "2026-06-13T09:00:00Z", "user": { "login": "mara" } }), - // In the gap — the comment that used to go missing entirely. + // In the gap — the event that used to go missing entirely. serde_json::json!({ "created_at": "2026-06-13T14:53:00Z", "user": { "login": "mara" } }), - // The comment being rendered right now — excluded (not "earlier"). + // The event being rendered right now — excluded (not "earlier"). serde_json::json!({ "created_at": "2026-06-13T17:01:00Z", "user": { "login": "mara" } }), ]; assert_eq!( - count_comments_between(&comments, since, current, "damocles"), + count_unseen_earlier_events(&events, since, current, "damocles"), 1 ); } #[test] - fn count_comments_between_excludes_own_comments() { - // The agent's own reply in the gap isn't "unseen" — it wrote it. + fn count_unseen_earlier_events_excludes_own_events() { + // The agent's own reply/action in the gap isn't "unseen" — it did it. let since = parse_rfc3339("2026-06-13T10:00:00Z").unwrap(); let current = parse_rfc3339("2026-06-13T17:01:00Z").unwrap(); - let comments = [ + let events = [ serde_json::json!({ "created_at": "2026-06-13T14:53:00Z", "user": { "login": "damocles" } }), ]; assert_eq!( - count_comments_between(&comments, since, current, "damocles"), + count_unseen_earlier_events(&events, since, current, "damocles"), 0 ); } #[test] - fn count_comments_between_zero_when_nothing_in_the_gap() { + fn count_unseen_earlier_events_zero_when_nothing_in_the_gap() { // The common case: `latest_comment_url` really was the only new - // comment, no marker should ever fire for it. + // thing, no marker should ever fire for it. let since = parse_rfc3339("2026-06-13T10:00:00Z").unwrap(); let current = parse_rfc3339("2026-06-13T17:01:00Z").unwrap(); - let comments = [ + let events = [ serde_json::json!({ "created_at": "2026-06-13T17:01:00Z", "user": { "login": "mara" } }), ]; assert_eq!( - count_comments_between(&comments, since, current, "damocles"), + count_unseen_earlier_events(&events, since, current, "damocles"), 0 ); } #[test] - fn count_comments_between_ignores_unparseable_entries() { - // A comment missing/garbling `created_at` degrades to "not in the + fn count_unseen_earlier_events_ignores_unparseable_entries() { + // An entry missing/garbling `created_at` degrades to "not in the // gap" rather than panicking or poisoning the count. let since = parse_rfc3339("2026-06-13T10:00:00Z").unwrap(); let current = parse_rfc3339("2026-06-13T17:01:00Z").unwrap(); - let comments = [ + let events = [ serde_json::json!({ "user": { "login": "mara" } }), serde_json::json!({ "created_at": "not-a-date", "user": { "login": "mara" } }), ]; assert_eq!( - count_comments_between(&comments, since, current, "damocles"), + count_unseen_earlier_events(&events, since, current, "damocles"), 0 ); } + + /// Regression for the whole point of the widening (mara's "count + /// everything" ruling): a non-comment timeline entry (a label add, no `body` + /// field, just `type`/`created_at`/`user`) must count exactly like a + /// comment does — the function has no type filter at all. + #[test] + fn count_unseen_earlier_events_counts_non_comment_types() { + let since = parse_rfc3339("2026-06-13T10:00:00Z").unwrap(); + let current = parse_rfc3339("2026-06-13T17:01:00Z").unwrap(); + let events = [ + serde_json::json!({ + "type": "label", + "created_at": "2026-06-13T14:53:00Z", + "user": { "login": "mara" }, + }), + serde_json::json!({ + "type": "pull_push", + "created_at": "2026-06-13T15:10:00Z", + "user": { "login": "mara" }, + }), + ]; + assert_eq!( + count_unseen_earlier_events(&events, since, current, "damocles"), + 2 + ); + } }