fix: dynamically calculate unread count in /subscriptions.get for iframe integration#37159
fix: dynamically calculate unread count in /subscriptions.get for iframe integration#37159JatinMehta007 wants to merge 4 commits into
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
|
WalkthroughThe subscriptions.get handler now strictly validates an optional updatedSince date, computes per-subscription unread counts only for array results (attaching an Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant API as subscriptions.get (Server)
participant DB as Data Layer
C->>API: GET /api/v1/subscriptions.get?updatedSince=...
API->>API: Validate `updatedSince` (strict)
alt invalid date
API-->>C: Meteor.error (code: error-invalid-date)
else valid or absent
API->>DB: Query subscriptions (user, optional updatedSince)
DB-->>API: Subscriptions result (array or non-array)
alt result is array
loop parallel mapping (Promise.all)
API->>DB: Compute unread count per subscription
DB-->>API: unread number
end
API-->>C: { update: [subscriptions with `unread`], remove: [] }
else non-array result
API->>DB: (optional) compute unread
DB-->>API: unread number
API-->>C: return payload directly
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ 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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/app/api/server/v1/subscriptions.ts (1)
38-48: Introduce batch unread count API for scalability.The current
countByRoomIdAndUserIdcall issues one DB query per subscription; for users with many subscriptions this may degrade performance. Consider adding a bulk method (e.g.,countByRoomIdsAndUserId) to fetch counts for multiple rooms in a single operation.
📜 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/app/api/server/v1/subscriptions.ts(1 hunks)
🔇 Additional comments (1)
apps/meteor/app/api/server/v1/subscriptions.ts (1)
50-57: LGTM! Response structure preserved correctly.The return statement correctly maintains the API response structure
{ update: [...], remove: [] }while enriching subscriptions with the unread count, ensuring backward compatibility.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/app/api/server/v1/subscriptions.ts (1)
50-57: Eliminate redundant array check.The condition
Array.isArray(result)is evaluated twice (lines 38 and 51). Since you've already determined the result type when computingsubscriptionsWithUnread, you can simplify this logic.Apply this diff to remove the redundant check:
- const subscriptionsWithUnread = Array.isArray(result) - ? await Promise.all( - result.map(async (sub) => { - const unreadCount = await unreadMessages.countByRoomIdAndUserId(sub.rid, this.userId); - return { - ...sub, - unread: unreadCount, - }; - }) - ) - : result; + const isArrayResult = Array.isArray(result); + const subscriptionsWithUnread = isArrayResult + ? await Promise.all( + result.map(async (sub) => { + const unreadCount = await unreadMessages.countByRoomIdAndUserId(sub.rid, this.userId); + return { + ...sub, + unread: unreadCount, + }; + }) + ) + : result; return API.v1.success( - Array.isArray(result) + isArrayResult ? { update: subscriptionsWithUnread, remove: [], } : subscriptionsWithUnread );
📜 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/app/api/server/v1/subscriptions.ts(1 hunks)
🔇 Additional comments (2)
apps/meteor/app/api/server/v1/subscriptions.ts (2)
25-34: Date validation looks good.The explicit validation and error handling for the
updatedSinceparameter is appropriate, and the error message correctly references the parameter name.
38-48: Now let me search the codebase directly for the subscriptions.ts file to examine the actual context:N+1 query pattern confirmed - significant performance concern for high-volume subscriptions.
After thorough investigation, there is no easy way to retrieve unread counts for multiple rooms without creating multiple requests. The code at lines 38–48 calls
unreadMessages.countByRoomIdAndUserId()once per subscription in the array, creating as many database queries as there are subscriptions. For users with dozens or hundreds of subscriptions, this creates corresponding database load.Based on codebase analysis:
- No batch-capable method exists for counting unread messages across multiple rooms
- This is a known limitation acknowledged in the project's issue history
- Alternative approaches to consider:
- Implement a batch method in the
unreadMessagesservice that accepts multiple room IDs- Use aggregation pipeline to compute all unread counts in a single query
- Pre-fetch and cache unread counts at a higher level before iterating subscriptions
2 participants
Closes : #23977
Description :
In /api/v1/subscriptions.get, the unread count was always 0 in iframe integrations, even if the channel had unread messages.
Solution :
How Lumyst helped :
I used Lumyst to explore the Rocket.Chat codebase, identify the flow of subscription data, and understand how unread counts are calculated. It helped trace the path from API endpoint → getSubscriptions → Subscriptions collection → unreadMessages logic.
Suggestions for Lumyst :
Summary by CodeRabbit
New Features
Refactor
Bug Fixes