forge-notify: list inline comment locations in the wake, not their text
This commit is contained in:
damocles
mara
parent
6494c5d7f4
commit
c6958c7baa
2 changed files with 101 additions and 78 deletions
|
|
@ -303,19 +303,20 @@ with the meta-suffix `reviewer:` line (requested reviewers, below).
|
|||
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.
|
||||
the PR's reviews (`/pulls/N/reviews`, up to four pages of 50) and, for the
|
||||
reviews new to the agent, appends an
|
||||
`inline review comments (read them with `hive-forge pr reviews N`):` block
|
||||
saying where they are: one `author: path:line, path, ...` line per review
|
||||
author (bare `path` when Forgejo dropped the position because the line left
|
||||
the diff; control characters in a path become spaces). The comment text stays
|
||||
out of the wake - the agent reads the full review with the CLI. 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 and at most 10 locations are listed (`(+N more)` for the rest).
|
||||
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
|
||||
|
||||
|
|
|
|||
|
|
@ -900,10 +900,8 @@ async fn fresh_post_close_comment_tail<S: Source>(
|
|||
Some(format!("\n\ncomment by {author}: {excerpt}{mentions}"))
|
||||
}
|
||||
|
||||
/// Inline review comments rendered in one wake; the rest become a count.
|
||||
/// Inline review comment locations listed 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
|
||||
|
|
@ -911,12 +909,12 @@ const INLINE_REVIEWS_MAX: usize = 5;
|
|||
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> {
|
||||
/// The PR's reviews endpoint and number 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, &str)> {
|
||||
let (head, number) = subject_api_url.rsplit_once("/issues/")?;
|
||||
Some(format!("{head}/pulls/{number}/reviews"))
|
||||
Some((format!("{head}/pulls/{number}/reviews"), number))
|
||||
}
|
||||
|
||||
/// `(review id, author)` of the reviews in `reviews` (the raw reviews
|
||||
|
|
@ -954,40 +952,46 @@ fn unseen_inline_reviews(
|
|||
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 {
|
||||
/// One comment's location for the wake: `path:line`, or just `path` when
|
||||
/// Forgejo dropped the position because the line left the diff. Control
|
||||
/// characters in the path (git allows a newline in one) become spaces so a
|
||||
/// PR author cannot shape the lines the agent reads.
|
||||
fn comment_location(c: &serde_json::Value) -> String {
|
||||
let path: String = c["path"]
|
||||
.as_str()
|
||||
.unwrap_or("?")
|
||||
.chars()
|
||||
.map(|ch| if ch.is_control() { ' ' } else { ch })
|
||||
.collect();
|
||||
match c["position"].as_u64().filter(|&line| line > 0) {
|
||||
Some(line) => format!("{path}:{line}"),
|
||||
None => path,
|
||||
}
|
||||
}
|
||||
|
||||
/// The `inline review comments` block for the wake: only where the unseen
|
||||
/// comments are, one line per review author (`author: path:line, ...`),
|
||||
/// capped at [`INLINE_COMMENTS_MAX`] locations in total with a count for the
|
||||
/// rest. The text stays out of the wake: the pointer names the CLI call that
|
||||
/// prints the full review. Empty when there is nothing to show.
|
||||
fn render_inline_comments(pr_number: &str, 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(),
|
||||
};
|
||||
let mut out =
|
||||
format!("\n\ninline review comments (read them with `hive-forge pr reviews {pr_number}`):");
|
||||
let mut budget = INLINE_COMMENTS_MAX;
|
||||
for (author, comments) in groups {
|
||||
let shown: Vec<String> = comments.iter().take(budget).map(comment_location).collect();
|
||||
if shown.is_empty() {
|
||||
continue;
|
||||
}
|
||||
budget -= shown.len();
|
||||
write!(out, "\n {author}: {}", shown.join(", ")).ok();
|
||||
}
|
||||
if total > INLINE_COMMENTS_MAX {
|
||||
write!(
|
||||
out,
|
||||
"\n (+{} more - `hive-forge pr reviews` lists them all)",
|
||||
total - INLINE_COMMENTS_MAX
|
||||
)
|
||||
.ok();
|
||||
write!(out, "\n (+{} more)", total - INLINE_COMMENTS_MAX).ok();
|
||||
}
|
||||
out
|
||||
}
|
||||
|
|
@ -1002,7 +1006,7 @@ async fn fetch_inline_review_tail<S: Source>(
|
|||
prev_updated_at: Option<&str>,
|
||||
own_login: &str,
|
||||
) -> String {
|
||||
let Some(reviews_url) = reviews_api_url(subject_api_url) else {
|
||||
let Some((reviews_url, pr_number)) = reviews_api_url(subject_api_url) else {
|
||||
return String::new();
|
||||
};
|
||||
let mut reviews: Vec<serde_json::Value> = Vec::new();
|
||||
|
|
@ -1027,7 +1031,7 @@ async fn fetch_inline_review_tail<S: Source>(
|
|||
groups.push((author, comments.clone()));
|
||||
}
|
||||
}
|
||||
render_inline_comments(&groups)
|
||||
render_inline_comments(pr_number, &groups)
|
||||
}
|
||||
|
||||
/// Parse an RFC 3339 timestamp as Forgejo emits them
|
||||
|
|
@ -1997,8 +2001,8 @@ mod tests {
|
|||
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"),
|
||||
reviews_api_url(subject),
|
||||
Some(("/api/v1/repos/o/r/pulls/42/reviews".to_owned(), "42")),
|
||||
);
|
||||
assert_eq!(reviews_api_url("/api/v1/repos/o/r"), None);
|
||||
}
|
||||
|
|
@ -2052,39 +2056,57 @@ mod tests {
|
|||
}
|
||||
|
||||
#[test]
|
||||
fn render_inline_comments_formats_path_line_and_fallback() {
|
||||
let groups = vec![(
|
||||
fn render_inline_comments_lists_locations_not_text() {
|
||||
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"}),
|
||||
serde_json::json!({"path": "src/b.rs", "position": 0, "body": "left the diff"}),
|
||||
],
|
||||
)];
|
||||
),
|
||||
(
|
||||
"mara".to_owned(),
|
||||
vec![serde_json::json!({"path": "src/c.rs", "position": 7, "body": "why"})],
|
||||
),
|
||||
];
|
||||
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"
|
||||
render_inline_comments("12", &groups),
|
||||
"\n\ninline review comments (read them with `hive-forge pr reviews 12`):\
|
||||
\n argus: src/a.rs:42, src/b.rs\
|
||||
\n mara: src/c.rs:7"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn render_inline_comments_empty_is_empty() {
|
||||
assert_eq!(render_inline_comments(&[]), "");
|
||||
assert_eq!(render_inline_comments(&[("argus".to_owned(), vec![])]), "");
|
||||
assert_eq!(render_inline_comments("12", &[]), "");
|
||||
assert_eq!(
|
||||
render_inline_comments("12", &[("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);
|
||||
fn render_inline_comments_caps_locations_across_authors() {
|
||||
let many = |n: u64| -> Vec<serde_json::Value> {
|
||||
(1..=n)
|
||||
.map(|i| serde_json::json!({"path": "f.rs", "position": i}))
|
||||
.collect()
|
||||
};
|
||||
let out = render_inline_comments(
|
||||
"12",
|
||||
&[("argus".to_owned(), many(8)), ("mara".to_owned(), many(5))],
|
||||
);
|
||||
// 8 from argus, the 2 left of the budget from mara, 3 counted.
|
||||
assert_eq!(out.matches("f.rs:").count(), INLINE_COMMENTS_MAX);
|
||||
assert!(out.contains("\n mara: f.rs:1, f.rs:2\n"));
|
||||
assert!(out.ends_with("\n (+3 more)"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn comment_location_neutralises_control_characters_in_the_path() {
|
||||
let c = serde_json::json!({"path": "a\nb\tc.rs", "position": 3});
|
||||
assert_eq!(comment_location(&c), "a b c.rs:3");
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue