forge-notify: widen earlier-activity marker to the full timeline

hive-forge-notify/src/notify.rs

@@ -375,12 +375,25 @@ async fn format_notification<S: Source>(
         } 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,
             &notif.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
+/// `NotifMeta`, only to power the "earlier unseen update" marker on the
-/// the plain-comment path — see [`count_unseen_earlier_comments`].
+/// plain-comment path — see [`fetch_unseen_earlier_events`]. The
-/// Kept as its own small struct (rather than folded into `NotifMeta`)
+/// state-change path wants the same two values, but fetches them itself in
-/// so that struct's other consumer (the state-change formatter) doesn't
+/// `format_notification` before calling the (sync) state-change formatter,
-/// have to carry fields it never uses.
+/// 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<S: Source>(
             "[{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
+        // single `latest_comment_url`, so anything else that happened on
-        // same thread since we last looked is otherwise never individually
+        // the same thread since we last looked — an earlier comment, a
-        // surfaced — the blurb silently shows only the newest one. Flag a
+        // label, a close/reopen, a review — is otherwise never individually
-        // count rather than rendering more text (see the forge-notify
+        // surfaced; the blurb silently shows only the newest comment. Flag
-        // hidden-comment issue for why).
+        // 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<S: Source>(
                 .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<S: Source>(
                 own_login,
             )
             .await;
-            if let Some(n) = unseen.filter(|&n| n > 0) {
+            out.push_str(&earlier_updates_marker(unseen));
-                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
+/// Count entries in `events` (raw JSON array from the issue/PR **timeline**
-/// comments-list endpoint) that landed strictly after `since` and
+/// endpoint — comments AND state-change events: label, assign, close/reopen,
-/// strictly before `current`, excluding any authored by `own_login`.
+/// review, `pull_push`, …) that landed strictly after `since` and strictly
-/// Pure for unit testing; [`count_unseen_earlier_comments`] does the
+/// before `current`, excluding any authored by `own_login`. Deliberately
-/// network fetch and calls this on the result.
+/// type-agnostic — every timeline entry the API returns is counted the same
-fn count_comments_between(
+/// way, per mara's review call ("count everything") rather than singling
-    comments: &[serde_json::Value],
+/// 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| {
+        .filter(|e| {
-            let created = c["created_at"].as_str().and_then(parse_rfc3339);
+            let created = e["created_at"].as_str().and_then(parse_rfc3339);
-            let author = c["user"]["login"].as_str().unwrap_or("");
+            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
+/// Fetch the subject's full timeline (comments + every state-change event)
-/// strictly between `since` (the thread's previously-delivered raw
+/// and count how many entries landed strictly between `since` (the thread's
-/// `updated_at`) and `current` (the comment now being rendered) — the
+/// previously-delivered raw `updated_at`) and `current` (the event now being
-/// comments Forgejo's one-row-per-subject `latest_comment_url` silently
+/// rendered) — everything Forgejo's one-row-per-subject notification
-/// drops between polls. `None` on any fetch/parse failure or an
+/// silently drops between polls, comment or not. `None` on any
-/// unparseable `since`; the caller just omits the marker rather than
+/// fetch/parse failure or an unparseable `since`; the caller just omits the
-/// blocking delivery on it.
+/// marker rather than blocking delivery on it.
-async fn count_unseen_earlier_comments<S: Source>(
+async fn fetch_unseen_earlier_events<S: Source>(
     client: &reqwest::Client,
     source: &S,
     subject_api_url: &str,
@@ -601,9 +616,23 @@ async fn count_unseen_earlier_comments<S: Source>(
     own_login: &str,
 ) -> Option<usize> {
     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<usize>) -> 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<String>,
+    earlier_updates: Option<usize>,
 ) -> Option<String> {
     // 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)
+        let merged_out =
-            .expect("merge notification must render");
+            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:
+    /// The widened marker (per mara's review call: "count everything") must
-    /// `count_comments_between` is the pure core of that marker.
+    /// 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() {
+    fn count_unseen_earlier_events_excludes_own_events() {
-        // The agent's own reply in the gap isn't "unseen" — it wrote it.
+        // 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() {
+    fn count_unseen_earlier_events_ignores_unparseable_entries() {
-        // A comment missing/garbling `created_at` degrades to "not in the
+        // 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
+        );
+    }
 }