feat(group_information): infer group_type from group name rather than number of members #345
Conversation
WalkthroughGroup type inference changed from participant-count to MLS group-name. Public APIs updated: create/get methods take group_name or require an Account for MLS lookups; Whitenoise facade updated accordingly. Missing groups are created with type inferred from the MLS group name. Tests and docs adjusted; order preserved. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Whitenoise
participant Account
participant GroupInformation
participant MLS as MLS Store
Client->>Whitenoise: get_group_information_by_mls_group_id(account, group_id)
Whitenoise->>GroupInformation: get_by_mls_group_id(account, group_id, whitenoise)
alt Group info exists
GroupInformation-->>Whitenoise: GroupInformation
else Missing group info
GroupInformation->>Account: fetch_mls_group(group_id)
Account->>MLS: get_group(group_id)
MLS-->>Account: group{name}
Account-->>GroupInformation: group{name}
GroupInformation->>GroupInformation: infer_group_type_from_group_name(name)
GroupInformation-->>Whitenoise: GroupInformation (created)
end
Whitenoise-->>Client: GroupInformation
sequenceDiagram
autonumber
participant Client
participant Whitenoise
participant GroupInformation
participant Account
participant MLS as MLS Store
Client->>Whitenoise: get_group_information_by_mls_group_ids(account, [ids])
Whitenoise->>GroupInformation: get_by_mls_group_ids(account, [ids], whitenoise)
loop For each id (preserve input order)
alt Info exists
GroupInformation-->>GroupInformation: collect existing
else Missing
GroupInformation->>Account: fetch_mls_group(id)
Account->>MLS: get_group(id)
MLS-->>Account: group{name}
Account-->>GroupInformation: group{name}
GroupInformation->>GroupInformation: infer type & create
GroupInformation-->>GroupInformation: collect created
end
end
GroupInformation-->>Whitenoise: Vec<GroupInformation> (input order)
Whitenoise-->>Client: Vec<GroupInformation>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
✨ 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. 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/whitenoise/groups.rs (1)
356-359: Unify timestamp arithmetic to avoid trait mismatchElsewhere you use
.add(u64_secs). Use the same here to avoid depending onAdd<Duration>impl.- let one_month_future = Timestamp::now() + Duration::from_secs(30 * 24 * 60 * 60); + use std::ops::Add; + let one_month_future = Timestamp::now().add(30 * 24 * 60 * 60);
🧹 Nitpick comments (8)
src/whitenoise/group_information.rs (4)
52-57: Make inference robust to whitespace-only namesTreat names like " " as DirectMessage too.
- pub(crate) fn infer_group_type_from_group_name(group_name: &str) -> GroupType { - match group_name { - "" => GroupType::DirectMessage, - _ => GroupType::Group, - } - } + pub(crate) fn infer_group_type_from_group_name(group_name: &str) -> GroupType { + if group_name.trim().is_empty() { + GroupType::DirectMessage + } else { + GroupType::Group + } + }
62-64: Fix outdated docstring (“participant count” → “group name”)Update to reflect the new inference rule.
- /// * `group_type` - Optional explicit group type. If None, will be inferred from participant count + /// * `group_type` - Optional explicit group type. If None, will be inferred from group name
101-110: Skip MLS init when input is empty or all IDs already existAvoid unnecessary
create_nostr_mlscost.pub async fn get_by_mls_group_ids( account: &Account, mls_group_ids: &[GroupId], whitenoise: &Whitenoise, ) -> Result<Vec<GroupInformation>, WhitenoiseError> { + if mls_group_ids.is_empty() { + return Ok(Vec::new()); + } @@ - let nostr_mls = Account::create_nostr_mls(account.pubkey, &whitenoise.config.data_dir)?; + if existing_map.len() == mls_group_ids.len() { + // All present: preserve order and return + let mut results = Vec::with_capacity(mls_group_ids.len()); + for id in mls_group_ids { + if let Some(gi) = existing_map.remove(&id.to_vec()) { + results.push(gi); + } + } + return Ok(results); + } + + let nostr_mls = Account::create_nostr_mls(account.pubkey, &whitenoise.config.data_dir)?;Also applies to: 118-121
223-232: Add a whitespace-only case to inference testCovers
trim().is_empty()behavior.fn test_infer_group_type_from_group_name() { assert_eq!( GroupInformation::infer_group_type_from_group_name("test"), GroupType::Group ); assert_eq!( GroupInformation::infer_group_type_from_group_name(""), GroupType::DirectMessage ); + assert_eq!( + GroupInformation::infer_group_type_from_group_name(" "), + GroupType::DirectMessage + ); }src/whitenoise/groups.rs (4)
67-74: Fix outdated docstring (“participant count” → “group name”)Keep docs aligned with the new inference mechanism.
- /// * `group_type` - Optional explicit group type. If None, will be inferred from participant count + /// * `group_type` - Optional explicit group type. If None, will be inferred from group name
114-116: Typo in log message“Succefully” → “Successfully”.
- tracing::debug!("Succefully fetched the key packages of members"); + tracing::debug!("Successfully fetched the key packages of members");
208-236: Optional: cache NostrMLS per account to reduce repeated initialization
Account::create_nostr_mlsis called frequently; consider a lightweight cache keyed bypubkeyinWhitenoiseor a helper that reuses storage handles.Also applies to: 298-306, 433-447, 463-475
591-605: Comment nit: remove stale participant_count mentionThis comment references participant counts; group type now derives from name.
- // Note: participant_count is stored separately and managed by the GroupInformation logic + // Note: group_type is inferred from group name during GroupInformation creation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/whitenoise/group_information.rs(18 hunks)src/whitenoise/groups.rs(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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/group_information.rs
📚 Learning: 2025-09-02T17:30:34.190Z
Learnt from: delcin-raj
PR: parres-hq/whitenoise#337
File: src/whitenoise/groups.rs:567-573
Timestamp: 2025-09-02T17:30:34.190Z
Learning: The `group_images` directory is created during Whitenoise initialization in `src/whitenoise/mod.rs` at line 144 using `std::fs::create_dir_all(data_dir.join("group_images"))`, so no additional directory creation is needed before writing cached group images in the `get_group_image` method.
Applied to files:
src/whitenoise/group_information.rs
📚 Learning: 2025-09-02T17:30:34.190Z
Learnt from: delcin-raj
PR: parres-hq/whitenoise#337
File: src/whitenoise/groups.rs:567-573
Timestamp: 2025-09-02T17:30:34.190Z
Learning: The `group_images` directory is created during Whitenoise initialization in `src/whitenoise/mod.rs`, so no additional directory creation is needed before writing cached group images in `get_group_image` method.
Applied to files:
src/whitenoise/group_information.rs
🧬 Code graph analysis (2)
src/whitenoise/groups.rs (2)
src/whitenoise/group_information.rs (2)
create_for_group(65-80)get_by_mls_group_id(83-99)src/whitenoise/mod.rs (1)
create_nostr_group_config_data(560-570)
src/whitenoise/group_information.rs (3)
src/whitenoise/accounts.rs (3)
create_nostr_mls(250-257)create_nostr_mls(953-955)new(51-70)src/whitenoise/database/group_information.rs (2)
find_or_create_by_mls_group_id(112-127)find_by_mls_group_ids(144-176)src/whitenoise/mod.rs (3)
new(56-70)create_nostr_group_config_data(560-570)create_mock_whitenoise(404-463)
⏰ 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). (3)
- GitHub Check: check (macos-14, native)
- GitHub Check: check (ubuntu-latest, native)
- GitHub Check: check (macos-latest, native)
🔇 Additional comments (6)
src/whitenoise/group_information.rs (4)
71-73: LGTM: explicit type overrides, otherwise inferred from nameThis aligns with the new contract.
82-99: LGTM: correct MLS access and error propagationConstructing NostrMLS via Account and using
?cleanly maps errors into WhitenoiseError; missing group surfaces asGroupNotFound.
144-159: No downstream usages—facade change isn’t breaking
Ripgrep across the repo only finds these methods’ own definitions insrc/whitenoise/group_information.rsand no external call sites to update. The added&Accountparameter has no breaking impact.Likely an incorrect or invalid review comment.
112-117: Ignore the refactor suggestion GroupId already implementsHash + Eq(required byGroupInformationRow’s#[derive(..., Hash, Eq)]in src/whitenoise/database/group_information.rs), so usingHashMap<GroupId, …>is valid and no change is needed.Likely an incorrect or invalid review comment.
src/whitenoise/groups.rs (2)
117-118: LGTM: pass group_name through to GroupInformationThis wires creation to name-based inference correctly.
Also applies to: 197-199
724-743: LGTM: DM classification via empty nameNice end-to-end validation that an empty
config.nameyields DirectMessage.
jgmontoya
force-pushed
the
feat/use-group-name-for-default-group-type
branch
from
1e57dd5 to
398f77d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/whitenoise/group_information.rs (4)
52-57: Trim group name before inferring type (handles whitespace-only names)Without trimming,
" "is treated as Group. If the intent is “empty after normalization => DirectMessage,” trim first.pub(crate) fn infer_group_type_from_group_name(group_name: &str) -> GroupType { - match group_name { + match group_name.trim() { "" => GroupType::DirectMessage, _ => GroupType::Group, } }If you adopt this, add a test case for whitespace-only names (see test suggestion below).
82-99: Minor: defer error construction and confirm not-found semantics
- Use
ok_or_elseto avoid eagerly constructing the error.- Semantics: returning
GroupNotFoundfor missing MLS groups is fine; just ensure this is the intended behavior (vs. creating a placeholder).let group = nostr_mls .get_group(mls_group_id)? - .ok_or(WhitenoiseError::GroupNotFound)?; + .ok_or_else(|| WhitenoiseError::GroupNotFound)?;
112-121: Batch retrieval: small perf nits and behavior check
- Pre-allocate
existing_mapandresults.- Early-return on empty input to avoid a DB hit (tests already cover empty case).
- Also switch to
ok_or_elsehere.-// Create a map for quick lookups, but continue to preserve input order -let mut existing_map: std::collections::HashMap<GroupId, GroupInformation> = existing - .into_iter() - .map(|gi| (gi.mls_group_id.clone(), gi)) - .collect(); +// Create a map for quick lookups, but continue to preserve input order +let mut existing_map: std::collections::HashMap<GroupId, GroupInformation> = + std::collections::HashMap::with_capacity(existing.len()); +for gi in existing.into_iter() { + existing_map.insert(gi.mls_group_id.clone(), gi); +} -let mut results = Vec::new(); +if mls_group_ids.is_empty() { + return Ok(Vec::new()); +} +let mut results = Vec::with_capacity(mls_group_ids.len()); - let group = nostr_mls - .get_group(mls_group_id)? - .ok_or(WhitenoiseError::GroupNotFound)?; + let group = nostr_mls + .get_group(mls_group_id)? + .ok_or_else(|| WhitenoiseError::GroupNotFound)?;Behavior check: When any MLS group is missing, the whole batch errors. If you prefer “best-effort” to return existing + creatable entries and skip missing, confirm and I can propose a follow-up patch.
Also applies to: 125-133
223-232: Add a whitespace-only inference test if trimming is adoptedCurrently tests cover empty vs non-empty. If you accept trimming, add a case to lock in behavior.
Rust test to add (outside current ranges):
#[test] fn test_infer_group_type_from_group_name_whitespace() { assert_eq!( GroupInformation::infer_group_type_from_group_name(" \t "), GroupType::DirectMessage ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/whitenoise/group_information.rs(17 hunks)src/whitenoise/groups.rs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/whitenoise/groups.rs
🧰 Additional context used
🧠 Learnings (3)
📚 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/group_information.rs
📚 Learning: 2025-09-02T17:30:34.190Z
Learnt from: delcin-raj
PR: parres-hq/whitenoise#337
File: src/whitenoise/groups.rs:567-573
Timestamp: 2025-09-02T17:30:34.190Z
Learning: The `group_images` directory is created during Whitenoise initialization in `src/whitenoise/mod.rs` at line 144 using `std::fs::create_dir_all(data_dir.join("group_images"))`, so no additional directory creation is needed before writing cached group images in the `get_group_image` method.
Applied to files:
src/whitenoise/group_information.rs
📚 Learning: 2025-09-02T17:30:34.190Z
Learnt from: delcin-raj
PR: parres-hq/whitenoise#337
File: src/whitenoise/groups.rs:567-573
Timestamp: 2025-09-02T17:30:34.190Z
Learning: The `group_images` directory is created during Whitenoise initialization in `src/whitenoise/mod.rs`, so no additional directory creation is needed before writing cached group images in `get_group_image` method.
Applied to files:
src/whitenoise/group_information.rs
⏰ 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 (4)
src/whitenoise/group_information.rs (4)
7-7: Good: MLS lookups now scoped by Account; error conversion via?is correctUsing
Accounthere aligns with the new API surface. Also, lettingAccountErrorconvert toWhitenoiseErrorvia?follows prior guidance.
71-73: Inference fallback looks goodUsing
unwrap_or_elsewith the helper keeps the explicit type authoritative and the behavior predictable.
144-160: Facade pass-throughs are consistent with the new APISignatures now require
&Accountand thread through correctly.
299-307: Tests are thorough and exercise key pathsGood coverage: explicit vs inferred types, idempotency, defaulting via MLS names, order preservation, and empty input.
Also applies to: 333-354, 356-396, 397-449, 451-521, 523-566, 568-579, 581-633
… number of members
jgmontoya
force-pushed
the
feat/use-group-name-for-default-group-type
branch
from
398f77d to
26cc01e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/whitenoise/groups.rs (2)
64-70: Doc comment is now stale: inference is by name, not participant countUpdate the parameter doc to reflect name-based inference.
-/// * `group_type` - Optional explicit group type. If None, will be inferred from participant count +/// * `group_type` - Optional explicit group type. If None, will be inferred from the group name
117-117: Infer type from the actual created group's name (avoid using pre-create config)The MLS layer could normalize/mutate the name. Use the returned
group.nameinstead ofconfig.name.- let group_name = config.name.clone(); ... - GroupInformation::create_for_group(self, &group.mls_group_id, group_type, &group_name) - .await?; + GroupInformation::create_for_group(self, &group.mls_group_id, group_type, &group.name) + .await?;Also applies to: 197-199
src/whitenoise/group_information.rs (3)
53-58: Trim whitespace when inferring from nameNames that are only whitespace should map to DirectMessage too.
- pub(crate) fn infer_group_type_from_group_name(group_name: &str) -> GroupType { - match group_name { - "" => GroupType::DirectMessage, - _ => GroupType::Group, - } - } + pub(crate) fn infer_group_type_from_group_name(group_name: &str) -> GroupType { + if group_name.trim().is_empty() { + GroupType::DirectMessage + } else { + GroupType::Group + } + }
104-121: Fast-path empty input to avoid unnecessary workReturn early when
mls_group_idsis empty to skip DB/MLS calls.pub async fn get_by_mls_group_ids( account_pubkey: PublicKey, mls_group_ids: &[GroupId], whitenoise: &Whitenoise, ) -> Result<Vec<GroupInformation>, WhitenoiseError> { + if mls_group_ids.is_empty() { + return Ok(Vec::new()); + }
224-233: LGTM: inference unit testCovers both non-empty and empty name paths. Consider adding a
" "whitespace case if you adopt the trim change.assert_eq!( GroupInformation::infer_group_type_from_group_name(""), GroupType::DirectMessage ); + assert_eq!( + GroupInformation::infer_group_type_from_group_name(" "), + GroupType::DirectMessage + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/whitenoise/group_information.rs(17 hunks)src/whitenoise/groups.rs(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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/group_information.rs
📚 Learning: 2025-09-02T17:30:34.190Z
Learnt from: delcin-raj
PR: parres-hq/whitenoise#337
File: src/whitenoise/groups.rs:567-573
Timestamp: 2025-09-02T17:30:34.190Z
Learning: The `group_images` directory is created during Whitenoise initialization in `src/whitenoise/mod.rs` at line 144 using `std::fs::create_dir_all(data_dir.join("group_images"))`, so no additional directory creation is needed before writing cached group images in the `get_group_image` method.
Applied to files:
src/whitenoise/group_information.rs
📚 Learning: 2025-09-02T17:30:34.190Z
Learnt from: delcin-raj
PR: parres-hq/whitenoise#337
File: src/whitenoise/groups.rs:567-573
Timestamp: 2025-09-02T17:30:34.190Z
Learning: The `group_images` directory is created during Whitenoise initialization in `src/whitenoise/mod.rs`, so no additional directory creation is needed before writing cached group images in `get_group_image` method.
Applied to files:
src/whitenoise/group_information.rs
🧬 Code graph analysis (2)
src/whitenoise/groups.rs (2)
src/whitenoise/group_information.rs (2)
create_for_group(66-81)get_by_mls_group_id(84-100)src/whitenoise/mod.rs (1)
create_nostr_group_config_data(560-570)
src/whitenoise/group_information.rs (3)
src/whitenoise/groups.rs (4)
nostr_mls(123-127)nostr_mls(209-214)nostr_mls(223-227)nostr_mls(236-242)src/whitenoise/accounts.rs (3)
create_nostr_mls(250-257)create_nostr_mls(953-955)new(51-70)src/whitenoise/mod.rs (3)
new(56-70)create_nostr_group_config_data(560-570)create_mock_whitenoise(404-463)
⏰ 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). (2)
- GitHub Check: check (macos-latest, native)
- GitHub Check: check (ubuntu-latest, native)
🔇 Additional comments (9)
src/whitenoise/groups.rs (3)
595-601: LGTM: account-context lookup path exercisedTests correctly pass the creator’s pubkey into the new
get_by_mls_group_idAPI and assert Group type.
727-729: LGTM: DM classification via empty nameSetting
config.name = ""to trigger DM inference matches the new rules.
737-743: LGTM: updated API usage in DM testAccount pubkey–based
get_by_mls_group_idis used consistently.src/whitenoise/group_information.rs (6)
89-92: Good use of Account::create_nostr_mls with?conversionError propagation aligns with the codebase convention for
AccountError -> WhitenoiseError.
102-142: Order preservation and single MLS handle — looks solidBatch lookup preserves input order and avoids repeated MLS instantiation.
126-132: Confirm desired behavior for missing groups in batchCurrently a single missing MLS group causes the entire batch to fail. Is that intended? If not, we can make this best-effort (skip missing and return the rest) or add a strict/lenient mode.
145-160: LGTM: Whitenoise wrappers reflect the new account-pubkey APIThin delegates are clear and consistent.
300-307: LGTM: idempotency testVerifies type preservation across repeated calls with differing hints.
343-355: LGTM: default Group creation via MLS-backed lookupTest ensures DB creation uses MLS name-derived inference.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes