diff --git a/hive-forge-notify/src/notify.rs b/hive-forge-notify/src/notify.rs index 7c9a0e95..c80afeb7 100644 --- a/hive-forge-notify/src/notify.rs +++ b/hive-forge-notify/src/notify.rs @@ -260,7 +260,6 @@ async fn format_notification( source: &S, notif: &PolledNotification, own_login: &str, - prev_updated_at: Option<&str>, ) -> Option { let subj = notif.thread.subject.as_ref(); let title = subj.and_then(|s| s.title.as_deref()).unwrap_or("?"); @@ -340,17 +339,12 @@ async fn format_notification( let is_fresh_state_change = state_change_is_current(¬if.state, notif.thread.updated_at, meta.subject.as_ref()); if has_comment && !is_fresh_state_change { - let enrichment = CommentEnrichment { - subject_api_url, - prev_updated_at, - }; format_comment_notification( client, source, &meta, comment_api_url, comment_html_url, - &enrichment, own_login, ) .await @@ -375,25 +369,12 @@ 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, ) } } @@ -445,24 +426,6 @@ fn build_meta_suffix(subject: Option<&serde_json::Value>, is_pr: bool) -> String out } -/// The extra context `format_comment_notification` needs, beyond -/// `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 - /// poller's in-process dedupe map. `None` when this is the first - /// delivery we've made for the thread this process's lifetime — in - /// that case there's no earlier baseline to diff comments against, - /// so no marker is computed. - prev_updated_at: Option<&'a str>, -} - /// Format a notification triggered by a new comment or review submission. async fn format_comment_notification( client: &reqwest::Client, @@ -470,7 +433,6 @@ async fn format_comment_notification( meta: &NotifMeta<'_>, comment_api_url: &str, comment_html_url: &str, - enrichment: &CommentEnrichment<'_>, own_login: &str, ) -> Option { let payload = fetch_json(client, comment_api_url, source).await; @@ -542,99 +504,11 @@ async fn format_comment_notification( let mut out = format!( "[{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 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 - .as_ref() - .and_then(|c| c["created_at"].as_str()) - .and_then(parse_rfc3339), - ) { - let unseen = fetch_unseen_earlier_events( - client, - source, - enrichment.subject_api_url, - since, - current, - own_login, - ) - .await; - out.push_str(&earlier_updates_marker(unseen)); - } out.push_str(meta_suffix); Some(out) } } -/// 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 { - events - .iter() - .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 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, - since: &str, - current: OffsetDateTime, - own_login: &str, -) -> Option { - let since = parse_rfc3339(since)?; - let list = fetch_json(client, &format!("{subject_api_url}/timeline"), source).await?; - let arr = list.as_array()?; - 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. /// /// Returns `None` for an agent's own *creation* (it opened the issue/PR) — @@ -647,7 +521,6 @@ 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" @@ -735,11 +608,6 @@ 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) } @@ -1046,20 +914,13 @@ pub async fn poll_once( // shows up unread in the next `?all=false` poll. Skip unless its // `updated_at` advanced since the version we last delivered (i.e. // genuinely new activity). See the `delivered` note in `run`. - // - // Captured before the `should_deliver` check consumes `delivered` - // (which only borrows) so the formatter can diff the thread's - // comment list against the version we last saw — see - // `CommentEnrichment`. - let prev_updated_at = delivered.get(id.as_str()).cloned(); let updated_at = notif.updated_at.clone(); if !should_deliver(delivered, &id, &updated_at) { debug!(%id, "forge_notify: skipping (already delivered this version)"); continue; } - let body_opt = - format_notification(client, source, notif, own_login, prev_updated_at.as_deref()).await; + let body_opt = format_notification(client, source, notif, own_login).await; // None means self-echo — mark read silently, no delivery. let Some(body) = body_opt else { @@ -1545,17 +1406,13 @@ 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, None).is_none() - ); + assert!(format_state_change_notification(None, "open", &meta, "damocles", 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, None).is_some() - ); + assert!(format_state_change_notification(None, "open", &meta, "damocles", None).is_some()); } #[test] @@ -1569,8 +1426,7 @@ mod tests { })); let event = parse_rfc3339("2026-06-22T16:00:00Z"); assert!( - format_state_change_notification(event, "closed", &meta, "damocles", None, None) - .is_some() + format_state_change_notification(event, "closed", &meta, "damocles", None).is_some() ); } @@ -1615,7 +1471,6 @@ 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")); @@ -1636,129 +1491,12 @@ mod tests { "user": { "login": "someone-else" }, "body": "the PR description", })); - let new_out = format_state_change_notification(None, "open", &meta, "damocles", None, None) + let new_out = format_state_change_notification(None, "open", &meta, "damocles", 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, None) - .expect("merge notification must render"); + let merged_out = format_state_change_notification(None, "merged", &meta, "damocles", None) + .expect("merge notification must render"); assert!(!merged_out.contains("the PR description")); } - - /// 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 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 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 event that used to go missing entirely. - serde_json::json!({ "created_at": "2026-06-13T14:53:00Z", "user": { "login": "mara" } }), - // 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_unseen_earlier_events(&events, since, current, "damocles"), - 1 - ); - } - - #[test] - 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 events = [ - serde_json::json!({ "created_at": "2026-06-13T14:53:00Z", "user": { "login": "damocles" } }), - ]; - assert_eq!( - count_unseen_earlier_events(&events, since, current, "damocles"), - 0 - ); - } - - #[test] - fn count_unseen_earlier_events_zero_when_nothing_in_the_gap() { - // The common case: `latest_comment_url` really was the only new - // 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 events = [ - serde_json::json!({ "created_at": "2026-06-13T17:01:00Z", "user": { "login": "mara" } }), - ]; - assert_eq!( - count_unseen_earlier_events(&events, since, current, "damocles"), - 0 - ); - } - - #[test] - 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 events = [ - serde_json::json!({ "user": { "login": "mara" } }), - serde_json::json!({ "created_at": "not-a-date", "user": { "login": "mara" } }), - ]; - assert_eq!( - 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 - ); - } }