regression: fix presence connections not being updated in DDPStreamer#37855
regression: fix presence connections not being updated in DDPStreamer#37855kodiakhq[bot] merged 5 commits intodevelopfrom
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughReplaces heartbeat/_seenPacket-based presence coordination with a throttled presence-update approach: incoming messages now invoke a throttled updater that calls the presence update when a userId exists; immediate presence-on-heartbeat and packet-tracking fields were removed, and presence-update errors are logged. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ 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.
|
📦 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/15 22:28", "11/16 01:28", "11/17 23:50", "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 01:40", "12/18 17: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]
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]
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]
line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 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]
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]
line "queue-worker-service" [0.14, 0.14, 0.14, 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]
line "rocketchat" [0.36, 0.36, 0.35, 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.35]
line "stream-hub-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]
Statistics (last 24 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Thu, 18 Dec 2025 17:20:11 GMT |
cardoso
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37855 +/- ##
========================================
Coverage 67.75% 67.75%
========================================
Files 3463 3463
Lines 113702 113702
Branches 20901 20901
========================================
+ Hits 77034 77038 +4
+ Misses 34501 34494 -7
- Partials 2167 2170 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
sampaiodiego
left a comment
sampaiodiego
left a comment
There was a problem hiding this comment.
instead of all this sheninagans with intervals and flags all around, pls change the implementation to a throttle to call Presence.updateConnection
![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)
ee/apps/ddp-streamer/src/Client.ts (1)
78-88: Consider using structured logging instead of console.error.The throttled presence update implementation looks correct, with appropriate throttle options for periodic updates. However, the error handling uses
console.errorwhich may not integrate with the application's logging system.If the codebase has a structured logging service or logger, consider using it instead of
console.errorfor consistency:- void Presence.updateConnection(this.userId, this.connection.id).catch((err) => { - console.error('Error updating connection presence:', err); - }); + void Presence.updateConnection(this.userId, this.connection.id).catch((err) => { + logger.error('Error updating connection presence:', err); + });
📜 Review details
Configuration used: Organization 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)
ee/apps/ddp-streamer/src/Client.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
ee/apps/ddp-streamer/src/Client.ts
🧬 Code graph analysis (1)
ee/apps/ddp-streamer/src/Client.ts (1)
ee/packages/presence/src/Presence.ts (1)
Presence(12-334)
⏰ 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 (1)
ee/apps/ddp-streamer/src/Client.ts (1)
217-217: LGTM! Throttled presence update integration is correct.The placement of
this.updatePresence()on every incoming message, combined with the throttle mechanism, ensures that active connections have their presence periodically refreshed without overwhelming the presence service. The throttle'sleading: trueconfiguration guarantees that the first message after each throttle period triggers an immediate update, which aligns with the PR objective of preventing active users from being marked offline.
cardoso
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
|
/bark |
|
/patch |
|
Pull request #37887 added to Project: "Patch 7.13.2" |
|
/backport 7.12.3 |
|
/backport 7.11.3 |
|
/backport 7.10.6 |
|
Pull request #37888 added to Project: "Patch 7.12.3" |
|
Pull request #37889 added to Project: "Patch 7.11.3" |
|
Pull request #37890 added to Project: "Patch 7.10.6" |
4 participants
Regression from #37551
Proposed changes (including videos or screenshots)
Clientpresence update mechanism to use a throttle-based approach instead of interval-based heartbeat with flag tracking:_seenPacketflag andheartbeatIntervallogic with a throttledupdatePresencemethodunderscore'sthrottleutility with{ leading: true, trailing: false }to ensure presence updates occur at most once perTIMEOUT(30 seconds)UsersSessions.connections._updatedAtis periodically refreshed for active DDPStreamer connections, preventing the PresenceReaper from incorrectly pruning them as stale.Issue(s)
Fixes https://rocketchat.atlassian.net/browse/CORE-1568
Steps to test or reproduce
usersSessionsand observe theirconnectionsarray:connections._updatedAtstops changing after the initial login.connections._updatedAtis bumped at most once per 30 seconds while the user is active.Further comments
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.