Modularise server into config, runtime and connection modules#262
Modularise server into config, runtime and connection modules#262
Conversation
Reviewer's GuideRefactors the WireframeServer by splitting its implementation into dedicated config, runtime, and connection modules, introducing a builder-style API, graceful shutdown support, panic-safe connection tasks, and comprehensive module-level tests. Class diagram for connection handling and panic-safe task spawningclassDiagram
class spawn_connection_task {
+spawn_connection_task(stream, factory, on_success, on_failure, tracker)
}
class process_stream {
+process_stream(stream, peer_addr, factory, on_success, on_failure)
}
class TaskTracker {
}
class TcpStream {
}
spawn_connection_task --> process_stream : calls
spawn_connection_task --> TaskTracker : tracker
spawn_connection_task --> TcpStream : stream
process_stream --> TcpStream : stream
process_stream ..> PreambleCallback : on_success
process_stream ..> PreambleErrorCallback : on_failure
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. Summary by CodeRabbit
WalkthroughRemove the monolithic Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TcpListener
participant WorkerTask
participant ConnectionTask
participant WireframeApp
Client->>TcpListener: Connects to server
TcpListener->>WorkerTask: Accepts connection
WorkerTask->>ConnectionTask: Spawns task for connection
ConnectionTask->>ConnectionTask: Decode preamble (if any)
alt Preamble success
ConnectionTask->>WireframeApp: Invoke preamble success callback
WireframeApp->>ConnectionTask: Handle connection
else Preamble failure
ConnectionTask->>ConnectionTask: Invoke preamble failure callback
ConnectionTask-->>WorkerTask: Drop connection
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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. 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}
There was a problem hiding this comment.
Gates Failed
(1 file with Code Duplication)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| config.rs | 1 advisory rule | 10.00 → 9.39 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
![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:
- There’s duplication in bind() and bind_listener() around setting nonblocking and converting the listener—consider extracting that into a shared helper to DRY up the code.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There’s duplication in bind() and bind_listener() around setting nonblocking and converting the listener—consider extracting that into a shared helper to DRY up the code.
- run() and run_with_shutdown() panic if bind() wasn’t called—you might use a typestate or builder pattern to enforce binding at compile time instead of panicking at runtime.
- The exponential backoff in accept_loop is hardcoded (10ms → 1s); consider making the initial delay and max cap configurable or pulling them out as named constants.
## Individual Comments
### Comment 1
<location> `src/server/config.rs:28` </location>
<code_context>
- // Ensure at least one worker is always configured. While
- // `available_parallelism` cannot return zero, defensive programming
- // protects against unexpected platform behaviour.
- let workers = std::thread::available_parallelism()
- .map_or(1, std::num::NonZeroUsize::get)
- .max(1);
- Self {
- factory,
</code_context>
<issue_to_address>
The use of .max(1) after map_or(1, ...) is redundant.
Removing .max(1) will simplify the code without affecting its behavior.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
let workers = std::thread::available_parallelism()
.map_or(1, std::num::NonZeroUsize::get)
.max(1);
=======
let workers = std::thread::available_parallelism()
.map_or(1, std::num::NonZeroUsize::get);
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `src/server/connection.rs:58` </location>
<code_context>
+ T: Preamble,
+{
+ let peer_addr = stream.peer_addr().ok();
+ match read_preamble::<_, T>(&mut stream).await {
+ Ok((preamble, leftover)) => {
+ if let Some(handler) = on_success.as_ref() {
+ if let Err(e) = handler(&preamble, &mut stream).await {
+ tracing::error!(error = ?e, ?peer_addr, "preamble callback error");
+ }
+ }
+ let stream = RewindStream::new(leftover, stream);
+ let app = (factory)();
+ app.handle_connection(stream).await;
+ }
+ Err(err) => {
+ if let Some(handler) = on_failure.as_ref() {
+ handler(&err);
</code_context>
<issue_to_address>
No error is logged if preamble decoding fails and no handler is set.
If on_failure is None, failed preamble decodes are ignored. Logging an error or warning here would help with debugging.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
Err(err) => {
if let Some(handler) = on_failure.as_ref() {
handler(&err);
}
}
=======
Err(err) => {
if let Some(handler) = on_failure.as_ref() {
handler(&err);
} else {
tracing::error!(error = ?err, ?peer_addr, "preamble decode failed and no failure handler set");
}
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `src/server/config.rs:121` </location>
<code_context>
- /// let addr: SocketAddr = "127.0.0.1:8080".parse().expect("Failed to parse address");
- /// let server = server.bind(addr).expect("Failed to bind address");
- /// ```
- pub fn bind(mut self, addr: SocketAddr) -> io::Result<Self> {
- let std_listener = StdTcpListener::bind(addr)?;
- std_listener.set_nonblocking(true)?;
</code_context>
<issue_to_address>
Consider refactoring the duplicated listener setup code in `bind` and `bind_listener` into a single private helper method.
Here’s one way to collapse the two `bind*` methods into a single helper and eliminate the duplicated non-blocking + `from_std` logic. You end up with only three small methods instead of two fairly big ones:
```rust
impl<F, T> WireframeServer<F, T>
where
F: Fn() -> WireframeApp + Send + Sync + Clone + 'static,
T: Preamble,
{
// move all the setup into one private helper
fn bind_std_listener(
mut self,
std_listener: std::net::TcpListener,
) -> std::io::Result<Self> {
std_listener.set_nonblocking(true)?;
let listener = TcpListener::from_std(std_listener)?;
self.listener = Some(Arc::new(listener));
Ok(self)
}
/// Bind by address, reusing the same helper
pub fn bind(self, addr: SocketAddr) -> io::Result<Self> {
let std_listener = StdTcpListener::bind(addr)?;
self.bind_std_listener(std_listener)
}
/// Bind from an existing `std::net::TcpListener`
pub fn bind_listener(self, listener: StdTcpListener) -> io::Result<Self> {
self.bind_std_listener(listener)
}
}
```
This:
- Removes the duplicated `set_nonblocking` + `from_std` code.
- Keeps all functionality—both `bind` and `bind_listener` still work.
- Stays in your single generic `impl<F, T>` block so you don’t have to merge or split impls any further just for these methods.
</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: 10
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
src/server.rs(0 hunks)src/server/config.rs(1 hunks)src/server/connection.rs(1 hunks)src/server/mod.rs(1 hunks)src/server/runtime.rs(1 hunks)src/server/test_util.rs(1 hunks)
💤 Files with no reviewable changes (1)
- src/server.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Function documentation must include clear examples demonstrating the usage and outcome of the function. Test documentation should omit examples where the example serves only to reiterate the test logic.
No single code file may be longer than 400 lines. Long switch statements or dispatch tables should be broken up by feature and constituents colocated with targets. Large blocks of test data should be moved to external data files.
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 using Arc to reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
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.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Avoid unsafe code unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not use return in single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Prefer .expect() over .unwrap().
Use concat!() to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums. Derive std::error::Error (via the thiserror crate) for any condition the caller might inspect, retry, or map to an HTTP status....
Files:
src/server/connection.rssrc/server/mod.rssrc/server/test_util.rssrc/server/runtime.rssrc/server/config.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/server/connection.rssrc/server/mod.rssrc/server/test_util.rssrc/server/runtime.rssrc/server/config.rs
🪛 GitHub Check: build-test
src/server/connection.rs
[warning] 87-87:
Diff in /home/runner/work/wireframe/wireframe/src/server/connection.rs
src/server/test_util.rs
[warning] 5-5:
Diff in /home/runner/work/wireframe/wireframe/src/server/test_util.rs
src/server/runtime.rs
[warning] 213-213:
Diff in /home/runner/work/wireframe/wireframe/src/server/runtime.rs
[warning] 12-12:
Diff in /home/runner/work/wireframe/wireframe/src/server/runtime.rs
[warning] 5-5:
Diff in /home/runner/work/wireframe/wireframe/src/server/runtime.rs
src/server/config.rs
[warning] 149-149:
Diff in /home/runner/work/wireframe/wireframe/src/server/config.rs
[warning] 104-104:
Diff in /home/runner/work/wireframe/wireframe/src/server/config.rs
🪛 GitHub Actions: CI
src/server/connection.rs
[warning] 87-95: Prettier formatting check failed. Code style differences detected. Run 'cargo fmt --all -- --write' to fix code style issues.
src/server/test_util.rs
[warning] 5-10: Prettier formatting check failed. Code style differences detected. Run 'cargo fmt --all -- --write' to fix code style issues.
src/server/runtime.rs
[warning] 5-11: Prettier formatting check failed. Code style differences detected. Run 'cargo fmt --all -- --write' to fix code style issues.
[warning] 12-19: Prettier formatting check failed. Code style differences detected. Run 'cargo fmt --all -- --write' to fix code style issues.
[warning] 213-218: Prettier formatting check failed. Code style differences detected. Run 'cargo fmt --all -- --write' to fix code style issues.
src/server/config.rs
[warning] 104-106: Prettier formatting check failed. Code style differences detected. Run 'cargo fmt --all -- --write' to fix code style issues.
[warning] 149-155: Prettier formatting check failed. Code style differences detected. Run 'cargo fmt --all -- --write' to fix code style issues.
🔇 Additional comments (5)
src/server/mod.rs (5)
1-6: Module documentation is well-structured and clear.The module-level documentation properly explains the purpose and functionality of the server module.
7-14: Imports are properly organized and specific.The imports follow Rust conventions with clear grouping of core/std, external, and internal dependencies.
16-28: Type aliases are well-designed for concurrent callback handling.The use of
ArcwithSend + Syncbounds ensures thread safety, and the HRTB pattern correctly handles lifetime requirements for the async callbacks.
29-62: Struct definition demonstrates excellent design patterns.The generic constraints are well-thought-out, field visibility is properly scoped, and the documentation thoroughly explains thread safety and usage semantics, particularly for the
ready_txfield.
64-69: Module structure aligns perfectly with the PR objectives.The separation into
config,connection, andruntimemodules achieves the intended modularization. The test utilities are properly gated with#[cfg(test)].
|
@coderabbitai please create new GitHub issues for the following:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll create both GitHub issues for you with detailed descriptions and appropriate context. @leynos I've created both GitHub issues as requested:
Both issues include context from this PR and are assigned to you. They provide clear problem statements and reference the suggested solutions you mentioned. (( ).)_) 🏁 Script executed: Length of output: 1653 Perfect! Both GitHub issues have been successfully created: Issue #263: Enforce server binding at compile time instead of runtime panics
Issue #264: Make exponential backoff parameters configurable in accept_loop
Both issues include detailed problem descriptions, suggested solutions, context from this PR, and are assigned to you for follow-up implementation. (( ).)_) |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/server/config.rs(1 hunks)src/server/connection.rs(1 hunks)src/server/runtime.rs(1 hunks)src/server/test_util.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/server/connection.rssrc/server/test_util.rssrc/server/runtime.rssrc/server/config.rs
🔇 Additional comments (13)
src/server/connection.rs (4)
16-49: Well-structured panic-safe connection handlerThe implementation properly isolates connection panics and provides detailed logging. The peer address retrieval error handling and panic message extraction are thorough.
51-84: Comprehensive preamble processing with proper error handlingThe function correctly handles all preamble decoding scenarios, including the case where no failure handler is set (as previously requested in reviews). The error logging provides good observability.
105-151: Thorough test for panic logging behaviourThe test effectively verifies that connection task panics are caught and logged with appropriate details including the panic message and peer address.
153-213: Excellent integration test for server resilienceThe test comprehensively verifies that the server continues operating correctly after a connection task panics, ensuring fault tolerance. The verification of both continued operation and proper logging is thorough.
src/server/test_util.rs (3)
11-15: Simple and effective test preamble structureThe
TestPreamblestruct is well-designed for testing with appropriate trait derivations.
17-29: Well-implemented test fixturesBoth fixtures follow best practices. The
free_portfixture cleverly uses OS port allocation to avoid conflicts in parallel test execution.
31-45: Effective test helper functionsThe helper functions reduce boilerplate and improve test readability. Good application of DRY principles.
src/server/runtime.rs (2)
75-83: Robust shutdown coordinationThe shutdown logic correctly handles both explicit shutdown signals and natural task completion. The use of
TaskTrackerensures all tasks complete before shutdown.
126-255: Comprehensive runtime test coverageThe test suite thoroughly covers all runtime scenarios including error cases, shutdown paths, and worker management. Excellent use of fixtures and focused test cases.
src/server/config.rs (4)
17-58: Well-designed server initialization and type conversionThe
newmethod provides sensible defaults, andwith_preamblecorrectly resets callbacks when changing preamble types to prevent type mismatches.
65-105: Robust configuration methods with proper validationThe configuration methods provide a clean builder API. The worker count validation and thread-safe callback storage are well-implemented.
113-135: Clean binding implementation with proper code reuseThe bind methods effectively use the helper function pattern to eliminate duplication while maintaining clear public APIs. The error handling for non-blocking configuration is appropriate.
138-359: Exemplary test coverage with proper organizationThe test suite comprehensively covers all configuration scenarios including edge cases. The
expected_default_worker_counthelper effectively eliminates duplication as requested in past reviews. Excellent use of rstest fixtures throughout.
There was a problem hiding this comment.
Gates Failed
(1 file with Code Duplication)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| config.rs | 1 advisory rule | 10.00 → 9.39 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/server/config.rs(1 hunks)src/server/mod.rs(1 hunks)src/server/runtime.rs(1 hunks)src/server/test_util.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Function documentation must include clear examples demonstrating the usage and outcome of the function. Test documentation should omit examples where the example serves only to reiterate the test logic.
No single code file may be longer than 400 lines. Long switch statements or dispatch tables should be broken up by feature and constituents colocated with targets. Large blocks of test data should be moved to external data files.
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 using Arc to reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
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.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Avoid unsafe code unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not use return in single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Prefer .expect() over .unwrap().
Use concat!() to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums. Derive std::error::Error (via the thiserror crate) for any condition the caller might inspect, retry, or map to an HTTP status....
Files:
src/server/test_util.rssrc/server/runtime.rssrc/server/mod.rssrc/server/config.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/server/test_util.rssrc/server/runtime.rssrc/server/mod.rssrc/server/config.rs
🧬 Code Graph Analysis (3)
src/server/test_util.rs (1)
src/server/config.rs (5)
WireframeServer(282-282)WireframeServer(360-362)new(27-38)bind(152-155)bind(182-185)
src/server/mod.rs (2)
src/server/test_util.rs (1)
WireframeServer(44-44)src/server/config.rs (2)
WireframeServer(282-282)WireframeServer(360-362)
src/server/config.rs (2)
tests/preamble.rs (1)
oneshot(71-71)src/server/test_util.rs (5)
WireframeServer(44-44)factory(18-20)bind_server(31-38)free_port(23-29)server_with_preamble(40-45)
🔇 Additional comments (3)
src/server/test_util.rs (1)
1-45: Test utilities are well-structured.The module provides appropriate test helpers with clear responsibilities. Good use of rstest fixtures for shared setup.
src/server/runtime.rs (1)
1-240: Runtime implementation is robust and well-tested.The module properly implements server runtime with graceful shutdown, exponential backoff for accept errors, and comprehensive test coverage. The previously identified issues regarding runtime panics and hardcoded backoff parameters have been addressed.
src/server/mod.rs (1)
1-77: Server module structure is well-designed.The module properly uses the typestate pattern for compile-time safety, has clear type aliases for callbacks, and comprehensive documentation. The modular structure improves maintainability compared to the monolithic design it replaces.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
- Refactor the duplicated bind logic in both the Unbound and Bound impls into a single private helper to reduce repetition and simplify maintenance.
- Consider using tokio::task::JoinSet instead of tokio_util::TaskTracker for spawning and managing worker tasks, which can streamline shutdown handling and align with current Tokio patterns.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Refactor the duplicated bind logic in both the Unbound and Bound impls into a single private helper to reduce repetition and simplify maintenance.
- Consider using tokio::task::JoinSet instead of tokio_util::TaskTracker for spawning and managing worker tasks, which can streamline shutdown handling and align with current Tokio patterns.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@coderabbitai Please create new GitHub issues for the following:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll create both GitHub issues for the additional improvement suggestions you've identified. @leynos I've created both GitHub issues as requested:
Both issues include detailed problem descriptions, suggested solutions, context from this PR, and are assigned to you for follow-up implementation. (( ).)_) 🏁 Script executed: Length of output: 1903 Perfect! Both GitHub issues have been successfully created: Issue #275: Refactor duplicated bind logic in server config implementations
Issue #276: Consider using tokio::task::JoinSet instead of TaskTracker for worker management
Both issues include detailed problem descriptions, suggested solutions, context from this PR, references to related issues, and are assigned to you for follow-up implementation. (( ).)_) |
1 participant
Summary
server.rsTesting
make lintmake testhttps://chatgpt.com/codex/tasks/task_e_68912bb0f8e48322a1c2ef700f2d776b
Summary by Sourcery
Split the WireframeServer into dedicated config, runtime, and connection modules and remove the legacy monolithic server implementation
Enhancements:
Tests: