Refactor/contact get relays of type #301
Conversation
WalkthroughA new method, Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Contact
participant Account
Caller->>Contact: get_relays_of_type(relay_type)
alt Relay set is not empty
Contact-->>Caller: Return cloned relay set
else Relay set is empty
Contact->>Account: default_relays()
Account-->>Contact: Default relays
Contact-->>Caller: Return default relays
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/whitenoise/accounts/contacts.rs (2)
73-87: Centralized relay fallback method: solid; add docs to lock semantics inThe implementation looks correct and preserves prior behavior while centralizing fallback logic. Please document the fallback to Account::default_relays and the fact that this returns an owned clone.
Apply this doc comment for clarity:
impl Contact { - pub fn get_relays_of_type(&self, relay_type: RelayType) -> DashSet<RelayUrl> { + /// Returns the relays for the given `RelayType`. + /// + /// Behavior: + /// - If the contact has non-empty relays of that type, returns a cloned DashSet of those relays. + /// - If empty, returns `Account::default_relays()` as a fallback. + /// + /// Note: This returns an owned DashSet because downstream callers expect ownership. + pub fn get_relays_of_type(&self, relay_type: RelayType) -> DashSet<RelayUrl> { let relays = match relay_type { RelayType::Nostr => &self.nip65_relays, RelayType::Inbox => &self.inbox_relays, RelayType::KeyPackage => &self.key_package_relays, }; if relays.is_empty() { Account::default_relays() } else { relays.clone() } } }
720-771: Tests cover all relay types; minor portability nit with DashSet::from_iterGood coverage for the three RelayType variants and equality checks. To avoid relying on an associated constructor that may differ across dashmap versions, prefer explicit set construction for expected values.
Apply these localized changes:
- assert!(Whitenoise::relayurl_dashset_eq( - nip65_relays, - DashSet::from_iter([nip65_relay]) - )); + let mut expected_nip65 = DashSet::new(); + expected_nip65.insert(nip65_relay); + assert!(Whitenoise::relayurl_dashset_eq(nip65_relays, expected_nip65));- assert!(Whitenoise::relayurl_dashset_eq( - inbox_relays, - DashSet::from_iter([inbox_relay]) - )); + let mut expected_inbox = DashSet::new(); + expected_inbox.insert(inbox_relay); + assert!(Whitenoise::relayurl_dashset_eq(inbox_relays, expected_inbox));- assert!(Whitenoise::relayurl_dashset_eq( - key_package_relays, - DashSet::from_iter([key_package_relay]) - )); + let mut expected_key_pkg = DashSet::new(); + expected_key_pkg.insert(key_package_relay); + assert!(Whitenoise::relayurl_dashset_eq(key_package_relays, expected_key_pkg));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/whitenoise/accounts/contacts.rs(2 hunks)src/whitenoise/accounts/groups.rs(5 hunks)
🔇 Additional comments (5)
src/whitenoise/accounts/contacts.rs (1)
774-808: No stray inline fallbacks remainAll active code now consistently uses
get_relays_of_type. The only remaining.is_empty()check is in thesrc/_OLD/OLD_commands/groups/create_group.rslegacy file, which isn’t part of the current codepath—no further changes needed.src/whitenoise/accounts/groups.rs (4)
4-4: Import for RelayType: LGTMNeeded for the new get_relays_of_type calls.
121-129: Inbox relay selection for welcome messages centralizedUsing contact.get_relays_of_type(RelayType::Inbox) here matches prior behavior and the new abstraction.
348-357: Inbox relay selection during add_members refactoredSame centralization as above; behavior preserved. Good simplification.
48-52: Residual ad-hoc key-package fallback remains in mod.rsWe’ve centralized all inline
get_relays_of_type(RelayType::KeyPackage)checks except for one remaining manual empty-check in the active code. Please replace it with the new common relay-selection logic.• File:
src/whitenoise/accounts/mod.rsLine 661- if key_package_relays.is_empty() { - // legacy fallback… - } + // use `contact.get_relays_of_type(RelayType::KeyPackage)` + + // common fetch_key_package_event_from(...) logic hereLikely an incorrect or invalid review comment.
jgmontoya
force-pushed
the
refactor/contact-get-relays-of-type
branch
from
dc01c2d to
4e5fc97
Compare
…method for automatic default relay fallback
jgmontoya
force-pushed
the
refactor/contact-get-relays-of-type
branch
from
4e5fc97 to
11c0856
Compare
Closes #297
Summary by CodeRabbit
Refactor
Tests