Conversation
Reviewer's GuideThis PR introduces a unified streaming interface for all Response variants by adding the Class diagram for updated Response and FrameStreamclassDiagram
class Response {
+Single(frame: F)
+Vec(frames: Vec<F>)
+Stream(stream: FrameStream<F, E>)
+MultiPacket(rx: Receiver<F>)
+Empty
+into_stream() FrameStream<F, E>
}
class FrameStream {
<<type alias>>
}
Response --> FrameStream : into_stream()
Response : +from(Vec<F>)
Flow diagram for unified Response streaming via into_streamflowchart TD
A["Response (any variant)"] --> B["into_stream()"]
B --> C["FrameStream<F, E>"]
C --> D["Downstream code iterates frames"]
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
WalkthroughIntroduce Response::into_stream to convert all Response variants into a FrameStream. Update tests to consume responses via streams instead of matching MultiPacket. Add design documentation describing the behaviour and usage. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller as Caller
participant Resp as Response<F,E>
participant API as Response::into_stream
participant FS as FrameStream<F,E>
participant Cons as Consumer
Caller->>Resp: obtain response
Caller->>API: into_stream(self)
API->>FS: build FrameStream from variant
note right of API #88ccee: Single → one Ok(frame)
note right of API #88ccee: Vec → Ok(frame) per item
note right of API #88ccee: Stream → passthrough
note right of API #88ccee: MultiPacket → unfold(rx.recv)
note right of API #88ccee: Empty → completes immediately
API-->>Caller: FrameStream
loop iterate frames
Caller->>FS: next / poll
FS-->>Cons: Result<Frame, E>
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
✨ Finishing Touches🧪 Generate unit tests
Comment |
![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.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
docs/multi-packet-and-streaming-responses-design.md(1 hunks)src/response.rs(1 hunks)tests/multi_packet.rs(5 hunks)tests/world.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/multi-packet-and-streaming-responses-design.md
**/*.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.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/world.rssrc/response.rstests/multi_packet.rs
🔇 Additional comments (7)
docs/multi-packet-and-streaming-responses-design.md (1)
113-116: Document the unified consumption path clearly (LGTM).The addition succinctly explains the purpose of
into_streamand avoids second-person phrasing. No further edits required for style or UK spelling.tests/world.rs (1)
11-11: Import only the extension used (LGTM).
StreamExtis required for.next(). Keep as-is.tests/multi_packet.rs (5)
3-3: Import StreamExt is correct for map/collect usage.
Keep this; it enables the combinators used below.
25-25: Adopt into_stream in the assertion path.
The new unified interface is exercised correctly.
35-35: Use into_stream for the empty-channel case.
This correctly asserts EOS behaviour.
46-46: Use into_stream for early sender drop.
Covers termination semantics; looks good.
60-60: Stream through capacity pressure scenario is correct.
Backpressure is exercised while preserving order.
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
♻️ Duplicate comments (1)
tests/multi_packet.rs (1)
10-13: Generalise drain_all to any TryStream.Widen the helper for reuse across tests.
-async fn drain_all(stream: wireframe::FrameStream<TestMsg, ()>) -> Vec<TestMsg> { - stream.try_collect::<Vec<_>>().await.expect("stream error") -} +async fn drain_all<S, E>(stream: S) -> Vec<TestMsg> +where + S: futures::stream::TryStream<Ok = TestMsg, Error = E>, + E: std::fmt::Debug, +{ + use futures::TryStreamExt; + stream.try_collect::<Vec<_>>().await.expect("stream error") +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/response.rs(1 hunks)tests/multi_packet.rs(5 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/world.rstests/multi_packet.rssrc/response.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/world.rstests/multi_packet.rssrc/response.rs
{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write unit and behavioural tests for new functionality
Files:
tests/world.rstests/multi_packet.rssrc/response.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/world.rstests/multi_packet.rs
🧬 Code graph analysis (1)
tests/multi_packet.rs (1)
tests/world.rs (1)
resp(220-223)
⏰ 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)
src/response.rs (1)
116-152: LGTM: Unified streaming API is clean and bounds are correct.The variant coverage and Send + 'static bounds look solid; Vec arm uses explicit Ok typing.
tests/multi_packet.rs (1)
3-3: LGTM: Tests align with into_stream and use error-aware draining.Import of TryStreamExt, try_collect in drain_all, updated doc comment, and into_stream usage read well.
Also applies to: 10-13, 15-16, 25-25, 35-35, 46-46, 60-60
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai pause |
|
@sourcery-ai review |
✅ Actions performedReviews paused. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `tests/multi_packet.rs` </location>
<code_context>
+async fn multi_packet_handles_channel_capacity() {
</code_context>
<issue_to_address>
Consider adding a test for empty Response::Vec and Response::Empty variants.
Please add tests for Response::Vec with an empty vector and Response::Empty to ensure into_stream returns an empty stream for these cases.
</issue_to_address>
### Comment 2
<location> `src/response.rs:139` </location>
<code_context>
+ /// # }
+ /// ```
+ #[must_use]
+ pub fn into_stream(self) -> FrameStream<F, E> {
+ match self {
+ Response::Single(frame) => Box::pin(futures::stream::once(async move {
</code_context>
<issue_to_address>
Consider refactoring the method to box the stream only once and use standard stream adapters for clarity and efficiency.
Here’s a trimmed-down version that
• boxes only once,
• uses `tokio_stream::wrappers::ReceiverStream` for `MultiPacket`,
• and leverages common adapters (`map(Ok)`, `stream::once`, `stream::iter`):
```rust
use tokio_stream::wrappers::ReceiverStream;
use futures::{stream, StreamExt, TryStreamExt};
impl<F: Send + 'static, E: Send + 'static> Response<F, E> {
pub fn into_stream(self) -> FrameStream<F, E> {
let s = match self {
Response::Single(f) => stream::once(async move { Ok::<F, WireframeError<E>>(f) }),
Response::Vec(frames) => stream::iter(frames.into_iter().map(Ok)),
Response::Stream(s) => s,
Response::MultiPacket(rx) => ReceiverStream::new(rx).map(Ok),
Response::Empty => stream::empty(),
};
Box::pin(s)
}
}
```
Steps to apply:
1. Import:
```rust
use tokio_stream::wrappers::ReceiverStream;
use futures::{stream, StreamExt, TryStreamExt};
```
2. Move the `Box::pin(...)` call to wrap the entire `match` result instead of each arm.
3. Replace the manual `unfold` on the channel with `ReceiverStream::new(rx).map(Ok)`.
4. Use `stream::once` and `stream::iter(... .map(Ok))` for `Single` / `Vec`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
leynos
deleted the
codex/add-unified-stream-interface-for-multipacket-responses
branch
1 participant
Summary
into_streamhelper to convert responses into frame streamsTesting
make fmtmake lintmake testhttps://chatgpt.com/codex/tasks/task_e_68bb302ff8488322b72730cfd6402ed7
Summary by Sourcery
Introduce a unified streaming interface for all Response variants by adding
into_stream, migrate multi-packet code and tests to use it, and update documentation to describe the new approachNew Features:
Response::into_streammethod to convert any Response variant into aFrameStreamEnhancements:
into_streaminstead of manual channel matchingDocumentation:
into_streamapproach for multi-packet and streaming responses in the design guideTests:
into_streamwithtry_collectand simplify thedrain_allhelper