Link past ACKs to a collapsible history section

[l0rinc]

Context

Inspired by #72 (comment), this adds a dedicated review history section so maintainers can quickly inspect older ACK/NACK context without cluttering the main Reviews table.

Other attempts

I originally tried superscript links and brace-style links next to the latest ACKs, but both were too verbose and hurt readability, so the history is now a separate collapsible section.

Structure

• First commit adds a test for summary_comment_template to lock the Reviews markdown format and show the effect of each change.
• Second one unburdens the follow-up change by extracting review parsing/selection into collect_user_reviews() to isolate the review selection logic for follow-up history rendering (this also helps with the preview patch below).
• Last commit adds a collapsible Review history table listing all non-ignored review events (type, reviewer link, date).

Reproducer

See https://gist.github.com/l0rinc/484dc5670f6a79a522d39acfa4b83b11 for an example output and apply the following patch to try it with any other PR:

Patch for `--preview-reviews` that fetches a PR’s comments/reviews and prints the generated Reviews markdown

diff --git a/webhook_features/src/features/summary_comment.rs b/webhook_features/src/features/summary_comment.rs
index 7ccbc37..e81bdb9 100644
--- a/webhook_features/src/features/summary_comment.rs
+++ b/webhook_features/src/features/summary_comment.rs
@@ -178,6 +178,54 @@ struct GitHubReviewComment {
     date: chrono::DateTime<chrono::Utc>,
 }
 
