Skip to content

Link past ACKs to a collapsible history section #74

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
l0rinc wants to merge 3 commits into from
Closed

Link past ACKs to a collapsible history section #74

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e3f0f99
Add test for `summary_comment_template` output validation
l0rinc Jan 10, 2026
86c7822
Extract review collection
l0rinc Jan 10, 2026
3c7e659
Link past ACKs to a collapsible history section
l0rinc Jan 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
160 changes: 119 additions & 41 deletions webhook_features/src/features/summary_comment.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ impl Feature for SummaryCommentFeature {

const BOT_SKIP_TAG: &str = "<!--meta-tag:bot-skip-->";

fn summary_comment_template(reviews: Vec<Review>) -> String {
fn summary_comment_template(reviews: Vec<Review>, history: Vec<Review>) -> String {
let review_url = "https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#code-review";
let mut comment = format!(
r#"
Expand Down Expand Up @@ -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::<Vec<_>>()
.join(", ")
);
}
}

if !history.is_empty() {
comment += "\n<details><summary>Review history</summary>\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</details>\n";
}

comment += "\n";
comment += "If your review is incorrectly listed, please copy-paste ";
comment += "<code>&lt;!--meta-tag:bot-skip--&gt;</code>" /* BOT_SKIP_TAG */;
Expand All @@ -176,13 +186,70 @@ 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,
body: String,
date: chrono::DateTime<chrono::Utc>,
}

fn collect_user_reviews(
all_comments: Vec<GitHubReviewComment>,
pr_author: &str,
head_commit: &str,
) -> (Vec<Review>, Vec<Review>) {
let mut user_reviews: HashMap<String, Vec<Review>> = 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::<Vec<_>>();
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::<Vec<_>>();

(reviews, history)
}

async fn refresh_summary_comment(
ctx: &Context,
repo: Repository,
Expand Down Expand Up @@ -283,51 +350,14 @@ For details see: https://corecheck.dev/{owner}/{repo}/pulls/{pull_num}.

let head_commit = pr.head.sha;

let mut user_reviews: HashMap<String, Vec<Review>> = HashMap::new(); // Need to store all acks per user to avoid duplicates

println!(
" ... Refresh of {num} comments from {url}.",
num = all_comments.len(),
url = pr.html_url.unwrap(),
);

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::<Vec<_>>();
let (user_reviews, history) = collect_user_reviews(all_comments, &pr_author, &head_commit);

let max_ack_date = user_reviews
.iter()
Expand Down Expand Up @@ -370,7 +400,7 @@ For details see: https://corecheck.dev/{owner}/{repo}/pulls/{pull_num}.
.map(|r| r.user.clone())
.collect::<Vec<_>>();

let comment = summary_comment_template(user_reviews);
let comment = summary_comment_template(user_reviews, history);
util::update_metadata_comment(
&issues_api,
&mut cmt,
Expand Down Expand Up @@ -518,7 +548,55 @@ async fn get_llm_check(llm_diff_pr: &str, llm_token: &str) -> Result<Vec<String>
#[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) |

<details><summary>Review history</summary>

| Type | Reviewer | Date |
| ---- | -------- | ---- |
| Concept ACK | [user](https://example.com/review/old) | 2026-01-01 |
| ACK | [user](https://example.com/review) | 2026-01-02 |

</details>

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.
"#
);
}
struct TestCase {
comment: &'static str,
expected: Option<AckCommit>,
Expand Down