Conversation
Reviewer's GuideThis PR adds a type-keyed shared state container to WireframeApp via a new app_data builder, carries registered state into MessageRequest with a state() accessor, implements a SharedState extractor (with ExtractError for missing state), updates documentation/examples to reflect the new API, and adds unit tests for extraction success and error cases. 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 WalkthroughThis update replaces the shared application state storage from a vector to a type-keyed hash map in both the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WireframeApp
participant MessageRequest
participant Extractor (SharedState)
User->>WireframeApp: app_data(state)
WireframeApp->>WireframeApp: Store state in app_data (HashMap keyed by type)
User->>WireframeApp: Build and run app
WireframeApp->>MessageRequest: Create request with app_data
Extractor (SharedState)->>MessageRequest: Extract SharedState<T>
alt State found
MessageRequest-->>Extractor (SharedState): Return SharedState<T>
else State missing
MessageRequest-->>Extractor (SharedState): Return ExtractError::MissingState
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/src/extractor.rs is usually changed with: wireframe/src/middleware.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[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 they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/extractor.rs:28` </location>
<code_context>
+ T: Send + Sync + 'static,
+ {
+ let data: SharedState<T> = state.into();
+ self.app_data
+ .push(Arc::new(data) as Arc<dyn Any + Send + Sync>);
+ Ok(self)
</code_context>
<issue_to_address>
Potential for subtle bugs if multiple SharedState<T> are registered.
Only the first registered SharedState<T> is returned, which may be unexpected. Please document this behavior clearly or enforce that only one instance can be registered.
</issue_to_address>
### Comment 2
<location> `src/extractor.rs:23` </location>
<code_context>
+
+impl MessageRequest {
+ /// Retrieve shared state of type `T` if available.
+ #[must_use]
+ pub fn state<T>(&self) -> Option<SharedState<T>>
+ where
</code_context>
<issue_to_address>
Function attribute #[must_use] is placed before the doc comment, but it should be after.
Please move the #[must_use] attribute to after the function's doc comment to comply with the style guide.
</issue_to_address>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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
README.md(3 hunks)docs/rust-binary-router-library-design.md(2 hunks)src/app.rs(7 hunks)src/extractor.rs(4 hunks)tests/app_data.rs(1 hunks)
🔇 Additional comments (14)
README.md (3)
16-16: Documentation improvement.Good fix for the markdown escaping issue that could have caused rendering problems.
36-36: API simplification reflected correctly.The removal of
.into()conversion suggests the app_data method now accepts the state directly, which simplifies the API.
49-50: Clear documentation of new functionality.The added explanation of
app_datafunctionality provides users with a clear understanding of how to store and retrieve shared state using theSharedState<T>extractor.tests/app_data.rs (1)
1-31: Comprehensive test coverage for shared state extraction.The tests effectively cover both the success case (extracting existing shared state) and the failure case (missing state returns appropriate error). The test setup using
MessageRequestwith manually constructedapp_dataensures the extraction logic is properly validated.docs/rust-binary-router-library-design.md (2)
629-629: Consistent with API changes.The removal of
.into()conversion aligns with the simplifiedapp_datamethod signature.
791-793: Accurate description of MessageRequest enhancement.The updated description correctly explains that
MessageRequestnow encapsulates both connection metadata and shared state registered viaWireframeApp::app_data.src/app.rs (4)
7-7: Appropriate imports for shared state functionality.The added imports for
std::any::Anyandextractor::SharedStateare necessary for the type-erased storage and shared state wrapper.Also applies to: 13-13
34-34: Well-designed shared state storage.Using
Vec<Arc<dyn Any + Send + Sync>>for type-erased storage is a sound approach that allows multiple different types to be stored whilst maintaining thread safety.
155-172: Clean API for registering shared state.The
app_datamethod provides a ergonomic way to register shared state. Converting toSharedState<T>before type-erasure ensures consistent storage format. TheResultreturn type provides forward compatibility for potential validation.
215-215: Proper state propagation in builder methods.The
app_datafield is correctly propagated in builder transformation methods, ensuring shared state is preserved when changing serializers or connection types.Also applies to: 261-261
src/extractor.rs (4)
17-19: Appropriate extension of MessageRequest.Adding the
app_datafield allows shared state to be accessed during request processing whilst maintaining type safety through the type-erased storage.
75-77: Manual Clone implementation necessary.The manual
Cloneimplementation is required sinceSharedState<T>contains anArc<T>and automatic derivation would requireT: Clone, which is not needed for reference-counted data.
119-134: Well-designed error type for extraction failures.The
ExtractErrorenum withMissingStatevariant provides clear error reporting when shared state is not found. The error message includes the type name for debugging.
136-149: Correct FromMessageRequest implementation.The implementation properly extracts shared state using the
state<T>method and returns an appropriate error when the state is missing. Usingstd::any::type_name::<T>()in the error provides helpful debugging information.
|
@sourcery-ai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Absence of Expected Change Pattern
- wireframe/src/extractor.rs is usually changed with: wireframe/src/middleware.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 - here's some feedback:
- The
app_databuilder method returns aResultbut never actually fails; consider simplifying its signature to returnSelfdirectly until you introduce real error conditions. - Keying shared state by
TypeId::of::<SharedState<T>>()is a bit confusing—it's more idiomatic to key byTypeId::of::<T>()and storeArc<T>directly to avoid the extra wrapper and streamline lookups. - Your tests use
MessageRequest::default()but noDefaultimpl exists onMessageRequest; either add aDefaultimplementation or update the tests to construct the struct explicitly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `app_data` builder method returns a `Result` but never actually fails; consider simplifying its signature to return `Self` directly until you introduce real error conditions.
- Keying shared state by `TypeId::of::<SharedState<T>>()` is a bit confusing—it's more idiomatic to key by `TypeId::of::<T>()` and store `Arc<T>` directly to avoid the extra wrapper and streamline lookups.
- Your tests use `MessageRequest::default()` but no `Default` impl exists on `MessageRequest`; either add a `Default` implementation or update the tests to construct the struct explicitly.
## Individual Comments
### Comment 1
<location> `src/app.rs:171` </location>
<code_context>
+ ///
+ /// This function currently always succeeds but returns [`Result`] for
+ /// future flexibility.
+ pub fn app_data<T>(mut self, state: T) -> Result<Self>
+ where
+ T: Send + Sync + 'static,
</code_context>
<issue_to_address>
Returning Result<Self> is unnecessary if the function cannot fail.
Since app_data always returns Ok(self), returning Self directly would simplify the API unless you expect this method to become fallible soon.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
///
/// # Errors
///
/// This function currently always succeeds but returns [`Result`] for
/// future flexibility.
pub fn app_data<T>(mut self, state: T) -> Result<Self>
where
T: Send + Sync + 'static,
{
let data: SharedState<T> = state.into();
self.app_data.insert(
TypeId::of::<SharedState<T>>(),
Arc::new(data) as Arc<dyn Any + Send + Sync>,
);
Ok(self)
}
=======
///
/// Store a shared state value accessible to request extractors.
///
/// The value can later be retrieved using [`SharedState<T>`]. Multiple
/// calls may register different types.
pub fn app_data<T>(mut self, state: T) -> Self
where
T: Send + Sync + 'static,
{
let data: SharedState<T> = state.into();
self.app_data.insert(
TypeId::of::<SharedState<T>>(),
Arc::new(data) as Arc<dyn Any + Send + Sync>,
);
self
}
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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> `src/extractor.rs:83` </location>
<code_context>
/// Shared application state accessible to handlers.
-#[derive(Clone)]
pub struct SharedState<T: Send + Sync>(Arc<T>);
+impl<T: Send + Sync> Clone for SharedState<T> {
</code_context>
<issue_to_address>
Manual Clone implementation is redundant with #[derive(Clone)].
Since Arc<T> already implements Clone, using #[derive(Clone)] is sufficient and avoids unnecessary manual code.
</issue_to_address>
### Comment 2
<location> `src/extractor.rs:131` </location>
<code_context>
+/// Errors that can occur when extracting built-in types.
+#[derive(Debug)]
+pub enum ExtractError {
+ /// No shared state of the requested type was found.
+ MissingState(&'static str),
</code_context>
<issue_to_address>
ExtractError is currently limited to MissingState; consider future extensibility.
Consider adding a non-exhaustive attribute or documentation to make it easier to add new error variants later.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
/// Errors that can occur when extracting built-in types.
#[derive(Debug)]
pub enum ExtractError {
=======
/// Errors that can occur when extracting built-in types.
///
/// Note: This enum is marked as non_exhaustive; more variants may be added in the future.
#[non_exhaustive]
#[derive(Debug)]
pub enum ExtractError {
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `docs/rust-binary-router-library-design.md:807` </location>
<code_context>
+ Rust
- ````rustrust
- async fn handle_user_update(update: Message<UserUpdateData>) -> Result<()> {
- // update.into_inner() gives UserUpdateData
- //...
</code_context>
<issue_to_address>
The comment in this code example uses 'you', which should be avoided per the style guide.
The comment '// update.into_inner() gives UserUpdateData' uses 'you'. Consider rephrasing to avoid 2nd person, e.g., '// update.into_inner() returns the UserUpdateData instance'.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- Consider keying
app_databyTypeId::of::<T>()instead ofTypeId::of::<SharedState<T>>()so that handlers can lookupSharedState<T>by the inner type directly and avoid confusion. - Since
app_data()never fails, you could simplify its signature to returnSelfinstead ofResult<Self>for a cleaner builder chain. - Tests call
MessageRequest::default()butMessageRequestdoesn’t implementDefault; either add aDefaultimpl or update the tests to construct it explicitly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider keying `app_data` by `TypeId::of::<T>()` instead of `TypeId::of::<SharedState<T>>()` so that handlers can lookup `SharedState<T>` by the inner type directly and avoid confusion.
- Since `app_data()` never fails, you could simplify its signature to return `Self` instead of `Result<Self>` for a cleaner builder chain.
- Tests call `MessageRequest::default()` but `MessageRequest` doesn’t implement `Default`; either add a `Default` impl or update the tests to construct it explicitly.
## Individual Comments
### Comment 1
<location> `src/extractor.rs:85` </location>
<code_context>
-#[derive(Clone)]
pub struct SharedState<T: Send + Sync>(Arc<T>);
+impl<T: Send + Sync> Clone for SharedState<T> {
+ fn clone(&self) -> Self { Self(self.0.clone()) }
+}
</code_context>
<issue_to_address>
Manual Clone implementation is redundant with #[derive(Clone)].
Deriving Clone is more concise and idiomatic here, as the manual implementation offers no additional benefit.
</issue_to_address>
### Comment 2
<location> `src/extractor.rs:130` </location>
<code_context>
}
+/// Errors that can occur when extracting built-in types.
+#[derive(Debug)]
+pub enum ExtractError {
+ /// No shared state of the requested type was found.
</code_context>
<issue_to_address>
The attribute #[derive(Debug)] is placed before the doc comment; attributes should come after the doc comment.
Please move the doc comment above the #[derive(Debug)] attribute so that the doc comment comes first, followed by the attribute, as per the review instruction.
</issue_to_address>
### Comment 3
<location> `docs/rust-binary-router-library-design.md:807` </location>
<code_context>
+ Rust
- ````rustrust
- async fn handle_user_update(update: Message<UserUpdateData>) -> Result<()> {
- // update.into_inner() gives UserUpdateData
- //...
</code_context>
<issue_to_address>
The comment in this code example uses 'you', which should be avoided per the style guide.
The comment '// update.into_inner() gives UserUpdateData' uses 'you'. Please rephrase to avoid 2nd person, e.g., '// update.into_inner() returns a UserUpdateData instance'.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Absence of Expected Change Pattern
- wireframe/src/extractor.rs is usually changed with: wireframe/src/middleware.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.
Gates Passed
See analysis details in CodeScene
Absence of Expected Change Pattern
- wireframe/src/extractor.rs is usually changed with: wireframe/src/middleware.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.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@sourcery-ai review |
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> `src/extractor.rs:26` </location>
<code_context>
+ ///
+ /// Values are keyed by their [`TypeId`]. Registering additional
+ /// state of the same type will replace the previous entry.
+ pub app_data: HashMap<TypeId, Arc<dyn Any + Send + Sync>>,
+}
+
</code_context>
<issue_to_address>
HashMap<TypeId, ...> may cause issues with multiple types having the same TypeId in different crates.
Since TypeId is only unique within a crate, using it as a key can cause conflicts if the same type exists in multiple crates (e.g., due to re-exports or version mismatches). Consider this if the library is used across crate boundaries.
Suggested implementation:
```rust
/// Shared state values registered with the application.
///
/// Values are keyed by a unique string (such as a type name or user-provided key).
/// Registering additional state with the same key will replace the previous entry.
///
/// This avoids issues with `TypeId` not being globally unique across crates.
pub app_data: HashMap<String, Arc<dyn Any + Send + Sync>>,
```
- All code that inserts or retrieves from `app_data` will need to use a `String` key (e.g., `std::any::type_name::<T>().to_string()` or a user-defined key) instead of `TypeId`.
- Update any helper methods or APIs that interact with `app_data` to accept/require a `String` key.
</issue_to_address>
### Comment 2
<location> `docs/rust-binary-router-library-design.md:812` </location>
<code_context>
- }
+ ````rustrust
+ async fn handle_user_update(update: Message<UserUpdateData>) -> Result<()> {
+ // update.into_inner() returns a `UserUpdateData` instance
+ //...
+ }
</code_context>
<issue_to_address>
The comment uses 'you', which is a 2nd person pronoun, violating the instruction.
Please rephrase the comment to avoid using 'you'. For example: 'update.into_inner() returns a `UserUpdateData` instance.'
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Absence of Expected Change Pattern
- wireframe/src/extractor.rs is usually changed with: wireframe/src/middleware.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.
1 participant
Summary
WireframeAppMessageRequestExtractErrorandSharedStateextractorapp_datausageTesting
cargo clippy -- -D warningsRUSTFLAGS="-D warnings" cargo testhttps://chatgpt.com/codex/tasks/task_e_6852d87f03e883229723421729a86bd5
Summary by Sourcery
Enable registering and extracting shared application state in WireframeApp via a new app_data API and SharedState extractor
New Features:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests