Conversation
Reviewer's GuideThis PR extends the core Response enum with a channel-backed MultiPacket variant using a tokio mpsc receiver, updates debugging and documentation for the new variant, and verifies its behavior through unit and Cucumber tests. Flow diagram for MultiPacket response lifecycleflowchart TD
A["Handler creates mpsc::channel (Sender, Receiver)"] --> B["Frames sent via Sender"]
B --> C["Receiver yields frames in send order"]
C --> D["Sender dropped after sending all frames"]
D --> E["Receiver returns None, signaling end of stream"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Note Reviews pausedUse the following commands to manage reviews:
Summary by CodeRabbit
WalkthroughAdd a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Handler
participant Tx as "mpsc::Sender<F>"
participant Rx as "mpsc::Receiver<F>"
participant Consumer as "Runtime / Drainer"
Client->>Handler: Send request
Handler->>Tx: Create channel (tx, rx)
Handler-->>Client: Return Response::MultiPacket(rx)
Note over Handler,Tx: Handler sends frames via tx and drops/closes tx when done
loop while rx yields frames
Tx-->>Rx: send(frame)
Rx-->>Consumer: recv() -> frame
Consumer->>Consumer: Process frame
end
Rx-->>Consumer: None (channel closed)
Consumer->>Client: Finalise response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/response.rs (2)
74-76: AddFrom<mpsc::Receiver<F>>for ergonomics.Enable
rx.into()like otherFromimpls to keep the API consistent.impl<F, E> From<Vec<F>> for Response<F, E> { fn from(v: Vec<F>) -> Self { Response::Vec(v) } } + +impl<F, E> From<mpsc::Receiver<F>> for Response<F, E> { + fn from(rx: mpsc::Receiver<F>) -> Self { Response::MultiPacket(rx) } +}
1-31: Extend examples to includeMultiPacketand annotate code fences.Add a
MultiPacketexample and specify the language on fenced blocks for better docs rendering.-//! ``` +//! ```rust //! use futures::{StreamExt, stream}; //! use wireframe::Response; @@ //! let empty: Response<Frame, Error> = Response::Empty; //! ``` +//! +//! ```rust,no_run +//! # type Frame = u8; +//! use wireframe::Response; +//! let (_tx, rx) = tokio::sync::mpsc::channel::<Frame>(4); +//! let multi: Response<Frame, ()> = Response::MultiPacket(rx); +//! ```docs/multi-packet-and-streaming-responses-design.md (2)
131-137: Align error conversion with implementation (from_iovsFrom<std::io::Error>).The design doc shows an
impl From<std::io::Error> for WireframeError<E>, but the code exposesWireframeError::from_io. Update the doc for consistency, or implement theFromtrait in code.-// Implement `From<std::io::Error>` for ergonomics. -impl<E> From<std::io::Error> for WireframeError<E> { - fn from(e: std::io::Error) -> Self { - WireframeError::Io(e) - } -} +impl<E> WireframeError<E> { + /// Convert an I/O error into a `WireframeError`. + pub fn from_io(e: std::io::Error) -> Self { + WireframeError::Io(e) + } +}
245-256: Use the mandated markdownlint directive form.Replace the block-wide
MD013disable withdisable-next-lineas required, and duplicate it for each long table line.-<!-- markdownlint-disable MD013 --> +<!-- markdownlint-disable-next-line MD013 -->Replicate the directive before the header and each row that exceeds 80 columns.
tests/cucumber.rs (2)
3-7: Update module docs to list the new suiteDocument the fourth suite to keep docs in sync with code.
//! Orchestrates three distinct test suites: -//! - `PanicWorld`: Tests server resilience during connection panics -//! - `CorrelationWorld`: Tests correlation ID propagation in multi-frame responses -//! - `StreamEndWorld`: Verifies end-of-stream signalling +//! Orchestrates four distinct test suites: +//! - `PanicWorld`: Tests server resilience during connection panics +//! - `CorrelationWorld`: Tests correlation ID propagation in multi-frame responses +//! - `StreamEndWorld`: Verifies end-of-stream signalling +//! - `MultiPacketWorld`: Verifies ordered delivery from channel-backed multi-packet responses
10-15: Extend the example list with the new featureKeep the doc example block exhaustive.
//! ```text //! tests/features/connection_panic.feature -> PanicWorld context //! tests/features/correlation_id.feature -> CorrelationWorld context //! tests/features/stream_end.feature -> StreamEndWorld context +//! tests/features/multi_packet.feature -> MultiPacketWorld context //! ```tests/world.rs (1)
1-5: Fix module-level doc driftBroaden the module doc; it no longer describes only panic tests.
-//! Test world state for Cucumber panic resilience tests. +//! Test worlds for Cucumber integration suites: +//! - Panic resilience during connection setup +//! - Correlation ID propagation across frames +//! - End-of-stream signalling +//! - Multi-packet channel-backed responses (ordered delivery)
📜 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 (9)
docs/multi-packet-and-streaming-responses-design.md(2 hunks)docs/roadmap.md(1 hunks)src/response.rs(4 hunks)tests/cucumber.rs(1 hunks)tests/features/multi_packet.feature(1 hunks)tests/multi_packet.rs(1 hunks)tests/steps/mod.rs(1 hunks)tests/steps/multi_packet_steps.rs(1 hunks)tests/world.rs(4 hunks)
🧰 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/multi_packet.rstests/steps/mod.rstests/steps/multi_packet_steps.rstests/cucumber.rssrc/response.rstests/world.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/multi_packet.rstests/steps/mod.rstests/steps/multi_packet_steps.rstests/cucumber.rssrc/response.rstests/world.rs
{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write unit and behavioural tests for new functionality
Files:
tests/multi_packet.rstests/steps/mod.rstests/steps/multi_packet_steps.rstests/cucumber.rssrc/response.rstests/world.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/multi_packet.rstests/steps/mod.rstests/steps/multi_packet_steps.rstests/cucumber.rstests/world.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
docs/**/*.md: Use docs/ markdown as the source of truth for requirements and decisions
Proactively update docs/ when requirements, dependencies, or architecture change
Documentation must use en-GB-oxendict spelling and grammar (LICENSE name exempt)When long lines are warranted in ...
Files:
docs/multi-packet-and-streaming-responses-design.mddocs/roadmap.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/multi-packet-and-streaming-responses-design.mddocs/roadmap.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/multi-packet-and-streaming-responses-design.mddocs/roadmap.md
docs/**
📄 CodeRabbit inference engine (docs/wireframe-1-0-detailed-development-roadmap.md)
docs/**: Document async-stream as the canonical way to create streams imperatively
Write comprehensive user guides for Duplex Messaging & Pushes, Streaming Responses, and Message Fragmentation with runnable examples
Files:
docs/multi-packet-and-streaming-responses-design.mddocs/roadmap.md
🔇 Additional comments (8)
docs/roadmap.md (1)
197-199: Align type with implementation: spell out the receiver type generically.Use the fully qualified type to match
src/response.rsanddocs/designfor clarity.- - [x] Introduce a `Response::MultiPacket` variant that contains a channel - `Receiver<Message>`. + - [x] Introduce a `Response::MultiPacket` variant that contains a channel + `tokio::sync::mpsc::Receiver<F>`.⛔ Skipped due to learnings
Learnt from: CR PR: leynos/wireframe#0 File: docs/roadmap.md:0-0 Timestamp: 2025-08-17T16:48:14.261Z Learning: Applies to docs/src/response.rs : Introduce Response::MultiPacket with a channel Receiver<Message>docs/multi-packet-and-streaming-responses-design.md (1)
92-99: Confirm API text matches code (genericF, Receiver fully qualified).Keep the
MultiPacket(tokio::sync::mpsc::Receiver<F>)spelling in this section aligned withsrc/response.rs.tests/steps/mod.rs (1)
7-7: LGTM: wire up multi-packet cucumber steps.The step module inclusion is correct and scoped. No issues.
tests/features/multi_packet.feature (1)
1-4: LGTM: Scenario reads clearly and maps 1:1 to stepsKeep the steps minimal and declarative as you’ve done.
tests/cucumber.rs (1)
24-24: LGTM: Wire the world and run the suiteImports and execution order look correct.
Also applies to: 31-31
tests/steps/multi_packet_steps.rs (1)
1-10: LGTM: Step definitions are lean and idiomaticKeep step bodies thin and delegate to world helpers.
Run the script to ensure the steps module registers these step defs.
#!/bin/bash # Verify tests/steps/mod.rs declares `mod multi_packet_steps;` rg -nP '^\s*mod\s+multi_packet_steps\s*;' tests/steps/mod.rs -C1tests/world.rs (2)
256-259: LGTM: Verify ordered deliveryAssertion expresses the contract crisply.
227-260: Verify docs and roadmap entries for the new Response::MultiPacketEnsure public docs and roadmap reflect the new variant per repo guidelines.
#!/bin/bash # Check docs mention `Response::MultiPacket` and channel semantics rg -n 'Response::MultiPacket' docs -C2 || true rg -nP 'multi-?packet|channel-?based responses' docs -C2 || true # Check roadmap updated rg -nP 'roadmap|Multi-?packet' docs -C2 || true
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/features/multi_packet.feature (1)
9-9: Terminate file with a trailing newlineKeep POSIX-friendly formatting and quieter diffs.
♻️ Duplicate comments (2)
tests/features/multi_packet.feature (1)
1-4: Add a feature tag for targeted runsEnable selective execution of this suite.
Apply this diff:
+@multi_packet Feature: Multi-packet responses Scenario: messages from a multi-packet response are delivered sequentially When a multi-packet response emits messages Then all messages are received in ordertests/multi_packet.rs (1)
58-79: Exercise back-pressure with parameterised casesAdd
rstestto cover varying capacities and message counts to stress FIFO and channel fullness.Apply this diff after existing tests:
+#[rstest::rstest] +#[case(1, 8)] +#[case(2, 32)] +#[case(4, 64)] +#[tokio::test] +async fn multi_packet_ordering_under_pressure(#[case] cap: usize, #[case] n: u8) { + let (tx, rx) = mpsc::channel(cap); + let send = tokio::spawn(async move { + for i in 0..n { + tx.send(TestMsg(i)).await.expect("send"); + } + // drop sender by exiting task + }); + let mut out = Vec::new(); + let mut rx = if let Response::MultiPacket(r) = Response::MultiPacket(rx) { r } else { unreachable!() }; + while let Some(m) = rx.recv().await { out.push(m); } + send.await.expect("join"); + assert_eq!(out, (0..n).map(TestMsg).collect::<Vec<_>>()); +}
📜 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/response.rs(4 hunks)tests/features/multi_packet.feature(1 hunks)tests/multi_packet.rs(1 hunks)tests/steps/multi_packet_steps.rs(1 hunks)tests/world.rs(2 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:
tests/multi_packet.rstests/steps/multi_packet_steps.rssrc/response.rstests/world.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/multi_packet.rstests/steps/multi_packet_steps.rssrc/response.rstests/world.rs
{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write unit and behavioural tests for new functionality
Files:
tests/multi_packet.rstests/steps/multi_packet_steps.rssrc/response.rstests/world.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/multi_packet.rstests/steps/multi_packet_steps.rstests/world.rs
🪛 GitHub Check: build-test
tests/world.rs
[failure] 241-241:
used underscore-prefixed binding
🪛 GitHub Actions: CI
tests/world.rs
[error] 241-241: Command 'cargo clippy --all-targets --all-features -- -D warnings' failed. Clippy error: 'used underscore-prefixed binding' at tests/world.rs:241:14.
🔇 Additional comments (6)
src/response.rs (3)
94-103: LGTM: Debug impl includes MultiPacketDebug output remains stable and non-invasive by eliding inner state.
106-112: LGTM: idiomatic one-liners for From implsFollow the style guide (no
returnin single-line functions).Also applies to: 110-112
35-36: Remove suggestion to abstract Tokio dependency
Crate is explicitly built on Tokio I/O and runtime (README.md:14 “Transport adapter built on Tokio I/O”; 33 “runs the Tokio event loop”), so exposingtokio::sync::mpsc::Receiverin the public API aligns with documented intent.Likely an incorrect or invalid review comment.
tests/multi_packet.rs (1)
9-25: LGTM: basic yield and drain path is correctValidate that all sent messages are received and order is preserved.
tests/steps/multi_packet_steps.rs (1)
1-16: LGTM: step wiring matches feature scenariosStep names and sync/async boundaries are correct; module docs use en-GB spelling.
tests/world.rs (1)
164-168: LGTM: verify() docs now correctly describe correlation-ID validationKeep this focused on CID semantics.
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/rust-binary-router-library-design.md (2)
666-672: Fix broken code block and misplaced MD013 directive.Close the code fence correctly, remove the HTML comment from inside the Rust snippet, and break the long chained call rather than disabling MD013 mid-snippet.
Apply:
- Router::new() - .message_guarded( - MessageType::GenericCommand, - <!-- markdownlint-disable-next-line MD013 --> - | msg_header: &CommandHeader | msg_header.sub_type == CommandSubType::Special, handle_special_command ).message(MessageType::GenericCommand, handle_generic_command) // Fallback ``` | + Router::new() + .message_guarded( + MessageType::GenericCommand, + |hdr: &CommandHeader| hdr.sub_type == CommandSubType::Special, + handle_special_command, + ) + .message(MessageType::GenericCommand, handle_generic_command);
1636-1642: Add missing footnote definitions in docs/rust-binary-router-library-design.md
Definitions currently cover only [^1]–[^6], yet the document references footnotes beyond [^6] (e.g. [^9–^14], [^19:known], [^23:A], …, [^270:]). Add definitions for each referenced footnote at the end of the file or renumber all footnotes sequentially so every reference has a matching definition.
♻️ Duplicate comments (4)
tests/multi_packet.rs (1)
58-79: Exercise back‑pressure with parameterised cases.Add rstest cases varying capacity and message count; interleave small sleeps in sender to stress ordering.
Apply (append to file):
+#[rstest::rstest] +#[case(1, 32u8)] +#[case(4, 64u8)] +#[tokio::test] +async fn multi_packet_ordering_under_pressure(#[case] cap: usize, #[case] n: u8) { + let (tx, rx) = mpsc::channel(cap); + tokio::spawn(async move { + for i in 0..n { + tx.send(TestMsg(i)).await.expect("send"); + if i % 7 == 0 { tokio::task::yield_now().await; } + } + }); + let received = if let Response::MultiPacket(rx) = Response::MultiPacket(rx) { + drain_all(rx).await + } else { unreachable!() }; + assert_eq!(received, (0..n).map(TestMsg).collect::<Vec<_>>()); +}tests/world.rs (3)
221-233: Defend against state bleed across steps.Clear
messagesat entry to make steps idempotent.Apply:
pub async fn process(&mut self) { + self.messages.clear(); let (tx, ch_rx) = tokio::sync::mpsc::channel(4);
239-248: Defend against state bleed inprocess_empty.Clear
messagesbefore draining.Apply:
pub async fn process_empty(&mut self) { + self.messages.clear(); let (tx, ch_rx) = tokio::sync::mpsc::channel(4);
235-241: Fix inaccurate panic docs for empty path.No sends occur; adjust summary and Panics section.
Apply:
- /// Send no messages through the channel and record them. - /// - /// # Panics - /// Panics if sending to the channel fails. + /// Record zero messages from a closed channel. + /// + /// # Panics + /// This function does not panic.
📜 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 (4)
docs/mocking-network-outages-in-rust.md(1 hunks)docs/rust-binary-router-library-design.md(3 hunks)tests/multi_packet.rs(1 hunks)tests/world.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
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
docs/**/*.md: Use docs/ markdown as the source of truth for requirements and decisions
Proactively update docs/ when requirements, dependencies, or architecture change
Documentation must use en-GB-oxendict spelling and grammar (LICENSE name exempt)When long lines are warranted in ...
Files:
docs/mocking-network-outages-in-rust.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/mocking-network-outages-in-rust.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/mocking-network-outages-in-rust.mddocs/rust-binary-router-library-design.md
docs/**
📄 CodeRabbit inference engine (docs/wireframe-1-0-detailed-development-roadmap.md)
docs/**: Document async-stream as the canonical way to create streams imperatively
Write comprehensive user guides for Duplex Messaging & Pushes, Streaming Responses, and Message Fragmentation with runnable examples
Files:
docs/mocking-network-outages-in-rust.mddocs/rust-binary-router-library-design.md
**/*.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/multi_packet.rstests/world.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/multi_packet.rstests/world.rs
{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write unit and behavioural tests for new functionality
Files:
tests/multi_packet.rstests/world.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/multi_packet.rstests/world.rs
🔍 Remote MCP
Additional Context for Reviewing PR #349
- The pull request introduces its changes on the branch codex/implement-response-multipacket-variant, which is active and was last updated on September 5, 2025 (github.com).
- As of that update, the feature branch contains 250 commits, compared to 235 commits on
main, meaning it is 15 commits ahead of the default branch. Reviewers should ensure the branch is rebased or merged cleanly against the latestmainto avoid any integration conflicts (github.com).
🔇 Additional comments (3)
docs/mocking-network-outages-in-rust.md (1)
56-57: Keep numbered list sequencing consistent.The renumbered shutdown-signal item aligns with markdownlint and the docs guidelines. No further action.
tests/world.rs (2)
164-168: LGTM: focused verification docs.The updated docstring precisely states the correlation‑ID check and panic behaviour.
1-4: Update module‑level docs to reflect all worlds.Document correlation IDs, stream end, and multi‑packet worlds per docs guidelines (en‑GB, ≤80 cols).
Apply:
-//! Test world state for Cucumber panic resilience tests. -//! -//! Provides shared state management for behavioural tests verifying -//! server resilience against connection task panics. +//! Test worlds for Cucumber suites: +//! - Panic resilience during connection setup +//! - Correlation ID propagation across frames +//! - End‑of‑stream signalling +//! - Channel‑backed multi‑packet responses (ordered delivery)Run:
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 (1)
tests/world.rs (1)
1-4: Update module-level docs to reflect all worlds.Document Panic, Correlation, StreamEnd, and MultiPacket worlds per guidelines.
Apply:
-//! Test world state for Cucumber panic resilience tests. -//! -//! Provides shared state management for behavioural tests verifying -//! server resilience against connection task panics. +//! Cucumber test worlds for integration suites: +//! - Panic resilience during connection setup +//! - Correlation ID propagation across frames +//! - End-of-stream signalling +//! - Multi-packet channel-backed responses (ordered delivery)
♻️ Duplicate comments (1)
tests/multi_packet.rs (1)
64-84: Parameterise to exercise back-pressure and larger sequences.Add rstest-powered cases to cover multiple capacities and message counts and insert light delays to interleave.
Append:
+use tokio::time::{sleep, Duration}; +use rstest::rstest; + +#[rstest] +#[case(1, 32u8)] +#[case(2, 64u8)] +#[case(4, 8u8)] +#[tokio::test] +async fn multi_packet_ordering_under_pressure(#[case] cap: usize, #[case] n: u8) { + let (tx, rx) = mpsc::channel(cap); + let sender = tokio::spawn(async move { + for i in 0..n { + tx.send(TestMsg(i)).await.expect("send"); + if i % 3 == 0 { sleep(Duration::from_millis(1)).await; } + } + }); + let received = drain_all(unwrap_rx(Response::MultiPacket(rx))).await; + sender.await.expect("join"); + assert_eq!(received, (0..n).map(TestMsg).collect::<Vec<_>>()); +}
📜 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 (2)
tests/multi_packet.rs(1 hunks)tests/world.rs(2 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:
tests/multi_packet.rstests/world.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/multi_packet.rstests/world.rs
{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write unit and behavioural tests for new functionality
Files:
tests/multi_packet.rstests/world.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/multi_packet.rstests/world.rs
🔍 Remote MCP
Additional Context for Reviewing PR #349
- The pull request introduces its changes on the branch codex/implement-response-multipacket-variant, which is active and was last updated on September 5, 2025 (github.com).
- As of that update, the feature branch contains 250 commits, compared to 235 commits on
main, meaning it is 15 commits ahead of the default branch. Reviewers should ensure the branch is rebased or merged cleanly against the latestmainto avoid any integration conflicts (github.com).
⏰ 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 (2)
tests/multi_packet.rs (1)
6-7: Derives are tight and appropriate.Keep as-is; no unused traits derived.
tests/world.rs (1)
164-168: Doc correction reads well.Accurately scopes CorrelationWorld verification to correlation IDs.
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
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.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- Consider providing a unified Stream interface for MultiPacket responses (for example, implementing
StreamforResponseor adding aninto_streamhelper) so downstream code can consume frames without manual pattern matches and channel plumbing. - There’s duplicated drain-and-verify logic in the unit tests and Cucumber
Worldsteps—extract a shared helper to reduce boilerplate when testing MultiPacket message sequences.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider providing a unified Stream interface for MultiPacket responses (for example, implementing `Stream` for `Response` or adding an `into_stream` helper) so downstream code can consume frames without manual pattern matches and channel plumbing.
- There’s duplicated drain-and-verify logic in the unit tests and Cucumber `World` steps—extract a shared helper to reduce boilerplate when testing MultiPacket message sequences.
## Individual Comments
### Comment 1
<location> `tests/multi_packet.rs:64` </location>
<code_context>
+ assert_eq!(received, vec![TestMsg(1)]);
+}
+
+/// Handles more messages than the channel capacity allows.
+#[tokio::test]
+async fn multi_packet_handles_channel_capacity() {
+ let (tx, rx) = mpsc::channel(2);
+ let send_task = tokio::spawn(async move {
</code_context>
<issue_to_address>
Consider adding a test for channel send failures.
Add a test where the sender tries to send after the receiver is dropped, confirming that the send fails as expected.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
assert_eq!(received, vec![TestMsg(1)]);
}
=======
assert_eq!(received, vec![TestMsg(1)]);
}
/// Test that sending fails after the receiver is dropped.
#[tokio::test]
async fn multi_packet_send_fails_after_receiver_dropped() {
let (tx, rx) = mpsc::channel::<TestMsg>(2);
drop(rx); // Drop the receiver
let result = tx.send(TestMsg(42)).await;
assert!(result.is_err(), "Send should fail when receiver is dropped");
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `src/response.rs:91` </location>
<code_context>
+ /// }
+ /// }
+ /// ```
+ MultiPacket(mpsc::Receiver<F>),
/// A response with no frames.
Empty,
</code_context>
<issue_to_address>
Consider deprecating the MultiPacket variant and consolidating channel outputs into the Stream variant with a From<Receiver<F>> implementation.
```markdown
It looks like `MultiPacket` is just a special‐case of your existing `Stream` variant, which means you can consolidate channel outputs into `Response::Stream`. You can then deprecate `MultiPacket` (keeping it around for backwards‐compat) and provide a `From<mpsc::Receiver<F>>` impl that automatically wraps the receiver into a boxed stream. This removes the extra branch from day‐to‐day use and avoids duplicating the docs/lifecycle rules.
1. Mark `MultiPacket` as deprecated and simplify its doc to a one‐liner:
```rust
pub enum Response<F, E = ()> {
Single(F),
Vec(Vec<F>),
Stream(FrameStream<F, E>),
#[deprecated(
note = "use `Response::Stream` (with `From<Receiver<F>>`) instead; \
this will be removed in a future release"
)]
/// Deprecated alias for `Stream` of a `tokio::sync::mpsc::Receiver`.
MultiPacket(tokio::sync::mpsc::Receiver<F>),
Empty,
}
```
2. Add an `impl From<Receiver<F>> for Response<F, E>` so users can just do:
`Response::from(rx)` and get a boxed stream under the hood:
```rust
use futures::{StreamExt, stream::Map};
use tokio::sync::mpsc;
use wireframe::{Response, WireframeError};
impl<F, E> From<mpsc::Receiver<F>> for Response<F, E> {
fn from(rx: mpsc::Receiver<F>) -> Self {
// map each `F` into `Ok(F)` and box the stream
let s = rx.map(|frame| Ok::<F, WireframeError<E>>(frame));
Response::Stream(Box::pin(s) as FrameStream<F, E>)
}
}
```
3. Adjust your examples to use `Response::Stream(Box::pin(rx.map(Ok)))` or simply `Response::from(rx)`.
With this:
- All channel‐based fly‐away frames go through the single `Stream` variant.
- You collapse two almost‐identical branches into one.
- You avoid re‐writing all the lifecycle docs twice (just document channels as a use‐case in `FrameStream`).
- You naturally deprecate `MultiPacket` without breaking existing callers.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
1 participant
Summary
ResponsewithMultiPacketvariant backed by a channelTesting
make fmtmake lintmake testmake markdownlintmake nixiehttps://chatgpt.com/codex/tasks/task_e_68b9ec01bb788322a32a6815d8a95667
Summary by Sourcery
Introduce a channel-backed multi-packet response variant to the core Response type and fully integrate it across code, documentation, and tests.
New Features:
Enhancements:
Documentation:
Tests: