Merge pull request #2163 from Urgau/gh-changes-since

Urgau · web-flow · commit 53704182965d · 2025-08-19T19:58:17.000Z
Update review body with link to the changes since the review
diff --git a/src/config.rs b/src/config.rs
@@ -49,6 +49,7 @@ pub(crate) struct Config {
     pub(crate) behind_upstream: Option<BehindUpstreamConfig>,
     pub(crate) backport: Option<BackportConfig>,
     pub(crate) range_diff: Option<RangeDiffConfig>,
+    pub(crate) review_changes_since: Option<ReviewChangesSinceConfig>,
 }
 
 #[derive(PartialEq, Eq, Debug, serde::Deserialize)]
@@ -563,6 +564,12 @@ pub(crate) struct BackportRuleConfig {
 #[serde(deny_unknown_fields)]
 pub(crate) struct RangeDiffConfig {}
 
+/// Configuration for the changes since link on review body
+#[derive(Default, PartialEq, Eq, Debug, serde::Deserialize)]
+#[serde(rename_all = "kebab-case")]
+#[serde(deny_unknown_fields)]
+pub(crate) struct ReviewChangesSinceConfig {}
+
 fn get_cached_config(repo: &str) -> Option<Result<Arc<Config>, ConfigurationError>> {
     let cache = CONFIG_CACHE.read().unwrap();
     cache.get(repo).and_then(|(config, fetch_time)| {
@@ -704,6 +711,8 @@ mod tests {
             add-labels = ["stable-nominated"]
 
             [range-diff]
+
+            [review-changes-since]
         "#;
         let config = toml::from_str::<Config>(&config).unwrap();
         let mut ping_teams = HashMap::new();
@@ -795,6 +804,7 @@ mod tests {
                 }),
                 backport: Some(backport_team_config),
                 range_diff: Some(RangeDiffConfig {}),
+                review_changes_since: Some(ReviewChangesSinceConfig {}),
             }
         );
     }
@@ -884,6 +894,7 @@ mod tests {
                 }),
                 backport: None,
                 range_diff: None,
+                review_changes_since: None,
             }
         );
     }
diff --git a/src/gh_changes_since.rs b/src/gh_changes_since.rs
@@ -0,0 +1,64 @@
+use std::sync::Arc;
+
+use anyhow::Context as _;
+use axum::{
+    extract::{Path, State},
+    response::{IntoResponse, Redirect, Response},
+};
+use hyper::StatusCode;
+
+use crate::{
+    github,
+    handlers::Context,
+    utils::{AppError, is_repo_autorized},
+};
+
+/// Redirects to either `/gh-range-diff` (when the base changed) or to GitHub's compare
+/// page (when the base is the same).
+///
+/// Takes an PR number and an `oldbase..oldhead` representing the range we are starting from.
+pub async fn gh_changes_since(
+    Path((owner, repo, pr_num, oldbasehead)): Path<(String, String, u64, String)>,
+    State(ctx): State<Arc<Context>>,
+) -> axum::response::Result<Response, AppError> {
+    let Some((oldbase, oldhead)) = oldbasehead.split_once("..") else {
+        return Ok((
+            StatusCode::BAD_REQUEST,
+            format!("`{oldbasehead}` is not in the form `base..head`"),
+        )
+            .into_response());
+    };
+
+    if !is_repo_autorized(&ctx, &owner, &repo).await? {
+        return Ok((
+            StatusCode::UNAUTHORIZED,
+            format!("repository `{owner}/{repo}` is not part of the Rust Project team repos"),
+        )
+            .into_response());
+    }
+
+    let issue_repo = github::IssueRepository {
+        organization: owner.to_string(),
+        repository: repo.to_string(),
+    };
+
+    let pr = ctx.github.pull_request(&issue_repo, pr_num).await?;
+
+    let newbase = &pr.base.as_ref().context("no base")?.sha;
+    let newhead = &pr.head.as_ref().context("no head")?.sha;
+
+    // Has the base changed?
+    if oldbase == newbase {
+        // No, redirect to GitHub native compare page
+        return Ok(Redirect::to(&format!(
+            "https://github.com/{owner}/{repo}/compare/{oldhead}..{newhead}"
+        ))
+        .into_response());
+    }
+
+    // Yes, use our Github range-diff instead
+    Ok(Redirect::to(&format!(
+        "/gh-range-diff/{owner}/{repo}/{oldbase}..{oldhead}/{newbase}..{newhead}"
+    ))
+    .into_response())
+}
diff --git a/src/gh_range_diff.rs b/src/gh_range_diff.rs
@@ -21,6 +21,7 @@ use pulldown_cmark_escape::FmtWriter;
 use regex::Regex;
 
 use crate::github::GithubCompare;
+use crate::utils::is_repo_autorized;
 use crate::{github, handlers::Context, utils::AppError};
 
 static MARKER_RE: LazyLock<Regex> =
