Remove admins param from create_group, refactor tests a bit #326
Conversation
WalkthroughReworks group admin handling by embedding admin pubkeys into NostrGroupConfigData, simplifies Whitenoise::create_group signature, adds Whitenoise::update_group_data, updates tests and event handlers, adjusts public re-exports for GroupType/GroupInformation, and aligns several nostr-related dependency references in Cargo.toml to a different remote repository. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant WN as Whitenoise
participant MLS as nostr_mls
participant DB as Storage
participant Relays as Relays
Note over Caller,WN: Create Group (admins embedded in config)
Caller->>WN: create_group(creator, members, config, group_type?)
WN->>MLS: create_group(creator, members, config, group_type?)
MLS-->>WN: Group + PendingCommit
WN->>DB: Persist group/state
WN->>Relays: Publish creation/evolution events
Relays-->>WN: Ack/No-ack (best effort)
WN-->>Caller: Group
sequenceDiagram
autonumber
actor Caller
participant WN as Whitenoise
participant MLS as nostr_mls
participant DB as Storage
participant Relays as Relays
Note over Caller,WN: Update Group Data
Caller->>WN: update_group_data(account, group_id, dataUpdate)
WN->>MLS: update_group_data(account, group_id, dataUpdate)
MLS-->>WN: evolution_event + PendingCommit
WN->>DB: Merge/persist commit and metadata
WN->>Relays: Publish evolution_event to group's relays
WN-->>Caller: Result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
erskingardner
force-pushed
the
updates-for-nostr-mls
branch
from
a10e2f1 to
7d7ea57
Compare
![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
🧹 Nitpick comments (11)
src/lib.rs (1)
24-24: No internal references to the old MLSGroupTypepath found
- Verified via
rgthat there are no remaining imports or fully-qualified uses ofnostr_mls::prelude::group_types::GroupTypeanywhere insrc/.- The crate root now publicly re-exports its own
GroupTypefromsrc/lib.rs:24(pub use whitenoise::group_information::{GroupInformation, GroupType};), replacing the MLS-provided enum with a local definition.- This constitutes a breaking change for downstream consumers who previously relied on the identity of
nostr_mls’sGroupType. To smooth migration:
- Call out this change explicitly under “Breaking changes” in your next release notes.
- Optionally add a temporary alias in
src/lib.rs, for example:#[deprecated( since = "X.Y.Z", note = "Use `whitenoise::GroupType`; this alias will be removed in the next major version" )] pub use whitenoise::group_information::GroupType as MlsGroupType;src/whitenoise/groups.rs (9)
61-61: Typo in log message“Succefully” → “Successfully”.
- tracing::debug!("Succefully fetched the key packages of members"); + tracing::debug!("Successfully fetched the key packages of members");
65-71: Admins now come from config — consider early validation for clearer errorsPassing config directly to nostr_mls.create_group is correct. For better UX, validate that:
- config.admins is non-empty
- All admins are among creator + member_pubkeys
Failing fast here yields clearer errors than the underlying MLS error.
- let create_group_result = nostr_mls.create_group( + // Optional pre-validation for clearer errors + if config.admins.is_empty() { + return Err(WhitenoiseError::NostrMlsError(nostr_mls::Error::Group( + "At least one admin is required".to_owned(), + ))); + } + let mut participants = member_pubkeys.clone(); + participants.push(creator_account.pubkey); + if !config.admins.iter().all(|a| participants.contains(a)) { + return Err(WhitenoiseError::NostrMlsError(nostr_mls::Error::Group( + "Admin must be a member".to_owned(), + ))); + } + let create_group_result = nostr_mls.create_group( &creator_account.pubkey, key_package_events.clone(), config )?;
106-111: Prefer Duration addition for readability and consistencyElsewhere you use Duration math. Keep it consistent.
- use std::ops::Add; - let one_month_future = Timestamp::now().add(30 * 24 * 60 * 60); + let one_month_future = Timestamp::now() + Duration::from_secs(30 * 24 * 60 * 60);
123-137: Empty group_relays leads to no subscriptions; add fallback like add_members_to_groupIf config.relays is empty, setup_group_messages_subscriptions_with_signer receives an empty relay list and won’t subscribe. Mirror the fallback you implemented in add_members_to_group by using the account’s NIP-65 relays.
- let mut relays = HashSet::new(); - for relay_url in group_relays { - let db_relay = self.find_or_create_relay_by_url(&relay_url).await?; - relays.insert(db_relay); - } - - self.nostr - .setup_group_messages_subscriptions_with_signer( - creator_account.pubkey, - &relays.into_iter().collect::<Vec<_>>(), - &group_ids, - keys, - ) - .await - .map_err(WhitenoiseError::from)?; + let relays_vec = if group_relays.is_empty() { + // Fallback to account relays + let fallback = account.nip65_relays(self).await?; + if fallback.is_empty() { + return Err(WhitenoiseError::Other(anyhow::anyhow!( + "No relays available for subscribing to group messages" + ))); + } + fallback + } else { + let mut relays = HashSet::new(); + for relay_url in group_relays { + let db_relay = self.find_or_create_relay_by_url(&relay_url).await?; + relays.insert(db_relay); + } + relays.into_iter().collect::<Vec<_>>() + }; + + self.nostr + .setup_group_messages_subscriptions_with_signer( + creator_account.pubkey, + &relays_vec, + &group_ids, + keys, + ) + .await + .map_err(WhitenoiseError::from)?;
311-324: TODO note: consider implementing the same fallback hereYou’ve already done the fallback logic in add_members_to_group above (when group_relays is empty). It would be consistent to adopt that here for welcome fan-out when a contact has no inbox relays configured.
365-391: New: update_group_data — good, but mirror relay fallback and DRY publishingFunction works, but:
- If group_relays is empty, publishing will silently do nothing. Use the same fallback to account relays as in add_members_to_group.
- The “resolve relays + publish evolution event” pattern is duplicated across add/remove/update. Consider extracting to a helper to reduce drift.
- let group_relays = nostr_mls.get_relays(group_id)?; - let evolution_event = update_result.evolution_event; - let mut relays = HashSet::new(); - for relay_url in group_relays { - let db_relay = self.find_or_create_relay_by_url(&relay_url).await?; - relays.insert(db_relay); - } - self.nostr - .publish_event_to(evolution_event, &relays.into_iter().collect::<Vec<_>>()) - .await?; + let group_relays = nostr_mls.get_relays(group_id)?; + let evolution_event = update_result.evolution_event; + + let relays_vec = if group_relays.is_empty() { + let fallback = account.nip65_relays(self).await?; + if fallback.is_empty() { + return Err(WhitenoiseError::Other(anyhow::anyhow!( + "No relays available for publishing evolution event" + ))); + } + fallback + } else { + let mut relays = HashSet::new(); + for relay_url in group_relays { + let db_relay = self.find_or_create_relay_by_url(&relay_url).await?; + relays.insert(db_relay); + } + relays.into_iter().collect::<Vec<_>>() + }; + + self.nostr.publish_event_to(evolution_event, &relays_vec).await?;Optionally, extract the “collect group relays or fallback to account relays” into a helper to use here and in add/remove.
470-526: Tests: use of clones and comments
- config.clone() is unnecessary where config isn’t reused; remove the clone to keep tests tidy.
- The comment “Verify group state and type… valid state” could become an assertion if GroupState is re-exported. If not, leaving it as-is is fine.
- .create_group( - creator_account, - member_pubkeys.clone(), - config.clone(), - None, - ) + .create_group(creator_account, member_pubkeys.clone(), config, None)
657-705: End-to-end membership tests look solid; minor flakiness risk due to relay timingGiven real relay use in CI, these can be timing-sensitive. If flakiness appears, consider:
- Using wait/retry helpers instead of fixed sleeps
- Mocking NostrManager for unit tests and leaving real relays to integration tests
If you want, I can draft a small test helper that polls for key package availability with a timeout to replace fixed sleeps.
708-751: update_group_data test covers name/description/image fields wellSolid assertions after update. You might also add a negative-path test (e.g., non-admin attempting update) if MLS layer returns a distinct error for permissions.
src/whitenoise/welcomes.rs (1)
175-177: Small cleanup: avoid unnecessary clone when building config
create_nostr_group_config_datatakes ownership of theVec<PublicKey>, so you can passadmin_pubkeysdirectly without cloning.- let admin_pubkeys = vec![creator_account.pubkey, member_pubkeys[0]]; - let config = create_nostr_group_config_data(admin_pubkeys.clone()); + let admin_pubkeys = vec![creator_account.pubkey, member_pubkeys[0]]; + let config = create_nostr_group_config_data(admin_pubkeys);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.toml(1 hunks)src/lib.rs(2 hunks)src/whitenoise/event_processor/event_handlers/handle_giftwrap.rs(1 hunks)src/whitenoise/event_processor/event_handlers/handle_mls_message.rs(2 hunks)src/whitenoise/groups.rs(10 hunks)src/whitenoise/mod.rs(1 hunks)src/whitenoise/welcomes.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-17T19:35:53.520Z
Learnt from: jmcorgan
PR: parres-hq/whitenoise#318
File: src/whitenoise/accounts/welcomes.rs:53-53
Timestamp: 2025-08-17T19:35:53.520Z
Learning: In the Whitenoise codebase, Account::create_nostr_mls should have its errors mapped to WhitenoiseError::NostrMlsError instead of using anyhow::anyhow!() wrapping, as this preserves type safety and specific error information.
Applied to files:
src/whitenoise/event_processor/event_handlers/handle_mls_message.rs
📚 Learning: 2025-08-17T19:34:30.333Z
Learnt from: jmcorgan
PR: parres-hq/whitenoise#318
File: src/whitenoise/accounts/messages.rs:193-195
Timestamp: 2025-08-17T19:34:30.333Z
Learning: In the whitenoise codebase, `Account::create_nostr_mls()` returns `Result<NostrMls<NostrMlsSqliteStorage>, AccountError>`, and `WhitenoiseError` has an `Account(#[from] AccountError)` variant that automatically converts. When handling MLS creation errors, use the `?` operator to let `AccountError` convert naturally to `WhitenoiseError::Account` rather than manually mapping to `WhitenoiseError::Configuration`.
Applied to files:
src/whitenoise/event_processor/event_handlers/handle_mls_message.rs
🧬 Code graph analysis (5)
src/lib.rs (1)
src/whitenoise/groups.rs (1)
nostr_mls(72-76)
src/whitenoise/event_processor/event_handlers/handle_mls_message.rs (1)
src/whitenoise/mod.rs (1)
create_nostr_group_config_data(521-530)
src/whitenoise/event_processor/event_handlers/handle_giftwrap.rs (1)
src/whitenoise/mod.rs (1)
create_nostr_group_config_data(521-530)
src/whitenoise/welcomes.rs (1)
src/whitenoise/mod.rs (1)
create_nostr_group_config_data(521-530)
src/whitenoise/groups.rs (3)
src/whitenoise/accounts.rs (4)
create_nostr_mls(250-257)create_nostr_mls(891-893)relays(85-93)new(51-70)src/whitenoise/database/users.rs (1)
relays(157-189)src/whitenoise/mod.rs (2)
new(52-66)create_nostr_group_config_data(521-530)
🪛 GitHub Actions: CI
Cargo.toml
[error] 1-1: Command 'cargo check --all-targets --all-features' failed. Could not load dependency 'nostr' for package 'whitenoise v0.1.0'. Missing file: /Users/runner/work/whitenoise/rust-nostr/crates/nostr/Cargo.toml (No such file or directory).
🔇 Additional comments (5)
src/lib.rs (1)
65-65: Re-export now excludes GroupType — consistent with the new API surfaceRe-exporting only Group and GroupState from nostr_mls keeps the crate-level API focused and avoids the previous type collision with your own GroupType. LGTM.
src/whitenoise/mod.rs (1)
521-529: Admins now embedded in NostrGroupConfigData (test util) — good alignment with create_group APIThe helper signature change keeps tests aligned with the new contract. One small tweak: accept an
Into<Vec<PublicKey>>to allow future call sites to pass slices (or other collections) without extra allocations. I ran the grep check and confirmed that today all tests pass aVec<PublicKey>, so this change is fully backward-compatible.-pub(crate) fn create_nostr_group_config_data(admins: Vec<PublicKey>) -> NostrGroupConfigData { +pub(crate) fn create_nostr_group_config_data(admins: impl Into<Vec<PublicKey>>) -> NostrGroupConfigData { NostrGroupConfigData { name: "Test group".to_owned(), description: "test description".to_owned(), image_url: Some("http://test_blossom:53232/fake_img.png".to_owned()), image_key: Some(b"fake key to encrypt image".to_vec()), relays: vec![RelayUrl::parse("ws://localhost:8080/").unwrap()], - admins, + admins: admins.into(), } }• All existing call sites invoke with a
Vec<PublicKey>(admin_pubkeys.clone(),vec![…], etc.), so no test breaks will occur.
• Future calls can now also pass slices (&[PublicKey]) by adding a trivialFromimpl or usingto_vec()implicitly viaInto.src/whitenoise/event_processor/event_handlers/handle_mls_message.rs (1)
63-66: Tests updated to embed admins in config — matches the new create_group signatureBoth tests correctly build config via create_nostr_group_config_data(vec![creator_account.pubkey]) and drop the separate creators/admin parameter. The error-path assertion expecting WhitenoiseError::NostrMlsError remains valid with the current mapping.
Also applies to: 112-115
src/whitenoise/event_processor/event_handlers/handle_giftwrap.rs (1)
113-118: Admins moved into config: test update aligns with API changePassing the creator’s pubkey into
create_nostr_group_config_data(vec![creator_account.pubkey])correctly reflects the removal of the separate admins parameter and ensures the creator is part of the admin set at group creation time. Looks good.src/whitenoise/welcomes.rs (1)
175-183: ✅ Signature update sanity check passedAll
create_nostr_group_config_dataand.create_groupcall-sites have been verified against the updated signatures:
- No lingering no-argument calls to
create_nostr_group_config_data()were found.- The
create_groupdefinition atsrc/whitenoise/groups.rsreflects the new signature.- Every invocation of
.create_group(...)across the codebase and tests supplies the required parameters, with no deprecated usages detected.No further changes are needed here.
|
@coderabbitai, please do a full review |
|
@erskingardner I'll conduct a comprehensive review of all the changes in this PR, including the API refactoring, dependency updates, and test modifications. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/whitenoise/groups.rs (2)
103-106: Bug: invalid Timestamp addition (likely compile-time error).
Timestamp::now().add(…)with a bare integer is not the common API and differs from the rest of the file whereDurationis used. This should use+ Duration::from_secs(...). Also drop the function-scopeduse std::ops::Add;.Apply:
- // Create a timestamp 1 month in the future - use std::ops::Add; - let one_month_future = Timestamp::now().add(30 * 24 * 60 * 60); + // Create a timestamp ~1 month in the future + let one_month_future = Timestamp::now() + Duration::from_secs(30 * 24 * 60 * 60);
350-359: Missing relay fallback when publishing evolution event after removals.Unlike
add_members_to_group, this path doesn’t handle the “no group relays configured” case. Use account relays as fallback, or return a clear error. Otherwise, updates can be dropped.Apply:
- self.nostr - .publish_event_to(evolution_event, &relays.into_iter().collect::<Vec<_>>()) - .await?; + if relays.is_empty() { + tracing::warn!( + target: "whitenoise::remove_members_from_group", + "Group has no relays configured, using account's default relays" + ); + let fallback_relays = account.nip65_relays(self).await?; + if fallback_relays.is_empty() { + return Err(WhitenoiseError::Other(anyhow::anyhow!( + "No relays available for publishing evolution event - both group relays and account relays are empty" + ))); + } + self.nostr.publish_event_to(evolution_event, &fallback_relays).await?; + } else { + self.nostr + .publish_event_to(evolution_event, &relays.into_iter().collect::<Vec<_>>()) + .await?; + }
🧹 Nitpick comments (11)
src/whitenoise/mod.rs (1)
521-529: Move-admins-to-config is correct; tighten helper API to avoid unnecessary clonesThe new helper correctly embeds admins into NostrGroupConfigData. To reduce call-site friction and avoid extra Vec clones in tests, accept any Into<Vec> (or even impl IntoIterator<Item=PublicKey>) and convert internally. Also add a brief doc comment to signal that admins must be non-empty.
Apply:
- pub(crate) fn create_nostr_group_config_data(admins: Vec<PublicKey>) -> NostrGroupConfigData { + /// Build a minimal NostrGroupConfigData for tests. + /// Note: `admins` must be non-empty. + pub(crate) fn create_nostr_group_config_data(admins: impl Into<Vec<PublicKey>>) -> NostrGroupConfigData { - NostrGroupConfigData { + NostrGroupConfigData { name: "Test group".to_owned(), description: "test description".to_owned(), image_url: Some("http://test_blossom:53232/fake_img.png".to_owned()), image_key: Some(b"fake key to encrypt image".to_vec()), relays: vec![RelayUrl::parse("ws://localhost:8080/").unwrap()], - admins, + admins: admins.into(), } }If you adopt this, update the two test call sites to pass the vector directly without cloning (see comments in welcomes.rs and handle_giftwrap.rs).
src/lib.rs (1)
24-24: Public API shift: GroupType re-export moved; add a compatibility aliasRe-exporting GroupType from whitenoise::group_information is fine, but it’s a breaking change for users who previously imported it from nostr_mls::prelude::group_types. Consider a deprecated alias to ease migration.
pub use whitenoise::group_information::{GroupInformation, GroupType}; +#[deprecated(note = "Use whitenoise::group_information::GroupType instead")] +pub type MlsGroupType = GroupType;Cargo.toml (1)
31-36: Use the official rust-nostr repo for production deps; reserve your fork for local patches onlyPinning by commit guarantees reproducibility, but Cargo.toml currently points core Nostr crates at your personal fork (github.com/erskingardner/rust-nostr), which raises supply-chain and CI stability risks if that repo ever becomes unavailable. Instead:
• Restore the upstream remotes under version 0.43.0 (github.com/rust-nostr/nostr), pinned to the same commit
• Move your fork overrides into a VCS-ignored[patch]section in.cargo/config.toml, so CI and other developers stay on the official sourceImpacted locations:
- Cargo.toml – lines 31–36 (nostr, nostr-mls, nostr-mls-sqlite-storage, nwc, nostr-blossom, nostr-sdk)
- Cargo.toml – lines 48–49 (additional nostr-sdk features)
Proposed Cargo.toml diff:
-nostr = { version = "0.43.0", git = "https://github.com/erskingardner/rust-nostr", rev = "03934fc8e1e1e6c831f282cde9e13589be15c21e", features = ["std"] } -nostr-mls = { version = "0.43.0", git = "https://github.com/erskingardner/rust-nostr", rev = "03934fc8e1e1e6c831f282cde9e13589be15c21e" } -nostr-mls-sqlite-storage = { version = "0.43.0", git = "https://github.com/erskingardner/rust-nostr", rev = "03934fc8e1e1e6c831f282cde9e13589be15c21e" } -nwc = { version = "0.43.0", git = "https://github.com/erskingardner/rust-nostr", rev = "03934fc8e1e1e6c831f282cde9e13589be15c21e" } -nostr-blossom = { version = "0.43.0", git = "https://github.com/erskingardner/rust-nostr", rev = "03934fc8e1e1e6c831f282cde9e13589be15c21e" } -nostr-sdk = { version = "0.43.0", git = "https://github.com/erskingardner/rust-nostr", rev = "03934fc8e1e1e6c831f282cde9e13589be15c21e", features = [ +nostr = { version = "0.43.0", git = "https://github.com/rust-nostr/nostr", rev = "d70bed548d2e1399b758ad8bb7da9f058a716064", features = ["std"] } +nostr-mls = { version = "0.43.0", git = "https://github.com/rust-nostr/nostr", rev = "d70bed548d2e1399b758ad8bb7da9f058a716064" } +nostr-mls-sqlite-storage = { version = "0.43.0", git = "https://github.com/rust-nostr/nostr", rev = "d70bed548d2e1399b758ad8bb7da9f058a716064" } +nwc = { version = "0.43.0", git = "https://github.com/rust-nostr/nostr", rev = "d70bed548d2e1399b758ad8bb7da9f058a716064" } +nostr-blossom = { version = "0.43.0", git = "https://github.com/rust-nostr/nostr", rev = "d70bed548d2e1399b758ad8bb7da9f058a716064" } +nostr-sdk = { version = "0.43.0", git = "https://github.com/rust-nostr/nostr", rev = "d70bed548d2e1399b758ad8bb7da9f058a716064", features = [ "lmdb", "nip04", "nip44", "nip47", "nip59", ] }Example local override (
.cargo/config.toml, uncommitted):[patch."https://github.com/rust-nostr/nostr"] nostr = { path = "../rust-nostr/crates/nostr" } nostr-mls = { path = "../rust-nostr/mls/nostr-mls" } nostr-mls-sqlite-storage = { path = "../rust-nostr/mls/nostr-mls-sqlite-storage" } nwc = { path = "../rust-nostr/crates/nwc" } nostr-blossom = { path = "../rust-nostr/rfs/nostr-blossom" } nostr-sdk = { path = "../rust-nostr/crates/nostr-sdk" }src/whitenoise/welcomes.rs (1)
176-180: Tests: avoid cloning Vecs; pass admins/members by valueNow that admins live in the config and create_group no longer needs a separate admins param, you can drop the clones here to keep tests lean.
- let config = create_nostr_group_config_data(admin_pubkeys.clone()); + let config = create_nostr_group_config_data(admin_pubkeys); - let group = whitenoise - .create_group(&creator_account, member_pubkeys.clone(), config, None) + let group = whitenoise + .create_group(&creator_account, member_pubkeys, config, None) .await;If you adopt the Into<Vec> helper signature suggested in mod.rs, this call site stays the same.
src/whitenoise/event_processor/event_handlers/handle_giftwrap.rs (1)
117-118: Good: config now carries admins for MLS group creationSwitching the nostr_mls::create_group call to pass NostrGroupConfigData with admins embedded matches the refactor’s goal. Consider using the generalized helper signature (impl Into<Vec>) to stay flexible in tests, but the current usage is fine.
If you adopt the helper refactor:
- create_nostr_group_config_data(vec![creator_account.pubkey]), + create_nostr_group_config_data([creator_account.pubkey]),src/integration_tests/test_cases/shared/create_group.rs (2)
53-60: Prefer centralizing test relay URL and consider a helper for config construction.
- The hard-coded relay URL (
ws://localhost:8080) appears in multiple tests; factor it into a test constant or helper to avoid drift.- Since all options are passed through
NostrGroupConfigData::new(...), consider a small test helper that fills image fields withNoneand sets the default relays/admins to reduce repetition at call sites.Apply this small refactor within your test utils:
+pub fn default_test_relays() -> Vec<RelayUrl> { + vec![RelayUrl::parse("ws://localhost:8080").unwrap()] +}Then in this test:
- NostrGroupConfigData::new( + NostrGroupConfigData::new( self.group_name.clone(), self.group_description.clone(), - None, // image_url - None, // image_key - vec![RelayUrl::parse("ws://localhost:8080").unwrap()], + None, // image_url + None, // image_key + default_test_relays(), admin_pubkeys, ),
65-66: Reduce flakiness: poll for condition instead of fixed sleep.Replace the fixed 100ms sleep with a short wait-until helper (with timeout) that checks for the group’s presence or expected membership count.
Here’s a sketch you can add to test utils, then use here:
pub async fn wait_until<F, Fut>(timeout_ms: u64, mut cond: F) -> bool where F: FnMut() -> Fut, Fut: std::future::Future<Output = bool>, { let start = std::time::Instant::now(); let step = std::time::Duration::from_millis(25); while start.elapsed().as_millis() < timeout_ms as u128 { if cond().await { return true; } tokio::time::sleep(step).await; } false }Usage here:
let ok = wait_until(500, || async { // e.g., try fetching groups and see if the new one appears !context.whitenoise.groups(creator, true).await.unwrap().is_empty() }).await; assert!(ok, "Group did not appear within timeout");src/whitenoise/groups.rs (1)
61-61: Typo in log message.“Succefully” → “Successfully”.
- tracing::debug!("Succefully fetched the key packages of members"); + tracing::debug!("Successfully fetched the key packages of members");src/whitenoise/event_processor/event_handlers/handle_mls_message.rs (3)
56-56: Mitigate timing flakiness in tests.Replace fixed sleeps with a short “wait-until” helper (polling for key package availability or group presence) to reduce intermittent failures on slow CI.
107-115: Same here: avoid fixed sleep where possible.Use the same helper proposed above to wait for relay propagation.
137-140: Intentional corruption is fine; consider adding a brief comment.Changing the outer event kind to
TextNoteensures MLS processing rejects it. Add a clarifying comment so future readers don’t confuse inner vs. outer event kinds.- // Corrupt it by changing the kind so MLS processing fails + // Corrupt the OUTER event kind so MLS processing rejects it (inner content remains a TextNote) let mut bad_event = valid_event.clone(); bad_event.kind = Kind::TextNote;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.toml(2 hunks)src/integration_tests/test_cases/shared/create_group.rs(1 hunks)src/lib.rs(2 hunks)src/whitenoise/event_processor/event_handlers/handle_giftwrap.rs(1 hunks)src/whitenoise/event_processor/event_handlers/handle_mls_message.rs(2 hunks)src/whitenoise/groups.rs(10 hunks)src/whitenoise/mod.rs(1 hunks)src/whitenoise/welcomes.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-17T19:35:53.520Z
Learnt from: jmcorgan
PR: parres-hq/whitenoise#318
File: src/whitenoise/accounts/welcomes.rs:53-53
Timestamp: 2025-08-17T19:35:53.520Z
Learning: In the Whitenoise codebase, Account::create_nostr_mls should have its errors mapped to WhitenoiseError::NostrMlsError instead of using anyhow::anyhow!() wrapping, as this preserves type safety and specific error information.
Applied to files:
src/whitenoise/event_processor/event_handlers/handle_mls_message.rs
📚 Learning: 2025-08-17T19:34:30.333Z
Learnt from: jmcorgan
PR: parres-hq/whitenoise#318
File: src/whitenoise/accounts/messages.rs:193-195
Timestamp: 2025-08-17T19:34:30.333Z
Learning: In the whitenoise codebase, `Account::create_nostr_mls()` returns `Result<NostrMls<NostrMlsSqliteStorage>, AccountError>`, and `WhitenoiseError` has an `Account(#[from] AccountError)` variant that automatically converts. When handling MLS creation errors, use the `?` operator to let `AccountError` convert naturally to `WhitenoiseError::Account` rather than manually mapping to `WhitenoiseError::Configuration`.
Applied to files:
src/whitenoise/event_processor/event_handlers/handle_mls_message.rs
🔇 Additional comments (7)
src/lib.rs (1)
65-65: No staleGroupTypeimports detected
I ran a repo-wide search for any remaining imports ofnostr_mls::prelude::group_types::GroupTypeand found none. AllGroupTypeusages inwhitenoise/groups.rscorrectly referencecrate::whitenoise::group_information::GroupType, so nothing is broken by dropping the re-export. No further changes are needed.src/integration_tests/test_cases/shared/create_group.rs (1)
46-46: Good: admins now flow via config (no separate arg).Setting
admin_pubkeysto include the creator aligns with the API change and maintains a valid admin set.src/whitenoise/groups.rs (3)
66-68: API change looks correct: config now carries admins.Passing
config(with embedded admins) tonostr_mls.create_groupmatches the refactor and keeps the surface smaller. Good change.
24-30: No stalecreate_groupcall sites detectedAll instances of
create_groupnow consistently passadmin_pubkeysviaNostrGroupConfigDataand thegroup_typeparameter. No lingering calls using the old signature remain.
308-318: I’ll wait for the extracted signatures and context to draft the precise fallback implementation.src/whitenoise/event_processor/event_handlers/handle_mls_message.rs (2)
17-37: Error mapping is correct and consistent with project guidance.
Account::create_nostr_mls(...)?leveragesFrom<AccountError> for WhitenoiseError, and MLS processing errors are mapped toWhitenoiseError::NostrMlsError(e). Good separation of concerns and logs.
59-65: Tests correctly updated to pass admins via config.The updated call to
create_groupreflects the new API and keeps test intent unchanged.
| pub async fn update_group_data( | ||
| &self, | ||
| account: &Account, | ||
| group_id: &GroupId, | ||
| group_data: NostrGroupDataUpdate, | ||
| ) -> Result<()> { | ||
| let nostr_mls = Account::create_nostr_mls(account.pubkey, &self.config.data_dir)?; | ||
| let update_result = nostr_mls.update_group_data(group_id, group_data)?; | ||
| nostr_mls.merge_pending_commit(group_id)?; | ||
| let group_relays = nostr_mls.get_relays(group_id)?; | ||
|
|
||
| let evolution_event = update_result.evolution_event; | ||
|
|
||
| let mut relays = HashSet::new(); | ||
| for relay_url in group_relays { | ||
| let db_relay = self.find_or_create_relay_by_url(&relay_url).await?; | ||
| relays.insert(db_relay); | ||
| } | ||
|
|
||
| self.nostr | ||
| .publish_event_to(evolution_event, &relays.into_iter().collect::<Vec<_>>()) | ||
| .await?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add relay fallback to update_group_data for consistency and robustness.
Mirror the fallback behavior used in add_members_to_group so metadata updates don’t get lost when group relays are empty.
- self.nostr
- .publish_event_to(evolution_event, &relays.into_iter().collect::<Vec<_>>())
- .await?;
+ if relays.is_empty() {
+ tracing::warn!(
+ target: "whitenoise::update_group_data",
+ "Group has no relays configured, using account's default relays"
+ );
+ let fallback_relays = account.nip65_relays(self).await?;
+ if fallback_relays.is_empty() {
+ return Err(WhitenoiseError::Other(anyhow::anyhow!(
+ "No relays available for publishing evolution event - both group relays and account relays are empty"
+ )));
+ }
+ self.nostr.publish_event_to(evolution_event, &fallback_relays).await?;
+ } else {
+ self.nostr
+ .publish_event_to(evolution_event, &relays.into_iter().collect::<Vec<_>>())
+ .await?;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub async fn update_group_data( | |
| &self, | |
| account: &Account, | |
| group_id: &GroupId, | |
| group_data: NostrGroupDataUpdate, | |
| ) -> Result<()> { | |
| let nostr_mls = Account::create_nostr_mls(account.pubkey, &self.config.data_dir)?; | |
| let update_result = nostr_mls.update_group_data(group_id, group_data)?; | |
| nostr_mls.merge_pending_commit(group_id)?; | |
| let group_relays = nostr_mls.get_relays(group_id)?; | |
| let evolution_event = update_result.evolution_event; | |
| let mut relays = HashSet::new(); | |
| for relay_url in group_relays { | |
| let db_relay = self.find_or_create_relay_by_url(&relay_url).await?; | |
| relays.insert(db_relay); | |
| } | |
| self.nostr | |
| .publish_event_to(evolution_event, &relays.into_iter().collect::<Vec<_>>()) | |
| .await?; | |
| Ok(()) | |
| } | |
| pub async fn update_group_data( | |
| &self, | |
| account: &Account, | |
| group_id: &GroupId, | |
| group_data: NostrGroupDataUpdate, | |
| ) -> Result<()> { | |
| let nostr_mls = Account::create_nostr_mls(account.pubkey, &self.config.data_dir)?; | |
| let update_result = nostr_mls.update_group_data(group_id, group_data)?; | |
| nostr_mls.merge_pending_commit(group_id)?; | |
| let group_relays = nostr_mls.get_relays(group_id)?; | |
| let evolution_event = update_result.evolution_event; | |
| let mut relays = HashSet::new(); | |
| for relay_url in group_relays { | |
| let db_relay = self.find_or_create_relay_by_url(&relay_url).await?; | |
| relays.insert(db_relay); | |
| } | |
| if relays.is_empty() { | |
| tracing::warn!( | |
| target: "whitenoise::update_group_data", | |
| "Group has no relays configured, using account's default relays" | |
| ); | |
| let fallback_relays = account.nip65_relays(self).await?; | |
| if fallback_relays.is_empty() { | |
| return Err(WhitenoiseError::Other(anyhow::anyhow!( | |
| "No relays available for publishing evolution event - both group relays and account relays are empty" | |
| ))); | |
| } | |
| self.nostr.publish_event_to(evolution_event, &fallback_relays).await?; | |
| } else { | |
| self.nostr | |
| .publish_event_to(evolution_event, &relays.into_iter().collect::<Vec<_>>()) | |
| .await?; | |
| } | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
In src/whitenoise/groups.rs around lines 370 to 393, update update_group_data to
mirror the relay fallback used in add_members_to_group: after retrieving
group_relays, if that list is empty replace it with the same fallback relay list
used in add_members_to_group (e.g., the account's relays or
self.config.default_relays), then continue creating/finding DB relay entries
from that final list and publish to those relays; implement the empty-check and
assignment before the loop so metadata updates are sent when no group relays
exist.
jgmontoya
left a comment
jgmontoya
left a comment
There was a problem hiding this comment.
Looks good, just a minor thing
| let mut relays = HashSet::new(); | ||
| for relay_url in group_relays { | ||
| let db_relay = self.find_or_create_relay_by_url(&relay_url).await?; | ||
| relays.insert(db_relay); | ||
| } |
There was a problem hiding this comment.
I don't think we need to deduplicate here because nostr_mls.get_relays already returns a set (BTreeSet<RelayUrl>)
There was a problem hiding this comment.
This is more about making sure the relays are actually created in our db rather than deduplication.
There was a problem hiding this comment.
let mut relays = Vec::new();
for relay_url in group_relays {
let db_relay = self.find_or_create_relay_by_url(&relay_url).await?;
relays.push(db_relay);
}
self.nostr
.publish_event_to(evolution_event, &relays)
.await?;This version is cleaner.
| let one_month_future = Timestamp::now() + Duration::from_secs(30 * 24 * 60 * 60); | ||
|
|
||
| // Use fallback relays if contact has no inbox relays configured | ||
| // TODO: Maybe need to use fallback relays if contact has no inbox relays configured |
There was a problem hiding this comment.
I can see that when a new user is created, their relays are not inserted by default.
Reference fn find_or_create_by_pubkey
There was a problem hiding this comment.
Yeah - actually - we need to be really careful about this don't we. When we're doing lots of different operations we first need to get user's and also make sure we have their relays etc. I think this deserves another issue that we tackle separately to ensure that we're checking every location where we find or create a user to ensure that we're also trying to fetch their relays and metadata.
| ) -> Result<()> { | ||
| let nostr_mls = Account::create_nostr_mls(account.pubkey, &self.config.data_dir)?; | ||
| let update_result = nostr_mls.update_group_data(group_id, group_data)?; | ||
| nostr_mls.merge_pending_commit(group_id)?; |
There was a problem hiding this comment.
This could be done in nostr-mls?
There was a problem hiding this comment.
No, needs to happen here. The client should be in control of when to call it because they have to be the ones to publish the actual event to relays
delcin-raj
left a comment
delcin-raj
left a comment
There was a problem hiding this comment.
Left some comments and questions.
The tests look comprehensive!
Summary by CodeRabbit
New Features
Refactor