From 530eedd5f425e8284dcf06bfbddfa759700767d1 Mon Sep 17 00:00:00 2001 From: damocles Date: Wed, 19 Aug 2026 04:19:55 +0200 Subject: [PATCH] forge_notify: flag earlier comments the latest_comment_url blurb hides --- hive-forge-notify/src/notify.rs | 171 +++++++++++++++++++++++++++++++- 1 file changed, 170 insertions(+), 1 deletion(-) diff --git a/hive-forge-notify/src/notify.rs b/hive-forge-notify/src/notify.rs index c80afeb7..57406cfb 100644 --- a/hive-forge-notify/src/notify.rs +++ b/hive-forge-notify/src/notify.rs @@ -260,6 +260,7 @@ 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("?"); @@ -339,12 +340,17 @@ 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 @@ -426,6 +432,22 @@ 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 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. +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, @@ -433,6 +455,7 @@ 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; @@ -504,11 +527,85 @@ 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 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). + 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 = count_unseen_earlier_comments( + client, + source, + enrichment.subject_api_url, + since, + current, + 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(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], + since: OffsetDateTime, + current: OffsetDateTime, + own_login: &str, +) -> usize { + comments + .iter() + .filter(|c| { + let created = c["created_at"].as_str().and_then(parse_rfc3339); + let author = c["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( + 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}/comments"), source).await?; + let arr = list.as_array()?; + Some(count_comments_between(arr, since, current, own_login)) +} + /// 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) — @@ -914,13 +1011,20 @@ 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).await; + let body_opt = + format_notification(client, source, notif, own_login, prev_updated_at.as_deref()).await; // None means self-echo — mark read silently, no delivery. let Some(body) = body_opt else { @@ -1499,4 +1603,69 @@ mod tests { .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. + #[test] + fn count_comments_between_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 = [ + // 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. + serde_json::json!({ "created_at": "2026-06-13T14:53:00Z", "user": { "login": "mara" } }), + // The comment 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"), + 1 + ); + } + + #[test] + fn count_comments_between_excludes_own_comments() { + // The agent's own reply in the gap isn't "unseen" — it wrote it. + let since = parse_rfc3339("2026-06-13T10:00:00Z").unwrap(); + let current = parse_rfc3339("2026-06-13T17:01:00Z").unwrap(); + let comments = [ + serde_json::json!({ "created_at": "2026-06-13T14:53:00Z", "user": { "login": "damocles" } }), + ]; + assert_eq!( + count_comments_between(&comments, since, current, "damocles"), + 0 + ); + } + + #[test] + fn count_comments_between_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. + let since = parse_rfc3339("2026-06-13T10:00:00Z").unwrap(); + let current = parse_rfc3339("2026-06-13T17:01:00Z").unwrap(); + let comments = [ + serde_json::json!({ "created_at": "2026-06-13T17:01:00Z", "user": { "login": "mara" } }), + ]; + assert_eq!( + count_comments_between(&comments, since, current, "damocles"), + 0 + ); + } + + #[test] + fn count_comments_between_ignores_unparseable_entries() { + // A comment 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 = [ + 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"), + 0 + ); + } }