feat(comms): channel invite sends email for non-existing user#2468
Merged
feat(comms): channel invite sends email for non-existing user#2468
Conversation
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughRefactors notification dispatch to call NotificationIngress directly (removing NotificationMsg/NotifEvent), adds ChannelInviteMetadata and invite email template, moves NotificationTitle trait into the notification crate, updates model re-exports and OpenAPI schema, adds tests and small Cargo dependency changes, and extracts MacroUserId::email_str. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
seanaye
force-pushed
the
seanaye/feat/email-channel-invite
branch
from
d21f227 to
4b1e607
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/cloud-storage/comms_service/src/notification.rs (1)
272-340: 🧹 Nitpick | 🔵 TrivialAdd
#[tracing::instrument(err)]to this public async function.Per coding guidelines, functions returning
Resultshould includeerrin thetracing::instrumentattribute.Proposed fix
+#[tracing::instrument(err, skip(api_context, participants, message, mentions))] pub async fn dispatch_notifications_for_message( api_context: &AppState, channel_id: &Uuid,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/comms_service/src/notification.rs` around lines 272 - 340, Add the tracing attribute to the function declaration for dispatch_notifications_for_message by annotating it with #[tracing::instrument(err)] immediately above the pub async fn line; ensure the tracing::instrument macro is in scope (use tracing) and, if any non-Debug parameters cause compile errors, add skip= for those specific parameters in the same attribute (e.g., #[tracing::instrument(err, skip(api_context, message))]) so the function compiles.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/cloud-storage/comms_service/src/api/channels/add_participants.rs`:
- Around line 98-116: The inline comment "There should always be participants,
but better safe than sorry" is stale because no guard checks req.participants
before calling comms_notification::dispatch_notifications_for_invite; remove or
update that comment to accurately reflect current behavior (either delete it or
replace it with a note that notifications are dispatched unconditionally) near
the CommonChannelMetadata creation and the dispatch_notifications_for_invite
call (referencing CommonChannelMetadata, dispatch_notifications_for_invite, and
req.participants).
In `@rust/cloud-storage/comms_service/src/notification.rs`:
- Around line 46-49: The ChannelMessageEvent::send async method lacks tracing
instrumentation; add #[tracing::instrument(err, skip(self, ingress))] (or skip
only fields that don't implement Debug) above the impl's send function to
capture errors automatically while excluding non-Debug references like self and
ingress; ensure the attribute is applied to async fn send(&self, ingress: &impl
NotificationIngress) -> anyhow::Result<()> so failures are recorded by tracing
without trying to format non-Debug types.
- Around line 235-267: Add an early return when there are no recipients to avoid
calling tokio::try_join! with empty sets: before the tokio::try_join! that
invokes api_context.notification_ingress_service.send_notification with
SendNotificationRequestBuilder (building ChannelInviteMetadata for
existing_users and not_existing_users), check if both existing_users.is_empty()
and not_existing_users.is_empty() and return early (e.g., Ok(()) or the
function's appropriate success value) to skip building/sending the
notifications.
- Around line 208-270: Add the tracing attribute to the public async function
dispatch_notifications_for_invite so errors are recorded: place
#[tracing::instrument(err)] immediately above the pub async fn
dispatch_notifications_for_invite(...) signature (this function returns
anyhow::Result<()>); ensure the attribute is imported or referenced correctly so
the macro expands (tracing::instrument) and that existing behavior and
parameters remain unchanged.
In `@rust/cloud-storage/invite_email/src/lib.rs`:
- Around line 146-164: The APNS payload is rebuilding the title/body instead of
reusing the existing NotificationTitle implementation on ChannelInviteMetadata;
update the as_apns() implementation to call
ChannelInviteMetadata::format_title(...) and format_body(...) (using the same
_sender_id handling as other callers) and propagate or handle the Result so the
APNS title/body come from those methods rather than duplicating the formatting;
also apply the same change to the other similar block referenced (the second
occurrence that currently hand-builds title/body).
- Around line 136-175: The email subject and template use the raw inviter id
(invited_by.as_ref()) which can include internal prefixes like "macro|"; derive
a display-safe sender string the same way format_title does (e.g., call
invited_by.email_part().email_str().to_string()) and use that sanitized sender
for the EmailContent.subject and also inject/replace the inviter value passed
into the template render so the rendered body and subject both show the cleaned
sender; update ChannelInviteMetadata::format_email to compute this sanitized
sender and reuse it for both subject and rendering instead of
invited_by.as_ref().
---
Outside diff comments:
In `@rust/cloud-storage/comms_service/src/notification.rs`:
- Around line 272-340: Add the tracing attribute to the function declaration for
dispatch_notifications_for_message by annotating it with
#[tracing::instrument(err)] immediately above the pub async fn line; ensure the
tracing::instrument macro is in scope (use tracing) and, if any non-Debug
parameters cause compile errors, add skip= for those specific parameters in the
same attribute (e.g., #[tracing::instrument(err, skip(api_context, message))])
so the function compiles.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8630a47a-98ad-41ea-b398-dfd02b68afc7
⛔ Files ignored due to path filters (1)
rust/cloud-storage/Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (11)
rust/cloud-storage/comms_service/Cargo.tomlrust/cloud-storage/comms_service/src/api/channels/add_participants.rsrust/cloud-storage/comms_service/src/notification.rsrust/cloud-storage/email_formatting/src/lib.rsrust/cloud-storage/email_formatting/templates/digest.htmlrust/cloud-storage/invite_email/Cargo.tomlrust/cloud-storage/invite_email/src/lib.rsrust/cloud-storage/invite_email/templates/invite_to_channel.htmlrust/cloud-storage/model_notifications/src/lib.rsrust/cloud-storage/model_notifications/src/metadata.rsrust/cloud-storage/notification/src/domain/models.rs
Comment on lines
46
to
+49
| impl ChannelMessageEvent<'_> { | ||
| fn generate_notifications(&self) -> Vec<NotificationMsg> { | ||
| let mut notifications: Vec<NotificationMsg> = vec![]; | ||
| async fn send(&self, ingress: &impl NotificationIngress) -> anyhow::Result<()> { | ||
| let entity = || EntityType::Channel.with_entity_string(self.channel_id.to_string()); | ||
| let sender = || Some(self.message.sender_id.clone()); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding #[tracing::instrument(err)] to this method.
Per coding guidelines, functions returning Result should have #[tracing::instrument(err)]. While the method has references that may complicate instrumentation, you could use skip to exclude non-Debug fields.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/comms_service/src/notification.rs` around lines 46 - 49,
The ChannelMessageEvent::send async method lacks tracing instrumentation; add
#[tracing::instrument(err, skip(self, ingress))] (or skip only fields that don't
implement Debug) above the impl's send function to capture errors automatically
while excluding non-Debug references like self and ingress; ensure the attribute
is applied to async fn send(&self, ingress: &impl NotificationIngress) ->
anyhow::Result<()> so failures are recorded by tracing without trying to format
non-Debug types.
Comment on lines
208
to
270
| pub async fn dispatch_notifications_for_invite( | ||
| api_context: &AppState, | ||
| channel_id: &Uuid, | ||
| invited_by_user_id: &MacroUserIdStr<'static>, | ||
| recipient_user_ids: Vec<String>, | ||
| common: CommonChannelMetadata, | ||
| ) -> anyhow::Result<()> { | ||
| let parsed_recipients: Vec<_> = recipient_user_ids | ||
| .iter() | ||
| .filter_map(|id| MacroUserIdStr::parse_from_str(id).ok()) | ||
| .map(|u| u.0) | ||
| .collect(); | ||
|
|
||
| let sender_profile_picture_url = | ||
| get_sender_profile_picture_url(&api_context.db, invited_by_user_id).await; | ||
|
|
||
| let event = ChannelInviteEvent { | ||
| channel_id, | ||
| invited_by_user_id, | ||
| recipient_user_ids: &recipient_user_ids, | ||
| common: &common, | ||
| }; | ||
|
|
||
| let mut notifications = event.generate_notifications(); | ||
| for n in &mut notifications { | ||
| set_sender_profile_picture( | ||
| &mut n.notification_event, | ||
| sender_profile_picture_url.clone(), | ||
| ); | ||
| } | ||
| let existing_users: HashSet<String> = | ||
| macro_db_client::user::get_all::get_existing_users(&api_context.db, &parsed_recipients) | ||
| .await? | ||
| .into_iter() | ||
| .collect(); | ||
|
|
||
| for notification in notifications { | ||
| send_notification_queue_message(&*api_context.notification_ingress_service, notification) | ||
| .await?; | ||
| } | ||
| let (existing_users, not_existing_users): (HashSet<_>, HashSet<_>) = parsed_recipients | ||
| .into_iter() | ||
| .map(MacroUserIdStr) | ||
| .partition(|id| existing_users.contains(id.as_ref())); | ||
|
|
||
| let _ = tokio::try_join!( | ||
| api_context.notification_ingress_service.send_notification( | ||
| SendNotificationRequestBuilder { | ||
| notification_entity: EntityType::Channel.with_entity_string(channel_id.to_string()), | ||
| notification: ChannelInviteMetadata { | ||
| invited_by: invited_by_user_id.clone(), | ||
| channel_name: common.channel_name.clone(), | ||
| sender_profile_picture_url: sender_profile_picture_url.clone(), | ||
| }, | ||
| sender_id: Some(invited_by_user_id.copied().into_owned()), | ||
| recipient_ids: existing_users, | ||
| } | ||
| .into_request() | ||
| .with_apns() | ||
| .with_conn_gateway(), | ||
| ), | ||
| api_context.notification_ingress_service.send_notification( | ||
| SendNotificationRequestBuilder { | ||
| notification_entity: EntityType::Channel.with_entity_string(channel_id.to_string()), | ||
| notification: ChannelInviteMetadata { | ||
| invited_by: invited_by_user_id.clone(), | ||
| channel_name: common.channel_name.clone(), | ||
| sender_profile_picture_url, | ||
| }, | ||
| sender_id: Some(invited_by_user_id.copied().into_owned()), | ||
| recipient_ids: not_existing_users, | ||
| } | ||
| .into_request() | ||
| .with_apns() | ||
| .with_conn_gateway(), | ||
| ) | ||
| ) | ||
| .map_err(|e| anyhow::anyhow!("{e:?}"))?; | ||
|
|
||
| Ok(()) | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add #[tracing::instrument(err)] to this public async function.
Per coding guidelines, functions returning Result should include err in the tracing::instrument attribute.
Proposed fix
+#[tracing::instrument(err, skip(api_context))]
pub async fn dispatch_notifications_for_invite(
api_context: &AppState,
channel_id: &Uuid,📝 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.
Suggested change
| pub async fn dispatch_notifications_for_invite( | |
| api_context: &AppState, | |
| channel_id: &Uuid, | |
| invited_by_user_id: &MacroUserIdStr<'static>, | |
| recipient_user_ids: Vec<String>, | |
| common: CommonChannelMetadata, | |
| ) -> anyhow::Result<()> { | |
| let parsed_recipients: Vec<_> = recipient_user_ids | |
| .iter() | |
| .filter_map(|id| MacroUserIdStr::parse_from_str(id).ok()) | |
| .map(|u| u.0) | |
| .collect(); | |
| let sender_profile_picture_url = | |
| get_sender_profile_picture_url(&api_context.db, invited_by_user_id).await; | |
| let event = ChannelInviteEvent { | |
| channel_id, | |
| invited_by_user_id, | |
| recipient_user_ids: &recipient_user_ids, | |
| common: &common, | |
| }; | |
| let mut notifications = event.generate_notifications(); | |
| for n in &mut notifications { | |
| set_sender_profile_picture( | |
| &mut n.notification_event, | |
| sender_profile_picture_url.clone(), | |
| ); | |
| } | |
| let existing_users: HashSet<String> = | |
| macro_db_client::user::get_all::get_existing_users(&api_context.db, &parsed_recipients) | |
| .await? | |
| .into_iter() | |
| .collect(); | |
| for notification in notifications { | |
| send_notification_queue_message(&*api_context.notification_ingress_service, notification) | |
| .await?; | |
| } | |
| let (existing_users, not_existing_users): (HashSet<_>, HashSet<_>) = parsed_recipients | |
| .into_iter() | |
| .map(MacroUserIdStr) | |
| .partition(|id| existing_users.contains(id.as_ref())); | |
| let _ = tokio::try_join!( | |
| api_context.notification_ingress_service.send_notification( | |
| SendNotificationRequestBuilder { | |
| notification_entity: EntityType::Channel.with_entity_string(channel_id.to_string()), | |
| notification: ChannelInviteMetadata { | |
| invited_by: invited_by_user_id.clone(), | |
| channel_name: common.channel_name.clone(), | |
| sender_profile_picture_url: sender_profile_picture_url.clone(), | |
| }, | |
| sender_id: Some(invited_by_user_id.copied().into_owned()), | |
| recipient_ids: existing_users, | |
| } | |
| .into_request() | |
| .with_apns() | |
| .with_conn_gateway(), | |
| ), | |
| api_context.notification_ingress_service.send_notification( | |
| SendNotificationRequestBuilder { | |
| notification_entity: EntityType::Channel.with_entity_string(channel_id.to_string()), | |
| notification: ChannelInviteMetadata { | |
| invited_by: invited_by_user_id.clone(), | |
| channel_name: common.channel_name.clone(), | |
| sender_profile_picture_url, | |
| }, | |
| sender_id: Some(invited_by_user_id.copied().into_owned()), | |
| recipient_ids: not_existing_users, | |
| } | |
| .into_request() | |
| .with_apns() | |
| .with_conn_gateway(), | |
| ) | |
| ) | |
| .map_err(|e| anyhow::anyhow!("{e:?}"))?; | |
| Ok(()) | |
| } | |
| #[tracing::instrument(err, skip(api_context))] | |
| pub async fn dispatch_notifications_for_invite( | |
| api_context: &AppState, | |
| channel_id: &Uuid, | |
| invited_by_user_id: &MacroUserIdStr<'static>, | |
| recipient_user_ids: Vec<String>, | |
| common: CommonChannelMetadata, | |
| ) -> anyhow::Result<()> { | |
| let parsed_recipients: Vec<_> = recipient_user_ids | |
| .iter() | |
| .filter_map(|id| MacroUserIdStr::parse_from_str(id).ok()) | |
| .map(|u| u.0) | |
| .collect(); | |
| let sender_profile_picture_url = | |
| get_sender_profile_picture_url(&api_context.db, invited_by_user_id).await; | |
| let existing_users: HashSet<String> = | |
| macro_db_client::user::get_all::get_existing_users(&api_context.db, &parsed_recipients) | |
| .await? | |
| .into_iter() | |
| .collect(); | |
| let (existing_users, not_existing_users): (HashSet<_>, HashSet<_>) = parsed_recipients | |
| .into_iter() | |
| .map(MacroUserIdStr) | |
| .partition(|id| existing_users.contains(id.as_ref())); | |
| let _ = tokio::try_join!( | |
| api_context.notification_ingress_service.send_notification( | |
| SendNotificationRequestBuilder { | |
| notification_entity: EntityType::Channel.with_entity_string(channel_id.to_string()), | |
| notification: ChannelInviteMetadata { | |
| invited_by: invited_by_user_id.clone(), | |
| channel_name: common.channel_name.clone(), | |
| sender_profile_picture_url: sender_profile_picture_url.clone(), | |
| }, | |
| sender_id: Some(invited_by_user_id.copied().into_owned()), | |
| recipient_ids: existing_users, | |
| } | |
| .into_request() | |
| .with_apns() | |
| .with_conn_gateway(), | |
| ), | |
| api_context.notification_ingress_service.send_notification( | |
| SendNotificationRequestBuilder { | |
| notification_entity: EntityType::Channel.with_entity_string(channel_id.to_string()), | |
| notification: ChannelInviteMetadata { | |
| invited_by: invited_by_user_id.clone(), | |
| channel_name: common.channel_name.clone(), | |
| sender_profile_picture_url, | |
| }, | |
| sender_id: Some(invited_by_user_id.copied().into_owned()), | |
| recipient_ids: not_existing_users, | |
| } | |
| .into_request() | |
| .with_apns() | |
| .with_conn_gateway(), | |
| ) | |
| ) | |
| .map_err(|e| anyhow::anyhow!("{e:?}"))?; | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/comms_service/src/notification.rs` around lines 208 - 270,
Add the tracing attribute to the public async function
dispatch_notifications_for_invite so errors are recorded: place
#[tracing::instrument(err)] immediately above the pub async fn
dispatch_notifications_for_invite(...) signature (this function returns
anyhow::Result<()>); ensure the attribute is imported or referenced correctly so
the macro expands (tracing::instrument) and that existing behavior and
parameters remain unchanged.
Comment on lines
+235
to
+267
| let _ = tokio::try_join!( | ||
| api_context.notification_ingress_service.send_notification( | ||
| SendNotificationRequestBuilder { | ||
| notification_entity: EntityType::Channel.with_entity_string(channel_id.to_string()), | ||
| notification: ChannelInviteMetadata { | ||
| invited_by: invited_by_user_id.clone(), | ||
| channel_name: common.channel_name.clone(), | ||
| sender_profile_picture_url: sender_profile_picture_url.clone(), | ||
| }, | ||
| sender_id: Some(invited_by_user_id.copied().into_owned()), | ||
| recipient_ids: existing_users, | ||
| } | ||
| .into_request() | ||
| .with_apns() | ||
| .with_conn_gateway(), | ||
| ), | ||
| api_context.notification_ingress_service.send_notification( | ||
| SendNotificationRequestBuilder { | ||
| notification_entity: EntityType::Channel.with_entity_string(channel_id.to_string()), | ||
| notification: ChannelInviteMetadata { | ||
| invited_by: invited_by_user_id.clone(), | ||
| channel_name: common.channel_name.clone(), | ||
| sender_profile_picture_url, | ||
| }, | ||
| sender_id: Some(invited_by_user_id.copied().into_owned()), | ||
| recipient_ids: not_existing_users, | ||
| } | ||
| .into_request() | ||
| .with_apns() | ||
| .with_conn_gateway(), | ||
| ) | ||
| ) | ||
| .map_err(|e| anyhow::anyhow!("{e:?}"))?; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider short-circuiting when recipient sets are empty.
If both existing_users and not_existing_users are empty, the function still executes tokio::try_join! with two async calls that process empty recipient lists. Adding an early return would avoid unnecessary work.
Proposed fix
let (existing_users, not_existing_users): (HashSet<_>, HashSet<_>) = parsed_recipients
.into_iter()
.map(MacroUserIdStr)
.partition(|id| existing_users.contains(id.as_ref()));
+ if existing_users.is_empty() && not_existing_users.is_empty() {
+ return Ok(());
+ }
+
let _ = tokio::try_join!(📝 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.
Suggested change
| let _ = tokio::try_join!( | |
| api_context.notification_ingress_service.send_notification( | |
| SendNotificationRequestBuilder { | |
| notification_entity: EntityType::Channel.with_entity_string(channel_id.to_string()), | |
| notification: ChannelInviteMetadata { | |
| invited_by: invited_by_user_id.clone(), | |
| channel_name: common.channel_name.clone(), | |
| sender_profile_picture_url: sender_profile_picture_url.clone(), | |
| }, | |
| sender_id: Some(invited_by_user_id.copied().into_owned()), | |
| recipient_ids: existing_users, | |
| } | |
| .into_request() | |
| .with_apns() | |
| .with_conn_gateway(), | |
| ), | |
| api_context.notification_ingress_service.send_notification( | |
| SendNotificationRequestBuilder { | |
| notification_entity: EntityType::Channel.with_entity_string(channel_id.to_string()), | |
| notification: ChannelInviteMetadata { | |
| invited_by: invited_by_user_id.clone(), | |
| channel_name: common.channel_name.clone(), | |
| sender_profile_picture_url, | |
| }, | |
| sender_id: Some(invited_by_user_id.copied().into_owned()), | |
| recipient_ids: not_existing_users, | |
| } | |
| .into_request() | |
| .with_apns() | |
| .with_conn_gateway(), | |
| ) | |
| ) | |
| .map_err(|e| anyhow::anyhow!("{e:?}"))?; | |
| let (existing_users, not_existing_users): (HashSet<_>, HashSet<_>) = parsed_recipients | |
| .into_iter() | |
| .map(MacroUserIdStr) | |
| .partition(|id| existing_users.contains(id.as_ref())); | |
| if existing_users.is_empty() && not_existing_users.is_empty() { | |
| return Ok(()); | |
| } | |
| let _ = tokio::try_join!( | |
| api_context.notification_ingress_service.send_notification( | |
| SendNotificationRequestBuilder { | |
| notification_entity: EntityType::Channel.with_entity_string(channel_id.to_string()), | |
| notification: ChannelInviteMetadata { | |
| invited_by: invited_by_user_id.clone(), | |
| channel_name: common.channel_name.clone(), | |
| sender_profile_picture_url: sender_profile_picture_url.clone(), | |
| }, | |
| sender_id: Some(invited_by_user_id.copied().into_owned()), | |
| recipient_ids: existing_users, | |
| } | |
| .into_request() | |
| .with_apns() | |
| .with_conn_gateway(), | |
| ), | |
| api_context.notification_ingress_service.send_notification( | |
| SendNotificationRequestBuilder { | |
| notification_entity: EntityType::Channel.with_entity_string(channel_id.to_string()), | |
| notification: ChannelInviteMetadata { | |
| invited_by: invited_by_user_id.clone(), | |
| channel_name: common.channel_name.clone(), | |
| sender_profile_picture_url, | |
| }, | |
| sender_id: Some(invited_by_user_id.copied().into_owned()), | |
| recipient_ids: not_existing_users, | |
| } | |
| .into_request() | |
| .with_apns() | |
| .with_conn_gateway(), | |
| ) | |
| ) | |
| .map_err(|e| anyhow::anyhow!("{e:?}"))?; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/comms_service/src/notification.rs` around lines 235 - 267,
Add an early return when there are no recipients to avoid calling
tokio::try_join! with empty sets: before the tokio::try_join! that invokes
api_context.notification_ingress_service.send_notification with
SendNotificationRequestBuilder (building ChannelInviteMetadata for
existing_users and not_existing_users), check if both existing_users.is_empty()
and not_existing_users.is_empty() and return early (e.g., Ok(()) or the
function's appropriate success value) to skip building/sending the
notifications.
Contributor
Author
There was a problem hiding this comment.
This is also redundant as this is handled downstream already
Contributor
There was a problem hiding this comment.
@seanaye, understood — the notification ingress service handles empty recipient_ids gracefully downstream, so the early-return guard is indeed unnecessary. I'll note this for future reviews.
✏️ Learnings added
Learnt from: seanaye
Repo: macro-inc/macro PR: 2468
File: rust/cloud-storage/comms_service/src/notification.rs:235-267
Timestamp: 2026-04-09T18:14:39.303Z
Learning: In `rust/cloud-storage/comms_service/src/notification.rs`, the `dispatch_notifications_for_invite` function does NOT need an early-return guard when both `existing_users` and `not_existing_users` are empty before calling `tokio::try_join!` with `send_notification`. Empty `recipient_ids` in `SendNotificationRequestBuilder` are handled gracefully downstream by the notification ingress service. Do not flag the absence of this guard as a performance issue or bug.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: seanaye
Repo: macro-inc/macro PR: 2468
File: rust/cloud-storage/comms_service/src/api/channels/add_participants.rs:98-116
Timestamp: 2026-04-09T18:14:01.990Z
Learning: In `rust/cloud-storage/comms_service/src/api/channels/add_participants.rs`, the `dispatch_notifications_for_invite` function handles an empty participant list gracefully downstream. No `!participants.is_empty()` guard is needed at the call site before invoking `comms_notification::dispatch_notifications_for_invite`. Do not flag the absence of this guard as a bug.
Learnt from: seanaye
Repo: macro-inc/macro PR: 2170
File: rust/cloud-storage/document_storage_service/src/api/annotations/edit_comment.rs:72-93
Timestamp: 2026-03-25T18:04:23.696Z
Learning: In the macro repository (Rust, cloud-storage), sender exclusion from notification recipients is handled downstream in the notification ingress service. It is not necessary to filter out the sender at the call site before invoking `send_notification` in `document_storage_service/src/api/annotations/edit_comment.rs` or similar annotation handlers.
Learnt from: seanaye
Repo: macro-inc/macro PR: 2170
File: rust/cloud-storage/document_storage_service/src/api/annotations/mod.rs:141-146
Timestamp: 2026-03-25T18:04:22.762Z
Learning: In the macro repository (Rust backend), sender exclusion for notification recipients is handled downstream in the notification ingress service, not at the point where recipient sets are computed (e.g., `compute_notification_recipients` in `document_storage_service/src/api/annotations/mod.rs`). Do not flag missing sender-filtering in recipient-computation helpers as a bug.
Learnt from: seanaye
Repo: macro-inc/macro PR: 2246
File: rust/cloud-storage/model_notifications/src/lib.rs:228-229
Timestamp: 2026-03-30T14:56:06.115Z
Learning: In `rust/cloud-storage/model_notifications/src/lib.rs`, `NotificationTitle::format_title` and `format_body` are only called for APNS (Apple Push Notification Service) push notification formatting. `InviteToTeam` is an email-only notification and does not support APNS; therefore, returning `Err(report!("not implemented"))` in both `format_title` and `format_body` for the `NotifEvent::InviteToTeam` variant is intentional and correct. Do not flag this as a bug or suggest a fallback.
Learnt from: seanaye
Repo: macro-inc/macro PR: 2246
File: rust/cloud-storage/teams/src/domain/team_service.rs:226-227
Timestamp: 2026-03-30T14:54:37.595Z
Learning: In `rust/cloud-storage/teams/src/domain/team_service.rs`, `MacroUserIdStr::try_from_email(invite.email.as_ref())` is infallible when `invite.email` is of type `Email<Lowercase<'_>>` (a pre-validated email type from `macro_user_id`). The `try_from_email` function merely prepends `"macro|"` to the email string; since `Email` already guarantees a valid email, this always succeeds. Using `.expect("this cannot fail")` here is correct and intentional — do not flag it as a potential panic.
Learnt from: gbirman
Repo: macro-inc/macro PR: 2209
File: rust/cloud-storage/document_text_extractor/src/handler/extract_text_citations.rs:153-156
Timestamp: 2026-03-26T18:22:38.662Z
Learning: When interacting with S3 in this repo, only URL-encode object keys for the `CopySource` parameter used in S3 copy operations (e.g., `CopyObjectCommand` / copy APIs). For S3 `GetObject` calls (e.g., a `get_document_bytes` helper), do NOT URL-encode the key—pass the canonical, unencoded object key directly as the identifier.
seanaye
force-pushed
the
seanaye/feat/email-channel-invite
branch
from
a6da8b2 to
e14e64d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/cloud-storage/comms_service/src/notification.rs`:
- Around line 218-222: The current parsing of recipient_user_ids silently drops
malformed IDs by using filter_map with MacroUserIdStr::parse_from_str(...).ok(),
so some recipients may be skipped; change parsing to collect failures instead of
ignoring them: map each id through MacroUserIdStr::parse_from_str and collect
into a Result<Vec<_>, _> (e.g., collect::<Result<Vec<_>, _>>()?) to propagate an
error, or explicitly log/return an error when parse fails, and then use the
successful Vec (parsed_recipients) only after the error is handled; reference
MacroUserIdStr::parse_from_str, recipient_user_ids, and parsed_recipients when
making the fix.
- Around line 56-79: The code currently stringifies and discards the original
ingress error via .map_err(|e| anyhow::anyhow!("{e:?}")), losing the error
chain; change the error propagation to preserve context by using anyhow::Context
(e.g., call .context("sending notification via ingress") on the Result returned
from send_notification(...).await) instead of wrapping the error with a
formatted string. Apply the same fix in the other send_notification branches so
SendNotificationRequestBuilder/send_notification(...) .await uses .context(...)
(or otherwise forwards the original error) rather than .map_err(|e|
anyhow::anyhow!("{e:?}")).
In `@rust/cloud-storage/comms_service/src/notification/test.rs`:
- Around line 122-219: Add a new async test that exercises
dispatch_notifications_for_invite by constructing a ChannelMessageEvent (or the
specific invite-triggering event) with a mix of existing and non-existing
recipients, send it via MockNotificationIngress::new() and
.send(&ingress).await.unwrap(), then collect ingress.recorded_requests() and
assert there are two send_notification/ChannelInviteMetadata requests: one
routed to the existing-recipient set and one routed to the non-existing (invite)
set; locate the invite requests by matching the notification type/name (e.g.,
"channel_invite" or ChannelInviteMetadata) and assert recipient ids in each
request match the expected partitions.
In `@rust/cloud-storage/invite_email/src/lib.rs`:
- Around line 183-193: The rate limit key uses mutable channel_name which can
collide across different channels; change rate_limit_key so it uses a stable
channel identifier (channel_id) instead of channel_name. Update the signature or
struct so rate_limit_key(&self) either reads a channel_id field (preferred: add
a channel_id member to the struct) or accepts channel_id as a parameter, then
call RateLimitKey::builder(&Self::TYPE_NAME).append(&self.channel_id).finish()
(you may keep channel_name for human logs but must use channel_id for the
rate-limit key).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3363df8b-2569-4eeb-a360-abe47417299d
⛔ Files ignored due to path filters (5)
js/app/packages/service-clients/service-notification/generated/schemas/channelInviteMetadata.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/channelInviteMetadataAllOf.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/channelInviteMetadataAllOfSenderProfilePictureUrl.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/channelInviteMetadataSenderProfilePictureUrl.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/index.tsis excluded by!**/generated/**
📒 Files selected for processing (6)
js/app/packages/service-clients/service-notification/openapi.jsonrust/cloud-storage/comms_service/src/notification.rsrust/cloud-storage/comms_service/src/notification/test.rsrust/cloud-storage/invite_email/src/lib.rsrust/cloud-storage/invite_email/templates/invite_to_channel.htmlrust/cloud-storage/macro_user_id/src/user_id.rs
Comment on lines
+183
to
+193
| fn rate_limit_config() -> RateLimitConfig { | ||
| RateLimitConfig { | ||
| max_count: 1, | ||
| window: Duration::from_hours(24 * 7), | ||
| } | ||
| } | ||
|
|
||
| fn rate_limit_key(&self) -> RateLimitKey { | ||
| RateLimitKey::builder(&Self::TYPE_NAME) | ||
| .append(&self.channel_name) | ||
| .finish() |
Contributor
There was a problem hiding this comment.
Rate limiting on channel_name will collide across unrelated channels.
#general-style names are not unique, so this key can suppress a legitimate invite for a different channel that happens to share the same name during the 7-day window. The call site in rust/cloud-storage/comms_service/src/notification.rs already has the channel_id; this metadata needs a stable channel identifier in the rate-limit key.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/invite_email/src/lib.rs` around lines 183 - 193, The rate
limit key uses mutable channel_name which can collide across different channels;
change rate_limit_key so it uses a stable channel identifier (channel_id)
instead of channel_name. Update the signature or struct so rate_limit_key(&self)
either reads a channel_id field (preferred: add a channel_id member to the
struct) or accepts channel_id as a parameter, then call
RateLimitKey::builder(&Self::TYPE_NAME).append(&self.channel_id).finish() (you
may keep channel_name for human logs but must use channel_id for the rate-limit
key).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/app/packages/service-clients/service-notification/openapi.json`:
- Around line 1104-1122: The diff removed the composed schema and thereby
dropped channelType from ChannelInviteMetadata, breaking consumers expecting
that property; restore ChannelInviteMetadata to compose/extend
CommonChannelMetadata (or re-add the channelType property) so generated clients
include channelType — update the ChannelInviteMetadata definition to either use
allOf with CommonChannelMetadata or explicitly include the channelType property
(matching the type/signature in CommonChannelMetadata) so
ChannelInviteMetadata.channelType is present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ebd66bc5-f962-4d44-a157-8e551b256e14
⛔ Files ignored due to path filters (6)
js/app/packages/service-clients/service-notification/generated/schemas/channelInviteMetadata.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/channelInviteMetadataAllOf.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/channelInviteMetadataAllOfSenderProfilePictureUrl.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/channelInviteMetadataSenderProfilePictureUrl.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/index.tsis excluded by!**/generated/**rust/cloud-storage/Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (14)
js/app/packages/service-clients/service-notification/openapi.jsonrust/cloud-storage/comms_service/Cargo.tomlrust/cloud-storage/comms_service/src/api/channels/add_participants.rsrust/cloud-storage/comms_service/src/notification.rsrust/cloud-storage/comms_service/src/notification/test.rsrust/cloud-storage/email_formatting/src/lib.rsrust/cloud-storage/email_formatting/templates/digest.htmlrust/cloud-storage/invite_email/Cargo.tomlrust/cloud-storage/invite_email/src/lib.rsrust/cloud-storage/invite_email/templates/invite_to_channel.htmlrust/cloud-storage/macro_user_id/src/user_id.rsrust/cloud-storage/model_notifications/src/lib.rsrust/cloud-storage/model_notifications/src/metadata.rsrust/cloud-storage/notification/src/domain/models.rs
Comment on lines
1104
to
1122
| "ChannelInviteMetadata": { | ||
| "allOf": [ | ||
| { | ||
| "$ref": "#/components/schemas/CommonChannelMetadata" | ||
| "type": "object", | ||
| "description": "Metadata for when a user is invited to a channel.", | ||
| "required": ["invitedBy"], | ||
| "properties": { | ||
| "channelName": { | ||
| "type": "string", | ||
| "description": "The name of the channel" | ||
| }, | ||
| { | ||
| "type": "object", | ||
| "required": ["invitedBy"], | ||
| "properties": { | ||
| "invitedBy": { | ||
| "type": "string" | ||
| }, | ||
| "senderProfilePictureUrl": { | ||
| "type": ["string", "null"] | ||
| } | ||
| } | ||
| "invitedBy": { | ||
| "type": "string", | ||
| "description": "The user who sent the invitation" | ||
| }, | ||
| "senderProfilePictureUrl": { | ||
| "type": ["string", "null"], | ||
| "description": "The sender's profile picture URL, if available." | ||
| } | ||
| ], | ||
| "description": "Metadata for when a user is invited to a channel" | ||
| } | ||
| }, |
Contributor
There was a problem hiding this comment.
Restore channelType on ChannelInviteMetadata to avoid a breaking API contract.
Line 1104 replaces the composed schema with a standalone object, which removes channelType inherited from CommonChannelMetadata. That changes generated client types and can break existing consumers expecting ChannelInviteMetadata.channelType.
🔧 Proposed OpenAPI fix
"ChannelInviteMetadata": {
- "type": "object",
- "description": "Metadata for when a user is invited to a channel.",
- "required": ["invitedBy"],
- "properties": {
- "channelName": {
- "type": "string",
- "description": "The name of the channel"
- },
- "invitedBy": {
- "type": "string",
- "description": "The user who sent the invitation"
- },
- "senderProfilePictureUrl": {
- "type": ["string", "null"],
- "description": "The sender's profile picture URL, if available."
- }
- }
+ "allOf": [
+ {
+ "$ref": "#/components/schemas/CommonChannelMetadata"
+ },
+ {
+ "type": "object",
+ "description": "Metadata for when a user is invited to a channel.",
+ "required": ["invitedBy"],
+ "properties": {
+ "invitedBy": {
+ "type": "string",
+ "description": "The user who sent the invitation"
+ },
+ "senderProfilePictureUrl": {
+ "type": ["string", "null"],
+ "description": "The sender's profile picture URL, if available."
+ }
+ }
+ }
+ ]
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/service-clients/service-notification/openapi.json` around
lines 1104 - 1122, The diff removed the composed schema and thereby dropped
channelType from ChannelInviteMetadata, breaking consumers expecting that
property; restore ChannelInviteMetadata to compose/extend CommonChannelMetadata
(or re-add the channelType property) so generated clients include channelType —
update the ChannelInviteMetadata definition to either use allOf with
CommonChannelMetadata or explicitly include the channelType property (matching
the type/signature in CommonChannelMetadata) so
ChannelInviteMetadata.channelType is present.
e14e64d to
7c74e3b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
js/app/packages/service-clients/service-notification/openapi.json (1)
1105-1122:⚠️ Potential issue | 🟠 MajorRestore
channelTypeonChannelInviteMetadatato prevent a breaking contract change.At Line [1105], switching from composed schema to standalone object drops
channelTypeinherited fromCommonChannelMetadata, which changes generated client types forchannel_inviteconsumers.🔧 Proposed OpenAPI fix
"ChannelInviteMetadata": { - "type": "object", - "description": "Metadata for when a user is invited to a channel.", - "required": ["invitedBy"], - "properties": { - "channelName": { - "type": "string", - "description": "The name of the channel" - }, - "invitedBy": { - "type": "string", - "description": "The user who sent the invitation" - }, - "senderProfilePictureUrl": { - "type": ["string", "null"], - "description": "The sender's profile picture URL, if available." - } - } + "allOf": [ + { + "$ref": "#/components/schemas/CommonChannelMetadata" + }, + { + "type": "object", + "description": "Metadata for when a user is invited to a channel.", + "required": ["invitedBy"], + "properties": { + "invitedBy": { + "type": "string", + "description": "The user who sent the invitation" + }, + "senderProfilePictureUrl": { + "type": ["string", "null"], + "description": "The sender's profile picture URL, if available." + } + } + } + ] },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/service-clients/service-notification/openapi.json` around lines 1105 - 1122, The ChannelInviteMetadata schema lost the inherited channelType from CommonChannelMetadata when it was converted to a standalone object; restore compatibility by reintroducing the inheritance or explicit property: update the ChannelInviteMetadata definition to either use an allOf referencing CommonChannelMetadata plus the existing object properties (so channelType is inherited), or add the channelType property back with the same type/enum as in CommonChannelMetadata, ensuring required/description match the original contract.rust/cloud-storage/invite_email/src/lib.rs (1)
190-193:⚠️ Potential issue | 🟠 MajorInclude channel identity in the rate-limit key.
Using only
invited_bycollapses every channel invite from the same sender into one weekly bucket. That will suppress a legitimate invite when the same person invites someone to two different channels in the same 7-day window.🛠️ Suggested direction
pub struct ChannelInviteMetadata { + /// The unique identifier of the channel. + #[serde(alias = "channel_id")] + pub channel_id: Uuid, + /// The user who sent the invitation #[serde(alias = "invited_by")] #[schema(value_type = String)] pub invited_by: MacroUserIdStr<'static>, @@ fn rate_limit_key(&self) -> RateLimitKey { RateLimitKey::builder(&Self::TYPE_NAME) + .append(&self.channel_id) .append(&self.invited_by) .finish() } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/invite_email/src/lib.rs` around lines 190 - 193, The rate_limit_key() implementation currently only appends self.invited_by, causing all invites from the same sender to share a weekly bucket; update RateLimitKey::builder(&Self::TYPE_NAME)...finish() in the rate_limit_key method to also append the channel identifier (e.g. .append(&self.channel_id) or the appropriate channel field on the struct) so the key includes both inviter and channel, ensuring invites to different channels produce distinct rate-limit keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/cloud-storage/invite_email/src/lib.rs`:
- Around line 151-159: In the NotificationTitle impl for ChannelInviteMetadata,
replace the manual email extraction (calling self.invited_by.email_part() /
email.email_str()) in format_title with the existing helper
self.sender_display() so the sender string is centralized; keep the same
formatted result ("{sender} invited you to join #{}") and return it as before
(Result<String, rootcause::Report>) using the value from self.sender_display().
---
Duplicate comments:
In `@js/app/packages/service-clients/service-notification/openapi.json`:
- Around line 1105-1122: The ChannelInviteMetadata schema lost the inherited
channelType from CommonChannelMetadata when it was converted to a standalone
object; restore compatibility by reintroducing the inheritance or explicit
property: update the ChannelInviteMetadata definition to either use an allOf
referencing CommonChannelMetadata plus the existing object properties (so
channelType is inherited), or add the channelType property back with the same
type/enum as in CommonChannelMetadata, ensuring required/description match the
original contract.
In `@rust/cloud-storage/invite_email/src/lib.rs`:
- Around line 190-193: The rate_limit_key() implementation currently only
appends self.invited_by, causing all invites from the same sender to share a
weekly bucket; update RateLimitKey::builder(&Self::TYPE_NAME)...finish() in the
rate_limit_key method to also append the channel identifier (e.g.
.append(&self.channel_id) or the appropriate channel field on the struct) so the
key includes both inviter and channel, ensuring invites to different channels
produce distinct rate-limit keys.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a551b025-8326-48c0-ae67-84a106264646
⛔ Files ignored due to path filters (5)
js/app/packages/service-clients/service-notification/generated/schemas/channelInviteMetadata.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/channelInviteMetadataAllOf.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/channelInviteMetadataAllOfSenderProfilePictureUrl.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/channelInviteMetadataSenderProfilePictureUrl.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/index.tsis excluded by!**/generated/**
📒 Files selected for processing (2)
js/app/packages/service-clients/service-notification/openapi.jsonrust/cloud-storage/invite_email/src/lib.rs
Comment on lines
+151
to
+159
| impl NotificationTitle for ChannelInviteMetadata { | ||
| fn format_title( | ||
| &self, | ||
| _sender_id: Option<MacroUserIdStr<'_>>, | ||
| ) -> Result<String, rootcause::Report> { | ||
| let email = self.invited_by.email_part(); | ||
| let sender = email.email_str(); | ||
| Ok(format!( | ||
| "{sender} invited you to join #{}", |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Reuse sender_display() in format_title().
That helper already centralizes the user-facing sender string, so using it here avoids future drift between email and push copy.
♻️ Proposed cleanup
fn format_title(
&self,
_sender_id: Option<MacroUserIdStr<'_>>,
) -> Result<String, rootcause::Report> {
- let email = self.invited_by.email_part();
- let sender = email.email_str();
+ let sender = self.sender_display();
Ok(format!(
"{sender} invited you to join #{}",
self.channel_name
))
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/invite_email/src/lib.rs` around lines 151 - 159, In the
NotificationTitle impl for ChannelInviteMetadata, replace the manual email
extraction (calling self.invited_by.email_part() / email.email_str()) in
format_title with the existing helper self.sender_display() so the sender string
is centralized; keep the same formatted result ("{sender} invited you to join
#{}") and return it as before (Result<String, rootcause::Report>) using the
value from self.sender_display().
This was referenced Apr 9, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds the capability for comms service to construct a email notification if a sent channel invite notification is going to a non-existent user