@@ -42,27 +43,11 @@ pub async fn gh_range_diff(
         ));
     };
 
-    let repos = ctx
-        .team
-        .repos()
-        .await
-        .context("unable to retrieve team repos")?;
-
-    // Verify that the request org is part of the Rust project
-    let Some(repos) = repos.repos.get(&owner) else {
-        return Ok((
-            StatusCode::BAD_REQUEST,
-            HeaderMap::new(),
-            format!("organization `{owner}` is not part of the Rust Project team repos"),
-        ));
-    };
-
-    // Verify that the request repo is part of the Rust project
-    if !repos.iter().any(|r| r.name == repo) {
+    if !is_repo_autorized(&ctx, &owner, &repo).await? {
         return Ok((
-            StatusCode::BAD_REQUEST,
+            StatusCode::UNAUTHORIZED,
             HeaderMap::new(),
-            format!("repository `{owner}` is not part of the Rust Project team repos"),
+            format!("repository `{owner}/{repo}` is not part of the Rust Project team repos"),
         ));
     }
 
@@ -166,27 +151,11 @@ pub async fn gh_ranges_diff(
         ));
     };
 
-    let repos = ctx
-        .team
-        .repos()
-        .await
-        .context("unable to retrieve team repos")?;
-
-    // Verify that the request org is part of the Rust project
-    let Some(repos) = repos.repos.get(&owner) else {
-        return Ok((
-            StatusCode::BAD_REQUEST,
-            HeaderMap::new(),
-            format!("organization `{owner}` is not part of the Rust Project team repos"),
-        ));
-    };
-
-    // Verify that the request repo is part of the Rust project
-    if !repos.iter().any(|r| r.name == repo) {
+    if !is_repo_autorized(&ctx, &owner, &repo).await? {
         return Ok((
-            StatusCode::BAD_REQUEST,
+            StatusCode::UNAUTHORIZED,
             HeaderMap::new(),
-            format!("repository `{owner}` is not part of the Rust Project team repos"),
+            format!("repository `{owner}/{repo}` is not part of the Rust Project team repos"),
         ));
     }
 
diff --git a/src/gha_logs.rs b/src/gha_logs.rs
@@ -1,7 +1,7 @@
 use crate::github::{self, WorkflowRunJob};
 use crate::handlers::Context;
 use crate::interactions::REPORT_TO;
-use crate::utils::AppError;
+use crate::utils::{AppError, is_repo_autorized};
 use anyhow::Context as _;
 use axum::extract::{Path, State};
 use axum::http::HeaderValue;
@@ -71,25 +71,11 @@ pub async fn gha_logs(
     Path((owner, repo, log_id)): Path<(String, String, u128)>,
     State(ctx): State<Arc<Context>>,
 ) -> axum::response::Result<impl IntoResponse, AppError> {
-    let repos = ctx
-        .team
-        .repos()
-        .await
-        .context("unable to retrieve team repos")?;
-
-    let Some(repos) = repos.repos.get(&owner) else {
-        return Ok((
-            StatusCode::BAD_REQUEST,
-            HeaderMap::new(),
-            format!("organization `{owner}` is not part of the Rust Project team repos"),
-        ));
-    };
-
-    if !repos.iter().any(|r| r.name == repo) {
+    if !is_repo_autorized(&ctx, &owner, &repo).await? {
         return Ok((
-            StatusCode::BAD_REQUEST,
+            StatusCode::UNAUTHORIZED,
             HeaderMap::new(),
-            format!("repository `{owner}` is not part of the Rust Project team repos"),
+            format!("repository `{owner}/{repo}` is not part of the Rust Project team repos"),
         ));
     }
 
diff --git a/src/github.rs b/src/github.rs
@@ -270,6 +270,16 @@ impl GithubClient {
             .await
             .context("failed to retrive the compare")
     }
+
+    pub async fn pull_request(&self, repo: &IssueRepository, pr_num: u64) -> anyhow::Result<Issue> {
+        let url = format!("{}/pulls/{pr_num}", repo.url(&self));
+        let mut pr: Issue = self
+            .json(self.get(&url))
+            .await
+            .with_context(|| format!("{repo} failed to get pr {pr_num}"))?;
+        pr.pull_request = Some(PullRequestDetails::new());
+        Ok(pr)
+    }
 }
 
 #[derive(Debug, serde::Serialize)]
@@ -709,6 +719,33 @@ impl Issue {
         Ok(comment)
     }
 
+    pub async fn edit_review(
+        &self,
+        client: &GithubClient,
+        id: u64,
+        new_body: &str,
+    ) -> anyhow::Result<()> {
+        let comment_url = format!(
+            "{}/pulls/{}/reviews/{}",
+            self.repository().url(client),
+            self.number,
+            id
+        );
+        #[derive(serde::Serialize)]
+        struct NewComment<'a> {
+            body: &'a str,
+        }
+        client
+            .send_req(
+                client
+                    .put(&comment_url)
+                    .json(&NewComment { body: new_body }),
+            )
+            .await
+            .context("failed to edit review comment")?;
+        Ok(())
+    }
+
     pub async fn post_comment(&self, client: &GithubClient, body: &str) -> anyhow::Result<Comment> {
         #[derive(serde::Serialize)]
         struct PostComment<'a> {
@@ -1982,13 +2019,15 @@ impl Repository {
     }
 
     pub async fn get_pr(&self, client: &GithubClient, pr_num: u64) -> anyhow::Result<Issue> {
-        let url = format!("{}/pulls/{pr_num}", self.url(client));
-        let mut pr: Issue = client
-            .json(client.get(&url))
+        client
+            .pull_request(
+                &IssueRepository {
+                    organization: self.owner().to_string(),
+                    repository: self.name().to_string(),
+                },
+                pr_num,
+            )
             .await
-            .with_context(|| format!("{} failed to get pr {pr_num}", self.full_name))?;
-        pr.pull_request = Some(PullRequestDetails::new());
-        Ok(pr)
     }
 
     /// Fetches information about merge conflicts on open PRs.
diff --git a/src/handlers.rs b/src/handlers.rs
@@ -53,6 +53,7 @@ pub mod pull_requests_assignment_update;
 mod relabel;
 mod relnotes;
 mod rendered_link;
+mod review_changes_since;
 mod review_requested;
 mod review_submitted;
 pub mod rustc_commits;
@@ -160,6 +161,20 @@ pub async fn handle(ctx: &Context, host: &str, event: &Event) -> Vec<HandlerErro
         }
     }
 
