refactor: improve the way we send DDP connection data to hooks #38294
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 |
|
WalkthroughServer event flow switched to strongly-typed, sau-prefixed events carrying explicit payload fields (userAgent, loginToken, clientAddress, instanceId, connectionId, host). SAUMonitor and device-management consume these payloads, parse UA strings from payloads, and a new getHeader utility exposes safe header extraction. Changes
Sequence Diagram(s)sequenceDiagram
participant Hooks as Hooks (sauMonitorHooks)
participant SauService as Sau Service (sau)
participant SAUMonitor as SAUMonitor
participant DeviceMgmt as Device Management
Hooks->>SauService: emit sau.accounts.login {userId, instanceId, connectionId, loginToken?, clientAddress, userAgent, host}
SauService->>SAUMonitor: forward sau.accounts.login payload
SAUMonitor->>SAUMonitor: _getUserAgentInfo(userAgent) -> device info
SAUMonitor->>SAUMonitor: _handleSession({userId,...}, params) -> persist/update session
Hooks->>SauService: emit sau.socket.disconnected {instanceId, connectionId}
SauService->>SAUMonitor: forward sau.socket.disconnected
SAUMonitor->>SAUMonitor: finish sessions for connectionId
SauService->>DeviceMgmt: emit device-login {userId, userAgent, clientAddress}
DeviceMgmt->>DeviceMgmt: parse UA, send notifications/emails
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist. 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 review |
✅ Actions performedReview triggered.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/app/statistics/server/lib/SAUMonitor.ts (1)
135-142: Update logout log messages to the new event name.The warning strings still mention
'accounts.logout', which no longer matches the hook name and can mislead debugging.🔧 Suggested fix
- if (!userId) { - logger.warn({ msg: "Received 'accounts.logout' event without 'userId'" }); + if (!userId) { + logger.warn({ msg: "Received 'sau.accounts.logout' event without 'userId'" }); return; } if (!sessionId) { - logger.warn({ msg: "Received 'accounts.logout' event without 'sessionId'" }); + logger.warn({ msg: "Received 'sau.accounts.logout' event without 'sessionId'" }); return; }
🤖 Fix all issues with AI agents
In `@packages/core-typings/src/sau/SocketConnectedPayload.ts`:
- Around line 1-3: Remove the inline comment inside the SocketConnectedPayload
type and replace the placeholder with an explicit empty payload type;
specifically, update export type SocketConnectedPayload to an explicit empty
shape (e.g., use Record<string, never> or an empty object type) so the intent is
encoded in the type rather than a comment and the inline comment is removed.
🧹 Nitpick comments (3)
apps/meteor/server/services/sauMonitor/service.ts (1)
35-38: Remove commented-out debug statement.Per coding guidelines, avoid code comments in the implementation. This commented-out console.log should be removed.
Suggested fix
this.onEvent('socket.connected', async (data) => { - // console.log('socket.connected', data); sauEvents.emit('sau.socket.connected', data); });apps/meteor/server/services/device-management/service.ts (1)
14-19: Address TODO: Add loginToken to payload.The TODO indicates
loginTokenshould be added to the device-login payload. TheDeviceLoginPayloadtype already supports an optionalloginTokenfield (as seen insession.tsusage at line 42).Would you like me to help implement the
loginTokenextraction similar to how it's done insauMonitorHooks.ts(usingAccounts._hashLoginToken(resume))? Or should I open an issue to track this task?apps/meteor/server/hooks/sauMonitorHooks.ts (1)
80-86: Remove inline comment and clarifySocketConnectedPayloadintent.The comment on line 82 should be removed per coding guidelines. Additionally, spreading the entire
connectionobject intoSocketConnectedPayloadappears to contradict the PR's goal of avoiding passing full connection objects downstream. Consider extracting only the required fields.Suggested fix
If only specific fields are needed, extract them explicitly:
Meteor.onConnection((connection) => { const socketConnectedEventObject: SocketConnectedPayload = { - // Implement SocketConnectedPayload type in case of using the sau.socket.connected hook - ...connection, + connectionId: connection.id, + // Add other required fields as needed }; sauEvents.emit('sau.socket.connected', socketConnectedEventObject); });If the full connection is intentionally needed here, the comment should at minimum be removed.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38294 +/- ##
===========================================
+ Coverage 70.35% 70.39% +0.03%
===========================================
Files 3162 3163 +1
Lines 110705 110719 +14
Branches 19923 19891 -32
===========================================
+ Hits 77892 77945 +53
+ Misses 30788 30746 -42
- Partials 2025 2028 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
📦 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", "02/10 19:07 (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: Tue, 10 Feb 2026 19:07:48 GMT |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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)
apps/meteor/app/statistics/server/lib/SAUMonitor.ts (1)
235-245: Replace the inline TODO block with an explicit UA result type.The comment block violates the "no implementation comments" guideline. Move the type information into code as a type alias instead.
However, the proposed solution using
ReturnType<typeof UAParserMobile.uaObject>won't work as expected sinceUAParserCustom.jsis untyped JavaScript. Define the union type explicitly instead:♻️ Suggested approach
+ type UAResult = + | UAParser.IResult + | { device?: { type: string }; app?: { name: string; version: string }; browser?: undefined; os?: undefined } + | { device?: { type: string }; os?: Record<string, string>; app?: { name: string; version: string }; browser?: undefined }; + - const result = ((): any => { + const result = ((): UAResult => {The types correspond to the three code branches: standard UAParser, mobile app, and desktop app respectively.
♻️ Duplicate comments (1)
packages/core-typings/src/sau/SocketConnectedPayload.ts (1)
1-2: Remove inline comment; encode empty payload in the type.Line 1 violates the “no implementation comments” guideline; encode the empty payload via the type instead. As per coding guidelines, avoid code comments in implementation.
♻️ Proposed change
-export type SocketConnectedPayload = {}; +export type SocketConnectedPayload = Record<string, never>;Is `Record<string, never>` the recommended TypeScript pattern for expressing an empty object payload?
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
✅ Actions performedReview triggered.
|
Co-authored-by: Kevin Aleman <kaleman960@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/meteor/server/hooks/sauMonitorHooks.ts`:
- Line 22: Replace the private Meteor call Accounts._hashLoginToken with the
public hashLoginToken from `@rocket.chat/account-utils`: import { hashLoginToken }
and change the expression that computes loginToken (currently using
Accounts._hashLoginToken(resume)) to use hashLoginToken(resume) (preserving the
ternary behavior with resume). Update any existing imports to add hashLoginToken
and remove reliance on Accounts._hashLoginToken so the code computes loginToken
via the public API.
- Line 38: Remove the duplicate device-login emission in sauMonitorHooks by
deleting the deviceManagementEvents.emit('device-login', { userId:
info.user._id, userAgent, loginToken, clientAddress }) call, and instead update
DeviceManagementService (the service that listens to accounts.login and emits
'device-login') to include the loginToken in its emitted payload so the single
emission covers the session handler in
apps/meteor/ee/server/lib/deviceManagement/session.ts; ensure the emitted object
from DeviceManagementService contains userId, userAgent, clientAddress and
loginToken to match the original payload.
🧹 Nitpick comments (1)
packages/tools/src/getHeader.ts (1)
3-3: Consider exportingHeaderLiketype for consumers.Consumers of
getHeadermight benefit from having access to theHeaderLiketype alias for their own type annotations. This is optional since the function's parameter typing provides guidance.♻️ Optional change
-type HeaderLike = IncomingHttpHeaders | Record<string, string | string[] | undefined>; +export type HeaderLike = IncomingHttpHeaders | Record<string, string | string[] | undefined>;
… into chore/ddp-headers
nazabucciarelli
force-pushed
the
chore/ddp-headers
branch
from
2f1df1c to
2a47751
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/tools/src/getHeader.ts">
<violation number="1" location="packages/tools/src/getHeader.ts:5">
P1: `getHeader` now uses a generic return type with `as T` casts, but it does not guarantee the runtime value matches `T` (e.g., `T=string[]` still returns a string for `Headers`, and the default mode can now return `string[]` for Node headers). This is both a type-safety issue and a potential runtime regression for callers that expect a string.</violation>
</file>
<file name="apps/meteor/server/hooks/sauMonitorHooks.ts">
<violation number="1" location="apps/meteor/server/hooks/sauMonitorHooks.ts:24">
P2: `clientAddress` is a required `string`, so it may be `''` when unknown. Using `??` won’t fall back to `x-real-ip` in that case (behavior change from the previous `getClientAddress` helper which used `||`). Use `||` to preserve the fallback when `clientAddress` is an empty string.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/meteor/server/hooks/sauMonitorHooks.ts`:
- Line 38: The same 'device-login' event is emitted twice (once from
DeviceManagementService reacting to the DDPStreamer accounts.login broadcast and
once from sauMonitorHooks.ts via Accounts.onLogin), causing duplicate handling;
remove the redundant emission in sauMonitorHooks.ts (the
deviceManagementEvents.emit('device-login', ...) call) or add a guard there so
it only emits when DeviceManagementService/EE path is not active (e.g., detect
presence of the DDPStreamer/DeviceManagementService or an EE flag) to ensure a
single emission per login.
🧹 Nitpick comments (1)
packages/tools/src/getHeader.ts (1)
5-15: Type assertions may return unexpected types whenTisstring[].The generic
Tdefaults tostring, but when a caller specifiesT = string[], the function can still return an empty string''(lines 7, 11) or a single string value (line 14) instead of an array. The test suite documents this as "by design" (line 40-42 in spec), so if this is intentional for simplicity, consider adding a brief JSDoc comment to clarify the contract for callers.📝 Suggested documentation
+/** + * Retrieves a header value from various header-like objects. + * Note: When T is string[], the actual return type depends on the header value; + * callers should verify the type if expecting an array. + */ export const getHeader = <T extends string | string[] = string>(headers: HeaderLike, key: string): T => {
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/meteor/server/hooks/sauMonitorHooks.ts`:
- Around line 56-58: Remove the inline implementation comment before the
sauEvents.emit call and instead document the expected event parameter types in
the events type definition file (services/sauMonitor/events.ts); update the type
declaration or add a comment in that file describing the parameters for the
'sau.socket.connected' event so the sauEvents.emit({ instanceId:
InstanceStatus.id(), connectionId: connection.id }) call remains clean and
implementation-only.
🧹 Nitpick comments (1)
packages/tools/src/getHeader.spec.ts (1)
7-8: WidengetHeaderinput type to avoid unsafe casts.Line 8 relies on
undefined as unknown as IncomingHttpHeaders, which bypasses type safety. Consider lettinggetHeaderacceptheaders?: HeaderLikeand call it withundefineddirectly to keep typings accurate.♻️ Suggested updates
- export const getHeader = <T extends string | string[] = string>(headers: HeaderLike, key: string): T => { + export const getHeader = <T extends string | string[] = string>(headers?: HeaderLike, key: string): T => { if (!headers) { return '' as T; }- expect(getHeader(undefined as unknown as IncomingHttpHeaders, 'origin')).toBe(''); + expect(getHeader(undefined, 'origin')).toBe('');As per coding guidelines, ensure accurate typing in tests.
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/meteor/server/services/device-management/service.ts">
<violation number="1" location="apps/meteor/server/services/device-management/service.ts:14">
P2: This guard suppresses `device-login` emissions whenever `connection.loginToken` is set. DDP-streamer always sets `connection.loginToken` on login, so device login emails will no longer fire for those logins. Emit the event unconditionally (as before) unless you have a reliable resume-only signal.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
nazabucciarelli
changed the title
dougfabris
changed the title
4 participants
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.
Changes:
Keeping the object construction at the call site improves traceability of fields (e.g. loginToken, instanceId, httpHeaders) and reduces unnecessary indirection.
Related to #38268
Issue(s)
CORE-1529 - Improve DDP header handling
Steps to test or reproduce
Further comments
Please keep in mind that this PR touches sensitive code, such as sessions and login/logout.
Summary by CodeRabbit
Refactor
Bug Fixes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.