fix: message being marked as sent before the request completes#39003
Merged
fix: message being marked as sent before the request completes#39003
Conversation
Contributor
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: ef5c372 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
WalkthroughRefactors client message send flow: moves preprocessing (reactions, length, slash commands, editing) to the flow layer, normalizes flags before send, returns callback results from low-level sendMessage, and removes local temporary-message state updates after successful send. Changes
Sequence DiagramsequenceDiagram
participant User
participant Flow as Client Flow
participant Proc as Processors
participant SDK as sendMessage SDK
participant Callbacks as Callbacks
participant Store as Messages Store
User->>Flow: send message
Flow->>Proc: processSetReaction(message)
alt reaction handled
Proc-->>Flow: true (exit)
Flow-->>User: return
end
Flow->>Proc: processTooLongMessage(message)
alt too long
Proc-->>Flow: true (exit)
Flow-->>User: return
end
Flow->>Proc: processSlashCommand(message)
alt slash command handled
Proc-->>Flow: true (exit)
Flow-->>User: return
end
Flow->>Proc: onClientBeforeSendMessage(normalize)
Proc-->>Flow: normalized message
Flow->>Proc: processMessageEditing(message)
alt editing handled
Proc-->>Flow: true (exit)
Flow-->>User: return
end
Flow->>SDK: sendMessage(message)
SDK->>Callbacks: clientCallbacks.run('afterSaveMessage', ...)
Callbacks-->>SDK: result
SDK-->>Flow: success
Flow->>Store: remove temporary flag from message
Flow-->>User: message sent
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/client/lib/chats/flows/sendMessage.ts`:
- Around line 40-45: Remove the inline comment above the Messages.state.update
call — the implementation must not contain code comments; locate the block that
calls Messages.state.update (the predicate using record._id === message._id &&
record.temp === true and the updater ({ temp: _, ...record }) => record) and
delete the comment line ("// after the request is complete we can go ahead and
mark as sent") so only the imperative code remains, ensuring no other inline
comments are left in that function.
- Around line 39-45: The temp record is left as temp:true when
sdk.call('sendMessage', message, previewUrls) throws; wrap the call in a
try/catch inside the process() flow and on error remove or mark the local stub
created earlier by using Messages.state.update to target record._id ===
message._id && record.temp === true and either delete it or set temp:false plus
a failure flag; ensure the catch also re-throws or surfaces the error for
existing error handling so UI shows the toast while local state is cleaned up.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/meteor/app/lib/client/methods/sendMessage.tsapps/meteor/client/lib/chats/flows/sendMessage.ts
📜 Review details
⏰ 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: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 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/client/lib/chats/flows/sendMessage.tsapps/meteor/app/lib/client/methods/sendMessage.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:06.802Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
📚 Learning: 2026-02-10T16:32:42.586Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:42.586Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON and Date fields arrive as Date objects; do not manually construct new Date() in stream handlers (for example, in sdk.stream()). Only REST API responses return plain JSON where dates are strings, so implement explicit conversion there if needed. Apply this guidance to all TypeScript files under apps/meteor/client to ensure consistent date handling in DDP streams and REST responses.
Applied to files:
apps/meteor/client/lib/chats/flows/sendMessage.ts
📚 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: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/lib/client/methods/sendMessage.ts
📚 Learning: 2026-02-20T09:04:55.725Z
Learnt from: Shreyas2004wagh
Repo: RocketChat/Rocket.Chat PR: 38681
File: apps/meteor/server/modules/streamer/streamer.module.ts:307-313
Timestamp: 2026-02-20T09:04:55.725Z
Learning: In apps/meteor/server/modules/streamer/streamer.module.ts, the catch block in sendToManySubscriptions intentionally uses SystemLogger.debug (not error or warn) for per-subscription delivery failures to keep logs less noisy, as this was a deliberate design choice reviewed and approved by KevLehman.
Applied to files:
apps/meteor/app/lib/client/methods/sendMessage.ts
🔇 Additional comments (2)
apps/meteor/client/lib/chats/flows/sendMessage.ts (1)
3-9: LGTM — imports are correctly placed and all consumed.apps/meteor/app/lib/client/methods/sendMessage.ts (1)
45-48: Verify thatafterSaveMessagehandlers tolerate messages withtemp: true.The
afterSaveMessagecallback now runs while the message still carriestemp: true. Specifically, the analytics event tracker logs all messages as "sent" regardless of confirmation status, and thread scroll managers may operate on unconfirmed messages. Ensure registered handlers either filter temp messages or handle them safely.
Comment on lines
39
to
+45
| await sdk.call('sendMessage', message, previewUrls); | ||
|
|
||
| // after the request is complete we can go ahead and mark as sent | ||
| Messages.state.update( | ||
| (record) => record._id === message._id && record.temp === true, | ||
| ({ temp: _, ...record }) => record, | ||
| ); |
Contributor
There was a problem hiding this comment.
Temp record is never cleaned up when sdk.call throws.
The Meteor method stub (in sendMessage.ts) stores the message with temp: true before the DDP round-trip. If sdk.call rejects (network error, server error, rate-limit, etc.) the exception propagates out of process(), the Messages.state.update is skipped, and the record remains stuck with temp: true in local state until a page reload — the error toast is shown but the message continues to appear as "sending" with no failure indication.
Consider cleaning up the orphaned record in an error branch, e.g.:
💡 Suggested approach
- await sdk.call('sendMessage', message, previewUrls);
-
- Messages.state.update(
- (record) => record._id === message._id && record.temp === true,
- ({ temp: _, ...record }) => record,
- );
+ try {
+ await sdk.call('sendMessage', message, previewUrls);
+ Messages.state.update(
+ (record) => record._id === message._id && record.temp === true,
+ ({ temp: _, ...record }) => record,
+ );
+ } catch (error) {
+ Messages.state.delete(message._id);
+ throw error;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await sdk.call('sendMessage', message, previewUrls); | |
| // after the request is complete we can go ahead and mark as sent | |
| Messages.state.update( | |
| (record) => record._id === message._id && record.temp === true, | |
| ({ temp: _, ...record }) => record, | |
| ); | |
| try { | |
| await sdk.call('sendMessage', message, previewUrls); | |
| // after the request is complete we can go ahead and mark as sent | |
| Messages.state.update( | |
| (record) => record._id === message._id && record.temp === true, | |
| ({ temp: _, ...record }) => record, | |
| ); | |
| } catch (error) { | |
| Messages.state.delete(message._id); | |
| throw error; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/client/lib/chats/flows/sendMessage.ts` around lines 39 - 45, The
temp record is left as temp:true when sdk.call('sendMessage', message,
previewUrls) throws; wrap the call in a try/catch inside the process() flow and
on error remove or mark the local stub created earlier by using
Messages.state.update to target record._id === message._id && record.temp ===
true and either delete it or set temp:false plus a failure flag; ensure the
catch also re-throws or surfaces the error for existing error handling so UI
shows the toast while local state is cleaned up.
Member
Author
There was a problem hiding this comment.
in case sdk.call throws it is ok to keep as temp: true, we'd actually to add a new UI to handle a retry scenario
Contributor
There was a problem hiding this comment.
@sampaiodiego Got it! Keeping temp: true on failure makes sense for a retry UI. That will provide a much better user experience than losing the message. Thank you for clarifying the intent!
✏️ Learnings added
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 39003
File: apps/meteor/client/lib/chats/flows/sendMessage.ts:39-45
Timestamp: 2026-02-24T19:16:32.051Z
Learning: In apps/meteor/client/lib/chats/flows/sendMessage.ts, when sdk.call('sendMessage', ...) throws an error, the message is intentionally left with temp: true (not deleted or cleaned up) to support a future retry UI feature. This allows users to retry sending failed messages rather than losing them.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 32703
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:52-58
Timestamp: 2026-02-12T15:39:28.416Z
Learning: In `apps/meteor/client/lib/chats/flows/uploadFiles.ts`, when E2E encryption is required but not allowed (e.g., `E2E_Enable_Encrypt_Files` setting is disabled), the function intentionally abandons the entire upload queue and displays a toast error. This fail-fast behavior prevents partial uploads when encryption requirements cannot be met and is the expected behavior, not a bug.
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
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.
Learnt from: Shreyas2004wagh
Repo: RocketChat/Rocket.Chat PR: 38681
File: apps/meteor/server/modules/streamer/streamer.module.ts:307-313
Timestamp: 2026-02-20T09:04:55.725Z
Learning: In apps/meteor/server/modules/streamer/streamer.module.ts, the catch block in sendToManySubscriptions intentionally uses SystemLogger.debug (not error or warn) for per-subscription delivery failures to keep logs less noisy, as this was a deliberate design choice reviewed and approved by KevLehman.
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:06.802Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:42.586Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON and Date fields arrive as Date objects; do not manually construct new Date() in stream handlers (for example, in sdk.stream()). Only REST API responses return plain JSON where dates are strings, so implement explicit conversion there if needed. Apply this guidance to all TypeScript files under apps/meteor/client to ensure consistent date handling in DDP streams and REST responses.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #39003 +/- ##
===========================================
- Coverage 70.65% 70.64% -0.01%
===========================================
Files 3189 3189
Lines 112716 112712 -4
Branches 20456 20386 -70
===========================================
- Hits 79635 79623 -12
- Misses 31033 31039 +6
- Partials 2048 2050 +2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
sampaiodiego
changed the title
sampaiodiego
changed the title
sampaiodiego
force-pushed
the
regression-mark-message-sent
branch
from
235aa09 to
ef5c372
Compare
ggazzo
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
ggazzo
approved these changes
Feb 25, 2026
Member
|
/jira ARCH-1935 |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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)
The previous code was running inside a method stub, that means it runs every before the actual request to
sendMessageis sent. With this change it is now properly waiting the request to complete to remove thetempproperty from the message.The only caveat here is that the previous behavior is still in place which means if we receive the new message via websockets even before the HTTP request finishes, the message will be marked as sent as well (this is not a really problem, just makes things potentially a bit inconsistent). this is caused by this code:
Rocket.Chat/apps/meteor/app/ui-utils/client/lib/LegacyRoomManager.ts
Line 191 in a54c020
Issue(s)
Introduced by #38067
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Bug Fixes
Refactor
Chores
Task: ARCH-1996