Conversation
Reviewer's GuideThis PR refactors the server module by extracting the worker and stream processing logic into a new Class diagram for test helpers and fixtures extractionclassDiagram
class server_helpers {
<<module>>
+factory() Fn() -> WireframeApp
+free_port() SocketAddr
+bind_server(factory, addr) WireframeServer
+server_with_preamble(factory) WireframeServer
}
class server_runtime {
<<test module>>
}
class server_runtime_more {
<<test module>>
}
server_runtime ..> server_helpers : uses
server_runtime_more ..> server_helpers : uses
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
WalkthroughRefactor the internal worker task and connection processing logic from Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TcpListener
participant WorkerTask
participant WireframeApp
participant Callback
loop Worker Loop
Client->>TcpListener: Connect
TcpListener->>WorkerTask: Accept connection
WorkerTask->>WorkerTask: Decode preamble
alt Success
WorkerTask->>Callback: on_success (optional)
WorkerTask->>WireframeApp: Create app instance
else Failure
WorkerTask->>Callback: on_failure (optional)
WorkerTask-->>Client: Close connection
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs⚙️ CodeRabbit Configuration File
Files:
🧬 Code Graph Analysis (3)src/server/worker.rs (3)
tests/server_helpers.rs (3)
tests/server_runtime.rs (3)
🔇 Additional comments (15)
✨ 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 (
|
![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 they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `tests/server_helpers.rs:17` </location>
<code_context>
- #[expect(dead_code, reason = "test helper for unused preamble type")]
- struct EmptyPreamble;
-
- #[fixture]
- fn factory() -> impl Fn() -> WireframeApp + Send + Sync + Clone + 'static {
- || WireframeApp::default()
</code_context>
<issue_to_address>
Potential race condition in free_port fixture.
Binding and releasing a port can allow another process to claim it before your test does. To prevent this, use the listener directly or pass it to the server.
Suggested implementation:
```rust
#[fixture]
/// Returns a bound TcpListener on a free port for use in tests.
/// This prevents race conditions by keeping the port reserved until the test is ready.
pub fn free_listener() -> std::net::TcpListener {
let addr = SocketAddr::new(Ipv4Addr::LOCALHOST.into(), 0);
std::net::TcpListener::bind(addr).expect("Failed to bind to localhost:0")
}
```
- Update any test or helper that previously used `free_port()` to use `free_listener()` and extract the address with `listener.local_addr().unwrap()` if needed.
- Pass the `TcpListener` directly to the server setup code instead of just the address.
</issue_to_address>
### Comment 2
<location> `src/server/worker.rs:24` </location>
<code_context>
+/// Each accepted connection is handled in a separate task, with optional callbacks for preamble
+/// decode success or failure. The worker listens for shutdown signals to terminate gracefully.
+/// Accept errors are retried with exponential backoff.
+pub async fn worker_task<F, T>(
+ listener: Arc<TcpListener>,
+ factory: F,
</code_context>
<issue_to_address>
Consider extracting the backoff accept loop and panic-catching spawn logic into separate helper functions to keep `worker_task` focused on high-level flow.
```markdown
Consider pulling out the backoff/accept‐loop and the panic‐wrapping spawn into small helpers. That keeps `worker_task` focused on “what” to do, not “how” to retry or catch panics.
1) Extract retry/backoff into its own async fn:
```rust
async fn accept_with_backoff(
listener: &TcpListener,
shutdown: &CancellationToken,
) -> Option<tokio::net::TcpStream> {
let mut delay = Duration::from_millis(10);
loop {
tokio::select! {
() = shutdown.cancelled() => return None,
res = listener.accept() => match res {
Ok((stream, _)) => return Some(stream),
Err(e) => {
tracing::error!(error = %e, "accept error");
sleep(delay).await;
delay = (delay * 2).min(Duration::from_secs(1));
}
}
}
}
}
```
2) Extract panic‐catching spawn into a helper:
```rust
fn spawn_conn_task<F, T>(
tracker: TaskTracker,
stream: TcpStream,
factory: F,
on_success: Option<PreambleCallback<T>>,
on_failure: Option<PreambleErrorCallback>,
)
where
F: Fn() -> WireframeApp + Send + Sync + Clone + 'static,
T: Preamble + 'static,
{
let peer_addr = stream.peer_addr().ok();
tracker.spawn(async move {
let res = std::panic::AssertUnwindSafe(process_stream(stream, factory, on_success, on_failure))
.catch_unwind()
.await;
if let Err(panic) = res {
let msg = panic
.downcast_ref::<&str>()
.copied()
.or_else(|| panic.downcast_ref::<String>().map(String::as_str))
.unwrap_or("<non-string panic>");
tracing::error!(panic = %msg, ?peer_addr, "connection task panicked");
}
});
}
```
3) Now `worker_task` becomes:
```rust
pub async fn worker_task<…>(…) {
loop {
if let Some(stream) = accept_with_backoff(&listener, &shutdown).await {
spawn_conn_task(tracker.clone(), stream, factory.clone(), on_success.clone(), on_failure.clone());
} else {
break;
}
}
}
```
This keeps the same behavior but reduces nesting and separates concerns.
</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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
No |
1 participant
Summary
server_helpersmoduleprocess_streamand expose helper functions for teststracing::error!Testing
make fmtmake lintmake testhttps://chatgpt.com/codex/tasks/task_e_688bf634507c8322a842ee9c9bccda9a
Summary by Sourcery
Refactor the server module by extracting worker logic into a dedicated submodule, improve error logging with tracing macros, and reorganize tests with shared fixtures and dedicated helper functions.
Bug Fixes:
Enhancements:
Tests: