diff --git a/webhook_features/src/features/summary_comment.rs b/webhook_features/src/features/summary_comment.rs index f20c888..196b87e 100644 --- a/webhook_features/src/features/summary_comment.rs +++ b/webhook_features/src/features/summary_comment.rs @@ -118,7 +118,7 @@ impl Feature for SummaryCommentFeature { const BOT_SKIP_TAG: &str = ""; -fn summary_comment_template(reviews: Vec) -> String { +fn summary_comment_template(reviews: Vec, history: Vec) -> String { let review_url = "https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#code-review"; let mut comment = format!( r#" @@ -159,13 +159,23 @@ See [the guideline]({review_url}) for information on the review process. ack_type.as_str(), users .iter() - .map(|(user, url, _)| format!("[{user}]({url})")) + .map(|(user, url, _)| format_ack(user, url)) .collect::>() .join(", ") ); } } + if !history.is_empty() { + comment += "\n
Review history\n\n"; + comment += "| Type | Reviewer | Date |\n"; + comment += "| ---- | -------- | ---- |\n"; + for Review { ack_type, user, url, date } in history { + comment += &format!("| {} | {} | {} |\n", ack_type.as_str(), format_ack(&user, &url), date.format("%F")); + } + comment += "\n
\n"; + } + comment += "\n"; comment += "If your review is incorrectly listed, please copy-paste "; comment += "<!--meta-tag:bot-skip-->" /* BOT_SKIP_TAG */; @@ -176,6 +186,8 @@ See [the guideline]({review_url}) for information on the review process. comment } +fn format_ack(user: &String, url: &String) -> String { format!("[{user}]({url})") } + struct GitHubReviewComment { user: String, url: String, @@ -183,6 +195,61 @@ struct GitHubReviewComment { date: chrono::DateTime, } +fn collect_user_reviews( + all_comments: Vec, + pr_author: &str, + head_commit: &str, +) -> (Vec, Vec) { + let mut user_reviews: HashMap> = HashMap::new(); // Need to store all acks per user to avoid duplicates + + for comment in all_comments.into_iter() { + if comment.user == pr_author { + continue; + } + if let Some(ac) = parse_review(&comment.body) { + let v = user_reviews.entry(comment.user.clone()).or_default(); + let has_current_head = ac.commit.is_some_and(|c| head_commit.starts_with(&c)); + v.push(Review { + user: comment.user.clone(), + ack_type: if comment.body.contains(BOT_SKIP_TAG) { + AckType::Ignored + } else if ac.ack_type == AckType::Ack && !has_current_head { + AckType::StaleAck + } else { + ac.ack_type + }, + url: comment.url, + date: comment.date, + }); + } + } + + let mut history = user_reviews + .values() + .flatten() + .filter(|r| r.ack_type != AckType::Ignored) + .cloned() + .collect::>(); + history.sort_by_key(|r| r.date); + + let reviews = user_reviews + .into_iter() + .map(|e| { + let e = e.1; + if let Some(ack) = e.iter().find(|r| r.ack_type == AckType::Ack) { + // Prefer ACK commit_hash over anything, to match the behavior of + // https://github.com/bitcoin-core/bitcoin-maintainer-tools/blob/f9b845614f7aecb9423d0621375e1bad17f92fde/github-merge.py#L208 + ack.clone() + } else { + // Fallback to the most recent comment, otherwise + e.into_iter().max_by_key(|r| r.date).unwrap() + } + }) + .collect::>(); + + (reviews, history) +} + async fn refresh_summary_comment( ctx: &Context, repo: Repository, @@ -283,8 +350,6 @@ For details see: https://corecheck.dev/{owner}/{repo}/pulls/{pull_num}. let head_commit = pr.head.sha; - let mut user_reviews: HashMap> = HashMap::new(); // Need to store all acks per user to avoid duplicates - println!( " ... Refresh of {num} comments from {url}.", num = all_comments.len(), @@ -292,42 +357,7 @@ For details see: https://corecheck.dev/{owner}/{repo}/pulls/{pull_num}. ); let pr_author = pr.user.unwrap().login; - for comment in all_comments.into_iter() { - if comment.user == pr_author { - continue; - } - if let Some(ac) = parse_review(&comment.body) { - let v = user_reviews.entry(comment.user.clone()).or_default(); - let has_current_head = ac.commit.is_some_and(|c| head_commit.starts_with(&c)); - v.push(Review { - user: comment.user.clone(), - ack_type: if comment.body.contains(BOT_SKIP_TAG) { - AckType::Ignored - } else if ac.ack_type == AckType::Ack && !has_current_head { - AckType::StaleAck - } else { - ac.ack_type - }, - url: comment.url, - date: comment.date, - }); - } - } - - let user_reviews = user_reviews - .into_iter() - .map(|e| { - let e = e.1; - if let Some(ack) = e.iter().find(|r| r.ack_type == AckType::Ack) { - // Prefer ACK commit_hash over anything, to match the behavior of - // https://github.com/bitcoin-core/bitcoin-maintainer-tools/blob/f9b845614f7aecb9423d0621375e1bad17f92fde/github-merge.py#L208 - ack.clone() - } else { - // Fallback to the most recent comment, otherwise - e.into_iter().max_by_key(|r| r.date).unwrap() - } - }) - .collect::>(); + let (user_reviews, history) = collect_user_reviews(all_comments, &pr_author, &head_commit); let max_ack_date = user_reviews .iter() @@ -370,7 +400,7 @@ For details see: https://corecheck.dev/{owner}/{repo}/pulls/{pull_num}. .map(|r| r.user.clone()) .collect::>(); - let comment = summary_comment_template(user_reviews); + let comment = summary_comment_template(user_reviews, history); util::update_metadata_comment( &issues_api, &mut cmt, @@ -518,7 +548,55 @@ async fn get_llm_check(llm_diff_pr: &str, llm_token: &str) -> Result #[cfg(test)] mod tests { use super::*; + use chrono::TimeZone; + #[test] + fn test_summary_comment_template_includes_history() { + let reviews = vec![Review { + user: "user".to_string(), + ack_type: AckType::Ack, + url: "https://example.com/review".to_string(), + date: chrono::Utc.with_ymd_and_hms(2026, 1, 2, 0, 0, 0).unwrap(), + }]; + + let history = vec![ + Review { + user: "user".to_string(), + ack_type: AckType::ConceptAck, + url: "https://example.com/review/old".to_string(), + date: chrono::Utc.with_ymd_and_hms(2026, 1, 1, 0, 0, 0).unwrap(), + }, + Review { + user: "user".to_string(), + ack_type: AckType::Ack, + url: "https://example.com/review".to_string(), + date: chrono::Utc.with_ymd_and_hms(2026, 1, 2, 0, 0, 0).unwrap(), + }, + ]; + + let text = summary_comment_template(reviews, history); + assert_eq!( + text, + r#" +### Reviews +See [the guideline](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#code-review) for information on the review process. +| Type | Reviewers | +| ---- | --------- | +| ACK | [user](https://example.com/review) | + +
Review history + +| Type | Reviewer | Date | +| ---- | -------- | ---- | +| Concept ACK | [user](https://example.com/review/old) | 2026-01-01 | +| ACK | [user](https://example.com/review) | 2026-01-02 | + +
+ +If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore. +"# + ); + } struct TestCase { comment: &'static str, expected: Option,