+    if let Some(config) = config
+        .as_ref()
+        .ok()
+        .and_then(|c| c.review_changes_since.as_ref())
+    {
+        if let Err(e) = review_changes_since::handle(ctx, host, event, config).await {
+            log::error!(
+                "failed to process event {:?} with review_changes_since handler: {:?}",
+                event,
+                e
+            )
+        }
+    }
+
     if let Some(ghr_config) = config
         .as_ref()
         .ok()
diff --git a/src/handlers/review_changes_since.rs b/src/handlers/review_changes_since.rs
@@ -0,0 +1,55 @@
+use anyhow::Context as _;
+
+use crate::{
+    config::ReviewChangesSinceConfig,
+    github::{Comment, Event, Issue, IssueCommentAction, IssueCommentEvent},
+    handlers::Context,
+};
+
+/// Checks if this event is a PR review creation and adds in the body a link our `gh-changes-since`
+/// endpoint to view changes since this review.
+pub(crate) async fn handle(
+    ctx: &Context,
+    host: &str,
+    event: &Event,
+    _config: &ReviewChangesSinceConfig,
+) -> anyhow::Result<()> {
+    if let Event::IssueComment(
+        event @ IssueCommentEvent {
+            action: IssueCommentAction::Created,
+            issue: Issue {
+                pull_request: Some(_),
+                ..
+            },
+            comment:
+                Comment {
+                    pr_review_state: Some(_),
+                    ..
+                },
+            ..
+        },
+    ) = event
+    {
+        // Add link our gh-changes-since endpoint to view changes since this review
+
+        let issue_repo = event.issue.repository();
+        let pr_num = event.issue.number;
+
+        let base = &event.issue.base.as_ref().context("no base")?.sha;
+        let head = &event.issue.head.as_ref().context("no head")?.sha;
+
+        let link = format!("https://{host}/gh-changes-since/{issue_repo}/{pr_num}/{base}..{head}");
+        let new_body = format!(
+            "{}\n\n*[View changes since this review]({link})*",
+            event.comment.body
+        );
+
+        event
+            .issue
+            .edit_review(&ctx.github, event.comment.id, &new_body)
+            .await
+            .context("failed to update the review body")?;
+    }
+
+    Ok(())
+}
diff --git a/src/lib.rs b/src/lib.rs
@@ -6,6 +6,7 @@ pub mod bors;
 mod changelogs;
 mod config;
 pub mod db;
+pub mod gh_changes_since;
 pub mod gh_range_diff;
 pub mod gha_logs;
 pub mod github;
diff --git a/src/main.rs b/src/main.rs
@@ -193,6 +193,10 @@ async fn run_server(addr: SocketAddr) -> anyhow::Result<()> {
             "/gh-range-diff/{owner}/{repo}/{oldbasehead}/{newbasehead}",
             get(triagebot::gh_range_diff::gh_ranges_diff),
         )
+        .route(
+            "/gh-changes-since/{owner}/{repo}/{pr}/{oldbasehead}",
+            get(triagebot::gh_changes_since::gh_changes_since),
+        )
         .nest("/agenda", agenda)
         .route("/bors-commit-list", get(triagebot::bors::bors_commit_list))
         .route(
diff --git a/src/utils.rs b/src/utils.rs
