Conversation
Reviewer's GuideRefactors the Class diagram for modularized app submodulesclassDiagram
class WireframeApp {
+routes: HashMap<u32, Handler<E>>
+services: Vec<Handler<E>>
+middleware: Vec<Box<dyn Transform<HandlerService<E>, Output = HandlerService<E>>>>
+frame_processor: BoxedFrameProcessor
+serializer: S
+app_data: HashMap<TypeId, Arc<dyn Any + Send + Sync>>
+on_connect: Option<Arc<ConnectionSetup<C>>>
+on_disconnect: Option<Arc<ConnectionTeardown<C>>>
+protocol: Option<Arc<dyn WireframeProtocol<Frame = Vec<u8>, ProtocolError = ()>>>
+push_dlq: Option<mpsc::Sender<Vec<u8>>>
+new()
+route(id, handler)
+service(handler)
+app_data(state)
+wrap(mw)
+on_connection_setup(f)
+on_connection_teardown(f)
+with_protocol(protocol)
+with_push_dlq(dlq)
+protocol()
+protocol_hooks()
+frame_processor(processor)
+serializer(serializer)
+send_response(stream, msg)
+handle_connection(stream)
}
class HandlerService {
+new(id, handler)
+call(request)
}
class Envelope {
+id: u32
+correlation_id: Option<u64>
+payload: Vec<u8>
+new(id, correlation_id, payload)
}
class PacketParts {
+id: u32
+correlation_id: Option<u64>
+payload: Vec<u8>
+new(id, correlation_id, payload)
+inherit_correlation(source)
}
class Packet {
<<interface>>
+id()
+correlation_id()
+into_parts()
+from_parts(parts)
}
class Middleware {
<<interface>>
}
class ConnectionSetup {
<<type alias>>
}
class ConnectionTeardown {
<<type alias>>
}
class WireframeError {
+DuplicateRoute(u32)
}
class SendError {
+Serialize(Box<dyn std::error::Error + Send + Sync>)
+Io(io::Error)
}
WireframeApp --> "1..*" HandlerService
WireframeApp --> "1..*" Middleware
WireframeApp --> "1" Envelope
Envelope ..|> Packet
PacketParts --> Envelope
Envelope --> PacketParts
WireframeApp --> ConnectionSetup
WireframeApp --> ConnectionTeardown
WireframeApp --> WireframeError
WireframeApp --> SendError
HandlerService --> Handler
Middleware ..|> Transform
WireframeApp --> Serializer
WireframeApp --> FrameProcessor
WireframeApp --> WireframeProtocol
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Note Reviews pausedUse the following commands to manage reviews:
Summary by CodeRabbit
WalkthroughRefactor the monolithic Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant IO as AsyncStream
participant Codec as LengthDelimitedCodec
participant App as WireframeApp<S,C,E>
participant Ser as Serializer
participant MW as MiddlewareChain
participant H as Handler
Client->>IO: Send framed bytes
IO->>Codec: Read frame
Codec-->>App: Deliver frame bytes
App->>Ser: Parse/deserialize -> Envelope
Ser-->>App: Envelope{id,corr,payload}
App->>MW: Route id -> handler chain
MW->>H: Invoke handler
H-->>MW: Optional response
MW-->>App: Return response
App->>Ser: Serialize response Envelope
Ser-->>App: Bytes
App->>Codec: Frame response
Codec-->>IO: Write bytes
IO-->>Client: Deliver response
note right of App: On errors -> metrics / logs / DLQ
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
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 (
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
wireframe_testing/src/helpers.rs (2)
18-22: Fix invalid lint suppression; use #[expect] with a reason.
#[allow(..., reason = "...")]is invalid and violates house rules. Use#[expect(..., reason = "...")]and scope it precisely.Apply:
-#[allow( - unused_braces, - reason = "Clippy is wrong here; this is not a redundant block" -)] +#[expect( + unused_braces, + reason = "This block scopes the rstest fixture; the lint misfires here" +)]
431-444: Correct the “Panics” section; this function does not panic.
run_with_duplex_serverawaitshandle_connectionand returns(). Remove or rephrase the panics section.Apply:
-/// # Panics -/// -/// Panics if `handle_connection` fails. +/// # Behaviour +/// +/// This helper drives the connection lifecycle and does not propagate errors.docs/wireframe-client-design.md (2)
26-29: Remove remainingFrameProcessorreferences to match codec model.Align wording with the codec approach; do not mention
FrameProcessorhere.Apply:
-- Encode outgoing messages using the selected `Serializer` and `FrameProcessor`. -- Decode incoming frames into typed responses. +- Encode outgoing messages using the selected `Serializer`; framing is provided + by the length‑delimited codec. +- Decode incoming frames via the length‑delimited codec into typed responses.
1-96: Standardise on en‑GB spelling.Replace “serialization/deserialize” with “serialisation/deserialise” across the document to meet style.
README.md (2)
131-139: ReplaceFrameProcessor/LengthPrefixedProcessornarrative with codec wording.This section contradicts the new codec-based architecture. Describe framing via the length‑delimited codec, not a custom frame processor.
Apply:
-Handlers can return types implementing the `Responder` trait. These values are -encoded using the application's configured serializer and written back through -the `FrameProcessor`【F:docs/rust-binary-router-library-design.md†L724-L730】. - -The included `LengthPrefixedProcessor` illustrates a simple framing strategy -that prefixes each frame with its length. The format is configurable (prefix -size and endianness) and defaults to a 4‑byte big‑endian length -prefix【F:docs/rust-binary-router-library-design.md†L1082-L1123】. +Handlers can return types implementing the `Responder` trait. These values are +encoded using the application's configured serializer and sent through the +length‑delimited codec (via `tokio‑util`), which prefixes each frame with its +length before I/O.
10-23: Standardise on en‑GB spelling in docs.Use “serialisation/deserialise/licence” consistently across the README.
Also applies to: 84-89, 129-139
tests/metadata.rs (1)
52-52: Relax atomic ordering in testsUse
Ordering::Relaxedfor counters; there’s no synchronisation dependency beyond counting.Apply:
- self.0.fetch_add(1, Ordering::SeqCst); + self.0.fetch_add(1, Ordering::Relaxed); @@ - assert_eq!(counter.load(Ordering::SeqCst), 1); + assert_eq!(counter.load(Ordering::Relaxed), 1); @@ - self.1.fetch_add(1, Ordering::SeqCst); + self.1.fetch_add(1, Ordering::Relaxed); @@ - self.0.fetch_add(1, Ordering::SeqCst); + self.0.fetch_add(1, Ordering::Relaxed); @@ - assert_eq!(deser_calls.load(Ordering::SeqCst), 1); + assert_eq!(deser_calls.load(Ordering::Relaxed), 1);Also applies to: 69-69, 86-86, 96-96, 115-115
examples/ping_pong.rs (1)
45-47: Replace#[allow]with narrowly scoped#[expect]Do not silence lints broadly. Use
#[expect(..., reason = "...")].Apply:
-#[allow(clippy::unused_async)] +#[expect(clippy::unused_async, reason = "Example keeps the handler async for symmetry with real handlers")] async fn ping_handler() {}
♻️ Duplicate comments (15)
src/app/error.rs (3)
1-6: Module docs present and scoped imports are correct.Keep
std::ioand the module‑level//!summary.
7-14: Derives and non‑exhaustive error enum look good.
WireframeErrorderivesPartialEq, Eqand is future‑proofed. Good.
16-26: Error plumbing forSendErroris correct.
thiserrorwith#[from] io::Errorand#[source]on serialisation preserves causality.src/app/envelope.rs (1)
1-6: Good: strengthen module docs with rationale and linkage.The docs now state the purpose (decouple serialisation and routing) and link to
WireframeApp.tests/wireframe_protocol.rs (1)
19-26: Adopt a local alias to reduce generic noise — LGTM.The imports and the TestApp alias align with the new modular exports and resolve the prior readability concern on this file.
tests/middleware_order.rs (1)
15-16: Add TestApp alias to remove verbosity — LGTM.This mirrors the repo-wide pattern and addresses the earlier nit.
tests/common/mod.rs (1)
21-24: Replace forbidden #[allow] with narrowly scoped #[expect] and annotate.Comply with guidelines by switching to
#[expect(..., reason = "...")]and add a FIXME note.Apply this diff within the selected lines:
-#[allow( - unused_braces, - reason = "rustc false positive for single line rstest fixtures" -)] +// FIXME: Track upstream false positive; remove when fixed. (link issue) +#[expect( + unused_braces, + reason = "rustc false positive for single-line rstest fixtures" +)]tests/routes.rs (1)
21-22: Introduce TestApp alias — LGTM.The alias improves legibility and standardises test setup.
src/app/mod.rs (1)
1-10: Comprehensive module documentation addresses past feedback.The enhanced documentation clearly explains the module's purpose as the unified import surface and lists all re-exported items with their categories. This directly addresses the previous review comment about improving discoverability.
src/app/connection.rs (5)
45-65: Fix framing inconsistency in send_response.The current implementation creates a new
LengthDelimitedCodecinstance and manually encodes to aBytesMut, then writes directly to the raw stream. This approach still suffers from the framing divergence issue flagged in previous reviews - mixing manual framing with theFramed<_, LengthDelimitedCodec>used elsewhere can corrupt stream state.Implement a unified framing approach:
- pub async fn send_response<W, M>( - &self, - stream: &mut W, - msg: &M, - ) -> std::result::Result<(), SendError> - where - W: AsyncWrite + Unpin, - M: Message, - { - let bytes = self - .serializer - .serialize(msg) - .map_err(SendError::Serialize)?; - let mut codec = LengthDelimitedCodec::builder().new_codec(); - let mut framed = BytesMut::new(); - codec - .encode(bytes.into(), &mut framed) - .map_err(|e| SendError::Io(io::Error::new(io::ErrorKind::InvalidData, e)))?; - stream.write_all(&framed).await.map_err(SendError::Io)?; - stream.flush().await.map_err(SendError::Io) - } + pub async fn send_response_framed<W, M>( + &self, + framed: &mut Framed<W, LengthDelimitedCodec>, + msg: &M, + ) -> std::result::Result<(), SendError> + where + W: AsyncWrite + Unpin, + M: Message, + { + let bytes = self.serializer.serialize(msg).map_err(SendError::Serialize)?; + framed.send(bytes.into()).await.map_err(SendError::Io) + }Replace calls to send_response with send_response_framed, passing the existing Framed instance to maintain consistent framing throughout the connection lifecycle.
90-94: Correct stale documentation.The docstring incorrectly describes this as a placeholder that immediately closes connections. The implementation actually performs full connection lifecycle management.
- /// Handle an accepted connection. - /// - /// This placeholder immediately closes the connection after the - /// preamble phase. A warning is logged so tests and callers are - /// aware of the current limitation. + /// Handle an accepted connection end-to-end. + /// + /// Runs optional connection setup to produce per-connection state, + /// initialises (and caches) route chains, processes the framed stream + /// with per-frame timeouts, and finally runs optional teardown.
146-150: Handle read timeouts explicitly.The current
while let Ok(Some(frame)) = timeout(..., framed.next()).awaitpattern silently exits on timeout, potentially leaving connections in an unclear state.- while let Ok(Some(frame)) = timeout(timeout_dur, framed.next()).await { - let buf = frame?; - self.handle_frame(&mut framed, buf.as_ref(), &mut deser_failures, routes) - .await?; - } + loop { + match timeout(timeout_dur, framed.next()).await { + Ok(Some(Ok(buf))) => { + self.handle_frame(&mut framed, buf.as_ref(), &mut deser_failures, routes).await?; + } + Ok(Some(Err(e))) => return Err(e), + Ok(None) => break, // EOF + Err(_) => { + tracing::debug!("read timeout elapsed; continuing to wait for next frame"); + continue; + } + } + }
76-88: Narrow error type to avoid heap allocation.Returning
Box<dyn Error + Send + Sync>for deserialisation errors obscures error classification and forces heap allocation. Define a concrete error enum to distinguish parse vs deserialise failures.+#[derive(Debug)] +enum ParseError<E> { + Parse(E), + Deserialize(String), // or a more specific deserialisation error type +} + - ) -> std::result::Result<(Envelope, usize), EnvelopeDecodeError<S::Error>> { + ) -> std::result::Result<(Envelope, usize), ParseError<S::Error>> { self.serializer .parse(frame) - .map_err(EnvelopeDecodeError::Parse) + .map_err(ParseError::Parse) .or_else(|_| { self.serializer .deserialize::<Envelope>(frame) - .map_err(EnvelopeDecodeError::Deserialize) + .map_err(|e| ParseError::Deserialize(e.to_string())) }) }
234-235: Avoid classifying routing misses as handler errors.Incrementing
inc_handler_errors()for "no handler for message id" skews handler error metrics. Unroutable frames are not handler failures.} else { tracing::warn!(id = env.id, correlation_id = ?env.correlation_id, "no handler for message id"); - crate::metrics::inc_handler_errors(); + // Note: routing misses are not handler errors - consider adding inc_unroutable() if needed }src/app/builder.rs (1)
387-399: Clamp upper bounds for buffer size and timeout to prevent pathological inputs.Add sane maxima and document them.
- /// Set the initial buffer capacity for framed reads (clamped to ≥64). + /// Set the initial buffer capacity for framed reads (clamped to [64, 16_777_216]). @@ - pub fn buffer_capacity(mut self, capacity: usize) -> Self { - self.buffer_capacity = capacity.max(64); + pub fn buffer_capacity(mut self, capacity: usize) -> Self { + self.buffer_capacity = capacity.clamp(64, 16 * 1024 * 1024); self } @@ - /// Configure the read timeout in milliseconds (clamped to ≥1). + /// Configure the read timeout in milliseconds (clamped to [1, 86_400_000]). @@ - pub fn read_timeout_ms(mut self, timeout_ms: u64) -> Self { - self.read_timeout_ms = timeout_ms.max(1); + pub fn read_timeout_ms(mut self, timeout_ms: u64) -> Self { + self.read_timeout_ms = timeout_ms.clamp(1, 86_400_000); self }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (23)
README.md(3 hunks)docs/generic-message-fragmentation-and-re-assembly-design.md(0 hunks)docs/rust-binary-router-library-design.md(4 hunks)docs/wireframe-client-design.md(2 hunks)docs/wireframe-testing-crate.md(0 hunks)examples/echo.rs(1 hunks)examples/metadata_routing.rs(2 hunks)examples/packet_enum.rs(2 hunks)examples/ping_pong.rs(2 hunks)src/app/builder.rs(1 hunks)src/app/connection.rs(1 hunks)src/app/envelope.rs(1 hunks)src/app/error.rs(1 hunks)src/app/mod.rs(1 hunks)tests/common/mod.rs(1 hunks)tests/lifecycle.rs(4 hunks)tests/metadata.rs(1 hunks)tests/middleware_order.rs(2 hunks)tests/response.rs(4 hunks)tests/routes.rs(5 hunks)tests/wireframe_protocol.rs(3 hunks)tests/world.rs(2 hunks)wireframe_testing/src/helpers.rs(10 hunks)
💤 Files with no reviewable changes (2)
- docs/generic-message-fragmentation-and-re-assembly-design.md
- docs/wireframe-testing-crate.md
🧰 Additional context used
📓 Path-based instructions (6)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use precise names; boolean names should start with is/has/should
Use en-GB-oxendict spelling and grammar in comments
Function documentation must include clear examples; test documentation should omit redundant examples
Keep code files ≤ 400 lines; split long switch/dispatch logic by feature; move large test data to external files
Disallow Clippy warnings
Fix warnings emitted during tests in code rather than silencing them
Extract helper functions for long functions; adhere to separation of concerns and CQRS
Group related parameters into meaningful structs when functions have many parameters
Consider using Arc for large error returns to reduce data size
Each Rust module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments to enable cargo doc generation
Prefer immutable data; avoid unnecessary mut
Handle errors with Result instead of panicking where feasible
Avoid unsafe code unless 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
Do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason
Prefer #[expect(..)] over #[allow(..)] for lints
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than escaping newlines
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Files:
tests/common/mod.rssrc/app/envelope.rsexamples/packet_enum.rssrc/app/mod.rstests/lifecycle.rstests/wireframe_protocol.rswireframe_testing/src/helpers.rsexamples/metadata_routing.rssrc/app/error.rstests/response.rstests/routes.rssrc/app/connection.rstests/world.rsexamples/echo.rsexamples/ping_pong.rstests/metadata.rssrc/app/builder.rstests/middleware_order.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.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
tests/common/mod.rssrc/app/envelope.rsexamples/packet_enum.rssrc/app/mod.rstests/lifecycle.rstests/wireframe_protocol.rswireframe_testing/src/helpers.rsexamples/metadata_routing.rssrc/app/error.rstests/response.rstests/routes.rssrc/app/connection.rstests/world.rsexamples/echo.rsexamples/ping_pong.rstests/metadata.rssrc/app/builder.rstests/middleware_order.rs
{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write unit and behavioural tests for new functionality
Files:
tests/common/mod.rssrc/app/envelope.rssrc/app/mod.rstests/lifecycle.rstests/wireframe_protocol.rssrc/app/error.rstests/response.rstests/routes.rssrc/app/connection.rstests/world.rstests/metadata.rssrc/app/builder.rstests/middleware_order.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Files:
tests/common/mod.rstests/lifecycle.rstests/wireframe_protocol.rstests/response.rstests/routes.rstests/world.rstests/metadata.rstests/middleware_order.rs
docs/**/*.md
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
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 runnable examples and explain the relevant concepts and APIs.
docs/**/*.md: Use docs/ markdown ...
Files:
docs/wireframe-client-design.mddocs/rust-binary-router-library-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/wireframe-client-design.mddocs/rust-binary-router-library-design.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Markdown paragraphs and bullet points must be wrapped at 80 columns
Markdown code blocks must be wrapped at 120 columns
Do not wrap tables and headings in Markdown
Use dashes (-) for list bullets in Markdown
Use GitHub-flavoured Markdown footnotes ([^1])
Files:
docs/wireframe-client-design.mdREADME.mddocs/rust-binary-router-library-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/wireframe-client-design.mdREADME.mddocs/rust-binary-router-library-design.md
🧬 Code graph analysis (14)
tests/common/mod.rs (2)
src/app/builder.rs (2)
serializer(367-385)new(148-148)src/app/connection.rs (1)
serializer(84-85)
examples/packet_enum.rs (2)
src/app/builder.rs (2)
serializer(367-385)new(148-148)src/app/connection.rs (1)
serializer(84-85)
tests/lifecycle.rs (2)
src/app/builder.rs (2)
serializer(367-385)new(148-148)wireframe_testing/src/helpers.rs (3)
processor(22-22)run_app(381-424)run_with_duplex_server(444-452)
tests/wireframe_protocol.rs (2)
src/app/builder.rs (2)
serializer(367-385)new(148-148)src/app/connection.rs (1)
serializer(84-85)
examples/metadata_routing.rs (2)
src/app/builder.rs (2)
serializer(367-385)new(148-148)src/app/connection.rs (1)
serializer(84-85)
tests/response.rs (2)
src/app/builder.rs (2)
serializer(367-385)new(148-148)src/app/connection.rs (1)
serializer(84-85)
tests/routes.rs (2)
src/app/builder.rs (2)
serializer(367-385)new(148-148)wireframe_testing/src/helpers.rs (1)
drive_with_bincode(339-358)
src/app/connection.rs (2)
src/app/builder.rs (2)
serializer(367-385)new(148-148)src/app/envelope.rs (10)
new(81-87)new(105-111)correlation_id(50-50)correlation_id(95-95)correlation_id(117-117)id(47-47)id(92-92)id(114-114)from_parts(56-56)from_parts(99-99)
tests/world.rs (2)
src/app/builder.rs (2)
serializer(367-385)new(148-148)src/app/connection.rs (1)
serializer(84-85)
examples/echo.rs (2)
src/app/builder.rs (2)
serializer(367-385)new(148-148)src/app/connection.rs (1)
serializer(84-85)
examples/ping_pong.rs (1)
src/app/builder.rs (2)
serializer(367-385)new(148-148)
tests/metadata.rs (3)
src/app/builder.rs (2)
serializer(367-385)with_serializer(174-191)src/app/connection.rs (1)
serializer(84-85)wireframe_testing/src/helpers.rs (1)
drive_with_bincode(339-358)
src/app/builder.rs (2)
src/extractor.rs (1)
state(52-60)src/hooks.rs (1)
from_protocol(140-176)
tests/middleware_order.rs (1)
src/app/builder.rs (2)
serializer(367-385)new(148-148)
🔍 Remote MCP
Additional context from Issue #291
-
Title & rationale: Issue #291, opened by the CodeRabbitAI bot on August 8, 2025, flags that
src/app.rshad grown to 731 lines—nearly twice the 400-line limit mandated by the project’s coding guidelines. The goal is to break the monolithic file into smaller, responsibility-focused modules to improve maintainability and adhere to single-responsibility principles. -
Proposed module breakdown:
packet.rs: Defines thePackettrait andPacketPartsstructenvelope.rs: Introduces theEnvelopetype and its conversionsbuilder.rs: Contains theWireframeAppbuilder API (methods likeroute,service,wrap,app_data)protocol.rs: Manages protocol selection and hook integrationio.rs: Encapsulates frame-processor selection and thesend_responselogicconnection.rs: Implements connection setup/teardown, the read/decode loop, andhandle_framedispatching
-
References & history: The issue references the earlier PR #282 (which introduced
PacketPartsand optional correlation IDs) and its discussion. It serves as the direct motivation for the comprehensive refactor embodied in the current PR #332, which reorganizes the application layer into modular submodules undersrc/app/.
🔇 Additional comments (34)
wireframe_testing/src/helpers.rs (2)
55-96: LGTM on connection driving utilities.The duplex driving helpers and panic-to-io mapping are sound and test-friendly.
Also applies to: 230-246, 309-324, 381-424
455-468: Macros read well and add actionable context to failures.Keep these; they aid triage without adding dependencies.
Also applies to: 475-484
docs/wireframe-client-design.md (1)
41-44: Good: clarify codec-based framing.The edits correctly state that framing is via a length‑delimited codec while serialisation remains via the
Serializer.Also applies to: 55-57
README.md (1)
50-55: LGTM on builder and example updates.The builder surface and examples are consistent with the modular refactor.
Also applies to: 64-66, 141-142
src/app/error.rs (2)
28-29: PublicResultalias is fine.The alias improves ergonomics across the builder API.
1-29: Confirm thiserror dependency presenceThe
thiserror = "2.0.12"entry exists in Cargo.toml, so the derive macros will compile as expected.src/app/envelope.rs (1)
59-66: LGTM on core types and conversions.
Packet,PacketParts, andEnvelopeAPIs are cohesive, clonable, and documented; conversions are correct.Also applies to: 71-88, 102-121, 160-171
tests/wireframe_protocol.rs (2)
57-60: Use the new constructor on the alias — LGTM.
TestApp::new()?.with_protocol(protocol)matches the builder API and keeps tests terse.
81-84: Repeat instantiation via alias — LGTM.Consistent usage improves clarity across tests.
examples/packet_enum.rs (2)
10-18: Introduce App alias and updated imports — LGTM.The alias binds the serializer and envelope explicitly, matching the refactored app surface.
83-89: Construct the app via alias in the factory — LGTM.The builder chain is clear and compiles to the expected app type.
tests/middleware_order.rs (2)
9-9: Update imports to new app exports — LGTM.Importing
EnvelopeandHandlerfromappmatches the re-exports.
58-66: Instantiate via alias — LGTM.Consistent builder usage reinforces test readability.
tests/common/mod.rs (1)
16-19: Define TestApp alias for shared fixtures — LGTM.Centralising the generic selection avoids repetition across tests.
tests/routes.rs (4)
61-74: Use alias-based construction — LGTM.The closure captures and echo path read cleanly.
101-108: Repeat alias-based construction — LGTM.The none-correlation case mirrors the first test succinctly.
133-140: Alias-based construction for sequence test — LGTM.Keeps multi-frame setup focused on behaviour, not types.
193-200: Alias-based construction for parameterised test — LGTM.Consistent style across cases aids maintenance.
examples/echo.rs (1)
7-9: Adopt alias and re-exports; reads cleanImport
EnvelopeandBincodeSerializer, introduceAppalias, and switch toApp::new(). No issues.Also applies to: 12-13, 17-17
tests/metadata.rs (1)
19-27: Resolve prior constructor churn by usingwith_serializerInitialise the app with
with_serializer(serializer)to avoid type churn flagged earlier. This closes that thread here.tests/response.rs (1)
44-47: Verify whether.serializer(BincodeSerializer)is redundantIf
TestApp::new()already initialisesBincodeSerializer, drop the explicit.serializer(...).#!/usr/bin/env bash # Inspect defaults to see if serializer is already BincodeSerializer rg -nP 'impl\s+Default\s+for\s+WireframeApp' src/app rg -nP 'struct\s+WireframeApp<.+>\s*\{[\s\S]*?serializer' src/app -n -C3 rg -nP 'fn\s+new\(\)\s*->\s*Result<Self>' src/app/builder.rs -n -C5examples/ping_pong.rs (1)
17-18: Alias and builder usage are tidyHide generics behind
Appand useApp::new()?.serializer(...). Looks good.Also applies to: 135-141
examples/metadata_routing.rs (1)
11-15: Example updates align with modular app surfaceUse
Envelopeimport,Appalias, and configureHeaderSerializer. No issues.Also applies to: 17-18, 66-87
tests/world.rs (3)
13-13: Import changes align with modular refactoring.The removal of
WireframeAppfrom the direct import and addition ofBincodeSerializerproperly reflects the new modular structure where apps are constructed via type aliases with explicit serialiser specification.Also applies to: 18-18
22-22: Type alias improves clarity and consistency.The
TestAppalias centralises the generic specialisation, making it easier to maintain and ensuring consistent usage across test files.
41-41: Constructor usage updated correctly.The switch from
WireframeApp::new()toTestApp::new()properly utilises the new type alias whilst maintaining the same functionality.tests/lifecycle.rs (4)
16-16: Import changes and type aliases enhance maintainability.The removal of direct
WireframeAppimport and introduction ofApp<E>andBasicApptype aliases provides better type safety and centralised configuration management for the test suite.Also applies to: 22-24
44-48: Function signatures updated consistently.The migration from raw
WireframeApptypes to theApp<E>alias maintains type safety whilst improving readability. The addition ofon_connection_teardownproperly tests the new lifecycle callback functionality.Also applies to: 55-61
80-83: BasicApp usage is appropriate for simplified tests.Using
BasicAppfor tests that don't require complex state management provides clarity about the test's intent and reduces unnecessary generic complexity.Also applies to: 95-98
138-140: Frame processor removal reflects architectural shift.The removal of the
.frame_processor(processor())call aligns with the documented deprecation of frame processor-based framing in favour of codec-based approaches.docs/rust-binary-router-library-design.md (2)
729-729: Deprecation messaging is clear and actionable.The documentation correctly marks
frame_processoras deprecated and explains that framing is now handled by the connection codec. This provides clear migration guidance for users.Also applies to: 747-748
1273-1273: Example code updated to reflect new architecture.The commented-out
frame_processorcalls with deprecation notes in the examples help users understand the architectural transition whilst maintaining historical context.Also applies to: 1385-1385
src/app/mod.rs (1)
12-19: Re-exports provide clean API surface.The curated re-exports from the four submodules (
builder,connection,envelope,error) create a logical and discoverable public API that supports the modular refactoring objectives.src/app/builder.rs (1)
1-6: Module docs are clear and compliant.Purpose and utility are stated up-front; good alignment with project documentation standards.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
wireframe_testing/src/helpers.rs (3)
195-197: Validate duplex capacity centrally to prevent panics and enforce 10 MiB ceiling.Add a single validation helper and use it in drive_internal; drop the duplicate checks in run_app. This also fulfils the retrieved learnings for capacity validation.
Apply:
@@ const DEFAULT_CAPACITY: usize = 4096; const MAX_CAPACITY: usize = 1024 * 1024 * 10; // 10MB limit + +fn validate_capacity(capacity: usize) -> io::Result<()> { + if capacity == 0 { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "capacity must be greater than zero", + )); + } + if capacity > MAX_CAPACITY { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + format!("capacity must not exceed {MAX_CAPACITY} bytes"), + )); + } + Ok(()) +} @@ async fn drive_internal<F, Fut>( server_fn: F, frames: Vec<Vec<u8>>, capacity: usize, ) -> io::Result<Vec<u8>> where @@ - let (mut client, server) = duplex(capacity); + validate_capacity(capacity)?; + let (mut client, server) = duplex(capacity); @@ pub async fn run_app<S, C, E>( app: WireframeApp<S, C, E>, frames: Vec<Vec<u8>>, capacity: Option<usize>, ) -> io::Result<Vec<u8>> @@ - let capacity = capacity.unwrap_or(DEFAULT_CAPACITY); - if capacity == 0 { - return Err(io::Error::new( - io::ErrorKind::InvalidInput, - "capacity must be greater than zero", - )); - } - if capacity > MAX_CAPACITY { - return Err(io::Error::new( - io::ErrorKind::InvalidInput, - format!("capacity must not exceed {MAX_CAPACITY} bytes"), - )); - } + let capacity = capacity.unwrap_or(DEFAULT_CAPACITY); + validate_capacity(capacity)?;Also applies to: 230-246, 277-279, 308-324, 55-60, 391-403
18-21: Replace #[allow(...)] with narrowly scoped #[expect(...)] per guidelines.Keep the explicit reason; only relax the single lint.
-#[allow( - unused_braces, - reason = "Clippy is wrong here; this is not a redundant block" -)] +#[expect( + unused_braces, + reason = "false positive: not a redundant block" +)]
1-486: Reduce file below 400 lines as mandated.Split helpers into focused modules (e.g., helpers/drive.rs, helpers/macros.rs). Move push_expect/recv_expect macros into a macros module and re-export. Keep drive_internal and capacity checks co-located.
I can sketch a minimal module split and the re-exports to keep the public API unchanged.
♻️ Duplicate comments (5)
wireframe_testing/src/helpers.rs (1)
161-168: Docs clean-up LGTM.Approve changes to imports, std::io::Result usage, and .expect(...) across doctests. These address prior review feedback.
Also applies to: 187-194, 205-212, 222-229, 253-260, 269-276, 285-292, 300-307, 329-338, 373-380, 436-444
tests/common/mod.rs (2)
10-14: Replace forbidden #[allow] with narrowly scoped #[expect]Do not use
#[allow]. Switch to#[expect(dead_code, reason = "...")]and annotate with a FIXME.-#[allow(dead_code)] +// FIXME: Track upstream usage; remove once helper is referenced across tests. (link issue) +#[expect(dead_code, reason = "helper is used ad hoc by some tests only")] pub fn unused_listener() -> StdTcpListener {
21-27: Replace forbidden #[allow(...)] with #[expect(...)] and keep scope minimalFollow the guideline to prefer
#[expect]with a reason and a FIXME.-#[allow( - unused_braces, - reason = "rustc false positive for single line rstest fixtures" -)] +// FIXME: Track rustc false positive; remove when fixed. (link issue) +#[expect( + unused_braces, + reason = "rustc false positive for single-line rstest fixtures" +)] pub fn factory() -> impl Fn() -> TestApp + Send + Sync + Clone + 'static {src/app/builder.rs (2)
224-231: Use the Middleware alias to tighten the API surface.Stop repeating the long
Transformbound.- pub fn wrap<M>(mut self, mw: M) -> Result<Self> - where - M: Transform<HandlerService<E>, Output = HandlerService<E>> + Send + Sync + 'static, + pub fn wrap<M>(mut self, mw: M) -> Result<Self> + where + M: Middleware<E> + 'static, {
72-79: Replace invalid #[allow(..., reason = ...)] with #[expect(..., reason = ...)].
#[allow]does not acceptreasonand is forbidden by house rules. Use#[expect]with a clear justification.- #[allow( - dead_code, - reason = "Deprecated: retained temporarily for API compatibility until codec-based \ - framing is fully removed" - )] + #[expect( + dead_code, + reason = "Deprecated: retained temporarily for API compatibility until codec-based framing is fully removed" + )]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
examples/metadata_routing.rs(2 hunks)src/app/builder.rs(1 hunks)src/app/connection.rs(1 hunks)src/app/envelope.rs(1 hunks)tests/common/mod.rs(1 hunks)tests/metadata.rs(2 hunks)tests/response.rs(5 hunks)wireframe_testing/src/helpers.rs(12 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use precise names; boolean names should start with is/has/should
Use en-GB-oxendict spelling and grammar in comments
Function documentation must include clear examples; test documentation should omit redundant examples
Keep code files ≤ 400 lines; split long switch/dispatch logic by feature; move large test data to external files
Disallow Clippy warnings
Fix warnings emitted during tests in code rather than silencing them
Extract helper functions for long functions; adhere to separation of concerns and CQRS
Group related parameters into meaningful structs when functions have many parameters
Consider using Arc for large error returns to reduce data size
Each Rust module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments to enable cargo doc generation
Prefer immutable data; avoid unnecessary mut
Handle errors with Result instead of panicking where feasible
Avoid unsafe code unless 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
Do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason
Prefer #[expect(..)] over #[allow(..)] for lints
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than escaping newlines
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Files:
src/app/connection.rsexamples/metadata_routing.rstests/response.rstests/common/mod.rssrc/app/envelope.rswireframe_testing/src/helpers.rstests/metadata.rssrc/app/builder.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.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
src/app/connection.rsexamples/metadata_routing.rstests/response.rstests/common/mod.rssrc/app/envelope.rswireframe_testing/src/helpers.rstests/metadata.rssrc/app/builder.rs
{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write unit and behavioural tests for new functionality
Files:
src/app/connection.rstests/response.rstests/common/mod.rssrc/app/envelope.rstests/metadata.rssrc/app/builder.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Files:
tests/response.rstests/common/mod.rstests/metadata.rs
🧠 Learnings (4)
📚 Learning: 2025-08-11T10:46:22.844Z
Learnt from: CR
PR: leynos/wireframe#0
File: docs/wireframe-testing-crate.md:0-0
Timestamp: 2025-08-11T10:46:22.844Z
Learning: Applies to docs/wireframe_testing/src/helpers.rs : Provide capacity variants `drive_with_frame_with_capacity` and `drive_with_frames_with_capacity` that validate capacity > 0 and <= 10 MB
Applied to files:
wireframe_testing/src/helpers.rs
📚 Learning: 2025-08-11T10:46:22.844Z
Learnt from: CR
PR: leynos/wireframe#0
File: docs/wireframe-testing-crate.md:0-0
Timestamp: 2025-08-11T10:46:22.844Z
Learning: Applies to docs/wireframe_testing/src/helpers.rs : Provide mutable-borrow variants `drive_with_frame_mut(app: &mut WireframeApp, ...)` and `drive_with_frames_mut(app: &mut WireframeApp, ...)` to reuse a single app instance
Applied to files:
wireframe_testing/src/helpers.rs
📚 Learning: 2025-08-11T10:46:22.844Z
Learnt from: CR
PR: leynos/wireframe#0
File: docs/wireframe-testing-crate.md:0-0
Timestamp: 2025-08-11T10:46:22.844Z
Learning: Applies to docs/wireframe_testing/src/helpers.rs : Implement helper functions in helpers.rs: `drive_with_frame(app: WireframeApp, frame: Vec<u8>) -> IoResult<Vec<u8>>` and `drive_with_frames(app: WireframeApp, frames: Vec<Vec<u8>>) -> IoResult<Vec<u8>>`
Applied to files:
wireframe_testing/src/helpers.rs
📚 Learning: 2025-08-11T10:46:22.844Z
Learnt from: CR
PR: leynos/wireframe#0
File: docs/wireframe-testing-crate.md:0-0
Timestamp: 2025-08-11T10:46:22.844Z
Learning: Applies to docs/wireframe_testing/src/helpers.rs : Implement `drive_with_bincode<M: Serialize>(app, msg)` that encodes with bincode, frames, then delegates to `drive_with_frame`
Applied to files:
wireframe_testing/src/helpers.rs
🧬 Code graph analysis (7)
src/app/connection.rs (2)
src/app/builder.rs (2)
serializer(364-382)new(152-152)src/app/envelope.rs (10)
new(83-89)new(107-113)correlation_id(52-52)correlation_id(97-97)correlation_id(119-119)id(49-49)id(94-94)id(116-116)from_parts(58-58)from_parts(101-101)
examples/metadata_routing.rs (3)
src/app/builder.rs (2)
serializer(364-382)new(152-152)src/app/connection.rs (1)
serializer(84-85)src/app/envelope.rs (2)
new(83-89)new(107-113)
tests/response.rs (2)
src/app/builder.rs (2)
new(152-152)default(113-130)wireframe_testing/src/helpers.rs (1)
processor(22-22)
tests/common/mod.rs (2)
src/app/builder.rs (2)
new(152-152)serializer(364-382)src/app/connection.rs (1)
serializer(84-85)
wireframe_testing/src/helpers.rs (2)
tests/world.rs (1)
drop(75-88)src/connection.rs (1)
drop(38-41)
tests/metadata.rs (2)
src/app/builder.rs (3)
serializer(364-382)with_serializer(182-186)new(152-152)wireframe_testing/src/helpers.rs (1)
drive_with_bincode(339-358)
src/app/builder.rs (2)
src/extractor.rs (1)
state(52-60)src/hooks.rs (1)
from_protocol(140-176)
🔍 Remote MCP
Additional context from Issue #291
-
Title & rationale: Issue #291, opened by the CodeRabbitAI bot on August 8, 2025, flags that
src/app.rshad grown to 731 lines—nearly twice the 400-line limit mandated by the project’s coding guidelines. The goal is to break the monolithic file into smaller, responsibility-focused modules to improve maintainability and adhere to single-responsibility principles. -
Proposed module breakdown:
packet.rs: Defines thePackettrait andPacketPartsstructenvelope.rs: Introduces theEnvelopetype and its conversionsbuilder.rs: Contains theWireframeAppbuilder API (methods likeroute,service,wrap,app_data)protocol.rs: Manages protocol selection and hook integrationio.rs: Encapsulates frame-processor selection and thesend_responselogicconnection.rs: Implements connection setup/teardown, the read/decode loop, andhandle_framedispatching
-
References & history: The issue references the earlier PR #282 (which introduced
PacketPartsand optional correlation IDs) and its discussion. It serves as the direct motivation for the comprehensive refactor embodied in the current PR #332, which reorganizes the application layer into modular submodules undersrc/app/.
🔇 Additional comments (6)
wireframe_testing/src/helpers.rs (2)
431-444: Align panic docs with actual behaviour of handle_connection.Verify the return type; if it returns (), the “Panics” section is misleading. If it returns Result, propagate or unwrap with context.
#!/bin/bash # Confirm the signature(s) of handle_connection in the wireframe crate rg -nP --type=rust -C3 '^\s*(pub\s+)?async\s+fn\s+handle_connection\s*\(|trait\s+\w+[^{]*\{[^}]*handle_connection' . # Find call sites that handle a Result from handle_connection rg -nP --type=rust -C2 '\.handle_connection\(' | rg -nP -C1 'await\?|\.(expect|unwrap)\(|match\s|if\s+let'
161-168: Verify doctest paths resolve via crate re-exports.Doctests import functions from crate root (wireframe_testing::drive_*). Ensure lib.rs re-exports these items.
#!/bin/bash # Check crate-root re-exports for drive_* helpers rg -nP --type=rust -C2 'pub\s+use\s+crate::helpers::\s*{?[^}]*\bdrive_with_(frame|frames)[^}]*}?;' wireframe_testing # List public exports from crate root for visibility rg -nP --type=rust -n 'pub\s+use' wireframe_testing/src/lib.rsAlso applies to: 187-194, 205-212, 222-229, 253-260, 269-276, 285-292, 300-307, 329-338, 373-380
src/app/connection.rs (1)
146-158: LGTM: explicit timeout handling preserves loop and logs idlenessThe loop now distinguishes EOF, I/O errors, and timeouts with a clear idle log. This resolves the prior silent-drop behaviour.
src/app/envelope.rs (2)
168-179: Conversions are tight and idiomatic.The
Fromimpls are correct and zero-copy where appropriate.
1-8: Update documentation links to use the publicWireframeAppre-exportReplace internal
crate::app::builder::WireframeAppreferences with the publiccrate::app::WireframeAppre-export (located insrc/app/mod.rson line 17).
- src/app/envelope.rs (doc comments, lines 1–8)
-//! builder (`crate::app::WireframeApp`) to route frames before full -//! deserialisation. See [`crate::app::builder::WireframeApp`] for how envelopes -//! are used when registering routes. +//! builder ([`crate::app::WireframeApp`]) to route frames before full +//! deserialisation. See [`crate::app::WireframeApp`] for how envelopes are used +//! when registering routes.src/app/builder.rs (1)
349-360: Deprecation path is sound.The deprecated
frame_processor()setter preserves API while steering callers to codec-managed framing.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
wireframe_testing/src/helpers.rs (2)
18-22: Replace invalid #[allow(..., reason = "...")] with #[expect]
#[allow(..., reason = "...")]is not valid. Use#[expect(..., reason = "...")]per guidelines.-#[allow( - unused_braces, - reason = "Clippy is wrong here; this is not a redundant block" -)] +#[expect( + unused_braces, + reason = "Braces are intentional here; false positive" +)] pub fn processor() -> LengthPrefixedProcessor { LengthPrefixedProcessor::default() }
43-51: Prefer std::io in examples; avoid aliasing tokio::io as ioReference
std::io::Resultand import only the tokio types you need.-/// use tokio::io::{self, AsyncWriteExt, DuplexStream}; +/// use tokio::io::{AsyncWriteExt, DuplexStream}; @@ -/// # async fn demo() -> io::Result<()> { +/// # async fn demo() -> std::io::Result<()> {
♻️ Duplicate comments (2)
src/app/builder.rs (2)
224-231: Use the Middleware alias in wrap()Tighten the public surface by using the alias instead of repeating bounds.
- pub fn wrap<M>(mut self, mw: M) -> Result<Self> - where - M: Transform<HandlerService<E>, Output = HandlerService<E>> + Send + Sync + 'static, + pub fn wrap<M>(mut self, mw: M) -> Result<Self> + where + M: Middleware<E> + 'static, { self.middleware.push(Box::new(mw)); self.routes = OnceCell::new(); Ok(self) }
178-186: Remove clippy suppression; construct with struct update syntaxDrop the lint suppression and avoid reassignment after default.
- #[allow( - clippy::field_reassign_with_default, - reason = "overriding serializer post-default simplifies builder" - )] pub fn with_serializer(serializer: S) -> Result<Self> { - let mut app = Self::default(); - app.serializer = serializer; - Ok(app) + Ok(Self { serializer, ..Self::default() }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/app/builder.rs(1 hunks)src/app/mod.rs(1 hunks)tests/metadata.rs(2 hunks)tests/response.rs(5 hunks)wireframe_testing/src/helpers.rs(13 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use precise names; boolean names should start with is/has/should
Use en-GB-oxendict spelling and grammar in comments
Function documentation must include clear examples; test documentation should omit redundant examples
Keep code files ≤ 400 lines; split long switch/dispatch logic by feature; move large test data to external files
Disallow Clippy warnings
Fix warnings emitted during tests in code rather than silencing them
Extract helper functions for long functions; adhere to separation of concerns and CQRS
Group related parameters into meaningful structs when functions have many parameters
Consider using Arc for large error returns to reduce data size
Each Rust module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments to enable cargo doc generation
Prefer immutable data; avoid unnecessary mut
Handle errors with Result instead of panicking where feasible
Avoid unsafe code unless 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
Do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason
Prefer #[expect(..)] over #[allow(..)] for lints
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than escaping newlines
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Files:
src/app/mod.rswireframe_testing/src/helpers.rstests/response.rssrc/app/builder.rstests/metadata.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.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
src/app/mod.rswireframe_testing/src/helpers.rstests/response.rssrc/app/builder.rstests/metadata.rs
{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write unit and behavioural tests for new functionality
Files:
src/app/mod.rstests/response.rssrc/app/builder.rstests/metadata.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Files:
tests/response.rstests/metadata.rs
🧬 Code graph analysis (3)
tests/response.rs (2)
src/app/builder.rs (2)
new(152-152)default(113-130)wireframe_testing/src/helpers.rs (1)
processor(22-22)
src/app/builder.rs (2)
src/extractor.rs (1)
state(52-60)src/hooks.rs (1)
from_protocol(140-176)
tests/metadata.rs (2)
src/app/builder.rs (2)
serializer(364-382)new(152-152)wireframe_testing/src/helpers.rs (1)
drive_with_bincode(340-359)
🔍 Remote MCP
Additional context from Issue #291
-
Title & rationale: Issue #291, opened by the CodeRabbitAI bot on August 8, 2025, flags that
src/app.rshad grown to 731 lines—nearly twice the 400-line limit mandated by the project’s coding guidelines. The goal is to break the monolithic file into smaller, responsibility-focused modules to improve maintainability and adhere to single-responsibility principles. -
Proposed module breakdown:
packet.rs: Defines thePackettrait andPacketPartsstructenvelope.rs: Introduces theEnvelopetype and its conversionsbuilder.rs: Contains theWireframeAppbuilder API (methods likeroute,service,wrap,app_data)protocol.rs: Manages protocol selection and hook integrationio.rs: Encapsulates frame-processor selection and thesend_responselogicconnection.rs: Implements connection setup/teardown, the read/decode loop, andhandle_framedispatching
-
References & history: The issue references the earlier PR #282 (which introduced
PacketPartsand optional correlation IDs) and its discussion. It serves as the direct motivation for the comprehensive refactor embodied in the current PR #332, which reorganizes the application layer into modular submodules undersrc/app/.
⏰ 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 (7)
wireframe_testing/src/helpers.rs (2)
98-101: LGTM: Introduce EMPTY_SERVER_CAPACITYGood extraction of the magic number into a named constant.
451-453: LGTM: Discard client half inlinePattern-discarding the client half with the named capacity constant is clean and avoids bind-then-drop.
src/app/builder.rs (1)
384-397: LGTM: Clamp configuration inputsClamping
buffer_capacityandread_timeout_msto sane ranges prevents pathological behaviour.tests/response.rs (2)
13-15: LGTM: Deduplicate TestApp via shared moduleImporting the shared
TestAppalias keeps tests consistent.
39-44: LGTM: Docs before attributes and -ize spellingDoc comments precede attributes and use “serializes/deserialized” as required.
src/app/mod.rs (1)
1-19: LGTM: Curated re-exports and module docsThe module-level docs clearly define the surface and re-exports. Meets the guideline for module documentation.
tests/metadata.rs (1)
30-31: LGTM: Derive Default for test serializersDefaults simplify construction in tests and enable the refactor above.
Also applies to: 74-75
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please provide an AI prompt for the removal of all code deprecated by this PR |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 10091 @leynos: Based on my analysis of the codebase, here's a comprehensive AI prompt for removing all code deprecated by this PR: 🐰✨ |
1 participant
Summary
appmod.rsTesting
make fmtmake lintmake testhttps://chatgpt.com/codex/tasks/task_e_68ac877d26688322beaffb55fde0c6d4
Summary by Sourcery
Reorganize the application layer into modular submodules under src/app and consolidate exports via a central mod.rs.
Enhancements: