Clarify preamble handler docs and server errors #300
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR refines how preamble handlers are documented and wired by preserving existing PhantomData in binding builders, improving error logging formatting for failed preamble handlers, and marking the server error enum as non-exhaustive. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 17 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
Summary by CodeRabbit
WalkthroughPreserve the existing Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant PreambleHandler
Client->>Server: Connect and send preamble
Server->>PreambleHandler: Decode preamble and invoke callback
alt Preamble decode success
PreambleHandler-->>Server: Success result
Server-->>Client: Proceed with connection
else Preamble decode error
PreambleHandler-->>Server: Error (returned)
Server-->>Server: Log error with display and debug fields
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 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/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/server/connection.rs:67` </location>
<code_context>
&& let Err(e) = handler(&preamble, &mut stream).await
{
- tracing::error!(error = ?e, ?peer_addr, "preamble handler error");
+ tracing::error!(error = %e, ?peer_addr, "preamble handler error");
}
let stream = RewindStream::new(leftover, stream);
</code_context>
<issue_to_address>
Switching from ?e to %e in tracing may affect error formatting.
Switching to %e uses Display formatting for errors. Please verify that this provides the necessary detail and is appropriate for your error type.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
tracing::error!(error = %e, ?peer_addr, "preamble handler error");
=======
tracing::error!(
error_display = %e,
error_debug = ?e,
?peer_addr,
"preamble handler error"
);
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/server/config/binding.rs(2 hunks)src/server/config/preamble.rs(1 hunks)src/server/connection.rs(1 hunks)src/server/error.rs(1 hunks)src/server/mod.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/preamble.rssrc/server/error.rssrc/server/connection.rssrc/server/mod.rssrc/server/config/binding.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/preamble.rssrc/server/error.rssrc/server/connection.rssrc/server/mod.rssrc/server/config/binding.rs
🧬 Code Graph Analysis (1)
src/server/connection.rs (1)
src/extractor.rs (1)
peer_addr(362-362)
🔍 MCP Research (1 server)
Deepwiki:
-
The
_preamblefield inWireframeServeris aPhantomData<T>marker used to associate the server with a specific preamble typeT. This field does not store data but enforces type safety related to preamble handling. The change in the PR to reuse the existing_preamblefield instead of resetting it to a newPhantomDatainstance preserves the original type marker, aligning with the design that this field is a type-level marker rather than a data container. (Wiki page "WireframeServer") -
The
bindmethod inWireframeServeris responsible for binding the server to a socket address and creating aTcpListener, but it does not directly handle the_preamblefield or its callbacks. Preamble handling occurs later during connection processing in theprocess_streamfunction, where the preamble is decoded and success or failure callbacks are invoked. The_preamblefield and related callbacks are part of the server's state carried through the builder chain. (Wiki page "WireframeServer") -
The
process_streamfunction inWireframeServer's worker tasks reads the preamble from the TCP stream usingread_preamble::<_, T>(&mut stream). On successful decode, it calls the success callback with the preamble and stream; on failure, it calls the failure callback and drops the connection. Leftover bytes from preamble decoding are wrapped in aRewindStreamfor further processing by the application. (Wiki page "WireframeServer")
🔇 Additional comments (5)
src/server/config/preamble.rs (1)
58-58: Tighten cross-reference to the ready-to-use alias.Keep this link to
PreambleHandler; it improves discoverability for users of the builder API.src/server/connection.rs (1)
67-67: Use Display (%) for error logging.Prefer
%here for human-readable logs. This aligns the log output with user-facing error messages.src/server/mod.rs (1)
59-59: Clarify the alias documentation.Keep the updated doc: “[
PreambleSuccessHandler] wrapped inArc.” It is precise and reduces ambiguity about the trait object.src/server/config/binding.rs (2)
94-95: Reuse the existing_preambletypestate marker.Retain
self._preambleinstead of instantiating a newPhantomData. This preserves the type marker and complies with the typestate design.
179-180: Preserve_preambleduring rebinds.Keep passing through
self._preambleon rebind. This correctly carries the preamble type across bindings.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR makes targeted improvements to the Wireframe server's API design and documentation quality. The changes focus on three main areas: enhanced type documentation, defensive API design, and minor code optimizations.
The preamble handler documentation has been refined to provide clearer guidance for developers. The PreambleHandler type alias documentation now succinctly explains that it's simply a PreambleSuccessHandler wrapped in Arc, eliminating unnecessary verbosity. Additionally, the preamble configuration documentation now includes a helpful reference to the PreambleHandler alias, making the API more discoverable for users who might prefer the ready-to-use type over implementing the trait directly.
A key architectural change involves marking the ServerError enum as #[non_exhaustive]. This is a forward-thinking API design decision that allows future error variants to be added without breaking downstream code that pattern matches on the enum. The change is properly documented in the changelog with clear guidance for users.
The PR also includes a micro-optimization in the server binding logic where existing PhantomData instances are reused instead of creating new ones during server rebinding operations. Since the preamble type remains unchanged during rebinding, reusing the existing phantom data marker is both semantically correct and slightly more efficient.
These changes integrate well with the existing Wireframe architecture, maintaining the library's focus on type safety while improving maintainability and user experience.
Important Files Changed
Files Changed
| Filename | Score | Overview |
|---|---|---|
src/server/error.rs |
5/5 | Added #[non_exhaustive] attribute to ServerError enum for future extensibility |
src/server/config/preamble.rs |
5/5 | Enhanced documentation with PreambleHandler reference and reused existing PhantomData |
CHANGELOG.md |
5/5 | Documented the ServerError non-exhaustive change with user guidance |
src/server/mod.rs |
5/5 | Simplified PreambleHandler documentation for better clarity |
src/server/config/binding.rs |
5/5 | Reused existing PhantomData instances during server rebinding operations |
Confidence score: 5/5
- This PR is safe to merge with minimal risk as it contains well-designed improvements and defensive coding practices
- Score reflects thoughtful API design changes, clear documentation improvements, and proper handling of the breaking change through changelog documentation
- No files require special attention as all changes are straightforward and follow Rust best practices
5 files reviewed, no comments
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/server/connection.rs (1)
67-72: Log both Display and Debug forms of the error.Keep this. It provides concise human-readable context and the detailed debug form for diagnostics. This aligns with the earlier suggestion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)src/server/connection.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.md
⚙️ CodeRabbit Configuration File
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- 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:
CHANGELOG.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.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Files:
src/server/connection.rs
🧬 Code Graph Analysis (1)
src/server/connection.rs (1)
src/extractor.rs (1)
peer_addr(362-362)
🔍 Additional research (Context7)
Summary of relevant context discovered for reviewing PR #300:
-
The repo's docs/wiki include extensive, up-to-date design and implementation details for WireframeServer, ConnectionActor, preamble handling, ProtocolHooks/PreambleHandler, push queues, error handling, and observability — all areas touched by this PR. These pages confirm the semantics of preamble callbacks, how RewindStream/leftover bytes are handled, connection logging/tracing, and ServerError usage patterns (affects non-exhaustive change).,
-
The library identifier resolution succeeded for the repository (context7 listing output). Useful for locating code/docs in Context7 if deeper doc fetches are needed.
-
Items directly relevant to the PR:
- Preamble: docs show on_preamble_decode_success is the documented hook and that handlers commonly use a PreambleHandler / ProtocolHooks to register PushHandle etc.; updating docs to reference crate::server::PreambleHandler aligns with existing patterns.
- Bind/listener logic & PhantomData: server bind/rebind behavior and WireframeServer factory/worker patterns are documented (preserving existing _preamble vs replacing is consistent with keeping connection-specific state).
- Connection logging: connection and preamble error logging is instrumented via tracing; changing log field formats to include both display and debug forms fits existing tracing usage.
- ServerError design: docs describe error classification (Io vs Protocol); marking ServerError #[non_exhaustive] is compatible with the stated goal of allowing future variants.
- Tests/CI: repo uses make lint/test and CI workflows; PR claim that make lint / make test passed is consistent with repo policies.
If you want, I can:
- Fetch the exact source diffs or files referenced (binding.rs, preamble.rs, connection.rs, error.rs, mod.rs, CHANGELOG.md) for line-by-line review.
- Run a focused search for tests referencing ServerError matches or preamble handler docs to confirm downstream impacts.
Tool calls used:,,
1 participant
Summary
ServerErroras non-exhaustivePhantomDatawhen rebindingTesting
make lintmake testhttps://chatgpt.com/codex/tasks/task_e_689796199c5883228eb17280729d1396
Summary by Sourcery
Clarify preamble handler documentation, mark the ServerError enum as non-exhaustive, reuse existing PhantomData in server bindings, and adjust preamble handler error logging format.
Enhancements:
%for error displayDocumentation: