Convert GitHub client to async using reqwest and tokio#238
Convert GitHub client to async using reqwest and tokio#238
Conversation
WalkthroughThis PR migrates GitHub HTTP interactions to async/await and introduces reqwest and tokio as dependencies. It replaces the previous token-based client with a configured Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wkfl/src/main.rs (1)
428-435:⚠️ Potential issue | 🟡 MinorFix formatting to pass CI —
cargo fmtcheck is failing on these lines.The pipeline reports that
cargo fmt --checkfailed on lines 425–441. Runcargo fmtto auto-fix the formatting of this.await?chain.
🧹 Nitpick comments (2)
wkfl/src/github.rs (2)
88-93: Consider configuring default headers and a timeout on thereqwest::Client.The
Authorization,User-Agent, andAcceptheaders are duplicated in bothapi_get(lines 108–110) andgraphql_query(lines 250–252). Setting them as default headers on the client eliminates this repetition. Additionally,Client::new()has no timeout, so a hung GitHub API connection will block the tokio runtime indefinitely.♻️ Proposed refactor
+use reqwest::header::{self, HeaderMap, HeaderValue}; + pub fn new(host: String, token: String) -> Self { // ...base URL logic unchanged... + let mut headers = HeaderMap::new(); + headers.insert( + header::AUTHORIZATION, + HeaderValue::from_str(&format!("Bearer {}", token)).expect("valid header"), + ); + headers.insert(header::USER_AGENT, HeaderValue::from_static("wkfl")); + headers.insert( + header::ACCEPT, + HeaderValue::from_static("application/vnd.github+json"), + ); GitHubClient { api_base, graphql_base, token, - client: Client::new(), + client: Client::builder() + .default_headers(headers) + .timeout(std::time::Duration::from_secs(30)) + .build() + .expect("Failed to build HTTP client"), } }Then remove the per-request
.header(...)calls fromapi_getandgraphql_query.
142-155: Consider fetching issue comments and review comments concurrently.
get_issue_commentsandget_review_commentsare independent calls that are currently awaited sequentially. Usingtokio::try_join!would execute them in parallel, roughly halving the latency.♻️ Proposed refactor
pub async fn get_pr_comments( &self, owner: &str, repo: &str, pr_number: u64, ) -> Result<PrComments> { - let issue_comments = self.get_issue_comments(owner, repo, pr_number).await?; - let review_comments = self.get_review_comments(owner, repo, pr_number).await?; + let (issue_comments, review_comments) = tokio::try_join!( + self.get_issue_comments(owner, repo, pr_number), + self.get_review_comments(owner, repo, pr_number), + )?; Ok(PrComments { issue_comments, review_comments, }) }
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wkfl/src/github.rs (1)
174-197:⚠️ Potential issue | 🟡 Minor
get_issue_commentsdoes not handle pagination — may silently truncate results.The GitHub REST API returns a maximum of 30 items per page by default. For PRs with more than 30 timeline comments, this method will only return the first page. This is inconsistent with
get_review_comments(line 200) which properly paginates via GraphQL.Consider either adding
per_page=100as a query parameter (to raise the limit) andLinkheader–based pagination, or migrating this endpoint to GraphQL with cursor pagination like the review comments.
1 participant
Motivation
reqwestfor async HTTP and GraphQL requests and run them on atokioruntime for compatibility with other async code.Description
reqwest::Clientstored onGitHubClientand convertedapi_getandgraphql_queryto async methods insrc/github.rs.get_pull_requests_for_commit,get_pr_comments,get_issue_comments,get_review_comments) and updated internal GraphQL pagination to await requests.prune_merged_branches,get_pull_request_for_commit, andget_pr_commentsasync and awaiting the client calls insrc/actions.rs.mainwith#[tokio::main]and awaited the modified action functions insrc/main.rs.reqwestandtokiodependencies toCargo.tomlto support async HTTP and runtime usage.Testing
Codex Task
Summary by CodeRabbit
New Features
Improvements