feat: Reaction notifications #40303
Open
DivyanshuVortex wants to merge 6 commits intoRocketChat:developfrom
Open
feat: Reaction notifications #40303DivyanshuVortex wants to merge 6 commits intoRocketChat:developfrom
DivyanshuVortex wants to merge 6 commits intoRocketChat:developfrom
Conversation
Contributor
|
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 |
|
|
|
Contributor
WalkthroughAdds server-side reaction notification handlers, client preference and UI for reaction notifications, subscription reaction counters with publish support, model methods to increment/decrement reaction counts, i18n keys, and a sidebar reaction badge; notifications are sent via existing desktop notification flow with an optional icon. Changes
Sequence Diagram(s)sequenceDiagram
actor Reactor as Reactor (User A)
participant Client as Client
participant ReactionSrv as Reactions Server
participant DB as Database
participant Subscriptions as Subscriptions Model
participant Notif as Notification Service
actor Recipient as Recipient (User B)
Reactor->>Client: add reaction to message
Client->>ReactionSrv: emit setReaction
ReactionSrv->>DB: load message + recipient id
ReactionSrv->>DB: load recipient user (active, language)
ReactionSrv->>DB: load recipient preferences (receiveReactionNotifications)
alt recipient active and preference enabled and not self-reaction
ReactionSrv->>Subscriptions: incReactionsForRoomIdAndUserIds(roomId, [recipientId])
Subscriptions-->>ReactionSrv: update result
ReactionSrv->>ReactionSrv: build localized Reaction_Notification (emoji, sender name, message text)
ReactionSrv->>Notif: notifyDesktopUser(recipientId, notification {icon?})
Notif-->>Recipient: desktop notification
ReactionSrv->>DB: notifyOnSubscriptionChangedByRoomIdAndUserId(roomId, recipientId)
else skip notification
ReactionSrv-->>ReactionSrv: no-op
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
coderabbitai
Bot
added
the
type: feature
![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: 3
🧹 Nitpick comments (4)
packages/i18n/src/locales/en.i18n.json (1)
4364-4364: Use sentence case for preference-label consistency.At Line 4364,
"Receive Reaction Notifications"is Title Case, while adjacent notification preference labels are sentence case. Consider aligning it for UI consistency.Proposed copy tweak
- "Receive_Reaction_Notifications": "Receive Reaction Notifications", + "Receive_Reaction_Notifications": "Receive reaction notifications",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/i18n/src/locales/en.i18n.json` at line 4364, The label key "Receive_Reaction_Notifications" uses Title Case while nearby notification preference labels use sentence case; update its value to sentence case (e.g., "Receive reaction notifications") to match the existing UI preference-label style and ensure consistency across notification keys like "Receive_Reaction_Notifications".apps/meteor/client/views/account/preferences/useAccountPreferencesValues.ts (1)
59-59: Consider passing an explicit default to match the server default.
receiveLoginDetectionEmailusesuseUserPreference<boolean>('receiveLoginDetectionEmail', true). For consistency and so the toggle reflects the server default (trueinaccounts.ts) when the user has never saved this preference, consider doing the same here.♻️ Proposed change
- const receiveReactionNotifications = useUserPreference<boolean>('receiveReactionNotifications'); + const receiveReactionNotifications = useUserPreference<boolean>('receiveReactionNotifications', true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/account/preferences/useAccountPreferencesValues.ts` at line 59, The preference hook call for receiveReactionNotifications lacks an explicit default so the UI may not match the server default; update the call to useUserPreference<boolean>('receiveReactionNotifications', true) so the toggle defaults to true when unset (same pattern used by receiveLoginDetectionEmail) — change the invocation of useUserPreference for receiveReactionNotifications to include the explicit true default.apps/meteor/app/reactions/server/notifications.ts (2)
10-44: Optional: confirm intended room scope.Linked issue
#40293explicitly scopes the initial rollout to DMs ("Initially apply the behavior to DMs (with possible future extension to channels/discussions or making it configurable)"), but this handler fires for every room type. Increments tounreadin large channels on every reaction can be noisy and surprising to users. Consider either (a) restricting toroom.t === 'd'for the first version, or (b) gating the behavior behind a room-type-aware setting so channel operators can opt in.♻️ Example DM-only gate
if (!message.u?._id || message.u._id === user._id) { return; } + + if (room.t !== 'd') { + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/reactions/server/notifications.ts` around lines 10 - 44, The afterSetReaction callback currently applies unread increments for every room type; restrict this behavior to DMs or make it room-type-aware by checking room.t before calling Subscriptions.incUserMentionsAndUnreadForRoomIdAndUserIds and notifyOnSubscriptionChangedByRoomIdAndUserId. Specifically, inside the callbacks.add('afterSetReaction', ...) handler, add a guard that only proceeds when room.t === 'd' (or consult a new room-type-aware setting like Reactions_Notify_In_DM_Only) before the recipient lookups and the call to Subscriptions.incUserMentionsAndUnreadForRoomIdAndUserIds and notifyOnSubscriptionChangedByRoomIdAndUserId so large channels no longer receive reaction-triggered unread increments unless explicitly allowed.
49-54: Avoidas anyon the emoji APIs.The double
as anycast loses type safety. Type definitions foremoji.list(withuc_output: string) already exist, and the conversion pattern is already implemented inapps/meteor/app/emoji-emojione/lib/getEmojiConfig.ts(line 205), which properly types the emojione module and list entries. Extract this conversion into a typed helper inapp/emojior reuse the existing pattern to maintain type safety.♻️ Sketch of a typed helper
type EmojioneLike = { convert: (code: string) => string }; type EmojiListEntry = { uc_output?: string }; const convertToEmojione = (reaction: string): string => { const emojione = (emoji.packages as Record<string, EmojioneLike | undefined>).emojione; const entry = emoji.list[reaction] as EmojiListEntry | undefined; if (emojione && entry?.uc_output) { return emojione.convert(entry.uc_output.toUpperCase()); } return reaction; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/reactions/server/notifications.ts` around lines 49 - 54, Replace the unsafe "as any" casts around emoji.packages/emojione and emoji.list entries by reusing the typed conversion pattern from apps/meteor/app/emoji-emojione/lib/getEmojiConfig.ts (or move it into a shared helper in app/emoji); create a typed helper (e.g., convertToEmojione) that narrows emoji.packages to a Record<string, { convert: (s: string) => string } | undefined> and emoji.list entries to { uc_output?: string } and then use that helper in place of the current emojione/emojione.convert logic so emojiActual is computed with proper types (keep references to emojione, emoji.packages, emoji.list and reaction to locate the call site).
🤖 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/reactions/server/notifications.ts`:
- Around line 79-107: The afterUnsetReaction callback currently decrements
subscription.unread unconditionally (except self-reaction), which can corrupt
the shared unread counter; update afterUnsetReaction to mirror the exact gating
used in afterSetReaction: check Troubleshoot_Disable_Notifications is off,
ensure the reactor is not the author, verify the recipient is active, and
confirm the recipient's receiveReactionNotifications preference before
performing the Subscriptions.updateOne decrement and calling
notifyOnSubscriptionChangedByRoomIdAndUserId; for a more robust fix, consider
tracking reaction-driven unreads separately (e.g., a dedicated reactionUnread
counter or recomputing reaction-derived unread state) instead of applying delta
increments/decrements to subscription.unread.
- Around line 71-73: Replace the console.error calls in the catch blocks in
notifications.ts with SystemLogger.error to use the project logging pipeline;
specifically, update the catch in the function that sends reaction notifications
(currently logging "Error sending reaction notification") and the other catch in
this file to call SystemLogger.error with a descriptive message and the caught
error object so the error details and stack are preserved.
In `@packages/i18n/src/locales/en.i18n.json`:
- Line 4343: The Reaction_Notification i18n string currently lacks the {{name}}
placeholder so the reactor's name passed from
apps/meteor/app/reactions/server/notifications.ts (name: reactorName) is never
shown; update the "Reaction_Notification" value to include {{name}}, e.g. change
it to something like "{{name}} reacted with {{reaction}} to your message:
{{message}}" so the template consumes the reactorName passed by the
notifications code.
---
Nitpick comments:
In `@apps/meteor/app/reactions/server/notifications.ts`:
- Around line 10-44: The afterSetReaction callback currently applies unread
increments for every room type; restrict this behavior to DMs or make it
room-type-aware by checking room.t before calling
Subscriptions.incUserMentionsAndUnreadForRoomIdAndUserIds and
notifyOnSubscriptionChangedByRoomIdAndUserId. Specifically, inside the
callbacks.add('afterSetReaction', ...) handler, add a guard that only proceeds
when room.t === 'd' (or consult a new room-type-aware setting like
Reactions_Notify_In_DM_Only) before the recipient lookups and the call to
Subscriptions.incUserMentionsAndUnreadForRoomIdAndUserIds and
notifyOnSubscriptionChangedByRoomIdAndUserId so large channels no longer receive
reaction-triggered unread increments unless explicitly allowed.
- Around line 49-54: Replace the unsafe "as any" casts around
emoji.packages/emojione and emoji.list entries by reusing the typed conversion
pattern from apps/meteor/app/emoji-emojione/lib/getEmojiConfig.ts (or move it
into a shared helper in app/emoji); create a typed helper (e.g.,
convertToEmojione) that narrows emoji.packages to a Record<string, { convert:
(s: string) => string } | undefined> and emoji.list entries to { uc_output?:
string } and then use that helper in place of the current
emojione/emojione.convert logic so emojiActual is computed with proper types
(keep references to emojione, emoji.packages, emoji.list and reaction to locate
the call site).
In `@apps/meteor/client/views/account/preferences/useAccountPreferencesValues.ts`:
- Line 59: The preference hook call for receiveReactionNotifications lacks an
explicit default so the UI may not match the server default; update the call to
useUserPreference<boolean>('receiveReactionNotifications', true) so the toggle
defaults to true when unset (same pattern used by receiveLoginDetectionEmail) —
change the invocation of useUserPreference for receiveReactionNotifications to
include the explicit true default.
In `@packages/i18n/src/locales/en.i18n.json`:
- Line 4364: The label key "Receive_Reaction_Notifications" uses Title Case
while nearby notification preference labels use sentence case; update its value
to sentence case (e.g., "Receive reaction notifications") to match the existing
UI preference-label style and ensure consistency across notification keys like
"Receive_Reaction_Notifications".
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2468993f-d271-4a73-b7cd-63967f800f93
📒 Files selected for processing (6)
apps/meteor/app/reactions/server/index.tsapps/meteor/app/reactions/server/notifications.tsapps/meteor/client/views/account/preferences/PreferencesNotificationsSection.tsxapps/meteor/client/views/account/preferences/useAccountPreferencesValues.tsapps/meteor/server/settings/accounts.tspackages/i18n/src/locales/en.i18n.json
📜 Review details
🧰 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/reactions/server/index.tsapps/meteor/client/views/account/preferences/PreferencesNotificationsSection.tsxapps/meteor/server/settings/accounts.tsapps/meteor/app/reactions/server/notifications.tsapps/meteor/client/views/account/preferences/useAccountPreferencesValues.ts
🧠 Learnings (13)
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
apps/meteor/app/reactions/server/index.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/reactions/server/index.tsapps/meteor/server/settings/accounts.tsapps/meteor/app/reactions/server/notifications.tsapps/meteor/client/views/account/preferences/useAccountPreferencesValues.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/reactions/server/index.tsapps/meteor/server/settings/accounts.tsapps/meteor/app/reactions/server/notifications.tsapps/meteor/client/views/account/preferences/useAccountPreferencesValues.ts
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.
Applied to files:
apps/meteor/client/views/account/preferences/PreferencesNotificationsSection.tsx
📚 Learning: 2026-03-15T14:31:28.969Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:28.969Z
Learning: In RocketChat/Rocket.Chat, the `UserCreateParamsPOST` type in `apps/meteor/app/api/server/v1/users.ts` (migrated from `packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts`) intentionally has `fields: string` (non-optional) and `settings?: IUserSettings` without a corresponding AJV schema entry. This is a pre-existing divergence carried over verbatim from the original rest-typings source (PR `#39647`). Do not flag this type/schema misalignment during the OpenAPI migration review — it is tracked as a separate follow-up fix.
Applied to files:
apps/meteor/server/settings/accounts.tsapps/meteor/client/views/account/preferences/useAccountPreferencesValues.ts
📚 Learning: 2025-11-05T20:53:57.761Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: apps/meteor/ee/server/startup/federation.ts:39-74
Timestamp: 2025-11-05T20:53:57.761Z
Learning: In Rocket.Chat (apps/meteor/app/settings/server/CachedSettings.ts), the settings.watchMultiple() method immediately invokes its callback with current values if all requested settings exist in the store, then continues watching for subsequent changes. It does not wait for a setting to change before the first invocation.
Applied to files:
apps/meteor/server/settings/accounts.ts
📚 Learning: 2026-03-11T22:04:20.529Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39545
File: apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts:59-61
Timestamp: 2026-03-11T22:04:20.529Z
Learning: In `apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts`, the `msg.u._id === uid` early-return in the `streamNewMessage` handler is intentional: the "New messages" indicator is designed to notify about messages from other users only. Self-sent messages — including those sent from a different session/device — are always skipped, by design. Do not flag this as a multi-session regression.
Applied to files:
apps/meteor/app/reactions/server/notifications.ts
📚 Learning: 2026-04-18T12:32:53.425Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 38623
File: apps/meteor/app/lib/server/functions/cleanRoomHistory.ts:146-149
Timestamp: 2026-04-18T12:32:53.425Z
Learning: In `apps/meteor/app/lib/server/functions/cleanRoomHistory.ts` (PR `#38623`), the read-receipt cleanup (both `ReadReceipts.removeByMessageIds` and `ReadReceiptsArchive.removeByMessageIds`) is intentionally only performed in the limited prune path (`limit && selectedMessageIds`). The unlimited/delete-all path (`limit === 0`) deliberately skips cleaning up orphaned read receipts in both hot and cold storage — this is by design. Do not flag this as a bug or missing cleanup in future reviews.
Applied to files:
apps/meteor/app/reactions/server/notifications.ts
📚 Learning: 2026-04-14T21:10:36.233Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 36292
File: apps/meteor/client/hooks/useHasValidLocationHash.ts:7-12
Timestamp: 2026-04-14T21:10:36.233Z
Learning: In the RocketChat/Rocket.Chat repository, adding JSDoc-style comments to React hooks (e.g., files under apps/meteor/client/hooks/) is considered a good pattern and should not be flagged as a violation of the "avoid code comments in the implementation" guideline. The guideline against code comments does not apply to JSDoc documentation on exported hooks.
Applied to files:
apps/meteor/app/reactions/server/notifications.ts
📚 Learning: 2025-12-09T20:01:07.355Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37532
File: ee/packages/federation-matrix/src/FederationMatrix.ts:920-927
Timestamp: 2025-12-09T20:01:07.355Z
Learning: In Rocket.Chat's federation invite handling (ee/packages/federation-matrix/src/FederationMatrix.ts), when a user rejects an invite via federationSDK.rejectInvite(), the subscription cleanup happens automatically through an event-driven flow: Matrix emits a leave event back, which is processed by handleLeave() in ee/packages/federation-matrix/src/events/member.ts, and that function calls Room.performUserRemoval() to clean up the subscription. No explicit cleanup is needed in the reject branch of handleInvite() because the leave event handler takes care of it.
<!-- </add_learning>
Applied to files:
apps/meteor/app/reactions/server/notifications.ts
📚 Learning: 2026-02-10T16:32:42.586Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:42.586Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON and Date fields arrive as Date objects; do not manually construct new Date() in stream handlers (for example, in sdk.stream()). Only REST API responses return plain JSON where dates are strings, so implement explicit conversion there if needed. Apply this guidance to all TypeScript files under apps/meteor/client to ensure consistent date handling in DDP streams and REST responses.
Applied to files:
apps/meteor/client/views/account/preferences/useAccountPreferencesValues.ts
📚 Learning: 2025-11-19T12:32:29.696Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
Applied to files:
packages/i18n/src/locales/en.i18n.json
📚 Learning: 2025-11-17T22:38:48.631Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37505
File: packages/i18n/src/locales/en.i18n.json:3765-3765
Timestamp: 2025-11-17T22:38:48.631Z
Learning: Rocket.Chat i18n copy: Keep sentence case for the value of "Notification_Desktop_show_voice_calls" in packages/i18n/src/locales/en.i18n.json (“Show desktop notifications for voice calls”) per design directive; do not change to Title Case even if nearby labels differ.
Applied to files:
packages/i18n/src/locales/en.i18n.json
🔇 Additional comments (3)
apps/meteor/app/reactions/server/index.ts (1)
2-2: LGTM.Import correctly wires the new server-side notification handlers into the reactions module entrypoint.
apps/meteor/server/settings/accounts.ts (1)
774-778: LGTM.The default user preference is defined consistently with adjacent entries (public boolean with i18nLabel). The
Accounts_Default_User_Preferences_prefix is correctly used so the server resolvesreceiveReactionNotificationsagainst this default.apps/meteor/client/views/account/preferences/PreferencesNotificationsSection.tsx (1)
207-216: LGTM.The new toggle follows the same
Controller+ToggleSwitchpattern used by the other boolean preferences in this section and is wired to thereceiveReactionNotificationsform field exposed byuseAccountPreferencesValues.
|
Hey, that was a fast PR, thanks for working on this! One concern I have is around how reaction notifications are represented in the UI. Right now, reactions and text messages contribute to the same unread indicator. I think it’s important to differentiate between them visually. I had proposed something like this: The reason is that reactions are often used more casually, while actual messages/replies usually carry more weight and urgency. If both are treated identically in unread counts, it can make it harder to prioritize what needs attention. A separate or lighter indicator for reactions could help preserve that distinction while still solving the “missed reaction” problem. |
coderabbitai
Bot
removed
the
type: feature
Author
|
Hey @amir2mi, I understand your point and I also read that in your issue. The reason I kept the notification behavior the same is that I was following the same approach Discord uses. Also, the notification already says: “{{name}} reacted with {{reaction}} to your message: {{message}}”, so it already provides the necessary context and does not feel misleading. |
That’s true for users who rely on notifications. However, in my case (and likely others), notifications are disabled to reduce noise, and the unread counter becomes the primary signal to decide which conversations need attention. In that scenario, reactions and messages are treated equally in the counter, even though they don’t carry the same weight. This makes it harder to prioritize, since a lightweight reaction can surface a conversation the same way a real reply would. |
Author
|
Yep, you’re correct. Let me add a small emoji reaction icon specifically for reaction notifications only. |
coderabbitai
Bot
added
the
type: feature
Author
Screencast.from.2026-04-25.10-32-52.webm
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/meteor/app/reactions/server/notifications.ts (1)
9-9:⚠️ Potential issue | 🔴 CriticalRemove absolute import path for
SystemLogger.The import uses an absolute path starting with
/, which will not resolve at runtime. All other imports in this file and throughout the codebase use relative paths. Change to../../../server/lib/logger/systemto match the pattern.Proposed fix
-import { SystemLogger } from '/server/lib/logger/system'; +import { SystemLogger } from '../../../server/lib/logger/system';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/reactions/server/notifications.ts` at line 9, In notifications.ts replace the absolute import "import { SystemLogger } from '/server/lib/logger/system';" with a relative import so the module resolves at runtime; update the import to use the project’s relative pattern (for example: import { SystemLogger } from '../../../server/lib/logger/system';) and ensure any linters/tsconfig path rules are satisfied.
🧹 Nitpick comments (6)
apps/meteor/client/sidebar/badges/ReactionBadge.tsx (3)
22-22: Prefer Fuselage spacing tokens over inline style.A small
marginLeft: '2px'magic value sidesteps the design system. Wrapping the count in a FuselageBox mis={2}(or similar) keeps spacing consistent with other badges and avoids inline styles. Optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/sidebar/badges/ReactionBadge.tsx` at line 22, Replace the inline magic spacing on the span that renders the count in ReactionBadge: instead of <span aria-hidden style={{ marginLeft: '2px' }}>{total}</span> use the Fuselage Box spacing token (e.g. Box with mis={2} or equivalent) so spacing follows the design system; import Box from '@rocket.chat/fuselage' if not present and render the count with Box (as a span or using is/as props) with aria-hidden and mis={2} to remove the inline style and keep semantics.
14-24: Consider hiding the badge from accessibility tree if theUnreadBadgealready conveys count.When a room has both messages and reactions unread, screen readers will announce two
role="status"regions with the sameunreadTitle— likely double-announcing the combined counter. Two options:
- Pass a reaction-specific title (e.g., just
t('reactions_counter', { count: total })) instead of the combinedunreadTitle.- Or set
aria-hiddenon this badge and rely onUnreadBadge's status region.Not a blocker, but worth a pass before release given the issue's emphasis on the visibility/UX of reactions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/sidebar/badges/ReactionBadge.tsx` around lines 14 - 24, The ReactionBadge currently renders a SidebarV2ItemBadge with role="status" and the combined unread title which can double-announce with UnreadBadge; update ReactionBadge to avoid duplicate announcements by either (A) replacing the aria-label/title passed into SidebarV2ItemBadge with a reaction-specific label (e.g., use the i18n key like t('reactions_counter', { count: total }) instead of t('__unreadTitle__from__roomTitle__')) or (B) mark the badge as non-accessible by adding aria-hidden={true} to the SidebarV2ItemBadge so UnreadBadge remains the sole status region; modify the ReactionBadge component accordingly (change the title/aria-label prop or add aria-hidden) and ensure Icon/span remain visually unchanged.
2-2: Drop the explicitReactimport.This monorepo uses the automatic JSX runtime. Other sidebar badge files such as
UnreadBadge.tsxdon't import React directly, and the import is unused in this component—JSX works without it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/sidebar/badges/ReactionBadge.tsx` at line 2, Remove the unused explicit React import from the ReactionBadge component: delete the line "import React from 'react';" in ReactionBadge.tsx so the file relies on the automatic JSX runtime (matching other badge files like UnreadBadge.tsx); ensure no other references to the React identifier remain in the file and run a quick typecheck/compile to verify JSX still works.apps/meteor/app/reactions/server/notifications.ts (3)
92-101: Use the same dotted projection style as the set handler.The
afterSetReactionblock at lines 23–30 uses the dotted form ('settings.preferences.receiveReactionNotifications': 1); this nested-object form works in MongoDB but is inconsistent within the same file and slightly less safe (MongoDB rejects mixing inclusion/exclusion at sibling keys). Aligning the two improves readability.♻️ Proposed fix
const recipient = await Users.findOneById(message.u._id, { projection: { - active: 1, - settings: { - preferences: { - receiveReactionNotifications: 1, - }, - }, + 'active': 1, + 'settings.preferences.receiveReactionNotifications': 1, }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/reactions/server/notifications.ts` around lines 92 - 101, The projection passed to Users.findOneById when fetching recipient uses a nested object for settings.preferences instead of the dotted-key style used elsewhere; update the projection in the Users.findOneById call (the recipient lookup) to use dotted keys (e.g., 'settings.preferences.receiveReactionNotifications': 1) and keep only numeric inclusion fields (active: 1 and the dotted settings key) so it matches the dotted-style projection used in afterSetReaction and avoids mixing sibling inclusion/exclusion.
23-30: Honorrecipient.statusbefore sending desktop notification (or drop it from the projection).
statusis fetched but never checked.notifyDesktopUserwill be a no-op for offline/away users in some flows, but the typical pattern in this codebase is to skip queue/work when the recipient isn't online to receive a desktop. If you intend to honor presence, add the check; otherwise removestatusfrom the projection to avoid misleading future readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/reactions/server/notifications.ts` around lines 23 - 30, The code fetches recipient.status but never uses it; either honor presence by checking recipient.status before queuing/sending desktop notifications or remove status from the projection to avoid confusion. Locate the Users.findOneById call and the subsequent code path that calls notifyDesktopUser (and any queueing/worker enqueuing for desktop notifications) and: if you intend to respect presence, add a guard that skips notifyDesktopUser/queueing unless recipient.status indicates the user is online (e.g., recipient.status === 'online'); otherwise, remove 'status' from the projection in Users.findOneById so the field isn't fetched needlessly.
73-73: Fix SystemLogger.error calls to use object format for proper error serialization.
SystemLogger.error()expects an object as the first argument (not two positional arguments). Passing a message and error as separate arguments causes the error to be silently dropped during serialization. Pass an object withmsganderrproperties instead.♻️ Proposed fix
- } catch (e) { - SystemLogger.error('Error sending reaction notification', e); - } + } catch (e) { + SystemLogger.error({ msg: 'Error sending reaction notification', err: e }); + }- } catch (e) { - SystemLogger.error('Error handling reaction notification removal', e); - } + } catch (e) { + SystemLogger.error({ msg: 'Error handling reaction notification removal', err: e }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/reactions/server/notifications.ts` at line 73, SystemLogger.error is being called with positional args which drops the error; update the call in reactions/server/notifications.ts (the SystemLogger.error invocation that currently passes 'Error sending reaction notification' and e) to pass a single object like { msg: 'Error sending reaction notification', err: e } so the error serializes correctly; apply the same object-shaped argument pattern for any other SystemLogger.error occurrences in this file/function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/meteor/app/reactions/server/notifications.ts`:
- Line 9: In notifications.ts replace the absolute import "import { SystemLogger
} from '/server/lib/logger/system';" with a relative import so the module
resolves at runtime; update the import to use the project’s relative pattern
(for example: import { SystemLogger } from '../../../server/lib/logger/system';)
and ensure any linters/tsconfig path rules are satisfied.
---
Nitpick comments:
In `@apps/meteor/app/reactions/server/notifications.ts`:
- Around line 92-101: The projection passed to Users.findOneById when fetching
recipient uses a nested object for settings.preferences instead of the
dotted-key style used elsewhere; update the projection in the Users.findOneById
call (the recipient lookup) to use dotted keys (e.g.,
'settings.preferences.receiveReactionNotifications': 1) and keep only numeric
inclusion fields (active: 1 and the dotted settings key) so it matches the
dotted-style projection used in afterSetReaction and avoids mixing sibling
inclusion/exclusion.
- Around line 23-30: The code fetches recipient.status but never uses it; either
honor presence by checking recipient.status before queuing/sending desktop
notifications or remove status from the projection to avoid confusion. Locate
the Users.findOneById call and the subsequent code path that calls
notifyDesktopUser (and any queueing/worker enqueuing for desktop notifications)
and: if you intend to respect presence, add a guard that skips
notifyDesktopUser/queueing unless recipient.status indicates the user is online
(e.g., recipient.status === 'online'); otherwise, remove 'status' from the
projection in Users.findOneById so the field isn't fetched needlessly.
- Line 73: SystemLogger.error is being called with positional args which drops
the error; update the call in reactions/server/notifications.ts (the
SystemLogger.error invocation that currently passes 'Error sending reaction
notification' and e) to pass a single object like { msg: 'Error sending reaction
notification', err: e } so the error serializes correctly; apply the same
object-shaped argument pattern for any other SystemLogger.error occurrences in
this file/function.
In `@apps/meteor/client/sidebar/badges/ReactionBadge.tsx`:
- Line 22: Replace the inline magic spacing on the span that renders the count
in ReactionBadge: instead of <span aria-hidden style={{ marginLeft: '2px'
}}>{total}</span> use the Fuselage Box spacing token (e.g. Box with mis={2} or
equivalent) so spacing follows the design system; import Box from
'@rocket.chat/fuselage' if not present and render the count with Box (as a span
or using is/as props) with aria-hidden and mis={2} to remove the inline style
and keep semantics.
- Around line 14-24: The ReactionBadge currently renders a SidebarV2ItemBadge
with role="status" and the combined unread title which can double-announce with
UnreadBadge; update ReactionBadge to avoid duplicate announcements by either (A)
replacing the aria-label/title passed into SidebarV2ItemBadge with a
reaction-specific label (e.g., use the i18n key like t('reactions_counter', {
count: total }) instead of t('__unreadTitle__from__roomTitle__')) or (B) mark
the badge as non-accessible by adding aria-hidden={true} to the
SidebarV2ItemBadge so UnreadBadge remains the sole status region; modify the
ReactionBadge component accordingly (change the title/aria-label prop or add
aria-hidden) and ensure Icon/span remain visually unchanged.
- Line 2: Remove the unused explicit React import from the ReactionBadge
component: delete the line "import React from 'react';" in ReactionBadge.tsx so
the file relies on the automatic JSX runtime (matching other badge files like
UnreadBadge.tsx); ensure no other references to the React identifier remain in
the file and run a quick typecheck/compile to verify JSX still works.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d515c549-c02b-4bf9-9798-e6837f962621
📒 Files selected for processing (10)
apps/meteor/app/lib/server/functions/notifications/desktop.tsapps/meteor/app/reactions/server/notifications.tsapps/meteor/client/sidebar/badges/ReactionBadge.tsxapps/meteor/client/sidebar/badges/SidebarItemBadges.tsxapps/meteor/lib/getSubscriptionUnreadData.tsapps/meteor/lib/publishFields.tspackages/core-typings/src/ISubscription.tspackages/i18n/src/locales/en.i18n.jsonpackages/model-typings/src/models/ISubscriptionsModel.tspackages/models/src/models/Subscriptions.ts
✅ Files skipped from review due to trivial changes (3)
- apps/meteor/lib/publishFields.ts
- packages/core-typings/src/ISubscription.ts
- apps/meteor/app/lib/server/functions/notifications/desktop.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/i18n/src/locales/en.i18n.json
📜 Review details
🧰 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:
packages/model-typings/src/models/ISubscriptionsModel.tsapps/meteor/client/sidebar/badges/SidebarItemBadges.tsxapps/meteor/client/sidebar/badges/ReactionBadge.tsxapps/meteor/lib/getSubscriptionUnreadData.tspackages/models/src/models/Subscriptions.tsapps/meteor/app/reactions/server/notifications.ts
🧠 Learnings (14)
📚 Learning: 2026-03-09T18:39:21.178Z
Learnt from: Harxhit
Repo: RocketChat/Rocket.Chat PR: 39476
File: apps/meteor/server/methods/addAllUserToRoom.ts:0-0
Timestamp: 2026-03-09T18:39:21.178Z
Learning: In apps/meteor/server/methods/addAllUserToRoom.ts, the implementation uses a single cursor pass (Users.find(userFilter).batchSize(100)) that collects both the full user objects (collectedUsers: IUser[]) and their usernames (usernames: string[]) in one iteration. `beforeAddUserToRoom` is then called once with the full usernames batch (preserving batch-validation semantics), and the subsequent subscription/message processing loop iterates over the same stable `collectedUsers` array — no second DB query is made. This avoids any race condition between validation and processing while preserving the original batch-validation behavior.
Applied to files:
packages/model-typings/src/models/ISubscriptionsModel.tspackages/models/src/models/Subscriptions.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:
packages/model-typings/src/models/ISubscriptionsModel.tspackages/models/src/models/Subscriptions.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:
packages/model-typings/src/models/ISubscriptionsModel.tspackages/models/src/models/Subscriptions.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/model-typings/src/models/ISubscriptionsModel.tsapps/meteor/lib/getSubscriptionUnreadData.tspackages/models/src/models/Subscriptions.tsapps/meteor/app/reactions/server/notifications.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/model-typings/src/models/ISubscriptionsModel.tsapps/meteor/lib/getSubscriptionUnreadData.tspackages/models/src/models/Subscriptions.tsapps/meteor/app/reactions/server/notifications.ts
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.
Applied to files:
apps/meteor/client/sidebar/badges/SidebarItemBadges.tsxapps/meteor/client/sidebar/badges/ReactionBadge.tsx
📚 Learning: 2026-03-11T22:04:20.529Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39545
File: apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts:59-61
Timestamp: 2026-03-11T22:04:20.529Z
Learning: In `apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts`, the `msg.u._id === uid` early-return in the `streamNewMessage` handler is intentional: the "New messages" indicator is designed to notify about messages from other users only. Self-sent messages — including those sent from a different session/device — are always skipped, by design. Do not flag this as a multi-session regression.
Applied to files:
apps/meteor/app/reactions/server/notifications.ts
📚 Learning: 2026-04-18T12:32:53.425Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 38623
File: apps/meteor/app/lib/server/functions/cleanRoomHistory.ts:146-149
Timestamp: 2026-04-18T12:32:53.425Z
Learning: In `apps/meteor/app/lib/server/functions/cleanRoomHistory.ts` (PR `#38623`), the read-receipt cleanup (both `ReadReceipts.removeByMessageIds` and `ReadReceiptsArchive.removeByMessageIds`) is intentionally only performed in the limited prune path (`limit && selectedMessageIds`). The unlimited/delete-all path (`limit === 0`) deliberately skips cleaning up orphaned read receipts in both hot and cold storage — this is by design. Do not flag this as a bug or missing cleanup in future reviews.
Applied to files:
apps/meteor/app/reactions/server/notifications.ts
📚 Learning: 2026-02-20T09:04:55.725Z
Learnt from: Shreyas2004wagh
Repo: RocketChat/Rocket.Chat PR: 38681
File: apps/meteor/server/modules/streamer/streamer.module.ts:307-313
Timestamp: 2026-02-20T09:04:55.725Z
Learning: In apps/meteor/server/modules/streamer/streamer.module.ts, the catch block in sendToManySubscriptions intentionally uses SystemLogger.debug (not error or warn) for per-subscription delivery failures to keep logs less noisy, as this was a deliberate design choice reviewed and approved by KevLehman.
Applied to files:
apps/meteor/app/reactions/server/notifications.ts
📚 Learning: 2026-04-23T18:10:55.887Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39857
File: apps/meteor/app/api/server/middlewares/metrics.ts:25-57
Timestamp: 2026-04-23T18:10:55.887Z
Learning: In RocketChat/Rocket.Chat, the route handler action wrapper in `apps/meteor/app/api/server/ApiClass.ts` (around line 892) contains a `catch` block that intercepts all route-handler errors and converts them into proper HTTP API response objects (e.g., `api.failure`, `api.unauthorized`, `api.tooManyRequests`). As a result, `next()` in Hono middleware (such as `metricsMiddleware` in `apps/meteor/app/api/server/middlewares/metrics.ts`) will never throw. Do not flag missing try/finally guards around `await next()` calls in this middleware for gauge/counter decrement safety, as the execution flow is never disrupted by route-level errors.
Applied to files:
apps/meteor/app/reactions/server/notifications.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/reactions/server/notifications.ts
📚 Learning: 2026-03-20T13:51:23.302Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts:179-181
Timestamp: 2026-03-20T13:51:23.302Z
Learning: In `apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`, the truthiness guards `...(integration.avatar && { avatar })`, `...(integration.emoji && { emoji })`, `...(integration.alias && { alias })`, and `...(integration.script && { script })` in the `$set` payload of `updateIncomingIntegration` are intentional. Empty-string values for these fields should NOT overwrite the stored value — only truthy values are persisted. Do not flag these as bugs preventing explicit clears.
Applied to files:
apps/meteor/app/reactions/server/notifications.ts
📚 Learning: 2026-04-14T21:10:36.233Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 36292
File: apps/meteor/client/hooks/useHasValidLocationHash.ts:7-12
Timestamp: 2026-04-14T21:10:36.233Z
Learning: In the RocketChat/Rocket.Chat repository, adding JSDoc-style comments to React hooks (e.g., files under apps/meteor/client/hooks/) is considered a good pattern and should not be flagged as a violation of the "avoid code comments in the implementation" guideline. The guideline against code comments does not apply to JSDoc documentation on exported hooks.
Applied to files:
apps/meteor/app/reactions/server/notifications.ts
📚 Learning: 2025-12-09T20:01:07.355Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37532
File: ee/packages/federation-matrix/src/FederationMatrix.ts:920-927
Timestamp: 2025-12-09T20:01:07.355Z
Learning: In Rocket.Chat's federation invite handling (ee/packages/federation-matrix/src/FederationMatrix.ts), when a user rejects an invite via federationSDK.rejectInvite(), the subscription cleanup happens automatically through an event-driven flow: Matrix emits a leave event back, which is processed by handleLeave() in ee/packages/federation-matrix/src/events/member.ts, and that function calls Room.performUserRemoval() to clean up the subscription. No explicit cleanup is needed in the reject branch of handleInvite() because the leave event handler takes care of it.
<!-- </add_learning>
Applied to files:
apps/meteor/app/reactions/server/notifications.ts
🔇 Additional comments (6)
packages/models/src/models/Subscriptions.ts (2)
1580-1599: LGTM — mirrors existing increment patterns.The new
incReactionsForRoomIdAndUserIdsfollows the same shape asincUserMentionsAndUnreadForRoomIdAndUserIds(also settingalert: true,open: true) and uses$inconreactions, which safely treats a missing field as0. Note that this does mean a reaction will reopen a room the recipient explicitly hid viahideByRoomIdAndUserId; this is consistent with how mentions/unread already behave, so no action needed here unless product wants reaction-specific behavior.
219-219: Good — clears reactions counter on read.Resetting
reactions: 0here keeps theReactionBadgein sync when the user reads the room. Consistent with other counters reset in this method.packages/model-typings/src/models/ISubscriptionsModel.ts (1)
282-282: Type declaration matches implementation.
inc?: numbercorrectly aligns with theinc = 1default inSubscriptionsRaw.incReactionsForRoomIdAndUserIds.apps/meteor/lib/getSubscriptionUnreadData.ts (1)
4-58: LGTM — reactions cleanly added as a separate dimension.
reactionsis excluded fromtotal/countmath, so it won't double-count with message unreads, and the new title segment is appended last only when non-zero. Thereactions || 0normalization keeps the consumer types simple.apps/meteor/client/sidebar/badges/SidebarItemBadges.tsx (1)
21-21: LGTM.Conditional rendering on
unreadCount.reactions > 0and reusingunreadTitleis consistent with theUnreadBadgepattern above.apps/meteor/app/reactions/server/notifications.ts (1)
50-55: Concerns aboutas anycasts are overblown; the logic is sound.The
as anycasts onemoji.packagesandemoji.list[reaction]are justified—these are dynamically-keyed objects without complete static typing. The code correctly handles all emoji types:
Reaction keys already include colons —
setReactiontransforms to:${name}:format before storing, andemoji.listis consistently indexed the same way, so the lookup is correct.Custom emojis properly fallback to raw string — Custom emoji objects lack
uc_output, so the optional chaining?.uc_outputnaturally falls through to|| reaction, displaying the raw emoji name in the notification. This is intended behavior.The suggested functions don't exist —
replaceEmojiInlineandEmoji.parseare not in the codebase, so that refactoring suggestion is not viable.The code is well-defended with optional chaining and appropriate fallback logic. Consider removing this comment unless a concrete typing improvement is available.
> Likely an incorrect or invalid review comment.
… in reaction notifications
coderabbitai
Bot
removed
the
type: feature
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/meteor/app/reactions/server/notifications.ts (1)
49-54: Prefer the emoji module's typed surface; removeas anycasts onemoji.packagesandemoji.list.The triple
as anycasts on lines 49–53 bypass the module's exported type definitions (EmojiPackages), allowing typos in property names (emojione,uc_output) to silently fail at runtime instead of being caught at compile time. The emoji module already exports structured types for bothpackagesand list entries withuc_outputas a typed field; use them directly instead of casting toany.♻️ Suggested approach
- const emojione = (emoji.packages as any).emojione; - const emojiActual = - (emojione && - (emoji.list[reaction] as any)?.uc_output && - emojione.convert((emoji.list[reaction] as any).uc_output.toUpperCase())) || - reaction; + const emojione = emoji.packages.emojione; + const ucOutput = emoji.list[reaction]?.uc_output; + const emojiActual = (emojione && ucOutput && emojione.convert(ucOutput.toUpperCase())) || reaction;Per coding guidelines: "Write concise, technical TypeScript/JavaScript with accurate typing."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/reactions/server/notifications.ts` around lines 49 - 54, The code uses three "as any" casts around emoji.packages and emoji.list which defeats the module's types and hides typos; replace these casts by importing/using the module's exported types (e.g., EmojiPackages and the list entry type that includes uc_output) and type-annotate emoji.packages and emoji.list so you can access emojione and uc_output with proper typing; update the logic that sets emojiActual to use typed optional chaining (e.g., emoji.packages.emojione and emoji.list[reaction]?.uc_output) and fall back to reaction if missing—remove all "as any" casts around emojione, emoji.packages, and emoji.list.
🤖 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/reactions/server/notifications.ts`:
- Around line 79-130: The unset handler (callbacks.add 'afterUnsetReaction')
currently gates decrementing on recipient.active and
recipient.settings.preferences.receiveReactionNotifications which can cause
counter drift; update the handler to keep the early returns for
Troubleshoot_Disable_Notifications and self-reactions but remove the checks that
read recipient.active and receiveReactionNotifications so it always runs the
Subscriptions.updateOne({ rid: room._id, 'u._id': recipientId, reactions: { $gt:
0 } }, { $inc: { reactions: -1 } }) and calls
notifyOnSubscriptionChangedByRoomIdAndUserId(room._id, recipientId); keep the
existing $gt: 0 safety and existing try/catch and logging.
- Around line 64-70: The notifyDesktopUser call is missing the optional icon?:
string parameter; update the invocation of notifyDesktopUser (the call that
passes userId, user, message, room, notificationMessage) to include the reaction
icon using the emoji-related variable already extracted earlier (e.g., emoji,
emojiUrl, or reactionEmoji from lines 49–54) so the desktop notification uses
that icon to distinguish reaction notifications from regular message
notifications.
---
Nitpick comments:
In `@apps/meteor/app/reactions/server/notifications.ts`:
- Around line 49-54: The code uses three "as any" casts around emoji.packages
and emoji.list which defeats the module's types and hides typos; replace these
casts by importing/using the module's exported types (e.g., EmojiPackages and
the list entry type that includes uc_output) and type-annotate emoji.packages
and emoji.list so you can access emojione and uc_output with proper typing;
update the logic that sets emojiActual to use typed optional chaining (e.g.,
emoji.packages.emojione and emoji.list[reaction]?.uc_output) and fall back to
reaction if missing—remove all "as any" casts around emojione, emoji.packages,
and emoji.list.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 09128731-f075-4c2f-bb0c-73f7f2619227
📒 Files selected for processing (2)
apps/meteor/app/reactions/server/notifications.tsapps/meteor/client/sidebar/badges/ReactionBadge.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/client/sidebar/badges/ReactionBadge.tsx
📜 Review details
🧰 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/reactions/server/notifications.ts
🧠 Learnings (10)
📓 Common learnings
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
📚 Learning: 2026-03-11T22:04:20.529Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39545
File: apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts:59-61
Timestamp: 2026-03-11T22:04:20.529Z
Learning: In `apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts`, the `msg.u._id === uid` early-return in the `streamNewMessage` handler is intentional: the "New messages" indicator is designed to notify about messages from other users only. Self-sent messages — including those sent from a different session/device — are always skipped, by design. Do not flag this as a multi-session regression.
Applied to files:
apps/meteor/app/reactions/server/notifications.ts
📚 Learning: 2026-02-20T09:04:55.725Z
Learnt from: Shreyas2004wagh
Repo: RocketChat/Rocket.Chat PR: 38681
File: apps/meteor/server/modules/streamer/streamer.module.ts:307-313
Timestamp: 2026-02-20T09:04:55.725Z
Learning: In apps/meteor/server/modules/streamer/streamer.module.ts, the catch block in sendToManySubscriptions intentionally uses SystemLogger.debug (not error or warn) for per-subscription delivery failures to keep logs less noisy, as this was a deliberate design choice reviewed and approved by KevLehman.
Applied to files:
apps/meteor/app/reactions/server/notifications.ts
📚 Learning: 2026-04-18T12:32:53.425Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 38623
File: apps/meteor/app/lib/server/functions/cleanRoomHistory.ts:146-149
Timestamp: 2026-04-18T12:32:53.425Z
Learning: In `apps/meteor/app/lib/server/functions/cleanRoomHistory.ts` (PR `#38623`), the read-receipt cleanup (both `ReadReceipts.removeByMessageIds` and `ReadReceiptsArchive.removeByMessageIds`) is intentionally only performed in the limited prune path (`limit && selectedMessageIds`). The unlimited/delete-all path (`limit === 0`) deliberately skips cleaning up orphaned read receipts in both hot and cold storage — this is by design. Do not flag this as a bug or missing cleanup in future reviews.
Applied to files:
apps/meteor/app/reactions/server/notifications.ts
📚 Learning: 2026-04-23T18:10:55.887Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39857
File: apps/meteor/app/api/server/middlewares/metrics.ts:25-57
Timestamp: 2026-04-23T18:10:55.887Z
Learning: In RocketChat/Rocket.Chat, the route handler action wrapper in `apps/meteor/app/api/server/ApiClass.ts` (around line 892) contains a `catch` block that intercepts all route-handler errors and converts them into proper HTTP API response objects (e.g., `api.failure`, `api.unauthorized`, `api.tooManyRequests`). As a result, `next()` in Hono middleware (such as `metricsMiddleware` in `apps/meteor/app/api/server/middlewares/metrics.ts`) will never throw. Do not flag missing try/finally guards around `await next()` calls in this middleware for gauge/counter decrement safety, as the execution flow is never disrupted by route-level errors.
Applied to files:
apps/meteor/app/reactions/server/notifications.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/reactions/server/notifications.ts
📚 Learning: 2026-03-20T13:51:23.302Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts:179-181
Timestamp: 2026-03-20T13:51:23.302Z
Learning: In `apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`, the truthiness guards `...(integration.avatar && { avatar })`, `...(integration.emoji && { emoji })`, `...(integration.alias && { alias })`, and `...(integration.script && { script })` in the `$set` payload of `updateIncomingIntegration` are intentional. Empty-string values for these fields should NOT overwrite the stored value — only truthy values are persisted. Do not flag these as bugs preventing explicit clears.
Applied to files:
apps/meteor/app/reactions/server/notifications.ts
📚 Learning: 2026-04-14T21:10:36.233Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 36292
File: apps/meteor/client/hooks/useHasValidLocationHash.ts:7-12
Timestamp: 2026-04-14T21:10:36.233Z
Learning: In the RocketChat/Rocket.Chat repository, adding JSDoc-style comments to React hooks (e.g., files under apps/meteor/client/hooks/) is considered a good pattern and should not be flagged as a violation of the "avoid code comments in the implementation" guideline. The guideline against code comments does not apply to JSDoc documentation on exported hooks.
Applied to files:
apps/meteor/app/reactions/server/notifications.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/reactions/server/notifications.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/reactions/server/notifications.ts
Comment on lines
+64
to
+70
| await notifyDesktopUser({ | ||
| userId: recipient._id, | ||
| user, | ||
| message, | ||
| room, | ||
| notificationMessage, | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm notifyDesktopUser still accepts `icon` and check for any reaction-specific icon helpers/assets.
rg -nP --type=ts -C5 'function notifyDesktopUser' apps/meteor
rg -nP --type=ts -C2 'icon\?\s*:\s*string' apps/meteor/app/lib/server/functions/notifications
rg -nP --type=ts -C2 'getEmojiUrl|emojiUrl|reactionIcon' apps/meteor packagesRepository: RocketChat/Rocket.Chat
Length of output: 3042
🏁 Script executed:
#!/bin/bash
# Check full implementation of notifyDesktopUser including how icon is used
cat -n apps/meteor/app/lib/server/functions/notifications/desktop.ts | head -80
# Search for server-side emoji or reaction icon utilities
rg -n 'getEmojiUrl|reactionIcon|emojiUrl' apps/meteor/app --type=ts | head -20
# Check for any constant assets or icons related to reactions
rg -n 'icon|emoji' apps/meteor/app/reactions/server --type=ts -A 2 -B 2Repository: RocketChat/Rocket.Chat
Length of output: 6004
🏁 Script executed:
#!/bin/bash
# Read the full reactions/server/notifications.ts file to see the complete context
cat -n apps/meteor/app/reactions/server/notifications.tsRepository: RocketChat/Rocket.Chat
Length of output: 4613
Pass an icon parameter to distinguish reaction notifications from message notifications.
The notifyDesktopUser function accepts an optional icon?: string parameter, but the call at line 64–70 doesn't provide one. Since emoji data is already being extracted (lines 49–54), consider passing a reaction icon URL or asset to help users visually distinguish reaction notifications from regular message notifications at the OS level.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/reactions/server/notifications.ts` around lines 64 - 70, The
notifyDesktopUser call is missing the optional icon?: string parameter; update
the invocation of notifyDesktopUser (the call that passes userId, user, message,
room, notificationMessage) to include the reaction icon using the emoji-related
variable already extracted earlier (e.g., emoji, emojiUrl, or reactionEmoji from
lines 49–54) so the desktop notification uses that icon to distinguish reaction
notifications from regular message notifications.
Comment on lines
+79
to
+130
| callbacks.add( | ||
| 'afterUnsetReaction', | ||
| async (message, { user, room }) => { | ||
| try { | ||
| if (settings.get<boolean>('Troubleshoot_Disable_Notifications') === true) { | ||
| return; | ||
| } | ||
|
|
||
| if (!message.u?._id || message.u._id === user._id) { | ||
| return; | ||
| } | ||
|
|
||
| const recipient = await Users.findOneById(message.u._id, { | ||
| projection: { | ||
| 'active': 1, | ||
| 'settings.preferences.receiveReactionNotifications': 1, | ||
| }, | ||
| }); | ||
|
|
||
| if (!recipient || !recipient.active) { | ||
| return; | ||
| } | ||
|
|
||
| const receiveReactionNotifications = | ||
| recipient.settings?.preferences?.receiveReactionNotifications ?? | ||
| settings.get<boolean>('Accounts_Default_User_Preferences_receiveReactionNotifications'); | ||
|
|
||
| if (!receiveReactionNotifications) { | ||
| return; | ||
| } | ||
|
|
||
| const recipientId = message.u._id; | ||
|
|
||
| await Subscriptions.updateOne( | ||
| { | ||
| rid: room._id, | ||
| 'u._id': recipientId, | ||
| reactions: { $gt: 0 }, | ||
| }, | ||
| { | ||
| $inc: { reactions: -1 }, | ||
| }, | ||
| ); | ||
|
|
||
| void notifyOnSubscriptionChangedByRoomIdAndUserId(room._id, recipientId); | ||
| } catch (e) { | ||
| SystemLogger.error({ msg: 'Error handling reaction notification removal', err: e }); | ||
| } | ||
| }, | ||
| callbacks.priority.LOW, | ||
| 'RecordReactionNotificationRemoval', | ||
| ); |
Contributor
There was a problem hiding this comment.
Counter drift when preference/active state changes between set and unset.
The set and unset handlers gate on the recipient's active flag and receiveReactionNotifications preference. If those evaluate true at set time (counter +1) and false at unset time (handler returns early before decrementing), the reactions counter drifts upward permanently for that subscription — leaving a stale reaction badge that can never be cleared via this flow. Equivalent drift occurs in the reverse direction if the user toggles the preference on between set and unset.
Since the increment is the side effect that creates user-visible state, the symmetric guard for unset should arguably be "did we previously increment?" rather than "would we increment now?". A few options:
- Always decrement on unset (after the self-reaction and
Troubleshoot_Disable_Notificationsguards), sincereactions: { $gt: 0 }already prevents going negative and the counter is reaction-specific (not aliased withunreadanymore). - Or, persist a per-reaction marker so unset only decrements reactions whose set was actually counted.
The first option is the smallest change and matches the existing $gt: 0 safety net.
🛠️ Minimal fix: drop the active/preference gate in unset
callbacks.add(
'afterUnsetReaction',
async (message, { user, room }) => {
try {
if (settings.get<boolean>('Troubleshoot_Disable_Notifications') === true) {
return;
}
if (!message.u?._id || message.u._id === user._id) {
return;
}
- const recipient = await Users.findOneById(message.u._id, {
- projection: {
- 'active': 1,
- 'settings.preferences.receiveReactionNotifications': 1,
- },
- });
-
- if (!recipient || !recipient.active) {
- return;
- }
-
- const receiveReactionNotifications =
- recipient.settings?.preferences?.receiveReactionNotifications ??
- settings.get<boolean>('Accounts_Default_User_Preferences_receiveReactionNotifications');
-
- if (!receiveReactionNotifications) {
- return;
- }
-
const recipientId = message.u._id;
await Subscriptions.updateOne(
{
rid: room._id,
'u._id': recipientId,
reactions: { $gt: 0 },
},
{
$inc: { reactions: -1 },
},
);
void notifyOnSubscriptionChangedByRoomIdAndUserId(room._id, recipientId);📝 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
| callbacks.add( | |
| 'afterUnsetReaction', | |
| async (message, { user, room }) => { | |
| try { | |
| if (settings.get<boolean>('Troubleshoot_Disable_Notifications') === true) { | |
| return; | |
| } | |
| if (!message.u?._id || message.u._id === user._id) { | |
| return; | |
| } | |
| const recipient = await Users.findOneById(message.u._id, { | |
| projection: { | |
| 'active': 1, | |
| 'settings.preferences.receiveReactionNotifications': 1, | |
| }, | |
| }); | |
| if (!recipient || !recipient.active) { | |
| return; | |
| } | |
| const receiveReactionNotifications = | |
| recipient.settings?.preferences?.receiveReactionNotifications ?? | |
| settings.get<boolean>('Accounts_Default_User_Preferences_receiveReactionNotifications'); | |
| if (!receiveReactionNotifications) { | |
| return; | |
| } | |
| const recipientId = message.u._id; | |
| await Subscriptions.updateOne( | |
| { | |
| rid: room._id, | |
| 'u._id': recipientId, | |
| reactions: { $gt: 0 }, | |
| }, | |
| { | |
| $inc: { reactions: -1 }, | |
| }, | |
| ); | |
| void notifyOnSubscriptionChangedByRoomIdAndUserId(room._id, recipientId); | |
| } catch (e) { | |
| SystemLogger.error({ msg: 'Error handling reaction notification removal', err: e }); | |
| } | |
| }, | |
| callbacks.priority.LOW, | |
| 'RecordReactionNotificationRemoval', | |
| ); | |
| callbacks.add( | |
| 'afterUnsetReaction', | |
| async (message, { user, room }) => { | |
| try { | |
| if (settings.get<boolean>('Troubleshoot_Disable_Notifications') === true) { | |
| return; | |
| } | |
| if (!message.u?._id || message.u._id === user._id) { | |
| return; | |
| } | |
| const recipientId = message.u._id; | |
| await Subscriptions.updateOne( | |
| { | |
| rid: room._id, | |
| 'u._id': recipientId, | |
| reactions: { $gt: 0 }, | |
| }, | |
| { | |
| $inc: { reactions: -1 }, | |
| }, | |
| ); | |
| void notifyOnSubscriptionChangedByRoomIdAndUserId(room._id, recipientId); | |
| } catch (e) { | |
| SystemLogger.error({ msg: 'Error handling reaction notification removal', err: e }); | |
| } | |
| }, | |
| callbacks.priority.LOW, | |
| 'RecordReactionNotificationRemoval', | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/reactions/server/notifications.ts` around lines 79 - 130, The
unset handler (callbacks.add 'afterUnsetReaction') currently gates decrementing
on recipient.active and
recipient.settings.preferences.receiveReactionNotifications which can cause
counter drift; update the handler to keep the early returns for
Troubleshoot_Disable_Notifications and self-reactions but remove the checks that
read recipient.active and receiveReactionNotifications so it always runs the
Subscriptions.updateOne({ rid: room._id, 'u._id': recipientId, reactions: { $gt:
0 } }, { $inc: { reactions: -1 } }) and calls
notifyOnSubscriptionChangedByRoomIdAndUserId(room._id, recipientId); keep the
existing $gt: 0 safety and existing try/catch and logging.
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
3 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.
Proposed changes (including video)
Screencast.from.2026-04-25.01-59-19.webm
This PR introduces desktop notifications and unread status tracking for message reactions.
Key improvements:
notifications.tsserver file using hooks (afterSetReaction,afterUnsetReaction) to keep the core reaction logic decoupled from notification delivery.Issue(s)
Closes #40293
Steps to test or reproduce
Summary by CodeRabbit
New Features
UI
Settings
Localization