Conversation
BREAKING CHANGE: added config to ws client
WalkthroughAdds per-type and per-item in-memory caching to the worldstate Client, splits listener logic into two modules (single-item and nested-item listeners), tightens Queryable trait bounds, adjusts several item model fields, and bumps MSRV and multiple dependencies. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Cache
participant HTTP as "HTTP Client"
participant Callback
rect rgb(245,249,255)
Note over Client,Cache: Type/item fetch with caching
Client->>Cache: lookup (TypeId, Language) / (Language, query)
alt cache hit
Cache-->>Client: cached result
Client-->>Caller: return cached result
else cache miss
Client->>HTTP: perform network fetch (query / query_with_lang)
HTTP-->>Client: response (OK / 404 / error)
Client->>Cache: store result (respect 404 config)
Client-->>Caller: return result
end
end
rect rgb(250,250,250)
Note over Client,Callback: Listener (call_on_update*) flow
Client->>HTTP: initial fetch T
Client->>Client: compute sleep until expiry
loop running
Client->>Client: sleep (expiry / nested_listener_sleep)
Client->>HTTP: fetch new T / Vec<T>
alt change detected
Client->>Callback: invoke with (old, new) or (state, old, new) / (item, Change)
Callback-->>Client: completed
Client->>Client: update stored state
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🧹 Nitpick comments (2)
src/worldstate/models/base.rs (1)
3-5: Unnecessaryselfimport.The
std::selfimport doesn't appear to serve any purpose. Consider removing it.🔎 Suggested fix
use std::{ - self, fmt::Write, ops::{src/worldstate/client.rs (1)
220-242: LGTM with minor note.The
cached_item()method correctly implements conditional caching based onconfig.cache_404_item_requests(lines 237-239), which is a good design to balance memory usage and network efficiency.Minor observation: Line 229 creates a new
Box<str>allocation for every cache lookup. While this is necessary given theItemCachetype definition, it could be optimized in the future by using&stras the cache key type if memory allocations become a concern.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
Cargo.tomlsrc/lib.rssrc/worldstate/client.rssrc/worldstate/listener.rssrc/worldstate/listener_nested.rssrc/worldstate/mod.rssrc/worldstate/models/base.rssrc/worldstate/models/items/warframe.rssrc/worldstate/models/items/weapon.rswarframe-macros/Cargo.toml
💤 Files with no reviewable changes (1)
- src/worldstate/models/items/weapon.rs
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Mettwasser
Repo: WFCD/warframe.rs PR: 24
File: src/market/client.rs:127-143
Timestamp: 2025-04-17T14:00:48.579Z
Learning: In the Warframe.rs codebase, the caching implementation in the market client is designed to return the direct value type (T) rather than an Arc<T> to maintain consistent API interfaces between cached and non-cached paths, even if it means some cloning overhead.
📚 Learning: 2025-04-05T10:21:42.020Z
Learnt from: Mettwasser
Repo: WFCD/warframe.rs PR: 24
File: src/worldstate/models/global_upgrades.rs:3-4
Timestamp: 2025-04-05T10:21:42.020Z
Learning: In the warframe.rs codebase, the `#[model(..., timed)]` attribute macro automatically adds `activation` and `expiry` fields to structs and implements the `TimedEvent` trait. These fields are not visible in the source code but are added during compilation.
Applied to files:
src/worldstate/models/items/warframe.rsCargo.toml
📚 Learning: 2025-04-17T14:00:48.579Z
Learnt from: Mettwasser
Repo: WFCD/warframe.rs PR: 24
File: src/market/client.rs:127-143
Timestamp: 2025-04-17T14:00:48.579Z
Learning: In the Warframe.rs codebase, the caching implementation in the market client is designed to return the direct value type (T) rather than an Arc<T> to maintain consistent API interfaces between cached and non-cached paths, even if it means some cloning overhead.
Applied to files:
Cargo.toml
🧬 Code graph analysis (3)
src/worldstate/listener_nested.rs (1)
src/worldstate/listener.rs (1)
ignore_state(8-13)
src/worldstate/listener.rs (3)
src/worldstate/listener_nested.rs (1)
ignore_state(10-15)src/market/client.rs (1)
item(141-141)src/worldstate/client.rs (1)
any(139-139)
src/worldstate/client.rs (3)
src/market/client.rs (4)
items(456-482)items(492-497)new(121-125)item(141-141)src/worldstate/models/base.rs (2)
query(80-92)query_with_language(97-110)src/worldstate/models/mod.rs (1)
query(94-96)
🔇 Additional comments (20)
src/lib.rs (1)
1-1: LGTM!The
doc_cfgfeature gate is correctly configured for docs.rs builds, enabling proper documentation of feature-gated items. This aligns well with the corresponding[package.metadata.docs.rs]configuration in Cargo.toml.Cargo.toml (1)
13-15: Docs.rs configuration looks correct.The
all-features = trueensures all feature-gated APIs are documented, and--cfg docsrsworks in conjunction with thecfg_attr(docsrs, ...)insrc/lib.rs.src/worldstate/models/items/warframe.rs (2)
20-20: Breaking change:aurais now optional.This prevents deserialization failures when the aura field is absent in the API response. Note that this is a breaking change for consumers who previously relied on
aurabeing a non-optionalString.
83-85: Breaking change: Wikia fields are now optional.Making
wikia_thumbnailandwikia_urloptional improves resilience against missing API data. This is a breaking change for existing consumers.src/worldstate/mod.rs (1)
33-34: Good modularization of listener logic.Extracting listener functionality into dedicated private modules improves code organization and separation of concerns. The modules extend
Clientthrough impl blocks without altering the public API surface.src/worldstate/models/base.rs (1)
74-76: Breaking change: Stricter trait bounds onQueryable.The added bounds (
Clone + 'staticon the trait,Send + Sync + Clone + 'staticonReturn) are necessary to support the new caching layer and async listener functionality. This is a breaking change for downstream implementations that don't satisfy these bounds.The bounds are appropriate:
Cloneenables cache value returns withoutArcoverhead (consistent with the codebase design per learnings).Send + Syncenables async task usage.'staticenables type-erased storage in the cache.src/worldstate/listener.rs (3)
8-13: LGTM!The
ignore_statehelper cleanly adapts a stateless callback to the stateful signature, avoiding code duplication in the public API methods.
43-49: LGTM!The public API is well-documented with a clear example. The delegation to
call_on_update_innerwith theignore_statewrapper is a clean pattern.
129-131: Verify comparison operator intent.The condition
item.expiry() >= new_item.expiry()continues without invoking the callback when expiries are equal. Confirm this is intentional—if items can have the same expiry but different content, changes might be missed.src/worldstate/listener_nested.rs (3)
10-15:ignore_statehelper is appropriately duplicated.While similar to the helper in
listener.rs, the different callback signature (&T, Changevs&T, &T) justifies having separate implementations. The naming consistency is good.
54-61: LGTM!The public API is well-documented with a clear example demonstrating the
Change::AddedandChange::Removedvariants.
117-165: Solid nested listener implementation.The diff-based approach using
CrossDiffis appropriate for tracking additions and removals inVec<T>types. Unlike the regular listener, this implementation avoids potential panics by not convertingTimeDeltatostd::Duration.One consideration: if
self.fetch::<T>().awaitfails, the error propagates and terminates the listener. You may want to add retry logic or logging for transient failures to improve resilience.warframe-macros/Cargo.toml (1)
19-21: Dependency version bumps look reasonable.The updates to
proc-macro2,quote, andsynare all patch versions to the latest releases, which should be backward compatible.src/worldstate/client.rs (7)
53-54: LGTM!The type aliases are well-defined. Using
TypeIdas part of the cache key forTypeCacheprovides type safety, andBox<str>forItemCacheis memory-efficient for owned strings.
83-88: LGTM!Field visibility is appropriately scoped:
pub(crate)for fields needed by listener modules (http,base_url,config), and private for implementation details (type_cache,items_cache).
99-104: Reasonable cache TTL choices.The cache TTLs are well-chosen: 5 minutes for frequently-changing worldstate data, and 12 hours for more stable item data.
112-126: LGTM!The updated constructor signature provides flexibility for advanced users to supply custom caches and configuration. This is appropriately documented as a breaking change in the PR description.
181-183: LGTM!The refactored
fetch()andfetch_using_lang()methods correctly delegate totype_cached()with appropriate language parameters and fallback functions.Also applies to: 214-218
273-280: LGTM!Both
query_item()andquery_item_using_lang()correctly delegate tocached_item()and properly useurlencoding::encode()for URL-safe query parameter encoding.Also applies to: 313-320
128-152: Verify AsyncFn trait requirement and edition configuration.The
AsyncFntrait bound (line 131) and edition "2024" setting indicate experimental or future Rust features. Confirm these are supported in your target Rust version and that the project builds successfully.The cloning pattern (lines 139, 148) intentionally returns
T::Returndirectly rather thanArc<T::Return>to maintain consistent API interfaces between cached and non-cached code paths. This is the intended design.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
README.md (2)
40-40: Fix grammar: "are linting" → "are linted".Line 40 uses present progressive tense incorrectly. The statement should use passive voice: "Commit messages are linted in the PR."
🔎 Proposed fix
-Commit messages are linting in the PR +Commit messages are linted in the PR
15-32: Fix grammar on line 40: change "are linting" to "are linted".The example code imports are all correctly exported from the public API. However, line 40 contains a grammar error in the commitlint section: "Commit messages are linting in the PR" should use the passive voice "are linted" instead of the present progressive "are linting".
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-04-05T10:21:42.020Z
Learnt from: Mettwasser
Repo: WFCD/warframe.rs PR: 24
File: src/worldstate/models/global_upgrades.rs:3-4
Timestamp: 2025-04-05T10:21:42.020Z
Learning: In the warframe.rs codebase, the `#[model(..., timed)]` attribute macro automatically adds `activation` and `expiry` fields to structs and implements the `TimedEvent` trait. These fields are not visible in the source code but are added during compilation.
Applied to files:
README.md
📚 Learning: 2025-04-17T14:00:48.579Z
Learnt from: Mettwasser
Repo: WFCD/warframe.rs PR: 24
File: src/market/client.rs:127-143
Timestamp: 2025-04-17T14:00:48.579Z
Learning: In the Warframe.rs codebase, the caching implementation in the market client is designed to return the direct value type (T) rather than an Arc<T> to maintain consistent API interfaces between cached and non-cached paths, even if it means some cloning overhead.
Applied to files:
README.md
🔇 Additional comments (1)
README.md (1)
8-11: Documentation improvements look good.The installation instruction, MSRV note update to 1.91, and readability enhancements align well with the PR's dependency upgrades and are clearly presented.
BREAKING CHANGE: call_on_update now returns a new error type
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/worldstate/listener.rs (2)
58-64: Consider removing unnecessary clippy allow attributes.The
#[allow(clippy::missing_errors_doc)]may be unnecessary since the method signature clearly returnsResult<(), ListenerError>with proper error handling. Themissing_panics_docsuppression also seems unneeded given the error handling at lines 156-158 prevents panics.🔎 Suggested change
- #[allow(clippy::missing_panics_doc, clippy::missing_errors_doc)] pub async fn call_on_update<T, Callback>(&self, callback: Callback) -> Result<(), ListenerError>
104-115: Consider removing unnecessary clippy allow attributes.Same as with
call_on_update, these allow attributes may be unnecessary given the proper error handling in place.🔎 Suggested change
- #[allow(clippy::missing_panics_doc, clippy::missing_errors_doc)] pub async fn call_on_update_with_state<S, T, Callback>(
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/worldstate/listener.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-29T16:49:08.409Z
Learnt from: Mettwasser
Repo: WFCD/warframe.rs PR: 31
File: src/worldstate/client.rs:46-47
Timestamp: 2025-12-29T16:49:08.409Z
Learning: When reviewing Rust code, note that std::time::Duration provides from_mins and from_hours since Rust 1.53.0. Do not flag these as non-existent or as custom extensions; treat them as standard library constructors and verify usage accordingly. This applies to all Rust source files, including src/worldstate/client.rs.
Applied to files:
src/worldstate/listener.rs
🧬 Code graph analysis (1)
src/worldstate/listener.rs (1)
src/worldstate/listener_nested.rs (1)
ignore_state(10-15)
🔇 Additional comments (4)
src/worldstate/listener.rs (4)
10-15: LGTM!The
ignore_statehelper correctly adapts a non-stateful callback to the stateful signature required by the inner implementation.
17-29: LGTM!The error type is well-designed with clear documentation. Good defensive programming to handle the
FailedToConvertTimecase even though it's theoretically impossible.
154-158: LGTM: Proper error handling for time conversion.The time conversion is now properly handled with error propagation instead of unwrapping, addressing potential issues when
chrono_time_to_sleepcould be negative. This is good defensive programming.
134-152: LGTM: Buffer sleep provides rate limiting.The logic correctly handles the case where items have expired or updated. The buffer sleep at line 143 (
self.config.listener_sleep_timeout) provides rate limiting to prevent tight loops if the API returns expired items repeatedly.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/worldstate/listener.rs (2)
54-54: Fix documentation typo (previously flagged).The comment has an extra 's': "don't forget to start it as a bg task (or .await it)s" should be "don't forget to start it as a bg task (or .await it)".
🔎 Suggested fix
- /// client.call_on_update(on_cetus_update); // don't forget to start it as a bg task (or .await it)s + /// client.call_on_update(on_cetus_update); // don't forget to start it as a bg task (or .await it)
96-96: Replace unprofessional content in documentation example (previously flagged).The example code contains inappropriate content that should be replaced with professional placeholder text.
🔎 Suggested fix
- /// _s: "My ginormous ass".into(), + /// _s: "Example string".into(),
🧹 Nitpick comments (2)
src/worldstate/listener.rs (2)
17-29: Error handling is sound, minor documentation note.The error variants are well-defined. Note that while the comment states
FailedToConvertTime"should in theory never happen," it can occur in edge cases like clock skew, very short-lived items, or if the initial fetch returns an already-expired item. The error handling is correct and defensive.
31-65: Consider showing complete example usage.The examples at lines 54 and 100 show method calls without demonstrating how to actually spawn them as background tasks or await them. While the comments do mention this requirement, showing a complete example would be clearer:
// Either spawn as background task: tokio::spawn(client.call_on_update(on_cetus_update)); // Or await in an async context: client.call_on_update(on_cetus_update).await?;Also applies to: 67-116
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/worldstate/listener.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-29T16:49:08.409Z
Learnt from: Mettwasser
Repo: WFCD/warframe.rs PR: 31
File: src/worldstate/client.rs:46-47
Timestamp: 2025-12-29T16:49:08.409Z
Learning: When reviewing Rust code, note that std::time::Duration provides from_mins and from_hours since Rust 1.53.0. Do not flag these as non-existent or as custom extensions; treat them as standard library constructors and verify usage accordingly. This applies to all Rust source files, including src/worldstate/client.rs.
Applied to files:
src/worldstate/listener.rs
🧬 Code graph analysis (1)
src/worldstate/listener.rs (1)
src/worldstate/listener_nested.rs (1)
ignore_state(10-15)
🔇 Additional comments (2)
src/worldstate/listener.rs (2)
10-15: LGTM: Clean adapter pattern.The
ignore_statehelper correctly adapts stateless callbacks to the stateful inner implementation by ignoring the unit parameter.
118-168: LGTM: Core listener logic is well-structured and handles edge cases appropriately.The implementation correctly handles the listener lifecycle:
- Buffer sleep (line 143) prevents API hammering after expiry
- The retry loop (line 148) handles cases where the API hasn't updated yet
- Proper error handling for time conversion (lines 156-158) catches edge cases like clock skew or very short-lived items
- Debug logging provides good observability
The previous concern about panic on line 144 has been addressed—the code now uses proper error propagation at lines 156-158 rather than
unwrap().
CraigEge
left a comment
CraigEge
left a comment
There was a problem hiding this comment.
LGTM after recent fixes
|
🎉 This PR is included in version 9.0.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
3 participants
What did you fix?
Some changes regarding the changes to worldstate.
Reproduction steps
Evidence/screenshot/link to line
Considerations
Summary by CodeRabbit
New Features
Improvements
Breaking Changes
✏️ Tip: You can customize this high-level summary in your review settings.