forge-notify: show unseen inline review comments in PR wakes
This commit is contained in:
parent
ff3d119c63
commit
6494c5d7f4
2 changed files with 315 additions and 33 deletions
|
|
@ -298,6 +298,25 @@ A review submitted with **no body** renders `reviewed by: <author>` in
|
||||||
place of the `<author>: <body>` line — deliberately worded to not collide
|
place of the `<author>: <body>` line — deliberately worded to not collide
|
||||||
with the meta-suffix `reviewer:` line (requested reviewers, below).
|
with the meta-suffix `reviewer:` line (requested reviewers, below).
|
||||||
|
|
||||||
|
### Inline review comments
|
||||||
|
|
||||||
|
A review's line comments belong to neither the review body nor the
|
||||||
|
`latest_comment_url` (a review with only inline comments has no such url
|
||||||
|
and otherwise lands as a bare `activity on PR`). So every PR wake fetches
|
||||||
|
the PR's reviews (`/pulls/N/reviews`, up to four pages of 50) and appends
|
||||||
|
the inline comments of those that are new to the agent as an
|
||||||
|
`inline review comments:` block, one `author [path:line]: text` entry
|
||||||
|
each (`[path]` when Forgejo dropped the position because the line left the
|
||||||
|
diff). A review counts as new when it was submitted strictly after the
|
||||||
|
thread's previously delivered `updated_at` (all of them without a
|
||||||
|
baseline, and any with no submit time, so nothing is hidden), is not the
|
||||||
|
agent's own, is not a `PENDING` draft, and has `comments_count > 0`. Only
|
||||||
|
the newest 5 such reviews are fetched, at most 10 comments are shown
|
||||||
|
(`(+N more ...)` for the rest), and each excerpt is truncated to 300 bytes
|
||||||
|
and heading-escaped like a body. Best effort: a failed fetch yields less
|
||||||
|
text, never a dropped wake. Cost: one reviews fetch per PR wake, plus one
|
||||||
|
per new commented review.
|
||||||
|
|
||||||
### Merge/close vs a later comment
|
### Merge/close vs a later comment
|
||||||
|
|
||||||
A notification carrying a `latest_comment_url` normally takes the comment
|
A notification carrying a `latest_comment_url` normally takes the comment
|
||||||
|
|
|
||||||
|
|
@ -361,7 +361,7 @@ async fn format_notification<S: Source>(
|
||||||
// comment stays on the comment path (see `state_change_is_current`).
|
// comment stays on the comment path (see `state_change_is_current`).
|
||||||
let is_fresh_state_change =
|
let is_fresh_state_change =
|
||||||
state_change_is_current(¬if.state, notif.thread.updated_at, meta.subject.as_ref());
|
state_change_is_current(¬if.state, notif.thread.updated_at, meta.subject.as_ref());
|
||||||
if has_comment && !is_fresh_state_change {
|
let formatted = if has_comment && !is_fresh_state_change {
|
||||||
let enrichment = CommentEnrichment {
|
let enrichment = CommentEnrichment {
|
||||||
subject_api_url,
|
subject_api_url,
|
||||||
prev_updated_at,
|
prev_updated_at,
|
||||||
|
|
@ -377,38 +377,16 @@ async fn format_notification<S: Source>(
|
||||||
)
|
)
|
||||||
.await
|
.await
|
||||||
} else {
|
} else {
|
||||||
// State-change path (merge/close/new/activity). A just-merged or
|
let (comment_tail, earlier_updates) = state_change_extras(
|
||||||
// closed subject keeps its pre-merge last comment on
|
|
||||||
// `latest_comment_url`, so `has_comment` is often true here. When
|
|
||||||
// that comment was genuinely posted AFTER the close — a comment
|
|
||||||
// racing the merge inside the state-change tolerance window — append
|
|
||||||
// it so it isn't lost (best of both worlds; see
|
|
||||||
// `docs/integrations/forge.md::Merge racing a comment`). The ordinary pre-merge
|
|
||||||
// last comment (created before `closed_at`) is left off.
|
|
||||||
let comment_tail = if has_comment {
|
|
||||||
fresh_post_close_comment_tail(
|
|
||||||
client,
|
client,
|
||||||
source,
|
source,
|
||||||
comment_api_url,
|
has_comment.then_some(comment_api_url),
|
||||||
meta.subject.as_ref(),
|
&meta,
|
||||||
|
subject_api_url,
|
||||||
|
prev_updated_at.zip(notif.thread.updated_at),
|
||||||
own_login,
|
own_login,
|
||||||
)
|
)
|
||||||
.await
|
.await;
|
||||||
} 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(
|
format_state_change_notification(
|
||||||
notif.thread.updated_at,
|
notif.thread.updated_at,
|
||||||
¬if.state,
|
¬if.state,
|
||||||
|
|
@ -417,7 +395,57 @@ async fn format_notification<S: Source>(
|
||||||
comment_tail,
|
comment_tail,
|
||||||
earlier_updates,
|
earlier_updates,
|
||||||
)
|
)
|
||||||
|
};
|
||||||
|
// A review's inline comments ride neither the review body nor the
|
||||||
|
// comment url (a bodiless review has no `latest_comment_url` at all),
|
||||||
|
// so every PR wake gets the unseen ones appended.
|
||||||
|
let mut out = formatted?;
|
||||||
|
if is_pr {
|
||||||
|
out.push_str(
|
||||||
|
&fetch_inline_review_tail(client, source, subject_api_url, prev_updated_at, own_login)
|
||||||
|
.await,
|
||||||
|
);
|
||||||
}
|
}
|
||||||
|
Some(out)
|
||||||
|
}
|
||||||
|
|
||||||
|
/// The two pre-fetches the sync state-change formatter needs.
|
||||||
|
///
|
||||||
|
/// State-change path (merge/close/new/activity). A just-merged or
|
||||||
|
/// closed subject keeps its pre-merge last comment on
|
||||||
|
/// `latest_comment_url`, so a comment url is often present here. When
|
||||||
|
/// that comment was genuinely posted AFTER the close - a comment
|
||||||
|
/// racing the merge inside the state-change tolerance window - it is
|
||||||
|
/// appended so it isn't lost (best of both worlds; see
|
||||||
|
/// `docs/integrations/forge.md::Merge racing a comment`). The ordinary
|
||||||
|
/// pre-merge last comment (created before `closed_at`) is left off.
|
||||||
|
/// The earlier-updates count is the same widened marker as the comment
|
||||||
|
/// path; both are fetched here because `format_state_change_notification`
|
||||||
|
/// stays sync.
|
||||||
|
async fn state_change_extras<S: Source>(
|
||||||
|
client: &reqwest::Client,
|
||||||
|
source: &S,
|
||||||
|
comment_api_url: Option<&str>,
|
||||||
|
meta: &NotifMeta<'_>,
|
||||||
|
subject_api_url: &str,
|
||||||
|
since_and_current: Option<(&str, OffsetDateTime)>,
|
||||||
|
own_login: &str,
|
||||||
|
) -> (Option<String>, Option<usize>) {
|
||||||
|
let comment_tail = match comment_api_url {
|
||||||
|
Some(url) => {
|
||||||
|
fresh_post_close_comment_tail(client, source, url, meta.subject.as_ref(), own_login)
|
||||||
|
.await
|
||||||
|
}
|
||||||
|
None => None,
|
||||||
|
};
|
||||||
|
let earlier_updates = match since_and_current {
|
||||||
|
Some((since, current)) => {
|
||||||
|
fetch_unseen_earlier_events(client, source, subject_api_url, since, current, own_login)
|
||||||
|
.await
|
||||||
|
}
|
||||||
|
None => None,
|
||||||
|
};
|
||||||
|
(comment_tail, earlier_updates)
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Shared notification metadata extracted from the polled notification.
|
/// Shared notification metadata extracted from the polled notification.
|
||||||
|
|
@ -872,6 +900,136 @@ async fn fresh_post_close_comment_tail<S: Source>(
|
||||||
Some(format!("\n\ncomment by {author}: {excerpt}{mentions}"))
|
Some(format!("\n\ncomment by {author}: {excerpt}{mentions}"))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Inline review comments rendered in one wake; the rest become a count.
|
||||||
|
const INLINE_COMMENTS_MAX: usize = 10;
|
||||||
|
/// Per-comment excerpt cap. Under [`BODY_TRUNCATE`]: one review carries many.
|
||||||
|
const INLINE_COMMENT_TRUNCATE: usize = 300;
|
||||||
|
/// Reviews whose inline comments are fetched for one wake (the newest).
|
||||||
|
const INLINE_REVIEWS_MAX: usize = 5;
|
||||||
|
/// Page size and page cap for the reviews list, so a long-lived PR's
|
||||||
|
/// newest reviews are not stranded behind the first page.
|
||||||
|
const REVIEWS_PAGE_SIZE: usize = 50;
|
||||||
|
const REVIEWS_PAGES_MAX: usize = 4;
|
||||||
|
|
||||||
|
/// The PR's reviews endpoint from the notification subject URL. Forgejo
|
||||||
|
/// hands out the *issues* URL for a PR (`.../issues/N`); its reviews live
|
||||||
|
/// under `.../pulls/N/reviews`. `None` for any other shape.
|
||||||
|
fn reviews_api_url(subject_api_url: &str) -> Option<String> {
|
||||||
|
let (head, number) = subject_api_url.rsplit_once("/issues/")?;
|
||||||
|
Some(format!("{head}/pulls/{number}/reviews"))
|
||||||
|
}
|
||||||
|
|
||||||
|
/// `(review id, author)` of the reviews in `reviews` (the raw reviews
|
||||||
|
/// list, oldest first) that carry inline comments the agent has not been
|
||||||
|
/// woken for: submitted strictly after `since` (all of them without a
|
||||||
|
/// baseline, and any whose time is missing, so nothing is hidden), not
|
||||||
|
/// the agent's own, and not an unsubmitted `PENDING` draft. Newest
|
||||||
|
/// [`INLINE_REVIEWS_MAX`] only.
|
||||||
|
fn unseen_inline_reviews(
|
||||||
|
reviews: &[serde_json::Value],
|
||||||
|
since: Option<OffsetDateTime>,
|
||||||
|
own_login: &str,
|
||||||
|
) -> Vec<(i64, String)> {
|
||||||
|
let mut unseen: Vec<(i64, String)> = reviews
|
||||||
|
.iter()
|
||||||
|
.filter(|r| r["comments_count"].as_i64().unwrap_or(0) > 0)
|
||||||
|
.filter(|r| r["state"].as_str() != Some("PENDING"))
|
||||||
|
.filter(|r| {
|
||||||
|
let author = r["user"]["login"].as_str().unwrap_or("");
|
||||||
|
own_login.is_empty() || author != own_login
|
||||||
|
})
|
||||||
|
.filter(
|
||||||
|
|r| match (since, r["submitted_at"].as_str().and_then(parse_rfc3339)) {
|
||||||
|
(Some(since), Some(at)) => at > since,
|
||||||
|
_ => true,
|
||||||
|
},
|
||||||
|
)
|
||||||
|
.filter_map(|r| {
|
||||||
|
let id = r["id"].as_i64()?;
|
||||||
|
Some((id, r["user"]["login"].as_str().unwrap_or("?").to_owned()))
|
||||||
|
})
|
||||||
|
.collect();
|
||||||
|
let skip = unseen.len().saturating_sub(INLINE_REVIEWS_MAX);
|
||||||
|
unseen.drain(..skip);
|
||||||
|
unseen
|
||||||
|
}
|
||||||
|
|
||||||
|
/// The `inline review comments:` block for the wake: one
|
||||||
|
/// `author [path:line]: text` entry per comment (`[path]` when Forgejo
|
||||||
|
/// dropped the position because the line left the diff), each excerpt
|
||||||
|
/// truncated + heading-escaped like a body, at most
|
||||||
|
/// [`INLINE_COMMENTS_MAX`] entries and a count for the rest. Empty when
|
||||||
|
/// there is nothing to show.
|
||||||
|
fn render_inline_comments(groups: &[(String, Vec<serde_json::Value>)]) -> String {
|
||||||
|
let total: usize = groups.iter().map(|(_, c)| c.len()).sum();
|
||||||
|
if total == 0 {
|
||||||
|
return String::new();
|
||||||
|
}
|
||||||
|
let mut out = String::from("\n\ninline review comments:");
|
||||||
|
let entries = groups
|
||||||
|
.iter()
|
||||||
|
.flat_map(|(author, comments)| comments.iter().map(move |c| (author, c)));
|
||||||
|
for (author, c) in entries.take(INLINE_COMMENTS_MAX) {
|
||||||
|
let path = c["path"].as_str().unwrap_or("?");
|
||||||
|
let body = escape_md_headings(&truncate(
|
||||||
|
c["body"].as_str().unwrap_or("").trim(),
|
||||||
|
INLINE_COMMENT_TRUNCATE,
|
||||||
|
))
|
||||||
|
.replace('\n', "\n ");
|
||||||
|
match c["position"].as_u64().filter(|&line| line > 0) {
|
||||||
|
Some(line) => write!(out, "\n {author} [{path}:{line}]: {body}").ok(),
|
||||||
|
None => write!(out, "\n {author} [{path}]: {body}").ok(),
|
||||||
|
};
|
||||||
|
}
|
||||||
|
if total > INLINE_COMMENTS_MAX {
|
||||||
|
write!(
|
||||||
|
out,
|
||||||
|
"\n (+{} more - `hive-forge pr reviews` lists them all)",
|
||||||
|
total - INLINE_COMMENTS_MAX
|
||||||
|
)
|
||||||
|
.ok();
|
||||||
|
}
|
||||||
|
out
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Fetch + render the unseen inline review comments of a PR (see
|
||||||
|
/// [`unseen_inline_reviews`]). Best effort like every other enrichment:
|
||||||
|
/// any failed fetch just yields less (or no) text, never a dropped wake.
|
||||||
|
async fn fetch_inline_review_tail<S: Source>(
|
||||||
|
client: &reqwest::Client,
|
||||||
|
source: &S,
|
||||||
|
subject_api_url: &str,
|
||||||
|
prev_updated_at: Option<&str>,
|
||||||
|
own_login: &str,
|
||||||
|
) -> String {
|
||||||
|
let Some(reviews_url) = reviews_api_url(subject_api_url) else {
|
||||||
|
return String::new();
|
||||||
|
};
|
||||||
|
let mut reviews: Vec<serde_json::Value> = Vec::new();
|
||||||
|
for page in 1..=REVIEWS_PAGES_MAX {
|
||||||
|
let url = format!("{reviews_url}?page={page}&limit={REVIEWS_PAGE_SIZE}");
|
||||||
|
let Some(batch) = fetch_json(client, &url, source).await else {
|
||||||
|
break;
|
||||||
|
};
|
||||||
|
let Some(batch) = batch.as_array() else {
|
||||||
|
break;
|
||||||
|
};
|
||||||
|
reviews.extend(batch.iter().cloned());
|
||||||
|
if batch.len() < REVIEWS_PAGE_SIZE {
|
||||||
|
break;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
let since = prev_updated_at.and_then(parse_rfc3339);
|
||||||
|
let mut groups = Vec::new();
|
||||||
|
for (id, author) in unseen_inline_reviews(&reviews, since, own_login) {
|
||||||
|
let comments = fetch_json(client, &format!("{reviews_url}/{id}/comments"), source).await;
|
||||||
|
if let Some(comments) = comments.as_ref().and_then(serde_json::Value::as_array) {
|
||||||
|
groups.push((author, comments.clone()));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
render_inline_comments(&groups)
|
||||||
|
}
|
||||||
|
|
||||||
/// Parse an RFC 3339 timestamp as Forgejo emits them
|
/// Parse an RFC 3339 timestamp as Forgejo emits them
|
||||||
/// (`2026-06-13T11:18:42+02:00` or `...Z`, optionally with fractional
|
/// (`2026-06-13T11:18:42+02:00` or `...Z`, optionally with fractional
|
||||||
/// seconds). Returns `None` on any shape `time` doesn't recognise so
|
/// seconds). Returns `None` on any shape `time` doesn't recognise so
|
||||||
|
|
@ -1824,4 +1982,109 @@ mod tests {
|
||||||
2
|
2
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
fn review(id: i64, user: &str, state: &str, n: i64, at: &str) -> serde_json::Value {
|
||||||
|
serde_json::json!({
|
||||||
|
"id": id,
|
||||||
|
"user": {"login": user},
|
||||||
|
"state": state,
|
||||||
|
"comments_count": n,
|
||||||
|
"submitted_at": at,
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn reviews_api_url_swaps_issues_for_pulls() {
|
||||||
|
let subject = "/api/v1/repos/o/r/issues/42"; // lint:allow test input, not a tracker reference
|
||||||
|
assert_eq!(
|
||||||
|
reviews_api_url(subject).as_deref(),
|
||||||
|
Some("/api/v1/repos/o/r/pulls/42/reviews"),
|
||||||
|
);
|
||||||
|
assert_eq!(reviews_api_url("/api/v1/repos/o/r"), None);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn unseen_inline_reviews_keeps_only_new_foreign_submitted_commented() {
|
||||||
|
let since = parse_rfc3339("2026-10-30T10:00:00Z");
|
||||||
|
let reviews = vec![
|
||||||
|
review(1, "argus", "COMMENT", 2, "2026-10-30T09:00:00Z"), // seen
|
||||||
|
review(2, "argus", "COMMENT", 0, "2026-10-30T11:00:00Z"), // no inline
|
||||||
|
review(3, "me", "COMMENT", 3, "2026-10-30T11:00:00Z"), // own
|
||||||
|
review(4, "argus", "PENDING", 1, "2026-10-30T11:00:00Z"), // draft
|
||||||
|
review(5, "argus", "REQUEST_CHANGES", 1, "2026-10-30T12:00:00Z"),
|
||||||
|
];
|
||||||
|
assert_eq!(
|
||||||
|
unseen_inline_reviews(&reviews, since, "me"),
|
||||||
|
vec![(5, "argus".to_owned())]
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn unseen_inline_reviews_without_baseline_or_time_hides_nothing() {
|
||||||
|
let reviews = vec![
|
||||||
|
review(1, "argus", "COMMENT", 1, "2026-10-30T09:00:00Z"),
|
||||||
|
serde_json::json!({"id": 2, "user": {"login": "argus"}, "state": "COMMENT", "comments_count": 1}),
|
||||||
|
];
|
||||||
|
let ids: Vec<i64> = unseen_inline_reviews(&reviews, None, "me")
|
||||||
|
.into_iter()
|
||||||
|
.map(|(id, _)| id)
|
||||||
|
.collect();
|
||||||
|
assert_eq!(ids, vec![1, 2]);
|
||||||
|
// A missing submit time is shown even against a baseline.
|
||||||
|
let since = parse_rfc3339("2026-10-30T10:00:00Z");
|
||||||
|
let ids: Vec<i64> = unseen_inline_reviews(&reviews, since, "me")
|
||||||
|
.into_iter()
|
||||||
|
.map(|(id, _)| id)
|
||||||
|
.collect();
|
||||||
|
assert_eq!(ids, vec![2]);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn unseen_inline_reviews_keeps_the_newest_few() {
|
||||||
|
let reviews: Vec<_> = (1..=8)
|
||||||
|
.map(|i| review(i, "argus", "COMMENT", 1, "2026-10-30T11:00:00Z"))
|
||||||
|
.collect();
|
||||||
|
let ids: Vec<i64> = unseen_inline_reviews(&reviews, None, "me")
|
||||||
|
.into_iter()
|
||||||
|
.map(|(id, _)| id)
|
||||||
|
.collect();
|
||||||
|
assert_eq!(ids, vec![4, 5, 6, 7, 8]);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn render_inline_comments_formats_path_line_and_fallback() {
|
||||||
|
let groups = vec![(
|
||||||
|
"argus".to_owned(),
|
||||||
|
vec![
|
||||||
|
serde_json::json!({"path": "src/a.rs", "position": 42, "body": "off by one"}),
|
||||||
|
serde_json::json!({"path": "src/b.rs", "position": 0, "body": "line left the diff"}),
|
||||||
|
serde_json::json!({"path": "src/c.rs", "body": "# heading\nsecond line"}),
|
||||||
|
],
|
||||||
|
)];
|
||||||
|
assert_eq!(
|
||||||
|
render_inline_comments(&groups),
|
||||||
|
"\n\ninline review comments:\
|
||||||
|
\n argus [src/a.rs:42]: off by one\
|
||||||
|
\n argus [src/b.rs]: line left the diff\
|
||||||
|
\n argus [src/c.rs]: \\# heading\n second line"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn render_inline_comments_empty_is_empty() {
|
||||||
|
assert_eq!(render_inline_comments(&[]), "");
|
||||||
|
assert_eq!(render_inline_comments(&[("argus".to_owned(), vec![])]), "");
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn render_inline_comments_caps_entries_and_excerpts() {
|
||||||
|
let comments: Vec<_> = (0..13)
|
||||||
|
.map(|i| serde_json::json!({"path": "f.rs", "position": i + 1, "body": "x".repeat(1000)}))
|
||||||
|
.collect();
|
||||||
|
let out = render_inline_comments(&[("argus".to_owned(), comments)]);
|
||||||
|
assert_eq!(out.matches("argus [f.rs:").count(), INLINE_COMMENTS_MAX);
|
||||||
|
assert!(out.ends_with("(+3 more - `hive-forge pr reviews` lists them all)"));
|
||||||
|
// Each excerpt is cut, not the full 1000 chars.
|
||||||
|
assert!(out.len() < INLINE_COMMENTS_MAX * (INLINE_COMMENT_TRUNCATE + 60) + 200);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue