Restore server typestate and split configuration#279
Conversation
Reviewer's GuideThis PR refactors the server to enforce compile-time binding state via a typestate pattern, splits the configuration builder into dedicated binding and preamble modules, introduces a new ServerError type for runtime failures, updates example code to align with the new error and typestate APIs, and applies minor documentation formatting tweaks with an added thiserror dependency. Class diagram for configuration module splitclassDiagram
class config {
}
class binding {
}
class preamble {
}
config --> binding : mod binding
config --> preamble : mod preamble
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbitNew Features
Refactor
Bug Fixes
Documentation
Chores
WalkthroughRefactor the server configuration and runtime to use a typestate pattern with explicit Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WireframeServer (Unbound)
participant WireframeServer (Bound)
participant TcpListener
participant AppFactory
User->>WireframeServer (Unbound): new(factory)
User->>WireframeServer (Unbound): with_preamble()/on_preamble_decode_success()
User->>WireframeServer (Unbound): bind(addr)
WireframeServer (Unbound)->>TcpListener: bind(addr)
TcpListener-->>WireframeServer (Bound): listener (Arc)
User->>WireframeServer (Bound): run().await
WireframeServer (Bound)->>AppFactory: create app
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}
![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:
- Verify that the new config/binding and config/preamble modules are publicly re‐exporting all builder methods (with_preamble, on_preamble_decode_*) so existing fluent calls still compile without extra imports.
- Consider extending ServerError to capture binding/setup failures (not just accept errors) and unify the error types returned by run/run_with_shutdown instead of mixing io::Error and ServerError.
- Align the example mains to use the new ServerError (or a concrete error type) instead of generic Box to improve clarity and consistency of error handling.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Verify that the new config/binding and config/preamble modules are publicly re‐exporting all builder methods (with_preamble, on_preamble_decode_*) so existing fluent calls still compile without extra imports.
- Consider extending ServerError to capture binding/setup failures (not just accept errors) and unify the error types returned by run/run_with_shutdown instead of mixing io::Error and ServerError.
- Align the example mains to use the new ServerError (or a concrete error type) instead of generic Box<dyn Error> to improve clarity and consistency of error handling.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: 4
📜 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 (14)
Cargo.toml(1 hunks)README.md(1 hunks)docs/asynchronous-outbound-messaging-design.md(4 hunks)examples/echo.rs(2 hunks)examples/metadata_routing.rs(1 hunks)examples/packet_enum.rs(3 hunks)examples/ping_pong.rs(2 hunks)src/server/config/binding.rs(1 hunks)src/server/config/mod.rs(2 hunks)src/server/config/preamble.rs(1 hunks)src/server/error.rs(1 hunks)src/server/mod.rs(2 hunks)src/server/runtime.rs(5 hunks)src/server/test_util.rs(3 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:
README.mddocs/asynchronous-outbound-messaging-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:
examples/echo.rsexamples/packet_enum.rssrc/server/error.rsexamples/ping_pong.rssrc/server/test_util.rsexamples/metadata_routing.rssrc/server/config/mod.rssrc/server/config/binding.rssrc/server/runtime.rssrc/server/mod.rssrc/server/config/preamble.rs
🧬 Code Graph Analysis (4)
examples/echo.rs (1)
examples/ping_pong.rs (1)
main(139-149)
examples/ping_pong.rs (3)
examples/echo.rs (1)
main(12-33)examples/packet_enum.rs (1)
main(79-97)examples/metadata_routing.rs (1)
main(63-108)
src/server/test_util.rs (1)
src/server/config/tests.rs (2)
WireframeServer(60-60)WireframeServer(135-137)
src/server/config/preamble.rs (3)
src/extractor.rs (1)
std(270-270)src/server/test_util.rs (1)
WireframeServer(46-46)src/server/config/mod.rs (1)
new(37-48)
🪛 LanguageTool
docs/asynchronous-outbound-messaging-design.md
[uncategorized] ~72-~72: Loose punctuation mark.
Context: ...low_priority_push_rx: mpsc::Receiver`: For standard, non-urgent background ...
(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (26)
docs/asynchronous-outbound-messaging-design.md (2)
41-47: LGTM! Table formatting improvements enhance readability.The aligned table columns make the requirements section much more readable and professional.
72-98: LGTM! Numbering sequence properly corrected.The sequential numbering (1, 2, 3, 4) for the polling order steps is now accurate and follows logical progression.
README.md (1)
86-86: LGTM! Minor formatting improvement.Removing the unnecessary line break improves text flow whilst preserving the markdownlint directive for the long URL.
examples/echo.rs (2)
12-12: LGTM! Improved error handling with generalised return type.The change to
Result<(), Box<dyn std::error::Error>>provides better compatibility with the newServerErrortype and follows the pattern established across other examples.
31-32: LGTM! Clean error propagation and explicit success handling.Using the
?operator for error propagation and explicitOk(())return improves code clarity and consistency with other examples.examples/metadata_routing.rs (1)
63-63: Approve error handling generalisation in metadata_routing.rsError handling pattern matches echo.rs: async calls use
?operator and an explicitOk(())return is present.
- examples/metadata_routing.rs:
client.write_all(&bytes).await?;,client.shutdown().await?;- examples/metadata_routing.rs: explicit
Ok(())at end ofmainsrc/server/error.rs (3)
1-1: LGTM!Module documentation properly explains purpose and follows required format with
//!comment.
3-5: LGTM!Imports are minimal and appropriate for the error handling implementation.
7-13: Well-designed error type with proper derives and automatic conversion.The enum follows Rust error handling best practices with appropriate derives, clear documentation, and automatic conversion from
io::Error. The design allows for easy extension with additional variants as the server functionality grows.examples/packet_enum.rs (2)
6-6: LGTM!Correctly removed unused
ioimport following the change fromio::Result<()>toBox<dyn std::error::Error>.
79-79: LGTM!Main function signature updated to use general error type and properly propagates errors with
?operator. Changes align with the newServerErrortype and are consistent with other examples.Also applies to: 95-96
examples/ping_pong.rs (4)
6-6: LGTM!Correctly removed unused
ioimport following the error handling changes.
139-139: LGTM!Function signature correctly updated to use general error type, consistent with other examples.
146-146: LGTM!Simplified address parsing by removing unnecessary error mapping. The
?operator handles conversion automatically with the new error type.
147-148: LGTM!Properly propagates errors with
?operator after await and returns explicitOk(()). Changes are consistent with other examples.src/server/test_util.rs (2)
8-8: LGTM!Correctly added
Boundimport to support the updated return type using typestate markers.
32-32: LGTM!Return type correctly updated to include
Boundstate marker, ensuring compile-time verification of server binding state.src/server/runtime.rs (5)
3-3: LGTM!Import changes correctly reflect the transition to custom error handling and typestate system. Removed unused
ioimport and added necessaryBoundandServerErrorimports.Also applies to: 15-15, 20-20
27-27: Excellent use of typestate pattern for compile-time safety.Constraining the impl block to
Boundservers prevents runtime errors by ensuring these methods can only be called on properly bound servers.
52-53: LGTM!Method signatures correctly updated to return custom
ServerErrortype, providing better domain-specific error handling than generic IO errors.Also applies to: 92-92
96-104: Excellent elimination of runtime checks through typestate design.The destructuring pattern
state: Bound { listener }leverages the type system to guarantee listener availability, removing the need for runtime Option checks. This is a significant safety improvement.
27-135: Outstanding architectural improvement with typestate pattern and custom errors.The combination of compile-time binding state verification and domain-specific error types significantly improves safety and maintainability. The elimination of runtime checks through type-level guarantees is exemplary Rust design.
src/server/mod.rs (1)
36-77: Well-implemented typestate pattern for compile-time safety.The introduction of
UnboundandBoundmarker types with theServerStatetrait provides excellent compile-time guarantees about the server's binding state. The use ofArc<TcpListener>inBoundcorrectly enables sharing between worker threads.src/server/config/preamble.rs (1)
1-101: Clean separation of preamble configuration concerns.The extraction of preamble configuration into a dedicated module improves modularity. The builder pattern implementation is correct, and the decision to reset callbacks when changing preamble types (lines 42-43) prevents potential type confusion issues.
src/server/config/mod.rs (1)
15-16: Module organisation enhances separation of concerns.The extraction of binding and preamble configuration into dedicated submodules improves code organisation and maintainability.
src/server/config/binding.rs (1)
1-177: Excellent implementation of typestate-based binding configuration.The binding module correctly implements state transitions between
UnboundandBoundstates. The support for rebinding (lines 117-176) provides flexibility whilst maintaining type safety. Setting listeners to non-blocking mode before Tokio conversion (lines 78, 163) is the correct approach.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/server/test_util.rs (2)
11-15: Replace#[allow]with#[expect]per coding guidelines.The coding guidelines specifically forbid
#[allow]and require narrowly scoped#[expect(lint, reason = "...")]instead.Apply this diff:
#[cfg_attr( not(test), expect(dead_code, reason = "Used in builder tests via fixtures") )] -#[cfg_attr(test, allow(dead_code, reason = "Used in builder tests via fixtures"))] +#[cfg_attr(test, expect(dead_code, reason = "Used in builder tests via fixtures"))]
45-49: Replace#[allow]with#[expect]per coding guidelines.The coding guidelines specifically forbid
#[allow]and require narrowly scoped#[expect(lint, reason = "...")]instead.Apply this diff:
#[cfg_attr( not(test), expect(dead_code, reason = "Only used in configuration tests") )] -#[cfg_attr(test, allow(dead_code, reason = "Only used in configuration tests"))] +#[cfg_attr(test, expect(dead_code, reason = "Only used in configuration tests"))]
📜 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 (9)
Cargo.toml(1 hunks)examples/echo.rs(2 hunks)examples/packet_enum.rs(3 hunks)examples/ping_pong.rs(2 hunks)src/server/config/binding.rs(1 hunks)src/server/config/mod.rs(2 hunks)src/server/error.rs(1 hunks)src/server/runtime.rs(7 hunks)src/server/test_util.rs(3 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/error.rsexamples/echo.rsexamples/packet_enum.rssrc/server/test_util.rsexamples/ping_pong.rssrc/server/runtime.rssrc/server/config/mod.rssrc/server/config/binding.rs
🧬 Code Graph Analysis (1)
src/server/test_util.rs (1)
src/server/config/tests.rs (2)
WireframeServer(60-60)WireframeServer(135-137)
🔍 MCP Research (2 servers)
Deepwiki:
-
The binding configuration is split into a dedicated module implementing binding methods for
WireframeServer<F, T, Unbound>andWireframeServer<F, T, Bound>. The unbound server provideslocal_addrreturningNone, andbindandbind_listenermethods to bind the server and transition to the bound state. The bound server provideslocal_addrreturning the bound address and allows rebinding viabindandbind_listener. All binding methods return aResultwith a customServerErrortype for error handling. This modularization supports the typestate pattern and clean separation of binding logic. (src/server/config/binding.rs) -
A new
ServerErrorenum was introduced to replaceio::Errorin server binding and runtime operations. It has variantsBind(io::Error)for binding or listener configuration errors andAccept(io::Error)for connection acceptance failures. This explicit error type improves error handling clarity in the server's binding and runtime code. (src/server/error.rs) -
The
runandrun_with_shutdownmethods onWireframeServernow require the server to be in theBoundstate and returnResult<(), ServerError>instead ofio::Result<()>. The listener is extracted from theBoundstate struct rather than an optional field. This change aligns with the typestate pattern and custom error handling introduced in the PR. (src/server/runtime.rs) -
The
WireframeServerstruct uses a generic parameterSrepresenting the server state, with marker structsUnboundandBoundto represent binding states. TheUnboundstate has no listener, while theBoundstate holds anArc<TcpListener>. Thebindmethod transitions the server fromUnboundtoBoundby creating and wrapping a TCP listener. Therunandrun_with_shutdownmethods require the server to be in theBoundstate. This typestate pattern enforces at compile time that the server must be bound before running. (Wiki page "WireframeServer")
Context7:
-
The
WireframeServerstruct was extended with a new generic parameterSconstrained by a new traitServerState. The previous optionallistenerfield of typeOption<Arc<TcpListener>>was replaced by a singlestatefield of typeS. Two new marker structs,UnboundandBound, were introduced to represent server binding states.Boundholds alistenerof typeArc<TcpListener>, whileUnboundis an empty marker. TheServerStatetrait is implemented for bothUnboundandBound. These changes introduce a type-level distinction between bound and unbound server states, encapsulating the listener presence within thestatefield rather than an optional listener field. (src/server/mod.rs) -
The server configuration is modularized by splitting binding and preamble logic into separate submodules (
bindingandpreamble). TheWireframeServerconfiguration interface is simplified by removing binding and preamble callback methods from the main config module and moving them to dedicated modules. Thenewconstructor now returns an unbound server explicitly. This modularization clarifies responsibilities and improves maintainability. (src/server/config/mod.rs) -
A new Rust module
src/server/error.rswas added to define error types related toWireframeServeroperations. It introduces a public enumServerErrorthat derivesDebugandthiserror::Error. The enum currently has two variants:BindandAccept, both wrapping anio::Error. TheBindvariant represents errors occurring during binding or configuring the listener, with an error message formatted as"bind error: {0}". TheAcceptvariant represents failures during connection acceptance, with an error message formatted as"accept error: {0}". Both variants use the#[source]attribute to indicate the underlying source error. (src/server/error.rs) -
The
WireframeServerimplementation was updated to include a third generic parameterBound. Therunandrun_with_shutdownmethods' return types were changed fromio::Result<()>toResult<(), ServerError>, replacing standard IO error handling with a customServerErrortype. The internal destructuring inrun_with_shutdownwas modified to extract the listener from aBoundstruct (state: Bound { listener }) instead of directly from alistenerfield. The previous explicit check and error return for an unbound listener (listener.ok_or_else(...)) was removed. (src/server/runtime.rs)
🔇 Additional comments (21)
Cargo.toml (1)
30-30: LGTM! Dependency addition aligns with error handling refactoring.The addition of
thiserror2.0.12 supports the newServerErrorenum introduced in the codebase and addresses the previous feedback about using the latest version.examples/echo.rs (3)
8-8: LGTM! Proper integration with new error handling.The import correctly brings in
ServerErrorto align with the updated server error handling system.
12-12: LGTM! Return type updated correctly.The function signature properly reflects the new error handling pattern using
ServerErrorinstead ofio::Error.
31-32: LGTM! Error propagation implemented correctly.The use of the
?operator after.awaitproperly propagatesServerError, and the explicitOk(())return satisfies the new return type.examples/packet_enum.rs (4)
6-6: LGTM! Import cleanup and explicit HashMap import.Removing unused
std::ioand explicitly importingHashMapimproves code clarity.
14-14: LGTM! Proper ServerError import.The import correctly brings in the new error type for consistent error handling across the server API.
79-79: LGTM! Function signature updated appropriately.The return type change to
Result<(), ServerError>aligns with the new error handling system.
95-96: LGTM! Error propagation implemented correctly.The
?operator after.awaitproperly propagates errors, and the explicitOk(())return satisfies the function signature.examples/ping_pong.rs (4)
6-6: LGTM! Import statement cleaned up.Removing unused
std::ioand keeping only necessary imports improves code clarity.
14-14: LGTM! Consistent error type import.The
ServerErrorimport aligns with the updated server error handling across all examples.
139-139: LGTM! Return type updated correctly.The function signature properly adopts the new
ServerErrortype for consistent error handling.
146-148: LGTM! Streamlined error handling in example.The use of
expectfor address parsing is appropriate for example code, and the method chaining with proper error propagation demonstrates the updated server API effectively.src/server/error.rs (3)
1-2: LGTM! Proper module documentation.The module-level documentation clearly explains the purpose and follows the coding guideline requirement for
//!doc comments.
3-6: LGTM! Appropriate dependencies imported.The imports are minimal and necessary for the error handling implementation using
thiserror.
7-17: LGTM! Well-designed error enum with clear separation of concerns.The
ServerErrorenum provides distinct variants for binding and acceptance failures, uses proper error message formatting, and correctly implements the#[source]attribute for error chaining. The design aligns well with the typestate pattern implementation in the broader codebase.src/server/test_util.rs (1)
36-43: Return type correctly reflects typestate pattern.The updated return type
WireframeServer<F, (), Bound>properly enforces thatbind_serverreturns a bound server, aligning with the typestate pattern implementation.src/server/runtime.rs (2)
27-58: Typestate implementation correctly enforces binding requirement.The constraint to
WireframeServer<F, T, Bound>ensures compile-time verification that the server is bound before running, and theServerErrorreturn type provides clearer error semantics.
92-104: State destructuring correctly leverages typestate guarantees.The extraction of the listener from
Bound { listener }eliminates the need for runtime checks, as the typestate pattern guarantees the listener's presence at compile time.src/server/config/mod.rs (1)
15-16: Modularization properly separates binding and preamble concerns.The introduction of dedicated
bindingandpreamblesubmodules improves code organization and maintainability, aligning with the single responsibility principle.src/server/config/binding.rs (2)
14-94: Unbound state implementation correctly handles binding transitions.The methods properly transition from
UnboundtoBoundstate, preserve configuration across transitions, and use appropriate error handling withServerError::Bind.
96-179: Bound state implementation provides flexible rebinding capabilities.The rebinding methods maintain the
Boundstate while allowing address changes, providing flexibility without sacrificing type safety.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/server/config/mod.rs (1)
71-74: Encode non-zero worker invariant at the type level (optional).Avoid runtime clamping by modelling
workersasNonZeroUsizeand accepting that inworkers(...). This tightens invariants and can catch misuse at compile time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/server/config/mod.rs(2 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/config/mod.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/config/mod.rs
🔍 MCP Research (1 server)
Deepwiki:
-
The
WireframeServerconfiguration module was refactored to remove all direct handling of TCP listener binding and preamble callback registration from the main builder interface. The generic parameterS: ServerStateis added to theWireframeServerimpl, and thenewconstructor now returns a server explicitly in theUnboundstate instead of holding an optional listener. All methods related to preamble customization (with_preamble), preamble decode success and failure callbacks (on_preamble_decode_success,on_preamble_decode_failure), and TCP binding (bind,bind_listener,local_addr) have been removed from this module. Correspondingly, imports related to standard IO errors, socket addresses, TCP listeners, futures, and synchronization primitives likeArchave been removed. The module now only importstokio::sync::oneshotand internal state types. Two new submodules,bindingandpreamble, have been introduced to encapsulate the removed functionality. The remaining builder methods focus solely on worker count configuration and ready-signal channel setup. Documentation comments have been updated to reflect that TCP binding and preamble behavior customization are now handled externally via the new submodules. Overall, the configuration API in this file is narrowed to basic server setup without direct TCP binding or preamble callback configuration. (src/server/config/mod.rs) -
This addition implements binding configuration methods for the
WireframeServertype parameterized by a factoryFand a preamble typeT, supporting both unbound and bound server states. For the unbound server (WireframeServer<F, T, Unbound>), it provides:- A
local_addrmethod that always returnsNonesince the server is not yet bound. - A
bindmethod that binds the server to a specified socket address by creating a new standard TCP listener and delegating tobind_listener. - A
bind_listenermethod that accepts an existing standard TCP listener, sets it to non-blocking mode, converts it to a Tokio TCP listener, and returns a newWireframeServerinstance in the bound state with the listener wrapped in anArc.
For the bound server (WireframeServer<F, T, Bound>), it provides: - A
local_addrmethod that returns the socket address the server is bound to, orNoneif retrieval fails. - A
bindmethod that allows rebinding to a new socket address by creating a new standard TCP listener and delegating tobind_listener. - A
bind_listenermethod that allows rebinding using an existing standard TCP listener, similarly setting it to non-blocking, converting to Tokio listener, and returning a new bound server instance.
All binding methods return aResultwith aServerErrorvariant to handle errors during binding or listener configuration. The server's internal state transitions from unbound to bound by wrapping the Tokio listener in anArcand storing it in theBoundstate. The factory, worker count, preamble handlers, and readiness channel are preserved across these transitions. The code includes documentation and usage examples for each method. (src/server/config/binding.rs)
- A
-
This addition introduces preamble configuration methods for the
WireframeServerstruct, enabling customization of the connection preamble type and handling. It adds a generic implementation block forWireframeServerparameterized over a factoryF, a preamble typeT, and a server stateS. The new methods include:with_preamble<P>(): Converts the server instance to use a specified preamble typePthat implements thePreambletrait, resetting any previously set preamble decode callbacks.on_preamble_decode_success<H>(handler): Registers an asynchronous callback invoked when the connection preamble decodes successfully. The handler receives a reference to the decoded preamble and a mutable reference to the TCP stream, returning a boxed future resolving to anio::Result<()>.on_preamble_decode_failure<H>(handler): Registers a synchronous callback invoked when the preamble decoding fails, receiving a reference to theDecodeError.
All callbacks are stored internally asArc-wrapped closures to allow shared ownership and thread safety. The methods use builder-style chaining by returningselfor a newWireframeServerinstance with updated generic parameters. The code includes documentation and usage examples demonstrating how to specify a custom preamble type and register success and failure handlers. (src/server/config/preamble.rs)
-
A new Rust module
src/server/error.rswas added to define error types related toWireframeServeroperations. It introduces a public enumServerErrorthat derivesDebugandthiserror::Error. The enum currently has two variants:BindandAccept, both wrapping anio::Error. TheBindvariant represents errors occurring during binding or configuring the listener, with an error message formatted as"bind error: {0}". TheAcceptvariant represents failures during connection acceptance, with an error message formatted as"accept error: {0}". Both variants use the#[source]attribute to indicate the underlying source error. (src/server/error.rs)
🔇 Additional comments (5)
src/server/config/mod.rs (5)
12-18: Module split and imports: LGTM.Expose
bindingandpreambleas public submodules and restrict imports to what this module uses. This aligns with the typestate split and Deepwiki decisions.
48-49: Initialise typestate toUnbound: LGTM.Restore explicit typestate instead of optional listener. This is consistent with the compile-time state model.
54-59: Generic state bound overS: ServerState: LGTM.Expose builder methods for all states via
S: ServerState. This matches the intended typestate design.
17-18: No action needed: module-level docs present
Remove this review comment. Bothsrc/server/config/binding.rsandsrc/server/config/preamble.rsbegin with//!module‐level documentation as required.
20-20: Document default preamble innew()Update the
new()method’s documentation insrc/server/config/mod.rsto state that it returns a server configured with the unit type (()) as the default preamble.Verify that
()implements thePreambletrait (which requiresBorrowDecode<'de, ()>) in this crate.
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 (1)
src/server/config/mod.rs(2 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/config/mod.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/config/mod.rs
🔍 MCP Research (1 server)
Deepwiki:
-
The
WireframeServerconfiguration module (src/server/config/mod.rs) has been simplified by removing all direct handling of TCP listener binding and preamble callback registration from the main builder interface. The generic parameterS: ServerStatewas added to theWireframeServerimpl, and thenewconstructor now returns a server explicitly in theUnboundstate instead of holding an optional listener. Methods related to preamble customization and TCP binding (with_preamble,on_preamble_decode_success,on_preamble_decode_failure,local_addr,bind,bind_listener) were removed. Two new public submodules,bindingandpreamble, were introduced to encapsulate the removed functionality. Remaining builder methods focus on worker count configuration and ready-signal channel setup. (src/server/config/mod.rs) -
The
src/server/config/binding.rsmodule adds binding configuration methods forWireframeServerparameterized by factoryFand preamble typeT, supporting both unbound and bound server states. For unbound servers, it provideslocal_addr(alwaysNone),bind(binds to a socket address), andbind_listener(binds using an existing standard TCP listener). For bound servers, it provideslocal_addr(returns bound socket address),bind(rebinds to new address), andbind_listener(rebinds using existing listener). All binding methods returnResultwith aServerErrorvariant. The server's internal state transitions from unbound to bound by wrapping the Tokio listener in anArcand storing it in theBoundstate. (src/server/config/binding.rs) -
The
src/server/config/preamble.rsmodule introduces preamble configuration methods forWireframeServer, enabling customization of the connection preamble type and handling. It adds methodswith_preamble<P>(),on_preamble_decode_success(handler), andon_preamble_decode_failure(handler). The success callback is asynchronous and receives a reference to the decoded preamble and a mutableTcpStream, returning a boxed future resolving toio::Result. The failure callback is synchronous and receives a reference to theDecodeError. Callbacks are stored internally asArc-wrapped closures. (src/server/config/preamble.rs) -
The
src/server/config/mod.rschanges include removal of imports related to standard IO errors, socket addresses, TCP listeners, futures, and synchronization primitives likeArc. The module now only importstokio::sync::oneshotand internal state types. Documentation comments were updated to reflect that TCP binding and preamble behavior customization are now handled externally via the new submodules. (src/server/config/mod.rs)
🔇 Additional comments (4)
src/server/config/mod.rs (4)
20-23: Approve typestate-aware default constructor target.Defaulting to
WireframeServer<F, (), Unbound>is correct and clearly encodes the initial unbound state in the type.
48-48: Approve typestate initialisation.Initialise
statewithUnboundto enforce the correct initial typestate.
54-59: Approve impl bounds and generalisation.Constrain
S: ServerStateon the generic impl to make builder methods available across server states without leaking binding concerns.
17-18: No action required—bothbinding.rsandpreamble.rsinclude top-of-file//!comments.
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/server/config/mod.rs(2 hunks)src/server/config/preamble.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/config/mod.rssrc/server/config/preamble.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/config/mod.rssrc/server/config/preamble.rs
🧬 Code Graph Analysis (1)
src/server/config/preamble.rs (1)
src/server/test_util.rs (1)
WireframeServer(54-54)
🔍 MCP Research (1 server)
Deepwiki:
-
The
WireframeServerconfiguration module was refactored to remove all direct TCP listener binding and preamble callback registration from the main builder interface. TheWireframeServerstruct now has a generic parameterS: ServerStaterepresenting the server's binding state. Thenewconstructor returns a server in theUnboundstate. Methods related to preamble customization and TCP binding (with_preamble,on_preamble_decode_success,on_preamble_decode_failure,local_addr,bind,bind_listener) were removed from this module. Two new public submodules,bindingandpreamble, were introduced to encapsulate the removed functionality. The remaining builder methods focus on worker count and ready-signal channel setup, implemented via macros. This narrows the configuration API to basic server setup without direct TCP binding or preamble callback configuration. (src/server/config/mod.rs) -
This addition introduces preamble configuration methods for
WireframeServer, enabling customization of the connection preamble type and handling. It adds generic implementations forWireframeServer<F, T, S>with methods:with_preamble<P>()to specify a preamble type implementing thePreambletrait, resetting any previously set preamble decode callbacks.on_preamble_decode_success<H>(handler)to register an async callback invoked on successful preamble decode.on_preamble_decode_failure<H>(handler)to register a synchronous callback invoked on preamble decode failure.
All callbacks are stored asArc-wrapped closures for thread safety. The methods use builder-style chaining and include documentation and usage examples. (src/server/config/preamble.rs)
-
This addition implements binding configuration methods for
WireframeServer<F, T, Unbound>andWireframeServer<F, T, Bound>. For the unbound server, it provides:local_addr()returningNonesince not bound.bind(addr)to bind to a socket address by creating a standard TCP listener and delegating tobind_listener.bind_listener(std)to accept an existing standard TCP listener, set it non-blocking, convert to Tokio TCP listener, and return a new bound server instance.
For the bound server, it provides:local_addr()returning the bound socket address orNoneon failure.bind(addr)andbind_listener(std)to rebind to a new address or listener.
All binding methods returnResultwith aServerErrorvariant for error handling. The server transitions from unbound to bound state by wrapping the Tokio listener in anArc. The factory, worker count, preamble handlers, and readiness channel are preserved across transitions. Documentation and usage examples are included. (src/server/config/binding.rs)
-
A new module defining the
ServerErrorenum was added. It derivesDebugandthiserror::Errorand has two variants:Bind(io::Error)representing errors during binding or listener configuration, with error message"bind error: {0}".Accept(io::Error)representing failures during connection acceptance, with error message"accept error: {0}".
Both variants use the#[source]attribute to indicate the underlying source error. (src/server/error.rs)
🔇 Additional comments (7)
src/server/config/preamble.rs (2)
1-1: Module-level docs meet guidelinesKeep this. It satisfies the “every module must begin with //!” rule and succinctly scopes the module.
34-48: with_preamble: correct state-preserving conversion; reset callbacks as intendedThe typestate-preserving rebuild is correct and the reset of both callbacks is appropriate.
src/server/config/mod.rs (5)
4-9: Docs: clear, linked, and en‑GB compliantThe module‑level docs clearly separate responsibilities, use intra‑doc links, and follow the -ize/-our rule. Keep this.
18-27: Builder setter macro: OKThe setter macro is minimal, attributes are placed after doc comments, and it returns
Selffor fluent chaining.
46-79: Constructor: typestate initialisation and defaults look correctInitialise to
Unbound, clamp workers viaavailable_parallelismwith safe fallback, and default callback fields andready_txtoNone. Matches the typestate design.
101-114: ready_signal(): clear API and exampleAccept
oneshot::Sender<()>and store asSome(tx); this is straightforward and documented.
4-9: Remove the verification request – intra-doc links resolve correctly
Thebindandbind_listenermethods onWireframeServer<_,_,Unbound>live in src/server/config/binding.rs and theUnboundtypestate lives in src/server/mod.rs. Rustdoc will correctly resolvebind,bind_listener, and [Unbound].
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
src/server/config/preamble.rs (1)
74-75: Reuse thePreambleSuccessHandleralias.
Replace the longfor<'a> Fn …bound with thecrate::server::PreambleSuccessHandler<T>alias to keep the API surface tidy and avoid future drift.src/server/config/mod.rs (1)
29-41: Fully qualify Arc in macro expansionGood fix; this de-couples call sites from needing
use Arc.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/server/config/mod.rs(2 hunks)src/server/config/preamble.rs(1 hunks)src/server/mod.rs(3 hunks)tests/preamble.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:
tests/preamble.rssrc/server/mod.rssrc/server/config/preamble.rssrc/server/config/mod.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:
tests/preamble.rssrc/server/mod.rssrc/server/config/preamble.rssrc/server/config/mod.rs
🧬 Code Graph Analysis (3)
src/server/mod.rs (1)
tests/preamble.rs (2)
WireframeServer(53-55)WireframeServer(234-256)
src/server/config/preamble.rs (2)
tests/preamble.rs (6)
WireframeServer(53-55)WireframeServer(234-256)tokio(129-129)tokio(130-130)tokio(228-228)tokio(229-229)src/server/test_util.rs (1)
WireframeServer(54-54)
src/server/config/mod.rs (2)
tests/preamble.rs (7)
tokio(129-129)tokio(130-130)tokio(228-228)tokio(229-229)oneshot(71-71)WireframeServer(53-55)WireframeServer(234-256)src/server/config/tests.rs (2)
WireframeServer(60-60)WireframeServer(135-137)
🔍 MCP Research (1 server)
Deepwiki:
-
The
WireframeServerconfiguration module was refactored to remove direct TCP binding and preamble callback registration from the main builder interface, moving these into new public submodulesbindingandpreamble. The server now uses a generic parameterS: ServerStateto represent binding state, with the constructor returning an unbound server. (src/server/config/mod.rs) -
The addition of
src/server/config/preamble.rsintroduces preamble configuration methods forWireframeServer, allowing specification of the preamble type and registration of asynchronous success and synchronous failure callbacks stored asArcclosures, supporting builder-style chaining and resetting previous callbacks when changing the preamble type. (src/server/config/preamble.rs) -
The addition of
src/server/config/binding.rsimplements binding configuration methods forWireframeServersupporting unbound and bound server states, with methods likebindandbind_listenertransitioning the server state and returningResultwith a newServerErrortype for error handling. (src/server/config/binding.rs) -
The
WireframeServerstruct was extended with a generic parameterS: ServerStaterepresenting binding state, replacing the optional listener field with a state field holding eitherUnboundorBoundmarker structs. The preamble success callback type alias was split into a new async handler type alias. A newerrormodule was added, and runtime methods now returnResult<(), ServerError>. (src/server/mod.rs)
🔇 Additional comments (7)
src/server/config/mod.rs (7)
4-9: Module-level docs: clear responsibilities and correct intra-doc linksKeep this; the module docs correctly link to binding/preamble and use en-GB with -ize/-our spelling.
18-27: Builder setter macro: sound and idiomaticRetain this;
#[must_use]on fluent setters is correct and assignment via$assignkeeps call sites concise.
43-44: Expose binding and preamble as public submodulesGood split that aligns with the typestate design and documented responsibilities.
53-56: Constructor docs: link Unbound and binding methodsDocs precisely describe the initial typestate and provide navigable links to
bind/bind_listener.
68-76: Initialise workers and typestate robustlyUsing
available_parallelism().map_or(1, NonZeroUsize::get)guarantees a sensible default; settingstate: Unboundmatches the constructor docs.
106-119: ready_signal(): focused API and exampleThe API surface is clear; the example uses
tokio::sync::oneshotas intended. Keep as-is.
131-134: worker_count(): const, inline, and side-effect freeThe accessor meets style guidance (no
returnin single-line functions) and is marked#[must_use]. LGTM.
There was a problem hiding this comment.
Gates Failed
(1 hotspot with Large Method)
(1 file with Large Method)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Prevent hotspot decline | Violations | Code Health Impact | |
|---|---|---|---|
| preamble.rs | 1 rule in this hotspot | 10.00 → 9.51 | Suppress |
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| preamble.rs | 1 advisory rule | 10.00 → 9.51 | 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 (3)
src/server/config/preamble.rs(1 hunks)src/server/mod.rs(3 hunks)tests/preamble.rs(3 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:
tests/preamble.rssrc/server/config/preamble.rssrc/server/mod.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:
tests/preamble.rssrc/server/config/preamble.rssrc/server/mod.rs
🧬 Code Graph Analysis (2)
src/server/config/preamble.rs (2)
tests/preamble.rs (2)
WireframeServer(53-55)WireframeServer(238-260)src/server/test_util.rs (1)
WireframeServer(54-54)
src/server/mod.rs (3)
tests/preamble.rs (2)
WireframeServer(53-55)WireframeServer(238-260)src/server/config/tests.rs (2)
WireframeServer(60-60)WireframeServer(135-137)src/server/test_util.rs (1)
WireframeServer(54-54)
🔍 MCP Research (2 servers)
Deepwiki:
-
Document src/server/config/preamble.rs introduces preamble configuration methods for
WireframeServer, enabling customization of connection preamble type and handling. It adds methodswith_preamble<P>()to specify a preamble type,on_preamble_decode_success<H>(handler)to register async callbacks on successful preamble decode, andon_preamble_decode_failure<H>(handler)to register sync callbacks on decode failure. Callbacks are stored asArcclosures for thread safety. (src/server/config/preamble.rs) -
Document src/server/mod.rs shows that
WireframeServerstruct is extended with a generic parameterS: ServerStaterepresenting binding state, replacing the optional listener field with a state field. Marker structsUnboundandBoundrepresent server states, withBoundholding the listener. ThePreambleCallbacktype alias is replaced by a traitPreambleSuccessHandler<T>. The module adds a new error module and re-exports config submodulesbindingandpreamble. (src/server/mod.rs) -
Document src/server/config/binding.rs implements binding configuration methods for
WireframeServer<F, T, Unbound>andWireframeServer<F, T, Bound>. For unbound servers,local_addr()returns None,bind()binds to a socket address creating a standard TCP listener, andbind_listener()accepts an existing listener, sets non-blocking mode, converts to Tokio listener, and returns a bound server instance. For bound servers,local_addr()returns the bound address, andbind()andbind_listener()allow rebinding. All methods returnResultwithServerErrorfor error handling. (src/server/config/binding.rs) -
Document src/server/error.rs adds a new enum
ServerErrorwith variantsBind(io::Error)for binding/configuration errors andAccept(io::Error)for connection acceptance failures. It derivesthiserror::Errorfor error handling. (src/server/error.rs)
Context7:
-
The server configuration module was refactored to remove direct TCP listener binding and preamble callback registration from the main builder interface, introducing generic server state tracking with
UnboundandBoundtypestates. Binding and preamble logic were moved to new submodulesbindingandpreamble, respectively, narrowing the configuration API to basic server setup without direct binding or preamble callback configuration. This modularization improves clarity and enforces compile-time state correctness. (src/server/config/mod.rs) -
The preamble configuration API was extended with methods to specify a custom preamble type and register asynchronous success and synchronous failure callbacks. These callbacks are stored as
Arc-wrapped closures for thread safety. The API uses builder-style chaining and supports resetting callbacks when changing preamble types. (src/server/config/preamble.rs) -
A new
ServerErrorenum was introduced to replaceio::Errorin server operations, with variants for binding and accepting errors. This change is reflected in runtime methods, which now returnResult<(), ServerError>instead ofio::Result<()>. (src/server/error.rs, src/server/runtime.rs) -
The
WireframeServerstruct was extended with a generic parameterSconstrained by aServerStatetrait, replacing the optional listener field with a state field of typeS. Marker structsUnboundandBoundrepresent the server's binding state, withBoundcontaining the listener wrapped in anArc. This typestate pattern enforces compile-time correctness of server binding state. (src/server/mod.rs) -
Tests in
tests/preamble.rswere enhanced to verify that callbacks associated with overridden preamble types are properly dropped, ensuring correct callback invocation behavior when changing preamble configurations. (tests/preamble.rs)
⏰ 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: build-test
🔇 Additional comments (8)
tests/preamble.rs (2)
173-176: Raise negative-assertion timeout to 500 ms — LGTMIncrease the window for the absent-callback assertion to reduce CI flakiness. This aligns with prior feedback.
184-187: Raise negative-assertion timeout to 500 ms — LGTMHarden the test against scheduler jitter by widening the timeout for the opposite callback. Good change.
src/server/config/preamble.rs (3)
20-52: with_preamble: correct typestate-preserving swap and explicit callback reset docs — LGTMPreserve the binding state
S, reset both callbacks, and document usage with an example. Attribute placement and module docs meet guidelines.
54-77: on_preamble_decode_success: clean handler bound and example — LGTMUse the
PreambleSuccessHandler<T>trait to reduce type noise and document the async handler shape clearly. Matches the refactor intent.
79-99: on_preamble_decode_failure: link the error type and show usage — LGTMDocument the sync failure handler with an intra-doc link to
bincode::error::DecodeError. Compliant with doc and example requirements.src/server/mod.rs (3)
16-35: Define an object-safe async handler trait for preamble success — LGTMKeep the handler object-safe and sendable via
Arc<dyn …>. Docs clarify permitted I/O before handoff.
78-91: Document typestate markers and trait — LGTMDescribe
Unbound,Bound, andServerStateclearly; this satisfies public-API documentation requirements and improves discoverability.
93-95: Re-exportbindingandpreambleconfig modules — LGTMExpose the split configuration surface from
server, aligning with the modular design.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/server/mod.rs (1)
49-58: Verify defaultT=()satisfies thePreamblebound; adjust if notThe comment asserts that
()already satisfiesPreamblevia bincode, but this depends on the presence of the requiredBorrowDecode/blanket impls. Validate to avoid surprises when constructingWireframeServer::new(factory)without.with_preamble().Run:
#!/bin/bash # 1) Find the Preamble trait and any blanket impls that might cover `()`. rg -n $'trait\\s+Preamble\\b' -A 20 rg -n $'impl<[^>]*>\\s+Preamble\\b' -A 10 # 2) Check whether bincode provides BorrowDecode or Decode for unit that your Preamble bound relies on. rg -n $'impl\\s+BorrowDecode\\b.*for\\s*\\(\\)' -A 2 rg -n $'impl\\s+Decode\\b.*for\\s*\\(\\)' -A 2 # 3) Grep for local impls that might cover unit. rg -n $'impl\\s+Preamble\\s+for\\s*\\(\\)' -A 2If
()does not satisfy the bound:
- Option A (preferred): Introduce a zero-sized
NoPreambletype in your preamble module that implementsPreamble, and change the default toT = NoPreamble.- Option B: Provide an explicit
impl Preamble for () { /* … */ }if allowed by your trait’s requirements.I can supply a patch for Option A on request.
tests/preamble.rs (2)
176-179: Approve increased timeouts to de-flake testsRaising the “should not receive” timeouts to 500 ms matches earlier feedback and improves robustness under scheduler jitter.
Also applies to: 187-190
258-310: Approve: deterministic synchronisation and clear assertionsAwait the success signal inside
with_running_serverto avoid arbitrary sleeps; then assert that all stale/incorrect callbacks were not invoked. This is precise and deterministic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/server/mod.rs(3 hunks)tests/preamble.rs(4 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:
tests/preamble.rssrc/server/mod.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:
tests/preamble.rssrc/server/mod.rs
🧬 Code Graph Analysis (1)
src/server/mod.rs (3)
tests/preamble.rs (4)
tokio(132-132)tokio(133-133)WireframeServer(56-58)WireframeServer(268-272)src/server/test_util.rs (1)
WireframeServer(54-54)src/server/config/tests.rs (2)
WireframeServer(60-60)WireframeServer(135-137)
🔍 MCP Research (1 server)
Deepwiki:
-
The
WireframeServerstruct was extended with a new generic parameterSconstrained by a new traitServerState. The previous optionallistenerfield was replaced by astatefield of typeS. Two new marker structs,UnboundandBound, were introduced to represent the server's binding state, withBoundcontaining thelistener. TheServerStatetrait is implemented for both marker structs. The type aliasPreambleCallback<T>was replaced by a new traitPreambleSuccessHandler<T>, which defines the signature for handlers invoked upon successful preamble decoding. This trait requires implementors to be async functions taking a reference toTand a mutable TCP stream, returning a boxed future of an I/O result, and to beSend,Sync, and'static. The originalPreambleCallback<T>is now anArcwrapping adyn PreambleSuccessHandler<T>. The documentation was updated to refer to "handler" and "implementors" instead of "callback." A new public moduleerrorwas added, and theconfigmodule'sbindingandpreamblesubmodules were re-exported. (src/server/mod.rs) -
The
WireframeServerconfiguration module was refactored to remove all direct handling of TCP listener binding and preamble callback registration from the main builder interface. The generic parameterS: ServerStateis added to theWireframeServerimpl, and thenewconstructor now returns a server explicitly in theUnboundstate instead of holding an optional listener. All methods related to preamble customization (with_preamble), preamble decode success and failure callbacks (on_preamble_decode_success,on_preamble_decode_failure), and TCP binding (bind,bind_listener,local_addr) have been removed from this module. Correspondingly, imports related to standard IO errors, socket addresses, TCP listeners, futures, and synchronization primitives likeArchave been removed. The module now only importstokio::sync::oneshotand internal state types. Two new public submodules,bindingandpreamble, have been introduced to encapsulate the removed functionality. The remaining builder methods focus solely on worker count configuration and ready-signal channel setup, implemented via macros for setter generation. Documentation comments have been updated to reflect that TCP binding and preamble behavior customization are now handled externally via the new submodules. Overall, the configuration API in this file is narrowed to basic server setup without direct TCP binding or preamble callback configuration. (src/server/config/mod.rs) -
This addition implements binding configuration methods for the
WireframeServertype parameterized by a factoryFand a preamble typeT, supporting both unbound and bound server states. For the unbound server (WireframeServer<F, T, Unbound>), it provides:- A
local_addrmethod that always returnsNonesince the server is not yet bound. - A
bindmethod that binds the server to a specified socket address by creating a new standard TCP listener and delegating tobind_listener. - A
bind_listenermethod that accepts an existing standard TCP listener, sets it to non-blocking mode, converts it to a Tokio TCP listener, and returns a newWireframeServerinstance in the bound state with the listener wrapped in anArc.
For the bound server (WireframeServer<F, T, Bound>), it provides: - A
local_addrmethod that returns the socket address the server is bound to, orNoneif retrieval fails. - A
bindmethod that allows rebinding to a new socket address by creating a new standard TCP listener and delegating tobind_listener. - A
bind_listenermethod that allows rebinding using an existing standard TCP listener, similarly setting it to non-blocking, converting to Tokio listener, and returning a new bound server instance.
All binding methods return aResultwith aServerErrorvariant to handle errors during binding or listener configuration. The server's internal state transitions from unbound to bound by wrapping the Tokio listener in anArcand storing it in theBoundstate. The factory, worker count, preamble handlers, and readiness channel are preserved across these transitions. The code includes documentation and usage examples for each method. (src/server/config/binding.rs)
- A
-
This addition introduces preamble configuration methods for the
WireframeServerstruct, enabling customization of the connection preamble type and handling. It adds a generic implementation block forWireframeServerparameterized over a factoryF, a preamble typeT, and a server stateS. The new methods include:with_preamble<P>(): Converts the server instance to use a specified preamble typePthat implements thePreambletrait, resetting any previously set preamble decode callbacks.on_preamble_decode_success<H>(handler): Registers an asynchronous callback invoked when the connection preamble decodes successfully. The handler receives a reference to the decoded preamble and a mutable reference to the TCP stream, returning a boxed future resolving to anio::Result<()>.on_preamble_decode_failure<H>(handler): Registers a synchronous callback invoked when the preamble decoding fails, receiving a reference to theDecodeError.
All callbacks are stored internally asArc-wrapped closures to allow shared ownership and thread safety. The methods use builder-style chaining by returningselfor a newWireframeServerinstance with updated generic parameters. The code includes documentation and usage examples demonstrating how to specify a custom preamble type and register success and failure handlers. (src/server/config/preamble.rs)
🔇 Additional comments (8)
src/server/mod.rs (7)
28-35: LGTM: blanket impl for handler trait is correctThe blanket impl correctly mirrors the trait bounds and remains object-safe via the Fn bound.
36-37: LGTM: alias preserves ergonomic storageWrapping the handler trait in
Arckeeps registration ergonomic and thread-safe.
76-76: LGTM: typestate field integrates cleanly
state: Scorrectly replaces the optional listener and pins the typestate at the type level.
80-87: LGTM: marker types are minimal and documented
Unbound/Boundare clear; wrapping the listener inArc<TcpListener>aligns with shared runtime access.
88-93: LGTM:ServerStatetrait and impls complete the typestate patternThe zero-method trait keeps the API lean while enforcing state tracking at compile time.
95-95: LGTM: re-export configuration modulesRe-exporting
bindingandpreambleclarifies the public API surface and supports discoverability.
97-97: LGTM: public error module enables semantic errorsExposing
errormatches the guideline to prefer semantic error enums andthiserrorderivations.tests/preamble.rs (1)
223-231: LGTM: helper types and closures reduce duplication and keep assertions crisp
OtherPreamble,Holder,channel_holder,success_cb, andfailure_cbneatly encapsulate boilerplate; the closures meet the required bounds and use.expect()per policy.Also applies to: 233-246, 248-256
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> rebase-temp-echo
leynos
force-pushed
the
codex/address-code-review-comments
branch
from
428862b to
da6dd67
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (2)
src/server/config/tests.rs (1)
98-118: Eliminate runtime panics by typing the test case instead of using &str.Replace the stringly typed
handler_type: &strwith a small enum to remove the default arm and unreachable/panic paths.Apply this diff:
-async fn test_preamble_handler_registration( - factory: impl Fn() -> WireframeApp + Send + Sync + Clone + 'static, - #[case] handler_type: &str, -) { +async fn test_preamble_handler_registration( + factory: impl Fn() -> WireframeApp + Send + Sync + Clone + 'static, + #[case] handler: HandlerKind, +) { @@ - let server = match handler_type { + let server = match handler { - "success" => server.on_preamble_decode_success(move |_p: &TestPreamble, _| { + HandlerKind::Success => server.on_preamble_decode_success(move |_p: &TestPreamble, _| { @@ - "failure" => server.on_preamble_decode_failure(move |_err: &DecodeError| { + HandlerKind::Failure => server.on_preamble_decode_failure(move |_err: &DecodeError| { @@ - _ => panic!("Invalid handler type"), }; @@ - match handler_type { - "success" => assert!(server.on_preamble_success.is_some()), - "failure" => assert!(server.on_preamble_failure.is_some()), - _ => unreachable!(), + match handler { + HandlerKind::Success => assert!(server.on_preamble_success.is_some()), + HandlerKind::Failure => assert!(server.on_preamble_failure.is_some()), }Add this outside the shown hunk:
#[derive(Clone, Copy)] enum HandlerKind { Success, Failure } #[rstest] #[case(HandlerKind::Success)] #[case(HandlerKind::Failure)]Also applies to: 121-125
src/server/runtime.rs (1)
129-157: Fix compile error: providebackoff_configand drop unstablelet-chains
backoff_configis passed toaccept_loopbut never defined in scope, so the code will not compile.
Introduce a locallet backoff_config = BackoffConfig::default();(or pull it from a field) before the worker-spawn loop.The
if let Some(tx) = ready_tx && tx.send(()).is_err()expression relies on the still-unstablelet_chainsfeature. Split the check into two statements or use a match to stay on stable Rust.- if let Some(tx) = ready_tx && tx.send(()).is_err() { + if let Some(tx) = ready_tx { + if tx.send(()).is_err() { + tracing::warn!("Failed to send readiness signal: receiver dropped"); + } + } + + let backoff_config = BackoffConfig::default();
♻️ Duplicate comments (6)
examples/echo.rs (1)
8-13: Return opaque error at app boundary (already raised).Switch
maintoResult<(), Box<dyn std::error::Error>>and drop the explicitServerErrorimport.examples/ping_pong.rs (1)
14-15: Return opaque error at app boundary (already raised).Replace
ServerErrorwithBox<dyn std::error::Error>inmainand imports.Also applies to: 139-149
examples/packet_enum.rs (1)
14-15: Return opaque error at app boundary (already raised).Adopt
Box<dyn std::error::Error>formainand removeServerErrorimport.Also applies to: 79-97
src/server/test_util.rs (2)
11-15: Remove #[allow]; use conditional #[expect] only.Do not use #[allow] per guidelines. Keep the conditional #[expect] for non-test builds and drop the test-specific allow.
Apply this diff:
#[cfg_attr( not(test), expect(dead_code, reason = "Used in builder tests via fixtures") )] -#[cfg_attr(test, allow(dead_code, reason = "Used in builder tests via fixtures"))]
45-50: Remove #[allow]; use conditional #[expect] only.Eliminate the test-only allow; it is forbidden. Use only the non-test #[expect] as you already do.
Apply this diff:
#[cfg_attr( not(test), expect(dead_code, reason = "Only used in configuration tests") )] -#[cfg_attr(test, allow(dead_code, reason = "Only used in configuration tests"))]tests/preamble.rs (1)
176-179: Raise the negative-assertion timeout. LGTM.Use 500 ms for “should not fire” paths to deflake under scheduler jitter. This addresses the prior flakiness note.
Also applies to: 187-190
📜 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 (15)
Cargo.toml(1 hunks)docs/asynchronous-outbound-messaging-design.md(4 hunks)examples/echo.rs(2 hunks)examples/packet_enum.rs(3 hunks)examples/ping_pong.rs(2 hunks)src/server/config/binding.rs(1 hunks)src/server/config/mod.rs(2 hunks)src/server/config/preamble.rs(1 hunks)src/server/config/tests.rs(5 hunks)src/server/connection.rs(4 hunks)src/server/error.rs(1 hunks)src/server/mod.rs(2 hunks)src/server/runtime.rs(9 hunks)src/server/test_util.rs(3 hunks)tests/preamble.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
Cargo.toml
📄 CodeRabbit Inference Engine (AGENTS.md)
Cargo.toml: Use explicit version ranges in Cargo.toml and keep dependencies up-to-date.
Mandate caret requirements for all dependencies. All crate versions specified in Cargo.toml must use SemVer-compatible caret requirements (e.g., some-crate = "1.2.3").
Prohibit unstable version specifiers. The use of wildcard (*) or open-ended inequality (>=) version requirements is strictly forbidden. Tilde requirements (~) should only be used where a dependency must be locked to patch-level updates for a specific, documented reason.
Files:
Cargo.toml
**/*.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:
examples/packet_enum.rsexamples/echo.rssrc/server/error.rsexamples/ping_pong.rssrc/server/test_util.rssrc/server/config/tests.rstests/preamble.rssrc/server/connection.rssrc/server/runtime.rssrc/server/config/binding.rssrc/server/config/preamble.rssrc/server/config/mod.rssrc/server/mod.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:
examples/packet_enum.rsexamples/echo.rssrc/server/error.rsexamples/ping_pong.rssrc/server/test_util.rssrc/server/config/tests.rstests/preamble.rssrc/server/connection.rssrc/server/runtime.rssrc/server/config/binding.rssrc/server/config/preamble.rssrc/server/config/mod.rssrc/server/mod.rs
docs/**/*.md
📄 CodeRabbit Inference Engine (docs/contents.md)
Conventions for writing project documentation as described in the documentation style guide
docs/**/*.md: Use British English based on the Oxford English Dictionary (en-oxendict) for documentation text.
The word "outwith" is acceptable in documentation.
Keep US spelling when used in an API, for examplecolor.
Use the Oxford comma in documentation text.
Treat company names as collective nouns in documentation (e.g., "Lille Industries are expanding").
Write headings in sentence case in documentation.
Use Markdown headings (#,##,###, etc.) in order without skipping levels.
Follow markdownlint recommendations for Markdown files.
Provide code blocks and lists using standard Markdown syntax.
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text.
Use-as the first level bullet and renumber lists when items change.
Prefer inline links using[text](url)or angle brackets around the URL; avoid reference-style links like[foo][bar].
Ensure blank lines before and after bulleted lists and fenced blocks in Markdown.
Ensure tables have a delimiter line below the header row in Markdown.
Expand any uncommon acronym on first use, for example, Continuous Integration (CI).
Wrap paragraphs at 80 columns in documentation.
Wrap code at 120 columns in documentation.
Do not wrap tables in documentation.
Use sequentially numbered footnotes referenced with[^1]and place definitions at the end of the file.
Where it adds clarity, include Mermaid diagrams in documentation.
When embedding figures, useand provide concise alt text describing the content.
Add a brief description before each Mermaid diagram in documentation for screen readers.Document examples showing how to deprecate old message versions gracefully
Write the official documentation for the new features. Create separate guides for "Duplex Messaging & Pushes", "Streaming Responses", and "Message Fragmentation". Each guide must include runna...
Files:
docs/asynchronous-outbound-messaging-design.md
docs/**/*.{md,rs}
📄 CodeRabbit Inference Engine (docs/multi-packet-and-streaming-responses-design.md)
docs/**/*.{md,rs}: The official documentation and examples must exclusively use the declarativeResponsemodel for handler responses.
The async-stream pattern must be documented as the canonical approach for dynamic stream generation.
Files:
docs/asynchronous-outbound-messaging-design.md
**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.md: Documentation must use en-GB-oxendict spelling and grammar. (EXCEPTION: the naming of the LICENSE file, which is to be left unchanged for community consistency.)
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown files must be wrapped at 120 columns.
Tables and headings in Markdown files must not be wrapped.
Use dashes (-) for list bullets in Markdown files.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
docs/asynchronous-outbound-messaging-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
- 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/asynchronous-outbound-messaging-design.md
🧠 Learnings (1)
📚 Learning: 2025-08-08T22:52:32.897Z
Learnt from: leynos
PR: leynos/netsuke#82
File: tests/support/mod.rs:151-155
Timestamp: 2025-08-08T22:52:32.897Z
Learning: In Rust code, when encountering the pattern:
```rust
#[allow(unfulfilled_lint_expectations, reason = "...")]
#[expect(dead_code, reason = "...")]
```
Recommend replacing it with:
```rust
#[cfg_attr(not(any(...)), expect(dead_code, reason = "..."))]
```
This uses conditional compilation to apply the `expect` attribute only when needed, avoiding the forbidden `#[allow]` attribute while properly handling cross-crate test utility usage scenarios.
Applied to files:
src/server/test_util.rs
🧬 Code Graph Analysis (7)
examples/packet_enum.rs (2)
examples/echo.rs (1)
main(12-33)examples/ping_pong.rs (1)
main(139-149)
examples/echo.rs (3)
examples/packet_enum.rs (1)
main(79-97)examples/ping_pong.rs (1)
main(139-149)src/app.rs (2)
new(213-218)new(312-312)
src/server/test_util.rs (1)
src/server/config/tests.rs (2)
WireframeServer(61-61)WireframeServer(136-138)
src/server/config/tests.rs (2)
src/server/test_util.rs (2)
factory(23-25)server_with_preamble(50-55)src/server/config/mod.rs (1)
new(66-69)
src/server/config/preamble.rs (3)
src/server/config/tests.rs (2)
WireframeServer(61-61)WireframeServer(136-138)src/server/test_util.rs (1)
WireframeServer(54-54)tests/preamble.rs (2)
WireframeServer(56-58)WireframeServer(268-272)
src/server/config/mod.rs (3)
tests/preamble.rs (5)
tokio(132-132)tokio(133-133)oneshot(74-74)WireframeServer(56-58)WireframeServer(268-272)src/server/config/tests.rs (2)
WireframeServer(61-61)WireframeServer(136-138)src/server/test_util.rs (2)
WireframeServer(54-54)factory(23-25)
src/server/mod.rs (3)
tests/preamble.rs (4)
tokio(132-132)tokio(133-133)WireframeServer(56-58)WireframeServer(268-272)src/server/config/tests.rs (2)
WireframeServer(61-61)WireframeServer(136-138)src/server/test_util.rs (1)
WireframeServer(54-54)
🪛 LanguageTool
docs/asynchronous-outbound-messaging-design.md
[uncategorized] ~72-~72: Loose punctuation mark.
Context: ...low_priority_push_rx: mpsc::Receiver`: For standard, non-urgent background ...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 GitHub Check: build-test
src/server/config/mod.rs
[warning] 9-9:
Diff in /home/runner/work/wireframe/wireframe/src/server/config/mod.rs
🪛 GitHub Actions: CI
src/server/config/mod.rs
[error] 9-9: Prettier formatting check failed. Diff detected at line 9. Run 'cargo fmt --all' to fix code style issues.
[error] 65-72: Prettier formatting check failed. Diff detected at lines 65-72. Run 'cargo fmt --all' to fix code style issues.
[error] 122-125: Prettier formatting check failed. Diff detected at lines 122-125. Run 'cargo fmt --all' to fix code style issues.
🔍 MCP Research (1 server)
Deepwiki:
-
The
WireframeServerstruct was refactored to use a typestate pattern with a generic parameterS: ServerState, replacing the optional listener field with astatefield that can be eitherUnboundorBound. TheBoundstruct contains the listener, and theServerStatetrait is sealed and implemented for both states. The server'srunandrun_with_shutdownmethods now require theBoundstate and returnResult<(), ServerError>instead ofio::Result. The preamble callback types were renamed toPreambleHandlerandPreambleErrorHandlerwith added lifetime bounds. A new error submodule was added, andServerErrorand config submodulesbindingandpreambleare re-exported. (src/server/mod.rs) -
A new
ServerErrorenum was introduced with variantsBind(io::Error)andAccept(io::Error)to represent errors during binding/configuring the listener and connection acceptance respectively, replacing previous use ofio::Errorfor server errors. (src/server/error.rs) -
The server configuration module was refactored to remove all direct TCP listener binding, preamble callback registration, and accept backoff configuration from the main builder interface. The generic parameter
S: ServerStatewas added, and the constructor now returns anUnboundserver without an optional listener. Methods related to preamble customization, binding, and accept backoff were removed. Two new public submodules,bindingandpreamble, were added to encapsulate the removed functionality. Remaining builder methods focus on worker count and ready-signal channel setup. (src/server/config/mod.rs) -
Binding configuration methods were added for
WireframeServer<F, T, Unbound>andWireframeServer<F, T, Bound>. ForUnbound,local_addrreturnsNone,bindbinds to a socket address creating a standard TCP listener and delegates tobind_listener, which sets non-blocking mode and converts to Tokio TCP listener, returning aBoundserver with the listener wrapped inArc. ForBound,local_addrreturns the bound address, andbindandbind_listenerallow rebinding similarly. All binding methods returnResultwithServerErrorvariants. (src/server/config/binding.rs)
🔇 Additional comments (9)
Cargo.toml (1)
30-30: Add fits guidelines; no issues.The new
thiserrordependency is version-pinned and mirrors existing style.docs/asynchronous-outbound-messaging-design.md (1)
41-48: Documentation edits comply.Numbering and table alignment improve clarity without breaking style rules.
Also applies to: 72-99, 788-795
src/server/error.rs (1)
7-17: Unify server errors behind a semantic enum. LGTM.Derive usage, sources, and messages are correct. This aligns with the guideline to prefer semantic error enums that implement std::error::Error.
src/server/test_util.rs (2)
8-8: Import the typestateBound. LGTM.Match the new typestate API at call sites.
36-36: Return a Bound server. LGTM.The signature reflects the post-bind typestate correctly.
src/server/config/tests.rs (2)
4-4: Terminology update to “handler”. LGTM.Keep docs/tests consistent with the code’s new naming.
134-136: Variable rename to “handler_invoked”. LGTM.Maintain consistent terminology in the method-chaining test and assert initial state.
Also applies to: 151-151
tests/preamble.rs (1)
223-231: Good extraction of reusable helpers and deflake via awaiting the success signal.Keep locks out of
.awaitpaths (you do). Channel holder and callback builders are clear and safe. Await the success oneshot insidewith_running_serverto avoid arbitrary sleeps.Also applies to: 233-256, 258-310
src/server/connection.rs (1)
9-22: Rename callbacks → handlers in the connection path. LGTM.Update import/types and parameter names to match the new handler API.
Also applies to: 56-58
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/server/config/binding.rs (1)
86-96: Reuse existingPhantomDatainstanceReuse
self._preambleinstead of constructing a freshPhantomData. The same issue was raised previously.Also applies to: 170-180
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/server/config/binding.rs(1 hunks)src/server/config/mod.rs(2 hunks)src/server/runtime.rs(10 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/runtime.rssrc/server/config/binding.rssrc/server/config/mod.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/runtime.rssrc/server/config/binding.rssrc/server/config/mod.rs
🔍 MCP Research (1 server)
Deepwiki:
-
The
src/server/config/binding.rsaddition implements binding configuration methods forWireframeServerparameterized by factoryFand preambleT, supporting unbound and bound states with methodslocal_addr,bind, andbind_listenerthat handle TCP listener creation, non-blocking setup, and state transitions, returningResultwithServerErrorfor error handling. (src/server/config/binding.rs) -
The
src/server/config/mod.rsrefactorsWireframeServerconfiguration by removing direct TCP listener binding, preamble callback registration, and accept backoff configuration from the main builder interface, adding a generic parameterS: ServerState, removing related public methods, and introducing new public submodulesbindingandpreambleto encapsulate that functionality. (src/server/config/mod.rs) -
The
src/server/runtime.rsupdatesWireframeServerto include a third generic parameterBound, changesrunandrun_with_shutdownmethods to returnResult<(), ServerError>instead ofio::Result<()>, modifies internal listener extraction to use theBoundstate, removesioimport, and renames preamble callback types to handler types. (src/server/runtime.rs)
🔇 Additional comments (1)
src/server/config/mod.rs (1)
4-9: No issuesDocumentation changes align with guidelines; no further action.
| pub fn bind(self, addr: SocketAddr) -> Result<WireframeServer<F, T, Bound>, ServerError> { | ||
| let std = StdTcpListener::bind(addr).map_err(ServerError::Bind)?; | ||
| self.bind_listener(std) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Factor out duplicated listener-setup code
Both bind/bind_listener paths repeat the set_nonblocking + from_std + struct rebuild sequence. Extract a private helper to trim duplication and keep the invariants in one place.
Also applies to: 143-180
🤖 Prompt for AI Agents
In src/server/config/binding.rs around lines 57 to 60 and also between lines 143
to 180, the code for setting up the listener duplicates the sequence of calling
set_nonblocking, from_std, and rebuilding the struct. To fix this, create a
private helper function that takes the standard listener, performs
set_nonblocking and from_std, and returns the rebuilt struct. Replace the
duplicated code in both bind and bind_listener with calls to this new helper to
centralize the logic and reduce duplication.
| /// Returns a [`ServerError`] if runtime initialisation fails. | ||
| pub async fn run(self) -> Result<(), ServerError> { | ||
| self.run_with_shutdown(async { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use the -ize spelling per style guide
Replace “initialisation” with “initialization” to follow the mandated en-GB-oxford spelling (-ize / -our).
Also applies to: 118-120
🤖 Prompt for AI Agents
In src/server/runtime.rs at lines 79-81 and also at lines 118-120, replace the
word "initialisation" with "initialization" to comply with the en-GB-oxford
spelling style guide that mandates using the -ize suffix instead of -ise.
1 participant
Summary
Bound/UnboundtypestateServerErrorand reorganise config into binding and preamble modulesTesting
make lintmake testhttps://chatgpt.com/codex/tasks/task_e_6893cfc7ed7c83228f453cfa2aec3629
Summary by Sourcery
Enforce compile-time server binding via typestate markers, reorganise configuration and error handling, and update examples and docs to reflect the new ServerError and typestate API.
New Features:
Enhancements:
?and returnResult<(), Box<dyn Error>>for unified error handlingBuild:
Documentation: