chore: prevent read receipts from loading unnecessary data when not saving detailed info#37280
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRefactors read-receipt handling into a singleton class with per-room debouncing and callback-based message retrieval, tightens typings (adds IReadReceipt / IReadReceiptWithUser), makes storeReadReceipts private, updates public method signatures, adds messages model thread rid parameter and index, and adds raw model deletion helpers. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ReadReceipt as ReadReceiptSingleton
participant DebounceMap as DebounceMap(room._id)
participant getMessages as getMessages()
participant Storage
Caller->>ReadReceipt: markMessagesAsRead(room: IRoom, userId, userLastSeen: Date)
activate ReadReceipt
rect rgba(200,220,255,0.18)
Note over ReadReceipt,DebounceMap: per-room debounce keyed by room._id
ReadReceipt->>DebounceMap: set/refresh timeout for room._id
end
Note over ReadReceipt: on debounce expiry -> invoke getMessages callback
ReadReceipt->>getMessages: getMessages()
activate getMessages
getMessages-->>ReadReceipt: messages[]
deactivate getMessages
ReadReceipt->>ReadReceipt: build receipts (include t,pinned,drid,tmid) + extraData
ReadReceipt->>Storage: persist receipts (private storeReadReceipts)
deactivate ReadReceipt
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (12)
🚧 Files skipped from review as they are similar to previous changes (5)
🧰 Additional context used🧬 Code graph analysis (7)apps/meteor/client/views/room/modals/ReadReceiptsModal/ReadReceiptRow.tsx (1)
apps/meteor/definition/IRoomTypeConfig.ts (1)
apps/meteor/ee/server/models/raw/ReadReceipts.ts (2)
packages/core-typings/src/IReadReceipt.ts (3)
packages/models/src/dummy/ReadReceipts.ts (2)
apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts (4)
packages/model-typings/src/models/IReadReceiptsModel.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (19)
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 |
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes read receipt data loading by preventing unnecessary database queries when detailed user information storage is disabled. The main change refactors the ReadReceipt module to defer message loading until it's confirmed that detailed read receipt storage is enabled, avoiding wasted database operations when Message_Read_Receipt_Store_Users is disabled.
Key Changes:
- Refactored
storeReadReceiptsto accept a function that returns messages instead of the messages directly, enabling lazy evaluation - Converted the
ReadReceiptobject to a class with proper TypeScript typing - Updated the
ReadReceipttype definition to remove user details and add message-related fields
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/core-typings/src/ReadReceipt.ts | Updated ReadReceipt type to replace user details with message metadata fields (t, pinned, drid, tmid) and added token field |
| apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts | Refactored read receipt logic to use lazy message loading, converted to class-based structure, and added comprehensive TypeScript types |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts (1)
117-152: Excellent centralization with effective optimization.The private method successfully centralizes receipt construction and defers query execution until after the settings check (line 127). This is the core optimization that prevents loading unnecessary data.
The runtime type validation (lines 123-125) is defensive but redundant with TypeScript's compile-time type checking.
Consider removing the runtime type check for cleaner code:
- if (typeof getMessages !== 'function') { - throw new Error('First parameter must be a function that returns the messages to store read receipts for'); - } - if (settings.get('Message_Read_Receipt_Store_Users')) {
📜 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 (2)
apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts(6 hunks)packages/core-typings/src/ReadReceipt.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/core-typings/src/ReadReceipt.ts (1)
packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-239)
apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts (4)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)apps/meteor/app/lib/server/lib/notifyListener.ts (1)
notifyOnRoomChangedById(70-78)packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-239)packages/core-typings/src/ReadReceipt.ts (1)
ReadReceipt(5-16)
⏰ 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 (7)
packages/core-typings/src/ReadReceipt.ts (1)
5-16: LGTM! Good optimization by deferring user data loading.The type changes support the PR's goal of preventing unnecessary data loading. By removing the
userfield and adding message metadata fields (t,pinned,drid,tmid,token), read receipts now store only essential data. User information can be fetched on-demand viagetReceiptswhen needed.apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts (6)
12-21: LGTM! Improved type safety with room-aware debouncing.The refactored debouncing logic now uses strong typing with
IRoomand maintains per-room timeout management. The implementation correctly clears previous timeouts and cleans up after execution.
23-39: LGTM! Cleaner implementation with optional chaining.The refactored
updateMessagesfunction uses destructuring and optional chaining for better code clarity and safety.
42-63: Excellent optimization with callback-based message retrieval.The refactored method now uses a callback pattern to defer message loading until after the settings check in
storeReadReceipts. This prevents executing the potentially expensivefindVisibleUnreadMessagesByRoomAndDatequery whenMessage_Read_Receipt_Store_Usersis disabled, directly addressing the PR's goal.
65-94: LGTM! Consistent callback pattern applied.The method correctly applies the callback pattern for consistency, even when the message is already available. The optimization still applies as receipt construction is skipped when not needed.
96-115: LGTM! Optimization applied to thread messages.The callback pattern effectively defers the
findUnreadThreadMessagesByDatequery, providing the same optimization benefit for thread read receipts.
154-168: <!-- [scratchpad_start]
[task review_file_2/2 review_segment_7/7]
Verify export pattern compatibility - complete with web search context
[observations]
- Current export:
export const ReadReceipt = new ReadReceiptClass();- This exports a named singleton instance (const)
- All 4 consumer files import with:
import { ReadReceipt } from '...';- Named import pattern is correct for named exports
- Web search shows two patterns: classic singleton with getInstance() method OR simple instance export
[analysis]- The code uses the "simple instance export" pattern mentioned in web search results
- Export instance, not the class itself
- export default new xxxClass() is natively a singleton
- Importing with { } for a named export is CORRECT
- NOT a breaking change - consumers are already expecting this pattern
- Review comment claim about breaking change is INCORRECT
[pending]
None - sufficient verification complete
[done]
Export pattern is compatible with all imports. Review comment assertion about breaking change is incorrect.
[scratchpad_start] -->Remove the claim about breaking changes — the singleton export is compatible with all existing imports.
The exports are correct. The singleton pattern exports an instance, not the class itself. All four consumer files (
afterSaveMessage.ts,afterReadMessages.ts,getReadReceipts.ts,message-reads/service.ts) import using the named import syntax{ ReadReceipt }, which is the correct way to consume this pattern. There is no breaking change.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-7.12.0 #37280 +/- ##
===============================================
Coverage 67.69% 67.70%
===============================================
Files 3342 3342
Lines 114028 114027 -1
Branches 20679 20675 -4
===============================================
+ Hits 77192 77200 +8
+ Misses 34156 34150 -6
+ Partials 2680 2677 -3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| fn.call(this, roomId, ...args); | ||
| delete list[roomId]; | ||
| const list: Record<string, NodeJS.Timeout> = {}; | ||
| const debounceByRoomId = function (fn: (room: IRoom) => Promise<void>) { |
There was a problem hiding this comment.
Unrelated to your changes, but this debounce here may cause some inconsistencies regarding which messages are flagged as read or not when new messages keep coming in after the first debounced call.
There was a problem hiding this comment.
@pierre-lehnen-rc why do you think that? BTW the debounce was not receiving the expected data anyways
There was a problem hiding this comment.
I didn't look at the whole process to see how it works and whether this is an actual problem or not, but:
Messages.setVisibleMessagesAsRead will update all unread messages in a room at the time it is executed, which due to the debounce will be at least 2 seconds after the call to 'subscriptions.read'. During this 2 second window, new messages may have been added to the collection - those new messages will also be flagged as read by the process that was scheduled before they were sent.
pierre-lehnen-rc
left a comment
pierre-lehnen-rc
left a comment
There was a problem hiding this comment.
Checking the setting before calling storeReadReceipts would have been simpler, but nbd.
rodrigok
force-pushed
the
chore/improve-read-receipts
branch
from
aa5c6bc to
72b488a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/ee/server/models/raw/ReadReceipts.ts (1)
42-53: Do not bypass BaseRaw deletion — use this.deleteMany to preserve trash/audit.Calling this.col.deleteMany skips BaseRaw’s trash/hook behavior; other removals here use this.deleteMany. Align this one.
Apply this diff:
- return this.col.deleteMany({ + return this.deleteMany({ roomId, t: { $in: [ 'otr', otrSystemMessages.USER_JOINED_OTR, otrSystemMessages.USER_REQUESTED_OTR_KEY_REFRESH, otrSystemMessages.USER_KEY_REFRESHED_SUCCESSFULLY, ], }, ts: { $lte: until }, });Also consider a supporting index for this pattern to keep deletes fast at scale:
protected modelIndexes(): IndexDescription[] { - return [{ key: { roomId: 1, userId: 1, messageId: 1 }, unique: true }, { key: { messageId: 1 } }, { key: { userId: 1 } }]; + return [ + { key: { roomId: 1, userId: 1, messageId: 1 }, unique: true }, + { key: { messageId: 1 } }, + { key: { userId: 1 } }, + // speeds up removeOTRReceiptsUntilDate queries + { key: { roomId: 1, ts: 1 } }, + ]; }
♻️ Duplicate comments (3)
apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts (2)
12-19: Debounce caveat acknowledged; verify behavior under sustained message flow.Room-scoped debounce can drop intermediate updates if messages keep arriving within 2s. Validate with noisy rooms.
123-125: Drop the runtime typeof check; TS already guarantees the type.This adds no value and costs cycles.
- if (typeof getMessages !== 'function') { - throw new Error('First parameter must be a function that returns the messages to store read receipts for'); - }packages/core-typings/src/ReadReceipt.ts (1)
6-13: Mirror IMessage optionality to avoid “defined-but-undefined” properties.t/pinned/drid/tmid in IMessage are optional; making them required-but-possibly-undefined encourages persisting undefined fields.
Apply this diff:
export type ReadReceipt = { token?: string; messageId: IMessage['_id']; roomId: IRoom['_id']; ts: Date; - t: IMessage['t']; - pinned: IMessage['pinned']; - drid: IMessage['drid']; - tmid: IMessage['tmid']; + t?: IMessage['t']; + pinned?: IMessage['pinned']; + drid?: IMessage['drid']; + tmid?: IMessage['tmid']; userId: IUser['_id']; _id: string; };
🧹 Nitpick comments (7)
apps/meteor/client/views/room/modals/ReadReceiptsModal/ReadReceiptRow.tsx (1)
1-1: Safer fallbacks for display name and avatar.Pass undefined instead of {}/'' to avoid unexpected UI text or extra avatar requests.
-const ReadReceiptRow = ({ user, ts }: ReadReceipt & { user: Pick<IUser, '_id' | 'name' | 'username'> | null }): ReactElement => { - const displayName = useUserDisplayName(user || {}); +const ReadReceiptRow = ({ user, ts }: ReadReceipt & { user: Pick<IUser, '_id' | 'name' | 'username'> | null }): ReactElement => { + const displayName = useUserDisplayName(user ?? undefined); const formatDateAndTime = useFormatDateAndTime({ withSeconds: true }); return ( <Box role='listitem' display='flex' flexDirection='row' justifyContent='space-between' alignItems='center' mbe={8}> <Box> - <UserAvatar username={user?.username || ''} size='x24' /> + <UserAvatar username={user?.username} size='x24' /> <Box is='span' mis={8}>{displayName}</Box> </Box> <Box is='span' fontScale='c1' color='hint'>{formatDateAndTime(ts)}</Box> </Box> );Ensure UserAvatar accepts undefined username without network requests for a blank avatar.
Also applies to: 9-19
apps/meteor/ee/server/methods/getReadReceipts.ts (1)
1-1: Remove duplicated runtime type checks for messageId.messageId is checked in both the method wrapper and getReadReceiptsFunction. One is enough.
Meteor.methods<ServerMethods>({ async getReadReceipts({ messageId }) { - check(messageId, String); + check(messageId, String); const uid = Meteor.userId(); if (!uid) { throw new Meteor.Error('error-invalid-user', 'Invalid user', { method: 'getReadReceipts' }); } - return getReadReceiptsFunction(messageId, uid); + return getReadReceiptsFunction(messageId, uid); }, });Alternatively, keep the wrapper check and drop the one inside getReadReceiptsFunction since its signature is typed.
Also applies to: 14-16, 20-23
apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts (5)
42-63: Time comparison should probably be >= to skip redundant work.If userLastSeen equals room.lm, receipts are still attempted. Consider >= to avoid unnecessary queries.
- if (!room || (room.lm && userLastSeen > room.lm)) { + if (!room || (room.lm && userLastSeen >= room.lm)) { return; }Run a quick check on equality cases to ensure no regressions in badge/“read” state.
65-94: Type room.t precisely and guard extraData keys.Type t as IRoom['t'] (or the RoomType alias) and prevent extraData from overriding message-derived fields.
-async markMessageAsReadBySender(message: IMessage, { _id: roomId, t }: { _id: string; t: string }, userId: string) { +async markMessageAsReadBySender(message: IMessage, { _id: roomId, t }: { _id: string; t: IRoom['t'] }, userId: string) { … - const extraData = roomCoordinator.getRoomDirectives(t).getReadReceiptsExtraData(message); + const extraData = roomCoordinator.getRoomDirectives(t).getReadReceiptsExtraData(message); + // Only allow whitelisted extras (e.g., token) + const safeExtra: Pick<IReadReceipt, 'token'> = extraData && 'token' in extraData ? { token: (extraData as any).token } : {}; void this.storeReadReceipts( () => Promise.resolve([message]), roomId, userId, - extraData, + safeExtra, );
96-115: Same equality nuance for threads.Use >= to avoid work when userLastSeen equals tlm.
- if (!message || (message.tlm && userLastSeen > message.tlm)) { + if (!message || (message.tlm && userLastSeen >= message.tlm)) { return; }
146-151: Handle duplicates gracefully and keep bulk insert going.Unique index may trigger E11000; use unordered inserts and ignore duplicate errors.
- try { - await ReadReceipts.insertMany(receipts); - } catch (err) { - SystemLogger.error({ msg: 'Error inserting read receipts per user', err }); - } + try { + await ReadReceipts.insertMany(receipts, { ordered: false }); + } catch (err: any) { + // ignore duplicate key errors caused by concurrent inserts/upserts + if (err?.code !== 11000) { + SystemLogger.error({ msg: 'Error inserting read receipts per user', err }); + } + }
154-167: Avoid N+1 lookups when resolving users/visitors.Batch-fetch users and visitors by unique ids/tokens to reduce DB roundtrips; then map locally.
📜 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 (5)
apps/meteor/client/views/room/modals/ReadReceiptsModal/ReadReceiptRow.tsx(1 hunks)apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts(6 hunks)apps/meteor/ee/server/methods/getReadReceipts.ts(2 hunks)apps/meteor/ee/server/models/raw/ReadReceipts.ts(2 hunks)packages/core-typings/src/ReadReceipt.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
apps/meteor/client/views/room/modals/ReadReceiptsModal/ReadReceiptRow.tsx (2)
packages/core-typings/src/ReadReceipt.ts (1)
ReadReceipt(5-16)packages/core-typings/src/IUser.ts (1)
IUser(186-255)
apps/meteor/ee/server/methods/getReadReceipts.ts (2)
packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-239)packages/core-typings/src/IUser.ts (1)
IUser(186-255)
apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts (5)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)apps/meteor/app/lib/server/lib/notifyListener.ts (1)
notifyOnRoomChangedById(70-78)packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-239)packages/core-typings/src/IUser.ts (1)
IUser(186-255)packages/core-typings/src/ReadReceipt.ts (1)
ReadReceipt(5-16)
packages/core-typings/src/ReadReceipt.ts (1)
packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-239)
⏰ 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). (7)
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 📦 Meteor Build - coverage
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/models/src/models/Messages.ts (1)
1602-1616: Bug: “Visible” query omits _hidden filterfindVisibleUnreadMessagesByRoomAndDate returns hidden messages. Add _hidden: { $ne: true }.
findVisibleUnreadMessagesByRoomAndDate(rid: string, after: Date): FindCursor<Pick<IMessage, '_id' | 't' | 'pinned' | 'drid' | 'tmid'>> { const query = { rid, + _hidden: { $ne: true }, unread: true,
♻️ Duplicate comments (2)
apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts (2)
123-125: Remove unnecessary runtime typeof check.
getMessagesis already typed; the runtime guard adds no value here.- if (typeof getMessages !== 'function') { - throw new Error('First parameter must be a function that returns the messages to store read receipts for'); - }
129-141: Whitelist extra fields and avoid persisting undefined meta keys.Spreading
extraDatacan override core fields, and unconditionally copying optional IMessage fields (t,pinned,drid,tmid) can storeundefined. Build receipts with a base + conditional meta and only allowtokenfrom extras.- const receipts = (await getMessages()).map((message) => ({ - _id: Random.id(), - roomId, - userId, - messageId: message._id, - ts, - t: message.t, - pinned: message.pinned, - drid: message.drid, - tmid: message.tmid, - ...extraData, - })); + const receipts = (await getMessages()).map((message) => { + const base = { + _id: Random.id(), + roomId, + userId, + messageId: message._id, + ts, + }; + const meta = { + ...(message.t !== undefined && { t: message.t }), + ...(message.pinned !== undefined && { pinned: message.pinned }), + ...(message.drid !== undefined && { drid: message.drid }), + ...(message.tmid !== undefined && { tmid: message.tmid }), + }; + const extra = extraData && 'token' in extraData && extraData.token ? { token: extraData.token } : {}; + return { ...base, ...meta, ...extra }; + });If you adopt conditional meta, ensure the ReadReceipt type allows these fields to be absent or
undefinedaccordingly.
🧹 Nitpick comments (7)
packages/models/src/models/Messages.ts (2)
63-63: Index addition aligns with new queries; consider namingPartial index on { rid, unread, ts, tmid, tshow } fits room/thread updates. Optionally add a stable name to ease migrations.
- { key: { rid: 1, unread: 1, ts: 1, tmid: 1, tshow: 1 }, partialFilterExpression: { unread: { '$exists': true } } }, + { name: 'unread_by_room_ts_thread_tshow', + key: { rid: 1, unread: 1, ts: 1, tmid: 1, tshow: 1 }, + partialFilterExpression: { unread: { '$exists': true } } },
1629-1641: Optional: exclude hidden in thread-unread tooFor consistency and to avoid processing hidden entries, consider filtering out hidden here as well.
findUnreadThreadMessagesByDate( rid: string, tmid: string, userId: string, after: Date, ): FindCursor<Pick<IMessage, '_id' | 't' | 'pinned' | 'drid' | 'tmid'>> { const query = { rid, + _hidden: { $ne: true }, 'unread': true, 'u._id': { $ne: userId }, tmid, 'tshow': { $exists: false }, ...(after && { ts: { $gt: after } }), };apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts (5)
117-122: NarrowextraDatatype to prevent accidental overrides.Limit
extraDatato{ token }only.- private async storeReadReceipts( - getMessages: () => Promise<Pick<IMessage, '_id' | 't' | 'pinned' | 'drid' | 'tmid'>[]>, - roomId: string, - userId: string, - extraData: Partial<IReadReceipt> = {}, - ) { + private async storeReadReceipts( + getMessages: () => Promise<Pick<IMessage, '_id' | 't' | 'pinned' | 'drid' | 'tmid'>[]>, + roomId: string, + userId: string, + extraData: Pick<IReadReceipt, 'token'> = {}, + ) {
146-151: Make inserts unordered to reduce fallout from duplicate keys; log duplicates succinctly.
insertManywithordered: falseavoids aborting the entire batch on the first duplicate.- try { - await ReadReceipts.insertMany(receipts); + try { + await ReadReceipts.insertMany(receipts, { ordered: false }); } catch (err) { SystemLogger.error({ msg: 'Error inserting read receipts per user', err }); }Also verify a unique index exists on
(messageId, userId)(or equivalent) to prevent duplicates. See verification script below.
65-65: Tighten room type.Use
IRoom['t'](orPick<IRoom, '_id' | 't'>) instead ofstring.- async markMessageAsReadBySender(message: IMessage, { _id: roomId, t }: { _id: string; t: string }, userId: string) { + async markMessageAsReadBySender( + message: IMessage, + { _id: roomId, t }: Pick<IRoom, '_id' | 't'>, + userId: string, + ) {
55-57: Ensure minimal projection in message fetchers to meet PR goal.Verify
findVisibleUnreadMessagesByRoomAndDateandfindUnreadThreadMessagesByDatereturn only_id,t,pinned,drid,tmid(no heavy fields). If not, add projections.Also applies to: 109-111
12-21: Debounce semantics: trailing-only — is this intentional?Calls within 2s coalesce into one trailing execution per room. If you need a first-fast (leading) run plus a trailing flush, consider a configurable debounce or immediate-first pattern.
📜 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 (4)
apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts(6 hunks)apps/meteor/ee/server/local-services/message-reads/service.ts(1 hunks)packages/model-typings/src/models/IMessagesModel.ts(1 hunks)packages/models/src/models/Messages.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/model-typings/src/models/IMessagesModel.ts (2)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-239)
packages/models/src/models/Messages.ts (1)
packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-239)
apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts (5)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)apps/meteor/app/lib/server/lib/notifyListener.ts (1)
notifyOnRoomChangedById(70-78)packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-239)packages/core-typings/src/IUser.ts (1)
IUser(186-255)packages/core-typings/src/ReadReceipt.ts (1)
ReadReceipt(5-16)
⏰ 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 (6)
apps/meteor/ee/server/local-services/message-reads/service.ts (1)
45-48: LGTM: updated to new 3‑arg APIPassing rid, tmid, until is correct; broadcast payload matches until.
packages/models/src/models/Messages.ts (1)
1573-1581: Thread read update: query keys match indexUsing rid + unread + ts + tmid is correct and benefits from the new partial index.
If desired, confirm index usage in prod with explain before/after deploy.
Also applies to: 1576-1580
packages/model-typings/src/models/IMessagesModel.ts (2)
277-277: No remaining breaking changes: signature is implemented and no 2-arg calls found.
281-285: AllfindUnreadThreadMessagesByDatecalls include the newridparameter. No 3-argument invocations remain; you can resolve this change.apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts (2)
50-52: Confirm equality semantics for “already read” check.Using
>means whenuserLastSeen === room.lmwe’ll still process receipts. If equality should be considered “already read”, use>=.- if (!room || (room.lm && userLastSeen > room.lm)) { + if (!room || (room.lm && userLastSeen >= room.lm)) {
154-167: No action needed on indexing. The ReadReceipts model already declares a{ key: { messageId: 1 } }index and a unique composite index on(roomId, userId, messageId).
rodrigok
force-pushed
the
chore/improve-read-receipts
branch
from
e9f4ecf to
d5fe65f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts (1)
117-136: Whitelist extraData to avoid overriding core fields.Spreading extraData can overwrite messageId/roomId/userId/ts/meta. Limit to token (or a vetted allowlist).
- const receipts = (await getMessages()).map((message) => ({ - _id: Random.id(), - roomId, - userId, - messageId: message._id, - ts, - ...(message.t && { t: message.t }), - ...(message.pinned && { pinned: true }), - ...(message.drid && { drid: message.drid }), - ...(message.tmid && { tmid: message.tmid }), - ...extraData, - })); + const receipts = (await getMessages()).map((message) => { + const base = { + _id: Random.id(), + roomId, + userId, + messageId: message._id, + ts, + } as const; + const meta = { + ...(message.t !== undefined && { t: message.t }), + ...(message.pinned === true && { pinned: true }), + ...(message.drid !== undefined && { drid: message.drid }), + ...(message.tmid !== undefined && { tmid: message.tmid }), + }; + const extra: Pick<IReadReceipt, 'token'> = + extraData && 'token' in extraData && extraData.token ? { token: extraData.token } : {}; + return { ...base, ...meta, ...extra }; + });
🧹 Nitpick comments (3)
packages/models/src/models/Messages.ts (1)
63-63: Tighten partial index and consider key order for common query shapes.
- Use partialFilterExpression: { unread: true } to shrink index and match filters exactly.
- Consider key order: rid, unread, tmid, tshow, ts (put range field last) to improve scans for both room and thread queries.
- { key: { rid: 1, unread: 1, ts: 1, tmid: 1, tshow: 1 }, partialFilterExpression: { unread: { '$exists': true } } }, + { key: { rid: 1, unread: 1, tmid: 1, tshow: 1, ts: 1 }, partialFilterExpression: { unread: true } },apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts (1)
42-63: Happy path reads; small nit on exclusivity.Logic to skip when userLastSeen > room.lm is fine. Consider documenting that subsequent queries use ts < until semantics to avoid confusion on equality.
packages/model-typings/src/models/IMessagesModel.ts (1)
277-277: Document exclusive date semantics
- Verified all calls to
setThreadMessagesAsRead(rid, tmid, until)andfindUnreadThreadMessagesByDate(rid, tmid, userId, after)include the new parameters.- Add JSDoc to
setThreadMessagesAsReadindicating “until” is exclusive (ts < until).- Likewise, document “after” in
findUnreadThreadMessagesByDateas exclusive (ts > after).
📜 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 (8)
apps/meteor/client/views/room/modals/ReadReceiptsModal/ReadReceiptRow.tsx(1 hunks)apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts(6 hunks)apps/meteor/ee/server/local-services/message-reads/service.ts(1 hunks)apps/meteor/ee/server/methods/getReadReceipts.ts(2 hunks)apps/meteor/ee/server/models/raw/ReadReceipts.ts(2 hunks)packages/core-typings/src/ReadReceipt.ts(1 hunks)packages/model-typings/src/models/IMessagesModel.ts(1 hunks)packages/models/src/models/Messages.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/ee/server/methods/getReadReceipts.ts
- apps/meteor/ee/server/models/raw/ReadReceipts.ts
🧰 Additional context used
🧬 Code graph analysis (5)
packages/model-typings/src/models/IMessagesModel.ts (1)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)
packages/models/src/models/Messages.ts (1)
packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-239)
apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts (5)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)apps/meteor/app/lib/server/lib/notifyListener.ts (1)
notifyOnRoomChangedById(70-78)packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-239)packages/core-typings/src/IUser.ts (1)
IUser(186-255)packages/core-typings/src/ReadReceipt.ts (1)
ReadReceipt(5-16)
apps/meteor/client/views/room/modals/ReadReceiptsModal/ReadReceiptRow.tsx (2)
packages/core-typings/src/ReadReceipt.ts (1)
ReadReceipt(5-16)packages/core-typings/src/IUser.ts (1)
IUser(186-255)
packages/core-typings/src/ReadReceipt.ts (2)
packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-239)packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)
⏰ 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 (9)
apps/meteor/ee/server/local-services/message-reads/service.ts (1)
45-48: Signature update looks correct.Passing rid, tmid, until matches the new API and surrounding checks are intact.
apps/meteor/client/views/room/modals/ReadReceiptsModal/ReadReceiptRow.tsx (1)
1-10: UI typing matches server payload.Intersection with user metadata is correct; rendering handles nulls safely.
packages/models/src/models/Messages.ts (2)
1573-1587: Thread read update filter looks correct.Including rid and tmid with ts < until and unread true aligns with the new API.
1605-1615: Unread constraints added appropriately.
- Room-wide unread query now enforces unread: true.
- Thread unread query adds rid and excludes author’s own messages; tshow exclusion is appropriate.
Also applies to: 1629-1652
apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts (5)
12-21: Per-room debounce LGTM.Simple and safe; cleans up the timeout key post-run.
23-39: Room-level read update flow is sound.Guard on ls, broadcast gated by modifiedCount, and lm check are consistent.
65-94: Sender-alone fast path and receipt enqueue look good.setAsReadById with notify is appropriate; extraData flow continues below.
96-115: Thread receipts: correct rid usage and gating on tlm.findUnreadThreadMessagesByDate now includes rid; good alignment.
150-163: Receipt enrichment is fine.Token-based visitor lookup vs Users.findOneById works; projections are minimal and _id remains included by default.
rodrigok
force-pushed
the
chore/improve-read-receipts
branch
from
d5fe65f to
8f2460d
Compare
rodrigok
force-pushed
the
chore/improve-read-receipts
branch
from
8f2460d to
3b9926e
Compare
rodrigok
force-pushed
the
chore/improve-read-receipts
branch
from
e803cf4 to
7f7b016
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts (2)
65-65: Consider using Pick for the room descriptor type.The inline type
{ _id: string; t: string }could be replaced withPick<IRoom, '_id' | 't'>for better type safety and to ensure alignment with the actualIRoominterface.Apply this diff:
- async markMessageAsReadBySender(message: IMessage, { _id: roomId, t }: { _id: string; t: string }, userId: string) { + async markMessageAsReadBySender(message: IMessage, { _id: roomId, t }: Pick<IRoom, '_id' | 't'>, userId: string) {
117-148: RestrictextraDataparameter to whitelist safe fields.Current implementations of
getReadReceiptsExtraData()are safe (returning{}by default, or only{ token }for livechat), but thePartial<IReadReceipt>type signature allows any field override, including core ones like_id,roomId,userId, andmessageId. To follow defensive programming principles and prevent future implementations from accidentally introducing unsafe fields, restrict the parameter:private async storeReadReceipts( getMessages: () => Promise<Pick<IMessage, '_id' | 't' | 'pinned' | 'drid' | 'tmid'>[]>, roomId: string, userId: string, extraData: Pick<IReadReceipt, 'token'> = {}, ) {
📜 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 (16)
apps/meteor/client/views/room/modals/ReadReceiptsModal/ReadReceiptRow.tsx(1 hunks)apps/meteor/definition/IRoomTypeConfig.ts(2 hunks)apps/meteor/ee/server/api/chat.ts(2 hunks)apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts(6 hunks)apps/meteor/ee/server/local-services/message-reads/service.ts(1 hunks)apps/meteor/ee/server/methods/getReadReceipts.ts(2 hunks)apps/meteor/ee/server/models/raw/ReadReceipts.ts(4 hunks)apps/meteor/server/lib/rooms/roomCoordinator.ts(2 hunks)packages/core-typings/src/IReadReceipt.ts(1 hunks)packages/core-typings/src/ReadReceipt.ts(0 hunks)packages/core-typings/src/index.ts(1 hunks)packages/model-typings/src/models/IMessagesModel.ts(1 hunks)packages/model-typings/src/models/IReadReceiptsModel.ts(1 hunks)packages/models/src/dummy/ReadReceipts.ts(1 hunks)packages/models/src/models/Messages.ts(4 hunks)packages/rest-typings/src/v1/chat.ts(2 hunks)
💤 Files with no reviewable changes (1)
- packages/core-typings/src/ReadReceipt.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/ee/server/local-services/message-reads/service.ts
- apps/meteor/client/views/room/modals/ReadReceiptsModal/ReadReceiptRow.tsx
🧰 Additional context used
🧬 Code graph analysis (12)
apps/meteor/ee/server/api/chat.ts (1)
packages/core-typings/src/IReadReceipt.ts (1)
IReadReceiptWithUser(18-20)
packages/rest-typings/src/v1/chat.ts (1)
packages/core-typings/src/IReadReceipt.ts (1)
IReadReceiptWithUser(18-20)
apps/meteor/server/lib/rooms/roomCoordinator.ts (2)
packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-239)packages/core-typings/src/IReadReceipt.ts (1)
IReadReceipt(5-16)
packages/model-typings/src/models/IMessagesModel.ts (1)
packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-239)
packages/model-typings/src/models/IReadReceiptsModel.ts (1)
packages/core-typings/src/IReadReceipt.ts (1)
IReadReceipt(5-16)
packages/models/src/dummy/ReadReceipts.ts (2)
packages/core-typings/src/IReadReceipt.ts (1)
IReadReceipt(5-16)packages/model-typings/src/models/IReadReceiptsModel.ts (1)
IReadReceiptsModel(6-24)
apps/meteor/definition/IRoomTypeConfig.ts (2)
packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-239)packages/core-typings/src/IReadReceipt.ts (1)
IReadReceipt(5-16)
packages/core-typings/src/IReadReceipt.ts (3)
packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-239)packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)packages/core-typings/src/IUser.ts (1)
IUser(186-255)
apps/meteor/ee/server/methods/getReadReceipts.ts (1)
packages/core-typings/src/IReadReceipt.ts (1)
IReadReceiptWithUser(18-20)
packages/models/src/models/Messages.ts (1)
packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-239)
apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts (4)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)apps/meteor/app/lib/server/lib/notifyListener.ts (1)
notifyOnRoomChangedById(70-78)packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-239)packages/core-typings/src/IReadReceipt.ts (2)
IReadReceipt(5-16)IReadReceiptWithUser(18-20)
apps/meteor/ee/server/models/raw/ReadReceipts.ts (2)
packages/core-typings/src/IReadReceipt.ts (1)
IReadReceipt(5-16)packages/model-typings/src/models/IReadReceiptsModel.ts (1)
IReadReceiptsModel(6-24)
⏰ 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 (27)
packages/core-typings/src/IReadReceipt.ts (1)
1-20: LGTM! Clean type definitions for read receipts.The introduction of
IReadReceiptandIReadReceiptWithUsertypes is well-structured. The optional fields (t,pinned,drid,tmid) correctly mirrorIMessageoptionality, and the separation of user-enriched receipts viaIReadReceiptWithUserprovides a clean API surface for different consumption contexts.packages/model-typings/src/models/IMessagesModel.ts (2)
277-277: LGTM! Method signature updated to enable index optimization.Adding
ridas the first parameter allows the implementation to leverage the new compound index{ rid: 1, unread: 1, ts: 1, tmid: 1, tshow: 1 }for efficient thread message queries.
280-285: LGTM! Method signature updated for room-scoped thread queries.The addition of
ridas the first parameter aligns with the optimization goal, enabling queries to use the new compound index and reducing the scan scope for unread thread messages.packages/models/src/models/Messages.ts (3)
63-63: LGTM! Well-designed compound index for unread thread queries.The new index
{ rid: 1, unread: 1, ts: 1, tmid: 1, tshow: 1 }withpartialFilterExpression: { unread: { $exists: true } }is optimally structured for the thread read receipt queries. The field order (rid first) and partial filter expression will significantly reduce query load when fetching unread thread messages, directly supporting the PR's optimization objective.
1573-1587: LGTM! Query correctly leverages the new compound index.The updated implementation now filters by
rid,tmid,unread, andts, matching the new index structure. This enables efficient updates of thread messages as read without scanning the entire collection.
1628-1652: LGTM! Room-scoped thread query with proper user exclusion.The implementation correctly:
- Uses
ridto scope the query (line 1635)- Excludes the user's own messages with
{ $ne: userId }(line 1637)- Filters by
tmidand other thread-related criteriaThis aligns with the new index and ensures users don't fetch read receipts for their own messages.
packages/core-typings/src/index.ts (1)
68-68: LGTM! Export updated to reflect new type location.The re-export change from
'./ReadReceipt'to'./IReadReceipt'is consistent with the type refactoring across the codebase.apps/meteor/definition/IRoomTypeConfig.ts (2)
6-6: LGTM! Import updated to use new type.The import change from
ReadReceipttoIReadReceiptis consistent with the type refactoring.
109-109: LGTM! Return type updated consistently.The method signature correctly returns
Partial<IReadReceipt>instead ofPartial<ReadReceipt>, maintaining consistency with the new type definitions.packages/rest-typings/src/v1/chat.ts (2)
5-5: LGTM! Import updated to use enriched receipt type.Importing
IReadReceiptWithUserinstead ofReadReceiptis appropriate for this API context, where read receipts include user information.
1025-1025: LGTM! API return type correctly uses IReadReceiptWithUser.The endpoint return type
{ receipts: IReadReceiptWithUser[] }accurately reflects that the API response includes user information alongside the receipt data.apps/meteor/server/lib/rooms/roomCoordinator.ts (2)
2-2: LGTM! Import updated to use new type.The import change to
IReadReceiptis consistent with the type refactoring across the codebase.
59-59: LGTM! Method return type updated consistently.The return type
Partial<IReadReceipt>correctly reflects the new type definition.packages/models/src/dummy/ReadReceipts.ts (3)
1-1: LGTM! Import updated to use new type.The import change to
IReadReceiptis consistent with the type refactoring.
7-7: LGTM! Generic type parameter updated consistently.The class correctly extends
BaseDummy<IReadReceipt>with the new type.
12-12: LGTM! Return type updated consistently.The method return type
FindCursor<IReadReceipt>correctly uses the new type definition.packages/model-typings/src/models/IReadReceiptsModel.ts (1)
1-7: LGTM! Type migration is clean and consistent.The migration from
ReadReceipttoIReadReceiptaligns with the new core typings introduced in this PR. The interface correctly extendsIBaseModel<IReadReceipt>and all method signatures are properly updated.apps/meteor/ee/server/api/chat.ts (1)
1-1: LGTM! REST API typing correctly updated.The public API now returns
IReadReceiptWithUser[]which properly reflects that the response includes user information alongside the receipt data. This aligns with the updatedgetReadReceiptsFunctionsignature.Also applies to: 17-17
apps/meteor/ee/server/methods/getReadReceipts.ts (1)
1-1: LGTM! Method signatures properly aligned.The type updates are consistent across both the DDP method and the exported function, correctly using
IReadReceiptWithUserto indicate that user information is included in the returned receipts.Also applies to: 14-14, 18-18
apps/meteor/ee/server/models/raw/ReadReceipts.ts (3)
1-1: LGTM! Type migrations are complete and correct.The model properly migrates from
ReadReceipttoIReadReceiptacross the class definition, constructor, and all method signatures. The types align with the interface definition inIReadReceiptsModel.Also applies to: 8-9, 17-17, 64-64
21-39: LGTM! Deletion helpers are well-implemented.The new deletion methods (
removeByUserId,removeByRoomId,removeByRoomIds,removeByMessageId,removeByMessageIds) provide clean, consistent APIs for removing read receipts. These align with the interface and follow the existing patterns in the codebase.
42-53: LGTM! Simplified direct return.The refactoring to directly return
this.col.deleteMany(...)is cleaner and eliminates the intermediate query variable without changing functionality.apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts (5)
12-21: LGTM! Debounce refactoring improves type safety.The refactored
debounceByRoomIdnow operates onIRoomobjects instead of strings, providing better type safety and allowing the debounced function to access room metadata (likelm) without additional lookups.
23-39: LGTM! Optional chaining prevents potential runtime errors.The addition of optional chaining on Line 26 (
firstSubscription?.ls) safely handles cases wherefirstSubscriptionmight be undefined, preventing null pointer exceptions.
41-63: LGTM! Callback pattern achieves PR objective.The refactoring to use a callback (
() => Messages.findVisibleUnreadMessagesByRoomAndDate(...)) combined with the setting check instoreReadReceipts(line 123) ensures that messages are only loaded when detailed read receipts are being stored. This directly addresses the PR objective of preventing unnecessary data loading.
125-136: Receipt construction logic correctly handles optional fields.The conditional spreading logic properly handles optional message fields, only including them in the receipt when they have truthy values. Note that
pinnedis only set totruewhen the message is pinned (Line 132), which aligns with the optional nature of this field inIReadReceipt.
150-161: LGTM! Receipt enrichment with user data is correctly implemented.The
getReceiptsmethod properly enriches receipts with user information, handling both regular users and livechat visitors (via token), and returns the correctly typedIReadReceiptWithUser[].
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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)
apps/meteor/client/views/room/modals/ReadReceiptsModal/ReadReceiptRow.tsx(1 hunks)apps/meteor/definition/IRoomTypeConfig.ts(2 hunks)apps/meteor/ee/server/api/chat.ts(2 hunks)apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts(6 hunks)apps/meteor/ee/server/methods/getReadReceipts.ts(2 hunks)apps/meteor/ee/server/models/raw/ReadReceipts.ts(4 hunks)apps/meteor/server/lib/rooms/roomCoordinator.ts(2 hunks)packages/core-typings/src/IReadReceipt.ts(1 hunks)packages/core-typings/src/index.ts(1 hunks)packages/model-typings/src/models/IReadReceiptsModel.ts(1 hunks)packages/models/src/dummy/ReadReceipts.ts(1 hunks)packages/rest-typings/src/v1/chat.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/rest-typings/src/v1/chat.ts
- apps/meteor/definition/IRoomTypeConfig.ts
- apps/meteor/ee/server/methods/getReadReceipts.ts
- apps/meteor/client/views/room/modals/ReadReceiptsModal/ReadReceiptRow.tsx
- apps/meteor/ee/server/api/chat.ts
- packages/models/src/dummy/ReadReceipts.ts
- packages/core-typings/src/IReadReceipt.ts
🧰 Additional context used
🧬 Code graph analysis (4)
apps/meteor/server/lib/rooms/roomCoordinator.ts (2)
packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-239)packages/core-typings/src/IReadReceipt.ts (1)
IReadReceipt(5-16)
apps/meteor/ee/server/models/raw/ReadReceipts.ts (2)
packages/core-typings/src/IReadReceipt.ts (1)
IReadReceipt(5-16)packages/model-typings/src/models/IReadReceiptsModel.ts (1)
IReadReceiptsModel(6-24)
apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts (4)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)apps/meteor/app/lib/server/lib/notifyListener.ts (1)
notifyOnRoomChangedById(70-78)packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-239)packages/core-typings/src/IReadReceipt.ts (2)
IReadReceipt(5-16)IReadReceiptWithUser(18-20)
packages/model-typings/src/models/IReadReceiptsModel.ts (1)
packages/core-typings/src/IReadReceipt.ts (1)
IReadReceipt(5-16)
⏰ 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 (15)
packages/core-typings/src/index.ts (1)
68-68: LGTM! Export updated to use IReadReceipt.The export path correctly reflects the migration from
ReadReceipttoIReadReceipttypes, aligning with the broader type refactoring across the codebase.apps/meteor/server/lib/rooms/roomCoordinator.ts (1)
2-2: LGTM! Type migration applied correctly.The import and return type have been consistently updated from
ReadReceipttoIReadReceipt, maintaining the method contract while aligning with the new type system.Also applies to: 59-59
packages/model-typings/src/models/IReadReceiptsModel.ts (2)
1-1: LGTM! Type migration applied consistently.The imports and interface generics have been correctly updated to use
IReadReceiptthroughout, maintaining type safety across the model interface.Also applies to: 6-7
11-23: LGTM! New deletion and update methods added.The new methods provide comprehensive management capabilities for read receipts, including message-level deletion, OTR cleanup, and metadata updates. The signatures are consistent with MongoDB patterns and properly typed.
apps/meteor/ee/server/models/raw/ReadReceipts.ts (3)
1-1: LGTM! Type migration applied consistently throughout the model.All type references have been correctly updated from
ReadReceipttoIReadReceipt, including the base class generic, constructor parameters, return types, and filter types.Also applies to: 8-9, 17-17, 64-64
21-39: LGTM! Deletion helpers added for flexible cleanup.The new removal methods provide granular deletion capabilities by user, room, and message identifiers. Implementations correctly use MongoDB
deleteManywith appropriate query filters.
42-53: LGTM! Simplified to direct return.The refactoring eliminates the intermediate query variable and directly returns the
deleteManyoperation, making the code more concise without changing behavior.apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts (8)
12-21: LGTM! Debounce refactored to accept room objects.The debouncing logic now keys by
room._idand passes the full room object to the debounced function. This enables the called function to access additional room properties (likelm) without additional database queries.
23-39: LGTM! Improved type safety with optional chaining.The function now receives the room object directly, and Line 26 uses optional chaining for safe access to
firstSubscription.ls. Line 35 properly checks forlmexistence before comparison.
41-63: LGTM! Callback-based approach reduces unnecessary data loading.The refactor to use a getter callback (lines 54-60) aligns with the PR objective—the callback is only invoked when
Message_Read_Receipt_Store_Usersis enabled instoreReadReceipts, preventing unnecessary message queries when detailed receipts aren't being saved.
65-94: LGTM! Sender-triggered receipts now use callback pattern.The callback wrapper (lines 86-93) ensures the message is only processed when the setting is enabled, avoiding unnecessary work. The
extraDatafrom room directives is correctly passed through.
96-115: LGTM! Thread receipts refactored for lazy evaluation.The callback-based approach (lines 108-114) defers the thread message query until needed, consistent with the optimization strategy. Line 104 properly guards against unnecessary work when
message.tlmindicates the thread was already read.
150-161: LGTM! Receipt retrieval now returns enriched data.The method correctly returns
IReadReceiptWithUser[]by enriching each receipt with user information (Lines 156-158), handling both visitor tokens and regular user IDs appropriately.
164-164: LGTM! Class-based singleton provides better encapsulation.The export of a singleton instance created from
ReadReceiptClassimproves maintainability by encapsulating state and makingstoreReadReceiptsprivate, while maintaining the same public API surface.
131-134: No action required—the conditional spreading is correct.The
MessageTypesarray contains exclusively non-empty string values ('e2e', 'uj', 'ul', etc.), all of which are truthy. SinceMessageTypesValuesis derived from these values,message.tcannot be a falsy value that should be persisted. The conditional checkmessage.t && { t: message.t }correctly prevents including the property whentis absent (undefined), while safely preserving it when present.
| private async storeReadReceipts( | ||
| getMessages: () => Promise<Pick<IMessage, '_id' | 't' | 'pinned' | 'drid' | 'tmid'>[]>, | ||
| roomId: string, | ||
| userId: string, | ||
| extraData: Partial<IReadReceipt> = {}, | ||
| ) { |
There was a problem hiding this comment.
Constrain extraData to prevent core field override.
extraData is typed as Partial<IReadReceipt>, allowing callers to override critical fields like _id, roomId, userId, messageId, and ts at Line 135. This could lead to data corruption or inconsistencies.
Based on the codebase usage (Line 85 passes data from getReadReceiptsExtraData), only the token field should be allowed in extraData.
Apply this diff to restrict extraData to safe fields:
private async storeReadReceipts(
getMessages: () => Promise<Pick<IMessage, '_id' | 't' | 'pinned' | 'drid' | 'tmid'>[]>,
roomId: string,
userId: string,
- extraData: Partial<IReadReceipt> = {},
+ extraData: Pick<IReadReceipt, 'token'> | Record<string, never> = {},
) {Or more permissively, exclude core fields:
- extraData: Partial<IReadReceipt> = {},
+ extraData: Omit<Partial<IReadReceipt>, '_id' | 'roomId' | 'userId' | 'messageId' | 'ts'> = {},Also applies to: 135-135
🤖 Prompt for AI Agents
In apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts around lines
117 to 122 (and also affecting line 135), extraData is currently typed as
Partial<IReadReceipt> which allows callers to override core fields (_id, roomId,
userId, messageId, ts) and can corrupt data; change the type to only allow safe
fields (e.g., { token?: string }) or use Omit<IReadReceipt, '_id' | 'roomId' |
'userId' | 'messageId' | 'ts'> so callers cannot supply or override those core
fields, and update any related call sites if necessary to satisfy the new
narrower type.
7f7b016 to
98b1171
Compare
ggazzo
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
|
/backport 7.11.2 |
…aving detailed info (#37280)
|
Pull request #37554 added to Project: "Patch 7.11.2" |
|
/backport 7.10.5 |
…aving detailed info (#37280)
|
Pull request #37555 added to Project: "Patch 7.10.5" |
5 participants
https://rocketchat.atlassian.net/browse/CORE-1506
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Refactor
UI
Performance