Refactor relay methods #296
Conversation
WalkthroughThis set of changes refactors and unifies relay management across account and group operations, introducing generic methods for adding and removing relays by type. The code now conditionally publishes relay information based on relay presence and enhances fallback logic for relay usage. Test utilities and test cases are updated to use streamlined account creation and relay handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Whitenoise
participant Account
participant Relay
Client->>Whitenoise: add_relay_to_account(pubkey, relay, relay_type)
Whitenoise->>Account: Fetch account by pubkey
Whitenoise->>Account: Add relay to specified relay set
Whitenoise->>Relay: ensure_relays_connected(relay)
Whitenoise->>Account: Save updated account state
Whitenoise->>Relay: publish_account_relay_info(account, prior_relays)
sequenceDiagram
participant Whitenoise
participant Account
participant Relay
Whitenoise->>Account: login(keys)
Account->>Relay: Query for key package event
alt Key package not found
Account->>Relay: Publish key package
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 (3)
🚧 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)
🔇 Additional comments (4)
✨ 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: 2
🧹 Nitpick comments (1)
src/whitenoise/accounts/mod.rs (1)
1106-1140: Consider documenting deprecation if these methods will be removedThese convenience methods provide a cleaner API for relay management. According to the AI summary, these will be removed in a later refactor. If that's the plan, consider adding deprecation warnings or documentation.
If these methods are temporary, add documentation:
+ /// Convenience method to add a Nostr (NIP-65) relay + /// Note: This method may be deprecated in favor of the generic add_relay_to_account method pub async fn add_nip65_relay(&self, account: &Account, relay: RelayUrl) -> Result<()> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/bin/integration_test.rs(3 hunks)src/nostr_manager/mod.rs(2 hunks)src/nostr_manager/query.rs(1 hunks)src/whitenoise/accounts/contacts.rs(2 hunks)src/whitenoise/accounts/groups.rs(6 hunks)src/whitenoise/accounts/mod.rs(5 hunks)src/whitenoise/accounts/relays.rs(4 hunks)src/whitenoise/mod.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/bin/integration_test.rs
📄 CodeRabbit Inference Engine (.cursor/rules/integration-test.mdc)
The integration test in src/bin/integration_test.rs should ALWAYS be run with the
just int-testcommand and not run on its own with different log and data directories.
Files:
src/bin/integration_test.rs
🧠 Learnings (1)
📚 Learning: applies to src/bin/integration_test.rs : the integration test in src/bin/integration_test.rs should ...
Learnt from: CR
PR: parres-hq/whitenoise#0
File: .cursor/rules/integration-test.mdc:0-0
Timestamp: 2025-08-07T12:20:56.734Z
Learning: Applies to src/bin/integration_test.rs : The integration test in src/bin/integration_test.rs should ALWAYS be run with the `just int-test` command and not run on its own with different log and data directories.
Applied to files:
src/bin/integration_test.rs
🧬 Code Graph Analysis (2)
src/nostr_manager/query.rs (1)
src/_OLD/OLD_commands/accounts/fetch_relays_list.rs (1)
fetch_relays_list(27-126)
src/nostr_manager/mod.rs (1)
src/nostr_manager/subscriptions.rs (1)
NostrManager(9-154)
🔇 Additional comments (22)
src/whitenoise/accounts/contacts.rs (2)
531-531: LGTM! Test simplification improves consistency.The replacement of manual account creation with
create_identity()standardizes test setup and ensures tests use the actual production code path for identity creation.
639-639: LGTM! Consistent test improvement.This change aligns with the standardization effort across test modules, using the unified
create_identity()method for test account setup.src/bin/integration_test.rs (3)
270-270: LGTM! WebSocket URL standardization.Removing the trailing slash from the WebSocket URL improves consistency and avoids potential connection issues.
380-381: LGTM! Improves test reliability with proper initialization timing.The added sleep ensures account4 is fully initialized and has published its events before proceeding with contact/member operations. This prevents race conditions in the integration test.
397-404: LGTM! Enhanced test observability.The added tracing logs provide clear runtime feedback on contact addition and group member operations, making the integration test easier to debug and monitor.
src/nostr_manager/query.rs (2)
39-42: LGTM! Efficient query optimization.Adding
.limit(1)to the filter reduces network overhead and memory usage while maintaining correctness, as relays typically return the most recent event when limited.
49-52: LGTM! Simplified event selection logic.The change from finding the maximum created_at timestamp to taking the first event aligns perfectly with the
.limit(1)optimization. Theclone()is appropriate for providing ownership torelay_urls_from_event().src/whitenoise/mod.rs (3)
698-710: LGTM!The refactoring to use
login()method for account creation and the change to useaccount.pubkeyinstead ofkeys.public_keyfor contact addition are both consistent and correct improvements.
887-892: LGTM!The refactoring to use
create_identity()simplifies the test setup and the change to useaccount.pubkeyis consistent with using the account as the primary interface.
916-924: LGTM!The refactoring to use
create_identity()and the updated comments correctly reflect that the expected error behavior changes sincenostr_mlsis now initialized. The switch toaccount.pubkeyis consistent.src/nostr_manager/mod.rs (1)
209-227: Excellent reliability improvement!The addition of
ensure_relays_connectedbefore publishing events is a significant improvement that prevents publish failures due to disconnected relays. This change aligns with the pattern already used in subscription methods and centralizes relay connection management.src/whitenoise/accounts/groups.rs (4)
257-261: Good fallback implementation!Adding fallback to default relays when
key_package_relaysis empty prevents failures and ensures robust key package fetching.
308-332: Excellent fallback and error handling!The refactored relay handling for evolution event publishing is a significant improvement:
- Clear fallback logic from group relays to account relays to prevent failures
- Proper error handling when no relays are available
- Good logging for debugging
- Clean conditional logic that's easy to follow
360-364: Consistent fallback pattern!The fallback to default relays for empty
inbox_relaysis consistent with the other relay fallback logic in this method and ensures welcome messages can be delivered reliably.
450-464: Clean test refactoring!The changes to use
create_mock_whitenoise()andcreate_identity()for account creation significantly simplify the test setup and make it more consistent with the broader refactoring efforts.src/whitenoise/accounts/mod.rs (2)
334-340: LGTM! Good optimization for conditional key package publishingThe change to only publish key packages when they're not found locally is a good optimization that reduces unnecessary network operations.
1088-1104: LGTM! Well-structured test helper methodThe test-only helper method is properly gated with
#[cfg(test)]and provides clean access to relay data for testing purposes.src/whitenoise/accounts/relays.rs (5)
157-200: LGTM! Well-structured unified relay managementThe generic
add_relay_to_accountmethod properly handles all relay types uniformly, maintains consistency between database and in-memory state, and ensures relay connections before publishing.
253-268: LGTM! Clean relay info publishing implementationThe method properly publishes relay lists for all relay types and correctly handles the optional prior_relays parameter for overwriting relay events on removed relays.
325-362: Excellent comprehensive test coverage!The test properly covers all relay types (Nostr, Inbox, KeyPackage) using a helper function to avoid code duplication. Good test structure!
697-700: Good use of #[should_panic] for invalid input testingTesting panic behavior for invalid relay types ensures the API contract is enforced.
702-754: Excellent database consistency verification!This test thoroughly verifies that in-memory and database states remain synchronized after relay operations, which is critical for data integrity.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/whitenoise/accounts/mod.rs (1)
1085-1137: Well-implemented convenience methods with minor optimization opportunity.The relay management convenience methods provide a clean, consistent API. The delegation pattern and naming conventions are excellent.
Consider these minor improvements:
- Parameter optimization: The methods only use
account.pubkey, so they could acceptPublicKeydirectly:-pub async fn add_nip65_relay(&self, account: &Account, relay: RelayUrl) -> Result<()> { - self.add_relay_to_account(account.pubkey, relay, RelayType::Nostr) +pub async fn add_nip65_relay(&self, pubkey: PublicKey, relay: RelayUrl) -> Result<()> { + self.add_relay_to_account(pubkey, relay, RelayType::Nostr)
- Account validation: Consider adding validation to ensure the account belongs to the current user (if not handled in the generic methods).
The current implementation is still perfectly functional as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/whitenoise/accounts/mod.rs(5 hunks)
⏰ 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)
- GitHub Check: check (ubuntu-latest, native)
🔇 Additional comments (3)
src/whitenoise/accounts/mod.rs (3)
641-696: Excellent refactor addressing redundant relay publishing!This implementation successfully addresses the redundant relay publishing issue from previous reviews. The logic correctly:
- Fetches each relay type separately from appropriate sources
- Falls back to defaults when empty and tracks publishing needs
- Publishes relay info only once if any relay type requires it
This is much more efficient than the previous approach of multiple identical calls to
publish_account_relay_info.
674-675: LGTM! Consistent with relay fetching refactor.The Account struct initialization now correctly uses the fetched relay sets instead of always defaulting, which is consistent with the improved relay fetching logic above.
689-695: Perfect implementation of the suggested fix!This conditional publishing logic exactly addresses the concern from previous reviews. By using a single OR condition to check all relay type flags, it eliminates the redundant multiple calls to
publish_account_relay_infowhile maintaining the same functionality.
| // TODO: This should only query local nostr cached events, not fetch from relays | ||
| let key_package_event = self | ||
| .fetch_key_package_event_from(account.key_package_relays.clone(), pubkey) | ||
| .await?; | ||
| if key_package_event.is_none() { | ||
| self.publish_key_package_for_account(&account).await?; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Address TODO: Avoid network calls during login.
The TODO comment indicates this should only query local cached events, but fetch_key_package_event_from suggests it may be making network calls to relays. Network operations during login can significantly slow down the user experience.
Consider implementing a local-only query method or clarify if this method actually hits the network:
🏁 Script executed:
#!/bin/bash
# Search for the implementation of fetch_key_package_event_from to verify if it makes network calls
ast-grep --pattern $'fetch_key_package_event_from($$$) {
$$$
}'Length of output: 66
🏁 Script executed:
#!/bin/bash
# Locate the definition and usages of fetch_key_package_event_from to confirm if it performs network calls
rg -n "fn fetch_key_package_event_from" -C 5Length of output: 826
🏁 Script executed:
#!/bin/bash
# Extract the full body of fetch_key_package_event_from to inspect its behavior
sed -n '138,200p' src/whitenoise/accounts/contacts.rsLength of output: 2259
🏁 Script executed:
#!/bin/bash
# Locate the implementation of fetch_user_key_package to determine if it performs network calls
rg -n "fn fetch_user_key_package" -C 5Length of output: 620
Avoid network calls in login’s key-package lookup
The fetch_key_package_event_from method in accounts/contacts.rs (lines 138–142) delegates to self.nostr.fetch_user_key_package (see nostr_manager/query.rs at line 149), which issues an actual relay query. This contradicts the TODO and risks slowing down login.
Please:
- Introduce a local-only lookup (e.g.,
fetch_cached_key_package_event) that reads from the local cache/DB without outbound network calls. - In the login flow (
src/whitenoise/accounts/mod.rs:334–340), use this local-only method first. - Defer any relay fetch for missing key packages to after login (e.g., background task), so login isn’t blocked by network latency.
Files to update:
- src/whitenoise/accounts/mod.rs (login logic)
- src/whitenoise/accounts/contacts.rs (add local-only fetch)
- src/nostr_manager/query.rs (relay-based fetch for background use)
🤖 Prompt for AI Agents
In src/whitenoise/accounts/mod.rs lines 334-340, replace the current call to
fetch_key_package_event_from, which performs a network relay query, with a new
local-only method fetch_cached_key_package_event that reads from the local cache
or database without network calls. Implement this new local-only fetch method in
src/whitenoise/accounts/contacts.rs to return cached key package events. Modify
src/nostr_manager/query.rs to keep the relay-based fetch method for use in
background tasks after login. Adjust the login flow to use the local-only fetch
first and defer any relay fetches for missing key packages to a background
process to avoid blocking login on network latency.
There was a problem hiding this comment.
Cool! 🎉 I tested it locally from the 431-relay-management-update branch with the last commit of this PR and I can add local relay and it connects (previously it didn't).
I still initially had no relays, I had to add it manually (for an existing account with data). But I'm not sure if maybe my relay list actually got screwed up and got published as empty so it is correct that now I don't have relays 🤔
After adding a relay, if I close the emulator and open the app again, the added relay appears 🎉
jgmontoya
left a comment
jgmontoya
left a comment
There was a problem hiding this comment.
Some comments/questions/suggestions :D
src/whitenoise/accounts/relays.rs
Outdated
| &self, | ||
| account: &Account, | ||
| relay_type: RelayType, | ||
| prior_relays: &Option<DashSet<RelayUrl>>, // If provided, this means at least one relay was removed. We need to publish to the prior relays as well. |
There was a problem hiding this comment.
I can see how this got here as removing a relay is the use case that made this required, however I'd rename it so it's more descriptive of what it does. What do you think about something like 'relays_to_use', or 'target_relays' or something like that? So basically we know that whatever we pass here means we're publishing to those relays.
| // Fetch key packages for all members | ||
| for pk in members.iter() { | ||
| let contact = self.load_contact(pk).await?; | ||
| let relays_to_use = if contact.key_package_relays.is_empty() { |
| self.setup_subscriptions(&account).await?; | ||
|
|
||
| self.publish_key_package_for_account(&account).await?; | ||
| // TODO: This should only query local nostr cached events, not fetch from relays |
There was a problem hiding this comment.
Keypackage events are regularly updated. For this alone we have to fetch from the nostr relays, I think we agreed on this earlier.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests