Conversation
Summary by CodeRabbit
WalkthroughRefactor the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ConnectionActor
participant HighRx
participant LowRx
participant ResponseStream
participant ShutdownToken
Client->>ConnectionActor: Call next_event()
alt Shutdown requested
ConnectionActor->>ShutdownToken: Await cancellation
ShutdownToken-->>ConnectionActor: Cancelled
ConnectionActor-->>Client: Event::Shutdown
else High priority message
ConnectionActor->>HighRx: Poll for message
HighRx-->>ConnectionActor: Message or None
ConnectionActor-->>Client: Event::High
else Low priority message
ConnectionActor->>LowRx: Poll for message
LowRx-->>ConnectionActor: Message or None
ConnectionActor-->>Client: Event::Low
else Response stream
ConnectionActor->>ResponseStream: Poll for response
ResponseStream-->>ConnectionActor: Response or None
ConnectionActor-->>Client: Event::Response
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (2)**/*.md⚙️ CodeRabbit Configuration File
Files:
**/*.rs⚙️ CodeRabbit Configuration File
Files:
🧠 Learnings (4)📚 Learning: applies to **/*.md : documentation must use en-gb-oxendict spelling and grammar (with the exception ...Applied to files:
📚 Learning: applies to **/*.md : documentation must use en-gb-oxendict spelling and grammar. (exception: the nam...Applied to files:
📚 Learning: applies to docs/**/*.md : documentation must use en-gb-oxendict spelling and grammar (with the excep...Applied to files:
📚 Learning: applies to **/*.md : documentation must use en-gb-oxendict spelling and grammar, except for the nami...Applied to files:
🪛 LanguageToolAGENTS.md[locale-violation] ~47-~47: LICENSE must be spelled with a “c” when used as a noun in British English. Use “licence”. (LICENCE_LICENSE_NOUN_SINGULAR) [uncategorized] ~161-~161: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA) README.md[grammar] ~87-~87: Please add a punctuation mark at the end of paragraph. (PUNCTUATION_PARAGRAPH_END) 🔇 Additional comments (10)
✨ 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 GuideExtracts each branch of the tokio::select! in next_event into its own async helper, simplifying the control flow for shutdown, high/low queue polling, and response stream handling. Flow diagram for event handling in next_event after refactorflowchart TD
A[next_event] --> B{tokio::select!}
B -->|Shutdown| C[handle_shutdown]
B -->|High queue| D[handle_high]
B -->|Low queue| E[handle_low]
B -->|Response stream| F[poll_response_stream]
B -->|Else| G[Event::Idle]
C --> H[Event::Shutdown]
D --> I[Event::High]
E --> J[Event::Low]
F --> K[Event::Response]
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![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 and found some issues that need to be addressed.
- Consider renaming poll_response_stream to handle_response for consistency with the handle_* naming scheme.
- You could extract a single generic helper to poll optional queues/streams and map the result to an Event, reducing the duplication between handle_high and handle_low.
- These small async helpers introduce extra state machines; consider marking them #[inline] or converting them into synchronous poll functions to reduce overhead inside the select! macro.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider renaming poll_response_stream to handle_response for consistency with the handle_* naming scheme.
- You could extract a single generic helper to poll optional queues/streams and map the result to an Event, reducing the duplication between handle_high and handle_low.
- These small async helpers introduce extra state machines; consider marking them #[inline] or converting them into synchronous poll functions to reduce overhead inside the select! macro.
## Individual Comments
### Comment 1
<location> `src/connection.rs:267` </location>
<code_context>
}
}
+ /// Await cancellation and emit a shutdown event.
+ async fn handle_shutdown(token: CancellationToken) -> Event<F, E> {
+ Self::wait_shutdown(token).await;
</code_context>
<issue_to_address>
Consider inlining the poll calls or using a macro instead of adding four small async wrapper functions.
```rust
// Instead of 4 tiny async wrappers, inline the poll calls or use a small macro.
// Option A: Inline closures (closest to pre-refactor):
async fn next_event(&mut self, state: &ActorState) -> Event<F, E> {
let high_avail = self.high_rx.is_some();
let low_avail = self.low_rx.is_some();
let resp_avail = self.response.is_some() && !state.is_shutting_down();
tokio::select! {
biased;
// shutdown
() = Self::wait_shutdown(self.shutdown.clone()), if state.is_active() => {
Event::Shutdown
}
// high priority queue
res = Self::poll_optional(self.high_rx.as_mut(), Self::recv_push), if high_avail => {
Event::High(res)
}
// low priority queue
res = Self::poll_optional(self.low_rx.as_mut(), Self::recv_push), if low_avail => {
Event::Low(res)
}
// response stream
res = Self::poll_optional(self.response.as_mut(), |s| s.next()), if resp_avail => {
Event::Response(res)
}
else => Event::Idle,
}
}
// Option B: Use a tiny macro to collapse repetition
macro_rules! poll_branch {
($guard:expr, $rx:expr, $recv:expr, $cons:ident) => {
res = Self::poll_optional($rx, $recv), if $guard => {
Event::$cons(res)
}
}
}
async fn next_event(&mut self, state: &ActorState) -> Event<F, E> {
let high_avail = self.high_rx.is_some();
let low_avail = self.low_rx.is_some();
let resp_avail = self.response.is_some() && !state.is_shutting_down();
tokio::select! {
biased;
() = Self::wait_shutdown(self.shutdown.clone()), if state.is_active() => {
Event::Shutdown
}
poll_branch!(high_avail, self.high_rx.as_mut(), Self::recv_push, High);
poll_branch!(low_avail, self.low_rx.as_mut(), Self::recv_push, Low);
poll_branch!(resp_avail, self.response.as_mut(), |s| s.next(), Response);
else => Event::Idle,
}
}
// Then remove `handle_shutdown`, `handle_high`, `handle_low`, and `poll_response_stream`.
```
</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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/connection.rs (1)
255-262: Inline the trivial async wrappers to avoid needless Future allocations
handle_shutdown,handle_high,handle_low, andpoll_response_streamwrap a single call each. Creating four separateasync fns spawns four additional state-machines and allocates four extraFutures pertokio::select!iteration. Inline the calls (or use a tiny macro/closure) to keep the hot path lean and the codebase smaller.- event = Self::handle_high(self.high_rx.as_mut()), if high_available => { event } + event = Self::poll_optional(self.high_rx.as_mut(), Self::recv_push), if high_available => { + Event::High(event) + }Repeat for the remaining three branches.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/connection.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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:
src/connection.rs
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary
next_eventinto helper functions for shutdown, queue and stream pollingnext_eventto clarify intentTesting
make fmtmake lintmake testhttps://chatgpt.com/codex/tasks/task_e_688bf5b7b90083229683553b9138e8a1
Summary by Sourcery
Refactor the connection event loop by extracting each select! branch into dedicated async helper methods.
Enhancements: