Fix generics and frame handling#113
Conversation
Reviewer's GuideIntroduce a third generic parameter E to WireframeApp and tighten serializer bounds, refactor frame handling to delegate envelope parsing, and update test helpers to accept the new generics. Class diagram for updated WireframeApp generics and boundsclassDiagram
class WireframeApp~S, C, E~ {
}
WireframeApp : +parse_envelope(frame)
WireframeApp : +handle_frame(...)
S : Serializer
S : FrameMetadata<Frame = Envelope>
S : Send
S : Sync
C : Send
C : 'static
E : Packet
WireframeApp ..|> S : uses
WireframeApp ..|> C : uses
WireframeApp ..|> E : uses
Class diagram for updated test helpers with new genericsclassDiagram
class TestHelper~S, C, E~ {
}
S : Serializer
S : FrameMetadata<Frame = Envelope>
S : Send
S : Sync
C : Send
C : 'static
E : Packet
TestHelper ..|> S : uses
TestHelper ..|> C : uses
TestHelper ..|> E : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes generalise the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WireframeApp
participant Router
Client->>WireframeApp: Send frame
WireframeApp->>WireframeApp: parse_envelope(frame)
alt Envelope parsed successfully
WireframeApp->>Router: Route envelope
Router-->>WireframeApp: Response
WireframeApp-->>Client: Send response
else Deserialization fails
WireframeApp-->>Client: Send error
end
Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate Unit Tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Absence of Expected Change Pattern
- wireframe/tests/util.rs is usually changed with: wireframe/tests/routes.rs
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
|
@sourcery-ai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Do not remove envelope handling logic without providing an equivalent replacement or justification. (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/app.rs:594` </location>
<code_context>
{
let (env, _) = match self.parse_envelope(frame) {
- Ok((env, _)) => {
+ Ok(result) => {
*deser_failures = 0;
- let (id, bytes) = env.into_parts();
</code_context>
<issue_to_address>
Do not remove envelope handling logic without providing an equivalent replacement or justification.
The previous logic handled extracting the id and bytes from the envelope, routing the request, and sending a response or logging errors. The new code simply returns 'result' without any of this handling. This is a significant change in behavior and should be justified or replaced with equivalent logic to avoid breaking functionality.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| { | ||
| let (env, _) = match self.parse_envelope(frame) { | ||
| Ok((env, _)) => { | ||
| Ok(result) => { |
There was a problem hiding this comment.
issue (review_instructions): Do not remove envelope handling logic without providing an equivalent replacement or justification.
The previous logic handled extracting the id and bytes from the envelope, routing the request, and sending a response or logging errors. The new code simply returns 'result' without any of this handling. This is a significant change in behavior and should be justified or replaced with equivalent logic to avoid breaking functionality.
Review instructions:
Path patterns: **/*
Instructions:
Create code-review comments for ALL issues. Avoid making general observations or non-specific feedback if at all possible.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@sourcery-ai review |
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Absence of Expected Change Pattern
- wireframe/tests/util.rs is usually changed with: wireframe/tests/routes.rs
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `tests/util.rs:45` </location>
<code_context>
/// Returns any I/O errors encountered while interacting with the in-memory
/// duplex stream.
-pub async fn run_app_with_frame<S>(app: WireframeApp<S>, frame: Vec<u8>) -> io::Result<Vec<u8>>
+pub async fn run_app_with_frame<S, C, E>(
+ app: WireframeApp<S, C, E>,
+ frame: Vec<u8>,
+) -> io::Result<Vec<u8>>
where
</code_context>
<issue_to_address>
Consider adding or updating tests that use these helpers to exercise the new generic parameters.
Ensure there are tests using the helpers with various C and E types to validate the generic logic and constraints.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| pub async fn run_app_with_frame<S, C, E>( | ||
| app: WireframeApp<S, C, E>, | ||
| frame: Vec<u8>, |
There was a problem hiding this comment.
suggestion (testing): Consider adding or updating tests that use these helpers to exercise the new generic parameters.
Ensure there are tests using the helpers with various C and E types to validate the generic logic and constraints.
leynos
merged commit 05e59bf
into
codex/define-framemetadata-trait-and-update-functionality
* Add metadata routing example * Add diagrams to FrameMetadata section * Clarify flag byte handling in example * Refine metadata parsing helper * Improve metadata demo and test fallback * Add test helper for metadata app * Reformat * Fix generics and frame handling (#113) * Fix generics and handler frame logic * Explain envelope handling
1 participant
Summary
WireframeApphandle_framelogicTesting
make fmtmake lintmake testhttps://chatgpt.com/codex/tasks/task_e_68588bc2d3888322a21d8d7a5016da52
Summary by Sourcery
Introduce a new Packet generic type to WireframeApp and tighten type bounds, refactor frame handling by separating parsing and routing logic, and update test utilities to support the new generics.
Enhancements:
Tests:
Summary by CodeRabbit