+pub async fn preview_reviews_markdown(
+    ctx: &Context,
+    owner: String,
+    repo: String,
+    pr_number: u64,
+) -> Result<String> {
+    let repo = Repository { owner, name: repo };
+    let issues_api = ctx.octocrab.issues(&repo.owner, &repo.name);
+    let pulls_api = ctx.octocrab.pulls(&repo.owner, &repo.name);
+    let pr = pulls_api.get(pr_number).await?;
+
+    let all_comments = ctx
+        .octocrab
+        .all_pages(issues_api.list_comments(pr_number).send().await?)
+        .await?;
+    let cmt = util::get_metadata_sections_from_comments(&all_comments, pr_number);
+
+    let mut all_comments = all_comments
+        .into_iter()
+        .filter(|c| cmt.id != Some(c.id))
+        .map(|c| GitHubReviewComment {
+            user: c.user.login,
+            url: c.html_url.to_string(),
+            body: c.body.unwrap_or_default(),
+            date: c.updated_at.unwrap_or(c.created_at),
+        })
+        .collect::<Vec<_>>();
+    let mut all_review_comments = ctx
+        .octocrab
+        .all_pages(pulls_api.list_reviews(pr_number).send().await?)
+        .await?
+        .into_iter()
+        .filter(|c| c.user.is_some())
+        .map(|c| GitHubReviewComment {
+            user: c.user.unwrap().login,
+            url: c.html_url.to_string(),
+            body: c.body.unwrap_or_default(),
+            date: c.submitted_at.unwrap(),
+        })
+        .collect::<Vec<_>>();
+    all_comments.append(&mut all_review_comments);
+
+    let head_commit = pr.head.sha;
+    let pr_author = pr.user.unwrap().login;
+    let user_reviews = collect_user_reviews(all_comments, &pr_author, &head_commit);
+    Ok(summary_comment_template(user_reviews))
+}
+
 fn collect_user_reviews(
     all_comments: Vec<GitHubReviewComment>,
     pr_author: &str,
diff --git a/webhook_features/src/main.rs b/webhook_features/src/main.rs
index 583dc84..4d185f2 100644
--- a/webhook_features/src/main.rs
+++ b/webhook_features/src/main.rs
@@ -35,10 +35,13 @@ struct Args {
     port: u16,
     /// The path to the yaml config file.
     #[arg(long)]
-    config_file: std::path::PathBuf,
+    config_file: Option<std::path::PathBuf>,
     /// Print changes/edits instead of calling the GitHub/CI API.
     #[arg(long, default_value_t = false)]
     dry_run: bool,
+    /// Print the generated Reviews markdown for this pull request and exit. Format: owner/repo/number or https://github.com/owner/repo/pull/number
+    #[arg(long)]
+    preview_reviews: Option<String>,
 }
 
 #[derive(Display, EnumString, PartialEq, Eq, Hash)]
@@ -146,20 +149,71 @@ async fn emit_event(ctx: &Context, event: GitHubEvent, data: web::Json<serde_jso
     num_errors
 }
 
+fn parse_pull_id(s: &str) -> Result<(String, String, u64)> {
+    let err = "Wrong format, expected owner/repo/number or a GitHub PR URL.";
+    let s = s
+        .trim()
+        .split('#')
+        .next()
+        .unwrap_or("")
+        .split('?')
+        .next()
+        .unwrap_or("")
+        .trim_end_matches('/');
+    let s = s
+        .strip_prefix("https://github.com/")
+        .or_else(|| s.strip_prefix("http://github.com/"))
+        .unwrap_or(s);
+
+    let parts = s.split('/').filter(|p| !p.is_empty()).collect::<Vec<_>>();
+    let (owner, repo, number) = match parts.as_slice() {
+        [owner, repo, "pull" | "pulls", number] | [owner, repo, number] => (owner, repo, number),
+        _ => return Err(anyhow::anyhow!(err)),
+    };
+
+    Ok((owner.to_string(), repo.to_string(), number.parse::<u64>()?))
+}
+
 #[actix_web::main]
 async fn main() -> Result<()> {
     let args = Args::parse();
 
-    let config: Config = serde_yaml::from_reader(
-        std::fs::File::open(args.config_file).expect("config file path error"),
-    )
-    .expect("yaml error");
-
     let octocrab = octocrab::Octocrab::builder()
         .personal_token(args.token.as_ref())
         .build()
         .map_err(DrahtBotError::GitHubError)?;
 
+    if let Some(pull_id) = args.preview_reviews {
+        let (owner, repo, number) = parse_pull_id(&pull_id)?;
+        let context = Context {
+            octocrab,
+            bot_username: "DrahtBot".to_string(),
+            config: Config {
+                repositories: Vec::new(),
+            },
+            github_token: args.token,
+            llm_token: args.llm_token,
+            dry_run: args.dry_run,
+        };
+        let md = crate::features::summary_comment::preview_reviews_markdown(
+            &context,
+            owner,
+            repo,
+            number,
+        )
+        .await?;
+        print!("{md}");
+        return Ok(());
+    }
+
+    let config_file = args
+        .config_file
+        .expect("--config-file is required unless --preview-reviews is used");
+    let config: Config = serde_yaml::from_reader(
+        std::fs::File::open(config_file).expect("config file path error"),
+    )
+    .expect("yaml error");
+
     println!("{}", list_features());
     println!();

After applying locally, you can test it with: e.g.

(cd webhook_features && cargo run -- --token "$(gh auth token)" --preview-reviews https://github.com/bitcoin/bitcoin/pull/29415)

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	fanquake
Approach NACK	maflcko
Stale ACK	ryanofsky

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

[maflcko]

Seems fine, but the use-case seems also very limited: Solely to refer to the earlier ACKs for the possibility of them containing relevant information.

I think, if there is any relevant information, worthy to be mentioned or referred to again, it seems easier to just refer to it or mention it again. This will also make re-reviews less boring. For example,

instead of a plain

re-ACK ffaa00

there could be a

re-ACK ffaa00 As mentioned in my initial review, the test changes are nice, and I tested them again in the same way

or

re-ACK ffaa00 Though, my test nit(s) may still be good to consider to be addressed here or in a follow-up.

Generally, not everyone puts relevant information into their review comment, so this feature could also be misleading. And if someone wanted to re-evaluate all relevant (review) comments, there is probably no easy way out, other than to read all comments.

No objected, but I think the use-case is very limited.

[ryanofsky]

Thanks for implementing this! I feel the collapsible history table in shown https://gist.github.com/l0rinc/484dc5670f6a79a522d39acfa4b83b11 is not very usable, though.

The problem I have is that before I merge a PR I want to look at previous review comments and make sure earlier concerns have been addressed. And often when I'm reviewing a PR I remember a person previously giving high level feedback but it is hard to find again because there is no easy way to find prior reviews from some person, so I wind up ctrl-f'ing through duplicate comments or searching gmail to scan the reviewer's posts chronologically.

To use the example in the gist, I simply want to replace:

Type	Reviewers
ACK	optout21, hodlinator
Stale ACK	yuvicc, vasild

with

Type	Reviewers
ACK	optout21, hodlinator [0]
Stale ACK	yuvicc [0], vasild

Simply appending each persons latest review with links to their previous reviews as [0][1][2] links.

I don't think this would add much noise, and I don't care about dates or types of previous reviews, I just want links to follow progression of a reviewers opinion and see if they have given meaningful higher level feedback. This is difficult currently because the current UI drops links to more substantive reviews when reviewers reack.

[l0rinc]

that's what I have attempted before, it's what I meant by:

I originally tried superscript links and brace-style links next to the latest ACKs, but both were too verbose and hurt readability, so the history is now a separate collapsible section.

Let me reimplement that and let's see if that's more useful.

[l0rinc]

@ryanofsky this is what I tried originally and decided against it - do you think this would be more useful?
Note that I have numbered them in reverse order to have stable indexes (adding a new ACK shouldn't change what [2] points to, and this way the list is in descending temporal order):

Type	Reviewers
ACK	pinheadmz [5][4][3][2][1][0], l0rinc [4][3][2][1][0], andrewtoth [10][9][8][7][6][5][4][3][2][1][0], w0xlt, mzumsande [3][2][1][0], janb84
Concept ACK	zzzi2p, nothingmuch, jonatack, kdmukai, kevkevinpal, RandyMcMillan, naiyoma, ajtowns, danielabrozzoni

---

Also, what do you think of the unresolved threads section in https://gist.github.com/l0rinc/484dc5670f6a79a522d39acfa4b83b11?permalink_comment_id=5937556#gistcomment-5937556?

[ryanofsky]

Thanks, that looks almost perfect to me, and the unresolved threads seems like a great idea, I really like it.

IMO, the previous review output shown above is not too noisy. It seems readable and helpful, and this is an extreme example in a PR open for 2 years with 827 comments. Almost any other PR would have a lot fewer previous reviews.

I do think previous review links would be more useful in ascending order. The first review [0] is likely to be a lot more substantive than the second-to-last review and if you are reading the reviews it makes sense to read them in order. Also [0][1][2] looks less strange than [2][1][0], IMO. To be clear, I don't think the existing links to current reviews should be changed, the suggestion is only to add links to previous reviews afterwards as convenient way to see and find them.

I do really like the "unresolved threads" table and just have to 2 suggestions on that:

• Would be good to drop the author column and have author name next to the comment like "mzumsande: First I thought this was not necessary because di…", instead of 2 columns over.
• Would be good to drop the links to the author github profiles. The links are distracting and unlikely to be helpful.

[l0rinc]

I will push those suggestions, thanks for the feedback.

The first review [0] is likely to be a lot more substantive than the second-to-last review and if you are reading the reviews it makes sense to read them in order

Beyond stability ([0] is always the first ACK, [1] always the second, even when a new ACK is added), the reverse ordering keeps the link list temporally consistent.

If the bracket links are shown in ascending order, the visual order becomes inconsistent:
fourth_ACK [first_ACK][second_ACK][third_ACK].
That is neither chronological nor reverse-chronological, and it is easy to misread.

With stable indices (oldest = [0]) but reverse display, you get: name -> latest, then [2][1][0] for previous reviews - monotonic in time and no renumbering when a new ACK comes in.

[fanquake]

This seems pretty noisey. Looks like a lot of the [0] are also being miscounted as ACK, when they are actually Concept ACK?

[maflcko]

I do really like the "unresolved threads" table and just have to 2 suggestions on that:

I wonder if such a table will be counter productive, because:

• reviewers may be less likely to start review, as long as there are unresolved threads
• the author is encouraged to close threads early to "silently sweep them under the carpet". This already happens (Added rescan option for import descriptors bitcoin/bitcoin#31668 (comment), and in other pulls)

Moreover, not all threads must be "solved". There are sometimes threads which contain tangential fun facts, or make review easier. Sure, those comments could be put in the commit message, or a code comment, but sometimes they are a thread, which I think is fine.

Personally I think the workflow for closing threads should be:

• Reviewer leaves a comment
• Author addresses it, but leaves the comment open
• Reviewer re-reviews and does not re-raise the concern
• Author may or may not close the thread

[l0rinc]

Thanks for the reviews!

reviewers may be less likely to start review, as long as there are unresolved threads

Wouldn't that motivate the author to be more responsive?

silently sweep them under the carpet

The reviewer can comment on those treads if this is weaponized.

not all threads must be "solved".

Sure, they will appear in the list and the maintainer can ignore them. Isn't this better than having unaddressed changes that the maintainers miss?

Looks like a lot of the [0] are also being miscounted as ACK, when they are actually Concept ACK?

Possible, I haven't pushed the latest version since we don't seem to have a consensus on how to do this in a way that maintainers find useful.

[maflcko]

not all threads must be "solved".

Sure, they will appear in the list and the maintainer can ignore them. Isn't this better than having unaddressed changes that the maintainers miss?

I think you answer your own questions: Not all requested changes must be addressed and some can be ignored. There are already different ack-types for reviewers to pick from to indicate if the code is rfm from their perspective, of if something should be addressed. I don't think there is anything wrong by saying "i like the changes, but there is a major flaw in the approach (see inline comment) -> Approach NACK". If a maintainer (and later reviewers) are missing an approach NACK, then I don't think this feature will help them.

Conversely, if there is just a minor (style) nit, or non-blocking comment, or follow-up idea, it is also trivial to mention in the review comment (#74 (comment))

I expect that most maintainers will skim the review comments and the last few comments on the thread, so the chance that they are missing something should be slim.

[maflcko]

(Still no objection to merging this, especially given that one maintainer likes it. I am just curious why the status quo is insufficient or bad, or where it has failed in the past)