Conversation
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 34 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
Summary by CodeRabbit
WalkthroughUpdate the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Main
participant LibRun
participant Daemon (Unix Socket)
User->>Main: Start CLI with arguments
Main->>LibRun: Parse args, call run(args)
LibRun->>LibRun: Parse repo slug
LibRun->>LibRun: Serialise CommentRequest to JSON
LibRun->>Daemon (Unix Socket): Connect and send request
Daemon (Unix Socket)-->>LibRun: (Acknowledge / close)
LibRun-->>Main: Return success or ClientError
Main-->>User: Print error if any, exit process
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Reviewer's GuideIntroduce an asynchronous Sequence diagram for client run function sending a comment requestsequenceDiagram
actor User
participant CLI as comenq CLI
participant Run as run(args)
participant Socket as UnixStream
User->>CLI: Provide CLI arguments
CLI->>Run: Call run(args)
Run->>Run: Parse repo_slug
Run->>Run: Create CommentRequest
Run->>Run: Serialize CommentRequest to JSON
Run->>Socket: Connect to Unix socket
Run->>Socket: Write JSON payload
Run->>Socket: Shutdown connection
Run-->>CLI: Return success or ClientError
CLI-->>User: Print error and exit (if error)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- In run(), map or propagate errors from stream.shutdown() instead of ignoring them so socket closure failures aren’t silently dropped.
- Simplify parse_slug by using split_once('/') and handling the Option directly, since invalid slug formats should have been ruled out by validation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In run(), map or propagate errors from stream.shutdown() instead of ignoring them so socket closure failures aren’t silently dropped.
- Simplify parse_slug by using split_once('/') and handling the Option directly, since invalid slug formats should have been ruled out by validation.
## Individual Comments
### Comment 1
<location> `crates/comenq/src/lib.rs:3` </location>
<code_context>
//! Library utilities for the `comenq` CLI.
use clap::Parser;
+use comenq_lib::CommentRequest;
use std::path::PathBuf;
</code_context>
<issue_to_address>
Consider moving the client logic and related tests into a separate module and simplifying slug parsing to keep the main file focused on argument parsing.
```suggestion
Consider splitting out the “client” bits (`run`, `ClientError`, `parse_slug`, and its integration‐style tests) into their own module to keep this file focused on argument parsing + validation. You can also simplify `parse_slug` by using `str::split_once` now that you already validate the format.
1. Create `src/client.rs`:
```rust
use crate::Args;
use comenq_lib::CommentRequest;
use thiserror::Error;
use tokio::io::AsyncWriteExt;
use tokio::net::UnixStream;
/// Errors when interacting with the daemon.
#[derive(Debug, Error)]
pub enum ClientError {
#[error("failed to connect to daemon: {0}")]
Connect(#[from] std::io::Error),
#[error("failed to serialise request: {0}")]
Serialise(#[from] serde_json::Error),
#[error("failed to write to daemon: {0}")]
Write(#[source] std::io::Error),
}
/// Send a `CommentRequest` to the daemon.
pub async fn run(args: Args) -> Result<(), ClientError> {
// safe unwrap: `validate_repo_slug` ensures there are always two non-empty parts
let (owner, repo) = args.repo_slug.split_once('/').unwrap();
let req = CommentRequest { owner: owner.to_string(), repo: repo.to_string(), pr_number: args.pr_number, body: args.comment_body };
let payload = serde_json::to_vec(&req)?;
let mut sock = UnixStream::connect(&args.socket).await.map_err(ClientError::Connect)?;
sock.write_all(&payload).await.map_err(ClientError::Write)?;
let _ = sock.shutdown().await;
Ok(())
}
#[cfg(test)]
mod client_tests {
use super::*;
use clap::Parser;
use rstest::rstest;
use tempfile::tempdir;
use tokio::io::AsyncReadExt;
use tokio::net::UnixListener;
// -- move your run_sends_request and run_errors_when_socket_missing tests here --
}
```
2. In `src/lib.rs` or `src/main.rs`:
```rust
mod client;
pub use client::{run, ClientError};
use clap::Parser;
// keep only `Args`, `validate_repo_slug`, and unit‐tests for slug parsing here
```
3. Simplify `validate_repo_slug` if desired, but keep as is or:
```rust
fn validate_repo_slug(s: &str) -> Result<String, String> {
if let Some((o, r)) = s.split_once('/') {
if !o.is_empty() && !r.is_empty() {
return Ok(s.to_string());
}
}
Err("invalid repository format, use 'owner/repo'".into())
}
```
This reduces nesting in your main CLI module, makes each part testable in isolation, and keeps functionality intact.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.toml(1 hunks)crates/comenq/Cargo.toml(1 hunks)crates/comenq/src/lib.rs(3 hunks)crates/comenq/src/main.rs(1 hunks)docs/comenq-design.md(1 hunks)docs/roadmap.md(1 hunks)tests/cucumber.rs(1 hunks)tests/features/client_main.feature(1 hunks)tests/steps/client_main_steps.rs(1 hunks)tests/steps/mod.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
Cargo.toml
📄 CodeRabbit Inference Engine (AGENTS.md)
Use explicit version ranges in
Cargo.tomland keep dependencies up-to-date.
Files:
Cargo.toml
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Comments must use en-GB-oxendict spelling and grammar.
Function documentation must include clear examples.
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.
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Clippy warnings MUST be disallowed.
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().
Use predicate functions for conditional criteria with more than two branches.
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.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Files:
crates/comenq/src/main.rstests/steps/mod.rstests/cucumber.rstests/steps/client_main_steps.rscrates/comenq/src/lib.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.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.
Files:
crates/comenq/src/main.rstests/steps/mod.rstests/cucumber.rstests/steps/client_main_steps.rscrates/comenq/src/lib.rs
**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.md: Validate Markdown files usingmake markdownlint.
Runmake fmtafter any documentation changes to format all Markdown files and fix table markup.
Validate Markdown Mermaid diagrams using by runningmake nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks must be wrapped at 120 columns.
Tables and headings must not be wrapped.
Use dashes (-) for list bullets.
Documentation must use en-GB-oxendict spelling and grammar (with the exception of "license" which is to be left unchanged for community consistency).
Files:
docs/roadmap.mddocs/comenq-design.md
⚙️ CodeRabbit Configuration File
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
- Code blocks should be wrapped to 120 columns.
- 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/roadmap.mddocs/comenq-design.md
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.
When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in thedocs/directory to reflect the latest state. Ensure the documentation remains accurate and current.
Files:
docs/roadmap.mddocs/comenq-design.md
🧬 Code Graph Analysis (2)
crates/comenq/src/main.rs (1)
crates/comenq/src/lib.rs (1)
run(70-90)
tests/steps/client_main_steps.rs (1)
crates/comenq/src/lib.rs (2)
run(70-90)serde_json(140-140)
🪛 LanguageTool
docs/comenq-design.md
[uncategorized] ~341-~341: Do not mix variants of the same word (‘serialise’ and ‘serialize’) within a single text.
Context: ...network logic directly. Any failures to serialise the request or communicate with the dae...
(EN_WORD_COHERENCY)
🔇 Additional comments (5)
crates/comenq/Cargo.toml (1)
15-15: Dependency addition uses workspace pin – looks good
thiserroris pulled in via the workspace table with an explicit1.0range, satisfying the version-pinning guideline without duplicating the version string here.tests/cucumber.rs (1)
3-3: LGTM!The new
ClientWorldtest suite is correctly integrated into the concurrent test execution.Also applies to: 9-11
crates/comenq/src/main.rs (1)
4-15: LGTM!The async main function correctly delegates to the
runfunction and handles errors appropriately by printing to stderr and exiting with status 1.crates/comenq/src/lib.rs (1)
130-172: LGTM!The async tests provide comprehensive coverage for both success and error scenarios, properly verifying the serialized request content and error handling.
tests/steps/client_main_steps.rs (1)
1-82: LGTM!The Cucumber step definitions are well-implemented, providing comprehensive test coverage for both daemon-present and daemon-absent scenarios. The use of
expectandunwrapin tests is appropriately justified.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
Cargo.toml (1)
18-18: LGTM: Workspace dependency correctly implemented.The
tempfiledependency is properly added to both the workspace dependencies and referenced in dev-dependencies usingworkspace = true. This follows Rust workspace best practices for dependency management.Also applies to: 42-42
tests/steps/mod.rs (1)
1-6: LGTM: Module declarations properly alphabetised.The module declarations are now correctly ordered alphabetically, maintaining a deterministic and easily scannable list. The new
client_main_stepsmodule appropriately supports the client socket communication functionality.tests/features/client_main.feature (1)
1-14: LGTM: Well-structured feature tests with appropriate tags.The feature file correctly defines test scenarios for client socket communication with proper tags (
@client_main,@happy_path,@unhappy_path) enabling selective execution. Both success and failure paths are covered appropriately.docs/comenq-design.md (1)
342-342: Fix spelling inconsistency: use "serialize" instead of "serialise".The past review comment about this exact spelling inconsistency remains valid. Replace "serialise" with "serialize" to maintain consistency with the document's en-GB-oxendict conventions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
Cargo.toml(2 hunks)crates/comenq/src/client.rs(1 hunks)crates/comenq/src/lib.rs(1 hunks)docs/comenq-design.md(1 hunks)docs/roadmap.md(1 hunks)tests/features/client_main.feature(1 hunks)tests/steps/mod.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.md
⚙️ CodeRabbit Configuration File
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
- Code blocks should be wrapped to 120 columns.
- 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/roadmap.mddocs/comenq-design.md
**/*.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.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.
Files:
crates/comenq/src/lib.rstests/steps/mod.rscrates/comenq/src/client.rs
🧬 Code Graph Analysis (1)
crates/comenq/src/lib.rs (1)
crates/comenq/src/client.rs (1)
run(49-69)
🔇 Additional comments (5)
docs/roadmap.md (1)
22-29: LGTM: Roadmap accurately reflects completed client implementation.The checklist items are correctly marked as completed and accurately describe the implemented client socket communication functionality. The text is properly wrapped and follows the en-GB-oxendict spelling conventions.
crates/comenq/src/lib.rs (2)
6-8: LGTM: Clean module structure with appropriate re-exports.The client module is properly declared and the necessary public APIs (
ClientErrorandrun) are correctly re-exported. This maintains a clean separation between argument parsing and client logic.
11-11: LGTM: Clone derivation supports async operations.Adding
Cloneto theArgsstruct derivation is appropriate as it enables the struct to be used in async contexts and shared between functions, which aligns with the new client socket communication functionality.crates/comenq/src/client.rs (2)
14-29: Excellent error handling design.The
ClientErrorenum demonstrates best practices with clear error variants, proper use of#[from]and#[source]attributes, and descriptive error messages that aid debugging.
77-137: Comprehensive test coverage with excellent practices.The test suite covers success paths, error handling, and helper functions. Good use of temporary directories, async testing patterns, and proper error variant verification.
1 participant
Summary
comenqclientrunfunctionmainClientErrorrunAPITesting
make lintmake testmake markdownlintmake nixiehttps://chatgpt.com/codex/tasks/task_e_688409fa63c08322a8950c2dbecb6f3a
Summary by Sourcery
Implement asynchronous client socket communication for the
comenqCLI by adding a newrunAPI that serializes and sends comment requests over a Unix domain socket, integrates it into the asyncmain, and surfaces errors viaClientError.New Features:
runfunction to serializeCommentRequestto JSON and send it over a Unix socketmainthat invokesrunand exits on failureEnhancements:
ClientErrorenum to wrap connection, serialization, and write failuresrunAPI and mark related roadmap items as completedDocumentation:
runAPI design in the client design guide and update the roadmap statusTests: