Implement WireframeProtocol trait integration#151
Conversation
Reviewer's GuideThis PR integrates the new WireframeProtocol trait and per-connection context into the core API: it defines ConnectionContext and the protocol trait, stitches protocol hooks into WireframeApp and ConnectionActor, and updates tests and documentation to verify context-aware callbacks. Class diagram for WireframeProtocol trait integrationclassDiagram
class WireframeProtocol {
<<trait>>
+Frame: FrameLike
+ProtocolError
+on_connection_setup(PushHandle<Frame>, &mut ConnectionContext)
+before_send(&mut Frame, &mut ConnectionContext)
+on_command_end(&mut ConnectionContext)
}
class ProtocolHooks {
-on_connection_setup: Option<OnConnectionSetupHook<F>>
-before_send: Option<BeforeSendHook<F>>
-on_command_end: Option<OnCommandEndHook>
+on_connection_setup(&mut self, PushHandle<F>, &mut ConnectionContext)
+before_send(&mut self, &mut F, &mut ConnectionContext)
+on_command_end(&mut self, &mut ConnectionContext)
+from_protocol(protocol: Arc<P>)
}
class ConnectionContext {
<<struct>>
}
class WireframeApp {
-protocol: Option<Arc<dyn WireframeProtocol<Frame=Vec<u8>, ProtocolError=()>>>
+with_protocol(protocol)
+protocol()
+protocol_hooks()
}
class ConnectionActor {
-hooks: ProtocolHooks<F>
-ctx: ConnectionContext
}
WireframeApp --> "1" WireframeProtocol : uses
WireframeApp --> "1" ProtocolHooks : creates
ProtocolHooks --> "1" WireframeProtocol : from_protocol
ConnectionActor --> "1" ProtocolHooks : uses
ConnectionActor --> "1" ConnectionContext : owns
ProtocolHooks --> "1" ConnectionContext : passes to hooks
WireframeProtocol --> "1" ConnectionContext : uses
WireframeProtocol --> "1" PushHandle : uses
WireframeProtocol <|.. ProtocolHooks : implemented by
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughThis update introduces a formal Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WireframeApp
participant Protocol (WireframeProtocol)
participant ConnectionActor
participant ConnectionContext
User->>WireframeApp: with_protocol(Protocol)
WireframeApp->>WireframeApp: Store Arc<Protocol>
User->>WireframeApp: protocol_hooks()
WireframeApp->>Protocol: Create ProtocolHooks from Protocol
User->>ConnectionActor: Construct with ProtocolHooks
ConnectionActor->>ConnectionContext: Initialise context
ConnectionActor->>ProtocolHooks: before_send(frame, ctx)
ProtocolHooks->>Protocol: before_send(frame, ctx)
Protocol->>ConnectionContext: Mutate context as needed
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
🧰 Additional context used📓 Path-based instructions (8)`docs/**/*.md`: Use the markdown files within the `docs/` directory as a knowled...
📄 Source: CodeRabbit Inference Engine (AGENTS.md) List of files the instruction was applied to:
`**/*.md`: Validate Markdown files using `markdownlint *.md **/*.md`. Run `mdfor...
📄 Source: CodeRabbit Inference Engine (AGENTS.md) List of files the instruction was applied to:
`docs/*`: Each file in the docs/ directory provides design, roadmap, testing, op...
📄 Source: CodeRabbit Inference Engine (docs/contents.md) List of files the instruction was applied to:
`docs/**/*.md`: Use British English spelling based on the Oxford English Diction...
📄 Source: CodeRabbit Inference Engine (docs/documentation-style-guide.md) List of files the instruction was applied to:
`docs/**/*`: Write official documentation for new features, including separate g...
📄 Source: CodeRabbit Inference Engine (docs/wireframe-1-0-detailed-development-roadmap.md) List of files the instruction was applied to:
`**/*.md`: * Avoid 2nd person or 1st person pronouns ("I", "you", "we") * Use en...
⚙️ Source: CodeRabbit Configuration File List of files the instruction was applied to:
`**/*.rs`: Every Rust module must begin with a module level (`//!`) comment expl...
📄 Source: CodeRabbit Inference Engine (AGENTS.md) List of files the instruction was applied to:
`**/*.rs`: * Seek to keep the cyclomatic complexity of functions no more than 12...
⚙️ Source: CodeRabbit Configuration File List of files the instruction was applied to:
🧠 Learnings (1)📓 Common learnings🧬 Code Graph Analysis (2)tests/connection_actor.rs (2)
tests/wireframe_protocol.rs (5)
🔇 Additional comments (25)
✨ Finishing Touches
🧪 Generate Unit Tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- ProtocolHooks::from_protocol only wires up before_send and on_command_end—
on_connection_setupis never registered or invoked, so you’ll need to add that hook when you create the ConnectionActor. - with_protocol is hard-coded to Frame = Vec and ProtocolError = (); if you want to support other frame types or errors, consider making WireframeApp generic over those types instead of fixing them in the API.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- ProtocolHooks::from_protocol only wires up before_send and on_command_end—`on_connection_setup` is never registered or invoked, so you’ll need to add that hook when you create the ConnectionActor.
- with_protocol is hard-coded to Frame = Vec<u8> and ProtocolError = (); if you want to support other frame types or errors, consider making WireframeApp generic over those types instead of fixing them in the API.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
docs/asynchronous-outbound-messaging-design.md(5 hunks)src/app.rs(6 hunks)src/connection.rs(7 hunks)src/hooks.rs(2 hunks)src/lib.rs(1 hunks)tests/connection_actor.rs(3 hunks)tests/wireframe_protocol.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
`**/*.rs`: Every Rust module must begin with a module level (`//!`) comment expl...
**/*.rs: Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public Rust APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Place function attributes after doc comments.
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
src/lib.rssrc/connection.rstests/wireframe_protocol.rssrc/app.rstests/connection_actor.rssrc/hooks.rs
`**/*.rs`: * Seek to keep the cyclomatic complexity of functions no more than 12...
**/*.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 must use en-GB-oxendict 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.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/
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
src/lib.rssrc/connection.rstests/wireframe_protocol.rssrc/app.rstests/connection_actor.rssrc/hooks.rs
`docs/**/*.md`: Use the markdown files within the `docs/` directory as a knowled...
docs/**/*.md: Use the markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
Proactively update the relevant file(s) in thedocs/directory to reflect the latest state when new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve.
Documentation in thedocs/directory must use en-GB-oxendict spelling and grammar, except for the word 'license'.
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
docs/asynchronous-outbound-messaging-design.md
`**/*.md`: Validate Markdown files using `markdownlint *.md **/*.md`. Run `mdfor...
**/*.md: Validate Markdown files usingmarkdownlint *.md **/*.md.
Runmdformat-allafter any documentation changes to format all Markdown files and fix table markup.
Validate Markdown Mermaid diagrams using thenixieCLI by runningnixie *.md **/*.md.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown files must be wrapped at 120 columns.
Tables and headings in Markdown files must not be wrapped.
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
docs/asynchronous-outbound-messaging-design.md
`docs/*`: Each file in the docs/ directory provides design, roadmap, testing, op...
docs/*: Each file in the docs/ directory provides design, roadmap, testing, operations, or reference documentation for the project. Use these files as a reference for understanding the project's architecture, development plans, testing strategies, operational guidelines, and documentation conventions.
📄 Source: CodeRabbit Inference Engine (docs/contents.md)
List of files the instruction was applied to:
docs/asynchronous-outbound-messaging-design.md
`docs/**/*.md`: Use British English spelling based on the Oxford English Diction...
docs/**/*.md: Use British English spelling based on the Oxford English Dictionary, except retain US spelling in API names (e.g., 'color').
Use the Oxford comma in lists.
Write headings in sentence case and use Markdown heading levels in order without skipping.
Follow markdownlint recommendations for Markdown formatting.
Always use fenced code blocks with a language identifier; use 'plaintext' for non-code text.
Use '-' as the first level bullet and renumber lists when items change.
Prefer inline links using 'text' or angle brackets around the URL.
Expand any uncommon acronym on first use, e.g., Continuous Integration (CI).
Wrap paragraphs at 80 columns, code at 120 columns, and do not wrap tables.
Use footnotes referenced with '[^label]'.
When embedding figures, use '' and provide concise alt text describing the content.
Add a short description before each Mermaid diagram for screen readers.
📄 Source: CodeRabbit Inference Engine (docs/documentation-style-guide.md)
List of files the instruction was applied to:
docs/asynchronous-outbound-messaging-design.md
`docs/**/*`: Write official documentation for new features, including separate g...
docs/**/*: Write official documentation for new features, including separate guides for 'Duplex Messaging & Pushes', 'Streaming Responses', and 'Message Fragmentation', each with runnable examples and explanations of relevant concepts and APIs.
📄 Source: CodeRabbit Inference Engine (docs/wireframe-1-0-detailed-development-roadmap.md)
List of files the instruction was applied to:
docs/asynchronous-outbound-messaging-design.md
`**/*.md`: * Avoid 2nd person or 1st person pronouns ("I", "you", "we") * Use en...
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-oxendic spelling and grammar.
- Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
- Code blocks should be wrapped to 120 columns.
- Headings must not be wrapped.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
docs/asynchronous-outbound-messaging-design.md
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: leynos/wireframe#0
File: docs/asynchronous-outbound-messaging-design.md:0-0
Timestamp: 2025-06-28T09:22:07.295Z
Learning: All protocol-specific logic and callbacks should be encapsulated in a single WireframeProtocol trait, which defines hooks for connection setup, frame mutation before send, and command completion. This trait-based approach is preferred over passing individual closures for extensibility and clarity.
🧬 Code Graph Analysis (1)
tests/wireframe_protocol.rs (5)
src/hooks.rs (5)
on_connection_setup(26-26)before_send(29-29)before_send(60-64)on_command_end(32-32)on_command_end(67-71)src/app.rs (4)
on_connection_setup(351-368)new(197-197)new(252-252)protocol(400-404)src/connection.rs (3)
new(86-92)new(387-396)with_hooks(96-113)tests/connection_actor.rs (1)
queues(18-18)src/push.rs (1)
bounded(131-145)
🪛 LanguageTool
docs/asynchronous-outbound-messaging-design.md
[uncategorized] ~73-~73: Loose punctuation mark.
Context: ...low_priority_push_rx: mpsc::Receiver`: For standard, non-urgent background ...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-test
🔇 Additional comments (20)
src/lib.rs (1)
23-23: LGTM: Appropriate API exports for the new protocol system.The addition of
ConnectionContextandWireframeProtocolto the public exports correctly exposes the new protocol trait and context APIs introduced in this PR.tests/connection_actor.rs (3)
181-181: LGTM: Import addition for updated hook signatures.Correctly adds
ConnectionContextimport to support the updated hook signatures.
195-195: LGTM: Hook signature updated for context parameter.The
before_sendhook closure correctly accepts the newConnectionContextparameter while preserving the existing test logic.
219-221: LGTM: Hook signature updated for context parameter.The
on_command_endhook closure correctly accepts the newConnectionContextparameter while preserving the existing test logic.docs/asynchronous-outbound-messaging-design.md (3)
42-48: LGTM: Table formatting improvements.The table alignment improvements enhance readability of the design requirements.
73-74: LGTM: Corrected polling order numbering.The numbering corrections accurately reflect the actual priority order used in the biased select loop.
Also applies to: 93-100
469-508: Excellent architectural documentation.The new Mermaid class diagram provides valuable visual context for understanding the relationships between
WireframeProtocol,ProtocolHooks,ConnectionContext,WireframeApp, andConnectionActor. This helps developers understand the protocol integration architecture.src/connection.rs (5)
16-16: LGTM: Appropriate import for ConnectionContext.Correctly imports
ConnectionContextto support the new context-aware hooks.
62-62: LGTM: ConnectionContext field addition.The
ctxfield appropriately stores the connection context that will be passed to protocol hooks.
213-213: LGTM: Consistent context parameter passing.All
before_sendhook calls correctly pass the mutable context reference, maintaining consistency across the codebase.Also applies to: 226-226, 279-279, 322-322
328-328: LGTM: Context parameter for command completion.The
on_command_endhook call correctly passes the mutable context reference when a command completes.
108-108: ConnectionContext initialisation pattern confirmed.
- Defined as a unit struct in src/hooks.rs with
#[derive(Default)].- Instantiating via
ConnectionContext(no parameters) is valid and intended.tests/wireframe_protocol.rs (3)
17-38: LGTM: Well-designed test protocol implementation.The
TestProtocolstruct provides a clean implementation for testing protocol integration, using atomic operations for thread-safe counter tracking and demonstrating frame modification in thebefore_sendhook.
40-56: LGTM: Comprehensive protocol hooks integration test.This test effectively verifies that
WireframeAppcorrectly generatesProtocolHooksfrom an installed protocol and that the hooks function properly with context parameters.
58-83: LGTM: End-to-end connection actor integration test.This test provides excellent coverage of the complete integration flow from app builder through connection actor, verifying that protocol hooks are applied to both pushed frames and streaming responses.
src/app.rs (2)
21-21: Import additions are appropriate.The import correctly brings in both
ProtocolHooksandWireframeProtocoltypes needed for the new protocol integration functionality.
84-84: Protocol field addition and propagation are well-implemented.The optional protocol field is properly typed with
Arcfor shared ownership, correctly initialized toNone, and consistently propagated through builder method transformations.Also applies to: 240-240, 366-366, 440-440
src/hooks.rs (3)
3-10: Module documentation and imports are well-structured.The updated documentation clearly describes both
ProtocolHooksandWireframeProtocol. Imports are minimal and necessary for the implementation.
16-33: TheWireframeProtocoltrait is excellently designed.The trait provides a clean abstraction with:
- Flexible associated types for frame and error handling
- Default implementations allowing partial adoption
- Context-aware callbacks for state management
PushHandlein setup for asynchronous messaging supportThis design aligns perfectly with the architectural goal of encapsulating protocol-specific logic.
36-36: Context-aware callback updates are properly implemented.The type aliases and methods correctly incorporate
ConnectionContextparameters, enabling stateful protocol implementations whilst maintaining clean separation of concerns.Also applies to: 39-39, 60-70
|
@sourcery-ai review |
leynos
deleted the
codex/implement-wireframeprotocol-integration-and-tests
branch
1 participant
Summary
WireframeProtocoltrait withConnectionContextProtocolHooksWireframeApp::with_protocolConnectionActorrstestTesting
make lintmake testhttps://chatgpt.com/codex/tasks/task_e_686056f891248322a58e136bfc07c812
Summary by Sourcery
Implement WireframeProtocol trait integration by introducing ConnectionContext and deriving protocol hooks for connection lifecycle events. Expose protocol setup in WireframeApp via with_protocol, update ConnectionActor to accept PushHandle and invoke hooks with context, and reinforce the design with updated tests and documentation.
New Features:
Enhancements:
Documentation:
Tests: