Add AppDataStore and byte_order utilities; refactor app data sharing#455
Merged
Add AppDataStore and byte_order utilities; refactor app data sharing#455
Conversation
Contributor
Reviewer's GuideIntroduces a reusable AppDataStore abstraction for type-erased, thread-safe application state and refactors builders, extractors, and codecs to use it and shared byte-order helpers, while documenting builder-pattern conventions. Sequence diagram for request handling using AppDataStoresequenceDiagram
actor Client
participant WireframeAppBuilder
participant AppDataStore
participant WireframeApp
participant MessageRequest
participant Handler
Client->>WireframeAppBuilder: with_state_MyConfig_(config)
WireframeAppBuilder->>AppDataStore: insert_MyConfig_(config)
WireframeAppBuilder-->>Client: WireframeApp instance
Client->>WireframeApp: handle_incoming_frame(frame)
WireframeApp->>MessageRequest: new(app_data_clone, frame)
WireframeApp-->>Handler: MessageRequest passed to handler
Handler->>MessageRequest: shared_state_MyConfig_()
MessageRequest->>AppDataStore: get_MyConfig_()
AppDataStore-->>MessageRequest: Option_Arc_MyConfig_
MessageRequest-->>Handler: Option_SharedState_MyConfig_
Handler-->>Client: response frame
Class diagram for AppDataStore integration with WireframeApp and MessageRequestclassDiagram
class AppDataStore {
+values: HashMap_TypeId_ArcAnySendSync_
+insert_T_(value: T)
+get_T_() Option_ArcT_
}
class WireframeApp_E_S_F_C_ {
+routes: OnceCell_Arc_HashMap_u32_HandlerService_E___
+middleware: Vec_Box_Middleware_E__
+serializer: S
+app_data: AppDataStore
+on_connect: Option_Arc_ConnectionSetup_C__
+on_disconnect: Option_Arc_ConnectionTeardown_C__
+protocol: Option_Arc_WireframeProtocol_Frame_EQ_F::Frame_ProtocolError_EQ____
+fragmenter: Option_Arc_Fragmenter_F::Frame__
+with_state_T_(state: T) WireframeApp_E_S_F_C_
+rebuild_with_params_S2_F2_(self_ref: WireframeApp_E_S_F_C_, serializer: S2, frame: F2, protocol: Option_Arc_WireframeProtocol_Frame_EQ_F2::Frame_ProtocolError_EQ____, fragmenter: Option_Arc_Fragmenter_F2::Frame__) WireframeApp_E_S2_F2_C_
}
class MessageRequest {
+peer_addr: Option_SocketAddr_
+app_data: AppDataStore
+body: Option_StreamingBody_
+shared_state_T_() Option_SharedState_T__
+insert_state_T_(state: T)
+set_body(body: StreamingBody)
}
class SharedState_T_ {
+value: Arc_T_
}
AppDataStore "1" <-- "1" WireframeApp_E_S_F_C_ : owns
AppDataStore "1" <-- "1" MessageRequest : owns
SharedState_T_ "1" --> "1" T : wraps
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughSummarise the refactor: add a concurrent AppDataStore for per-type app state, extend byte-order helpers to u32/u64 and replace local helpers, and standardise builder reconstruction using a RebuildParams struct; update codec, frame conversion, request extractor, tests and documentation accordingly. Changes
Sequence Diagram(s)(Skipped — changes are refactors and API additions that do not create a new multi-actor runtime control flow requiring a sequence diagram.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![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}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
AppDataStoredocs and PR description call it "thread-safe", but the implementation only guarantees safe concurrent reads (mutations still require&mut self); consider clarifying the concurrency model in the docs or adding internal synchronization if concurrent mutation from multiple threads is expected. - If you expect downstream users to need more than insert/get semantics on
AppDataStore, it may be worth adding acontains::<T>()orremove::<T>()API now to avoid them reaching into the underlying map later.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `AppDataStore` docs and PR description call it "thread-safe", but the implementation only guarantees safe concurrent reads (mutations still require `&mut self`); consider clarifying the concurrency model in the docs or adding internal synchronization if concurrent mutation from multiple threads is expected.
- If you expect downstream users to need more than insert/get semantics on `AppDataStore`, it may be worth adding a `contains::<T>()` or `remove::<T>()` API now to avoid them reaching into the underlying map later.
## Individual Comments
### Comment 1
<location> `src/byte_order.rs:132` </location>
<code_context>
+ write_network_u64,
+ };
+
+ #[test]
+ fn u16_round_trip() {
+ let value = 0x1234;
</code_context>
<issue_to_address>
**issue (review_instructions):** The new round-trip tests duplicate the same pattern across u16/u32/u64 without using rstest parameterisation.
The three `*_round_trip` tests share identical structure (construct value, serialise, assert equality, then parse and assert equality). Per the testing instructions, these should be consolidated into a `#[rstest]` with parameters for the type/constructor/value rather than three separate tests.
Example (sketch):
```rust
#[rstest]
#[case::u16(0x1234u16, write_network_u16, read_network_u16, [0x12, 0x34])]
#[case::u32(0x1234_5678u32, write_network_u32, read_network_u32, [0x12, 0x34, 0x56, 0x78])]
#[case::u64(0x1122_3344_5566_7788u64, write_network_u64, read_network_u64, [0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88])]
fn round_trip_cases<...>(...) { ... }
```
That would satisfy the requirement to replace duplicated tests with parameterised rstest cases.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.rs`
**Instructions:**
* Use `rstest` fixtures for shared setup.
* Replace duplicated tests with `#[rstest(...)]` parameterised cases.
</details>
</issue_to_address>
### Comment 2
<location> `src/app_data_store.rs:98` </location>
<code_context>
+ write_network_u64,
+ };
+
+ #[test]
+ fn u16_round_trip() {
+ let value = 0x1234;
</code_context>
<issue_to_address>
**issue (review_instructions):** The new AppDataStore tests share setup but do not use rstest fixtures or parameterised cases.
Several tests (`insert_and_get_multiple_types`, `insert_overwrites_existing_value`, `missing_type_returns_none`) all start from `AppDataStore::default()` and then vary the operations/assertions. The guidelines ask to use `rstest` fixtures for shared setup and parameterise duplicated tests.
Consider introducing a `#[fixture] fn empty_store() -> AppDataStore` and, where appropriate, using `#[rstest]` parameterised cases rather than repeating very similar patterns by hand.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.rs`
**Instructions:**
* Use `rstest` fixtures for shared setup.
* Replace duplicated tests with `#[rstest(...)]` parameterised cases.
</details>
</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}
Contributor
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)
src/frame/conversion.rs (1)
110-127: 🧹 Nitpick | 🔵 TrivialNote:
write_bytes_with_endiannesscould use the network-order write helpers.The read path (line 83) now uses
read_network_u64, but the write path still uses manual byte shifting. For consistency, theEndianness::Bigbranch here could delegate towrite_network_u32/write_network_u64when the size matches. This is not blocking—the current implementation is correct—but raises the question of symmetry.#!/bin/bash # Check if write_network_u32/u64 are used elsewhere in the codebase for encoding rg -n 'write_network_u(32|64)' --type rustsrc/app/builder.rs (1)
127-146:⚠️ Potential issue | 🟠 MajorGroup rebuild parameters and remove the lint expectation.
The
rebuild_with_paramsfunction has 5 parameters exceeding the configured clippy threshold of 4. Wrap the serializer, codec, protocol, fragmentation and message_assembler inputs in aRebuildParamsstruct to bring the function signature to 2 parameters (self + params struct), eliminating the need for#[expect(clippy::too_many_arguments)]. Both call sites at lines 181 and 260–266 use consistent parameter patterns, so refactoring is straightforward.♻️ Suggested refactor sketch
@@ - #[expect( - clippy::too_many_arguments, - reason = "Helper handles multi-field type transitions; see builder-pattern-conventions.md." - )] - fn rebuild_with_params<S2, F2>( - self, - serializer: S2, - codec: F2, - protocol: Option<Arc<dyn WireframeProtocol<Frame = F2::Frame, ProtocolError = ()>>>, - fragmentation: Option<FragmentationConfig>, - message_assembler: Option<Arc<dyn MessageAssembler>>, - ) -> WireframeApp<S2, C, E, F2> + fn rebuild_with_params<S2, F2>( + self, + params: RebuildParams<S2, F2>, + ) -> WireframeApp<S2, C, E, F2> @@ - serializer, + serializer: params.serializer, @@ - codec, + codec: params.codec, @@ - protocol, + protocol: params.protocol, @@ - fragmentation, - message_assembler, + fragmentation: params.fragmentation, + message_assembler: params.message_assembler, } }// Add near the impl block (module scope). struct RebuildParams<S2, F2> { serializer: S2, codec: F2, protocol: Option<Arc<dyn WireframeProtocol<Frame = F2::Frame, ProtocolError = ()>>>, fragmentation: Option<FragmentationConfig>, message_assembler: Option<Arc<dyn MessageAssembler>>, }
🤖 Fix all issues with AI agents
In `@src/app_data_store.rs`:
- Around line 86-143: Create an rstest fixture that yields a fresh AppDataStore
and use it in the tests to remove repeated AppDataStore::default() calls: add a
fixture function (e.g., fn app_data_store() -> AppDataStore) annotated with
#[fixture] in the tests module, then update insert_and_get_multiple_types,
insert_overwrites_existing_value, and missing_type_returns_none to accept that
fixture (e.g., app: AppDataStore) instead of creating a new store locally;
ensure you keep the existing assertions and the store_is_send_and_sync test
unchanged.
In `@src/client/builder.rs`:
- Around line 34-40: Update the comment text to use British English by replacing
the word "centralized" with "centralised" in the docstring that references
WireframeApp::rebuild_with_params and docs/builder-pattern-conventions.md;
ensure the sentence now reads "...keep the reconstruction logic centralised and
easier to audit" so the spelling follows the en-GB-oxendict guideline.
leynos
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/frame/conversion.rs`:
- Around line 147-153: The cast in write_little_endian_prefix triggers Clippy's
cast_possible_truncation even though the value is masked; add a narrowly-scoped
attribute on the assignment line to suppress this lint by inserting
#[expect(clippy::cast_possible_truncation, reason = "mask `& 0xff` ensures value
fits in u8")] immediately above the `*byte = ((value >> shift) & 0xff) as u8;`
statement in fn write_little_endian_prefix so the suppression mirrors the
existing pattern used in write_big_endian_prefix.
Owner
Author
|
@coderabbitai have the following now been resolved? |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 9454 🏁 Script executed: Length of output: 1214
✅ Overall CommentsAppDataStore thread-safety: The implementation now uses API extensions: Both ✅ Individual CommentsComment 1 (byte_order.rs tests): The three separate Comment 2 (app_data_store.rs tests): The tests now use a |
…tate Introduce AppDataStore to manage application-scoped data with type-safe accessors, replacing HashMap<TypeId, Arc<dyn Any + Send + Sync>> usage. Refactor WireframeApp and MessageRequest to use AppDataStore for improved encapsulation and simpler API. Add documentation on builder pattern conventions and update related client and app builders accordingly. Also add network byte order serialization helpers for u32 and u64 to byte_order module and replace ad-hoc conversions in codec and frame conversion code. Add tests for byte order utilities and AppDataStore. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
…ntainment Replaced the underlying HashMap in AppDataStore with a DashMap to support concurrent insertions and retrievals without external locking. Added contains<T> and remove<T> methods to query and delete stored data concurrently. Updated insert and get to be thread-safe by removing mutable requirements. Added tests verifying concurrent inserts, gets, and removals. Also refactored WireframeApp builder to consolidate parameters into a struct. Minor extractor API change to allow insert_state without mutable self. Improved frame/conversion write functions for endianness handling. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
…d tests - Replace multiple `#[test]` functions with `#[rstest]` in `app_data_store.rs` improving test reuse and readability - Add a reusable `empty_store` fixture for simplified test setup in `AppDataStore` tests - Convert byte order tests to parameterized tests using `rstest` in `byte_order.rs` - Minor test code cleanups such as macro lint expectations and consistent use of fixtures - Fix typos and unify test documentation style This refactor improves test quality and maintainability without changing functionality. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
devboxerhub
Bot
force-pushed
the
refactor-app-data-store-furyr1
branch
from
ff81d13 to
84d225c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/codec.rs (1)
198-202:⚠️ Potential issue | 🟠 MajorUse fallible conversion to handle potential truncation of the length header.
The cast
as usizetriggersclippy::cast_possible_truncation(enabled via pedantic linting with-D warnings) and truncates on 16-bit targets. Useusize::try_from()instead, which correctly returnsNonewhen the network-supplied length header value exceeds the platform's maximumusize.Proposed fix
let expected = src .get(..LENGTH_HEADER_SIZE) .and_then(|slice| <[u8; LENGTH_HEADER_SIZE]>::try_from(slice).ok()) - .map(|bytes| read_network_u32(bytes) as usize); + .and_then(|bytes| usize::try_from(read_network_u32(bytes)).ok());
🤖 Fix all issues with AI agents
In `@docs/builder-pattern-conventions.md`:
- Around line 26-27: Update the docs to point to the correct source file for the
macro: replace the path `src/client/builder.rs` with `src/client/builder/mod.rs`
where `builder_field_update!` is referenced (the macro named
`builder_field_update!` in the builder documentation), so the documented path
matches the actual definition in `src/client/builder/mod.rs`.
In `@src/app_data_store.rs`:
- Around line 138-162: Add a module-level doc comment at the top of the tests
module (use an internal `//!` comment) describing the purpose of these tests,
and remove the module-level #[expect(...)] attribute; instead apply the
#[expect(unused_braces, reason = concat!(...))] directly to the test item that
triggers it (the `empty_store` fixture) using concat!() to build the long reason
string rather than a backslash-continued literal; locate the `mod tests { ... }`
block and move the lint attribute to the `fn empty_store() -> AppDataStore`
fixture, replacing the escaped newline reason with concat!() and keeping the
rest of the test contents (including `CustomState` and `AppDataStore`
references) unchanged.
In `@src/byte_order.rs`:
- Around line 121-123: Add a module-level documentation comment for the test
module: update the #[cfg(test)] mod tests block by inserting a top-of-module //!
comment that briefly describes the purpose of the tests (what behaviors or
functions are being validated) and any notes about test utilities used (e.g.,
rstest). Ensure the comment sits immediately before the mod tests declaration so
the module has a proper module-level docstring.
Converted the u32 value read from network byte order to usize safely using try_from to prevent potential truncation or conversion issues. Also added documentation comments to tests, and updated module paths and attributes for clarity and lint handling. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@src/app_data_store.rs`:
- Around line 26-33: Remove the unnecessary `no_run` marker from the doctest
code fences for the AppDataStore examples that exercise insert and get: update
the triple-backtick fence for the doctests associated with the insert(...)
example and the get::<T>() example (the doctests referencing
AppDataStore::insert and AppDataStore::get) so they use ```rust instead of
```rust,no_run, allowing the tests to execute.
…ation Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
devboxerhub Bot
added a commit
that referenced
this pull request
Feb 18, 2026
…455) * feat(app_data_store): add AppDataStore for typed shared application state Introduce AppDataStore to manage application-scoped data with type-safe accessors, replacing HashMap<TypeId, Arc<dyn Any + Send + Sync>> usage. Refactor WireframeApp and MessageRequest to use AppDataStore for improved encapsulation and simpler API. Add documentation on builder pattern conventions and update related client and app builders accordingly. Also add network byte order serialization helpers for u32 and u64 to byte_order module and replace ad-hoc conversions in codec and frame conversion code. Add tests for byte order utilities and AppDataStore. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> * feat(app_data_store): make AppDataStore concurrent and add removal/containment Replaced the underlying HashMap in AppDataStore with a DashMap to support concurrent insertions and retrievals without external locking. Added contains<T> and remove<T> methods to query and delete stored data concurrently. Updated insert and get to be thread-safe by removing mutable requirements. Added tests verifying concurrent inserts, gets, and removals. Also refactored WireframeApp builder to consolidate parameters into a struct. Minor extractor API change to allow insert_state without mutable self. Improved frame/conversion write functions for endianness handling. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> * test(app_data_store,byte_order): add rstest fixtures and parameterized tests - Replace multiple `#[test]` functions with `#[rstest]` in `app_data_store.rs` improving test reuse and readability - Add a reusable `empty_store` fixture for simplified test setup in `AppDataStore` tests - Convert byte order tests to parameterized tests using `rstest` in `byte_order.rs` - Minor test code cleanups such as macro lint expectations and consistent use of fixtures - Fix typos and unify test documentation style This refactor improves test quality and maintainability without changing functionality. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> * fix(codec): fix usize conversion for network u32 read in codec Converted the u32 value read from network byte order to usize safely using try_from to prevent potential truncation or conversion issues. Also added documentation comments to tests, and updated module paths and attributes for clarity and lint handling. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> * docs(app_data_store): remove no_run from Rust code blocks in documentation Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> --------- Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Changes
New
Core Refactors
Documentation
Public API
Testing
Migration / Compatibility
📎 Task: https://www.devboxer.com/task/156ddc30-5dfb-47f9-865c-1eb7d6667f0d