fix: Remove the delay of waiting on event on WS to consider a message sent #38067
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
|
WalkthroughsendMessage stores the message, then invokes Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client.sendMessage
participant Receiver as onClientMessageReceived
participant Store as Messages.state
participant Callbacks as clientCallbacks.run
Client->>Receiver: send message / receive processed message
Receiver-->>Store: return message
Store->>Store: store message (Messages.state.store)
Store->>Callbacks: invoke afterSaveMessage(message) (fire-and-forget)
Note right of Callbacks `#f8f0d6`: callback runs asynchronously\n(not awaited by sender)
Store->>Store: update record where _id==message._id && temp==true → remove temp
Note right of Store `#e8f6f3`: temp flag removed immediately after storing\nand after callback is invoked
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
🧠 Learnings (1)📚 Learning: 2025-11-19T18:20:37.116ZApplied to files:
🧬 Code graph analysis (1)apps/meteor/app/lib/client/methods/sendMessage.ts (1)
⏰ 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)
🔇 Additional comments (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 |
geekgonecrazy
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @apps/meteor/app/lib/client/methods/sendMessage.ts:
- Around line 45-53: The UI is delayed marking messages as sent because the code
awaits clientCallbacks.run('afterSaveMessage', ...) before clearing the temp
flag; move the Messages.state.update(...) call that clears temp to immediately
after Messages.state.store(message) and before awaiting
onClientMessageReceived(...). Concretely: after calling
Messages.state.store(message) perform
Messages.state.update((r)=>r._id===message._id&&r.temp===true, (r)=>({...r,
temp:false})), then await clientCallbacks.run('afterSaveMessage', message, {
room, user }) so side-effect callbacks run without delaying the sent-state
update.
In @apps/meteor/app/ui-utils/client/lib/LegacyRoomManager.ts:
- Line 191: In LegacyRoomManager (in the method containing the orphaned
expression "{ _id: msg._id, temp: { $ne: true } });" remove the dead/orphaned
object expression statement; it serves no purpose—either delete the line
entirely or replace it with the intended check/usage (e.g., use it in a
conditional or pass it to a query) so no unused expression remains.
🧹 Nitpick comments (2)
apps/meteor/app/ui-utils/client/lib/LegacyRoomManager.ts (1)
180-189: Delay logging improves observability.The message receive delay logging aligns well with the PR objective to make message send times visible via metrics. The logic correctly calculates the delay and logs warnings for significant delays.
Optional: Consider making the threshold configurable
The 2000ms threshold is hardcoded. For future flexibility, consider making it configurable via settings:
- // Log warning if delay is significant (>2 seconds) - if (receiveDelay > 2000) { - console.warn(`[Message Delivery] High delay detected: ${receiveDelay}ms. Possible network or backend issue.`); - } + const delayThreshold = getConfig('messageDelayThreshold') ?? '2000'; + if (receiveDelay > parseInt(delayThreshold)) { + console.warn(`[Message Delivery] High delay detected: ${receiveDelay}ms. Possible network or backend issue.`); + }apps/meteor/app/lib/client/methods/sendMessage.ts (1)
45-54: Add error handling for failed message sends.The current implementation lacks visible error handling. If
onClientMessageReceivedorafterSaveMessagefails, the message will remain withtemp: trueindefinitely, leaving users with a message stuck in "sending" state.🔎 Suggested error handling
await onClientMessageReceived(message as IMessage).then(async (message) => { Messages.state.store(message); await clientCallbacks.run('afterSaveMessage', message, { room, user }); // Now that the message is stored, we can go ahead and mark as sent Messages.state.update( (record) => record._id === message._id && record.temp === true, (record) => ({ ...record, temp: false }), ); + }).catch((error) => { + // Remove failed temporary message or mark as failed + Messages.state.delete(message._id); + dispatchToastMessage({ type: 'error', message: t('Message_Send_Failed') }); + throw error; });
📜 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 (3)
apps/meteor/app/lib/client/methods/sendMessage.tsapps/meteor/app/ui-utils/client/lib/LegacyRoomManager.tsapps/meteor/client/stores/Messages.ts
🧰 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:
apps/meteor/app/ui-utils/client/lib/LegacyRoomManager.tsapps/meteor/app/lib/client/methods/sendMessage.tsapps/meteor/client/stores/Messages.ts
🧠 Learnings (1)
📚 Learning: 2025-11-19T18:20:37.116Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.
Applied to files:
apps/meteor/app/lib/client/methods/sendMessage.ts
🧬 Code graph analysis (2)
apps/meteor/app/lib/client/methods/sendMessage.ts (1)
apps/meteor/client/stores/Messages.ts (1)
Messages(7-15)
apps/meteor/client/stores/Messages.ts (1)
apps/meteor/client/lib/cachedStores/DocumentMapStore.ts (1)
createDocumentMapStore(169-308)
⏰ 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). (5)
- GitHub Check: check
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
apps/meteor/client/stores/Messages.ts (1)
9-13: Type extension correctly supports temporary message feature.The addition of the
temp?: booleanfield to the Messages store type enables the optimistic UI update pattern described in the PR. The type definition is clean and consistent with existing optional fields.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38067 +/- ##
===========================================
+ Coverage 70.62% 70.65% +0.02%
===========================================
Files 3143 3143
Lines 108664 108677 +13
Branches 19539 19600 +61
===========================================
+ Hits 76740 76781 +41
+ Misses 29910 29894 -16
+ Partials 2014 2002 -12
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/07 05:49 (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, 07 Jan 2026 05:49:23 GMT |
geekgonecrazy
force-pushed
the
fix/remove-unneeded-msg-delay
branch
from
3fa9726 to
9bd97e2
Compare
geekgonecrazy
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/app/ui-utils/client/lib/LegacyRoomManager.ts (1)
181-181: Remove inline comment.Per coding guidelines: "Avoid code comments in the implementation". The code is self-explanatory.
🔎 Proposed change
- // Measure and log message receive delay for messages if (msg.ts) {
📜 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 (3)
apps/meteor/app/lib/client/methods/sendMessage.tsapps/meteor/app/ui-utils/client/lib/LegacyRoomManager.tsapps/meteor/client/stores/Messages.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/client/stores/Messages.ts
- apps/meteor/app/lib/client/methods/sendMessage.ts
🧰 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:
apps/meteor/app/ui-utils/client/lib/LegacyRoomManager.ts
🧠 Learnings (2)
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/app/ui-utils/client/lib/LegacyRoomManager.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
apps/meteor/app/ui-utils/client/lib/LegacyRoomManager.ts
⏰ 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
geekgonecrazy
force-pushed
the
fix/remove-unneeded-msg-delay
branch
from
9bd97e2 to
d0d1f56
Compare
tassoevan
force-pushed
the
fix/remove-unneeded-msg-delay
branch
from
d0d1f56 to
04ae8fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/app/lib/client/methods/sendMessage.ts (1)
45-53: Implementation correctly awaits afterSaveMessage before marking message as sent.The async/await pattern is properly implemented, and the state update correctly targets the temporary message by both
_idandtemp === true. This approach respects the callback execution order that was discussed in previous reviews.Optional: Consider error recovery for robustness.
If
clientCallbacks.run('afterSaveMessage', ...)throws an error, the message will remain withtemp: trueindefinitely, leaving it in a "sending" state in the UI. For better user experience, you might consider wrapping the callback in a try-catch to ensure the message is still marked as sent even if analytics or other side effects fail:Optional refactor for error resilience
await onClientMessageReceived(message as IMessage).then(async (message) => { Messages.state.store(message); - await clientCallbacks.run('afterSaveMessage', message, { room, user }); - - // Now that the message is stored, we can go ahead and mark as sent - Messages.state.update( - (record) => record._id === message._id && record.temp === true, - (record) => ({ ...record, temp: false }), - ); + + try { + await clientCallbacks.run('afterSaveMessage', message, { room, user }); + } catch (error) { + console.error('Error in afterSaveMessage callbacks:', error); + } finally { + // Mark as sent even if callbacks fail + Messages.state.update( + (record) => record._id === message._id && record.temp === true, + (record) => ({ ...record, temp: false }), + ); + } });This is optional and depends on whether callback failures should prevent the message from being marked as sent. The current implementation may be intentional if callback success is required before showing the message as delivered.
📜 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 (3)
apps/meteor/app/lib/client/methods/sendMessage.tsapps/meteor/app/ui-utils/client/lib/LegacyRoomManager.tsapps/meteor/client/stores/Messages.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/client/stores/Messages.ts
🧰 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:
apps/meteor/app/lib/client/methods/sendMessage.tsapps/meteor/app/ui-utils/client/lib/LegacyRoomManager.ts
🧠 Learnings (6)
📚 Learning: 2025-11-19T18:20:37.116Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.
Applied to files:
apps/meteor/app/lib/client/methods/sendMessage.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/app/ui-utils/client/lib/LegacyRoomManager.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
apps/meteor/app/ui-utils/client/lib/LegacyRoomManager.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/app/ui-utils/client/lib/LegacyRoomManager.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/app/ui-utils/client/lib/LegacyRoomManager.ts
📚 Learning: 2025-11-27T17:56:26.050Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
Applied to files:
apps/meteor/app/ui-utils/client/lib/LegacyRoomManager.ts
🧬 Code graph analysis (1)
apps/meteor/app/lib/client/methods/sendMessage.ts (1)
apps/meteor/client/stores/Messages.ts (1)
Messages(7-17)
⏰ 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). (7)
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 📦 Meteor Build (coverage)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
apps/meteor/app/ui-utils/client/lib/LegacyRoomManager.ts (1)
181-189: LGTM! Message receive delay logging is well-implemented.The delay measurement logic is straightforward and achieves the PR objective of making message delivery latency observable. The 2000ms threshold is reasonable for detecting network or backend issues, and the warning message is clear and actionable.
tassoevan
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
Currently to consider a message sent we wait on the message to be received over websocket. This adds a perceived delay that is constantly reported and cannot be visualized in metrics. By removing this we now show a message sent as soon as the backend returns successful.
This now allows us to see message send times by the method.call/sendMessage api response times. If these elevate from an operations perspective we can clearly see there is an issue.
Issue(s)
Steps to test or reproduce
Further comments
Next up would be metrics to let us see if there are delays in event propagation. That is coming in a seperate changeset.
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.