Refactor resolve_comment logic - #127
Conversation
Reviewer's GuideThis PR refactors the resolve comment workflow by extracting REST and GraphQL helpers for posting replies and paginating review comments, streamlining resolve_comment to use those helpers, enhancing CLI error handling with distinct exit codes, and updating tests and documentation to reflect the new logic. Sequence diagram for the refactored resolve_comment workflowsequenceDiagram
participant CLI
participant "resolve_comment()"
participant RestClient
participant GraphQLClient
participant GitHub
CLI->>"resolve_comment()": Call with token, CommentRef, message
alt message is non-empty
"resolve_comment()"->>RestClient: Create REST client
RestClient->>GitHub: POST reply to comment
GitHub-->>RestClient: 201 Created or error
RestClient-->>"resolve_comment()": Return result
end
"resolve_comment()"->>GraphQLClient: Create GraphQL client
GraphQLClient->>GitHub: Paginate review comments to find thread ID
GitHub-->>GraphQLClient: Return review comments pages
GraphQLClient-->>"resolve_comment()": Return thread ID
"resolve_comment()"->>GraphQLClient: Resolve thread by ID
GraphQLClient->>GitHub: Run resolve mutation
GitHub-->>GraphQLClient: Return mutation result
GraphQLClient-->>"resolve_comment()": Return result
"resolve_comment()"-->>CLI: Return success or error
CLI->>CLI: Map error to exit code and print message
Class diagram for new helper functions in resolve.rsclassDiagram
class post_reply {
+post_reply(rest: &RestClient, reference: CommentRef, body: &str): Future<Result<(), VkError>>
}
class get_thread_id {
+get_thread_id(gql: &GraphQLClient, reference: CommentRef): Future<Result<String, VkError>>
}
class resolve_thread {
+resolve_thread(gql: &GraphQLClient, thread_id: &str): Future<Result<(), VkError>>
}
class find_comment_in_page {
+find_comment_in_page(nodes: &[Value], comment_id: u64): Option<String>
}
class extract_review_comments {
+extract_review_comments(data: &Value): Result<&Value, VkError>
}
class process_comments_page {
+process_comments_page(comments: &Value, comment_id: u64): Result<Option<String>, VkError>
}
class get_page_info {
+get_page_info(comments: &Value): Result<(bool, Option<String>), VkError>
}
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Note Reviews pausedUse the following commands to manage reviews:
Summary by CodeRabbit
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- resolve.rs is growing quite large and mixes REST and GraphQL logic—consider extracting the REST client/reply helpers and the pagination helpers into separate submodules to improve readability and separation of concerns.
- The HTTP timeouts in github_client (10s) and RestClient (5s) are hard-coded; consider making them configurable (via CLI flags or env vars) so users can tune for different network conditions.
- We add pagination in get_thread_id but don’t have a test for when the target comment appears on the second (or later) page—adding such a test would exercise the loop and cursor logic end-to-end.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- resolve.rs is growing quite large and mixes REST and GraphQL logic—consider extracting the REST client/reply helpers and the pagination helpers into separate submodules to improve readability and separation of concerns.
- The HTTP timeouts in github_client (10s) and RestClient (5s) are hard-coded; consider making them configurable (via CLI flags or env vars) so users can tune for different network conditions.
- We add pagination in get_thread_id but don’t have a test for when the target comment appears on the second (or later) page—adding such a test would exercise the loop and cursor logic end-to-end.
## Individual Comments
### Comment 1
<location> `src/resolve.rs:161` </location>
<code_context>
+ Ok(())
+}
+
+/// Search a page of review comment nodes for a matching comment.
+///
+/// Returns the thread ID when the comment matches.
</code_context>
<issue_to_address>
Consider splitting REST and GraphQL logic into separate modules and using typed deserialization to replace manual JSON traversal helpers.
Here are a few small, focused steps you can take to collapse most of those `Value`–walking helpers and split REST vs. GraphQL concerns into their own files without losing any functionality.
1. Extract REST logic into a new `resolve/rest.rs`:
```rust
// resolve/rest.rs
use crate::{VkError, ref_parser::RepoInfo};
use reqwest::{Client, StatusCode};
use serde::Deserialize;
pub struct RestClient {
api: String,
client: Client,
}
impl RestClient {
pub fn new(token: &str) -> Result<Self, VkError> {
let api = std::env::var("GITHUB_API_URL")
.unwrap_or_else(|_| "https://api.github.com".into())
.trim_end_matches('/')
.to_owned();
let client = super::github_client(token)?;
Ok(Self { api, client })
}
pub async fn post_reply(
&self,
repo: &RepoInfo,
pull_number: u64,
comment_id: u64,
body: &str,
) -> Result<(), VkError> {
let body = body.trim();
if body.is_empty() {
return Ok(());
}
let url = format!(
"{}/repos/{}/{}/pulls/{}/comments/{}/replies",
self.api, repo.owner, repo.name, pull_number, comment_id
);
let resp = self.client.post(&url)
.json(&serde_json::json!({ "body": body }))
.send().await
.map_err(|e| VkError::RequestContext { context: "post reply".into(), source: Box::new(e) })?;
match resp.status() {
StatusCode::NOT_FOUND => Err(VkError::CommentNotFound { comment_id }),
StatusCode::CREATED => Ok(()),
_ => resp.error_for_status()
.map(|_| ())
.map_err(|e| VkError::Request(Box::new(e))),
}
}
}
```
2. Extract GraphQL logic into `resolve/graphql.rs`, defining typed structs to replace all the manual JSON traversals:
```rust
// resolve/graphql.rs
use crate::{api::GraphQLClient, VkError, ref_parser::RepoInfo};
use serde::Deserialize;
const REVIEW_COMMENTS_PAGE: &str = r#"
query($owner: String!, $name: String!, $number: Int!, $after: String) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewComments(first: 100, after: $after) {
pageInfo { hasNextPage endCursor }
nodes { databaseId pullRequestReviewThread { id } }
}
}
}
}
"#;
#[derive(Deserialize)]
pub struct PageInfo { hasNextPage: bool, endCursor: Option<String> }
#[derive(Deserialize)]
pub struct Thread { id: String }
#[derive(Deserialize)]
pub struct CommentNode { databaseId: u64, pullRequestReviewThread: Thread }
#[derive(Deserialize)]
pub struct ReviewComments { pageInfo: PageInfo, nodes: Vec<CommentNode> }
#[derive(Deserialize)]
struct ResponseData {
repository: Repo { pullRequest: PullRequest },
}
#[derive(Deserialize)]
struct Repo { pullRequest: PullRequest }
#[derive(Deserialize)]
struct PullRequest { reviewComments: ReviewComments }
pub async fn get_thread_id(
gql: &GraphQLClient,
repo: &RepoInfo,
pull_number: u64,
comment_id: u64,
) -> Result<String, VkError> {
let mut cursor = None;
loop {
let data: ResponseData = gql
.run_query(REVIEW_COMMENTS_PAGE, serde_json::json!({
"owner": repo.owner, "name": repo.name,
"number": pull_number, "after": cursor,
}))
.await?;
let rc = data.repository.pullRequest.reviewComments;
if let Some(node) = rc.nodes.into_iter().find(|n| n.databaseId == comment_id) {
return Ok(node.pullRequestReviewThread.id);
}
if !rc.pageInfo.hasNextPage {
break;
}
cursor = rc.pageInfo.endCursor;
}
Err(VkError::CommentNotFound { comment_id })
}
pub async fn resolve_thread(
gql: &GraphQLClient,
thread_id: &str,
) -> Result<(), VkError> {
const RESOLVE_THREAD_MUTATION: &str = r#"
mutation($id: ID!) {
resolveReviewThread(input: { threadId: $id }) { clientMutationId }
}
"#;
gql.run_query::<serde_json::Value, _>(
RESOLVE_THREAD_MUTATION,
serde_json::json!({ "id": thread_id }),
)
.await?;
Ok(())
}
```
3. Wire them up in your top‐level `resolve_comment`:
```rust
// resolve.rs
mod rest;
mod graphql;
pub async fn resolve_comment(
token: &str,
reference: CommentRef<'_>,
#[cfg(feature = "unstable-rest-resolve")] message: Option<String>,
) -> Result<(), VkError> {
#[cfg(feature = "unstable-rest-resolve")]
if let Some(body) = message.as_deref().map(str::trim).filter(|b| !b.is_empty()) {
let rest = rest::RestClient::new(token)?;
rest.post_reply(reference.repo, reference.pull_number, reference.comment_id, body)
.await?;
}
let gql = GraphQLClient::new(token, None)?;
let thread_id = graphql::get_thread_id(&gql, reference.repo, reference.pull_number, reference.comment_id).await?;
graphql::resolve_thread(&gql, &thread_id).await?;
Ok(())
}
```
This:
- Splits REST vs. GraphQL into two files.
- Uses `#[derive(Deserialize)]` to eliminate all of your `get(...).and_then(Value::…)` helpers and pagination extractors.
- Keeps every bit of existing behavior exactly the same.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
|
@coderabbitai please create new GitHub issues for the following::
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/vk-end-to-end-testing-guide.md (2)
142-150: Use a single language identifier for the fenced block.Replace “ini,toml” with “toml” to satisfy Markdown linters and syntax highlighters.
-```ini,toml +```toml [dev-dependencies] assert_cmd = "2.0" insta = { version = "1.34", features = ["redactions"] } third-wheel = "0.6" tokio = { version = "1.0", features = ["full"] } serde_json = "1.0" tempfile = "3.8"--- `1019-1115`: **Adopt GitHub‑flavoured footnotes for references.** Replace bare numeric call‑outs with footnotes ([^n]) and define them accordingly to comply with our documentation standard. Run make markdownlint and make fmt after updating citations. </blockquote></details> </blockquote></details>📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
docs/ortho-config-users-guide.md(1 hunks)docs/vk-end-to-end-testing-guide.md(2 hunks)src/main.rs(3 hunks)src/resolve.rs(3 hunks)tests/resolve.rs(3 hunks)🧰 Additional context used
📓 Path-based instructions (3)
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Use the markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
Proactively update the relevant file(s) in thedocs/directory to reflect the latest state when new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve.Files:
docs/ortho-config-users-guide.mddocs/vk-end-to-end-testing-guide.md**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Documentation must use en-GB-oxendict spelling and grammar, except for the naming of the "LICENSE" file.
Validate Markdown files usingmake markdownlint.
Runmake fmtafter any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by runningmake nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.Files:
docs/ortho-config-users-guide.mddocs/vk-end-to-end-testing-guide.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/ortho-config-users-guide.mddocs/vk-end-to-end-testing-guide.md**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Prefer semantic error enums: Derivestd::error::Error(via thethiserrorcrate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary: Useeyre::Reportfor human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library: Convert to domain enums at API boundaries, and toeyreonly in the mainmain()entrypoint or top-level async task.
Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Useconcat!()to combine long string literals rather than escaping newlines with a backslash.Files:
src/main.rstests/resolve.rssrc/resolve.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
src/main.rstests/resolve.rssrc/resolve.rs🧬 Code graph analysis (1)
tests/resolve.rs (1)
tests/utils/mod.rs (3)
shutdown(30-33)start_mitm(49-98)vk_cmd(109-117)🔍 Remote MCP Deepwiki, Ref
Additional context found (concise)
Repository wiki contains docs directly relevant to this PR’s changes: 2.3 GitHub API Integration; 3.2 GitHub Authentication Setup; 5.3 End-to-End Testing; 4.4 CI/CD Pipeline; 6.2 Review Data Processing; 7 Testing Data and Fixtures.
I attempted to fetch the PR and repo files directly but automated read/search calls failed (HTTP 402), so I could not retrieve file-level diffs beyond the context you provided.
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (15)
src/main.rs (2)
136-138: Add explicit not‑found error for comment resolution.Good addition; enables distinct user feedback and exit mapping.
416-440: Map VkError to deterministic exit codes and assert them in tests.Assert exit codes 2 (VkError::MissingAuth) and 3 (VkError::CommentNotFound) instead of only .failure(). Apply this diff to tests/resolve.rs:
@@ tokio::task::spawn_blocking(move || { vk_cmd(addr) .args([ "resolve", "https://github.com/o/r/pull/83#discussion_r1", "-m", "done", ]) .assert() - .failure() + .failure() + .code(3) .stderr(contains("not found"));tests/resolve.rs (4)
24-26: Use the new GraphQL page shape for thread lookup.LGTM; the nodes payload matches the paginated reviewComments schema.
73-77: Return 201 for REST replies and 200 for GraphQL.LGTM; mirrors GitHub’s semantics and avoids false positives.
99-103: Assert expected REST reply path.LGTM; endpoint path is correct: /repos/{owner}/{repo}/pulls/comments/{id}/replies.
132-141: Skip empty reply body.LGTM; the “ -m ' ' ” case is handled and does not hit REST.
src/resolve.rs (9)
23-34: Page through review comments to find thread IDs.LGTM; querying first: 100 with pageInfo is the right trade‑off and matches tests.
74-83: Set a connect timeout on REST client.LGTM; avoids long hangs on network issues.
84-101: Encapsulate REST base URL and client.LGTM; honours GITHUB_API_URL for tests and production default.
172-186: Match on databaseId with graceful fallback.LGTM; returns the thread ID when present and keeps scanning otherwise.
202-207: Validate presence of reviewComments in response.LGTM; fail fast with a domain error.
224-237: Detect missing thread IDs for matching comments.LGTM; surfaces malformed responses instead of silently skipping.
254-273: Extract pagination info robustly.LGTM; enforces both hasNextPage and endCursor when required.
288-315: Return CommentNotFound when pages are exhausted.LGTM; the loop and termination conditions are clear and correct.
328-331: Resolve thread via mutation.LGTM.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll create the three GitHub issues you've requested. Let me generate them now: @leynos I've created three GitHub issues as requested:
All issues have been assigned to you and include references back to this PR and comment thread for context. 🏁 Script executed: Length of output: 119 Perfect! I've successfully created all three GitHub issues:
Each issue includes detailed problem descriptions, proposed solutions, context linking back to this PR and comment thread, and has been assigned to you. The third issue is marked as a bug since it affects the functionality of |
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Adopt GraphQL pagination helpers for thread lookup to replace brittle node-id logic; keeps this branch’s refactor intent intact. - Integrate configurable HTTP timeouts into REST client and `resolve_comment` (feature-gated) to incorporate improvements from main. - Preserve REST reply flow; treat 404 when posting a reply as non-fatal so resolution proceeds, aligning with main’s tests and expected UX. - Update tests to new GraphQL shape, keep negative reply cases, remove obsolete not-found test. Apply fmt and ensure clippy passes. Validated with: - make check-fmt - make test (all targets, all features) - make lint
0de385d to
09cbf3f
Compare
1 participant
Summary
resolve_commentto use new helperspost_replyparameters by passingCommentRefdead_codesuppression onCommentRefresolve_commentCommentRefand resolve path lookupget_thread_idparameters intoCommentRefnumberget_thread_idcomplexityTesting
make fmtmake lintmake testhttps://chatgpt.com/codex/tasks/task_e_68c064ac14bc832290bc6d6c92a39229
Summary by Sourcery
Extract REST and GraphQL helpers to streamline resolve_comment, implement paginated thread lookup, support optional comment replies, improve error codes, and update tests and docs accordingly.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores: