vinzenz/hyperhive
vinzenz/hyperhive
Watch
0
0
Fork
You've already forked hyperhive
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

forge_notify: flag earlier comments the latest_comment_url blurb hides

Browse source
This commit is contained in:
damocles 2026-08-19 04:19:55 +02:00 • committed by mara
commit 530eedd5f4
1 changed files with 170 additions and 1 deletions
Download patch file Download diff file Expand all files Collapse all files

171
hive-forge-notify/src/notify.rs
View file

@ -260,6 +260,7 @@ async fn format_notification<S: Source>(
source: &S,
notif: &PolledNotification,
own_login: &str,
prev_updated_at: Option<&str>,
) -> Option<String> {
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<S: Source>(
let is_fresh_state_change =
state_change_is_current(&notif.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<S: Source>(
client: &reqwest::Client,
@ -433,6 +455,7 @@ async fn format_comment_notification<S: Source>(
meta: &NotifMeta<'_>,
comment_api_url: &str,
comment_html_url: &str,
enrichment: &CommentEnrichment<'_>,
own_login: &str,
) -> Option<String> {
let payload = fetch_json(client, comment_api_url, source).await;
@ -504,11 +527,85 @@ async fn format_comment_notification<S: Source>(
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<S: Source>(
client: &reqwest::Client,
source: &S,
subject_api_url: &str,
since: &str,
current: OffsetDateTime,
own_login: &str,
) -> Option<usize> {
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<S: Source, H: std::hash::BuildHasher>(
// 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
);
}
}