chore: prevent federated room from being handled by read receipt #37721
Conversation
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
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 |
🦋 Changeset detectedLatest commit: 7855153 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
WalkthroughPass room objects (IRoom) instead of room IDs across read-message APIs and callbacks; validate room existence in Subscriptions.read; skip read-receipt processing for federated rooms and use room._id when marking reads; add client-side gating to disable read receipts for federated rooms. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API as Subscriptions API
participant Server as readMessages
participant Callbacks
participant ReadReceipt
Client->>API: request markAsRead(roomId)
API->>Server: fetch room (Rooms.findOneById(roomId))
Server-->>API: room object (validated)
API->>Server: readMessages(room, userId, readThreads)
Server->>Callbacks: run beforeReadMessages(room, {uid,...})
Server->>Subscriptions: setAsReadByRoomIdAndUserId(room._id, userId)
Server->>Callbacks: run afterReadMessages(room, {uid, tmid})
alt room not federated
Callbacks->>ReadReceipt: markMessagesAsRead(room._id, uid, lastSeen)
else room federated
Callbacks-->>Server: skip read-receipt processing
end
Server-->>Client: confirmation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
ggazzo
changed the title
Contributor
📦 Docker Image Size Report📈 Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/15 22:28", "11/16 01:28", "11/17 23:50", "11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 19:56", "12/10 21:49 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12]
line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "queue-worker-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.35]
line "stream-hub-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
Statistics (last 17 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Wed, 10 Dec 2025 21:49:12 GMT |
ggazzo
force-pushed
the
chore/disable-read-receipt
branch
from
0635537 to
900bc6d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37721 +/- ##
===========================================
- Coverage 67.78% 67.70% -0.09%
===========================================
Files 3452 3452
Lines 113975 113975
Branches 20940 20941 +1
===========================================
- Hits 77262 77167 -95
- Misses 34580 34680 +100
+ Partials 2133 2128 -5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![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: 0
🧹 Nitpick comments (1)
apps/meteor/lib/callbacks.ts (1)
46-46: Rename parameterridtoroomfor clarity.The parameter type changed from
IRoom['_id']toIRoom, but the parameter namerid(conventionally "room id") is misleading. Renaming toroomimproves readability and prevents confusion.Apply this diff:
- 'afterReadMessages': (rid: IRoom, params: { uid: IUser['_id']; lastSeen?: Date; tmid?: IMessage['_id'] }) => void; + 'afterReadMessages': (room: IRoom, params: { uid: IUser['_id']; lastSeen?: Date; tmid?: IMessage['_id'] }) => void;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
apps/meteor/app/api/server/v1/subscriptions.ts(2 hunks)apps/meteor/app/threads/server/methods/getThreadMessages.ts(1 hunks)apps/meteor/ee/app/message-read-receipt/server/hooks/afterReadMessages.ts(1 hunks)apps/meteor/lib/callbacks.ts(1 hunks)apps/meteor/server/lib/readMessages.ts(1 hunks)apps/meteor/server/methods/readMessages.ts(1 hunks)apps/meteor/server/methods/readThreads.ts(1 hunks)
🧰 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/server/methods/readThreads.tsapps/meteor/app/api/server/v1/subscriptions.tsapps/meteor/server/lib/readMessages.tsapps/meteor/lib/callbacks.tsapps/meteor/ee/app/message-read-receipt/server/hooks/afterReadMessages.tsapps/meteor/app/threads/server/methods/getThreadMessages.tsapps/meteor/server/methods/readMessages.ts
🧠 Learnings (8)
📓 Common learnings
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/server/methods/readThreads.tsapps/meteor/app/api/server/v1/subscriptions.tsapps/meteor/server/lib/readMessages.tsapps/meteor/lib/callbacks.tsapps/meteor/ee/app/message-read-receipt/server/hooks/afterReadMessages.tsapps/meteor/app/threads/server/methods/getThreadMessages.tsapps/meteor/server/methods/readMessages.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/server/methods/readThreads.tsapps/meteor/app/api/server/v1/subscriptions.tsapps/meteor/server/lib/readMessages.tsapps/meteor/app/threads/server/methods/getThreadMessages.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/server/methods/readThreads.tsapps/meteor/app/api/server/v1/subscriptions.tsapps/meteor/server/lib/readMessages.tsapps/meteor/app/threads/server/methods/getThreadMessages.ts
📚 Learning: 2025-11-27T17:56:26.050Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
Applied to files:
apps/meteor/app/api/server/v1/subscriptions.ts
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
apps/meteor/ee/app/message-read-receipt/server/hooks/afterReadMessages.tsapps/meteor/server/methods/readMessages.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: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
apps/meteor/ee/app/message-read-receipt/server/hooks/afterReadMessages.tsapps/meteor/app/threads/server/methods/getThreadMessages.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/threads/server/methods/getThreadMessages.tsapps/meteor/server/methods/readMessages.ts
🧬 Code graph analysis (7)
apps/meteor/server/methods/readThreads.ts (1)
apps/meteor/lib/callbacks.ts (1)
callbacks(252-260)
apps/meteor/app/api/server/v1/subscriptions.ts (2)
apps/meteor/server/lib/readMessages.ts (1)
readMessages(7-28)apps/meteor/server/methods/readMessages.ts (1)
readMessages(18-38)
apps/meteor/server/lib/readMessages.ts (5)
apps/meteor/server/methods/readMessages.ts (1)
readMessages(18-38)packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)packages/core-typings/src/IUser.ts (1)
IUser(186-256)apps/meteor/server/methods/readThreads.ts (1)
readThreads(20-50)apps/meteor/app/lib/server/lib/notifyListener.ts (1)
notifyOnSubscriptionChangedByRoomIdAndUserId(521-533)
apps/meteor/lib/callbacks.ts (3)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)packages/core-typings/src/IUser.ts (1)
IUser(186-256)packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(162-260)
apps/meteor/ee/app/message-read-receipt/server/hooks/afterReadMessages.ts (1)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)
apps/meteor/app/threads/server/methods/getThreadMessages.ts (1)
apps/meteor/lib/callbacks.ts (1)
callbacks(252-260)
apps/meteor/server/methods/readMessages.ts (1)
apps/meteor/server/lib/readMessages.ts (1)
readMessages(7-28)
🔇 Additional comments (7)
apps/meteor/server/methods/readThreads.ts (1)
48-48: LGTM! Callback signature aligned with room object pattern.The change correctly passes the full room object to the
afterReadMessagescallback, consistent with the PR's refactoring to enable federated room filtering.apps/meteor/server/methods/readMessages.ts (1)
37-37: LGTM! Method correctly invokes readMessages with room object.The call site correctly passes the validated room object to
readMessages, aligning with the updated signature.apps/meteor/app/threads/server/methods/getThreadMessages.ts (1)
58-59: LGTM! Callback invocation updated correctly.The
afterReadMessagescallback now receives the full room object, consistent with the signature change across the codebase.apps/meteor/server/lib/readMessages.ts (1)
7-28: LGTM! Comprehensive refactoring to room object pattern.The function signature and all internal references correctly updated to work with the room object. All downstream calls properly use
room._idwhere room IDs are required.apps/meteor/app/api/server/v1/subscriptions.ts (1)
88-93: LGTM! Room validation ensures safe downstream processing.The added room existence check prevents invalid operations and the room object is correctly passed to
readMessages. This aligns with the consistent pattern across the PR.apps/meteor/ee/app/message-read-receipt/server/hooks/afterReadMessages.ts (2)
10-17: LGTM! Federated room guard correctly prevents read receipt processing.The callback signature correctly updated to receive the room object, and the new federated room guard (lines 15-17) implements the PR's core objective: preventing read receipt computation for federated rooms. The guard is positioned appropriately after the settings check.
23-23: LGTM! Correct usage of room._id for downstream call.The call to
ReadReceipt.markMessagesAsReadcorrectly extractsroom._idfrom the room object, maintaining the expected signature.
aleksandernsilva
previously requested changes
Dec 8, 2025
aleksandernsilva
dismissed
their stale review
changes requested were addressed
…of room ID - Modified the readMessages function to take a room object as a parameter. - Updated related callbacks and method calls to use the room object. - Ensured consistency across the application by adjusting the afterReadMessages callback to accept the room object.
- Implemented a check to prevent processing of read receipts for federated rooms. - Enhanced the afterReadMessages callback to ensure compatibility with room federation.
Co-authored-by: Aleksander Nicacio da Silva <aleksander.silva@rocket.chat>
Co-authored-by: Guilherme Gazzo <guilherme@gazzo.xyz>
ggazzo
force-pushed
the
chore/disable-read-receipt
branch
from
16d9517 to
d6fd99a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/ee/app/message-read-receipt/server/hooks/afterReadMessages.ts (1)
10-24: LGTM! Federation guard correctly implemented.The changes correctly:
- Update the callback signature to receive the room object
- Add an early return for federated rooms to prevent read receipt processing
- Use
room._idfor the read receipt marking callThe implementation aligns with the PR objective to prevent federated rooms from having read receipts computed.
Minor optional improvement: The comment on line 14 could read "Federated rooms are not supported yet" for slightly better grammar.
- // Rooms federated are not supported yet + // Federated rooms are not supported yet
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
.changeset/cold-chefs-rhyme.md(1 hunks)apps/meteor/app/api/server/v1/subscriptions.ts(2 hunks)apps/meteor/app/threads/server/methods/getThreadMessages.ts(1 hunks)apps/meteor/client/components/message/list/MessageListContext.tsx(2 hunks)apps/meteor/client/components/message/toolbar/useReadReceiptsDetailsAction.spec.tsx(1 hunks)apps/meteor/client/components/message/variants/RoomMessage.spec.tsx(2 hunks)apps/meteor/client/views/room/MessageList/providers/MessageListProvider.tsx(2 hunks)apps/meteor/ee/app/message-read-receipt/server/hooks/afterReadMessages.ts(1 hunks)apps/meteor/lib/callbacks.ts(1 hunks)apps/meteor/server/lib/readMessages.ts(1 hunks)apps/meteor/server/methods/readMessages.ts(1 hunks)apps/meteor/server/methods/readThreads.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/app/threads/server/methods/getThreadMessages.ts
- apps/meteor/app/api/server/v1/subscriptions.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/lib/callbacks.tsapps/meteor/client/components/message/variants/RoomMessage.spec.tsxapps/meteor/client/components/message/toolbar/useReadReceiptsDetailsAction.spec.tsxapps/meteor/client/components/message/list/MessageListContext.tsxapps/meteor/client/views/room/MessageList/providers/MessageListProvider.tsxapps/meteor/ee/app/message-read-receipt/server/hooks/afterReadMessages.tsapps/meteor/server/methods/readMessages.tsapps/meteor/server/lib/readMessages.tsapps/meteor/server/methods/readThreads.ts
🧠 Learnings (15)
📓 Common learnings
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/lib/callbacks.tsapps/meteor/client/views/room/MessageList/providers/MessageListProvider.tsxapps/meteor/ee/app/message-read-receipt/server/hooks/afterReadMessages.tsapps/meteor/server/methods/readMessages.tsapps/meteor/server/lib/readMessages.ts.changeset/cold-chefs-rhyme.mdapps/meteor/server/methods/readThreads.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/lib/callbacks.tsapps/meteor/server/lib/readMessages.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/lib/callbacks.tsapps/meteor/server/lib/readMessages.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/client/components/message/variants/RoomMessage.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/client/components/message/variants/RoomMessage.spec.tsxapps/meteor/client/components/message/toolbar/useReadReceiptsDetailsAction.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/client/components/message/variants/RoomMessage.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright tests
Applied to files:
apps/meteor/client/components/message/variants/RoomMessage.spec.tsxapps/meteor/client/components/message/toolbar/useReadReceiptsDetailsAction.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/client/components/message/toolbar/useReadReceiptsDetailsAction.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/client/components/message/toolbar/useReadReceiptsDetailsAction.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/client/components/message/toolbar/useReadReceiptsDetailsAction.spec.tsx
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
apps/meteor/client/views/room/MessageList/providers/MessageListProvider.tsxapps/meteor/ee/app/message-read-receipt/server/hooks/afterReadMessages.tsapps/meteor/server/methods/readMessages.ts.changeset/cold-chefs-rhyme.md
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
apps/meteor/client/views/room/MessageList/providers/MessageListProvider.tsx.changeset/cold-chefs-rhyme.md
📚 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: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
apps/meteor/ee/app/message-read-receipt/server/hooks/afterReadMessages.tsapps/meteor/server/lib/readMessages.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/server/lib/readMessages.ts
🧬 Code graph analysis (7)
apps/meteor/client/components/message/variants/RoomMessage.spec.tsx (2)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)apps/meteor/client/components/message/list/MessageListContext.tsx (2)
MessageListContext(79-79)messageListContextDefaultValue(48-77)
apps/meteor/client/components/message/toolbar/useReadReceiptsDetailsAction.spec.tsx (3)
apps/meteor/client/components/message/list/MessageListContext.tsx (1)
useMessageListReadReceipts(107-107)apps/meteor/tests/mocks/data.ts (1)
createFakeMessage(93-108)packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)
apps/meteor/client/views/room/MessageList/providers/MessageListProvider.tsx (1)
packages/core-typings/src/IRoom.ts (1)
isRoomFederated(123-123)
apps/meteor/ee/app/message-read-receipt/server/hooks/afterReadMessages.ts (1)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)
apps/meteor/server/methods/readMessages.ts (2)
apps/meteor/server/lib/readMessages.ts (1)
readMessages(7-28)apps/meteor/server/methods/readThreads.ts (1)
readThreads(20-50)
apps/meteor/server/lib/readMessages.ts (6)
apps/meteor/server/methods/readMessages.ts (1)
readMessages(18-38)packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)packages/core-typings/src/IUser.ts (1)
IUser(186-256)apps/meteor/server/methods/readThreads.ts (1)
readThreads(20-50)apps/meteor/lib/callbacks.ts (1)
callbacks(252-260)apps/meteor/app/lib/server/lib/notifyListener.ts (1)
notifyOnSubscriptionChangedByRoomIdAndUserId(521-533)
apps/meteor/server/methods/readThreads.ts (1)
apps/meteor/lib/callbacks.ts (1)
callbacks(252-260)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (11)
.changeset/cold-chefs-rhyme.md (1)
1-5: LGTM!The changeset is properly formatted and accurately describes the change. The patch version bump is appropriate for this feature modification, and the message clearly communicates the intent to disable read receipts in federated rooms with a note on future re-enablement.
apps/meteor/client/components/message/list/MessageListContext.tsx (1)
48-79: LGTM! Clean extraction of default context value for testability.Exporting
messageListContextDefaultValueenables tests to spread and override specific properties while retaining sensible defaults. This is a good pattern for context testing.apps/meteor/client/components/message/variants/RoomMessage.spec.tsx (2)
111-134: LGTM! Good test coverage for read receipt enabled state.The test correctly wraps the component with a context provider that enables read receipts and verifies the expected status element is rendered.
136-159: LGTM! Proper negative test case for disabled read receipts.This complements the enabled test by verifying the status element is absent when receipts are disabled.
apps/meteor/client/components/message/toolbar/useReadReceiptsDetailsAction.spec.tsx (1)
1-50: LGTM! Comprehensive test coverage for the hook's conditional logic.The three test cases properly cover all branches of the read receipts action visibility logic. Good use of
jest.mockfor isolation andcreateFakeMessagefor test data.apps/meteor/server/methods/readThreads.ts (1)
48-48: LGTM! Callback now receives the room object for federation-aware processing.This change aligns with the PR objective to enable downstream callbacks to inspect room properties (e.g., federation status) without additional lookups.
apps/meteor/server/methods/readMessages.ts (1)
37-37: LGTM! Consistent with updatedreadMessagessignature.Passing the room object (already fetched for access control) enables federation-aware processing in the underlying
readMessagesfunction without additional database lookups.apps/meteor/lib/callbacks.ts (1)
46-46: LGTM! Callback signature correctly updated.The signature change from room ID to room object is well-coordinated across the codebase. All call sites and implementations have been updated consistently.
apps/meteor/client/views/room/MessageList/providers/MessageListProvider.tsx (2)
1-1: LGTM! Import correctly added.The
isRoomFederatedtype guard is the appropriate utility for checking federation status client-side.
43-43: LGTM! Federation check correctly prevents read receipts UI in federated rooms.This client-side guard ensures read receipt indicators won't be displayed in federated rooms, complementing the server-side processing guard for defense in depth.
apps/meteor/server/lib/readMessages.ts (1)
7-27: LGTM! Function refactored correctly to use room object.All references have been consistently updated:
- Function parameter changed from
ridtoroom- Internal calls use
room._idwhere room ID is required- Callback invocations correctly pass the room object
The refactoring maintains type safety and existing error handling logic.
sampaiodiego
requested changes
Dec 10, 2025
apps/meteor/ee/app/message-read-receipt/server/hooks/afterReadMessages.ts
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/ee/app/message-read-receipt/server/hooks/afterReadMessages.ts (1)
14-17: Federation guard correctly implements the PR objective.The early return for federated rooms prevents read-receipt processing as intended. The use of
isRoomFederated(room)type guard is appropriate.Consider rewording the comment for clarity:
- // Rooms federated are not supported yet + // Federated rooms are not supported yet
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/meteor/ee/app/message-read-receipt/server/hooks/afterReadMessages.ts(1 hunks)
🧰 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/ee/app/message-read-receipt/server/hooks/afterReadMessages.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/ee/app/message-read-receipt/server/hooks/afterReadMessages.ts
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
apps/meteor/ee/app/message-read-receipt/server/hooks/afterReadMessages.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/ee/app/message-read-receipt/server/hooks/afterReadMessages.ts
🧬 Code graph analysis (1)
apps/meteor/ee/app/message-read-receipt/server/hooks/afterReadMessages.ts (2)
packages/core-typings/src/IRoom.ts (2)
IRoom(21-95)isRoomFederated(123-123)apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts (1)
ReadReceipt(164-164)
🔇 Additional comments (3)
apps/meteor/ee/app/message-read-receipt/server/hooks/afterReadMessages.ts (3)
2-2: LGTM! Type imports are appropriate.The added type imports (
IUser,IRoom,IMessage) support the improved callback signature, andisRoomFederatedenables the federated room guard as per the PR objective.
10-10: LGTM! Callback signature improves type safety.The change from accepting
ridto accepting the fullroomobject enables the federation check and improves type safety with explicit parameter typing.
23-23: LGTM! Correctly uses room._id.The change from
ridtoroom._idis consistent with the updated callback signature and correctly extracts the room ID from the room object.
sampaiodiego
approved these changes
Dec 10, 2025
aleksandernsilva
approved these changes
Dec 10, 2025
ggazzo
added
the
stat: QA assured
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
https://rocketchat.atlassian.net/browse/FB-76
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.