Implement response framing with serialization #68
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR implements pluggable response serialization and framing by introducing a Serializer trait with a default BincodeSerializer, converting FrameProcessor to a synchronous interface with a new LengthPrefixedProcessor, extending WireframeApp to store and configure both serializer and frame processor along with a new send_response API, and updating related documentation and tests. Sequence diagram for send_response serialization and framing processsequenceDiagram
participant App as WireframeApp
participant Ser as Serializer
participant FP as FrameProcessor
participant Stream as AsyncWrite
participant Msg as Message
App->>Ser: serialize(Msg)
Ser-->>App: Vec<u8> (serialized bytes)
App->>FP: encode(bytes, framed_buffer)
FP-->>App: framed_buffer (with length prefix)
App->>Stream: write_all(framed_buffer)
App->>Stream: flush()
Stream-->>App: ok/error
Class diagram for new serialization and framing abstractionsclassDiagram
class Serializer {
<<trait>>
+serialize(value: M): Result<Vec<u8>, Error>
+deserialize(bytes: &[u8]): Result<(M, usize), Error>
}
class BincodeSerializer {
+serialize(value: M): Result<Vec<u8>, Error>
+deserialize(bytes: &[u8]): Result<(M, usize), Error>
}
Serializer <|.. BincodeSerializer
class FrameProcessor {
<<trait>>
+decode(src: &mut BytesMut): Result<Option<Frame>, Error>
+encode(frame: &Frame, dst: &mut BytesMut): Result<(), Error>
}
class LengthPrefixedProcessor {
+decode(src: &mut BytesMut): Result<Option<Vec<u8>>, Error>
+encode(frame: &Vec<u8>, dst: &mut BytesMut): Result<(), Error>
}
FrameProcessor <|.. LengthPrefixedProcessor
class WireframeApp {
-routes: HashMap<u32, Service>
-services: Vec<Service>
-middleware: Vec<Box<dyn Middleware>>
-frame_processor: Box<FrameProcessor>
-serializer: Serializer
+frame_processor(processor): Self
+serializer(serializer): WireframeApp<Ser>
+send_response(stream, msg): Result<(), SendError>
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update introduces response serialization and transmission to the framework. It adds a configurable serialization format and frame processor to the application struct, implements a length-prefixed framing protocol, and provides utilities for encoding, framing, and sending handler responses. Documentation and tests are updated to reflect and verify these new capabilities. Changes
Sequence Diagram(s)sequenceDiagram
participant Handler
participant WireframeApp
participant FrameProcessor
participant Stream
Handler->>WireframeApp: yield response (implements Message)
WireframeApp->>WireframeApp: serialize response (SerializationFormat)
WireframeApp->>FrameProcessor: encode serialized bytes (frame)
FrameProcessor-->>WireframeApp: framed bytes
WireframeApp->>Stream: write framed bytes
Stream-->>WireframeApp: flush
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/app.rs:146` </location>
<code_context>
+ S: AsyncWrite + Unpin,
+ M: Message,
+ {
+ let bytes = self.serializer.serialize(msg).map_err(io::Error::other)?;
+ let mut framed = BytesMut::new();
+ self.frame_processor.encode(&bytes, &mut framed).await?;
</code_context>
<issue_to_address>
Wrapping serialization errors with `io::Error::other` loses context
Use `io::Error::new(io::ErrorKind::InvalidData, e)` or implement `From<EncodeError> for io::Error` to retain error details and kind.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
let bytes = self.serializer.serialize(msg).map_err(io::Error::other)?;
=======
let bytes = self.serializer.serialize(msg).map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e))?;
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `src/app.rs:131` </location>
<code_context>
+ /// # Errors
+ ///
+ /// This function currently never fails.
+ pub fn serialization_format(mut self, format: SerializationFormat) -> Result<Self> {
+ self.serializer = format;
+ Ok(self)
</code_context>
<issue_to_address>
`serialization_format` never fails, so `Result` is unnecessary
Consider returning `Self` directly from this setter to simplify the API, as it cannot currently fail.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
/// Choose the serialization format for messages.
///
/// # Errors
///
/// This function currently never fails.
pub fn serialization_format(mut self, format: SerializationFormat) -> Result<Self> {
self.serializer = format;
Ok(self)
=======
/// Choose the serialization format for messages.
pub fn serialization_format(mut self, format: SerializationFormat) -> Self {
self.serializer = format;
self
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
docs/roadmap.md(1 hunks)docs/rust-binary-router-library-design.md(1 hunks)src/app.rs(3 hunks)src/config.rs(1 hunks)src/frame.rs(2 hunks)src/lib.rs(1 hunks)tests/response.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/response.rs (2)
src/app.rs (1)
new(72-72)src/message.rs (1)
from_bytes(49-54)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-test
🔇 Additional comments (3)
docs/roadmap.md (1)
65-67: Road-map checkbox tick looks good – no action neededThe completed item accurately reflects the newly-merged implementation; nothing further to flag.
docs/rust-binary-router-library-design.md (1)
721-724: Wording update is clear and technically correctThe added detail on how the
SerializationFormatandFrameProcessorinteract aligns with the code changes. No further edits required.tests/response.rs (1)
14-19: Double-checksend_response’s generic bounds
send_responseis awaited yet is passed a plain&mut Vec<u8>.
Vec<u8>does not implementtokio::io::AsyncWrite, so the method must either:
- Accept any
&mut impl Extend<u8>/&mut Vec<u8>and be internally synchronous, or- Be generic over
AsyncWritebut usetokio::io::AsyncWriteExt, in which case this test will not compile.Please ensure the first case is true, or update the test to wrap the buffer in
tokio::io::sink()/Cursor<Vec<u8>>.
|
@sourcery-ai resolve |
|
@coderabbitai resolve |
|
@sourcery-ai review |
✅ Actions performedComments resolved and changes approved. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- send_response currently requires &mut self because frame_processor.encode mutates internal state—consider making frame processors stateless or use interior mutability so send_response can take &self and support concurrent responses.
- Since FrameProcessor implementations only operate on memory buffers, you might simplify the API by making encode/decode synchronous functions to remove unnecessary async/await overhead for purely CPU-bound framing logic.
- Mapping serialization errors into a generic io::ErrorKind::InvalidData hides the underlying EncodeError context—consider wrapping or preserving the original error for better diagnostics.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- send_response currently requires &mut self because frame_processor.encode mutates internal state—consider making frame processors stateless or use interior mutability so send_response can take &self and support concurrent responses.
- Since FrameProcessor implementations only operate on memory buffers, you might simplify the API by making encode/decode synchronous functions to remove unnecessary async/await overhead for purely CPU-bound framing logic.
- Mapping serialization errors into a generic io::ErrorKind::InvalidData hides the underlying EncodeError context—consider wrapping or preserving the original error for better diagnostics.
## Individual Comments
### Comment 1
<location> `src/frame.rs:48` </location>
<code_context>
+ let mut len_bytes = [0u8; 4];
+ len_bytes.copy_from_slice(&src[..4]);
+ let len = u32::from_be_bytes(len_bytes);
+ let len_usize = usize::try_from(len).map_err(|_| io::Error::other("frame too large"))?;
+ if src.len() < 4 + len_usize {
+ return Ok(None);
</code_context>
<issue_to_address>
io::Error::other is not a stable API.
For broader compatibility, use io::Error::new(io::ErrorKind::InvalidData, ...) instead.
</issue_to_address>
### Comment 2
<location> `src/config.rs:11` </location>
<code_context>
+use crate::message::Message;
+
+/// Serialization formats supported by `wireframe`.
+#[non_exhaustive]
+#[derive(Clone, Copy, Debug)]
+pub enum SerializationFormat {
</code_context>
<issue_to_address>
Consider deriving PartialEq and Eq for SerializationFormat.
This will allow easier comparisons in user code and tests, which is standard for configuration enums.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
#[derive(Clone, Copy, Debug)]
=======
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `src/app.rs:112` </location>
<code_context>
Ok(self)
}
+ /// Set the frame processor used for encoding and decoding frames.
+ #[must_use]
+ pub fn frame_processor<P>(mut self, processor: P) -> Self
</code_context>
<issue_to_address>
The #[must_use] attribute is placed before the function doc comment; it should be after the doc comment.
Rust convention and the review instruction require that attributes like #[must_use] are placed after the function's doc comment, not before. Please move the attribute below the doc comment for consistency and clarity.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/frame.rs (1)
60-62: Use the explicit big-endian helper for clarity
bytes::BufMut::put_u32is big-endian today, but that’s an implementation detail.put_u32_bemakes the intent unambiguous and future-proof.This was mentioned in a previous review; surfacing again for completeness.
tests/response.rs (1)
22-24: Redundant processor re-creation and extra allocation (same feedback as earlier)Re-using the existing
LengthPrefixedProcessorand decoding directly fromoutavoids one heap allocation and shortens the test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
README.md(1 hunks)src/app.rs(3 hunks)src/config.rs(1 hunks)src/frame.rs(2 hunks)src/lib.rs(1 hunks)tests/response.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/frame.rs (2)
Learnt from: leynos
PR: leynos/wireframe#68
File: src/frame.rs:55-60
Timestamp: 2025-06-17T08:01:20.069Z
Learning: The wireframe project targets Rust 2024 edition and does not need to support older toolchains, so modern Rust APIs like `io::Error::other` can be used without compatibility concerns.
Learnt from: leynos
PR: leynos/mxd#137
File: postgres_setup_unpriv/Cargo.toml:4-4
Timestamp: 2025-06-16T23:32:38.317Z
Learning: Rust Edition 2024 is a valid and finalized edition that was officially released as part of Rust 1.85.0 on February 20, 2025. It can be used in Cargo.toml files with `edition = "2024"`.
🔇 Additional comments (3)
src/lib.rs (1)
8-9: Convenient re-export looks good
SerializationFormatis now accessible directly from the crate root, matching the earlier recommendation. This will spare users an extra module hop.src/config.rs (1)
10-16: Enum is correctly marked#[non_exhaustive]and debuggableForward-compatibility and logging concerns are addressed. 👍
tests/response.rs (1)
50-55:std::io::Error::otherbumps MSRV – useError::newinstead
std::io::Error::otheris only available from Rust 1.71. If your crate’s MSRV is lower (the default Wireframe CI currently asserts 1.65), this will fail to compile. Construct the error viastd::io::Error::newto stay MSRV-agnostic.- std::task::Poll::Ready(Err(std::io::Error::other("fail"))) + std::task::Poll::Ready(Err(std::io::Error::new( + std::io::ErrorKind::Other, + "fail", + )))⛔ Skipped due to learnings
Learnt from: leynos PR: leynos/wireframe#68 File: src/frame.rs:55-60 Timestamp: 2025-06-17T08:01:20.069Z Learning: The wireframe project targets Rust 2024 edition and does not need to support older toolchains, so modern Rust APIs like `io::Error::other` can be used without compatibility concerns.
Comment on lines
81
to
88
| ## Response Serialization and Framing | ||
|
|
||
| Handlers can return types implementing the `Responder` trait. These values are | ||
| encoded using the application's configured `SerializationFormat` and written | ||
| back through the `FrameProcessor`【F:docs/rust-binary-router-library-design.md†L718-L724】. | ||
| The included `LengthPrefixedProcessor` illustrates a simple framing strategy | ||
| based on a big‑endian length prefix【F:docs/rust-binary-router-library-design.md†L1076-L1117】. | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Tiny doc nit: link to the concrete types for quick navigation
Readers will likely want to jump straight to LengthPrefixedProcessor and SerializationFormat definitions. Adding inline markdown links (e.g. [`LengthPrefixedProcessor`]) would improve discoverability without cluttering the text.
🤖 Prompt for AI Agents
In README.md around lines 81 to 88, the references to `LengthPrefixedProcessor`
and `SerializationFormat` are plain text and not clickable links. Update these
mentions to use inline markdown links by enclosing the type names in backticks
and square brackets, like [`LengthPrefixedProcessor`] and
[`SerializationFormat`], to enable quick navigation to their definitions.
Comment on lines
+48
to
+51
| let len_usize = usize::try_from(len).map_err(|_| io::Error::other("frame too large"))?; | ||
| if src.len() < 4 + len_usize { | ||
| return Ok(None); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
No upper bound on frame length – potential OOM risk
usize::try_from(len) guards against 32-bit overflow but still allows a peer to request allocation of ~4 GiB. A malicious client could exhaust memory or trigger DoS.
Consider enforcing a sane application-level limit (e.g. MAX_FRAME = 8 * 1024 * 1024) before allocating:
let len_usize = usize::try_from(len).map_err(|_| io::Error::other("frame too large"))?;
+const MAX_FRAME: usize = 8 * 1024 * 1024; // 8 MiB – tune as appropriate
+if len_usize > MAX_FRAME {
+ return Err(io::Error::new(io::ErrorKind::InvalidData, "frame length too large"));
+}🤖 Prompt for AI Agents
In src/frame.rs around lines 48 to 51, the code converts the frame length to
usize without an upper bound, risking excessive memory allocation. Introduce a
constant MAX_FRAME (e.g., 8 * 1024 * 1024) and check if len_usize exceeds this
limit before proceeding. If it does, return an appropriate error to prevent
potential OOM or DoS attacks by rejecting frames that are too large.
Comment on lines
139
to
147
| let bytes = self | ||
| .serializer | ||
| .serialize(msg) | ||
| .map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e))?; | ||
| let mut framed = BytesMut::with_capacity(4 + bytes.len()); | ||
| self.frame_processor.encode(&bytes, &mut framed).await?; | ||
| stream.write_all(&framed).await?; | ||
| stream.flush().await | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider bubbling the original EncodeError instead of wrapping
io::Error::new(io::ErrorKind::InvalidData, e) preserves the source error via e.into(), but callers must still down-cast to inspect it. A bespoke error enum (AppError { Io(io::Error), Encode(EncodeError) }) or thiserror-based approach would let upstream code match on the precise failure without type erasure.
Not critical for functionality, yet improves diagnosability when serialization fails.
🤖 Prompt for AI Agents
In src/app.rs around lines 139 to 147, instead of wrapping the EncodeError
inside an io::Error, refactor the error handling to define a custom error enum
(e.g., AppError) that includes variants for both io::Error and EncodeError.
Modify the function signature to return this custom error type and propagate the
original EncodeError directly without type erasure. This allows upstream code to
match on specific error variants for better diagnosability.
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Potential for denial-of-service if len_usize is very large. (link)
General comments:
- Refactor FrameProcessor to use synchronous encode/decode methods (or offer a sync API) since your implementations perform no real async work and an async trait adds unneeded state-machine overhead.
- Change send_response to take &self instead of &mut self by making the frame processor stateless or internally mutable, so you don’t need a mutable borrow just to send a response.
- Consider replacing the SerializationFormat enum with a trait-based serializer (or trait object) so users can plug in custom formats (JSON, CBOR, etc.) without modifying the core enum.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Refactor FrameProcessor to use synchronous encode/decode methods (or offer a sync API) since your implementations perform no real async work and an async trait adds unneeded state-machine overhead.
- Change send_response to take &self instead of &mut self by making the frame processor stateless or internally mutable, so you don’t need a mutable borrow just to send a response.
- Consider replacing the SerializationFormat enum with a trait-based serializer (or trait object) so users can plug in custom formats (JSON, CBOR, etc.) without modifying the core enum.
## Individual Comments
### Comment 1
<location> `src/frame.rs:48` </location>
<code_context>
+ let mut len_bytes = [0u8; 4];
+ len_bytes.copy_from_slice(&src[..4]);
+ let len = u32::from_be_bytes(len_bytes);
+ let len_usize = usize::try_from(len).map_err(|_| io::Error::other("frame too large"))?;
+ if src.len() < 4 + len_usize {
+ return Ok(None);
</code_context>
<issue_to_address>
io::Error::other is not a stable API.
Use io::Error::new(io::ErrorKind::InvalidData, ...) instead for stable Rust compatibility.
</issue_to_address>
### Comment 2
<location> `src/frame.rs:49` </location>
<code_context>
+ len_bytes.copy_from_slice(&src[..4]);
+ let len = u32::from_be_bytes(len_bytes);
+ let len_usize = usize::try_from(len).map_err(|_| io::Error::other("frame too large"))?;
+ if src.len() < 4 + len_usize {
+ return Ok(None);
+ }
</code_context>
<issue_to_address>
Potential for denial-of-service if len_usize is very large.
A large length prefix could trigger excessive memory allocation. Add a maximum frame size to prevent memory exhaustion.
</issue_to_address>
### Comment 3
<location> `src/app.rs:113` </location>
<code_context>
}
+ /// Set the frame processor used for encoding and decoding frames.
+ #[must_use]
+ pub fn frame_processor<P>(mut self, processor: P) -> Self
+ where
</code_context>
<issue_to_address>
Function attribute #[must_use] is placed before the doc comment; attributes must come after the doc comment.
Please move the #[must_use] attribute so that it appears after the function's doc comment, as per the review instruction.
</issue_to_address>
### Comment 4
<location> `src/app.rs:113` </location>
<code_context>
}
+ /// Set the frame processor used for encoding and decoding frames.
+ #[must_use]
+ pub fn frame_processor<P>(mut self, processor: P) -> Self
+ where
</code_context>
<issue_to_address>
Function attribute #[must_use] is placed before the doc comment; attributes must come after the doc comment.
Please move the #[must_use] attribute so that it appears after the function's doc comment, as per the review instruction.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/app.rs:143` </location>
<code_context>
+ .serializer
+ .serialize(msg)
+ .map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e))?;
+ let mut framed = BytesMut::with_capacity(4 + bytes.len());
+ self.frame_processor.encode(&bytes, &mut framed).await?;
+ stream.write_all(&framed).await?;
</code_context>
<issue_to_address>
Potential inefficiency in allocating BytesMut for each response.
Repeatedly allocating new buffers can impact performance. Buffer reuse or pooling may help reduce overhead in high-throughput scenarios.
</issue_to_address>
### Comment 2
<location> `src/frame.rs:48` </location>
<code_context>
+ let mut len_bytes = [0u8; 4];
+ len_bytes.copy_from_slice(&src[..4]);
+ let len = u32::from_be_bytes(len_bytes);
+ let len_usize = usize::try_from(len).map_err(|_| io::Error::other("frame too large"))?;
+ if src.len() < 4 + len_usize {
+ return Ok(None);
</code_context>
<issue_to_address>
io::Error::other is not a stable API.
Please use io::Error::new with a stable ErrorKind, such as InvalidData, instead of io::Error::other.
</issue_to_address>
### Comment 3
<location> `src/app.rs:112` </location>
<code_context>
Ok(self)
}
+ /// Set the frame processor used for encoding and decoding frames.
+ #[must_use]
+ pub fn frame_processor<P>(mut self, processor: P) -> Self
</code_context>
<issue_to_address>
The #[must_use] attribute is placed before the function doc comment; it should be after.
Rust convention and the review instruction require that function attributes (like #[must_use]) are placed after the function's doc comment, not before. Please move the attribute below the doc comment for consistency.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai resolve |
|
@sourcery-ai review |
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/frame.rs:41` </location>
<code_context>
+ fn encode(&self, frame: &Self::Frame, dst: &mut BytesMut) -> Result<(), Self::Error>;
+}
+
+/// Simple length-prefixed framing using big-endian u32 lengths.
+pub struct LengthPrefixedProcessor;
+
</code_context>
<issue_to_address>
Missing module-level item comment (`//!`) for the new module.
Please add a module-level item comment at the top of this file using the `//!` syntax to describe the purpose of the module, as required by the review instructions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- You have duplicate
WireframeApp::newimplementations in both the genericimpl<S>and theimpl WireframeApp<BincodeSerializer>blocks—consolidate them to avoid duplication and potential conflicts. - The old
config::SerializationFormatmodule is no longer used—consider deprecating or removing it now that the.serializer()API replaces it. - Several code snippets in the design docs and README have mismatched backticks and extra fences—clean up the Markdown so the examples render correctly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- You have duplicate `WireframeApp::new` implementations in both the generic `impl<S>` and the `impl WireframeApp<BincodeSerializer>` blocks—consolidate them to avoid duplication and potential conflicts.
- The old `config::SerializationFormat` module is no longer used—consider deprecating or removing it now that the `.serializer()` API replaces it.
- Several code snippets in the design docs and README have mismatched backticks and extra fences—clean up the Markdown so the examples render correctly.
## Individual Comments
### Comment 1
<location> `src/frame.rs:41` </location>
<code_context>
+ fn encode(&self, frame: &Self::Frame, dst: &mut BytesMut) -> Result<(), Self::Error>;
+}
+
+/// Simple length-prefixed framing using big-endian u32 lengths.
+pub struct LengthPrefixedProcessor;
+
</code_context>
<issue_to_address>
Missing module-level item comment (`//!`) at the top of the file.
Please add a `//!` comment at the top of `src/frame.rs` to describe the module, as required by the review instructions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| fn encode(&self, frame: &Self::Frame, dst: &mut BytesMut) -> Result<(), Self::Error>; | ||
| } | ||
|
|
||
| /// Simple length-prefixed framing using big-endian u32 lengths. |
There was a problem hiding this comment.
issue (review_instructions): Missing module-level item comment (//!) at the top of the file.
Please add a //! comment at the top of src/frame.rs to describe the module, as required by the review instructions.
Review instructions:
Path patterns: **/*.rs
Instructions:
All modules MUST have a containing item comment (//!)
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/frame.rs:55` </location>
<code_context>
+ let mut len_bytes = [0u8; 4];
+ len_bytes.copy_from_slice(&src[..4]);
+ let len = u32::from_be_bytes(len_bytes);
+ let len_usize = usize::try_from(len).map_err(|_| io::Error::other("frame too large"))?;
+ if src.len() < 4 + len_usize {
+ return Ok(None);
</code_context>
<issue_to_address>
io::Error::other is not a stable API.
Use io::Error::new(io::ErrorKind::InvalidData, ...) for stable Rust compatibility.
</issue_to_address>
### Comment 2
<location> `src/frame.rs:41` </location>
<code_context>
+ fn encode(&self, frame: &Self::Frame, dst: &mut BytesMut) -> Result<(), Self::Error>;
+}
+
+/// Simple length-prefixed framing using big-endian u32 lengths.
+pub struct LengthPrefixedProcessor;
+
</code_context>
<issue_to_address>
Missing module-level item comment (`//!`) for the new module.
Please add a module-level item comment at the top of this file using the `//!` syntax to describe the purpose of the module, as required by the review instructions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| let mut len_bytes = [0u8; 4]; | ||
| len_bytes.copy_from_slice(&src[..4]); | ||
| let len = u32::from_be_bytes(len_bytes); | ||
| let len_usize = usize::try_from(len).map_err(|_| io::Error::other("frame too large"))?; |
There was a problem hiding this comment.
issue: io::Error::other is not a stable API.
Use io::Error::new(io::ErrorKind::InvalidData, ...) for stable Rust compatibility.
| fn encode(&self, frame: &Self::Frame, dst: &mut BytesMut) -> Result<(), Self::Error>; | ||
| } | ||
|
|
||
| /// Simple length-prefixed framing using big-endian u32 lengths. |
There was a problem hiding this comment.
issue (review_instructions): Missing module-level item comment (//!) for the new module.
Please add a module-level item comment at the top of this file using the //! syntax to describe the purpose of the module, as required by the review instructions.
Review instructions:
Path patterns: **/*.rs
Instructions:
All modules MUST have a containing item comment (//!)
|
@coderabbitai generate docstrings |
✅ Actions performedDocstrings generation started. |
|
Caution An unexpected error occurred while opening a pull request: Not Found - https://docs.github.com/rest/git/refs#get-a-reference |
This was referenced Jun 18, 2025
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
SerializationFormatconfig moduleLengthPrefixedProcessorand response sending helpersTesting
cargo clippy -- -D warningsRUSTFLAGS="-D warnings" cargo testmarkdownlint docs/preamble-validator.md docs/roadmap.md docs/rust-binary-router-library-design.md docs/rust-testing-with-rstest-fixtures.md README.mdnixie docs/preamble-validator.md docs/roadmap.md docs/rust-binary-router-library-design.md docs/rust-testing-with-rstest-fixtures.md README.mdhttps://chatgpt.com/codex/tasks/task_e_6850a3439ea8832287c8431cdfbb87f9
Summary by Sourcery
Implement configurable response serialization and framing by introducing a Serializer trait and FrameProcessor abstraction with default BincodeSerializer and LengthPrefixedProcessor, extend WireframeApp with serializer and frame_processor fields, and add an async send_response method for encoding and transmitting handler outputs; update examples and documentation to reflect these changes and add tests covering encoding, framing, and error scenarios.
New Features:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Documentation
Tests