fix: unread count for DMs when user has mentions-only preference#38836
fix: unread count for DMs when user has mentions-only preference#38836NagajyothiChukka wants to merge 4 commits intoRocketChat:developfrom
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
No actionable comments were generated in the recent review. 🎉 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
WalkthroughBroadened unread/notification update logic for direct-message rooms: direct messages ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts (1)
128-142:⚠️ Potential issue | 🟠 Major
shouldIncUnreadis inconsistent with the DB increment — real-time unread badge remains 0 for DMs.Line 118 now correctly increments the DB unread count for DMs when
!toAll && !toHere && !unreadAllMessages, but the real-time WebSocket notification path at line 130 still uses:const shouldIncUnread = hasUserMention || toAll || toHere || unreadAllMessages;For a DM with no mentions and
Unread_Count_DM = 'user_mentions_only', every term isfalse, soshouldIncUnread = false. ThenotifyOnSubscriptionChangedpayload is broadcast withoutunread: sub.unread + 1, so clients relying on the WebSocket stream (mobile, desktop) will still display unread count = 0 in real-time. The correct value only appears after a full page refresh that re-reads the DB.This contradicts the stated PR goal of fixing badges on mobile and desktop apps.
🐛 Proposed fix
- const shouldIncUnread = hasUserMention || toAll || toHere || unreadAllMessages; + const shouldIncUnread = hasUserMention || toAll || toHere || unreadAllMessages || room.t === 'd';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts` around lines 128 - 142, The real-time increment boolean shouldIncUnread used inside subs.forEach does not match the DB increment logic, causing DMs to show 0 until refresh; update shouldIncUnread (used before calling notifyOnSubscriptionChanged) to use the exact same condition/expression you used when updating the DB (the boolean that caused the DB unread increment), or extract that DB condition into a shared variable (e.g., shouldIncUnreadDb) and reuse it here so notifyOnSubscriptionChanged receives unread: sub.unread + 1 whenever the DB was incremented (ensure you still include hasUserMention, toAll, toHere, unreadAllMessages and the DM-specific case from the DB update logic when computing the boolean).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts
🧠 Learnings (4)
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts
📚 Learning: 2025-11-19T18:20:37.116Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.
Applied to files:
apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts
⏰ 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: cubic · AI code reviewer
🔇 Additional comments (1)
apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts (1)
118-120: DB-side fix is logically sound.The new
|| room.t === 'd'condition correctly firesincUnreadForRoomIdExcludingUserIdsfor all non-mentioned, non-sender subscribers in a DM whenUnread_Count_DMis notall_messages. No double-counting occurs because:
- Mentioned users are already excluded via
[...userIds, message.u._id]@all/@herecases are guarded by!toAll && !toHere- When
unreadAllMessagesis alsotrue, the||evaluates once, still calling the increment exactly once
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts`:
- Around line 128-142: The real-time increment boolean shouldIncUnread used
inside subs.forEach does not match the DB increment logic, causing DMs to show 0
until refresh; update shouldIncUnread (used before calling
notifyOnSubscriptionChanged) to use the exact same condition/expression you used
when updating the DB (the boolean that caused the DB unread increment), or
extract that DB condition into a shared variable (e.g., shouldIncUnreadDb) and
reuse it here so notifyOnSubscriptionChanged receives unread: sub.unread + 1
whenever the DB was incremented (ensure you still include hasUserMention, toAll,
toHere, unreadAllMessages and the DM-specific case from the DB update logic when
computing the boolean).
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
KevLehman
added
the
contrib: valid
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts (2)
104-109:⚠️ Potential issue | 🟡 Minor
onlyReadnot updated to match the new DM increment logicThe
subsquery'sonlyReadevaluates totruefor DMs whenUnread_Count_DM !== 'all_messages'(i.e., whenunreadAllMessages === false). This limits which subscriptions are included insubsfor real-timenotifyOnSubscriptionChangeddispatch. While the DB increment at line 119 fires correctly for all DMs, subscriptions in non-"read" state (e.g.,open=false— users who have never opened the DM) may be excluded from the real-time path and won't receive a live unread-count update.For consistency with the new DM behavior,
onlyReadshould also befalsefor DM rooms:🔧 Proposed fix
- onlyRead: !toAll && !toHere && !unreadAllMessages, + onlyRead: !toAll && !toHere && !unreadAllMessages && room.t !== 'd',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts` around lines 104 - 109, The subs query uses onlyRead: !toAll && !toHere && !unreadAllMessages which remains true for DMs when Unread_Count_DM !== 'all_messages' and therefore excludes subscriptions that should get real-time updates; update the call to Subscriptions.findByRoomIdAndNotAlertOrOpenExcludingUserIds (the subs query) to force onlyRead = false for direct-message rooms by including a room type check (e.g., room.t !== 'd') so the final expression becomes onlyRead: !toAll && !toHere && !unreadAllMessages && room.t !== 'd', ensuring DM subscriptions are included for real-time notifyOnSubscriptionChanged dispatch.
104-109:⚠️ Potential issue | 🟡 Minor
onlyReadnot updated to match the new DM increment pathFor a DM room where
Unread_Count_DM !== 'all_messages',unreadAllMessagesisfalse, soonlyReadevaluates totrue. This limits thesubsarray to subscriptions already in a "read" state. For the typical case (recipient has read all previous messages,alert=false), this is fine — the real-time notification still fires. However, subscriptions whereopen=false(e.g., a brand-new DM inbox that was never opened) satisfy "NOT open" but may be excluded byonlyRead=true, meaning their real-time update is silently dropped even though the DB unread count is correctly incremented at line 119.🔧 Proposed fix
- onlyRead: !toAll && !toHere && !unreadAllMessages, + onlyRead: !toAll && !toHere && !unreadAllMessages && room.t !== 'd',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts` around lines 104 - 109, The subscriptions query uses onlyRead derived from unreadAllMessages but doesn't account for the DM increment path; update the onlyRead argument passed to Subscriptions.findByRoomIdAndNotAlertOrOpenExcludingUserIds so that when handling a direct message room (room.t === 'd') where the DM unread increment path is used (i.e., Unread_Count_DM !== 'all_messages'), onlyRead is forced to false; in practice change the onlyRead expression (currently !toAll && !toHere && !unreadAllMessages) to also check room.t and the DM-unread condition and set onlyRead = false for that DM case so subscriptions with open=false are included.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts
🧠 Learnings (3)
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts
🔇 Additional comments (1)
apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts (1)
118-120: Verify DM unread increment bypass is intentionalThe
room.t === 'd'guards at lines 118 and 130 cause all DM messages to increment the unread counter regardless of theUnread_Count_DMsetting. WhenUnread_Count_DM='user_mentions_only', plain (non-mentioned) users will still receive unread increments.This pattern is consistent with the existing
getUserMentions/getGroupMentionshelpers (lines 43–53), which already return 1 for DMs regardless of the setting. However, those helpers apply only to mentioned users (lines 99–100), whereas lines 118 and 130 extend the bypass to all users in a DM via the plain message path. Confirm whether treating DM messages as always unread-eligible—overriding the admin-configured setting—is the intended product behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts`:
- Around line 118-120: The DM branch currently forces general unread increments
by calling Subscriptions.incUnreadForRoomIdExcludingUserIds when room.t === 'd',
which bypasses the Unread_Count_DM setting; update the condition (the if that
uses toAll, toHere, unreadAllMessages and room.t) to consult the Unread_Count_DM
value (e.g., unreadCount or however the setting is read in this module) and only
treat DMs as auto-incrementing when the setting is not 'mentions_only' (i.e.,
change the room.t === 'd' check to something like room.t === 'd' && unreadCount
!== 'mentions_only'), ensuring Subscriptions.incUnreadForRoomIdExcludingUserIds
is only called when the configured policy allows it.
---
Outside diff comments:
In `@apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts`:
- Around line 104-109: The subs query uses onlyRead: !toAll && !toHere &&
!unreadAllMessages which remains true for DMs when Unread_Count_DM !==
'all_messages' and therefore excludes subscriptions that should get real-time
updates; update the call to
Subscriptions.findByRoomIdAndNotAlertOrOpenExcludingUserIds (the subs query) to
force onlyRead = false for direct-message rooms by including a room type check
(e.g., room.t !== 'd') so the final expression becomes onlyRead: !toAll &&
!toHere && !unreadAllMessages && room.t !== 'd', ensuring DM subscriptions are
included for real-time notifyOnSubscriptionChanged dispatch.
- Around line 104-109: The subscriptions query uses onlyRead derived from
unreadAllMessages but doesn't account for the DM increment path; update the
onlyRead argument passed to
Subscriptions.findByRoomIdAndNotAlertOrOpenExcludingUserIds so that when
handling a direct message room (room.t === 'd') where the DM unread increment
path is used (i.e., Unread_Count_DM !== 'all_messages'), onlyRead is forced to
false; in practice change the onlyRead expression (currently !toAll && !toHere
&& !unreadAllMessages) to also check room.t and the DM-unread condition and set
onlyRead = false for that DM case so subscriptions with open=false are included.
| if (!toAll && !toHere && (unreadAllMessages || room.t === 'd')) { | ||
| await Subscriptions.incUnreadForRoomIdExcludingUserIds(room._id, [...userIds, message.u._id], 1); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "Unread_Count_DM" --type ts -C 3Repository: RocketChat/Rocket.Chat
Length of output: 1034
🏁 Script executed:
cat -n apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts | head -150Repository: RocketChat/Rocket.Chat
Length of output: 6772
🏁 Script executed:
# Search for Unread_Count_DM in all file types to understand its definition
rg "Unread_Count_DM" -C 5Repository: RocketChat/Rocket.Chat
Length of output: 50378
🏁 Script executed:
# Get the full setting definition for Unread_Count_DM
sed -n '146,160p' apps/meteor/server/settings/general.tsRepository: RocketChat/Rocket.Chat
Length of output: 380
🏁 Script executed:
# Also check what values are allowed for the setting
rg -A 20 "this.add\('Unread_Count_DM'" apps/meteor/server/settings/general.tsRepository: RocketChat/Rocket.Chat
Length of output: 483
🏁 Script executed:
# Look for any comments or recent changes around the lines in question
sed -n '85,135p' apps/meteor/app/lib/server/lib/notifyUsersOnMessage.tsRepository: RocketChat/Rocket.Chat
Length of output: 2186
🏁 Script executed:
# Get the exact function context around lines 50-58 to see the precedent
sed -n '48,59p' apps/meteor/app/lib/server/lib/notifyUsersOnMessage.tsRepository: RocketChat/Rocket.Chat
Length of output: 820
🏁 Script executed:
# Verify the values parameter for Unread_Count_DM is complete
rg -A 15 "'mentions_only'" apps/meteor/server/settings/general.ts | head -20Repository: RocketChat/Rocket.Chat
Length of output: 422
Unread_Count_DM setting is effectively bypassed for general unread increments on DM rooms
The room.t === 'd' guard at lines 118-120 and 130 unconditionally increments unread counts for all DM messages, overriding the Unread_Count_DM setting when configured to mentions_only. This creates an inconsistency: while user and group mention increments correctly treat DMs as a special case (lines 50-58 always return 1 for DMs), the general unread increment ignores the setting entirely for DM rooms.
This may be intentional per historical DM notification behavior, but since Unread_Count_DM remains configurable with two distinct options, confirm with the product team whether the setting should be fully ignored for the unread increment path in DMs, or whether a narrower condition (e.g., only bypass when unreadCount !== 'mentions_only') is intended.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts` around lines 118 -
120, The DM branch currently forces general unread increments by calling
Subscriptions.incUnreadForRoomIdExcludingUserIds when room.t === 'd', which
bypasses the Unread_Count_DM setting; update the condition (the if that uses
toAll, toHere, unreadAllMessages and room.t) to consult the Unread_Count_DM
value (e.g., unreadCount or however the setting is read in this module) and only
treat DMs as auto-incrementing when the setting is not 'mentions_only' (i.e.,
change the room.t === 'd' check to something like room.t === 'd' && unreadCount
!== 'mentions_only'), ensuring Subscriptions.incUnreadForRoomIdExcludingUserIds
is only called when the configured policy allows it.
2 participants
Summary
This PR fixes Issue #23977 where users with the "Mentions Only" notification preference would not see unread badges for Direct Messages.
Details
Modified
notifyUsersOnMessage.tsto explicitly check for the Direct Message room type (room.t === 'd'). This ensures that even if a user's global preference is set to 'Mentions Only', DMs will still trigger an unread count increment in the database.Impact
/api/v1/subscriptions.get).Related Issue
Closes #23977
Summary by CodeRabbit