feat(dss): add more comment notification types#2170
Merged
Conversation
seanaye
force-pushed
the
seanaye/feat/more-document-comment-notifs
branch
from
e319e0c to
584237e
Compare
seanaye
force-pushed
the
seanaye/feat/more-document-comment-notifs
branch
from
584237e to
d86754c
Compare
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR introduces two notification types — Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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: 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/authentication_service/src/main.rs`:
- Line 179: The code mistakenly uses dbg!(config.notification_queue.clone())
when initializing queue_url, which prints to stderr; replace the dbg! wrapper
with the plain value config.notification_queue.clone() so queue_url is set
without debug output, and remove any now-unused dbg imports if present; target
the queue_url initialization where config.notification_queue is cloned.
In
`@rust/cloud-storage/document_storage_service/src/api/annotations/create_comment.rs`:
- Around line 140-144: The code re-parses IDs from
recipients.thread_reply_recipients into participant_ids using
MacroUserIdStr::parse_from_str even though compute_notification_recipients
already validated and normalized them; change compute_notification_recipients to
return the normalized IDs (e.g., Vec<MacroUserIdStr<'static>> or
HashSet<MacroUserIdStr<'static>>) so callers like the create_comment path can
use the returned collection directly, and update the create_comment code to
accept and use that collection (replace the filter_map parse block that builds
participant_ids) to eliminate the redundant parsing and unnecessary allocations.
In `@rust/cloud-storage/model_notifications/src/lib.rs`:
- Around line 171-176: The match in set_sender_profile_picture currently uses a
wildcard arm and misses the new NotifEvent variants
RepliedToDocumentCommentThread and CommentedOnDocument, so add explicit match
arms for these variants in set_sender_profile_picture to populate
sender_profile_picture_url (same enrichment logic used for other user-related
variants) instead of relying on the wildcard; update the NotifEvent match to
enumerate all handled variants (including
RepliedToDocumentCommentThread(RepliedToDocumentCommentThreadMetadata) and
CommentedOnDocument(CommentedOnDocumentMetadata)) and ensure
sender_profile_picture_url is set appropriately for each.
In `@rust/cloud-storage/model_notifications/src/metadata.rs`:
- Around line 796-799: The code currently treats self.file_type as required in
format_title, causing format_title/as_apns to error and return None when
file_type is None; update the logic in format_title (and the parallel block used
by MentionedInDocumentCommentMetadata) to accept an Option<&str> or to use a
fallback when file_type is None (for example, omit the extension or use an empty
string) so that as_apns returns a valid notification instead of None; locate the
use of self.file_type (the let file_type_str = ... line) and change the handling
to gracefully handle None rather than failing with report!, ensuring APNS
payloads are produced even when file_type is missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fc1744d9-edc5-42b3-94b6-7bc7defd3070
⛔ Files ignored due to path filters (27)
flake.lockis excluded by!**/*.lockjs/app/packages/service-clients/service-notification/generated/schemas/commentedOnDocumentMetadata.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/commentedOnDocumentMetadataFileType.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/commentedOnDocumentMetadataSenderProfilePictureUrl.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/index.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/notifEvent.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/notifEventOneOfNine.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/notifEventOneOfNineTag.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/notifEventOneOfOnefive.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/notifEventOneOfOnefiveTag.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/notifEventOneOfOnenine.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/notifEventOneOfOnenineTag.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/notifEventOneOfOneone.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/notifEventOneOfOneoneTag.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/notifEventOneOfOneseven.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/notifEventOneOfOnesevenTag.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/notifEventOneOfOnethree.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/notifEventOneOfOnethreeTag.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/notifEventOneOfSeven.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/notifEventOneOfSevenTag.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/notifEventOneOfTwoone.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/notifEventOneOfTwooneTag.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/notifEventOneOfTwothree.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/notifEventOneOfTwothreeTag.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/repliedToDocumentCommentThreadMetadata.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/repliedToDocumentCommentThreadMetadataFileType.tsis excluded by!**/generated/**js/app/packages/service-clients/service-notification/generated/schemas/repliedToDocumentCommentThreadMetadataSenderProfilePictureUrl.tsis excluded by!**/generated/**
📒 Files selected for processing (15)
js/app/packages/core/component/LexicalMarkdown/component/core/LinkWithPreview.tsxjs/app/packages/entity/src/extractors-notification/notification-description-helpers.tsjs/app/packages/entity/src/extractors-notification/notification-icon.tsxjs/app/packages/entity/src/utils/notification.tsjs/app/packages/notifications/notification-metadata.tsjs/app/packages/notifications/notification-navigation.tsjs/app/packages/notifications/notification-preview.tsjs/app/packages/service-clients/service-notification/openapi.jsonrust/cloud-storage/authentication_service/src/main.rsrust/cloud-storage/document_storage_service/src/api/annotations/create_comment.rsrust/cloud-storage/document_storage_service/src/api/annotations/mod.rsrust/cloud-storage/document_storage_service/src/api/annotations/test.rsrust/cloud-storage/model_notifications/src/lib.rsrust/cloud-storage/model_notifications/src/metadata.rsrust/cloud-storage/notification_service/src/api/swagger.rs
rust/cloud-storage/document_storage_service/src/api/annotations/create_comment.rs
Outdated
Show resolved
Hide resolved
seanaye
force-pushed
the
seanaye/feat/more-document-comment-notifs
branch
2 times, most recently
from
ddc6b4c to
d4bf398
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/document_storage_service/src/api/annotations/edit_comment.rs`:
- Around line 68-84: Add a short explanatory comment in edit_comment_handler (in
edit_comment.rs) near the notif_ctx/build_mention_notif call stating that only
mention notifications are intentionally sent on edit (no thread-reply or
document-owner notifications) to avoid spamming users, mirroring the
documentation style used in create_comment_handler; place the comment
immediately above the notif_ctx or request variable so future readers see the
rationale next to the notification construction logic.
In `@rust/cloud-storage/document_storage_service/src/api/annotations/mod.rs`:
- Around line 128-181: compute_notification_recipients currently returns
mention_recipients as Vec<String> while thread_reply_recipients is
HashSet<MacroUserIdStr<'static>>, forcing callers (e.g., build_mention_notif) to
re-parse; change mention_recipients to HashSet<MacroUserIdStr<'static>> so
parsed/validated IDs are returned directly: when building mention_recipients,
parse each id into MacroUserIdStr (use the same parse/try_from logic used for
thread_comment_owners), insert the parsed MacroUserIdStr into the HashSet, and
use parsed.as_ref().to_string() (or keep a separate notified HashSet of strings)
to populate the notified set and dedupe; update the NotificationRecipients
struct/type to reflect mention_recipients: HashSet<MacroUserIdStr<'static>> and
adjust callers (e.g., build_mention_notif) to accept the HashSet without
re-parsing.
In `@rust/cloud-storage/document_storage_service/src/api/annotations/test.rs`:
- Around line 30-56: The test
mentioned_user_who_is_also_thread_participant_gets_only_mention is asserting
against the raw string bob ("macro|bob@test.com") but
compute_notification_recipients normalizes IDs (via
MacroUserIdStr::parse_from_str/user_id), so the contains/assert checks can fail;
update the assertions to compare against the normalized ID (use the same
normalization helper used elsewhere—e.g., user_id(bob) or
MacroUserIdStr::parse_from_str(bob).unwrap()) when checking
result.mention_recipients and result.thread_reply_recipients so both sides use
the same canonical form.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0dbbb455-9345-4863-baea-3a80b0a52af8
📒 Files selected for processing (6)
rust/cloud-storage/comms_service/src/notification.rsrust/cloud-storage/document_storage_service/src/api/annotations/create_comment.rsrust/cloud-storage/document_storage_service/src/api/annotations/edit_comment.rsrust/cloud-storage/document_storage_service/src/api/annotations/mod.rsrust/cloud-storage/document_storage_service/src/api/annotations/test.rsrust/cloud-storage/model_notifications/src/metadata.rs
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
rust/cloud-storage/document_storage_service/src/api/annotations/mod.rs (1)
218-305: 🧹 Nitpick | 🔵 TrivialType inconsistency between
mention_recipients(Vec) andthread_reply_recipients(HashSet).The past review noted this inconsistency but it's still present.
build_mention_notifre-parses the string IDs at lines 247-250, which duplicates work already done incompute_notification_recipients. This isn't a bug, but changingmention_recipientstoHashSet<MacroUserIdStr<'static>>would eliminate the redundant parsing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/document_storage_service/src/api/annotations/mod.rs` around lines 218 - 305, Summary: eliminate redundant parsing by switching mention recipients from Vec<String> to HashSet<MacroUserIdStr<'static>> and feeding that into build_mention_notif. Update compute_notification_recipients (or wherever mention_recipients is produced) to return HashSet<MacroUserIdStr<'static>> instead of Vec<String>, change CommentNotifContext::build_mention_notif signature to accept that HashSet (e.g., &HashSet<MacroUserIdStr<'static>> or take ownership), and remove the filter_map parsing block that builds recipient_ids (lines creating recipient_ids from mentions); instead assign or clone the provided HashSet into SendNotificationRequestBuilder.recipient_ids and keep constructing MentionedInDocumentCommentMetadata as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@rust/cloud-storage/document_storage_service/src/api/annotations/mod.rs`:
- Around line 218-305: Summary: eliminate redundant parsing by switching mention
recipients from Vec<String> to HashSet<MacroUserIdStr<'static>> and feeding that
into build_mention_notif. Update compute_notification_recipients (or wherever
mention_recipients is produced) to return HashSet<MacroUserIdStr<'static>>
instead of Vec<String>, change CommentNotifContext::build_mention_notif
signature to accept that HashSet (e.g., &HashSet<MacroUserIdStr<'static>> or
take ownership), and remove the filter_map parsing block that builds
recipient_ids (lines creating recipient_ids from mentions); instead assign or
clone the provided HashSet into SendNotificationRequestBuilder.recipient_ids and
keep constructing MentionedInDocumentCommentMetadata as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4bdbd8e8-c532-4fea-bf53-3e6618c20dec
📒 Files selected for processing (6)
rust/cloud-storage/comms_service/src/notification.rsrust/cloud-storage/document_storage_service/src/api/annotations/create_comment.rsrust/cloud-storage/document_storage_service/src/api/annotations/edit_comment.rsrust/cloud-storage/document_storage_service/src/api/annotations/mod.rsrust/cloud-storage/document_storage_service/src/api/annotations/test.rsrust/cloud-storage/model_notifications/src/metadata.rs
seanaye
force-pushed
the
seanaye/feat/more-document-comment-notifs
branch
from
d4bf398 to
b634943
Compare
seanaye
force-pushed
the
seanaye/feat/more-document-comment-notifs
branch
from
b634943 to
b57e7a6
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/document_storage_service/src/api/annotations/edit_comment.rs`:
- Around line 72-93: The edit path currently builds recipient_ids directly from
users and can include the editor themself; modify the edit flow around
CommentNotifContext::build_mention_notif so you normalize and remove the sender
(use the same normalization used by compute_notification_recipients or
MacroUserIdStr::try_from on user_id) from recipient_ids before creating the
request, and if the resulting recipient_ids set is empty, do not call
send_notification (skip building/sending the request). Ensure the change touches
the variables recipient_ids, notif_ctx, and the code path that calls
build_mention_notif()/into_request()/with_apns()/with_conn_gateway() so the
editor is excluded and sending is skipped when there are no recipients.
In `@rust/cloud-storage/document_storage_service/src/api/annotations/mod.rs`:
- Around line 141-146: This code collects normalized mention_recipients from
mentioned_user_ids and then extends notified with them, but it doesn't exclude
the comment sender; update the logic that builds mention_recipients (the
conversion using MacroUserIdStr::try_from on mentioned_user_ids) to filter out
the sender's ID (compare against the sender's MacroUserIdStr or raw ID before
collecting) so the sender is not added to the mention_recipients HashSet and
thus not pushed into notified in create_comment_handler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 09a1d94d-a4cd-48da-926a-eab3bbdce52b
📒 Files selected for processing (6)
rust/cloud-storage/comms_service/src/notification.rsrust/cloud-storage/document_storage_service/src/api/annotations/create_comment.rsrust/cloud-storage/document_storage_service/src/api/annotations/edit_comment.rsrust/cloud-storage/document_storage_service/src/api/annotations/mod.rsrust/cloud-storage/document_storage_service/src/api/annotations/test.rsrust/cloud-storage/model_notifications/src/metadata.rs
rust/cloud-storage/document_storage_service/src/api/annotations/edit_comment.rs
Show resolved
Hide resolved
Comment on lines
+141
to
+146
| // 1. Mention recipients — normalize to MacroUserIdStr format for consistent comparison | ||
| let mention_recipients: HashSet<MacroUserIdStr<'static>> = mentioned_user_ids | ||
| .iter() | ||
| .filter_map(|id| MacroUserIdStr::parse_from_str(id).ok()) | ||
| .filter_map(|id| MacroUserIdStr::try_from(id.clone()).ok()) | ||
| .collect(); | ||
| notified.extend(mention_recipients.iter().map(|id| id.as_ref().to_string())); |
Contributor
There was a problem hiding this comment.
Filter the sender out of mention_recipients.
The mention pass currently adds every normalized ID from mentioned_user_ids into the highest-priority bucket. If a commenter mentions themselves, create_comment_handler will still send a mention notification back to the sender, which breaks the sender-exclusion guarantee this helper is meant to enforce.
Suggested fix
let mention_recipients: HashSet<MacroUserIdStr<'static>> = mentioned_user_ids
.iter()
.filter_map(|id| MacroUserIdStr::try_from(id.clone()).ok())
+ .filter(|id| {
+ sender_id
+ .map(|sender| sender.as_ref() != id.as_ref())
+ .unwrap_or(true)
+ })
.collect();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/document_storage_service/src/api/annotations/mod.rs`
around lines 141 - 146, This code collects normalized mention_recipients from
mentioned_user_ids and then extends notified with them, but it doesn't exclude
the comment sender; update the logic that builds mention_recipients (the
conversion using MacroUserIdStr::try_from on mentioned_user_ids) to filter out
the sender's ID (compare against the sender's MacroUserIdStr or raw ID before
collecting) so the sender is not added to the mention_recipients HashSet and
thus not pushed into notified in create_comment_handler.
Contributor
Author
There was a problem hiding this comment.
This is already handled downstream in the notification ingress Service
Contributor
There was a problem hiding this comment.
@seanaye, understood — thanks for the clarification. If the notification ingress service already suppresses delivery back to the sender as a general guarantee, then the mention pass here doesn't need to replicate that logic.
I'll note this for future reviews so I don't flag the same pattern again.
✏️ Learnings added
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:21.214Z
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.
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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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/document_storage_service/src/api/annotations/edit_comment.rs (1)
94-97:⚠️ Potential issue | 🟡 MinorFix the
tracing::error!macro formatting to match repo standard.Change
error =? etoerror=?eon line 97. The structured error field should follow the consistent format acrosscloud-storage:tracing::error!(error=?e, "error msg")..inspect_err(|e| tracing::error!(error=?e, "couldn't send document mention notification"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/document_storage_service/src/api/annotations/edit_comment.rs` around lines 94 - 97, The tracing::error! invocation inside the send path for notification_ingress_service (in the closure passed to inspect_err on the future returned by send_notification) uses `error =? e` and should be formatted to the repo standard `error=?e`; update the closure passed to inspect_err(|e| ...) so the tracing::error! macro reads tracing::error!(error=?e, "couldn't send document mention notification") while leaving the surrounding call chain (notification_ingress_service.send_notification(request).await.inspect_err(...)) intact.
🤖 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/model_notifications/src/metadata.rs`:
- Around line 812-815: The collapse_key implementations for
RepliedToDocumentCommentThreadMetadata and CommentedOnDocumentMetadata currently
only use entity_type + entity_id, causing different comments on the same
document to collide; update each collapse_key(&self, entity: &Entity<'_>)
function to build the NotifCollapseKey starting from
NotifCollapseKey::new(entity_type) and then append entity.entity_id,
self.comment_id and self.thread_id (in that order), ensuring the collapse key
includes comment_id and thread_id so per-comment notifications are unique.
---
Outside diff comments:
In
`@rust/cloud-storage/document_storage_service/src/api/annotations/edit_comment.rs`:
- Around line 94-97: The tracing::error! invocation inside the send path for
notification_ingress_service (in the closure passed to inspect_err on the future
returned by send_notification) uses `error =? e` and should be formatted to the
repo standard `error=?e`; update the closure passed to inspect_err(|e| ...) so
the tracing::error! macro reads tracing::error!(error=?e, "couldn't send
document mention notification") while leaving the surrounding call chain
(notification_ingress_service.send_notification(request).await.inspect_err(...))
intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0591178e-a367-4522-aa3d-f2956651f5bb
📒 Files selected for processing (6)
rust/cloud-storage/comms_service/src/notification.rsrust/cloud-storage/document_storage_service/src/api/annotations/create_comment.rsrust/cloud-storage/document_storage_service/src/api/annotations/edit_comment.rsrust/cloud-storage/document_storage_service/src/api/annotations/mod.rsrust/cloud-storage/document_storage_service/src/api/annotations/test.rsrust/cloud-storage/model_notifications/src/metadata.rs
Comment on lines
+71
to
80
| let notif_ctx = CommentNotifContext { | ||
| text: req.text.clone().unwrap_or_default(), | ||
| comment_id: res.comment.comment_id, | ||
| thread_id: req.thread_id, | ||
| document_name: res.document_name.clone(), | ||
| document_id: res.document_id.to_string(), | ||
| owner: res.document_owner.clone(), | ||
| file_type: res.file_type.clone(), | ||
| sender_id: user_id.clone().try_into().ok(), | ||
| sender_profile_picture_url, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify whether the edited comment/response already exposes canonical text/thread fields.
rg -n -C3 'struct EditCommentRequest|struct EditCommentResponse|edit_document_comment|\bthread_id\b|\bthreadId\b|\btext\b' rust/cloud-storage --glob '*.rs'Repository: macro-inc/macro
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find and read the edit_comment.rs file to understand the full context
FILE_PATH="rust/cloud-storage/document_storage_service/src/api/annotations/edit_comment.rs"
if [ -f "$FILE_PATH" ]; then
echo "=== File: $FILE_PATH ==="
wc -l "$FILE_PATH"
cat -n "$FILE_PATH" | head -120
else
echo "File not found: $FILE_PATH"
fiRepository: macro-inc/macro
Length of output: 5287
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search specifically for EditCommentRequest and EditCommentResponse in document_storage_service
find rust/cloud-storage/document_storage_service -name "*.rs" -type f | head -20Repository: macro-inc/macro
Length of output: 1668
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for struct definitions of EditCommentRequest and EditCommentResponse
rg -A10 "struct EditCommentRequest|struct EditCommentResponse" rust/cloud-storage/document_storage_service --type rsRepository: macro-inc/macro
Length of output: 85
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for EditCommentRequest and EditCommentResponse struct definitions
rg -A15 "struct EditCommentRequest|struct EditCommentResponse" rust/cloud-storage --type rs -g "*/model/*"Repository: macro-inc/macro
Length of output: 85
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# More focused search - look in model crate
find . -path "*/model/src/annotations/edit*" -name "*.rs" 2>/dev/null | head -10Repository: macro-inc/macro
Length of output: 109
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Try broader search for annotations edit types
find . -path "*/annotations/*" -name "*.rs" 2>/dev/null | grep -i edit | head -20Repository: macro-inc/macro
Length of output: 409
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the EditCommentRequest and EditCommentResponse definitions
cat -n ./rust/cloud-storage/model/src/annotations/edit.rsRepository: macro-inc/macro
Length of output: 2393
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Also check the database layer to see what edit_document_comment returns
cat -n ./rust/cloud-storage/macro_db_client/src/annotations/edit_comment.rs | head -100Repository: macro-inc/macro
Length of output: 3773
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the Comment struct to confirm it has text and thread_id fields
rg -A30 "^pub struct Comment" rust/cloud-storage/model/src/annotations/ --type-not mdRepository: macro-inc/macro
Length of output: 4592
Use persisted comment fields for notification context instead of request payload.
The text and thread_id in the notification context should come from res.comment (the saved, canonical comment) rather than req (client input). The request's text is Option<String> and calling .unwrap_or_default() produces an empty string when the client omits it, even though the comment wasn't modified. Similarly, req.thread_id may differ from the actual persisted thread; use res.comment.thread_id instead for correctness.
Additionally, the error logging at line 97 should follow the guideline format: tracing::error!(error=?e, "couldn't send document mention notification") (no spaces around the =? operator).
synoet
approved these changes
Mar 25, 2026
Contributor
synoet
left a comment
synoet
left a comment
There was a problem hiding this comment.
Gave it a light pass, overall looks good.
This was referenced Apr 9, 2026
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
2 participants
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.
Summary
Adds two new document comment notification types with dedup guarantees, bringing document comments closer to parity with channel notifications:
replied_to_document_comment_thread— Notifies all thread participants (excluding sender) when someone replies to a comment thread they participated incommented_on_document— Notifies the document owner when someone comments on their documentNotifications follow a strict priority-based dedup: mention > thread reply > document owner. A user will never receive more than one notification per comment. The dedup logic is extracted into a pure
compute_notification_recipientsfunction with 10 unit tests covering overlap scenarios, case-insensitive ID matching, and sender exclusion.Bug fix
The previous inline dedup logic compared raw mention strings against normalized
MacroUserIdStrvalues (which are lowercased on parse). This meant a mention like"macro|Bob@Test.COM"would not match the lowercased thread participant"macro|bob@test.com", causing duplicate notifications. The extracted function normalizes all IDs throughMacroUserIdStr::parse_from_strbefore comparison.Changes
Backend (Rust)
model_notifications/src/metadata.rs— NewRepliedToDocumentCommentThreadMetadataandCommentedOnDocumentMetadatastructs withNotification,NotificationTitle, andNotificationExtIostrait implsmodel_notifications/src/lib.rs— NewNotifEventvariants andNotificationTitledelegationnotification_service/src/api/swagger.rs— Register new metadata types in OpenAPI schemadocument_storage_service/src/api/annotations/mod.rs—compute_notification_recipientspure function + builder helpersdocument_storage_service/src/api/annotations/create_comment.rs— Refactored dispatch logic usingcompute_notification_recipientsdocument_storage_service/src/api/annotations/test.rs— 10 new tests for recipient computationFrontend (TypeScript) — 6 files with exhaustive match updates
notification-metadata.ts— action, target name, content, show-targetnotification-preview.ts— label mapping (REPLY, COMMENT)notification-navigation.ts— document navigation handlersnotification-icon.tsx— reply arrow, comment chat iconnotification-description-helpers.ts— action verbs and type nounsnotification.ts— action text and message content extractionGenerated — OpenAPI spec + TypeScript client types
Test plan
cargo test -p document_storage_service— 11 tests pass (10 new + 1 existing)cargo test -p model_notifications— passescargo check— no errors or warningsbun check— TypeScript compiles cleancommented_on_documentnotificationreplied_to_document_comment_thread