chore: improve the way we send DDP connection data to hooks #38268
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 |
|
Contributor
WalkthroughThis PR refactors the account session and device login event system by replacing connection-object-based payloads with structured, explicit payload types. Login and logout handlers now emit events with computed metadata (userAgent, clientAddress, instanceId, loginToken) as separate payload fields, and consumers adapt to these new payload shapes while maintaining existing functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant Conn as Connection Event
participant Hook as sauMonitorHooks
participant Emitter as Event Emitters
participant SAUMon as SAUMonitor
participant DevMgmt as Device Management
Conn->>Hook: accounts.login fired
Hook->>Hook: Extract userId, connection
Hook->>Hook: Compute instanceId, userAgent,<br/>clientAddress, host, loginToken
Hook->>Hook: Construct LoginSessionPayload
Hook->>Emitter: emit sau.accounts.login<br/>(LoginSessionPayload)
Hook->>Emitter: emit device-login<br/>(DeviceLoginPayload)
Emitter->>SAUMon: sau.accounts.login received
SAUMon->>SAUMon: _handleSession with payload fields
SAUMon->>SAUMon: Call _getUserAgentInfo(userAgent)
SAUMon->>SAUMon: Build ISession with computed data
Emitter->>DevMgmt: device-login received
DevMgmt->>DevMgmt: Destructure userAgent,<br/>clientAddress, loginToken
DevMgmt->>DevMgmt: Parse userAgent, send notification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
nazabucciarelli
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38268 +/- ##
===========================================
+ Coverage 70.70% 70.76% +0.05%
===========================================
Files 3139 3142 +3
Lines 108744 108920 +176
Branches 19560 19663 +103
===========================================
+ Hits 76887 77072 +185
+ Misses 29856 29841 -15
- Partials 2001 2007 +6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
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/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 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 21:03", "12/22 18:54", "12/23 16:16", "12/24 19:38", "12/25 17:51", "12/26 13:18", "12/29 19:01", "12/30 20:52", "01/21 22:20 (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, 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.00]
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, 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.00]
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, 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.00]
line "omnichannel-transcript-service" [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, 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, 0.13, 0.13, 0.00]
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, 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.00]
line "queue-worker-service" [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, 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, 0.13, 0.13, 0.00]
line "rocketchat" [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.34, 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.34, 0.34, 0.34, 0.00]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Wed, 21 Jan 2026 22:20:18 GMT |
nazabucciarelli
force-pushed
the
refactor/ddp-headers
branch
from
77d91f0 to
a4f7f61
Compare
nazabucciarelli
force-pushed
the
refactor/ddp-headers
branch
from
a4f7f61 to
0b0a9a2
Compare
nazabucciarelli
changed the title
nazabucciarelli
changed the title
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![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: 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/server/hooks/sauMonitorHooks.ts (1)
3-50: Replace empty-string loginToken withundefinedto prevent logout failures.When
resumeis missing,loginTokendefaults to'', which gets stored in the Sessions collection during login. During logout, SAUMonitor checksif (!session?.loginToken)(line 148 of SAUMonitor.ts), and the falsy empty string triggers a logout error. SinceLoginSessionPayloadmarksloginTokenas optional, useundefinedinstead.🛠️ Suggested adjustment
- const loginToken = resume ? Accounts._hashLoginToken(resume) : ''; + const loginToken = resume ? Accounts._hashLoginToken(resume) : undefined;
🤖 Fix all issues with AI agents
In `@apps/meteor/server/services/sauMonitor/events.ts`:
- Around line 1-8: The SAU event keys were renamed to namespaced keys but
SAUMonitorClass._handleOnConnection still subscribes to the old
'socket.disconnected' event; update that listener to use the new
'sau.socket.disconnected' key so it matches the sauEvents emitter (reference the
sauEvents Emitter and the _handleOnConnection method in SAUMonitorClass),
ensuring the handler remains registered and sessions are closed correctly.
Comment on lines
+1
to
+8
| import type { ISocketConnection, LoginSessionPayload, LogoutSessionPayload } from '@rocket.chat/core-typings'; | ||
| import { Emitter } from '@rocket.chat/emitter'; | ||
|
|
||
| export const sauEvents = new Emitter<{ | ||
| 'accounts.login': { userId: string; connection: ISocketConnectionLogged }; | ||
| 'accounts.logout': { userId: string; connection: ISocketConnection }; | ||
| 'socket.connected': ISocketConnection; | ||
| 'socket.disconnected': ISocketConnection; | ||
| 'sau.accounts.login': LoginSessionPayload; | ||
| 'sau.accounts.logout': LogoutSessionPayload; | ||
| 'sau.socket.connected': ISocketConnection; | ||
| 'sau.socket.disconnected': ISocketConnection; |
Contributor
There was a problem hiding this comment.
Update remaining listeners to sau.socket.disconnected.
Event keys changed here, but SAUMonitorClass._handleOnConnection still listens to socket.disconnected (Line 99 in apps/meteor/app/statistics/server/lib/SAUMonitor.ts). That listener will stop firing, leaving sessions open.
🔧 Suggested fix
- sauEvents.on('socket.disconnected', async ({ id, instanceId }) => {
+ sauEvents.on('sau.socket.disconnected', async ({ id, instanceId }) => {🤖 Prompt for AI Agents
In `@apps/meteor/server/services/sauMonitor/events.ts` around lines 1 - 8, The SAU
event keys were renamed to namespaced keys but
SAUMonitorClass._handleOnConnection still subscribes to the old
'socket.disconnected' event; update that listener to use the new
'sau.socket.disconnected' key so it matches the sauEvents emitter (reference the
sauEvents Emitter and the _handleOnConnection method in SAUMonitorClass),
ensuring the handler remains registered and sessions are closed correctly.
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.
Proposed changes (including videos or screenshots)
After some investigation, context gathering, and a discussion with Diego Sampaio, we aligned on the scope of this task.
The goal is to improve how DDP connection headers are handled in sauMonitorHooks.ts.
While the broader issue involves multiple places where the full connection object is passed downstream instead of only the required data, addressing all of those cases is outside the scope of this task.
To make the change safer and more explicit, I will introduce dedicated types for the event payloads instead of passing the entire connection object.
Issue(s)
Steps to test or reproduce
Further comments
Doubt:
Are the new types (DeviceLoginPayload, LoginSessionPayload, LogoutSessionPayload) and the util function (getHeader) properly located in the directory tree?
Summary by CodeRabbit
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.