chore: improve the way we send DDP connection data to hooks #38268 #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 |
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request refactors event payload structures across SAU monitoring and device management systems by replacing connection object passing with explicit typed fields (userId, instanceId, userAgent, loginToken, connectionId, clientAddress, host). It renames generic event names to SAU-prefixed variants and introduces a new header retrieval utility function. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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 |
|
@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.73% 70.73% -0.01%
===========================================
Files 3158 3160 +2
Lines 109359 109391 +32
Branches 19695 19645 -50
===========================================
+ Hits 77358 77380 +22
- Misses 29966 29976 +10
Partials 2035 2035
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", "01/23 22:27 (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: Fri, 23 Jan 2026 22:27:12 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?
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
@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)
130-157: Potential logout miss whenloginTokenis absent.
LoginSessionPayload.loginTokenandISession.loginTokenare optional, but this path treats a missing token as “session not found” and returns. That can leave sessions open and skew SAU. Consider enforcing a non-optional token in the payload or adding a fallback to close bysessionIdwhen the token is missing.🔧 Suggested fallback when `loginToken` is missing
- const session = await Sessions.getLoggedInByUserIdAndSessionId<Pick<ISession, 'loginToken'>>(userId, sessionId, { - projection: { loginToken: 1 }, - }); - if (!session?.loginToken) { + const session = await Sessions.getLoggedInByUserIdAndSessionId<Pick<ISession, 'loginToken' | 'instanceId'>>(userId, sessionId, { + projection: { loginToken: 1, instanceId: 1 }, + }); + if (!session) { if (!isProdEnv) { throw new Error('Session not found during logout'); } logger.error({ msg: 'Session not found during logout', userId, sessionId }); return; } + if (!session.loginToken) { + await Sessions.closeByInstanceIdAndSessionId(session.instanceId, sessionId); + return; + } await Sessions.logoutBySessionIdAndUserId({ loginToken: session.loginToken, userId });
|
@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/services/sauMonitor/events.ts`:
- Around line 4-12: The event type for 'sau.accounts.login' is inconsistent with
ISocketConnection because connection.loginToken is optional; update the event
definition in events.ts to make loginToken optional (change loginToken: string
to loginToken?: string) so it matches ISocketConnection and avoids the type
contract violation, then recompile/typecheck; alternatively, if you prefer
forcing a string, update the caller in service.ts where connection.loginToken is
passed to provide a default (e.g., connection.loginToken ?? '')—pick one
approach and apply it consistently across sau.accounts.login, service.ts, and
references in sauMonitorHooks.ts.
🧹 Nitpick comments (3)
apps/meteor/server/services/sauMonitor/service.ts (1)
42-45: Remove commented-out debug code.The commented
console.logstatement should be removed to keep the codebase clean.♻️ Proposed fix
this.onEvent('socket.connected', async (data) => { - // console.log('socket.connected', data); sauEvents.emit('sau.socket.connected', { instanceId: InstanceStatus.id(), connectionId: data.id }); });apps/meteor/server/hooks/sauMonitorHooks.ts (1)
49-58: Consider consolidating the twoMeteor.onConnectionhandlers.Both handlers register on the same connection event. Merging them would reduce overhead and improve code organization.
♻️ Proposed consolidation
Meteor.onConnection((connection) => { + // in case of implementing a listener of this event, define the parameters type in services/sauMonitor/events.ts + sauEvents.emit('sau.socket.connected', { instanceId: InstanceStatus.id(), connectionId: connection.id }); + connection.onClose(async () => { sauEvents.emit('sau.socket.disconnected', { connectionId: connection.id, instanceId: InstanceStatus.id() }); }); }); - -Meteor.onConnection((connection) => { - // in case of implementing a listener of this event, define the parameters type in services/sauMonitor/events.ts - sauEvents.emit('sau.socket.connected', { instanceId: InstanceStatus.id(), connectionId: connection.id }); -});apps/meteor/app/statistics/server/lib/SAUMonitor.ts (1)
238-307: Return type includesundefinedbut function always returns a value.The function always returns
{ device: info }(lines 304-306), but the return type is{ device: ISessionDevice } | undefined. This could lead to unnecessary null checks by callers.♻️ Proposed fix
- private _getUserAgentInfo(uaString: string): { device: ISessionDevice } | undefined { + private _getUserAgentInfo(uaString: string): { device: ISessionDevice } {
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:
Issue(s)
CORE-1529 - Improve DDP header handling
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
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.