diff --git a/src/core/forge/github/api.rs b/src/core/forge/github/api.rs index fd4e72f4..bf66cd63 100644 --- a/src/core/forge/github/api.rs +++ b/src/core/forge/github/api.rs @@ -555,12 +555,38 @@ impl GitHubApi { )); } - let mut result = GitHubPullRequestReviewData::default(); + let variables = json!({ "owner": owner, "repo": repo, "number": number }); + let (metadata, threads) = std::thread::scope(|scope| { + let metadata = scope.spawn(|| { + self.review_pull_request(PULL_REQUEST_REVIEW_METADATA_QUERY, variables.clone()) + }); + let threads = self.fetch_pull_request_review_threads(owner, repo, number); + let metadata = metadata.join().unwrap_or_else(|_| { + Err(DiffyError::General( + "pull request metadata fetch panicked".to_owned(), + )) + }); + (metadata, threads) + }); + let metadata = metadata?; + Ok(GitHubPullRequestReviewData { + metadata: parse_pull_request_review_metadata(&metadata), + reviews: parse_pull_request_reviews(metadata.pointer("/latestReviews/nodes")), + threads: threads?, + }) + } + + fn fetch_pull_request_review_threads( + &self, + owner: &str, + repo: &str, + number: i32, + ) -> Result> { + let query = review_graphql_query(PULL_REQUEST_REVIEW_THREADS_QUERY); + let mut threads = Vec::new(); let mut after: Option = None; - let query = review_graphql_query(PULL_REQUEST_REVIEW_DATA_QUERY); loop { - let value = transport::graphql_query( - &self.token, + let pull_request = self.review_pull_request( &query, json!({ "owner": owner, @@ -569,33 +595,7 @@ impl GitHubApi { "after": after, }), )?; - let errors = transport::graphql_errors(&value); - let pull_request = value - .pointer("/data/repository/pullRequest") - .filter(|pull_request| pull_request.is_object()); - let pull_request = match pull_request { - Some(pull_request) if errors.iter().all(is_withheld_pull_request_field) => { - if !errors.is_empty() { - tracing::warn!( - "GitHub withheld pull request fields: {}", - graphql_error_message(errors) - ); - } - pull_request - } - _ if !errors.is_empty() => return Err(graphql_failure(errors)), - _ => { - return Err(DiffyError::Parse( - "missing pull request in GraphQL response".to_owned(), - )); - } - }; - if result.metadata.node_id.is_empty() { - result.metadata = parse_pull_request_review_metadata(pull_request); - result.reviews = - parse_pull_request_reviews(pull_request.pointer("/latestReviews/nodes")); - } - result.threads.extend(parse_review_threads( + threads.extend(parse_review_threads( pull_request.pointer("/reviewThreads/nodes"), )); @@ -616,8 +616,33 @@ impl GitHubApi { } after = next; } + Ok(threads) + } - Ok(result) + fn review_pull_request(&self, query: &str, variables: Value) -> Result { + let mut value = transport::graphql_query(&self.token, query, variables)?; + let errors = transport::graphql_errors(&value); + let found = value + .pointer("/data/repository/pullRequest") + .is_some_and(Value::is_object); + if found && errors.iter().all(is_withheld_pull_request_field) { + if !errors.is_empty() { + tracing::warn!( + "GitHub withheld pull request fields: {}", + graphql_error_message(errors) + ); + } + return Ok(value + .pointer_mut("/data/repository/pullRequest") + .map(Value::take) + .unwrap_or_default()); + } + if !errors.is_empty() { + return Err(graphql_failure(errors)); + } + Err(DiffyError::Parse( + "missing pull request in GraphQL response".to_owned(), + )) } pub fn create_pull_request_review_comment( @@ -989,8 +1014,8 @@ query DiffyPullRequestFingerprint($owner: String!, $repo: String!, $number: Int! } "#; -const PULL_REQUEST_REVIEW_DATA_QUERY: &str = r#" -query DiffyPullRequestReviewData($owner: String!, $repo: String!, $number: Int!, $after: String) { +const PULL_REQUEST_REVIEW_METADATA_QUERY: &str = r#" +query DiffyPullRequestReviewMetadata($owner: String!, $repo: String!, $number: Int!) { repository(owner: $owner, name: $repo) { pullRequest(number: $number) { id @@ -1061,6 +1086,15 @@ query DiffyPullRequestReviewData($owner: String!, $repo: String!, $number: Int!, commit { oid } } } + } + } +} +"#; + +const PULL_REQUEST_REVIEW_THREADS_QUERY: &str = r#" +query DiffyPullRequestReviewThreads($owner: String!, $repo: String!, $number: Int!, $after: String) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $number) { reviewThreads(first: 100, after: $after) { pageInfo { hasNextPage diff --git a/src/core/forge/github/overview.rs b/src/core/forge/github/overview.rs index 5a807103..4095260e 100644 --- a/src/core/forge/github/overview.rs +++ b/src/core/forge/github/overview.rs @@ -235,15 +235,25 @@ impl GitHubApi { number: i32, ) -> Result { let base = format!("https://api.github.com/repos/{owner}/{repo}"); - let pull = self.get_json( - &format!("{base}/pulls/{number}"), - "GitHub pull request fetch", - )?; - let comments = self.get_json( - &format!("{base}/issues/{number}/comments?per_page=100"), - "GitHub pull request comments fetch", - )?; - Ok(parse_public_overview(&pull, &comments)) + let (pull, comments) = std::thread::scope(|scope| { + let comments = scope.spawn(|| { + self.get_json( + &format!("{base}/issues/{number}/comments?per_page=100"), + "GitHub pull request comments fetch", + ) + }); + let pull = self.get_json( + &format!("{base}/pulls/{number}"), + "GitHub pull request fetch", + ); + let comments = comments.join().unwrap_or_else(|_| { + Err(DiffyError::General( + "pull request comments fetch panicked".to_owned(), + )) + }); + (pull, comments) + }); + Ok(parse_public_overview(&pull?, &comments?)) } } diff --git a/src/core/vcs/git/service.rs b/src/core/vcs/git/service.rs index 40167d36..39516b1a 100644 --- a/src/core/vcs/git/service.rs +++ b/src/core/vcs/git/service.rs @@ -1760,13 +1760,18 @@ impl GitService { self.diff_between_refs(&left, &right) } - fn fetch_refspecs(&self, repo_url: &str, refspecs: &[String]) -> Result<()> { + fn fetch_refspecs( + &self, + repo_url: &str, + refspecs: &[String], + env: &[(&str, String)], + ) -> Result<()> { let mut args = Vec::with_capacity(refspecs.len() + 2); args.push(OsString::from("fetch")); args.push(OsString::from(repo_url)); args.extend(refspecs.iter().map(OsString::from)); let started = Instant::now(); - let result = run_system_git(self.repo_path_ref()?, &args); + let result = run_system_git_inner(self.repo_path_ref()?, &args, false, env).map(|_| ()); tracing::info!( refspecs = refspecs.len(), elapsed_ms = started.elapsed().as_millis(), @@ -1974,12 +1979,6 @@ impl GitService { elapsed_ms = stage_started.elapsed().as_millis(), "github-pr: ref resolver fetched metadata" ); - let fetch_source = github_fetch_source_for_repo( - self.repo_path_ref()?, - &parsed.owner, - &parsed.repo, - &info.base_repo_url, - ); let base_source = if info.base_sha.is_empty() { format!("refs/heads/{}", info.base_branch) } else { @@ -2003,7 +2002,24 @@ impl GitService { "github-pr: cached refs are current" ); } else { - self.fetch_refspecs(&fetch_source, &refspecs)?; + let https = if github_token.is_empty() { + None + } else { + let url = format!("https://github.com/{}/{}.git", parsed.owner, parsed.repo); + Some(self.fetch_refspecs(&url, &refspecs, &github_auth_env(github_token))) + }; + if let Some(Err(error)) = &https { + tracing::warn!("github-pr: https fetch failed, trying the remote: {error}"); + } + if !matches!(https, Some(Ok(()))) { + let fetch_source = github_fetch_source_for_repo( + self.repo_path_ref()?, + &parsed.owner, + &parsed.repo, + &info.base_repo_url, + ); + self.fetch_refspecs(&fetch_source, &refspecs, &[])?; + } } tracing::info!( owner = %parsed.owner,